Head info handles gerrit metadata#13905
Conversation
Byron
left a comment
Byron
left a comment
There was a problem hiding this comment.
I think this is great, particularly the added docs.
Overall, having the db in involved in the RefInfo construction is inevitable.
Once meta is part of db, what could happen is that db is passed directly and it can get the handles it needs internally for convenience. Technically it's better to only pass what's actually used, so maybe that's the route we take instead, and it's a route now established by this PR. That's a good thing!
| }; | ||
| fn new_ref_info_options() -> ref_info::Options<'static> { | ||
| ref_info::Options { | ||
| // TODO(perf): make this so it can be enabled for a specific stack-id. |
There was a problem hiding this comment.
I think this TODO can be removed by now, as the whole "expensive operations" problem will benefit for a more general solution.
There was a problem hiding this comment.
Cool! I'll snip it out.
| /// Configure whether Gerrit metadata should augment the standard graph-derived result. | ||
| pub gerrit_mode: GerritMode<'db>, |
There was a problem hiding this comment.
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" 😁.
There was a problem hiding this comment.
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
Caleb-T-Owens
force-pushed
the
caleb/head-info-has-gerrit-metadata
branch
from
cec2bc9 to
e27c1f1
Compare
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.
Caleb-T-Owens
force-pushed
the
caleb/head-info-has-gerrit-metadata
branch
from
e27c1f1 to
deaeac0
Compare
There was a problem hiding this comment.
Pull request overview
This PR extends but-workspace’s RefInfo construction to optionally incorporate Gerrit push metadata (stored in the cache DB) so commit relations, push status, and UI commit review URLs can reflect refs/for/* pushes that don’t update normal remote-tracking refs.
Changes:
- Introduces
GerritModeand threads it throughbut_workspace::ref_info::Optionsto enable/disable Gerrit metadata enrichment. - Enriches
RefInfolocal commits withgerrit_review_urland derives Gerrit-aware push status from recorded patchset commit IDs. - Adds workspace tests covering Gerrit enrichment behavior and updates call sites to use
..Default::default()for the new options field.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/gitbutler-branch-actions/src/upstream_integration.rs | Updates ref_info::Options construction to include defaults (new options field compatibility). |
| crates/gitbutler-branch-actions/src/branch.rs | Updates ref_info::Options construction to include defaults (new options field compatibility). |
| crates/but-workspace/tests/workspace/ref_info/with_workspace_commit/mod.rs | Adds Gerrit-mode tests for review URL propagation and Gerrit-aware push status. |
| crates/but-workspace/tests/workspace/ref_info/with_workspace_commit/journey/mod.rs | Updates test utility options return type for the new Options<'db> signature. |
| crates/but-workspace/tests/workspace/ref_info/mod.rs | Updates test utilities to return Options<'static> and include new defaults. |
| crates/but-workspace/src/ui/mod.rs | Plumbs gerrit_review_url from ref_info local commits into UI commit model. |
| crates/but-workspace/src/ref_info.rs | Adds GerritMode, gerrit_review_url field, and Gerrit metadata application pass + push status derivation. |
| crates/but-workspace/src/legacy/stacks.rs | Updates legacy paths to build ref_info::Options with defaults and avoid cloning options. |
| crates/but-workspace/Cargo.toml | Adds but-db dependency (and chrono for tests) to support Gerrit metadata lookup. |
| crates/but-db/src/lib.rs | Re-exports GerritMetadataHandle for consumers. |
| crates/but-api/src/workspace_state.rs | Updates ref_info::Options initialization to include defaults. |
| crates/but-api/src/legacy/workspace.rs | Enables Gerrit mode in head_info via repo setting + cache DB handle. |
| Cargo.lock | Records dependency graph changes for but-db and chrono usage. |
| 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 | ||
| ) | ||
| })?; |
| 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; | ||
| }; |
| #[test] | ||
| fn gerrit_mode_uses_metadata_for_commit_review_urls_and_push_status() -> anyhow::Result<()> { | ||
| let (repo, mut meta) = read_only_in_memory_scenario("remote-advanced-ff")?; | ||
| add_stack(&mut meta, 1, "A", StackState::InWorkspace); | ||
| let base_info = head_info(&repo, &meta, standard_options())?; | ||
| let local_commit = base_info.stacks[0].segments[0].commits[0].clone(); | ||
| let change_id = local_commit.change_id().to_string(); |
| pub fn standard_options() -> but_workspace::ref_info::Options<'static> { | ||
| ref_info::Options { | ||
| expensive_commit_info: true, | ||
| traversal: Default::default(), | ||
| ..Default::default() | ||
| } |
This brings the old gerrit-mode logic that previously was only present on the super legacy
stacks_detailsfunction, and brings in into theRefInfoconstruction.I realise that this is the first time bringing the database into the
but-workspacecrate, but in this instance, it felt reasonable to provide a specificGerritMetadataHandledirectly rather than adding more abstraction layers over it.