{"thread":{"id":"66002","subject":"[PATCH] diff: ignore unmerged paths outside prefix with --relative --cached","startedAt":"2026-07-15T06:05:30Z","lastAt":"2026-07-28T17:12:50Z","messageCount":12,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"548210","messageId":"20260715060523.GA517940@coredump.intra.peff.net","threadId":"66002","inReplyTo":null,"subject":"[PATCH] diff: ignore unmerged paths outside prefix with --relative --cached","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-15T06:05:23Z","receivedAt":"2026-07-15T06:05:30Z","isPatch":true,"body":"A diff using --relative ignores entries outside the current directory.\nThis results in a segfault when we try to process an unmerged entry\nthat's outside of our prefix, since we end up with a NULL diff_filepair\nand use it without checking that it's valid.\n\nI think this bug goes back to 76399c0195 (diff.c: return filepair from\ndiff_unmerge(), 2011-04-22). Prior to that, diff_unmerge() knew to skip\nentries outside of our prefix, due to cd676a5136 (diff --relative:\noutput paths as relative to the current subdirectory, 2008-02-12). Back\nthen the caller didn't care that we hadn't added anything to the queue.\nIn 76399c0195 that changed; we now returned the pair (or NULL), and the\ncaller in do_oneway_diff() was then called fill_filespec() itself. And\nit does so without checking for NULL, causing a segfault.\n\nThe obvious fix is to skip the fill_filespec() call (after which we just\nreturn), which this patch does.\n\nThere's another call to diff_unmerge() in run_diff_files(). That case\nwas already fixed by 8174627b3d (diff-lib: ignore paths that are outside\n$cwd if --relative asked, 2021-08-22), but of course it didn't help us\nfor --cached.\n\nThat commit also claims that checking the result of diff_unmerge() is\nnot enough, as we'd want other code paths to skip the entry, too (even\nif they wouldn't segfault). But as far as I can tell, that is not true\nfor --cached. We eventually end up in diff_queue_addremove() or in\ndiff_queue_change(), both of which know to return early when we're\noutside of the prefix.\n\nArguably we could be checking at the top of oneway_diff() whether the\npath is interesting at all. That would not only avoid this code path\nentirely, but would also possibly save a small amount of work. But since\neverything else appears to work OK, I went for the smallest fix here to\navoid any regression.\n\nSpecifically, a comment in oneway_diff() claims we're supposed to\nadvance o->pos, which we might fail to do if we return early. Though\nthat \"advance\" seems to have gone away in da165f470e (unpack-trees.c:\nprepare for looking ahead in the index, 2010-01-07), so it is possible\nthe comment is simply out of date.  We can explore that separately;\nchecking for a NULL return from diff_unmerge() seems like a sensible\nthing to do regardless.\n\nWe can piggy-back on the tests added by 8174627b3d; we're just checking\nthe --cached variant.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n+cc Junio, as you may have some wisdom on that further exploration.\n\n diff-lib.c               | 2 +-\n t/t4045-diff-relative.sh | 9 +++++++++\n 2 files changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex ae91027a02..a23119b852 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -467,7 +467,7 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n \tif (cached && idx && ce_stage(idx)) {\n \t\tstruct diff_filepair *pair;\n \t\tpair = diff_unmerge(&revs->diffopt, idx->name);\n-\t\tif (tree)\n+\t\tif (pair && tree)\n \t\t\tfill_filespec(pair->one, &tree->oid, 1,\n \t\t\t\t      tree->ce_mode);\n \t\treturn;\ndiff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh\nindex 2c8493fe66..167be0bdcc 100755\n--- a/t/t4045-diff-relative.sh\n+++ b/t/t4045-diff-relative.sh\n@@ -245,4 +245,13 @@ test_expect_failure 'diff --relative with change in subdir' '\n \ttest_cmp expected out\n '\n \n+test_expect_success 'diff --relative --cached with change in subdir' '\n+\tgit switch br3 &&\n+\ttest_when_finished \"git merge --abort\" &&\n+\ttest_must_fail git merge sub1 &&\n+\techo file0 >expected &&\n+\tgit -C subdir diff --relative --name-only --cached >out &&\n+\ttest_cmp expected out\n+'\n+\n test_done\n-- \n2.55.0.612.g68473ef936\n"},{"id":"548288","messageId":"xmqqjyqwp9jh.fsf@gitster.g","threadId":"66002","inReplyTo":"20260715060523.GA517940@coredump.intra.peff.net","subject":"Re: [PATCH] diff: ignore unmerged paths outside prefix with --relative --cached","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-15T15:17:06Z","receivedAt":"2026-07-15T15:17:08Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n> A diff using --relative ignores entries outside the current directory.\n> This results in a segfault when we try to process an unmerged entry\n> that's outside of our prefix, since we end up with a NULL diff_filepair\n> and use it without checking that it's valid.\n> ...\n> +cc Junio, as you may have some wisdom on that further exploration.\n\nWill take a look at the history myself, but I would probably not\nhave much wisdom on a change from 2011.  I often do not even\nremember what I ate for breakfast yesterday ;-).\n\n>  diff-lib.c               | 2 +-\n>  t/t4045-diff-relative.sh | 9 +++++++++\n>  2 files changed, 10 insertions(+), 1 deletion(-)\n>\n> diff --git a/diff-lib.c b/diff-lib.c\n> index ae91027a02..a23119b852 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -467,7 +467,7 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n>  \tif (cached && idx && ce_stage(idx)) {\n>  \t\tstruct diff_filepair *pair;\n>  \t\tpair = diff_unmerge(&revs->diffopt, idx->name);\n> -\t\tif (tree)\n> +\t\tif (pair && tree)\n>  \t\t\tfill_filespec(pair->one, &tree->oid, 1,\n>  \t\t\t\t      tree->ce_mode);\n>  \t\treturn;\n> diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh\n> index 2c8493fe66..167be0bdcc 100755\n> --- a/t/t4045-diff-relative.sh\n> +++ b/t/t4045-diff-relative.sh\n> @@ -245,4 +245,13 @@ test_expect_failure 'diff --relative with change in subdir' '\n>  \ttest_cmp expected out\n>  '\n>  \n> +test_expect_success 'diff --relative --cached with change in subdir' '\n> +\tgit switch br3 &&\n> +\ttest_when_finished \"git merge --abort\" &&\n> +\ttest_must_fail git merge sub1 &&\n> +\techo file0 >expected &&\n> +\tgit -C subdir diff --relative --name-only --cached >out &&\n> +\ttest_cmp expected out\n> +'\n> +\n>  test_done\n"},{"id":"548927","messageId":"xmqqo6fw3wki.fsf@gitster.g","threadId":"66002","inReplyTo":"20260715060523.GA517940@coredump.intra.peff.net","subject":"Re: [PATCH] diff: ignore unmerged paths outside prefix with --relative --cached","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-24T21:30:05Z","receivedAt":"2026-07-24T21:30:10Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n> diff --git a/diff-lib.c b/diff-lib.c\n> index ae91027a02..a23119b852 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -467,7 +467,7 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n>  \tif (cached && idx && ce_stage(idx)) {\n>  \t\tstruct diff_filepair *pair;\n>  \t\tpair = diff_unmerge(&revs->diffopt, idx->name);\n> -\t\tif (tree)\n> +\t\tif (pair && tree)\n>  \t\t\tfill_filespec(pair->one, &tree->oid, 1,\n>  \t\t\t\t      tree->ce_mode);\n>  \t\treturn;\n\nOK, so if diff_unmerge() gives a NULL for a path outside out area of\ninterest, we of course do not fill the filepair and return.  We\nwon't do any further processing, like showing the entry or recursing\ninto it, and diff_queued_diff hasn't been told about this path (as\ndiff_unmerge() returns NULL before queuing the pair), so it is the\nend of story for this path here.\n\nLooks good to me.\n\n> diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh\n> index 2c8493fe66..167be0bdcc 100755\n> --- a/t/t4045-diff-relative.sh\n> +++ b/t/t4045-diff-relative.sh\n> @@ -245,4 +245,13 @@ test_expect_failure 'diff --relative with change in subdir' '\n>  \ttest_cmp expected out\n>  '\n>  \n> +test_expect_success 'diff --relative --cached with change in subdir' '\n> +\tgit switch br3 &&\n> +\ttest_when_finished \"git merge --abort\" &&\n> +\ttest_must_fail git merge sub1 &&\n> +\techo file0 >expected &&\n> +\tgit -C subdir diff --relative --name-only --cached >out &&\n> +\ttest_cmp expected out\n> +'\n> +\n>  test_done\n"},{"id":"548992","messageId":"20260726084550.GC2366012@coredump.intra.peff.net","threadId":"66002","inReplyTo":"xmqqjyqwp9jh.fsf@gitster.g","subject":"[PATCH 0/2] diff-lib relative-path cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-26T08:45:50Z","receivedAt":"2026-07-26T08:45:52Z","isPatch":true,"body":"On Wed, Jul 15, 2026 at 08:17:06AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > A diff using --relative ignores entries outside the current directory.\n> > This results in a segfault when we try to process an unmerged entry\n> > that's outside of our prefix, since we end up with a NULL diff_filepair\n> > and use it without checking that it's valid.\n> > ...\n> > +cc Junio, as you may have some wisdom on that further exploration.\n> \n> Will take a look at the history myself, but I would probably not\n> have much wisdom on a change from 2011.  I often do not even\n> remember what I ate for breakfast yesterday ;-).\n\nI have the same problem. ;)\n\nLooks like you reviewed the patch in question already. Here's what I\nuncovered by digging into the history. I don't think it should have any\nfunctional difference (and even the \"avoid unnecessary work\" in patch 2\nis probably not very much work in practice), but it might be worth\ndoing.\n\nThis would go on top (even though patch 2 makes the original fix here\nunnecessary, I'd rather have both in place).\n\n  [1/2]: diff-lib: drop stale comment about advancing o->pos\n  [2/2]: diff-lib: skip paths outside prefix in oneway_diff()\n\n diff-lib.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\n-Peff\n"},{"id":"548993","messageId":"20260726084622.GA3529698@coredump.intra.peff.net","threadId":"66002","inReplyTo":"20260726084550.GC2366012@coredump.intra.peff.net","subject":"[PATCH 1/2] diff-lib: drop stale comment about advancing o->pos","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-26T08:46:22Z","receivedAt":"2026-07-26T08:46:24Z","isPatch":true,"body":"The comment above oneway_diff() claims that the callback must advance\no->pos to skip index entries it has already processed. That stopped\nbeing true in da165f470e (unpack-trees.c: prepare for looking ahead in\nthe index, 2010-01-07), which moved that bookkeeping into\nunpack_trees().\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff-lib.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex a23119b852..95f920a9a0 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -508,11 +508,9 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n  * For diffing, the index is more important, and we only have a\n  * single tree.\n  *\n- * We're supposed to advance o->pos to skip what we have already processed.\n- *\n  * This wrapper makes it all more readable, and takes care of all\n  * the fairly complex unpack_trees() semantic requirements, including\n- * the skipping, the path matching, the type conflict cases etc.\n+ * the path matching, the type conflict cases etc.\n  */\n static int oneway_diff(const struct cache_entry * const *src,\n \t\t       struct unpack_trees_options *o)\n-- \n2.55.0.742.gf2bff09aa6\n\n"},{"id":"548994","messageId":"20260726084705.GB3529698@coredump.intra.peff.net","threadId":"66002","inReplyTo":"20260726084550.GC2366012@coredump.intra.peff.net","subject":"[PATCH 2/2] diff-lib: skip paths outside prefix in oneway_diff()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-26T08:47:05Z","receivedAt":"2026-07-26T08:47:06Z","isPatch":true,"body":"Commit 8174627b3d (diff-lib: ignore paths that are outside $cwd if\n--relative asked, 2021-08-22) taught run_diff_files() to skip entries\noutside the requested prefix before processing them.\n\nDo the same in oneway_diff(), which handles the diff-index code path.\nThe lower-level diff queue functions already reject such paths, but\nchecking here avoids unnecessary work and keeps them out of every\ndo_oneway_diff() code path.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff-lib.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 95f920a9a0..9986f5b141 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -528,6 +528,11 @@ static int oneway_diff(const struct cache_entry * const *src,\n \tif (tree == o->df_conflict_entry)\n \t\ttree = NULL;\n \n+\tif (revs->diffopt.prefix &&\n+\t    strncmp((idx ? idx : tree)->name, revs->diffopt.prefix,\n+\t\t    revs->diffopt.prefix_length))\n+\t\treturn 0;\n+\n \tif (ce_path_match(revs->diffopt.repo->index,\n \t\t\t  idx ? idx : tree,\n \t\t\t  &revs->prune_data, NULL)) {\n-- \n2.55.0.742.gf2bff09aa6\n"},{"id":"549041","messageId":"xmqqjyqhv7s7.fsf@gitster.g","threadId":"66002","inReplyTo":"20260726084550.GC2366012@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] diff-lib relative-path cleanups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-26T20:02:32Z","receivedAt":"2026-07-26T20:02:35Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Jul 15, 2026 at 08:17:06AM -0700, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> > A diff using --relative ignores entries outside the current directory.\n>> > This results in a segfault when we try to process an unmerged entry\n>> > that's outside of our prefix, since we end up with a NULL diff_filepair\n>> > and use it without checking that it's valid.\n>> > ...\n>> > +cc Junio, as you may have some wisdom on that further exploration.\n>> \n>> Will take a look at the history myself, but I would probably not\n>> have much wisdom on a change from 2011.  I often do not even\n>> remember what I ate for breakfast yesterday ;-).\n>\n> I have the same problem. ;)\n>\n> Looks like you reviewed the patch in question already. Here's what I\n> uncovered by digging into the history. I don't think it should have any\n> functional difference (and even the \"avoid unnecessary work\" in patch 2\n> is probably not very much work in practice), but it might be worth\n> doing.\n>\n> This would go on top (even though patch 2 makes the original fix here\n> unnecessary, I'd rather have both in place).\n>\n>   [1/2]: diff-lib: drop stale comment about advancing o->pos\n>   [2/2]: diff-lib: skip paths outside prefix in oneway_diff()\n\nBoth patches look good to me.  Let's combine them with the original\nfix into a three-patch series and merge them into 'next'.\n\nThanks.\n"},{"id":"549077","messageId":"20260727093912.GA591426@coredump.intra.peff.net","threadId":"66002","inReplyTo":"20260726084705.GB3529698@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] diff-lib: skip paths outside prefix in oneway_diff()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-27T09:39:12Z","receivedAt":"2026-07-27T09:39:19Z","isPatch":true,"body":"On Sun, Jul 26, 2026 at 04:47:05AM -0400, Jeff King wrote:\n\n> diff --git a/diff-lib.c b/diff-lib.c\n> index 95f920a9a0..9986f5b141 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -528,6 +528,11 @@ static int oneway_diff(const struct cache_entry * const *src,\n>  \tif (tree == o->df_conflict_entry)\n>  \t\ttree = NULL;\n>  \n> +\tif (revs->diffopt.prefix &&\n> +\t    strncmp((idx ? idx : tree)->name, revs->diffopt.prefix,\n> +\t\t    revs->diffopt.prefix_length))\n> +\t\treturn 0;\n> +\n\nBTW, Coverity complains here that \"tree\" could be NULL (because we set\nit that way in the lines above due to a D/F conflict).\n\nI _think_ it is fine. We only look at \"tree\" if idx is NULL, and I think\nidx is only NULL when we have a deletion. So that implies either:\n\n  1. unpack_trees() passed us both entries as NULL, which doesn't make\n     sense. There was no entry to delete!\n\n  2. We set tree to NULL due to a D/F conflict. But a conflict with\n     what? There is nothing at the path in the index to conflict.\n\nSo AFAICT this is OK and it's just a false positive from Coverity\n(though an understandable one; the semantics of the relationship between\n\"idx\" and \"tree\" are not represented in the code).\n\nPossibly adding:\n\n  if (!idx && !tree)\n\tBUG(\"oneway diff with no endpoints\");\n\nwould help static analysis, but I don't know if that makes things more\nor less clear to a human.\n\n-Peff\n"},{"id":"549104","messageId":"xmqq4ihkgd06.fsf@gitster.g","threadId":"66002","inReplyTo":"20260727093912.GA591426@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] diff-lib: skip paths outside prefix in oneway_diff()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-28T00:43:21Z","receivedAt":"2026-07-28T00:43:24Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n>> --- a/diff-lib.c\n>> +++ b/diff-lib.c\n>> @@ -528,6 +528,11 @@ static int oneway_diff(const struct cache_entry * const *src,\n>>  \tif (tree == o->df_conflict_entry)\n>>  \t\ttree = NULL;\n>>  \n>> +\tif (revs->diffopt.prefix &&\n>> +\t    strncmp((idx ? idx : tree)->name, revs->diffopt.prefix,\n>> +\t\t    revs->diffopt.prefix_length))\n>> +\t\treturn 0;\n>> +\n>\n> BTW, Coverity complains here that \"tree\" could be NULL (because we set\n> it that way in the lines above due to a D/F conflict).\n>\n> I _think_ it is fine. We only look at \"tree\" if idx is NULL, and I think\n> idx is only NULL when we have a deletion. So that implies either:\n>\n>   1. unpack_trees() passed us both entries as NULL, which doesn't make\n>      sense. There was no entry to delete!\n>\n>   2. We set tree to NULL due to a D/F conflict. But a conflict with\n>      what? There is nothing at the path in the index to conflict.\n>\n> So AFAICT this is OK and it's just a false positive from Coverity\n> (though an understandable one; the semantics of the relationship between\n> \"idx\" and \"tree\" are not represented in the code).\n\nYeah, I agree with all of the above.\n\n> Possibly adding:\n>\n>   if (!idx && !tree)\n> \tBUG(\"oneway diff with no endpoints\");\n>\n> would help static analysis, but I don't know if that makes things more\n> or less clear to a human.\n\nWe could help humans that the BUG is not expected to fire and only\nto help static analysis by a crafted message, perhaps?\n\n   if (!idx && !tree)\n \tBUG(\"Hey, Coverity, this does not happen\");\n\n?\n"},{"id":"549136","messageId":"20260728151458.GB41931@coredump.intra.peff.net","threadId":"66002","inReplyTo":"xmqq4ihkgd06.fsf@gitster.g","subject":"[PATCH] diff-lib: add idx/tree sanity check to oneway_diff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-28T15:14:58Z","receivedAt":"2026-07-28T15:15:00Z","isPatch":true,"body":"On Mon, Jul 27, 2026 at 05:43:21PM -0700, Junio C Hamano wrote:\n\n> > Possibly adding:\n> >\n> >   if (!idx && !tree)\n> > \tBUG(\"oneway diff with no endpoints\");\n> >\n> > would help static analysis, but I don't know if that makes things more\n> > or less clear to a human.\n> \n> We could help humans that the BUG is not expected to fire and only\n> to help static analysis by a crafted message, perhaps?\n> \n>    if (!idx && !tree)\n>  \tBUG(\"Hey, Coverity, this does not happen\");\n\nIf we are helping humans we can probably afford to be a little more\neloquent. ;)\n\nSo maybe this on top of jk/diff-relative-cached-unmerged? I'd also be\nhappy to just let it be. It would not be the first Coverity false\npositive by a long shot.\n\n-- >8 --\nSubject: [PATCH] diff-lib: add idx/tree sanity check to oneway_diff\n\nWhen looking just at the code in oneway_diff(), it seems possible for\nboth \"idx\" and \"tree\" to be NULL, in which case we'd potentially\nsegfault while checking the relative prefix.\n\nBut if you consider what these items actually mean, it shouldn't be\npossible for both to be NULL. Let's add an assertion and a comment\ndocumenting this. It might help human readers, but should also silence\nstatic analyzers like Coverity which complain about the potential\nsegfault.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff-lib.c | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 9986f5b141..d07e5d8d5b 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -528,6 +528,16 @@ static int oneway_diff(const struct cache_entry * const *src,\n \tif (tree == o->df_conflict_entry)\n \t\ttree = NULL;\n \n+\t/*\n+\t * We should only see a NULL idx when the entry was present in the tree\n+\t * but deleted in the idx. In which case it should be impossible\n+\t * that a NULL tree was passed in (there would have been no entry at\n+\t * all) or that we got a df conflict above (you need a directory and a\n+\t * file to get such a conflict, which implies both sides are present).\n+\t */\n+\tif (!idx && !tree)\n+\t\tBUG(\"oneway_diff with neither idx nor tree\");\n+\n \tif (revs->diffopt.prefix &&\n \t    strncmp((idx ? idx : tree)->name, revs->diffopt.prefix,\n \t\t    revs->diffopt.prefix_length))\n-- \n2.55.0.749.g30c495c7a6\n\n"},{"id":"549150","messageId":"xmqqo6frdqy5.fsf@gitster.g","threadId":"66002","inReplyTo":"20260728151458.GB41931@coredump.intra.peff.net","subject":"Re: [PATCH] diff-lib: add idx/tree sanity check to oneway_diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-28T16:22:42Z","receivedAt":"2026-07-28T16:22:44Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n>> We could help humans that the BUG is not expected to fire and only\n>> to help static analysis by a crafted message, perhaps?\n>> \n>>    if (!idx && !tree)\n>>  \tBUG(\"Hey, Coverity, this does not happen\");\n>\n> If we are helping humans we can probably afford to be a little more\n> eloquent. ;)\n\nI do not mind eloquence but does the comment clearly say this is\nprimarily for unconfusing static analyzers?  My first reaction to\nthe message was \"OK, you explained very well why this condition\nwould never happen, but then why do you need to check and BUG() on\nit???\"\n\nBut I guess the point is a future modification may invalidate this,\nin which case I agree with the comment.  If it is hard for static\nanalysers to get it right, it probably is equally difficult to grok\nfor AI agents many people seem to be using to draft their changes\nthese days ;-).\n\n> +\t/*\n> +\t * We should only see a NULL idx when the entry was present in the tree\n> +\t * but deleted in the idx. In which case it should be impossible\n> +\t * that a NULL tree was passed in (there would have been no entry at\n> +\t * all) or that we got a df conflict above (you need a directory and a\n> +\t * file to get such a conflict, which implies both sides are present).\n> +\t */\n> +\tif (!idx && !tree)\n> +\t\tBUG(\"oneway_diff with neither idx nor tree\");\n> +\n>  \tif (revs->diffopt.prefix &&\n>  \t    strncmp((idx ? idx : tree)->name, revs->diffopt.prefix,\n>  \t\t    revs->diffopt.prefix_length))\n"},{"id":"549156","messageId":"20260728171249.GA643101@coredump.intra.peff.net","threadId":"66002","inReplyTo":"xmqqo6frdqy5.fsf@gitster.g","subject":"Re: [PATCH] diff-lib: add idx/tree sanity check to oneway_diff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-28T17:12:49Z","receivedAt":"2026-07-28T17:12:50Z","isPatch":true,"body":"On Tue, Jul 28, 2026 at 09:22:42AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> >> We could help humans that the BUG is not expected to fire and only\n> >> to help static analysis by a crafted message, perhaps?\n> >> \n> >>    if (!idx && !tree)\n> >>  \tBUG(\"Hey, Coverity, this does not happen\");\n> >\n> > If we are helping humans we can probably afford to be a little more\n> > eloquent. ;)\n> \n> I do not mind eloquence but does the comment clearly say this is\n> primarily for unconfusing static analyzers?  My first reaction to\n> the message was \"OK, you explained very well why this condition\n> would never happen, but then why do you need to check and BUG() on\n> it???\"\n\nI guess by the time I did all of the digging and thinking, my thought\nwas that it _wasn't_ primarily for static analyzers, but to capture the\noutput of that research. But then yeah, we don't really need the actual\nBUG(), but without it, it feels strange to even have the comment at all.\n\nI think that's why I waffled on sending the patch at all.\n\n> But I guess the point is a future modification may invalidate this,\n> in which case I agree with the comment.  If it is hard for static\n> analysers to get it right, it probably is equally difficult to grok\n> for AI agents many people seem to be using to draft their changes\n> these days ;-).\n\nYeah, I guess it may help them, too. :)\n\n-Peff\n"}]}