Migrate to only doing marshalling twice, and compare via deep copy

Author: Chris McDonnell

pkg/config/app_config.go

@@ -5,6 +5,7 @@ import (
 	"log"
 	"os"
 	"path/filepath"
+	"reflect"
 	"strings"
 	"time"
 
@@ -237,7 +238,17 @@ func migrateUserConfig(path string, content []byte) ([]byte, error) {
 
 // A pure function helper for testing purposes
 func computeMigratedConfig(path string, content []byte) ([]byte, error) {
-	changedContent := content
+	var err error
+	var rootNode yaml.Node
+	err = yaml.Unmarshal(content, &rootNode)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse YAML: %w", err)
+	}
+	var originalCopy yaml.Node
+	err = yaml.Unmarshal(content, &originalCopy)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse YAML, but only the second time!?!? How did that happen: %w", err)
+	}
 
 	pathsToReplace := []struct {
 		oldPath []string
@@ -248,46 +259,53 @@ func computeMigratedConfig(path string, content []byte) ([]byte, error) {
 		{[]string{"gui", "windowSize"}, "screenMode"},
 	}
 
-	var err error
 	for _, pathToReplace := range pathsToReplace {
-		changedContent, err = yaml_utils.RenameYamlKey(changedContent, pathToReplace.oldPath, pathToReplace.newName)
+		_, err := yaml_utils.RenameYamlKey(&rootNode, pathToReplace.oldPath, pathToReplace.newName)
 		if err != nil {
 			return nil, fmt.Errorf("Couldn't migrate config file at `%s` for key %s: %s", path, strings.Join(pathToReplace.oldPath, "."), err)
 		}
 	}
 
-	changedContent, err = changeNullKeybindingsToDisabled(changedContent)
+	err = changeNullKeybindingsToDisabled(&rootNode)
 	if err != nil {
 		return nil, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
 	}
 
-	changedContent, err = changeElementToSequence(changedContent, []string{"git", "commitPrefix"})
+	err = changeElementToSequence(&rootNode, []string{"git", "commitPrefix"})
 	if err != nil {
 		return nil, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
 	}
 
-	changedContent, err = changeCommitPrefixesMap(changedContent)
+	err = changeCommitPrefixesMap(&rootNode)
 	if err != nil {
 		return nil, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
 	}
+
 	// Add more migrations here...
 
-	return changedContent, nil
+	if !reflect.DeepEqual(rootNode, originalCopy) {
+		newContent, err := yaml_utils.YamlMarshal(&rootNode)
+		if err != nil {
+			return nil, fmt.Errorf("Failed to remarsall!")
+		}
+		return newContent, nil
+	} else {
+		return content, nil
+	}
 }
 
-func changeNullKeybindingsToDisabled(changedContent []byte) ([]byte, error) {
-	return yaml_utils.Walk(changedContent, func(node *yaml.Node, path string) bool {
+func changeNullKeybindingsToDisabled(rootNode *yaml.Node) error {
+	return yaml_utils.Walk(rootNode, func(node *yaml.Node, path string) {
 		if strings.HasPrefix(path, "keybinding.") && node.Kind == yaml.ScalarNode && node.Tag == "!!null" {
 			node.Value = "<disabled>"
 			node.Tag = "!!str"
-			return true
 		}
-		return false
+		return
 	})
 }
 
-func changeElementToSequence(changedContent []byte, path []string) ([]byte, error) {
-	return yaml_utils.TransformNode(changedContent, path, func(node *yaml.Node) (bool, error) {
+func changeElementToSequence(rootNode *yaml.Node, path []string) error {
+	return yaml_utils.TransformNode(rootNode, path, func(node *yaml.Node) error {
 		if node.Kind == yaml.MappingNode {
 			nodeContentCopy := node.Content
 			node.Kind = yaml.SequenceNode
@@ -298,15 +316,14 @@ func changeElementToSequence(changedContent []byte, path []string) ([]byte, erro
 				Content: nodeContentCopy,
 			}}
 
-			return true, nil
+			return nil
 		}
-		return false, nil
+		return nil
 	})
 }
 
-func changeCommitPrefixesMap(changedContent []byte) ([]byte, error) {
-	return yaml_utils.TransformNode(changedContent, []string{"git", "commitPrefixes"}, func(prefixesNode *yaml.Node) (bool, error) {
-		changedAnyNodes := false
+func changeCommitPrefixesMap(rootNode *yaml.Node) error {
+	return yaml_utils.TransformNode(rootNode, []string{"git", "commitPrefixes"}, func(prefixesNode *yaml.Node) error {
 		if prefixesNode.Kind == yaml.MappingNode {
 			for _, contentNode := range prefixesNode.Content {
 				if contentNode.Kind == yaml.MappingNode {
@@ -318,11 +335,10 @@ func changeCommitPrefixesMap(changedContent []byte) ([]byte, error) {
 						Kind:    yaml.MappingNode,
 						Content: nodeContentCopy,
 					}}
-					changedAnyNodes = true
 				}
 			}
 		}
-		return changedAnyNodes, nil
+		return nil
 	})
 }
 

pkg/utils/yaml_utils/yaml_utils.go

@@ -35,7 +35,7 @@ func UpdateYamlValue(yamlBytes []byte, path []string, value string) ([]byte, err
 	}
 
 	// Convert the updated YAML node back to YAML bytes.
-	updatedYAMLBytes, err := yamlMarshal(body)
+	updatedYAMLBytes, err := YamlMarshal(body)
 	if err != nil {
 		return nil, fmt.Errorf("failed to convert YAML node to bytes: %w", err)
 	}
@@ -100,83 +100,57 @@ func lookupKey(node *yaml.Node, key string) (*yaml.Node, *yaml.Node) {
 	return nil, nil
 }
 
-// Walks a yaml document to the specified path, and then applies the transformation to that node.
+// Walks a yaml document from the root node to the specified path, and then applies the transformation to that node.
 //
 // The transform must return true if it made changes to the node.
 // If the requested path is not defined in the document, no changes are made to the document.
 //
-// If no changes are made, the original document is returned.
-// If changes are made, a newly marshalled document is returned. (This may result in different indentation for all nodes)
-func TransformNode(yamlBytes []byte, path []string, transform func(node *yaml.Node) (bool, error)) ([]byte, error) {
-	// Parse the YAML file.
-	var node yaml.Node
-	err := yaml.Unmarshal(yamlBytes, &node)
-	if err != nil {
-		return nil, fmt.Errorf("failed to parse YAML: %w", err)
-	}
-
+// Returns true if the transform function made changes to any node
+func TransformNode(rootNode *yaml.Node, path []string, transform func(node *yaml.Node) error) error {
 	// Empty document: nothing to do.
-	if len(node.Content) == 0 {
-		return yamlBytes, nil
+	if len(rootNode.Content) == 0 {
+		return nil
 	}
 
-	body := node.Content[0]
+	body := rootNode.Content[0]
 
-	if didTransform, err := transformNode(body, path, transform); err != nil || !didTransform {
-		return yamlBytes, err
+	if err := transformNode(body, path, transform); err != nil {
+		return err
 	}
 
-	// Convert the updated YAML node back to YAML bytes.
-	updatedYAMLBytes, err := yamlMarshal(body)
-	if err != nil {
-		return nil, fmt.Errorf("failed to convert YAML node to bytes: %w", err)
-	}
-
-	return updatedYAMLBytes, nil
+	return nil
 }
 
 // A recursive function to walk down the tree. See TransformNode for more details.
-func transformNode(node *yaml.Node, path []string, transform func(node *yaml.Node) (bool, error)) (bool, error) {
+func transformNode(node *yaml.Node, path []string, transform func(node *yaml.Node) error) error {
 	if len(path) == 0 {
 		return transform(node)
 	}
 
 	keyNode, valueNode := lookupKey(node, path[0])
 	if keyNode == nil {
-		return false, nil
+		return nil
 	}
 
 	return transformNode(valueNode, path[1:], transform)
 }
 
-// takes a yaml document in bytes, a path to a key, and a new name for the key.
+// Takes the root node of a yaml document, a path to a key, and a new name for the key.
 // Will rename the key to the new name if it exists, and do nothing otherwise.
-func RenameYamlKey(yamlBytes []byte, path []string, newKey string) ([]byte, error) {
-	// Parse the YAML file.
-	var node yaml.Node
-	err := yaml.Unmarshal(yamlBytes, &node)
-	if err != nil {
-		return nil, fmt.Errorf("failed to parse YAML: %w for bytes %s", err, string(yamlBytes))
-	}
-
+// Returns true if it found the old path and renamed the key
+func RenameYamlKey(rootNode *yaml.Node, path []string, newKey string) (bool, error) {
 	// Empty document: nothing to do.
-	if len(node.Content) == 0 {
-		return yamlBytes, nil
+	if len(rootNode.Content) == 0 {
+		return false, nil
 	}
 
-	body := node.Content[0]
+	body := rootNode.Content[0]
 
 	if didRename, err := renameYamlKey(body, path, newKey); err != nil || !didRename {
-		return yamlBytes, err
-	}
-
-	// Convert the updated YAML node back to YAML bytes.
-	updatedYAMLBytes, err := yamlMarshal(body)
-	if err != nil {
-		return nil, fmt.Errorf("failed to convert YAML node to bytes: %w", err)
+		return false, err
 	}
 
-	return updatedYAMLBytes, nil
+	return true, nil
 }
 
 // Recursive function to rename the YAML key.
@@ -205,42 +179,27 @@ func renameYamlKey(node *yaml.Node, path []string, newKey string) (bool, error)
 }
 
 // Traverses a yaml document, calling the callback function for each node. The
-// callback is allowed to modify the node in place, in which case it should
-// return true. The function returns the original yaml document if none of the
-// callbacks returned true, and the modified document otherwise.
-func Walk(yamlBytes []byte, callback func(node *yaml.Node, path string) bool) ([]byte, error) {
-	// Parse the YAML file.
-	var node yaml.Node
-	err := yaml.Unmarshal(yamlBytes, &node)
-	if err != nil {
-		return nil, fmt.Errorf("failed to parse YAML: %w", err)
-	}
-
+// callback is expected to modify the node in place
+func Walk(rootNode *yaml.Node, callback func(node *yaml.Node, path string)) error {
 	// Empty document: nothing to do.
-	if len(node.Content) == 0 {
-		return yamlBytes, nil
+	if len(rootNode.Content) == 0 {
+		return nil
 	}
 
-	body := node.Content[0]
-
-	if didChange, err := walk(body, "", callback); err != nil || !didChange {
-		return yamlBytes, err
-	}
+	body := rootNode.Content[0]
 
-	// Convert the updated YAML node back to YAML bytes.
-	updatedYAMLBytes, err := yamlMarshal(body)
-	if err != nil {
-		return nil, fmt.Errorf("failed to convert YAML node to bytes: %w", err)
+	if err := walk(body, "", callback); err != nil {
+		return err
 	}
 
-	return updatedYAMLBytes, nil
+	return nil
 }
 
-func walk(node *yaml.Node, path string, callback func(*yaml.Node, string) bool) (bool, error) {
-	didChange := callback(node, path)
+func walk(node *yaml.Node, path string, callback func(*yaml.Node, string)) error {
+	callback(node, path)
 	switch node.Kind {
 	case yaml.DocumentNode:
-		return false, errors.New("Unexpected document node in the middle of a yaml tree")
+		return errors.New("Unexpected document node in the middle of a yaml tree")
 	case yaml.MappingNode:
 		for i := 0; i < len(node.Content); i += 2 {
 			name := node.Content[i].Value
@@ -251,31 +210,29 @@ func walk(node *yaml.Node, path string, callback func(*yaml.Node, string) bool)
 			} else {
 				childPath = fmt.Sprintf("%s.%s", path, name)
 			}
-			didChangeChild, err := walk(childNode, childPath, callback)
+			err := walk(childNode, childPath, callback)
 			if err != nil {
-				return false, err
+				return err
 			}
-			didChange = didChange || didChangeChild
 		}
 	case yaml.SequenceNode:
 		for i := 0; i < len(node.Content); i++ {
 			childPath := fmt.Sprintf("%s[%d]", path, i)
-			didChangeChild, err := walk(node.Content[i], childPath, callback)
+			err := walk(node.Content[i], childPath, callback)
 			if err != nil {
-				return false, err
+				return err
 			}
-			didChange = didChange || didChangeChild
 		}
 	case yaml.ScalarNode:
 		// nothing to do
 	case yaml.AliasNode:
-		return false, errors.New("Alias nodes are not supported")
+		return errors.New("Alias nodes are not supported")
 	}
 
-	return didChange, nil
+	return nil
 }
 
-func yamlMarshal(node *yaml.Node) ([]byte, error) {
+func YamlMarshal(node *yaml.Node) ([]byte, error) {
 	var buffer bytes.Buffer
 	encoder := yaml.NewEncoder(&buffer)
 	encoder.SetIndent(2)