{"thread":{"id":"48293","subject":"Regression in patch add?","startedAt":"2018-04-15T12:21:32Z","lastAt":"2018-07-11T20:50:26Z","messageCount":22,"participants":["mqudsi@neosmart.net","Martin Ågren","Phillip Wood","Oliver Joseph Ash","Junio C Hamano","Jacob Keller","Eric Sunshine","Jeff Felchner"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"344713","messageId":"01010162c940b8bb-d8139971-3ee2-4cd6-bb19-35126d46753b-000000@us-west-2.amazonses.com","threadId":"48293","inReplyTo":null,"subject":"Regression in patch add?","fromName":"","fromEmail":"mqudsi@neosmart.net","sentAt":"2018-04-15T12:21:24Z","receivedAt":"2018-04-15T12:21:32Z","isPatch":false,"sender":{"key":"mqudsi@neosmart.net","avatar":"https://gravatar.com/avatar/c2643dd7c6df61aed49d9f3d917ac6d61cafbbda5f9b1619f50d3b749dca415a?d=mp&s=160"},"body":"Hello all,\n\nI'm currently running the latest version of git built from `master`, and\nI'm running into what appears to be a regression in the behavior of the\npiecewise `git add -p` when applying a manually edited chunk.\n\nI first run `git add -p`, then manually edit a chunk (after hitting `s`\nonce, if it matters). The chunk originally contains the following:\n\n```diff\n# Manual hunk edit mode -- see bottom for a quick guide\n@@ -20,7 +20,7 @@\n \t\"call dein#add('Shougo/dein.vim', {'rev': 'master'})\n\n \t\" Add or remove your plugins here:\n-\t\" call dein#add('flazz/vim-colorschemes')\n-\tcall dein#add('Haron-Prime/evening_vim')\n+\tcall dein#add('flazz/vim-colorschemes')\n+\tcall dein#add('danilo-augusto/vim-afterglow')\n\n \t\"core plugins that change the behavior of vim and how we use it globally\n```\n\nUnder git 2.7.4, I can edit it to the following, which is accepted\nwithout a problem:\n\n```diff\n# Manual hunk edit mode -- see bottom for a quick guide\n@@ -20,7 +20,7 @@\n\t\"call dein#add('Shougo/dein.vim', {'rev': 'master'})\n\n\t\" Add or remove your plugins here:\n-\t\" call dein#add('flazz/vim-colorschemes')\n-\tcall dein#add('Haron-Prime/evening_vim')\n+\tcall dein#add('flazz/vim-colorschemes')\n+\tcall dein#add('Haron-Prime/evening_vim')\n\n\t\"core plugins that change the behavior of vim and how we use it globally\n```\n\nAll I did here was remove one `+` line and manually add another (which\nis a variant of the second `-` line).\n\nUnder git 2.17.0.252.gfe0a9ea, the same piece is opened in $VISUAL for\nediting (and if left unmodified applies OK), but when modified in the\nto the same exact value, after exiting the editor I receive the\nfollowing error from git:\n\n    error: patch fragment without header at line 15: @@ -25,7 +25,8 @@\n\nI'm not sure what to make of this.\n\nThank you,\n\nMahmoud Al-Qudsi\nNeoSmart Technologies\n\n\n"},{"id":"344716","messageId":"CAN0heSqCZWR1OD4k+-_OBdtCjCNW-UexLo4P-C5XyBqfU6KBEA@mail.gmail.com","threadId":"48293","inReplyTo":"01010162c940b8bb-d8139971-3ee2-4cd6-bb19-35126d46753b-000000@us-west-2.amazonses.com","subject":"Re: Regression in patch add?","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-04-15T13:59:17Z","receivedAt":"2018-04-15T13:59:23Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Hi Mahmoud\n\nOn 15 April 2018 at 14:21,  <mqudsi@neosmart.net> wrote:\n> I first run `git add -p`, then manually edit a chunk (after hitting `s`\n> once, if it matters). The chunk originally contains the following:\n\n[...]\n\n> Under git 2.7.4, I can edit it to the following, which is accepted\n> without a problem:\n>\n> ```diff\n> # Manual hunk edit mode -- see bottom for a quick guide\n> @@ -20,7 +20,7 @@\n>         \"call dein#add('Shougo/dein.vim', {'rev': 'master'})\n>\n>         \" Add or remove your plugins here:\n> -       \" call dein#add('flazz/vim-colorschemes')\n> -       call dein#add('Haron-Prime/evening_vim')\n> +       call dein#add('flazz/vim-colorschemes')\n> +       call dein#add('Haron-Prime/evening_vim')\n>\n>         \"core plugins that change the behavior of vim and how we use it globally\n> ```\n>\n> All I did here was remove one `+` line and manually add another (which\n> is a variant of the second `-` line).\n\nSo the line is identical (sans s/^-/+/). Interesting.\n\n> Under git 2.17.0.252.gfe0a9ea, the same piece is opened in $VISUAL for\n> editing (and if left unmodified applies OK), but when modified in the\n> to the same exact value, after exiting the editor I receive the\n> following error from git:\n>\n>     error: patch fragment without header at line 15: @@ -25,7 +25,8 @@\n\nI can't seem to reproduce this with some very simple testing. Are you\nable to share your files? Or even better, derive a minimal reproduction\nrecipe?\n\nWhat happens if you do not do a \"remove this line, then add it again\",\nbut instead turn that unchanged line into context? That is, you edit the\nhunk into something like this (but without white-space damage):\n\n...\n-       \" call dein#add('flazz/vim-colorschemes')\n+       call dein#add('flazz/vim-colorschemes')\n        call dein#add('Haron-Prime/evening_vim')\n...\n\nAdding Phillip to cc, since he was recently working in this area and\nmight have an idea.\n\nMartin\n"},{"id":"344782","messageId":"20828345-bd21-3c45-a899-e6a97910d663@talktalk.net","threadId":"48293","inReplyTo":"01010162c940b8bb-d8139971-3ee2-4cd6-bb19-35126d46753b-000000@us-west-2.amazonses.com","subject":"Re: Regression in patch add?","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-04-16T10:00:53Z","receivedAt":"2018-04-16T10:01:00Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 15/04/18 13:21, mqudsi wrote:\n> \n> Hello all,\n> \n> I'm currently running the latest version of git built from `master`, and\n> I'm running into what appears to be a regression in the behavior of the\n> piecewise `git add -p` when applying a manually edited chunk.\n> \n> I first run `git add -p`, then manually edit a chunk (after hitting `s`\n> once, if it matters).\n\nThanks for mentioning that, it can matter as the code that stitches\nsplit hunks back together can't cope with edited hunks properly (though\nthe code that checks the hunk immediately after it's been edited doesn't\nbother to try and stitch things back together).\n\n> The chunk originally contains the following:\n> \n> ```diff\n> # Manual hunk edit mode -- see bottom for a quick guide\n> @@ -20,7 +20,7 @@\n>  \t\"call dein#add('Shougo/dein.vim', {'rev': 'master'})\n> \n>  \t\" Add or remove your plugins here:\n> -\t\" call dein#add('flazz/vim-colorschemes')\n> -\tcall dein#add('Haron-Prime/evening_vim')\n> +\tcall dein#add('flazz/vim-colorschemes')\n> +\tcall dein#add('danilo-augusto/vim-afterglow')\n> \n>  \t\"core plugins that change the behavior of vim and how we use it globally\n> ```\n> \n> Under git 2.7.4, I can edit it to the following, which is accepted\n> without a problem:\n> \n> ```diff\n> # Manual hunk edit mode -- see bottom for a quick guide\n> @@ -20,7 +20,7 @@\n> \t\"call dein#add('Shougo/dein.vim', {'rev': 'master'})\n> \n> \t\" Add or remove your plugins here:\n> -\t\" call dein#add('flazz/vim-colorschemes')\n> -\tcall dein#add('Haron-Prime/evening_vim')\n> +\tcall dein#add('flazz/vim-colorschemes')\n> +\tcall dein#add('Haron-Prime/evening_vim')\n> \n> \t\"core plugins that change the behavior of vim and how we use it globally\n> ```\n> \n> All I did here was remove one `+` line and manually add another (which\n> is a variant of the second `-` line).\n> \n> Under git 2.17.0.252.gfe0a9ea, the same piece is opened in $VISUAL for\n> editing (and if left unmodified applies OK), but when modified in the\n> to the same exact value, after exiting the editor I receive the\n> following error from git:\n> \n>     error: patch fragment without header at line 15: @@ -25,7 +25,8 @@\n\nI'm not quite sure what that error message is telling us, I need to\nspend some time understanding the code in apply.c that creates this\nerror message.\n\nI assume that the header is coming from the next hunk which was created\nwhen you split the original hunk. If you could post the original hunk\nbefore it was split and the hunk starting at line 25 after it was split\nthat might help.\n\nAs Martin said if you could share the files or come up with a\nreproducible example that would really help in figuring out what is\ngoing wrong.\n\nThanks for reporting this\n\nPhillip\n\n\n> I'm not sure what to make of this.\n> \n> Thank you,\n> \n> Mahmoud Al-Qudsi\n> NeoSmart Technologies\n> \n> \n\n"},{"id":"344783","messageId":"aa2599d6-6349-0c98-6a31-f68c8b8bf0f8@talktalk.net","threadId":"48293","inReplyTo":"CAN0heSqCZWR1OD4k+-_OBdtCjCNW-UexLo4P-C5XyBqfU6KBEA@mail.gmail.com","subject":"Re: Regression in patch add?","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-04-16T10:01:58Z","receivedAt":"2018-04-16T10:02:08Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 15/04/18 14:59, Martin Ågren wrote:\n> Hi Mahmoud\n> \n> On 15 April 2018 at 14:21,  <mqudsi@neosmart.net> wrote:\n>> I first run `git add -p`, then manually edit a chunk (after hitting `s`\n>> once, if it matters). The chunk originally contains the following:\n> \n> [...]\n> \n>> Under git 2.7.4, I can edit it to the following, which is accepted\n>> without a problem:\n>>\n>> ```diff\n>> # Manual hunk edit mode -- see bottom for a quick guide\n>> @@ -20,7 +20,7 @@\n>>         \"call dein#add('Shougo/dein.vim', {'rev': 'master'})\n>>\n>>         \" Add or remove your plugins here:\n>> -       \" call dein#add('flazz/vim-colorschemes')\n>> -       call dein#add('Haron-Prime/evening_vim')\n>> +       call dein#add('flazz/vim-colorschemes')\n>> +       call dein#add('Haron-Prime/evening_vim')\n>>\n>>         \"core plugins that change the behavior of vim and how we use it globally\n>> ```\n>>\n>> All I did here was remove one `+` line and manually add another (which\n>> is a variant of the second `-` line).\n> \n> So the line is identical (sans s/^-/+/). Interesting.\n> \n>> Under git 2.17.0.252.gfe0a9ea, the same piece is opened in $VISUAL for\n>> editing (and if left unmodified applies OK), but when modified in the\n>> to the same exact value, after exiting the editor I receive the\n>> following error from git:\n>>\n>>     error: patch fragment without header at line 15: @@ -25,7 +25,8 @@\n> \n> I can't seem to reproduce this with some very simple testing. Are you\n> able to share your files? Or even better, derive a minimal reproduction\n> recipe?\n> \n> What happens if you do not do a \"remove this line, then add it again\",\n> but instead turn that unchanged line into context? That is, you edit the\n> hunk into something like this (but without white-space damage):\n> \n> ...\n> -       \" call dein#add('flazz/vim-colorschemes')\n> +       call dein#add('flazz/vim-colorschemes')\n>         call dein#add('Haron-Prime/evening_vim')\n> ...\n\nThat's a good idea to try\n\n> Adding Phillip to cc, since he was recently working in this area\n\nThanks for cc-ing me\n\n> and might have an idea.\n\nI wish I did!\n\nBest Wishes\n\nPhillip\n> \n> Martin\n> \n\n"},{"id":"347208","messageId":"20180510104136.8653-1-oliverjash@gmail.com","threadId":"48293","inReplyTo":"01010162c940b8bb-d8139971-3ee2-4cd6-bb19-35126d46753b-000000@us-west-2.amazonses.com","subject":"Re: Regression in patch add?","fromName":"Oliver Joseph Ash","fromEmail":"oliverjash@gmail.com","sentAt":"2018-05-10T10:41:36Z","receivedAt":"2018-05-10T10:41:51Z","isPatch":false,"sender":{"key":"oliverjash@gmail.com","avatar":"https://gravatar.com/avatar/1266c37ce2a44f50b57deef71172fc89b87db179a0d5448b7c019af6f75eb21d?d=mp&s=160"},"body":"I just ran into a similar problem: https://stackoverflow.com/questions/50258565/git-editing-hunks-fails-when-file-has-other-hunks\n\nI can reproduce on 2.17.0. The issue doesn't occur on 2.16.2, however.\n\nIs this a bug?\n"},{"id":"347212","messageId":"CAN0heSq5SyPgoEURRVHupcabVu3jX+tmX+0U-6azrJDDgfZ5Gw@mail.gmail.com","threadId":"48293","inReplyTo":"20180510104136.8653-1-oliverjash@gmail.com","subject":"Re: Regression in patch add?","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-10T12:17:34Z","receivedAt":"2018-05-10T12:20:45Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 10 May 2018 at 12:41, Oliver Joseph Ash <oliverjash@gmail.com> wrote:\n> I just ran into a similar problem: https://stackoverflow.com/questions/50258565/git-editing-hunks-fails-when-file-has-other-hunks\n>\n> I can reproduce on 2.17.0. The issue doesn't occur on 2.16.2, however.\n>\n> Is this a bug?\n\nI would think so. Thanks for finding this thread. To keep history\naround, it would be nice to have your reproduction recipe on the list,\nnot just on stackoverflow. That said, I cannot reproduce on v2.17.0\nusing your recipe. I suspect there is something quite interesting going\non here, considering how trivial your edit is.\n\nAs a shot in the dark, does your test involve unusual file systems,\nfunny characters in filenames, ..? You are on some sort of Linux, right?\n\nThe first thing to try out might be something like\n\n$ # create the initial file as before, with \"bar\"\n$ # git add, git commit ...\n$ # do the \"change bar to bar1\" everywhere\n$ git diff >test-patch\n$ git reset --hard\n$ # edit the *FIRST* hunk in test.patch like before (bar1 -> bar2)\n$ git apply --check test.patch && echo \"ok...\"\n$ git apply test.patch\n\nDoes that succeed at all?\n\n$ git diff\n\nshould now show bar2 in the first hunk and bar1 in the second hunk,\njust like your edited test.patch.\n\nIf that works, it would seem that the problem is with `git add -p`, and\nhow it is generating the patches for `git apply`. I have some ideas\nabout how to debug from there, but ... How comfortable are you with\nbuilding Git from the sources? Or with temporarily fiddling around with\nyour Git installation? (git-add--interactive is a Perl script, so it\nwould be possible to edit it in place to emit various debug\ninformation. That has potential for messing up royally, though.)\n\nMartin\n"},{"id":"347222","messageId":"20180510131502.17739-1-oliverjash@gmail.com","threadId":"48293","inReplyTo":"01010162c940b8bb-d8139971-3ee2-4cd6-bb19-35126d46753b-000000@us-west-2.amazonses.com","subject":"Re: Regression in patch add?","fromName":"Oliver Joseph Ash","fromEmail":"oliverjash@gmail.com","sentAt":"2018-05-10T13:15:02Z","receivedAt":"2018-05-10T13:15:28Z","isPatch":false,"sender":{"key":"oliverjash@gmail.com","avatar":"https://gravatar.com/avatar/1266c37ce2a44f50b57deef71172fc89b87db179a0d5448b7c019af6f75eb21d?d=mp&s=160"},"body":"> does your test involve unusual file systems, funny characters in filenames, ..? You are on some sort of Linux, right?\n\nI'm running macOS 10.13.4. I don't have any unusual file system setup, as far as I'm aware. The filename in my test case is simply `foo`.\n\nI tried the steps you suggested: on git 2.17.0, saving the patch, editing it, and applying it, and it succeeded.\n\n> should now show bar2 in the first hunk and bar1 in the second hunk, just like your edited test.patch.\n\nThat was the case, although I had to remove the `--check` flag from `git apply`.\n\n> How comfortable are you with building Git from the sources?\n\nI've never done it before, but I assume it's well documented, so I'm willing to give it a shot!\n\nHappy to try any steps to debug this! Although I'm a bit surprised no-one else can reproduce it with the same version of Git, which makes it seem less likely this could be a bug, and more likely it's something in my setup.\n"},{"id":"347223","messageId":"20180510131626.17859-1-oliverjash@gmail.com","threadId":"48293","inReplyTo":"CAN0heSq5SyPgoEURRVHupcabVu3jX+tmX+0U-6azrJDDgfZ5Gw@mail.gmail.com","subject":"Re: Regression in patch add?","fromName":"Oliver Joseph Ash","fromEmail":"oliverjash@gmail.com","sentAt":"2018-05-10T13:16:26Z","receivedAt":"2018-05-10T13:16:32Z","isPatch":false,"sender":{"key":"oliverjash@gmail.com","avatar":"https://gravatar.com/avatar/1266c37ce2a44f50b57deef71172fc89b87db179a0d5448b7c019af6f75eb21d?d=mp&s=160"},"body":"(Apologies, I accidentally sent this as a reply to the original post, instead of your email. I'm new to this!)\n\n> does your test involve unusual file systems, funny characters in filenames, ..? You are on some sort of Linux, right?\n\nI'm running macOS 10.13.4. I don't have any unusual file system setup, as far as I'm aware. The filename in my test case is simply `foo`.\n\nI tried the steps you suggested: on git 2.17.0, saving the patch, editing it, and applying it, and it succeeded.\n\n> should now show bar2 in the first hunk and bar1 in the second hunk, just like your edited test.patch.\n\nThat was the case, although I had to remove the `--check` flag from `git apply`.\n\n> How comfortable are you with building Git from the sources?\n\nI've never done it before, but I assume it's well documented, so I'm willing to give it a shot!\n\nHappy to try any steps to debug this! Although I'm a bit surprised no-one else can reproduce it with the same version of Git, which makes it seem less likely this could be a bug, and more likely it's something in my setup.\n"},{"id":"347224","messageId":"be321106-2f10-e678-8237-449d2dd30fee@talktalk.net","threadId":"48293","inReplyTo":"CAN0heSq5SyPgoEURRVHupcabVu3jX+tmX+0U-6azrJDDgfZ5Gw@mail.gmail.com","subject":"Re: Regression in patch add?","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-05-10T13:49:32Z","receivedAt":"2018-05-10T13:49:38Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 10/05/18 13:17, Martin Ågren wrote:\n> \n> On 10 May 2018 at 12:41, Oliver Joseph Ash <oliverjash@gmail.com> wrote:\n>> I just ran into a similar problem: https://stackoverflow.com/questions/50258565/git-editing-hunks-fails-when-file-has-other-hunks\n>>\n>> I can reproduce on 2.17.0. The issue doesn't occur on 2.16.2, however.\n>>\n>> Is this a bug?\n> \n> I would think so. Thanks for finding this thread. To keep history\n> around, it would be nice to have your reproduction recipe on the list,\n> not just on stackoverflow. That said, I cannot reproduce on v2.17.0\n> using your recipe. I suspect there is something quite interesting going\n> on here, considering how trivial your edit is.\n\nThanks Oliver for posting an example that we can test, that said I can't\nreproduce it on Linux if the hunk is edited correctly. However if I\nremove the leading space from the empty line between 'baz' and 'foo'\nthen I get the same error as you. Perhaps your editor is stripping\ntrailing white space? If so that will lead to problems when editing\ndiffs as the leading space is needed for apply to know that it's an\nempty context line.\n\nFor the mailing list the hunk in question looks like\n@@ -1,5 +1,5 @@\n foo\n-bar\n+bar1\n baz\n\n foo\n\nI've tried using 'git apply --recount --cached' directly and was\nsurprised to see that it accepts the patch with the broken context line.\nIn 2.17.0 'add -p' no longer uses the --recount option, instead it\ncounts the patch it's self but stops counting when it runs out of lines\nstarting with [- +], this explains the difference from earlier versions.\nIt seems it's not uncommon for editors to strip the space from empty\ncontext lines so maybe 'add -p' should take that into account when\nrecounting patches. I'm about to go off line for a couple of weeks so it\nwill probably be next month before I'm able to put a patch together\n(assuming Junio agrees we should support broken hunks)\n\nBest Wishes\n\nPhillip\n\n\n> As a shot in the dark, does your test involve unusual file systems,\n> funny characters in filenames, ..? You are on some sort of Linux, right?\n> \n> The first thing to try out might be something like\n> \n> $ # create the initial file as before, with \"bar\"\n> $ # git add, git commit ...\n> $ # do the \"change bar to bar1\" everywhere\n> $ git diff >test-patch\n> $ git reset --hard\n> $ # edit the *FIRST* hunk in test.patch like before (bar1 -> bar2)\n> $ git apply --check test.patch && echo \"ok...\"\n> $ git apply test.patch\n> \n> Does that succeed at all?\n> \n> $ git diff\n> \n> should now show bar2 in the first hunk and bar1 in the second hunk,\n> just like your edited test.patch.\n> \n> If that works, it would seem that the problem is with `git add -p`, and\n> how it is generating the patches for `git apply`. I have some ideas\n> about how to debug from there, but ... How comfortable are you with\n> building Git from the sources? Or with temporarily fiddling around with\n> your Git installation? (git-add--interactive is a Perl script, so it\n> would be possible to edit it in place to emit various debug\n> information. That has potential for messing up royally, though.)\n> \n> Martin\n> \n\n"},{"id":"347225","messageId":"CAN0heSoNP6ZjoU4x=tjwXxN_4oeOdrPG2LuahTPvGz0Y9WPp3w@mail.gmail.com","threadId":"48293","inReplyTo":"20180510131626.17859-1-oliverjash@gmail.com","subject":"Re: Regression in patch add?","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-10T13:54:19Z","receivedAt":"2018-05-10T13:54:23Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 10 May 2018 at 15:16, Oliver Joseph Ash <oliverjash@gmail.com> wrote:\n> (Apologies, I accidentally sent this as a reply to the original post, instead of your email. I'm new to this!)\n\n(No worries.) ;-)\n\n>> does your test involve unusual file systems, funny characters in filenames, ..? You are on some sort of Linux, right?\n>\n> I'm running macOS 10.13.4. I don't have any unusual file system setup, as far as I'm aware. The filename in my test case is simply `foo`.\n\nI'm not too familiar with Mac, unfortunately, but let's see..\n\n> I tried the steps you suggested: on git 2.17.0, saving the patch, editing it, and applying it, and it succeeded.\n>\n>> should now show bar2 in the first hunk and bar1 in the second hunk, just like your edited test.patch.\n>\n> That was the case, although I had to remove the `--check` flag from `git apply`.\n\nHmm, you mean that `git apply --check test.patch` failed? With error\nmessages? Or, you had to remove the --check flag in order for the patch\nto actually be applied on disk? I would guess it's the latter, but just\nto be clear.\n\n>> How comfortable are you with building Git from the sources?\n>\n> I've never done it before, but I assume it's well documented, so I'm willing to give it a shot!\n>\n> Happy to try any steps to debug this! Although I'm a bit surprised no-one else can reproduce it with the same version of Git, which makes it seem less likely this could be a bug, and more likely it's something in my setup.\n\nWhere do the git 2.17.0 and 2.16.2 come from that you have been testing?\nHomebrew? Apple? (Ple\n\nSo you should be able to do `git clone https://github.com/git/git.git`\nand read INSTALL. It might be useful to start with `git checkout\nv2.17.0` to make sure you're testing roughly the same thing as before.\n\nAs for obtaining the dependencies, since I'm not familiar with Mac, I\ncannot give any good hints.\n\nI see now that Phillip has replied with a good guess. Let's hope he\nhas managed to circle in on what's causing your problem.\n\nMartin\n"},{"id":"347229","messageId":"20180510141125.21677-1-oliverjash@gmail.com","threadId":"48293","inReplyTo":"be321106-2f10-e678-8237-449d2dd30fee@talktalk.net","subject":"Re: Regression in patch add?","fromName":"Oliver Joseph Ash","fromEmail":"oliverjash@gmail.com","sentAt":"2018-05-10T14:11:25Z","receivedAt":"2018-05-10T14:11:32Z","isPatch":false,"sender":{"key":"oliverjash@gmail.com","avatar":"https://gravatar.com/avatar/1266c37ce2a44f50b57deef71172fc89b87db179a0d5448b7c019af6f75eb21d?d=mp&s=160"},"body":"You found the problem Phillip! My editor was trimming trailing white space, which breaks the context line.\n\nI had tried to use an alternative editor to account for any editor specific behaviour, but it turns out both the editors I tested in were doing this!\n\nI suspect this change in behaviour will effect a lot of users? If so, it would be good if `git add -p` allowed for this behaviour, in the same way `git apply` does.\n\nMeanwhile, I can easily configure my editor not to do this for `*.diff` files.\n\nThanks for your help, Phillip and Martin!\n\nMahmoud, does this also explain your problem as per your original post?\n"},{"id":"347278","messageId":"e8aedc6b-5b3e-cfb2-be9d-971bfd9adde8@talktalk.net","threadId":"48293","inReplyTo":"20180510141125.21677-1-oliverjash@gmail.com","subject":"Re: Regression in patch add?","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-05-10T17:58:11Z","receivedAt":"2018-05-10T17:58:17Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 10/05/18 15:11, Oliver Joseph Ash wrote:\n> You found the problem Phillip! My editor was trimming trailing white space, which breaks the context line.\n\nI'm glad we found the source of the problem (and that it wasn't some\nobscure bug)\n\n> I had tried to use an alternative editor to account for any editor specific behaviour, but it turns out both the editors I tested in were doing this!\n> \n> I suspect this change in behaviour will effect a lot of users? If so, it would be good if `git add -p` allowed for this behaviour, in the same way `git apply` does.\n\nYes, I think it probably makes sense to do that. Originally I didn't\ncount empty lines as context lines in case the user accidentally added\nsome empty lines at the end of the hunk but if 'git apply' does then I\nthink 'git add -p' should as well\n\n> Meanwhile, I can easily configure my editor not to do this for `*.diff` files.\n> \n> Thanks for your help, Phillip and Martin!\n\nThanks for posting an example so we could test it, it makes it much\neasier to track the problem down\n\nBest Wishes\n\nPhillip\n\n> Mahmoud, does this also explain your problem as per your original post?\n> \n\n"},{"id":"347327","messageId":"xmqqzi16hpr4.fsf@gitster-ct.c.googlers.com","threadId":"48293","inReplyTo":"e8aedc6b-5b3e-cfb2-be9d-971bfd9adde8@talktalk.net","subject":"Re: Regression in patch add?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-11T02:47:59Z","receivedAt":"2018-05-11T02:48:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood@talktalk.net> writes:\n\n> Yes, I think it probably makes sense to do that. Originally I didn't\n> count empty lines as context lines in case the user accidentally added\n> some empty lines at the end of the hunk but if 'git apply' does then I\n> think 'git add -p' should as well\n\nI am not sure if \"adding to the tail\" should be tolerated, but in\nany case, newer GNU diff can show an empty unaffected line as an\nempty line (unlike traditional unified context format in which such\na line is expressed as a line with a lone SP on it), which is\nallowed as \"implementation defined\" by POSIX.1 [*1*]. Modern \"git\napply\" knows about this.\n\nIf \"add -p\" parses a patch, it should learn to do so, too.\n\n\n[Reference]\n\n*1* http://pubs.opengroup.org/onlinepubs/9699919799/utilities/diff.html\n\n>\n>> Meanwhile, I can easily configure my editor not to do this for `*.diff` files.\n>> \n>> Thanks for your help, Phillip and Martin!\n>\n> Thanks for posting an example so we could test it, it makes it much\n> easier to track the problem down\n>\n> Best Wishes\n>\n> Phillip\n>\n>> Mahmoud, does this also explain your problem as per your original post?\n>> \n"},{"id":"347376","messageId":"9a7d35c7-2889-05e4-f6f3-5706c710d327@talktalk.net","threadId":"48293","inReplyTo":"xmqqzi16hpr4.fsf@gitster-ct.c.googlers.com","subject":"Re: Regression in patch add?","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-05-11T18:23:42Z","receivedAt":"2018-05-11T18:23:53Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 11/05/18 03:47, Junio C Hamano wrote:\n> Phillip Wood <phillip.wood@talktalk.net> writes:\n> \n>> Yes, I think it probably makes sense to do that. Originally I didn't\n>> count empty lines as context lines in case the user accidentally added\n>> some empty lines at the end of the hunk but if 'git apply' does then I\n>> think 'git add -p' should as well\n> \n> I am not sure if \"adding to the tail\" should be tolerated, but in\n> any case, newer GNU diff can show an empty unaffected line as an\n> empty line (unlike traditional unified context format in which such\n> a line is expressed as a line with a lone SP on it), which is\n> allowed as \"implementation defined\" by POSIX.1 [*1*]. Modern \"git\n> apply\" knows about this.\n\nThanks for the reference, I hadn't realized the space was optional.\n\n> If \"add -p\" parses a patch, it should learn to do so, too.\n\nI'm about to go off line for a while, I'll send a fix when I'm back up\nand running at next month (unfortunately the reroll of pw/add-p-select\nwill have to wait until then as well)\n\nBest Wishes\n\nPhillip\n\n\n> \n> [Reference]\n> \n> *1* http://pubs.opengroup.org/onlinepubs/9699919799/utilities/diff.html\n> \n>>\n>>> Meanwhile, I can easily configure my editor not to do this for `*.diff` files.\n>>>\n>>> Thanks for your help, Phillip and Martin!\n>>\n>> Thanks for posting an example so we could test it, it makes it much\n>> easier to track the problem down\n>>\n>> Best Wishes\n>>\n>> Phillip\n>>\n>>> Mahmoud, does this also explain your problem as per your original post?\n>>>\n\n"},{"id":"349006","messageId":"20180601174644.13055-1-phillip.wood@talktalk.net","threadId":"48293","inReplyTo":"01010162c940b8bb-d8139971-3ee2-4cd6-bb19-35126d46753b-000000@us-west-2.amazonses.com","subject":"[PATCH] add -p: fix counting empty context lines in edited patches","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-06-01T17:46:44Z","receivedAt":"2018-06-01T17:47:23Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nrecount_edited_hunk() introduced in commit 2b8ea7f3c7 (\"add -p:\ncalculate offset delta for edited patches\", 2018-03-05) required all\ncontext lines to start with a space, empty lines are not counted. This\nwas intended to avoid any recounting problems if the user had\nintroduced empty lines at the end when editing the patch. However this\nintroduced a regression into 'git add -p' as it seems it is common for\neditors to strip the trailing whitespace from empty context lines when\npatches are edited thereby introducing empty lines that should be\ncounted. 'git apply' knows how to deal with such empty lines and POSIX\nstates that whether or not there is an space on an empty context line\nis implementation defined [1].\n\nFix the regression by counting lines consist solely of a newline as\nwell as lines starting with a space as context lines and add a test to\nprevent future regressions.\n\n[1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/diff.html\n\nReported-by: Mahmoud Al-Qudsi <mqudsi@neosmart.net>\nReported-by: Oliver Joseph Ash <oliverjash@gmail.com>\nReported-by: Jeff Felchner <jfelchner1@gmail.com>\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\nMy apologies to everyone who was affected by this regression.\n\n git-add--interactive.perl  |  2 +-\n t/t3701-add-interactive.sh | 43 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 44 insertions(+), 1 deletion(-)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex ab022ec073..bb6f249f03 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -1047,7 +1047,7 @@ sub recount_edited_hunk {\n \t\t\t$o_cnt++;\n \t\t} elsif ($mode eq '+') {\n \t\t\t$n_cnt++;\n-\t\t} elsif ($mode eq ' ') {\n+\t\t} elsif ($mode eq ' ' or $_ eq \"\\n\") {\n \t\t\t$o_cnt++;\n \t\t\t$n_cnt++;\n \t\t}\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex e5c66f7500..f1bb879ea4 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -175,6 +175,49 @@ test_expect_success 'real edit works' '\n \tdiff_cmp expected output\n '\n \n+test_expect_success 'setup file' '\n+\ttest_write_lines a \"\" b \"\" c >file &&\n+\tgit add file &&\n+\ttest_write_lines a \"\" d \"\" c >file\n+'\n+\n+test_expect_success 'setup patch' '\n+\tSP=\" \" &&\n+\tNULL=\"\" &&\n+\tcat >patch <<-EOF\n+\t@@ -1,4 +1,4 @@\n+\t a\n+\t$NULL\n+\t-b\n+\t+f\n+\t$SP\n+\tc\n+\tEOF\n+'\n+\n+test_expect_success 'setup expected' '\n+\tcat >expected <<-EOF\n+\tdiff --git a/file b/file\n+\tindex b5dd6c9..f910ae9 100644\n+\t--- a/file\n+\t+++ b/file\n+\t@@ -1,5 +1,5 @@\n+\t a\n+\t$SP\n+\t-f\n+\t+d\n+\t$SP\n+\t c\n+\tEOF\n+'\n+\n+test_expect_success 'edit can strip spaces from empty context lines' '\n+\ttest_write_lines e n q | git add -p 2>error &&\n+\ttest_must_be_empty error &&\n+\tgit diff >output &&\n+\tdiff_cmp expected output\n+'\n+\n test_expect_success 'skip files similarly as commit -a' '\n \tgit reset &&\n \techo file >.gitignore &&\n-- \n2.17.0\n\n"},{"id":"349010","messageId":"CA+P7+xokjMcbbrH2iCt5SWMxgsstA+VXHOQgx4debd7Oou-RRA@mail.gmail.com","threadId":"48293","inReplyTo":"20180601174644.13055-1-phillip.wood@talktalk.net","subject":"Re: [PATCH] add -p: fix counting empty context lines in edited patches","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2018-06-01T19:07:35Z","receivedAt":"2018-06-01T19:08:00Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, Jun 1, 2018 at 10:46 AM, Phillip Wood <phillip.wood@talktalk.net> wrote:\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> recount_edited_hunk() introduced in commit 2b8ea7f3c7 (\"add -p:\n> calculate offset delta for edited patches\", 2018-03-05) required all\n> context lines to start with a space, empty lines are not counted. This\n> was intended to avoid any recounting problems if the user had\n> introduced empty lines at the end when editing the patch. However this\n> introduced a regression into 'git add -p' as it seems it is common for\n> editors to strip the trailing whitespace from empty context lines when\n> patches are edited thereby introducing empty lines that should be\n> counted. 'git apply' knows how to deal with such empty lines and POSIX\n> states that whether or not there is an space on an empty context line\n> is implementation defined [1].\n>\n> Fix the regression by counting lines consist solely of a newline as\n> well as lines starting with a space as context lines and add a test to\n> prevent future regressions.\n>\n> [1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/diff.html\n>\n> Reported-by: Mahmoud Al-Qudsi <mqudsi@neosmart.net>\n> Reported-by: Oliver Joseph Ash <oliverjash@gmail.com>\n> Reported-by: Jeff Felchner <jfelchner1@gmail.com>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n> My apologies to everyone who was affected by this regression.\n>\n\nAhhh I suspect this is why my edited code in add -p was sometimes\nfailing to apply!\n\nThanks,\nJake\n\n>  git-add--interactive.perl  |  2 +-\n>  t/t3701-add-interactive.sh | 43 ++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 44 insertions(+), 1 deletion(-)\n>\n> diff --git a/git-add--interactive.perl b/git-add--interactive.perl\n> index ab022ec073..bb6f249f03 100755\n> --- a/git-add--interactive.perl\n> +++ b/git-add--interactive.perl\n> @@ -1047,7 +1047,7 @@ sub recount_edited_hunk {\n>                         $o_cnt++;\n>                 } elsif ($mode eq '+') {\n>                         $n_cnt++;\n> -               } elsif ($mode eq ' ') {\n> +               } elsif ($mode eq ' ' or $_ eq \"\\n\") {\n>                         $o_cnt++;\n>                         $n_cnt++;\n>                 }\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index e5c66f7500..f1bb879ea4 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -175,6 +175,49 @@ test_expect_success 'real edit works' '\n>         diff_cmp expected output\n>  '\n>\n> +test_expect_success 'setup file' '\n> +       test_write_lines a \"\" b \"\" c >file &&\n> +       git add file &&\n> +       test_write_lines a \"\" d \"\" c >file\n> +'\n> +\n> +test_expect_success 'setup patch' '\n> +       SP=\" \" &&\n> +       NULL=\"\" &&\n> +       cat >patch <<-EOF\n> +       @@ -1,4 +1,4 @@\n> +        a\n> +       $NULL\n> +       -b\n> +       +f\n> +       $SP\n> +       c\n> +       EOF\n> +'\n> +\n> +test_expect_success 'setup expected' '\n> +       cat >expected <<-EOF\n> +       diff --git a/file b/file\n> +       index b5dd6c9..f910ae9 100644\n> +       --- a/file\n> +       +++ b/file\n> +       @@ -1,5 +1,5 @@\n> +        a\n> +       $SP\n> +       -f\n> +       +d\n> +       $SP\n> +        c\n> +       EOF\n> +'\n> +\n> +test_expect_success 'edit can strip spaces from empty context lines' '\n> +       test_write_lines e n q | git add -p 2>error &&\n> +       test_must_be_empty error &&\n> +       git diff >output &&\n> +       diff_cmp expected output\n> +'\n> +\n>  test_expect_success 'skip files similarly as commit -a' '\n>         git reset &&\n>         echo file >.gitignore &&\n> --\n> 2.17.0\n>\n"},{"id":"349019","messageId":"CAPig+cSSj2ETXfk8FYUc+=tE6bfoRuqF5Ld4kOgE4+DDpfL+BA@mail.gmail.com","threadId":"48293","inReplyTo":"20180601174644.13055-1-phillip.wood@talktalk.net","subject":"Re: [PATCH] add -p: fix counting empty context lines in edited patches","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-06-01T20:03:59Z","receivedAt":"2018-06-01T20:04:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jun 1, 2018 at 1:46 PM, Phillip Wood <phillip.wood@talktalk.net> wrote:\n> recount_edited_hunk() introduced in commit 2b8ea7f3c7 (\"add -p:\n> calculate offset delta for edited patches\", 2018-03-05) required all\n> context lines to start with a space, empty lines are not counted. This\n> was intended to avoid any recounting problems if the user had\n> introduced empty lines at the end when editing the patch. However this\n> introduced a regression into 'git add -p' as it seems it is common for\n> editors to strip the trailing whitespace from empty context lines when\n> patches are edited thereby introducing empty lines that should be\n> counted. 'git apply' knows how to deal with such empty lines and POSIX\n> states that whether or not there is an space on an empty context line\n> is implementation defined [1].\n>\n> Fix the regression by counting lines consist solely of a newline as\n\ns/consist/&ing/\n--or--\ns/consist/that &/\n\n> well as lines starting with a space as context lines and add a test to\n> prevent future regressions.\n>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>  git-add--interactive.perl  |  2 +-\n> diff --git a/git-add--interactive.perl b/git-add--interactive.perl\n> @@ -1047,7 +1047,7 @@ sub recount_edited_hunk {\n> -               } elsif ($mode eq ' ') {\n> +               } elsif ($mode eq ' ' or $_ eq \"\\n\") {\n\nBased upon a very cursory read of parts of git-add-interactive.perl,\ndo I understand correctly that we don't have to worry about $_ ever\nbeing \"\\r\\n\" on Windows?\n\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> @@ -175,6 +175,49 @@ test_expect_success 'real edit works' '\n> +test_expect_success 'setup file' '\n> +       test_write_lines a \"\" b \"\" c >file &&\n> +       git add file &&\n> +       test_write_lines a \"\" d \"\" c >file\n> +'\n> +\n> +test_expect_success 'setup patch' '\n> +       SP=\" \" &&\n> +       NULL=\"\" &&\n> +       cat >patch <<-EOF\n> +       [...]\n> +       EOF\n> +'\n> +\n> +test_expect_success 'setup expected' '\n> +       cat >expected <<-EOF\n> +       [...]\n> +       EOF\n> +'\n> +\n> +test_expect_success 'edit can strip spaces from empty context lines' '\n> +       test_write_lines e n q | git add -p 2>error &&\n> +       test_must_be_empty error &&\n> +       git diff >output &&\n> +       diff_cmp expected output\n> +'\n\nI would have expected all the setup work to be contained directly in\nthe sole test which needs it rather than spread over three tests (two\nof which are composed of a single command). Not a big deal, and not\nworth a re-roll.\n"},{"id":"349223","messageId":"36f2d9e0-ba79-64d3-ffb5-d0772cafa153@talktalk.net","threadId":"48293","inReplyTo":"CAPig+cSSj2ETXfk8FYUc+=tE6bfoRuqF5Ld4kOgE4+DDpfL+BA@mail.gmail.com","subject":"Re: [PATCH] add -p: fix counting empty context lines in edited patches","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-06-04T10:08:39Z","receivedAt":"2018-06-04T10:08:46Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 01/06/18 21:03, Eric Sunshine wrote:\n> On Fri, Jun 1, 2018 at 1:46 PM, Phillip Wood <phillip.wood@talktalk.net> wrote:\n>> recount_edited_hunk() introduced in commit 2b8ea7f3c7 (\"add -p:\n>> calculate offset delta for edited patches\", 2018-03-05) required all\n>> context lines to start with a space, empty lines are not counted. This\n>> was intended to avoid any recounting problems if the user had\n>> introduced empty lines at the end when editing the patch. However this\n>> introduced a regression into 'git add -p' as it seems it is common for\n>> editors to strip the trailing whitespace from empty context lines when\n>> patches are edited thereby introducing empty lines that should be\n>> counted. 'git apply' knows how to deal with such empty lines and POSIX\n>> states that whether or not there is an space on an empty context line\n>> is implementation defined [1].\n>>\n>> Fix the regression by counting lines consist solely of a newline as\n> \n> s/consist/&ing/\n> --or--\n> s/consist/that &/\n\nThanks, I'd intended to say 'that consist'\n\n>> well as lines starting with a space as context lines and add a test to\n>> prevent future regressions.\n>>\n>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n>> ---\n>>  git-add--interactive.perl  |  2 +-\n>> diff --git a/git-add--interactive.perl b/git-add--interactive.perl\n>> @@ -1047,7 +1047,7 @@ sub recount_edited_hunk {\n>> -               } elsif ($mode eq ' ') {\n>> +               } elsif ($mode eq ' ' or $_ eq \"\\n\") {\n> \n> Based upon a very cursory read of parts of git-add-interactive.perl,\n> do I understand correctly that we don't have to worry about $_ ever\n> being \"\\r\\n\" on Windows?\n>\n\nGood question, I think the short answer no. If my understanding of the\nnewline section of perlport [1] is correct then on Windows \"\\n\" eq\n\"\\012\" and the io layer replaces \"\\015\\012\" with \"\\n\" when reading in\n'text' mode (which I think is the default if you don't specify one when\nopening the file/process or with binmode()). As \"\\n\" is only one\ncharacter it would perhaps be better to test '$mode' rather than '$_'\nabove - what do you think.\n\n[1] http://perldoc.perl.org/perlport.html#Newlines\n\n>> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n>> @@ -175,6 +175,49 @@ test_expect_success 'real edit works' '\n>> +test_expect_success 'setup file' '\n>> +       test_write_lines a \"\" b \"\" c >file &&\n>> +       git add file &&\n>> +       test_write_lines a \"\" d \"\" c >file\n>> +'\n>> +\n>> +test_expect_success 'setup patch' '\n>> +       SP=\" \" &&\n>> +       NULL=\"\" &&\n>> +       cat >patch <<-EOF\n>> +       [...]\n>> +       EOF\n>> +'\n>> +\n>> +test_expect_success 'setup expected' '\n>> +       cat >expected <<-EOF\n>> +       [...]\n>> +       EOF\n>> +'\n>> +\n>> +test_expect_success 'edit can strip spaces from empty context lines' '\n>> +       test_write_lines e n q | git add -p 2>error &&\n>> +       test_must_be_empty error &&\n>> +       git diff >output &&\n>> +       diff_cmp expected output\n>> +'\n> \n> I would have expected all the setup work to be contained directly in\n> the sole test which needs it rather than spread over three tests (two\n> of which are composed of a single command). Not a big deal, and not\n> worth a re-roll.\n\nGood point I was torn between that and matching the existing style in\nthat file seems to be to create a million ancillary tests to do the set-up.\n\nThanks\n\nPhillip\n\n\n"},{"id":"349281","messageId":"CAPig+cSc49B4Mn7m9XOp+QF3qcT06ncGGXJpssFJMNjCQ8e7Mw@mail.gmail.com","threadId":"48293","inReplyTo":"36f2d9e0-ba79-64d3-ffb5-d0772cafa153@talktalk.net","subject":"Re: [PATCH] add -p: fix counting empty context lines in edited patches","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-06-04T17:21:04Z","receivedAt":"2018-06-04T17:21:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 4, 2018 at 6:08 AM, Phillip Wood <phillip.wood@talktalk.net> wrote:\n> On 01/06/18 21:03, Eric Sunshine wrote:\n>> On Fri, Jun 1, 2018 at 1:46 PM, Phillip Wood <phillip.wood@talktalk.net> wrote:\n>>> +               } elsif ($mode eq ' ' or $_ eq \"\\n\") {\n>>\n>> Based upon a very cursory read of parts of git-add-interactive.perl,\n>> do I understand correctly that we don't have to worry about $_ ever\n>> being \"\\r\\n\" on Windows?\n>\n> Good question, I think the short answer no. If my understanding of the\n> newline section of perlport [1] is correct then on Windows \"\\n\" eq\n> \"\\012\" and the io layer replaces \"\\015\\012\" with \"\\n\" when reading in\n> 'text' mode (which I think is the default if you don't specify one when\n> opening the file/process or with binmode()).\n\nThat was my interpretation, as well (though I didn't audit the code closely).\n\n> As \"\\n\" is only one\n> character it would perhaps be better to test '$mode' rather than '$_'\n> above - what do you think.\n\nThat could be clearer. As a reviewer, I had to spend extra brain\ncycles wondering why $mode was used everywhere else but $_ in just\nthis one place. (Not that it was difficult to figure out.)\n\n>>> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n>>> @@ -175,6 +175,49 @@ test_expect_success 'real edit works' '\n>>> +test_expect_success 'setup file' '\n>>> +       [...]\n>>> +'\n>>> +test_expect_success 'setup patch' '\n>>> +       [...]\n>>> +'\n>>> +test_expect_success 'setup expected' '\n>>> +       [...]\n>>> +'\n>>> +test_expect_success 'edit can strip spaces from empty context lines' '\n>>> +       test_write_lines e n q | git add -p 2>error &&\n>>> +       test_must_be_empty error &&\n>>> +       git diff >output &&\n>>> +       diff_cmp expected output\n>>> +'\n>>\n>> I would have expected all the setup work to be contained directly in\n>> the sole test which needs it rather than spread over three tests (two\n>> of which are composed of a single command). Not a big deal, and not\n>> worth a re-roll.\n>\n> Good point I was torn between that and matching the existing style in\n> that file seems to be to create a million ancillary tests to do the set-up.\n\nI see what you mean. Following existing practice in the file makes\nsense, though breaking from that practice by bundling all the setup\ninto the single test which uses it wouldn't hurt either. It's a\njudgment call (and not worrying about too much).\n"},{"id":"349887","messageId":"20180611094602.17469-1-phillip.wood@talktalk.net","threadId":"48293","inReplyTo":"20180601174644.13055-1-phillip.wood@talktalk.net","subject":"[PATCH v2] add -p: fix counting empty context lines in edited patches","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-06-11T09:46:02Z","receivedAt":"2018-06-11T09:46:16Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nrecount_edited_hunk() introduced in commit 2b8ea7f3c7 (\"add -p:\ncalculate offset delta for edited patches\", 2018-03-05) required all\ncontext lines to start with a space, empty lines are not counted. This\nwas intended to avoid any recounting problems if the user had\nintroduced empty lines at the end when editing the patch. However this\nintroduced a regression into 'git add -p' as it seems it is common for\neditors to strip the trailing whitespace from empty context lines when\npatches are edited thereby introducing empty lines that should be\ncounted. 'git apply' knows how to deal with such empty lines and POSIX\nstates that whether or not there is an space on an empty context line\nis implementation defined [1].\n\nFix the regression by counting lines that consist solely of a newline\nas well as lines starting with a space as context lines and add a test\nto prevent future regressions.\n\n[1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/diff.html\n\nReported-by: Mahmoud Al-Qudsi <mqudsi@neosmart.net>\nReported-by: Oliver Joseph Ash <oliverjash@gmail.com>\nReported-by: Jeff Felchner <jfelchner1@gmail.com>\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n\nThanks for the feedback, the only changes since v1 are to fix the\ncommit message to match what was in pu and to change '$_' to '$mode'\nin the comparison as I think that is clearer. In the end I decided to\nleave the tests as they are.\n\n git-add--interactive.perl  |  2 +-\n t/t3701-add-interactive.sh | 43 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 44 insertions(+), 1 deletion(-)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex ab022ec073..8361ef45e7 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -1047,7 +1047,7 @@ sub recount_edited_hunk {\n \t\t\t$o_cnt++;\n \t\t} elsif ($mode eq '+') {\n \t\t\t$n_cnt++;\n-\t\t} elsif ($mode eq ' ') {\n+\t\t} elsif ($mode eq ' ' or $mode eq \"\\n\") {\n \t\t\t$o_cnt++;\n \t\t\t$n_cnt++;\n \t\t}\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex e5c66f7500..f1bb879ea4 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -175,6 +175,49 @@ test_expect_success 'real edit works' '\n \tdiff_cmp expected output\n '\n \n+test_expect_success 'setup file' '\n+\ttest_write_lines a \"\" b \"\" c >file &&\n+\tgit add file &&\n+\ttest_write_lines a \"\" d \"\" c >file\n+'\n+\n+test_expect_success 'setup patch' '\n+\tSP=\" \" &&\n+\tNULL=\"\" &&\n+\tcat >patch <<-EOF\n+\t@@ -1,4 +1,4 @@\n+\t a\n+\t$NULL\n+\t-b\n+\t+f\n+\t$SP\n+\tc\n+\tEOF\n+'\n+\n+test_expect_success 'setup expected' '\n+\tcat >expected <<-EOF\n+\tdiff --git a/file b/file\n+\tindex b5dd6c9..f910ae9 100644\n+\t--- a/file\n+\t+++ b/file\n+\t@@ -1,5 +1,5 @@\n+\t a\n+\t$SP\n+\t-f\n+\t+d\n+\t$SP\n+\t c\n+\tEOF\n+'\n+\n+test_expect_success 'edit can strip spaces from empty context lines' '\n+\ttest_write_lines e n q | git add -p 2>error &&\n+\ttest_must_be_empty error &&\n+\tgit diff >output &&\n+\tdiff_cmp expected output\n+'\n+\n test_expect_success 'skip files similarly as commit -a' '\n \tgit reset &&\n \techo file >.gitignore &&\n-- \n2.17.0\n\n"},{"id":"352293","messageId":"C9B989D9-5148-4AF1-80EB-ADFAE0DB8FF8@gmail.com","threadId":"48293","inReplyTo":"20180611094602.17469-1-phillip.wood@talktalk.net","subject":"Re: [PATCH v2] add -p: fix counting empty context lines in edited patches","fromName":"Jeff Felchner","fromEmail":"jfelchner1@gmail.com","sentAt":"2018-07-11T20:27:57Z","receivedAt":"2018-07-11T20:28:01Z","isPatch":true,"sender":{"key":"jfelchner1@gmail.com","avatar":null},"body":"Hey all, I assumed this was going to be in 2.18, but I'm still having the same issue.  What's the plan for release of this?\n\n> On 2018 Jun 11, at 4:46, Phillip Wood <phillip.wood@talktalk.net> wrote:\n> \n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n> \n> recount_edited_hunk() introduced in commit 2b8ea7f3c7 (\"add -p:\n> calculate offset delta for edited patches\", 2018-03-05) required all\n> context lines to start with a space, empty lines are not counted. This\n> was intended to avoid any recounting problems if the user had\n> introduced empty lines at the end when editing the patch. However this\n> introduced a regression into 'git add -p' as it seems it is common for\n> editors to strip the trailing whitespace from empty context lines when\n> patches are edited thereby introducing empty lines that should be\n> counted. 'git apply' knows how to deal with such empty lines and POSIX\n> states that whether or not there is an space on an empty context line\n> is implementation defined [1].\n> \n> Fix the regression by counting lines that consist solely of a newline\n> as well as lines starting with a space as context lines and add a test\n> to prevent future regressions.\n> \n> [1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/diff.html\n> \n> Reported-by: Mahmoud Al-Qudsi <mqudsi@neosmart.net>\n> Reported-by: Oliver Joseph Ash <oliverjash@gmail.com>\n> Reported-by: Jeff Felchner <jfelchner1@gmail.com>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n> \n> Thanks for the feedback, the only changes since v1 are to fix the\n> commit message to match what was in pu and to change '$_' to '$mode'\n> in the comparison as I think that is clearer. In the end I decided to\n> leave the tests as they are.\n> \n> git-add--interactive.perl  |  2 +-\n> t/t3701-add-interactive.sh | 43 ++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 44 insertions(+), 1 deletion(-)\n> \n> diff --git a/git-add--interactive.perl b/git-add--interactive.perl\n> index ab022ec073..8361ef45e7 100755\n> --- a/git-add--interactive.perl\n> +++ b/git-add--interactive.perl\n> @@ -1047,7 +1047,7 @@ sub recount_edited_hunk {\n> \t\t\t$o_cnt++;\n> \t\t} elsif ($mode eq '+') {\n> \t\t\t$n_cnt++;\n> -\t\t} elsif ($mode eq ' ') {\n> +\t\t} elsif ($mode eq ' ' or $mode eq \"\\n\") {\n> \t\t\t$o_cnt++;\n> \t\t\t$n_cnt++;\n> \t\t}\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index e5c66f7500..f1bb879ea4 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -175,6 +175,49 @@ test_expect_success 'real edit works' '\n> \tdiff_cmp expected output\n> '\n> \n> +test_expect_success 'setup file' '\n> +\ttest_write_lines a \"\" b \"\" c >file &&\n> +\tgit add file &&\n> +\ttest_write_lines a \"\" d \"\" c >file\n> +'\n> +\n> +test_expect_success 'setup patch' '\n> +\tSP=\" \" &&\n> +\tNULL=\"\" &&\n> +\tcat >patch <<-EOF\n> +\t@@ -1,4 +1,4 @@\n> +\t a\n> +\t$NULL\n> +\t-b\n> +\t+f\n> +\t$SP\n> +\tc\n> +\tEOF\n> +'\n> +\n> +test_expect_success 'setup expected' '\n> +\tcat >expected <<-EOF\n> +\tdiff --git a/file b/file\n> +\tindex b5dd6c9..f910ae9 100644\n> +\t--- a/file\n> +\t+++ b/file\n> +\t@@ -1,5 +1,5 @@\n> +\t a\n> +\t$SP\n> +\t-f\n> +\t+d\n> +\t$SP\n> +\t c\n> +\tEOF\n> +'\n> +\n> +test_expect_success 'edit can strip spaces from empty context lines' '\n> +\ttest_write_lines e n q | git add -p 2>error &&\n> +\ttest_must_be_empty error &&\n> +\tgit diff >output &&\n> +\tdiff_cmp expected output\n> +'\n> +\n> test_expect_success 'skip files similarly as commit -a' '\n> \tgit reset &&\n> \techo file >.gitignore &&\n> -- \n> 2.17.0\n> \n\n"},{"id":"352297","messageId":"xmqqy3eh1oqa.fsf@gitster-ct.c.googlers.com","threadId":"48293","inReplyTo":"C9B989D9-5148-4AF1-80EB-ADFAE0DB8FF8@gmail.com","subject":"Re: [PATCH v2] add -p: fix counting empty context lines in edited patches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-11T20:50:21Z","receivedAt":"2018-07-11T20:50:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff Felchner <jfelchner1@gmail.com> writes:\n\n> Hey all, I assumed this was going to be in 2.18, but I'm still having the same issue.  What's the plan for release of this?\n\nYou assumed wrong ;-)  A patch written on June 11th that is already\ndeep into pre-release freeze, unless it is about fixing a regression\nduring the same cycle, would never be in the release tagged on 21st.\n\nIt is already a part of the 'master' branch after v2.18, so v2.19\nwould be the first feature release that would see it (unless we\ndiscover problems in that change and need to revert it, that is).\n"}]}