threads / patch / 4610

patchrebase --merge: fix for rebasing more than 7 commits.

Subject: [PATCH] rebase --merge: fix for rebasing more than 7 commits.

## tl;dr

10 messages between Jun 22, 2006 and Jun 25, 2006. Diffs are folded; open one to read it.

replies: 9people: 3as markdown or json

Junio C Hamano· Jun 22, 2006, 08:44 UTC · lore

Instead of using 4-digit numbers to name commits being rebased, just use "cmt.$msgnum" string, with $msgnum as a decimal number without leading zero padding. This makes it possible to rebase more than 9999 commits, but of more practical importance is that the earlier code used "printf" to format already formatted $msgnum and barfed when it counted up to 0008. In other words, the old code was incapable of rebasing more than 7 commits, and this fixes that problem.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 * I wanted to raise my confidence level in the new rebase --merge
   code, so I did a little exercise which resulted in finding this
   buglet.
   I did not have "read/write-tree --prefix" on the "master"
   branch, when I received the patch series that begins with
   "Make git-write-tree a builtin" from Lukas.  Now, my policy
   is to avoid starting a new topic based on "next" (because it
   would make later pulling in the topic into "master" without
   all the other uncooked stuff in "next" impossible), but the
   series was based on "next" (which was actually nice, since
   write-tree between "master" and "next" were somewhat diverged
   back then), and I couldn't easily rebase Lukas's patch series
   on top of "master" because we did not have "rebase --merge"
   option.  Now I can with the updated "rebase".
   So the exercise went like this:
   (1) Start from a clone of git.git, with "master" at 8c278ab
       (where the "master" was before "read/write-tree --prefix"
       was merged), and "ls/am" at ab195e1 (what applying
       Lukas's series on top of "next" would have produced).
       Use "reset --hard" as needed.
   (2) "git rebase --merge --onto master ls/am~9" (ls/am~8 being
       the first one of the series).  This fails with a merge
       conflict on builtin-write-tree.c (the "rebase --merge"
       code did the right thing with the renamed path).  Fix it
       up and also fix builtin.h (signature of write_tree()
       needs to be changed to drop prefix).
       It was very helpful to view:
           git diff :3:builtin-write-tree.c builtin-write-tree.c	
       during the hand resolution of the conflicts; it lets me
       see what Lukas did, so I can adjust the patch by removing
       parts that are specific to "next" and not applicable to
       "master".  
  (3) "git update-index builtin-write-tree.c builtin.h" and then
      "git rebase --continue".  The bug manifests itself during
      the finalization step, which this commit fixes.
  With this fix, the above works beautifully.  I am reasonably
  happy with this shiny new toy.  Good job, Eric! and thanks.
 git-rebase.sh |   23 ++++++++++-------------
 1 files changed, 10 insertions(+), 13 deletions(-)
Show changes to git-rebase.sh +10 −13
diff --git a/git-rebase.sh b/git-rebase.sh
index b9ce112..9159477 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -67,16 +67,16 @@ continue_merge () {
 	prev_head=`git-rev-parse HEAD^0`
 
 	# save the resulting commit so we can read-tree on it later
-	echo "$prev_head" > "$dotest/`printf %0${prec}d $msgnum`.result"
+	echo "$prev_head" > "$dotest/cmt.$msgnum.result"
 	echo "$prev_head" > "$dotest/prev_head"
 
 	# onto the next patch:
 	msgnum=$(($msgnum + 1))
-	printf "%0${prec}d" "$msgnum" > "$dotest/msgnum"
+	echo "$msgnum" >"$dotest/msgnum"
 }
 
 call_merge () {
-	cmt="$(cat $dotest/`printf %0${prec}d $1`)"
+	cmt="$(cat $dotest/cmt.$1)"
 	echo "$cmt" > "$dotest/current"
 	git-merge-$strategy "$cmt^" -- HEAD "$cmt"
 	rv=$?
@@ -108,15 +108,12 @@ finish_rb_merge () {
 	end="`cat $dotest/end`"
 	while test "$msgnum" -le "$end"
 	do
-		msgnum=`printf "%0${prec}d" "$msgnum"`
-		printf "%0${prec}d" "$msgnum" > "$dotest/msgnum"
-
-		git-read-tree `cat "$dotest/$msgnum.result"`
+		git-read-tree `cat "$dotest/cmt.$msgnum.result"`
 		git-checkout-index -q -f -u -a
-		git-commit -C "`cat $dotest/$msgnum`"
+		git-commit -C "`cat $dotest/cmt.$msgnum`"
 
-		echo "Committed $msgnum"
-		echo '    '`git-rev-list --pretty=oneline -1 HEAD | \
+		printf "Committed %0${prec}d" $msgnum
+		echo ' '`git-rev-list --pretty=oneline -1 HEAD | \
 					sed 's/^[a-f0-9]\+ //'`
 		msgnum=$(($msgnum + 1))
 	done
@@ -322,11 +319,11 @@ for cmt in `git-rev-list --no-merges "$u
 			| perl -e 'print reverse <>'`
 do
 	msgnum=$(($msgnum + 1))
-	echo "$cmt" > "$dotest/`printf "%0${prec}d" $msgnum`"
+	echo "$cmt" > "$dotest/cmt.$msgnum"
 done
 
-printf "%0${prec}d" 1 > "$dotest/msgnum"
-printf "%0${prec}d" "$msgnum" > "$dotest/end"
+echo 1 >"$dotest/msgnum"
+echo $msgnum >"$dotest/end"
 
 end=$msgnum
 msgnum=1
-- 
1.4.0.gfba6
Junio C Hamano· Jun 22, 2006, 08:54 UTC · re: Junio C Hamano · lore

Re: [PATCH] rebase --merge: fix for rebasing more than 7 commits.

Junio C Hamano <junkio@cox.net> writes:
Show 8 quoted lines
>  * I wanted to raise my confidence level in the new rebase --merge
>    code, so I did a little exercise which resulted in finding this
>    buglet.
>...
>    So the exercise went like this:
>...
>   With this fix, the above works beautifully.  I am reasonably
>   happy with this shiny new toy.  Good job, Eric! and thanks.

By the way, I do not quite understand the reasoning behind not moving the head being rebased until the finalization phase.

Also I think --skip would be straightforward. What you look at in call_merge() is the current HEAD, the commit being rebased and its direct parent (actually what you are interested in are trees of these commits and not ancestry chains among them -- if we can tell git-merge-recursive not to try its own "recursive" merge base finding but just use what we give it as the base, I could sleep better. I think the current code could misbehave in funnier ancestry graph if we allow it to pick merge base on its own), so skipping is just a matter of, eh, skipping the commit.

Eric Wong· Jun 22, 2006, 11:09 UTC · re: Junio C Hamano · lore

Re: [PATCH] rebase --merge: fix for rebasing more than 7 commits.

Junio C Hamano <junkio@cox.net> wrote:
Show 10 quoted lines
> Junio C Hamano <junkio@cox.net> writes:
> 
> >  * I wanted to raise my confidence level in the new rebase --merge
> >    code, so I did a little exercise which resulted in finding this
> >    buglet.
> >...
> >    So the exercise went like this:
> >...
> >   With this fix, the above works beautifully.  I am reasonably
> >   happy with this shiny new toy.  Good job, Eric! and thanks.
:)  Thanks for the extra QA and fix.
> By the way, I do not quite understand the reasoning behind not
> moving the head being rebased until the finalization phase.

That's because my original patch that only used git-merge, which didn't let me manually commit with all the information from a previous commit.

Show 9 quoted lines
> Also I think --skip would be straightforward.  What you look at
> in call_merge() is the current HEAD, the commit being rebased
> and its direct parent (actually what you are interested in are
> trees of these commits and not ancestry chains among them -- if
> we can tell git-merge-recursive not to try its own "recursive"
> merge base finding but just use what we give it as the base, I
> could sleep better.  I think the current code could misbehave in
> funnier ancestry graph if we allow it to pick merge base on its
> own), so skipping is just a matter of, eh, skipping the commit.

Another consequence of relying on plain git-merge in my original patch. --skip should be very doable now that we can specify the correct base. I'll look into it more when I'm more awake.

-- 
Eric Wong
Junio C Hamano· Jun 24, 2006, 07:09 UTC · re: Eric Wong · lore

Re: [PATCH] rebase --merge: fix for rebasing more than 7 commits.

Eric Wong <normalperson@yhbt.net> writes:
Show 13 quoted lines
> Junio C Hamano <junkio@cox.net> wrote:
>> Junio C Hamano <junkio@cox.net> writes:
>> 
>> >  * I wanted to raise my confidence level in the new rebase --merge
>> >    code, so I did a little exercise which resulted in finding this
>> >    buglet.
>> >...
>> >    So the exercise went like this:
>> >...
>> >   With this fix, the above works beautifully.  I am reasonably
>> >   happy with this shiny new toy.  Good job, Eric! and thanks.
>
> :)  Thanks for the extra QA and fix.

Another thing I noticed is while rebasing onto the mainline that has accepted a few of the patches from the topic. The original rebase with "git am -3" logic notices that the patch has already been applied and drops that commit, which is rather nice, but the new "rebase --merge" logic barfs when git-commit notices there is nothing to commit. I think you could before calling git-commit check if the git-merge-$strategy gave you the tree identical to the HEAD tree, and simply skip it (maybe after giving the user "patch already applied"message).

Eric Wong· Jun 25, 2006, 01:29 UTC · re: Eric Wong · lore

[PATCH 0/3] rebase --merge improvements and fixes

These patches should address the current issues with rebase --merge usage. It can now do everything the original format-patch | am -3 strategy including --skip and detection of merged commits by upstream.

-- 
Eric Wong
Eric Wong· Jun 25, 2006, 01:29 UTC · re: Eric Wong · lore

[PATCH 1/3] rebase: allow --merge option to handle patches merged upstream

Enhance t3401-rebase-partial to test with --merge as well as the standard am -3 strategy.

Signed-off-by: Eric Wong <normalperson@yhbt.net>
---
 git-rebase.sh             |   15 +++++++++++----
 t/t3401-rebase-partial.sh |   13 ++++++++++++-
 2 files changed, 23 insertions(+), 5 deletions(-)
Show changes to 2 files +23 −5

git-rebase.sh, t/t3401-rebase-partial.sh

diff --git a/git-rebase.sh b/git-rebase.sh
index 9159477..53fb14e 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -82,7 +82,10 @@ call_merge () {
 	rv=$?
 	case "$rv" in
 	0)
-		git-commit -C "$cmt" || die "commit failed: $MRESOLVEMSG"
+		if test -n "`git-diff-index HEAD`"
+		then
+			git-commit -C "$cmt" || die "commit failed: $MRESOLVEMSG"
+		fi
 		;;
 	1)
 		test -d "$GIT_DIR/rr-cache" && git-rerere
@@ -110,9 +113,13 @@ finish_rb_merge () {
 	do
 		git-read-tree `cat "$dotest/cmt.$msgnum.result"`
 		git-checkout-index -q -f -u -a
-		git-commit -C "`cat $dotest/cmt.$msgnum`"
-
-		printf "Committed %0${prec}d" $msgnum
+		if test -n "`git-diff-index HEAD`"
+		then
+			git-commit -C "`cat $dotest/cmt.$msgnum`"
+			printf "Committed %0${prec}d" $msgnum
+		else
+			printf "Already applied: %0${prec}d" $msgnum
+		fi
 		echo ' '`git-rev-list --pretty=oneline -1 HEAD | \
 					sed 's/^[a-f0-9]\+ //'`
 		msgnum=$(($msgnum + 1))
diff --git a/t/t3401-rebase-partial.sh b/t/t3401-rebase-partial.sh
index 32dc9c5..360a670 100755
--- a/t/t3401-rebase-partial.sh
+++ b/t/t3401-rebase-partial.sh
@@ -37,7 +37,9 @@ test_expect_success \
 test_expect_success \
     'pick top patch from topic branch into master' \
     'git-cherry-pick my-topic-branch^0 &&
-     git-checkout -f my-topic-branch
+     git-checkout -f my-topic-branch &&
+     git-branch master-merge master &&
+     git-branch my-topic-branch-merge my-topic-branch
 '
 
 test_debug \
@@ -50,4 +52,13 @@ test_expect_success \
     'rebase topic branch against new master and check git-am did not get halted' \
     'git-rebase master && test ! -d .dotest'
 
+if test -z "$no_python"
+then
+    test_expect_success \
+	'rebase --merge topic branch that was partially merged upstream' \
+	'git-checkout -f my-topic-branch-merge &&
+	 git-rebase --merge master-merge &&
+	 test ! -d .git/.dotest-merge'
+fi
+
 test_done
-- 
1.4.0.g937a
Johannes Schindelin· Jun 25, 2006, 02:04 UTC · re: Eric Wong · lore

Re: [PATCH 1/3] rebase: allow --merge option to handle patches merged upstream

Hi,
On Sat, 24 Jun 2006, Eric Wong wrote:
> +		if test -n "`git-diff-index HEAD`"

This is not a sufficient test if the patch was already merged to upstream. For example, you can have two patches which touched the same file, and one of them was applied to upstream, the other not. The test fails to see that. Or am I missing something?

Ciao, Dscho

Eric Wong· Jun 25, 2006, 04:59 UTC · re: Johannes Schindelin · lore

Re: [PATCH 1/3] rebase: allow --merge option to handle patches merged upstream

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
Show 10 quoted lines
> Hi,
> 
> On Sat, 24 Jun 2006, Eric Wong wrote:
> 
> > +		if test -n "`git-diff-index HEAD`"
> 
> This is not a sufficient test if the patch was already merged to upstream. 
> For example, you can have two patches which touched the same file, and one 
> of them was applied to upstream, the other not. The test fails to see 
> that. Or am I missing something?

This is just to tell if there's anything worth committing after a merge is complete. If not, then it assumes it's been a) merged upstream or b) empty in the first place (very unlikely).

-- 
Eric Wong
Eric Wong· Jun 25, 2006, 01:29 UTC · re: Eric Wong · lore

[PATCH 2/3] rebase: cleanup rebasing with --merge

We no longer have to recommit each patch to remove the parent information we're rebasing since we're using the low-level merge strategies directly instead of git-merge.

Signed-off-by: Eric Wong <normalperson@yhbt.net>
---
 git-rebase.sh |   33 +++++----------------------------
 1 files changed, 5 insertions(+), 28 deletions(-)
Show changes to git-rebase.sh +5 −28
diff --git a/git-rebase.sh b/git-rebase.sh
index 53fb14e..a95ada6 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -59,15 +59,16 @@ continue_merge () {
 
 	if test -n "`git-diff-index HEAD`"
 	then
+		printf "Committed: %0${prec}d" $msgnum
 		git-commit -C "`cat $dotest/current`"
 	else
-		echo "Previous merge succeeded automatically"
+		printf "Already applied: %0${prec}d" $msgnum
 	fi
+	echo ' '`git-rev-list --pretty=oneline -1 HEAD | \
+				sed 's/^[a-f0-9]\+ //'`
 
 	prev_head=`git-rev-parse HEAD^0`
-
 	# save the resulting commit so we can read-tree on it later
-	echo "$prev_head" > "$dotest/cmt.$msgnum.result"
 	echo "$prev_head" > "$dotest/prev_head"
 
 	# onto the next patch:
@@ -82,10 +83,7 @@ call_merge () {
 	rv=$?
 	case "$rv" in
 	0)
-		if test -n "`git-diff-index HEAD`"
-		then
-			git-commit -C "$cmt" || die "commit failed: $MRESOLVEMSG"
-		fi
+		return
 		;;
 	1)
 		test -d "$GIT_DIR/rr-cache" && git-rerere
@@ -103,27 +101,6 @@ call_merge () {
 }
 
 finish_rb_merge () {
-	set -e
-
-	msgnum=1
-	echo "Finalizing rebased commits..."
-	git-reset --hard "`cat $dotest/onto`"
-	end="`cat $dotest/end`"
-	while test "$msgnum" -le "$end"
-	do
-		git-read-tree `cat "$dotest/cmt.$msgnum.result"`
-		git-checkout-index -q -f -u -a
-		if test -n "`git-diff-index HEAD`"
-		then
-			git-commit -C "`cat $dotest/cmt.$msgnum`"
-			printf "Committed %0${prec}d" $msgnum
-		else
-			printf "Already applied: %0${prec}d" $msgnum
-		fi
-		echo ' '`git-rev-list --pretty=oneline -1 HEAD | \
-					sed 's/^[a-f0-9]\+ //'`
-		msgnum=$(($msgnum + 1))
-	done
 	rm -r "$dotest"
 	echo "All done."
 }
-- 
1.4.0.g937a
Eric Wong· Jun 25, 2006, 01:29 UTC · re: Eric Wong · lore

[PATCH 3/3] rebase: allow --skip to work with --merge

Now that we control the merge base selection, we won't be forced into rolling things in that we wanted to skip beforehand.

Also, add a test to ensure this all works as intended.
Signed-off-by: Eric Wong <normalperson@yhbt.net>
---
 Documentation/git-rebase.txt |    1 -
 git-rebase.sh                |   13 ++++++++-
 t/t3403-rebase-skip.sh       |   61 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 73 insertions(+), 2 deletions(-)
Show changes to 3 files +73 −2

Documentation/git-rebase.txt, git-rebase.sh, t/t3403-rebase-skip.sh

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index c339c45..9d7bcaa 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -108,7 +108,6 @@ OPTIONS
 
 --skip::
 	Restart the rebasing process by skipping the current patch.
-	This does not work with the --merge option.
 
 --merge::
 	Use merging strategies to rebase.  When the recursive (default) merge
diff --git a/git-rebase.sh b/git-rebase.sh
index a95ada6..9ad1c44 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -137,7 +137,18 @@ do
 	--skip)
 		if test -d "$dotest"
 		then
-			die "--skip is not supported when using --merge"
+			prev_head="`cat $dotest/prev_head`"
+			end="`cat $dotest/end`"
+			msgnum="`cat $dotest/msgnum`"
+			msgnum=$(($msgnum + 1))
+			onto="`cat $dotest/onto`"
+			while test "$msgnum" -le "$end"
+			do
+				call_merge "$msgnum"
+				continue_merge
+			done
+			finish_rb_merge
+			exit
 		fi
 		git am -3 --skip --resolvemsg="$RESOLVEMSG"
 		exit
diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
new file mode 100755
index 0000000..8ab63c5
--- /dev/null
+++ b/t/t3403-rebase-skip.sh
@@ -0,0 +1,61 @@
+#!/bin/sh
+#
+# Copyright (c) 2006 Eric Wong
+#
+
+test_description='git rebase --merge --skip tests'
+
+. ./test-lib.sh
+
+# we assume the default git-am -3 --skip strategy is tested independently
+# and always works :)
+
+if test "$no_python"; then
+	echo "Skipping: no python => no recursive merge"
+	test_done
+	exit 0
+fi
+
+test_expect_success setup '
+	echo hello > hello &&
+	git add hello &&
+	git commit -m "hello" &&
+	git branch skip-reference &&
+
+	echo world >> hello &&
+	git commit -a -m "hello world" &&
+	echo goodbye >> hello &&
+	git commit -a -m "goodbye" &&
+
+	git checkout -f skip-reference &&
+	echo moo > hello &&
+	git commit -a -m "we should skip this" &&
+	echo moo > cow &&
+	git add cow &&
+	git commit -m "this should not be skipped" &&
+	git branch pre-rebase skip-reference &&
+	git branch skip-merge skip-reference
+	'
+
+test_expect_failure 'rebase with git am -3 (default)' 'git rebase master'
+
+test_expect_success 'rebase --skip with am -3' '
+	git reset --hard HEAD &&
+	git rebase --skip
+	'
+test_expect_success 'checkout skip-merge' 'git checkout -f skip-merge'
+
+test_expect_failure 'rebase with --merge' 'git rebase --merge master'
+
+test_expect_success 'rebase --skip with --merge' '
+	git reset --hard HEAD &&
+	git rebase --skip
+	'
+
+test_expect_success 'merge and reference trees equal' \
+	'test -z "`git-diff-tree skip-merge skip-reference`"'
+
+test_debug 'gitk --all & sleep 1'
+
+test_done
+
-- 
1.4.0.g937a

← back to recent threads