fixup! Log a list of migration changes to the console

Author: stefanhaller

pkg/config/app_config.go

@@ -11,10 +11,10 @@ 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"
-	orderedmap "github.com/wk8/go-ordered-map/v2"
 	"gopkg.in/yaml.v3"
 )
 
@@ -216,12 +216,18 @@ 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) {
-	changes := orderedmap.New[string, bool]()
+	changes := NewChangesSet()
 
 	changedContent, didChange, err := computeMigratedConfig(path, content, changes)
 	if err != nil {
@@ -234,9 +240,9 @@ func migrateUserConfig(path string, content []byte, isGuiInitialized bool) ([]by
 	}
 
 	changesText := "The following changes were made:\n\n"
-	for pair := changes.Oldest(); pair != nil; pair = pair.Next() {
-		changesText += fmt.Sprintf("- %s\n", pair.Key)
-	}
+	changesText += strings.Join(lo.Map(changes.ToSliceFromOldest(), func(change string, _ int) string {
+		return fmt.Sprintf("- %s\n", change)
+	}), "")
 
 	// Write config back
 	if !isGuiInitialized {
@@ -257,7 +263,7 @@ func migrateUserConfig(path string, content []byte, isGuiInitialized bool) ([]by
 }
 
 // A pure function helper for testing purposes
-func computeMigratedConfig(path string, content []byte, changes *orderedmap.OrderedMap[string, bool]) ([]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)
@@ -285,7 +291,7 @@ func computeMigratedConfig(path string, content []byte, changes *orderedmap.Orde
 			return nil, false, fmt.Errorf("Couldn't migrate config file at `%s` for key %s: %s", path, strings.Join(pathToReplace.oldPath, "."), err)
 		}
 		if didReplace {
-			changes.Set(fmt.Sprintf("Renamed '%s' to '%s'", strings.Join(pathToReplace.oldPath, "."), pathToReplace.newName), true)
+			changes.Add(fmt.Sprintf("Renamed '%s' to '%s'", strings.Join(pathToReplace.oldPath, "."), pathToReplace.newName))
 		}
 	}
 
@@ -327,17 +333,17 @@ func computeMigratedConfig(path string, content []byte, changes *orderedmap.Orde
 	return newContent, true, nil
 }
 
-func changeNullKeybindingsToDisabled(rootNode *yaml.Node, changes *orderedmap.OrderedMap[string, bool]) 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 = "<disabled>"
 			node.Tag = "!!str"
-			changes.Set(fmt.Sprintf("Changed 'null' to '<disabled>' for keybinding '%s'", path), true)
+			changes.Add(fmt.Sprintf("Changed 'null' to '<disabled>' for keybinding '%s'", path))
 		}
 	})
 }
 
-func changeElementToSequence(rootNode *yaml.Node, path []string, changes *orderedmap.OrderedMap[string, bool]) 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
@@ -349,15 +355,15 @@ func changeElementToSequence(rootNode *yaml.Node, path []string, changes *ordere
 				Content: nodeContentCopy,
 			}}
 
-			changes.Set(fmt.Sprintf("Changed '%s' to an array of strings", strings.Join(path, ".")), true)
+			changes.Add(fmt.Sprintf("Changed '%s' to an array of strings", strings.Join(path, ".")))
 
 			return nil
 		}
 		return nil
 	})
 }
 
-func changeCommitPrefixesMap(rootNode *yaml.Node, changes *orderedmap.OrderedMap[string, bool]) 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 {
@@ -370,15 +376,15 @@ func changeCommitPrefixesMap(rootNode *yaml.Node, changes *orderedmap.OrderedMap
 						Kind:    yaml.MappingNode,
 						Content: nodeContentCopy,
 					}}
-					changes.Set("Changed 'git.commitPrefixes' elements to arrays of strings", true)
+					changes.Add("Changed 'git.commitPrefixes' elements to arrays of strings")
 				}
 			}
 		}
 		return nil
 	})
 }
 
-func changeCustomCommandStreamAndOutputToOutputEnum(rootNode *yaml.Node, changes *orderedmap.OrderedMap[string, bool]) 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
@@ -389,25 +395,25 @@ func changeCustomCommandStreamAndOutputToOutputEnum(rootNode *yaml.Node, changes
 			if streamKey, streamValue := yaml_utils.RemoveKey(node, "subprocess"); streamKey != nil {
 				if streamValue.Kind == yaml.ScalarNode && streamValue.Value == "true" {
 					output = "terminal"
-					changes.Set("Changed 'subprocess: true' to 'output: terminal' in custom command", true)
+					changes.Add("Changed 'subprocess: true' to 'output: terminal' in custom command")
 				} else {
-					changes.Set("Deleted redundant 'subprocess: false' in custom command", true)
+					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.Set("Changed 'stream: true' to 'output: log' in custom command", true)
+					changes.Add("Changed 'stream: true' to 'output: log' in custom command")
 				} else {
-					changes.Set(fmt.Sprintf("Deleted redundant 'stream: %v' property in custom command", streamValue.Value), true)
+					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.Set("Changed 'showOutput: true' to 'output: popup' in custom command", true)
+					changes.Add("Changed 'showOutput: true' to 'output: popup' in custom command")
 					output = "popup"
 				} else {
-					changes.Set(fmt.Sprintf("Deleted redundant 'showOutput: %v' property in custom command", streamValue.Value), true)
+					changes.Add(fmt.Sprintf("Deleted redundant 'showOutput: %v' property in custom command", streamValue.Value))
 				}
 			}
 			if output != "" {
@@ -431,7 +437,7 @@ func changeCustomCommandStreamAndOutputToOutputEnum(rootNode *yaml.Node, changes
 // 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, changes *orderedmap.OrderedMap[string, bool]) 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
@@ -462,12 +468,12 @@ func migrateAllBranchesLogCmd(rootNode *yaml.Node, changes *orderedmap.OrderedMa
 		if cmdValueNode.Value != "" {
 			// Prepending the individual element to make it show up first in the list, which was prior behavior
 			cmdsValueNode.Content = utils.Prepend(cmdsValueNode.Content, &yaml.Node{Kind: yaml.ScalarNode, Value: cmdValueNode.Value})
-			changes.Set(change, true)
+			changes.Add(change)
 		}
 
 		// Clear out the existing allBranchesLogCmd, now that we have migrated it into the list
 		_, _ = yaml_utils.RemoveKey(gitNode, "allBranchesLogCmd")
-		changes.Set("Removed obsolete git.allBranchesLogCmd", true)
+		changes.Add("Removed obsolete git.allBranchesLogCmd")
 
 		return nil
 	})

pkg/config/app_config_test.go

@@ -4,17 +4,8 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
-	orderedmap "github.com/wk8/go-ordered-map/v2"
 )
 
-func orderedMapToSlice(m *orderedmap.OrderedMap[string, bool]) []string {
-	result := make([]string, 0, m.Len())
-	for i := m.Oldest(); i != nil; i = i.Next() {
-		result = append(result, i.Key)
-	}
-	return result
-}
-
 func TestMigrationOfRenamedKeys(t *testing.T) {
 	scenarios := []struct {
 		name              string
@@ -75,14 +66,14 @@ keybinding:
 
 	for _, s := range scenarios {
 		t.Run(s.name, func(t *testing.T) {
-			changes := orderedmap.New[string, bool]()
+			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, orderedMapToSlice(changes))
+			assert.Equal(t, s.expectedChanges, changes.ToSliceFromOldest())
 		})
 	}
 }
@@ -147,14 +138,14 @@ func TestMigrateNullKeybindingsToDisabled(t *testing.T) {
 
 	for _, s := range scenarios {
 		t.Run(s.name, func(t *testing.T) {
-			changes := orderedmap.New[string, bool]()
+			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, orderedMapToSlice(changes))
+			assert.Equal(t, s.expectedChanges, changes.ToSliceFromOldest())
 		})
 	}
 }
@@ -235,14 +226,14 @@ git:
 
 	for _, s := range scenarios {
 		t.Run(s.name, func(t *testing.T) {
-			changes := orderedmap.New[string, bool]()
+			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, orderedMapToSlice(changes))
+			assert.Equal(t, s.expectedChanges, changes.ToSliceFromOldest())
 		})
 	}
 }
@@ -358,14 +349,14 @@ func TestCustomCommandsOutputMigration(t *testing.T) {
 
 	for _, s := range scenarios {
 		t.Run(s.name, func(t *testing.T) {
-			changes := orderedmap.New[string, bool]()
+			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, orderedMapToSlice(changes))
+			assert.Equal(t, s.expectedChanges, changes.ToSliceFromOldest())
 		})
 	}
 }
@@ -974,7 +965,7 @@ keybinding:
 
 func BenchmarkMigrationOnLargeConfiguration(b *testing.B) {
 	for b.Loop() {
-		changes := orderedmap.New[string, bool]()
+		changes := NewChangesSet()
 		_, _, _ = computeMigratedConfig("path doesn't matter", largeConfiguration, changes)
 	}
 }
@@ -1084,14 +1075,14 @@ func TestAllBranchesLogCmdMigrations(t *testing.T) {
 
 	for _, s := range scenarios {
 		t.Run(s.name, func(t *testing.T) {
-			changes := orderedmap.New[string, bool]()
+			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, orderedMapToSlice(changes))
+			assert.Equal(t, s.expectedChanges, changes.ToSliceFromOldest())
 		})
 	}
 }