Re: [PATCH v8 3/3] rebase: add a config option for --rebase-merges

> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>>> @@ -1514,13 +1542,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>>    				break;
>>>      		if (i >= 0 || options.type == REBASE_APPLY) {
>>> -			if (is_merge(&options))
>>> -				die(_("apply options and merge options "
>>> -					  "cannot be used together"));
>>
>> This isn't a new change but having thought about it I'm not sure
>> moving this code is a good idea. If the user runs
>>
>> 	git -c rebase.updateRefs=true rebase --whitespace=fix --exec "make test"
>>
>> then instead of getting an message saying that they cannot use apply
>> and merge options together they'll get a message suggesting they pass
>> --no-update-refs which wont fix the problem for them.
> 
> Hmph.  Instead of dying here, ...
> 
>>> +			if (options.autosquash == -1 && options.config_autosquash == 1)
>>>    				die(_("apply options are incompatible with rebase.autoSquash.  Consider adding --no-autosquash"));
>>> +			else if (options.rebase_merges == -1 && options.config_rebase_merges == 1)
>>> +				die(_("apply options are incompatible with rebase.rebaseMerges.  Consider adding --no-rebase-merges"));
>>>    			else if (options.update_refs == -1 && options.config_update_refs == 1)
>>>    				die(_("apply options are incompatible with rebase.updateRefs.  Consider adding --no-update-refs"));
> 
> ... we get caught here, and the next one is not triggered.
> 
>>> +			else if (is_merge(&options))
>>> +				die(_("apply options and merge options "
>>> +					  "cannot be used together"));
>>>    			else
>>>    				options.type = REBASE_APPLY;
> 
> What's the reason why "cannot be used together" is moved to the last
> in the chain?
> 
> The first two new conditions in this chain try to catch an
> invocation with some apply-specific command line option
> (e.g. "--whitespace=fix") when used with configuration variables
> specific to the merge-backend (e.g. "rebase.merges") and suggest
> overriding the configuration from the command line, and I suspect
> that the motivation behind this change is that their error messages
> are more specific than the generic "apply and merge do not mix".
> 
> But are these two new errors and hints universally a good suggestion
> to make?  I actually think a command line option forcing us to use
> the apply backend should simply silently ignore (aka "override")
> configuration variables that take effect only when we are using the
> merge-backend.  "git rebase --whitespace=fix --rebase-merges",
> giving both from the command line, is making conflicting and
> unsatisfyable request, and it is worth giving an error message.
> 
> But if you configure rebase.autosquash only because you want to it
> to be in effect for your invocations of "git rebase -i", you
> shouldn't have to override it from the command line when you say
> "git rebase" or "git rebase --whitespace=fix", should you?