diff --git a/pkg/config/app_config.go b/pkg/config/app_config.go index 400ab15d5..180457fdf 100644 --- a/pkg/config/app_config.go +++ b/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 { diff --git a/pkg/config/app_config_test.go b/pkg/config/app_config_test.go index d1f165cb3..c9554baa7 100644 --- a/pkg/config/app_config_test.go +++ b/pkg/config/app_config_test.go @@ -8,14 +8,15 @@ import ( func TestCommitPrefixMigrations(t *testing.T) { scenarios := []struct { - name string - input string - expected string + name string + input string + expected string + expectedDidChange bool }{ { - name: "Empty String", - input: "", - expected: "", + name: "Empty String", + input: "", + 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:", + name: "Incomplete Configuration", + input: "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,37 +70,33 @@ git: foo: - pattern: "^\\w+-\\w+.*" replace: '[JIRA $0] '`, - expected: ` -git: - commitPrefix: - - pattern: "Hello World" - replace: "Goodbye" - commitPrefixes: - foo: - - pattern: "^\\w+-\\w+.*" - replace: '[JIRA $0] '`, + expectedDidChange: false, }, } 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.expected, string(actual)) + assert.Equal(t, s.expectedDidChange, didChange) + if didChange { + assert.Equal(t, s.expected, string(actual)) + } }) } } func TestCustomCommandsOutputMigration(t *testing.T) { scenarios := []struct { - name string - input string - expected string + name string + input string + expected string + expectedDidChange bool }{ { - name: "Empty String", - input: "", - expected: "", + name: "Empty String", + input: "", + 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.expected, string(actual)) + assert.Equal(t, s.expectedDidChange, didChange) + if didChange { + assert.Equal(t, s.expected, string(actual)) + } }) } } @@ -786,20 +792,21 @@ keybinding: func BenchmarkMigrationOnLargeConfiguration(b *testing.B) { for b.Loop() { - _, _ = computeMigratedConfig("path doesn't matter", largeConfiguration) + _, _, _ = computeMigratedConfig("path doesn't matter", largeConfiguration) } } func TestAllBranchesLogCmdMigrations(t *testing.T) { scenarios := []struct { - name string - input string - expected string + name string + input string + expected string + expectedDidChange bool }{ { - name: "Incomplete Configuration Passes uneventfully", - input: "git:", - expected: "git:", + name: "Incomplete Configuration Passes uneventfully", + input: "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: - allBranchesLogCmds: - - git log -`, + expected: "", }, { 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.expected, string(actual)) + assert.Equal(t, s.expectedDidChange, didChange) + if didChange { + assert.Equal(t, s.expected, string(actual)) + } }) } }