#3531 Change cherry-pick terminal-run commands into normal commands w/ proper error handling

src/commands/git/cherry-pick.ts

@@ -4,8 +4,10 @@ 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 { FlagsQuickPickItem } from '../../quickpicks/items/flags';
 import { createFlagsQuickPickItem } from '../../quickpicks/items/flags';
+import { Logger } from '../../system/logger';
 import type { ViewsWithRepositoryFolders } from '../../views/viewBase';
 import type {
 	PartialStepState,
@@ -29,12 +31,15 @@ interface Context {
 	title: string;
 }
 
-type Flags = '--edit' | '--no-commit';
+type CherryPickOptions = {
+	noCommit?: boolean;
+	edit?: boolean;
+};
 
 interface State<Refs = GitReference | GitReference[]> {
 	repo: string | Repository;
 	references: Refs;
-	flags: Flags[];
+	options: CherryPickOptions;
 }
 
 export interface CherryPickGitCommandArgs {
@@ -80,8 +85,15 @@ export class CherryPickGitCommand extends QuickCommand<State> {
 		return false;
 	}
 
-	execute(state: CherryPickStepState<State<GitReference[]>>) {
-		state.repo.cherryPick(...state.flags, ...state.references.map(c => c.ref).reverse());
+	async execute(state: CherryPickStepState<State<GitReference[]>>) {
+		for (const ref of state.references.map(c => c.ref).reverse()) {
+			try {
+				await state.repo.git.cherryPick(ref, state.options);
+			} catch (ex) {
+				Logger.error(ex, this.title);
+				void showGenericErrorMessage(ex.message);
+			}
+		}
 	}
 
 	override isFuzzyMatch(name: string) {
@@ -99,8 +111,8 @@ export class CherryPickGitCommand extends QuickCommand<State> {
 			title: this.title,
 		};
 
-		if (state.flags == null) {
-			state.flags = [];
+		if (state.options == null) {
+			state.options = {};
 		}
 
 		if (state.references != null && !Array.isArray(state.references)) {
@@ -221,35 +233,35 @@ export class CherryPickGitCommand extends QuickCommand<State> {
 				const result = yield* this.confirmStep(state as CherryPickStepState, context);
 				if (result === StepResultBreak) continue;
 
-				state.flags = result;
+				state.options = Object.assign({}, ...result);
 			}
 
 			endSteps(state);
-			this.execute(state as CherryPickStepState<State<GitReference[]>>);
+			await this.execute(state as CherryPickStepState<State<GitReference[]>>);
 		}
 
 		return state.counter < 0 ? StepResultBreak : undefined;
 	}
 
-	private *confirmStep(state: CherryPickStepState, context: Context): StepResultGenerator<Flags[]> {
-		const step: QuickPickStep<FlagsQuickPickItem<Flags>> = createConfirmStep(
+	private *confirmStep(state: CherryPickStepState, context: Context): StepResultGenerator<CherryPickOptions[]> {
+		const step: QuickPickStep<FlagsQuickPickItem<CherryPickOptions>> = createConfirmStep(
 			appendReposToTitle(`Confirm ${context.title}`, state, context),
 			[
-				createFlagsQuickPickItem<Flags>(state.flags, [], {
+				createFlagsQuickPickItem<CherryPickOptions>([], [], {
 					label: this.title,
 					detail: `Will apply ${getReferenceLabel(state.references, { label: false })} to ${getReferenceLabel(
 						context.destination,
 						{ label: false },
 					)}`,
 				}),
-				createFlagsQuickPickItem<Flags>(state.flags, ['--edit'], {
+				createFlagsQuickPickItem<CherryPickOptions>([], [{ edit: true }], {
 					label: `${this.title} & Edit`,
 					description: '--edit',
 					detail: `Will edit and apply ${getReferenceLabel(state.references, {
 						label: false,
 					})} to ${getReferenceLabel(context.destination, { label: false })}`,
 				}),
-				createFlagsQuickPickItem<Flags>(state.flags, ['--no-commit'], {
+				createFlagsQuickPickItem<CherryPickOptions>([], [{ noCommit: true }], {
 					label: `${this.title} without Committing`,
 					description: '--no-commit',
 					detail: `Will apply ${getReferenceLabel(state.references, { label: false })} to ${getReferenceLabel(

src/env/node/git/git.ts

@@ -78,6 +78,8 @@ export const GitErrors = {
 	changesWouldBeOverwritten: /Your local changes to the following files would be overwritten/i,
 	commitChangesFirst: /Please, commit your changes before you can/i,
 	conflict: /^CONFLICT \([^)]+\): \b/m,
+	cherryPickUnmerged:
+		/error: Cherry-picking.*unmerged files\.\nhint:.*\nhint:.*make a commit\.\nfatal: cherry-pick failed/i,
 	failedToDeleteDirectoryNotEmpty: /failed to delete '(.*?)': Directory not empty/i,
 	invalidObjectName: /invalid object name: (.*)\s/i,
 	invalidObjectNameList: /could not open object name list: (.*)\s/i,
@@ -165,6 +167,12 @@ function getStdinUniqueKey(): number {
 type ExitCodeOnlyGitCommandOptions = GitCommandOptions & { exitCodeOnly: true };
 export type PushForceOptions = { withLease: true; ifIncludes?: boolean } | { withLease: false; ifIncludes?: never };
 
+const cherryPickErrorAndReason: [RegExp, CherryPickErrorReason][] = [
+	[GitErrors.changesWouldBeOverwritten, CherryPickErrorReason.AbortedWouldOverwrite],
+	[GitErrors.conflict, CherryPickErrorReason.Conflicts],
+	[GitErrors.cherryPickUnmerged, CherryPickErrorReason.Conflicts],
+];
+
 const tagErrorAndReason: [RegExp, TagErrorReason][] = [
 	[GitErrors.tagAlreadyExists, TagErrorReason.TagAlreadyExists],
 	[GitErrors.tagNotFound, TagErrorReason.TagNotFound],
@@ -617,28 +625,18 @@ export class Git {
 		return this.git<string>({ cwd: repoPath }, ...params);
 	}
 
-	async cherrypick(repoPath: string, sha: string, options: { noCommit?: boolean; errors?: GitErrorHandling } = {}) {
-		const params = ['cherry-pick'];
-		if (options?.noCommit) {
-			params.push('-n');
-		}
-		params.push(sha);
-
+	async cherryPick(repoPath: string, options: { errors?: GitErrorHandling } = {}, args: string[]) {
 		try {
-			await this.git<string>({ cwd: repoPath, errors: options?.errors }, ...params);
+			await this.git<string>({ cwd: repoPath, errors: options?.errors }, 'cherry-pick', ...args);
 		} catch (ex) {
 			const msg: string = ex?.toString() ?? '';
-			let reason: CherryPickErrorReason = CherryPickErrorReason.Other;
-			if (
-				GitErrors.changesWouldBeOverwritten.test(msg) ||
-				GitErrors.changesWouldBeOverwritten.test(ex.stderr ?? '')
-			) {
-				reason = CherryPickErrorReason.AbortedWouldOverwrite;
-			} else if (GitErrors.conflict.test(msg) || GitErrors.conflict.test(ex.stdout ?? '')) {
-				reason = CherryPickErrorReason.Conflicts;
+			for (const [error, reason] of cherryPickErrorAndReason) {
+				if (error.test(msg) || error.test(ex.stderr ?? '')) {
+					throw new CherryPickError(reason, ex);
+				}
 			}
 
-			throw new CherryPickError(reason, ex, sha);
+			throw new CherryPickError(CherryPickErrorReason.Other, ex);
 		}
 	}
 

src/env/node/git/localGitProvider.ts

@@ -1097,6 +1097,34 @@ export class LocalGitProvider implements GitProvider, Disposable {
 		this.container.events.fire('git:cache:reset', { repoPath: repoPath, caches: ['remotes'] });
 	}
 
+	@log()
+	async cherryPick(
+		repoPath: string,
+		ref: string,
+		options: { noCommit?: boolean; edit?: boolean; errors?: GitErrorHandling },
+	): Promise<void> {
+		const args: string[] = [];
+		if (options?.noCommit) {
+			args.push('-n');
+		}
+
+		if (options?.edit) {
+			args.push('-e');
+		}
+
+		args.push(ref);
+
+		try {
+			await this.git.cherryPick(repoPath, undefined, args);
+		} catch (ex) {
+			if (ex instanceof CherryPickError) {
+				throw ex.WithRef(ref);
+			}
+
+			throw ex;
+		}
+	}
+
 	@log()
 	async applyChangesToWorkingFile(uri: GitUri, ref1?: string, ref2?: string) {
 		const scope = getLogScope();
@@ -1246,7 +1274,7 @@ export class LocalGitProvider implements GitProvider, Disposable {
 
 		// Apply the patch using a cherry pick without committing
 		try {
-			await this.git.cherrypick(targetPath, ref, { noCommit: true, errors: GitErrorHandling.Throw });
+			await this.git.cherryPick(targetPath, undefined, [ref, '--no-commit']);
 		} catch (ex) {
 			Logger.error(ex, scope);
 			if (ex instanceof CherryPickError) {

src/git/errors.ts

@@ -364,37 +364,49 @@ export class CherryPickError extends Error {
 
 	readonly original?: Error;
 	readonly reason: CherryPickErrorReason | undefined;
+	ref?: string;
+
+	private static buildCherryPickErrorMessage(reason: CherryPickErrorReason | undefined, ref?: string) {
+		let baseMessage = `Unable to cherry-pick${ref ? ` commit '${ref}'` : ''}`;
+		switch (reason) {
+			case CherryPickErrorReason.AbortedWouldOverwrite:
+				baseMessage += ' as some local changes would be overwritten';
+				break;
+			case CherryPickErrorReason.Conflicts:
+				baseMessage += ' due to conflicts';
+				break;
+		}
+		return baseMessage;
+	}
 
 	constructor(reason?: CherryPickErrorReason, original?: Error, sha?: string);
 	constructor(message?: string, original?: Error);
-	constructor(messageOrReason: string | CherryPickErrorReason | undefined, original?: Error, sha?: string) {
-		let message;
-		const baseMessage = `Unable to cherry-pick${sha ? ` commit '${sha}'` : ''}`;
+	constructor(messageOrReason: string | CherryPickErrorReason | undefined, original?: Error, ref?: string) {
 		let reason: CherryPickErrorReason | undefined;
-		if (messageOrReason == null) {
-			message = baseMessage;
-		} else if (typeof messageOrReason === 'string') {
-			message = messageOrReason;
-			reason = undefined;
+		if (typeof messageOrReason !== 'string') {
+			reason = messageOrReason as CherryPickErrorReason;
 		} else {
-			reason = messageOrReason;
-			switch (reason) {
-				case CherryPickErrorReason.AbortedWouldOverwrite:
-					message = `${baseMessage} as some local changes would be overwritten.`;
-					break;
-				case CherryPickErrorReason.Conflicts:
-					message = `${baseMessage} due to conflicts.`;
-					break;
-				default:
-					message = baseMessage;
-			}
+			super(messageOrReason);
 		}
+
+		const message =
+			typeof messageOrReason === 'string'
+				? messageOrReason
+				: CherryPickError.buildCherryPickErrorMessage(messageOrReason as CherryPickErrorReason, ref);
 		super(message);
 
 		this.original = original;
 		this.reason = reason;
+		this.ref = ref;
+
 		Error.captureStackTrace?.(this, CherryPickError);
 	}
+
+	WithRef(ref: string) {
+		this.ref = ref;
+		this.message = CherryPickError.buildCherryPickErrorMessage(this.reason, ref);
+		return this;
+	}
 }
 
 export class WorkspaceUntrustedError extends Error {

src/git/gitProvider.ts

@@ -126,6 +126,8 @@ export interface GitProviderRepository {
 	pruneRemote?(repoPath: string, name: string): Promise<void>;
 	removeRemote?(repoPath: string, name: string): Promise<void>;
 
+	cherryPick?(repoPath: string, ref: string, options: { noCommit?: boolean; edit?: boolean }): Promise<void>;
+
 	applyUnreachableCommitForPatch?(
 		repoPath: string,
 		ref: string,

src/git/gitProviderService.ts

@@ -1334,6 +1334,18 @@ export class GitProviderService implements Disposable {
 		return provider.removeRemote(path, name);
 	}
 
+	@log()
+	cherryPick(
+		repoPath: string | Uri,
+		ref: string,
+		options: { noCommit?: boolean; edit?: boolean } = {},
+	): Promise<void> {
+		const { provider, path } = this.getProvider(repoPath);
+		if (provider.cherryPick == null) throw new ProviderNotSupportedError(provider.descriptor.name);
+
+		return provider.cherryPick(path, ref, options);
+	}
+
 	@log()
 	applyChangesToWorkingFile(uri: GitUri, ref1?: string, ref2?: string): Promise<void> {
 		const { provider } = this.getProvider(uri);

src/git/models/repository.ts

@@ -634,11 +634,6 @@ export class Repository implements Disposable {
 		}
 	}
 
-	@log()
-	cherryPick(...args: string[]) {
-		void this.runTerminalCommand('cherry-pick', ...args);
-	}
-
 	containsUri(uri: Uri) {
 		return this === this.container.git.getRepository(uri);
 	}