From 8ae9540f2cc728f857eddec25939dc42ddd771a9 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sun, 29 May 2022 17:18:56 -0700 Subject: [PATCH] cli: make `move/squash/unsquash` keep empty working-copy commit If the source commit becomes empty as a result of `move/squash/unsquash`, we abandon it. However, perhaps we shouldn't do that if the source commit is a working-copy commit because working-copy commits are often work-in-progress commits. The background for this change is that @arxanas had just started a new change and had set a description on it, and then decided to make some changes in the working copy that should be in the parent commit. Running `jj squash` then abandoned the working-copy commit, resuling in the description getting lost. --- CHANGELOG.md | 3 +++ lib/src/view.rs | 6 ++++++ src/commands.rs | 22 +++++++++++++++------- tests/test_edit_command.rs | 2 +- tests/test_squash_command.rs | 9 +++++---- tests/test_workspaces.rs | 10 +++++----- 6 files changed, 35 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index edd1f9dc2..197976e2d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -142,6 +142,9 @@ 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). + ## [0.4.0] - 2022-04-02 ### Breaking changes diff --git a/lib/src/view.rs b/lib/src/view.rs index 6f3a67f5f..baa3615df 100644 --- a/lib/src/view.rs +++ b/lib/src/view.rs @@ -14,6 +14,8 @@ use std::collections::{BTreeMap, HashMap, HashSet}; +use itertools::Itertools; + use crate::backend::CommitId; use crate::index::IndexRef; use crate::op_store; @@ -48,6 +50,10 @@ impl View { self.data.checkouts.get(workspace_id) } + pub fn is_checkout(&self, commit_id: &CommitId) -> bool { + self.data.checkouts.values().contains(commit_id) + } + pub fn heads(&self) -> &HashSet { &self.data.head_ids } diff --git a/src/commands.rs b/src/commands.rs index 9676d9b42..8fa8d957c 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -1400,7 +1400,8 @@ 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. +/// now empty compared to its parent, it will be abandoned unless it's checked +/// out. Without `--interactive`, the source change will always be empty. #[derive(clap::Args, Clone, Debug)] #[clap(group(ArgGroup::new("to_move").args(&["from", "to"]).multiple(true).required(true)))] struct MoveArgs { @@ -1422,8 +1423,8 @@ 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. This will always be the case -/// without `--interactive`. +/// compared to its parent, it will be abandoned unless it's checked out. +/// Without `--interactive`, the child change will always be empty. #[derive(clap::Args, Clone, Debug)] #[clap(visible_alias = "amend")] struct SquashArgs { @@ -1438,6 +1439,12 @@ struct SquashArgs { } /// Move changes from a revision's parent into the revision +/// +/// 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. #[derive(clap::Args, Clone, Debug)] #[clap(visible_alias = "unamend")] struct UnsquashArgs { @@ -1445,7 +1452,7 @@ struct UnsquashArgs { revision: String, /// Interactively choose which parts to unsquash // TODO: It doesn't make much sense to run this without -i. We should make that - // the default. We should also abandon the parent commit if that becomes empty. + // the default. #[clap(long, short)] interactive: bool, } @@ -3423,7 +3430,7 @@ 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() { + if new_source_tree_id == *parent_tree.id() && !repo.view().is_checkout(source.id()) { mut_repo.record_abandoned_commit(source.id().clone()); } else { CommitBuilder::for_rewrite_from(ui.settings(), repo.store(), &source) @@ -3495,7 +3502,8 @@ 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(); + let abandon_child = + &new_parent_tree_id == commit.tree_id() && !repo.view().is_checkout(commit.id()); let new_parent = CommitBuilder::for_rewrite_from(ui.settings(), repo.store(), parent) .set_tree(new_parent_tree_id) .set_predecessors(vec![parent.id().clone(), commit.id().clone()]) @@ -3560,7 +3568,7 @@ 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() { + if &new_parent_tree_id == parent_base_tree.id() && !repo.view().is_checkout(parent.id()) { mut_repo.record_abandoned_commit(parent.id().clone()); // Commit the new child on top of the parent's parents. CommitBuilder::for_rewrite_from(ui.settings(), repo.store(), &commit) diff --git a/tests/test_edit_command.rs b/tests/test_edit_command.rs index 295bdf75d..2e4d82e4e 100644 --- a/tests/test_edit_command.rs +++ b/tests/test_edit_command.rs @@ -130,7 +130,7 @@ fn test_edit_merge() { insta::assert_snapshot!(stdout, @r###" Created 608f32ad9e19 merge Rebased 1 descendant commits - Working copy now at: a791bdbda05c (no description set) + Working copy now at: 2eca803962db (no description set) Added 0 files, modified 0 files, removed 1 files "###); let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s", "-r", "@-"]); diff --git a/tests/test_squash_command.rs b/tests/test_squash_command.rs index 59a37fa19..9d01ed415 100644 --- a/tests/test_squash_command.rs +++ b/tests/test_squash_command.rs @@ -43,10 +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: 6ca29c9d2e7c (no description set) + Working copy now at: 5f56e6899bce (no description set) "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - @ 6ca29c9d2e7c b c + @ 5f56e6899bce c + o 6ca29c9d2e7c b o 90aeefd03044 a o 000000000000 "###); @@ -105,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: 626d78245205 (no description set) + Working copy now at: b4e9307669d0 (no description set) "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - @ 626d78245205 + @ b4e9307669d0 o 2a25465aba5f e |\ o | 9a18f8da1e69 d diff --git a/tests/test_workspaces.rs b/tests/test_workspaces.rs index 07ce3da52..cecf6d407 100644 --- a/tests/test_workspaces.rs +++ b/tests/test_workspaces.rs @@ -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: 86bef7fee095 (no description set) + Working copy now at: 6d004761e813 (no description set) "###); // The secondary workspace's checkout was updated insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r###" - @ 86bef7fee095bb5626d853c222764fc7c9fb88ac default@ - | o 8d8269a323a01a287236c4fd5f64dc9737febb5b secondary@ + o 8d8269a323a01a287236c4fd5f64dc9737febb5b secondary@ + | @ 6d004761e81306cf8b2168a18868fbc84f182556 default@ |/ 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###" - o 86bef7fee095bb5626d853c222764fc7c9fb88ac default@ - | @ 8d8269a323a01a287236c4fd5f64dc9737febb5b secondary@ + @ 8d8269a323a01a287236c4fd5f64dc9737febb5b secondary@ + | o 6d004761e81306cf8b2168a18868fbc84f182556 default@ |/ o 52601f748bf6cb00ad5389922f530f20a7ecffaa o 0000000000000000000000000000000000000000