Skip to content

Commit deaeac0

Browse files
committed
Head info handles gerrit metadata
This brings the old gerrit-mode logic that previously was only present on the super legacy `stacks_details` function, and brings in into the `RefInfo` construction. I realise that this is the first time bringing the database into the `but-workspace` crate, but in this instance, it felt reasonable to provide a specific `GerritMetadataHandle` directly rather than adding more abstraction layers over it.
1 parent 5fd9c9b commit deaeac0

13 files changed

Lines changed: 303 additions & 93 deletions

File tree

Cargo.lock

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

crates/but-api/src/legacy/workspace.rs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,12 +30,21 @@ use crate::json::HexHash;
3030
pub fn head_info(ctx: &but_ctx::Context) -> Result<but_workspace::RefInfo> {
3131
let repo = ctx.clone_repo_for_merging_non_persisting()?;
3232
let meta = ctx.meta()?;
33+
let gerrit_mode_enabled = repo.git_settings()?.gitbutler_gerrit_mode.unwrap_or(false);
34+
let db = gerrit_mode_enabled
35+
.then(|| ctx.db.get_cache())
36+
.transpose()?;
37+
let gerrit_mode = match db.as_ref() {
38+
Some(db) => but_workspace::ref_info::GerritMode::Enabled(db.gerrit_metadata()),
39+
None => but_workspace::ref_info::GerritMode::Disabled,
40+
};
3341
but_workspace::head_info(
3442
&repo,
3543
&meta,
3644
but_workspace::ref_info::Options {
3745
traversal: but_graph::init::Options::limited(),
3846
expensive_commit_info: true,
47+
gerrit_mode,
3948
},
4049
)
4150
.map(|info| info.pruned_to_entrypoint())

crates/but-api/src/workspace_state.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ impl WorkspaceState {
3838
but_workspace::ref_info::Options {
3939
traversal: but_graph::init::Options::limited(),
4040
expensive_commit_info: true,
41+
..Default::default()
4142
},
4243
)?
4344
.pruned_to_entrypoint();

crates/but-db/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ pub use table::{
7676
claude::{ClaudeMessage, ClaudePermissionRequest, ClaudeSession},
7777
file_write_locks::FileWriteLock,
7878
workspace_rules::WorkspaceRule,
79-
gerrit_metadata::GerritMeta,
79+
gerrit_metadata::{GerritMeta, GerritMetadataHandle},
8080
forge_reviews::ForgeReview,
8181
ci_checks::CiCheck,
8282
virtual_branches::{VbBranchTarget, VbStack, VbStackHead, VbState, VirtualBranchesSnapshot, VirtualBranchesHandle, VirtualBranchesHandleMut},

crates/but-workspace/Cargo.toml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ export-schema = ["dep:schemars", "dep:but-schemars", "but-core/export-schema"]
2929

3030
[dependencies]
3131
but-core.workspace = true
32+
but-db.workspace = true
3233
but-graph.workspace = true
3334
but-rebase.workspace = true
3435
but-error.workspace = true
@@ -68,6 +69,7 @@ insta = { version = "1.47.2", features = ["json"] }
6869
# TODO: remove once `gitbutler-repo` isn't needed anymore.
6970
gitbutler-commit = { workspace = true, features = ["testing"] }
7071
gitbutler-reference.workspace = true
72+
chrono.workspace = true
7173

7274
[lints]
7375
workspace = true

crates/but-workspace/src/legacy/stacks.rs

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -229,6 +229,7 @@ pub fn stacks_v3(
229229
let options = ref_info::Options {
230230
expensive_commit_info: false,
231231
traversal: but_graph::init::Options::limited(),
232+
..Default::default()
232233
};
233234
let info = match ref_name_override {
234235
None => head_info(repo, meta, options),
@@ -304,11 +305,14 @@ pub fn stack_details_v3(
304305
.into_iter()
305306
.find(|stack| stack.id == Some(stack_id))
306307
}
307-
let mut ref_info_options = ref_info::Options {
308-
// TODO(perf): make this so it can be enabled for a specific stack-id.
309-
expensive_commit_info: true,
310-
traversal: but_graph::init::Options::limited(),
311-
};
308+
fn new_ref_info_options() -> ref_info::Options<'static> {
309+
ref_info::Options {
310+
expensive_commit_info: true,
311+
traversal: but_graph::init::Options::limited(),
312+
..Default::default()
313+
}
314+
}
315+
let mut ref_info_options = new_ref_info_options();
312316
let mut stack = match stack_id {
313317
None => {
314318
// assume single-branch mode.
@@ -335,7 +339,7 @@ pub fn stack_details_v3(
335339
}
336340
Some(stack_id) => {
337341
if let Some(stack) =
338-
stack_by_id(head_info(repo, meta, ref_info_options.clone())?, stack_id)
342+
stack_by_id(head_info(repo, meta, new_ref_info_options())?, stack_id)
339343
{
340344
stack
341345
} else {
@@ -349,7 +353,7 @@ pub fn stack_details_v3(
349353
.with_context(|| {
350354
format!("Couldn't find any refs for stack {stack_id} in the repository")
351355
})?;
352-
let ref_info = ref_info(existing_ref, meta, ref_info_options)?;
356+
let ref_info = ref_info(existing_ref, meta, new_ref_info_options())?;
353357
stack_by_id(ref_info, stack_id).with_context(|| {
354358
format!("Really couldn't find {stack_id} in the current workspace projection")
355359
})?

crates/but-workspace/src/ref_info.rs

Lines changed: 125 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
use std::{
66
borrow::Cow,
77
ops::{Deref, DerefMut},
8+
str::FromStr,
89
};
910

1011
use bstr::BString;
@@ -45,6 +46,8 @@ pub struct Commit {
4546
/// if it's not stored in the Commit. Use [`Self::change_id()`] to always get the change id,
4647
/// if necessary, by deriving it from the commit hash itself.
4748
pub change_id: Option<but_core::ChangeId>,
49+
/// Optional URL to the Gerrit review for this commit, if applicable.
50+
pub gerrit_review_url: Option<String>,
4851
}
4952

5053
impl std::fmt::Debug for Commit {
@@ -80,6 +83,7 @@ impl From<but_core::Commit<'_>> for Commit {
8083
change_id,
8184
refs: Vec::new(),
8285
flags: StackCommitFlags::empty(),
86+
gerrit_review_url: None,
8387
}
8488
}
8589
}
@@ -114,6 +118,7 @@ impl Commit {
114118
refs: graph_commit.refs.clone(),
115119
flags: graph_commit.flags.into(),
116120
change_id: hdr.and_then(|hdr| hdr.change_id),
121+
gerrit_review_url: None,
117122
}
118123
}
119124
}
@@ -222,15 +227,48 @@ impl WorkspaceExt for but_graph::Workspace {
222227
}
223228
}
224229

230+
/// Controls whether [`RefInfo`] should be interpreted with Gerrit push metadata.
231+
///
232+
/// Standard `head_info()` derives commit relation and push status from the Git
233+
/// graph: local branch tips, remote-tracking refs, target reachability, and
234+
/// similarity checks. Gerrit mode has an extra source of truth: after a push,
235+
/// GitButler records the Gerrit Change-Id, the patchset commit id accepted by
236+
/// Gerrit, and the review URL in the cache database.
237+
///
238+
/// When enabled, that recorded metadata is applied after the standard graph and
239+
/// similarity pass. This lets `RefInfo` report commits as already present on
240+
/// Gerrit even when there is no normal remote-tracking branch update for
241+
/// `refs/for/*` pushes, and lets the UI link commits back to their Gerrit
242+
/// reviews.
243+
#[derive(Default)]
244+
pub enum GerritMode<'db> {
245+
/// Use only the standard graph-derived `head_info()` data.
246+
#[default]
247+
Disabled,
248+
/// Apply Gerrit metadata from the cache database to commits and push status.
249+
Enabled(but_db::GerritMetadataHandle<'db>),
250+
}
251+
252+
impl std::fmt::Debug for GerritMode<'_> {
253+
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
254+
match self {
255+
GerritMode::Disabled => f.write_str("Disabled"),
256+
GerritMode::Enabled(_) => f.write_str("Enabled(..)"),
257+
}
258+
}
259+
}
260+
225261
/// Options for the [`ref_info()`](crate::ref_info()) call.
226-
#[derive(Default, Debug, Clone)]
227-
pub struct Options {
262+
#[derive(Default, Debug)]
263+
pub struct Options<'db> {
228264
/// Control how to traverse the commit-graph as the basis for the workspace conversion.
229265
pub traversal: but_graph::init::Options,
230266
/// Perform expensive computations on a per-commit basis.
231267
///
232268
/// Note that less expensive checks are still performed.
233269
pub expensive_commit_info: bool,
270+
/// Configure whether Gerrit metadata should augment the standard graph-derived result.
271+
pub gerrit_mode: GerritMode<'db>,
234272
}
235273

236274
/// A segment of a commit graph, representing a set of commits exclusively.
@@ -358,7 +396,7 @@ impl std::fmt::Debug for Segment {
358396
}
359397
}
360398

361-
use anyhow::bail;
399+
use anyhow::{Context as _, bail};
362400
use but_core::{is_workspace_ref_name, ref_metadata::ValueInfo};
363401
use but_graph::{
364402
Graph,
@@ -377,7 +415,7 @@ use crate::{AncestorWorkspaceCommit, RefInfo, WorkspaceCommit, branch, ui::PushS
377415
pub fn head_info(
378416
repo: &gix::Repository,
379417
meta: &impl but_core::RefMetadata,
380-
opts: Options,
418+
opts: Options<'_>,
381419
) -> anyhow::Result<RefInfo> {
382420
let graph = Graph::from_head(repo, meta, opts.traversal.clone())?;
383421
graph_to_ref_info(&graph.into_workspace()?, repo, opts)
@@ -395,7 +433,7 @@ pub fn head_info(
395433
pub fn ref_info(
396434
mut existing_ref: gix::Reference<'_>,
397435
meta: &impl but_core::RefMetadata,
398-
opts: Options,
436+
opts: Options<'_>,
399437
) -> anyhow::Result<RefInfo> {
400438
let id = existing_ref.peel_to_id()?;
401439
let repo = id.repo;
@@ -451,7 +489,7 @@ pub(crate) fn find_ancestor_workspace_commit(
451489
pub fn graph_to_ref_info(
452490
workspace: &but_graph::Workspace,
453491
repo: &gix::Repository,
454-
opts: Options,
492+
opts: Options<'_>,
455493
) -> anyhow::Result<RefInfo> {
456494
if workspace.graph.hard_limit_hit() {
457495
tracing::warn!(hard_limit=?opts.traversal.hard_limit,
@@ -518,9 +556,90 @@ pub fn graph_to_ref_info(
518556
bail!("{msg}");
519557
}
520558
info.compute_similarity(graph, repo, opts.expensive_commit_info)?;
559+
if let GerritMode::Enabled(metadata) = opts.gerrit_mode {
560+
info.apply_gerrit_metadata(metadata)?;
561+
}
521562
Ok(info)
522563
}
523564

565+
impl RefInfo {
566+
/// Enrich standard `RefInfo` output with Gerrit review metadata.
567+
///
568+
/// The regular construction path has already computed stack shape, commit
569+
/// similarity, integration state, and ordinary push status from refs and
570+
/// graph reachability. Gerrit pushes do not update a branch remote-tracking
571+
/// ref in the same way a normal Git push does, so those graph-only signals
572+
/// are not enough to tell whether a local commit has already been accepted
573+
/// by Gerrit.
574+
///
575+
/// For each local commit, this pass looks up its GitButler Change-Id in the
576+
/// Gerrit metadata table populated after successful pushes. A hit attaches
577+
/// the review URL and, unless the commit is already integrated, marks the
578+
/// commit as `LocalAndRemote(recorded_patchset_commit_id)`. If the recorded
579+
/// patchset id differs from the local commit id, the commit is treated as a
580+
/// rewritten patchset that would require another Gerrit push.
581+
fn apply_gerrit_metadata(
582+
&mut self,
583+
metadata: but_db::GerritMetadataHandle<'_>,
584+
) -> anyhow::Result<()> {
585+
for segment in self
586+
.stacks
587+
.iter_mut()
588+
.flat_map(|stack| stack.segments.iter_mut())
589+
{
590+
for commit in &mut segment.commits {
591+
let Some(meta) = metadata.get(&commit.change_id().to_string())? else {
592+
continue;
593+
};
594+
commit.inner.gerrit_review_url = Some(meta.review_url);
595+
if !matches!(commit.relation, LocalCommitRelation::Integrated(_)) {
596+
let remote_id =
597+
gix::ObjectId::from_str(&meta.commit_id).with_context(|| {
598+
format!(
599+
"Gerrit metadata for change-id {} has invalid commit id",
600+
meta.change_id
601+
)
602+
})?;
603+
commit.relation = LocalCommitRelation::LocalAndRemote(remote_id);
604+
}
605+
}
606+
segment.push_status = gerrit_push_status(segment);
607+
}
608+
Ok(())
609+
}
610+
}
611+
612+
/// Derive push status for a Gerrit-enriched segment.
613+
///
614+
/// Standard push status compares local branches with their remote-tracking
615+
/// branches. Gerrit mode instead compares local commits with the patchset
616+
/// commit ids recorded in Gerrit metadata. A matching id means the current
617+
/// local commit is already pushed as-is, a different recorded id means the
618+
/// local commit has been rewritten since the last Gerrit patchset, and a
619+
/// local-only commit still needs to be pushed.
620+
fn gerrit_push_status(segment: &crate::ref_info::Segment) -> PushStatus {
621+
let has_local_only = segment
622+
.commits
623+
.iter()
624+
.any(|commit| matches!(commit.relation, LocalCommitRelation::LocalOnly));
625+
let has_diverged = segment.commits.iter().any(|commit| {
626+
matches!(commit.relation, LocalCommitRelation::LocalAndRemote(remote_id) if commit.id != remote_id)
627+
});
628+
let all_pushed = segment.commits.iter().all(|commit| {
629+
matches!(commit.relation, LocalCommitRelation::LocalAndRemote(remote_id) if commit.id == remote_id)
630+
});
631+
632+
if has_diverged {
633+
PushStatus::UnpushedCommitsRequiringForce
634+
} else if has_local_only {
635+
PushStatus::UnpushedCommits
636+
} else if all_pushed {
637+
PushStatus::NothingToPush
638+
} else {
639+
segment.push_status
640+
}
641+
}
642+
524643
impl branch::Stack {
525644
fn try_from_graph_stack(
526645
stack: &but_graph::workspace::Stack,

crates/but-workspace/src/ui/mod.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -387,6 +387,7 @@ impl From<&crate::ref_info::Commit> for ui::UpstreamCommit {
387387
// TODO: Represent this in the UI (maybe) and/or deal with divergence of the local and remote tracking branch.
388388
has_conflicts: _,
389389
change_id,
390+
gerrit_review_url: _,
390391
}: &crate::ref_info::Commit,
391392
) -> Self {
392393
ui::UpstreamCommit {
@@ -417,6 +418,7 @@ impl From<&LocalCommit> for ui::Commit {
417418
flags: _,
418419
has_conflicts,
419420
change_id,
421+
gerrit_review_url,
420422
},
421423
relation,
422424
}: &LocalCommit,
@@ -437,7 +439,7 @@ impl From<&LocalCommit> for ui::Commit {
437439
.unwrap_or_else(|| {
438440
but_core::commit::Headers::synthetic_change_id_from_commit_id(*id).to_string()
439441
}),
440-
gerrit_review_url: None,
442+
gerrit_review_url: gerrit_review_url.clone(),
441443
}
442444
}
443445
}

crates/but-workspace/tests/workspace/ref_info/mod.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -857,10 +857,11 @@ mod utils {
857857
Ok((tmp, repo, meta))
858858
}
859859

860-
pub fn standard_options() -> but_workspace::ref_info::Options {
860+
pub fn standard_options() -> but_workspace::ref_info::Options<'static> {
861861
ref_info::Options {
862862
expensive_commit_info: true,
863863
traversal: Default::default(),
864+
..Default::default()
864865
}
865866
}
866867
}

crates/but-workspace/tests/workspace/ref_info/with_workspace_commit/journey/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ mod utils {
77
pub fn standard_options_with_extra_target(
88
repo: &gix::Repository,
99
revspec: &str,
10-
) -> but_workspace::ref_info::Options {
10+
) -> but_workspace::ref_info::Options<'static> {
1111
but_workspace::ref_info::Options {
1212
traversal: but_graph::init::Options {
1313
extra_target_commit_id: repo.rev_parse_single(revspec).unwrap().detach().into(),

0 commit comments

Comments
 (0)