Re: [PATCH 2/2] diff-filter: be more careful when looking for negative bits
"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(-)
Show changes to 2 files +31 −4
diff.c, t/t4202-log.sh
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
Johannes Schindelin via GitGitGadget
