diff mbox series

[v2,3/3] diff-filter: be more careful when looking for negative bits

Message ID b041d2b7a3bd4433e86438cddbd52857e5f375a6.1643310510.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Fix two --diff-filter bugs | expand

Commit Message

Johannes Schindelin Jan. 27, 2022, 7:08 p.m. UTC 
From: Johannes Schindelin <johannes.schindelin@gmx.de>

The `--diff-filter=<bits>` option allows to filter the diff by certain
criteria, for example `R` to only show renamed files. It also supports
negating a filter via a down-cased letter, i.e. `r` to show _everything
but_ renamed files.

However, the code is a bit overzealous when trying to figure out whether
`git diff` should start with all diff-filters turned on because the user
provided a lower-case letter: if the `--diff-filter` argument starts
with an upper-case letter, we must not start with all bits turned on.

Even worse, it is possible to specify the diff filters in multiple,
separate options, e.g. `--diff-filter=AM [...] --diff-filter=m`.

Let's accumulate the include/exclude filters independently, and only
special-case the "only exclude filters were specified" case after
parsing the options altogether.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 diff.c         | 23 +++++++----------------
 diff.h         |  2 +-
 t/t4202-log.sh | 13 +++++++++++++
 3 files changed, 21 insertions(+), 17 deletions(-)

Comments

Junio C Hamano Jan. 28, 2022, 1:16 a.m. UTC | #1 
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> diff --git a/diff.c b/diff.c
> index 5081052c431..4ab4299b817 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4720,6 +4720,12 @@ void diff_setup_done(struct diff_options *options)
>  	if (!options->use_color || external_diff())
>  		options->color_moved = 0;
>  
> +	if (options->filter_not) {
> +		if (!options->filter)
> +			options->filter = ~filter_bit[DIFF_STATUS_FILTER_AON];

Unlike the original, options->filter will have excess higher bit all
on, in addition to all the filter bits except for the all-or-none
bit.  I do not know offhand if that makes the difference, but I
trust that you have audited all uses of the options->filter flag
word and these high bits are truly unused and the difference does
not matter.

> +		options->filter &= ~options->filter_not;
> +	}

>  	for (i = 0; (optch = optarg[i]) != '\0'; i++) {
>  		unsigned int bit;
>  		int negate;
> @@ -4851,7 +4842,7 @@ static int diff_opt_diff_filter(const struct option *option,
>  			return error(_("unknown change class '%c' in --diff-filter=%s"),
>  				     optarg[i], optarg);
>  		if (negate)
> -			opt->filter &= ~bit;
> +			opt->filter_not |= bit;
>  		else
>  			opt->filter |= bit;
>  	}

And this ... 

> diff --git a/diff.h b/diff.h
> index 8ba85c5e605..a70e7c478c1 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -283,7 +283,7 @@ struct diff_options {
>  	struct diff_flags flags;
>  
>  	/* diff-filter bits */
> -	unsigned int filter;
> +	unsigned int filter, filter_not;

... is exactly I wrote in the NEEDSWORK comment I gave you in my
earlier review.  Excellent.

> +test_expect_success 'multiple --diff-filter bits' '
> +
> +	git log -M --pretty="format:%s" --diff-filter=R HEAD >expect &&
> +	git log -M --pretty="format:%s" --diff-filter=Ra HEAD >actual &&
> +	test_cmp expect actual &&
> +	git log -M --pretty="format:%s" --diff-filter=aR HEAD >actual &&
> +	test_cmp expect actual &&
> +	git log -M --pretty="format:%s" \
> +		--diff-filter=a --diff-filter=R HEAD >actual &&
> +	test_cmp expect actual
> +
> +'

Good.

Thanks for noticing and fixing the long-standing issue.
Johannes Schindelin Jan. 28, 2022, 12:01 p.m. UTC | #2 
Hi Junio,

On Thu, 27 Jan 2022, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > diff --git a/diff.c b/diff.c
> > index 5081052c431..4ab4299b817 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -4720,6 +4720,12 @@ void diff_setup_done(struct diff_options *options)
> >  	if (!options->use_color || external_diff())
> >  		options->color_moved = 0;
> >
> > +	if (options->filter_not) {
> > +		if (!options->filter)
> > +			options->filter = ~filter_bit[DIFF_STATUS_FILTER_AON];
>
> Unlike the original, options->filter will have excess higher bit all
> on, in addition to all the filter bits except for the all-or-none
> bit.

Thank you for your thoroughness. Indeed, you are correct that I no longer
do the `(1 << (ARRAY_SIZE(diff_status_letters) - 1)) - 1` dance.

In an uncommitted iteration, I actuall forced that mask in
`diff_setup_done()` via:

	options->filter &= (1 << (ARRAY_SIZE(diff_status_letters) - 1)) - 1;

But then I got curious and looked how `options->filter` is accessed, and
we never loop over its bits, we always ask whether a specific bit is set.
So I got rid of that (quite ugly) line.

I made a mental note to write about this in the commit message, but wanted
to quickly look at all the code accessing `options->filter` by using the
very nice refactoring support of VS Code to rename the field so that all
of the accesses would be visible in a diff, and then I wanted to quickly
run the entire test suite first, just in case my analysis missed
something. And by the end of it, my mental note had evaporated.

Thanks to your reminder, I added this to the end of the commit message:

    Note: The code replaced by this commit took pains to avoid setting any
    unused bits of `options->filter`. That was unnecessary, though, as all
    accesses happen via the `filter_bit_tst()` function using specific bits,
    and setting the unused bits has no effect. Therefore, we can simplify
    the code by using `~0` (or in this instance, `~<unwanted-bit>`).

Ciao,
Dscho
diff mbox series

Patch

diff --git a/diff.c b/diff.c
index 5081052c431..4ab4299b817 100644
--- a/diff.c
+++ b/diff.c
@@ -4720,6 +4720,12 @@  void diff_setup_done(struct diff_options *options)
 	if (!options->use_color || external_diff())
 		options->color_moved = 0;
 
+	if (options->filter_not) {
+		if (!options->filter)
+			options->filter = ~filter_bit[DIFF_STATUS_FILTER_AON];
+		options->filter &= ~options->filter_not;
+	}
+
 	FREE_AND_NULL(options->parseopts);
 }
 
@@ -4820,21 +4826,6 @@  static int diff_opt_diff_filter(const struct option *option,
 	BUG_ON_OPT_NEG(unset);
 	prepare_filter_bits();
 
-	/*
-	 * If there is a negation e.g. 'd' in the input, and we haven't
-	 * initialized the filter field with another --diff-filter, start
-	 * from full set of bits, except for AON.
-	 */
-	if (!opt->filter) {
-		for (i = 0; (optch = optarg[i]) != '\0'; i++) {
-			if (optch < 'a' || 'z' < optch)
-				continue;
-			opt->filter = (1 << (ARRAY_SIZE(diff_status_letters) - 1)) - 1;
-			opt->filter &= ~filter_bit[DIFF_STATUS_FILTER_AON];
-			break;
-		}
-	}
-
 	for (i = 0; (optch = optarg[i]) != '\0'; i++) {
 		unsigned int bit;
 		int negate;
@@ -4851,7 +4842,7 @@  static int diff_opt_diff_filter(const struct option *option,
 			return error(_("unknown change class '%c' in --diff-filter=%s"),
 				     optarg[i], optarg);
 		if (negate)
-			opt->filter &= ~bit;
+			opt->filter_not |= bit;
 		else
 			opt->filter |= bit;
 	}
diff --git a/diff.h b/diff.h
index 8ba85c5e605..a70e7c478c1 100644
--- a/diff.h
+++ b/diff.h
@@ -283,7 +283,7 @@  struct diff_options {
 	struct diff_flags flags;
 
 	/* diff-filter bits */
-	unsigned int filter;
+	unsigned int filter, filter_not;
 
 	int use_color;
 
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 50495598619..b25182379ff 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -142,6 +142,19 @@  test_expect_success 'diff-filter=R' '
 
 '
 
+test_expect_success 'multiple --diff-filter bits' '
+
+	git log -M --pretty="format:%s" --diff-filter=R HEAD >expect &&
+	git log -M --pretty="format:%s" --diff-filter=Ra HEAD >actual &&
+	test_cmp expect actual &&
+	git log -M --pretty="format:%s" --diff-filter=aR HEAD >actual &&
+	test_cmp expect actual &&
+	git log -M --pretty="format:%s" \
+		--diff-filter=a --diff-filter=R HEAD >actual &&
+	test_cmp expect actual
+
+'
+
 test_expect_success 'diff-filter=C' '
 
 	git log -C -C --pretty="format:%s" --diff-filter=C HEAD >actual &&