From 57e636629fafeec2bf6f76c9ef525560307283fb Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Sun, 17 Dec 2023 17:44:32 -0800 Subject: [PATCH 1/4] Make CommitBuilder constructors private to the library crate The implementation of `CommitBuilder::write` is tightly bound to the MutRepo, so only MutRepo should construct CommitBuilder-s. --- lib/src/commit_builder.rs | 6 ++++-- lib/src/repo.rs | 2 ++ 2 files changed, 6 insertions(+), 2 deletions(-) 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..cf4b03c4db 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( From f3bde31b664d756be60f88b8ae2f315cb2e61d12 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Sat, 16 Dec 2023 21:08:49 -0800 Subject: [PATCH 2/4] `move.rs`: remove use of `MutRepo::create_descenant_rebaser`. After this, the internal function is only used in tests. --- cli/src/commands/move.rs | 7 ++++--- lib/src/repo.rs | 1 + 2 files changed, 5 insertions(+), 3 deletions(-) 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/lib/src/repo.rs b/lib/src/repo.rs index cf4b03c4db..2ef71080fd 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -849,6 +849,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, From 3b0a3d86f6986e531fde9ed61a0544a240f4d2b5 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Fri, 22 Dec 2023 20:29:05 -0800 Subject: [PATCH 3/4] test_split_command: check that new commits inherit author dates At some point, I tried `new_commit` instead of `rewrite_commit` in the split command. That seemed to work, but messed up the dates in a subtle way. This commit should prevents repeats of this mistake and emphasize the importance of the author dates being preserved. --- cli/tests/test_split_command.rs | 54 ++++++++++++++++++++++----------- 1 file changed, 37 insertions(+), 17 deletions(-) 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]) +} From 8e8bcaf8a89902684a45d17c9978b030637afccc Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Sun, 17 Dec 2023 21:28:14 -0800 Subject: [PATCH 4/4] `split.rs`: stop using DescendantRebaser::new This requires creating a new public API as a substitute. I took the opportunity to also add some comments to the `MutRepo::record_rewritten_commit`/`record_abandoned_commit` functions. I imade the simplest possible addition to the API; it is not a very elegant one. Eventually, the entire `record_rewritten_commit` API should probably be refactored again. I also added some comments explaining what these functions do. --- cli/src/commands/split.rs | 19 +++++++------ lib/src/repo.rs | 57 ++++++++++++++++++++++++++++++++------- lib/tests/test_rewrite.rs | 1 + 3 files changed, 57 insertions(+), 20 deletions(-) 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/lib/src/repo.rs b/lib/src/repo.rs index 2ef71080fd..75fd85c197 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -810,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 @@ -828,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); @@ -877,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();