Richard Lowe writes:
> A webrev is available for prelim code review:
>
> http://cr.opensolaris.org/~richlowe/scm-review_90
Completing the review notes from yesterday:
usr/src/tools/onbld/Scm/WorkSpace.py
Contains a number of XXX comments; these should be removed prior to
putback. (If they have useful information, then that should be
moved over to bugs.)
515: this seems to occur in a lot of cases, including just listing
the active files. Something likely eventually needs to be done
about this. (Not sure if it's a cache or something else.)
I'll admit I didn't read a lot of the rest of the code. I frankly
don't understand it.
usr/src/tools/onbld/hgext/cdm.py
23-25: looks like text that belongs in an RFE instead.
usr/src/tools/scripts/Makefile
73: nit: this didn't have to move (and now it's out of order)
usr/src/tools/scripts/cddlchk.1
34: s/obsolute/obsolete/
34: should expand CDDL at least once.
41: nit: s/they/any/
73: s/Invalvid/Invalid/
usr/src/tools/scripts/copyrightchk.py
41,54-55: nit: stray blank lines.
48: why sys.exit? Why not continue to next file?
usr/src/tools/scripts/cstyle.pl
194,203,211: why set this flag only when we print errors? It seems
to me that even if $no_errs is set, we should still return an error
code when something is wrong. (Shouldn't we?)
usr/src/tools/scripts/flg.flp.sh
94: the egrep doesn't look right to me -- shouldn't this be more
like:
"^($dirs)(/.*/|/)${path#s.}\$"
The pattern that's there doesn't seem to match $(dirs)/${path#s.}
(i.e., files in the top level of a $dirs entry).
154: what does this change do?
usr/src/tools/scripts/hdrchk.1
63: ?
usr/src/tools/scripts/hdrchk.py
64: should probably go to stderr.
usr/src/tools/scripts/hgsetup.1
37,39: nit: use 'and' rather than '&'.
usr/src/tools/scripts/hgsetup.sh
54: could possibly default to `id -...@`hostname` or some such.
77: nit: s/a pre-/an /
98: the first path searched should be just "$(dirname $(whence $0))/.." --
this allows for a relocatable SUNWonbld package. (In fact, I think
it's hard to see how the other paths would ever be used ...)
usr/src/tools/scripts/nightly.1
275: should add back in "This is the source to be built."
293: or pulls, right?
usr/src/tools/scripts/nightly.sh
382-383,2548-2549: what is this? It looks out of place.
1707: uuoc
usr/src/tools/scripts/webrev.1
135: missing .PP?
140: just a style nit: I think \fP is preferred over \fR, so that
attributes can nest.
147-153: this isn't quite true anymore. It should probably
reference which_scm(1), and note that it makes a special check for
"-p" pointing to a prior webrev.
155-157: text probably not needed now.
158: missing control of some sort (.PP)?
175-179: not quite true; it gets the information from which_scm.
186-191: similar.
206: does this actually do what you want? (I think you've got an
italic '.' in the output. Not that it's noticeable ...)
238: nit: wx (1) (missing space)
345-348: need to pick one style. Either make all of foo(n) italic,
or just foo italic.
usr/src/tools/scripts/wx2hg.1
139-140,143: this appears to be stale. hgmerge is gone.
usr/src/tools/scripts/wx2hg.sh
115: does this work?
--
James Carlson, Solaris Networking <james.d.carlson at sun.com>
Sun Microsystems / 35 Network Drive 71.232W Vox +1 781 442 2084
MS UBUR02-212 / Burlington MA 01803-2757 42.496N Fax +1 781 442 1677
