3533 Change reset terminal-run commands into normal commands w/ proper error handling

src/commands/git/reset.ts

@@ -4,8 +4,10 @@ import type { GitLog } from '../../git/models/log';
 import type { GitReference, GitRevisionReference, GitTagReference } from '../../git/models/reference';
 import { getReferenceLabel } 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,
@@ -27,12 +29,15 @@ interface Context {
 	title: string;
 }
 
-type Flags = '--hard' | '--soft';
+type ResetOptions = {
+	hard?: boolean;
+	soft?: boolean;
+};
 
 interface State {
 	repo: string | Repository;
 	reference: GitRevisionReference | GitTagReference;
-	flags: Flags[];
+	options: ResetOptions;
 }
 
 export interface ResetGitCommandArgs {
@@ -69,8 +74,13 @@ export class ResetGitCommand extends QuickCommand<State> {
 		return this._canSkipConfirm;
 	}
 
-	execute(state: ResetStepState) {
-		state.repo.reset(...state.flags, state.reference.ref);
+	async execute(state: ResetStepState) {
+		try {
+			await state.repo.git.reset(state.options, state.reference.ref);
+		} catch (ex) {
+			Logger.error(ex, this.title);
+			void showGenericErrorMessage(ex.message);
+		}
 	}
 
 	protected async *steps(state: PartialStepState<State>): StepGenerator {
@@ -82,8 +92,8 @@ export class ResetGitCommand extends QuickCommand<State> {
 			title: this.title,
 		};
 
-		if (state.flags == null) {
-			state.flags = [];
+		if (state.options == null) {
+			state.options = {};
 		}
 
 		let skippedStepOne = false;
@@ -152,34 +162,34 @@ export class ResetGitCommand extends QuickCommand<State> {
 				const result = yield* this.confirmStep(state as ResetStepState, context);
 				if (result === StepResultBreak) continue;
 
-				state.flags = result;
+				state.options = Object.assign({}, ...result);
 			}
 
 			endSteps(state);
-			this.execute(state as ResetStepState);
+			await this.execute(state as ResetStepState);
 		}
 
 		return state.counter < 0 ? StepResultBreak : undefined;
 	}
 
-	private *confirmStep(state: ResetStepState, context: Context): StepResultGenerator<Flags[]> {
-		const step: QuickPickStep<FlagsQuickPickItem<Flags>> = this.createConfirmStep(
+	private *confirmStep(state: ResetStepState, context: Context): StepResultGenerator<ResetOptions[]> {
+		const step: QuickPickStep<FlagsQuickPickItem<ResetOptions>> = this.createConfirmStep(
 			appendReposToTitle(`Confirm ${context.title}`, state, context),
 			[
-				createFlagsQuickPickItem<Flags>(state.flags, [], {
+				createFlagsQuickPickItem<ResetOptions>([], [], {
 					label: this.title,
 					detail: `Will reset (leaves changes in the working tree) ${getReferenceLabel(
 						context.destination,
 					)} to ${getReferenceLabel(state.reference)}`,
 				}),
-				createFlagsQuickPickItem<Flags>(state.flags, ['--soft'], {
+				createFlagsQuickPickItem<ResetOptions>([], [{ soft: true }], {
 					label: `Soft ${this.title}`,
 					description: '--soft',
 					detail: `Will soft reset (leaves changes in the index and working tree) ${getReferenceLabel(
 						context.destination,
 					)} to ${getReferenceLabel(state.reference)}`,
 				}),
-				createFlagsQuickPickItem<Flags>(state.flags, ['--hard'], {
+				createFlagsQuickPickItem<ResetOptions>([], [{ hard: true }], {
 					label: `Hard ${this.title}`,
 					description: '--hard',
 					detail: `Will hard reset (discards all changes) ${getReferenceLabel(

src/env/node/git/git.ts

@@ -20,6 +20,8 @@ import {
 	PullErrorReason,
 	PushError,
 	PushErrorReason,
+	ResetError,
+	ResetErrorReason,
 	StashPushError,
 	StashPushErrorReason,
 	TagError,
@@ -73,38 +75,42 @@ const textDecoder = new TextDecoder('utf8');
 const rootSha = '4b825dc642cb6eb9a060e54bf8d69288fbee4904';
 
 export const GitErrors = {
+	alreadyCheckedOut: /already checked out/i,
+	alreadyExists: /already exists/i,
+	ambiguousArgument: /fatal:\s*ambiguous argument ['"].+['"]: unknown revision or path not in the working tree/i,
 	badRevision: /bad revision '(.*?)'/i,
 	cantLockRef: /cannot lock ref|unable to update local ref/i,
 	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,
+	entryNotUpToDate: /error:\s*Entry ['"].+['"] not uptodate\. Cannot merge\./i,
 	failedToDeleteDirectoryNotEmpty: /failed to delete '(.*?)': Directory not empty/i,
+	invalidLineCount: /file .+? has only \d+ lines/i,
 	invalidObjectName: /invalid object name: (.*)\s/i,
 	invalidObjectNameList: /could not open object name list: (.*)\s/i,
+	invalidTagName: /invalid tag name/i,
+	mainWorkingTree: /is a main working tree/i,
 	noFastForward: /\(non-fast-forward\)/i,
 	noMergeBase: /no merge base/i,
 	noRemoteRepositorySpecified: /No remote repository specified\./i,
+	noUpstream: /^fatal: The current branch .* has no upstream branch/i,
+	noUserNameConfigured: /Please tell me who you are\./i,
 	notAValidObjectName: /Not a valid object name/i,
 	notAWorkingTree: /'(.*?)' is not a working tree/i,
-	noUserNameConfigured: /Please tell me who you are\./i,
-	invalidLineCount: /file .+? has only \d+ lines/i,
-	uncommittedChanges: /contains modified or untracked files/i,
-	alreadyExists: /already exists/i,
-	alreadyCheckedOut: /already checked out/i,
-	mainWorkingTree: /is a main working tree/i,
-	noUpstream: /^fatal: The current branch .* has no upstream branch/i,
 	permissionDenied: /Permission.*denied/i,
 	pushRejected: /^error: failed to push some refs to\b/m,
 	rebaseMultipleBranches: /cannot rebase onto multiple branches/i,
+	refLocked: /fatal:\s*cannot lock ref ['"].+['"]: unable to create file/i,
 	remoteAhead: /rejected because the remote contains work/i,
 	remoteConnection: /Could not read from remote repository/i,
+	remoteRejected: /rejected because the remote contains work/i,
+	tagAlreadyExists: /tag .* already exists/i,
 	tagConflict: /! \[rejected\].*\(would clobber existing tag\)/m,
+	tagNotFound: /tag .* not found/i,
+	uncommittedChanges: /contains modified or untracked files/i,
+	unmergedChanges: /error:\s*you need to resolve your current index first/i,
 	unmergedFiles: /is not possible because you have unmerged files/i,
 	unstagedChanges: /You have unstaged changes/i,
-	tagAlreadyExists: /tag .* already exists/i,
-	tagNotFound: /tag .* not found/i,
-	invalidTagName: /invalid tag name/i,
-	remoteRejected: /rejected because the remote contains work/i,
 };
 
 const GitWarnings = {
@@ -173,6 +179,13 @@ const tagErrorAndReason: [RegExp, TagErrorReason][] = [
 	[GitErrors.remoteRejected, TagErrorReason.RemoteRejected],
 ];
 
+const resetErrorAndReason: [RegExp, ResetErrorReason][] = [
+	[GitErrors.unmergedChanges, ResetErrorReason.UnmergedChanges],
+	[GitErrors.ambiguousArgument, ResetErrorReason.AmbiguousArgument],
+	[GitErrors.entryNotUpToDate, ResetErrorReason.EntryNotUpToDate],
+	[GitErrors.refLocked, ResetErrorReason.RefLocked],
+];
+
 export class Git {
 	/** Map of running git commands -- avoids running duplicate overlaping commands */
 	private readonly pendingCommands = new Map<string, Promise<string | Buffer>>();
@@ -1584,8 +1597,19 @@ export class Git {
 		return this.git<string>({ cwd: repoPath }, 'remote', 'get-url', remote);
 	}
 
-	reset(repoPath: string | undefined, pathspecs: string[]) {
-		return this.git<string>({ cwd: repoPath }, 'reset', '-q', '--', ...pathspecs);
+	async reset(repoPath: string, pathspecs: string[], ...args: string[]) {
+		try {
+			await this.git<string>({ cwd: repoPath }, 'reset', '-q', ...args, '--', ...pathspecs);
+		} catch (ex) {
+			const msg: string = ex?.toString() ?? '';
+			for (const [error, reason] of resetErrorAndReason) {
+				if (error.test(msg) || error.test(ex.stderr ?? '')) {
+					throw new ResetError(reason, ex);
+				}
+			}
+
+			throw new ResetError(ResetErrorReason.Other, ex);
+		}
 	}
 
 	async rev_list(

src/env/node/git/localGitProvider.ts

@@ -5974,6 +5974,18 @@ export class LocalGitProvider implements GitProvider, Disposable {
 		}
 	}
 
+	@log()
+	async reset(repoPath: string, ref: string, options?: { hard?: boolean; soft?: boolean }): Promise<void> {
+		const flags = [];
+		if (options?.hard) {
+			flags.push('--hard');
+		} else if (options?.soft) {
+			flags.push('--soft');
+		}
+
+		await this.git.reset(repoPath, [], ...flags, ref);
+	}
+
 	@log({ args: { 2: false } })
 	async runGitCommandViaTerminal(
 		repoPath: string,

src/git/actions/repository.ts

@@ -41,12 +41,12 @@ export function rebase(repo?: string | Repository, ref?: GitReference, interacti
 export function reset(
 	repo?: string | Repository,
 	ref?: GitRevisionReference | GitTagReference,
-	flags?: NonNullable<ResetGitCommandArgs['state']>['flags'],
+	options?: NonNullable<ResetGitCommandArgs['state']>['options'],
 ) {
 	return executeGitCommand({
 		command: 'reset',
-		confirm: flags == null || flags.includes('--hard'),
-		state: { repo: repo, reference: ref, flags: flags },
+		confirm: options == null || options.hard,
+		state: { repo: repo, reference: ref, options: options },
 	});
 }
 

src/git/errors.ts

@@ -567,3 +567,54 @@ export class TagError extends Error {
 		return this;
 	}
 }
+
+export const enum ResetErrorReason {
+	UnmergedChanges,
+	AmbiguousArgument,
+	EntryNotUpToDate,
+	RefLocked,
+	Other,
+}
+
+export class ResetError extends Error {
+	static is(ex: unknown, reason?: ResetErrorReason): ex is ResetError {
+		return ex instanceof ResetError && (reason == null || ex.reason === reason);
+	}
+
+	readonly original?: Error;
+	readonly reason: ResetErrorReason | undefined;
+	constructor(reason?: ResetErrorReason, original?: Error);
+	constructor(message?: string, original?: Error);
+	constructor(messageOrReason: string | ResetErrorReason | undefined, original?: Error) {
+		let message;
+		let reason: ResetErrorReason | undefined;
+		if (messageOrReason == null) {
+			message = 'Unable to reset';
+		} else if (typeof messageOrReason === 'string') {
+			message = messageOrReason;
+			reason = undefined;
+		} else {
+			reason = messageOrReason;
+			message = 'Unable to reset';
+			switch (reason) {
+				case ResetErrorReason.UnmergedChanges:
+					message = `${message} because there are unmerged changes`;
+					break;
+				case ResetErrorReason.AmbiguousArgument:
+					message = `${message} because the argument is ambiguous`;
+					break;
+				case ResetErrorReason.EntryNotUpToDate:
+					message = `${message} because the entry is not up to date`;
+					break;
+				case ResetErrorReason.RefLocked:
+					message = `${message} because the ref is locked`;
+					break;
+			}
+		}
+		super(message);
+
+		this.original = original;
+		this.reason = reason;
+		Error.captureStackTrace?.(this, ResetError);
+	}
+}

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>;
 
+	reset?(repoPath: string, ref: string, options?: { hard?: boolean; soft?: boolean }): Promise<void>;
+
 	applyUnreachableCommitForPatch?(
 		repoPath: string,
 		ref: string,

src/git/gitProviderService.ts

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

src/git/models/repository.ts

@@ -850,11 +850,6 @@ export class Repository implements Disposable {
 		);
 	}
 
-	@log()
-	reset(...args: string[]) {
-		void this.runTerminalCommand('reset', ...args);
-	}
-
 	resume() {
 		if (!this._suspended) return;