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,

Um, a test -f preceding the cat?  Why, when cat implicitly checks
existence anyway?

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


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"
    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?
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to