{"thread":{"id":"58353","subject":"[PATCH 3/3] add -p: handle `diff-so-fancy`'s hunk headers better","startedAt":"2022-08-23T19:23:24Z","lastAt":"2022-09-02T08:53:21Z","messageCount":67,"participants":["Johannes Schindelin via GitGitGadget","Philippe Blain","Johannes Schindelin","Junio C Hamano","Phillip Wood","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"461868","messageId":"9dac9f74d2e19899b3e6c1d28e83878ded4469d6.1661277870.git.gitgitgadget@gmail.com","threadId":"58353","inReplyTo":"pull.1336.git.1661277870.gitgitgadget@gmail.com","subject":"[PATCH 3/3] add -p: handle `diff-so-fancy`'s hunk headers better","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-23T18:04:30Z","receivedAt":"2022-08-23T19:23:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe `diff-so-fancy` diff colorizer produces hunk headers that look\nnothing like the built-in `add -p` expects: there is no `@@ ... @@` line\nrange, and therefore the parser cannot determine where any extra\ninformation starts, such as the function name that is often added to\nthose hunk header lines.\n\nHowever, we can do better than simply swallowing the unparseable hunk\nheader. In the `diff-so-fancy` case, it shows something like `@ file:1\n@`. Let's just show the complete hunk header because it probably offers\nuseful information.\n\nSigned-off-by: Johannes Schindelin \n---\n add-patch.c | 8 +++++++-\n t/t3701-add-interactive.sh | 3 ++-\n 2 files changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex f2fffe1af02..1f3f3611ee9 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -8,6 +8,7 @@\n #include \"diff.h\"\n #include \"compat/terminal.h\"\n #include \"prompt.h\"\n+#include \"utf8.h\"\n \n enum prompt_mode_type {\n \tPROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_ADDITION, PROMPT_HUNK,\n@@ -363,7 +364,7 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)\n \t\theader->colored_extra_start = p + 3 - s->colored.buf;\n \telse\n \t\t/* could not parse colored hunk header, showing nothing */\n-\t\theader->colored_extra_start = hunk->colored_start;\n+\t\theader->colored_extra_start = line - s->colored.buf;\n \theader->colored_extra_end = hunk->colored_start;\n \n \treturn 0;\n@@ -649,6 +650,7 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n \t\tsize_t len;\n \t\tunsigned long old_offset = header->old_offset;\n \t\tunsigned long new_offset = header->new_offset;\n+\t\tint needs_extra_space = 0;\n \n \t\tif (!colored) {\n \t\t\tp = s->plain.buf + header->extra_start;\n@@ -658,6 +660,8 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n \t\t\tp = s->colored.buf + header->colored_extra_start;\n \t\t\tlen = header->colored_extra_end\n \t\t\t\t- header->colored_extra_start;\n+\t\t\tif (utf8_strnwidth(p, len, 1 /* skip ANSI */) > 0)\n+\t\t\t\tneeds_extra_space = 1;\n \t\t}\n \n \t\tif (s->mode->is_reverse)\n@@ -673,6 +677,8 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n \t\t\tstrbuf_addf(out, \",%lu\", header->new_count);\n \t\tstrbuf_addstr(out, \" @@\");\n \n+\t\tif (needs_extra_space)\n+\t\t\tstrbuf_addch(out, ' ');\n \t\tif (len)\n \t\t\tstrbuf_add(out, p, len);\n \t\telse if (colored)\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 7e3c1de71f5..9deb7a87f1e 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -772,7 +772,8 @@ test_expect_success 'gracefully fail to parse colored hunk header' '\n \techo content >test &&\n \ttest_config interactive.diffFilter \"sed s/@@/XX/g\" &&\n \tprintf y >y &&\n-\tforce_color git add -p output 2>&1 \n\nWhen parsing the colored version of a diff, the interactive `add`\ncommand really relies on the colored version having the same number of\nlines as the non-colored version. That is an invariant.\n\nHowever, in the 'detect bogus diffFilter output' test case in t3701, we\nessentially required a hunk header that contains parseable `@@ ... @@`\nhunk headers, and called all colored diffs without such hunks bogus.\n\nThe reason for this is that we would like to show the users the adjusted\nhunk headers _including_ the extra part after the `@@ ... @@`\ninformation, which usually contains things like the function name or\nsoms such.\n\nNow, there is a _very_ popular diff colorizer called `diff-so-fancy`\nthat does not produce such colored diffs as the built-in `add` command\nexpects. Nevertheless, the Perl variant of the `add` command handles\nthose nicely, essentially by ignoring the hunk header and saying \"there\nis nothing else we can show except the original hunk header, even if we\nhad to adjust the line range and the original hunk header might get that\nwrong\".\n\nIn preparation for teaching the built-in interactive `add` to be a bit\nmore lenient, let's change the 'detect bogus diffFilter output' test\ncase so that it verifies that a mismatched number of lines causes the\ncommand to error out, but not an unparseable hunk header.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t3701-add-interactive.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex b354fb39de8..b40d1c94d99 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -761,7 +761,7 @@ test_expect_success 'detect bogus diffFilter output' '\n \tgit reset --hard &&\n \n \techo content >test &&\n-\ttest_config interactive.diffFilter \"sed 1d\" &&\n+\ttest_config interactive.diffFilter \"sed q\" &&\n \tprintf y >y &&\n \tforce_color test_must_fail git add -p \n\nIn\nhttps://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com,\nPhillipe Blain reported that the built-in `git add -p` command fails\nwhen asked to use [`diff-so-fancy`][diff-so-fancy] to colorize the diff.\n\nThe reason is that this tool produces colored diffs with a hunk header\nthat does not contain any parseable `@@ ... @@` line range information,\nand therefore we cannot detect any part in that header that comes after\nthe line range.\n\nLet's punt for now and simply show nothing apart from the line range in\nthat case.\n\n[diff-so-fancy]: https://github.com/so-fancy/diff-so-fancy\n\nSigned-off-by: Johannes Schindelin \n---\n add-patch.c | 15 ++++++---------\n t/t3701-add-interactive.sh | 9 +++++++++\n 2 files changed, 15 insertions(+), 9 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 509ca04456b..f2fffe1af02 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -357,16 +357,13 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)\n \teol = memchr(line, '\\n', s->colored.len - hunk->colored_start);\n \tif (!eol)\n \t\teol = s->colored.buf + s->colored.len;\n-\tp = memmem(line, eol - line, \"@@ -\", 4);\n-\tif (!p)\n-\t\treturn error(_(\"could not parse colored hunk header '%.*s'\"),\n-\t\t\t (int)(eol - line), line);\n-\tp = memmem(p + 4, eol - p - 4, \" @@\", 3);\n-\tif (!p)\n-\t\treturn error(_(\"could not parse colored hunk header '%.*s'\"),\n-\t\t\t (int)(eol - line), line);\n \thunk->colored_start = eol - s->colored.buf + (*eol == '\\n');\n-\theader->colored_extra_start = p + 3 - s->colored.buf;\n+\tp = memmem(line, eol - line, \"@@ -\", 4);\n+\tif (p && (p = memmem(p + 4, eol - p - 4, \" @@\", 3)))\n+\t\theader->colored_extra_start = p + 3 - s->colored.buf;\n+\telse\n+\t\t/* could not parse colored hunk header, showing nothing */\n+\t\theader->colored_extra_start = hunk->colored_start;\n \theader->colored_extra_end = hunk->colored_start;\n \n \treturn 0;\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex b40d1c94d99..7e3c1de71f5 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -766,6 +766,15 @@ test_expect_success 'detect bogus diffFilter output' '\n \tforce_color test_must_fail git add -p test &&\n+\ttest_config interactive.diffFilter \"sed s/@@/XX/g\" &&\n+\tprintf y >y &&\n+\tforce_color git add -p Philippe Blain reported in\n> https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com\n> that there is a problem when running the built-in version of git add -p with\n> diff-so-fancy [https://github.com/so-fancy/diff-so-fancy] as diff colorizer.\n> The symptom is this:\n> \n> error: could not parse colored hunk header '?[36m?[1m?[38;5;13m@ file:1 @?[1m?[0m'\n> \n> \n> This patch series addresses that and should fix\n> https://github.com/so-fancy/diff-so-fancy/issues/437\n\nThanks for the quick reaction!\nI've verified that the patches fix the example in my reproducer, and they also fix it\nif using \"delta --color-only\" as interactive.diffFilter. Delta is another diff colorizer\n(and more) that's facing the same issue as diff-so-fancy [1].\n\nHowever, I've tried it on a more \"real-life\" example, and then I get:\n\n error: mismatched output from interactive.diffFilter\n hint: Your filter must maintain a one-to-one correspondence\n hint: between its input and output lines.\n\nThis is despite using \"diff-so-fancy --patch\" as interactive.diffFilter, which should \nkeep the number of lines the same. This ('--patch') was added in [2], about a month after Peff wrote the message\nyou mention in https://lore.kernel.org/git/s40ss309-3311-o08s-38r2-9144r33pq549@tzk.qr/.\n\nAgain, when using the Perl version with this new example, it works correctly. I'll try to come up with a new\nreproducer for this... But this new example does work with delta with the builtin version,\nso it might be diff-so-fancy that's the culprit...\n\nCheers,\n\nPhilippe.\n\n[1] https://github.com/dandavison/delta/issues/1114\n[2] https://github.com/so-fancy/diff-so-fancy/commit/13d3f8949e15dd62f6b49bc652fe94af6a9bfc79\n"},{"id":"461889","messageId":"1r3pq48q-s728-8r2r-r4q3-72413r5483o1@tzk.qr","threadId":"58353","inReplyTo":"ab012782-1d02-b90b-9a4a-179ecc3f8e9e@gmail.com","subject":"Re: [PATCH 0/3] built-in add -p: support diff-so-fancy better","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-24T06:27:40Z","receivedAt":"2022-08-24T06:27:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Philippe,\n\nOn Tue, 23 Aug 2022, Philippe Blain wrote:\n\n> Le 2022-08-23 à 14:04, Johannes Schindelin via GitGitGadget a écrit :\n> > Philippe Blain reported in\n> > https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com\n> > that there is a problem when running the built-in version of git add -p with\n> > diff-so-fancy [https://github.com/so-fancy/diff-so-fancy] as diff colorizer.\n> > The symptom is this:\n> >\n> > error: could not parse colored hunk header '?[36m?[1m?[38;5;13m@ file:1 @?[1m?[0m'\n> >\n> >\n> > This patch series addresses that and should fix\n> > https://github.com/so-fancy/diff-so-fancy/issues/437\n>\n> Thanks for the quick reaction!\n> I've verified that the patches fix the example in my reproducer, and they also fix it\n> if using \"delta --color-only\" as interactive.diffFilter. Delta is another diff colorizer\n> (and more) that's facing the same issue as diff-so-fancy [1].\n\nGreat!\n\n> However, I've tried it on a more \"real-life\" example, and then I get:\n>\n> error: mismatched output from interactive.diffFilter\n> hint: Your filter must maintain a one-to-one correspondence\n> hint: between its input and output lines.\n>\n> This is despite using \"diff-so-fancy --patch\" as interactive.diffFilter, which should\n> keep the number of lines the same.\n\nWould you mind sharing the example with me?\n\nThanks,\nDscho\n\n> This ('--patch') was added in [2], about a month after Peff wrote the message\n> you mention in https://lore.kernel.org/git/s40ss309-3311-o08s-38r2-9144r33pq549@tzk.qr/.\n>\n> Again, when using the Perl version with this new example, it works correctly. I'll try to come up with a new\n> reproducer for this... But this new example does work with delta with the builtin version,\n> so it might be diff-so-fancy that's the culprit...\n>\n> Cheers,\n>\n> Philippe.\n>\n> [1] https://github.com/dandavison/delta/issues/1114\n> [2] https://github.com/so-fancy/diff-so-fancy/commit/13d3f8949e15dd62f6b49bc652fe94af6a9bfc79\n>\n"},{"id":"461895","messageId":"41781ceb-bfad-2534-2970-c74618c93f2a@gmail.com","threadId":"58353","inReplyTo":"1r3pq48q-s728-8r2r-r4q3-72413r5483o1@tzk.qr","subject":"Re: [PATCH 0/3] built-in add -p: support diff-so-fancy better","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2022-08-24T13:21:18Z","receivedAt":"2022-08-24T13:21:25Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Dscho,\n\nLe 2022-08-24 à 02:27, Johannes Schindelin a écrit :\n> Hi Philippe,\n> \n> On Tue, 23 Aug 2022, Philippe Blain wrote:\n> \n>> However, I've tried it on a more \"real-life\" example, and then I get:\n>>\n>> error: mismatched output from interactive.diffFilter\n>> hint: Your filter must maintain a one-to-one correspondence\n>> hint: between its input and output lines.\n>>\n>> This is despite using \"diff-so-fancy --patch\" as interactive.diffFilter, which should\n>> keep the number of lines the same.\n> \n> Would you mind sharing the example with me?\n> \n> Thanks,\n> Dscho\n\nIn trying to write a reproducer, I realize that this is in fact a separate \"bug\".\nI happened to have a submodule in the repository I was testing, and it had modified \ncontent. This is what was causing the \"mismatched output\" error, although I'm not\nsure why. If I use a pathspec that does not include the submodule when invoking \n'git add -p', it works correctly. \n\nI'm a bit out of time now but I'll try to write a separate reproducer for this later today.\n\nAs for my real-life example, I noticed a little change in the output. \nHere is what it looks like with the Perl version:\n\n$ git -c interactive.diffFilter=\"diff-so-fancy --patch\" -c add.interactive.useBuiltin=false -add p ice_dyn_vp.F90\n\n───────────────────────────────────────────────────────────────────────\nmodified: ice_dyn_vp.F90\n───────────────────────────────────────────────────────────────────────\n@ ice_dyn_vp.F90:274 @ subroutine implicit_solver (dt)\n do i = 1, nx_block\n rdg_conv (i,j,iblk) = c0\n rdg_shear(i,j,iblk) = c0\n divu (i,j,iblk) = c0\n divv (i,j,iblk) = c0\n shear(i,j,iblk) = c0\n enddo\n enddo\n(1/1) Stage this hunk [y,n,q,a,d,e,?]? \n\nThe first part of the hunk header \"ice_dyn_vp.F90:274\" is in pink and the subroutine\nname is in light mauve (default diff-so-fancy colors). Here is what it looks like\nwith your patches with the builtin version:\n\n$ git -c interactive.diffFilter=\"diff-so-fancy --patch\" add -p ice_dyn_vp.F90\n\n───────────────────────────────────────────────────────────────────────\nmodified: ice_dyn_vp.F90\n───────────────────────────────────────────────────────────────────────\n@@ -271,7 +271,7 @@ @ ice_dyn_vp.F90:274 @ subroutine implicit_solver (dt)\n do i = 1, nx_block\n rdg_conv (i,j,iblk) = c0\n rdg_shear(i,j,iblk) = c0\n divu (i,j,iblk) = c0\n divv (i,j,iblk) = c0\n shear(i,j,iblk) = c0\n enddo\n enddo\n(1/1) Stage this hunk [y,n,q,a,d,e,?]? \n\nNotice we have the line numbers (-271,7 +271,7) in cyan, then the first part\nof the hunk header modified by diff-so-fancy (ice_dyn_vp.F90:274) in pink, and\nthen the subroutine name (in light mauve). \n\nWe can see the same thing with my earlier reproducer, only there is no function\nmatched. I really don't mind having this first part with the line numbers, I\njust wanted to share my observation.\n\nThanks,\n\nPhilippe.\n"},{"id":"461903","messageId":"145ddce5-8084-a024-39b5-9dea870d1afe@gmail.com","threadId":"58353","inReplyTo":"41781ceb-bfad-2534-2970-c74618c93f2a@gmail.com","subject":"Re: [PATCH 0/3] built-in add -p: support diff-so-fancy better","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2022-08-24T17:49:30Z","receivedAt":"2022-08-24T17:49:40Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Dscho,\n\nLe 2022-08-24 à 09:21, Philippe Blain a écrit :\n> Hi Dscho,\n> \n> Le 2022-08-24 à 02:27, Johannes Schindelin a écrit :\n>> Hi Philippe,\n>>\n>> On Tue, 23 Aug 2022, Philippe Blain wrote:\n>>\n>>> However, I've tried it on a more \"real-life\" example, and then I get:\n>>>\n>>> error: mismatched output from interactive.diffFilter\n>>> hint: Your filter must maintain a one-to-one correspondence\n>>> hint: between its input and output lines.\n>>>\n>>> This is despite using \"diff-so-fancy --patch\" as interactive.diffFilter, which should\n>>> keep the number of lines the same.\n>>\n>> Would you mind sharing the example with me?\n>>\n>> Thanks,\n>> Dscho\n> \n> In trying to write a reproducer, I realize that this is in fact a separate \"bug\".\n> I happened to have a submodule in the repository I was testing, and it had modified \n> content. This is what was causing the \"mismatched output\" error, although I'm not\n> sure why. If I use a pathspec that does not include the submodule when invoking \n> 'git add -p', it works correctly. \n> \n> I'm a bit out of time now but I'll try to write a separate reproducer for this later today.\n\nSo in trying to write a new reproducer, I found the cause of the bug. A submodule in \"modified \ncontent\" or \"untracked content\" state (i.e. no new commit) has no \"index\" line in its diff\nheader. diff-so-fancy, with --patch, always *adds* a blank line before the diff-header \nbecause its modified diff header is 3 lines, whereas Git's is 4. So the count is off \nspecifically for submodules in \"modified/untracked content\" state. \n\nIt seems the C version is less lenient than the Perl version, because the Perl version does\nnot complain. But I'd say that it's more of a bug on the diff-so-fancy side, even if it \nis a regression of the builtin version...\nDelta does not alter the header (in its default configuration) and so it is not affected here.\n\nFor what it's worth, here is the updated reproducer script. It also downloads the 'delta' executable (for Linux)\nand runs it. With delta, both Perl and C versions work. With diff-so-fancy, the C version \ncomplains bout mismatched output.\n\n```bash\n#!/bin/bash\n\nset -euEo pipefail\n\nrepo=repro\nrm -rf $repo\n\nTEST_AUTHOR_LOCALNAME=author\nTEST_AUTHOR_DOMAIN=example.com\nGIT_AUTHOR_EMAIL=${TEST_AUTHOR_LOCALNAME}@${TEST_AUTHOR_DOMAIN}\nGIT_AUTHOR_NAME='A U Thor'\nGIT_AUTHOR_DATE='1112354055 +0200'\nTEST_COMMITTER_LOCALNAME=committer\nTEST_COMMITTER_DOMAIN=example.com\nGIT_COMMITTER_EMAIL=${TEST_COMMITTER_LOCALNAME}@${TEST_COMMITTER_DOMAIN}\nGIT_COMMITTER_NAME='C O Mitter'\nGIT_COMMITTER_DATE='1112354055 +0200'\nexport GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME\nexport GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME\nexport GIT_COMMITTER_DATE GIT_AUTHOR_DATE\nexport HOME=\n\ngit -c init.defaultBranch=unimportant init $repo\ncd $repo\ndate>file\ngit add file\ngit commit -m file\ngit clone https://github.com/so-fancy/diff-so-fancy.git\ndate>>file\nexport PATH=\"diff-so-fancy:$PATH\"\ngit submodule add ./diff-so-fancy\ngit commit -m \"add sub\"\ndate>diff-so-fancy/README.md\ncurl -sSLO https://github.com/dandavison/delta/releases/download/0.12.1/delta-0.12.1-x86_64-unknown-linux-gnu.tar.gz\ntar xzf delta-0.12.1-x86_64-unknown-linux-gnu.tar.gz\nexport PATH=\"delta-0.12.1-x86_64-unknown-linux-gnu:$PATH\"\necho -----\ngit -c interactive.diffFilter=\"delta --color-only\" -c add.interactive.useBuiltin=false add -p < <(echo n)\necho -----\ngit -c interactive.diffFilter=\"delta --color-only\" add -p < <(echo n)\necho -----\ngit -c interactive.diffFilter=\"diff-so-fancy --patch\" -c add.interactive.useBuiltin=false add -p < <(echo n)\necho -----\ngit -c interactive.diffFilter=\"diff-so-fancy --patch\" add -p < <(echo n) \n```\n\nCheers,\n\nPhilippe.\n"},{"id":"461906","messageId":"xmqqmtbt7b5p.fsf@gitster.g","threadId":"58353","inReplyTo":"145ddce5-8084-a024-39b5-9dea870d1afe@gmail.com","subject":"Re: [PATCH 0/3] built-in add -p: support diff-so-fancy better","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-24T18:24:02Z","receivedAt":"2022-08-24T18:24:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Philippe Blain writes:\n\n>>>> However, I've tried it on a more \"real-life\" example, and then I get:\n>>>>\n>>>> error: mismatched output from interactive.diffFilter\n>>>> hint: Your filter must maintain a one-to-one correspondence\n>>>> hint: between its input and output lines.\n>>>>\n>>>> This is despite using \"diff-so-fancy --patch\" as interactive.diffFilter, which should\n>>>> keep the number of lines the same.\n>>>\n>>> Would you mind sharing the example with me?\n>>>\n>>> Thanks,\n>>> Dscho\n>> \n>> In trying to write a reproducer, I realize that this is in fact a separate \"bug\".\n>> I happened to have a submodule in the repository I was testing, and it had modified \n>> content. This is what was causing the \"mismatched output\" error, although I'm not\n>> sure why. If I use a pathspec that does not include the submodule when invoking \n>> 'git add -p', it works correctly. \n>> \n>> I'm a bit out of time now but I'll try to write a separate\n>> reproducer for this later today.\n>\n> So in trying to write a new reproducer, I found the cause of the\n> bug. ...\n\nI somehow had an impression that that the C reimplementation was\ndesigned to be a bug-for-bug compatible faithful rewrite of the\noriginal scripted version, but looking at the way how this bug is\nhandled, I am not sure (it looks as if the initial fix was to patch\nthe code to match the symptom, not to make the code to behave more\nfaithfully to the scripted version). As with any big rewrite, a\ncomplete re-implementation always has risks to introduce unintended\nregressions and that is why starting with faithful rewrite with the\nstaged transition plan is valuable.\n\nIn this case, the staged transition plan, the step to flip the\ndefault without before remove the old one, is working beautifully.\nAnd it was only made possible with the actual users reporting issues\nbefore they say \"the new one is broken, so let's use the knob to\nretain the old implementation\". Please thank those in diff-so-fancy\nland for reporting the issue.\n\nThanks, you two, for working on this.\n"},{"id":"461911","messageId":"34qs5607-001o-0n6o-rson-9587q8r909qn@tzk.qr","threadId":"58353","inReplyTo":"xmqqmtbt7b5p.fsf@gitster.g","subject":"Re: [PATCH 0/3] built-in add -p: support diff-so-fancy better","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-24T21:05:38Z","receivedAt":"2022-08-24T21:05:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio & Philippe,\n\nOn Wed, 24 Aug 2022, Junio C Hamano wrote:\n\n> Philippe Blain writes:\n>\n> >>>> However, I've tried it on a more \"real-life\" example, and then I get:\n> >>>>\n> >>>> error: mismatched output from interactive.diffFilter\n> >>>> hint: Your filter must maintain a one-to-one correspondence\n> >>>> hint: between its input and output lines.\n> >>>>\n> >>>> This is despite using \"diff-so-fancy --patch\" as interactive.diffFilter, which should\n> >>>> keep the number of lines the same.\n> >>>\n> >>> Would you mind sharing the example with me?\n> >>\n> >> In trying to write a reproducer, I realize that this is in fact a separate \"bug\".\n> >> I happened to have a submodule in the repository I was testing, and it had modified\n> >> content. This is what was causing the \"mismatched output\" error, although I'm not\n> >> sure why. If I use a pathspec that does not include the submodule when invoking\n> >> 'git add -p', it works correctly.\n> >>\n> >> I'm a bit out of time now but I'll try to write a separate\n> >> reproducer for this later today.\n> >\n> > So in trying to write a new reproducer, I found the cause of the\n> > bug. ...\n\nAh. Submodules. The gift that keeps on giving.\n\nI'm not _really_ complaining, though. It seems that with beautiful\nregularity, submodules come up as crucial parts of attack vectors in\nexploits that are reported to the git-security mailing list, requiring\nsomebody with my abilities to implement fixes. Therefore, submodules award\nme with a sort of job security. :-)\n\n> I somehow had an impression that that the C reimplementation was\n> designed to be a bug-for-bug compatible faithful rewrite of the\n> original scripted version,\n\nYes, it was designed as that, with a few things done differently, though,\nsuch as avoiding unnecessary `git diff` invocations, and using C-native\ndata structures and memory management.\n\nOne unfortunate consequence of avoiding the `git diff` invocations of\n`git-add--interactive.perl` that are not actually necessary is that I\nmissed that one of those invocations specifically ignores dirty\nsubmodules, and that the Perl script then only processes files whose\nnumstat shows up in _that_ diff output.\n\nFortunately, the fix is easy: simply ignore dirty submodules in _all_ `git\ndiff` invocations of `add -p`.\n\nI will submit the next iteration as soon as the PR builds pass.\n\n> but looking at the way how this bug is handled, I am not sure (it looks\n> as if the initial fix was to patch the code to match the symptom, not to\n> make the code to behave more faithfully to the scripted version).\n\nThe built-in `add -p` does something slightly different from the Perl\nversion when it comes to the line ranges in hunk headers: instead of\nstoring stringified versions of them and parsing & re-rendering them when\nthe hunks are modified, the C version stores the actual numbers, adjusts\nthem as needed (e.g. when hunks are split), and, crucially, renders them\non the fly when displaying the hunks.\n\nThat means that also the colorized hunk headers are generated on the fly\nwhenever the hunks are displayed, and they are never stored in rendered\nform, neither while parsing diffs nor when hunks are split or otherwise\nedited. That's why the built-in `add -p` looked for a `@@ ... @@` part in\nthe colorized diff, so that it can display the remainder of that line\nafter showing the rendered line range.\n\nAnd yes, this is a deviation from the bug-for-bug principle I follow for\nconversions from scripted commands to built-in C code, but it was for a\ngood reason: memory management in C is less trivial than in Perl (where\none essentially YOLOs memory management), and I was not prepared to invent\na rope data structure just to be able to replace parts of a long string\nbuffer with rendered text of a different length, nor was I prepared to\ncall `memmove()` tons of times.\n\nNote that the Perl version does something slightly iffy, too: if a diff\ncolorizer is configured, it initially shows the original hunk headers\n(e.g. \"@ file:16 @\" in the `diff-so-fancy` case) but when the hunk is\nmodified in any way, it generates \"@@ -1,6 +1,7 @@\"-style hunk headers and\ndoes not show the ones generated by the diff colorizer _at all_ anymore.\n\nIn this patch series, I teach the built-in `add -p` to be more lenient and\nsimply show the entire original hunk header after the rendered line range\nif no `@@ ... @@` part was found. If that part was found, we still replace\nit with the rendered line range.\n\nYes, it is a deviation from what the Perl version does. I consider it an\nimprovement, though.\n\n> As with any big rewrite, a complete re-implementation always has risks\n> to introduce unintended regressions and that is why starting with\n> faithful rewrite with the staged transition plan is valuable.\n>\n> In this case, the staged transition plan, the step to flip the\n> default without before remove the old one, is working beautifully.\n> And it was only made possible with the actual users reporting issues\n> before they say \"the new one is broken, so let's use the knob to\n> retain the old implementation\".\n\nIndeed, and thank you, Philippe, for bringing this to the Git mailing list\nso that I could do something about the bug.\n\nCiao,\nDscho\n"},{"id":"461915","messageId":"pull.1336.v2.git.1661376112.gitgitgadget@gmail.com","threadId":"58353","inReplyTo":"pull.1336.git.1661277870.gitgitgadget@gmail.com","subject":"[PATCH v2 0/4] built-in add -p: support diff-so-fancy better","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-24T21:21:47Z","receivedAt":"2022-08-24T21:21:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Philippe Blain reported in\nhttps://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com\nthat there is a problem when running the built-in version of git add -p with\ndiff-so-fancy [https://github.com/so-fancy/diff-so-fancy] as diff colorizer.\nThe symptom is this:\n\n error: could not parse colored hunk header '?[36m?[1m?[38;5;13m@ file:1 @?[1m?[0m'\n\n\nThis patch series addresses that and should fix\nhttps://github.com/so-fancy/diff-so-fancy/issues/437\n\nChanges since v1:\n\n * Added a commit to ignore dirty submodules just like the Perl version\n does.\n\nJohannes Schindelin (4):\n t3701: redefine what is \"bogus\" output of a diff filter\n add -p: gracefully ignore unparseable hunk headers in colored diffs\n add -p: handle `diff-so-fancy`'s hunk headers better\n add -p: ignore dirty submodules\n\n add-patch.c | 24 ++++++++++++++----------\n t/t3701-add-interactive.sh | 24 +++++++++++++++++++++++-\n 2 files changed, 37 insertions(+), 11 deletions(-)\n\n\nbase-commit: 795ea8776befc95ea2becd8020c7a284677b4161\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1336%2Fdscho%2Fdiff-so-fancy-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1336/dscho/diff-so-fancy-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1336\n\nRange-diff vs v1:\n\n 1: 74ab50eeb1c = 1: 74ab50eeb1c t3701: redefine what is \"bogus\" output of a diff filter\n 2: b07f85a0359 = 2: b07f85a0359 add -p: gracefully ignore unparseable hunk headers in colored diffs\n 3: 9dac9f74d2e = 3: 9dac9f74d2e add -p: handle `diff-so-fancy`'s hunk headers better\n -: ----------- > 4: 540ce27c38a add -p: ignore dirty submodules\n\n-- \ngitgitgadget\n"},{"id":"461916","messageId":"74ab50eeb1c6debd2f371e548cf6c782479f69af.1661376112.git.gitgitgadget@gmail.com","threadId":"58353","inReplyTo":"pull.1336.v2.git.1661376112.gitgitgadget@gmail.com","subject":"[PATCH v2 1/4] t3701: redefine what is \"bogus\" output of a diff filter","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-24T21:21:48Z","receivedAt":"2022-08-24T21:22:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen parsing the colored version of a diff, the interactive `add`\ncommand really relies on the colored version having the same number of\nlines as the non-colored version. That is an invariant.\n\nHowever, in the 'detect bogus diffFilter output' test case in t3701, we\nessentially required a hunk header that contains parseable `@@ ... @@`\nhunk headers, and called all colored diffs without such hunks bogus.\n\nThe reason for this is that we would like to show the users the adjusted\nhunk headers _including_ the extra part after the `@@ ... @@`\ninformation, which usually contains things like the function name or\nsoms such.\n\nNow, there is a _very_ popular diff colorizer called `diff-so-fancy`\nthat does not produce such colored diffs as the built-in `add` command\nexpects. Nevertheless, the Perl variant of the `add` command handles\nthose nicely, essentially by ignoring the hunk header and saying \"there\nis nothing else we can show except the original hunk header, even if we\nhad to adjust the line range and the original hunk header might get that\nwrong\".\n\nIn preparation for teaching the built-in interactive `add` to be a bit\nmore lenient, let's change the 'detect bogus diffFilter output' test\ncase so that it verifies that a mismatched number of lines causes the\ncommand to error out, but not an unparseable hunk header.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t3701-add-interactive.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex b354fb39de8..b40d1c94d99 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -761,7 +761,7 @@ test_expect_success 'detect bogus diffFilter output' '\n \tgit reset --hard &&\n \n \techo content >test &&\n-\ttest_config interactive.diffFilter \"sed 1d\" &&\n+\ttest_config interactive.diffFilter \"sed q\" &&\n \tprintf y >y &&\n \tforce_color test_must_fail git add -p \n\nIn\nhttps://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com,\nPhillipe Blain reported that the built-in `git add -p` command fails\nwhen asked to use [`diff-so-fancy`][diff-so-fancy] to colorize the diff.\n\nThe reason is that this tool produces colored diffs with a hunk header\nthat does not contain any parseable `@@ ... @@` line range information,\nand therefore we cannot detect any part in that header that comes after\nthe line range.\n\nLet's punt for now and simply show nothing apart from the line range in\nthat case.\n\n[diff-so-fancy]: https://github.com/so-fancy/diff-so-fancy\n\nSigned-off-by: Johannes Schindelin \n---\n add-patch.c | 15 ++++++---------\n t/t3701-add-interactive.sh | 9 +++++++++\n 2 files changed, 15 insertions(+), 9 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 509ca04456b..f2fffe1af02 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -357,16 +357,13 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)\n \teol = memchr(line, '\\n', s->colored.len - hunk->colored_start);\n \tif (!eol)\n \t\teol = s->colored.buf + s->colored.len;\n-\tp = memmem(line, eol - line, \"@@ -\", 4);\n-\tif (!p)\n-\t\treturn error(_(\"could not parse colored hunk header '%.*s'\"),\n-\t\t\t (int)(eol - line), line);\n-\tp = memmem(p + 4, eol - p - 4, \" @@\", 3);\n-\tif (!p)\n-\t\treturn error(_(\"could not parse colored hunk header '%.*s'\"),\n-\t\t\t (int)(eol - line), line);\n \thunk->colored_start = eol - s->colored.buf + (*eol == '\\n');\n-\theader->colored_extra_start = p + 3 - s->colored.buf;\n+\tp = memmem(line, eol - line, \"@@ -\", 4);\n+\tif (p && (p = memmem(p + 4, eol - p - 4, \" @@\", 3)))\n+\t\theader->colored_extra_start = p + 3 - s->colored.buf;\n+\telse\n+\t\t/* could not parse colored hunk header, showing nothing */\n+\t\theader->colored_extra_start = hunk->colored_start;\n \theader->colored_extra_end = hunk->colored_start;\n \n \treturn 0;\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex b40d1c94d99..7e3c1de71f5 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -766,6 +766,15 @@ test_expect_success 'detect bogus diffFilter output' '\n \tforce_color test_must_fail git add -p test &&\n+\ttest_config interactive.diffFilter \"sed s/@@/XX/g\" &&\n+\tprintf y >y &&\n+\tforce_color git add -p \n\nThe `diff-so-fancy` diff colorizer produces hunk headers that look\nnothing like the built-in `add -p` expects: there is no `@@ ... @@` line\nrange, and therefore the parser cannot determine where any extra\ninformation starts, such as the function name that is often added to\nthose hunk header lines.\n\nHowever, we can do better than simply swallowing the unparseable hunk\nheader. In the `diff-so-fancy` case, it shows something like `@ file:1\n@`. Let's just show the complete hunk header because it probably offers\nuseful information.\n\nSigned-off-by: Johannes Schindelin \n---\n add-patch.c | 8 +++++++-\n t/t3701-add-interactive.sh | 3 ++-\n 2 files changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex f2fffe1af02..1f3f3611ee9 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -8,6 +8,7 @@\n #include \"diff.h\"\n #include \"compat/terminal.h\"\n #include \"prompt.h\"\n+#include \"utf8.h\"\n \n enum prompt_mode_type {\n \tPROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_ADDITION, PROMPT_HUNK,\n@@ -363,7 +364,7 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)\n \t\theader->colored_extra_start = p + 3 - s->colored.buf;\n \telse\n \t\t/* could not parse colored hunk header, showing nothing */\n-\t\theader->colored_extra_start = hunk->colored_start;\n+\t\theader->colored_extra_start = line - s->colored.buf;\n \theader->colored_extra_end = hunk->colored_start;\n \n \treturn 0;\n@@ -649,6 +650,7 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n \t\tsize_t len;\n \t\tunsigned long old_offset = header->old_offset;\n \t\tunsigned long new_offset = header->new_offset;\n+\t\tint needs_extra_space = 0;\n \n \t\tif (!colored) {\n \t\t\tp = s->plain.buf + header->extra_start;\n@@ -658,6 +660,8 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n \t\t\tp = s->colored.buf + header->colored_extra_start;\n \t\t\tlen = header->colored_extra_end\n \t\t\t\t- header->colored_extra_start;\n+\t\t\tif (utf8_strnwidth(p, len, 1 /* skip ANSI */) > 0)\n+\t\t\t\tneeds_extra_space = 1;\n \t\t}\n \n \t\tif (s->mode->is_reverse)\n@@ -673,6 +677,8 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n \t\t\tstrbuf_addf(out, \",%lu\", header->new_count);\n \t\tstrbuf_addstr(out, \" @@\");\n \n+\t\tif (needs_extra_space)\n+\t\t\tstrbuf_addch(out, ' ');\n \t\tif (len)\n \t\t\tstrbuf_add(out, p, len);\n \t\telse if (colored)\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 7e3c1de71f5..9deb7a87f1e 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -772,7 +772,8 @@ test_expect_success 'gracefully fail to parse colored hunk header' '\n \techo content >test &&\n \ttest_config interactive.diffFilter \"sed s/@@/XX/g\" &&\n \tprintf y >y &&\n-\tforce_color git add -p output 2>&1 \n\nThanks to alwyas running `diff-index` and `diff-files` with the\n`--numstat` option (the latter with `--ignore-submodules=dirty`) before\neven generating any real diff to parse, the Perl version of `git add -p`\nsimply ignored dirty submodules and does not even offer them up for\nstaging.\n\nHowever, the built-in variant did not use that flag because it tries to\nrun only one `diff` command, skipping the unneeded\n`diff-index`/`diff-files` invocation of the Perl variant and therefore\nonly faithfully recapitulates what the Perl code does once it _does_\ngenerate and parse the real diff.\n\nThis causes a problem when running the built-in `add -p` with\n`diff-so-fancy` because that diff colorizer always inserts an empty line\nbefore the diff header to ensure that it produces 4 lines as expected by\n`git add -p` (the equivalent of the non-colorized `diff`, `index`, `---`\nand `+++` lines). But `git diff-files` does not produce any `index` line\nfor dirty submodules.\n\nThe underlying problem is not even the discrepancy in lines, but that\n`git add -p` presents diffs for dirty submodules: there is nothing that\n_can_ be staged for those.\n\nLet's fix that bug, and teach the built-in `add -p` to ignore dirty\nsubmodules, too. This _incidentally_ also fixes the `diff-so-fancy`\nproblem ;-)\n\nSigned-off-by: Johannes Schindelin \n---\n add-patch.c | 3 ++-\n t/t3701-add-interactive.sh | 12 ++++++++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 1f3f3611ee9..64807e7cdc0 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -417,7 +417,8 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n \t}\n \tcolor_arg_index = args.nr;\n \t/* Use `--no-color` explicitly, just in case `diff.color = always`. */\n-\tstrvec_pushl(&args, \"--no-color\", \"-p\", \"--\", NULL);\n+\tstrvec_pushl(&args, \"--no-color\", \"--ignore-submodules=dirty\", \"-p\",\n+\t\t \"--\", NULL);\n \tfor (i = 0; i < ps->nr; i++)\n \t\tstrvec_push(&args, ps->items[i].original);\n \ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 9deb7a87f1e..10058b7ce42 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -941,6 +941,18 @@ test_expect_success 'status ignores dirty submodules (except HEAD)' '\n \t! grep dirty-otherwise output\n '\n \n+test_expect_success 'handle submodules' '\n+\techo 123 >>for-submodules/dirty-otherwise/initial.t &&\n+\n+\tforce_color git -C for-submodules add -p dirty-otherwise >output 2>&1 &&\n+\tgrep \"No changes\" output &&\n+\n+\tforce_color git -C for-submodules add -p dirty-head >output 2>&1 actual &&\n+\trev=\"$(git -C for-submodules/dirty-head rev-parse HEAD)\" &&\n+\tgrep \"$rev\" actual\n+'\n+\n test_expect_success 'set up pathological context' '\n \tgit reset --hard &&\n \ttest_write_lines a a a a a a a a a a a >a &&\n-- \ngitgitgadget\n"},{"id":"461920","messageId":"xmqq7d2x5nnb.fsf@gitster.g","threadId":"58353","inReplyTo":"34qs5607-001o-0n6o-rson-9587q8r909qn@tzk.qr","subject":"Re: [PATCH 0/3] built-in add -p: support diff-so-fancy better","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-24T21:37:12Z","receivedAt":"2022-08-24T21:37:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> One unfortunate consequence of avoiding the `git diff` invocations of\n> `git-add--interactive.perl` that are not actually necessary is that I\n> missed that one of those invocations specifically ignores dirty\n> submodules, and that the Perl script then only processes files whose\n> numstat shows up in _that_ diff output.\n>\n> Fortunately, the fix is easy: simply ignore dirty submodules in _all_ `git\n> diff` invocations of `add -p`.\n\nOK, so it wasn't removing what is useless, but removing something\nwhose result was used. And our next move is not to bring us closer\nto how we used to correctly handle third-party diff filters by\nmimicking the original better but doing the handling of this\nparticular piece differently.\n\n> I will submit the next iteration as soon as the PR builds pass.\n\nOK.\n\n>> As with any big rewrite, a complete re-implementation always has risks\n>> to introduce unintended regressions and that is why starting with\n>> faithful rewrite with the staged transition plan is valuable.\n>>\n>> In this case, the staged transition plan, the step to flip the\n>> default without before remove the old one, is working beautifully.\n>> And it was only made possible with the actual users reporting issues\n>> before they say \"the new one is broken, so let's use the knob to\n>> retain the old implementation\".\n>\n> Indeed, and thank you, Philippe, for bringing this to the Git mailing list\n> so that I could do something about the bug.\n\nYup.\n\nTo some people, \"refatoring to match my subjective code aesthetics\"\nmay be personally the most important, and to some other people,\n\"getting rid of scripted Porcelains\" may be, but they are both wrong\nand these are their second most important goals ;-) Not regressing\nend-user experience is, and we learned a valuable lesson here. Be\nconservative and make sure we can revert to buy time when making a\nbig rewrite like this one.\n\nThanks, all.\n"},{"id":"461923","messageId":"xmqq35dl5m1z.fsf@gitster.g","threadId":"58353","inReplyTo":"pull.1336.v2.git.1661376112.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/4] built-in add -p: support diff-so-fancy better","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-24T22:11:36Z","receivedAt":"2022-08-24T22:11:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> Philippe Blain reported in\n> https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com\n> that there is a problem when running the built-in version of git add -p with\n> diff-so-fancy [https://github.com/so-fancy/diff-so-fancy] as diff colorizer.\n> The symptom is this:\n>\n> error: could not parse colored hunk header '?[36m?[1m?[38;5;13m@ file:1 @?[1m?[0m'\n>\n>\n> This patch series addresses that and should fix\n> https://github.com/so-fancy/diff-so-fancy/issues/437\n>\n> Changes since v1:\n>\n> * Added a commit to ignore dirty submodules just like the Perl version\n> does.\n\nThanks, all. Will queue, but it may miss this afternoon's\nintegration I am preparing right now for pushing out.\n\n"},{"id":"461929","messageId":"241d3760-5b7a-fb3e-00dc-bf54bda92c82@gmail.com","threadId":"58353","inReplyTo":"pull.1336.v2.git.1661376112.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/4] built-in add -p: support diff-so-fancy better","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2022-08-25T00:18:13Z","receivedAt":"2022-08-25T00:18:18Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Dscho,\n\nLe 2022-08-24 à 17:21, Johannes Schindelin via GitGitGadget a écrit :\n> Philippe Blain reported in\n> https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com\n> that there is a problem when running the built-in version of git add -p with\n> diff-so-fancy [https://github.com/so-fancy/diff-so-fancy] as diff colorizer.\n> The symptom is this:\n> \n> error: could not parse colored hunk header '?[36m?[1m?[38;5;13m@ file:1 @?[1m?[0m'\n> \n> \n> This patch series addresses that and should fix\n> https://github.com/so-fancy/diff-so-fancy/issues/437\n> \n> Changes since v1:\n> \n> * Added a commit to ignore dirty submodules just like the Perl version\n> does.\n> \n> Johannes Schindelin (4):\n> t3701: redefine what is \"bogus\" output of a diff filter\n> add -p: gracefully ignore unparseable hunk headers in colored diffs\n> add -p: handle `diff-so-fancy`'s hunk headers better\n> add -p: ignore dirty submodules\n> \n> add-patch.c | 24 ++++++++++++++----------\n> t/t3701-add-interactive.sh | 24 +++++++++++++++++++++++-\n> 2 files changed, 37 insertions(+), 11 deletions(-)\n> \n> \n> base-commit: 795ea8776befc95ea2becd8020c7a284677b4161\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1336%2Fdscho%2Fdiff-so-fancy-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1336/dscho/diff-so-fancy-v2\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1336\n> \n> Range-diff vs v1:\n> \n> 1: 74ab50eeb1c = 1: 74ab50eeb1c t3701: redefine what is \"bogus\" output of a diff filter\n> 2: b07f85a0359 = 2: b07f85a0359 add -p: gracefully ignore unparseable hunk headers in colored diffs\n> 3: 9dac9f74d2e = 3: 9dac9f74d2e add -p: handle `diff-so-fancy`'s hunk headers better\n> -: ----------- > 4: 540ce27c38a add -p: ignore dirty submodules\n> \n\nThanks, 4/4 fixes the mismatched output bug. Just after I sent my last email, \nI asked myself \"but why does 'git add -p' presents dirty submodule to the user,\nas they can't be staged?\" :) \n\nA small question about 2/4, any reason why you did not use a \"Reported-by:\" \ntrailer ? Not that I care that much, but I think using such a trailer is more\nstandard, and makes for easier statistics as it's more parseable :)\n\nThanks again for the quick fixes,\n\nPhilippe.\n"},{"id":"461992","messageId":"818s0453-677s-sos7-24r2-qq66o4r81r4q@tzk.qr","threadId":"58353","inReplyTo":"241d3760-5b7a-fb3e-00dc-bf54bda92c82@gmail.com","subject":"Re: [PATCH v2 0/4] built-in add -p: support diff-so-fancy better","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-26T11:43:57Z","receivedAt":"2022-08-26T11:44:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Philippe,\n\nOn Wed, 24 Aug 2022, Philippe Blain wrote:\n\n> Le 2022-08-24 à 17:21, Johannes Schindelin via GitGitGadget a écrit :\n> > Philippe Blain reported in\n> > https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com\n> > that there is a problem when running the built-in version of git add -p with\n> > diff-so-fancy [https://github.com/so-fancy/diff-so-fancy] as diff colorizer.\n> > The symptom is this:\n> >\n> > error: could not parse colored hunk header '?[36m?[1m?[38;5;13m@ file:1 @?[1m?[0m'\n> >\n> >\n> > This patch series addresses that and should fix\n> > https://github.com/so-fancy/diff-so-fancy/issues/437\n> >\n> > Changes since v1:\n> >\n> > * Added a commit to ignore dirty submodules just like the Perl version\n> > does.\n> >\n> > Johannes Schindelin (4):\n> > t3701: redefine what is \"bogus\" output of a diff filter\n> > add -p: gracefully ignore unparseable hunk headers in colored diffs\n> > add -p: handle `diff-so-fancy`'s hunk headers better\n> > add -p: ignore dirty submodules\n> >\n> > add-patch.c | 24 ++++++++++++++----------\n> > t/t3701-add-interactive.sh | 24 +++++++++++++++++++++++-\n> > 2 files changed, 37 insertions(+), 11 deletions(-)\n> >\n> >\n> > base-commit: 795ea8776befc95ea2becd8020c7a284677b4161\n> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1336%2Fdscho%2Fdiff-so-fancy-v2\n> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1336/dscho/diff-so-fancy-v2\n> > Pull-Request: https://github.com/gitgitgadget/git/pull/1336\n> >\n> > Range-diff vs v1:\n> >\n> > 1: 74ab50eeb1c = 1: 74ab50eeb1c t3701: redefine what is \"bogus\" output of a diff filter\n> > 2: b07f85a0359 = 2: b07f85a0359 add -p: gracefully ignore unparseable hunk headers in colored diffs\n> > 3: 9dac9f74d2e = 3: 9dac9f74d2e add -p: handle `diff-so-fancy`'s hunk headers better\n> > -: ----------- > 4: 540ce27c38a add -p: ignore dirty submodules\n> >\n>\n> Thanks, 4/4 fixes the mismatched output bug. Just after I sent my last email,\n> I asked myself \"but why does 'git add -p' presents dirty submodule to the user,\n> as they can't be staged?\" :)\n>\n> A small question about 2/4, any reason why you did not use a \"Reported-by:\"\n> trailer ? Not that I care that much, but I think using such a trailer is more\n> standard, and makes for easier statistics as it's more parseable :)\n\nGood suggestion.\n\nHow about adding your review? I'll then add a \"Reviewed-by:\" trailer, too\n;-)\n\nCiao,\nDscho\n"},{"id":"462019","messageId":"a9a6c73c-65d5-795e-6830-a543c2513941@gmail.com","threadId":"58353","inReplyTo":"818s0453-677s-sos7-24r2-qq66o4r81r4q@tzk.qr","subject":"Re: [PATCH v2 0/4] built-in add -p: support diff-so-fancy better","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2022-08-26T23:15:40Z","receivedAt":"2022-08-26T23:15:47Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Dscho,\n\nLe 2022-08-26 à 07:43, Johannes Schindelin a écrit :\n> Hi Philippe,\n> \n> On Wed, 24 Aug 2022, Philippe Blain wrote:\n> \n>> Le 2022-08-24 à 17:21, Johannes Schindelin via GitGitGadget a écrit :\n>>> Philippe Blain reported in\n>>> https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com\n>>> that there is a problem when running the built-in version of git add -p with\n>>> diff-so-fancy [https://github.com/so-fancy/diff-so-fancy] as diff colorizer.\n>>> The symptom is this:\n>>>\n>>> error: could not parse colored hunk header '?[36m?[1m?[38;5;13m@ file:1 @?[1m?[0m'\n>>>\n>>>\n>>> This patch series addresses that and should fix\n>>> https://github.com/so-fancy/diff-so-fancy/issues/437\n>>>\n>>> Changes since v1:\n>>>\n>>> * Added a commit to ignore dirty submodules just like the Perl version\n>>> does.\n>>>\n>>> Johannes Schindelin (4):\n>>> t3701: redefine what is \"bogus\" output of a diff filter\n>>> add -p: gracefully ignore unparseable hunk headers in colored diffs\n>>> add -p: handle `diff-so-fancy`'s hunk headers better\n>>> add -p: ignore dirty submodules\n>>>\n>>> add-patch.c | 24 ++++++++++++++----------\n>>> t/t3701-add-interactive.sh | 24 +++++++++++++++++++++++-\n>>> 2 files changed, 37 insertions(+), 11 deletions(-)\n>>>\n>>>\n>>> base-commit: 795ea8776befc95ea2becd8020c7a284677b4161\n>>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1336%2Fdscho%2Fdiff-so-fancy-v2\n>>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1336/dscho/diff-so-fancy-v2\n>>> Pull-Request: https://github.com/gitgitgadget/git/pull/1336\n>>>\n>>> Range-diff vs v1:\n>>>\n>>> 1: 74ab50eeb1c = 1: 74ab50eeb1c t3701: redefine what is \"bogus\" output of a diff filter\n>>> 2: b07f85a0359 = 2: b07f85a0359 add -p: gracefully ignore unparseable hunk headers in colored diffs\n>>> 3: 9dac9f74d2e = 3: 9dac9f74d2e add -p: handle `diff-so-fancy`'s hunk headers better\n>>> -: ----------- > 4: 540ce27c38a add -p: ignore dirty submodules\n>>>\n>>\n>> Thanks, 4/4 fixes the mismatched output bug. Just after I sent my last email,\n>> I asked myself \"but why does 'git add -p' presents dirty submodule to the user,\n>> as they can't be staged?\" :)\n>>\n>> A small question about 2/4, any reason why you did not use a \"Reported-by:\"\n>> trailer ? Not that I care that much, but I think using such a trailer is more\n>> standard, and makes for easier statistics as it's more parseable :)\n> \n> Good suggestion.\n> \n> How about adding your review? I'll then add a \"Reviewed-by:\" trailer, too\n> ;-)\n\nI won't have time to look at the code changes themselves, and I've never looked \nat the builtin add -p code (nor the original Perl one!) so it would take some time.\n\nCheers,\nPhilippe.\n"},{"id":"462073","messageId":"xmqqbks3qy8o.fsf@gitster.g","threadId":"58353","inReplyTo":"b07f85a035954bb9145a0fe034e6f17aeb128f52.1661376112.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/4] add -p: gracefully ignore unparseable hunk headers in colored diffs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-29T07:56:23Z","receivedAt":"2022-08-29T07:57:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> diff --git a/add-patch.c b/add-patch.c\n> index 509ca04456b..f2fffe1af02 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -357,16 +357,13 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)\n> \teol = memchr(line, '\\n', s->colored.len - hunk->colored_start);\n> \tif (!eol)\n> \t\teol = s->colored.buf + s->colored.len;\n> -\tp = memmem(line, eol - line, \"@@ -\", 4);\n> -\tif (!p)\n> -\t\treturn error(_(\"could not parse colored hunk header '%.*s'\"),\n> -\t\t\t (int)(eol - line), line);\n> -\tp = memmem(p + 4, eol - p - 4, \" @@\", 3);\n> -\tif (!p)\n> -\t\treturn error(_(\"could not parse colored hunk header '%.*s'\"),\n> -\t\t\t (int)(eol - line), line);\n> \thunk->colored_start = eol - s->colored.buf + (*eol == '\\n');\n> -\theader->colored_extra_start = p + 3 - s->colored.buf;\n> +\tp = memmem(line, eol - line, \"@@ -\", 4);\n> +\tif (p && (p = memmem(p + 4, eol - p - 4, \" @@\", 3)))\n> +\t\theader->colored_extra_start = p + 3 - s->colored.buf;\n> +\telse\n> +\t\t/* could not parse colored hunk header, showing nothing */\n> +\t\theader->colored_extra_start = hunk->colored_start;\n> \theader->colored_extra_end = hunk->colored_start;\n\nOK. We keep two copies s->plain and s->colored of the line in the\npatch, and the real information on the hunk header has already been\nparsed out of the \"plain\" version.\n\nHere, w want to find the end of \"@@ -, + @@\"\" on the\ncolored variant, presumably because that is where the \"funcname\"\nmay appear and more useful pieces of information can be gleaned.\n\nThe original code assumed that we should be able to find such a\nplace (after all, we successfully parsed the \"corresponding\" plain\nversion to find it) and threw an error otherwise. Otherwise,\n\n * hunk->colored_start is set to the beginning of the next line.\n\n * header->colored_extra_start is set to the byte after the \" @@\",\n i.e. where the \"funcname\" ought to begin.\n\n * header->colored_extra_end is set to the beginning of the next\n line (i.e. \"funcname\" is the remainder of the line starting at\n \"colored_extra_start\").\n\nThe new code loosens the requirement that colored one has the same\nhunk header on the line (presumably prefixed by some color codes).\nWhen the colored line does not look like a hunk header and we cannot\ntell where the \"funcname\" part begins, we can pretend that the entire\nline was just the hunk header without \"funcname\" part by setting the\nextra_start and extra_end both at the end of the line.\n\nThere is nothing \"structurally\" important on the \"funcname\" part, so\npresumably we will only use the information to show to the display\nand nothing else, so I think this is perfectly OK to do.\n\nAnother obvious possibility is to show everything on that\nunparseable line. We know it corresponds to the hunk header in the\n\"real\" patch (i.e. s->plain), and it may have something human can\nunderstand. Perhaps that may be changed in the later part of this\nseries (I haven't looked, but if that happens \"showing nothing\"\ncomment needs to be updated).\n\nSo far, looking good.\n\n"},{"id":"462074","messageId":"xmqq5yibqxs0.fsf@gitster.g","threadId":"58353","inReplyTo":"9dac9f74d2e19899b3e6c1d28e83878ded4469d6.1661376112.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/4] add -p: handle `diff-so-fancy`'s hunk headers better","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-29T08:06:23Z","receivedAt":"2022-08-29T08:06:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> \telse\n> \t\t/* could not parse colored hunk header, showing nothing */\n> -\t\theader->colored_extra_start = hunk->colored_start;\n> +\t\theader->colored_extra_start = line - s->colored.buf;\n\nThis is the only thing that corresponds to the proposed log message.\nThe comment that says \"showing nothing\" is no longer correct, and\nneeds to be updated.\n\nEverything else in this patch is about adding an extra space\ndepending on what is in the \"funcname\" part. The code does not know\nor care if it is an attempt to do diff-so-fancy's headers better by\ndoing something we didn't do to the normal header we managed to have\nparsed; rather the extra space thing is done unconditionally and\ndoes not know or care if extra_start is truly after \" @@\" or a place\nin the line the new code computed.\n\nSo the following three hunks either need to be made into a separate\npatch, or deserves to be explained in a new paragraph in the\nproposed log message.\n\n> \theader->colored_extra_end = hunk->colored_start;\n> \n> \treturn 0;\n> @@ -649,6 +650,7 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n> \t\tsize_t len;\n> \t\tunsigned long old_offset = header->old_offset;\n> \t\tunsigned long new_offset = header->new_offset;\n> +\t\tint needs_extra_space = 0;\n> \n> \t\tif (!colored) {\n> \t\t\tp = s->plain.buf + header->extra_start;\n> @@ -658,6 +660,8 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n> \t\t\tp = s->colored.buf + header->colored_extra_start;\n> \t\t\tlen = header->colored_extra_end\n> \t\t\t\t- header->colored_extra_start;\n> +\t\t\tif (utf8_strnwidth(p, len, 1 /* skip ANSI */) > 0)\n> +\t\t\t\tneeds_extra_space = 1;\n> \t\t}\n> \n> \t\tif (s->mode->is_reverse)\n> @@ -673,6 +677,8 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n> \t\t\tstrbuf_addf(out, \",%lu\", header->new_count);\n> \t\tstrbuf_addstr(out, \" @@\");\n> \n> +\t\tif (needs_extra_space)\n> +\t\t\tstrbuf_addch(out, ' ');\n> \t\tif (len)\n> \t\t\tstrbuf_add(out, p, len);\n> \t\telse if (colored)\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index 7e3c1de71f5..9deb7a87f1e 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -772,7 +772,8 @@ test_expect_success 'gracefully fail to parse colored hunk header' '\n> \techo content >test &&\n> \ttest_config interactive.diffFilter \"sed s/@@/XX/g\" &&\n> \tprintf y >y &&\n> -\tforce_color git add -p +\tforce_color git add -p >output 2>&1 +\tgrep XX output\n> '\n\nIt is good to make sure that XX that was munged appears in the\noutput. This however does not check anything about the\nneeds-extra-space logic. It should. If the extra-space logic is\nmoved to a separate patch, then this step can have the above test,\nbut then the separate patch needs to update it to check the\nadditional logic.\n\nOther than that, looking very good.\n"},{"id":"462088","messageId":"0q0psp09-8993-96r6-3r90-q4s368p98510@tzk.qr","threadId":"58353","inReplyTo":"xmqq5yibqxs0.fsf@gitster.g","subject":"Re: [PATCH v2 3/4] add -p: handle `diff-so-fancy`'s hunk headers better","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-29T13:32:24Z","receivedAt":"2022-08-29T13:32:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 29 Aug 2022, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > \telse\n> > \t\t/* could not parse colored hunk header, showing nothing */\n> > -\t\theader->colored_extra_start = hunk->colored_start;\n> > +\t\theader->colored_extra_start = line - s->colored.buf;\n>\n> This is the only thing that corresponds to the proposed log message.\n> The comment that says \"showing nothing\" is no longer correct, and\n> needs to be updated.\n\nCorrect.\n\n> Everything else in this patch is about adding an extra space\n> depending on what is in the \"funcname\" part.\n\n... because that logic was not relevant before this commit.\n\n> The code does not know or care if it is an attempt to do diff-so-fancy's\n> headers better by doing something we didn't do to the normal header we\n> managed to have parsed; rather the extra space thing is done\n> unconditionally and does not know or care if extra_start is truly after\n> \" @@\" or a place in the line the new code computed.\n>\n> So the following three hunks either need to be made into a separate\n> patch, or deserves to be explained in a new paragraph in the\n> proposed log message.\n\nI've opted to split the changes out into their own patch because it\nimproves the reviewability of the patch series.\n\n> > \theader->colored_extra_end = hunk->colored_start;\n> >\n> > \treturn 0;\n> > @@ -649,6 +650,7 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n> > \t\tsize_t len;\n> > \t\tunsigned long old_offset = header->old_offset;\n> > \t\tunsigned long new_offset = header->new_offset;\n> > +\t\tint needs_extra_space = 0;\n> >\n> > \t\tif (!colored) {\n> > \t\t\tp = s->plain.buf + header->extra_start;\n> > @@ -658,6 +660,8 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n> > \t\t\tp = s->colored.buf + header->colored_extra_start;\n> > \t\t\tlen = header->colored_extra_end\n> > \t\t\t\t- header->colored_extra_start;\n> > +\t\t\tif (utf8_strnwidth(p, len, 1 /* skip ANSI */) > 0)\n> > +\t\t\t\tneeds_extra_space = 1;\n\nLet me add a review of my own: This hunk is incorrect ;-)\n\nHere is why: in the _regular_ case, i.e. when we have a colored hunk\nheader like `@@ -936 +936 @@ wow()`, we manage to parse the line range,\nand then record the offset of the extra part that starts afterwards.\n\nThis extra part is non-empty, therefore we add an extra space.\n\nBut that part already starts with a space, so now we end up with two\nspaces.\n\nA fix for this will be part of the next iteration.\n\nCiao,\nDscho\n\n> > \t\t}\n> >\n> > \t\tif (s->mode->is_reverse)\n> > @@ -673,6 +677,8 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n> > \t\t\tstrbuf_addf(out, \",%lu\", header->new_count);\n> > \t\tstrbuf_addstr(out, \" @@\");\n> >\n> > +\t\tif (needs_extra_space)\n> > +\t\t\tstrbuf_addch(out, ' ');\n> > \t\tif (len)\n> > \t\t\tstrbuf_add(out, p, len);\n> > \t\telse if (colored)\n> > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> > index 7e3c1de71f5..9deb7a87f1e 100755\n> > --- a/t/t3701-add-interactive.sh\n> > +++ b/t/t3701-add-interactive.sh\n> > @@ -772,7 +772,8 @@ test_expect_success 'gracefully fail to parse colored hunk header' '\n> > \techo content >test &&\n> > \ttest_config interactive.diffFilter \"sed s/@@/XX/g\" &&\n> > \tprintf y >y &&\n> > -\tforce_color git add -p > +\tforce_color git add -p >output 2>&1 > +\tgrep XX output\n> > '\n>\n> It is good to make sure that XX that was munged appears in the\n> output. This however does not check anything about the\n> needs-extra-space logic. It should. If the extra-space logic is\n> moved to a separate patch, then this step can have the above test,\n> but then the separate patch needs to update it to check the\n> additional logic.\n>\n> Other than that, looking very good.\n>\n"},{"id":"462089","messageId":"pull.1336.v3.git.1661785916.gitgitgadget@gmail.com","threadId":"58353","inReplyTo":"pull.1336.v2.git.1661376112.gitgitgadget@gmail.com","subject":"[PATCH v3 0/5] built-in add -p: support diff-so-fancy better","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-29T15:11:51Z","receivedAt":"2022-08-29T15:12:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Philippe Blain reported in\nhttps://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com\nthat there is a problem when running the built-in version of git add -p with\ndiff-so-fancy [https://github.com/so-fancy/diff-so-fancy] as diff colorizer.\nThe symptom is this:\n\n error: could not parse colored hunk header '?[36m?[1m?[38;5;13m@ file:1 @?[1m?[0m'\n\n\nThis patch series addresses that and should fix\nhttps://github.com/so-fancy/diff-so-fancy/issues/437\n\nChanges since v2:\n\n * Added the appropriate \"Reported-by\" trailer to the commit message.\n * Split out the logic to insert a space between the colored line range and\n the extra information, if needed.\n * That logic was now corrected to see whether that space is really needed.\n * To verify that the logic does what we need it to do, the added regression\n test now specifically tests for that (single) extra space that we want to\n be inserted.\n * Reworded a stale comment that claimed that we might suppress the entire\n colored hunk header (which we no longer do).\n * Rebased to the current tip of the main branch to avoid a merge conflict\n with 716c1f649e3 (pipe_command(): mark stdin descriptor as non-blocking,\n 2022-08-17).\n\nChanges since v1:\n\n * Added a commit to ignore dirty submodules just like the Perl version\n does.\n\nJohannes Schindelin (5):\n t3701: redefine what is \"bogus\" output of a diff filter\n add -p: gracefully ignore unparseable hunk headers in colored diffs\n add -p: insert space in colored hunk header as needed\n add -p: handle `diff-so-fancy`'s hunk headers better\n add -p: ignore dirty submodules\n\n add-patch.c | 49 ++++++++++++++++++++++++++++++--------\n t/t3701-add-interactive.sh | 35 ++++++++++++++++++++++++++-\n 2 files changed, 73 insertions(+), 11 deletions(-)\n\n\nbase-commit: 07ee72db0e97b5c233f8ada0abb412248c2f1c6f\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1336%2Fdscho%2Fdiff-so-fancy-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1336/dscho/diff-so-fancy-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1336\n\nRange-diff vs v2:\n\n 1: 74ab50eeb1c = 1: a01fa5d25e4 t3701: redefine what is \"bogus\" output of a diff filter\n 2: b07f85a0359 ! 2: cbe833bd141 add -p: gracefully ignore unparseable hunk headers in colored diffs\n @@ Commit message\n \n [diff-so-fancy]: https://github.com/so-fancy/diff-so-fancy\n \n + Reported-by: Philippe Blain \n Signed-off-by: Johannes Schindelin \n \n ## add-patch.c ##\n @@ t/t3701-add-interactive.sh: test_expect_success 'detect bogus diffFilter output'\n \tforce_color test_must_fail git add -p test &&\n -+\ttest_config interactive.diffFilter \"sed s/@@/XX/g\" &&\n -+\tprintf y >y &&\n -+\tforce_color git add -p n &&\n ++\tforce_color git -c interactive.diffFilter=\"sed s/@@/XX/g\" \\\n ++\t\tadd -p \n \n ## Commit message ##\n - add -p: handle `diff-so-fancy`'s hunk headers better\n + add -p: insert space in colored hunk header as needed\n \n - The `diff-so-fancy` diff colorizer produces hunk headers that look\n - nothing like the built-in `add -p` expects: there is no `@@ ... @@` line\n - range, and therefore the parser cannot determine where any extra\n - information starts, such as the function name that is often added to\n - those hunk header lines.\n + We are about to teach `git add -p` to show the entire hunk header if the\n + `@@ ... @@` line range cannot be parsed. Previously, we showed only the\n + remainder of that hunk header as an \"colored_extra\" part.\n \n - However, we can do better than simply swallowing the unparseable hunk\n - header. In the `diff-so-fancy` case, it shows something like `@ file:1\n - @`. Let's just show the complete hunk header because it probably offers\n - useful information.\n + To prepare for that, detect if that \"colored_extra\" part starts with any\n + non-whitespace character (ignoring ANSI escape sequences) and insert a\n + space, to make the output much more pleasant.\n +\n + Note that this has an effect already before we make `git add -p` more\n + lenient when parsing the hunk headers: diff filters could already remove\n + the space after the line range, which is precisely what we do in the\n + regression test introduced by this commit.\n \n Signed-off-by: Johannes Schindelin \n \n @@ add-patch.c\n \n enum prompt_mode_type {\n \tPROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_ADDITION, PROMPT_HUNK,\n -@@ add-patch.c: static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)\n - \t\theader->colored_extra_start = p + 3 - s->colored.buf;\n - \telse\n - \t\t/* could not parse colored hunk header, showing nothing */\n --\t\theader->colored_extra_start = hunk->colored_start;\n -+\t\theader->colored_extra_start = line - s->colored.buf;\n - \theader->colored_extra_end = hunk->colored_start;\n +@@ add-patch.c: static size_t find_next_line(struct strbuf *sb, size_t offset)\n + \treturn eol - sb->buf + 1;\n + }\n \n - \treturn 0;\n ++static int starts_with_non_ws(const char *p, size_t len)\n ++{\n ++\tfor (;;) {\n ++\t\tsize_t skip;\n ++\n ++\t\tif (!len || isspace(*p))\n ++\t\t\treturn 0;\n ++\t\tskip = display_mode_esc_sequence_len(p);\n ++\t\tif (!skip)\n ++\t\t\treturn 1;\n ++\t\tif (skip > len)\n ++\t\t\treturn 0;\n ++\t\tp += skip;\n ++\t\tlen -= skip;\n ++\t}\n ++}\n ++\n + static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n + \t\t\tssize_t delta, int colored, struct strbuf *out)\n + {\n @@ add-patch.c: static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n \t\tsize_t len;\n \t\tunsigned long old_offset = header->old_offset;\n @@ add-patch.c: static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n \t\t\tp = s->colored.buf + header->colored_extra_start;\n \t\t\tlen = header->colored_extra_end\n \t\t\t\t- header->colored_extra_start;\n -+\t\t\tif (utf8_strnwidth(p, len, 1 /* skip ANSI */) > 0)\n -+\t\t\t\tneeds_extra_space = 1;\n ++\t\t\tneeds_extra_space = starts_with_non_ws(p, len);\n \t\t}\n \n \t\tif (s->mode->is_reverse)\n @@ add-patch.c: static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n \t\telse if (colored)\n \n ## t/t3701-add-interactive.sh ##\n -@@ t/t3701-add-interactive.sh: test_expect_success 'gracefully fail to parse colored hunk header' '\n +@@ t/t3701-add-interactive.sh: test_expect_success 'handle iffy colored hunk headers' '\n \techo content >test &&\n - \ttest_config interactive.diffFilter \"sed s/@@/XX/g\" &&\n - \tprintf y >y &&\n --\tforce_color git add -p output 2>&1 n &&\n + \tforce_color git -c interactive.diffFilter=\"sed s/@@/XX/g\" \\\n +-\t\tadd -p output 2>&1 4: e3e3a178f98 add -p: handle `diff-so-fancy`'s hunk headers better\n 4: 540ce27c38a = 5: cfa6914aee0 add -p: ignore dirty submodules\n\n-- \ngitgitgadget\n"},{"id":"462090","messageId":"a01fa5d25e4a94dd8ece5e328f853c000a2ad0f9.1661785916.git.gitgitgadget@gmail.com","threadId":"58353","inReplyTo":"pull.1336.v3.git.1661785916.gitgitgadget@gmail.com","subject":"[PATCH v3 1/5] t3701: redefine what is \"bogus\" output of a diff filter","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-29T15:11:52Z","receivedAt":"2022-08-29T15:12:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen parsing the colored version of a diff, the interactive `add`\ncommand really relies on the colored version having the same number of\nlines as the non-colored version. That is an invariant.\n\nHowever, in the 'detect bogus diffFilter output' test case in t3701, we\nessentially required a hunk header that contains parseable `@@ ... @@`\nhunk headers, and called all colored diffs without such hunks bogus.\n\nThe reason for this is that we would like to show the users the adjusted\nhunk headers _including_ the extra part after the `@@ ... @@`\ninformation, which usually contains things like the function name or\nsoms such.\n\nNow, there is a _very_ popular diff colorizer called `diff-so-fancy`\nthat does not produce such colored diffs as the built-in `add` command\nexpects. Nevertheless, the Perl variant of the `add` command handles\nthose nicely, essentially by ignoring the hunk header and saying \"there\nis nothing else we can show except the original hunk header, even if we\nhad to adjust the line range and the original hunk header might get that\nwrong\".\n\nIn preparation for teaching the built-in interactive `add` to be a bit\nmore lenient, let's change the 'detect bogus diffFilter output' test\ncase so that it verifies that a mismatched number of lines causes the\ncommand to error out, but not an unparseable hunk header.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t3701-add-interactive.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 3b7df9bed5a..88d8133f38f 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -761,7 +761,7 @@ test_expect_success 'detect bogus diffFilter output' '\n \tgit reset --hard &&\n \n \techo content >test &&\n-\ttest_config interactive.diffFilter \"sed 1d\" &&\n+\ttest_config interactive.diffFilter \"sed q\" &&\n \tprintf y >y &&\n \tforce_color test_must_fail git add -p \n\nIn\nhttps://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com,\nPhillipe Blain reported that the built-in `git add -p` command fails\nwhen asked to use [`diff-so-fancy`][diff-so-fancy] to colorize the diff.\n\nThe reason is that this tool produces colored diffs with a hunk header\nthat does not contain any parseable `@@ ... @@` line range information,\nand therefore we cannot detect any part in that header that comes after\nthe line range.\n\nLet's punt for now and simply show nothing apart from the line range in\nthat case.\n\n[diff-so-fancy]: https://github.com/so-fancy/diff-so-fancy\n\nReported-by: Philippe Blain \nSigned-off-by: Johannes Schindelin \n---\n add-patch.c | 15 ++++++---------\n t/t3701-add-interactive.sh | 9 +++++++++\n 2 files changed, 15 insertions(+), 9 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 509ca04456b..f2fffe1af02 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -357,16 +357,13 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)\n \teol = memchr(line, '\\n', s->colored.len - hunk->colored_start);\n \tif (!eol)\n \t\teol = s->colored.buf + s->colored.len;\n-\tp = memmem(line, eol - line, \"@@ -\", 4);\n-\tif (!p)\n-\t\treturn error(_(\"could not parse colored hunk header '%.*s'\"),\n-\t\t\t (int)(eol - line), line);\n-\tp = memmem(p + 4, eol - p - 4, \" @@\", 3);\n-\tif (!p)\n-\t\treturn error(_(\"could not parse colored hunk header '%.*s'\"),\n-\t\t\t (int)(eol - line), line);\n \thunk->colored_start = eol - s->colored.buf + (*eol == '\\n');\n-\theader->colored_extra_start = p + 3 - s->colored.buf;\n+\tp = memmem(line, eol - line, \"@@ -\", 4);\n+\tif (p && (p = memmem(p + 4, eol - p - 4, \" @@\", 3)))\n+\t\theader->colored_extra_start = p + 3 - s->colored.buf;\n+\telse\n+\t\t/* could not parse colored hunk header, showing nothing */\n+\t\theader->colored_extra_start = hunk->colored_start;\n \theader->colored_extra_end = hunk->colored_start;\n \n \treturn 0;\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 88d8133f38f..c2187f9cec8 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -766,6 +766,15 @@ test_expect_success 'detect bogus diffFilter output' '\n \tforce_color test_must_fail git add -p test &&\n+\tprintf n >n &&\n+\tforce_color git -c interactive.diffFilter=\"sed s/@@/XX/g\" \\\n+\t\tadd -p \n\nThe `diff-so-fancy` diff colorizer produces hunk headers that look\nnothing like what the built-in `add -p` expects: there is no `@@ ... @@`\nline range, and therefore the parser cannot determine where any extra\ninformation starts (such as the function name that is often added to\nthose hunk header lines).\n\nHowever, we can do better than simply swallowing the unparseable hunk\nheader. There is probably information the user wants to see, after all.\nIn the `diff-so-fancy` case, it shows something like `@ file:1 @`.\n\nIf the line range could not be found in the colored hunk header, let's\njust show the complete hunk header.\n\nSigned-off-by: Johannes Schindelin \n---\n add-patch.c | 13 +++++++++++--\n t/t3701-add-interactive.sh | 4 ++++\n 2 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 9d575d30ed0..0217cdd7c4a 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -363,8 +363,17 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)\n \tif (p && (p = memmem(p + 4, eol - p - 4, \" @@\", 3)))\n \t\theader->colored_extra_start = p + 3 - s->colored.buf;\n \telse\n-\t\t/* could not parse colored hunk header, showing nothing */\n-\t\theader->colored_extra_start = hunk->colored_start;\n+\t\t/*\n+\t\t * We tried to parse the line range out of the colored hunk\n+\t\t * header, so that we could show just the extra information\n+\t\t * after the line range.\n+\t\t *\n+\t\t * At this point, we did not find that line range, but the hunk\n+\t\t * header likely has information that the user might find\n+\t\t * interesting. Let's just show the entire hunk header instead\n+\t\t * in that case.\n+\t\t */\n+\t\theader->colored_extra_start = line - s->colored.buf;\n \theader->colored_extra_end = hunk->colored_start;\n \n \treturn 0;\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 49200b7df68..39e68b6d066 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -775,10 +775,14 @@ test_expect_success 'handle iffy colored hunk headers' '\n \t\tadd -p output 2>&1 output-so-fancy 2>&1 \n\nWe are about to teach `git add -p` to show the entire hunk header if the\n`@@ ... @@` line range cannot be parsed. Previously, we showed only the\nremainder of that hunk header as an \"colored_extra\" part.\n\nTo prepare for that, detect if that \"colored_extra\" part starts with any\nnon-whitespace character (ignoring ANSI escape sequences) and insert a\nspace, to make the output much more pleasant.\n\nNote that this has an effect already before we make `git add -p` more\nlenient when parsing the hunk headers: diff filters could already remove\nthe space after the line range, which is precisely what we do in the\nregression test introduced by this commit.\n\nSigned-off-by: Johannes Schindelin \n---\n add-patch.c | 22 ++++++++++++++++++++++\n t/t3701-add-interactive.sh | 10 +++++++++-\n 2 files changed, 31 insertions(+), 1 deletion(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex f2fffe1af02..9d575d30ed0 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -8,6 +8,7 @@\n #include \"diff.h\"\n #include \"compat/terminal.h\"\n #include \"prompt.h\"\n+#include \"utf8.h\"\n \n enum prompt_mode_type {\n \tPROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_ADDITION, PROMPT_HUNK,\n@@ -635,6 +636,23 @@ static size_t find_next_line(struct strbuf *sb, size_t offset)\n \treturn eol - sb->buf + 1;\n }\n \n+static int starts_with_non_ws(const char *p, size_t len)\n+{\n+\tfor (;;) {\n+\t\tsize_t skip;\n+\n+\t\tif (!len || isspace(*p))\n+\t\t\treturn 0;\n+\t\tskip = display_mode_esc_sequence_len(p);\n+\t\tif (!skip)\n+\t\t\treturn 1;\n+\t\tif (skip > len)\n+\t\t\treturn 0;\n+\t\tp += skip;\n+\t\tlen -= skip;\n+\t}\n+}\n+\n static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n \t\t\tssize_t delta, int colored, struct strbuf *out)\n {\n@@ -649,6 +667,7 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n \t\tsize_t len;\n \t\tunsigned long old_offset = header->old_offset;\n \t\tunsigned long new_offset = header->new_offset;\n+\t\tint needs_extra_space = 0;\n \n \t\tif (!colored) {\n \t\t\tp = s->plain.buf + header->extra_start;\n@@ -658,6 +677,7 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n \t\t\tp = s->colored.buf + header->colored_extra_start;\n \t\t\tlen = header->colored_extra_end\n \t\t\t\t- header->colored_extra_start;\n+\t\t\tneeds_extra_space = starts_with_non_ws(p, len);\n \t\t}\n \n \t\tif (s->mode->is_reverse)\n@@ -673,6 +693,8 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n \t\t\tstrbuf_addf(out, \",%lu\", header->new_count);\n \t\tstrbuf_addstr(out, \" @@\");\n \n+\t\tif (needs_extra_space)\n+\t\t\tstrbuf_addch(out, ' ');\n \t\tif (len)\n \t\t\tstrbuf_add(out, p, len);\n \t\telse if (colored)\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex c2187f9cec8..49200b7df68 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -772,7 +772,15 @@ test_expect_success 'handle iffy colored hunk headers' '\n \techo content >test &&\n \tprintf n >n &&\n \tforce_color git -c interactive.diffFilter=\"sed s/@@/XX/g\" \\\n-\t\tadd -p output 2>&1 \n\nThanks to alwyas running `diff-index` and `diff-files` with the\n`--numstat` option (the latter with `--ignore-submodules=dirty`) before\neven generating any real diff to parse, the Perl version of `git add -p`\nsimply ignored dirty submodules and does not even offer them up for\nstaging.\n\nHowever, the built-in variant did not use that flag because it tries to\nrun only one `diff` command, skipping the unneeded\n`diff-index`/`diff-files` invocation of the Perl variant and therefore\nonly faithfully recapitulates what the Perl code does once it _does_\ngenerate and parse the real diff.\n\nThis causes a problem when running the built-in `add -p` with\n`diff-so-fancy` because that diff colorizer always inserts an empty line\nbefore the diff header to ensure that it produces 4 lines as expected by\n`git add -p` (the equivalent of the non-colorized `diff`, `index`, `---`\nand `+++` lines). But `git diff-files` does not produce any `index` line\nfor dirty submodules.\n\nThe underlying problem is not even the discrepancy in lines, but that\n`git add -p` presents diffs for dirty submodules: there is nothing that\n_can_ be staged for those.\n\nLet's fix that bug, and teach the built-in `add -p` to ignore dirty\nsubmodules, too. This _incidentally_ also fixes the `diff-so-fancy`\nproblem ;-)\n\nSigned-off-by: Johannes Schindelin \n---\n add-patch.c | 3 ++-\n t/t3701-add-interactive.sh | 12 ++++++++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 0217cdd7c4a..ee6a3d3b712 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -426,7 +426,8 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n \t}\n \tcolor_arg_index = args.nr;\n \t/* Use `--no-color` explicitly, just in case `diff.color = always`. */\n-\tstrvec_pushl(&args, \"--no-color\", \"-p\", \"--\", NULL);\n+\tstrvec_pushl(&args, \"--no-color\", \"--ignore-submodules=dirty\", \"-p\",\n+\t\t \"--\", NULL);\n \tfor (i = 0; i < ps->nr; i++)\n \t\tstrvec_push(&args, ps->items[i].original);\n \ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 39e68b6d066..a4f45fc48a0 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -965,6 +965,18 @@ test_expect_success 'status ignores dirty submodules (except HEAD)' '\n \t! grep dirty-otherwise output\n '\n \n+test_expect_success 'handle submodules' '\n+\techo 123 >>for-submodules/dirty-otherwise/initial.t &&\n+\n+\tforce_color git -C for-submodules add -p dirty-otherwise >output 2>&1 &&\n+\tgrep \"No changes\" output &&\n+\n+\tforce_color git -C for-submodules add -p dirty-head >output 2>&1 actual &&\n+\trev=\"$(git -C for-submodules/dirty-head rev-parse HEAD)\" &&\n+\tgrep \"$rev\" actual\n+'\n+\n test_expect_success 'set up pathological context' '\n \tgit reset --hard &&\n \ttest_write_lines a a a a a a a a a a a >a &&\n-- \ngitgitgadget\n"},{"id":"462099","messageId":"xmqqilmbotl0.fsf@gitster.g","threadId":"58353","inReplyTo":"0q0psp09-8993-96r6-3r90-q4s368p98510@tzk.qr","subject":"Re: [PATCH v2 3/4] add -p: handle `diff-so-fancy`'s hunk headers better","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-29T17:19:55Z","receivedAt":"2022-08-29T17:20:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Here is why: in the _regular_ case, i.e. when we have a colored hunk\n> header like `@@ -936 +936 @@ wow()`, we manage to parse the line range,\n> and then record the offset of the extra part that starts afterwards.\n>\n> This extra part is non-empty, therefore we add an extra space.\n>\n> But that part already starts with a space, so now we end up with two\n> spaces.\n\nIn other words, this breaks because the original code depended on\nhaving the extra whitespace before the \"funcname\" part.\n\nStepping back a bit, if the final goal for the UI generation out of\nthis string is to append the material with a whitespace before it\nfor better visual separation, then the original should probably have\n(at least conceptually) lstrip'ed the leading whitespaces from the\nstring it found after \" @@\" and then appended the result to where it\nis showing, with its own single whitespace as a prefix. It would\nhave prevented this bug from happening by future-proofing, and made\nthe final output nicer, as any excess whitespaces between the \" @@\"\nand \"funcname\" would have been turned into a single SP.\n\nThe \"prepend one iff it does not already begin with a whitespace\" is\na (at least mentally to the developer) less expensive approach that\nis fine, too.\n\nThanks.\n"},{"id":"462104","messageId":"xmqq5yibnd3q.fsf@gitster.g","threadId":"58353","inReplyTo":"pull.1336.v3.git.1661785916.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/5] built-in add -p: support diff-so-fancy better","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-29T18:01:13Z","receivedAt":"2022-08-29T18:01:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> * That logic was now corrected to see whether that space is really needed.\n> * To verify that the logic does what we need it to do, the added regression\n> test now specifically tests for that (single) extra space that we want to\n> be inserted.\n\n\n> * Rebased to the current tip of the main branch to avoid a merge conflict\n> with 716c1f649e3 (pipe_command(): mark stdin descriptor as non-blocking,\n> 2022-08-17).\n\nI'd prefer you (or any other contributor) not to pick a more recent\nbase only to work around a merge conflict, especially when it is\nsomething simple (like this case, where both sides added a new test\npiece each in a way that semantically do not conflict). I'll ask\nthe author to rebase if the conflicts becomes unmanageable.\n\nWe promoted the built-in one as the primary implementation in\n2.37.0; I think we want to keep this fix mergeable down to the\n2.37.x maintenance track (either by me, or by distro LTS folks who\nare motivated enough).\n\nThis time, as you can guess by my above description on the exact way\nhow conflict happens, I've rebased the series back for 2.37.x and\nwill resolve the conflict myself (and make sure the result matches\nthe application of these patches directly on top of 'master').\n\nIt appears that this round is good to go down to 'next' soonish, but\nlet's see what others find.\n\nThanks.\n"},{"id":"462185","messageId":"9261de42-3287-6ccb-6cf5-21c0a8ee1f17@gmail.com","threadId":"58353","inReplyTo":"a01fa5d25e4a94dd8ece5e328f853c000a2ad0f9.1661785916.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/5] t3701: redefine what is \"bogus\" output of a diff filter","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-08-30T13:17:49Z","receivedAt":"2022-08-30T13:17:55Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 29/08/2022 16:11, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> When parsing the colored version of a diff, the interactive `add`\n> command really relies on the colored version having the same number of\n> lines as the non-colored version. That is an invariant.\n> \n> However, in the 'detect bogus diffFilter output' test case in t3701, we\n> essentially required a hunk header that contains parseable `@@ ... @@`\n> hunk headers, and called all colored diffs without such hunks bogus.\n> \n> The reason for this is that we would like to show the users the adjusted\n> hunk headers _including_ the extra part after the `@@ ... @@`\n> information, which usually contains things like the function name or\n> soms such.\n> \n> Now, there is a _very_ popular diff colorizer called `diff-so-fancy`\n> that does not produce such colored diffs as the built-in `add` command\n> expects. Nevertheless, the Perl variant of the `add` command handles\n> those nicely, essentially by ignoring the hunk header and saying \"there\n> is nothing else we can show except the original hunk header, even if we\n> had to adjust the line range and the original hunk header might get that\n> wrong\".\n> \n> In preparation for teaching the built-in interactive `add` to be a bit\n> more lenient, let's change the 'detect bogus diffFilter output' test\n> case so that it verifies that a mismatched number of lines causes the\n> command to error out, but not an unparseable hunk header.\n\nThe existing test deletes the first line of the diff[1] - so it is \nremoving the \"diff --git ...\" header not the hunk header. This patch \nchanges the filter to delete everything except the diff header which \nseems like a less realistic test.\n\nBest Wishes\n\nPhillip\n\n[1] To verify this I changed the filter to \"sed 1d | tee \nfiltered-diff\". filtered diff-contains\nindex 0889435..d95f3ad 100644\n\n--- a/test\n\n+++ b/test\n\n@@ -1,6 +1 @@\n\n-10\n\n-20\n\n-30\n\n-40\n\n-50\n\n-60\n\n+content\n\n\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> t/t3701-add-interactive.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index 3b7df9bed5a..88d8133f38f 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -761,7 +761,7 @@ test_expect_success 'detect bogus diffFilter output' '\n> \tgit reset --hard &&\n> \n> \techo content >test &&\n> -\ttest_config interactive.diffFilter \"sed 1d\" &&\n> +\ttest_config interactive.diffFilter \"sed q\" &&\n> \tprintf y >y &&\n> \tforce_color test_must_fail git add -p '\n"},{"id":"462186","messageId":"4960a9a9-3a48-f6ce-6412-0e6eb8997e1d@gmail.com","threadId":"58353","inReplyTo":"e3e3a178f98183032b3df8ad9c81a096fe4af556.1661785916.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 4/5] add -p: handle `diff-so-fancy`'s hunk headers better","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-08-30T13:23:07Z","receivedAt":"2022-08-30T13:23:13Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 29/08/2022 16:11, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> The `diff-so-fancy` diff colorizer produces hunk headers that look\n> nothing like what the built-in `add -p` expects: there is no `@@ ... @@`\n> line range, and therefore the parser cannot determine where any extra\n> information starts (such as the function name that is often added to\n> those hunk header lines).\n> \n> However, we can do better than simply swallowing the unparseable hunk\n> header. There is probably information the user wants to see, after all.\n> In the `diff-so-fancy` case, it shows something like `@ file:1 @`.\n> \n> If the line range could not be found in the colored hunk header, let's\n> just show the complete hunk header.\n\nLooking at the tests, I don't think we just show the complete hunk \nheader, we show the offsets from the unfiltered diff as well. I think \nthat is unfortunate as it kind of defeats the purpose of \ninteractive.diffFilter which is to the the user see the diff how they \nwant to (so long as it has the same number of lines)\n\nBest Wishes\n\nPhillip\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> add-patch.c | 13 +++++++++++--\n> t/t3701-add-interactive.sh | 4 ++++\n> 2 files changed, 15 insertions(+), 2 deletions(-)\n> \n> diff --git a/add-patch.c b/add-patch.c\n> index 9d575d30ed0..0217cdd7c4a 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -363,8 +363,17 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)\n> \tif (p && (p = memmem(p + 4, eol - p - 4, \" @@\", 3)))\n> \t\theader->colored_extra_start = p + 3 - s->colored.buf;\n> \telse\n> -\t\t/* could not parse colored hunk header, showing nothing */\n> -\t\theader->colored_extra_start = hunk->colored_start;\n> +\t\t/*\n> +\t\t * We tried to parse the line range out of the colored hunk\n> +\t\t * header, so that we could show just the extra information\n> +\t\t * after the line range.\n> +\t\t *\n> +\t\t * At this point, we did not find that line range, but the hunk\n> +\t\t * header likely has information that the user might find\n> +\t\t * interesting. Let's just show the entire hunk header instead\n> +\t\t * in that case.\n> +\t\t */\n> +\t\theader->colored_extra_start = line - s->colored.buf;\n> \theader->colored_extra_end = hunk->colored_start;\n> \n> \treturn 0;\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index 49200b7df68..39e68b6d066 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -775,10 +775,14 @@ test_expect_success 'handle iffy colored hunk headers' '\n> \t\tadd -p \tforce_color git -c interactive.diffFilter=\"sed \\\"s/\\(.*@@\\).*/\\1FN/\\\"\" \\\n> \t\tadd -p >output 2>&1 +\tforce_color git -c interactive.diffFilter=\"sed \\\"s/\\(.*@@\\).*/file/\\\"\" \\\n> +\t\tadd -p >output-so-fancy 2>&1 \tif test_have_prereq ADD_I_USE_BUILTIN\n> \tthen\n> +\t\tgrep \"@ file\\$\" output-so-fancy &&\n> \t\tgrep \"@ FN\\$\" output\n> \telse\n> +\t\tgrep \"^file\\$\" output-so-fancy &&\n> \t\tgrep \"@FN\\$\" output\n> \tfi\n> '\n"},{"id":"462187","messageId":"c205f8e2-62f9-d4b2-0ccf-51dcabc12224@gmail.com","threadId":"58353","inReplyTo":"cfa6914aee0d3ef84d726b97699f438fd4b254d9.1661785916.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 5/5] add -p: ignore dirty submodules","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-08-30T13:26:28Z","receivedAt":"2022-08-30T13:27:13Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 29/08/2022 16:11, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> Thanks to alwyas running `diff-index` and `diff-files` with the\n> `--numstat` option (the latter with `--ignore-submodules=dirty`) before\n> even generating any real diff to parse, the Perl version of `git add -p`\n> simply ignored dirty submodules and does not even offer them up for\n> staging.\n\nI had a bit of a hard time understanding this paragraph. To me the fact \nthat the perl version is using --numstat is not that important here, \nwhat is important is that it is using --ignore-submodules=dirty when it \ngenerates its list of files to show and that information is consigned to \na parenthesized aside.\n\nThe fix itself looks good.\n\nBest Wishes\n\nPhillip\n\n> However, the built-in variant did not use that flag because it tries to\n> run only one `diff` command, skipping the unneeded\n> `diff-index`/`diff-files` invocation of the Perl variant and therefore\n> only faithfully recapitulates what the Perl code does once it _does_\n> generate and parse the real diff.\n> \n> This causes a problem when running the built-in `add -p` with\n> `diff-so-fancy` because that diff colorizer always inserts an empty line\n> before the diff header to ensure that it produces 4 lines as expected by\n> `git add -p` (the equivalent of the non-colorized `diff`, `index`, `---`\n> and `+++` lines). But `git diff-files` does not produce any `index` line\n> for dirty submodules.\n> \n> The underlying problem is not even the discrepancy in lines, but that\n> `git add -p` presents diffs for dirty submodules: there is nothing that\n> _can_ be staged for those.\n> \n> Let's fix that bug, and teach the built-in `add -p` to ignore dirty\n> submodules, too. This _incidentally_ also fixes the `diff-so-fancy`\n> problem ;-)\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> add-patch.c | 3 ++-\n> t/t3701-add-interactive.sh | 12 ++++++++++++\n> 2 files changed, 14 insertions(+), 1 deletion(-)\n> \n> diff --git a/add-patch.c b/add-patch.c\n> index 0217cdd7c4a..ee6a3d3b712 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -426,7 +426,8 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n> \t}\n> \tcolor_arg_index = args.nr;\n> \t/* Use `--no-color` explicitly, just in case `diff.color = always`. */\n> -\tstrvec_pushl(&args, \"--no-color\", \"-p\", \"--\", NULL);\n> +\tstrvec_pushl(&args, \"--no-color\", \"--ignore-submodules=dirty\", \"-p\",\n> +\t\t \"--\", NULL);\n> \tfor (i = 0; i < ps->nr; i++)\n> \t\tstrvec_push(&args, ps->items[i].original);\n> \n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index 39e68b6d066..a4f45fc48a0 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -965,6 +965,18 @@ test_expect_success 'status ignores dirty submodules (except HEAD)' '\n> \t! grep dirty-otherwise output\n> '\n> \n> +test_expect_success 'handle submodules' '\n> +\techo 123 >>for-submodules/dirty-otherwise/initial.t &&\n> +\n> +\tforce_color git -C for-submodules add -p dirty-otherwise >output 2>&1 &&\n> +\tgrep \"No changes\" output &&\n> +\n> +\tforce_color git -C for-submodules add -p dirty-head >output 2>&1 +\tgit -C for-submodules ls-files --stage dirty-head >actual &&\n> +\trev=\"$(git -C for-submodules/dirty-head rev-parse HEAD)\" &&\n> +\tgrep \"$rev\" actual\n> +'\n> +\n> test_expect_success 'set up pathological context' '\n> \tgit reset --hard &&\n> \ttest_write_lines a a a a a a a a a a a >a &&\n"},{"id":"462189","messageId":"1d4bf456-6fc0-b675-f0ee-f8ce99bd38b2@gmail.com","threadId":"58353","inReplyTo":"pull.1336.v3.git.1661785916.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/5] built-in add -p: support diff-so-fancy better","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-08-30T13:29:31Z","receivedAt":"2022-08-30T13:29:38Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nThanks for working on this, I've left a few comments on the patches - \nI'm not convinced about patch 1 or about showing the hunk offsets with \ninteractive.diffFilter in patch 4 but the series looks basically sound.\n\nBest Wishes\n\nPhillip\n\nOn 29/08/2022 16:11, Johannes Schindelin via GitGitGadget wrote:\n> Philippe Blain reported in\n> https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com\n> that there is a problem when running the built-in version of git add -p with\n> diff-so-fancy [https://github.com/so-fancy/diff-so-fancy] as diff colorizer.\n> The symptom is this:\n> \n> error: could not parse colored hunk header '?[36m?[1m?[38;5;13m@ file:1 @?[1m?[0m'\n> \n> \n> This patch series addresses that and should fix\n> https://github.com/so-fancy/diff-so-fancy/issues/437\n> \n> Changes since v2:\n> \n> * Added the appropriate \"Reported-by\" trailer to the commit message.\n> * Split out the logic to insert a space between the colored line range and\n> the extra information, if needed.\n> * That logic was now corrected to see whether that space is really needed.\n> * To verify that the logic does what we need it to do, the added regression\n> test now specifically tests for that (single) extra space that we want to\n> be inserted.\n> * Reworded a stale comment that claimed that we might suppress the entire\n> colored hunk header (which we no longer do).\n> * Rebased to the current tip of the main branch to avoid a merge conflict\n> with 716c1f649e3 (pipe_command(): mark stdin descriptor as non-blocking,\n> 2022-08-17).\n> \n> Changes since v1:\n> \n> * Added a commit to ignore dirty submodules just like the Perl version\n> does.\n> \n> Johannes Schindelin (5):\n> t3701: redefine what is \"bogus\" output of a diff filter\n> add -p: gracefully ignore unparseable hunk headers in colored diffs\n> add -p: insert space in colored hunk header as needed\n> add -p: handle `diff-so-fancy`'s hunk headers better\n> add -p: ignore dirty submodules\n> \n> add-patch.c | 49 ++++++++++++++++++++++++++++++--------\n> t/t3701-add-interactive.sh | 35 ++++++++++++++++++++++++++-\n> 2 files changed, 73 insertions(+), 11 deletions(-)\n> \n> \n> base-commit: 07ee72db0e97b5c233f8ada0abb412248c2f1c6f\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1336%2Fdscho%2Fdiff-so-fancy-v3\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1336/dscho/diff-so-fancy-v3\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1336\n> \n> Range-diff vs v2:\n> \n> 1: 74ab50eeb1c = 1: a01fa5d25e4 t3701: redefine what is \"bogus\" output of a diff filter\n> 2: b07f85a0359 ! 2: cbe833bd141 add -p: gracefully ignore unparseable hunk headers in colored diffs\n> @@ Commit message\n> \n> [diff-so-fancy]: https://github.com/so-fancy/diff-so-fancy\n> \n> + Reported-by: Philippe Blain \n> Signed-off-by: Johannes Schindelin \n> \n> ## add-patch.c ##\n> @@ t/t3701-add-interactive.sh: test_expect_success 'detect bogus diffFilter output'\n> \tforce_color test_must_fail git add -p '\n> \n> -+test_expect_success 'gracefully fail to parse colored hunk header' '\n> ++test_expect_success 'handle iffy colored hunk headers' '\n> +\tgit reset --hard &&\n> +\n> +\techo content >test &&\n> -+\ttest_config interactive.diffFilter \"sed s/@@/XX/g\" &&\n> -+\tprintf y >y &&\n> -+\tforce_color git add -p ++\tprintf n >n &&\n> ++\tforce_color git -c interactive.diffFilter=\"sed s/@@/XX/g\" \\\n> ++\t\tadd -p +'\n> +\n> - test_expect_success 'diff.algorithm is passed to `git diff-files`' '\n> + test_expect_success 'handle very large filtered diff' '\n> \tgit reset --hard &&\n> -\n> + \t# The specific number here is not important, but it must\n> 3: 9dac9f74d2e ! 3: 7a9f0b107e6 add -p: handle `diff-so-fancy`'s hunk headers better\n> @@ Metadata\n> Author: Johannes Schindelin \n> \n> ## Commit message ##\n> - add -p: handle `diff-so-fancy`'s hunk headers better\n> + add -p: insert space in colored hunk header as needed\n> \n> - The `diff-so-fancy` diff colorizer produces hunk headers that look\n> - nothing like the built-in `add -p` expects: there is no `@@ ... @@` line\n> - range, and therefore the parser cannot determine where any extra\n> - information starts, such as the function name that is often added to\n> - those hunk header lines.\n> + We are about to teach `git add -p` to show the entire hunk header if the\n> + `@@ ... @@` line range cannot be parsed. Previously, we showed only the\n> + remainder of that hunk header as an \"colored_extra\" part.\n> \n> - However, we can do better than simply swallowing the unparseable hunk\n> - header. In the `diff-so-fancy` case, it shows something like `@ file:1\n> - @`. Let's just show the complete hunk header because it probably offers\n> - useful information.\n> + To prepare for that, detect if that \"colored_extra\" part starts with any\n> + non-whitespace character (ignoring ANSI escape sequences) and insert a\n> + space, to make the output much more pleasant.\n> +\n> + Note that this has an effect already before we make `git add -p` more\n> + lenient when parsing the hunk headers: diff filters could already remove\n> + the space after the line range, which is precisely what we do in the\n> + regression test introduced by this commit.\n> \n> Signed-off-by: Johannes Schindelin \n> \n> @@ add-patch.c\n> \n> enum prompt_mode_type {\n> \tPROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_ADDITION, PROMPT_HUNK,\n> -@@ add-patch.c: static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)\n> - \t\theader->colored_extra_start = p + 3 - s->colored.buf;\n> - \telse\n> - \t\t/* could not parse colored hunk header, showing nothing */\n> --\t\theader->colored_extra_start = hunk->colored_start;\n> -+\t\theader->colored_extra_start = line - s->colored.buf;\n> - \theader->colored_extra_end = hunk->colored_start;\n> +@@ add-patch.c: static size_t find_next_line(struct strbuf *sb, size_t offset)\n> + \treturn eol - sb->buf + 1;\n> + }\n> \n> - \treturn 0;\n> ++static int starts_with_non_ws(const char *p, size_t len)\n> ++{\n> ++\tfor (;;) {\n> ++\t\tsize_t skip;\n> ++\n> ++\t\tif (!len || isspace(*p))\n> ++\t\t\treturn 0;\n> ++\t\tskip = display_mode_esc_sequence_len(p);\n> ++\t\tif (!skip)\n> ++\t\t\treturn 1;\n> ++\t\tif (skip > len)\n> ++\t\t\treturn 0;\n> ++\t\tp += skip;\n> ++\t\tlen -= skip;\n> ++\t}\n> ++}\n> ++\n> + static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n> + \t\t\tssize_t delta, int colored, struct strbuf *out)\n> + {\n> @@ add-patch.c: static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n> \t\tsize_t len;\n> \t\tunsigned long old_offset = header->old_offset;\n> @@ add-patch.c: static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n> \t\t\tp = s->colored.buf + header->colored_extra_start;\n> \t\t\tlen = header->colored_extra_end\n> \t\t\t\t- header->colored_extra_start;\n> -+\t\t\tif (utf8_strnwidth(p, len, 1 /* skip ANSI */) > 0)\n> -+\t\t\t\tneeds_extra_space = 1;\n> ++\t\t\tneeds_extra_space = starts_with_non_ws(p, len);\n> \t\t}\n> \n> \t\tif (s->mode->is_reverse)\n> @@ add-patch.c: static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n> \t\telse if (colored)\n> \n> ## t/t3701-add-interactive.sh ##\n> -@@ t/t3701-add-interactive.sh: test_expect_success 'gracefully fail to parse colored hunk header' '\n> +@@ t/t3701-add-interactive.sh: test_expect_success 'handle iffy colored hunk headers' '\n> \techo content >test &&\n> - \ttest_config interactive.diffFilter \"sed s/@@/XX/g\" &&\n> - \tprintf y >y &&\n> --\tforce_color git add -p -+\tforce_color git add -p >output 2>&1 -+\tgrep XX output\n> + \tprintf n >n &&\n> + \tforce_color git -c interactive.diffFilter=\"sed s/@@/XX/g\" \\\n> +-\t\tadd -p ++\t\tadd -p ++\tforce_color git -c interactive.diffFilter=\"sed \\\"s/\\(.*@@\\).*/\\1FN/\\\"\" \\\n> ++\t\tadd -p >output 2>&1 ++\tif test_have_prereq ADD_I_USE_BUILTIN\n> ++\tthen\n> ++\t\tgrep \"@ FN\\$\" output\n> ++\telse\n> ++\t\tgrep \"@FN\\$\" output\n> ++\tfi\n> '\n> \n> - test_expect_success 'diff.algorithm is passed to `git diff-files`' '\n> + test_expect_success 'handle very large filtered diff' '\n> -: ----------- > 4: e3e3a178f98 add -p: handle `diff-so-fancy`'s hunk headers better\n> 4: 540ce27c38a = 5: cfa6914aee0 add -p: ignore dirty submodules\n> \n"},{"id":"462197","messageId":"p11s25nn-15qr-o2q1-5700-1q581sr9331n@tzk.qr","threadId":"58353","inReplyTo":"xmqqilmbotl0.fsf@gitster.g","subject":"Re: [PATCH v2 3/4] add -p: handle `diff-so-fancy`'s hunk headers better","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-30T14:14:37Z","receivedAt":"2022-08-30T14:14:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 29 Aug 2022, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> > Here is why: in the _regular_ case, i.e. when we have a colored hunk\n> > header like `@@ -936 +936 @@ wow()`, we manage to parse the line range,\n> > and then record the offset of the extra part that starts afterwards.\n> >\n> > This extra part is non-empty, therefore we add an extra space.\n> >\n> > But that part already starts with a space, so now we end up with two\n> > spaces.\n>\n> In other words, this breaks because the original code depended on\n> having the extra whitespace before the \"funcname\" part.\n\nYes.\n\n> Stepping back a bit, if the final goal for the UI generation out of\n> this string is to append the material with a whitespace before it\n> for better visual separation, then the original should probably have\n> (at least conceptually) lstrip'ed the leading whitespaces from the\n> string it found after \" @@\" and then appended the result to where it\n> is showing, with its own single whitespace as a prefix.\n\nYes, with one twist: ANSI escape sequences can make lstrip'ing non-trivial\n(granted, the line range parser totally ignores the fact that `@@ `\nis a totally legitimate end of a colored line range).\n\n> It would have prevented this bug from happening by future-proofing, and\n> made the final output nicer, as any excess whitespaces between the \" @@\"\n> and \"funcname\" would have been turned into a single SP.\n>\n> The \"prepend one iff it does not already begin with a whitespace\" is\n> a (at least mentally to the developer) less expensive approach that\n> is fine, too.\n\nYes, it is definitely less expensive.\n\nCiao,\nDscho\n"},{"id":"462200","messageId":"p1r4p079-p699-6053-14p5-4r4spr5s6059@tzk.qr","threadId":"58353","inReplyTo":"xmqq5yibnd3q.fsf@gitster.g","subject":"Re: [PATCH v3 0/5] built-in add -p: support diff-so-fancy better","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-30T14:22:04Z","receivedAt":"2022-08-30T14:22:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 29 Aug 2022, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > * That logic was now corrected to see whether that space is really needed.\n> > * To verify that the logic does what we need it to do, the added regression\n> > test now specifically tests for that (single) extra space that we want to\n> > be inserted.\n>\n>\n> > * Rebased to the current tip of the main branch to avoid a merge conflict\n> > with 716c1f649e3 (pipe_command(): mark stdin descriptor as non-blocking,\n> > 2022-08-17).\n>\n> I'd prefer you (or any other contributor) not to pick a more recent\n> base only to work around a merge conflict, [...]\n\nNoted. I just wanted to be nice.\n\n> We promoted the built-in one as the primary implementation in\n> 2.37.0; I think we want to keep this fix mergeable down to the\n> 2.37.x maintenance track (either by me, or by distro LTS folks who\n> are motivated enough).\n>\n> This time, as you can guess by my above description on the exact way\n> how conflict happens, I've rebased the series back for 2.37.x and\n> will resolve the conflict myself (and make sure the result matches\n> the application of these patches directly on top of 'master').\n>\n> It appears that this round is good to go down to 'next' soonish, but\n> let's see what others find.\n\nSounds good.\n\nThanks,\nDscho\n"},{"id":"462246","messageId":"xmqq7d2pifby.fsf@gitster.g","threadId":"58353","inReplyTo":"9261de42-3287-6ccb-6cf5-21c0a8ee1f17@gmail.com","subject":"Re: [PATCH v3 1/5] t3701: redefine what is \"bogus\" output of a diff filter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-30T21:36:33Z","receivedAt":"2022-08-30T21:36:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> The existing test deletes the first line of the diff[1] - so it is\n> removing the \"diff --git ...\" header not the hunk header. This patch\n> changes the filter to delete everything except the diff header which\n> seems like a less realistic test.\n\nIs it that all it cares about is that the output has the same number\nof lines as the input? If so, I agree that it is much less realistic,\nbut it may not matter in practice. Even an \"exit 0\" might do ;-)\n\nI may be quite off the mark on this one, though.\n\n"},{"id":"462272","messageId":"742b42af-a298-219d-a35f-1fa8ba985aed@gmail.com","threadId":"58353","inReplyTo":"xmqq7d2pifby.fsf@gitster.g","subject":"Re: [PATCH v3 1/5] t3701: redefine what is \"bogus\" output of a diff filter","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-08-31T09:26:17Z","receivedAt":"2022-08-31T09:26:48Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 30/08/2022 22:36, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>> The existing test deletes the first line of the diff[1] - so it is\n>> removing the \"diff --git ...\" header not the hunk header. This patch\n>> changes the filter to delete everything except the diff header which\n>> seems like a less realistic test.\n> \n> Is it that all it cares about is that the output has the same number\n> of lines as the input? If so, I agree that it is much less realistic,\n> but it may not matter in practice. Even an \"exit 0\" might do ;-)\n\nYes I think it only cares that there are the same number of lines which \nbegs the question \"why are we changing this test in the first place?\". \nThe commit message talks about the being unable to parse the hunk header \nbut that's only true because we would be looking in the wrong place for \nit - the output does in fact contain a valid hunk header. With this \nchange there is no hunk header in the filtered output at all. In \npractice if the number of lines differs it is normally because the \nfilter messed with the diff header and removed some lines from it which \nis what the existing test simulates.\n\nI'm struggling to understand the need for this change from the \nexplanation in the commit message as it seems to me to assume the line \nbeing deleted is the hunk header when in fact it is deleting the diff \nheader.\n\nBest Wishes\n\nPhillip\n\n> I may be quite off the mark on this one, though.\n> \n"},{"id":"462279","messageId":"Yw9/81QFPsSjDUTM@coredump.intra.peff.net","threadId":"58353","inReplyTo":"742b42af-a298-219d-a35f-1fa8ba985aed@gmail.com","subject":"Re: [PATCH v3 1/5] t3701: redefine what is \"bogus\" output of a diff filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-31T15:36:19Z","receivedAt":"2022-08-31T15:36:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 31, 2022 at 10:26:17AM +0100, Phillip Wood wrote:\n\n> > Is it that all it cares about is that the output has the same number\n> > of lines as the input? If so, I agree that it is much less realistic,\n> > but it may not matter in practice. Even an \"exit 0\" might do ;-)\n> \n> Yes I think it only cares that there are the same number of lines which begs\n> the question \"why are we changing this test in the first place?\". The commit\n> message talks about the being unable to parse the hunk header but that's\n> only true because we would be looking in the wrong place for it - the output\n> does in fact contain a valid hunk header. With this change there is no hunk\n> header in the filtered output at all. In practice if the number of lines\n> differs it is normally because the filter messed with the diff header and\n> removed some lines from it which is what the existing test simulates.\n> \n> I'm struggling to understand the need for this change from the explanation\n> in the commit message as it seems to me to assume the line being deleted is\n> the hunk header when in fact it is deleting the diff header.\n\nFWIW, as the author of the original test, I'm also confused about why it\nneeds to be changed. The filter I wrote in the original test was just\n\"echo too-short\". It was switched to \"sed 1d\" because the original did\nnot read the input at all, which racily caused Git to see SIGPIPE:\n\n https://lore.kernel.org/git/20200113170417.GK32750@szeder.dev/\n\nSwitching to \"exit 0\" would bring that problem back. But I think \"sed q\"\npotentially does, too, because sed will quit without reading all of the\ninput. We really do want something like \"sed 1d\" that changes the number\nof lines, but ensures that the filter reads to EOF.\n\n-Peff\n"},{"id":"462280","messageId":"Yw+Cm+0EuqIZvY66@coredump.intra.peff.net","threadId":"58353","inReplyTo":"Yw9/81QFPsSjDUTM@coredump.intra.peff.net","subject":"Re: [PATCH v3 1/5] t3701: redefine what is \"bogus\" output of a diff filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-31T15:47:39Z","receivedAt":"2022-08-31T15:49:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 31, 2022 at 11:36:19AM -0400, Jeff King wrote:\n\n> > I'm struggling to understand the need for this change from the explanation\n> > in the commit message as it seems to me to assume the line being deleted is\n> > the hunk header when in fact it is deleting the diff header.\n> \n> FWIW, as the author of the original test, I'm also confused about why it\n> needs to be changed. The filter I wrote in the original test was just\n> \"echo too-short\". It was switched to \"sed 1d\" because the original did\n> not read the input at all, which racily caused Git to see SIGPIPE:\n> \n> https://lore.kernel.org/git/20200113170417.GK32750@szeder.dev/\n> \n> Switching to \"exit 0\" would bring that problem back. But I think \"sed q\"\n> potentially does, too, because sed will quit without reading all of the\n> input. We really do want something like \"sed 1d\" that changes the number\n> of lines, but ensures that the filter reads to EOF.\n\nBy the way, the test change is needed for it to pass with the change in\npatch 2, where we become more lenient about parsing the hunk header.\nThat implies to me that the builtin version's check for one-to-one line\ncorrespondence is broken, but we didn't notice.\n\nIn the perl version, that test fails because it triggers the\nmismatched-output check:\n\n $ GIT_TEST_ADD_I_USE_BUILTIN=false ./t3701-add-interactive.sh --verbose-only=56\n [...]\n fatal: mismatched output from interactive.diffFilter\n hint: Your filter must maintain a one-to-one correspondence\n hint: between its input and output lines.\n ok 56 - detect bogus diffFilter output\n\nbut the builtin version complains about the hunk header:\n\n $ GIT_TEST_ADD_I_USE_BUILTIN=true ./t3701-add-interactive.sh --verbose-only=56\n [...]\n error: could not parse colored hunk header '?[31m-10?[m'\n ok 56 - detect bogus diffFilter output\n\nOnce patch 2 is applied, we stop complaining there, and we _should_\ncomplain that the number of lines isn't the same. But we don't:\n\n $ GIT_TEST_ADD_I_USE_BUILTIN=true ./t3701-add-interactive.sh --verbose-only=56\n test_must_fail: command succeeded: git add -p\n not ok 56 - detect bogus diffFilter output\n\n-Peff\n"},{"id":"462308","messageId":"r3nsqo6s-5531-r8rn-0434-315q4r0r88q9@tzk.qr","threadId":"58353","inReplyTo":"Yw+Cm+0EuqIZvY66@coredump.intra.peff.net","subject":"Re: [PATCH v3 1/5] t3701: redefine what is \"bogus\" output of a diff filter","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-31T19:57:47Z","receivedAt":"2022-08-31T19:58:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 31 Aug 2022, Jeff King wrote:\n\n> On Wed, Aug 31, 2022 at 11:36:19AM -0400, Jeff King wrote:\n>\n> > > I'm struggling to understand the need for this change from the explanation\n> > > in the commit message as it seems to me to assume the line being deleted is\n> > > the hunk header when in fact it is deleting the diff header.\n> >\n> > FWIW, as the author of the original test, I'm also confused about why it\n> > needs to be changed. The filter I wrote in the original test was just\n> > \"echo too-short\". It was switched to \"sed 1d\" because the original did\n> > not read the input at all, which racily caused Git to see SIGPIPE:\n> >\n> > https://lore.kernel.org/git/20200113170417.GK32750@szeder.dev/\n> >\n> > Switching to \"exit 0\" would bring that problem back. But I think \"sed q\"\n> > potentially does, too, because sed will quit without reading all of the\n> > input. We really do want something like \"sed 1d\" that changes the number\n> > of lines, but ensures that the filter reads to EOF.\n>\n> By the way, the test change is needed for it to pass with the change in\n> patch 2, where we become more lenient about parsing the hunk header.\n> That implies to me that the builtin version's check for one-to-one line\n> correspondence is broken, but we didn't notice.\n\nAnd that's exactly the case. It was an off-by-one bug ;-)\n\n> In the perl version, that test fails because it triggers the\n> mismatched-output check:\n>\n> $ GIT_TEST_ADD_I_USE_BUILTIN=false ./t3701-add-interactive.sh --verbose-only=56\n> [...]\n> fatal: mismatched output from interactive.diffFilter\n> hint: Your filter must maintain a one-to-one correspondence\n> hint: between its input and output lines.\n> ok 56 - detect bogus diffFilter output\n>\n> but the builtin version complains about the hunk header:\n>\n> $ GIT_TEST_ADD_I_USE_BUILTIN=true ./t3701-add-interactive.sh --verbose-only=56\n> [...]\n> error: could not parse colored hunk header '?[31m-10?[m'\n> ok 56 - detect bogus diffFilter output\n>\n> Once patch 2 is applied, we stop complaining there, and we _should_\n> complain that the number of lines isn't the same. But we don't:\n>\n> $ GIT_TEST_ADD_I_USE_BUILTIN=true ./t3701-add-interactive.sh --verbose-only=56\n> test_must_fail: command succeeded: git add -p\n> not ok 56 - detect bogus diffFilter output\n\nYes, that matches my analysis, and I already pushed a new iteration before\neven noticing your mail, only waiting for the CI run to finish before I\nsubmit it.\n\nCiao,\nDscho\n"},{"id":"462310","messageId":"op19530n-27qo-r35s-r1q7-18s3259ps78o@tzk.qr","threadId":"58353","inReplyTo":"c205f8e2-62f9-d4b2-0ccf-51dcabc12224@gmail.com","subject":"Re: [PATCH v3 5/5] add -p: ignore dirty submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-31T20:05:23Z","receivedAt":"2022-08-31T20:05:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Tue, 30 Aug 2022, Phillip Wood wrote:\n\n> On 29/08/2022 16:11, Johannes Schindelin via GitGitGadget wrote:\n> > From: Johannes Schindelin \n> >\n> > Thanks to alwyas running `diff-index` and `diff-files` with the\n> > `--numstat` option (the latter with `--ignore-submodules=dirty`) before\n> > even generating any real diff to parse, the Perl version of `git add -p`\n> > simply ignored dirty submodules and does not even offer them up for\n> > staging.\n>\n> I had a bit of a hard time understanding this paragraph. To me the fact that\n> the perl version is using --numstat is not that important here, what is\n> important is that it is using --ignore-submodules=dirty when it generates its\n> list of files to show and that information is consigned to a parenthesized\n> aside.\n\nYes, the `--ignore-submodules=dirty` part is important, but so is the\n`--numstat` part.\n\nIt is important because the Perl script would parse that numstat part and\nonly offer files up for further processing for which it saw those numbers.\nThen it would go ahead and run a full diff on those files, which basically\ndoubled the work.\n\nAnd it is that doubling of work that I tried to avoid when implementing\nthe built-in version. The bug came about because the full diff call wasn't\nusing the `--ignore-submodules=dirty` option, and that's what I missed.\n\nThis is maybe more interesting a story for the cover letter, to be able to\nunderstand how this bug was introduced, and maybe to offer an opportunity\nfor others (in addition to myself) to learn from my mistake.\n\n> The fix itself looks good.\n\nThanks!\nDscho\n"},{"id":"462313","messageId":"xmqqr10wdv4b.fsf@gitster.g","threadId":"58353","inReplyTo":"op19530n-27qo-r35s-r1q7-18s3259ps78o@tzk.qr","subject":"Re: [PATCH v3 5/5] add -p: ignore dirty submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-31T20:19:00Z","receivedAt":"2022-08-31T20:19:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> And it is that doubling of work that I tried to avoid when implementing\n> the built-in version. The bug came about because the full diff call wasn't\n> using the `--ignore-submodules=dirty` option, and that's what I missed.\n>\n> This is maybe more interesting a story for the cover letter, to be able to\n> understand how this bug was introduced, and maybe to offer an opportunity\n> for others (in addition to myself) to learn from my mistake.\n\nYup, the moral of the story is premature optimization bites because\nwe are not always careful ;-)\n\nAnyhow, I am getting an impression that the behaviour of the next\niteration would be much closer to the original, which is excellent.\n\nWe have seen too many \"ah, this is broken and here is a fix that is\nappropriate in the context of how the new C version does it\", not\n\"ok, let's make the whole thing behave more like what we used to\nhave\".\n\nThanks.\n"},{"id":"462314","messageId":"pull.1336.v4.git.1661977877.gitgitgadget@gmail.com","threadId":"58353","inReplyTo":"pull.1336.v3.git.1661785916.gitgitgadget@gmail.com","subject":"[PATCH v4 0/3] built-in add -p: support diff-so-fancy better","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-31T20:31:14Z","receivedAt":"2022-08-31T20:31:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Philippe Blain reported in\nhttps://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com\nthat there is a problem when running the built-in version of git add -p with\ndiff-so-fancy [https://github.com/so-fancy/diff-so-fancy] as diff colorizer.\nThe symptom is this:\n\n error: could not parse colored hunk header '?[36m?[1m?[38;5;13m@ file:1 @?[1m?[0m'\n\n\nThis patch series addresses that and should fix\nhttps://github.com/so-fancy/diff-so-fancy/issues/437\n\nChanges since v3:\n\n * Instead of deviating from how the Perl version of git add -p did things,\n we now teach the built-in version to display hunk headers verbatim when\n no line range could be parsed out (instead of showing the line range\n anyways). This was a very good idea of Phillip's, dramatically\n simplifying the patch series.\n * Also, this iteration drops the first patch that claims to redefine what\n we consider bogus, but only hides an off-by-one. In its stead, there is\n now a patch that fixes said off-by-one.\n\nChanges since v2:\n\n * Added the appropriate \"Reported-by\" trailer to the commit message.\n * Split out the logic to insert a space between the colored line range and\n the extra information, if needed.\n * That logic was now corrected to see whether that space is really needed.\n * To verify that the logic does what we need it to do, the added regression\n test now specifically tests for that (single) extra space that we want to\n be inserted.\n * Reworded a stale comment that claimed that we might suppress the entire\n colored hunk header (which we no longer do).\n * Rebased to the current tip of the main branch to avoid a merge conflict\n with 716c1f649e3 (pipe_command(): mark stdin descriptor as non-blocking,\n 2022-08-17).\n\nChanges since v1:\n\n * Added a commit to ignore dirty submodules just like the Perl version\n does.\n\nJohannes Schindelin (3):\n add -p: detect more mismatches between plain vs colored diffs\n add -p: gracefully handle unparseable hunk headers in colored diffs\n add -p: ignore dirty submodules\n\n add-patch.c | 50 +++++++++++++++++++++-----------------\n t/t3701-add-interactive.sh | 27 ++++++++++++++++++--\n 2 files changed, 53 insertions(+), 24 deletions(-)\n\n\nbase-commit: 07ee72db0e97b5c233f8ada0abb412248c2f1c6f\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1336%2Fdscho%2Fdiff-so-fancy-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1336/dscho/diff-so-fancy-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1336\n\nRange-diff vs v3:\n\n 1: a01fa5d25e4 ! 1: 25187c3a3c2 t3701: redefine what is \"bogus\" output of a diff filter\n @@ Metadata\n Author: Johannes Schindelin \n \n ## Commit message ##\n - t3701: redefine what is \"bogus\" output of a diff filter\n + add -p: detect more mismatches between plain vs colored diffs\n \n When parsing the colored version of a diff, the interactive `add`\n command really relies on the colored version having the same number of\n - lines as the non-colored version. That is an invariant.\n -\n - However, in the 'detect bogus diffFilter output' test case in t3701, we\n - essentially required a hunk header that contains parseable `@@ ... @@`\n - hunk headers, and called all colored diffs without such hunks bogus.\n -\n - The reason for this is that we would like to show the users the adjusted\n - hunk headers _including_ the extra part after the `@@ ... @@`\n - information, which usually contains things like the function name or\n - soms such.\n -\n - Now, there is a _very_ popular diff colorizer called `diff-so-fancy`\n - that does not produce such colored diffs as the built-in `add` command\n - expects. Nevertheless, the Perl variant of the `add` command handles\n - those nicely, essentially by ignoring the hunk header and saying \"there\n - is nothing else we can show except the original hunk header, even if we\n - had to adjust the line range and the original hunk header might get that\n - wrong\".\n -\n - In preparation for teaching the built-in interactive `add` to be a bit\n - more lenient, let's change the 'detect bogus diffFilter output' test\n - case so that it verifies that a mismatched number of lines causes the\n - command to error out, but not an unparseable hunk header.\n + lines as the plain (uncolored) version. That is an invariant.\n +\n + We already have code to verify correctly when the colored diff has less\n + lines than the plain diff. Modulo an off-by-one bug: If the last diff\n + line has no matching colored one, the code pretends to succeed, still.\n +\n + To make matters worse, when we adjusted the test in 1e4ffc765db (t3701:\n + adjust difffilter test, 2020-01-14), we did not catch this because `add\n + -p` fails for a _different_ reason: it does not find any colored hunk\n + header that contains a parseable line range.\n +\n + If we change the test case so that the line range _can_ be parsed, the\n + bug is exposed.\n +\n + Let's address all of the above by\n +\n + - fixing the off-by-one,\n +\n + - adjusting the test case to allow `add -p` to parse the line range\n +\n + - making the test case more stringent by verifying that the expected\n + error message is shown\n +\n + Also adjust a misleading code comment about the now-fixed code.\n \n Signed-off-by: Johannes Schindelin \n \n + ## add-patch.c ##\n +@@ add-patch.c: static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n + \t\t\tif (colored_eol)\n + \t\t\t\tcolored_p = colored_eol + 1;\n + \t\t\telse if (p != pend)\n +-\t\t\t\t/* colored shorter than non-colored? */\n ++\t\t\t\t/* non-colored has more lines? */\n ++\t\t\t\tgoto mismatched_output;\n ++\t\t\telse if (colored_p == colored_pend)\n ++\t\t\t\t/* last line has no matching colored one? */\n + \t\t\t\tgoto mismatched_output;\n + \t\t\telse\n + \t\t\t\tcolored_p = colored_pend;\n +\n ## t/t3701-add-interactive.sh ##\n @@ t/t3701-add-interactive.sh: test_expect_success 'detect bogus diffFilter output' '\n \tgit reset --hard &&\n \n \techo content >test &&\n -\ttest_config interactive.diffFilter \"sed 1d\" &&\n -+\ttest_config interactive.diffFilter \"sed q\" &&\n ++\ttest_config interactive.diffFilter \"sed 6d\" &&\n \tprintf y >y &&\n - \tforce_color test_must_fail git add -p output 2>&1 &&\n ++\tgrep \"mismatched output\" output\n '\n + \n + test_expect_success 'handle very large filtered diff' '\n 2: cbe833bd141 ! 2: cd1c5100506 add -p: gracefully ignore unparseable hunk headers in colored diffs\n @@ Metadata\n Author: Johannes Schindelin \n \n ## Commit message ##\n - add -p: gracefully ignore unparseable hunk headers in colored diffs\n + add -p: gracefully handle unparseable hunk headers in colored diffs\n \n In\n https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com,\n @@ Commit message\n and therefore we cannot detect any part in that header that comes after\n the line range.\n \n - Let's punt for now and simply show nothing apart from the line range in\n - that case.\n + As proposed by Phillip Wood, let's take that for a clear indicator that\n + we should show the hunk headers verbatim. This is what the Perl version\n + of the interactive `add` command did, too.\n +\n + This commit is best viewed with `--color-moved --ignore-space-change`.\n \n [diff-so-fancy]: https://github.com/so-fancy/diff-so-fancy\n \n Reported-by: Philippe Blain \n + Helped-by: Phillip Wood \n Signed-off-by: Johannes Schindelin \n \n ## add-patch.c ##\n +@@ add-patch.c: struct hunk_header {\n + \t * include the newline.\n + \t */\n + \tsize_t extra_start, extra_end, colored_extra_start, colored_extra_end;\n ++\tunsigned suppress_colored_line_range:1;\n + };\n + \n + struct hunk {\n @@ add-patch.c: static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)\n - \teol = memchr(line, '\\n', s->colored.len - hunk->colored_start);\n \tif (!eol)\n \t\teol = s->colored.buf + s->colored.len;\n --\tp = memmem(line, eol - line, \"@@ -\", 4);\n + \tp = memmem(line, eol - line, \"@@ -\", 4);\n -\tif (!p)\n -\t\treturn error(_(\"could not parse colored hunk header '%.*s'\"),\n -\t\t\t (int)(eol - line), line);\n @@ add-patch.c: static int parse_hunk_header(struct add_p_state *s, struct hunk *hu\n -\tif (!p)\n -\t\treturn error(_(\"could not parse colored hunk header '%.*s'\"),\n -\t\t\t (int)(eol - line), line);\n - \thunk->colored_start = eol - s->colored.buf + (*eol == '\\n');\n --\theader->colored_extra_start = p + 3 - s->colored.buf;\n -+\tp = memmem(line, eol - line, \"@@ -\", 4);\n +\tif (p && (p = memmem(p + 4, eol - p - 4, \" @@\", 3)))\n +\t\theader->colored_extra_start = p + 3 - s->colored.buf;\n -+\telse\n -+\t\t/* could not parse colored hunk header, showing nothing */\n ++\telse {\n ++\t\t/* could not parse colored hunk header, leave as-is */\n +\t\theader->colored_extra_start = hunk->colored_start;\n ++\t\theader->suppress_colored_line_range = 1;\n ++\t}\n + \thunk->colored_start = eol - s->colored.buf + (*eol == '\\n');\n +-\theader->colored_extra_start = p + 3 - s->colored.buf;\n \theader->colored_extra_end = hunk->colored_start;\n \n \treturn 0;\n +@@ add-patch.c: static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n + \t\t\t\t- header->colored_extra_start;\n + \t\t}\n + \n +-\t\tif (s->mode->is_reverse)\n +-\t\t\told_offset -= delta;\n +-\t\telse\n +-\t\t\tnew_offset += delta;\n +-\n +-\t\tstrbuf_addf(out, \"@@ -%lu\", old_offset);\n +-\t\tif (header->old_count != 1)\n +-\t\t\tstrbuf_addf(out, \",%lu\", header->old_count);\n +-\t\tstrbuf_addf(out, \" +%lu\", new_offset);\n +-\t\tif (header->new_count != 1)\n +-\t\t\tstrbuf_addf(out, \",%lu\", header->new_count);\n +-\t\tstrbuf_addstr(out, \" @@\");\n ++\t\tif (!colored || !header->suppress_colored_line_range) {\n ++\t\t\tif (s->mode->is_reverse)\n ++\t\t\t\told_offset -= delta;\n ++\t\t\telse\n ++\t\t\t\tnew_offset += delta;\n ++\n ++\t\t\tstrbuf_addf(out, \"@@ -%lu\", old_offset);\n ++\t\t\tif (header->old_count != 1)\n ++\t\t\t\tstrbuf_addf(out, \",%lu\", header->old_count);\n ++\t\t\tstrbuf_addf(out, \" +%lu\", new_offset);\n ++\t\t\tif (header->new_count != 1)\n ++\t\t\t\tstrbuf_addf(out, \",%lu\", header->new_count);\n ++\t\t\tstrbuf_addstr(out, \" @@\");\n ++\t\t}\n + \n + \t\tif (len)\n + \t\t\tstrbuf_add(out, p, len);\n \n ## t/t3701-add-interactive.sh ##\n @@ t/t3701-add-interactive.sh: test_expect_success 'detect bogus diffFilter output' '\n - \tforce_color test_must_fail git add -p test &&\n +\tprintf n >n &&\n -+\tforce_color git -c interactive.diffFilter=\"sed s/@@/XX/g\" \\\n -+\t\tadd -p output 2>&1 \n\nIn\nhttps://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com,\nPhillipe Blain reported that the built-in `git add -p` command fails\nwhen asked to use [`diff-so-fancy`][diff-so-fancy] to colorize the diff.\n\nThe reason is that this tool produces colored diffs with a hunk header\nthat does not contain any parseable `@@ ... @@` line range information,\nand therefore we cannot detect any part in that header that comes after\nthe line range.\n\nAs proposed by Phillip Wood, let's take that for a clear indicator that\nwe should show the hunk headers verbatim. This is what the Perl version\nof the interactive `add` command did, too.\n\nThis commit is best viewed with `--color-moved --ignore-space-change`.\n\n[diff-so-fancy]: https://github.com/so-fancy/diff-so-fancy\n\nReported-by: Philippe Blain \nHelped-by: Phillip Wood \nSigned-off-by: Johannes Schindelin \n---\n add-patch.c | 42 ++++++++++++++++++++------------------\n t/t3701-add-interactive.sh | 10 +++++++++\n 2 files changed, 32 insertions(+), 20 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 34f3807ff32..3699ca1fcaf 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -238,6 +238,7 @@ struct hunk_header {\n \t * include the newline.\n \t */\n \tsize_t extra_start, extra_end, colored_extra_start, colored_extra_end;\n+\tunsigned suppress_colored_line_range:1;\n };\n \n struct hunk {\n@@ -358,15 +359,14 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)\n \tif (!eol)\n \t\teol = s->colored.buf + s->colored.len;\n \tp = memmem(line, eol - line, \"@@ -\", 4);\n-\tif (!p)\n-\t\treturn error(_(\"could not parse colored hunk header '%.*s'\"),\n-\t\t\t (int)(eol - line), line);\n-\tp = memmem(p + 4, eol - p - 4, \" @@\", 3);\n-\tif (!p)\n-\t\treturn error(_(\"could not parse colored hunk header '%.*s'\"),\n-\t\t\t (int)(eol - line), line);\n+\tif (p && (p = memmem(p + 4, eol - p - 4, \" @@\", 3)))\n+\t\theader->colored_extra_start = p + 3 - s->colored.buf;\n+\telse {\n+\t\t/* could not parse colored hunk header, leave as-is */\n+\t\theader->colored_extra_start = hunk->colored_start;\n+\t\theader->suppress_colored_line_range = 1;\n+\t}\n \thunk->colored_start = eol - s->colored.buf + (*eol == '\\n');\n-\theader->colored_extra_start = p + 3 - s->colored.buf;\n \theader->colored_extra_end = hunk->colored_start;\n \n \treturn 0;\n@@ -666,18 +666,20 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n \t\t\t\t- header->colored_extra_start;\n \t\t}\n \n-\t\tif (s->mode->is_reverse)\n-\t\t\told_offset -= delta;\n-\t\telse\n-\t\t\tnew_offset += delta;\n-\n-\t\tstrbuf_addf(out, \"@@ -%lu\", old_offset);\n-\t\tif (header->old_count != 1)\n-\t\t\tstrbuf_addf(out, \",%lu\", header->old_count);\n-\t\tstrbuf_addf(out, \" +%lu\", new_offset);\n-\t\tif (header->new_count != 1)\n-\t\t\tstrbuf_addf(out, \",%lu\", header->new_count);\n-\t\tstrbuf_addstr(out, \" @@\");\n+\t\tif (!colored || !header->suppress_colored_line_range) {\n+\t\t\tif (s->mode->is_reverse)\n+\t\t\t\told_offset -= delta;\n+\t\t\telse\n+\t\t\t\tnew_offset += delta;\n+\n+\t\t\tstrbuf_addf(out, \"@@ -%lu\", old_offset);\n+\t\t\tif (header->old_count != 1)\n+\t\t\t\tstrbuf_addf(out, \",%lu\", header->old_count);\n+\t\t\tstrbuf_addf(out, \" +%lu\", new_offset);\n+\t\t\tif (header->new_count != 1)\n+\t\t\t\tstrbuf_addf(out, \",%lu\", header->new_count);\n+\t\t\tstrbuf_addstr(out, \" @@\");\n+\t\t}\n \n \t\tif (len)\n \t\t\tstrbuf_add(out, p, len);\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 8a594700f7b..47ed6698943 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -767,6 +767,16 @@ test_expect_success 'detect bogus diffFilter output' '\n \tgrep \"mismatched output\" output\n '\n \n+test_expect_success 'handle iffy colored hunk headers' '\n+\tgit reset --hard &&\n+\n+\techo content >test &&\n+\tprintf n >n &&\n+\tforce_color git -c interactive.diffFilter=\"sed s/.*@@.*/XX/\" \\\n+\t\tadd -p >output 2>&1 \n\nThanks to alwyas running `diff-index` and `diff-files` with the\n`--numstat` option (the latter with `--ignore-submodules=dirty`) before\neven generating any real diff to parse, the Perl version of `git add -p`\nsimply ignored dirty submodules and does not even offer them up for\nstaging.\n\nHowever, the built-in variant did not use that flag because it tries to\nrun only one `diff` command, skipping the unneeded\n`diff-index`/`diff-files` invocation of the Perl variant and therefore\nonly faithfully recapitulates what the Perl code does once it _does_\ngenerate and parse the real diff.\n\nThis causes a problem when running the built-in `add -p` with\n`diff-so-fancy` because that diff colorizer always inserts an empty line\nbefore the diff header to ensure that it produces 4 lines as expected by\n`git add -p` (the equivalent of the non-colorized `diff`, `index`, `---`\nand `+++` lines). But `git diff-files` does not produce any `index` line\nfor dirty submodules.\n\nThe underlying problem is not even the discrepancy in lines, but that\n`git add -p` presents diffs for dirty submodules: there is nothing that\n_can_ be staged for those.\n\nLet's fix that bug, and teach the built-in `add -p` to ignore dirty\nsubmodules, too. This _incidentally_ also fixes the `diff-so-fancy`\nproblem ;-)\n\nSigned-off-by: Johannes Schindelin \n---\n add-patch.c | 3 ++-\n t/t3701-add-interactive.sh | 12 ++++++++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 3699ca1fcaf..c7eebe9de89 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -419,7 +419,8 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n \t}\n \tcolor_arg_index = args.nr;\n \t/* Use `--no-color` explicitly, just in case `diff.color = always`. */\n-\tstrvec_pushl(&args, \"--no-color\", \"-p\", \"--\", NULL);\n+\tstrvec_pushl(&args, \"--no-color\", \"--ignore-submodules=dirty\", \"-p\",\n+\t\t \"--\", NULL);\n \tfor (i = 0; i < ps->nr; i++)\n \t\tstrvec_push(&args, ps->items[i].original);\n \ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 47ed6698943..2d710645719 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -955,6 +955,18 @@ test_expect_success 'status ignores dirty submodules (except HEAD)' '\n \t! grep dirty-otherwise output\n '\n \n+test_expect_success 'handle submodules' '\n+\techo 123 >>for-submodules/dirty-otherwise/initial.t &&\n+\n+\tforce_color git -C for-submodules add -p dirty-otherwise >output 2>&1 &&\n+\tgrep \"No changes\" output &&\n+\n+\tforce_color git -C for-submodules add -p dirty-head >output 2>&1 actual &&\n+\trev=\"$(git -C for-submodules/dirty-head rev-parse HEAD)\" &&\n+\tgrep \"$rev\" actual\n+'\n+\n test_expect_success 'set up pathological context' '\n \tgit reset --hard &&\n \ttest_write_lines a a a a a a a a a a a >a &&\n-- \ngitgitgadget\n"},{"id":"462317","messageId":"25187c3a3c2bb440ab0af34011db41361d4e2496.1661977877.git.gitgitgadget@gmail.com","threadId":"58353","inReplyTo":"pull.1336.v4.git.1661977877.gitgitgadget@gmail.com","subject":"[PATCH v4 1/3] add -p: detect more mismatches between plain vs colored diffs","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-31T20:31:15Z","receivedAt":"2022-08-31T20:31:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen parsing the colored version of a diff, the interactive `add`\ncommand really relies on the colored version having the same number of\nlines as the plain (uncolored) version. That is an invariant.\n\nWe already have code to verify correctly when the colored diff has less\nlines than the plain diff. Modulo an off-by-one bug: If the last diff\nline has no matching colored one, the code pretends to succeed, still.\n\nTo make matters worse, when we adjusted the test in 1e4ffc765db (t3701:\nadjust difffilter test, 2020-01-14), we did not catch this because `add\n-p` fails for a _different_ reason: it does not find any colored hunk\nheader that contains a parseable line range.\n\nIf we change the test case so that the line range _can_ be parsed, the\nbug is exposed.\n\nLet's address all of the above by\n\n- fixing the off-by-one,\n\n- adjusting the test case to allow `add -p` to parse the line range\n\n- making the test case more stringent by verifying that the expected\n error message is shown\n\nAlso adjust a misleading code comment about the now-fixed code.\n\nSigned-off-by: Johannes Schindelin \n---\n add-patch.c | 5 ++++-\n t/t3701-add-interactive.sh | 5 +++--\n 2 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 509ca04456b..34f3807ff32 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -592,7 +592,10 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n \t\t\tif (colored_eol)\n \t\t\t\tcolored_p = colored_eol + 1;\n \t\t\telse if (p != pend)\n-\t\t\t\t/* colored shorter than non-colored? */\n+\t\t\t\t/* non-colored has more lines? */\n+\t\t\t\tgoto mismatched_output;\n+\t\t\telse if (colored_p == colored_pend)\n+\t\t\t\t/* last line has no matching colored one? */\n \t\t\t\tgoto mismatched_output;\n \t\t\telse\n \t\t\t\tcolored_p = colored_pend;\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 3b7df9bed5a..8a594700f7b 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -761,9 +761,10 @@ test_expect_success 'detect bogus diffFilter output' '\n \tgit reset --hard &&\n \n \techo content >test &&\n-\ttest_config interactive.diffFilter \"sed 1d\" &&\n+\ttest_config interactive.diffFilter \"sed 6d\" &&\n \tprintf y >y &&\n-\tforce_color test_must_fail git add -p output 2>&1 &&\n+\tgrep \"mismatched output\" output\n '\n \n test_expect_success 'handle very large filtered diff' '\n-- \ngitgitgadget\n\n"},{"id":"462318","messageId":"q77sq7pn-n473-p12s-po5o-6970rr884528@tzk.qr","threadId":"58353","inReplyTo":"xmqqr10wdv4b.fsf@gitster.g","subject":"Re: [PATCH v3 5/5] add -p: ignore dirty submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-31T20:38:40Z","receivedAt":"2022-08-31T20:38:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 31 Aug 2022, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> > And it is that doubling of work that I tried to avoid when implementing\n> > the built-in version. The bug came about because the full diff call wasn't\n> > using the `--ignore-submodules=dirty` option, and that's what I missed.\n> >\n> > This is maybe more interesting a story for the cover letter, to be able to\n> > understand how this bug was introduced, and maybe to offer an opportunity\n> > for others (in addition to myself) to learn from my mistake.\n>\n> Yup, the moral of the story is premature optimization bites because\n> we are not always careful ;-)\n\nYes. Maybe even more generally: trying to do too many things at once is\nprone to introduce bugs.\n\n> Anyhow, I am getting an impression that the behaviour of the next\n> iteration would be much closer to the original, which is excellent.\n\nYep!\n\n> We have seen too many \"ah, this is broken and here is a fix that is\n> appropriate in the context of how the new C version does it\", not\n> \"ok, let's make the whole thing behave more like what we used to\n> have\".\n\nUnfortunately so.\n\nIt is a fine line, and not always possible to tread safely, between trying\nto stay close to the original and trying to learn the lesson from Elijah\nthat converting scripts to C without playing to C's strengths causes a lot\nof hiccups later on. There are probably better alternatives for\nprototyping Git functionality than using either an untyped language that\ncannot hold complex data structures in memory or a language to which\neverything looks like a map ;-)\n\nCiao,\nDscho\n"},{"id":"462319","messageId":"31rnq09n-5nor-22n0-r68n-5o46p313p5p3@tzk.qr","threadId":"58353","inReplyTo":"1d4bf456-6fc0-b675-f0ee-f8ce99bd38b2@gmail.com","subject":"Re: [PATCH v3 0/5] built-in add -p: support diff-so-fancy better","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-08-31T20:44:18Z","receivedAt":"2022-08-31T20:44:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Tue, 30 Aug 2022, Phillip Wood wrote:\n\n> I'm not convinced about patch 1 or about showing the hunk offsets with\n> interactive.diffFilter in patch 4 but the series looks basically sound.\n\nThe fourth iteration of this patch series that I just sent out should\naddress both concerns.\n\nThank you!\nDscho\n"},{"id":"462446","messageId":"a6f68650-678a-ab5f-a432-87fab8821dc2@gmail.com","threadId":"58353","inReplyTo":"25187c3a3c2bb440ab0af34011db41361d4e2496.1661977877.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 1/3] add -p: detect more mismatches between plain vs colored diffs","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-09-01T13:19:57Z","receivedAt":"2022-09-01T13:22:10Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 31/08/2022 21:31, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> When parsing the colored version of a diff, the interactive `add`\n> command really relies on the colored version having the same number of\n> lines as the plain (uncolored) version. That is an invariant.\n> \n> We already have code to verify correctly when the colored diff has less\n> lines than the plain diff. Modulo an off-by-one bug: If the last diff\n> line has no matching colored one, the code pretends to succeed, still.\n> \n> To make matters worse, when we adjusted the test in 1e4ffc765db (t3701:\n> adjust difffilter test, 2020-01-14), we did not catch this because `add\n> -p` fails for a _different_ reason: it does not find any colored hunk\n> header that contains a parseable line range.\n> \n> If we change the test case so that the line range _can_ be parsed, the\n> bug is exposed.\n> \n> Let's address all of the above by\n> \n> - fixing the off-by-one,\n> \n> - adjusting the test case to allow `add -p` to parse the line range\n> \n> - making the test case more stringent by verifying that the expected\n> error message is shown\n> \n> Also adjust a misleading code comment about the now-fixed code.\n\nThanks for re-rolling, this looks good. The commit message explains the \nproblem well and the fix is good, I especially like the fact that you've \nadded a grep for the correct error message.\n\nRelated to this we also have code that detects over-long output from the \nfilter, I'm not sure if we have a test for that but I think the \nimplementation looks ok.\n\nBest Wishes\n\nPhillip\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> add-patch.c | 5 ++++-\n> t/t3701-add-interactive.sh | 5 +++--\n> 2 files changed, 7 insertions(+), 3 deletions(-)\n> \n> diff --git a/add-patch.c b/add-patch.c\n> index 509ca04456b..34f3807ff32 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -592,7 +592,10 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n> \t\t\tif (colored_eol)\n> \t\t\t\tcolored_p = colored_eol + 1;\n> \t\t\telse if (p != pend)\n> -\t\t\t\t/* colored shorter than non-colored? */\n> +\t\t\t\t/* non-colored has more lines? */\n> +\t\t\t\tgoto mismatched_output;\n> +\t\t\telse if (colored_p == colored_pend)\n> +\t\t\t\t/* last line has no matching colored one? */\n> \t\t\t\tgoto mismatched_output;\n> \t\t\telse\n> \t\t\t\tcolored_p = colored_pend;\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index 3b7df9bed5a..8a594700f7b 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -761,9 +761,10 @@ test_expect_success 'detect bogus diffFilter output' '\n> \tgit reset --hard &&\n> \n> \techo content >test &&\n> -\ttest_config interactive.diffFilter \"sed 1d\" &&\n> +\ttest_config interactive.diffFilter \"sed 6d\" &&\n> \tprintf y >y &&\n> -\tforce_color test_must_fail git add -p +\tforce_color test_must_fail git add -p output 2>&1 &&\n> +\tgrep \"mismatched output\" output\n> '\n> \n> test_expect_success 'handle very large filtered diff' '\n"},{"id":"462447","messageId":"19d4c44b-868e-628d-5715-54f37ef56c0d@gmail.com","threadId":"58353","inReplyTo":"cd1c51005068247fc92f1c515469bcd384bfe589.1661977877.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 2/3] add -p: gracefully handle unparseable hunk headers in colored diffs","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-09-01T13:53:52Z","receivedAt":"2022-09-01T13:54:03Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 31/08/2022 21:31, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> In\n> https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com,\n> Phillipe Blain reported that the built-in `git add -p` command fails\n> when asked to use [`diff-so-fancy`][diff-so-fancy] to colorize the diff.\n> \n> The reason is that this tool produces colored diffs with a hunk header\n> that does not contain any parseable `@@ ... @@` line range information,\n> and therefore we cannot detect any part in that header that comes after\n> the line range.\n> \n> As proposed by Phillip Wood, let's take that for a clear indicator that\n> we should show the hunk headers verbatim. This is what the Perl version\n> of the interactive `add` command did, too.\n> \n> This commit is best viewed with `--color-moved --ignore-space-change`.\n\nor --color-moved-ws=allow-indentation-change\n\nThis looks pretty good, unsurprisingly I like the fact that the output \nnow matches the perl version. I was confused by the grep in the test \nwhich lead me to realize we're printing an color escape code when we \nshouldn't.\n\n> [diff-so-fancy]: https://github.com/so-fancy/diff-so-fancy\n> \n> Reported-by: Philippe Blain \n> Helped-by: Phillip Wood \n> Signed-off-by: Johannes Schindelin \n> ---\n> add-patch.c | 42 ++++++++++++++++++++------------------\n> t/t3701-add-interactive.sh | 10 +++++++++\n> 2 files changed, 32 insertions(+), 20 deletions(-)\n> \n> diff --git a/add-patch.c b/add-patch.c\n> index 34f3807ff32..3699ca1fcaf 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -238,6 +238,7 @@ struct hunk_header {\n> \t * include the newline.\n> \t */\n> \tsize_t extra_start, extra_end, colored_extra_start, colored_extra_end;\n> +\tunsigned suppress_colored_line_range:1;\n> };\n> \n> struct hunk {\n> @@ -358,15 +359,14 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)\n> \tif (!eol)\n> \t\teol = s->colored.buf + s->colored.len;\n> \tp = memmem(line, eol - line, \"@@ -\", 4);\n> -\tif (!p)\n> -\t\treturn error(_(\"could not parse colored hunk header '%.*s'\"),\n> -\t\t\t (int)(eol - line), line);\n> -\tp = memmem(p + 4, eol - p - 4, \" @@\", 3);\n> -\tif (!p)\n> -\t\treturn error(_(\"could not parse colored hunk header '%.*s'\"),\n> -\t\t\t (int)(eol - line), line);\n> +\tif (p && (p = memmem(p + 4, eol - p - 4, \" @@\", 3)))\n\nnit: there should be braces round both arms of the if, but it's hardly \nthe first one that does not follow our official style.\n\n> +\t\theader->colored_extra_start = p + 3 - s->colored.buf;\n> +\telse {\n> +\t\t/* could not parse colored hunk header, leave as-is */\n> +\t\theader->colored_extra_start = hunk->colored_start;\n> +\t\theader->suppress_colored_line_range = 1;\n> +\t}\n> \thunk->colored_start = eol - s->colored.buf + (*eol == '\\n');\n> -\theader->colored_extra_start = p + 3 - s->colored.buf;\n> \theader->colored_extra_end = hunk->colored_start;\n> \n> \treturn 0;\n> @@ -666,18 +666,20 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n if (!colored) {\n p = s->plain.buf + header->extra_start;\n len = header->extra_end - header->extra_start;\n } else {\n strbuf_addstr(out, s->s.fraginfo_color);\n\nI don't think we want to add this escape sequence unless we're going to \ndynamically generate the hunk header\n\n p = s->colored.buf + header->colored_extra_start;\n len = header->colored_extra_end\n - header->colored_extra_start;\n\nIf we cannot parse the hunk header then len is non-zero...\n\n }\n> \n> -\t\tif (s->mode->is_reverse)\n> -\t\t\told_offset -= delta;\n> -\t\telse\n> -\t\t\tnew_offset += delta;\n> -\n> -\t\tstrbuf_addf(out, \"@@ -%lu\", old_offset);\n> -\t\tif (header->old_count != 1)\n> -\t\t\tstrbuf_addf(out, \",%lu\", header->old_count);\n> -\t\tstrbuf_addf(out, \" +%lu\", new_offset);\n> -\t\tif (header->new_count != 1)\n> -\t\t\tstrbuf_addf(out, \",%lu\", header->new_count);\n> -\t\tstrbuf_addstr(out, \" @@\");\n> +\t\tif (!colored || !header->suppress_colored_line_range) {\n> +\t\t\tif (s->mode->is_reverse)\n> +\t\t\t\told_offset -= delta;\n> +\t\t\telse\n> +\t\t\t\tnew_offset += delta;\n> +\n> +\t\t\tstrbuf_addf(out, \"@@ -%lu\", old_offset);\n> +\t\t\tif (header->old_count != 1)\n> +\t\t\t\tstrbuf_addf(out, \",%lu\", header->old_count);\n> +\t\t\tstrbuf_addf(out, \" +%lu\", new_offset);\n> +\t\t\tif (header->new_count != 1)\n> +\t\t\t\tstrbuf_addf(out, \",%lu\", header->new_count);\n> +\t\t\tstrbuf_addstr(out, \" @@\");\n> +\t\t}\n> \n> \t\tif (len)\n> \t\t\tstrbuf_add(out, p, len);\n\n... and so we print the filtered hunk header here and do not reset the \ncolor below. That is probably ok as the filter should be resetting it's \nown colors but we shouldn't be printing the color at the beginning of \nthe line above in that case.\n\n else if (colored)\n strbuf_addf(out, \"%s\\n\", s->s.reset_color);\n else\n strbuf_addch(out, '\\n');\n }\n\n\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index 8a594700f7b..47ed6698943 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -767,6 +767,16 @@ test_expect_success 'detect bogus diffFilter output' '\n> \tgrep \"mismatched output\" output\n> '\n> \n> +test_expect_success 'handle iffy colored hunk headers' '\n> +\tgit reset --hard &&\n> +\n> +\techo content >test &&\n> +\tprintf n >n &&\n> +\tforce_color git -c interactive.diffFilter=\"sed s/.*@@.*/XX/\" \\\n> +\t\tadd -p >output 2>&1 +\tgrep \"^[^@]*XX[^@]*$\" output\n\nI was wondering why this wasn't just `grep \"^XX$\"` as we should be \nprinting the output of the diff filter verbatim. That lead to my \ncomments about outputting escape codes above. Apart from that this patch \nlooks good.\n\nBest Wishes\n\nPhillip\n\n> +'\n> +\n> test_expect_success 'handle very large filtered diff' '\n> \tgit reset --hard &&\n> \t# The specific number here is not important, but it must\n"},{"id":"462448","messageId":"77046611-03ae-3b0f-736a-c377a63274c2@gmail.com","threadId":"58353","inReplyTo":"pull.1336.v4.git.1661977877.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 0/3] built-in add -p: support diff-so-fancy better","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-09-01T13:55:30Z","receivedAt":"2022-09-01T13:55:46Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 31/08/2022 21:31, Johannes Schindelin via GitGitGadget wrote:\n> Philippe Blain reported in\n> https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com\n> that there is a problem when running the built-in version of git add -p with\n> diff-so-fancy [https://github.com/so-fancy/diff-so-fancy] as diff colorizer.\n> The symptom is this:\n> \n> error: could not parse colored hunk header '?[36m?[1m?[38;5;13m@ file:1 @?[1m?[0m'\n> \n> \n> This patch series addresses that and should fix\n> https://github.com/so-fancy/diff-so-fancy/issues/437\n> \n> Changes since v3:\n> \n> * Instead of deviating from how the Perl version of git add -p did things,\n> we now teach the built-in version to display hunk headers verbatim when\n> no line range could be parsed out (instead of showing the line range\n> anyways). This was a very good idea of Phillip's, dramatically\n> simplifying the patch series.\n> * Also, this iteration drops the first patch that claims to redefine what\n> we consider bogus, but only hides an off-by-one. In its stead, there is\n> now a patch that fixes said off-by-one.\n\nThis version is looking much simpler and nicer, I've left a couple of \ncomments on the second patch as we're still not quite printing the \noutput of the diff filter verbatim.\n\nThanks\n\nPhillip\n\n> Changes since v2:\n> \n> * Added the appropriate \"Reported-by\" trailer to the commit message.\n> * Split out the logic to insert a space between the colored line range and\n> the extra information, if needed.\n> * That logic was now corrected to see whether that space is really needed.\n> * To verify that the logic does what we need it to do, the added regression\n> test now specifically tests for that (single) extra space that we want to\n> be inserted.\n> * Reworded a stale comment that claimed that we might suppress the entire\n> colored hunk header (which we no longer do).\n> * Rebased to the current tip of the main branch to avoid a merge conflict\n> with 716c1f649e3 (pipe_command(): mark stdin descriptor as non-blocking,\n> 2022-08-17).\n> \n> Changes since v1:\n> \n> * Added a commit to ignore dirty submodules just like the Perl version\n> does.\n> \n> Johannes Schindelin (3):\n> add -p: detect more mismatches between plain vs colored diffs\n> add -p: gracefully handle unparseable hunk headers in colored diffs\n> add -p: ignore dirty submodules\n> \n> add-patch.c | 50 +++++++++++++++++++++-----------------\n> t/t3701-add-interactive.sh | 27 ++++++++++++++++++--\n> 2 files changed, 53 insertions(+), 24 deletions(-)\n> \n> \n> base-commit: 07ee72db0e97b5c233f8ada0abb412248c2f1c6f\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1336%2Fdscho%2Fdiff-so-fancy-v4\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1336/dscho/diff-so-fancy-v4\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1336\n> \n> Range-diff vs v3:\n> \n> 1: a01fa5d25e4 ! 1: 25187c3a3c2 t3701: redefine what is \"bogus\" output of a diff filter\n> @@ Metadata\n> Author: Johannes Schindelin \n> \n> ## Commit message ##\n> - t3701: redefine what is \"bogus\" output of a diff filter\n> + add -p: detect more mismatches between plain vs colored diffs\n> \n> When parsing the colored version of a diff, the interactive `add`\n> command really relies on the colored version having the same number of\n> - lines as the non-colored version. That is an invariant.\n> -\n> - However, in the 'detect bogus diffFilter output' test case in t3701, we\n> - essentially required a hunk header that contains parseable `@@ ... @@`\n> - hunk headers, and called all colored diffs without such hunks bogus.\n> -\n> - The reason for this is that we would like to show the users the adjusted\n> - hunk headers _including_ the extra part after the `@@ ... @@`\n> - information, which usually contains things like the function name or\n> - soms such.\n> -\n> - Now, there is a _very_ popular diff colorizer called `diff-so-fancy`\n> - that does not produce such colored diffs as the built-in `add` command\n> - expects. Nevertheless, the Perl variant of the `add` command handles\n> - those nicely, essentially by ignoring the hunk header and saying \"there\n> - is nothing else we can show except the original hunk header, even if we\n> - had to adjust the line range and the original hunk header might get that\n> - wrong\".\n> -\n> - In preparation for teaching the built-in interactive `add` to be a bit\n> - more lenient, let's change the 'detect bogus diffFilter output' test\n> - case so that it verifies that a mismatched number of lines causes the\n> - command to error out, but not an unparseable hunk header.\n> + lines as the plain (uncolored) version. That is an invariant.\n> +\n> + We already have code to verify correctly when the colored diff has less\n> + lines than the plain diff. Modulo an off-by-one bug: If the last diff\n> + line has no matching colored one, the code pretends to succeed, still.\n> +\n> + To make matters worse, when we adjusted the test in 1e4ffc765db (t3701:\n> + adjust difffilter test, 2020-01-14), we did not catch this because `add\n> + -p` fails for a _different_ reason: it does not find any colored hunk\n> + header that contains a parseable line range.\n> +\n> + If we change the test case so that the line range _can_ be parsed, the\n> + bug is exposed.\n> +\n> + Let's address all of the above by\n> +\n> + - fixing the off-by-one,\n> +\n> + - adjusting the test case to allow `add -p` to parse the line range\n> +\n> + - making the test case more stringent by verifying that the expected\n> + error message is shown\n> +\n> + Also adjust a misleading code comment about the now-fixed code.\n> \n> Signed-off-by: Johannes Schindelin \n> \n> + ## add-patch.c ##\n> +@@ add-patch.c: static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n> + \t\t\tif (colored_eol)\n> + \t\t\t\tcolored_p = colored_eol + 1;\n> + \t\t\telse if (p != pend)\n> +-\t\t\t\t/* colored shorter than non-colored? */\n> ++\t\t\t\t/* non-colored has more lines? */\n> ++\t\t\t\tgoto mismatched_output;\n> ++\t\t\telse if (colored_p == colored_pend)\n> ++\t\t\t\t/* last line has no matching colored one? */\n> + \t\t\t\tgoto mismatched_output;\n> + \t\t\telse\n> + \t\t\t\tcolored_p = colored_pend;\n> +\n> ## t/t3701-add-interactive.sh ##\n> @@ t/t3701-add-interactive.sh: test_expect_success 'detect bogus diffFilter output' '\n> \tgit reset --hard &&\n> \n> \techo content >test &&\n> -\ttest_config interactive.diffFilter \"sed 1d\" &&\n> -+\ttest_config interactive.diffFilter \"sed q\" &&\n> ++\ttest_config interactive.diffFilter \"sed 6d\" &&\n> \tprintf y >y &&\n> - \tforce_color test_must_fail git add -p +-\tforce_color test_must_fail git add -p ++\tforce_color test_must_fail git add -p output 2>&1 &&\n> ++\tgrep \"mismatched output\" output\n> '\n> +\n> + test_expect_success 'handle very large filtered diff' '\n> 2: cbe833bd141 ! 2: cd1c5100506 add -p: gracefully ignore unparseable hunk headers in colored diffs\n> @@ Metadata\n> Author: Johannes Schindelin \n> \n> ## Commit message ##\n> - add -p: gracefully ignore unparseable hunk headers in colored diffs\n> + add -p: gracefully handle unparseable hunk headers in colored diffs\n> \n> In\n> https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com,\n> @@ Commit message\n> and therefore we cannot detect any part in that header that comes after\n> the line range.\n> \n> - Let's punt for now and simply show nothing apart from the line range in\n> - that case.\n> + As proposed by Phillip Wood, let's take that for a clear indicator that\n> + we should show the hunk headers verbatim. This is what the Perl version\n> + of the interactive `add` command did, too.\n> +\n> + This commit is best viewed with `--color-moved --ignore-space-change`.\n> \n> [diff-so-fancy]: https://github.com/so-fancy/diff-so-fancy\n> \n> Reported-by: Philippe Blain \n> + Helped-by: Phillip Wood \n> Signed-off-by: Johannes Schindelin \n> \n> ## add-patch.c ##\n> +@@ add-patch.c: struct hunk_header {\n> + \t * include the newline.\n> + \t */\n> + \tsize_t extra_start, extra_end, colored_extra_start, colored_extra_end;\n> ++\tunsigned suppress_colored_line_range:1;\n> + };\n> +\n> + struct hunk {\n> @@ add-patch.c: static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)\n> - \teol = memchr(line, '\\n', s->colored.len - hunk->colored_start);\n> \tif (!eol)\n> \t\teol = s->colored.buf + s->colored.len;\n> --\tp = memmem(line, eol - line, \"@@ -\", 4);\n> + \tp = memmem(line, eol - line, \"@@ -\", 4);\n> -\tif (!p)\n> -\t\treturn error(_(\"could not parse colored hunk header '%.*s'\"),\n> -\t\t\t (int)(eol - line), line);\n> @@ add-patch.c: static int parse_hunk_header(struct add_p_state *s, struct hunk *hu\n> -\tif (!p)\n> -\t\treturn error(_(\"could not parse colored hunk header '%.*s'\"),\n> -\t\t\t (int)(eol - line), line);\n> - \thunk->colored_start = eol - s->colored.buf + (*eol == '\\n');\n> --\theader->colored_extra_start = p + 3 - s->colored.buf;\n> -+\tp = memmem(line, eol - line, \"@@ -\", 4);\n> +\tif (p && (p = memmem(p + 4, eol - p - 4, \" @@\", 3)))\n> +\t\theader->colored_extra_start = p + 3 - s->colored.buf;\n> -+\telse\n> -+\t\t/* could not parse colored hunk header, showing nothing */\n> ++\telse {\n> ++\t\t/* could not parse colored hunk header, leave as-is */\n> +\t\theader->colored_extra_start = hunk->colored_start;\n> ++\t\theader->suppress_colored_line_range = 1;\n> ++\t}\n> + \thunk->colored_start = eol - s->colored.buf + (*eol == '\\n');\n> +-\theader->colored_extra_start = p + 3 - s->colored.buf;\n> \theader->colored_extra_end = hunk->colored_start;\n> \n> \treturn 0;\n> +@@ add-patch.c: static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n> + \t\t\t\t- header->colored_extra_start;\n> + \t\t}\n> +\n> +-\t\tif (s->mode->is_reverse)\n> +-\t\t\told_offset -= delta;\n> +-\t\telse\n> +-\t\t\tnew_offset += delta;\n> +-\n> +-\t\tstrbuf_addf(out, \"@@ -%lu\", old_offset);\n> +-\t\tif (header->old_count != 1)\n> +-\t\t\tstrbuf_addf(out, \",%lu\", header->old_count);\n> +-\t\tstrbuf_addf(out, \" +%lu\", new_offset);\n> +-\t\tif (header->new_count != 1)\n> +-\t\t\tstrbuf_addf(out, \",%lu\", header->new_count);\n> +-\t\tstrbuf_addstr(out, \" @@\");\n> ++\t\tif (!colored || !header->suppress_colored_line_range) {\n> ++\t\t\tif (s->mode->is_reverse)\n> ++\t\t\t\told_offset -= delta;\n> ++\t\t\telse\n> ++\t\t\t\tnew_offset += delta;\n> ++\n> ++\t\t\tstrbuf_addf(out, \"@@ -%lu\", old_offset);\n> ++\t\t\tif (header->old_count != 1)\n> ++\t\t\t\tstrbuf_addf(out, \",%lu\", header->old_count);\n> ++\t\t\tstrbuf_addf(out, \" +%lu\", new_offset);\n> ++\t\t\tif (header->new_count != 1)\n> ++\t\t\t\tstrbuf_addf(out, \",%lu\", header->new_count);\n> ++\t\t\tstrbuf_addstr(out, \" @@\");\n> ++\t\t}\n> +\n> + \t\tif (len)\n> + \t\t\tstrbuf_add(out, p, len);\n> \n> ## t/t3701-add-interactive.sh ##\n> @@ t/t3701-add-interactive.sh: test_expect_success 'detect bogus diffFilter output' '\n> - \tforce_color test_must_fail git add -p + \tgrep \"mismatched output\" output\n> '\n> \n> +test_expect_success 'handle iffy colored hunk headers' '\n> @@ t/t3701-add-interactive.sh: test_expect_success 'detect bogus diffFilter output'\n> +\n> +\techo content >test &&\n> +\tprintf n >n &&\n> -+\tforce_color git -c interactive.diffFilter=\"sed s/@@/XX/g\" \\\n> -+\t\tadd -p ++\tforce_color git -c interactive.diffFilter=\"sed s/.*@@.*/XX/\" \\\n> ++\t\tadd -p >output 2>&1 ++\tgrep \"^[^@]*XX[^@]*$\" output\n> +'\n> +\n> test_expect_success 'handle very large filtered diff' '\n> 3: 7a9f0b107e6 < -: ----------- add -p: insert space in colored hunk header as needed\n> 4: e3e3a178f98 < -: ----------- add -p: handle `diff-so-fancy`'s hunk headers better\n> 5: cfa6914aee0 = 3: 116f0cf5cab add -p: ignore dirty submodules\n> \n"},{"id":"462451","messageId":"1q42767p-7oqo-np69-0099-2r5784q5q140@tzk.qr","threadId":"58353","inReplyTo":"19d4c44b-868e-628d-5715-54f37ef56c0d@gmail.com","subject":"Re: [PATCH v4 2/3] add -p: gracefully handle unparseable hunk headers in colored diffs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-09-01T15:09:56Z","receivedAt":"2022-09-01T15:10:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Thu, 1 Sep 2022, Phillip Wood wrote:\n\n> On 31/08/2022 21:31, Johannes Schindelin via GitGitGadget wrote:\n> [...]\n> > @@ -358,15 +359,14 @@ static int parse_hunk_header(struct add_p_state *s,\n> > @@ struct hunk *hunk)\n> > if (!eol)\n> > \teol = s->colored.buf + s->colored.len;\n> > \tp = memmem(line, eol - line, \"@@ -\", 4);\n> > -\tif (!p)\n> > -\t\treturn error(_(\"could not parse colored hunk header '%.*s'\"),\n> > -\t\t\t (int)(eol - line), line);\n> > -\tp = memmem(p + 4, eol - p - 4, \" @@\", 3);\n> > -\tif (!p)\n> > -\t\treturn error(_(\"could not parse colored hunk header '%.*s'\"),\n> > -\t\t\t (int)(eol - line), line);\n> > +\tif (p && (p = memmem(p + 4, eol - p - 4, \" @@\", 3)))\n>\n> nit: there should be braces round both arms of the if, but it's hardly the\n> first one that does not follow our official style.\n\nFixed.\n\n> > @@ -666,18 +666,20 @@ static void render_hunk(struct add_p_state *s, struct\n> > @@ hunk *hunk,\n> if (!colored) {\n> p = s->plain.buf + header->extra_start;\n> len = header->extra_end - header->extra_start;\n> } else {\n> strbuf_addstr(out, s->s.fraginfo_color);\n>\n> I don't think we want to add this escape sequence unless we're going to\n> dynamically generate the hunk header\n\nTrue.\n\n>\n> p = s->colored.buf + header->colored_extra_start;\n> len = header->colored_extra_end\n> - header->colored_extra_start;\n>\n> If we cannot parse the hunk header then len is non-zero...\n>\n> }\n> > -\t\tif (s->mode->is_reverse)\n> > -\t\t\told_offset -= delta;\n> > -\t\telse\n> > -\t\t\tnew_offset += delta;\n> > -\n> > -\t\tstrbuf_addf(out, \"@@ -%lu\", old_offset);\n> > -\t\tif (header->old_count != 1)\n> > -\t\t\tstrbuf_addf(out, \",%lu\", header->old_count);\n> > -\t\tstrbuf_addf(out, \" +%lu\", new_offset);\n> > -\t\tif (header->new_count != 1)\n> > -\t\t\tstrbuf_addf(out, \",%lu\", header->new_count);\n> > -\t\tstrbuf_addstr(out, \" @@\");\n> > +\t\tif (!colored || !header->suppress_colored_line_range) {\n> > +\t\t\tif (s->mode->is_reverse)\n> > +\t\t\t\told_offset -= delta;\n> > +\t\t\telse\n> > +\t\t\t\tnew_offset += delta;\n> > +\n> > +\t\t\tstrbuf_addf(out, \"@@ -%lu\", old_offset);\n> > +\t\t\tif (header->old_count != 1)\n> > +\t\t\t\tstrbuf_addf(out, \",%lu\", header->old_count);\n> > +\t\t\tstrbuf_addf(out, \" +%lu\", new_offset);\n> > +\t\t\tif (header->new_count != 1)\n> > +\t\t\t\tstrbuf_addf(out, \",%lu\", header->new_count);\n> > +\t\t\tstrbuf_addstr(out, \" @@\");\n> > +\t\t}\n> >\n> > if (len)\n> > strbuf_add(out, p, len);\n>\n> ... and so we print the filtered hunk header here and do not reset the color\n> below. That is probably ok as the filter should be resetting it's own colors\n> but we shouldn't be printing the color at the beginning of the line above in\n> that case.\n>\n> else if (colored)\n> strbuf_addf(out, \"%s\\n\", s->s.reset_color);\n> else\n> strbuf_addch(out, '\\n');\n\nI've changed the code so that the `suppress_colored_line_range` code path\nprints no extra sequence but the hunk header and the hunk verbatim (they\nstill have to be two separate `strbuf*()` calls because the hunk could be\nedited).\n\nSince this code path now also returns early, the patch avoids the\nindentation change altogether.\n\n> }\n>\n>\n> > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> > index 8a594700f7b..47ed6698943 100755\n> > --- a/t/t3701-add-interactive.sh\n> > +++ b/t/t3701-add-interactive.sh\n> > @@ -767,6 +767,16 @@ test_expect_success 'detect bogus diffFilter output' '\n> > \tgrep \"mismatched output\" output\n> > '\n> >\n> > +test_expect_success 'handle iffy colored hunk headers' '\n> > +\tgit reset --hard &&\n> > +\n> > +\techo content >test &&\n> > +\tprintf n >n &&\n> > +\tforce_color git -c interactive.diffFilter=\"sed s/.*@@.*/XX/\" \\\n> > +\t\tadd -p >output 2>&1 > +\tgrep \"^[^@]*XX[^@]*$\" output\n>\n> I was wondering why this wasn't just `grep \"^XX$\"` as we should be printing\n> the output of the diff filter verbatim. That lead to my comments about\n> outputting escape codes above. Apart from that this patch looks good.\n\nI changed it to `^XX$` as you suggested.\n\nThank you for your excellent review,\nDscho\n\n>\n> Best Wishes\n>\n> Phillip\n>\n> > +'\n> > +\n> > test_expect_success 'handle very large filtered diff' '\n> > git reset --hard &&\n> > # The specific number here is not important, but it must\n>\n>\n"},{"id":"462453","messageId":"pull.1336.v5.git.1662046939.gitgitgadget@gmail.com","threadId":"58353","inReplyTo":"pull.1336.v4.git.1661977877.gitgitgadget@gmail.com","subject":"[PATCH v5 0/3] built-in add -p: support diff-so-fancy better","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-01T15:42:16Z","receivedAt":"2022-09-01T15:42:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Philippe Blain reported in\nhttps://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com\nthat there is a problem when running the built-in version of git add -p with\ndiff-so-fancy [https://github.com/so-fancy/diff-so-fancy] as diff colorizer.\nThe symptom is this:\n\n error: could not parse colored hunk header '?[36m?[1m?[38;5;13m@ file:1 @?[1m?[0m'\n\n\nThis patch series addresses that and should fix\nhttps://github.com/so-fancy/diff-so-fancy/issues/437\n\nChanges since v4:\n\n * The second patch was further simplified, avoiding to print extra ANSI\n sequences if the hunk header is shown verbatim.\n\nChanges since v3:\n\n * Instead of deviating from how the Perl version of git add -p did things,\n we now teach the built-in version to display hunk headers verbatim when\n no line range could be parsed out (instead of showing the line range\n anyways). This was a very good idea of Phillip's, dramatically\n simplifying the patch series.\n * Also, this iteration drops the first patch that claims to redefine what\n we consider bogus, but only hides an off-by-one. In its stead, there is\n now a patch that fixes said off-by-one.\n\nChanges since v2:\n\n * Added the appropriate \"Reported-by\" trailer to the commit message.\n * Split out the logic to insert a space between the colored line range and\n the extra information, if needed.\n * That logic was now corrected to see whether that space is really needed.\n * To verify that the logic does what we need it to do, the added regression\n test now specifically tests for that (single) extra space that we want to\n be inserted.\n * Reworded a stale comment that claimed that we might suppress the entire\n colored hunk header (which we no longer do).\n * Rebased to the current tip of the main branch to avoid a merge conflict\n with 716c1f649e3 (pipe_command(): mark stdin descriptor as non-blocking,\n 2022-08-17).\n\nChanges since v1:\n\n * Added a commit to ignore dirty submodules just like the Perl version\n does.\n\nJohannes Schindelin (3):\n add -p: detect more mismatches between plain vs colored diffs\n add -p: gracefully handle unparseable hunk headers in colored diffs\n add -p: ignore dirty submodules\n\n add-patch.c | 33 +++++++++++++++++++++++----------\n t/t3701-add-interactive.sh | 27 +++++++++++++++++++++++++--\n 2 files changed, 48 insertions(+), 12 deletions(-)\n\n\nbase-commit: 07ee72db0e97b5c233f8ada0abb412248c2f1c6f\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1336%2Fdscho%2Fdiff-so-fancy-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1336/dscho/diff-so-fancy-v5\nPull-Request: https://github.com/gitgitgadget/git/pull/1336\n\nRange-diff vs v4:\n\n 1: 25187c3a3c2 = 1: 25187c3a3c2 add -p: detect more mismatches between plain vs colored diffs\n 2: cd1c5100506 ! 2: 93d0e3b4d2a add -p: gracefully handle unparseable hunk headers in colored diffs\n @@ Commit message\n we should show the hunk headers verbatim. This is what the Perl version\n of the interactive `add` command did, too.\n \n - This commit is best viewed with `--color-moved --ignore-space-change`.\n -\n [diff-so-fancy]: https://github.com/so-fancy/diff-so-fancy\n \n Reported-by: Philippe Blain \n @@ add-patch.c: static int parse_hunk_header(struct add_p_state *s, struct hunk *hu\n -\tif (!p)\n -\t\treturn error(_(\"could not parse colored hunk header '%.*s'\"),\n -\t\t\t (int)(eol - line), line);\n -+\tif (p && (p = memmem(p + 4, eol - p - 4, \" @@\", 3)))\n ++\tif (p && (p = memmem(p + 4, eol - p - 4, \" @@\", 3))) {\n +\t\theader->colored_extra_start = p + 3 - s->colored.buf;\n -+\telse {\n ++\t} else {\n +\t\t/* could not parse colored hunk header, leave as-is */\n +\t\theader->colored_extra_start = hunk->colored_start;\n +\t\theader->suppress_colored_line_range = 1;\n @@ add-patch.c: static int parse_hunk_header(struct add_p_state *s, struct hunk *hu\n \n \treturn 0;\n @@ add-patch.c: static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n - \t\t\t\t- header->colored_extra_start;\n - \t\t}\n - \n --\t\tif (s->mode->is_reverse)\n --\t\t\told_offset -= delta;\n --\t\telse\n --\t\t\tnew_offset += delta;\n --\n --\t\tstrbuf_addf(out, \"@@ -%lu\", old_offset);\n --\t\tif (header->old_count != 1)\n --\t\t\tstrbuf_addf(out, \",%lu\", header->old_count);\n --\t\tstrbuf_addf(out, \" +%lu\", new_offset);\n --\t\tif (header->new_count != 1)\n --\t\t\tstrbuf_addf(out, \",%lu\", header->new_count);\n --\t\tstrbuf_addstr(out, \" @@\");\n -+\t\tif (!colored || !header->suppress_colored_line_range) {\n -+\t\t\tif (s->mode->is_reverse)\n -+\t\t\t\told_offset -= delta;\n -+\t\t\telse\n -+\t\t\t\tnew_offset += delta;\n + \t\tif (!colored) {\n + \t\t\tp = s->plain.buf + header->extra_start;\n + \t\t\tlen = header->extra_end - header->extra_start;\n ++\t\t} else if (header->suppress_colored_line_range) {\n ++\t\t\tstrbuf_add(out,\n ++\t\t\t\t s->colored.buf + header->colored_extra_start,\n ++\t\t\t\t header->colored_extra_end -\n ++\t\t\t\t header->colored_extra_start);\n +\n -+\t\t\tstrbuf_addf(out, \"@@ -%lu\", old_offset);\n -+\t\t\tif (header->old_count != 1)\n -+\t\t\t\tstrbuf_addf(out, \",%lu\", header->old_count);\n -+\t\t\tstrbuf_addf(out, \" +%lu\", new_offset);\n -+\t\t\tif (header->new_count != 1)\n -+\t\t\t\tstrbuf_addf(out, \",%lu\", header->new_count);\n -+\t\t\tstrbuf_addstr(out, \" @@\");\n -+\t\t}\n - \n - \t\tif (len)\n - \t\t\tstrbuf_add(out, p, len);\n ++\t\t\tstrbuf_add(out, s->colored.buf + hunk->colored_start,\n ++\t\t\t\t hunk->colored_end - hunk->colored_start);\n ++\t\t\treturn;\n + \t\t} else {\n + \t\t\tstrbuf_addstr(out, s->s.fraginfo_color);\n + \t\t\tp = s->colored.buf + header->colored_extra_start;\n \n ## t/t3701-add-interactive.sh ##\n @@ t/t3701-add-interactive.sh: test_expect_success 'detect bogus diffFilter output' '\n @@ t/t3701-add-interactive.sh: test_expect_success 'detect bogus diffFilter output'\n +\tprintf n >n &&\n +\tforce_color git -c interactive.diffFilter=\"sed s/.*@@.*/XX/\" \\\n +\t\tadd -p >output 2>&1 \n\nWhen parsing the colored version of a diff, the interactive `add`\ncommand really relies on the colored version having the same number of\nlines as the plain (uncolored) version. That is an invariant.\n\nWe already have code to verify correctly when the colored diff has less\nlines than the plain diff. Modulo an off-by-one bug: If the last diff\nline has no matching colored one, the code pretends to succeed, still.\n\nTo make matters worse, when we adjusted the test in 1e4ffc765db (t3701:\nadjust difffilter test, 2020-01-14), we did not catch this because `add\n-p` fails for a _different_ reason: it does not find any colored hunk\nheader that contains a parseable line range.\n\nIf we change the test case so that the line range _can_ be parsed, the\nbug is exposed.\n\nLet's address all of the above by\n\n- fixing the off-by-one,\n\n- adjusting the test case to allow `add -p` to parse the line range\n\n- making the test case more stringent by verifying that the expected\n error message is shown\n\nAlso adjust a misleading code comment about the now-fixed code.\n\nSigned-off-by: Johannes Schindelin \n---\n add-patch.c | 5 ++++-\n t/t3701-add-interactive.sh | 5 +++--\n 2 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 509ca04456b..34f3807ff32 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -592,7 +592,10 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n \t\t\tif (colored_eol)\n \t\t\t\tcolored_p = colored_eol + 1;\n \t\t\telse if (p != pend)\n-\t\t\t\t/* colored shorter than non-colored? */\n+\t\t\t\t/* non-colored has more lines? */\n+\t\t\t\tgoto mismatched_output;\n+\t\t\telse if (colored_p == colored_pend)\n+\t\t\t\t/* last line has no matching colored one? */\n \t\t\t\tgoto mismatched_output;\n \t\t\telse\n \t\t\t\tcolored_p = colored_pend;\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 3b7df9bed5a..8a594700f7b 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -761,9 +761,10 @@ test_expect_success 'detect bogus diffFilter output' '\n \tgit reset --hard &&\n \n \techo content >test &&\n-\ttest_config interactive.diffFilter \"sed 1d\" &&\n+\ttest_config interactive.diffFilter \"sed 6d\" &&\n \tprintf y >y &&\n-\tforce_color test_must_fail git add -p output 2>&1 &&\n+\tgrep \"mismatched output\" output\n '\n \n test_expect_success 'handle very large filtered diff' '\n-- \ngitgitgadget\n\n"},{"id":"462455","messageId":"47943b603b1f2330b36a3050fe7463bca6eeddef.1662046939.git.gitgitgadget@gmail.com","threadId":"58353","inReplyTo":"pull.1336.v5.git.1662046939.gitgitgadget@gmail.com","subject":"[PATCH v5 3/3] add -p: ignore dirty submodules","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-01T15:42:19Z","receivedAt":"2022-09-01T15:42:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThanks to alwyas running `diff-index` and `diff-files` with the\n`--numstat` option (the latter with `--ignore-submodules=dirty`) before\neven generating any real diff to parse, the Perl version of `git add -p`\nsimply ignored dirty submodules and does not even offer them up for\nstaging.\n\nHowever, the built-in variant did not use that flag because it tries to\nrun only one `diff` command, skipping the unneeded\n`diff-index`/`diff-files` invocation of the Perl variant and therefore\nonly faithfully recapitulates what the Perl code does once it _does_\ngenerate and parse the real diff.\n\nThis causes a problem when running the built-in `add -p` with\n`diff-so-fancy` because that diff colorizer always inserts an empty line\nbefore the diff header to ensure that it produces 4 lines as expected by\n`git add -p` (the equivalent of the non-colorized `diff`, `index`, `---`\nand `+++` lines). But `git diff-files` does not produce any `index` line\nfor dirty submodules.\n\nThe underlying problem is not even the discrepancy in lines, but that\n`git add -p` presents diffs for dirty submodules: there is nothing that\n_can_ be staged for those.\n\nLet's fix that bug, and teach the built-in `add -p` to ignore dirty\nsubmodules, too. This _incidentally_ also fixes the `diff-so-fancy`\nproblem ;-)\n\nSigned-off-by: Johannes Schindelin \n---\n add-patch.c | 3 ++-\n t/t3701-add-interactive.sh | 12 ++++++++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex a6bd150de51..a6596532867 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -419,7 +419,8 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n \t}\n \tcolor_arg_index = args.nr;\n \t/* Use `--no-color` explicitly, just in case `diff.color = always`. */\n-\tstrvec_pushl(&args, \"--no-color\", \"-p\", \"--\", NULL);\n+\tstrvec_pushl(&args, \"--no-color\", \"--ignore-submodules=dirty\", \"-p\",\n+\t\t \"--\", NULL);\n \tfor (i = 0; i < ps->nr; i++)\n \t\tstrvec_push(&args, ps->items[i].original);\n \ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex a94e7c53c8a..5a7a0ea7e83 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -955,6 +955,18 @@ test_expect_success 'status ignores dirty submodules (except HEAD)' '\n \t! grep dirty-otherwise output\n '\n \n+test_expect_success 'handle submodules' '\n+\techo 123 >>for-submodules/dirty-otherwise/initial.t &&\n+\n+\tforce_color git -C for-submodules add -p dirty-otherwise >output 2>&1 &&\n+\tgrep \"No changes\" output &&\n+\n+\tforce_color git -C for-submodules add -p dirty-head >output 2>&1 actual &&\n+\trev=\"$(git -C for-submodules/dirty-head rev-parse HEAD)\" &&\n+\tgrep \"$rev\" actual\n+'\n+\n test_expect_success 'set up pathological context' '\n \tgit reset --hard &&\n \ttest_write_lines a a a a a a a a a a a >a &&\n-- \ngitgitgadget\n"},{"id":"462456","messageId":"93d0e3b4d2a3dec5588c6e41d69313385356aa8f.1662046939.git.gitgitgadget@gmail.com","threadId":"58353","inReplyTo":"pull.1336.v5.git.1662046939.gitgitgadget@gmail.com","subject":"[PATCH v5 2/3] add -p: gracefully handle unparseable hunk headers in colored diffs","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-01T15:42:18Z","receivedAt":"2022-09-01T15:42:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn\nhttps://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com,\nPhillipe Blain reported that the built-in `git add -p` command fails\nwhen asked to use [`diff-so-fancy`][diff-so-fancy] to colorize the diff.\n\nThe reason is that this tool produces colored diffs with a hunk header\nthat does not contain any parseable `@@ ... @@` line range information,\nand therefore we cannot detect any part in that header that comes after\nthe line range.\n\nAs proposed by Phillip Wood, let's take that for a clear indicator that\nwe should show the hunk headers verbatim. This is what the Perl version\nof the interactive `add` command did, too.\n\n[diff-so-fancy]: https://github.com/so-fancy/diff-so-fancy\n\nReported-by: Philippe Blain \nHelped-by: Phillip Wood \nSigned-off-by: Johannes Schindelin \n---\n add-patch.c | 25 +++++++++++++++++--------\n t/t3701-add-interactive.sh | 10 ++++++++++\n 2 files changed, 27 insertions(+), 8 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 34f3807ff32..a6bd150de51 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -238,6 +238,7 @@ struct hunk_header {\n \t * include the newline.\n \t */\n \tsize_t extra_start, extra_end, colored_extra_start, colored_extra_end;\n+\tunsigned suppress_colored_line_range:1;\n };\n \n struct hunk {\n@@ -358,15 +359,14 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk)\n \tif (!eol)\n \t\teol = s->colored.buf + s->colored.len;\n \tp = memmem(line, eol - line, \"@@ -\", 4);\n-\tif (!p)\n-\t\treturn error(_(\"could not parse colored hunk header '%.*s'\"),\n-\t\t\t (int)(eol - line), line);\n-\tp = memmem(p + 4, eol - p - 4, \" @@\", 3);\n-\tif (!p)\n-\t\treturn error(_(\"could not parse colored hunk header '%.*s'\"),\n-\t\t\t (int)(eol - line), line);\n+\tif (p && (p = memmem(p + 4, eol - p - 4, \" @@\", 3))) {\n+\t\theader->colored_extra_start = p + 3 - s->colored.buf;\n+\t} else {\n+\t\t/* could not parse colored hunk header, leave as-is */\n+\t\theader->colored_extra_start = hunk->colored_start;\n+\t\theader->suppress_colored_line_range = 1;\n+\t}\n \thunk->colored_start = eol - s->colored.buf + (*eol == '\\n');\n-\theader->colored_extra_start = p + 3 - s->colored.buf;\n \theader->colored_extra_end = hunk->colored_start;\n \n \treturn 0;\n@@ -659,6 +659,15 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n \t\tif (!colored) {\n \t\t\tp = s->plain.buf + header->extra_start;\n \t\t\tlen = header->extra_end - header->extra_start;\n+\t\t} else if (header->suppress_colored_line_range) {\n+\t\t\tstrbuf_add(out,\n+\t\t\t\t s->colored.buf + header->colored_extra_start,\n+\t\t\t\t header->colored_extra_end -\n+\t\t\t\t header->colored_extra_start);\n+\n+\t\t\tstrbuf_add(out, s->colored.buf + hunk->colored_start,\n+\t\t\t\t hunk->colored_end - hunk->colored_start);\n+\t\t\treturn;\n \t\t} else {\n \t\t\tstrbuf_addstr(out, s->s.fraginfo_color);\n \t\t\tp = s->colored.buf + header->colored_extra_start;\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 8a594700f7b..a94e7c53c8a 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -767,6 +767,16 @@ test_expect_success 'detect bogus diffFilter output' '\n \tgrep \"mismatched output\" output\n '\n \n+test_expect_success 'handle iffy colored hunk headers' '\n+\tgit reset --hard &&\n+\n+\techo content >test &&\n+\tprintf n >n &&\n+\tforce_color git -c interactive.diffFilter=\"sed s/.*@@.*/XX/\" \\\n+\t\tadd -p >output 2>&1 Thanks to alwyas running `diff-index` and `diff-files` with the\n\nNot a big deal, but since it sounds like you're going to re-roll anyway:\n\n s/alwyas/always/\n\nI hadn't been closely following the earlier iterations of this series,\nbut the approach of this version definitely matches what I'd expected to\nsee based on our previous discussions about filters.\n\nModulo the coloring thing Phillip brought up in patch 2, it all looks\ngood to me. Thanks for working on it. I know you said earlier that the\nbugs we've found in the conversion make you hesitant to remove the perl\nversion for now. But to me, your responsiveness in fixing those bugs\ngives me more confidence in dumping the perl one. ;) (eventually, at\nleast)\n\n-Peff\n"},{"id":"462459","messageId":"YxDUpVz6im0pcPUi@coredump.intra.peff.net","threadId":"58353","inReplyTo":"YxDTrUI0kOkQr9q5@coredump.intra.peff.net","subject":"Re: [PATCH v4 3/3] add -p: ignore dirty submodules","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-01T15:49:57Z","receivedAt":"2022-09-01T15:50:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 01, 2022 at 11:45:49AM -0400, Jeff King wrote:\n\n> On Wed, Aug 31, 2022 at 08:31:17PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> \n> > Thanks to alwyas running `diff-index` and `diff-files` with the\n> \n> Not a big deal, but since it sounds like you're going to re-roll anyway:\n> \n> s/alwyas/always/\n\nDoh, looks like I raced you sending the new version. :)\n\nFWIW, the new iteration looks good to me.\n\n-Peff\n"},{"id":"462460","messageId":"a560ecb1-2049-7844-758f-4bd44b61eea7@gmail.com","threadId":"58353","inReplyTo":"93d0e3b4d2a3dec5588c6e41d69313385356aa8f.1662046939.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v5 2/3] add -p: gracefully handle unparseable hunk headers in colored diffs","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-09-01T16:03:23Z","receivedAt":"2022-09-01T16:03:29Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 01/09/2022 16:42, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> In\n> https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com,\n> Phillipe Blain reported that the built-in `git add -p` command fails\n> when asked to use [`diff-so-fancy`][diff-so-fancy] to colorize the diff.\n> \n> The reason is that this tool produces colored diffs with a hunk header\n> that does not contain any parseable `@@ ... @@` line range information,\n> and therefore we cannot detect any part in that header that comes after\n> the line range.\n> \n> As proposed by Phillip Wood, let's take that for a clear indicator that\n> we should show the hunk headers verbatim. This is what the Perl version\n> of the interactive `add` command did, too.\n> \n> [diff-so-fancy]: https://github.com/so-fancy/diff-so-fancy\n> \n> Reported-by: Philippe Blain \n> Helped-by: Phillip Wood \n> Signed-off-by: Johannes Schindelin \n> ---\n\n> @@ -659,6 +659,15 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n> \t\tif (!colored) {\n> \t\t\tp = s->plain.buf + header->extra_start;\n> \t\t\tlen = header->extra_end - header->extra_start;\n> +\t\t} else if (header->suppress_colored_line_range) {\n> +\t\t\tstrbuf_add(out,\n> +\t\t\t\t s->colored.buf + header->colored_extra_start,\n> +\t\t\t\t header->colored_extra_end -\n> +\t\t\t\t header->colored_extra_start);\n> +\n> +\t\t\tstrbuf_add(out, s->colored.buf + hunk->colored_start,\n> +\t\t\t\t hunk->colored_end - hunk->colored_start);\n> +\t\t\treturn;\n\nHaving an extra branch for this that returns early makes it easy to see \nwhat is being printed when we cannot parse the hunk header.\n\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index 8a594700f7b..a94e7c53c8a 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -767,6 +767,16 @@ test_expect_success 'detect bogus diffFilter output' '\n> \tgrep \"mismatched output\" output\n> '\n> \n> +test_expect_success 'handle iffy colored hunk headers' '\n> +\tgit reset --hard &&\n> +\n> +\techo content >test &&\n> +\tprintf n >n &&\n> +\tforce_color git -c interactive.diffFilter=\"sed s/.*@@.*/XX/\" \\\n> +\t\tadd -p >output 2>&1 +\tgrep \"^XX$\" output\n\nWe check that the hunk header line from filtered diff was reproduced \nverbatim\n\nThis all looks good to me\n\nBest Wishes\n\nPhillip\n"},{"id":"462461","messageId":"ced55750-21bb-c264-ef2d-666aca77fa2b@gmail.com","threadId":"58353","inReplyTo":"pull.1336.v5.git.1662046939.gitgitgadget@gmail.com","subject":"Re: [PATCH v5 0/3] built-in add -p: support diff-so-fancy better","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-09-01T16:04:15Z","receivedAt":"2022-09-01T16:04:23Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 01/09/2022 16:42, Johannes Schindelin via GitGitGadget wrote:\n> Philippe Blain reported in\n> https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com\n> that there is a problem when running the built-in version of git add -p with\n> diff-so-fancy [https://github.com/so-fancy/diff-so-fancy] as diff colorizer.\n> The symptom is this:\n> \n> error: could not parse colored hunk header '?[36m?[1m?[38;5;13m@ file:1 @?[1m?[0m'\n> \n> \n> This patch series addresses that and should fix\n> https://github.com/so-fancy/diff-so-fancy/issues/437\n> \n> Changes since v4:\n> \n> * The second patch was further simplified, avoiding to print extra ANSI\n> sequences if the hunk header is shown verbatim.\n\nThat was a quick re-roll! I'm very happy with this version\n\nThanks\n\nPhillip\n\n> Changes since v3:\n> \n> * Instead of deviating from how the Perl version of git add -p did things,\n> we now teach the built-in version to display hunk headers verbatim when\n> no line range could be parsed out (instead of showing the line range\n> anyways). This was a very good idea of Phillip's, dramatically\n> simplifying the patch series.\n> * Also, this iteration drops the first patch that claims to redefine what\n> we consider bogus, but only hides an off-by-one. In its stead, there is\n> now a patch that fixes said off-by-one.\n> \n> Changes since v2:\n> \n> * Added the appropriate \"Reported-by\" trailer to the commit message.\n> * Split out the logic to insert a space between the colored line range and\n> the extra information, if needed.\n> * That logic was now corrected to see whether that space is really needed.\n> * To verify that the logic does what we need it to do, the added regression\n> test now specifically tests for that (single) extra space that we want to\n> be inserted.\n> * Reworded a stale comment that claimed that we might suppress the entire\n> colored hunk header (which we no longer do).\n> * Rebased to the current tip of the main branch to avoid a merge conflict\n> with 716c1f649e3 (pipe_command(): mark stdin descriptor as non-blocking,\n> 2022-08-17).\n> \n> Changes since v1:\n> \n> * Added a commit to ignore dirty submodules just like the Perl version\n> does.\n> \n> Johannes Schindelin (3):\n> add -p: detect more mismatches between plain vs colored diffs\n> add -p: gracefully handle unparseable hunk headers in colored diffs\n> add -p: ignore dirty submodules\n> \n> add-patch.c | 33 +++++++++++++++++++++++----------\n> t/t3701-add-interactive.sh | 27 +++++++++++++++++++++++++--\n> 2 files changed, 48 insertions(+), 12 deletions(-)\n> \n> \n> base-commit: 07ee72db0e97b5c233f8ada0abb412248c2f1c6f\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1336%2Fdscho%2Fdiff-so-fancy-v5\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1336/dscho/diff-so-fancy-v5\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1336\n> \n> Range-diff vs v4:\n> \n> 1: 25187c3a3c2 = 1: 25187c3a3c2 add -p: detect more mismatches between plain vs colored diffs\n> 2: cd1c5100506 ! 2: 93d0e3b4d2a add -p: gracefully handle unparseable hunk headers in colored diffs\n> @@ Commit message\n> we should show the hunk headers verbatim. This is what the Perl version\n> of the interactive `add` command did, too.\n> \n> - This commit is best viewed with `--color-moved --ignore-space-change`.\n> -\n> [diff-so-fancy]: https://github.com/so-fancy/diff-so-fancy\n> \n> Reported-by: Philippe Blain \n> @@ add-patch.c: static int parse_hunk_header(struct add_p_state *s, struct hunk *hu\n> -\tif (!p)\n> -\t\treturn error(_(\"could not parse colored hunk header '%.*s'\"),\n> -\t\t\t (int)(eol - line), line);\n> -+\tif (p && (p = memmem(p + 4, eol - p - 4, \" @@\", 3)))\n> ++\tif (p && (p = memmem(p + 4, eol - p - 4, \" @@\", 3))) {\n> +\t\theader->colored_extra_start = p + 3 - s->colored.buf;\n> -+\telse {\n> ++\t} else {\n> +\t\t/* could not parse colored hunk header, leave as-is */\n> +\t\theader->colored_extra_start = hunk->colored_start;\n> +\t\theader->suppress_colored_line_range = 1;\n> @@ add-patch.c: static int parse_hunk_header(struct add_p_state *s, struct hunk *hu\n> \n> \treturn 0;\n> @@ add-patch.c: static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n> - \t\t\t\t- header->colored_extra_start;\n> - \t\t}\n> -\n> --\t\tif (s->mode->is_reverse)\n> --\t\t\told_offset -= delta;\n> --\t\telse\n> --\t\t\tnew_offset += delta;\n> --\n> --\t\tstrbuf_addf(out, \"@@ -%lu\", old_offset);\n> --\t\tif (header->old_count != 1)\n> --\t\t\tstrbuf_addf(out, \",%lu\", header->old_count);\n> --\t\tstrbuf_addf(out, \" +%lu\", new_offset);\n> --\t\tif (header->new_count != 1)\n> --\t\t\tstrbuf_addf(out, \",%lu\", header->new_count);\n> --\t\tstrbuf_addstr(out, \" @@\");\n> -+\t\tif (!colored || !header->suppress_colored_line_range) {\n> -+\t\t\tif (s->mode->is_reverse)\n> -+\t\t\t\told_offset -= delta;\n> -+\t\t\telse\n> -+\t\t\t\tnew_offset += delta;\n> + \t\tif (!colored) {\n> + \t\t\tp = s->plain.buf + header->extra_start;\n> + \t\t\tlen = header->extra_end - header->extra_start;\n> ++\t\t} else if (header->suppress_colored_line_range) {\n> ++\t\t\tstrbuf_add(out,\n> ++\t\t\t\t s->colored.buf + header->colored_extra_start,\n> ++\t\t\t\t header->colored_extra_end -\n> ++\t\t\t\t header->colored_extra_start);\n> +\n> -+\t\t\tstrbuf_addf(out, \"@@ -%lu\", old_offset);\n> -+\t\t\tif (header->old_count != 1)\n> -+\t\t\t\tstrbuf_addf(out, \",%lu\", header->old_count);\n> -+\t\t\tstrbuf_addf(out, \" +%lu\", new_offset);\n> -+\t\t\tif (header->new_count != 1)\n> -+\t\t\t\tstrbuf_addf(out, \",%lu\", header->new_count);\n> -+\t\t\tstrbuf_addstr(out, \" @@\");\n> -+\t\t}\n> -\n> - \t\tif (len)\n> - \t\t\tstrbuf_add(out, p, len);\n> ++\t\t\tstrbuf_add(out, s->colored.buf + hunk->colored_start,\n> ++\t\t\t\t hunk->colored_end - hunk->colored_start);\n> ++\t\t\treturn;\n> + \t\t} else {\n> + \t\t\tstrbuf_addstr(out, s->s.fraginfo_color);\n> + \t\t\tp = s->colored.buf + header->colored_extra_start;\n> \n> ## t/t3701-add-interactive.sh ##\n> @@ t/t3701-add-interactive.sh: test_expect_success 'detect bogus diffFilter output' '\n> @@ t/t3701-add-interactive.sh: test_expect_success 'detect bogus diffFilter output'\n> +\tprintf n >n &&\n> +\tforce_color git -c interactive.diffFilter=\"sed s/.*@@.*/XX/\" \\\n> +\t\tadd -p >output 2>&1 -+\tgrep \"^[^@]*XX[^@]*$\" output\n> ++\tgrep \"^XX$\" output\n> +'\n> +\n> test_expect_success 'handle very large filtered diff' '\n> 3: 116f0cf5cab = 3: 47943b603b1 add -p: ignore dirty submodules\n> \n"},{"id":"462464","messageId":"xmqqo7vzax2u.fsf@gitster.g","threadId":"58353","inReplyTo":"116f0cf5cabee3590957731740b33e800b762f34.1661977877.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 3/3] add -p: ignore dirty submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-01T16:17:13Z","receivedAt":"2022-09-01T16:17:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> Thanks to alwyas running `diff-index` and `diff-files` with the\n\n\"always\".\n\nI notice that this round (like the previous one, v3) is made not to\napply on 'maint'. As I said in the earlier review, in general I\nprefer to fork a bugfix topic out of 'maint' unless there is a\nstrong reason not to.\n\nIt is a different matter if all these bugfix topics are actually\nmerged down to 'maint' and tagged a new maintenance release by me.\nBut I'd prefer to make it easier for motivated distro packagers to\nadopt fixes that proves good in our 'master' front to their maint\nreleases, and it would be much easier to just merge a topic based on\n'maint' than having to cherry-pick a topic based on 'master'.\n\nThanks.\n"},{"id":"462466","messageId":"xmqqk06nawyg.fsf@gitster.g","threadId":"58353","inReplyTo":"77046611-03ae-3b0f-736a-c377a63274c2@gmail.com","subject":"Re: [PATCH v4 0/3] built-in add -p: support diff-so-fancy better","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-01T16:19:51Z","receivedAt":"2022-09-01T16:19:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> This version is looking much simpler and nicer, I've left a couple of\n> comments on the second patch as we're still not quite printing the\n> output of the diff filter verbatim.\n\nYes, this round looks a lot more sensible. Thanks, all.\n"},{"id":"462471","messageId":"xmqq35dbavco.fsf@gitster.g","threadId":"58353","inReplyTo":"ced55750-21bb-c264-ef2d-666aca77fa2b@gmail.com","subject":"Re: [PATCH v5 0/3] built-in add -p: support diff-so-fancy better","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-01T16:54:31Z","receivedAt":"2022-09-01T16:54:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Hi Dscho\n>\n> On 01/09/2022 16:42, Johannes Schindelin via GitGitGadget wrote:\n>> Philippe Blain reported in\n>> https://lore.kernel.org/git/ecf6f5be-22ca-299f-a8f1-bda38e5ca246@gmail.com\n>> that there is a problem when running the built-in version of git add -p with\n>> diff-so-fancy [https://github.com/so-fancy/diff-so-fancy] as diff colorizer.\n>> The symptom is this:\n>> error: could not parse colored hunk header\n>> '?[36m?[1m?[38;5;13m@ file:1 @?[1m?[0m'\n>> This patch series addresses that and should fix\n>> https://github.com/so-fancy/diff-so-fancy/issues/437\n>> Changes since v4:\n>> * The second patch was further simplified, avoiding to print extra\n>> ANSI\n>> sequences if the hunk header is shown verbatim.\n>\n> That was a quick re-roll! I'm very happy with this version\n\nYeah, looking good.\n"},{"id":"462472","messageId":"xmqqv8q79gr3.fsf@gitster.g","threadId":"58353","inReplyTo":"47943b603b1f2330b36a3050fe7463bca6eeddef.1662046939.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v5 3/3] add -p: ignore dirty submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-01T16:55:12Z","receivedAt":"2022-09-01T16:55:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> Thanks to alwyas running `diff-index` and `diff-files` with the\n\n\"always\". If I do not forget I'll amend before merging it to 'seen'.\n\nThanks.\n"},{"id":"462516","messageId":"sq60608q-2179-1737-q987-736o14s1731q@tzk.qr","threadId":"58353","inReplyTo":"xmqqo7vzax2u.fsf@gitster.g","subject":"Re: [PATCH v4 3/3] add -p: ignore dirty submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-09-02T08:53:01Z","receivedAt":"2022-09-02T08:53:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 1 Sep 2022, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > From: Johannes Schindelin \n> >\n> > Thanks to alwyas running `diff-index` and `diff-files` with the\n>\n> \"always\".\n>\n> I notice that this round (like the previous one, v3) is made not to\n> apply on 'maint'. As I said in the earlier review, in general I\n> prefer to fork a bugfix topic out of 'maint' unless there is a\n> strong reason not to.\n\nSorry, I thought that your comment was more about changing the base commit\ntoo often, not meant as \"please change it back for your next iteration\".\n\nI have changed it back, fixed the typo and force-pushed. In case a new\niteration is still required, that iteration will have these changes.\n\nCiao,\nDscho\n\n> It is a different matter if all these bugfix topics are actually\n> merged down to 'maint' and tagged a new maintenance release by me.\n> But I'd prefer to make it easier for motivated distro packagers to\n> adopt fixes that proves good in our 'master' front to their maint\n> releases, and it would be much easier to just merge a topic based on\n> 'maint' than having to cherry-pick a topic based on 'master'.\n>\n> Thanks.\n>\n>\n"}]}