Re: [PATCH v2 2/2] rebase: mark --update-refs as requiring the merge backend

[Elijah Newren <newren@gmail.com>]

On Fri, Jan 20, 2023 at 8:46 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Elijah
>
> Thanks for working on this
>
> On 20/01/2023 04:56, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > --update-refs is built in terms of the sequencer, which requires the
> > merge backend.  It was already marked as incompatible with the apply
> > backend in the git-rebase manual, but the code didn't check for this
> > incompatibility and warn the user.  Check and warn now.
>
> Strictly speaking we die rather than warn but I don't think that
> warrants a re-roll.

Oh, good catch.  I'm re-rolling anyway, so I might as well fix this.

> I just had a quick look to see how easy it would be
> to add the advice Stolee's patch had if the user has set
> rebase.updaterefs but does not pass "--no-update-refs" when using the
> apply backend but it looks a bit fiddly unfortunately as we could die in
> imply_merge() or later on.

Yeah, and it gets even more finicky than that.  If the user specifies
_any_ merge-specific options on the command line together with an
apply-specific option, then there's no point bringing up
rebase.updaterefs (or rebase.autosquash).  We only want to bring up
those config options if they are the only reasons for getting a
backends-are-incompatible error message.

> Thinking more generally, imply_merge() does a good job of telling the
> user which option is incompatible with "--apply" but if the user passes
> a merge option with "--whitespace=fix" and omits "--apply" then we just
> print a generic message saying "apply options and merge options cannot
> be used together" which isn't terribly helpful to the user (doubly so
> when the merge option come from a config setting).

That's not specific to --whitespace=fix (it also happens with -C, and
in the past happened with other options that used to only work with
the apply backend).  In particular, it's whenever both backends are
implied -- in those cases, we don't try to keep track of which options
implied it and thus only provide a very generic error message.

> I've also noticed that "--autosquash" is ignored if we end up using the
> apply backend. That's a separate issue but shares the "this may have
> come from a config setting rather than a command line argument" problem.

Yeah, Stolee also pointed this one out...and --autosquash was missing
the same incompatible-with-apply-options warnings too.

> All in all I'm not sure if it is friendlier to die when the user has
> rebsase.updaterefs set and they try to rebase with "--whitespace=fix" or
> if it is better just to ignore the config in that case. If we can find a
> way to print some help when we die in that case it would be nicer for
> the user.

I think ignoring it would be worse, as I argued over at [1].  But
another thing to keep in mind is that we can eventually make the
question obsolete by deprecating and eventually removing the apply
backend, as suggested by Junio[2].  That would allow us to remove all
the incompatibility checking and simplify the manual.


[1] https://lore.kernel.org/git/CABPp-BHDhpSVpuaubTP=smWaf7FBmpzB-_Frh0Dn5oN+vx0xzw@mail.gmail.com/
[2] See "longer term goal" of
https://lore.kernel.org/git/xmqqa78d2qmk.fsf@gitster-ct.c.googlers.com/