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

[Felipe Contreras <felipe.contreras@gmail.com>]

On Wed, Jun 5, 2013 at 1:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Tue, Jun 4, 2013 at 1:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>
>>>> You didn't answer, what happens when you run --skip-empty and --allow-empty?
>>>
>>> I'll answer to a slightly different question: What should happen?
>>>
>>> I think it should error out, because --allow-empty is about
>>> "allowing empty commits to be preserved", and --skip-empty is about
>>> "skipping empty commits (as it says on the package)".
>>
>> Exactly, so they are related after all. However, --allow-empty says this:
>>
>> "By default, cherry-picking an empty commit will fail,"
>
> OK, that is a very good point.  Especially because by the time
> reader reaches this new description, --allow-empty has already
> mentioned this, we can just be brief and it is sufficient to say
> "Instead of failing," upfront.
>
>> In fact, it might make sense to change the default in v2.0 to
>> --empty-commits=skip. If it makes sense in 'git rebase', why doesn't
>> it in 'git cherry-pick'?
>
> I think the primary reason behind the "Why are you picking a no-op?
> Let me stop to make sure you didn't make a mistake." is because
> cherry-pick and revert have long been operations for a single commit
> explicitly given by the user, not bunch of commits in a range.

Yeah, but this has changed already.

> These two operating modes are conceptually very different, even when
> we consider scripted use.  A script that feeds one commit at a time
> has a chance to do patch equivalence or user-interactive filtering
> on its own, and would be helped by the "are you sure you meant to
> record that no-op?" check if it filtered in a wrong way (e.g. the
> user specified an empty commit by mistake).  One that feeds a range,
> on the other hand, relies on or at least expects cherry-pick to have
> such smart, and a smarter cherry-pick could select what to pick from
> the given range.

How would a script know that a single pick ends up as a no-op? It
can't know what is the reason the cherry-pick failed. The only way to
know for sure is to check the last commit, and for that we don't need
the last cherry-pick to fail.

> In the longer term, especially if we envision to move large part of
> logic to prepare the sequencer insn list from rebase to cherry-pick,
> a ranged cherry-pick should learn a way to filter the range with
> patch equivalence, for example.  Once that happens, the behaviour
> would become inconsistent at the end-user level if we stopped at a
> commit only because it was not exactly patch equivalent to another
> one that is already on the branch we are cherry-picking to, but
> ended up to be a no-op, while we did not stop at another commit
> because patch equivalence filtered it out beforehand to skip it.
> Skipping a no-op by default would remove that inconsistency.
>
> So in that sense, one could argue that it may be a bugfix to skip
> commits that become no-op when replayed, when picking a range of
> commits (but not a single one).  And I do not think you would need
> to wait until 2.0 for such a change.

Right. But first we need to agree that failing an empty cherry-pick
serves no purpose.

-- 
Felipe Contreras