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

[Taylor Blau <me@ttaylorr.com>]

On Tue, Jan 25, 2022 at 11:23:28PM -0800, Junio C Hamano wrote:
>  diff.c         | 31 ++++++++++++++++++++++++++++---
>  t/t4202-log.sh |  4 +++-
>  2 files changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index fc1151b9c7..a57e458f63 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4821,13 +4821,38 @@ static int diff_opt_diff_filter(const struct option *option,
>  	prepare_filter_bits();
>
>  	/*
> -	 * If the input starts with a negation e.g. 'd', and we haven't
> +	 * 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.
> +	 * However, the user can try to limit to selected positive bits,
> +	 * in which case we do not have to.
> +	 *
> +	 * NEEDSWORK: the "we haven't initialied" above is meant to
> +	 * address cases where multiple options, e.g. --diff-filter=d
> +	 * --diff-filter=a are given.  But this implementation is
> +	 * insufficient when we refrain from starting from full set
> +	 * when any positive bit is given.  Consider "--diff-filter=D
> +	 * --diff-filter=r", which ought to behave the same way as
> +	 * "--diff-filter=Dr" and "--diff-filter=rD".  The right fix
> +	 * would probably involve two "opt->filter[NP]" fields,
> +	 * records positive and negative bits separately in them while
> +	 * parsing, and then after processing all options, compute
> +	 * opt->filter by subtracting opt->filterN from opt->filterP
> +	 * (and when we do so, fill opt->filterP to full bits if it is
> +	 * absolutely empty).
>  	 */
>  	if (!opt->filter) {
> -		optch = optarg[0];
> -		if (optch >= 'a' && 'z' >= optch) {
> +		int has_positive = 0;
> +		int has_negative = 0;
> +
> +		for (i = 0; (optch = optarg[i]) != '\0'; i++) {
> +			if (optch < 'a' || 'z' < optch)
> +				has_positive++;
> +			else
> +				has_negative++;
> +		}
> +
> +		if (!has_positive && has_negative) {
>  			opt->filter = (1 << (ARRAY_SIZE(diff_status_letters) - 1)) - 1;
>  			opt->filter &= ~filter_bit[DIFF_STATUS_FILTER_AON];
>  		}

Ahh. I feel much better about this implementation. Something was nagging
me about treating optarg[0] specially, and you put very succinctly what
it was that was bothering me.

(One small nit that I absolutely do not care about is using a variable
that starts with 'has_'--which I would expect to be a boolean--to count
the number of positive/negative filters. Perhaps calling these
positive_nr, and negative_nr, respectively, would be clearer.)

> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 28f727937d..128183e66f 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -142,10 +142,12 @@ test_expect_success 'diff-filter=R' '
>
>  '
>
> -test_expect_success 'diff-filter=Ra' '
> +test_expect_success 'diff-filter=Ra and aR' '
>
>  	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

Perfect.

Thanks,
Taylor