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

[Max Kirillov <max@max630.net>]

On Wed, Oct 21, 2015 at 09:25:37PM -0700, Junio C Hamano wrote:
> 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