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

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

On 1/19/2023 12:36 AM, Elijah Newren via GitGitGadget wrote:

Thank you for submitting this version.

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

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.

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

On Thu, Jan 19, 2023 at 1:47 PM Derrick Stolee <derrickstolee@github.com> wrote:

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

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

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

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

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

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

....

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

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

Derrick Stolee <derrickstolee@github.com> writes:

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.

On Fri, Jan 20, 2023 at 7:27 AM Junio C Hamano <gitster@pobox.com> wrote:

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

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

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

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

On Fri, Jan 20, 2023 at 12:14 AM Elijah Newren via GitGitGadget <gitgitgadget@gmail.com> wrote:

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.

On Thu, Jan 19, 2023 at 9:40 PM Eric Sunshine <sunshine@sunshineco.com> wrote:

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

On Fri, 20 Jan 2023 at 06:27, Elijah Newren via GitGitGadget <gitgitgadget@gmail.com> wrote:

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

On Fri, Jan 20, 2023 at 1:55 AM Martin Ågren <martin.agren@gmail.com> wrote:

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.

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

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>

On Fri, Jan 20, 2023 at 4:05 AM Junio C Hamano <gitster@pobox.com> wrote:

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.

Elijah Newren <newren@gmail.com> writes:

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

On Fri, Jan 20, 2023 at 8:16 AM Junio C Hamano <gitster@pobox.com> wrote:

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/

Elijah Newren <newren@gmail.com> writes:

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

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

Hi Elijah

Thanks for working on this

On 20/01/2023 04:56, Elijah Newren via GitGitGadget wrote:

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

On Fri, Jan 20, 2023 at 8:46 AM Phillip Wood <phillip.wood123@gmail.com> wrote:

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

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

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

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

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

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

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

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

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

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

Hi Elijah

On 21/01/2023 01:55, Elijah Newren via GitGitGadget wrote:

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

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

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

Hi Elijah

On 21/01/2023 01:55, Elijah Newren via GitGitGadget wrote:

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

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

On 21/01/2023 15:20, Phillip Wood wrote:

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

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

Phillip

On Sat, Jan 21, 2023 at 11:25 AM Phillip Wood <phillip.wood123@gmail.com> wrote:

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

Right, nor do we support
      --whitespace=fix --keep-base --no-reapply-cherry-picks
(If you try it, you'll notice that the code accepts those flags and
starts the rebase pretending everything is fine, but it silently
ignores the --no-reapply-cherry-picks flag.)

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

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

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

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

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

Hi Elijah

On 21/01/2023 01:55, Elijah Newren via GitGitGadget wrote:

Thanks for clearing up my mess!

Phillip

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

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

Changes since 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

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

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

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

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

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

Hi Elijah

On 22/01/2023 06:12, Elijah Newren via GitGitGadget wrote:

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

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

Hi Phillip,

On Mon, Jan 23, 2023 at 12:08 PM Phillip Wood <phillip.wood123@gmail.com> wrote:

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

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

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

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

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

But we could not allow commands like

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

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

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

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

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

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

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

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

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

Hi Elijah

On 24/01/2023 02:36, Elijah Newren wrote:

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

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

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

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

Best Wishes

Phillip

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

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

Best Wishes

Phillip

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

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

Hi Phillip,

On Tue, Jan 24, 2023 at 5:16 AM Phillip Wood <phillip.wood123@gmail.com> 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.

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?

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

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

Hi Elijah

On 24/01/2023 15:41, Elijah Newren wrote:

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

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

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

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

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

That's a good idea

Yes, that's good

Thanks again for working on this

Phillip

On Tue, Jan 24, 2023 at 8:48 AM Phillip Wood <phillip.wood123@gmail.com> wrote:

[...]

I like that; sounds good to me.

Somewhat wordy, but perhaps:

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

?

On 24/01/2023 17:12, Elijah Newren wrote:

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

Best Wishes

Phillip

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

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

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

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

On 1/22/2023 1:12 AM, Elijah Newren via GitGitGadget wrote:

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

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

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

Thanks! -Stolee

On Mon, Jan 23, 2023 at 7:56 AM Derrick Stolee <derrickstolee@github.com> wrote:

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

Thanks for reading!

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

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

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

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

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

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

Hi Elijah

On 25/01/2023 04:03, Elijah Newren via GitGitGadget wrote:

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

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

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

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

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

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

Hi Elijah

On 25/01/2023 04:03, Elijah Newren via GitGitGadget wrote:

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

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

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.

On Wed, Jan 25, 2023 at 8:39 AM Junio C Hamano <gitster@pobox.com> wrote:

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.

Hi Elijah,

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

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

On Thu, Feb 2, 2023 at 2:29 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

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