Merge pull request #395 from jupyterlab/pathfix2

Use the path relative to server root for diffs

Author: jaipreet-s

jupyterlab_git/handlers.py

@@ -443,6 +443,14 @@ def post(self):
         self.finish(json.dumps(response))
 
 
+class GitServerRootHandler(GitHandler):
+
+    def get(self):
+        # Similar to https://github.com/jupyter/nbdime/blob/master/nbdime/webapp/nb_server_extension.py#L90-L91
+        self.finish(json.dumps({
+            "server_root": getattr(self.contents_manager, 'root_dir', None)
+        }))
+
 def setup_handlers(web_app):
     """
     Setups all of the git command handlers.
@@ -471,7 +479,8 @@ def setup_handlers(web_app):
         ("/git/clone", GitCloneHandler),
         ("/git/upstream", GitUpstreamHandler),
         ("/git/config", GitConfigHandler),
-        ("/git/changed_files", GitChangedFilesHandler)
+        ("/git/changed_files", GitChangedFilesHandler),
+        ("/git/server_root", GitServerRootHandler)
     ]
 
     # add the baseurl to our paths

src/components/FileItem.tsx

@@ -1,34 +1,34 @@
 import { JupyterFrontEnd } from '@jupyterlab/application';
 
 import {
-  changeStageButtonStyle,
   changeStageButtonLeftStyle,
-  discardFileButtonStyle,
-  diffFileButtonStyle
+  changeStageButtonStyle,
+  diffFileButtonStyle,
+  discardFileButtonStyle
 } from '../componentsStyle/GitStageStyle';
 
 import {
-  fileStyle,
-  selectedFileStyle,
-  expandedFileStyle,
   disabledFileStyle,
-  fileIconStyle,
-  fileLabelStyle,
-  fileChangedLabelStyle,
-  selectedFileChangedLabelStyle,
+  discardFileButtonSelectedStyle,
+  expandedFileStyle,
+  fileButtonStyle,
   fileChangedLabelBrandStyle,
   fileChangedLabelInfoStyle,
-  fileButtonStyle,
+  fileChangedLabelStyle,
   fileGitButtonStyle,
-  discardFileButtonSelectedStyle,
+  fileIconStyle,
+  fileLabelStyle,
+  fileStyle,
+  selectedFileChangedLabelStyle,
+  selectedFileStyle,
   sideBarExpandedFileLabelStyle
 } from '../componentsStyle/FileItemStyle';
 
 import { classes } from 'typestyle';
 
 import * as React from 'react';
 
-import { showDialog, Dialog } from '@jupyterlab/apputils';
+import { Dialog, showDialog } from '@jupyterlab/apputils';
 import { ISpecialRef } from './diff/model';
 import { IRenderMimeRegistry } from '@jupyterlab/rendermime';
 import { isDiffSupported } from './diff/Diff';
@@ -325,15 +325,16 @@ export class FileItem extends React.Component<IFileItemProps, {}> {
       <button
         className={`jp-Git-button ${this.getDiffFileIconClass()}`}
         title={'Diff this file'}
-        onClick={() => {
-          openDiffView(
+        onClick={async () => {
+          await openDiffView(
             this.props.file.to,
             this.props.app,
             {
               previousRef: { gitRef: 'HEAD' },
               currentRef: { specialRef: currentRef.specialRef }
             },
-            this.props.renderMime
+            this.props.renderMime,
+            this.props.topRepoPath
           );
         }}
       />

src/components/FileList.tsx

@@ -129,15 +129,16 @@ export class FileList extends React.Component<IFileListProps, IFileListState> {
       commands.addCommand(CommandIDs.gitFileDiffWorking, {
         label: 'Diff',
         caption: 'Diff selected file',
-        execute: () => {
-          openDiffView(
+        execute: async () => {
+          await openDiffView(
             this.state.contextMenuFile,
             this.props.app,
             {
               currentRef: { specialRef: 'WORKING' },
               previousRef: { gitRef: 'HEAD' }
             },
-            this.props.renderMime
+            this.props.renderMime,
+            this.props.topRepoPath
           );
         }
       });
@@ -147,15 +148,16 @@ export class FileList extends React.Component<IFileListProps, IFileListState> {
       commands.addCommand(CommandIDs.gitFileDiffIndex, {
         label: 'Diff',
         caption: 'Diff selected file',
-        execute: () => {
-          openDiffView(
+        execute: async () => {
+          await openDiffView(
             this.state.contextMenuFile,
             this.props.app,
             {
               currentRef: { specialRef: 'INDEX' },
               previousRef: { gitRef: 'HEAD' }
             },
-            this.props.renderMime
+            this.props.renderMime,
+            this.props.topRepoPath
           );
         }
       });

src/components/SinglePastCommitInfo.tsx

@@ -217,15 +217,16 @@ export class SinglePastCommitInfo extends React.Component<
                     <button
                       className={`${diffIconStyle}`}
                       title={'View file changes'}
-                      onClick={() => {
-                        openDiffView(
+                      onClick={async () => {
+                        await openDiffView(
                           modifiedFile.modified_file_path,
                           this.props.app,
                           {
                             previousRef: { gitRef: this.props.data.pre_commit },
                             currentRef: { gitRef: this.props.data.commit }
                           },
-                          this.props.renderMime
+                          this.props.renderMime,
+                          this.props.topRepoPath
                         );
                       }}
                     />

src/components/diff/Diff.tsx

@@ -34,7 +34,6 @@ export interface IDiffProps {
  */
 export function Diff(props: IDiffProps) {
   const fileExtension = PathExt.extname(props.path).toLocaleLowerCase();
-
   if (fileExtension in DIFF_PROVIDER_REGISTRY) {
     const DiffProvider = DIFF_PROVIDER_REGISTRY[fileExtension];
     return <DiffProvider {...props} />;

src/components/diff/DiffWidget.tsx

@@ -7,24 +7,27 @@ import { JupyterFrontEnd } from '@jupyterlab/application';
 import { ReactWidget, showDialog } from '@jupyterlab/apputils';
 import { PathExt } from '@jupyterlab/coreutils';
 import { style } from 'typestyle';
+import { httpGitRequest } from '../../git';
 
 /**
  * Method to open a main menu panel to show the diff of a given Notebook file.
  * If one already exists, just activates the existing one.
  *
- * @param path the path relative to the Jupyter server root.
+ * @param filePath the path relative to the Git repo root.
  * @param app The JupyterLab application instance
  * @param diffContext the context in which the diff is being requested
  * @param renderMime the renderMime registry instance from the
+ * @param topRepoPath the Git repo root path.
  */
-export function openDiffView(
-  path: string,
+export async function openDiffView(
+  filePath: string,
   app: JupyterFrontEnd,
   diffContext: IDiffContext,
-  renderMime: IRenderMimeRegistry
+  renderMime: IRenderMimeRegistry,
+  topRepoPath: string
 ) {
-  if (isDiffSupported(path)) {
-    const id = `nbdiff-${path}-${getRefValue(diffContext.currentRef)}`;
+  if (isDiffSupported(filePath)) {
+    const id = `nbdiff-${filePath}-${getRefValue(diffContext.currentRef)}`;
     let mainAreaItems = app.shell.widgets('main');
     let mainAreaItem = mainAreaItems.next();
     while (mainAreaItem) {
@@ -35,13 +38,17 @@ export function openDiffView(
       mainAreaItem = mainAreaItems.next();
     }
     if (!mainAreaItem) {
+      const relativeFilePath: string = await getRelativeFilePath(
+        filePath,
+        topRepoPath
+      );
       const nbDiffWidget = ReactWidget.create(
         <RenderMimeProvider value={renderMime}>
-          <Diff path={path} diffContext={diffContext} />
+          <Diff path={relativeFilePath} diffContext={diffContext} />
         </RenderMimeProvider>
       );
       nbDiffWidget.id = id;
-      nbDiffWidget.title.label = PathExt.basename(path);
+      nbDiffWidget.title.label = PathExt.basename(filePath);
       nbDiffWidget.title.iconClass = style({
         backgroundImage: 'var(--jp-icon-diff)'
       });
@@ -55,8 +62,40 @@ export function openDiffView(
     showDialog({
       title: 'Diff Not Supported',
       body: `Diff is not supported for ${PathExt.extname(
-        path
+        filePath
       ).toLocaleLowerCase()} files.`
     });
   }
 }
+
+let serverRootResultCache = null;
+/**
+ * Gets the path of the file relative to the Jupyter server root. This resolves the server root path after calling
+ * the `/git/server_root` REST API, and uses the Git repo root and the file path relative to the repo root to resolve
+ * the rest.
+ * @param path the file path relative to Git repo root
+ * @param topRepoPath the Git repo root path
+ */
+export async function getRelativeFilePath(
+  path: string,
+  topRepoPath: string
+): Promise<string> {
+  if (serverRootResultCache !== null) {
+    return serverRootResultCache;
+  } else {
+    const response = await httpGitRequest('/git/server_root', 'GET', null);
+    const responseData = await response.json();
+    serverRootResultCache = PathExt.join(
+      PathExt.relative(responseData['server_root'], topRepoPath),
+      path
+    );
+    return serverRootResultCache;
+  }
+}
+
+/**
+ * Visible for testing.
+ */
+export function clearCache() {
+  serverRootResultCache = null;
+}

src/git.ts

@@ -132,12 +132,19 @@ export interface IIdentity {
 export function httpGitRequest(
   url: string,
   method: string,
-  request: Object
+  request: Object | null
 ): Promise<Response> {
-  let fullRequest = {
-    method: method,
-    body: JSON.stringify(request)
-  };
+  let fullRequest;
+  if (request === null) {
+    fullRequest = {
+      method: method
+    };
+  } else {
+    fullRequest = {
+      method: method,
+      body: JSON.stringify(request)
+    };
+  }
 
   let setting = ServerConnection.makeSettings();
   let fullUrl = URLExt.join(setting.baseUrl, url);

tests/test-components/DiffWidget.spec.tsx

@@ -0,0 +1,51 @@
+import 'jest';
+import { httpGitRequest } from '../../src/git';
+import { getRelativeFilePath } from '../../src/components/diff/DiffWidget';
+import { clearCache } from '../../src/components/diff/DiffWidget';
+import { createTestResponse } from './testutils';
+
+jest.mock('../../src/git');
+
+describe('DiffWidget', () => {
+  it('should return relative path correctly ', async function() {
+    const testData = [
+      [
+        'somefolder/file',
+        '/path/to/server/dir1/dir2/repo',
+        'dir1/dir2/repo/somefolder/file'
+      ],
+      ['file', '/path/to/server/repo', 'repo/file'],
+      ['somefolder/file', '/path/to/server/repo', 'repo/somefolder/file'],
+      ['somefolder/file', '/path/to/server', 'somefolder/file'],
+      ['file', '/path/to/server', 'file']
+    ];
+
+    for (const testDatum of testData) {
+      await testGetRelativeFilePath(testDatum[0], testDatum[1], testDatum[2]);
+    }
+  });
+
+  async function testGetRelativeFilePath(
+    filePath: string,
+    repoPath: string,
+    expectedResult: string
+  ): Promise<void> {
+    // Given
+    const jsonResult: Promise<any> = Promise.resolve({
+      server_root: '/path/to/server'
+    });
+
+    (httpGitRequest as jest.Mock).mockReturnValue(
+      createTestResponse(200, jsonResult)
+    );
+
+    // When
+    clearCache();
+    const result = await getRelativeFilePath(filePath, repoPath);
+
+    // Then
+    expect(result).toBe(expectedResult);
+    expect(httpGitRequest).toHaveBeenCalled();
+    expect(httpGitRequest).toBeCalledWith('/git/server_root', 'GET', null);
+  }
+});

tests/test-components/NBDiff.spec.tsx

@@ -6,6 +6,7 @@ import { CellDiff, NBDiff } from '../../src/components/diff/NbDiff';
 import * as diffResponse from '../test-components/data/diffResponse.json';
 import { httpGitRequest } from '../../src/git';
 import { NBDiffHeader } from '../../src/components/diff/NBDiffHeader';
+import { createTestResponse } from './testutils';
 
 jest.mock('../../src/git');
 
@@ -84,28 +85,3 @@ describe('NBDiff', () => {
     });
   });
 });
-
-function createTestResponse(
-  status: number,
-  json: Promise<any>
-): Promise<Response> {
-  const testResponse: Response = {
-    status: status,
-    json: () => json,
-    headers: Headers as any,
-    ok: true,
-    redirected: false,
-    statusText: 'foo',
-    trailer: Promise.resolve(Headers as any),
-    type: 'basic',
-    url: '/foo/bar',
-    clone: jest.fn<Response, []>(),
-    body: null,
-    bodyUsed: false,
-    arrayBuffer: jest.fn<Promise<ArrayBuffer>, []>(),
-    blob: jest.fn<Promise<Blob>, []>(),
-    formData: jest.fn<Promise<FormData>, []>(),
-    text: jest.fn<Promise<string>, []>()
-  };
-  return Promise.resolve(testResponse);
-}

tests/test-components/testutils.ts

@@ -0,0 +1,24 @@
+export function createTestResponse(
+  status: number,
+  json: Promise<any>
+): Promise<Response> {
+  const testResponse: Response = {
+    status: status,
+    json: () => json,
+    headers: Headers as any,
+    ok: true,
+    redirected: false,
+    statusText: 'foo',
+    trailer: Promise.resolve(Headers as any),
+    type: 'basic',
+    url: '/foo/bar',
+    clone: jest.fn<Response, []>(),
+    body: null,
+    bodyUsed: false,
+    arrayBuffer: jest.fn<Promise<ArrayBuffer>, []>(),
+    blob: jest.fn<Promise<Blob>, []>(),
+    formData: jest.fn<Promise<FormData>, []>(),
+    text: jest.fn<Promise<string>, []>()
+  };
+  return Promise.resolve(testResponse);
+}