Re: [PATCH] diff-lib: add idx/tree sanity check to oneway_diff

Jeff King <[email protected]> writes:

>> We could help humans that the BUG is not expected to fire and only
>> to help static analysis by a crafted message, perhaps?
>> 
>>    if (!idx && !tree)
>>  	BUG("Hey, Coverity, this does not happen");
>
> If we are helping humans we can probably afford to be a little more
> eloquent. ;)

I do not mind eloquence but does the comment clearly say this is
primarily for unconfusing static analyzers?  My first reaction to
the message was "OK, you explained very well why this condition
would never happen, but then why do you need to check and BUG() on
it???"

But I guess the point is a future modification may invalidate this,
in which case I agree with the comment.  If it is hard for static
analysers to get it right, it probably is equally difficult to grok
for AI agents many people seem to be using to draft their changes
these days ;-).

> +	/*
> +	 * We should only see a NULL idx when the entry was present in the tree
> +	 * but deleted in the idx. In which case it should be impossible
> +	 * that a NULL tree was passed in (there would have been no entry at
> +	 * all) or that we got a df conflict above (you need a directory and a
> +	 * file to get such a conflict, which implies both sides are present).
> +	 */
> +	if (!idx && !tree)
> +		BUG("oneway_diff with neither idx nor tree");
> +
>  	if (revs->diffopt.prefix &&
>  	    strncmp((idx ? idx : tree)->name, revs->diffopt.prefix,
>  		    revs->diffopt.prefix_length))