Skip to content
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.

Sign up for GitHub

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

Jump to bottom

conflicts: refactor materialize_merge_result() #4928

Merged
merged 4 commits into from
Nov 21, 2024
Merged

conflicts: refactor materialize_merge_result() #4928

merged 4 commits into from
Nov 21, 2024
+115 −136

Conversation

yuja
Copy link
Contributor

@yuja yuja commented Nov 20, 2024

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

yuja added 4 commits November 20, 2024 22:27
@yuja
conflicts: extract inner block of materialize_merge_result()
bca4a5c 
I'm going to make materialize_merge_result() accept reference type. This patch
extracts large non-generic part to a separate function.
@yuja
conflicts: make materialize_merge_result() accept reference type
4e1e42f 
Because the output of diff.hunks() is a list of [&BStr]s, a Merge object
reconstructed from a diff hunk will be Merge<&BStr>. I'm not pretty sure if
we'll implement conflict diffs in that way, but this change should be harmless
anyway.
@yuja
conflicts: extract materialize_merge_result_to_bytes() helper
8daf301 
We have many callers of materialize_merge_result() who just want in-memory
buffer.
@yuja
annotate: remove unneeded BString<->Vec round-trip
c86535c
martinvonz
martinvonz approved these changes Nov 20, 2024
View reviewed changes
Copy link
Member

@martinvonz martinvonz left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but maybe @scott2000 (author of #4897 ) and @ilyagr (author of #4251) also want a look?

(But I know @scott2000 already basically approved it.)

scott2000
scott2000 approved these changes Nov 20, 2024
View reviewed changes
Copy link
Contributor

@scott2000 scott2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

ilyagr
ilyagr approved these changes Nov 21, 2024
View reviewed changes
Copy link
Contributor

@ilyagr ilyagr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as well, thank you!

@yuja yuja merged commit 7906b3f into jj-vcs:main Nov 21, 2024
18 checks passed
@yuja yuja deleted the push-puzorutuyrzw branch November 21, 2024 01:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scott2000 scott2000 scott2000 approved these changes

@ilyagr ilyagr ilyagr approved these changes

@martinvonz martinvonz martinvonz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yuja @martinvonz @ilyagr @scott2000