Fixes from code review

Author: ktaletsk

jupyterlab_git/git.py

@@ -13,13 +13,12 @@
 ALLOWED_OPTIONS = ['user.name', 'user.email']
 
 
-class git_auth_input_wrapper:
+class GitAuthInputWrapper:
     """
     Helper class which is meant to replace subprocess.Popen for communicating
     with git CLI when also sending username and password for auth
     """
-    def __init__(self, command, cwd, env, username, password, *args, **kwargs):
-        super(git_auth_input_wrapper, self).__init__(*args, **kwargs)
+    def __init__(self, command, cwd, env, username, password):
         self.command = command
         self.cwd = cwd
         self.env = env
@@ -162,7 +161,7 @@ def clone(self, current_path, repo_url, auth=None):
         env = os.environ.copy()
         if (auth):
             env["GIT_TERMINAL_PROMPT"] = "1"
-            p = git_auth_input_wrapper(
+            p = GitAuthInputWrapper(
                 command='git clone {} -q'.format(unquote(repo_url)),
                 cwd=os.path.join(self.root_dir, current_path),
                 env = env,
@@ -589,36 +588,14 @@ def checkout_all(self, top_repo_path):
         )
         return my_output
 
-    def commit(self, commit_msg, top_repo_path, author_name=None, author_email=None):
+    def commit(self, commit_msg, top_repo_path):
         """
         Execute git commit <filename> command & return the result.
         """
-        command = ["git", "commit", "-m", commit_msg]
-
-        if author_name != None and author_email != None:
-            command.insert(1, '-c')
-            command.insert(2, f'user.name=\'{author_name}\'')
-            command.insert(3, '-c')
-            command.insert(4, f'user.email=\'{author_email}\'')
-
-        p = Popen(
-            command,
-            stdout=PIPE,
-            stderr=PIPE,
-            cwd=top_repo_path,
+        my_output = subprocess.check_output(
+            ["git", "commit", "-m", commit_msg], cwd=top_repo_path
         )
-        my_output, my_error = p.communicate()
-        
-        if p.returncode == 0:
-            return {
-                "code": p.returncode,
-                "message": my_output.decode("utf-8").strip("\n"),
-            }
-        else:
-            return {
-                "code": p.returncode,
-                "message": my_error.decode("utf-8").strip("\n"),
-            }
+        return my_output
 
     def pull(self, curr_fb_path, auth=None):
         """
@@ -628,7 +605,7 @@ def pull(self, curr_fb_path, auth=None):
         env = os.environ.copy()
         if (auth):
             env["GIT_TERMINAL_PROMPT"] = "1"
-            p = git_auth_input_wrapper(
+            p = GitAuthInputWrapper(
                 command = 'git pull --no-commit',
                 cwd = os.path.join(self.root_dir, curr_fb_path),
                 env = env,
@@ -661,7 +638,7 @@ def push(self, remote, branch, curr_fb_path, auth=None):
         env = os.environ.copy()
         if (auth):
             env["GIT_TERMINAL_PROMPT"] = "1"
-            p = git_auth_input_wrapper(
+            p = GitAuthInputWrapper(
                 command = 'git push {} {}'.format(remote, branch),
                 cwd = os.path.join(self.root_dir, curr_fb_path),
                 env = env,

jupyterlab_git/handlers.py

@@ -306,16 +306,6 @@ def post(self):
 class GitCommitHandler(GitHandler):
     """
     Handler for 'git commit -m <message>'. Commits files.
-    
-    When author information is sent, adds that information to commit
-
-    Input format:
-        {
-            'top_repo_path': 'top/repo/path',
-            'commit_msg': 'commit_message_to_add',
-            OPTIONAL: 'author_name' : 'Notebook User',
-            OPTIONAL: 'author_email' : 'email@domain.com'
-        }
     """
 
     def post(self):
@@ -325,12 +315,7 @@ def post(self):
         data = self.get_json_body()
         top_repo_path = data["top_repo_path"]
         commit_msg = data["commit_msg"]
-        if "author_name" in data.keys() and "author_email" in data.keys():
-            author_name = data["author_name"]
-            author_email = data["author_email"]
-            my_output = self.git.commit(commit_msg, top_repo_path, author_name, author_email)
-        else:
-            my_output = self.git.commit(commit_msg, top_repo_path)
+        my_output = self.git.commit(commit_msg, top_repo_path)
         self.finish(my_output)
 
 
@@ -361,13 +346,7 @@ def post(self):
         POST request handler, pulls files from a remote branch to your current branch.
         """
         data = self.get_json_body()
-
-        #Different request with and without auth
-        if 'auth' in data.keys():
-            auth = data['auth']
-            response = self.git.pull(data['current_path'], data['auth'])
-        else:
-            response = self.git.pull(data['current_path'])
+        response = self.git.pull(data['current_path'], data.get('auth', None))
 
         self.finish(json.dumps(response))
 
@@ -402,12 +381,7 @@ def post(self):
                 remote = upstream[0]
                 branch = ":".join(["HEAD", upstream[1]])
 
-            #Different request with and without auth
-            if 'auth' in data.keys():
-                auth = data['auth']
-                response = self.git.push(remote, branch, current_path, auth)
-            else:
-                response = self.git.push(remote, branch, current_path)
+            response = self.git.push(remote, branch, current_path, data.get('auth', None))
 
         else:
             response = {

jupyterlab_git/tests/test_clone.py

@@ -2,7 +2,7 @@
 
 from mock import patch, call, Mock
 
-from jupyterlab_git.git import Git, git_auth_input_wrapper
+from jupyterlab_git.git import Git, GitAuthInputWrapper
 
 
 @patch('subprocess.Popen')

src/components/CommitAuthorBox.tsx

@@ -10,13 +10,13 @@ import * as React from 'react';
 
 import { classes } from 'typestyle';
 
-import { Git, IGitAuth } from '../git';
+import { Git } from '../git';
 
 import { Dialog } from '@jupyterlab/apputils';
 
 import { GitPullPushDialog, Operation } from '../widgets/gitPushPull';
 
-import { GitCredentialsForm } from './CredentialsBox';
+import { GitCredentialsForm } from '../widgets/CredentialsBox';
 
 export interface IPathHeaderState {
   refresh: any;
@@ -97,7 +97,7 @@ export class PathHeader extends React.Component<
     let result = await dialog.launch();
     dialog.dispose();
     let retry = false;
-    while (result.button.label === 'Cancel') {
+    while (!result.button.accept) {
       let credentialsDialog = new Dialog({
         title: 'Git credentials required',
         body: new GitCredentialsForm(
@@ -111,19 +111,15 @@ export class PathHeader extends React.Component<
       let response = await credentialsDialog.launch();
       credentialsDialog.dispose();
 
-      if (response.button.label === 'OK') {
+      if (response.button.accept) {
         // user accepted attempt to login
-
-        let authJson = JSON.parse(decodeURIComponent(response.value));
-        // call gitApi.push again with credentials
-        let auth: IGitAuth = {
-          username: authJson.username,
-          password: authJson.password
-        };
-
         dialog = new Dialog({
           title: `Git ${operation}`,
-          body: new GitPullPushDialog(currentFileBrowserPath, operation, auth),
+          body: new GitPullPushDialog(
+            currentFileBrowserPath,
+            operation,
+            response.value
+          ),
           buttons: [Dialog.okButton({ label: 'DISMISS' })]
         });
         result = await dialog.launch();

src/components/CredentialsBox.tsx

@@ -197,17 +197,12 @@ export class Git {
   constructor() {}
 
   /** Make request for the Git Pull API. */
-  async pull(
-    path: string,
-    auth: IGitAuth = undefined
-  ): Promise<IGitPushPullResult> {
+  async pull(path: string, auth?: IGitAuth): Promise<IGitPushPullResult> {
     try {
       let obj: IGitPushPull = {
-        current_path: path
+        current_path: path,
+        auth
       };
-      if (auth) {
-        obj.auth = auth;
-      }
 
       let response = await httpGitRequest('/git/pull', 'POST', obj);
       if (response.status !== 200) {
@@ -221,19 +216,13 @@ export class Git {
   }
 
   /** Make request for the Git Push API. */
-  async push(
-    path: string,
-    auth: IGitAuth = undefined
-  ): Promise<IGitPushPullResult> {
+  async push(path: string, auth?: IGitAuth): Promise<IGitPushPullResult> {
     try {
       let obj: IGitPushPull = {
-        current_path: path
+        current_path: path,
+        auth
       };
 
-      if (auth) {
-        obj.auth = auth;
-      }
-
       let response = await httpGitRequest('/git/push', 'POST', obj);
       if (response.status !== 200) {
         const data = await response.json();
@@ -249,18 +238,15 @@ export class Git {
   async clone(
     path: string,
     url: string,
-    auth: IGitAuth = undefined
+    auth?: IGitAuth
   ): Promise<IGitCloneResult> {
     try {
       let obj: IGitClone = {
         current_path: path,
-        clone_url: url
+        clone_url: url,
+        auth
       };
 
-      if (auth) {
-        obj.auth = auth;
-      }
-
       let response = await httpGitRequest('/git/clone', 'POST', obj);
       if (response.status !== 200) {
         const data = await response.json();