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

[Junio C Hamano <gitster@pobox.com>]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> The `--diff-filter=<bits>` option allows to filter the diff by certain
> criteria, for example `R` to only show renamed files. It also supports
> negating a filter via a down-cased letter, i.e. `r` to show _everything
> but_ renamed files.

[jc: Squashable fix-up attached at the end]

>
> However, the code is a bit overzealous when trying to figure out whether
> `git diff` should start with all diff-filters turned on because the user
> provided a lower-case letter: if the `--diff-filter` argument starts
> with an upper-case letter, we must not start with all bits turned on.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  diff.c         | 8 +++-----
>  t/t4202-log.sh | 8 ++++++++
>  2 files changed, 11 insertions(+), 5 deletions(-)
>
> 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) {

Style.  When both sides of && must be satisfied, list these three
things in the order that should appear.  For example,

		if ('z' >= optch && optch >= 'a')

is OK because it makes it clear that optch sits between 'z' and 'a'
for the expression to be true.  The existing one is also OK for the
same reason.  The condition holds when either optch is below
(i.e. comes before) 'a', or it is above (i.e. comes after) 'z', so

		if (optch < 'a' || 'z' < optch)

orders them naturally.  Also

		if ('a' <= optch && optch <= 'z')

is good for the same reason as the first example, but probably is
better because the three things are ordered from smaller to larger,
which is usually how people count things.

I'd usually let this pass if it were new code, but because the patch
breaks the ordering the existing code has, it is a different story.

But more importantly, I do not know if the updated code is correct.

>  			opt->filter = (1 << (ARRAY_SIZE(diff_status_letters) - 1)) - 1;
>  			opt->filter &= ~filter_bit[DIFF_STATUS_FILTER_AON];
> -			break;
>  		}
>  	}

While the finding in the cover letter (i.e. "--diff-filter=Dr does
not work as expected") is certainly good, I do not know about this
implementation.  "--diff-filter=rD" and "--diff-filter=Dr" ought to
behave the same way, but if we base our logic on optarg[0], then the
first letter and only the first letter is made special, which does
not smell right.

> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 50495598619..28f727937dd 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -142,6 +142,14 @@ test_expect_success 'diff-filter=R' '
>  
>  '
>  
> +test_expect_success 'diff-filter=Ra' '
> +
> +	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
> +

In other words, this should work the same way, no?

> +	git log -M --pretty="format:%s" --diff-filter=aR HEAD >actual &&
	
> +'
> +
>  test_expect_success 'diff-filter=C' '
>  
>  	git log -C -C --pretty="format:%s" --diff-filter=C HEAD >actual &&



------------------------------------------------------------

 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];
 		}
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
 
 '
-- 
2.35.0-155-g0eb5153edc