Extract atomic move function to utils.py

Moving atomically a file/directory can be tricky since different
errors might be raised for the same underlying problem.

Having a utility function to reduce this discrepancies will help
in ensuring we have correct behavior

4 files changed, 137 insertions, 81 deletions

diff --git a/buildstream/plugins/sources/git.py b/buildstream/plugins/sources/git.py
index b51ed79d9..18c8be82a 100644
--- a/buildstream/plugins/sources/git.py
+++ b/buildstream/plugins/sources/git.py
@@ -86,7 +86,6 @@ This plugin also utilises the following configurable core plugin warnings:
 """
 
 import os
-import errno
 import re
 import shutil
 from collections.abc import Mapping
@@ -97,6 +96,7 @@ from configparser import RawConfigParser
 from buildstream import Source, SourceError, Consistency, SourceFetcher
 from buildstream import utils
 from buildstream.plugin import CoreWarnings
+from buildstream.utils import move_atomic, DirectoryExistsError
 
 GIT_MODULES = '.gitmodules'
 
@@ -141,24 +141,16 @@ class GitMirror(SourceFetcher):
                                  fail="Failed to clone git repository {}".format(url),
                                  fail_temporarily=True)
 
-                self._atomic_move_mirror(tmpdir, url)
-
-    def _atomic_move_mirror(self, tmpdir, url):
-        # Attempt atomic rename into destination, this will fail if
-        # another process beat us to the punch
-        try:
-            os.rename(tmpdir, self.mirror)
-        except OSError as e:
-            # When renaming and the destination repo already exists, os.rename()
-            # will fail with either ENOTEMPTY or EEXIST, depending on the underlying
-            # implementation.
-            # An empty directory would always be replaced.
-            if e.errno in (errno.EEXIST, errno.ENOTEMPTY):
-                self.source.status("{}: Discarding duplicate clone of {}"
-                                   .format(self.source, url))
-            else:
-                raise SourceError("{}: Failed to move cloned git repository {} from '{}' to '{}': {}"
-                                  .format(self.source, url, tmpdir, self.mirror, e)) from e
+                try:
+                    move_atomic(tmpdir, self.mirror)
+                except DirectoryExistsError:
+                    # Another process was quicker to download this repository.
+                    # Let's discard our own
+                    self.source.status("{}: Discarding duplicate clone of {}"
+                                       .format(self.source, url))
+                except OSError as e:
+                    raise SourceError("{}: Failed to move cloned git repository {} from '{}' to '{}': {}"
+                                      .format(self.source, url, tmpdir, self.mirror, e)) from e
 
     def _fetch(self, alias_override=None):
         url = self.source.translate_url(self.url,
diff --git a/buildstream/utils.py b/buildstream/utils.py
index dc66f3e62..968d87f7b 100644
--- a/buildstream/utils.py
+++ b/buildstream/utils.py
@@ -72,6 +72,11 @@ class ProgramNotFoundError(BstError):
         super().__init__(message, domain=ErrorDomain.PROG_NOT_FOUND, reason=reason)
 
 
+class DirectoryExistsError(OSError):
+    """Raised when a `os.rename` is attempted but the destination is an existing directory.
+    """
+
+
 class FileListResult():
     """An object which stores the result of one of the operations
     which run on a list of files.
@@ -500,6 +505,38 @@ def get_bst_version():
                         .format(__version__))
 
 
+def move_atomic(source, destination, ensure_parents=True):
+    """Move the source to the destination using atomic primitives.
+
+    This uses `os.rename` to move a file or directory to a new destination.
+    It wraps some `OSError` thrown errors to ensure their handling is correct.
+
+    The main reason for this to exist is that rename can throw different errors
+    for the same symptom (https://www.unix.com/man-page/POSIX/3posix/rename/).
+
+    We are especially interested here in the case when the destination already
+    exists. In this case, either EEXIST or ENOTEMPTY are thrown.
+
+    In order to ensure consistent handling of these exceptions, this function
+    should be used instead of `os.rename`
+
+    Args:
+      source (str or Path): source to rename
+      destination (str or Path): destination to which to move the source
+      ensure_parents (bool): Whether or not to create the parent's directories
+                             of the destination (default: True)
+    """
+    if ensure_parents:
+        os.makedirs(os.path.dirname(str(destination)), exist_ok=True)
+
+    try:
+        os.rename(str(source), str(destination))
+    except OSError as exc:
+        if exc.errno in (errno.EEXIST, errno.ENOTEMPTY):
+            raise DirectoryExistsError(*exc.args) from exc
+        raise
+
+
 @contextmanager
 def save_file_atomic(filename, mode='w', *, buffering=-1, encoding=None,
                      errors=None, newline=None, closefd=True, opener=None, tempdir=None):
diff --git a/tests/sources/git.py b/tests/sources/git.py
index e5bce44a8..7ab32a6b5 100644
--- a/tests/sources/git.py
+++ b/tests/sources/git.py
@@ -21,14 +21,11 @@
 #
 
 import os
-from unittest import mock
 import pytest
-import py.path
 
 from buildstream._exceptions import ErrorDomain
-from buildstream import _yaml, SourceError
+from buildstream import _yaml
 from buildstream.plugin import CoreWarnings
-from buildstream.plugins.sources.git import GitMirror
 
 from tests.testutils import cli, create_repo
 from tests.testutils.site import HAVE_GIT
@@ -39,64 +36,6 @@ DATA_DIR = os.path.join(
 )
 
 
-@pytest.mark.parametrize(
-    ["type_", "setup"],
-    [
-        ("inexistent-dir", lambda path: None),
-        ("empty-dir", lambda path: path.mkdir()),
-        ("non-empty-dir", lambda path: path.join("bad").write("hi", ensure=True)),
-        ("file", lambda path: path.write("no")),
-    ],
-)
-def test_git_mirror_atomic_move(tmpdir, type_, setup):
-    # Create required elements
-    class DummySource:
-        def get_mirror_directory(self):
-            return str(tmpdir.join("source").mkdir())
-
-        status = mock.MagicMock()
-
-    url = "file://{}/url".format(tmpdir)
-
-    # Create fake download directory
-    download_dir = tmpdir.join("download_dir")
-    download_dir.join("oracle").write("hello", ensure=True)
-
-    # Initiate mirror
-    mirror = GitMirror(DummySource(), None, url, None)
-
-    # Setup destination directory
-    setup(py.path.local(mirror.mirror))
-
-    # Copy directory content
-    if type_ == "file":
-        with pytest.raises(SourceError):
-            mirror._atomic_move_mirror(str(download_dir), mirror.url)
-    else:
-        mirror._atomic_move_mirror(str(download_dir), mirror.url)
-
-    # Validate
-    if type_ == "non-empty-dir":
-        # With a non empty directory, we should not have overriden
-        # and notified a status
-        assert DummySource.status.called
-
-        expected_oracle = os.path.join(mirror.mirror, "bad")
-        expected_content = "hi"
-    elif type_ == "file":
-        expected_oracle = mirror.mirror
-        expected_content = "no"
-    else:
-        # Otherwise we should have a new directory and the data
-        expected_oracle = os.path.join(mirror.mirror, "oracle")
-        expected_content = "hello"
-
-    assert os.path.exists(expected_oracle)
-
-    with open(expected_oracle) as fp:
-        assert fp.read() == expected_content
-
-
 @pytest.mark.skipif(HAVE_GIT is False, reason="git is not available")
 @pytest.mark.datafiles(os.path.join(DATA_DIR, 'template'))
 def test_fetch_bad_ref(cli, tmpdir, datafiles):
diff --git a/tests/utils/movedirectory.py b/tests/utils/movedirectory.py
new file mode 100755
index 000000000..6edbec611
--- /dev/null
+++ b/tests/utils/movedirectory.py
@@ -0,0 +1,88 @@
+import pytest
+
+from buildstream.utils import move_atomic, DirectoryExistsError
+
+
+@pytest.fixture
+def src(tmp_path):
+    src = tmp_path.joinpath("src")
+    src.mkdir()
+
+    with src.joinpath("test").open("w") as fp:
+        fp.write("test")
+
+    return src
+
+
+def test_move_to_empty_dir(src, tmp_path):
+    dst = tmp_path.joinpath("dst")
+
+    move_atomic(src, dst)
+
+    assert dst.joinpath("test").exists()
+
+
+def test_move_to_empty_dir_create_parents(src, tmp_path):
+    dst = tmp_path.joinpath("nested/dst")
+
+    move_atomic(src, dst)
+    assert dst.joinpath("test").exists()
+
+
+def test_move_to_empty_dir_no_create_parents(src, tmp_path):
+    dst = tmp_path.joinpath("nested/dst")
+
+    with pytest.raises(FileNotFoundError):
+        move_atomic(src, dst, ensure_parents=False)
+
+
+def test_move_non_existing_dir(tmp_path):
+    dst = tmp_path.joinpath("dst")
+    src = tmp_path.joinpath("src")
+
+    with pytest.raises(FileNotFoundError):
+        move_atomic(src, dst)
+
+
+def test_move_to_existing_empty_dir(src, tmp_path):
+    dst = tmp_path.joinpath("dst")
+    dst.mkdir()
+
+    move_atomic(src, dst)
+    assert dst.joinpath("test").exists()
+
+
+def test_move_to_existing_file(src, tmp_path):
+    dst = tmp_path.joinpath("dst")
+
+    with dst.open("w") as fp:
+        fp.write("error")
+
+    with pytest.raises(NotADirectoryError):
+        move_atomic(src, dst)
+
+
+def test_move_file_to_existing_file(tmp_path):
+    dst = tmp_path.joinpath("dst")
+    src = tmp_path.joinpath("src")
+
+    with src.open("w") as fp:
+        fp.write("src")
+
+    with dst.open("w") as fp:
+        fp.write("dst")
+
+    move_atomic(src, dst)
+    with dst.open() as fp:
+        assert fp.read() == "src"
+
+
+def test_move_to_existing_non_empty_dir(src, tmp_path):
+    dst = tmp_path.joinpath("dst")
+    dst.mkdir()
+
+    with dst.joinpath("existing").open("w") as fp:
+        fp.write("already there")
+
+    with pytest.raises(DirectoryExistsError):
+        move_atomic(src, dst)