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

From: Brian Lyles <brianmlyles@gmail.com>
Date: Mar 16, 2024, 05:20 UTC
Message-ID: <CAHPHrSfiMbU55K2=8+hJZy1cMSRbYM77pCK8BdcAPHLvapHO_A@mail.gmail.com>
In-Reply-To: <xmqq1q8edpb4.fsf@gitster.g>

Hi Phillip and Junio

Apologies in advance if this is a duplicate message -- it appears that my reply never showed up on the public archive at https://lore.kernel.org/git/ for some reason, and I'm unsure if those CC'd received it either. As such, I am resending it.

On Wed, Mar 13, 2024 at 12:17 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

> phillip.wood123@gmail.com writes:
>
>>> +Note that this option specifies how to handle a commit that was not initially
>>> +empty, but rather became empty due to a previous commit. Commits that were
>>> +initially empty will cause the cherry-pick to fail. To force the inclusion of
>>> +those commits, use `--allow-empty`.
>>
>> I found this last paragraph is slightly confusing now --empty=keep
>> implies --allow-empty. Maybe we could change the middle sentence to
>> say something like
>>
>>     With the exception of `--empty=keep` commits that were initially
>>     empty will cause the cherry-pick to fail.
>
> That is certainly easier to read and much less confusing.

I agree that this paragraph is slightly confusing. I tried this suggestion on but found it to not sit quite right, I think because the two exceptions (--empty=keep and --allow-empty) were not part of the same sentence, so it felt a little disjointed. How would you feel about the following instead, which aims to be more clear and specific about the behavior?

        Note that `--empty=drop` and `--empty=stop` only specify how to
        handle a commit that was not initially empty, but rather became
        empty due to a previous commit. Commits that were initially empty
        will still cause the cherry-pick to fail unless one of
        `--empty=keep` or `--allow-empty` are specified.

Thank you both again for your time reviewing this!

-- 
Thank you,
Brian Lyles