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(-)
Show changes to 2 files +10 −1
diff-lib.c, t/t4045-diff-relative.sh
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