PR#2243: Lock the git repo when removing elements from it - pagure

pagure

#2243 Lock the git repo when removing elements from it

Merged 6 years ago by pingou. Opened 7 years ago by pingou.

fix_lock into master

Add a check that the file lock is being called

Pierre-Yves Chibon • 6 years ago

3c5702a

Always remove the lockfile after using it, just check if it is still present

Pierre-Yves Chibon • 6 years ago

774fee0

Lock the git repo when removing elements from it

Pierre-Yves Chibon • 6 years ago

eccb1c1

pagure/lib/git.py

file modified

+66 -46

		`@@ -259,7 +259,9 @@`
		`# If not change, return`
		`if not files and not added:`
		`shutil.rmtree(newpath)`
		`- os.unlink(lockfile)`
		`+ if os.path.exists(lockfile):`
		`+ # Remove the lock file`
		`+ os.unlink(lockfile)`
		`return`

		`# See if there is a parent to this commit`
		`@@ -296,8 +298,9 @@`
		`# Remove the clone`
		`shutil.rmtree(newpath)`

		`- # Remove the lock file`
		`- os.unlink(lockfile)`
		`+ if os.path.exists(lockfile):`
		`+ # Remove the lock file`
		`+ os.unlink(lockfile)`


		`def clean_git(obj, repo, repofolder):`
		`@@ -312,58 +315,66 @@`

		`# Get the fork`
		`repopath = os.path.join(repofolder, repo.path)`
		`+ lockfile = '%s.lock' % repopath`

		`- # Clone the repo into a temp folder`
		`- newpath = tempfile.mkdtemp(prefix='pagure-')`
		`- new_repo = pygit2.clone_repository(repopath, newpath)`
		`+ lock = filelock.FileLock(lockfile)`
		`+ with lock:`

		`- file_path = os.path.join(newpath, obj.uid)`
		`+ # Clone the repo into a temp folder`
		`+ newpath = tempfile.mkdtemp(prefix='pagure-')`
		`+ new_repo = pygit2.clone_repository(repopath, newpath)`

		`- # Get the current index`
		`- index = new_repo.index`
		`+ file_path = os.path.join(newpath, obj.uid)`

		`- # Are we adding files`
		`- if not os.path.exists(file_path):`
		`- shutil.rmtree(newpath)`
		`- return`
		`+ # Get the current index`
		`+ index = new_repo.index`

		`- # Remove the file`
		`- os.unlink(file_path)`
		`+ # Are we adding files`
		`+ if not os.path.exists(file_path):`
		`+ shutil.rmtree(newpath)`
		`+ return`
		`+`
		`+ # Remove the file`
		`+ os.unlink(file_path)`

		`- # Add the changes to the index`
		`- index.remove(obj.uid)`
		`+ # Add the changes to the index`
		`+ index.remove(obj.uid)`

		`- # See if there is a parent to this commit`
		`- parent = None`
		`- if not new_repo.is_empty:`
		`- parent = new_repo.head.get_object().oid`
		`+ # See if there is a parent to this commit`
		`+ parent = None`
		`+ if not new_repo.is_empty:`
		`+ parent = new_repo.head.get_object().oid`
		`+`
		`+ parents = []`
		`+ if parent:`
		`+ parents.append(parent)`

		`- parents = []`
		`- if parent:`
		`- parents.append(parent)`
		`+ # Author/commiter will always be this one`
		`+ author = pygit2.Signature(name='pagure', email='pagure')`

		`- # Author/commiter will always be this one`
		`- author = pygit2.Signature(name='pagure', email='pagure')`
		`+ # Actually commit`
		`+ new_repo.create_commit(`
		`+ 'refs/heads/master',`
		`+ author,`
		`+ author,`
		`+ 'Removed %s %s: %s' % (obj.isa, obj.uid, obj.title),`
		`+ new_repo.index.write_tree(),`
		`+ parents)`
		`+ index.write()`

		`- # Actually commit`
		`- new_repo.create_commit(`
		`- 'refs/heads/master',`
		`- author,`
		`- author,`
		`- 'Removed %s %s: %s' % (obj.isa, obj.uid, obj.title),`
		`- new_repo.index.write_tree(),`
		`- parents)`
		`- index.write()`
		`+ # Push to origin`
		`+ ori_remote = new_repo.remotes[0]`
		`+ master_ref = new_repo.lookup_reference('HEAD').resolve()`
		`+ refname = '%s:%s' % (master_ref.name, master_ref.name)`

		`- # Push to origin`
		`- ori_remote = new_repo.remotes[0]`
		`- master_ref = new_repo.lookup_reference('HEAD').resolve()`
		`- refname = '%s:%s' % (master_ref.name, master_ref.name)`
		`+ PagureRepo.push(ori_remote, refname)`

		`- PagureRepo.push(ori_remote, refname)`
		`+ # Remove the clone`
		`+ shutil.rmtree(newpath)`

		`- # Remove the clone`
		`- shutil.rmtree(newpath)`
		`+ if os.path.exists(lockfile):`
		`+ # Remove the lock file`
		`+ os.unlink(lockfile)`


		`def get_user_from_json(session, jsondata, key='user'):`
		`@@ -913,6 +924,10 @@`
		`# Remove the clone`
		`shutil.rmtree(newpath)`

		`+ if os.path.exists(lockfile):`
		`+ # Remove the lock file`
		`+ os.unlink(lockfile)`
		`+`
		`return os.path.join('files', filename)`


		`@@ -970,7 +985,9 @@`
		`# If not change, return`
		`if not files and not added:`
		`shutil.rmtree(newpath)`
		`- os.unlink(lockfile)`
		`+ if os.path.exists(lockfile):`
		`+ # Remove the lock file`
		`+ os.unlink(lockfile)`
		`return`

		`# See if there is a parent to this commit`
		`@@ -1011,7 +1028,9 @@`
		`try:`
		`PagureRepo.push(ori_remote, refname)`
		`except pygit2.GitError as err: # pragma: no cover`
		`- os.unlink(lockfile)`
		`+ if os.path.exists(lockfile):`
		`+ # Remove the lock file`
		`+ os.unlink(lockfile)`
		`shutil.rmtree(newpath)`
		`raise pagure.exceptions.PagureException(`
		`'Commit could not be done: %s' % err)`
		`@@ -1019,8 +1038,9 @@`
		`# Remove the clone`
		`shutil.rmtree(newpath)`

		`- # Remove the lock file`
		`- os.unlink(lockfile)`
		`+ if os.path.exists(lockfile):`
		`+ # Remove the lock file`
		`+ os.unlink(lockfile)`

		`return os.path.join('files', filename)`

tests/test_pagure_lib_git.py

file modified

+6 -1

		`@@ -1517,11 +1517,13 @@`
		`#print patch`
		`self.assertEqual(patch, exp)`

		`- def test_clean_git(self):`
		`+ @patch('filelock.FileLock')`
		`+ def test_clean_git(self, mock_fl):`
		`""" Test the clean_git method of pagure.lib.git. """`
		`pagure.lib.git.clean_git(None, None, None)`

		`self.test_update_git()`
		`+ self.assertEqual(mock_fl.call_count, 3)`

		`gitpath = os.path.join(self.path, 'test_ticket_repo.git')`
		`gitrepo = pygit2.init_repository(gitpath, bare=True)`
		`@@ -1543,6 +1545,9 @@`
		`issue = pagure.lib.search_issues(self.session, repo, issueid=1)`
		`pagure.lib.git.clean_git(issue, repo, self.path)`

		`+ # 4 times: 3 in test_update_git + 1 here`
		`+ self.assertEqual(mock_fl.call_count, 4)`
		`+`
		`# No more files in the git repo`
		`commit = gitrepo.revparse_single('HEAD')`
		`files = [entry.name for entry in commit.tree]`

pingou commented 7 years ago

This to ensure there are no concurrent writing to the repo which could
create dangling commits.

Signed-off-by: Pierre-Yves Chibon pingou@pingoured.fr

pingou commented 7 years ago

Pretty please pagure-ci rebuild

bowlofeggs commented 7 years ago

I recommend testing this change. If anything, you could just verify that a lock is used by mocking the lock and ensuring that its enter and exit calls happen.

bowlofeggs commented 7 years ago

LGTM though (sorry forgot to mention that ☺)

rebased

7 years ago

2 new commits added