Re: [PATCH 2/2] diff-lib: skip paths outside prefix in oneway_diff()

Jeff King [off-list ref] writes:

quoted

--- a/diff-lib.c
+++ b/diff-lib.c

@@ -528,6 +528,11 @@ static int oneway_diff(const struct cache_entry * const *src,
 	if (tree == o->df_conflict_entry)
 		tree = NULL;
 
+	if (revs->diffopt.prefix &&
+	    strncmp((idx ? idx : tree)->name, revs->diffopt.prefix,
+		    revs->diffopt.prefix_length))
+		return 0;
+

BTW, Coverity complains here that "tree" could be NULL (because we set
it that way in the lines above due to a D/F conflict).

I _think_ it is fine. We only look at "tree" if idx is NULL, and I think
idx is only NULL when we have a deletion. So that implies either:

  1. unpack_trees() passed us both entries as NULL, which doesn't make
     sense. There was no entry to delete!

  2. We set tree to NULL due to a D/F conflict. But a conflict with
     what? There is nothing at the path in the index to conflict.

So AFAICT this is OK and it's just a false positive from Coverity
(though an understandable one; the semantics of the relationship between
"idx" and "tree" are not represented in the code).

Yeah, I agree with all of the above.

Possibly adding:

  if (!idx && !tree)
	BUG("oneway diff with no endpoints");

would help static analysis, but I don't know if that makes things more
or less clear to a human.

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");

?