[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

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

[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 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.