{"thread":{"id":"57060","subject":"builtin add interactive regression","startedAt":"2021-12-09T19:37:02Z","lastAt":"2021-12-20T15:04:22Z","messageCount":3,"participants":["SZEDER Gábor","Phillip Wood"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"443685","messageId":"20211209193625.GA3294@szeder.dev","threadId":"57060","inReplyTo":null,"subject":"builtin add interactive regression","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-12-09T19:36:25Z","receivedAt":"2021-12-09T19:37:02Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Hi,\n\nThis morning 'git add --patch' told me \"Sorry, cannot split this hunk\"\neven though that hunk in question was definitely splittable.  Then I\nspent my lunch break trying to reproduce the issue, and it turned out\nto be a regression in the builtin add interactive (I have\n'add.interactive.useBuiltin=true' in my config).  The following test\ndemonstrates this issue:\n\n  ---  >8  ---\n\n#!/bin/sh\n\ntest_description='test'\n. ./test-lib.sh\n\ntest_expect_success 'builtin interactive add cannot split hunk?!' '\n\tprintf \"%s\\n\" 1 2 >file &&\n\techo a >zzzz &&\n\tgit add file zzzz &&\n\tgit commit -m initial &&\n\tcat >file <<-\\EOF &&\n\t1\n\tadd a line\n\t2\n\tadd a line at the end (this is important!)\n\tEOF\n\techo \"modify one more file (this, too, is important!)\" >zzzz &&\n\n\tgit diff file &&\n\n\tgit -c add.interactive.useBuiltin=false add -u -p >expect &&\n\tgit -c add.interactive.useBuiltin=true add -u -p >actual &&\n\t# Uh-oh, \"s\" (for splitting the hunk) is missing!\n\ttest_cmp expect actual\n'\n\ntest_done\n\n  ---  8<  ---\n\nThis fails with:\n\n  + git -c add.interactive.useBuiltin=false add -u -p\n  + git -c add.interactive.useBuiltin=true add -u -p\n  + test_cmp expect actual\n  --- expect\t2021-12-09 19:21:23.354461170 +0000\n  +++ actual\t2021-12-09 19:21:23.358461215 +0000\n  @@ -7,7 +7,7 @@\n   +add a line\n    2\n   +add a line at the end (this is important!)\n  -(1/1) Stage this hunk [y,n,q,a,d,s,e,?]? \n  +(1/1) Stage this hunk [y,n,q,a,d,e,?]? \n   diff --git a/zzzz b/zzzz\n   index 7898192..84e1b35 100644\n   --- a/zzzz\n  error: last command exited with $?=1\n  not ok 1 - builtin interactive add cannot split hunk?!\n\nSo the builtin add interactive doesn't even offer the 's - split the\ncurrent hunk into smaller hunks' option, but my finger memory pressed\n's' anyway, and then it told me that \"Sorry...\" message.  The scripted\nversion can split the hunk just fine.\n\n"},{"id":"443803","messageId":"53fdf2c2-1e01-a105-6a20-8115f88b39f6@gmail.com","threadId":"57060","inReplyTo":"20211209193625.GA3294@szeder.dev","subject":"Re: builtin add interactive regression","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-12-10T14:13:45Z","receivedAt":"2021-12-10T14:13:50Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 09/12/2021 19:36, SZEDER Gábor wrote:\n> Hi,\n> \n> This morning 'git add --patch' told me \"Sorry, cannot split this hunk\"\n> even though that hunk in question was definitely splittable.  Then I\n> spent my lunch break trying to reproduce the issue, and it turned out\n> to be a regression in the builtin add interactive (I have\n> 'add.interactive.useBuiltin=true' in my config).  The following test\n> demonstrates this issue:\n> \n>    ---  >8  ---\n> \n> #!/bin/sh\n> \n> test_description='test'\n> . ./test-lib.sh\n> \n> test_expect_success 'builtin interactive add cannot split hunk?!' '\n> \tprintf \"%s\\n\" 1 2 >file &&\n> \techo a >zzzz &&\n> \tgit add file zzzz &&\n> \tgit commit -m initial &&\n> \tcat >file <<-\\EOF &&\n> \t1\n> \tadd a line\n> \t2\n> \tadd a line at the end (this is important!)\n> \tEOF\n> \techo \"modify one more file (this, too, is important!)\" >zzzz &&\n> \n> \tgit diff file &&\n> \n> \tgit -c add.interactive.useBuiltin=false add -u -p >expect &&\n> \tgit -c add.interactive.useBuiltin=true add -u -p >actual &&\n> \t# Uh-oh, \"s\" (for splitting the hunk) is missing!\n> \ttest_cmp expect actual\n> '\n> \n> test_done\n> \n>    ---  8<  ---\n> \n> This fails with:\n> \n>    + git -c add.interactive.useBuiltin=false add -u -p\n>    + git -c add.interactive.useBuiltin=true add -u -p\n>    + test_cmp expect actual\n>    --- expect\t2021-12-09 19:21:23.354461170 +0000\n>    +++ actual\t2021-12-09 19:21:23.358461215 +0000\n>    @@ -7,7 +7,7 @@\n>     +add a line\n>      2\n>     +add a line at the end (this is important!)\n>    -(1/1) Stage this hunk [y,n,q,a,d,s,e,?]?\n>    +(1/1) Stage this hunk [y,n,q,a,d,e,?]?\n>     diff --git a/zzzz b/zzzz\n>     index 7898192..84e1b35 100644\n>     --- a/zzzz\n>    error: last command exited with $?=1\n>    not ok 1 - builtin interactive add cannot split hunk?!\n> \n> So the builtin add interactive doesn't even offer the 's - split the\n> current hunk into smaller hunks' option, but my finger memory pressed\n> 's' anyway, and then it told me that \"Sorry...\" message.  The scripted\n> version can split the hunk just fine.\n\nThanks for the easy reproduction recipe, the hunk below makes it\npass. We're already checking marker at the very end of the loop\nbut we need to do it after the end of each diff (I think the perl\nversion only processes one diff at a time so does not have to\nworry about this). If anyone wants to take this forward please\ndo, if not I'll try and find some time later next week to turn it\ninto a proper patch with a commit message and test.\n\nBest Wishes\n\nPhillip\n\n---- >8 -----\ndiff --git a/add-patch.c b/add-patch.c\nindex 8c41cdfe39..4c2db78460 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -472,6 +472,14 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n                         eol = pend;\n  \n                 if (starts_with(p, \"diff \")) {\n+                       if (marker == '-' || marker == '+')\n+                               /*\n+                                * Last hunk ended in non-context line\n+                                * (i.e. it appended lines to the\n+                                * file, so there are no trailing\n+                                * context lines).\n+                                */\n+                               hunk->splittable_into++;\n                         ALLOC_GROW_BY(s->file_diff, s->file_diff_nr, 1,\n                                    file_diff_alloc);\n                         file_diff = s->file_diff + s->file_diff_nr - 1;\n\n"},{"id":"444476","messageId":"1de02f5c-2b47-50dd-f80c-708dd811e2ab@gmail.com","threadId":"57060","inReplyTo":"53fdf2c2-1e01-a105-6a20-8115f88b39f6@gmail.com","subject":"Re: builtin add interactive regression","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-12-20T14:51:51Z","receivedAt":"2021-12-20T15:04:22Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 10/12/2021 14:13, Phillip Wood wrote:\n> [...]\n> If anyone wants to take this forward please\n> do, if not I'll try and find some time later next week to turn it\n> into a proper patch with a commit message and test.\n\nI've submitted the fix at \nhttps://lore.kernel.org/git/pull.1100.git.1640010777.gitgitgadget@gmail.com/T/#t\n\nBest Wishes\n\nPhillip\n"}]}