Merge pull request #12367 from gitbutlerapp/stack-entry-reviews

Stack Entry: Carry associated review information

Author: estib-vega

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

@@ -59,11 +59,16 @@ pub fn create_virtual_branch(ctx: &mut Context, branch: BranchCreateRequest) ->
             .context("BUG: didn't find a stack that was just created")?;
         let stack = &new_ws.stacks[stack_idx];
         let tip = stack.segments[segment_idx].tip().unwrap_or(repo.object_hash().null());
+        let review_id = stack.segments[segment_idx]
+            .metadata
+            .as_ref()
+            .and_then(|meta| meta.review.pull_request);
 
         let out = StackEntryNoOpt {
             id: stack.id.context("BUG: all new stacks are created with an ID")?,
             heads: vec![StackHeadInfo {
                 name: new_ref.shorten().into(),
+                review_id,
                 tip,
                 is_checked_out: false,
             }],

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

@@ -52,6 +52,15 @@ fn id_from_name_v2_to_v3_opt(
     }))
 }
 
+/// Get the associated forge review information out of the metadata, if any.
+fn review_id_from_meta(
+    name: &gix::refs::FullNameRef,
+    meta: &VirtualBranchesTomlMetadata,
+) -> anyhow::Result<Option<usize>> {
+    let pull_request = meta.branch_opt(name)?.and_then(|ref_meta| ref_meta.review.pull_request);
+    Ok(pull_request)
+}
+
 /// Returns the list of branch information for the branches in a stack.
 pub fn stack_heads_info(stack: &Stack, repo: &gix::Repository) -> anyhow::Result<Vec<StackHeadInfo>> {
     let branches = stack
@@ -62,6 +71,7 @@ pub fn stack_heads_info(stack: &Stack, repo: &gix::Repository) -> anyhow::Result
             let tip = branch.head_oid(repo).ok()?;
             Some(StackHeadInfo {
                 name: branch.name().to_owned().into(),
+                review_id: branch.pr_number,
                 tip,
                 is_checked_out: false,
             })
@@ -84,6 +94,7 @@ fn try_from_stack_v3(
         .segments
         .into_iter()
         .map(|segment| -> anyhow::Result<_> {
+            let review_id = segment.metadata.and_then(|meta| meta.review.pull_request);
             let ref_name = segment
                 .ref_info
                 .context("This type can't represent this state and it shouldn't have to")?
@@ -95,6 +106,7 @@ fn try_from_stack_v3(
                     .and_then(|r| r.try_id())
                     .map(|id| id.detach())
                     .unwrap_or(repo.object_hash().null()),
+                review_id,
                 name: ref_name.shorten().into(),
                 is_checked_out: segment.is_entrypoint,
             })
@@ -160,6 +172,7 @@ pub fn stacks_v3(
                 heads: vec![StackHeadInfo {
                     name: ref_name.shorten().into(),
                     tip,
+                    review_id: review_id_from_meta(ref_name.as_ref(), meta)?,
                     is_checked_out: false,
                 }],
                 tip,

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

@@ -18,6 +18,8 @@ pub struct StackHeadInfo {
     #[serde(with = "but_serde::object_id")]
     #[cfg_attr(feature = "export-ts", ts(type = "string"))]
     pub tip: gix::ObjectId,
+    /// The associated forge review with this branch, e.g. GitHub PRs or GitLab MRs
+    pub review_id: Option<usize>,
     /// If `true`, then this head is checked directly so `HEAD` points to it, and this is only ever `true` for a single head.
     /// This is `false` if the worktree is checked out.
     pub is_checked_out: bool,

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

@@ -69,6 +69,7 @@ fn unborn_untracked() -> anyhow::Result<()> {
                 StackHeadInfo {
                     name: "main",
                     tip: Sha1(0000000000000000000000000000000000000000),
+                    review_id: None,
                     is_checked_out: false,
                 },
             ],
@@ -228,6 +229,7 @@ fn conflicted_in_local_branch() -> anyhow::Result<()> {
                 StackHeadInfo {
                     name: "main",
                     tip: Sha1(84503317a1e1464381fcff65ece14bc1f4315b7c),
+                    review_id: None,
                     is_checked_out: false,
                 },
             ],
@@ -347,6 +349,7 @@ fn single_branch() -> anyhow::Result<()> {
                 StackHeadInfo {
                     name: "main",
                     tip: Sha1(b5743a3aa79957bcb7f654d7d4ad11d995ad5303),
+                    review_id: None,
                     is_checked_out: false,
                 },
             ],
@@ -521,26 +524,31 @@ fn single_branch_multiple_segments() -> anyhow::Result<()> {
                 StackHeadInfo {
                     name: "main",
                     tip: Sha1(b5743a3aa79957bcb7f654d7d4ad11d995ad5303),
+                    review_id: None,
                     is_checked_out: false,
                 },
                 StackHeadInfo {
                     name: "nine",
                     tip: Sha1(344e3209e344c1eb90bedb4b00b4d4999a84406c),
+                    review_id: None,
                     is_checked_out: false,
                 },
                 StackHeadInfo {
                     name: "six",
                     tip: Sha1(c4f2a356d6ed7250bab3dd7c58e1922b95f288c5),
+                    review_id: None,
                     is_checked_out: false,
                 },
                 StackHeadInfo {
                     name: "three",
                     tip: Sha1(281da9454d5b41844d28e453e80b24925a7c8c7a),
+                    review_id: None,
                     is_checked_out: false,
                 },
                 StackHeadInfo {
                     name: "one",
                     tip: Sha1(3d57fc18d679a1ba45bc7f79e394a5e2606719ee),
+                    review_id: None,
                     is_checked_out: false,
                 },
             ],

crates/but-workspace/tests/workspace/ref_info/with_workspace_commit/legacy.rs

@@ -38,11 +38,13 @@ mod stacks {
                     StackHeadInfo {
                         name: "C-on-A",
                         tip: Sha1(5f37dbfd4b1c3d2ee75f216665ab4edf44c843cb),
+                        review_id: None,
                         is_checked_out: false,
                     },
                     StackHeadInfo {
                         name: "A",
                         tip: Sha1(d79bba960b112dbd25d45921c47eeda22288022b),
+                        review_id: None,
                         is_checked_out: false,
                     },
                 ],
@@ -58,11 +60,13 @@ mod stacks {
                     StackHeadInfo {
                         name: "B-on-A",
                         tip: Sha1(4e5484ac0f1da1909414b1e16bd740c1a3599509),
+                        review_id: None,
                         is_checked_out: false,
                     },
                     StackHeadInfo {
                         name: "A",
                         tip: Sha1(d79bba960b112dbd25d45921c47eeda22288022b),
+                        review_id: None,
                         is_checked_out: false,
                     },
                 ],
@@ -85,11 +89,13 @@ mod stacks {
                     StackHeadInfo {
                         name: "C-on-A",
                         tip: Sha1(5f37dbfd4b1c3d2ee75f216665ab4edf44c843cb),
+                        review_id: None,
                         is_checked_out: false,
                     },
                     StackHeadInfo {
                         name: "A",
                         tip: Sha1(d79bba960b112dbd25d45921c47eeda22288022b),
+                        review_id: None,
                         is_checked_out: false,
                     },
                 ],
@@ -105,11 +111,13 @@ mod stacks {
                     StackHeadInfo {
                         name: "B-on-A",
                         tip: Sha1(4e5484ac0f1da1909414b1e16bd740c1a3599509),
+                        review_id: None,
                         is_checked_out: false,
                     },
                     StackHeadInfo {
                         name: "A",
                         tip: Sha1(d79bba960b112dbd25d45921c47eeda22288022b),
+                        review_id: None,
                         is_checked_out: false,
                     },
                 ],
@@ -137,6 +145,7 @@ mod stacks {
                     StackHeadInfo {
                         name: "A",
                         tip: Sha1(d79bba960b112dbd25d45921c47eeda22288022b),
+                        review_id: None,
                         is_checked_out: true,
                     },
                 ],
@@ -229,6 +238,7 @@ mod stacks {
                     StackHeadInfo {
                         name: "main",
                         tip: Sha1(c166d42d4ef2e5e742d33554d03805cfb0b24d11),
+                        review_id: None,
                         is_checked_out: false,
                     },
                 ],

crates/but/src/command/legacy/forge/review.rs

@@ -86,7 +86,7 @@ pub async fn create_review(
         Some(get_branch_names(&ctx.legacy_project, &branch_id)?)
     } else {
         // Find branches without PRs
-        let branches_without_prs = get_branches_without_prs(&review_map, &applied_stacks);
+        let branches_without_prs = get_branches_without_prs(&review_map, &applied_stacks)?;
 
         if branches_without_prs.is_empty() {
             if let Some(out) = out.for_human() {
@@ -182,19 +182,25 @@ async fn ensure_forge_authentication(ctx: &mut Context) -> Result<(), anyhow::Er
 fn get_branches_without_prs(
     review_map: &std::collections::HashMap<String, Vec<but_forge::ForgeReview>>,
     applied_stacks: &[but_workspace::legacy::ui::StackEntry],
-) -> Vec<String> {
+) -> anyhow::Result<Vec<String>> {
     let mut branches_without_prs = Vec::new();
     for stack_entry in applied_stacks {
         for head in &stack_entry.heads {
-            let branch_name = head.name.to_string();
-            if !review_map.contains_key(&branch_name)
-                || review_map.get(&branch_name).map(|v| v.is_empty()).unwrap_or(true)
+            let branch_name = &head.name.to_string();
+            if !review_map.contains_key(branch_name)
+                || review_map.get(branch_name).map(|v| v.is_empty()).unwrap_or(true)
             {
-                branches_without_prs.push(branch_name);
+                // This means that there are no associated reviews that are open, but that doesn't mean that there are
+                // no associated reviews.
+                // Check whether there's an associated forge review.
+                if head.review_id.is_none() {
+                    // If there's no associated review, the append the branch
+                    branches_without_prs.push(branch_name.to_owned());
+                }
             }
         }
     }
-    branches_without_prs
+    Ok(branches_without_prs)
 }
 
 fn get_branch_names(project: &Project, branch_id: &str) -> anyhow::Result<Vec<String>> {
@@ -789,7 +795,6 @@ pub fn get_review_map(
     cache_config: Option<but_forge::CacheConfig>,
 ) -> anyhow::Result<std::collections::HashMap<String, Vec<but_forge::ForgeReview>>> {
     let reviews = but_api::legacy::forge::list_reviews(ctx, cache_config).unwrap_or_default();
-
     let branch_review_map = reviews
         .into_iter()
         .fold(std::collections::HashMap::new(), |mut acc, r| {

packages/core/src/generated/workspace/legacy/index.ts

@@ -41,6 +41,10 @@ export type StackHeadInfo = {
 	 * The tip of the branch.
 	 */
 	tip: string;
+	/**
+	 * The associated forge review with this branch, e.g. GitHub PRs or GitLab MRs
+	 */
+	reviewId: number | null;
 	/**
 	 * If `true`, then this head is checked directly so `HEAD` points to it, and this is only ever `true` for a single head.
 	 * This is `false` if the worktree is checked out.