Re: [PATCH v2 2/2] blame: allow blame --reverse --first-parent when it makes sense

From: Max Kirillov <max@max630.net>
Date: Oct 22, 2015, 14:56 UTC
Message-ID: <20151022145606.GD5722@wheezy.local>
In-Reply-To: <xmqqfv13ttq6.fsf@gitster.mtv.corp.google.com>

On Wed, Oct 21, 2015 at 09:25:37PM -0700, Junio C Hamano wrote:

Show 21 quoted lines

> Max Kirillov <max@max630.net> writes:
> 
>> Do not die immediately when the two flags are specified. Instead
>> check that the specified range is along first-parent chain. Exploit
>> how prepare_revision_walk() handles first_parent_only flag: the commits
>> outside of first-parent chain are either unknown (and do not have any
>> children recorded) or appear as non-first parent of a commit along the
>> first-parent chain.
>>
>> Since the check seems fragile, add test which verifies that blame dies
>> in both cases.
> 
> It is not quite clear in what way the "check seems fragile".
> 
> It is either "correct" or "appears to have worked by chance and
> nobody has any confidence that it would tell if 'it makes sense'
> reliably", and the latter cannot be papered over with any number of
> tests.
> 
> The logic you implemented feels solid to me, at least at a first
> glance.  What kind of gotchas are you worried about?

The fact than arbitrary commit's children are unknown does not seem reliable to me. It more fits the "works by chance" description.

Actually, I realized that this can produce false positive if the final commit is no-ff merged into first-parent chain, because it woul be both first and non-first parent of the first-parent chain. But afaik such merge is not possible to create by frontent git commands, so I'm not sure I should consider such case.

-- 
Max