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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jan 28, 2022, 12:01 UTC
Message-ID: <nycvar.QRO.7.76.6.2201281255110.347@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqq35l8zmkh.fsf@gitster.g>

Hi Junio,

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

Show 18 quoted lines

> "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