On Thu, Mar 23, 2023 at 12:45 PM Junio C Hamano <gitster@xxxxxxxxx> 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".
Phillip specifically asked for `git -c rebase.rebaseMerges=true rebase
--whitespace=fix` to print "error: apply options are incompatible with
rebase.rebaseMerges. Consider adding --no-rebase-merges".[1] I could
have sworn that "if (is_merge(&options))" had to be after "if
(options.rebase_merges == -1 && options.config_rebase_merges == 1)" to
make that happen, but now it works without that change. I must have
been debugging with some intermediate version that still had
'imply_merge(&options, "--rebase-merges")' before this if chain. I'll
send a v9 that puts "if (is_merge(&options))" back at the top.
Thanks for your attention to detail!
-Alex
[1] https://lore.kernel.org/git/7ba8a92d-8c94-5226-5416-6ed3a8e2e389@xxxxxxxxxxxxx/
