threads / patch / 16685

patchShow a failure of rebase -p if the merge had a conflict

Subject: [PATCH] Show a failure of rebase -p if the merge had a conflict

## tl;dr

6 messages between Dec 11, 2008 and Dec 22, 2008. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Johannes Sixt· Dec 11, 2008, 16:21 UTC · lore

This extends t3409-rebase-preserve-merges by a case where the merge that is rebased has a conflict. Therefore, the rebase stops and expects that the user resolves the conflict. However, currently rebase --continue fails because .git/rebase-merge/author-script is missing.

The test script had allocated two identical clones, but only one of them (clone2) was used. Now we use both as indicated in the comment. Also, an instance of && was missing in the setup part.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
  BTW, I'm not 100% sure whether the additional tests of what to expect
  from the test if it did not fail are correct.

  I am unable to fix the failure.

  -- Hannes

  [Sorry, Junio, for the resend. git send-email & PEBCAK. :-/ ]

 t/t3409-rebase-preserve-merges.sh |   43 ++++++++++++++++++++++++++++++++----
 1 files changed, 38 insertions(+), 5 deletions(-)

Show changes to t/t3409-rebase-preserve-merges.sh +38 −5

diff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh
index 8cde40f..02a6401 100755
--- a/t/t3409-rebase-preserve-merges.sh
+++ b/t/t3409-rebase-preserve-merges.sh
@@ -11,7 +11,7 @@ Run "git rebase -p" and check that merges are properly carried along
 GIT_AUTHOR_EMAIL=bogus_email_address
 export GIT_AUTHOR_EMAIL
 
-#echo 'Setting up:
+#Clone 1 (trivial merge):
 #
 #A1--A2  <-- origin/master
 # \   \
@@ -19,7 +19,15 @@ export GIT_AUTHOR_EMAIL
 #   \
 #    B2  <-- origin/topic
 #
-#'
+#Clone 2 (conflicting merge):
+#
+#A1--A2--B3   <-- origin/master
+# \       \
+#  B1------M  <-- topic
+#   \
+#    B2       <-- origin/topic
+#
+# In both cases, 'topic' is rebased onto 'origin/topic'.
 
 test_expect_success 'setup for merge-preserving rebase' \
 	'echo First > A &&
@@ -37,12 +45,19 @@ test_expect_success 'setup for merge-preserving rebase' \
 	cd clone1 &&
 	git checkout -b topic origin/topic &&
 	git merge origin/master &&
-	cd ..
+	cd .. &&
+
+	echo Fifth > B &&
+	git add B &&
+	git commit -m "Add different B" &&
 
 	git clone ./. clone2
 	cd clone2 &&
 	git checkout -b topic origin/topic &&
-	git merge origin/master &&
+	test_must_fail git merge origin/master &&
+	echo Resolved > B &&
+	git add B &&
+	git commit -m "Merge origin/master into topic" &&
 	cd .. &&
 
 	git checkout topic &&
@@ -51,11 +66,29 @@ test_expect_success 'setup for merge-preserving rebase' \
 '
 
 test_expect_success 'rebase -p fakes interactive rebase' '
-	cd clone2 &&
+	(
+	cd clone1 &&
 	git fetch &&
 	git rebase -p origin/topic &&
 	test 1 = $(git rev-list --all --pretty=oneline | grep "Modify A" | wc -l) &&
 	test 1 = $(git rev-list --all --pretty=oneline | grep "Merge commit" | wc -l)
+	)
+'
+
+test_expect_failure '--continue works after a conflict' '
+	(
+	cd clone2 &&
+	git fetch &&
+	test_must_fail git rebase -p origin/topic &&
+	test 2 = $(git ls-files B | wc -l) &&
+	echo Resolved again > B &&
+	test_must_fail git rebase --continue &&
+	git add B &&
+	git rebase --continue &&
+	test 1 = $(git rev-list --all --pretty=oneline | grep "Modify A" | wc -l) &&
+	test 1 = $(git rev-list --all --pretty=oneline | grep "Add different" | wc -l) &&
+	test 1 = $(git rev-list --all --pretty=oneline | grep "Merge origin" | wc -l)
+	)
 '
 
 test_done

-- 
1.6.1.rc2.22.gf3bf84

Johannes Sixt· Dec 15, 2008, 10:05 UTC · re: Johannes Sixt · lore

[PATCH 1/2 updated] Show a failure of rebase -p if the merge had a conflict

The test script had allocated two identical clones, but only one of them (clone2) was used. Now we use both as indicated in the comment. Also, two instances of && was missing in the setup part.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 This is a resend of the earlier patch with a minor update: I missed one
 instance of && in the setup part, so there were actually two missing.

 -- Hannes

 t/t3409-rebase-preserve-merges.sh |   55 +++++++++++++++++++++++++++++-------
 1 files changed, 44 insertions(+), 11 deletions(-)

Show changes to t/t3409-rebase-preserve-merges.sh +44 −11

diff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh
index 8cde40f..5ddd1d1 100755
--- a/t/t3409-rebase-preserve-merges.sh
+++ b/t/t3409-rebase-preserve-merges.sh
@@ -11,15 +11,23 @@ Run "git rebase -p" and check that merges are properly carried along
 GIT_AUTHOR_EMAIL=bogus_email_address
 export GIT_AUTHOR_EMAIL
 
-#echo 'Setting up:
+# Clone 1 (trivial merge):
 #
-#A1--A2  <-- origin/master
-# \   \
-#  B1--M  <-- topic
-#   \
-#    B2  <-- origin/topic
+# A1--A2  <-- origin/master
+#  \   \
+#   B1--M  <-- topic
+#    \
+#     B2  <-- origin/topic
 #
-#'
+# Clone 2 (conflicting merge):
+#
+# A1--A2--B3   <-- origin/master
+#  \       \
+#   B1------M  <-- topic
+#    \
+#     B2       <-- origin/topic
+#
+# In both cases, 'topic' is rebased onto 'origin/topic'.
 
 test_expect_success 'setup for merge-preserving rebase' \
 	'echo First > A &&
@@ -37,12 +45,19 @@ test_expect_success 'setup for merge-preserving rebase' \
 	cd clone1 &&
 	git checkout -b topic origin/topic &&
 	git merge origin/master &&
-	cd ..
+	cd .. &&
+
+	echo Fifth > B &&
+	git add B &&
+	git commit -m "Add different B" &&
 
-	git clone ./. clone2
+	git clone ./. clone2 &&
 	cd clone2 &&
 	git checkout -b topic origin/topic &&
-	git merge origin/master &&
+	test_must_fail git merge origin/master &&
+	echo Resolved > B &&
+	git add B &&
+	git commit -m "Merge origin/master into topic" &&
 	cd .. &&
 
 	git checkout topic &&
@@ -51,11 +66,29 @@ test_expect_success 'setup for merge-preserving rebase' \
 '
 
 test_expect_success 'rebase -p fakes interactive rebase' '
-	cd clone2 &&
+	(
+	cd clone1 &&
 	git fetch &&
 	git rebase -p origin/topic &&
 	test 1 = $(git rev-list --all --pretty=oneline | grep "Modify A" | wc -l) &&
 	test 1 = $(git rev-list --all --pretty=oneline | grep "Merge commit" | wc -l)
+	)
+'
+
+test_expect_failure '--continue works after a conflict' '
+	(
+	cd clone2 &&
+	git fetch &&
+	test_must_fail git rebase -p origin/topic &&
+	test 2 = $(git ls-files B | wc -l) &&
+	echo Resolved again > B &&
+	test_must_fail git rebase --continue &&
+	git add B &&
+	git rebase --continue &&
+	test 1 = $(git rev-list --all --pretty=oneline | grep "Modify A" | wc -l) &&
+	test 1 = $(git rev-list --all --pretty=oneline | grep "Add different" | wc -l) &&
+	test 1 = $(git rev-list --all --pretty=oneline | grep "Merge origin" | wc -l)
+	)
 '
 
 test_done

-- 
1.6.1.rc2.22.gf3bf84

Johannes Sixt· Dec 15, 2008, 10:05 UTC · re: Johannes Sixt · lore

[PATCH 2/2] rebase -i -p: Fix --continue after a merge could not be redone

When a merge that has a conflict was rebased, then rebase stopped to let the user resolve the conflicts. However, thereafter --continue failed because the author-script was not saved. (This is rebase -i's way to preserve a commit's authorship.) This fixes it by doing taking the same failure route after a merge that is also taken after a normal cherry-pick.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 This is an attempt at fixing the failure.  I don't know whether it is
 problematic to leave a "patch" behind if there was actually a merge.
 Nevertheless, all rebase tests pass.

 -- Hannes
 
 git-rebase--interactive.sh        |    3 +--
 t/t3409-rebase-preserve-merges.sh |    2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

Show changes to 2 files +2 −3

git-rebase--interactive.sh, t/t3409-rebase-preserve-merges.sh

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 1172e47..89c39eb 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -253,15 +253,14 @@ pick_one_preserving_merges () {
 			if ! GIT_AUTHOR_NAME="$GIT_AUTHOR_NAME" \
 				GIT_AUTHOR_EMAIL="$GIT_AUTHOR_EMAIL" \
 				GIT_AUTHOR_DATE="$GIT_AUTHOR_DATE" \
 				output git merge $STRATEGY -m "$msg" \
 					$new_parents
 			then
-				git rerere
 				printf "%s\n" "$msg" > "$GIT_DIR"/MERGE_MSG
-				die Error redoing merge $sha1
+				die_with_patch $sha1 "Error redoing merge $sha1"
 			fi
 			;;
 		*)
 			output git cherry-pick "$@" ||
 				die_with_patch $sha1 "Could not pick $sha1"
 			;;
diff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh
index 5ddd1d1..820e010 100755
--- a/t/t3409-rebase-preserve-merges.sh
+++ b/t/t3409-rebase-preserve-merges.sh
@@ -72,13 +72,13 @@ test_expect_success 'rebase -p fakes interactive rebase' '
 	git rebase -p origin/topic &&
 	test 1 = $(git rev-list --all --pretty=oneline | grep "Modify A" | wc -l) &&
 	test 1 = $(git rev-list --all --pretty=oneline | grep "Merge commit" | wc -l)
 	)
 '
 
-test_expect_failure '--continue works after a conflict' '
+test_expect_success '--continue works after a conflict' '
 	(
 	cd clone2 &&
 	git fetch &&
 	test_must_fail git rebase -p origin/topic &&
 	test 2 = $(git ls-files B | wc -l) &&
 	echo Resolved again > B &&

-- 
1.6.1.rc2.22.gf3bf84

Junio C Hamano· Dec 15, 2008, 10:16 UTC · re: Johannes Sixt · lore

Re: [PATCH 2/2] rebase -i -p: Fix --continue after a merge could not be redone

Johannes Sixt <j6t@kdbg.org> writes:

Show 11 quoted lines

> When a merge that has a conflict was rebased, then rebase stopped to let
> the user resolve the conflicts. However, thereafter --continue failed
> because the author-script was not saved. (This is rebase -i's way to
> preserve a commit's authorship.) This fixes it by doing taking the same
> failure route after a merge that is also taken after a normal cherry-pick.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>  This is an attempt at fixing the failure.  I don't know whether it is
>  problematic to leave a "patch" behind if there was actually a merge.
>  Nevertheless, all rebase tests pass.

Interesting approach.  Dscho?

Johannes Schindelin· Dec 22, 2008, 21:06 UTC · re: Johannes Sixt · lore

Re: [PATCH 2/2] rebase -i -p: Fix --continue after a merge could not be redone

Hi,

On Mon, 15 Dec 2008, Johannes Sixt wrote:

Show 10 quoted lines

> When a merge that has a conflict was rebased, then rebase stopped to let
> the user resolve the conflicts. However, thereafter --continue failed
> because the author-script was not saved. (This is rebase -i's way to
> preserve a commit's authorship.) This fixes it by doing taking the same
> failure route after a merge that is also taken after a normal cherry-pick.
> 
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>  This is an attempt at fixing the failure.  I don't know whether it is
>  problematic to leave a "patch" behind if there was actually a merge.

I'd suggest leaving behind a --cc patch, so that nobody is tempted to "fix" the patch until it applies.

Show 12 quoted lines

> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 1172e47..89c39eb 100755
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -253,15 +253,14 @@ pick_one_preserving_merges () {
>  			if ! GIT_AUTHOR_NAME="$GIT_AUTHOR_NAME" \
>  				GIT_AUTHOR_EMAIL="$GIT_AUTHOR_EMAIL" \
>  				GIT_AUTHOR_DATE="$GIT_AUTHOR_DATE" \
>  				output git merge $STRATEGY -m "$msg" \
>  					$new_parents
>  			then
> -				git rerere

Why?  I found rerere pretty helpful when rebasing merges.

Ah, it is done by die_with_patch() already. You might want to prevent headscratching and hunting around for a few minutes, by simply mentioning that fact in the commit message.

I'll leave the commit message fixing to you, but the --cc patch probably looks something like this -- feel free to squash in:

---

 git-rebase--interactive.sh |   18 +++++++++++++++---
 1 files changed, 15 insertions(+), 3 deletions(-)

Show changes to git-rebase--interactive.sh +15 −3

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 89c39eb..b20c101 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -115,9 +115,21 @@ mark_action_done () {
 }
 
 make_patch () {
-	parent_sha1=$(git rev-parse --verify "$1"^) ||
-		die "Cannot get patch for $1^"
-	git diff-tree -p "$parent_sha1".."$1" > "$DOTEST"/patch
+	sha1_and_parents="$(git rev-list --parents -1 $1)"
+	case "$sha1_and_parents" in
+	*' '*' '*' '*)
+		echo "Octopus merge"
+	;;
+	*' '*' '*)
+		git diff --cc $sha1_and_parents
+	;;
+	*' '*)
+		git diff-tree -R -p $sha1_and_parents
+	;;
+	*)
+		echo "Root commit"
+	;;
+	esac > "$DOTEST"/patch
 	test -f "$DOTEST"/message ||
 		git cat-file commit "$1" | sed "1,/^$/d" > "$DOTEST"/message
 	test -f "$DOTEST"/author-script ||

Johannes Schindelin· Dec 22, 2008, 21:16 UTC · re: Johannes Schindelin · lore

Re: [PATCH 2/2] rebase -i -p: Fix --continue after a merge could not be redone

Hi,

On Mon, 22 Dec 2008, Johannes Schindelin wrote:

Show 5 quoted lines

> I'll leave the commit message fixing to you, but the --cc patch probably 
> looks something like this -- feel free to squash in:
> 
> ---
> [... a patch ...]

Ooops. Scratch that. I tested without running the test suite, and sure enough, it failed. Take that instead.

--

 git-rebase--interactive.sh |   18 +++++++++++++++---
 1 files changed, 15 insertions(+), 3 deletions(-)

Show changes to git-rebase--interactive.sh +15 −3

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 89c39eb..f08409c 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -115,9 +115,21 @@ mark_action_done () {
 }
 
 make_patch () {
-	parent_sha1=$(git rev-parse --verify "$1"^) ||
-		die "Cannot get patch for $1^"
-	git diff-tree -p "$parent_sha1".."$1" > "$DOTEST"/patch
+	sha1_and_parents="$(git rev-list --parents -1 $1)"
+	case "$sha1_and_parents" in
+	*' '*' '*' '*)
+		echo "Octopus merge"
+	;;
+	*' '*' '*)
+		git diff --cc $sha1_and_parents
+	;;
+	*' '*)
+		git diff-tree -p $1^!
+	;;
+	*)
+		echo "Root commit"
+	;;
+	esac > "$DOTEST"/patch
 	test -f "$DOTEST"/message ||
 		git cat-file commit "$1" | sed "1,/^$/d" > "$DOTEST"/message
 	test -f "$DOTEST"/author-script ||