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

[sergiolms]

Description

This task creates the git revert commands to move away from runTerminalCommand

Solves #3530

Checklist

• I have followed the guidelines in the Contributing document
• My changes follow the coding style of this project
• My changes build without any errors or warnings
• My changes have been formatted and linted
• My changes include any required corresponding changes to the documentation (including CHANGELOG.md and README.md)
• My changes have been rebased and squashed to the minimal number (typically 1) of relevant commits
• My changes have a descriptive commit message with a short title, including a Fixes $XXX - or Closes #XXX - prefix to auto-close the issue that your PR addresses

[axosoft-ramint]

I'm not sure if this is a pattern in other places or not but, why at the git provider service level should the caller provide flags instead of just passing in options? Flags feel like low-level terminal args that should be abstracted down to the git.ts level and anything coming in from above should really only be dealing with a list of options.

[sergiolms]

Yeah, we could parse that in src/commands/git/* directly. Since currently this method is only being called from commands I didn't have preference on where to perform that conversion, but we can absolutely do that before and send the options directly to the method. Even better, maybe we can have an alternative

createFlagsQuickPickItem<Flags>(state.flags, ['--no-edit'], {
	label: this.title,
	description: '--no-edit',
	detail: `Will revert ${getReferenceLabel(state.references)}`,
}),

That accepts and returns an option set so we don't have to parse the flags every time. Let me think this through

[axosoft-ramint]

This should take constrained options, generate the flags and params necessary off of those options, and then run the command with the args it generated.

[sergiolms]

That is being performed at localGitProvider. From my understanding, localGitProvider is an abstraction from GitLens that uses the local implementation for git (the git binary in our case). As such, it should receive the options (business logic) and make the proper adaptations to call the local git implementation (the git methods).
The current implementation keeps the methods in git as atomic as possible and separated from our business logic (we could even move the error parsing to localGitProvider if necessary).

If you think it would be clearer to perform that here, I could move that logic here and leave the revert at localGitProvider with just the git call. Is this what you mean?

// localGitProvider.ts 
export class LocalGitProvider implements GitProvider, Disposable {
	// ...

	revert(repoPath: string, ref: string, options?: { edit?: boolean }): Promise<void> {
		return this.git.revert(repoPath, ref, options);
	}
}


// git.ts
export class Git {
	// ...
	
	async revert(repoPath: string, ref: string, options?: { edit?: boolean }) {
		const args = [];
		if (options.edit !== undefined) {
			args.push(options.edit ? '--edit' : '--no-edit');
		}
		
		args.push(ref);

		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, ref);
				}
			}

			throw new RevertError(RevertErrorReason.Other, ex, ref);
		}
	}
}

[axosoft-ramint]

This is probably going to result in me receiving a crash course on git revert flows, but for my own education, can you explain what this is doing?

[sergiolms]

The revert command could return a "changes would be overwritten" error when the user had WIP while reverting. With this piece of code, we would catch that, let the user know they have WIP they should either commit or stash, and ask them what to do. Then, depending on the option they chose, we run & wait for the command and retry the revert action (unless they cancelled).

EDIT: Just tried it, and autostash is only available for rebase. I've seen some inconsistencies with the retry, since it fires before the other command finishes. Removing it now.