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

> 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.

>> 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/

> [...]
>> 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`.