Merge pull request #469 from telamonian/fix_git_branch_edgecases

Fixes #460: better handling of branch edge cases

Author: fcollonval

jupyterlab_git/git.py

@@ -383,6 +383,7 @@ def branch_heads(self, current_path):
         """
         Execute 'git for-each-ref' command on refs/heads & return the result.
         """
+        # Format reference: https://git-scm.com/docs/git-for-each-ref#_field_names
         formats = ['refname:short', 'objectname', 'upstream:short', 'HEAD']
         cmd = ["git", "for-each-ref", "--format=" + "%09".join("%({})".format(f) for f in formats), "refs/heads/"]
         p = subprocess.Popen(
@@ -397,7 +398,6 @@ def branch_heads(self, current_path):
             results = []
             try:
                 for name,commit_sha,upstream_name,is_current_branch in (line.split('\t') for line in output.decode("utf-8").splitlines()):
-                    # Format reference : https://git-scm.com/docs/git-for-each-ref#_field_names
                     is_current_branch = bool(is_current_branch.strip())
 
                     branch = {
@@ -412,15 +412,14 @@ def branch_heads(self, current_path):
                     if is_current_branch:
                         current_branch = branch
 
-                # Remote branch is seleted use 'git branch -a' as fallback machanism
-                # to get add detached head on remote branch to preserve older functionality
-                # TODO : Revisit this to checkout new local branch with same name as remote
-                # when the remote branch is seleted, VS Code git does the same thing.
-                if not current_branch and self.get_current_branch(current_path) == "HEAD":
+                # Above can fail in certain cases, such as an empty repo with
+                # no commits. In that case, just fall back to determining
+                # current branch
+                if not current_branch:
                     branch = {
                         "is_current_branch": True,
                         "is_remote_branch": False,
-                        "name": self._get_detached_head_name(current_path),
+                        "name": self.get_current_branch(current_path),
                         "upstream": None,
                         "top_commit": None,
                         "tag": None,
@@ -447,6 +446,7 @@ def branch_remotes(self, current_path):
         """
         Execute 'git for-each-ref' command on refs/heads & return the result.
         """
+        # Format reference: https://git-scm.com/docs/git-for-each-ref#_field_names
         formats = ['refname:short', 'objectname']
         cmd = ["git", "for-each-ref", "--format=" + "%09".join("%({})".format(f) for f in formats), "refs/remotes/"]
         p = subprocess.Popen(
@@ -460,7 +460,6 @@ def branch_remotes(self, current_path):
             results = []
             try:
                 for name,commit_sha in (line.split('\t') for line in output.decode("utf-8").splitlines()):
-                    # Format reference : https://git-scm.com/docs/git-for-each-ref#_field_names
                     results.append({
                         "is_current_branch": False,
                         "is_remote_branch": True,
@@ -771,18 +770,6 @@ def init(self, current_path):
         )
         return my_output
 
-    def _is_branch(self, reference_name):
-        """Check if the given reference is a branch
-        """
-        return reference_name.startswith("refs/heads/") or reference_name.startswith(
-            "refs/remotes/"
-        )
-
-    def _is_current_branch(self, branch_name, current_branch_name):
-        """Check if given branch is current branch
-        """
-        return branch_name == current_branch_name
-
     def _is_remote_branch(self, branch_reference):
         """Check if given branch is remote branch by comparing with 'remotes/',
         TODO : Consider a better way to check remote branch
@@ -800,10 +787,12 @@ def _get_branch_name(self, branch_reference):
         raise ValueError("Reference [{}] is not a valid branch.", branch_reference)
 
     def get_current_branch(self, current_path):
-        """Execute 'git rev-parse --abbrev-ref HEAD' to
-        check if given branch is current branch
+        """Use `symbolic-ref` to get the current branch name. In case of
+        failure, assume that the HEAD is currently detached, and fall back
+        to the `branch` command to get the name.
+        See https://git-blame.blogspot.com/2013/06/checking-current-branch-programatically.html
         """
-        command = ["git", "rev-parse", "--abbrev-ref", "HEAD"]
+        command = ["git", "symbolic-ref", "HEAD"]
         p = subprocess.Popen(
             command,
             stdout=PIPE,
@@ -812,15 +801,17 @@ def get_current_branch(self, current_path):
         )
         output, error = p.communicate()
         if p.returncode == 0:
-            return output.decode("utf-8").strip()
+            return output.decode("utf-8").split('/')[-1].strip()
+        elif "not a symbolic ref" in error.decode("utf-8").lower():
+            return self._get_current_branch_detached(current_path)
         else:
             raise Exception(
                 "Error [{}] occurred while executing [{}] command to get current branch.".format(
                     error.decode("utf-8"), " ".join(command)
                 )
             )
 
-    def _get_detached_head_name(self, current_path):
+    def _get_current_branch_detached(self, current_path):
         """Execute 'git branch -a' to get current branch details in case of detached HEAD
         """
         command = ["git", "branch", "-a"]
@@ -901,4 +892,3 @@ def _get_tag(self, current_path, commit_sha):
                     error.decode("utf-8"), " ".join(command)
                 )
             )
-

jupyterlab_git/tests/test_branch.py

@@ -7,34 +7,6 @@
 from jupyterlab_git.git import Git
 
 
-def test_is_branch():
-    test_cases = [
-        ('refs/heads/feature-foo', True),
-        ('refs/heads/master', True),
-        ('refs/remotes/origin/feature-foo', True),
-        ('refs/remotes/origin/HEAD', True),
-        ('refs/stash', False),
-        ('refs/tags/v0.1.0', False),
-        ('refs/tags/[email protected]', False)
-    ]
-    for test_case in test_cases:
-        actual_response = Git(root_dir='/bin')._is_branch(test_case[0])
-        assert test_case[1] == actual_response
-
-
-def test_is_current_branch():
-    current_branch = 'feature-foo'
-    test_cases = [
-        ('feature-foo', True),
-        ('master', False),
-        ('origin/feature-foo', False),
-        ('origin/HEAD', False)
-    ]
-    for test_case in test_cases:
-        actual_response = Git(root_dir='/bin')._is_current_branch(test_case[0], current_branch)
-        assert test_case[1] == actual_response
-
-
 def test_is_remote_branch():
     test_cases = [
         ('refs/heads/feature-foo', False),
@@ -89,7 +61,7 @@ def test_get_current_branch_success(mock_subproc_popen):
     # Then
     mock_subproc_popen.assert_has_calls([
         call(
-            ['git', 'rev-parse', '--abbrev-ref', 'HEAD'],
+            ['git', 'symbolic-ref', 'HEAD'],
             stdout=PIPE,
             stderr=PIPE,
             cwd='/bin/test_curr_path'
@@ -366,20 +338,20 @@ def test_get_current_branch_failure(mock_subproc_popen):
     # Then
     mock_subproc_popen.assert_has_calls([
         call(
-            ['git', 'rev-parse', '--abbrev-ref', 'HEAD'],
+            ['git', 'symbolic-ref', 'HEAD'],
             stdout=PIPE,
             stderr=PIPE,
             cwd='/bin/test_curr_path'
         ),
         call().communicate()
     ])
     assert 'Error [fatal: Not a git repository (or any of the parent directories): .git] ' \
-           'occurred while executing [git rev-parse --abbrev-ref HEAD] command to get current branch.' == str(
+           'occurred while executing [git symbolic-ref HEAD] command to get current branch.' == str(
         error.value)
 
 
 @patch('subprocess.Popen')
-def test_get_detached_head_name_success(mock_subproc_popen):
+def test_get_current_branch_detached_success(mock_subproc_popen):
     # Given
     process_output = [
         '* (HEAD detached at origin/feature-foo)',
@@ -396,7 +368,7 @@ def test_get_detached_head_name_success(mock_subproc_popen):
     mock_subproc_popen.return_value = process_mock
 
     # When
-    actual_response = Git(root_dir='/bin')._get_detached_head_name(
+    actual_response = Git(root_dir='/bin')._get_current_branch_detached(
         current_path='test_curr_path')
 
     # Then
@@ -413,7 +385,7 @@ def test_get_detached_head_name_success(mock_subproc_popen):
 
 
 @patch('subprocess.Popen')
-def test_get_detached_head_name_failure(mock_subproc_popen):
+def test_get_current_branch_detached_failure(mock_subproc_popen):
     # Given
     process_mock = Mock()
     attrs = {
@@ -426,7 +398,7 @@ def test_get_detached_head_name_failure(mock_subproc_popen):
 
     # When
     with pytest.raises(Exception) as error:
-        Git(root_dir='/bin')._get_detached_head_name(current_path='test_curr_path')
+        Git(root_dir='/bin')._get_current_branch_detached(current_path='test_curr_path')
 
     # Then
     mock_subproc_popen.assert_has_calls([
@@ -798,19 +770,23 @@ def test_branch_success_detached_head(mock_subproc_popen):
         '  master',
         '  remotes/origin/feature-foo'
     ]
-    process_mock = Mock(returncode=0)
-    process_mock.communicate.side_effect = [
+
+    process_mock = Mock()
+    com_returncodes = [0, 128, 0, 0]
+    com_returns = [
         # Response for get all refs/heads
         ('\n'.join(process_output_heads).encode('utf-8'), ''.encode('utf-8')),
-
         # Response for get current branch
-        ('HEAD'.encode('utf-8'), ''.encode('utf-8')),
-        # Responses for detached head name
+        (''.encode('utf-8'), 'fatal: ref HEAD is not a symbolic ref'.encode('utf-8')),
+        # Response for get current branch detached
         ('\n'.join(detached_head_output).encode('utf-8'), ''.encode('utf-8')),
-
         # Response for get all refs/remotes
         ('\n'.join(process_output_remotes).encode('utf-8'), ''.encode('utf-8')),
     ]
+    def com_mock_side_effect():
+        process_mock.returncode = com_returncodes.pop(0)
+        return com_returns.pop(0)
+    process_mock.communicate.side_effect = com_mock_side_effect
     mock_subproc_popen.return_value = process_mock
 
     expected_response = {
@@ -868,13 +844,14 @@ def test_branch_success_detached_head(mock_subproc_popen):
 
         # call to get current branch
         call(
-            ['git', 'rev-parse', '--abbrev-ref', 'HEAD'],
+            ['git', 'symbolic-ref', 'HEAD'],
             stdout=PIPE,
             stderr=PIPE,
             cwd='/bin/test_curr_path'
         ),
         call().communicate(),
-        # call to get detached head name
+
+        # call to get current branch name given a detached head
         call(
             ['git', 'branch', '-a'],
             stdout=PIPE,