Re: [PATCH v2 8/8] cherry-pick: add `--empty` for more robust redundant commit handling

[phillip.wood123@gmail.com]

Hi Brian

On 10/02/2024 07:43, Brian Lyles wrote:
> As with git-rebase(1) and git-am(1), git-cherry-pick(1) can result in a
> commit being made redundant if the content from the picked commit is
> already present in the target history. However, git-cherry-pick(1) does
> not have the same options available that git-rebase(1) and git-am(1) have.
> 
> There are three things that can be done with these redundant commits:
> drop them, keep them, or have the cherry-pick stop and wait for the user
> to take an action. git-rebase(1) has the `--empty` option added in commit
> e98c4269c8 (rebase (interactive-backend): fix handling of commits that
> become empty, 2020-02-15), which handles all three of these scenarios.
> Similarly, git-am(1) got its own `--empty` in 7c096b8d61 (am: support
> --empty=<option> to handle empty patches, 2021-12-09).
> 
> git-cherry-pick(1), on the other hand, only supports two of the three
> possiblities: Keep the redundant commits via `--keep-redundant-commits`,
> or have the cherry-pick fail by not specifying that option. There is no
> way to automatically drop redundant commits.
> 
> In order to bring git-cherry-pick(1) more in-line with git-rebase(1) and
> git-am(1), this commit adds an `--empty` option to git-cherry-pick(1). It
> has the same three options (keep, drop, and stop), and largely behaves
> the same. The notable difference is that for git-cherry-pick(1), the
> default will be `stop`, which maintains the current behavior when the
> option is not specified.
> 
> 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 leaning towards leaving `--keep-redundant-commits` alone. That 
introduces an inconsistency between `--keep-redundant-commits` and 
`--empty=keep` as the latter does not imply `--allow-empty` but it does 
avoid breaking existing users. We could document 
`--keep-redundant-commits` as predating `--empty` and behaving like 
`--empty=keep --allow-empty`. The documentation and implementation of 
the new option look good modulo the typo that has already been pointed 
out and a couple of small comments below.

> +enum empty_action {
> +	EMPTY_COMMIT_UNSPECIFIED = 0,

We tend to use -1 for unspecified options

> +	STOP_ON_EMPTY_COMMIT,      /* output errors and stop in the middle of a cherry-pick */
> +	DROP_EMPTY_COMMIT,         /* skip with a notice message */
> +	KEEP_EMPTY_COMMIT,         /* keep recording as empty commits */
> +};


> +test_expect_success 'cherry-pick persists --empty=stop correctly' '
> +	pristine_detach initial &&
> +	# to make sure that the session to cherry-pick a sequence
> +	# gets interrupted, use a high-enough number that is larger
> +	# than the number of parents of any commit we have created
> +	mainline=4 &&
> +	test_expect_code 128 git cherry-pick -s -m $mainline --empty=stop initial..anotherpick &&
> +	test_path_is_file .git/sequencer/opts &&
> +	test_must_fail git config --file=.git/sequencer/opts --get-all options.keep-redundant-commits &&
> +	test_must_fail git config --file=.git/sequencer/opts --get-all options.drop-redundant-commits
> +'

Thanks for adding these tests to check that the --empty option persists. 
Usually for tests like this we prefer to check the user visible behavior 
rather than the implementation details (I suspect we have some older 
tests that do the latter). To check the behavior we usually arrange for 
a merge conflict but using -m is a creative alternative, then we'd run 
"git cherry-pick --continue" and check that the commits that become 
empty have been preserved or dropped or that the cherry-pick stops.

Best Wishes

Phillip