rebase: mark --update-refs as requiring the merge backend

From: Elijah Newren <[email protected]>

--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.

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).

---
rebase: mark --update-refs as requiring the merge backend

--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.

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).

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1466%2Fnewren%2Frebase-update-refs-imply-merge-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1466/newren/rebase-update-refs-imply-merge-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1466

 builtin/rebase.c                       |  3 +++
 t/t3422-rebase-incompatible-options.sh | 15 ++++++++++-----
 2 files changed, 13 insertions(+), 5 deletions(-)

index 1481c5b6a5b..accd62fce48 100644
@@ -1514,6 +1514,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");

index 6dabb05a2ad..5a00618d265 100755
@@ -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

base-commit: 2b4f5a4e4bb102ac8d967cea653ed753b608193c

gitgitgadget

Thank you for submitting this version.

Ooh, that does sound like a useful future idea to improve things further.

Sorry if it was confusing. I was trying to figure out how to word it to remove the old confusion, and actually spent a while thinking about this point you brought up...but after a while gave up and just submitted my patch because it was taking too long and I didn't think the -C option was worth as much brain power as has been spent on it. But since you noted the incongruity, let me explain my thought process a little...

* I stated that --whitespace=... was the last *useful* option, not that it was the last option. Yes, that was an implicit assertion that -C is not useful, though I avoided stating it directly for fear I'd have to dig up proof and/or spend a bunch of time trying to reproduce an old bug and debug it.

* The correct way to port the '-C' option to the merge backend would probably be to just accept the flag and ignore it. The merge backend already functions with entire files or essentially infinite context. I can't think of any case where ignoring this option would result in negative surprises, other than possibly confusing people about how the algorithm behaves and making them decide to tweak it to no avail. And even if users were to waste time twiddling that flag in combination with the merge backend, even that might be a correct "port" of the existing -C option because...

* I once tried to use the '-C' option with the apply backend to see if it could workaround a bug inherent to the apply backend (where people have a source file with several very similar code blocks, and rebase or am applying changes to the wrong one due to fuzz factor and large deletions/insertions having happened upstream elsewhere in the file). It appeared to have absolutely no effect. I suspected it was buggy, but didn't feel the option was worth debugging (I thought switching the default rebase backend was a much better use of time).

* We have *zero* tests of the functionality of rebase's -C option in the testsuite. The only testing we do for rebase's -C option is solely around option parsing.

....

And you inspired me to do some digging. rebase -C was introduced with 67dad687ad ("add -C[NUM] to git-am", 2007-02-08). Based on feedback on the patch, the author added the -C option not just to git-am but also to git-rebase. However, the way it was added to rebase was to just pass it through to git-am, with no corresponding option to format-patch. Therefore, format-patch is going to continue generating patches with 3 lines of context, while we tell git-am/git-apply to pay attention to more than 3 lines of context. The -C<num> option is documented as using all available context if <num> exceeds the number of lines of context provided in the input patches. So, the -C option to rebase has never done anything useful, and the port from the legacy rebase script to the builtin C did not accidentally introduce any modicum of utility to this option; rather, it faithfully ported this pointless option precisely as-is.

So, I'll adjust this series to include a preliminary patch that rips the rebase -C option out.

--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.

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).

Changes since v1:

* Add a patch nuking the -C option to rebase (fixes confusion around the comment in t3422 and acknowledges the fact that the option is totally and utterly useless and always has been. It literally never affects the results of a rebase.)

Elijah Newren (2):
rebase: remove completely useless -C option
rebase: mark --update-refs as requiring the merge backend

 Documentation/git-rebase.txt           |  9 ---------
 builtin/rebase.c                       | 12 ++++--------
 t/t3406-rebase-message.sh              |  7 -------
 t/t3422-rebase-incompatible-options.sh | 16 ++++++++++------
 4 files changed, 14 insertions(+), 30 deletions(-)

base-commit: 2b4f5a4e4bb102ac8d967cea653ed753b608193c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1466%2Fnewren%2Frebase-update-refs-imply-merge-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1466/newren/rebase-update-refs-imply-merge-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1466

Range-diff vs v1:

 -:  ----------- > 1:  a0f8f5fac1c rebase: remove completely useless -C option
 1:  f7459f0996b = 2:  2e44d0b7e57 rebase: mark --update-refs as requiring the merge backend

gitgitgadget

From: Elijah Newren <[email protected]>

The -C option to rebase was introduced with 67dad687ad ("add -C[NUM] to git-am", 2007-02-08).  Based on feedback on the patch, the author added the -C option not just to git-am but also to git-rebase.  But did it such that the option was just passed through to git-am (which passes it along to git-apply), with no corresponding option to format-patch.

As per the git-apply documentation for the -C option:
Ensure at least <n> lines of surrounding context match...When fewer lines of surrounding context exist they all must match.

The fact that format-patch was not passed a -U option to increase the number of context lines meant that there would still only be 3 lines of context to match on. So, anyone attempting to use this option in git-rebase would just be spinning their wheels and wasting time. I was one such user a number of years ago.

Since this option can at best waste users time and is nothing more than a confusing no-op, and has never been anything other than a confusing no-op, and no one has ever bothered to create a testcase for it that goes beyond option parsing, simply excise the option.

---
 Documentation/git-rebase.txt           | 9 ---------
 builtin/rebase.c                       | 9 +--------
 t/t3406-rebase-message.sh              | 7 -------
 t/t3422-rebase-incompatible-options.sh | 1 -
 4 files changed, 1 insertion(+), 25 deletions(-)

index f9675bd24e6..412887deda7 100644
@@ -416,14 +416,6 @@ include::rerere-options.txt[]
 	Allows the pre-rebase hook to run, which is the default.  This option can
 	be used to override `--no-verify`.  See also linkgit:githooks[5].
 
--C<n>::
-	Ensure at least `<n>` lines of surrounding context match before
-	and after each change.  When fewer lines of surrounding
-	context exist they all must match.  By default no context is
-	ever ignored.  Implies `--apply`.
-+
-See also INCOMPATIBLE OPTIONS below.
-
 --no-ff::
 --force-rebase::
 -f::

@@ -631,7 +623,6 @@ The following options:
 
  * --apply
  * --whitespace
- * -C
 
 are incompatible with the following options:
 

index 1481c5b6a5b..ace8bd4a41c 100644
@@ -1067,8 +1067,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			 N_("ignore author date and use current date")),
 		OPT_HIDDEN_BOOL(0, "ignore-date", &options.ignore_date,
 				N_("synonym of --reset-author-date")),
-		OPT_PASSTHRU_ARGV('C', NULL, &options.git_am_opts, N_("n"),
-				  N_("passed to 'git apply'"), 0),
 		OPT_BOOL(0, "ignore-whitespace", &ignore_whitespace,
 			 N_("ignore changes in whitespace")),
 		OPT_PASSTHRU_ARGV(0, "whitespace", &options.git_am_opts,

@@ -1390,12 +1388,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		if (!strcmp(option, "--whitespace=fix") ||
 		    !strcmp(option, "--whitespace=strip"))
 			allow_preemptive_ff = 0;
-		else if (skip_prefix(option, "-C", &p)) {
-			while (*p)
-				if (!isdigit(*(p++)))
-					die(_("switch `C' expects a "
-					      "numerical value"));
-		} else if (skip_prefix(option, "--whitespace=", &p)) {
+		else if (skip_prefix(option, "--whitespace=", &p)) {
 			if (*p && strcmp(p, "warn") && strcmp(p, "nowarn") &&
 			    strcmp(p, "error") && strcmp(p, "error-all"))
 				die("Invalid whitespace option: '%s'", p);

index ceca1600053..c8dca845dd7 100755
@@ -78,13 +78,6 @@ test_expect_success 'rebase --onto outputs the invalid ref' '
 	test_i18ngrep "invalid-ref" err
 '
 
-test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' '
-	test_must_fail git rebase -Cnot-a-number HEAD 2>err &&
-	test_i18ngrep "numerical value" err &&
-	test_must_fail git rebase --whitespace=bad HEAD 2>err &&
-	test_i18ngrep "Invalid whitespace option" err
-'
-
 write_reflog_expect () {
 	if test $mode = --apply
 	then

index 6dabb05a2ad..ebcbd79ab8a 100755
@@ -63,6 +63,5 @@ test_rebase_am_only () {
 }
 
 test_rebase_am_only --whitespace=fix
-test_rebase_am_only -C4
 
 test_done

gitgitgadget

From: Elijah Newren <[email protected]>

--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.

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).

---
 builtin/rebase.c                       |  3 +++
 t/t3422-rebase-incompatible-options.sh | 15 ++++++++++-----
 2 files changed, 13 insertions(+), 5 deletions(-)

index ace8bd4a41c..e8bcdbf9fcd 100644
@@ -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");

index ebcbd79ab8a..d72c863b21b 100755
@@ -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

gitgitgadget

Is there a chance that this patch could break some existing tooling or someone's workflow or alias? If so, perhaps it would be better to continue recognizing it, but as a proper no-op? That is:

(1) continue accepting the option but mark it PARSE_OPT_HIDDEN or some such

(2) completely ignore the option

(3) in case someone runs across it in some existing script or example and wants to know what it does, leave one mention of it in the documentation, such as:

-C<n>
Does nothing. This option is accepted for backward compatibility only. Do not use.

If an option becomes a no-op over time due to other changes, this would likely be the route I'd lean towards. I'm just not sure it's merited in this case.

We already turned numerous no-op choices in rebase into errors with commit 8295ed690b ("rebase: make the backend configurable via config setting", 2020-02-15), where we started pointing out that apply-based and merge-based options were incompatible (as opposed to silently accepting competing options and ignoring ones not supported by whichever backend happened to trump at the time based on the options.)
So, there's recent precedent to jump directly to errors with no-ops
in rebase. Those cases are slightly different in that options were only conditionally no-ops, whereas in this case we have an option that is unconditionally a no-op, suggesting we might be able to do something stronger.

On top of that, I just can't find any evidence of anyone ever using this option: (a) it was only put in as an afterthought by the original author (who was only really interested in git-am -C), (b) there are absolutely no testcases in the testsuite covering its behavior, (c) my searches of the mailing list and google don't turn up any hits though admittedly it's hard to figure out what to search on, and (d) while I tried to use the option at times, it was only because I was doing work to make backends consistent and switch the default one and trying to understand all the inner workings, and even then I gave up on it and punted it down the road.

And, of course, the option never worked as advertised anyway.

That combination makes me think nuking it would go completely unnoticed outside this mailing list. If others would rather see the more cautious route despite all the above, I can implement it, but likely alter your step (2) from ignoring into throwing an error message (or at the very least a warning).

I suppose someone might have something like GIT_DIFF_OPTS="--unified=20" meaning they would actually have more context for their -C to act on. So I guess there is a chance that someone somewhere has actually been able to make use of git rebase -C after all? I'm not really arguing either way -- just noting the possibility, as remote as it may be.

Martin

I am afraid that this is only less than half true. Isn't the intended use of -C<num> similar to how "patch --fuzz" is used?

That is, even when a patch does not cleanly apply with full context in the incoming diff, by requiring *smaller* number of lines to match, the diff *could* be forced to apply with reduced precision?

My read of "even if context has changed a bit" in the log of that commit is exactly that. And for such a use case (which I think is the primary use case for the feature), you do not need to futz with the patch generation side at all.

commit 67dad687ad15d26d8e26f4d27874af0bc0965ce2

Author: Michael S. Tsirkin <[email protected]>
Date:   Thu Feb 8 15:57:08 2007 +0200

add -C[NUM] to git-am

Add -C[NUM] to git-am and git-rebase so that patches can be applied even if context has changed a bit.

If features A and B are incompatible and both can be specified from both command line and configuration, ideally I would expect the system to operate in one of two ways. I haven't thought it through to decide which one I prefer between the two.

* Take "command line trumps configuration" one step further, so that A that is configured but not asked for from the command line is defeated by B that is asked for from the command line.

This way, only when A and B are both requested via the configuration, of via the command line, we'd fail the operation by saying A and B are incompatible. Otherwise, the one that is configured but overridden is turned off (either silently or with a warning).

* Declare "command line trumps configuration" is only among the same feature. Regardless of how features A and B that are incompatible are requested, the command will error out, citing incompatibility. It would be very nice if the warning mentioned where the requests for features A and B came from (e.g. "You asked for -B from the command line, but you have A configured, and both cannot be active at the same time---disable A from the command line, or do not ask for B")

When A is configured and B is requested from the command line, the command will error out, and the user must defeat A from the command line before the user can use B, e.g. "git cmd --no-A -B".

A knee-jerk reaction to the situation is that the latter feels somewhat safer than the former, but when I imagine the actual end user who saw the error message, especially the suggested solution "disable A from the command line or do not ask for B from the command line", may say "well, I asked for B for this invocation explicitly with -B from the command line, and you(Git) should be able to make it imply --no-A", which amounts to the same thing as the former choice.

Oh! Reducing the number of lines of context to pay attention to never even occurred to me for whatever reason. I'll drop the patch.

Ah, good point. And combined with Junio's point that -C is apparently about reducing rather than increasing context, I should just drop the patch.

Yup. "completely useless" on the title is less than half correct, but still correct for a minor use case where -C is used to use *more* context lines than the default.

We could update "rebase --apply" codepath to increase the context lines generated by format-patch. That would make the "completely useless" totally incorrect ;-)

Hi Elijah

Thanks for working on this

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

I agree that one of the two ways you highlight below would be ideal. Should this series be held up on extending the checks to implement this ideal, or are you just commenting for whoever later circles back to implement this?

If it is clear to the user that A and B preclude each other, then I agree with this sentiment that the former choice (silently ignoring the config) would avoid a minor frustration for some users and thus would be better. But I don't think that's applicable here. There is no reason that --whitespace=fix shouldn't be available from the merge backend other than that we haven't implemented it yet, and it's likely feasible to implement --update-refs for the apply backend with enough effort if we thought it was worth it. So, if a user sets rebase.updateRefs=true in their config because they always want included branches updated, but one time they run git rebase --whitespace=fix, they will likely have a negative experience like the one that inspired this patch. Perhaps we're forced to choose between possible frustration by different end users, but if so, I think trying to debug and figure out "Wait, I switched to this branch and started tweaking it but it appears to not have some relevant changes I'm sure I made to it yesterday. What happened?" is a much worse frustration than "I have to manually specify --no-A in this special case". So, when it's not at all obvious that A and B preclude each other, I think we're better off giving the error.

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

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.

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.

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

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/[email protected]/

We had a report about --update-refs being ignored when --whitespace=fix was passed, confusing an end user. These were already marked as incompatible in the manual, but the code didn't check for the incompatibility and provide an error to the user.

Folks brought up other flags tangentially when reviewing an earlier round of this series, and I found we had more that were missing checks, and that we were missing some testcases, and that the documentation was wrong on some of the relevant options. So, I ended up getting lots of little fixes to straighten these all out.

Left out of this series:

* If an option like rebase.autosquash or rebase.updateRefs are selected and the user specifies e.g. --whitespace=fix, we should either (a) tailor the error message better to point out that it's a config option that is incompatible with their command line flag, or (b) implement --whitespace=fix for the merge backend so we can just deprecate and eventually remove the apply backend[1].

[1] See "longer term goal" at
https://lore.kernel.org/git/[email protected]/

Changes since v2:

* Remove the extra patch and reword the comment in t3422 more thoroughly.
* Add tests for other flag incompatibilities that were missing
* Add code that was missing for checking various flag incompatibilities
* Fix incorrect claims in the documentation around incompatible options
* A few other miscellaneous fixups noticed while doing all the above

Changes since v1:

* Add a patch nuking the -C option to rebase (fixes confusion around the comment in t3422 and acknowledges the fact that the option is totally and utterly useless and always has been. It literally never affects the results of a rebase.)

Elijah Newren (7):
rebase: mark --update-refs as requiring the merge backend
rebase: flag --apply and --merge as incompatible
rebase: remove --allow-empty-message from incompatible opts
rebase: fix docs about incompatibilities with --root
rebase: add coverage of other incompatible options
rebase: clarify the OPT_CMDMODE incompatibilities
rebase: fix formatting of rebase --reapply-cherry-picks option in docs

 Documentation/git-rebase.txt           | 73 +++++++++++++-------------
 builtin/rebase.c                       | 36 +++++++++----
 t/t3422-rebase-incompatible-options.sh | 42 +++++++++++++--
 3 files changed, 101 insertions(+), 50 deletions(-)

base-commit: 221222b278e713054e65cbbbcb2b1ac85483ea89
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1466%2Fnewren%2Frebase-update-refs-imply-merge-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1466/newren/rebase-update-refs-imply-merge-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1466

Range-diff vs v2:

2: 2e44d0b7e57 ! 1: 9089834adea rebase: mark --update-refs as requiring the merge backend
@@ Commit message
--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.
+ incompatibility and warn the user. Check and error now.

- 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).
+ While at it, fix a typo in t3422...and fix some misleading wording
+ (most options which used to be am-specific have since been implemented
+ in the merge backend as well).

+ ## Documentation/git-rebase.txt ##
+@@ Documentation/git-rebase.txt: start would be overridden by the presence of
+ +
+ If the configuration variable rebase.updateRefs is set, then this option
+ can be used to override and disable this setting.
+++
++See also INCOMPATIBLE OPTIONS below.
+
+ INCOMPATIBLE OPTIONS
+ --------------------
+
## builtin/rebase.c ##
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
}
@@ t/t3422-rebase-incompatible-options.sh: test_expect_success 'setup' '
-# --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.
++# Rebase has a couple options which are specific to the apply backend, ++# and several options which are specific to the merge backend. Flags ++# from the different sets cannot work together, and we do not want to ++# just ignore one of the sets of flags. Make sure rebase warns the ++# user and aborts instead.
#

test_rebase_am_only () {
1: a0f8f5fac1c ! 2: a8b5a0e4fb0 rebase: remove completely useless -C option
@@ Metadata
Author: Elijah Newren <[email protected]>

## Commit message ##
- rebase: remove completely useless -C option
+ rebase: flag --apply and --merge as incompatible

- The -C option to rebase was introduced with 67dad687ad ("add -C[NUM]
- to git-am", 2007-02-08).  Based on feedback on the patch, the author
- added the -C option not just to git-am but also to git-rebase.  But did
- it such that the option was just passed through to git-am (which passes
- it along to git-apply), with no corresponding option to format-patch.
-
- As per the git-apply documentation for the -C option:
- Ensure at least <n> lines of surrounding context match...When fewer
- lines of surrounding context exist they all must match.
-
- The fact that format-patch was not passed a -U option to increase the
- number of context lines meant that there would still only be 3 lines of
- context to match on. So, anyone attempting to use this option in
- git-rebase would just be spinning their wheels and wasting time. I was
- one such user a number of years ago.
-
- Since this option can at best waste users time and is nothing more than
- a confusing no-op, and has never been anything other than a confusing
- no-op, and no one has ever bothered to create a testcase for it that
- goes beyond option parsing, simply excise the option.
+ Previously, we flagged options which implied --apply as being
+ incompatible with options which implied --merge. But if both options
+ were given explicitly, then we didn't flag the incompatibility. The
+ same is true with --apply and --interactive. Add the check, and add
+ some testcases to verify these are also caught.

- ## Documentation/git-rebase.txt ##
-@@ Documentation/git-rebase.txt: include::rerere-options.txt[]
- Allows the pre-rebase hook to run, which is the default. This option can
- be used to override --no-verify. See also linkgit:githooks[5].
+ ## builtin/rebase.c ##
+@@ builtin/rebase.c: static int parse_opt_am(const struct option *opt, const char *arg, int unset)
+ BUG_ON_OPT_NEG(unset);
+ BUG_ON_OPT_ARG(arg);

---C<n>::
-- Ensure at least <n> lines of surrounding context match before
-- and after each change. When fewer lines of surrounding
-- context exist they all must match. By default no context is
-- ever ignored. Implies --apply.
--+
--See also INCOMPATIBLE OPTIONS below.
--
- --no-ff::
- --force-rebase::
- -f::
-@@ Documentation/git-rebase.txt: The following options:
++ if (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_APPLY)
++ die(_("apply options and merge options cannot be used together"));
++
+ opts->type = REBASE_APPLY;

- * --apply
- * --whitespace
-- * -C
+ return 0;
+@@ builtin/rebase.c: static int parse_opt_merge(const struct option *opt, const char *arg, int unset)
+ BUG_ON_OPT_NEG(unset);
+ BUG_ON_OPT_ARG(arg);

- are incompatible with the following options:
+- if (!is_merge(opts))
+- opts->type = REBASE_MERGE;
++ if (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_MERGE)
++ die(_("apply options and merge options cannot be used together"));
++
++ opts->type = REBASE_MERGE;

-
- ## builtin/rebase.c ##
-@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
- N_("ignore author date and use current date")),
- OPT_HIDDEN_BOOL(0, "ignore-date", &options.ignore_date,
- N_("synonym of --reset-author-date")),
-- OPT_PASSTHRU_ARGV('C', NULL, &options.git_am_opts, N_("n"),
-- N_("passed to 'git apply'"), 0),
- OPT_BOOL(0, "ignore-whitespace", &ignore_whitespace,
- N_("ignore changes in whitespace")),
- OPT_PASSTHRU_ARGV(0, "whitespace", &options.git_am_opts,
-@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
- if (!strcmp(option, "--whitespace=fix") ||
- !strcmp(option, "--whitespace=strip"))
- allow_preemptive_ff = 0;
-- else if (skip_prefix(option, "-C", &p)) {
-- while (*p)
-- if (!isdigit(*(p++)))
-- die(_("switch `C' expects a "
-- "numerical value"));
-- } else if (skip_prefix(option, "--whitespace=", &p)) {
-+ else if (skip_prefix(option, "--whitespace=", &p)) {
- if (*p && strcmp(p, "warn") && strcmp(p, "nowarn") &&
- strcmp(p, "error") && strcmp(p, "error-all"))
- die("Invalid whitespace option: '%s'", p);
-
- ## t/t3406-rebase-message.sh ##
-@@ t/t3406-rebase-message.sh: test_expect_success 'rebase --onto outputs the invalid ref' '
- test_i18ngrep "invalid-ref" err
- '
+ return 0;
+ }
+@@ builtin/rebase.c: static int parse_opt_interactive(const struct option *opt, const char *arg,
+ BUG_ON_OPT_NEG(unset);
+ BUG_ON_OPT_ARG(arg);
+
++ if (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_MERGE)
++ die(_("apply options and merge options cannot be used together"));
++
+ opts->type = REBASE_MERGE;
+ opts->flags |= REBASE_INTERACTIVE_EXPLICIT;

--test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' '
-- test_must_fail git rebase -Cnot-a-number HEAD 2>err &&
-- test_i18ngrep "numerical value" err &&
-- test_must_fail git rebase --whitespace=bad HEAD 2>err &&
-- test_i18ngrep "Invalid whitespace option" err
--'
--
- write_reflog_expect () {
- if test $mode = --apply
- then

## t/t3422-rebase-incompatible-options.sh ##
@@ t/t3422-rebase-incompatible-options.sh: test_rebase_am_only () {
+
}

++# Check options which imply --apply
test_rebase_am_only --whitespace=fix
--test_rebase_am_only -C4
+ test_rebase_am_only -C4
++# Also check an explicit --apply
++test_rebase_am_only --apply

test_done

 -:  ----------- > 3:  f4fbfd40d45 rebase: remove --allow-empty-message from incompatible opts
 -:  ----------- > 4:  a1e61ac8f21 rebase: fix docs about incompatibilities with --root
 -:  ----------- > 5:  48c40c0dda0 rebase: add coverage of other incompatible options
 -:  ----------- > 6:  8664cce6cf7 rebase: clarify the OPT_CMDMODE incompatibilities
 -:  ----------- > 7:  0e8b06163f2 rebase: fix formatting of rebase --reapply-cherry-picks option in docs

gitgitgadget

From: Elijah Newren <[email protected]>

--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 error now.

While at it, fix a typo in t3422...and fix some misleading wording (most options which used to be am-specific have since been implemented in the merge backend as well).

---
 Documentation/git-rebase.txt           |  2 ++
 builtin/rebase.c                       |  3 +++
 t/t3422-rebase-incompatible-options.sh | 15 ++++++++++-----
 3 files changed, 15 insertions(+), 5 deletions(-)

index f9675bd24e6..00d21d7287d 100644
@@ -623,6 +623,8 @@ start would be overridden by the presence of
 +
 If the configuration variable `rebase.updateRefs` is set, then this option
 can be used to override and disable this setting.
++
+See also INCOMPATIBLE OPTIONS below.
 
 INCOMPATIBLE OPTIONS
 --------------------

index 1481c5b6a5b..accd62fce48 100644
@@ -1514,6 +1514,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");

index 6dabb05a2ad..9da39cd91c2 100755
@@ -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 couple options which are specific to the apply backend,
+# and several options which are specific to the merge backend.  Flags
+# from the different sets cannot work together, and we do not want to
+# just ignore one of the sets of flags.  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

gitgitgadget

From: Elijah Newren <[email protected]>

Previously, we flagged options which implied --apply as being incompatible with options which implied --merge. But if both options were given explicitly, then we didn't flag the incompatibility. The same is true with --apply and --interactive. Add the check, and add some testcases to verify these are also caught.

---
 builtin/rebase.c                       | 12 ++++++++++--
 t/t3422-rebase-incompatible-options.sh |  3 +++
 2 files changed, 13 insertions(+), 2 deletions(-)

index accd62fce48..2a5e0e8a7a0 100644
@@ -920,6 +920,9 @@ static int parse_opt_am(const struct option *opt, const char *arg, int unset)
 	BUG_ON_OPT_NEG(unset);
 	BUG_ON_OPT_ARG(arg);
 
+	if (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_APPLY)
+	    die(_("apply options and merge options cannot be used together"));
+
 	opts->type = REBASE_APPLY;
 
 	return 0;

@@ -933,8 +936,10 @@ static int parse_opt_merge(const struct option *opt, const char *arg, int unset)
 	BUG_ON_OPT_NEG(unset);
 	BUG_ON_OPT_ARG(arg);
 
-	if (!is_merge(opts))
-		opts->type = REBASE_MERGE;
+	if (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_MERGE)
+	    die(_("apply options and merge options cannot be used together"));
+
+	opts->type = REBASE_MERGE;
 
 	return 0;
 }

@@ -948,6 +953,9 @@ static int parse_opt_interactive(const struct option *opt, const char *arg,
 	BUG_ON_OPT_NEG(unset);
 	BUG_ON_OPT_ARG(arg);
 
+	if (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_MERGE)
+	    die(_("apply options and merge options cannot be used together"));
+
 	opts->type = REBASE_MERGE;
 	opts->flags |= REBASE_INTERACTIVE_EXPLICIT;
 

index 9da39cd91c2..9b9e78479f6 100755
@@ -67,7 +67,10 @@ test_rebase_am_only () {
 
 }
 
+# Check options which imply --apply
 test_rebase_am_only --whitespace=fix
 test_rebase_am_only -C4
+# Also check an explicit --apply
+test_rebase_am_only --apply
 
 test_done

gitgitgadget

From: Elijah Newren <[email protected]>

--allow-empty-message was turned into a no-op and even documented as such; the flag is simply ignored. Since the flag is ignored, it shouldn't be documented as being incompatible with other flags.

---
 Documentation/git-rebase.txt | 1 -
 1 file changed, 1 deletion(-)

index 00d21d7287d..3929535c0cd 100644
@@ -640,7 +640,6 @@ are incompatible with the following options:
  * --merge
  * --strategy
  * --strategy-option
- * --allow-empty-message
  * --[no-]autosquash
  * --rebase-merges
  * --interactive

gitgitgadget

From: Elijah Newren <[email protected]>

In commit 5dacd4abdd ("git-rebase.txt: document incompatible options", 2018-06-25), I added notes about incompatibilities between options for the apply and merge backends. Unfortunately, I inverted the condition when --root was incompatible with the apply backend. Fix the documentation, and add a testcase that verifies the documentation matches the code.

While at it, the documentation for --root also tried to cover some of the backend differences between the apply and merge backends in relation to reapplying cherry picks. The information:
* assumed that the apply backend was the default (it isn't anymore)
* was written before --reapply-cherry-picks became an option
* was written before the detailed information on backend differences
All of these factors make the sentence under --root about reapplying cherry picks contradict information that is now available elsewhere in the manual, and the other references are correct. So just strike this sentence.

---
 Documentation/git-rebase.txt           | 7 ++-----
 t/t3422-rebase-incompatible-options.sh | 4 ++++
 2 files changed, 6 insertions(+), 5 deletions(-)

index 3929535c0cd..3f12d242d83 100644
@@ -567,10 +567,7 @@ See also INCOMPATIBLE OPTIONS below.
 --root::
 	Rebase all commits reachable from `<branch>`, instead of
 	limiting them with an `<upstream>`.  This allows you to rebase
-	the root commit(s) on a branch.  When used with `--onto`, it
-	will skip changes already contained in `<newbase>` (instead of
-	`<upstream>`) whereas without `--onto` it will operate on every
-	change.
+	the root commit(s) on a branch.
 +
 See also INCOMPATIBLE OPTIONS below.
 

@@ -649,7 +646,7 @@ are incompatible with the following options:
  * --reapply-cherry-picks
  * --edit-todo
  * --update-refs
- * --root when used in combination with --onto
+ * --root when used without --onto
 
 In addition, the following pairs of options are incompatible:
 

index 9b9e78479f6..f86274990b0 100755
@@ -65,6 +65,10 @@ test_rebase_am_only () {
 		test_must_fail git rebase $opt --update-refs A
 	"
 
+	test_expect_success "$opt incompatible with --root without --onto" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --root A
+	"
 }
 
 # Check options which imply --apply

gitgitgadget

From: Elijah Newren <[email protected]>

The git-rebase manual noted several sets of incompatible options, but we were missing tests for a few of these. Further, we were missing code checks for some of these, which could result in command line options being silently ignored.

---
 builtin/rebase.c                       | 21 ++++++++++++++-------
 t/t3422-rebase-incompatible-options.sh | 20 ++++++++++++++++++++
 2 files changed, 34 insertions(+), 7 deletions(-)

index 2a5e0e8a7a0..6dcdb59bb02 100644
@@ -1238,6 +1238,17 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		if (options.fork_point < 0)
 			options.fork_point = 0;
 	}
+	/*
+	 * reapply_cherry_picks is slightly weird.  It starts out with a
+	 * value of -1.  It will be assigned a value of keep_base below and
+	 * then handled specially.  The apply backend is hardcoded to
+	 * behave like reapply_cherry_picks==1, so if it has that value, we
+	 * can just ignore the flag with the apply backend.  Thus, we only
+	 * really need to throw an error and require the merge backend if
+	 * reapply_cherry_picks==0.
+	 */
+	if (options.reapply_cherry_picks == 0)
+		imply_merge(&options, "--no-reapply-cherry-picks");
 	/*
 	 * --keep-base defaults to --reapply-cherry-picks to avoid losing
 	 * commits when using this option.

@@ -1420,13 +1431,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	if (options.empty != EMPTY_UNSPECIFIED)
 		imply_merge(&options, "--empty");
 
-	/*
-	 * --keep-base implements --reapply-cherry-picks by altering upstream so
-	 * it works with both backends.
-	 */
-	if (options.reapply_cherry_picks && !keep_base)
-		imply_merge(&options, "--reapply-cherry-picks");
-
 	if (gpg_sign)
 		options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);
 

@@ -1525,6 +1529,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	if (options.update_refs)
 		imply_merge(&options, "--update-refs");
 
+	if (options.autosquash)
+		imply_merge(&options, "--autosquash");
+
 	if (options.type == REBASE_UNSPECIFIED) {
 		if (!strcmp(options.default_backend, "merge"))
 			imply_merge(&options, "--merge");

index f86274990b0..c830025470f 100755
@@ -50,6 +50,11 @@ test_rebase_am_only () {
 		test_must_fail git rebase $opt --strategy-option=ours A
 	"
 
+	test_expect_success "$opt incompatible with --autosquash" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --autosquash A
+	"
+
 	test_expect_success "$opt incompatible with --interactive" "
 		git checkout B^0 &&
 		test_must_fail git rebase $opt --interactive A

@@ -60,6 +65,21 @@ test_rebase_am_only () {
 		test_must_fail git rebase $opt --exec 'true' A
 	"
 
+	test_expect_success "$opt incompatible with --keep-empty" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --keep-empty A
+	"
+
+	test_expect_success "$opt incompatible with --empty=..." "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --empty=ask A
+	"
+
+	test_expect_success "$opt incompatible with --no-reapply-cherry-picks" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --no-reapply-cherry-picks A
+	"
+
 	test_expect_success "$opt incompatible with --update-refs" "
 		git checkout B^0 &&
 		test_must_fail git rebase $opt --update-refs A

gitgitgadget

From: Elijah Newren <[email protected]>

--edit-todo was documented as being incompatible with any of the options for the apply backend. However, it is also incompatible with any of the options for the merge backend, and is incompatible with any options that are not backend specific as well. The same can be said for --continue, --skip, --abort, --quit, etc.

This is already somewhat implicitly covered by the synopsis, but since "[<options>]" in the first two variants are vague it might be easy to miss this. That might not be a big deal, but since the rebase manpage has to spend so much verbiage about incompatibility of options, making a separate section for these options that are incompatible with everything else seems clearer. Do that, and remove the needless inclusion of --edit-todo in the explicit incompatibility list.

---
 Documentation/git-rebase.txt | 61 +++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 28 deletions(-)

index 3f12d242d83..00a9e22bc32 100644
@@ -201,6 +201,39 @@ Alternatively, you can undo the 'git rebase' with
 
     git rebase --abort
 
+MODE OPTIONS
+------------
+
+The options in this section cannot be used with any other option,
+including not with each other:
+
+--continue::
+	Restart the rebasing process after having resolved a merge conflict.
+
+--skip::
+	Restart the rebasing process by skipping the current patch.
+
+--abort::
+	Abort the rebase operation and reset HEAD to the original
+	branch. If `<branch>` was provided when the rebase operation was
+	started, then `HEAD` will be reset to `<branch>`. Otherwise `HEAD`
+	will be reset to where it was when the rebase operation was
+	started.
+
+--quit::
+	Abort the rebase operation but `HEAD` is not reset back to the
+	original branch. The index and working tree are also left
+	unchanged as a result. If a temporary stash entry was created
+	using `--autostash`, it will be saved to the stash list.
+
+--edit-todo::
+	Edit the todo list during an interactive rebase.
+
+--show-current-patch::
+	Show the current patch in an interactive rebase or when rebase
+	is stopped because of conflicts. This is the equivalent of
+	`git show REBASE_HEAD`.
+
 OPTIONS
 -------
 --onto <newbase>::

@@ -242,22 +275,6 @@ See also INCOMPATIBLE OPTIONS below.
 <branch>::
 	Working branch; defaults to `HEAD`.
 
---continue::
-	Restart the rebasing process after having resolved a merge conflict.
-
---abort::
-	Abort the rebase operation and reset HEAD to the original
-	branch. If `<branch>` was provided when the rebase operation was
-	started, then `HEAD` will be reset to `<branch>`. Otherwise `HEAD`
-	will be reset to where it was when the rebase operation was
-	started.
-
---quit::
-	Abort the rebase operation but `HEAD` is not reset back to the
-	original branch. The index and working tree are also left
-	unchanged as a result. If a temporary stash entry was created
-	using `--autostash`, it will be saved to the stash list.
-
 --apply::
 	Use applying strategies to rebase (calling `git-am`
 	internally).  This option may become a no-op in the future

@@ -338,17 +355,6 @@ See also INCOMPATIBLE OPTIONS below.
 +
 See also INCOMPATIBLE OPTIONS below.
 
---skip::
-	Restart the rebasing process by skipping the current patch.
-
---edit-todo::
-	Edit the todo list during an interactive rebase.
-
---show-current-patch::
-	Show the current patch in an interactive rebase or when rebase
-	is stopped because of conflicts. This is the equivalent of
-	`git show REBASE_HEAD`.
-
 -m::
 --merge::
 	Using merging strategies to rebase (default).

@@ -644,7 +650,6 @@ are incompatible with the following options:
  * --no-keep-empty
  * --empty=
  * --reapply-cherry-picks
- * --edit-todo
  * --update-refs
  * --root when used without --onto
 

gitgitgadget

From: Elijah Newren <[email protected]>

Commit ce5238a690 ("rebase --keep-base: imply --reapply-cherry-picks", 2022-10-17) accidentally added some blank lines that cause extra paragraphs about --reapply-cherry-picks to be considered not part of the documentation of that option. Remove the blank lines to make it clear we are still discussing --reapply-cherry-picks.

---
 Documentation/git-rebase.txt | 2 --
 1 file changed, 2 deletions(-)

index 00a9e22bc32..140c984d0ea 100644
@@ -331,7 +331,6 @@ See also INCOMPATIBLE OPTIONS below.
 	upstream changes, the behavior towards them is controlled by
 	the `--empty` flag.)
 +
-
 In the absence of `--keep-base` (or if `--no-reapply-cherry-picks` is
 given), these commits will be automatically dropped.  Because this
 necessitates reading all upstream commits, this can be expensive in

@@ -340,7 +339,6 @@ read. When using the 'merge' backend, warnings will be issued for each
 dropped commit (unless `--quiet` is given). Advice will also be issued
 unless `advice.skippedCherryPicks` is set to false (see
 linkgit:git-config[1]).
-
 +
 `--reapply-cherry-picks` allows rebase to forgo reading all upstream
 commits, potentially improving performance.

gitgitgadget

haha :-)

I'd probably go for that, but since in my mind the plan is still to deprecate and remove the apply backend as you suggested at [1], I'm not particularly motivated to improve/extend options specific to the apply backend of rebase.

[1] https://lore.kernel.org/git/[email protected]/

Hi Elijah

The patch looks fine. Just to note that

#leftoverbits - I notice there is some code in builtin/rebase.c, builtin/revert.c and sequencer.[ch] related to this option that could be removed. The setting seems to be completely ignored by the sequencer and so could be removed from struct replay_opts.

Best Wishes

Phillip

Hi Elijah

	git format-patch --right-only $upstream...$head

	--whitespace=fix --no-reapply-cherry-picks
	--whitespace=fix --keep-base --reapply-cherry-picks

	--whitespace=fix --reapply-cherry-picks

Hi Elijah

Thanks for clearing up my mess!

Phillip

Sorry I somhow managed to omit --cherry-pick from that, it should have been

	git format-patch --right-only --cherry-pick $upstream...$head

Phillip

I still consider that deprecating and removing the "am|format-patch pipeline" mode is a good longer term goal, but it seems we still have some features that are only available with the mode and not the other "sequence of cherry-pick" mode? We'd need to port that over to the other backend before we can wean ourselves off of the apply backend. Until then, ...

Oh, indeed, I was reading wrong. Thanks for the correction. I still think there's something to fix up here, which I'll include in my re-roll.

Right, nor do we support

      --whitespace=fix --keep-base --no-reapply-cherry-picks

(If you try it, you'll notice that the code accepts those flags and starts the rebase pretending everything is fine, but it silently ignores the --no-reapply-cherry-picks flag.)

Stepping back a bit, though, the apply backend doesn't support toggling --[no-]reapply-cherry-picks at all. It hard codes the behavior (in a way dependent upon --keep-base). So, if the user passes --[no-]reapply-cherry-picks and we don't error out, we are just going to ignore whatever they passed and do whatever hardcoded thing is baked into the algorithm.

While the user can pass the flag in a way that happens to match what is baked into the apply backend, I'd still say it's wrong for them to specify it since we will just ignore it. Doing so is likely to result in the user later toggling the flag and being surprised that they get an error when it used to be accepted, or seeing that it only sometimes gets accepted.

Anyway, I'll update this patch to document this a bit more clearly in a code comment and add an improved check.

We had a report about --update-refs being ignored when --whitespace=fix was passed, confusing an end user. These were already marked as incompatible in the manual, but the code didn't check for the incompatibility and provide an error to the user.

Folks brought up other flags tangentially when reviewing an earlier round of this series, and I found we had more that were missing checks, and that we were missing some testcases, and that the documentation was wrong on some of the relevant options. So, I ended up getting lots of little fixes to straighten these all out.

Changes since v3:

* Corrected the code surrounding --[no-]reapply-cherry-picks, and extended the testcases (Thanks to Phillip for pointing out my error)
* I went ahead and implemented the better error message when the merge backend is triggered solely via config options.

Changes since v2:

* Remove the extra patch and reword the comment in t3422 more thoroughly.
* Add tests for other flag incompatibilities that were missing
* Add code that was missing for checking various flag incompatibilities
* Fix incorrect claims in the documentation around incompatible options
* A few other miscellaneous fixups noticed while doing all the above

Changes since v1:

* Add a patch nuking the -C option to rebase (fixes confusion around the comment in t3422 and acknowledges the fact that the option is totally and utterly useless and always has been. It literally never affects the results of a rebase.)

Elijah Newren (9):
rebase: mark --update-refs as requiring the merge backend
rebase: flag --apply and --merge as incompatible
rebase: remove --allow-empty-message from incompatible opts
rebase: fix docs about incompatibilities with --root
rebase: add coverage of other incompatible options
rebase: clarify the OPT_CMDMODE incompatibilities
rebase: fix formatting of rebase --reapply-cherry-picks option in docs
rebase: put rebase_options initialization in single place
rebase: provide better error message for apply options vs. merge
config

 Documentation/git-rebase.txt           | 77 +++++++++++++-------------
 builtin/rebase.c                       | 72 +++++++++++++++++++-----
 t/t3422-rebase-incompatible-options.sh | 71 ++++++++++++++++++++++--
 3 files changed, 162 insertions(+), 58 deletions(-)

base-commit: 56c8fb1e95377900ec9d53c07886022af0a5d3c2
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1466%2Fnewren%2Frebase-update-refs-imply-merge-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1466/newren/rebase-update-refs-imply-merge-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1466

Range-diff vs v3:

  1:  9089834adea =  1:  8a676e6ec1a rebase: mark --update-refs as requiring the merge backend
  2:  a8b5a0e4fb0 =  2:  cc129b87185 rebase: flag --apply and --merge as incompatible
  3:  f4fbfd40d45 =  3:  9464bbbe9ba rebase: remove --allow-empty-message from incompatible opts
  4:  a1e61ac8f21 =  4:  b702f15ed7c rebase: fix docs about incompatibilities with --root
  5:  48c40c0dda0 !  5:  5e4851e611e rebase: add coverage of other incompatible options

@@ Commit message
code checks for some of these, which could result in command line options being silently ignored.

+ Also, note that adding a check for autosquash means that using
+ --whitespace=fix together with the config setting rebase.autosquash=true
+ will trigger an error. A subsequent commit will improve the error
+ message.
+

+ ## Documentation/git-rebase.txt ##
+@@ Documentation/git-rebase.txt: are incompatible with the following options:
+ * --exec
+ * --no-keep-empty
+ * --empty=
+- * --reapply-cherry-picks
++ * --[no-]reapply-cherry-picks
+ * --edit-todo
+ * --update-refs
+ * --root when used without --onto
+
## builtin/rebase.c ##
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
if (options.fork_point < 0)
options.fork_point = 0;
}
+ /*
-+ * reapply_cherry_picks is slightly weird. It starts out with a
-+ * value of -1. It will be assigned a value of keep_base below and
-+ * then handled specially. The apply backend is hardcoded to
-+ * behave like reapply_cherry_picks==1, so if it has that value, we
-+ * can just ignore the flag with the apply backend. Thus, we only
-+ * really need to throw an error and require the merge backend if
-+ * reapply_cherry_picks==0.
++ * The apply backend does not support --[no-]reapply-cherry-picks.
++ * The behavior it implements by default is equivalent to
++ * --no-reapply-cherry-picks (due to passing --cherry-picks to
++ * format-patch), but --keep-base alters the upstream such that no
++ * cherry-picks can be found (effectively making it act like
++ * --reapply-cherry-picks).
++ *
++ * Now, if the user does specify --[no-]reapply-cherry-picks, but
++ * does so in such a way that options.reapply_cherry_picks ==
++ * keep_base, then the behavior they get will match what they
++ * expect despite options.reapply_cherry_picks being ignored. We
++ * could just allow the flag in that case, but it seems better to
++ * just alert the user that they've specified a flag that the
++ * backend ignores.
+ */
-+ if (options.reapply_cherry_picks == 0)
-+ imply_merge(&options, "--no-reapply-cherry-picks");
++ if (options.reapply_cherry_picks >= 0)
++ imply_merge(&options, options.reapply_cherry_picks ? "--reapply-cherry-picks" :
++ "--no-reapply-cherry-picks");
++
/*
* --keep-base defaults to --reapply-cherry-picks to avoid losing
* commits when using this option.
@@ t/t3422-rebase-incompatible-options.sh: test_rebase_am_only () {
+ git checkout B^0 &&
+ test_must_fail git rebase $opt --no-reapply-cherry-picks A
+ "
++
++ test_expect_success "$opt incompatible with --reapply-cherry-picks" "
++ git checkout B^0 &&
++ test_must_fail git rebase $opt --reapply-cherry-picks A
++ "
+
test_expect_success "$opt incompatible with --update-refs" "
git checkout B^0 &&
6: 8664cce6cf7 ! 6: 21ae9e05313 rebase: clarify the OPT_CMDMODE incompatibilities
@@ Documentation/git-rebase.txt: See also INCOMPATIBLE OPTIONS below.
@@ Documentation/git-rebase.txt: are incompatible with the following options:
* --no-keep-empty
* --empty=
- * --reapply-cherry-picks
+ * --[no-]reapply-cherry-picks
- * --edit-todo
* --update-refs
* --root when used without --onto

  7:  0e8b06163f2 =  7:  74a216bf0c0 rebase: fix formatting of rebase --reapply-cherry-picks option in docs
  -:  ----------- >  8:  a8adf7fda61 rebase: put rebase_options initialization in single place
  -:  ----------- >  9:  5cb00e5103b rebase: provide better error message for apply options vs. merge config

gitgitgadget

From: Elijah Newren <[email protected]>

--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 error now.

While at it, fix a typo in t3422...and fix some misleading wording (most options which used to be am-specific have since been implemented in the merge backend as well).

---
 Documentation/git-rebase.txt           |  2 ++
 builtin/rebase.c                       |  3 +++
 t/t3422-rebase-incompatible-options.sh | 15 ++++++++++-----
 3 files changed, 15 insertions(+), 5 deletions(-)

index d811c1cf443..6490bc96a15 100644
@@ -630,6 +630,8 @@ start would be overridden by the presence of
 +
 If the configuration variable `rebase.updateRefs` is set, then this option
 can be used to override and disable this setting.
++
+See also INCOMPATIBLE OPTIONS below.
 
 INCOMPATIBLE OPTIONS
 --------------------

index a26cc0cfdb5..c111b89e137 100644
@@ -1492,6 +1492,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");

index 6dabb05a2ad..9da39cd91c2 100755
@@ -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 couple options which are specific to the apply backend,
+# and several options which are specific to the merge backend.  Flags
+# from the different sets cannot work together, and we do not want to
+# just ignore one of the sets of flags.  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

gitgitgadget

From: Elijah Newren <[email protected]>

Previously, we flagged options which implied --apply as being incompatible with options which implied --merge. But if both options were given explicitly, then we didn't flag the incompatibility. The same is true with --apply and --interactive. Add the check, and add some testcases to verify these are also caught.

---
 builtin/rebase.c                       | 12 ++++++++++--
 t/t3422-rebase-incompatible-options.sh |  3 +++
 2 files changed, 13 insertions(+), 2 deletions(-)

index c111b89e137..b742cc8fb5c 100644
@@ -907,6 +907,9 @@ static int parse_opt_am(const struct option *opt, const char *arg, int unset)
 	BUG_ON_OPT_NEG(unset);
 	BUG_ON_OPT_ARG(arg);
 
+	if (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_APPLY)
+	    die(_("apply options and merge options cannot be used together"));
+
 	opts->type = REBASE_APPLY;
 
 	return 0;

@@ -920,8 +923,10 @@ static int parse_opt_merge(const struct option *opt, const char *arg, int unset)
 	BUG_ON_OPT_NEG(unset);
 	BUG_ON_OPT_ARG(arg);
 
-	if (!is_merge(opts))
-		opts->type = REBASE_MERGE;
+	if (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_MERGE)
+	    die(_("apply options and merge options cannot be used together"));
+
+	opts->type = REBASE_MERGE;
 
 	return 0;
 }

@@ -935,6 +940,9 @@ static int parse_opt_interactive(const struct option *opt, const char *arg,
 	BUG_ON_OPT_NEG(unset);
 	BUG_ON_OPT_ARG(arg);
 
+	if (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_MERGE)
+	    die(_("apply options and merge options cannot be used together"));
+
 	opts->type = REBASE_MERGE;
 	opts->flags |= REBASE_INTERACTIVE_EXPLICIT;
 

index 9da39cd91c2..9b9e78479f6 100755
@@ -67,7 +67,10 @@ test_rebase_am_only () {
 
 }
 
+# Check options which imply --apply
 test_rebase_am_only --whitespace=fix
 test_rebase_am_only -C4
+# Also check an explicit --apply
+test_rebase_am_only --apply
 
 test_done

gitgitgadget

From: Elijah Newren <[email protected]>

--allow-empty-message was turned into a no-op and even documented as such; the flag is simply ignored. Since the flag is ignored, it shouldn't be documented as being incompatible with other flags.

---
 Documentation/git-rebase.txt | 1 -
 1 file changed, 1 deletion(-)

index 6490bc96a15..7d01d1412d1 100644
@@ -647,7 +647,6 @@ are incompatible with the following options:
  * --merge
  * --strategy
  * --strategy-option
- * --allow-empty-message
  * --[no-]autosquash
  * --rebase-merges
  * --interactive

gitgitgadget

From: Elijah Newren <[email protected]>

In commit 5dacd4abdd ("git-rebase.txt: document incompatible options", 2018-06-25), I added notes about incompatibilities between options for the apply and merge backends. Unfortunately, I inverted the condition when --root was incompatible with the apply backend. Fix the documentation, and add a testcase that verifies the documentation matches the code.

While at it, the documentation for --root also tried to cover some of the backend differences between the apply and merge backends in relation to reapplying cherry picks. The information:
* assumed that the apply backend was the default (it isn't anymore)
* was written before --reapply-cherry-picks became an option
* was written before the detailed information on backend differences
All of these factors make the sentence under --root about reapplying cherry picks contradict information that is now available elsewhere in the manual, and the other references are correct. So just strike this sentence.

---
 Documentation/git-rebase.txt           | 7 ++-----
 t/t3422-rebase-incompatible-options.sh | 4 ++++
 2 files changed, 6 insertions(+), 5 deletions(-)

index 7d01d1412d1..846aeed1b69 100644
@@ -574,10 +574,7 @@ See also INCOMPATIBLE OPTIONS below.
 --root::
 	Rebase all commits reachable from `<branch>`, instead of
 	limiting them with an `<upstream>`.  This allows you to rebase
-	the root commit(s) on a branch.  When used with `--onto`, it
-	will skip changes already contained in `<newbase>` (instead of
-	`<upstream>`) whereas without `--onto` it will operate on every
-	change.
+	the root commit(s) on a branch.
 +
 See also INCOMPATIBLE OPTIONS below.
 

@@ -656,7 +653,7 @@ are incompatible with the following options:
  * --reapply-cherry-picks
  * --edit-todo
  * --update-refs
- * --root when used in combination with --onto
+ * --root when used without --onto
 
 In addition, the following pairs of options are incompatible:
 

index 9b9e78479f6..f86274990b0 100755
@@ -65,6 +65,10 @@ test_rebase_am_only () {
 		test_must_fail git rebase $opt --update-refs A
 	"
 
+	test_expect_success "$opt incompatible with --root without --onto" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --root A
+	"
 }
 
 # Check options which imply --apply

gitgitgadget

From: Elijah Newren <[email protected]>

The git-rebase manual noted several sets of incompatible options, but we were missing tests for a few of these. Further, we were missing code checks for some of these, which could result in command line options being silently ignored.

Also, note that adding a check for autosquash means that using --whitespace=fix together with the config setting rebase.autosquash=true will trigger an error. A subsequent commit will improve the error message.

---
 Documentation/git-rebase.txt           |  2 +-
 builtin/rebase.c                       | 30 ++++++++++++++++++++------
 t/t3422-rebase-incompatible-options.sh | 25 +++++++++++++++++++++
 3 files changed, 49 insertions(+), 8 deletions(-)

index 846aeed1b69..8cb075b2bdb 100644
@@ -650,7 +650,7 @@ are incompatible with the following options:
  * --exec
  * --no-keep-empty
  * --empty=
- * --reapply-cherry-picks
+ * --[no-]reapply-cherry-picks
  * --edit-todo
  * --update-refs
  * --root when used without --onto

index b742cc8fb5c..ed7475804cb 100644
@@ -1224,6 +1224,26 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		if (options.fork_point < 0)
 			options.fork_point = 0;
 	}
+	/*
+	 * The apply backend does not support --[no-]reapply-cherry-picks.
+	 * The behavior it implements by default is equivalent to
+	 * --no-reapply-cherry-picks (due to passing --cherry-picks to
+	 * format-patch), but --keep-base alters the upstream such that no
+	 * cherry-picks can be found (effectively making it act like
+	 * --reapply-cherry-picks).
+	 *
+	 * Now, if the user does specify --[no-]reapply-cherry-picks, but
+	 * does so in such a way that options.reapply_cherry_picks ==
+	 * keep_base, then the behavior they get will match what they
+	 * expect despite options.reapply_cherry_picks being ignored.  We
+	 * could just allow the flag in that case, but it seems better to
+	 * just alert the user that they've specified a flag that the
+	 * backend ignores.
+	 */
+	if (options.reapply_cherry_picks >= 0)
+		imply_merge(&options, options.reapply_cherry_picks ? "--reapply-cherry-picks" :
+								     "--no-reapply-cherry-picks");
+
 	/*
 	 * --keep-base defaults to --reapply-cherry-picks to avoid losing
 	 * commits when using this option.

@@ -1406,13 +1426,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	if (options.empty != EMPTY_UNSPECIFIED)
 		imply_merge(&options, "--empty");
 
-	/*
-	 * --keep-base implements --reapply-cherry-picks by altering upstream so
-	 * it works with both backends.
-	 */
-	if (options.reapply_cherry_picks && !keep_base)
-		imply_merge(&options, "--reapply-cherry-picks");
-
 	if (gpg_sign)
 		options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);
 

@@ -1503,6 +1516,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	if (options.update_refs)
 		imply_merge(&options, "--update-refs");
 
+	if (options.autosquash)
+		imply_merge(&options, "--autosquash");
+
 	if (options.type == REBASE_UNSPECIFIED) {
 		if (!strcmp(options.default_backend, "merge"))
 			imply_merge(&options, "--merge");

index f86274990b0..6a17b571ec7 100755
@@ -50,6 +50,11 @@ test_rebase_am_only () {
 		test_must_fail git rebase $opt --strategy-option=ours A
 	"
 
+	test_expect_success "$opt incompatible with --autosquash" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --autosquash A
+	"
+
 	test_expect_success "$opt incompatible with --interactive" "
 		git checkout B^0 &&
 		test_must_fail git rebase $opt --interactive A

@@ -60,6 +65,26 @@ test_rebase_am_only () {
 		test_must_fail git rebase $opt --exec 'true' A
 	"
 
+	test_expect_success "$opt incompatible with --keep-empty" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --keep-empty A
+	"
+
+	test_expect_success "$opt incompatible with --empty=..." "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --empty=ask A
+	"
+
+	test_expect_success "$opt incompatible with --no-reapply-cherry-picks" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --no-reapply-cherry-picks A
+	"
+
+	test_expect_success "$opt incompatible with --reapply-cherry-picks" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --reapply-cherry-picks A
+	"
+
 	test_expect_success "$opt incompatible with --update-refs" "
 		git checkout B^0 &&
 		test_must_fail git rebase $opt --update-refs A

gitgitgadget

From: Elijah Newren <[email protected]>

--edit-todo was documented as being incompatible with any of the options for the apply backend. However, it is also incompatible with any of the options for the merge backend, and is incompatible with any options that are not backend specific as well. The same can be said for --continue, --skip, --abort, --quit, etc.

This is already somewhat implicitly covered by the synopsis, but since "[<options>]" in the first two variants are vague it might be easy to miss this. That might not be a big deal, but since the rebase manpage has to spend so much verbiage about incompatibility of options, making a separate section for these options that are incompatible with everything else seems clearer. Do that, and remove the needless inclusion of --edit-todo in the explicit incompatibility list.

---
 Documentation/git-rebase.txt | 61 +++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 28 deletions(-)

index 8cb075b2bdb..1ba691e4c5f 100644
@@ -208,6 +208,39 @@ Alternatively, you can undo the 'git rebase' with
 
     git rebase --abort
 
+MODE OPTIONS
+------------
+
+The options in this section cannot be used with any other option,
+including not with each other:
+
+--continue::
+	Restart the rebasing process after having resolved a merge conflict.
+
+--skip::
+	Restart the rebasing process by skipping the current patch.
+
+--abort::
+	Abort the rebase operation and reset HEAD to the original
+	branch. If `<branch>` was provided when the rebase operation was
+	started, then `HEAD` will be reset to `<branch>`. Otherwise `HEAD`
+	will be reset to where it was when the rebase operation was
+	started.
+
+--quit::
+	Abort the rebase operation but `HEAD` is not reset back to the
+	original branch. The index and working tree are also left
+	unchanged as a result. If a temporary stash entry was created
+	using `--autostash`, it will be saved to the stash list.
+
+--edit-todo::
+	Edit the todo list during an interactive rebase.
+
+--show-current-patch::
+	Show the current patch in an interactive rebase or when rebase
+	is stopped because of conflicts. This is the equivalent of
+	`git show REBASE_HEAD`.
+
 OPTIONS
 -------
 --onto <newbase>::

@@ -249,22 +282,6 @@ See also INCOMPATIBLE OPTIONS below.
 <branch>::
 	Working branch; defaults to `HEAD`.
 
---continue::
-	Restart the rebasing process after having resolved a merge conflict.
-
---abort::
-	Abort the rebase operation and reset HEAD to the original
-	branch. If `<branch>` was provided when the rebase operation was
-	started, then `HEAD` will be reset to `<branch>`. Otherwise `HEAD`
-	will be reset to where it was when the rebase operation was
-	started.
-
---quit::
-	Abort the rebase operation but `HEAD` is not reset back to the
-	original branch. The index and working tree are also left
-	unchanged as a result. If a temporary stash entry was created
-	using `--autostash`, it will be saved to the stash list.
-
 --apply::
 	Use applying strategies to rebase (calling `git-am`
 	internally).  This option may become a no-op in the future

@@ -345,17 +362,6 @@ See also INCOMPATIBLE OPTIONS below.
 +
 See also INCOMPATIBLE OPTIONS below.
 
---skip::
-	Restart the rebasing process by skipping the current patch.
-
---edit-todo::
-	Edit the todo list during an interactive rebase.
-
---show-current-patch::
-	Show the current patch in an interactive rebase or when rebase
-	is stopped because of conflicts. This is the equivalent of
-	`git show REBASE_HEAD`.
-
 -m::
 --merge::
 	Using merging strategies to rebase (default).

@@ -651,7 +657,6 @@ are incompatible with the following options:
  * --no-keep-empty
  * --empty=
  * --[no-]reapply-cherry-picks
- * --edit-todo
  * --update-refs
  * --root when used without --onto
 

gitgitgadget

From: Elijah Newren <[email protected]>

Commit ce5238a690 ("rebase --keep-base: imply --reapply-cherry-picks", 2022-10-17) accidentally added some blank lines that cause extra paragraphs about --reapply-cherry-picks to be considered not part of the documentation of that option. Remove the blank lines to make it clear we are still discussing --reapply-cherry-picks.

---
 Documentation/git-rebase.txt | 2 --
 1 file changed, 2 deletions(-)

index 1ba691e4c5f..9f794f5bdcc 100644
@@ -338,7 +338,6 @@ See also INCOMPATIBLE OPTIONS below.
 	upstream changes, the behavior towards them is controlled by
 	the `--empty` flag.)
 +
-
 In the absence of `--keep-base` (or if `--no-reapply-cherry-picks` is
 given), these commits will be automatically dropped.  Because this
 necessitates reading all upstream commits, this can be expensive in

@@ -347,7 +346,6 @@ read. When using the 'merge' backend, warnings will be issued for each
 dropped commit (unless `--quiet` is given). Advice will also be issued
 unless `advice.skippedCherryPicks` is set to false (see
 linkgit:git-config[1]).
-
 +
 `--reapply-cherry-picks` allows rebase to forgo reading all upstream
 commits, potentially improving performance.

gitgitgadget

From: Elijah Newren <[email protected]>

---
 builtin/rebase.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index ed7475804cb..2358605b254 100644
@@ -134,6 +134,8 @@ struct rebase_options {
 		.exec = STRING_LIST_INIT_NODUP,		\
 		.git_format_patch_opt = STRBUF_INIT,	\
 		.fork_point = -1,			\
+		.reapply_cherry_picks = -1,             \
+		.allow_empty_message = 1,               \
 	}
 
 static struct replay_opts get_replay_opts(const struct rebase_options *opts)

@@ -1158,8 +1160,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	prepare_repo_settings(the_repository);
 	the_repository->settings.command_requires_full_index = 0;
 
-	options.reapply_cherry_picks = -1;
-	options.allow_empty_message = 1;
 	git_config(rebase_config, &options);
 	/* options.gpg_sign_opt will be either "-S" or NULL */
 	gpg_sign = options.gpg_sign_opt ? "" : NULL;

gitgitgadget

From: Elijah Newren <[email protected]>

When config which selects the merge backend (currently, rebase.autosquash=true or rebase.updateRefs=true) conflicts with other options on the command line (such as --whitespace=fix), make the error message specifically call out the config option and specify how to override that config option on the command line.

---
 Documentation/git-rebase.txt           |  2 +-
 builtin/rebase.c                       | 27 ++++++++++++++++++++------
 t/t3422-rebase-incompatible-options.sh | 24 +++++++++++++++++++++++
 3 files changed, 46 insertions(+), 7 deletions(-)

index 9f794f5bdcc..c357f6c3d5c 100644
@@ -648,7 +648,7 @@ are incompatible with the following options:
  * --merge
  * --strategy
  * --strategy-option
- * --[no-]autosquash
+ * --autosquash
  * --rebase-merges
  * --interactive
  * --exec

index 2358605b254..dd7e2c788f5 100644
@@ -122,6 +122,8 @@ struct rebase_options {
 	int reapply_cherry_picks;
 	int fork_point;
 	int update_refs;
+	int config_autosquash;
+	int config_update_refs;
 };
 
 #define REBASE_OPTIONS_INIT {			  	\

@@ -136,6 +138,10 @@ struct rebase_options {
 		.fork_point = -1,			\
 		.reapply_cherry_picks = -1,             \
 		.allow_empty_message = 1,               \
+		.autosquash = -1,                       \
+		.config_autosquash = -1,                \
+		.update_refs = -1,                      \
+		.config_update_refs = -1,               \
 	}
 
 static struct replay_opts get_replay_opts(const struct rebase_options *opts)

@@ -778,7 +784,7 @@ static int rebase_config(const char *var, const char *value, void *data)
 	}
 
 	if (!strcmp(var, "rebase.autosquash")) {
-		opts->autosquash = git_config_bool(var, value);
+		opts->config_autosquash = git_config_bool(var, value);
 		return 0;
 	}
 

@@ -795,7 +801,7 @@ static int rebase_config(const char *var, const char *value, void *data)
 	}
 
 	if (!strcmp(var, "rebase.updaterefs")) {
-		opts->update_refs = git_config_bool(var, value);
+		opts->config_update_refs = git_config_bool(var, value);
 		return 0;
 	}
 

@@ -1393,7 +1399,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	if ((options.flags & REBASE_INTERACTIVE_EXPLICIT) ||
 	    (options.action != ACTION_NONE) ||
 	    (options.exec.nr > 0) ||
-	    options.autosquash) {
+	    (options.autosquash == -1 && options.config_autosquash == 1) ||
+	    options.autosquash == 1) {
 		allow_preemptive_ff = 0;
 	}
 	if (options.committer_date_is_author_date || options.ignore_date)

@@ -1504,20 +1511,28 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			if (strcmp(options.git_am_opts.v[i], "-q"))
 				break;
 
-		if (i >= 0) {
+		if (i >= 0 || options.type == REBASE_APPLY) {
 			if (is_merge(&options))
 				die(_("apply options and merge options "
 					  "cannot be used together"));
+			else if (options.autosquash == -1 && options.config_autosquash == 1)
+				die(_("apply options are incompatible with rebase.autosquash.  Consider adding --no-autosquash"));
+			else if (options.update_refs == -1 && options.config_update_refs == 1)
+				die(_("apply options are incompatible with rebase.updateRefs.  Consider adding --no-update-refs"));
 			else
 				options.type = REBASE_APPLY;
 		}
 	}
 
-	if (options.update_refs)
+	if (options.update_refs == 1)
 		imply_merge(&options, "--update-refs");
+	options.update_refs = (options.update_refs >= 0) ? options.update_refs :
+			     ((options.config_update_refs >= 0) ? options.config_update_refs : 0);
 
-	if (options.autosquash)
+	if (options.autosquash == 1)
 		imply_merge(&options, "--autosquash");
+	options.autosquash = (options.autosquash >= 0) ? options.autosquash :
+			     ((options.config_autosquash >= 0) ? options.config_autosquash : 0);
 
 	if (options.type == REBASE_UNSPECIFIED) {
 		if (!strcmp(options.default_backend, "merge"))

index 6a17b571ec7..4711b37a288 100755
@@ -94,6 +94,30 @@ test_rebase_am_only () {
 		git checkout B^0 &&
 		test_must_fail git rebase $opt --root A
 	"
+
+	test_expect_success "$opt incompatible with rebase.autosquash" "
+		git checkout B^0 &&
+		test_must_fail git -c rebase.autosquash=true rebase $opt A 2>err &&
+		grep -e --no-autosquash err
+	"
+
+	test_expect_success "$opt incompatible with rebase.updateRefs" "
+		git checkout B^0 &&
+		test_must_fail git -c rebase.updateRefs=true rebase $opt A 2>err &&
+		grep -e --no-update-refs err
+	"
+
+	test_expect_success "$opt okay with overridden rebase.autosquash" "
+		test_when_finished \"git reset --hard B^0\" &&
+		git checkout B^0 &&
+		git -c rebase.autosquash=true rebase --no-autosquash $opt A
+	"
+
+	test_expect_success "$opt okay with overridden rebase.updateRefs" "
+		test_when_finished \"git reset --hard B^0\" &&
+		git checkout B^0 &&
+		git -c rebase.updateRefs=true rebase --no-update-refs $opt A
+	"
 }
 
 # Check options which imply --apply

gitgitgadget

Wow, this really expanded since I last looked at it. Thanks for taking on all of that extra work! (That was not my intention when recommending that you take over the fix.)

I really appreciate this extra attention to detail. I'm also really happy with how you implemented it, using different variables to signal how the option was specified (and using -1 for "unset" in both cases).

While I had not been following version 2 or 3, I read this version in its entirety and everything looked good to me. These improvements to our docs, tests, and implementation will be felt by users.

Thanks!
-Stolee

Hi Elijah

Thanks for updating the commit message and for the new commits at the end of the series.

	if (options.reapply_cherry_picks < 0)
		options.reapply_cherry_picks = keep_base;

Yeah, I know you were willing to let some of the work wait for some future series, and I intended to take that route. But...
* both you and Phillip brought up --autosquash, and it turns out we
didn't check it
* the above made me check the whole list of incompatibilities and
discover other missing checks
* adding tests made me discover that the documented
incompatibilities had a few issues
* you mentioned that an explicit --apply wasn't checked either, and
since I was already diving in I figured I might as well handle that too
* even though you mentioned the config fix wasn't needed, both Junio
and Phillip brought it up as well,
and based on the various feedback gotten so far, I started think
that just addressing it might require
less work than going through more back and forth on review of the
feature without that implementation.

Thanks for reading!

Hi Phillip,

options.reapply_cherry_picks is totally ignored by the apply backend, regardless of whether it's set by the user or the setup code in builtin/rebase.c. And if we have an option which is ignored, isn't it nicer to provide an error message to the user if they tried to set it?

Said another way, while users could start with these command lines:

    (Y) git rebase --whitespace=fix
    (Z) git rebase --whitespace=fix --keep-base

and modify them to include flags that would be ignored, we could allow:

    (A) git rebase --whitespace=fix --no-reapply-cherry-picks
    (B) git rebase --whitespace=fix --keep-base --reapply-cherry-picks

But we could not allow commands like

    (C) git rebase --whitespace=fix --reapply-cherry-picks
    (D) git rebase --whitespace=fix --keep-base --no-reapply-cherry-picks

For all four cases (A)-(D), the apply backend will ignore whatever --[no-]reapply-cherry-picks flag is provided. For (A) and (B), the behavior the apply backend provides happens to match what the user is requesting, while for (C) and (D) the behavior does not match. So we should at least reject (C) and (D). But, although we could technically allow (A) and (B), what advantage would it provide? I think the results of allowing those two commands would be:

1) Confusion by end users -- why should (C) & (D) throw errors if
(A) and (B) are accepted? That's not an easy rule to understand.

2) More confusion by end users -- the documentation for years has stated that --reapply-cherry-picks is incompatible with the apply backend, suggesting users would be surprised if at least (B) and probably (A) didn't throw error messages.

3) Confusing documentation -- If we don't want to throw errors for
(A) and (B), how do we modify the "INCOMPATIBLE OPTIONS" section of Documentation/git-rebase.txt to explain the relevant details of when these flags are (or are not) incompatible with the apply
backend? I think it'd end up with a very verbose explanation
that likely confuses more than it helps.

4) Excessively complicated code -- The previous attempts to implement this got it wrong. Prior to ce5238a690 ("rebase --keep-base: imply --reapply-cherry-picks", 2022-10-17), the code would error out on (B) and (C). After that commit, it would only error out on (C). Both solutions are incorrect since they miss (D), and I think the code just becomes hard to hard to follow in order to only error out on both (C) and (D) without (A) and (B).

(#2 and #3 might just be a repeat of the same issue, documentation, but it seemed easier to write separately.)

I think it's simpler for the code, for the documentation, and for end users to just error out on all of (A), (B), (C), and (D).
--[no-]reapply-cherry-picks is not supported by the apply backend.

But, given this lengthy email, perhaps I should split out the handling of --[no-]reapply-cherry-picks into its own commit and copy some or all of the description above into the commit message?

Hi Elijah

(C) is already an error
(D) is currently allowed and I think works as expected (--keep-base only implies --reapply-cherry-picks, the user is free to override that with --no-reapply-cherry-picks) so I don't see why we'd want to make it an error.

For (D) the flag is respected, (C) errors out, the other cases correspond to the default so it's like saying

	git rebase --merge --no-reapply-cherry-picks

ignores the flag. Arguably it is confusing that the apply backend only supports -[-no]-reapply-cherry-picks if --keep-base is given but I'm not sure that is a good reason to reject a combination that currently works as expected.

Best Wishes

Phillip

On reflection that is only true for (B). I agree that we should error out on (A) which we don't at the moment.

I'd support a change that errors out on (A) and (C) but continues to allow (B) and (D). I think we can do that with the diff below

Best Wishes

Phillip

index 1481c5b6a5..66aef356b8 100644
@@ -1230,12 +1230,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
                 if (options.fork_point < 0)
                         options.fork_point = 0;
         }
-        /*
-         * --keep-base defaults to --reapply-cherry-picks to avoid losing
-         * commits when using this option.
-         */
-        if (options.reapply_cherry_picks < 0)
-                options.reapply_cherry_picks = keep_base;
 
         if (options.root && options.fork_point > 0)
                 die(_("options '%s' and '%s' cannot be used 

together"), "--root", "--fork-point");
@@ -1412,11 +1406,17 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)

         if (options.empty != EMPTY_UNSPECIFIED)
                 imply_merge(&options, "--empty");

- /*

-         * --keep-base implements --reapply-cherry-picks by altering upstream so
-         * it works with both backends.
-         */
-        if (options.reapply_cherry_picks && !keep_base)
+        if (options.reapply_cherry_picks < 0)
+                /*
+                 * --keep-base defaults to --reapply-cherry-picks to
+                 * avoid losing commits when using this option.
+                 */
+                options.reapply_cherry_picks = keep_base;
+        else if (!keep_base)
+                /*
+                 * --keep-base implements --reapply-cherry-picks by
+                 * altering upstream so it works with both backends.
+                 */
                 imply_merge(&options, "--reapply-cherry-picks");

         if (gpg_sign)

Thanks, both of you, for well reasoned design work and respectful communication.

Hi Phillip,

Ah, despite looking over the code multiple times to check my statements, I somehow kept missing this:

    if (keep_base && options.reapply_cherry_picks)
        options.upstream = options.onto;

which is how --[no-]reapply-cherry-picks is supported in conjunction with --keep-base. Thanks.

         "--no-reapply-cherry-picks");

Hi Elijah

Clearly this comment has not been as helpful as I indented it to be. I think maybe we should spell out the defaults with and without --keep-base. Perhaps something like

default to --no-reapply-cherry-picks unless --keep-base is given in which case default to --reapply-cherry-picks

This has obviously proved to be confusing. The aim was to explain that in order to work with the apply backend "[--reapply-cherry-picks] --keep-base" was doing something unusual with upstream to reapply cherry picks. "--no-reapply-cherry-picks --keep-base" does not do anything unusual with upstream. I don't think changing it to --[no-]reapply-cherry-picks quite captures that. I came up with

To support --reapply-cherry-picks (which is not supported by the apply backend) --keep-base alters upstream to prevent cherry picked commits from being dropped.

but it really needs to mention that --keep-base also supports --no-reapply-cherry-picks in the usual way

That's a good idea

Yes, that's good

Thanks again for working on this

Phillip

[...]

I like that; sounds good to me.

Somewhat wordy, but perhaps:

/*
* The apply backend always searches for and drops cherry
* picks. This is often not wanted with --keep-base, so
* --keep-base allows --reapply-cherry-picks to be
* simulated by altering the upstream such that
* cherry-picks cannot be detected and thus all commits are
* reapplied. Thus, --[no-]reapply-cherry-picks is
* supported when --keep-base is specified, but not when
* --keep-base is left out.
*/

?

That sounds good to me, it is definitely an improvement on the current comment which I think is too terse.

Best Wishes

Phillip

We had a report about --update-refs being ignored when --whitespace=fix was passed, confusing an end user. These were already marked as incompatible in the manual, but the code didn't check for the incompatibility and provide an error to the user.

Folks brought up other flags tangentially when reviewing an earlier round of this series, and I found we had more that were missing checks, and that we were missing some testcases, and that the documentation was wrong on some of the relevant options. So, I ended up getting lots of little fixes to straighten these all out.

Changes since v4:

* Pulled out the changes regarding incompatibility detection for --[no-]reapply-cherry-picks into a separate patch. Phillip helped a lot with understanding the behavior, suggesting changes, and getting the wording right, and I think it deserves its own patch with its own explanation.

Changes since v3:

* Corrected the code surrounding --[no-]reapply-cherry-picks, and extended the testcases (Thanks to Phillip for pointing out my error)
* I went ahead and implemented the better error message when the merge backend is triggered solely via config options.

Changes since v2:

* Remove the extra patch and reword the comment in t3422 more thoroughly.
* Add tests for other flag incompatibilities that were missing
* Add code that was missing for checking various flag incompatibilities
* Fix incorrect claims in the documentation around incompatible options
* A few other miscellaneous fixups noticed while doing all the above

Changes since v1:

* Add a patch nuking the -C option to rebase (fixes confusion around the comment in t3422 and acknowledges the fact that the option is totally and utterly useless and always has been. It literally never affects the results of a rebase.)

Elijah Newren (10):
rebase: mark --update-refs as requiring the merge backend
rebase: flag --apply and --merge as incompatible
rebase: remove --allow-empty-message from incompatible opts
rebase: fix docs about incompatibilities with --root
rebase: fix incompatiblity checks for --[no-]reapply-cherry-picks
rebase: add coverage of other incompatible options
rebase: clarify the OPT_CMDMODE incompatibilities
rebase: fix formatting of rebase --reapply-cherry-picks option in docs
rebase: put rebase_options initialization in single place
rebase: provide better error message for apply options vs. merge
config

 Documentation/git-rebase.txt           | 77 ++++++++++++-------------
 builtin/rebase.c                       | 79 +++++++++++++++++++-------
 t/t3422-rebase-incompatible-options.sh | 71 +++++++++++++++++++++--
 3 files changed, 163 insertions(+), 64 deletions(-)

base-commit: 56c8fb1e95377900ec9d53c07886022af0a5d3c2
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1466%2Fnewren%2Frebase-update-refs-imply-merge-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1466/newren/rebase-update-refs-imply-merge-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1466

Range-diff vs v4:

  1:  8a676e6ec1a =  1:  8a676e6ec1a rebase: mark --update-refs as requiring the merge backend
  2:  cc129b87185 =  2:  cc129b87185 rebase: flag --apply and --merge as incompatible
  3:  9464bbbe9ba =  3:  9464bbbe9ba rebase: remove --allow-empty-message from incompatible opts
  4:  b702f15ed7c =  4:  b702f15ed7c rebase: fix docs about incompatibilities with --root
  -:  ----------- >  5:  3a8429f3d2b rebase: fix incompatiblity checks for --[no-]reapply-cherry-picks
  5:  5e4851e611e !  6:  2777dd2788a rebase: add coverage of other incompatible options

@@ Commit message

The git-rebase manual noted several sets of incompatible options, but we were missing tests for a few of these. Further, we were missing
- code checks for some of these, which could result in command line
+ code checks for one of these, which could result in command line
options being silently ignored.

Also, note that adding a check for autosquash means that using
@@ Commit message

- ## Documentation/git-rebase.txt ##
-@@ Documentation/git-rebase.txt: are incompatible with the following options:
- * --exec
- * --no-keep-empty
- * --empty=
-- * --reapply-cherry-picks
-+ * --[no-]reapply-cherry-picks
- * --edit-todo
- * --update-refs
- * --root when used without --onto
-
## builtin/rebase.c ##
-@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
- if (options.fork_point < 0)
- options.fork_point = 0;
- }
-+ /*
-+ * The apply backend does not support --[no-]reapply-cherry-picks.
-+ * The behavior it implements by default is equivalent to
-+ * --no-reapply-cherry-picks (due to passing --cherry-picks to
-+ * format-patch), but --keep-base alters the upstream such that no
-+ * cherry-picks can be found (effectively making it act like
-+ * --reapply-cherry-picks).
-+ *
-+ * Now, if the user does specify --[no-]reapply-cherry-picks, but
-+ * does so in such a way that options.reapply_cherry_picks ==
-+ * keep_base, then the behavior they get will match what they
-+ * expect despite options.reapply_cherry_picks being ignored. We
-+ * could just allow the flag in that case, but it seems better to
-+ * just alert the user that they've specified a flag that the
-+ * backend ignores.
-+ */
-+ if (options.reapply_cherry_picks >= 0)
-+ imply_merge(&options, options.reapply_cherry_picks ? "--reapply-cherry-picks" :
-+ "--no-reapply-cherry-picks");
-+
- /*
- * --keep-base defaults to --reapply-cherry-picks to avoid losing
- * commits when using this option.
-@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
- if (options.empty != EMPTY_UNSPECIFIED)
- imply_merge(&options, "--empty");
-
-- /*
-- * --keep-base implements --reapply-cherry-picks by altering upstream so
-- * it works with both backends.
-- */
-- if (options.reapply_cherry_picks && !keep_base)
-- imply_merge(&options, "--reapply-cherry-picks");
--
- if (gpg_sign)
- options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);
-
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
if (options.update_refs)
imply_merge(&options, "--update-refs");
@@ t/t3422-rebase-incompatible-options.sh: test_rebase_am_only () {
+ test_must_fail git rebase $opt --empty=ask A
+ "
+
-+ test_expect_success "$opt incompatible with --no-reapply-cherry-picks" "
-+ git checkout B^0 &&
-+ test_must_fail git rebase $opt --no-reapply-cherry-picks A
-+ "
-+
-+ test_expect_success "$opt incompatible with --reapply-cherry-picks" "
-+ git checkout B^0 &&
-+ test_must_fail git rebase $opt --reapply-cherry-picks A
-+ "
-+
- test_expect_success "$opt incompatible with --update-refs" "
+ test_expect_success "$opt incompatible with --no-reapply-cherry-picks" "
git checkout B^0 &&
- test_must_fail git rebase $opt --update-refs A
+ test_must_fail git rebase $opt --no-reapply-cherry-picks A
6: 21ae9e05313 ! 7: 0d0541ea243 rebase: clarify the OPT_CMDMODE incompatibilities
@@ Documentation/git-rebase.txt: See also INCOMPATIBLE OPTIONS below.
@@ Documentation/git-rebase.txt: are incompatible with the following options:
* --no-keep-empty
* --empty=
- * --[no-]reapply-cherry-picks
+ * --[no-]reapply-cherry-picks when used without --keep-base
- * --edit-todo
* --update-refs
* --root when used without --onto

  7:  74a216bf0c0 =  8:  01808cf84a8 rebase: fix formatting of rebase --reapply-cherry-picks option in docs
  8:  a8adf7fda61 =  9:  f646abee524 rebase: put rebase_options initialization in single place
  9:  5cb00e5103b = 10:  328f5a1d534 rebase: provide better error message for apply options vs. merge config

gitgitgadget

From: Elijah Newren <[email protected]>

--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 error now.

While at it, fix a typo in t3422...and fix some misleading wording (most options which used to be am-specific have since been implemented in the merge backend as well).

---
 Documentation/git-rebase.txt           |  2 ++
 builtin/rebase.c                       |  3 +++
 t/t3422-rebase-incompatible-options.sh | 15 ++++++++++-----
 3 files changed, 15 insertions(+), 5 deletions(-)

index d811c1cf443..6490bc96a15 100644
@@ -630,6 +630,8 @@ start would be overridden by the presence of
 +
 If the configuration variable `rebase.updateRefs` is set, then this option
 can be used to override and disable this setting.
++
+See also INCOMPATIBLE OPTIONS below.
 
 INCOMPATIBLE OPTIONS
 --------------------

index a26cc0cfdb5..c111b89e137 100644
@@ -1492,6 +1492,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");

index 6dabb05a2ad..9da39cd91c2 100755
@@ -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 couple options which are specific to the apply backend,
+# and several options which are specific to the merge backend.  Flags
+# from the different sets cannot work together, and we do not want to
+# just ignore one of the sets of flags.  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

gitgitgadget

From: Elijah Newren <[email protected]>

Previously, we flagged options which implied --apply as being incompatible with options which implied --merge. But if both options were given explicitly, then we didn't flag the incompatibility. The same is true with --apply and --interactive. Add the check, and add some testcases to verify these are also caught.

---
 builtin/rebase.c                       | 12 ++++++++++--
 t/t3422-rebase-incompatible-options.sh |  3 +++
 2 files changed, 13 insertions(+), 2 deletions(-)

index c111b89e137..b742cc8fb5c 100644
@@ -907,6 +907,9 @@ static int parse_opt_am(const struct option *opt, const char *arg, int unset)
 	BUG_ON_OPT_NEG(unset);
 	BUG_ON_OPT_ARG(arg);
 
+	if (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_APPLY)
+	    die(_("apply options and merge options cannot be used together"));
+
 	opts->type = REBASE_APPLY;
 
 	return 0;

@@ -920,8 +923,10 @@ static int parse_opt_merge(const struct option *opt, const char *arg, int unset)
 	BUG_ON_OPT_NEG(unset);
 	BUG_ON_OPT_ARG(arg);
 
-	if (!is_merge(opts))
-		opts->type = REBASE_MERGE;
+	if (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_MERGE)
+	    die(_("apply options and merge options cannot be used together"));
+
+	opts->type = REBASE_MERGE;
 
 	return 0;
 }

@@ -935,6 +940,9 @@ static int parse_opt_interactive(const struct option *opt, const char *arg,
 	BUG_ON_OPT_NEG(unset);
 	BUG_ON_OPT_ARG(arg);
 
+	if (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_MERGE)
+	    die(_("apply options and merge options cannot be used together"));
+
 	opts->type = REBASE_MERGE;
 	opts->flags |= REBASE_INTERACTIVE_EXPLICIT;
 

index 9da39cd91c2..9b9e78479f6 100755
@@ -67,7 +67,10 @@ test_rebase_am_only () {
 
 }
 
+# Check options which imply --apply
 test_rebase_am_only --whitespace=fix
 test_rebase_am_only -C4
+# Also check an explicit --apply
+test_rebase_am_only --apply
 
 test_done

gitgitgadget

From: Elijah Newren <[email protected]>

--allow-empty-message was turned into a no-op and even documented as such; the flag is simply ignored. Since the flag is ignored, it shouldn't be documented as being incompatible with other flags.

---
 Documentation/git-rebase.txt | 1 -
 1 file changed, 1 deletion(-)

index 6490bc96a15..7d01d1412d1 100644
@@ -647,7 +647,6 @@ are incompatible with the following options:
  * --merge
  * --strategy
  * --strategy-option
- * --allow-empty-message
  * --[no-]autosquash
  * --rebase-merges
  * --interactive

gitgitgadget

From: Elijah Newren <[email protected]>

In commit 5dacd4abdd ("git-rebase.txt: document incompatible options", 2018-06-25), I added notes about incompatibilities between options for the apply and merge backends. Unfortunately, I inverted the condition when --root was incompatible with the apply backend. Fix the documentation, and add a testcase that verifies the documentation matches the code.

While at it, the documentation for --root also tried to cover some of the backend differences between the apply and merge backends in relation to reapplying cherry picks. The information:
* assumed that the apply backend was the default (it isn't anymore)
* was written before --reapply-cherry-picks became an option
* was written before the detailed information on backend differences
All of these factors make the sentence under --root about reapplying cherry picks contradict information that is now available elsewhere in the manual, and the other references are correct. So just strike this sentence.

---
 Documentation/git-rebase.txt           | 7 ++-----
 t/t3422-rebase-incompatible-options.sh | 4 ++++
 2 files changed, 6 insertions(+), 5 deletions(-)

index 7d01d1412d1..846aeed1b69 100644
@@ -574,10 +574,7 @@ See also INCOMPATIBLE OPTIONS below.
 --root::
 	Rebase all commits reachable from `<branch>`, instead of
 	limiting them with an `<upstream>`.  This allows you to rebase
-	the root commit(s) on a branch.  When used with `--onto`, it
-	will skip changes already contained in `<newbase>` (instead of
-	`<upstream>`) whereas without `--onto` it will operate on every
-	change.
+	the root commit(s) on a branch.
 +
 See also INCOMPATIBLE OPTIONS below.
 

@@ -656,7 +653,7 @@ are incompatible with the following options:
  * --reapply-cherry-picks
  * --edit-todo
  * --update-refs
- * --root when used in combination with --onto
+ * --root when used without --onto
 
 In addition, the following pairs of options are incompatible:
 

index 9b9e78479f6..f86274990b0 100755
@@ -65,6 +65,10 @@ test_rebase_am_only () {
 		test_must_fail git rebase $opt --update-refs A
 	"
 
+	test_expect_success "$opt incompatible with --root without --onto" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --root A
+	"
 }
 
 # Check options which imply --apply

gitgitgadget

From: Elijah Newren <[email protected]>

--[no-]reapply-cherry-picks was traditionally only supported by the sequencer. Support was added for the apply backend, when --keep-base is also specified, in commit ce5238a690 ("rebase --keep-base: imply --reapply-cherry-picks", 2022-10-17). Make the code error out when --[no-]reapply-cherry-picks is specified AND the apply backend is used AND --keep-base is not specified. Also, clarify a number of comments surrounding the interaction of these flags.

---
 Documentation/git-rebase.txt           |  2 +-
 builtin/rebase.c                       | 34 ++++++++++++++++----------
 t/t3422-rebase-incompatible-options.sh | 10 ++++++++
 3 files changed, 32 insertions(+), 14 deletions(-)

index 846aeed1b69..8a09f12d897 100644
@@ -650,7 +650,7 @@ are incompatible with the following options:
  * --exec
  * --no-keep-empty
  * --empty=
- * --reapply-cherry-picks
+ * --[no-]reapply-cherry-picks when used without --keep-base
  * --edit-todo
  * --update-refs
  * --root when used without --onto

index b742cc8fb5c..05b130bfae5 100644
@@ -1224,13 +1224,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		if (options.fork_point < 0)
 			options.fork_point = 0;
 	}
-	/*
-	 * --keep-base defaults to --reapply-cherry-picks to avoid losing
-	 * commits when using this option.
-	 */
-	if (options.reapply_cherry_picks < 0)
-		options.reapply_cherry_picks = keep_base;
-
 	if (options.root && options.fork_point > 0)
 		die(_("options '%s' and '%s' cannot be used together"), "--root", "--fork-point");
 

@@ -1406,12 +1399,27 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	if (options.empty != EMPTY_UNSPECIFIED)
 		imply_merge(&options, "--empty");
 
-	/*
-	 * --keep-base implements --reapply-cherry-picks by altering upstream so
-	 * it works with both backends.
-	 */
-	if (options.reapply_cherry_picks && !keep_base)
-		imply_merge(&options, "--reapply-cherry-picks");
+	if (options.reapply_cherry_picks < 0)
+		/*
+		 * We default to --no-reapply-cherry-picks unless
+		 * --keep-base is given; when --keep-base is given, we want
+		 * to default to --reapply-cherry-picks.
+		 */
+		options.reapply_cherry_picks = keep_base;
+	else if (!keep_base)
+		/*
+		 * The apply backend always searches for and drops cherry
+		 * picks.  This is often not wanted with --keep-base, so
+		 * --keep-base allows --reapply-cherry-picks to be
+		 * simulated by altering the upstream such that
+		 * cherry-picks cannot be detected and thus all commits are
+		 * reapplied.  Thus, --[no-]reapply-cherry-picks is
+		 * supported when --keep-base is specified, but not when
+		 * --keep-base is left out.
+		 */
+		imply_merge(&options, options.reapply_cherry_picks ?
+					  "--reapply-cherry-picks" :
+					  "--no-reapply-cherry-picks");
 
 	if (gpg_sign)
 		options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);

index f86274990b0..826f3b94ae6 100755
@@ -60,6 +60,16 @@ test_rebase_am_only () {
 		test_must_fail git rebase $opt --exec 'true' A
 	"
 
+	test_expect_success "$opt incompatible with --no-reapply-cherry-picks" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --no-reapply-cherry-picks A
+	"
+
+	test_expect_success "$opt incompatible with --reapply-cherry-picks" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --reapply-cherry-picks A
+	"
+
 	test_expect_success "$opt incompatible with --update-refs" "
 		git checkout B^0 &&
 		test_must_fail git rebase $opt --update-refs A

gitgitgadget

From: Elijah Newren <[email protected]>

Commit ce5238a690 ("rebase --keep-base: imply --reapply-cherry-picks", 2022-10-17) accidentally added some blank lines that cause extra paragraphs about --reapply-cherry-picks to be considered not part of the documentation of that option. Remove the blank lines to make it clear we are still discussing --reapply-cherry-picks.

---
 Documentation/git-rebase.txt | 2 --
 1 file changed, 2 deletions(-)

index d2b731bd605..99aadac28ef 100644
@@ -338,7 +338,6 @@ See also INCOMPATIBLE OPTIONS below.
 	upstream changes, the behavior towards them is controlled by
 	the `--empty` flag.)
 +
-
 In the absence of `--keep-base` (or if `--no-reapply-cherry-picks` is
 given), these commits will be automatically dropped.  Because this
 necessitates reading all upstream commits, this can be expensive in

@@ -347,7 +346,6 @@ read. When using the 'merge' backend, warnings will be issued for each
 dropped commit (unless `--quiet` is given). Advice will also be issued
 unless `advice.skippedCherryPicks` is set to false (see
 linkgit:git-config[1]).
-
 +
 `--reapply-cherry-picks` allows rebase to forgo reading all upstream
 commits, potentially improving performance.

gitgitgadget

From: Elijah Newren <[email protected]>

The git-rebase manual noted several sets of incompatible options, but we were missing tests for a few of these. Further, we were missing code checks for one of these, which could result in command line options being silently ignored.

Also, note that adding a check for autosquash means that using --whitespace=fix together with the config setting rebase.autosquash=true will trigger an error. A subsequent commit will improve the error message.

---
 builtin/rebase.c                       |  3 +++
 t/t3422-rebase-incompatible-options.sh | 15 +++++++++++++++
 2 files changed, 18 insertions(+)

index 05b130bfae5..d6b20a6a536 100644
@@ -1511,6 +1511,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	if (options.update_refs)
 		imply_merge(&options, "--update-refs");
 
+	if (options.autosquash)
+		imply_merge(&options, "--autosquash");
+
 	if (options.type == REBASE_UNSPECIFIED) {
 		if (!strcmp(options.default_backend, "merge"))
 			imply_merge(&options, "--merge");

index 826f3b94ae6..6a17b571ec7 100755
@@ -50,6 +50,11 @@ test_rebase_am_only () {
 		test_must_fail git rebase $opt --strategy-option=ours A
 	"
 
+	test_expect_success "$opt incompatible with --autosquash" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --autosquash A
+	"
+
 	test_expect_success "$opt incompatible with --interactive" "
 		git checkout B^0 &&
 		test_must_fail git rebase $opt --interactive A

@@ -60,6 +65,16 @@ test_rebase_am_only () {
 		test_must_fail git rebase $opt --exec 'true' A
 	"
 
+	test_expect_success "$opt incompatible with --keep-empty" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --keep-empty A
+	"
+
+	test_expect_success "$opt incompatible with --empty=..." "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --empty=ask A
+	"
+
 	test_expect_success "$opt incompatible with --no-reapply-cherry-picks" "
 		git checkout B^0 &&
 		test_must_fail git rebase $opt --no-reapply-cherry-picks A

gitgitgadget

From: Elijah Newren <[email protected]>

--edit-todo was documented as being incompatible with any of the options for the apply backend. However, it is also incompatible with any of the options for the merge backend, and is incompatible with any options that are not backend specific as well. The same can be said for --continue, --skip, --abort, --quit, etc.

This is already somewhat implicitly covered by the synopsis, but since "[<options>]" in the first two variants are vague it might be easy to miss this. That might not be a big deal, but since the rebase manpage has to spend so much verbiage about incompatibility of options, making a separate section for these options that are incompatible with everything else seems clearer. Do that, and remove the needless inclusion of --edit-todo in the explicit incompatibility list.

---
 Documentation/git-rebase.txt | 61 +++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 28 deletions(-)

index 8a09f12d897..d2b731bd605 100644
@@ -208,6 +208,39 @@ Alternatively, you can undo the 'git rebase' with
 
     git rebase --abort
 
+MODE OPTIONS
+------------
+
+The options in this section cannot be used with any other option,
+including not with each other:
+
+--continue::
+	Restart the rebasing process after having resolved a merge conflict.
+
+--skip::
+	Restart the rebasing process by skipping the current patch.
+
+--abort::
+	Abort the rebase operation and reset HEAD to the original
+	branch. If `<branch>` was provided when the rebase operation was
+	started, then `HEAD` will be reset to `<branch>`. Otherwise `HEAD`
+	will be reset to where it was when the rebase operation was
+	started.
+
+--quit::
+	Abort the rebase operation but `HEAD` is not reset back to the
+	original branch. The index and working tree are also left
+	unchanged as a result. If a temporary stash entry was created
+	using `--autostash`, it will be saved to the stash list.
+
+--edit-todo::
+	Edit the todo list during an interactive rebase.
+
+--show-current-patch::
+	Show the current patch in an interactive rebase or when rebase
+	is stopped because of conflicts. This is the equivalent of
+	`git show REBASE_HEAD`.
+
 OPTIONS
 -------
 --onto <newbase>::

@@ -249,22 +282,6 @@ See also INCOMPATIBLE OPTIONS below.
 <branch>::
 	Working branch; defaults to `HEAD`.
 
---continue::
-	Restart the rebasing process after having resolved a merge conflict.
-
---abort::
-	Abort the rebase operation and reset HEAD to the original
-	branch. If `<branch>` was provided when the rebase operation was
-	started, then `HEAD` will be reset to `<branch>`. Otherwise `HEAD`
-	will be reset to where it was when the rebase operation was
-	started.
-
---quit::
-	Abort the rebase operation but `HEAD` is not reset back to the
-	original branch. The index and working tree are also left
-	unchanged as a result. If a temporary stash entry was created
-	using `--autostash`, it will be saved to the stash list.
-
 --apply::
 	Use applying strategies to rebase (calling `git-am`
 	internally).  This option may become a no-op in the future

@@ -345,17 +362,6 @@ See also INCOMPATIBLE OPTIONS below.
 +
 See also INCOMPATIBLE OPTIONS below.
 
---skip::
-	Restart the rebasing process by skipping the current patch.
-
---edit-todo::
-	Edit the todo list during an interactive rebase.
-
---show-current-patch::
-	Show the current patch in an interactive rebase or when rebase
-	is stopped because of conflicts. This is the equivalent of
-	`git show REBASE_HEAD`.
-
 -m::
 --merge::
 	Using merging strategies to rebase (default).

@@ -651,7 +657,6 @@ are incompatible with the following options:
  * --no-keep-empty
  * --empty=
  * --[no-]reapply-cherry-picks when used without --keep-base
- * --edit-todo
  * --update-refs
  * --root when used without --onto
 

gitgitgadget

From: Elijah Newren <[email protected]>

---
 builtin/rebase.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index d6b20a6a536..ad5ebecbbdb 100644
@@ -134,6 +134,8 @@ struct rebase_options {
 		.exec = STRING_LIST_INIT_NODUP,		\
 		.git_format_patch_opt = STRBUF_INIT,	\
 		.fork_point = -1,			\
+		.reapply_cherry_picks = -1,             \
+		.allow_empty_message = 1,               \
 	}
 
 static struct replay_opts get_replay_opts(const struct rebase_options *opts)

@@ -1158,8 +1160,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	prepare_repo_settings(the_repository);
 	the_repository->settings.command_requires_full_index = 0;
 
-	options.reapply_cherry_picks = -1;
-	options.allow_empty_message = 1;
 	git_config(rebase_config, &options);
 	/* options.gpg_sign_opt will be either "-S" or NULL */
 	gpg_sign = options.gpg_sign_opt ? "" : NULL;

gitgitgadget

From: Elijah Newren <[email protected]>

When config which selects the merge backend (currently, rebase.autosquash=true or rebase.updateRefs=true) conflicts with other options on the command line (such as --whitespace=fix), make the error message specifically call out the config option and specify how to override that config option on the command line.

---
 Documentation/git-rebase.txt           |  2 +-
 builtin/rebase.c                       | 27 ++++++++++++++++++++------
 t/t3422-rebase-incompatible-options.sh | 24 +++++++++++++++++++++++
 3 files changed, 46 insertions(+), 7 deletions(-)

index 99aadac28ef..9a295bcee45 100644
@@ -648,7 +648,7 @@ are incompatible with the following options:
  * --merge
  * --strategy
  * --strategy-option
- * --[no-]autosquash
+ * --autosquash
  * --rebase-merges
  * --interactive
  * --exec

index ad5ebecbbdb..7171be40eeb 100644
@@ -122,6 +122,8 @@ struct rebase_options {
 	int reapply_cherry_picks;
 	int fork_point;
 	int update_refs;
+	int config_autosquash;
+	int config_update_refs;
 };
 
 #define REBASE_OPTIONS_INIT {			  	\

@@ -136,6 +138,10 @@ struct rebase_options {
 		.fork_point = -1,			\
 		.reapply_cherry_picks = -1,             \
 		.allow_empty_message = 1,               \
+		.autosquash = -1,                       \
+		.config_autosquash = -1,                \
+		.update_refs = -1,                      \
+		.config_update_refs = -1,               \
 	}
 
 static struct replay_opts get_replay_opts(const struct rebase_options *opts)

@@ -778,7 +784,7 @@ static int rebase_config(const char *var, const char *value, void *data)
 	}
 
 	if (!strcmp(var, "rebase.autosquash")) {
-		opts->autosquash = git_config_bool(var, value);
+		opts->config_autosquash = git_config_bool(var, value);
 		return 0;
 	}
 

@@ -795,7 +801,7 @@ static int rebase_config(const char *var, const char *value, void *data)
 	}
 
 	if (!strcmp(var, "rebase.updaterefs")) {
-		opts->update_refs = git_config_bool(var, value);
+		opts->config_update_refs = git_config_bool(var, value);
 		return 0;
 	}
 

@@ -1366,7 +1372,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	if ((options.flags & REBASE_INTERACTIVE_EXPLICIT) ||
 	    (options.action != ACTION_NONE) ||
 	    (options.exec.nr > 0) ||
-	    options.autosquash) {
+	    (options.autosquash == -1 && options.config_autosquash == 1) ||
+	    options.autosquash == 1) {
 		allow_preemptive_ff = 0;
 	}
 	if (options.committer_date_is_author_date || options.ignore_date)

@@ -1499,20 +1506,28 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			if (strcmp(options.git_am_opts.v[i], "-q"))
 				break;
 
-		if (i >= 0) {
+		if (i >= 0 || options.type == REBASE_APPLY) {
 			if (is_merge(&options))
 				die(_("apply options and merge options "
 					  "cannot be used together"));
+			else if (options.autosquash == -1 && options.config_autosquash == 1)
+				die(_("apply options are incompatible with rebase.autosquash.  Consider adding --no-autosquash"));
+			else if (options.update_refs == -1 && options.config_update_refs == 1)
+				die(_("apply options are incompatible with rebase.updateRefs.  Consider adding --no-update-refs"));
 			else
 				options.type = REBASE_APPLY;
 		}
 	}
 
-	if (options.update_refs)
+	if (options.update_refs == 1)
 		imply_merge(&options, "--update-refs");
+	options.update_refs = (options.update_refs >= 0) ? options.update_refs :
+			     ((options.config_update_refs >= 0) ? options.config_update_refs : 0);
 
-	if (options.autosquash)
+	if (options.autosquash == 1)
 		imply_merge(&options, "--autosquash");
+	options.autosquash = (options.autosquash >= 0) ? options.autosquash :
+			     ((options.config_autosquash >= 0) ? options.config_autosquash : 0);
 
 	if (options.type == REBASE_UNSPECIFIED) {
 		if (!strcmp(options.default_backend, "merge"))

index 6a17b571ec7..4711b37a288 100755
@@ -94,6 +94,30 @@ test_rebase_am_only () {
 		git checkout B^0 &&
 		test_must_fail git rebase $opt --root A
 	"
+
+	test_expect_success "$opt incompatible with rebase.autosquash" "
+		git checkout B^0 &&
+		test_must_fail git -c rebase.autosquash=true rebase $opt A 2>err &&
+		grep -e --no-autosquash err
+	"
+
+	test_expect_success "$opt incompatible with rebase.updateRefs" "
+		git checkout B^0 &&
+		test_must_fail git -c rebase.updateRefs=true rebase $opt A 2>err &&
+		grep -e --no-update-refs err
+	"
+
+	test_expect_success "$opt okay with overridden rebase.autosquash" "
+		test_when_finished \"git reset --hard B^0\" &&
+		git checkout B^0 &&
+		git -c rebase.autosquash=true rebase --no-autosquash $opt A
+	"
+
+	test_expect_success "$opt okay with overridden rebase.updateRefs" "
+		test_when_finished \"git reset --hard B^0\" &&
+		git checkout B^0 &&
+		git -c rebase.updateRefs=true rebase --no-update-refs $opt A
+	"
 }
 
 # Check options which imply --apply

gitgitgadget

Hi Elijah

This looks great to me. Sorry it took me so long to realize that the apply backend should be erroring out on --no-reapply-cherry-picks without --keep-base. Thanks for taking the time to update the comments, hopefully they'll be more helpful to future readers that the current ones have proved to be.

Best Wishes

Phillip

Hi Elijah

This version looks good to me. Thanks for working on it - it turned out to be quite a bit of work in the end but it is nice improvement, especially the last patch.

Best Wishes

Phillip

Hmph, does this replace the previous round that has already been in 'next' for two days? I do not mind reverting the merge and requeuing it, but just wanted ot make sure before doing anything.

Thanks.

Sorry, I didn't realize it had merged to next. With Phillip's ongoing reviews and requests for changes, which you had even commented on
(https://lore.kernel.org/git/[email protected]/ and the thread that was in the middle of), I assumed it was still only in seen.

But yes, if you could revert from next and replace, that would be great. Now that v5 has Phillip's blessing (and multiple of his corrections), I think it's good to merge down.

Hi Elijah,

Thank you for working on this!

FWIW this report (and your patch series) made me wistful about that Google Summer of Code project that I hoped would bring about the trick to combine --whitespace=fix with interactive rebases. But in that GSoC project, this goal was treated as a "bonus feature" and pushed so far back that we did not even get to analyzing the complexity of the task, let alone any details.

So I sat down and started that analysis. The result is
https://github.com/gitgitgadget/git/issues/1472 where you can see that addressing the incompatibility is definitely outside of trivial. It does seem doable, if Outreachy/GSoC project-sized.

Ciao,
Johannes

Nice! Thanks for digging in and documenting what is needed!