Re: [PATCH 1/8] am: suppress error output from a conditional

From

Ramkumar Ramachandra <artagnon@gmail.com>

Date

May 11, 2013, 23:38 UTC

Message-ID

<CALkWK0mtJU_fWK7A2BXN3f5VRT4Qh69PE0nfQaTJ6GU1Z5=wig@mail.gmail.com>

In-Reply-To

<7vhaia51vh.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:

> If you are introducing "dotest exists but next/last may not be
> present" as a valid new state, it probably should check the presence
> and/or absence of them explicitly,

> but more importantly, it is a
> good idea to move "test $# != 0" higher.

This?

diff --git a/git-am.sh b/git-am.sh
index 47c1021..1e10e31 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -446,9 +446,9 @@ done
 # If the dotest directory exists, but we have finished applying all the
 # patches in them, clear it out.
 if test -d "$dotest" &&
+   test $# != 0 &&
    last=$(cat "$dotest/last" 2>/dev/null) &&
    next=$(cat "$dotest/next" 2>/dev/null) &&
-   test $# != 0 &&
    test "$next" -gt "$last"
 then
    rm -fr "$dotest"

> Earlier it did not matter
> because when $dotest existed, next/last were supposed to be there
> and absence of them was an error codepath. Now, missing these files
> is not an error but is a perfectly normal state,

It's not a "normal state" for the purposes of git-am.sh.  There is no
codepath in the program that depends only on $dotest, but not
next/last.  I would frame this as: "checking for $dotest is not a
sufficient prerequisite anymore; we have to additionally look for
next/last".  To git-am.sh, an empty $dotest or just a
$dotest/autostash is equivalent to no $dotest at all.

> so checking what
> can be checked more cheaply makes sense as a general code hygiene.

Yeah, I agree.  See "am: tighten a conditional that checks for
$dotest", where I get away with just checking $dotest/last (and assume
that $dotest/next is present by extension).  How do I apply your
suggestion to this particular patch?