cleanup and refactor diff / patch

diff.go

@@ -12,15 +12,17 @@ import (
 )
 
 type DiffFlag int
+
 const (
-	DiffFlagBinary = DiffFlag(C.GIT_DIFF_FLAG_BINARY)
+	DiffFlagBinary    DiffFlag = C.GIT_DIFF_FLAG_BINARY
 	DiffFlagNotBinary          = C.GIT_DIFF_FLAG_NOT_BINARY
 	DiffFlagValidOid           = C.GIT_DIFF_FLAG_VALID_OID
 )
 
 type Delta int
+
 const (
-	DeltaUnmodified = Delta(C.GIT_DELTA_UNMODIFIED)
+	DeltaUnmodified Delta = C.GIT_DELTA_UNMODIFIED
 	DeltaAdded            = C.GIT_DELTA_ADDED
 	DeltaDeleted          = C.GIT_DELTA_DELETED
 	DeltaModified         = C.GIT_DELTA_MODIFIED
@@ -32,8 +34,9 @@ const (
 )
 
 type DiffLineType int
+
 const (
-	DiffLineContext = DiffLineType(C.GIT_DIFF_LINE_CONTEXT)
+	DiffLineContext      DiffLineType = C.GIT_DIFF_LINE_CONTEXT
 	DiffLineAddition                  = C.GIT_DIFF_LINE_ADDITION
 	DiffLineDeletion                  = C.GIT_DIFF_LINE_DELETION
 	DiffLineContextEOFNL              = C.GIT_DIFF_LINE_CONTEXT_EOFNL
@@ -53,7 +56,7 @@ type DiffFile struct {
 	Mode  uint16
 }
 
-func newDiffFile(file *C.git_diff_file) *DiffFile {
+func newDiffFileFromC(file *C.git_diff_file) *DiffFile {
 	return &DiffFile{
 		Path:  C.GoString(file.path),
 		Oid:   newOidFromC(&file.oid),
@@ -71,13 +74,13 @@ type DiffDelta struct {
 	NewFile    *DiffFile
 }
 
-func newDiffDelta(delta *C.git_diff_delta) *DiffDelta {
+func newDiffDeltaFromC(delta *C.git_diff_delta) *DiffDelta {
 	return &DiffDelta{
 		Status:     Delta(delta.status),
 		Flags:      DiffFlag(delta.flags),
 		Similarity: uint16(delta.similarity),
-		OldFile: newDiffFile(&delta.old_file),
-		NewFile: newDiffFile(&delta.new_file),
+		OldFile:    newDiffFileFromC(&delta.old_file),
+		NewFile:    newDiffFileFromC(&delta.new_file),
 	}
 }
 
@@ -90,14 +93,14 @@ type DiffHunk struct {
 	DiffDelta
 }
 
-func newDiffHunk(delta *C.git_diff_delta, hunk *C.git_diff_hunk) *DiffHunk {
+func newDiffHunkFromC(delta *C.git_diff_delta, hunk *C.git_diff_hunk) *DiffHunk {
 	return &DiffHunk{
 		OldStart:  int(hunk.old_start),
 		OldLines:  int(hunk.old_lines),
 		NewStart:  int(hunk.new_start),
 		NewLines:  int(hunk.new_lines),
 		Header:    C.GoStringN(&hunk.header[0], C.int(hunk.header_len)),
-		DiffDelta: *newDiffDelta(delta),
+		DiffDelta: *newDiffDeltaFromC(delta),
 	}
 }
 
@@ -110,14 +113,14 @@ type DiffLine struct {
 	DiffHunk
 }
 
-func newDiffLine(delta *C.git_diff_delta, hunk *C.git_diff_hunk, line *C.git_diff_line) *DiffLine {
+func newDiffLineFromC(delta *C.git_diff_delta, hunk *C.git_diff_hunk, line *C.git_diff_line) *DiffLine {
 	return &DiffLine{
 		Origin:    DiffLineType(line.origin),
 		OldLineno: int(line.old_lineno),
 		NewLineno: int(line.new_lineno),
 		NumLines:  int(line.num_lines),
 		Content:   C.GoStringN(line.content, C.int(line.content_len)),
-		DiffHunk: *newDiffHunk(delta, hunk),
+		DiffHunk:  *newDiffHunkFromC(delta, hunk),
 	}
 }
 
@@ -125,7 +128,7 @@ type Diff struct {
 	ptr *C.git_diff
 }
 
-func newDiff(ptr *C.git_diff) *Diff {
+func newDiffFromC(ptr *C.git_diff) *Diff {
 	if ptr == nil {
 		return nil
 	}
@@ -138,100 +141,165 @@ func newDiff(ptr *C.git_diff) *Diff {
 	return diff
 }
 
-func (diff *Diff) Free() {
+func (diff *Diff) Free() error {
+	if diff.ptr != nil {
+		return ErrInvalid
+	}
 	runtime.SetFinalizer(diff, nil)
 	C.git_diff_free(diff.ptr)
+	return nil
 }
 
-func (diff *Diff) forEachFileWrap(ch chan *DiffDelta) {
-	C._go_git_diff_foreach(diff.ptr, 1, 0, 0, unsafe.Pointer(&ch))
-	close(ch)
+type diffForEachFileData struct {
+	Callback DiffForEachFileCallback
+	Error    error
 }
 
-func (diff *Diff) ForEachFile() chan *DiffDelta {
-	ch := make(chan *DiffDelta, 0)
-	go diff.forEachFileWrap(ch)
-	return ch
+func (diff *Diff) ForEachFile(cb DiffForEachFileCallback) error {
+	if diff.ptr != nil {
+		return ErrInvalid
+	}
+
+	data := &diffForEachFileData{
+		Callback: cb,
+	}
+	ecode := C._go_git_diff_foreach(diff.ptr, 1, 0, 0, unsafe.Pointer(&data))
+	if ecode < 0 {
+		return data.Error
+	}
+	return nil
 }
 
 //export diffForEachFileCb
 func diffForEachFileCb(delta *C.git_diff_delta, progress C.float, payload unsafe.Pointer) int {
-	ch := *(*chan *DiffDelta)(payload)
+	data := *diffForEachFileData(payload)
 
-	select {
-	case ch <-newDiffDelta(delta):
-	case <-ch:
+	err := data.Callback(newDiffDeltaFromC(delta))
+	if err != nil {
+		data.Error = err
 		return -1
 	}
 
 	return 0
 }
 
-func (diff *Diff) forEachHunkWrap(ch chan *DiffHunk) {
-	C._go_git_diff_foreach(diff.ptr, 0, 1, 0, unsafe.Pointer(&ch))
-	close(ch)
+type diffForEachHunkData struct {
+	Callback DiffForEachHunkCallback
+	Error    error
 }
 
-func (diff *Diff) ForEachHunk() chan *DiffHunk {
-	ch := make(chan *DiffHunk, 0)
-	go diff.forEachHunkWrap(ch)
-	return ch
+type DiffForEachHunkCallback func(*DiffHunk) error
+
+func (diff *Diff) ForEachHunk(cb DiffForEachHunkCallback) error {
+	if diff.ptr != nil {
+		return ErrInvalid
+	}
+	data := &diffForEachHunkData{
+		Callback: cb,
+	}
+	ecode := C._go_git_diff_foreach(diff.ptr, 0, 1, 0, unsafe.Pointer(data))
+	if ecode < 0 {
+		return data.Error
+	}
+	return nil
 }
 
 //export diffForEachHunkCb
 func diffForEachHunkCb(delta *C.git_diff_delta, hunk *C.git_diff_hunk, payload unsafe.Pointer) int {
-	ch := *(*chan *DiffHunk)(payload)
+	data := *diffForEachHunkData(payload)
 
-	select {
-	case ch <-newDiffHunk(delta, hunk):
-	case <-ch:
+	err := data.Callback(newDiffHunkFromC(delta, hunk))
+	if err < 0 {
+		data.Error = err
 		return -1
 	}
 
 	return 0
 }
 
-func (diff *Diff) forEachLineWrap(ch chan *DiffLine) {
-	C._go_git_diff_foreach(diff.ptr, 0, 0, 1, unsafe.Pointer(&ch))
-	close(ch)
+type diffForEachLineData struct {
+	Callback DiffForEachLineCallback
+	Error    error
 }
 
-func (diff *Diff) ForEachLine() chan *DiffLine {
-	ch := make(chan *DiffLine, 0)
-	go diff.forEachLineWrap(ch)
-	return ch
+type DiffForEachLineCallback func(*DiffLine) error
+
+func (diff *Diff) ForEachLine(cb DiffForEachLineCallback) error {
+	if diff.ptr != nil {
+		return ErrInvalid
+	}
+
+	data := &diffForEachLineData{
+		Callback: cb,
+	}
+
+	ecode := C._go_git_diff_foreach(diff.ptr, 0, 0, 1, unsafe.Pointer(data))
+	if ecode < 0 {
+		return data.Error
+	}
+	return nil
 }
 
 //export diffForEachLineCb
 func diffForEachLineCb(delta *C.git_diff_delta, hunk *C.git_diff_hunk, line *C.git_diff_line, payload unsafe.Pointer) int {
-	ch := *(*chan *DiffLine)(payload)
 
-	select {
-	case ch <-newDiffLine(delta, hunk, line):
-	case <-ch:
+	data := *diffForEachLineData(payload)
+
+	err := data.Callback(newDiffLineFromC(delta, hunk, line))
+	if err != nil {
+		data.Error = err
 		return -1
 	}
 
 	return 0
 }
 
-func (diff *Diff) NumDeltas() int {
-	return int(C.git_diff_num_deltas(diff.ptr))
+func (diff *Diff) NumDeltas() (int, error) {
+	if diff.ptr != nil {
+		return -1, ErrInvalid
+	}
+	return int(C.git_diff_num_deltas(diff.ptr)), nil
 }
 
-func (diff *Diff) GetDelta(index int) *DiffDelta {
+func (diff *Diff) GetDelta(index int) (*DiffDelta, error) {
+	if diff.ptr != nil {
+		return nil, ErrInvalid
+	}
 	ptr := C.git_diff_get_delta(diff.ptr, C.size_t(index))
 	if ptr == nil {
 		return nil
 	}
 
-	return newDiffDelta(ptr)
+	return newDiffDeltaFromC(ptr), nil
 }
 
-func (diff *Diff) Patch(deltaIndex int) *Patch {
+func (diff *Diff) Patch(deltaIndex int) (*Patch, error) {
+	if diff.ptr != nil {
+		return nil, ErrInvalid
+	}
 	var patchPtr *C.git_patch
 
-	C.git_patch_from_diff(&patchPtr, diff.ptr, C.size_t(deltaIndex))
-
-	return newPatch(patchPtr)
+	ecode := C.git_patch_from_diff(&patchPtr, diff.ptr, C.size_t(deltaIndex))
+	if ecode < 0 {
+		return nil, MakeGitError(ecode)
+	}
+
+	return newPatchFromC(patchPtr), nil
+}
+
+func (v *Repository) DiffTreeToTree(oldTree, newTree *Tree) *Diff {
+	var diffPtr *C.git_diff
+	var oldPtr, newPtr *C.git_tree
+
+	if oldTree != nil {
+		oldPtr = oldTree.gitObject.ptr
+	}
+
+	if newTree != nil {
+		newPtr = newTree.gitObject.ptr
+	}
+
+	C.git_diff_tree_to_tree(&diffPtr, v.ptr, oldPtr, newPtr, nil)
+
+	return newDiff(diffPtr)
 }

diff_test.go

@@ -0,0 +1,41 @@
+package git
+
+import (
+	"testing"
+)
+
+func TestDiffTreeToTree(t *testing.T) {
+	repo := createTestRepo(t)
+	defer repo.Free()
+	defer os.RemoveAll(repo.Workdir())
+
+	_, originalTreeId := seedTestRepo(t, repo)
+	originalTree, err := repo.LookupTree(originalTreeId)
+
+	checkFatal(t, err)
+	updateReadme(t, repo, "file changed\n")
+
+	_, newTreeId := seedTestRepo(t, repo)
+	newTree, err := repo.LookupTree(newTreeId)
+	checkFatal(t, err)
+
+	diff, err := repo.DiffTreeToTree(originalTreeId, newTreeId)
+	checkFatal(t, err)
+
+	files := make([]string, 0)
+
+	err := diff.ForEachFile(func(file *DiffFile) error {
+		files = append(files, file.Path)
+		return nil
+	})
+
+	checkFatal(t, err)
+
+	if len(files) != 0 {
+		t.Fatal("Incorrect number of files in diff")
+	}
+
+	if files[0] != "README" {
+		t.Fatal("File in diff was expected to be README")
+	}
+}

git.go

@@ -10,8 +10,8 @@ import (
 	"bytes"
 	"errors"
 	"runtime"
-	"unsafe"
 	"strings"
+	"unsafe"
 )
 
 const (
@@ -22,6 +22,7 @@ const (
 
 var (
 	ErrIterOver = errors.New("Iteration is over")
+	ErrInvalid  = errors.New("Invalid state for operation")
 )
 
 func init() {
@@ -93,7 +94,7 @@ func (oid *Oid) Equal(oid2 *Oid) bool {
 }
 
 func (oid *Oid) IsZero() bool {
-	for _, a := range(oid.bytes) {
+	for _, a := range oid.bytes {
 		if a != '0' {
 			return false
 		}
@@ -147,14 +148,14 @@ func LastError() error {
 }
 
 func cbool(b bool) C.int {
-	if (b) {
+	if b {
 		return C.int(1)
 	}
 	return C.int(0)
 }
 
 func ucbool(b bool) C.uint {
-	if (b) {
+	if b {
 		return C.uint(1)
 	}
 	return C.uint(0)

git_test.go

@@ -1,8 +1,8 @@
 package git
 
 import (
-	"testing"
 	"io/ioutil"
+	"testing"
 	"time"
 )
 
@@ -45,3 +45,31 @@ func seedTestRepo(t *testing.T, repo *Repository) (*Oid, *Oid) {
 	return commitId, treeId
 }
 
+func updateReadme(t *testing.T, repo *Repository, content string) (*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),
+	}
+
+	tmpfile := "README"
+	err = ioutil.WriteFile(repo.Path()+"/"+tmpfile, []byte(content), 0644)
+	checkFatal(t, err)
+
+	idx, err := repo.Index()
+	checkFatal(t, err)
+	err = idx.AddByPath("README")
+	checkFatal(t, err)
+	treeId, err := idx.WriteTree()
+	checkFatal(t, err)
+
+	message := "This is a commit\n"
+	tree, err := repo.LookupTree(treeId)
+	checkFatal(t, err)
+	commitId, err := repo.CreateCommit("HEAD", sig, sig, message, tree)
+	checkFatal(t, err)
+
+	return commitId, treeId
+}

patch.go

@@ -12,7 +12,7 @@ type Patch struct {
 	ptr *C.git_patch
 }
 
-func newPatch(ptr *C.git_patch) *Patch {
+func newPatchFromC(ptr *C.git_patch) *Patch {
 	if ptr == nil {
 		return nil
 	}
@@ -25,13 +25,20 @@ func newPatch(ptr *C.git_patch) *Patch {
 	return patch
 }
 
-func (patch *Patch) Free() {
+func (patch *Patch) Free() error {
+	if patch.ptr == nil {
+		return ErrInvalid
+	}
 	runtime.SetFinalizer(patch, nil)
 	C.git_patch_free(patch.ptr)
+	return nil
 }
 
-func (patch *Patch) String() string {
+func (patch *Patch) String() (string, error) {
+	if diff.ptr != nil {
+		return "", ErrInvalid
+	}
 	var cptr *C.char
 	C.git_patch_to_str(&cptr, patch.ptr)
-	return C.GoString(cptr)
+	return C.GoString(cptr), nil
 }

repository.go

@@ -255,23 +255,6 @@ func (v *Repository) CreateCommit(
 	return oid, nil
 }
 
-func (v *Repository) DiffTreeToTree(oldTree, newTree *Tree) *Diff {
-	var diffPtr *C.git_diff
-	var oldPtr, newPtr *C.git_tree
-
-	if oldTree != nil {
-		oldPtr = oldTree.gitObject.ptr
-	}
-
-	if newTree != nil {
-		newPtr = newTree.gitObject.ptr
-	}
-
-	C.git_diff_tree_to_tree(&diffPtr, v.ptr, oldPtr, newPtr, nil)
-
-	return newDiff(diffPtr)
-}
-
 func (v *Odb) Free() {
 	runtime.SetFinalizer(v, nil)
 	C.git_odb_free(v.ptr)