ok/jj
1
0
Fork 0
forked from mirrors/jj
Code Pull requests Activity

cli: make jj move/squash/unsquash ask for combined description

Browse source 
In 8ae9540f2c, I made `jj move/squash/unsquash` not abandon the
working copy if it became empty because that would lose any
description associated with it. It turned out that the new behavior
was also confusing because it made it unclear if the working-copy
commit was actually abandoned. Let's roll back that change and instead
ask the user for a combined description when both the source and
destination commits have non-empty descriptions. Not discarding a
non-empty description seems like a good improvement regardless of the
behavior related to working-copy commits. It's also how `hg fold`
behaves (though hg doesn't allow the description to be empty).
This commit is contained in:
Martin von Zweigbergk 2022-08-28 10:28:53 -07:00 committed by Martin von Zweigbergk
parent 478dfd74fc
commit b5378caa81
9 changed files with 330 additions and 35 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

6
CHANGELOG.md
View file

@ -184,8 +184,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* When checking out a commit, the previous commit is no longer abandoned if it
has a non-empty description.
* When using `jj move/squash/unsquash` results in the source commit becoming
empty, it is no longer abandoned if it is checked out (in any workspace).
* When `jj move/squash/unsquash` abandons the source commit and both the source
and the destination commits have non-empty descriptions, it now asks for a
combined description. If either description was empty, it uses the other
without asking.
* All commands now consistently snapshot the working copy (it was missing from
e.g. `jj undo` and `jj merge` before).

72
src/commands.rs
View file

@ -1413,8 +1413,12 @@ struct NewArgs {
/// destination. The selected changes (or all the changes in the source revision
/// if not using `--interactive`) will be moved into the destination. The
/// changes will be removed from the source. If that means that the source is
/// now empty compared to its parent, it will be abandoned unless it's checked
/// out. Without `--interactive`, the source change will always be empty.
/// now empty compared to its parent, it will be abandoned. Without
/// `--interactive`, the source change will always be empty.
///
/// If the source became empty and both the source and destination had a
/// non-empty description, you will be asked for the combined description. If
/// either was empty, then the other one will be used.
#[derive(clap::Args, Clone, Debug)]
#[clap(group(ArgGroup::new("to_move").args(&["from", "to"]).multiple(true).required(true)))]
struct MoveArgs {
@ -1436,8 +1440,12 @@ struct MoveArgs {
///
/// After moving the changes into the parent, the child revision will have the
/// same content state as before. If that means that the change is now empty
/// compared to its parent, it will be abandoned unless it's checked out.
/// compared to its parent, it will be abandoned.
/// Without `--interactive`, the child change will always be empty.
///
/// If the source became empty and both the source and destination had a
/// non-empty description, you will be asked for the combined description. If
/// either was empty, then the other one will be used.
#[derive(clap::Args, Clone, Debug)]
#[clap(visible_alias = "amend")]
struct SquashArgs {
@ -1455,9 +1463,12 @@ struct SquashArgs {
///
/// After moving the changes out of the parent, the child revision will have the
/// same content state as before. If moving the change out of the parent change
/// made it empty compared to its parent, it will be abandoned unless it's
/// checked out. Without `--interactive`, the parent change will always become
/// empty.
/// made it empty compared to its parent, it will be abandoned. Without
/// `--interactive`, the parent change will always become empty.
///
/// If the source became empty and both the source and destination had a
/// non-empty description, you will be asked for the combined description. If
/// either was empty, then the other one will be used.
#[derive(clap::Args, Clone, Debug)]
#[clap(visible_alias = "unamend")]
struct UnsquashArgs {
@ -3482,6 +3493,32 @@ fn cmd_new(ui: &mut Ui, command: &CommandHelper, args: &NewArgs) -> Result<(), C
Ok(())
}
fn combine_messages(
ui: &Ui,
repo: &ReadonlyRepo,
source: &Commit,
destination: &Commit,
abandon_source: bool,
) -> Result<String, CommandError> {
let description = if abandon_source {
if source.description().is_empty() {
destination.description().to_string()
} else if destination.description().is_empty() {
source.description().to_string()
} else {
let combined = "JJ: Enter a description for the combined commit.\n".to_string()
+ "JJ: Description from the destination commit:\n"
+ destination.description()
+ "\nJJ: Description from the source commit:\n"
+ source.description();
edit_description(ui, repo, &combined)?
}
} else {
destination.description().to_string()
};
Ok(description)
}
fn cmd_move(ui: &mut Ui, command: &CommandHelper, args: &MoveArgs) -> Result<(), CommandError> {
let mut workspace_command = command.workspace_helper(ui)?;
let source = workspace_command.resolve_single_rev(args.from.as_deref().unwrap_or("@"))?;
@ -3536,7 +3573,8 @@ from the source will be moved into the destination.
.get_tree(&RepoPath::root(), &new_parent_tree_id)?;
// Apply the reverse of the selected changes onto the source
let new_source_tree_id = merge_trees(&source_tree, &new_parent_tree, &parent_tree)?;
if new_source_tree_id == *parent_tree.id() && !repo.view().is_checkout(source.id()) {
let abandon_source = new_source_tree_id == *parent_tree.id();
if abandon_source {
mut_repo.record_abandoned_commit(source.id().clone());
} else {
CommitBuilder::for_rewrite_from(ui.settings(), &source)
@ -3555,8 +3593,16 @@ from the source will be moved into the destination.
}
// Apply the selected changes onto the destination
let new_destination_tree_id = merge_trees(&destination.tree(), &parent_tree, &new_parent_tree)?;
let description = combine_messages(
ui,
workspace_command.repo(),
&source,
&destination,
abandon_source,
)?;
CommitBuilder::for_rewrite_from(ui.settings(), &destination)
.set_tree(new_destination_tree_id)
.set_description(description)
.write_to_repo(mut_repo);
workspace_command.finish_transaction(ui, tx)?;
Ok(())
@ -3606,12 +3652,14 @@ from the source will be moved into the parent.
}
// Abandon the child if the parent now has all the content from the child
// (always the case in the non-interactive case).
let abandon_child =
&new_parent_tree_id == commit.tree_id() && !tx.base_repo().view().is_checkout(commit.id());
let abandon_child = &new_parent_tree_id == commit.tree_id();
let mut_repo = tx.mut_repo();
let description =
combine_messages(ui, workspace_command.repo(), &commit, parent, abandon_child)?;
let new_parent = CommitBuilder::for_rewrite_from(ui.settings(), parent)
.set_tree(new_parent_tree_id)
.set_predecessors(vec![parent.id().clone(), commit.id().clone()])
.set_description(description)
.write_to_repo(mut_repo);
if abandon_child {
mut_repo.record_abandoned_commit(commit.id().clone());
@ -3672,13 +3720,13 @@ aborted.
}
// Abandon the parent if it is now empty (always the case in the non-interactive
// case).
if &new_parent_tree_id == parent_base_tree.id()
&& !tx.base_repo().view().is_checkout(parent.id())
{
if &new_parent_tree_id == parent_base_tree.id() {
tx.mut_repo().record_abandoned_commit(parent.id().clone());
let description = combine_messages(ui, workspace_command.repo(), parent, &commit, true)?;
// Commit the new child on top of the parent's parents.
CommitBuilder::for_rewrite_from(ui.settings(), &commit)
.set_parents(parent.parent_ids())
.set_description(description)
.write_to_repo(tx.mut_repo());
} else {
let new_parent = CommitBuilder::for_rewrite_from(ui.settings(), parent)

11
tests/test_describe_command.rs
View file

@ -30,11 +30,16 @@ fn test_describe() {
Working copy now at: 7e0db3b0ad17 description from CLI
"###);
// Check that the text file gets initialized with the current description and make no changes
std::fs::write(&edit_script, r#"expect
// Check that the text file gets initialized with the current description and
// make no changes
std::fs::write(
&edit_script,
r#"expect
description from CLI
JJ: Lines starting with "JJ: " (like this one) will be removed.
"#).unwrap();
"#,
)
.unwrap();
let stdout = test_env.jj_cmd_success(&repo_path, &["describe"]);
insta::assert_snapshot!(stdout, @r###"
Working copy now at: 45bfa10db64d description from CLI

80
tests/test_move_command.rs
View file

@ -315,3 +315,83 @@ fn test_move_partial() {
fn get_log_output(test_env: &TestEnvironment, cwd: &Path) -> String {
test_env.jj_cmd_success(cwd, &["log", "-T", r#"commit_id.short() " " branches"#])
}
#[test]
fn test_move_description() {
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");
let edit_script = test_env.set_up_fake_editor();
std::fs::write(&edit_script, r#""#).unwrap();
// If both descriptions are empty, the resulting description is empty
std::fs::write(repo_path.join("file1"), "a\n").unwrap();
std::fs::write(repo_path.join("file2"), "a\n").unwrap();
test_env.jj_cmd_success(&repo_path, &["new"]);
std::fs::write(repo_path.join("file1"), "b\n").unwrap();
std::fs::write(repo_path.join("file2"), "b\n").unwrap();
test_env.jj_cmd_success(&repo_path, &["move", "--to", "@-"]);
insta::assert_snapshot!(get_description(&test_env, &repo_path, "@-"), @r###"
(no description set)
"###);
// If the destination's description is empty and the source's description is
// non-empty, the resulting description is from the source
test_env.jj_cmd_success(&repo_path, &["undo"]);
test_env.jj_cmd_success(&repo_path, &["describe", "-m", "source"]);
test_env.jj_cmd_success(&repo_path, &["move", "--to", "@-"]);
insta::assert_snapshot!(get_description(&test_env, &repo_path, "@-"), @r###"
source
"###);
// If the destination's description is non-empty and the source's description is
// empty, the resulting description is from the destination
test_env.jj_cmd_success(&repo_path, &["undo"]);
test_env.jj_cmd_success(&repo_path, &["describe", "@-", "-m", "destination"]);
test_env.jj_cmd_success(&repo_path, &["move", "--to", "@-"]);
insta::assert_snapshot!(get_description(&test_env, &repo_path, "@-"), @r###"
destination
source
"###);
// If both descriptions were non-empty, we get asked for a combined description
test_env.jj_cmd_success(&repo_path, &["undo"]);
test_env.jj_cmd_success(&repo_path, &["describe", "-m", "source"]);
std::fs::write(
&edit_script,
r#"expect
JJ: Enter a description for the combined commit.
JJ: Description from the destination commit:
destination
JJ: Description from the source commit:
source
JJ: Lines starting with "JJ: " (like this one) will be removed.
"#,
)
.unwrap();
test_env.jj_cmd_success(&repo_path, &["move", "--to", "@-"]);
insta::assert_snapshot!(get_description(&test_env, &repo_path, "@-"), @r###"
destination
source
"###);
// If the source's *content* doesn't become empty, then the source remains and
// both descriptions are unchanged
test_env.jj_cmd_success(&repo_path, &["undo"]);
std::fs::write(repo_path.join("file2"), "b\n").unwrap();
test_env.jj_cmd_success(&repo_path, &["move", "--to", "@-", "file1"]);
insta::assert_snapshot!(get_description(&test_env, &repo_path, "@-"), @r###"
destination
"###);
insta::assert_snapshot!(get_description(&test_env, &repo_path, "@"), @r###"
source
"###);
}
fn get_description(test_env: &TestEnvironment, repo_path: &Path, rev: &str) -> String {
test_env.jj_cmd_success(
repo_path,
&["log", "--no-graph", "-T", "description", "-r", rev],
)
}

9
tests/test_obslog_command.rs
View file

@ -123,7 +123,7 @@ fn test_obslog_with_or_without_diff() {
#[test]
fn test_obslog_squash() {
let test_env = TestEnvironment::default();
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");
@ -131,12 +131,15 @@ fn test_obslog_squash() {
std::fs::write(repo_path.join("file1"), "foo\n").unwrap();
test_env.jj_cmd_success(&repo_path, &["new", "-m", "second"]);
std::fs::write(repo_path.join("file1"), "foo\nbar\n").unwrap();
let edit_script = test_env.set_up_fake_editor();
std::fs::write(&edit_script, "write\nsquashed").unwrap();
test_env.jj_cmd_success(&repo_path, &["squash"]);
let stdout = get_log_output(&test_env, &repo_path, &["obslog", "-p", "-r", "@-"]);
insta::assert_snapshot!(stdout, @r###"
o c36a0819516d test.user@example.com 2001-02-03 04:05:07.000 +07:00
|\ first
o 9b6d4a272a6a test.user@example.com 2001-02-03 04:05:07.000 +07:00
|\ squashed
| | Modified regular file file1:
| | 1 1: foo
| | 2: bar

89
tests/test_squash_command.rs
View file

@ -43,11 +43,11 @@ fn test_squash() {
// Squashes the working copy into the parent by default
let stdout = test_env.jj_cmd_success(&repo_path, &["squash"]);
insta::assert_snapshot!(stdout, @r###"
Working copy now at: 5f56e6899bce (no description set)
Working copy now at: b9280a9898cb (no description set)
"###);
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
@ 5f56e6899bce c
o 6ca29c9d2e7c b
@ b9280a9898cb
o 6ca29c9d2e7c b c
o 90aeefd03044 a
o 000000000000
"###);
@ -106,10 +106,10 @@ fn test_squash() {
std::fs::write(repo_path.join("file1"), "e\n").unwrap();
let stdout = test_env.jj_cmd_success(&repo_path, &["squash"]);
insta::assert_snapshot!(stdout, @r###"
Working copy now at: e14f5bbc3213 (no description set)
Working copy now at: 5cd0d6858fd0 (no description set)
"###);
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
@ e14f5bbc3213
@ 5cd0d6858fd0
o 5301447e4764 e
|\
o | 5658521e0f8b d
@ -239,3 +239,82 @@ fn get_log_output(test_env: &TestEnvironment, repo_path: &Path) -> String {
&["log", "-T", r#"commit_id.short() " " branches"#],
)
}
#[test]
fn test_squash_description() {
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");
let edit_script = test_env.set_up_fake_editor();
std::fs::write(&edit_script, r#""#).unwrap();
// If both descriptions are empty, the resulting description is empty
std::fs::write(repo_path.join("file1"), "a\n").unwrap();
std::fs::write(repo_path.join("file2"), "a\n").unwrap();
test_env.jj_cmd_success(&repo_path, &["new"]);
std::fs::write(repo_path.join("file1"), "b\n").unwrap();
std::fs::write(repo_path.join("file2"), "b\n").unwrap();
test_env.jj_cmd_success(&repo_path, &["squash"]);
insta::assert_snapshot!(get_description(&test_env, &repo_path, "@-"), @r###"
(no description set)
"###);
// If the destination's description is empty and the source's description is
// non-empty, the resulting description is from the source
test_env.jj_cmd_success(&repo_path, &["undo"]);
test_env.jj_cmd_success(&repo_path, &["describe", "-m", "source"]);
test_env.jj_cmd_success(&repo_path, &["squash"]);
insta::assert_snapshot!(get_description(&test_env, &repo_path, "@-"), @r###"
source
"###);
// If the destination description is non-empty and the source's description is
// empty, the resulting description is from the destination
test_env.jj_cmd_success(&repo_path, &["undo"]);
test_env.jj_cmd_success(&repo_path, &["describe", "@-", "-m", "destination"]);
test_env.jj_cmd_success(&repo_path, &["squash"]);
insta::assert_snapshot!(get_description(&test_env, &repo_path, "@-"), @r###"
destination
source
"###);
// If both descriptions were non-empty, we get asked for a combined description
test_env.jj_cmd_success(&repo_path, &["undo"]);
test_env.jj_cmd_success(&repo_path, &["describe", "-m", "source"]);
std::fs::write(
&edit_script,
r#"expect
JJ: Enter a description for the combined commit.
JJ: Description from the destination commit:
destination
JJ: Description from the source commit:
source
JJ: Lines starting with "JJ: " (like this one) will be removed.
"#,
)
.unwrap();
test_env.jj_cmd_success(&repo_path, &["squash"]);
insta::assert_snapshot!(get_description(&test_env, &repo_path, "@-"), @r###"
destination
source
"###);
// If the source's *content* doesn't become empty, then the source remains and
// both descriptions are unchanged
test_env.jj_cmd_success(&repo_path, &["undo"]);
test_env.jj_cmd_success(&repo_path, &["squash", "file1"]);
insta::assert_snapshot!(get_description(&test_env, &repo_path, "@-"), @r###"
destination
"###);
insta::assert_snapshot!(get_description(&test_env, &repo_path, "@"), @r###"
source
"###);
}
fn get_description(test_env: &TestEnvironment, repo_path: &Path, rev: &str) -> String {
test_env.jj_cmd_success(
repo_path,
&["log", "--no-graph", "-T", "description", "-r", rev],
)
}

2
tests/test_touchup_command.rs
View file

@ -130,7 +130,7 @@ fn test_edit_merge() {
insta::assert_snapshot!(stdout, @r###"
Created 9fed260d03e9 merge
Rebased 1 descendant commits
Working copy now at: e80e55af699c (no description set)
Working copy now at: b6c2fd3364eb (no description set)
Added 0 files, modified 0 files, removed 1 files
"###);
let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s", "-r", "@-"]);

82
tests/test_unsquash_command.rs
View file

@ -105,10 +105,10 @@ fn test_unsquash() {
std::fs::write(repo_path.join("file1"), "e\n").unwrap();
let stdout = test_env.jj_cmd_success(&repo_path, &["unsquash"]);
insta::assert_snapshot!(stdout, @r###"
Working copy now at: 60ac673b534b (no description set)
Working copy now at: 0aabd9784f4d merge
"###);
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
@ 60ac673b534b
@ 0aabd9784f4d
|\
o | 5658521e0f8b d e?
| o 90fe0a96fc90 c e?
@ -205,3 +205,81 @@ fn get_log_output(test_env: &TestEnvironment, repo_path: &Path) -> String {
&["log", "-T", r#"commit_id.short() " " branches"#],
)
}
#[test]
fn test_unsquash_description() {
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");
let edit_script = test_env.set_up_fake_editor();
std::fs::write(&edit_script, r#""#).unwrap();
// If both descriptions are empty, the resulting description is empty
std::fs::write(repo_path.join("file1"), "a\n").unwrap();
std::fs::write(repo_path.join("file2"), "a\n").unwrap();
test_env.jj_cmd_success(&repo_path, &["new"]);
std::fs::write(repo_path.join("file1"), "b\n").unwrap();
std::fs::write(repo_path.join("file2"), "b\n").unwrap();
test_env.jj_cmd_success(&repo_path, &["unsquash"]);
insta::assert_snapshot!(get_description(&test_env, &repo_path, "@"), @r###"
(no description set)
"###);
// If the destination's description is empty and the source's description is
// non-empty, the resulting description is from the source
test_env.jj_cmd_success(&repo_path, &["undo"]);
test_env.jj_cmd_success(&repo_path, &["describe", "@-", "-m", "source"]);
test_env.jj_cmd_success(&repo_path, &["unsquash"]);
insta::assert_snapshot!(get_description(&test_env, &repo_path, "@"), @r###"
source
"###);
// If the destination description is non-empty and the source's description is
// empty, the resulting description is from the destination
test_env.jj_cmd_success(&repo_path, &["undo"]);
test_env.jj_cmd_success(&repo_path, &["describe", "-m", "destination"]);
test_env.jj_cmd_success(&repo_path, &["unsquash"]);
insta::assert_snapshot!(get_description(&test_env, &repo_path, "@"), @r###"
destination
source
"###);
// If both descriptions were non-empty, we get asked for a combined description
test_env.jj_cmd_success(&repo_path, &["undo"]);
test_env.jj_cmd_success(&repo_path, &["describe", "@-", "-m", "source"]);
std::fs::write(
&edit_script,
r#"expect
JJ: Enter a description for the combined commit.
JJ: Description from the destination commit:
destination
JJ: Description from the source commit:
source
JJ: Lines starting with "JJ: " (like this one) will be removed.
"#,
)
.unwrap();
test_env.jj_cmd_success(&repo_path, &["unsquash"]);
insta::assert_snapshot!(get_description(&test_env, &repo_path, "@"), @r###"
destination
source
"###);
// If the source's *content* doesn't become empty, then the source remains and
// both descriptions are unchanged
test_env.jj_cmd_success(&repo_path, &["undo"]);
insta::assert_snapshot!(get_description(&test_env, &repo_path, "@-"), @r###"
source
"###);
insta::assert_snapshot!(get_description(&test_env, &repo_path, "@"), @r###"
destination
"###);
}
fn get_description(test_env: &TestEnvironment, repo_path: &Path, rev: &str) -> String {
test_env.jj_cmd_success(
repo_path,
&["log", "--no-graph", "-T", "description", "-r", rev],
)
}

14
tests/test_workspaces.rs
View file

@ -101,13 +101,13 @@ fn test_workspaces_conflicting_edits() {
let stdout = test_env.jj_cmd_success(&main_path, &["squash"]);
insta::assert_snapshot!(stdout, @r###"
Rebased 1 descendant commits
Working copy now at: 6d004761e813 (no description set)
Working copy now at: 86bef7fee095 (no description set)
"###);
// The secondary workspace's checkout was updated
insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r###"
o 8d8269a323a01a287236c4fd5f64dc9737febb5b secondary@
| @ 6d004761e81306cf8b2168a18868fbc84f182556 default@
@ 86bef7fee095bb5626d853c222764fc7c9fb88ac default@
| o 8d8269a323a01a287236c4fd5f64dc9737febb5b secondary@
|/
o 52601f748bf6cb00ad5389922f530f20a7ecffaa
o 0000000000000000000000000000000000000000
@ -118,8 +118,8 @@ fn test_workspaces_conflicting_edits() {
// have been committed first (causing divergence)
assert!(stdout.starts_with("The working copy is stale"));
insta::assert_snapshot!(stdout.lines().skip(1).join("\n"), @r###"
@ 8d8269a323a01a287236c4fd5f64dc9737febb5b secondary@
| o 6d004761e81306cf8b2168a18868fbc84f182556 default@
o 86bef7fee095bb5626d853c222764fc7c9fb88ac default@
| @ 8d8269a323a01a287236c4fd5f64dc9737febb5b secondary@
|/
o 52601f748bf6cb00ad5389922f530f20a7ecffaa
o 0000000000000000000000000000000000000000
@ -129,8 +129,8 @@ fn test_workspaces_conflicting_edits() {
let stdout = get_log_output(&test_env, &secondary_path);
assert!(!stdout.starts_with("The working copy is stale"));
insta::assert_snapshot!(stdout, @r###"
@ 8d8269a323a01a287236c4fd5f64dc9737febb5b secondary@
| o 6d004761e81306cf8b2168a18868fbc84f182556 default@
o 86bef7fee095bb5626d853c222764fc7c9fb88ac default@
| @ 8d8269a323a01a287236c4fd5f64dc9737febb5b secondary@
|/
o 52601f748bf6cb00ad5389922f530f20a7ecffaa
o 0000000000000000000000000000000000000000