Fix os.rename in git source element to correctly handle error codes

According to the documentation
(https://www.unix.com/man-page/POSIX/3posix/rename/), when the directory
already is there, either EEXIST or ENOTEMPTY could be thrown.

Previously only ENOTEMPTY was checked.

Done:
  - Separated the move into its own function
  - Check for both errors
  - Create unit tests for it, covering most test cases

2 files changed, 80 insertions, 16 deletions

diff --git a/buildstream/plugins/sources/git.py b/buildstream/plugins/sources/git.py
index 23e3369b1..b51ed79d9 100644
--- a/buildstream/plugins/sources/git.py
+++ b/buildstream/plugins/sources/git.py
@@ -141,21 +141,24 @@ class GitMirror(SourceFetcher):
                                  fail="Failed to clone git repository {}".format(url),
                                  fail_temporarily=True)
 
-                # 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 ENOTEMPTY, since an empty directory will be silently
-                    # replaced
-                    if e.errno == 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
+                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
 
     def _fetch(self, alias_override=None):
         url = self.source.translate_url(self.url,
diff --git a/tests/sources/git.py b/tests/sources/git.py
index 7ab32a6b5..e5bce44a8 100644
--- a/tests/sources/git.py
+++ b/tests/sources/git.py
@@ -21,11 +21,14 @@
 #
 
 import os
+from unittest import mock
 import pytest
+import py.path
 
 from buildstream._exceptions import ErrorDomain
-from buildstream import _yaml
+from buildstream import _yaml, SourceError
 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
@@ -36,6 +39,64 @@ 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):