-
Notifications
You must be signed in to change notification settings - Fork 281
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
Proof of concept: structured diff of conflicted commits #4251
Draft
ilyagr
wants to merge
13
commits into
martinvonz:main
Choose a base branch
from
ilyagr:diffofmerge
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+1,186
−294
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ilyagr
changed the title
Diffofmerge
Proof of concept: structured diff of conflicted commits
Aug 11, 2024
ilyagr
force-pushed
the
diffofmerge
branch
9 times, most recently
from
August 13, 2024 05:50
6f1379a
to
ef24817
Compare
1 task
ilyagr
force-pushed
the
diffofmerge
branch
4 times, most recently
from
August 16, 2024 03:05
50a483c
to
7fc0f29
Compare
4 tasks
ilyagr
force-pushed
the
diffofmerge
branch
3 times, most recently
from
August 23, 2024 01:38
58c0a90
to
78ba3fc
Compare
ilyagr
force-pushed
the
diffofmerge
branch
3 times, most recently
from
September 5, 2024 22:19
629d1b0
to
d1c35e0
Compare
These will be used for diffs of Merge-s. This also refactors one of the less important tests. I did it mostly automatically with `ast-grep` (which I don't necessarily recommend), but we could also just delete this test I think.
This reorders the result slightly and makes it easier to generalize this to work on diffs of merges (where there is the same number of adds and removes).
This represents a diff of two merges, up to reordering and cancelling out of terms. It can also be thought of as a stack of `n` diffs. This type is not very suitable for presenting to the user (since it loses information they might want to see), but is very natural from the perspective of conflict theory.
…tion For textual conflicts, the distance function will be the size of the diff between two chunks of text.
…ture This turns two Merge-s into an object that represents a way to present their difference to the user, in the form of `Vec<DiffExplanationAtom>`. `DiffExplanationAtom` is the key structure for the data model of differences between diffs. The algorithm is inefficient and far from perfect, for now my focus is on the data model.
The goal here is to create a single operation to split a conflicted file text into a sequence of (potentially conflicted) hunks, both for regular conflicts and conflict diffs. The new algorithm is not efficient and I'm guessing an algorithm that gives better results (more logical hunks) is possilbe. We may want to instead keep the conflicts and conflict diff code path separate until the algorithm improves, let me know.
This works with all the places diffs are shown, e.g. `jj diff`, `jj log -p`, `jj interdiff`, etc.
I initially tried just `+` and `-`, but that seemed confusing. The color helps, but I think this is ~understandable either way
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a proof of concept for #4062. None of the design, the code, or functionality are in a finished state, but I'd like to polish it up to the point where it can be merged for later improvement.
My main goal here is to set up the data model for diffs between conflicts, and to have an example of a UI for showing these diffs to the user. I am not sure whether the latter will be of actual use to users who don't understand conflict theory very well, but the goal is 1) do our best 2) have a debugging tool for exports and 3) most importantly, have a way to present these data structures in tests as we improve them to the point where they can be used in
blame
,absorb
, etc.The algorithms that actually do all the jobs are inefficient and I already have examples of cases where they do a bad job. Hopefully, they can be improved as a separate project.
Organization
The PR splits into three parts:
Part 1. Everything up to the
lib merge: the key explain_diff_of_merges function and data structure
commit exists to set up that key commit.That commit sets up the
DiffOfMergeExplanationAtom<T>
type, presented below. The key function has the signaturewhere the distance function
T⨯T→usize
is used to decide which pieces of the conflicts should be paired together into diffs. The distance function we'll use is the size of the diff between two chunks of text.Part 2. There is a questionable (and less important for now) part that deals with splitting a
Merge<Vec<u8>>
into hunks.Part 3. The rest sets up
jj diff --conflicts
, which is a way the UI to present an object of typeVec<DiffOfMergeExplanationAtom<T>>
to the user. The key commit is cli diff-util: implement diff --structured-conflicts, the commits after that try to add a bit of polish.Unless people object to the general direction of this, I'll start by trying to merge part 1.
Data model: `DiffOfMergeExplanationAtom`
Examples
The exact UI will very likely change in the future. For now, it's designed so that it doesn't absolutely require color and so that I can understand this.
Here is a simple example of conflict resolution from #4246 (reply in thread) (the discussion question helpfully has reproduction commands):
This is equivalent to:
or, equivalently,
This is how
jj
actually printed it. There's a discussion at #4246 (reply in thread) about whether it would make sense to adjust this PR's presentation to make it more similar.Another example of conflict resolution
I am not sure why the "removed diff" does not have + and - lines more interleaved; I think that would be better.
In textual form:
Presumably, the user would compare whether the two diffs in the example above mean the same thing.
Example of conflict creation
This gives a good overview of what conflict creations look like, but it also shows some problems with this PR's technique. It's hard to see the actual conflict in there, which looks as follows.
It's unclear to me at the moment whether and how this could be addressed in general. We could just print the created conflict, but could this be generalized to all situations?
TODOs
Missing features outside the scope of this PR
Off the top of my head:
blame
orabsorb
, so probably the next priority after this PR)diff3
formatcolor-words
formatChecklist
If applicable:
CHANGELOG.md