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

merge_tools: create builtin merge editor #2118

Merged
merged 1 commit into from
Sep 18, 2023
Merged

merge_tools: create builtin merge editor #2118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cli/src/cli_util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ impl From<DiffGenerateError> for CommandError {

impl From<ConflictResolveError> for CommandError {
fn from(err: ConflictResolveError) -> Self {
user_error(format!("Failed to use external tool to resolve: {err}"))
user_error(format!("Failed to resolve conflicts: {err}"))
}
}

Expand Down
205 changes: 205 additions & 0 deletions cli/src/merge_tools/builtin.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use std::sync::Arc;
use itertools::Itertools;
use jj_lib::backend::{BackendError, FileId, MergedTreeId, ObjectId, TreeValue};
use jj_lib::diff::{find_line_ranges, Diff, DiffHunk};
use jj_lib::files::{self, ContentHunk, MergeResult};
use jj_lib::matchers::EverythingMatcher;
use jj_lib::merge::Merge;
use jj_lib::merged_tree::{MergedTree, MergedTreeBuilder};
Expand Down Expand Up @@ -424,8 +425,123 @@ pub fn edit_diff_builtin(
Ok(tree_id)
}

fn make_merge_sections(
merge_result: MergeResult,
) -> Result<Vec<scm_record::Section<'static>>, BuiltinToolError> {
let mut sections = Vec::new();
match merge_result {
MergeResult::Resolved(ContentHunk(buf)) => {
let contents = buf_to_file_contents(None, buf);
let section = match contents {
FileContents::Absent => None,
FileContents::Text {
contents,
hash: _,
num_bytes: _,
} => Some(scm_record::Section::Unchanged {
lines: contents
.split_inclusive('\n')
.map(|line| Cow::Owned(line.to_owned()))
.collect(),
}),
FileContents::Binary { hash, num_bytes } => Some(scm_record::Section::Binary {
is_checked: false,
old_description: None,
new_description: Some(Cow::Owned(describe_binary(hash.as_deref(), num_bytes))),
}),
};
if let Some(section) = section {
sections.push(section);
}
}
MergeResult::Conflict(hunks) => {
for hunk in hunks {
let section = match hunk.into_resolved() {
Ok(ContentHunk(contents)) => {
let contents = std::str::from_utf8(&contents).map_err(|err| {
BuiltinToolError::DecodeUtf8 {
source: err,
item: "unchanged hunk",
}
})?;
scm_record::Section::Unchanged {
lines: contents
.split_inclusive('\n')
.map(|line| Cow::Owned(line.to_owned()))
.collect(),
}
}
Err(merge) => {
let lines: Vec<scm_record::SectionChangedLine> = merge
.iter()
.zip(
[
scm_record::ChangeType::Added,
scm_record::ChangeType::Removed,
]
.into_iter()
.cycle(),
)
.map(|(contents, change_type)| -> Result<_, BuiltinToolError> {
let ContentHunk(contents) = contents;
let contents = std::str::from_utf8(contents).map_err(|err| {
BuiltinToolError::DecodeUtf8 {
source: err,
item: "conflicting hunk",
}
})?;
let changed_lines =
make_section_changed_lines(contents, change_type);
Ok(changed_lines)
})
.flatten_ok()
.try_collect()?;
scm_record::Section::Changed { lines }
}
};
sections.push(section);
}
}
}
Ok(sections)
}

pub fn edit_merge_builtin(
tree: &MergedTree,
path: &RepoPath,
content: Merge<ContentHunk>,
) -> Result<MergedTreeId, BuiltinToolError> {
let slices = content.map(|ContentHunk(v)| v.as_slice());
let merge_result = files::merge(slices.removes(), slices.adds());
let sections = make_merge_sections(merge_result)?;
let recorder = scm_record::Recorder::new(
scm_record::RecordState {
is_read_only: false,
files: vec![scm_record::File {
old_path: None,
path: Cow::Owned(path.to_fs_path(Path::new(""))),
file_mode: None,
sections,
}],
},
scm_record::EventSource::Crossterm,
);
let state = recorder.run()?;

let file = state.files.into_iter().exactly_one().unwrap();
apply_diff_builtin(
tree.store().clone(),
tree,
tree,
vec![path.clone()],
&[file],
)
.map_err(BuiltinToolError::BackendError)
}

#[cfg(test)]
mod tests {
use jj_lib::conflicts::extract_as_single_hunk;
use jj_lib::repo::Repo;
use testutils::TestRepo;

Expand Down Expand Up @@ -572,4 +688,93 @@ mod tests {
"all-changes tree was different",
);
}

#[test]
fn test_make_merge_sections() {
let test_repo = TestRepo::init(false);
let store = test_repo.repo.store();

let path = RepoPath::from_internal_string("file");
let base_tree = testutils::create_tree(
&test_repo.repo,
&[(&path, "base 1\nbase 2\nbase 3\nbase 4\nbase 5\n")],
);
let left_tree = testutils::create_tree(
&test_repo.repo,
&[(&path, "left 1\nbase 2\nbase 3\nbase 4\nleft 5\n")],
);
let right_tree = testutils::create_tree(
&test_repo.repo,
&[(&path, "right 1\nbase 2\nbase 3\nbase 4\nright 5\n")],
);

fn to_file_id(tree_value: Merge<Option<TreeValue>>) -> Option<FileId> {
match tree_value.into_resolved() {
Ok(Some(TreeValue::File { id, executable: _ })) => Some(id.clone()),
other => {
panic!("merge should have been a FileId: {other:?}")
}
}
}
let merge = Merge::new(
vec![to_file_id(base_tree.path_value(&path))],
vec![
to_file_id(left_tree.path_value(&path)),
to_file_id(right_tree.path_value(&path)),
],
);
let content = extract_as_single_hunk(&merge, store, &path);
let slices = content.map(|ContentHunk(buf)| buf.as_slice());
let merge_result = files::merge(slices.removes(), slices.adds());
let sections = make_merge_sections(merge_result).unwrap();
insta::assert_debug_snapshot!(sections, @r###"
[
Changed {
lines: [
SectionChangedLine {
is_checked: false,
change_type: Added,
line: "left 1\n",
},
SectionChangedLine {
is_checked: false,
change_type: Removed,
line: "base 1\n",
},
SectionChangedLine {
is_checked: false,
change_type: Added,
line: "right 1\n",
},
],
},
Unchanged {
lines: [
"base 2\n",
"base 3\n",
"base 4\n",
],
},
Changed {
lines: [
SectionChangedLine {
is_checked: false,
change_type: Added,
line: "left 5\n",
},
SectionChangedLine {
is_checked: false,
change_type: Removed,
line: "base 5\n",
},
SectionChangedLine {
is_checked: false,
change_type: Added,
line: "right 5\n",
},
],
},
]
"###);
}
}
7 changes: 5 additions & 2 deletions cli/src/merge_tools/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ use jj_lib::settings::{ConfigResultExt as _, UserSettings};
use jj_lib::working_copy::SnapshotError;
use thiserror::Error;

use self::builtin::{edit_diff_builtin, BuiltinToolError};
use self::builtin::{edit_diff_builtin, edit_merge_builtin, BuiltinToolError};
use self::external::{edit_diff_external, DiffCheckoutError, ExternalToolError};
pub use self::external::{generate_diff, ExternalMergeTool};
use crate::config::CommandNameAndArgs;
Expand Down Expand Up @@ -113,7 +113,10 @@ pub fn run_mergetool(

let editor = get_merge_tool_from_settings(ui, settings)?;
match editor {
MergeTool::Builtin => unimplemented!("run_mergetool with builtin mergetool"),
MergeTool::Builtin => {
let tree_id = edit_merge_builtin(tree, repo_path, content).map_err(Box::new)?;
Ok(tree_id)
}
MergeTool::External(editor) => external::run_mergetool_external(
&editor, file_merge, content, repo_path, conflict, tree,
),
Expand Down
10 changes: 5 additions & 5 deletions cli/tests/test_resolve_command.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,8 +288,8 @@ fn check_resolve_produces_input_file(
// in the future. See also https://github.com/mitsuhiko/insta/issues/313.
assert_eq!(
&test_env.jj_cmd_failure(repo_path, &["resolve", "--config-toml", &merge_arg_config]),
"Error: Failed to use external tool to resolve: The output file is either unchanged or \
empty after the editor quit (run with --verbose to see the exact invocation).\n"
"Error: Failed to resolve conflicts: The output file is either unchanged or empty after \
the editor quit (run with --verbose to see the exact invocation).\n"
);
}

Expand Down Expand Up @@ -397,7 +397,7 @@ fn test_too_many_parents() {

let error = test_env.jj_cmd_failure(&repo_path, &["resolve"]);
insta::assert_snapshot!(error, @r###"
Error: Failed to use external tool to resolve: The conflict at "file" has 3 sides. At most 2 sides are supported.
Error: Failed to resolve conflicts: The conflict at "file" has 3 sides. At most 2 sides are supported.
"###);
}

Expand Down Expand Up @@ -472,7 +472,7 @@ fn test_file_vs_dir() {
"###);
let error = test_env.jj_cmd_failure(&repo_path, &["resolve"]);
insta::assert_snapshot!(error, @r###"
Error: Failed to use external tool to resolve: Only conflicts that involve normal files (not symlinks, not executable, etc.) are supported. Conflict summary for "file":
Error: Failed to resolve conflicts: Only conflicts that involve normal files (not symlinks, not executable, etc.) are supported. Conflict summary for "file":
Conflict:
Removing file with id df967b96a579e45a18b8251732d16804b2e56a55
Adding file with id 78981922613b2afb6025042ff6bd878ac1994e85
Expand Down Expand Up @@ -526,7 +526,7 @@ fn test_description_with_dir_and_deletion() {
"###);
let error = test_env.jj_cmd_failure(&repo_path, &["resolve"]);
insta::assert_snapshot!(error, @r###"
Error: Failed to use external tool to resolve: Only conflicts that involve normal files (not symlinks, not executable, etc.) are supported. Conflict summary for "file":
Error: Failed to resolve conflicts: Only conflicts that involve normal files (not symlinks, not executable, etc.) are supported. Conflict summary for "file":
Conflict:
Removing file with id df967b96a579e45a18b8251732d16804b2e56a55
Removing file with id df967b96a579e45a18b8251732d16804b2e56a55
Expand Down