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

[phillip.wood123@gmail.com]

Hi Brian

On 26/02/2024 03:32, Brian Lyles wrote:
> Hi Phillip and Junio
> On Fri, Feb 23, 2024 at 12:08 AM Brian Lyles <brianmlyles@gmail.com> wrote:
>>
>> On Thu, Feb 22, 2024 at 10:35 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>>> I agree that if we were starting from scratch there would be no reason
>>> to tie --apply-empty and --keep-redundant-commits together but I'm not
>>> sure it is worth the disruption of changing it now. We're about to add
>>> empty=keep which won't imply --allow-empty for anyone who wants that
>>> behavior and I still tend to think the practical effect of implying
>>> --allow-empty with --keep-redundant-commits is largely beneficial as I'm
>>> skeptical that users want to keep commits that become empty but not the
>>> ones that started empty.
>>
>> I think that's fair. I am okay dropping this potentially disruptive
>> change.
>>
>> It sounds like you are on board with `--empty=keep` not having the same
>> implication?

Yes indeed

>> That said...
>>
>> On Thu, Feb 22, 2024 at 12:41 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>>> I do not quite see a good reason to do the opposite, dropping
>>> commits that started out as empty but keeping the ones that have
>>> become empty.  Such a behaviour has additional downside that after
>>> such a cherry-pick, when you cherry-pick the resulting history onto
>>> yet another base, your precious "were not empty but have become so
>>> during the initial cherry-pick" commits will appear as commits that
>>> were empty from the start.  So I do not see much reason to allow the
>>> decoupling, even with the new "empty=keep" thing that does not imply
>>> "allow-empty".
>>
>> Junio -- can you clarify this part?
>>
>>> So I do not see much reason to allow the decoupling, even with the new
>>> "empty=keep" thing that does not imply "allow-empty"
>>
>> I'm not 100% sure if you are saying that you want `--empty=keep` to
>> *also* imply `--allow-empty`, or that you simply want
>> `--keep-redundant-commits` to continue implying `--allow-empty`
>> *despite* the new `--empty=keep` no implying the same.

FWIW I read it as the latter, but I can't claim to know what was in 
Junio's mind when he wrote it.

>> On the one hand, I agree with Phillip's sentiment of "if we were
>> starting from scratch there would be no reason to tie --apply-empty and
>> --keep-redundant-commits together" (though your points perhaps provide
>> such a reason). On the other, if both `--keep-redundant-commits` and
>> `--empty=keep` behave the same way, it makes sense to soft-deprecate
>> `--keep-redundant-commits` as I have currently done later in this
>> series. If they do not behave the same way, that deprecation makes less
>> sense and we have two very similar (but not quite identical) options.
>>
>> Just to make sure we're on the same page, the options I see are:
>>
>> - (A): Neither `--keep-redundant-commits` nor `--empty=keep` imply
>>    `--allow-empty`, `--keep-redundant-commits` is soft-deprecated
>> - (B): Both `--keep-redundant-commits` and `--empty=keep` imply
>>    `--allow-empty`, `--keep-redundant-commits` is soft-deprecated
>> - (C): Both `--keep-redundant-commits` and `--empty=keep` imply
>>    `--allow-empty`, `--keep-redundant-commits` is *not* soft-deprecated
>>    as it is more descriptive as noted by Junio here[1]
>> - (D): `--keep-redundant-commits` continues to imply `--allow-empty` but
>>    `--empty=keep` does not. `--keep-redundant-commits` is *not*
>>    soft-deprecated as it behaves differently.
>>
>> (A) represents this v2 of the patch.
>>
>> I'm coming around to (B) based on Junio's workflow concerns, but to be
>> honest I am fine with any of these options. Junio, I *think* you're
>> saying you'd prefer (B) or (C)? Phillip, it sounds like you are okay
>> with (D) based on your response -- how do you feel about (B)?

Yes, I'd prefer (D) as I think it gets confusing if some values of 
--empty imply --allow-empty and others don't. I could live with (B) though.

Best Wishes

Phillip