Mark J. Nelson wrote: <snip>
>>> webrev is here: >>> http://cr.opensolaris.org/~vkotal/webrev_delete-6771203.onnv/ > > The date in the .TH section of the manpage should be updated. grr, I always forget to update this one. fixed. > webrev.sh: > 171, 236: When do you ever call this inappropriately? This message > doesn't seem particularly useful to the user, what does it really mean > that they need to change? Only during development. It's basically additional safety net so it can be removed. They were added at the time when delete_webrev() had 2 arguments and forgot to specify one of them. > 241-243: I'm not 100% confident that these are what you want. If I read > them correctly. Can't a remote_target specification be of the form > > protocol://[usern...@]host.spec[:port]/ > > ? Let's look at the parsing in delete_webrev() first: 240 + # Strip the transport specification part of remote target first. 241 + typeset -r stripped_target=${remote_target##*://} 242 + typeset -r host_spec=${stripped_target%%:*} 243 + typeset -r dir_spec=${stripped_target##*:} delete_webrev() accepts target specification in form protocol://[usern...@]hostname[:path]. The assignment on line 241 strips the protocol prefix, line 242 takes the username+hostname part and line 243 the directory specification. The full URI (remote_target, which is global variable) is used in webrev_delete() in order to avoid special casing when calling it from ssh_upload() because it already has the protocol prefix stripped. As for the port specification, this is not possible to do in remote target URI. Instead ssh_config(4) and Port directive should be used. > Specific questions: > - Do the host_spec and dir_spec assignments need to account for the > optional "username@" portion? Or does the stripped_target assignment > need to also strip that? No, rsync(1)/ssh(1) will cope with that. > - I can believe that you won't see multiple occurrences of "://" in the > remote_target spec, so ## seems appropriate, but what about ":" > characters? It seems like the correct complement to "%%:*" would be > "#*:" instead of "##*:" ? > > Of course, if we're always talking remote_host:remote_path, then what's > here makes sense, as long as ":" is not a legal character? Good catch. fixed. > 252-259: You're assuming two things here, and I think only one of them > is correct. (1) The remote_target is the path to the top level > directory of the webrev to be removed (this is valid). (2) The top > level directory of the webrev to be removed lives at the top of the > default directory tree for sftp (this is invalid). > > You might avoid the second assumption by adding a "cd" command to the > batch file used by sftp? In any event, the following two remote_target sftp does not allow to do that. > specifications should BOTH work correctly for user mjnelson: > > cr.opensolaris.org:webrev.481 > cr.opensolaris.org:toolsreview/webrev.bugs > > ...because they're both real paths. This will work fine (but only after I fixed it since there was indeed a bug in dir_rm handling). This is because 'rename foo/bar/my_webrev .trash/removed.3141' will do the right thing and move just the 'my_webrev' directory. The 'foo/bar' will stay. I've added a comment to the -t option to explain the behavior. > 273: So we're really collecting "deleted" webrevs forever? Or is there > something special about a ".trash" directory that isn't mentioned in the > sftp manpage? This seems evil: isn't part of the goal here to maintain > a smaller footprint? How do users REALLY get rid of them, or even known > that they SHOULD? On cr.opensolaris.org, the '.trash' directory is part of every user's directory and is purged periodically. This is done because of sftp's inability to recursively delete a directory. But I guess you know this. It's indeed bad to make webrev.sh specific to one host so I made the trash directory configurable (via env var) and added a note to the man page. > 273: Does rename fail if the target name already exists? Yes. That's why it is using process ID to lower the possibility. > 280: Is this useful to the user? Yes, because check is non-zero and this means -D was used by itself, i.e. the user explicitly wants to remove the remote webrev. It is fair to tell him that it failed. > 2475: s/implicates/implies/ fixed. Incremental webrev is here: http://cr.opensolaris.org/~vkotal/webrev_delete-6771203.onnv.rd1/ v.
