{"thread":{"id":"56459","subject":"[PATCH 0/2] A pair of git am --abort issues","startedAt":"2021-09-08T02:17:17Z","lastAt":"2021-09-13T12:08:56Z","messageCount":15,"participants":["Elijah Newren via GitGitGadget","Bagas Sanjaya","Elijah Newren","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"435007","messageId":"pull.1087.git.git.1631067429.gitgitgadget@gmail.com","threadId":"56459","inReplyTo":null,"subject":"[PATCH 0/2] A pair of git am --abort issues","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-08T02:17:07Z","receivedAt":"2021-09-08T02:17:17Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This series documents a few issues with git am --abort in the form of new\ntestcases, and fixes one of them. However, while I was surprised the abort\nleft the working directory dirty, I couldn't find any documentation to\nconfirm it should or shouldn't be, and reading the code led me to question\nif perhaps it was intentional. Anyway, if it's intended, let me know and\nI'll drop that testcase.\n\nFor frame of reference, these were some issues I found while working on\nunintentional removal of untracked files/directories and the current working\ndirectory, and I'm just submitting them separately.\n\nElijah Newren (2):\n t4151: document a pair of am --abort bugs\n am: fix incorrect exit status on am fail to abort\n\n builtin/am.c | 3 ++-\n t/t4151-am-abort.sh | 31 +++++++++++++++++++++++++++++++\n 2 files changed, 33 insertions(+), 1 deletion(-)\n\n\nbase-commit: e0a2f5cbc585657e757385ad918f167f519cfb96\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1087%2Fnewren%2Fam-issues-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1087/newren/am-issues-v1\nPull-Request: https://github.com/git/git/pull/1087\n-- \ngitgitgadget\n"},{"id":"435008","messageId":"b8a418bc63ab0a4add25724a11eb5f992e3d4472.1631067429.git.gitgitgadget@gmail.com","threadId":"56459","inReplyTo":"pull.1087.git.git.1631067429.gitgitgadget@gmail.com","subject":"[PATCH 1/2] t4151: document a pair of am --abort bugs","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-08T02:17:08Z","receivedAt":"2021-09-08T02:17:17Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nSigned-off-by: Elijah Newren \n---\n t/t4151-am-abort.sh | 31 +++++++++++++++++++++++++++++++\n 1 file changed, 31 insertions(+)\n\ndiff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh\nindex 9d8d3c72e7e..501a7a9d211 100755\n--- a/t/t4151-am-abort.sh\n+++ b/t/t4151-am-abort.sh\n@@ -23,7 +23,11 @@ test_expect_success setup '\n \t\ttest_tick &&\n \t\tgit commit -a -m $i || return 1\n \tdone &&\n+\tgit branch changes &&\n \tgit format-patch --no-numbered initial &&\n+\tgit checkout -b conflicting initial &&\n+\techo different >>file-1 &&\n+\tgit commit -a -m different &&\n \tgit checkout -b side initial &&\n \techo local change >file-2-expect\n '\n@@ -191,4 +195,31 @@ test_expect_success 'am --abort leaves index stat info alone' '\n \tgit diff-files --exit-code --quiet\n '\n \n+test_expect_failure 'git am --abort return failed exit status when it fails' '\n+\ttest_when_finished \"rm -rf file-2/ && git reset --hard\" &&\n+\tgit checkout changes &&\n+\tgit format-patch -1 --stdout conflicting >changes.mbox &&\n+\ttest_must_fail git am --3way changes.mbox &&\n+\n+\tgit rm file-2 &&\n+\tmkdir file-2 &&\n+\techo precious >file-2/somefile &&\n+\ttest_must_fail git am --abort &&\n+\ttest_path_is_dir file-2/\n+'\n+\n+test_expect_failure 'git am --abort returns us to a clean state' '\n+\tgit checkout changes &&\n+\tgit format-patch -1 --stdout conflicting >changes.mbox &&\n+\ttest_must_fail git am --3way changes.mbox &&\n+\n+\t# Make a change related to the rest of the am work\n+\techo related change >>file-2 &&\n+\n+\t# Abort, and expect the related change to go away too\n+\tgit am --abort &&\n+\tgit status --porcelain -uno >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"435009","messageId":"5fa7daf264b75fc5419e829683be2e6d83191908.1631067429.git.gitgitgadget@gmail.com","threadId":"56459","inReplyTo":"pull.1087.git.git.1631067429.gitgitgadget@gmail.com","subject":"[PATCH 2/2] am: fix incorrect exit status on am fail to abort","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-08T02:17:09Z","receivedAt":"2021-09-08T02:17:20Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nSigned-off-by: Elijah Newren \n---\n builtin/am.c | 3 ++-\n t/t4151-am-abort.sh | 2 +-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 0c2ad96b70e..c79e0167e98 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -2106,7 +2106,8 @@ static void am_abort(struct am_state *state)\n \tif (!has_orig_head)\n \t\toidcpy(&orig_head, the_hash_algo->empty_tree);\n \n-\tclean_index(&curr_head, &orig_head);\n+\tif (clean_index(&curr_head, &orig_head))\n+\t\tdie(_(\"failed to clean index\"));\n \n \tif (has_orig_head)\n \t\tupdate_ref(\"am --abort\", \"HEAD\", &orig_head,\ndiff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh\nindex 501a7a9d211..f889f25a98f 100755\n--- a/t/t4151-am-abort.sh\n+++ b/t/t4151-am-abort.sh\n@@ -195,7 +195,7 @@ test_expect_success 'am --abort leaves index stat info alone' '\n \tgit diff-files --exit-code --quiet\n '\n \n-test_expect_failure 'git am --abort return failed exit status when it fails' '\n+test_expect_success 'git am --abort return failed exit status when it fails' '\n \ttest_when_finished \"rm -rf file-2/ && git reset --hard\" &&\n \tgit checkout changes &&\n \tgit format-patch -1 --stdout conflicting >changes.mbox &&\n-- \ngitgitgadget\n"},{"id":"435020","messageId":"96e001e5-56d7-7806-19ad-ac41f833b0a7@gmail.com","threadId":"56459","inReplyTo":"b8a418bc63ab0a4add25724a11eb5f992e3d4472.1631067429.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] t4151: document a pair of am --abort bugs","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-09-08T05:13:11Z","receivedAt":"2021-09-08T05:13:17Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On 08/09/21 09.17, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> Signed-off-by: Elijah Newren \n> ---\n> t/t4151-am-abort.sh | 31 +++++++++++++++++++++++++++++++\n> 1 file changed, 31 insertions(+)\n> \n> diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh\n> index 9d8d3c72e7e..501a7a9d211 100755\n> --- a/t/t4151-am-abort.sh\n> +++ b/t/t4151-am-abort.sh\n> @@ -23,7 +23,11 @@ test_expect_success setup '\n> \t\ttest_tick &&\n> \t\tgit commit -a -m $i || return 1\n> \tdone &&\n> +\tgit branch changes &&\n> \tgit format-patch --no-numbered initial &&\n> +\tgit checkout -b conflicting initial &&\n> +\techo different >>file-1 &&\n> +\tgit commit -a -m different &&\n> \tgit checkout -b side initial &&\n> \techo local change >file-2-expect\n> '\n> @@ -191,4 +195,31 @@ test_expect_success 'am --abort leaves index stat info alone' '\n> \tgit diff-files --exit-code --quiet\n> '\n> \n> +test_expect_failure 'git am --abort return failed exit status when it fails' '\n> +\ttest_when_finished \"rm -rf file-2/ && git reset --hard\" &&\n> +\tgit checkout changes &&\n> +\tgit format-patch -1 --stdout conflicting >changes.mbox &&\n> +\ttest_must_fail git am --3way changes.mbox &&\n> +\n> +\tgit rm file-2 &&\n> +\tmkdir file-2 &&\n> +\techo precious >file-2/somefile &&\n> +\ttest_must_fail git am --abort &&\n> +\ttest_path_is_dir file-2/\n> +'\n> +\n> +test_expect_failure 'git am --abort returns us to a clean state' '\n> +\tgit checkout changes &&\n> +\tgit format-patch -1 --stdout conflicting >changes.mbox &&\n> +\ttest_must_fail git am --3way changes.mbox &&\n> +\n> +\t# Make a change related to the rest of the am work\n> +\techo related change >>file-2 &&\n> +\n> +\t# Abort, and expect the related change to go away too\n> +\tgit am --abort &&\n> +\tgit status --porcelain -uno >actual &&\n> +\ttest_must_be_empty actual\n> +'\n> +\n> test_done\n> \n\nI expect BUGS section in git-am(1) to be added to describe known bugs \nyou demonstrated above, judging from the patch subject.\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"435025","messageId":"CABPp-BE812VNn9Qq8bWdCMH0bMcaN8JqKJYt=XKC430y1eE=xA@mail.gmail.com","threadId":"56459","inReplyTo":"96e001e5-56d7-7806-19ad-ac41f833b0a7@gmail.com","subject":"Re: [PATCH 1/2] t4151: document a pair of am --abort bugs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-09-08T05:24:20Z","receivedAt":"2021-09-08T05:24:37Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Sep 7, 2021 at 10:13 PM Bagas Sanjaya wrote:\n>\n> On 08/09/21 09.17, Elijah Newren via GitGitGadget wrote:\n> > From: Elijah Newren \n> >\n> > Signed-off-by: Elijah Newren \n> > ---\n> > t/t4151-am-abort.sh | 31 +++++++++++++++++++++++++++++++\n> > 1 file changed, 31 insertions(+)\n> >\n> > diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh\n> > index 9d8d3c72e7e..501a7a9d211 100755\n> > --- a/t/t4151-am-abort.sh\n> > +++ b/t/t4151-am-abort.sh\n> > @@ -23,7 +23,11 @@ test_expect_success setup '\n> > test_tick &&\n> > git commit -a -m $i || return 1\n> > done &&\n> > + git branch changes &&\n> > git format-patch --no-numbered initial &&\n> > + git checkout -b conflicting initial &&\n> > + echo different >>file-1 &&\n> > + git commit -a -m different &&\n> > git checkout -b side initial &&\n> > echo local change >file-2-expect\n> > '\n> > @@ -191,4 +195,31 @@ test_expect_success 'am --abort leaves index stat info alone' '\n> > git diff-files --exit-code --quiet\n> > '\n> >\n> > +test_expect_failure 'git am --abort return failed exit status when it fails' '\n> > + test_when_finished \"rm -rf file-2/ && git reset --hard\" &&\n> > + git checkout changes &&\n> > + git format-patch -1 --stdout conflicting >changes.mbox &&\n> > + test_must_fail git am --3way changes.mbox &&\n> > +\n> > + git rm file-2 &&\n> > + mkdir file-2 &&\n> > + echo precious >file-2/somefile &&\n> > + test_must_fail git am --abort &&\n> > + test_path_is_dir file-2/\n> > +'\n> > +\n> > +test_expect_failure 'git am --abort returns us to a clean state' '\n> > + git checkout changes &&\n> > + git format-patch -1 --stdout conflicting >changes.mbox &&\n> > + test_must_fail git am --3way changes.mbox &&\n> > +\n> > + # Make a change related to the rest of the am work\n> > + echo related change >>file-2 &&\n> > +\n> > + # Abort, and expect the related change to go away too\n> > + git am --abort &&\n> > + git status --porcelain -uno >actual &&\n> > + test_must_be_empty actual\n> > +'\n> > +\n> > test_done\n> >\n>\n> I expect BUGS section in git-am(1) to be added to describe known bugs\n> you demonstrated above, judging from the patch subject.\n\nThere's no point documenting the first one since it'll be fixed by the\nnext patch. As for the second, as I noted in my cover letter, I'm not\nquite sure that it really is a bug. If it isn't, the second testcase\nshould be dropped. However, if the second testcase represents an\nactual bug rather than me just misjudging the intent, then your\nsuggestion certainly makes sense.\n"},{"id":"435032","messageId":"xmqq8s077doe.fsf@gitster.g","threadId":"56459","inReplyTo":"b8a418bc63ab0a4add25724a11eb5f992e3d4472.1631067429.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] t4151: document a pair of am --abort bugs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-08T07:02:25Z","receivedAt":"2021-09-08T07:02:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> +test_expect_failure 'git am --abort returns us to a clean state' '\n> +\tgit checkout changes &&\n> +\tgit format-patch -1 --stdout conflicting >changes.mbox &&\n> +\ttest_must_fail git am --3way changes.mbox &&\n> +\n> +\t# Make a change related to the rest of the am work\n> +\techo related change >>file-2 &&\n> +\n> +\t# Abort, and expect the related change to go away too\n> +\tgit am --abort &&\n> +\tgit status --porcelain -uno >actual &&\n> +\ttest_must_be_empty actual\n\nThis test makes me worried. It is perfectly normal for \"am\" to be\nasked to work in a dirty working tree as long as the index is clean\nand the working tree files that are involved in the patch are\nunmodified. Even though you may want \"am --abort\" to restore the\npaths that the operation touched to their original state, I am not\nsure if that is always possible, given that there may have been\ndirty working tree files to begin with.\n\nAnd the above test would succeed if \"git am --abort\" internally\ncalled \"git reset --hard\", which definitely is not what we want to\nsee. We want the local changes in dirty working tree files that\nweren't involved in the patch application to stay, even after\nrunning \"am --abort\".\n"},{"id":"435039","messageId":"CABPp-BGD9WdeiquX0eiMz2458pDhJpjOXGyUOqoXu52ohZVCsQ@mail.gmail.com","threadId":"56459","inReplyTo":"xmqq8s077doe.fsf@gitster.g","subject":"Re: [PATCH 1/2] t4151: document a pair of am --abort bugs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-09-08T08:00:43Z","receivedAt":"2021-09-08T08:00:58Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Sep 8, 2021 at 12:02 AM Junio C Hamano wrote:\n>\n> \"Elijah Newren via GitGitGadget\" writes:\n>\n> > +test_expect_failure 'git am --abort returns us to a clean state' '\n> > + git checkout changes &&\n> > + git format-patch -1 --stdout conflicting >changes.mbox &&\n> > + test_must_fail git am --3way changes.mbox &&\n> > +\n> > + # Make a change related to the rest of the am work\n> > + echo related change >>file-2 &&\n> > +\n> > + # Abort, and expect the related change to go away too\n> > + git am --abort &&\n> > + git status --porcelain -uno >actual &&\n> > + test_must_be_empty actual\n>\n> This test makes me worried. It is perfectly normal for \"am\" to be\n> asked to work in a dirty working tree as long as the index is clean\n> and the working tree files that are involved in the patch are\n> unmodified.\n\nAh, I think I am just too used to rebase where it refuses to start if\nthe working tree isn't clean, assumed the same with am (which I don't\nuse that much), and then projected from there.\n\nI'll drop the second test; thanks for the explanation.\n"},{"id":"435042","messageId":"nycvar.QRO.7.76.6.2109081007230.55@tvgsbejvaqbjf.bet","threadId":"56459","inReplyTo":"pull.1087.git.git.1631067429.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] A pair of git am --abort issues","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-09-08T08:08:51Z","receivedAt":"2021-09-08T08:08:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nOn Wed, 8 Sep 2021, Elijah Newren via GitGitGadget wrote:\n\n> This series documents a few issues with git am --abort in the form of new\n> testcases, and fixes one of them. However, while I was surprised the abort\n> left the working directory dirty, I couldn't find any documentation to\n> confirm it should or shouldn't be, and reading the code led me to question\n> if perhaps it was intentional. Anyway, if it's intended, let me know and\n> I'll drop that testcase.\n\nAs far as I understand, `git am --abort` should indeed clean up. The\nbehavior you described is what I would expect more of `git am --quit`.\n\nThe patches look good to me.\n\nThanks!\nDscho\n\n>\n> For frame of reference, these were some issues I found while working on\n> unintentional removal of untracked files/directories and the current working\n> directory, and I'm just submitting them separately.\n>\n> Elijah Newren (2):\n> t4151: document a pair of am --abort bugs\n> am: fix incorrect exit status on am fail to abort\n>\n> builtin/am.c | 3 ++-\n> t/t4151-am-abort.sh | 31 +++++++++++++++++++++++++++++++\n> 2 files changed, 33 insertions(+), 1 deletion(-)\n>\n>\n> base-commit: e0a2f5cbc585657e757385ad918f167f519cfb96\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1087%2Fnewren%2Fam-issues-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1087/newren/am-issues-v1\n> Pull-Request: https://github.com/git/git/pull/1087\n> --\n> gitgitgadget\n>\n"},{"id":"435117","messageId":"xmqq1r5z58oq.fsf@gitster.g","threadId":"56459","inReplyTo":"CABPp-BGD9WdeiquX0eiMz2458pDhJpjOXGyUOqoXu52ohZVCsQ@mail.gmail.com","subject":"Re: [PATCH 1/2] t4151: document a pair of am --abort bugs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-08T16:33:09Z","receivedAt":"2021-09-08T16:33:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> On Wed, Sep 8, 2021 at 12:02 AM Junio C Hamano wrote:\n>>\n>> \"Elijah Newren via GitGitGadget\" writes:\n>>\n>> > +test_expect_failure 'git am --abort returns us to a clean state' '\n>> > + git checkout changes &&\n>> > + git format-patch -1 --stdout conflicting >changes.mbox &&\n>> > + test_must_fail git am --3way changes.mbox &&\n>> > +\n>> > + # Make a change related to the rest of the am work\n>> > + echo related change >>file-2 &&\n>> > +\n>> > + # Abort, and expect the related change to go away too\n>> > + git am --abort &&\n>> > + git status --porcelain -uno >actual &&\n>> > + test_must_be_empty actual\n>>\n>> This test makes me worried. It is perfectly normal for \"am\" to be\n>> asked to work in a dirty working tree as long as the index is clean\n>> and the working tree files that are involved in the patch are\n>> unmodified.\n>\n> Ah, I think I am just too used to rebase where it refuses to start if\n> the working tree isn't clean, assumed the same with am (which I don't\n> use that much), and then projected from there.\n>\n> I'll drop the second test; thanks for the explanation.\n\nActually, if you test that unrelated dirty files are kept, then the\ntest is a welcome addition. \"returns us to a 'clean' state\" needs a\nbit different title, though.\n\nThanks.\n"},{"id":"435118","messageId":"xmqqwnnr3u3l.fsf@gitster.g","threadId":"56459","inReplyTo":"5fa7daf264b75fc5419e829683be2e6d83191908.1631067429.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] am: fix incorrect exit status on am fail to abort","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-08T16:33:34Z","receivedAt":"2021-09-08T16:33:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> From: Elijah Newren \n>\n> Signed-off-by: Elijah Newren \n> ---\n\nObviously correct.\n\n> builtin/am.c | 3 ++-\n> t/t4151-am-abort.sh | 2 +-\n> 2 files changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/am.c b/builtin/am.c\n> index 0c2ad96b70e..c79e0167e98 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -2106,7 +2106,8 @@ static void am_abort(struct am_state *state)\n> \tif (!has_orig_head)\n> \t\toidcpy(&orig_head, the_hash_algo->empty_tree);\n> \n> -\tclean_index(&curr_head, &orig_head);\n> +\tif (clean_index(&curr_head, &orig_head))\n> +\t\tdie(_(\"failed to clean index\"));\n> \n> \tif (has_orig_head)\n> \t\tupdate_ref(\"am --abort\", \"HEAD\", &orig_head,\n> diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh\n> index 501a7a9d211..f889f25a98f 100755\n> --- a/t/t4151-am-abort.sh\n> +++ b/t/t4151-am-abort.sh\n> @@ -195,7 +195,7 @@ test_expect_success 'am --abort leaves index stat info alone' '\n> \tgit diff-files --exit-code --quiet\n> '\n> \n> -test_expect_failure 'git am --abort return failed exit status when it fails' '\n> +test_expect_success 'git am --abort return failed exit status when it fails' '\n> \ttest_when_finished \"rm -rf file-2/ && git reset --hard\" &&\n> \tgit checkout changes &&\n> \tgit format-patch -1 --stdout conflicting >changes.mbox &&\n"},{"id":"435379","messageId":"pull.1087.v2.git.git.1631269876.gitgitgadget@gmail.com","threadId":"56459","inReplyTo":"pull.1087.git.git.1631067429.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] A pair of git am --abort issues","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-10T10:31:13Z","receivedAt":"2021-09-10T10:31:20Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This series documents a few issues with git am --abort in the form of new\ntestcases, and fixes one of them. However, while I was surprised the abort\nleft the working directory dirty, I couldn't find any documentation to\nconfirm it should or shouldn't be, and reading the code led me to question\nif perhaps it was intentional. Anyway, if it's intended, let me know and\nI'll drop that testcase.\n\nFor frame of reference, these were some issues I found while working on\nunintentional removal of untracked files/directories and the current working\ndirectory, and I'm just submitting them separately.\n\nChanges since v1:\n\n * Added a patch to tweak the documentation to clarify that partial cleaning\n of worktree is expected with --abort\n * Tweaked the second test to be a test that unrelated dirty files are kept,\n as suggested by Junio\n\nElijah Newren (3):\n git-am.txt: clarify --abort behavior\n t4151: add a few am --abort tests\n am: fix incorrect exit status on am fail to abort\n\n Documentation/git-am.txt | 2 ++\n builtin/am.c | 3 ++-\n t/t4151-am-abort.sh | 39 +++++++++++++++++++++++++++++++++++++++\n 3 files changed, 43 insertions(+), 1 deletion(-)\n\n\nbase-commit: e0a2f5cbc585657e757385ad918f167f519cfb96\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1087%2Fnewren%2Fam-issues-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1087/newren/am-issues-v2\nPull-Request: https://github.com/git/git/pull/1087\n\nRange-diff vs v1:\n\n -: ----------- > 1: e199df0f3bc git-am.txt: clarify --abort behavior\n 1: b8a418bc63a ! 2: 38a7063b959 t4151: document a pair of am --abort bugs\n @@ Metadata\n Author: Elijah Newren \n \n ## Commit message ##\n - t4151: document a pair of am --abort bugs\n + t4151: add a few am --abort tests\n \n Signed-off-by: Elijah Newren \n \n @@ t/t4151-am-abort.sh: test_expect_success setup '\n \tgit format-patch --no-numbered initial &&\n +\tgit checkout -b conflicting initial &&\n +\techo different >>file-1 &&\n -+\tgit commit -a -m different &&\n ++\techo whatever >new-file &&\n ++\tgit add file-1 new-file &&\n ++\tgit commit -m different &&\n \tgit checkout -b side initial &&\n \techo local change >file-2-expect\n '\n @@ t/t4151-am-abort.sh: test_expect_success 'am --abort leaves index stat info alon\n '\n \n +test_expect_failure 'git am --abort return failed exit status when it fails' '\n -+\ttest_when_finished \"rm -rf file-2/ && git reset --hard\" &&\n ++\ttest_when_finished \"rm -rf file-2/ && git reset --hard && git am --abort\" &&\n +\tgit checkout changes &&\n +\tgit format-patch -1 --stdout conflicting >changes.mbox &&\n +\ttest_must_fail git am --3way changes.mbox &&\n @@ t/t4151-am-abort.sh: test_expect_success 'am --abort leaves index stat info alon\n +\ttest_path_is_dir file-2/\n +'\n +\n -+test_expect_failure 'git am --abort returns us to a clean state' '\n ++test_expect_success 'git am --abort cleans relevant files' '\n +\tgit checkout changes &&\n +\tgit format-patch -1 --stdout conflicting >changes.mbox &&\n +\ttest_must_fail git am --3way changes.mbox &&\n +\n -+\t# Make a change related to the rest of the am work\n -+\techo related change >>file-2 &&\n ++\ttest_path_is_file new-file &&\n ++\techo further changes >>file-1 &&\n ++\techo change other file >>file-2 &&\n +\n -+\t# Abort, and expect the related change to go away too\n ++\t# Abort, and expect the files touched by am to be reverted\n +\tgit am --abort &&\n -+\tgit status --porcelain -uno >actual &&\n -+\ttest_must_be_empty actual\n ++\n ++\ttest_path_is_missing new-file &&\n ++\n ++\t# Files not involved in am operation are left modified\n ++\tgit diff --name-only changes >actual &&\n ++\ttest_write_lines file-2 >expect &&\n ++\ttest_cmp expect actual\n +'\n +\n test_done\n 2: 5fa7daf264b ! 3: a46f5c79fbf am: fix incorrect exit status on am fail to abort\n @@ t/t4151-am-abort.sh: test_expect_success 'am --abort leaves index stat info alon\n \n -test_expect_failure 'git am --abort return failed exit status when it fails' '\n +test_expect_success 'git am --abort return failed exit status when it fails' '\n - \ttest_when_finished \"rm -rf file-2/ && git reset --hard\" &&\n + \ttest_when_finished \"rm -rf file-2/ && git reset --hard && git am --abort\" &&\n \tgit checkout changes &&\n \tgit format-patch -1 --stdout conflicting >changes.mbox &&\n\n-- \ngitgitgadget\n"},{"id":"435380","messageId":"e199df0f3bc5a99e0d77c172316ad119c533f790.1631269876.git.gitgitgadget@gmail.com","threadId":"56459","inReplyTo":"pull.1087.v2.git.git.1631269876.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] git-am.txt: clarify --abort behavior","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-10T10:31:14Z","receivedAt":"2021-09-10T10:31:21Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nBoth Johannes and I assumed (perhaps due to familiarity with rebase)\nthat am --abort would return the user to a clean state. However, since\nam, unlike rebase, is intended to be used within a dirty working tree,\n--abort will only clean the files involved in the am operation.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-am.txt | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/Documentation/git-am.txt b/Documentation/git-am.txt\nindex 8714dfcb762..0a4a984dfde 100644\n--- a/Documentation/git-am.txt\n+++ b/Documentation/git-am.txt\n@@ -178,6 +178,8 @@ default. You can use `--no-utf8` to override this.\n \n --abort::\n \tRestore the original branch and abort the patching operation.\n+\tRevert contents of files involved in the am operation to their\n+\tpre-am state.\n \n --quit::\n \tAbort the patching operation but keep HEAD and the index\n-- \ngitgitgadget\n\n"},{"id":"435381","messageId":"38a7063b959f6a404b292586b52b9c3b4bbec308.1631269876.git.gitgitgadget@gmail.com","threadId":"56459","inReplyTo":"pull.1087.v2.git.git.1631269876.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] t4151: add a few am --abort tests","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-10T10:31:15Z","receivedAt":"2021-09-10T10:31:24Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nSigned-off-by: Elijah Newren \n---\n t/t4151-am-abort.sh | 39 +++++++++++++++++++++++++++++++++++++++\n 1 file changed, 39 insertions(+)\n\ndiff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh\nindex 9d8d3c72e7e..15f2f92cd76 100755\n--- a/t/t4151-am-abort.sh\n+++ b/t/t4151-am-abort.sh\n@@ -23,7 +23,13 @@ test_expect_success setup '\n \t\ttest_tick &&\n \t\tgit commit -a -m $i || return 1\n \tdone &&\n+\tgit branch changes &&\n \tgit format-patch --no-numbered initial &&\n+\tgit checkout -b conflicting initial &&\n+\techo different >>file-1 &&\n+\techo whatever >new-file &&\n+\tgit add file-1 new-file &&\n+\tgit commit -m different &&\n \tgit checkout -b side initial &&\n \techo local change >file-2-expect\n '\n@@ -191,4 +197,37 @@ test_expect_success 'am --abort leaves index stat info alone' '\n \tgit diff-files --exit-code --quiet\n '\n \n+test_expect_failure 'git am --abort return failed exit status when it fails' '\n+\ttest_when_finished \"rm -rf file-2/ && git reset --hard && git am --abort\" &&\n+\tgit checkout changes &&\n+\tgit format-patch -1 --stdout conflicting >changes.mbox &&\n+\ttest_must_fail git am --3way changes.mbox &&\n+\n+\tgit rm file-2 &&\n+\tmkdir file-2 &&\n+\techo precious >file-2/somefile &&\n+\ttest_must_fail git am --abort &&\n+\ttest_path_is_dir file-2/\n+'\n+\n+test_expect_success 'git am --abort cleans relevant files' '\n+\tgit checkout changes &&\n+\tgit format-patch -1 --stdout conflicting >changes.mbox &&\n+\ttest_must_fail git am --3way changes.mbox &&\n+\n+\ttest_path_is_file new-file &&\n+\techo further changes >>file-1 &&\n+\techo change other file >>file-2 &&\n+\n+\t# Abort, and expect the files touched by am to be reverted\n+\tgit am --abort &&\n+\n+\ttest_path_is_missing new-file &&\n+\n+\t# Files not involved in am operation are left modified\n+\tgit diff --name-only changes >actual &&\n+\ttest_write_lines file-2 >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"435382","messageId":"a46f5c79fbf40be98d4e679ff14197f995bd804a.1631269876.git.gitgitgadget@gmail.com","threadId":"56459","inReplyTo":"pull.1087.v2.git.git.1631269876.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] am: fix incorrect exit status on am fail to abort","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-10T10:31:16Z","receivedAt":"2021-09-10T10:31:24Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nSigned-off-by: Elijah Newren \n---\n builtin/am.c | 3 ++-\n t/t4151-am-abort.sh | 2 +-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 0c2ad96b70e..c79e0167e98 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -2106,7 +2106,8 @@ static void am_abort(struct am_state *state)\n \tif (!has_orig_head)\n \t\toidcpy(&orig_head, the_hash_algo->empty_tree);\n \n-\tclean_index(&curr_head, &orig_head);\n+\tif (clean_index(&curr_head, &orig_head))\n+\t\tdie(_(\"failed to clean index\"));\n \n \tif (has_orig_head)\n \t\tupdate_ref(\"am --abort\", \"HEAD\", &orig_head,\ndiff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh\nindex 15f2f92cd76..2374151662b 100755\n--- a/t/t4151-am-abort.sh\n+++ b/t/t4151-am-abort.sh\n@@ -197,7 +197,7 @@ test_expect_success 'am --abort leaves index stat info alone' '\n \tgit diff-files --exit-code --quiet\n '\n \n-test_expect_failure 'git am --abort return failed exit status when it fails' '\n+test_expect_success 'git am --abort return failed exit status when it fails' '\n \ttest_when_finished \"rm -rf file-2/ && git reset --hard && git am --abort\" &&\n \tgit checkout changes &&\n \tgit format-patch -1 --stdout conflicting >changes.mbox &&\n-- \ngitgitgadget\n"},{"id":"435715","messageId":"nycvar.QRO.7.76.6.2109131408030.55@tvgsbejvaqbjf.bet","threadId":"56459","inReplyTo":"pull.1087.v2.git.git.1631269876.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/3] A pair of git am --abort issues","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-09-13T12:08:49Z","receivedAt":"2021-09-13T12:08:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nOn Fri, 10 Sep 2021, Elijah Newren via GitGitGadget wrote:\n\n> This series documents a few issues with git am --abort in the form of new\n> testcases, and fixes one of them. However, while I was surprised the abort\n> left the working directory dirty, I couldn't find any documentation to\n> confirm it should or shouldn't be, and reading the code led me to question\n> if perhaps it was intentional. Anyway, if it's intended, let me know and\n> I'll drop that testcase.\n>\n> For frame of reference, these were some issues I found while working on\n> unintentional removal of untracked files/directories and the current working\n> directory, and I'm just submitting them separately.\n>\n> Changes since v1:\n>\n> * Added a patch to tweak the documentation to clarify that partial cleaning\n> of worktree is expected with --abort\n> * Tweaked the second test to be a test that unrelated dirty files are kept,\n> as suggested by Junio\n\nFWIW I gave this a light read-over and am happy with it!\n\nCiao,\nDscho\n"}]}