clone: improve handling of remote create callback

The clone options contain fields for ae remote create callback
and its payload, which can be used to override the behavior when
the default remote is being created for newly cloned
repositories.

Currently we only accept a C function as callback, though, making
it overly complicated to use it. We also unconditionally `free`
the payload if its address is non-`nil`, which may cause the
program to segfault when the memory is not dynamically allocated.

Instead, we want callers to provide a Go function that is
subsequently being called by us. To do this, we introduce an
indirection such that we are able to extract the provided
function and payload when being called by `git_clone` and handle
the return values of the user-provided function.

Author: pks-t

clone.go

@@ -3,20 +3,22 @@ package git
 /*
 #include <git2.h>
 
+extern void _go_git_populate_remote_cb(git_clone_options *opts);
 */
 import "C"
 import (
 	"runtime"
 	"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))
 }

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")
+	}
+}

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);