{"thread":{"id":"57309","subject":"[PATCH 0/2] Fix two --diff-filter bugs","startedAt":"2022-01-25T22:29:25Z","lastAt":"2022-01-28T12:02:59Z","messageCount":21,"participants":["Johannes Schindelin via GitGitGadget","Taylor Blau","Junio C Hamano","Ævar Arnfjörð Bjarmason","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"446879","messageId":"pull.1127.git.1643149759.gitgitgadget@gmail.com","threadId":"57309","inReplyTo":null,"subject":"[PATCH 0/2] Fix two --diff-filter bugs","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-25T22:29:17Z","receivedAt":"2022-01-25T22:29:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"A colleague noticed that git diff --diff-filter=Dr behaved in an unexpected\nway. The expectation was that the command shows only deleted files, but not\nrenamed ones.\n\nTurns out that Git's code is incorrect and turns on all diff-filter flags\nbecause the argument contains a lower-case letter. But since it starts with\nan upper-case letter, we should actually not turn all those flags on.\n\nWhile working on the fix, I realized that the documentation of the\n--diff-filter flag was not updated when intent-to-add files were no longer\nshown as modified by git diff, but as added.\n\nJohannes Schindelin (2):\n  docs(diff): lose incorrect claim about `diff-files --diff-filter=A`\n  diff-filter: be more careful when looking for negative bits\n\n Documentation/diff-options.txt | 7 ++-----\n diff.c                         | 8 +++-----\n t/t4202-log.sh                 | 8 ++++++++\n 3 files changed, 13 insertions(+), 10 deletions(-)\n\n\nbase-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1127%2Fdscho%2Fdiff-filter-buglets-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1127/dscho/diff-filter-buglets-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1127\n-- \ngitgitgadget\n"},{"id":"446880","messageId":"704bb2ba18ee3af5857064f6fd61689f98da0480.1643149759.git.gitgitgadget@gmail.com","threadId":"57309","inReplyTo":"pull.1127.git.1643149759.gitgitgadget@gmail.com","subject":"[PATCH 1/2] docs(diff): lose incorrect claim about `diff-files --diff-filter=A`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-25T22:29:18Z","receivedAt":"2022-01-25T22:29:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nOriginally, before we had `--intent-to-add`, there was no way that `git\ndiff-files` could see added files: if a file did not exist in the index,\n`git diff-files` would not show it because it looks only at worktree\nfiles when there is an index entry at the same path.\n\nWe used this example in the documentation of the diff options to explain\nthat not every `--diff-filter=<option>` has an effect in all scenarios.\n\nEven when we added `--intent-to-add`, the comment was still correct,\nbecause initially we showed such files as modified instead of added.\n\nHowever, when that bug was fixed in feea6946a5b (diff-files: treat\n\"i-t-a\" files as \"not-in-index\", 2020-06-20), the comment in the\ndocumentation became incorrect.\n\nLet's just remove it.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n Documentation/diff-options.txt | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex c89d530d3d1..2549df0d212 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -616,11 +616,8 @@ ifndef::git-format-patch[]\n Also, these upper-case letters can be downcased to exclude.  E.g.\n `--diff-filter=ad` excludes added and deleted paths.\n +\n-Note that not all diffs can feature all types. For instance, diffs\n-from the index to the working tree can never have Added entries\n-(because the set of paths included in the diff is limited by what is in\n-the index).  Similarly, copied and renamed entries cannot appear if\n-detection for those types is disabled.\n+Note that not all diffs can feature all types. For instance, copied and\n+renamed entries cannot appear if detection for those types is disabled.\n \n -S<string>::\n \tLook for differences that change the number of occurrences of\n-- \ngitgitgadget\n\n"},{"id":"446881","messageId":"e8006493a9ed4da9b9125865e004ba7ace20e7a4.1643149759.git.gitgitgadget@gmail.com","threadId":"57309","inReplyTo":"pull.1127.git.1643149759.gitgitgadget@gmail.com","subject":"[PATCH 2/2] diff-filter: be more careful when looking for negative bits","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-25T22:29:19Z","receivedAt":"2022-01-25T22:29:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nThe `--diff-filter=<bits>` option allows to filter the diff by certain\ncriteria, for example `R` to only show renamed files. It also supports\nnegating a filter via a down-cased letter, i.e. `r` to show _everything\nbut_ renamed files.\n\nHowever, the code is a bit overzealous when trying to figure out whether\n`git diff` should start with all diff-filters turned on because the user\nprovided a lower-case letter: if the `--diff-filter` argument starts\nwith an upper-case letter, we must not start with all bits turned on.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n diff.c         | 8 +++-----\n t/t4202-log.sh | 8 ++++++++\n 2 files changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex c862771a589..fc1151b9c73 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4821,17 +4821,15 @@ static int diff_opt_diff_filter(const struct option *option,\n \tprepare_filter_bits();\n \n \t/*\n-\t * If there is a negation e.g. 'd' in the input, and we haven't\n+\t * If the input starts with a negation e.g. 'd', and we haven't\n \t * initialized the filter field with another --diff-filter, start\n \t * from full set of bits, except for AON.\n \t */\n \tif (!opt->filter) {\n-\t\tfor (i = 0; (optch = optarg[i]) != '\\0'; i++) {\n-\t\t\tif (optch < 'a' || 'z' < optch)\n-\t\t\t\tcontinue;\n+\t\toptch = optarg[0];\n+\t\tif (optch >= 'a' && 'z' >= optch) {\n \t\t\topt->filter = (1 << (ARRAY_SIZE(diff_status_letters) - 1)) - 1;\n \t\t\topt->filter &= ~filter_bit[DIFF_STATUS_FILTER_AON];\n-\t\t\tbreak;\n \t\t}\n \t}\n \ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 50495598619..28f727937dd 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -142,6 +142,14 @@ test_expect_success 'diff-filter=R' '\n \n '\n \n+test_expect_success 'diff-filter=Ra' '\n+\n+\tgit log -M --pretty=\"format:%s\" --diff-filter=R HEAD >expect &&\n+\tgit log -M --pretty=\"format:%s\" --diff-filter=Ra HEAD >actual &&\n+\ttest_cmp expect actual\n+\n+'\n+\n test_expect_success 'diff-filter=C' '\n \n \tgit log -C -C --pretty=\"format:%s\" --diff-filter=C HEAD >actual &&\n-- \ngitgitgadget\n"},{"id":"446894","messageId":"YfCGDY5qXPCFXpfp@nand.local","threadId":"57309","inReplyTo":"e8006493a9ed4da9b9125865e004ba7ace20e7a4.1643149759.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] diff-filter: be more careful when looking for negative bits","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-01-25T23:21:49Z","receivedAt":"2022-01-25T23:21:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jan 25, 2022 at 10:29:19PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> diff --git a/diff.c b/diff.c\n> index c862771a589..fc1151b9c73 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -4821,17 +4821,15 @@ static int diff_opt_diff_filter(const struct option *option,\n>  \tprepare_filter_bits();\n>\n>  \t/*\n> -\t * If there is a negation e.g. 'd' in the input, and we haven't\n> +\t * If the input starts with a negation e.g. 'd', and we haven't\n>  \t * initialized the filter field with another --diff-filter, start\n>  \t * from full set of bits, except for AON.\n>  \t */\n>  \tif (!opt->filter) {\n> -\t\tfor (i = 0; (optch = optarg[i]) != '\\0'; i++) {\n> -\t\t\tif (optch < 'a' || 'z' < optch)\n> -\t\t\t\tcontinue;\n> +\t\toptch = optarg[0];\n> +\t\tif (optch >= 'a' && 'z' >= optch) {\n>  \t\t\topt->filter = (1 << (ARRAY_SIZE(diff_status_letters) - 1)) - 1;\n>  \t\t\topt->filter &= ~filter_bit[DIFF_STATUS_FILTER_AON];\n> -\t\t\tbreak;\n>  \t\t}\n>  \t}\n\nThinking through how this would have worked before with\n`--diff-filter=Dr`, I think it goes something like:\n\n  1. We set all bits (except the all-or-none bit) on via the first loop.\n  2. Then we OR in the bit for deletions, which does not change the\n     overall filter (since it was already set by the previous step).\n  3. Then we unset the bit corresponding to renames.\n\nThat leaves us with all bits on except two: DIFF_STATUS_RENAMED and\nDIFF_STATUS_FILTER_AON.\n\nAs far as I can understand, the AON \"filter\" shows all files as long as\nat least one of them matches the filter, otherwise it shows nothing at\nall.\n\nBut that doesn't save us, since we have many more bits on than we should\nhave, meaning that `--diff-filter=Dr` doesn't work at all (assuming you\nexpected it to show just deletions, like `--diff-filter=D` does).\n\nIt's possible that I don't understand what the all-or-nothing bit is\nsupposed to be doing, though.\n\nThanks,\nTaylor\n"},{"id":"446916","messageId":"xmqqczkft1vm.fsf@gitster.g","threadId":"57309","inReplyTo":"704bb2ba18ee3af5857064f6fd61689f98da0480.1643149759.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] docs(diff): lose incorrect claim about `diff-files --diff-filter=A`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-26T07:03:25Z","receivedAt":"2022-01-26T07:03:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\nwrites:\n\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n>\n> Originally, before we had `--intent-to-add`, there was no way that `git\n> diff-files` could see added files: if a file did not exist in the index,\n> `git diff-files` would not show it because it looks only at worktree\n> files when there is an index entry at the same path.\n\nGood find.  Looks good.\n"},{"id":"446919","messageId":"xmqq1r0vt0y7.fsf@gitster.g","threadId":"57309","inReplyTo":"e8006493a9ed4da9b9125865e004ba7ace20e7a4.1643149759.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] diff-filter: be more careful when looking for negative bits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-26T07:23:28Z","receivedAt":"2022-01-26T07:23:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\nwrites:\n\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n>\n> The `--diff-filter=<bits>` option allows to filter the diff by certain\n> criteria, for example `R` to only show renamed files. It also supports\n> negating a filter via a down-cased letter, i.e. `r` to show _everything\n> but_ renamed files.\n\n[jc: Squashable fix-up attached at the end]\n\n>\n> However, the code is a bit overzealous when trying to figure out whether\n> `git diff` should start with all diff-filters turned on because the user\n> provided a lower-case letter: if the `--diff-filter` argument starts\n> with an upper-case letter, we must not start with all bits turned on.\n>\n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n>  diff.c         | 8 +++-----\n>  t/t4202-log.sh | 8 ++++++++\n>  2 files changed, 11 insertions(+), 5 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index c862771a589..fc1151b9c73 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -4821,17 +4821,15 @@ static int diff_opt_diff_filter(const struct option *option,\n>  \tprepare_filter_bits();\n>  \n>  \t/*\n> -\t * If there is a negation e.g. 'd' in the input, and we haven't\n> +\t * If the input starts with a negation e.g. 'd', and we haven't\n>  \t * initialized the filter field with another --diff-filter, start\n>  \t * from full set of bits, except for AON.\n>  \t */\n>  \tif (!opt->filter) {\n> -\t\tfor (i = 0; (optch = optarg[i]) != '\\0'; i++) {\n> -\t\t\tif (optch < 'a' || 'z' < optch)\n> -\t\t\t\tcontinue;\n> +\t\toptch = optarg[0];\n> +\t\tif (optch >= 'a' && 'z' >= optch) {\n\nStyle.  When both sides of && must be satisfied, list these three\nthings in the order that should appear.  For example,\n\n\t\tif ('z' >= optch && optch >= 'a')\n\nis OK because it makes it clear that optch sits between 'z' and 'a'\nfor the expression to be true.  The existing one is also OK for the\nsame reason.  The condition holds when either optch is below\n(i.e. comes before) 'a', or it is above (i.e. comes after) 'z', so\n\n\t\tif (optch < 'a' || 'z' < optch)\n\norders them naturally.  Also\n\n\t\tif ('a' <= optch && optch <= 'z')\n\nis good for the same reason as the first example, but probably is\nbetter because the three things are ordered from smaller to larger,\nwhich is usually how people count things.\n\nI'd usually let this pass if it were new code, but because the patch\nbreaks the ordering the existing code has, it is a different story.\n\nBut more importantly, I do not know if the updated code is correct.\n\n>  \t\t\topt->filter = (1 << (ARRAY_SIZE(diff_status_letters) - 1)) - 1;\n>  \t\t\topt->filter &= ~filter_bit[DIFF_STATUS_FILTER_AON];\n> -\t\t\tbreak;\n>  \t\t}\n>  \t}\n\nWhile the finding in the cover letter (i.e. \"--diff-filter=Dr does\nnot work as expected\") is certainly good, I do not know about this\nimplementation.  \"--diff-filter=rD\" and \"--diff-filter=Dr\" ought to\nbehave the same way, but if we base our logic on optarg[0], then the\nfirst letter and only the first letter is made special, which does\nnot smell right.\n\n> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n> index 50495598619..28f727937dd 100755\n> --- a/t/t4202-log.sh\n> +++ b/t/t4202-log.sh\n> @@ -142,6 +142,14 @@ test_expect_success 'diff-filter=R' '\n>  \n>  '\n>  \n> +test_expect_success 'diff-filter=Ra' '\n> +\n> +\tgit log -M --pretty=\"format:%s\" --diff-filter=R HEAD >expect &&\n> +\tgit log -M --pretty=\"format:%s\" --diff-filter=Ra HEAD >actual &&\n> +\ttest_cmp expect actual\n> +\n\nIn other words, this should work the same way, no?\n\n> +\tgit log -M --pretty=\"format:%s\" --diff-filter=aR HEAD >actual &&\n\t\n> +'\n> +\n>  test_expect_success 'diff-filter=C' '\n>  \n>  \tgit log -C -C --pretty=\"format:%s\" --diff-filter=C HEAD >actual &&\n\n\n\n------------------------------------------------------------\n\n diff.c         | 31 ++++++++++++++++++++++++++++---\n t/t4202-log.sh |  4 +++-\n 2 files changed, 31 insertions(+), 4 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex fc1151b9c7..a57e458f63 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4821,13 +4821,38 @@ static int diff_opt_diff_filter(const struct option *option,\n \tprepare_filter_bits();\n \n \t/*\n-\t * If the input starts with a negation e.g. 'd', and we haven't\n+\t * If there is a negation e.g. 'd' in the input, and we haven't\n \t * initialized the filter field with another --diff-filter, start\n \t * from full set of bits, except for AON.\n+\t * However, the user can try to limit to selected positive bits,\n+\t * in which case we do not have to.\n+\t *\n+\t * NEEDSWORK: the \"we haven't initialied\" above is meant to\n+\t * address cases where multiple options, e.g. --diff-filter=d\n+\t * --diff-filter=a are given.  But this implementation is\n+\t * insufficient when we refrain from starting from full set\n+\t * when any positive bit is given.  Consider \"--diff-filter=D\n+\t * --diff-filter=r\", which ought to behave the same way as\n+\t * \"--diff-filter=Dr\" and \"--diff-filter=rD\".  The right fix\n+\t * would probably involve two \"opt->filter[NP]\" fields,\n+\t * records positive and negative bits separately in them while\n+\t * parsing, and then after processing all options, compute\n+\t * opt->filter by subtracting opt->filterN from opt->filterP\n+\t * (and when we do so, fill opt->filterP to full bits if it is\n+\t * absolutely empty).\n \t */\n \tif (!opt->filter) {\n-\t\toptch = optarg[0];\n-\t\tif (optch >= 'a' && 'z' >= optch) {\n+\t\tint has_positive = 0;\n+\t\tint has_negative = 0;\n+\n+\t\tfor (i = 0; (optch = optarg[i]) != '\\0'; i++) {\n+\t\t\tif (optch < 'a' || 'z' < optch)\n+\t\t\t\thas_positive++;\n+\t\t\telse\n+\t\t\t\thas_negative++;\n+\t\t}\n+\n+\t\tif (!has_positive && has_negative) {\n \t\t\topt->filter = (1 << (ARRAY_SIZE(diff_status_letters) - 1)) - 1;\n \t\t\topt->filter &= ~filter_bit[DIFF_STATUS_FILTER_AON];\n \t\t}\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 28f727937d..128183e66f 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -142,10 +142,12 @@ test_expect_success 'diff-filter=R' '\n \n '\n \n-test_expect_success 'diff-filter=Ra' '\n+test_expect_success 'diff-filter=Ra and aR' '\n \n \tgit log -M --pretty=\"format:%s\" --diff-filter=R HEAD >expect &&\n \tgit log -M --pretty=\"format:%s\" --diff-filter=Ra HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\tgit log -M --pretty=\"format:%s\" --diff-filter=aR HEAD >actual &&\n \ttest_cmp expect actual\n \n '\n-- \n2.35.0-155-g0eb5153edc\n\n"},{"id":"446982","messageId":"220126.868rv2e5ts.gmgdl@evledraar.gmail.com","threadId":"57309","inReplyTo":"e8006493a9ed4da9b9125865e004ba7ace20e7a4.1643149759.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] diff-filter: be more careful when looking for negative bits","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-26T17:57:51Z","receivedAt":"2022-01-26T17:59:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jan 25 2022, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n>\n> The `--diff-filter=<bits>` option allows to filter the diff by certain\n> criteria, for example `R` to only show renamed files. It also supports\n> negating a filter via a down-cased letter, i.e. `r` to show _everything\n> but_ renamed files.\n>\n> However, the code is a bit overzealous when trying to figure out whether\n> `git diff` should start with all diff-filters turned on because the user\n> provided a lower-case letter: if the `--diff-filter` argument starts\n> with an upper-case letter, we must not start with all bits turned on.\n>\n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n>  diff.c         | 8 +++-----\n>  t/t4202-log.sh | 8 ++++++++\n>  2 files changed, 11 insertions(+), 5 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index c862771a589..fc1151b9c73 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -4821,17 +4821,15 @@ static int diff_opt_diff_filter(const struct option *option,\n>  \tprepare_filter_bits();\n>  \n>  \t/*\n> -\t * If there is a negation e.g. 'd' in the input, and we haven't\n> +\t * If the input starts with a negation e.g. 'd', and we haven't\n>  \t * initialized the filter field with another --diff-filter, start\n>  \t * from full set of bits, except for AON.\n>  \t */\n>  \tif (!opt->filter) {\n> -\t\tfor (i = 0; (optch = optarg[i]) != '\\0'; i++) {\n> -\t\t\tif (optch < 'a' || 'z' < optch)\n> -\t\t\t\tcontinue;\n> +\t\toptch = optarg[0];\n> +\t\tif (optch >= 'a' && 'z' >= optch) {\n\nWe'll probably never have to deal with non-ASCII, so maybe this is being\noverzelous, but perhaps changing this to islower(optch) is worth it?\n\nThis relies on non-standard C both in the pre- and post-image, but in\nreality it works everywhere, until someone attempts to port git to an\nEBCDIC system...\n\n>  \t\t\topt->filter = (1 << (ARRAY_SIZE(diff_status_letters) - 1)) - 1;\n>  \t\t\topt->filter &= ~filter_bit[DIFF_STATUS_FILTER_AON];\n> -\t\t\tbreak;\n>  \t\t}\n>  \t}\n>  \n> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n> index 50495598619..28f727937dd 100755\n> --- a/t/t4202-log.sh\n> +++ b/t/t4202-log.sh\n> @@ -142,6 +142,14 @@ test_expect_success 'diff-filter=R' '\n>  \n>  '\n>  \n> +test_expect_success 'diff-filter=Ra' '\n> +\n\nnit: extra \\n\n\n> +\tgit log -M --pretty=\"format:%s\" --diff-filter=R HEAD >expect &&\n> +\tgit log -M --pretty=\"format:%s\" --diff-filter=Ra HEAD >actual &&\n> +\ttest_cmp expect actual\n> +\n> +'\n> +\n>  test_expect_success 'diff-filter=C' '\n>  \n>  \tgit log -C -C --pretty=\"format:%s\" --diff-filter=C HEAD >actual &&\n\n"},{"id":"447013","messageId":"YfHRTrdwZVwxcPBK@nand.local","threadId":"57309","inReplyTo":"xmqq1r0vt0y7.fsf@gitster.g","subject":"Re: [PATCH 2/2] diff-filter: be more careful when looking for negative bits","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-01-26T22:55:10Z","receivedAt":"2022-01-26T22:55:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jan 25, 2022 at 11:23:28PM -0800, Junio C Hamano wrote:\n>  diff.c         | 31 ++++++++++++++++++++++++++++---\n>  t/t4202-log.sh |  4 +++-\n>  2 files changed, 31 insertions(+), 4 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index fc1151b9c7..a57e458f63 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -4821,13 +4821,38 @@ static int diff_opt_diff_filter(const struct option *option,\n>  \tprepare_filter_bits();\n>\n>  \t/*\n> -\t * If the input starts with a negation e.g. 'd', and we haven't\n> +\t * If there is a negation e.g. 'd' in the input, and we haven't\n>  \t * initialized the filter field with another --diff-filter, start\n>  \t * from full set of bits, except for AON.\n> +\t * However, the user can try to limit to selected positive bits,\n> +\t * in which case we do not have to.\n> +\t *\n> +\t * NEEDSWORK: the \"we haven't initialied\" above is meant to\n> +\t * address cases where multiple options, e.g. --diff-filter=d\n> +\t * --diff-filter=a are given.  But this implementation is\n> +\t * insufficient when we refrain from starting from full set\n> +\t * when any positive bit is given.  Consider \"--diff-filter=D\n> +\t * --diff-filter=r\", which ought to behave the same way as\n> +\t * \"--diff-filter=Dr\" and \"--diff-filter=rD\".  The right fix\n> +\t * would probably involve two \"opt->filter[NP]\" fields,\n> +\t * records positive and negative bits separately in them while\n> +\t * parsing, and then after processing all options, compute\n> +\t * opt->filter by subtracting opt->filterN from opt->filterP\n> +\t * (and when we do so, fill opt->filterP to full bits if it is\n> +\t * absolutely empty).\n>  \t */\n>  \tif (!opt->filter) {\n> -\t\toptch = optarg[0];\n> -\t\tif (optch >= 'a' && 'z' >= optch) {\n> +\t\tint has_positive = 0;\n> +\t\tint has_negative = 0;\n> +\n> +\t\tfor (i = 0; (optch = optarg[i]) != '\\0'; i++) {\n> +\t\t\tif (optch < 'a' || 'z' < optch)\n> +\t\t\t\thas_positive++;\n> +\t\t\telse\n> +\t\t\t\thas_negative++;\n> +\t\t}\n> +\n> +\t\tif (!has_positive && has_negative) {\n>  \t\t\topt->filter = (1 << (ARRAY_SIZE(diff_status_letters) - 1)) - 1;\n>  \t\t\topt->filter &= ~filter_bit[DIFF_STATUS_FILTER_AON];\n>  \t\t}\n\nAhh. I feel much better about this implementation. Something was nagging\nme about treating optarg[0] specially, and you put very succinctly what\nit was that was bothering me.\n\n(One small nit that I absolutely do not care about is using a variable\nthat starts with 'has_'--which I would expect to be a boolean--to count\nthe number of positive/negative filters. Perhaps calling these\npositive_nr, and negative_nr, respectively, would be clearer.)\n\n> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n> index 28f727937d..128183e66f 100755\n> --- a/t/t4202-log.sh\n> +++ b/t/t4202-log.sh\n> @@ -142,10 +142,12 @@ test_expect_success 'diff-filter=R' '\n>\n>  '\n>\n> -test_expect_success 'diff-filter=Ra' '\n> +test_expect_success 'diff-filter=Ra and aR' '\n>\n>  \tgit log -M --pretty=\"format:%s\" --diff-filter=R HEAD >expect &&\n>  \tgit log -M --pretty=\"format:%s\" --diff-filter=Ra HEAD >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit log -M --pretty=\"format:%s\" --diff-filter=aR HEAD >actual &&\n>  \ttest_cmp expect actual\n\nPerfect.\n\nThanks,\nTaylor\n"},{"id":"447015","messageId":"xmqqtudqnk7y.fsf@gitster.g","threadId":"57309","inReplyTo":"YfHRTrdwZVwxcPBK@nand.local","subject":"Re: [PATCH 2/2] diff-filter: be more careful when looking for negative bits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-26T23:36:01Z","receivedAt":"2022-01-26T23:36:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n>> +\t\tint has_positive = 0;\n>> +\t\tint has_negative = 0;\n>> +\n>> +\t\tfor (i = 0; (optch = optarg[i]) != '\\0'; i++) {\n>> +\t\t\tif (optch < 'a' || 'z' < optch)\n>> +\t\t\t\thas_positive++;\n>> +\t\t\telse\n>> +\t\t\t\thas_negative++;\n>> +\t\t}\n>> +\n>> +\t\tif (!has_positive && has_negative) {\n>\n> (One small nit that I absolutely do not care about is using a variable\n> that starts with 'has_'--which I would expect to be a boolean--to count\n> the number of positive/negative filters. Perhaps calling these\n\nThese variables are indeed used in the boolean sense (see how they\nare used in the condition in the \"if\" statement).\n\nIf we wanted to short-circuit, we could do this:\n\n\tfor (i = 0;\n\t     !has_positive && !has_negative &&\n\t     (optch = optarg[i]) != '\\0';\n\t     i++) {\n\t\tif (isupper(optch))\n\t\t\thas_negative++;\n\t\telse\n\t\t\thas_positive++;\n\t}\n\nand thanks to their names, nobody would be confused to find it is a\nbug that these do not count when the loop exits early.  We shouldn't\nname these positive_nr or negative_nr because we are not interested\nin counting them.\n\nIt is just that \"bool++\" is more idiomatic than repeated assignment\n\"bool = 1\" when setter and getter both know it is merely used as a\nBoolean, and that is why they named as has_X, which clearly is a\nname for a Boolean, not a counter.\n\nHaving said that, I do not mind if an assignment is used instead of\npost-increment in new code.  I just think it is waste of time to go\nfind increments that toggle a Boolean to true and changing them to\nassignments, and it is even more waste having to review such a code\nchurn, so let's not go there ;-)\n\nBut as I wrote in the big NEEDSWORK comment, this loop should\ndisappear when the code is properly rewritten to correct the\ninteraction between two or more \"--diff-filter=\" options, so it\nwould not matter too much.\n\nThanks.\n"},{"id":"447117","messageId":"pull.1127.v2.git.1643310510.gitgitgadget@gmail.com","threadId":"57309","inReplyTo":"pull.1127.git.1643149759.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] Fix two --diff-filter bugs","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-27T19:08:26Z","receivedAt":"2022-01-27T19:08:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"A colleague noticed that git diff --diff-filter=Dr behaved in an unexpected\nway. The expectation was that the command shows only deleted files, but not\nrenamed ones.\n\nTurns out that Git's code is incorrect and turns on all diff-filter flags\nbecause the argument contains a lower-case letter. But since it starts with\nan upper-case letter, we should actually not turn all those flags on.\n\nWhile working on the fix, I realized that the documentation of the\n--diff-filter flag was not updated when intent-to-add files were no longer\nshown as modified by git diff, but as added.\n\nChanges since v1:\n\n * Now even the case of multiple --diff-filter options is handled.\n\nJohannes Schindelin (3):\n  docs(diff): lose incorrect claim about `diff-files --diff-filter=A`\n  diff.c: move the diff filter bits definitions up a bit\n  diff-filter: be more careful when looking for negative bits\n\n Documentation/diff-options.txt |  7 +--\n diff.c                         | 97 +++++++++++++++-------------------\n diff.h                         |  2 +-\n t/t4202-log.sh                 | 13 +++++\n 4 files changed, 60 insertions(+), 59 deletions(-)\n\n\nbase-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1127%2Fdscho%2Fdiff-filter-buglets-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1127/dscho/diff-filter-buglets-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1127\n\nRange-diff vs v1:\n\n 1:  704bb2ba18e = 1:  704bb2ba18e docs(diff): lose incorrect claim about `diff-files --diff-filter=A`\n -:  ----------- > 2:  19c7223e265 diff.c: move the diff filter bits definitions up a bit\n 2:  e8006493a9e ! 3:  b041d2b7a3b diff-filter: be more careful when looking for negative bits\n     @@ Commit message\n          provided a lower-case letter: if the `--diff-filter` argument starts\n          with an upper-case letter, we must not start with all bits turned on.\n      \n     +    Even worse, it is possible to specify the diff filters in multiple,\n     +    separate options, e.g. `--diff-filter=AM [...] --diff-filter=m`.\n     +\n     +    Let's accumulate the include/exclude filters independently, and only\n     +    special-case the \"only exclude filters were specified\" case after\n     +    parsing the options altogether.\n     +\n          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n      \n       ## diff.c ##\n     +@@ diff.c: void diff_setup_done(struct diff_options *options)\n     + \tif (!options->use_color || external_diff())\n     + \t\toptions->color_moved = 0;\n     + \n     ++\tif (options->filter_not) {\n     ++\t\tif (!options->filter)\n     ++\t\t\toptions->filter = ~filter_bit[DIFF_STATUS_FILTER_AON];\n     ++\t\toptions->filter &= ~options->filter_not;\n     ++\t}\n     ++\n     + \tFREE_AND_NULL(options->parseopts);\n     + }\n     + \n      @@ diff.c: static int diff_opt_diff_filter(const struct option *option,\n     + \tBUG_ON_OPT_NEG(unset);\n       \tprepare_filter_bits();\n       \n     - \t/*\n     +-\t/*\n      -\t * If there is a negation e.g. 'd' in the input, and we haven't\n     -+\t * If the input starts with a negation e.g. 'd', and we haven't\n     - \t * initialized the filter field with another --diff-filter, start\n     - \t * from full set of bits, except for AON.\n     - \t */\n     - \tif (!opt->filter) {\n     +-\t * initialized the filter field with another --diff-filter, start\n     +-\t * from full set of bits, except for AON.\n     +-\t */\n     +-\tif (!opt->filter) {\n      -\t\tfor (i = 0; (optch = optarg[i]) != '\\0'; i++) {\n      -\t\t\tif (optch < 'a' || 'z' < optch)\n      -\t\t\t\tcontinue;\n     -+\t\toptch = optarg[0];\n     -+\t\tif (optch >= 'a' && 'z' >= optch) {\n     - \t\t\topt->filter = (1 << (ARRAY_SIZE(diff_status_letters) - 1)) - 1;\n     - \t\t\topt->filter &= ~filter_bit[DIFF_STATUS_FILTER_AON];\n     +-\t\t\topt->filter = (1 << (ARRAY_SIZE(diff_status_letters) - 1)) - 1;\n     +-\t\t\topt->filter &= ~filter_bit[DIFF_STATUS_FILTER_AON];\n      -\t\t\tbreak;\n     - \t\t}\n     +-\t\t}\n     +-\t}\n     +-\n     + \tfor (i = 0; (optch = optarg[i]) != '\\0'; i++) {\n     + \t\tunsigned int bit;\n     + \t\tint negate;\n     +@@ diff.c: static int diff_opt_diff_filter(const struct option *option,\n     + \t\t\treturn error(_(\"unknown change class '%c' in --diff-filter=%s\"),\n     + \t\t\t\t     optarg[i], optarg);\n     + \t\tif (negate)\n     +-\t\t\topt->filter &= ~bit;\n     ++\t\t\topt->filter_not |= bit;\n     + \t\telse\n     + \t\t\topt->filter |= bit;\n       \t}\n     +\n     + ## diff.h ##\n     +@@ diff.h: struct diff_options {\n     + \tstruct diff_flags flags;\n     + \n     + \t/* diff-filter bits */\n     +-\tunsigned int filter;\n     ++\tunsigned int filter, filter_not;\n     + \n     + \tint use_color;\n       \n      \n       ## t/t4202-log.sh ##\n     @@ t/t4202-log.sh: test_expect_success 'diff-filter=R' '\n       \n       '\n       \n     -+test_expect_success 'diff-filter=Ra' '\n     ++test_expect_success 'multiple --diff-filter bits' '\n      +\n      +\tgit log -M --pretty=\"format:%s\" --diff-filter=R HEAD >expect &&\n      +\tgit log -M --pretty=\"format:%s\" --diff-filter=Ra HEAD >actual &&\n     ++\ttest_cmp expect actual &&\n     ++\tgit log -M --pretty=\"format:%s\" --diff-filter=aR HEAD >actual &&\n     ++\ttest_cmp expect actual &&\n     ++\tgit log -M --pretty=\"format:%s\" \\\n     ++\t\t--diff-filter=a --diff-filter=R HEAD >actual &&\n      +\ttest_cmp expect actual\n      +\n      +'\n\n-- \ngitgitgadget\n"},{"id":"447118","messageId":"704bb2ba18ee3af5857064f6fd61689f98da0480.1643310510.git.gitgitgadget@gmail.com","threadId":"57309","inReplyTo":"pull.1127.v2.git.1643310510.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] docs(diff): lose incorrect claim about `diff-files --diff-filter=A`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-27T19:08:27Z","receivedAt":"2022-01-27T19:08:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nOriginally, before we had `--intent-to-add`, there was no way that `git\ndiff-files` could see added files: if a file did not exist in the index,\n`git diff-files` would not show it because it looks only at worktree\nfiles when there is an index entry at the same path.\n\nWe used this example in the documentation of the diff options to explain\nthat not every `--diff-filter=<option>` has an effect in all scenarios.\n\nEven when we added `--intent-to-add`, the comment was still correct,\nbecause initially we showed such files as modified instead of added.\n\nHowever, when that bug was fixed in feea6946a5b (diff-files: treat\n\"i-t-a\" files as \"not-in-index\", 2020-06-20), the comment in the\ndocumentation became incorrect.\n\nLet's just remove it.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n Documentation/diff-options.txt | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex c89d530d3d1..2549df0d212 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -616,11 +616,8 @@ ifndef::git-format-patch[]\n Also, these upper-case letters can be downcased to exclude.  E.g.\n `--diff-filter=ad` excludes added and deleted paths.\n +\n-Note that not all diffs can feature all types. For instance, diffs\n-from the index to the working tree can never have Added entries\n-(because the set of paths included in the diff is limited by what is in\n-the index).  Similarly, copied and renamed entries cannot appear if\n-detection for those types is disabled.\n+Note that not all diffs can feature all types. For instance, copied and\n+renamed entries cannot appear if detection for those types is disabled.\n \n -S<string>::\n \tLook for differences that change the number of occurrences of\n-- \ngitgitgadget\n\n"},{"id":"447119","messageId":"19c7223e265962eac2f56d82d87c2fd069f04c6e.1643310510.git.gitgitgadget@gmail.com","threadId":"57309","inReplyTo":"pull.1127.v2.git.1643310510.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] diff.c: move the diff filter bits definitions up a bit","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-27T19:08:28Z","receivedAt":"2022-01-27T19:08:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nThis prepares for a more careful handling of the `--diff-filter`\noptions over the next few commits.\n\nThis commit is best viewed with `--color-moved`.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n diff.c | 74 +++++++++++++++++++++++++++++-----------------------------\n 1 file changed, 37 insertions(+), 37 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex c862771a589..5081052c431 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4570,6 +4570,43 @@ void repo_diff_setup(struct repository *r, struct diff_options *options)\n \tprep_parse_options(options);\n }\n \n+static const char diff_status_letters[] = {\n+\tDIFF_STATUS_ADDED,\n+\tDIFF_STATUS_COPIED,\n+\tDIFF_STATUS_DELETED,\n+\tDIFF_STATUS_MODIFIED,\n+\tDIFF_STATUS_RENAMED,\n+\tDIFF_STATUS_TYPE_CHANGED,\n+\tDIFF_STATUS_UNKNOWN,\n+\tDIFF_STATUS_UNMERGED,\n+\tDIFF_STATUS_FILTER_AON,\n+\tDIFF_STATUS_FILTER_BROKEN,\n+\t'\\0',\n+};\n+\n+static unsigned int filter_bit['Z' + 1];\n+\n+static void prepare_filter_bits(void)\n+{\n+\tint i;\n+\n+\tif (!filter_bit[DIFF_STATUS_ADDED]) {\n+\t\tfor (i = 0; diff_status_letters[i]; i++)\n+\t\t\tfilter_bit[(int) diff_status_letters[i]] = (1 << i);\n+\t}\n+}\n+\n+static unsigned filter_bit_tst(char status, const struct diff_options *opt)\n+{\n+\treturn opt->filter & filter_bit[(int) status];\n+}\n+\n+unsigned diff_filter_bit(char status)\n+{\n+\tprepare_filter_bits();\n+\treturn filter_bit[(int) status];\n+}\n+\n void diff_setup_done(struct diff_options *options)\n {\n \tunsigned check_mask = DIFF_FORMAT_NAME |\n@@ -4774,43 +4811,6 @@ static int parse_dirstat_opt(struct diff_options *options, const char *params)\n \treturn 1;\n }\n \n-static const char diff_status_letters[] = {\n-\tDIFF_STATUS_ADDED,\n-\tDIFF_STATUS_COPIED,\n-\tDIFF_STATUS_DELETED,\n-\tDIFF_STATUS_MODIFIED,\n-\tDIFF_STATUS_RENAMED,\n-\tDIFF_STATUS_TYPE_CHANGED,\n-\tDIFF_STATUS_UNKNOWN,\n-\tDIFF_STATUS_UNMERGED,\n-\tDIFF_STATUS_FILTER_AON,\n-\tDIFF_STATUS_FILTER_BROKEN,\n-\t'\\0',\n-};\n-\n-static unsigned int filter_bit['Z' + 1];\n-\n-static void prepare_filter_bits(void)\n-{\n-\tint i;\n-\n-\tif (!filter_bit[DIFF_STATUS_ADDED]) {\n-\t\tfor (i = 0; diff_status_letters[i]; i++)\n-\t\t\tfilter_bit[(int) diff_status_letters[i]] = (1 << i);\n-\t}\n-}\n-\n-static unsigned filter_bit_tst(char status, const struct diff_options *opt)\n-{\n-\treturn opt->filter & filter_bit[(int) status];\n-}\n-\n-unsigned diff_filter_bit(char status)\n-{\n-\tprepare_filter_bits();\n-\treturn filter_bit[(int) status];\n-}\n-\n static int diff_opt_diff_filter(const struct option *option,\n \t\t\t\tconst char *optarg, int unset)\n {\n-- \ngitgitgadget\n\n"},{"id":"447120","messageId":"b041d2b7a3bd4433e86438cddbd52857e5f375a6.1643310510.git.gitgitgadget@gmail.com","threadId":"57309","inReplyTo":"pull.1127.v2.git.1643310510.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] diff-filter: be more careful when looking for negative bits","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-27T19:08:29Z","receivedAt":"2022-01-27T19:08:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nThe `--diff-filter=<bits>` option allows to filter the diff by certain\ncriteria, for example `R` to only show renamed files. It also supports\nnegating a filter via a down-cased letter, i.e. `r` to show _everything\nbut_ renamed files.\n\nHowever, the code is a bit overzealous when trying to figure out whether\n`git diff` should start with all diff-filters turned on because the user\nprovided a lower-case letter: if the `--diff-filter` argument starts\nwith an upper-case letter, we must not start with all bits turned on.\n\nEven worse, it is possible to specify the diff filters in multiple,\nseparate options, e.g. `--diff-filter=AM [...] --diff-filter=m`.\n\nLet's accumulate the include/exclude filters independently, and only\nspecial-case the \"only exclude filters were specified\" case after\nparsing the options altogether.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n diff.c         | 23 +++++++----------------\n diff.h         |  2 +-\n t/t4202-log.sh | 13 +++++++++++++\n 3 files changed, 21 insertions(+), 17 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 5081052c431..4ab4299b817 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4720,6 +4720,12 @@ void diff_setup_done(struct diff_options *options)\n \tif (!options->use_color || external_diff())\n \t\toptions->color_moved = 0;\n \n+\tif (options->filter_not) {\n+\t\tif (!options->filter)\n+\t\t\toptions->filter = ~filter_bit[DIFF_STATUS_FILTER_AON];\n+\t\toptions->filter &= ~options->filter_not;\n+\t}\n+\n \tFREE_AND_NULL(options->parseopts);\n }\n \n@@ -4820,21 +4826,6 @@ static int diff_opt_diff_filter(const struct option *option,\n \tBUG_ON_OPT_NEG(unset);\n \tprepare_filter_bits();\n \n-\t/*\n-\t * If there is a negation e.g. 'd' in the input, and we haven't\n-\t * initialized the filter field with another --diff-filter, start\n-\t * from full set of bits, except for AON.\n-\t */\n-\tif (!opt->filter) {\n-\t\tfor (i = 0; (optch = optarg[i]) != '\\0'; i++) {\n-\t\t\tif (optch < 'a' || 'z' < optch)\n-\t\t\t\tcontinue;\n-\t\t\topt->filter = (1 << (ARRAY_SIZE(diff_status_letters) - 1)) - 1;\n-\t\t\topt->filter &= ~filter_bit[DIFF_STATUS_FILTER_AON];\n-\t\t\tbreak;\n-\t\t}\n-\t}\n-\n \tfor (i = 0; (optch = optarg[i]) != '\\0'; i++) {\n \t\tunsigned int bit;\n \t\tint negate;\n@@ -4851,7 +4842,7 @@ static int diff_opt_diff_filter(const struct option *option,\n \t\t\treturn error(_(\"unknown change class '%c' in --diff-filter=%s\"),\n \t\t\t\t     optarg[i], optarg);\n \t\tif (negate)\n-\t\t\topt->filter &= ~bit;\n+\t\t\topt->filter_not |= bit;\n \t\telse\n \t\t\topt->filter |= bit;\n \t}\ndiff --git a/diff.h b/diff.h\nindex 8ba85c5e605..a70e7c478c1 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -283,7 +283,7 @@ struct diff_options {\n \tstruct diff_flags flags;\n \n \t/* diff-filter bits */\n-\tunsigned int filter;\n+\tunsigned int filter, filter_not;\n \n \tint use_color;\n \ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 50495598619..b25182379ff 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -142,6 +142,19 @@ test_expect_success 'diff-filter=R' '\n \n '\n \n+test_expect_success 'multiple --diff-filter bits' '\n+\n+\tgit log -M --pretty=\"format:%s\" --diff-filter=R HEAD >expect &&\n+\tgit log -M --pretty=\"format:%s\" --diff-filter=Ra HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\tgit log -M --pretty=\"format:%s\" --diff-filter=aR HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\tgit log -M --pretty=\"format:%s\" \\\n+\t\t--diff-filter=a --diff-filter=R HEAD >actual &&\n+\ttest_cmp expect actual\n+\n+'\n+\n test_expect_success 'diff-filter=C' '\n \n \tgit log -C -C --pretty=\"format:%s\" --diff-filter=C HEAD >actual &&\n-- \ngitgitgadget\n"},{"id":"447161","messageId":"xmqq35l8zmkh.fsf@gitster.g","threadId":"57309","inReplyTo":"b041d2b7a3bd4433e86438cddbd52857e5f375a6.1643310510.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/3] diff-filter: be more careful when looking for negative bits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-28T01:16:46Z","receivedAt":"2022-01-28T01:16:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\nwrites:\n\n> diff --git a/diff.c b/diff.c\n> index 5081052c431..4ab4299b817 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -4720,6 +4720,12 @@ void diff_setup_done(struct diff_options *options)\n>  \tif (!options->use_color || external_diff())\n>  \t\toptions->color_moved = 0;\n>  \n> +\tif (options->filter_not) {\n> +\t\tif (!options->filter)\n> +\t\t\toptions->filter = ~filter_bit[DIFF_STATUS_FILTER_AON];\n\nUnlike the original, options->filter will have excess higher bit all\non, in addition to all the filter bits except for the all-or-none\nbit.  I do not know offhand if that makes the difference, but I\ntrust that you have audited all uses of the options->filter flag\nword and these high bits are truly unused and the difference does\nnot matter.\n\n> +\t\toptions->filter &= ~options->filter_not;\n> +\t}\n\n>  \tfor (i = 0; (optch = optarg[i]) != '\\0'; i++) {\n>  \t\tunsigned int bit;\n>  \t\tint negate;\n> @@ -4851,7 +4842,7 @@ static int diff_opt_diff_filter(const struct option *option,\n>  \t\t\treturn error(_(\"unknown change class '%c' in --diff-filter=%s\"),\n>  \t\t\t\t     optarg[i], optarg);\n>  \t\tif (negate)\n> -\t\t\topt->filter &= ~bit;\n> +\t\t\topt->filter_not |= bit;\n>  \t\telse\n>  \t\t\topt->filter |= bit;\n>  \t}\n\nAnd this ... \n\n> diff --git a/diff.h b/diff.h\n> index 8ba85c5e605..a70e7c478c1 100644\n> --- a/diff.h\n> +++ b/diff.h\n> @@ -283,7 +283,7 @@ struct diff_options {\n>  \tstruct diff_flags flags;\n>  \n>  \t/* diff-filter bits */\n> -\tunsigned int filter;\n> +\tunsigned int filter, filter_not;\n\n... is exactly I wrote in the NEEDSWORK comment I gave you in my\nearlier review.  Excellent.\n\n> +test_expect_success 'multiple --diff-filter bits' '\n> +\n> +\tgit log -M --pretty=\"format:%s\" --diff-filter=R HEAD >expect &&\n> +\tgit log -M --pretty=\"format:%s\" --diff-filter=Ra HEAD >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit log -M --pretty=\"format:%s\" --diff-filter=aR HEAD >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit log -M --pretty=\"format:%s\" \\\n> +\t\t--diff-filter=a --diff-filter=R HEAD >actual &&\n> +\ttest_cmp expect actual\n> +\n> +'\n\nGood.\n\nThanks for noticing and fixing the long-standing issue.\n"},{"id":"447193","messageId":"nycvar.QRO.7.76.6.2201281249310.347@tvgsbejvaqbjf.bet","threadId":"57309","inReplyTo":"YfCGDY5qXPCFXpfp@nand.local","subject":"Re: [PATCH 2/2] diff-filter: be more careful when looking for negative bits","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-01-28T11:52:45Z","receivedAt":"2022-01-28T11:52:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Taylor,\n\nOn Tue, 25 Jan 2022, Taylor Blau wrote:\n\n> On Tue, Jan 25, 2022 at 10:29:19PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> > diff --git a/diff.c b/diff.c\n> > index c862771a589..fc1151b9c73 100644\n> > --- a/diff.c\n> > +++ b/diff.c\n> > @@ -4821,17 +4821,15 @@ static int diff_opt_diff_filter(const struct option *option,\n> >  \tprepare_filter_bits();\n> >\n> >  \t/*\n> > -\t * If there is a negation e.g. 'd' in the input, and we haven't\n> > +\t * If the input starts with a negation e.g. 'd', and we haven't\n> >  \t * initialized the filter field with another --diff-filter, start\n> >  \t * from full set of bits, except for AON.\n> >  \t */\n> >  \tif (!opt->filter) {\n> > -\t\tfor (i = 0; (optch = optarg[i]) != '\\0'; i++) {\n> > -\t\t\tif (optch < 'a' || 'z' < optch)\n> > -\t\t\t\tcontinue;\n> > +\t\toptch = optarg[0];\n> > +\t\tif (optch >= 'a' && 'z' >= optch) {\n> >  \t\t\topt->filter = (1 << (ARRAY_SIZE(diff_status_letters) - 1)) - 1;\n> >  \t\t\topt->filter &= ~filter_bit[DIFF_STATUS_FILTER_AON];\n> > -\t\t\tbreak;\n> >  \t\t}\n> >  \t}\n>\n> Thinking through how this would have worked before with\n> `--diff-filter=Dr`, I think it goes something like:\n>\n>   1. We set all bits (except the all-or-none bit) on via the first loop.\n>   2. Then we OR in the bit for deletions, which does not change the\n>      overall filter (since it was already set by the previous step).\n>   3. Then we unset the bit corresponding to renames.\n>\n> That leaves us with all bits on except two: DIFF_STATUS_RENAMED and\n> DIFF_STATUS_FILTER_AON.\n\nCorrect. And since we asked only for \"Deleted\", we get way more than we\nbargained for.\n\n> As far as I can understand, the AON \"filter\" shows all files as long as\n> at least one of them matches the filter, otherwise it shows nothing at\n> all.\n\nRight, so on its own, it is quite useless. It needs to be combined with\nanother diff filter to make sense.\n\n> But that doesn't save us, since we have many more bits on than we should\n> have, meaning that `--diff-filter=Dr` doesn't work at all (assuming you\n> expected it to show just deletions, like `--diff-filter=D` does).\n\nCorrect.\n\nCiao,\nDscho\n"},{"id":"447194","messageId":"nycvar.QRO.7.76.6.2201281253020.347@tvgsbejvaqbjf.bet","threadId":"57309","inReplyTo":"xmqqtudqnk7y.fsf@gitster.g","subject":"Re: [PATCH 2/2] diff-filter: be more careful when looking for negative bits","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-01-28T11:54:50Z","receivedAt":"2022-01-28T11:55:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 26 Jan 2022, Junio C Hamano wrote:\n\n> [...] this loop should disappear when the code is properly rewritten to\n> correct the interaction between two or more \"--diff-filter=\" options, so\n> it would not matter too much.\n\nAnd so it did. Thank you for the suggestion. At first I thought that it\nwould require too extensive a code change, but was pleasantly surprised\nwhen I figured out that the final step can be done as part of\n`diff_setup_done()`, with a quite pleasant-to-read patch, if I may say so.\n\nCiao,\nDscho\n"},{"id":"447195","messageId":"nycvar.QRO.7.76.6.2201281255110.347@tvgsbejvaqbjf.bet","threadId":"57309","inReplyTo":"xmqq35l8zmkh.fsf@gitster.g","subject":"Re: [PATCH v2 3/3] diff-filter: be more careful when looking for negative bits","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-01-28T12:01:47Z","receivedAt":"2022-01-28T12:01:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 27 Jan 2022, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\n> writes:\n>\n> > diff --git a/diff.c b/diff.c\n> > index 5081052c431..4ab4299b817 100644\n> > --- a/diff.c\n> > +++ b/diff.c\n> > @@ -4720,6 +4720,12 @@ void diff_setup_done(struct diff_options *options)\n> >  \tif (!options->use_color || external_diff())\n> >  \t\toptions->color_moved = 0;\n> >\n> > +\tif (options->filter_not) {\n> > +\t\tif (!options->filter)\n> > +\t\t\toptions->filter = ~filter_bit[DIFF_STATUS_FILTER_AON];\n>\n> Unlike the original, options->filter will have excess higher bit all\n> on, in addition to all the filter bits except for the all-or-none\n> bit.\n\nThank you for your thoroughness. Indeed, you are correct that I no longer\ndo the `(1 << (ARRAY_SIZE(diff_status_letters) - 1)) - 1` dance.\n\nIn an uncommitted iteration, I actuall forced that mask in\n`diff_setup_done()` via:\n\n\toptions->filter &= (1 << (ARRAY_SIZE(diff_status_letters) - 1)) - 1;\n\nBut then I got curious and looked how `options->filter` is accessed, and\nwe never loop over its bits, we always ask whether a specific bit is set.\nSo I got rid of that (quite ugly) line.\n\nI made a mental note to write about this in the commit message, but wanted\nto quickly look at all the code accessing `options->filter` by using the\nvery nice refactoring support of VS Code to rename the field so that all\nof the accesses would be visible in a diff, and then I wanted to quickly\nrun the entire test suite first, just in case my analysis missed\nsomething. And by the end of it, my mental note had evaporated.\n\nThanks to your reminder, I added this to the end of the commit message:\n\n    Note: The code replaced by this commit took pains to avoid setting any\n    unused bits of `options->filter`. That was unnecessary, though, as all\n    accesses happen via the `filter_bit_tst()` function using specific bits,\n    and setting the unused bits has no effect. Therefore, we can simplify\n    the code by using `~0` (or in this instance, `~<unwanted-bit>`).\n\nCiao,\nDscho\n"},{"id":"447196","messageId":"pull.1127.v3.git.1643371370.gitgitgadget@gmail.com","threadId":"57309","inReplyTo":"pull.1127.v2.git.1643310510.gitgitgadget@gmail.com","subject":"[PATCH v3 0/3] Fix two --diff-filter bugs","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-28T12:02:47Z","receivedAt":"2022-01-28T12:02:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"A colleague noticed that git diff --diff-filter=Dr behaved in an unexpected\nway. The expectation was that the command shows only deleted files, but not\nrenamed ones.\n\nTurns out that Git's code is incorrect and turns on all diff-filter flags\nbecause the argument contains a lower-case letter. But since it starts with\nan upper-case letter, we should actually not turn all those flags on.\n\nWhile working on the fix, I realized that the documentation of the\n--diff-filter flag was not updated when intent-to-add files were no longer\nshown as modified by git diff, but as added.\n\nChanges since v2:\n\n * Augmented the commit message to explain why we do not need to be careful\n   about setting only the used bits of options->filter.\n\nChanges since v1:\n\n * Now even the case of multiple --diff-filter options is handled.\n\nJohannes Schindelin (3):\n  docs(diff): lose incorrect claim about `diff-files --diff-filter=A`\n  diff.c: move the diff filter bits definitions up a bit\n  diff-filter: be more careful when looking for negative bits\n\n Documentation/diff-options.txt |  7 +--\n diff.c                         | 97 +++++++++++++++-------------------\n diff.h                         |  2 +-\n t/t4202-log.sh                 | 13 +++++\n 4 files changed, 60 insertions(+), 59 deletions(-)\n\n\nbase-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1127%2Fdscho%2Fdiff-filter-buglets-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1127/dscho/diff-filter-buglets-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1127\n\nRange-diff vs v2:\n\n 1:  704bb2ba18e = 1:  704bb2ba18e docs(diff): lose incorrect claim about `diff-files --diff-filter=A`\n 2:  19c7223e265 = 2:  19c7223e265 diff.c: move the diff filter bits definitions up a bit\n 3:  b041d2b7a3b ! 3:  f1f027ad61b diff-filter: be more careful when looking for negative bits\n     @@ Commit message\n          special-case the \"only exclude filters were specified\" case after\n          parsing the options altogether.\n      \n     +    Note: The code replaced by this commit took pains to avoid setting any\n     +    unused bits of `options->filter`. That was unnecessary, though, as all\n     +    accesses happen via the `filter_bit_tst()` function using specific bits,\n     +    and setting the unused bits has no effect. Therefore, we can simplify\n     +    the code by using `~0` (or in this instance, `~<unwanted-bit>`).\n     +\n          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n      \n       ## diff.c ##\n\n-- \ngitgitgadget\n"},{"id":"447197","messageId":"704bb2ba18ee3af5857064f6fd61689f98da0480.1643371370.git.gitgitgadget@gmail.com","threadId":"57309","inReplyTo":"pull.1127.v3.git.1643371370.gitgitgadget@gmail.com","subject":"[PATCH v3 1/3] docs(diff): lose incorrect claim about `diff-files --diff-filter=A`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-28T12:02:48Z","receivedAt":"2022-01-28T12:02:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nOriginally, before we had `--intent-to-add`, there was no way that `git\ndiff-files` could see added files: if a file did not exist in the index,\n`git diff-files` would not show it because it looks only at worktree\nfiles when there is an index entry at the same path.\n\nWe used this example in the documentation of the diff options to explain\nthat not every `--diff-filter=<option>` has an effect in all scenarios.\n\nEven when we added `--intent-to-add`, the comment was still correct,\nbecause initially we showed such files as modified instead of added.\n\nHowever, when that bug was fixed in feea6946a5b (diff-files: treat\n\"i-t-a\" files as \"not-in-index\", 2020-06-20), the comment in the\ndocumentation became incorrect.\n\nLet's just remove it.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n Documentation/diff-options.txt | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex c89d530d3d1..2549df0d212 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -616,11 +616,8 @@ ifndef::git-format-patch[]\n Also, these upper-case letters can be downcased to exclude.  E.g.\n `--diff-filter=ad` excludes added and deleted paths.\n +\n-Note that not all diffs can feature all types. For instance, diffs\n-from the index to the working tree can never have Added entries\n-(because the set of paths included in the diff is limited by what is in\n-the index).  Similarly, copied and renamed entries cannot appear if\n-detection for those types is disabled.\n+Note that not all diffs can feature all types. For instance, copied and\n+renamed entries cannot appear if detection for those types is disabled.\n \n -S<string>::\n \tLook for differences that change the number of occurrences of\n-- \ngitgitgadget\n\n"},{"id":"447198","messageId":"19c7223e265962eac2f56d82d87c2fd069f04c6e.1643371370.git.gitgitgadget@gmail.com","threadId":"57309","inReplyTo":"pull.1127.v3.git.1643371370.gitgitgadget@gmail.com","subject":"[PATCH v3 2/3] diff.c: move the diff filter bits definitions up a bit","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-28T12:02:49Z","receivedAt":"2022-01-28T12:02:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nThis prepares for a more careful handling of the `--diff-filter`\noptions over the next few commits.\n\nThis commit is best viewed with `--color-moved`.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n diff.c | 74 +++++++++++++++++++++++++++++-----------------------------\n 1 file changed, 37 insertions(+), 37 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex c862771a589..5081052c431 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4570,6 +4570,43 @@ void repo_diff_setup(struct repository *r, struct diff_options *options)\n \tprep_parse_options(options);\n }\n \n+static const char diff_status_letters[] = {\n+\tDIFF_STATUS_ADDED,\n+\tDIFF_STATUS_COPIED,\n+\tDIFF_STATUS_DELETED,\n+\tDIFF_STATUS_MODIFIED,\n+\tDIFF_STATUS_RENAMED,\n+\tDIFF_STATUS_TYPE_CHANGED,\n+\tDIFF_STATUS_UNKNOWN,\n+\tDIFF_STATUS_UNMERGED,\n+\tDIFF_STATUS_FILTER_AON,\n+\tDIFF_STATUS_FILTER_BROKEN,\n+\t'\\0',\n+};\n+\n+static unsigned int filter_bit['Z' + 1];\n+\n+static void prepare_filter_bits(void)\n+{\n+\tint i;\n+\n+\tif (!filter_bit[DIFF_STATUS_ADDED]) {\n+\t\tfor (i = 0; diff_status_letters[i]; i++)\n+\t\t\tfilter_bit[(int) diff_status_letters[i]] = (1 << i);\n+\t}\n+}\n+\n+static unsigned filter_bit_tst(char status, const struct diff_options *opt)\n+{\n+\treturn opt->filter & filter_bit[(int) status];\n+}\n+\n+unsigned diff_filter_bit(char status)\n+{\n+\tprepare_filter_bits();\n+\treturn filter_bit[(int) status];\n+}\n+\n void diff_setup_done(struct diff_options *options)\n {\n \tunsigned check_mask = DIFF_FORMAT_NAME |\n@@ -4774,43 +4811,6 @@ static int parse_dirstat_opt(struct diff_options *options, const char *params)\n \treturn 1;\n }\n \n-static const char diff_status_letters[] = {\n-\tDIFF_STATUS_ADDED,\n-\tDIFF_STATUS_COPIED,\n-\tDIFF_STATUS_DELETED,\n-\tDIFF_STATUS_MODIFIED,\n-\tDIFF_STATUS_RENAMED,\n-\tDIFF_STATUS_TYPE_CHANGED,\n-\tDIFF_STATUS_UNKNOWN,\n-\tDIFF_STATUS_UNMERGED,\n-\tDIFF_STATUS_FILTER_AON,\n-\tDIFF_STATUS_FILTER_BROKEN,\n-\t'\\0',\n-};\n-\n-static unsigned int filter_bit['Z' + 1];\n-\n-static void prepare_filter_bits(void)\n-{\n-\tint i;\n-\n-\tif (!filter_bit[DIFF_STATUS_ADDED]) {\n-\t\tfor (i = 0; diff_status_letters[i]; i++)\n-\t\t\tfilter_bit[(int) diff_status_letters[i]] = (1 << i);\n-\t}\n-}\n-\n-static unsigned filter_bit_tst(char status, const struct diff_options *opt)\n-{\n-\treturn opt->filter & filter_bit[(int) status];\n-}\n-\n-unsigned diff_filter_bit(char status)\n-{\n-\tprepare_filter_bits();\n-\treturn filter_bit[(int) status];\n-}\n-\n static int diff_opt_diff_filter(const struct option *option,\n \t\t\t\tconst char *optarg, int unset)\n {\n-- \ngitgitgadget\n\n"},{"id":"447199","messageId":"f1f027ad61beb1bd0dee73acbffdee5c0f967e9a.1643371370.git.gitgitgadget@gmail.com","threadId":"57309","inReplyTo":"pull.1127.v3.git.1643371370.gitgitgadget@gmail.com","subject":"[PATCH v3 3/3] diff-filter: be more careful when looking for negative bits","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-28T12:02:50Z","receivedAt":"2022-01-28T12:02:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nThe `--diff-filter=<bits>` option allows to filter the diff by certain\ncriteria, for example `R` to only show renamed files. It also supports\nnegating a filter via a down-cased letter, i.e. `r` to show _everything\nbut_ renamed files.\n\nHowever, the code is a bit overzealous when trying to figure out whether\n`git diff` should start with all diff-filters turned on because the user\nprovided a lower-case letter: if the `--diff-filter` argument starts\nwith an upper-case letter, we must not start with all bits turned on.\n\nEven worse, it is possible to specify the diff filters in multiple,\nseparate options, e.g. `--diff-filter=AM [...] --diff-filter=m`.\n\nLet's accumulate the include/exclude filters independently, and only\nspecial-case the \"only exclude filters were specified\" case after\nparsing the options altogether.\n\nNote: The code replaced by this commit took pains to avoid setting any\nunused bits of `options->filter`. That was unnecessary, though, as all\naccesses happen via the `filter_bit_tst()` function using specific bits,\nand setting the unused bits has no effect. Therefore, we can simplify\nthe code by using `~0` (or in this instance, `~<unwanted-bit>`).\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n diff.c         | 23 +++++++----------------\n diff.h         |  2 +-\n t/t4202-log.sh | 13 +++++++++++++\n 3 files changed, 21 insertions(+), 17 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 5081052c431..4ab4299b817 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4720,6 +4720,12 @@ void diff_setup_done(struct diff_options *options)\n \tif (!options->use_color || external_diff())\n \t\toptions->color_moved = 0;\n \n+\tif (options->filter_not) {\n+\t\tif (!options->filter)\n+\t\t\toptions->filter = ~filter_bit[DIFF_STATUS_FILTER_AON];\n+\t\toptions->filter &= ~options->filter_not;\n+\t}\n+\n \tFREE_AND_NULL(options->parseopts);\n }\n \n@@ -4820,21 +4826,6 @@ static int diff_opt_diff_filter(const struct option *option,\n \tBUG_ON_OPT_NEG(unset);\n \tprepare_filter_bits();\n \n-\t/*\n-\t * If there is a negation e.g. 'd' in the input, and we haven't\n-\t * initialized the filter field with another --diff-filter, start\n-\t * from full set of bits, except for AON.\n-\t */\n-\tif (!opt->filter) {\n-\t\tfor (i = 0; (optch = optarg[i]) != '\\0'; i++) {\n-\t\t\tif (optch < 'a' || 'z' < optch)\n-\t\t\t\tcontinue;\n-\t\t\topt->filter = (1 << (ARRAY_SIZE(diff_status_letters) - 1)) - 1;\n-\t\t\topt->filter &= ~filter_bit[DIFF_STATUS_FILTER_AON];\n-\t\t\tbreak;\n-\t\t}\n-\t}\n-\n \tfor (i = 0; (optch = optarg[i]) != '\\0'; i++) {\n \t\tunsigned int bit;\n \t\tint negate;\n@@ -4851,7 +4842,7 @@ static int diff_opt_diff_filter(const struct option *option,\n \t\t\treturn error(_(\"unknown change class '%c' in --diff-filter=%s\"),\n \t\t\t\t     optarg[i], optarg);\n \t\tif (negate)\n-\t\t\topt->filter &= ~bit;\n+\t\t\topt->filter_not |= bit;\n \t\telse\n \t\t\topt->filter |= bit;\n \t}\ndiff --git a/diff.h b/diff.h\nindex 8ba85c5e605..a70e7c478c1 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -283,7 +283,7 @@ struct diff_options {\n \tstruct diff_flags flags;\n \n \t/* diff-filter bits */\n-\tunsigned int filter;\n+\tunsigned int filter, filter_not;\n \n \tint use_color;\n \ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 50495598619..b25182379ff 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -142,6 +142,19 @@ test_expect_success 'diff-filter=R' '\n \n '\n \n+test_expect_success 'multiple --diff-filter bits' '\n+\n+\tgit log -M --pretty=\"format:%s\" --diff-filter=R HEAD >expect &&\n+\tgit log -M --pretty=\"format:%s\" --diff-filter=Ra HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\tgit log -M --pretty=\"format:%s\" --diff-filter=aR HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\tgit log -M --pretty=\"format:%s\" \\\n+\t\t--diff-filter=a --diff-filter=R HEAD >actual &&\n+\ttest_cmp expect actual\n+\n+'\n+\n test_expect_success 'diff-filter=C' '\n \n \tgit log -C -C --pretty=\"format:%s\" --diff-filter=C HEAD >actual &&\n-- \ngitgitgadget\n"}]}