Re: [Feature Request] git blame showing only revisions from git rev-list --first-parent

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 12, 2015, 08:29 UTC
Message-ID: <xmqqy4gcxcp4.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20150912033054.GA30431@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 5 quoted lines

> Yeah, I think what is happening in this first hunk:
> ...
> is doing the right thing. It did feel a little weird to me to be munging
> the global commit objects themselves, but I guess it is fairly normal
> for git code.

Yeah, the above may be brilliant, but ...

Show 15 quoted lines

>> diff --git a/revision.c b/revision.c
>> index af2a18e..a020a42 100644
>> --- a/revision.c
>> +++ b/revision.c
>> @@ -2765,7 +2765,9 @@ static void set_children(struct rev_info *revs)
>>  		struct commit *commit = l->item;
>>  		struct commit_list *p;
>>  
>> -		for (p = commit->parents; p; p = p->next)
>> +		for (p = commit->parents;
>> +		     p && !revs->first_parent_only;
>> +		     p = p->next)
>>  			add_child(revs, p->item, commit);
>>  	}
>>  }

... this is a total crap and shows that I am doubly an idiot.

The loop is a no-op when first-parent-only (the intent is to call add-child for just the first parent), so the code is stupid and wrong in the first place, but more importantly, the logic is utterly confused.

The thing is, traversing first-parent chain in reverse fundamentally is undefined. You can fork multiple topics at the tip of 'master' and each of the topics may be single strand of pearls, but which one of the topics is the first-child-chain---there is no answer to that question.

The add_child() call above is exactly that. It is saying "record the fact that commit is child of p->item for these p on that parents list". We may limit the call to record that commit C is a child of its first parent P (and nobody else), but that does not make us record that that P has only one child (which is C). This loop will be entered with another commit C' whose first parent is the same P and when we return from the set_children() function, we will find that P has more than one children (C and C', at least); the only thing we ensured is that all of these recorded children have P as their first parent. It does not tell us which one of C's we would want to pick when digging in reverse from P.