ok/jj
1
0
Fork 0
forked from mirrors/jj

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.
This commit is contained in:
Jonathan Tan 2024-01-16 14:12:02 -08:00 committed by jonathantanmy
parent 08da40bc82
commit 0bc1341fd0
6 changed files with 84 additions and 16 deletions

View file

@ -84,7 +84,7 @@ jj-cli = { path = ".", features = ["test-fakes"], default-features = false }
default = ["watchman"] default = ["watchman"]
bench = ["dep:criterion"] bench = ["dep:criterion"]
packaging = [] packaging = []
test-fakes = [] test-fakes = ["jj-lib/testing"]
vendored-openssl = ["git2/vendored-openssl", "jj-lib/vendored-openssl"] vendored-openssl = ["git2/vendored-openssl", "jj-lib/vendored-openssl"]
watchman = ["jj-lib/watchman"] watchman = ["jj-lib/watchman"]

View file

@ -697,19 +697,25 @@ fn cmd_branch_list(
if local_target.is_present() && !synced { if local_target.is_present() && !synced {
let remote_added_ids = remote_ref.target.added_ids().cloned().collect_vec(); let remote_added_ids = remote_ref.target.added_ids().cloned().collect_vec();
let local_added_ids = local_target.added_ids().cloned().collect_vec(); let local_added_ids = local_target.added_ids().cloned().collect_vec();
let remote_ahead_count = let (remote_ahead_lower, remote_ahead_upper) =
revset::walk_revs(repo.as_ref(), &remote_added_ids, &local_added_ids)?.count(); revset::walk_revs(repo.as_ref(), &remote_added_ids, &local_added_ids)?
let local_ahead_count = .count_estimate();
revset::walk_revs(repo.as_ref(), &local_added_ids, &remote_added_ids)?.count(); let (local_ahead_lower, local_ahead_upper) =
let remote_ahead_message = if remote_ahead_count != 0 { revset::walk_revs(repo.as_ref(), &local_added_ids, &remote_added_ids)?
Some(format!("ahead by {remote_ahead_count} commits")) .count_estimate();
} else { let remote_ahead_message = match remote_ahead_upper {
None 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 { let local_ahead_message = match local_ahead_upper {
Some(format!("behind by {local_ahead_count} commits")) Some(0) => None,
} else { Some(upper) if upper == local_ahead_lower => {
None 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) { match (remote_ahead_message, local_ahead_message) {
(Some(rm), Some(lm)) => { (Some(rm), Some(lm)) => {

View file

@ -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 { fn get_log_output(test_env: &TestEnvironment, cwd: &Path) -> String {
let template = r#"branches ++ " " ++ commit_id.short()"#; let template = r#"branches ++ " " ++ commit_id.short()"#;
test_env.jj_cmd_success(cwd, &["log", "-T", template]) test_env.jj_cmd_success(cwd, &["log", "-T", template])

View file

@ -74,3 +74,4 @@ tokio = { workspace = true, features = ["full"] }
default = [] default = []
vendored-openssl = ["git2/vendored-openssl"] vendored-openssl = ["git2/vendored-openssl"]
watchman = ["dep:tokio", "dep:watchman_client"] watchman = ["dep:tokio", "dep:watchman_client"]
testing = []

View file

@ -130,8 +130,21 @@ impl<I: AsCompositeIndex> Revset for RevsetImpl<I> {
self.entries().next().is_none() self.entries().next().is_none()
} }
fn count(&self) -> usize { fn count_estimate(&self) -> (usize, Option<usize>) {
self.entries().count() 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))
}
} }
} }

View file

@ -2415,7 +2415,11 @@ pub trait Revset: fmt::Debug {
fn is_empty(&self) -> bool; 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<usize>);
} }
pub trait RevsetIteratorExt<'index, I> { pub trait RevsetIteratorExt<'index, I> {