Merge branch '265-git-service-error-handling' into 'main'

fix(git_service): throw errors when git command fails, don't return null

See merge request gitlab-org/gitlab-vscode-extension!169

src/commands/insert_snippet.ts

@@ -34,9 +34,6 @@ export const insertSnippet = async (): Promise<void> => {
   const gitService = createGitService(workspaceFolder);
   const gitLabService = await createGitLabNewService(workspaceFolder);
   const remote = await gitService.fetchGitRemote();
-  if (!remote) {
-    throw new Error('Could not get parsed remote for your workspace');
-  }
   const snippets = await gitLabService.getSnippets(`${remote.namespace}/${remote.project}`);
   if (snippets.length === 0) {
     vscode.window.showInformationMessage('There are no project snippets.');

src/data_providers/issuable.ts

@@ -6,6 +6,8 @@ import * as gitLabService from '../gitlab_service';
 import { CustomQuery } from '../gitlab/custom_query';
 import { ItemModel } from './items/item_model';
 import { CONFIG_CUSTOM_QUERIES, CONFIG_NAMESPACE } from '../constants';
+import { logError } from '../log';
+import { ErrorItem } from './items/error_item';
 
 export class DataProvider implements vscode.TreeDataProvider<ItemModel | vscode.TreeItem> {
   private eventEmitter = new vscode.EventEmitter<void>();
@@ -19,8 +21,13 @@ export class DataProvider implements vscode.TreeDataProvider<ItemModel | vscode.
 
     this.children.forEach(ch => ch.dispose());
     this.children = [];
-
-    const projects = await gitLabService.getAllGitlabProjects();
+    let projects: VsProject[] = [];
+    try {
+      projects = await gitLabService.getAllGitlabProjects();
+    } catch (e) {
+      logError(e);
+      return [new ErrorItem('Fetching Issues and MRs failed')];
+    }
     if (projects.length === 0) return [new vscode.TreeItem('No projects found')];
     const customQueries =
       vscode.workspace

src/data_providers/items/error_item.ts

 import { TreeItem, ThemeIcon } from 'vscode';
+import { USER_COMMANDS } from '../../command_names';
 
 export class ErrorItem extends TreeItem {
   constructor(message = 'Error occurred, please try to refresh.') {
     super(message);
     this.iconPath = new ThemeIcon('error');
+    this.command = {
+      command: USER_COMMANDS.SHOW_OUTPUT,
+      title: 'Show error output',
+    };
   }
 }

src/git_service.test.ts

@@ -83,18 +83,6 @@ describe('git_service', () => {
       });
     });
 
-    describe('fetchBranchName', () => {
-      it('gets the current branch name', async () => {
-        await git.checkout(['-b', 'new-branch']);
-        // TODO if we use git branch command, we don't have to create a commit
-        await git.commit('Test commit', [], { '--allow-empty': null });
-
-        const branchName = await gitService.fetchBranchName();
-
-        expect(branchName).toEqual('new-branch');
-      });
-    });
-
     describe('fetchLastCommitId', () => {
       it('returns the last commit sha', async () => {
         await git.commit('Test commit', [], { '--allow-empty': null });
@@ -155,24 +143,20 @@ describe('git_service', () => {
       gitService = new GitService(options);
     });
 
-    it('fetchGitRemote returns null', async () => {
-      expect(await gitService.fetchGitRemote()).toEqual(null);
-    });
-
-    it('fetchBranchName returns null', async () => {
-      expect(await gitService.fetchBranchName()).toEqual(null);
+    it('fetchGitRemote returns throws', async () => {
+      expect(gitService.fetchGitRemote()).rejects.toBeInstanceOf(Error);
     });
 
     it('fetchLastCommitId returns null', async () => {
-      expect(await gitService.fetchLastCommitId()).toEqual(null);
+      expect(gitService.fetchLastCommitId()).rejects.toBeInstanceOf(Error);
     });
 
     it('fetchGitRemotePipeline returns null', async () => {
-      expect(await gitService.fetchGitRemotePipeline()).toEqual(null);
+      expect(gitService.fetchGitRemotePipeline()).rejects.toBeInstanceOf(Error);
     });
 
     it('fetchTrackingBranchName returns null', async () => {
-      expect(await gitService.fetchTrackingBranchName()).toEqual(null);
+      expect(gitService.fetchTrackingBranchName()).rejects.toBeInstanceOf(Error);
     });
   });
 });

src/git_service.ts

 import * as execa from 'execa';
+import { UserFriendlyError } from './errors/user_friendly_error';
 import { parseGitRemote, GitRemote } from './git/git_remote_parser';
 import { getInstanceUrl } from './utils/get_instance_url';
 
@@ -25,28 +26,24 @@ export class GitService {
     this.log = options.log;
   }
 
-  private async fetch(cmd: string): Promise<string | null> {
+  private async fetch(cmd: string): Promise<string> {
     const [git, ...args] = cmd.trim().split(' ');
-    try {
-      const { stdout } = await execa(git, args, {
-        cwd: this.workspaceFolder,
-        preferLocal: false,
-      });
-      return stdout;
-    } catch (e) {
-      this.log(`${e.message}\n${e.stack}`);
-    }
-    return null;
+    const { stdout } = await execa(git, args, {
+      cwd: this.workspaceFolder,
+      preferLocal: false,
+    });
+    return stdout;
   }
 
-  private async fetchRemoteUrl(remoteName = ''): Promise<GitRemote | null> {
+  private async fetchRemoteUrl(remoteName = ''): Promise<GitRemote> {
     // If remote name isn't provided, the command returns default remote for the current branch
+    // if there's no default branch, the command fails but that's part of the normal flow and it can't throw
     const getUrlForRemoteName = async (name: string) =>
-      this.fetch(`git ls-remote --get-url ${name}`);
+      this.fetch(`git ls-remote --get-url ${name}`).catch(e => null);
 
     const getFirstRemoteName = async () => {
       const multilineRemotes = await this.fetch('git remote');
-      return (multilineRemotes || '').split('\n')[0];
+      return multilineRemotes.split('\n')[0];
     };
 
     let remoteUrl = await getUrlForRemoteName(remoteName);
@@ -56,21 +53,20 @@ export class GitService {
     }
 
     if (remoteUrl) {
-      return parseGitRemote(remoteUrl, await getInstanceUrl(this.workspaceFolder));
+      const parsedRemote = parseGitRemote(remoteUrl, await getInstanceUrl(this.workspaceFolder));
+      if (!parsedRemote) {
+        throw new Error(`git remote "${remoteUrl}" could not be parsed`);
+      }
+      return parsedRemote;
     }
-
-    return null;
+    throw new Error('"git remote" does not return any URL');
   }
 
-  async fetchGitRemote(): Promise<GitRemote | null> {
+  async fetchGitRemote(): Promise<GitRemote> {
     return await this.fetchRemoteUrl(this.remoteName);
   }
 
-  async fetchBranchName(): Promise<string | null> {
-    return await this.fetch('git rev-parse --abbrev-ref HEAD');
-  }
-
-  async fetchLastCommitId(): Promise<string | null> {
+  async fetchLastCommitId(): Promise<string> {
     return await this.fetch('git log --format=%H -n 1');
   }
 
@@ -85,22 +81,25 @@ export class GitService {
    * Fixes #1 where local branch name is renamed and doesn't exists on remote but
    * local branch still tracks another branch on remote.
    */
-  async fetchTrackingBranchName(): Promise<string | null> {
-    const branchName = await this.fetchBranchName();
-
+  async fetchTrackingBranchName(): Promise<string> {
     try {
-      const ref = await this.fetch(`git config --get branch.${branchName}.merge`);
-
-      if (ref) {
-        return ref.replace('refs/heads/', '');
+      const branchName = await this.fetch('git rev-parse --abbrev-ref HEAD');
+
+      try {
+        const ref = await this.fetch(`git config --get branch.${branchName}.merge`);
+
+        if (ref) {
+          return ref.replace('refs/heads/', '');
+        }
+      } catch (e) {
+        this.log(
+          `Couldn't find tracking branch. Extension will fallback to branch name ${branchName}`,
+        );
       }
+
+      return branchName;
     } catch (e) {
-      this.log(
-        `Couldn't find tracking branch. Extension will fallback to branch name ${branchName}`,
-      );
-      this.log(`${e.message}\n${e.stack}`);
+      throw new UserFriendlyError('Cannot get current git branch', e);
     }
-
-    return branchName;
   }
 }