On Fri 06 Oct 2006 at 04:14PM, David Bustos wrote:
> Quoth Dan Price on Sun, Oct 01, 2006 at 09:52:59PM -0700:
> > http://cr.grommit.com/~dp/webrev.3/
David,
Thanks for the thorough review.
> tools/scripts/webrev.1
> 33: It looks like you have an unmatched ']' at the end of this line.
Fixed.
> 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.
...
> 50: Do you mean "for basis of comparison"? Do we really need to
> abbreviate workspace?
Ok, I'll just refer the user to the OPTIONS section and then improve
that.
> 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?
Yes, \fR goes back to regular.
> 58: A style nit, but I think "... suitable for reviewing source
> changes..." would be better.
I improved the language.
> 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.
I improved the language.
> 84: Manual edits will be overwritten, won't they? That should be
> noted.
Done.
> 89: I think you should specify the reason for seeing the -i option,
> like "To include a specific [different] file, use the -i option."
Done.
> 96: Doesn't webrev determine my workspace if I'm cd'ed into it? This
> paragraph should be rewritten with that in mind.
It doesn't do that for teamware. It may in the future for other
SCMs. I'll work on the language.
> 100: I think you should prepend a dollar sign to CODEMGR_PARENT.
Ok.
> 146: Shouldn't this be "all of OS/Net usr/src"?
I changed it to: (thousands of files)
> 165: This should probably explain the interaction with $WDIR.
Done.
> 167: Why is this separate from -w on line 124?
An error. Fixed.
> 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
Fixed.
> 243: Should you mention the format for renamed files here?
I'll look into it.
> 282: I think "bug to HTML" should be hyphenated.
Done.
> 294: I wonder if this should be CODEMGR_PARENT-relative, rather than
> hardcoded to /ws/onnv-gate.
Then you'd break people working on children of project gates I think.
Let's leave it as is for now, it would be easy to change if we get
complaints. I believe I also look for wdiff in $PATH, which should
help.
> 298: Perhaps you should add yourself to the acknowledgements section.
That rogue's gallery? (kidding). Ehh. Maybe.
> tools/scripts/wdiff.pl
> 77: Should some of this be in a style file shared with the other HTML
> files?
Maybe-- but it would make these tools less self contained, which
seems like a nice feature currently. The cost of keeping them
in sync is pretty small.
> 88-101: Why not use the lineref? This seems less accurate.
I was seeing this perform poorly (like, a noticable resize occurring)
especially when I fixed a tables bug in the code. But as I note
below, I'll go revisit these choices.
> 105: If you don't specify the font-family, what happens?
You get a monospace font, inherited from the div.
> 212-3: These changes seem superfluous.
You're not supposed to use unquoted literals for attribute values
in XHTML transitional, IIRC, so this falls under the XHTML compliance
bucket. (part of 6465389 webrev emits really crappy HTML).
> 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.
I'll go back and revisit. When I played with this it was causing
me a lot of grief, because all the cmdbox tables were nested on the
page, due to this bug:
181 -<table>
247 +</table>
But maybe I misunderstood the intent. Is the desired behavior for
the elision box to never be wider than 80 columns?
> tools/scripts/webrev.sh
I'll work through these later...
-dp
--
Daniel Price - Solaris Kernel Engineering - [EMAIL PROTECTED] - blogs.sun.com/dp
_______________________________________________
tools-discuss mailing list
tools-discuss@opensolaris.org
