Extract env building into a testable helper.

Previously env dict building was untested and mixed with other mutative
actions. Extract the dict building into a dedicated function and author
tests to ensure the functionality is working as expected.

BUG: b/255376186
BUG: https://crbug.com/gerrit/16247
Change-Id: I0c88e53eb285c5c3fb27f8e6b3a903aedb8e02a8
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/351874
Reviewed-by: LaMont Jones <lamontjones@google.com>
Tested-by: Sam Saccone <samccone@google.com>

git_command.py

@@ -16,6 +16,7 @@ import functools
 import os
 import sys
 import subprocess
+from typing import Any, Optional
 
 from error import GitError
 from git_refs import HEAD
@@ -157,6 +158,53 @@ def git_require(min_version, fail=False, msg=''):
   return False
 
 
+def _build_env(
+  _kwargs_only=(),
+  bare: Optional[bool] = False,
+  disable_editor: Optional[bool] = False,
+  ssh_proxy: Optional[Any] = None,
+  gitdir: Optional[str] = None,
+  objdir: Optional[str] = None
+):
+  """Constucts an env dict for command execution."""
+
+  assert _kwargs_only == (), '_build_env only accepts keyword arguments.'
+
+  env = GitCommand._GetBasicEnv()
+
+  if disable_editor:
+    env['GIT_EDITOR'] = ':'
+  if ssh_proxy:
+    env['REPO_SSH_SOCK'] = ssh_proxy.sock()
+    env['GIT_SSH'] = ssh_proxy.proxy
+    env['GIT_SSH_VARIANT'] = 'ssh'
+  if 'http_proxy' in env and 'darwin' == sys.platform:
+    s = "'http.proxy=%s'" % (env['http_proxy'],)
+    p = env.get('GIT_CONFIG_PARAMETERS')
+    if p is not None:
+      s = p + ' ' + s
+    env['GIT_CONFIG_PARAMETERS'] = s
+  if 'GIT_ALLOW_PROTOCOL' not in env:
+    env['GIT_ALLOW_PROTOCOL'] = (
+        'file:git:http:https:ssh:persistent-http:persistent-https:sso:rpc')
+  env['GIT_HTTP_USER_AGENT'] = user_agent.git
+
+  if objdir:
+    # Set to the place we want to save the objects.
+    env['GIT_OBJECT_DIRECTORY'] = objdir
+
+    alt_objects = os.path.join(gitdir, 'objects') if gitdir else None
+    if (alt_objects and
+        os.path.realpath(alt_objects) != os.path.realpath(objdir)):
+      # Allow git to search the original place in case of local or unique refs
+      # that git will attempt to resolve even if we aren't fetching them.
+      env['GIT_ALTERNATE_OBJECT_DIRECTORIES'] = alt_objects
+  if bare and gitdir is not None:
+      env[GIT_DIR] = gitdir
+
+  return env
+
+
 class GitCommand(object):
   """Wrapper around a single git invocation."""
 
@@ -173,30 +221,13 @@ class GitCommand(object):
                cwd=None,
                gitdir=None,
                objdir=None):
-    env = self._GetBasicEnv()
-
-    if disable_editor:
-      env['GIT_EDITOR'] = ':'
-    if ssh_proxy:
-      env['REPO_SSH_SOCK'] = ssh_proxy.sock()
-      env['GIT_SSH'] = ssh_proxy.proxy
-      env['GIT_SSH_VARIANT'] = 'ssh'
-    if 'http_proxy' in env and 'darwin' == sys.platform:
-      s = "'http.proxy=%s'" % (env['http_proxy'],)
-      p = env.get('GIT_CONFIG_PARAMETERS')
-      if p is not None:
-        s = p + ' ' + s
-      env['GIT_CONFIG_PARAMETERS'] = s
-    if 'GIT_ALLOW_PROTOCOL' not in env:
-      env['GIT_ALLOW_PROTOCOL'] = (
-          'file:git:http:https:ssh:persistent-http:persistent-https:sso:rpc')
-    env['GIT_HTTP_USER_AGENT'] = user_agent.git
 
     if project:
       if not cwd:
         cwd = project.worktree
       if not gitdir:
         gitdir = project.gitdir
+
     # Git on Windows wants its paths only using / for reliability.
     if platform_utils.isWindows():
       if objdir:
@@ -204,18 +235,16 @@ class GitCommand(object):
       if gitdir:
         gitdir = gitdir.replace('\\', '/')
 
-    if objdir:
-      # Set to the place we want to save the objects.
-      env['GIT_OBJECT_DIRECTORY'] = objdir
-      if gitdir:
-        # Allow git to search the original place in case of local or unique refs
-        # that git will attempt to resolve even if we aren't fetching them.
-        env['GIT_ALTERNATE_OBJECT_DIRECTORIES'] = gitdir + '/objects'
+    env = _build_env(
+      disable_editor=disable_editor,
+      ssh_proxy=ssh_proxy,
+      objdir=objdir,
+      gitdir=gitdir,
+      bare=bare,
+    )
 
     command = [GIT]
     if bare:
-      if gitdir:
-        env[GIT_DIR] = gitdir
       cwd = None
     command.append(cmdv[0])
     # Need to use the --progress flag for fetch/clone so output will be

man/repo.1

@@ -25,7 +25,7 @@ control color usage: auto, always, never
 \fB\-\-trace\fR
 trace git command execution (REPO_TRACE=1)
 .TP
-\fB\-\-trace-to-stderr\fR
+\fB\-\-trace\-to\-stderr\fR
 trace outputs go to stderr in addition to
 \&.repo/TRACE_FILE
 .TP

progress.py

@@ -15,7 +15,7 @@
 import os
 import sys
 from time import time
-from repo_trace import IsTrace
+from repo_trace import IsTraceToStderr
 
 _NOT_TTY = not os.isatty(2)
 
@@ -80,7 +80,7 @@ class Progress(object):
   def update(self, inc=1, msg=''):
     self._done += inc
 
-    if _NOT_TTY or IsTrace():
+    if _NOT_TTY or IsTraceToStderr():
       return
 
     if not self._show:
@@ -113,7 +113,7 @@ class Progress(object):
       sys.stderr.flush()
 
   def end(self):
-    if _NOT_TTY or IsTrace() or not self._show:
+    if _NOT_TTY or IsTraceToStderr() or not self._show:
       return
 
     duration = duration_str(time() - self._start)

release/update-manpages

@@ -83,11 +83,6 @@ def main(argv):
     with multiprocessing.Pool() as pool:
       pool.map(partial(worker, cwd=tempdir, check=True), cmdlist)
 
-  regex = (
-      (r'(It was generated by help2man) [0-9.]+', '\g<1>.'),
-      (r'^\.IP\n(.*:)\n', '.SS \g<1>\n'),
-      (r'^\.PP\nDescription', '.SH DETAILS'),
-  )
   for tmp_path in MANDIR.glob('*.1.tmp'):
     path = tmp_path.parent / tmp_path.stem
     old_data = path.read_text() if path.exists() else ''
@@ -95,8 +90,7 @@ def main(argv):
     data = tmp_path.read_text()
     tmp_path.unlink()
 
-    for pattern, replacement in regex:
-      data = re.sub(pattern, replacement, data, flags=re.M)
+    data = replace_regex(data)
 
     # If the only thing that changed was the date, don't refresh.  This avoids
     # a lot of noise when only one file actually updates.
@@ -106,5 +100,25 @@ def main(argv):
       path.write_text(data)
 
 
+def replace_regex(data):
+  """Replace semantically null regexes in the data.
+
+  Args:
+    data: manpage text.
+
+  Returns:
+    Updated manpage text.
+  """
+  regex = (
+      (r'(It was generated by help2man) [0-9.]+', '\g<1>.'),
+      (r'^\033\[[0-9;]*m([^\033]*)\033\[m', '\g<1>'),
+      (r'^\.IP\n(.*:)\n', '.SS \g<1>\n'),
+      (r'^\.PP\nDescription', '.SH DETAILS'),
+  )
+  for pattern, replacement in regex:
+    data = re.sub(pattern, replacement, data, flags=re.M)
+  return data
+
+
 if __name__ == '__main__':
   sys.exit(main(sys.argv[1:]))

repo_trace.py

@@ -32,19 +32,14 @@ REPO_TRACE = 'REPO_TRACE'
 
 # Temporarily set tracing to always on unless user expicitly sets to 0.
 _TRACE = os.environ.get(REPO_TRACE) != '0'
-
 _TRACE_TO_STDERR = False
-
 _TRACE_FILE = None
-
 _TRACE_FILE_NAME = 'TRACE_FILE'
-
 _MAX_SIZE = 70  # in mb
-
 _NEW_COMMAND_SEP = '+++++++++++++++NEW COMMAND+++++++++++++++++++'
 
 
-def IsStraceToStderr():
+def IsTraceToStderr():
   return _TRACE_TO_STDERR
 
 
@@ -62,67 +57,76 @@ def SetTrace():
   _TRACE = True
 
 
-def _SetTraceFile():
+def _SetTraceFile(quiet):
   global _TRACE_FILE
-  _TRACE_FILE = _GetTraceFile()
+  _TRACE_FILE = _GetTraceFile(quiet)
 
 
 class Trace(ContextDecorator):
 
-    def _time(self):
-      """Generate nanoseconds of time in a py3.6 safe way"""
-      return int(time.time()*1e+9)
+  def _time(self):
+    """Generate nanoseconds of time in a py3.6 safe way"""
+    return int(time.time() * 1e+9)
 
-    def __init__(self, fmt, *args, first_trace=False):
-      if not IsTrace():
-        return
-      self._trace_msg = fmt % args
+  def __init__(self, fmt, *args, first_trace=False, quiet=True):
+    """Initialize the object.
 
-      if not _TRACE_FILE:
-        _SetTraceFile()
+    Args:
+      fmt: The format string for the trace.
+      *args: Arguments to pass to formatting.
+      first_trace: Whether this is the first trace of a `repo` invocation.
+      quiet: Whether to suppress notification of trace file location.
+    """
+    if not IsTrace():
+      return
+    self._trace_msg = fmt % args
 
-      if first_trace:
-        _ClearOldTraces()
-        self._trace_msg = '%s %s' % (_NEW_COMMAND_SEP, self._trace_msg)
+    if not _TRACE_FILE:
+      _SetTraceFile(quiet)
 
+    if first_trace:
+      _ClearOldTraces()
+      self._trace_msg = f'{_NEW_COMMAND_SEP} {self._trace_msg}'
 
-    def __enter__(self):
-      if not IsTrace():
-        return self
-
-      print_msg = f"PID: {os.getpid()} START: {self._time()} :" + self._trace_msg + '\n'
-
-      with open(_TRACE_FILE, 'a') as f:
-        print(print_msg, file=f)
-
-      if _TRACE_TO_STDERR:
-        print(print_msg, file=sys.stderr)
-
+  def __enter__(self):
+    if not IsTrace():
       return self
 
-    def __exit__(self, *exc):
-      if not IsTrace():
-        return False
+    print_msg = f'PID: {os.getpid()} START: {self._time()} :{self._trace_msg}\n'
 
-      print_msg = f"PID: {os.getpid()} END: {self._time()} :" + self._trace_msg + '\n'
+    with open(_TRACE_FILE, 'a') as f:
+      print(print_msg, file=f)
 
-      with open(_TRACE_FILE, 'a') as f:
-        print(print_msg, file=f)
+    if _TRACE_TO_STDERR:
+      print(print_msg, file=sys.stderr)
 
-      if _TRACE_TO_STDERR:
-        print(print_msg, file=sys.stderr)
+    return self
 
+  def __exit__(self, *exc):
+    if not IsTrace():
       return False
 
+    print_msg = f'PID: {os.getpid()} END: {self._time()} :{self._trace_msg}\n'
 
-def _GetTraceFile():
+    with open(_TRACE_FILE, 'a') as f:
+      print(print_msg, file=f)
+
+    if _TRACE_TO_STDERR:
+      print(print_msg, file=sys.stderr)
+
+    return False
+
+
+def _GetTraceFile(quiet):
   """Get the trace file or create one."""
   # TODO: refactor to pass repodir to Trace.
   repo_dir = os.path.dirname(os.path.dirname(__file__))
   trace_file = os.path.join(repo_dir, _TRACE_FILE_NAME)
-  print('Trace outputs in %s' % trace_file, file=sys.stderr)
+  if not quiet:
+    print(f'Trace outputs in {trace_file}', file=sys.stderr)
   return trace_file
 
+
 def _ClearOldTraces():
   """Clear the oldest commands if trace file is too big.
 
@@ -131,13 +135,13 @@ def _ClearOldTraces():
         will not work precisely.
   """
   if os.path.isfile(_TRACE_FILE):
-    while os.path.getsize(_TRACE_FILE)/(1024*1024) > _MAX_SIZE:
+    while os.path.getsize(_TRACE_FILE) / (1024 * 1024) > _MAX_SIZE:
       temp_file = _TRACE_FILE + '.tmp'
       with open(_TRACE_FILE, 'r', errors='ignore') as fin:
         with open(temp_file, 'w') as tf:
           trace_lines = fin.readlines()
-          for i , l in enumerate(trace_lines):
+          for i, l in enumerate(trace_lines):
             if 'END:' in l and _NEW_COMMAND_SEP in l:
-              tf.writelines(trace_lines[i+1:])
+              tf.writelines(trace_lines[i + 1:])
               break
       platform_utils.rename(temp_file, _TRACE_FILE)

subcmds/branches.py

@@ -155,11 +155,11 @@ is shown, then the branch appears in all projects.
         if i.IsSplitCurrent or (in_cnt <= project_cnt - in_cnt):
           in_type = 'in'
           for b in i.projects:
-            relpath = b.project.relpath
+            relpath = _RelPath(b.project)
             if not i.IsSplitCurrent or b.current:
-              paths.append(_RelPath(b.project))
+              paths.append(relpath)
             else:
-              non_cur_paths.append(_RelPath(b.project))
+              non_cur_paths.append(relpath)
         else:
           fmt = out.notinproject
           in_type = 'not in'

subcmds/diffmanifests.py

@@ -77,33 +77,35 @@ synced and their revisions won't be found.
                  metavar='<FORMAT>',
                  help='print the log using a custom git pretty format string')
 
-  def _printRawDiff(self, diff, pretty_format=None):
+  def _printRawDiff(self, diff, pretty_format=None, local=False):
+    _RelPath = lambda p: p.RelPath(local=local)
     for project in diff['added']:
-      self.printText("A %s %s" % (project.relpath, project.revisionExpr))
+      self.printText("A %s %s" % (_RelPath(project), project.revisionExpr))
       self.out.nl()
 
     for project in diff['removed']:
-      self.printText("R %s %s" % (project.relpath, project.revisionExpr))
+      self.printText("R %s %s" % (_RelPath(project), project.revisionExpr))
       self.out.nl()
 
     for project, otherProject in diff['changed']:
-      self.printText("C %s %s %s" % (project.relpath, project.revisionExpr,
+      self.printText("C %s %s %s" % (_RelPath(project), project.revisionExpr,
                                      otherProject.revisionExpr))
       self.out.nl()
       self._printLogs(project, otherProject, raw=True, color=False, pretty_format=pretty_format)
 
     for project, otherProject in diff['unreachable']:
-      self.printText("U %s %s %s" % (project.relpath, project.revisionExpr,
+      self.printText("U %s %s %s" % (_RelPath(project), project.revisionExpr,
                                      otherProject.revisionExpr))
       self.out.nl()
 
-  def _printDiff(self, diff, color=True, pretty_format=None):
+  def _printDiff(self, diff, color=True, pretty_format=None, local=False):
+    _RelPath = lambda p: p.RelPath(local=local)
     if diff['added']:
       self.out.nl()
       self.printText('added projects : \n')
       self.out.nl()
       for project in diff['added']:
-        self.printProject('\t%s' % (project.relpath))
+        self.printProject('\t%s' % (_RelPath(project)))
         self.printText(' at revision ')
         self.printRevision(project.revisionExpr)
         self.out.nl()
@@ -113,7 +115,7 @@ synced and their revisions won't be found.
       self.printText('removed projects : \n')
       self.out.nl()
       for project in diff['removed']:
-        self.printProject('\t%s' % (project.relpath))
+        self.printProject('\t%s' % (_RelPath(project)))
         self.printText(' at revision ')
         self.printRevision(project.revisionExpr)
         self.out.nl()
@@ -123,7 +125,7 @@ synced and their revisions won't be found.
       self.printText('missing projects : \n')
       self.out.nl()
       for project in diff['missing']:
-        self.printProject('\t%s' % (project.relpath))
+        self.printProject('\t%s' % (_RelPath(project)))
         self.printText(' at revision ')
         self.printRevision(project.revisionExpr)
         self.out.nl()
@@ -133,7 +135,7 @@ synced and their revisions won't be found.
       self.printText('changed projects : \n')
       self.out.nl()
       for project, otherProject in diff['changed']:
-        self.printProject('\t%s' % (project.relpath))
+        self.printProject('\t%s' % (_RelPath(project)))
         self.printText(' changed from ')
         self.printRevision(project.revisionExpr)
         self.printText(' to ')
@@ -148,7 +150,7 @@ synced and their revisions won't be found.
       self.printText('projects with unreachable revisions : \n')
       self.out.nl()
       for project, otherProject in diff['unreachable']:
-        self.printProject('\t%s ' % (project.relpath))
+        self.printProject('\t%s ' % (_RelPath(project)))
         self.printRevision(project.revisionExpr)
         self.printText(' or ')
         self.printRevision(otherProject.revisionExpr)
@@ -214,6 +216,8 @@ synced and their revisions won't be found.
 
     diff = manifest1.projectsDiff(manifest2)
     if opt.raw:
-      self._printRawDiff(diff, pretty_format=opt.pretty_format)
+      self._printRawDiff(diff, pretty_format=opt.pretty_format,
+                         local=opt.this_manifest_only)
     else:
-      self._printDiff(diff, color=opt.color, pretty_format=opt.pretty_format)
+      self._printDiff(diff, color=opt.color, pretty_format=opt.pretty_format,
+                      local=opt.this_manifest_only)

subcmds/sync.py

@@ -68,9 +68,12 @@ from manifest_xml import GitcManifest
 _ONE_DAY_S = 24 * 60 * 60
 # Env var to implicitly turn off object backups.
 REPO_BACKUP_OBJECTS = 'REPO_BACKUP_OBJECTS'
-
 _BACKUP_OBJECTS = os.environ.get(REPO_BACKUP_OBJECTS) != '0'
 
+# Env var to implicitly turn auto-gc back on.
+_REPO_AUTO_GC = 'REPO_AUTO_GC'
+_AUTO_GC = os.environ.get(_REPO_AUTO_GC) == '1'
+
 
 class _FetchOneResult(NamedTuple):
   """_FetchOne return value.
@@ -312,7 +315,7 @@ later is required to fix a server side protocol bug.
                  help='delete refs that no longer exist on the remote (default)')
     p.add_option('--no-prune', dest='prune', action='store_false',
                  help='do not delete refs that no longer exist on the remote')
-    p.add_option('--auto-gc', action='store_true',
+    p.add_option('--auto-gc', action='store_true', default=None,
                  help='run garbage collection on all synced projects')
     p.add_option('--no-auto-gc', dest='auto_gc', action='store_false',
                  help='do not run garbage collection on any projects (default)')
@@ -720,7 +723,7 @@ later is required to fix a server side protocol bug.
         # ...we'll let existing jobs finish, though.
         if not success:
           ret = False
-          err_results.append(project.relpath)
+          err_results.append(project.RelPath(local=opt.this_manifest_only))
           if opt.fail_fast:
             if pool:
               pool.close()
@@ -1219,6 +1222,11 @@ later is required to fix a server side protocol bug.
     if opt.prune is None:
       opt.prune = True
 
+    if opt.auto_gc is None and _AUTO_GC:
+      print(f"Will run `git gc --auto` because {_REPO_AUTO_GC} is set.",
+            file=sys.stderr)
+      opt.auto_gc = True
+
   def Execute(self, opt, args):
     manifest = self.outer_manifest
     if not opt.outer_manifest:

subcmds/upload.py

@@ -278,8 +278,9 @@ Gerrit Code Review:  https://www.gerritcodereview.com/
     script = []
     script.append('# Uncomment the branches to upload:')
     for project, avail in pending:
+      project_path = project.RelPath(local=opt.this_manifest_only)
       script.append('#')
-      script.append('# project %s/:' % project.RelPath(local=opt.this_manifest_only))
+      script.append(f'# project {project_path}/:')
 
       b = {}
       for branch in avail:
@@ -302,8 +303,8 @@ Gerrit Code Review:  https://www.gerritcodereview.com/
           script.append('#         %s' % commit)
         b[name] = branch
 
-      projects[project.RelPath(local=opt.this_manifest_only)] = project
-      branches[project.name] = b
+      projects[project_path] = project
+      branches[project_path] = b
     script.append('')
 
     script = Editor.EditString("\n".join(script)).split("\n")
@@ -328,9 +329,10 @@ Gerrit Code Review:  https://www.gerritcodereview.com/
         name = m.group(1)
         if not project:
           _die('project for branch %s not in script', name)
-        branch = branches[project.name].get(name)
+        project_path = project.RelPath(local=opt.this_manifest_only)
+        branch = branches[project_path].get(name)
         if not branch:
-          _die('branch %s not in %s', name, project.RelPath(local=opt.this_manifest_only))
+          _die('branch %s not in %s', name, project_path)
         todo.append(branch)
     if not todo:
       _die("nothing uncommented for upload")

tests/test_git_command.py

@@ -15,6 +15,7 @@
 """Unittests for the git_command.py module."""
 
 import re
+import os
 import unittest
 
 try:
@@ -26,6 +27,38 @@ import git_command
 import wrapper
 
 
+class GitCommandTest(unittest.TestCase):
+  """Tests the GitCommand class (via git_command.git)."""
+
+  def setUp(self):
+
+    def realpath_mock(val):
+      return val
+
+    mock.patch.object(os.path, 'realpath', side_effect=realpath_mock).start()
+
+  def tearDown(self):
+    mock.patch.stopall()
+
+  def test_alternative_setting_when_matching(self):
+    r = git_command._build_env(
+      objdir = 'zap/objects',
+      gitdir = 'zap'
+    )
+
+    self.assertIsNone(r.get('GIT_ALTERNATE_OBJECT_DIRECTORIES'))
+    self.assertEqual(r.get('GIT_OBJECT_DIRECTORY'),  'zap/objects')
+
+  def test_alternative_setting_when_different(self):
+    r = git_command._build_env(
+      objdir = 'wow/objects',
+      gitdir = 'zap'
+    )
+
+    self.assertEqual(r.get('GIT_ALTERNATE_OBJECT_DIRECTORIES'), 'zap/objects')
+    self.assertEqual(r.get('GIT_OBJECT_DIRECTORY'),  'wow/objects')
+
+
 class GitCallUnitTest(unittest.TestCase):
   """Tests the _GitCall class (via git_command.git)."""
 

tests/test_update_manpages.py

@@ -0,0 +1,27 @@
+# Copyright 2022 The Android Open Source Project
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Unittests for the update_manpages module."""
+
+import unittest
+import tests.update_manpages as um
+
+
+class UpdateManpagesTest(unittest.TestCase):
+  """Tests the update-manpages code."""
+
+  def test_replace_regex(self):
+    """Check that replace_regex works."""
+    data = '\n\033[1mSummary\033[m\n'
+    self.assertEqual(um.replace_regex(data),'\nSummary\n')

tests/update_manpages.py

@@ -0,0 +1 @@
+../release/update-manpages