[PATCH 0/2] Fix two --diff-filter bugs

[PATCH 0/2] Fix two --diff-filter bugs

[Johannes Schindelin via GitGitGadget]

A colleague noticed that git diff --diff-filter=Dr behaved in an unexpected
way. The expectation was that the command shows only deleted files, but not
renamed ones.

Turns out that Git's code is incorrect and turns on all diff-filter flags
because the argument contains a lower-case letter. But since it starts with
an upper-case letter, we should actually not turn all those flags on.

While working on the fix, I realized that the documentation of the
--diff-filter flag was not updated when intent-to-add files were no longer
shown as modified by git diff, but as added.

Johannes Schindelin (2):
  docs(diff): lose incorrect claim about `diff-files --diff-filter=A`
  diff-filter: be more careful when looking for negative bits

 Documentation/diff-options.txt | 7 ++-----
 diff.c                         | 8 +++-----
 t/t4202-log.sh                 | 8 ++++++++
 3 files changed, 13 insertions(+), 10 deletions(-)


base-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1127%2Fdscho%2Fdiff-filter-buglets-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1127/dscho/diff-filter-buglets-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1127
-- 
gitgitgadget

[PATCH 1/2] docs(diff): lose incorrect claim about `diff-files --diff-filter=A`

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Originally, before we had `--intent-to-add`, there was no way that `git
diff-files` could see added files: if a file did not exist in the index,
`git diff-files` would not show it because it looks only at worktree
files when there is an index entry at the same path.

We used this example in the documentation of the diff options to explain
that not every `--diff-filter=<option>` has an effect in all scenarios.

Even when we added `--intent-to-add`, the comment was still correct,
because initially we showed such files as modified instead of added.

However, when that bug was fixed in feea6946a5b (diff-files: treat
"i-t-a" files as "not-in-index", 2020-06-20), the comment in the
documentation became incorrect.

Let's just remove it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/diff-options.txt | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index c89d530d3d1..2549df0d212 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -616,11 +616,8 @@ ifndef::git-format-patch[]
 Also, these upper-case letters can be downcased to exclude.  E.g.
 `--diff-filter=ad` excludes added and deleted paths.
 +
-Note that not all diffs can feature all types. For instance, diffs
-from the index to the working tree can never have Added entries
-(because the set of paths included in the diff is limited by what is in
-the index).  Similarly, copied and renamed entries cannot appear if
-detection for those types is disabled.
+Note that not all diffs can feature all types. For instance, copied and
+renamed entries cannot appear if detection for those types is disabled.
 
 -S<string>::
 	Look for differences that change the number of occurrences of
-- 
gitgitgadget

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

[Johannes Schindelin via GitGitGadget]

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.

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) {
 			opt->filter = (1 << (ARRAY_SIZE(diff_status_letters) - 1)) - 1;
 			opt->filter &= ~filter_bit[DIFF_STATUS_FILTER_AON];
-			break;
 		}
 	}
 
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
+
+'
+
 test_expect_success 'diff-filter=C' '
 
 	git log -C -C --pretty="format:%s" --diff-filter=C HEAD >actual &&
-- 
gitgitgadget

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

[Taylor Blau]

On Tue, Jan 25, 2022 at 10:29:19PM +0000, Johannes Schindelin via GitGitGadget wrote:
> 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) {
>  			opt->filter = (1 << (ARRAY_SIZE(diff_status_letters) - 1)) - 1;
>  			opt->filter &= ~filter_bit[DIFF_STATUS_FILTER_AON];
> -			break;
>  		}
>  	}

Thinking through how this would have worked before with
`--diff-filter=Dr`, I think it goes something like:

  1. We set all bits (except the all-or-none bit) on via the first loop.
  2. Then we OR in the bit for deletions, which does not change the
     overall filter (since it was already set by the previous step).
  3. Then we unset the bit corresponding to renames.

That leaves us with all bits on except two: DIFF_STATUS_RENAMED and
DIFF_STATUS_FILTER_AON.

As far as I can understand, the AON "filter" shows all files as long as
at least one of them matches the filter, otherwise it shows nothing at
all.

But that doesn't save us, since we have many more bits on than we should
have, meaning that `--diff-filter=Dr` doesn't work at all (assuming you
expected it to show just deletions, like `--diff-filter=D` does).

It's possible that I don't understand what the all-or-nothing bit is
supposed to be doing, though.

Thanks,
Taylor

Re: [PATCH 1/2] docs(diff): lose incorrect claim about `diff-files --diff-filter=A`

[Junio C Hamano]

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

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Originally, before we had `--intent-to-add`, there was no way that `git
> diff-files` could see added files: if a file did not exist in the index,
> `git diff-files` would not show it because it looks only at worktree
> files when there is an index entry at the same path.

Good find.  Looks good.

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

[Junio C Hamano]

"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

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

[Ævar Arnfjörð Bjarmason]

On Tue, Jan 25 2022, Johannes Schindelin via GitGitGadget wrote:

> 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.
>
> 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) {

We'll probably never have to deal with non-ASCII, so maybe this is being
overzelous, but perhaps changing this to islower(optch) is worth it?

This relies on non-standard C both in the pre- and post-image, but in
reality it works everywhere, until someone attempts to port git to an
EBCDIC system...

>  			opt->filter = (1 << (ARRAY_SIZE(diff_status_letters) - 1)) - 1;
>  			opt->filter &= ~filter_bit[DIFF_STATUS_FILTER_AON];
> -			break;
>  		}
>  	}
>  
> 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' '
> +

nit: extra \n

> +	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
> +
> +'
> +
>  test_expect_success 'diff-filter=C' '
>  
>  	git log -C -C --pretty="format:%s" --diff-filter=C HEAD >actual &&

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

[Taylor Blau]

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

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

>> +		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) {
>
> (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

These variables are indeed used in the boolean sense (see how they
are used in the condition in the "if" statement).

If we wanted to short-circuit, we could do this:

	for (i = 0;
	     !has_positive && !has_negative &&
	     (optch = optarg[i]) != '\0';
	     i++) {
		if (isupper(optch))
			has_negative++;
		else
			has_positive++;
	}

and thanks to their names, nobody would be confused to find it is a
bug that these do not count when the loop exits early.  We shouldn't
name these positive_nr or negative_nr because we are not interested
in counting them.

It is just that "bool++" is more idiomatic than repeated assignment
"bool = 1" when setter and getter both know it is merely used as a
Boolean, and that is why they named as has_X, which clearly is a
name for a Boolean, not a counter.

Having said that, I do not mind if an assignment is used instead of
post-increment in new code.  I just think it is waste of time to go
find increments that toggle a Boolean to true and changing them to
assignments, and it is even more waste having to review such a code
churn, so let's not go there ;-)

But as I wrote in the big NEEDSWORK comment, this loop should
disappear when the code is properly rewritten to correct the
interaction between two or more "--diff-filter=" options, so it
would not matter too much.

Thanks.

[PATCH v2 0/3] Fix two --diff-filter bugs

[Johannes Schindelin via GitGitGadget]

A colleague noticed that git diff --diff-filter=Dr behaved in an unexpected
way. The expectation was that the command shows only deleted files, but not
renamed ones.

Turns out that Git's code is incorrect and turns on all diff-filter flags
because the argument contains a lower-case letter. But since it starts with
an upper-case letter, we should actually not turn all those flags on.

While working on the fix, I realized that the documentation of the
--diff-filter flag was not updated when intent-to-add files were no longer
shown as modified by git diff, but as added.

Changes since v1:

 * Now even the case of multiple --diff-filter options is handled.

Johannes Schindelin (3):
  docs(diff): lose incorrect claim about `diff-files --diff-filter=A`
  diff.c: move the diff filter bits definitions up a bit
  diff-filter: be more careful when looking for negative bits

 Documentation/diff-options.txt |  7 +--
 diff.c                         | 97 +++++++++++++++-------------------
 diff.h                         |  2 +-
 t/t4202-log.sh                 | 13 +++++
 4 files changed, 60 insertions(+), 59 deletions(-)


base-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1127%2Fdscho%2Fdiff-filter-buglets-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1127/dscho/diff-filter-buglets-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1127

Range-diff vs v1:

 1:  704bb2ba18e = 1:  704bb2ba18e docs(diff): lose incorrect claim about `diff-files --diff-filter=A`
 -:  ----------- > 2:  19c7223e265 diff.c: move the diff filter bits definitions up a bit
 2:  e8006493a9e ! 3:  b041d2b7a3b diff-filter: be more careful when looking for negative bits
     @@ Commit message
          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.
      
     +    Even worse, it is possible to specify the diff filters in multiple,
     +    separate options, e.g. `--diff-filter=AM [...] --diff-filter=m`.
     +
     +    Let's accumulate the include/exclude filters independently, and only
     +    special-case the "only exclude filters were specified" case after
     +    parsing the options altogether.
     +
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## diff.c ##
     +@@ diff.c: 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];
     ++		options->filter &= ~options->filter_not;
     ++	}
     ++
     + 	FREE_AND_NULL(options->parseopts);
     + }
     + 
      @@ diff.c: static int diff_opt_diff_filter(const struct option *option,
     + 	BUG_ON_OPT_NEG(unset);
       	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) {
     +-	 * 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) {
     - 			opt->filter = (1 << (ARRAY_SIZE(diff_status_letters) - 1)) - 1;
     - 			opt->filter &= ~filter_bit[DIFF_STATUS_FILTER_AON];
     +-			opt->filter = (1 << (ARRAY_SIZE(diff_status_letters) - 1)) - 1;
     +-			opt->filter &= ~filter_bit[DIFF_STATUS_FILTER_AON];
      -			break;
     - 		}
     +-		}
     +-	}
     +-
     + 	for (i = 0; (optch = optarg[i]) != '\0'; i++) {
     + 		unsigned int bit;
     + 		int negate;
     +@@ diff.c: static int diff_opt_diff_filter(const struct option *option,
     + 			return error(_("unknown change class '%c' in --diff-filter=%s"),
     + 				     optarg[i], optarg);
     + 		if (negate)
     +-			opt->filter &= ~bit;
     ++			opt->filter_not |= bit;
     + 		else
     + 			opt->filter |= bit;
       	}
     +
     + ## diff.h ##
     +@@ diff.h: struct diff_options {
     + 	struct diff_flags flags;
     + 
     + 	/* diff-filter bits */
     +-	unsigned int filter;
     ++	unsigned int filter, filter_not;
     + 
     + 	int use_color;
       
      
       ## t/t4202-log.sh ##
     @@ t/t4202-log.sh: test_expect_success 'diff-filter=R' '
       
       '
       
     -+test_expect_success 'diff-filter=Ra' '
     ++test_expect_success 'multiple --diff-filter bits' '
      +
      +	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 &&
     ++	git log -M --pretty="format:%s" \
     ++		--diff-filter=a --diff-filter=R HEAD >actual &&
      +	test_cmp expect actual
      +
      +'

-- 
gitgitgadget

[PATCH v2 1/3] docs(diff): lose incorrect claim about `diff-files --diff-filter=A`

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Originally, before we had `--intent-to-add`, there was no way that `git
diff-files` could see added files: if a file did not exist in the index,
`git diff-files` would not show it because it looks only at worktree
files when there is an index entry at the same path.

We used this example in the documentation of the diff options to explain
that not every `--diff-filter=<option>` has an effect in all scenarios.

Even when we added `--intent-to-add`, the comment was still correct,
because initially we showed such files as modified instead of added.

However, when that bug was fixed in feea6946a5b (diff-files: treat
"i-t-a" files as "not-in-index", 2020-06-20), the comment in the
documentation became incorrect.

Let's just remove it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/diff-options.txt | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index c89d530d3d1..2549df0d212 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -616,11 +616,8 @@ ifndef::git-format-patch[]
 Also, these upper-case letters can be downcased to exclude.  E.g.
 `--diff-filter=ad` excludes added and deleted paths.
 +
-Note that not all diffs can feature all types. For instance, diffs
-from the index to the working tree can never have Added entries
-(because the set of paths included in the diff is limited by what is in
-the index).  Similarly, copied and renamed entries cannot appear if
-detection for those types is disabled.
+Note that not all diffs can feature all types. For instance, copied and
+renamed entries cannot appear if detection for those types is disabled.
 
 -S<string>::
 	Look for differences that change the number of occurrences of
-- 
gitgitgadget

[PATCH v2 2/3] diff.c: move the diff filter bits definitions up a bit

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

This prepares for a more careful handling of the `--diff-filter`
options over the next few commits.

This commit is best viewed with `--color-moved`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 diff.c | 74 +++++++++++++++++++++++++++++-----------------------------
 1 file changed, 37 insertions(+), 37 deletions(-)

diff --git a/diff.c b/diff.c
index c862771a589..5081052c431 100644
--- a/diff.c
+++ b/diff.c
@@ -4570,6 +4570,43 @@ void repo_diff_setup(struct repository *r, struct diff_options *options)
 	prep_parse_options(options);
 }
 
+static const char diff_status_letters[] = {
+	DIFF_STATUS_ADDED,
+	DIFF_STATUS_COPIED,
+	DIFF_STATUS_DELETED,
+	DIFF_STATUS_MODIFIED,
+	DIFF_STATUS_RENAMED,
+	DIFF_STATUS_TYPE_CHANGED,
+	DIFF_STATUS_UNKNOWN,
+	DIFF_STATUS_UNMERGED,
+	DIFF_STATUS_FILTER_AON,
+	DIFF_STATUS_FILTER_BROKEN,
+	'\0',
+};
+
+static unsigned int filter_bit['Z' + 1];
+
+static void prepare_filter_bits(void)
+{
+	int i;
+
+	if (!filter_bit[DIFF_STATUS_ADDED]) {
+		for (i = 0; diff_status_letters[i]; i++)
+			filter_bit[(int) diff_status_letters[i]] = (1 << i);
+	}
+}
+
+static unsigned filter_bit_tst(char status, const struct diff_options *opt)
+{
+	return opt->filter & filter_bit[(int) status];
+}
+
+unsigned diff_filter_bit(char status)
+{
+	prepare_filter_bits();
+	return filter_bit[(int) status];
+}
+
 void diff_setup_done(struct diff_options *options)
 {
 	unsigned check_mask = DIFF_FORMAT_NAME |
@@ -4774,43 +4811,6 @@ static int parse_dirstat_opt(struct diff_options *options, const char *params)
 	return 1;
 }
 
-static const char diff_status_letters[] = {
-	DIFF_STATUS_ADDED,
-	DIFF_STATUS_COPIED,
-	DIFF_STATUS_DELETED,
-	DIFF_STATUS_MODIFIED,
-	DIFF_STATUS_RENAMED,
-	DIFF_STATUS_TYPE_CHANGED,
-	DIFF_STATUS_UNKNOWN,
-	DIFF_STATUS_UNMERGED,
-	DIFF_STATUS_FILTER_AON,
-	DIFF_STATUS_FILTER_BROKEN,
-	'\0',
-};
-
-static unsigned int filter_bit['Z' + 1];
-
-static void prepare_filter_bits(void)
-{
-	int i;
-
-	if (!filter_bit[DIFF_STATUS_ADDED]) {
-		for (i = 0; diff_status_letters[i]; i++)
-			filter_bit[(int) diff_status_letters[i]] = (1 << i);
-	}
-}
-
-static unsigned filter_bit_tst(char status, const struct diff_options *opt)
-{
-	return opt->filter & filter_bit[(int) status];
-}
-
-unsigned diff_filter_bit(char status)
-{
-	prepare_filter_bits();
-	return filter_bit[(int) status];
-}
-
 static int diff_opt_diff_filter(const struct option *option,
 				const char *optarg, int unset)
 {
-- 
gitgitgadget

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

[Johannes Schindelin via GitGitGadget]

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.

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.

Even worse, it is possible to specify the diff filters in multiple,
separate options, e.g. `--diff-filter=AM [...] --diff-filter=m`.

Let's accumulate the include/exclude filters independently, and only
special-case the "only exclude filters were specified" case after
parsing the options altogether.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 diff.c         | 23 +++++++----------------
 diff.h         |  2 +-
 t/t4202-log.sh | 13 +++++++++++++
 3 files changed, 21 insertions(+), 17 deletions(-)

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];
+		options->filter &= ~options->filter_not;
+	}
+
 	FREE_AND_NULL(options->parseopts);
 }
 
@@ -4820,21 +4826,6 @@ static int diff_opt_diff_filter(const struct option *option,
 	BUG_ON_OPT_NEG(unset);
 	prepare_filter_bits();
 
-	/*
-	 * 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.
-	 */
-	if (!opt->filter) {
-		for (i = 0; (optch = optarg[i]) != '\0'; i++) {
-			if (optch < 'a' || 'z' < optch)
-				continue;
-			opt->filter = (1 << (ARRAY_SIZE(diff_status_letters) - 1)) - 1;
-			opt->filter &= ~filter_bit[DIFF_STATUS_FILTER_AON];
-			break;
-		}
-	}
-
 	for (i = 0; (optch = optarg[i]) != '\0'; i++) {
 		unsigned int bit;
 		int negate;
@@ -4851,7 +4842,7 @@ static int diff_opt_diff_filter(const struct option *option,
 			return error(_("unknown change class '%c' in --diff-filter=%s"),
 				     optarg[i], optarg);
 		if (negate)
-			opt->filter &= ~bit;
+			opt->filter_not |= bit;
 		else
 			opt->filter |= bit;
 	}
diff --git a/diff.h b/diff.h
index 8ba85c5e605..a70e7c478c1 100644
--- a/diff.h
+++ b/diff.h
@@ -283,7 +283,7 @@ struct diff_options {
 	struct diff_flags flags;
 
 	/* diff-filter bits */
-	unsigned int filter;
+	unsigned int filter, filter_not;
 
 	int use_color;
 
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 50495598619..b25182379ff 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -142,6 +142,19 @@ test_expect_success 'diff-filter=R' '
 
 '
 
+test_expect_success 'multiple --diff-filter bits' '
+
+	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 &&
+	git log -M --pretty="format:%s" \
+		--diff-filter=a --diff-filter=R HEAD >actual &&
+	test_cmp expect actual
+
+'
+
 test_expect_success 'diff-filter=C' '
 
 	git log -C -C --pretty="format:%s" --diff-filter=C HEAD >actual &&
-- 
gitgitgadget

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

[Junio C Hamano]

"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.  I do not know offhand if that makes the difference, but I
trust that you have audited all uses of the options->filter flag
word and these high bits are truly unused and the difference does
not matter.

> +		options->filter &= ~options->filter_not;
> +	}

>  	for (i = 0; (optch = optarg[i]) != '\0'; i++) {
>  		unsigned int bit;
>  		int negate;
> @@ -4851,7 +4842,7 @@ static int diff_opt_diff_filter(const struct option *option,
>  			return error(_("unknown change class '%c' in --diff-filter=%s"),
>  				     optarg[i], optarg);
>  		if (negate)
> -			opt->filter &= ~bit;
> +			opt->filter_not |= bit;
>  		else
>  			opt->filter |= bit;
>  	}

And this ... 

> diff --git a/diff.h b/diff.h
> index 8ba85c5e605..a70e7c478c1 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -283,7 +283,7 @@ struct diff_options {
>  	struct diff_flags flags;
>  
>  	/* diff-filter bits */
> -	unsigned int filter;
> +	unsigned int filter, filter_not;

... is exactly I wrote in the NEEDSWORK comment I gave you in my
earlier review.  Excellent.

> +test_expect_success 'multiple --diff-filter bits' '
> +
> +	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 &&
> +	git log -M --pretty="format:%s" \
> +		--diff-filter=a --diff-filter=R HEAD >actual &&
> +	test_cmp expect actual
> +
> +'

Good.

Thanks for noticing and fixing the long-standing issue.

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

[Johannes Schindelin]

Hi Taylor,

On Tue, 25 Jan 2022, Taylor Blau wrote:

> On Tue, Jan 25, 2022 at 10:29:19PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > 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) {
> >  			opt->filter = (1 << (ARRAY_SIZE(diff_status_letters) - 1)) - 1;
> >  			opt->filter &= ~filter_bit[DIFF_STATUS_FILTER_AON];
> > -			break;
> >  		}
> >  	}
>
> Thinking through how this would have worked before with
> `--diff-filter=Dr`, I think it goes something like:
>
>   1. We set all bits (except the all-or-none bit) on via the first loop.
>   2. Then we OR in the bit for deletions, which does not change the
>      overall filter (since it was already set by the previous step).
>   3. Then we unset the bit corresponding to renames.
>
> That leaves us with all bits on except two: DIFF_STATUS_RENAMED and
> DIFF_STATUS_FILTER_AON.

Correct. And since we asked only for "Deleted", we get way more than we
bargained for.

> As far as I can understand, the AON "filter" shows all files as long as
> at least one of them matches the filter, otherwise it shows nothing at
> all.

Right, so on its own, it is quite useless. It needs to be combined with
another diff filter to make sense.

> But that doesn't save us, since we have many more bits on than we should
> have, meaning that `--diff-filter=Dr` doesn't work at all (assuming you
> expected it to show just deletions, like `--diff-filter=D` does).

Correct.

Ciao,
Dscho

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

[Johannes Schindelin]

Hi Junio,

On Wed, 26 Jan 2022, Junio C Hamano wrote:

> [...] this loop should disappear when the code is properly rewritten to
> correct the interaction between two or more "--diff-filter=" options, so
> it would not matter too much.

And so it did. Thank you for the suggestion. At first I thought that it
would require too extensive a code change, but was pleasantly surprised
when I figured out that the final step can be done as part of
`diff_setup_done()`, with a quite pleasant-to-read patch, if I may say so.

Ciao,
Dscho

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

[Johannes Schindelin]

Hi Junio,

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

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

[PATCH v3 0/3] Fix two --diff-filter bugs

[Johannes Schindelin via GitGitGadget]

A colleague noticed that git diff --diff-filter=Dr behaved in an unexpected
way. The expectation was that the command shows only deleted files, but not
renamed ones.

Turns out that Git's code is incorrect and turns on all diff-filter flags
because the argument contains a lower-case letter. But since it starts with
an upper-case letter, we should actually not turn all those flags on.

While working on the fix, I realized that the documentation of the
--diff-filter flag was not updated when intent-to-add files were no longer
shown as modified by git diff, but as added.

Changes since v2:

 * Augmented the commit message to explain why we do not need to be careful
   about setting only the used bits of options->filter.

Changes since v1:

 * Now even the case of multiple --diff-filter options is handled.

Johannes Schindelin (3):
  docs(diff): lose incorrect claim about `diff-files --diff-filter=A`
  diff.c: move the diff filter bits definitions up a bit
  diff-filter: be more careful when looking for negative bits

 Documentation/diff-options.txt |  7 +--
 diff.c                         | 97 +++++++++++++++-------------------
 diff.h                         |  2 +-
 t/t4202-log.sh                 | 13 +++++
 4 files changed, 60 insertions(+), 59 deletions(-)


base-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1127%2Fdscho%2Fdiff-filter-buglets-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1127/dscho/diff-filter-buglets-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1127

Range-diff vs v2:

 1:  704bb2ba18e = 1:  704bb2ba18e docs(diff): lose incorrect claim about `diff-files --diff-filter=A`
 2:  19c7223e265 = 2:  19c7223e265 diff.c: move the diff filter bits definitions up a bit
 3:  b041d2b7a3b ! 3:  f1f027ad61b diff-filter: be more careful when looking for negative bits
     @@ Commit message
          special-case the "only exclude filters were specified" case after
          parsing the options altogether.
      
     +    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>`).
     +
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## diff.c ##

-- 
gitgitgadget

[PATCH v3 1/3] docs(diff): lose incorrect claim about `diff-files --diff-filter=A`

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Originally, before we had `--intent-to-add`, there was no way that `git
diff-files` could see added files: if a file did not exist in the index,
`git diff-files` would not show it because it looks only at worktree
files when there is an index entry at the same path.

We used this example in the documentation of the diff options to explain
that not every `--diff-filter=<option>` has an effect in all scenarios.

Even when we added `--intent-to-add`, the comment was still correct,
because initially we showed such files as modified instead of added.

However, when that bug was fixed in feea6946a5b (diff-files: treat
"i-t-a" files as "not-in-index", 2020-06-20), the comment in the
documentation became incorrect.

Let's just remove it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/diff-options.txt | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index c89d530d3d1..2549df0d212 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -616,11 +616,8 @@ ifndef::git-format-patch[]
 Also, these upper-case letters can be downcased to exclude.  E.g.
 `--diff-filter=ad` excludes added and deleted paths.
 +
-Note that not all diffs can feature all types. For instance, diffs
-from the index to the working tree can never have Added entries
-(because the set of paths included in the diff is limited by what is in
-the index).  Similarly, copied and renamed entries cannot appear if
-detection for those types is disabled.
+Note that not all diffs can feature all types. For instance, copied and
+renamed entries cannot appear if detection for those types is disabled.
 
 -S<string>::
 	Look for differences that change the number of occurrences of
-- 
gitgitgadget

[PATCH v3 2/3] diff.c: move the diff filter bits definitions up a bit

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

This prepares for a more careful handling of the `--diff-filter`
options over the next few commits.

This commit is best viewed with `--color-moved`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 diff.c | 74 +++++++++++++++++++++++++++++-----------------------------
 1 file changed, 37 insertions(+), 37 deletions(-)

diff --git a/diff.c b/diff.c
index c862771a589..5081052c431 100644
--- a/diff.c
+++ b/diff.c
@@ -4570,6 +4570,43 @@ void repo_diff_setup(struct repository *r, struct diff_options *options)
 	prep_parse_options(options);
 }
 
+static const char diff_status_letters[] = {
+	DIFF_STATUS_ADDED,
+	DIFF_STATUS_COPIED,
+	DIFF_STATUS_DELETED,
+	DIFF_STATUS_MODIFIED,
+	DIFF_STATUS_RENAMED,
+	DIFF_STATUS_TYPE_CHANGED,
+	DIFF_STATUS_UNKNOWN,
+	DIFF_STATUS_UNMERGED,
+	DIFF_STATUS_FILTER_AON,
+	DIFF_STATUS_FILTER_BROKEN,
+	'\0',
+};
+
+static unsigned int filter_bit['Z' + 1];
+
+static void prepare_filter_bits(void)
+{
+	int i;
+
+	if (!filter_bit[DIFF_STATUS_ADDED]) {
+		for (i = 0; diff_status_letters[i]; i++)
+			filter_bit[(int) diff_status_letters[i]] = (1 << i);
+	}
+}
+
+static unsigned filter_bit_tst(char status, const struct diff_options *opt)
+{
+	return opt->filter & filter_bit[(int) status];
+}
+
+unsigned diff_filter_bit(char status)
+{
+	prepare_filter_bits();
+	return filter_bit[(int) status];
+}
+
 void diff_setup_done(struct diff_options *options)
 {
 	unsigned check_mask = DIFF_FORMAT_NAME |
@@ -4774,43 +4811,6 @@ static int parse_dirstat_opt(struct diff_options *options, const char *params)
 	return 1;
 }
 
-static const char diff_status_letters[] = {
-	DIFF_STATUS_ADDED,
-	DIFF_STATUS_COPIED,
-	DIFF_STATUS_DELETED,
-	DIFF_STATUS_MODIFIED,
-	DIFF_STATUS_RENAMED,
-	DIFF_STATUS_TYPE_CHANGED,
-	DIFF_STATUS_UNKNOWN,
-	DIFF_STATUS_UNMERGED,
-	DIFF_STATUS_FILTER_AON,
-	DIFF_STATUS_FILTER_BROKEN,
-	'\0',
-};
-
-static unsigned int filter_bit['Z' + 1];
-
-static void prepare_filter_bits(void)
-{
-	int i;
-
-	if (!filter_bit[DIFF_STATUS_ADDED]) {
-		for (i = 0; diff_status_letters[i]; i++)
-			filter_bit[(int) diff_status_letters[i]] = (1 << i);
-	}
-}
-
-static unsigned filter_bit_tst(char status, const struct diff_options *opt)
-{
-	return opt->filter & filter_bit[(int) status];
-}
-
-unsigned diff_filter_bit(char status)
-{
-	prepare_filter_bits();
-	return filter_bit[(int) status];
-}
-
 static int diff_opt_diff_filter(const struct option *option,
 				const char *optarg, int unset)
 {
-- 
gitgitgadget

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

[Johannes Schindelin via GitGitGadget]

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.

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.

Even worse, it is possible to specify the diff filters in multiple,
separate options, e.g. `--diff-filter=AM [...] --diff-filter=m`.

Let's accumulate the include/exclude filters independently, and only
special-case the "only exclude filters were specified" case after
parsing the options altogether.

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>`).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 diff.c         | 23 +++++++----------------
 diff.h         |  2 +-
 t/t4202-log.sh | 13 +++++++++++++
 3 files changed, 21 insertions(+), 17 deletions(-)

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];
+		options->filter &= ~options->filter_not;
+	}
+
 	FREE_AND_NULL(options->parseopts);
 }
 
@@ -4820,21 +4826,6 @@ static int diff_opt_diff_filter(const struct option *option,
 	BUG_ON_OPT_NEG(unset);
 	prepare_filter_bits();
 
-	/*
-	 * 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.
-	 */
-	if (!opt->filter) {
-		for (i = 0; (optch = optarg[i]) != '\0'; i++) {
-			if (optch < 'a' || 'z' < optch)
-				continue;
-			opt->filter = (1 << (ARRAY_SIZE(diff_status_letters) - 1)) - 1;
-			opt->filter &= ~filter_bit[DIFF_STATUS_FILTER_AON];
-			break;
-		}
-	}
-
 	for (i = 0; (optch = optarg[i]) != '\0'; i++) {
 		unsigned int bit;
 		int negate;
@@ -4851,7 +4842,7 @@ static int diff_opt_diff_filter(const struct option *option,
 			return error(_("unknown change class '%c' in --diff-filter=%s"),
 				     optarg[i], optarg);
 		if (negate)
-			opt->filter &= ~bit;
+			opt->filter_not |= bit;
 		else
 			opt->filter |= bit;
 	}
diff --git a/diff.h b/diff.h
index 8ba85c5e605..a70e7c478c1 100644
--- a/diff.h
+++ b/diff.h
@@ -283,7 +283,7 @@ struct diff_options {
 	struct diff_flags flags;
 
 	/* diff-filter bits */
-	unsigned int filter;
+	unsigned int filter, filter_not;
 
 	int use_color;
 
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 50495598619..b25182379ff 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -142,6 +142,19 @@ test_expect_success 'diff-filter=R' '
 
 '
 
+test_expect_success 'multiple --diff-filter bits' '
+
+	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 &&
+	git log -M --pretty="format:%s" \
+		--diff-filter=a --diff-filter=R HEAD >actual &&
+	test_cmp expect actual
+
+'
+
 test_expect_success 'diff-filter=C' '
 
 	git log -C -C --pretty="format:%s" --diff-filter=C HEAD >actual &&
-- 
gitgitgadget