threads / patch / 25698

v2rebase -X: do not clobber strategy

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

## tl;dr

8 messages between Nov 10, 2010 and Nov 12, 2010. Diffs are folded; open one to read it.

replies: 7people: 4as markdown or json

Martin von Zweigbergk· Nov 10, 2010, 07:14 UTC · lore

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(-)
Show changes to git-rebase.sh +0 −4
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· Nov 10, 2010, 18:56 UTC · re: Martin von Zweigbergk · lore

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

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
Martin von Zweigbergk· Nov 11, 2010, 01:11 UTC · re: Sverre Rabbelier · lore

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

On Wed, Nov 10, 2010 at 1:56 PM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
Show 8 quoted lines
> 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)"?

Thomas Rast· Nov 11, 2010, 10:41 UTC · re: Martin von Zweigbergk · lore

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

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· Nov 11, 2010, 12:59 UTC · re: Thomas Rast · lore

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

On Thu, Nov 11, 2010 at 5:41 AM, Thomas Rast <trast@student.ethz.ch> wrote:
Show 8 quoted lines
> 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· Nov 11, 2010, 22:01 UTC · re: Martin von Zweigbergk · lore

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

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(-)
Show changes to t/t3402-rebase-merge.sh +21 −0
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· Nov 12, 2010, 01:31 UTC · re: Junio C Hamano · lore

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

On Thu, Nov 11, 2010 at 5:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 49 quoted lines
> 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.

Junio C Hamano· Nov 12, 2010, 20:55 UTC · re: Martin von Zweigbergk · lore

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

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
Show 5 quoted lines
> 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.

← back to recent threads