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

> Arnaud Fontaine <ar...@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

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

Arnaud Fontaine

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to