John Plocher wrote: <snip>
> I'm not a webrev expert, but here's a few nits... > > usr/src/tools/scripts/webrev.1: > 371 For custom remote targets, -t option allows to specify all > components: > > Better: -t option allows one to provide a completely specified upload > target > > Q: Does this override the use of other options (such as -o...)? > if so, maybe the options that are overridden/ignored when -t is > used could be enumerated here as well. Yes, -t overrides -o when the URI is fully specified. Added a note. > usr/src/tools/scripts/webrev.sh: > > 164 # end source directory with backslash in order to copy just > 165 # directory contents, not the whole directory > 166 $RSYNC -r -q $WDIR/ $dst > > > Nit: The "/" character is a slash, not a backslash (which is "\") fixed. > 1873 function usage > > The usage message doesn't show the same set of options as implemented > in the getopt loop... D'oh, fixed. > 2010 # more sanity checking > 2011 if [[ -n $nflag && -z $Uflag ]]; then > 2012 print "it does not make sense to skip webrev generation > without -U" > 2013 exit 1 > 2014 fi > > > ReallySmallNit: Usually, -n means "do everything, but only tell me > what you would have done instead of actually performing the actions". > This usage of "skip this step" seems odd. If it is not too late to > change, maybe "-g" for "don't generate"...? I'd rather leave the -n for the do-not-generate case because: - webrev implicitly always generates a webrev. If there is something which it should not do, it will be the do-not-generate-option. - it's hard to tell whether -n is used in the majority of cases for the write-but-do-not-perform action. In some cases, it is used to suppress only part of the functionality, e.g. naming lookups (SSH, ping) which sets a precedent. - using -g (or any other lower case letter besides 'n') suggests 'do something' as opposed to the negation Maybe -N would be better than -n ? incremental webrev uploaded to: http://cr.opensolaris.org/~vkotal/webrev_upload.onnv.rd2/ v.
