Skip to content

Commit

Permalink
merge_tools: introduce Diff/MergeEditor newtypes
Browse files Browse the repository at this point in the history
I'm going to make them be loaded by caller, and these newtypes will provide
extra compile-time safety (plus nicer API to be added later.) The error types
will be cleaned up later patches.
  • Loading branch information
yuja committed Mar 2, 2024
1 parent 529acb3 commit 863a264
Showing 1 changed file with 54 additions and 37 deletions.
91 changes: 54 additions & 37 deletions cli/src/merge_tools/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ pub fn run_mergetool(
};
let content = extract_as_single_hunk(&file_merge, tree.store(), repo_path).block_on();

let editor = get_merge_tool_from_settings(ui, settings)?;
let editor = MergeEditor::from_settings(ui, settings)?.tool;
match editor {
MergeTool::Builtin => {
let tree_id = edit_merge_builtin(tree, repo_path, content).map_err(Box::new)?;
Expand All @@ -137,7 +137,7 @@ pub fn edit_diff(
settings: &UserSettings,
) -> Result<MergedTreeId, DiffEditError> {
// Start a diff editor on the two directories.
let editor = get_diff_editor_from_settings(ui, settings)?;
let editor = DiffEditor::from_settings(ui, settings)?.tool;
match editor {
MergeTool::Builtin => {
let tree_id = edit_diff_builtin(left_tree, right_tree, matcher).map_err(Box::new)?;
Expand Down Expand Up @@ -215,38 +215,51 @@ pub fn get_external_tool_config(
}
}

fn get_diff_editor_from_settings(
ui: &Ui,
settings: &UserSettings,
) -> Result<MergeTool, ExternalToolError> {
let args = editor_args_from_settings(ui, settings, "ui.diff-editor")?;
let editor = if let CommandNameAndArgs::String(name) = &args {
get_tool_config(settings, name)?
} else {
None
}
.unwrap_or_else(|| MergeTool::External(ExternalMergeTool::with_edit_args(&args)));
Ok(editor)
/// Configured diff editor.
#[derive(Clone, Debug)]
pub struct DiffEditor {
tool: MergeTool,
}

fn get_merge_tool_from_settings(
ui: &Ui,
settings: &UserSettings,
) -> Result<MergeTool, ExternalToolError> {
let args = editor_args_from_settings(ui, settings, "ui.merge-editor")?;
let mergetool = if let CommandNameAndArgs::String(name) = &args {
get_tool_config(settings, name)?
} else {
None
impl DiffEditor {
/// Loads the default diff editor from the settings.
// TODO: extract tool configuration error from DiffEditError
pub fn from_settings(ui: &Ui, settings: &UserSettings) -> Result<Self, DiffEditError> {
let args = editor_args_from_settings(ui, settings, "ui.diff-editor")?;
let tool = if let CommandNameAndArgs::String(name) = &args {
get_tool_config(settings, name).map_err(ExternalToolError::Config)?
} else {
None
}
.unwrap_or_else(|| MergeTool::External(ExternalMergeTool::with_edit_args(&args)));
Ok(DiffEditor { tool })
}
.unwrap_or_else(|| MergeTool::External(ExternalMergeTool::with_merge_args(&args)));
match mergetool {
MergeTool::External(mergetool) if mergetool.merge_args.is_empty() => {
Err(ExternalToolError::MergeArgsNotConfigured {
}

/// Configured 3-way merge editor.
#[derive(Clone, Debug)]
pub struct MergeEditor {
tool: MergeTool,
}

impl MergeEditor {
/// Loads the default 3-way merge editor from the settings.
// TODO: extract tool configuration error from ConflictResolveError
pub fn from_settings(ui: &Ui, settings: &UserSettings) -> Result<Self, ConflictResolveError> {
let args = editor_args_from_settings(ui, settings, "ui.merge-editor")?;
let tool = if let CommandNameAndArgs::String(name) = &args {
get_tool_config(settings, name).map_err(ExternalToolError::Config)?
} else {
None
}
.unwrap_or_else(|| MergeTool::External(ExternalMergeTool::with_merge_args(&args)));
if matches!(&tool, MergeTool::External(mergetool) if mergetool.merge_args.is_empty()) {
return Err(ExternalToolError::MergeArgsNotConfigured {
tool_name: args.to_string(),
})
}
.into());
}
mergetool => Ok(mergetool),
Ok(MergeEditor { tool })
}
}

Expand All @@ -269,7 +282,7 @@ mod tests {
let config = config_from_string(text);
let ui = Ui::with_config(&config).unwrap();
let settings = UserSettings::from_config(config);
get_diff_editor_from_settings(&ui, &settings)
DiffEditor::from_settings(&ui, &settings).map(|editor| editor.tool)
};

// Default
Expand Down Expand Up @@ -418,17 +431,19 @@ mod tests {
let config = config_from_string(text);
let ui = Ui::with_config(&config).unwrap();
let settings = UserSettings::from_config(config);
get_merge_tool_from_settings(&ui, &settings)
MergeEditor::from_settings(&ui, &settings).map(|editor| editor.tool)
};

// Default
insta::assert_debug_snapshot!(get("").unwrap(), @"Builtin");

// Just program name
insta::assert_debug_snapshot!(get(r#"ui.merge-editor = "my-merge""#).unwrap_err(), @r###"
MergeArgsNotConfigured {
tool_name: "my-merge",
}
ExternalTool(
MergeArgsNotConfigured {
tool_name: "my-merge",
},
)
"###);

// String args
Expand Down Expand Up @@ -516,9 +531,11 @@ mod tests {
// List args should never be a merge-tools key
insta::assert_debug_snapshot!(
get(r#"ui.merge-editor = ["meld"]"#).unwrap_err(), @r###"
MergeArgsNotConfigured {
tool_name: "meld",
}
ExternalTool(
MergeArgsNotConfigured {
tool_name: "meld",
},
)
"###);

// Invalid type
Expand Down

0 comments on commit 863a264

Please sign in to comment.