Does extending `--empty` to git-cherry-pick make sense?

Does extending `--empty` to git-cherry-pick make sense?

[Brian Lyles]

The `--empty=(keep|drop|ask)` option added to git-rebase in
e98c4269c86019bfe057a91b4305f784365b6f0b seems like it would be
applicable to git-cherry-pick (and maybe git-revert for completeness?).
While the shared sequencer code likely would already handle this fairly
well (at a cursory glance from someone with very little knowledge of C
or git's codebase, admittedly), it's only exposed via git-rebase.

The use case in which this came up involves a semi-automated workflow
for moving commits between branches. At a high level, a tool is:

- Identifying commits to be picked based on a specific trailer value
- Using git-cherry-pick with `-x` to pick those commits
- Relying on the presence of the `(cherry picked from commit ...)`
  trailer to avoid re-picking commits that have already been picked

If the picked commits are then rewritten in the upstream such that there
are squashes or fixups, that trailer can end up missing in the upstream.
The next time the tool runs, it will end up trying to re-pick commits
that are already represented there. As a result, those commits become
empty upon being picked a second time and the cherry-pick ends up
breaking for the user to resolve:

    $ git cherry-pick main
    On branch feature
    You are currently cherry-picking commit cfd7cd9.
      (all conflicts fixed: run "git cherry-pick --continue")
      (use "git cherry-pick --skip" to skip this patch)
      (use "git cherry-pick --abort" to cancel the cherry-pick operation)

    nothing to commit, working tree clean
    The previous cherry-pick is now empty, possibly due to conflict resolution.
    If you wish to commit it anyway, use:

        git commit --allow-empty

    Otherwise, please use 'git cherry-pick --skip'

I'll spare the details, but we do expect this to happen with enough
frequency that we'd really like to be able to prevent it. The
`--empty=drop` option sounds like exactly what we want here:

    --empty=(drop|keep|ask)
    How to handle commits that are not empty to start and are not
    clean cherry-picks of any upstream commit, but which become
    empty after rebasing (because they contain a subset of already
    upstream changes). With drop (the default), commits that become
    empty are dropped.

Is there any real barrier to exposing that option to git-cherry-pick as
well? Was this an oversight, or intentionally left out? The
corresponding commit message doesn't seem to indicate any specific
reason for limiting it to git-rebase.

Thanks,
-Brian Lyles

Re: Does extending `--empty` to git-cherry-pick make sense?

[Taylor Blau]

[+cc Elijah]

On Thu, Jan 04, 2024 at 12:57:18AM -0600, Brian Lyles wrote:
> Is there any real barrier to exposing that option to git-cherry-pick as
> well? Was this an oversight, or intentionally left out? The
> corresponding commit message doesn't seem to indicate any specific
> reason for limiting it to git-rebase.

I am not nearly as familiar with this code as Elijah is, but this
certainly appears possible by setting the `drop_redundant_commits` and
`keep_redundant_commits` flags in the replay_opts struct.

I don't see any fundamental reason why cherry-pick shouldn't have the
same functionality.

Thanks,
Taylor

Re: Does extending `--empty` to git-cherry-pick make sense?

[Elijah Newren]

On Thu, Jan 4, 2024 at 11:33 AM Taylor Blau <me@ttaylorr.com> wrote:
>
> [+cc Elijah]
>
> On Thu, Jan 04, 2024 at 12:57:18AM -0600, Brian Lyles wrote:
> > Is there any real barrier to exposing that option to git-cherry-pick as
> > well? Was this an oversight, or intentionally left out? The
> > corresponding commit message doesn't seem to indicate any specific
> > reason for limiting it to git-rebase.
>
> I am not nearly as familiar with this code as Elijah is, but this
> certainly appears possible by setting the `drop_redundant_commits` and
> `keep_redundant_commits` flags in the replay_opts struct.
>
> I don't see any fundamental reason why cherry-pick shouldn't have the
> same functionality.

I was indeed focused on simply getting the multiple rebase backends to
have consistent behavior (we had like 4-5 backends at the time,
right?) and just didn't consider cherry-pick at the time.  Now that
those are more consistent (though there's still work to be done on
that end too), cherry-pick and rebase really ought to share a lot more
between each other, from command line flags, to temporary control
files written, to more shared code paths.  Adding an --empty flag to
cherry-pick as a step along the way makes sense.

Note that git-am also gained a similar flag in the meantime, but
changed the names slightly: --empty=(stop,drop,keep).  I think 'stop'
is a better name than 'ask', and we really should make rebase suggest
'stop' instead (but keep 'ask' as a synonym for 'stop', for backwards
compatibility).  Also, I kind of want to replace 'keep' with 'roll' in
the --empty option for both git-am and git-rebase, while keeping
'keep' as a synonym for 'roll'.  But I'm not sure if others on the
list would go along with it...

Re: Does extending `--empty` to git-cherry-pick make sense?

[Brian Lyles]

On Thu, Jan 4, 2024 at 8:28 PM Elijah Newren <newren@gmail.com> wrote:
>
> I was indeed focused on simply getting the multiple rebase backends to
> have consistent behavior (we had like 4-5 backends at the time,
> right?) and just didn't consider cherry-pick at the time.  Now that
> those are more consistent (though there's still work to be done on
> that end too), cherry-pick and rebase really ought to share a lot more
> between each other, from command line flags, to temporary control
> files written, to more shared code paths.  Adding an --empty flag to
> cherry-pick as a step along the way makes sense.

I appreciate the insight from both of you.

It sounds like I'll be diving into some C code for the first time in
over a decade, then! I'll plan to take a crack at this soon.

-Brian Lyles