On Fri, Jan 20, 2023 at 8:46 AM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote: > > Hi Elijah > > Thanks for working on this > > On 20/01/2023 04:56, Elijah Newren via GitGitGadget wrote: > > From: Elijah Newren <newren@xxxxxxxxx> > > > > --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@xxxxxxxxxxxxxx/ [2] See "longer term goal" of https://lore.kernel.org/git/xmqqa78d2qmk.fsf@xxxxxxxxxxxxxxxxxxxxxxxxx/
