consider changes to webapi "Move" API before release #1732

New issue

Closed

opened 2012-05-09 23:56:56 +00:00 by warner · 51 comments

warner commented

2012-05-09 23:56:56 +00:00

POST /uri/$DIRCAP?t=move&from_name=OLD&to_dir=SUBDIRNAME&target_type=name

or to a (possibly unrelated) subdir by its dircap:

POST /uri/$DIRCAP?t=move&from_name=OLD&to_dir=NEWDIRCAP&target_type=uri

That feels a bit weird. I'm kind of thinking that we should accept to_dirname= or to_dircap= (and throw an error if you provide both), instead of switching the interpretation of to_dir= according to the presence of that target_type= argument.

thoughts? note this is a blocker for 1.10 (or whatever release is next done on trunk), since we don't want to wind up supporting the wrong API forever.

I just finished reviewing and landing #1579, and it looks nice. One thing we should probably consider though, before committing to the API, is whether we're happy with the split "subdirname-vs-dircap" API. In the current code, you can either move a child to a subdir by name: ``` POST /uri/$DIRCAP?t=move&from_name=OLD&to_dir=SUBDIRNAME&target_type=name ``` or to a (possibly unrelated) subdir by its dircap: ``` POST /uri/$DIRCAP?t=move&from_name=OLD&to_dir=NEWDIRCAP&target_type=uri ``` That feels a bit weird. I'm kind of thinking that we should accept `to_dirname=` or `to_dircap=` (and throw an error if you provide both), instead of switching the interpretation of `to_dir=` according to the presence of that `target_type=` argument. thoughts? note this is a blocker for 1.10 (or whatever release is next done on trunk), since we don't want to wind up supporting the wrong API forever.

warner added the

labels 2012-05-09 23:56:56 +00:00

warner added this to the 1.10.0 milestone 2012-05-09 23:56:56 +00:00

zooko commented

2012-05-10 00:06:43 +00:00

I agree that to_dirname=/to_dircap= is a better API than to_dir=&target_type=.

I agree that `to_dirname=`/`to_dircap=` is a better API than `to_dir=&target_type=`.

daira commented

2012-05-10 00:43:19 +00:00

I agree as well (in general I don't like overloading of the interpretation of one argument based on another argument, in any API).

Should to_dircap accept a path after the dircap? Note that supporting that would allow changing tahoe mv to use this API, which is a prerequisite to fixing #943 in the way proposed in /tahoe-lafs/trac/issues/26887#comment:-1 (since that fix depends on knowing the path that was used in the tahoe mv command to specify the destination). If it does accept a path then it should be called something other than to_dircap.

Unix mv can rename a file at the same time as moving it. Should there be a to_name argument (defaulting to from_name)?

[fix wrong bug number for #943]edit:

I agree as well (in general I don't like overloading of the interpretation of one argument based on another argument, in any API). Should `to_dircap` accept a path after the dircap? Note that supporting that would allow changing `tahoe mv` to use this API, which is a prerequisite to fixing #943 in the way proposed in [/tahoe-lafs/trac/issues/26887](/tahoe-lafs/trac/issues/26887)#[comment:-1](/tahoe-lafs/trac/issues/1732#issuecomment--1) (since that fix depends on knowing the path that was used in the `tahoe mv` command to specify the destination). If it does accept a path then it should be called something other than `to_dircap`. Unix `mv` can rename a file at the same time as moving it. Should there be a `to_name` argument (defaulting to `from_name`)? [fix wrong bug number for #943]edit:

daira added

t/defect

and removed

t/task

labels 2012-05-10 00:43:19 +00:00

marcusw commented

2012-05-10 15:02:37 +00:00

Owner

This looks good; the current API is simply a result of the URI target functionality being tacked onto the subdir option. I like warner's API better.

Think I'll change this from "defect" to "enhancement" because my API did in fact work, even if not optimal :p

This looks good; the current API is simply a result of the URI target functionality being tacked onto the subdir option. I like warner's API better. Think I'll change this from "defect" to "enhancement" because my API did in fact work, even if not optimal :p

tahoe-lafs added

t/enhancement

and removed

t/defect

labels 2012-05-10 15:02:37 +00:00

zooko modified the milestone from 1.10.0 to 1.9.2

2012-05-16 22:10:52 +00:00

zooko modified the milestone from 1.9.2 to 1.10.0

2012-05-16 22:11:38 +00:00

daira commented

2012-05-16 22:49:47 +00:00

Milestone: 1.10.0 is correct because the 'Move' feature is only in trunk, not on the 1.9.2 branch.

daira added

p/major

and removed

p/normal

labels 2012-05-16 22:49:47 +00:00

warner commented

2012-12-20 17:51:46 +00:00

Author

I'll try to write a patch for this over the weekend.

warner self-assigned this 2012-12-20 17:51:46 +00:00

zooko commented

2013-02-07 17:10:49 +00:00

Replying to davidsarah:

Should to_dircap accept a path after the dircap? Note that supporting that would allow changing tahoe mv to use this API, which is a prerequisite to fixing #943 in the way proposed in /tahoe-lafs/trac/issues/26887#comment:-1 (since that fix depends on knowing the path that was used in the tahoe mv command to specify the destination). If it does accept a path then it should be called something other than to_dircap.

There this thing that we use a lot in the WUI/WAPI: a dircap optionally followed by a list of childnames. The list of childnames is clearly a unix (relative) path. But there's no word for "dircap+relpath". I think we need to coin a word for it. I can't think of anything clever and clear, so how about something awkward and explicit and clear, like "dircappluspath"?

Hm, or actually I think we can extend the vocabulary of unix here. Unix already has "absolute paths" and "relative paths". How about if we call these things "rooted paths"?

Replying to [davidsarah](/tahoe-lafs/trac/issues/1732#issuecomment-389759): > > Should `to_dircap` accept a path after the dircap? Note that supporting that would allow changing `tahoe mv` to use this API, which is a prerequisite to fixing #943 in the way proposed in [/tahoe-lafs/trac/issues/26887](/tahoe-lafs/trac/issues/26887)#[comment:-1](/tahoe-lafs/trac/issues/1732#issuecomment--1) (since that fix depends on knowing the path that was used in the `tahoe mv` command to specify the destination). If it does accept a path then it should be called something other than `to_dircap`. There this thing that we use a lot in the WUI/WAPI: a dircap optionally followed by a list of childnames. The list of childnames is clearly a unix (relative) path. But there's no word for "dircap+relpath". I think we need to coin a word for it. I can't think of anything clever and clear, so how about something awkward and explicit and clear, like "dircappluspath"? Hm, or actually I think we can extend the vocabulary of unix here. Unix already has "absolute paths" and "relative paths". How about if we call these things "rooted paths"?

daira commented

2013-02-09 00:22:35 +00:00

I like "rooted path" because it's consistent with the use of "rooted" in graph theory.

zooko commented

2013-02-10 12:42:47 +00:00

Great! I think "rooted path" is a good word for this. So in the WAPI, we currently have:

POST /uri/$DIRCAP?t=move&from_name=OLD&to_dir=SUBDIRNAME&target_type=name

POST /uri/$DIRCAP?t=move&from_name=OLD&to_dir=NEWDIRCAP&target_type=uri

And the proposal is to change it to:

POST /uri/$DIRCAP?t=move&from_name=OLD&to_dirname=SUBDIRNAME

and

POST /uri/$DIRCAP?t=move&from_name=OLD&to=NEWROOTEDPATH

Great! I think "rooted path" is a good word for this. So in the WAPI, we currently have: ``` POST /uri/$DIRCAP?t=move&from_name=OLD&to_dir=SUBDIRNAME&target_type=name ``` ``` POST /uri/$DIRCAP?t=move&from_name=OLD&to_dir=NEWDIRCAP&target_type=uri ``` And the proposal is to change it to: ``` POST /uri/$DIRCAP?t=move&from_name=OLD&to_dirname=SUBDIRNAME ``` and ``` POST /uri/$DIRCAP?t=move&from_name=OLD&to=NEWROOTEDPATH ``` ?

daira commented

2013-02-10 22:17:15 +00:00

Would to_relative and to_rooted be better names for the parameters? (to_dirname seems inaccurate for the case where the target is in the same directory as before.)

Would `to_relative` and `to_rooted` be better names for the parameters? (`to_dirname` seems inaccurate for the case where the target is in the same directory as before.)

daira commented

2013-02-10 22:20:59 +00:00

Actually, if we allow rooted paths, is it any longer necessary to have distinct parameters? A relative path and a rooted path can be distinguished exactly as they are by the CLI.

daira commented

2013-02-10 22:21:27 +00:00

(except for not allowing aliases)

zooko commented

2013-02-10 23:26:07 +00:00

Replying to davidsarah:

Actually, if we allow rooted paths, is it any longer necessary to have distinct parameters? A relative path and a rooted path can be distinguished exactly as they are by the CLI.

Well, here is the form that uses a rooted path:

POST /uri/$DIRCAP?t=move&from_name=OLD&to=NEWROOTEDPATH

The only use of the other form -- POST /uri/$DIRCAP?t=move&from_name=OLD&to_dirname=SUBDIRNAME would be to more succinctly express when you want the "to_dirname" to be relative to $DIRCAP instead of relative to a (different) root. So, I guess it is a tiny optimization and not worth the added complexity of API, IMO.

So I think David-Sarah is right, and a single API (POST /uri/$DIRCAP?t=move&from_name=OLD&to=NEWROOTEDPATH) is sufficient.

Replying to [davidsarah](/tahoe-lafs/trac/issues/1732#issuecomment-389776): > Actually, if we allow rooted paths, is it any longer necessary to have distinct parameters? A relative path and a rooted path can be distinguished exactly as they are by the CLI. Well, here is the form that uses a rooted path: ``` POST /uri/$DIRCAP?t=move&from_name=OLD&to=NEWROOTEDPATH ``` The only use of the other form -- `POST /uri/$DIRCAP?t=move&from_name=OLD&to_dirname=SUBDIRNAME` would be to more succinctly express when you want the "to_dirname" to be relative to `$DIRCAP` instead of relative to a (different) root. So, I guess it is a tiny optimization and not worth the added complexity of API, IMO. So I think David-Sarah is right, and a single API (`POST /uri/$DIRCAP?t=move&from_name=OLD&to=NEWROOTEDPATH`) is sufficient.

ClashTheBunny commented

2013-03-07 17:38:49 +00:00

Owner

We made sure that the WUI does not display or use a move button in 1.9.x, so there are no supported WUI's out there that would be broken. This means that there shouldn't be anybody left in the cold because of a change to this API.

My one thing that I think I understand but want to explicitly state is:

If "NEWROOTEDPATH" exists and is a directory, it is considered a destination directory where the file will be moved with the same name.
If "NEWROOTEDPATH" exists and is a file, it is an exception and should be raised.
If "NEWROOTEDPATH" doesn't exist, but the path above it exists, it's considered the destination with a (possibly new) name.
If the path above "NEWROOTEDPATH" doesn't exist, it is an exception and should be raised.
If "NEWROOTEDPATH" isn't a rooted path, an exception is raised. This case shall be shielded against for users of the WUI. The WUI will probably default to appending the current DIRCAP.

Rooted path seems to be the correct behavior that we would want to use for a programmatic interface. The only reason to not require the rooted path would be to protect humans from another DIRCAP or a copy paste error. Because we're not trying to protect humans from this, there was agreement on the weekly dev chat that this is the technically correct approach. We made sure that the WUI does not display or use a move button in 1.9.x, so there are no supported WUI's out there that would be broken. This means that there shouldn't be anybody left in the cold because of a change to this API. My one thing that I think I understand but want to explicitly state is: * If "NEWROOTEDPATH" exists and is a directory, it is considered a destination directory where the file will be moved with the same name. * If "NEWROOTEDPATH" exists and is a file, it is an exception and should be raised. * If "NEWROOTEDPATH" doesn't exist, but the path above it exists, it's considered the destination with a (possibly new) name. * If the path above "NEWROOTEDPATH" doesn't exist, it is an exception and should be raised. * If "NEWROOTEDPATH" isn't a rooted path, an exception is raised. This case shall be shielded against for users of the WUI. The WUI will probably default to appending the current DIRCAP.

zooko commented

2013-03-11 05:29:21 +00:00

Thanks a lot for the review, ClashTheBunny! I think your 5-point rules for handling this API call are right. Brian: does that look like a good API to you? Reassigning this ticket to Brian.

One more question, too: I earlier, in comment:16, wrote

POST /uri/$DIRCAP?t=move&from_name=OLD&to=NEWROOTEDPATH

Should it actually be:

POST /uri/$ROOTEDPATH_TO_DIR?t=move&from_name=OLD&to=NEWROOTEDPATH

? That is: does the caller have to supply the actual dircap of the original (source) directory, or can they use a rooted path to that directory?

Thanks a lot for the review, ClashTheBunny! I think your 5-point rules for handling this API call are right. Brian: does that look like a good API to you? Reassigning this ticket to Brian. One more question, too: I earlier, in comment:16, wrote `POST /uri/$DIRCAP?t=move&from_name=OLD&to=NEWROOTEDPATH` Should it actually be: `POST /uri/$ROOTEDPATH_TO_DIR?t=move&from_name=OLD&to=NEWROOTEDPATH` ? That is: does the caller have to supply the actual dircap of the original (source) directory, or can they use a rooted path to that directory?

ClashTheBunny commented

2013-03-14 16:28:23 +00:00

Owner

The rooted path is the most general representation of a file or directory, as it could just be a dircap or a dircap with subdirectories. As such, the correct representation is as zooko mentions:

POST /uri/$ROOTEDPATH_TO_DIR?t=move&from_name=OLD&to=NEWROOTEDPATH

It then adds this slight complication to my 5 point list above:

If ROOTEDPATH_TO_DIR actually points to a file, raise an exception.

The rooted path is the most general representation of a file or directory, as it could just be a dircap or a dircap with subdirectories. As such, the correct representation is as zooko mentions: POST /uri/$ROOTEDPATH_TO_DIR?t=move&from_name=OLD&to=NEWROOTEDPATH It then adds this slight complication to my 5 point list above: * If ROOTEDPATH_TO_DIR actually points to a file, raise an exception.

daira commented

2013-03-14 16:38:04 +00:00

I was thinking that zooko's suggested change might be useful to fix #943 (as suggested in /tahoe-lafs/trac/issues/26887#comment:-1), but it isn't actually necessary -- knowing the full path to the destination is sufficient to fix that ticket. OTOH, we agreed in the Dev Chat that this change was a good one anyway, just for generality and convenience.

I was thinking that zooko's suggested change might be useful to fix #943 (as suggested in [/tahoe-lafs/trac/issues/26887](/tahoe-lafs/trac/issues/26887)#[comment:-1](/tahoe-lafs/trac/issues/1732#issuecomment--1)), but it isn't actually necessary -- knowing the full path to the destination is sufficient to fix that ticket. OTOH, we agreed in the Dev Chat that this change was a good one anyway, just for generality and convenience.

zooko commented

2013-03-14 16:40:41 +00:00

Replying to ClashTheBunny:

If ROOTEDPATH_TO_DIR actually points to a file, raise an exception.

Hey that was a good comment.

Replying to [ClashTheBunny](/tahoe-lafs/trac/issues/1732#issuecomment-389781): > > * If ROOTEDPATH_TO_DIR actually points to a file, raise an exception. Hey that was a good comment.

daira commented

2013-03-14 17:26:46 +00:00

Proposed doc:

Moving A Child
--------------

``POST /uri/$DIRCAP/[SUBDIRS../]?t=move&from_name=OLD&to=$NEWDIRCAP/[NEWSUBDIRS../]NEW``

 This instructs the node to move a child of the given directory to a
 different directory, both of which must be writeable. The to= parameter
 should contain a path to the destination directory (multiple levels of
 descent are supported) ending with the new name. The new name must be
 included even if it is the same as the old name.

 The default behavior is to overwrite any existing object at the given
 location. To prevent this (and make the operation return an error instead
 of overwriting), add a "replace=false" argument. With replace=false, this
 operation will return an HTTP 409 "Conflict" error if there is already an
 object at the given location, rather than overwriting the existing object.
 To allow the operation to overwrite a file, but return an error when trying
 to overwrite a directory, use "replace=only-files" (this behavior is closer
 to the traditional UNIX "mv" command). Note that "true", "t", and "1" are
 all synonyms for "True", and "false", "f", and "0" are synonyms for "False",
 and the parameter is case-insensitive.

 For safety, the child is not unlinked from the old directory until it has
 been successfully added to the new directory.

This includes the "replace=only-files" option to avoid accidentally clobbering a directory, as discussed on the Dev Chat.

Proposed doc: ``` Moving A Child -------------- ``POST /uri/$DIRCAP/[SUBDIRS../]?t=move&from_name=OLD&to=$NEWDIRCAP/[NEWSUBDIRS../]NEW`` This instructs the node to move a child of the given directory to a different directory, both of which must be writeable. The to= parameter should contain a path to the destination directory (multiple levels of descent are supported) ending with the new name. The new name must be included even if it is the same as the old name. The default behavior is to overwrite any existing object at the given location. To prevent this (and make the operation return an error instead of overwriting), add a "replace=false" argument. With replace=false, this operation will return an HTTP 409 "Conflict" error if there is already an object at the given location, rather than overwriting the existing object. To allow the operation to overwrite a file, but return an error when trying to overwrite a directory, use "replace=only-files" (this behavior is closer to the traditional UNIX "mv" command). Note that "true", "t", and "1" are all synonyms for "True", and "false", "f", and "0" are synonyms for "False", and the parameter is case-insensitive. For safety, the child is not unlinked from the old directory until it has been successfully added to the new directory. ``` This includes the "replace=only-files" option to avoid accidentally clobbering a directory, as discussed on the Dev Chat.

daira commented

2013-03-14 17:40:49 +00:00

Note that this description doesn't define or use the term "rooted path". I've opened #1929 for that.

daira commented

2013-03-14 17:51:03 +00:00

Improvements to clarify that metadata is preserved and the crash/failure behaviour.

Moving A Child
--------------

``POST /uri/$DIRCAP/[SUBDIRS../]?t=move&from_name=OLD&to=$NEWDIRCAP/[NEWSUBDIRS../]NEW``

 This instructs the node to move a child of the given directory to a
 different directory, both of which must be writeable. Metadata from the
 directory entry is preserved. The to= parameter should contain a path
 to the destination directory (multiple levels of descent are supported)
 ending with the new name. The new name must be included even if it is the
 same as the old name.

 The default behavior is to overwrite any existing object at the given
 location. To prevent this (and make the operation return an error instead
 of overwriting), add a "replace=false" argument. With replace=false, this
 operation will return an HTTP 409 "Conflict" error if there is already an
 object at the given location, rather than overwriting the existing object.
 To allow the operation to overwrite a file, but return an error when trying
 to overwrite a directory, use "replace=only-files" (this behavior is closer
 to the traditional UNIX "mv" command). Note that "true", "t", and "1" are
 all synonyms for "True", and "false", "f", and "0" are synonyms for "False",
 and the parameter is case-insensitive.

 For safety, the child is not unlinked from the old directory until it has
 been successfully added to the new directory. This implies that in case of
 a crash or failure, the child will not be lost, but could be linked at both
 the old and new locations.

Improvements to clarify that metadata is preserved and the crash/failure behaviour. ``` Moving A Child -------------- ``POST /uri/$DIRCAP/[SUBDIRS../]?t=move&from_name=OLD&to=$NEWDIRCAP/[NEWSUBDIRS../]NEW`` This instructs the node to move a child of the given directory to a different directory, both of which must be writeable. Metadata from the directory entry is preserved. The to= parameter should contain a path to the destination directory (multiple levels of descent are supported) ending with the new name. The new name must be included even if it is the same as the old name. The default behavior is to overwrite any existing object at the given location. To prevent this (and make the operation return an error instead of overwriting), add a "replace=false" argument. With replace=false, this operation will return an HTTP 409 "Conflict" error if there is already an object at the given location, rather than overwriting the existing object. To allow the operation to overwrite a file, but return an error when trying to overwrite a directory, use "replace=only-files" (this behavior is closer to the traditional UNIX "mv" command). Note that "true", "t", and "1" are all synonyms for "True", and "false", "f", and "0" are synonyms for "False", and the parameter is case-insensitive. For safety, the child is not unlinked from the old directory until it has been successfully added to the new directory. This implies that in case of a crash or failure, the child will not be lost, but could be linked at both the old and new locations. ```

ClashTheBunny commented

2013-03-14 18:54:11 +00:00

Owner

Some possible improvements included below:

add 'replace' to top command line including options.
add replace=true as being the default value for replace
add comments about failure situations in the first paragraph Maybe there's a better response than a 400?

Moving A Child
--------------

``POST /uri/$DIRCAP/[SUBDIRS../]?t=move&from_name=OLD&to=$NEWDIRCAP/[NEWSUBDIRS../]NEW[&replace=true|false|only-files]``

 This instructs the node to move a child of the given directory to a
 different directory, both of which must be writeable. Metadata from the
 directory entry is preserved. The to= parameter should contain a path
 to the destination directory (multiple levels of descent are supported)
 ending with the new name. The new name must be included even if it is the
 same as the old name.  This operation will return an HTTP 400 "Bad Request"
 if ``$DIRCAP/[SUBDIRS../]`` refers to a file or doesn't exist or if 
 ``$NEWDIRCAP/[NEWSUBDIRS../]`` doesn't exist.

 The default behavior is to overwrite any existing object at the given
 location (replace=true). To prevent this (and make the operation return an error instead
 of overwriting), add a "replace=false" argument. With replace=false, this
 operation will return an HTTP 409 "Conflict" error if there is already an
 object at the given location, rather than overwriting the existing object.
 To allow the operation to overwrite a file, but return an error when trying
 to overwrite a directory, use "replace=only-files" (this behavior is closer
 to the traditional UNIX "mv" command). Note that "true", "t", and "1" are
 all synonyms for "True", and "false", "f", and "0" are synonyms for "False",
 and the parameter is case-insensitive.

 For safety, the child is not unlinked from the old directory until it has
 been successfully added to the new directory. This implies that in case of
 a crash or failure, the child will not be lost, but could be linked at both
 the old and new locations.

Some possible improvements included below: * add 'replace' to top command line including options. * add replace=true as being the default value for replace * add comments about failure situations in the first paragraph Maybe there's a better response than a 400? ``` Moving A Child -------------- ``POST /uri/$DIRCAP/[SUBDIRS../]?t=move&from_name=OLD&to=$NEWDIRCAP/[NEWSUBDIRS../]NEW[&replace=true|false|only-files]`` This instructs the node to move a child of the given directory to a different directory, both of which must be writeable. Metadata from the directory entry is preserved. The to= parameter should contain a path to the destination directory (multiple levels of descent are supported) ending with the new name. The new name must be included even if it is the same as the old name. This operation will return an HTTP 400 "Bad Request" if ``$DIRCAP/[SUBDIRS../]`` refers to a file or doesn't exist or if ``$NEWDIRCAP/[NEWSUBDIRS../]`` doesn't exist. The default behavior is to overwrite any existing object at the given location (replace=true). To prevent this (and make the operation return an error instead of overwriting), add a "replace=false" argument. With replace=false, this operation will return an HTTP 409 "Conflict" error if there is already an object at the given location, rather than overwriting the existing object. To allow the operation to overwrite a file, but return an error when trying to overwrite a directory, use "replace=only-files" (this behavior is closer to the traditional UNIX "mv" command). Note that "true", "t", and "1" are all synonyms for "True", and "false", "f", and "0" are synonyms for "False", and the parameter is case-insensitive. For safety, the child is not unlinked from the old directory until it has been successfully added to the new directory. This implies that in case of a crash or failure, the child will not be lost, but could be linked at both the old and new locations. ```

daira commented

2013-03-15 06:15:17 +00:00

The correct response code would be 404 Not Found if the source doesn't exist, or 403 Forbidden if it is a file. (You could also make a case for 418 ;-)

The correct response code would be 404 Not Found if the source doesn't exist, or 403 Forbidden if it is a file. (You could also make a case for [418](https://tools.ietf.org/html/rfc2324) ;-)

zooko commented

2013-03-18 14:31:29 +00:00

We agreed in the dev chat of 2013-03-14 that this API won't support an implicit final name, so you can't say POST /uri/$OLDDIRCAP/$OLDNAME/?t=move&from_name=$OLD&to=$NEWDIRCAP/$NEWNAME/ to mean that the new name should be $NEWDIRCAP/$NEWNAME/$OLD. This is something that unix mv supports ("mv foo/bar baz/"), and tahoe mv might or might not support it, but this API won't. The reason not to is that if it did, then it becomes harder to disambiguate if the user means to add to the directory $NEWDIRCAP/$NEWNAME a link under the name $OLD, or to add to the directory $NEWDIRCAP a link under the name $NEWNAME.

Now I'm confused about why we spell it $CAP/$NAME/&arg=$FINALNAME in the source but $CAP/$NAME/$FINALNAME in the destination? It's not consistent. In the interests of consistency, how about one of these two APIs:

POST /uri/$DIRCAP/[SUBDIRS../]OLD?t=move&to=$NEWDIRCAP/[NEWSUBDIRS../]NEW[&replace=true|false|only-files]

POST /uri/$DIRCAP/[SUBDIRS../]?t=move&from_name=OLD&to=$NEWDIRCAP/[NEWSUBDIRS../]&to_name=NEW[&replace=true|false|only-files]

Between these two, the first one has the advantage of being less wordy.

We agreed in the dev chat of 2013-03-14 that this API won't support an implicit final name, so you can't say `POST /uri/$OLDDIRCAP/$OLDNAME/?t=move&from_name=$OLD&to=$NEWDIRCAP/$NEWNAME/` to mean that the new name should be `$NEWDIRCAP/$NEWNAME/$OLD`. This is something that unix `mv` supports ("`mv foo/bar baz/`"), and tahoe mv might or might not support it, but this API won't. The reason not to is that if it did, then it becomes harder to disambiguate if the user means to add to the directory `$NEWDIRCAP/$NEWNAME` a link under the name `$OLD`, or to add to the directory `$NEWDIRCAP` a link under the name `$NEWNAME`. Now I'm confused about why we spell it `$CAP/$NAME/&arg=$FINALNAME` in the source but `$CAP/$NAME/$FINALNAME` in the destination? It's not consistent. In the interests of consistency, how about one of these two APIs: ``` POST /uri/$DIRCAP/[SUBDIRS../]OLD?t=move&to=$NEWDIRCAP/[NEWSUBDIRS../]NEW[&replace=true|false|only-files] ``` or ``` POST /uri/$DIRCAP/[SUBDIRS../]?t=move&from_name=OLD&to=$NEWDIRCAP/[NEWSUBDIRS../]&to_name=NEW[&replace=true|false|only-files] ``` Between these two, the first one has the advantage of being less wordy.

daira commented

2013-03-18 17:17:49 +00:00

I can think of two arguments for the second alternative:

a) Consistency with ?t=rename.

b) The object modified by the POST is $DIRCAP/SUBDIRS../. $DIRCAP/SUBDIRS../OLD is not modified (unless it happens to be aliased to the source or destination directory).

a) is not a strong argument; I would prefer conciseness over consistency with ?t=rename.

b) is a stronger argument, although the operation also modifies the destination directory which is not the object of the request. Still, I suppose this does mean that the second alternative is more consistent with REST~~, so I'm -0 on switching to the first alternative~~. I'm not sure which is more important, that or internal consistency.

I can think of two arguments for the second alternative: a) Consistency with `?t=rename`. b) The object modified by the POST is `$DIRCAP/SUBDIRS../`. `$DIRCAP/SUBDIRS../OLD` is not modified (unless it happens to be aliased to the source or destination directory). a) is not a strong argument; I would prefer conciseness over consistency with `?t=rename`. b) is a stronger argument, although the operation also modifies the destination directory which is not the object of the request. Still, I suppose this does mean that the second alternative is more consistent with REST~~, so I'm -0 on switching to the first alternative~~. I'm not sure which is more important, that or internal consistency.

daira commented

2013-03-18 17:31:06 +00:00

BTW, ?t=move has the potential to leak filenames and/or cap URIs into logs, similar to ?t=rename and ?t=rename-form in #1904.

BTW, `?t=move` has the potential to leak filenames and/or cap URIs into logs, similar to `?t=rename` and `?t=rename-form` in #1904.

warner commented

2013-03-18 18:04:26 +00:00

Author

Talking with zooko at pycon, we think the following might be best:

POST /uri/$DIRCAP/[SUBDIRS../]?t=move&from_name=OLD&to_dir=$NEWDIRCAP/[NEWSUBDIRS../]&to_name=NEW[&replace=true|false|only-files]

Zooko's first spelling (.../OLD?t=move&to=...NEW) would be nice(r), but the webapi code uses twisted.web Resource traversal to find the dir/file object referenced by the URL, and to accomodate .../OLD) we'd need to change that code to remember the penultimate Resource (the directory), which would be a lot of work. (parsing to= is easier: we could just regexp off the last slash).

Talking with zooko at pycon, we think the following might be best: ``` POST /uri/$DIRCAP/[SUBDIRS../]?t=move&from_name=OLD&to_dir=$NEWDIRCAP/[NEWSUBDIRS../]&to_name=NEW[&replace=true|false|only-files] ``` Zooko's first spelling (`.../OLD?t=move&to=...NEW`) would be nice(r), but the webapi code uses twisted.web Resource traversal to find the dir/file object referenced by the URL, and to accomodate `.../OLD`) we'd need to change that code to remember the penultimate Resource (the directory), which would be a lot of work. (parsing `to=` is easier: we could just regexp off the last slash).

daira commented

2013-03-18 20:01:57 +00:00

Replying to warner:

Talking with zooko at pycon, we think the following might be best:
POST /uri/$DIRCAP/SUBDIRS../?t=move&from_name=OLD&to_dir=$NEWDIRCAP/NEWSUBDIRS../&to_name=NEW[&replace=true|false|only-files]

Ah yes, I see the consistency argument for that. In that case perhaps to_name should default to from_name after all.

Zooko's first spelling (.../OLD?t=move&to=...NEW) would be nice(r), but the webapi code uses twisted.web Resource traversal to find the dir/file object referenced by the URL, and to accomodate .../OLD) we'd need to change that code to remember the penultimate Resource (the directory), which would be a lot of work. (parsing to= is easier: we could just regexp off the last slash).

That seems like a compelling argument. So the choice is now between comment:25 and comment:389792.

Replying to [warner](/tahoe-lafs/trac/issues/1732#issuecomment-389792): > Talking with zooko at pycon, we think the following might be best: > > ``` > POST /uri/$DIRCAP/SUBDIRS../?t=move&from_name=OLD&to_dir=$NEWDIRCAP/NEWSUBDIRS../&to_name=NEW[&replace=true|false|only-files] > ``` Ah yes, I see the consistency argument for that. In that case perhaps `to_name` should default to `from_name` after all. > Zooko's first spelling (`.../OLD?t=move&to=...NEW`) would be nice(r), but the webapi code uses twisted.web Resource traversal to find the dir/file object referenced by the URL, and to accomodate `.../OLD`) we'd need to change that code to remember the penultimate Resource (the directory), which would be a lot of work. (parsing `to=` is easier: we could just regexp off the last slash). That seems like a compelling argument. So the choice is now between comment:25 and [comment:389792](/tahoe-lafs/trac/issues/1732#issuecomment-389792).

daira commented

2013-03-18 20:33:07 +00:00

Oh, I've had another idea. What if we just generalize t=rename so that it can take an optional to_dir parameter? I.e.:

Moving or Renaming a Child
--------------------------

``POST /uri/$DIRCAP/[SUBDIRS../]?t=rename&from_name=OLD
       [&to_dir=$NEWDIRCAP/[NEWSUBDIRS../][&to_name=NEW][&replace=true|false|only-files]``

 This instructs the node to move or rename a child of the given directory,
 which must be writeable. If `to_dir` is present, the child is moved
 into the directory specified by `to_dir`, which must also be writeable.
 If `to_dir` is not present, the child is renamed within the same directory.
 If `to_name` is not present then it defaults to `from_name`.
 If the destination directory and name are the same as the source, the
 operation has no effect.

 Metadata from the source directory entry is preserved. Multiple levels of
 descent in the source and destination paths are supported.

 This operation will return an HTTP 404 "Not Found" error if
 ``$DIRCAP/[SUBDIRS../]``, the child being moved, or `to_dir` (if given)
 does not exist. It will return an HTTP 400 "Bad Request" error if any
 entry in the source or destination paths is not a directory.

 The default behavior is to overwrite any existing object at the given
 location (replace=true). To prevent this (and make the operation return
 an error instead of overwriting), add a "replace=false" argument. With
 replace=false, this operation will return an HTTP 409 "Conflict" error if
 there is already an object at the given location, rather than overwriting
 the existing object. To allow the operation to overwrite a file, but
 return an HTTP 409 error when trying to overwrite a directory, use
 "replace=only-files" (this behavior is closer to the traditional UNIX "mv"
 command). Note that "true", "t", and "1" are all synonyms for "True";
 "false", "f", and "0" are synonyms for "False"; and the parameter is
 case-insensitive.

 When moving to a different directory, for safety, the child is not unlinked
 from the old directory until it has been successfully added to the new
 directory. This implies that in case of a crash or failure, the child will
 not be lost, but could be linked at both the old and new locations.

 Prior to Tahoe-LAFS v1.10, the `to_dir` parameter and the
 `replace=only-files` option were not supported.

Oh, I've had another idea. What if we just generalize `t=rename` so that it can take an optional `to_dir` parameter? I.e.: ``` Moving or Renaming a Child -------------------------- ``POST /uri/$DIRCAP/[SUBDIRS../]?t=rename&from_name=OLD [&to_dir=$NEWDIRCAP/[NEWSUBDIRS../][&to_name=NEW][&replace=true|false|only-files]`` This instructs the node to move or rename a child of the given directory, which must be writeable. If `to_dir` is present, the child is moved into the directory specified by `to_dir`, which must also be writeable. If `to_dir` is not present, the child is renamed within the same directory. If `to_name` is not present then it defaults to `from_name`. If the destination directory and name are the same as the source, the operation has no effect. Metadata from the source directory entry is preserved. Multiple levels of descent in the source and destination paths are supported. This operation will return an HTTP 404 "Not Found" error if ``$DIRCAP/[SUBDIRS../]``, the child being moved, or `to_dir` (if given) does not exist. It will return an HTTP 400 "Bad Request" error if any entry in the source or destination paths is not a directory. The default behavior is to overwrite any existing object at the given location (replace=true). To prevent this (and make the operation return an error instead of overwriting), add a "replace=false" argument. With replace=false, this operation will return an HTTP 409 "Conflict" error if there is already an object at the given location, rather than overwriting the existing object. To allow the operation to overwrite a file, but return an HTTP 409 error when trying to overwrite a directory, use "replace=only-files" (this behavior is closer to the traditional UNIX "mv" command). Note that "true", "t", and "1" are all synonyms for "True"; "false", "f", and "0" are synonyms for "False"; and the parameter is case-insensitive. When moving to a different directory, for safety, the child is not unlinked from the old directory until it has been successfully added to the new directory. This implies that in case of a crash or failure, the child will not be lost, but could be linked at both the old and new locations. Prior to Tahoe-LAFS v1.10, the `to_dir` parameter and the `replace=only-files` option were not supported. ```

daira commented

2013-03-19 03:01:42 +00:00

IRC conversation, edited for formatting:

davidsarah: did you see my suggestion on #1732 ?

warner: yeah

warner: I'm uncertain about changing t=rename. Basically t=rename currently addresses a directory, and tells it to do something to itself. It's a single atomic operation (well, as atomic as changing any mutable file)

davidsarah: well, the operation we want is a strict generalization of rename

warner: whereas t=move addresses one directory and tells it to talk to a (possibly different) directory

warner: from a filesystem point of view, yeah

davidsarah: but only possibly different

warner: but in terms of dirnodes, that generalization would increase the number of actors from one to two

davidsarah: if the destination directory for t=move is the same as the source, then it should be atomic (from the filesystem point of view)

warner: true

davidsarah: so the complexity is still there

warner: yeah. I guess I think of a lot of the webapi in terms of speaking to a specific node and telling it to do something

davidsarah: basically the only difference between t=rename in my suggestion and t=move in (@@https://tahoe-lafs.org/trac/tahoe-lafs/ticket/1732#comment:389792@@) is the defaults for to_dir and to_name. I.e. to_dir defaults to the source dir, and to_name defaults to from_name

warner: from the outside, yeah

davidsarah: I'm all about eliminating redundancy :-)

warner: but the current t=rename is quasi-atomic (there's no way to wind up with multiple copies of the child), and that would change it to be sometimes non-atomic (if interrupted, it might add to the target but not remove from the original)

warner: I dunno

davidsarah: [would be non-atomic]It only if to_dir is provided

warner: I guess we could argue that having a t=move with the proposed syntax means it'd be cleaner to modify t=rename instead

davidsarah: besides, there was a suggestion to deprecate t=rename if we added t=move

warner: hm. ok, so I think "rename" shouldn't be used to move something long distances, whereas "move" is a fine verb for that. And I guess my attitude suggests that, if we only have one command, then it should be "move"

davidsarah: hmm. I don't place much significance on the operation name

warner: I dunno. [...] let's pester zooko to think about this too

davidsarah: well, we don't have the option of adding move and immediately removing rename... if we want to retain backward compatibility

warner: yeah, true. So if we keep rename around, then it suggests that "move" should be something different

davidsarah: I think we need Zooko as a casting vote :-) (it is actually quite useful having 3 of us deciding these things)

davidsarah: here's another advantage of my suggestion: the existing tests for t=rename will test the same-directory case, and so the new tests only need to cover the different-directory case

IRC conversation, edited for formatting: davidsarah: did you see my suggestion on #1732 ? warner: yeah warner: I'm uncertain about changing t=rename. Basically t=rename currently addresses a directory, and tells it to do something to itself. It's a single atomic operation (well, as atomic as changing any mutable file) davidsarah: well, the operation we want is a strict generalization of rename warner: whereas t=move addresses one directory and tells it to talk to a (possibly different) directory warner: from a filesystem point of view, yeah davidsarah: but only possibly different warner: but in terms of dirnodes, that generalization would increase the number of actors from one to two davidsarah: if the destination directory for t=move is the same as the source, then it should be atomic (from the filesystem point of view) warner: true davidsarah: so the complexity is still there warner: yeah. I guess I think of a lot of the webapi in terms of speaking to a specific node and telling it to do something davidsarah: basically the only difference between t=rename in my suggestion and t=move in (@@https://tahoe-lafs.org/trac/tahoe-lafs/ticket/1732#[comment:389792](/tahoe-lafs/trac/issues/1732#issuecomment-389792)@@) is the defaults for to_dir and to_name. I.e. to_dir defaults to the source dir, and to_name defaults to from_name warner: from the outside, yeah davidsarah: I'm all about eliminating redundancy :-) warner: but the current t=rename is quasi-atomic (there's no way to wind up with multiple copies of the child), and that would change it to be sometimes non-atomic (if interrupted, it might add to the target but not remove from the original) warner: I dunno davidsarah: [would be non-atomic]It only if to_dir is provided warner: I guess we could argue that having a t=move with the proposed syntax means it'd be cleaner to modify t=rename instead davidsarah: besides, there was a suggestion to deprecate t=rename if we added t=move warner: hm. ok, so I think "rename" shouldn't be used to move something long distances, whereas "move" is a fine verb for that. And I guess my attitude suggests that, if we only have one command, then it should be "move" davidsarah: hmm. I don't place much significance on the operation name warner: I dunno. [...] let's pester zooko to think about this too davidsarah: well, we don't have the option of adding move and immediately removing rename... if we want to retain backward compatibility warner: yeah, true. So if we keep rename around, then it suggests that "move" should be something different davidsarah: I think we need Zooko as a casting vote :-) (it is actually quite useful having 3 of us deciding these things) davidsarah: here's another advantage of my suggestion: the existing tests for t=rename will test the same-directory case, and so the new tests only need to cover the different-directory case

warner commented

2013-03-20 21:31:56 +00:00

Author

marking "critical" to indicate this is a blocker for 1.10

warner added

p/critical

and removed

p/major

labels 2013-03-20 21:31:56 +00:00

warner commented

2013-03-21 18:20:47 +00:00

Author

don't remove an existing API (so t=rename must stick around)
don't add parameters to an existing API that can't be discovered by callers, since our webapi practice is to ignore arguments we don't recognize

The latter constraint precludes adding some new argument to t=rename that would change behavior, because then a new client (who knows about the new argument) who submits such a request to an older gateway (who ignores the new argument) will get a very different behavior than they were expecting. It's the same reason that python functions reject unrecognized keyword-arguments: the use of kwargs allows new callees to add new kwargs over time, but still get a clear error when a new caller calls an older callee, instead of silently ignoring the new arg and doing something completely different.

So I guess that means I am picky about it, and I'm voting for a new t=move. :)

I mentioned to zooko this morning that I don't think I'm picky about this API, and am happy go along with whatever everyone else likes. But then I thought of two constraints that I think are important: * don't remove an existing API (so `t=rename` must stick around) * don't add parameters to an existing API that can't be discovered by callers, since our webapi practice is to ignore arguments we don't recognize The latter constraint precludes adding some new argument to `t=rename` that would change behavior, because then a new client (who knows about the new argument) who submits such a request to an older gateway (who ignores the new argument) will get a very different behavior than they were expecting. It's the same reason that python functions reject unrecognized keyword-arguments: the use of kwargs allows new callees to add new kwargs over time, but still get a clear error when a new caller calls an older callee, instead of silently ignoring the new arg and doing something completely different. So I guess that means I *am* picky about it, and I'm voting for a new `t=move`. :)

ClashTheBunny commented

2013-03-21 19:00:12 +00:00

Owner

Zooko and I talked about this during the dev talk and I wanted to share my two points.

I first felt that not having the full rooted cap to the file to be moved was punting in that it was hard to implement the other way, so we would do it this way. Zooko helped me understand that the real operation happens at the directory, in that the directory is a file allocation table that is being edited. He told me about the history with the command line 'rm' being confusing in that it doesn't remove a file, but just removes the entry in the directory's file allocation table. Because of this, I'm good with the new verbage of doing a move operation on the containing directory. It's also a win for people who want to just replace "rename" with the new API and things will just work.

The second thing we talked about is that his new API should really be called "relink" in that it's not operating on (moving) a file, but it's creating a link for a file in a new place, and then deleting the old link (or editing the link in the current directory). That brings me to my proposal for a slight alteration to comment 30's definition of the new API:

POST /uri/$DIRCAP/[SUBDIRS../]?t=relink&from_name=OLD&to_dir=$NEWDIRCAP/[NEWSUBDIRS../]&to_name=NEW[&replace=true|false|only-files]

This way people understand that the operation doesn't change a file in any way, but it actually edits links to the file. It operates on the directory, and not the file too, which is reflected in this. It also leave rename the same, but allows for people who want to update to the new API from 'rename' to just replace 'rename' with 'relink'.

Zooko and I talked about this during the dev talk and I wanted to share my two points. I first felt that not having the full rooted cap to the file to be moved was punting in that it was hard to implement the other way, so we would do it this way. Zooko helped me understand that the real operation happens at the directory, in that the directory is a file allocation table that is being edited. He told me about the history with the command line 'rm' being confusing in that it doesn't remove a file, but just removes the entry in the directory's file allocation table. Because of this, I'm good with the new verbage of doing a move operation on the containing directory. It's also a win for people who want to just replace "rename" with the new API and things will just work. The second thing we talked about is that his new API should really be called "relink" in that it's not operating on (moving) a file, but it's creating a link for a file in a new place, and then deleting the old link (or editing the link in the current directory). That brings me to my proposal for a slight alteration to [comment 30's definition of the new API](/tahoe-lafs/trac/issues/1732#issuecomment-389792): ``` POST /uri/$DIRCAP/[SUBDIRS../]?t=relink&from_name=OLD&to_dir=$NEWDIRCAP/[NEWSUBDIRS../]&to_name=NEW[&replace=true|false|only-files] ``` This way people understand that the operation doesn't change a file in any way, but it actually edits links to the file. It operates on the directory, and not the file too, which is reflected in this. It also leave rename the same, but allows for people who want to update to the new API from 'rename' to just replace 'rename' with 'relink'.

warner commented

2013-03-21 21:29:36 +00:00

Author

I'm cool with that.

daira commented

2013-03-21 23:30:32 +00:00

Replying to warner:
[...]

don't add parameters to an existing API that can't be discovered by callers, since our webapi practice is to ignore arguments we don't recognize

The latter constraint precludes adding some new argument to t=rename that would change behavior, because then a new client (who knows about the new argument) who submits such a request to an older gateway (who ignores the new argument) will get a very different behavior than they were expecting.

Oh, I am dense for not seeing that. OK, that rules out just generalizing t=rename. +1 for calling it t=relink.

Replying to [warner](/tahoe-lafs/trac/issues/1732#issuecomment-389799): [...] > * don't add parameters to an existing API that can't be discovered by callers, since our webapi practice is to ignore arguments we don't recognize > > The latter constraint precludes adding some new argument to `t=rename` that would change behavior, because then a new client (who knows about the new argument) who submits such a request to an older gateway (who ignores the new argument) will get a very different behavior than they were expecting. Oh, I am dense for not seeing that. OK, that rules out just generalizing `t=rename`. +1 for calling it `t=relink`.

daira commented

2013-03-22 00:16:03 +00:00

OK, I think this is what we have now:


Relinking a Child
-----------------

``POST /uri/$DIRCAP/[SUBDIRS../]?t=relink&from_name=OLD``
       ``&to_dir=$NEWDIRCAP/[NEWSUBDIRS../]&to_name=NEW``
       ``[&replace=true|false|only-files]``    (Tahoe >= v1.10)

 This instructs the node to relink a child of the given source directory,
 into a different directory and/or to a different name. The source and
 destination directories must be writeable. `to_dir` may be the same
 as the source directory and `to_name` may be the same as `from_name`,
 but `to_dir` and `to_name` are always required. If the destination
 link (directory and name) is the same as the source link, the operation has
 no effect.

 Metadata from the source directory entry is preserved. Multiple levels of
 descent in the source and destination paths are supported.

 This operation will return an HTTP 404 "Not Found" error if
 ``$DIRCAP/[SUBDIRS../]``, the child being moved, or the destination
 directory does not exist. It will return an HTTP 400 "Bad Request" error
 if any entry in the source or destination paths is not a directory.

 The default behavior is to overwrite any existing object at the destination
 (replace=true). To prevent this (and make the operation return an error
 instead of overwriting), add a "replace=false" argument. With replace=false,
 this operation will return an HTTP 409 "Conflict" error if there is already
 an object at the destination, rather than overwriting the existing object.
 To allow the operation to overwrite a file, but return an HTTP 409 error
 when trying to overwrite a directory, use "replace=only-files" (this behavior
 is closer to the traditional UNIX "mv" command). Note that "true", "t", and
 "1" are all synonyms for "True"; "false", "f", and "0" are synonyms for
 "False"; and the parameter is case-insensitive.

 When relinking into a different directory, for safety, the child is not
 unlinked from the old directory until it has been successfully linked into
 the new directory. This implies that in case of a crash or failure, the
 child will not be lost, but could be linked at both the old and new
 locations.

 The source link should not be the same as any link (directory and child name)
 in the ``to_dir`` path. This restriction is not enforced, but it may be
 enforced in a future version. If it were violated then the result would be
 to create a cycle in the directory structure that is not necessarily reachable
 from the root of the destination path (``$NEWDIRCAP``), which could result in
 data loss, as described in ticket `#943`_.

.. _`#943`: https://tahoe-lafs.org/trac/tahoe-lafs/ticket/943

(I just added the bit about #943.)

OK, I think this is what we have now: ``` Relinking a Child ----------------- ``POST /uri/$DIRCAP/[SUBDIRS../]?t=relink&from_name=OLD`` ``&to_dir=$NEWDIRCAP/[NEWSUBDIRS../]&to_name=NEW`` ``[&replace=true|false|only-files]`` (Tahoe >= v1.10) This instructs the node to relink a child of the given source directory, into a different directory and/or to a different name. The source and destination directories must be writeable. `to_dir` may be the same as the source directory and `to_name` may be the same as `from_name`, but `to_dir` and `to_name` are always required. If the destination link (directory and name) is the same as the source link, the operation has no effect. Metadata from the source directory entry is preserved. Multiple levels of descent in the source and destination paths are supported. This operation will return an HTTP 404 "Not Found" error if ``$DIRCAP/[SUBDIRS../]``, the child being moved, or the destination directory does not exist. It will return an HTTP 400 "Bad Request" error if any entry in the source or destination paths is not a directory. The default behavior is to overwrite any existing object at the destination (replace=true). To prevent this (and make the operation return an error instead of overwriting), add a "replace=false" argument. With replace=false, this operation will return an HTTP 409 "Conflict" error if there is already an object at the destination, rather than overwriting the existing object. To allow the operation to overwrite a file, but return an HTTP 409 error when trying to overwrite a directory, use "replace=only-files" (this behavior is closer to the traditional UNIX "mv" command). Note that "true", "t", and "1" are all synonyms for "True"; "false", "f", and "0" are synonyms for "False"; and the parameter is case-insensitive. When relinking into a different directory, for safety, the child is not unlinked from the old directory until it has been successfully linked into the new directory. This implies that in case of a crash or failure, the child will not be lost, but could be linked at both the old and new locations. The source link should not be the same as any link (directory and child name) in the ``to_dir`` path. This restriction is not enforced, but it may be enforced in a future version. If it were violated then the result would be to create a cycle in the directory structure that is not necessarily reachable from the root of the destination path (``$NEWDIRCAP``), which could result in data loss, as described in ticket `#943`_. .. _`#943`: https://tahoe-lafs.org/trac/tahoe-lafs/ticket/943 ``` (I just added the bit about #943.)

ClashTheBunny commented

2013-03-22 06:15:24 +00:00

Owner

I believe from_name is always required, which would be nice to include in the text. Maybe the first paragraph could look like this:

This instructs the node to relink a child of the given source directory, into a different directory and/or to a different name. The source and destination directories must be writeable. `from_name` must exist and must be be supplied.  `to_dir` may be the same as the source directory and `to_name` may be the same as `from_name`, but `to_dir` and `to_name` are always required. If the destination link (directory and name) is the same as the source link, the operation has no effect.

and again as a quote:


This instructs the node to relink a child of the given source directory,
into a different directory and/or to a different name. The source and
destination directories must be writeable. `from_name` must exist
and must be be supplied.  `to_dir` may be the same as the source
directory and `to_name` may be the same as `from_name`, but
`to_dir` and `to_name` are always required. If the destination
link (directory and name) is the same as the source link, the operation
has no effect.

But even without that, I think this is great. +1

I believe from_name is always required, which would be nice to include in the text. Maybe the first paragraph could look like this: This instructs the node to relink a child of the given source directory, into a different directory and/or to a different name. The source and destination directories must be writeable. `from_name` must exist and must be be supplied. `to_dir` may be the same as the source directory and `to_name` may be the same as `from_name`, but `to_dir` and `to_name` are always required. If the destination link (directory and name) is the same as the source link, the operation has no effect. and again as a quote: ``` This instructs the node to relink a child of the given source directory, into a different directory and/or to a different name. The source and destination directories must be writeable. `from_name` must exist and must be be supplied. `to_dir` may be the same as the source directory and `to_name` may be the same as `from_name`, but `to_dir` and `to_name` are always required. If the destination link (directory and name) is the same as the source link, the operation has no effect. ``` But even without that, I think this is great. +1

zooko commented

2013-03-22 21:13:46 +00:00

I've thought about this. I'm +1 on comment:389803 and comment:40, with the following comments:

to_name could be optional, and if omitted it defaults to from_name. The reason we earlier agreed to require it was that it could be ambiguous what was the name and what was the path. Now that the to_name lives in a separate field from the to_dir, there's no ambiguity.
Let's not deprecate the old rename at this time, but I'd like to revisit that in the future…

I've thought about this. I'm +1 on [comment:389803](/tahoe-lafs/trac/issues/1732#issuecomment-389803) and comment:40, with the following comments: * `to_name` could be optional, and if omitted it defaults to `from_name`. The reason we earlier agreed to require it was that it could be ambiguous what was the name and what was the path. Now that the `to_name` lives in a separate field from the `to_dir`, there's no ambiguity. * Let's not deprecate the old `rename` at this time, but I'd like to revisit that in the future…

warner commented

2013-03-23 18:03:48 +00:00

Author

I'm vaguely -0 on anything being optional, but it's a really weak -0.. I wouldn't object to it landing in that form.

ClashTheBunny commented

2013-03-23 20:07:52 +00:00

Owner

Philosophically, I'm for APIs that easy to use, but only if they are unambiguous and easy to not misuse. I don't think that having either to_dir or to_name be required makes the API ambiguous or easy to misuse. A good programmer with an attention to details would probably fill them both in anyhow.

My personal feeling is that this API would benefit from having a total compatibility with rename such that if rename were deprecated in the future, it would be a one word change from 'rename' to 'relink'. You could even do it with a hex editor on a binary since it's the same number of bits. I don't care as much about things being optional as I care about things being compatible. If we require to_dir, if people want to jump from rename to relink, that much more work will be required and that much more chance of people messing up. Philosophically, I'm for APIs that easy to use, but only if they are unambiguous and easy to not misuse. I don't think that having either to_dir or to_name be required makes the API ambiguous or easy to misuse. A good programmer with an attention to details would probably fill them both in anyhow.

daira commented

2013-03-24 09:18:06 +00:00

Replying to ClashTheBunny:

My personal feeling is that this API would benefit from having a total compatibility with rename such that if rename were deprecated in the future, it would be a one word change from 'rename' to 'relink'. You could even do it with a hex editor on a binary since it's the same number of bits. I don't care as much about things being optional as I care about things being compatible. If we require to_dir, if people want to jump from rename to relink, that much more work will be required and that much more chance of people messing up.

Well, if that were a concern then we could just not deprecate rename. It's not really imposing significant complexity, since it will be implementable by delegating to relink.

I'm with warner: -0 on having either to_dir or to_name be optional. Do we have concensus on the comment:389803 spec then? If so I will start implementing it tomorrow.

Replying to [ClashTheBunny](/tahoe-lafs/trac/issues/1732#issuecomment-389807): > My personal feeling is that this API would benefit from having a total compatibility with rename such that if rename were deprecated in the future, it would be a one word change from 'rename' to 'relink'. You could even do it with a hex editor on a binary since it's the same number of bits. I don't care as much about things being optional as I care about things being compatible. If we require to_dir, if people want to jump from rename to relink, that much more work will be required and that much more chance of people messing up. Well, if that were a concern then we could just not deprecate `rename`. It's not really imposing significant complexity, since it will be implementable by delegating to `relink`. I'm with warner: -0 on having either `to_dir` or `to_name` be optional. Do we have concensus on the [comment:389803](/tahoe-lafs/trac/issues/1732#issuecomment-389803) spec then? If so I will start implementing it tomorrow.

warner commented

2013-03-25 01:33:40 +00:00

Author

Ok, I think we have consensus. to_name= is optional, per zooko, to_dir= is optional, per ClashTheBunny, and we leave t=rename untouched.

Thanks davidsarah for implementing this!

Ok, I think we have consensus. `to_name=` is optional, per zooko, `to_dir=` is optional, per ClashTheBunny, and we leave `t=rename` untouched. Thanks davidsarah for implementing this!

zooko commented

2013-03-26 16:49:47 +00:00

Replying to warner:

Ok, I think we have consensus. to_name= is optional, per zooko, to_dir= is optional, per ClashTheBunny, and we leave t=rename untouched.

Agreed!

This means we need a sensible error message if both to_name= and to_dir= are omitted. ☺

Replying to [warner](/tahoe-lafs/trac/issues/1732#issuecomment-389809): > Ok, I think we have consensus. `to_name=` is optional, per zooko, `to_dir=` is optional, per ClashTheBunny, and we leave `t=rename` untouched. Agreed! This means we need a sensible error message if both `to_name=` and `to_dir=` are omitted. ☺

zooko commented

2013-04-04 04:10:16 +00:00

I posted a patch review: https://github.com/davidsarah/tahoe-lafs/commit/f75e9c7a3aff04204df0977d37e3b130549a506c#commitcomment-2941334

I posted a patch review: <https://github.com/davidsarah/tahoe-lafs/commit/f75e9c7a3aff04204df0977d37e3b130549a506c#commitcomment-2941334>

warner was unassigned by daira

2013-04-04 16:47:43 +00:00

daira self-assigned this 2013-04-04 16:47:43 +00:00

daira commented

2013-04-05 05:14:42 +00:00

Please review https://github.com/davidsarah/tahoe-lafs/commit/05d19771b3abf158a282f748198cfc2fcf849f53.

Please review <https://github.com/davidsarah/tahoe-lafs/commit/05d19771b3abf158a282f748198cfc2fcf849f53>.

daira removed their assignment 2013-04-05 05:15:05 +00:00

zooko was assigned by daira

2013-04-05 05:15:05 +00:00

zooko commented

2013-04-05 11:59:08 +00:00

+1! ☺

zooko removed their assignment 2013-04-05 11:59:08 +00:00

daira was assigned by zooko

2013-04-05 11:59:08 +00:00

daira commented

2013-04-05 18:42:04 +00:00

Fixed in changeset:35f37cc5b8646540.

daira added the

r/fixed

label 2013-04-05 18:42:04 +00:00

daira closed this issue

2013-04-05 18:42:04 +00:00

daira commented

2013-04-05 18:43:51 +00:00

Replying to [zooko]comment:46:

This means we need a sensible error message if both to_name= and to_dir= are omitted. ☺

If both are omitted then the operation succeeds with no effect.

Replying to [zooko]comment:46: > This means we need a sensible error message if both `to_name=` and `to_dir=` are omitted. ☺ If both are omitted then the operation succeeds with no effect.

zooko commented

2013-04-09 12:16:20 +00:00

If I understand the github WUI correctly, Brian made some docs changes that I don't agree with:

https://github.com/tahoe-lafs/tahoe-lafs/commit/f9335892f2008198bd8210b98dd953665d8a5e08

If I understand the github WUI correctly, Brian made some docs changes that I don't agree with: <https://github.com/tahoe-lafs/tahoe-lafs/commit/f9335892f2008198bd8210b98dd953665d8a5e08>

zooko removed the

r/fixed

label 2013-04-09 12:16:20 +00:00

zooko reopened this issue

2013-04-09 12:16:20 +00:00

zooko commented

2013-04-09 13:09:55 +00:00

Here's an attempt at fixing the issues I have with these docs while preserving Brian's apparent idea of making it discoverable to people who think in terms of "moving" the child: https://github.com/tahoe-lafs/tahoe-lafs/pull/36 Please review!

Here's an attempt at fixing the issues I have with these docs while preserving Brian's apparent idea of making it discoverable to people who think in terms of "moving" the child: <https://github.com/tahoe-lafs/tahoe-lafs/pull/36> Please review!

warner commented

2013-04-09 17:57:58 +00:00

Author

I like those changes.. landed.

warner added the

r/fixed

label 2013-04-09 17:57:58 +00:00

warner closed this issue

2013-04-09 17:57:58 +00:00

daira commented

2013-04-10 02:06:51 +00:00

Reviewed, +1 for zooko's changes.