{"thread":{"id":"55999","subject":"git add --interactive patch improvement for split hunks","startedAt":"2021-06-24T10:42:26Z","lastAt":"2021-06-30T17:06:40Z","messageCount":9,"participants":["Ulrich Windl","Jeff King","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"428369","messageId":"60D45FE4020000A100041FCE@gwsmtp.uni-regensburg.de","threadId":"55999","inReplyTo":null,"subject":"git add --interactive patch improvement for split hunks","fromName":"Ulrich Windl","fromEmail":"ulrich.windl@rz.uni-regensburg.de","sentAt":"2021-06-24T10:35:16Z","receivedAt":"2021-06-24T10:42:26Z","isPatch":false,"sender":{"key":"ulrich.windl@rz.uni-regensburg.de","avatar":null},"body":"Hi!\n\n\nI noticed that git add -interactive's patch displays the function context for the diffs, but that function context is lost when the hunks are split.\nIt would help the user (especially for hunks covering multiple functioins) if function context were still provided for split hunks.\n\n\nMaybe just consider this (buggy) example (with screen shot in case the lines get severely mangled):\n(15/20) Stage this hunk [y,n,q,a,d,K,j,J,g,/,e,?]? n\n@@ -1097,13 +1743,13 @@ static inline void bitmap_copy_bits(\n }\n else\n {\n- const unsigned s_i = FASTBIT(s_pos);\n- const unsigned d_i = FASTBIT(d_pos);\n+ const unsigned s_i = FASTBIT(s_pos + count);\n+ const unsigned d_i = FASTBIT(d_pos + count);\n const fastword_t *s_fw_p;\n fastword_t *d_fw_p;\n \n- for ( s_fw_p = src->fast_words + FASTWORD(s_pos),\n- d_fw_p = dst->fast_words + FASTWORD(d_pos);\n+ for ( s_fw_p = src->fast_words + FASTWORD(s_pos + count),\n+ d_fw_p = dst->fast_words + FASTWORD(d_pos + count);\n count >= FASTWORD_BITS;\n --s_fw_p, --d_fw_p, count -= FASTWORD_BITS )\n {\n(16/20) Stage this hunk [y,n,q,a,d,K,j,J,g,/,s,e,?]? s\nSplit into 2 hunks.\n@@ -1097,8 +1743,8 @@\n }\n else\n {\n- const unsigned s_i = FASTBIT(s_pos);\n- const unsigned d_i = FASTBIT(d_pos);\n+ const unsigned s_i = FASTBIT(s_pos + count);\n+ const unsigned d_i = FASTBIT(d_pos + count);\n const fastword_t *s_fw_p;\n fastword_t *d_fw_p;\n \n(16/21) Stage this hunk [y,n,q,a,d,K,j,J,g,/,e,?]? n\n@@ -1102,8 +1748,8 @@\n const fastword_t *s_fw_p;\n fastword_t *d_fw_p;\n \n- for ( s_fw_p = src->fast_words + FASTWORD(s_pos),\n- d_fw_p = dst->fast_words + FASTWORD(d_pos);\n+ for ( s_fw_p = src->fast_words + FASTWORD(s_pos + count),\n+ d_fw_p = dst->fast_words + FASTWORD(d_pos + count);\n count >= FASTWORD_BITS;\n --s_fw_p, --d_fw_p, count -= FASTWORD_BITS )\n {\n(17/21) Stage this hunk [y,n,q,a,d,K,j,J,g,/,e,?]?\n\n\n\n"},{"id":"428382","messageId":"YNSnlhbE30xDfVMY@coredump.intra.peff.net","threadId":"55999","inReplyTo":"60D45FE4020000A100041FCE@gwsmtp.uni-regensburg.de","subject":"Re: git add --interactive patch improvement for split hunks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-24T15:41:10Z","receivedAt":"2021-06-24T15:41:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 24, 2021 at 12:35:16PM +0200, Ulrich Windl wrote:\n\n> I noticed that git add -interactive's patch displays the function\n> context for the diffs, but that function context is lost when the\n> hunks are split.\n>\n> It would help the user (especially for hunks covering multiple\n> functioins) if function context were still provided for split hunks.\n\nThis was discussed a while ago (and there is even a patch) in this\nthread:\n\n https://lore.kernel.org/git/20201117020522.GD19433@coredump.intra.peff.net/\n\nThe short of it is that the upcoming builtin-in-C version of the code\nwill preserve the function header when splitting. The patch in that\nmessage adds it to the existing perl version, but I didn't really bother\nmoving it forward, since that code is all supposed to eventually go\naway[0].\n\nOne thing you may not like, though: both the builtin version and that\npatch only put the funcname context in the _first_ hunk of the split.\nDoing it for subsequent hunks is much trickier, since there can be a\nfuncname in the split context itself. E.g.:\n\n @@ ... @@ void foo()\n int x;\n - int y = 1;\n + int y = 2;\n \n - x = 3;\n + x = 4;\n }\n\ncould split into two hunks, both annotated with \"void foo()\". But:\n\n @@ ... @@ void foo()\n int x;\n - x = 3;\n + x = 4;\n }\n void bar()\n {\n - int y = 1;\n + int y = 2;\n }\n\nwould be wrong to say \"void foo()\" for the second hunk. We'd have to\nre-scan the interior context lines for a funcname to find it. That's\nall-but-impossible in the perl version, but might be do-able in the C\nversion (since it has easy access to the funcname-matching patterns and\nmachinery).\n\n-Peff\n\n[0] I'm not sure what the timetable is for switching to the C version of\n add--interactive. If it's going to be a while, I don't mind moving\n forward the other patch I showed. But maybe the time is here to\n think about switching the default of add.interactive.useBuiltin, and\n ironing out any final bugs?\n"},{"id":"428601","messageId":"60D9A01C020000A100042099@gwsmtp.uni-regensburg.de","threadId":"55999","inReplyTo":"YNSnlhbE30xDfVMY@coredump.intra.peff.net","subject":"Antw: [EXT] Re: git add --interactive patch improvement for split hunks","fromName":"Ulrich Windl","fromEmail":"ulrich.windl@rz.uni-regensburg.de","sentAt":"2021-06-28T10:10:36Z","receivedAt":"2021-06-28T10:10:44Z","isPatch":false,"sender":{"key":"ulrich.windl@rz.uni-regensburg.de","avatar":null},"body":">>> Jeff King schrieb am 24.06.2021 um 17:41 in Nachricht\n:\n\n[...]\n> One thing you may not like, though: both the builtin version and that\n> patch only put the funcname context in the _first_ hunk of the split.\n> Doing it for subsequent hunks is much trickier, since there can be a\n> funcname in the split context itself. E.g.:\n> \n> @@ ... @@ void foo()\n> int x;\n> - int y = 1;\n> + int y = 2;\n> \n> - x = 3;\n> + x = 4;\n> }\n> \n> could split into two hunks, both annotated with \"void foo()\". But:\n> \n> @@ ... @@ void foo()\n> int x;\n> - x = 3;\n> + x = 4;\n> }\n> void bar()\n> {\n> - int y = 1;\n> + int y = 2;\n> }\n> \n> would be wrong to say \"void foo()\" for the second hunk. We'd have to\n> re-scan the interior context lines for a funcname to find it. That's\n> all-but-impossible in the perl version, but might be do-able in the C\n> version (since it has easy access to the funcname-matching patterns and\n> machinery).\n\nThere always was a related bug (IMHO) that showed the context of the previous function even though the actual change was within a new function (that starts within the context lines). So if that bug were fixed, my guess is that the other would be as well.\nHowever I don't know how easy or hard the fix will be.\nMaybe the \"definition\" of function context is just different; I don't really know.\n\n> \n> -Peff\n> \n> [0] I'm not sure what the timetable is for switching to the C version of\n> add--interactive. If it's going to be a while, I don't mind moving\n> forward the other patch I showed. But maybe the time is here to\n> think about switching the default of add.interactive.useBuiltin, and\n> ironing out any final bugs?\n\n\n\n\n"},{"id":"428603","messageId":"87eecmgrnx.fsf@evledraar.gmail.com","threadId":"55999","inReplyTo":"60D9A01C020000A100042099@gwsmtp.uni-regensburg.de","subject":"Re: Antw: [EXT] Re: git add --interactive patch improvement for split hunks","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-28T11:20:46Z","receivedAt":"2021-06-28T11:22:32Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jun 28 2021, Ulrich Windl wrote:\n\n>>>> Jeff King schrieb am 24.06.2021 um 17:41 in Nachricht\n> :\n>\n> [...]\n>> One thing you may not like, though: both the builtin version and that\n>> patch only put the funcname context in the _first_ hunk of the split.\n>> Doing it for subsequent hunks is much trickier, since there can be a\n>> funcname in the split context itself. E.g.:\n>> \n>> @@ ... @@ void foo()\n>> int x;\n>> - int y = 1;\n>> + int y = 2;\n>> \n>> - x = 3;\n>> + x = 4;\n>> }\n>> \n>> could split into two hunks, both annotated with \"void foo()\". But:\n>> \n>> @@ ... @@ void foo()\n>> int x;\n>> - x = 3;\n>> + x = 4;\n>> }\n>> void bar()\n>> {\n>> - int y = 1;\n>> + int y = 2;\n>> }\n>> \n>> would be wrong to say \"void foo()\" for the second hunk. We'd have to\n>> re-scan the interior context lines for a funcname to find it. That's\n>> all-but-impossible in the perl version, but might be do-able in the C\n>> version (since it has easy access to the funcname-matching patterns and\n>> machinery).\n>\n> There always was a related bug (IMHO) that showed the context of the\n> previous function even though the actual change was within a new\n> function (that starts within the context lines). So if that bug were\n> fixed, my guess is that the other would be as well.\n> However I don't know how easy or hard the fix will be.\n> Maybe the \"definition\" of function context is just different; I don't really know.\n\nDoes that bug perhaps have anything to do with:\nhttps://lore.kernel.org/git/20210215155020.2804-2-avarab@gmail.com/\n\nI have some planned fixes to that behavior, but it's currently blocked\non a combination of myself having a lot of outstanding patches, and that\nlinked patch needing another series (as of yet unsubmitted/un-re-rolled)\nto get us proper testing in this area of git. I.e. our testing of what\nfunction context we should find is really lacking.\n"},{"id":"428797","messageId":"YNvT+tUlW98dQY3B@coredump.intra.peff.net","threadId":"55999","inReplyTo":"87eecmgrnx.fsf@evledraar.gmail.com","subject":"Re: Antw: [EXT] Re: git add --interactive patch improvement for split hunks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-30T02:16:26Z","receivedAt":"2021-06-30T02:16:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 28, 2021 at 01:20:46PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > There always was a related bug (IMHO) that showed the context of the\n> > previous function even though the actual change was within a new\n> > function (that starts within the context lines). So if that bug were\n> > fixed, my guess is that the other would be as well.\n> > However I don't know how easy or hard the fix will be.\n> > Maybe the \"definition\" of function context is just different; I don't really know.\n> \n> Does that bug perhaps have anything to do with:\n> https://lore.kernel.org/git/20210215155020.2804-2-avarab@gmail.com/\n\nI think it's similar. The issue is that we search backwards for a\nfuncname match from the top of the hunk, _not_ from the first changed\nline. IIRC, that has been discussed before and considered \"not a bug\",\nbut I could be mis-remembering (and it's a tricky thing to search in the\narchive for[0]).\n\nThe problem with split hunks is different, though; we do not search for\na funcname line at all on the second half of the hunk.\n\n-Peff\n\n[0] I did come up with:\n\n https://lore.kernel.org/git/1399824596-4670-1-git-send-email-avarab@gmail.com/\n\n which has discussion between you and me on this very same splitting\n topic back in 2014! Double-curious, your patch there implements the\n same \"keep the hunk header on split\" we've been discussing here, and\n we were all positive on it. Yet it doesn't seem to have ever gotten\n applied.\n\n It looks like Junio carried it in \"What's Cooking\" for almost a\n year, marked as \"waiting for re-roll\" to handle the squash, but then\n eventually discarded it as stale. :(\n"},{"id":"428806","messageId":"xmqq5yxvvq7k.fsf@gitster.g","threadId":"55999","inReplyTo":"YNvT+tUlW98dQY3B@coredump.intra.peff.net","subject":"Re: Antw: [EXT] Re: git add --interactive patch improvement for split hunks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-30T06:09:19Z","receivedAt":"2021-06-30T06:10:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> It looks like Junio carried it in \"What's Cooking\" for almost a\n> year, marked as \"waiting for re-roll\" to handle the squash, but then\n> eventually discarded it as stale. :(\n\nHeh, thanks for digging.\n\nIs the moral of the story that we should merge down unfinished\ntopics more aggressively (hoping that the untied loose ends would be\ntied after they hit released version), we should prod owners of\nstalled topics with sharper stick more often, or something else?\n"},{"id":"428809","messageId":"YNwd6wmt4FTyySgH@coredump.intra.peff.net","threadId":"55999","inReplyTo":"xmqq5yxvvq7k.fsf@gitster.g","subject":"Re: Antw: [EXT] Re: git add --interactive patch improvement for split hunks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-30T07:31:55Z","receivedAt":"2021-06-30T07:31:59Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 29, 2021 at 11:09:19PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > It looks like Junio carried it in \"What's Cooking\" for almost a\n> > year, marked as \"waiting for re-roll\" to handle the squash, but then\n> > eventually discarded it as stale. :(\n> \n> Heh, thanks for digging.\n> \n> Is the moral of the story that we should merge down unfinished\n> topics more aggressively (hoping that the untied loose ends would be\n> tied after they hit released version), we should prod owners of\n> stalled topics with sharper stick more often, or something else?\n\nI'm not sure. I think the topic would have graduated if either you had\njust applied the squash and merged it down, or if the original author\nhad checked back in over the intervening year to say \"hey, what happened\nto my patch\" (either by reading \"what's cooking\" or manually).\n\nI suspect drive-by contributors might not realize they need to do the\nlatter in some cases, but I wouldn't have counted 2014-era Ævar in that\nboat. So I dunno.\n\n-Peff\n"},{"id":"428810","messageId":"874kdfg32w.fsf@evledraar.gmail.com","threadId":"55999","inReplyTo":"YNwd6wmt4FTyySgH@coredump.intra.peff.net","subject":"Re: Antw: [EXT] Re: git add --interactive patch improvement for split hunks","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-30T08:27:16Z","receivedAt":"2021-06-30T08:38:04Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 30 2021, Jeff King wrote:\n\n> On Tue, Jun 29, 2021 at 11:09:19PM -0700, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > It looks like Junio carried it in \"What's Cooking\" for almost a\n>> > year, marked as \"waiting for re-roll\" to handle the squash, but then\n>> > eventually discarded it as stale. :(\n>> \n>> Heh, thanks for digging.\n>> \n>> Is the moral of the story that we should merge down unfinished\n>> topics more aggressively (hoping that the untied loose ends would be\n>> tied after they hit released version), we should prod owners of\n>> stalled topics with sharper stick more often, or something else?\n>\n> I'm not sure. I think the topic would have graduated if either you had\n> just applied the squash and merged it down, or if the original author\n> had checked back in over the intervening year to say \"hey, what happened\n> to my patch\" (either by reading \"what's cooking\" or manually).\n>\n> I suspect drive-by contributors might not realize they need to do the\n> latter in some cases, but I wouldn't have counted 2014-era Ævar in that\n> boat. So I dunno.\n\nOr maybe the moral of the story that it's a net addition of complexity\nto git-add--interactive.perl. If I didn't care enough to remember or\nnotice the issue again maybe it wasn't all that important to begin with.\n\nLikewise when it got ejected nobody else seemed to notice/care enough to\nsay \"hey I liked that feature\" & to pick it up.\n\nI'd entirely forgotten I wrote that. Now that I'm reminded of it I don't\ncare enough myself to rebase it, test it again, and especially not to\nfigure out if/how it's going to interact with the new C implementation /\nadd and adjust a test for the two.\n\nBut maybe someone else will, it would be neat if someone has more of an\nitch from the lack of that feature & wants to pick it up.\n"},{"id":"428834","messageId":"YNykmFejFG8cEjue@coredump.intra.peff.net","threadId":"55999","inReplyTo":"874kdfg32w.fsf@evledraar.gmail.com","subject":"Re: Antw: [EXT] Re: git add --interactive patch improvement for split hunks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-30T17:06:32Z","receivedAt":"2021-06-30T17:06:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 30, 2021 at 10:27:16AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > I'm not sure. I think the topic would have graduated if either you had\n> > just applied the squash and merged it down, or if the original author\n> > had checked back in over the intervening year to say \"hey, what happened\n> > to my patch\" (either by reading \"what's cooking\" or manually).\n> >\n> > I suspect drive-by contributors might not realize they need to do the\n> > latter in some cases, but I wouldn't have counted 2014-era Ævar in that\n> > boat. So I dunno.\n> \n> Or maybe the moral of the story that it's a net addition of complexity\n> to git-add--interactive.perl. If I didn't care enough to remember or\n> notice the issue again maybe it wasn't all that important to begin with.\n> \n> Likewise when it got ejected nobody else seemed to notice/care enough to\n> say \"hey I liked that feature\" & to pick it up.\n\nYeah, that's probably a fair interpretation, too. :)\n\n> I'd entirely forgotten I wrote that. Now that I'm reminded of it I don't\n> care enough myself to rebase it, test it again, and especially not to\n> figure out if/how it's going to interact with the new C implementation /\n> add and adjust a test for the two.\n> \n> But maybe someone else will, it would be neat if someone has more of an\n> itch from the lack of that feature & wants to pick it up.\n\nI can probably save you a little time/mental energy here: the C version\nalready does what your patch was trying to do. Once we switch to it as\nthe default, your patch would be obsolete anyway. :)\n\n-Peff\n"}]}