diff --git a/go.mod b/go.mod index 30ae0e978..b281e6448 100644 --- a/go.mod +++ b/go.mod @@ -35,6 +35,7 @@ require ( github.com/spkg/bom v0.0.0-20160624110644-59b7046e48ad github.com/stefanhaller/git-todo-parser v0.0.7-0.20250429125209-dcf39e4641f5 github.com/stretchr/testify v1.10.0 + github.com/wk8/go-ordered-map/v2 v2.1.8 github.com/xo/terminfo v0.0.0-20210125001918-ca9a967f8778 golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56 golang.org/x/sync v0.13.0 @@ -74,7 +75,6 @@ require ( github.com/rivo/uniseg v0.4.7 // indirect github.com/sergi/go-diff v1.3.2-0.20230802210424-5b0b94c5c0d3 // indirect github.com/skeema/knownhosts v1.3.1 // indirect - github.com/wk8/go-ordered-map/v2 v2.1.8 // indirect github.com/xanzy/ssh-agent v0.3.3 // indirect golang.org/x/crypto v0.37.0 // indirect golang.org/x/net v0.39.0 // indirect diff --git a/pkg/config/app_config.go b/pkg/config/app_config.go index 86e827724..eb3315383 100644 --- a/pkg/config/app_config.go +++ b/pkg/config/app_config.go @@ -14,6 +14,7 @@ import ( "github.com/jesseduffield/lazygit/pkg/utils" "github.com/jesseduffield/lazygit/pkg/utils/yaml_utils" "github.com/samber/lo" + orderedmap "github.com/wk8/go-ordered-map/v2" "gopkg.in/yaml.v3" ) @@ -96,7 +97,7 @@ func NewAppConfig( configFiles = []*ConfigFile{configFile} } - userConfig, err := loadUserConfigWithDefaults(configFiles) + userConfig, err := loadUserConfigWithDefaults(configFiles, false) if err != nil { return nil, err } @@ -145,11 +146,11 @@ func findOrCreateConfigDir() (string, error) { return folder, os.MkdirAll(folder, 0o755) } -func loadUserConfigWithDefaults(configFiles []*ConfigFile) (*UserConfig, error) { - return loadUserConfig(configFiles, GetDefaultConfig()) +func loadUserConfigWithDefaults(configFiles []*ConfigFile, isGuiInitialized bool) (*UserConfig, error) { + return loadUserConfig(configFiles, GetDefaultConfig(), isGuiInitialized) } -func loadUserConfig(configFiles []*ConfigFile, base *UserConfig) (*UserConfig, error) { +func loadUserConfig(configFiles []*ConfigFile, base *UserConfig, isGuiInitialized bool) (*UserConfig, error) { for _, configFile := range configFiles { path := configFile.Path statInfo, err := os.Stat(path) @@ -194,7 +195,7 @@ func loadUserConfig(configFiles []*ConfigFile, base *UserConfig) (*UserConfig, e return nil, err } - content, err = migrateUserConfig(path, content) + content, err = migrateUserConfig(path, content, isGuiInitialized) if err != nil { return nil, err } @@ -219,37 +220,54 @@ func loadUserConfig(configFiles []*ConfigFile, base *UserConfig) (*UserConfig, e // config over time; examples are renaming a key to a better name, moving a key // 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) +func migrateUserConfig(path string, content []byte, isGuiInitialized bool) ([]byte, error) { + changes := orderedmap.New[string, bool]() + + changedContent, didChange, err := computeMigratedConfig(path, content, changes) if err != nil { return nil, err } - // Write config back if changed - if string(changedContent) != string(content) { - fmt.Println("Provided user config is deprecated but auto-fixable. Attempting to write fixed version back to file...") - if err := os.WriteFile(path, changedContent, 0o644); err != nil { - return nil, fmt.Errorf("While attempting to write back fixed user config to %s, an error occurred: %s", path, err) - } - fmt.Printf("Success. New config written to %s\n", path) - return changedContent, nil + // Nothing to do if config didn't change + if !didChange { + return content, nil } - return content, nil + changesText := "The following changes were made:\n\n" + for pair := changes.Oldest(); pair != nil; pair = pair.Next() { + changesText += fmt.Sprintf("- %s\n", pair.Key) + } + + // Write config back + if !isGuiInitialized { + fmt.Printf("The user config file %s must be migrated. Attempting to do this automatically.\n", path) + fmt.Println(changesText) + } + if err := os.WriteFile(path, changedContent, 0o644); err != nil { + errorMsg := fmt.Sprintf("While attempting to write back migrated user config to %s, an error occurred: %s", path, err) + if isGuiInitialized { + errorMsg += "\n\n" + changesText + } + return nil, errors.New(errorMsg) + } + if !isGuiInitialized { + fmt.Printf("Config file saved successfully to %s\n", path) + } + return changedContent, nil } // A pure function helper for testing purposes -func computeMigratedConfig(path string, content []byte) ([]byte, error) { +func computeMigratedConfig(path string, content []byte, changes *orderedmap.OrderedMap[string, bool]) ([]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 { @@ -262,60 +280,64 @@ func computeMigratedConfig(path string, content []byte) ([]byte, error) { } for _, pathToReplace := range pathsToReplace { - err := yaml_utils.RenameYamlKey(&rootNode, pathToReplace.oldPath, pathToReplace.newName) + err, didReplace := 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) + } + if didReplace { + changes.Set(fmt.Sprintf("Renamed '%s' to '%s'", strings.Join(pathToReplace.oldPath, "."), pathToReplace.newName), true) } } - err = changeNullKeybindingsToDisabled(&rootNode) + err = changeNullKeybindingsToDisabled(&rootNode, changes) 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"}) + err = changeElementToSequence(&rootNode, []string{"git", "commitPrefix"}, changes) 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) + err = changeCommitPrefixesMap(&rootNode, changes) 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) + err = changeCustomCommandStreamAndOutputToOutputEnum(&rootNode, changes) 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) + err = migrateAllBranchesLogCmd(&rootNode, changes) 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) { - newContent, err := yaml_utils.YamlMarshal(&rootNode) - if err != nil { - return nil, fmt.Errorf("Failed to remarsal!\n %w", err) - } - return newContent, nil - } else { - return content, nil + if reflect.DeepEqual(rootNode, originalCopy) { + return nil, false, nil } + + newContent, err := yaml_utils.YamlMarshal(&rootNode) + if err != nil { + return nil, false, fmt.Errorf("Failed to remarsal!\n %w", err) + } + return newContent, true, nil } -func changeNullKeybindingsToDisabled(rootNode *yaml.Node) error { +func changeNullKeybindingsToDisabled(rootNode *yaml.Node, changes *orderedmap.OrderedMap[string, bool]) 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 = "" node.Tag = "!!str" + changes.Set(fmt.Sprintf("Changed 'null' to '' for keybinding '%s'", path), true) } }) } -func changeElementToSequence(rootNode *yaml.Node, path []string) error { +func changeElementToSequence(rootNode *yaml.Node, path []string, changes *orderedmap.OrderedMap[string, bool]) error { return yaml_utils.TransformNode(rootNode, path, func(node *yaml.Node) error { if node.Kind == yaml.MappingNode { nodeContentCopy := node.Content @@ -327,13 +349,15 @@ func changeElementToSequence(rootNode *yaml.Node, path []string) error { Content: nodeContentCopy, }} + changes.Set(fmt.Sprintf("Changed '%s' to an array of strings", strings.Join(path, ".")), true) + return nil } return nil }) } -func changeCommitPrefixesMap(rootNode *yaml.Node) error { +func changeCommitPrefixesMap(rootNode *yaml.Node, changes *orderedmap.OrderedMap[string, bool]) error { return yaml_utils.TransformNode(rootNode, []string{"git", "commitPrefixes"}, func(prefixesNode *yaml.Node) error { if prefixesNode.Kind == yaml.MappingNode { for _, contentNode := range prefixesNode.Content { @@ -346,6 +370,7 @@ func changeCommitPrefixesMap(rootNode *yaml.Node) error { Kind: yaml.MappingNode, Content: nodeContentCopy, }} + changes.Set("Changed 'git.commitPrefixes' elements to arrays of strings", true) } } } @@ -353,7 +378,7 @@ func changeCommitPrefixesMap(rootNode *yaml.Node) error { }) } -func changeCustomCommandStreamAndOutputToOutputEnum(rootNode *yaml.Node) error { +func changeCustomCommandStreamAndOutputToOutputEnum(rootNode *yaml.Node, changes *orderedmap.OrderedMap[string, bool]) error { return yaml_utils.Walk(rootNode, func(node *yaml.Node, path string) { // We are being lazy here and rely on the fact that the only mapping // nodes in the tree under customCommands are actual custom commands. If @@ -364,16 +389,25 @@ func changeCustomCommandStreamAndOutputToOutputEnum(rootNode *yaml.Node) error { if streamKey, streamValue := yaml_utils.RemoveKey(node, "subprocess"); streamKey != nil { if streamValue.Kind == yaml.ScalarNode && streamValue.Value == "true" { output = "terminal" + changes.Set("Changed 'subprocess: true' to 'output: terminal' in custom command", true) + } else { + changes.Set("Deleted redundant 'subprocess: false' in custom command", true) } } if streamKey, streamValue := yaml_utils.RemoveKey(node, "stream"); streamKey != nil { if streamValue.Kind == yaml.ScalarNode && streamValue.Value == "true" && output == "" { output = "log" + changes.Set("Changed 'stream: true' to 'output: log' in custom command", true) + } else { + changes.Set(fmt.Sprintf("Deleted redundant 'stream: %v' property in custom command", streamValue.Value), true) } } if streamKey, streamValue := yaml_utils.RemoveKey(node, "showOutput"); streamKey != nil { if streamValue.Kind == yaml.ScalarNode && streamValue.Value == "true" && output == "" { + changes.Set("Changed 'showOutput: true' to 'output: popup' in custom command", true) output = "popup" + } else { + changes.Set(fmt.Sprintf("Deleted redundant 'showOutput: %v' property in custom command", streamValue.Value), true) } } if output != "" { @@ -397,7 +431,7 @@ func changeCustomCommandStreamAndOutputToOutputEnum(rootNode *yaml.Node) error { // a single element at `allBranchesLogCmd` and the sequence at `allBranchesLogCmds`. // Some users have explicitly set `allBranchesLogCmd` to be an empty string in order // to remove it, so in that case we just delete the element, and add nothing to the list -func migrateAllBranchesLogCmd(rootNode *yaml.Node) error { +func migrateAllBranchesLogCmd(rootNode *yaml.Node, changes *orderedmap.OrderedMap[string, bool]) error { return yaml_utils.TransformNode(rootNode, []string{"git"}, func(gitNode *yaml.Node) error { cmdKeyNode, cmdValueNode := yaml_utils.LookupKey(gitNode, "allBranchesLogCmd") // Nothing to do if they do not have the deprecated item @@ -406,6 +440,7 @@ func migrateAllBranchesLogCmd(rootNode *yaml.Node) error { } cmdsKeyNode, cmdsValueNode := yaml_utils.LookupKey(gitNode, "allBranchesLogCmds") + var change string if cmdsKeyNode == nil { // Create empty sequence node and attach it onto the root git node // We will later populate it with the individual allBranchesLogCmd record @@ -415,17 +450,24 @@ func migrateAllBranchesLogCmd(rootNode *yaml.Node) error { cmdsKeyNode, cmdsValueNode, ) - } else if cmdsValueNode.Kind != yaml.SequenceNode { - return errors.New("You should have an allBranchesLogCmds defined as a sequence!") + change = "Created git.allBranchesLogCmds array containing value of git.allBranchesLogCmd" + } else { + if cmdsValueNode.Kind != yaml.SequenceNode { + return errors.New("You should have an allBranchesLogCmds defined as a sequence!") + } + + change = "Prepended git.allBranchesLogCmd value to git.allBranchesLogCmds array" } if cmdValueNode.Value != "" { // Prepending the individual element to make it show up first in the list, which was prior behavior cmdsValueNode.Content = utils.Prepend(cmdsValueNode.Content, &yaml.Node{Kind: yaml.ScalarNode, Value: cmdValueNode.Value}) + changes.Set(change, true) } // Clear out the existing allBranchesLogCmd, now that we have migrated it into the list _, _ = yaml_utils.RemoveKey(gitNode, "allBranchesLogCmd") + changes.Set("Removed obsolete git.allBranchesLogCmd", true) return nil }) @@ -471,7 +513,7 @@ func (c *AppConfig) GetUserConfigDir() string { func (c *AppConfig) ReloadUserConfigForRepo(repoConfigFiles []*ConfigFile) error { configFiles := append(c.globalUserConfigFiles, repoConfigFiles...) - userConfig, err := loadUserConfigWithDefaults(configFiles) + userConfig, err := loadUserConfigWithDefaults(configFiles, true) if err != nil { return err } @@ -496,7 +538,7 @@ func (c *AppConfig) ReloadChangedUserConfigFiles() (error, bool) { return nil, false } - userConfig, err := loadUserConfigWithDefaults(c.userConfigFiles) + userConfig, err := loadUserConfigWithDefaults(c.userConfigFiles, true) if err != nil { return err, false } diff --git a/pkg/config/app_config_test.go b/pkg/config/app_config_test.go index 98c82d8ee..0a2843741 100644 --- a/pkg/config/app_config_test.go +++ b/pkg/config/app_config_test.go @@ -4,19 +4,176 @@ import ( "testing" "github.com/stretchr/testify/assert" + orderedmap "github.com/wk8/go-ordered-map/v2" ) +func orderedMapToSlice(m *orderedmap.OrderedMap[string, bool]) []string { + result := make([]string, 0, m.Len()) + for i := m.Oldest(); i != nil; i = i.Next() { + result = append(result, i.Key) + } + return result +} + +func TestMigrationOfRenamedKeys(t *testing.T) { + scenarios := []struct { + name string + input string + expected string + expectedDidChange bool + expectedChanges []string + }{ + { + name: "Empty String", + input: "", + expectedDidChange: false, + expectedChanges: []string{}, + }, + { + name: "No rename needed", + input: `foo: + bar: 5 +`, + expectedDidChange: false, + expectedChanges: []string{}, + }, + { + name: "Rename one", + input: `gui: + skipUnstageLineWarning: true +`, + expected: `gui: + skipDiscardChangeWarning: true +`, + expectedDidChange: true, + expectedChanges: []string{"Renamed 'gui.skipUnstageLineWarning' to 'skipDiscardChangeWarning'"}, + }, + { + name: "Rename several", + input: `gui: + windowSize: half + skipUnstageLineWarning: true +keybinding: + universal: + executeCustomCommand: a +`, + expected: `gui: + screenMode: half + skipDiscardChangeWarning: true +keybinding: + universal: + executeShellCommand: a +`, + expectedDidChange: true, + expectedChanges: []string{ + "Renamed 'gui.skipUnstageLineWarning' to 'skipDiscardChangeWarning'", + "Renamed 'keybinding.universal.executeCustomCommand' to 'executeShellCommand'", + "Renamed 'gui.windowSize' to 'screenMode'", + }, + }, + } + + for _, s := range scenarios { + t.Run(s.name, func(t *testing.T) { + changes := orderedmap.New[string, bool]() + actual, didChange, err := computeMigratedConfig("path doesn't matter", []byte(s.input), changes) + assert.NoError(t, err) + assert.Equal(t, s.expectedDidChange, didChange) + if didChange { + assert.Equal(t, s.expected, string(actual)) + } + assert.Equal(t, s.expectedChanges, orderedMapToSlice(changes)) + }) + } +} + +func TestMigrateNullKeybindingsToDisabled(t *testing.T) { + scenarios := []struct { + name string + input string + expected string + expectedDidChange bool + expectedChanges []string + }{ + { + name: "Empty String", + input: "", + expectedDidChange: false, + expectedChanges: []string{}, + }, + { + name: "No change needed", + input: `keybinding: + universal: + quit: q +`, + expectedDidChange: false, + expectedChanges: []string{}, + }, + { + name: "Change one", + input: `keybinding: + universal: + quit: null +`, + expected: `keybinding: + universal: + quit: +`, + expectedDidChange: true, + expectedChanges: []string{"Changed 'null' to '' for keybinding 'keybinding.universal.quit'"}, + }, + { + name: "Change several", + input: `keybinding: + universal: + quit: null + return: + new: null +`, + expected: `keybinding: + universal: + quit: + return: + new: +`, + expectedDidChange: true, + expectedChanges: []string{ + "Changed 'null' to '' for keybinding 'keybinding.universal.quit'", + "Changed 'null' to '' for keybinding 'keybinding.universal.new'", + }, + }, + } + + for _, s := range scenarios { + t.Run(s.name, func(t *testing.T) { + changes := orderedmap.New[string, bool]() + actual, didChange, err := computeMigratedConfig("path doesn't matter", []byte(s.input), changes) + assert.NoError(t, err) + assert.Equal(t, s.expectedDidChange, didChange) + if didChange { + assert.Equal(t, s.expected, string(actual)) + } + assert.Equal(t, s.expectedChanges, orderedMapToSlice(changes)) + }) + } +} + func TestCommitPrefixMigrations(t *testing.T) { scenarios := []struct { - name string - input string - expected string + name string + input string + expected string + expectedDidChange bool + expectedChanges []string }{ { - name: "Empty String", - input: "", - expected: "", - }, { + name: "Empty String", + input: "", + expectedDidChange: false, + expectedChanges: []string{}, + }, + { name: "Single CommitPrefix Rename", input: `git: commitPrefix: @@ -28,7 +185,10 @@ func TestCommitPrefixMigrations(t *testing.T) { - pattern: "^\\w+-\\w+.*" replace: '[JIRA $0] ' `, - }, { + expectedDidChange: true, + expectedChanges: []string{"Changed 'git.commitPrefix' to an array of strings"}, + }, + { name: "Complicated CommitPrefixes Rename", input: `git: commitPrefixes: @@ -48,13 +208,16 @@ func TestCommitPrefixMigrations(t *testing.T) { - pattern: "^foo.bar*" replace: '[FUN $0] ' `, - }, { - name: "Incomplete Configuration", - input: "git:", - expected: "git:", - }, { - // This test intentionally uses non-standard indentation to test that the migration - // does not change the input. + expectedDidChange: true, + expectedChanges: []string{"Changed 'git.commitPrefixes' elements to arrays of strings"}, + }, + { + name: "Incomplete Configuration", + input: "git:", + expectedDidChange: false, + expectedChanges: []string{}, + }, + { name: "No changes made when already migrated", input: ` git: @@ -65,40 +228,40 @@ 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, + expectedChanges: []string{}, }, } for _, s := range scenarios { t.Run(s.name, func(t *testing.T) { - actual, err := computeMigratedConfig("path doesn't matter", []byte(s.input)) - if err != nil { - t.Error(err) + changes := orderedmap.New[string, bool]() + actual, didChange, err := computeMigratedConfig("path doesn't matter", []byte(s.input), changes) + assert.NoError(t, err) + assert.Equal(t, s.expectedDidChange, didChange) + if didChange { + assert.Equal(t, s.expected, string(actual)) } - assert.Equal(t, s.expected, string(actual)) + assert.Equal(t, s.expectedChanges, orderedMapToSlice(changes)) }) } } func TestCustomCommandsOutputMigration(t *testing.T) { scenarios := []struct { - name string - input string - expected string + name string + input string + expected string + expectedDidChange bool + expectedChanges []string }{ { - name: "Empty String", - input: "", - expected: "", - }, { + name: "Empty String", + input: "", + expectedDidChange: false, + expectedChanges: []string{}, + }, + { name: "Convert subprocess to output=terminal", input: `customCommands: - command: echo 'hello' @@ -108,7 +271,10 @@ func TestCustomCommandsOutputMigration(t *testing.T) { - command: echo 'hello' output: terminal `, - }, { + expectedDidChange: true, + expectedChanges: []string{"Changed 'subprocess: true' to 'output: terminal' in custom command"}, + }, + { name: "Convert stream to output=log", input: `customCommands: - command: echo 'hello' @@ -118,7 +284,10 @@ func TestCustomCommandsOutputMigration(t *testing.T) { - command: echo 'hello' output: log `, - }, { + expectedDidChange: true, + expectedChanges: []string{"Changed 'stream: true' to 'output: log' in custom command"}, + }, + { name: "Convert showOutput to output=popup", input: `customCommands: - command: echo 'hello' @@ -128,7 +297,10 @@ func TestCustomCommandsOutputMigration(t *testing.T) { - command: echo 'hello' output: popup `, - }, { + expectedDidChange: true, + expectedChanges: []string{"Changed 'showOutput: true' to 'output: popup' in custom command"}, + }, + { name: "Subprocess wins over the other two", input: `customCommands: - command: echo 'hello' @@ -140,7 +312,14 @@ func TestCustomCommandsOutputMigration(t *testing.T) { - command: echo 'hello' output: terminal `, - }, { + expectedDidChange: true, + expectedChanges: []string{ + "Changed 'subprocess: true' to 'output: terminal' in custom command", + "Deleted redundant 'stream: true' property in custom command", + "Deleted redundant 'showOutput: true' property in custom command", + }, + }, + { name: "Stream wins over showOutput", input: `customCommands: - command: echo 'hello' @@ -151,7 +330,13 @@ func TestCustomCommandsOutputMigration(t *testing.T) { - command: echo 'hello' output: log `, - }, { + expectedDidChange: true, + expectedChanges: []string{ + "Changed 'stream: true' to 'output: log' in custom command", + "Deleted redundant 'showOutput: true' property in custom command", + }, + }, + { name: "Explicitly setting to false doesn't create an output=none key", input: `customCommands: - command: echo 'hello' @@ -162,14 +347,25 @@ func TestCustomCommandsOutputMigration(t *testing.T) { expected: `customCommands: - command: echo 'hello' `, + expectedDidChange: true, + expectedChanges: []string{ + "Deleted redundant 'subprocess: false' in custom command", + "Deleted redundant 'stream: false' property in custom command", + "Deleted redundant 'showOutput: false' property in custom command", + }, }, } for _, s := range scenarios { t.Run(s.name, func(t *testing.T) { - actual, err := computeMigratedConfig("path doesn't matter", []byte(s.input)) + changes := orderedmap.New[string, bool]() + actual, didChange, err := computeMigratedConfig("path doesn't matter", []byte(s.input), changes) 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)) + } + assert.Equal(t, s.expectedChanges, orderedMapToSlice(changes)) }) } } @@ -778,21 +974,26 @@ keybinding: func BenchmarkMigrationOnLargeConfiguration(b *testing.B) { for b.Loop() { - _, _ = computeMigratedConfig("path doesn't matter", largeConfiguration) + changes := orderedmap.New[string, bool]() + _, _, _ = computeMigratedConfig("path doesn't matter", largeConfiguration, changes) } } func TestAllBranchesLogCmdMigrations(t *testing.T) { scenarios := []struct { - name string - input string - expected string + name string + input string + expected string + expectedDidChange bool + expectedChanges []string }{ { - name: "Incomplete Configuration Passes uneventfully", - input: "git:", - expected: "git:", - }, { + name: "Incomplete Configuration Passes uneventfully", + input: "git:", + expectedDidChange: false, + expectedChanges: []string{}, + }, + { name: "Single Cmd with no Cmds", input: `git: allBranchesLogCmd: git log --graph --oneline @@ -801,7 +1002,13 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) { allBranchesLogCmds: - git log --graph --oneline `, - }, { + expectedDidChange: true, + expectedChanges: []string{ + "Created git.allBranchesLogCmds array containing value of git.allBranchesLogCmd", + "Removed obsolete git.allBranchesLogCmd", + }, + }, + { name: "Cmd with one existing Cmds", input: `git: allBranchesLogCmd: git log --graph --oneline @@ -813,17 +1020,22 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) { - git log --graph --oneline - git log --graph --oneline --pretty `, - }, { + expectedDidChange: true, + expectedChanges: []string{ + "Prepended git.allBranchesLogCmd value to git.allBranchesLogCmds array", + "Removed obsolete git.allBranchesLogCmd", + }, + }, + { name: "Only Cmds set have no changes", input: `git: allBranchesLogCmds: - git log `, - expected: `git: - allBranchesLogCmds: - - git log -`, - }, { + expected: "", + expectedChanges: []string{}, + }, + { name: "Removes Empty Cmd When at end of yaml", input: `git: allBranchesLogCmds: @@ -834,7 +1046,10 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) { allBranchesLogCmds: - git log --graph --oneline `, - }, { + expectedDidChange: true, + expectedChanges: []string{"Removed obsolete git.allBranchesLogCmd"}, + }, + { name: "Migrates when sequence defined inline", input: `git: allBranchesLogCmds: [foo, bar] @@ -843,7 +1058,13 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) { expected: `git: allBranchesLogCmds: [baz, foo, bar] `, - }, { + expectedDidChange: true, + expectedChanges: []string{ + "Prepended git.allBranchesLogCmd value to git.allBranchesLogCmds array", + "Removed obsolete git.allBranchesLogCmd", + }, + }, + { name: "Removes Empty Cmd With Keys Afterwards", input: `git: allBranchesLogCmds: @@ -856,14 +1077,21 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) { - git log --graph --oneline foo: bar `, + expectedDidChange: true, + expectedChanges: []string{"Removed obsolete git.allBranchesLogCmd"}, }, } for _, s := range scenarios { t.Run(s.name, func(t *testing.T) { - actual, err := computeMigratedConfig("path doesn't matter", []byte(s.input)) + changes := orderedmap.New[string, bool]() + actual, didChange, err := computeMigratedConfig("path doesn't matter", []byte(s.input), changes) 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)) + } + assert.Equal(t, s.expectedChanges, orderedMapToSlice(changes)) }) } } diff --git a/pkg/utils/yaml_utils/yaml_utils.go b/pkg/utils/yaml_utils/yaml_utils.go index 391ba8a4f..432915f5e 100644 --- a/pkg/utils/yaml_utils/yaml_utils.go +++ b/pkg/utils/yaml_utils/yaml_utils.go @@ -65,41 +65,37 @@ func transformNode(node *yaml.Node, path []string, transform func(node *yaml.Nod // 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(rootNode *yaml.Node, path []string, newKey string) error { +func RenameYamlKey(rootNode *yaml.Node, path []string, newKey string) (error, bool) { // Empty document: nothing to do. if len(rootNode.Content) == 0 { - return nil + return nil, false } body := rootNode.Content[0] - if err := renameYamlKey(body, path, newKey); err != nil { - return err - } - - return nil + return renameYamlKey(body, path, newKey) } // Recursive function to rename the YAML key. -func renameYamlKey(node *yaml.Node, path []string, newKey string) error { +func renameYamlKey(node *yaml.Node, path []string, newKey string) (error, bool) { if node.Kind != yaml.MappingNode { - return errors.New("yaml node in path is not a dictionary") + return errors.New("yaml node in path is not a dictionary"), false } keyNode, valueNode := LookupKey(node, path[0]) if keyNode == nil { - return nil + return nil, false } // end of path reached: rename key if len(path) == 1 { // Check that new key doesn't exist yet if newKeyNode, _ := LookupKey(node, newKey); newKeyNode != nil { - return fmt.Errorf("new key `%s' already exists", newKey) + return fmt.Errorf("new key `%s' already exists", newKey), false } keyNode.Value = newKey - return nil + return nil, true } return renameYamlKey(valueNode, path[1:], newKey) diff --git a/pkg/utils/yaml_utils/yaml_utils_test.go b/pkg/utils/yaml_utils/yaml_utils_test.go index b49130ea7..9ae099396 100644 --- a/pkg/utils/yaml_utils/yaml_utils_test.go +++ b/pkg/utils/yaml_utils/yaml_utils_test.go @@ -10,77 +10,85 @@ import ( func TestRenameYamlKey(t *testing.T) { tests := []struct { - name string - in string - path []string - newKey string - expectedOut string - expectedErr string + name string + in string + path []string + newKey string + expectedOut string + expectedDidRename bool + expectedErr string }{ { - name: "rename key", - in: "foo: 5\n", - path: []string{"foo"}, - newKey: "bar", - expectedOut: "bar: 5\n", - expectedErr: "", + name: "rename key", + in: "foo: 5\n", + path: []string{"foo"}, + newKey: "bar", + expectedOut: "bar: 5\n", + expectedDidRename: true, + expectedErr: "", }, { - name: "rename key, nested", - in: "foo:\n bar: 5\n", - path: []string{"foo", "bar"}, - newKey: "baz", - expectedOut: "foo:\n baz: 5\n", - expectedErr: "", + name: "rename key, nested", + in: "foo:\n bar: 5\n", + path: []string{"foo", "bar"}, + newKey: "baz", + expectedOut: "foo:\n baz: 5\n", + expectedDidRename: true, + expectedErr: "", }, { - name: "rename non-scalar key", - in: "foo:\n bar: 5\n", - path: []string{"foo"}, - newKey: "qux", - expectedOut: "qux:\n bar: 5\n", - expectedErr: "", + name: "rename non-scalar key", + in: "foo:\n bar: 5\n", + path: []string{"foo"}, + newKey: "qux", + expectedOut: "qux:\n bar: 5\n", + expectedDidRename: true, + expectedErr: "", }, { - name: "don't rewrite file if value didn't change", - in: "foo:\n bar: 5\n", - path: []string{"nonExistingKey"}, - newKey: "qux", - expectedOut: "foo:\n bar: 5\n", - expectedErr: "", + name: "don't rewrite file if value didn't change", + in: "foo:\n bar: 5\n", + path: []string{"nonExistingKey"}, + newKey: "qux", + expectedOut: "foo:\n bar: 5\n", + expectedDidRename: false, + expectedErr: "", }, // Error cases { - name: "existing document is not a dictionary", - in: "42\n", - path: []string{"foo"}, - newKey: "bar", - expectedOut: "42\n", - expectedErr: "yaml node in path is not a dictionary", + name: "existing document is not a dictionary", + in: "42\n", + path: []string{"foo"}, + newKey: "bar", + expectedOut: "42\n", + expectedDidRename: false, + expectedErr: "yaml node in path is not a dictionary", }, { - name: "not all path elements are dictionaries", - in: "foo:\n bar: [1, 2, 3]\n", - path: []string{"foo", "bar", "baz"}, - newKey: "qux", - expectedOut: "foo:\n bar: [1, 2, 3]\n", - expectedErr: "yaml node in path is not a dictionary", + name: "not all path elements are dictionaries", + in: "foo:\n bar: [1, 2, 3]\n", + path: []string{"foo", "bar", "baz"}, + newKey: "qux", + expectedOut: "foo:\n bar: [1, 2, 3]\n", + expectedDidRename: false, + expectedErr: "yaml node in path is not a dictionary", }, { - name: "new key exists", - in: "foo: 5\nbar: 7\n", - path: []string{"foo"}, - newKey: "bar", - expectedOut: "foo: 5\nbar: 7\n", - expectedErr: "new key `bar' already exists", + name: "new key exists", + in: "foo: 5\nbar: 7\n", + path: []string{"foo"}, + newKey: "bar", + expectedOut: "foo: 5\nbar: 7\n", + expectedDidRename: false, + expectedErr: "new key `bar' already exists", }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { node := unmarshalForTest(t, test.in) - actualErr := RenameYamlKey(&node, test.path, test.newKey) + actualErr, didRename := RenameYamlKey(&node, test.path, test.newKey) if test.expectedErr == "" { assert.NoError(t, actualErr) } else { @@ -89,6 +97,8 @@ func TestRenameYamlKey(t *testing.T) { out := marshalForTest(t, &node) assert.Equal(t, test.expectedOut, out) + + assert.Equal(t, test.expectedDidRename, didRename) }) } }