[PATCH 0/6] am --skip/--abort: improve index/worktree cleanup

[PATCH 0/6] am --skip/--abort: improve index/worktree cleanup

[Paul Tan]

Currently git-am attempts to clean up the index/worktree when skipping or
aborting, but does not do it very well:

* While it discards conflicted index entries, it does not remove any other
  modifications made to the index due to a previous threeway merge.

* It expects HEAD/ORIG_HEAD to exist, and thus does not clean up the index when
  on an unborn branch.

This patch series addresses the above two general problems by making the calls
to git-read-tree aware of the differences between our current index and
HEAD/ORIG_HEAD, and by explictly defining what happens when we are on an unborn
branch.


Paul Tan (6):
  am --skip: revert changes introduced by failed 3way merge
  am -3: support 3way merge on unborn branch
  am --skip: support skipping while on unborn branch
  am --abort: revert changes introduced by failed 3way merge
  am --abort: support aborting to unborn branch
  am --abort: keep unrelated commits on unborn branch

 git-am.sh           | 31 ++++++++++++++------
 t/t4151-am-abort.sh | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 104 insertions(+), 8 deletions(-)

-- 
2.1.4

[PATCH 1/6] am --skip: revert changes introduced by failed 3way merge

[Paul Tan]

Even when a merge conflict occurs with am --3way, the index will be
modified with the results of any succesfully merged files (such as a new
file). These changes to the index will not be reverted with a
"git read-tree --reset -u HEAD HEAD", as git read-tree will not be aware
of how the current index differs from HEAD.

To fix this, we first reset any conflicting entries from the index. The
resulting index will contain the results of successfully merged files.
We write the index to a tree, then use git read-tree -m to fast-forward
the "index tree" back to HEAD, thus undoing all the changes from the
failed merge.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 git-am.sh           |  7 ++++++-
 t/t4151-am-abort.sh | 11 +++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/git-am.sh b/git-am.sh
index 761befb..df3c8f4 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -69,6 +69,8 @@ then
 	cmdline="$cmdline -3"
 fi
 
+empty_tree=4b825dc642cb6eb9a060e54bf8d69288fbee4904
+
 sq () {
 	git rev-parse --sq-quote "$@"
 }
@@ -502,7 +504,10 @@ then
 		;;
 	t,)
 		git rerere clear
-		git read-tree --reset -u HEAD HEAD
+		head_tree=$(git rev-parse --verify -q HEAD || echo $empty_tree) &&
+		git read-tree --reset -u $head_tree $head_tree &&
+		index_tree=$(git write-tree) &&
+		git read-tree -m -u $index_tree $head_tree
 		orig_head=$(cat "$GIT_DIR/ORIG_HEAD")
 		git reset HEAD
 		git update-ref ORIG_HEAD $orig_head
diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh
index 8d90634..e15acc8 100755
--- a/t/t4151-am-abort.sh
+++ b/t/t4151-am-abort.sh
@@ -63,6 +63,17 @@ do
 
 done
 
+test_expect_success 'am -3 --skip removes otherfile-4' '
+	git reset --hard initial &&
+	test_must_fail git am -3 0003-*.patch &&
+	test 3 -eq $(git ls-files -u | wc -l) &&
+	test 4 = "$(cat otherfile-4)" &&
+	git am --skip &&
+	test_cmp_rev initial HEAD &&
+	test -z $(git ls-files -u) &&
+	test_path_is_missing otherfile-4
+'
+
 test_expect_success 'am --abort will keep the local commits intact' '
 	test_must_fail git am 0004-*.patch &&
 	test_commit unrelated &&
-- 
2.1.4

[PATCH 2/6] am -3: support 3way merge on unborn branch

[Paul Tan]

While on an unborn branch, git am -3 will fail to do a threeway merge as
it references HEAD as "our tree", but HEAD does not point to a valid
tree.

Fix this by using an empty tree as "our tree" when we are on an unborn
branch.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 git-am.sh           | 3 ++-
 t/t4151-am-abort.sh | 9 +++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/git-am.sh b/git-am.sh
index df3c8f4..c847b58 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -179,7 +179,8 @@ It does not apply to blobs recorded in its index.")"
     then
 	    GIT_MERGE_VERBOSITY=0 && export GIT_MERGE_VERBOSITY
     fi
-    git-merge-recursive $orig_tree -- HEAD $his_tree || {
+    our_tree=$(git rev-parse --verify -q HEAD || echo $empty_tree)
+    git-merge-recursive $orig_tree -- $our_tree $his_tree || {
 	    git rerere $allow_rerere_autoupdate
 	    die "$(gettext "Failed to merge in the changes.")"
     }
diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh
index e15acc8..b618ee0 100755
--- a/t/t4151-am-abort.sh
+++ b/t/t4151-am-abort.sh
@@ -83,4 +83,13 @@ test_expect_success 'am --abort will keep the local commits intact' '
 	test_cmp expect actual
 '
 
+test_expect_success 'am -3 stops on conflict on unborn branch' '
+	git checkout -f --orphan orphan &&
+	git reset &&
+	rm -f otherfile-4 &&
+	test_must_fail git am -3 0003-*.patch &&
+	test 2 -eq $(git ls-files -u | wc -l) &&
+	test 4 = "$(cat otherfile-4)"
+'
+
 test_done
-- 
2.1.4

[PATCH 3/6] am --skip: support skipping while on unborn branch

[Paul Tan]

When git am --skip is run, git am will copy HEAD's tree entries to the
index with "git reset HEAD". However, on an unborn branch, HEAD does not
point to a tree, so "git reset HEAD" will fail.

Fix this by treating HEAD as en empty tree when we are on an unborn
branch.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 git-am.sh           |  4 +---
 t/t4151-am-abort.sh | 10 ++++++++++
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index c847b58..67f4f25 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -509,9 +509,7 @@ then
 		git read-tree --reset -u $head_tree $head_tree &&
 		index_tree=$(git write-tree) &&
 		git read-tree -m -u $index_tree $head_tree
-		orig_head=$(cat "$GIT_DIR/ORIG_HEAD")
-		git reset HEAD
-		git update-ref ORIG_HEAD $orig_head
+		git read-tree $head_tree
 		;;
 	,t)
 		if test -f "$dotest/rebasing"
diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh
index b618ee0..ea4a49e 100755
--- a/t/t4151-am-abort.sh
+++ b/t/t4151-am-abort.sh
@@ -92,4 +92,14 @@ test_expect_success 'am -3 stops on conflict on unborn branch' '
 	test 4 = "$(cat otherfile-4)"
 '
 
+test_expect_success 'am -3 --skip clears index on unborn branch' '
+	test_path_is_dir .git/rebase-apply &&
+	echo tmpfile >tmpfile &&
+	git add tmpfile &&
+	git am --skip &&
+	test -z "$(git ls-files)" &&
+	test_path_is_missing otherfile-4 &&
+	test_path_is_missing tmpfile
+'
+
 test_done
-- 
2.1.4

[PATCH 4/6] am --abort: revert changes introduced by failed 3way merge

[Paul Tan]

Even when a merge conflict occurs with am --3way, the index will be
modified with the results of any successfully merged files. These
changes to the index will not be reverted with a
"git read-tree --reset -u HEAD ORIG_HEAD", as git read-tree will not be
aware of how the current index differs from HEAD or ORIG_HEAD.

To fix this, we first reset any conflicting entries in the index. The
resulting index will contain the results of successfully merged files
introduced by the failed merge. We write this index to a tree, and then
use git read-tree to fast-forward this "index tree" back to ORIG_HEAD,
thus undoing all the changes from the failed merge.

When we are on an unborn branch, HEAD and ORIG_HEAD will not point to
valid trees. In this case, use an empty tree.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 git-am.sh           |  6 +++++-
 t/t4151-am-abort.sh | 23 +++++++++++++++++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/git-am.sh b/git-am.sh
index 67f4f25..e4fe3ed 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -519,7 +519,11 @@ then
 		git rerere clear
 		if safe_to_abort
 		then
-			git read-tree --reset -u HEAD ORIG_HEAD
+			head_tree=$(git rev-parse --verify -q HEAD || echo $empty_tree) &&
+			git read-tree --reset -u $head_tree $head_tree &&
+			index_tree=$(git write-tree) &&
+			orig_head=$(git rev-parse --verify -q ORIG_HEAD || echo $empty_tree) &&
+			git read-tree -m -u $index_tree $orig_head
 			git reset ORIG_HEAD
 		fi
 		rm -fr "$dotest"
diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh
index ea4a49e..2366042 100755
--- a/t/t4151-am-abort.sh
+++ b/t/t4151-am-abort.sh
@@ -70,6 +70,17 @@ test_expect_success 'am -3 --skip removes otherfile-4' '
 	test 4 = "$(cat otherfile-4)" &&
 	git am --skip &&
 	test_cmp_rev initial HEAD &&
+	test -z "$(git ls-files -u)" &&
+	test_path_is_missing otherfile-4
+'
+
+test_expect_success 'am -3 --abort removes otherfile-4' '
+	git reset --hard initial &&
+	test_must_fail git am -3 0003-*.patch &&
+	test 3 -eq $(git ls-files -u | wc -l) &&
+	test 4 = "$(cat otherfile-4)" &&
+	git am --abort &&
+	test_cmp_rev initial HEAD &&
 	test -z $(git ls-files -u) &&
 	test_path_is_missing otherfile-4
 '
@@ -102,4 +113,16 @@ test_expect_success 'am -3 --skip clears index on unborn branch' '
 	test_path_is_missing tmpfile
 '
 
+test_expect_success 'am -3 --abort removes otherfile-4 on unborn branch' '
+	git checkout -f --orphan orphan &&
+	git reset &&
+	rm -f otherfile-4 file-1 &&
+	test_must_fail git am -3 0003-*.patch &&
+	test 2 -eq $(git ls-files -u | wc -l) &&
+	test 4 = "$(cat otherfile-4)" &&
+	git am --abort &&
+	test -z "$(git ls-files -u)" &&
+	test_path_is_missing otherfile-4
+'
+
 test_done
-- 
2.1.4

[PATCH 5/6] am --abort: support aborting to unborn branch

[Paul Tan]

When git-am is first run on an unborn branch, no ORIG_HEAD is created.
As such, any applied commits will remain even after a git am --abort.

To be consistent with the behavior of git am --abort when it is not run
from an unborn branch, we empty the index, and then destroy the branch
pointed to by HEAD if there is no ORIG_HEAD.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 git-am.sh           |  9 ++++++++-
 t/t4151-am-abort.sh | 17 +++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/git-am.sh b/git-am.sh
index e4fe3ed..95f90a0 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -524,7 +524,14 @@ then
 			index_tree=$(git write-tree) &&
 			orig_head=$(git rev-parse --verify -q ORIG_HEAD || echo $empty_tree) &&
 			git read-tree -m -u $index_tree $orig_head
-			git reset ORIG_HEAD
+			if git rev-parse --verify -q ORIG_HEAD >/dev/null 2>&1
+			then
+				git reset ORIG_HEAD
+			else
+				git read-tree $empty_tree
+				curr_branch=$(git symbolic-ref HEAD 2>/dev/null) &&
+				git update-ref -d $curr_branch
+			fi
 		fi
 		rm -fr "$dotest"
 		exit ;;
diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh
index 2366042..b2a7fc5 100755
--- a/t/t4151-am-abort.sh
+++ b/t/t4151-am-abort.sh
@@ -14,6 +14,7 @@ test_expect_success setup '
 	git add file-1 file-2 &&
 	git commit -m initial &&
 	git tag initial &&
+	git format-patch --stdout --root initial >initial.patch &&
 	for i in 2 3 4 5 6
 	do
 		echo $i >>file-1 &&
@@ -125,4 +126,20 @@ test_expect_success 'am -3 --abort removes otherfile-4 on unborn branch' '
 	test_path_is_missing otherfile-4
 '
 
+test_expect_success 'am -3 --abort on unborn branch removes applied commits' '
+	git checkout -f --orphan orphan &&
+	git reset &&
+	rm -f otherfile-4 otherfile-2 file-1 file-2 &&
+	test_must_fail git am -3 initial.patch 0003-*.patch &&
+	test 3 -eq $(git ls-files -u | wc -l) &&
+	test 4 = "$(cat otherfile-4)" &&
+	git am --abort &&
+	test -z "$(git ls-files -u)" &&
+	test_path_is_missing otherfile-4 &&
+	test_path_is_missing file-1 &&
+	test_path_is_missing file-2 &&
+	test 0 -eq $(git log --oneline 2>/dev/null | wc -l) &&
+	test refs/heads/orphan = "$(git symbolic-ref HEAD)"
+'
+
 test_done
-- 
2.1.4

[PATCH 6/6] am --abort: keep unrelated commits on unborn branch

[Paul Tan]

Since 7b3b7e3 (am --abort: keep unrelated commits since the last failure
and warn, 2010-12-21), git-am would refuse to rewind HEAD if commits
were made since the last git-am failure. This check was implemented in
safe_to_abort(), which checked to see if HEAD's hash matched the
abort-safety file.

However, this check was skipped if the abort-safety file was empty,
which can happen if git-am failed while on an unborn branch. As such, if
any commits were made since then, they would be discarded. Fix this by
carrying on the abort safety check even if the abort-safety file is
empty.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 git-am.sh           |  2 +-
 t/t4151-am-abort.sh | 11 +++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/git-am.sh b/git-am.sh
index 95f90a0..4324bb1 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -87,7 +87,7 @@ safe_to_abort () {
 		return 1
 	fi
 
-	if ! test -s "$dotest/abort-safety"
+	if ! test -f "$dotest/abort-safety"
 	then
 		return 0
 	fi
diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh
index b2a7fc5..833e7b2 100755
--- a/t/t4151-am-abort.sh
+++ b/t/t4151-am-abort.sh
@@ -142,4 +142,15 @@ test_expect_success 'am -3 --abort on unborn branch removes applied commits' '
 	test refs/heads/orphan = "$(git symbolic-ref HEAD)"
 '
 
+test_expect_success 'am --abort on unborn branch will keep local commits intact' '
+	git checkout -f --orphan orphan &&
+	git reset &&
+	test_must_fail git am 0004-*.patch &&
+	test_commit unrelated2 &&
+	git rev-parse HEAD >expect &&
+	git am --abort &&
+	git rev-parse HEAD >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.1.4

Re: [PATCH 0/6] am --skip/--abort: improve index/worktree cleanup

[Stefan Beller]

On Sat, Jun 6, 2015 at 4:46 AM, Paul Tan <pyokagan@gmail.com> wrote:
> Currently git-am attempts to clean up the index/worktree when skipping or
> aborting, but does not do it very well:
>
> * While it discards conflicted index entries, it does not remove any other
>   modifications made to the index due to a previous threeway merge.
>
> * It expects HEAD/ORIG_HEAD to exist, and thus does not clean up the index when
>   on an unborn branch.
>
> This patch series addresses the above two general problems by making the calls
> to git-read-tree aware of the differences between our current index and
> HEAD/ORIG_HEAD, and by explictly defining what happens when we are on an unborn
> branch.
>
>
> Paul Tan (6):
>   am --skip: revert changes introduced by failed 3way merge
>   am -3: support 3way merge on unborn branch
>   am --skip: support skipping while on unborn branch
>   am --abort: revert changes introduced by failed 3way merge
>   am --abort: support aborting to unborn branch
>   am --abort: keep unrelated commits on unborn branch

The whole series looks good to me.

Thanks,
Stefan

>
>  git-am.sh           | 31 ++++++++++++++------
>  t/t4151-am-abort.sh | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 104 insertions(+), 8 deletions(-)
>
> --
> 2.1.4
>

Re: [PATCH 4/6] am --abort: revert changes introduced by failed 3way merge

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> Even when a merge conflict occurs with am --3way, the index will be
> modified with the results of any successfully merged files. These
> changes to the index will not be reverted with a
> "git read-tree --reset -u HEAD ORIG_HEAD", as git read-tree will not be
> aware of how the current index differs from HEAD or ORIG_HEAD.
>
> To fix this, we first reset any conflicting entries in the index. The
> resulting index will contain the results of successfully merged files
> introduced by the failed merge. We write this index to a tree, and then
> use git read-tree to fast-forward this "index tree" back to ORIG_HEAD,
> thus undoing all the changes from the failed merge.
>
> When we are on an unborn branch, HEAD and ORIG_HEAD will not point to
> valid trees. In this case, use an empty tree.
>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>  git-am.sh           |  6 +++++-
>  t/t4151-am-abort.sh | 23 +++++++++++++++++++++++
>  2 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/git-am.sh b/git-am.sh
> index 67f4f25..e4fe3ed 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -519,7 +519,11 @@ then
>  		git rerere clear
>  		if safe_to_abort
>  		then
> -			git read-tree --reset -u HEAD ORIG_HEAD
> +			head_tree=$(git rev-parse --verify -q HEAD || echo $empty_tree) &&
> +			git read-tree --reset -u $head_tree $head_tree &&
> +			index_tree=$(git write-tree) &&
> +			orig_head=$(git rev-parse --verify -q ORIG_HEAD || echo $empty_tree) &&
> +			git read-tree -m -u $index_tree $orig_head
>  			git reset ORIG_HEAD

What would the last "reset" do when ORIG_HEAD is invalid?  In other
words, does it make sense to worry about the case where ORIG_HEAD
does not exist?  At which point in the flow does it get written using
what information?

>  		fi
>  		rm -fr "$dotest"
> diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh
> index ea4a49e..2366042 100755
> --- a/t/t4151-am-abort.sh
> +++ b/t/t4151-am-abort.sh
> @@ -70,6 +70,17 @@ test_expect_success 'am -3 --skip removes otherfile-4' '
>  	test 4 = "$(cat otherfile-4)" &&
>  	git am --skip &&
>  	test_cmp_rev initial HEAD &&
> +	test -z "$(git ls-files -u)" &&
> +	test_path_is_missing otherfile-4
> +'
> +
> +test_expect_success 'am -3 --abort removes otherfile-4' '
> +	git reset --hard initial &&
> +	test_must_fail git am -3 0003-*.patch &&
> +	test 3 -eq $(git ls-files -u | wc -l) &&
> +	test 4 = "$(cat otherfile-4)" &&
> +	git am --abort &&
> +	test_cmp_rev initial HEAD &&
>  	test -z $(git ls-files -u) &&
>  	test_path_is_missing otherfile-4
>  '
> @@ -102,4 +113,16 @@ test_expect_success 'am -3 --skip clears index on unborn branch' '
>  	test_path_is_missing tmpfile
>  '
>  
> +test_expect_success 'am -3 --abort removes otherfile-4 on unborn branch' '
> +	git checkout -f --orphan orphan &&
> +	git reset &&
> +	rm -f otherfile-4 file-1 &&
> +	test_must_fail git am -3 0003-*.patch &&
> +	test 2 -eq $(git ls-files -u | wc -l) &&
> +	test 4 = "$(cat otherfile-4)" &&
> +	git am --abort &&
> +	test -z "$(git ls-files -u)" &&
> +	test_path_is_missing otherfile-4
> +'
> +
>  test_done

Re: [PATCH 5/6] am --abort: support aborting to unborn branch

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> When git-am is first run on an unborn branch, no ORIG_HEAD is created.
> As such, any applied commits will remain even after a git am --abort.

I think this answered my question on 4/6; that may indicate that
these two are either done in a wrong order or perhaps should be a
single change.



>
> To be consistent with the behavior of git am --abort when it is not run
> from an unborn branch, we empty the index, and then destroy the branch
> pointed to by HEAD if there is no ORIG_HEAD.
>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>  git-am.sh           |  9 ++++++++-
>  t/t4151-am-abort.sh | 17 +++++++++++++++++
>  2 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/git-am.sh b/git-am.sh
> index e4fe3ed..95f90a0 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -524,7 +524,14 @@ then
>  			index_tree=$(git write-tree) &&
>  			orig_head=$(git rev-parse --verify -q ORIG_HEAD || echo $empty_tree) &&
>  			git read-tree -m -u $index_tree $orig_head
> -			git reset ORIG_HEAD
> +			if git rev-parse --verify -q ORIG_HEAD >/dev/null 2>&1
> +			then
> +				git reset ORIG_HEAD
> +			else
> +				git read-tree $empty_tree
> +				curr_branch=$(git symbolic-ref HEAD 2>/dev/null) &&
> +				git update-ref -d $curr_branch
> +			fi
>  		fi
>  		rm -fr "$dotest"
>  		exit ;;
> diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh
> index 2366042..b2a7fc5 100755
> --- a/t/t4151-am-abort.sh
> +++ b/t/t4151-am-abort.sh
> @@ -14,6 +14,7 @@ test_expect_success setup '
>  	git add file-1 file-2 &&
>  	git commit -m initial &&
>  	git tag initial &&
> +	git format-patch --stdout --root initial >initial.patch &&
>  	for i in 2 3 4 5 6
>  	do
>  		echo $i >>file-1 &&
> @@ -125,4 +126,20 @@ test_expect_success 'am -3 --abort removes otherfile-4 on unborn branch' '
>  	test_path_is_missing otherfile-4
>  '
>  
> +test_expect_success 'am -3 --abort on unborn branch removes applied commits' '
> +	git checkout -f --orphan orphan &&
> +	git reset &&
> +	rm -f otherfile-4 otherfile-2 file-1 file-2 &&
> +	test_must_fail git am -3 initial.patch 0003-*.patch &&
> +	test 3 -eq $(git ls-files -u | wc -l) &&
> +	test 4 = "$(cat otherfile-4)" &&
> +	git am --abort &&
> +	test -z "$(git ls-files -u)" &&
> +	test_path_is_missing otherfile-4 &&
> +	test_path_is_missing file-1 &&
> +	test_path_is_missing file-2 &&
> +	test 0 -eq $(git log --oneline 2>/dev/null | wc -l) &&
> +	test refs/heads/orphan = "$(git symbolic-ref HEAD)"
> +'
> +
>  test_done

Re: [PATCH 6/6] am --abort: keep unrelated commits on unborn branch

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> Since 7b3b7e3 (am --abort: keep unrelated commits since the last failure
> and warn, 2010-12-21), git-am would refuse to rewind HEAD if commits
> were made since the last git-am failure. This check was implemented in
> safe_to_abort(), which checked to see if HEAD's hash matched the
> abort-safety file.
>
> However, this check was skipped if the abort-safety file was empty,
> which can happen if git-am failed while on an unborn branch.

Shouldn't we then be checking that the HEAD is still unborn if this
file is found and says that there was no history in the beginning,
in order to give the "am on top of unborn" workflow the same degree
of safety?  Or is the fact that the file is empty sufficient not to
match any valid commit name that is 40-hex?

> As such, if
> any commits were made since then, they would be discarded. Fix this by
> carrying on the abort safety check even if the abort-safety file is
> empty.
>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>  git-am.sh           |  2 +-
>  t/t4151-am-abort.sh | 11 +++++++++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/git-am.sh b/git-am.sh
> index 95f90a0..4324bb1 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -87,7 +87,7 @@ safe_to_abort () {
>  		return 1
>  	fi
>  
> -	if ! test -s "$dotest/abort-safety"
> +	if ! test -f "$dotest/abort-safety"
>  	then
>  		return 0
>  	fi
> diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh
> index b2a7fc5..833e7b2 100755
> --- a/t/t4151-am-abort.sh
> +++ b/t/t4151-am-abort.sh
> @@ -142,4 +142,15 @@ test_expect_success 'am -3 --abort on unborn branch removes applied commits' '
>  	test refs/heads/orphan = "$(git symbolic-ref HEAD)"
>  '
>  
> +test_expect_success 'am --abort on unborn branch will keep local commits intact' '
> +	git checkout -f --orphan orphan &&
> +	git reset &&
> +	test_must_fail git am 0004-*.patch &&
> +	test_commit unrelated2 &&
> +	git rev-parse HEAD >expect &&
> +	git am --abort &&
> +	git rev-parse HEAD >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_done

Re: [PATCH 6/6] am --abort: keep unrelated commits on unborn branch

[Paul Tan]

On Tue, Jun 9, 2015 at 4:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Paul Tan <pyokagan@gmail.com> writes:
>
>> Since 7b3b7e3 (am --abort: keep unrelated commits since the last failure
>> and warn, 2010-12-21), git-am would refuse to rewind HEAD if commits
>> were made since the last git-am failure. This check was implemented in
>> safe_to_abort(), which checked to see if HEAD's hash matched the
>> abort-safety file.
>>
>> However, this check was skipped if the abort-safety file was empty,
>> which can happen if git-am failed while on an unborn branch.
>
> Shouldn't we then be checking that the HEAD is still unborn if this
> file is found and says that there was no history in the beginning,
> in order to give the "am on top of unborn" workflow the same degree
> of safety?

We do already check to see if the HEAD is still unborn:

        abort_safety=$(cat "$dotest/abort-safety")
        if test "z$(git rev-parse --verify -q HEAD)" = "z$abort_safety"
        then
            return 0
        fi
        gettextln "You seem to have moved HEAD since the last 'am' failure.
    Not rewinding to ORIG_HEAD" >&2

If HEAD is unborn, then git rev-parse will not print anything, so we
would be comparing an empty string to an empty string.

Thanks,
Paul

Re: [PATCH 5/6] am --abort: support aborting to unborn branch

[Paul Tan]

On Tue, Jun 9, 2015 at 4:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Paul Tan <pyokagan@gmail.com> writes:
>
>> When git-am is first run on an unborn branch, no ORIG_HEAD is created.
>> As such, any applied commits will remain even after a git am --abort.
>
> I think this answered my question on 4/6; that may indicate that
> these two are either done in a wrong order or perhaps should be a
> single change.

Ah right, patch 4/6 was too sneaky in that it tried to do the "support
unborn branch" thing as well, which should only be handled in this
patch.

I still think the patches should be separate though since they are
conceptually different. 4/6 modifies the "index clean up" function,
while 5/6 modifies the "reset HEAD" function.

Thanks,
Paul