Vladimir Kotal wrote:
Mark J. Nelson wrote:
Howdy--
Heya,
http://cr.opensolaris.org/~mjnelson/onnv-bugfixes/
That webrev has been updated, plus an incremental:
http://cr.opensolaris.org/~mjnelson/webrev.incr/
This wad addresses
6830663 webrev -n -U, but without -o, no longer works after 6820421
Thanks for the quick response.
You might want to call external commands the ksh-way, using the $()
brackets, lines 2274, 2285, 2286 (and comments at 2267, 2281, 2282).
Ok.
The command at 2275 should be performed using && and checked if
codemgr_ws is non-empty afterwards I believe.
Implying that "workspace name" might have returned nothing, even though
which_scm returned "teamware" above?
I'm more inclined to modify the catch-all, so intead of being an "else"
for the series of SCM_MODE tests, it's just "if not set by any of the
previous code." See the incremental webrev if that's not clear.
Also, you might consider adding some sort of ASSERT at line 2448 to
check CWS is indeed set/non-empty to support the comment at 2444-2447.
The whole block at 2263-2314 makes sure CWS is always set (however,
checking if CWS is a directory should probably be done for the else
branch at 2308 since CODEMGR_WS grabbed from the environment could be
bogus) but preventing future code from breaking things does not hurt.
The need for such an assertion should be obviated by the previous change.
I'm not inclined to protect from bad env settings for CODEMGR_WS.
Mostly I have just refactored this code.
Otherwise this looks good.
Thanks for the review.
2 comments, not related to this fix:
- going through the script, it seems it has grown over the limit of
maintainability - it should probably be split into multiple files
according to the functionality (HTML/PS stuff, misc, SCM specific stuff).
Agreed. Seems like everybody that touches webrev says "wouldn't it be
great if..." But nobody has time. I plead the same.
- for the 3 supported SCMs the script is calling the commands
(workspace for Teamware, hg for Mercurial, svn for Subversion) directly.
Checking for the presence of all the commands is not the right thing to
do (not everyone has access to Teamware tools) but the script should be
more robust in case the commands are not present (lines 2274, 2286, 2297).
Agreed, but "not this fix."
--Mark
_______________________________________________
tools-discuss mailing list
tools-discuss@opensolaris.org
