[1/4] sequencer: Do not require `allow_empty` for redundant commit options

Commit Message

Brian Lyles Jan. 19, 2024, 5:59 a.m. UTC

From: Brian Lyles <[e-mail]>

Previously, a consumer of the sequencer that wishes to take advantage of
either the `keep_redundant_commits` or `drop_redundant_commits` feature
must also specify `allow_empty`.

The only consumer of `drop_redundant_commits` is `git-rebase`, which
already allows empty commits by default and simply always enables
`allow_empty`. `keep_redundant_commits` was also consumed by
`git-cherry-pick`, which had to specify `allow-empty` when
`keep_redundant_commits` was specified in order for the sequencer's
`allow_empty()` to actually respect `keep_redundant_commits`.

The latter is an interesting case: As noted in the docs, this means that
`--keep-redundant-commits` implies `--allow-empty`, despite the two
having distinct, non-overlapping meanings:

- `allow_empty` refers specifically to commits which start empty, as
  indicated by the documentation for `--allow-empty` within
  `git-cherry-pick`:

  "Note also, that use of this option only keeps commits that were
  initially empty (i.e. the commit recorded the same tree as its
  parent). Commits which are made empty due to a previous commit are
  dropped. To force the inclusion of those commits use
  --keep-redundant-commits."

- `keep_redundant_commits` refers specifically to commits that do not
  start empty, but become empty due to the content already existing in
  the target history. This is indicated by the documentation for
  `--keep-redundant-commits` within `git-cherry-pick`:

  "If a commit being cherry picked duplicates a commit already in the
  current history, it will become empty. By default these redundant
  commits cause cherry-pick to stop so the user can examine the commit.
  This option overrides that behavior and creates an empty commit
  object. Implies --allow-empty."

This implication of `--allow-empty` therefore seems incorrect: One
should be able to keep a commit that becomes empty without also being
forced to pick commits that start as empty. However, today, the
following series of commands would result in both the commit that became
empty and the commit that started empty being picked despite only
`--keep-redundant-commits` being specified:

    git init
    echo "a" >test
    git add test
    git commit -m "Initial commit"
    echo "b" >test
    git commit -am "a -> b"
    git commit --allow-empty -m "empty"
    git cherry-pick --keep-redundant-commits HEAD^ HEAD

The same cherry-pick with `--allow-empty` would fail on the redundant
commit, and with neither option would fail on the empty commit.

In a future commit, an `--empty` option will be added to
`git-cherry-pick`, meaning that `drop_redundant_commits` will be
available in that command. For that to be possible with the current
implementation of the sequencer's `allow_empty()`, `git-cherry-pick`
would need to specify `allow_empty` with `drop_redundant_commits` as
well, which is an even less intuitive implication of `--allow-empty`: in
order to prevent redundant commits automatically, initially-empty
commits would need to be kept automatically.

Instead, this commit rewrites the `allow_empty()` logic to remove the
over-arching requirement that `allow_empty` be specified in order to
reach any of the keep/drop behaviors. Only if the commit was originally
empty will `allow_empty` have an effect.

For some amount of backwards compatibility with the existing code and
tests, I have opted to preserve the behavior of returning 0 when:

- `allow_empty` is specified, and
- either `is_index_unchanged` or `is_original_commit_empty` indicates an
  error

This is primarily out of caution -- I am not positive what downstream
impacts this might have.

Note that this commit is a breaking change: `--keep-redundant-commits`
will no longer imply `--allow-empty`. It would be possible to maintain
the current behavior of `--keep-redundant-commits` implying
`--allow-empty` if it were needed to avoid a breaking change, but I
believe that decoupling them entirely is the correct behavior.

Signed-off-by: Brian Lyles <[e-mail]>
---

Disclaimer: This is my first contribution to the git project, and thus
my first attempt at submitting a patch via `git-send-email`. It is also
the first time I've touched worked in C in over a decade, and I really
didn't work with it much before that either. I welcome any and all
feedback on what I may have gotten wrong regarding the patch submission
process, the code changes, or my commit messages.

This is the first in a series of commits that aims to introduce an
`--empty` option to `git-cherry-pick` that provides the same flexibility
as the `--empty` options for `git-rebase` and `git-am`, as well as
improve the consistency in the values and documentation for this option
across the three commands.

The main thing that may be controversial with this particular commit is
that I am proposing a breaking change. As described in the above
message, I do not think that it makes sense to tie `--allow-empty` and
`--keep-redundant-commits` together since they appear to be intended to
work with different types of empty commits. That being said, if it is
deemed unacceptable to make this breaking change, we can consider an
alternative approach where we maintain the behavior of
`--keep-redundant-commits` implying `--allow-empty`, while preventing
the need for the future `--empty=drop` to have that same implication.

 Documentation/git-cherry-pick.txt | 10 +++++++---
 builtin/revert.c                  |  4 ----
 sequencer.c                       | 18 ++++++++++--------
 t/t3505-cherry-pick-empty.sh      |  5 +++++
 4 files changed, 22 insertions(+), 15 deletions(-)

Comments

Kristoffer Haugsbakk Jan. 20, 2024, 9:38 p.m. UTC | #1

Initial discussion (proposal): https://lore.kernel.org/git/CAHPHrSevBdQF0BisR8VK=jM=wj1dTUYEVrv31gLerAzL9=[e-mail]/

On Fri, Jan 19, 2024, at 06:59, [e-mail] wrote:
> From: Brian Lyles <[e-mail]>
>
> Previously, a consumer of the sequencer that wishes to take advantage of
> either the `keep_redundant_commits` or `drop_redundant_commits` feature
> must also specify `allow_empty`.

Previously to this change? It is preferred to describe what the code
currently does without this change in the present tense.[1] The change
itself uses the imperative mood.[2]

† 1: SubmittingPatches, “The problem statement that describes the status
    quo …”
† 2: SubmittingPatches, “Describe your changes in imperative mood […] as
    if you are giving orders to the codebase to change its behavior.”

> The only consumer of `drop_redundant_commits` is `git-rebase`, which
> already allows empty commits by default and simply always enables
> `allow_empty`. `keep_redundant_commits` was also consumed by
> `git-cherry-pick`, which had to specify `allow-empty` when
> `keep_redundant_commits` was specified in order for the sequencer's
> `allow_empty()` to actually respect `keep_redundant_commits`.
>
> The latter is an interesting case: As noted in the docs, this means that
> `--keep-redundant-commits` implies `--allow-empty`, despite the two
> having distinct, non-overlapping meanings:

Huh. I’m used to the git-rebase(1) behavior and I definitely would have
just assumed that git-cherry-pick(1) behaves the same. :) Nice catch.

> This implication of `--allow-empty` therefore seems incorrect: One
> should be able to keep a commit that becomes empty without also being
> forced to pick commits that start as empty. However, today, the
> following series of commands would result in both the commit that became
> empty and the commit that started empty being picked despite only
> `--keep-redundant-commits` being specified:

Nice description of the current problem. All of it.

> In a future commit, an `--empty` option will be added to
> `git-cherry-pick`, meaning that `drop_redundant_commits` will be
> available in that command. For that to be possible with the current
> implementation of the sequencer's `allow_empty()`, `git-cherry-pick`
> would need to specify `allow_empty` with `drop_redundant_commits` as
> well, which is an even less intuitive implication of `--allow-empty`: in
> order to prevent redundant commits automatically, initially-empty
> commits would need to be kept automatically.
>
> Instead, this commit rewrites the `allow_empty()` logic to remove the
> over-arching requirement that `allow_empty` be specified in order to
> reach any of the keep/drop behaviors. Only if the commit was originally
> empty will `allow_empty` have an effect.

In general, phrases like “this commit <verb>” or “this patch <verb>” can
be rewritten to the “commanding” style (see [2]).[3] But here you’re
starting a new paragraph after having talked about a future commit, so
using the commanding style might be stylistically difficult to pull off
without breaking the flow of the text.

And “this [commit][patch] <verb>” seems to be used with some regularity in
any case.

Brian Lyles Jan. 21, 2024, 6:19 p.m. UTC | #2

On Sat, Jan 20, 2024 at 3:38 PM Kristoffer Haugsbakk
<[e-mail]> wrote:

Hi Kristoffer,
Thank you for taking some time to review my patch.

> Initial discussion (proposal): https://lore.kernel.org/git/CAHPHrSevBdQF0BisR8VK=jM=wj1dTUYEVrv31gLerAzL9=[e-mail]/

I ought to have included this in a cover letter -- thanks for linking it
here, and I will include this with v2.

> On Fri, Jan 19, 2024, at 06:59, [e-mail] wrote:
> > From: Brian Lyles <[e-mail]>
> >
> > Previously, a consumer of the sequencer that wishes to take advantage of
> > either the `keep_redundant_commits` or `drop_redundant_commits` feature
> > must also specify `allow_empty`.
>
> Previously to this change? It is preferred to describe what the code
> currently does without this change in the present tense.[1] The change
> itself uses the imperative mood.[2]
>
> † 1: SubmittingPatches, “The problem statement that describes the status
>     quo …”
> † 2: SubmittingPatches, “Describe your changes in imperative mood […] as
>     if you are giving orders to the codebase to change its behavior.”

I appreciate the stylistic feedback here. I will tweak this for v2 of
the patch.

> > In a future commit, an `--empty` option will be added to
> > `git-cherry-pick`, meaning that `drop_redundant_commits` will be
> > available in that command. For that to be possible with the current
> > implementation of the sequencer's `allow_empty()`, `git-cherry-pick`
> > would need to specify `allow_empty` with `drop_redundant_commits` as
> > well, which is an even less intuitive implication of `--allow-empty`: in
> > order to prevent redundant commits automatically, initially-empty
> > commits would need to be kept automatically.
> >
> > Instead, this commit rewrites the `allow_empty()` logic to remove the
> > over-arching requirement that `allow_empty` be specified in order to
> > reach any of the keep/drop behaviors. Only if the commit was originally
> > empty will `allow_empty` have an effect.
>
> In general, phrases like “this commit <verb>” or “this patch <verb>” can
> be rewritten to the “commanding” style (see [2]).[3] But here you’re
> starting a new paragraph after having talked about a future commit, so
> using the commanding style might be stylistically difficult to pull off
> without breaking the flow of the text.
>
> And “this [commit][patch] <verb>” seems to be used with some regularity in
> any case.
>
> 

Phillip Wood Jan. 23, 2024, 2:23 p.m. UTC | #3

Hi Brian

Let me start by saying that overall I'm impressed with the quality of 
this submission. I've left quite a few comments but for a first patch 
series it is very good.

On 19/01/2024 05:59, [e-mail] wrote:
> From: Brian Lyles <[e-mail]>
> 
> Previously, a consumer of the sequencer that wishes to take advantage of
> either the `keep_redundant_commits` or `drop_redundant_commits` feature
> must also specify `allow_empty`.
> 
> The only consumer of `drop_redundant_commits` is `git-rebase`, which
> already allows empty commits by default and simply always enables
> `allow_empty`. `keep_redundant_commits` was also consumed by
> `git-cherry-pick`, which had to specify `allow-empty` when
> `keep_redundant_commits` was specified in order for the sequencer's
> `allow_empty()` to actually respect `keep_redundant_commits`.

I think it might be more persuasive to start the commit message by 
explaining what user visible change you're trying to make and why rather 
than concentrating on the implementation details.

> The latter is an interesting case: As noted in the docs, this means that
> `--keep-redundant-commits` implies `--allow-empty`, despite the two
> having distinct, non-overlapping meanings:
> 
> - `allow_empty` refers specifically to commits which start empty, as
>    indicated by the documentation for `--allow-empty` within
>    `git-cherry-pick`:
> 
>    "Note also, that use of this option only keeps commits that were
>    initially empty (i.e. the commit recorded the same tree as its
>    parent). Commits which are made empty due to a previous commit are
>    dropped. To force the inclusion of those commits use
>    --keep-redundant-commits."
> 
> - `keep_redundant_commits` refers specifically to commits that do not
>    start empty, but become empty due to the content already existing in
>    the target history. This is indicated by the documentation for
>    `--keep-redundant-commits` within `git-cherry-pick`:
> 
>    "If a commit being cherry picked duplicates a commit already in the
>    current history, it will become empty. By default these redundant
>    commits cause cherry-pick to stop so the user can examine the commit.
>    This option overrides that behavior and creates an empty commit
>    object. Implies --allow-empty."
> 
> This implication of `--allow-empty` therefore seems incorrect: One
> should be able to keep a commit that becomes empty without also being
> forced to pick commits that start as empty.

Do you have a practical example of where you want to keep the commits 
that become empty but not the ones that start empty? I agree there is a 
distinction but I think the common case is that the user wants to keep 
both types of empty commit or none. I'm not against giving the user the 
option to keep one or the other if it is useful but I'm wary of changing 
the default.

> However, today, the
> following series of commands would result in both the commit that became
> empty and the commit that started empty being picked despite only
> `--keep-redundant-commits` being specified:
> 
>      git init
>      echo "a" >test
>      git add test
>      git commit -m "Initial commit"
>      echo "b" >test
>      git commit -am "a -> b"
>      git commit --allow-empty -m "empty"
>      git cherry-pick --keep-redundant-commits HEAD^ HEAD
> 
> The same cherry-pick with `--allow-empty` would fail on the redundant
> commit, and with neither option would fail on the empty commit.
> 
> In a future commit, an `--empty` option will be added to
> `git-cherry-pick`, meaning that `drop_redundant_commits` will be
> available in that command. For that to be possible with the current
> implementation of the sequencer's `allow_empty()`, `git-cherry-pick`
> would need to specify `allow_empty` with `drop_redundant_commits` as
> well, which is an even less intuitive implication of `--allow-empty`: in
> order to prevent redundant commits automatically, initially-empty
> commits would need to be kept automatically.
>
> Instead, this commit rewrites the `allow_empty()` logic to remove the
> over-arching requirement that `allow_empty` be specified in order to
> reach any of the keep/drop behaviors. Only if the commit was originally
> empty will `allow_empty` have an effect.

rebase always sets "opts->allow_empty = 1" in 
builtin/rebase.c:get_replay_opts() and if the user passes 
--no-keep-empty drops commits that start empty from the list of commits 
to be picked. This is slightly confusing but is more efficient as we 
don't do waste time trying to pick a commit we're going to drop. Can we 
do something similar for "git cherry-pick"? When cherry-picking a 
sequence of commits I think it should just work because the code is 
shared with rebase, for a single commit we'd need to add a test to see 
if it is empty in single_pick() before calling pick_commits().

> For some amount of backwards compatibility with the existing code and
> tests, I have opted to preserve the behavior of returning 0 when:
> 
> - `allow_empty` is specified, and
> - either `is_index_unchanged` or `is_original_commit_empty` indicates an
>    error

I'm not sure that is a good idea as it is hiding an error that we didn't 
hit before because we returned early.

> This is primarily out of caution -- I am not positive what downstream
> impacts this might have.
> 
> Note that this commit is a breaking change: `--keep-redundant-commits`
> will no longer imply `--allow-empty`. It would be possible to maintain
> the current behavior of `--keep-redundant-commits` implying
> `--allow-empty` if it were needed to avoid a breaking change, but I
> believe that decoupling them entirely is the correct behavior.

Thank you for being clear about the change in behavior, as I said above 
I'm wary of changing the default unless there is a compelling reason but 
I'm happy to support

     git cherry-pick --keep-redundant-commits --no-allow-empty

if it is needed.

> Signed-off-by: Brian Lyles <[e-mail]>
> ---
> 
> Disclaimer: This is my first contribution to the git project, and thus
> my first attempt at submitting a patch via `git-send-email`. It is also
> the first time I've touched worked in C in over a decade, and I really
> didn't work with it much before that either. I welcome any and all
> feedback on what I may have gotten wrong regarding the patch submission
> process, the code changes, or my commit messages.

As others have mentioned I think it would be useful to have a 
cover-letter where we can discuss the aim of the patch series 
independently of the individual patches.

> This is the first in a series of commits that aims to introduce an
> `--empty` option to `git-cherry-pick` that provides the same flexibility
> as the `--empty` options for `git-rebase` and `git-am`, as well as
> improve the consistency in the values and documentation for this option
> across the three commands.

I think that is a good aim

> The main thing that may be controversial with this particular commit is
> that I am proposing a breaking change. As described in the above
> message, I do not think that it makes sense to tie `--allow-empty` and
> `--keep-redundant-commits` together since they appear to be intended to
> work with different types of empty commits. That being said, if it is
> deemed unacceptable to make this breaking change, we can consider an
> alternative approach where we maintain the behavior of
> `--keep-redundant-commits` implying `--allow-empty`, while preventing
> the need for the future `--empty=drop` to have that same implication.

As I said above I think it would be worth looking at what "git rebase" 
does to see if we can do the same thing for "git cherry-pick".

 > [...]> +test_expect_success 'cherry pick an empty non-ff commit with 
--keep-redundant-commits' '
> +	git checkout main &&
> +	test_must_fail git cherry-pick --keep-redundant-commits empty-change-branch

When using test_must_fail it is a good idea to check the error message 
to make sure that it's failing for the reason we expect (see patch 4).

Best Wishes

Phillip

Junio C Hamano Jan. 23, 2024, 6:18 p.m. UTC | #4

Phillip Wood <[e-mail]> writes:

>> This implication of `--allow-empty` therefore seems incorrect: One
>> should be able to keep a commit that becomes empty without also being
>> forced to pick commits that start as empty.
>
> Do you have a practical example of where you want to keep the commits
> that become empty but not the ones that start empty? I agree there is
> a distinction but I think the common case is that the user wants to
> keep both types of empty commit or none. I'm not against giving the
> user the option to keep one or the other if it is useful but I'm wary
> of changing the default.

This may not a new issue introduced by this series, but one thing I
would be worried about the usability of the keep-redundant is that I
know it takes more than one tries of cherry-picking of the same
series, at least to me, to get a series right.  The initial attempt
may make some commit empty and thanks to --keep-redundant they will
be kept, but I'll inevitably find more things I need to tweak and
cherry-pick the resulting series, possibly on a different base.  And
to this second round of cherry-pick, these "were not, but now have
become empty" commits appear empty from the start.

Phillip Wood Jan. 24, 2024, 11:01 a.m. UTC | #5

Hi Brian

Here are some code comments now I've realized why we need to change it

On 19/01/2024 05:59, [e-mail] wrote:
> From: Brian Lyles <[e-mail]>
 >   Documentation/git-cherry-pick.txt | 10 +++++++---
 >   builtin/revert.c                  |  4 ----
 >   sequencer.c                       | 18 ++++++++++--------
 >   t/t3505-cherry-pick-empty.sh      |  5 +++++
 >   4 files changed, 22 insertions(+), 15 deletions(-)
 >
 > diff --git a/Documentation/git-cherry-pick.txt 
b/Documentation/git-cherry-pick.txt
 > index fdcad3d200..806295a730 100644
 > --- a/Documentation/git-cherry-pick.txt
 > +++ b/Documentation/git-cherry-pick.txt
 > @@ -131,8 +131,8 @@ effect to your index in a row.
 >       even without this option.  Note also, that use of this option only
 >       keeps commits that were initially empty (i.e. the commit 
recorded the
 >       same tree as its parent).  Commits which are made empty due to a
 > -    previous commit are dropped.  To force the inclusion of those 
commits
 > -    use `--keep-redundant-commits`.
 > +    previous commit will cause the cherry-pick to fail.  To force the
 > +    inclusion of those commits use `--keep-redundant-commits`.
 >
 >   --allow-empty-message::
 >       By default, cherry-picking a commit with an empty message will 
fail.
 > @@ -144,7 +144,11 @@ effect to your index in a row.
 >       current history, it will become empty.  By default these
 >       redundant commits cause `cherry-pick` to stop so the user can
 >       examine the commit. This option overrides that behavior and
 > -    creates an empty commit object.  Implies `--allow-empty`.
 > +    creates an empty commit object. Note that use of this option only
 > +    results in an empty commit when the commit was not initially empty,
 > +    but rather became empty due to a previous commit. Commits that were
 > +    initially empty will cause the cherry-pick to fail. To force the
 > +    inclusion of those commits use `--allow-empty`.
 >
 >   --strategy=<strategy>::
 >       Use the given merge strategy.  Should only be used once.
 > diff --git a/builtin/revert.c b/builtin/revert.c
 > index e6f9a1ad26..b2cfde7a87 100644
 > --- a/builtin/revert.c
 > +++ b/builtin/revert.c
 > @@ -136,10 +136,6 @@ static int run_sequencer(int argc, const char 
**argv, const char *prefix,
 >       prepare_repo_settings(the_repository);
 >       the_repository->settings.command_requires_full_index = 0;
 >
 > -    /* implies allow_empty */
 > -    if (opts->keep_redundant_commits)
 > -        opts->allow_empty = 1;

I'm wary of this, especially after Juino's comments in 
https://lore.kernel.org/git/xmqqy1cfnca7.fsf@gitster.g/

The documentation changes look if we do decide to change the default.

 >       if (cleanup_arg) {
 >           opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1);
 >           opts->explicit_cleanup = 1;
 > diff --git a/sequencer.c b/sequencer.c
 > index d584cac8ed..582bde8d46 100644
 > --- a/sequencer.c
 > +++ b/sequencer.c
 > @@ -1739,22 +1739,24 @@ static int allow_empty(struct repository *r,
 >        *
 >        * (4) we allow both.
 >        */

The comment above should be updated if we change the behavior of this 
function.

 > -    if (!opts->allow_empty)
 > -        return 0; /* let "git commit" barf as necessary */
 > -

Here we stop returning early if allow_empty is not set - this allows us 
to apply allow_empty only to commits that start empty below.

 >       index_unchanged = is_index_unchanged(r);
 > -    if (index_unchanged < 0)
 > +    if (index_unchanged < 0) {
 > +        if (!opts->allow_empty)
 > +            return 0;
 >           return index_unchanged;
 > +    }

I don't think we want to hide the error here or below from 
originally_empty()

 >       if (!index_unchanged)
 >           return 0; /* we do not have to say --allow-empty */
 >
 > -    if (opts->keep_redundant_commits)
 > -        return 1;
 > -

We move this check so that we do not unconditionally keep commits that 
are initially empty when opts->keep_redundant_commits is set.

 >       originally_empty = is_original_commit_empty(commit);
 > -    if (originally_empty < 0)
 > +    if (originally_empty < 0) {
 > +        if (!opts->allow_empty)
 > +            return 0;
 >           return originally_empty;
 > +    }
 >       if (originally_empty)
 > +        return opts->allow_empty;

Now opts->allow_empty only applies to commits that were originally empty

 > +    else if (opts->keep_redundant_commits)
 >           return 1;

This ensures we keep commits that become empty when 
opts->redundant_commits is set.

The changes to allow_empty() all looks good apart from hiding errors 
from index_unchanged() and originally_empty()

Best Wishes

Phillip

Phillip Wood Jan. 24, 2024, 11:01 a.m. UTC | #6

Hi Brian

On 23/01/2024 14:23, Phillip Wood wrote:

> rebase always sets "opts->allow_empty = 1" in 
> builtin/rebase.c:get_replay_opts() and if the user passes 
> --no-keep-empty drops commits that start empty from the list of commits 
> to be picked. This is slightly confusing but is more efficient as we 
> don't do waste time trying to pick a commit we're going to drop. Can we 
> do something similar for "git cherry-pick"? When cherry-picking a 
> sequence of commits I think it should just work because the code is 
> shared with rebase, for a single commit we'd need to add a test to see 
> if it is empty in single_pick() before calling pick_commits().

Having thought about this again I don't think we can reuse the same 
approach as rebase because cherry-pick and rebase have different 
behaviors. "git rebase --no-keep-empty" drops empty commits whereas "git 
cherry-pick" wants to error out if it sees an empty commit. So I think 
your approach of reworking allow_empty() in the sequencer is the right 
approach.

Sorry for the confusion

Phillip

Brian Lyles Jan. 27, 2024, 11:30 p.m. UTC | #7

[+cc Junio]

Hi Phillip

On Tue, Jan 23, 2024 at 8:23 AM Phillip Wood <[e-mail]> wrote:
>
> Hi Brian
>
> Let me start by saying that overall I'm impressed with the quality of
> this submission. I've left quite a few comments but for a first patch
> series it is very good.

Thank you for the kind words!

> On 19/01/2024 05:59, [e-mail] wrote:
> > From: Brian Lyles <[e-mail]>
> >
> > Previously, a consumer of the sequencer that wishes to take advantage of
> > either the `keep_redundant_commits` or `drop_redundant_commits` feature
> > must also specify `allow_empty`.
> >
> > The only consumer of `drop_redundant_commits` is `git-rebase`, which
> > already allows empty commits by default and simply always enables
> > `allow_empty`. `keep_redundant_commits` was also consumed by
> > `git-cherry-pick`, which had to specify `allow-empty` when
> > `keep_redundant_commits` was specified in order for the sequencer's
> > `allow_empty()` to actually respect `keep_redundant_commits`.
>
> I think it might be more persuasive to start the commit message by
> explaining what user visible change you're trying to make and why rather
> than concentrating on the implementation details.

I struggled a bit with this initially because the motivation behind the
change in this particular commit was driven by a technical issue in my
mind. The side-effect with git-cherry-pick(s) `--allow-empty` and
`--keep-redundant-commits` was mildly problematic, but less concerning
that the future problem that we'd have once git-cherry-pick(1) got the
more robust `--empty` option in a later commit in this series.

I think my problem came down to this commit trying to solve two problems
at once -- the underlying technical concern _and_ the git-cherry-pick(1)
behavior.

In v2, I intend to break this commit into two:

- Update `allow_empty()` to not require `allow_empty`, but without
  actually changing any consumers (and thus without making any
  functional change)
- Update git-cherry-pick(1) such that `--keep-redundant-commits` no
  longer implies `--allow-empty`.

This allows me to better justify the technical change technically and
the functional change functionally, while also making it easier to drop
the functional change if we decide that a breaking change is not
warranted to address this.

> Do you have a practical example of where you want to keep the commits
> that become empty but not the ones that start empty? I agree there is a
> distinction but I think the common case is that the user wants to keep
> both types of empty commit or none. I'm not against giving the user the
> option to keep one or the other if it is useful but I'm wary of changing
> the default.

That practical example is documented in the initial discussion[1], which
I should have ought to have linked in a cover letter for this series
(and will do so in v2). I'll avoid copying the details here, but we'd
very much like to be able to programmatically drop the commits that
become empty when doing the automated cherry-pick described there.

[1]: https://lore.kernel.org/git/CAHPHrSevBdQF0BisR8VK=jM=wj1dTUYEVrv31gLerAzL9=[e-mail]/

> rebase always sets "opts->allow_empty = 1" in
> builtin/rebase.c:get_replay_opts() and if the user passes
> --no-keep-empty drops commits that start empty from the list of commits
> to be picked. This is slightly confusing but is more efficient as we
> don't do waste time trying to pick a commit we're going to drop. Can we
> do something similar for "git cherry-pick"? When cherry-picking a
> sequence of commits I think it should just work because the code is
> shared with rebase, for a single commit we'd need to add a test to see
> if it is empty in single_pick() before calling pick_commits().

Just noting here for future readers here that you sent a followup email
indicating this was not viable:

> On Wed, Jan 24, 2024 at 5:01 AM Phillip Wood <[e-mail]> wrote:
>
> Having thought about this again I don't think we can reuse the same
> approach as rebase because cherry-pick and rebase have different
> behaviors. "git rebase --no-keep-empty" drops empty commits whereas "git
> cherry-pick" wants to error out if it sees an empty commit. So I think
> your approach of reworking allow_empty() in the sequencer is the right
> approach.
>
> Sorry for the confusion

No worries. If you have any suggestions for how I might better explain
these changes in the commit message, please let me know.

> > For some amount of backwards compatibility with the existing code and
> > tests, I have opted to preserve the behavior of returning 0 when:
> >
> > - `allow_empty` is specified, and
> > - either `is_index_unchanged` or `is_original_commit_empty` indicates an
> >    error
>
> I'm not sure that is a good idea as it is hiding an error that we didn't
> hit before because we returned early.

I think you're right -- Previously the error could not have been hit,
but now it can. An error is still an error, and we should handle it
regardless of how `allow_empty` was set. I'll address this in v2 by
simply returning the error.

> > Note that this commit is a breaking change: `--keep-redundant-commits`
> > will no longer imply `--allow-empty`. It would be possible to maintain
> > the current behavior of `--keep-redundant-commits` implying
> > `--allow-empty` if it were needed to avoid a breaking change, but I
> > believe that decoupling them entirely is the correct behavior.
>
> Thank you for being clear about the change in behavior, as I said above
> I'm wary of changing the default unless there is a compelling reason but
> I'm happy to support
>
>      git cherry-pick --keep-redundant-commits --no-allow-empty
>
> if it is needed.

I totally understand being wary here.

I've certainly convinced myself that having the future `--empty=drop`
behavior introduced later in this patch should not imply
`--allow-empty`.

I also _think_ that the existing behavior of `--keep-redundant-commits`
is probably technically not ideal or correct, but could be convinced
that changing it now is not worthwhile. I will defer to group consensus
here.

> > Signed-off-by: Brian Lyles <[e-mail]>
> > ---
> >
> > Disclaimer: This is my first contribution to the git project, and thus
> > my first attempt at submitting a patch via `git-send-email`. It is also
> > the first time I've touched worked in C in over a decade, and I really
> > didn't work with it much before that either. I welcome any and all
> > feedback on what I may have gotten wrong regarding the patch submission
> > process, the code changes, or my commit messages.
>
> As others have mentioned I think it would be useful to have a
> cover-letter where we can discuss the aim of the patch series
> independently of the individual patches.

Absolutely, will do in v2.

>  > [...]> +test_expect_success 'cherry pick an empty non-ff commit with
> --keep-redundant-commits' '
> > +     git checkout main &&
> > +     test_must_fail git cherry-pick --keep-redundant-commits empty-change-branch
>
> When using test_must_fail it is a good idea to check the error message
> to make sure that it's failing for the reason we expect (see patch 4).

Thanks for the tip, I'll add this in v2.

On Wed, Jan 24, 2024 at 5:01 AM Phillip Wood <[e-mail]> wrote:

> Here are some code comments now I've realized why we need to change it
>
> On 19/01/2024 05:59, [e-mail] wrote:
> > From: Brian Lyles <[e-mail]>
>  >   Documentation/git-cherry-pick.txt | 10 +++++++---
>  >   builtin/revert.c                  |  4 ----
>  >   sequencer.c                       | 18 ++++++++++--------
>  >   t/t3505-cherry-pick-empty.sh      |  5 +++++
>  >   4 files changed, 22 insertions(+), 15 deletions(-)
>  >
>  > diff --git a/Documentation/git-cherry-pick.txt
> b/Documentation/git-cherry-pick.txt
>  > index fdcad3d200..806295a730 100644
>  > --- a/Documentation/git-cherry-pick.txt
>  > +++ b/Documentation/git-cherry-pick.txt
>  > @@ -131,8 +131,8 @@ effect to your index in a row.
>  >       even without this option.  Note also, that use of this option only
>  >       keeps commits that were initially empty (i.e. the commit
> recorded the
>  >       same tree as its parent).  Commits which are made empty due to a
>  > -    previous commit are dropped.  To force the inclusion of those
> commits
>  > -    use `--keep-redundant-commits`.
>  > +    previous commit will cause the cherry-pick to fail.  To force the
>  > +    inclusion of those commits use `--keep-redundant-commits`.
>  >
>  >   --allow-empty-message::
>  >       By default, cherry-picking a commit with an empty message will
> fail.
>  > @@ -144,7 +144,11 @@ effect to your index in a row.
>  >       current history, it will become empty.  By default these
>  >       redundant commits cause `cherry-pick` to stop so the user can
>  >       examine the commit. This option overrides that behavior and
>  > -    creates an empty commit object.  Implies `--allow-empty`.
>  > +    creates an empty commit object. Note that use of this option only
>  > +    results in an empty commit when the commit was not initially empty,
>  > +    but rather became empty due to a previous commit. Commits that were
>  > +    initially empty will cause the cherry-pick to fail. To force the
>  > +    inclusion of those commits use `--allow-empty`.
>  >
>  >   --strategy=<strategy>::
>  >       Use the given merge strategy.  Should only be used once.
>  > diff --git a/builtin/revert.c b/builtin/revert.c
>  > index e6f9a1ad26..b2cfde7a87 100644
>  > --- a/builtin/revert.c
>  > +++ b/builtin/revert.c
>  > @@ -136,10 +136,6 @@ static int run_sequencer(int argc, const char
> **argv, const char *prefix,
>  >       prepare_repo_settings(the_repository);
>  >       the_repository->settings.command_requires_full_index = 0;
>  >
>  > -    /* implies allow_empty */
>  > -    if (opts->keep_redundant_commits)
>  > -        opts->allow_empty = 1;
>
> I'm wary of this, especially after Juino's comments in
> https://lore.kernel.org/git/xmqqy1cfnca7.fsf@gitster.g/

As noted above, I've split this commit into two, and am open to
discussing dropping the functional change to `--keep-redundant-commits`

>  >       if (cleanup_arg) {
>  >           opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1);
>  >           opts->explicit_cleanup = 1;
>  > diff --git a/sequencer.c b/sequencer.c
>  > index d584cac8ed..582bde8d46 100644
>  > --- a/sequencer.c
>  > +++ b/sequencer.c
>  > @@ -1739,22 +1739,24 @@ static int allow_empty(struct repository *r,
>  >        *
>  >        * (4) we allow both.
>  >        */
>
> The comment above should be updated if we change the behavior of this
> function.

Of course, good catch.

> I don't think we want to hide the error here or below from
> originally_empty()
>
>  >       if (!index_unchanged)
>  >           return 0; /* we do not have to say --allow-empty */
>  >
>  > -    if (opts->keep_redundant_commits)
>  > -        return 1;
>  > -

Agreed, will address in v2 as mentioned above.

Thanks,
Brian Lyles

Brian Lyles Jan. 28, 2024, 4:36 p.m. UTC | #8

[+cc Junio]

On Sat, Jan 27, 2024 at 5:30 PM Brian Lyles <[e-mail]> wrote:

> > > For some amount of backwards compatibility with the existing code and
> > > tests, I have opted to preserve the behavior of returning 0 when:
> > >
> > > - `allow_empty` is specified, and
> > > - either `is_index_unchanged` or `is_original_commit_empty` indicates an
> > >    error
> >
> > I'm not sure that is a good idea as it is hiding an error that we didn't
> > hit before because we returned early.
>
> I think you're right -- Previously the error could not have been hit,
> but now it can. An error is still an error, and we should handle it
> regardless of how `allow_empty` was set. I'll address this in v2 by
> simply returning the error.

As I dig into this more, I'm noticing that this may have unintended side
effects that I'm unsure of. After making this change, I noticed a couple
of failures in the cherry-pick test suite. The others may be a knock-on
of this initial failure:

    expecting success of 3501.8 'cherry-pick on unborn branch':
            git checkout --orphan unborn &&
            git rm --cached -r . &&
            rm -rf * &&
            git cherry-pick initial &&
            git diff --quiet initial &&
            test_cmp_rev ! initial HEAD

    A       extra_file
    Switched to a new branch 'unborn'
    rm 'extra_file'
    rm 'spoo'
    error: could not resolve HEAD commit
    fatal: cherry-pick failed
    not ok 8 - cherry-pick on unborn branch
    #
    #               git checkout --orphan unborn &&
    #               git rm --cached -r . &&
    #               rm -rf * &&
    #               git cherry-pick initial &&
    #               git diff --quiet initial &&
    #               test_cmp_rev ! initial HEAD
    #

It looks like this is caused specifically by not hiding the error from
`index_unchanged`

    index_unchanged = is_index_unchanged(r);
    if (index_unchanged < 0) {
        return index_unchanged;
    }

At this point, my inexperience with the git codebase and these more edge
case scenarios starts to show. I'm unsure how to best approach this. It
seems that exposing these errors when `allow_empty` is not set causes
the entire cherry-pick to fail in situations where it would not
previously. Here is what that same test looks like prior to any of my
changes from this series:

    expecting success of 3501.8 'cherry-pick on unborn branch':
            git checkout --orphan unborn &&
            git rm --cached -r . &&
            rm -rf * &&
            git cherry-pick initial &&
            git diff --quiet initial &&
            test_cmp_rev ! initial HEAD

    A       extra_file
    Switched to a new branch 'unborn'
    rm 'extra_file'
    rm 'spoo'
    [unborn 38e6d75] initial
     Author: A U Thor <[e-mail]>
     Date: Thu Apr 7 15:13:13 2005 -0700
     1 file changed, 15 insertions(+)
     create mode 100644 oops
    ok 8 - cherry-pick on unborn branch

Conceptually I definitely agree that it seems odd to hide these errors
just because `allow_empty` was not set, but I fear this to be a breaking
change for which I don't have a good understanding of the impact.

Any guidance here would be appreciated.

Thank you,
Brian Lyles

Phillip Wood Jan. 29, 2024, 10:55 a.m. UTC | #9

Hi Brian

On 28/01/2024 16:36, Brian Lyles wrote:
> [+cc Junio]
> 
> On Sat, Jan 27, 2024 at 5:30 PM Brian Lyles <[e-mail]> wrote:
> 
>>>> For some amount of backwards compatibility with the existing code and
>>>> tests, I have opted to preserve the behavior of returning 0 when:
>>>>
>>>> - `allow_empty` is specified, and
>>>> - either `is_index_unchanged` or `is_original_commit_empty` indicates an
>>>>     error
>>>
>>> I'm not sure that is a good idea as it is hiding an error that we didn't
>>> hit before because we returned early.
>>
>> I think you're right -- Previously the error could not have been hit,
>> but now it can. An error is still an error, and we should handle it
>> regardless of how `allow_empty` was set. I'll address this in v2 by
>> simply returning the error.
> 
> As I dig into this more, I'm noticing that this may have unintended side
> effects that I'm unsure of. After making this change, I noticed a couple
> of failures in the cherry-pick test suite. The others may be a knock-on
> of this initial failure:
> 
>      expecting success of 3501.8 'cherry-pick on unborn branch':
>              git checkout --orphan unborn &&
>              git rm --cached -r . &&
>              rm -rf * &&
>              git cherry-pick initial &&
>              git diff --quiet initial &&
>              test_cmp_rev ! initial HEAD
> 
>      A       extra_file
>      Switched to a new branch 'unborn'
>      rm 'extra_file'
>      rm 'spoo'
>      error: could not resolve HEAD commit
>      fatal: cherry-pick failed
>      not ok 8 - cherry-pick on unborn branch
>      #
>      #               git checkout --orphan unborn &&
>      #               git rm --cached -r . &&
>      #               rm -rf * &&
>      #               git cherry-pick initial &&
>      #               git diff --quiet initial &&
>      #               test_cmp_rev ! initial HEAD
>      #
> 
> It looks like this is caused specifically by not hiding the error from
> `index_unchanged`

Oh dear, that's a pain. I haven't checked but suspect we already hit 
this when running

     git cherry-pick --allow-empty

on an orphan checkout. In do_pick_commit() we treat an error reading 
HEAD as an unborn branch so I think we could do the same here. If the 
branch is unborn then we can use the_hash_algo->empty_tree as the tree 
to compare to.

Best Wishes

Phillip

Phillip Wood Feb. 1, 2024, 10:57 a.m. UTC | #10

Hi Brian

On 27/01/2024 23:30, Brian Lyles wrote:
> On Tue, Jan 23, 2024 at 8:23 AM Phillip Wood <[e-mail]> wrote:
>> On 19/01/2024 05:59, [e-mail] wrote:
>>> From: Brian Lyles <[e-mail]>
>>>
>>> Previously, a consumer of the sequencer that wishes to take advantage of
>>> either the `keep_redundant_commits` or `drop_redundant_commits` feature
>>> must also specify `allow_empty`.
>>>
>>> The only consumer of `drop_redundant_commits` is `git-rebase`, which
>>> already allows empty commits by default and simply always enables
>>> `allow_empty`. `keep_redundant_commits` was also consumed by
>>> `git-cherry-pick`, which had to specify `allow-empty` when
>>> `keep_redundant_commits` was specified in order for the sequencer's
>>> `allow_empty()` to actually respect `keep_redundant_commits`.
>>
>> I think it might be more persuasive to start the commit message by
>> explaining what user visible change you're trying to make and why rather
>> than concentrating on the implementation details.
> 
> I struggled a bit with this initially because the motivation behind the
> change in this particular commit was driven by a technical issue in my
> mind. The side-effect with git-cherry-pick(s) `--allow-empty` and
> `--keep-redundant-commits` was mildly problematic, but less concerning
> that the future problem that we'd have once git-cherry-pick(1) got the
> more robust `--empty` option in a later commit in this series.
> 
> I think my problem came down to this commit trying to solve two problems
> at once -- the underlying technical concern _and_ the git-cherry-pick(1)
> behavior.
> 
> In v2, I intend to break this commit into two:
> 
> - Update `allow_empty()` to not require `allow_empty`, but without
>    actually changing any consumers (and thus without making any
>    functional change)
> - Update git-cherry-pick(1) such that `--keep-redundant-commits` no
>    longer implies `--allow-empty`.
> 
> This allows me to better justify the technical change technically and
> the functional change functionally, while also making it easier to drop
> the functional change if we decide that a breaking change is not
> warranted to address this.

That sounds like a good strategy. I'm wondering if we should not change 
the behavior of `--keep-redundant-commits` to avoid breaking existing 
users but have `--empty=keep|drop` not imply `--allow-empty`. What ever 
we do we'll annoy someone. It is confusing to have subtly different 
behaviors for `--keep-redundant-commits` and `--empty=keep` but it 
avoids breaking existing users. If we change `--keep-redundant-commits` 
we potentially upset existing users but we don't confuse others with the 
subtle difference between the two.

>> Do you have a practical example of where you want to keep the commits
>> that become empty but not the ones that start empty? I agree there is a
>> distinction but I think the common case is that the user wants to keep
>> both types of empty commit or none. I'm not against giving the user the
>> option to keep one or the other if it is useful but I'm wary of changing
>> the default.
> 
> That practical example is documented in the initial discussion[1], which
> I should have ought to have linked in a cover letter for this series
> (and will do so in v2). I'll avoid copying the details here, but we'd
> very much like to be able to programmatically drop the commits that
> become empty when doing the automated cherry-pick described there.
> 
> [1]: https://lore.kernel.org/git/CAHPHrSevBdQF0BisR8VK=jM=wj1dTUYEVrv31gLerAzL9=[e-mail]/

Maybe I've missed something but that seems to be an argument for 
implementing `--empty=drop` which is completely reasonable but doesn't 
explain why someone using `--keep-redundant-commits` would want to keep 
the commits that become empty while dropping the commits that start empty.

> [...]
>> Thank you for being clear about the change in behavior, as I said above
>> I'm wary of changing the default unless there is a compelling reason but
>> I'm happy to support
>>
>>       git cherry-pick --keep-redundant-commits --no-allow-empty
>>
>> if it is needed.
> 
> I totally understand being wary here.
> 
> I've certainly convinced myself that having the future `--empty=drop`
> behavior introduced later in this patch should not imply
> `--allow-empty`.

I agree with that

> I also _think_ that the existing behavior of `--keep-redundant-commits`
> is probably technically not ideal or correct, but could be convinced
> that changing it now is not worthwhile. I will defer to group consensus
> here.

There is definitely an argument that the existing behavior conflates the 
two flavors of empty commit. I think one can also argue that the 
conflation is beneficial as most of the time users don't care about the 
distinction when using `--keep-redundant-commits` and so it is not worth 
changing the behavior of the existing option.

I sounds like you've got a good plan for v2, I look forward to reading it.

Best Wishes

Phillip

Brian Lyles Feb. 10, 2024, 4:34 a.m. UTC | #11

Hi Phillip

On Thu, Feb 1, 2024 at 4:57 AM Phillip Wood <[e-mail]>
wrote:

> That sounds like a good strategy. I'm wondering if we should not change 
> the behavior of `--keep-redundant-commits` to avoid breaking existing 
> users but have `--empty=keep|drop` not imply `--allow-empty`. What ever 
> we do we'll annoy someone. It is confusing to have subtly different 
> behaviors for `--keep-redundant-commits` and `--empty=keep` but it 
> avoids breaking existing users. If we change `--keep-redundant-commits` 
> we potentially upset existing users but we don't confuse others with the 
> subtle difference between the two.

I am starting to come around to this approach for this particular series
just to avoid anything potentially controversial holding it up.

>>> Do you have a practical example of where you want to keep the commits
>>> that become empty but not the ones that start empty? I agree there is a
>>> distinction but I think the common case is that the user wants to keep
>>> both types of empty commit or none. I'm not against giving the user the
>>> option to keep one or the other if it is useful but I'm wary of changing
>>> the default.
>> 
>> That practical example is documented in the initial discussion[1], which
>> I should have ought to have linked in a cover letter for this series
>> (and will do so in v2). I'll avoid copying the details here, but we'd
>> very much like to be able to programmatically drop the commits that
>> become empty when doing the automated cherry-pick described there.
>> 
>> [1]: https://lore.kernel.org/git/CAHPHrSevBdQF0BisR8VK=jM=wj1dTUYEVrv31gLerAzL9=[e-mail]/
> 
> Maybe I've missed something but that seems to be an argument for 
> implementing `--empty=drop` which is completely reasonable but doesn't 
> explain why someone using `--keep-redundant-commits` would want to keep 
> the commits that become empty while dropping the commits that start empty.

Nope, you didn't miss something -- I just didn't read properly.

I don't have a concrete example here, but the behavior *feels* quite odd
to me. But I suppose that's not a good enough reason to make a breaking
change.

Brian Lyles Feb. 10, 2024, 5:50 a.m. UTC | #12

On Mon, Jan 29, 2024 at 4:55 AM Phillip Wood <[e-mail]>
wrote:

>>>> I'm not sure that is a good idea as it is hiding an error that we didn't
>>>> hit before because we returned early.
>>>
>>> I think you're right -- Previously the error could not have been hit,
>>> but now it can. An error is still an error, and we should handle it
>>> regardless of how `allow_empty` was set. I'll address this in v2 by
>>> simply returning the error.
>> 
>> As I dig into this more, I'm noticing that this may have unintended side
>> effects that I'm unsure of. After making this change, I noticed a couple
>> of failures in the cherry-pick test suite. The others may be a knock-on
>> of this initial failure:
>> 
>>      expecting success of 3501.8 'cherry-pick on unborn branch':
>>              git checkout --orphan unborn &&
>>              git rm --cached -r . &&
>>              rm -rf * &&
>>              git cherry-pick initial &&
>>              git diff --quiet initial &&
>>              test_cmp_rev ! initial HEAD
>> 
>>      A       extra_file
>>      Switched to a new branch 'unborn'
>>      rm 'extra_file'
>>      rm 'spoo'
>>      error: could not resolve HEAD commit
>>      fatal: cherry-pick failed
>>      not ok 8 - cherry-pick on unborn branch
>>      #
>>      #               git checkout --orphan unborn &&
>>      #               git rm --cached -r . &&
>>      #               rm -rf * &&
>>      #               git cherry-pick initial &&
>>      #               git diff --quiet initial &&
>>      #               test_cmp_rev ! initial HEAD
>>      #
>> 
>> It looks like this is caused specifically by not hiding the error from
>> `index_unchanged`
> 
> Oh dear, that's a pain. I haven't checked but suspect we already hit 
> this when running
> 
>      git cherry-pick --allow-empty
> 
> on an orphan checkout. In do_pick_commit() we treat an error reading 
> HEAD as an unborn branch so I think we could do the same here. If the 
> branch is unborn then we can use the_hash_algo->empty_tree as the tree 
> to compare to.

You're correct that `git cherry-pick --allow-empty` on an unborn branch
hits the same error today, and that using `empty_tree` seems to resolve
it. Thank you for this tip, I'll include a fix and corresponding test as
a separate commit in v2.

Patch

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index fdcad3d200..806295a730 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -131,8 +131,8 @@  effect to your index in a row.
 	even without this option.  Note also, that use of this option only
 	keeps commits that were initially empty (i.e. the commit recorded the
 	same tree as its parent).  Commits which are made empty due to a
-	previous commit are dropped.  To force the inclusion of those commits
-	use `--keep-redundant-commits`.
+	previous commit will cause the cherry-pick to fail.  To force the
+	inclusion of those commits use `--keep-redundant-commits`.
 
 --allow-empty-message::
 	By default, cherry-picking a commit with an empty message will fail.
@@ -144,7 +144,11 @@  effect to your index in a row.
 	current history, it will become empty.  By default these
 	redundant commits cause `cherry-pick` to stop so the user can
 	examine the commit. This option overrides that behavior and
-	creates an empty commit object.  Implies `--allow-empty`.
+	creates an empty commit object. Note that use of this option only
+	results in an empty commit when the commit was not initially empty,
+	but rather became empty due to a previous commit. Commits that were
+	initially empty will cause the cherry-pick to fail. To force the
+	inclusion of those commits use `--allow-empty`.
 
 --strategy=<strategy>::
 	Use the given merge strategy.  Should only be used once.
diff --git a/builtin/revert.c b/builtin/revert.c
index e6f9a1ad26..b2cfde7a87 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -136,10 +136,6 @@  static int run_sequencer(int argc, const char **argv, const char *prefix,
 	prepare_repo_settings(the_repository);
 	the_repository->settings.command_requires_full_index = 0;
 
-	/* implies allow_empty */
-	if (opts->keep_redundant_commits)
-		opts->allow_empty = 1;
-
 	if (cleanup_arg) {
 		opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1);
 		opts->explicit_cleanup = 1;
diff --git a/sequencer.c b/sequencer.c
index d584cac8ed..582bde8d46 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1739,22 +1739,24 @@  static int allow_empty(struct repository *r,
 	 *
 	 * (4) we allow both.
 	 */
-	if (!opts->allow_empty)
-		return 0; /* let "git commit" barf as necessary */
-
 	index_unchanged = is_index_unchanged(r);
-	if (index_unchanged < 0)
+	if (index_unchanged < 0) {
+		if (!opts->allow_empty)
+			return 0;
 		return index_unchanged;
+	}
 	if (!index_unchanged)
 		return 0; /* we do not have to say --allow-empty */
 
-	if (opts->keep_redundant_commits)
-		return 1;
-
 	originally_empty = is_original_commit_empty(commit);
-	if (originally_empty < 0)
+	if (originally_empty < 0) {
+		if (!opts->allow_empty)
+			return 0;
 		return originally_empty;
+	}
 	if (originally_empty)
+		return opts->allow_empty;
+	else if (opts->keep_redundant_commits)
 		return 1;
 	else if (opts->drop_redundant_commits)
 		return 2;
diff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh
index eba3c38d5a..6adfd25351 100755
--- a/t/t3505-cherry-pick-empty.sh
+++ b/t/t3505-cherry-pick-empty.sh
@@ -59,6 +59,11 @@  test_expect_success 'cherry pick an empty non-ff commit without --allow-empty' '
 	test_must_fail git cherry-pick empty-change-branch
 '
 
+test_expect_success 'cherry pick an empty non-ff commit with --keep-redundant-commits' '
+	git checkout main &&
+	test_must_fail git cherry-pick --keep-redundant-commits empty-change-branch
+'
+
 test_expect_success 'cherry pick an empty non-ff commit with --allow-empty' '
 	git checkout main &&
 	git cherry-pick --allow-empty empty-change-branch