Addressed review feedback and added a test

Author: lhchavez

mempack.go

@@ -32,15 +32,13 @@ func NewMempack(odb *Odb) (mempack *Mempack, err error) {
 	if ret < 0 {
 		return nil, MakeGitError(ret)
 	}
-	runtime.SetFinalizer(mempack, (*Mempack).Free)
 
 	ret = C.git_odb_add_backend(odb.ptr, mempack.ptr, C.int(999))
 	if ret < 0 {
 		// Since git_odb_add_alternate() takes ownership of the ODB backend, the
 		// only case in which we free the mempack's memory is if it fails to be
-		// added to the ODB. Mempack.Free() is actually just a no-op.
+		// added to the ODB.
 		C._go_git_odb_backend_free(mempack.ptr)
-		mempack.Free()
 		return nil, MakeGitError(ret)
 	}
 
@@ -66,16 +64,12 @@ func (mempack *Mempack) Dump(repository *Repository) ([]byte, error) {
 	runtime.LockOSThread()
 	defer runtime.UnlockOSThread()
 
-	var repoPtr *C.git_repository
-	if repository != nil {
-		repoPtr = repository.ptr
-	}
-
-	ret := C.git_mempack_dump(&buf, repoPtr, mempack.ptr)
+	ret := C.git_mempack_dump(&buf, repository.ptr, mempack.ptr)
+	runtime.KeepAlive(repository)
 	if ret < 0 {
 		return nil, MakeGitError(ret)
 	}
-	defer C.git_buf_free(&buf)
+	defer C.git_buf_dispose(&buf)
 
 	return C.GoBytes(unsafe.Pointer(buf.ptr), C.int(buf.size)), nil
 }
@@ -85,13 +79,5 @@ func (mempack *Mempack) Dump(repository *Repository) ([]byte, error) {
 // This assumes that Mempack.Dump has been called before to store all the
 // queued objects into a single packfile.
 func (mempack *Mempack) Reset() {
-	runtime.LockOSThread()
-	defer runtime.UnlockOSThread()
-
 	C.git_mempack_reset(mempack.ptr)
 }
-
-// Free frees the mempack and its resources.
-func (mempack *Mempack) Free() {
-	runtime.SetFinalizer(mempack, nil)
-}

mempack_test.go

@@ -0,0 +1,60 @@
+package git
+
+import (
+	"bytes"
+	"testing"
+)
+
+func TestMempack(t *testing.T) {
+	t.Parallel()
+
+	odb, err := NewOdb()
+	checkFatal(t, err)
+
+	repo, err := NewRepositoryWrapOdb(odb)
+	checkFatal(t, err)
+
+	mempack, err := NewMempack(odb)
+	checkFatal(t, err)
+
+	id, err := odb.Write([]byte("hello, world!"), ObjectBlob)
+	checkFatal(t, err)
+
+	expectedId, err := NewOid("30f51a3fba5274d53522d0f19748456974647b4f")
+	checkFatal(t, err)
+	if !expectedId.Equal(id) {
+		t.Errorf("mismatched id. expected %v, got %v", expectedId.String(), id.String())
+	}
+
+	// The object should be available from the odb.
+	{
+		obj, err := odb.Read(expectedId)
+		checkFatal(t, err)
+		defer obj.Free()
+	}
+
+	data, err := mempack.Dump(repo)
+	checkFatal(t, err)
+
+	expectedData := []byte{
+		0x50, 0x41, 0x43, 0x4b, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00,
+		0x02, 0x9d, 0x08, 0x82, 0x3b, 0xd8, 0xa8, 0xea, 0xb5, 0x10, 0xad, 0x6a,
+		0xc7, 0x5c, 0x82, 0x3c, 0xfd, 0x3e, 0xd3, 0x1e,
+	}
+	if !bytes.Equal(expectedData, data) {
+		t.Errorf("mismatched mempack data. expected %v, got %v", expectedData, data)
+	}
+
+	mempack.Reset()
+
+	// After the reset, the object should now be unavailable.
+	{
+		obj, err := odb.Read(expectedId)
+		if err == nil {
+			t.Errorf("object %s unexpectedly found", obj.Id().String())
+			obj.Free()
+		} else if !IsErrorCode(err, ErrNotFound) {
+			t.Errorf("unexpected error %v", err)
+		}
+	}
+}