Re: "git-diff -p :/anything" always segfaults

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Mar 11, 2007, 20:25 UTC
Message-ID: <Pine.LNX.4.64.0703111322180.9690@woody.linux-foundation.org>
In-Reply-To: <87d53fsjiz.fsf@rho.meyering.net>

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.

Alternatively, we could just change the semantics, and have it return NULL when the list is empty. That would probably be fine too, but then the old code was correct.

Hmm?

		Linus