3530 Change revert terminal-run commands into normal commands w/ proper error handling

src/commands/git/revert.ts

@@ -1,11 +1,16 @@
+import { Commands } from '../../constants.commands';
 import type { Container } from '../../container';
+import { RevertError, RevertErrorReason } from '../../git/errors';
 import type { GitBranch } from '../../git/models/branch';
 import type { GitLog } from '../../git/models/log';
 import type { GitRevisionReference } from '../../git/models/reference';
 import { getReferenceLabel } from '../../git/models/reference';
 import type { Repository } from '../../git/models/repository';
+import { showGenericErrorMessage, showShouldCommitOrStashPrompt } from '../../messages';
 import type { FlagsQuickPickItem } from '../../quickpicks/items/flags';
 import { createFlagsQuickPickItem } from '../../quickpicks/items/flags';
+import { Logger } from '../../system/logger';
+import { executeCommand, executeCoreCommand } from '../../system/vscode/command';
 import type { ViewsWithRepositoryFolders } from '../../views/viewBase';
 import type {
 	PartialStepState,
@@ -27,12 +32,12 @@ interface Context {
 	title: string;
 }
 
-type Flags = '--edit' | '--no-edit';
+type RevertOptions = { edit?: boolean };
 
 interface State<Refs = GitRevisionReference | GitRevisionReference[]> {
 	repo: string | Repository;
 	references: Refs;
-	flags: Flags[];
+	options: RevertOptions;
 }
 
 export interface RevertGitCommandArgs {
@@ -71,8 +76,30 @@ export class RevertGitCommand extends QuickCommand<State> {
 		return false;
 	}
 
-	execute(state: RevertStepState<State<GitRevisionReference[]>>) {
-		state.repo.revert(...state.flags, ...state.references.map(c => c.ref).reverse());
+	async execute(state: RevertStepState<State<GitRevisionReference[]>>) {
+		for (const ref of state.references.reverse()) {
+			try {
+				await state.repo.git.revert(ref.ref, state.options);
+			} catch (ex) {
+				if (RevertError.is(ex, RevertErrorReason.LocalChangesWouldBeOverwritten)) {
+					const response = await showShouldCommitOrStashPrompt();
+					if (response == null || response === 'Cancel') {
+						continue;
+					}
+
+					if (response === 'Stash') {
+						await executeCommand(Commands.GitCommandsStashPush);
+					} else if (response === 'Commit') {
+						await executeCoreCommand('workbench.view.scm');
+					}
+
+					continue;
+				}
+
+				Logger.error(ex, this.title);
+				void showGenericErrorMessage(ex.message);
+			}
+		}
 	}
 
 	protected async *steps(state: PartialStepState<State>): StepGenerator {
@@ -84,8 +111,8 @@ export class RevertGitCommand 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)) {
@@ -157,25 +184,25 @@ export class RevertGitCommand extends QuickCommand<State> {
 			const result = yield* this.confirmStep(state as RevertStepState, context);
 			if (result === StepResultBreak) continue;
 
-			state.flags = result;
+			state.options = Object.assign({}, ...result);
 
 			endSteps(state);
-			this.execute(state as RevertStepState<State<GitRevisionReference[]>>);
+			await this.execute(state as RevertStepState<State<GitRevisionReference[]>>);
 		}
 
 		return state.counter < 0 ? StepResultBreak : undefined;
 	}
 
-	private *confirmStep(state: RevertStepState, context: Context): StepResultGenerator<Flags[]> {
-		const step: QuickPickStep<FlagsQuickPickItem<Flags>> = this.createConfirmStep(
+	private *confirmStep(state: RevertStepState, context: Context): StepResultGenerator<RevertOptions[]> {
+		const step: QuickPickStep<FlagsQuickPickItem<RevertOptions>> = this.createConfirmStep(
 			appendReposToTitle(`Confirm ${context.title}`, state, context),
 			[
-				createFlagsQuickPickItem<Flags>(state.flags, ['--no-edit'], {
+				createFlagsQuickPickItem<RevertOptions>([], [{ edit: false }], {
 					label: this.title,
 					description: '--no-edit',
 					detail: `Will revert ${getReferenceLabel(state.references)}`,
 				}),
-				createFlagsQuickPickItem<Flags>(state.flags, ['--edit'], {
+				createFlagsQuickPickItem<RevertOptions>([], [{ edit: true }], {
 					label: `${this.title} & Edit`,
 					description: '--edit',
 					detail: `Will revert and edit ${getReferenceLabel(state.references)}`,

src/env/node/git/git.ts

@@ -20,6 +20,8 @@ import {
 	PullErrorReason,
 	PushError,
 	PushErrorReason,
+	RevertError,
+	RevertErrorReason,
 	StashPushError,
 	StashPushErrorReason,
 	TagError,
@@ -105,6 +107,8 @@ export const GitErrors = {
 	tagNotFound: /tag .* not found/i,
 	invalidTagName: /invalid tag name/i,
 	remoteRejected: /rejected because the remote contains work/i,
+	revertHasConflicts: /(error: could not revert .*) (hint: After resolving the conflicts)/gi,
+	localChangesWouldBeOverwritten: /error: your local changes would be overwritten/i,
 };
 
 const GitWarnings = {
@@ -173,6 +177,14 @@ const tagErrorAndReason: [RegExp, TagErrorReason][] = [
 	[GitErrors.remoteRejected, TagErrorReason.RemoteRejected],
 ];
 
+const revertErrorAndReason = [
+	[GitErrors.badRevision, RevertErrorReason.BadRevision],
+	[GitErrors.invalidObjectName, RevertErrorReason.InvalidObjectName],
+	[GitErrors.revertHasConflicts, RevertErrorReason.Conflict],
+	[GitErrors.changesWouldBeOverwritten, RevertErrorReason.LocalChangesWouldBeOverwritten],
+	[GitErrors.localChangesWouldBeOverwritten, RevertErrorReason.LocalChangesWouldBeOverwritten],
+];
+
 export class Git {
 	/** Map of running git commands -- avoids running duplicate overlaping commands */
 	private readonly pendingCommands = new Map<string, Promise<string | Buffer>>();
@@ -1588,6 +1600,21 @@ export class Git {
 		return this.git<string>({ cwd: repoPath }, 'reset', '-q', '--', ...pathspecs);
 	}
 
+	async revert(repoPath: string, ...args: string[]) {
+		try {
+			await this.git<string>({ cwd: repoPath }, 'revert', ...args);
+		} catch (ex) {
+			const msg: string = ex?.toString() ?? '';
+			for (const [error, reason] of revertErrorAndReason) {
+				if (error.test(msg) || error.test(ex.stderr ?? '')) {
+					throw new RevertError(reason, ex);
+				}
+			}
+
+			throw new RevertError(RevertErrorReason.Other, ex);
+		}
+	}
+
 	async rev_list(
 		repoPath: string,
 		ref: string,

src/env/node/git/localGitProvider.ts

@@ -31,6 +31,7 @@ import {
 	PullError,
 	PushError,
 	PushErrorReason,
+	RevertError,
 	StashApplyError,
 	StashApplyErrorReason,
 	StashPushError,
@@ -6242,6 +6243,24 @@ export class LocalGitProvider implements GitProvider, Disposable {
 		return worktrees;
 	}
 
+	@log()
+	async revert(repoPath: string, ref: string, options?: { edit?: boolean }): Promise<void> {
+		const args = [];
+		if (options.edit !== undefined) {
+			args.push(options.edit ? '--edit' : '--no-edit');
+		}
+
+		try {
+			await this.git.revert(repoPath, ...args, ref);
+		} catch (ex) {
+			if (ex instanceof RevertError) {
+				throw ex.WithRef(ref);
+			}
+
+			throw ex;
+		}
+	}
+
 	@log()
 	// eslint-disable-next-line @typescript-eslint/require-await
 	async getWorktreesDefaultUri(repoPath: string): Promise<Uri | undefined> {

src/git/errors.ts

@@ -567,3 +567,65 @@ export class TagError extends Error {
 		return this;
 	}
 }
+
+export const enum RevertErrorReason {
+	BadRevision,
+	InvalidObjectName,
+	Conflict,
+	LocalChangesWouldBeOverwritten,
+	Other,
+}
+
+export class RevertError extends Error {
+	static is(ex: unknown, reason?: RevertErrorReason): ex is RevertError {
+		return ex instanceof RevertError && (reason == null || ex.reason === reason);
+	}
+
+	readonly original?: Error;
+	readonly reason: RevertErrorReason | undefined;
+	ref?: string;
+
+	constructor(reason?: RevertErrorReason, original?: Error, ref?: string);
+	constructor(message?: string, original?: Error);
+	constructor(messageOrReason: string | RevertErrorReason | undefined, original?: Error, ref?: string) {
+		let message;
+		let reason: RevertErrorReason | undefined;
+		if (messageOrReason == null) {
+			message = 'Unable to revert';
+		} else if (typeof messageOrReason === 'string') {
+			message = messageOrReason;
+			reason = undefined;
+		} else {
+			reason = messageOrReason;
+			message = RevertError.buildRevertErrorMessage(reason, ref);
+		}
+		super(message);
+
+		this.original = original;
+		this.reason = reason;
+		this.ref = ref;
+		Error.captureStackTrace?.(this, RevertError);
+	}
+
+	WithRef(ref: string): this {
+		this.ref = ref;
+		this.message = RevertError.buildRevertErrorMessage(this.reason, ref);
+		return this;
+	}
+
+	private static buildRevertErrorMessage(reason?: RevertErrorReason, ref?: string): string {
+		const baseMessage = `Unable to revert${ref ? ` revision '${ref}'` : ''}`;
+		switch (reason) {
+			case RevertErrorReason.BadRevision:
+				return `${baseMessage} because it is not a valid revision.`;
+			case RevertErrorReason.InvalidObjectName:
+				return `${baseMessage} because it is not a valid object name.`;
+			case RevertErrorReason.Conflict:
+				return `${baseMessage} it has unresolved conflicts. Resolve the conflicts and try again.`;
+			case RevertErrorReason.LocalChangesWouldBeOverwritten:
+				return `${baseMessage} because local changes would be overwritten. Commit or stash your changes first.`;
+			default:
+				return `${baseMessage}.`;
+		}
+	}
+}

src/git/gitProvider.ts

@@ -125,6 +125,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>;
+	revert?(repoPath: string, ref: string, options?: { edit?: boolean }): Promise<void>;
 
 	applyUnreachableCommitForPatch?(
 		repoPath: string,

src/git/gitProviderService.ts

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

src/git/models/repository.ts

@@ -871,11 +871,6 @@ export class Repository implements Disposable {
 		}
 	}
 
-	@log()
-	revert(...args: string[]) {
-		void this.runTerminalCommand('revert', ...args);
-	}
-
 	async setRemoteAsDefault(remote: GitRemote, value: boolean = true) {
 		await this.container.storage.storeWorkspace('remote:default', value ? remote.name : undefined);
 

src/messages.ts

@@ -230,6 +230,22 @@ export function showIntegrationRequestTimedOutWarningMessage(providerName: strin
 	);
 }
 
+export async function showShouldCommitOrStashPrompt(): Promise<string | undefined> {
+	const stash = { title: 'Stash' };
+	const commit = { title: 'Commit' };
+	const cancel = { title: 'Cancel', isCloseAffordance: true };
+	const result = await showMessage(
+		'warn',
+		'You have changes in your working tree. Commit or stash them before reverting',
+		undefined,
+		null,
+		stash,
+		commit,
+		cancel,
+	);
+	return result?.title;
+}
+
 export async function showWhatsNewMessage(majorVersion: string) {
 	const confirm = { title: 'OK', isCloseAffordance: true };
 	const releaseNotes = { title: 'View Release Notes' };