Re: [PATCH v2 3/8] cherry-pick: add --skip-empty option

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 3, 2013, 21:45 UTC
Message-ID: <7vehcidg1i.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CAMP44s1GQ2iwwscx4Sx1cxK0U6yWWYvOJqDk_FO_GkFcNKREYw@mail.gmail.com>

Felipe Contreras <felipe.contreras@gmail.com> writes:

Show 29 quoted lines

> On Mon, Jun 3, 2013 at 1:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> Pretty much what it says on the tin.
>>>
>>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>> ---
>>>  Documentation/git-cherry-pick.txt   |  3 +++
>>>  builtin/revert.c                    |  2 ++
>>>  sequencer.c                         |  6 ++++++
>>>  sequencer.h                         |  1 +
>>>  t/t3508-cherry-pick-many-commits.sh | 13 +++++++++++++
>>>  5 files changed, 25 insertions(+)
>>>
>>> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
>>> index c205d23..fccd936 100644
>>> --- a/Documentation/git-cherry-pick.txt
>>> +++ b/Documentation/git-cherry-pick.txt
>>> @@ -129,6 +129,9 @@ effect to your index in a row.
>>>       redundant commits are ignored.  This option overrides that behavior and
>>>       creates an empty commit object.  Implies `--allow-empty`.
>>>
>>> +--skip-empty::
>>> +     Instead of failing, skip commits that are or become empty.
>>
>> Not quite sure.  Is this "instead of recording an empty commit,"
>> (which may or may not fail depending on the allow-empty settings)?
>
> We are explaining --skip-empty, not --allow-empty.

Exactly. That is why I found the "Instead of failing" questionable. It is very easy to read the above as "commits that are or become empty usually causes the command to fail, and this is a way to cause it not to fail.".

It is true that

    cherry-pick A

when A becomes empty, dies.  But

    cherry-pick --allow-empty A

when A becomes empty, does not fail, but still does create an empty commit. A large difference with --skip-empty, which applies to use scenario different from --allow-empty was meant to address, is that

    cherry-pick --skip-empty A

when A becomes empty, does not fail and does not create an empty commit, either.

I think just "Skip commits that are or become empty" without saying "Instead of failing," is fine, too.