Re: [PATCH] setup_revisions(): turn on diffs for all-negative diff filter

[Karthik Nayak <karthik.188@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4158 bytes --]

Jeff King <peff@peff.net> 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@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;
>

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]