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

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

[Junio C Hamano]

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

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

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

[Junio C Hamano]

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.

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.

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

[Eric Wong]

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.

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

> 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

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

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

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

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

[Eric Wong]

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

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

[Eric Wong]

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

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

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

[Eric Wong]

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

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

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

[Eric Wong]

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

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

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

[Johannes Schindelin]

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

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

[Eric Wong]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> 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