{"thread":{"id":"57123","subject":"[PATCH 0/2] builtin add -p: fix hunk splitting","startedAt":"2021-12-20T14:33:02Z","lastAt":"2022-01-24T11:10:51Z","messageCount":21,"participants":["Phillip Wood via GitGitGadget","Ævar Arnfjörð Bjarmason","Junio C Hamano","Phillip Wood","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"444469","messageId":"pull.1100.git.1640010777.gitgitgadget@gmail.com","threadId":"57123","inReplyTo":null,"subject":"[PATCH 0/2] builtin add -p: fix hunk splitting","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-20T14:32:55Z","receivedAt":"2021-12-20T14:33:02Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Fix a small regression in the hunk splitting of the builtin version compared\nto the perl version. Thanks to Szeder for the easy to follow bug report.\n\nPhillip Wood (2):\n  t3701: clean up hunk splitting tests\n  builtin add -p: fix hunk splitting\n\n add-patch.c                |  7 ++++++\n t/t3701-add-interactive.sh | 48 ++++++++++++++++++++++++++++++++++----\n 2 files changed, 50 insertions(+), 5 deletions(-)\n\n\nbase-commit: cd3e606211bb1cf8bc57f7d76bab98cc17a150bc\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1100%2Fphillipwood%2Fwip%2Fadd-p-fix-hunk-splitting-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1100/phillipwood/wip/add-p-fix-hunk-splitting-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1100\n-- \ngitgitgadget\n"},{"id":"444470","messageId":"cc8639fc29db18da00ba2a95a1305efc3ea24205.1640010777.git.gitgitgadget@gmail.com","threadId":"57123","inReplyTo":"pull.1100.git.1640010777.gitgitgadget@gmail.com","subject":"[PATCH 1/2] t3701: clean up hunk splitting tests","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-20T14:32:56Z","receivedAt":"2021-12-20T14:33:03Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nClean up some test constructs in preparation for extending the tests\nin the next commit. There are three small changes, I've grouped them\ntogether as they're so small it didn't seem worth creating three\nseparate commits.\n 1 - \"cat file | sed expression\" is better written as\n     \"sed expression file\".\n 2 - Follow our usual practice of redirecting the output of git\n     commands to a file rather than piping it into another command.\n 3 - Use test_write_lines rather than 'printf \"%s\\n\"'.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n t/t3701-add-interactive.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 207714655f2..77de0029ba5 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -347,7 +347,7 @@ test_expect_success 'setup patch' '\n # Expected output, diff is similar to the patch but w/ diff at the top\n test_expect_success 'setup expected' '\n \techo diff --git a/file b/file >expected &&\n-\tcat patch |sed \"/^index/s/ 100644/ 100755/\" >>expected &&\n+\tsed \"/^index/s/ 100644/ 100755/\" patch >>expected &&\n \tcat >expected-output <<-\\EOF\n \t--- a/file\n \t+++ b/file\n@@ -373,9 +373,9 @@ test_expect_success 'setup expected' '\n test_expect_success 'add first line works' '\n \tgit commit -am \"clear local changes\" &&\n \tgit apply patch &&\n-\tprintf \"%s\\n\" s y y | git add -p file 2>error |\n-\t\tsed -n -e \"s/^([1-2]\\/[1-2]) Stage this hunk[^@]*\\(@@ .*\\)/\\1/\" \\\n-\t\t       -e \"/^[-+@ \\\\\\\\]\"/p  >output &&\n+\ttest_write_lines s y y | git add -p file 2>error >raw-output &&\n+\tsed -n -e \"s/^([1-2]\\/[1-2]) Stage this hunk[^@]*\\(@@ .*\\)/\\1/\" \\\n+\t       -e \"/^[-+@ \\\\\\\\]\"/p raw-output >output &&\n \ttest_must_be_empty error &&\n \tgit diff --cached >diff &&\n \tdiff_cmp expected diff &&\n-- \ngitgitgadget\n\n"},{"id":"444471","messageId":"5d5639c2b0474680850b7adbb7c5ec81d124eb50.1640010777.git.gitgitgadget@gmail.com","threadId":"57123","inReplyTo":"pull.1100.git.1640010777.gitgitgadget@gmail.com","subject":"[PATCH 2/2] builtin add -p: fix hunk splitting","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-20T14:32:57Z","receivedAt":"2021-12-20T14:33:04Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nTo determine whether a hunk can be split a counter is incremented each\ntime a context line follows an insertion or deletion. If at the end of\nthe hunk the value of this counter is greater than one then the hunk\ncan be split into that number of smaller hunks. If the last hunk in a\nfile ends with an insertion or deletion then there is no following\ncontext line and the counter will not be incremented. This case is\nalready handled at the end of the loop where counter is incremented if\nthe last hunk ended with an insertion or deletion. Unfortunately there\nis no similar check between files (likely because the perl version\nonly ever parses one diff at a time). Fix this by checking if the last\nhunk ended with an insertion or deletion when we see the diff header\nof a new file and extend the existing regression test.\n\nReproted-by: SZEDER Gábor <szeder.dev@gmail.com>\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n add-patch.c                |  7 ++++++\n t/t3701-add-interactive.sh | 46 ++++++++++++++++++++++++++++++++++----\n 2 files changed, 49 insertions(+), 4 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 8c41cdfe39b..5cea70666e9 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -472,6 +472,13 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n \t\t\teol = pend;\n \n \t\tif (starts_with(p, \"diff \")) {\n+\t\t\tif (marker == '-' || marker == '+')\n+\t\t\t\t/*\n+\t\t\t\t * Last hunk ended in non-context line (i.e. it\n+\t\t\t\t * appended lines to the file, so there are no\n+\t\t\t\t * trailing context lines).\n+\t\t\t\t */\n+\t\t\t\thunk->splittable_into++;\n \t\t\tALLOC_GROW_BY(s->file_diff, s->file_diff_nr, 1,\n \t\t\t\t   file_diff_alloc);\n \t\t\tfile_diff = s->file_diff + s->file_diff_nr - 1;\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 77de0029ba5..94537a6b40a 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -326,7 +326,9 @@ test_expect_success 'correct message when there is nothing to do' '\n test_expect_success 'setup again' '\n \tgit reset --hard &&\n \ttest_chmod +x file &&\n-\techo content >>file\n+\techo content >>file &&\n+\ttest_write_lines A B C D>file2 &&\n+\tgit add file2\n '\n \n # Write the patch file with a new line at the top and bottom\n@@ -341,13 +343,27 @@ test_expect_success 'setup patch' '\n \t content\n \t+lastline\n \t\\ No newline at end of file\n+\tdiff --git a/file2 b/file2\n+\tindex 8422d40..35b930a 100644\n+\t--- a/file2\n+\t+++ b/file2\n+\t@@ -1,4 +1,5 @@\n+\t-A\n+\t+Z\n+\t B\n+\t+Y\n+\t C\n+\t-D\n+\t+X\n \tEOF\n '\n \n # Expected output, diff is similar to the patch but w/ diff at the top\n test_expect_success 'setup expected' '\n \techo diff --git a/file b/file >expected &&\n-\tsed \"/^index/s/ 100644/ 100755/\" patch >>expected &&\n+\tsed -e \"/^index 180b47c/s/ 100644/ 100755/\" \\\n+\t    -e /1,5/s//1,4/ \\\n+\t    -e /Y/d patch >>expected &&\n \tcat >expected-output <<-\\EOF\n \t--- a/file\n \t+++ b/file\n@@ -366,6 +382,28 @@ test_expect_success 'setup expected' '\n \t content\n \t+lastline\n \t\\ No newline at end of file\n+\t--- a/file2\n+\t+++ b/file2\n+\t@@ -1,4 +1,5 @@\n+\t-A\n+\t+Z\n+\t B\n+\t+Y\n+\t C\n+\t-D\n+\t+X\n+\t@@ -1,2 +1,2 @@\n+\t-A\n+\t+Z\n+\t B\n+\t@@ -2,2 +2,3 @@\n+\t B\n+\t+Y\n+\t C\n+\t@@ -3,2 +4,2 @@\n+\t C\n+\t-D\n+\t+X\n \tEOF\n '\n \n@@ -373,8 +411,8 @@ test_expect_success 'setup expected' '\n test_expect_success 'add first line works' '\n \tgit commit -am \"clear local changes\" &&\n \tgit apply patch &&\n-\ttest_write_lines s y y | git add -p file 2>error >raw-output &&\n-\tsed -n -e \"s/^([1-2]\\/[1-2]) Stage this hunk[^@]*\\(@@ .*\\)/\\1/\" \\\n+\ttest_write_lines s y y s y n y | git add -p 2>error >raw-output &&\n+\tsed -n -e \"s/^([1-9]\\/[1-9]) Stage this hunk[^@]*\\(@@ .*\\)/\\1/\" \\\n \t       -e \"/^[-+@ \\\\\\\\]\"/p raw-output >output &&\n \ttest_must_be_empty error &&\n \tgit diff --cached >diff &&\n-- \ngitgitgadget\n"},{"id":"444523","messageId":"211220.86mtkvt7cy.gmgdl@evledraar.gmail.com","threadId":"57123","inReplyTo":"5d5639c2b0474680850b7adbb7c5ec81d124eb50.1640010777.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] builtin add -p: fix hunk splitting","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-20T19:06:00Z","receivedAt":"2021-12-20T19:16:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Dec 20 2021, Phillip Wood via GitGitGadget wrote:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> To determine whether a hunk can be split a counter is incremented each\n> time a context line follows an insertion or deletion. If at the end of\n> the hunk the value of this counter is greater than one then the hunk\n> can be split into that number of smaller hunks. If the last hunk in a\n> file ends with an insertion or deletion then there is no following\n> context line and the counter will not be incremented. This case is\n> already handled at the end of the loop where counter is incremented if\n> the last hunk ended with an insertion or deletion. Unfortunately there\n> is no similar check between files (likely because the perl version\n> only ever parses one diff at a time). Fix this by checking if the last\n> hunk ended with an insertion or deletion when we see the diff header\n> of a new file and extend the existing regression test.\n>\n> Reproted-by: SZEDER Gábor <szeder.dev@gmail.com>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>  add-patch.c                |  7 ++++++\n>  t/t3701-add-interactive.sh | 46 ++++++++++++++++++++++++++++++++++----\n>  2 files changed, 49 insertions(+), 4 deletions(-)\n>\n> diff --git a/add-patch.c b/add-patch.c\n> index 8c41cdfe39b..5cea70666e9 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -472,6 +472,13 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n>  \t\t\teol = pend;\n>  \n>  \t\tif (starts_with(p, \"diff \")) {\n> +\t\t\tif (marker == '-' || marker == '+')\n> +\t\t\t\t/*\n> +\t\t\t\t * Last hunk ended in non-context line (i.e. it\n> +\t\t\t\t * appended lines to the file, so there are no\n> +\t\t\t\t * trailing context lines).\n> +\t\t\t\t */\n> +\t\t\t\thunk->splittable_into++;\n\nI wondered if factoring out these several \"marker == '-' || marker ==\n'+'\" cases in parse_diff() into a \"is_plus_minus(marker)\" was worth it,\nbut probably not.\n\n>  \t\t\tALLOC_GROW_BY(s->file_diff, s->file_diff_nr, 1,\n>  \t\t\t\t   file_diff_alloc);\n>  \t\t\tfile_diff = s->file_diff + s->file_diff_nr - 1;\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index 77de0029ba5..94537a6b40a 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -326,7 +326,9 @@ test_expect_success 'correct message when there is nothing to do' '\n>  test_expect_success 'setup again' '\n>  \tgit reset --hard &&\n>  \ttest_chmod +x file &&\n> -\techo content >>file\n> +\techo content >>file &&\n> +\ttest_write_lines A B C D>file2 &&\n\nstyle nit: \"cmd args >file2\" not \"cmd args>file2\"\n\n> @@ -373,8 +411,8 @@ test_expect_success 'setup expected' '\n>  test_expect_success 'add first line works' '\n>  \tgit commit -am \"clear local changes\" &&\n>  \tgit apply patch &&\n> -\ttest_write_lines s y y | git add -p file 2>error >raw-output &&\n> -\tsed -n -e \"s/^([1-2]\\/[1-2]) Stage this hunk[^@]*\\(@@ .*\\)/\\1/\" \\\n> +\ttest_write_lines s y y s y n y | git add -p 2>error >raw-output &&\n> +\tsed -n -e \"s/^([1-9]\\/[1-9]) Stage this hunk[^@]*\\(@@ .*\\)/\\1/\" \\\n>  \t       -e \"/^[-+@ \\\\\\\\]\"/p raw-output >output &&\n>  \ttest_must_be_empty error &&\n>  \tgit diff --cached >diff &&\n\nstyle/diff nit: maybe worth it to in 1/2 do some version of:\n\n    test_write_lines ... >lines &&\n    git ... <lines .. &&\n    ...\n    sed -n \\\n    \t-e ... \\\n        -e ... \\\n        >output\n\nJust to make the diff smaller, i.e. just the \"test_write_lines\" line\nwould be modified here.\n\nThe changes themselves & this series LGTM.\n"},{"id":"444542","messageId":"xmqqczlrugny.fsf@gitster.g","threadId":"57123","inReplyTo":"cc8639fc29db18da00ba2a95a1305efc3ea24205.1640010777.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] t3701: clean up hunk splitting tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-20T21:09:53Z","receivedAt":"2021-12-20T21:09:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> Clean up some test constructs in preparation for extending the tests\n> in the next commit. There are three small changes, I've grouped them\n> together as they're so small it didn't seem worth creating three\n> separate commits.\n>  1 - \"cat file | sed expression\" is better written as\n>      \"sed expression file\".\n>  2 - Follow our usual practice of redirecting the output of git\n>      commands to a file rather than piping it into another command.\n>  3 - Use test_write_lines rather than 'printf \"%s\\n\"'.\n\nAll good points.  Somehow people seem to forget \"do not cat a single\nfile into a pipe\".\n\n> @@ -373,9 +373,9 @@ test_expect_success 'setup expected' '\n>  test_expect_success 'add first line works' '\n>  \tgit commit -am \"clear local changes\" &&\n>  \tgit apply patch &&\n> -\tprintf \"%s\\n\" s y y | git add -p file 2>error |\n> -\t\tsed -n -e \"s/^([1-2]\\/[1-2]) Stage this hunk[^@]*\\(@@ .*\\)/\\1/\" \\\n> -\t\t       -e \"/^[-+@ \\\\\\\\]\"/p  >output &&\n> +\ttest_write_lines s y y | git add -p file 2>error >raw-output &&\n> +\tsed -n -e \"s/^([1-2]\\/[1-2]) Stage this hunk[^@]*\\(@@ .*\\)/\\1/\" \\\n> +\t       -e \"/^[-+@ \\\\\\\\]\"/p raw-output >output &&\n\nLooks good.  Thanks.\n"},{"id":"444545","messageId":"xmqq4k73ufq7.fsf@gitster.g","threadId":"57123","inReplyTo":"5d5639c2b0474680850b7adbb7c5ec81d124eb50.1640010777.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] builtin add -p: fix hunk splitting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-20T21:30:08Z","receivedAt":"2021-12-20T21:30:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> To determine whether a hunk can be split a counter is incremented each\n> time a context line follows an insertion or deletion. If at the end of\n> the hunk the value of this counter is greater than one then the hunk\n> can be split into that number of smaller hunks. If the last hunk in a\n> file ends with an insertion or deletion then there is no following\n> context line and the counter will not be incremented. This case is\n> already handled at the end of the loop where counter is incremented if\n> the last hunk ended with an insertion or deletion. Unfortunately there\n> is no similar check between files (likely because the perl version\n> only ever parses one diff at a time).\n\nIn other words, the original laid out the code in such a way that\nsuch a bug will be impossible, and the rewrite broke it because it\nrolled both \"next file\" and \"next hunk\" into the same loop?\n\n> Fix this by checking if the last\n> hunk ended with an insertion or deletion when we see the diff header\n> of a new file and extend the existing regression test.\n\nYou should be able to explain what end-user visible bug is in a\nsimple single sentence before all of the above.\n\n\"The C reimplementation of 'add -p' fails to split a hunk when the\nhunk ends with addition or deletion without post context line.\" or\nsomething like that.\n\n>  \t\tif (starts_with(p, \"diff \")) {\n> +\t\t\tif (marker == '-' || marker == '+')\n> +\t\t\t\t/*\n> +\t\t\t\t * Last hunk ended in non-context line (i.e. it\n> +\t\t\t\t * appended lines to the file, so there are no\n> +\t\t\t\t * trailing context lines).\n> +\t\t\t\t */\n> +\t\t\t\thunk->splittable_into++;\n\nThis looks correct but unsatisfactory.  We have the same processing\nimmediately after loop---what is common between them is that this is\na process to \"conclude\" the hunks for the file we have been reading\nthe patch for.\n\nCan we at least make a helper function that identifies what it does\nclearly by its name, and use it here and after the loop, to clarify\nwhat is going on?  Then you do not need the 5-line comment there.\n\n\t\tif (starts_with(p, \"diff \")) {\n+\t\t\tconclude_file(hunk, marker);\n\nor something like that, perhaps.\n\nThanks.\n"},{"id":"445912","messageId":"pull.1100.v2.git.1641899530.gitgitgadget@gmail.com","threadId":"57123","inReplyTo":"pull.1100.git.1640010777.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] builtin add -p: fix hunk splitting","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-11T11:12:08Z","receivedAt":"2022-01-11T11:12:15Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Thanks to Junio and Ævar for their comments on V1. I've updated the commit\nmessage and added a helper function as suggested.\n\nV1 Cover Letter: Fix a small regression in the hunk splitting of the builtin\nversion compared to the perl version. Thanks to Szeder for the easy to\nfollow bug report.\n\nPhillip Wood (2):\n  t3701: clean up hunk splitting tests\n  builtin add -p: fix hunk splitting\n\n add-patch.c                | 20 ++++++++++------\n t/t3701-add-interactive.sh | 48 ++++++++++++++++++++++++++++++++++----\n 2 files changed, 56 insertions(+), 12 deletions(-)\n\n\nbase-commit: cd3e606211bb1cf8bc57f7d76bab98cc17a150bc\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1100%2Fphillipwood%2Fwip%2Fadd-p-fix-hunk-splitting-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1100/phillipwood/wip/add-p-fix-hunk-splitting-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1100\n\nRange-diff vs v1:\n\n 1:  cc8639fc29d = 1:  cc8639fc29d t3701: clean up hunk splitting tests\n 2:  5d5639c2b04 ! 2:  b698989e265 builtin add -p: fix hunk splitting\n     @@ Metadata\n       ## Commit message ##\n          builtin add -p: fix hunk splitting\n      \n     +    The C reimplementation of \"add -p\" fails to split the last hunk in a\n     +    file if hunk ends with an addition or deletion without any post context\n     +    line unless it is the last file to be processed.\n     +\n          To determine whether a hunk can be split a counter is incremented each\n          time a context line follows an insertion or deletion. If at the end of\n          the hunk the value of this counter is greater than one then the hunk\n     @@ Commit message\n          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n      \n       ## add-patch.c ##\n     +@@ add-patch.c: static int is_octal(const char *p, size_t len)\n     + \treturn 1;\n     + }\n     + \n     ++static void complete_file(char marker, struct hunk *hunk)\n     ++{\n     ++\tif (marker == '-' || marker == '+')\n     ++\t\t/*\n     ++\t\t * Last hunk ended in non-context line (i.e. it\n     ++\t\t * appended lines to the file, so there are no\n     ++\t\t * trailing context lines).\n     ++\t\t */\n     ++\t\thunk->splittable_into++;\n     ++}\n     ++\n     + static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n     + {\n     + \tstruct strvec args = STRVEC_INIT;\n      @@ add-patch.c: static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n       \t\t\teol = pend;\n       \n       \t\tif (starts_with(p, \"diff \")) {\n     -+\t\t\tif (marker == '-' || marker == '+')\n     -+\t\t\t\t/*\n     -+\t\t\t\t * Last hunk ended in non-context line (i.e. it\n     -+\t\t\t\t * appended lines to the file, so there are no\n     -+\t\t\t\t * trailing context lines).\n     -+\t\t\t\t */\n     -+\t\t\t\thunk->splittable_into++;\n     ++\t\t\tcomplete_file(marker, hunk);\n       \t\t\tALLOC_GROW_BY(s->file_diff, s->file_diff_nr, 1,\n       \t\t\t\t   file_diff_alloc);\n       \t\t\tfile_diff = s->file_diff + s->file_diff_nr - 1;\n     +@@ add-patch.c: static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n     + \t\t\t\tfile_diff->hunk->colored_end = hunk->colored_end;\n     + \t\t}\n     + \t}\n     +-\n     +-\tif (marker == '-' || marker == '+')\n     +-\t\t/*\n     +-\t\t * Last hunk ended in non-context line (i.e. it appended lines\n     +-\t\t * to the file, so there are no trailing context lines).\n     +-\t\t */\n     +-\t\thunk->splittable_into++;\n     ++\tcomplete_file(marker, hunk);\n     + \n     + \t/* non-colored shorter than colored? */\n     + \tif (colored_p != colored_pend) {\n      \n       ## t/t3701-add-interactive.sh ##\n      @@ t/t3701-add-interactive.sh: test_expect_success 'correct message when there is nothing to do' '\n\n-- \ngitgitgadget\n"},{"id":"445913","messageId":"cc8639fc29db18da00ba2a95a1305efc3ea24205.1641899530.git.gitgitgadget@gmail.com","threadId":"57123","inReplyTo":"pull.1100.v2.git.1641899530.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] t3701: clean up hunk splitting tests","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-11T11:12:09Z","receivedAt":"2022-01-11T11:12:16Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nClean up some test constructs in preparation for extending the tests\nin the next commit. There are three small changes, I've grouped them\ntogether as they're so small it didn't seem worth creating three\nseparate commits.\n 1 - \"cat file | sed expression\" is better written as\n     \"sed expression file\".\n 2 - Follow our usual practice of redirecting the output of git\n     commands to a file rather than piping it into another command.\n 3 - Use test_write_lines rather than 'printf \"%s\\n\"'.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n t/t3701-add-interactive.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 207714655f2..77de0029ba5 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -347,7 +347,7 @@ test_expect_success 'setup patch' '\n # Expected output, diff is similar to the patch but w/ diff at the top\n test_expect_success 'setup expected' '\n \techo diff --git a/file b/file >expected &&\n-\tcat patch |sed \"/^index/s/ 100644/ 100755/\" >>expected &&\n+\tsed \"/^index/s/ 100644/ 100755/\" patch >>expected &&\n \tcat >expected-output <<-\\EOF\n \t--- a/file\n \t+++ b/file\n@@ -373,9 +373,9 @@ test_expect_success 'setup expected' '\n test_expect_success 'add first line works' '\n \tgit commit -am \"clear local changes\" &&\n \tgit apply patch &&\n-\tprintf \"%s\\n\" s y y | git add -p file 2>error |\n-\t\tsed -n -e \"s/^([1-2]\\/[1-2]) Stage this hunk[^@]*\\(@@ .*\\)/\\1/\" \\\n-\t\t       -e \"/^[-+@ \\\\\\\\]\"/p  >output &&\n+\ttest_write_lines s y y | git add -p file 2>error >raw-output &&\n+\tsed -n -e \"s/^([1-2]\\/[1-2]) Stage this hunk[^@]*\\(@@ .*\\)/\\1/\" \\\n+\t       -e \"/^[-+@ \\\\\\\\]\"/p raw-output >output &&\n \ttest_must_be_empty error &&\n \tgit diff --cached >diff &&\n \tdiff_cmp expected diff &&\n-- \ngitgitgadget\n\n"},{"id":"445914","messageId":"b698989e265cba851aa7ba55ea38d7e7b86548ac.1641899530.git.gitgitgadget@gmail.com","threadId":"57123","inReplyTo":"pull.1100.v2.git.1641899530.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] builtin add -p: fix hunk splitting","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-11T11:12:10Z","receivedAt":"2022-01-11T11:12:21Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nThe C reimplementation of \"add -p\" fails to split the last hunk in a\nfile if hunk ends with an addition or deletion without any post context\nline unless it is the last file to be processed.\n\nTo determine whether a hunk can be split a counter is incremented each\ntime a context line follows an insertion or deletion. If at the end of\nthe hunk the value of this counter is greater than one then the hunk\ncan be split into that number of smaller hunks. If the last hunk in a\nfile ends with an insertion or deletion then there is no following\ncontext line and the counter will not be incremented. This case is\nalready handled at the end of the loop where counter is incremented if\nthe last hunk ended with an insertion or deletion. Unfortunately there\nis no similar check between files (likely because the perl version\nonly ever parses one diff at a time). Fix this by checking if the last\nhunk ended with an insertion or deletion when we see the diff header\nof a new file and extend the existing regression test.\n\nReproted-by: SZEDER Gábor <szeder.dev@gmail.com>\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n add-patch.c                | 20 +++++++++++------\n t/t3701-add-interactive.sh | 46 ++++++++++++++++++++++++++++++++++----\n 2 files changed, 55 insertions(+), 11 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 8c41cdfe39b..89ffda32b26 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -383,6 +383,17 @@ static int is_octal(const char *p, size_t len)\n \treturn 1;\n }\n \n+static void complete_file(char marker, struct hunk *hunk)\n+{\n+\tif (marker == '-' || marker == '+')\n+\t\t/*\n+\t\t * Last hunk ended in non-context line (i.e. it\n+\t\t * appended lines to the file, so there are no\n+\t\t * trailing context lines).\n+\t\t */\n+\t\thunk->splittable_into++;\n+}\n+\n static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n {\n \tstruct strvec args = STRVEC_INIT;\n@@ -472,6 +483,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n \t\t\teol = pend;\n \n \t\tif (starts_with(p, \"diff \")) {\n+\t\t\tcomplete_file(marker, hunk);\n \t\t\tALLOC_GROW_BY(s->file_diff, s->file_diff_nr, 1,\n \t\t\t\t   file_diff_alloc);\n \t\t\tfile_diff = s->file_diff + s->file_diff_nr - 1;\n@@ -598,13 +610,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n \t\t\t\tfile_diff->hunk->colored_end = hunk->colored_end;\n \t\t}\n \t}\n-\n-\tif (marker == '-' || marker == '+')\n-\t\t/*\n-\t\t * Last hunk ended in non-context line (i.e. it appended lines\n-\t\t * to the file, so there are no trailing context lines).\n-\t\t */\n-\t\thunk->splittable_into++;\n+\tcomplete_file(marker, hunk);\n \n \t/* non-colored shorter than colored? */\n \tif (colored_p != colored_pend) {\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 77de0029ba5..94537a6b40a 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -326,7 +326,9 @@ test_expect_success 'correct message when there is nothing to do' '\n test_expect_success 'setup again' '\n \tgit reset --hard &&\n \ttest_chmod +x file &&\n-\techo content >>file\n+\techo content >>file &&\n+\ttest_write_lines A B C D>file2 &&\n+\tgit add file2\n '\n \n # Write the patch file with a new line at the top and bottom\n@@ -341,13 +343,27 @@ test_expect_success 'setup patch' '\n \t content\n \t+lastline\n \t\\ No newline at end of file\n+\tdiff --git a/file2 b/file2\n+\tindex 8422d40..35b930a 100644\n+\t--- a/file2\n+\t+++ b/file2\n+\t@@ -1,4 +1,5 @@\n+\t-A\n+\t+Z\n+\t B\n+\t+Y\n+\t C\n+\t-D\n+\t+X\n \tEOF\n '\n \n # Expected output, diff is similar to the patch but w/ diff at the top\n test_expect_success 'setup expected' '\n \techo diff --git a/file b/file >expected &&\n-\tsed \"/^index/s/ 100644/ 100755/\" patch >>expected &&\n+\tsed -e \"/^index 180b47c/s/ 100644/ 100755/\" \\\n+\t    -e /1,5/s//1,4/ \\\n+\t    -e /Y/d patch >>expected &&\n \tcat >expected-output <<-\\EOF\n \t--- a/file\n \t+++ b/file\n@@ -366,6 +382,28 @@ test_expect_success 'setup expected' '\n \t content\n \t+lastline\n \t\\ No newline at end of file\n+\t--- a/file2\n+\t+++ b/file2\n+\t@@ -1,4 +1,5 @@\n+\t-A\n+\t+Z\n+\t B\n+\t+Y\n+\t C\n+\t-D\n+\t+X\n+\t@@ -1,2 +1,2 @@\n+\t-A\n+\t+Z\n+\t B\n+\t@@ -2,2 +2,3 @@\n+\t B\n+\t+Y\n+\t C\n+\t@@ -3,2 +4,2 @@\n+\t C\n+\t-D\n+\t+X\n \tEOF\n '\n \n@@ -373,8 +411,8 @@ test_expect_success 'setup expected' '\n test_expect_success 'add first line works' '\n \tgit commit -am \"clear local changes\" &&\n \tgit apply patch &&\n-\ttest_write_lines s y y | git add -p file 2>error >raw-output &&\n-\tsed -n -e \"s/^([1-2]\\/[1-2]) Stage this hunk[^@]*\\(@@ .*\\)/\\1/\" \\\n+\ttest_write_lines s y y s y n y | git add -p 2>error >raw-output &&\n+\tsed -n -e \"s/^([1-9]\\/[1-9]) Stage this hunk[^@]*\\(@@ .*\\)/\\1/\" \\\n \t       -e \"/^[-+@ \\\\\\\\]\"/p raw-output >output &&\n \ttest_must_be_empty error &&\n \tgit diff --cached >diff &&\n-- \ngitgitgadget\n"},{"id":"445915","messageId":"23b6c237-5a95-7c3c-2bcd-567dbef5aafe@gmail.com","threadId":"57123","inReplyTo":"211220.86mtkvt7cy.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 2/2] builtin add -p: fix hunk splitting","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-01-11T11:13:33Z","receivedAt":"2022-01-11T11:13:40Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ævar\n\nOn 20/12/2021 19:06, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Mon, Dec 20 2021, Phillip Wood via GitGitGadget wrote:\n> \n>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>\n>> To determine whether a hunk can be split a counter is incremented each\n>> time a context line follows an insertion or deletion. If at the end of\n>> the hunk the value of this counter is greater than one then the hunk\n>> can be split into that number of smaller hunks. If the last hunk in a\n>> file ends with an insertion or deletion then there is no following\n>> context line and the counter will not be incremented. This case is\n>> already handled at the end of the loop where counter is incremented if\n>> the last hunk ended with an insertion or deletion. Unfortunately there\n>> is no similar check between files (likely because the perl version\n>> only ever parses one diff at a time). Fix this by checking if the last\n>> hunk ended with an insertion or deletion when we see the diff header\n>> of a new file and extend the existing regression test.\n>>\n>> Reproted-by: SZEDER Gábor <szeder.dev@gmail.com>\n>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n>> ---\n>>   add-patch.c                |  7 ++++++\n>>   t/t3701-add-interactive.sh | 46 ++++++++++++++++++++++++++++++++++----\n>>   2 files changed, 49 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/add-patch.c b/add-patch.c\n>> index 8c41cdfe39b..5cea70666e9 100644\n>> --- a/add-patch.c\n>> +++ b/add-patch.c\n>> @@ -472,6 +472,13 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n>>   \t\t\teol = pend;\n>>   \n>>   \t\tif (starts_with(p, \"diff \")) {\n>> +\t\t\tif (marker == '-' || marker == '+')\n>> +\t\t\t\t/*\n>> +\t\t\t\t * Last hunk ended in non-context line (i.e. it\n>> +\t\t\t\t * appended lines to the file, so there are no\n>> +\t\t\t\t * trailing context lines).\n>> +\t\t\t\t */\n>> +\t\t\t\thunk->splittable_into++;\n> \n> I wondered if factoring out these several \"marker == '-' || marker ==\n> '+'\" cases in parse_diff() into a \"is_plus_minus(marker)\" was worth it,\n> but probably not.\n\nYeah in the end I just factored out this hunk into a new function but I \ndidn't add a function for \"marker == '-' || marker ==\n > '+'\"\n\n>>   \t\t\tALLOC_GROW_BY(s->file_diff, s->file_diff_nr, 1,\n>>   \t\t\t\t   file_diff_alloc);\n>>   \t\t\tfile_diff = s->file_diff + s->file_diff_nr - 1;\n>> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n>> index 77de0029ba5..94537a6b40a 100755\n>> --- a/t/t3701-add-interactive.sh\n>> +++ b/t/t3701-add-interactive.sh\n>> @@ -326,7 +326,9 @@ test_expect_success 'correct message when there is nothing to do' '\n>>   test_expect_success 'setup again' '\n>>   \tgit reset --hard &&\n>>   \ttest_chmod +x file &&\n>> -\techo content >>file\n>> +\techo content >>file &&\n>> +\ttest_write_lines A B C D>file2 &&\n> \n> style nit: \"cmd args >file2\" not \"cmd args>file2\"\n> \n>> @@ -373,8 +411,8 @@ test_expect_success 'setup expected' '\n>>   test_expect_success 'add first line works' '\n>>   \tgit commit -am \"clear local changes\" &&\n>>   \tgit apply patch &&\n>> -\ttest_write_lines s y y | git add -p file 2>error >raw-output &&\n>> -\tsed -n -e \"s/^([1-2]\\/[1-2]) Stage this hunk[^@]*\\(@@ .*\\)/\\1/\" \\\n>> +\ttest_write_lines s y y s y n y | git add -p 2>error >raw-output &&\n>> +\tsed -n -e \"s/^([1-9]\\/[1-9]) Stage this hunk[^@]*\\(@@ .*\\)/\\1/\" \\\n>>   \t       -e \"/^[-+@ \\\\\\\\]\"/p raw-output >output &&\n>>   \ttest_must_be_empty error &&\n>>   \tgit diff --cached >diff &&\n> \n> style/diff nit: maybe worth it to in 1/2 do some version of:\n> \n>      test_write_lines ... >lines &&\n>      git ... <lines .. &&\n>      ...\n>      sed -n \\\n>      \t-e ... \\\n>          -e ... \\\n>          >output\n> \n> Just to make the diff smaller, i.e. just the \"test_write_lines\" line\n> would be modified here.\n\nIn the end I decided to leave this as is, while refactoring slightly \nsimplifies this patch it makes the previous one bigger and means that \nwould need to be reviewed again.\n\n\n> The changes themselves & this series LGTM.\n\nThanks\n\nBest Wishes\n\nPhillip\n\n\n"},{"id":"445916","messageId":"220111.865yqqzebc.gmgdl@evledraar.gmail.com","threadId":"57123","inReplyTo":"23b6c237-5a95-7c3c-2bcd-567dbef5aafe@gmail.com","subject":"Re: [PATCH 2/2] builtin add -p: fix hunk splitting","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-11T11:44:33Z","receivedAt":"2022-01-11T11:45:15Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jan 11 2022, Phillip Wood wrote:\n\n> Hi Ævar\n>\n> On 20/12/2021 19:06, Ævar Arnfjörð Bjarmason wrote:\n>> On Mon, Dec 20 2021, Phillip Wood via GitGitGadget wrote:\n>> \n>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>>\n>>> To determine whether a hunk can be split a counter is incremented each\n>>> time a context line follows an insertion or deletion. If at the end of\n>>> the hunk the value of this counter is greater than one then the hunk\n>>> can be split into that number of smaller hunks. If the last hunk in a\n>>> file ends with an insertion or deletion then there is no following\n>>> context line and the counter will not be incremented. This case is\n>>> already handled at the end of the loop where counter is incremented if\n>>> the last hunk ended with an insertion or deletion. Unfortunately there\n>>> is no similar check between files (likely because the perl version\n>>> only ever parses one diff at a time). Fix this by checking if the last\n>>> hunk ended with an insertion or deletion when we see the diff header\n>>> of a new file and extend the existing regression test.\n>>>\n>>> Reproted-by: SZEDER Gábor <szeder.dev@gmail.com>\n>>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>> ---\n>>>   add-patch.c                |  7 ++++++\n>>>   t/t3701-add-interactive.sh | 46 ++++++++++++++++++++++++++++++++++----\n>>>   2 files changed, 49 insertions(+), 4 deletions(-)\n>>>\n>>> diff --git a/add-patch.c b/add-patch.c\n>>> index 8c41cdfe39b..5cea70666e9 100644\n>>> --- a/add-patch.c\n>>> +++ b/add-patch.c\n>>> @@ -472,6 +472,13 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n>>>   \t\t\teol = pend;\n>>>     \t\tif (starts_with(p, \"diff \")) {\n>>> +\t\t\tif (marker == '-' || marker == '+')\n>>> +\t\t\t\t/*\n>>> +\t\t\t\t * Last hunk ended in non-context line (i.e. it\n>>> +\t\t\t\t * appended lines to the file, so there are no\n>>> +\t\t\t\t * trailing context lines).\n>>> +\t\t\t\t */\n>>> +\t\t\t\thunk->splittable_into++;\n>> I wondered if factoring out these several \"marker == '-' || marker\n>> ==\n>> '+'\" cases in parse_diff() into a \"is_plus_minus(marker)\" was worth it,\n>> but probably not.\n>\n> Yeah in the end I just factored out this hunk into a new function but\n> I didn't add a function for \"marker == '-' || marker ==\n>> '+'\"\n>\n>>>   \t\t\tALLOC_GROW_BY(s->file_diff, s->file_diff_nr, 1,\n>>>   \t\t\t\t   file_diff_alloc);\n>>>   \t\t\tfile_diff = s->file_diff + s->file_diff_nr - 1;\n>>> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n>>> index 77de0029ba5..94537a6b40a 100755\n>>> --- a/t/t3701-add-interactive.sh\n>>> +++ b/t/t3701-add-interactive.sh\n>>> @@ -326,7 +326,9 @@ test_expect_success 'correct message when there is nothing to do' '\n>>>   test_expect_success 'setup again' '\n>>>   \tgit reset --hard &&\n>>>   \ttest_chmod +x file &&\n>>> -\techo content >>file\n>>> +\techo content >>file &&\n>>> +\ttest_write_lines A B C D>file2 &&\n>> style nit: \"cmd args >file2\" not \"cmd args>file2\"\n>> \n>>> @@ -373,8 +411,8 @@ test_expect_success 'setup expected' '\n>>>   test_expect_success 'add first line works' '\n>>>   \tgit commit -am \"clear local changes\" &&\n>>>   \tgit apply patch &&\n>>> -\ttest_write_lines s y y | git add -p file 2>error >raw-output &&\n>>> -\tsed -n -e \"s/^([1-2]\\/[1-2]) Stage this hunk[^@]*\\(@@ .*\\)/\\1/\" \\\n>>> +\ttest_write_lines s y y s y n y | git add -p 2>error >raw-output &&\n>>> +\tsed -n -e \"s/^([1-9]\\/[1-9]) Stage this hunk[^@]*\\(@@ .*\\)/\\1/\" \\\n>>>   \t       -e \"/^[-+@ \\\\\\\\]\"/p raw-output >output &&\n>>>   \ttest_must_be_empty error &&\n>>>   \tgit diff --cached >diff &&\n>> style/diff nit: maybe worth it to in 1/2 do some version of:\n>>      test_write_lines ... >lines &&\n>>      git ... <lines .. &&\n>>      ...\n>>      sed -n \\\n>>      \t-e ... \\\n>>          -e ... \\\n>>          >output\n>> Just to make the diff smaller, i.e. just the \"test_write_lines\" line\n>> would be modified here.\n>\n> In the end I decided to leave this as is, while refactoring slightly\n> simplifies this patch it makes the previous one bigger and means that \n> would need to be reviewed again.\n\nAll sounds good to me. Just stuff I thought I'd point out in case you\nthought it made sense. Going with it as-is is fine too.\n\n>> The changes themselves & this series LGTM.\n>\n> Thanks\n>\n> Best Wishes\n>\n> Phillip\n\n"},{"id":"445917","messageId":"220111.861r1ezcxn.gmgdl@evledraar.gmail.com","threadId":"57123","inReplyTo":"pull.1100.v2.git.1641899530.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] builtin add -p: fix hunk splitting","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-11T12:07:52Z","receivedAt":"2022-01-11T12:15:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jan 11 2022, Phillip Wood via GitGitGadget wrote:\n\n> Thanks to Junio and Ævar for their comments on V1. I've updated the commit\n> message and added a helper function as suggested.\n\nThis v2 LGTM as far as the functionality of the end-state is concerned.\n\nAs a remaining nit the complete_file() helper you introduce in 2/2\nchanges 2/4 places that increment \"hunk->splittable+into\".\n\nI grabbed this PR and came up with this amendmend to it which adds a 2/3\nstep that converts 3/3 of them, followed by adding the 4th user in your\n2/2 (now patch 3/3):\nhttps://github.com/git/git/compare/master...avar:phillipwood-avar/wip/add-p-fix-hunk-splitting-v2.1\n\nIt changes nothing as far as the end-state is concerned, but I think it\nmakes this easier to read & follow. The actual behavior change becomes a\none-line addition to add-patch.c, instead of being mixed up with the\nrefactoring of adding the new helper.\n\nIf you'd like to pick that up & run with it as a v3 that's fine by me,\nand if not that's also fine :) Just a suggestion.\n\nA range-diff between your v2 here and that linked-to\nphillipwood-avar/wip/add-p-fix-hunk-splitting-v2.1:\n\n1:  cc8639fc29d = 1:  34392397f04 t3701: clean up hunk splitting tests\n-:  ----------- > 2:  c082176f8c5 add-file.c: use static helper to check marker == +|-\n2:  b698989e265 ! 3:  defca0baba4 builtin add -p: fix hunk splitting\n    @@ Commit message\n         Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n     \n      ## add-patch.c ##\n    -@@ add-patch.c: static int is_octal(const char *p, size_t len)\n    - \treturn 1;\n    - }\n    - \n    -+static void complete_file(char marker, struct hunk *hunk)\n    -+{\n    -+\tif (marker == '-' || marker == '+')\n    -+\t\t/*\n    -+\t\t * Last hunk ended in non-context line (i.e. it\n    -+\t\t * appended lines to the file, so there are no\n    -+\t\t * trailing context lines).\n    -+\t\t */\n    -+\t\thunk->splittable_into++;\n    -+}\n    -+\n    - static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n    - {\n    - \tstruct strvec args = STRVEC_INIT;\n     @@ add-patch.c: static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n      \t\t\teol = pend;\n      \n      \t\tif (starts_with(p, \"diff \")) {\n    -+\t\t\tcomplete_file(marker, hunk);\n    ++\t\t\tcomplete_file(marker, &hunk->splittable_into);\n      \t\t\tALLOC_GROW_BY(s->file_diff, s->file_diff_nr, 1,\n      \t\t\t\t   file_diff_alloc);\n      \t\t\tfile_diff = s->file_diff + s->file_diff_nr - 1;\n    -@@ add-patch.c: static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n    - \t\t\t\tfile_diff->hunk->colored_end = hunk->colored_end;\n    - \t\t}\n    - \t}\n    --\n    --\tif (marker == '-' || marker == '+')\n    --\t\t/*\n    --\t\t * Last hunk ended in non-context line (i.e. it appended lines\n    --\t\t * to the file, so there are no trailing context lines).\n    --\t\t */\n    --\t\thunk->splittable_into++;\n    -+\tcomplete_file(marker, hunk);\n    - \n    - \t/* non-colored shorter than colored? */\n    - \tif (colored_p != colored_pend) {\n     \n      ## t/t3701-add-interactive.sh ##\n     @@ t/t3701-add-interactive.sh: test_expect_success 'correct message when there is nothing to do' '\n"},{"id":"445951","messageId":"348d37a3-f01b-cda6-8ce4-1536e0bd3b06@gmail.com","threadId":"57123","inReplyTo":"220111.861r1ezcxn.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 0/2] builtin add -p: fix hunk splitting","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-01-11T18:57:06Z","receivedAt":"2022-01-11T18:57:12Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ævar\n\nOn 11/01/2022 12:07, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Tue, Jan 11 2022, Phillip Wood via GitGitGadget wrote:\n> \n>> Thanks to Junio and Ævar for their comments on V1. I've updated the commit\n>> message and added a helper function as suggested.\n> \n> This v2 LGTM as far as the functionality of the end-state is concerned.\n\nThanks for taking a look\n\n> As a remaining nit the complete_file() helper you introduce in 2/2\n> changes 2/4 places that increment \"hunk->splittable+into\".\n> \n> I grabbed this PR and came up with this amendmend to it which adds a 2/3\n> step that converts 3/3 of them, followed by adding the 4th user in your\n> 2/2 (now patch 3/3):\n> https://github.com/git/git/compare/master...avar:phillipwood-avar/wip/add-p-fix-hunk-splitting-v2.1\n> \n> It changes nothing as far as the end-state is concerned, but I think it\n> makes this easier to read & follow. The actual behavior change becomes a\n> one-line addition to add-patch.c, instead of being mixed up with the\n> refactoring of adding the new helper.\n> \n> If you'd like to pick that up & run with it as a v3 that's fine by me,\n> and if not that's also fine :) Just a suggestion.\n\nI'm not sure I want to go with your extra changes. I've left some\ncomments on them below\n\n> @@ -488,12 +499,12 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n>  \telse if (starts_with(p, \"@@ \") ||\n>  \t\t (hunk == &file_diff->head &&\n>  \t\t  (skip_prefix(p, \"deleted file\", &deleted)))) {\n> -\t\tif (marker == '-' || marker == '+')\n> -\t\t\t/*\n> -\t\t\t * Should not happen; previous hunk did not end\n> -\t\t\t * in a context line? Handle it anyway.\n> -\t\t\t */\n> +\t\t\thunk->splittable_into++;\n> +\t\t/*\n> +\t\t * Should not increment \"splittable_into\";\n> +\t\t * previous hunk did not end in a context\n> +\t\t * line? Handle it anyway.\n> +\t\t */\n> +\t\tcomplete_file(marker, &hunk->splittable_into);\n>  \n>  \t\tALLOC_GROW_BY(file_diff->hunk, file_diff->hunk_nr, 1,\n>  \t\t\t   file_diff->hunk_alloc);\n\nI deliberately left this alone as I think we should probably make this\nBUG() out instead of silently accepting an invalid diff.\n\n> @@ -566,8 +577,8 @@ 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\t\thunk->splittable_into++;\n> +\t\tif (*p == ' ')\n> +\t\t\tcomplete_file(marker, &hunk->splittable_into);\n>  \t\tif (marker && *p != '\\\\')\n>  \t\t\tmarker = *p;\n  \nHere you are calling complete_file() which has the following comment\n\n      /*\n       * Last hunk ended in non-context line (i.e. it\n       * appended lines to the file, so there are no\n       * trailing context lines).\n       */\n\nfor all context lines so the function name and comment would need\nupdating.\n\nBest Wishes\n\nPhillip\n\n> A range-diff between your v2 here and that linked-to\n> phillipwood-avar/wip/add-p-fix-hunk-splitting-v2.1:\n> \n> 1:  cc8639fc29d = 1:  34392397f04 t3701: clean up hunk splitting tests\n> -:  ----------- > 2:  c082176f8c5 add-file.c: use static helper to check marker == +|-\n> 2:  b698989e265 ! 3:  defca0baba4 builtin add -p: fix hunk splitting\n>      @@ Commit message\n>           Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n>       \n>        ## add-patch.c ##\n>      -@@ add-patch.c: static int is_octal(const char *p, size_t len)\n>      - \treturn 1;\n>      - }\n>      -\n>      -+static void complete_file(char marker, struct hunk *hunk)\n>      -+{\n>      -+\tif (marker == '-' || marker == '+')\n>      -+\t\t/*\n>      -+\t\t * Last hunk ended in non-context line (i.e. it\n>      -+\t\t * appended lines to the file, so there are no\n>      -+\t\t * trailing context lines).\n>      -+\t\t */\n>      -+\t\thunk->splittable_into++;\n>      -+}\n>      -+\n>      - static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n>      - {\n>      - \tstruct strvec args = STRVEC_INIT;\n>       @@ add-patch.c: static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n>        \t\t\teol = pend;\n>        \n>        \t\tif (starts_with(p, \"diff \")) {\n>      -+\t\t\tcomplete_file(marker, hunk);\n>      ++\t\t\tcomplete_file(marker, &hunk->splittable_into);\n>        \t\t\tALLOC_GROW_BY(s->file_diff, s->file_diff_nr, 1,\n>        \t\t\t\t   file_diff_alloc);\n>        \t\t\tfile_diff = s->file_diff + s->file_diff_nr - 1;\n>      -@@ add-patch.c: static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n>      - \t\t\t\tfile_diff->hunk->colored_end = hunk->colored_end;\n>      - \t\t}\n>      - \t}\n>      --\n>      --\tif (marker == '-' || marker == '+')\n>      --\t\t/*\n>      --\t\t * Last hunk ended in non-context line (i.e. it appended lines\n>      --\t\t * to the file, so there are no trailing context lines).\n>      --\t\t */\n>      --\t\thunk->splittable_into++;\n>      -+\tcomplete_file(marker, hunk);\n>      -\n>      - \t/* non-colored shorter than colored? */\n>      - \tif (colored_p != colored_pend) {\n>       \n>        ## t/t3701-add-interactive.sh ##\n>       @@ t/t3701-add-interactive.sh: test_expect_success 'correct message when there is nothing to do' '\n\n"},{"id":"446045","messageId":"xmqq4k68x0ov.fsf@gitster.g","threadId":"57123","inReplyTo":"pull.1100.v2.git.1641899530.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] builtin add -p: fix hunk splitting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-12T18:34:40Z","receivedAt":"2022-01-12T18:35:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> Thanks to Junio and Ævar for their comments on V1. I've updated the commit\n> message and added a helper function as suggested.\n>\n> V1 Cover Letter: Fix a small regression in the hunk splitting of the builtin\n> version compared to the perl version. Thanks to Szeder for the easy to\n> follow bug report.\n\nLooking good.  After comparing the output from \n\n    $ git grep -e 'finalize[_a-z]*(' -e 'complete[_a-z]*(' \\*.c\n\nI would have called the helper \"finalize_file()\", but in the context\nof this file, the name complete_file() is not misleading enough to\nrequire renaming.\n\nWill queue.  Thanks.\n"},{"id":"446047","messageId":"xmqqv8yovlc2.fsf@gitster.g","threadId":"57123","inReplyTo":"348d37a3-f01b-cda6-8ce4-1536e0bd3b06@gmail.com","subject":"Re: [PATCH v2 0/2] builtin add -p: fix hunk splitting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-12T18:51:41Z","receivedAt":"2022-01-12T18:52:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> I'm not sure I want to go with your extra changes. I've left some\n> comments on them below\n>\n>> @@ -488,12 +499,12 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n>>  \telse if (starts_with(p, \"@@ \") ||\n>>  \t\t (hunk == &file_diff->head &&\n>>  \t\t  (skip_prefix(p, \"deleted file\", &deleted)))) {\n>> -\t\tif (marker == '-' || marker == '+')\n>> -\t\t\t/*\n>> -\t\t\t * Should not happen; previous hunk did not end\n>> -\t\t\t * in a context line? Handle it anyway.\n>> -\t\t\t */\n>> +\t\t\thunk->splittable_into++;\n>> +\t\t/*\n>> +\t\t * Should not increment \"splittable_into\";\n>> +\t\t * previous hunk did not end in a context\n>> +\t\t * line? Handle it anyway.\n>> +\t\t */\n>> +\t\tcomplete_file(marker, &hunk->splittable_into);\n>>   \t\tALLOC_GROW_BY(file_diff->hunk, file_diff->hunk_nr, 1,\n>>  \t\t\t   file_diff->hunk_alloc);\n>\n> I deliberately left this alone as I think we should probably make this\n> BUG() out instead of silently accepting an invalid diff.\n\nAs we are reading our own output, I agree that such a data error is\na BUG().\n\nIn any case, a helper to see if the file ended without post-context\nis one thing, and a helper that specify what happens after we are\ndone with a single file, before we move on top the next file or\nafter processing the last file, is another thing.  The latter may be\nable to make use of the former, but the latter may want to do more\nthan that in the future.\n\nAs complete_file() is about finalizing the processing we have done\nto the current file, it should be used for that purpose, and nothing\nelse, I think the hunk I see at\nhttps://github.com/git/git/commit/c082176f8c5a1fc1c8b2a93991ca28fd63aae73a\n(reproduced below) is simply a nonsense.\n\nStepping back a bit, though, is this helper really finalizing the\ncurrent file, or is it finalizing the current hunk?  If it were the\nlatter, then its use in the hunk I called \"nonsense\" above actually\nmakes perfect sense.  There may not be anything other than finalizing\nthe last hunk when we see the end of a file right now, so we may not\nneed to add a finalize_file() helper right now, and when we need to\ndo something more than finalizing the last hunk, we may need to capture\nthe distinction by adding one.\n\ndiff --git i/add-patch.c w/add-patch.c\nindex 89ffda32b2..6094290c86 100644\n--- i/add-patch.c\n+++ w/add-patch.c\n@@ -578,8 +578,8 @@ 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\t\thunk->splittable_into++;\n+\t\tif (*p == ' ')\n+\t\t\tcomplete_file(marker, &hunk->splittable_into);\n \t\tif (marker && *p != '\\\\')\n \t\t\tmarker = *p;\n \n\n"},{"id":"446520","messageId":"47e7b23a-56a3-b533-63ed-a854503b59ed@gmail.com","threadId":"57123","inReplyTo":"xmqqv8yovlc2.fsf@gitster.g","subject":"Re: [PATCH v2 0/2] builtin add -p: fix hunk splitting","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-01-19T20:01:54Z","receivedAt":"2022-01-19T20:02:01Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 12/01/2022 18:51, Junio C Hamano wrote:\n> Phillip Wood <phillip.wood123@gmail.com> writes:\n> \n>> I'm not sure I want to go with your extra changes. I've left some\n>> comments on them below\n>>\n>>> @@ -488,12 +499,12 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n>>>   \telse if (starts_with(p, \"@@ \") ||\n>>>   \t\t (hunk == &file_diff->head &&\n>>>   \t\t  (skip_prefix(p, \"deleted file\", &deleted)))) {\n>>> -\t\tif (marker == '-' || marker == '+')\n>>> -\t\t\t/*\n>>> -\t\t\t * Should not happen; previous hunk did not end\n>>> -\t\t\t * in a context line? Handle it anyway.\n>>> -\t\t\t */\n>>> +\t\t\thunk->splittable_into++;\n>>> +\t\t/*\n>>> +\t\t * Should not increment \"splittable_into\";\n>>> +\t\t * previous hunk did not end in a context\n>>> +\t\t * line? Handle it anyway.\n>>> +\t\t */\n>>> +\t\tcomplete_file(marker, &hunk->splittable_into);\n>>>    \t\tALLOC_GROW_BY(file_diff->hunk, file_diff->hunk_nr, 1,\n>>>   \t\t\t   file_diff->hunk_alloc);\n>>\n>> I deliberately left this alone as I think we should probably make this\n>> BUG() out instead of silently accepting an invalid diff.\n> \n> As we are reading our own output, I agree that such a data error is\n> a BUG().\n> \n> In any case, a helper to see if the file ended without post-context\n> is one thing, and a helper that specify what happens after we are\n> done with a single file, before we move on top the next file or\n> after processing the last file, is another thing.  The latter may be\n> able to make use of the former, but the latter may want to do more\n> than that in the future.\n> \n> As complete_file() is about finalizing the processing we have done\n> to the current file, it should be used for that purpose, and nothing\n> else, I think the hunk I see at\n> https://github.com/git/git/commit/c082176f8c5a1fc1c8b2a93991ca28fd63aae73a\n> (reproduced below) is simply a nonsense.\n> \n> Stepping back a bit, though, is this helper really finalizing the\n> current file, or is it finalizing the current hunk?  If it were the\n> latter, then its use in the hunk I called \"nonsense\" above actually\n> makes perfect sense.\n\nEven if the helper is finalizing the current hunk then I think that \n\"nonsense\" hunk would still wrong as it would be calling finalize_hunk() \non _every_ context line in the hunk rather than just being called once \nto finalize the hunk. We could call the function something like \nupdate_splittable() but then we'd need to explain why we were calling \nthat function at the start of a diff and at the end of the loop.\n\n> There may not be anything other than finalizing\n> the last hunk when we see the end of a file right now, so we may not\n> need to add a finalize_file() helper right now, and when we need to\n> do something more than finalizing the last hunk, we may need to capture\n> the distinction by adding one.\n\nYes, if you're happy lets leave this series as it is\n\nBest Wishes\n\nPhillip\n\n> diff --git i/add-patch.c w/add-patch.c\n> index 89ffda32b2..6094290c86 100644\n> --- i/add-patch.c\n> +++ w/add-patch.c\n> @@ -578,8 +578,8 @@ 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\t\thunk->splittable_into++;\n> +\t\tif (*p == ' ')\n> +\t\t\tcomplete_file(marker, &hunk->splittable_into);\n>   \t\tif (marker && *p != '\\\\')\n>   \t\t\tmarker = *p;\n>   \n> \n\n"},{"id":"446536","messageId":"xmqqr193j8y4.fsf@gitster.g","threadId":"57123","inReplyTo":"47e7b23a-56a3-b533-63ed-a854503b59ed@gmail.com","subject":"Re: [PATCH v2 0/2] builtin add -p: fix hunk splitting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-20T05:02:43Z","receivedAt":"2022-01-20T05:02:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> Even if the helper is finalizing the current hunk then I think that\n> \"nonsense\" hunk would still wrong as it would be calling\n> finalize_hunk() on _every_ context line in the hunk rather than just\n> being called once to finalize the hunk.\n\nTrue; this triggers every time we finish reading the common context\nlines and not at the end of hunk.  In any case, I think what we\nqueued looks good for 'next'.\n\n>>   -\t\tif ((marker == '-' || marker == '+') && *p == ' ')\n>> -\t\t\thunk->splittable_into++;\n>> +\t\tif (*p == ' ')\n>> +\t\t\tcomplete_file(marker, &hunk->splittable_into);\n"},{"id":"446542","messageId":"220120.86czkmn6gj.gmgdl@evledraar.gmail.com","threadId":"57123","inReplyTo":"xmqqr193j8y4.fsf@gitster.g","subject":"Re: [PATCH v2 0/2] builtin add -p: fix hunk splitting","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-20T08:42:27Z","receivedAt":"2022-01-20T08:43:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jan 19 2022, Junio C Hamano wrote:\n\n> Phillip Wood <phillip.wood123@gmail.com> writes:\n>\n>> Even if the helper is finalizing the current hunk then I think that\n>> \"nonsense\" hunk would still wrong as it would be calling\n>> finalize_hunk() on _every_ context line in the hunk rather than just\n>> being called once to finalize the hunk.\n>\n> True; this triggers every time we finish reading the common context\n> lines and not at the end of hunk.  In any case, I think what we\n> queued looks good for 'next'.\n\nFor what it's worth (and as the person who started this side-thread) I\nagree. This looks good as-is, thanks both!\n\n>>>   -\t\tif ((marker == '-' || marker == '+') && *p == ' ')\n>>> -\t\t\thunk->splittable_into++;\n>>> +\t\tif (*p == ' ')\n>>> +\t\t\tcomplete_file(marker, &hunk->splittable_into);\n\n"},{"id":"446594","messageId":"xmqqfspii5je.fsf@gitster.g","threadId":"57123","inReplyTo":"220120.86czkmn6gj.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 0/2] builtin add -p: fix hunk splitting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-20T19:13:57Z","receivedAt":"2022-01-20T19:15:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> On Wed, Jan 19 2022, Junio C Hamano wrote:\n>\n>> Phillip Wood <phillip.wood123@gmail.com> writes:\n>>\n>>> Even if the helper is finalizing the current hunk then I think that\n>>> \"nonsense\" hunk would still wrong as it would be calling\n>>> finalize_hunk() on _every_ context line in the hunk rather than just\n>>> being called once to finalize the hunk.\n>>\n>> True; this triggers every time we finish reading the common context\n>> lines and not at the end of hunk.  In any case, I think what we\n>> queued looks good for 'next'.\n>\n> For what it's worth (and as the person who started this side-thread) I\n> agree. This looks good as-is, thanks both!\n>\n>>>>   -\t\tif ((marker == '-' || marker == '+') && *p == ' ')\n>>>> -\t\t\thunk->splittable_into++;\n>>>> +\t\tif (*p == ' ')\n>>>> +\t\t\tcomplete_file(marker, &hunk->splittable_into);\n\nYup, thanks all.  The fix is now in 'next' and I expect we can\nsafely merge it down as part of the first batch next cycle.\n\n"},{"id":"446683","messageId":"nycvar.QRO.7.76.6.2201220938360.2121@tvgsbejvaqbjf.bet","threadId":"57123","inReplyTo":"47e7b23a-56a3-b533-63ed-a854503b59ed@gmail.com","subject":"Re: [PATCH v2 0/2] builtin add -p: fix hunk splitting","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-01-22T09:05:14Z","receivedAt":"2022-01-22T09:05:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nfirst of all: thank you for these patches. I read over them and they have\nmy ACK.\n\nOn Wed, 19 Jan 2022, Phillip Wood wrote:\n\n> On 12/01/2022 18:51, Junio C Hamano wrote:\n> > Phillip Wood <phillip.wood123@gmail.com> writes:\n> >\n> > > I'm not sure I want to go with your extra changes. I've left some\n> > > comments on them below\n> > >\n> > > > @@ -488,12 +499,12 @@ static int parse_diff(struct add_p_state *s, const\n> > > > struct pathspec *ps)\n> > > >    else if (starts_with(p, \"@@ \") ||\n> > > >      (hunk == &file_diff->head &&\n> > > >   \t\t  (skip_prefix(p, \"deleted file\", &deleted)))) {\n> > > > -\t\tif (marker == '-' || marker == '+')\n> > > > -\t\t\t/*\n> > > > -\t\t\t * Should not happen; previous hunk did not end\n> > > > -\t\t\t * in a context line? Handle it anyway.\n> > > > -\t\t\t */\n> > > > +\t\t\thunk->splittable_into++;\n> > > > +\t\t/*\n> > > > +\t\t * Should not increment \"splittable_into\";\n> > > > +\t\t * previous hunk did not end in a context\n> > > > +\t\t * line? Handle it anyway.\n> > > > +\t\t */\n> > > > +\t\tcomplete_file(marker, &hunk->splittable_into);\n> > > >      ALLOC_GROW_BY(file_diff->hunk, file_diff->hunk_nr, 1,\n> > > >         file_diff->hunk_alloc);\n> > >\n> > > I deliberately left this alone as I think we should probably make\n> > > this BUG() out instead of silently accepting an invalid diff.\n\nFWIW this was overzealous defensive programming on my part. More on that\nbelow.\n\n> > As we are reading our own output, I agree that such a data error is\n> > a BUG().\n\nIndeed. I was less worried about the output format changing, and more\nconcerned with bugs in my parser ;-)\n\nAlthough, having said that, I had meant to verify that `git add -p` cannot\nbe asked to produce and consume diffs with `-U0` when I wrote that\ncomment. Now I did that, and I am now confident that there is no way to ask\n`git add -p` to generate and use context line-free diffs: we neither add\n`-U<n>` in https://github.com/git/git/blob/v2.34.1/add-patch.c#L398-L417\nnor do we call the user-facing `git diff` command that would interpret\n`diff.context`, but instead we use `git diff-index` and `git diff-files`\n(which ignore that config setting).\n\n> > In any case, a helper to see if the file ended without post-context\n> > is one thing, and a helper that specify what happens after we are\n> > done with a single file, before we move on top the next file or\n> > after processing the last file, is another thing.  The latter may be\n> > able to make use of the former, but the latter may want to do more\n> > than that in the future.\n\nIf you are concerned about the name of the function: maybe a better name\nwould be `maybe_increment_splittable_hunk_count(marker)`.\n\n> >\n> > As complete_file() is about finalizing the processing we have done\n> > to the current file, it should be used for that purpose, and nothing\n> > else, I think the hunk I see at\n> > https://github.com/git/git/commit/c082176f8c5a1fc1c8b2a93991ca28fd63aae73a\n> > (reproduced below) is simply a nonsense.\n> >\n> > Stepping back a bit, though, is this helper really finalizing the\n> > current file, or is it finalizing the current hunk?  If it were the\n> > latter, then its use in the hunk I called \"nonsense\" above actually\n> > makes perfect sense.\n>\n> Even if the helper is finalizing the current hunk then I think that \"nonsense\"\n> hunk would still wrong as it would be calling finalize_hunk() on _every_\n> context line in the hunk rather than just being called once to finalize the\n> hunk. We could call the function something like update_splittable() but then\n> we'd need to explain why we were calling that function at the start of a diff\n> and at the end of the loop.\n\nRight. The point of this check is to see whether we missed counting a\nsplittable hunk. Then it makes more sense to call it at the beginning of a\nfile, at the end of a file _and_ at a context line.\n\nHaving said all that, I am really fine with what landed in `next`.\n\nThank you,\nDscho\n"},{"id":"446746","messageId":"649cfa82-5fbf-71f3-244b-bc37404c40ab@gmail.com","threadId":"57123","inReplyTo":"nycvar.QRO.7.76.6.2201220938360.2121@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 0/2] builtin add -p: fix hunk splitting","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-01-24T11:10:47Z","receivedAt":"2022-01-24T11:10:51Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 22/01/2022 09:05, Johannes Schindelin wrote:\n> Hi Phillip,\n> \n> first of all: thank you for these patches. I read over them and they have\n> my ACK.\n\nThanks\n\n> On Wed, 19 Jan 2022, Phillip Wood wrote:\n> \n>> On 12/01/2022 18:51, Junio C Hamano wrote:\n>>> Phillip Wood <phillip.wood123@gmail.com> writes:\n>>>\n>>>> I'm not sure I want to go with your extra changes. I've left some\n>>>> comments on them below\n>>>>\n>>>>> @@ -488,12 +499,12 @@ static int parse_diff(struct add_p_state *s, const\n>>>>> struct pathspec *ps)\n>>>>>     else if (starts_with(p, \"@@ \") ||\n>>>>>       (hunk == &file_diff->head &&\n>>>>>    \t\t  (skip_prefix(p, \"deleted file\", &deleted)))) {\n>>>>> -\t\tif (marker == '-' || marker == '+')\n>>>>> -\t\t\t/*\n>>>>> -\t\t\t * Should not happen; previous hunk did not end\n>>>>> -\t\t\t * in a context line? Handle it anyway.\n>>>>> -\t\t\t */\n>>>>> +\t\t\thunk->splittable_into++;\n>>>>> +\t\t/*\n>>>>> +\t\t * Should not increment \"splittable_into\";\n>>>>> +\t\t * previous hunk did not end in a context\n>>>>> +\t\t * line? Handle it anyway.\n>>>>> +\t\t */\n>>>>> +\t\tcomplete_file(marker, &hunk->splittable_into);\n>>>>>       ALLOC_GROW_BY(file_diff->hunk, file_diff->hunk_nr, 1,\n>>>>>          file_diff->hunk_alloc);\n>>>>\n>>>> I deliberately left this alone as I think we should probably make\n>>>> this BUG() out instead of silently accepting an invalid diff.\n> \n> FWIW this was overzealous defensive programming on my part. More on that\n> below.\n> \n>>> As we are reading our own output, I agree that such a data error is\n>>> a BUG().\n> \n> Indeed. I was less worried about the output format changing, and more\n> concerned with bugs in my parser ;-)\n> \n> Although, having said that, I had meant to verify that `git add -p` cannot\n> be asked to produce and consume diffs with `-U0` when I wrote that\n> comment. Now I did that, and I am now confident that there is no way to ask\n> `git add -p` to generate and use context line-free diffs: we neither add\n> `-U<n>` in https://github.com/git/git/blob/v2.34.1/add-patch.c#L398-L417\n> nor do we call the user-facing `git diff` command that would interpret\n> `diff.context`, but instead we use `git diff-index` and `git diff-files`\n> (which ignore that config setting).\n\nI did think about zero context diffs but realized that they can never be \nsplit so we don't need to worry about incrementing hunk->splittable_into \nin that case. It does mean that hunk->splittable_into will be zero in \nthe -U0 case rather than one but I dont think that matters as we only \ncare if it is >2 for splitting.\n\nBest Wishes\n\nPhillip\n\n>>> In any case, a helper to see if the file ended without post-context\n>>> is one thing, and a helper that specify what happens after we are\n>>> done with a single file, before we move on top the next file or\n>>> after processing the last file, is another thing.  The latter may be\n>>> able to make use of the former, but the latter may want to do more\n>>> than that in the future.\n> \n> If you are concerned about the name of the function: maybe a better name\n> would be `maybe_increment_splittable_hunk_count(marker)`.\n> \n>>>\n>>> As complete_file() is about finalizing the processing we have done\n>>> to the current file, it should be used for that purpose, and nothing\n>>> else, I think the hunk I see at\n>>> https://github.com/git/git/commit/c082176f8c5a1fc1c8b2a93991ca28fd63aae73a\n>>> (reproduced below) is simply a nonsense.\n>>>\n>>> Stepping back a bit, though, is this helper really finalizing the\n>>> current file, or is it finalizing the current hunk?  If it were the\n>>> latter, then its use in the hunk I called \"nonsense\" above actually\n>>> makes perfect sense.\n>>\n>> Even if the helper is finalizing the current hunk then I think that \"nonsense\"\n>> hunk would still wrong as it would be calling finalize_hunk() on _every_\n>> context line in the hunk rather than just being called once to finalize the\n>> hunk. We could call the function something like update_splittable() but then\n>> we'd need to explain why we were calling that function at the start of a diff\n>> and at the end of the loop.\n> \n> Right. The point of this check is to see whether we missed counting a\n> splittable hunk. Then it makes more sense to call it at the beginning of a\n> file, at the end of a file _and_ at a context line.\n> \n> Having said all that, I am really fine with what landed in `next`.\n> \n> Thank you,\n> Dscho\n"}]}