Merge ~racb/git-ubuntu:fix-rich-history-unescaped-patch into git-ubuntu:master

[Robie Basak]

Make Jenkins happy

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

PASSED: Continuous integration, rev:e30452d73045c9a44dae172dae0029d79bbd58e8
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/512/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    SUCCESS: Unit Tests
    IN_PROGRESS: Declarative: Post Actions

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

review: Approve (continuous-integration)

[Bryce Harrington (bryce)]

8f84439e71276f94c56837872b97d3d9432bc33d
  - Test refactoring, to make it use a single repo rather than source
    and dest ones.
  - Drops the repo_fixture
  - LGTM
  √ Testsuite result: passed

1c52d81028861d18a43d09d18e926bbeb969fd2d
  - Adds xfail test for bug about patch in message

  - I would suggest some verbage changes to the test description:
    """Commit messages with patch-like contents should round trip

    If a commit message in rich history contains text in a patch-like
    format (such as a diff of some file), it should not prevent correct
    round-tripping. "git format-patch" followed by "git am" fails this
    test, for example.
    ...
    """
  - + :param repo: our standard repo fixture.
    Should be ":param Repo repo: our..."
  √ Testsuite result: passed

c2555935fcb8fc6285d60bcf5daa0599d7f6b0ec
  - Changes from using git am to git cherry-pick, the main implication
    is needing to hand it commit id's rather than patch mbox's.
  - This feels like it will be far more robust
  - One xfail test case is re-enabled
  √ Testsuite result: passed
  - LGTM

e30452d73045c9a44dae172dae0029d79bbd58e8
  - LGTM

This MP is well organized, first introducing an xfail test case that catches the bug, then the fix, and separating out a preliminary refactoring to support the test, and a cleanup after. I verified the xfail case does indeed fail initially, and is fixed in subsequent commits.

The fix itself seems a lot more robust to me, because it shifts to rely on git cherry-pick rather than git am and in my experience the former is a lot stricter and thus more reliable for our use case. cherry-pick leverages git's internal functionality, while git am's nature has it limited by text file i/o and the patch file format.

I have a couple suggestions on improvements for test case pydocs, but nothing major and no need for re-review.

review: Approve

[Robie Basak (racb)]

Thanks. Agreed with the docstring change. I have updated the patchset and pushed. Now awaiting CI result before landing.

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

PASSED: Continuous integration, rev:0376feae802c37ba6e71b7eca899230c32bc7904
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/513/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    SUCCESS: Unit Tests
    IN_PROGRESS: Declarative: Post Actions

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

review: Approve (continuous-integration)

diff --git a/doc/STYLE.md b/doc/STYLE.md
index 8a54d95..eb1e166 100644
--- a/doc/STYLE.md
+++ b/doc/STYLE.md
@@ -101,12 +101,3 @@ We use [pytest fixtures](https://docs.pytest.org/en/latest/fixture.html)
 to help us with this. Use of pytest's built-in `tmpdir` fixture is very
 common, as is `repo` and `pygit2_repo` as defined in
 `gitubuntu.test_fixtures`.
-
-Sometimes a test needs multiple instances of these fixtures.
-`gitubuntu.test_fixtures.repo_factory` achieves this for the `repo`
-fixture, and
-`gitubuntu.rich_history_test::test_rich_history_preservation` is an
-example of its use. A similar pattern may be used if multiple instances
-of other fixtures become necessary. In time it may make sense to
-refactor this into a generic "make a factory out of this fixture"
-function.
diff --git a/gitubuntu/rich_history.py b/gitubuntu/rich_history.py
index ac178fb..c474f3f 100644
--- a/gitubuntu/rich_history.py
+++ b/gitubuntu/rich_history.py
@@ -63,11 +63,13 @@ def export_upload_tag(repo, path, upload_tag, commit_map):
     """Export a single upload tag for later reconstruction
 
     If successful, two files will be created in the output directory: one with
-    extension '.base', and one with extension '.mbox'. The base name of these
+    extension '.base', and one with extension '.pick'. The base name of these
     files is the version string from the upload tag. The '.base' file will
     contain a single LF-terminated string which is the version string from the
-    import tag on which the upload tag patchset is based. The '.mbox' file will
-    contain the patchset as exported using 'git format-patch --stdout'.
+    import tag on which the upload tag patchset is based. The '.pick' file will
+    contain the patchset described as a series of commit hash strings that can
+    be picked in order, one on each line, as described by 'git rev-list
+    --reverse'.
 
     Note that the upload and import tag names are already escaped according to
     dep14, so the output filenames will have their version parts escaped
@@ -81,7 +83,7 @@ def export_upload_tag(repo, path, upload_tag, commit_map):
     :raises BaseNotFoundError: if a base import tag cannot be found.
     :raises MultipleParentError: if multiple parents for a commit are found.
     :raises subprocess.CalledProcessError: if the underlying call to "git
-        format-patch" fails.
+        rev-list" fails.
     :returns: None.
     """
     import_tag = get_upload_tag_base(repo, upload_tag, commit_map)
@@ -96,12 +98,12 @@ def export_upload_tag(repo, path, upload_tag, commit_map):
         initial_env=repo._initial_env,
         update_env=repo.env,
     )
-    with open(os.path.join(path, export_name) + '.mbox', 'w') as f:
+    with open(os.path.join(path, export_name) + '.pick', 'w') as f:
         subprocess.check_call(
             [
                 'git',
-                'format-patch',
-                '--stdout',
+                'rev-list',
+                '--reverse',
                 '%s..%s' % (import_tag.name, upload_tag.name),
             ],
             env=env,
@@ -155,8 +157,8 @@ def import_single(repo, path, version, namespace='importer'):
     :raises FileNotFoundError: if rich history cannot be found for this
         version.
     :raises BaseNotFoundError: if a base import tag cannot be found.
-    :raises subprocess.CalledProcessError: if the underlying call to "git
-        am" or "git tag" fail.
+    :raises subprocess.CalledProcessError: if any of the underlying calls to
+        "git cherry-pick" or "git tag" fail.
     :returns: None.
     """
     version_name = gitubuntu.git_repository.git_dep14_tag(version)
@@ -170,17 +172,17 @@ def import_single(repo, path, version, namespace='importer'):
     except KeyError as e:
         raise BaseNotFoundError() from e
 
-    with open(os.path.join(path, version_name) + '.mbox', 'r') as f:
+    with open(os.path.join(path, version_name) + '.pick', 'r') as pick:
         with repo.temporary_worktree(import_tag_name, 'rich-history-import'):
             env = {
                 'GIT_COMMITTER_NAME': spec.SYNTHESIZED_COMMITTER_NAME,
                 'GIT_COMMITTER_EMAIL': spec.SYNTHESIZED_COMMITTER_EMAIL,
             }
-            subprocess.check_call(
-                ['git', 'am'],
-                stdin=f,
-                env=env,
-            )
+            for commit_string in pick:
+                subprocess.check_call(
+                    ['git', 'cherry-pick', commit_string.strip()],
+                    env=env,
+                )
             upload_tag_name = '%s/upload/%s' % (namespace, version_name)
             subprocess.check_call(
                 [
diff --git a/gitubuntu/rich_history_test.py b/gitubuntu/rich_history_test.py
index 5223f2d..1716546 100644
--- a/gitubuntu/rich_history_test.py
+++ b/gitubuntu/rich_history_test.py
@@ -5,12 +5,12 @@ import pytest
 
 from gitubuntu.repo_builder import Blob, Commit, Placeholder, Repo, Tree
 import gitubuntu.spec as spec
-from gitubuntu.test_fixtures import repo, repo_factory
+from gitubuntu.test_fixtures import repo
 
 import gitubuntu.rich_history as target
 
 
-def test_rich_history_preservation(tmpdir, repo_factory):
+def test_rich_history_preservation(tmpdir, repo):
     """An export followed by an import should preserve rich history
 
     Given a minimal repository that should be able to have upload tags be
@@ -19,11 +19,8 @@ def test_rich_history_preservation(tmpdir, repo_factory):
     the result should be a correctly reconstructed upload tag.
 
     :param py.path tmpdir: the pytest standard tmpdir fixture.
-    :param repo_factory: our standard repo_factory fixture.
+    :param repo: our standard repo fixture.
     """
-    source_repo = next(repo_factory)
-    dest_repo = next(repo_factory)
-
     Repo(
         commits=[
             Commit(
@@ -51,8 +48,9 @@ def test_rich_history_preservation(tmpdir, repo_factory):
             'importer/import/1': Placeholder('1'),
             'importer/upload/2': Placeholder('3'),
         }
-    ).write(source_repo.raw_repo)
-    target.export_all(source_repo, str(tmpdir))
+    ).write(repo.raw_repo)
+    target.export_all(repo, str(tmpdir))
+    repo.delete_tags_in_namespace('importer')
     Repo(
         commits=[
             Commit(
@@ -64,9 +62,9 @@ def test_rich_history_preservation(tmpdir, repo_factory):
             )
         ],
         tags={'importer/import/1': Placeholder('1')},
-    ).write(dest_repo.raw_repo)
-    target.import_single(dest_repo, tmpdir, '2')
-    new_tag = dest_repo.raw_repo.lookup_reference(
+    ).write(repo.raw_repo)
+    target.import_single(repo, tmpdir, '2')
+    new_tag = repo.raw_repo.lookup_reference(
         'refs/tags/importer/upload/2',
     ).peel(pygit2.Tag)
     assert new_tag.tagger.name == spec.SYNTHESIZED_COMMITTER_NAME
@@ -75,8 +73,8 @@ def test_rich_history_preservation(tmpdir, repo_factory):
     assert new_commit.committer.name == spec.SYNTHESIZED_COMMITTER_NAME
     assert new_commit.committer.email == spec.SYNTHESIZED_COMMITTER_EMAIL
     new_tree = new_commit.peel(pygit2.Tree)
-    assert dest_repo.raw_repo.get(new_tree['a'].id).data == b'abc'
-    assert dest_repo.raw_repo.get(new_tree['b'].id).data == b'b'
+    assert repo.raw_repo.get(new_tree['a'].id).data == b'abc'
+    assert repo.raw_repo.get(new_tree['b'].id).data == b'b'
 
 
 def test_rich_history_preservation_multiple_parents(tmpdir, repo):
@@ -89,7 +87,7 @@ def test_rich_history_preservation_multiple_parents(tmpdir, repo):
     code.
 
     :param py.path tmpdir: the pytest standard tmpdir fixture.
-    :param repo_factory: our standard repo_factory fixture.
+    :param repo: our standard repo fixture.
     """
     Repo(
         commits=[
@@ -147,3 +145,55 @@ def test_rich_history_import_no_base(tmpdir, repo):
     tmpdir.join('2.base').write("1\n")
     with pytest.raises(target.BaseNotFoundError):
         target.import_single(repo, str(tmpdir), '2')
+
+
+def test_rich_history_preservation_patch_in_message(tmpdir, repo):
+    """Commit messages with patch-like contents should round trip
+
+    If a commit message in rich history contains text in a patch-like
+    format (such as a diff of some file), it should not prevent correct
+    round-tripping. "git format-patch" followed by "git am" fails this
+    test, for example.
+
+    :param py.path tmpdir: the pytest standard tmpdir fixture.
+    :param GitUbuntuRepository repo: our standard repo fixture.
+    """
+    Repo(
+        commits=[
+            Commit(
+                name='1',
+                message='a',
+                tree=Tree({
+                    'a': Blob(b'a'),
+                }),
+            ),
+            Commit(
+                name='2',
+                message='''First line
+
+Inline patch that isn't part of the real patch starts here
+
+--- a/foo
++++ b/foo
+@@ -1 +1,2 @@
+ foo
++bar
+''',
+                tree=Tree({
+                    'a': Blob(b'ab'),
+                }),
+                parents=[Placeholder('1')],
+            ),
+        ],
+        tags={
+            'importer/import/1': Placeholder('1'),
+            'importer/upload/2': Placeholder('2'),
+        }
+    ).write(repo.raw_repo)
+    target.export_all(repo, str(tmpdir))
+    repo.raw_repo.lookup_reference('refs/tags/importer/upload/2').delete()
+
+    # We already test in test_rich_history_preservation() that the import works
+    # correctly; this test solely checks that the following call does not raise
+    # an exception, so no further assertion is required.
+    target.import_single(repo, tmpdir, '2')
diff --git a/gitubuntu/test_fixtures.py b/gitubuntu/test_fixtures.py
index bbeb8f6..60a5457 100644
--- a/gitubuntu/test_fixtures.py
+++ b/gitubuntu/test_fixtures.py
@@ -35,26 +35,6 @@ def repo():
 
 
 @pytest.fixture
-def repo_factory():
-    """A fixture that generates a series of repo fixtures
-
-    This can be used by tests that require multiple independent repo fixtures.
-    """
-
-    # See https://stackoverflow.com/q/21583833/478206
-    with contextlib.ExitStack() as stack:
-        class Factory:
-            def __iter__(self):
-                return self
-
-            def __next__(self):
-                fixture_generator = repo()
-                stack.callback(fixture_generator.close)
-                return next(fixture_generator)
-        yield Factory()
-
-
[e-mail]
 def pygit2_repo():
     """An empty pygit2 repository fixture in a temporary directory"""
     with tempfile.TemporaryDirectory() as tmp_dir: