Re: [PATCH 2/2] diff-filter: be more careful when looking for negative bits
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Jan 26, 2022, 22:55 UTC
- Message-ID
- <YfHRTrdwZVwxcPBK@nand.local>
- In-Reply-To
- <xmqq1r0vt0y7.fsf@gitster.g>
On Tue, Jan 25, 2022 at 11:23:28PM -0800, Junio C Hamano wrote:
Show 50 quoted lines
> 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.)
Show 16 quoted lines
> 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