Re: [PATCH 2/2] diff-filter: be more careful when looking for negative bits

From: Taylor Blau <me@ttaylorr.com>
Date: Jan 25, 2022, 23:21 UTC
Message-ID: <YfCGDY5qXPCFXpfp@nand.local>
In-Reply-To: <e8006493a9ed4da9b9125865e004ba7ace20e7a4.1643149759.git.gitgitgadget@gmail.com>

On Tue, Jan 25, 2022 at 10:29:19PM +0000, Johannes Schindelin via GitGitGadget wrote:

Show 24 quoted lines

> diff --git a/diff.c b/diff.c
> index c862771a589..fc1151b9c73 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4821,17 +4821,15 @@ static int diff_opt_diff_filter(const struct option *option,
>  	prepare_filter_bits();
>
>  	/*
> -	 * If there is a negation e.g. 'd' in the input, and we haven't
> +	 * If the input starts with a negation e.g. 'd', 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;
> +		optch = optarg[0];
> +		if (optch >= 'a' && 'z' >= optch) {
>  			opt->filter = (1 << (ARRAY_SIZE(diff_status_letters) - 1)) - 1;
>  			opt->filter &= ~filter_bit[DIFF_STATUS_FILTER_AON];
> -			break;
>  		}
>  	}

Thinking through how this would have worked before with `--diff-filter=Dr`, I think it goes something like:

  1. We set all bits (except the all-or-none bit) on via the first loop.
  2. Then we OR in the bit for deletions, which does not change the
     overall filter (since it was already set by the previous step).
  3. Then we unset the bit corresponding to renames.

That leaves us with all bits on except two: DIFF_STATUS_RENAMED and DIFF_STATUS_FILTER_AON.

As far as I can understand, the AON "filter" shows all files as long as at least one of them matches the filter, otherwise it shows nothing at all.

But that doesn't save us, since we have many more bits on than we should have, meaning that `--diff-filter=Dr` doesn't work at all (assuming you expected it to show just deletions, like `--diff-filter=D` does).

It's possible that I don't understand what the all-or-nothing bit is supposed to be doing, though.

Thanks, Taylor