From: Junio C Hamano <junkio@cox.net>
Date: Sun, 11 Mar 2007 20:58:18 GMT
Subject: Re: "git-diff -p :/anything" always segfaults
Message-ID: <7vslcbv6ol.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <Pine.LNX.4.64.0703111322180.9690@woody.linux-foundation.org>

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Sun, 11 Mar 2007, Jim Meyering wrote:
>>
>> I like the idea of the new ':/<oneline prefix>' notation, and gave it
>> a try, but all I could get was a segfault.  It was dereferencing a NULL
>> commit list.  Fix below.  With it, this example now works:
>
> The fix is correct, but not complete.
>
>> -	while ((commit = pop_most_recent_commit(&list, ONELINE_SEEN))) {
>> +	while (list && (commit = pop_most_recent_commit(&list, ONELINE_SEEN))) {
>
> The old code was broken, but the new one isn't much better.
>
> "pop_most_recent_commit()" simply doesn't work that way. It *never* 
> returns NULL. So having it as part of a while-loop was buggy to begin 
> with, and you fixed the test, but the thing is, it should just look like
>
> 	while (list) {
> 		struct commit *commit;
>
> 		commit = pop_most_recent_commit(&list, ONELINE_SEEN);
> 		..
>
> and the "pop_most_recent_commit()" simply shouldn't be part of the 
> conditional at all.

That's what I did in my tentative commit based on Jim's patch
(except "commit" is also used to determine the return value from
the function).