[Notes] [Git][BuildStream/buildstream][bschubert/fix-atomic-move-git-repo] Fix os.rename in git source element to correctly handle error codes

[Benjamin Schubert <gitlab mg gitlab com>]

Title: GitLab

Benjamin Schubert pushed to branch bschubert/fix-atomic-move-git-repo at BuildStream / buildstream

Commits:

• c966ae05
  by Benjamin Schubert at 2018-11-07T15:14:32Z

  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
  

3 changed files:

• buildstream/plugins/sources/git.py
• tests/frontend/buildtrack.py
• tests/sources/git.py

Changes:

buildstream/plugins/sources/git.py

---

...	...	@@ -141,21 +141,24 @@ class GitMirror(SourceFetcher):
141	141	fail="Failed to clone git repository {}".format(url),
142	142	fail_temporarily=True)
143	143
144		- # Attempt atomic rename into destination, this will fail if
145		- # another process beat us to the punch
146		- try:
147		- os.rename(tmpdir, self.mirror)
148		- except OSError as e:
149		-
150		- # When renaming and the destination repo already exists, os.rename()
151		- # will fail with ENOTEMPTY, since an empty directory will be silently
152		- # replaced
153		- if e.errno == errno.ENOTEMPTY:
154		- self.source.status("{}: Discarding duplicate clone of {}"
155		- .format(self.source, url))
156		- else:
157		- raise SourceError("{}: Failed to move cloned git repository {} from '{}' to '{}': {}"
158		- .format(self.source, url, tmpdir, self.mirror, e)) from e
	144	+ self._atomic_move_mirror(tmpdir, url)
	145	+
	146	+ def _atomic_move_mirror(self, tmpdir, url):
	147	+ # Attempt atomic rename into destination, this will fail if
	148	+ # another process beat us to the punch
	149	+ try:
	150	+ os.rename(tmpdir, self.mirror)
	151	+ except OSError as e:
	152	+ # When renaming and the destination repo already exists, os.rename()
	153	+ # will fail with either ENOTEMPTY or EEXIST, depending on the underlying
	154	+ # implementation.
	155	+ # An empyt directoryw ould always be replaced.
	156	+ if e.errno in (errno.EEXIST, errno.ENOTEMPTY):
	157	+ self.source.status("{}: Discarding duplicate clone of {}"
	158	+ .format(self.source, url))
	159	+ else:
	160	+ raise SourceError("{}: Failed to move cloned git repository {} from '{}' to '{}': {}"
	161	+ .format(self.source, url, tmpdir, self.mirror, e)) from e
159	162
160	163	def _fetch(self, alias_override=None):
161	164	url = self.source.translate_url(self.url,

tests/frontend/buildtrack.py

---

...	...	@@ -115,6 +115,7 @@ def test_build_track(cli, datafiles, tmpdir, ref_storage,
115	115	args += ['0.bst']
116	116
117	117	result = cli.run(project=project, silent=True, args=args)
	118	+ result.assert_success()
118	119	tracked_elements = result.get_tracked_elements()
119	120
120	121	assert set(tracked_elements) == set(tracked)

tests/sources/git.py

---

...	...	@@ -21,11 +21,14 @@
21	21	#
22	22
23	23	import os
	24	+from unittest import mock
24	25	import pytest
	26	+import py.path
25	27
26	28	from buildstream._exceptions import ErrorDomain
27		-from buildstream import _yaml
	29	+from buildstream import _yaml, SourceError
28	30	from buildstream.plugin import CoreWarnings
	31	+from buildstream.plugins.sources.git import GitMirror
29	32
30	33	from tests.testutils import cli, create_repo
31	34	from tests.testutils.site import HAVE_GIT
...	...	@@ -36,6 +39,64 @@ DATA_DIR = os.path.join(
36	39	)
37	40
38	41
	42	+@pytest.mark.parametrize(
	43	+ ["type_", "setup"],
	44	+ [
	45	+ ("inexistent-dir", lambda path: None),
	46	+ ("empty-dir", lambda path: path.mkdir()),
	47	+ ("non-empty-dir", lambda path: path.join("bad").write("hi", ensure=True)),
	48	+ ("file", lambda path: path.write("no")),
	49	+ ],
	50	+)
	51	+def test_git_mirror_atomic_move(tmpdir, type_, setup):
	52	+ # Create required elements
	53	+ class DummySource:
	54	+ def get_mirror_directory(self):
	55	+ return str(tmpdir.join("source").mkdir())
	56	+
	57	+ status = mock.MagicMock()
	58	+
	59	+ url = "file://{}/url".format(tmpdir)
	60	+
	61	+ # Create fake download directory
	62	+ download_dir = tmpdir.join("download_dir")
	63	+ download_dir.join("oracle").write("hello", ensure=True)
	64	+
	65	+ # Initiate mirror
	66	+ mirror = GitMirror(DummySource(), None, url, None)
	67	+
	68	+ # Setup destination directory
	69	+ setup(py.path.local(mirror.mirror))
	70	+
	71	+ # Copy directory content
	72	+ if type_ == "file":
	73	+ with pytest.raises(SourceError):
	74	+ mirror._atomic_move_mirror(str(download_dir), mirror.url)
	75	+ else:
	76	+ mirror._atomic_move_mirror(str(download_dir), mirror.url)
	77	+
	78	+ # Validate
	79	+ if type_ == "non-empty-dir":
	80	+ # With a non empty directory, we should not have overriden
	81	+ # and notified a status
	82	+ assert DummySource.status.called
	83	+
	84	+ expected_oracle = os.path.join(mirror.mirror, "bad")
	85	+ expected_content = "hi"
	86	+ elif type_ == "file":
	87	+ expected_oracle = mirror.mirror
	88	+ expected_content = "no"
	89	+ else:
	90	+ # Otherwise we should have a new directory and the data
	91	+ expected_oracle = os.path.join(mirror.mirror, "oracle")
	92	+ expected_content = "hello"
	93	+
	94	+ assert os.path.exists(expected_oracle)
	95	+
	96	+ with open(expected_oracle) as fp:
	97	+ assert fp.read() == expected_content
	98	+
	99	+
39	100	@pytest.mark.skipif(HAVE_GIT is False, reason="git is not available")
40	101	@pytest.mark.datafiles(os.path.join(DATA_DIR, 'template'))
41	102	def test_fetch_bad_ref(cli, tmpdir, datafiles):

—
View it on GitLab.
You're receiving this email because of your account on gitlab.com. If you'd like to receive fewer emails, you can adjust your notification settings.