Thread (12 messages) read the whole thread 12 messages, 2 authors, 2d ago
WARM2d

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

From: Jeff King <hidden>
Date: 2026-07-28 15:15:00
Subsystem: the rest · Maintainer: Linus Torvalds

On Mon, Jul 27, 2026 at 05:43:21PM -0700, Junio C Hamano wrote:
quoted
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");
If we are helping humans we can probably afford to be a little more
eloquent. ;)

So maybe this on top of jk/diff-relative-cached-unmerged? I'd also be
happy to just let it be. It would not be the first Coverity false
positive by a long shot.

-- >8 --
Subject: [PATCH] diff-lib: add idx/tree sanity check to oneway_diff

When looking just at the code in oneway_diff(), it seems possible for
both "idx" and "tree" to be NULL, in which case we'd potentially
segfault while checking the relative prefix.

But if you consider what these items actually mean, it shouldn't be
possible for both to be NULL. Let's add an assertion and a comment
documenting this. It might help human readers, but should also silence
static analyzers like Coverity which complain about the potential
segfault.

Signed-off-by: Jeff King <redacted>
---
 diff-lib.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
diff --git a/diff-lib.c b/diff-lib.c
index 9986f5b141..d07e5d8d5b 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -528,6 +528,16 @@ static int oneway_diff(const struct cache_entry * const *src,
 	if (tree == o->df_conflict_entry)
 		tree = NULL;
 
+	/*
+	 * 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))
-- 
2.55.0.749.g30c495c7a6
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help