Skip to content

Commit

Permalink
merge_tools: process "ui.diff-instructions" option by caller
Browse files Browse the repository at this point in the history
This gets rid of the last UserSettings dependency from edit_diff_external().
I'm going to remove it from edit_diff() too, and let callers pass a
preconfigured MergeTool struct instead.

These changes will make it easier to add --tool=<name> argument jj-vcs#2575.
  • Loading branch information
yuja committed Mar 1, 2024
1 parent ea1aa53 commit 38a0844
Show file tree
Hide file tree
Showing 4 changed files with 29 additions and 12 deletions.
5 changes: 5 additions & 0 deletions cli/src/cli_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1746,6 +1746,11 @@ impl WorkspaceCommandTransaction<'_> {
) -> Result<MergedTreeId, CommandError> {
let base_ignores = self.helper.base_ignores()?;
let settings = &self.helper.settings;
let instructions = if settings.diff_instructions() {
instructions
} else {
None
};
Ok(crate::merge_tools::edit_diff(
ui,
left_tree,
Expand Down
5 changes: 1 addition & 4 deletions cli/src/merge_tools/external.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ use jj_lib::matchers::Matcher;
use jj_lib::merge::{Merge, MergedTreeValue};
use jj_lib::merged_tree::{MergedTree, MergedTreeBuilder};
use jj_lib::repo_path::{RepoPath, RepoPathBuf};
use jj_lib::settings::UserSettings;
use jj_lib::store::Store;
use jj_lib::working_copy::{CheckoutError, SnapshotOptions};
use pollster::FutureExt;
Expand Down Expand Up @@ -429,7 +428,6 @@ pub fn edit_diff_external(
matcher: &dyn Matcher,
instructions: Option<&str>,
base_ignores: Arc<GitIgnoreFile>,
settings: &UserSettings,
) -> Result<MergedTreeId, DiffEditError> {
let got_output_field = find_all_variables(&editor.edit_args).contains(&"output");
let store = left_tree.store();
Expand All @@ -452,8 +450,7 @@ pub fn edit_diff_external(
let instructions_path_to_cleanup = output_wc_path.join("JJ-INSTRUCTIONS");
// In the unlikely event that the file already exists, then the user will simply
// not get any instructions.
let add_instructions = if settings.diff_instructions() && !instructions_path_to_cleanup.exists()
{
let add_instructions = if !instructions_path_to_cleanup.exists() {
instructions
} else {
None
Expand Down
1 change: 0 additions & 1 deletion cli/src/merge_tools/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,6 @@ pub fn edit_diff(
matcher,
instructions,
base_ignores,
settings,
),
}
}
Expand Down
30 changes: 23 additions & 7 deletions cli/tests/test_diffedit_command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,22 @@ fn test_diffedit() {
M file2
"###);

// Try again with ui.diff-instructions=false
std::fs::write(&edit_script, "files-before file1 file2\0files-after file2").unwrap();
let (stdout, stderr) = test_env.jj_cmd_ok(
&repo_path,
&["diffedit", "--config-toml=ui.diff-instructions=false"],
);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Nothing changed.
"###);
let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s"]);
insta::assert_snapshot!(stdout, @r###"
D file1
M file2
"###);

// Nothing happens if the diff-editor exits with an error
std::fs::write(&edit_script, "rm file2\0fail").unwrap();
insta::assert_snapshot!(&test_env.jj_cmd_failure(&repo_path, &["diffedit"]), @r###"
Expand All @@ -64,8 +80,8 @@ fn test_diffedit() {
let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["diffedit"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Created kkmpptxz 1930da4a (no description set)
Working copy now at: kkmpptxz 1930da4a (no description set)
Created kkmpptxz ae84b067 (no description set)
Working copy now at: kkmpptxz ae84b067 (no description set)
Parent commit : rlvkpnrz 613028a4 (no description set)
Added 0 files, modified 1 files, removed 0 files
"###);
Expand All @@ -80,10 +96,10 @@ fn test_diffedit() {
let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["diffedit", "-r", "@-"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Created rlvkpnrz c03ae967 (no description set)
Created rlvkpnrz 51a4f74c (no description set)
Rebased 1 descendant commits
Working copy now at: kkmpptxz 2a4dc204 (no description set)
Parent commit : rlvkpnrz c03ae967 (no description set)
Working copy now at: kkmpptxz 574af856 (no description set)
Parent commit : rlvkpnrz 51a4f74c (no description set)
Added 0 files, modified 1 files, removed 0 files
"###);
let contents = String::from_utf8(std::fs::read(repo_path.join("file3")).unwrap()).unwrap();
Expand All @@ -101,8 +117,8 @@ fn test_diffedit() {
let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["diffedit", "--from", "@--"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Created kkmpptxz 15f2c966 (no description set)
Working copy now at: kkmpptxz 15f2c966 (no description set)
Created kkmpptxz cd638328 (no description set)
Working copy now at: kkmpptxz cd638328 (no description set)
Parent commit : rlvkpnrz 613028a4 (no description set)
Added 0 files, modified 0 files, removed 1 files
"###);
Expand Down

0 comments on commit 38a0844

Please sign in to comment.