On Sun, Nov 7, 2021 at 5:42 PM David Aguilar <davvid@xxxxxxxxx> wrote:
> On Thu, Nov 4, 2021 at 9:10 AM Fernando Ramos <greenfoo@xxxxxx> 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.
