[PATCH] diff: ignore unmerged paths outside prefix with --relative --cached

[PATCH] diff: ignore unmerged paths outside prefix with --relative --cached

[Jeff King]

A diff using --relative ignores entries outside the current directory.
This results in a segfault when we try to process an unmerged entry
that's outside of our prefix, since we end up with a NULL diff_filepair
and use it without checking that it's valid.

I think this bug goes back to 76399c0195 (diff.c: return filepair from
diff_unmerge(), 2011-04-22). Prior to that, diff_unmerge() knew to skip
entries outside of our prefix, due to cd676a5136 (diff --relative:
output paths as relative to the current subdirectory, 2008-02-12). Back
then the caller didn't care that we hadn't added anything to the queue.
In 76399c0195 that changed; we now returned the pair (or NULL), and the
caller in do_oneway_diff() was then called fill_filespec() itself. And
it does so without checking for NULL, causing a segfault.

The obvious fix is to skip the fill_filespec() call (after which we just
return), which this patch does.

There's another call to diff_unmerge() in run_diff_files(). That case
was already fixed by 8174627b3d (diff-lib: ignore paths that are outside
$cwd if --relative asked, 2021-08-22), but of course it didn't help us
for --cached.

That commit also claims that checking the result of diff_unmerge() is
not enough, as we'd want other code paths to skip the entry, too (even
if they wouldn't segfault). But as far as I can tell, that is not true
for --cached. We eventually end up in diff_queue_addremove() or in
diff_queue_change(), both of which know to return early when we're
outside of the prefix.

Arguably we could be checking at the top of oneway_diff() whether the
path is interesting at all. That would not only avoid this code path
entirely, but would also possibly save a small amount of work. But since
everything else appears to work OK, I went for the smallest fix here to
avoid any regression.

Specifically, a comment in oneway_diff() claims we're supposed to
advance o->pos, which we might fail to do if we return early. Though
that "advance" seems to have gone away in da165f470e (unpack-trees.c:
prepare for looking ahead in the index, 2010-01-07), so it is possible
the comment is simply out of date.  We can explore that separately;
checking for a NULL return from diff_unmerge() seems like a sensible
thing to do regardless.

We can piggy-back on the tests added by 8174627b3d; we're just checking
the --cached variant.

Signed-off-by: Jeff King <peff@peff.net>
---
+cc Junio, as you may have some wisdom on that further exploration.

 diff-lib.c               | 2 +-
 t/t4045-diff-relative.sh | 9 +++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/diff-lib.c b/diff-lib.c
index ae91027a02..a23119b852 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -467,7 +467,7 @@ static void do_oneway_diff(struct unpack_trees_options *o,
 	if (cached && idx && ce_stage(idx)) {
 		struct diff_filepair *pair;
 		pair = diff_unmerge(&revs->diffopt, idx->name);
-		if (tree)
+		if (pair && tree)
 			fill_filespec(pair->one, &tree->oid, 1,
 				      tree->ce_mode);
 		return;
diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
index 2c8493fe66..167be0bdcc 100755
--- a/t/t4045-diff-relative.sh
+++ b/t/t4045-diff-relative.sh
@@ -245,4 +245,13 @@ test_expect_failure 'diff --relative with change in subdir' '
 	test_cmp expected out
 '
 
+test_expect_success 'diff --relative --cached with change in subdir' '
+	git switch br3 &&
+	test_when_finished "git merge --abort" &&
+	test_must_fail git merge sub1 &&
+	echo file0 >expected &&
+	git -C subdir diff --relative --name-only --cached >out &&
+	test_cmp expected out
+'
+
 test_done
-- 
2.55.0.612.g68473ef936

Re: [PATCH] diff: ignore unmerged paths outside prefix with --relative --cached

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> A diff using --relative ignores entries outside the current directory.
> This results in a segfault when we try to process an unmerged entry
> that's outside of our prefix, since we end up with a NULL diff_filepair
> and use it without checking that it's valid.
> ...
> +cc Junio, as you may have some wisdom on that further exploration.

Will take a look at the history myself, but I would probably not
have much wisdom on a change from 2011.  I often do not even
remember what I ate for breakfast yesterday ;-).

>  diff-lib.c               | 2 +-
>  t/t4045-diff-relative.sh | 9 +++++++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/diff-lib.c b/diff-lib.c
> index ae91027a02..a23119b852 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -467,7 +467,7 @@ static void do_oneway_diff(struct unpack_trees_options *o,
>  	if (cached && idx && ce_stage(idx)) {
>  		struct diff_filepair *pair;
>  		pair = diff_unmerge(&revs->diffopt, idx->name);
> -		if (tree)
> +		if (pair && tree)
>  			fill_filespec(pair->one, &tree->oid, 1,
>  				      tree->ce_mode);
>  		return;
> diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
> index 2c8493fe66..167be0bdcc 100755
> --- a/t/t4045-diff-relative.sh
> +++ b/t/t4045-diff-relative.sh
> @@ -245,4 +245,13 @@ test_expect_failure 'diff --relative with change in subdir' '
>  	test_cmp expected out
>  '
>  
> +test_expect_success 'diff --relative --cached with change in subdir' '
> +	git switch br3 &&
> +	test_when_finished "git merge --abort" &&
> +	test_must_fail git merge sub1 &&
> +	echo file0 >expected &&
> +	git -C subdir diff --relative --name-only --cached >out &&
> +	test_cmp expected out
> +'
> +
>  test_done

[PATCH 0/2] diff-lib relative-path cleanups

[Jeff King]

On Wed, Jul 15, 2026 at 08:17:06AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > A diff using --relative ignores entries outside the current directory.
> > This results in a segfault when we try to process an unmerged entry
> > that's outside of our prefix, since we end up with a NULL diff_filepair
> > and use it without checking that it's valid.
> > ...
> > +cc Junio, as you may have some wisdom on that further exploration.
> 
> Will take a look at the history myself, but I would probably not
> have much wisdom on a change from 2011.  I often do not even
> remember what I ate for breakfast yesterday ;-).

I have the same problem. ;)

Looks like you reviewed the patch in question already. Here's what I
uncovered by digging into the history. I don't think it should have any
functional difference (and even the "avoid unnecessary work" in patch 2
is probably not very much work in practice), but it might be worth
doing.

This would go on top (even though patch 2 makes the original fix here
unnecessary, I'd rather have both in place).

  [1/2]: diff-lib: drop stale comment about advancing o->pos
  [2/2]: diff-lib: skip paths outside prefix in oneway_diff()

 diff-lib.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

-Peff

[PATCH 1/2] diff-lib: drop stale comment about advancing o->pos

[Jeff King]

The comment above oneway_diff() claims that the callback must advance
o->pos to skip index entries it has already processed. That stopped
being true in da165f470e (unpack-trees.c: prepare for looking ahead in
the index, 2010-01-07), which moved that bookkeeping into
unpack_trees().

Signed-off-by: Jeff King <peff@peff.net>
---
 diff-lib.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index a23119b852..95f920a9a0 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -508,11 +508,9 @@ static void do_oneway_diff(struct unpack_trees_options *o,
  * For diffing, the index is more important, and we only have a
  * single tree.
  *
- * We're supposed to advance o->pos to skip what we have already processed.
- *
  * This wrapper makes it all more readable, and takes care of all
  * the fairly complex unpack_trees() semantic requirements, including
- * the skipping, the path matching, the type conflict cases etc.
+ * the path matching, the type conflict cases etc.
  */
 static int oneway_diff(const struct cache_entry * const *src,
 		       struct unpack_trees_options *o)
-- 
2.55.0.742.gf2bff09aa6

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

[Jeff King]

Commit 8174627b3d (diff-lib: ignore paths that are outside $cwd if
--relative asked, 2021-08-22) taught run_diff_files() to skip entries
outside the requested prefix before processing them.

Do the same in oneway_diff(), which handles the diff-index code path.
The lower-level diff queue functions already reject such paths, but
checking here avoids unnecessary work and keeps them out of every
do_oneway_diff() code path.

Signed-off-by: Jeff King <peff@peff.net>
---
 diff-lib.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/diff-lib.c b/diff-lib.c
index 95f920a9a0..9986f5b141 100644
--- 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;
+
 	if (ce_path_match(revs->diffopt.repo->index,
 			  idx ? idx : tree,
 			  &revs->prune_data, NULL)) {
-- 
2.55.0.742.gf2bff09aa6

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

[Jeff King]

On Sun, Jul 26, 2026 at 04:47:05AM -0400, Jeff King wrote:

> diff --git a/diff-lib.c b/diff-lib.c
> index 95f920a9a0..9986f5b141 100644
> --- 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).

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.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

?

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

[Jeff King]

On Mon, Jul 27, 2026 at 05:43:21PM -0700, Junio C Hamano wrote:

> > 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 <peff@peff.net>
---
 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> 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))

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

[Jeff King]

On Tue, Jul 28, 2026 at 09:22:42AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> 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???"

I guess by the time I did all of the digging and thinking, my thought
was that it _wasn't_ primarily for static analyzers, but to capture the
output of that research. But then yeah, we don't really need the actual
BUG(), but without it, it feels strange to even have the comment at all.

I think that's why I waffled on sending the patch at all.

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

Yeah, I guess it may help them, too. :)

-Peff

Re: [PATCH 0/2] diff-lib relative-path cleanups

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Jul 15, 2026 at 08:17:06AM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > A diff using --relative ignores entries outside the current directory.
>> > This results in a segfault when we try to process an unmerged entry
>> > that's outside of our prefix, since we end up with a NULL diff_filepair
>> > and use it without checking that it's valid.
>> > ...
>> > +cc Junio, as you may have some wisdom on that further exploration.
>> 
>> Will take a look at the history myself, but I would probably not
>> have much wisdom on a change from 2011.  I often do not even
>> remember what I ate for breakfast yesterday ;-).
>
> I have the same problem. ;)
>
> Looks like you reviewed the patch in question already. Here's what I
> uncovered by digging into the history. I don't think it should have any
> functional difference (and even the "avoid unnecessary work" in patch 2
> is probably not very much work in practice), but it might be worth
> doing.
>
> This would go on top (even though patch 2 makes the original fix here
> unnecessary, I'd rather have both in place).
>
>   [1/2]: diff-lib: drop stale comment about advancing o->pos
>   [2/2]: diff-lib: skip paths outside prefix in oneway_diff()

Both patches look good to me.  Let's combine them with the original
fix into a three-patch series and merge them into 'next'.

Thanks.

Re: [PATCH] diff: ignore unmerged paths outside prefix with --relative --cached

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> diff --git a/diff-lib.c b/diff-lib.c
> index ae91027a02..a23119b852 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -467,7 +467,7 @@ static void do_oneway_diff(struct unpack_trees_options *o,
>  	if (cached && idx && ce_stage(idx)) {
>  		struct diff_filepair *pair;
>  		pair = diff_unmerge(&revs->diffopt, idx->name);
> -		if (tree)
> +		if (pair && tree)
>  			fill_filespec(pair->one, &tree->oid, 1,
>  				      tree->ce_mode);
>  		return;

OK, so if diff_unmerge() gives a NULL for a path outside out area of
interest, we of course do not fill the filepair and return.  We
won't do any further processing, like showing the entry or recursing
into it, and diff_queued_diff hasn't been told about this path (as
diff_unmerge() returns NULL before queuing the pair), so it is the
end of story for this path here.

Looks good to me.

> diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
> index 2c8493fe66..167be0bdcc 100755
> --- a/t/t4045-diff-relative.sh
> +++ b/t/t4045-diff-relative.sh
> @@ -245,4 +245,13 @@ test_expect_failure 'diff --relative with change in subdir' '
>  	test_cmp expected out
>  '
>  
> +test_expect_success 'diff --relative --cached with change in subdir' '
> +	git switch br3 &&
> +	test_when_finished "git merge --abort" &&
> +	test_must_fail git merge sub1 &&
> +	echo file0 >expected &&
> +	git -C subdir diff --relative --name-only --cached >out &&
> +	test_cmp expected out
> +'
> +
>  test_done