From 39f481f2da35acd23f16280fbed24da2450892bd Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Wed, 16 Apr 2025 18:30:08 +0900 Subject: [PATCH] absorb: add basic support for file deletion This works if the file was added and wasn't modified within the destination range. Closes #6140 --- CHANGELOG.md | 3 + cli/tests/test_absorb_command.rs | 150 ++++++++++++++++++++++++++++++- lib/src/absorb.rs | 35 ++++---- 3 files changed, 167 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 98b2eb296..bc24f3feb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). * Added `duplicate_description` template, which allows [customizing the descriptions of the commits `jj duplicate` creates](docs/config.md#duplicate-commit-description). +* `jj absorb` can now squash a deleted file if it was added by one of the + destination revisions. + ### Fixed bugs * Fixed crash on change-delete conflict resolution. diff --git a/cli/tests/test_absorb_command.rs b/cli/tests/test_absorb_command.rs index c8c4c4f47..05ae20473 100644 --- a/cli/tests/test_absorb_command.rs +++ b/cli/tests/test_absorb_command.rs @@ -479,15 +479,161 @@ fn test_absorb_deleted_file() { work_dir.run_jj(["describe", "-m1"]).success(); work_dir.write_file("file1", "1a\n"); + work_dir.write_file("file2", "1a\n"); + work_dir.write_file("file3", ""); work_dir.run_jj(["new"]).success(); work_dir.remove_file("file1"); + work_dir.write_file("file2", ""); // emptied + work_dir.remove_file("file3"); // no content change + // Since the destinations are chosen based on content diffs, file3 cannot be + // absorbed. let output = work_dir.run_jj(["absorb"]); insta::assert_snapshot!(output, @r" ------- stderr ------- - Warning: Skipping file1: Deleted file - Nothing changed. + Absorbed changes into 1 revisions: + qpvuntsm f3c5cd48 1 + Rebased 1 descendant commits. + Working copy (@) now at: kkmpptxz 691fa544 (no description set) + Parent commit (@-) : qpvuntsm f3c5cd48 1 + Remaining changes: + D file3 + [EOF] + "); + + insta::assert_snapshot!(get_diffs(&work_dir, "mutable()"), @r" + @ kkmpptxz 691fa544 (no description set) + │ diff --git a/file3 b/file3 + │ deleted file mode 100644 + │ index e69de29bb2..0000000000 + ○ qpvuntsm f3c5cd48 1 + │ diff --git a/file2 b/file2 + ~ new file mode 100644 + index 0000000000..e69de29bb2 + diff --git a/file3 b/file3 + new file mode 100644 + index 0000000000..e69de29bb2 + [EOF] + "); +} + +#[test] +fn test_absorb_deleted_file_with_multiple_hunks() { + let test_env = TestEnvironment::default(); + test_env.run_jj_in(".", ["git", "init", "repo"]).success(); + let work_dir = test_env.work_dir("repo"); + + work_dir.run_jj(["describe", "-m1"]).success(); + work_dir.write_file("file1", "1a\n1b\n"); + work_dir.write_file("file2", "1a\n"); + + work_dir.run_jj(["new", "-m2"]).success(); + work_dir.write_file("file1", "1a\n"); + work_dir.write_file("file2", "1a\n1b\n"); + + // These changes produce conflicts because + // - for file1, "1a\n" is deleted from the commit 1, + // - for file2, two consecutive hunks are deleted. + // + // Since file2 change is split to two separate hunks, the file deletion + // cannot be propagated. If we implement merging based on interleaved delta, + // the file2 change will apply cleanly. The file1 change might be split into + // "1a\n" deletion at the commit 1 and file deletion at the commit 2, but + // I'm not sure if that's intuitive. + work_dir.run_jj(["new"]).success(); + work_dir.remove_file("file1"); + work_dir.remove_file("file2"); + let output = work_dir.run_jj(["absorb"]); + insta::assert_snapshot!(output, @r" + ------- stderr ------- + Absorbed changes into 2 revisions: + kkmpptxz ca0a3d3c (conflict) 2 + qpvuntsm f2703d39 (conflict) 1 + Rebased 1 descendant commits. + Working copy (@) now at: zsuskuln 0156c3af (no description set) + Parent commit (@-) : kkmpptxz ca0a3d3c (conflict) 2 + New conflicts appeared in 2 commits: + kkmpptxz ca0a3d3c (conflict) 2 + qpvuntsm f2703d39 (conflict) 1 + Hint: To resolve the conflicts, start by updating to the first one: + jj new qpvuntsm + Then use `jj resolve`, or edit the conflict markers in the file directly. + Once the conflicts are resolved, you may want to inspect the result with `jj diff`. + Then run `jj squash` to move the resolution into the conflicted commit. + Remaining changes: + D file2 + [EOF] + "); + + insta::assert_snapshot!(get_diffs(&work_dir, "mutable()"), @r" + @ zsuskuln 0156c3af (no description set) + │ diff --git a/file2 b/file2 + │ deleted file mode 100644 + │ index 0000000000..0000000000 + │ --- a/file2 + │ +++ /dev/null + │ @@ -1,7 +0,0 @@ + │ -<<<<<<< Conflict 1 of 1 + │ -%%%%%%% Changes from base to side #1 + │ --1a + │ - 1b + │ -+++++++ Contents of side #2 + │ -1a + │ ->>>>>>> Conflict 1 of 1 ends + × kkmpptxz ca0a3d3c (conflict) 2 + │ diff --git a/file1 b/file1 + │ deleted file mode 100644 + │ index 0000000000..0000000000 + │ --- a/file1 + │ +++ /dev/null + │ @@ -1,6 +0,0 @@ + │ -<<<<<<< Conflict 1 of 1 + │ -%%%%%%% Changes from base to side #1 + │ - 1a + │ -+1b + │ -+++++++ Contents of side #2 + │ ->>>>>>> Conflict 1 of 1 ends + │ diff --git a/file2 b/file2 + │ --- a/file2 + │ +++ b/file2 + │ @@ -1,7 +1,7 @@ + │ <<<<<<< Conflict 1 of 1 + │ %%%%%%% Changes from base to side #1 + │ - 1a + │ --1b + │ +-1a + │ + 1b + │ +++++++ Contents of side #2 + │ -1b + │ +1a + │ >>>>>>> Conflict 1 of 1 ends + × qpvuntsm f2703d39 (conflict) 1 + │ diff --git a/file1 b/file1 + ~ new file mode 100644 + index 0000000000..0000000000 + --- /dev/null + +++ b/file1 + @@ -0,0 +1,6 @@ + +<<<<<<< Conflict 1 of 1 + +%%%%%%% Changes from base to side #1 + + 1a + ++1b + ++++++++ Contents of side #2 + +>>>>>>> Conflict 1 of 1 ends + diff --git a/file2 b/file2 + new file mode 100644 + index 0000000000..0000000000 + --- /dev/null + +++ b/file2 + @@ -0,0 +1,7 @@ + +<<<<<<< Conflict 1 of 1 + +%%%%%%% Changes from base to side #1 + + 1a + +-1b + ++++++++ Contents of side #2 + +1b + +>>>>>>> Conflict 1 of 1 ends [EOF] "); } diff --git a/lib/src/absorb.rs b/lib/src/absorb.rs index 868515586..12bef216c 100644 --- a/lib/src/absorb.rs +++ b/lib/src/absorb.rs @@ -117,17 +117,9 @@ pub async fn split_hunks_to_trees( continue; } }; - let right_text = match to_file_value(right_value) { - Ok(Some(mut value)) => value.read_all(right_path)?, - // Deleted file could be absorbed, but that would require special - // handling to propagate deletion of the tree entry - Ok(None) => { - let reason = "Deleted file".to_owned(); - selected_trees - .skipped_paths - .push((right_path.to_owned(), reason)); - continue; - } + let (right_text, deleted) = match to_file_value(right_value) { + Ok(Some(mut value)) => (value.read_all(right_path)?, false), + Ok(None) => (vec![], true), Err(reason) => { selected_trees .skipped_paths @@ -157,14 +149,19 @@ pub async fn split_hunks_to_trees( .entry(commit_id.clone()) .or_insert_with(|| MergedTreeBuilder::new(left_tree.id().clone())); let new_text = combine_texts(&left_text, &right_text, ranges); - let id = repo - .store() - .write_file(left_path, &mut new_text.as_slice()) - .await?; - tree_builder.set_or_remove( - left_path.to_owned(), - Merge::normal(TreeValue::File { id, executable }), - ); + // Since changes to be absorbed are represented as diffs relative to + // the source parent, we can propagate file deletion only if the + // whole file content is deleted at a single destination commit. + let new_tree_value = if new_text.is_empty() && deleted { + Merge::absent() + } else { + let id = repo + .store() + .write_file(left_path, &mut new_text.as_slice()) + .await?; + Merge::normal(TreeValue::File { id, executable }) + }; + tree_builder.set_or_remove(left_path.to_owned(), new_tree_value); } }