From 8be345afeccab0c4d0c64f8037483f47dab3b472 Mon Sep 17 00:00:00 2001 From: Frederic Collonval Date: Sat, 31 Oct 2020 12:48:27 +0100 Subject: [PATCH 1/2] Fix path repository refresh bug --- src/model.ts | 52 ++++++++++--------- src/tokens.ts | 6 +-- .../{GitExtension.spec.tsx => model.spec.tsx} | 38 ++++++++++++++ tests/plugin.spec.ts | 10 +--- 4 files changed, 71 insertions(+), 35 deletions(-) rename tests/{GitExtension.spec.tsx => model.spec.tsx} (88%) diff --git a/src/model.ts b/src/model.ts index 5b60a7370..926bf40db 100644 --- a/src/model.ts +++ b/src/model.ts @@ -137,26 +137,18 @@ export class GitExtension implements IGitExtension { const currentReady = this._readyPromise; this._pendingReadyPromise += 1; this._readyPromise = Promise.all([currentReady, this.showTopLevel(v)]) - .then(r => { - const results = r[1]; - if (results.code === 0) { - this._pathRepository = results.top_repo_path; - change.newValue = results.top_repo_path; - } else { - this._pathRepository = null; - } + .then(([_, path]) => { + change.newValue = this._pathRepository = path; if (change.newValue !== change.oldValue) { this.refresh().then(() => this._repositoryChanged.emit(change)); } + this._pendingReadyPromise -= 1; }) .catch(reason => { + this._pendingReadyPromise -= 1; console.error(`Fail to find Git top level for path ${v}.\n${reason}`); }); - - void this._readyPromise.then(() => { - this._pendingReadyPromise -= 1; - }); } } @@ -885,19 +877,31 @@ export class GitExtension implements IGitExtension { * @throws {Git.GitResponseError} If the server response is not ok * @throws {ServerConnection.NetworkError} If the request cannot be made */ - async showTopLevel(path: string): Promise { - return await this._taskHandler.execute( - 'git:fetch:top_level_path', - async () => { - return await requestAPI( - 'show_top_level', - 'POST', - { - current_path: path - } - ); + async showTopLevel(path: string): Promise { + try { + const data = await this._taskHandler.execute( + 'git:fetch:top_level_path', + async () => { + return await requestAPI( + 'show_top_level', + 'POST', + { + current_path: path + } + ); + } + ); + return data.top_repo_path || null; + } catch (error) { + if ( + error instanceof Git.GitResponseError && + error.response.status === 500 && + error.json.code === 128 + ) { + return null; } - ); + throw error; + } } /** diff --git a/src/tokens.ts b/src/tokens.ts index bfd8accf2..8ef0a2081 100644 --- a/src/tokens.ts +++ b/src/tokens.ts @@ -374,7 +374,7 @@ export interface IGitExtension extends IDisposable { revertCommit(message: string, hash: string): Promise; /** - * Make request for the prefix path of a directory 'path', + * Get the prefix path of a directory 'path', * with respect to the root directory of repository * * @param path Path for which the prefix is searched for @@ -386,14 +386,14 @@ export interface IGitExtension extends IDisposable { showPrefix(path: string): Promise; /** - * Make request for top level path of repository 'path' + * Get the top level path of repository 'path' * * @param path Path from which the top Git repository needs to be found * * @throws {Git.GitResponseError} If the server response is not ok * @throws {ServerConnection.NetworkError} If the request cannot be made */ - showTopLevel(path: string): Promise; + showTopLevel(path: string): Promise; /** * Make request to list all the tags present in the remote repo diff --git a/tests/GitExtension.spec.tsx b/tests/model.spec.tsx similarity index 88% rename from tests/GitExtension.spec.tsx rename to tests/model.spec.tsx index ac6c8cb5e..6545c564f 100644 --- a/tests/GitExtension.spec.tsx +++ b/tests/model.spec.tsx @@ -119,6 +119,44 @@ describe('IGitExtension', () => { }); }); + describe('#showTopLevel', () => { + it('should return a string if the folder is a git repository', async () => { + const fakeRepo = '/path/to/repo'; + mockResponses['show_top_level'] = { + body: () => { + return { code: 0, top_repo_path: fakeRepo }; + } + }; + const topLevel = await model.showTopLevel('/path/to/repo/cwd'); + expect(topLevel).toEqual(fakeRepo); + }); + + it('should return null if the repository is not a git repository', async () => { + mockResponses['show_top_level'] = { + body: () => { + return { code: 128 }; + }, + status: 500 + }; + const topLevel = await model.showTopLevel('/path/to/repo/cwd'); + expect(topLevel).toBeNull(); + }); + + it('should throw an exception if the server otherwise', async () => { + mockResponses['show_top_level'] = { + body: () => { + return { code: 128 }; + }, + status: 401 + }; + try { + await model.showTopLevel('/path/to/repo/cwd'); + } catch (error) { + expect(error).toBeInstanceOf(Git.GitResponseError); + } + }); + }); + describe('#status', () => { it('should be an empty list if not in a git repository', async () => { let status: Git.IStatusFileResult[] = []; diff --git a/tests/plugin.spec.ts b/tests/plugin.spec.ts index f67bee6c9..4c6a37b94 100644 --- a/tests/plugin.spec.ts +++ b/tests/plugin.spec.ts @@ -6,8 +6,7 @@ import { ISettingRegistry, SettingRegistry } from '@jupyterlab/settingregistry'; import { JupyterLab } from '@jupyterlab/application'; import { showErrorMessage } from '@jupyterlab/apputils'; import { URLExt } from '@jupyterlab/coreutils'; -import { ReadonlyJSONObject } from '@lumino/coreutils'; -import { mockedRequestAPI } from './utils'; +import { IMockedResponses, mockedRequestAPI } from './utils'; jest.mock('../src/git'); jest.mock('@jupyterlab/application'); @@ -18,12 +17,7 @@ describe('plugin', () => { const mockGit = git as jest.Mocked; const fakeRoot = '/path/to/server'; let app: jest.Mocked; - let mockResponses: { - [url: string]: { - body?: (request: Object) => ReadonlyJSONObject; - status?: number; - }; - } = {}; + let mockResponses: IMockedResponses = {}; let settingRegistry: jest.Mocked; beforeAll(() => { From 0f923ad55c5aa425418c7ac39db5026c5441a2be Mon Sep 17 00:00:00 2001 From: Frederic Collonval Date: Sat, 31 Oct 2020 12:52:38 +0100 Subject: [PATCH 2/2] Correct showPrefix --- src/model.ts | 32 ++++++++++++++++++++++++-------- src/tokens.ts | 2 +- tests/model.spec.tsx | 38 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 63 insertions(+), 9 deletions(-) diff --git a/src/model.ts b/src/model.ts index 926bf40db..61eb42b76 100644 --- a/src/model.ts +++ b/src/model.ts @@ -857,15 +857,31 @@ export class GitExtension implements IGitExtension { * @throws {Git.GitResponseError} If the server response is not ok * @throws {ServerConnection.NetworkError} If the request cannot be made */ - async showPrefix(path: string): Promise { - return await this._taskHandler.execute( - 'git:fetch:prefix_path', - async () => { - return await requestAPI('show_prefix', 'POST', { - current_path: path - }); + async showPrefix(path: string): Promise { + try { + const data = await this._taskHandler.execute( + 'git:fetch:prefix_path', + async () => { + return await requestAPI( + 'show_prefix', + 'POST', + { + current_path: path + } + ); + } + ); + return data.under_repo_path || null; + } catch (error) { + if ( + error instanceof Git.GitResponseError && + error.response.status === 500 && + error.json.code === 128 + ) { + return null; } - ); + throw error; + } } /** diff --git a/src/tokens.ts b/src/tokens.ts index 8ef0a2081..def8d9068 100644 --- a/src/tokens.ts +++ b/src/tokens.ts @@ -383,7 +383,7 @@ export interface IGitExtension extends IDisposable { * @throws {Git.GitResponseError} If the server response is not ok * @throws {ServerConnection.NetworkError} If the request cannot be made */ - showPrefix(path: string): Promise; + showPrefix(path: string): Promise; /** * Get the top level path of repository 'path' diff --git a/tests/model.spec.tsx b/tests/model.spec.tsx index 6545c564f..27cbe302e 100644 --- a/tests/model.spec.tsx +++ b/tests/model.spec.tsx @@ -119,6 +119,44 @@ describe('IGitExtension', () => { }); }); + describe('#showPrefix', () => { + it('should return a string if the folder is a git repository', async () => { + const fakeRepo = '/repo'; + mockResponses['show_prefix'] = { + body: () => { + return { code: 0, under_repo_path: fakeRepo }; + } + }; + const topLevel = await model.showPrefix('/repo/cwd'); + expect(topLevel).toEqual(fakeRepo); + }); + + it('should return null if the repository is not a git repository', async () => { + mockResponses['show_prefix'] = { + body: () => { + return { code: 128 }; + }, + status: 500 + }; + const topLevel = await model.showPrefix('/repo/cwd'); + expect(topLevel).toBeNull(); + }); + + it('should throw an exception if the server otherwise', async () => { + mockResponses['show_prefix'] = { + body: () => { + return { code: 128 }; + }, + status: 401 + }; + try { + await model.showPrefix('/repo/cwd'); + } catch (error) { + expect(error).toBeInstanceOf(Git.GitResponseError); + } + }); + }); + describe('#showTopLevel', () => { it('should return a string if the folder is a git repository', async () => { const fakeRepo = '/path/to/repo';