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

[Junio C Hamano <gitster@pobox.com>]

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

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