Skip to content

Commit 20c41f3

Browse files
eamodioeamodio
committed
Improves branch merge target
 - Imporves the UX for editing -- adds inline edit option - Improves merge target status UI to better deal with truncation - Consolidates logic to find the best merge target - Renames the Git config methods dealing with merge target for clarity - Renames Git config keys for user-set (`gk-merge-target-user`) and auto-detected (`gk-merge-target`) merge targets - Deprecates the `gk-target-base` config key
1 parent 35a496c commit 20c41f3

File tree

15 files changed

+272
-289
lines changed

15 files changed

+272
-289
lines changed

‎src/commands/changeBranchMergeTarget.ts‎

Lines changed: 16 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
1-
import type { CancellationToken } from 'vscode';
21
import type { Container } from '../container';
32
import type { GitBranch } from '../git/models/branch';
43
import type { Repository } from '../git/models/repository';
5-
import { getSettledValue } from '../system/promise';
64
import type { ViewsWithRepositoryFolders } from '../views/viewBase';
75
import type { PartialStepState, StepGenerator, StepState } from './quickCommand';
86
import { endSteps, QuickCommand, StepResultBreak } from './quickCommand';
@@ -64,9 +62,7 @@ export class ChangeBranchMergeTargetCommand extends QuickCommand {
6462
while (this.canStepsContinue(state)) {
6563
if (state.counter < 1 || !state.repo || typeof state.repo === 'string') {
6664
const result = yield* pickRepositoryStep(state, context);
67-
if (result === StepResultBreak) {
68-
break;
69-
}
65+
if (result === StepResultBreak) break;
7066

7167
state.repo = result;
7268
}
@@ -79,9 +75,7 @@ export class ChangeBranchMergeTargetCommand extends QuickCommand {
7975
placeholder: 'Pick a branch to edit',
8076
filter: (branch: GitBranch) => !branch.remote,
8177
});
82-
if (branches === StepResultBreak) {
83-
continue;
84-
}
78+
if (branches === StepResultBreak) continue;
8579

8680
state.branch = branches.name;
8781
}
@@ -92,62 +86,32 @@ export class ChangeBranchMergeTargetCommand extends QuickCommand {
9286
.getBaseBranchName?.(state.branch);
9387
}
9488

95-
const gitBranch = await state.repo.git.branches().getBranch(state.branch);
96-
const detectedMergeTarget = gitBranch && (await getDetectedMergeTarget(this.container, gitBranch));
89+
const detectedMergeTarget = await this.container.git
90+
.branches(state.repo.path)
91+
.getStoredDetectedMergeTargetBranchName?.(state.branch);
92+
const userMergeTarget = await this.container.git
93+
.branches(state.repo.path)
94+
.getStoredUserMergeTargetBranchName?.(state.branch);
9795

9896
const result = yield* pickOrResetBranchStep(state, context, {
9997
picked: state.mergeBranch,
10098
placeholder: 'Pick a merge target branch',
10199
filter: (branch: GitBranch) => branch.remote && branch.name !== state.branch,
102-
resetTitle: 'Reset Merge Target',
103-
resetDescription: detectedMergeTarget ? `Reset to "${detectedMergeTarget}"` : '',
100+
reset: {
101+
allowed: userMergeTarget != null && detectedMergeTarget !== userMergeTarget,
102+
label: 'Reset Merge Target',
103+
detail: 'Reset the merge target branch to be automatically detected',
104+
},
104105
});
105-
if (result === StepResultBreak) {
106-
continue;
107-
}
106+
if (result === StepResultBreak) continue;
107+
108108
if (state.branch) {
109109
await this.container.git
110110
.branches(state.repo.path)
111-
.setUserMergeTargetBranchName?.(state.branch, result?.name);
111+
.storeUserMergeTargetBranchName?.(state.branch, result?.name);
112112
}
113113

114114
endSteps(state);
115115
}
116116
}
117117
}
118-
119-
async function getDetectedMergeTarget(
120-
container: Container,
121-
branch: GitBranch,
122-
options?: { cancellation?: CancellationToken },
123-
): Promise<string | undefined> {
124-
const [baseResult, defaultResult, targetResult] = await Promise.allSettled([
125-
container.git.branches(branch.repoPath).getBaseBranchName?.(branch.name),
126-
container.git.branches(branch.repoPath).getDefaultBranchName(branch.getRemoteName()),
127-
container.git.branches(branch.repoPath).getTargetBranchName?.(branch.name),
128-
]);
129-
130-
const baseBranchName = getSettledValue(baseResult);
131-
const defaultBranchName = getSettledValue(defaultResult);
132-
const targetMaybeResult = getSettledValue(targetResult);
133-
const localValue = targetMaybeResult || baseBranchName || defaultBranchName;
134-
if (localValue) {
135-
return localValue;
136-
}
137-
138-
// only if nothing found locally, try value from integration
139-
return getIntegrationDefaultBranchName(container, branch.repoPath, options);
140-
}
141-
142-
async function getIntegrationDefaultBranchName(
143-
container: Container,
144-
repoPath: string,
145-
options?: { cancellation?: CancellationToken },
146-
): Promise<string | undefined> {
147-
const remote = await container.git.remotes(repoPath).getBestRemoteWithIntegration();
148-
if (remote == null) return undefined;
149-
150-
const integration = await remote.getIntegration();
151-
const defaultBranch = await integration?.getDefaultBranch?.(remote.provider.repoDesc, options);
152-
return defaultBranch && `${remote.name}/${defaultBranch?.name}`;
153-
}

‎src/commands/explainBranch.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -147,10 +147,11 @@ async function getMergeTarget(
147147
branch: GitBranch,
148148
options?: { cancellation?: CancellationToken },
149149
): Promise<string | undefined> {
150-
const localValue = await container.git.branches(branch.repoPath).getMergeTargetBranchName?.(branch);
151-
if (localValue) {
152-
return localValue;
153-
}
150+
const localValue = await container.git
151+
.branches(branch.repoPath)
152+
.getBestMergeTargetBranchName?.(branch.name, branch.getRemoteName());
153+
if (localValue) return localValue;
154+
154155
return getIntegrationDefaultBranchName(container, branch.repoPath, options);
155156
}
156157

‎src/commands/quickCommand.steps.ts‎

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -795,15 +795,18 @@ export function* pickOrResetBranchStep<
795795
picked,
796796
placeholder,
797797
title,
798-
resetTitle,
799-
resetDescription,
798+
reset,
800799
}: {
801800
filter?: (b: GitBranch) => boolean;
802801
picked?: string | string[];
803802
placeholder: string;
804803
title?: string;
805-
resetTitle: string;
806-
resetDescription: string;
804+
reset: {
805+
allowed: boolean;
806+
label?: string;
807+
description?: string;
808+
detail?: string;
809+
};
807810
},
808811
): StepResultGenerator<GitBranchReference | undefined> {
809812
const items = getBranches(state.repo, {
@@ -814,25 +817,31 @@ export function* pickOrResetBranchStep<
814817
branches.length === 0
815818
? [createDirectiveQuickPickItem(Directive.Back, true), createDirectiveQuickPickItem(Directive.Cancel)]
816819
: [
817-
createDirectiveQuickPickItem(Directive.Reset, false, {
818-
label: resetTitle,
819-
description: resetDescription,
820-
}),
820+
...(reset.allowed
821+
? [
822+
createDirectiveQuickPickItem(Directive.Reset, false, {
823+
label: reset.label,
824+
description: reset.description,
825+
detail: reset.detail,
826+
}),
827+
]
828+
: []),
821829
...branches,
822830
],
823831
);
824832

825833
const resetButton: QuickInputButton = {
826-
iconPath: new ThemeIcon('notebook-revert'),
827-
tooltip: resetDescription || resetTitle,
834+
iconPath: new ThemeIcon('discard'),
835+
tooltip: reset.label,
828836
};
837+
829838
let resetButtonClicked = false;
830839
const step = createPickStep<BranchQuickPickItem>({
831840
title: appendReposToTitle(title ?? context.title, state, context),
832841
placeholder: count => (!count ? `No branches found in ${state.repo.formattedName}` : placeholder),
833842
matchOnDetail: true,
834843
items: items,
835-
additionalButtons: [resetButton],
844+
additionalButtons: reset.allowed ? [resetButton] : [],
836845
onDidClickButton: (_quickpick, button) => {
837846
if (button === resetButton) {
838847
resetButtonClicked = true;

‎src/constants.ts‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,12 +48,15 @@ export const enum CharCode {
4848
}
4949

5050
export type GitConfigKeys =
51-
| `branch.${string}.${'gk' | 'vscode'}-merge-base`
52-
| `branch.${string}.gk-user-merge-target`
53-
| `branch.${string}.gk-target-base`
51+
| `branch.${string}.vscode-merge-base`
52+
| `branch.${string}.gk-merge-base`
53+
| `branch.${string}.gk-merge-target`
54+
| `branch.${string}.gk-merge-target-user`
5455
| `branch.${string}.gk-associated-issues`
5556
| `branch.${string}.github-pr-owner-number`;
5657

58+
export type DeprecatedGitConfigKeys = `branch.${string}.gk-target-base`;
59+
5760
export const enum GlyphChars {
5861
AngleBracketLeftHeavy = '\u2770',
5962
AngleBracketRightHeavy = '\u2771',

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

Lines changed: 65 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
import type { CancellationToken } from 'vscode';
2-
import type { GitConfigKeys } from '../../../../constants';
32
import type { Container } from '../../../../container';
43
import { CancellationError, isCancellationError } from '../../../../errors';
54
import type { GitCache } from '../../../../git/cache';
@@ -256,18 +255,13 @@ export class BranchesGitSubProvider implements GitBranchesSubProvider {
256255
const scope = getLogScope();
257256

258257
try {
259-
let baseOrTargetBranch = await this.getBaseBranchName(repoPath, ref, cancellation);
260-
// If the base looks like its remote branch, look for the target or default
261-
if (baseOrTargetBranch == null || baseOrTargetBranch.endsWith(`/${ref}`)) {
262-
baseOrTargetBranch = await this.getTargetBranchName(repoPath, ref);
263-
baseOrTargetBranch ??= await this.getDefaultBranchName(repoPath, undefined, cancellation);
264-
if (baseOrTargetBranch == null) return undefined;
265-
}
258+
const mergeTarget = await this.getBestMergeTargetBranchName(repoPath, ref);
259+
if (mergeTarget == null) return undefined;
266260

267261
const mergeBase = await this.provider.refs.getMergeBase(
268262
repoPath,
269263
ref,
270-
baseOrTargetBranch,
264+
mergeTarget,
271265
undefined,
272266
cancellation,
273267
);
@@ -314,7 +308,7 @@ export class BranchesGitSubProvider implements GitBranchesSubProvider {
314308
return {
315309
repoPath: repoPath,
316310
branch: ref,
317-
baseOrTargetBranch: baseOrTargetBranch,
311+
mergeTarget: mergeTarget,
318312
mergeBase: mergeBase,
319313

320314
commits: totalCommits,
@@ -669,7 +663,7 @@ export class BranchesGitSubProvider implements GitBranchesSubProvider {
669663
const branch = await this.provider.refs.getSymbolicReferenceName(repoPath, mergeBase);
670664
if (branch != null) {
671665
if (update) {
672-
void this.setBaseBranchName(repoPath, ref, branch);
666+
void this.storeBaseBranchName(repoPath, ref, branch);
673667
}
674668
return branch;
675669
}
@@ -679,48 +673,13 @@ export class BranchesGitSubProvider implements GitBranchesSubProvider {
679673

680674
const branch = await this.getBaseBranchFromReflog(repoPath, ref, { upstream: true }, cancellation);
681675
if (branch != null) {
682-
void this.setBaseBranchName(repoPath, ref, branch);
676+
void this.storeBaseBranchName(repoPath, ref, branch);
683677
return branch;
684678
}
685679

686680
return undefined;
687681
}
688682

689-
@log()
690-
async setBaseBranchName(repoPath: string, ref: string, base: string): Promise<void> {
691-
const mergeBaseConfigKey: GitConfigKeys = `branch.${ref}.gk-merge-base`;
692-
693-
await this.provider.config.setConfig(repoPath, mergeBaseConfigKey, base);
694-
}
695-
696-
@log()
697-
async getUserMergeTargetBranchName(repoPath: string, ref: string): Promise<string | undefined> {
698-
const mergeTargetConfigKey: GitConfigKeys = `branch.${ref}.gk-user-merge-target`;
699-
const target = await this.provider.config.getConfig(repoPath, mergeTargetConfigKey);
700-
return target?.trim() || undefined;
701-
}
702-
703-
@log()
704-
async setUserMergeTargetBranchName(repoPath: string, ref: string, target: string | undefined): Promise<void> {
705-
const mergeTargetConfigKey: GitConfigKeys = `branch.${ref}.gk-user-merge-target`;
706-
await this.provider.config.setConfig(repoPath, mergeTargetConfigKey, target);
707-
}
708-
709-
async getMergeTargetBranchName(repoPath: string, branch: GitBranch): Promise<string | undefined> {
710-
const [baseResult, defaultResult, targetResult, userTargetResult] = await Promise.allSettled([
711-
this.getBaseBranchName?.(repoPath, branch.name),
712-
this.getDefaultBranchName(repoPath, branch.getRemoteName()),
713-
this.getTargetBranchName?.(repoPath, branch.name),
714-
this.getUserMergeTargetBranchName?.(repoPath, branch.name),
715-
]);
716-
717-
const baseBranchName = getSettledValue(baseResult);
718-
const defaultBranchName = getSettledValue(defaultResult);
719-
const targetMaybeResult = getSettledValue(targetResult);
720-
const userTargetBranchName = getSettledValue(userTargetResult);
721-
return userTargetBranchName || targetMaybeResult || baseBranchName || defaultBranchName;
722-
}
723-
724683
private async getBaseBranchFromReflog(
725684
repoPath: string,
726685
ref: string,
@@ -782,26 +741,75 @@ export class BranchesGitSubProvider implements GitBranchesSubProvider {
782741
return undefined;
783742
}
784743

785-
@log({ exit: true })
786-
async getTargetBranchName(repoPath: string, ref: string): Promise<string | undefined> {
787-
const targetBaseConfigKey: GitConfigKeys = `branch.${ref}.gk-target-base`;
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+
]);
788756

789-
let target = await this.provider.config.getConfig(repoPath, targetBaseConfigKey);
790-
if (target) {
791-
target = await this.provider.refs.getSymbolicReferenceName(repoPath, target);
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;
792765
}
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+
774+
@log({ exit: true })
775+
async getStoredMergeTargetBranchName(repoPath: string, ref: string): Promise<string | undefined> {
776+
const target =
777+
(await this.getStoredUserMergeTargetBranchName?.(repoPath, ref)) ??
778+
(await this.getStoredDetectedMergeTargetBranchName?.(repoPath, ref));
793779
return target?.trim() || undefined;
794780
}
795781

796-
@log()
797-
async setTargetBranchName(repoPath: string, ref: string, target: string): Promise<void> {
798-
const targetBaseConfigKey: GitConfigKeys = `branch.${ref}.gk-target-base`;
782+
@log({ exit: true })
783+
async getStoredDetectedMergeTargetBranchName(repoPath: string, ref: string): Promise<string | undefined> {
784+
const target =
785+
(await this.provider.config.getConfig(repoPath, `branch.${ref}.gk-merge-target`)) ??
786+
(await this.provider.config.getConfig(repoPath, `branch.${ref}.gk-target-base`));
787+
return target?.trim() || undefined;
788+
}
799789

800-
await this.provider.config.setConfig(repoPath, targetBaseConfigKey, target);
790+
@log()
791+
async getStoredUserMergeTargetBranchName(repoPath: string, ref: string): Promise<string | undefined> {
792+
const target = await this.provider.config.getConfig(repoPath, `branch.${ref}.gk-merge-target-user`);
793+
return target?.trim() || undefined;
801794
}
802795

803796
@log()
804797
async renameBranch(repoPath: string, oldName: string, newName: string): Promise<void> {
805798
await this.git.exec({ cwd: repoPath }, 'branch', '-m', oldName, newName);
806799
}
800+
801+
@log()
802+
async storeBaseBranchName(repoPath: string, ref: string, base: string): Promise<void> {
803+
await this.provider.config.setConfig(repoPath, `branch.${ref}.gk-merge-base`, base);
804+
}
805+
806+
@log()
807+
async storeMergeTargetBranchName(repoPath: string, ref: string, target: string): Promise<void> {
808+
await this.provider.config.setConfig(repoPath, `branch.${ref}.gk-merge-target`, target);
809+
}
810+
811+
@log()
812+
async storeUserMergeTargetBranchName(repoPath: string, ref: string, target: string | undefined): Promise<void> {
813+
await this.provider.config.setConfig(repoPath, `branch.${ref}.gk-merge-target-user`, target);
814+
}
807815
}

0 commit comments

Comments
 (0)