Refactor all callbacks (#700) (#704)

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`.

(cherry picked from commit 5d8eaf7)

Author: lhchavez

checkout.go

@@ -3,10 +3,11 @@ package git
 /*
 #include <git2.h>
 
-extern void _go_git_populate_checkout_cb(git_checkout_options *opts);
+extern void _go_git_populate_checkout_callbacks(git_checkout_options *opts);
 */
 import "C"
 import (
+	"errors"
 	"os"
 	"runtime"
 	"unsafe"
@@ -75,30 +76,37 @@ func checkoutOptionsFromC(c *C.git_checkout_options) CheckoutOptions {
 		NotifyFlags:    CheckoutNotifyType(c.notify_flags),
 	}
 	if c.notify_payload != nil {
-		opts.NotifyCallback = pointerHandles.Get(c.notify_payload).(*CheckoutOptions).NotifyCallback
+		opts.NotifyCallback = pointerHandles.Get(c.notify_payload).(*checkoutCallbackData).options.NotifyCallback
 	}
 	if c.progress_payload != nil {
-		opts.ProgressCallback = pointerHandles.Get(c.progress_payload).(*CheckoutOptions).ProgressCallback
+		opts.ProgressCallback = pointerHandles.Get(c.progress_payload).(*checkoutCallbackData).options.ProgressCallback
 	}
 	if c.target_directory != nil {
 		opts.TargetDirectory = C.GoString(c.target_directory)
 	}
 	return opts
 }
 
-func (opts *CheckoutOptions) toC() *C.git_checkout_options {
+func (opts *CheckoutOptions) toC(errorTarget *error) *C.git_checkout_options {
 	if opts == nil {
 		return nil
 	}
-	c := C.git_checkout_options{}
-	populateCheckoutOptions(&c, opts)
-	return &c
+	return populateCheckoutOptions(&C.git_checkout_options{}, opts, errorTarget)
+}
+
+type checkoutCallbackData struct {
+	options     *CheckoutOptions
+	errorTarget *error
 }
 
 //export checkoutNotifyCallback
-func checkoutNotifyCallback(why C.git_checkout_notify_t, cpath *C.char, cbaseline, ctarget, cworkdir, data unsafe.Pointer) int {
-	if data == nil {
-		return 0
+func checkoutNotifyCallback(
+	why C.git_checkout_notify_t,
+	cpath *C.char,
+	cbaseline, ctarget, cworkdir, handle unsafe.Pointer,
+) C.int {
+	if handle == nil {
+		return C.int(ErrorCodeOK)
 	}
 	path := C.GoString(cpath)
 	var baseline, target, workdir DiffFile
@@ -111,26 +119,35 @@ func checkoutNotifyCallback(why C.git_checkout_notify_t, cpath *C.char, cbaselin
 	if cworkdir != nil {
 		workdir = diffFileFromC((*C.git_diff_file)(cworkdir))
 	}
-	opts := pointerHandles.Get(data).(*CheckoutOptions)
-	if opts.NotifyCallback == nil {
-		return 0
+	data := pointerHandles.Get(handle).(*checkoutCallbackData)
+	if data.options.NotifyCallback == nil {
+		return C.int(ErrorCodeOK)
+	}
+	ret := data.options.NotifyCallback(CheckoutNotifyType(why), path, baseline, target, workdir)
+	if ret < 0 {
+		*data.errorTarget = errors.New(ErrorCode(ret).String())
+		return C.int(ErrorCodeUser)
 	}
-	return int(opts.NotifyCallback(CheckoutNotifyType(why), path, baseline, target, workdir))
+	return C.int(ErrorCodeOK)
 }
 
 //export checkoutProgressCallback
-func checkoutProgressCallback(path *C.char, completed_steps, total_steps C.size_t, data unsafe.Pointer) int {
-	opts := pointerHandles.Get(data).(*CheckoutOptions)
-	if opts.ProgressCallback == nil {
-		return 0
+func checkoutProgressCallback(
+	path *C.char,
+	completed_steps, total_steps C.size_t,
+	handle unsafe.Pointer,
+) {
+	data := pointerHandles.Get(handle).(*checkoutCallbackData)
+	if data.options.ProgressCallback == nil {
+		return
 	}
-	return int(opts.ProgressCallback(C.GoString(path), uint(completed_steps), uint(total_steps)))
+	data.options.ProgressCallback(C.GoString(path), uint(completed_steps), uint(total_steps))
 }
 
 // Convert the CheckoutOptions struct to the corresponding
 // C-struct. Returns a pointer to ptr, or nil if opts is nil, in order
 // to help with what to pass.
-func populateCheckoutOptions(ptr *C.git_checkout_options, opts *CheckoutOptions) *C.git_checkout_options {
+func populateCheckoutOptions(ptr *C.git_checkout_options, opts *CheckoutOptions, errorTarget *error) *C.git_checkout_options {
 	if opts == nil {
 		return nil
 	}
@@ -142,14 +159,18 @@ func populateCheckoutOptions(ptr *C.git_checkout_options, opts *CheckoutOptions)
 	ptr.file_mode = C.uint(opts.FileMode.Perm())
 	ptr.notify_flags = C.uint(opts.NotifyFlags)
 	if opts.NotifyCallback != nil || opts.ProgressCallback != nil {
-		C._go_git_populate_checkout_cb(ptr)
-	}
-	payload := pointerHandles.Track(opts)
-	if opts.NotifyCallback != nil {
-		ptr.notify_payload = payload
-	}
-	if opts.ProgressCallback != nil {
-		ptr.progress_payload = payload
+		C._go_git_populate_checkout_callbacks(ptr)
+		data := &checkoutCallbackData{
+			options:     opts,
+			errorTarget: errorTarget,
+		}
+		payload := pointerHandles.Track(data)
+		if opts.NotifyCallback != nil {
+			ptr.notify_payload = payload
+		}
+		if opts.ProgressCallback != nil {
+			ptr.progress_payload = payload
+		}
 	}
 	if opts.TargetDirectory != "" {
 		ptr.target_directory = C.CString(opts.TargetDirectory)
@@ -176,6 +197,8 @@ func freeCheckoutOptions(ptr *C.git_checkout_options) {
 	}
 	if ptr.notify_payload != nil {
 		pointerHandles.Untrack(ptr.notify_payload)
+	} else if ptr.progress_payload != nil {
+		pointerHandles.Untrack(ptr.progress_payload)
 	}
 }
 
@@ -185,11 +208,16 @@ func (v *Repository) CheckoutHead(opts *CheckoutOptions) error {
 	runtime.LockOSThread()
 	defer runtime.UnlockOSThread()
 
-	cOpts := opts.toC()
+	var err error
+	cOpts := opts.toC(&err)
 	defer freeCheckoutOptions(cOpts)
 
 	ret := C.git_checkout_head(v.ptr, cOpts)
 	runtime.KeepAlive(v)
+
+	if ret == C.int(ErrorCodeUser) && err != nil {
+		return err
+	}
 	if ret < 0 {
 		return MakeGitError(ret)
 	}
@@ -209,11 +237,15 @@ func (v *Repository) CheckoutIndex(index *Index, opts *CheckoutOptions) error {
 	runtime.LockOSThread()
 	defer runtime.UnlockOSThread()
 
-	cOpts := opts.toC()
+	var err error
+	cOpts := opts.toC(&err)
 	defer freeCheckoutOptions(cOpts)
 
 	ret := C.git_checkout_index(v.ptr, iptr, cOpts)
 	runtime.KeepAlive(v)
+	if ret == C.int(ErrorCodeUser) && err != nil {
+		return err
+	}
 	if ret < 0 {
 		return MakeGitError(ret)
 	}
@@ -225,11 +257,16 @@ func (v *Repository) CheckoutTree(tree *Tree, opts *CheckoutOptions) error {
 	runtime.LockOSThread()
 	defer runtime.UnlockOSThread()
 
-	cOpts := opts.toC()
+	var err error
+	cOpts := opts.toC(&err)
 	defer freeCheckoutOptions(cOpts)
 
 	ret := C.git_checkout_tree(v.ptr, tree.ptr, cOpts)
 	runtime.KeepAlive(v)
+	runtime.KeepAlive(tree)
+	if ret == C.int(ErrorCodeUser) && err != nil {
+		return err
+	}
 	if ret < 0 {
 		return MakeGitError(ret)
 	}

cherrypick.go

@@ -25,22 +25,23 @@ func cherrypickOptionsFromC(c *C.git_cherrypick_options) CherrypickOptions {
 	return opts
 }
 
-func (opts *CherrypickOptions) toC() *C.git_cherrypick_options {
+func (opts *CherrypickOptions) toC(errorTarget *error) *C.git_cherrypick_options {
 	if opts == nil {
 		return nil
 	}
 	c := C.git_cherrypick_options{}
 	c.version = C.uint(opts.Version)
 	c.mainline = C.uint(opts.Mainline)
 	c.merge_opts = *opts.MergeOpts.toC()
-	c.checkout_opts = *opts.CheckoutOpts.toC()
+	c.checkout_opts = *opts.CheckoutOpts.toC(errorTarget)
 	return &c
 }
 
 func freeCherrypickOpts(ptr *C.git_cherrypick_options) {
 	if ptr == nil {
 		return
 	}
+	freeMergeOptions(&ptr.merge_opts)
 	freeCheckoutOptions(&ptr.checkout_opts)
 }
 
@@ -62,14 +63,18 @@ func (v *Repository) Cherrypick(commit *Commit, opts CherrypickOptions) error {
 	runtime.LockOSThread()
 	defer runtime.UnlockOSThread()
 
-	cOpts := opts.toC()
+	var err error
+	cOpts := opts.toC(&err)
 	defer freeCherrypickOpts(cOpts)
 
-	ecode := C.git_cherrypick(v.ptr, commit.cast_ptr, cOpts)
+	ret := C.git_cherrypick(v.ptr, commit.cast_ptr, cOpts)
 	runtime.KeepAlive(v)
 	runtime.KeepAlive(commit)
-	if ecode < 0 {
-		return MakeGitError(ecode)
+	if ret == C.int(ErrorCodeUser) && err != nil {
+		return err
+	}
+	if ret < 0 {
+		return MakeGitError(ret)
 	}
 	return nil
 }
@@ -79,6 +84,7 @@ func (r *Repository) CherrypickCommit(pick, our *Commit, opts CherrypickOptions)
 	defer runtime.UnlockOSThread()
 
 	cOpts := opts.MergeOpts.toC()
+	defer freeMergeOptions(cOpts)
 
 	var ptr *C.git_index
 	ret := C.git_cherrypick_commit(&ptr, r.ptr, pick.cast_ptr, our.cast_ptr, C.uint(opts.Mainline), cOpts)

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)
-
-		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")
-		}
-
-		return C.int(errorCode)
-	} else {
+	data, ok := pointerHandles.Get(payload).(*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
+
+	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_init_options(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))
 }