Hi Junio
On 23/03/2023 18:45, Junio C Hamano wrote:
Phillip Wood <phillip.wood123@xxxxxxxxx> 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?
I think there are two conflicting viewpoints here which depend on what
one thinks the user wants when they set these configuration variables.
As I understand it Elijah's thinking was that if the user set
rebase.autosquash they'd be surprised when their fixup commits were not
squashed by
git rebase --whitespace=fix
and that's why his patch series changed things to error out.
The other point of view is that the user expects that these
configuration variables to apply only when they are using the "merge"
backend and so we should not error out.
Personally I lean a little more towards the latter but I don't feel that
strongly about it and can see an argument for having either behavior. I
do think we should leave the ordering alone though so the user gets a
useful error message in the case of "git rebase --exec 'make test'
--whitespace=fix"
Best Wishes
Phillip
Thanks.
