Re: [PATCH 3/7] am: tighten a conditional that checks for $dotest

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 23, 2013, 16:45 UTC
Message-ID: <7v8v49jivl.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1366725724-1016-4-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> We currently assume that, if a $dotest directory exists, an am had
> been called earlier.  This assumption might get our conditional to
> match a stray $dotest directory created somewhere else,...

Hmm. that explanation sounds like that is sweeping a real issue under the rug. Shouldn't your patch fixing that creator of the stray one? That stray one that errorneously creates $dotest, even when it is not applying patch messages, may create the directory with 'last' but without 'next' or the messages, so checking only 'last' does not sound like solving any problem.

If this were done as a part of an addition that has to create $dotest even when it does not (yet) deposit patches in there, the way that particular addition creates and uses $dotest may justify why testing only 'last' is sufficient. But as a standalone change, neither the patch text or the above explanation makes much sense to me.

Show 10 quoted lines

> diff --git a/git-am.sh b/git-am.sh
> index 88aa438..f4ef8fc 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -454,7 +454,7 @@ then
>     rm -fr "$dotest"
>  fi
>  
> -if test -d "$dotest"
> +if test -d "$dotest" && test -f "$dotest/last"

>  then
>  	case "$#,$skip$resolved$abort" in
>  	0,*t*)