Merge pull request #242 from pks-t/fix-populate-clone-options

clone: do not free clone options' payload

3 files changed, 97 insertions, 12 deletions

diff --git a/clone.go b/clone.go
index 4de4aea..1265d7f 100644
--- a/clone.go
+++ b/clone.go
@@ -3,6 +3,7 @@ package git
 /*
 #include <git2.h>
 
+extern void _go_git_populate_remote_cb(git_clone_options *opts);
 */
 import "C"
 import (
@@ -10,13 +11,14 @@ import (
 	"unsafe"
 )
 
+type RemoteCreateCallback func(repo Repository, name, url string) (*Remote, ErrorCode)
+
 type CloneOptions struct {
 	*CheckoutOpts
 	*RemoteCallbacks
 	Bare                 bool
 	CheckoutBranch       string
-	RemoteCreateCallback C.git_remote_create_cb
-	RemoteCreatePayload  unsafe.Pointer
+	RemoteCreateCallback RemoteCreateCallback
 }
 
 func Clone(url string, path string, options *CloneOptions) (*Repository, error) {
@@ -30,6 +32,7 @@ func Clone(url string, path string, options *CloneOptions) (*Repository, error)
 
 	copts := (*C.git_clone_options)(C.calloc(1, C.size_t(unsafe.Sizeof(C.git_clone_options{}))))
 	populateCloneOptions(copts, options)
+	defer freeCloneOptions(copts)
 
 	if len(options.CheckoutBranch) != 0 {
 		copts.checkout_branch = C.CString(options.CheckoutBranch)
@@ -38,9 +41,6 @@ func Clone(url string, path string, options *CloneOptions) (*Repository, error)
 	runtime.LockOSThread()
 	defer runtime.UnlockOSThread()
 	ret := C.git_clone(&repo.ptr, curl, cpath, copts)
-	freeCheckoutOpts(&copts.checkout_opts)
-	C.free(unsafe.Pointer(copts.checkout_branch))
-	C.free(unsafe.Pointer(copts))
 
 	if ret < 0 {
 		return nil, MakeGitError(ret)
@@ -50,6 +50,32 @@ func Clone(url string, path string, options *CloneOptions) (*Repository, error)
 	return repo, nil
 }
 
+//export remoteCreateCallback
+func remoteCreateCallback(cremote unsafe.Pointer, crepo unsafe.Pointer, cname, curl *C.char, payload unsafe.Pointer) C.int {
+	name := C.GoString(cname)
+	url := C.GoString(curl)
+	repo := Repository{(*C.git_repository)(crepo)}
+
+	if opts, ok := pointerHandles.Get(payload).(CloneOptions); ok {
+		remote, err := opts.RemoteCreateCallback(repo, name, url)
+
+		if err == ErrOk && remote != nil {
+			// clear finalizer as the calling C function will
+			// free the remote itself
+			runtime.SetFinalizer(remote, nil)
+
+			cptr := (**C.git_remote)(cremote)
+			*cptr = remote.ptr
+		} else if err == ErrOk && remote == nil {
+			panic("no remote created by callback")
+		}
+
+		return C.int(err)
+	} else {
+		panic("invalid remote create callback")
+	}
+}
+
 func populateCloneOptions(ptr *C.git_clone_options, opts *CloneOptions) {
 	C.git_clone_init_options(ptr, C.GIT_CLONE_OPTIONS_VERSION)
 
@@ -61,12 +87,23 @@ func populateCloneOptions(ptr *C.git_clone_options, opts *CloneOptions) {
 	ptr.bare = cbool(opts.Bare)
 
 	if opts.RemoteCreateCallback != nil {
-		ptr.remote_cb = opts.RemoteCreateCallback
-		defer C.free(unsafe.Pointer(opts.RemoteCreateCallback))
+		// Go v1.1 does not allow to assign a C function pointer
+		C._go_git_populate_remote_cb(ptr)
+		ptr.remote_cb_payload = pointerHandles.Track(*opts)
+	}
+}
 
-		if opts.RemoteCreatePayload != nil {
-			ptr.remote_cb_payload = opts.RemoteCreatePayload
-			defer C.free(opts.RemoteCreatePayload)
-		}
+func freeCloneOptions(ptr *C.git_clone_options) {
+	if ptr == nil {
+		return
 	}
+
+	freeCheckoutOpts(&ptr.checkout_opts)
+
+	if ptr.remote_cb_payload != nil {
+		pointerHandles.Untrack(ptr.remote_cb_payload)
+	}
+
+	C.free(unsafe.Pointer(ptr.checkout_branch))
+	C.free(unsafe.Pointer(ptr))
 }
diff --git a/clone_test.go b/clone_test.go
index fd83fec..86fced8 100644
--- a/clone_test.go
+++ b/clone_test.go
@@ -5,8 +5,11 @@ import (
 	"testing"
 )
 
-func TestClone(t *testing.T) {
+const (
+	REMOTENAME = "testremote"
+)
 
+func TestClone(t *testing.T) {
 	repo := createTestRepo(t)
 	defer cleanupTestRepo(t, repo)
 
@@ -20,3 +23,43 @@ func TestClone(t *testing.T) {
 
 	checkFatal(t, err)
 }
+
+func TestCloneWithCallback(t *testing.T) {
+	testPayload := 0
+
+	repo := createTestRepo(t)
+	defer cleanupTestRepo(t, repo)
+
+	seedTestRepo(t, repo)
+
+	path, err := ioutil.TempDir("", "git2go")
+	checkFatal(t, err)
+
+	opts := CloneOptions{
+		Bare: true,
+		RemoteCreateCallback: func(r Repository, name, url string) (*Remote, ErrorCode) {
+			testPayload += 1
+
+			remote, err := r.CreateRemote(REMOTENAME, url)
+			if err != nil {
+				return nil, ErrGeneric
+			}
+
+			return remote, ErrOk
+		},
+	}
+
+	repo2, err := Clone(repo.Path(), path, &opts)
+	defer cleanupTestRepo(t, repo2)
+
+	checkFatal(t, err)
+
+	if testPayload != 1 {
+		t.Fatal("Payload's value has not been changed")
+	}
+
+	remote, err := repo2.LookupRemote(REMOTENAME)
+	if err != nil || remote == nil {
+		t.Fatal("Remote was not created properly")
+	}
+}
diff --git a/wrapper.c b/wrapper.c
index 017168d..3b88f93 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -5,6 +5,11 @@
 
 typedef int (*gogit_submodule_cbk)(git_submodule *sm, const char *name, void *payload);
 
+void _go_git_populate_remote_cb(git_clone_options *opts)
+{
+	opts->remote_cb = (git_remote_create_cb)remoteCreateCallback;
+}
+
 int _go_git_visit_submodule(git_repository *repo, void *fct)
 {
 	  return git_submodule_foreach(repo, (gogit_submodule_cbk)&SubmoduleVisitor, fct);