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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Jun 4, 2013, 17:10 UTC
Message-ID: <CAMP44s2DvtiT6hi-RoPt_bVdGr01S367usz=089E1Ht+Bvh-tA@mail.gmail.com>
In-Reply-To: <7vehcidg1i.fsf@alter.siamese.dyndns.org>

On Mon, Jun 3, 2013 at 4:45 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 53 quoted lines

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

We are not explaining --allow-empty.

What happens when you do --skip-empty --allow-empty? Somebody suggested a new option, so we could do --foo-empty=skip,allow to clarify that.

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

I think "Instead of failing" makes perfect sense, because it's not our job to describe what other options do, simply explain what this option do. If the user is interested in other options, he can read them in the help for those.

-- 
Felipe Contreras