Merge ~racb/usd-importer:fix-applied-branch-updates into usd-importer:master

[Robie Basak]

Make Jenkins happy

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

PASSED: Continuous integration, rev:d5543ac1e3788a1766f9a24ed874fa9948e9bd4f
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/219/
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/219/rebuild

review: Approve (continuous-integration)

[ChristianEhrhardt (paelzer)]

Hi,
the implementation of the quilt wrapper looks correct, but it might be duplication.

I wonder if you could instead make it work with git_repository.py::quilt_env ?

Current differences:
 - quilt_env sets up a temporary worktree - would we want to do that as well anyway?
   - if not then we could slightly enhance quilt_env to not do so if called without committish
 - your new wrapper has QUILT_REFRESH_ARGS - the other has EDITOR
   - I think we want all our wrappers to agree.

What is your opinion on that detail?

[Robie Basak (racb)]

10:58 <rbasak> Ah thanks. I wasn't aware of that wrapper. I'll look now.

11:00 <rbasak> cpaelzer: I think you're right. There are things covered there that I didn't cover, such as debian.patches. I should refactor and alter that method as needed.

review: Needs Fixing

[Robie Basak (racb)]

I think this is ready. I've rewritten the quilt modifications based on Christian's feedback (thanks!).

I've tested this manually checking that the commit graph appears correctly now using uvtool as a test import. I also checked that "git ubuntu build" still appears to work. I found bug 1734366, bug 1734370 and bug 1734371 while doing that, but none appear to be regressions caused by this branch.

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

PASSED: Continuous integration, rev:e7002beaad2f1197cb933d8bfd7b2db70a00981b
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/220/
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/220/rebuild

review: Approve (continuous-integration)

[ChristianEhrhardt (paelzer)]

Re-Reviewing:
Code: ok (I must admit thinking fully into the mock'iness of the test plus the depth of 71d70043 changing the behavior makes me shiver, but I couldn't spot any issues on this revision anymore).

Testing the related imports I had known: ok (on gnome-shell for example where I have seen this issue before).
Applied branches look good and are at the right different targets.

review: Approve

diff --git a/gitubuntu/git_repository.py b/gitubuntu/git_repository.py
index 616069c..b04cd16 100644
--- a/gitubuntu/git_repository.py
+++ b/gitubuntu/git_repository.py
@@ -679,6 +679,59 @@ class GitUbuntuRepositoryFetchError(Exception):
     pass
 
 
+def determine_quilt_series_path(pygit2_repo, treeish_obj):
+    """Find the active quilt series file path in use.
+
+    Look in the given tree for the Debian patch series file that is active
+    according to the search algorithm described in dpkg-source(1). If none are
+    found, return the default series path (again from dpkg-source(1)).
+
+    :param pygit2.Repo pygit2_repo: repository to look in.
+    :param pygit2.Object treeish_obj: object that peels to a pygit2.Tree.
+    :returns: relative path to series file.
+    :rtype: str
+    """
+    for series_name in ['debian.series', 'series']:
+        try:
+            series_path = posixpath.join('debian/patches', series_name)
+            blob = follow_symlinks_to_blob(
+                repo=pygit2_repo,
+                treeish_object=treeish_obj,
+                path=series_path,
+            )
+        except KeyError:
+            continue  # try the next path using our search list
+        return series_path  # series file blob found at this path
+
+    logging.debug("Unable to find a series file in %r", treeish_obj)
+    return 'debian/patches/series'  # default when no series file found
+
+
+def quilt_env(pygit2_repo, treeish):
+    """Find the appropriate quilt environment to use.
+
+    Return the canonical environment that should be used when calling quilt.
+    Since the series file doesn't necessarily always have the same name, a
+    source tree is examined to determine the name and set QUILT_SERIES
+    appropriately.
+
+    This does not integrate any other environment variables. Only environment
+    variables that influence quilt are returned.
+
+    :param pygit2.Repo pygit2_repo: repository to look in.
+    :param pygit2.Object treeish: object that peels to a pygit2.Tree.
+    :returns: quilt-specific environment settings
+    :rtype: dict
+    """
+    return {
+        'QUILT_PATCHES': 'debian/patches',
+        'QUILT_SERIES': determine_quilt_series_path(pygit2_repo, treeish),
+        'QUILT_NO_DIFF_INDEX': '1',
+        'QUILT_NO_DIFF_TIMESTAMPS': '1',
+        'EDITOR': 'true',
+    }
+
+
 class GitUbuntuRepository:
     """A class for interacting with an importer git repository
 
@@ -1882,38 +1935,47 @@ class GitUbuntuRepository:
         return subpath_tree_hash1 == subpath_tree_hash2
 
     @lru_cache()
-    def quilt_env(self, commit_hash):
-        with self.temporary_worktree(commit_hash):
-            combined_env = self.env.copy()
-            combined_env.update(
-                {
-                    'QUILT_PATCHES': 'debian/patches',
-                    'QUILT_NO_DIFF_INDEX': '1',
-                    'QUILT_NO_DIFF_TIMESTAMPS': '1',
-                    'EDITOR': 'true',
-                }
-            )
-            for path in [
-                os.path.join('debian', 'patches', 'debian.series'),
-                os.path.join('debian', 'patches', 'series'),
-            ]:
-                if os.path.exists(path):
-                    combined_env.update({'QUILT_SERIES': path,})
-                    break
-            else:
-                logging.debug(
-                    "Unable to find a series file in %s",
-                    commit_hash
-                )
+    def quilt_env(self, treeish):
+        """Return a suitable environment for running quilt.
+
+        This varies depending on the supplied commit since both
+        debian/patches/series and debian/patches/debian.series may be valid.
+        See dpkg-source(1) for details.
+
+        The returned environment includes all necessary environment by
+        combining self.env with the needed quilt-specific environment.
+
+        :param pygit.Object treeish: object that peels to the pygit2.Tree on
+            which quilt will operate.
+        :rtype: dict
+        :returns: an environment suitable for running quilt.
+        """
+        env = self.env.copy()
+        env.update(quilt_env(self.raw_repo, treeish))
+        return env
 
-            return combined_env
+    def quilt_env_from_treeish_str(self, treeish_str):
+        """Return a suitable environment for running quilt.
+
+        This is a thin wrapper around quilt_env() that works with a treeish hex
+        string instead of directly with a treeish object.
+
+        :param str treeish_str: the hash of the tree on which quilt will
+            operate, in hex.
+        :rtype: dict
+        :returns: an environment suitable for running quilt.
+        """
+        return self.quilt_env(self.raw_repo.get(treeish_str))
 
     def is_patches_applied(self, commit_hash, regenerated_pc_path):
         # first see if quilt push -a would do anything to
         # differentiate between applied an unapplied
         with self.temporary_worktree(commit_hash):
             try:
-                run(['quilt', 'push', '-a'], env=self.quilt_env(commit_hash))
+                run(
+                    ['quilt', 'push', '-a'],
+                    env=self.quilt_env_from_treeish_str(commit_hash),
+                )
                 # False if in an unapplied state, which is signified by
                 # successful push (rc=0)
                 return False
@@ -1936,7 +1998,10 @@ class GitUbuntuRepository:
                         raise e
 
                     try:
-                        run(['quilt', 'push', '-a'], env=self.quilt_env(commit_hash))
+                        run(
+                            ['quilt', 'push', '-a'],
+                            env=self.quilt_env_from_treeish_str(commit_hash),
+                        )
                         # False if in an unapplied state
                         return False
                     except CalledProcessError as e:
@@ -2031,7 +2096,7 @@ class GitUbuntuRepository:
                 # generate the equivalent .pc directory
                 run(
                     ['quilt', 'push', '-a'],
-                    env=self.quilt_env(commit_hash),
+                    env=self.quilt_env_from_treeish_str(commit_hash),
                     rcs=[2],
                 )
 
@@ -2082,7 +2147,7 @@ class GitUbuntuRepository:
                         '.',
                         'git-ubuntu-fixup.patch',
                     ],
-                    env=self.quilt_env(commit_hash),
+                    env=self.quilt_env_from_treeish_str(commit_hash),
                 )
 
                 # do not want the .pc directory in the resulting
diff --git a/gitubuntu/importer.py b/gitubuntu/importer.py
index 050d1d3..e8fb4f8 100644
--- a/gitubuntu/importer.py
+++ b/gitubuntu/importer.py
@@ -36,6 +36,7 @@ from subprocess import CalledProcessError
 import sys
 import tempfile
 import time
+import traceback
 from gitubuntu.__main__ import top_level_defaults
 from gitubuntu.cache import CACHE_PATH
 from gitubuntu.dsc import (
@@ -746,16 +747,29 @@ def import_patches_applied_tree(repo, dsc_pathname):
         raise SourceExtractionException("Failed to find an extracted "
                                         "directory from dpkg-source -x")
 
-    if os.path.isdir(os.path.join(extracted_dir, '.pc')):
-        repo.git_run(
-            ['--work-tree', extracted_dir, 'add', '-f', '-A']
-        )
+    repo.git_run(
+        ['--work-tree', extracted_dir, 'add', '-f', '-A']
+    )
+
+    # If .pc exists then we remove it in a separate commit so that it doesn't
+    # interfere with our upcoming quilt patch applications.
+    has_dot_pc = os.path.isdir(os.path.join(extracted_dir, '.pc'))
+    if has_dot_pc:
         repo.git_run(
             ['--work-tree', extracted_dir, 'rm', '-r', '-f', '.pc']
         )
-        import_tree_hash, _ = repo.git_run(
-            ['--work-tree', extracted_dir, 'write-tree']
-        )
+
+    # In all cases we need the tree object in order to determine the correct
+    # quilt environment later. import_tree_hash will be updated to always be
+    # the hex hash string of the tree object before quilt push was applied.
+    import_tree_hash_ws, _ = repo.git_run(
+        ['--work-tree', extracted_dir, 'write-tree']
+    )
+    import_tree_hash = import_tree_hash_ws.strip()
+
+    # If .pc did exist, then we yield its tree object hash string in order for
+    # the caller to create a separate commit for it.
+    if has_dot_pc:
         yield (
             import_tree_hash.strip(),
             None,
@@ -769,10 +783,20 @@ def import_patches_applied_tree(repo, dsc_pathname):
         while True:
             try:
                 os.chdir(extracted_dir)
-                run(['quilt', 'push'], verbose_on_failure=False,)
-                patch_name, _ = run(['quilt', 'top'])
+                run(
+                    ['quilt', 'push'],
+                    env=repo.quilt_env_from_treeish_str(import_tree_hash),
+                    verbose_on_failure=False,
+                )
+                patch_name, _ = run(
+                    ['quilt', 'top'],
+                    env=repo.quilt_env_from_treeish_str(import_tree_hash),
+                )
                 patch_name = patch_name.strip()
-                header, _  = run(['quilt', 'header'])
+                header, _  = run(
+                    ['quilt', 'header'],
+                    env=repo.quilt_env_from_treeish_str(import_tree_hash),
+                )
                 header = header.strip()
                 patch_desc = None
                 for regex in (r'Subject:\s*(.*?)$',
@@ -802,11 +826,16 @@ def import_patches_applied_tree(repo, dsc_pathname):
                     '--',
                     '.pc',
                 ])
-                import_tree_hash, _ = repo.git_run(
+
+                # Update import_tree_hash both in order to be able to yield it
+                # but also to make it available to determine the quilt
+                # environment in the next iteration of this loop.
+                import_tree_hash_ws, _ = repo.git_run(
                     ['--work-tree', extracted_dir, 'write-tree']
                 )
+                import_tree_hash = import_tree_hash_ws.strip()
 
-                yield (import_tree_hash.strip(), patch_name, patch_desc)
+                yield (import_tree_hash, patch_name, patch_desc)
             except CalledProcessError as e:
                 # quilt returns 2 when done pushing
                 if e.returncode != 2:
@@ -1326,7 +1355,7 @@ def import_applied_spi(
                                 )
                 break
 
-    applied_tag = repo.get_applied_tag(version, namespace)
+    applied_tag = repo.get_applied_tag(spi.version, namespace)
     if applied_tag:
         # XXX: What should be checked here? The result of pushing all
         # the patches? How to do it most non-destructively? (Might be
@@ -1461,6 +1490,7 @@ def import_publishes(
                 raise GitUbuntuImportError(msg) from e
             else:
                 logging.error(msg)
+                logging.error(traceback.format_exc())
         else:
             history_found = True
 
diff --git a/gitubuntu/test_git_repository.py b/gitubuntu/test_git_repository.py
index b4f44d4..19ef990 100644
--- a/gitubuntu/test_git_repository.py
+++ b/gitubuntu/test_git_repository.py
@@ -367,3 +367,96 @@ def test_git_escape_dir_to_tree(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))
+
+
[e-mail]('tree_data,expected_path', [
+    # Empty tree -> default
+    (Tree({}), 'debian/patches/series'),
+
+    # Empty debian/patches directory -> default
+    (Tree({'debian': Tree({'patches': Tree({})})}), 'debian/patches/series'),
+
+    # Only debian/patches/series -> that one
+    (
+        Tree({'debian': Tree({'patches': Tree({'series': Blob(b'')})})}),
+        'debian/patches/series',
+    ),
+
+    # Only debian/patches/debian.series -> that one
+    (
+        Tree({'debian': Tree({'patches': Tree({
+            'debian.series': Blob(b'')
+        })})}),
+        'debian/patches/debian.series',
+    ),
+
+    # Both -> debian.series
+    (
+        Tree({'debian': Tree({'patches': Tree({
+            'debian.series': Blob(b''),
+            'series': Blob(b''),
+        })})}),
+        'debian/patches/debian.series',
+    ),
+])
+def test_determine_quilt_series_path(pygit2_repo, tree_data, expected_path):
+    tree_obj = pygit2_repo.get(tree_data.write(pygit2_repo))
+    path = target.determine_quilt_series_path(pygit2_repo, tree_obj)
+    assert path == expected_path
+
+
+def test_quilt_env(pygit2_repo):
+    tree_builder = Tree({'debian':
+        Tree({'patches': Tree({'debian.series': Blob(b'')})})
+    })
+    tree_obj = pygit2_repo.get(tree_builder.write(pygit2_repo))
+    env = target.quilt_env(pygit2_repo, tree_obj)
+    assert env == {
+        'EDITOR': 'true',
+        'QUILT_NO_DIFF_INDEX': '1',
+        'QUILT_NO_DIFF_TIMESTAMPS': '1',
+        'QUILT_PATCHES': 'debian/patches',
+        'QUILT_SERIES': 'debian/patches/debian.series',
+    }
+
+
+def test_repo_quilt_env(repo):
+    tree_builder = Tree({'debian':
+        Tree({'patches': Tree({'debian.series': Blob(b'')})})
+    })
+    tree_obj = repo.raw_repo.get(tree_builder.write(repo.raw_repo))
+    env = repo.quilt_env(tree_obj)
+    expected_inside = {
+        'EDITOR': 'true',
+        'QUILT_NO_DIFF_INDEX': '1',
+        'QUILT_NO_DIFF_TIMESTAMPS': '1',
+        'QUILT_PATCHES': 'debian/patches',
+        'QUILT_SERIES': 'debian/patches/debian.series',
+    }
+    for k, v in expected_inside.items():
+        assert env[k] == v
+
+    # In addition to the settings above, check that
+    # GitUbuntuRepository.quilt_env has correctly merged in the usual
+    # environment. Testing that a few keys that we expect to be set are set
+    # should suffice.
+    expected_other_keys = ['HOME', 'GIT_DIR', 'GIT_WORK_TREE']
+    for k in expected_other_keys:
+        assert env[k]
+
+
+def test_repo_quilt_env_from_treeish_str(repo):
+    tree_builder = Tree({'debian':
+        Tree({'patches': Tree({'debian.series': Blob(b'')})})
+    })
+    tree_obj = repo.raw_repo.get(tree_builder.write(repo.raw_repo))
+    env = repo.quilt_env_from_treeish_str(str(tree_obj.id))
+    expected_inside = {
+        'EDITOR': 'true',
+        'QUILT_NO_DIFF_INDEX': '1',
+        'QUILT_NO_DIFF_TIMESTAMPS': '1',
+        'QUILT_PATCHES': 'debian/patches',
+        'QUILT_SERIES': 'debian/patches/debian.series',
+    }
+    for k, v in expected_inside.items():
+        assert env[k] == v