Re: [PATCH 1/3] vimdiff: new implementation with layout support

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, Nov 7, 2021 at 5:42 PM David Aguilar <davvid@gmail.com> wrote:
> On Thu, Nov 4, 2021 at 9:10 AM Fernando Ramos <greenfoo@u92.eu> wrote:
> > +       AUX=$(echo "$LAYOUT" | grep -oe "[A-Z]\+\*")
>
> From Documentatin/CodingGuidelines:
>
> As to use of grep, stick to a subset of BRE (namely, no {m,n},
>    [::], [==], or [..]) for portability.

Also, `grep -o` isn't POSIX.

> > +       if test $(echo $LAYOUT | wc -w) == "1"
> > +       then
> > +               CMD="$CMD | bufdo diffthis"
> > +        else
> > +               CMD="$CMD | tabdo windo diffthis"
> > +       fi
>
> The output of "wc -c" is non-portable. It contains leading whitespace
> on some platforms.
>
> The test expression should be:
>
>    test "$value" = 1
>
> with a single "=" rather than "==".

For clarification, the leading whitespace emitted by some `wc`
implementations is only a problem when encapsulated in a string. For
instance, like this:

    if test "$(... | wc -w)" = "1"

in which case "  1"  won't equal "1". The usage here, however, should
be okay since the output is not quoted.

Quite correct about using "=" (or even "-eq") here rather than "==", though.

> > +       if $base_present
> > +       then
> > +               eval "$merge_tool_path" \
> > +                       -f $FINAL_CMD "$LOCAL" "$BASE" "$REMOTE" "$MERGED"
> > +       else
> > +               [...]
> > +               eval "$merge_tool_path" \
> > +                       -f $FINAL_CMD "$LOCAL" "$REMOTE" "$MERGED"
> > +       fi
> > +
> > +       ret="$?"
> > +       if test "$ret" -eq 0
>
> This should be:
>
>     if test "$ret" = 0

Or simpler, no need for `ret` at all:

    if test $? -eq 0

(or `if test $? = 0` -- either works)

Another (perhaps better) alternative is to assign the result of `eval`
to `ret` at the point of invocation, which lessens the cognitive load
a bit since you don't have to scan backward through the code trying to
figure out what $? refers to.

Also, why is `eval` needed here? Is there something non-obvious going
on? (Genuine question; I didn't trace the code thoroughly to
understand.)

> > +                       eval cp -- \$"$FINAL_TARGET" "$MERGED"
>
> This eval may not be safe when the value contains whitespace or shell
> metacharacters.
>
> I think it might be better to just spell it out and be explicit.
>
> It's more code but it'll be easier to follow:
> [...]
> if test -n "$source_path"
> then
>     cp -- "$source_path" "$MERGED"
> fi

I suspect `--` also needs to be avoided since it is not POSIX.