# [PATCH] rebase --merge: fix for rebasing more than 7 commits. 10 messages from 2006-06-22 to 2006-06-25. Participants: Junio C Hamano, Eric Wong, Johannes Schindelin. Thread: https://gitlist.dev/t/4610 ## Junio C Hamano, 2006-06-22 08:44 Subject: [PATCH] rebase --merge: fix for rebasing more than 7 commits. Message-ID: <7vy7vptw8p.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vy7vptw8p.fsf%40assigned-by-dhcp.cox.net ``` 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 --- * 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 ``` ## Junio C Hamano, 2006-06-22 08:54 Subject: Re: [PATCH] rebase --merge: fix for rebasing more than 7 commits. Message-ID: <7vpsh1tvt1.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vpsh1tvt1.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <7vy7vptw8p.fsf@assigned-by-dhcp.cox.net> ``` Junio C Hamano 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. ``` ## Eric Wong, 2006-06-22 11:09 Subject: Re: [PATCH] rebase --merge: fix for rebasing more than 7 commits. Message-ID: <20060622110941.GA32261@hand.yhbt.net> URL: https://gitlist.dev/e/20060622110941.GA32261%40hand.yhbt.net In-Reply-To: <7vpsh1tvt1.fsf@assigned-by-dhcp.cox.net> ``` Junio C Hamano wrote: > Junio C Hamano 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 ``` ## Junio C Hamano, 2006-06-24 07:09 Subject: Re: [PATCH] rebase --merge: fix for rebasing more than 7 commits. Message-ID: <7vac83114g.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vac83114g.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <20060622110941.GA32261@hand.yhbt.net> ``` Eric Wong writes: > Junio C Hamano wrote: >> Junio C Hamano 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, 2006-06-25 01:29 Subject: [PATCH 0/3] rebase --merge improvements and fixes Message-ID: <11511989892359-git-send-email-normalperson@yhbt.net> URL: https://gitlist.dev/e/11511989892359-git-send-email-normalperson%40yhbt.net In-Reply-To: <20060622110941.GA32261@hand.yhbt.net> ``` 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, 2006-06-25 01:29 Subject: [PATCH 1/3] rebase: allow --merge option to handle patches merged upstream Message-ID: <11511989902239-git-send-email-normalperson@yhbt.net> URL: https://gitlist.dev/e/11511989902239-git-send-email-normalperson%40yhbt.net In-Reply-To: <20060622110941.GA32261@hand.yhbt.net> ``` Enhance t3401-rebase-partial to test with --merge as well as the standard am -3 strategy. Signed-off-by: Eric Wong --- 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 ``` ## Eric Wong, 2006-06-25 01:29 Subject: [PATCH 2/3] rebase: cleanup rebasing with --merge Message-ID: <11511989922423-git-send-email-normalperson@yhbt.net> URL: https://gitlist.dev/e/11511989922423-git-send-email-normalperson%40yhbt.net In-Reply-To: <20060622110941.GA32261@hand.yhbt.net> ``` 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 --- 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 ``` ## Eric Wong, 2006-06-25 01:29 Subject: [PATCH 3/3] rebase: allow --skip to work with --merge Message-ID: <11511989931962-git-send-email-normalperson@yhbt.net> URL: https://gitlist.dev/e/11511989931962-git-send-email-normalperson%40yhbt.net In-Reply-To: <20060622110941.GA32261@hand.yhbt.net> ``` 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 --- 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 ``` ## Johannes Schindelin, 2006-06-25 02:04 Subject: Re: [PATCH 1/3] rebase: allow --merge option to handle patches merged upstream Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0606250401490.29667%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <11511989902239-git-send-email-normalperson@yhbt.net> ``` 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, 2006-06-25 04:59 Subject: Re: [PATCH 1/3] rebase: allow --merge option to handle patches merged upstream Message-ID: <20060625045912.GB17674@hand.yhbt.net> URL: https://gitlist.dev/e/20060625045912.GB17674%40hand.yhbt.net In-Reply-To: ``` Johannes Schindelin 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 ```