Improved consistency of map key sorting.

[joshdk]

There are some specific sets of keys that will be sorted inconsistently when marshaling. This can result in YAML documents that change significantly when re-marshaled, even if the underlying data remains identical.

I noticed this when marshaling a cache object (vanilla map[string]string) as part of a CI pipeline. The resulting (git) diff kept thrashing even if the cache data didn't.

As a minimal repro, I was able to find a set of keys that trigger this behavior. These keys do not have a strict ordering, as z01 < z12, z12 < z1a, and z1a < z01, creating a cycle.

func (s *S) TestSortedMap(c *C) {
	var (
		m = map[string]struct{}{
			"z01": {},
			"z12": {},
			"z1a": {},
		}

		expected = []byte("z01: {}\nz12: {}\nz1a: {}\n")
	)

	// Repeatedly marshal the same exact data, and check for any differences in
	// the output.
	for i := 0; i < 100; i++ {
		actual, err := yaml.Marshal(m)
		c.Assert(err, IsNil)

		if bytes.Compare(expected, actual) != 0 {
			fmt.Printf("Expected: \n%s\n", string(expected))
			fmt.Printf("Actual:   \n%s\n", string(actual))
			c.Fatalf("Difference on iteration %d", i)
		}
	}
}

Expected: 
z01: {}
z12: {}
z1a: {}

Actual:   
z12: {}
z1a: {}
z01: {}

<snip>
Error: Difference on iteration 4

The changes in this PR removes this inconsistency, while maintaining the same ordering behavior asserted in the existing test suite.

Some specific feedback that I think this PR needs:

• How you would like to test functionality of private (yaml) package contents? - All current tests exist in an external (yaml_test) package, and cannot assert this behavior directly.
• How would you like to test functionality that has not made it into v2 (or v3, etc)? - All current tests import "gopkg.in/yaml.v2" which would make new tests fail as that package doesn't yet contain the corresponding new fixes.

Looking forward to hearing your feedback, cheers!

[joshdk]

Do you have any feedback @niemeyer and @rogpeppe? Thank you!

[dominikgrygiel]

I've tested this change and it works great 🥇

Another way of unit testing this change could be with something like:

var keys1 keyList = keyList{reflect.ValueOf("z01"), reflect.ValueOf("z13"), reflect.ValueOf("z1a")}
var keys2 keyList = keyList{reflect.ValueOf("z1a"), reflect.ValueOf("z01"), reflect.ValueOf("z13")}

sort.Sort(keys1)
sort.Sort(keys2)
// compare keys1 and keys2, would be different before the fix and the same after this fix