Guess default push target (#721)

* Fresh rebase of #412

* Add Python logger

* Correct api response handling for push/pull/tag

* Reverse tag order to put newer on top

* Fix Python 3.5 error

Author: fcollonval

jupyterlab_git/git.py

@@ -13,13 +13,15 @@
 import tornado.locks
 import datetime
 
+from .log import get_logger
+
 
-# Git configuration options exposed through the REST API
-ALLOWED_OPTIONS = ['user.name', 'user.email']
 # Regex pattern to capture (key, value) of Git configuration options.
 # See https://git-scm.com/docs/git-config#_syntax for git var syntax
 CONFIG_PATTERN = re.compile(r"(?:^|\n)([\w\-\.]+)\=")
 DEFAULT_REMOTE_NAME = "origin"
+# Maximum number of character of command output to print in debug log
+MAX_LOG_OUTPUT = 500  # type: int
 # How long to wait to be executed or finished your execution before timing out
 MAX_WAIT_FOR_EXECUTE_S = 20
 # Ensure on NFS or similar, that we give the .git/index.lock time to be removed
@@ -100,7 +102,7 @@ def call_subprocess(
 
     try:
         await execution_lock.acquire(timeout=datetime.timedelta(seconds=MAX_WAIT_FOR_EXECUTE_S))
-    except  tornado.util.TimeoutError:
+    except tornado.util.TimeoutError:
         return (1, "", "Unable to get the lock on the directory")
 
     try:
@@ -113,6 +115,7 @@ def call_subprocess(
 
         # If the lock still exists at this point, we will likely fail anyway, but let's try anyway
 
+        get_logger().debug("Execute {!s} in {!s}.".format(cmdline, cwd))
         if username is not None and password is not None:
             code, output, error = await call_subprocess_with_authentication(
                 cmdline,
@@ -126,6 +129,11 @@ def call_subprocess(
             code, output, error = await current_loop.run_in_executor(
                 None, call_subprocess, cmdline, cwd, env
             )
+        log_output = output[:MAX_LOG_OUTPUT] + "..." if len(output) > MAX_LOG_OUTPUT else output
+        log_error = error[:MAX_LOG_OUTPUT] + "..." if len(error) > MAX_LOG_OUTPUT else error
+        get_logger().debug("Code: {}\nOutput: {}\nError: {}".format(code, log_output, log_error))
+    except BaseException:
+        get_logger().warning("Fail to execute {!s}".format(cmdline), exc_info=True)
     finally:
         execution_lock.release()
 
@@ -158,9 +166,7 @@ async def config(self, top_repo_path, **kwargs):
 
         if len(kwargs):
             output = []
-            for k, v in filter(
-                lambda t: True if t[0] in ALLOWED_OPTIONS else False, kwargs.items()
-            ):
+            for k, v in kwargs.items():
                 cmd = ["git", "config", "--add", k, v]
                 code, out, err = await execute(cmd, cwd=top_repo_path)
                 output.append(out.strip())
@@ -182,7 +188,7 @@ async def config(self, top_repo_path, **kwargs):
             else:
                 raw = output.strip()
                 s = CONFIG_PATTERN.split(raw)
-                response["options"] = {k:v for k, v in zip(s[1::2], s[2::2]) if k in ALLOWED_OPTIONS}
+                response["options"] = {k:v for k, v in zip(s[1::2], s[2::2])}
 
         return response
 
@@ -841,15 +847,20 @@ async def pull(self, curr_fb_path, auth=None, cancel_on_conflict=False):
 
         return response
 
-    async def push(self, remote, branch, curr_fb_path, auth=None):
+    async def push(self, remote, branch, curr_fb_path, auth=None, set_upstream=False):
         """
         Execute `git push $UPSTREAM $BRANCH`. The choice of upstream and branch is up to the caller.
-        """
+        """        
+        command = ["git", "push"]
+        if set_upstream:
+            command.append("--set-upstream")
+        command.extend([remote, branch])
+
         env = os.environ.copy()
         if auth:
             env["GIT_TERMINAL_PROMPT"] = "1"
             code, _, error = await execute(
-                ["git", "push", remote, branch],
+                command,
                 username=auth["username"],
                 password=auth["password"],
                 cwd=os.path.join(self.root_dir, curr_fb_path),
@@ -858,7 +869,7 @@ async def push(self, remote, branch, curr_fb_path, auth=None):
         else:
             env["GIT_TERMINAL_PROMPT"] = "0"
             code, _, error = await execute(
-                ["git", "push", remote, branch],
+                command,
                 env=env,
                 cwd=os.path.join(self.root_dir, curr_fb_path),
             )
@@ -1125,7 +1136,7 @@ async def _is_binary(self, filename, ref, top_repo_path):
         # For binary files, `--numstat` outputs two `-` characters separated by TABs:
         return output.startswith('-\t-\t')
 
-    def remote_add(self, top_repo_path, url, name=DEFAULT_REMOTE_NAME):
+    async def remote_add(self, top_repo_path, url, name=DEFAULT_REMOTE_NAME):
         """Handle call to `git remote add` command.
 
         top_repo_path: str
@@ -1136,19 +1147,37 @@ def remote_add(self, top_repo_path, url, name=DEFAULT_REMOTE_NAME):
             Remote name; default "origin"
         """
         cmd = ["git", "remote", "add", name, url]
-        p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=top_repo_path)
-        _, my_error = p.communicate()
-        if p.returncode == 0:
-            return {
-                "code": p.returncode,
+        code, _, error = await execute(cmd, cwd=top_repo_path)
+        response = {
+                "code": code,
                 "command": " ".join(cmd)
             }
-        else:
-            return {
-                "code": p.returncode,
-                "command": " ".join(cmd),
-                "message": my_error.decode("utf-8").strip()
+        
+        if code != 0:
+            response["message"] = error
+
+        return response
+
+    async def remote_show(self, path):
+        """Handle call to `git remote show` command.
+        Args:
+            path (str): Git repository path
+        
+        Returns:
+            List[str]: Known remotes
+        """
+        command = ["git", "remote", "show"]
+        code, output, error = await execute(command, cwd=path)
+        response = {
+                "code": code,
+                "command": " ".join(command)
             }
+        if code == 0:
+            response["remotes"] = [r.strip() for r in output.splitlines()]
+        else:
+            response["message"] = error
+
+        return response
 
     async def ensure_gitignore(self, top_repo_path):
         """Handle call to ensure .gitignore file exists and the 

jupyterlab_git/handlers.py

@@ -13,6 +13,11 @@
 from ._version import __version__
 from .git import DEFAULT_REMOTE_NAME
 
+
+# Git configuration options exposed through the REST API
+ALLOWED_OPTIONS = ['user.name', 'user.email']
+
+
 class GitHandler(APIHandler):
     """
     Top-level parent class.
@@ -265,13 +270,14 @@ async def post(self):
 class GitRemoteAddHandler(GitHandler):
     """Handler for 'git remote add <name> <url>'."""
 
-    def post(self):
+    @web.authenticated
+    async def post(self):
         """POST request handler to add a remote."""
         data = self.get_json_body()
         top_repo_path = data["top_repo_path"]
         name = data.get("name", DEFAULT_REMOTE_NAME)
         url = data["url"]
-        output = self.git.remote_add(top_repo_path, url, name)
+        output = await self.git.remote_add(top_repo_path, url, name)
         if(output["code"] == 0):
             self.set_status(201)
         else:
@@ -422,6 +428,9 @@ async def post(self):
         data = self.get_json_body()
         response = await self.git.pull(data["current_path"], data.get("auth", None), data.get("cancel_on_conflict", False))
 
+        if response["code"] != 0:
+            self.set_status(500)
+
         self.finish(json.dumps(response))
 
 
@@ -436,32 +445,76 @@ async def post(self):
         """
         POST request handler,
         pushes committed files from your current branch to a remote branch
+        
+        Request body: 
+        {
+            current_path: string, # Git repository path
+            remote?: string # Remote to push to; i.e. <remote_name> or <remote_name>/<branch>
+        }
         """
         data = self.get_json_body()
         current_path = data["current_path"]
+        known_remote = data.get("remote")
 
         current_local_branch = await self.git.get_current_branch(current_path)
-        upstream = await self.git.get_upstream_branch(
+        
+        set_upstream = False
+        current_upstream_branch = await self.git.get_upstream_branch(
             current_path, current_local_branch
         )
 
-        if upstream['code'] == 0:
-            branch = ":".join(["HEAD", upstream['remote_branch']])
+        if known_remote is not None:
+            set_upstream = current_upstream_branch['code'] != 0
+
+            remote_name, _, remote_branch = known_remote.partition("/")
+            
+            current_upstream_branch = {
+                "code": 0,
+                "remote_branch": remote_branch or current_local_branch,
+                "remote_short_name": remote_name
+            }
+
+        if current_upstream_branch['code'] == 0:
+            branch = ":".join(["HEAD", current_upstream_branch['remote_branch']])
             response = await self.git.push(
-                upstream['remote_short_name'], branch, current_path, data.get("auth", None)
+                current_upstream_branch['remote_short_name'], branch, current_path, data.get("auth", None), set_upstream
             )
 
         else:
-            if ("no upstream configured for branch" in upstream['message'].lower()
-                 or 'unknown revision or path' in upstream['message'].lower()):
+            # Allow users to specify upstream through their configuration
+            # https://git-scm.com/docs/git-config#Documentation/git-config.txt-pushdefault
+            # Or use the remote defined if only one remote exists
+            config = await self.git.config(current_path)
+            config_options = config["options"]
+            list_remotes = await self.git.remote_show(current_path)
+            remotes = list_remotes.get("remotes", list())
+            push_default = config_options.get('remote.pushdefault')
+
+            default_remote = None
+            if push_default is not None and push_default in remotes:
+                default_remote = push_default
+            elif len(remotes) == 1:
+                default_remote = remotes[0]
+
+            if default_remote is not None:
+                response = await self.git.push(
+                    default_remote, 
+                    current_local_branch, 
+                    current_path, 
+                    data.get("auth", None), 
+                    set_upstream=True,
+                )
+            else:
                 response = {
                     "code": 128,
                     "message": "fatal: The current branch {} has no upstream branch.".format(
                         current_local_branch
                     ),
+                    "remotes": remotes  # Returns the list of known remotes
                 }
-            else:
-                self.set_status(500)
+
+        if response["code"] != 0:
+            self.set_status(500)
 
         self.finish(json.dumps(response))
 
@@ -504,8 +557,12 @@ async def post(self):
         """
         data = self.get_json_body()
         top_repo_path = data["path"]
-        options = data.get("options", {})
-        response = await self.git.config(top_repo_path, **options)
+        options = data.get("options", {})        
+        
+        filtered_options = {k: v for k, v in options.items() if k in ALLOWED_OPTIONS}
+        response = await self.git.config(top_repo_path, **filtered_options)
+        if "options" in response:
+            response["options"] = {k:v for k, v in response["options"].items() if k in ALLOWED_OPTIONS}
 
         if response["code"] != 0:
             self.set_status(500)
@@ -601,6 +658,9 @@ async def post(self):
         """
         current_path = self.get_json_body()["current_path"]
         result = await self.git.tags(current_path)
+
+        if result["code"] != 0:
+            self.set_status(500)
         self.finish(json.dumps(result))
 
 
@@ -618,6 +678,9 @@ async def post(self):
         current_path = data["current_path"]
         tag = data["tag_id"]
         result = await self.git.tag_checkout(current_path, tag)
+
+        if result["code"] != 0:
+            self.set_status(500)
         self.finish(json.dumps(result))
 
 

jupyterlab_git/log.py

@@ -0,0 +1,18 @@
+import logging
+
+from traitlets.config import Application
+
+
+class _ExtensionLogger:
+    _LOGGER = None  # type: Optional[logging.Logger]
+
+    @classmethod
+    def get_logger(cls) -> logging.Logger:
+        if cls._LOGGER is None:
+            app = Application.instance()
+            cls._LOGGER = logging.getLogger("{!s}.jupyterlab_git".format(app.log.name))
+        
+        return cls._LOGGER
+
+
+get_logger = _ExtensionLogger.get_logger

jupyterlab_git/tests/test_config.py

@@ -75,7 +75,7 @@ def test_git_get_config_multiline(self, mock_execute):
 
     @patch("jupyterlab_git.git.execute")
     @patch(
-        "jupyterlab_git.git.ALLOWED_OPTIONS",
+        "jupyterlab_git.handlers.ALLOWED_OPTIONS",
         ["alias.summary", "alias.topic-base-branch-name"],
     )
     def test_git_get_config_accepted_multiline(self, mock_execute):