fix: preserve manually added files when dir contains fileTree and file nodes#483
fix: preserve manually added files when dir contains fileTree and file nodes#483VelmiraS wants to merge 2 commits into
Conversation
…e nodes RemoveNodeFromParent used swap-and-truncate which silently deleted the last sibling of a removed fileTree node. Replace with order-preserving deletion so manually added file nodes are not lost during fileTree expansion. Add regression test covering a dir with both fileTree and a manually added file entry.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
gardener-prow
Bot
added
do-not-merge/needs-kind
RemoveNodeFromParent inside a range loop shifted the underlying array while the range header still held the original length, causing elements to be visited twice and triggering a false file-collision error. Collect nodes to remove during the loop and delete them after.
| size := len(parent.Structure) | ||
| parent.Structure[i] = parent.Structure[size-1] | ||
| parent.Structure = parent.Structure[:size-1] | ||
| parent.Structure = append(parent.Structure[:i], parent.Structure[i+1:]...) |
There was a problem hiding this comment.
This seems to be editing the parent structure we are iterating on. For me, this always ends in a bug.
There was a problem hiding this comment.
You're right that mutating a slice during range is risky. In RemoveNodeFromParent it's safe because append is immediately followed by return — the loop stops right there, so no index shifting or double-visiting can happen.
The real mutation-during-range risk was in mergeFolders — and the second commit addresses exactly that: nodes are collected into toRemove during the loop and only removed after, so we never modify the slice we're iterating over.
| nodeNameToNode[child.File] = child | ||
| } | ||
| } | ||
| for _, child := range toRemove { |
There was a problem hiding this comment.
Phase 1 – We loop through node.Structure with for _, child := range node.Structure. During this loop, we do not change node.Structure. If we find a node that should be removed, we only do toRemove = append(toRemove, child). toRemove is a different slice, so it does not affect node.Structure. The loop continues safely because node.Structure does not change while we are iterating over it.
Phase 2 – After the first loop is completely finished, we start a second loop: for _, child := range toRemove. Now it is safe to modify node.Structure because we are no longer iterating over it.
The old version of the remove function moved the last element into the removed element's position.
2 participants
Summary
RemoveNodeFromParentused a swap-and-truncate pattern to remove nodes, which silently deleted the last sibling of the removedfileTreenodeappend(s[:i], s[i+1:]...)) so manually addedfile:nodes are not lost duringfileTreeexpansionfileTreeand a manually addedfileentryFixes the bug reported where a manually added file appears at the top level of the TOC instead of inside its parent directory's subnav.
Test plan
go test ./pkg/manifest/...— 6/6 passgo test ./...— all passgen-tocwith a manifest containing a dir with bothfileTree:andfile:— manually added file appears under the correct directory in the TOC