Danek Duvall wrote:
> xref:
>
>> if [[ $SCM_MODE == "unknown" ]];then
>> print -u2 "Unable to determine SCM type currently in use."
>> print -u2 "For teamware: webrev looks for \$CODEMGR_WS either in"
>> print -u2 " the environment or in the file list."
>> print -u2 "For mercurial: webrev runs 'hg root'."
>> exit 1
>
> Why the references to webrev here? (Same applies to flg.flp.)
It looks to be where the code came from (which explains some of your other
comments).
>> elif [[ $SCM_MODE == "mercurial" ]]; then
>> hg_root=`hg root 2>/dev/null`
>> codemgr_ws=$hg_root
>> SRC=$codemgr_ws/usr/src export SRC
>> MACH=`uname -p` export MACH
>
> I would set MACH and SRC outside of this if, and get rid of the use of
> $hg_root -- just set codemgr_ws directly. Ditto with the teamware clause.
>
>> elif [[ $SCM_MODE == "teamware" ]]; then
>> codemgr_ws=$CODEMGR_WS
>
> You set codemgr_ws here, even though CODEMGR_WS is potentially set later in
> this clause. But why introduce codemgr_ws at all, instead of just using
> CODEMGR_WS throughout? I don't see a reason to avoid that environment
> variable in mercurial scenarios. (Same applies to flg.flp.)
There was much (much) discussion in that regard previously, it was voiced
that CODEMGR_* were a "TeamWare thing" and should not be used with
mercurial. I disagree entirely, but the discussion never really finished.
If we want to default to me winning, that's fine by me ;)
>> print -u2 " SCM detected: $SCM_MODE"
>
> This needs to be removed before integration (I assume it's just for
> debugging).
Also from webrev (I'm not sure why in that case, either).
>> make -e -f $XREFMK xref.flg
>
> You removed the "> /dev/null" here. Why?
I'd assume debugging.
>
> flg.flp:
>
>> #
>> # detect_scm
>> #
>> # We dynamically test the SCM type; this allows future extensions to
>> # new SCM types
>> #
>> function detect_scm
>> {
>> #
>> # The presence of $CODEMGR_WS and a Codemgr_wsdata directory
>> # is our clue that this is a teamware workspace.
>> #
>> if [[ -n $CODEMGR_WS && -d "$CODEMGR_WS/Codemgr_wsdata" ]]; then
>
> Wouldn't the output of workspace name be more appropriate here?
This is verbatim from dp's webrev cleanup work (4921786 et al.) If I recall
the email exchange correctly, Dan wasn't aware that 'workspace name' would
behave in all cases.
>> elif [ "$SCM_MODE" = "mercurial" ]; then
>> for dir; do
>> if [ -d $codemgr_ws/$dir ]; then
>> cd $codemgr_ws
>> hg locate "glob:${dir}/${pat##s.}" | prpath
>
> You just need a single '#' there. But I'm not sure that this does the same
> thing as the find command in the teamware clause above. There, if $pat is
> "Makefile", then it'll find all Makefiles under $dir. This will simply
> find $dir/Makefile.
That would be my fault, when giving examples. Sorry. (the ## would be my
fault, too)
> Getting this working right runs into mercurial bug 108:
>
> http://www.selenic.com/mercurial/bts/issue108
>
> for which there's a workaround. So I think you probably want something
> like
>
> hg locate "glob:$dir/**/${pat#s.}"
>
> It's a bit sad that this takes ten or twelve times as long as the find
> command. You could use something like
>
> hg manifest | grep "^$dir/.*/${pat#s.}\$"
>
> which is faster, but still not as fast as the find command.
>
When I was looking at this, the difference between the last two was minimal,
but the difference from 'normal' was high. The bug on grommit mentions
flg.flp -r in $SRC/uts taking 15 minutes (versus a handful of seconds), when
I was trying.
-- Rich
