Re: [PATCH] am: handle stray $dotest directory case

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 13, 2013, 17:09 UTC
Message-ID: <7v61xivsxh.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1371133031-28049-1-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Show 13 quoted lines

> The following bug has been observed since rr/rebase-autostash:
>
>   $ git am  # no input file
>   ^C
>   $ git am --abort
>   Resolve operation not in progress, we are not resuming.
>
> This happens because the following test fails:
>
>   test -d "$dotest" && test -f "$dotest/last" && test -f "$dotest/next"
>
> and am precludes the possibility of a stray $dotest directory
> existing (when $dotest/{last,next} are not present).

Why did the original code sequence that read:

	if test -d "$dotest"
        then
		... handle skip, resolved, abort, because
                ... these can be run ONLY when we know we have
                ... started an "am" session.
		... catch new "git am mbox" invocation and error
                ... out, because that should not be allowed when
                ... we know we have started an "am" session.

had to change its guarding condition to

	if test -d "$dotest" && test -f "$dotest/last" && test -f "$dotest/next"

in the first place? Perhaps _that_ guarding condition is what needs to be fixed, by reverting it back to just "does $dotest exist?"

Adding a single case workaround smells like a band-aid to me.

Show 25 quoted lines

> Fix the bug by checking for a stray $dotest directory explicitly and
> removing it on --abort.
>
> Reported-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  git-am.sh | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/git-am.sh b/git-am.sh
> index 1cf3d1d..f46a123 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -506,6 +506,11 @@ then
>  	esac
>  	rm -f "$dotest/dirtyindex"
>  else
> +	# Possible stray $dotest directory
> +	if test -d "$dotest" && test t = "$abort"; then
> +		clean_abort
> +	fi
> +
>  	# Make sure we are not given --skip, --resolved, nor --abort
>  	test "$skip$resolved$abort" = "" ||
>  		die "$(gettext "Resolve operation not in progress, we are not resuming.")"