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

Message

Philippe Blain via GitGitGadget Jan. 27, 2022, 7:08 p.m. UTC

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 <[e-mail]>
      
       ## 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
      +
      +'