[PATCH] Do not ignore merge options in interactive rebase

[PATCH] Do not ignore merge options in interactive rebase

[Arnaud Fontaine]

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

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

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

[Junio C Hamano]

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.

> 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?

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

[Arnaud Fontaine]

Junio C Hamano <gitster@pobox.com> writes:

> 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].

>> 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.

>> 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.

>> +		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.

> 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...

>> +			$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

[PATCH] Do not ignore merge options in interactive rebase

[Arnaud Fontaine]

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
+
 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

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

[Junio C Hamano]

Arnaud Fontaine <arnau@debian.org> writes:

>>> +			$(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.
	

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

[Junio C Hamano]

Arnaud Fontaine <arnau@debian.org> writes:

> 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.

>  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

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

[Junio C Hamano]

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

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

[Arnaud Fontaine]

Hello,

Sorry for the late reply.

Junio C Hamano <gitster@pobox.com> writes:

> 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

[PATCH] Do not ignore merge options in interactive rebase

[Arnaud Fontaine]

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(-)

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