Repository navigation
Add support for Jujutsu conflict markers - #64750
Ryan Fitzgerald (rf-) wants to merge 1 commit into
Conversation
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
There was a problem hiding this comment.
🟢 Approval recommended
The scanner changes are consistent with existing conflict-marker handling and comprehensively covered by compiler baselines.
0 open findings
What changed in this PR
Adds native compiler support for Jujutsu-style conflict markers.
Changes:
- Recognizes
%,\, and+conflict separators. - Extends conflict-trivia scanning and skipping.
- Adds compiler coverage and expected baselines.
| File | Description |
|---|---|
tsc/internal/scanner/scanner.go |
Recognizes and skips Jujutsu conflict sections. |
tsc/testdata/tests/cases/compiler/conflictMarkerJujutsuTrivia1.ts |
Exercises all Jujutsu marker forms. |
tsc/testdata/baselines/reference/compiler/conflictMarkerJujutsuTrivia1.errors.txt |
Verifies TS1185 diagnostics. |
tsc/testdata/baselines/reference/compiler/conflictMarkerJujutsuTrivia1.js |
Verifies conflicted code is omitted from emit. |
tsc/testdata/baselines/reference/compiler/conflictMarkerJujutsuTrivia1.symbols |
Verifies unaffected symbol discovery. |
tsc/testdata/baselines/reference/compiler/conflictMarkerJujutsuTrivia1.types |
Verifies unaffected type discovery. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Is this mandatory or just a nice-to-have? I'm happy to do it if any maintainers feel like it's necessary/helpful in this case, but not sure if it's needed here. |
|
@microsoft-github-policy-service agree |
|
TypeScript Bot (@typescript-bot) perf test this faster |
|
Jake Bailey (@jakebailey) Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
An issue would be a better place to sort out the comments that are currently being discussed in the code review (e.g. what are the actual observed forms of the conflict marker?). |
|
Good points, thanks Jake Bailey (@jakebailey)! Apologies for not covering all of the possible formats, it definitely makes sense to do them all at once. I'll open an issue to ref in the PR. |
|
Coming back to this, one thing I'm confused about is the |
|
I think we can probably just do the simple easy version of this and it be sufficient; this is a best effort recovery after all so I think that's ok. |
32321db to
05caab8
Compare
|
Jake Bailey (@jakebailey) I ended up feeling like it was worth it to just bite the bullet and do it correctly, since parsing one side of the conflict instead of dropping everything makes a big difference to the editor experience. I just pushed an update that should hopefully address everything that's come up 🙏 |
05caab8 to
17b4024
Compare
By default, the [Jujutsu](https://github.com/jj-vcs/jj) VCS represents conflicts using a different style than Git. The overall conflict area is still delimited by `<<<<<<<` and `>>>>>>>` lines, but the intermediate sections are separated by `%`, `\`, `+`, and/or `-` characters instead of `=` and `|`. We have two basic requirements for handling conflict areas: - The conflict markers themselves should be flagged as errors, and the compiler should ignore all other text on each marker line. - Exactly one version of the conflicted code should be processed by the compiler, and any other conflict sections should be ignored. This reduces the odds of spurious errors showing up in surrounding code. With Git conflicts, these goals can be achieved without keeping any state between markers. Each call to `scanConflictMarkerTrivia()` handles one marker line, and the call for each intermediate marker also consumes the content until the next intermediate marker, while leaving the first section after `<<<<<<<` to be scanned normally. Jujutsu conflicts are trickier. The `<<<<<<<` line is followed by another marker line, not the actual content, and there may be multiple "snapshot" sections marked by `+++++++` lines that contain content from different commits. We want to scan one of these sections but omit any others, so we need to keep a flag tracking whether we've allowed one to be scanned yet. Another subtlety here is that if the file contains any lines that look like normal-length conflict markers, Jujutsu will use longer markers to disambiguate. Handling this correctly requires us to track the length of the markers in the current conflicted section. Points of interest: - There's a private `couldStartTrivia()` function that I would have had to update to keep in sync with `skipTrivia()`, but it seems to just be dead code. This commit deletes it. - The old code is picky about whitespace after the initial marker string, requiring a space character after anything but `=`. This is simplified now so that any marker string can be followed by either a space or a newline. This adds a test case for each of the scenarios described in the Jujutsu docs (default style, snapshot style, longer markers, and missing terminating newline) and one for CRLF line endings.
17b4024 to
d83049d
Compare
| conflictState.scanNextSnapshot = false | ||
| return pos, conflictState |
| // everything up to the start of the next marker. | ||
| for pos < length { | ||
| currentChar := text[pos] | ||
| if (currentChar == '=' || currentChar == '%' || currentChar == '\\' || currentChar == '+' || currentChar == '-' || currentChar == '>') && rune(currentChar) != markerChar && isConflictMarkerTrivia(text, pos, conflictState) { |


By default, the Jujutsu VCS represents
conflicts using a different style than Git. The overall conflict area is
still delimited by
<<<<<<<and>>>>>>>lines, but the intermediatesections are separated by
%,\,+, and/or-characters insteadof
=and|.We have two basic requirements for handling conflict areas:
The conflict markers themselves should be flagged as errors, and the
compiler should ignore all other text on each marker line.
Exactly one version of the conflicted code should be processed by the
compiler, and any other conflict sections should be ignored. This
reduces the odds of spurious errors showing up in surrounding code.
With Git conflicts, these goals can be achieved without keeping any
state between markers. Each call to
scanConflictMarkerTrivia()handlesone marker line, and the call for each intermediate marker also consumes
the content until the next intermediate marker, while leaving the first
section after
<<<<<<<to be scanned normally.Jujutsu conflicts are trickier. The
<<<<<<<line is followed byanother marker line, not the actual content, and there may be multiple
"snapshot" sections marked by
+++++++lines that contain content fromdifferent commits. We want to scan one of these sections but omit any
others, so we need to keep a flag tracking whether we've allowed one to
be scanned yet.
Another subtlety here is that if the file contains any lines that look
like normal-length conflict markers, Jujutsu will use longer markers to
disambiguate. Handling this correctly requires us to track the length of
the markers in the current conflicted section.
Points of interest:
There's a private
couldStartTrivia()function that I would have hadto update to keep in sync with
skipTrivia(), but it seems to just bedead code. This commit deletes it.
The old code is picky about whitespace after the initial marker
string, requiring a space character after anything but
=. This issimplified now so that any marker string can be followed by either a
space or a newline.
This adds a test case for each of the scenarios described in the Jujutsu
docs (default style, snapshot style, longer markers, and missing
terminating newline).