From 794dbe7b9cb5475e6c98d5e51abe6b10a7c7151c Mon Sep 17 00:00:00 2001 From: Matthew Duggan Date: Mon, 18 Aug 2014 18:22:45 +0900 Subject: [PATCH 1/6] set callbacks just before fetch, clear them after (fixes #403). --- pygit2/decl.h | 2 ++ pygit2/remote.py | 44 ++++++++++++++++++++++++++------------------ 2 files changed, 28 insertions(+), 18 deletions(-) diff --git a/pygit2/decl.h b/pygit2/decl.h index 5573ecd83..0676b6796 100644 --- a/pygit2/decl.h +++ b/pygit2/decl.h @@ -12,6 +12,7 @@ typedef ... git_index_conflict_iterator; #define GIT_OID_RAWSZ ... #define GIT_PATH_MAX ... +#define GIT_REMOTE_CALLBACKS_VERSION ... typedef struct git_oid { unsigned char id[20]; @@ -136,6 +137,7 @@ int git_remote_add_push(git_remote *remote, const char *refspec); int git_remote_add_fetch(git_remote *remote, const char *refspec); int git_remote_save(const git_remote *remote); int git_remote_set_callbacks(git_remote *remote, const git_remote_callbacks *callbacks); +int git_remote_init_callbacks(git_remote_callbacks *opts, unsigned int version); size_t git_remote_refspec_count(git_remote *remote); const git_refspec * git_remote_get_refspec(git_remote *remote, size_t n); diff --git a/pygit2/remote.py b/pygit2/remote.py index b2fdec24d..dfeb4d705 100644 --- a/pygit2/remote.py +++ b/pygit2/remote.py @@ -127,20 +127,6 @@ def __init__(self, repo, ptr): self._remote = ptr self._stored_exception = None - # Build the callback structure - callbacks = ffi.new('git_remote_callbacks *') - callbacks.version = 1 - callbacks.sideband_progress = self._sideband_progress_cb - callbacks.transfer_progress = self._transfer_progress_cb - callbacks.update_tips = self._update_tips_cb - callbacks.credentials = self._credentials_cb - # We need to make sure that this handle stays alive - self._self_handle = ffi.new_handle(self) - callbacks.payload = self._self_handle - - err = C.git_remote_set_callbacks(self._remote, callbacks) - check_error(err) - def __del__(self): C.git_remote_free(self._remote) @@ -205,17 +191,39 @@ def fetch(self, signature=None, message=None): Perform a fetch against this remote. """ + defaultcallbacks = ffi.new('git_remote_callbacks *') + err = C.git_remote_init_callbacks(defaultcallbacks, + C.GIT_REMOTE_CALLBACKS_VERSION) + + # Build the callback structure + callbacks = ffi.new('git_remote_callbacks *') + callbacks.version = 1 + callbacks.sideband_progress = self._sideband_progress_cb + callbacks.transfer_progress = self._transfer_progress_cb + callbacks.update_tips = self._update_tips_cb + callbacks.credentials = self._credentials_cb + callbacks.payload = ffi.new_handle(self) + + err = C.git_remote_set_callbacks(self._remote, callbacks) + check_error(err) + if signature: ptr = signature._pointer[:] else: ptr = ffi.NULL self._stored_exception = None - err = C.git_remote_fetch(self._remote, ptr, to_bytes(message)) - if self._stored_exception: - raise self._stored_exception - check_error(err) + try: + err = C.git_remote_fetch(self._remote, ptr, to_bytes(message)) + if self._stored_exception: + raise self._stored_exception + + check_error(err) + finally: + # Even on error, clear stored callbacks and reset to default + err = C.git_remote_set_callbacks(self._remote, defaultcallbacks) + check_error(err) return TransferProgress(C.git_remote_stats(self._remote)) From 9ce6a26db33b6f46865f0878bf950624ed52b738 Mon Sep 17 00:00:00 2001 From: Matthew Duggan Date: Mon, 18 Aug 2014 18:23:10 +0900 Subject: [PATCH 2/6] Add some tests for refcounts to check for leaks --- test/test_commit.py | 10 ++++++++++ test/test_remote.py | 9 +++++++++ test/test_repository.py | 9 +++++++++ 3 files changed, 28 insertions(+) diff --git a/test/test_commit.py b/test/test_commit.py index f7a42b680..0ce52233f 100644 --- a/test/test_commit.py +++ b/test/test_commit.py @@ -30,6 +30,7 @@ from __future__ import absolute_import from __future__ import unicode_literals import unittest +import sys from pygit2 import GIT_OBJ_COMMIT, Signature, Oid from . import utils @@ -46,6 +47,15 @@ class CommitTest(utils.BareRepoTestCase): + def test_commit_refcount(self): + commit = self.repo[COMMIT_SHA] + start = sys.getrefcount(commit) + tree = commit.tree + del tree + end = sys.getrefcount(commit) + self.assertEqual(start, end) + + def test_read_commit(self): commit = self.repo[COMMIT_SHA] self.assertEqual(COMMIT_SHA, str(commit.id)) diff --git a/test/test_remote.py b/test/test_remote.py index 57814a386..aea1bb06d 100644 --- a/test/test_remote.py +++ b/test/test_remote.py @@ -30,6 +30,7 @@ import unittest import pygit2 +import sys from pygit2 import Oid from . import utils @@ -163,6 +164,14 @@ def test_remote_save(self): self.assertEqual('http://example.com/test.git', self.repo.remotes[0].url) + def test_remote_refcount(self): + start = sys.getrefcount(self.repo) + remote = self.repo.remotes[0] + del remote + end = sys.getrefcount(self.repo) + + self.assertEqual(start, end) + def test_add_refspec(self): remote = self.repo.create_remote('test_add_refspec', REMOTE_URL) remote.add_push('refs/heads/*:refs/heads/test_refspec/*') diff --git a/test/test_repository.py b/test/test_repository.py index cda846c08..53def2c22 100644 --- a/test/test_repository.py +++ b/test/test_repository.py @@ -37,6 +37,7 @@ import tempfile import os from os.path import join, realpath +import sys # Import from pygit2 from pygit2 import GIT_OBJ_ANY, GIT_OBJ_BLOB, GIT_OBJ_COMMIT @@ -150,6 +151,14 @@ def test_lookup_commit_prefix(self): commit.message) self.assertRaises(ValueError, self.repo.__getitem__, too_short_prefix) + def test_lookup_commit_refcount(self): + start = sys.getrefcount(self.repo) + commit_sha = '5fe808e8953c12735680c257f56600cb0de44b10' + commit = self.repo[commit_sha] + del commit + end = sys.getrefcount(self.repo) + self.assertEqual(start, end) + def test_get_path(self): directory = realpath(self.repo.path) expected = realpath(self.repo_path) From 99433ca66ab746ab1863e626eddf85ccab04c640 Mon Sep 17 00:00:00 2001 From: Matthew Duggan Date: Mon, 18 Aug 2014 21:12:34 +0900 Subject: [PATCH 3/6] Hard-code callback version because we'll need to change the defs if it changes. --- pygit2/decl.h | 1 - pygit2/remote.py | 7 ++++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pygit2/decl.h b/pygit2/decl.h index 0676b6796..4e3df66b5 100644 --- a/pygit2/decl.h +++ b/pygit2/decl.h @@ -12,7 +12,6 @@ typedef ... git_index_conflict_iterator; #define GIT_OID_RAWSZ ... #define GIT_PATH_MAX ... -#define GIT_REMOTE_CALLBACKS_VERSION ... typedef struct git_oid { unsigned char id[20]; diff --git a/pygit2/remote.py b/pygit2/remote.py index dfeb4d705..e147fc57f 100644 --- a/pygit2/remote.py +++ b/pygit2/remote.py @@ -191,11 +191,12 @@ def fetch(self, signature=None, message=None): Perform a fetch against this remote. """ + # Get the default callbacks first defaultcallbacks = ffi.new('git_remote_callbacks *') - err = C.git_remote_init_callbacks(defaultcallbacks, - C.GIT_REMOTE_CALLBACKS_VERSION) + err = C.git_remote_init_callbacks(defaultcallbacks, 1) + check_error(err) - # Build the callback structure + # Build custom callback structure callbacks = ffi.new('git_remote_callbacks *') callbacks.version = 1 callbacks.sideband_progress = self._sideband_progress_cb From bedd9ee315d1a6101b55e524097c8e820b49bb53 Mon Sep 17 00:00:00 2001 From: Matthew Duggan Date: Mon, 18 Aug 2014 21:13:03 +0900 Subject: [PATCH 4/6] Don't test refcounts in pypy as they don't make sense there --- test/test_commit.py | 1 + test/test_remote.py | 7 ++++++- test/test_repository.py | 6 ++++++ 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/test/test_commit.py b/test/test_commit.py index 0ce52233f..75470abc2 100644 --- a/test/test_commit.py +++ b/test/test_commit.py @@ -47,6 +47,7 @@ class CommitTest(utils.BareRepoTestCase): + @unittest.skipIf(__pypy__ is not None) def test_commit_refcount(self): commit = self.repo[COMMIT_SHA] start = sys.getrefcount(commit) diff --git a/test/test_remote.py b/test/test_remote.py index aea1bb06d..97cb72f48 100644 --- a/test/test_remote.py +++ b/test/test_remote.py @@ -34,6 +34,11 @@ from pygit2 import Oid from . import utils +try: + import __pypy__ +except ImportError: + __pypy__ = None + REMOTE_NAME = 'origin' REMOTE_URL = 'git://github.com/libgit2/pygit2.git' REMOTE_FETCHSPEC_SRC = 'refs/heads/*' @@ -164,12 +169,12 @@ def test_remote_save(self): self.assertEqual('http://example.com/test.git', self.repo.remotes[0].url) + @unittest.skipIf(__pypy__ is not None) def test_remote_refcount(self): start = sys.getrefcount(self.repo) remote = self.repo.remotes[0] del remote end = sys.getrefcount(self.repo) - self.assertEqual(start, end) def test_add_refspec(self): diff --git a/test/test_repository.py b/test/test_repository.py index 53def2c22..082876aa2 100644 --- a/test/test_repository.py +++ b/test/test_repository.py @@ -46,6 +46,11 @@ import pygit2 from . import utils +try: + import __pypy__ +except ImportError: + __pypy__ = None + HEAD_SHA = '784855caf26449a1914d2cf62d12b9374d76ae78' PARENT_SHA = 'f5e5aa4e36ab0fe62ee1ccc6eb8f79b866863b87' # HEAD^ @@ -151,6 +156,7 @@ def test_lookup_commit_prefix(self): commit.message) self.assertRaises(ValueError, self.repo.__getitem__, too_short_prefix) + @unittest.skipIf(__pypy__ is not None) def test_lookup_commit_refcount(self): start = sys.getrefcount(self.repo) commit_sha = '5fe808e8953c12735680c257f56600cb0de44b10' From 0813ec7923d2ff062dcf42f812e53ae48dca0364 Mon Sep 17 00:00:00 2001 From: Matthew Duggan Date: Mon, 18 Aug 2014 21:17:36 +0900 Subject: [PATCH 5/6] Fix use of skipIf. --- test/test_commit.py | 2 +- test/test_remote.py | 2 +- test/test_repository.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/test/test_commit.py b/test/test_commit.py index 75470abc2..00f81cd19 100644 --- a/test/test_commit.py +++ b/test/test_commit.py @@ -47,7 +47,7 @@ class CommitTest(utils.BareRepoTestCase): - @unittest.skipIf(__pypy__ is not None) + @unittest.skipIf(__pypy__ is not None, "skip refcounts checks in pypy") def test_commit_refcount(self): commit = self.repo[COMMIT_SHA] start = sys.getrefcount(commit) diff --git a/test/test_remote.py b/test/test_remote.py index 97cb72f48..d98527b64 100644 --- a/test/test_remote.py +++ b/test/test_remote.py @@ -169,7 +169,7 @@ def test_remote_save(self): self.assertEqual('http://example.com/test.git', self.repo.remotes[0].url) - @unittest.skipIf(__pypy__ is not None) + @unittest.skipIf(__pypy__ is not None, "skip refcounts checks in pypy") def test_remote_refcount(self): start = sys.getrefcount(self.repo) remote = self.repo.remotes[0] diff --git a/test/test_repository.py b/test/test_repository.py index 082876aa2..c37b1309f 100644 --- a/test/test_repository.py +++ b/test/test_repository.py @@ -156,7 +156,7 @@ def test_lookup_commit_prefix(self): commit.message) self.assertRaises(ValueError, self.repo.__getitem__, too_short_prefix) - @unittest.skipIf(__pypy__ is not None) + @unittest.skipIf(__pypy__ is not None, "skip refcounts checks in pypy") def test_lookup_commit_refcount(self): start = sys.getrefcount(self.repo) commit_sha = '5fe808e8953c12735680c257f56600cb0de44b10' From 2f2d4005c70eac2aefe66db19a0f7004ac765cbf Mon Sep 17 00:00:00 2001 From: Matthew Duggan Date: Mon, 25 Aug 2014 22:39:16 +0900 Subject: [PATCH 6/6] Ensure self handle stays alive - keeping it in callbacks is not enough. --- pygit2/remote.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/pygit2/remote.py b/pygit2/remote.py index e147fc57f..821c9fe5b 100644 --- a/pygit2/remote.py +++ b/pygit2/remote.py @@ -203,10 +203,15 @@ def fetch(self, signature=None, message=None): callbacks.transfer_progress = self._transfer_progress_cb callbacks.update_tips = self._update_tips_cb callbacks.credentials = self._credentials_cb - callbacks.payload = ffi.new_handle(self) + # We need to make sure that this handle stays alive + self._self_handle = ffi.new_handle(self) + callbacks.payload = self._self_handle err = C.git_remote_set_callbacks(self._remote, callbacks) - check_error(err) + try: + check_error(err) + finally: + self._self_handle = None if signature: ptr = signature._pointer[:] @@ -223,6 +228,7 @@ def fetch(self, signature=None, message=None): check_error(err) finally: # Even on error, clear stored callbacks and reset to default + self._self_handle = None err = C.git_remote_set_callbacks(self._remote, defaultcallbacks) check_error(err)