Quoth Dan Price on Sun, Oct 01, 2006 at 09:52:59PM -0700:
> http://cr.grommit.com/~dp/webrev.3/
tools/scripts/webrev.1
33: It looks like you have an unmatched ']' at the end of this line.
37: Aren't these options already listed in the options section?
39:
- Doesn't change not the bugids/arc cases printed, but the links in
them? Doesn't it also hide files in usr/closed?
- The rest of the option descriptions are not capitalized.
46: Maybe my nroff is rusty, but isn't the non-italic stuff supposed
to be on a separate line? Or is that what \fR does?
50: Do you mean "for basis of comparison"? Do we really need to
abbreviate workspace?
58: A style nit, but I think "... suitable for reviewing source
changes..." would be better.
64: I think this reference to teamware implies that we should avoid
using webrev on TeamWare workspaces. I think you mean the use of
putback may take a long time. In that case, I think you should
specify the -l option instead.
84: Manual edits will be overwritten, won't they? That should be
noted.
89: I think you should specify the reason for seeing the -i option,
like "To include a specific [different] file, use the -i option."
96: Doesn't webrev determine my workspace if I'm cd'ed into it? This
paragraph should be rewritten with that in mind.
100: I think you should prepend a dollar sign to CODEMGR_PARENT.
146: Shouldn't this be "all of OS/Net usr/src"?
165: This should probably explain the interaction with $WDIR.
167: Why is this separate from -w on line 124?
215: Did you have Yoda write this sentence? How about
To compare the workspace against one other than the parent,
include a CODEMGR_PARENT line in the file list, like
243: Should you mention the format for renamed files here?
282: I think "bug to HTML" should be hyphenated.
294: I wonder if this should be CODEMGR_PARENT-relative, rather than
hardcoded to /ws/onnv-gate.
298: Perhaps you should add yourself to the acknowledgements section.
tools/scripts/wdiff.pl
77: Should some of this be in a style file shared with the other HTML
files?
88-101: Why not use the lineref? This seems less accurate.
105: If you don't specify the font-family, what happens?
212-3: These changes seem superfluous.
old 296: You're no longer setting the width of the hb-elided elements.
That was the whole point of setting widths: to tell whether the code
is wider than 80 columns.
tools/scripts/webrev.sh
HTML= & FRAMEHTML=: These seem mostly the same. Can we combine them
with printf(1) or such?
76: This semicolon is unnecessary, isn't it?
129: Is the font-family necessary? Won't most browsers render
preformatted elements monospaced by default? And why are you
setting the font size explicitly?
154,164,174,585,636: Can you make these input_cmd & output_cmd?
154: Lines 428 & 431 also do "html_quote input_file | output_cmd".
199: Is this paragraph really necessary?
202: I think this sentence can be coalesced with the introductory
paragraph.
212s/changes/changed/
sdiff_to_html(): Would this be any simpler in perl?
257-8: It appears you're adding arguments. You should probably
document them in the preceding comment.
346-57: These lines are remarkably similar to 264-74. Did you
consider combining them?
427,430: Should /tmp/sN be prefixed with $$?
454: Does this need to be before the </body></html> line? It looks
like framed_sdiff directs all of its output to another file.
framed_sdiff: This comment should probably say where all of the
content is placed.
482: It seems like this might be better done with a heredoc.
479: Would this be better accomplished with <BASE>?
518: Does this not work with EOF indented? ksh(1) indicates that it
should, with <<-.
525: "Merge concatenate" seems like a grammatical error.
537: This seems unused.
554: text or what?
590: Shouldn't this be "a horizontal rule"?
596: I think you have invalidated this comment about color & font.
strip_unchanged():
- Can we put this closer to sdiff_to_html(), which is the only place
it's used?
- Wouldn't it be a lot easier to take the diff -u output and blank
the + lines for the left side and blank the - lines for the right
side? I suppose you'd have to do some computation for changed
lines.
645: You invalidated the font color portion of this comment.
694s/relative_cws/relative_dir/
1020-5: Would this be neater as a heredoc?
1066:
- I think it would be clearer if you passed $1 & $2 to html_quote.
- Didn't you change other files to use four columns for the line
number instead of three?
1378: You could reduce the indentation in this function by doing
[[ -r $FLIST ]] || return
1439,1444: These descriptions are capitalized, but the rest are not.
1522-3: Why not combine these conditions?
1751: If mkdir fails, won't it complain?
1766-7: I wonder if you should be using printf for this, so it will be
easy to adjust the indentation.
1883-4: I think you can do this with [[ $pclosed = usr/closed/* ]], or
with a case statement.
1894: Do you mean absolute paths in the output of diff?
1910: Blank line, please.
1913: Why are you using /bin/rm here, but not elsewhere?
1957: Isn't this redundant with 1953?
2113: This doesn't seem like good English.
2117: I think you can do this with
userupper=`perl -e "print ucfirst $username"`
2262: This change seems unnecessary.
2279: This seems like it's missing something.
David
_______________________________________________
tools-discuss mailing list
[email protected]
