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. 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 <newren@xxxxxxxxx>
---
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
