threads / patch / 34237

patchDo not ignore merge options in interactive rebase

Subject: [PATCH] Do not ignore merge options in interactive rebase

## tl;dr

9 messages between Jun 21, 2013 and Jul 2, 2013. Diffs are folded; open one to read it.

replies: 8people: 2as markdown or json

Arnaud Fontaine· Jun 21, 2013, 05:23 UTC · lore

Merge strategy and its options can be specified in `git rebase`, but with `--interactive`, they were completely ignored.

Signed-off-by: Arnaud Fontaine <arnau@debian.org>
---
 git-rebase--interactive.sh    | 11 ++++++++++-
 t/t3404-rebase-interactive.sh | 11 +++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)
 mode change 100644 => 100755 git-rebase--interactive.sh
Show changes to 2 files +21 −1

git-rebase--interactive.sh, t/t3404-rebase-interactive.sh

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
old mode 100644
new mode 100755
index f953d8d..c157fdf
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -239,7 +239,16 @@ pick_one () {
 
 	test -d "$rewritten" &&
 		pick_one_preserving_merges "$@" && return
-	output git cherry-pick $empty_args $ff "$@"
+
+	if test -n "$do_merge"
+	then
+		test -z "$strategy" && strategy=recursive
+		output git cherry-pick --strategy=$strategy \
+			$(echo $strategy_opts | sed "s/'--\([^']*\)'/-X\1/g") \
+			$empty_args $ff "$@"
+	else
+		output git cherry-pick $empty_args $ff "$@"
+	fi
 }
 
 pick_one_preserving_merges () {
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 79e8d3c..8b6a36f 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -947,4 +947,15 @@ test_expect_success 'rebase -i respects core.commentchar' '
 	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
+test_expect_success 'rebase -i with --strategy and -X' '
+	git checkout -b conflict-merge-use-theirs conflict-branch &&
+	git reset --hard HEAD^ &&
+	echo five >conflict &&
+	echo Z >file1 &&
+	git commit -a -m "one file conflict" &&
+	EDITOR=true git rebase -i --strategy=recursive -Xours conflict-branch &&
+	test $(git show conflict-branch:conflict) = $(cat conflict) &&
+	test $(cat file1) = Z
+'
+
 test_done

-- 
1.8.3.GIT
Junio C Hamano· Jun 21, 2013, 20:43 UTC · re: Arnaud Fontaine · lore

Re: [PATCH] Do not ignore merge options in interactive rebase

Arnaud Fontaine <arnau@debian.org> writes:
> Merge strategy and its options can be specified in `git rebase`,
> but with `--interactive`, they were completely ignored.

And why is it a bad thing? If you meant s/--interactive/-m/ in the above, then I can sort of understand the justification, though.

> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> old mode 100644
> new mode 100755
I see an unjustifiable mode change here.
Show 11 quoted lines
> index f953d8d..c157fdf
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -239,7 +239,16 @@ pick_one () {
>  
>  	test -d "$rewritten" &&
>  		pick_one_preserving_merges "$@" && return
> -	output git cherry-pick $empty_args $ff "$@"
> +
> +	if test -n "$do_merge"
> +	then
So you _did_ mean "rebase -m"?
> +		test -z "$strategy" && strategy=recursive
> +		output git cherry-pick --strategy=$strategy \
This is a bad change.
I would understand if the above were:
	git cherry-pick ${strategy+--strategy=$strategy} ...

in other words, "if there is no strategy specified, do not override the configured default that might be different from recursive" (pull.twohead may be set to resolve).

> +			$(echo $strategy_opts | sed "s/'--\([^']*\)'/-X\1/g") \
Is it guaranteed $startegy_opts do not have a space in it?

There is a call to "git merge" that uses "${strategy+-s $strategy}", but it does not seem to propagate the strategy option. Does it need a similar change? It seems that the first step might be to factor out these calls to the "git cherry-pick" and "git merge" to helper functions to make it easier to call them with -s/-X options in a consistent way.

> +			$empty_args $ff "$@"
> +	else
> +		output git cherry-pick $empty_args $ff "$@"
> +	fi

It seems that there is another call to "git cherry-pick" in the script ("git grep" for it). Does it need a similar change?

Arnaud Fontaine· Jun 24, 2013, 07:40 UTC · re: Junio C Hamano · lore

Re: [PATCH] Do not ignore merge options in interactive rebase

Junio C Hamano <gitster@pobox.com> writes:
Show 7 quoted lines
> Arnaud Fontaine <arnau@debian.org> writes:
>
>> Merge strategy and its options can  be specified in `git rebase`, but
>> with `--interactive`, they were completely ignored.
>
> And why  is it a bad  thing?  If you meant  s/--interactive/-m/ in the
> above, then I can sort of understand the justification, though.

Sorry, it was not clear. I meant that you can do 'rebase -m --strategy recursive'. But with 'rebase --interactive -m --strategy recursive', '-m --strategy recursive' is ignored. To me, this is not consistent because the behavior is different in interactive mode... Personally, I needed to specify the strategy and its options while using interactive mode and it seems I'm not the only one[0].

Show 5 quoted lines
>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>> old mode 100644
>> new mode 100755
>
> I see an unjustifiable mode change here.
Sorry about that, I fixed it.
Show 13 quoted lines
>> index f953d8d..c157fdf
>> --- a/git-rebase--interactive.sh
>> +++ b/git-rebase--interactive.sh
>> @@ -239,7 +239,16 @@ pick_one () {
>>  
>>  	test -d "$rewritten" &&
>>  		pick_one_preserving_merges "$@" && return
>> -	output git cherry-pick $empty_args $ff "$@"
>> +
>> +	if test -n "$do_merge"
>> +	then
>
> So you _did_ mean "rebase -m"?

I really meant 'rebase --interactive -m'. do_merge is set to true if either '--strategy' or '-m' or '-X' is given according to git-rebase.sh.

Show 12 quoted lines
>> +		test -z "$strategy" && strategy=recursive
>> +		output git cherry-pick --strategy=$strategy \
>
> This is a bad change.
>
> I would understand if the above were:
>
> 	git cherry-pick ${strategy+--strategy=$strategy} ...
>
> in other  words, "if there is  no strategy specified, do  not override
> the  configured  default  that  might  be  different  from  recursive"
> (pull.twohead may be set to resolve).

Indeed, I did not know about that. I wrongly thought it was a good idea to do the same as both git-rebase (when -X is given) and git-rebase--merge which do the same test ('test -z "$strategy" && strategy=recursive'). However after checking more carefully, I guess that, for the former case, it is because only recursive currently takes options, whereas, for the latter case, it is to call a default git-rebase-$strategy.

>> +			$(echo $strategy_opts | sed "s/'--\([^']*\)'/-X\1/g") \
>
> Is it guaranteed $startegy_opts do not have a space in it?

strategy_opts may be something like (git-rebase.sh): "'--foo' '--bar'", but I'm not sure what is wrong if there is a space in it though.

Show 6 quoted lines
> There is a  call to "git merge" that  uses "${strategy+-s $strategy}",
> but it does not seem to propagate the strategy option.  Does it need a
> similar change?  It  seems that the first step might  be to factor out
> these  calls  to the  "git  cherry-pick"  and  "git merge"  to  helper
> functions  to make  it easier  to call  them with  -s/-X options  in a
> consistent way.

As far as I understand, yes. I changed it. As it is really short, I just added an if/else inside the script itself, not sure if that's ok...

Show 7 quoted lines
>> +			$empty_args $ff "$@"
>> +	else
>> +		output git cherry-pick $empty_args $ff "$@"
>> +	fi
>
> It seems that there is another call to "git cherry-pick" in the script
> ("git grep" for it).  Does it need a similar change?
As far as I understand, yes. So I changed it as well.

I have sent the fixed patch in my next email. Many thanks for the review!

Cheers,
-- 
Arnaud Fontaine

[0] http://git.661346.n2.nabble.com/git-rebase-interactive-does-not-respect-merge-options-td7500093.html
Arnaud Fontaine· Jun 24, 2013, 07:47 UTC · re: Arnaud Fontaine · lore

Fix inconsistency where `--strategy` and/or `--strategy-option` can be specified in git rebase, but with `--interactive` argument only there were completely ignored.

Signed-off-by: Arnaud Fontaine <arnau@debian.org>
---
 git-rebase--interactive.sh    | 13 ++++++++++---
 t/t3404-rebase-interactive.sh | 11 +++++++++++
 2 files changed, 21 insertions(+), 3 deletions(-)
Show changes to 2 files +21 −3

git-rebase--interactive.sh, t/t3404-rebase-interactive.sh

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f953d8d..e558397 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -80,6 +80,13 @@ amend="$state_dir"/amend
 rewritten_list="$state_dir"/rewritten-list
 rewritten_pending="$state_dir"/rewritten-pending
 
+strategy_args=
+if test -n "$do_merge"
+then
+	strategy_args="${strategy+--strategy=$strategy}
+ $(echo $strategy_opts | sed "s/'--\([^']*\)'/-X\1/g")"
+fi
+
 GIT_CHERRY_PICK_HELP="$resolvemsg"
 export GIT_CHERRY_PICK_HELP
 
@@ -239,7 +246,7 @@ pick_one () {
 
 	test -d "$rewritten" &&
 		pick_one_preserving_merges "$@" && return
-	output git cherry-pick $empty_args $ff "$@"
+	output git cherry-pick $strategy_args $empty_args $ff "$@"
 }
 
 pick_one_preserving_merges () {
@@ -341,7 +348,7 @@ pick_one_preserving_merges () {
 			# No point in merging the first parent, that's HEAD
 			new_parents=${new_parents# $first_parent}
 			if ! do_with_author output \
-				git merge --no-ff ${strategy:+-s $strategy} -m \
+				git merge --no-ff $strategy_args -m \
 					"$msg_content" $new_parents
 			then
 				printf "%s\n" "$msg_content" > "$GIT_DIR"/MERGE_MSG
@@ -350,7 +357,7 @@ pick_one_preserving_merges () {
 			echo "$sha1 $(git rev-parse HEAD^0)" >> "$rewritten_list"
 			;;
 		*)
-			output git cherry-pick "$@" ||
+			output git cherry-pick $strategy_args "$@" ||
 				die_with_patch $sha1 "Could not pick $sha1"
 			;;
 		esac
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 79e8d3c..8b6a36f 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -947,4 +947,15 @@ test_expect_success 'rebase -i respects core.commentchar' '
 	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
+test_expect_success 'rebase -i with --strategy and -X' '
+	git checkout -b conflict-merge-use-theirs conflict-branch &&
+	git reset --hard HEAD^ &&
+	echo five >conflict &&
+	echo Z >file1 &&
+	git commit -a -m "one file conflict" &&
+	EDITOR=true git rebase -i --strategy=recursive -Xours conflict-branch &&
+	test $(git show conflict-branch:conflict) = $(cat conflict) &&
+	test $(cat file1) = Z
+'
+
 test_done

-- 
1.8.3.GIT
Junio C Hamano· Jun 25, 2013, 17:04 UTC · re: Arnaud Fontaine · lore

Re: [PATCH] Do not ignore merge options in interactive rebase

Arnaud Fontaine <arnau@debian.org> writes:
Show 24 quoted lines
> Fix inconsistency where `--strategy` and/or `--strategy-option` can be
> specified in git rebase, but with `--interactive` argument only there
> were completely ignored.
>
> Signed-off-by: Arnaud Fontaine <arnau@debian.org>
> ---
>  git-rebase--interactive.sh    | 13 ++++++++++---
>  t/t3404-rebase-interactive.sh | 11 +++++++++++
>  2 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index f953d8d..e558397 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -80,6 +80,13 @@ amend="$state_dir"/amend
>  rewritten_list="$state_dir"/rewritten-list
>  rewritten_pending="$state_dir"/rewritten-pending
>  
> +strategy_args=
> +if test -n "$do_merge"
> +then
> +	strategy_args="${strategy+--strategy=$strategy}
> + $(echo $strategy_opts | sed "s/'--\([^']*\)'/-X\1/g")"
> +fi

The "${var+if_set_use_this}" interpolates if $var is set (as opposed to "unset"); this will cause it to have "--strategy= " in strategy_args when $do_merge is set. If you ran t3421, you would have noticed breakage.

You should use ${var:+if_set_to_nonempty_use_this} here.

stragety_opts is designed to be a valid shell scriptlet that can be inserted as a part of eval'ed string. git-rebase.sh does this:

	-X)
		shift
		strategy_opts="$strategy_opts $(git rev-parse --sq-quote "--$1")"
		do_merge=t
so that git-rebase--merge.sh can use it like so:
	eval 'git-merge-$strategy' $strategy_opts '"$cmt^" -- "$hd" "$cmt"'

without having to worry about quoting issues when future strategy options have letters that need quoting, e.g.

	$ git rebase -X foo="bar ' baz"

git rev-parse --sq-quote turns it into '--foo=bar '\'' baz' and then in the eval'ed string, it becomes a single argument given to the git-merge-$strategy command, even though it may have spaces and single quotes and other characters that may be tricky to quote manually without mistakes.

So munging it manually with sloppy "sed" script is simply wrong.
Show 50 quoted lines
>  GIT_CHERRY_PICK_HELP="$resolvemsg"
>  export GIT_CHERRY_PICK_HELP
>  
> @@ -239,7 +246,7 @@ pick_one () {
>  
>  	test -d "$rewritten" &&
>  		pick_one_preserving_merges "$@" && return
> -	output git cherry-pick $empty_args $ff "$@"
> +	output git cherry-pick $strategy_args $empty_args $ff "$@"
>  }
>  
>  pick_one_preserving_merges () {
> @@ -341,7 +348,7 @@ pick_one_preserving_merges () {
>  			# No point in merging the first parent, that's HEAD
>  			new_parents=${new_parents# $first_parent}
>  			if ! do_with_author output \
> -				git merge --no-ff ${strategy:+-s $strategy} -m \
> +				git merge --no-ff $strategy_args -m \
>  					"$msg_content" $new_parents
>  			then
>  				printf "%s\n" "$msg_content" > "$GIT_DIR"/MERGE_MSG
> @@ -350,7 +357,7 @@ pick_one_preserving_merges () {
>  			echo "$sha1 $(git rev-parse HEAD^0)" >> "$rewritten_list"
>  			;;
>  		*)
> -			output git cherry-pick "$@" ||
> +			output git cherry-pick $strategy_args "$@" ||
>  				die_with_patch $sha1 "Could not pick $sha1"
>  			;;
>  		esac
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 79e8d3c..8b6a36f 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -947,4 +947,15 @@ test_expect_success 'rebase -i respects core.commentchar' '
>  	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
>  '
>  
> +test_expect_success 'rebase -i with --strategy and -X' '
> +	git checkout -b conflict-merge-use-theirs conflict-branch &&
> +	git reset --hard HEAD^ &&
> +	echo five >conflict &&
> +	echo Z >file1 &&
> +	git commit -a -m "one file conflict" &&
> +	EDITOR=true git rebase -i --strategy=recursive -Xours conflict-branch &&
> +	test $(git show conflict-branch:conflict) = $(cat conflict) &&
> +	test $(cat file1) = Z
> +'
> +
>  test_done
Junio C Hamano· Jun 25, 2013, 20:28 UTC · re: Junio C Hamano · lore

Re: [PATCH] Do not ignore merge options in interactive rebase

Junio C Hamano <gitster@pobox.com> writes:
> You should use ${var:+if_set_to_nonempty_use_this} here.
> ...
> So munging it manually with sloppy "sed" script is simply wrong.
Perhaps something like this on top of your patch squashed in?

The eval magic lets the shell to split $strategy_opts back into individual words (e.g. "--subtree=My Project" is a single word), strip the leading "--", and then uses "rev-parse --sq-quote" again to turn them into "-X 'subtree=My Project'" (two words), which can be inserted into a string later to be eval'ed.

 git-rebase--interactive.sh | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)
Show changes to git-rebase--interactive.sh +11 −7
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index e558397..ae2da7a 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -83,8 +83,13 @@ rewritten_pending="$state_dir"/rewritten-pending
 strategy_args=
 if test -n "$do_merge"
 then
-	strategy_args="${strategy+--strategy=$strategy}
- $(echo $strategy_opts | sed "s/'--\([^']*\)'/-X\1/g")"
+	strategy_args=${strategy:+--strategy=$strategy}
+	eval '
+		for strategy_opt in '"$strategy_opts"'
+		do
+			strategy_args="$strategy_args -X$(git rev-parse --sq-quote "${strategy_opt#--}")"
+		done
+	'
 fi
 
 GIT_CHERRY_PICK_HELP="$resolvemsg"
@@ -246,7 +251,7 @@ pick_one () {
 
 	test -d "$rewritten" &&
 		pick_one_preserving_merges "$@" && return
-	output git cherry-pick $strategy_args $empty_args $ff "$@"
+	output eval git cherry-pick "$strategy_args" $empty_args $ff "$@"
 }
 
 pick_one_preserving_merges () {
@@ -347,9 +352,8 @@ pick_one_preserving_merges () {
 			msg_content="$(commit_message $sha1)"
 			# No point in merging the first parent, that's HEAD
 			new_parents=${new_parents# $first_parent}
-			if ! do_with_author output \
-				git merge --no-ff $strategy_args -m \
-					"$msg_content" $new_parents
+			if ! do_with_author output eval \
+			'git merge --no-ff $strategy_args -m "$msg_content" $new_parents'
 			then
 				printf "%s\n" "$msg_content" > "$GIT_DIR"/MERGE_MSG
 				die_with_patch $sha1 "Error redoing merge $sha1"
@@ -357,7 +361,7 @@ pick_one_preserving_merges () {
 			echo "$sha1 $(git rev-parse HEAD^0)" >> "$rewritten_list"
 			;;
 		*)
-			output git cherry-pick $strategy_args "$@" ||
+			output eval git cherry-pick "$strategy_args" "$@" ||
 				die_with_patch $sha1 "Could not pick $sha1"
 			;;
 		esac
Arnaud Fontaine· Jul 2, 2013, 08:05 UTC · re: Junio C Hamano · lore

Re: [PATCH] Do not ignore merge options in interactive rebase

Hello,
Sorry for the late reply.
Junio C Hamano <gitster@pobox.com> writes:
Show 67 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> You should use ${var:+if_set_to_nonempty_use_this} here.
>> ...
>> So munging it manually with sloppy "sed" script is simply wrong.
>
> Perhaps something like this on top of your patch squashed in?
>
> The  eval magic  lets  the  shell to  split  $strategy_opts back  into
> individual words (e.g. "--subtree=My Project" is a single word), strip
> the leading "--",  and then uses "rev-parse --sq-quote"  again to turn
> them into "-X 'subtree=My Project'" (two words), which can be inserted
> into a string later to be eval'ed.
>
>  git-rebase--interactive.sh | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index e558397..ae2da7a 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -83,8 +83,13 @@ rewritten_pending="$state_dir"/rewritten-pending
>  strategy_args=
>  if test -n "$do_merge"
>  then
> -	strategy_args="${strategy+--strategy=$strategy}
> - $(echo $strategy_opts | sed "s/'--\([^']*\)'/-X\1/g")"
> +	strategy_args=${strategy:+--strategy=$strategy}
> +	eval '
> +		for strategy_opt in '"$strategy_opts"'
> +		do
> +			strategy_args="$strategy_args -X$(git rev-parse --sq-quote "${strategy_opt#--}")"
> +		done
> +	'
>  fi
>  
>  GIT_CHERRY_PICK_HELP="$resolvemsg"
> @@ -246,7 +251,7 @@ pick_one () {
>  
>  	test -d "$rewritten" &&
>  		pick_one_preserving_merges "$@" && return
> -	output git cherry-pick $strategy_args $empty_args $ff "$@"
> +	output eval git cherry-pick "$strategy_args" $empty_args $ff "$@"
>  }
>  
>  pick_one_preserving_merges () {
> @@ -347,9 +352,8 @@ pick_one_preserving_merges () {
>  			msg_content="$(commit_message $sha1)"
>  			# No point in merging the first parent, that's HEAD
>  			new_parents=${new_parents# $first_parent}
> -			if ! do_with_author output \
> -				git merge --no-ff $strategy_args -m \
> -					"$msg_content" $new_parents
> +			if ! do_with_author output eval \
> +			'git merge --no-ff $strategy_args -m "$msg_content" $new_parents'
>  			then
>  				printf "%s\n" "$msg_content" > "$GIT_DIR"/MERGE_MSG
>  				die_with_patch $sha1 "Error redoing merge $sha1"
> @@ -357,7 +361,7 @@ pick_one_preserving_merges () {
>  			echo "$sha1 $(git rev-parse HEAD^0)" >> "$rewritten_list"
>  			;;
>  		*)
> -			output git cherry-pick $strategy_args "$@" ||
> +			output eval git cherry-pick "$strategy_args" "$@" ||
>  				die_with_patch $sha1 "Could not pick $sha1"
>  			;;
>  		esac

Indeed, this is much better this way. Thank you very much! I have just squashed your patch into mine and ran successfully t3404 and t3421 tests after rebasing my branch on top of current master.

I will send the final patch in another email. Of course, feel free to take authorship of the patch or add yourself as Signed-off-by.

Cheers,
-- 
Arnaud Fontaine
Arnaud Fontaine· Jul 2, 2013, 08:05 UTC · re: Junio C Hamano · lore

Merge strategy and its options can be specified in `git rebase`, but with `--interactive`, they were completely ignored.

Signed-off-by: Arnaud Fontaine <arnau@debian.org>
---
 git-rebase--interactive.sh    | 21 ++++++++++++++++-----
 t/t3404-rebase-interactive.sh | 11 +++++++++++
 2 files changed, 27 insertions(+), 5 deletions(-)
Show changes to 2 files +27 −5

git-rebase--interactive.sh, t/t3404-rebase-interactive.sh

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 169e876..157690b 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -80,6 +80,18 @@ amend="$state_dir"/amend
 rewritten_list="$state_dir"/rewritten-list
 rewritten_pending="$state_dir"/rewritten-pending
 
+strategy_args=
+if test -n "$do_merge"
+then
+	strategy_args=${strategy:+--strategy=$strategy}
+	eval '
+		for strategy_opt in '"$strategy_opts"'
+		do
+			strategy_args="$strategy_args -X$(git rev-parse --sq-quote "${strategy_opt#--}")"
+		done
+	'
+fi
+
 GIT_CHERRY_PICK_HELP="$resolvemsg"
 export GIT_CHERRY_PICK_HELP
 
@@ -239,7 +251,7 @@ pick_one () {
 
 	test -d "$rewritten" &&
 		pick_one_preserving_merges "$@" && return
-	output git cherry-pick $empty_args $ff "$@"
+	output eval git cherry-pick "$strategy_args" $empty_args $ff "$@"
 }
 
 pick_one_preserving_merges () {
@@ -340,9 +352,8 @@ pick_one_preserving_merges () {
 			msg_content="$(commit_message $sha1)"
 			# No point in merging the first parent, that's HEAD
 			new_parents=${new_parents# $first_parent}
-			if ! do_with_author output \
-				git merge --no-ff ${strategy:+-s $strategy} -m \
-					"$msg_content" $new_parents
+			if ! do_with_author output eval \
+			'git merge --no-ff $strategy_args -m "$msg_content" $new_parents'
 			then
 				printf "%s\n" "$msg_content" > "$GIT_DIR"/MERGE_MSG
 				die_with_patch $sha1 "Error redoing merge $sha1"
@@ -350,7 +361,7 @@ pick_one_preserving_merges () {
 			echo "$sha1 $(git rev-parse HEAD^0)" >> "$rewritten_list"
 			;;
 		*)
-			output git cherry-pick "$@" ||
+			output eval git cherry-pick "$strategy_args" "$@" ||
 				die_with_patch $sha1 "Could not pick $sha1"
 			;;
 		esac
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index d6b4143..8a6ec03 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -950,4 +950,15 @@ test_expect_success 'rebase -i, with <onto> and <upstream> specified as :/quuxer
 	git checkout branch1
 '
 
+test_expect_success 'rebase -i with --strategy and -X' '
+	git checkout -b conflict-merge-use-theirs conflict-branch &&
+	git reset --hard HEAD^ &&
+	echo five >conflict &&
+	echo Z >file1 &&
+	git commit -a -m "one file conflict" &&
+	EDITOR=true git rebase -i --strategy=recursive -Xours conflict-branch &&
+	test $(git show conflict-branch:conflict) = $(cat conflict) &&
+	test $(cat file1) = Z
+'
+
 test_done

-- 
1.8.3.GIT
Junio C Hamano· Jun 24, 2013, 14:16 UTC · re: Arnaud Fontaine · lore

Re: [PATCH] Do not ignore merge options in interactive rebase

Arnaud Fontaine <arnau@debian.org> writes:
Show 6 quoted lines
>>> +			$(echo $strategy_opts | sed "s/'--\([^']*\)'/-X\1/g") \
>>
>> Is it guaranteed $startegy_opts do not have a space in it?
>
> strategy_opts may be something like (git-rebase.sh): "'--foo' '--bar'",
> but I'm not sure what is wrong if there is a space in it though.

I was primarily worried about "'--frotz=nitfol xyzzy'", where you need to pass -X='frotz=nitfol xyzzy' so that 'xyzzy' part does not become a separate argument directly given to 'git merge' and friends.

And adding '' around \1 is not sufficient, because the value given to the "--frotz" may have to be "nitfol 'n xyzzy".

A comment next to that sed script that says "Currently there is no
strategy option that needs quoting" (if it is the case; I didn't
check), together with a guard to protect us against unexpected
strategy-opts, might be a workable band-aid, though.

← back to recent threads