Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions crates/but-api/src/legacy/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,21 @@ use crate::json::HexHash;
pub fn head_info(ctx: &but_ctx::Context) -> Result<but_workspace::RefInfo> {
let repo = ctx.clone_repo_for_merging_non_persisting()?;
let meta = ctx.meta()?;
let gerrit_mode_enabled = repo.git_settings()?.gitbutler_gerrit_mode.unwrap_or(false);
let db = gerrit_mode_enabled
.then(|| ctx.db.get_cache())
.transpose()?;
let gerrit_mode = match db.as_ref() {
Some(db) => but_workspace::ref_info::GerritMode::Enabled(db.gerrit_metadata()),
None => but_workspace::ref_info::GerritMode::Disabled,
};
but_workspace::head_info(
&repo,
&meta,
but_workspace::ref_info::Options {
traversal: but_graph::init::Options::limited(),
expensive_commit_info: true,
gerrit_mode,
},
)
.map(|info| info.pruned_to_entrypoint())
Expand Down
1 change: 1 addition & 0 deletions crates/but-api/src/workspace_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ impl WorkspaceState {
but_workspace::ref_info::Options {
traversal: but_graph::init::Options::limited(),
expensive_commit_info: true,
..Default::default()
},
)?
.pruned_to_entrypoint();
Expand Down
2 changes: 1 addition & 1 deletion crates/but-db/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ pub use table::{
claude::{ClaudeMessage, ClaudePermissionRequest, ClaudeSession},
file_write_locks::FileWriteLock,
workspace_rules::WorkspaceRule,
gerrit_metadata::GerritMeta,
gerrit_metadata::{GerritMeta, GerritMetadataHandle},
forge_reviews::ForgeReview,
ci_checks::CiCheck,
virtual_branches::{VbBranchTarget, VbStack, VbStackHead, VbState, VirtualBranchesSnapshot, VirtualBranchesHandle, VirtualBranchesHandleMut},
Expand Down
2 changes: 2 additions & 0 deletions crates/but-workspace/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ export-schema = ["dep:schemars", "dep:but-schemars", "but-core/export-schema"]

[dependencies]
but-core.workspace = true
but-db.workspace = true
but-graph.workspace = true
but-rebase.workspace = true
but-error.workspace = true
Expand Down Expand Up @@ -68,6 +69,7 @@ insta = { version = "1.47.2", features = ["json"] }
# TODO: remove once `gitbutler-repo` isn't needed anymore.
gitbutler-commit = { workspace = true, features = ["testing"] }
gitbutler-reference.workspace = true
chrono.workspace = true

[lints]
workspace = true
18 changes: 11 additions & 7 deletions crates/but-workspace/src/legacy/stacks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,7 @@ pub fn stacks_v3(
let options = ref_info::Options {
expensive_commit_info: false,
traversal: but_graph::init::Options::limited(),
..Default::default()
};
let info = match ref_name_override {
None => head_info(repo, meta, options),
Expand Down Expand Up @@ -304,11 +305,14 @@ pub fn stack_details_v3(
.into_iter()
.find(|stack| stack.id == Some(stack_id))
}
let mut ref_info_options = ref_info::Options {
// TODO(perf): make this so it can be enabled for a specific stack-id.
expensive_commit_info: true,
traversal: but_graph::init::Options::limited(),
};
fn new_ref_info_options() -> ref_info::Options<'static> {
ref_info::Options {
expensive_commit_info: true,
traversal: but_graph::init::Options::limited(),
..Default::default()
}
}
let mut ref_info_options = new_ref_info_options();
let mut stack = match stack_id {
None => {
// assume single-branch mode.
Expand All @@ -335,7 +339,7 @@ pub fn stack_details_v3(
}
Some(stack_id) => {
if let Some(stack) =
stack_by_id(head_info(repo, meta, ref_info_options.clone())?, stack_id)
stack_by_id(head_info(repo, meta, new_ref_info_options())?, stack_id)
{
stack
} else {
Expand All @@ -349,7 +353,7 @@ pub fn stack_details_v3(
.with_context(|| {
format!("Couldn't find any refs for stack {stack_id} in the repository")
})?;
let ref_info = ref_info(existing_ref, meta, ref_info_options)?;
let ref_info = ref_info(existing_ref, meta, new_ref_info_options())?;
stack_by_id(ref_info, stack_id).with_context(|| {
format!("Really couldn't find {stack_id} in the current workspace projection")
})?
Expand Down
131 changes: 125 additions & 6 deletions crates/but-workspace/src/ref_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use std::{
borrow::Cow,
ops::{Deref, DerefMut},
str::FromStr,
};

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

impl std::fmt::Debug for Commit {
Expand Down Expand Up @@ -80,6 +83,7 @@ impl From<but_core::Commit<'_>> for Commit {
change_id,
refs: Vec::new(),
flags: StackCommitFlags::empty(),
gerrit_review_url: None,
}
}
}
Expand Down Expand Up @@ -114,6 +118,7 @@ impl Commit {
refs: graph_commit.refs.clone(),
flags: graph_commit.flags.into(),
change_id: hdr.and_then(|hdr| hdr.change_id),
gerrit_review_url: None,
}
}
}
Expand Down Expand Up @@ -222,15 +227,48 @@ impl WorkspaceExt for but_graph::Workspace {
}
}

/// Controls whether [`RefInfo`] should be interpreted with Gerrit push metadata.
///
/// Standard `head_info()` derives commit relation and push status from the Git
/// graph: local branch tips, remote-tracking refs, target reachability, and
/// similarity checks. Gerrit mode has an extra source of truth: after a push,
/// GitButler records the Gerrit Change-Id, the patchset commit id accepted by
/// Gerrit, and the review URL in the cache database.
///
/// When enabled, that recorded metadata is applied after the standard graph and
/// similarity pass. This lets `RefInfo` report commits as already present on
/// Gerrit even when there is no normal remote-tracking branch update for
/// `refs/for/*` pushes, and lets the UI link commits back to their Gerrit
/// reviews.
#[derive(Default)]
pub enum GerritMode<'db> {
/// Use only the standard graph-derived `head_info()` data.
#[default]
Disabled,
/// Apply Gerrit metadata from the cache database to commits and push status.
Enabled(but_db::GerritMetadataHandle<'db>),
}

impl std::fmt::Debug for GerritMode<'_> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
GerritMode::Disabled => f.write_str("Disabled"),
GerritMode::Enabled(_) => f.write_str("Enabled(..)"),
}
}
}

/// Options for the [`ref_info()`](crate::ref_info()) call.
#[derive(Default, Debug, Clone)]
pub struct Options {
#[derive(Default, Debug)]
pub struct Options<'db> {
/// Control how to traverse the commit-graph as the basis for the workspace conversion.
pub traversal: but_graph::init::Options,
/// Perform expensive computations on a per-commit basis.
///
/// Note that less expensive checks are still performed.
pub expensive_commit_info: bool,
/// Configure whether Gerrit metadata should augment the standard graph-derived result.
pub gerrit_mode: GerritMode<'db>,
Comment on lines +270 to +271

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can work and even serve as a good pattern for optional data sources.

The alternative would be to pass the whole db as part of the standard constructor, and make this a boolean, which seems more invasive than it has to be.

However, if I were to predict the future, this is exactly what's going to happen as db.cache will be used provide 'cheap' data from the cache right away, and that data is varied enough to not want to split it up into individual handles.

If you buy into that future, pass in db to the constructor, if not, this is perfect to show how scoped this data is right now.

And if I were to follow my own rules, then "don't try to predict the future" 😁.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same decision tree I was going through 😂

I'm happy to start with this because it's just a boilerplate change that a clanker can make for us if/when this future comes into reality

}

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

use anyhow::bail;
use anyhow::{Context as _, bail};
use but_core::{is_workspace_ref_name, ref_metadata::ValueInfo};
use but_graph::{
Graph,
Expand All @@ -377,7 +415,7 @@ use crate::{AncestorWorkspaceCommit, RefInfo, WorkspaceCommit, branch, ui::PushS
pub fn head_info(
repo: &gix::Repository,
meta: &impl but_core::RefMetadata,
opts: Options,
opts: Options<'_>,
) -> anyhow::Result<RefInfo> {
let graph = Graph::from_head(repo, meta, opts.traversal.clone())?;
graph_to_ref_info(&graph.into_workspace()?, repo, opts)
Expand All @@ -395,7 +433,7 @@ pub fn head_info(
pub fn ref_info(
mut existing_ref: gix::Reference<'_>,
meta: &impl but_core::RefMetadata,
opts: Options,
opts: Options<'_>,
) -> anyhow::Result<RefInfo> {
let id = existing_ref.peel_to_id()?;
let repo = id.repo;
Expand Down Expand Up @@ -451,7 +489,7 @@ pub(crate) fn find_ancestor_workspace_commit(
pub fn graph_to_ref_info(
workspace: &but_graph::Workspace,
repo: &gix::Repository,
opts: Options,
opts: Options<'_>,
) -> anyhow::Result<RefInfo> {
if workspace.graph.hard_limit_hit() {
tracing::warn!(hard_limit=?opts.traversal.hard_limit,
Expand Down Expand Up @@ -518,9 +556,90 @@ pub fn graph_to_ref_info(
bail!("{msg}");
}
info.compute_similarity(graph, repo, opts.expensive_commit_info)?;
if let GerritMode::Enabled(metadata) = opts.gerrit_mode {
info.apply_gerrit_metadata(metadata)?;
}
Ok(info)
}

impl RefInfo {
/// Enrich standard `RefInfo` output with Gerrit review metadata.
///
/// The regular construction path has already computed stack shape, commit
/// similarity, integration state, and ordinary push status from refs and
/// graph reachability. Gerrit pushes do not update a branch remote-tracking
/// ref in the same way a normal Git push does, so those graph-only signals
/// are not enough to tell whether a local commit has already been accepted
/// by Gerrit.
///
/// For each local commit, this pass looks up its GitButler Change-Id in the
/// Gerrit metadata table populated after successful pushes. A hit attaches
/// the review URL and, unless the commit is already integrated, marks the
/// commit as `LocalAndRemote(recorded_patchset_commit_id)`. If the recorded
/// patchset id differs from the local commit id, the commit is treated as a
/// rewritten patchset that would require another Gerrit push.
fn apply_gerrit_metadata(
&mut self,
metadata: but_db::GerritMetadataHandle<'_>,
) -> anyhow::Result<()> {
for segment in self
.stacks
.iter_mut()
.flat_map(|stack| stack.segments.iter_mut())
{
for commit in &mut segment.commits {
let Some(meta) = metadata.get(&commit.change_id().to_string())? else {
continue;
};
Comment on lines +585 to +593

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nonsensical. Sqlite is fast with repeated queries, so unless this can be proven to really hurt performance, we shouldn't worry about it.

Especially since gerrit workflows won't have loads and loads of these commits applied at once

commit.inner.gerrit_review_url = Some(meta.review_url);
if !matches!(commit.relation, LocalCommitRelation::Integrated(_)) {
let remote_id =
gix::ObjectId::from_str(&meta.commit_id).with_context(|| {
format!(
"Gerrit metadata for change-id {} has invalid commit id",
meta.change_id
)
})?;
Comment on lines +594 to +602
commit.relation = LocalCommitRelation::LocalAndRemote(remote_id);
}
}
segment.push_status = gerrit_push_status(segment);
}
Ok(())
}
}

/// Derive push status for a Gerrit-enriched segment.
///
/// Standard push status compares local branches with their remote-tracking
/// branches. Gerrit mode instead compares local commits with the patchset
/// commit ids recorded in Gerrit metadata. A matching id means the current
/// local commit is already pushed as-is, a different recorded id means the
/// local commit has been rewritten since the last Gerrit patchset, and a
/// local-only commit still needs to be pushed.
fn gerrit_push_status(segment: &crate::ref_info::Segment) -> PushStatus {
let has_local_only = segment
.commits
.iter()
.any(|commit| matches!(commit.relation, LocalCommitRelation::LocalOnly));
let has_diverged = segment.commits.iter().any(|commit| {
matches!(commit.relation, LocalCommitRelation::LocalAndRemote(remote_id) if commit.id != remote_id)
});
let all_pushed = segment.commits.iter().all(|commit| {
matches!(commit.relation, LocalCommitRelation::LocalAndRemote(remote_id) if commit.id == remote_id)
});

if has_diverged {
PushStatus::UnpushedCommitsRequiringForce
} else if has_local_only {
PushStatus::UnpushedCommits
} else if all_pushed {
PushStatus::NothingToPush
} else {
segment.push_status
}
}

impl branch::Stack {
fn try_from_graph_stack(
stack: &but_graph::workspace::Stack,
Expand Down
4 changes: 3 additions & 1 deletion crates/but-workspace/src/ui/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,7 @@ impl From<&crate::ref_info::Commit> for ui::UpstreamCommit {
// TODO: Represent this in the UI (maybe) and/or deal with divergence of the local and remote tracking branch.
has_conflicts: _,
change_id,
gerrit_review_url: _,
}: &crate::ref_info::Commit,
) -> Self {
ui::UpstreamCommit {
Expand Down Expand Up @@ -417,6 +418,7 @@ impl From<&LocalCommit> for ui::Commit {
flags: _,
has_conflicts,
change_id,
gerrit_review_url,
},
relation,
}: &LocalCommit,
Expand All @@ -437,7 +439,7 @@ impl From<&LocalCommit> for ui::Commit {
.unwrap_or_else(|| {
but_core::commit::Headers::synthetic_change_id_from_commit_id(*id).to_string()
}),
gerrit_review_url: None,
gerrit_review_url: gerrit_review_url.clone(),
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion crates/but-workspace/tests/workspace/ref_info/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -869,10 +869,11 @@ mod utils {
Ok((tmp, repo, meta))
}

pub fn standard_options() -> but_workspace::ref_info::Options {
pub fn standard_options() -> but_workspace::ref_info::Options<'static> {
ref_info::Options {
expensive_commit_info: true,
traversal: Default::default(),
..Default::default()
}
Comment on lines +872 to 877

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same, this is a test context.

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ mod utils {
pub fn standard_options_with_extra_target(
repo: &gix::Repository,
revspec: &str,
) -> but_workspace::ref_info::Options {
) -> but_workspace::ref_info::Options<'static> {
but_workspace::ref_info::Options {
traversal: but_graph::init::Options {
extra_target_commit_id: repo.rev_parse_single(revspec).unwrap().detach().into(),
Expand Down
Loading
Loading