Refactor all callbacks (#700)

This change is a preparation for another change that makes all callback
types return a Go error instead of an error code / an integer. That is
going to make make things a lot more idiomatic.

The reason this change is split is threefold:

a) This change is mostly mechanical and should contain no semantic
   changes.
b) This change is backwards-compatible (in the Go API compatibility
   sense of the word), and thus can be backported to all other releases.
c) It makes the other change a bit smaller and more focused on just one
   thing.

Concretely, this change makes all callbacks populate a Go error when
they fail. If the callback is invoked from the same stack as the
function to which it was passed (e.g. for `Tree.Walk`), it will preserve
the error object directly into a struct that also holds the callback
function. Otherwise if the callback is pased to one func and will be
invoked when run from another one (e.g. for `Repository.InitRebase`),
the error string is saved into the libgit2 thread-local storage and then
re-created as a `GitError`.

1 files changed, 52 insertions, 27 deletions

diff --git a/clone.go b/clone.go
index 26251c8..8166b08 100644
--- a/clone.go
+++ b/clone.go
@@ -3,10 +3,11 @@ package git
 /*
 #include <git2.h>
 
-extern void _go_git_populate_remote_cb(git_clone_options *opts);
+extern void _go_git_populate_clone_callbacks(git_clone_options *opts);
 */
 import "C"
 import (
+	"errors"
 	"runtime"
 	"unsafe"
 )
@@ -28,20 +29,23 @@ func Clone(url string, path string, options *CloneOptions) (*Repository, error)
 	cpath := C.CString(path)
 	defer C.free(unsafe.Pointer(cpath))
 
-	copts := (*C.git_clone_options)(C.calloc(1, C.size_t(unsafe.Sizeof(C.git_clone_options{}))))
-	populateCloneOptions(copts, options)
-	defer freeCloneOptions(copts)
+	var err error
+	cOptions := populateCloneOptions(&C.git_clone_options{}, options, &err)
+	defer freeCloneOptions(cOptions)
 
 	if len(options.CheckoutBranch) != 0 {
-		copts.checkout_branch = C.CString(options.CheckoutBranch)
+		cOptions.checkout_branch = C.CString(options.CheckoutBranch)
 	}
 
 	runtime.LockOSThread()
 	defer runtime.UnlockOSThread()
 
 	var ptr *C.git_repository
-	ret := C.git_clone(&ptr, curl, cpath, copts)
+	ret := C.git_clone(&ptr, curl, cpath, cOptions)
 
+	if ret == C.int(ErrorCodeUser) && err != nil {
+		return nil, err
+	}
 	if ret < 0 {
 		return nil, MakeGitError(ret)
 	}
@@ -50,47 +54,69 @@ func Clone(url string, path string, options *CloneOptions) (*Repository, error)
 }
 
 //export remoteCreateCallback
-func remoteCreateCallback(cremote unsafe.Pointer, crepo unsafe.Pointer, cname, curl *C.char, payload unsafe.Pointer) C.int {
+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 := 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)
 
-	if opts, ok := pointerHandles.Get(payload).(CloneOptions); ok {
-		remote, errorCode := opts.RemoteCreateCallback(repo, name, url)
-		// clear finalizer as the calling C function will
-		// free the remote itself
-		runtime.SetFinalizer(remote, nil)
+	data, ok := pointerHandles.Get(payload).(*cloneCallbackData)
+	if !ok {
+		panic("invalid remote create callback")
+	}
 
-		if errorCode == ErrorCodeOK && remote != nil {
-			cptr := (**C.git_remote)(cremote)
-			*cptr = remote.ptr
-		} else if errorCode == ErrorCodeOK && remote == nil {
-			panic("no remote created by 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)
 
-		return C.int(errorCode)
-	} else {
-		panic("invalid remote create callback")
+	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
+
+	return C.int(ErrorCodeOK)
+}
+
+type cloneCallbackData struct {
+	options     *CloneOptions
+	errorTarget *error
 }
 
-func populateCloneOptions(ptr *C.git_clone_options, opts *CloneOptions) {
+func populateCloneOptions(ptr *C.git_clone_options, opts *CloneOptions, errorTarget *error) *C.git_clone_options {
 	C.git_clone_options_init(ptr, C.GIT_CLONE_OPTIONS_VERSION)
 
 	if opts == nil {
-		return
+		return nil
 	}
-	populateCheckoutOptions(&ptr.checkout_opts, opts.CheckoutOpts)
+	populateCheckoutOptions(&ptr.checkout_opts, opts.CheckoutOpts, errorTarget)
 	populateFetchOptions(&ptr.fetch_opts, opts.FetchOptions)
 	ptr.bare = cbool(opts.Bare)
 
 	if opts.RemoteCreateCallback != nil {
+		data := &cloneCallbackData{
+			options:     opts,
+			errorTarget: errorTarget,
+		}
 		// 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)
+		C._go_git_populate_clone_callbacks(ptr)
+		ptr.remote_cb_payload = pointerHandles.Track(data)
 	}
+
+	return ptr
 }
 
 func freeCloneOptions(ptr *C.git_clone_options) {
@@ -105,5 +131,4 @@ func freeCloneOptions(ptr *C.git_clone_options) {
 	}
 
 	C.free(unsafe.Pointer(ptr.checkout_branch))
-	C.free(unsafe.Pointer(ptr))
 }