From eccb1c1ca8290a0ab438bec4f82d8e4f3a735541 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Apr 30 2017 15:31:50 +0000 Subject: [PATCH 1/3] Lock the git repo when removing elements from it This to ensure there are no concurrent writing to the repo which could create dangling commits. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/git.py b/pagure/lib/git.py index 50f9237..3cb76a7 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -312,58 +312,62 @@ def clean_git(obj, repo, repofolder): # 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) def get_user_from_json(session, jsondata, key='user'): From 774fee05097be77296fca32cda68028c3fd509e5 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Apr 30 2017 15:31:50 +0000 Subject: [PATCH 2/3] Always remove the lockfile after using it, just check if it is still present Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/git.py b/pagure/lib/git.py index 3cb76a7..8d4f8ae 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -259,7 +259,9 @@ def update_git(obj, repo, repofolder): # 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 @@ def update_git(obj, repo, repofolder): # 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): @@ -369,6 +372,10 @@ def clean_git(obj, repo, repofolder): # 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'): """ From the given json blob, retrieve the user info and search for it @@ -917,6 +924,10 @@ def add_file_to_git(repo, issue, ticketfolder, user, filename, filestream): # Remove the clone shutil.rmtree(newpath) + if os.path.exists(lockfile): + # Remove the lock file + os.unlink(lockfile) + return os.path.join('files', filename) @@ -974,7 +985,9 @@ def update_file_in_git( # 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 @@ -1015,7 +1028,9 @@ def update_file_in_git( 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) @@ -1023,8 +1038,9 @@ def update_file_in_git( # 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) From 3c5702ac14a7121371a7b1d9e2e38309166a5606 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Apr 30 2017 15:31:50 +0000 Subject: [PATCH 3/3] Add a check that the file lock is being called Signed-off-by: Pierre-Yves Chibon --- diff --git a/tests/test_pagure_lib_git.py b/tests/test_pagure_lib_git.py index f53abe8..b05e95d 100644 --- a/tests/test_pagure_lib_git.py +++ b/tests/test_pagure_lib_git.py @@ -1517,11 +1517,13 @@ index 458821a..77674a8 #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 @@ index 458821a..77674a8 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]