merge_tools: Allow 3-pane diff editing

As discussed in https://github.com/martinvonz/jj/discussions/1905#discussioncomment-6589673
This commit is contained in:
Ilya Grigoriev 2023-08-07 16:40:10 -07:00
parent ccd4f8e159
commit 038867fd3f
5 changed files with 246 additions and 17 deletions

View file

@ -71,6 +71,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
external program. For configuration, see [the documentation](docs/config.md).
[#1886](https://github.com/martinvonz/jj/issues/1886)
* A new experimental diff editor `meld-3` is introduced that sets up Meld to
allow you to see both sides of the original diff while editing. This can be
used with `jj split`, `jj move -i`, etc.
* `jj log`/`obslog`/`op log` now supports `--limit N` option to show the first
`N` entries.

View file

@ -7,6 +7,10 @@ merge-args = ["$base", "$left", "$right", "-o", "$output", "--auto"]
edit-args = ["$left", "$right"]
merge-args = ["$left", "$base", "$right", "-o", "$output", "--auto-merge"]
[merge-tools.meld-3]
program="meld"
edit-args = ["$left", "$output", "$right", "-o", "$output"]
[merge-tools.vimdiff]
program = "vim"
# `-d` enables diff mode. `-f` makes vim run in foreground even if it starts a GUI.

View file

@ -144,6 +144,7 @@ struct DiffWorkingCopies {
_temp_dir: TempDir,
left_tree_state: TreeState,
right_tree_state: TreeState,
output_tree_state: Option<TreeState>,
}
impl DiffWorkingCopies {
@ -155,13 +156,28 @@ impl DiffWorkingCopies {
self.right_tree_state.working_copy_path()
}
fn output_working_copy_path(&self) -> Option<&Path> {
self.output_tree_state
.as_ref()
.map(|state| state.working_copy_path())
}
fn to_command_variables(&self) -> HashMap<&'static str, &str> {
let left_wc_dir = self.left_working_copy_path();
let right_wc_dir = self.right_working_copy_path();
maplit::hashmap! {
let mut result = maplit::hashmap! {
"left" => left_wc_dir.to_str().expect("temp_dir should be valid utf-8"),
"right" => right_wc_dir.to_str().expect("temp_dir should be valid utf-8"),
};
if let Some(output_wc_dir) = self.output_working_copy_path() {
result.insert(
"output",
output_wc_dir
.to_str()
.expect("temp_dir should be valid utf-8"),
);
}
result
}
}
@ -205,12 +221,19 @@ fn set_readonly_recursively(path: &Path) -> Result<(), std::io::Error> {
}
}
#[derive(Debug, Clone, Copy)]
enum DiffSide {
// Left,
Right,
}
/// Check out the two trees in temporary directories. Only include changed files
/// in the sparse checkout patterns.
fn check_out_trees(
store: &Arc<Store>,
left_tree: &Tree,
right_tree: &Tree,
output_is: Option<DiffSide>,
matcher: &dyn Matcher,
) -> Result<DiffWorkingCopies, DiffCheckoutError> {
let changed_files = left_tree
@ -235,12 +258,29 @@ fn check_out_trees(
right_wc_dir,
right_state_dir,
right_tree,
changed_files,
changed_files.clone(),
)?;
let output_tree_state = output_is
.map(|output_side| {
let output_wc_dir = temp_dir.path().join("output");
let output_state_dir = temp_dir.path().join("output_state");
check_out(
store.clone(),
output_wc_dir,
output_state_dir,
match output_side {
// DiffSide::Left => left_tree,
DiffSide::Right => right_tree,
},
changed_files,
)
})
.transpose()?;
Ok(DiffWorkingCopies {
_temp_dir: temp_dir,
left_tree_state,
right_tree_state,
output_tree_state,
})
}
@ -369,7 +409,6 @@ fn interpolate_variables<V: AsRef<str>>(
}
/// Return all variable names found in the args, without the dollar sign
#[allow(unused)]
fn find_all_variables(args: &[String]) -> Vec<String> {
args.iter()
.flat_map(|arg| VARIABLE_REGEX.find_iter(arg))
@ -385,20 +424,70 @@ pub fn edit_diff_external(
base_ignores: Arc<GitIgnoreFile>,
settings: &UserSettings,
) -> Result<TreeId, DiffEditError> {
let got_output_field = find_all_variables(&editor.edit_args).contains(&"output".to_owned());
let store = left_tree.store();
let diff_wc = check_out_trees(store, left_tree, right_tree, &EverythingMatcher)?;
let diff_wc = check_out_trees(
store,
left_tree,
right_tree,
got_output_field.then_some(DiffSide::Right),
&EverythingMatcher,
)?;
set_readonly_recursively(diff_wc.left_working_copy_path())
.map_err(ExternalToolError::SetUpDir)?;
let instructions_path = diff_wc.right_working_copy_path().join("JJ-INSTRUCTIONS");
if got_output_field {
set_readonly_recursively(diff_wc.right_working_copy_path())
.map_err(ExternalToolError::SetUpDir)?;
}
let output_wc_path = diff_wc
.output_working_copy_path()
.unwrap_or_else(|| diff_wc.right_working_copy_path());
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 =
settings.diff_instructions() && !instructions.is_empty() && !instructions_path.exists();
let add_instructions = settings.diff_instructions()
&& !instructions.is_empty()
&& !instructions_path_to_cleanup.exists();
if add_instructions {
// TODO: This can be replaced with std::fs::write. Is this used in other places
// as well?
let mut file = File::create(&instructions_path).map_err(ExternalToolError::SetUpDir)?;
file.write_all(instructions.as_bytes())
let mut output_instructions_file =
File::create(&instructions_path_to_cleanup).map_err(ExternalToolError::SetUpDir)?;
if diff_wc.right_working_copy_path() != output_wc_path {
let mut right_instructions_file =
File::create(diff_wc.right_working_copy_path().join("JJ-INSTRUCTIONS"))
.map_err(ExternalToolError::SetUpDir)?;
right_instructions_file
.write_all(
b"\
The content of this pane should NOT be edited. Any edits will be
lost.
You are using the experimental 3-pane diff editor config. Some of
the following instructions may have been written with a 2-pane
diff editing in mind and be a little inaccurate.
",
)
.map_err(ExternalToolError::SetUpDir)?;
right_instructions_file
.write_all(instructions.as_bytes())
.map_err(ExternalToolError::SetUpDir)?;
// Note that some diff tools might not show this message and delete the contents
// of the output dir instead. Meld does show this message.
output_instructions_file
.write_all(
b"\
Please make your edits in this pane.
You are using the experimental 3-pane diff editor config. Some of
the following instructions may have been written with a 2-pane
diff editing in mind and be a little inaccurate.
",
)
.map_err(ExternalToolError::SetUpDir)?;
}
output_instructions_file
.write_all(instructions.as_bytes())
.map_err(ExternalToolError::SetUpDir)?;
}
@ -418,17 +507,19 @@ pub fn edit_diff_external(
}));
}
if add_instructions {
std::fs::remove_file(instructions_path).ok();
std::fs::remove_file(instructions_path_to_cleanup).ok();
}
let mut right_tree_state = diff_wc.right_tree_state;
right_tree_state.snapshot(SnapshotOptions {
let mut output_tree_state = diff_wc
.output_tree_state
.unwrap_or(diff_wc.right_tree_state);
output_tree_state.snapshot(SnapshotOptions {
base_ignores,
fsmonitor_kind: settings.fsmonitor_kind()?,
progress: None,
max_new_file_size: settings.max_new_file_size()?,
})?;
Ok(right_tree_state.current_tree_id().clone())
Ok(output_tree_state.current_tree_id().clone())
}
/// Generates textual diff by the specified `tool`, and writes into `writer`.
@ -441,7 +532,7 @@ pub fn generate_diff(
tool: &ExternalMergeTool,
) -> Result<(), DiffGenerateError> {
let store = left_tree.store();
let diff_wc = check_out_trees(store, left_tree, right_tree, matcher)?;
let diff_wc = check_out_trees(store, left_tree, right_tree, None, matcher)?;
set_readonly_recursively(diff_wc.left_working_copy_path())
.map_err(ExternalToolError::SetUpDir)?;
set_readonly_recursively(diff_wc.right_working_copy_path())

View file

@ -12,7 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.
use crate::common::TestEnvironment;
use crate::common::{escaped_fake_diff_editor_path, TestEnvironment};
pub mod common;
@ -173,6 +173,122 @@ fn test_diffedit_new_file() {
"###);
}
#[test]
fn test_diffedit_3pane() {
let mut test_env = TestEnvironment::default();
test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]);
let repo_path = test_env.env_root().join("repo");
std::fs::write(repo_path.join("file1"), "a\n").unwrap();
test_env.jj_cmd_success(&repo_path, &["new"]);
std::fs::write(repo_path.join("file2"), "a\n").unwrap();
std::fs::write(repo_path.join("file3"), "a\n").unwrap();
test_env.jj_cmd_success(&repo_path, &["new"]);
std::fs::remove_file(repo_path.join("file1")).unwrap();
std::fs::write(repo_path.join("file2"), "b\n").unwrap();
// 2 configs for a 3-pane setup. In the first, "$right" is passed to what the
// fake diff editor considers the "after" state.
let config_with_right_as_after = format!(
r#"ui.diff-editor=["{}", "$left", "$right", "--ignore=$output"]"#,
escaped_fake_diff_editor_path()
);
let config_with_output_as_after = format!(
r#"ui.diff-editor=["{}", "$left", "$output", "--ignore=$right"]"#,
escaped_fake_diff_editor_path()
);
let edit_script = test_env.env_root().join("diff_edit_script");
std::fs::write(&edit_script, "").unwrap();
test_env.add_env_var("DIFF_EDIT_SCRIPT", edit_script.to_str().unwrap());
// Nothing happens if we make no changes
std::fs::write(
&edit_script,
"files-before file1 file2\0files-after JJ-INSTRUCTIONS file2",
)
.unwrap();
let stdout = test_env.jj_cmd_success(
&repo_path,
&["diffedit", "--config-toml", &config_with_output_as_after],
);
insta::assert_snapshot!(stdout, @r###"
Nothing changed.
"###);
let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s"]);
insta::assert_snapshot!(stdout, @r###"
R file1
M file2
"###);
// Nothing happens if we make no changes, `config_with_right_as_after` version
let stdout = test_env.jj_cmd_success(
&repo_path,
&["diffedit", "--config-toml", &config_with_right_as_after],
);
insta::assert_snapshot!(stdout, @r###"
Nothing changed.
"###);
let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s"]);
insta::assert_snapshot!(stdout, @r###"
R file1
M file2
"###);
// Can edit changes to individual files
std::fs::write(&edit_script, "reset file2").unwrap();
let stdout = test_env.jj_cmd_success(
&repo_path,
&["diffedit", "--config-toml", &config_with_output_as_after],
);
insta::assert_snapshot!(stdout, @r###"
Created kkmpptxz 1930da4a (no description set)
Working copy now at: kkmpptxz 1930da4a (no description set)
Parent commit : rlvkpnrz 613028a4 (no description set)
Added 0 files, modified 1 files, removed 0 files
"###);
let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s"]);
insta::assert_snapshot!(stdout, @r###"
R file1
"###);
// Can write something new to `file1`
test_env.jj_cmd_success(&repo_path, &["undo"]);
std::fs::write(&edit_script, "write file1\nnew content").unwrap();
let stdout = test_env.jj_cmd_success(
&repo_path,
&["diffedit", "--config-toml", &config_with_output_as_after],
);
insta::assert_snapshot!(stdout, @r###"
Created kkmpptxz ff2907b6 (no description set)
Working copy now at: kkmpptxz ff2907b6 (no description set)
Parent commit : rlvkpnrz 613028a4 (no description set)
Added 1 files, modified 0 files, removed 0 files
"###);
let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s"]);
insta::assert_snapshot!(stdout, @r###"
M file1
M file2
"###);
// But nothing happens if we modify the right side
test_env.jj_cmd_success(&repo_path, &["undo"]);
std::fs::write(&edit_script, "write file1\nnew content").unwrap();
let stdout = test_env.jj_cmd_success(
&repo_path,
&["diffedit", "--config-toml", &config_with_right_as_after],
);
insta::assert_snapshot!(stdout, @r###"
Nothing changed.
"###);
let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s"]);
insta::assert_snapshot!(stdout, @r###"
R file1
M file2
"###);
// TODO: test with edit_script of "reset file2". This fails on right side
// since the file is readonly.
}
#[test]
fn test_diffedit_merge() {
let mut test_env = TestEnvironment::default();

View file

@ -374,6 +374,20 @@ merge-tools.kdiff3.edit-args = [
"--merge", "--cs", "CreateBakFiles=0", "$left", "$right"]
```
### Experimental 3-pane diff editing
The special `"meld-3"` diff editor sets up Meld to show 3 panes: the sides of
the diff on the left and right, and an editing pane in the middle. This allow
you to see both sides of the original diff while editing. If you use
`ui.diff-editor = "meld-3"`, note that you can still get the 2-pane Meld view
using `jj diff --tool meld`.
To configure other diff editors, you can include `$output` together with `$left`
and `$right` in `merge-tools.TOOL.edit-args`. `jj` will replace `$output` with
the directory where the diff editor will be expected to put the result of the
user's edits. Initially, the contents of `$output` will be the same as the
contents of `$right`.
### Setting up `scm-diff-editor`
`scm-diff-editor` is a terminal-based diff editor that is part of