diff --git a/clone.go b/clone.go index 4a3c3d1..ba208bc 100644 --- a/clone.go +++ b/clone.go @@ -55,38 +55,36 @@ func Clone(url string, path string, options *CloneOptions) (*Repository, error) //export remoteCreateCallback func remoteCreateCallback( - cremote unsafe.Pointer, - crepo unsafe.Pointer, + out **C.git_remote, + crepo *C.git_repository, cname, curl *C.char, - payload unsafe.Pointer, + handle unsafe.Pointer, ) C.int { name := C.GoString(cname) url := C.GoString(curl) - repo := newRepositoryFromC((*C.git_repository)(crepo)) - // We don't own this repository, so make sure we don't try to free it - runtime.SetFinalizer(repo, nil) + repo := newRepositoryFromC(crepo) + repo.weak = true + defer repo.Free() - data, ok := pointerHandles.Get(payload).(*cloneCallbackData) + data, ok := pointerHandles.Get(handle).(*cloneCallbackData) if !ok { panic("invalid remote create callback") } remote, ret := data.options.RemoteCreateCallback(repo, name, url) - // clear finalizer as the calling C function will - // free the remote itself - runtime.SetFinalizer(remote, nil) - if ret < 0 { *data.errorTarget = errors.New(ErrorCode(ret).String()) return C.int(ErrorCodeUser) } - if remote == nil { panic("no remote created by callback") } - cptr := (**C.git_remote)(cremote) - *cptr = remote.ptr + *out = remote.ptr + + // clear finalizer as the calling C function will + // free the remote itself + runtime.SetFinalizer(remote, nil) return C.int(ErrorCodeOK) } diff --git a/clone_test.go b/clone_test.go index ded9847..acfbbcb 100644 --- a/clone_test.go +++ b/clone_test.go @@ -74,4 +74,5 @@ func TestCloneWithCallback(t *testing.T) { if err != nil || remote == nil { t.Fatal("Remote was not created properly") } + defer remote.Free() } diff --git a/git_test.go b/git_test.go index 1bd311a..1c57f79 100644 --- a/git_test.go +++ b/git_test.go @@ -1,13 +1,33 @@ package git import ( + "fmt" "io/ioutil" "os" "path" + "reflect" "testing" "time" ) +func TestMain(m *testing.M) { + ret := m.Run() + + // Ensure that we are not leaking any pointer handles. + pointerHandles.Lock() + if len(pointerHandles.handles) > 0 { + for h, ptr := range pointerHandles.handles { + fmt.Printf("%016p: %v %+v\n", h, reflect.TypeOf(ptr), ptr) + } + panic("pointer handle list not empty") + } + pointerHandles.Unlock() + + Shutdown() + + os.Exit(ret) +} + func cleanupTestRepo(t *testing.T, r *Repository) { var err error if r.IsBare() { diff --git a/object.go b/object.go index e7d3890..66e848f 100644 --- a/object.go +++ b/object.go @@ -77,14 +77,14 @@ func (o *Object) Type() ObjectType { return ret } -// Owner returns a weak reference to the repository which owns this -// object. This won't keep the underlying repository alive. +// Owner returns a weak reference to the repository which owns this object. +// This won't keep the underlying repository alive, but it should still be +// Freed. func (o *Object) Owner() *Repository { - ret := &Repository{ - ptr: C.git_object_owner(o.ptr), - } + repo := newRepositoryFromC(C.git_object_owner(o.ptr)) runtime.KeepAlive(o) - return ret + repo.weak = true + return repo } func dupObject(obj *Object, kind ObjectType) (*C.git_object, error) { diff --git a/push_test.go b/push_test.go index f372882..311b86d 100644 --- a/push_test.go +++ b/push_test.go @@ -14,15 +14,18 @@ func TestRemotePush(t *testing.T) { remote, err := localRepo.Remotes.Create("test_push", repo.Path()) checkFatal(t, err) + defer remote.Free() seedTestRepo(t, localRepo) err = remote.Push([]string{"refs/heads/master"}, nil) checkFatal(t, err) - _, err = localRepo.References.Lookup("refs/remotes/test_push/master") + ref, err := localRepo.References.Lookup("refs/remotes/test_push/master") checkFatal(t, err) + defer ref.Free() - _, err = repo.References.Lookup("refs/heads/master") + ref, err = repo.References.Lookup("refs/heads/master") checkFatal(t, err) + defer ref.Free() } diff --git a/reference.go b/reference.go index 02022a4..d15962e 100644 --- a/reference.go +++ b/reference.go @@ -293,12 +293,14 @@ func (v *Reference) Peel(t ObjectType) (*Object, error) { return allocObject(cobj, v.repo), nil } -// Owner returns a weak reference to the repository which owns this -// reference. +// Owner returns a weak reference to the repository which owns this reference. +// This won't keep the underlying repository alive, but it should still be +// Freed. func (v *Reference) Owner() *Repository { - return &Repository{ - ptr: C.git_reference_owner(v.ptr), - } + repo := newRepositoryFromC(C.git_reference_owner(v.ptr)) + runtime.KeepAlive(v) + repo.weak = true + return repo } // Cmp compares v to ref2. It returns 0 on equality, otherwise a diff --git a/remote.go b/remote.go index 1e6a0f9..37fbc9a 100644 --- a/remote.go +++ b/remote.go @@ -432,10 +432,12 @@ func RemoteIsValidName(name string) bool { return C.git_remote_is_valid_name(cname) == 1 } +// Free releases the resources of the Remote. func (r *Remote) Free() { runtime.SetFinalizer(r, nil) C.git_remote_free(r.ptr) r.ptr = nil + r.repo = nil } type RemoteCollection struct { diff --git a/remote_test.go b/remote_test.go index 4f10c0d..ccd436a 100644 --- a/remote_test.go +++ b/remote_test.go @@ -23,9 +23,9 @@ func TestListRemotes(t *testing.T) { repo := createTestRepo(t) defer cleanupTestRepo(t, repo) - _, err := repo.Remotes.Create("test", "git://foo/bar") - + remote, err := repo.Remotes.Create("test", "git://foo/bar") checkFatal(t, err) + defer remote.Free() expected := []string{ "test", @@ -53,6 +53,7 @@ func TestCertificateCheck(t *testing.T) { remote, err := repo.Remotes.Create("origin", "https://github.com/libgit2/TestGitRepository") checkFatal(t, err) + defer remote.Free() options := FetchOptions{ RemoteCallbacks: RemoteCallbacks{ @@ -73,6 +74,7 @@ func TestRemoteConnect(t *testing.T) { remote, err := repo.Remotes.Create("origin", "https://github.com/libgit2/TestGitRepository") checkFatal(t, err) + defer remote.Free() err = remote.ConnectFetch(nil, nil, nil) checkFatal(t, err) @@ -85,6 +87,7 @@ func TestRemoteLs(t *testing.T) { remote, err := repo.Remotes.Create("origin", "https://github.com/libgit2/TestGitRepository") checkFatal(t, err) + defer remote.Free() err = remote.ConnectFetch(nil, nil, nil) checkFatal(t, err) @@ -104,6 +107,7 @@ func TestRemoteLsFiltering(t *testing.T) { remote, err := repo.Remotes.Create("origin", "https://github.com/libgit2/TestGitRepository") checkFatal(t, err) + defer remote.Free() err = remote.ConnectFetch(nil, nil, nil) checkFatal(t, err) @@ -136,11 +140,13 @@ func TestRemotePruneRefs(t *testing.T) { err = config.SetBool("remote.origin.prune", true) checkFatal(t, err) - _, err = repo.Remotes.Create("origin", "https://github.com/libgit2/TestGitRepository") + remote, err := repo.Remotes.Create("origin", "https://github.com/libgit2/TestGitRepository") checkFatal(t, err) + defer remote.Free() - remote, err := repo.Remotes.Lookup("origin") + remote, err = repo.Remotes.Lookup("origin") checkFatal(t, err) + defer remote.Free() if !remote.PruneRefs() { t.Fatal("Expected remote to be configured to prune references") @@ -159,6 +165,7 @@ func TestRemotePrune(t *testing.T) { remoteRef, err := remoteRepo.CreateBranch("test-prune", commit, true) checkFatal(t, err) + defer remoteRef.Free() repo := createTestRepo(t) defer cleanupTestRepo(t, repo) @@ -170,12 +177,14 @@ func TestRemotePrune(t *testing.T) { remoteUrl := fmt.Sprintf("file://%s", remoteRepo.Workdir()) remote, err := repo.Remotes.Create("origin", remoteUrl) checkFatal(t, err) + defer remote.Free() err = remote.Fetch([]string{"test-prune"}, nil, "") checkFatal(t, err) - _, err = repo.References.Create("refs/remotes/origin/test-prune", head, true, "remote reference") + ref, err := repo.References.Create("refs/remotes/origin/test-prune", head, true, "remote reference") checkFatal(t, err) + defer ref.Free() err = remoteRef.Delete() checkFatal(t, err) @@ -185,6 +194,7 @@ func TestRemotePrune(t *testing.T) { rr, err := repo.Remotes.Lookup("origin") checkFatal(t, err) + defer rr.Free() err = rr.ConnectFetch(nil, nil, nil) checkFatal(t, err) @@ -192,8 +202,9 @@ func TestRemotePrune(t *testing.T) { err = rr.Prune(nil) checkFatal(t, err) - _, err = repo.References.Lookup("refs/remotes/origin/test-prune") + ref, err = repo.References.Lookup("refs/remotes/origin/test-prune") if err == nil { + ref.Free() t.Fatal("Expected error getting a pruned reference") } } diff --git a/repository.go b/repository.go index fdd38f6..42e74a0 100644 --- a/repository.go +++ b/repository.go @@ -35,6 +35,10 @@ type Repository struct { // Stashes represents the collection of stashes and can be used to // save, apply and iterate over stash states in this repository. Stashes StashCollection + + // weak indicates that a repository is a weak pointer and should not be + // freed. + weak bool } func newRepositoryFromC(ptr *C.git_repository) *Repository { @@ -136,8 +140,13 @@ func (v *Repository) SetRefdb(refdb *Refdb) { } func (v *Repository) Free() { + ptr := v.ptr + v.ptr = nil runtime.SetFinalizer(v, nil) - C.git_repository_free(v.ptr) + if v.weak { + return + } + C.git_repository_free(ptr) } func (v *Repository) Config() (*Config, error) {