Feat: Only abandon empty commits with squash if the user intended it.

If the user was thinking in terms of files, instead of commits, then we shouldn't abandon the commit, as that can have side effects such as moving branch pointers.

Fixes #3815

CHANGELOG.md

@@ -8,6 +8,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## [Unreleased]
 
 ### Breaking changes
+* If `jj squash` receives paths or `--interactive`, then it will not abandon the
+  source commit even if it became empty.
 
 * Dropped support for `ui.default-revset` config (replaced by `revsets.log` in
   0.8.0).
@@ -37,6 +39,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### New features
 
+* `jj squash` now accepts a `--no-abandon` option to keep the source commit.
+
 * `jj branch list`/`tag list` now accept `-T`/`--template` option. The tag list
   prints commit summary along with the tag name by default.
 

cli/src/commands/move.rs

@@ -17,7 +17,7 @@ use jj_lib::object_id::ObjectId;
 use tracing::instrument;
 
 use super::squash::{move_diff, SquashedDescription};
-use crate::cli_util::{CommandHelper, RevisionArg};
+use crate::cli_util::{CommandHelper, DiffSelector, RevisionArg};
 use crate::command_error::{user_error, CommandError};
 use crate::ui::Ui;
 
@@ -89,6 +89,13 @@ pub(crate) fn cmd_move(
         source.id().hex(),
         destination.id().hex()
     );
+
+    // If the user provided --interactive or files, the intent of the user was
+    // clearly to work on files rather than on commits, - it would be strange
+    // if the user wrote jj squash foo.rs and found that the branch pointer had
+    // moved to the parent, for example.
+    let abandon = !matches!(diff_selector, DiffSelector::Interactive(_)) && args.paths.is_empty();
+
     move_diff(
         ui,
         &mut tx,
@@ -97,9 +104,14 @@ pub(crate) fn cmd_move(
         &destination,
         matcher.as_ref(),
         &diff_selector,
-        SquashedDescription::Combine,
+        if abandon {
+            SquashedDescription::Combine
+        } else {
+            SquashedDescription::UseDestination
+        },
         false,
         &args.paths,
+        abandon,
     )?;
     tx.finish(ui, tx_description)?;
     Ok(())

cli/src/commands/squash.rs

@@ -37,13 +37,12 @@ use crate::ui::Ui;
 /// For example, `jj squash --into @--` moves changes from the working-copy
 /// commit to the grandparent.
 ///
-/// If, after moving changes out, the source revision is empty compared to its
-/// parent(s), it will be abandoned. Without `--interactive`, the source
-/// revision will always be empty.
+/// If `--interactive` is not set, no paths are provided, and `--keep`
+/// is not set, the source will be abandoned.
 ///
-/// 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.
+/// If the source was abandoned 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.
 ///
 /// If a working-copy commit gets abandoned, it will be given a new, empty
 /// commit. This is true in general; it is not specific to this command.
@@ -65,15 +64,19 @@ pub(crate) struct SquashArgs {
     /// description(s) of the source revision(s)
     #[arg(long, short, conflicts_with = "message_paragraphs")]
     use_destination_message: bool,
-    /// Interactively choose which parts to squash
+    /// Interactively choose which parts to squash (implies --no-abandon)
     #[arg(long, short)]
     interactive: bool,
     /// Specify diff editor to be used (implies --interactive)
     #[arg(long, value_name = "NAME")]
     tool: Option<String>,
-    /// Move only changes to these paths (instead of all paths)
+    /// Move only changes to these paths instead of all paths (implies
+    /// --no-abandon)
     #[arg(conflicts_with_all = ["interactive", "tool"], value_hint = clap::ValueHint::AnyPath)]
     paths: Vec<String>,
+    /// The source revision will not be abandoned
+    #[arg(long)]
+    no_abandon: bool,
 }
 
 #[instrument(skip_all)]
@@ -119,6 +122,15 @@ pub(crate) fn cmd_squash(
         .to_matcher();
     let diff_selector =
         workspace_command.diff_selector(ui, args.tool.as_deref(), args.interactive)?;
+
+    // If the user provided --interactive or files, the intent of the user was
+    // clearly to work on files rather than on commits, - it would be strange
+    // if the user wrote jj squash foo.rs and found that the branch pointer had
+    // moved to the parent, for example.
+    let abandon = !args.no_abandon
+        && !matches!(diff_selector, DiffSelector::Interactive(_))
+        && args.paths.is_empty();
+
     let mut tx = workspace_command.start_transaction();
     let tx_description = format!("squash commits into {}", destination.id().hex());
     move_diff(
@@ -129,9 +141,10 @@ pub(crate) fn cmd_squash(
         &destination,
         matcher.as_ref(),
         &diff_selector,
-        SquashedDescription::from_args(args),
+        SquashedDescription::from_args(args, abandon),
         args.revision.is_none() && args.from.is_empty() && args.into.is_none(),
         &args.paths,
+        abandon,
     )?;
     tx.finish(ui, tx_description)?;
     Ok(())
@@ -150,14 +163,14 @@ pub(crate) enum SquashedDescription {
 
 // TODO(#2882): Remove public visibility once `jj move` is deleted.
 impl SquashedDescription {
-    pub(crate) fn from_args(args: &SquashArgs) -> Self {
+    pub(crate) fn from_args(args: &SquashArgs, abandon: bool) -> Self {
         // These options are incompatible and Clap is configured to prevent this.
         assert!(args.message_paragraphs.is_empty() || !args.use_destination_message);
 
         if !args.message_paragraphs.is_empty() {
             let desc = join_message_paragraphs(&args.message_paragraphs);
             SquashedDescription::Exact(desc)
-        } else if args.use_destination_message {
+        } else if args.use_destination_message || !abandon {
             SquashedDescription::UseDestination
         } else {
             SquashedDescription::Combine
@@ -177,6 +190,7 @@ pub fn move_diff(
     description: SquashedDescription,
     no_rev_arg: bool,
     path_arg: &[String],
+    abandon: bool,
 ) -> Result<(), CommandError> {
     tx.base_workspace_helper()
         .check_rewritable(sources.iter().chain(std::iter::once(destination)).ids())?;
@@ -185,7 +199,6 @@ pub fn move_diff(
         commit: &'a Commit,
         parent_tree: MergedTree,
         selected_tree: MergedTree,
-        abandon: bool,
     }
     let mut source_commits = vec![];
     for source in sources {
@@ -210,7 +223,6 @@ from the source will be moved into the destination.
         let selected_tree_id =
             diff_selector.select(&parent_tree, &source_tree, matcher, Some(&instructions))?;
         let selected_tree = tx.repo().store().get_root_tree(&selected_tree_id)?;
-        let abandon = selected_tree.id() == source_tree.id();
         if !abandon && selected_tree_id == parent_tree.id() {
             // Nothing selected from this commit. If it's abandoned (i.e. already empty), we
             // still include it so `jj squash` can be used for abandoning an empty commit in
@@ -223,7 +235,6 @@ from the source will be moved into the destination.
             commit: source,
             parent_tree,
             selected_tree,
-            abandon,
         });
     }
     if source_commits.is_empty() {
@@ -250,7 +261,7 @@ from the source will be moved into the destination.
     }
 
     for source in &source_commits {
-        if source.abandon {
+        if abandon {
             tx.mut_repo()
                 .record_abandoned_commit(source.commit.id().clone());
         } else {
@@ -285,13 +296,12 @@ from the source will be moved into the destination.
     let description = match description {
         SquashedDescription::Exact(description) => description,
         SquashedDescription::UseDestination => destination.description().to_owned(),
-        SquashedDescription::Combine => {
-            let abandoned_commits = source_commits
-                .iter()
-                .filter_map(|source| source.abandon.then_some(source.commit))
-                .collect_vec();
-            combine_messages(tx.base_repo(), &abandoned_commits, destination, settings)?
-        }
+        SquashedDescription::Combine => combine_messages(
+            tx.base_repo(),
+            &sources.into_iter().collect_vec(),
+            destination,
+            settings,
+        )?,
     };
     let mut predecessors = vec![destination.id().clone()];
     predecessors.extend(

cli/tests/cli-reference@.md.snap

@@ -1781,17 +1781,17 @@ With the `-r` option, moves the changes from the specified revision to the paren
 
 With the `--from` and/or `--into` options, moves changes from/to the given revisions. If either is left out, it defaults to the working-copy commit. For example, `jj squash --into @--` moves changes from the working-copy commit to the grandparent.
 
-If, after moving changes out, the source revision is empty compared to its parent(s), it will be abandoned. Without `--interactive`, the source revision will always be empty.
+If `--interactive` is not set, no paths are provided, and `--keep` is not set, the source will be abandoned.
 
-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.
+If the source was abandoned 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.
 
 If a working-copy commit gets abandoned, it will be given a new, empty commit. This is true in general; it is not specific to this command.
 
 **Usage:** `jj squash [OPTIONS] [PATHS]...`
 
 ###### **Arguments:**
 
-* `<PATHS>` — Move only changes to these paths (instead of all paths)
+* `<PATHS>` — Move only changes to these paths instead of all paths (implies --no-abandon)
 
 ###### **Options:**
 
@@ -1803,11 +1803,15 @@ If a working-copy commit gets abandoned, it will be given a new, empty commit. T
 
   Possible values: `true`, `false`
 
-* `-i`, `--interactive` — Interactively choose which parts to squash
+* `-i`, `--interactive` — Interactively choose which parts to squash (implies --no-abandon)
 
   Possible values: `true`, `false`
 
 * `--tool <NAME>` — Specify diff editor to be used (implies --interactive)
+* `--no-abandon` — The source revision will not be abandoned
+
+  Possible values: `true`, `false`
+
 
 
 

cli/tests/test_move_command.rs

@@ -221,7 +221,8 @@ fn test_move_partial() {
     "###);
     insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
     @  71b69e433fbc d
-    │ ◉  55171e33db26 b c
+    │ ◉  9f21d0f664f3 c
+    │ ◉  55171e33db26 b
     ├─╯
     ◉  3db0a2f5b535 a
     ◉  000000000000

cli/tests/test_squash_command.rs

@@ -160,13 +160,14 @@ fn test_squash_partial() {
     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 f03d5ce4 c | (no description set)
-    Parent commit      : qpvuntsm c9f931cd a b | (no description set)
+    Rebased 2 descendant commits
+    Working copy now at: mzvwutvl 1fc0b9c1 c | (no description set)
+    Parent commit      : kkmpptxz a54b88b5 b | (empty) (no description set)
     "###);
     insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
-    @  f03d5ce4a973 c
-    ◉  c9f931cd78af a b
+    @  1fc0b9c19783 c
+    ◉  a54b88b5e186 b
+    ◉  c9f931cd78af a
     ◉  000000000000
     "###);
     let stdout = test_env.jj_cmd_success(&repo_path, &["print", "file1", "-r", "a"]);
@@ -445,7 +446,8 @@ fn test_squash_from_to_partial() {
     "###);
     insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
     @  71b69e433fbc d
-    │ ◉  55171e33db26 b c
+    │ ◉  9f21d0f664f3 c
+    │ ◉  55171e33db26 b
     ├─╯
     ◉  3db0a2f5b535 a
     ◉  000000000000
@@ -885,10 +887,8 @@ fn test_squash_from_multiple_partial_no_op() {
     "###);
 
     // Source commits that didn't match the paths are not rewritten
-    let (stdout, stderr) = test_env.jj_cmd_ok(
-        &repo_path,
-        &["squash", "--from=@-+ ~ @", "--into=@", "-m=d", "b"],
-    );
+    let (stdout, stderr) =
+        test_env.jj_cmd_ok(&repo_path, &["squash", "--from=@-+ ~ @", "--into=@", "b"]);
     insta::assert_snapshot!(stdout, @"");
     insta::assert_snapshot!(stderr, @r###"
     Working copy now at: mzvwutvl 9227d0d7 d
@@ -897,6 +897,8 @@ fn test_squash_from_multiple_partial_no_op() {
     "###);
     insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
     @  9227d0d780fa d
+    │ ◉  8a4c9234c310 b
+    ├─╯
     │ ◉  5ad3ca4090a7 c
     ├─╯
     ◉  3df52ee1f8a9 a