Hi Brian
On 23/02/2024 06:58, Brian Lyles wrote:
On Thu, Feb 22, 2024 at 10:36 AM <phillip.wood123@xxxxxxxxx> wrote:
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.
I think I'm on board with leaving `--keep-redundant-commits` alone. I'm
on the fence about having `--empty=keep` imply `--allow-empty` after
seeing Junio's concerns. I laid out the options that I see in a reply to
patch 6/8[1] and would appreciate input there. I'll adjust the details
of this commit in v3 based on what we decide there.
I'll take a look at that in the next couple of days
[1]: https://lore.kernel.org/git/17b666ca6c4b7561.70b1dd9aae081c6e.203dcd72f6563036@zivdesk/
+enum empty_action {
+ EMPTY_COMMIT_UNSPECIFIED = 0,
We tend to use -1 for unspecified options
Thanks, I'll update this in v3.
+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.
Indeed, I was modelling these new tests after other existing tests in
this file. While I agree with you in theory, I am hesitant to make these
new tests drastically different from the existing tests that are testing
the same mechanisms (and appear to be very intentionally testing that
the options are persisted in that config file). I'm also hesitant to
update the existing tests as part of this series (primarily due to a
lack of familiarity, and partially to avoid scope creep of the series).
I certainly don't think it should be up to you to update the existing
tests. I'm not sure adding more tests in the same pattern is a good idea
though. Apart from the fact that they are testing an implementation
detail rather than the user facing behavior they don't actually check
that the option is respected by "git cherry-pick --continue", only that
we save it when stopping for a conflict resolution.
How concerned are you about the current implementation? Does it make
sense to you to defer this suggestion to a future series that cleans up
the tests to do more user-oriented checks?
I think adding tests that follow a pattern we want to change is just
storing up work for the future and makes it less likely we'll improve
things because it will be more work to do so.
Best Wishes
Phillip
