Skip to content

Commit

Permalink
merge_tools: Allow 3-pane diff editing
Browse files Browse the repository at this point in the history 
As discussed in #1905 (reply in thread)
  • Loading branch information
@ilyagr
ilyagr committed Aug 21, 2023
1 parent 7668ab3 commit 64dd590
Show file tree
Hide file tree
Showing 5 changed files with 289 additions and 27 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,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.

Expand Down
4 changes: 4 additions & 0 deletions cli/src/config/merge_tools.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
176 changes: 150 additions & 26 deletions cli/src/merge_tools/external.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ struct DiffWorkingCopies {
_temp_dir: TempDir,
left_tree_state: TreeState,
right_tree_state: TreeState,
output_tree_state: Option<TreeState>,
}

impl DiffWorkingCopies {
Expand All @@ -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
}
}

Expand Down Expand Up @@ -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
Expand All @@ -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,
})
}

Expand Down Expand Up @@ -344,27 +384,40 @@ pub fn run_mergetool_external(
Ok(tree_builder.write_tree())
}

// Not interested in $UPPER_CASE_VARIABLES
static VARIABLE_REGEX: once_cell::sync::Lazy<Result<Regex, regex::Error>> =
once_cell::sync::Lazy::new(|| Regex::new(r"\$([a-z0-9_]+)\b"));

fn interpolate_variables<V: AsRef<str>>(
args: &[String],
variables: &HashMap<&str, V>,
) -> Vec<String> {
// Not interested in $UPPER_CASE_VARIABLES
let re = Regex::new(r"\$([a-z0-9_]+)\b").unwrap();
args.iter()
.map(|arg| {
re.replace_all(arg, |caps: &Captures| {
let name = &caps[1];
if let Some(subst) = variables.get(name) {
subst.as_ref().to_owned()
} else {
caps[0].to_owned()
}
})
.into_owned()
VARIABLE_REGEX
.as_ref()
.unwrap()
.replace_all(arg, |caps: &Captures| {
let name = &caps[1];
if let Some(subst) = variables.get(name) {
subst.as_ref().to_owned()
} else {
caps[0].to_owned()
}
})
.into_owned()
})
.collect()
}

/// Return all variable names found in the args, without the dollar sign
fn find_all_variables(args: &[String]) -> Vec<String> {
args.iter()
.flat_map(|arg| VARIABLE_REGEX.as_ref().unwrap().find_iter(arg))
.map(|single_match| single_match.as_str()[1..].to_owned())
.collect()
}

pub fn edit_diff_external(
editor: ExternalMergeTool,
left_tree: &Tree,
Expand All @@ -373,20 +426,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)?;
}

Expand All @@ -406,17 +509,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`.
Expand All @@ -429,7 +534,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())
Expand Down Expand Up @@ -515,4 +620,23 @@ mod tests {
["$left $right"],
);
}

#[test]
fn test_find_all_variables() {
assert_eq!(
find_all_variables(
&[
"$left",
"$1",
"$right",
"$2",
"--can-be-part-of-string=$output",
"$NOT_CAPITALS",
"--can-repeat=$right"
]
.map(ToOwned::to_owned),
),
["left", "1", "right", "2", "output", "right"],
);
}
}
Loading

0 comments on commit 64dd590

Please sign in to comment.