From e1e1b4b1e164b4aba04e43f8b93c9a552fdc6ab4 Mon Sep 17 00:00:00 2001 From: Dmitri Shuralyov Date: Mon, 6 Jul 2015 19:27:58 -0700 Subject: [PATCH 1/7] Prevent slot int variable from being GCed. Before this change, there were no users of slot int variable in the Go world (just a pointer to it that ended up in C world only), so Go's garbage collector would free it and its value could not retrieved later (once a pointer to it comes back to Go world from C world). Keep a pointer to it in the Go world so that does not happen. Fixes #218. --- handles.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/handles.go b/handles.go index ec62a48..a062231 100644 --- a/handles.go +++ b/handles.go @@ -10,14 +10,15 @@ type HandleList struct { sync.RWMutex // stores the Go pointers handles []interface{} - // indicates which indices are in use - set map[int]bool + // Indicates which indices are in use, and keeps a pointer to slot int variable (the handle) + // in the Go world, so that the Go garbage collector does not free it. + set map[int]*int } func NewHandleList() *HandleList { return &HandleList{ handles: make([]interface{}, 5), - set: make(map[int]bool), + set: make(map[int]*int), } } @@ -25,7 +26,7 @@ func NewHandleList() *HandleList { // list. You must only run this function while holding a write lock. func (v *HandleList) findUnusedSlot() int { for i := 1; i < len(v.handles); i++ { - isUsed := v.set[i] + _, isUsed := v.set[i] if !isUsed { return i } @@ -47,7 +48,7 @@ func (v *HandleList) Track(pointer interface{}) unsafe.Pointer { slot := v.findUnusedSlot() v.handles[slot] = pointer - v.set[slot] = true + v.set[slot] = &slot // Keep a pointer to slot in Go world, so it's not freed by GC. v.Unlock() -- 2.45.2 From 47d82916e2f9720470a44db22ed942ffcca20a20 Mon Sep 17 00:00:00 2001 From: Andreas Beer Date: Wed, 29 Jul 2015 11:28:05 +0200 Subject: [PATCH 2/7] Typos/unclarities in readme. --- README.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 386ff83..49e1ae9 100644 --- a/README.md +++ b/README.md @@ -8,13 +8,13 @@ Go bindings for [libgit2](http://libgit2.github.com/). The `master` branch follo Installing ---------- -This project wraps the functionality provided by libgit2. If you're using a stable version, install it to your system via your system's package manger and then install git2go as usual. +This project wraps the functionality provided by libgit2. If you're using a stable version, install it to your system via your system's package manaager and then install git2go as usual. Otherwise (`next` which tracks an unstable version), we need to build libgit2 as well. In order to build it, you need `cmake`, `pkg-config` and a C compiler. You will also need the development packages for OpenSSL and LibSSH2 installed if you want libgit2 to support HTTPS and SSH respectively. ### Stable version -git2go has `master` which tracks the latest release of libgit2, and versioned branches which indicate which version of libgit2 they work against. Install the development package it on your system via your favourite package manager or from source and you can use a service like gopkg.in to use the appropriate version. For the libgit2 v0.22 case, you can use +git2go has `master` which tracks the latest release of libgit2, and versioned branches which indicate which version of libgit2 they work against. Install the development package on your system via your favourite package manager or from source and you can use a service like gopkg.in to use the appropriate version. For the libgit2 v0.22 case, you can use import "gopkg.in/libgit2/git2go.v22" @@ -28,13 +28,13 @@ to use the version which works against the latest release. The `next` branch follows libgit2's master branch, which means there is no stable API or ABI to link against. git2go can statically link against a vendored version of libgit2. -Run `go get -d github.com/libgit2/git2go` to download the code and go to your `$GOPATH/src/github.com/libgit2/git2go` dir. From there, we need to build the C code and put it into the resulting go binary. +Run `go get -d github.com/libgit2/git2go` to download the code and go to your `$GOPATH/src/github.com/libgit2/git2go` directory. From there, we need to build the C code and put it into the resulting go binary. git checkout next git submodule update --init # get libgit2 make install -will compile libgit2 and run `go install` such that it's statically linked to the git2go package. +will compile libgit2. Run `go install` so that it's statically linked to the git2go package. Paralellism and network operations ---------------------------------- @@ -48,7 +48,7 @@ For the stable version, `go test` will work as usual. For the `next` branch, sim make test -alternatively, if you want to pass arguments to `go test`, you can use the script that sets it all up +Alternatively, if you want to pass arguments to `go test`, you can use the script that sets it all up ./script/with-static.sh go test -v -- 2.45.2 From 08d30893b6268ff655e4b3661dbc26c22515b1b0 Mon Sep 17 00:00:00 2001 From: Andreas Beer Date: Wed, 29 Jul 2015 11:28:34 +0200 Subject: [PATCH 3/7] Headline typo --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 49e1ae9..a5e6100 100644 --- a/README.md +++ b/README.md @@ -36,7 +36,7 @@ Run `go get -d github.com/libgit2/git2go` to download the code and go to your `$ will compile libgit2. Run `go install` so that it's statically linked to the git2go package. -Paralellism and network operations +Parallelism and network operations ---------------------------------- libgit2 uses OpenSSL and LibSSH2 for performing encrypted network connections. For now, git2go asks libgit2 to set locking for OpenSSL. This makes HTTPS connections thread-safe, but it is fragile and will likely stop doing it soon. This may also make SSH connections thread-safe if your copy of libssh2 is linked against OpenSSL. Check libgit2's `THREADSAFE.md` for more information. -- 2.45.2 From efd61a4bc0443ae604dbb4dbebd3a123c079b65b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Fri, 31 Jul 2015 11:37:18 +0200 Subject: [PATCH 4/7] Wrap MergeBases While here, test MergeBase as well. --- merge.go | 31 +++++++++++++++++++++++++++ merge_test.go | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+) diff --git a/merge.go b/merge.go index 5b68a8b..8c87391 100644 --- a/merge.go +++ b/merge.go @@ -10,6 +10,7 @@ extern git_annotated_commit* _go_git_annotated_commit_array_get(git_annotated_co */ import "C" import ( + "reflect" "runtime" "unsafe" ) @@ -243,6 +244,36 @@ func (r *Repository) MergeBase(one *Oid, two *Oid) (*Oid, error) { return newOidFromC(&oid), nil } +// MergeBases retrieves the list of merge bases between two commits. +// +// If none are found, an empty slice is returned and the error is set +// approprately +func (r *Repository) MergeBases(one, two *Oid) ([]*Oid, error) { + runtime.LockOSThread() + defer runtime.UnlockOSThread() + + var coids C.git_oidarray + ret := C.git_merge_bases(&coids, r.ptr, one.toC(), two.toC()) + if ret < 0 { + return make([]*Oid, 0), MakeGitError(ret) + } + + oids := make([]*Oid, coids.count) + hdr := reflect.SliceHeader { + Data: uintptr(unsafe.Pointer(coids.ids)), + Len: int(coids.count), + Cap: int(coids.count), + } + + goSlice := *(*[]C.git_oid)(unsafe.Pointer(&hdr)) + + for i, cid := range goSlice { + oids[i] = newOidFromC(&cid) + } + + return oids, nil +} + //TODO: int git_merge_base_many(git_oid *out, git_repository *repo, size_t length, const git_oid input_array[]); //TODO: GIT_EXTERN(int) git_merge_base_octopus(git_oid *out,git_repository *repo,size_t length,const git_oid input_array[]); diff --git a/merge_test.go b/merge_test.go index 0b1faca..c09deed 100644 --- a/merge_test.go +++ b/merge_test.go @@ -2,6 +2,7 @@ package git import ( "testing" + "time" ) func TestMergeWithSelf(t *testing.T) { @@ -88,6 +89,64 @@ func TestMergeTreesWithoutAncestor(t *testing.T) { } +func appendCommit(t *testing.T, repo *Repository) (*Oid, *Oid) { + loc, err := time.LoadLocation("Europe/Berlin") + checkFatal(t, err) + sig := &Signature{ + Name: "Rand Om Hacker", + Email: "random@hacker.com", + When: time.Date(2013, 03, 06, 14, 30, 0, 0, loc), + } + + idx, err := repo.Index() + checkFatal(t, err) + err = idx.AddByPath("README") + checkFatal(t, err) + treeId, err := idx.WriteTree() + checkFatal(t, err) + + message := "This is another commit\n" + tree, err := repo.LookupTree(treeId) + checkFatal(t, err) + + ref, err := repo.LookupReference("HEAD") + checkFatal(t, err) + + parent, err := ref.Peel(ObjectCommit) + checkFatal(t, err) + + commitId, err := repo.CreateCommit("HEAD", sig, sig, message, tree, parent.(*Commit)) + checkFatal(t, err) + + return commitId, treeId +} + +func TestMergeBase(t *testing.T) { + repo := createTestRepo(t) + defer cleanupTestRepo(t, repo) + + commitAId, _ := seedTestRepo(t, repo) + commitBId, _ := appendCommit(t, repo) + + mergeBase, err := repo.MergeBase(commitAId, commitBId) + checkFatal(t, err) + + if mergeBase.Cmp(commitAId) != 0 { + t.Fatalf("unexpected merge base") + } + + mergeBases, err := repo.MergeBases(commitAId, commitBId) + checkFatal(t, err) + + if len(mergeBases) != 1 { + t.Fatalf("expected merge bases len to be 1, got %v", len(mergeBases)) + } + + if mergeBases[0].Cmp(commitAId) != 0 { + t.Fatalf("unexpected merge base") + } +} + func compareBytes(t *testing.T, expected, actual []byte) { for i, v := range expected { if actual[i] != v { -- 2.45.2 From cce14aa58b746b0669d9a35e9812a41124f9114e Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 12 Aug 2015 10:10:18 +0200 Subject: [PATCH 5/7] branch: fix memory leaks related to CStrings --- branch.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/branch.go b/branch.go index 42e1216..8cf73b6 100644 --- a/branch.go +++ b/branch.go @@ -94,6 +94,7 @@ func (repo *Repository) CreateBranch(branchName string, target *Commit, force bo var ptr *C.git_reference cBranchName := C.CString(branchName) + defer C.free(unsafe.Pointer(cBranchName)) cForce := cbool(force) cSignature, err := signature.toC() @@ -134,6 +135,7 @@ func (b *Branch) Delete() error { func (b *Branch) Move(newBranchName string, force bool, signature *Signature, msg string) (*Branch, error) { var ptr *C.git_reference cNewBranchName := C.CString(newBranchName) + defer C.free(unsafe.Pointer(cNewBranchName)) cForce := cbool(force) cSignature, err := signature.toC() @@ -180,6 +182,7 @@ func (repo *Repository) LookupBranch(branchName string, bt BranchType) (*Branch, var ptr *C.git_reference cName := C.CString(branchName) + defer C.free(unsafe.Pointer(cName)) runtime.LockOSThread() defer runtime.UnlockOSThread() @@ -208,6 +211,7 @@ func (b *Branch) Name() (string, error) { func (repo *Repository) RemoteName(canonicalBranchName string) (string, error) { cName := C.CString(canonicalBranchName) + defer C.free(unsafe.Pointer(cName)) nameBuf := C.git_buf{} @@ -225,6 +229,7 @@ func (repo *Repository) RemoteName(canonicalBranchName string) (string, error) { func (b *Branch) SetUpstream(upstreamName string) error { cName := C.CString(upstreamName) + defer C.free(unsafe.Pointer(cName)) runtime.LockOSThread() defer runtime.UnlockOSThread() @@ -251,6 +256,7 @@ func (b *Branch) Upstream() (*Reference, error) { func (repo *Repository) UpstreamName(canonicalBranchName string) (string, error) { cName := C.CString(canonicalBranchName) + defer C.free(unsafe.Pointer(cName)) nameBuf := C.git_buf{} -- 2.45.2 From 37bb1a6025eac8b1e689686f8830a20a266394cf Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 12 Aug 2015 10:21:49 +0200 Subject: [PATCH 6/7] merge: fix memory leak related to merge file opts --- merge.go | 1 + 1 file changed, 1 insertion(+) diff --git a/merge.go b/merge.go index 8c87391..183305c 100644 --- a/merge.go +++ b/merge.go @@ -394,6 +394,7 @@ func MergeFile(ancestor MergeFileInput, ours MergeFileInput, theirs MergeFileInp return nil, MakeGitError(ecode) } populateCMergeFileOptions(copts, *options) + defer freeCMergeFileOptions(copts) } runtime.LockOSThread() -- 2.45.2 From 0b530c15cfff492e61c7afae55888fe1eeffe214 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 12 Aug 2015 12:44:58 +0200 Subject: [PATCH 7/7] 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); -- 2.45.2