# [PATCH v2] rebase -X: do not clobber strategy 8 messages from 2010-11-10 to 2010-11-12. Participants: Martin von Zweigbergk, Sverre Rabbelier, Thomas Rast, Junio C Hamano. Thread: https://gitlist.dev/t/25698 ## Martin von Zweigbergk, 2010-11-10 07:14 Subject: [PATCH v2] rebase -X: do not clobber strategy Message-ID: <1289373266-32593-1-git-send-email-martin.von.zweigbergk@gmail.com> URL: https://gitlist.dev/e/1289373266-32593-1-git-send-email-martin.von.zweigbergk%40gmail.com ``` If any strategy options are passed to -X, the strategy will always be set to 'recursive'. According to the documentation, it should default to 'recursive' if it is not set, but it should be possible to set it to other values. Signed-off-by: Martin von Zweigbergk --- git-rebase.sh | 4 ---- 1 files changed, 0 insertions(+), 4 deletions(-) diff --git a/git-rebase.sh b/git-rebase.sh index 42c0628..ec08f9c 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -312,10 +312,6 @@ do esac strategy_opts="$strategy_opts $(git rev-parse --sq-quote "--$newopt")" do_merge=t - if test -n "$strategy" - then - strategy=recursive - fi ;; -s=*|--s=*|--st=*|--str=*|--stra=*|--strat=*|--strate=*|\ --strateg=*|--strategy=*|\ -- 1.7.3.2.167.ga361b ``` ## Sverre Rabbelier, 2010-11-10 18:56 Subject: Re: [PATCH v2] rebase -X: do not clobber strategy Message-ID: URL: https://gitlist.dev/e/AANLkTimEvp2mQu1QWhx-FDEx37NLi8jose_p%2BA1y1iHK%40mail.gmail.com In-Reply-To: <1289373266-32593-1-git-send-email-martin.von.zweigbergk@gmail.com> ``` Heya, On Wed, Nov 10, 2010 at 08:14, Martin von Zweigbergk wrote: > If any strategy options are passed to -X, the strategy will always be > set to 'recursive'. According to the documentation, it should default to > 'recursive' if it is not set, but it should be possible to set it to > other values. Repeat of other threat since this is a new patch: can we have a test for this? -- Cheers, Sverre Rabbelier ``` ## Martin von Zweigbergk, 2010-11-11 01:11 Subject: Re: [PATCH v2] rebase -X: do not clobber strategy Message-ID: URL: https://gitlist.dev/e/AANLkTiks87caBYo78Xh1hwnEfDF9yKwBn5q3wa87hBo3%40mail.gmail.com In-Reply-To: ``` On Wed, Nov 10, 2010 at 1:56 PM, Sverre Rabbelier wrote: > On Wed, Nov 10, 2010 at 08:14, Martin von Zweigbergk > wrote: >> If any strategy options are passed to -X, the strategy will always be >> set to 'recursive'. According to the documentation, it should default to >> 'recursive' if it is not set, but it should be possible to set it to >> other values. > > Repeat of other threat since this is a new patch: can we have a test for this? I don't think there are any merge strategies other than recursive that accept options, so what I could add a test case for is that e.g. '-s ours -X foo' uses the 'ours' strategy, even though 'foo' will be ignored. I have very little experience with merge strategies, but I will give it a try. Hopefully there is some existing test case I can copy and modify. Btw, why is the default (if no strategy is specifed) for 'git rebase' to use 'recursive', while for 'git merge' "a built-in list of strategies is used instead (git merge-recursive when merging a single head, git merge-octopus otherwise)"? ``` ## Thomas Rast, 2010-11-11 10:41 Subject: Re: [PATCH v2] rebase -X: do not clobber strategy Message-ID: <201011111141.33623.trast@student.ethz.ch> URL: https://gitlist.dev/e/201011111141.33623.trast%40student.ethz.ch In-Reply-To: ``` Martin von Zweigbergk wrote: > Btw, why is the default (if no strategy is specifed) for 'git rebase' to > use 'recursive', while for 'git merge' "a built-in list of strategies is > used instead (git merge-recursive when merging a single head, git > merge-octopus otherwise)"? Because rebase does a tree-level merge, so it never attempts to merge than one branch, so octopus never enters the picture. Sorry for the original breakage; while it has Mike Lundy assigned as author, I resurrected and resubmitted his patch and should have noticed. -- Thomas Rast trast@{inf,student}.ethz.ch ``` ## Martin von Zweigbergk, 2010-11-11 12:59 Subject: Re: [PATCH v2] rebase -X: do not clobber strategy Message-ID: URL: https://gitlist.dev/e/AANLkTinBhDR2%2BSGtz4rmj8CrOh4yLPAB3A74BV-yBCxa%40mail.gmail.com In-Reply-To: <201011111141.33623.trast@student.ethz.ch> ``` On Thu, Nov 11, 2010 at 5:41 AM, Thomas Rast wrote: > Martin von Zweigbergk wrote: >> Btw, why is the default (if no strategy is specifed) for 'git rebase' to >> use 'recursive', while for 'git merge' "a built-in list of strategies is >> used instead (git merge-recursive when merging a single head, git >> merge-octopus otherwise)"? > > Because rebase does a tree-level merge, so it never attempts to merge > than one branch, so octopus never enters the picture. I can see why octopus doesn't make sense when doing a linearizing rebase, but what if it's merge-preserving rebase? The call to 'git merge' in git-rebase--interactive.sh looks like this: new_parents=${new_parents# $first_parent} [...] git merge $strategy -m "$msg" $new_parents Also, it sounds like "a built-in list" is something that could one day be expanded. So my question is then whether it would make sense to call 'git merge' without specifying a strategy when 'git rebase' is called without a strategy? > Sorry for the original breakage; while it has Mike Lundy assigned as > author, I resurrected and resubmitted his patch and should have > noticed. Well, since there are no other strategies than recursive that support strategy options, it was not really a breakage in reality, at least as far as I can see. Speaking of that and about my earlier comment about writing a test case, what should really happen if the user calls 'git rebase -s ours -X foo'? Should it really be allowed? (I tried it and it does work, though.) ``` ## Junio C Hamano, 2010-11-11 22:01 Subject: Re: [PATCH v2] rebase -X: do not clobber strategy Message-ID: <7vpqubfrkj.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vpqubfrkj.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Martin von Zweigbergk writes: > I don't think there are any merge strategies other than recursive that > accept options, so what I could add a test case for is that e.g. > '-s ours -X foo' uses the 'ours' strategy, even though 'foo' will be > ignored. You would do something like this. -- >8 -- Subject: [PATCH] t3402: test "rebase -s -X" Signed-off-by: Junio C Hamano --- t/t3402-rebase-merge.sh | 21 +++++++++++++++++++++ 1 files changed, 21 insertions(+), 0 deletions(-) diff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh index 2bea656..be8c1d5 100755 --- a/t/t3402-rebase-merge.sh +++ b/t/t3402-rebase-merge.sh @@ -117,4 +117,25 @@ test_expect_success 'picking rebase' ' esac ' +test_expect_success 'rebase -s funny -Xopt' ' + test_when_finished "rm -fr test-bin funny.was.run" && + mkdir test-bin && + cat >test-bin/git-merge-funny <<-EOF && + #!$SHELL_PATH + case "\$1" in --opt) ;; *) exit 2 ;; esac + shift && + >funny.was.run && + exec git merge-recursive "\$@" + EOF + chmod +x test-bin/git-merge-funny && + git reset --hard && + git checkout -b test-funny master^ && + test_commit funny && + ( + PATH=./test-bin:$PATH + git rebase -s funny -Xopt master + ) && + test -f funny.was.run +' + test_done -- 1.7.3.2.334.gd1031 ``` ## Martin von Zweigbergk, 2010-11-12 01:31 Subject: Re: [PATCH v2] rebase -X: do not clobber strategy Message-ID: URL: https://gitlist.dev/e/AANLkTinrhfmeeToMZfW942xvJGz9vgiQd6%2B2bXkSawJU%40mail.gmail.com In-Reply-To: <7vpqubfrkj.fsf@alter.siamese.dyndns.org> ``` On Thu, Nov 11, 2010 at 5:01 PM, Junio C Hamano wrote: > Martin von Zweigbergk writes: > >> I don't think there are any merge strategies other than recursive that >> accept options, so what I could add a test case for is that e.g. >> '-s ours -X foo' uses the 'ours' strategy, even though 'foo' will be >> ignored. > > You would do something like this. > > -- >8 -- > Subject: [PATCH] t3402: test "rebase -s -X" > > Signed-off-by: Junio C Hamano > --- >  t/t3402-rebase-merge.sh |   21 +++++++++++++++++++++ >  1 files changed, 21 insertions(+), 0 deletions(-) > > diff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh > index 2bea656..be8c1d5 100755 > --- a/t/t3402-rebase-merge.sh > +++ b/t/t3402-rebase-merge.sh > @@ -117,4 +117,25 @@ test_expect_success 'picking rebase' ' >        esac >  ' > > +test_expect_success 'rebase -s funny -Xopt' ' > +       test_when_finished "rm -fr test-bin funny.was.run" && > +       mkdir test-bin && > +       cat >test-bin/git-merge-funny <<-EOF && > +       #!$SHELL_PATH > +       case "\$1" in --opt) ;; *) exit 2 ;; esac > +       shift && > +       >funny.was.run && > +       exec git merge-recursive "\$@" > +       EOF > +       chmod +x test-bin/git-merge-funny && > +       git reset --hard && > +       git checkout -b test-funny master^ && > +       test_commit funny && > +       ( > +               PATH=./test-bin:$PATH > +               git rebase -s funny -Xopt master > +       ) && > +       test -f funny.was.run > +' > + >  test_done > -- > 1.7.3.2.334.gd1031 That's clever. Thank you! The fix for this bug touches the same parts of the code as my refactoring of git rebase. If you don't mind, I will post your test case and the fix as part of the refactoring series. ``` ## Junio C Hamano, 2010-11-12 20:55 Subject: Re: [PATCH v2] rebase -X: do not clobber strategy Message-ID: <7vaalefeio.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vaalefeio.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Martin von Zweigbergk writes: > That's clever. Thank you! > > The fix for this bug touches the same parts of the code as my > refactoring of git rebase. If you don't mind, I will post your test > case and the fix as part of the refactoring series. Nah, I'd rather squash the test script into the fix. ```