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

From: Arnaud Fontaine <arnau@debian.org>
Date: Jun 24, 2013, 07:40 UTC
Message-ID: <87bo6wyn0z.fsf@duckcorp.org>
In-Reply-To: <7vr4fvkxew.fsf@alter.siamese.dyndns.org>

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