{"thread":{"id":"49232","subject":"[PATCH] add -p: coalesce hunks before testing applicability","startedAt":"2018-08-28T09:11:54Z","lastAt":"2019-06-04T13:33:00Z","messageCount":12,"participants":["Jochen Sprickerhof","Junio C Hamano","Phillip Wood","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"356648","messageId":"20180828085858.3933-1-git@jochen.sprickerhof.de","threadId":"49232","inReplyTo":null,"subject":"[PATCH] add -p: coalesce hunks before testing applicability","fromName":"Jochen Sprickerhof","fromEmail":"git@jochen.sprickerhof.de","sentAt":"2018-08-28T08:58:58Z","receivedAt":"2018-08-28T09:11:54Z","isPatch":true,"sender":{"key":"git@jochen.sprickerhof.de","avatar":null},"body":"When a hunk was split before being edited manually, it does not apply\nanymore cleanly. Apply coalesce_overlapping_hunks() first to make it\nwork. Enable test for it as well.\n\nSigned-off-by: Jochen Sprickerhof <git@jochen.sprickerhof.de>\n---\n git-add--interactive.perl  | 8 ++++----\n t/t3701-add-interactive.sh | 2 +-\n 2 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex 36f38ced9..c9f434e4a 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -1195,10 +1195,10 @@ sub edit_hunk_loop {\n \t\t# delta from the original unedited hunk.\n \t\t$hunk->{OFS_DELTA} and\n \t\t\t\t$newhunk->{OFS_DELTA} += $hunk->{OFS_DELTA};\n-\t\tif (diff_applies($head,\n-\t\t\t\t @{$hunks}[0..$ix-1],\n-\t\t\t\t $newhunk,\n-\t\t\t\t @{$hunks}[$ix+1..$#{$hunks}])) {\n+\t\tmy @hunk = @{$hunks};\n+\t\tsplice (@hunk, $ix, 1, $newhunk);\n+\t\t@hunk = coalesce_overlapping_hunks(@hunk);\n+\t\tif (diff_applies($head, @hunk)) {\n \t\t\t$newhunk->{DISPLAY} = [color_diff(@{$newtext})];\n \t\t\treturn $newhunk;\n \t\t}\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex b170fb02b..b04806ad7 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -348,7 +348,7 @@ test_expect_success 'split hunk \"add -p (edit)\"' '\n \t! grep \"^+15\" actual\n '\n \n-test_expect_failure 'split hunk \"add -p (no, yes, edit)\"' '\n+test_expect_success 'split hunk \"add -p (no, yes, edit)\"' '\n \ttest_write_lines 5 10 20 21 30 31 40 50 60 >test &&\n \tgit reset &&\n \t# test sequence is s(plit), n(o), y(es), e(dit)\n-- \n2.18.0\n\n"},{"id":"356713","messageId":"xmqq36uygyau.fsf@gitster-ct.c.googlers.com","threadId":"49232","inReplyTo":"20180828085858.3933-1-git@jochen.sprickerhof.de","subject":"Re: [PATCH] add -p: coalesce hunks before testing applicability","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-28T18:07:21Z","receivedAt":"2018-08-28T18:07:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jochen Sprickerhof <git@jochen.sprickerhof.de> writes:\n\n> When a hunk was split before being edited manually, it does not apply\n> anymore cleanly. Apply coalesce_overlapping_hunks() first to make it\n> work. Enable test for it as well.\n>\n> Signed-off-by: Jochen Sprickerhof <git@jochen.sprickerhof.de>\n> ---\n>  git-add--interactive.perl  | 8 ++++----\n>  t/t3701-add-interactive.sh | 2 +-\n>  2 files changed, 5 insertions(+), 5 deletions(-)\n>\n> diff --git a/git-add--interactive.perl b/git-add--interactive.perl\n> index 36f38ced9..c9f434e4a 100755\n> --- a/git-add--interactive.perl\n> +++ b/git-add--interactive.perl\n> @@ -1195,10 +1195,10 @@ sub edit_hunk_loop {\n>  \t\t# delta from the original unedited hunk.\n>  \t\t$hunk->{OFS_DELTA} and\n>  \t\t\t\t$newhunk->{OFS_DELTA} += $hunk->{OFS_DELTA};\n> -\t\tif (diff_applies($head,\n> -\t\t\t\t @{$hunks}[0..$ix-1],\n> -\t\t\t\t $newhunk,\n> -\t\t\t\t @{$hunks}[$ix+1..$#{$hunks}])) {\n> +\t\tmy @hunk = @{$hunks};\n> +\t\tsplice (@hunk, $ix, 1, $newhunk);\n> +\t\t@hunk = coalesce_overlapping_hunks(@hunk);\n> +\t\tif (diff_applies($head, @hunk)) {\n>  \t\t\t$newhunk->{DISPLAY} = [color_diff(@{$newtext})];\n>  \t\t\treturn $newhunk;\n>  \t\t}\n\nOK.  I think I understand how this may be needed in certain forms of\nedit.  I do not think coalesce would reliably work against arbitrary\nkind of edit, but the function is called at the end of the loop of\nthe caller of this function anyway, so it is not like this is making\nanything worse at all.  Let's queue it and try it out.\n\nThanks.\n\nAll of the following is a tangent for future/further work, and\nshould not be done as part of your patch.  While this change may\nwork around the immediate issue of diff_applies() returning \"no\", it\nmakes it feel a bit iffy to keep the interface to return $newhunk.\n\nWith the current interface, the function is saying the caller \"here\nis a text that shows a new hunk, when spliced back into the @hunk\narray and coalesced together with others, would apply cleanly to the\ncurrent index\".  But the corrected code is already doing a trial\nsplice with trial coalescing anyway, so perhaps it may make more\nsense to update the interface into this function for the caller to\nsay \"the user asks to edit $ix'th hunk in @$hunks\" and the function\nto answer \"Here is the edited result in @$hunks; I've made sure it\nwould cleanly apply\".\n\nIncidentally, that would make it possible in the future to allow\nedit_hunk_loop to be told \"the user wants to edit this $ix'th hunk\",\nallow the editor to split that hunk into multiple hunks, and return\nthe result by stuffing them (not just a single $newhunk) into the\n@hunk array.  The caller's loop could then further select or join\nthese hunks---such an interaction is impossible with the current\ninterface that allows edit_hunk_loop to take a single hunk and\nreturn a single newhunk.\n\nBut again, all of the above is a tangent for future/further work,\nand should not be done as part of your patch.\n"},{"id":"356973","messageId":"e5b2900a-0558-d3bf-8ea1-d526b078bbc2@talktalk.net","threadId":"49232","inReplyTo":"xmqq36uygyau.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] add -p: coalesce hunks before testing applicability","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-08-30T13:47:04Z","receivedAt":"2018-08-30T13:47:09Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Dear Jochen/Junio\n\nOn 28/08/18 19:07, Junio C Hamano wrote:\n> Jochen Sprickerhof <git@jochen.sprickerhof.de> writes:\n> \n>> When a hunk was split before being edited manually, it does not apply\n>> anymore cleanly. Apply coalesce_overlapping_hunks() first to make it\n>> work. Enable test for it as well.\n>>\n>> Signed-off-by: Jochen Sprickerhof <git@jochen.sprickerhof.de>\n>> ---\n>>   git-add--interactive.perl  | 8 ++++----\n>>   t/t3701-add-interactive.sh | 2 +-\n>>   2 files changed, 5 insertions(+), 5 deletions(-)\n>>\n>> diff --git a/git-add--interactive.perl b/git-add--interactive.perl\n>> index 36f38ced9..c9f434e4a 100755\n>> --- a/git-add--interactive.perl\n>> +++ b/git-add--interactive.perl\n>> @@ -1195,10 +1195,10 @@ sub edit_hunk_loop {\n>>   \t\t# delta from the original unedited hunk.\n>>   \t\t$hunk->{OFS_DELTA} and\n>>   \t\t\t\t$newhunk->{OFS_DELTA} += $hunk->{OFS_DELTA};\n>> -\t\tif (diff_applies($head,\n>> -\t\t\t\t @{$hunks}[0..$ix-1],\n>> -\t\t\t\t $newhunk,\n>> -\t\t\t\t @{$hunks}[$ix+1..$#{$hunks}])) {\n>> +\t\tmy @hunk = @{$hunks};\n>> +\t\tsplice (@hunk, $ix, 1, $newhunk);\n>> +\t\t@hunk = coalesce_overlapping_hunks(@hunk);\n>> +\t\tif (diff_applies($head, @hunk)) {\n>>   \t\t\t$newhunk->{DISPLAY} = [color_diff(@{$newtext})];\n>>   \t\t\treturn $newhunk;\n>>   \t\t}\n> \n> OK.  I think I understand how this may be needed in certain forms of\n> edit. \n\nWhen $newhunk is created it is marked as dirty to prevent \ncoalesce_overlapping_hunks() from coalescing it. This patch does not \nchange that. What is happening is that by calling \ncoalesce_overlapping_hunks() the hunks that are not currently selected \nare filtered out and any hunks that can be coalesced are (I think that \nin the test that starts passing with this patch the only change is the \nfiltering as there's only a single hunk selected).\n\nThis is a subtle change to the test for the applicability of an edited \nhunk. Previously when all the hunks were used to create the test patch \nwe could be certain that if the test patch applied then if the user \nlater selected any unselected hunk or deselected any selected hunk then \nthat operation would succeed. I'm not sure that is true now (but I \nhaven't thought about it for very long). We could restore the old test \ncondition and coalesce the hunks by copying all the hunks and setting \n$hunk->{USE}=1 when creating the test patch if that turns out to be \nuseful (it would be interesting to see if the test still passes with \nthat change).\n\nBest Wishes\n\nPhillip\n\n  I do not think coalesce would reliably work against arbitrary\n> kind of edit, but the function is called at the end of the loop of\n> the caller of this function anyway, so it is not like this is making\n> anything worse at all.  Let's queue it and try it out.\n> \n> Thanks.\n> \n> All of the following is a tangent for future/further work, and\n> should not be done as part of your patch.  While this change may\n> work around the immediate issue of diff_applies() returning \"no\", it\n> makes it feel a bit iffy to keep the interface to return $newhunk.\n> \n> With the current interface, the function is saying the caller \"here\n> is a text that shows a new hunk, when spliced back into the @hunk\n> array and coalesced together with others, would apply cleanly to the\n> current index\".  But the corrected code is already doing a trial\n> splice with trial coalescing anyway, so perhaps it may make more\n> sense to update the interface into this function for the caller to\n> say \"the user asks to edit $ix'th hunk in @$hunks\" and the function\n> to answer \"Here is the edited result in @$hunks; I've made sure it\n> would cleanly apply\".\n> \n> Incidentally, that would make it possible in the future to allow\n> edit_hunk_loop to be told \"the user wants to edit this $ix'th hunk\",\n> allow the editor to split that hunk into multiple hunks, and return\n> the result by stuffing them (not just a single $newhunk) into the\n> @hunk array.  The caller's loop could then further select or join\n> these hunks---such an interaction is impossible with the current\n> interface that allows edit_hunk_loop to take a single hunk and\n> return a single newhunk.\n> \n> But again, all of the above is a tangent for future/further work,\n> and should not be done as part of your patch.\n> \n"},{"id":"356978","messageId":"xmqqwos77vru.fsf@gitster-ct.c.googlers.com","threadId":"49232","inReplyTo":"e5b2900a-0558-d3bf-8ea1-d526b078bbc2@talktalk.net","subject":"Re: [PATCH] add -p: coalesce hunks before testing applicability","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-30T14:51:17Z","receivedAt":"2018-08-30T14:51:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood@talktalk.net> writes:\n\n> When $newhunk is created it is marked as dirty to prevent\n> coalesce_overlapping_hunks() from coalescing it. This patch does not\n> change that. What is happening is that by calling\n> coalesce_overlapping_hunks() the hunks that are not currently selected\n> are filtered out and any hunks that can be coalesced are (I think that\n> in the test that starts passing with this patch the only change is the\n> filtering as there's only a single hunk selected).\n>\n> This is a subtle change to the test for the applicability of an edited\n> hunk. Previously when all the hunks were used to create the test patch\n> we could be certain that if the test patch applied then if the user\n> later selected any unselected hunk or deselected any selected hunk\n> then that operation would succeed. I'm not sure that is true now (but\n> I haven't thought about it for very long). We could restore the old\n> test condition and coalesce the hunks by copying all the hunks and\n> setting $hunk->{USE}=1 when creating the test patch if that turns out\n> to be useful (it would be interesting to see if the test still passes\n> with that change).\n>\n> Best Wishes\n>\n> Phillip\n\nOK, I marked the topic as \"will merge to next\" but unmark it for\nnow, as we are not in a hurry to graduate new topics to 'master'\nanyway.  Hopefully between Jochen and you, perhaps others, can\nexplore the issues you raised and come to some conclusion before it\nbecomes necessary (i.e. when the next cycle begins).\n\nThanks.\n\n"},{"id":"357252","messageId":"20180903190114.GC17416@vis","threadId":"49232","inReplyTo":"e5b2900a-0558-d3bf-8ea1-d526b078bbc2@talktalk.net","subject":"Re: [PATCH] add -p: coalesce hunks before testing applicability","fromName":"Jochen Sprickerhof","fromEmail":"jochen@sprickerhof.de","sentAt":"2018-09-03T19:01:14Z","receivedAt":"2018-09-03T19:16:42Z","isPatch":true,"sender":{"key":"jochen@sprickerhof.de","avatar":null},"body":"Hi Phillip,\n\n* Phillip Wood <phillip.wood@talktalk.net> [2018-08-30 14:47]:\n>When $newhunk is created it is marked as dirty to prevent \n>coalesce_overlapping_hunks() from coalescing it. This patch does not \n>change that. What is happening is that by calling \n>coalesce_overlapping_hunks() the hunks that are not currently selected \n>are filtered out and any hunks that can be coalesced are (I think that \n>in the test that starts passing with this patch the only change is the \n>filtering as there's only a single hunk selected).\n\nAgreed here. It would be enough to include the first hunk in the test to \nmake it fail again. Still I would see the patch as going in the right \ndirection as we need something like coalesce_overlapping_hunks() to make \nthe hunks applicable after the edit.\n\n>This is a subtle change to the test for the applicability of an edited \n>hunk. Previously when all the hunks were used to create the test patch \n>we could be certain that if the test patch applied then if the user \n>later selected any unselected hunk or deselected any selected hunk \n>then that operation would succeed. I'm not sure that is true now (but \n>I haven't thought about it for very long).\n\nI'm not sure here. If we use the same test from t3701, do s(plit), \ny(es), e(dit), it would fail later on. Can you come up with an example?\n\n> We could restore the old \n>test condition and coalesce the hunks by copying all the hunks and \n>setting $hunk->{USE}=1 when creating the test patch if that turns out \n>to be useful (it would be interesting to see if the test still passes \n>with that change).\n\nWe set USE=1 for $newhunk already, or where would you set it?\n\nCheers Jochen\n"},{"id":"358042","messageId":"d6a8f77b-0a83-90ae-a7fb-a3954ac3b346@talktalk.net","threadId":"49232","inReplyTo":"20180903190114.GC17416@vis","subject":"Re: [PATCH] add -p: coalesce hunks before testing applicability","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-09-13T10:20:28Z","receivedAt":"2018-09-13T10:20:34Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Jochen\n\nOn 03/09/2018 20:01, Jochen Sprickerhof wrote:\n> Hi Phillip,\n> \n> * Phillip Wood <phillip.wood@talktalk.net> [2018-08-30 14:47]:\n>> When $newhunk is created it is marked as dirty to prevent\n>> coalesce_overlapping_hunks() from coalescing it. This patch does not\n>> change that. What is happening is that by calling\n>> coalesce_overlapping_hunks() the hunks that are not currently selected\n>> are filtered out and any hunks that can be coalesced are (I think that\n>> in the test that starts passing with this patch the only change is the\n>> filtering as there's only a single hunk selected).\n> \n> Agreed here. It would be enough to include the first hunk in the test to\n> make it fail again. Still I would see the patch as going in the right\n> direction as we need something like coalesce_overlapping_hunks() to make\n> the hunks applicable after the edit.\n\nYes in the long term we want to be able to coalesce edited hunks, but I\nthink it is confusing to call coalesce_overlapping_hunks() at the moment\nas it will not coalesce the edited hunks.\n\n>> This is a subtle change to the test for the applicability of an edited\n>> hunk. Previously when all the hunks were used to create the test patch\n>> we could be certain that if the test patch applied then if the user\n>> later selected any unselected hunk or deselected any selected hunk\n>> then that operation would succeed. I'm not sure that is true now (but\n>> I haven't thought about it for very long).\n> \n> I'm not sure here. If we use the same test from t3701, do s(plit),\n> y(es), e(dit), it would fail later on. Can you come up with an example?\n\nI think that if you split a hunk, edit the first subhunk, transforming a\ntrailing context line to a deletion then try if you try to stage the\nsecond subhunk it will fail. With your patch the edit will succeed as\nthe second subhunk is skipped when testing the edited patch. Then when\nyou try to stage the second subhunk it will fail as it's leading context\nwill contradict the trailing lines of the edited subhunk. With the old\nmethod the edit failed but didn't store up trouble for the future.\n\n>> We could restore the old test condition and coalesce the hunks by\n>> copying all the hunks and setting $hunk->{USE}=1 when creating the\n>> test patch if that turns out to be useful (it would be interesting to\n>> see if the test still passes with that change).\n> \n> We set USE=1 for $newhunk already, or where would you set it?\n\nTo match the old test it needs to be set on the hunks we've skipped or\nhaven't got to yet so they're all in the patch that's tested after\nediting a hunk.\n\nBest Wishes\n\nPhillip\n\n> \n> Cheers Jochen\n\n"},{"id":"358729","messageId":"20180923171626.GN26251@vis","threadId":"49232","inReplyTo":"d6a8f77b-0a83-90ae-a7fb-a3954ac3b346@talktalk.net","subject":"Re: [PATCH] add -p: coalesce hunks before testing applicability","fromName":"Jochen Sprickerhof","fromEmail":"jochen@sprickerhof.de","sentAt":"2018-09-23T17:16:26Z","receivedAt":"2018-09-23T17:16:32Z","isPatch":true,"sender":{"key":"jochen@sprickerhof.de","avatar":null},"body":"* Phillip Wood <phillip.wood@talktalk.net> [2018-09-13 11:20]:\n>Yes in the long term we want to be able to coalesce edited hunks, but I\n>think it is confusing to call coalesce_overlapping_hunks() at the moment\n>as it will not coalesce the edited hunks.\n\nI would see it as a first step into that direction.\n\n>I think that if you split a hunk, edit the first subhunk, transforming a\n>trailing context line to a deletion then try if you try to stage the\n>second subhunk it will fail. With your patch the edit will succeed as\n>the second subhunk is skipped when testing the edited patch. Then when\n>you try to stage the second subhunk it will fail as it's leading context\n>will contradict the trailing lines of the edited subhunk. With the old\n>method the edit failed but didn't store up trouble for the future.\n\nAgreed. I guess the question is if you assume a hunk to be applied or \nskipped as the default. You can still find enough cases where neither \nthe current nor the patched version works. I stumbled upon the one case \nwhere I wanted to stage only one part of a split hunk and that one \nworked after my patch. I leave it up to you if the added benefit \noverweights the stored up trouble.\n\nCheers Jochen\n"},{"id":"372242","messageId":"nycvar.QRO.7.76.6.1903221453360.41@tvgsbejvaqbjf.bet","threadId":"49232","inReplyTo":"d6a8f77b-0a83-90ae-a7fb-a3954ac3b346@talktalk.net","subject":"Re: [PATCH] add -p: coalesce hunks before testing applicability","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-22T14:06:25Z","receivedAt":"2019-03-22T14:06:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 13 Sep 2018, Phillip Wood wrote:\n\n> On 03/09/2018 20:01, Jochen Sprickerhof wrote:\n>\n> > * Phillip Wood <phillip.wood@talktalk.net> [2018-08-30 14:47]:\n> >> When $newhunk is created it is marked as dirty to prevent\n> >> coalesce_overlapping_hunks() from coalescing it. This patch does not\n> >> change that. What is happening is that by calling\n> >> coalesce_overlapping_hunks() the hunks that are not currently selected\n> >> are filtered out and any hunks that can be coalesced are (I think that\n> >> in the test that starts passing with this patch the only change is the\n> >> filtering as there's only a single hunk selected).\n> >\n> > Agreed here. It would be enough to include the first hunk in the test to\n> > make it fail again. Still I would see the patch as going in the right\n> > direction as we need something like coalesce_overlapping_hunks() to make\n> > the hunks applicable after the edit.\n>\n> Yes in the long term we want to be able to coalesce edited hunks, but I\n> think it is confusing to call coalesce_overlapping_hunks() at the moment\n> as it will not coalesce the edited hunks.\n\nAgreed. I actually have code to coalesce even edited hunks, but it is all\nin C.\n\n> >> This is a subtle change to the test for the applicability of an\n> >> edited hunk. Previously when all the hunks were used to create the\n> >> test patch we could be certain that if the test patch applied then if\n> >> the user later selected any unselected hunk or deselected any\n> >> selected hunk then that operation would succeed. I'm not sure that is\n> >> true now (but I haven't thought about it for very long).\n> >\n> > I'm not sure here. If we use the same test from t3701, do s(plit),\n> > y(es), e(dit), it would fail later on. Can you come up with an\n> > example?\n>\n> I think that if you split a hunk, edit the first subhunk, transforming a\n> trailing context line to a deletion then try if you try to stage the\n> second subhunk it will fail. With your patch the edit will succeed as\n> the second subhunk is skipped when testing the edited patch. Then when\n> you try to stage the second subhunk it will fail as it's leading context\n> will contradict the trailing lines of the edited subhunk. With the old\n> method the edit failed but didn't store up trouble for the future.\n\nIndeed, this is a problem I also stumbled over.\n\n> >> We could restore the old test condition and coalesce the hunks by\n> >> copying all the hunks and setting $hunk->{USE}=1 when creating the\n> >> test patch if that turns out to be useful (it would be interesting to\n> >> see if the test still passes with that change).\n> >\n> > We set USE=1 for $newhunk already, or where would you set it?\n>\n> To match the old test it needs to be set on the hunks we've skipped or\n> haven't got to yet so they're all in the patch that's tested after\n> editing a hunk.\n\nThe way I fixed this in the C code is by teaching the equivalent of the\n`coalesce_overlapping_hunks()` function to simply ignore the equivalent of\n`$hunk->{USE}`: the function signature takes an additional `use_all`\nparameter, which will override the `use` field.\n\nFurthermore, my C code actually does the coalescing as part of the\n`reassemble_patch()` function, feeding the output directly into the\n`stdin` of the `git apply` process (with, or without `--check`).\n\nAnd crucially, my C code does not try to assemble a new `hunks` array, but\nsimply works in-place, reverting the changes if the hunk edits result in a\npatch that does not apply. The Perl version probably does not need that\npart, as it is pretty careless with memory (as Perl encourages to do).\n\nSee for yourself:\nhttps://github.com/dscho/git/commit/6f8ac4809280f2cd018683ab5199b004ada2350e#diff-f58d2179be56b196b9f35c6d24799a8eR337\n\nCiao,\nDscho\n\nP.S.: Yes, this is part of my work to complete Slavica's \"`git add -i`\nin C\" project. There are quite a few loose ends to tie, but I can already\nuse it myself to call `git add -p`, which is what I care most about, as it\nregularly takes more than one second to spin up Perl on Windows, which is\nfriggin' annoying, I tell ya.\n"},{"id":"376548","messageId":"a23789e9-ee99-d23b-ee25-1acef8d8d114@gmail.com","threadId":"49232","inReplyTo":"nycvar.QRO.7.76.6.1903221453360.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] add -p: coalesce hunks before testing applicability","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-06-02T14:17:13Z","receivedAt":"2019-06-02T14:17:20Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\n\nSorry it's taken me so long to get round to replying to this\n\nOn 22/03/2019 14:06, Johannes Schindelin wrote:\n> Hi,\n> \n> On Thu, 13 Sep 2018, Phillip Wood wrote:\n> \n>> On 03/09/2018 20:01, Jochen Sprickerhof wrote:\n>>\n>>> * Phillip Wood <phillip.wood@talktalk.net> [2018-08-30 14:47]:\n>>>> When $newhunk is created it is marked as dirty to prevent\n>>>> coalesce_overlapping_hunks() from coalescing it. This patch does not\n>>>> change that. What is happening is that by calling\n>>>> coalesce_overlapping_hunks() the hunks that are not currently selected\n>>>> are filtered out and any hunks that can be coalesced are (I think that\n>>>> in the test that starts passing with this patch the only change is the\n>>>> filtering as there's only a single hunk selected).\n>>>\n>>> Agreed here. It would be enough to include the first hunk in the test to\n>>> make it fail again. Still I would see the patch as going in the right\n>>> direction as we need something like coalesce_overlapping_hunks() to make\n>>> the hunks applicable after the edit.\n>>\n>> Yes in the long term we want to be able to coalesce edited hunks, but I\n>> think it is confusing to call coalesce_overlapping_hunks() at the moment\n>> as it will not coalesce the edited hunks.\n> \n> Agreed. I actually have code to coalesce even edited hunks, but it is all\n> in C.\n> \n>>>> This is a subtle change to the test for the applicability of an\n>>>> edited hunk. Previously when all the hunks were used to create the\n>>>> test patch we could be certain that if the test patch applied then if\n>>>> the user later selected any unselected hunk or deselected any\n>>>> selected hunk then that operation would succeed. I'm not sure that is\n>>>> true now (but I haven't thought about it for very long).\n>>>\n>>> I'm not sure here. If we use the same test from t3701, do s(plit),\n>>> y(es), e(dit), it would fail later on. Can you come up with an\n>>> example?\n>>\n>> I think that if you split a hunk, edit the first subhunk, transforming a\n>> trailing context line to a deletion then try if you try to stage the\n>> second subhunk it will fail. With your patch the edit will succeed as\n>> the second subhunk is skipped when testing the edited patch. Then when\n>> you try to stage the second subhunk it will fail as it's leading context\n>> will contradict the trailing lines of the edited subhunk. With the old\n>> method the edit failed but didn't store up trouble for the future.\n> \n> Indeed, this is a problem I also stumbled over.\n> \n>>>> We could restore the old test condition and coalesce the hunks by\n>>>> copying all the hunks and setting $hunk->{USE}=1 when creating the\n>>>> test patch if that turns out to be useful (it would be interesting to\n>>>> see if the test still passes with that change).\n>>>\n>>> We set USE=1 for $newhunk already, or where would you set it?\n>>\n>> To match the old test it needs to be set on the hunks we've skipped or\n>> haven't got to yet so they're all in the patch that's tested after\n>> editing a hunk.\n> \n> The way I fixed this in the C code is by teaching the equivalent of the\n> `coalesce_overlapping_hunks()` function to simply ignore the equivalent of\n> `$hunk->{USE}`: the function signature takes an additional `use_all`\n> parameter, which will override the `use` field.\n\nThat sounds like a good solution. Thanks for working on the conversion \nto C, I'll try and find time look at the code on github.\n\nBest Wishes\n\nPhillip\n\n> \n> Furthermore, my C code actually does the coalescing as part of the\n> `reassemble_patch()` function, feeding the output directly into the\n> `stdin` of the `git apply` process (with, or without `--check`).\n> \n> And crucially, my C code does not try to assemble a new `hunks` array, but\n> simply works in-place, reverting the changes if the hunk edits result in a\n> patch that does not apply. The Perl version probably does not need that\n> part, as it is pretty careless with memory (as Perl encourages to do).\n> \n> See for yourself:\n> https://github.com/dscho/git/commit/6f8ac4809280f2cd018683ab5199b004ada2350e#diff-f58d2179be56b196b9f35c6d24799a8eR337\n> \n> Ciao,\n> Dscho\n> \n> P.S.: Yes, this is part of my work to complete Slavica's \"`git add -i`\n> in C\" project. There are quite a few loose ends to tie, but I can already\n> use it myself to call `git add -p`, which is what I care most about, as it\n> regularly takes more than one second to spin up Perl on Windows, which is\n> friggin' annoying, I tell ya.\n> \n"},{"id":"376564","messageId":"nycvar.QRO.7.76.6.1906031229110.48@tvgsbejvaqbjf.bet","threadId":"49232","inReplyTo":"a23789e9-ee99-d23b-ee25-1acef8d8d114@gmail.com","subject":"Re: [PATCH] add -p: coalesce hunks before testing applicability","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-06-03T13:40:39Z","receivedAt":"2019-06-03T13:40:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Sun, 2 Jun 2019, Phillip Wood wrote:\n\n> On 22/03/2019 14:06, Johannes Schindelin wrote:\n>\n> > On Thu, 13 Sep 2018, Phillip Wood wrote:\n> >\n> > > On 03/09/2018 20:01, Jochen Sprickerhof wrote:\n> > >\n> > > > * Phillip Wood <phillip.wood@talktalk.net> [2018-08-30 14:47]:\n> > > >\n> > > > > We could restore the old test condition and coalesce the hunks\n> > > > > by copying all the hunks and setting $hunk->{USE}=1 when\n> > > > > creating the test patch if that turns out to be useful (it would\n> > > > > be interesting to see if the test still passes with that\n> > > > > change).\n> > > >\n> > > > We set USE=1 for $newhunk already, or where would you set it?\n> > >\n> > > To match the old test it needs to be set on the hunks we've skipped\n> > > or haven't got to yet so they're all in the patch that's tested\n> > > after editing a hunk.\n> >\n> > The way I fixed this in the C code is by teaching the equivalent of\n> > the `coalesce_overlapping_hunks()` function to simply ignore the\n> > equivalent of `$hunk->{USE}`: the function signature takes an\n> > additional `use_all` parameter, which will override the `use` field.\n>\n> That sounds like a good solution. Thanks for working on the conversion\n> to C, I'll try and find time look at the code on github.\n\nPlease note that I did not update the Pull Requests on GitGitGadget\nlately, as I had no reviewer feedback on #170 and did not want to waste\ntoo much time on synchronizing my work between those PRs and Git for Windows\n(which now has the built-in `git add -i` as an opt-in feature).\n\nSo: the latest patches (as of time of writing) can be found here:\nhttps://github.com/git-for-windows/git/compare/9f09372011%5E...9f09372011%5E2\n\nThanks,\nDscho\n"},{"id":"376570","messageId":"3ac6c94c-edd5-b376-4d44-cbf7aebf37a0@gmail.com","threadId":"49232","inReplyTo":"nycvar.QRO.7.76.6.1906031229110.48@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] add -p: coalesce hunks before testing applicability","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-06-03T14:59:42Z","receivedAt":"2019-06-03T14:59:47Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi\n\nOn 03/06/2019 14:40, Johannes Schindelin wrote:\n> Hi Phillip,\n> \n> On Sun, 2 Jun 2019, Phillip Wood wrote:\n> \n>> On 22/03/2019 14:06, Johannes Schindelin wrote:\n>>\n>>> On Thu, 13 Sep 2018, Phillip Wood wrote:\n>>>\n>>>> On 03/09/2018 20:01, Jochen Sprickerhof wrote:\n>>>>\n>>>>> * Phillip Wood <phillip.wood@talktalk.net> [2018-08-30 14:47]:\n>>>>>\n>>>>>> We could restore the old test condition and coalesce the hunks\n>>>>>> by copying all the hunks and setting $hunk->{USE}=1 when\n>>>>>> creating the test patch if that turns out to be useful (it would\n>>>>>> be interesting to see if the test still passes with that\n>>>>>> change).\n>>>>>\n>>>>> We set USE=1 for $newhunk already, or where would you set it?\n>>>>\n>>>> To match the old test it needs to be set on the hunks we've skipped\n>>>> or haven't got to yet so they're all in the patch that's tested\n>>>> after editing a hunk.\n>>>\n>>> The way I fixed this in the C code is by teaching the equivalent of\n>>> the `coalesce_overlapping_hunks()` function to simply ignore the\n>>> equivalent of `$hunk->{USE}`: the function signature takes an\n>>> additional `use_all` parameter, which will override the `use` field.\n>>\n>> That sounds like a good solution. Thanks for working on the conversion\n>> to C, I'll try and find time look at the code on github.\n> \n> Please note that I did not update the Pull Requests on GitGitGadget\n> lately, as I had no reviewer feedback on #170 and did not want to waste\n> too much time on synchronizing my work between those PRs and Git for Windows\n> (which now has the built-in `git add -i` as an opt-in feature).\n> \n> So: the latest patches (as of time of writing) can be found here:\n> https://github.com/git-for-windows/git/compare/9f09372011%5E...9f09372011%5E2\n\nThanks, I left some comments on the commits on the add-p-in-c branch at \nhttps://github.com/dscho [1,2] before I saw your email. I've still got \nquite a few commits to look at so I'll leave any further comments on the \ngit-for-windows repo instead.\n\nBest Wishes\n\nPhillip\n[1] \nhttps://github.com/dscho/git/commit/9b7d0fbb095e9e14491d4344981ae346c97e1692\n\n[2] \nhttps://github.com/dscho/git/commit/74e058179ae7743a8a99e5a20d5362bf55563505\n\n"},{"id":"376653","messageId":"nycvar.QRO.7.76.6.1906041532000.1775@tvgsbejvaqbjf.bet","threadId":"49232","inReplyTo":"3ac6c94c-edd5-b376-4d44-cbf7aebf37a0@gmail.com","subject":"Re: [PATCH] add -p: coalesce hunks before testing applicability","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-06-04T13:32:33Z","receivedAt":"2019-06-04T13:33:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Mon, 3 Jun 2019, Phillip Wood wrote:\n\n> On 03/06/2019 14:40, Johannes Schindelin wrote:\n> >\n> > On Sun, 2 Jun 2019, Phillip Wood wrote:\n> >\n> > > On 22/03/2019 14:06, Johannes Schindelin wrote:\n> > >\n> > > > On Thu, 13 Sep 2018, Phillip Wood wrote:\n> > > >\n> > > > > On 03/09/2018 20:01, Jochen Sprickerhof wrote:\n> > > > >\n> > > > > > * Phillip Wood <phillip.wood@talktalk.net> [2018-08-30 14:47]:\n> > > > > >\n> > > > > > > We could restore the old test condition and coalesce the\n> > > > > > > hunks by copying all the hunks and setting $hunk->{USE}=1\n> > > > > > > when creating the test patch if that turns out to be useful\n> > > > > > > (it would be interesting to see if the test still passes\n> > > > > > > with that change).\n> > > > > >\n> > > > > > We set USE=1 for $newhunk already, or where would you set it?\n> > > > >\n> > > > > To match the old test it needs to be set on the hunks we've\n> > > > > skipped or haven't got to yet so they're all in the patch that's\n> > > > > tested after editing a hunk.\n> > > >\n> > > > The way I fixed this in the C code is by teaching the equivalent\n> > > > of the `coalesce_overlapping_hunks()` function to simply ignore\n> > > > the equivalent of `$hunk->{USE}`: the function signature takes an\n> > > > additional `use_all` parameter, which will override the `use`\n> > > > field.\n> > >\n> > > That sounds like a good solution. Thanks for working on the\n> > > conversion to C, I'll try and find time look at the code on github.\n> >\n> > Please note that I did not update the Pull Requests on GitGitGadget\n> > lately, as I had no reviewer feedback on #170 and did not want to\n> > waste too much time on synchronizing my work between those PRs and Git\n> > for Windows (which now has the built-in `git add -i` as an opt-in\n> > feature).\n> >\n> > So: the latest patches (as of time of writing) can be found here:\n> > https://github.com/git-for-windows/git/compare/9f09372011%5E...9f09372011%5E2\n>\n> Thanks, I left some comments on the commits on the add-p-in-c branch at\n> https://github.com/dscho [1,2] before I saw your email. I've still got\n> quite a few commits to look at so I'll leave any further comments on the\n> git-for-windows repo instead.\n\nThank you so much! I hope to get to your comments very soon ;-)\n\nCiao,\nDscho\n\n> [1]\n> https://github.com/dscho/git/commit/9b7d0fbb095e9e14491d4344981ae346c97e1692\n>\n> [2]\n> https://github.com/dscho/git/commit/74e058179ae7743a8a99e5a20d5362bf55563505\n>\n>\n"}]}