From 34976fa936c5b69b986d7b3e9fd712cec21b5ec4 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 22 Mar 2022 09:10:29 -0500 Subject: [PATCH 1/5] fix(cli): Find a good-enough base when none Originally, we would bail out if there is no base. We might split off branches that are very-divergent branches from their base, so we need to make this better. The first step was allowing it. This is the next where we infer a "good enough" base to probide context for the user. --- src/bin/git-stack/stack.rs | 9 +++++++-- src/git/branches.rs | 23 +++++++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/src/bin/git-stack/stack.rs b/src/bin/git-stack/stack.rs index 724684f..692640b 100644 --- a/src/bin/git-stack/stack.rs +++ b/src/bin/git-stack/stack.rs @@ -808,8 +808,13 @@ fn resolve_implicit_base( AnnotatedOid::with_branch(branch.to_owned()) } None => { - log::warn!("Could not find protected branch for {}", head_oid); - AnnotatedOid::new(head_oid) + let assumed_base_oid = git_stack::git::infer_base(repo, head_oid).unwrap_or(head_oid); + log::warn!( + "Could not find protected branch for {}, assuming {}", + head_oid, + assumed_base_oid + ); + AnnotatedOid::new(assumed_base_oid) } }; branch diff --git a/src/git/branches.rs b/src/git/branches.rs index 083a42e..2b92af0 100644 --- a/src/git/branches.rs +++ b/src/git/branches.rs @@ -279,3 +279,26 @@ pub fn find_protected_base<'b>( None } + +pub fn infer_base(repo: &dyn crate::git::Repo, head_oid: git2::Oid) -> Option { + let head_commit = repo.find_commit(head_oid)?; + let head_committer = head_commit.committer.clone(); + + let mut next_oid = head_oid; + loop { + let next_commit = repo.find_commit(next_oid)?; + if next_commit.committer != head_committer { + return Some(next_oid); + } + let parent_ids = repo.parent_ids(next_oid).ok()?; + match parent_ids.len() { + 1 => { + next_oid = parent_ids[0]; + } + _ => { + // Assume merge-commits are topic branches being merged into the upstream + return Some(next_oid); + } + } + } +} From 264a7f979cf417cd55d107c20552286914d30bab Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 22 Mar 2022 11:00:32 -0500 Subject: [PATCH 2/5] refactor(git): Make merge-base cache more resilient --- src/git/repo.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/git/repo.rs b/src/git/repo.rs index 64e03fa..0033660 100644 --- a/src/git/repo.rs +++ b/src/git/repo.rs @@ -194,11 +194,12 @@ impl GitRepo { return Some(one); } + let (smaller, larger) = if one < two { (one, two) } else { (two, one) }; *self .bases .borrow_mut() - .entry((one, two)) - .or_insert_with(|| self.repo.merge_base(one, two).ok()) + .entry((smaller, larger)) + .or_insert_with(|| self.repo.merge_base(smaller, larger).ok()) } pub fn find_commit(&self, id: git2::Oid) -> Option> { From ac704e62c9e0fc14ebebc6005872663369402b22 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 22 Mar 2022 12:03:47 -0500 Subject: [PATCH 3/5] refactor(git): Split low-level/high-level --- src/git/repo.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/git/repo.rs b/src/git/repo.rs index 0033660..6e32784 100644 --- a/src/git/repo.rs +++ b/src/git/repo.rs @@ -199,7 +199,11 @@ impl GitRepo { .bases .borrow_mut() .entry((smaller, larger)) - .or_insert_with(|| self.repo.merge_base(smaller, larger).ok()) + .or_insert_with(|| self.merge_base_raw(smaller, larger)) + } + + fn merge_base_raw(&self, one: git2::Oid, two: git2::Oid) -> Option { + self.repo.merge_base(one, two).ok() } pub fn find_commit(&self, id: git2::Oid) -> Option> { From 8227f349e10647a1efae3d9893c0ce90d6b9b4f5 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 22 Mar 2022 12:14:33 -0500 Subject: [PATCH 4/5] perf: Cache commit counts --- src/git/repo.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/git/repo.rs b/src/git/repo.rs index 6e32784..5f93575 100644 --- a/src/git/repo.rs +++ b/src/git/repo.rs @@ -124,6 +124,7 @@ pub struct GitRepo { commits: std::cell::RefCell>>, interned_strings: std::cell::RefCell>>, bases: std::cell::RefCell>>, + counts: std::cell::RefCell>>, } impl GitRepo { @@ -135,6 +136,7 @@ impl GitRepo { commits: Default::default(), interned_strings: Default::default(), bases: Default::default(), + counts: Default::default(), } } @@ -289,6 +291,14 @@ impl GitRepo { return Some(0); } + *self + .counts + .borrow_mut() + .entry((base_id, head_id)) + .or_insert_with(|| self.commit_count_raw(base_id, head_id)) + } + + fn commit_count_raw(&self, base_id: git2::Oid, head_id: git2::Oid) -> Option { let merge_base_id = self.merge_base(base_id, head_id)?; if merge_base_id != base_id { return None; From a99bccaba061a5398bf278fc4d79a45ba9868dca Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 23 Mar 2022 12:26:43 -0500 Subject: [PATCH 5/5] perf(cli): Split off highly divergent branches In #216, a user is backporting changes on Linux and hadn't marked protected branches which would serve as bases, causing git-stack to topologically sort commits that are very distant from each other, causing a simple showing of the commit graph to take 20-30s. This change infers that these branches are do not belong to their base and split them off. Now showing the commit graph takes about 6s as it tries each base. --- docs/reference.md | 1 + src/bin/git-stack/args.rs | 1 + src/bin/git-stack/stack.rs | 52 ++++++++++++++++++++++++++++++++++---- src/config.rs | 28 ++++++++++++++++++++ 4 files changed, 77 insertions(+), 5 deletions(-) diff --git a/docs/reference.md b/docs/reference.md index 7f06c17..dd40481 100644 --- a/docs/reference.md +++ b/docs/reference.md @@ -129,6 +129,7 @@ Configuration is read from the following (in precedence order): | stack.protected-branch | \- | multivar of globs | Branch names that match these globs (`.gitignore` syntax) are considered protected branches | | stack.protect-commit-count | \- | integer | Protect commits that are on a branch with `count`+ commits | | stack.protect-commit-age | \- | time delta (e.g. 10days) | Protect commits that older than the specified time | +| stack.auto-base-commit-count | \- | integer | Split off branches that are more than `count` commits away from the implied base | | stack.stack | --stack | "current", "dependents", "descendants", "all" | Which development branch-stacks to operate on | | stack.push-remote | \- | string | Development remote for pushing local branches | | stack.pull-remote | \- | string | Upstream remote for pulling protected branches | diff --git a/src/bin/git-stack/args.rs b/src/bin/git-stack/args.rs index f402538..3b8d788 100644 --- a/src/bin/git-stack/args.rs +++ b/src/bin/git-stack/args.rs @@ -89,6 +89,7 @@ impl Args { protected_branches: None, protect_commit_count: None, protect_commit_age: None, + auto_base_commit_count: None, stack: self.stack, push_remote: None, pull_remote: None, diff --git a/src/bin/git-stack/stack.rs b/src/bin/git-stack/stack.rs index 692640b..90b641d 100644 --- a/src/bin/git-stack/stack.rs +++ b/src/bin/git-stack/stack.rs @@ -135,8 +135,13 @@ impl State { (None, None, git_stack::config::Stack::All) => { let mut stack_branches = std::collections::BTreeMap::new(); for (branch_id, branch) in branches.iter() { - let base_branch = - resolve_implicit_base(&repo, branch_id, &branches, &protected_branches); + let base_branch = resolve_implicit_base( + &repo, + branch_id, + &branches, + &protected_branches, + repo_config.auto_base_commit_count(), + ); stack_branches .entry(base_branch) .or_insert_with(git_stack::git::Branches::default) @@ -163,6 +168,7 @@ impl State { head_commit.id, &branches, &protected_branches, + repo_config.auto_base_commit_count(), ); // HACK: Since `base` might have come back with a remote branch, treat it as an // "onto" to find the local version. @@ -175,6 +181,7 @@ impl State { head_commit.id, &branches, &protected_branches, + repo_config.auto_base_commit_count(), ); let base = resolve_base_from_onto(&repo, &onto); (base, onto) @@ -787,9 +794,45 @@ fn resolve_implicit_base( head_oid: git2::Oid, branches: &git_stack::git::Branches, protected_branches: &git_stack::git::Branches, + auto_base_commit_count: Option, ) -> AnnotatedOid { - let branch = match git_stack::git::find_protected_base(repo, protected_branches, head_oid) { + match git_stack::git::find_protected_base(repo, protected_branches, head_oid) { Some(branch) => { + let merge_base_id = repo + .merge_base(branch.id, head_oid) + .expect("to be a base, there must be a merge base"); + if let Some(max_commit_count) = auto_base_commit_count { + let ahead_count = repo + .commit_count(merge_base_id, head_oid) + .expect("merge_base should ensure a count exists "); + let behind_count = repo + .commit_count(merge_base_id, branch.id) + .expect("merge_base should ensure a count exists "); + if max_commit_count <= ahead_count + behind_count { + let assumed_base_oid = + git_stack::git::infer_base(repo, head_oid).unwrap_or(head_oid); + log::warn!( + "{} is {} ahead and {} behind {}, using {} as --base instead", + branches + .get(head_oid) + .map(|b| b[0].to_string()) + .or_else(|| { + repo.find_commit(head_oid)? + .summary + .to_str() + .ok() + .map(ToOwned::to_owned) + }) + .unwrap_or_else(|| "target".to_owned()), + ahead_count, + behind_count, + branch, + assumed_base_oid + ); + return AnnotatedOid::new(assumed_base_oid); + } + } + log::debug!( "Chose branch {} as the base for {}", branch, @@ -816,8 +859,7 @@ fn resolve_implicit_base( ); AnnotatedOid::new(assumed_base_oid) } - }; - branch + } } fn resolve_base_from_onto(repo: &git_stack::git::GitRepo, onto: &AnnotatedOid) -> AnnotatedOid { diff --git a/src/config.rs b/src/config.rs index eba1752..75182e9 100644 --- a/src/config.rs +++ b/src/config.rs @@ -5,6 +5,7 @@ pub struct RepoConfig { pub protected_branches: Option>, pub protect_commit_count: Option, pub protect_commit_age: Option, + pub auto_base_commit_count: Option, pub stack: Option, pub push_remote: Option, pub pull_remote: Option, @@ -20,6 +21,7 @@ pub struct RepoConfig { static PROTECTED_STACK_FIELD: &str = "stack.protected-branch"; static PROTECT_COMMIT_COUNT: &str = "stack.protect-commit-count"; static PROTECT_COMMIT_AGE: &str = "stack.protect-commit-age"; +static AUTO_BASE_COMMIT_COUNT: &str = "stack.auto-base-commit-count"; static STACK_FIELD: &str = "stack.stack"; static PUSH_REMOTE_FIELD: &str = "stack.push-remote"; static PULL_REMOTE_FIELD: &str = "stack.pull-remote"; @@ -34,6 +36,7 @@ static DEFAULT_PROTECTED_BRANCHES: [&str; 4] = ["main", "master", "dev", "stable static DEFAULT_PROTECT_COMMIT_COUNT: usize = 50; static DEFAULT_PROTECT_COMMIT_AGE: std::time::Duration = std::time::Duration::from_secs(60 * 60 * 24 * 14); +static DEFAULT_AUTO_BASE_COMMIT_COUNT: usize = 500; const DEFAULT_CAPACITY: usize = 30; impl RepoConfig { @@ -132,6 +135,10 @@ impl RepoConfig { { config.protect_commit_age = Some(value); } + } else if key == AUTO_BASE_COMMIT_COUNT { + if let Some(value) = value.as_ref().and_then(|v| FromStr::from_str(v).ok()) { + config.auto_base_commit_count = Some(value); + } } else if key == STACK_FIELD { if let Some(value) = value.as_ref().and_then(|v| FromStr::from_str(v).ok()) { config.stack = Some(value); @@ -190,6 +197,7 @@ impl RepoConfig { let mut conf = Self::default(); conf.protect_commit_count = Some(conf.protect_commit_count().unwrap_or(0)); conf.protect_commit_age = Some(conf.protect_commit_age()); + conf.auto_base_commit_count = Some(conf.auto_base_commit_count().unwrap_or(0)); conf.stack = Some(conf.stack()); conf.push_remote = Some(conf.push_remote().to_owned()); conf.pull_remote = Some(conf.pull_remote().to_owned()); @@ -240,6 +248,11 @@ impl RepoConfig { .ok() .and_then(|s| humantime::parse_duration(&s).ok()); + let auto_base_commit_count = config + .get_i64(AUTO_BASE_COMMIT_COUNT) + .ok() + .map(|i| i.max(0) as usize); + let push_remote = config .get_string(PUSH_REMOTE_FIELD) .ok() @@ -279,6 +292,7 @@ impl RepoConfig { protected_branches, protect_commit_count, protect_commit_age, + auto_base_commit_count, push_remote, pull_remote, stack, @@ -320,6 +334,7 @@ impl RepoConfig { } self.protect_commit_count = other.protect_commit_count.or(self.protect_commit_count); self.protect_commit_age = other.protect_commit_age.or(self.protect_commit_age); + self.auto_base_commit_count = other.auto_base_commit_count.or(self.auto_base_commit_count); self.push_remote = other.push_remote.or(self.push_remote); self.pull_remote = other.pull_remote.or(self.pull_remote); self.stack = other.stack.or(self.stack); @@ -349,6 +364,13 @@ impl RepoConfig { .unwrap_or(DEFAULT_PROTECT_COMMIT_AGE) } + pub fn auto_base_commit_count(&self) -> Option { + let auto_base_commit_count = self + .auto_base_commit_count + .unwrap_or(DEFAULT_AUTO_BASE_COMMIT_COUNT); + (auto_base_commit_count != 0).then(|| auto_base_commit_count) + } + pub fn push_remote(&self) -> &str { self.push_remote.as_deref().unwrap_or("origin") } @@ -412,6 +434,12 @@ impl std::fmt::Display for RepoConfig { PROTECT_COMMIT_AGE.split_once('.').unwrap().1, humantime::format_duration(self.protect_commit_age()) )?; + writeln!( + f, + "\t{}={}", + AUTO_BASE_COMMIT_COUNT.split_once('.').unwrap().1, + self.auto_base_commit_count().unwrap_or(0) + )?; writeln!( f, "\t{}={}",