I don't have any other comments. --Mark
>>>> 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. > _______________________________________________ > scm-migration-dev mailing list > scm-migration-dev at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/scm-migration-dev
