From d53a9ea854ae4529ef54e49e6ff6ea6926dd5ff6 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Mon, 29 Jun 2026 17:43:30 +0200 Subject: [PATCH] Add MoveYamlKey helper to move config keys between sections RenameYamlKey can only rename a key in place, under the same parent. To migrate a keybinding from one section to another we need to relocate the key to a different parent mapping, which is a move, not a rename. MoveYamlKey creates intermediate maps at the destination as needed and prunes any maps left empty behind the key, so a section that held only the moved key doesn't linger as an empty mapping in the user's config. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/utils/yaml_utils/yaml_utils.go | 92 +++++++++++++++++++++++++ pkg/utils/yaml_utils/yaml_utils_test.go | 82 ++++++++++++++++++++++ 2 files changed, 174 insertions(+) diff --git a/pkg/utils/yaml_utils/yaml_utils.go b/pkg/utils/yaml_utils/yaml_utils.go index f8f7f0679..251d4dc01 100644 --- a/pkg/utils/yaml_utils/yaml_utils.go +++ b/pkg/utils/yaml_utils/yaml_utils.go @@ -101,6 +101,98 @@ func renameYamlKey(node *yaml.Node, path []string, newKey string) (error, bool) return renameYamlKey(valueNode, path[1:], newKey) } +// Takes the root node of a yaml document, the path to an existing key, and the +// path at which it should live instead. If the key exists, it (and its value) +// is moved to the new path, creating intermediate mapping nodes as needed, and +// any mapping nodes left empty behind it are removed. Does nothing if the key +// at oldPath doesn't exist. Returns an error if a key already exists at newPath, +// or if a node along either path exists but isn't a mapping. +func MoveYamlKey(rootNode *yaml.Node, oldPath []string, newPath []string) (error, bool) { + // Empty document: nothing to do. + if len(rootNode.Content) == 0 { + return nil, false + } + + body := rootNode.Content[0] + + // Bail out early if there's nothing to move. + oldParent, err := findContainingMap(body, oldPath, false) + if err != nil { + return err, false + } + if oldParent == nil { + return nil, false + } + keyNode, valueNode := LookupKey(oldParent, oldPath[len(oldPath)-1]) + if keyNode == nil { + return nil, false + } + + // Find or create the destination map, and make sure it's free. + newParent, err := findContainingMap(body, newPath, true) + if err != nil { + return err, false + } + newKey := newPath[len(newPath)-1] + if existing, _ := LookupKey(newParent, newKey); existing != nil { + return fmt.Errorf("new key `%s' already exists", newKey), false + } + + // Move the key, then prune any maps that became empty behind it. The + // destination is populated first so that a map shared by both paths isn't + // mistaken for empty during pruning. + RemoveKey(oldParent, oldPath[len(oldPath)-1]) + keyNode.Value = newKey + newParent.Content = append(newParent.Content, keyNode, valueNode) + removeEmptyMaps(body, oldPath[:len(oldPath)-1]) + + return nil, true +} + +// Descends path (excluding its final element) and returns the mapping node that +// should directly contain that final element. With create set, missing +// intermediate maps are created; otherwise a missing intermediate yields a nil +// result. Returns an error if a node along the path exists but isn't a mapping. +func findContainingMap(node *yaml.Node, path []string, create bool) (*yaml.Node, error) { + for _, key := range path[:len(path)-1] { + if node.Kind != yaml.MappingNode { + return nil, errors.New("yaml node in path is not a dictionary") + } + _, valueNode := LookupKey(node, key) + if valueNode == nil { + if !create { + return nil, nil + } + valueNode = &yaml.Node{Kind: yaml.MappingNode} + node.Content = append(node.Content, + &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: key}, + valueNode) + } + node = valueNode + } + if node.Kind != yaml.MappingNode { + return nil, errors.New("yaml node in path is not a dictionary") + } + return node, nil +} + +// Walks path from node and removes any mapping that is empty once its child has +// been removed, cascading upward. Stops at the first non-empty ancestor (which +// keeps every ancestor above it non-empty too). +func removeEmptyMaps(node *yaml.Node, path []string) { + if len(path) == 0 { + return + } + _, child := LookupKey(node, path[0]) + if child == nil { + return + } + removeEmptyMaps(child, path[1:]) + if child.Kind == yaml.MappingNode && len(child.Content) == 0 { + RemoveKey(node, path[0]) + } +} + // Traverses a yaml document, calling the callback function for each node. The // callback is expected to modify the node in place func Walk(rootNode *yaml.Node, callback func(node *yaml.Node, path string)) error { diff --git a/pkg/utils/yaml_utils/yaml_utils_test.go b/pkg/utils/yaml_utils/yaml_utils_test.go index d4d1fe074..059f65022 100644 --- a/pkg/utils/yaml_utils/yaml_utils_test.go +++ b/pkg/utils/yaml_utils/yaml_utils_test.go @@ -103,6 +103,88 @@ func TestRenameYamlKey(t *testing.T) { } } +func TestMoveYamlKey(t *testing.T) { + tests := []struct { + name string + in string + oldPath []string + newPath []string + expectedOut string + expectedDidMove bool + expectedErr string + }{ + { + name: "move key into an existing section", + in: "keybinding:\n worktrees:\n viewWorktreeOptions: w\n universal:\n quit: q\n", + oldPath: []string{"keybinding", "worktrees", "viewWorktreeOptions"}, + newPath: []string{"keybinding", "universal", "newWorktree"}, + expectedOut: "keybinding:\n universal:\n quit: q\n newWorktree: w\n", + expectedDidMove: true, + }, + { + name: "create the destination section if it doesn't exist", + in: "keybinding:\n worktrees:\n viewWorktreeOptions: w\n", + oldPath: []string{"keybinding", "worktrees", "viewWorktreeOptions"}, + newPath: []string{"keybinding", "universal", "newWorktree"}, + expectedOut: "keybinding:\n universal:\n newWorktree: w\n", + expectedDidMove: true, + }, + { + name: "keep non-empty siblings when pruning the old section", + in: "keybinding:\n worktrees:\n viewWorktreeOptions: w\n other: x\n", + oldPath: []string{"keybinding", "worktrees", "viewWorktreeOptions"}, + newPath: []string{"keybinding", "universal", "newWorktree"}, + expectedOut: "keybinding:\n worktrees:\n other: x\n universal:\n newWorktree: w\n", + expectedDidMove: true, + }, + { + name: "don't rewrite file if the key doesn't exist", + in: "keybinding:\n universal:\n quit: q\n", + oldPath: []string{"keybinding", "worktrees", "viewWorktreeOptions"}, + newPath: []string{"keybinding", "universal", "newWorktree"}, + expectedOut: "keybinding:\n universal:\n quit: q\n", + expectedDidMove: false, + }, + + // Error cases + { + name: "destination key already exists", + in: "keybinding:\n worktrees:\n viewWorktreeOptions: w\n universal:\n newWorktree: x\n", + oldPath: []string{"keybinding", "worktrees", "viewWorktreeOptions"}, + newPath: []string{"keybinding", "universal", "newWorktree"}, + expectedOut: "keybinding:\n worktrees:\n viewWorktreeOptions: w\n universal:\n newWorktree: x\n", + expectedDidMove: false, + expectedErr: "new key `newWorktree' already exists", + }, + { + name: "node in path is not a dictionary", + in: "keybinding:\n worktrees: nonsense\n", + oldPath: []string{"keybinding", "worktrees", "viewWorktreeOptions"}, + newPath: []string{"keybinding", "universal", "newWorktree"}, + expectedOut: "keybinding:\n worktrees: nonsense\n", + expectedDidMove: false, + expectedErr: "yaml node in path is not a dictionary", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + node := unmarshalForTest(t, test.in) + actualErr, didMove := MoveYamlKey(&node, test.oldPath, test.newPath) + if test.expectedErr == "" { + assert.NoError(t, actualErr) + } else { + assert.EqualError(t, actualErr, test.expectedErr) + } + out := marshalForTest(t, &node) + + assert.Equal(t, test.expectedOut, out) + + assert.Equal(t, test.expectedDidMove, didMove) + }) + } +} + func TestWalk_paths(t *testing.T) { tests := []struct { name string