On Wed 11 Oct 2006 at 08:49AM, Stacey Marshall wrote: > This is looking real good :-) > I like the new 'raw' file, most useful :-) > I wonder if 'new' and 'old' should simply be the raw files??? > Anyhow, I have looked at webrev.sh: > > line 694: > < # relative_cws > > # relative_dir
Fixed. > line 706: this should be a composite pattern as suggested by Sean > Sprague in thread > http://www.opensolaris.org/jive/message.jspa?messageID=48859#48859 > < typeset cur="${1##$2(/)}" > > typeset cur="${1##$2?(/)}" Fixed. > line 1022: blank line contains space characters. Fixed. > line 1158 & 1543: Make these a ksh test "[[ ]]" Fixed. > line 1386: Nothing wrong here with this 'logical if', but line 1392 > has similar test with 'statement if'... I've no strong feeling > about this but should we stick to one style? Fixed. > line 1442: The 'f' appears to be missing: > < -p <compare-against>: use specified parent wkspc or basis of comparison > > -p <compare-against>: use specified parent wkspc for basis of comparison > This also needs addressing in webrev.1 No, the message is correct. It means: use specified parent workspace OR basis-for-comparison. I changed it slightly to be more clear. > line 1522: I'm not sure what is being accomplished within this block, > I've yet to try out this functionality. A comment would be most > welcome. Done. > line 1597: if the wx/active file is not specified and not > auto-detected should we not exit? Sure. Fixed. > line: 1683: Blank space at EOL. Fixed. > line 1813 & 2162: The use of 'cat' would not be necessary if the input was > read directly from the file using stdin redirection > "while read line; do ... done < filename" > However, that said the, this way the filename is nearer > the actual use of the file! So happy to leave this ;-) My thought exactly... > line 1837, 1844, 1851 (and possibly others): the use of "==" is > preferred in ksh test "[[ ]]". Really? I couldn't find anything in the test or ksh man page about this. How do I learn these preferences? > line 1346: Specific use of /bin/rm here, but not else where! Fixed. Thanks for your review... -dp -- Daniel Price - Solaris Kernel Engineering - [EMAIL PROTECTED] - blogs.sun.com/dp _______________________________________________ tools-discuss mailing list tools-discuss@opensolaris.org
