> From roland.mainz at nrubsig.org Thu Sep 4 11:49:28 2008 > > John Zolnowsky x69422/408-404-5064 wrote: > > > > I'm seeking codereview for my changes to relieve a minor annoyance. > > > > The webrev is at > > http://cr.opensolaris.org/~johnz/webrev.6738408/ > > > > The big insertion and deletion are just the relocation of the otherwise > > unmodified which_scm tests to immediately after the determination of > > the workspace and parent workspace paths. > > >From http://cr.opensolaris.org/~johnz/webrev.6738408/on-jzx.patch (patch > code is quoted with "> " and yes... I know you're just coping the code > around but the cleanup would be nice): > > +# Echo the SCM types of $CODEMGR_WS and $BRINGOVER_WS > > +function wstypes { > > + typeset parent_type child_type > > + > > Please define the variable "junk" as local variable.
Done. > > + env CODEMGR_WS=$BRINGOVER_WS $WHICH_SCM 2>/dev/null | read parent_type > > junk > > Please change this to: > -- snip -- > CODEMGR_WS="$BRINGOVER_WS" "$WHICH_SCM" 2>/dev/null | read parent_type junk > -- snip -- > e.g. add double-quotes and the "env" is AFAIK not neccesary. Done. I'm declining your remaining suggestions: > [snip] > > + > > + echo $child_type $parent_type > > +} > > Please use: > -- snip -- > printf "%s %s\n" "$child_type" "$parent_type" > -- snip -- > (see http://www.opensolaris.org/os/project/shell/shellstyle/#avoid_echo) The echo is not being used to format output, but rather to pass results back to the invoking instance. To me, the suggested change lowers readability, replacing a common idiom with an unusual form requiring an otherwise unnecessary format. > ... and at the end of the function "wstypes" a "return 0" is missing > (basically > http://www.opensolaris.org/os/project/shell/shellstyle/#use_proper_exit_code). If the echo happens to fail, the function should fail. > From Mark.J.Nelson at Sun.COM Thu Sep 4 13:21:19 2008 > > Because of this logic: > > 73 WHICH_SCM=$(dirname $nightly_path)/which_scm > 74 if [[ ! -x $WHICH_SCM ]]; then > 75 WHICH_SCM=which_scm > 76 fi > > ...it seems like the wstypes function should not be moved before the > PATH determination logic at 1539-1542. > > Philosophically, I don't have a strong preference for "as early as > possible" vs "as late as possible" to run this code, it seems like > you've gone for (and maybe overshot) the early side, and I might have > pegged just before the "Decide whether to clobber" comment on the late side. As suggested, I've moved the "wsytpe" block to just before the "Decide to clobber" block. I've also removed the #ident vestige. -JZ
