From 0b530c15cfff492e61c7afae55888fe1eeffe214 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 12 Aug 2015 12:44:58 +0200 Subject: [PATCH] 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. --- clone.go | 61 +++++++++++++++++++++++++++++++++++++++++---------- clone_test.go | 45 ++++++++++++++++++++++++++++++++++++- wrapper.c | 5 +++++ 3 files changed, 98 insertions(+), 13 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 +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)) - - if opts.RemoteCreatePayload != nil { - ptr.remote_cb_payload = opts.RemoteCreatePayload - defer C.free(opts.RemoteCreatePayload) - } + // 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) } } + +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);