Junio C Hamano <gitster@xxxxxxxxx> writes:
> Arnaud Fontaine <arnau@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html