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: escape conflict markers by making them longer #4969

Merged
merged 3 commits into from
Dec 21, 2024
Merged

conflicts: escape conflict markers by making them longer #4969

merged 3 commits into from
Dec 21, 2024

Conversation

scott2000
Copy link
Contributor

@scott2000 scott2000 commented Nov 25, 2024 •
edited
Loading

Related to #3975 and #4088.

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

@scott2000 scott2000 mentioned this pull request Nov 25, 2024
Closed
4 tasks
@scott2000 scott2000 force-pushed the escape-conflict-markers branch 4 times, most recently from c547024 to d62d14c Compare November 25, 2024 17:29
@joyously
Copy link

Perhaps this should be tested by correctly parsing its own source code.

@scott2000
Copy link
Contributor Author

Perhaps this should be tested by correctly parsing its own source code.

Surprisingly, the source code for conflicts.rs doesn't actually contain any lines that look like conflict markers! The only two files that have lines which can be confused for conflict markers currently are tutorial.md and conflicts.md it looks like.

@scott2000 scott2000 force-pushed the escape-conflict-markers branch from d62d14c to 6446b3d Compare November 26, 2024 15:35
@scott2000 scott2000 marked this pull request as ready for review November 26, 2024 15:46
@scott2000 scott2000 force-pushed the escape-conflict-markers branch from 6446b3d to 9f0be3b Compare November 26, 2024 20:29
yuja
yuja reviewed Nov 27, 2024
View reviewed changes
lib/src/conflicts.rs Outdated Show resolved Hide resolved
lib/src/conflicts.rs Outdated Show resolved Hide resolved
lib/src/conflicts.rs Outdated Show resolved Hide resolved
lib/src/conflicts.rs Outdated Show resolved Hide resolved
lib/src/conflicts.rs Outdated Show resolved Hide resolved
lib/src/conflicts.rs Outdated Show resolved Hide resolved
lib/src/conflicts.rs Outdated Show resolved Hide resolved
lib/src/conflicts.rs Outdated Show resolved Hide resolved
lib/src/conflicts.rs Show resolved Hide resolved
@scott2000 scott2000 force-pushed the escape-conflict-markers branch from 9f0be3b to cb50ba3 Compare December 3, 2024 01:25
martinvonz
martinvonz reviewed Dec 3, 2024
View reviewed changes
Copy link
Member

@martinvonz martinvonz left a comment

Choose a reason for hiding this comment

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

Sorry, I thought I had already sent these comments last week

lib/src/conflicts.rs Outdated Show resolved Hide resolved
lib/src/conflicts.rs Outdated Show resolved Hide resolved
@ilyagr ilyagr mentioned this pull request Dec 3, 2024
Merged
4 tasks
@scott2000 scott2000 force-pushed the escape-conflict-markers branch 5 times, most recently from 8b28066 to 5299c8f Compare December 7, 2024 15:41
yuja
yuja reviewed Dec 8, 2024
View reviewed changes
lib/src/conflicts.rs Outdated Show resolved Hide resolved
lib/tests/test_conflicts.rs Outdated Show resolved Hide resolved
lib/tests/test_conflicts.rs Show resolved Hide resolved
cli/src/diff_util.rs Outdated Show resolved Hide resolved
lib/src/conflicts.rs Outdated Show resolved Hide resolved
@scott2000 scott2000 force-pushed the escape-conflict-markers branch from 5299c8f to 82cad26 Compare December 8, 2024 16:22
martinvonz
martinvonz reviewed Dec 8, 2024
View reviewed changes
lib/tests/test_conflicts.rs Outdated Show resolved Hide resolved
lib/src/conflicts.rs Outdated Show resolved Hide resolved
lib/tests/test_conflicts.rs Show resolved Hide resolved
@scott2000 scott2000 force-pushed the escape-conflict-markers branch 4 times, most recently from cf0c834 to b99285a Compare December 17, 2024 01:05
@scott2000 scott2000 force-pushed the escape-conflict-markers branch 2 times, most recently from f2d6c28 to 92faf01 Compare December 19, 2024 20:37
yuja
yuja approved these changes Dec 20, 2024
View reviewed changes
Copy link
Contributor

@yuja yuja left a comment

Choose a reason for hiding this comment

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

The code generally looks good to me, thanks.

lib/src/local_working_copy.rs Outdated Show resolved Hide resolved
lib/src/local_working_copy.rs Show resolved Hide resolved
lib/src/local_working_copy.rs Outdated Show resolved Hide resolved
lib/src/local_working_copy.rs Show resolved Hide resolved
@scott2000 scott2000 force-pushed the escape-conflict-markers branch 2 times, most recently from d796a4c to d073127 Compare December 20, 2024 20:53
yuja
yuja reviewed Dec 21, 2024
View reviewed changes
lib/src/local_working_copy.rs Outdated Show resolved Hide resolved
lib/src/local_working_copy.rs Show resolved Hide resolved
@scott2000
conflicts: refactor conflict marker writing and parsing
3ad41cf 
These changes make the code a bit more readable, and they will make it
easier to have conflict markers of different lengths in the next commit.
@scott2000
conflicts: escape conflict markers by making them longer
fc11c5b 
If a file contains lines which look like conflict markers, then we need
to make the real conflict markers longer so that the materialized
conflicts can be parsed unambiguously.

When parsing the conflict, we require that the conflict markers are at
least as long as the materialized conflict markers based on the current
tree. This can lead to some unintuitive edge cases which will be solved
in the next commit.

For instance, if we have a file explaining the differences between
Jujutsu's conflict markers and Git's conflict markers, it could produce
a conflict with long markers like this:

```
<<<<<<<<<<< Conflict 1 of 1
%%%%%%%%%%% Changes from base to side jj-vcs#1
 Jujutsu uses different conflict markers than Git, which just shows the
-sides of a conflict without a diff.
+sides of a conflict without a diff:
+
+<<<<<<<
+left
+|||||||
+base
+=======
+right
+>>>>>>>
+++++++++++ Contents of side jj-vcs#2
Jujutsu uses different conflict markers than Git:

<<<<<<<
%%%%%%%
-base
+left
+++++++
right
>>>>>>>
>>>>>>>>>>> Conflict 1 of 1 ends
```

We should support these options for "git" conflict marker style as well,
since Git actually does support producing longer conflict markers in
some cases through .gitattributes:

https://git-scm.com/docs/gitattributes#_conflict_marker_size

We may also want to support passing the conflict marker length to merge
tools as well in the future, since Git supports a "%L" parameter to pass
the conflict marker length to merge drivers:

https://git-scm.com/docs/gitattributes#_defining_a_custom_merge_driver
@scott2000
local_working_copy: store materialized conflict marker length
252ffad 
Storing the conflict marker length in the working copy makes conflict
parsing more consistent, and it allows us to parse valid conflict hunks
even if the user left some invalid conflict markers in the file while
resolving the conflicts.
@scott2000 scott2000 force-pushed the escape-conflict-markers branch from d073127 to 252ffad Compare December 21, 2024 16:33
@scott2000 scott2000 merged commit 6baa436 into jj-vcs:main Dec 21, 2024
18 checks passed
@scott2000 scott2000 deleted the escape-conflict-markers branch December 21, 2024 17:36
@scott2000 scott2000 mentioned this pull request Dec 22, 2024
Merged
4 tasks
@jonathantanmy jonathantanmy mentioned this pull request Jan 2, 2025
Closed
@jonathantanmy
Copy link
Contributor

I think that we should make conflict_marker_len in conflicts::update_from_content optional. I've started a discussion at #5243 , including my reasons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ilyagr ilyagr ilyagr left review comments

@martinvonz martinvonz martinvonz left review comments

@yuja yuja yuja 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.
6 participants
@scott2000 @joyously @jonathantanmy @yuja @martinvonz @ilyagr