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

[Junio C Hamano <gitster@pobox.com>]

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