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..e3bc4cd40 100644 --- a/pkg/config/app_config.go +++ b/pkg/config/app_config.go @@ -11,6 +11,7 @@ import ( "time" "github.com/adrg/xdg" + "github.com/jesseduffield/generics/orderedset" "github.com/jesseduffield/lazygit/pkg/utils" "github.com/jesseduffield/lazygit/pkg/utils/yaml_utils" "github.com/samber/lo" @@ -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 } @@ -215,41 +216,64 @@ func loadUserConfig(configFiles []*ConfigFile, base *UserConfig) (*UserConfig, e return base, nil } +type ChangesSet = orderedset.OrderedSet[string] + +func NewChangesSet() *ChangesSet { + return orderedset.New[string]() +} + // Do any backward-compatibility migrations of things that have changed in the // 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 := NewChangesSet() + + 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" + changesText += strings.Join(lo.Map(changes.ToSliceFromOldest(), func(change string, _ int) string { + return fmt.Sprintf("- %s\n", change) + }), "") + + // 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 *ChangesSet) ([]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 +286,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.Add(fmt.Sprintf("Renamed '%s' to '%s'", strings.Join(pathToReplace.oldPath, "."), pathToReplace.newName)) } } - 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 *ChangesSet) 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.Add(fmt.Sprintf("Changed 'null' to '' for keybinding '%s'", path)) } }) } -func changeElementToSequence(rootNode *yaml.Node, path []string) error { +func changeElementToSequence(rootNode *yaml.Node, path []string, changes *ChangesSet) error { return yaml_utils.TransformNode(rootNode, path, func(node *yaml.Node) error { if node.Kind == yaml.MappingNode { nodeContentCopy := node.Content @@ -327,13 +355,15 @@ func changeElementToSequence(rootNode *yaml.Node, path []string) error { Content: nodeContentCopy, }} + changes.Add(fmt.Sprintf("Changed '%s' to an array of strings", strings.Join(path, "."))) + return nil } return nil }) } -func changeCommitPrefixesMap(rootNode *yaml.Node) error { +func changeCommitPrefixesMap(rootNode *yaml.Node, changes *ChangesSet) 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 +376,7 @@ func changeCommitPrefixesMap(rootNode *yaml.Node) error { Kind: yaml.MappingNode, Content: nodeContentCopy, }} + changes.Add("Changed 'git.commitPrefixes' elements to arrays of strings") } } } @@ -353,7 +384,7 @@ func changeCommitPrefixesMap(rootNode *yaml.Node) error { }) } -func changeCustomCommandStreamAndOutputToOutputEnum(rootNode *yaml.Node) error { +func changeCustomCommandStreamAndOutputToOutputEnum(rootNode *yaml.Node, changes *ChangesSet) 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 +395,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.Add("Changed 'subprocess: true' to 'output: terminal' in custom command") + } else { + changes.Add("Deleted redundant 'subprocess: false' in custom command") } } if streamKey, streamValue := yaml_utils.RemoveKey(node, "stream"); streamKey != nil { if streamValue.Kind == yaml.ScalarNode && streamValue.Value == "true" && output == "" { output = "log" + changes.Add("Changed 'stream: true' to 'output: log' in custom command") + } else { + changes.Add(fmt.Sprintf("Deleted redundant 'stream: %v' property in custom command", streamValue.Value)) } } if streamKey, streamValue := yaml_utils.RemoveKey(node, "showOutput"); streamKey != nil { if streamValue.Kind == yaml.ScalarNode && streamValue.Value == "true" && output == "" { + changes.Add("Changed 'showOutput: true' to 'output: popup' in custom command") output = "popup" + } else { + changes.Add(fmt.Sprintf("Deleted redundant 'showOutput: %v' property in custom command", streamValue.Value)) } } if output != "" { @@ -397,7 +437,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 *ChangesSet) 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 +446,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 +456,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.Add(change) } // Clear out the existing allBranchesLogCmd, now that we have migrated it into the list _, _ = yaml_utils.RemoveKey(gitNode, "allBranchesLogCmd") + changes.Add("Removed obsolete git.allBranchesLogCmd") return nil }) @@ -471,7 +519,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 +544,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..90c13ce6c 100644 --- a/pkg/config/app_config_test.go +++ b/pkg/config/app_config_test.go @@ -6,17 +6,165 @@ import ( "github.com/stretchr/testify/assert" ) -func TestCommitPrefixMigrations(t *testing.T) { +func TestMigrationOfRenamedKeys(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: "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 := NewChangesSet() + 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, changes.ToSliceFromOldest()) + }) + } +} + +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 := NewChangesSet() + 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, changes.ToSliceFromOldest()) + }) + } +} + +func TestCommitPrefixMigrations(t *testing.T) { + scenarios := []struct { + name string + input string + expected string + expectedDidChange bool + expectedChanges []string + }{ + { + name: "Empty String", + input: "", + expectedDidChange: false, + expectedChanges: []string{}, + }, + { name: "Single CommitPrefix Rename", input: `git: commitPrefix: @@ -28,7 +176,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 +199,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 +219,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 := NewChangesSet() + 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, changes.ToSliceFromOldest()) }) } } 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 +262,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 +275,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 +288,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 +303,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 +321,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 +338,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 := NewChangesSet() + 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, changes.ToSliceFromOldest()) }) } } @@ -778,21 +965,26 @@ keybinding: func BenchmarkMigrationOnLargeConfiguration(b *testing.B) { for b.Loop() { - _, _ = computeMigratedConfig("path doesn't matter", largeConfiguration) + changes := NewChangesSet() + _, _, _ = 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 +993,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 +1011,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 +1037,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 +1049,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 +1068,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 := NewChangesSet() + 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, changes.ToSliceFromOldest()) }) } } 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) }) } } diff --git a/vendor/github.com/jesseduffield/generics/orderedset/orderedset.go b/vendor/github.com/jesseduffield/generics/orderedset/orderedset.go new file mode 100644 index 000000000..ed5c95b54 --- /dev/null +++ b/vendor/github.com/jesseduffield/generics/orderedset/orderedset.go @@ -0,0 +1,65 @@ +package orderedset + +import ( + orderedmap "github.com/wk8/go-ordered-map/v2" +) + +type OrderedSet[T comparable] struct { + om *orderedmap.OrderedMap[T, bool] +} + +func New[T comparable]() *OrderedSet[T] { + return &OrderedSet[T]{om: orderedmap.New[T, bool]()} +} + +func NewFromSlice[T comparable](slice []T) *OrderedSet[T] { + result := &OrderedSet[T]{om: orderedmap.New[T, bool](len(slice))} + result.Add(slice...) + return result +} + +func (os *OrderedSet[T]) Add(values ...T) { + for _, value := range values { + os.om.Set(value, true) + } +} + +func (os *OrderedSet[T]) Remove(value T) { + os.om.Delete(value) +} + +func (os *OrderedSet[T]) RemoveSlice(slice []T) { + for _, value := range slice { + os.Remove(value) + } +} + +func (os *OrderedSet[T]) Includes(value T) bool { + return os.om.Value(value) +} + +func (os *OrderedSet[T]) Len() int { + return os.om.Len() +} + +func (os *OrderedSet[T]) ToSliceFromOldest() []T { + // TODO: can be simplified to + // return os.om.KeysFromOldest() + // when we update to a newer version of go-ordered-map + result := make([]T, 0, os.Len()) + for pair := os.om.Oldest(); pair != nil; pair = pair.Next() { + result = append(result, pair.Key) + } + return result +} + +func (os *OrderedSet[T]) ToSliceFromNewest() []T { + // TODO: can be simplified to + // return os.om.KeysFromNewest() + // when we update to a newer version of go-ordered-map + result := make([]T, 0, os.Len()) + for pair := os.om.Newest(); pair != nil; pair = pair.Prev() { + result = append(result, pair.Key) + } + return result +} diff --git a/vendor/modules.txt b/vendor/modules.txt index 9859a3944..f47a39f1d 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -177,6 +177,7 @@ github.com/jbenet/go-context/io # github.com/jesseduffield/generics v0.0.0-20250406224309-4f541cb84918 ## explicit; go 1.18 github.com/jesseduffield/generics/maps +github.com/jesseduffield/generics/orderedset github.com/jesseduffield/generics/set # github.com/jesseduffield/go-git/v5 v5.14.1-0.20250407170251-e1a013310ccd ## explicit; go 1.23.0