Merge ~racb/git-ubuntu:decouple-gitubunturepository into git-ubuntu:master

[Robie Basak]

Make Jenkins happy

This MP focuses on refactoring and pulling out methods in GitUbuntuRepository for operations that don't really need a GitUbuntuRepository instance. This allows for easier unit testing.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:a71ff92849ae06203ce7c2c5d7bdce355f042560
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/279/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Style Check
    SUCCESS: Unit Tests
    SUCCESS: Integration Tests
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/279/rebuild

review: Approve (continuous-integration)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:4d1d2bdb1d514bcf5bf0ac3e736bf88ff372dbca
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/281/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Style Check
    SUCCESS: Unit Tests
    SUCCESS: Integration Tests
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/281/rebuild

review: Approve (continuous-integration)

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:ebf9415338956776bfc27cd84d1ab4bc40e1d2bb
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/283/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Style Check
    SUCCESS: Unit Tests
    FAILED: Integration Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/283/rebuild

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:ebf9415338956776bfc27cd84d1ab4bc40e1d2bb
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/285/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Style Check
    SUCCESS: Unit Tests
    FAILED: Integration Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/285/rebuild

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:ebf9415338956776bfc27cd84d1ab4bc40e1d2bb
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/287/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Style Check
    SUCCESS: Unit Tests
    SUCCESS: Integration Tests
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/287/rebuild

review: Approve (continuous-integration)

[Robie Basak (racb)]

Just noticed a potential bug in the first commit that needs checking. @lru_cache will mean that every caller gets the same dictionary. Perhaps I should wrap the env property getter with a dict copy to make sure that one caller cannot accidentally modify the result given to another caller.

In fact I think I'd like to definitely do this for robustness. Please review assuming that this change is made:

- return self._derive_env()
+ return dict(self._derive_env())

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:b7d356fd39f76a2498866c639fbdbcbf168b7188
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/288/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Style Check
    SUCCESS: Unit Tests
    FAILED: Integration Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/288/rebuild

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:b7d356fd39f76a2498866c639fbdbcbf168b7188
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/289/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Style Check
    SUCCESS: Unit Tests
    SUCCESS: Integration Tests
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/289/rebuild

review: Approve (continuous-integration)

[Nish Aravamudan (nacc)]

review: Approve

diff --git a/gitubuntu/git_repository.py b/gitubuntu/git_repository.py
index f3bd406..d574783 100644
--- a/gitubuntu/git_repository.py
+++ b/gitubuntu/git_repository.py
@@ -109,6 +109,99 @@ def follow_symlinks_to_blob(repo, treeish_object, path):
     )
 
 
+def _derive_git_cli_env(
+    pygit2_repo,
+    initial_env=None,
+    update_env=None,
+    work_tree_path=None,
+    index_path=None,
+):
+    """Calculate the environment to be used in a call to the git CLI
+
+    :param pygit2.Repository pygit2_repo: the repository for which to calculate
+        the environment
+    :param dict initial_env: the environment to start with
+    :param dict update_env: additional environment setings with which to
+        override the result
+    :param str work_tree_path: in the case of an alternate work tree being
+        used, specify this here and GIT_WORK_TREE will be set to it instead of
+        the default being taken from the work tree used by pygit2_repo
+    :param str index_path: if an alternate index is being used, specify it here
+        and GIT_INDEX_FILE will be set accordingly.
+    :rtype: dict
+    :returns: a dictionary representing the environment with which to call the
+        git CLI
+
+    This function encapsulates the setting of the GIT_DIR, GIT_WORK_TREE and
+    GIT_INDEX_FILE environment variables as necessary. The provided
+    pygit2.Repository instance is used to determine these values. initial_env,
+    if provided, specifies the initial environment to use instead of defaulting
+    to the process' current environment. update_env allows extra environment
+    variables to be added as well as the override of any variables set by this
+    function, including GIT_DIR, GIT_WORK_TREE and GIT_INDEX_FILE.
+    """
+    if initial_env is None:
+        env = os.environ.copy()
+    else:
+        env = initial_env.copy()
+
+    env['GIT_DIR'] = pygit2_repo.path
+
+    if work_tree_path is None:
+        env['GIT_WORK_TREE'] = pygit2_repo.workdir
+    else:
+        env['GIT_WORK_TREE'] = work_tree_path
+
+    if index_path is not None:
+        env['GIT_INDEX_FILE'] = index_path
+
+    if update_env:
+        env.update(update_env)
+
+    return env
+
+
+def git_run(
+    pygit2_repo,
+    args,
+    initial_env=None,
+    update_env=None,
+    work_tree_path=None,
+    index_path=None,
+    **kwargs
+):
+    """Run the git CLI with the provided arguments
+
+    :param pygit2.Repository: the repository on which to act
+    :param list(str) args: arguments to the git CLI
+    :param dict initial_env: the environment to use
+    :param dict update_env: additional environment variables and overrides
+    :param dict **kwargs: further arguments to pass through to
+        gitubuntu.run.run()
+    :raises subprocess.CalledProcessError: if git exits non-zero
+    :rtype: (str, str)
+    :returns: stdout and stderr strings containing the subprocess output
+
+    If initial_env is not set, it defaults to the current process' environment.
+
+    The GIT_DIR, GIT_WORK_TREE and GIT_INDEX_FILE environment variables are set
+    automatically as necessary based on the repository's existing location and
+    settings.
+
+    If update_env is set, then the environment to be used is updated with env
+    before the call to git is made. This can override GIT_DIR,
+    GIT_WORK_TREE, GIT_INDEX_FILE and anything else.
+    """
+    env = _derive_git_cli_env(
+        pygit2_repo=pygit2_repo,
+        initial_env=initial_env,
+        update_env=update_env,
+        work_tree_path=work_tree_path,
+        index_path=index_path,
+    )
+    return run(['git'] + list(args), env=env, **kwargs)
+
+
 class RenameableDir:
     """An on-disk directory that can be renamed and traversed recursively.
 
@@ -779,10 +872,24 @@ class GitUbuntuRepository:
             self._local_dir = local_dir
 
         self.raw_repo = pygit2.init_repository(self._local_dir)
+        # We rely on raw_repo.workdir to be identical to self._local_dir to
+        # avoid changing previous behaviour in the setting of GIT_WORK_TREE, so
+        # assert that it is so. This may not be the case if the git repository
+        # has a different workdir stored in its configuration or if the git
+        # repository is a bare repository. We didn't handle these cases before
+        # anyway, so with this assertion we can fail noisily and early.
+        assert (
+            os.path.normpath(self.raw_repo.workdir) ==
+                os.path.normpath(self._local_dir)
+        )
 
-        self._env = os.environ.copy()
-        self._env['GIT_DIR'] = self.raw_repo.path
-        self._env['GIT_WORK_TREE'] = self._local_dir
+        # Since previous behaviour of this class depended on the state of the
+        # environment at the time it was constructed, save this for later use
+        # (for example in deriving the environment to use for calls to the git
+        # CLI). This permits the behaviour to remain identical for now.
+        # Eventually we can break previous behaviour and eliminate the need for
+        # this.
+        self._initial_env = os.environ.copy()
 
         self.set_git_attributes()
 
@@ -1224,7 +1331,24 @@ class GitUbuntuRepository:
 
     @property
     def env(self):
-        return self._env
+        # Return a copy of the cached _derive_env method result so that the
+        # caller cannot inadvertently modify our cached answer. Unfortunately
+        # this leaks the lru_cache-ness of the _derive_env method to this
+        # property getter, but this seems better than nothing.
+        return dict(self._derive_env())
+
+    @lru_cache()
+    def _derive_env(self):
+        """Determine what the git CLI environment should be
+
+        This depends on the initial environment saved from the constructor and
+        the paths associated with self.raw_repo, neither of which should change
+        in the lifetime of this class instance.
+        """
+        return _derive_git_cli_env(
+            self.raw_repo,
+            initial_env=self._initial_env
+        )
 
     @property
     def local_dir(self):
@@ -1308,12 +1432,33 @@ class GitUbuntuRepository:
         return stdout.strip()
 
     def git_run(self, args, env=None, **kwargs):
-        # Explicitly take a copy of self._env so the following update doesn't
-        # accidentally update the main instance _env dictionary in-place
-        combined_env = dict(self._env)
-        if env is not None:
-            combined_env.update(env)
-        return run(['git'] + args, env=combined_env, **kwargs)
+        """Run the git CLI with the provided arguments
+
+        :param list(str) args: arguments to the git CLI
+        :param dict env: additional environment variables to use
+        :param dict **kwargs: further arguments to pass through to
+            gitubuntu.run.run()
+        :raises subprocess.CalledProcessError: if git exits non-zero
+        :rtype: (str, str)
+        :returns: stdout and stderr strings containing the subprocess output
+
+        The environment used is based on the Python process' environment at the
+        time this class instance was constructed.
+
+        The GIT_DIR and GIT_WORK_TREE environment variables are set
+        automatically based on the repository's existing location and settings.
+
+        If env is set, then the environment to be used is updated with env
+        before the call to git is made. This can override GIT_DIR,
+        GIT_WORK_TREE, and anything else.
+        """
+        return git_run(
+            pygit2_repo=self.raw_repo,
+            args=args,
+            initial_env=self._initial_env,
+            update_env=env,
+            **kwargs,
+        )
 
     def garbage_collect(self):
         self.git_run(['gc'])
@@ -1626,9 +1771,9 @@ class GitUbuntuRepository:
     def clean_repository_state(self):
         """Cleanup working tree"""
         runq(['git', 'checkout', '--orphan', 'master'],
-             check=False, env=self._env)
-        runq(['git', 'reset', '--hard'], env=self._env)
-        runq(['git', 'clean', '-f', '-d'], env=self._env)
+             check=False, env=self.env)
+        runq(['git', 'reset', '--hard'], env=self.env)
+        runq(['git', 'clean', '-f', '-d'], env=self.env)
 
     def get_all_changelog_versions_from_treeish(self, treeish):
         changelog = self.get_changelog_from_treeish(treeish)
@@ -1795,9 +1940,13 @@ class GitUbuntuRepository:
         else:
             return tree_builder.write()  # create replacement tree object
 
-    def dir_to_tree(self, path, escape=False):
+    @classmethod
+    def dir_to_tree(cls, pygit2_repo, path, escape=False):
         """Create a git tree object from the given filesystem path
 
+        :param pygit2.Repository pygit2_repo: the repository on which to
+            operate. If you have a GitUbuntuRepository instance, you can use
+            its raw_repo property.
         :param path: path to filesystem directory to be the root of the tree
         :param escape: if True, escape using escape_dot_git() first. This
                        mutates the provided filesystem tree.
@@ -1821,30 +1970,24 @@ class GitUbuntuRepository:
         # we can use safely.
         with tempfile.TemporaryDirectory() as index_dir:
             index_path = os.path.join(index_dir, 'index')
-            index_env = {'GIT_INDEX_FILE': index_path}
-            self.git_run(
-                ['--work-tree', path, 'add', '-f', '-A'],
-                env=index_env,
-            )
-            self.git_run(
-                ['--work-tree', path, 'reset', 'HEAD', '--', '.git',],
-                env=index_env,
-            )
-            self.git_run(
-                ['--work-tree', path, 'reset', 'HEAD', '--', '.pc',],
-                env=index_env,
-            )
-            tree_hash_str, _ = self.git_run(
-                ['--work-tree', path, 'write-tree'],
-                env=index_env,
-            )
+            def indexed_git_run(*args):
+                return git_run(
+                    pygit2_repo=pygit2_repo,
+                    args=args,
+                    work_tree_path=path,
+                    index_path=index_path,
+                )
+            indexed_git_run('add', '-f', '-A')
+            indexed_git_run('reset', 'HEAD', '--', '.git')
+            indexed_git_run('reset', 'HEAD', '--', '.pc')
+            tree_hash_str, _ = indexed_git_run('write-tree')
             tree_hash_str = tree_hash_str.strip()
-            tree = self.raw_repo.get(tree_hash_str)
+            tree = pygit2_repo.get(tree_hash_str)
 
             # Add any empty directories that git did not import. Workaround for LP:
             # #1687057.
-            replacement_oid = self._add_missing_tree_dirs(
-                repo=self.raw_repo,
+            replacement_oid = cls._add_missing_tree_dirs(
+                repo=pygit2_repo,
                 top_path=path,
                 top_tree_object=tree,
             )
@@ -2194,7 +2337,7 @@ with other relevant fields (see below for details).
                         with open(fixup_patch_path, 'rb') as f:
                             run(['patch', '-Rp1',], input=f.read())
 
-                    return self.dir_to_tree('.')
+                    return self.dir_to_tree(self.raw_repo, '.')
                 else:
                     return commit_tree_hash
 
@@ -2291,7 +2434,7 @@ with other relevant fields (see below for details).
                     ],
                     env=self.env,
                 )
-                return self.dir_to_tree('.')
+                return self.dir_to_tree(self.raw_repo, '.')
 
         # otherwise, return @commit_hash's tree hash
         return commit_tree_hash
diff --git a/gitubuntu/importer.py b/gitubuntu/importer.py
index c998cf2..9f9c1ee 100644
--- a/gitubuntu/importer.py
+++ b/gitubuntu/importer.py
@@ -90,10 +90,10 @@ class ParentOverrideError(GitUbuntuImportError):
     pass
 
 
-def dsc_to_tree_hash(repo, dsc_path):
+def dsc_to_tree_hash(pygit2_repo, dsc_path):
     '''Convert a dsc file into a git tree in the given repo
 
-    repo: GitUbuntuRepository object
+    pygit2_repo: pygit2.Repository object
     dsc_path: string path to dsc file
 
     Returns: tree hash as a hex string
@@ -122,7 +122,11 @@ def dsc_to_tree_hash(repo, dsc_path):
                 "dpkg-source -x"
             )
 
-        return repo.dir_to_tree(extracted_dir, escape=True)
+        return GitUbuntuRepository.dir_to_tree(
+            pygit2_repo,
+            extracted_dir,
+            escape=True,
+        )
 
 
 def cleanup(no_clean, local_dir):
@@ -730,7 +734,7 @@ def import_patches_unapplied_tree(repo, dsc_pathname):
     dsc_pathname - A string path to a DSC file
     """
 
-    import_tree_hash = dsc_to_tree_hash(repo, dsc_pathname)
+    import_tree_hash = dsc_to_tree_hash(repo.raw_repo, dsc_pathname)
     repo.clean_repository_state()
 
     return import_tree_hash
diff --git a/gitubuntu/queue.py b/gitubuntu/queue.py
index 8c01b1f..8ea4690 100644
--- a/gitubuntu/queue.py
+++ b/gitubuntu/queue.py
@@ -196,7 +196,7 @@ def _commit_upload(repo, upload, parent_commit):
 
         # Import the source package into a git tree object
         tree_hash = gitubuntu.importer.dsc_to_tree_hash(
-            repo,
+            repo.raw_repo,
             os.path.join(tmpdir, download_key['dsc']),
         )
 
diff --git a/gitubuntu/source_builder_test.py b/gitubuntu/source_builder_test.py
index 0960836..b5eb896 100644
--- a/gitubuntu/source_builder_test.py
+++ b/gitubuntu/source_builder_test.py
@@ -16,7 +16,7 @@ def dsc_path_to_tree(repo, dsc_path):
     :param str dsc_path: path to dsc file
     :rtype: pygit2.Tree
     """
-    tree_hash = importer.dsc_to_tree_hash(repo, dsc_path)
+    tree_hash = importer.dsc_to_tree_hash(repo.raw_repo, dsc_path)
     return repo.raw_repo.get(tree_hash)
 
 
@@ -27,7 +27,7 @@ def test_source_is_created():
 
 def test_source_create_with_version(repo):
     with target.Source(target.SourceSpec(version=3)) as f:
-        tree_hash = importer.dsc_to_tree_hash(repo, f)
+        tree_hash = importer.dsc_to_tree_hash(repo.raw_repo, f)
         changelog = repo.get_changelog_from_treeish(tree_hash)
         assert changelog.version == '3'
 
diff --git a/gitubuntu/test_git_repository.py b/gitubuntu/test_git_repository.py
index dcb7d0e..4e29f9a 100644
--- a/gitubuntu/test_git_repository.py
+++ b/gitubuntu/test_git_repository.py
@@ -342,21 +342,31 @@ def test_escape_dot_git_ordering(direction):
         assert all(x is y for x, y in zip(top._rename_record, expected_order))
 
 
-def test_empty_dir_to_tree(repo, tmpdir):
-    tree_hash = repo.dir_to_tree(str(tmpdir))
-    assert tree_hash == str(Tree({}).write(repo.raw_repo))
+def test_empty_dir_to_tree(pygit2_repo, tmpdir):
+    tree_hash = target.GitUbuntuRepository.dir_to_tree(
+        pygit2_repo,
+        str(tmpdir),
+    )
+    assert tree_hash == str(Tree({}).write(pygit2_repo))
 
 
-def test_onefile_dir_to_tree(repo, tmpdir):
+def test_onefile_dir_to_tree(pygit2_repo, tmpdir):
     tmpdir.join('foo').write('bar')
-    tree_hash = repo.dir_to_tree(str(tmpdir))
-    assert tree_hash == str(Tree({'foo': Blob(b'bar')}).write(repo.raw_repo))
+    tree_hash = target.GitUbuntuRepository.dir_to_tree(
+        pygit2_repo,
+        str(tmpdir),
+    )
+    assert tree_hash == str(Tree({'foo': Blob(b'bar')}).write(pygit2_repo))
 
 
-def test_git_escape_dir_to_tree(repo, tmpdir):
+def test_git_escape_dir_to_tree(pygit2_repo, tmpdir):
     tmpdir.mkdir('.git')
-    tree_hash = repo.dir_to_tree(str(tmpdir), escape=True)
-    assert tree_hash == str(Tree({'..git': Tree({})}).write(repo.raw_repo))
+    tree_hash = target.GitUbuntuRepository.dir_to_tree(
+        pygit2_repo,
+        str(tmpdir),
+        escape=True,
+    )
+    assert tree_hash == str(Tree({'..git': Tree({})}).write(pygit2_repo))
 
 
 @pytest.mark.parametrize('tree_data,expected_path', [
@@ -450,3 +460,14 @@ def test_repo_quilt_env_from_treeish_str(repo):
     }
     for k, v in expected_inside.items():
         assert env[k] == v
+
+
+def test_repo_derive_env_change(repo):
+    # Changing the dictionary of a GitUbuntuRepository instance env attribute
+    # must not have any effect on the env itself. While this may stretch a
+    # little further than a normal instance property, it's worth enforcing this
+    # as this particular attribute is at particular risk due to how it tends to
+    # be used.
+    e1 = repo.env
+    e1[unittest.mock.sentinel.k] = unittest.mock.sentinel.v
+    assert unittest.mock.sentinel.k not in repo.env