Re: [PATCH 1/4] sequencer: Do not require `allow_empty` for redundant commit options

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Brian

On 27/01/2024 23:30, Brian Lyles wrote:
> On Tue, Jan 23, 2024 at 8:23 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>> On 19/01/2024 05:59, brianmlyles@gmail.com wrote:
>>> From: Brian Lyles <brianmlyles@gmail.com>
>>>
>>> Previously, a consumer of the sequencer that wishes to take advantage of
>>> either the `keep_redundant_commits` or `drop_redundant_commits` feature
>>> must also specify `allow_empty`.
>>>
>>> The only consumer of `drop_redundant_commits` is `git-rebase`, which
>>> already allows empty commits by default and simply always enables
>>> `allow_empty`. `keep_redundant_commits` was also consumed by
>>> `git-cherry-pick`, which had to specify `allow-empty` when
>>> `keep_redundant_commits` was specified in order for the sequencer's
>>> `allow_empty()` to actually respect `keep_redundant_commits`.
>>
>> I think it might be more persuasive to start the commit message by
>> explaining what user visible change you're trying to make and why rather
>> than concentrating on the implementation details.
> 
> I struggled a bit with this initially because the motivation behind the
> change in this particular commit was driven by a technical issue in my
> mind. The side-effect with git-cherry-pick(s) `--allow-empty` and
> `--keep-redundant-commits` was mildly problematic, but less concerning
> that the future problem that we'd have once git-cherry-pick(1) got the
> more robust `--empty` option in a later commit in this series.
> 
> I think my problem came down to this commit trying to solve two problems
> at once -- the underlying technical concern _and_ the git-cherry-pick(1)
> behavior.
> 
> In v2, I intend to break this commit into two:
> 
> - Update `allow_empty()` to not require `allow_empty`, but without
>    actually changing any consumers (and thus without making any
>    functional change)
> - Update git-cherry-pick(1) such that `--keep-redundant-commits` no
>    longer implies `--allow-empty`.
> 
> This allows me to better justify the technical change technically and
> the functional change functionally, while also making it easier to drop
> the functional change if we decide that a breaking change is not
> warranted to address this.

That sounds like a good strategy. I'm wondering if we should not change 
the behavior of `--keep-redundant-commits` to avoid breaking existing 
users but have `--empty=keep|drop` not imply `--allow-empty`. What ever 
we do we'll annoy someone. It is confusing to have subtly different 
behaviors for `--keep-redundant-commits` and `--empty=keep` but it 
avoids breaking existing users. If we change `--keep-redundant-commits` 
we potentially upset existing users but we don't confuse others with the 
subtle difference between the two.

>> Do you have a practical example of where you want to keep the commits
>> that become empty but not the ones that start empty? I agree there is a
>> distinction but I think the common case is that the user wants to keep
>> both types of empty commit or none. I'm not against giving the user the
>> option to keep one or the other if it is useful but I'm wary of changing
>> the default.
> 
> That practical example is documented in the initial discussion[1], which
> I should have ought to have linked in a cover letter for this series
> (and will do so in v2). I'll avoid copying the details here, but we'd
> very much like to be able to programmatically drop the commits that
> become empty when doing the automated cherry-pick described there.
> 
> [1]: https://lore.kernel.org/git/CAHPHrSevBdQF0BisR8VK=jM=wj1dTUYEVrv31gLerAzL9=Cd8Q@mail.gmail.com/

Maybe I've missed something but that seems to be an argument for 
implementing `--empty=drop` which is completely reasonable but doesn't 
explain why someone using `--keep-redundant-commits` would want to keep 
the commits that become empty while dropping the commits that start empty.

> [...]
>> Thank you for being clear about the change in behavior, as I said above
>> I'm wary of changing the default unless there is a compelling reason but
>> I'm happy to support
>>
>>       git cherry-pick --keep-redundant-commits --no-allow-empty
>>
>> if it is needed.
> 
> I totally understand being wary here.
> 
> I've certainly convinced myself that having the future `--empty=drop`
> behavior introduced later in this patch should not imply
> `--allow-empty`.

I agree with that

> I also _think_ that the existing behavior of `--keep-redundant-commits`
> is probably technically not ideal or correct, but could be convinced
> that changing it now is not worthwhile. I will defer to group consensus
> here.

There is definitely an argument that the existing behavior conflates the 
two flavors of empty commit. I think one can also argue that the 
conflation is beneficial as most of the time users don't care about the 
distinction when using `--keep-redundant-commits` and so it is not worth 
changing the behavior of the existing option.

I sounds like you've got a good plan for v2, I look forward to reading it.

Best Wishes

Phillip