Merge pull request #691 from ianhi/slash-in-branch

FIx pushing when there are slashes in branch names or in remote name

Author: fcollonval

jupyterlab_git/git.py

@@ -892,12 +892,12 @@ async def get_current_branch(self, current_path):
         to the `branch` command to get the name.
         See https://git-blame.blogspot.com/2013/06/checking-current-branch-programatically.html
         """
-        command = ["git", "symbolic-ref", "HEAD"]
+        command = ["git", "symbolic-ref", "--short", "HEAD"]
         code, output, error = await execute(
             command, cwd=os.path.join(self.root_dir, current_path)
         )
         if code == 0:
-            return output.split("/")[-1].strip()
+            return output.strip()
         elif "not a symbolic ref" in error.lower():
             current_branch = await self._get_current_branch_detached(current_path)
             return current_branch
@@ -941,18 +941,22 @@ async def get_upstream_branch(self, current_path, branch_name):
         code, output, error = await execute(
             command, cwd=os.path.join(self.root_dir, current_path)
         )
-        if code == 0:
-            return output.strip()
-        elif "fatal: no upstream configured for branch" in error.lower():
-            return None
-        elif "unknown revision or path not in the working tree" in error.lower():
-            return None
-        else:
-            raise Exception(
-                "Error [{}] occurred while executing [{}] command to get upstream branch.".format(
-                    error, " ".join(command)
-                )
-            )
+        if code != 0:
+            return {"code": code, "command": " ".join(command), "message": error}
+        rev_parse_output = output.strip()
+
+        command = ["git", "config", "--local", "branch.{}.remote".format(branch_name)]
+        code, output, error = await execute(
+            command, cwd=os.path.join(self.root_dir, current_path)
+        )
+        if code != 0:
+            return {"code": code, "command": " ".join(cmd), "message": error}
+
+        remote_name = output.strip()
+        remote_branch = rev_parse_output.strip().lstrip(remote_name+"/")
+        return {"code": code, "remote_short_name": remote_name, "remote_branch": remote_branch}
+
+        
 
     async def _get_tag(self, current_path, commit_sha):
         """Execute 'git describe commit_sha' to get

jupyterlab_git/handlers.py

@@ -403,8 +403,10 @@ async def post(self):
         """
         current_path = self.get_json_body()["current_path"]
         current_branch = await self.git.get_current_branch(current_path)
-        upstream = await self.git.get_upstream_branch(current_path, current_branch)
-        self.finish(json.dumps({"upstream": upstream}))
+        response = await self.git.get_upstream_branch(current_path, current_branch)
+        if response['code'] != 0:
+            self.set_status(500)
+        self.finish(json.dumps(response))
 
 
 class GitPullHandler(GitHandler):
@@ -439,32 +441,28 @@ async def post(self):
         current_path = data["current_path"]
 
         current_local_branch = await self.git.get_current_branch(current_path)
-        current_upstream_branch = await self.git.get_upstream_branch(
+        upstream = await self.git.get_upstream_branch(
             current_path, current_local_branch
         )
 
-        if current_upstream_branch and current_upstream_branch.strip():
-            upstream = current_upstream_branch.split("/")
-            if len(upstream) == 1:
-                # If upstream is a local branch
-                remote = "."
-                branch = ":".join(["HEAD", upstream[0]])
-            else:
-                # If upstream is a remote branch
-                remote = upstream[0]
-                branch = ":".join(["HEAD", upstream[1]])
-
+        if upstream['code'] == 0:
+            branch = ":".join(["HEAD", upstream['remote_branch']])
             response = await self.git.push(
-                remote, branch, current_path, data.get("auth", None)
+                upstream['remote_short_name'], branch, current_path, data.get("auth", None)
             )
 
         else:
-            response = {
-                "code": 128,
-                "message": "fatal: The current branch {} has no upstream branch.".format(
-                    current_local_branch
-                ),
-            }
+            if ("no upstream configured for branch" in upstream['message'].lower()
+                 or 'unknown revision or path' in upstream['message'].lower()):
+                response = {
+                    "code": 128,
+                    "message": "fatal: The current branch {} has no upstream branch.".format(
+                        current_local_branch
+                    ),
+                }
+            else:
+                self.set_status(500)
+
         self.finish(json.dumps(response))
 
 

jupyterlab_git/tests/test_branch.py

@@ -44,7 +44,7 @@ async def test_get_current_branch_success():
 
         # Then
         mock_execute.assert_called_once_with(
-            ["git", "symbolic-ref", "HEAD"], cwd=os.path.join("/bin", "test_curr_path")
+            ["git", "symbolic-ref", "--short", "HEAD"], cwd=os.path.join("/bin", "test_curr_path")
         )
         assert "feature-foo" == actual_response
 
@@ -337,11 +337,11 @@ async def test_get_current_branch_failure():
 
         # Then
         mock_execute.assert_called_once_with(
-            ["git", "symbolic-ref", "HEAD"], cwd=os.path.join("/bin", "test_curr_path")
+            ["git", "symbolic-ref", "--short", "HEAD"], cwd=os.path.join("/bin", "test_curr_path")
         )
         assert (
             "Error [fatal: Not a git repository (or any of the parent directories): .git] "
-            "occurred while executing [git symbolic-ref HEAD] command to get current branch."
+            "occurred while executing [git symbolic-ref --short HEAD] command to get current branch."
             == str(error.value)
         )
 
@@ -403,29 +403,42 @@ async def test_get_current_branch_detached_failure():
 
 @pytest.mark.asyncio
 @pytest.mark.parametrize(
-    "branch,upstream",
+    "branch,upstream,remotename",
     [
-        ("feature-foo", "origin/master"),
-        ("master", "origin/master"),
-        ("feature-bar", "feature-foo"),
+        ("feature-foo", "master", "origin/withslash"),
+        ("master", "master", "origin"),
+        ("feature/bar", "feature-foo", ""),
     ],
 )
-async def test_get_upstream_branch_success(branch, upstream):
+async def test_get_upstream_branch_success(branch, upstream, remotename):
     with patch("jupyterlab_git.git.execute") as mock_execute:
         # Given
-        mock_execute.return_value = maybe_future((0, upstream, ""))
+        mock_execute.side_effect = [
+            maybe_future((0, remotename + '/' + upstream, '')), 
+            maybe_future((0, remotename, ''))
+            ]
 
         # When
         actual_response = await Git(FakeContentManager("/bin")).get_upstream_branch(
             current_path="test_curr_path", branch_name=branch
         )
 
         # Then
-        mock_execute.assert_called_once_with(
-            ["git", "rev-parse", "--abbrev-ref", "{}@{{upstream}}".format(branch)],
-            cwd=os.path.join("/bin", "test_curr_path"),
+        mock_execute.assert_has_calls(
+            [
+                call(
+                    ["git", "rev-parse", "--abbrev-ref", "{}@{{upstream}}".format(branch)],
+                    cwd=os.path.join("/bin", "test_curr_path"),
+                ),
+                call(
+                    ['git', 'config', '--local', 'branch.{}.remote'.format(branch)],
+                     cwd='/bin/test_curr_path',
+                ),
+
+            ],
+            any_order=False,
         )
-        assert upstream == actual_response
+        assert {'code': 0, 'remote_branch': upstream, 'remote_short_name': remotename} == actual_response
 
 
 @pytest.mark.asyncio
@@ -454,17 +467,12 @@ async def test_get_upstream_branch_failure(outputs, message):
         mock_execute.return_value = maybe_future(outputs)
 
         # When
-        if message:
-            with pytest.raises(Exception) as error:
-                await Git(FakeContentManager("/bin")).get_upstream_branch(
-                    current_path="test_curr_path", branch_name="blah"
-                )
-            assert message == str(error.value)
-        else:
-            response = await Git(FakeContentManager("/bin")).get_upstream_branch(
-                current_path="test_curr_path", branch_name="blah"
-            )
-            assert response is None
+        response = await Git(FakeContentManager("/bin")).get_upstream_branch(
+            current_path="test_curr_path", branch_name="blah"
+        )
+        expected = {'code': 128, 'command': 'git rev-parse --abbrev-ref blah@{upstream}', 'message': outputs[2]}
+
+        assert response == expected
 
         # Then
         mock_execute.assert_has_calls(
@@ -807,7 +815,7 @@ async def test_branch_success_detached_head():
                 ),
                 # call to get current branch
                 call(
-                    ["git", "symbolic-ref", "HEAD"],
+                    ["git", "symbolic-ref", "--short", "HEAD"],
                     cwd=os.path.join("/bin", "test_curr_path"),
                 ),
                 # call to get current branch name given a detached head

jupyterlab_git/tests/test_handlers.py

@@ -166,9 +166,13 @@ class TestPush(ServerTest):
     @patch("jupyterlab_git.handlers.GitPushHandler.git", spec=Git)
     def test_push_handler_localbranch(self, mock_git):
         # Given
-        mock_git.get_current_branch.return_value = maybe_future("foo")
+        mock_git.get_current_branch.return_value = maybe_future("localbranch")
         mock_git.get_upstream_branch.return_value = maybe_future(
-            "localbranch"
+            {
+                "code": 0,
+                "remote_short_name": ".",
+                "remote_branch": "localbranch"
+            }
         )
         mock_git.push.return_value = maybe_future({"code": 0})
 
@@ -178,7 +182,7 @@ def test_push_handler_localbranch(self, mock_git):
 
         # Then
         mock_git.get_current_branch.assert_called_with("test_path")
-        mock_git.get_upstream_branch.assert_called_with("test_path", "foo")
+        mock_git.get_upstream_branch.assert_called_with("test_path", "localbranch")
         mock_git.push.assert_called_with(".", "HEAD:localbranch", "test_path", None)
 
         assert response.status_code == 200
@@ -188,10 +192,11 @@ def test_push_handler_localbranch(self, mock_git):
     @patch("jupyterlab_git.handlers.GitPushHandler.git", spec=Git)
     def test_push_handler_remotebranch(self, mock_git):
         # Given
-        mock_git.get_current_branch.return_value = maybe_future("foo")
-        mock_git.get_upstream_branch.return_value = maybe_future(
-            "origin/remotebranch"
-        )
+        mock_git.get_current_branch.return_value = maybe_future("foo/bar")
+        upstream = {"code": 0,
+                    "remote_short_name": "origin/something",
+                    "remote_branch": "remote-branch-name"}
+        mock_git.get_upstream_branch.return_value = maybe_future(upstream)
         mock_git.push.return_value = maybe_future({"code": 0})
 
         # When
@@ -200,9 +205,9 @@ def test_push_handler_remotebranch(self, mock_git):
 
         # Then
         mock_git.get_current_branch.assert_called_with("test_path")
-        mock_git.get_upstream_branch.assert_called_with("test_path", "foo")
+        mock_git.get_upstream_branch.assert_called_with("test_path", "foo/bar")
         mock_git.push.assert_called_with(
-            "origin", "HEAD:remotebranch", "test_path", None
+            "origin/something", "HEAD:remote-branch-name", "test_path", None
         )
 
         assert response.status_code == 200
@@ -213,7 +218,12 @@ def test_push_handler_remotebranch(self, mock_git):
     def test_push_handler_noupstream(self, mock_git):
         # Given
         mock_git.get_current_branch.return_value = maybe_future("foo")
-        mock_git.get_upstream_branch.return_value = maybe_future("")
+        upstream = {
+            "code": 128,
+            "command": "",
+            "message": "fatal: no upstream configured for branch 'foo'"
+        }
+        mock_git.get_upstream_branch.return_value = maybe_future(upstream)
         mock_git.push.return_value = maybe_future({"code": 0})
 
         # When
@@ -234,23 +244,47 @@ def test_push_handler_noupstream(self, mock_git):
 
 
 class TestUpstream(ServerTest):
+    @patch("jupyterlab_git.handlers.GitUpstreamHandler.git", spec=Git)
+    def test_upstream_handler_forward_slashes(self, mock_git):
+        # Given
+        mock_git.get_current_branch.return_value = maybe_future("foo/bar")
+        upstream = {"code": 0,
+                    "remote_short_name": "origin/something",
+                    "remote_branch": "foo/bar"}
+        mock_git.get_upstream_branch.return_value = maybe_future(upstream)
+
+        # When
+        body = {"current_path": "test_path"}
+        response = self.tester.post(["upstream"], body=body)
+
+        # Then
+        mock_git.get_current_branch.assert_called_with("test_path")
+        mock_git.get_upstream_branch.assert_called_with("test_path", "foo/bar")
+
+        assert response.status_code == 200
+        payload = response.json()
+        assert payload == upstream 
+
     @patch("jupyterlab_git.handlers.GitUpstreamHandler.git", spec=Git)
     def test_upstream_handler_localbranch(self, mock_git):
         # Given
-        mock_git.get_current_branch.return_value = maybe_future("foo")
-        mock_git.get_upstream_branch.return_value = maybe_future("bar")
+        mock_git.get_current_branch.return_value = maybe_future("foo/bar")
+        upstream = {"code": 0,
+                    "remote_short_name": ".",
+                    "remote_branch": "foo/bar"}
+        mock_git.get_upstream_branch.return_value = maybe_future(upstream)
 
         # When
         body = {"current_path": "test_path"}
         response = self.tester.post(["upstream"], body=body)
 
         # Then
         mock_git.get_current_branch.assert_called_with("test_path")
-        mock_git.get_upstream_branch.assert_called_with("test_path", "foo")
+        mock_git.get_upstream_branch.assert_called_with("test_path", "foo/bar")
 
         assert response.status_code == 200
         payload = response.json()
-        assert payload == {"upstream": "bar"}
+        assert payload == upstream 
 
 
 class TestDiffContent(ServerTest):