[PATCH] rebase: mark --update-refs as requiring the merge backend

[PATCH] rebase: mark --update-refs as requiring the merge backend

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

--update-refs is built in terms of the sequencer, which requires the
merge backend.  It was already marked as incompatible with the apply
backend in the git-rebase manual, but the code didn't check for this
incompatibility and warn the user.  Check and warn now.

While at it, fix a typo in t3422...and fix some misleading wording (all
useful options other than --whitespace=fix have long since been
implemented in the merge backend).

Signed-off-by: Elijah Newren <newren@gmail.com>
---
    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).
    
    Signed-off-by: Elijah Newren newren@gmail.com

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

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 1481c5b6a5b..accd62fce48 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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");
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 6dabb05a2ad..5a00618d265 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -25,11 +25,11 @@ test_expect_success 'setup' '
 '
 
 #
-# Rebase has lots of useful options like --whitepsace=fix, which are
-# actually all built in terms of flags to git-am.  Since neither
-# --merge nor --interactive (nor any options that imply those two) use
-# git-am, using them together will result in flags like --whitespace=fix
-# being ignored.  Make sure rebase warns the user and aborts instead.
+# Rebase has a useful option, --whitespace=fix, which is actually
+# built in terms of flags to git-am.  Since neither --merge nor
+# --interactive (nor any options that imply those two) use git-am,
+# using them together will result in --whitespace=fix being ignored.
+# Make sure rebase warns the user and aborts instead.
 #
 
 test_rebase_am_only () {
@@ -60,6 +60,11 @@ test_rebase_am_only () {
 		test_must_fail git rebase $opt --exec 'true' A
 	"
 
+	test_expect_success "$opt incompatible with --update-refs" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --update-refs A
+	"
+
 }
 
 test_rebase_am_only --whitespace=fix

base-commit: 2b4f5a4e4bb102ac8d967cea653ed753b608193c
-- 
gitgitgadget

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

[Derrick Stolee]

On 1/19/2023 12:36 AM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> --update-refs is built in terms of the sequencer, which requires the
> merge backend.  It was already marked as incompatible with the apply
> backend in the git-rebase manual, but the code didn't check for this
> incompatibility and warn the user.  Check and warn now.

Thank you for submitting this version.

> @@ -1514,6 +1514,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  		}
>  	}
>  
> +	if (options.update_refs)
> +		imply_merge(&options, "--update-refs");
> +

This solution is very elegant. The only downside is the lack of warning
if --update-refs was implied by rebase.updateRefs=true, but I'm happy to
delay implementing that warning in favor of your complete solution here.

Thinking ahead to that option, are there other options that are implied
by config that are required in imply_merge()? Is --autosquash in that
camp? If so, then maybe it would make sense to expand imply_merge() to
include a boolean config key and supply that warning within its
implementation. (This consideration should not block this patch, as it
is complete as-is.)

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

Thanks for the update here. The -C option is also used in this test,
so --whitespace=fix isn't the only option, right? At least, -C doesn't
make sense to port over to the merge backend, so maybe that's what
you mean by --whitespace=fix being the last one?

The user could also explicitly request the apply backend with --apply,
but this test script doesn't check it, strangely. That seems like an
oversight, but not a drawback to your patch.

>  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
> +	"
> +

Thanks for adding this test. I would delay the rebase.updateRefs
version until there is extra behavior to check, such as the
warning message.

Thanks,
-Stolee

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

[Elijah Newren]

On Thu, Jan 19, 2023 at 1:47 PM Derrick Stolee <derrickstolee@github.com> wrote:
>
> On 1/19/2023 12:36 AM, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > --update-refs is built in terms of the sequencer, which requires the
> > merge backend.  It was already marked as incompatible with the apply
> > backend in the git-rebase manual, but the code didn't check for this
> > incompatibility and warn the user.  Check and warn now.
>
> Thank you for submitting this version.
>
> > @@ -1514,6 +1514,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> >               }
> >       }
> >
> > +     if (options.update_refs)
> > +             imply_merge(&options, "--update-refs");
> > +
>
> This solution is very elegant. The only downside is the lack of warning
> if --update-refs was implied by rebase.updateRefs=true, but I'm happy to
> delay implementing that warning in favor of your complete solution here.
>
> Thinking ahead to that option, are there other options that are implied
> by config that are required in imply_merge()? Is --autosquash in that
> camp? If so, then maybe it would make sense to expand imply_merge() to
> include a boolean config key and supply that warning within its
> implementation. (This consideration should not block this patch, as it
> is complete as-is.)

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

> > 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 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.
> >  #
>
> Thanks for the update here. The -C option is also used in this test,
> so --whitespace=fix isn't the only option, right? At least, -C doesn't
> make sense to port over to the merge backend, so maybe that's what
> you mean by --whitespace=fix being the last one?

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.

> The user could also explicitly request the apply backend with --apply,
> but this test script doesn't check it, strangely. That seems like an
> oversight, but not a drawback to your patch.
>
> >  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
> > +     "
> > +
>
> Thanks for adding this test. I would delay the rebase.updateRefs
> version until there is extra behavior to check, such as the
> warning message.
>
> Thanks,
> -Stolee

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

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

>> +	if (options.update_refs)
>> +		imply_merge(&options, "--update-refs");
>> +
>
> This solution is very elegant. The only downside is the lack of warning
> if --update-refs was implied by rebase.updateRefs=true, but I'm happy to
> delay implementing that warning in favor of your complete solution here.

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.

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

[Elijah Newren]

On Fri, Jan 20, 2023 at 7:27 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Derrick Stolee <derrickstolee@github.com> writes:
>
> >> +    if (options.update_refs)
> >> +            imply_merge(&options, "--update-refs");
> >> +
> >
> > This solution is very elegant. The only downside is the lack of warning
> > if --update-refs was implied by rebase.updateRefs=true, but I'm happy to
> > delay implementing that warning in favor of your complete solution here.
>
> 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 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?

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

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.

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

[Elijah Newren via GitGitGadget]

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

Signed-off-by: Elijah Newren newren@gmail.com

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

[PATCH v2 1/2] rebase: remove completely useless -C option

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 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(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index f9675bd24e6..412887deda7 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -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:
 
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 1481c5b6a5b..ace8bd4a41c 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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);
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index ceca1600053..c8dca845dd7 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -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
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 6dabb05a2ad..ebcbd79ab8a 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -63,6 +63,5 @@ test_rebase_am_only () {
 }
 
 test_rebase_am_only --whitespace=fix
-test_rebase_am_only -C4
 
 test_done
-- 
gitgitgadget

Re: [PATCH v2 1/2] rebase: remove completely useless -C option

[Eric Sunshine]

On Fri, Jan 20, 2023 at 12:14 AM Elijah Newren via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>

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.

Re: [PATCH v2 1/2] rebase: remove completely useless -C option

[Elijah Newren]

On Thu, Jan 19, 2023 at 9:40 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Fri, Jan 20, 2023 at 12:14 AM Elijah Newren via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > 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.
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
>
> 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).

Re: [PATCH v2 1/2] rebase: remove completely useless -C option

[Martin Ågren]

On Fri, 20 Jan 2023 at 06:27, Elijah Newren via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> 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.

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

Re: [PATCH v2 1/2] rebase: remove completely useless -C option

[Elijah Newren]

On Fri, Jan 20, 2023 at 1:55 AM Martin Ågren <martin.agren@gmail.com> wrote:
>
> On Fri, 20 Jan 2023 at 06:27, Elijah Newren via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > 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.
>
> 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

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.

Re: [PATCH v2 1/2] rebase: remove completely useless -C option

[Junio C Hamano]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Elijah Newren <newren@gmail.com>
>
> The `-C` option to rebase was introduced with 67dad687ad ("add -C[NUM]
> to git-am", 2007-02-08).
> ...
> 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.

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 <mst@kernel.org>
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.
    
    Signed-off-by: Michael S. Tsirkin <mst@mellanox.co.il>
    Signed-off-by: Junio C Hamano <junkio@cox.net>

Re: [PATCH v2 1/2] rebase: remove completely useless -C option

[Elijah Newren]

On Fri, Jan 20, 2023 at 4:05 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > The `-C` option to rebase was introduced with 67dad687ad ("add -C[NUM]
> > to git-am", 2007-02-08).
> > ...
> > 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.
>
> 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?

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.

Re: [PATCH v2 1/2] rebase: remove completely useless -C option

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> On Fri, Jan 20, 2023 at 4:05 AM Junio C Hamano <gitster@pobox.com> wrote:
>> ...
>> 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?
>
> 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.

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

Re: [PATCH v2 1/2] rebase: remove completely useless -C option

[Elijah Newren]

On Fri, Jan 20, 2023 at 8:16 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > On Fri, Jan 20, 2023 at 4:05 AM Junio C Hamano <gitster@pobox.com> wrote:
> >> ...
> >> 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?
> >
> > 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.
>
> 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 ;-)

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/xmqqa78d2qmk.fsf@gitster-ct.c.googlers.com/

Re: [PATCH v2 1/2] rebase: remove completely useless -C option

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

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

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

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

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

--update-refs is built in terms of the sequencer, which requires the
merge backend.  It was already marked as incompatible with the apply
backend in the git-rebase manual, but the code didn't check for this
incompatibility and warn the user.  Check and warn now.

While at it, fix a typo in t3422...and fix some misleading wording (all
useful options other than --whitespace=fix have long since been
implemented in the merge backend).

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/rebase.c                       |  3 +++
 t/t3422-rebase-incompatible-options.sh | 15 ++++++++++-----
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index ace8bd4a41c..e8bcdbf9fcd 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1507,6 +1507,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		}
 	}
 
+	if (options.update_refs)
+		imply_merge(&options, "--update-refs");
+
 	if (options.type == REBASE_UNSPECIFIED) {
 		if (!strcmp(options.default_backend, "merge"))
 			imply_merge(&options, "--merge");
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index ebcbd79ab8a..d72c863b21b 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -25,11 +25,11 @@ test_expect_success 'setup' '
 '
 
 #
-# Rebase has lots of useful options like --whitepsace=fix, which are
-# actually all built in terms of flags to git-am.  Since neither
-# --merge nor --interactive (nor any options that imply those two) use
-# git-am, using them together will result in flags like --whitespace=fix
-# being ignored.  Make sure rebase warns the user and aborts instead.
+# Rebase has a useful option, --whitespace=fix, which is actually
+# built in terms of flags to git-am.  Since neither --merge nor
+# --interactive (nor any options that imply those two) use git-am,
+# using them together will result in --whitespace=fix being ignored.
+# Make sure rebase warns the user and aborts instead.
 #
 
 test_rebase_am_only () {
@@ -60,6 +60,11 @@ test_rebase_am_only () {
 		test_must_fail git rebase $opt --exec 'true' A
 	"
 
+	test_expect_success "$opt incompatible with --update-refs" "
+		git checkout B^0 &&
+		test_must_fail git rebase $opt --update-refs A
+	"
+
 }
 
 test_rebase_am_only --whitespace=fix
-- 
gitgitgadget

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

[Phillip Wood]

Hi Elijah

Thanks for working on this

On 20/01/2023 04:56, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> --update-refs is built in terms of the sequencer, which requires the
> merge backend.  It was already marked as incompatible with the apply
> backend in the git-rebase manual, but the code didn't check for this
> incompatibility and warn the user.  Check and warn now.

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

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

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

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

Best Wishes

Phillip

> While at it, fix a typo in t3422...and fix some misleading wording (all
> useful options other than --whitespace=fix have long since been
> implemented in the merge backend).
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>   builtin/rebase.c                       |  3 +++
>   t/t3422-rebase-incompatible-options.sh | 15 ++++++++++-----
>   2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index ace8bd4a41c..e8bcdbf9fcd 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1507,6 +1507,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   		}
>   	}
>   
> +	if (options.update_refs)
> +		imply_merge(&options, "--update-refs");
> +
>   	if (options.type == REBASE_UNSPECIFIED) {
>   		if (!strcmp(options.default_backend, "merge"))
>   			imply_merge(&options, "--merge");
> diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
> index ebcbd79ab8a..d72c863b21b 100755
> --- a/t/t3422-rebase-incompatible-options.sh
> +++ b/t/t3422-rebase-incompatible-options.sh
> @@ -25,11 +25,11 @@ test_expect_success 'setup' '
>   '
>   
>   #
> -# Rebase has lots of useful options like --whitepsace=fix, which are
> -# actually all built in terms of flags to git-am.  Since neither
> -# --merge nor --interactive (nor any options that imply those two) use
> -# git-am, using them together will result in flags like --whitespace=fix
> -# being ignored.  Make sure rebase warns the user and aborts instead.
> +# Rebase has a useful option, --whitespace=fix, which is actually
> +# built in terms of flags to git-am.  Since neither --merge nor
> +# --interactive (nor any options that imply those two) use git-am,
> +# using them together will result in --whitespace=fix being ignored.
> +# Make sure rebase warns the user and aborts instead.
>   #
>   
>   test_rebase_am_only () {
> @@ -60,6 +60,11 @@ test_rebase_am_only () {
>   		test_must_fail git rebase $opt --exec 'true' A
>   	"
>   
> +	test_expect_success "$opt incompatible with --update-refs" "
> +		git checkout B^0 &&
> +		test_must_fail git rebase $opt --update-refs A
> +	"
> +
>   }
>   
>   test_rebase_am_only --whitespace=fix

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

[Elijah Newren]

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

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

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

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

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

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

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

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

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

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


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

[PATCH v3 0/7] rebase: fix several code/testing/documentation issues around flag incompatibilities

[Elijah Newren via GitGitGadget]

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/xmqqa78d2qmk.fsf@gitster-ct.c.googlers.com/

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

Signed-off-by: Elijah Newren newren@gmail.com

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).
      
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
     + ## 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 <newren@gmail.com>
      
       ## 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.
      
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
     - ## 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

[PATCH v3 1/7] rebase: mark --update-refs as requiring the merge backend

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

--update-refs is built in terms of the sequencer, which requires the
merge backend.  It was already marked as incompatible with the apply
backend in the git-rebase manual, but the code didn't check for this
incompatibility and warn the user.  Check and 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).

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt           |  2 ++
 builtin/rebase.c                       |  3 +++
 t/t3422-rebase-incompatible-options.sh | 15 ++++++++++-----
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index f9675bd24e6..00d21d7287d 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -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
 --------------------
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 1481c5b6a5b..accd62fce48 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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");
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 6dabb05a2ad..9da39cd91c2 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -25,11 +25,11 @@ test_expect_success 'setup' '
 '
 
 #
-# Rebase has lots of useful options like --whitepsace=fix, which are
-# actually all built in terms of flags to git-am.  Since neither
-# --merge nor --interactive (nor any options that imply those two) use
-# git-am, using them together will result in flags like --whitespace=fix
-# being ignored.  Make sure rebase warns the user and aborts instead.
+# Rebase has a 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

[PATCH v3 2/7] rebase: flag --apply and --merge as incompatible

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/rebase.c                       | 12 ++++++++++--
 t/t3422-rebase-incompatible-options.sh |  3 +++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index accd62fce48..2a5e0e8a7a0 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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;
 
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 9da39cd91c2..9b9e78479f6 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -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

[PATCH v3 3/7] rebase: remove --allow-empty-message from incompatible opts

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt | 1 -
 1 file changed, 1 deletion(-)

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

Re: [PATCH v3 3/7] rebase: remove --allow-empty-message from incompatible opts

[Phillip Wood]

Hi Elijah

On 21/01/2023 01:55, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> --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.

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

> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>   Documentation/git-rebase.txt | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 00d21d7287d..3929535c0cd 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -640,7 +640,6 @@ are incompatible with the following options:
>    * --merge
>    * --strategy
>    * --strategy-option
> - * --allow-empty-message
>    * --[no-]autosquash
>    * --rebase-merges
>    * --interactive

[PATCH v3 4/7] rebase: fix docs about incompatibilities with --root

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt           | 7 ++-----
 t/t3422-rebase-incompatible-options.sh | 4 ++++
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 3929535c0cd..3f12d242d83 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -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:
 
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 9b9e78479f6..f86274990b0 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -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

[PATCH v3 5/7] rebase: add coverage of other incompatible options

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/rebase.c                       | 21 ++++++++++++++-------
 t/t3422-rebase-incompatible-options.sh | 20 ++++++++++++++++++++
 2 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 2a5e0e8a7a0..6dcdb59bb02 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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");
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index f86274990b0..c830025470f 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -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

Re: [PATCH v3 5/7] rebase: add coverage of other incompatible options

[Phillip Wood]

Hi Elijah

On 21/01/2023 01:55, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> 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.
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>   builtin/rebase.c                       | 21 ++++++++++++++-------
>   t/t3422-rebase-incompatible-options.sh | 20 ++++++++++++++++++++
>   2 files changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 2a5e0e8a7a0..6dcdb59bb02 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -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,

I think it is hard coded to 0 not 1. We generate the patches with

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

so cherry-picks will not be reapplied. I'm hardly an impartial observer 
but I think the current check is correct. We support

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

but not

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

> @@ -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");

Thanks for adding this, it maybe merits a mention in the commit message 
though as it is a change in behavior for users who have 
rebase.autosquash set and try to use the apply backend.

Best Wishes

Phillip

>   	if (options.type == REBASE_UNSPECIFIED) {
>   		if (!strcmp(options.default_backend, "merge"))
>   			imply_merge(&options, "--merge");
> diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
> index f86274990b0..c830025470f 100755
> --- a/t/t3422-rebase-incompatible-options.sh
> +++ b/t/t3422-rebase-incompatible-options.sh
> @@ -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

Re: [PATCH v3 5/7] rebase: add coverage of other incompatible options

[Phillip Wood]

On 21/01/2023 15:20, Phillip Wood wrote:
>> @@ -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,
> 
> I think it is hard coded to 0 not 1. We generate the patches with
> 
>      git format-patch --right-only $upstream...$head

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

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

Phillip


> so cherry-picks will not be reapplied. I'm hardly an impartial observer 
> but I think the current check is correct. We support
> 
>      --whitespace=fix --no-reapply-cherry-picks
>      --whitespace=fix --keep-base --reapply-cherry-picks
> 
> but not
> 
>      --whitespace=fix --reapply-cherry-picks
> 
>> @@ -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");
> 
> Thanks for adding this, it maybe merits a mention in the commit message 
> though as it is a change in behavior for users who have 
> rebase.autosquash set and try to use the apply backend.
> 
> Best Wishes
> 
> Phillip
> 
>>       if (options.type == REBASE_UNSPECIFIED) {
>>           if (!strcmp(options.default_backend, "merge"))
>>               imply_merge(&options, "--merge");
>> diff --git a/t/t3422-rebase-incompatible-options.sh 
>> b/t/t3422-rebase-incompatible-options.sh
>> index f86274990b0..c830025470f 100755
>> --- a/t/t3422-rebase-incompatible-options.sh
>> +++ b/t/t3422-rebase-incompatible-options.sh
>> @@ -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

Re: [PATCH v3 5/7] rebase: add coverage of other incompatible options

[Elijah Newren]

On Sat, Jan 21, 2023 at 11:25 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> On 21/01/2023 15:20, Phillip Wood wrote:
> >> @@ -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,
> >
> > I think it is hard coded to 0 not 1. We generate the patches with
> >
> >      git format-patch --right-only $upstream...$head
>
> Sorry I somhow managed to omit --cherry-pick from that, it should have been
>
>         git format-patch --right-only --cherry-pick $upstream...$head
>
> Phillip

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.

> > so cherry-picks will not be reapplied. I'm hardly an impartial observer
> > but I think the current check is correct. We support
> >
> >      --whitespace=fix --no-reapply-cherry-picks
> >      --whitespace=fix --keep-base --reapply-cherry-picks
> >
> > but not
> >
> >      --whitespace=fix --reapply-cherry-picks

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.

[PATCH v3 6/7] rebase: clarify the OPT_CMDMODE incompatibilities

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt | 61 +++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 28 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 3f12d242d83..00a9e22bc32 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -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

[PATCH v3 7/7] rebase: fix formatting of rebase --reapply-cherry-picks option in docs

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt | 2 --
 1 file changed, 2 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 00a9e22bc32..140c984d0ea 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -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

Re: [PATCH v3 7/7] rebase: fix formatting of rebase --reapply-cherry-picks option in docs

[Phillip Wood]

Hi Elijah

On 21/01/2023 01:55, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> 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.

Thanks for clearing up my mess!

Phillip

> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>   Documentation/git-rebase.txt | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 00a9e22bc32..140c984d0ea 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -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.

[PATCH v4 0/9] rebase: fix several code/testing/documentation issues around flag incompatibilities

[Elijah Newren via GitGitGadget]

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

Signed-off-by: Elijah Newren newren@gmail.com

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.
     +
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
     + ## 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

[PATCH v4 1/9] rebase: mark --update-refs as requiring the merge backend

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

--update-refs is built in terms of the sequencer, which requires the
merge backend.  It was already marked as incompatible with the apply
backend in the git-rebase manual, but the code didn't check for this
incompatibility and warn the user.  Check and 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).

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt           |  2 ++
 builtin/rebase.c                       |  3 +++
 t/t3422-rebase-incompatible-options.sh | 15 ++++++++++-----
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index d811c1cf443..6490bc96a15 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -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
 --------------------
diff --git a/builtin/rebase.c b/builtin/rebase.c
index a26cc0cfdb5..c111b89e137 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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");
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 6dabb05a2ad..9da39cd91c2 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -25,11 +25,11 @@ test_expect_success 'setup' '
 '
 
 #
-# Rebase has lots of useful options like --whitepsace=fix, which are
-# actually all built in terms of flags to git-am.  Since neither
-# --merge nor --interactive (nor any options that imply those two) use
-# git-am, using them together will result in flags like --whitespace=fix
-# being ignored.  Make sure rebase warns the user and aborts instead.
+# Rebase has a 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

[PATCH v4 2/9] rebase: flag --apply and --merge as incompatible

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/rebase.c                       | 12 ++++++++++--
 t/t3422-rebase-incompatible-options.sh |  3 +++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index c111b89e137..b742cc8fb5c 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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;
 
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 9da39cd91c2..9b9e78479f6 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -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

[PATCH v4 3/9] rebase: remove --allow-empty-message from incompatible opts

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt | 1 -
 1 file changed, 1 deletion(-)

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

[PATCH v4 4/9] rebase: fix docs about incompatibilities with --root

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt           | 7 ++-----
 t/t3422-rebase-incompatible-options.sh | 4 ++++
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 7d01d1412d1..846aeed1b69 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -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:
 
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 9b9e78479f6..f86274990b0 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -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

[PATCH v4 5/9] rebase: add coverage of other incompatible options

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt           |  2 +-
 builtin/rebase.c                       | 30 ++++++++++++++++++++------
 t/t3422-rebase-incompatible-options.sh | 25 +++++++++++++++++++++
 3 files changed, 49 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 846aeed1b69..8cb075b2bdb 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -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
diff --git a/builtin/rebase.c b/builtin/rebase.c
index b742cc8fb5c..ed7475804cb 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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");
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index f86274990b0..6a17b571ec7 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -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

Re: [PATCH v4 5/9] rebase: add coverage of other incompatible options

[Phillip Wood]

Hi Elijah

On 22/01/2023 06:12, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> 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.

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

> Signed-off-by: Elijah Newren <newren@gmail.com>
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -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.
> +	 */

I'm a bit confused by this. --keep-base works with either 
--reapply-cherry-picks (which is the default if --keep-base is given) or 
--no-reapply-cherry-picks. Just below this hunk we have

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

So we only set options.reapply_cherry_picks to match keep_base if the 
user did not specify -[-no]-reapply-cherry-picks on the commandline.

Best Wishes

Phillip

> +	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");
> diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
> index f86274990b0..6a17b571ec7 100755
> --- a/t/t3422-rebase-incompatible-options.sh
> +++ b/t/t3422-rebase-incompatible-options.sh
> @@ -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

Re: [PATCH v4 5/9] rebase: add coverage of other incompatible options

[Elijah Newren]

Hi Phillip,

On Mon, Jan 23, 2023 at 12:08 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Elijah
>
> On 22/01/2023 06:12, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > 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.
>
> Thanks for updating the commit message and for the new commits at the
> end of the series.
>
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -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.
> > +      */
>
> I'm a bit confused by this. --keep-base works with either
> --reapply-cherry-picks (which is the default if --keep-base is given) or
> --no-reapply-cherry-picks. Just below this hunk we have
>
>         if (options.reapply_cherry_picks < 0)
>                 options.reapply_cherry_picks = keep_base;
>
> So we only set options.reapply_cherry_picks to match keep_base if the
> user did not specify -[-no]-reapply-cherry-picks on the commandline.

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?

Re: [PATCH v4 5/9] rebase: add coverage of other incompatible options

[Phillip Wood]

Hi Elijah

On 24/01/2023 02:36, Elijah Newren wrote:
> Hi Phillip,
> 
> On Mon, Jan 23, 2023 at 12:08 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> Hi Elijah
>>
>> On 22/01/2023 06:12, Elijah Newren via GitGitGadget wrote:
>>> From: Elijah Newren <newren@gmail.com>
>>>
>>> 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.
>>
>> Thanks for updating the commit message and for the new commits at the
>> end of the series.
>>
>>> Signed-off-by: Elijah Newren <newren@gmail.com>
>>> --- a/builtin/rebase.c
>>> +++ b/builtin/rebase.c
>>> @@ -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.
>>> +      */
>>
>> I'm a bit confused by this. --keep-base works with either
>> --reapply-cherry-picks (which is the default if --keep-base is given) or
>> --no-reapply-cherry-picks. Just below this hunk we have
>>
>>          if (options.reapply_cherry_picks < 0)
>>                  options.reapply_cherry_picks = keep_base;
>>
>> So we only set options.reapply_cherry_picks to match keep_base if the
>> user did not specify -[-no]-reapply-cherry-picks on the commandline.
> 
> 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

(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 all four cases (A)-(D), the apply backend will ignore whatever
> --[no-]reapply-cherry-picks flag is provided.

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

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

Re: [PATCH v4 5/9] rebase: add coverage of other incompatible options

[Phillip Wood]

>>>> Signed-off-by: Elijah Newren <newren@gmail.com>
>>>> --- a/builtin/rebase.c
>>>> +++ b/builtin/rebase.c
>>>> @@ -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.
>>>> +      */
>>>
>>> I'm a bit confused by this. --keep-base works with either
>>> --reapply-cherry-picks (which is the default if --keep-base is given) or
>>> --no-reapply-cherry-picks. Just below this hunk we have
>>>
>>>          if (options.reapply_cherry_picks < 0)
>>>                  options.reapply_cherry_picks = keep_base;
>>>
>>> So we only set options.reapply_cherry_picks to match keep_base if the
>>> user did not specify -[-no]-reapply-cherry-picks on the commandline.
>>
>> 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
> 
> (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 all four cases (A)-(D), the apply backend will ignore whatever
>> --[no-]reapply-cherry-picks flag is provided.
> 
> 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.

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

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 1481c5b6a5..66aef356b8 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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)

Re: [PATCH v4 5/9] rebase: add coverage of other incompatible options

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> 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

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

Re: [PATCH v4 5/9] rebase: add coverage of other incompatible options

[Elijah Newren]

Hi Phillip,

On Tue, Jan 24, 2023 at 5:16 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> >>>> Signed-off-by: Elijah Newren <newren@gmail.com>
> >>>> --- a/builtin/rebase.c
> >>>> +++ b/builtin/rebase.c
> >>>> @@ -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.
> >>>> +      */
> >>>
> >>> I'm a bit confused by this. --keep-base works with either
> >>> --reapply-cherry-picks (which is the default if --keep-base is given) or
> >>> --no-reapply-cherry-picks. Just below this hunk we have
> >>>
> >>>          if (options.reapply_cherry_picks < 0)
> >>>                  options.reapply_cherry_picks = keep_base;
> >>>
> >>> So we only set options.reapply_cherry_picks to match keep_base if the
> >>> user did not specify -[-no]-reapply-cherry-picks on the commandline.
> >>
> >> 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
> >
> > (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.

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.

> >> For all four cases (A)-(D), the apply backend will ignore whatever
> >> --[no-]reapply-cherry-picks flag is provided.
> >
> > 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.
>
> 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
>
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 1481c5b6a5..66aef356b8 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -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.
> +                 */

I know you were copying the previous comment, but this comment is
really confusing to me.  Shouldn't it read "--reapply-cherry-picks
defaults to --keep-base" instead of vice-versa?

> +                options.reapply_cherry_picks = keep_base;
> +        else if (!keep_base)
> +                /*
> +                 * --keep-base implements --reapply-cherry-picks by

Should this be --[no-]reapply-cherry-picks, to clarify that it handles
both cases?  Especially given how many times I missed it?

> +                 * altering upstream so it works with both backends.
> +                 */
>                   imply_merge(&options, "--reapply-cherry-picks");

And perhaps this should be

    imply_merge(&options, options.reapply_cherry_picks ?
"--reapply-cherry-picks" :
         "--no-reapply-cherry-picks");

Also, the comment in git-rebase.txt about incompatibilities would become

     * --[no-]reapply-cherry-picks, when --keep-base is not provided

Re: [PATCH v4 5/9] rebase: add coverage of other incompatible options

[Phillip Wood]

Hi Elijah

On 24/01/2023 15:41, Elijah Newren wrote:
> 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.
> 
>>>> For all four cases (A)-(D), the apply backend will ignore whatever
>>>> --[no-]reapply-cherry-picks flag is provided.
>>>
>>> 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.
>>
>> 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
>>
>>
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index 1481c5b6a5..66aef356b8 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -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.
>> +                 */
> 
> I know you were copying the previous comment, but this comment is
> really confusing to me.  Shouldn't it read "--reapply-cherry-picks
> defaults to --keep-base" instead of vice-versa?

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


>> +                options.reapply_cherry_picks = keep_base;
>> +        else if (!keep_base)
>> +                /*
>> +                 * --keep-base implements --reapply-cherry-picks by
> 
> Should this be --[no-]reapply-cherry-picks, to clarify that it handles
> both cases?  Especially given how many times I missed it?

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

>> +                 * altering upstream so it works with both backends.
>> +                 */
>>                    imply_merge(&options, "--reapply-cherry-picks");
> 
> And perhaps this should be
> 
>      imply_merge(&options, options.reapply_cherry_picks ?
> "--reapply-cherry-picks" :
>           "--no-reapply-cherry-picks");

That's a good idea

> Also, the comment in git-rebase.txt about incompatibilities would become
> 
>       * --[no-]reapply-cherry-picks, when --keep-base is not provided

Yes, that's good

Thanks again for working on this

Phillip

Re: [PATCH v4 5/9] rebase: add coverage of other incompatible options

[Elijah Newren]

On Tue, Jan 24, 2023 at 8:48 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Elijah
>
> On 24/01/2023 15:41, Elijah Newren wrote:
[...]
> >> -        /*
> >> -         * --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.
> >> +                 */
> >
> > I know you were copying the previous comment, but this comment is
> > really confusing to me.  Shouldn't it read "--reapply-cherry-picks
> > defaults to --keep-base" instead of vice-versa?
>
> 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

I like that; sounds good to me.

> >> +                options.reapply_cherry_picks = keep_base;
> >> +        else if (!keep_base)
> >> +                /*
> >> +                 * --keep-base implements --reapply-cherry-picks by
> >
> > Should this be --[no-]reapply-cherry-picks, to clarify that it handles
> > both cases?  Especially given how many times I missed it?
>
> 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

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

?

Re: [PATCH v4 5/9] rebase: add coverage of other incompatible options

[Phillip Wood]

On 24/01/2023 17:12, Elijah Newren wrote:
> On Tue, Jan 24, 2023 at 8:48 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>>> +                options.reapply_cherry_picks = keep_base;
>>>> +        else if (!keep_base)
>>>> +                /*
>>>> +                 * --keep-base implements --reapply-cherry-picks by
>>>
>>> Should this be --[no-]reapply-cherry-picks, to clarify that it handles
>>> both cases?  Especially given how many times I missed it?
>>
>> 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
> 
> 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

[PATCH v4 6/9] rebase: clarify the OPT_CMDMODE incompatibilities

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt | 61 +++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 28 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 8cb075b2bdb..1ba691e4c5f 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -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

[PATCH v4 7/9] rebase: fix formatting of rebase --reapply-cherry-picks option in docs

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt | 2 --
 1 file changed, 2 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 1ba691e4c5f..9f794f5bdcc 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -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

[PATCH v4 8/9] rebase: put rebase_options initialization in single place

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/rebase.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index ed7475804cb..2358605b254 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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

[PATCH v4 9/9] rebase: provide better error message for apply options vs. merge config

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt           |  2 +-
 builtin/rebase.c                       | 27 ++++++++++++++++++++------
 t/t3422-rebase-incompatible-options.sh | 24 +++++++++++++++++++++++
 3 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 9f794f5bdcc..c357f6c3d5c 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -648,7 +648,7 @@ are incompatible with the following options:
  * --merge
  * --strategy
  * --strategy-option
- * --[no-]autosquash
+ * --autosquash
  * --rebase-merges
  * --interactive
  * --exec
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 2358605b254..dd7e2c788f5 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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"))
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 6a17b571ec7..4711b37a288 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -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

Re: [PATCH v4 0/9] rebase: fix several code/testing/documentation issues around flag incompatibilities

[Derrick Stolee]

On 1/22/2023 1:12 AM, Elijah Newren via GitGitGadget wrote:
> 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.

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

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

Re: [PATCH v4 0/9] rebase: fix several code/testing/documentation issues around flag incompatibilities

[Elijah Newren]

On Mon, Jan 23, 2023 at 7:56 AM Derrick Stolee <derrickstolee@github.com> wrote:
>
> On 1/22/2023 1:12 AM, Elijah Newren via GitGitGadget wrote:
> > 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.
>
> 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.)

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.

> > 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.
>
> 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 for reading!

[PATCH v5 00/10] rebase: fix several code/testing/documentation issues around flag incompatibilities

[Elijah Newren via GitGitGadget]

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

Signed-off-by: Elijah Newren newren@gmail.com

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
      
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
     - ## 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

[PATCH v5 01/10] rebase: mark --update-refs as requiring the merge backend

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

--update-refs is built in terms of the sequencer, which requires the
merge backend.  It was already marked as incompatible with the apply
backend in the git-rebase manual, but the code didn't check for this
incompatibility and warn the user.  Check and 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).

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt           |  2 ++
 builtin/rebase.c                       |  3 +++
 t/t3422-rebase-incompatible-options.sh | 15 ++++++++++-----
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index d811c1cf443..6490bc96a15 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -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
 --------------------
diff --git a/builtin/rebase.c b/builtin/rebase.c
index a26cc0cfdb5..c111b89e137 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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");
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 6dabb05a2ad..9da39cd91c2 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -25,11 +25,11 @@ test_expect_success 'setup' '
 '
 
 #
-# Rebase has lots of useful options like --whitepsace=fix, which are
-# actually all built in terms of flags to git-am.  Since neither
-# --merge nor --interactive (nor any options that imply those two) use
-# git-am, using them together will result in flags like --whitespace=fix
-# being ignored.  Make sure rebase warns the user and aborts instead.
+# Rebase has a 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

[PATCH v5 02/10] rebase: flag --apply and --merge as incompatible

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/rebase.c                       | 12 ++++++++++--
 t/t3422-rebase-incompatible-options.sh |  3 +++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index c111b89e137..b742cc8fb5c 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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;
 
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 9da39cd91c2..9b9e78479f6 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -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

[PATCH v5 03/10] rebase: remove --allow-empty-message from incompatible opts

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt | 1 -
 1 file changed, 1 deletion(-)

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

[PATCH v5 04/10] rebase: fix docs about incompatibilities with --root

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt           | 7 ++-----
 t/t3422-rebase-incompatible-options.sh | 4 ++++
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 7d01d1412d1..846aeed1b69 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -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:
 
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 9b9e78479f6..f86274990b0 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -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

[PATCH v5 05/10] rebase: fix incompatiblity checks for --[no-]reapply-cherry-picks

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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

Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt           |  2 +-
 builtin/rebase.c                       | 34 ++++++++++++++++----------
 t/t3422-rebase-incompatible-options.sh | 10 ++++++++
 3 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 846aeed1b69..8a09f12d897 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -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
diff --git a/builtin/rebase.c b/builtin/rebase.c
index b742cc8fb5c..05b130bfae5 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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);
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index f86274990b0..826f3b94ae6 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -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

Re: [PATCH v5 05/10] rebase: fix incompatiblity checks for --[no-]reapply-cherry-picks

[Phillip Wood]

Hi Elijah

On 25/01/2023 04:03, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> --[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.

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

> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>   Documentation/git-rebase.txt           |  2 +-
>   builtin/rebase.c                       | 34 ++++++++++++++++----------
>   t/t3422-rebase-incompatible-options.sh | 10 ++++++++
>   3 files changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 846aeed1b69..8a09f12d897 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -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
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index b742cc8fb5c..05b130bfae5 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -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);
> diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
> index f86274990b0..826f3b94ae6 100755
> --- a/t/t3422-rebase-incompatible-options.sh
> +++ b/t/t3422-rebase-incompatible-options.sh
> @@ -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

[PATCH v5 06/10] rebase: add coverage of other incompatible options

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/rebase.c                       |  3 +++
 t/t3422-rebase-incompatible-options.sh | 15 +++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 05b130bfae5..d6b20a6a536 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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");
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 826f3b94ae6..6a17b571ec7 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -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

[PATCH v5 07/10] rebase: clarify the OPT_CMDMODE incompatibilities

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt | 61 +++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 28 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 8a09f12d897..d2b731bd605 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -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

[PATCH v5 08/10] rebase: fix formatting of rebase --reapply-cherry-picks option in docs

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt | 2 --
 1 file changed, 2 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index d2b731bd605..99aadac28ef 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -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

[PATCH v5 09/10] rebase: put rebase_options initialization in single place

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/rebase.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index d6b20a6a536..ad5ebecbbdb 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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

[PATCH v5 10/10] rebase: provide better error message for apply options vs. merge config

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt           |  2 +-
 builtin/rebase.c                       | 27 ++++++++++++++++++++------
 t/t3422-rebase-incompatible-options.sh | 24 +++++++++++++++++++++++
 3 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 99aadac28ef..9a295bcee45 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -648,7 +648,7 @@ are incompatible with the following options:
  * --merge
  * --strategy
  * --strategy-option
- * --[no-]autosquash
+ * --autosquash
  * --rebase-merges
  * --interactive
  * --exec
diff --git a/builtin/rebase.c b/builtin/rebase.c
index ad5ebecbbdb..7171be40eeb 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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"))
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 6a17b571ec7..4711b37a288 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -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

Re: [PATCH v5 00/10] rebase: fix several code/testing/documentation issues around flag incompatibilities

[Phillip Wood]

Hi Elijah

On 25/01/2023 04:03, Elijah Newren via GitGitGadget wrote:
> 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.

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


> 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.)
> 
> Signed-off-by: Elijah Newren newren@gmail.com
> 
> 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
>        
>            Signed-off-by: Elijah Newren <newren@gmail.com>
>        
>       - ## 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
> 

Re: [PATCH v5 00/10] rebase: fix several code/testing/documentation issues around flag incompatibilities

[Junio C Hamano]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

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.

Re: [PATCH v5 00/10] rebase: fix several code/testing/documentation issues around flag incompatibilities

[Elijah Newren]

On Wed, Jan 25, 2023 at 8:39 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > 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.
>
> 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.

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/xmqqpmb4j8e9.fsf@gitster.g/ 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.

rebase --merge vs --whitespace=fix, was Re: [PATCH v5 00/10] rebase: fix several code/testing/documentation issues around flag incompatibilities

[Johannes Schindelin]

Hi Elijah,

On Wed, 25 Jan 2023, Elijah Newren via GitGitGadget wrote:

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

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

Re: rebase --merge vs --whitespace=fix, was Re: [PATCH v5 00/10] rebase: fix several code/testing/documentation issues around flag incompatibilities

[Elijah Newren]

On Thu, Feb 2, 2023 at 2:29 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Elijah,
>
> On Wed, 25 Jan 2023, Elijah Newren via GitGitGadget wrote:
>
> > 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.
>
> 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.

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