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