patch, 2 partsA pair of git am --abort issues

> From: Elijah Newren <newren@gmail.com>
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>   t/t4151-am-abort.sh | 31 +++++++++++++++++++++++++++++++
>   1 file changed, 31 insertions(+)
> 
> diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh
> index 9d8d3c72e7e..501a7a9d211 100755
> --- a/t/t4151-am-abort.sh
> +++ b/t/t4151-am-abort.sh
> @@ -23,7 +23,11 @@ test_expect_success setup '
>   		test_tick &&
>   		git commit -a -m $i || return 1
>   	done &&
> +	git branch changes &&
>   	git format-patch --no-numbered initial &&
> +	git checkout -b conflicting initial &&
> +	echo different >>file-1 &&
> +	git commit -a -m different &&
>   	git checkout -b side initial &&
>   	echo local change >file-2-expect
>   '
> @@ -191,4 +195,31 @@ test_expect_success 'am --abort leaves index stat info alone' '
>   	git diff-files --exit-code --quiet
>   '
>   
> +test_expect_failure 'git am --abort return failed exit status when it fails' '
> +	test_when_finished "rm -rf file-2/ && git reset --hard" &&
> +	git checkout changes &&
> +	git format-patch -1 --stdout conflicting >changes.mbox &&
> +	test_must_fail git am --3way changes.mbox &&
> +
> +	git rm file-2 &&
> +	mkdir file-2 &&
> +	echo precious >file-2/somefile &&
> +	test_must_fail git am --abort &&
> +	test_path_is_dir file-2/
> +'
> +
> +test_expect_failure 'git am --abort returns us to a clean state' '
> +	git checkout changes &&
> +	git format-patch -1 --stdout conflicting >changes.mbox &&
> +	test_must_fail git am --3way changes.mbox &&
> +
> +	# Make a change related to the rest of the am work
> +	echo related change >>file-2 &&
> +
> +	# Abort, and expect the related change to go away too
> +	git am --abort &&
> +	git status --porcelain -uno >actual &&
> +	test_must_be_empty actual
> +'
> +
>   test_done
> 

>
> On 08/09/21 09.17, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> >   t/t4151-am-abort.sh | 31 +++++++++++++++++++++++++++++++
> >   1 file changed, 31 insertions(+)
> >
> > diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh
> > index 9d8d3c72e7e..501a7a9d211 100755
> > --- a/t/t4151-am-abort.sh
> > +++ b/t/t4151-am-abort.sh
> > @@ -23,7 +23,11 @@ test_expect_success setup '
> >               test_tick &&
> >               git commit -a -m $i || return 1
> >       done &&
> > +     git branch changes &&
> >       git format-patch --no-numbered initial &&
> > +     git checkout -b conflicting initial &&
> > +     echo different >>file-1 &&
> > +     git commit -a -m different &&
> >       git checkout -b side initial &&
> >       echo local change >file-2-expect
> >   '
> > @@ -191,4 +195,31 @@ test_expect_success 'am --abort leaves index stat info alone' '
> >       git diff-files --exit-code --quiet
> >   '
> >
> > +test_expect_failure 'git am --abort return failed exit status when it fails' '
> > +     test_when_finished "rm -rf file-2/ && git reset --hard" &&
> > +     git checkout changes &&
> > +     git format-patch -1 --stdout conflicting >changes.mbox &&
> > +     test_must_fail git am --3way changes.mbox &&
> > +
> > +     git rm file-2 &&
> > +     mkdir file-2 &&
> > +     echo precious >file-2/somefile &&
> > +     test_must_fail git am --abort &&
> > +     test_path_is_dir file-2/
> > +'
> > +
> > +test_expect_failure 'git am --abort returns us to a clean state' '
> > +     git checkout changes &&
> > +     git format-patch -1 --stdout conflicting >changes.mbox &&
> > +     test_must_fail git am --3way changes.mbox &&
> > +
> > +     # Make a change related to the rest of the am work
> > +     echo related change >>file-2 &&
> > +
> > +     # Abort, and expect the related change to go away too
> > +     git am --abort &&
> > +     git status --porcelain -uno >actual &&
> > +     test_must_be_empty actual
> > +'
> > +
> >   test_done
> >
>
> I expect BUGS section in git-am(1) to be added to describe known bugs
> you demonstrated above, judging from the patch subject.

> +test_expect_failure 'git am --abort returns us to a clean state' '
> +	git checkout changes &&
> +	git format-patch -1 --stdout conflicting >changes.mbox &&
> +	test_must_fail git am --3way changes.mbox &&
> +
> +	# Make a change related to the rest of the am work
> +	echo related change >>file-2 &&
> +
> +	# Abort, and expect the related change to go away too
> +	git am --abort &&
> +	git status --porcelain -uno >actual &&
> +	test_must_be_empty actual

>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > +test_expect_failure 'git am --abort returns us to a clean state' '
> > +     git checkout changes &&
> > +     git format-patch -1 --stdout conflicting >changes.mbox &&
> > +     test_must_fail git am --3way changes.mbox &&
> > +
> > +     # Make a change related to the rest of the am work
> > +     echo related change >>file-2 &&
> > +
> > +     # Abort, and expect the related change to go away too
> > +     git am --abort &&
> > +     git status --porcelain -uno >actual &&
> > +     test_must_be_empty actual
>
> This test makes me worried.  It is perfectly normal for "am" to be
> asked to work in a dirty working tree as long as the index is clean
> and the working tree files that are involved in the patch are
> unmodified.

> On Wed, Sep 8, 2021 at 12:02 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>> > +test_expect_failure 'git am --abort returns us to a clean state' '
>> > +     git checkout changes &&
>> > +     git format-patch -1 --stdout conflicting >changes.mbox &&
>> > +     test_must_fail git am --3way changes.mbox &&
>> > +
>> > +     # Make a change related to the rest of the am work
>> > +     echo related change >>file-2 &&
>> > +
>> > +     # Abort, and expect the related change to go away too
>> > +     git am --abort &&
>> > +     git status --porcelain -uno >actual &&
>> > +     test_must_be_empty actual
>>
>> This test makes me worried.  It is perfectly normal for "am" to be
>> asked to work in a dirty working tree as long as the index is clean
>> and the working tree files that are involved in the patch are
>> unmodified.
>
> Ah, I think I am just too used to rebase where it refuses to start if
> the working tree isn't clean, assumed the same with am (which I don't
> use that much), and then projected from there.
>
> I'll drop the second test; thanks for the explanation.

>  builtin/am.c        | 3 ++-
>  t/t4151-am-abort.sh | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/am.c b/builtin/am.c
> index 0c2ad96b70e..c79e0167e98 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -2106,7 +2106,8 @@ static void am_abort(struct am_state *state)
>  	if (!has_orig_head)
>  		oidcpy(&orig_head, the_hash_algo->empty_tree);
>  
> -	clean_index(&curr_head, &orig_head);
> +	if (clean_index(&curr_head, &orig_head))
> +		die(_("failed to clean index"));
>  
>  	if (has_orig_head)
>  		update_ref("am --abort", "HEAD", &orig_head,
> diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh
> index 501a7a9d211..f889f25a98f 100755
> --- a/t/t4151-am-abort.sh
> +++ b/t/t4151-am-abort.sh
> @@ -195,7 +195,7 @@ test_expect_success 'am --abort leaves index stat info alone' '
>  	git diff-files --exit-code --quiet
>  '
>  
> -test_expect_failure 'git am --abort return failed exit status when it fails' '
> +test_expect_success 'git am --abort return failed exit status when it fails' '
>  	test_when_finished "rm -rf file-2/ && git reset --hard" &&
>  	git checkout changes &&
>  	git format-patch -1 --stdout conflicting >changes.mbox &&

> This series documents a few issues with git am --abort in the form of new
> testcases, and fixes one of them. However, while I was surprised the abort
> left the working directory dirty, I couldn't find any documentation to
> confirm it should or shouldn't be, and reading the code led me to question
> if perhaps it was intentional. Anyway, if it's intended, let me know and
> I'll drop that testcase.

>
> For frame of reference, these were some issues I found while working on
> unintentional removal of untracked files/directories and the current working
> directory, and I'm just submitting them separately.
>
> Elijah Newren (2):
>   t4151: document a pair of am --abort bugs
>   am: fix incorrect exit status on am fail to abort
>
>  builtin/am.c        |  3 ++-
>  t/t4151-am-abort.sh | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+), 1 deletion(-)
>
>
> base-commit: e0a2f5cbc585657e757385ad918f167f519cfb96
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1087%2Fnewren%2Fam-issues-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1087/newren/am-issues-v1
> Pull-Request: https://github.com/git/git/pull/1087
> --
> gitgitgadget
>

> This series documents a few issues with git am --abort in the form of new
> testcases, and fixes one of them. However, while I was surprised the abort
> left the working directory dirty, I couldn't find any documentation to
> confirm it should or shouldn't be, and reading the code led me to question
> if perhaps it was intentional. Anyway, if it's intended, let me know and
> I'll drop that testcase.
>
> For frame of reference, these were some issues I found while working on
> unintentional removal of untracked files/directories and the current working
> directory, and I'm just submitting them separately.
>
> Changes since v1:
>
>  * Added a patch to tweak the documentation to clarify that partial cleaning
>    of worktree is expected with --abort
>  * Tweaked the second test to be a test that unrelated dirty files are kept,
>    as suggested by Junio