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. [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). 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? -- Thank you, Brian Lyles
