From 5d8eaf7e65c404a0d10d3705697dd99369630dda Mon Sep 17 00:00:00 2001 From: lhchavez Date: Sat, 5 Dec 2020 13:13:59 -0800 Subject: 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`. --- diff.go | 398 ++++++++++++++++++++++++++++++++++++++-------------------------- 1 file changed, 234 insertions(+), 164 deletions(-) (limited to 'diff.go') diff --git a/diff.go b/diff.go index e50cd70..dd793e4 100644 --- a/diff.go +++ b/diff.go @@ -3,7 +3,7 @@ package git /* #include -extern void _go_git_populate_apply_cb(git_apply_options *options); +extern void _go_git_populate_apply_callbacks(git_apply_options *options); extern int _go_git_diff_foreach(git_diff *diff, int eachFile, int eachHunk, int eachLine, void *payload); extern void _go_git_setup_diff_notify_callbacks(git_diff_options* opts); extern 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); @@ -294,11 +294,11 @@ func (diff *Diff) Stats() (*DiffStats, error) { return stats, nil } -type diffForEachData struct { - FileCallback DiffForEachFileCallback - HunkCallback DiffForEachHunkCallback - LineCallback DiffForEachLineCallback - Error error +type diffForEachCallbackData struct { + fileCallback DiffForEachFileCallback + hunkCallback DiffForEachHunkCallback + lineCallback DiffForEachLineCallback + errorTarget *error } type DiffForEachFileCallback func(delta DiffDelta, progress float64) (DiffForEachHunkCallback, error) @@ -326,82 +326,91 @@ func (diff *Diff) ForEach(cbFile DiffForEachFileCallback, detail DiffDetail) err intLines = C.int(1) } - data := &diffForEachData{ - FileCallback: cbFile, + var err error + data := &diffForEachCallbackData{ + fileCallback: cbFile, + errorTarget: &err, } handle := pointerHandles.Track(data) defer pointerHandles.Untrack(handle) - ecode := C._go_git_diff_foreach(diff.ptr, 1, intHunks, intLines, handle) + runtime.LockOSThread() + defer runtime.UnlockOSThread() + + ret := C._go_git_diff_foreach(diff.ptr, 1, intHunks, intLines, handle) runtime.KeepAlive(diff) - if ecode < 0 { - return data.Error + if ret == C.int(ErrorCodeUser) && err != nil { + return err + } + if ret < 0 { + return MakeGitError(ret) } + return nil } -//export diffForEachFileCb -func diffForEachFileCb(delta *C.git_diff_delta, progress C.float, handle unsafe.Pointer) int { +//export diffForEachFileCallback +func diffForEachFileCallback(delta *C.git_diff_delta, progress C.float, handle unsafe.Pointer) C.int { payload := pointerHandles.Get(handle) - data, ok := payload.(*diffForEachData) + data, ok := payload.(*diffForEachCallbackData) if !ok { panic("could not retrieve data for handle") } - data.HunkCallback = nil - if data.FileCallback != nil { - cb, err := data.FileCallback(diffDeltaFromC(delta), float64(progress)) + data.hunkCallback = nil + if data.fileCallback != nil { + cb, err := data.fileCallback(diffDeltaFromC(delta), float64(progress)) if err != nil { - data.Error = err - return -1 + *data.errorTarget = err + return C.int(ErrorCodeUser) } - data.HunkCallback = cb + data.hunkCallback = cb } - return 0 + return C.int(ErrorCodeOK) } type DiffForEachHunkCallback func(DiffHunk) (DiffForEachLineCallback, error) -//export diffForEachHunkCb -func diffForEachHunkCb(delta *C.git_diff_delta, hunk *C.git_diff_hunk, handle unsafe.Pointer) int { +//export diffForEachHunkCallback +func diffForEachHunkCallback(delta *C.git_diff_delta, hunk *C.git_diff_hunk, handle unsafe.Pointer) C.int { payload := pointerHandles.Get(handle) - data, ok := payload.(*diffForEachData) + data, ok := payload.(*diffForEachCallbackData) if !ok { panic("could not retrieve data for handle") } - data.LineCallback = nil - if data.HunkCallback != nil { - cb, err := data.HunkCallback(diffHunkFromC(hunk)) + data.lineCallback = nil + if data.hunkCallback != nil { + cb, err := data.hunkCallback(diffHunkFromC(hunk)) if err != nil { - data.Error = err - return -1 + *data.errorTarget = err + return C.int(ErrorCodeUser) } - data.LineCallback = cb + data.lineCallback = cb } - return 0 + return C.int(ErrorCodeOK) } type DiffForEachLineCallback func(DiffLine) error -//export diffForEachLineCb -func diffForEachLineCb(delta *C.git_diff_delta, hunk *C.git_diff_hunk, line *C.git_diff_line, handle unsafe.Pointer) int { +//export diffForEachLineCallback +func diffForEachLineCallback(delta *C.git_diff_delta, hunk *C.git_diff_hunk, line *C.git_diff_line, handle unsafe.Pointer) C.int { payload := pointerHandles.Get(handle) - data, ok := payload.(*diffForEachData) + data, ok := payload.(*diffForEachCallbackData) if !ok { panic("could not retrieve data for handle") } - err := data.LineCallback(diffLineFromC(line)) + err := data.lineCallback(diffLineFromC(line)) if err != nil { - data.Error = err - return -1 + *data.errorTarget = err + return C.int(ErrorCodeUser) } - return 0 + return C.int(ErrorCodeOK) } func (diff *Diff) Patch(deltaIndex int) (*Patch, error) { @@ -586,93 +595,98 @@ var ( ErrDeltaSkip = errors.New("Skip delta") ) -type diffNotifyData struct { - Callback DiffNotifyCallback - Repository *Repository - Error error +type diffNotifyCallbackData struct { + callback DiffNotifyCallback + repository *Repository + errorTarget *error } -//export diffNotifyCb -func diffNotifyCb(_diff_so_far unsafe.Pointer, delta_to_add *C.git_diff_delta, matched_pathspec *C.char, handle unsafe.Pointer) int { +//export diffNotifyCallback +func diffNotifyCallback(_diff_so_far unsafe.Pointer, delta_to_add *C.git_diff_delta, matched_pathspec *C.char, handle unsafe.Pointer) C.int { diff_so_far := (*C.git_diff)(_diff_so_far) payload := pointerHandles.Get(handle) - data, ok := payload.(*diffNotifyData) + data, ok := payload.(*diffNotifyCallbackData) if !ok { panic("could not retrieve data for handle") } - if data != nil { - // We are not taking ownership of this diff pointer, so no finalizer is set. - diff := &Diff{ - ptr: diff_so_far, - repo: data.Repository, - runFinalizer: false, - } + if data == nil { + return C.int(ErrorCodeOK) + } - err := data.Callback(diff, diffDeltaFromC(delta_to_add), C.GoString(matched_pathspec)) + // We are not taking ownership of this diff pointer, so no finalizer is set. + diff := &Diff{ + ptr: diff_so_far, + repo: data.repository, + runFinalizer: false, + } - // Since the callback could theoretically keep a reference to the diff - // (which could be freed by libgit2 if an error occurs later during the - // diffing process), this converts a use-after-free (terrible!) into a nil - // dereference ("just" pretty bad). - diff.ptr = nil + err := data.callback(diff, diffDeltaFromC(delta_to_add), C.GoString(matched_pathspec)) - if err == ErrDeltaSkip { - return 1 - } else if err != nil { - data.Error = err - return -1 - } else { - return 0 - } - } - return 0 -} + // Since the callback could theoretically keep a reference to the diff + // (which could be freed by libgit2 if an error occurs later during the + // diffing process), this converts a use-after-free (terrible!) into a nil + // dereference ("just" pretty bad). + diff.ptr = nil -func diffOptionsToC(opts *DiffOptions, repo *Repository) (copts *C.git_diff_options) { - cpathspec := C.git_strarray{} - if opts != nil { - notifyData := &diffNotifyData{ - Callback: opts.NotifyCallback, - Repository: repo, - } + if err == ErrDeltaSkip { + return 1 + } + if err != nil { + *data.errorTarget = err + return C.int(ErrorCodeUser) + } - if opts.Pathspec != nil { - cpathspec.count = C.size_t(len(opts.Pathspec)) - cpathspec.strings = makeCStringsFromStrings(opts.Pathspec) - } + return C.int(ErrorCodeOK) +} - copts = &C.git_diff_options{ - version: C.GIT_DIFF_OPTIONS_VERSION, - flags: C.uint32_t(opts.Flags), - ignore_submodules: C.git_submodule_ignore_t(opts.IgnoreSubmodules), - pathspec: cpathspec, - context_lines: C.uint32_t(opts.ContextLines), - interhunk_lines: C.uint32_t(opts.InterhunkLines), - id_abbrev: C.uint16_t(opts.IdAbbrev), - max_size: C.git_off_t(opts.MaxSize), - old_prefix: C.CString(opts.OldPrefix), - new_prefix: C.CString(opts.NewPrefix), - } +func (opts *DiffOptions) toC(repo *Repository, errorTarget *error) *C.git_diff_options { + if opts == nil { + return nil + } - if opts.NotifyCallback != nil { - C._go_git_setup_diff_notify_callbacks(copts) - copts.payload = pointerHandles.Track(notifyData) + cpathspec := C.git_strarray{} + if opts.Pathspec != nil { + cpathspec.count = C.size_t(len(opts.Pathspec)) + cpathspec.strings = makeCStringsFromStrings(opts.Pathspec) + } + + copts := &C.git_diff_options{ + version: C.GIT_DIFF_OPTIONS_VERSION, + flags: C.uint32_t(opts.Flags), + ignore_submodules: C.git_submodule_ignore_t(opts.IgnoreSubmodules), + pathspec: cpathspec, + context_lines: C.uint32_t(opts.ContextLines), + interhunk_lines: C.uint32_t(opts.InterhunkLines), + id_abbrev: C.uint16_t(opts.IdAbbrev), + max_size: C.git_off_t(opts.MaxSize), + old_prefix: C.CString(opts.OldPrefix), + new_prefix: C.CString(opts.NewPrefix), + } + + if opts.NotifyCallback != nil { + notifyData := &diffNotifyCallbackData{ + callback: opts.NotifyCallback, + repository: repo, + errorTarget: errorTarget, } + C._go_git_setup_diff_notify_callbacks(copts) + copts.payload = pointerHandles.Track(notifyData) } - return + return copts } func freeDiffOptions(copts *C.git_diff_options) { - if copts != nil { - cpathspec := copts.pathspec - freeStrarray(&cpathspec) - C.free(unsafe.Pointer(copts.old_prefix)) - C.free(unsafe.Pointer(copts.new_prefix)) - if copts.payload != nil { - pointerHandles.Untrack(copts.payload) - } + if copts == nil { + return + } + cpathspec := copts.pathspec + freeStrarray(&cpathspec) + C.free(unsafe.Pointer(copts.old_prefix)) + C.free(unsafe.Pointer(copts.new_prefix)) + if copts.payload != nil { + pointerHandles.Untrack(copts.payload) } } @@ -688,17 +702,21 @@ func (v *Repository) DiffTreeToTree(oldTree, newTree *Tree, opts *DiffOptions) ( newPtr = newTree.cast_ptr } - copts := diffOptionsToC(opts, v) + var err error + copts := opts.toC(v, &err) defer freeDiffOptions(copts) runtime.LockOSThread() defer runtime.UnlockOSThread() - ecode := C.git_diff_tree_to_tree(&diffPtr, v.ptr, oldPtr, newPtr, copts) + ret := C.git_diff_tree_to_tree(&diffPtr, v.ptr, oldPtr, newPtr, copts) runtime.KeepAlive(oldTree) runtime.KeepAlive(newTree) - if ecode < 0 { - return nil, MakeGitError(ecode) + if ret == C.int(ErrorCodeUser) && err != nil { + return nil, err + } + if ret < 0 { + return nil, MakeGitError(ret) } return newDiffFromC(diffPtr, v), nil } @@ -711,17 +729,22 @@ func (v *Repository) DiffTreeToWorkdir(oldTree *Tree, opts *DiffOptions) (*Diff, oldPtr = oldTree.cast_ptr } - copts := diffOptionsToC(opts, v) + var err error + copts := opts.toC(v, &err) defer freeDiffOptions(copts) runtime.LockOSThread() defer runtime.UnlockOSThread() - ecode := C.git_diff_tree_to_workdir(&diffPtr, v.ptr, oldPtr, copts) + ret := C.git_diff_tree_to_workdir(&diffPtr, v.ptr, oldPtr, copts) runtime.KeepAlive(oldTree) - if ecode < 0 { - return nil, MakeGitError(ecode) + if ret == C.int(ErrorCodeUser) && err != nil { + return nil, err + } + if ret < 0 { + return nil, MakeGitError(ret) } + return newDiffFromC(diffPtr, v), nil } @@ -738,18 +761,23 @@ func (v *Repository) DiffTreeToIndex(oldTree *Tree, index *Index, opts *DiffOpti indexPtr = index.ptr } - copts := diffOptionsToC(opts, v) + var err error + copts := opts.toC(v, &err) defer freeDiffOptions(copts) runtime.LockOSThread() defer runtime.UnlockOSThread() - ecode := C.git_diff_tree_to_index(&diffPtr, v.ptr, oldPtr, indexPtr, copts) + ret := C.git_diff_tree_to_index(&diffPtr, v.ptr, oldPtr, indexPtr, copts) runtime.KeepAlive(oldTree) runtime.KeepAlive(index) - if ecode < 0 { - return nil, MakeGitError(ecode) + if ret == C.int(ErrorCodeUser) && err != nil { + return nil, err + } + if ret < 0 { + return nil, MakeGitError(ret) } + return newDiffFromC(diffPtr, v), nil } @@ -761,17 +789,22 @@ func (v *Repository) DiffTreeToWorkdirWithIndex(oldTree *Tree, opts *DiffOptions oldPtr = oldTree.cast_ptr } - copts := diffOptionsToC(opts, v) + var err error + copts := opts.toC(v, &err) defer freeDiffOptions(copts) runtime.LockOSThread() defer runtime.UnlockOSThread() - ecode := C.git_diff_tree_to_workdir_with_index(&diffPtr, v.ptr, oldPtr, copts) + ret := C.git_diff_tree_to_workdir_with_index(&diffPtr, v.ptr, oldPtr, copts) runtime.KeepAlive(oldTree) - if ecode < 0 { - return nil, MakeGitError(ecode) + if ret == C.int(ErrorCodeUser) && err != nil { + return nil, err } + if ret < 0 { + return nil, MakeGitError(ret) + } + return newDiffFromC(diffPtr, v), nil } @@ -783,25 +816,32 @@ func (v *Repository) DiffIndexToWorkdir(index *Index, opts *DiffOptions) (*Diff, indexPtr = index.ptr } - copts := diffOptionsToC(opts, v) + var err error + copts := opts.toC(v, &err) defer freeDiffOptions(copts) runtime.LockOSThread() defer runtime.UnlockOSThread() - ecode := C.git_diff_index_to_workdir(&diffPtr, v.ptr, indexPtr, copts) + ret := C.git_diff_index_to_workdir(&diffPtr, v.ptr, indexPtr, copts) runtime.KeepAlive(index) - if ecode < 0 { - return nil, MakeGitError(ecode) + if ret == C.int(ErrorCodeUser) && err != nil { + return nil, err } + if ret < 0 { + return nil, MakeGitError(ret) + } + return newDiffFromC(diffPtr, v), nil } // DiffBlobs performs a diff between two arbitrary blobs. You can pass // whatever file names you'd like for them to appear as in the diff. func DiffBlobs(oldBlob *Blob, oldAsPath string, newBlob *Blob, newAsPath string, opts *DiffOptions, fileCallback DiffForEachFileCallback, detail DiffDetail) error { - data := &diffForEachData{ - FileCallback: fileCallback, + var err error + data := &diffForEachCallbackData{ + fileCallback: fileCallback, + errorTarget: &err, } intHunks := C.int(0) @@ -833,17 +873,20 @@ func DiffBlobs(oldBlob *Blob, oldAsPath string, newBlob *Blob, newAsPath string, newBlobPath := C.CString(newAsPath) defer C.free(unsafe.Pointer(newBlobPath)) - copts := diffOptionsToC(opts, repo) + copts := opts.toC(repo, &err) defer freeDiffOptions(copts) runtime.LockOSThread() defer runtime.UnlockOSThread() - ecode := C._go_git_diff_blobs(oldBlobPtr, oldBlobPath, newBlobPtr, newBlobPath, copts, 1, intHunks, intLines, handle) + ret := C._go_git_diff_blobs(oldBlobPtr, oldBlobPath, newBlobPtr, newBlobPath, copts, 1, intHunks, intLines, handle) runtime.KeepAlive(oldBlob) runtime.KeepAlive(newBlob) - if ecode < 0 { - return MakeGitError(ecode) + if ret == C.int(ErrorCodeUser) && err != nil { + return err + } + if ret < 0 { + return MakeGitError(ret) } return nil @@ -864,56 +907,59 @@ type ApplyOptions struct { Flags uint } +type applyCallbackData struct { + options *ApplyOptions + errorTarget *error +} + //export hunkApplyCallback func hunkApplyCallback(_hunk *C.git_diff_hunk, _payload unsafe.Pointer) C.int { - opts, ok := pointerHandles.Get(_payload).(*ApplyOptions) + data, ok := pointerHandles.Get(_payload).(*applyCallbackData) if !ok { panic("invalid apply options payload") } - if opts.ApplyHunkCallback == nil { - return 0 + if data.options.ApplyHunkCallback == nil { + return C.int(ErrorCodeOK) } hunk := diffHunkFromC(_hunk) - apply, err := opts.ApplyHunkCallback(&hunk) + apply, err := data.options.ApplyHunkCallback(&hunk) if err != nil { - if gitError, ok := err.(*GitError); ok { - return C.int(gitError.Code) - } - return -1 - } else if apply { - return 0 - } else { + *data.errorTarget = err + return C.int(ErrorCodeUser) + } + + if !apply { return 1 } + return C.int(ErrorCodeOK) } //export deltaApplyCallback func deltaApplyCallback(_delta *C.git_diff_delta, _payload unsafe.Pointer) C.int { - opts, ok := pointerHandles.Get(_payload).(*ApplyOptions) + data, ok := pointerHandles.Get(_payload).(*applyCallbackData) if !ok { panic("invalid apply options payload") } - if opts.ApplyDeltaCallback == nil { - return 0 + if data.options.ApplyDeltaCallback == nil { + return C.int(ErrorCodeOK) } delta := diffDeltaFromC(_delta) - apply, err := opts.ApplyDeltaCallback(&delta) + apply, err := data.options.ApplyDeltaCallback(&delta) if err != nil { - if gitError, ok := err.(*GitError); ok { - return C.int(gitError.Code) - } - return -1 - } else if apply { - return 0 - } else { + *data.errorTarget = err + return C.int(ErrorCodeUser) + } + + if !apply { return 1 } + return C.int(ErrorCodeOK) } // DefaultApplyOptions returns default options for applying diffs or patches. @@ -925,14 +971,13 @@ func DefaultApplyOptions() (*ApplyOptions, error) { ecode := C.git_apply_options_init(&opts, C.GIT_APPLY_OPTIONS_VERSION) if int(ecode) != 0 { - return nil, MakeGitError(ecode) } return applyOptionsFromC(&opts), nil } -func (a *ApplyOptions) toC() *C.git_apply_options { +func (a *ApplyOptions) toC(errorTarget *error) *C.git_apply_options { if a == nil { return nil } @@ -943,13 +988,26 @@ func (a *ApplyOptions) toC() *C.git_apply_options { } if a.ApplyDeltaCallback != nil || a.ApplyHunkCallback != nil { - C._go_git_populate_apply_cb(opts) - opts.payload = pointerHandles.Track(a) + data := &applyCallbackData{ + options: a, + errorTarget: errorTarget, + } + C._go_git_populate_apply_callbacks(opts) + opts.payload = pointerHandles.Track(data) } return opts } +func freeApplyOptions(opts *C.git_apply_options) { + if opts == nil { + return + } + if opts.payload != nil { + pointerHandles.Untrack(opts.payload) + } +} + func applyOptionsFromC(opts *C.git_apply_options) *ApplyOptions { return &ApplyOptions{ Flags: uint(opts.flags), @@ -979,13 +1037,19 @@ func (v *Repository) ApplyDiff(diff *Diff, location ApplyLocation, opts *ApplyOp runtime.LockOSThread() defer runtime.UnlockOSThread() - cOpts := opts.toC() - ecode := C.git_apply(v.ptr, diff.ptr, C.git_apply_location_t(location), cOpts) + var err error + cOpts := opts.toC(&err) + defer freeApplyOptions(cOpts) + + ret := C.git_apply(v.ptr, diff.ptr, C.git_apply_location_t(location), cOpts) runtime.KeepAlive(v) runtime.KeepAlive(diff) runtime.KeepAlive(cOpts) - if ecode < 0 { - return MakeGitError(ecode) + if ret == C.int(ErrorCodeUser) && err != nil { + return err + } + if ret < 0 { + return MakeGitError(ret) } return nil @@ -996,14 +1060,20 @@ func (v *Repository) ApplyToTree(diff *Diff, tree *Tree, opts *ApplyOptions) (*I runtime.LockOSThread() defer runtime.UnlockOSThread() + var err error + cOpts := opts.toC(&err) + defer freeApplyOptions(cOpts) + var indexPtr *C.git_index - cOpts := opts.toC() - ecode := C.git_apply_to_tree(&indexPtr, v.ptr, tree.cast_ptr, diff.ptr, cOpts) + ret := C.git_apply_to_tree(&indexPtr, v.ptr, tree.cast_ptr, diff.ptr, cOpts) runtime.KeepAlive(diff) runtime.KeepAlive(tree) runtime.KeepAlive(cOpts) - if ecode != 0 { - return nil, MakeGitError(ecode) + if ret == C.int(ErrorCodeUser) && err != nil { + return nil, err + } + if ret < 0 { + return nil, MakeGitError(ret) } return newIndexFromC(indexPtr, v), nil -- cgit v1.2.3