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

[Alex Henrie <alexhenrie24@gmail.com>]

On Thu, Mar 23, 2023 at 12:45 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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".

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@dunelm.org.uk/