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, 326 insertions, 78 deletions

diff --git a/wrapper.c b/wrapper.c
index 6dea2f3..cee8d00 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -1,36 +1,149 @@
 #include "_cgo_export.h"
+
 #include <git2.h>
 #include <git2/sys/odb_backend.h>
 #include <git2/sys/refdb_backend.h>
 #include <git2/sys/cred.h>
 
-typedef int (*gogit_submodule_cbk)(git_submodule *sm, const char *name, void *payload);
+// There are two ways in which to declare a callback:
+//
+// * If there is a guarantee that the callback will always be called within the
+//   same stack (e.g. by passing the callback directly into a function / into a
+//   struct that goes into a function), the following pattern is preferred,
+//   which preserves the error object as-is:
+//
+//   // myfile.go
+//   type FooCallback func(...) (..., error)
+//   type FooCallbackData struct {
+//     callback    FooCallback
+//     errorTarget *error
+//   }
+//
+//   //export fooCallback
+//   func fooCallback(..., handle unsafe.Pointer) C.int {
+//     payload := pointerHandles.Get(handle)
+//     data := payload.(*fooCallbackData)
+//     ...
+//     err := data.callback(...)
+//     if err != nil {
+//       *data.errorTarget = err
+//       return C.int(ErrorCodeUser)
+//     }
+//     return C.int(ErrorCodeOK)
+//   }
+//
+//   func MyFunction(... callback FooCallback) error {
+//    var err error
+//    data := fooCallbackData{
+//      callback:    callback,
+//      errorTarget: &err,
+//    }
+//    handle := pointerHandles.Track(&data)
+//    defer pointerHandles.Untrack(handle)
+//
+//    runtime.LockOSThread()
+//    defer runtime.UnlockOSThread()
+//
+//    ret := C._go_git_my_function(..., handle)
+//    if ret == C.int(ErrorCodeUser) && err != nil {
+//      return err
+//    }
+//    if ret < 0 {
+//      return MakeGitError(ret)
+//    }
+//    return nil
+//   }
+//
+//   // wrapper.c
+//   int _go_git_my_function(..., void *payload)
+//   {
+//     return git_my_function(..., (git_foo_cb)&fooCallback, payload);
+//   }
+//
+// * Otherwise, if the same callback can be invoked from multiple functions or
+//   from different stacks (e.g. when passing the callback to an object), a
+//   different pattern should be used instead, which has the downside of losing
+//   the original error object and converting it to a GitError:
+//
+//   // myfile.go
+//   type FooCallback func(...) (..., error)
+//
+//   //export fooCallback
+//   func fooCallback(errorMessage **C.char, ..., handle unsafe.Pointer) C.int {
+//     callback := pointerHandles.Get(data).(*FooCallback)
+//     ...
+//     err := callback(...)
+//     if err != nil {
+//       return setCallbackError(errorMessage, err)
+//     }
+//     return C.int(ErrorCodeOK)
+//   }
+//
+//   // wrapper.c
+//   static int foo_callback(...)
+//   {
+//     char *error_message = NULL;
+//     const int ret = fooCallback(&error_message, ...);
+//     return set_callback_error(error_message, ret);
+//   }
+
+/**
+ * Sets the thread-local error to the provided string. This needs to happen in
+ * C because Go might change Goroutines _just_ before returning, which would
+ * lose the contents of the error message.
+ */
+static int set_callback_error(char *error_message, int ret)
+{
+	if (error_message != NULL) {
+		if (ret < 0)
+			git_error_set_str(GIT_ERROR_CALLBACK, error_message);
+		free(error_message);
+	}
+	return ret;
+}
+
+void _go_git_populate_apply_callbacks(git_apply_options *options)
+{
+	options->delta_cb = (git_apply_delta_cb)&deltaApplyCallback;
+	options->hunk_cb = (git_apply_hunk_cb)&hunkApplyCallback;
+}
 
-void _go_git_populate_apply_cb(git_apply_options *options)
+static int commit_signing_callback(
+		git_buf *signature,
+		git_buf *signature_field,
+		const char *commit_contents,
+		void *payload)
 {
-	options->delta_cb = (git_apply_delta_cb)deltaApplyCallback;
-	options->hunk_cb = (git_apply_hunk_cb)hunkApplyCallback;
+	char *error_message = NULL;
+	const int ret = commitSigningCallback(
+			&error_message,
+			signature,
+			signature_field,
+			(char *)commit_contents,
+			payload
+	);
+	return set_callback_error(error_message, ret);
 }
 
-void _go_git_populate_commit_sign_cb(git_rebase_options *opts)
+void _go_git_populate_rebase_callbacks(git_rebase_options *opts)
 {
-	opts->signing_cb = (git_commit_signing_cb)commitSignCallback;
+	opts->signing_cb = commit_signing_callback;
 }
 
-void _go_git_populate_remote_cb(git_clone_options *opts)
+void _go_git_populate_clone_callbacks(git_clone_options *opts)
 {
-	opts->remote_cb = (git_remote_create_cb)remoteCreateCallback;
+	opts->remote_cb = (git_remote_create_cb)&remoteCreateCallback;
 }
 
-void _go_git_populate_checkout_cb(git_checkout_options *opts)
+void _go_git_populate_checkout_callbacks(git_checkout_options *opts)
 {
-	opts->notify_cb = (git_checkout_notify_cb)checkoutNotifyCallback;
-	opts->progress_cb = (git_checkout_progress_cb)checkoutProgressCallback;
+	opts->notify_cb = (git_checkout_notify_cb)&checkoutNotifyCallback;
+	opts->progress_cb = (git_checkout_progress_cb)&checkoutProgressCallback;
 }
 
 int _go_git_visit_submodule(git_repository *repo, void *fct)
 {
-	  return git_submodule_foreach(repo, (gogit_submodule_cbk)&submoduleCallback, fct);
+	return git_submodule_foreach(repo, (git_submodule_cb)&submoduleCallback, fct);
 }
 
 int _go_git_treewalk(git_tree *tree, git_treewalk_mode mode, void *ptr)
@@ -40,28 +153,26 @@ int _go_git_treewalk(git_tree *tree, git_treewalk_mode mode, void *ptr)
 
 int _go_git_packbuilder_foreach(git_packbuilder *pb, void *payload)
 {
-    return git_packbuilder_foreach(pb, (git_packbuilder_foreach_cb)&packbuilderForEachCb, payload);
+	return git_packbuilder_foreach(pb, (git_packbuilder_foreach_cb)&packbuilderForEachCallback, payload);
 }
 
 int _go_git_odb_foreach(git_odb *db, void *payload)
 {
-    return git_odb_foreach(db, (git_odb_foreach_cb)&odbForEachCb, payload);
+	return git_odb_foreach(db, (git_odb_foreach_cb)&odbForEachCallback, payload);
 }
 
 void _go_git_odb_backend_free(git_odb_backend *backend)
 {
-    if (backend->free)
-      backend->free(backend);
-
-    return;
+	if (!backend->free)
+		return;
+	backend->free(backend);
 }
 
 void _go_git_refdb_backend_free(git_refdb_backend *backend)
 {
-    if (backend->free)
-      backend->free(backend);
-
-    return;
+	if (!backend->free)
+		return;
+	backend->free(backend);
 }
 
 int _go_git_diff_foreach(git_diff *diff, int eachFile, int eachHunk, int eachLine, void *payload)
@@ -70,83 +181,210 @@ int _go_git_diff_foreach(git_diff *diff, int eachFile, int eachHunk, int eachLin
 	git_diff_hunk_cb hcb = NULL;
 	git_diff_line_cb lcb = NULL;
 
-	if (eachFile) {
-		fcb = (git_diff_file_cb)&diffForEachFileCb;
-	}
-
-	if (eachHunk) {
-		hcb = (git_diff_hunk_cb)&diffForEachHunkCb;
-	}
-
-	if (eachLine) {
-		lcb = (git_diff_line_cb)&diffForEachLineCb;
-	}
+	if (eachFile)
+		fcb = (git_diff_file_cb)&diffForEachFileCallback;
+	if (eachHunk)
+		hcb = (git_diff_hunk_cb)&diffForEachHunkCallback;
+	if (eachLine)
+		lcb = (git_diff_line_cb)&diffForEachLineCallback;
 
 	return git_diff_foreach(diff, fcb, NULL, hcb, lcb, payload);
 }
 
-int _go_git_diff_blobs(git_blob *old, const char *old_path, git_blob *new, const char *new_path, git_diff_options *opts, int eachFile, int eachHunk, int eachLine, void *payload)
+int _go_git_diff_blobs(
+		git_blob *old,
+		const char *old_path,
+		git_blob *new,
+		const char *new_path,
+		git_diff_options *opts,
+		int eachFile,
+		int eachHunk,
+		int eachLine,
+		void *payload)
 {
 	git_diff_file_cb fcb = NULL;
 	git_diff_hunk_cb hcb = NULL;
 	git_diff_line_cb lcb = NULL;
 
-	if (eachFile) {
-		fcb = (git_diff_file_cb)&diffForEachFileCb;
-	}
+	if (eachFile)
+		fcb = (git_diff_file_cb)&diffForEachFileCallback;
+	if (eachHunk)
+		hcb = (git_diff_hunk_cb)&diffForEachHunkCallback;
+	if (eachLine)
+		lcb = (git_diff_line_cb)&diffForEachLineCallback;
 
-	if (eachHunk) {
-		hcb = (git_diff_hunk_cb)&diffForEachHunkCb;
-	}
+	return git_diff_blobs(old, old_path, new, new_path, opts, fcb, NULL, hcb, lcb, payload);
+}
 
-	if (eachLine) {
-		lcb = (git_diff_line_cb)&diffForEachLineCb;
-	}
+void _go_git_setup_diff_notify_callbacks(git_diff_options *opts)
+{
+	opts->notify_cb = (git_diff_notify_cb)&diffNotifyCallback;
+}
 
-	return git_diff_blobs(old, old_path, new, new_path, opts, fcb, NULL, hcb, lcb, payload);
+static int sideband_progress_callback(const char *str, int len, void *payload)
+{
+	char *error_message = NULL;
+	const int ret = sidebandProgressCallback(&error_message, (char *)str, len, payload);
+	return set_callback_error(error_message, ret);
+}
+
+static int completion_callback(git_remote_completion_type completion_type, void *data)
+{
+	char *error_message = NULL;
+	const int ret = completionCallback(&error_message, completion_type, data);
+	return set_callback_error(error_message, ret);
 }
 
-void _go_git_setup_diff_notify_callbacks(git_diff_options *opts) {
-	opts->notify_cb = (git_diff_notify_cb)diffNotifyCb;
+static int credentials_callback(
+		git_credential **cred,
+		const char *url,
+		const char *username_from_url,
+		unsigned int allowed_types,
+		void *data)
+{
+	char *error_message = NULL;
+	const int ret = credentialsCallback(
+			&error_message,
+			cred,
+			(char *)url,
+			(char *)username_from_url,
+			allowed_types,
+			data
+	);
+	return set_callback_error(error_message, ret);
+}
+
+static int transfer_progress_callback(const git_transfer_progress *stats, void *data)
+{
+	char *error_message = NULL;
+	const int ret = transferProgressCallback(
+			&error_message,
+			(git_transfer_progress *)stats,
+			data
+	);
+	return set_callback_error(error_message, ret);
+}
+
+static int update_tips_callback(const char *refname, const git_oid *a, const git_oid *b, void *data)
+{
+	char *error_message = NULL;
+	const int ret = updateTipsCallback(
+			&error_message,
+			(char *)refname,
+			(git_oid *)a,
+			(git_oid *)b,
+			data
+	);
+	return set_callback_error(error_message, ret);
 }
 
-void _go_git_setup_callbacks(git_remote_callbacks *callbacks) {
-	typedef int (*completion_cb)(git_remote_completion_type type, void *data);
-	typedef int (*update_tips_cb)(const char *refname, const git_oid *a, const git_oid *b, void *data);
-	typedef int (*push_update_reference_cb)(const char *refname, const char *status, void *data);
+static int certificate_check_callback(git_cert *cert, int valid, const char *host, void *data)
+{
+	char *error_message = NULL;
+	const int ret = certificateCheckCallback(
+			&error_message,
+			cert,
+			valid,
+			(char *)host,
+			data
+	);
+	return set_callback_error(error_message, ret);
+}
 
-	callbacks->sideband_progress = (git_transport_message_cb)sidebandProgressCallback;
-	callbacks->completion = (completion_cb)completionCallback;
-	callbacks->credentials = (git_credential_acquire_cb)credentialsCallback;
-	callbacks->transfer_progress = (git_transfer_progress_cb)transferProgressCallback;
-	callbacks->update_tips = (update_tips_cb)updateTipsCallback;
-	callbacks->certificate_check = (git_transport_certificate_check_cb) certificateCheckCallback;
-	callbacks->pack_progress = (git_packbuilder_progress) packProgressCallback;
-	callbacks->push_transfer_progress = (git_push_transfer_progress) pushTransferProgressCallback;
-	callbacks->push_update_reference = (push_update_reference_cb) pushUpdateReferenceCallback;
+static int pack_progress_callback(int stage, unsigned int current, unsigned int total, void *data)
+{
+	char *error_message = NULL;
+	const int ret = packProgressCallback(
+			&error_message,
+			stage,
+			current,
+			total,
+			data
+	);
+	return set_callback_error(error_message, ret);
 }
 
-int _go_git_index_add_all(git_index *index, const git_strarray *pathspec, unsigned int flags, void *callback) {
-	git_index_matched_path_cb cb = callback ? (git_index_matched_path_cb) &indexMatchedPathCallback : NULL;
+static int push_transfer_progress_callback(
+		unsigned int current,
+		unsigned int total,
+		size_t bytes,
+		void *data)
+{
+	char *error_message = NULL;
+	const int ret = pushTransferProgressCallback(
+			&error_message,
+			current,
+			total,
+			bytes,
+			data
+	);
+	return set_callback_error(error_message, ret);
+}
+
+static int push_update_reference_callback(const char *refname, const char *status, void *data)
+{
+	char *error_message = NULL;
+	const int ret = pushUpdateReferenceCallback(
+			&error_message,
+			(char *)refname,
+			(char *)status,
+			data
+	);
+	return set_callback_error(error_message, ret);
+}
+
+void _go_git_populate_remote_callbacks(git_remote_callbacks *callbacks)
+{
+	callbacks->sideband_progress = sideband_progress_callback;
+	callbacks->completion = completion_callback;
+	callbacks->credentials = credentials_callback;
+	callbacks->transfer_progress = transfer_progress_callback;
+	callbacks->update_tips = update_tips_callback;
+	callbacks->certificate_check = certificate_check_callback;
+	callbacks->pack_progress = pack_progress_callback;
+	callbacks->push_transfer_progress = push_transfer_progress_callback;
+	callbacks->push_update_reference = push_update_reference_callback;
+}
+
+int _go_git_index_add_all(git_index *index, const git_strarray *pathspec, unsigned int flags, void *callback)
+{
+	git_index_matched_path_cb cb = callback ? (git_index_matched_path_cb)&indexMatchedPathCallback : NULL;
 	return git_index_add_all(index, pathspec, flags, cb, callback);
 }
 
-int _go_git_index_update_all(git_index *index, const git_strarray *pathspec, void *callback) {
-	git_index_matched_path_cb cb = callback ? (git_index_matched_path_cb) &indexMatchedPathCallback : NULL;
+int _go_git_index_update_all(git_index *index, const git_strarray *pathspec, void *callback)
+{
+	git_index_matched_path_cb cb = callback ? (git_index_matched_path_cb)&indexMatchedPathCallback : NULL;
 	return git_index_update_all(index, pathspec, cb, callback);
 }
 
-int _go_git_index_remove_all(git_index *index, const git_strarray *pathspec, void *callback) {
-	git_index_matched_path_cb cb = callback ? (git_index_matched_path_cb) &indexMatchedPathCallback : NULL;
+int _go_git_index_remove_all(git_index *index, const git_strarray *pathspec, void *callback)
+{
+	git_index_matched_path_cb cb = callback ? (git_index_matched_path_cb)&indexMatchedPathCallback : NULL;
 	return git_index_remove_all(index, pathspec, cb, callback);
 }
 
 int _go_git_tag_foreach(git_repository *repo, void *payload)
 {
-    return git_tag_foreach(repo, (git_tag_foreach_cb)&gitTagForeachCb, payload);
+	return git_tag_foreach(repo, (git_tag_foreach_cb)&tagForeachCallback, payload);
 }
 
-int _go_git_merge_file(git_merge_file_result* out, char* ancestorContents, size_t ancestorLen, char* ancestorPath, unsigned int ancestorMode, char* oursContents, size_t oursLen, char* oursPath, unsigned int oursMode, char* theirsContents, size_t theirsLen, char* theirsPath, unsigned int theirsMode, git_merge_file_options* copts) {
+int _go_git_merge_file(
+		git_merge_file_result* out,
+		char* ancestorContents,
+		size_t ancestorLen,
+		char* ancestorPath,
+		unsigned int ancestorMode,
+		char* oursContents,
+		size_t oursLen,
+		char* oursPath,
+		unsigned int oursMode,
+		char* theirsContents,
+		size_t theirsLen,
+		char* theirsPath,
+		unsigned int theirsMode,
+		git_merge_file_options* copts)
+{
 	git_merge_file_input ancestor = GIT_MERGE_FILE_INPUT_INIT;
 	git_merge_file_input ours = GIT_MERGE_FILE_INPUT_INIT;
 	git_merge_file_input theirs = GIT_MERGE_FILE_INPUT_INIT;
@@ -169,12 +407,14 @@ int _go_git_merge_file(git_merge_file_result* out, char* ancestorContents, size_
 	return git_merge_file(out, &ancestor, &ours, &theirs, copts);
 }
 
-void _go_git_setup_stash_apply_progress_callbacks(git_stash_apply_options *opts) {
-	opts->progress_cb = (git_stash_apply_progress_cb)stashApplyProgressCb;
+void _go_git_populate_stash_apply_callbacks(git_stash_apply_options *opts)
+{
+	opts->progress_cb = (git_stash_apply_progress_cb)&stashApplyProgressCallback;
 }
 
-int _go_git_stash_foreach(git_repository *repo, void *payload) {
-    return git_stash_foreach(repo, (git_stash_cb)&stashForeachCb, payload);
+int _go_git_stash_foreach(git_repository *repo, void *payload)
+{
+	return git_stash_foreach(repo, (git_stash_cb)&stashForeachCallback, payload);
 }
 
 int _go_git_writestream_write(git_writestream *stream, const char *buffer, size_t len)
@@ -192,16 +432,21 @@ void _go_git_writestream_free(git_writestream *stream)
 	stream->free(stream);
 }
 
-git_credential_t _go_git_credential_credtype(git_credential *cred) {
+git_credential_t _go_git_credential_credtype(git_credential *cred)
+{
 	return cred->credtype;
 }
 
 int _go_git_odb_write_pack(git_odb_writepack **out, git_odb *db, void *progress_payload)
 {
-	return git_odb_write_pack(out, db, (git_transfer_progress_cb)transferProgressCallback, progress_payload);
+	return git_odb_write_pack(out, db, transfer_progress_callback, progress_payload);
 }
 
-int _go_git_odb_writepack_append(git_odb_writepack *writepack, const void *data, size_t size, git_transfer_progress *stats)
+int _go_git_odb_writepack_append(
+		git_odb_writepack *writepack,
+		const void *data,
+		size_t size,
+		git_transfer_progress *stats)
 {
 	return writepack->append(writepack, data, size, stats);
 }
@@ -216,12 +461,15 @@ void _go_git_odb_writepack_free(git_odb_writepack *writepack)
 	writepack->free(writepack);
 }
 
-int _go_git_indexer_new(git_indexer **out, const char *path, unsigned int mode, git_odb *odb, void *progress_cb_payload)
+int _go_git_indexer_new(
+		git_indexer **out,
+		const char *path,
+		unsigned int mode,
+		git_odb *odb,
+		void *progress_cb_payload)
 {
 	git_indexer_options indexer_options = GIT_INDEXER_OPTIONS_INIT;
-	indexer_options.progress_cb = (git_transfer_progress_cb)transferProgressCallback;
+	indexer_options.progress_cb = transfer_progress_callback;
 	indexer_options.progress_cb_payload = progress_cb_payload;
 	return git_indexer_new(out, path, mode, odb, &indexer_options);
 }
-
-/* EOF */