# [PATCH v2 0/2] Fix am with stray $dotest directory 16 messages from 2013-06-14 to 2013-06-16. Participants: Ramkumar Ramachandra, Junio C Hamano. Thread: https://gitlist.dev/t/34134 ## Ramkumar Ramachandra, 2013-06-14 07:47 Subject: [PATCH v2 0/2] Fix am with stray $dotest directory Message-ID: <1371196058-23948-1-git-send-email-artagnon@gmail.com> URL: https://gitlist.dev/e/1371196058-23948-1-git-send-email-artagnon%40gmail.com ``` [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 ``` ## Ramkumar Ramachandra, 2013-06-14 07:47 Subject: [PATCH 1/2] am: handle stray $dotest directory Message-ID: <1371196058-23948-2-git-send-email-artagnon@gmail.com> URL: https://gitlist.dev/e/1371196058-23948-2-git-send-email-artagnon%40gmail.com In-Reply-To: <1371196058-23948-1-git-send-email-artagnon@gmail.com> ``` 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 Signed-off-by: Ramkumar Ramachandra --- 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 ``` ## Ramkumar Ramachandra, 2013-06-14 07:47 Subject: [PATCH 2/2] t/am: use test_path_is_missing() where appropriate Message-ID: <1371196058-23948-3-git-send-email-artagnon@gmail.com> URL: https://gitlist.dev/e/1371196058-23948-3-git-send-email-artagnon%40gmail.com In-Reply-To: <1371196058-23948-1-git-send-email-artagnon@gmail.com> ``` Replace instances of ! test -d with test_path_is_missing. Signed-off-by: Ramkumar Ramachandra --- 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 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 && test_must_fail git am URL: https://gitlist.dev/e/7vli6crau5.fsf%40alter.siamese.dyndns.org In-Reply-To: <1371196058-23948-2-git-send-email-artagnon@gmail.com> ``` Ramkumar Ramachandra 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 > Signed-off-by: Ramkumar Ramachandra > --- > 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 && ``` ## Junio C Hamano, 2013-06-14 15:08 Subject: Re: [PATCH 1/2] am: handle stray $dotest directory Message-ID: <7vhah0raps.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vhah0raps.fsf%40alter.siamese.dyndns.org In-Reply-To: <1371196058-23948-2-git-send-email-artagnon@gmail.com> ``` Ramkumar Ramachandra writes: > + die "$(eval_gettext "Stray $dotest directory found. > +Use \"git am --abort\" to remove it.")" $dotest, or \$dotest? ``` ## Ramkumar Ramachandra, 2013-06-14 15:11 Subject: Re: [PATCH 1/2] am: handle stray $dotest directory Message-ID: URL: https://gitlist.dev/e/CALkWK0mF7s7Zfp4VTekT9AhHNhd80Jj3GT8SaKVs4gAATACsBA%40mail.gmail.com In-Reply-To: <7vli6crau5.fsf@alter.siamese.dyndns.org> ``` 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) ``` ## Ramkumar Ramachandra, 2013-06-14 15:11 Subject: Re: [PATCH 1/2] am: handle stray $dotest directory Message-ID: URL: https://gitlist.dev/e/CALkWK0m6r4yRfCLb3dejdY%2B1KJgdOHPfrvPXbftNucApXFx%2B0A%40mail.gmail.com In-Reply-To: <7vhah0raps.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano wrote: > $dotest, or \$dotest? Works fine for me like this. Why do we escape the dollar in the other strings? ``` ## Junio C Hamano, 2013-06-14 15:17 Subject: Re: [PATCH 1/2] am: handle stray $dotest directory Message-ID: <7v61xgraba.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v61xgraba.fsf%40alter.siamese.dyndns.org In-Reply-To: <7vli6crau5.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano 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). ``` ## Junio C Hamano, 2013-06-14 15:19 Subject: Re: [PATCH 1/2] am: handle stray $dotest directory Message-ID: <7v1u84ra77.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v1u84ra77.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Ramkumar Ramachandra 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. ``` ## Ramkumar Ramachandra, 2013-06-14 16:13 Subject: Re: [PATCH 1/2] am: handle stray $dotest directory Message-ID: URL: https://gitlist.dev/e/CALkWK0m6iiDAmbxG9wrpgxJ3%3DnM816U7RM519Mu62qSgDwJbaQ%40mail.gmail.com In-Reply-To: <7v61xgraba.fsf@alter.siamese.dyndns.org> ``` 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) ``` ## Ramkumar Ramachandra, 2013-06-14 16:15 Subject: Re: [PATCH 1/2] am: handle stray $dotest directory Message-ID: URL: https://gitlist.dev/e/CALkWK0n%2BKSrehL3tZ557c2hfKna6p_jVGL%2Ba2az5V%2Bh4%3DDstJQ%40mail.gmail.com In-Reply-To: <7v1u84ra77.fsf@alter.siamese.dyndns.org> ``` 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). ``` ## Junio C Hamano, 2013-06-14 17:09 Subject: Re: [PATCH 1/2] am: handle stray $dotest directory Message-ID: <7vip1gpqjp.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vip1gpqjp.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Ramkumar Ramachandra 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. ``` ## Junio C Hamano, 2013-06-14 17:20 Subject: Re: [PATCH 1/2] am: handle stray $dotest directory Message-ID: <7vehc4pq1c.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vehc4pq1c.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Ramkumar Ramachandra 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. ``` ## Junio C Hamano, 2013-06-14 19:00 Subject: Re: [PATCH 1/2] am: handle stray $dotest directory Message-ID: <7vbo78o6ut.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vbo78o6ut.fsf%40alter.siamese.dyndns.org In-Reply-To: <7vip1gpqjp.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano writes: > Ramkumar Ramachandra 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.")" ``` ## Ramkumar Ramachandra, 2013-06-15 12:35 Subject: Re: [PATCH 1/2] am: handle stray $dotest directory Message-ID: URL: https://gitlist.dev/e/CALkWK0nRtrvd%3DoX04XfqovYg1dnx3h%2BaNSNWH6MXnssOBqz%3Dxw%40mail.gmail.com In-Reply-To: <7vbo78o6ut.fsf@alter.siamese.dyndns.org> ``` 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. ``` ## Ramkumar Ramachandra, 2013-06-16 06:36 Subject: Re: [PATCH 1/2] am: handle stray $dotest directory Message-ID: URL: https://gitlist.dev/e/CALkWK0k9hhCvYs%3DSrCi_s%3Dq-MfhFWtkw8q2PYG%2BVy-tRq65nsA%40mail.gmail.com In-Reply-To: ``` 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? ```