On Fri 13 Jun 2008 at 11:40PM, Mark J. Nelson wrote:
>
> Standard apology for folks on lots of e-mail lists...
>
I reviewed webrev.1 and webrev.sh:
webrev.1:
1) It would be nice if webrev offered a rendered nroff view, reviewing raw
nroff for content is hard. Off topic, I realize.
2) It might be a good idea to call out at the top of the man page that
webrev is applicable to Teamware, Mercurial, and Subversion users.
perhaps:
"webrev builds a set of HTML files suitable for performing code review
of source changes in a web browser. It supports Mercurial, Subversion,
and Teamware repositories. At its most basic ..."
And if there's a dependency on the cadmium extension, you might want
to call that out too. (FYI-- you might want to also depend against
lack of cadmium if it is indeed needed).
webrev.sh:
In general, looks pretty good. Nice to see that my previous round
of changes was worth the effort.
---
Strongly suggest that span.chmod be styled in something other than
bright red. When I've seen this in recent code reviews it has stuck out
at me that this is indicating an error message. Orange might be better,
or something.
---
Could you do me a favor and fix the comment at 1072
(teamwarecomments --> comments_from_teamware)
Ditto 1112, wxcomments --> comments_from_wx
Similarly, 1154 should follow preceding functions and have
a header comment. It's not incredibly obvious what the arguments
are.
---
Is the change at 1216 (eval) stylistic, or is there an effect?
---
usage(): usage should probably be updated to reflect limited
usefulness of -l option. Perhaps move it into the SCM environment
section (and maybe retitle that section "TeamWare Specific Options").
---
1493: Should be $PERL instead of hardcoding. The last... 20 or so
nevada builds also have /usr/bin/stat, which seems like a nice tool for
doing this.
---
1464(old)/1753(in the new code): nothing to update here? I noticed
that hg-active seems to emit HG_PARENT. Is that something settable
here?
---
1629 (and several others): "parents" -> "parent's"
---
1779: should look for which_scm and set it as WHICH_SCM, and exit
gracefully if it cannot be found, especially since which_scm is
a new tool you are introducing.
---
1877: which_scm --> $WHICH_SCM
---
2318,2657: I thought the old code, which had the cat at the
top, was clearer: you could tell what the input to the loop
is without having to find the loop termination point.
-dp
--
Daniel Price - Solaris Kernel Engineering - dp at eng.sun.com - blogs.sun.com/dp
