automatically upload a file when it is put in a given local directory #1429

New Issue

tahoe-lafs · 2011-07-16T16:27:19Z

davidsarah commented

2011-07-16 16:27:19 +00:00

During the Tahoe-LAFS summit, I (David-Sarah) implemented a prototype of a dropbox-like uploader: if you write a file into a given directory, it will upload it to a given Tahoe mutable directory (with the same name as its name in the local filesystem).

Its current limitations are:

it only handles one directory, not including subdirectories
~~the behaviour when multiple files are put in the directory (or the same file more than once) has not been well-tested so far~~
~~it doesn't have unit tests or docs (I will write those)~~
it must be given the URI of a mutable directory to upload to; it would be more usable to allow that to be a path that may start with an alias
it uses the twisted.internet.inotify API, which depends on Twisted 10.1 and is only supported on platforms with inotify.

During the Tahoe-LAFS summit, I (David-Sarah) implemented a prototype of a dropbox-like uploader: if you write a file into a given directory, it will upload it to a given Tahoe mutable directory (with the same name as its name in the local filesystem). Its current limitations are: * it only handles one directory, not including subdirectories * ~~the behaviour when multiple files are put in the directory (or the same file more than once) has not been well-tested so far~~ * ~~it doesn't have unit tests or docs (I will write those)~~ * it must be given the URI of a mutable directory to upload to; it would be more usable to allow that to be a path that may start with an alias * it uses the [twisted.internet.inotify](http://twistedmatrix.com/documents/10.1.0/api/twisted.internet.inotify.INotify.html) API, which depends on Twisted 10.1 and is only supported on platforms with inotify.

tahoe-lafs added the

labels 2011-07-16 16:27:19 +00:00

tahoe-lafs added this to the soon milestone 2011-07-16 16:27:19 +00:00

davidsarah commented

2011-07-16 16:44:07 +00:00

Attachment drop-upload.darcs.patch (18988 bytes) added

Prototype implementation of drop-upload from Tahoe-LAFS summit. No tests or docs. (Corrected to include new file.)

**Attachment** drop-upload.darcs.patch (18988 bytes) added Prototype implementation of drop-upload from Tahoe-LAFS summit. No tests or docs. (Corrected to include new file.)

drop-upload.darcs.patch

18 KiB

davidsarah commented

2011-07-19 00:54:15 +00:00

(http://tahoe-lafs.org/trac/tahoe-lafs/attachment/ticket/1435/dependency-updates.darcs.patch) updates the dependency for Twisted to 10.1, which ensures that twisted.internet.inotify is available. (Note that it conflicts with drop-upload.darcs.patch. I'll rebase the latter when I have tests and docs.)

(http://tahoe-lafs.org/trac/tahoe-lafs/attachment/ticket/1435/dependency-updates.darcs.patch) updates the dependency for Twisted to 10.1, which ensures that `twisted.internet.inotify` is available. (Note that it conflicts with [drop-upload.darcs.patch](/tahoe-lafs/trac-2024-07-25/attachments/000078ac-7801-beec-42de-c34afc8812bc). I'll rebase the latter when I have tests and docs.)

david-sarah@jacaranda.org commented

2011-07-22 05:26:54 +00:00

In changeset:8b4082677477daf1:

Update the dependency on Twisted to >= 10.1. This allows us to simplify some documentation: it's no longer necessary to install pywin32 on Windows, or apply a patch to Twisted in order to use the FTP frontend. fixes #1274, #1438. refs #1429

In changeset:8b4082677477daf1: ``` Update the dependency on Twisted to >= 10.1. This allows us to simplify some documentation: it's no longer necessary to install pywin32 on Windows, or apply a patch to Twisted in order to use the FTP frontend. fixes #1274, #1438. refs #1429 ```

zooko commented

2011-07-22 20:40:32 +00:00

Here is a review of drop-upload.darcs.patch .

First of all: yay! This feature is neat. :-)

What is the "FIXME: Unicode paths"? I don't see what needs to change there to handle non-ASCII paths.
Why are we catching Exception--was it just because we might have Twisted < 10.1? We should remove any checks for if we have Twisted < 10.1 (except, of course, for the checks we do for all dependencies in source:src/allmydata/init.py).
"FIXME: error reporting" sounds like a serious incomplete part, but what exactly should be added? Logging to the Tahoe-LAFS logging system of certain sorts of failures? What could fail here?
Perhaps the logging in _notify() is too verbose.
the "dump event to file" part of _notify() is just for debugging and should be removed.
"FIXME: what happens if we get another event for the same file while it is uploading?" Well, I looked at the code of dirnode.add_file() and Uploader.upload() and I guess it just redundantly uploads it a second time. (See also #952, which is about what happens when you upload the bytewise-identical file more than once at a time through the WAPI.) What should it do? Well, what should the user experience look like? You drag and drop a file into this directory, and then you drag and drop a new version of it one second later, and then what? Maybe the visual display of the directory should have been updated as soon as you dropped the first one to show that it is in the middle of uploading that file. Then, if you drop another version on it, the display should update to show that there is another update scheduled to happen after this one completes. So I guess we need a new ticket for a GUI frontend to this functionality (integrated into some extant GUI like Nautilus, I suppose). But at this layer, I guess what it should do is schedule another upload for as soon as this upload is finished unless another such upload is already scheduled, in which case it should do nothing.
"FIXME: should all notified events cause an upload?": from http://www.kernel.org/doc/man-pages/online/pages/man7/inotify.7.html and http://inotify.aiken.cz/?section=inotify&page=faq&lang=en it looks like we should respond to IN_CLOSE_WRITE | IN_CREATE | IN_MOVE_SELF | IN_MOVE_TO
Also of course the other FIXMEs deserve to be investigated more. :-)

Other than that, it looks good. It is neat how few lines of code are needed to add this functionality! :-)

Here is a review of [drop-upload.darcs.patch](/tahoe-lafs/trac-2024-07-25/attachments/000078ac-7801-beec-42de-c34afc8812bc) . First of all: yay! This feature is neat. :-) * What is the "FIXME: Unicode paths"? I don't see what needs to change there to handle non-ASCII paths. * Why are we catching `Exception`--was it just because we might have Twisted < 10.1? We should remove any checks for if we have Twisted < 10.1 (except, of course, for the checks we do for all dependencies in source:src/allmydata/*init*.py). * "FIXME: error reporting" sounds like a serious incomplete part, but what exactly should be added? Logging to the Tahoe-LAFS logging system of certain sorts of failures? What could fail here? * Perhaps the logging in _notify() is too verbose. * the "dump event to file" part of _notify() is just for debugging and should be removed. * "FIXME: what happens if we get another event for the same file while it is uploading?" Well, I looked at the code of `dirnode.add_file()` and `Uploader.upload()` and I guess it just redundantly uploads it a second time. (See also #952, which is about what happens when you upload the bytewise-identical file more than once at a time through the WAPI.) What *should* it do? Well, what should the user experience look like? You drag and drop a file into this directory, and then you drag and drop a new version of it one second later, and then what? Maybe the visual display of the directory should have been updated as soon as you dropped the first one to show that it is in the middle of uploading that file. Then, if you drop another version on it, the display should update to show that there is another update scheduled to happen after this one completes. So I guess we need a new ticket for a GUI frontend to this functionality (integrated into some extant GUI like Nautilus, I suppose). But at this layer, I guess what it should do is schedule another upload for as soon as this upload is finished *unless* another such upload is already scheduled, in which case it should do nothing. * "FIXME: should all notified events cause an upload?": from <http://www.kernel.org/doc/man-pages/online/pages/man7/inotify.7.html> and <http://inotify.aiken.cz/?section=inotify&page=faq&lang=en> it looks like we should respond to `IN_CLOSE_WRITE | IN_CREATE | IN_MOVE_SELF | IN_MOVE_TO` * Also of course the other FIXMEs deserve to be investigated more. :-) Other than that, it looks good. It is neat how few lines of code are needed to add this functionality! :-)

davidsarah commented

2011-07-22 21:03:44 +00:00

Replying to zooko:

Here is a review of drop-upload.darcs.patch .

First of all: yay! This feature is neat. :-)

Thanks :-)

What is the "FIXME: Unicode paths"? I don't see what needs to change there to handle non-ASCII paths.

tahoe.cfg is encoded in UTF-8, but the path wants to be in sys.getfilesystemencoding().

Why are we catching Exception--was it just because we might have Twisted < 10.1? We should remove any checks for if we have Twisted < 10.1 (except, of course, for the checks we do for all dependencies in [source:src/allmydata/init.py]).

If there is any exception here, the node will silently fail to start. (The same is true of exceptions during initialization of the other frontends.) We should fix that in a better way -- it seems relevant to #355 and #1360 for example -- but I wanted to avoid this failure mode while I was debugging.

"FIXME: error reporting" sounds like a serious incomplete part, but what exactly should be added? Logging to the Tahoe-LAFS logging system of certain sorts of failures? What could fail here?

We should raise an exception with a proper message (and ensure that gets logged) rather than failing an assert.

Perhaps the logging in _notify() is too verbose.

the "dump event to file" part of _notify() is just for debugging and should be removed.

Yes, it was just easier to see the events that way while prototyping. Logging to the Twisted log is sufficient, and the message could be shorter.

"FIXME: what happens if we get another event for the same file while it is uploading?" Well, I looked at the code of dirnode.add_file() and Uploader.upload() and I guess it just redundantly uploads it a second time.

Yes, I think this is fairly harmless, modulo the fact that we should probably be using a more restrictive event mask.

(See also #952, which is about what happens when you upload the bytewise-identical file more than once at a time through the WAPI.) What ''should'' it do? Well, what should the user experience look like? You drag and drop a file into this directory, and then you drag and drop a new version of it one second later, and then what? Maybe the visual display of the directory should have been updated as soon as you dropped the first one to show that it is in the middle of uploading that file. Then, if you drop another version on it, the display should update to show that there is another update scheduled to happen after this one completes. So I guess we need a new ticket for a GUI frontend to this functionality (integrated into some extant GUI like Nautilus, I suppose). But at this layer, I guess what it should do is schedule another upload for as soon as this upload is finished ''unless'' another such upload is already scheduled, in which case it should do nothing.

Right, I'll open another ticket about suppressing redundant uploads.

"FIXME: should all notified events cause an upload?": from http://www.kernel.org/doc/man-pages/online/pages/man7/inotify.7.html and http://inotify.aiken.cz/?section=inotify&page=faq&lang=en it looks like we should respond to IN_CLOSE_WRITE | IN_CREATE | IN_MOVE_SELF | IN_MOVE_TO

That looks about right, although IN_MOVE_SELF should be treated differently, since it's moving the local directory away from its current path. I don't know what that should do -- probably the safest thing is to disable the drop-upload feature entirely until the node is restarted (and say "don't do that" in the docs).

Also of course the other FIXMEs deserve to be investigated more. :-)

Other than that, it looks good. It is neat how few lines of code are needed to add this functionality! :-)

Yeah, twisted.internet.inotify rocks!

Replying to [zooko](/tahoe-lafs/trac-2024-07-25/issues/1429#issuecomment-84018): > Here is a review of [drop-upload.darcs.patch](/tahoe-lafs/trac-2024-07-25/attachments/000078ac-7801-beec-42de-c34afc8812bc) . > > First of all: yay! This feature is neat. :-) Thanks :-) > * What is the "FIXME: Unicode paths"? I don't see what needs to change there to handle non-ASCII paths. `tahoe.cfg` is encoded in UTF-8, but the path wants to be in `sys.getfilesystemencoding()`. > * Why are we catching `Exception`--was it just because we might have Twisted < 10.1? We should remove any checks for if we have Twisted < 10.1 (except, of course, for the checks we do for all dependencies in [source:src/allmydata/__init__.py]). If there is any exception here, the node will silently fail to start. (The same is true of exceptions during initialization of the other frontends.) We should fix that in a better way -- it seems relevant to #355 and #1360 for example -- but I wanted to avoid this failure mode while I was debugging. > * "FIXME: error reporting" sounds like a serious incomplete part, but what exactly should be added? Logging to the Tahoe-LAFS logging system of certain sorts of failures? What could fail here? We should raise an exception with a proper message (and ensure that gets logged) rather than failing an assert. > * Perhaps the logging in _notify() is too verbose. > * the "dump event to file" part of _notify() is just for debugging and should be removed. Yes, it was just easier to see the events that way while prototyping. Logging to the Twisted log is sufficient, and the message could be shorter. > * "FIXME: what happens if we get another event for the same file while it is uploading?" Well, I looked at the code of `dirnode.add_file()` and `Uploader.upload()` and I guess it just redundantly uploads it a second time. Yes, I think this is fairly harmless, modulo the fact that we should probably be using a more restrictive event mask. > (See also #952, which is about what happens when you upload the bytewise-identical file more than once at a time through the WAPI.) What ''should'' it do? Well, what should the user experience look like? You drag and drop a file into this directory, and then you drag and drop a new version of it one second later, and then what? Maybe the visual display of the directory should have been updated as soon as you dropped the first one to show that it is in the middle of uploading that file. Then, if you drop another version on it, the display should update to show that there is another update scheduled to happen after this one completes. So I guess we need a new ticket for a GUI frontend to this functionality (integrated into some extant GUI like Nautilus, I suppose). But at this layer, I guess what it should do is schedule another upload for as soon as this upload is finished ''unless'' another such upload is already scheduled, in which case it should do nothing. Right, I'll open another ticket about suppressing redundant uploads. > * "FIXME: should all notified events cause an upload?": from http://www.kernel.org/doc/man-pages/online/pages/man7/inotify.7.html and http://inotify.aiken.cz/?section=inotify&page=faq&lang=en it looks like we should respond to `IN_CLOSE_WRITE | IN_CREATE | IN_MOVE_SELF | IN_MOVE_TO` That looks about right, although IN_MOVE_SELF should be treated differently, since it's moving the local directory away from its current path. I don't know what that should do -- probably the safest thing is to disable the drop-upload feature entirely until the node is restarted (and say "don't do that" in the docs). > * Also of course the other FIXMEs deserve to be investigated more. :-) > > Other than that, it looks good. It is neat how few lines of code are needed to add this functionality! :-) Yeah, `twisted.internet.inotify` rocks!

davidsarah commented

2011-07-23 04:52:05 +00:00

Replying to davidsarah:

Replying to zooko:

What is the "FIXME: Unicode paths"? I don't see what needs to change there to handle non-ASCII paths.

tahoe.cfg is encoded in UTF-8, but the path wants to be in sys.getfilesystemencoding().

Also, in _notify the conversion to a Tahoe Unicode filename is wrong (unicode(filepath.basename()) will use the default ASCII encoding, when it should be converting from sys.getfilesystemencoding()).

Replying to [davidsarah](/tahoe-lafs/trac-2024-07-25/issues/1429#issuecomment--1): > Replying to [zooko](/tahoe-lafs/trac-2024-07-25/issues/1429#issuecomment-84018): > > * What is the "FIXME: Unicode paths"? I don't see what needs to change there to handle non-ASCII paths. > > `tahoe.cfg` is encoded in UTF-8, but the path wants to be in `sys.getfilesystemencoding()`. Also, in `_notify` the conversion to a Tahoe Unicode filename is wrong (`unicode(filepath.basename())` will use the default ASCII encoding, when it should be converting from `sys.getfilesystemencoding()`).

davidsarah commented

2011-07-25 04:40:02 +00:00

Attachment drop-upload-2.darcs.patch (30700 bytes) added

Drop-upload frontend, with tests (but no documentation). refs #1429

**Attachment** drop-upload-2.darcs.patch (30700 bytes) added Drop-upload frontend, with tests (but no documentation). refs #1429

drop-upload-2.darcs.patch

30 KiB

davidsarah commented

2011-07-25 04:44:30 +00:00

The tests could do with covering a few more corner cases and error conditions, but I'm pretty sure there will be enough time for that before the beta.

tahoe-lafs modified the milestone from soon to 1.9.0

2011-07-25 04:44:30 +00:00

warner commented

2011-07-25 06:14:03 +00:00

This is pretty cool stuff. I'm hesitant about landing it in Tahoe core, though.. at least in its present form, it feels more like something that wants to be in a plugin, or in an extensions/ sort of directory. If it were a more complete Dropbox-ish replacment, I'd feel better about it: watching all directories under a root, handling modification of existing files not just new ones, and ideally some kind of multiple-client support. (as is, it's more like an automatic backup tool than a sync-a-virtual-directory-across-multiple-machines tool).

Can you envision maintaining its current UI for a couple years? Or do you think you'll look at it in six months and go "oh, that should really look like X instead".

This is pretty cool stuff. I'm hesitant about landing it in Tahoe core, though.. at least in its present form, it feels more like something that wants to be in a plugin, or in an extensions/ sort of directory. If it were a more complete Dropbox-ish replacment, I'd feel better about it: watching all directories under a root, handling modification of existing files not just new ones, and ideally some kind of multiple-client support. (as is, it's more like an automatic backup tool than a sync-a-virtual-directory-across-multiple-machines tool). Can you envision maintaining its current UI for a couple years? Or do you think you'll look at it in six months and go "oh, that should really look like X instead".

warner added

enhancement

and removed

defect

labels 2011-07-25 06:14:03 +00:00

davidsarah commented

2011-07-25 12:24:40 +00:00

Replying to warner:

This is pretty cool stuff. I'm hesitant about landing it in Tahoe core, though.. at least in its present form, it feels more like something that wants to be in a plugin, or in an extensions/ sort of directory.

We don't have a plugin mechanism. In any case, I'm not encouraged by the fate of things that were previously out-of-core, like the FUSE implementations. I'd prefer it to be in the core, tested by default, and available for all users to try out without installing anything else.

If it introduced new dependencies that would be a different matter, but it doesn't (and we'd decided that it was OK to depend on Twisted 10.1, which has other advantages).

If it were a more complete Dropbox-ish replacment, I'd feel better about it: watching all directories under a root, handling modification of existing files not just new ones, and ideally some kind of multiple-client support. (as is, it's more like an automatic backup tool than a sync-a-virtual-directory-across-multiple-machines tool).

I didn't intend it to be a sync-a-virtual-directory-across-multiple-machines tool. It just uploads things that you drop into a particular directory. Perhaps the name 'drop-upload' suggests that it is more similar to Dropbox than it is. We have time before the 1.9 release to rename it, if a better name is suggested.

It already handles modification of existing files (I'll add that to the tests). Watching all directories under a root is #1433, and is a relatively straightforward evolution of the current code.

Can you envision maintaining its current UI for a couple years?

Yes, absolutely. It'll be easy to maintain compatibility with the current UI, that's only a few lines of code and documentation. It also needs to be in core to get sufficient feedback on the UI to improve it.

Replying to [warner](/tahoe-lafs/trac-2024-07-25/issues/1429#issuecomment-84023): > This is pretty cool stuff. I'm hesitant about landing it in Tahoe core, though.. at least in its present form, it feels more like something that wants to be in a plugin, or in an extensions/ sort of directory. We don't have a plugin mechanism. In any case, I'm not encouraged by the fate of things that were previously out-of-core, like the FUSE implementations. I'd prefer it to be in the core, tested by default, and available for all users to try out without installing anything else. If it introduced *new* dependencies that would be a different matter, but it doesn't (and we'd decided that it was OK to depend on Twisted 10.1, which has other advantages). > If it were a more complete Dropbox-ish replacment, I'd feel better about it: watching all directories under a root, handling modification of existing files not just new ones, and ideally some kind of multiple-client support. (as is, it's more like an automatic backup tool than a sync-a-virtual-directory-across-multiple-machines tool). I didn't intend it to be a sync-a-virtual-directory-across-multiple-machines tool. It just uploads things that you drop into a particular directory. Perhaps the name 'drop-upload' suggests that it is more similar to Dropbox than it is. We have time before the 1.9 release to rename it, if a better name is suggested. It already handles modification of existing files (I'll add that to the tests). Watching all directories under a root is #1433, and is a relatively straightforward evolution of the current code. > Can you envision maintaining its current UI for a couple years? Yes, absolutely. It'll be easy to maintain compatibility with the current UI, that's only a few lines of code and documentation. It also needs to be in core to get sufficient feedback on the UI to improve it.

davidsarah commented

2011-07-27 01:08:57 +00:00

Attachment drop-upload-3.darcs.patch (34494 bytes) added

attachment:drop-upload-3.darcs.patch fixes some bugs in error paths, and has better test coverage. The tests also pass on Windows now.

**Attachment** drop-upload-3.darcs.patch (34494 bytes) added attachment:drop-upload-3.darcs.patch fixes some bugs in error paths, and has better test coverage. The tests also pass on Windows now.

drop-upload-3.darcs.patch

34 KiB

davidsarah commented

2011-07-27 03:31:46 +00:00

Attachment drop-upload-4.darcs.patch (42570 bytes) added

drop-upload: make counts visible on the statistics page, and disable some debugging. refs #1429

**Attachment** drop-upload-4.darcs.patch (42570 bytes) added drop-upload: make counts visible on the statistics page, and disable some debugging. refs #1429

drop-upload-4.darcs.patch

42 KiB

davidsarah commented

2011-07-31 19:09:55 +00:00

There are some lines > 80 characters in drop-upload-docs.darcs.patch; I'll rewrap those before committing it.

There are some lines > 80 characters in [drop-upload-docs.darcs.patch](/tahoe-lafs/trac-2024-07-25/attachments/000078ac-7801-beec-42de-0c166cefbaf0); I'll rewrap those before committing it.

nejucomo self-assigned this 2011-08-01 15:54:33 +00:00

nejucomo commented

2011-08-01 16:08:04 +00:00

I just read over these ticket comments and I'm about to review each patch in sequence. On success I can say: "This code makes sense to me and the tests work and the docs are clear.", but I cannot say: "We should / should not include this in trunk."

I will bug repository writers in IRC about that policy decision after reviewing the ticket, but before I change "review-needed" to "reviewed" to avoid an unintended 1.9 merge.

I just read over these ticket comments and I'm about to review each patch in sequence. On success I can say: "This code makes sense to me and the tests work and the docs are clear.", but I cannot say: "We should / should not include this in trunk." I will bug repository writers in IRC about that policy decision after reviewing the ticket, but before I change "review-needed" to "reviewed" to avoid an unintended 1.9 merge.

nejucomo commented

2011-08-01 16:40:55 +00:00

Attachment requirements-comment-merge.darcs.patch (20736 bytes) added

Fix requirement justification comments for Twisted >= 10.1.0

**Attachment** requirements-comment-merge.darcs.patch (20736 bytes) added Fix requirement justification comments for Twisted >= 10.1.0

requirements-comment-merge.darcs.patch

20 KiB

nejucomo commented

2011-08-01 16:48:45 +00:00

There's a comment collision for drop-upload.darcs.patch in _auto_deps.py which causes no change, just more justifications, for the requirement "Twisted>=10.1.0". I've attached requirements-comment-merge.darcs.patch with the merge of the requirements comments.

There's a comment collision for [drop-upload.darcs.patch](/tahoe-lafs/trac-2024-07-25/attachments/000078ac-7801-beec-42de-c34afc8812bc) in `_auto_deps.py` which causes no change, just more justifications, for the requirement "Twisted>=10.1.0". I've attached [requirements-comment-merge.darcs.patch](/tahoe-lafs/trac-2024-07-25/attachments/000078ac-7801-beec-42de-67b77e8a03e1) with the merge of the requirements comments.

nejucomo commented

2011-08-01 16:50:59 +00:00

When applying drop-upload-2.darcs.patch there are merge conflicts in ./src/allmydata/client.py, ./src/allmydata/scripts/create_node.py, and ./src/allmydata/test/test_runner.py.

I'm going to punt understanding and resolving these conflicts back to davidsarah and move on to reviewing other patches.

When applying [drop-upload-2.darcs.patch](/tahoe-lafs/trac-2024-07-25/attachments/000078ac-7801-beec-42de-4dc65d206b6f) there are merge conflicts in `./src/allmydata/client.py`, `./src/allmydata/scripts/create_node.py`, and `./src/allmydata/test/test_runner.py`. I'm going to punt understanding and resolving these conflicts back to davidsarah and move on to reviewing other patches.

davidsarah commented

2011-08-01 17:24:17 +00:00

drop-upload-2.darcs.patch is obsolete. Use drop-upload-4.darcs.patch .

[drop-upload-2.darcs.patch](/tahoe-lafs/trac-2024-07-25/attachments/000078ac-7801-beec-42de-4dc65d206b6f) is obsolete. Use [drop-upload-4.darcs.patch](/tahoe-lafs/trac-2024-07-25/attachments/000078ac-7801-beec-42de-e98c376bb7fb) .

davidsarah commented

2011-08-01 17:35:23 +00:00

The wording in requirements-comment-merge.darcs.patch isn't quite equivalent, because the inotify requirement is specific to Linux and the FTP server one is cross-platform. I think the wording as drop-upload-4.darcs.patch leaves it is fine.

The wording in [requirements-comment-merge.darcs.patch](/tahoe-lafs/trac-2024-07-25/attachments/000078ac-7801-beec-42de-67b77e8a03e1) isn't quite equivalent, because the inotify requirement is specific to Linux and the FTP server one is cross-platform. I think the wording as [drop-upload-4.darcs.patch](/tahoe-lafs/trac-2024-07-25/attachments/000078ac-7801-beec-42de-e98c376bb7fb) leaves it is fine.

nejucomo commented

2011-08-01 17:40:11 +00:00

I am resuming this ticket after I realized later patch attachments supercede earlier ones.

nejucomo commented

2011-08-01 17:50:58 +00:00

In the DropUploader constructor there may be a race condition between:

        if not self._local_path.isdir():
            raise AssertionError("The drop-upload local path %r was not an existing directory." % quote_output(local_dir))

-and later-

        self._notifier.watch(self._local_path, mask=mask, callbacks=[self._notify])

What happens if self._local_path is deleted between these lines?

Would it be better to catch and handle the error of self._local_path not existing in the call to .watch ? (Would that call signal the error?)

I do not consider this issue significant enough to require another iteration on this ticket; but a new ticket may be necessary. I haven't thought of any security problems as of yet, and only a rare usability problem.

In the `DropUploader` constructor there may be a race condition between: ``` if not self._local_path.isdir(): raise AssertionError("The drop-upload local path %r was not an existing directory." % quote_output(local_dir)) ``` -and later- ``` self._notifier.watch(self._local_path, mask=mask, callbacks=[self._notify]) ``` What happens if `self._local_path` is deleted between these lines? Would it be better to catch and handle the error of `self._local_path` not existing in the call to `.watch` ? (Would that call signal the error?) I do not consider this issue significant enough to require another iteration on this ticket; but a new ticket may be necessary. I haven't thought of any security problems as of yet, and only a rare usability problem.

davidsarah commented

2011-08-01 18:54:17 +00:00

Replying to nejucomo:

In the DropUploader constructor there may be a race condition between:
        if not self._local_path.isdir():
            raise [AssertionError](wiki/AssertionError)("The drop-upload local path %r was not an existing directory." % quote_output(local_dir))
-and later-
        self._notifier.watch(self._local_path, mask=mask, callbacks=self._notify)
What happens if self._local_path is deleted between these lines?

If the directory doesn't exist, the call to watch seems to succeed but not work as intended (even if the directory is later created). Similarly if the path points to a file rather than a directory. So the isdir() check is necessary for proper error reporting. In general I agree that it is "Better To Ask Forgiveness Than Permission" as opposed to "Looking Before You Leap", but it doesn't seem to be possible to do that here.

There is no security problem if the directory is deleted, that will just cause there to be no further notifications.

Replying to [nejucomo](/tahoe-lafs/trac-2024-07-25/issues/1429#issuecomment-84035): > In the `DropUploader` constructor there may be a race condition between: > > ``` > if not self._local_path.isdir(): > raise [AssertionError](wiki/AssertionError)("The drop-upload local path %r was not an existing directory." % quote_output(local_dir)) > ``` > > -and later- > > ``` > self._notifier.watch(self._local_path, mask=mask, callbacks=self._notify) > ``` > > What happens if `self._local_path` is deleted between these lines? If the directory doesn't exist, the call to `watch` seems to succeed but not work as intended (even if the directory is later created). Similarly if the path points to a file rather than a directory. So the `isdir()` check is necessary for proper error reporting. In general I agree that it is "Better To Ask Forgiveness Than Permission" as opposed to "Looking Before You Leap", but it doesn't seem to be possible to do that here. There is no security problem if the directory is deleted, that will just cause there to be no further notifications.

davidsarah commented

2011-08-01 18:59:30 +00:00

Reassigning to warner to decide whether this goes in the beta release.

nejucomo was unassigned by tahoe-lafs

2011-08-01 18:59:30 +00:00

warner was assigned by tahoe-lafs

2011-08-01 18:59:30 +00:00

nejucomo commented

2011-08-01 21:23:02 +00:00

I concur that this is reviewed. davidsarah answered the question about the _local_path race condition in IRC: The inotify interface will not indicate if the target directory does not exist.

It still may be possible to remove the race condition by calling isdir() after the call to watch() but this detail does not seem important enough to prevent inclusion.

I concur that this is reviewed. davidsarah answered the question about the `_local_path` race condition in IRC: The inotify interface will not indicate if the target directory does not exist. It still may be possible to remove the race condition by calling `isdir()` after the call to `watch()` but this detail does not seem important enough to prevent inclusion.

davidsarah commented

2011-08-02 00:18:17 +00:00

Proposed NEWS:

New Features
''''''''''''

- A "drop-upload" feature has been added, which allows you to upload
  files to a Tahoe-LAFS directory just by writing them to a local
  directory. This feature is experimental and should not be relied on
  to store the only copy of valuable data. It is currently available
  only on Linux. See `<docs/frontends/drop-upload.rst>`_ for
  documentation. (`#1429`_)

Compatibility and Dependencies
''''''''''''''''''''''''''''''

- The Twisted dependency has been raised to version 10.1. This ensures
  that we no longer require pywin32 on Windows even when using older
  versions of Twisted, that the new drop-upload feature has the required
  support from Twisted on Linux, and that it is never necessary to
  patch Twisted in order to use the FTP frontend.
  (`#1274`_, `#1429`_, `#1438`_)

Proposed NEWS: ``` New Features '''''''''''' - A "drop-upload" feature has been added, which allows you to upload files to a Tahoe-LAFS directory just by writing them to a local directory. This feature is experimental and should not be relied on to store the only copy of valuable data. It is currently available only on Linux. See `<docs/frontends/drop-upload.rst>`_ for documentation. (`#1429`_) ``` ``` Compatibility and Dependencies '''''''''''''''''''''''''''''' - The Twisted dependency has been raised to version 10.1. This ensures that we no longer require pywin32 on Windows even when using older versions of Twisted, that the new drop-upload feature has the required support from Twisted on Linux, and that it is never necessary to patch Twisted in order to use the FTP frontend. (`#1274`_, `#1429`_, `#1438`_) ```

zooko commented

2011-08-02 06:14:31 +00:00

Brian mentioned on IRC that he was uncertain about including this feature in 1.9. His reasoning persuaded me that we might want to be a bit conservative about distributing this feature in a way that makes people think of it as being a fully supported feature of Tahoe-LAFS. It is new and we haven't thought about it that much, and we don't want to take on the burden of backward compatibility for this feature the way we do for all of the supported features in Tahoe-LAFS.

I think maybe we should go ahead and include this feature in Tahoe-LAFS v1.9 but mark it as an experimental feature which we don't necessarily commit to supporting at this time.

Having a new feature in a release seems like a great way to learn about whether we want to promote it to a fully supported feature in a future release.

If this is agreeable to everyone, we should make sure to flag it as experimental in the release announcement and docs.

Brian mentioned on IRC that he was uncertain about including this feature in 1.9. His reasoning persuaded me that we might want to be a bit conservative about distributing this feature in a way that makes people think of it as being a fully supported feature of Tahoe-LAFS. It is new and we haven't thought about it that much, and we don't want to take on the burden of backward compatibility for this feature the way we do for all of the supported features in Tahoe-LAFS. I think maybe we should go ahead and include this feature in Tahoe-LAFS v1.9 but mark it as an experimental feature which we don't necessarily commit to supporting at this time. Having a new feature in a release seems like a great way to learn about whether we want to promote it to a fully supported feature in a future release. If this is agreeable to everyone, we should make sure to flag it as experimental in the release announcement and docs.

davidsarah commented

2011-08-08 02:53:13 +00:00

(http://tahoe-lafs.org/trac/tahoe-lafs/attachment/ticket/1431/drop-upload-docs-including-windows.darcs.patch) is an alternative docs patch if the Windows implementation is included in 1.9.

davidsarah commented

2011-08-08 19:29:14 +00:00

Attachment drop-upload-docs.darcs.patch (35586 bytes) added

Documentation for drop-upload frontend. refs #1429

**Attachment** drop-upload-docs.darcs.patch (35586 bytes) added Documentation for drop-upload frontend. refs #1429

drop-upload-docs.darcs.patch

35 KiB

davidsarah commented

2011-08-09 00:47:39 +00:00

Attachment drop-upload-5a.darcs.patch (50509 bytes) added

Drop-upload frontend, rerecorded for 1.9 beta (and correcting a minor mistake). Includes some fixes for Windows but not the Windows inotify implementation. fixes #1429

**Attachment** drop-upload-5a.darcs.patch (50509 bytes) added Drop-upload frontend, rerecorded for 1.9 beta (and correcting a minor mistake). Includes some fixes for Windows but not the Windows inotify implementation. fixes #1429

drop-upload-5a.darcs.patch

49 KiB

david-sarah@jacaranda.org commented

2011-08-09 00:52:24 +00:00

In changeset:32a7717205ed824a:

Drop-upload frontend, rerecorded for 1.9 beta (and correcting a minor mistake). Includes some fixes for Windows but not the Windows inotify implementation. fixes #1429

In changeset:32a7717205ed824a: ``` Drop-upload frontend, rerecorded for 1.9 beta (and correcting a minor mistake). Includes some fixes for Windows but not the Windows inotify implementation. fixes #1429 ```

tahoe-lafs added the

fixed

label 2011-08-09 00:52:24 +00:00

david-sarah@jacaranda.org closed this issue

2011-08-09 00:52:24 +00:00

david-sarah@jacaranda.org commented

2011-08-09 00:52:24 +00:00

In changeset:667b086b59ee37d3:

Documentation for drop-upload frontend. refs #1429

In changeset:667b086b59ee37d3: ``` Documentation for drop-upload frontend. refs #1429 ```

david-sarah@jacaranda.org commented

2011-08-09 01:02:46 +00:00

In changeset:08af9cea50c3c3cd:

test_drop_upload.py: fix unused imports. refs #1429

In changeset:08af9cea50c3c3cd: ``` test_drop_upload.py: fix unused imports. refs #1429 ```

davidsarah commented

2011-08-09 19:57:11 +00:00

Attachment rename-upload-uri-to-dircap.darcs.patch (37632 bytes) added

drop-upload: rename the 'upload.uri' parameter to 'upload.dircap', and a couple of cleanups to error messages. refs #1429

**Attachment** rename-upload-uri-to-dircap.darcs.patch (37632 bytes) added drop-upload: rename the 'upload.uri' parameter to 'upload.dircap', and a couple of cleanups to error messages. refs #1429

rename-upload-uri-to-dircap.darcs.patch

37 KiB

zooko commented

2011-08-09 22:24:56 +00:00

Attachment 1429-rerecord-and-add-test-of-absent-localdir.darcs.patch (39023 bytes) added

**Attachment** 1429-rerecord-and-add-test-of-absent-localdir.darcs.patch (39023 bytes) added

1429-rerecord-and-add-test-of-absent-localdir.darcs.patch

38 KiB

zooko commented

2011-08-09 22:32:01 +00:00

Reviewed attachment:rename-upload-uri-to-dircap.darcs.patch. There are some issues. I'm attaching patches that fix a couple of them but not all.

Out of curiosity I rerecorded the patch with darcs record; this helped me find issues in the patch; see below. The resulting patches are in the latest attachment -- they redo the renaming with darcs replace and would be an acceptable variant to commit to trunk (also a suitably fixed version with hunks instead of darcs replace would be acceptable to me).

issues fixed by the latest attachment:

There was a missed rename in [Client.init_drop_uploader]source:trunk/src/allmydata/client.py?annotate=blame&rev=5124#L425 which would cause an exception if the code were executed.
In [init_drop_uploader]source:trunk/src/allmydata/client.py?annotate=blame&rev=5124#L425 the local variable could be changed from upload_uri to upload_cap, the way this patch changed such names elsewhere. This is changed by the darcs record rename in the attached patches.
In [DropUploader.init]source:trunk/src/allmydata/frontends/drop_upload.py?annotate=blame&rev=5124#L19 it uses FilePath.is_dir as if it will return False when then thing doesn't exist or the thing is a non-directory, but FilePath.is_dir actually raises exception when the thing doesn't exist. The attached patches fix it to use FilePath.exists and change it to report those two cases separately.

issues not fixed:

The presence of the missed rename also means that the code in Client.init_drop_uploader isn't exercised by unit tests.
The "was not valid utf-8" and the "could not be represented in your local filesystem" exceptions could be caught and reported separately in [DropUploader.init]source:trunk/src/allmydata/frontends/drop_upload.py?annotate=blame&rev=5124#L19 so that the user would know which one happened. Not necessary for this ticket, but could be nice.

Reviewed attachment:rename-upload-uri-to-dircap.darcs.patch. There are some issues. I'm attaching patches that fix a couple of them but not all. Out of curiosity I rerecorded the patch with `darcs record`; this helped me find issues in the patch; see below. The resulting patches are in [the latest attachment](/tahoe-lafs/trac-2024-07-25/attachments/000078ac-7801-beec-42de-dcae8114c7c0) -- they redo the renaming with `darcs replace` and would be an acceptable variant to commit to trunk (also a suitably fixed version with hunks instead of `darcs replace` would be acceptable to me). issues fixed by [the latest attachment](/tahoe-lafs/trac-2024-07-25/attachments/000078ac-7801-beec-42de-dcae8114c7c0): * There was a missed rename in [Client.init_drop_uploader]source:trunk/src/allmydata/client.py?annotate=blame&rev=5124#L425 which would cause an exception if the code were executed. * In [init_drop_uploader]source:trunk/src/allmydata/client.py?annotate=blame&rev=5124#L425 the local variable could be changed from `upload_uri` to `upload_cap`, the way this patch changed such names elsewhere. This is changed by the `darcs record` rename in the attached patches. * In [DropUploader.*init*]source:trunk/src/allmydata/frontends/drop_upload.py?annotate=blame&rev=5124#L19 it uses `FilePath.is_dir` as if it will return `False` when then thing doesn't exist *or* the thing is a non-directory, but `FilePath.is_dir` actually raises exception when the thing doesn't exist. The attached patches fix it to use `FilePath.exists` and change it to report those two cases separately. issues not fixed: * The presence of the missed rename also means that the code in `Client.init_drop_uploader` isn't exercised by unit tests. * The "was not valid utf-8" and the "could not be represented in your local filesystem" exceptions could be caught and reported separately in [DropUploader.*init*]source:trunk/src/allmydata/frontends/drop_upload.py?annotate=blame&rev=5124#L19 so that the user would know which one happened. Not necessary for this ticket, but could be nice.

zooko removed the

fixed

label 2011-08-09 22:32:01 +00:00

zooko reopened this issue

2011-08-09 22:32:01 +00:00

zooko commented

2011-08-09 22:38:40 +00:00

Specifically it is the code inside the "if drop-upload is configured" block in Client.init_drop_uploader. The unit tests instantiate a DropUploader instance directly, but don't test what happens when you instantiate a Client instance which is configured to create a DropUploader.

Specifically it is the code inside the "if drop-upload is configured" block in `Client.init_drop_uploader`. The unit tests instantiate a `DropUploader` instance directly, but don't test what happens when you instantiate a `Client` instance which is configured to create a `DropUploader`.

zooko commented

2011-08-09 22:41:05 +00:00

I use the following bash script (executed by emacs) to generate code coverage results for just test_drop_upload:

cd ~/playground/tahoe-lafs/1429-rerecord
rm -rf ./.cover* ./htmlcov*
coverage run --branch --include="`pwd`/src/*" /usr/local/bin/trial allmydata.test.test_drop_upload

I use the following bash script (executed by emacs) to generate code coverage results for just `test_drop_upload`: ``` cd ~/playground/tahoe-lafs/1429-rerecord rm -rf ./.cover* ./htmlcov* coverage run --branch --include="`pwd`/src/*" /usr/local/bin/trial allmydata.test.test_drop_upload ```

zooko@zooko.com commented

2011-08-10 04:26:26 +00:00

In changeset:720bc2433b9bd16d:

drop-upload: unit tests for the configuration options being named "cap" instead of "uri"
This is a subset of a patch that David-Sarah attached to #1429. This is just the unit-tests part of that patch, and uses darcs record instead of hunks to change the names.
refs #1429

In changeset:720bc2433b9bd16d: ``` drop-upload: unit tests for the configuration options being named "cap" instead of "uri" This is a subset of a patch that David-Sarah attached to #1429. This is just the unit-tests part of that patch, and uses darcs record instead of hunks to change the names. refs #1429 ```

zooko@zooko.com commented

2011-08-10 04:26:27 +00:00

In changeset:5633375d267e5728:

drop-upload test for non-existent local dir separately from test for non-directory local dir
A candidate patch for #1429 has a bug when it is using FilePath.is_dir() to detect whether the configured local dir exists and is a directory. FilePath.is_dir() raises exception, instead of returning False, if the thing doesn't exist. This test is to make sure that DropUploader.__init__ raise different exceptions for those two cases.
refs #1429

In changeset:5633375d267e5728: ``` drop-upload test for non-existent local dir separately from test for non-directory local dir A candidate patch for #1429 has a bug when it is using FilePath.is_dir() to detect whether the configured local dir exists and is a directory. FilePath.is_dir() raises exception, instead of returning False, if the thing doesn't exist. This test is to make sure that DropUploader.__init__ raise different exceptions for those two cases. refs #1429 ```

zooko@zooko.com commented

2011-08-10 04:26:29 +00:00

In changeset:b7683d9b83a23cdd:

drop-upload: rename the 'upload.uri' parameter to 'upload.dircap', and a couple of cleanups to error messages. refs #1429
I rerecorded this patch, originally by David-Sarah, to use "darcs replace" instead of editing to do the renames. This uncovered one missed rename in Client.init_drop_uploader. (Which also means that code isn't exercised by the current unit tests.)
refs #1429

In changeset:b7683d9b83a23cdd: ``` drop-upload: rename the 'upload.uri' parameter to 'upload.dircap', and a couple of cleanups to error messages. refs #1429 I rerecorded this patch, originally by David-Sarah, to use "darcs replace" instead of editing to do the renames. This uncovered one missed rename in Client.init_drop_uploader. (Which also means that code isn't exercised by the current unit tests.) refs #1429 ```

zooko@zooko.com commented

2011-08-10 04:26:29 +00:00

In changeset:612abca271703508:

drop-upload: report the configured local directory being absent differently from it being a file
refs #1429

In changeset:612abca271703508: ``` drop-upload: report the configured local directory being absent differently from it being a file refs #1429 ```

david-sarah@jacaranda.org commented

2011-08-10 04:26:30 +00:00

In changeset:369e30b1dfa2b77f:

test_drop_upload.py: fix some grammatical and spelling nits. refs #1429

In changeset:369e30b1dfa2b77f: ``` test_drop_upload.py: fix some grammatical and spelling nits. refs #1429 ```

david-sarah@jacaranda.org commented

2011-08-10 04:26:31 +00:00

In changeset:f157b733676b33f5:

test_drop_upload.py: add comment explaining why we don't use FilePath.setContent. refs #1429

In changeset:f157b733676b33f5: ``` test_drop_upload.py: add comment explaining why we don't use FilePath.setContent. refs #1429 ```

david-sarah@jacaranda.org commented

2011-08-10 04:26:32 +00:00

In changeset:10ee22f50e5bdfd3:

drop-upload: rename 'start' method to 'startService', which is what you're supposed to use to start a Service. refs #1429

In changeset:10ee22f50e5bdfd3: ``` drop-upload: rename 'start' method to 'startService', which is what you're supposed to use to start a Service. refs #1429 ```

david-sarah@jacaranda.org commented

2011-08-10 04:26:33 +00:00

In changeset:c102056ac1df1784:

test_client.py: add a test that the drop-uploader is initialized correctly by client.py. Also give the DropUploader service a name, which is necessary for the test. refs #1429

In changeset:c102056ac1df1784: ``` test_client.py: add a test that the drop-uploader is initialized correctly by client.py. Also give the DropUploader service a name, which is necessary for the test. refs #1429 ```

david-sarah@jacaranda.org commented

2011-08-10 04:26:34 +00:00

In changeset:db22fdc20dc93a3e:

Factor out methods dealing with non-ASCII directories and filenames from test_drop_upload.py into common_util.py. refs #1429, #1472

In changeset:db22fdc20dc93a3e: ``` Factor out methods dealing with non-ASCII directories and filenames from test_drop_upload.py into common_util.py. refs #1429, #1472 ```

david-sarah@jacaranda.org commented

2011-08-10 06:32:21 +00:00

In changeset:ab9eb12f7006322f:

test_client.py: relax a check in test_create_drop_uploader so that it should pass on Python 2.4.x. refs #1429

In changeset:ab9eb12f7006322f: ``` test_client.py: relax a check in test_create_drop_uploader so that it should pass on Python 2.4.x. refs #1429 ```

david-sarah@jacaranda.org commented

2011-08-10 17:28:00 +00:00

In [5148/ticket393-MDMF-2]:

Drop-upload frontend, rerecorded for 1.9 beta (and correcting a minor mistake). Includes some fixes for Windows but not the Windows inotify implementation. fixes #1429

In [5148/ticket393-MDMF-2]: ``` Drop-upload frontend, rerecorded for 1.9 beta (and correcting a minor mistake). Includes some fixes for Windows but not the Windows inotify implementation. fixes #1429 ```

tahoe-lafs added the

fixed

label 2011-08-10 17:28:00 +00:00

david-sarah@jacaranda.org closed this issue

2011-08-10 17:28:00 +00:00

david-sarah@jacaranda.org commented

2011-08-10 17:28:04 +00:00

In [5149/ticket393-MDMF-2]:

Documentation for drop-upload frontend. refs #1429

In [5149/ticket393-MDMF-2]: ``` Documentation for drop-upload frontend. refs #1429 ```

david-sarah@jacaranda.org commented

2011-08-10 17:28:04 +00:00

In [5150/ticket393-MDMF-2]:

test_drop_upload.py: fix unused imports. refs #1429

In [5150/ticket393-MDMF-2]: ``` test_drop_upload.py: fix unused imports. refs #1429 ```

zooko@zooko.com commented

2011-08-10 17:28:06 +00:00

In [5155/ticket393-MDMF-2]:

drop-upload: unit tests for the configuration options being named "cap" instead of "uri"
This is a subset of a patch that David-Sarah attached to #1429. This is just the unit-tests part of that patch, and uses darcs record instead of hunks to change the names.
refs #1429

In [5155/ticket393-MDMF-2]: ``` drop-upload: unit tests for the configuration options being named "cap" instead of "uri" This is a subset of a patch that David-Sarah attached to #1429. This is just the unit-tests part of that patch, and uses darcs record instead of hunks to change the names. refs #1429 ```

zooko@zooko.com commented

2011-08-10 17:28:07 +00:00

In [5156/ticket393-MDMF-2]:

drop-upload test for non-existent local dir separately from test for non-directory local dir
A candidate patch for #1429 has a bug when it is using FilePath.is_dir() to detect whether the configured local dir exists and is a directory. FilePath.is_dir() raises exception, instead of returning False, if the thing doesn't exist. This test is to make sure that DropUploader.__init__ raise different exceptions for those two cases.
refs #1429

In [5156/ticket393-MDMF-2]: ``` drop-upload test for non-existent local dir separately from test for non-directory local dir A candidate patch for #1429 has a bug when it is using FilePath.is_dir() to detect whether the configured local dir exists and is a directory. FilePath.is_dir() raises exception, instead of returning False, if the thing doesn't exist. This test is to make sure that DropUploader.__init__ raise different exceptions for those two cases. refs #1429 ```

zooko@zooko.com commented

2011-08-10 17:28:07 +00:00

In [5157/ticket393-MDMF-2]:

drop-upload: rename the 'upload.uri' parameter to 'upload.dircap', and a couple of cleanups to error messages. refs #1429
I rerecorded this patch, originally by David-Sarah, to use "darcs replace" instead of editing to do the renames. This uncovered one missed rename in Client.init_drop_uploader. (Which also means that code isn't exercised by the current unit tests.)
refs #1429

In [5157/ticket393-MDMF-2]: ``` drop-upload: rename the 'upload.uri' parameter to 'upload.dircap', and a couple of cleanups to error messages. refs #1429 I rerecorded this patch, originally by David-Sarah, to use "darcs replace" instead of editing to do the renames. This uncovered one missed rename in Client.init_drop_uploader. (Which also means that code isn't exercised by the current unit tests.) refs #1429 ```

zooko@zooko.com commented

2011-08-10 17:28:08 +00:00

In [5158/ticket393-MDMF-2]:

drop-upload: report the configured local directory being absent differently from it being a file
refs #1429

In [5158/ticket393-MDMF-2]: ``` drop-upload: report the configured local directory being absent differently from it being a file refs #1429 ```

david-sarah@jacaranda.org commented

2011-08-10 17:28:09 +00:00

In [5159/ticket393-MDMF-2]:

test_drop_upload.py: fix some grammatical and spelling nits. refs #1429

In [5159/ticket393-MDMF-2]: ``` test_drop_upload.py: fix some grammatical and spelling nits. refs #1429 ```

david-sarah@jacaranda.org commented

2011-08-10 17:28:10 +00:00

In [5160/ticket393-MDMF-2]:

test_drop_upload.py: add comment explaining why we don't use FilePath.setContent. refs #1429

In [5160/ticket393-MDMF-2]: ``` test_drop_upload.py: add comment explaining why we don't use FilePath.setContent. refs #1429 ```

david-sarah@jacaranda.org commented

2011-08-10 17:28:11 +00:00

In [5161/ticket393-MDMF-2]:

drop-upload: rename 'start' method to 'startService', which is what you're supposed to use to start a Service. refs #1429

In [5161/ticket393-MDMF-2]: ``` drop-upload: rename 'start' method to 'startService', which is what you're supposed to use to start a Service. refs #1429 ```

david-sarah@jacaranda.org commented

2011-08-10 17:28:12 +00:00

In [5162/ticket393-MDMF-2]:

test_client.py: add a test that the drop-uploader is initialized correctly by client.py. Also give the DropUploader service a name, which is necessary for the test. refs #1429

In [5162/ticket393-MDMF-2]: ``` test_client.py: add a test that the drop-uploader is initialized correctly by client.py. Also give the DropUploader service a name, which is necessary for the test. refs #1429 ```

david-sarah@jacaranda.org commented

2011-08-10 17:28:13 +00:00

In [5163/ticket393-MDMF-2]:

Factor out methods dealing with non-ASCII directories and filenames from test_drop_upload.py into common_util.py. refs #1429, #1472

In [5163/ticket393-MDMF-2]: ``` Factor out methods dealing with non-ASCII directories and filenames from test_drop_upload.py into common_util.py. refs #1429, #1472 ```

david-sarah@jacaranda.org commented

2011-08-10 17:28:15 +00:00

In [5167/ticket393-MDMF-2]:

test_client.py: relax a check in test_create_drop_uploader so that it should pass on Python 2.4.x. refs #1429

In [5167/ticket393-MDMF-2]: ``` test_client.py: relax a check in test_create_drop_uploader so that it should pass on Python 2.4.x. refs #1429 ```

davidsarah commented

2011-08-10 20:12:02 +00:00

Although this was auto-closed for the wrong reason (a commit of the original patch on a branch), it is in fact fixed.

warner commented

2011-11-01 06:20:41 +00:00

just to be clear, this made it into 1.9.0 (released yesterday). It's labeled as "experimental", which means we aren't committed to supporting it long-term yet, and it may get pulled out if it misbehaves :).

Sign in to join this conversation.