Cleanup: return didChange bool from computeMigratedConfig

It's a bit silly to find out by string comparison whether computeMigratedConfig
did something, when it knows this already and can just return the information.

This doesn't make a huge difference to the production code; the string
comparison isn't very expensive, so this isn't a big deal. However, it makes the
tests clearer; we don't have to bother specifying an expected output string if
the didChange flag is false, and in particular we can get rid of the ugly "This
test intentionally uses non-standard indentation" bit in one of the tests.

pkg/config/app_config.go

@@ -220,13 +220,13 @@ func loadUserConfig(configFiles []*ConfigFile, base *UserConfig) (*UserConfig, e
 // from one container to another, or changing the type of a key (e.g. from bool
 // to an enum).
 func migrateUserConfig(path string, content []byte) ([]byte, error) {
-	changedContent, err := computeMigratedConfig(path, content)
+	changedContent, didChange, err := computeMigratedConfig(path, content)
 	if err != nil {
 		return nil, err
 	}
 
 	// Nothing to do if config didn't change
-	if string(changedContent) == string(content) {
+	if !didChange {
 		return content, nil
 	}
 
@@ -240,17 +240,17 @@ func migrateUserConfig(path string, content []byte) ([]byte, error) {
 }
 
 // A pure function helper for testing purposes
-func computeMigratedConfig(path string, content []byte) ([]byte, error) {
+func computeMigratedConfig(path string, content []byte) ([]byte, bool, error) {
 	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)
+		return nil, false, 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)
+		return nil, false, fmt.Errorf("failed to parse YAML, but only the second time!?!? How did that happen: %w", err)
 	}
 
 	pathsToReplace := []struct {
@@ -265,46 +265,46 @@ func computeMigratedConfig(path string, content []byte) ([]byte, error) {
 	for _, pathToReplace := range pathsToReplace {
 		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)
+			return nil, false, fmt.Errorf("Couldn't migrate config file at `%s` for key %s: %s", path, strings.Join(pathToReplace.oldPath, "."), err)
 		}
 	}
 
 	err = changeNullKeybindingsToDisabled(&rootNode)
 	if err != nil {
-		return nil, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
+		return nil, false, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
 	}
 
 	err = changeElementToSequence(&rootNode, []string{"git", "commitPrefix"})
 	if err != nil {
-		return nil, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
+		return nil, false, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
 	}
 
 	err = changeCommitPrefixesMap(&rootNode)
 	if err != nil {
-		return nil, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
+		return nil, false, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
 	}
 
 	err = changeCustomCommandStreamAndOutputToOutputEnum(&rootNode)
 	if err != nil {
-		return nil, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
+		return nil, false, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
 	}
 
 	err = migrateAllBranchesLogCmd(&rootNode)
 	if err != nil {
-		return nil, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
+		return nil, false, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err)
 	}
 
 	// Add more migrations here...
 
 	if reflect.DeepEqual(rootNode, originalCopy) {
-		return content, nil
+		return nil, false, nil
 	}
 
 	newContent, err := yaml_utils.YamlMarshal(&rootNode)
 	if err != nil {
-		return nil, fmt.Errorf("Failed to remarsal!\n %w", err)
+		return nil, false, fmt.Errorf("Failed to remarsal!\n %w", err)
 	}
-	return newContent, nil
+	return newContent, true, nil
 }
 
 func changeNullKeybindingsToDisabled(rootNode *yaml.Node) error {

pkg/config/app_config_test.go

@@ -11,11 +11,12 @@ func TestCommitPrefixMigrations(t *testing.T) {
 		name              string
 		input             string
 		expected          string
+		expectedDidChange bool
 	}{
 		{
 			name:              "Empty String",
 			input:             "",
-			expected: "",
+			expectedDidChange: false,
 		},
 		{
 			name: "Single CommitPrefix Rename",
@@ -29,6 +30,7 @@ func TestCommitPrefixMigrations(t *testing.T) {
     - pattern: "^\\w+-\\w+.*"
       replace: '[JIRA $0] '
 `,
+			expectedDidChange: true,
 		},
 		{
 			name: "Complicated CommitPrefixes Rename",
@@ -50,15 +52,14 @@ func TestCommitPrefixMigrations(t *testing.T) {
       - pattern: "^foo.bar*"
         replace: '[FUN $0] '
 `,
+			expectedDidChange: true,
 		},
 		{
 			name:              "Incomplete Configuration",
 			input:             "git:",
-			expected: "git:",
+			expectedDidChange: false,
 		},
 		{
-			// This test intentionally uses non-standard indentation to test that the migration
-			// does not change the input.
 			name: "No changes made when already migrated",
 			input: `
 git:
@@ -69,23 +70,18 @@ git:
     foo:
       - pattern: "^\\w+-\\w+.*"
         replace: '[JIRA $0] '`,
-			expected: `
+			expectedDidChange: false,
-git:
-   commitPrefix:
-    - pattern: "Hello World"
-      replace: "Goodbye"
-   commitPrefixes:
-    foo:
-      - pattern: "^\\w+-\\w+.*"
-        replace: '[JIRA $0] '`,
 		},
 	}
 
 	for _, s := range scenarios {
 		t.Run(s.name, func(t *testing.T) {
-			actual, err := computeMigratedConfig("path doesn't matter", []byte(s.input))
+			actual, didChange, err := computeMigratedConfig("path doesn't matter", []byte(s.input))
 			assert.NoError(t, err)
+			assert.Equal(t, s.expectedDidChange, didChange)
+			if didChange {
 				assert.Equal(t, s.expected, string(actual))
+			}
 		})
 	}
 }
@@ -95,11 +91,12 @@ func TestCustomCommandsOutputMigration(t *testing.T) {
 		name              string
 		input             string
 		expected          string
+		expectedDidChange bool
 	}{
 		{
 			name:              "Empty String",
 			input:             "",
-			expected: "",
+			expectedDidChange: false,
 		},
 		{
 			name: "Convert subprocess to output=terminal",
@@ -111,6 +108,7 @@ func TestCustomCommandsOutputMigration(t *testing.T) {
   - command: echo 'hello'
     output: terminal
 `,
+			expectedDidChange: true,
 		},
 		{
 			name: "Convert stream to output=log",
@@ -122,6 +120,7 @@ func TestCustomCommandsOutputMigration(t *testing.T) {
   - command: echo 'hello'
     output: log
 `,
+			expectedDidChange: true,
 		},
 		{
 			name: "Convert showOutput to output=popup",
@@ -133,6 +132,7 @@ func TestCustomCommandsOutputMigration(t *testing.T) {
   - command: echo 'hello'
     output: popup
 `,
+			expectedDidChange: true,
 		},
 		{
 			name: "Subprocess wins over the other two",
@@ -146,6 +146,7 @@ func TestCustomCommandsOutputMigration(t *testing.T) {
   - command: echo 'hello'
     output: terminal
 `,
+			expectedDidChange: true,
 		},
 		{
 			name: "Stream wins over showOutput",
@@ -158,6 +159,7 @@ func TestCustomCommandsOutputMigration(t *testing.T) {
   - command: echo 'hello'
     output: log
 `,
+			expectedDidChange: true,
 		},
 		{
 			name: "Explicitly setting to false doesn't create an output=none key",
@@ -170,14 +172,18 @@ func TestCustomCommandsOutputMigration(t *testing.T) {
 			expected: `customCommands:
   - command: echo 'hello'
 `,
+			expectedDidChange: true,
 		},
 	}
 
 	for _, s := range scenarios {
 		t.Run(s.name, func(t *testing.T) {
-			actual, err := computeMigratedConfig("path doesn't matter", []byte(s.input))
+			actual, didChange, err := computeMigratedConfig("path doesn't matter", []byte(s.input))
 			assert.NoError(t, err)
+			assert.Equal(t, s.expectedDidChange, didChange)
+			if didChange {
 				assert.Equal(t, s.expected, string(actual))
+			}
 		})
 	}
 }
@@ -786,7 +792,7 @@ keybinding:
 
 func BenchmarkMigrationOnLargeConfiguration(b *testing.B) {
 	for b.Loop() {
-		_, _ = computeMigratedConfig("path doesn't matter", largeConfiguration)
+		_, _, _ = computeMigratedConfig("path doesn't matter", largeConfiguration)
 	}
 }
 
@@ -795,11 +801,12 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) {
 		name              string
 		input             string
 		expected          string
+		expectedDidChange bool
 	}{
 		{
 			name:              "Incomplete Configuration Passes uneventfully",
 			input:             "git:",
-			expected: "git:",
+			expectedDidChange: false,
 		},
 		{
 			name: "Single Cmd with no Cmds",
@@ -810,6 +817,7 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) {
   allBranchesLogCmds:
     - git log --graph --oneline
 `,
+			expectedDidChange: true,
 		},
 		{
 			name: "Cmd with one existing Cmds",
@@ -823,6 +831,7 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) {
     - git log --graph --oneline
     - git log --graph --oneline --pretty
 `,
+			expectedDidChange: true,
 		},
 		{
 			name: "Only Cmds set have no changes",
@@ -830,10 +839,7 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) {
   allBranchesLogCmds:
     - git log
 `,
-			expected: `git:
+			expected: "",
-  allBranchesLogCmds:
-    - git log
-`,
 		},
 		{
 			name: "Removes Empty Cmd When at end of yaml",
@@ -846,6 +852,7 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) {
   allBranchesLogCmds:
     - git log --graph --oneline
 `,
+			expectedDidChange: true,
 		},
 		{
 			name: "Migrates when sequence defined inline",
@@ -856,6 +863,7 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) {
 			expected: `git:
   allBranchesLogCmds: [baz, foo, bar]
 `,
+			expectedDidChange: true,
 		},
 		{
 			name: "Removes Empty Cmd With Keys Afterwards",
@@ -870,14 +878,18 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) {
     - git log --graph --oneline
   foo: bar
 `,
+			expectedDidChange: true,
 		},
 	}
 
 	for _, s := range scenarios {
 		t.Run(s.name, func(t *testing.T) {
-			actual, err := computeMigratedConfig("path doesn't matter", []byte(s.input))
+			actual, didChange, err := computeMigratedConfig("path doesn't matter", []byte(s.input))
 			assert.NoError(t, err)
+			assert.Equal(t, s.expectedDidChange, didChange)
+			if didChange {
 				assert.Equal(t, s.expected, string(actual))
+			}
 		})
 	}
 }