From 5686e63af31b7c199683e9746ecaa17391c42a57 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 17 May 2025 15:38:29 +0200 Subject: [PATCH 01/12] Bump generics dependency --- go.mod | 2 +- go.sum | 4 +- .../jesseduffield/generics/maps/maps.go | 4 +- .../generics/orderedset/orderedset.go | 65 +++++++++++++++++++ .../jesseduffield/generics/set/set.go | 9 +-- vendor/modules.txt | 3 +- 6 files changed, 75 insertions(+), 12 deletions(-) create mode 100644 vendor/github.com/jesseduffield/generics/orderedset/orderedset.go diff --git a/go.mod b/go.mod index 30ae0e97806..46783393e44 100644 --- a/go.mod +++ b/go.mod @@ -13,7 +13,7 @@ require ( github.com/go-errors/errors v1.5.1 github.com/gookit/color v1.4.2 github.com/integrii/flaggy v1.4.0 - github.com/jesseduffield/generics v0.0.0-20250406224309-4f541cb84918 + github.com/jesseduffield/generics v0.0.0-20250517122708-b0b4a53a6f5c github.com/jesseduffield/go-git/v5 v5.14.1-0.20250407170251-e1a013310ccd github.com/jesseduffield/gocui v0.3.1-0.20250421160159-82c9aaeba2b9 github.com/jesseduffield/kill v0.0.0-20250101124109-e216ddbe133a diff --git a/go.sum b/go.sum index 3c1aaab393d..85db0bcca21 100644 --- a/go.sum +++ b/go.sum @@ -190,8 +190,8 @@ github.com/invopop/jsonschema v0.10.0 h1:c1ktzNLBun3LyQQhyty5WE3lulbOdIIyOVlkmDL github.com/invopop/jsonschema v0.10.0/go.mod h1:ffZ5Km5SWWRAIN6wbDXItl95euhFz2uON45H2qjYt+0= github.com/jbenet/go-context v0.0.0-20150711004518-d14ea06fba99 h1:BQSFePA1RWJOlocH6Fxy8MmwDt+yVQYULKfN0RoTN8A= github.com/jbenet/go-context v0.0.0-20150711004518-d14ea06fba99/go.mod h1:1lJo3i6rXxKeerYnT8Nvf0QmHCRC1n8sfWVwXF2Frvo= -github.com/jesseduffield/generics v0.0.0-20250406224309-4f541cb84918 h1:meoUDZGF6jZAbhW5IBwj92mTqGmrOn+Cuu0jM7/aUcs= -github.com/jesseduffield/generics v0.0.0-20250406224309-4f541cb84918/go.mod h1:+LLj9/WUPAP8LqCchs7P+7X0R98HiFujVFANdNaxhGk= +github.com/jesseduffield/generics v0.0.0-20250517122708-b0b4a53a6f5c h1:tC2PaiisXAC5sOjDPfMArSnbswDObtCssx+xn28edX4= +github.com/jesseduffield/generics v0.0.0-20250517122708-b0b4a53a6f5c/go.mod h1:F2fEBk0ddf6ixrBrJjY7phfQ3hL9rXG0uSjvwYe50bE= github.com/jesseduffield/go-git/v5 v5.14.1-0.20250407170251-e1a013310ccd h1:ViKj6qth8FgcIWizn9KiACWwPemWSymx62OPN0tHT+Q= github.com/jesseduffield/go-git/v5 v5.14.1-0.20250407170251-e1a013310ccd/go.mod h1:lRhCiBr6XjQrvcQVa+UYsy/99d3wMXn/a0nSQlhnhlA= github.com/jesseduffield/gocui v0.3.1-0.20250421160159-82c9aaeba2b9 h1:k23sCKHCNpAvwJP8Yr16CBUItuarmUHBGH7FaAm2glc= diff --git a/vendor/github.com/jesseduffield/generics/maps/maps.go b/vendor/github.com/jesseduffield/generics/maps/maps.go index 9d41a3303a9..26d9c32c2b0 100644 --- a/vendor/github.com/jesseduffield/generics/maps/maps.go +++ b/vendor/github.com/jesseduffield/generics/maps/maps.go @@ -19,7 +19,7 @@ func Values[Key comparable, Value any](m map[Key]Value) []Value { func TransformValues[Key comparable, Value any, NewValue any]( m map[Key]Value, fn func(Value) NewValue, ) map[Key]NewValue { - output := make(map[Key]NewValue) + output := make(map[Key]NewValue, len(m)) for key, value := range m { output[key] = fn(value) } @@ -27,7 +27,7 @@ func TransformValues[Key comparable, Value any, NewValue any]( } func TransformKeys[Key comparable, Value any, NewKey comparable](m map[Key]Value, fn func(Key) NewKey) map[NewKey]Value { - output := make(map[NewKey]Value) + output := make(map[NewKey]Value, len(m)) for key, value := range m { output[fn(key)] = value } 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 00000000000..ed5c95b54c9 --- /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/github.com/jesseduffield/generics/set/set.go b/vendor/github.com/jesseduffield/generics/set/set.go index 0f0f4beb13f..5b8f0090e4e 100644 --- a/vendor/github.com/jesseduffield/generics/set/set.go +++ b/vendor/github.com/jesseduffield/generics/set/set.go @@ -11,12 +11,9 @@ func New[T comparable]() *Set[T] { } func NewFromSlice[T comparable](slice []T) *Set[T] { - hashMap := make(map[T]bool) - for _, value := range slice { - hashMap[value] = true - } - - return &Set[T]{hashMap: hashMap} + result := &Set[T]{hashMap: make(map[T]bool, len(slice))} + result.Add(slice...) + return result } func (s *Set[T]) Add(values ...T) { diff --git a/vendor/modules.txt b/vendor/modules.txt index 9859a3944de..f2f6489effd 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -174,9 +174,10 @@ github.com/integrii/flaggy # github.com/jbenet/go-context v0.0.0-20150711004518-d14ea06fba99 ## explicit github.com/jbenet/go-context/io -# github.com/jesseduffield/generics v0.0.0-20250406224309-4f541cb84918 +# github.com/jesseduffield/generics v0.0.0-20250517122708-b0b4a53a6f5c ## 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 From bf13e0bc6a09a4b949b59a075ef551ab34dc7992 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 4 May 2025 12:44:39 +0200 Subject: [PATCH 02/12] Cleanup: use assert.NoError --- pkg/config/app_config_test.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/pkg/config/app_config_test.go b/pkg/config/app_config_test.go index 98c82d8ee63..3e2290c92bd 100644 --- a/pkg/config/app_config_test.go +++ b/pkg/config/app_config_test.go @@ -80,9 +80,7 @@ git: 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) - } + assert.NoError(t, err) assert.Equal(t, s.expected, string(actual)) }) } From da5789ba5fa6b8bd1b2e24051c830bcefc0e03b7 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 4 May 2025 12:57:11 +0200 Subject: [PATCH 03/12] Cleanup: flip conditions for less indentation --- pkg/config/app_config.go | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/pkg/config/app_config.go b/pkg/config/app_config.go index 86e82772426..400ab15d5d7 100644 --- a/pkg/config/app_config.go +++ b/pkg/config/app_config.go @@ -225,17 +225,18 @@ func migrateUserConfig(path string, content []byte) ([]byte, error) { 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 string(changedContent) == string(content) { + return content, nil } - return content, nil + // Write config back + 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 } // A pure function helper for testing purposes @@ -295,15 +296,15 @@ func computeMigratedConfig(path string, content []byte) ([]byte, error) { // 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 { + if reflect.DeepEqual(rootNode, originalCopy) { return content, nil } + + newContent, err := yaml_utils.YamlMarshal(&rootNode) + if err != nil { + return nil, fmt.Errorf("Failed to remarsal!\n %w", err) + } + return newContent, nil } func changeNullKeybindingsToDisabled(rootNode *yaml.Node) error { From 4f959da9f82e675e247f88af94abeb1920c3e922 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 8 May 2025 09:19:16 +0200 Subject: [PATCH 04/12] Cleanup: fix formatting of test cases --- pkg/config/app_config_test.go | 48 +++++++++++++++++++++++------------ 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/pkg/config/app_config_test.go b/pkg/config/app_config_test.go index 3e2290c92bd..d1f165cb3ec 100644 --- a/pkg/config/app_config_test.go +++ b/pkg/config/app_config_test.go @@ -16,7 +16,8 @@ func TestCommitPrefixMigrations(t *testing.T) { name: "Empty String", input: "", expected: "", - }, { + }, + { name: "Single CommitPrefix Rename", input: `git: commitPrefix: @@ -28,7 +29,8 @@ func TestCommitPrefixMigrations(t *testing.T) { - pattern: "^\\w+-\\w+.*" replace: '[JIRA $0] ' `, - }, { + }, + { name: "Complicated CommitPrefixes Rename", input: `git: commitPrefixes: @@ -48,11 +50,13 @@ 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. name: "No changes made when already migrated", @@ -96,7 +100,8 @@ func TestCustomCommandsOutputMigration(t *testing.T) { name: "Empty String", input: "", expected: "", - }, { + }, + { name: "Convert subprocess to output=terminal", input: `customCommands: - command: echo 'hello' @@ -106,7 +111,8 @@ func TestCustomCommandsOutputMigration(t *testing.T) { - command: echo 'hello' output: terminal `, - }, { + }, + { name: "Convert stream to output=log", input: `customCommands: - command: echo 'hello' @@ -116,7 +122,8 @@ func TestCustomCommandsOutputMigration(t *testing.T) { - command: echo 'hello' output: log `, - }, { + }, + { name: "Convert showOutput to output=popup", input: `customCommands: - command: echo 'hello' @@ -126,7 +133,8 @@ func TestCustomCommandsOutputMigration(t *testing.T) { - command: echo 'hello' output: popup `, - }, { + }, + { name: "Subprocess wins over the other two", input: `customCommands: - command: echo 'hello' @@ -138,7 +146,8 @@ func TestCustomCommandsOutputMigration(t *testing.T) { - command: echo 'hello' output: terminal `, - }, { + }, + { name: "Stream wins over showOutput", input: `customCommands: - command: echo 'hello' @@ -149,7 +158,8 @@ func TestCustomCommandsOutputMigration(t *testing.T) { - command: echo 'hello' output: log `, - }, { + }, + { name: "Explicitly setting to false doesn't create an output=none key", input: `customCommands: - command: echo 'hello' @@ -790,7 +800,8 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) { name: "Incomplete Configuration Passes uneventfully", input: "git:", expected: "git:", - }, { + }, + { name: "Single Cmd with no Cmds", input: `git: allBranchesLogCmd: git log --graph --oneline @@ -799,7 +810,8 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) { allBranchesLogCmds: - git log --graph --oneline `, - }, { + }, + { name: "Cmd with one existing Cmds", input: `git: allBranchesLogCmd: git log --graph --oneline @@ -811,7 +823,8 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) { - git log --graph --oneline - git log --graph --oneline --pretty `, - }, { + }, + { name: "Only Cmds set have no changes", input: `git: allBranchesLogCmds: @@ -821,7 +834,8 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) { allBranchesLogCmds: - git log `, - }, { + }, + { name: "Removes Empty Cmd When at end of yaml", input: `git: allBranchesLogCmds: @@ -832,7 +846,8 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) { allBranchesLogCmds: - git log --graph --oneline `, - }, { + }, + { name: "Migrates when sequence defined inline", input: `git: allBranchesLogCmds: [foo, bar] @@ -841,7 +856,8 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) { expected: `git: allBranchesLogCmds: [baz, foo, bar] `, - }, { + }, + { name: "Removes Empty Cmd With Keys Afterwards", input: `git: allBranchesLogCmds: From 0249d4c8ab40226960c25ed13872d3a8497d05c9 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 4 May 2025 12:54:29 +0200 Subject: [PATCH 05/12] 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 | 28 +++++----- pkg/config/app_config_test.go | 98 ++++++++++++++++++++--------------- 2 files changed, 69 insertions(+), 57 deletions(-) diff --git a/pkg/config/app_config.go b/pkg/config/app_config.go index 400ab15d5d7..180457fdfe5 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 d1f165cb3ec..c9554baa72b 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)) + } }) } } From df805f3a1a2f6626e6f31ff1dc51d01c488a0dfd Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 8 May 2025 09:17:42 +0200 Subject: [PATCH 06/12] Add tests for migration of renamed keys --- pkg/config/app_config_test.go | 61 +++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/pkg/config/app_config_test.go b/pkg/config/app_config_test.go index c9554baa72b..a4f31ecc86e 100644 --- a/pkg/config/app_config_test.go +++ b/pkg/config/app_config_test.go @@ -6,6 +6,67 @@ import ( "github.com/stretchr/testify/assert" ) +func TestMigrationOfRenamedKeys(t *testing.T) { + scenarios := []struct { + name string + input string + expected string + expectedDidChange bool + }{ + { + name: "Empty String", + input: "", + expectedDidChange: false, + }, + { + name: "No rename needed", + input: `foo: + bar: 5 +`, + expectedDidChange: false, + }, + { + name: "Rename one", + input: `gui: + skipUnstageLineWarning: true +`, + expected: `gui: + skipDiscardChangeWarning: true +`, + expectedDidChange: true, + }, + { + 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, + }, + } + + for _, s := range scenarios { + t.Run(s.name, func(t *testing.T) { + 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)) + } + }) + } +} + func TestCommitPrefixMigrations(t *testing.T) { scenarios := []struct { name string From 61822b73f06cd97e43faed9bd808326eae83867d Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 8 May 2025 09:37:44 +0200 Subject: [PATCH 07/12] Add tests for migrating null keybindings to --- pkg/config/app_config_test.go | 62 +++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/pkg/config/app_config_test.go b/pkg/config/app_config_test.go index a4f31ecc86e..6f15b6d4ae6 100644 --- a/pkg/config/app_config_test.go +++ b/pkg/config/app_config_test.go @@ -67,6 +67,68 @@ keybinding: } } +func TestMigrateNullKeybindingsToDisabled(t *testing.T) { + scenarios := []struct { + name string + input string + expected string + expectedDidChange bool + }{ + { + name: "Empty String", + input: "", + expectedDidChange: false, + }, + { + name: "No change needed", + input: `keybinding: + universal: + quit: q +`, + expectedDidChange: false, + }, + { + name: "Change one", + input: `keybinding: + universal: + quit: null +`, + expected: `keybinding: + universal: + quit: +`, + expectedDidChange: true, + }, + { + name: "Change several", + input: `keybinding: + universal: + quit: null + return: + new: null +`, + expected: `keybinding: + universal: + quit: + return: + new: +`, + expectedDidChange: true, + }, + } + + for _, s := range scenarios { + t.Run(s.name, func(t *testing.T) { + 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)) + } + }) + } +} + func TestCommitPrefixMigrations(t *testing.T) { scenarios := []struct { name string From ffda51014d9619712dd8313d3e712c7489bbd641 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 3 May 2025 20:44:07 +0200 Subject: [PATCH 08/12] Print migration hints only when GUI hasn't started yet Most migrations happen at startup when loading the global config file, at a time where the GUI hasn't been initialized yet. We can safely print to the console at that point. However, it is also possible that repo-local config files need to be migrated, and this happens when the GUI has already started, at which point we had better not print anything to stdout; this totally messes up the UI. In this commit we simply suppress the logging when the GUI is running already. This is probably good enough, because the logging is mostly useful in the case that writing back the migrated config file fails, so that users understand better why lazygit doesn't start up; and this is very unlikely to happen for repo-local config files, because why would users make them read-only. --- pkg/config/app_config.go | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/pkg/config/app_config.go b/pkg/config/app_config.go index 180457fdfe5..ae1b0d38ea4 100644 --- a/pkg/config/app_config.go +++ b/pkg/config/app_config.go @@ -96,7 +96,7 @@ func NewAppConfig( configFiles = []*ConfigFile{configFile} } - userConfig, err := loadUserConfigWithDefaults(configFiles) + userConfig, err := loadUserConfigWithDefaults(configFiles, false) if err != nil { return nil, err } @@ -145,11 +145,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 +194,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,7 +219,7 @@ 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) { +func migrateUserConfig(path string, content []byte, isGuiInitialized bool) ([]byte, error) { changedContent, didChange, err := computeMigratedConfig(path, content) if err != nil { return nil, err @@ -231,11 +231,15 @@ func migrateUserConfig(path string, content []byte) ([]byte, error) { } // Write config back - fmt.Println("Provided user config is deprecated but auto-fixable. Attempting to write fixed version back to file...") + if !isGuiInitialized { + 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) + if !isGuiInitialized { + fmt.Printf("Success. New config written to %s\n", path) + } return changedContent, nil } @@ -472,7 +476,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 } @@ -497,7 +501,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 } From 48305c18f7e21062cdb72e0dcb65b8521276ad35 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Wed, 7 May 2025 18:11:46 +0200 Subject: [PATCH 09/12] Cleanup: remove redundant if statement --- pkg/utils/yaml_utils/yaml_utils.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/pkg/utils/yaml_utils/yaml_utils.go b/pkg/utils/yaml_utils/yaml_utils.go index 391ba8a4f58..5f2532a3d58 100644 --- a/pkg/utils/yaml_utils/yaml_utils.go +++ b/pkg/utils/yaml_utils/yaml_utils.go @@ -73,11 +73,7 @@ func RenameYamlKey(rootNode *yaml.Node, path []string, newKey string) error { 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. From caa8c921e6fb0eafd9dfd3e463db576bf6b817e9 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Wed, 7 May 2025 18:12:25 +0200 Subject: [PATCH 10/12] Make RenameYamlKey return a bool --- pkg/config/app_config.go | 2 +- pkg/utils/yaml_utils/yaml_utils.go | 14 +-- pkg/utils/yaml_utils/yaml_utils_test.go | 108 +++++++++++++----------- 3 files changed, 67 insertions(+), 57 deletions(-) diff --git a/pkg/config/app_config.go b/pkg/config/app_config.go index ae1b0d38ea4..010db02ef75 100644 --- a/pkg/config/app_config.go +++ b/pkg/config/app_config.go @@ -267,7 +267,7 @@ func computeMigratedConfig(path string, content []byte) ([]byte, bool, error) { } for _, pathToReplace := range pathsToReplace { - err := yaml_utils.RenameYamlKey(&rootNode, pathToReplace.oldPath, pathToReplace.newName) + err, _ := yaml_utils.RenameYamlKey(&rootNode, pathToReplace.oldPath, pathToReplace.newName) if err != nil { return nil, false, fmt.Errorf("Couldn't migrate config file at `%s` for key %s: %s", path, strings.Join(pathToReplace.oldPath, "."), err) } diff --git a/pkg/utils/yaml_utils/yaml_utils.go b/pkg/utils/yaml_utils/yaml_utils.go index 5f2532a3d58..432915f5e50 100644 --- a/pkg/utils/yaml_utils/yaml_utils.go +++ b/pkg/utils/yaml_utils/yaml_utils.go @@ -65,10 +65,10 @@ 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] @@ -77,25 +77,25 @@ func RenameYamlKey(rootNode *yaml.Node, path []string, newKey string) error { } // 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 b49130ea74f..9ae09939663 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) }) } } From a4f43cb275e078999820658b9f0d05e186e9829c Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Mon, 5 May 2025 10:58:21 +0200 Subject: [PATCH 11/12] Log a list of migration changes to the console This might be useful to see in general (users will normally only see it after they quit lazygit again, but still). But it is especially useful when writing back the config file fails for some reason, because users can then make these changes manually if they want. We do this only at startup, when the GUI hasn't started yet. This is probably good enough, because it is much less likely that writing back a migrated repo-local config fails because it is not writeable. --- pkg/config/app_config.go | 73 +++++++++++++++++++++++------- pkg/config/app_config_test.go | 84 ++++++++++++++++++++++++++++++++--- 2 files changed, 133 insertions(+), 24 deletions(-) diff --git a/pkg/config/app_config.go b/pkg/config/app_config.go index 010db02ef75..560031cc14f 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" @@ -215,12 +216,20 @@ func loadUserConfig(configFiles []*ConfigFile, base *UserConfig, isGuiInitialize 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, isGuiInitialized bool) ([]byte, error) { - changedContent, didChange, err := computeMigratedConfig(path, content) + changes := NewChangesSet() + + changedContent, didChange, err := computeMigratedConfig(path, content, changes) if err != nil { return nil, err } @@ -230,21 +239,27 @@ func migrateUserConfig(path string, content []byte, isGuiInitialized bool) ([]by 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.Println("Provided user config is deprecated but auto-fixable. Attempting to write fixed version back to file...") + 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 { return nil, fmt.Errorf("While attempting to write back fixed user config to %s, an error occurred: %s", path, err) } if !isGuiInitialized { - fmt.Printf("Success. New config written to %s\n", path) + 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, bool, error) { +func computeMigratedConfig(path string, content []byte, changes *ChangesSet) ([]byte, bool, error) { var err error var rootNode yaml.Node err = yaml.Unmarshal(content, &rootNode) @@ -267,33 +282,36 @@ func computeMigratedConfig(path string, content []byte) ([]byte, bool, 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, 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, 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, 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, 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, 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, false, fmt.Errorf("Couldn't migrate config file at `%s`: %s", path, err) } @@ -311,16 +329,17 @@ func computeMigratedConfig(path string, content []byte) ([]byte, bool, error) { 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 @@ -332,13 +351,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 { @@ -351,6 +372,7 @@ func changeCommitPrefixesMap(rootNode *yaml.Node) error { Kind: yaml.MappingNode, Content: nodeContentCopy, }} + changes.Add("Changed 'git.commitPrefixes' elements to arrays of strings") } } } @@ -358,7 +380,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 @@ -369,16 +391,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 != "" { @@ -402,7 +433,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 @@ -411,6 +442,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 @@ -420,17 +452,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 }) diff --git a/pkg/config/app_config_test.go b/pkg/config/app_config_test.go index 6f15b6d4ae6..90c13ce6cd2 100644 --- a/pkg/config/app_config_test.go +++ b/pkg/config/app_config_test.go @@ -12,11 +12,13 @@ func TestMigrationOfRenamedKeys(t *testing.T) { input string expected string expectedDidChange bool + expectedChanges []string }{ { name: "Empty String", input: "", expectedDidChange: false, + expectedChanges: []string{}, }, { name: "No rename needed", @@ -24,6 +26,7 @@ func TestMigrationOfRenamedKeys(t *testing.T) { bar: 5 `, expectedDidChange: false, + expectedChanges: []string{}, }, { name: "Rename one", @@ -34,6 +37,7 @@ func TestMigrationOfRenamedKeys(t *testing.T) { skipDiscardChangeWarning: true `, expectedDidChange: true, + expectedChanges: []string{"Renamed 'gui.skipUnstageLineWarning' to 'skipDiscardChangeWarning'"}, }, { name: "Rename several", @@ -52,17 +56,24 @@ keybinding: 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) { - actual, didChange, 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.expectedDidChange, didChange) if didChange { assert.Equal(t, s.expected, string(actual)) } + assert.Equal(t, s.expectedChanges, changes.ToSliceFromOldest()) }) } } @@ -73,11 +84,13 @@ func TestMigrateNullKeybindingsToDisabled(t *testing.T) { input string expected string expectedDidChange bool + expectedChanges []string }{ { name: "Empty String", input: "", expectedDidChange: false, + expectedChanges: []string{}, }, { name: "No change needed", @@ -86,6 +99,7 @@ func TestMigrateNullKeybindingsToDisabled(t *testing.T) { quit: q `, expectedDidChange: false, + expectedChanges: []string{}, }, { name: "Change one", @@ -98,6 +112,7 @@ func TestMigrateNullKeybindingsToDisabled(t *testing.T) { quit: `, expectedDidChange: true, + expectedChanges: []string{"Changed 'null' to '' for keybinding 'keybinding.universal.quit'"}, }, { name: "Change several", @@ -114,17 +129,23 @@ func TestMigrateNullKeybindingsToDisabled(t *testing.T) { 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) { - actual, didChange, 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.expectedDidChange, didChange) if didChange { assert.Equal(t, s.expected, string(actual)) } + assert.Equal(t, s.expectedChanges, changes.ToSliceFromOldest()) }) } } @@ -135,11 +156,13 @@ func TestCommitPrefixMigrations(t *testing.T) { input string expected string expectedDidChange bool + expectedChanges []string }{ { name: "Empty String", input: "", expectedDidChange: false, + expectedChanges: []string{}, }, { name: "Single CommitPrefix Rename", @@ -154,6 +177,7 @@ func TestCommitPrefixMigrations(t *testing.T) { replace: '[JIRA $0] ' `, expectedDidChange: true, + expectedChanges: []string{"Changed 'git.commitPrefix' to an array of strings"}, }, { name: "Complicated CommitPrefixes Rename", @@ -176,11 +200,13 @@ func TestCommitPrefixMigrations(t *testing.T) { replace: '[FUN $0] ' `, 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", @@ -194,17 +220,20 @@ git: - pattern: "^\\w+-\\w+.*" replace: '[JIRA $0] '`, expectedDidChange: false, + expectedChanges: []string{}, }, } for _, s := range scenarios { t.Run(s.name, func(t *testing.T) { - actual, didChange, 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.expectedDidChange, didChange) if didChange { assert.Equal(t, s.expected, string(actual)) } + assert.Equal(t, s.expectedChanges, changes.ToSliceFromOldest()) }) } } @@ -215,11 +244,13 @@ func TestCustomCommandsOutputMigration(t *testing.T) { input string expected string expectedDidChange bool + expectedChanges []string }{ { name: "Empty String", input: "", expectedDidChange: false, + expectedChanges: []string{}, }, { name: "Convert subprocess to output=terminal", @@ -232,6 +263,7 @@ func TestCustomCommandsOutputMigration(t *testing.T) { output: terminal `, expectedDidChange: true, + expectedChanges: []string{"Changed 'subprocess: true' to 'output: terminal' in custom command"}, }, { name: "Convert stream to output=log", @@ -244,6 +276,7 @@ func TestCustomCommandsOutputMigration(t *testing.T) { output: log `, expectedDidChange: true, + expectedChanges: []string{"Changed 'stream: true' to 'output: log' in custom command"}, }, { name: "Convert showOutput to output=popup", @@ -256,6 +289,7 @@ func TestCustomCommandsOutputMigration(t *testing.T) { output: popup `, expectedDidChange: true, + expectedChanges: []string{"Changed 'showOutput: true' to 'output: popup' in custom command"}, }, { name: "Subprocess wins over the other two", @@ -270,6 +304,11 @@ func TestCustomCommandsOutputMigration(t *testing.T) { 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", @@ -283,6 +322,10 @@ func TestCustomCommandsOutputMigration(t *testing.T) { 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", @@ -296,17 +339,24 @@ func TestCustomCommandsOutputMigration(t *testing.T) { - 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, didChange, 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.expectedDidChange, didChange) if didChange { assert.Equal(t, s.expected, string(actual)) } + assert.Equal(t, s.expectedChanges, changes.ToSliceFromOldest()) }) } } @@ -915,7 +965,8 @@ keybinding: func BenchmarkMigrationOnLargeConfiguration(b *testing.B) { for b.Loop() { - _, _, _ = computeMigratedConfig("path doesn't matter", largeConfiguration) + changes := NewChangesSet() + _, _, _ = computeMigratedConfig("path doesn't matter", largeConfiguration, changes) } } @@ -925,11 +976,13 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) { input string expected string expectedDidChange bool + expectedChanges []string }{ { name: "Incomplete Configuration Passes uneventfully", input: "git:", expectedDidChange: false, + expectedChanges: []string{}, }, { name: "Single Cmd with no Cmds", @@ -941,6 +994,10 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) { - 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", @@ -955,6 +1012,10 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) { - 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", @@ -962,7 +1023,8 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) { allBranchesLogCmds: - git log `, - expected: "", + expected: "", + expectedChanges: []string{}, }, { name: "Removes Empty Cmd When at end of yaml", @@ -976,6 +1038,7 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) { - git log --graph --oneline `, expectedDidChange: true, + expectedChanges: []string{"Removed obsolete git.allBranchesLogCmd"}, }, { name: "Migrates when sequence defined inline", @@ -987,6 +1050,10 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) { 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", @@ -1002,17 +1069,20 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) { foo: bar `, expectedDidChange: true, + expectedChanges: []string{"Removed obsolete git.allBranchesLogCmd"}, }, } for _, s := range scenarios { t.Run(s.name, func(t *testing.T) { - actual, didChange, 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.expectedDidChange, didChange) if didChange { assert.Equal(t, s.expected, string(actual)) } + assert.Equal(t, s.expectedChanges, changes.ToSliceFromOldest()) }) } } From cabcd54508a162d58a8ee6108064b09849c21a71 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Fri, 9 May 2025 09:31:42 +0200 Subject: [PATCH 12/12] Include migration changes in the error message if we can't log them This is for the unlikely case that a repo-local config file can't be written back after migration; in this case we can't log the migration changes to the console, so include them in the error popup instead. --- pkg/config/app_config.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/config/app_config.go b/pkg/config/app_config.go index 560031cc14f..e3bc4cd40c3 100644 --- a/pkg/config/app_config.go +++ b/pkg/config/app_config.go @@ -250,7 +250,11 @@ func migrateUserConfig(path string, content []byte, isGuiInitialized bool) ([]by fmt.Println(changesText) } 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) + 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)