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

[Arnaud Fontaine <arnau@debian.org>]

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