* Re: Feature Request: Custom split location for `git add --patch`
2022-06-16 10:19 Feature Request: Custom split location for `git add --patch` Dennis van Gerwen
@ 2022-06-18 13:01 ` Johannes Schindelin
2022-06-21 15:49 ` Junio C Hamano
0 siblings, 1 reply; 3+ messages in thread
From: Johannes Schindelin @ 2022-06-18 13:01 UTC (permalink / raw)
To: Dennis van Gerwen; +Cc: git
Hi Dennis,
On Thu, 16 Jun 2022, Dennis van Gerwen wrote:
> ## Problem
>
> If a hunk cannot be split automatically, the `s` (split) option disappears
> from the list of options, and we get:
>
> > (1/6) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? s
> > Sorry, cannot split this hunk
>
>
> See for example these questions on StackOverflow: [2], [3]
>
> I am aware that this problem can be solved by using the [`--edit` option][4]
> to edit the hunk, as explained in the docs under [editing patches][5].
>
> However, if we have a large contiguous hunk, and we only want to split at one
> specific location, using `--edit` can be quite cumbersome, because (as far as
> I know) we need to edit all the lines that we do not want to stage.
>
> ## Feature request
>
> Would it be possible to add a "custom split location" feature, allowing the
> user to specify where a contiguous hunk should be split?
>
> Some options:
>
> - Always show the `s` (split hunk) option, and let the user specify a line
> number if an automatic split cannot be made.
>
> - Add a new [patch option][6], in addition to `s` (split hunk), to split any
> hunk at a custom location.
>
> - Add a new operation for editing patches, for example "s" (in addition to
> "+", "-", and " "), indicating where to split (e.g. "split after this line").
> This way the user only needs to modify a single line in edit mode.
>
> I'm sure there are a lot of nasty details to consider, but I do think such a
> feature would be very convenient.
While I am personally not interested in having this option (I find that I
always have to edit other aspects when splitting at non-context lines,
anyway, so I _have_ to use `e`), if you are interested in giving this a
go, I will be more than happy to help.
The most important thing to note is that there are currently _two_
implementations of the `git add -p` command, one in Perl and one in C. The
former one is slated to be dropped in favor of the latter, so I will
concentrate on the implementation in C.
The function implementing the current split logic is not exactly for the
faint of heart, ~150 lines of almost undocumented code:
https://github.com/git/git/blob/v2.37.0-rc0/add-patch.c#L892-L1049
But then, it does both a lot more _and_ a lot less of what you want: It
automatically finds all stretches of context lines and then expands the
indicated single hunk into as many hunks as it can split it into, sharing
the stretches of context lines between adjacent hunks.
Obviously, you would need to add a new function that is inspired by this
function, but differs in the following aspects:
- It requires an additional parameter, the line offset at which to split
the hunk.
- It always splits the hunk into two hunks, no more, no less.
However, from my experience with the Git GUI code implementing line
staging, the biggest challenge will be that one of the assumptions of the
current hunk splitting design will be broken by your feature: so far,
when applying a hunk, no other hunks need to be touched: they remain
unchanged.
When splitting a hunk by context lines, the new hunks will share
overlapping context lines, but those remain invariant whether hunks are
applied or not.
With your desired feature, this would change. Take this hunk, for example:
-- snip --
@@
All journeys start with a first step.
-[tbd]
+Actually, some journeys, some interesting ones, end in a disaster
+right at the start, because the first step was skipped.
And this concludes the chapter. Good night.
-- snap --
Let's just split this for simplicity's sake between the `-` and the `+`
lines. So the first hunk would become:
-- snip --
@@
All journeys start with a first step.
-[tbd]
And this concludes the chapter. Good night.
-- snap --
and the second:
-- snip --
@@
All journeys start with a first step.
[tbd]
+Actually, some journeys, some interesting ones, end in a disaster
+right at the start, because the first step was skipped.
And this concludes the chapter. Good night.
-- snap --
Note how the `-` line became a context line? It _has_ to be, otherwise the
second hunk could not be applied as-is. That is, until the first hunk has
been applied. In which case the second hunk needs to be modified:
-- snip --
@@
All journeys start with a first step.
+Actually, some journeys, some interesting ones, end in a disaster
+right at the start, because the first step was skipped.
And this concludes the chapter. Good night.
-- snap --
Now the `[tbd]` line in the context has vanished.
This is really an important concept to keep in mind: if you split at
context lines, your hunks can remain immutable. If you want to split
elsewhere, after one of the hunks resulting from said split is a applied,
you have to recompute the other hunk.
In Git GUI, that's easy: it always presents the entire diff, and when
something gets (un-)staged, the entire diff is recomputed.
In `add -p`, we try very hard _not_ to stage anything until the user is
done deciding what to stage and what to skip. It then applies the entire
file diff in one fell swoop.
So why does it work with `edit`, then? You can edit a hunk and stage it,
right? That's because it is still a single hunk. It does not modify any
line that might be the context line of a different hunk.
With this in mind, if you are still interested to tackle this feature,
here are a couple of notes that should get you started:
- The hunks are recorded as an array of element type `struct hunk`
(https://github.com/git/git/blob/v2.37.0-rc0/add-patch.c#L243-L248) in
the `struct file_diff`
(https://github.com/git/git/blob/v2.37.0-rc0/add-patch.c#L256-L261).
- Each hunk has a `start` and `end` offset. This is the offset into the
diff that is stored in plain text in the `plain` strbuf in `struct
add_p_state`.
They have to be offsets instead of pointers because that strbuf can be
modified, i.e. it might need to grow so much that its needs to move to a
new memory location (which would invalidate all existing pointers).
- Likewise, there are `colored_start` and `colored_end` (byte) offsets
that point into the `colored` strbuf of the `struct file_diff`. This is
because the colored version of the diff may optionally be generated by
another tool than Git itself, and the only requirement is that it
generates the same amount of lines and Git assumes a 1:1 correlation
between the lines.
- Each hunk also has a hunk header. The `old_offset`, `old_count`,
`new_offset` and `new_count` attributes correspond to the diff hunk
header, i.e. they refer to line numbers/counts in the pre/post image.
- When splitting a hunk, a new element has to be inserted into the `hunk`
array of the `struct file_diff`, and the new hunk's numbers have to be
adjusted carefully, as well as the hunk from which the new hunk was
split out.
- Since the split hunks require context lines that the corresponding lines
did not have in the original diff, you cannot simply adjust the `start`
and `end` offsets to point into the existing diff.
You have to use the same trick as the `edit` feature: append the new
hunk with its context lines to `plain` (and to `colored`, unless that's
empty), and then point to those offsets.
- You need to take care to record relationships between hunks so that one
hunk's content can be adjusted accordingly if another hunk is staged to
be applied and therefore changes the former hunk's context lines.
This is similar to the role of the `delta` field that keeps track how
the post image line number in the hunk header needs to be adjusted
depending how previous hunks have been edited.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 3+ messages in thread