A pair of git am --abort issues

From: Elijah Newren <[email protected]>

Signed-off-by Elijah Newren <[email protected]>

---
  t/t4151-am-abort.sh | 31 +++++++++++++++++++++++++++++++
  1 file changed, 31 insertions(+)

t/t4151-am-abort.sh +22 -0

index 9d8d3c72e7e..501a7a9d211 100755
@@ -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 &&
+

Quoted 51 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

Signed-off-by Elijah Newren <[email protected]>

---
  t/t4151-am-abort.sh | 31 +++++++++++++++++++++++++++++++
  1 file changed, 31 insertions(+)

t/t4151-am-abort.sh +28 -0

index 9d8d3c72e7e..501a7a9d211 100755
@@ -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 &&

t/t4151-am-abort.sh test_expect_success 'am --abort leaves index stat info alone' ' · line 225

Quoted 5 lines

t/t4151-am-abort.sh +3 -0

+     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.

Quoted 9 lines · Elijah Newren via GitGitGadget

t/t4151-am-abort.sh +9 -0

+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

t/t4151-am-abort.sh test_expect_success 'am --abort leaves index stat info alone' ' · line 222

Quoted 3 lines

t/t4151-am-abort.sh +3 -0

+     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.

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

Quoted 12 lines · Junio C Hamano

Quoted 9 lines · Elijah Newren via GitGitGadget

t/t4151-am-abort.sh +9 -0

+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

Quoted 8 lines

t/t4151-am-abort.sh test_expect_success 'am --abort leaves index stat info alone' ' · line 222

Quoted 3 lines

t/t4151-am-abort.sh +3 -0

+     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.

From: Elijah Newren <[email protected]>

Signed-off-by Elijah Newren <[email protected]>

---

 builtin/am.c        | 3 ++-
 t/t4151-am-abort.sh | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

builtin/am.c +2 -1

index 0c2ad96b70e..c79e0167e98 100644
@@ -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,

t/t4151-am-abort.sh +1 -1

index 501a7a9d211..f889f25a98f 100755
@@ -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.

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