{"thread":{"id":"61726","subject":"git crashes in `git commit --patch` with diff.suppressBlankEmpty = true","startedAt":"2024-07-03T14:41:45Z","lastAt":"2024-07-18T14:23:13Z","messageCount":12,"participants":["Ilya Tumaykin","Phillip Wood","Junio C Hamano","Jeff King","phillip.wood123@gmail.com"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"498031","messageId":"9b31e86f-c408-4625-8d13-f48a209b541b@gmail.com","threadId":"61726","inReplyTo":null,"subject":"git crashes in `git commit --patch` with diff.suppressBlankEmpty = true","fromName":"Ilya Tumaykin","fromEmail":"itumaykin@gmail.com","sentAt":"2024-07-03T14:41:40Z","receivedAt":"2024-07-03T14:41:45Z","isPatch":false,"sender":{"key":"itumaykin@gmail.com","avatar":null},"body":"Hello.\n\n`git commit --patch` crashes with diff.suppressBlankEmpty option enabled \nunder certain conditions.\n\n\nSteps to reproduce:\n1. Prepare .gitconfig:\n[user]\n\tname = User\n\temail = user@example.com\n[diff]\n\tsuppressBlankEmpty = true\n\n2. Initialize repo:\n$ mkdir git_bug && cd git_bug\n$ git init\n$ echo -e 'test\\n\\n test \\n\\ntest' > test.txt\n$ git add test.txt\n$ git commit test.txt -m 'initial'\n\n3. Make changes:\n$ echo -e 'test\\n\\n test\\ntest\\n \\n' > test.txt\n\n4. Try to commit new changes\n$ git commit --patch test.txt\n\n5. Try to split the first hunk, press 's' in the git-commit interactive \ninterface.\n\n\nActual results:\ndiff --git a/test.txt b/test.txt\nindex 366cd4b..611ca9d 100644\n--- a/test.txt\n+++ b/test.txt\n@@ -1,5 +1,6 @@\n test\n\n- test\n-\n+ test\n test\n+\n+\n(1/1) Stage this hunk [y,n,q,a,d,s,e,p,?]? s\nBUG: add-patch.c:994: unhandled diff marker: '\n'\nAborted (core dumped)\n\n\nExpected results:\ngit-commit splits the hunk and continues.\n\n\nComment:\nIf I set diff.suppressBlankEmpty = false, then I get the expected behavior.\n\ngit --version: 2.45.2\nOS: up-to-date Fedora 40\n\n-- \nBest regards.\nIlya Tumaykin.\n"},{"id":"498082","messageId":"ab974e62-098c-4200-bee3-7de8d9115516@gmail.com","threadId":"61726","inReplyTo":"9b31e86f-c408-4625-8d13-f48a209b541b@gmail.com","subject":"Re: git crashes in `git commit --patch` with diff.suppressBlankEmpty = true","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-04T13:14:56Z","receivedAt":"2024-07-04T13:14:59Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ilya\n\nThanks for reporting this and for the example reproduction. The problem \nis that the code that splits hunks expects context lines to begin with ' \n'. We could fix that fairly simply but I wonder if we should change \n'diff-index' and 'diff-files' to ignore diff.suppressBlankEmpty instead. \nThe plumbing diff commands already ignore most of the options that \nchange diff output so I'm not quite sure why they respect this \nparticular config setting. I've cc'd a few people to see what they think.\n\nBest Wishes\n\nPhillip\n\nOn 03/07/2024 15:41, Ilya Tumaykin wrote:\n> Hello.\n> \n> `git commit --patch` crashes with diff.suppressBlankEmpty option enabled \n> under certain conditions.\n> \n> \n> Steps to reproduce:\n> 1. Prepare .gitconfig:\n> [user]\n>     name = User\n>     email = user@example.com\n> [diff]\n>     suppressBlankEmpty = true\n> \n> 2. Initialize repo:\n> $ mkdir git_bug && cd git_bug\n> $ git init\n> $ echo -e 'test\\n\\n test \\n\\ntest' > test.txt\n> $ git add test.txt\n> $ git commit test.txt -m 'initial'\n> \n> 3. Make changes:\n> $ echo -e 'test\\n\\n test\\ntest\\n \\n' > test.txt\n> \n> 4. Try to commit new changes\n> $ git commit --patch test.txt\n> \n> 5. Try to split the first hunk, press 's' in the git-commit interactive \n> interface.\n> \n> \n> Actual results:\n> diff --git a/test.txt b/test.txt\n> index 366cd4b..611ca9d 100644\n> --- a/test.txt\n> +++ b/test.txt\n> @@ -1,5 +1,6 @@\n>  test\n> \n> - test\n> -\n> + test\n>  test\n> +\n> +\n> (1/1) Stage this hunk [y,n,q,a,d,s,e,p,?]? s\n> BUG: add-patch.c:994: unhandled diff marker: '\n> '\n> Aborted (core dumped)\n> \n> \n> Expected results:\n> git-commit splits the hunk and continues.\n> \n> \n> Comment:\n> If I set diff.suppressBlankEmpty = false, then I get the expected behavior.\n> \n> git --version: 2.45.2\n> OS: up-to-date Fedora 40\n> \n"},{"id":"498108","messageId":"xmqq4j937pyf.fsf@gitster.g","threadId":"61726","inReplyTo":"ab974e62-098c-4200-bee3-7de8d9115516@gmail.com","subject":"Re: git crashes in `git commit --patch` with diff.suppressBlankEmpty = true","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-05T16:39:52Z","receivedAt":"2024-07-05T16:39:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> ... but I wonder if we\n> should change 'diff-index' and 'diff-files' to ignore\n> diff.suppressBlankEmpty instead. The plumbing diff commands already\n> ignore most of the options that change diff output so I'm not quite\n> sure why they respect this particular config setting.\n\nVery true. Even though POSIX adopted the text:\n\n It is implementation-defined whether an empty unaffected line is\n written as an empty line or a line containing a single \n character.\n\nit merely allows the implementation to show an empty unaffected line\nas an empty line (where traditionally such an output was not allowed),\nand we probably want to give priority to consistent and stable output.\nIf the addition of diff.suppressBlankEmpty were done in the usually\nrecommended way to first add command line option to prove that the\nfeature is useful and then add configuration variable to pretend as\nif it were passed, then it would have been perfect. We then could\nhave made the plumbing to completely ignore the configuration to\nmake the output more stable, while allowing script writers a choice\nto invoke plumbing commands with explicit comand line options.\n\nBut that was not what happened, unfortunately.\n\nIf we really wanted to force the world line to where we did what we\nshould have done back then, I would say we need to do a two-step\ntransition.\n\n - Add the --[no-]suppress-blank-empty option from the command line\n to all commands in the diff family. Plumbing diff trio will\n still pay attention to diff.suppressBlankEmpty but when they see\n it is set to any non-default value (i.e. true) without being set\n by the new command line option, we loudly warn that we will fix\n this historical mistake in Git 3.0 and encourage script writers\n to update their invocation of plumbing diff trio to use the\n command line to custimze.\n\n - At Git 3.0, plumbing diff trio stops paying attention to the\n diff.suppressBlankEmpty configuration. By this time, the warning\n we gave in earlier versions would have helped existing scripts to\n migrate away from relying on it and if they want they would\n instead explicitly pass the command line option, so we stop\n warning.\n\nAs to the \"commit -p\" issue, I think the patch parser is in the\nwrong and needs to be corrected, period. As long as the patches\ngiven as input are well-formed, we should be prepared to grok\nthem (we even allow manual editing of patches, right?).\n\nThanks.\n\nhttps://pubs.opengroup.org/onlinepubs/9699919799/utilities/diff.html\n"},{"id":"498431","messageId":"20240710093610.GA2076910@coredump.intra.peff.net","threadId":"61726","inReplyTo":"xmqq4j937pyf.fsf@gitster.g","subject":"[RFC/PATCH] add-patch: handle splitting hunks with diff.suppressBlankEmpty","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-10T09:36:10Z","receivedAt":"2024-07-10T09:36:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 05, 2024 at 09:39:52AM -0700, Junio C Hamano wrote:\n\n> As to the \"commit -p\" issue, I think the patch parser is in the\n> wrong and needs to be corrected, period. As long as the patches\n> given as input are well-formed, we should be prepared to grok\n> them (we even allow manual editing of patches, right?).\n\nMaybe this?\n\n-- >8 --\nSubject: add-patch: handle splitting hunks with diff.suppressBlankEmpty\n\nWhen \"add -p\" parses diffs, it looks for context lines starting with a\nsingle space. But when diff.suppressBlankEmpty is in effect, an empty\ncontext line will omit the space, giving us a true empty line. This\nconfuses the parser, which is unable to split based on such a line.\n\nIt's tempting to say that we should just make sure that we generate a\ndiff without that option. But we may parse diffs not only generated by\nGit, but ones that users have manually edited. And POSIX calls the\ndecision of whether to print the space here \"implementation-defined\".\n\nSo let's handle both cases: a context line either starts with a space or\nconsists of a totally empty line.\n\nReported-by: Ilya Tumaykin \nSigned-off-by: Jeff King \n---\nI'm a little worried that this creates ambiguities, since I don't think\nwe are careful about following the hunk header's line counts. Imagine\nyou had an input like this:\n\n @@ -1,2 +1,2 @@\n -old\n +new\n stuff\n\n some garbage\n\nWe obviously know that \"some garbage\" is not a context line and is just\ntrailing junk, because it does not begin with \"-\", \"+\" or space. But\nwhat about the blank line in between? It looks like an empty context\nline, but we can only know that it is not by respecting the counts in\nthe hunk header.\n\nI don't think we'd ever generate this ourselves, but could somebody\nmanually edit a hunk into this shape? When I tried it in practice, it\nlooks like we fail to apply the result even before my patch, though. I'm\nnot sure why that is. If I put \"some garbage\" without the blank line, we\ncorrectly realize it should be discarded. It's possible I'm just holding\nit wrong.\n\n add-patch.c | 8 ++++----\n t/t3701-add-interactive.sh | 32 ++++++++++++++++++++++++++++++++\n 2 files changed, 36 insertions(+), 4 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 6e176cd21a..7beead1d0a 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -588,7 +588,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n \t\t\t (int)(eol - (plain->buf + file_diff->head.start)),\n \t\t\t plain->buf + file_diff->head.start);\n \n-\t\tif ((marker == '-' || marker == '+') && *p == ' ')\n+\t\tif ((marker == '-' || marker == '+') && (*p == ' ' || *p == '\\n'))\n \t\t\thunk->splittable_into++;\n \t\tif (marker && *p != '\\\\')\n \t\t\tmarker = *p;\n@@ -964,7 +964,7 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,\n \t\t * Is this the first context line after a chain of +/- lines?\n \t\t * Then record the start of the next split hunk.\n \t\t */\n-\t\tif ((marker == '-' || marker == '+') && ch == ' ') {\n+\t\tif ((marker == '-' || marker == '+') && (ch == ' ' || ch == '\\n')) {\n \t\t\tfirst = 0;\n \t\t\thunk[1].start = current;\n \t\t\tif (colored)\n@@ -979,14 +979,14 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,\n \t\t * Then just increment the appropriate counter and continue\n \t\t * with the next line.\n \t\t */\n-\t\tif (marker != ' ' || (ch != '-' && ch != '+')) {\n+\t\tif ((marker != ' ' && marker != '\\n') || (ch != '-' && ch != '+')) {\n next_hunk_line:\n \t\t\t/* Comment lines are attached to the previous line */\n \t\t\tif (ch == '\\\\')\n \t\t\t\tch = marker ? marker : ' ';\n \n \t\t\t/* current hunk not done yet */\n-\t\t\tif (ch == ' ')\n+\t\t\tif (ch == ' ' || ch == '\\n')\n \t\t\t\tcontext_line_count++;\n \t\t\telse if (ch == '-')\n \t\t\t\theader->old_count++;\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 5d78868ac1..92c8e6dc8c 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -1164,4 +1164,36 @@ test_expect_success 'reset -p with unmerged files' '\n \ttest_must_be_empty staged\n '\n \n+test_expect_success 'splitting handles diff.suppressBlankEmpty' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\tcat >file <<-\\EOF &&\n+\t1\n+\t2\n+\n+\t3\n+\t4\n+\tEOF\n+\tgit add file &&\n+\n+\tcat >file <<-\\EOF &&\n+\tone\n+\ttwo\n+\n+\tthree\n+\tfour\n+\tEOF\n+\ttest_write_lines s n y |\n+\tgit -c diff.suppressBlankEmpty=true add -p &&\n+\n+\tgit cat-file blob :file >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t1\n+\t2\n+\n+\tthree\n+\tfour\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.45.2.1249.gb036353db5\n\n"},{"id":"498447","messageId":"d5d27cad-bacb-4a79-bb50-e65d2bb6808b@gmail.com","threadId":"61726","inReplyTo":"20240710093610.GA2076910@coredump.intra.peff.net","subject":"Re: [RFC/PATCH] add-patch: handle splitting hunks with diff.suppressBlankEmpty","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-10T13:46:30Z","receivedAt":"2024-07-10T13:46:32Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Peff\n\nOn 10/07/2024 10:36, Jeff King wrote:\n> Subject: add-patch: handle splitting hunks with diff.suppressBlankEmpty\n\n> When \"add -p\" parses diffs, it looks for context lines starting with a\n> single space. But when diff.suppressBlankEmpty is in effect, an empty\n> context line will omit the space, giving us a true empty line. This\n> confuses the parser, which is unable to split based on such a line.\n\n> It's tempting to say that we should just make sure that we generate a\n> diff without that option. But we may parse diffs not only generated by\n> Git, but ones that users have manually edited. And POSIX calls the\n> decision of whether to print the space here \"implementation-defined\".\n\nDo we ever parse an edited hunk with this code? I'm not sure there is a\nway of splitting a hunk after it has been edited as I don't think we\never display it again.\n\n> So let's handle both cases: a context line either starts with a space or\n> consists of a totally empty line.\n> \n> Reported-by: Ilya Tumaykin \n> Signed-off-by: Jeff King \n> ---\n> I'm a little worried that this creates ambiguities, since I don't think\n> we are careful about following the hunk header's line counts. Imagine\n> you had an input like this:\n> \n> @@ -1,2 +1,2 @@> \n> -old\n> +new\n> stuff\n> \n> some garbage\n> \n> We obviously know that \"some garbage\" is not a context line and is just\n> trailing junk, because it does not begin with \"-\", \"+\" or space. But\n> what about the blank line in between? It looks like an empty context\n> line, but we can only know that it is not by respecting the counts in\n> the hunk header.\n> \n> I don't think we'd ever generate this ourselves, but could somebody\n> manually edit a hunk into this shape? When I tried it in practice, it\n> looks like we fail to apply the result even before my patch, though. I'm> not sure why that is. If I put \"some garbage\" without the blank line, we\n> correctly realize it should be discarded. It's possible I'm just holding\n> it wrong.\n\nWhen we recount the hunk after it has been edited we ignore lines that\ndon't begin with '+', '-', ' ', or '\\n' so if you add some garbage at\nthe end of the hunk the recounted hunk header excludes it when it gets\napplied.\n\nI think your patch looks good. I did wonder if we wanted to fix this\nby normalizing context lines instead as shown in the diff below. That\nmight make it less likely to miss adding \"|| '\\n'\" in future code that\nis looking for a context line but I don't have a strong preference\neither way.\n\nBest Wishes\n\nPhillip\n\n---- >8 ----\ndiff --git a/add-patch.c b/add-patch.c\nindex d8ea05ff108..795aa772b7a 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -400,6 +400,12 @@ static void complete_file(char marker, struct hunk *hunk)\n \t\thunk->splittable_into++;\n }\n \n+/* Empty context lines may omit the leading ' ' */\n+static int normalize_marker(char marker)\n+{\n+\treturn marker == '\\n' ? ' ' : marker;\n+}\n+\n static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n {\n \tstruct strvec args = STRVEC_INIT;\n@@ -485,6 +491,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n \twhile (p != pend) {\n \t\tchar *eol = memchr(p, '\\n', pend - p);\n \t\tconst char *deleted = NULL, *mode_change = NULL;\n+\t\tchar ch = normalize_marker(*p);\n \n \t\tif (!eol)\n \t\t\teol = pend;\n@@ -532,7 +539,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n \t\t\t * Start counting into how many hunks this one can be\n \t\t\t * split\n \t\t\t */\n-\t\t\tmarker = *p;\n+\t\t\tmarker = ch;\n \t\t} else if (hunk == &file_diff->head &&\n \t\t\t starts_with(p, \"new file\")) {\n \t\t\tfile_diff->added = 1;\n@@ -586,10 +593,10 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n \t\t\t (int)(eol - (plain->buf + file_diff->head.start)),\n \t\t\t plain->buf + file_diff->head.start);\n \n-\t\tif ((marker == '-' || marker == '+') && *p == ' ')\n+\t\tif ((marker == '-' || marker == '+') && ch == ' ')\n \t\t\thunk->splittable_into++;\n-\t\tif (marker && *p != '\\\\')\n-\t\t\tmarker = *p;\n+\t\tif (marker && ch != '\\\\')\n+\t\t\tmarker = ch;\n \n \t\tp = eol == pend ? pend : eol + 1;\n \t\thunk->end = p - plain->buf;\n@@ -953,7 +960,7 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,\n \tcontext_line_count = 0;\n \n \twhile (splittable_into > 1) {\n-\t\tch = s->plain.buf[current];\n+\t\tch = normalize_marker(s->plain.buf[current]);\n \n \t\tif (!ch)\n \t\t\tBUG(\"buffer overrun while splitting hunks\");\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 5d78868ac16..385c246baf0 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -1164,4 +1164,31 @@ test_expect_success 'reset -p with unmerged files' '\n \ttest_must_be_empty staged\n '\n \n+test_expect_success 'hunk splitting works with diff.suppressBlankEmpty' '\n+\tcat >expect <<-\\EOF &&\n+\tdiff --git a/file b/file\n+\tindex 777b174..ebc9684 100755\n+\t--- a/file\n+\t+++ b/file\n+\t@@ -2,6 +2,6 @@ p\n+\t q\n+\t r\n+\n+\t-d\n+\t-e\n+\t-f\n+\t+s\n+\t+t\n+\t+u\n+\tEOF\n+\n+\ttest_config diff.suppressBlankEmpty true &&\n+\ttest_write_lines a b c \"\" d e f >file &&\n+\tgit add file &&\n+\ttest_write_lines p q r \"\" s t u >file &&\n+\ttest_write_lines s y n q | git add -p &&\n+\tgit diff >actual &&\n+\tdiff_cmp expect actual\n+'\n+\n test_done\n"},{"id":"498468","messageId":"xmqqed81xjl7.fsf@gitster.g","threadId":"61726","inReplyTo":"20240710093610.GA2076910@coredump.intra.peff.net","subject":"Re: [RFC/PATCH] add-patch: handle splitting hunks with diff.suppressBlankEmpty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-10T17:06:28Z","receivedAt":"2024-07-10T17:06:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'm a little worried that this creates ambiguities, since I don't think\n> we are careful about following the hunk header's line counts. Imagine\n> you had an input like this:\n>\n> @@ -1,2 +1,2 @@\n> -old\n> +new\n> stuff\n>\n> some garbage\n\nTrue. Especially if you allow editing of hunks, the only thing you\ncould make available to you are the version you gave the user and\nthe version you got back from the user, but comparing them would not\nhelp resolve the ambiguity to correct the hunk header all that much.\n\"diff\" edit mode various editors may have _could_ help as they know\neach and every keystroke by the user and how the modified patch was\nconstructed to guess the intention better than a mere comparison of\nbefore- and after- shape of the patch (but the last time I checked,\ndiff edit mode of GNU Emacs did not adjust the hunk header correctly\nin some corner cases).\n\n> I don't think we'd ever generate this ourselves, but could somebody\n> manually edit a hunk into this shape? When I tried it in practice, it\n> looks like we fail to apply the result even before my patch, though. I'm\n> not sure why that is. If I put \"some garbage\" without the blank line, we\n> correctly realize it should be discarded. It's possible I'm just holding\n> it wrong.\n\nI've given up on the \"hunk edit\" doing wrong things to a patch\nalready. \n\nThe \"edit\" codepath used to be a lot less careless before Phillip\nwhipped it into a much better shape with the series that ended at\n436d18f2 (Merge branch 'pw/add-p-recount', 2018-03-14), that\nintroduced recount_edited_hunk() that special cases \"+/-/ \". It\nalready knows that a completely empty line in a patch is an empty\nand unmodified line (which was ported from f4d35a6b (add -p: fix\ncounting empty context lines in edited patches, 2018-06-11)), so\nthis patch does not have to do anything new there.\n\nBut \"recounting\" will be fooled by garbage left in the edited\nresult, so I think we have done all we can do to resolve possible\nambiguities. The patch under discussion is not adding anything new\nand making it any worse, I would say.\n\n> diff --git a/add-patch.c b/add-patch.c\n> index 6e176cd21a..7beead1d0a 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -588,7 +588,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n> \t\t\t (int)(eol - (plain->buf + file_diff->head.start)),\n> \t\t\t plain->buf + file_diff->head.start);\n> \n> -\t\tif ((marker == '-' || marker == '+') && *p == ' ')\n> +\t\tif ((marker == '-' || marker == '+') && (*p == ' ' || *p == '\\n'))\n> \t\t\thunk->splittable_into++;\n> \t\tif (marker && *p != '\\\\')\n> \t\t\tmarker = *p;\n> @@ -964,7 +964,7 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,\n> \t\t * Is this the first context line after a chain of +/- lines?\n> \t\t * Then record the start of the next split hunk.\n> \t\t */\n> -\t\tif ((marker == '-' || marker == '+') && ch == ' ') {\n> +\t\tif ((marker == '-' || marker == '+') && (ch == ' ' || ch == '\\n')) {\n> \t\t\tfirst = 0;\n> \t\t\thunk[1].start = current;\n> \t\t\tif (colored)\n> @@ -979,14 +979,14 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,\n> \t\t * Then just increment the appropriate counter and continue\n> \t\t * with the next line.\n> \t\t */\n> -\t\tif (marker != ' ' || (ch != '-' && ch != '+')) {\n> +\t\tif ((marker != ' ' && marker != '\\n') || (ch != '-' && ch != '+')) {\n> next_hunk_line:\n> \t\t\t/* Comment lines are attached to the previous line */\n> \t\t\tif (ch == '\\\\')\n> \t\t\t\tch = marker ? marker : ' ';\n> \n> \t\t\t/* current hunk not done yet */\n> -\t\t\tif (ch == ' ')\n> +\t\t\tif (ch == ' ' || ch == '\\n')\n> \t\t\t\tcontext_line_count++;\n> \t\t\telse if (ch == '-')\n> \t\t\t\theader->old_count++;\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index 5d78868ac1..92c8e6dc8c 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -1164,4 +1164,36 @@ test_expect_success 'reset -p with unmerged files' '\n> \ttest_must_be_empty staged\n> '\n> \n> +test_expect_success 'splitting handles diff.suppressBlankEmpty' '\n> +\ttest_when_finished \"git reset --hard\" &&\n> +\tcat >file <<-\\EOF &&\n> +\t1\n> +\t2\n> +\n> +\t3\n> +\t4\n> +\tEOF\n> +\tgit add file &&\n> +\n> +\tcat >file <<-\\EOF &&\n> +\tone\n> +\ttwo\n> +\n> +\tthree\n> +\tfour\n> +\tEOF\n> +\ttest_write_lines s n y |\n> +\tgit -c diff.suppressBlankEmpty=true add -p &&\n> +\n> +\tgit cat-file blob :file >actual &&\n> +\tcat >expect <<-\\EOF &&\n> +\t1\n> +\t2\n> +\n> +\tthree\n> +\tfour\n> +\tEOF\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n"},{"id":"498539","messageId":"20240711212628.GA3648684@coredump.intra.peff.net","threadId":"61726","inReplyTo":"d5d27cad-bacb-4a79-bb50-e65d2bb6808b@gmail.com","subject":"Re: [RFC/PATCH] add-patch: handle splitting hunks with diff.suppressBlankEmpty","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-11T21:26:28Z","receivedAt":"2024-07-11T21:26:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 10, 2024 at 02:46:30PM +0100, Phillip Wood wrote:\n\n> > It's tempting to say that we should just make sure that we generate a\n> > diff without that option. But we may parse diffs not only generated by\n> > Git, but ones that users have manually edited. And POSIX calls the\n> > decision of whether to print the space here \"implementation-defined\".\n> \n> Do we ever parse an edited hunk with this code? I'm not sure there is a\n> way of splitting a hunk after it has been edited as I don't think we\n> ever display it again.\n\nHmm, I just assumed this code would see the edited hunk, but now I'm not\nsure. Note that the changes required do go outside of split_hunk(); the\ninitial parse_diff() needs to decide whether the hunk is splittable in\nthe first place (as an aside, that puzzled me at first why just changing\nsplit_hunk() was enough for the case that started this thread, but not\nthe one in the included test. The difference is that without the empty\nline as context, the hunk in the test wouldn't be splittable at all).\n\nBut looking closer: yes, I do think parse_diff() is used only for the\ninitial patch. So we really would only see git-generated patches here.\nWhich I think takes away my ambiguity concern, but does mean the commit\nmessage is wrong.\n\n> > I don't think we'd ever generate this ourselves, but could somebody\n> > manually edit a hunk into this shape? When I tried it in practice, it\n> > looks like we fail to apply the result even before my patch, though. I'm\n> > not sure why that is. If I put \"some garbage\" without the blank line, we\n> > correctly realize it should be discarded. It's possible I'm just holding\n> > it wrong.\n> \n> When we recount the hunk after it has been edited we ignore lines that\n> don't begin with '+', '-', ' ', or '\\n' so if you add some garbage at\n> the end of the hunk the recounted hunk header excludes it when it gets\n> applied.\n\nOK, that makes sense. And we could never rely on the hunk header in the\nedited hunk anyway, since the whole point is that we have to recount it.\nSo the user must accept that an extra blank line is potential context\n(and that goes all the way back to bcdd297b78 (built-in add -p:\nimplement hunk editing, 2019-12-13).\n\n> I think your patch looks good. I did wonder if we wanted to fix this\n> by normalizing context lines instead as shown in the diff below. That\n> might make it less likely to miss adding \"|| '\\n'\" in future code that\n> is looking for a context line but I don't have a strong preference\n> either way.\n\nYeah, I had a similar thought, but it got messy because we have to deal\nwith the source buffer. But the extra \"char ch\" you added in the patch\nbelow fixes that. I think the result is better.\n\nLooking at the blank-line handling in recount_edited_hunk(), we also\nhandle a CRLF empty line there. Should we do so here, too? If so, then\nit would just be a matter of touching normalize_marker() in your patch.\n\nDo you want to just re-send your patch with a commit message to replace\nmine? (Feel free to steal the non-wrong parts of my message ;) ).\n\n> ---- >8 ----\n> diff --git a/add-patch.c b/add-patch.c\n> index d8ea05ff108..795aa772b7a 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -400,6 +400,12 @@ static void complete_file(char marker, struct hunk *hunk)\n> \t\thunk->splittable_into++;\n> }\n> +/* Empty context lines may omit the leading ' ' */\n> +static int normalize_marker(char marker)\n> +{\n> +\treturn marker == '\\n' ? ' ' : marker;\n> +}\n> +\n> static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n\nMinor nit: missing blank line between functions.\n\n-Peff\n"},{"id":"498540","messageId":"20240711213208.GB3648684@coredump.intra.peff.net","threadId":"61726","inReplyTo":"xmqqed81xjl7.fsf@gitster.g","subject":"Re: [RFC/PATCH] add-patch: handle splitting hunks with diff.suppressBlankEmpty","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-11T21:32:08Z","receivedAt":"2024-07-11T21:32:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 10, 2024 at 10:06:28AM -0700, Junio C Hamano wrote:\n\n> > I don't think we'd ever generate this ourselves, but could somebody\n> > manually edit a hunk into this shape? When I tried it in practice, it\n> > looks like we fail to apply the result even before my patch, though. I'm\n> > not sure why that is. If I put \"some garbage\" without the blank line, we\n> > correctly realize it should be discarded. It's possible I'm just holding\n> > it wrong.\n> \n> I've given up on the \"hunk edit\" doing wrong things to a patch\n> already. \n> \n> The \"edit\" codepath used to be a lot less careless before Phillip\n> whipped it into a much better shape with the series that ended at\n> 436d18f2 (Merge branch 'pw/add-p-recount', 2018-03-14), that\n> introduced recount_edited_hunk() that special cases \"+/-/ \". It\n> already knows that a completely empty line in a patch is an empty\n> and unmodified line (which was ported from f4d35a6b (add -p: fix\n> counting empty context lines in edited patches, 2018-06-11)), so\n> this patch does not have to do anything new there.\n\nYeah, I was being paranoid without actually thinking through the\nimplications. As Phillip noted, we do not even run the code I changed on\nthe edited hunk (which already does handle the empty line). So not only\ncould I not break it, but it is already doing the right thing thanks to\nthat earlier work.\n\n> But \"recounting\" will be fooled by garbage left in the edited\n> result, so I think we have done all we can do to resolve possible\n> ambiguities. The patch under discussion is not adding anything new\n> and making it any worse, I would say.\n\nYep, agreed. So modulo a slight inaccuracy in the commit message, I\nthink my patch is OK. That said, I like what Phillip showed in his\nreply. If he wants to wrap that up into a patch, I think it could\nreplace mine.\n\n-Peff\n"},{"id":"498612","messageId":"622849f1-de52-4b92-9465-931014c8d3eb@gmail.com","threadId":"61726","inReplyTo":"20240711212628.GA3648684@coredump.intra.peff.net","subject":"Re: [RFC/PATCH] add-patch: handle splitting hunks with diff.suppressBlankEmpty","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-13T13:19:39Z","receivedAt":"2024-07-13T13:19:47Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Peff\n\nOn 11/07/2024 22:26, Jeff King wrote:\n> On Wed, Jul 10, 2024 at 02:46:30PM +0100, Phillip Wood wrote:\n >\n>> I think your patch looks good. I did wonder if we wanted to fix this\n>> by normalizing context lines instead as shown in the diff below. That\n>> might make it less likely to miss adding \"|| '\\n'\" in future code that\n>> is looking for a context line but I don't have a strong preference\n>> either way.\n> \n> Yeah, I had a similar thought, but it got messy because we have to deal\n> with the source buffer. But the extra \"char ch\" you added in the patch\n> below fixes that. I think the result is better.\n> \n> Looking at the blank-line handling in recount_edited_hunk(), we also\n> handle a CRLF empty line there. Should we do so here, too? If so, then\n> it would just be a matter of touching normalize_marker() in your patch.\n> \n> Do you want to just re-send your patch with a commit message to replace\n> mine? (Feel free to steal the non-wrong parts of my message ;) ).\n\nThanks, I'll do that\n\nBest Wishes\n\nPhillip\n\n>> ---- >8 ----\n>> diff --git a/add-patch.c b/add-patch.c\n>> index d8ea05ff108..795aa772b7a 100644\n>> --- a/add-patch.c\n>> +++ b/add-patch.c\n>> @@ -400,6 +400,12 @@ static void complete_file(char marker, struct hunk *hunk)\n>> \t\thunk->splittable_into++;\n>> }\n>> +/* Empty context lines may omit the leading ' ' */\n>> +static int normalize_marker(char marker)\n>> +{\n>> +\treturn marker == '\\n' ? ' ' : marker;\n>> +}\n>> +\n>> static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n> \n> Minor nit: missing blank line between functions.\n> \n> -Peff\n"},{"id":"498650","messageId":"20240713212105.GB554779@coredump.intra.peff.net","threadId":"61726","inReplyTo":"622849f1-de52-4b92-9465-931014c8d3eb@gmail.com","subject":"Re: [RFC/PATCH] add-patch: handle splitting hunks with diff.suppressBlankEmpty","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-13T21:21:05Z","receivedAt":"2024-07-13T21:21:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 13, 2024 at 02:19:39PM +0100, phillip.wood123@gmail.com wrote:\n\n> > Do you want to just re-send your patch with a commit message to replace\n> > mine? (Feel free to steal the non-wrong parts of my message ;) ).\n> \n> Thanks, I'll do that\n\nThanks. Note that mine is in 'next', but I think it would not be a big\ndeal to revert and replace (I'm not sure it is even on track for 2.46\nanyway).\n\n-Peff\n"},{"id":"498919","messageId":"fce1483f-a963-48ef-9613-ec683e34ec6a@gmail.com","threadId":"61726","inReplyTo":"20240713212105.GB554779@coredump.intra.peff.net","subject":"Re: [RFC/PATCH] add-patch: handle splitting hunks with diff.suppressBlankEmpty","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-18T14:22:58Z","receivedAt":"2024-07-18T14:23:01Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 13/07/2024 22:21, Jeff King wrote:\n> On Sat, Jul 13, 2024 at 02:19:39PM +0100, phillip.wood123@gmail.com wrote:\n> \n>>> Do you want to just re-send your patch with a commit message to replace\n>>> mine? (Feel free to steal the non-wrong parts of my message ;) ).\n>>\n>> Thanks, I'll do that\n> \n> Thanks. Note that mine is in 'next', but I think it would not be a big\n> deal to revert and replace (I'm not sure it is even on track for 2.46\n> anyway).\n\nThanks for the heads-up, I see that Junio has reverted it in the latest \nwhat's cooking email\n\nPhillip\n\n> -Peff\n> \n"},{"id":"498920","messageId":"3220d5f6-b771-4972-9c63-3f39d9ff1450@gmail.com","threadId":"61726","inReplyTo":"20240711212628.GA3648684@coredump.intra.peff.net","subject":"Re: [RFC/PATCH] add-patch: handle splitting hunks with diff.suppressBlankEmpty","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-18T14:23:10Z","receivedAt":"2024-07-18T14:23:13Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 11/07/2024 22:26, Jeff King wrote:\n> On Wed, Jul 10, 2024 at 02:46:30PM +0100, Phillip Wood wrote:\n> \n>>> It's tempting to say that we should just make sure that we generate a\n>>> diff without that option. But we may parse diffs not only generated by\n>>> Git, but ones that users have manually edited. And POSIX calls the\n>>> decision of whether to print the space here \"implementation-defined\".\n>>\n>> Do we ever parse an edited hunk with this code? I'm not sure there is a\n>> way of splitting a hunk after it has been edited as I don't think we\n>> ever display it again.\n> \n> Hmm, I just assumed this code would see the edited hunk, but now I'm not\n> sure. Note that the changes required do go outside of split_hunk(); the\n> initial parse_diff() needs to decide whether the hunk is splittable in\n> the first place (as an aside, that puzzled me at first why just changing\n> split_hunk() was enough for the case that started this thread, but not\n> the one in the included test. The difference is that without the empty\n> line as context, the hunk in the test wouldn't be splittable at all).\n> \n> But looking closer: yes, I do think parse_diff() is used only for the\n> initial patch.\n\nThat's true but I've realized that we do in fact allow the user to \nre-display and split an edited hunk. If there are two changes in a file \nthen you can edit the first hunk and when prompted about the second \npress 'K' to go back to the first one and then split it with 's' (I \nmessed up my test for this before sending my previous mail as I changed \ntwo separate files rather than putting two changes in a single file). So \nsplit_hunk() can encounter empty context lines even without \ndiff.suppressBlankEmpty being set as lots of editors strip the leading \nspace when the rest of the line is empty[1].\n\nAs we haven't had any bug reports about that I suspect people are not \nsplitting the hunks they edited which I guess makes sense. I think there \nis another bug lurking for anyone who does try to split an edited hunk \nas we don't update `hunk->splittable_into` after it has been edited and \nthe edit might change a deleted of an empty line to a context line or \nvice versa. We should make sure any garbage at the end of the hunk is \ndiscarded as well so we don't show it when we display the edited hunk.\n\nBest Wishes\n\nPhillip\n\n[1] When I added the code to recount the hunk header rather than relying \non \"git apply --recount\" initially it did not support empty context \nlines and we received quite a few bug reports pretty quickly after it \nwas released.\n\n> So we really would only see git-generated patches here.\n> Which I think takes away my ambiguity concern, but does mean the commit\n> message is wrong.\n> \n>>> I don't think we'd ever generate this ourselves, but could somebody\n>>> manually edit a hunk into this shape? When I tried it in practice, it\n>>> looks like we fail to apply the result even before my patch, though. I'm\n>>> not sure why that is. If I put \"some garbage\" without the blank line, we\n>>> correctly realize it should be discarded. It's possible I'm just holding\n>>> it wrong.\n>>\n>> When we recount the hunk after it has been edited we ignore lines that\n>> don't begin with '+', '-', ' ', or '\\n' so if you add some garbage at\n>> the end of the hunk the recounted hunk header excludes it when it gets\n>> applied.\n> \n> OK, that makes sense. And we could never rely on the hunk header in the\n> edited hunk anyway, since the whole point is that we have to recount it.\n> So the user must accept that an extra blank line is potential context\n> (and that goes all the way back to bcdd297b78 (built-in add -p:\n> implement hunk editing, 2019-12-13).\n> \n>> I think your patch looks good. I did wonder if we wanted to fix this\n>> by normalizing context lines instead as shown in the diff below. That\n>> might make it less likely to miss adding \"|| '\\n'\" in future code that\n>> is looking for a context line but I don't have a strong preference\n>> either way.\n> \n> Yeah, I had a similar thought, but it got messy because we have to deal\n> with the source buffer. But the extra \"char ch\" you added in the patch\n> below fixes that. I think the result is better.\n> \n> Looking at the blank-line handling in recount_edited_hunk(), we also\n> handle a CRLF empty line there. Should we do so here, too? If so, then\n> it would just be a matter of touching normalize_marker() in your patch.\n> \n> Do you want to just re-send your patch with a commit message to replace\n> mine? (Feel free to steal the non-wrong parts of my message ;) ).\n> \n>> ---- >8 ----\n>> diff --git a/add-patch.c b/add-patch.c\n>> index d8ea05ff108..795aa772b7a 100644\n>> --- a/add-patch.c\n>> +++ b/add-patch.c\n>> @@ -400,6 +400,12 @@ static void complete_file(char marker, struct hunk *hunk)\n>> \t\thunk->splittable_into++;\n>> }\n>> +/* Empty context lines may omit the leading ' ' */\n>> +static int normalize_marker(char marker)\n>> +{\n>> +\treturn marker == '\\n' ? ' ' : marker;\n>> +}\n>> +\n>> static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n> \n> Minor nit: missing blank line between functions.\n> \n> -Peff\n> \n\n"}]}