Repository navigation
Add support for Jujutsu conflict markers - #64750
Ryan Fitzgerald (rf-) wants to merge 1 commit into
Conversation
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 `+` characters instead of `=` and `|`. Since the TypeScript parser only looks for Git-style markers, TypeScript files with Jujutsu-style markers report many spurious errors inside the conflicted areas instead of the nice `TS1185` errors you get with Git. This commit extends the existing logic for `=` and `|` to also apply to `%`, `\`, and `+`; this ended up being pretty painless as far as I can tell.
typescript-automation
Bot
added
the
For Uncommitted Bug
|
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: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| s.token = s.scanTemplateAndSetTokenValue(false /*shouldEmitInvalidEscapeError*/) | ||
| case '%': | ||
| if s.charAt(1) == '%' && isConflictMarkerTrivia(s.text, s.pos) { | ||
| s.pos = scanConflictMarkerTrivia(s.text, s.pos, s.errorAt) |
There was a problem hiding this comment.
I think this still misses Jujutsu’s longer markers, e.g. %%%%%%%%%%%%%%% diff from: base, because the existing check expects a space after the seventh character.
https://docs.jj-vcs.dev/latest/conflicts/#long-conflict-markers
There was a problem hiding this comment.
Actually, I think just accepting “seven or more” characters would still be problematic: jj lengthens markers so shorter marker-like lines can remain ordinary content. For example, =========== can appear inside a conflict delimited by fifteen-character markers, so we’d need to match the delimiter width.
There was a problem hiding this comment.
Actually, I think just accepting “seven or more” characters would still be problematic: jj lengthens markers so shorter marker-like lines can remain ordinary content. For example, =========== can appear inside a conflict delimited by fifteen-character markers, so we’d need to match the delimiter width.
Hmm, I wonder if this is really necessary. TS doesn't try to account for ambiguous/misleading markers in its current Git support, right?
I guess it's possible to construct a situation where a >>>>>>> inside a string or whatever tricks the parser into thinking it's a conflict marker, but it seems really unlikely to happen by accident, and the consequences wouldn't be that bad. I'm not sure if it's worth adding extra state in the scanner just to avoid it.
There was a problem hiding this comment.
Maybe it's not worth it if it rarely happens; I was just reading up the docs since I wasn't sure what stuff jj did extra over git.
| for pos < length { | ||
| currentChar := text[pos] | ||
| if (currentChar == '=' || currentChar == '>') && rune(currentChar) != ch && isConflictMarkerTrivia(text, pos) { | ||
| if (currentChar == '=' || currentChar == '%' || currentChar == '\\' || currentChar == '+' || currentChar == '>') && rune(currentChar) != ch && isConflictMarkerTrivia(text, pos) { |
There was a problem hiding this comment.
I believe snapshot style is also missing some delimiter diagnostics. In this example, ------- and the second +++++++ get swallowed by the first snapshot section:
<<<<<<< conflict 1 of 1
+++++++ ours
ours();
------- base
base();
+++++++ theirs
theirs();
>>>>>>> conflict 1 of 1 ends
https://docs.jj-vcs.dev/latest/conflicts/#alternative-conflict-marker-styles
There was a problem hiding this comment.
I believe this also affects ui.conflict-marker-style = "git", since jj falls back to snapshot style for conflicts with more than two sides?
There was a problem hiding this comment.
Yep, this is easy to fix by adding -, thanks for flagging.
| + a(); | ||
| +++++++ uxvuvmmx 392ce8b5 "call b()" | ||
| b(); | ||
| >>>>>>> conflict 1 of 1 ends |
There was a problem hiding this comment.
I think the missing-newline example would be useful coverage too: it puts +++++++ before %%%%%%% and ends the >>>>>>> line at EOF without a newline.
https://docs.jj-vcs.dev/latest/conflicts/#conflicts-with-missing-terminating-newline
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. |
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 intermediate sections are separated by%,\, and+characters instead of=and|.Since the TypeScript parser only looks for Git-style markers, TypeScript files with Jujutsu-style markers report many spurious errors inside the conflicted areas instead of the nice
TS1185errors you get with Git.This commit extends the existing logic for
=and|to also apply to%,\, and+; this ended up being pretty painless as far as I can tell.