Be explicit about git.Repo lifecycle

The git.Repo object creates a pair of long-running "git cat-file" processes
which it uses to read data from the git repo.  Most operations that involve
commits trigger the start of these processes.  They are terminated when
the git.Repo object is gc'd, or when it is "closed", where "closed" may
mean an explicit call to the close() method, or via the exit call when
used as a context manager.

To improve the likelihood that these processes are actually terminated and
not leaked, this change does the following:

1) Turns almost all usages of the git.Repo object into a context manager.
   (The exceptions are a couple of calls that are part of the initialization
   process and don't involve any commits).
2) Avoids returning commit objects from methods, instead returning the
   string hexshas of those commits.  All of the instances where we used
   a returned commit can be simplified to just use the hexsha.

Some unused methods are cleaned up as well.

Change-Id: I6cfac53f86498f26dc906ac35cccc18aa0e22699
This commit is contained in:
James E. Blair 2024-08-06 14:07:19 -07:00
parent cb50e819dd
commit 1fddbdb959
3 changed files with 413 additions and 401 deletions

View File

@ -264,7 +264,7 @@ class TestMergerRepo(ZuulTestCase):
'refs/remotes/origin/master': new_sha,
'refs/heads/broken': 'deadbeefdeadbeefdeadbeefdeadbeefdeadbeef',
})
self.assertEqual(work_repo.getBranchHead('master').hexsha, new_sha)
self.assertEqual(work_repo.getBranchHead('master'), new_sha)
self.assertIn('master', repo.remotes.origin.refs)
# Git tags have a special packed-refs format. Check that we can
@ -274,7 +274,7 @@ class TestMergerRepo(ZuulTestCase):
repo.git.describe('annotated')
work_repo.setRefs({'refs/heads/master': remote_sha})
self.assertEqual(work_repo.getBranchHead('master').hexsha, remote_sha)
self.assertEqual(work_repo.getBranchHead('master'), remote_sha)
self.assertNotIn('master', repo.remotes.origin.refs)
def test_set_remote_ref(self):

View File

@ -1556,14 +1556,14 @@ class AnsibleJob(object):
'BuildCheckout',
attributes={'connection': project['connection'],
'project': project['name']}):
commit = repo.checkout(selected_ref)
hexsha = repo.checkout(selected_ref)
# Update the inventory variables to indicate the ref we
# checked out
p = args['zuul']['projects'][project['canonical_name']]
p['checkout'] = selected_ref
p['checkout_description'] = selected_desc
p['commit'] = commit.hexsha
p['commit'] = hexsha
self.merge_ops.append(zuul.model.MergeOp(
cmd=['git', 'checkout', selected_ref],
path=repo.workspace_project_path))
@ -2340,13 +2340,13 @@ class AnsibleJob(object):
self.executor_server.merge_root,
logger=self.log,
scheme=zuul.model.SCHEME_GOLANG)
commit = merger.checkoutBranch(
hexsha = merger.checkoutBranch(
project.connection_name, project.name,
branch,
repo_state=self.repo_state,
process_worker=self.executor_server.process_worker,
zuul_event_id=self.zuul_event_id)
pi.commit = commit.hexsha
pi.commit = hexsha
else:
self.log.debug("Using existing repo %s@%s in trusted space %s",
project, branch, pi.root)
@ -2392,9 +2392,7 @@ class AnsibleJob(object):
# We call it a branch, but it can actually be any
# ref including a tag. Get the ref object so we
# can duplicate the full path.
ref_obj = repo.getRef(branch)
ref_path = ref_obj.path
ref_sha = ref_obj.commit.hexsha
ref_path, ref_sha = repo.getRef(branch)
repo_state = {
p['connection']: {
p['name']: {
@ -2418,12 +2416,12 @@ class AnsibleJob(object):
self.log.debug("Cloning %s@%s into new untrusted space %s",
project, branch, pi.root)
commit = merger.checkoutBranch(
hexsha = merger.checkoutBranch(
project.connection_name, project.name,
branch, repo_state=repo_state,
process_worker=self.executor_server.process_worker,
zuul_event_id=self.zuul_event_id)
pi.commit = commit.hexsha
pi.commit = hexsha
else:
self.log.debug("Using existing repo %s@%s in trusted space %s",
project, branch, pi.root)

File diff suppressed because it is too large Load Diff