#3532: Change merge terminal-run commands into normal commands w/ proper error handling

src/commands/git/merge.ts

@@ -1,13 +1,16 @@
 import type { Container } from '../../container';
+import type { MergeOptions } from '../../git/gitProvider';
 import type { GitBranch } from '../../git/models/branch';
 import type { GitLog } from '../../git/models/log';
 import type { GitReference } from '../../git/models/reference';
 import { createRevisionRange, getReferenceLabel, isRevisionReference } from '../../git/models/reference';
 import type { Repository } from '../../git/models/repository';
+import { showGenericErrorMessage } from '../../messages';
 import type { DirectiveQuickPickItem } from '../../quickpicks/items/directive';
 import { createDirectiveQuickPickItem, Directive } from '../../quickpicks/items/directive';
 import type { FlagsQuickPickItem } from '../../quickpicks/items/flags';
 import { createFlagsQuickPickItem } from '../../quickpicks/items/flags';
+import { Logger } from '../../system/logger';
 import { pluralize } from '../../system/string';
 import type { ViewsWithRepositoryFolders } from '../../views/viewBase';
 import type {
@@ -35,12 +38,10 @@ interface Context {
 	title: string;
 }
 
-type Flags = '--ff-only' | '--no-ff' | '--squash' | '--no-commit';
-
 interface State {
 	repo: string | Repository;
 	reference: GitReference;
-	flags: Flags[];
+	options: MergeOptions;
 }
 
 export interface MergeGitCommandArgs {
@@ -76,8 +77,13 @@ export class MergeGitCommand extends QuickCommand<State> {
 		return false;
 	}
 
-	execute(state: MergeStepState) {
-		state.repo.merge(...state.flags, state.reference.ref);
+	async execute(state: MergeStepState) {
+		try {
+			await state.repo.git.merge(state.reference.ref, state.options);
+		} catch (ex) {
+			Logger.error(ex, this.title);
+			void showGenericErrorMessage(ex);
+		}
 	}
 
 	protected async *steps(state: PartialStepState<State>): StepGenerator {
@@ -93,8 +99,8 @@ export class MergeGitCommand extends QuickCommand<State> {
 			title: this.title,
 		};
 
-		if (state.flags == null) {
-			state.flags = [];
+		if (state.options == null) {
+			state.options = {};
 		}
 
 		let skippedStepOne = false;
@@ -197,16 +203,16 @@ export class MergeGitCommand extends QuickCommand<State> {
 			const result = yield* this.confirmStep(state as MergeStepState, context);
 			if (result === StepResultBreak) continue;
 
-			state.flags = result;
+			state.options = Object.assign({}, ...result);
 
 			endSteps(state);
-			this.execute(state as MergeStepState);
+			await this.execute(state as MergeStepState);
 		}
 
 		return state.counter < 0 ? StepResultBreak : undefined;
 	}
 
-	private async *confirmStep(state: MergeStepState, context: Context): AsyncStepResultGenerator<Flags[]> {
+	private async *confirmStep(state: MergeStepState, context: Context): AsyncStepResultGenerator<MergeOptions[]> {
 		const counts = await this.container.git.getLeftRightCommitCount(
 			state.repo.path,
 			createRevisionRange(context.destination.ref, state.reference.ref, '...'),
@@ -240,31 +246,31 @@ export class MergeGitCommand extends QuickCommand<State> {
 			return StepResultBreak;
 		}
 
-		const step: QuickPickStep<FlagsQuickPickItem<Flags>> = this.createConfirmStep(
+		const step: QuickPickStep<FlagsQuickPickItem<MergeOptions>> = this.createConfirmStep(
 			appendReposToTitle(`Confirm ${title}`, state, context),
 			[
-				createFlagsQuickPickItem<Flags>(state.flags, [], {
+				createFlagsQuickPickItem<MergeOptions>([], [], {
 					label: this.title,
 					detail: `Will merge ${pluralize('commit', count)} from ${getReferenceLabel(state.reference, {
 						label: false,
 					})} into ${getReferenceLabel(context.destination, { label: false })}`,
 				}),
-				createFlagsQuickPickItem<Flags>(state.flags, ['--ff-only'], {
+				createFlagsQuickPickItem<MergeOptions>([], [{ fastForwardOnly: true }], {
 					label: `Fast-forward ${this.title}`,
 					description: '--ff-only',
 					detail: `Will fast-forward merge ${pluralize('commit', count)} from ${getReferenceLabel(
 						state.reference,
 						{ label: false },
 					)} into ${getReferenceLabel(context.destination, { label: false })}`,
 				}),
-				createFlagsQuickPickItem<Flags>(state.flags, ['--squash'], {
+				createFlagsQuickPickItem<MergeOptions>([], [{ squash: true }], {
 					label: `Squash ${this.title}`,
 					description: '--squash',
 					detail: `Will squash ${pluralize('commit', count)} from ${getReferenceLabel(state.reference, {
 						label: false,
 					})} into one when merging into ${getReferenceLabel(context.destination, { label: false })}`,
 				}),
-				createFlagsQuickPickItem<Flags>(state.flags, ['--no-ff'], {
+				createFlagsQuickPickItem<MergeOptions>([], [{ noFastForward: true }], {
 					label: `No Fast-forward ${this.title}`,
 					description: '--no-ff',
 					detail: `Will create a merge commit when merging ${pluralize(
@@ -275,7 +281,7 @@ export class MergeGitCommand extends QuickCommand<State> {
 						{ label: false },
 					)}`,
 				}),
-				createFlagsQuickPickItem<Flags>(state.flags, ['--no-ff', '--no-commit'], {
+				createFlagsQuickPickItem<MergeOptions>([], [{ noCommit: true, noFastForward: true }], {
 					label: `Don't Commit ${this.title}`,
 					description: '--no-commit --no-ff',
 					detail: `Will pause before committing the merge of ${pluralize(

src/commands/git/switch.ts

@@ -104,7 +104,7 @@ export class SwitchGitCommand extends QuickCommand<State> {
 		);
 
 		if (state.fastForwardTo != null) {
-			state.repos[0].merge('--ff-only', state.fastForwardTo.ref);
+			await state.repos[0].git.merge(state.fastForwardTo.ref, { fastForwardOnly: true });
 		}
 	}
 
@@ -211,7 +211,7 @@ export class SwitchGitCommand extends QuickCommand<State> {
 				);
 				if (worktree != null && !worktree.isDefault) {
 					if (state.fastForwardTo != null) {
-						state.repos[0].merge('--ff-only', state.fastForwardTo.ref);
+						await state.repos[0].git.merge(state.fastForwardTo.ref, { fastForwardOnly: true });
 					}
 
 					const worktreeResult = yield* getSteps(

src/env/node/git/git.ts

@@ -16,6 +16,8 @@ import {
 	CherryPickErrorReason,
 	FetchError,
 	FetchErrorReason,
+	MergeError,
+	MergeErrorReason,
 	PullError,
 	PullErrorReason,
 	PushError,
@@ -173,6 +175,12 @@ const tagErrorAndReason: [RegExp, TagErrorReason][] = [
 	[GitErrors.remoteRejected, TagErrorReason.RemoteRejected],
 ];
 
+const mergeErrorAndReason: [RegExp, MergeErrorReason][] = [
+	[GitErrors.conflict, MergeErrorReason.Conflict],
+	[GitErrors.unmergedFiles, MergeErrorReason.UnmergedFiles],
+	[GitErrors.unstagedChanges, MergeErrorReason.UnstagedChanges],
+];
+
 export class Git {
 	/** Map of running git commands -- avoids running duplicate overlaping commands */
 	private readonly pendingCommands = new Map<string, Promise<string | Buffer>>();
@@ -1092,6 +1100,21 @@ export class Git {
 		}
 	}
 
+	async merge(repoPath: string, args: string[]) {
+		try {
+			await this.git<string>({ cwd: repoPath }, 'merge', ...args);
+		} catch (ex) {
+			const msg: string = ex?.toString() ?? '';
+			for (const [error, reason] of mergeErrorAndReason) {
+				if (error.test(msg) || error.test(ex.stderr ?? '')) {
+					throw new MergeError(reason, ex);
+				}
+			}
+
+			throw new MergeError(MergeErrorReason.Other, ex);
+		}
+	}
+
 	for_each_ref__branch(repoPath: string, options: { all: boolean } = { all: false }) {
 		const params = ['for-each-ref', `--format=${parseGitBranchesDefaultFormat}`, 'refs/heads'];
 		if (options.all) {

src/env/node/git/localGitProvider.ts

@@ -47,6 +47,7 @@ import type {
 	GitProvider,
 	GitProviderDescriptor,
 	LeftRightCommitCountResult,
+	MergeOptions,
 	NextComparisonUrisResult,
 	PagedResult,
 	PagingOptions,
@@ -1097,6 +1098,29 @@ export class LocalGitProvider implements GitProvider, Disposable {
 		this.container.events.fire('git:cache:reset', { repoPath: repoPath, caches: ['remotes'] });
 	}
 
+	@log()
+	async merge(repoPath: string, ref: string, options?: MergeOptions): Promise<void> {
+		const args: string[] = [];
+
+		if (options?.fastForwardOnly) {
+			args.push('--ff-only');
+		} else if (options?.noFastForward) {
+			args.push('--no-ff');
+		}
+
+		if (options?.noCommit) {
+			args.push('--no-commit');
+		}
+
+		if (options?.squash) {
+			args.push('--squash');
+		}
+
+		args.push(ref);
+
+		await this.git.merge(repoPath, args);
+	}
+
 	@log()
 	async applyChangesToWorkingFile(uri: GitUri, ref1?: string, ref2?: string) {
 		const scope = getLogScope();

src/git/errors.ts

@@ -567,3 +567,70 @@ export class TagError extends Error {
 		return this;
 	}
 }
+
+export const enum MergeErrorReason {
+	Conflict,
+	UnmergedFiles,
+	UnstagedChanges,
+	Other,
+}
+
+export class MergeError extends Error {
+	static is(ex: unknown, reason?: MergeErrorReason): ex is MergeError {
+		return ex instanceof MergeError && (reason == null || ex.reason === reason);
+	}
+
+	readonly original?: Error;
+	readonly reason: MergeErrorReason | undefined;
+	ref?: string;
+
+	private static buildMergeErrorMessage(reason?: MergeErrorReason, ref?: string): string {
+		let baseMessage: string;
+		if (ref != null) {
+			baseMessage = `Unable to merge ${ref}`;
+		} else {
+			baseMessage = `Unable to merge`;
+		}
+
+		switch (reason) {
+			case MergeErrorReason.Conflict:
+				return `${baseMessage} due to conflicts`;
+			case MergeErrorReason.UnmergedFiles:
+				return `${baseMessage} because you have unmerged files`;
+			case MergeErrorReason.UnstagedChanges:
+				return `${baseMessage} because you have unstaged changes`;
+			default:
+				return baseMessage;
+		}
+
+		return baseMessage;
+	}
+
+	constructor(reason?: MergeErrorReason, original?: Error, ref?: string);
+	constructor(message?: string, original?: Error);
+	constructor(messageOrReason: string | MergeErrorReason | undefined, original?: Error, ref?: string) {
+		let reason: MergeErrorReason | undefined;
+		if (typeof messageOrReason !== 'string') {
+			reason = messageOrReason as MergeErrorReason;
+		} else {
+			super(messageOrReason);
+		}
+
+		const message =
+			typeof messageOrReason === 'string'
+				? messageOrReason
+				: MergeError.buildMergeErrorMessage(messageOrReason as MergeErrorReason, ref);
+		super(message);
+
+		this.original = original;
+		this.reason = reason;
+		this.ref = ref;
+		Error.captureStackTrace?.(this, MergeError);
+	}
+
+	WithRef(ref: string) {
+		this.ref = ref;
+		this.message = MergeError.buildMergeErrorMessage(this.reason, ref);
+		return this;
+	}
+}

src/git/gitProvider.ts

@@ -117,6 +117,13 @@ export interface BranchContributorOverview {
 	readonly contributors?: GitContributor[];
 }
 
+export type MergeOptions = {
+	fastForwardOnly?: boolean;
+	noFastForward?: boolean;
+	noCommit?: boolean;
+	squash?: boolean;
+};
+
 export interface GitProviderRepository {
 	createBranch?(repoPath: string, name: string, ref: string): Promise<void>;
 	renameBranch?(repoPath: string, oldName: string, newName: string): Promise<void>;
@@ -125,6 +132,7 @@ export interface GitProviderRepository {
 	addRemote?(repoPath: string, name: string, url: string, options?: { fetch?: boolean }): Promise<void>;
 	pruneRemote?(repoPath: string, name: string): Promise<void>;
 	removeRemote?(repoPath: string, name: string): Promise<void>;
+	merge?(repoPath: string, ref: string, options?: MergeOptions): Promise<void>;
 
 	applyUnreachableCommitForPatch?(
 		repoPath: string,

src/git/gitProviderService.ts

@@ -50,6 +50,7 @@ import type {
 	GitProviderDescriptor,
 	GitProviderId,
 	LeftRightCommitCountResult,
+	MergeOptions,
 	NextComparisonUrisResult,
 	PagedResult,
 	PagingOptions,
@@ -1334,6 +1335,14 @@ export class GitProviderService implements Disposable {
 		return provider.removeRemote(path, name);
 	}
 
+	@log()
+	merge(repoPath: string, ref: string, options: MergeOptions = {}): Promise<void> {
+		const { provider, path } = this.getProvider(repoPath);
+		if (provider.merge == null) throw new ProviderNotSupportedError(provider.descriptor.name);
+
+		return provider.merge(path, ref, options);
+	}
+
 	@log()
 	applyChangesToWorkingFile(uri: GitUri, ref1?: string, ref2?: string): Promise<void> {
 		const { provider } = this.getProvider(uri);

src/git/models/repository.ts

@@ -734,11 +734,6 @@ export class Repository implements Disposable {
 		return this.git.getWorktree(w => w.uri.toString() === url);
 	}
 
-	@log()
-	merge(...args: string[]) {
-		void this.runTerminalCommand('merge', ...args);
-	}
-
 	@gate()
 	@log()
 	async pull(options?: { progress?: boolean; rebase?: boolean }) {