git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Jan 26, 2022, 07:23 UTC
Message-ID
<xmqq1r0vt0y7.fsf@gitster.g>
In-Reply-To
<e8006493a9ed4da9b9125865e004ba7ace20e7a4.1643149759.git.gitgitgadget@gmail.com>

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

Show 6 quoted lines
> 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]
Show 31 quoted lines
>
> 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.
Show 5 quoted lines
>  			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.

Show 14 quoted lines
> 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 &&
Show 5 quoted lines
> +'
> +
>  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
Previous: Johannes SchindelinNext: Taylor Blau
Message 7 of 21 in “Fix two --diff-filter bugs”
  1. 0/2 Fix two --diff-filter bugsJohannes Schindelin via GitGitGadget, Jan 25, 2022
  2. 1/2 docs(diff): lose incorrect claim about `diff-files --diff-filter=A`Johannes Schindelin via GitGitGadget, Jan 25, 2022
  3. Junio C HamanoJan 26, 2022
  4. 2/2 diff-filter: be more careful when looking for negative bitsJohannes Schindelin via GitGitGadget, Jan 25, 2022
  5. Taylor BlauJan 25, 2022
  6. Johannes SchindelinJan 28, 2022
  7. Junio C HamanoJan 26, 2022
  8. Taylor BlauJan 26, 2022
  9. Junio C HamanoJan 26, 2022
  10. Johannes SchindelinJan 28, 2022
  11. Ævar Arnfjörð BjarmasonJan 26, 2022
  12. 0/3 Fix two --diff-filter bugsJohannes Schindelin via GitGitGadget, Jan 27, 2022
  13. 1/3 docs(diff): lose incorrect claim about `diff-files --diff-filter=A`Johannes Schindelin via GitGitGadget, Jan 27, 2022
  14. 2/3 diff.c: move the diff filter bits definitions up a bitJohannes Schindelin via GitGitGadget, Jan 27, 2022
  15. 3/3 diff-filter: be more careful when looking for negative bitsJohannes Schindelin via GitGitGadget, Jan 27, 2022
  16. Junio C HamanoJan 28, 2022
  17. Johannes SchindelinJan 28, 2022
  18. 0/3 Fix two --diff-filter bugsJohannes Schindelin via GitGitGadget, Jan 28, 2022
  19. 1/3 docs(diff): lose incorrect claim about `diff-files --diff-filter=A`Johannes Schindelin via GitGitGadget, Jan 28, 2022
  20. 2/3 diff.c: move the diff filter bits definitions up a bitJohannes Schindelin via GitGitGadget, Jan 28, 2022
  21. 3/3 diff-filter: be more careful when looking for negative bitsJohannes Schindelin via GitGitGadget, Jan 28, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.