Fix two --diff-filter bugs

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

From: Johannes Schindelin <[email protected]>

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.

---
 Documentation/diff-options.txt | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

index c89d530d3d1..2549df0d212 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 diff.c         | 8 +++-----
 t/t4202-log.sh | 8 ++++++++
 2 files changed, 11 insertions(+), 5 deletions(-)

index c862771a589..fc1151b9c73 100644
@@ -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;
 		}
 	}
 

index 50495598619..28f727937dd 100755
@@ -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

"Johannes Schindelin via GitGitGadget" <[email protected]>

Good find. Looks good.

"Johannes Schindelin via GitGitGadget" <[email protected]>

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

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

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

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

 diff.c         | 31 ++++++++++++++++++++++++++++---
 t/t4202-log.sh |  4 +++-
 2 files changed, 31 insertions(+), 4 deletions(-)

index fc1151b9c7..a57e458f63 100644
@@ -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];
 		}

index 28f727937d..128183e66f 100755
@@ -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

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.

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 <[email protected]>
      
       ## 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

From: Johannes Schindelin <[email protected]>

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.

---
 Documentation/diff-options.txt | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

index c89d530d3d1..2549df0d212 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 diff.c | 74 +++++++++++++++++++++++++++++-----------------------------
 1 file changed, 37 insertions(+), 37 deletions(-)

index c862771a589..5081052c431 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 diff.c         | 23 +++++++----------------
 diff.h         |  2 +-
 t/t4202-log.sh | 13 +++++++++++++
 3 files changed, 21 insertions(+), 17 deletions(-)

index 5081052c431..4ab4299b817 100644
@@ -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;
 	}

index 8ba85c5e605..a70e7c478c1 100644
@@ -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;
 

index 50495598619..b25182379ff 100755
@@ -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

"Johannes Schindelin via GitGitGadget" <[email protected]>

Hi Taylor,

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

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

Correct.

Ciao,
Dscho

Hi Junio,

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

Hi Junio,

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

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

## diff.c ##

gitgitgadget

From: Johannes Schindelin <[email protected]>

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.

---
 Documentation/diff-options.txt | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

index c89d530d3d1..2549df0d212 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 diff.c | 74 +++++++++++++++++++++++++++++-----------------------------
 1 file changed, 37 insertions(+), 37 deletions(-)

index c862771a589..5081052c431 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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

---
 diff.c         | 23 +++++++----------------
 diff.h         |  2 +-
 t/t4202-log.sh | 13 +++++++++++++
 3 files changed, 21 insertions(+), 17 deletions(-)

index 5081052c431..4ab4299b817 100644
@@ -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;
 	}

index 8ba85c5e605..a70e7c478c1 100644
@@ -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;
 

index 50495598619..b25182379ff 100755
@@ -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