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

[phillip.wood123@gmail.com]

Hi Brian

On 23/02/2024 06:58, Brian Lyles wrote:
> 
> On Thu, Feb 22, 2024 at 10:36 AM <phillip.wood123@gmail.com> 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