Re: [PATCH 4/4] cherry-pick: Add `--empty` for more robust redundant commit handling

[Brian Lyles <brianmlyles@gmail.com>]

Hi Junio

On Tue, Jan 23, 2024 at 12:01 PM Junio C Hamano <gitster@pobox.com> wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
> > Thanks for the well explained commit message
>
> ;-)
>
> >> The `--keep-redundant-commits` option will be documented as a deprecated
> >> synonym of `--empty=keep`, and will be supported for backwards
> >> compatibility for the time being.
> >
> > I'm not sure if we need to deprecate it as in "it will be removed in
> > the future" or just reduce it prominence in favor of --empty
>
> True, especially since --empty=keep is much less descriptive and the
> part after "note that ..." below will take a long time before
> sticking in readers' brain.

I responded to this in my reply to Phillip, and CC'd you there.

> >> +--empty=(stop|drop|keep)::
> >> +    How to handle commits being cherry-picked that are redundant with
> >> +    changes already in the current history.
>
> It might make it easier to understand if we moved the description in
> 'keep' that says something about --allow-empty here, as it should
> apply to other two choices if I understand correctly.  Let me try:
>
>     This option specifies how a commit that is not originally empty
>     but becomes a no-op when cherry-picked due to earlier changes
>     already applied or already in the current history.  Regardless
>     of this this option, `cherry-pick` will fail on a commit that is
>     empty in the original history---see `--allow-empty` to pass them
>     intact.
>
> or something.  Then the description of `keep` can become just as
> short as other two, e.g. a single sentence "The commit that becomes
> empty will be kept".

Thank you for this suggestion. You are correct that the difference
between `--empty` and `allow-empty` is relevant regardless of the option
selected by the user.

In fact, this entire tidbit is somewhat duplicative with the note I
already added after the options:

> Note that commits which start empty will cause the cherry-pick to fail
> (unless `--allow-empty` is specified).

I'll clean this up in v2. Here's what I am thinking currently:

> --empty=(stop|drop|keep)::
>     How to handle commits being cherry-picked that are redundant with
>     changes already in the current history.
> +
> --
> `stop`;;
>     The cherry-pick will stop when the commit is applied, allowing
>     you to examine the commit. This is the default behavior.
> `drop`;;
>     The commit will be dropped.
> `keep`;;
>     The commit will be kept.
> --
> +
> Note that this option species how to handle a commit that 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`.
> +

Thank you,
Brian Lyles