conflicts: add "ui.conflict-marker-style" config #4897

scott2000 · 2024-11-17T15:39:13Z

Resolves #823.

Adds support for "diff" (current default), "snapshot", and "git-diff3" (falls back to "diff" if more than 2 sides) options.

Example of "diff"

<<<<<<< Conflict 1 of 1
+++++++ Contents of side #1
fn example(word: String) {
    println!("word is {word}");
%%%%%%% Changes from base to side #2
-fn example(w: String) {
+fn example(w: &str) {
     println!("word is {w}");
>>>>>>> Conflict 1 of 1 ends
}

Example of "snapshot"

<<<<<<< Conflict 1 of 1
+++++++ Contents of side #1
fn example(word: String) {
    println!("word is {word}");
------- Contents of base
fn example(w: String) {
    println!("word is {w}");
+++++++ Contents of side #2
fn example(w: &str) {
    println!("word is {w}");
>>>>>>> Conflict 1 of 1 ends
}

Example of "git"

<<<<<<< Side #1 (Conflict 1 of 1)
fn example(word: String) {
    println!("word is {word}");
||||||| Base
fn example(w: String) {
    println!("word is {w}");
=======
fn example(w: &str) {
    println!("word is {w}");
>>>>>>> Side #2 (Conflict 1 of 1 ends)
}

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

tim-janik · 2024-11-17T17:14:55Z

Example of "snapshot"

+1 for supporting the "snapshot" markers.

martinvonz

I haven't reviewd the last two commits, but I've read the commit message for them and it sounds like you didn't miss anything (in particular, it would have been easy to miss that we want support for parsing either style regardless of configured style).

lib/src/conflicts.rs

CHANGELOG.md

lib/src/conflicts.rs

CHANGELOG.md

martinvonz · 2024-11-17T22:42:35Z

Let me know when this is ready for review. (I don't think I get a notification when a PR is marked ready.)

cli/src/diff_util.rs

scott2000 · 2024-11-20T01:36:26Z

Ok, I think it's ready for review now! Let me know if I should split some of these commits into separate PRs.

ilyagr

This is a clever approach, it's simpler than what I was thinking (allowing other conflict styles only in a jj file edit command). I was worried for a bit that this would make #3975 worse, but I currently think that it's probably manageable and might be worth the complexity. I wrote down some related thoughts in case they spark anybody else's thinking.

cli/src/config-schema.json

lib/src/local_working_copy.rs

lib/src/conflicts.rs

martinvonz

Thanks a lot for this PR! I know a lot of people want this feature.

CHANGELOG.md

lib/tests/test_conflicts.rs

lib/src/conflicts.rs

cli/src/config/merge_tools.toml

docs/conflicts.md

lib/src/annotate.rs

lib/src/default_index/revset_engine.rs

lib/src/settings.rs

lib/src/local_working_copy.rs

yuja

I would split the merge-tools patches to separate PR because this PR is getting bigger.

lib/src/conflicts.rs

cli/tests/test_config_command.rs

lib/src/conflicts.rs

Adds a new "ui.conflict-marker-style" config option. The "diff" option is the default jj-style conflict markers with a snapshot and a series of diffs to apply to the snapshot. New conflict marker style options will be added in later commits. The majority of the changes in this commit are from passing the config option down to the code that materializes the conflicts. Example of "diff" conflict markers: ``` <<<<<<< Conflict 1 of 1 +++++++ Contents of side jj-vcs#1 fn example(word: String) { println!("word is {word}"); %%%%%%% Changes from base to side jj-vcs#2 -fn example(w: String) { +fn example(w: &str) { println!("word is {w}"); >>>>>>> Conflict 1 of 1 ends } ```

This makes the code a bit easier to follow in my opinion.

This will make it easier to add separate logic for "snapshot" style conflict markers in the next commit.

Adds a new "snapshot" conflict marker style which returns a series of snapshots, similar to Git's "diff3" conflict style. The "snapshot" option uses a subset of the conflict hunk headers as the "diff" option (it just doesn't use "%%%%%%%"), meaning that the two options are trivially compatible with each other (i.e. a file materialized with "snapshot" can be parsed with "diff" and vice versa). Example of "snapshot" conflict markers: ``` <<<<<<< Conflict 1 of 1 +++++++ Contents of side jj-vcs#1 fn example(word: String) { println!("word is {word}"); ------- Contents of base fn example(w: String) { println!("word is {w}"); +++++++ Contents of side jj-vcs#2 fn example(w: &str) { println!("word is {w}"); >>>>>>> Conflict 1 of 1 ends } ```

This will make it easier to add a second code path for Git-style conflicts in a later commit.

This was already possible using `merge.iter().as_slice()`, but I think this is cleaner.

Adds a new "git" conflict marker style option. This option matches Git's "diff3" conflict style, allowing these conflicts to be parsed by some external tools that don't support JJ-style conflicts. If a conflict has more than 2 sides, then it falls back to the similar "snapshot" conflict marker style. The conflict parsing code now supports parsing Git-style conflict markers in addition to the normal JJ-style conflict markers, regardless of the conflict marker style setting. This has the benefit of allowing the user to switch the conflict marker style while they already have conflicts checked out, and their old conflicts will still be parsed correctly. Example of "git" conflict markers: ``` <<<<<<< Side jj-vcs#1 (Conflict 1 of 1) fn example(word: String) { println!("word is {word}"); ||||||| Base fn example(w: String) { println!("word is {w}"); ======= fn example(w: &str) { println!("word is {w}"); >>>>>>> Side jj-vcs#2 (Conflict 1 of 1 ends) } ```

The current docs for conflict markers start out by introducing Git "diff3" conflict markers, and then discussing how Jujutsu's conflict markers are different from Git's. I don't think this is the best way to explain it, because it requires the reader to first understand Git's conflict markers before they go on to learn Jujutsu's conflict markers. Instead, I think it's better to explain the conflict markers from scratch with a more detailed example so that we don't assume any particular knowledge of Git. This structure also works better with the new config option, since we can talk about the default option first, and then go on to explain alternatives later.

scott2000 force-pushed the conflict-marker-style branch 4 times, most recently from 6bb9f91 to 3588eac Compare November 17, 2024 16:26

scott2000 force-pushed the conflict-marker-style branch from 3588eac to 381aba9 Compare November 17, 2024 17:22

martinvonz reviewed Nov 17, 2024

View reviewed changes

lib/src/conflicts.rs Outdated Show resolved Hide resolved

CHANGELOG.md Show resolved Hide resolved

lib/src/conflicts.rs Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

scott2000 force-pushed the conflict-marker-style branch 2 times, most recently from 6aed3c0 to bfdf68b Compare November 17, 2024 21:38

scott2000 force-pushed the conflict-marker-style branch from bfdf68b to c0181bd Compare November 18, 2024 03:15

yuja reviewed Nov 18, 2024

View reviewed changes

cli/src/diff_util.rs Outdated Show resolved Hide resolved

scott2000 force-pushed the conflict-marker-style branch 4 times, most recently from 4a03751 to c1207f8 Compare November 20, 2024 01:28

scott2000 marked this pull request as ready for review November 20, 2024 01:34

ilyagr reviewed Nov 20, 2024

View reviewed changes

cli/src/config-schema.json Show resolved Hide resolved

cli/src/config-schema.json Show resolved Hide resolved

cli/src/config-schema.json Outdated Show resolved Hide resolved

cli/src/config-schema.json Show resolved Hide resolved

yuja reviewed Nov 20, 2024

View reviewed changes

lib/src/local_working_copy.rs Outdated Show resolved Hide resolved

lib/src/conflicts.rs Show resolved Hide resolved

lib/src/conflicts.rs Outdated Show resolved Hide resolved

martinvonz mentioned this pull request Nov 20, 2024

conflicts: refactor materialize_merge_result() #4928

Merged

4 tasks

scott2000 force-pushed the conflict-marker-style branch 2 times, most recently from da58564 to bb19e68 Compare November 21, 2024 04:00

scott2000 commented Nov 21, 2024

View reviewed changes

lib/src/conflicts.rs Outdated Show resolved Hide resolved

scott2000 force-pushed the conflict-marker-style branch 2 times, most recently from 323b1e0 to 7213011 Compare November 22, 2024 04:36

scott2000 mentioned this pull request Nov 22, 2024

conflicts: whitespace parsing fixes #4944

Merged

4 tasks

martinvonz reviewed Nov 22, 2024

View reviewed changes

yuja reviewed Nov 22, 2024

View reviewed changes

lib/src/annotate.rs Outdated Show resolved Hide resolved

lib/src/default_index/revset_engine.rs Outdated Show resolved Hide resolved

lib/src/settings.rs Outdated Show resolved Hide resolved

lib/src/local_working_copy.rs Outdated Show resolved Hide resolved

yuja reviewed Nov 22, 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 Show resolved Hide resolved

cli/tests/test_config_command.rs Outdated Show resolved Hide resolved

lib/src/conflicts.rs Show resolved Hide resolved

scott2000 force-pushed the conflict-marker-style branch from 7213011 to 6d643ce Compare November 22, 2024 23:20

scott2000 mentioned this pull request Nov 22, 2024

merge_tools: allow setting conflict marker style per-tool #4951

Merged

4 tasks

scott2000 force-pushed the conflict-marker-style branch 3 times, most recently from 45280c7 to ceb25d7 Compare November 23, 2024 00:49

martinvonz approved these changes Nov 23, 2024

View reviewed changes

scott2000 force-pushed the conflict-marker-style branch from ceb25d7 to 362d6f4 Compare November 23, 2024 01:32

yuja approved these changes Nov 23, 2024

View reviewed changes

lib/src/conflicts.rs Show resolved Hide resolved

lib/src/conflicts.rs Show resolved Hide resolved

scott2000 added 8 commits November 23, 2024 08:05

conflicts: replace "if let" with "let else"

2b95863

This makes the code a bit easier to follow in my opinion.

conflicts: extract header output into separate functions

e07c4e4

This will make it easier to add separate logic for "snapshot" style conflict markers in the next commit.

conflicts: extract function for materializing hunk

608ea6e

This will make it easier to add a second code path for Git-style conflicts in a later commit.

merge: add as_slice() method

d3ca98c

This was already possible using `merge.iter().as_slice()`, but I think this is cleaner.

scott2000 force-pushed the conflict-marker-style branch from 362d6f4 to 9f69077 Compare November 23, 2024 14:07

scott2000 merged commit 472fd35 into jj-vcs:main Nov 23, 2024
18 checks passed

scott2000 deleted the conflict-marker-style branch November 23, 2024 14:28

scott2000 mentioned this pull request Nov 26, 2024

conflicts: escape conflict markers by making them longer #4969

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

conflicts: add "ui.conflict-marker-style" config #4897

conflicts: add "ui.conflict-marker-style" config #4897

scott2000 commented Nov 17, 2024 •

edited

Loading

tim-janik commented Nov 17, 2024

martinvonz left a comment

martinvonz commented Nov 17, 2024

scott2000 commented Nov 20, 2024

ilyagr left a comment

martinvonz left a comment

yuja left a comment

conflicts: add "ui.conflict-marker-style" config #4897

conflicts: add "ui.conflict-marker-style" config #4897

Conversation

scott2000 commented Nov 17, 2024 • edited Loading

Checklist

tim-janik commented Nov 17, 2024

martinvonz left a comment

Choose a reason for hiding this comment

martinvonz commented Nov 17, 2024

scott2000 commented Nov 20, 2024

ilyagr left a comment

Choose a reason for hiding this comment

martinvonz left a comment

Choose a reason for hiding this comment

yuja left a comment

Choose a reason for hiding this comment

scott2000 commented Nov 17, 2024 •

edited

Loading