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,20 +235,16 @@ class GitCommand(object):
       if gitdir:
         gitdir = gitdir.replace('\\', '/')
 
-    if objdir:
+    env = _build_env(
-      # Set to the place we want to save the objects.
+      disable_editor=disable_editor,
-      env['GIT_OBJECT_DIRECTORY'] = objdir
+      ssh_proxy=ssh_proxy,
-
+      objdir=objdir,
-      alt_objects = os.path.join(gitdir, 'objects') if gitdir else None
+      gitdir=gitdir,
-      if alt_objects and os.path.realpath(alt_objects) != os.path.realpath(objdir):
+      bare=bare,
-        # 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
 
     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

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)."""