From: Phillip Wood Date: Fri, 20 Jan 2023 16:46:04 GMT Subject: Re: [PATCH v2 2/2] rebase: mark --update-refs as requiring the merge backend Message-ID: In-Reply-To: <2e44d0b7e571cfac2a25d00f3fe3d143c895793b.1674190573.git.gitgitgadget@gmail.com> Hi Elijah Thanks for working on this On 20/01/2023 04:56, Elijah Newren via GitGitGadget wrote: > From: Elijah Newren > > --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. 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. 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). 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. 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. Best Wishes Phillip > While at it, fix a typo in t3422...and fix some misleading wording (all > useful options other than --whitespace=fix have long since been > implemented in the merge backend). > > Signed-off-by: Elijah Newren > --- > builtin/rebase.c | 3 +++ > t/t3422-rebase-incompatible-options.sh | 15 ++++++++++----- > 2 files changed, 13 insertions(+), 5 deletions(-) > > diff --git a/builtin/rebase.c b/builtin/rebase.c > index ace8bd4a41c..e8bcdbf9fcd 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -1507,6 +1507,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > } > } > > + if (options.update_refs) > + imply_merge(&options, "--update-refs"); > + > if (options.type == REBASE_UNSPECIFIED) { > if (!strcmp(options.default_backend, "merge")) > imply_merge(&options, "--merge"); > diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh > index ebcbd79ab8a..d72c863b21b 100755 > --- a/t/t3422-rebase-incompatible-options.sh > +++ b/t/t3422-rebase-incompatible-options.sh > @@ -25,11 +25,11 @@ test_expect_success 'setup' ' > ' > > # > -# Rebase has lots of useful options like --whitepsace=fix, which are > -# actually all built in terms of flags to git-am. Since neither > -# --merge nor --interactive (nor any options that imply those two) use > -# git-am, using them together will result in flags like --whitespace=fix > -# being ignored. Make sure rebase warns the user and aborts instead. > +# Rebase has a useful option, --whitespace=fix, which is actually > +# built in terms of flags to git-am. Since neither --merge nor > +# --interactive (nor any options that imply those two) use git-am, > +# using them together will result in --whitespace=fix being ignored. > +# Make sure rebase warns the user and aborts instead. > # > > test_rebase_am_only () { > @@ -60,6 +60,11 @@ test_rebase_am_only () { > test_must_fail git rebase $opt --exec 'true' A > " > > + test_expect_success "$opt incompatible with --update-refs" " > + git checkout B^0 && > + test_must_fail git rebase $opt --update-refs A > + " > + > } > > test_rebase_am_only --whitespace=fix