[PATCH v2] rebase -X: do not clobber strategy

[PATCH v2] rebase -X: do not clobber strategy

[Martin von Zweigbergk]

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 <martin.von.zweigbergk@gmail.com>
---
 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

Re: [PATCH v2] rebase -X: do not clobber strategy

[Sverre Rabbelier]

Heya,

On Wed, Nov 10, 2010 at 08:14, Martin von Zweigbergk
<martin.von.zweigbergk@gmail.com> 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

Re: [PATCH v2] rebase -X: do not clobber strategy

[Martin von Zweigbergk]

On Wed, Nov 10, 2010 at 1:56 PM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> On Wed, Nov 10, 2010 at 08:14, Martin von Zweigbergk
> <martin.von.zweigbergk@gmail.com> 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)"?

Re: [PATCH v2] rebase -X: do not clobber strategy

[Thomas Rast]

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

Re: [PATCH v2] rebase -X: do not clobber strategy

[Martin von Zweigbergk]

On Thu, Nov 11, 2010 at 5:41 AM, Thomas Rast <trast@student.ethz.ch> 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.)

Re: [PATCH v2] rebase -X: do not clobber strategy

[Junio C Hamano]

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> 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<strategy> -X<opt>"

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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

Re: [PATCH v2] rebase -X: do not clobber strategy

[Martin von Zweigbergk]

On Thu, Nov 11, 2010 at 5:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> 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<strategy> -X<opt>"
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  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.

Re: [PATCH v2] rebase -X: do not clobber strategy

[Junio C Hamano]

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> 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.