Assert roundtrip for patch formats

Re-parse our string representation and make sure it is equal. This is
important for binary patches due to inconsistencies in DEFLATE
implementations, but we do it for text patches too.

Author: bluekeyes

gitdiff/gitdiff.go

@@ -137,9 +137,11 @@ func (f *File) String() string {
 		} else {
 			diff.WriteString("GIT binary patch\n")
 			diff.WriteString(f.BinaryFragment.String())
+			diff.WriteByte('\n')
+
 			if f.ReverseBinaryFragment != nil {
-				diff.WriteByte('\n')
 				diff.WriteString(f.ReverseBinaryFragment.String())
+				diff.WriteByte('\n')
 			}
 		}
 	}
@@ -402,35 +404,10 @@ func (f *BinaryFragment) String() string {
 	return diff.String()
 }
 
-// TODO(bkeyes): The 'compress/flate' package does not produce minimal output
-// streams. Instead of flagging that the last block of data represents the end
-// of the stream, it always writes a final empty block to mark the end. Git's
-// implementation using the 'zlib' C library does not do this, which means that
-// what we produce for binary patches does not match the input, even though it
-// is valid.
-//
-// This is mostly a problem for my tests, where I compare the input and output
-// bytes. This comparison isn't required, but is helpful to catch invalid
-// output that might otherwise still parse.
-//
-// Options for fixing this:
-//
-//  1. Fix the tests to compare parsed objects instead of raw patches, at least
-//     for binary patches. This means writing something to do reasonable
-//     comparisons of File structs.
-//
-//  2. Add my own deflate function. By default, Git appears to use no
-//     compression on binary patch data, which means "delfate" is just adding
-//     the appropriate headers and checksums around the data. This would fix my
-//     tests but means we could never emit compressed data, so we'd differ from
-//     Git in other situations.
-//
-// Either way, there will be situations in which re-formatted binary patches
-// differ from the original inputs.
 func deflateBinaryChunk(data []byte) []byte {
 	var b bytes.Buffer
 
-	zw, _ := zlib.NewWriterLevel(&b, zlib.NoCompression)
+	zw := zlib.NewWriter(&b)
 	_, _ = zw.Write(data)
 	_ = zw.Close()
 

gitdiff/gitdiff_string_test.go

@@ -2,11 +2,13 @@ package gitdiff
 
 import (
 	"bytes"
+	"fmt"
 	"os"
+	"slices"
 	"testing"
 )
 
-func TestFile_String(t *testing.T) {
+func TestParseRoundtrip(t *testing.T) {
 	sources := []string{
 		"testdata/string/binary_modify.patch",
 		"testdata/string/binary_new.patch",
@@ -35,6 +37,11 @@ func TestFile_String(t *testing.T) {
 		if string(b) != str {
 			t.Errorf("%s: incorrect patch\nexpected: %q\n  actual: %q\n", src, string(b), str)
 		}
+
+		reparsed := assertParseSingleFile(t, fmt.Sprintf("Parse(%q).String()", src), []byte(str))
+
+		// TODO(bkeyes): include source in these messages (via subtest?)
+		assertFilesEqual(t, original, reparsed)
 	}
 }
 
@@ -49,19 +56,91 @@ func assertParseSingleFile(t *testing.T, src string, b []byte) *File {
 	return files[0]
 }
 
-/*
-func TestDecode(t *testing.T) {
-	actual := []byte("cmV-O")
-	mine := []byte("cmV)N")
+func assertFilesEqual(t *testing.T, expected, actual *File) {
+	assertEqual(t, expected.OldName, actual.OldName, "OldName")
+	assertEqual(t, expected.NewName, actual.NewName, "NewName")
+
+	assertEqual(t, expected.IsNew, actual.IsNew, "IsNew")
+	assertEqual(t, expected.IsDelete, actual.IsDelete, "IsDelete")
+	assertEqual(t, expected.IsCopy, actual.IsCopy, "IsCopy")
+	assertEqual(t, expected.IsRename, actual.IsRename, "IsRename")
+
+	assertEqual(t, expected.OldMode, actual.OldMode, "OldMode")
+	assertEqual(t, expected.NewMode, actual.NewMode, "NewMode")
+
+	assertEqual(t, expected.OldOIDPrefix, actual.OldOIDPrefix, "OldOIDPrefix")
+	assertEqual(t, expected.NewOIDPrefix, actual.NewOIDPrefix, "NewOIDPrefix")
+	assertEqual(t, expected.Score, actual.Score, "Score")
+
+	if len(expected.TextFragments) == len(actual.TextFragments) {
+		for i := range expected.TextFragments {
+			prefix := fmt.Sprintf("TextFragments[%d].", i)
+			ef := expected.TextFragments[i]
+			af := actual.TextFragments[i]
+
+			assertEqual(t, ef.Comment, af.Comment, prefix+"Comment")
+
+			assertEqual(t, ef.OldPosition, af.OldPosition, prefix+"OldPosition")
+			assertEqual(t, ef.OldLines, af.OldLines, prefix+"OldLines")
+
+			assertEqual(t, ef.NewPosition, af.NewPosition, prefix+"NewPosition")
+			assertEqual(t, ef.NewLines, af.NewLines, prefix+"NewLines")
+
+			assertEqual(t, ef.LinesAdded, af.LinesAdded, prefix+"LinesAdded")
+			assertEqual(t, ef.LinesDeleted, af.LinesDeleted, prefix+"LinesDeleted")
+
+			assertEqual(t, ef.LeadingContext, af.LeadingContext, prefix+"LeadingContext")
+			assertEqual(t, ef.TrailingContext, af.TrailingContext, prefix+"TrailingContext")
+
+			if !slices.Equal(ef.Lines, af.Lines) {
+				t.Errorf("%sLines: expected %#v, actual %#v", prefix, ef.Lines, af.Lines)
+			}
+		}
+	} else {
+		t.Errorf("TextFragments: expected length %d, actual length %d", len(expected.TextFragments), len(actual.TextFragments))
+	}
+
+	assertEqual(t, expected.IsBinary, actual.IsBinary, "IsBinary")
+
+	if expected.BinaryFragment != nil {
+		if actual.BinaryFragment == nil {
+			t.Errorf("BinaryFragment: expected non-nil, actual is nil")
+		} else {
+			ef := expected.BinaryFragment
+			af := expected.BinaryFragment
+
+			assertEqual(t, ef.Method, af.Method, "BinaryFragment.Method")
+			assertEqual(t, ef.Size, af.Size, "BinaryFragment.Size")
+
+			if !slices.Equal(ef.Data, af.Data) {
+				t.Errorf("BinaryFragment.Data: expected %#v, actual %#v", ef.Data, af.Data)
+			}
+		}
+	} else if actual.BinaryFragment != nil {
+		t.Errorf("BinaryFragment: expected nil, actual is non-nil")
+	}
 
-	dst := make([]byte, 4)
+	if expected.ReverseBinaryFragment != nil {
+		if actual.ReverseBinaryFragment == nil {
+			t.Errorf("ReverseBinaryFragment: expected non-nil, actual is nil")
+		} else {
+			ef := expected.ReverseBinaryFragment
+			af := expected.ReverseBinaryFragment
 
-	base85Decode(dst, actual)
-	t.Logf("actual: %x / %b", dst, dst)
+			assertEqual(t, ef.Method, af.Method, "ReverseBinaryFragment.Method")
+			assertEqual(t, ef.Size, af.Size, "ReverseBinaryFragment.Size")
 
-	base85Decode(dst, mine)
-	t.Logf("  mine: %x / %b", dst, dst)
+			if !slices.Equal(ef.Data, af.Data) {
+				t.Errorf("ReverseBinaryFragment.Data: expected %#v, actual %#v", ef.Data, af.Data)
+			}
+		}
+	} else if actual.ReverseBinaryFragment != nil {
+		t.Errorf("ReverseBinaryFragment: expected nil, actual is non-nil")
+	}
+}
 
-	t.FailNow()
+func assertEqual[T comparable](t *testing.T, expected, actual T, name string) {
+	if expected != actual {
+		t.Errorf("%s: expected %#v, actual %#v", name, expected, actual)
+	}
 }
-*/

go.mod

@@ -1,3 +1,3 @@
 module github.com/bluekeyes/go-gitdiff
 
-go 1.13
+go 1.20