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

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