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

@@ -37,6 +37,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### New features
 
+* `jj squash` now accepts a `--keep-empty` option to keep empty commits.
+  This option is implied by `--interactive` and specifying paths.
+
 * `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

@@ -100,6 +100,7 @@ pub(crate) fn cmd_move(
         SquashedDescription::Combine,
         false,
         &args.paths,
+        false,
     )?;
     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-empty`
+/// 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 --keep-empty)
     #[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
+    /// --keep-empty)
     #[arg(conflicts_with_all = ["interactive", "tool"], value_hint = clap::ValueHint::AnyPath)]
     paths: Vec<String>,
+    /// If true, then empty revisions will be kept.
+    #[arg(long)]
+    keep_empty: 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 keep_empty = args.keep_empty
+        || 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(
@@ -132,6 +144,7 @@ pub(crate) fn cmd_squash(
         SquashedDescription::from_args(args),
         args.revision.is_none() && args.from.is_empty() && args.into.is_none(),
         &args.paths,
+        keep_empty,
     )?;
     tx.finish(ui, tx_description)?;
     Ok(())
@@ -177,6 +190,7 @@ pub fn move_diff(
     description: SquashedDescription,
     no_rev_arg: bool,
     path_arg: &[String],
+    keep_empty: bool,
 ) -> Result<(), CommandError> {
     tx.base_workspace_helper()
         .check_rewritable(sources.iter().chain(std::iter::once(destination)).ids())?;
@@ -210,7 +224,7 @@ 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();
+        let abandon = selected_tree.id() == source_tree.id() && !keep_empty;
         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

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-empty` 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 --keep-empty)
 
 ###### **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 --keep-empty)
 
   Possible values: `true`, `false`
 
 * `--tool <NAME>` — Specify diff editor to be used (implies --interactive)
+* `--keep-empty` — If true, then empty revisions will be kept
+
+  Possible values: `true`, `false`
+
 
 
 

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