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

>
> 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, brianmlyles@gmail.com wrote:
> > From: Brian Lyles <brianmlyles@gmail.com>
> >
> > 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.

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

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

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

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

> > 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 <brianmlyles@gmail.com>
> > ---
> >
> > 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.

>  > [...]> +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).

> 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:
> > From: Brian Lyles <brianmlyles@gmail.com>
>  >   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/

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

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