[PATCH] setup_revisions(): turn on diffs for all-negative diff filter
- From
Jeff King <peff@peff.net>
- Date
- Jul 3, 2025, 22:44 UTC
- Message-ID
- <20250703224428.GB1909836@coredump.intra.peff.net>
- In-Reply-To
- <20250703153438.GA1309870@coredump.intra.peff.net>
On Thu, Jul 03, 2025 at 11:34:38AM -0400, Jeff King wrote:
Show 22 quoted lines
> On Wed, Jul 02, 2025 at 03:28:43PM -0500, Eric Salem wrote: > > > The git log --diff-filter documentation[1] for deleted files says: > > > > > Select only files that are Added (A), Copied (C), Deleted (D)... > > > > > Also, these upper-case letters can be downcased to exclude. > > > E.g. --diff-filter=ad excludes added and deleted paths. > > > > A simple test: > > [...] > > --diff-filter=D behaves as expected, but when using "d" instead, I don't > > get any output unless I add another option (such as --stat or > > --name-only). > > Looks like a bug. This used to produce the output I'd expect (i.e., > commits "first" and "third", which do not have deletions), but that > changed in 75408ca949 (diff-filter: be more careful when looking for > negative bits, 2022-01-28). > > I don't have time to dig into it now, but I've cc'd the author (and left > your whole reproduction recipe quoted below).
Argh, I forgot to add Johannes to the cc. Fortunately since then I had a moment to look at this, and the solution is pretty simple. So here it is as a patch with a test.
-- >8 -- Subject: setup_revisions(): turn on diffs for all-negative diff filter
When the user gives us a diff filter like --diff-filter=D, we need to do a tree diff even if we're not planning to show the diff result itself, in order to decide whether to show the commit at all. So there's an explicit check of revs->diffopt.filter in setup_revisions(), and we set revs->diff if any bits are set.
Originally that "filter" field covered both positive capital-letter filters (like "D") and also negative lowercase filters (like "d"), so it was sufficient for both cases. But later, 75408ca949 (diff-filter: be more careful when looking for negative bits, 2022-01-28) split the negative bits out into a "filter_not" field.
We eventually fold those into "filter", but not until diff_setup_done() is called, which happens after our explicit check. As a result, a purely negative filter like:
git log --diff-filter=d
failed to turn on diffs at all. But rather than fail to filter by diff, because the filter variable is eventually set, we mistakenly show no commits at all, thinking that the empty diffs were cases where nothing passed through the filter.
The smallest fix here is to just have our check look for any bits in either "filter" or "filter_not". I suspect it would also be OK to reorder the function a bit to call diff_setup_done() earlier, but that risks violating some other subtle ordering dependency. So I went with the simple and safe solution here.
Signed-off-by: Jeff King <peff@peff.net> --- revision.c | 2 +- t/t4202-log.sh | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/revision.c b/revision.c index afee111196..9892d08748 100644 --- a/revision.c +++ b/revision.c @@ -3112,7 +3112,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s /* Pickaxe, diff-filter and rename following need diffs */ if ((revs->diffopt.pickaxe_opts & DIFF_PICKAXE_KINDS_MASK) || - revs->diffopt.filter || + revs->diffopt.filter || revs->diffopt.filter_not || revs->diffopt.flags.follow_renames) revs->diff = 1; diff --git a/t/t4202-log.sh b/t/t4202-log.sh index 4a6c4dfbf4..05cee9e41b 100755 --- a/t/t4202-log.sh +++ b/t/t4202-log.sh @@ -134,6 +134,12 @@ test_expect_success 'diff-filter=D' ' ' +test_expect_success 'all-negative filter' ' + git log --no-renames --format=%s --diff-filter=d HEAD >actual && + printf "%s\n" fifth fourth third second initial >expect && + test_cmp expect actual +' + test_expect_success 'diff-filter=R' ' git log -M --pretty="format:%s" --diff-filter=R HEAD >actual &&
-- 2.50.0.438.g3b3bebd3e8