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

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

[Johannes Sixt]

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(-)

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

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

[Johannes Sixt]

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,
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(-)

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

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

[Johannes Sixt]

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(-)

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

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

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

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

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

[Johannes Schindelin]

Hi,

On Mon, 15 Dec 2008, Johannes Sixt wrote:

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

> 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(-)

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

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

[Johannes Schindelin]

Hi,

On Mon, 22 Dec 2008, Johannes Schindelin wrote:

> 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(-)

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