diff options
author | Benjamin Schubert <ben.c.schubert@gmail.com> | 2018-11-07 13:07:09 +0000 |
---|---|---|
committer | richardmaw-codethink <richard.maw@codethink.co.uk> | 2018-11-19 11:39:51 +0000 |
commit | a6defc0b47423180c3f17a723692019458603bb6 (patch) | |
tree | af58414e2b742b0c98f11a718472f2c1ac6bef6f | |
parent | f23b6031b1b6bd27858f44d3deb63282483e6197 (diff) | |
download | buildstream-a6defc0b47423180c3f17a723692019458603bb6.tar.gz |
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
-rw-r--r-- | buildstream/plugins/sources/git.py | 33 | ||||
-rw-r--r-- | tests/sources/git.py | 63 |
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): |