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

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.

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.