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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 5, 2013, 18:13 UTC
Message-ID: <7vsj0w1l4v.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CAMP44s1X=M2sV9OiHS_ggVjmu6txBQVSdK+aK6PnQm5-9EpQgw@mail.gmail.com>

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

Show 14 quoted lines

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