Consolidated 'git' refish parsing into a class

gclient_scm.py

Index: gclient_scm.py
diff --git a/gclient_scm.py b/gclient_scm.py
index b183f91f8ab66412535dae41dfc362f5bfc646a9..5b1a390dae30086e8fdc3b7c420c7689f3557fc2 100644
--- a/gclient_scm.py
+++ b/gclient_scm.py
@@ -6,6 +6,7 @@
 
 from __future__ import print_function
 
+import collections
 import errno
 import logging
 import os
@@ -160,8 +161,8 @@ class SCMWrapper(object):
     return getattr(self, command)(options, args, file_list)
 
   def GetActualRemoteURL(self, options):
+    # Disable 'unused argument: options' warning | pylint: disable=W0613

[M-A Ruel, 2014/06/24 21:18:11]

Can you remove it instead?

[dnj, 2014/06/25 01:55:05]

Done.

     """Attempt to determine the remote URL for this SCMWrapper."""
-    # Git
     if os.path.exists(os.path.join(self.checkout_path, '.git')):
       actual_remote_url = shlex.split(scm.GIT.Capture(
           ['config', '--local', '--get-regexp', r'remote.*.url'],
@@ -230,6 +231,133 @@ class SCMWrapper(object):
       shutil.move(self.checkout_path, dest_path)
 
 
+_GitRefishBase = collections.namedtuple('GitRef', (
+        # The initial string that the parsed Refish came from

[M-A Ruel, 2014/06/24 21:18:11]

+4

[dnj, 2014/06/24 22:10:50]

Done.

+        'source',
+        # Is this ref a branch?
+        'is_branch',
+        # The name of this ref when accessed through the local repository
+        'local_ref',
+        # The name of the remote that this refish resides on
+        'remote',
+        # The path of this refish on the remote (e.g., 'master')
+        'remote_ref',
+        # The remote-qualified path to this refish (e.g., 'origin/master')
+        'remote_refspec',
+        # If a branch, the expected name of the upstream branch that it should
+        # track; otherwise, 'None'
+        'upstream_branch',
+        ))
+

[M-A Ruel, 2014/06/24 21:18:11]

2 lines

[dnj, 2014/06/24 22:10:50]

Done.

+class GitRefish(_GitRefishBase):
+  # Disable 'no __init__' warning | pylint: disable=W0232
+  """Class to implement refish parsing and properties.

[M-A Ruel, 2014/06/24 21:18:11]

No need to state a class is a class, so use something like:
"""Implements refish parsing and properties.

[dnj, 2014/06/24 22:10:50]

Done.

+
+  This class is designed to concentrate and codify the assumptions made by
+  'gclient' code about refs and revisions.
+  """
+
+  _DEFAULT_REMOTE = 'origin'
+
+  _GIT_SHA1_RE = re.compile('[0-9a-f]{40}', re.IGNORECASE)
+  _GIT_SHA1_SHORT_RE = re.compile('[0-9a-f]{4,40}', re.IGNORECASE)
+
+  def __str__(self):
+    return self.source
+
+  @classmethod
+  def Parse(cls, ref, remote=None, other_remotes=None):
+    """Parses a ref into a 'GitRefish' instance.

[M-A Ruel, 2014/06/24 21:18:11]

"""Parses a ref into a 'GitRefish' instance."""

[dnj, 2014/06/24 22:10:50]

Done.

+    """
+    # Use default set of remotes
+    if remote is None:
+      remote = cls._DEFAULT_REMOTE

[M-A Ruel, 2014/06/24 21:18:11]

shorthand is:
remote = remote or cls._DEFAULT_REMOTE

[dnj, 2014/06/24 22:10:50]

Done.

+    remotes = {remote}
+    if other_remotes is not None:

[M-A Ruel, 2014/06/24 21:18:11]

if other_remotes:

[dnj, 2014/06/24 22:10:50]

Done.

+      remotes.update(other_remotes)
+
+    ref = ref.strip()
+    # Treat 'ref' as a '/'-delimited set of items; analyze their contents
+    ref_split = local_ref = remote_ref = tuple(ref.split('/'))
+    is_branch = True
+    if (
+        len(ref_split) > 1 and
+        ref_split[:1] == ('refs',)):
+      if (len(ref_split) >= 3) and (ref_split[1] == 'heads'):
+        # refs/heads/foo/bar
+        #
+        # Local Ref:  foo/bar
+        # Remote Ref: foo/bar
+        local_ref = remote_ref = ref_split[2:]
+      elif (len(ref_split) >= 4) and (ref_split[1] == 'remotes'):
+        # refs/remotes/<REMOTE>/foo/bar
+        # This is a bad assumption, and this logic can be improved, but it
+        # is consistent with traditional 'gclient' logic.
+        #
+        # Local Ref:  refs/remotes/<REMOTE>/foo/bar
+        # Remote:     <REMOTE>
+        # Remote Ref: foo/bar
+        remote = ref_split[2]
+        remote_ref = ref_split[3:]
+      else:
+        # If it's not part of the standard branch set, assume it's a remote
+        # branch.
+        #
+        # refs/foo/bar
+        #
+        # Local Ref:  refs/foo/bar
+        # Remote ref: refs/foo/bar
+        pass
+    elif (len(ref_split) >= 2) and (ref_split[0] in remotes):
+      # origin/master
+      #
+      # Local Ref:  refs/remotes/origin/master
+      # Remote Ref: master
+      remote = ref_split[0]
+      remote_ref = ref_split[1:]
+      local_ref = ('refs', 'remotes') + ref_split
+    else:
+      # It could be a hash, a short-hash, or a tag
+      is_branch = False
+
+    # Determine how the ref should be referenced remotely
+    if is_branch:
+      # foo/bar/baz => origin/foo/bar/baz
+      remote_refspec = (remote,) + remote_ref
+    else:
+      # Refer to the hash/tag directly. This is actually not allowed in
+      # 'fetch', although it oftentimes works regardless.
+      #
+      # <HASH/TAG> => <HASH/TAG>
+      remote_refspec = (ref,)
+
+    # Calculate the upstream branch
+    if is_branch:
+      # Convert '/refs/heads/...' to 'refs/remotes/REMOTE/...'
+      if ref_split[:2] == ('refs', 'heads'):
+        upstream_branch = ('refs', 'remotes', remote) + ref_split[2:]
+      else:
+        upstream_branch = ref_split
+    else:
+      upstream_branch = None
+
+    def compose(ref_tuple):
+      if ref_tuple is not None:
+        ref_tuple = '/'.join(ref_tuple)
+      return ref_tuple
+
+    # Instantiate
+    return cls(
+        source=ref,
+        is_branch=is_branch,
+        local_ref=compose(local_ref),
+        remote=remote,
+        remote_ref=compose(remote_ref),
+        remote_refspec=compose(remote_refspec),
+        upstream_branch=compose(upstream_branch),
+    )
+
+
 class GitWrapper(SCMWrapper):
   """Wrapper for Git"""
   name = 'git'
@@ -259,6 +387,10 @@ class GitWrapper(SCMWrapper):
     except OSError:
       return False
 
+  def ParseRefish(self, value, **kwargs):
+    kwargs.setdefault('remote', self.remote)
+    return GitRefish.Parse(value, **kwargs)
+
   def GetCheckoutRoot(self):
     return scm.GIT.GetCheckoutRoot(self.checkout_path)
 
@@ -293,7 +425,7 @@ class GitWrapper(SCMWrapper):
         cwd=self.checkout_path,
         filter_fn=GitDiffFilterer(self.relpath).Filter, print_func=self.Print)
 
-  def _FetchAndReset(self, revision, file_list, options):
+  def _FetchAndReset(self, refish, file_list, options):
     """Equivalent to git fetch; git reset."""
     quiet = []
     if not options.verbose:
@@ -301,7 +433,7 @@ class GitWrapper(SCMWrapper):
     self._UpdateBranchHeads(options, fetch=False)
 
     self._Fetch(options, prune=True, quiet=options.verbose)
-    self._Run(['reset', '--hard', revision] + quiet, options)
+    self._Run(['reset', '--hard', refish.local_ref] + quiet, options)
     if file_list is not None:
       files = self._Capture(['ls-files']).splitlines()
       file_list.extend([os.path.join(self.checkout_path, f) for f in files])
@@ -320,7 +452,7 @@ class GitWrapper(SCMWrapper):
     self._CheckMinVersion("1.6.6")
 
     # If a dependency is not pinned, track the default remote branch.
-    default_rev = 'refs/remotes/%s/master' % self.remote
+    default_rev = 'refs/remotes/%s/master' % (self.remote,)

[M-A Ruel, 2014/06/24 21:18:11]

Not needed.

[dnj, 2014/06/24 22:10:50]

I think it looks cleaner to be explicit in formatting strings. I'll revert this
line, though, if you disagree.

[M-A Ruel, 2014/06/25 01:24:58]

Please ask around to others if anyone else ever used this format. If you can
find me someone that can vouch for this style, as I don't recall anyone else
doing this, I'll let you do use these 3 extra chars. :)

[dnj, 2014/06/25 01:55:06]

I actually picked up this habit from iannucci@, who has just confirmed that he
prefers this.

     url, deps_revision = gclient_utils.SplitUrlRevision(self.url)
     rev_str = ""
     revision = deps_revision
@@ -354,26 +486,17 @@ class GitWrapper(SCMWrapper):
       verbose = ['--verbose']
       printed_path = True
 
+    refish = self.ParseRefish(revision)
     url = self._CreateOrUpdateCache(url, options)
 
-    if revision.startswith('refs/'):
-      rev_type = "branch"
-    elif revision.startswith(self.remote + '/'):
-      # Rewrite remote refs to their local equivalents.
-      revision = 'refs/remotes/' + revision
-      rev_type = "branch"
-    else:
-      # hash is also a tag, only make a distinction at checkout
-      rev_type = "hash"
-
     if (not os.path.exists(self.checkout_path) or
         (os.path.isdir(self.checkout_path) and
          not os.path.exists(os.path.join(self.checkout_path, '.git')))):
       try:
-        self._Clone(revision, url, options)
+        self._Clone(refish.local_ref, url, options)
       except subprocess2.CalledProcessError:
         self._DeleteOrMove(options.force)
-        self._Clone(revision, url, options)
+        self._Clone(refish.local_ref, url, options)
       if deps_revision and deps_revision.startswith('branch-heads/'):
         deps_branch = deps_revision.replace('branch-heads/', '')
         self._Capture(['branch', deps_branch, deps_revision])
@@ -411,7 +534,7 @@ class GitWrapper(SCMWrapper):
       self._CheckClean(rev_str)
       # Switch over to the new upstream
       self._Run(['remote', 'set-url', self.remote, url], options)
-      self._FetchAndReset(revision, file_list, options)
+      self._FetchAndReset(refish, file_list, options)
       return_early = True
 
     if return_early:
@@ -454,7 +577,8 @@ class GitWrapper(SCMWrapper):
       else:
         raise gclient_utils.Error('Invalid Upstream: %s' % upstream_branch)
 
-    if not scm.GIT.IsValidRevision(self.checkout_path, revision, sha_only=True):
+    if not scm.GIT.IsValidRevision(self.checkout_path, refish.local_ref,
+                                   sha_only=True):
       # Update the remotes first so we have all the refs.
       remote_output = scm.GIT.Capture(['remote'] + verbose + ['update'],
               cwd=self.checkout_path)
@@ -474,10 +598,10 @@ class GitWrapper(SCMWrapper):
       # case 0
       self._CheckClean(rev_str)
       self._CheckDetachedHead(rev_str, options)
-      if self._Capture(['rev-list', '-n', '1', 'HEAD']) == revision:
+      if self._Capture(['rev-list', '-n', '1', 'HEAD']) == refish.local_ref:
         self.Print('Up-to-date; skipping checkout.')
       else:
-        self._Checkout(options, revision, quiet=True)
+        self._Checkout(options, refish.local_ref, quiet=True)
       if not printed_path:
         self.Print('_____ %s%s' % (self.relpath, rev_str), timestamp=False)
     elif current_type == 'hash':
@@ -485,8 +609,8 @@ class GitWrapper(SCMWrapper):
       if scm.GIT.IsGitSvn(self.checkout_path) and upstream_branch is not None:
         # Our git-svn branch (upstream_branch) is our upstream
         self._AttemptRebase(upstream_branch, files, options,
-                            newbase=revision, printed_path=printed_path,
-                            merge=options.merge)
+                            newbase=refish.local_ref,
+                            printed_path=printed_path, merge=options.merge)
         printed_path = True
       else:
         # Can't find a merge-base since we don't know our upstream. That makes
@@ -494,19 +618,19 @@ class GitWrapper(SCMWrapper):
         # assume origin is our upstream since that's what the old behavior was.
         upstream_branch = self.remote
         if options.revision or deps_revision:
-          upstream_branch = revision
+          upstream_branch = refish.local_ref
         self._AttemptRebase(upstream_branch, files, options,
                             printed_path=printed_path, merge=options.merge)
         printed_path = True
-    elif rev_type == 'hash':
+    elif not refish.is_branch:
       # case 2
       self._AttemptRebase(upstream_branch, files, options,
-                          newbase=revision, printed_path=printed_path,
+                          newbase=refish.local_ref, printed_path=printed_path,
                           merge=options.merge)
       printed_path = True
-    elif revision.replace('heads', 'remotes/' + self.remote) != upstream_branch:
+    elif refish.upstream_branch != upstream_branch:
       # case 4
-      new_base = revision.replace('heads', 'remotes/' + self.remote)
+      new_base = refish.upstream_branch
       if not printed_path:
         self.Print('_____ %s%s' % (self.relpath, rev_str), timestamp=False)
       switch_error = ("Switching upstream branch from %s to %s\n"
@@ -637,9 +761,8 @@ class GitWrapper(SCMWrapper):
     _, deps_revision = gclient_utils.SplitUrlRevision(self.url)
     if not deps_revision:
       deps_revision = default_rev
-    if deps_revision.startswith('refs/heads/'):
-      deps_revision = deps_revision.replace('refs/heads/', self.remote + '/')
-    deps_revision = self.GetUsableRev(deps_revision, options)
+    refish = self.ParseRefish(deps_revision)
+    deps_revision = self.GetUsableRev(refish.remote_refspec, options)
 
     if file_list is not None:
       files = self._Capture(['diff', deps_revision, '--name-only']).split()
@@ -819,14 +942,16 @@ class GitWrapper(SCMWrapper):
         self.Print('_____ removing non-empty tmp dir %s' % tmp_dir)
       gclient_utils.rmtree(tmp_dir)
     self._UpdateBranchHeads(options, fetch=True)
-    self._Checkout(options, revision.replace('refs/heads/', ''), quiet=True)
+
+    refish = self.ParseRefish(revision)
+    self._Checkout(options, refish.local_ref, quiet=True)
     if self._GetCurrentBranch() is None:
       # Squelch git's very verbose detached HEAD warning and use our own
-      self.Print(
-        ('Checked out %s to a detached HEAD. Before making any commits\n'
-         'in this repo, you should use \'git checkout <branch>\' to switch to\n'
-         'an existing branch or use \'git checkout %s -b <branch>\' to\n'
-         'create a new branch for your work.') % (revision, self.remote))
+      self.Print("""\
+Checked out %s to a detached HEAD. Before making any commits

[M-A Ruel, 2014/06/24 21:18:11]

It really breaks vim collapsing when using indent mode, so that's why I
generally recommend against doing that.

[dnj, 2014/06/24 22:10:50]

Done.

+in this repo, you should use 'git checkout <branch>' to switch to
+an existing branch or use 'git checkout %s -b <branch>' to
+create a new branch for your work.""" % (revision, self.remote))
 
   def _AskForData(self, prompt, options):
     if options.jobs > 1: