squash: don't rewrite commits that didn't change

Closes #3334

CHANGELOG.md

@@ -84,6 +84,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   literals. This means that `snapshot.max-new-file-size="1"` and
   `snapshot.max-new-file-size=1` are now equivalent.
 
+* `jj squash <path>` is now a no-op if the path argument didn't match any paths
+  (it used to create new commits with bumped timestamp).
+  [#3334](https://github.com/martinvonz/jj/issues/3334)
+
 ## [0.16.0] - 2024-04-03
 
 ### Deprecations

cli/src/commands/squash.rs

@@ -213,6 +213,11 @@ from the source will be moved into the destination.
             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 was already empty, we still include
+            // it so it gets abandoned.
+            continue;
+        }
         // TODO: Do we want to optimize the case of moving to the parent commit (`jj
         // squash -r`)? The source tree will be unchanged in that case.
         source_commits.push(SourceCommit {
@@ -222,10 +227,7 @@ from the source will be moved into the destination.
             abandon,
         });
     }
-    if source_commits
-        .iter()
-        .all(|source| source.selected_tree == source.parent_tree)
-    {
+    if source_commits.is_empty() {
         if diff_selector.is_interactive() {
             return Err(user_error("No changes selected"));
         }
@@ -245,6 +247,8 @@ from the source will be moved into the destination.
                 )?;
             }
         }
+
+        return Ok(());
     }
 
     for source in &source_commits {

cli/tests/test_move_command.rs

@@ -345,16 +345,14 @@ fn test_move_partial() {
     a
     "###);
 
-    // If we specify only a non-existent file, then the move still succeeds and
-    // creates unchanged commits.
+    // If we specify only a non-existent file, then nothing changes.
     test_env.jj_cmd_ok(&repo_path, &["undo"]);
     let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["move", "--from", "c", "nonexistent"]);
     insta::assert_snapshot!(stdout, @"");
     insta::assert_snapshot!(stderr, @r###"
     Warning: `jj move` is deprecated; use `jj squash` instead, which is equivalent
     Warning: `jj move` will be removed in a future version, and this will be a hard error
-    Working copy now at: vruxwmqv b670567d d | (no description set)
-    Parent commit      : qpvuntsm 3db0a2f5 a | (no description set)
+    Nothing changed.
     "###);
 }
 

cli/tests/test_squash_command.rs

@@ -241,25 +241,20 @@ fn test_squash_partial() {
     b
     "###);
 
-    // If we specify only a non-existent file, then the squash still succeeds and
-    // creates unchanged commits.
+    // If we specify only a non-existent file, then nothing changes.
     test_env.jj_cmd_ok(&repo_path, &["undo"]);
     let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["squash", "-r", "b", "nonexistent"]);
     insta::assert_snapshot!(stdout, @"");
     insta::assert_snapshot!(stderr, @r###"
-    Rebased 2 descendant commits
-    Working copy now at: mzvwutvl 5e297967 c | (no description set)
-    Parent commit      : kkmpptxz ac258609 b | (no description set)
+    Nothing changed.
     "###);
 
     // We get a warning if we pass a positional argument that looks like a revset
     test_env.jj_cmd_ok(&repo_path, &["undo"]);
     let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["squash", "b"]);
     insta::assert_snapshot!(stderr, @r###"
     Warning: The argument "b" is being interpreted as a path. To specify a revset, pass -r "b" instead.
-    Rebased 1 descendant commits
-    Working copy now at: mzvwutvl 1c4e5596 c | (no description set)
-    Parent commit      : kkmpptxz 16cc94b4 b | (no description set)
+    Nothing changed.
     "###);
     insta::assert_snapshot!(stdout, @"");
 }
@@ -570,15 +565,13 @@ fn test_squash_from_to_partial() {
     a
     "###);
 
-    // If we specify only a non-existent file, then the move still succeeds and
-    // creates unchanged commits.
+    // If we specify only a non-existent file, then nothing changes.
     test_env.jj_cmd_ok(&repo_path, &["undo"]);
     let (stdout, stderr) =
         test_env.jj_cmd_ok(&repo_path, &["squash", "--from", "c", "nonexistent"]);
     insta::assert_snapshot!(stdout, @"");
     insta::assert_snapshot!(stderr, @r###"
-    Working copy now at: vruxwmqv b670567d d | (no description set)
-    Parent commit      : qpvuntsm 3db0a2f5 a | (no description set)
+    Nothing changed.
     "###);
 }
 
@@ -692,12 +685,11 @@ fn test_squash_from_multiple() {
     f
     "###);
 
-    // Empty squash shouldn't crash (could be noop)
+    // Empty squash shouldn't crash
     let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["squash", "--from=none()"]);
     insta::assert_snapshot!(stdout, @"");
     insta::assert_snapshot!(stderr, @r###"
-    Working copy now at: xznxytkn 68d54010 (empty) (no description set)
-    Parent commit      : yostqsxw b7bc1dda e f | (no description set)
+    Nothing changed.
     "###);
 }
 
@@ -893,7 +885,6 @@ fn test_squash_from_multiple_partial_no_op() {
     "###);
 
     // Source commits that didn't match the paths are not rewritten
-    // TODO: Comit c should be unchanged
     let (stdout, stderr) = test_env.jj_cmd_ok(
         &repo_path,
         &["squash", "--from=@-+ ~ @", "--into=@", "-m=d", "b"],
@@ -906,7 +897,7 @@ fn test_squash_from_multiple_partial_no_op() {
     "###);
     insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
     @  9227d0d780fa d
-    │ ◉  8907be4aab96 c
+    │ ◉  5ad3ca4090a7 c
     ├─╯
     ◉  3df52ee1f8a9 a
     ◉  000000000000
@@ -932,22 +923,20 @@ fn test_squash_from_multiple_partial_no_op() {
     "###);
 
     // If no source commits match the paths, then the whole operation is a no-op
-    // TODO: All commits should be unchanged
     test_env.jj_cmd_ok(&repo_path, &["undo"]);
     let (stdout, stderr) = test_env.jj_cmd_ok(
         &repo_path,
         &["squash", "--from=@-+ ~ @", "--into=@", "-m=d", "a"],
     );
     insta::assert_snapshot!(stdout, @"");
     insta::assert_snapshot!(stderr, @r###"
-    Working copy now at: mzvwutvl 17f5bc7a d
-    Parent commit      : qpvuntsm 3df52ee1 a
+    Nothing changed.
     "###);
     insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
-    @  17f5bc7ab82a d
-    │ ◉  6eba26a0b46d c
+    @  09441f0a6266 d
+    │ ◉  5ad3ca4090a7 c
     ├─╯
-    │ ◉  88c573358ed5 b
+    │ ◉  285201979c90 b
     ├─╯
     ◉  3df52ee1f8a9 a
     ◉  000000000000

lib/src/rewrite.rs

@@ -186,9 +186,10 @@ impl<'repo> CommitRewriter<'repo> {
 
     /// Records the old commit as abandoned with the new parents.
     pub fn record_abandoned_commit(self) {
-        let old_commit_id =self.old_commit.id().clone();
+        let old_commit_id = self.old_commit.id().clone();
         let new_parents = self.new_parents;
-        self.mut_repo.record_abandoned_commit_with_parents(old_commit_id, new_parents);
+        self.mut_repo
+            .record_abandoned_commit_with_parents(old_commit_id, new_parents);
     }
 
     /// Rebase the old commit onto the new parents. Returns a `CommitBuilder`