project.py: Remove extraneous ','

Bug: https://crbug.com/1061473
Change-Id: I0f02f122d6313679c1ae5ad6fb4e05f68b764186
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/263112
Tested-by: George Engelbrecht <engeg@google.com>
Reviewed-by: George Engelbrecht <engeg@google.com>
Reviewed-by: SPA SARC <spanc.sarc@gmail.com>
Reviewed-by: Mike Frysinger <vapier@google.com>

git_command.py

@@ -16,6 +16,7 @@
 
 from __future__ import print_function
 import os
+import re
 import sys
 import subprocess
 import tempfile
@@ -47,6 +48,35 @@ LAST_CWD = None
 _ssh_proxy_path = None
 _ssh_sock_path = None
 _ssh_clients = []
+_ssh_version = None
+
+
+def _run_ssh_version():
+  """run ssh -V to display the version number"""
+  return subprocess.check_output(['ssh', '-V'], stderr=subprocess.STDOUT).decode()
+
+
+def _parse_ssh_version(ver_str=None):
+  """parse a ssh version string into a tuple"""
+  if ver_str is None:
+    ver_str = _run_ssh_version()
+  m = re.match(r'^OpenSSH_([0-9.]+)(p[0-9]+)?\s', ver_str)
+  if m:
+    return tuple(int(x) for x in m.group(1).split('.'))
+  else:
+    return ()
+
+
+def ssh_version():
+  """return ssh version as a tuple"""
+  global _ssh_version
+  if _ssh_version is None:
+    try:
+      _ssh_version = _parse_ssh_version()
+    except subprocess.CalledProcessError:
+      print('fatal: unable to detect ssh version', file=sys.stderr)
+      sys.exit(1)
+  return _ssh_version
 
 
 def ssh_sock(create=True):
@@ -57,9 +87,13 @@ def ssh_sock(create=True):
     tmp_dir = '/tmp'
     if not os.path.exists(tmp_dir):
       tmp_dir = tempfile.gettempdir()
+    if ssh_version() < (6, 7):
+      tokens = '%r@%h:%p'
+    else:
+      tokens = '%C'  # hash of %l%h%p%r
     _ssh_sock_path = os.path.join(
         tempfile.mkdtemp('', 'ssh-', tmp_dir),
-        'master-%r@%h:%p')
+        'master-' + tokens)
   return _ssh_sock_path
 
 

project.py

@@ -55,6 +55,12 @@ else:
   input = raw_input  # noqa: F821
 
 
+# Maximum sleep time allowed during retries.
+MAXIMUM_RETRY_SLEEP_SEC = 3600.0
+# +-10% random jitter is added to each Fetches retry sleep duration.
+RETRY_JITTER_PERCENT = 0.1
+
+
 def _lwrite(path, content):
   lock = '%s.lock' % path
 
@@ -875,6 +881,7 @@ class Project(object):
                is_derived=False,
                dest_branch=None,
                optimized_fetch=False,
+               retry_fetches=0,
                old_revision=None):
     """Init a Project object.
 
@@ -901,6 +908,8 @@ class Project(object):
       dest_branch: The branch to which to push changes for review by default.
       optimized_fetch: If True, when a project is set to a sha1 revision, only
                        fetch from the remote if the sha1 is not present locally.
+      retry_fetches: Retry remote fetches n times upon receiving transient error
+                     with exponential backoff and jitter.
       old_revision: saved git commit id for open GITC projects.
     """
     self.manifest = manifest
@@ -936,6 +945,7 @@ class Project(object):
     self.use_git_worktrees = use_git_worktrees
     self.is_derived = is_derived
     self.optimized_fetch = optimized_fetch
+    self.retry_fetches = max(0, retry_fetches)
     self.subprojects = []
 
     self.snapshots = {}
@@ -1449,6 +1459,7 @@ class Project(object):
                        tags=True,
                        archive=False,
                        optimized_fetch=False,
+                       retry_fetches=0,
                        prune=False,
                        submodules=False,
                        clone_filter=None):
@@ -1532,7 +1543,7 @@ class Project(object):
               current_branch_only=current_branch_only,
               tags=tags, prune=prune, depth=depth,
               submodules=submodules, force_sync=force_sync,
-              clone_filter=clone_filter):
+              clone_filter=clone_filter, retry_fetches=retry_fetches):
         return False
 
     mp = self.manifest.manifestProject
@@ -2334,8 +2345,10 @@ class Project(object):
                    depth=None,
                    submodules=False,
                    force_sync=False,
-                   clone_filter=None):
-
+                   clone_filter=None,
+                   retry_fetches=2,
+                   retry_sleep_initial_sec=4.0,
+                   retry_exp_factor=2.0):
     is_sha1 = False
     tag_name = None
     # The depth should not be used when fetching to a mirror because
@@ -2497,18 +2510,37 @@ class Project(object):
 
     cmd.extend(spec)
 
-    ok = False
-    for _i in range(2):
+    # At least one retry minimum due to git remote prune.
+    retry_fetches = max(retry_fetches, 2)
+    retry_cur_sleep = retry_sleep_initial_sec
+    ok = prune_tried = False
+    for try_n in range(retry_fetches):
       gitcmd = GitCommand(self, cmd, bare=True, ssh_proxy=ssh_proxy,
                           merge_output=True, capture_stdout=quiet)
       ret = gitcmd.Wait()
       if ret == 0:
         ok = True
         break
-      # If needed, run the 'git remote prune' the first time through the loop
-      elif (not _i and
-            "error:" in gitcmd.stderr and
-            "git remote prune" in gitcmd.stderr):
+
+      # Retry later due to HTTP 429 Too Many Requests.
+      elif ('error:' in gitcmd.stderr and
+            'HTTP 429' in gitcmd.stderr):
+        if not quiet:
+          print('429 received, sleeping: %s sec' % retry_cur_sleep,
+                file=sys.stderr)
+        time.sleep(retry_cur_sleep)
+        retry_cur_sleep = min(retry_exp_factor * retry_cur_sleep,
+                              MAXIMUM_RETRY_SLEEP_SEC)
+        retry_cur_sleep *= (1 - random.uniform(-RETRY_JITTER_PERCENT,
+                                               RETRY_JITTER_PERCENT))
+        continue
+
+      # If this is not last attempt, try 'git remote prune'.
+      elif (try_n < retry_fetches - 1 and
+            'error:' in gitcmd.stderr and
+            'git remote prune' in gitcmd.stderr and
+            not prune_tried):
+        prune_tried = True
         prunecmd = GitCommand(self, ['remote', 'prune', name], bare=True,
                               ssh_proxy=ssh_proxy)
         ret = prunecmd.Wait()
@@ -2644,7 +2676,9 @@ class Project(object):
         # returned another error with the HTTP error code being 400 or above.
         # This return code only appears if -f, --fail is used.
         if verbose:
-          print('Server does not provide clone.bundle; ignoring.')
+          print('%s: Unable to retrieve clone.bundle; ignoring.' % self.name)
+          if output:
+            print('Curl output:\n%s' % output)
         return False
       elif curlret and not verbose and output:
         print('%s' % output, file=sys.stderr)

subcmds/forall.py

@@ -370,8 +370,8 @@ def DoWork(project, mirror, opt, cmd, shell, cnt, config):
       for s in in_ready:
         buf = s.read().decode()
         if not buf:
-          s.close()
           s_in.remove(s)
+          s.close()
           continue
 
         if not opt.verbose:

subcmds/sync.py

@@ -265,6 +265,9 @@ later is required to fix a server side protocol bug.
     p.add_option('--optimized-fetch',
                  dest='optimized_fetch', action='store_true',
                  help='only fetch projects fixed to sha1 if revision does not exist locally')
+    p.add_option('--retry-fetches',
+                 default=0, action='store', type='int',
+                 help='number of times to retry fetches on transient errors')
     p.add_option('--prune', dest='prune', action='store_true',
                  help='delete refs that no longer exist on the remote')
     if show_smart:
@@ -342,6 +345,7 @@ later is required to fix a server side protocol bug.
             clone_bundle=opt.clone_bundle,
             tags=opt.tags, archive=self.manifest.IsArchive,
             optimized_fetch=opt.optimized_fetch,
+            retry_fetches=opt.retry_fetches,
             prune=opt.prune,
             clone_filter=clone_filter)
         self._fetch_times.Set(project, time.time() - start)
@@ -777,6 +781,7 @@ later is required to fix a server side protocol bug.
                                     current_branch_only=opt.current_branch_only,
                                     tags=opt.tags,
                                     optimized_fetch=opt.optimized_fetch,
+                                    retry_fetches=opt.retry_fetches,
                                     submodules=self.manifest.HasSubmodules,
                                     clone_filter=self.manifest.CloneFilter)
       finish = time.time()

subcmds/version.py

@@ -43,6 +43,7 @@ class Version(Command, MirrorSafeCommand):
     rp_ver = rp.bare_git.describe(HEAD)
     print('repo version %s' % rp_ver)
     print('       (from %s)' % rem.url)
+    print('       (%s)' % rp.bare_git.log('-1', '--format=%cD', HEAD))
 
     if self.wrapper_path is not None:
       print('repo launcher version %s' % self.wrapper_version)

tests/test_git_command.py

@@ -30,6 +30,33 @@ import git_command
 import wrapper
 
 
+class SSHUnitTest(unittest.TestCase):
+  """Tests the ssh functions."""
+
+  def test_ssh_version(self):
+    """Check ssh_version() handling."""
+    ver = git_command._parse_ssh_version('Unknown\n')
+    self.assertEqual(ver, ())
+    ver = git_command._parse_ssh_version('OpenSSH_1.0\n')
+    self.assertEqual(ver, (1, 0))
+    ver = git_command._parse_ssh_version('OpenSSH_6.6.1p1 Ubuntu-2ubuntu2.13, OpenSSL 1.0.1f 6 Jan 2014\n')
+    self.assertEqual(ver, (6, 6, 1))
+    ver = git_command._parse_ssh_version('OpenSSH_7.6p1 Ubuntu-4ubuntu0.3, OpenSSL 1.0.2n  7 Dec 2017\n')
+    self.assertEqual(ver, (7, 6))
+
+  def test_ssh_sock(self):
+    """Check ssh_sock() function."""
+    with mock.patch('tempfile.mkdtemp', return_value='/tmp/foo'):
+      # old ssh version uses port
+      with mock.patch('git_command.ssh_version', return_value=(6, 6)):
+        self.assertTrue(git_command.ssh_sock().endswith('%p'))
+      git_command._ssh_sock_path = None
+      # new ssh version uses hash
+      with mock.patch('git_command.ssh_version', return_value=(6, 7)):
+        self.assertTrue(git_command.ssh_sock().endswith('%C'))
+      git_command._ssh_sock_path = None
+
+
 class GitCallUnitTest(unittest.TestCase):
   """Tests the _GitCall class (via git_command.git)."""
 

tests/test_wrapper.py

@@ -304,6 +304,7 @@ class SetupGnuPG(RepoWrapperTestCase):
     """Make sure it works completely."""
     with TemporaryDirectory() as tempdir:
       self.wrapper.home_dot_repo = tempdir
+      self.wrapper.gpg_dir = os.path.join(self.wrapper.home_dot_repo, 'gnupg')
       self.assertTrue(self.wrapper.SetupGnuPG(True))
       with open(os.path.join(tempdir, 'keyring-version'), 'r') as fp:
         data = fp.read()