Re: [PATCH] revision.c: Correctly dereference interesting_cache

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Stefan Beller wrote:

> This was introduced at b6e8a3b5 (2015-04-17, limit_list: avoid
> quadratic behavior from still_interesting), which
> also introduced the check a few lines before, which already dereferences
> `interesting_cache`. So at this point `interesting_cache` is guaranteed to
> be not NULL.

The above is the rationale for the coverity warning, but it does not
explain why this change is safe.

>                The code is called referencing the address of a local
> variable, so `interesting_cache` can actually never be NULL and trigger a
> segmentation fault by dereferencing it a few lines before this.

I'm having trouble parsing this sentence.  Do you mean that limit_list()
only calls still_interesting() (and thus, indirectly,
everybody_uninteresting()), with the second parameter equal to the
address of the local interesting_cache variable, so it can never be
NULL?

That makes sense, but I had to look at the code and reread the above
sentence a few times before I understood.

Do you know what this code is trying to check for?  What does it mean
for *interesting_cache to be NULL?

Should there be

	if (!interesting_cache)
		die("BUG: &interesting_cache == NULL");

checks at the top of still_interesting and everybody_uninteresting to
futureproof this?

What does the *interesting_cache variable represent, anyway?

This code seems to be underdocumented.

Thanks and hope that helps,
Jonathan