Skip to content

Commit 8242cd1

Browse files
eamodioeamodio
committed
Improves merge target consistency across usages
1 parent d300f2c commit 8242cd1

File tree

5 files changed

+88
-100
lines changed

5 files changed

+88
-100
lines changed

‎src/commands/explainBranch.ts‎

Lines changed: 8 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
1-
import type { CancellationToken, TextEditor, Uri } from 'vscode';
1+
import type { TextEditor, Uri } from 'vscode';
22
import { ProgressLocation } from 'vscode';
33
import type { Source } from '../constants.telemetry';
44
import type { Container } from '../container';
55
import { GitUri } from '../git/gitUri';
6-
import type { GitBranch } from '../git/models/branch';
76
import type { GitBranchReference } from '../git/models/reference';
7+
import { getBranchMergeTargetName } from '../git/utils/-webview/branch.utils';
88
import { showGenericErrorMessage } from '../messages';
99
import { prepareCompareDataForAIRequest } from '../plus/ai/aiProviderService';
1010
import { ReferencesQuickPickIncludes, showReferencePicker } from '../quickpicks/referencePicker';
@@ -85,8 +85,12 @@ export class ExplainBranchCommand extends GlCommandBase {
8585
}
8686

8787
// Clarifying the base branch
88-
const baseBranchName = await getMergeTarget(this.container, branch);
89-
const baseBranch = await repository.git.branches().getBranch(baseBranchName);
88+
const baseBranchNameResult = await getBranchMergeTargetName(this.container, branch);
89+
let baseBranch;
90+
if (!baseBranchNameResult.paused) {
91+
baseBranch = await repository.git.branches().getBranch(baseBranchNameResult.value);
92+
}
93+
9094
if (!baseBranch) {
9195
void showGenericErrorMessage(`Unable to find the base branch for branch ${branch.name}.`);
9296
return;
@@ -142,31 +146,3 @@ export class ExplainBranchCommand extends GlCommandBase {
142146
}
143147
}
144148
}
145-
146-
async function getMergeTarget(
147-
container: Container,
148-
branch: GitBranch,
149-
options?: { cancellation?: CancellationToken },
150-
): Promise<string | undefined> {
151-
const localValue = await container.git
152-
.branches(branch.repoPath)
153-
.getBestMergeTargetBranchName?.(branch.name, branch.getRemoteName());
154-
if (localValue) return localValue;
155-
156-
return getIntegrationDefaultBranchName(container, branch.repoPath, options);
157-
}
158-
159-
// This is similar to what we have in changeBranchMergeTarget.ts
160-
// what is a proper utils files to put it to?
161-
async function getIntegrationDefaultBranchName(
162-
container: Container,
163-
repoPath: string,
164-
options?: { cancellation?: CancellationToken },
165-
): Promise<string | undefined> {
166-
const remote = await container.git.remotes(repoPath).getBestRemoteWithIntegration();
167-
if (remote == null) return undefined;
168-
169-
const integration = await remote.getIntegration();
170-
const defaultBranch = await integration?.getDefaultBranch?.(remote.provider.repoDesc, options);
171-
return defaultBranch && `${remote.name}/${defaultBranch?.name}`;
172-
}

‎src/env/node/git/sub-providers/branches.ts‎

Lines changed: 10 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import type { MergeConflict } from '../../../../git/models/mergeConflict';
1515
import type { GitBranchReference } from '../../../../git/models/reference';
1616
import { parseMergeTreeConflict } from '../../../../git/parsers/mergeTreeParser';
1717
import { getBranchParser } from '../../../../git/parsers/refParser';
18+
import { getBranchMergeTargetName } from '../../../../git/utils/-webview/branch.utils';
1819
import { getReferenceFromBranch } from '../../../../git/utils/-webview/reference.utils';
1920
import type { BranchSortOptions } from '../../../../git/utils/-webview/sorting';
2021
import { sortBranches, sortContributors } from '../../../../git/utils/-webview/sorting';
@@ -255,7 +256,15 @@ export class BranchesGitSubProvider implements GitBranchesSubProvider {
255256
const scope = getLogScope();
256257

257258
try {
258-
const mergeTarget = await this.getBestMergeTargetBranchName(repoPath, ref);
259+
const branch = await this.getBranch(repoPath, ref, cancellation);
260+
if (branch == null) return undefined;
261+
262+
const mergeTargetResult = await getBranchMergeTargetName(this.container, branch, {
263+
cancellation: cancellation,
264+
});
265+
if (mergeTargetResult.paused) return undefined;
266+
267+
const mergeTarget = mergeTargetResult.value;
259268
if (mergeTarget == null) return undefined;
260269

261270
const mergeBase = await this.provider.refs.getMergeBase(
@@ -741,36 +750,6 @@ export class BranchesGitSubProvider implements GitBranchesSubProvider {
741750
return undefined;
742751
}
743752

744-
@log()
745-
async getBestMergeTargetBranchName(
746-
repoPath: string,
747-
ref: string,
748-
remote?: string,
749-
options?: { detectedOnly?: boolean },
750-
cancellation?: CancellationToken,
751-
): Promise<string | undefined> {
752-
const [userTargetResult, targetResult] = await Promise.allSettled([
753-
options?.detectedOnly ? undefined : this.getStoredUserMergeTargetBranchName?.(repoPath, ref),
754-
this.getStoredDetectedMergeTargetBranchName?.(repoPath, ref),
755-
]);
756-
757-
const targetBranchName = getSettledValue(userTargetResult) || getSettledValue(targetResult);
758-
if (targetBranchName) {
759-
const validated = await this.provider.refs.getSymbolicReferenceName(
760-
repoPath,
761-
targetBranchName,
762-
cancellation,
763-
);
764-
return validated || targetBranchName;
765-
}
766-
767-
const [baseResult, defaultResult] = await Promise.allSettled([
768-
this.getBaseBranchName?.(repoPath, ref, cancellation),
769-
this.getDefaultBranchName(repoPath, remote, cancellation),
770-
]);
771-
return getSettledValue(baseResult) || getSettledValue(defaultResult);
772-
}
773-
774753
@log({ exit: true })
775754
async getStoredMergeTargetBranchName(repoPath: string, ref: string): Promise<string | undefined> {
776755
const target =

‎src/git/gitProvider.ts‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -274,17 +274,6 @@ export interface GitBranchesSubProvider {
274274
cancellation?: CancellationToken,
275275
): Promise<MergeConflict | undefined>;
276276
getBaseBranchName?(repoPath: string, ref: string, cancellation?: CancellationToken): Promise<string | undefined>;
277-
/** Gets the best merge target branch name */
278-
getBestMergeTargetBranchName?(
279-
repoPath: string,
280-
ref: string,
281-
remote?: string,
282-
options?: {
283-
/** Excludes the user chosen merge target */
284-
detectedOnly?: boolean;
285-
},
286-
cancellation?: CancellationToken,
287-
): Promise<string | undefined>;
288277
/** Gets the stored merge target branch name, first checking the user target, then the detected target */
289278
getStoredMergeTargetBranchName?(repoPath: string, ref: string): Promise<string | undefined>;
290279
/** Gets the stored detected merge target branch name */

‎src/git/utils/-webview/branch.utils.ts‎

Lines changed: 68 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -17,11 +17,7 @@ export async function getBranchMergeTargetInfo(
1717
},
1818
): Promise<BranchTargetInfo> {
1919
const [targetResult, baseResult, defaultResult] = await Promise.allSettled([
20-
getMergeTargetBranchName(container, branch, {
21-
cancellation: options?.cancellation,
22-
detectedOnly: options?.detectedOnly,
23-
timeout: options?.timeout,
24-
}),
20+
getBranchMergeTargetNameWithoutFallback(container, branch, options),
2521
container.git.branches(branch.repoPath).getBaseBranchName?.(branch.name, options?.cancellation),
2622
getDefaultBranchName(container, branch.repoPath, branch.getRemoteName(), {
2723
cancellation: options?.cancellation,
@@ -37,30 +33,55 @@ export async function getBranchMergeTargetInfo(
3733
};
3834
}
3935

40-
export async function getDefaultBranchName(
36+
export async function getBranchMergeTargetName(
4137
container: Container,
42-
repoPath: string,
43-
remoteName?: string,
44-
options?: { cancellation?: CancellationToken },
45-
): Promise<string | undefined> {
46-
const name = await container.git.branches(repoPath).getDefaultBranchName(remoteName, options?.cancellation);
47-
return name ?? getDefaultBranchNameFromIntegration(container, repoPath, options);
48-
}
38+
branch: GitBranch,
39+
options?: {
40+
associatedPullRequest?: Promise<PullRequest | undefined>;
41+
cancellation?: CancellationToken;
42+
detectedOnly?: boolean;
43+
timeout?: number;
44+
},
45+
): Promise<MaybePausedResult<string | undefined>> {
46+
async function getMergeTargetFallback() {
47+
const [baseResult, defaultResult] = await Promise.allSettled([
48+
container.git.branches(branch.repoPath).getBaseBranchName?.(branch.name, options?.cancellation),
49+
getDefaultBranchName(container, branch.repoPath, branch.getRemoteName(), {
50+
cancellation: options?.cancellation,
51+
}),
52+
]);
53+
return getSettledValue(baseResult) ?? getSettledValue(defaultResult);
54+
}
4955

50-
export async function getDefaultBranchNameFromIntegration(
51-
container: Container,
52-
repoPath: string,
53-
options?: { cancellation?: CancellationToken },
54-
): Promise<string | undefined> {
55-
const remote = await container.git.remotes(repoPath).getBestRemoteWithIntegration(undefined, options?.cancellation);
56-
if (remote == null) return undefined;
56+
const result = await getBranchMergeTargetNameWithoutFallback(container, branch, options);
57+
if (!result.paused) {
58+
if (result.value) return { value: result.value, paused: false };
5759

58-
const integration = await remote.getIntegration();
59-
const defaultBranch = await integration?.getDefaultBranch?.(remote.provider.repoDesc, options);
60-
return defaultBranch && `${remote.name}/${defaultBranch?.name}`;
60+
if (options?.cancellation?.isCancellationRequested) {
61+
return { value: Promise.resolve(undefined), paused: true, reason: 'cancelled' };
62+
}
63+
64+
const fallback = await getMergeTargetFallback();
65+
if (options?.cancellation?.isCancellationRequested) {
66+
return { value: Promise.resolve(undefined), paused: true, reason: 'cancelled' };
67+
}
68+
69+
return { value: fallback, paused: false };
70+
}
71+
72+
if (options?.cancellation?.isCancellationRequested || result.reason === 'cancelled') {
73+
return { value: Promise.resolve(undefined), paused: true, reason: 'cancelled' };
74+
}
75+
76+
return {
77+
value: result.value.then(r => r ?? getMergeTargetFallback()),
78+
paused: true,
79+
reason: 'timedout',
80+
};
6181
}
6282

63-
export async function getMergeTargetBranchName(
83+
/** This is an internal helper function for getting only the merge target from stored data or a PR, not falling back to base/default */
84+
async function getBranchMergeTargetNameWithoutFallback(
6485
container: Container,
6586
branch: GitBranch,
6687
options?: {
@@ -95,3 +116,26 @@ export async function getMergeTargetBranchName(
95116
options?.timeout,
96117
);
97118
}
119+
120+
export async function getDefaultBranchName(
121+
container: Container,
122+
repoPath: string,
123+
remoteName?: string,
124+
options?: { cancellation?: CancellationToken },
125+
): Promise<string | undefined> {
126+
const name = await container.git.branches(repoPath).getDefaultBranchName(remoteName, options?.cancellation);
127+
return name ?? getDefaultBranchNameFromIntegration(container, repoPath, options);
128+
}
129+
130+
export async function getDefaultBranchNameFromIntegration(
131+
container: Container,
132+
repoPath: string,
133+
options?: { cancellation?: CancellationToken },
134+
): Promise<string | undefined> {
135+
const remote = await container.git.remotes(repoPath).getBestRemoteWithIntegration(undefined, options?.cancellation);
136+
if (remote == null) return undefined;
137+
138+
const integration = await remote.getIntegration();
139+
const defaultBranch = await integration?.getDefaultBranch?.(remote.provider.repoDesc, options);
140+
return defaultBranch && `${remote.name}/${defaultBranch?.name}`;
141+
}

‎src/views/nodes/branchNode.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ import type { GitBranchReference } from '../../git/models/reference';
1515
import type { Repository } from '../../git/models/repository';
1616
import type { GitUser } from '../../git/models/user';
1717
import type { GitWorktree } from '../../git/models/worktree';
18-
import { getMergeTargetBranchName } from '../../git/utils/-webview/branch.utils';
18+
import { getBranchMergeTargetName } from '../../git/utils/-webview/branch.utils';
1919
import { getBranchIconPath, getRemoteIconPath, getWorktreeBranchIconPath } from '../../git/utils/-webview/icons';
2020
import { getLastFetchedUpdateInterval } from '../../git/utils/fetch.utils';
2121
import { getHighlanderProviders } from '../../git/utils/remote.utils';
@@ -252,7 +252,7 @@ export class BranchNode
252252
? this.view.container.git.branches(this.branch.repoPath).getBaseBranchName?.(this.branch.name)
253253
: undefined,
254254
loadComparisonDefaultCompareWith
255-
? getMergeTargetBranchName(this.view.container, this.branch, {
255+
? getBranchMergeTargetName(this.view.container, this.branch, {
256256
associatedPullRequest: prPromise,
257257
timeout: 100,
258258
})

0 commit comments

Comments
 (0)