Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli: skip formatting instructions if not required #4052

Merged
merged 1 commit into from
Jul 10, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
cli: skip formatting instructions if not required
`tx.format_commit_summary()` can be expensive because it needs to build
an IdPrefixContext now, so it's best to avoid formatting instruction
messages unless they are actually required.
scott2000 committed Jul 9, 2024
commit 50b375da20563e3a7daf152f63516e7b25543dcc
4 changes: 2 additions & 2 deletions cli/src/cli_util.rs
Original file line number Diff line number Diff line change
@@ -2248,12 +2248,12 @@ impl DiffSelector {
left_tree: &MergedTree,
right_tree: &MergedTree,
matcher: &dyn Matcher,
instructions: Option<&str>,
format_instructions: impl FnOnce() -> String,
) -> Result<MergedTreeId, CommandError> {
match self {
DiffSelector::NonInteractive => Ok(restore_tree(right_tree, left_tree, matcher)?),
DiffSelector::Interactive(editor) => {
Ok(editor.edit(left_tree, right_tree, matcher, instructions)?)
Ok(editor.edit(left_tree, right_tree, matcher, format_instructions)?)
}
}
}
12 changes: 7 additions & 5 deletions cli/src/commands/commit.rs
Original file line number Diff line number Diff line change
@@ -71,21 +71,23 @@ pub(crate) fn cmd_commit(
workspace_command.diff_selector(ui, args.tool.as_deref(), args.interactive)?;
let mut tx = workspace_command.start_transaction();
let base_tree = commit.parent_tree(tx.repo())?;
let instructions = format!(
"\
let format_instructions = || {
format!(
"\
You are splitting the working-copy commit: {}

The diff initially shows all changes. Adjust the right side until it shows the
contents you want for the first commit. The remainder will be included in the
new working-copy commit.
",
tx.format_commit_summary(&commit)
);
tx.format_commit_summary(&commit)
)
};
let tree_id = diff_selector.select(
&base_tree,
&commit.tree()?,
matcher.as_ref(),
Some(&instructions),
format_instructions,
)?;
let middle_tree = tx.repo().store().get_root_tree(&tree_id)?;
if !args.paths.is_empty() && middle_tree.id() == base_tree.id() {
12 changes: 7 additions & 5 deletions cli/src/commands/diffedit.rs
Original file line number Diff line number Diff line change
@@ -89,19 +89,21 @@ pub(crate) fn cmd_diffedit(

let diff_editor = workspace_command.diff_editor(ui, args.tool.as_deref())?;
let mut tx = workspace_command.start_transaction();
let instructions = format!(
"\
let format_instructions = || {
format!(
"\
You are editing changes in: {}

{diff_description}

Adjust the right side until it shows the contents you want. If you
don't make any changes, then the operation will be aborted.",
tx.format_commit_summary(&target_commit),
);
tx.format_commit_summary(&target_commit),
)
};
let base_tree = merge_commit_trees(tx.repo(), base_commits.as_slice())?;
let tree = target_commit.tree()?;
let tree_id = diff_editor.edit(&base_tree, &tree, &EverythingMatcher, Some(&instructions))?;
let tree_id = diff_editor.edit(&base_tree, &tree, &EverythingMatcher, format_instructions)?;
if tree_id == *target_commit.tree_id() {
writeln!(ui.status(), "Nothing changed.")?;
} else {
12 changes: 7 additions & 5 deletions cli/src/commands/split.rs
Original file line number Diff line number Diff line change
@@ -88,8 +88,9 @@ pub(crate) fn cmd_split(
let mut tx = workspace_command.start_transaction();
let end_tree = commit.tree()?;
let base_tree = commit.parent_tree(tx.repo())?;
let instructions = format!(
"\
let format_instructions = || {
format!(
"\
You are splitting a commit into two: {}

The diff initially shows the changes in the commit you're splitting.
@@ -98,12 +99,13 @@ Adjust the right side until it shows the contents you want for the first commit.
The remainder will be in the second commit. If you don't make any changes, then
the operation will be aborted.
",
tx.format_commit_summary(&commit)
);
tx.format_commit_summary(&commit)
)
};

// Prompt the user to select the changes they want for the first commit.
let selected_tree_id =
diff_selector.select(&base_tree, &end_tree, matcher.as_ref(), Some(&instructions))?;
diff_selector.select(&base_tree, &end_tree, matcher.as_ref(), format_instructions)?;
if &selected_tree_id == commit.tree_id() && diff_selector.is_interactive() {
// The user selected everything from the original commit.
writeln!(ui.status(), "Nothing changed.")?;
14 changes: 8 additions & 6 deletions cli/src/commands/squash.rs
Original file line number Diff line number Diff line change
@@ -196,8 +196,9 @@ pub fn move_diff(
for source in sources {
let parent_tree = source.parent_tree(tx.repo())?;
let source_tree = source.tree()?;
let instructions = format!(
"\
let format_instructions = || {
format!(
"\
You are moving changes from: {}
into commit: {}

@@ -209,11 +210,12 @@ Adjust the right side until the diff shows the changes you want to move
to the destination. If you don't make any changes, then all the changes
from the source will be moved into the destination.
",
tx.format_commit_summary(source),
tx.format_commit_summary(destination)
);
tx.format_commit_summary(source),
tx.format_commit_summary(destination)
)
};
let selected_tree_id =
diff_selector.select(&parent_tree, &source_tree, matcher, Some(&instructions))?;
diff_selector.select(&parent_tree, &source_tree, matcher, format_instructions)?;
let selected_tree = tx.repo().store().get_root_tree(&selected_tree_id)?;
let abandon = !keep_emptied && selected_tree.id() == source_tree.id();
if !abandon && selected_tree_id == parent_tree.id() {
14 changes: 8 additions & 6 deletions cli/src/commands/unsquash.rs
Original file line number Diff line number Diff line change
@@ -71,8 +71,9 @@ pub(crate) fn cmd_unsquash(
let parent_base_tree = parent.parent_tree(tx.repo())?;
let new_parent_tree_id;
if let Some(diff_editor) = &interactive_editor {
let instructions = format!(
"\
let format_instructions = || {
format!(
"\
You are moving changes from: {}
into its child: {}

@@ -83,15 +84,16 @@ the parent commit. The changes you edited out will be moved into the
child commit. If you don't make any changes, then the operation will be
aborted.
",
tx.format_commit_summary(&parent),
tx.format_commit_summary(&commit)
);
tx.format_commit_summary(&parent),
tx.format_commit_summary(&commit)
)
};
let parent_tree = parent.tree()?;
new_parent_tree_id = diff_editor.edit(
&parent_base_tree,
&parent_tree,
&EverythingMatcher,
Some(&instructions),
format_instructions,
)?;
if new_parent_tree_id == parent_base_tree.id() {
return Err(user_error("No changes selected"));
6 changes: 3 additions & 3 deletions cli/src/merge_tools/mod.rs
Original file line number Diff line number Diff line change
@@ -219,20 +219,20 @@ impl DiffEditor {
left_tree: &MergedTree,
right_tree: &MergedTree,
matcher: &dyn Matcher,
instructions: Option<&str>,
format_instructions: impl FnOnce() -> String,
) -> Result<MergedTreeId, DiffEditError> {
match &self.tool {
MergeTool::Builtin => {
Ok(edit_diff_builtin(left_tree, right_tree, matcher).map_err(Box::new)?)
}
MergeTool::External(editor) => {
let instructions = self.use_instructions.then_some(instructions).flatten();
let instructions = self.use_instructions.then(format_instructions);
edit_diff_external(
editor,
left_tree,
right_tree,
matcher,
instructions,
instructions.as_deref(),
self.base_ignores.clone(),
)
}
6 changes: 5 additions & 1 deletion cli/testing/fake-diff-editor.rs
Original file line number Diff line number Diff line change
@@ -57,7 +57,7 @@ fn files_recursively(p: &Path) -> HashSet<String> {
fn main() {
let args: Args = Args::parse();
let edit_script_path = PathBuf::from(std::env::var_os("DIFF_EDIT_SCRIPT").unwrap());
let edit_script = String::from_utf8(std::fs::read(edit_script_path).unwrap()).unwrap();
let edit_script = String::from_utf8(std::fs::read(&edit_script_path).unwrap()).unwrap();
for instruction in edit_script.split('\0') {
let (command, payload) = instruction.split_once('\n').unwrap_or((instruction, ""));
let parts = command.split(' ').collect_vec();
@@ -111,6 +111,10 @@ fn main() {
std::fs::remove_file(args.after.join(file)).unwrap();
}
}
["dump", file, dest] => {
let dest_path = edit_script_path.parent().unwrap().join(dest);
std::fs::copy(args.after.join(file), dest_path).unwrap();
}
["write", file] => {
std::fs::write(args.after.join(file), payload).unwrap();
}
12 changes: 11 additions & 1 deletion cli/tests/test_commit_command.rs
Original file line number Diff line number Diff line change
@@ -104,11 +104,21 @@ fn test_commit_interactive() {
std::fs::write(edit_script, ["dump editor"].join("\0")).unwrap();

let diff_editor = test_env.set_up_fake_diff_editor();
std::fs::write(diff_editor, "rm file2").unwrap();
let diff_script = ["rm file2", "dump JJ-INSTRUCTIONS instrs"].join("\0");
std::fs::write(diff_editor, diff_script).unwrap();

// Create a commit interactively and select only file1
test_env.jj_cmd_ok(&workspace_path, &["commit", "-i"]);

insta::assert_snapshot!(
std::fs::read_to_string(test_env.env_root().join("instrs")).unwrap(), @r###"
You are splitting the working-copy commit: qpvuntsm 4219467e add files

The diff initially shows all changes. Adjust the right side until it shows the
contents you want for the first commit. The remainder will be included in the
new working-copy commit.
"###);

insta::assert_snapshot!(
std::fs::read_to_string(test_env.env_root().join("editor")).unwrap(), @r###"
add files
16 changes: 15 additions & 1 deletion cli/tests/test_diffedit_command.rs
Original file line number Diff line number Diff line change
@@ -33,14 +33,28 @@ fn test_diffedit() {
// Test the setup; nothing happens if we make no changes
std::fs::write(
&edit_script,
"files-before file1 file2\0files-after JJ-INSTRUCTIONS file2",
[
"files-before file1 file2",
"files-after JJ-INSTRUCTIONS file2",
"dump JJ-INSTRUCTIONS instrs",
]
.join("\0"),
)
.unwrap();
let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["diffedit"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Nothing changed.
"###);
insta::assert_snapshot!(
std::fs::read_to_string(test_env.env_root().join("instrs")).unwrap(), @r###"
You are editing changes in: kkmpptxz 3d4cce89 (no description set)

The diff initially shows the commit's changes.

Adjust the right side until it shows the contents you want. If you
don't make any changes, then the operation will be aborted.
"###);
let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s"]);
insta::assert_snapshot!(stdout, @r###"
D file1
48 changes: 48 additions & 0 deletions cli/tests/test_split_command.rs
Original file line number Diff line number Diff line change
@@ -559,3 +559,51 @@ fn test_split_message_editor_avoids_unc() {
// over 260 chars.
assert_eq!(edited_path, dunce::simplified(&edited_path));
}

#[test]
fn test_split_interactive() {
let mut test_env = TestEnvironment::default();
test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]);
let workspace_path = test_env.env_root().join("repo");

std::fs::write(workspace_path.join("file1"), "foo\n").unwrap();
std::fs::write(workspace_path.join("file2"), "bar\n").unwrap();
let edit_script = test_env.set_up_fake_editor();
std::fs::write(edit_script, ["dump editor"].join("\0")).unwrap();

let diff_editor = test_env.set_up_fake_diff_editor();
let diff_script = ["rm file2", "dump JJ-INSTRUCTIONS instrs"].join("\0");
std::fs::write(diff_editor, diff_script).unwrap();

// Split the working commit interactively and select only file1
let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_path, &["split"]);

insta::assert_snapshot!(
std::fs::read_to_string(test_env.env_root().join("instrs")).unwrap(), @r###"
You are splitting a commit into two: qpvuntsm 44af2155 (no description set)

The diff initially shows the changes in the commit you're splitting.

Adjust the right side until it shows the contents you want for the first commit.
The remainder will be in the second commit. If you don't make any changes, then
the operation will be aborted.
"###);

insta::assert_snapshot!(
std::fs::read_to_string(test_env.env_root().join("editor")).unwrap(), @r###"
JJ: Enter a description for the first commit.

JJ: This commit contains the following changes:
JJ: A file1

JJ: Lines starting with "JJ: " (like this one) will be removed.
"###);

insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
First part: qpvuntsm 0e15949e (no description set)
Second part: rlvkpnrz 9ed12e4c (no description set)
Working copy now at: rlvkpnrz 9ed12e4c (no description set)
Parent commit : qpvuntsm 0e15949e (no description set)
"###);
}
16 changes: 16 additions & 0 deletions cli/tests/test_squash_command.rs
Original file line number Diff line number Diff line change
@@ -157,13 +157,29 @@ fn test_squash_partial() {
// If we don't make any changes in the diff-editor, the whole change is moved
// into the parent
let edit_script = test_env.set_up_fake_diff_editor();
std::fs::write(&edit_script, "dump JJ-INSTRUCTIONS instrs").unwrap();
let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["squash", "-r", "b", "-i"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Rebased 1 descendant commits
Working copy now at: mzvwutvl 3c633226 c | (no description set)
Parent commit : qpvuntsm 38ffd8b9 a b | (no description set)
"###);

insta::assert_snapshot!(
std::fs::read_to_string(test_env.env_root().join("instrs")).unwrap(), @r###"
You are moving changes from: kkmpptxz d117da27 b | (no description set)
into commit: qpvuntsm 54d3c1c0 a | (no description set)

The left side of the diff shows the contents of the parent commit. The
right side initially shows the contents of the commit you're moving
changes from.

Adjust the right side until the diff shows the changes you want to move
to the destination. If you don't make any changes, then all the changes
from the source will be moved into the destination.
"###);

insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
@ 3c6332267ea8 c
◉ 38ffd8b98578 a b
15 changes: 15 additions & 0 deletions cli/tests/test_unsquash_command.rs
Original file line number Diff line number Diff line change
@@ -156,13 +156,28 @@ fn test_unsquash_partial() {
// If we don't make any changes in the diff-editor, the whole change is moved
// from the parent
let edit_script = test_env.set_up_fake_diff_editor();
std::fs::write(&edit_script, "dump JJ-INSTRUCTIONS instrs").unwrap();
let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["unsquash", "-r", "b", "-i"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Rebased 1 descendant commits
Working copy now at: mzvwutvl 8802263d c | (no description set)
Parent commit : kkmpptxz 5bd83140 b | (no description set)
"###);

insta::assert_snapshot!(
std::fs::read_to_string(test_env.env_root().join("instrs")).unwrap(), @r###"
You are moving changes from: qpvuntsm 54d3c1c0 a | (no description set)
into its child: kkmpptxz d117da27 b | (no description set)

The diff initially shows the parent commit's changes.

Adjust the right side until it shows the contents you want to keep in
the parent commit. The changes you edited out will be moved into the
child commit. If you don't make any changes, then the operation will be
aborted.
"###);

insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
@ 8802263dbd92 c
◉ 5bd83140fd47 b