From 8406e3a8e5e714ac41a0f4d262669e72e41b27b2 Mon Sep 17 00:00:00 2001 From: Tomas Vik Date: Tue, 11 May 2021 12:05:20 +0000 Subject: [PATCH] refactor: expose GitService and GitLab service in WrappedRepository --- src/commands/insert_snippet.ts | 11 ++- src/git/git_extension_wrapper.test.ts | 13 +++- src/git/git_extension_wrapper.ts | 6 ++ src/git/wrapped_repository.test.ts | 80 ++++++++++++++++++++ src/git/wrapped_repository.ts | 73 +++++++++++++++++++ src/git_service.test.ts | 7 +- src/gitlab_service.ts | 6 +- src/openers.ts | 10 +-- src/test_utils/fake_git_extension.ts | 21 +++++- src/utils/get_extension_configuration.ts | 2 + src/utils/get_instance_url.test.ts | 83 --------------------- src/utils/get_instance_url.ts | 93 ++---------------------- test/integration/mr_review.test.js | 2 +- test/integration/user_agent.test.js | 3 +- test/integration/webview.test.js | 2 +- 15 files changed, 216 insertions(+), 196 deletions(-) create mode 100644 src/git/wrapped_repository.test.ts delete mode 100644 src/utils/get_instance_url.test.ts diff --git a/src/commands/insert_snippet.ts b/src/commands/insert_snippet.ts index c315e84..2329a4e 100644 --- a/src/commands/insert_snippet.ts +++ b/src/commands/insert_snippet.ts @@ -1,7 +1,6 @@ import * as vscode from 'vscode'; import { gitExtensionWrapper } from '../git/git_extension_wrapper'; import { GqlBlob, GqlSnippet } from '../gitlab/graphql/get_snippets'; -import { createGitService, createGitLabNewService } from '../service_factory'; const pickSnippet = async (snippets: GqlSnippet[]) => { const quickPickItems = snippets.map(s => ({ @@ -31,10 +30,10 @@ export const insertSnippet = async (): Promise => { if (!repository) { return; } - const gitService = createGitService(repository.rootFsPath); - const gitLabService = await createGitLabNewService(repository.rootFsPath); - const remote = await gitService.fetchGitRemote(); - const snippets = await gitLabService.getSnippets(`${remote.namespace}/${remote.project}`); + const remote = await repository.gitService.fetchGitRemote(); + const snippets = await repository.gitLabService.getSnippets( + `${remote.namespace}/${remote.project}`, + ); if (snippets.length === 0) { vscode.window.showInformationMessage('There are no project snippets.'); return; @@ -49,7 +48,7 @@ export const insertSnippet = async (): Promise => { if (!blob) { return; } - const snippet = await gitLabService.getSnippetContent(result.original, blob); + const snippet = await repository.gitLabService.getSnippetContent(result.original, blob); const editor = vscode.window.activeTextEditor; await editor.edit(editBuilder => { editBuilder.insert(editor.selection.start, snippet); diff --git a/src/git/git_extension_wrapper.test.ts b/src/git/git_extension_wrapper.test.ts index f649063..a84bfd5 100644 --- a/src/git/git_extension_wrapper.test.ts +++ b/src/git/git_extension_wrapper.test.ts @@ -33,8 +33,8 @@ describe('GitExtensionWrapper', () => { }); describe('repositories', () => { - const fakeRepository = createFakeRepository('/repository/root/path/'); - const fakeRepository2 = createFakeRepository('/repository/root/path2/'); + const fakeRepository = createFakeRepository({ rootUriPath: '/repository/root/path/' }); + const fakeRepository2 = createFakeRepository({ rootUriPath: '/repository/root/path2/' }); it('returns no repositories when the extension is disabled', () => { fakeExtension.gitApi.repositories = [fakeRepository]; @@ -104,5 +104,14 @@ describe('GitExtensionWrapper', () => { fakeRepository2.rootUri.fsPath, ]); }); + + it('returns repository wrapped repository for a repositoryRootPath', () => { + fakeExtension.gitApi.repositories = [fakeRepository, fakeRepository2]; + wrapper.init(); + + const repository = wrapper.getRepository('/repository/root/path/'); + + expect(repository?.rootFsPath).toBe('/repository/root/path/'); + }); }); }); diff --git a/src/git/git_extension_wrapper.ts b/src/git/git_extension_wrapper.ts index a5647ab..3928bb8 100644 --- a/src/git/git_extension_wrapper.ts +++ b/src/git/git_extension_wrapper.ts @@ -48,6 +48,12 @@ export class GitExtensionWrapper implements vscode.Disposable { return this.wrappedRepositories; } + getRepository(repositoryRoot: string): WrappedRepository | undefined { + const rawRepository = this.gitApi?.getRepository(vscode.Uri.file(repositoryRoot)); + if (!rawRepository) return undefined; + return this.repositories.find(r => r.hasSameRootAs(rawRepository)); + } + private register() { assert(this.gitExtension); try { diff --git a/src/git/wrapped_repository.test.ts b/src/git/wrapped_repository.test.ts new file mode 100644 index 0000000..e6af6fe --- /dev/null +++ b/src/git/wrapped_repository.test.ts @@ -0,0 +1,80 @@ +import * as vscode from 'vscode'; +import { WrappedRepository } from './wrapped_repository'; +import { getExtensionConfiguration } from '../utils/get_extension_configuration'; +import { tokenService } from '../services/token_service'; +import { createFakeRepository } from '../test_utils/fake_git_extension'; +import { Repository } from '../api/git'; +import { GITLAB_COM_URL } from '../constants'; + +jest.mock('../utils/get_extension_configuration'); + +describe('WrappedRepository', () => { + let repository: Repository; + let wrappedRepository: WrappedRepository; + + beforeEach(() => { + repository = createFakeRepository(); + wrappedRepository = new WrappedRepository(repository); + }); + + describe('instanceUrl', () => { + let tokens = {}; + const fakeContext = { + globalState: { + get: () => tokens, + }, + }; + + beforeEach(() => { + tokens = {}; + tokenService.init((fakeContext as any) as vscode.ExtensionContext); + }); + it('should return configured instanceUrl', async () => { + (getExtensionConfiguration as jest.Mock).mockReturnValue({ + instanceUrl: 'https://test.com', + }); + + expect(wrappedRepository.instanceUrl).toBe('https://test.com'); + }); + + it('returns default instanceUrl when there is no configuration', async () => { + (getExtensionConfiguration as jest.Mock).mockReturnValue({}); + expect(wrappedRepository.instanceUrl).toBe(GITLAB_COM_URL); + }); + + describe('heuristic', () => { + it('returns instanceUrl when there is exactly one match between remotes and token URLs', async () => { + repository = createFakeRepository({ + remotes: [ + 'https://git@gitlab.com/gitlab-org/gitlab-vscode-extension.git', + 'https://git@test-instance.com/g/extension.git', + ], + }); + tokens = { + 'https://test-instance.com': 'abc', + }; + + wrappedRepository = new WrappedRepository(repository); + + expect(wrappedRepository.instanceUrl).toBe('https://test-instance.com'); + }); + + it('returns default instanceUrl when there is multiple matches between remotes and token URLs', async () => { + repository = createFakeRepository({ + remotes: [ + 'https://git@gitlab.com/gitlab-org/gitlab-vscode-extension.git', + 'https://git@test-instance.com/g/extension.git', + ], + }); + tokens = { + 'https://test-instance.com': 'abc', + 'https://gitlab.com': 'def', + }; + + wrappedRepository = new WrappedRepository(repository); + + expect(wrappedRepository.instanceUrl).toBe(GITLAB_COM_URL); + }); + }); + }); +}); diff --git a/src/git/wrapped_repository.ts b/src/git/wrapped_repository.ts index c1580fe..61a1b64 100644 --- a/src/git/wrapped_repository.ts +++ b/src/git/wrapped_repository.ts @@ -1,6 +1,60 @@ +import * as url from 'url'; import { basename } from 'path'; import { Repository } from '../api/git'; +import { GITLAB_COM_URL } from '../constants'; +import { tokenService } from '../services/token_service'; +import { log } from '../log'; +import { parseGitRemote } from './git_remote_parser'; +import { getExtensionConfiguration } from '../utils/get_extension_configuration'; +import { GitLabNewService } from '../gitlab/gitlab_new_service'; +import { GitService } from '../git_service'; + +function intersectionOfInstanceAndTokenUrls(gitRemoteHosts: string[]) { + const instanceUrls = tokenService.getInstanceUrls(); + + return instanceUrls.filter(instanceUrl => + gitRemoteHosts.includes(url.parse(instanceUrl).host || ''), + ); +} + +function heuristicInstanceUrl(gitRemoteHosts: string[]) { + // if the intersection of git remotes and configured PATs exists and is exactly + // one hostname, use it + const intersection = intersectionOfInstanceAndTokenUrls(gitRemoteHosts); + if (intersection.length === 1) { + const heuristicUrl = intersection[0]; + log(`Found ${heuristicUrl} in the PAT list and git remotes, using it as the instanceUrl`); + return heuristicUrl; + } + + if (intersection.length > 1) { + log(`Found more than one intersection of git remotes and configured PATs, ${intersection}`); + } + + return null; +} + +export function getInstanceUrlFromRemotes(gitRemoteUrls: string[]): string { + const { instanceUrl } = getExtensionConfiguration(); + // if the workspace setting exists, use it + if (instanceUrl) { + return instanceUrl; + } + + // try to determine the instance URL heuristically + const gitRemoteHosts = gitRemoteUrls + .map((uri: string) => parseGitRemote(uri)?.host) + .filter((h): h is string => Boolean(h)); + const heuristicUrl = heuristicInstanceUrl(gitRemoteHosts); + if (heuristicUrl) { + return heuristicUrl; + } + + // default to Gitlab cloud + return GITLAB_COM_URL; +} + export class WrappedRepository { private readonly rawRepository: Repository; @@ -8,6 +62,25 @@ export class WrappedRepository { this.rawRepository = rawRepository; } + get instanceUrl(): string { + const remoteUrls = this.rawRepository.state.remotes + .map(r => r.fetchUrl) + .filter((r): r is string => Boolean(r)); + return getInstanceUrlFromRemotes(remoteUrls); + } + + get gitLabService(): GitLabNewService { + return new GitLabNewService(this.instanceUrl); + } + + get gitService(): GitService { + const { remoteName } = getExtensionConfiguration(); + return new GitService({ + repositoryRoot: this.rootFsPath, + preferredRemoteName: remoteName, + }); + } + get name(): string { return basename(this.rawRepository.rootUri.fsPath); } diff --git a/src/git_service.test.ts b/src/git_service.test.ts index 014a59b..0eb272e 100644 --- a/src/git_service.test.ts +++ b/src/git_service.test.ts @@ -5,6 +5,9 @@ import * as path from 'path'; import simpleGit, { SimpleGit } from 'simple-git'; import { GitService, GitServiceOptions } from './git_service'; import { gitExtensionWrapper } from './git/git_extension_wrapper'; +import { getInstanceUrl } from './utils/get_instance_url'; + +jest.mock('./utils/get_instance_url'); const isMac = () => Boolean(process.platform.match(/darwin/)); @@ -37,9 +40,7 @@ describe('git_service', () => { }); beforeEach(() => { - (vscode.workspace.getConfiguration as jest.Mock).mockReturnValue({ - instanceUrl: 'https://gitlab.com', - }); + (getInstanceUrl as jest.Mock).mockReturnValue('https://gitlab.com'); jest.spyOn(gitExtensionWrapper, 'gitBinaryPath', 'get').mockReturnValue('git'); }); diff --git a/src/gitlab_service.ts b/src/gitlab_service.ts index 7389d45..f28159f 100644 --- a/src/gitlab_service.ts +++ b/src/gitlab_service.ts @@ -40,7 +40,7 @@ const projectCache: Record = {}; let versionCache: string | null = null; async function fetch( - repositoryRoot: string | undefined, + repositoryRoot: string, path: string, method = 'GET', data?: Record, @@ -161,7 +161,7 @@ export async function fetchCurrentUser(repositoryRoot: string): Promise { const repository = await gitExtensionWrapper.getActiveRepositoryOrSelectOne(); if (!repository) return; const project = await gitLabService.fetchCurrentProject(repository.rootFsPath); - const branchName = await createGitService(repository.rootFsPath).fetchTrackingBranchName(); + const branchName = await repository.gitService.fetchTrackingBranchName(); openUrl(`${project!.webUrl}/merge_requests/new?merge_request%5Bsource_branch%5D=${branchName}`); } @@ -142,7 +140,7 @@ export async function compareCurrentBranch(): Promise { if (!repository) return; const project = await gitLabService.fetchCurrentProject(repository.rootFsPath); - const lastCommitId = await createGitService(repository.rootFsPath).fetchLastCommitId(); + const lastCommitId = await repository.gitService.fetchLastCommitId(); if (project && lastCommitId) { openUrl(`${project.webUrl}/compare/master...${lastCommitId}`); diff --git a/src/test_utils/fake_git_extension.ts b/src/test_utils/fake_git_extension.ts index c007658..5cd03bd 100644 --- a/src/test_utils/fake_git_extension.ts +++ b/src/test_utils/fake_git_extension.ts @@ -7,8 +7,21 @@ const removeFromArray = (array: any[], element: any): any[] => { return array.filter(el => el !== element); }; -export const createFakeRepository = (rootUriPath: string): Repository => - (({ rootUri: vscode.Uri.file(rootUriPath) } as unknown) as Repository); +export const fakeStateOptions = { + rootUriPath: '/path/to/repo', + remotes: ['git@a.com:gitlab/extension.git', 'git@b.com:gitlab/extension.git'], +}; +export const createFakeRepository = ( + options: Partial = {}, +): Repository => { + const { rootUriPath, remotes } = { ...fakeStateOptions, ...options }; + return ({ + rootUri: vscode.Uri.file(rootUriPath), + state: { + remotes: remotes.map(r => ({ fetchUrl: r })), + }, + } as unknown) as Repository; +}; /** * This is a simple test double for the native Git extension API @@ -40,6 +53,10 @@ class FakeGitApi { }; } + getRepository(uri: vscode.Uri) { + return this.repositories.find(r => r.rootUri.toString() === uri.toString()); + } + registerRemoteSourceProvider(provider: any) { this.remoteSourceProviders.push(provider); return { diff --git a/src/utils/get_extension_configuration.ts b/src/utils/get_extension_configuration.ts index 0386adf..a58c8ed 100644 --- a/src/utils/get_extension_configuration.ts +++ b/src/utils/get_extension_configuration.ts @@ -2,6 +2,7 @@ import * as vscode from 'vscode'; import { CONFIG_NAMESPACE } from '../constants'; interface ExtensionConfiguration { + instanceUrl?: string; remoteName?: string; pipelineGitRemoteName?: string; featureFlags?: string[]; @@ -13,6 +14,7 @@ const turnNullToUndefined = (val: T | null | undefined): T | undefined => val export function getExtensionConfiguration(): ExtensionConfiguration { const workspaceConfig = vscode.workspace.getConfiguration(CONFIG_NAMESPACE); return { + instanceUrl: turnNullToUndefined(workspaceConfig.instanceUrl), remoteName: turnNullToUndefined(workspaceConfig.remoteName), pipelineGitRemoteName: turnNullToUndefined(workspaceConfig.pipelineGitRemoteName), featureFlags: turnNullToUndefined(workspaceConfig.featureFlags), diff --git a/src/utils/get_instance_url.test.ts b/src/utils/get_instance_url.test.ts deleted file mode 100644 index d254828..0000000 --- a/src/utils/get_instance_url.test.ts +++ /dev/null @@ -1,83 +0,0 @@ -import * as temp from 'temp'; -import * as vscode from 'vscode'; -import simpleGit, { SimpleGit } from 'simple-git'; -import { getInstanceUrl } from './get_instance_url'; -import { tokenService } from '../services/token_service'; -import { GITLAB_COM_URL } from '../constants'; -import { gitExtensionWrapper } from '../git/git_extension_wrapper'; - -describe('get_instance_url', () => { - const ORIGIN = 'origin'; - const SECOND_REMOTE = 'second'; // name is important, we need this remote to be alphabetically behind origin - - let repositoryRoot: string; - let git: SimpleGit; - - temp.track(); // clean temporary folders after the tests finish - - const createTempFolder = (): Promise => - new Promise((resolve, reject) => { - temp.mkdir('vscodeWorkplace', (err, dirPath) => { - if (err) reject(err); - resolve(dirPath); - }); - }); - - beforeEach(() => { - jest.spyOn(gitExtensionWrapper, 'gitBinaryPath', 'get').mockReturnValue('git'); - }); - - it('returns configured instanceUrl if the config contains one', async () => { - (vscode.workspace.getConfiguration as jest.Mock).mockReturnValue({ - instanceUrl: 'https://test.com', - }); - expect(await getInstanceUrl()).toBe('https://test.com'); - }); - - it('returns default instanceUrl when there is no configuration', async () => { - (vscode.workspace.getConfiguration as jest.Mock).mockReturnValue({}); - expect(await getInstanceUrl()).toBe(GITLAB_COM_URL); - }); - - describe('heuristic', () => { - let tokens = {}; - const fakeContext = { - globalState: { - get: () => tokens, - }, - }; - beforeEach(async () => { - repositoryRoot = await createTempFolder(); - git = simpleGit(repositoryRoot, { binary: 'git' }); - await git.init(); - await git.addRemote(ORIGIN, 'https://git@gitlab.com/gitlab-org/gitlab-vscode-extension.git'); - tokens = {}; - tokenService.init((fakeContext as any) as vscode.ExtensionContext); - }); - - it('returns instanceUrl when there is exactly one match between remotes and token URLs', async () => { - await git.addRemote(SECOND_REMOTE, 'https://git@test-instance.com/g/extension.git'); - tokens = { - 'https://test-instance.com': 'abc', - }; - expect(await getInstanceUrl(repositoryRoot)).toBe('https://test-instance.com'); - }); - - it('returns default instanceUrl when there is multiple matches between remotes and token URLs', async () => { - await git.addRemote(SECOND_REMOTE, 'https://git@test-instance.com/g/extension.git'); - tokens = { - 'https://test-instance.com': 'abc', - 'https://gitlab.com': 'def', - }; - expect(await getInstanceUrl(repositoryRoot)).toBe(GITLAB_COM_URL); - }); - - it('it works with URLs in git format', async () => { - await git.addRemote(SECOND_REMOTE, 'git@test-instance.com:g/extension.git'); - tokens = { - 'https://test-instance.com': 'abc', - }; - expect(await getInstanceUrl(repositoryRoot)).toBe('https://test-instance.com'); - }); - }); -}); diff --git a/src/utils/get_instance_url.ts b/src/utils/get_instance_url.ts index 6d9b9b9..ff2473e 100644 --- a/src/utils/get_instance_url.ts +++ b/src/utils/get_instance_url.ts @@ -1,91 +1,8 @@ -import * as vscode from 'vscode'; -import * as execa from 'execa'; -import * as url from 'url'; -import { GITLAB_COM_URL } from '../constants'; -import { tokenService } from '../services/token_service'; -import { log } from '../log'; -import { parseGitRemote } from '../git/git_remote_parser'; +import * as assert from 'assert'; import { gitExtensionWrapper } from '../git/git_extension_wrapper'; -async function fetch(cmd: string, repositoryRoot: string): Promise { - const [, ...args] = cmd.trim().split(' '); - const { stdout } = await execa(gitExtensionWrapper.gitBinaryPath, args, { - cwd: repositoryRoot, - preferLocal: false, - }); - return stdout; -} - -async function fetchGitRemoteUrls(repositoryRoot: string): Promise { - const fetchGitRemotesVerbose = async (): Promise => { - const output = await fetch('git remote -v', repositoryRoot); - - return (output || '').split('\n'); - }; - - const parseRemoteFromVerboseLine = (line: string) => { - // git remote -v output looks like - // origin[TAB]git@gitlab.com:gitlab-org/gitlab-vscode-extension.git[WHITESPACE](fetch) - // the interesting part is surrounded by a tab symbol and a whitespace - - return line.split(/\t| /)[1]; - }; - - const remotes = await fetchGitRemotesVerbose(); - const remoteUrls = remotes.map(remote => parseRemoteFromVerboseLine(remote)).filter(Boolean); - - // git remote -v returns a (fetch) and a (push) line for each remote, - // so we need to remove duplicates - return [...new Set(remoteUrls)]; -} - -async function intersectionOfInstanceAndTokenUrls(repositoryRoot: string) { - const uriHostname = (uri: string) => parseGitRemote(uri)?.host; - - const instanceUrls = tokenService.getInstanceUrls(); - const gitRemotes = await fetchGitRemoteUrls(repositoryRoot); - const gitRemoteHosts = gitRemotes.map(uriHostname); - - return instanceUrls.filter(instanceUrl => - gitRemoteHosts.includes(url.parse(instanceUrl).host || undefined), - ); -} - -async function heuristicInstanceUrl(repositoryRoot: string) { - // if the intersection of git remotes and configured PATs exists and is exactly - // one hostname, use it - const intersection = await intersectionOfInstanceAndTokenUrls(repositoryRoot); - if (intersection.length === 1) { - const heuristicUrl = intersection[0]; - log(`Found ${heuristicUrl} in the PAT list and git remotes, using it as the instanceUrl`); - return heuristicUrl; - } - - if (intersection.length > 1) { - log(`Found more than one intersection of git remotes and configured PATs, ${intersection}`); - } - - return null; -} - -export async function getInstanceUrl(repositoryRoot?: string): Promise { - // FIXME: if you are touching this configuration statement, move the configuration to get_extension_configuration.ts - const { instanceUrl } = vscode.workspace.getConfiguration('gitlab'); - // if the workspace setting exists, use it - if (instanceUrl) { - return instanceUrl; - } - - // legacy logic in GitLabService might not have the workspace folder available - // in that case we just skip the heuristic - if (repositoryRoot) { - // try to determine the instance URL heuristically - const heuristicUrl = await heuristicInstanceUrl(repositoryRoot); - if (heuristicUrl) { - return heuristicUrl; - } - } - - // default to Gitlab cloud - return GITLAB_COM_URL; +export function getInstanceUrl(repositoryRoot: string): string { + const repository = gitExtensionWrapper.getRepository(repositoryRoot); + assert(repository, `${repositoryRoot} doesn't contain a git repository`); + return repository.instanceUrl; } diff --git a/test/integration/mr_review.test.js b/test/integration/mr_review.test.js index 8e7e216..2159f75 100644 --- a/test/integration/mr_review.test.js +++ b/test/integration/mr_review.test.js @@ -57,7 +57,7 @@ describe('MR Review', () => { beforeEach(() => { server.resetHandlers(); dataProvider = new IssuableDataProvider(); - mrItemModel = new MrItemModel(openMergeRequestResponse, getRepositoryRoot()); + mrItemModel = new MrItemModel(openMergeRequestResponse, { uri: getRepositoryRoot() }); }); after(async () => { diff --git a/test/integration/user_agent.test.js b/test/integration/user_agent.test.js index dfa30b9..ddad020 100644 --- a/test/integration/user_agent.test.js +++ b/test/integration/user_agent.test.js @@ -8,6 +8,7 @@ const gitLabService = require('../../src/gitlab_service'); const { GitLabNewService } = require('../../src/gitlab/gitlab_new_service'); const snippetsResponse = require('./fixtures/graphql/snippets.json'); const packageJson = require('../../package.json'); +const { getRepositoryRoot } = require('./test_infrastructure/helpers'); const validateUserAgent = req => { const userAgent = req.headers.get('User-Agent'); @@ -50,7 +51,7 @@ describe('User-Agent header', () => { }); it('is sent with requests from GitLabService', async () => { - await gitLabService.fetchCurrentUser(); + await gitLabService.fetchCurrentUser(getRepositoryRoot()); validateUserAgent(capturedRequest); }); diff --git a/test/integration/webview.test.js b/test/integration/webview.test.js index fda40e0..7fd67f0 100644 --- a/test/integration/webview.test.js +++ b/test/integration/webview.test.js @@ -80,7 +80,7 @@ describe('GitLab webview', () => { replacePanelEventSystem(); webviewPanel = await webviewController.create( openIssueResponse, - vscode.workspace.workspaceFolders[0], + vscode.workspace.workspaceFolders[0].uri.fsPath, ); }); -- GitLab