Skip to content

Commit 5d8eaf7

Browse files
lhchavezlhchavez
authored
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 parent 137c05e commit 5d8eaf7

21 files changed

+1153
-578
lines changed

‎checkout.go‎

Lines changed: 68 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,11 @@ package git
33
/*
44
#include <git2.h>
55
6-
extern void _go_git_populate_checkout_cb(git_checkout_options *opts);
6+
extern void _go_git_populate_checkout_callbacks(git_checkout_options *opts);
77
*/
88
import "C"
99
import (
10+
"errors"
1011
"os"
1112
"runtime"
1213
"unsafe"
@@ -75,30 +76,37 @@ func checkoutOptionsFromC(c *C.git_checkout_options) CheckoutOptions {
7576
NotifyFlags: CheckoutNotifyType(c.notify_flags),
7677
}
7778
if c.notify_payload != nil {
78-
opts.NotifyCallback = pointerHandles.Get(c.notify_payload).(*CheckoutOptions).NotifyCallback
79+
opts.NotifyCallback = pointerHandles.Get(c.notify_payload).(*checkoutCallbackData).options.NotifyCallback
7980
}
8081
if c.progress_payload != nil {
81-
opts.ProgressCallback = pointerHandles.Get(c.progress_payload).(*CheckoutOptions).ProgressCallback
82+
opts.ProgressCallback = pointerHandles.Get(c.progress_payload).(*checkoutCallbackData).options.ProgressCallback
8283
}
8384
if c.target_directory != nil {
8485
opts.TargetDirectory = C.GoString(c.target_directory)
8586
}
8687
return opts
8788
}
8889

89-
func (opts *CheckoutOptions) toC() *C.git_checkout_options {
90+
func (opts *CheckoutOptions) toC(errorTarget *error) *C.git_checkout_options {
9091
if opts == nil {
9192
return nil
9293
}
93-
c := C.git_checkout_options{}
94-
populateCheckoutOptions(&c, opts)
95-
return &c
94+
return populateCheckoutOptions(&C.git_checkout_options{}, opts, errorTarget)
95+
}
96+
97+
type checkoutCallbackData struct {
98+
options *CheckoutOptions
99+
errorTarget *error
96100
}
97101

98102
//export checkoutNotifyCallback
99-
func checkoutNotifyCallback(why C.git_checkout_notify_t, cpath *C.char, cbaseline, ctarget, cworkdir, data unsafe.Pointer) int {
100-
if data == nil {
101-
return 0
103+
func checkoutNotifyCallback(
104+
why C.git_checkout_notify_t,
105+
cpath *C.char,
106+
cbaseline, ctarget, cworkdir, handle unsafe.Pointer,
107+
) C.int {
108+
if handle == nil {
109+
return C.int(ErrorCodeOK)
102110
}
103111
path := C.GoString(cpath)
104112
var baseline, target, workdir DiffFile
@@ -111,26 +119,35 @@ func checkoutNotifyCallback(why C.git_checkout_notify_t, cpath *C.char, cbaselin
111119
if cworkdir != nil {
112120
workdir = diffFileFromC((*C.git_diff_file)(cworkdir))
113121
}
114-
opts := pointerHandles.Get(data).(*CheckoutOptions)
115-
if opts.NotifyCallback == nil {
116-
return 0
122+
data := pointerHandles.Get(handle).(*checkoutCallbackData)
123+
if data.options.NotifyCallback == nil {
124+
return C.int(ErrorCodeOK)
125+
}
126+
ret := data.options.NotifyCallback(CheckoutNotifyType(why), path, baseline, target, workdir)
127+
if ret < 0 {
128+
*data.errorTarget = errors.New(ErrorCode(ret).String())
129+
return C.int(ErrorCodeUser)
117130
}
118-
return int(opts.NotifyCallback(CheckoutNotifyType(why), path, baseline, target, workdir))
131+
return C.int(ErrorCodeOK)
119132
}
120133

121134
//export checkoutProgressCallback
122-
func checkoutProgressCallback(path *C.char, completed_steps, total_steps C.size_t, data unsafe.Pointer) int {
123-
opts := pointerHandles.Get(data).(*CheckoutOptions)
124-
if opts.ProgressCallback == nil {
125-
return 0
135+
func checkoutProgressCallback(
136+
path *C.char,
137+
completed_steps, total_steps C.size_t,
138+
handle unsafe.Pointer,
139+
) {
140+
data := pointerHandles.Get(handle).(*checkoutCallbackData)
141+
if data.options.ProgressCallback == nil {
142+
return
126143
}
127-
return int(opts.ProgressCallback(C.GoString(path), uint(completed_steps), uint(total_steps)))
144+
data.options.ProgressCallback(C.GoString(path), uint(completed_steps), uint(total_steps))
128145
}
129146

130147
// Convert the CheckoutOptions struct to the corresponding
131148
// C-struct. Returns a pointer to ptr, or nil if opts is nil, in order
132149
// to help with what to pass.
133-
func populateCheckoutOptions(ptr *C.git_checkout_options, opts *CheckoutOptions) *C.git_checkout_options {
150+
func populateCheckoutOptions(ptr *C.git_checkout_options, opts *CheckoutOptions, errorTarget *error) *C.git_checkout_options {
134151
if opts == nil {
135152
return nil
136153
}
@@ -142,14 +159,18 @@ func populateCheckoutOptions(ptr *C.git_checkout_options, opts *CheckoutOptions)
142159
ptr.file_mode = C.uint(opts.FileMode.Perm())
143160
ptr.notify_flags = C.uint(opts.NotifyFlags)
144161
if opts.NotifyCallback != nil || opts.ProgressCallback != nil {
145-
C._go_git_populate_checkout_cb(ptr)
146-
}
147-
payload := pointerHandles.Track(opts)
148-
if opts.NotifyCallback != nil {
149-
ptr.notify_payload = payload
150-
}
151-
if opts.ProgressCallback != nil {
152-
ptr.progress_payload = payload
162+
C._go_git_populate_checkout_callbacks(ptr)
163+
data := &checkoutCallbackData{
164+
options: opts,
165+
errorTarget: errorTarget,
166+
}
167+
payload := pointerHandles.Track(data)
168+
if opts.NotifyCallback != nil {
169+
ptr.notify_payload = payload
170+
}
171+
if opts.ProgressCallback != nil {
172+
ptr.progress_payload = payload
173+
}
153174
}
154175
if opts.TargetDirectory != "" {
155176
ptr.target_directory = C.CString(opts.TargetDirectory)
@@ -176,6 +197,8 @@ func freeCheckoutOptions(ptr *C.git_checkout_options) {
176197
}
177198
if ptr.notify_payload != nil {
178199
pointerHandles.Untrack(ptr.notify_payload)
200+
} else if ptr.progress_payload != nil {
201+
pointerHandles.Untrack(ptr.progress_payload)
179202
}
180203
}
181204

@@ -185,11 +208,16 @@ func (v *Repository) CheckoutHead(opts *CheckoutOptions) error {
185208
runtime.LockOSThread()
186209
defer runtime.UnlockOSThread()
187210

188-
cOpts := opts.toC()
211+
var err error
212+
cOpts := opts.toC(&err)
189213
defer freeCheckoutOptions(cOpts)
190214

191215
ret := C.git_checkout_head(v.ptr, cOpts)
192216
runtime.KeepAlive(v)
217+
218+
if ret == C.int(ErrorCodeUser) && err != nil {
219+
return err
220+
}
193221
if ret < 0 {
194222
return MakeGitError(ret)
195223
}
@@ -209,11 +237,15 @@ func (v *Repository) CheckoutIndex(index *Index, opts *CheckoutOptions) error {
209237
runtime.LockOSThread()
210238
defer runtime.UnlockOSThread()
211239

212-
cOpts := opts.toC()
240+
var err error
241+
cOpts := opts.toC(&err)
213242
defer freeCheckoutOptions(cOpts)
214243

215244
ret := C.git_checkout_index(v.ptr, iptr, cOpts)
216245
runtime.KeepAlive(v)
246+
if ret == C.int(ErrorCodeUser) && err != nil {
247+
return err
248+
}
217249
if ret < 0 {
218250
return MakeGitError(ret)
219251
}
@@ -225,11 +257,16 @@ func (v *Repository) CheckoutTree(tree *Tree, opts *CheckoutOptions) error {
225257
runtime.LockOSThread()
226258
defer runtime.UnlockOSThread()
227259

228-
cOpts := opts.toC()
260+
var err error
261+
cOpts := opts.toC(&err)
229262
defer freeCheckoutOptions(cOpts)
230263

231264
ret := C.git_checkout_tree(v.ptr, tree.ptr, cOpts)
232265
runtime.KeepAlive(v)
266+
runtime.KeepAlive(tree)
267+
if ret == C.int(ErrorCodeUser) && err != nil {
268+
return err
269+
}
233270
if ret < 0 {
234271
return MakeGitError(ret)
235272
}

‎cherrypick.go‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -25,22 +25,23 @@ func cherrypickOptionsFromC(c *C.git_cherrypick_options) CherrypickOptions {
2525
return opts
2626
}
2727

28-
func (opts *CherrypickOptions) toC() *C.git_cherrypick_options {
28+
func (opts *CherrypickOptions) toC(errorTarget *error) *C.git_cherrypick_options {
2929
if opts == nil {
3030
return nil
3131
}
3232
c := C.git_cherrypick_options{}
3333
c.version = C.uint(opts.Version)
3434
c.mainline = C.uint(opts.Mainline)
3535
c.merge_opts = *opts.MergeOpts.toC()
36-
c.checkout_opts = *opts.CheckoutOpts.toC()
36+
c.checkout_opts = *opts.CheckoutOpts.toC(errorTarget)
3737
return &c
3838
}
3939

4040
func freeCherrypickOpts(ptr *C.git_cherrypick_options) {
4141
if ptr == nil {
4242
return
4343
}
44+
freeMergeOptions(&ptr.merge_opts)
4445
freeCheckoutOptions(&ptr.checkout_opts)
4546
}
4647

@@ -62,14 +63,18 @@ func (v *Repository) Cherrypick(commit *Commit, opts CherrypickOptions) error {
6263
runtime.LockOSThread()
6364
defer runtime.UnlockOSThread()
6465

65-
cOpts := opts.toC()
66+
var err error
67+
cOpts := opts.toC(&err)
6668
defer freeCherrypickOpts(cOpts)
6769

68-
ecode := C.git_cherrypick(v.ptr, commit.cast_ptr, cOpts)
70+
ret := C.git_cherrypick(v.ptr, commit.cast_ptr, cOpts)
6971
runtime.KeepAlive(v)
7072
runtime.KeepAlive(commit)
71-
if ecode < 0 {
72-
return MakeGitError(ecode)
73+
if ret == C.int(ErrorCodeUser) && err != nil {
74+
return err
75+
}
76+
if ret < 0 {
77+
return MakeGitError(ret)
7378
}
7479
return nil
7580
}
@@ -79,6 +84,7 @@ func (r *Repository) CherrypickCommit(pick, our *Commit, opts CherrypickOptions)
7984
defer runtime.UnlockOSThread()
8085

8186
cOpts := opts.MergeOpts.toC()
87+
defer freeMergeOptions(cOpts)
8288

8389
var ptr *C.git_index
8490
ret := C.git_cherrypick_commit(&ptr, r.ptr, pick.cast_ptr, our.cast_ptr, C.uint(opts.Mainline), cOpts)

‎clone.go‎

Lines changed: 53 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,11 @@ package git
33
/*
44
#include <git2.h>
55
6-
extern void _go_git_populate_remote_cb(git_clone_options *opts);
6+
extern void _go_git_populate_clone_callbacks(git_clone_options *opts);
77
*/
88
import "C"
99
import (
10+
"errors"
1011
"runtime"
1112
"unsafe"
1213
)
@@ -28,20 +29,23 @@ func Clone(url string, path string, options *CloneOptions) (*Repository, error)
2829
cpath := C.CString(path)
2930
defer C.free(unsafe.Pointer(cpath))
3031

31-
copts := (*C.git_clone_options)(C.calloc(1, C.size_t(unsafe.Sizeof(C.git_clone_options{}))))
32-
populateCloneOptions(copts, options)
33-
defer freeCloneOptions(copts)
32+
var err error
33+
cOptions := populateCloneOptions(&C.git_clone_options{}, options, &err)
34+
defer freeCloneOptions(cOptions)
3435

3536
if len(options.CheckoutBranch) != 0 {
36-
copts.checkout_branch = C.CString(options.CheckoutBranch)
37+
cOptions.checkout_branch = C.CString(options.CheckoutBranch)
3738
}
3839

3940
runtime.LockOSThread()
4041
defer runtime.UnlockOSThread()
4142

4243
var ptr *C.git_repository
43-
ret := C.git_clone(&ptr, curl, cpath, copts)
44+
ret := C.git_clone(&ptr, curl, cpath, cOptions)
4445

46+
if ret == C.int(ErrorCodeUser) && err != nil {
47+
return nil, err
48+
}
4549
if ret < 0 {
4650
return nil, MakeGitError(ret)
4751
}
@@ -50,47 +54,69 @@ func Clone(url string, path string, options *CloneOptions) (*Repository, error)
5054
}
5155

5256
//export remoteCreateCallback
53-
func remoteCreateCallback(cremote unsafe.Pointer, crepo unsafe.Pointer, cname, curl *C.char, payload unsafe.Pointer) C.int {
57+
func remoteCreateCallback(
58+
cremote unsafe.Pointer,
59+
crepo unsafe.Pointer,
60+
cname, curl *C.char,
61+
payload unsafe.Pointer,
62+
) C.int {
5463
name := C.GoString(cname)
5564
url := C.GoString(curl)
5665
repo := newRepositoryFromC((*C.git_repository)(crepo))
5766
// We don't own this repository, so make sure we don't try to free it
5867
runtime.SetFinalizer(repo, nil)
5968

60-
if opts, ok := pointerHandles.Get(payload).(CloneOptions); ok {
61-
remote, errorCode := opts.RemoteCreateCallback(repo, name, url)
62-
// clear finalizer as the calling C function will
63-
// free the remote itself
64-
runtime.SetFinalizer(remote, nil)
65-
66-
if errorCode == ErrorCodeOK && remote != nil {
67-
cptr := (**C.git_remote)(cremote)
68-
*cptr = remote.ptr
69-
} else if errorCode == ErrorCodeOK && remote == nil {
70-
panic("no remote created by callback")
71-
}
72-
73-
return C.int(errorCode)
74-
} else {
69+
data, ok := pointerHandles.Get(payload).(*cloneCallbackData)
70+
if !ok {
7571
panic("invalid remote create callback")
7672
}
73+
74+
remote, ret := data.options.RemoteCreateCallback(repo, name, url)
75+
// clear finalizer as the calling C function will
76+
// free the remote itself
77+
runtime.SetFinalizer(remote, nil)
78+
79+
if ret < 0 {
80+
*data.errorTarget = errors.New(ErrorCode(ret).String())
81+
return C.int(ErrorCodeUser)
82+
}
83+
84+
if remote == nil {
85+
panic("no remote created by callback")
86+
}
87+
88+
cptr := (**C.git_remote)(cremote)
89+
*cptr = remote.ptr
90+
91+
return C.int(ErrorCodeOK)
92+
}
93+
94+
type cloneCallbackData struct {
95+
options *CloneOptions
96+
errorTarget *error
7797
}
7898

79-
func populateCloneOptions(ptr *C.git_clone_options, opts *CloneOptions) {
99+
func populateCloneOptions(ptr *C.git_clone_options, opts *CloneOptions, errorTarget *error) *C.git_clone_options {
80100
C.git_clone_options_init(ptr, C.GIT_CLONE_OPTIONS_VERSION)
81101

82102
if opts == nil {
83-
return
103+
return nil
84104
}
85-
populateCheckoutOptions(&ptr.checkout_opts, opts.CheckoutOpts)
105+
populateCheckoutOptions(&ptr.checkout_opts, opts.CheckoutOpts, errorTarget)
86106
populateFetchOptions(&ptr.fetch_opts, opts.FetchOptions)
87107
ptr.bare = cbool(opts.Bare)
88108

89109
if opts.RemoteCreateCallback != nil {
110+
data := &cloneCallbackData{
111+
options: opts,
112+
errorTarget: errorTarget,
113+
}
90114
// Go v1.1 does not allow to assign a C function pointer
91-
C._go_git_populate_remote_cb(ptr)
92-
ptr.remote_cb_payload = pointerHandles.Track(*opts)
115+
C._go_git_populate_clone_callbacks(ptr)
116+
ptr.remote_cb_payload = pointerHandles.Track(data)
93117
}
118+
119+
return ptr
94120
}
95121

96122
func freeCloneOptions(ptr *C.git_clone_options) {
@@ -105,5 +131,4 @@ func freeCloneOptions(ptr *C.git_clone_options) {
105131
}
106132

107133
C.free(unsafe.Pointer(ptr.checkout_branch))
108-
C.free(unsafe.Pointer(ptr))
109134
}

0 commit comments

Comments
 (0)