From 7dbadb13a9d36e31bf272a7964fb4573f958c9e6 Mon Sep 17 00:00:00 2001 From: estib Date: Mon, 16 Sep 2024 18:15:06 +0200 Subject: [PATCH] Get the upstream name from the pushed virtual branch Once the push of a virtual branch succeeded, return the ref name and the remote name in order to correctly determine which branch name to create a PR from. --- .../src/lib/branch/BranchHeader.svelte | 7 +++-- apps/desktop/src/lib/utils/branch.test.ts | 26 +++++++++++++++---- apps/desktop/src/lib/utils/branch.ts | 20 +++++++++++--- .../src/lib/vbranches/branchController.ts | 8 +++--- apps/desktop/src/lib/vbranches/types.ts | 5 ++++ .../gitbutler-branch-actions/src/actions.rs | 2 +- .../gitbutler-branch-actions/src/virtual.rs | 26 +++++++++++++------ .../gitbutler-tauri/src/virtual_branches.rs | 3 ++- 8 files changed, 72 insertions(+), 25 deletions(-) diff --git a/apps/desktop/src/lib/branch/BranchHeader.svelte b/apps/desktop/src/lib/branch/BranchHeader.svelte index fe2b21ad0..69e4dde07 100644 --- a/apps/desktop/src/lib/branch/BranchHeader.svelte +++ b/apps/desktop/src/lib/branch/BranchHeader.svelte @@ -121,8 +121,11 @@ if (branch.commits.some((c) => !c.isRemote)) { const firstPush = !branch.upstream; - const remoteBranchRef = await branchController.pushBranch(branch.id, branch.requiresForce); - upstreamBranchName = getBranchNameFromRef(remoteBranchRef); + const { refname, remote } = await branchController.pushBranch( + branch.id, + branch.requiresForce + ); + upstreamBranchName = getBranchNameFromRef(refname, remote); if (firstPush) { // TODO: fix this hack for reactively available prService. diff --git a/apps/desktop/src/lib/utils/branch.test.ts b/apps/desktop/src/lib/utils/branch.test.ts index 797ec9a98..07da39545 100644 --- a/apps/desktop/src/lib/utils/branch.test.ts +++ b/apps/desktop/src/lib/utils/branch.test.ts @@ -4,20 +4,36 @@ import { expect, test, describe } from 'vitest'; describe.concurrent('getBranchNameFromRef', () => { test('When provided a ref with a remote prefix, it returns the branch name', () => { const ref = 'refs/remotes/origin/main'; + const remote = 'origin'; - expect(BranchUtils.getBranchNameFromRef(ref)).toBe('main'); + expect(BranchUtils.getBranchNameFromRef(ref, remote)).toBe('main'); }); - test('When provided a ref without a remote prefix, it returns the branch name', () => { + test("When provided a ref with a remote prefix that can't be found, it throws an error", () => { const ref = 'main'; + const remote = 'origin'; - expect(BranchUtils.getBranchNameFromRef(ref)).toBe('main'); + expect(() => BranchUtils.getBranchNameFromRef(ref, remote)).toThrowError(); }); - test('When provided a ref with a remote prefix and multiple separators, it returns the branch name', () => { + test('When provided a ref with a remote prefix and multiple separators, it returns the correct branch name', () => { const ref = 'refs/remotes/origin/feature/cool-thing'; + const remote = 'origin'; - expect(BranchUtils.getBranchNameFromRef(ref)).toBe('feature/cool-thing'); + expect(BranchUtils.getBranchNameFromRef(ref, remote)).toBe('feature/cool-thing'); + }); + + test('When provided a ref with a remote that has multiple separators in it, it returns the correct branch name', () => { + const ref = 'refs/remotes/origin/feature/cool-thing'; + const remote = 'origin/feature'; + + expect(BranchUtils.getBranchNameFromRef(ref, remote)).toBe('cool-thing'); + }); + + test('When provided a ref but no explicit remote, it returns the branch name with the remote prefix', () => { + const ref = 'refs/remotes/origin/main'; + + expect(BranchUtils.getBranchNameFromRef(ref)).toBe('origin/main'); }); }); diff --git a/apps/desktop/src/lib/utils/branch.ts b/apps/desktop/src/lib/utils/branch.ts index 8a1f62383..0d6fdb7f5 100644 --- a/apps/desktop/src/lib/utils/branch.ts +++ b/apps/desktop/src/lib/utils/branch.ts @@ -2,13 +2,25 @@ const PREFERRED_REMOTE = 'origin'; const BRANCH_SEPARATOR = '/'; const REF_REMOTES_PREFIX = 'refs/remotes/'; -export function getBranchNameFromRef(ref: string): string | undefined { +/** + * Get the branch name from a refname. + * + * If a remote is provided, the remote prefix will be removed. + */ +export function getBranchNameFromRef(ref: string, remote?: string): string | undefined { if (ref.startsWith(REF_REMOTES_PREFIX)) { - ref = ref.slice(REF_REMOTES_PREFIX.length); + ref = ref.replace(REF_REMOTES_PREFIX, ''); } - const parts = ref.split(BRANCH_SEPARATOR); - return parts.length > 1 ? parts.slice(1).join(BRANCH_SEPARATOR) : ref; + if (remote !== undefined) { + const originPrefix = `${remote}${BRANCH_SEPARATOR}`; + if (!ref.startsWith(originPrefix)) { + throw new Error('Failed to parse branch name as reference'); + } + ref = ref.replace(originPrefix, ''); + } + + return ref; } export function getBranchRemoteFromRef(ref: string): string | undefined { diff --git a/apps/desktop/src/lib/vbranches/branchController.ts b/apps/desktop/src/lib/vbranches/branchController.ts index 8109f17b2..a006ed87f 100644 --- a/apps/desktop/src/lib/vbranches/branchController.ts +++ b/apps/desktop/src/lib/vbranches/branchController.ts @@ -4,7 +4,7 @@ import * as toasts from '$lib/utils/toasts'; import posthog from 'posthog-js'; import type { BaseBranchService } from '$lib/baseBranch/baseBranchService'; import type { RemoteBranchService } from '$lib/stores/remoteBranches'; -import type { Hunk, LocalFile } from './types'; +import type { BranchPushResult, Hunk, LocalFile } from './types'; import type { VirtualBranchService } from './virtualBranch'; export class BranchController { @@ -261,16 +261,16 @@ export class BranchController { } } - async pushBranch(branchId: string, withForce: boolean): Promise { + async pushBranch(branchId: string, withForce: boolean): Promise { try { - const upstreamRef = await invoke('push_virtual_branch', { + const pushResult = await invoke('push_virtual_branch', { projectId: this.projectId, branchId, withForce }); posthog.capture('Push Successful'); await this.vbranchService.refresh(); - return upstreamRef; + return pushResult; } catch (err: any) { console.error(err); const { code, message } = err; diff --git a/apps/desktop/src/lib/vbranches/types.ts b/apps/desktop/src/lib/vbranches/types.ts index 82d23c8c6..1018eac6c 100644 --- a/apps/desktop/src/lib/vbranches/types.ts +++ b/apps/desktop/src/lib/vbranches/types.ts @@ -376,3 +376,8 @@ export class BranchData { return this.name.replace('refs/remotes/', '').replace('origin/', '').replace('refs/heads/', ''); } } + +export interface BranchPushResult { + refname: string; + remote: string; +} diff --git a/crates/gitbutler-branch-actions/src/actions.rs b/crates/gitbutler-branch-actions/src/actions.rs index 38c99c709..0aba22503 100644 --- a/crates/gitbutler-branch-actions/src/actions.rs +++ b/crates/gitbutler-branch-actions/src/actions.rs @@ -418,7 +418,7 @@ pub fn push_virtual_branch( branch_id: BranchId, with_force: bool, askpass: Option>, -) -> Result { +) -> Result { let ctx = open_with_verify(project)?; assure_open_workspace_mode(&ctx).context("Pushing a branch requires open workspace mode")?; vbranch::push(&ctx, branch_id, with_force, askpass) diff --git a/crates/gitbutler-branch-actions/src/virtual.rs b/crates/gitbutler-branch-actions/src/virtual.rs index f40ddab48..a693893fd 100644 --- a/crates/gitbutler-branch-actions/src/virtual.rs +++ b/crates/gitbutler-branch-actions/src/virtual.rs @@ -82,6 +82,13 @@ pub struct VirtualBranches { pub skipped_files: Vec, } +#[derive(Debug, PartialEq, Clone, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct PushResult { + pub remote: String, + pub refname: Refname, +} + pub fn unapply_ownership( ctx: &CommandContext, ownership: &BranchOwnershipClaims, @@ -1054,19 +1061,19 @@ pub(crate) fn push( branch_id: BranchId, with_force: bool, askpass: Option>, -) -> Result { +) -> Result { let vb_state = ctx.project().virtual_branches(); + let default_target = vb_state.get_default_target()?; + let upstream_remote = match default_target.push_remote_name { + Some(remote) => remote.clone(), + None => default_target.branch.remote().to_owned(), + }; + let mut vbranch = vb_state.get_branch_in_workspace(branch_id)?; let remote_branch = if let Some(upstream_branch) = &vbranch.upstream { upstream_branch.clone() } else { - let default_target = vb_state.get_default_target()?; - let upstream_remote = match default_target.push_remote_name { - Some(remote) => remote.clone(), - None => default_target.branch.remote().to_owned(), - }; - let remote_branch = format!( "refs/remotes/{}/{}", upstream_remote, @@ -1101,7 +1108,10 @@ pub(crate) fn push( .context("failed to write target branch after push")?; ctx.fetch(remote_branch.remote(), askpass.map(|_| "modal".to_string()))?; - Ok(gitbutler_reference::Refname::Remote(remote_branch)) + Ok(PushResult { + remote: upstream_remote, + refname: gitbutler_reference::Refname::Remote(remote_branch), + }) } struct IsCommitIntegrated<'repo> { diff --git a/crates/gitbutler-tauri/src/virtual_branches.rs b/crates/gitbutler-tauri/src/virtual_branches.rs index d1456fd6f..7b3920302 100644 --- a/crates/gitbutler-tauri/src/virtual_branches.rs +++ b/crates/gitbutler-tauri/src/virtual_branches.rs @@ -3,6 +3,7 @@ pub mod commands { use gitbutler_branch::{ BranchCreateRequest, BranchId, BranchOwnershipClaims, BranchUpdateRequest, }; + use gitbutler_branch_actions::internal::PushResult; use gitbutler_branch_actions::upstream_integration::{BranchStatuses, Resolution}; use gitbutler_branch_actions::{ BaseBranch, BranchListing, BranchListingDetails, BranchListingFilter, RemoteBranch, @@ -267,7 +268,7 @@ pub mod commands { project_id: ProjectId, branch_id: BranchId, with_force: bool, - ) -> Result { + ) -> Result { let project = projects.get(project_id)?; let upstream_refname = gitbutler_branch_actions::push_virtual_branch( &project,