[PATCH v2 0/2] Fix am with stray $dotest directory

[PATCH v2 0/2] Fix am with stray $dotest directory

[Ramkumar Ramachandra]

[1/2] is now equipped to handle any am invocation in the presence of a
stray $dotest directory.

[2/2] is a "while we're there".

Thanks.

Ramkumar Ramachandra (2):
  am: handle stray $dotest directory
  t/am: use test_path_is_missing() where appropriate

 git-am.sh     | 14 ++++++++++++++
 t/t4150-am.sh | 40 +++++++++++++++++++++++-----------------
 2 files changed, 37 insertions(+), 17 deletions(-)

-- 
1.8.3.1.380.g67e7d64.dirty

[PATCH 1/2] am: handle stray $dotest directory

[Ramkumar Ramachandra]

The following bug has been observed:

  $ 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 the codepath for an "am in-progress" is not executed.  It falls back
to the codepath that treats this as a "fresh execution".  Before
rr/rebase-autostash, this condition was

  test -d "$dotest"

It would incorrectly execute the "normal" am --abort codepath:

  git read-tree --reset -u HEAD ORIG_HEAD
  git reset ORIG_HEAD

by incorrectly assuming that an am is "in progress" (i.e. ORIG_HEAD
etc. was written during the previous execution).

Notice that

  $ git am
  ^C

executes nothing of significance, is equivalent to

  $ mkdir .git/rebase-apply

Therefore, the correct solution is to treat .git/rebase-apply as a
"stray directory" and remove it on --abort in the fresh-execution
codepath.

While at it, tell the user to run "git am --abort" to get rid of the
stray $dotest directory, if she attempts anything else.

Reported-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 git-am.sh     | 14 ++++++++++++++
 t/t4150-am.sh |  6 ++++++
 2 files changed, 20 insertions(+)

diff --git a/git-am.sh b/git-am.sh
index 1cf3d1d..37ee18b 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -506,6 +506,20 @@ then
 	esac
 	rm -f "$dotest/dirtyindex"
 else
+	# Possible stray $dotest directory
+	if test -d "$dotest"; then
+	case "$skip,$resolved,$abort" in
+		,,t)
+			rm -fr "$dotest"
+			exit 0
+			;;
+		*)
+			die "$(eval_gettext "Stray $dotest directory found.
+Use \"git am --abort\" to remove it.")"
+			;;
+	esac
+	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.")"
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 12f6b02..6c2cc3e 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -363,6 +363,12 @@ test_expect_success 'am --skip works' '
 	test_cmp expected another
 '
 
+test_expect_success 'am --abort removes a stray directory' '
+	mkdir .git/rebase-apply &&
+	git am --abort &&
+	test_path_is_missing .git/rebase-apply
+'
+
 test_expect_success 'am --resolved works' '
 	echo goodbye >expected &&
 	rm -fr .git/rebase-apply &&
-- 
1.8.3.1.380.g67e7d64.dirty

[PATCH 2/2] t/am: use test_path_is_missing() where appropriate

[Ramkumar Ramachandra]

Replace instances of ! test -d with test_path_is_missing.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t4150-am.sh | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 6c2cc3e..5edb79a 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -147,7 +147,7 @@ test_expect_success 'am applies patch correctly' '
 	git checkout first &&
 	test_tick &&
 	git am <patch1 &&
-	! test -d .git/rebase-apply &&
+	test_path_is_missing .git/rebase-apply &&
 	git diff --exit-code second &&
 	test "$(git rev-parse second)" = "$(git rev-parse HEAD)" &&
 	test "$(git rev-parse second^)" = "$(git rev-parse HEAD^)"
@@ -158,7 +158,7 @@ test_expect_success 'am applies patch e-mail not in a mbox' '
 	git reset --hard &&
 	git checkout first &&
 	git am patch1.eml &&
-	! test -d .git/rebase-apply &&
+	test_path_is_missing .git/rebase-apply &&
 	git diff --exit-code second &&
 	test "$(git rev-parse second)" = "$(git rev-parse HEAD)" &&
 	test "$(git rev-parse second^)" = "$(git rev-parse HEAD^)"
@@ -169,7 +169,7 @@ test_expect_success 'am applies patch e-mail not in a mbox with CRLF' '
 	git reset --hard &&
 	git checkout first &&
 	git am patch1-crlf.eml &&
-	! test -d .git/rebase-apply &&
+	test_path_is_missing .git/rebase-apply &&
 	git diff --exit-code second &&
 	test "$(git rev-parse second)" = "$(git rev-parse HEAD)" &&
 	test "$(git rev-parse second^)" = "$(git rev-parse HEAD^)"
@@ -180,7 +180,7 @@ test_expect_success 'am applies patch e-mail with preceding whitespace' '
 	git reset --hard &&
 	git checkout first &&
 	git am patch1-ws.eml &&
-	! test -d .git/rebase-apply &&
+	test_path_is_missing .git/rebase-apply &&
 	git diff --exit-code second &&
 	test "$(git rev-parse second)" = "$(git rev-parse HEAD)" &&
 	test "$(git rev-parse second^)" = "$(git rev-parse HEAD^)"
@@ -206,7 +206,7 @@ test_expect_success 'am changes committer and keeps author' '
 	git reset --hard &&
 	git checkout first &&
 	git am patch2 &&
-	! test -d .git/rebase-apply &&
+	test_path_is_missing .git/rebase-apply &&
 	test "$(git rev-parse master^^)" = "$(git rev-parse HEAD^^)" &&
 	git diff --exit-code master..HEAD &&
 	git diff --exit-code master^..HEAD^ &&
@@ -258,7 +258,7 @@ test_expect_success 'am --keep really keeps the subject' '
 	git reset --hard &&
 	git checkout HEAD^ &&
 	git am --keep patch4 &&
-	! test -d .git/rebase-apply &&
+	test_path_is_missing .git/rebase-apply &&
 	git cat-file commit HEAD >actual &&
 	grep "Re: Re: Re: \[PATCH 1/5 v2\] \[foo\] third" actual
 '
@@ -268,7 +268,7 @@ test_expect_success 'am --keep-non-patch really keeps the non-patch part' '
 	git reset --hard &&
 	git checkout HEAD^ &&
 	git am --keep-non-patch patch4 &&
-	! test -d .git/rebase-apply &&
+	test_path_is_missing .git/rebase-apply &&
 	git cat-file commit HEAD >actual &&
 	grep "^\[foo\] third" actual
 '
@@ -283,7 +283,7 @@ test_expect_success 'am -3 falls back to 3-way merge' '
 	test_tick &&
 	git commit -m "copied stuff" &&
 	git am -3 lorem-move.patch &&
-	! test -d .git/rebase-apply &&
+	test_path_is_missing .git/rebase-apply &&
 	git diff --exit-code lorem
 '
 
@@ -297,7 +297,7 @@ test_expect_success 'am -3 -p0 can read --no-prefix patch' '
 	test_tick &&
 	git commit -m "copied stuff" &&
 	git am -3 -p0 lorem-zero.patch &&
-	! test -d .git/rebase-apply &&
+	test_path_is_missing .git/rebase-apply &&
 	git diff --exit-code lorem
 '
 
@@ -307,7 +307,7 @@ test_expect_success 'am can rename a file' '
 	git reset --hard &&
 	git checkout lorem^0 &&
 	git am rename.patch &&
-	! test -d .git/rebase-apply &&
+	test_path_is_missing .git/rebase-apply &&
 	git update-index --refresh &&
 	git diff --exit-code rename
 '
@@ -318,7 +318,7 @@ test_expect_success 'am -3 can rename a file' '
 	git reset --hard &&
 	git checkout lorem^0 &&
 	git am -3 rename.patch &&
-	! test -d .git/rebase-apply &&
+	test_path_is_missing .git/rebase-apply &&
 	git update-index --refresh &&
 	git diff --exit-code rename
 '
@@ -329,7 +329,7 @@ test_expect_success 'am -3 can rename a file after falling back to 3-way merge'
 	git reset --hard &&
 	git checkout lorem^0 &&
 	git am -3 rename-add.patch &&
-	! test -d .git/rebase-apply &&
+	test_path_is_missing .git/rebase-apply &&
 	git update-index --refresh &&
 	git diff --exit-code rename
 '
@@ -358,7 +358,7 @@ test_expect_success 'am pauses on conflict' '
 test_expect_success 'am --skip works' '
 	echo goodbye >expected &&
 	git am --skip &&
-	! test -d .git/rebase-apply &&
+	test_path_is_missing .git/rebase-apply &&
 	git diff --exit-code lorem2^^ -- file &&
 	test_cmp expected another
 '
@@ -379,7 +379,7 @@ test_expect_success 'am --resolved works' '
 	echo resolved >>file &&
 	git add file &&
 	git am --resolved &&
-	! test -d .git/rebase-apply &&
+	test_path_is_missing .git/rebase-apply &&
 	test_cmp expected another
 '
 
@@ -388,7 +388,7 @@ test_expect_success 'am takes patches from a Pine mailbox' '
 	git reset --hard &&
 	git checkout first &&
 	cat pine patch1 | git am &&
-	! test -d .git/rebase-apply &&
+	test_path_is_missing .git/rebase-apply &&
 	git diff --exit-code master^..HEAD
 '
 
@@ -397,7 +397,7 @@ test_expect_success 'am fails on mail without patch' '
 	git reset --hard &&
 	test_must_fail git am <failmail &&
 	git am --abort &&
-	! test -d .git/rebase-apply
+	test_path_is_missing .git/rebase-apply
 '
 
 test_expect_success 'am fails on empty patch' '
@@ -406,7 +406,7 @@ test_expect_success 'am fails on empty patch' '
 	echo "---" >>failmail &&
 	test_must_fail git am <failmail &&
 	git am --skip &&
-	! test -d .git/rebase-apply
+	test_path_is_missing .git/rebase-apply
 '
 
 test_expect_success 'am works from stdin in subdirectory' '
-- 
1.8.3.1.380.g67e7d64.dirty

Re: [PATCH 1/2] am: handle stray $dotest directory

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> The following bug has been observed:
>
>   $ 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 the codepath for an "am in-progress" is not executed.  It falls back
> to the codepath that treats this as a "fresh execution".  Before
> rr/rebase-autostash, this condition was
>
>   test -d "$dotest"
>
> It would incorrectly execute the "normal" am --abort codepath:
>
>   git read-tree --reset -u HEAD ORIG_HEAD
>   git reset ORIG_HEAD
>
> by incorrectly assuming that an am is "in progress" (i.e. ORIG_HEAD
> etc. was written during the previous execution).
>
> Notice that
>
>   $ git am
>   ^C
>
> executes nothing of significance, is equivalent to
>
>   $ mkdir .git/rebase-apply
>
> Therefore, the correct solution is to treat .git/rebase-apply as a
> "stray directory" and remove it on --abort in the fresh-execution
> codepath.
>
> While at it, tell the user to run "git am --abort" to get rid of the
> stray $dotest directory, if she attempts anything else.
>
> Reported-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  git-am.sh     | 14 ++++++++++++++
>  t/t4150-am.sh |  6 ++++++
>  2 files changed, 20 insertions(+)
>
> diff --git a/git-am.sh b/git-am.sh
> index 1cf3d1d..37ee18b 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -506,6 +506,20 @@ then
>  	esac
>  	rm -f "$dotest/dirtyindex"
>  else
> +	# Possible stray $dotest directory
> +	if test -d "$dotest"; then
> +	case "$skip,$resolved,$abort" in
> +		,,t)
> +			rm -fr "$dotest"
> +			exit 0
> +			;;
> +		*)
> +			die "$(eval_gettext "Stray $dotest directory found.
> +Use \"git am --abort\" to remove it.")"
> +			;;
> +	esac

These two case arms are indented one level too deep (will locally
touch up).

> +	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.")"
> diff --git a/t/t4150-am.sh b/t/t4150-am.sh
> index 12f6b02..6c2cc3e 100755
> --- a/t/t4150-am.sh
> +++ b/t/t4150-am.sh
> @@ -363,6 +363,12 @@ test_expect_success 'am --skip works' '
>  	test_cmp expected another
>  '
>  
> +test_expect_success 'am --abort removes a stray directory' '
> +	mkdir .git/rebase-apply &&
> +	git am --abort &&
> +	test_path_is_missing .git/rebase-apply
> +'
> +
>  test_expect_success 'am --resolved works' '
>  	echo goodbye >expected &&
>  	rm -fr .git/rebase-apply &&

Re: [PATCH 1/2] am: handle stray $dotest directory

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> +			die "$(eval_gettext "Stray $dotest directory found.
> +Use \"git am --abort\" to remove it.")"

$dotest, or \$dotest?

Re: [PATCH 1/2] am: handle stray $dotest directory

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> These two case arms are indented one level too deep (will locally
> touch up).

Thanks.  Can you tell me how to get shell-script-mode to indent the
case statement properly? (I used the default indentation)

Re: [PATCH 1/2] am: handle stray $dotest directory

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> $dotest, or \$dotest?

Works fine for me like this.  Why do we escape the dollar in the other strings?

Re: [PATCH 1/2] am: handle stray $dotest directory

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> +	# Possible stray $dotest directory
>> +	if test -d "$dotest"; then
>> +	case "$skip,$resolved,$abort" in
>> +		,,t)
>> +			rm -fr "$dotest"
>> +			exit 0
>> +			;;
>> +		*)
>> +			die "$(eval_gettext "Stray $dotest directory found.
>> +Use \"git am --abort\" to remove it.")"
>> +			;;
>> +	esac
>
> These two case arms are indented one level too deep (will locally
> touch up).

And then the message triggers at the second test in t3420 when
applied on top of 587947750bd7 (rebase: implement --[no-]autostash
and rebase.autostash, 2013-05-12) or 45acb7592825 (Merge branch
'rr/rebase-autostash', 2013-06-11).

Re: [PATCH 1/2] am: handle stray $dotest directory

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> $dotest, or \$dotest?
>
> Works fine for me like this.  Why do we escape the dollar in the other strings?

The reason would become clear once you think what string you are
feeding eval_gettext with if you do not escape.  The translators
translate a fixed string (possibly with placeholders) to a fixed
translated string (possibly with placeholders).

	eval_gettext "Stray $dotest directory found. ..."

would allow the shell to expand $dotest before eval_gettext sees it,
which would mean the string is no longer a constant.

Re: [PATCH 1/2] am: handle stray $dotest directory

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> And then the message triggers at the second test in t3420 when
> applied on top of 587947750bd7 (rebase: implement --[no-]autostash
> and rebase.autostash, 2013-05-12) or 45acb7592825 (Merge branch
> 'rr/rebase-autostash', 2013-06-11).

What was triggered?  (I didn't understand what you said)

Re: [PATCH 1/2] am: handle stray $dotest directory

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> The reason would become clear once you think what string you are
> feeding eval_gettext with if you do not escape.  The translators
> translate a fixed string (possibly with placeholders) to a fixed
> translated string (possibly with placeholders).
>
>         eval_gettext "Stray $dotest directory found. ..."
>
> would allow the shell to expand $dotest before eval_gettext sees it,
> which would mean the string is no longer a constant.

Ah.  I was scratching my head wondering why $dotest needed to be
translated (it's just a path).

Re: [PATCH 1/2] am: handle stray $dotest directory

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> And then the message triggers at the second test in t3420 when
>> applied on top of 587947750bd7 (rebase: implement --[no-]autostash
>> and rebase.autostash, 2013-05-12) or 45acb7592825 (Merge branch
>> 'rr/rebase-autostash', 2013-06-11).
>
> What was triggered?  (I didn't understand what you said)

The patch applied on top of either of these commits will *BREAK* the
second test in t3420.

Re: [PATCH 1/2] am: handle stray $dotest directory

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> The reason would become clear once you think what string you are
>> feeding eval_gettext with if you do not escape.  The translators
>> translate a fixed string (possibly with placeholders) to a fixed
>> translated string (possibly with placeholders).
>>
>>         eval_gettext "Stray $dotest directory found. ..."
>>
>> would allow the shell to expand $dotest before eval_gettext sees it,
>> which would mean the string is no longer a constant.
>
> Ah.  I was scratching my head wondering why $dotest needed to be
> translated (it's just a path).

"Stray" and "directory found" are to be translated.  You can have
infinite possibilities to $dotest that is computed at runtime, so
that should be kept as a variable, untranslated in the message
template.

Re: [PATCH 1/2] am: handle stray $dotest directory

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> Junio C Hamano wrote:
>>> And then the message triggers at the second test in t3420 when
>>> applied on top of 587947750bd7 (rebase: implement --[no-]autostash
>>> and rebase.autostash, 2013-05-12) or 45acb7592825 (Merge branch
>>> 'rr/rebase-autostash', 2013-06-11).
>>
>> What was triggered?  (I didn't understand what you said)
>
> The patch applied on top of either of these commits will *BREAK* the
> second test in t3420.

Here is how "sh -x t3420-*.sh -i" ends:

    ...
    ++ git rebase unrelated-onto-branch
    Created autostash: cdca6ca
    HEAD is now at 0c4d2f1 third commit
    First, rewinding head to replay your work on top of it...
    Stray /srv/project/git/git.git/t/trash
    directory.t3420-rebase-autostash/.git/rebase-apply directory found.
    Use "git am --abort" to remove it.
    + eval_ret=1
    + test -z t
    + test 1 = 0
    + test -n ''
    + test t = t
    + test -n ''
    + return 1
    + test_failure_ 'rebase: dirty worktree, non-conflicting rebase' '
    ...

I _think_ the new check you added may be too loose.

A fix-up may look like this.


 git-am.sh | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index 37edfae..3f89cf6 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -505,9 +505,9 @@ then
 		exit ;;
 	esac
 	rm -f "$dotest/dirtyindex"
-else
-	# Possible stray $dotest directory
-	if test -d "$dotest"; then
+elif test -d "$dotest" && ! test -f "$dotest/autostash"
+then
+	# stray $dotest directory
 	case "$skip,$resolved,$abort" in
 	,,t)
 		rm -fr "$dotest"
@@ -518,8 +518,7 @@ else
 Use \"git am --abort\" to remove it.")"
 		;;
 	esac
-	fi
-
+else
 	# 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.")"

Re: [PATCH 1/2] am: handle stray $dotest directory

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> I _think_ the new check you added may be too loose.

Yep, I totally forgot about the case when git-am.sh is called from an
existing script.  In that case, it is upto the caller to handle
whatever stray directories; we have no business meddling with that.

> A fix-up may look like this.

No, don't leak autostash detail.

Just change that condition to

  if test -d "$dotest" && test -z "$rebasing"

and we're done.  I'll send in a re-roll.

Re: [PATCH 1/2] am: handle stray $dotest directory

[Ramkumar Ramachandra]

Ramkumar Ramachandra wrote:
> Can you tell me how to get shell-script-mode to indent the
> case statement properly? (I used the default indentation)

Never mind; I figured it out:

(setq sh-indent-for-case-label 0)
(setq sh-indent-for-case-alt '+)

Maybe we should dump the relevant parts of my .emacs somewhere in-tree?