A pair of git am --abort issues

PATCH Superseded Elijah Newren via GitGitGadget 5 participants 15 messages Sep 8, 2021 – Sep 13, 2021 Documentation

Share & tools

Elijah Newren via GitGitGadget

author

2021-09-08 02:17 UTC #0 lore raw

cover A pair of git am --abort issues

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

Signature · 1 line

gitgitgadget

Elijah Newren via GitGitGadget

author

2021-09-08 02:17 UTC #1 lore raw

PATCH 1/2 t4151: document a pair of am --abort bugs

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 +31 -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 &&
+	test_must_be_empty actual
+'
+
 test_done

Signature · 1 line

gitgitgadget

Bagas Sanjaya

2021-09-08 05:13 UTC #2 lore raw

Re: [PATCH 1/2] t4151: document a pair of am --abort bugs

Quoted 46 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 +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 &&
+

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

Quoted 10 lines

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

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

Signature · 1 line

An old man doll... just what I always wanted! - Clara

Elijah Newren

2021-09-08 05:24 UTC #3 lore raw

Re: [PATCH 1/2] t4151: document a pair of am --abort bugs

Quoted 53 lines · Bagas Sanjaya

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 &&

Quoted 8 lines

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.

There's no point documenting the first one since it'll be fixed by the next patch. As for the second, as I noted in my cover letter, I'm not quite sure that it really is a bug. If it isn't, the second testcase should be dropped. However, if the second testcase represents an actual bug rather than me just misjudging the intent, then your suggestion certainly makes sense.

Junio C Hamano

2021-09-08 07:02 UTC #4 lore raw

Re: [PATCH 1/2] t4151: document a pair of am --abort bugs

In reply to Elijah Newren via GitGitGadget

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

Quoted 12 lines · Elijah Newren via GitGitGadget

t/t4151-am-abort.sh +12 -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
+	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. Even though you may want "am --abort" to restore the paths that the operation touched to their original state, I am not sure if that is always possible, given that there may have been dirty working tree files to begin with.

And the above test would succeed if "git am --abort" internally called "git reset --hard", which definitely is not what we want to see. We want the local changes in dirty working tree files that weren't involved in the patch application to stay, even after running "am --abort".

Elijah Newren

2021-09-08 08:00 UTC #5 lore raw

Re: [PATCH 1/2] t4151: document a pair of am --abort bugs

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.

Junio C Hamano

2021-09-08 16:33 UTC #6 lore raw

Re: [PATCH 1/2] t4151: document a pair of am --abort bugs

Quoted 22 lines · Elijah Newren

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.

Quoted 5 lines

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.

Actually, if you test that unrelated dirty files are kept, then the test is a welcome addition. "returns us to a 'clean' state" needs a bit different title, though.

Thanks.

Elijah Newren via GitGitGadget

author

2021-09-08 02:17 UTC #7 lore raw

PATCH 2/2 am: fix incorrect exit status on am fail to abort

In reply to Elijah Newren via GitGitGadget

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 &&

Signature · 1 line

gitgitgadget

Junio C Hamano

2021-09-08 16:33 UTC #8 lore raw

Re: [PATCH 2/2] am: fix incorrect exit status on am fail to abort

Quoted 4 lines · Elijah Newren via GitGitGadget

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

Obviously correct.

Quoted 31 lines

 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 &&

Johannes Schindelin

2021-09-08 08:08 UTC #9 lore raw

In reply to Elijah Newren via GitGitGadget

Hi Elijah,

Quoted 6 lines · Elijah Newren via 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.

As far as I understand, git am --abort should indeed clean up. The behavior you described is what I would expect more of git am --quit.

The patches look good to me.

Thanks!
Dscho

Quoted 21 lines

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

Elijah Newren via GitGitGadget

author

2021-09-10 10:31 UTC #10 lore raw

v2 cover A pair of git am --abort issues

In reply to Elijah Newren via GitGitGadget

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

Elijah Newren (3):
git-am.txt: clarify --abort behavior
t4151: add a few am --abort tests
am: fix incorrect exit status on am fail to abort

 Documentation/git-am.txt |  2 ++
 builtin/am.c             |  3 ++-
 t/t4151-am-abort.sh      | 39 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 43 insertions(+), 1 deletion(-)

base-commit: e0a2f5cbc585657e757385ad918f167f519cfb96
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1087%2Fnewren%2Fam-issues-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1087/newren/am-issues-v2
Pull-Request: https://github.com/git/git/pull/1087

Range-diff vs v1:

 -:  ----------- > 1:  e199df0f3bc git-am.txt: clarify --abort behavior
 1:  b8a418bc63a ! 2:  38a7063b959 t4151: document a pair of am --abort bugs
     @@ Metadata
      Author: Elijah Newren <[email protected]>
      
       ## Commit message ##
     -    t4151: document a pair of am --abort bugs
     +    t4151: add a few am --abort tests
      
          Signed-off-by: Elijah Newren <[email protected]>
      
     @@ t/t4151-am-abort.sh: test_expect_success setup '
       	git format-patch --no-numbered initial &&
      +	git checkout -b conflicting initial &&
      +	echo different >>file-1 &&
     -+	git commit -a -m different &&
     ++	echo whatever >new-file &&
     ++	git add file-1 new-file &&
     ++	git commit -m different &&
       	git checkout -b side initial &&
       	echo local change >file-2-expect
       '
     @@ t/t4151-am-abort.sh: test_expect_success 'am --abort leaves index stat info alon
       '
       
      +test_expect_failure 'git am --abort return failed exit status when it fails' '
     -+	test_when_finished "rm -rf file-2/ && git reset --hard" &&
     ++	test_when_finished "rm -rf file-2/ && git reset --hard && git am --abort" &&
      +	git checkout changes &&
      +	git format-patch -1 --stdout conflicting >changes.mbox &&
      +	test_must_fail git am --3way changes.mbox &&
     @@ t/t4151-am-abort.sh: test_expect_success 'am --abort leaves index stat info alon
      +	test_path_is_dir file-2/
      +'
      +
     -+test_expect_failure 'git am --abort returns us to a clean state' '
     ++test_expect_success 'git am --abort cleans relevant files' '
      +	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 &&
     ++	test_path_is_file new-file &&
     ++	echo further changes >>file-1 &&
     ++	echo change other file >>file-2 &&
      +
     -+	# Abort, and expect the related change to go away too
     ++	# Abort, and expect the files touched by am to be reverted
      +	git am --abort &&
     -+	git status --porcelain -uno >actual &&
     -+	test_must_be_empty actual
     ++
     ++	test_path_is_missing new-file &&
     ++
     ++	# Files not involved in am operation are left modified
     ++	git diff --name-only changes >actual &&
     ++	test_write_lines file-2 >expect &&
     ++	test_cmp expect actual
      +'
      +
       test_done

2: 5fa7daf264b ! 3: a46f5c79fbf am: fix incorrect exit status on am fail to abort
@@ t/t4151-am-abort.sh: test_expect_success 'am --abort leaves index stat info alon

-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" &&
+ test_when_finished "rm -rf file-2/ && git reset --hard && git am --abort" &&
git checkout changes &&
git format-patch -1 --stdout conflicting >changes.mbox &&

Signature · 1 line

gitgitgadget

Elijah Newren via GitGitGadget

author

2021-09-10 10:31 UTC #11 lore raw

PATCH v2 1/3 git-am.txt: clarify --abort behavior

From: Elijah Newren <[email protected]>

Both Johannes and I assumed (perhaps due to familiarity with rebase) that am --abort would return the user to a clean state. However, since am, unlike rebase, is intended to be used within a dirty working tree, --abort will only clean the files involved in the am operation.

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

---
 Documentation/git-am.txt | 2 ++
 1 file changed, 2 insertions(+)

Documentation/git-am.txt +2 -0

index 8714dfcb762..0a4a984dfde 100644
@@ -178,6 +178,8 @@ default.   You can use `--no-utf8` to override this.
 
 --abort::
 	Restore the original branch and abort the patching operation.
+	Revert contents of files involved in the am operation to their
+	pre-am state.
 
 --quit::
 	Abort the patching operation but keep HEAD and the index

Signature · 1 line

gitgitgadget

Elijah Newren via GitGitGadget

author

2021-09-10 10:31 UTC #12 lore raw

PATCH v2 2/3 t4151: add a few am --abort tests

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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

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

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

index 9d8d3c72e7e..15f2f92cd76 100755
@@ -23,7 +23,13 @@ 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 &&
+	echo whatever >new-file &&
+	git add file-1 new-file &&
+	git commit -m different &&
 	git checkout -b side initial &&
 	echo local change >file-2-expect
 '

@@ -191,4 +197,37 @@ 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 am --abort" &&
+	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_success 'git am --abort cleans relevant files' '
+	git checkout changes &&
+	git format-patch -1 --stdout conflicting >changes.mbox &&
+	test_must_fail git am --3way changes.mbox &&
+
+	test_path_is_file new-file &&
+	echo further changes >>file-1 &&
+	echo change other file >>file-2 &&
+
+	# Abort, and expect the files touched by am to be reverted
+	git am --abort &&
+
+	test_path_is_missing new-file &&
+
+	# Files not involved in am operation are left modified
+	git diff --name-only changes >actual &&
+	test_write_lines file-2 >expect &&
+	test_cmp expect actual
+'
+
 test_done

Signature · 1 line

gitgitgadget

Elijah Newren via GitGitGadget

author

2021-09-10 10:31 UTC #13 lore raw

PATCH v2 3/3 am: fix incorrect exit status on am fail to abort

In reply to Elijah Newren via GitGitGadget

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 15f2f92cd76..2374151662b 100755
@@ -197,7 +197,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 am --abort" &&
 	git checkout changes &&
 	git format-patch -1 --stdout conflicting >changes.mbox &&

Signature · 1 line

gitgitgadget

Johannes Schindelin

2021-09-13 12:08 UTC #14 lore raw

In reply to Elijah Newren via GitGitGadget

Hi Elijah,

Quoted 11 lines · Elijah Newren via 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.

Quoted 6 lines

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

FWIW I gave this a light read-over and am happy with it!

Ciao,
Dscho