Jeff King <peff@xxxxxxxx> writes: > On Thu, Jul 03, 2025 at 11:34:38AM -0400, Jeff King wrote: > >> 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. So if `revs->diff` is set, then we compute the tree diff for the given commit. > > 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. > The explanation here was really nice to read and explained the problem well. > Signed-off-by: Jeff King <peff@xxxxxxxx> > --- > 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; > Makes sense. > 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 The fix looks great. Thanks!
Attachment:
signature.asc
Description: PGP signature
