-
-
Notifications
You must be signed in to change notification settings - Fork 376
Add HunkHeader
and DiffLineType
to ConsumeHunk::consume_hunk
#2102
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
I’m going to mark this PR as ready for review in order to indicate that I’m now done with the initial version and would welcome a first round of feedback! There’s a few genuine |
98f5370
to
b765afe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some notes from our session:
UnifiedDiffSink
->UnifiedDiff
- make it a breaking change.collect()
- reuse the buffer for lines instead.
b765afe
to
1fe6b19
Compare
1fe6b19
to
e24d8f4
Compare
I’ve now updated the PR.
|
HunkHeader
and DiffLineType
to ConsumeHunk::consume_hunk
Apologies for the late response! In any case, I think this is the way to go. Pushing newline handling further down seems like perfectly reasonable, the new 'abstract' version doesn't need to know about that, after all it has a header, it has each line in a buffer, that's all that it needs. Regarding the lifetime issues with the buffer, I see why would that be the case. There is this hidden lifetime in I set this PR back to draft, but invite you to change it back to what it is now to clearly indicate when it's ready for review. |
This commit modifies the public API of `ConsumeHunk::consume_hunk` to use the new types `DiffLineType` and `HunkHeader`. It also shifts responsibility for adding newlines to the API's consumer.
e24d8f4
to
0924bd3
Compare
The lifetime issue fortunately was much easier to solve than expected. I removed the intermediate Once CI passes, this will be ready for review! |
This PR changes
ConsumeHunk::consume_hunk
to accept typed data instead of strings.This PR was initially motivated by gitui-org/gitui#2685.
Initial description (no longer accurate as of 2025-08-24)
This PR is an attempt at extracting a part of
UnifiedDiff
, more specifically a part that exposes an API for dealing with structured data as opposed to string-based data. It introducesConsumeTypedHunk
which is very similar toConsumeHunk
, except that is usesHunkHeader
andDiffLineType
to encode diff-related information in proper types.There are still a few open issues/questions:
ConsumeTypedHunk
is notNewlineSeparator
-aware. Is that a good thing or a bad thing? I’m leaning more towards the former.TODO:
comments.What do you think about the PR’s overall design? If you think this works, I’d start addressing the remaining issues.