Skip to content

Commit

Permalink
diff_working_copies: Simplify tree checkout functions and update Diff…
Browse files Browse the repository at this point in the history 
…Side enum.

I got annoyed by the checkout function above in my previous commit and tried to
compress the code a bit, and I think the result is much nicer. I'm not certain
that a closure is the right solution here, but I think it becomes easier to
reason about the two temp filenames that get generated.

I initially changed Option<DiffSide> to a bool, but found an enum more meaningful
and more likely to be flexible in the future.

I also moved the instruction writing to its own function. This makes the main
`DiffEditWorkingCopies::check_out()` function is much nicer. 

J: This commit contains the following changes:
  • Loading branch information
@wrzian
wrzian committed Sep 24, 2024
1 parent d3adb58 commit 98fe553
Show file tree
Hide file tree
Showing 2 changed files with 99 additions and 115 deletions.
192 changes: 84 additions & 108 deletions cli/src/merge_tools/diff_working_copies.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ use jj_lib::matchers::EverythingMatcher;
use jj_lib::matchers::Matcher;
use jj_lib::merged_tree::MergedTree;
use jj_lib::merged_tree::TreeDiffEntry;
use jj_lib::repo_path::RepoPathBuf;
use jj_lib::store::Store;
use jj_lib::working_copy::CheckoutError;
use jj_lib::working_copy::SnapshotOptions;
Expand Down Expand Up @@ -78,22 +77,6 @@ impl DiffWorkingCopies {
}
}

fn check_out(
store: Arc<Store>,
wc_dir: PathBuf,
state_dir: PathBuf,
tree: &MergedTree,
sparse_patterns: Vec<RepoPathBuf>,
exec_config: Option<bool>,
) -> Result<TreeState, DiffCheckoutError> {
std::fs::create_dir(&wc_dir).map_err(DiffCheckoutError::SetUpDir)?;
std::fs::create_dir(&state_dir).map_err(DiffCheckoutError::SetUpDir)?;
let mut tree_state = TreeState::init(store, wc_dir, state_dir, exec_config)?;
tree_state.set_sparse_patterns(sparse_patterns)?;
tree_state.check_out(tree)?;
Ok(tree_state)
}

pub(crate) fn new_utf8_temp_dir(prefix: &str) -> io::Result<TempDir> {
let temp_dir = tempfile::Builder::new().prefix(prefix).tempdir()?;
if temp_dir.path().to_str().is_none() {
Expand Down Expand Up @@ -122,20 +105,24 @@ pub(crate) fn set_readonly_recursively(path: &Path) -> Result<(), std::io::Error
}
}

/// How to prepare tree states from the working copy for a diff viewer/editor.
///
/// TwoWay: prepare a left and right tree; left is readonly.
/// ThreeWay: prepare left, right, and output trees; left + right are readonly.
#[derive(Debug, Clone, Copy)]
pub(crate) enum DiffSide {
// Left,
Right,
pub(crate) enum DiffType {
TwoWay,
ThreeWay,
}

/// Check out the two trees in temporary directories. Only include changed files
/// in the sparse checkout patterns.
/// Check out the two trees in temporary directories and make appropriate sides
/// readonly. Only include changed files in the sparse checkout patterns.
pub(crate) fn check_out_trees(
store: &Arc<Store>,
left_tree: &MergedTree,
right_tree: &MergedTree,
matcher: &dyn Matcher,
output_is: Option<DiffSide>,
diff_type: DiffType,
exec_config: Option<bool>,
) -> Result<DiffWorkingCopies, DiffCheckoutError> {
let changed_files: Vec<_> = left_tree
Expand All @@ -145,43 +132,32 @@ pub(crate) fn check_out_trees(
.block_on();

let temp_dir = new_utf8_temp_dir("jj-diff-").map_err(DiffCheckoutError::SetUpDir)?;
let left_wc_dir = temp_dir.path().join("left");
let left_state_dir = temp_dir.path().join("left_state");
let right_wc_dir = temp_dir.path().join("right");
let right_state_dir = temp_dir.path().join("right_state");
let left_tree_state = check_out(
store.clone(),
left_wc_dir,
left_state_dir,
left_tree,
changed_files.clone(),
exec_config,
)?;
let right_tree_state = check_out(
store.clone(),
right_wc_dir,
right_state_dir,
right_tree,
changed_files.clone(),
exec_config,
)?;
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,
exec_config,
)
})
.transpose()?;
let temp_path = temp_dir.path();

let check_out = |name: &str, tree, files, read_only| -> Result<TreeState, DiffCheckoutError> {
let wc_dir = temp_path.join(name);
let state_dir = temp_path.join(format!("{}_state", name));
std::fs::create_dir(&wc_dir).map_err(DiffCheckoutError::SetUpDir)?;
std::fs::create_dir(&state_dir).map_err(DiffCheckoutError::SetUpDir)?;
let mut tree_state = TreeState::init(store.clone(), wc_dir, state_dir, exec_config)?;
tree_state.set_sparse_patterns(files)?;
tree_state.check_out(tree)?;
if read_only {
set_readonly_recursively(tree_state.working_copy_path())
.map_err(DiffCheckoutError::SetUpDir)?;
}
Ok(tree_state)
};

let left_tree_state = check_out("left", left_tree, changed_files.clone(), true)?;
let (right_tree_state, output_tree_state) = match diff_type {
DiffType::TwoWay => (check_out("right", right_tree, changed_files, false)?, None),
DiffType::ThreeWay => (
check_out("right", right_tree, changed_files.clone(), true)?,
Some(check_out("output", right_tree, changed_files, false)?),
),
};

Ok(DiffWorkingCopies {
_temp_dir: temp_dir,
left_tree_state,
Expand All @@ -203,7 +179,7 @@ impl DiffEditWorkingCopies {
left_tree: &MergedTree,
right_tree: &MergedTree,
matcher: &dyn Matcher,
output_is: Option<DiffSide>,
diff_type: DiffType,
instructions: Option<&str>,
exec_config: Option<bool>,
) -> Result<Self, DiffEditError> {
Expand All @@ -212,38 +188,42 @@ impl DiffEditWorkingCopies {
left_tree,
right_tree,
matcher,
output_is,
diff_type,
exec_config,
)?;
let got_output_field = output_is.is_some();
let instructions_path_to_cleanup = instructions
.map(|instructions| Self::write_edit_instructions(&diff_wc, instructions))
.transpose()?;
Ok(Self {
working_copies: diff_wc,
instructions_path_to_cleanup,
})
}

set_readonly_recursively(diff_wc.left_working_copy_path())
.map_err(ExternalToolError::SetUpDir)?;
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());
fn write_edit_instructions(
diff_wc: &DiffWorkingCopies,
instructions: &str,
) -> Result<PathBuf, DiffEditError> {
let (right_wc_path, output_wc_path) = match diff_wc.output_working_copy_path() {
Some(output_path) => (Some(diff_wc.right_working_copy_path()), output_path),
None => (None, diff_wc.right_working_copy_path()),
};
let output_instructions_path = 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 !output_instructions_path.exists() {
instructions
} else {
None
};
if let Some(instructions) = add_instructions {
let mut output_instructions_file =
File::create(&output_instructions_path).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"\
if output_instructions_path.exists() {
return Ok(output_instructions_path);
}
let mut output_instructions_file =
File::create(&output_instructions_path).map_err(ExternalToolError::SetUpDir)?;

// Write out our experimental three-way merge instructions first.
if let Some(right_wc_path) = right_wc_path {
let mut right_instructions_file = File::create(right_wc_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.
Expand All @@ -252,35 +232,31 @@ 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"\
)
.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)?;
};

Ok(Self {
working_copies: diff_wc,
instructions_path_to_cleanup: add_instructions.map(|_| output_instructions_path),
})
}
// Now write the passed-in instructions.
output_instructions_file
.write_all(instructions.as_bytes())
.map_err(ExternalToolError::SetUpDir)?;
Ok(output_instructions_path)
}

pub fn snapshot_results(
Expand Down
22 changes: 15 additions & 7 deletions cli/src/merge_tools/external.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ use super::diff_working_copies::check_out_trees;
use super::diff_working_copies::new_utf8_temp_dir;
use super::diff_working_copies::set_readonly_recursively;
use super::diff_working_copies::DiffEditWorkingCopies;
use super::diff_working_copies::DiffSide;
use super::diff_working_copies::DiffType;
use super::ConflictResolveError;
use super::DiffEditError;
use super::DiffGenerateError;
Expand Down Expand Up @@ -258,13 +258,18 @@ pub fn edit_diff_external(
exec_config: Option<bool>,
) -> Result<MergedTreeId, DiffEditError> {
let got_output_field = find_all_variables(&editor.edit_args).contains(&"output");
let diff_type = if got_output_field {
DiffType::ThreeWay
} else {
DiffType::TwoWay
};
let store = left_tree.store();
let diffedit_wc = DiffEditWorkingCopies::check_out(
store,
left_tree,
right_tree,
matcher,
got_output_field.then_some(DiffSide::Right),
diff_type,
instructions,
exec_config,
)?;
Expand Down Expand Up @@ -298,11 +303,14 @@ 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, None, ui.exec_config)?;
set_readonly_recursively(diff_wc.left_working_copy_path())
.map_err(ExternalToolError::SetUpDir)?;
set_readonly_recursively(diff_wc.right_working_copy_path())
.map_err(ExternalToolError::SetUpDir)?;
let diff_wc = check_out_trees(
store,
left_tree,
right_tree,
matcher,
DiffType::TwoWay,
ui.exec_config,
)?;
invoke_external_diff(ui, writer, tool, &diff_wc.to_command_variables())
}

Expand Down

0 comments on commit 98fe553

Please sign in to comment.