Merge ~racb/usd-importer:escape-git into usd-importer:master

[Robie Basak]

Make Jenkins happy

[Nish Aravamudan (nacc)]

As mentioned in HO, this will need some setup.py changes in order to specify py.path as a dependency at runtime, or the snap will break. I think that would get caught by CI as `git ubuntu <...>` will probably fail to run.

[Nish Aravamudan (nacc)]

Looks really nice overall, just a few minor comments.

I think your commit message is also missinng a bug referencne.

[Robie Basak (racb)]

I've addressed all review comments so far. I've tested that origin/master fails with elinks, and that this branch succeeds with elinks with the escaping as expected.

[Robie Basak (racb)]

To address the setup.py issue, I dropped the dependency of py.path in runtime (as opposed to test time) code. The tests still use py.path, but I don't think this should be an issue as we already depend on py.test to run the tests and py.test depends on py.path already.

[Robie Basak (racb)]

https://code.launchpad.net/~racb/usd-importer/+git/usd-importer/+merge/334033 is a prerequisite of this MP, but I don't see how to add that after the MP has been created (save for a resubmit, which I won't bother with).

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

FAILED: Continuous integration, rev:fd857cd3f467477d5a68795ee808da67aeed98dd
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/216/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Style Check

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

review: Needs Fixing (continuous-integration)

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

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

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

review: Needs Fixing (continuous-integration)

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

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

review: Approve (continuous-integration)

[Andreas Hasenack (ahasenack)]

I see we set self._lp_user (notice the starting underscore) to either None or the correct value.

Later one we check if self._lp_user is None and then raise an exception, which is fine. But we subsequently use self.lp_user (no starting underscore).

This pattern happens in several places. Here is an example:

    def fetch_lpuser_remote(self, verbose=False):
        if not self._fetch_proto:
            raise Exception('Cannot fetch using an object without a protocol')
        if not self._lp_user:
            raise RuntimeError("Cannot fetch without knowing lp_user")
        self.fetch_remote(remote_name=self.lp_user, verbose=verbose)

Unless I'm missing something, self.lp_user does not exist. I'll highlight this in the diff below where I see it.

review: Needs Fixing

[Andreas Hasenack (ahasenack)]

Ah, ignore that comment. I see the def lp_user() @property now.

[Andreas Hasenack (ahasenack)]

Continuing the review. Looks like I cannot change a N/F review to just a comment.

[Andreas Hasenack (ahasenack)]

I reviewed up to 54c9b42 (add .git escaping functions). I see the Fake class there, used in some tests. Isn't it a bit risky to have basically two separate implementations/classes for RenameableDir, and rely on tests with the Fake one that will never be used for real in the code? Maybe I misunderstood.

To be continued

[Robie Basak (racb)]

On Tue, Nov 21, 2017 at 07:32:06PM -0000, Andreas Hasenack wrote:
> I reviewed up to 54c9b42 (add .git escaping functions). I see the Fake class there, used in some tests. Isn't it a bit risky to have basically two separate implementations/classes for RenameableDir, and rely on tests with the Fake one that will never be used for real in the code? Maybe I misunderstood.

It's unit testing. I'm testing that _escape_unescape_dot_git does the
right thing independently of RenameableDir. This makes it easier to test
all the different variations. But also, I have unit tests that check all
behaviour on both RenameableDir and FakeRenamableDir are correct.

Separately, elsewhere I expect to test everything integrated. For
example test_git_escape_dir_to_tree checks that escaping works
end-to-end with a real RenameableDir.

[Andreas Hasenack (ahasenack)]

I wanted to do an import of one of the problematic packages (elinks or lockfile-progs) and then try to build the source package with git ubuntu build-source, to see if the .git directory would be in there after all these manipulations, but that doesn't work yet:
- the import needs to be "for real", and not just local (aka, no --no-push)
- the build.* steps do not unescape yet (need a bug for that)

So I'll do that sometime later, or after these packages were imported for real.

Other than that, thanks a lot for the super detailed docstrings and tests, they were instrumental in verifying what the code was doing.

+1, let's land this

review: Approve

diff --git a/gitubuntu/git_repository.py b/gitubuntu/git_repository.py
index c598c10..616069c 100644
--- a/gitubuntu/git_repository.py
+++ b/gitubuntu/git_repository.py
@@ -4,6 +4,7 @@
 import collections
 from contextlib import contextmanager
 from copy import copy
+import enum
 from functools import lru_cache
 import itertools
 import logging
@@ -101,6 +102,231 @@ def follow_symlinks_to_blob(repo, treeish_object, path):
         search_path=posixpath.normpath(path),
     )
 
+
+class RenameableDir:
+    """An on-disk directory that can be renamed and traversed recursively.
+
+    This is a thin wrapper around a filesystem path string (and must be
+    instantiated with one). Methods and attributes are modeled around a
+    py.path, but we do not use py.path as we don't really need its
+    functionality and it would add another dependency. This interface allows
+    for filesystem operations to be easily faked with a FakeRenameableDir for
+    testing consumers of this class.
+
+    One wart around renaming and py.path is that once renamed a py.path object
+    becomes useless as it no longer validly refers to an on-disk path. Instead
+    of supporting a rename method, this wrapper instead provides a basename
+    setter which handles the rename and replacement wrapped string object
+    transparently. This moves complexity away from the class consumer, allowing
+    the consumer to be tested more easily.
+    """
+    def __init__(self, path):
+        """Create a new RenameableDir instance.
+
+        :param str path: the on-disk directory to wrap, which must exist.
+        """
+        assert os.path.exists(path)
+        self._path = path
+
+    @property
+    def basename(self):
+        """The name of the directory itself."""
+        return os.path.basename(self._path)
+
+    @basename.setter
+    def basename(self, new_basename):
+        """Rename this directory."""
+        renamed_path = os.path.join(os.path.dirname(self._path), new_basename)
+        os.rename(self._path, renamed_path)
+        self._path = renamed_path
+
+    def listdir(self, fil):
+        """Return subdirectory objects.
+
+        :param fil: a function that, given a basename, returns a boolean
+            indicating whether or not the corresponding object should be
+            returned in the results.
+        """
+        return [
+            RenameableDir(os.path.join(self._path, p))
+            for p in os.listdir(self._path)
+            if fil(p)
+        ]
+
+    @property
+    def recursive(self):
+        """Indicate if this object can contain subdirectory objects.
+
+        An object representing a file will return False. An object representing
+        a directory will return True, even if it is empty.
+
+        Symlinks return False even if they point to a directory.
+
+        :rtype: bool
+        """
+        st = os.stat(self._path, follow_symlinks=False)
+        return stat.S_ISDIR(st.st_mode)
+
+    def __str__(self):
+        return str(self._path)
+
+    def __repr__(self):
+        return 'RenameableDir(%r)' % str(self)
+
+    def __hash__(self):
+        # https://stackoverflow.com/q/2909106/478206
+        return hash((
+            type(self),
+            self._path
+        ))
+
+    def __eq__(self, other):
+        return hash(self) == hash(other)
+
+
+class FakeRenameableDir:
+    """A fake RenameDir that retains its structure in memory.
+
+    This is useful for testing consumers of a RenameableDir.
+
+    In addition, renames are recorded and those records passed up to parent
+    FakeRenameableDir objects so that the order of renames that occur can be
+    checked later.
+    """
+    def __init__(self, basename, subdirs):
+        """Create a new RenameableDir instance.
+
+        :param str basename: the basename of this instance.
+        :param subdirs: FakeRenameableDir objects contained within this one.
+            For non-recursive objects (such as those intended to represent
+            files), use None.
+        :type subdirs: list(FakeRenameableDir)
+        """
+        self._basename = basename
+        self._subdirs = subdirs
+
+        if self._subdirs:
+            for subdir in self._subdirs:
+                subdir._parent = self
+
+        self._parent = None
+        self._rename_record = []
+
+    @property
+    def basename(self):
+        return self._basename
+
+    @basename.setter
+    def basename(self, new_basename):
+        self._record_rename(self)
+        self._basename = new_basename
+
+    def _record_rename(self, obj):
+        self._rename_record.append(obj)
+        if self._parent:
+            self._parent._record_rename(obj)
+
+    def listdir(self, fil):
+        return (subdir for subdir in self._subdirs if fil(subdir.basename))
+
+    @property
+    def recursive(self):
+        return self._subdirs is not None
+
+    def __hash__(self):
+        # https://stackoverflow.com/q/2909106/478206
+        return hash((
+            type(self),
+            self.basename,
+            None if self._subdirs is None else tuple(self._subdirs),
+        ))
+
+    def __eq__(self, other):
+        return hash(self) == hash(other)
+
+    def __repr__(self):
+        return 'FakeRenameableDir(%r, %r)' % (self.basename, self._subdirs)
+
+
+_dot_git_match = re.compile(r'^\.+git$').search
+_EscapeDirection = enum.Enum('EscapeDirection', ['ESCAPE', 'UNESCAPE'])
+
+
+def _escape_unescape_dot_git(path, direction):
+    """Escape or unescape .git entries in a directory recursively.
+
+    :param RenameableDir path: top of directory tree to escape or unescape.
+    :param _EscapeDirection direction: whether to escape or unescape.
+
+    Escaping rules:
+        .git -> ..git
+        ..git -> ...git
+        ...git -> ....git
+        etc.
+
+        All these escaping rules apply all of the time, regardless of whether
+        or not .git exists. Only names matching '.git' with zero or more '.'
+        prepended are touched.
+
+    This allows any directory tree to be losslessly stored in git, since git
+    does not permit entries named '.git'.
+
+    Unescaping is the inverse of escaping. Before unescaping, an entry called
+    '.git' must not exist. If it does, RuntimeError is raised, and the
+    directory is left in an undefined (probably partially unescaped) state.
+    """
+    # When escaping, we have to rename ..git to ...git before renaming .git to
+    # ..git in order to make room, and the reverse for unescaping. If we do the
+    # renames ordered by length of name, we can meet this requirement.
+    # Escaping: order by longest first; unescaping: order by shortest first.
+    sorted_subpaths_to_rename = sorted(
+        path.listdir(fil=_dot_git_match),
+        key=lambda p: len(p.basename),
+        reverse=direction is _EscapeDirection.ESCAPE,
+    )
+    for entry in sorted_subpaths_to_rename:
+        if direction is _EscapeDirection.ESCAPE:
+            # Add a leading '.'
+            entry.basename = '.' + entry.basename
+        else:
+            assert direction is _EscapeDirection.UNESCAPE
+            if entry.basename == '.git':
+                raise RuntimeError(
+                    "%s exists but is invalid when unescaping" % entry,
+                )
+            # Drop the leading '.'
+            assert entry.basename[0] == '.'
+            entry.basename = entry.basename[1:]
+        if entry.recursive:
+            _escape_unescape_dot_git(entry, direction=direction)
+
+
+def escape_dot_git(path):
+    """Apply .git escaping to a filesystem path.
+
+    :param str path: path to filesystem to change
+    """
+    return _escape_unescape_dot_git(
+        path=RenameableDir(path),
+        direction=_EscapeDirection.ESCAPE,
+    )
+
+
+def unescape_dot_git(path):
+    """Unapply .git escaping to a filesystem path.
+
+    :param str path: path to filesystem to change
+
+    Any entry (including recursively) called '.git' in path is an error and
+    will raise a RuntimeError. If an exception is raised, path may be left in a
+    partially unescaped state.
+    """
+    return _escape_unescape_dot_git(
+        path=RenameableDir(path),
+        direction=_EscapeDirection.UNESCAPE,
+    )
+
+
 class ChangelogError(Exception):
     pass
 
@@ -511,8 +737,7 @@ class GitUbuntuRepository:
                 )
                 self._lp_user = self._lp_user.strip()
             except CalledProcessError:
-                logging.error("Unable to determine Launchpad user")
-                sys.exit(1)
+                self._lp_user = None
 
         if fetch_proto is None:
             fetch_proto = top_level_defaults.proto
@@ -815,6 +1040,8 @@ class GitUbuntuRepository:
     def _add_remote(self, remote_name, remote_url):
         if not self._fetch_proto:
             raise Exception('Cannot fetch using an object without a protocol')
+        if not self._lp_user:
+            raise RuntimeError("Cannot add remote without knowing lp_user")
         fetch_url = '%s://%s' % (self._fetch_proto, remote_url)
         push_url = 'ssh://%s@%s' % (self.lp_user, remote_url)
 
@@ -840,6 +1067,8 @@ class GitUbuntuRepository:
     def add_lpuser_remote(self, pkgname):
         if not self._fetch_proto:
             raise Exception('Cannot add a fetch using an object without a protocol')
+        if not self._lp_user:
+            raise RuntimeError("Cannot add remote without knowing lp_user")
         remote_url = ('git.launchpad.net/~%s/ubuntu/+source/%s' %
                       (self.lp_user, pkgname))
 
@@ -911,6 +1140,8 @@ class GitUbuntuRepository:
     def fetch_lpuser_remote(self, verbose=False):
         if not self._fetch_proto:
             raise Exception('Cannot fetch using an object without a protocol')
+        if not self._lp_user:
+            raise RuntimeError("Cannot fetch without knowing lp_user")
         self.fetch_remote(remote_name=self.lp_user, verbose=verbose)
 
     def copy_base_references(self, namespace):
@@ -1001,6 +1232,8 @@ class GitUbuntuRepository:
 
     @property
     def lp_user(self):
+        if not self._lp_user:
+            raise RuntimeError("lp_user is not set")
         return self._lp_user
 
     def get_commitish(self, commitish):
@@ -1505,7 +1738,27 @@ class GitUbuntuRepository:
         else:
             return tree_builder.write()  # create replacement tree object
 
-    def dir_to_tree(self, path):
+    def dir_to_tree(self, path, escape=False):
+        """Create a git tree object from the given filesystem path
+
+        :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.
+
+        escape should be used when the directory being moved into git is
+        directly from a source package, since the source package may contain
+        files or directories named '.git' and these cannot otherwise be
+        represented in a git tree object.
+
+        escape should not be used if the directory has already been escaped
+        previously. For example: if escape was previously used to move into a
+        git tree object, and that git tree object has been extracted to a
+        working directory for manipulation without unescaping, then escape
+        should not be used again to move that result back into a git tree
+        object.
+        """
+        if escape:
+            escape_dot_git(path)
         # git expects the index file to not exist (in order to create a fresh
         # one), so create a temporary directory to put it in so we have a name
         # we can use safely.
diff --git a/gitubuntu/importer.py b/gitubuntu/importer.py
index 2063f83..050d1d3 100644
--- a/gitubuntu/importer.py
+++ b/gitubuntu/importer.py
@@ -115,7 +115,7 @@ def dsc_to_tree_hash(repo, dsc_path):
                 "dpkg-source -x"
             )
 
-        return repo.dir_to_tree(extracted_dir)
+        return repo.dir_to_tree(extracted_dir, escape=True)
 
 
 def cleanup(no_clean, local_dir):
diff --git a/gitubuntu/source_information.py b/gitubuntu/source_information.py
index b97bc8a..9e8f172 100644
--- a/gitubuntu/source_information.py
+++ b/gitubuntu/source_information.py
@@ -16,7 +16,8 @@ try:
     from launchpadlib.launchpad import Launchpad as LP
     pkg = 'python3-ubuntutools'
     from ubuntutools.archive import UbuntuSourcePackage, DebianSourcePackage, DownloadError
-    from ubuntutools.lp.lpapicache import Launchpad, SourcePackagePublishingHistory
+    import ubuntutools.lp.lpapicache
+    from ubuntutools.lp.lpapicache import SourcePackagePublishingHistory
 except ImportError:
     logging.error('Is %s installed?', pkg)
     sys.exit(1)
@@ -37,7 +38,9 @@ def launchpad_login():
 
     _LP_LOGIN = LP.login_anonymously('git-ubuntu-importer', _lp_service,
                                      version=_lp_api_version)
-    Launchpad.login_existing(_LP_LOGIN)
+    # This is deliberately in a long form and we do not import the Launchpad
+    # object into the module namespace itself to work around LP: #1733388
+    ubuntutools.lp.lpapicache.Launchpad.login_existing(_LP_LOGIN)
 
     return _LP_LOGIN
 
diff --git a/gitubuntu/test_git_repository.py b/gitubuntu/test_git_repository.py
index 640dc16..b4f44d4 100644
--- a/gitubuntu/test_git_repository.py
+++ b/gitubuntu/test_git_repository.py
@@ -1,10 +1,14 @@
+import copy
+import itertools
+import os
 import tempfile
 import unittest
+import unittest.mock
 
 import pygit2
 import pytest
 
-from gitubuntu.git_repository import follow_symlinks_to_blob
+import gitubuntu.git_repository as target
 from gitubuntu.repo_builder import (
     Blob,
     Symlink,
@@ -14,6 +18,13 @@ from gitubuntu.repo_builder import (
 
 @pytest.fixture
 def repo():
+    """A empty GitUbuntuRepository repository fixture"""
+    with tempfile.TemporaryDirectory() as path:
+        yield target.GitUbuntuRepository(path)
+
+
[e-mail]
+def pygit2_repo():
     """A empty pygit2 repository fixture in a temporary directory"""
     with tempfile.TemporaryDirectory() as tmp_dir:
         yield pygit2.init_repository(tmp_dir)
@@ -65,11 +76,15 @@ def repo():
         'debian': Symlink('pkg'),
     })
 ])
-def test_follow_symlinks_to_blob(repo, tree_func):
+def test_follow_symlinks_to_blob(pygit2_repo, tree_func):
     blob = Blob(b'')
-    blob_id = blob.write(repo)
-    tree = repo.get(tree_func(blob).write(repo))
-    result_blob = follow_symlinks_to_blob(repo, tree, 'debian/changelog')
+    blob_id = blob.write(pygit2_repo)
+    tree = pygit2_repo.get(tree_func(blob).write(pygit2_repo))
+    result_blob = target.follow_symlinks_to_blob(
+        pygit2_repo,
+        tree,
+        'debian/changelog',
+    )
     assert result_blob.id == blob_id
 
 
@@ -79,7 +94,276 @@ def test_follow_symlinks_to_blob(repo, tree_func):
     Tree({'debian': Tree({'changelog': Symlink('other')})}),
     Tree({'debian': Tree({'changelog': Symlink('../other')})}),
 ])
-def test_follow_symlinks_to_blob_not_found(repo, tree):
-    pygit2_tree = repo.get(tree.write(repo))
+def test_follow_symlinks_to_blob_not_found(pygit2_repo, tree):
+    pygit2_tree = pygit2_repo.get(tree.write(pygit2_repo))
     with pytest.raises(KeyError):
-        follow_symlinks_to_blob(repo, pygit2_tree, 'debian/changelog')
+        target.follow_symlinks_to_blob(
+            pygit2_repo,
+            pygit2_tree,
+            'debian/changelog',
+        )
+
+
+def test_renameable_dir_basename(tmpdir):
+    p = tmpdir.join('foo')
+    p.ensure()
+    rd = target.RenameableDir(str(p))
+    assert rd.basename == 'foo'
+
+
+def test_renameable_dir_basename_setter(tmpdir):
+    p = tmpdir.join('foo')
+    p.ensure()
+    rd = target.RenameableDir(str(p))
+    rd.basename = 'bar'
+    assert rd.basename == 'bar'
+    assert tmpdir.join('bar').check()
+
+
+def test_dot_git_match(tmpdir):
+    for name in ['.git', 'git', '..git', 'other']:
+        tmpdir.join(name).ensure()
+
+    result = set(
+        x.basename
+        for x in tmpdir.listdir(
+            fil=lambda x: target._dot_git_match(str(x.basename))
+        )
+    )
+    assert result == set(['.git', '..git'])
+
+
+def test_renameable_dir_listdir(tmpdir):
+    for name in ['.git', 'git', '..git', 'other']:
+        tmpdir.join(name).ensure()
+    rd = target.RenameableDir(str(tmpdir))
+    result = set(rd.listdir(target._dot_git_match))
+    assert result == set([
+        target.RenameableDir(os.path.join(str(tmpdir), '.git')),
+        target.RenameableDir(os.path.join(str(tmpdir), '..git')),
+    ])
+
+
+def test_renamable_dir_recursive(tmpdir):
+    a = tmpdir.join('foo')
+    a.ensure_dir()
+    b = tmpdir.join('bar')
+    b.ensure()
+    assert target.RenameableDir(str(a)).recursive
+    assert not target.RenameableDir(str(b)).recursive
+
+
+def test_renameable_dir_str(tmpdir):
+    p = tmpdir.join('foo')
+    p.ensure()
+    rd = target.RenameableDir(str(p))
+    assert str(rd) == os.path.join(str(tmpdir), 'foo')
+
+
+def test_renameable_dir_repr(tmpdir):
+    p = tmpdir.join('foo')
+    p.ensure()
+    rd = target.RenameableDir(str(p))
+    assert repr(rd) == ("RenameableDir('%s/foo')" % str(tmpdir))
+
+
+def test_renameable_dir_hash_eq(tmpdir):
+    p1a = tmpdir.join('foo')
+    p1b = tmpdir.join('foo')
+    p2 = tmpdir.join('bar')
+
+    p1a.ensure()
+    p2.ensure()
+
+    rd1a = target.RenameableDir(str(p1a))
+    rd1b = target.RenameableDir(str(p1b))
+    rd2 = target.RenameableDir(str(p2))
+
+    assert rd1a == rd1b
+    assert rd1a != rd2
+
+
+def test_fake_renameable_dir_basename():
+    path = target.FakeRenameableDir('foo', None)
+    assert path.basename == 'foo'
+
+
+def test_fake_renameable_dir_basename_setter():
+    path = target.FakeRenameableDir('foo', None)
+    path.basename = 'bar'
+    assert path.basename == 'bar'
+
+
+def test_fake_renameable_dir_listdir():
+    path = target.FakeRenameableDir(None, [
+        target.FakeRenameableDir('.git', None),
+        target.FakeRenameableDir('git', None),
+        target.FakeRenameableDir('..git', None),
+        target.FakeRenameableDir('other', None),
+    ])
+    result = set(x.basename for x in path.listdir(fil=target._dot_git_match))
+    assert result == set(['.git', '..git'])
+
+
+def test_fake_renameable_dir_recursive():
+    assert target.FakeRenameableDir(['foo'], []).recursive
+    assert not target.FakeRenameableDir(['foo'], None).recursive
+
+
+def test_fake_renameable_dir_hash_eq():
+    variations = [
+        target.FakeRenameableDir(None, None),
+        target.FakeRenameableDir(None, []),
+        target.FakeRenameableDir('foo', []),
+        target.FakeRenameableDir(None, [
+            target.FakeRenameableDir('foo', None)]
+        ),
+        target.FakeRenameableDir(None, [
+            target.FakeRenameableDir('foo', [
+                target.FakeRenameableDir('bar', None)
+            ]),
+        ]),
+    ]
+    for a, b in itertools.product(variations, variations):
+        if a is b:
+            assert a == b
+        else:
+            assert a != b
+
+
+def test_fake_renameable_dir_repr():
+    rd = target.FakeRenameableDir('foo', [target.FakeRenameableDir('bar', [])])
+    assert (
+        repr(rd) == "FakeRenameableDir('foo', [FakeRenameableDir('bar', [])])"
+    )
+
+
[e-mail]('initial,expected', [
+    # Empty directory remains unchanged
+    (
+        target.FakeRenameableDir(None, []),
+        target.FakeRenameableDir(None, []),
+    ),
+    # Basic .git -> ..git escape
+    (
+        target.FakeRenameableDir(
+            None,
+            [target.FakeRenameableDir('.git', None)],
+        ),
+        target.FakeRenameableDir(
+            None,
+            [target.FakeRenameableDir('..git', None)],
+        ),
+    ),
+    # .git and ..git both exist
+    (
+        target.FakeRenameableDir(
+            None,
+            [
+                target.FakeRenameableDir(
+                    '.git',
+                    [target.FakeRenameableDir('.git', None)],
+                )
+            ],
+        ),
+        target.FakeRenameableDir(
+            None,
+            [
+                target.FakeRenameableDir(
+                    '..git',
+                    [target.FakeRenameableDir('..git', None)],
+                )
+            ],
+        ),
+    ),
+    # git remains unchanged
+    (
+        target.FakeRenameableDir(
+            None,
+            [target.FakeRenameableDir('git', None)],
+        ),
+        target.FakeRenameableDir(
+            None,
+            [target.FakeRenameableDir('git', None)],
+        ),
+    ),
+    # .git contains a .git
+    (
+        target.FakeRenameableDir(
+            None,
+            [
+                target.FakeRenameableDir('.git', None),
+                target.FakeRenameableDir('..git', None),
+            ],
+        ),
+        target.FakeRenameableDir(
+            None,
+            [
+                target.FakeRenameableDir('..git', None),
+                target.FakeRenameableDir('...git', None),
+            ],
+        ),
+    ),
+])
+def test_escape_dot_git(initial, expected):
+    state = copy.deepcopy(initial)
+    # Once escaped, we should get to what was expected
+    target._escape_unescape_dot_git(state, target._EscapeDirection.ESCAPE)
+    assert state == expected
+    # Once unescaped, we should get back to where we started since the escaping
+    # mechanism is lossless.
+    target._escape_unescape_dot_git(state, target._EscapeDirection.UNESCAPE)
+    assert state == initial
+
+
+def test_unescape_dot_git_raises():
+    """Test that unescaping something with '.git' raises an exception."""
+    with pytest.raises(RuntimeError):
+        target._escape_unescape_dot_git(
+            target.FakeRenameableDir(
+                None,
+                [target.FakeRenameableDir('.git', None)],
+            ),
+            direction=target._EscapeDirection.UNESCAPE,
+        )
+
+
[e-mail]('direction', [
+    target._EscapeDirection.ESCAPE,
+    target._EscapeDirection.UNESCAPE,
+])
+def test_escape_dot_git_ordering(direction):
+    """Test that renames happen in the correct order.
+
+    ...git -> ....git must happen before ..git -> ...git to avoid a collision,
+    and vice versa in the unescape case.
+    """
+    # Avoid '.git' as it isn't valid in the reverse direction
+    inner2 = target.FakeRenameableDir('..git', None)
+    inner3 = target.FakeRenameableDir('...git', None)
+    inputs = [inner2, inner3]
+    if direction is target._EscapeDirection.ESCAPE:
+        expected_order = [inner3, inner2]
+    else:
+        expected_order = [inner2, inner3]
+    for given_order in [inputs, reversed(inputs)]:
+        top = target.FakeRenameableDir(None, given_order)
+        target._escape_unescape_dot_git(top, 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_onefile_dir_to_tree(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))
+
+
+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))