From 34b1960d1081a133ec6846f2966fe992b5ec27b6 Mon Sep 17 00:00:00 2001
From: Andy Arthur <andy@liquidata.co>
Date: Thu, 27 Feb 2020 11:29:46 -0800
Subject: [PATCH] PR feedback

---
 bats/compatibility/runner.sh                  | 10 +++++-----
 .../schema/encoding/schema_marshaling.go      |  7 +++++--
 .../schema/encoding/schema_marshaling_test.go | 19 ++++++++++++++++---
 3 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/bats/compatibility/runner.sh b/bats/compatibility/runner.sh
index e9dfdbef2c9..8df2b9ae3fe 100755
--- a/bats/compatibility/runner.sh
+++ b/bats/compatibility/runner.sh
@@ -1,10 +1,5 @@
 #!/bin/bash
 
-if [[ $(git diff --stat) != '' ]]; then
-  echo "cannot run compatibility tests with git working changes"
-  exit
-fi
-
 function build_dolt() {
   pushd "$dolt_dir" > /dev/null || exit
   git checkout "$1" > /dev/null
@@ -31,6 +26,11 @@ function run_bats_tests() {
   popd > /dev/null || exit
 }
 
+# ensure that we have a clean working change set before we begin
+if [[ $(git diff --stat) != '' ]]; then
+  echo "cannot run compatibility tests with git working changes"
+  exit
+fi
 
 # copy all the test files to take them out of source control
 # when we checkout different Dolt releases we don't want to
diff --git a/go/libraries/doltcore/schema/encoding/schema_marshaling.go b/go/libraries/doltcore/schema/encoding/schema_marshaling.go
index b31cfbb48c7..0c8ad2a9af0 100644
--- a/go/libraries/doltcore/schema/encoding/schema_marshaling.go
+++ b/go/libraries/doltcore/schema/encoding/schema_marshaling.go
@@ -27,7 +27,7 @@ import (
 
 // Correct Marshalling & Unmarshalling is essential to compatibility across Dolt versions
 // any changes to the fields of Schema or other persisted objects must be append only, no
-// fields can ever be removed without break compatibility.
+// fields can ever be removed without breaking compatibility.
 //
 // the marshalling annotations of new fields must have the "omitempty" option to allow newer
 // versions of Dolt to read objects serialized by older Dolt versions where the field did not
@@ -46,6 +46,8 @@ type encodedColumn struct {
 	TypeInfo encodedTypeInfo `noms:"typeinfo,omitempty" json:"typeinfo,omitempty"`
 
 	Constraints []encodedConstraint `noms:"col_constraints" json:"col_constraints"`
+
+	// NB: all new fields must have the 'omitempty' annotation. See comment above
 }
 
 func encodeAllColConstraints(constraints []schema.ColConstraint) []encodedConstraint {
@@ -80,7 +82,8 @@ func encodeColumn(col schema.Column) encodedColumn {
 		col.KindString(),
 		col.IsPartOfPK,
 		encodeTypeInfo(col.TypeInfo),
-		encodeAllColConstraints(col.Constraints)}
+		encodeAllColConstraints(col.Constraints),
+	}
 }
 
 func (nfd encodedColumn) decodeColumn() (schema.Column, error) {
diff --git a/go/libraries/doltcore/schema/encoding/schema_marshaling_test.go b/go/libraries/doltcore/schema/encoding/schema_marshaling_test.go
index 22b8deca9e9..a3d232bb504 100644
--- a/go/libraries/doltcore/schema/encoding/schema_marshaling_test.go
+++ b/go/libraries/doltcore/schema/encoding/schema_marshaling_test.go
@@ -74,7 +74,7 @@ func TestNomsMarshalling(t *testing.T) {
 	validated, err := validateUnmarshaledNomsValue(context.Background(), types.Format_7_18, val)
 
 	if err != nil {
-		t.Fatal("Failed compatibility test")
+		t.Fatal("Failed compatibility test. Schema could not be unmarshalled with mirror type")
 	}
 
 	if !reflect.DeepEqual(tSchema, validated) {
@@ -186,9 +186,22 @@ func validateUnmarshaledNomsValue(ctx context.Context, nbf *types.NomsBinFormat,
 	return sd.decodeSchema()
 }
 
+func TestMirroredTypes(t *testing.T) {
+	realType := reflect.ValueOf(&encodedColumn{}).Elem()
+	mirrorType := reflect.ValueOf(&testEncodedColumn{}).Elem()
+	require.Equal(t, mirrorType.NumField(), realType.NumField())
+
+	// TODO: create reflection tests to ensure that:
+	// - no fields in testEncodeColumn have the 'omitempty' annotation
+	// - no legacy fields in encodeColumn have the 'omitempty' annotation (with whitelist)
+	// - all new fields in encodeColumn have the 'omitempty' annotation
+}
+
 // testEncodedColumn is a mirror type of encodedColumn that helps ensure compatibility between Dolt versions
-// fields in this test struct should be added WITHOUT the "omitempty" annotation in order to guarentee that
-// all fields are being always being written when encodedColumn is serialized.
+//
+// Fields in this test struct should be added WITHOUT the "omitempty" annotation in order to guarantee that
+// all fields in encodeColumn are always being written when encodedColumn is serialized.
+// See the comment above type encodeColumn.
 type testEncodedColumn struct {
 	Tag uint64 `noms:"tag" json:"tag"`