git_command: refactor User-Agent settings

Convert the RepoUserAgent function into a UserAgent class.  This
makes it cleaner to hold internal state, and will make it easier
to add a separate git User-Agent, although we don't do it here.

We make the RepoSourceVersion independent of GitCommand so that
it can be called by the class (later).

Bug: https://crbug.com/gerrit/11144
Change-Id: Iab4e1f974b8733a36b243b2d03f5085a96effa19
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/239232
Reviewed-by: David Pursehouse <dpursehouse@collab.net>
Tested-by: Mike Frysinger <vapier@google.com>

git_command.py

@@ -22,6 +22,7 @@ import tempfile
 from signal import SIGTERM
 
 from error import GitError
+from git_refs import HEAD
 import platform_utils
 from repo_trace import REPO_TRACE, IsTrace, Trace
 from wrapper import Wrapper
@@ -99,21 +100,47 @@ class _GitCall(object):
 git = _GitCall()
 
 
-_user_agent = None
+def RepoSourceVersion():
+  """Return the version of the repo.git tree."""
+  ver = getattr(RepoSourceVersion, 'version', None)
 
-def RepoUserAgent():
+  # We avoid GitCommand so we don't run into circular deps -- GitCommand needs
-  """Return a User-Agent string suitable for HTTP-like services.
+  # to initialize version info we provide.
+  if ver is None:
+    env = GitCommand._GetBasicEnv()
+
+    proj = os.path.dirname(os.path.abspath(__file__))
+    env[GIT_DIR] = os.path.join(proj, '.git')
+
+    p = subprocess.Popen([GIT, 'describe', HEAD], stdout=subprocess.PIPE,
+                         env=env)
+    if p.wait() == 0:
+      ver = p.stdout.read().strip().decode('utf-8')
+      if ver.startswith('v'):
+        ver = ver[1:]
+    else:
+      ver = 'unknown'
+    setattr(RepoSourceVersion, 'version', ver)
+
+  return ver
+
+
+class UserAgent(object):
+  """Mange User-Agent settings when talking to external services
 
   We follow the style as documented here:
   https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/User-Agent
   """
-  global _user_agent
 
-  if _user_agent is None:
+  _os = None
-    py_version = sys.version_info
+  _repo_ua = None
 
+  @property
+  def os(self):
+    """The operating system name."""
+    if self._os is None:
       os_name = sys.platform
-    if os_name == 'linux2':
+      if os_name.lower().startswith('linux'):
         os_name = 'Linux'
       elif os_name == 'win32':
         os_name = 'Win32'
@@ -121,28 +148,24 @@ def RepoUserAgent():
         os_name = 'Cygwin'
       elif os_name == 'darwin':
         os_name = 'Darwin'
+      self._os = os_name
 
-    p = GitCommand(
+    return self._os
-        None, ['describe', 'HEAD'],
-        cwd=os.path.dirname(__file__),
-        capture_stdout=True)
-    if p.Wait() == 0:
-      repo_version = p.stdout
-      if repo_version and repo_version[-1] == '\n':
-        repo_version = repo_version[0:-1]
-      if repo_version and repo_version[0] == 'v':
-        repo_version = repo_version[1:]
-    else:
-      repo_version = 'unknown'
 
-    _user_agent = 'git-repo/%s (%s) git/%s Python/%d.%d.%d' % (
+  @property
-        repo_version,
+  def repo(self):
-        os_name,
+    """The UA when connecting directly from repo."""
+    if self._repo_ua is None:
+      py_version = sys.version_info
+      self._repo_ua = 'git-repo/%s (%s) git/%s Python/%d.%d.%d' % (
+          RepoSourceVersion(),
+          self.os,
           git.version_tuple().full,
           py_version.major, py_version.minor, py_version.micro)
 
-  return _user_agent
+    return self._repo_ua
 
+user_agent = UserAgent()
 
 def git_require(min_version, fail=False, msg=''):
   git_version = git.version_tuple()
@@ -171,17 +194,7 @@ class GitCommand(object):
                ssh_proxy = False,
                cwd = None,
                gitdir = None):
-    env = os.environ.copy()
+    env = self._GetBasicEnv()
-
-    for key in [REPO_TRACE,
-              GIT_DIR,
-              'GIT_ALTERNATE_OBJECT_DIRECTORIES',
-              'GIT_OBJECT_DIRECTORY',
-              'GIT_WORK_TREE',
-              'GIT_GRAFT_FILE',
-              'GIT_INDEX_FILE']:
-      if key in env:
-        del env[key]
 
     # If we are not capturing std* then need to print it.
     self.tee = {'stdout': not capture_stdout, 'stderr': not capture_stderr}
@@ -273,6 +286,23 @@ class GitCommand(object):
     self.process = p
     self.stdin = p.stdin
 
+  @staticmethod
+  def _GetBasicEnv():
+    """Return a basic env for running git under.
+
+    This is guaranteed to be side-effect free.
+    """
+    env = os.environ.copy()
+    for key in (REPO_TRACE,
+                GIT_DIR,
+                'GIT_ALTERNATE_OBJECT_DIRECTORIES',
+                'GIT_OBJECT_DIRECTORY',
+                'GIT_WORK_TREE',
+                'GIT_GRAFT_FILE',
+                'GIT_INDEX_FILE'):
+      env.pop(key, None)
+    return env
+
   def Wait(self):
     try:
       p = self.process

main.py

@@ -46,7 +46,7 @@ except ImportError:
 from color import SetDefaultColoring
 import event_log
 from repo_trace import SetTrace
-from git_command import git, GitCommand, RepoUserAgent
+from git_command import git, GitCommand, user_agent
 from git_config import init_ssh, close_ssh
 from command import InteractiveCommand
 from command import MirrorSafeCommand
@@ -297,11 +297,11 @@ def _PruneOptions(argv, opt):
 
 class _UserAgentHandler(urllib.request.BaseHandler):
   def http_request(self, req):
-    req.add_header('User-Agent', RepoUserAgent())
+    req.add_header('User-Agent', user_agent.repo)
     return req
 
   def https_request(self, req):
-    req.add_header('User-Agent', RepoUserAgent())
+    req.add_header('User-Agent', user_agent.repo)
     return req
 
 def _AddPasswordFromUserInput(handler, msg, req):

tests/test_git_command.py

@@ -50,12 +50,20 @@ class GitCallUnitTest(unittest.TestCase):
     self.assertNotEqual('', ver.full)
 
 
-class RepoUserAgentUnitTest(unittest.TestCase):
+class UserAgentUnitTest(unittest.TestCase):
-  """Tests the RepoUserAgent function."""
+  """Tests the UserAgent function."""
 
-  def test_smoke(self):
+  def test_smoke_os(self):
-    """Make sure it returns something useful."""
+    """Make sure UA OS setting returns something useful."""
-    ua = git_command.RepoUserAgent()
+    os_name = git_command.user_agent.os
+    # We can't dive too deep because of OS/tool differences, but we can check
+    # the general form.
+    m = re.match(r'^[^ ]+$', os_name)
+    self.assertIsNotNone(m)
+
+  def test_smoke_repo(self):
+    """Make sure repo UA returns something useful."""
+    ua = git_command.user_agent.repo
     # We can't dive too deep because of OS/tool differences, but we can check
     # the general form.
     m = re.match(r'^git-repo/[^ ]+ ([^ ]+) git/[^ ]+ Python/[0-9.]+', ua)