diff --git a/cli/src/commands/move.rs b/cli/src/commands/move.rs index ff65ed40ef..c321074647 100644 --- a/cli/src/commands/move.rs +++ b/cli/src/commands/move.rs @@ -113,9 +113,10 @@ from the source will be moved into the destination. // rewritten source. Otherwise it will likely already have the content // changes we're moving, so applying them will have no effect and the // changes will disappear. - let mut rebaser = tx.mut_repo().create_descendant_rebaser(command.settings()); - rebaser.rebase_all()?; - let rebased_destination_id = rebaser.rebased().get(destination.id()).unwrap().clone(); + let rebase_map = tx + .mut_repo() + .rebase_descendants_return_map(command.settings())?; + let rebased_destination_id = rebase_map.get(destination.id()).unwrap().clone(); destination = tx.mut_repo().store().get_commit(&rebased_destination_id)?; } // Apply the selected changes onto the destination diff --git a/cli/src/commands/split.rs b/cli/src/commands/split.rs index 7fe3914521..21e78257f5 100644 --- a/cli/src/commands/split.rs +++ b/cli/src/commands/split.rs @@ -15,8 +15,7 @@ use std::io::Write; use jj_lib::backend::ObjectId; use jj_lib::repo::Repo; -use jj_lib::rewrite::{merge_commit_trees, DescendantRebaser}; -use maplit::{hashmap, hashset}; +use jj_lib::rewrite::merge_commit_trees; use tracing::instrument; use crate::cli_util::{CommandError, CommandHelper, RevisionArg}; @@ -135,14 +134,14 @@ don't make any changes, then the operation will be aborted. .generate_new_change_id() .set_description(second_description) .write()?; - let mut rebaser = DescendantRebaser::new( - command.settings(), - tx.mut_repo(), - hashmap! { commit.id().clone() => hashset!{second_commit.id().clone()} }, - hashset! {}, - ); - rebaser.rebase_all()?; - let num_rebased = rebaser.rebased().len(); + + // Currently, `rebase_descendents` would treat `commit` as being rewritten to + // *both* `first_commit` and `second_commit`, as if it was becoming divergent. + // However, we want only the `second_commit` to inherit `commit`'s branches and + // descendants. + tx.mut_repo() + .set_rewritten_commit(commit.id().clone(), [second_commit.id().clone()]); + let num_rebased = tx.mut_repo().rebase_descendants(command.settings())?; if num_rebased > 0 { writeln!(ui.stderr(), "Rebased {num_rebased} descendant commits")?; } diff --git a/cli/tests/test_split_command.rs b/cli/tests/test_split_command.rs index 92dfdcc913..42ebc0e288 100644 --- a/cli/tests/test_split_command.rs +++ b/cli/tests/test_split_command.rs @@ -32,6 +32,10 @@ fn test_split_by_paths() { @ qpvuntsmwlqt false ◉ zzzzzzzzzzzz true "###); + insta::assert_snapshot!(get_recorded_dates(&test_env, &repo_path,"@"), @r###" + Author date: 2001-02-03 04:05:07.000 +07:00 + Committer date: 2001-02-03 04:05:08.000 +07:00 + "###); let edit_script = test_env.set_up_fake_editor(); std::fs::write( @@ -42,10 +46,10 @@ fn test_split_by_paths() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["split", "file2"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" - First part: qpvuntsm 5eebce1d (no description set) - Second part: kkmpptxz 45833353 (no description set) - Working copy now at: kkmpptxz 45833353 (no description set) - Parent commit : qpvuntsm 5eebce1d (no description set) + First part: qpvuntsm d62c056f (no description set) + Second part: zsuskuln 5a32af4a (no description set) + Working copy now at: zsuskuln 5a32af4a (no description set) + Parent commit : qpvuntsm d62c056f (no description set) "###); insta::assert_snapshot!( std::fs::read_to_string(test_env.env_root().join("editor0")).unwrap(), @r###" @@ -59,11 +63,22 @@ fn test_split_by_paths() { assert!(!test_env.env_root().join("editor1").exists()); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - @ kkmpptxzrspx false + @ zsuskulnrvyr false ◉ qpvuntsmwlqt false ◉ zzzzzzzzzzzz true "###); + // The author dates of the new commits should be inherited from the commit being + // split. The committer dates should be newer. + insta::assert_snapshot!(get_recorded_dates(&test_env, &repo_path,"@"), @r###" + Author date: 2001-02-03 04:05:07.000 +07:00 + Committer date: 2001-02-03 04:05:10.000 +07:00 + "###); + insta::assert_snapshot!(get_recorded_dates(&test_env, &repo_path,"@-"), @r###" + Author date: 2001-02-03 04:05:07.000 +07:00 + Committer date: 2001-02-03 04:05:10.000 +07:00 + "###); + let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s", "-r", "@-"]); insta::assert_snapshot!(stdout, @r###" A file2 @@ -80,15 +95,15 @@ fn test_split_by_paths() { insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" Rebased 1 descendant commits - First part: qpvuntsm 31425b56 (no description set) - Second part: yqosqzyt af096392 (empty) (no description set) - Working copy now at: kkmpptxz 28d4ec20 (no description set) - Parent commit : yqosqzyt af096392 (empty) (no description set) + First part: qpvuntsm b76d731d (no description set) + Second part: znkkpsqq 924604b2 (empty) (no description set) + Working copy now at: zsuskuln fffe30fb (no description set) + Parent commit : znkkpsqq 924604b2 (empty) (no description set) "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - @ kkmpptxzrspx false - ◉ yqosqzytrlsw true + @ zsuskulnrvyr false + ◉ znkkpsqqskkl true ◉ qpvuntsmwlqt false ◉ zzzzzzzzzzzz true "###); @@ -108,15 +123,15 @@ fn test_split_by_paths() { insta::assert_snapshot!(stderr, @r###" The given paths do not match any file: nonexistent Rebased 1 descendant commits - First part: qpvuntsm 0647b2cb (empty) (no description set) - Second part: kpqxywon d5d77af6 (no description set) - Working copy now at: kkmpptxz 86f228dc (no description set) - Parent commit : kpqxywon d5d77af6 (no description set) + First part: qpvuntsm 7086b0bc (empty) (no description set) + Second part: lylxulpl 2252ed18 (no description set) + Working copy now at: zsuskuln a3f2136a (no description set) + Parent commit : lylxulpl 2252ed18 (no description set) "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - @ kkmpptxzrspx false - ◉ kpqxywonksrl false + @ zsuskulnrvyr false + ◉ lylxulplsnyw false ◉ qpvuntsmwlqt true ◉ zzzzzzzzzzzz true "###); @@ -222,3 +237,8 @@ fn get_log_output(test_env: &TestEnvironment, cwd: &Path) -> String { let template = r#"separate(" ", change_id.short(), empty, description)"#; test_env.jj_cmd_success(cwd, &["log", "-T", template]) } + +fn get_recorded_dates(test_env: &TestEnvironment, cwd: &Path, revset: &str) -> String { + let template = r#"separate("\n", "Author date: " ++ author.timestamp(), "Committer date: " ++ committer.timestamp())"#; + test_env.jj_cmd_success(cwd, &["log", "--no-graph", "-T", template, "-r", revset]) +} diff --git a/lib/src/commit_builder.rs b/lib/src/commit_builder.rs index 67c50e48a2..7a6ea1d2bb 100644 --- a/lib/src/commit_builder.rs +++ b/lib/src/commit_builder.rs @@ -32,7 +32,8 @@ pub struct CommitBuilder<'repo> { } impl CommitBuilder<'_> { - pub fn for_new_commit<'repo>( + // Use MutRepo::new_commit() instead + pub(crate) fn for_new_commit<'repo>( mut_repo: &'repo mut MutableRepo, settings: &UserSettings, parents: Vec, @@ -61,7 +62,8 @@ impl CommitBuilder<'_> { } } - pub fn for_rewrite_from<'repo>( + // Use MutRepo::rewrite_commit() instead + pub(crate) fn for_rewrite_from<'repo>( mut_repo: &'repo mut MutableRepo, settings: &UserSettings, predecessor: &Commit, diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 5f2540f6fa..75fd85c197 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -796,6 +796,8 @@ impl MutableRepo { predecessor: &Commit, ) -> CommitBuilder { CommitBuilder::for_rewrite_from(self, settings, predecessor) + // CommitBuilder::write will record the rewrite in + // `self.rewritten_commits` } pub fn write_commit( @@ -808,12 +810,43 @@ impl MutableRepo { Ok(commit) } - /// Record a commit as having been rewritten in this transaction. This - /// record is used by `rebase_descendants()`. + /// Record a commit as having been rewritten to one or more ids in this + /// transaction. /// - /// Rewritten commits don't have to be recorded here. This is just a - /// convenient place to record it. It won't matter after the transaction - /// has been committed. + /// This record is used by `rebase_descendants` to know which commits have + /// children that need to be rebased, and where to rebase them to. See the + /// docstring for `record_rewritten_commit` for details. + // TODO(ilyagr): Make this API saner, e.g. make `self.rewritten_commits` public + // and make empty values correspond to abandoned commits. + pub fn set_rewritten_commit( + &mut self, + old_id: CommitId, + new_ids: impl IntoIterator, + ) { + assert_ne!(old_id, *self.store().root_commit_id()); + self.rewritten_commits + .insert(old_id, new_ids.into_iter().collect()); + } + + /// Record a commit as having been rewritten in this transaction. If it was + /// already rewritten, mark it as divergent (unlike `set_rewritten_commit`) + /// + /// This record is used by `rebase_descendants` to know which commits have + /// children that need to be rebased, and where to rebase the children (as + /// well as branches) to. + /// + /// The `rebase_descendants` logic treats these records as follows: + /// + /// - If a commit is recorded as rewritten to a single commit, its + /// descendants would be rebased to become descendants of `new_id`. Any + /// branches at `old_id` are also moved to `new_id`. + /// - If a commit is recorded as rewritten to more than one commit, it is + /// assumed to have become divergent. Its descendants are *not* rebased. + /// However, the *branches* that were at `old_id` are moved to each of the + /// new ids, and thus become conflicted. + /// + /// In neither case would `rebase_descendants` modify the `old_id` commit + /// itself. pub fn record_rewritten_commit(&mut self, old_id: CommitId, new_id: CommitId) { assert_ne!(old_id, *self.store().root_commit_id()); self.rewritten_commits @@ -826,12 +859,15 @@ impl MutableRepo { self.rewritten_commits.clear(); } - /// Record a commit as having been abandoned in this transaction. This - /// record is used by `rebase_descendants()`. + /// Record a commit as having been abandoned in this transaction. + /// + /// This record is used by `rebase_descendants` to know which commits have + /// children that need to be rebased, and where to rebase the children (as + /// well as branches) to. /// - /// Abandoned commits don't have to be recorded here. This is just a - /// convenient place to record it. It won't matter after the transaction - /// has been committed. + /// The `rebase_descendants` logic will rebase the descendants of `old_id` + /// to become the descendants of parent(s) of `old_id`. Any branches at + /// `old_id` would be moved to the parent(s) of `old_id` as well. pub fn record_abandoned_commit(&mut self, old_id: CommitId) { assert_ne!(old_id, *self.store().root_commit_id()); self.abandoned_commits.insert(old_id); @@ -847,6 +883,7 @@ impl MutableRepo { /// Creates a `DescendantRebaser` to rebase descendants of the recorded /// rewritten and abandoned commits. + // TODO(ilyagr): Inline this. It's only used in tests. pub fn create_descendant_rebaser<'settings, 'repo>( &'repo mut self, settings: &'settings UserSettings, @@ -874,6 +911,9 @@ impl MutableRepo { Ok(Some(rebaser)) } + // TODO(ilyagr): Either document that this also moves branches (rename the + // function and the related functions?) or change things so that this only + // rebases descendants. pub fn rebase_descendants_with_options( &mut self, settings: &UserSettings, diff --git a/lib/tests/test_rewrite.rs b/lib/tests/test_rewrite.rs index 17dd66f4e9..7e278d6d1e 100644 --- a/lib/tests/test_rewrite.rs +++ b/lib/tests/test_rewrite.rs @@ -1119,6 +1119,7 @@ fn test_rebase_descendants_update_branches_after_divergent_rewrite() { // B main |/B2 main? // | => |/ // A A + // TODO(ilyagr): Check what happens if B had a descendant with a branch on it. let mut tx = repo.start_transaction(&settings); let mut graph_builder = CommitGraphBuilder::new(&settings, tx.mut_repo()); let commit_a = graph_builder.initial_commit();