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
