From 2594f3f889f93196bc9707ca6962f4f773806f10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Tue, 6 May 2014 14:19:34 +0200 Subject: [PATCH 1/2] Odb: use a callback instead of a channel for ForEach A channel provides no way to specify whether we stopped sending data because of an error or because there is no more data. Therefore, make Odb.ForEach() take a callback with which the user is free to do whatever they need, letting us return en error. --- odb.go | 44 +++++++++++++++++++++++++++----------------- odb_test.go | 36 +++++++++++++++++++++++++++++++++++- 2 files changed, 62 insertions(+), 18 deletions(-) diff --git a/odb.go b/odb.go index abfa1cd..7076e20 100644 --- a/odb.go +++ b/odb.go @@ -84,30 +84,40 @@ func (v *Odb) Read(oid *Oid) (obj *OdbObject, err error) { return obj, nil } +type OdbForEachCallback func(id *Oid) error + +type foreachData struct { + callback OdbForEachCallback + err error +} + //export odbForEachCb func odbForEachCb(id *C.git_oid, payload unsafe.Pointer) int { - ch := *(*chan *Oid)(payload) - oid := newOidFromC(id) - // Because the channel is unbuffered, we never read our own data. If ch is - // readable, the user has sent something on it, which means we should - // abort. - select { - case ch <- oid: - case <-ch: - return -1 + data := (*foreachData)(payload) + + err := data.callback(newOidFromC(id)) + if err != nil { + data.err = err + return C.GIT_EUSER } + return 0 } -func (v *Odb) forEachWrap(ch chan *Oid) { - C._go_git_odb_foreach(v.ptr, unsafe.Pointer(&ch)) - close(ch) -} +func (v *Odb) ForEach(callback OdbForEachCallback) error { + data := foreachData { + callback: callback, + err: nil, + } -func (v *Odb) ForEach() chan *Oid { - ch := make(chan *Oid, 0) - go v.forEachWrap(ch) - return ch + ret := C._go_git_odb_foreach(v.ptr, unsafe.Pointer(&data)) + if ret == C.GIT_EUSER { + return data.err + } else if ret < 0 { + return MakeGitError(ret) + } + + return nil } // Hash determines the object-ID (sha1) of a data buffer. diff --git a/odb_test.go b/odb_test.go index 17b3ad2..14a3658 100644 --- a/odb_test.go +++ b/odb_test.go @@ -3,6 +3,7 @@ package git import ( "io" "os" + "errors" "testing" ) @@ -48,7 +49,7 @@ parent 66e1c476199ebcd3e304659992233132c5a52c6c author John Doe 1390682018 +0000 committer John Doe 1390682018 +0000 -Initial commit.`; +Initial commit.` oid, error := odb.Hash([]byte(str), ObjectCommit) checkFatal(t, error) @@ -60,3 +61,36 @@ Initial commit.`; t.Fatal("Hash and write Oids are different") } } + +func TestOdbForeach(t *testing.T) { + repo := createTestRepo(t) + defer os.RemoveAll(repo.Workdir()) + _, _ = seedTestRepo(t, repo) + + odb, err := repo.Odb() + checkFatal(t, err) + + expect := 3 + count := 0 + err = odb.ForEach(func(id *Oid) error { + count++ + return nil + }) + + checkFatal(t, err) + if count != expect { + t.Fatalf("Expected %v objects, got %v") + } + + expect = 1 + count = 0 + to_return := errors.New("not really an error") + err = odb.ForEach(func(id *Oid) error { + count++ + return to_return + }) + + if err != to_return { + t.Fatalf("Odb.ForEach() did not return the expected error, got %v", err) + } +} From 7e3c361ac4c97f48d80d7ca63358e92ea464a087 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Tue, 6 May 2014 14:43:38 +0200 Subject: [PATCH 2/2] Packbuilder: use a callback for ForEach instead of a channel Channels provide no means to report an error. Closing a channel could mean anything. This is particularly important when dealing with IO, which we do quite often in the pack builder. Use ForEach which returns an error instead. --- packbuilder.go | 58 +++++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 31 deletions(-) diff --git a/packbuilder.go b/packbuilder.go index 70c4530..666f5c4 100644 --- a/packbuilder.go +++ b/packbuilder.go @@ -94,55 +94,51 @@ func (pb *Packbuilder) WriteToFile(name string, mode os.FileMode) error { } func (pb *Packbuilder) Write(w io.Writer) error { - ch, stop := pb.ForEach() - for slice := range ch { + return pb.ForEach(func(slice []byte) error { _, err := w.Write(slice) - if err != nil { - close(stop) - return err - } - } - return nil + return err + }) } func (pb *Packbuilder) Written() uint32 { return uint32(C.git_packbuilder_written(pb.ptr)) } +type PackbuilderForeachCallback func([]byte) error type packbuilderCbData struct { - ch chan<- []byte - stop <-chan bool + callback PackbuilderForeachCallback + err error } //export packbuilderForEachCb func packbuilderForEachCb(buf unsafe.Pointer, size C.size_t, payload unsafe.Pointer) int { data := (*packbuilderCbData)(payload) - ch := data.ch - stop := data.stop - slice := C.GoBytes(buf, C.int(size)) - select { - case <- stop: - return -1 - case ch <- slice: + + err := data.callback(slice) + if err != nil { + data.err = err + return C.GIT_EUSER } return 0 } -func (pb *Packbuilder) forEachWrap(data *packbuilderCbData) { - C._go_git_packbuilder_foreach(pb.ptr, unsafe.Pointer(data)) - close(data.ch) -} +// ForEach repeatedly calls the callback with new packfile data until +// there is no more data or the callback returns an error +func (pb *Packbuilder) ForEach(callback PackbuilderForeachCallback) error { + data := packbuilderCbData{ + callback: callback, + err: nil, + } -// Foreach sends the packfile as slices through the "data" channel. If -// you want to stop the pack-building process (e.g. there's an error -// writing to the output), close or write a value into the "stop" -// channel. -func (pb *Packbuilder) ForEach() (<-chan []byte, chan<- bool) { - ch := make(chan []byte) - stop := make(chan bool) - data := packbuilderCbData{ch, stop} - go pb.forEachWrap(&data) - return ch, stop + err := C._go_git_packbuilder_foreach(pb.ptr, unsafe.Pointer(&data)) + if err == C.GIT_EUSER { + return data.err + } + if err < 0 { + return MakeGitError(err) + } + + return nil }