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
