Skip to content

Commit

Permalink
local_working_copy: store materialized conflict marker length
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
scott2000 committed Dec 21, 2024
1 parent b11ce6b commit 6baa436
Show file tree
Hide file tree
Showing 8 changed files with 409 additions and 38 deletions.
3 changes: 2 additions & 1 deletion cli/src/commands/debug/local_working_copy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,11 @@ pub fn cmd_debug_local_working_copy(
for (file, state) in wc.file_states()? {
writeln!(
ui.stdout(),
"{:?} {:13?} {:10?} {:?}",
"{:?} {:13?} {:10?} {:?} {:?}",
state.file_type,
state.size,
state.mtime.0,
state.materialized_conflict_data,
file
)?;
}
Expand Down
20 changes: 18 additions & 2 deletions cli/src/merge_tools/external.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@ use jj_lib::backend::FileId;
use jj_lib::backend::MergedTreeId;
use jj_lib::backend::TreeValue;
use jj_lib::conflicts;
use jj_lib::conflicts::materialize_merge_result_to_bytes;
use jj_lib::conflicts::choose_materialized_conflict_marker_len;
use jj_lib::conflicts::materialize_merge_result_to_bytes_with_marker_len;
use jj_lib::conflicts::ConflictMarkerStyle;
use jj_lib::conflicts::MIN_CONFLICT_MARKER_LEN;
use jj_lib::gitignore::GitIgnoreFile;
use jj_lib::matchers::Matcher;
use jj_lib::merge::Merge;
Expand Down Expand Up @@ -181,8 +183,21 @@ pub fn run_mergetool_external(
.conflict_marker_style
.unwrap_or(default_conflict_marker_style);

// If the merge tool doesn't get conflict markers pre-populated in the output
// file, we should default to accepting MIN_CONFLICT_MARKER_LEN since the
// merge tool is unlikely to know about our rules for conflict marker length.
// In the future, we may want to add a "$markerLength" variable.
let conflict_marker_len = if editor.merge_tool_edits_conflict_markers {
choose_materialized_conflict_marker_len(&content)
} else {
MIN_CONFLICT_MARKER_LEN
};
let initial_output_content = if editor.merge_tool_edits_conflict_markers {
materialize_merge_result_to_bytes(&content, conflict_marker_style)
materialize_merge_result_to_bytes_with_marker_len(
&content,
conflict_marker_style,
conflict_marker_len,
)
} else {
BString::default()
};
Expand Down Expand Up @@ -257,6 +272,7 @@ pub fn run_mergetool_external(
repo_path,
output_file_contents.as_slice(),
conflict_marker_style,
conflict_marker_len,
)
.block_on()?
} else {
Expand Down
180 changes: 180 additions & 0 deletions cli/tests/test_working_copy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
// limitations under the License.

use indoc::indoc;
use regex::Regex;

use crate::common::TestEnvironment;

Expand Down Expand Up @@ -262,3 +263,182 @@ fn test_snapshot_invalid_ignore_pattern() {
2: invalid utf-8 sequence of 1 bytes from index 0
"##);
}

#[test]
fn test_conflict_marker_length_stored_in_working_copy() {
let test_env = TestEnvironment::default();
test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]);
let repo_path = test_env.env_root().join("repo");

// Create a conflict in the working copy with long markers on one side
let conflict_file = repo_path.join("file");
std::fs::write(
&conflict_file,
indoc! {"
line 1
line 2
line 3
"},
)
.unwrap();
test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "base"]);
std::fs::write(
&conflict_file,
indoc! {"
line 1
line 2 - left
line 3 - left
"},
)
.unwrap();
test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "side-a"]);
test_env.jj_cmd_ok(&repo_path, &["new", "description(base)", "-m", "side-b"]);
std::fs::write(
&conflict_file,
indoc! {"
line 1
======= fake marker
line 2 - right
======= fake marker
line 3
"},
)
.unwrap();
test_env.jj_cmd_ok(
&repo_path,
&["new", "description(side-a)", "description(side-b)"],
);

// File should be materialized with long conflict markers
insta::assert_snapshot!(std::fs::read_to_string(&conflict_file).unwrap(), @r##"
line 1
<<<<<<<<<<< Conflict 1 of 1
%%%%%%%%%%% Changes from base to side #1
-line 2
-line 3
+line 2 - left
+line 3 - left
+++++++++++ Contents of side #2
======= fake marker
line 2 - right
======= fake marker
line 3
>>>>>>>>>>> Conflict 1 of 1 ends
"##);

// The timestamps in the `jj debug local-working-copy` output change, so we want
// to remove them before asserting the snapshot
let timestamp_regex = Regex::new(r"\b\d{10,}\b").unwrap();
// On Windows, executable is always `()`, but on Unix-like systems, it's `true`
// or `false`, so we want to remove it from the output as well
let executable_regex = Regex::new("executable: [^ ]+").unwrap();

let redact_output = |output: &str| {
let output = timestamp_regex.replace_all(output, "<timestamp>");
let output = executable_regex.replace_all(&output, "<executable>");
output.into_owned()
};

// Working copy should contain conflict marker length
let stdout = test_env.jj_cmd_success(&repo_path, &["debug", "local-working-copy"]);
insta::assert_snapshot!(redact_output(&stdout), @r#"
Current operation: OperationId("6feb53603f9f7324085d2d89dca19a6dac93fef6795cfd5d57090ff803d404ab1196b45d5b97faa641f6a78302ac0fbd149f5e5a880d1fd64d6520c31beab213")
Current tree: Merge(Conflicted([TreeId("381273b50cf73f8c81b3f1502ee89e9bbd6c1518"), TreeId("771f3d31c4588ea40a8864b2a981749888e596c2"), TreeId("f56b8223da0dab22b03b8323ced4946329aeb4e0")]))
Normal { <executable> } 249 <timestamp> Some(MaterializedConflictData { conflict_marker_len: 11 }) "file"
"#);

// Update the conflict with more fake markers, and it should still parse
// correctly (the markers should be ignored)
std::fs::write(
&conflict_file,
indoc! {"
line 1
<<<<<<<<<<< Conflict 1 of 1
%%%%%%%%%%% Changes from base to side #1
-line 2
-line 3
+line 2 - left
+line 3 - left
+++++++++++ Contents of side #2
<<<<<<< fake marker
||||||| fake marker
line 2 - right
======= fake marker
line 3
>>>>>>> fake marker
>>>>>>>>>>> Conflict 1 of 1 ends
"},
)
.unwrap();

// The file should still be conflicted, and the new content should be saved
let stdout = test_env.jj_cmd_success(&repo_path, &["st"]);
insta::assert_snapshot!(stdout, @r#"
Working copy changes:
M file
There are unresolved conflicts at these paths:
file 2-sided conflict
Working copy : mzvwutvl 3a981880 (conflict) (no description set)
Parent commit: rlvkpnrz ce613b49 side-a
Parent commit: zsuskuln 7b2b03ab side-b
"#);
insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "--git"]), @r##"
diff --git a/file b/file
--- a/file
+++ b/file
@@ -6,8 +6,10 @@
+line 2 - left
+line 3 - left
+++++++++++ Contents of side #2
-======= fake marker
+<<<<<<< fake marker
+||||||| fake marker
line 2 - right
======= fake marker
line 3
+>>>>>>> fake marker
>>>>>>>>>>> Conflict 1 of 1 ends
"##);

// Working copy should still contain conflict marker length
let stdout = test_env.jj_cmd_success(&repo_path, &["debug", "local-working-copy"]);
insta::assert_snapshot!(redact_output(&stdout), @r#"
Current operation: OperationId("205bc702428a522e0b175938a51c51b59741c854a609ba63c89de76ffda6e5eff6fcc00725328b1a91f448401769773cefcff01fac3448190d2cea4e137d2166")
Current tree: Merge(Conflicted([TreeId("381273b50cf73f8c81b3f1502ee89e9bbd6c1518"), TreeId("771f3d31c4588ea40a8864b2a981749888e596c2"), TreeId("3329c18c95f7b7a55c278c2259e9c4ce711fae59")]))
Normal { <executable> } 289 <timestamp> Some(MaterializedConflictData { conflict_marker_len: 11 }) "file"
"#);

// Resolve the conflict
std::fs::write(
&conflict_file,
indoc! {"
line 1
<<<<<<< fake marker
||||||| fake marker
line 2 - left
line 2 - right
======= fake marker
line 3 - left
>>>>>>> fake marker
"},
)
.unwrap();

let stdout = test_env.jj_cmd_success(&repo_path, &["st"]);
insta::assert_snapshot!(stdout, @r#"
Working copy changes:
M file
Working copy : mzvwutvl 1aefd866 (no description set)
Parent commit: rlvkpnrz ce613b49 side-a
Parent commit: zsuskuln 7b2b03ab side-b
"#);

// When the file is resolved, the conflict marker length is removed from the
// working copy
let stdout = test_env.jj_cmd_success(&repo_path, &["debug", "local-working-copy"]);
insta::assert_snapshot!(redact_output(&stdout), @r#"
Current operation: OperationId("2206ce3c108b1573df0841138c226bba1ab3cff900a5899ed31ac69162c7d6f30d37fb5ab43da60dba88047b8ab22d453887fff688f26dfcf04f2c99420a5563")
Current tree: Merge(Resolved(TreeId("6120567b3cb2472d549753ed3e4b84183d52a650")))
Normal { <executable> } 130 <timestamp> None "file"
"#);
}
26 changes: 24 additions & 2 deletions lib/src/conflicts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ pub fn materialize_merge_result<T: AsRef<[u8]>>(
}
}

fn materialize_merge_result_with_marker_len<T: AsRef<[u8]>>(
pub fn materialize_merge_result_with_marker_len<T: AsRef<[u8]>>(
single_hunk: &Merge<T>,
conflict_marker_style: ConflictMarkerStyle,
conflict_marker_len: usize,
Expand Down Expand Up @@ -381,6 +381,28 @@ pub fn materialize_merge_result_to_bytes<T: AsRef<[u8]>>(
}
}

pub fn materialize_merge_result_to_bytes_with_marker_len<T: AsRef<[u8]>>(
single_hunk: &Merge<T>,
conflict_marker_style: ConflictMarkerStyle,
conflict_marker_len: usize,
) -> BString {
let merge_result = files::merge(single_hunk);
match merge_result {
MergeResult::Resolved(content) => content,
MergeResult::Conflict(hunks) => {
let mut output = Vec::new();
materialize_conflict_hunks(
&hunks,
conflict_marker_style,
conflict_marker_len,
&mut output,
)
.expect("writing to an in-memory buffer should never fail");
output.into()
}
}
}

fn materialize_conflict_hunks(
hunks: &[Merge<BString>],
conflict_marker_style: ConflictMarkerStyle,
Expand Down Expand Up @@ -824,6 +846,7 @@ pub async fn update_from_content(
path: &RepoPath,
content: &[u8],
conflict_marker_style: ConflictMarkerStyle,
conflict_marker_len: usize,
) -> BackendResult<Merge<Option<FileId>>> {
let simplified_file_ids = file_ids.clone().simplify();
let simplified_file_ids = &simplified_file_ids;
Expand All @@ -835,7 +858,6 @@ pub async fn update_from_content(
// copy.
let mut old_content = Vec::with_capacity(content.len());
let merge_hunk = extract_as_single_hunk(simplified_file_ids, store, path).await?;
let conflict_marker_len = choose_materialized_conflict_marker_len(&merge_hunk);
materialize_merge_result_with_marker_len(
&merge_hunk,
conflict_marker_style,
Expand Down
Loading

0 comments on commit 6baa436

Please sign in to comment.