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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 21, 2013, 20:43 UTC
Message-ID: <7vr4fvkxew.fsf@alter.siamese.dyndns.org>
In-Reply-To: <87bo70dokb.fsf@duckcorp.org>

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?