From 0bc1341fd0140a96ad0c3d27be4b1d8412b6626d Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Tue, 16 Jan 2024 14:12:02 -0800 Subject: [PATCH] revset: add count_estimate() to Revset trait The count() function in this trait is used by "jj branch" to determine (and then report) how many commits a certain branch is ahead/behind another branch. This is currently implemented by walking all commits in the revset, counting how many were encountered. But this could be improved: if the number is large, it is probably sufficient to report "at least N" (instead of walking all the way), and this does not scale well to jj backends that may not have all commits present locally (which may prefer to return an estimate, rather than access the network). Therefore, add a function that is explicitly documented to be O(1) and that can return a range of values if the backend so chooses. Also remove count(), as it is not immediately obvious that it is an expensive call, and callers that are willing to pay the cost can obtain the exact same functionality through iter().count() anyway. (In this commit, all users of count() are migrated to iter().count() to preserve all existing functionality; they will be migrated to count_estimate() in a subsequent commit.) "branch" needed to be updated due to this change. Although jj is currently only available in English, I have attempted to keep user-visible text from being assembled piece by piece, so that if we later decide to translate jj into other languages, things will be easier for translators. --- cli/Cargo.toml | 2 +- cli/src/commands/branch.rs | 30 +++++++++++------- cli/tests/test_branch_command.rs | 44 ++++++++++++++++++++++++++ lib/Cargo.toml | 1 + lib/src/default_index/revset_engine.rs | 17 ++++++++-- lib/src/revset.rs | 6 +++- 6 files changed, 84 insertions(+), 16 deletions(-) diff --git a/cli/Cargo.toml b/cli/Cargo.toml index b5de30e7e..f6b1ab76a 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -84,7 +84,7 @@ jj-cli = { path = ".", features = ["test-fakes"], default-features = false } default = ["watchman"] bench = ["dep:criterion"] packaging = [] -test-fakes = [] +test-fakes = ["jj-lib/testing"] vendored-openssl = ["git2/vendored-openssl", "jj-lib/vendored-openssl"] watchman = ["jj-lib/watchman"] diff --git a/cli/src/commands/branch.rs b/cli/src/commands/branch.rs index a798487c5..e4ab3e588 100644 --- a/cli/src/commands/branch.rs +++ b/cli/src/commands/branch.rs @@ -697,19 +697,25 @@ fn cmd_branch_list( if local_target.is_present() && !synced { let remote_added_ids = remote_ref.target.added_ids().cloned().collect_vec(); let local_added_ids = local_target.added_ids().cloned().collect_vec(); - let remote_ahead_count = - revset::walk_revs(repo.as_ref(), &remote_added_ids, &local_added_ids)?.count(); - let local_ahead_count = - revset::walk_revs(repo.as_ref(), &local_added_ids, &remote_added_ids)?.count(); - let remote_ahead_message = if remote_ahead_count != 0 { - Some(format!("ahead by {remote_ahead_count} commits")) - } else { - None + let (remote_ahead_lower, remote_ahead_upper) = + revset::walk_revs(repo.as_ref(), &remote_added_ids, &local_added_ids)? + .count_estimate(); + let (local_ahead_lower, local_ahead_upper) = + revset::walk_revs(repo.as_ref(), &local_added_ids, &remote_added_ids)? + .count_estimate(); + let remote_ahead_message = match remote_ahead_upper { + Some(0) => None, + Some(upper) if upper == remote_ahead_lower => { + Some(format!("ahead by {remote_ahead_lower} commits")) + } + _ => Some(format!("ahead by at least {remote_ahead_lower} commits")), }; - let local_ahead_message = if local_ahead_count != 0 { - Some(format!("behind by {local_ahead_count} commits")) - } else { - None + let local_ahead_message = match local_ahead_upper { + Some(0) => None, + Some(upper) if upper == local_ahead_lower => { + Some(format!("behind by {local_ahead_lower} commits")) + } + _ => Some(format!("behind by at least {local_ahead_lower} commits")), }; match (remote_ahead_message, local_ahead_message) { (Some(rm), Some(lm)) => { diff --git a/cli/tests/test_branch_command.rs b/cli/tests/test_branch_command.rs index 29ede5e93..3828d6e5d 100644 --- a/cli/tests/test_branch_command.rs +++ b/cli/tests/test_branch_command.rs @@ -1139,6 +1139,50 @@ fn test_branch_list_filtered() { "###); } +#[test] +fn test_branch_list_much_remote_divergence() { + let test_env = TestEnvironment::default(); + test_env.add_config("git.auto-local-branch = true"); + + // Initialize remote refs + test_env.jj_cmd_ok(test_env.env_root(), &["init", "remote", "--git"]); + let remote_path = test_env.env_root().join("remote"); + test_env.jj_cmd_ok(&remote_path, &["new", "root()", "-m", "remote-unsync"]); + for _ in 0..15 { + test_env.jj_cmd_ok(&remote_path, &["new", "-m", "remote-unsync"]); + } + test_env.jj_cmd_ok(&remote_path, &["branch", "create", "remote-unsync"]); + test_env.jj_cmd_ok(&remote_path, &["new"]); + test_env.jj_cmd_ok(&remote_path, &["git", "export"]); + + // Initialize local refs + let mut remote_git_path = remote_path; + remote_git_path.extend([".jj", "repo", "store", "git"]); + test_env.jj_cmd_ok( + test_env.env_root(), + &["git", "clone", remote_git_path.to_str().unwrap(), "local"], + ); + let local_path = test_env.env_root().join("local"); + test_env.jj_cmd_ok(&local_path, &["new", "root()", "-m", "local-only"]); + for _ in 0..15 { + test_env.jj_cmd_ok(&local_path, &["new", "-m", "local-only"]); + } + test_env.jj_cmd_ok(&local_path, &["branch", "create", "local-only"]); + + // Mutate refs in local repository + test_env.jj_cmd_ok( + &local_path, + &["branch", "set", "--allow-backwards", "remote-unsync"], + ); + + insta::assert_snapshot!( + test_env.jj_cmd_success(&local_path, &["branch", "list"]), @r###" + local-only: zkyosouw 4ab3f751 (empty) local-only + remote-unsync: zkyosouw 4ab3f751 (empty) local-only + @origin (ahead by at least 10 commits, behind by at least 10 commits): lxyktnks 19582022 (empty) remote-unsync + "###); +} + fn get_log_output(test_env: &TestEnvironment, cwd: &Path) -> String { let template = r#"branches ++ " " ++ commit_id.short()"#; test_env.jj_cmd_success(cwd, &["log", "-T", template]) diff --git a/lib/Cargo.toml b/lib/Cargo.toml index 7869b42ed..7aad8b420 100644 --- a/lib/Cargo.toml +++ b/lib/Cargo.toml @@ -74,3 +74,4 @@ tokio = { workspace = true, features = ["full"] } default = [] vendored-openssl = ["git2/vendored-openssl"] watchman = ["dep:tokio", "dep:watchman_client"] +testing = [] diff --git a/lib/src/default_index/revset_engine.rs b/lib/src/default_index/revset_engine.rs index 1ad24a7e5..15de39cd0 100644 --- a/lib/src/default_index/revset_engine.rs +++ b/lib/src/default_index/revset_engine.rs @@ -130,8 +130,21 @@ impl Revset for RevsetImpl { self.entries().next().is_none() } - fn count(&self) -> usize { - self.entries().count() + fn count_estimate(&self) -> (usize, Option) { + if cfg!(feature = "testing") { + // Exercise the estimation feature in tests. (If we ever have a Revset + // implementation in production code that returns estimates, we can probably + // remove this and rewrite the associated tests.) + let count = self.entries().take(10).count(); + if count < 10 { + (count, Some(count)) + } else { + (10, None) + } + } else { + let count = self.iter().count(); + (count, Some(count)) + } } } diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 15745faee..f5fee3ebd 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -2415,7 +2415,11 @@ pub trait Revset: fmt::Debug { fn is_empty(&self) -> bool; - fn count(&self) -> usize; + /// Inclusive lower bound and, optionally, inclusive upper bound of how many + /// commits are in the revset. The implementation can use its discretion as + /// to how much effort should be put into the estimation, and how accurate + /// the resulting estimate should be. + fn count_estimate(&self) -> (usize, Option); } pub trait RevsetIteratorExt<'index, I> {