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

Hi Brian

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

On 19/01/2024 05:59, brianmlyles@gmail.com wrote:

 >   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