{"thread":{"id":"58021","subject":"Feature Request: Custom split location for `git add --patch`","startedAt":"2022-06-16T10:20:16Z","lastAt":"2022-06-21T15:49:37Z","messageCount":3,"participants":["Dennis van Gerwen","Johannes Schindelin","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"457363","messageId":"da79cc66-42a0-2563-d09b-fbc0ad5b28bd@xs4all.nl","threadId":"58021","inReplyTo":null,"subject":"Feature Request: Custom split location for `git add --patch`","fromName":"Dennis van Gerwen","fromEmail":"dvg@xs4all.nl","sentAt":"2022-06-16T10:19:08Z","receivedAt":"2022-06-16T10:20:16Z","isPatch":false,"sender":{"key":"dvg@xs4all.nl","avatar":null},"body":"L.S.\n\nPlease consider this feature request for interactive staging in patch \nmode, using [`git add --patch`][1].\n\n## Problem\n\nIf a hunk cannot be split automatically, the `s` (split) option \ndisappears from the list of options, and we get:\n\n > (1/6) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? s\n > Sorry, cannot split this hunk\n\n\nSee for example these questions on StackOverflow: [2], [3]\n\nI am aware that this problem can be solved by using the [`--edit` \noption][4] to edit the hunk, as explained in the docs under [editing \npatches][5].\n\nHowever, if we have a large contiguous hunk, and we only want to split \nat one specific location, using `--edit` can be quite cumbersome, \nbecause (as far as I know) we need to edit all the lines that we do not \nwant to stage.\n\n## Feature request\n\nWould it be possible to add a \"custom split location\" feature, allowing \nthe user to specify where a contiguous hunk should be split?\n\nSome options:\n\n- Always show the `s` (split hunk) option, and let the user specify a \nline number if an automatic split cannot be made.\n\n- Add a new [patch option][6], in addition to `s` (split hunk), to split \nany hunk at a custom location.\n\n- Add a new operation for editing patches, for example \"s\" (in addition \nto \"+\", \"-\", and \" \"), indicating where to split (e.g. \"split after this \nline\"). This way the user only needs to modify a single line in edit mode.\n\nI'm sure there are a lot of nasty details to consider, but I do think \nsuch a feature would be very convenient.\n\nThanks for your help,\n\nDennis\n\n[1]: https://git-scm.com/docs/git-add#Documentation/git-add.txt---patch\n[2]: https://stackoverflow.com/q/6276752\n[3]: https://stackoverflow.com/q/56892981\n[4]: https://git-scm.com/docs/git-add#Documentation/git-add.txt---edit\n[5]: https://git-scm.com/docs/git-add#_editing_patches\n[6]: https://git-scm.com/docs/git-add#Documentation/git-add.txt-patch\n\n"},{"id":"457501","messageId":"nycvar.QRO.7.76.6.2206181359400.349@tvgsbejvaqbjf.bet","threadId":"58021","inReplyTo":"da79cc66-42a0-2563-d09b-fbc0ad5b28bd@xs4all.nl","subject":"Re: Feature Request: Custom split location for `git add --patch`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-06-18T13:01:06Z","receivedAt":"2022-06-18T13:01:14Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Dennis,\n\nOn Thu, 16 Jun 2022, Dennis van Gerwen wrote:\n\n> ## Problem\n>\n> If a hunk cannot be split automatically, the `s` (split) option disappears\n> from the list of options, and we get:\n>\n> > (1/6) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? s\n> > Sorry, cannot split this hunk\n>\n>\n> See for example these questions on StackOverflow: [2], [3]\n>\n> I am aware that this problem can be solved by using the [`--edit` option][4]\n> to edit the hunk, as explained in the docs under [editing patches][5].\n>\n> However, if we have a large contiguous hunk, and we only want to split at one\n> specific location, using `--edit` can be quite cumbersome, because (as far as\n> I know) we need to edit all the lines that we do not want to stage.\n>\n> ## Feature request\n>\n> Would it be possible to add a \"custom split location\" feature, allowing the\n> user to specify where a contiguous hunk should be split?\n>\n> Some options:\n>\n> - Always show the `s` (split hunk) option, and let the user specify a line\n> number if an automatic split cannot be made.\n>\n> - Add a new [patch option][6], in addition to `s` (split hunk), to split any\n> hunk at a custom location.\n>\n> - Add a new operation for editing patches, for example \"s\" (in addition to\n> \"+\", \"-\", and \" \"), indicating where to split (e.g. \"split after this line\").\n> This way the user only needs to modify a single line in edit mode.\n>\n> I'm sure there are a lot of nasty details to consider, but I do think such a\n> feature would be very convenient.\n\nWhile I am personally not interested in having this option (I find that I\nalways have to edit other aspects when splitting at non-context lines,\nanyway, so I _have_ to use `e`), if you are interested in giving this a\ngo, I will be more than happy to help.\n\nThe most important thing to note is that there are currently _two_\nimplementations of the `git add -p` command, one in Perl and one in C. The\nformer one is slated to be dropped in favor of the latter, so I will\nconcentrate on the implementation in C.\n\nThe function implementing the current split logic is not exactly for the\nfaint of heart, ~150 lines of almost undocumented code:\nhttps://github.com/git/git/blob/v2.37.0-rc0/add-patch.c#L892-L1049\n\nBut then, it does both a lot more _and_ a lot less of what you want: It\nautomatically finds all stretches of context lines and then expands the\nindicated single hunk into as many hunks as it can split it into, sharing\nthe stretches of context lines between adjacent hunks.\n\nObviously, you would need to add a new function that is inspired by this\nfunction, but differs in the following aspects:\n\n- It requires an additional parameter, the line offset at which to split\n  the hunk.\n\n- It always splits the hunk into two hunks, no more, no less.\n\nHowever, from my experience with the Git GUI code implementing line\nstaging, the biggest challenge will be that one of the assumptions of the\ncurrent hunk splitting design will be broken by your feature: so far,\nwhen applying a hunk, no other hunks need to be touched: they remain\nunchanged.\n\nWhen splitting a hunk by context lines, the new hunks will share\noverlapping context lines, but those remain invariant whether hunks are\napplied or not.\n\nWith your desired feature, this would change. Take this hunk, for example:\n\n\t-- snip --\n\t@@\n\t All journeys start with a first step.\n\n\t-[tbd]\n\t+Actually, some journeys, some interesting ones, end in a disaster\n\t+right at the start, because the first step was skipped.\n\n\t And this concludes the chapter. Good night.\n\t-- snap --\n\nLet's just split this for simplicity's sake between the `-` and the `+`\nlines. So the first hunk would become:\n\n\t-- snip --\n\t@@\n\t All journeys start with a first step.\n\n\t-[tbd]\n\n\t And this concludes the chapter. Good night.\n\t-- snap --\n\nand the second:\n\n\t-- snip --\n\t@@\n\t All journeys start with a first step.\n\n\t [tbd]\n\t+Actually, some journeys, some interesting ones, end in a disaster\n\t+right at the start, because the first step was skipped.\n\n\t And this concludes the chapter. Good night.\n\t-- snap --\n\nNote how the `-` line became a context line? It _has_ to be, otherwise the\nsecond hunk could not be applied as-is. That is, until the first hunk has\nbeen applied. In which case the second hunk needs to be modified:\n\n\t-- snip --\n\t@@\n\t All journeys start with a first step.\n\n\t+Actually, some journeys, some interesting ones, end in a disaster\n\t+right at the start, because the first step was skipped.\n\n\t And this concludes the chapter. Good night.\n\t-- snap --\n\nNow the `[tbd]` line in the context has vanished.\n\nThis is really an important concept to keep in mind: if you split at\ncontext lines, your hunks can remain immutable. If you want to split\nelsewhere, after one of the hunks resulting from said split is a applied,\nyou have to recompute the other hunk.\n\nIn Git GUI, that's easy: it always presents the entire diff, and when\nsomething gets (un-)staged, the entire diff is recomputed.\n\nIn `add -p`, we try very hard _not_ to stage anything until the user is\ndone deciding what to stage and what to skip. It then applies the entire\nfile diff in one fell swoop.\n\nSo why does it work with `edit`, then? You can edit a hunk and stage it,\nright? That's because it is still a single hunk. It does not modify any\nline that might be the context line of a different hunk.\n\nWith this in mind, if you are still interested to tackle this feature,\nhere are a couple of notes that should get you started:\n\n- The hunks are recorded as an array of element type `struct hunk`\n  (https://github.com/git/git/blob/v2.37.0-rc0/add-patch.c#L243-L248) in\n  the `struct file_diff`\n  (https://github.com/git/git/blob/v2.37.0-rc0/add-patch.c#L256-L261).\n\n- Each hunk has a `start` and `end` offset. This is the offset into the\n  diff that is stored in plain text in the `plain` strbuf in `struct\n  add_p_state`.\n\n  They have to be offsets instead of pointers because that strbuf can be\n  modified, i.e. it might need to grow so much that its needs to move to a\n  new memory location (which would invalidate all existing pointers).\n\n- Likewise, there are `colored_start` and `colored_end` (byte) offsets\n  that point into the `colored` strbuf of the `struct file_diff`. This is\n  because the colored version of the diff may optionally be generated by\n  another tool than Git itself, and the only requirement is that it\n  generates the same amount of lines and Git assumes a 1:1 correlation\n  between the lines.\n\n- Each hunk also has a hunk header. The `old_offset`, `old_count`,\n  `new_offset` and `new_count` attributes correspond to the diff hunk\n  header, i.e. they refer to line numbers/counts in the pre/post image.\n\n- When splitting a hunk, a new element has to be inserted into the `hunk`\n  array of the `struct file_diff`, and the new hunk's numbers have to be\n  adjusted carefully, as well as the hunk from which the new hunk was\n  split out.\n\n- Since the split hunks require context lines that the corresponding lines\n  did not have in the original diff, you cannot simply adjust the `start`\n  and `end` offsets to point into the existing diff.\n\n  You have to use the same trick as the `edit` feature: append the new\n  hunk with its context lines to `plain` (and to `colored`, unless that's\n  empty), and then point to those offsets.\n\n- You need to take care to record relationships between hunks so that one\n  hunk's content can be adjusted accordingly if another hunk is staged to\n  be applied and therefore changes the former hunk's context lines.\n\n  This is similar to the role of the `delta` field that keeps track how\n  the post image line number in the hunk header needs to be adjusted\n  depending how previous hunks have been edited.\n\nCiao,\nDscho\n"},{"id":"457622","messageId":"xmqq4k0ej9tl.fsf@gitster.g","threadId":"58021","inReplyTo":"nycvar.QRO.7.76.6.2206181359400.349@tvgsbejvaqbjf.bet","subject":"Re: Feature Request: Custom split location for `git add --patch`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-21T15:49:26Z","receivedAt":"2022-06-21T15:49:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Hi Dennis,\n>\n> On Thu, 16 Jun 2022, Dennis van Gerwen wrote:\n>\n> ...\n> Note how the `-` line became a context line? It _has_ to be, otherwise the\n> second hunk could not be applied as-is. That is, until the first hunk has\n> been applied. In which case the second hunk needs to be modified:\n>\n> \t-- snip --\n> \t@@\n> \t All journeys start with a first step.\n>\n> \t+Actually, some journeys, some interesting ones, end in a disaster\n> \t+right at the start, because the first step was skipped.\n>\n> \t And this concludes the chapter. Good night.\n> \t-- snap --\n>\n> Now the `[tbd]` line in the context has vanished.\n>\n> This is really an important concept to keep in mind: if you split at\n> context lines, your hunks can remain immutable. If you want to split\n> elsewhere, after one of the hunks resulting from said split is a applied,\n> you have to recompute the other hunk.\n\nGood write-up.\n"}]}