# [PATCH v3 0/2] Fix am with stray $dotest directory 8 messages from 2013-06-15 to 2013-06-18. Participants: Ramkumar Ramachandra, Junio C Hamano. Thread: https://gitlist.dev/t/34147 ## Ramkumar Ramachandra, 2013-06-15 12:43 Subject: [PATCH v3 0/2] Fix am with stray $dotest directory Message-ID: <1371300192-6222-1-git-send-email-artagnon@gmail.com> URL: https://gitlist.dev/e/1371300192-6222-1-git-send-email-artagnon%40gmail.com ``` The test in [1/2] was too loose in the previous iteration: guard it with "test -z $rebasing". Also fix a couple of minor problems pointed out by Junio (extra indentation, $-unescaped). Thanks. Ramkumar Ramachandra (2): am: handle stray $dotest directory t/am: use test_path_is_missing() where appropriate git-am.sh | 17 +++++++++++++++++ t/t4150-am.sh | 40 +++++++++++++++++++++++----------------- 2 files changed, 40 insertions(+), 17 deletions(-) -- 1.8.3.1.383.g8881048.dirty ``` ## Ramkumar Ramachandra, 2013-06-15 12:43 Subject: [PATCH v3 1/2] am: handle stray $dotest directory Message-ID: <1371300192-6222-2-git-send-email-artagnon@gmail.com> URL: https://gitlist.dev/e/1371300192-6222-2-git-send-email-artagnon%40gmail.com In-Reply-To: <1371300192-6222-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. Also ensure that we're not called with --rebasing from git-rebase--am.sh; in that case, it is the responsibility of the caller to handle and stray directories. 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 | 17 +++++++++++++++++ t/t4150-am.sh | 6 ++++++ 2 files changed, 23 insertions(+) diff --git a/git-am.sh b/git-am.sh index 1cf3d1d..91a2bcc 100755 --- a/git-am.sh +++ b/git-am.sh @@ -506,6 +506,23 @@ then esac rm -f "$dotest/dirtyindex" else + # Possible stray $dotest directory in the independent-run + # case; in the --rebasing case, it is upto the caller + # (git-rebase--am) to take care of stray directories. + if test -d "$dotest" && test -z "$rebasing" + 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.383.g8881048.dirty ``` ## Ramkumar Ramachandra, 2013-06-15 12:43 Subject: [PATCH v3 2/2] t/am: use test_path_is_missing() where appropriate Message-ID: <1371300192-6222-3-git-send-email-artagnon@gmail.com> URL: https://gitlist.dev/e/1371300192-6222-3-git-send-email-artagnon%40gmail.com In-Reply-To: <1371300192-6222-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/7v38shjw01.fsf%40alter.siamese.dyndns.org In-Reply-To: <1371300192-6222-2-git-send-email-artagnon@gmail.com> ``` Ramkumar Ramachandra writes: > else > + # Possible stray $dotest directory in the independent-run > + # case; in the --rebasing case, it is upto the caller > + # (git-rebase--am) to take care of stray directories. > + if test -d "$dotest" && test -z "$rebasing" The $rebasing variable is set only when the command line of "git am" has --rebasing, i.e. only when it is driven by "git rebase". So this will not affect what happens to "git am --abort" that is run by the end user. That sounds like a safe thing to do as far as "am" is concerned. Will replace what has been queued on 'pu'. Thanks. ``` ## Junio C Hamano, 2013-06-17 17:54 Subject: Re: [PATCH v3 1/2] am: handle stray $dotest directory Message-ID: <7vy5a8ei6u.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vy5a8ei6u.fsf%40alter.siamese.dyndns.org In-Reply-To: <7v38shjw01.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano writes: > Will replace what has been queued on 'pu'. ... after fixing an indentation error, that is. ``` ## Ramkumar Ramachandra, 2013-06-18 07:05 Subject: Re: [PATCH v3 1/2] am: handle stray $dotest directory Message-ID: URL: https://gitlist.dev/e/CALkWK0kq6%2BK4fotWq1Qz9BrqOG56NezJqnfH9Xrg1vh6AS9bnw%40mail.gmail.com In-Reply-To: <7vy5a8ei6u.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano wrote: >> Will replace what has been queued on 'pu'. > > ... after fixing an indentation error, that is. Where did the error occur? I thought I taught my Emacs to always indent properly. ``` ## Junio C Hamano, 2013-06-18 14:31 Subject: Re: [PATCH v3 1/2] am: handle stray $dotest directory Message-ID: <7va9mna3tm.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7va9mna3tm.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Ramkumar Ramachandra writes: > Junio C Hamano wrote: >>> Will replace what has been queued on 'pu'. >> >> ... after fixing an indentation error, that is. > > Where did the error occur? I think you can compare what is in 'pu' and what you sent out without asking. ``` ## Ramkumar Ramachandra, 2013-06-18 14:44 Subject: Re: [PATCH v3 1/2] am: handle stray $dotest directory Message-ID: URL: https://gitlist.dev/e/CALkWK0kQwyM4UD_mVkHKdVT-j%2Bgxgg3quZN70jnE3tUO7m6bzA%40mail.gmail.com In-Reply-To: <7va9mna3tm.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano wrote: >> Where did the error occur? > > I think you can compare what is in 'pu' and what you sent out > without asking. Ah, I sent out the series before I taught Emacs to indent properly. Thanks. ```