diff --git a/pkg/lib/pkg/util.go b/pkg/lib/pkg/util.go index 077e51923d..7bcf4cf697 100644 --- a/pkg/lib/pkg/util.go +++ b/pkg/lib/pkg/util.go @@ -171,6 +171,9 @@ func CopyPackage(src, dst string, copyRootKptfile bool, matcher SubpackageMatche // RemoveStaleItems removes files and directories from the dst package that were present in the org package, // but are not present in the src package. It does not remove the root Kptfile of the dst package. +// Directories that are stale but still contain locally-added files are preserved rather than +// causing an error, since this represents a legitimate merge outcome where upstream deleted a +// directory but the local package added files to it. func RemoveStaleItems(org, src, dst string, _ bool, _ SubpackageMatcher) error { var dirsToDelete []string walkErr := filepath.Walk(dst, func(path string, info os.FileInfo, err error) error { @@ -215,11 +218,28 @@ func RemoveStaleItems(org, src, dst string, _ bool, _ SubpackageMatcher) error { if walkErr != nil { return walkErr } + // Sort directories deepest-first so children are processed before parents. sort.Slice(dirsToDelete, SubPkgFirstSorter(dirsToDelete)) for _, dir := range dirsToDelete { - if err := os.Remove(dir); err != nil { + // Check whether the directory is empty before removing it. It may still + // contain files that were added locally (not present in upstream) and therefore + // intentionally preserved by the filepath walk above. Removing a non-empty + // directory with os.Remove would fail; instead we skip it so those + // locally-added files survive the merge. + f, err := os.Open(dir) + if err != nil { return err } + _, readErr := f.Readdirnames(1) + f.Close() + if readErr == io.EOF { + // Directory is empty — safe to remove. + if err := os.Remove(dir); err != nil { + return err + } + } + // Otherwise the directory still has contents (locally-added files); + // leave it in place. } return nil diff --git a/pkg/lib/pkg/util_test.go b/pkg/lib/pkg/util_test.go index 9613b37225..12da37c28f 100644 --- a/pkg/lib/pkg/util_test.go +++ b/pkg/lib/pkg/util_test.go @@ -566,7 +566,7 @@ func TestRemoveStaleItems_RemovesFile(t *testing.T) { assert.True(t, os.IsNotExist(err)) } -func TestRemoveStaleItems_ErrorOnRemove(t *testing.T) { +func TestRemoveStaleItems_PreservesNonEmptyLocalDir(t *testing.T) { org := t.TempDir() src := t.TempDir() dst := t.TempDir() @@ -578,12 +578,136 @@ func TestRemoveStaleItems_ErrorOnRemove(t *testing.T) { assert.NoError(t, os.WriteFile(filePathOrg, []byte("content"), 0644)) assert.NoError(t, os.WriteFile(filePathDst, []byte("content"), 0644)) - // Replace file in dst with a non-empty directory to force os.Remove error + // Replace file in dst with a non-empty directory (simulates upstream deleting + // a path that locally became a directory with added files). assert.NoError(t, os.Remove(filePathDst)) assert.NoError(t, os.Mkdir(filePathDst, 0755)) assert.NoError(t, os.WriteFile(filepath.Join(filePathDst, "dummy"), []byte("x"), 0644)) + // RemoveStaleItems should succeed and preserve the non-empty directory. + err := RemoveStaleItems(org, src, dst, true, All) + assert.NoError(t, err) + + // The directory and its locally-added file must still exist. + _, err = os.Stat(filePathDst) + assert.NoError(t, err, "non-empty stale directory should be preserved") + _, err = os.Stat(filepath.Join(filePathDst, "dummy")) + assert.NoError(t, err, "locally-added file inside stale directory should be preserved") +} + +func TestRemoveStaleItems_PreservesLocalFilesInDeletedDir(t *testing.T) { + org := t.TempDir() + src := t.TempDir() + dst := t.TempDir() + + // Simulate: origin has configs/base.yaml, upstream deletes entire configs/ dir, + // but local added configs/custom.yaml. + configsDirOrg := filepath.Join(org, "configs") + configsDirDst := filepath.Join(dst, "configs") + + assert.NoError(t, os.Mkdir(configsDirOrg, 0755)) + assert.NoError(t, os.WriteFile(filepath.Join(configsDirOrg, "base.yaml"), []byte("original"), 0644)) + + assert.NoError(t, os.Mkdir(configsDirDst, 0755)) + assert.NoError(t, os.WriteFile(filepath.Join(configsDirDst, "base.yaml"), []byte("original"), 0644)) + assert.NoError(t, os.WriteFile(filepath.Join(configsDirDst, "custom.yaml"), []byte("local-addition"), 0644)) + + // src (new upstream) has no configs/ directory at all. + + err := RemoveStaleItems(org, src, dst, true, All) + assert.NoError(t, err) + + // base.yaml was in origin and not in upstream — should be removed. + _, err = os.Stat(filepath.Join(configsDirDst, "base.yaml")) + assert.True(t, os.IsNotExist(err), "stale file base.yaml should be removed") + + // custom.yaml was NOT in origin — should be preserved. + _, err = os.Stat(filepath.Join(configsDirDst, "custom.yaml")) + assert.NoError(t, err, "locally-added file custom.yaml should be preserved") + + // configs/ directory should still exist because it contains custom.yaml. + info, err := os.Stat(configsDirDst) + assert.NoError(t, err, "directory with local files should be preserved") + assert.True(t, info.IsDir()) +} + +func TestRemoveStaleItems_RemovesEmptyDirAfterStaleFileCleanup(t *testing.T) { + org := t.TempDir() + src := t.TempDir() + dst := t.TempDir() + + // Simulate: origin has configs/base.yaml, upstream deletes the directory, + // and local has no additions — directory should be removed entirely. + configsDirOrg := filepath.Join(org, "configs") + configsDirDst := filepath.Join(dst, "configs") + + assert.NoError(t, os.Mkdir(configsDirOrg, 0755)) + assert.NoError(t, os.WriteFile(filepath.Join(configsDirOrg, "base.yaml"), []byte("original"), 0644)) + + assert.NoError(t, os.Mkdir(configsDirDst, 0755)) + assert.NoError(t, os.WriteFile(filepath.Join(configsDirDst, "base.yaml"), []byte("original"), 0644)) + + err := RemoveStaleItems(org, src, dst, true, All) + assert.NoError(t, err) + + // Both the file and directory should be gone. + _, err = os.Stat(filepath.Join(configsDirDst, "base.yaml")) + assert.True(t, os.IsNotExist(err), "stale file should be removed") + _, err = os.Stat(configsDirDst) + assert.True(t, os.IsNotExist(err), "empty stale directory should be removed") +} + +func TestRemoveStaleItems_NestedDirsWithLocalFile(t *testing.T) { + org := t.TempDir() + src := t.TempDir() + dst := t.TempDir() + + // Origin has configs/nested/base.yaml and configs/top.yaml. + // Upstream (src) deletes everything. + // Local added configs/nested/custom.yaml. + assert.NoError(t, os.MkdirAll(filepath.Join(org, "configs", "nested"), 0755)) + assert.NoError(t, os.WriteFile(filepath.Join(org, "configs", "top.yaml"), []byte("orig"), 0644)) + assert.NoError(t, os.WriteFile(filepath.Join(org, "configs", "nested", "base.yaml"), []byte("orig"), 0644)) + + assert.NoError(t, os.MkdirAll(filepath.Join(dst, "configs", "nested"), 0755)) + assert.NoError(t, os.WriteFile(filepath.Join(dst, "configs", "top.yaml"), []byte("orig"), 0644)) + assert.NoError(t, os.WriteFile(filepath.Join(dst, "configs", "nested", "base.yaml"), []byte("orig"), 0644)) + assert.NoError(t, os.WriteFile(filepath.Join(dst, "configs", "nested", "custom.yaml"), []byte("local"), 0644)) + + err := RemoveStaleItems(org, src, dst, true, All) + assert.NoError(t, err) + + // Stale files should be removed. + _, err = os.Stat(filepath.Join(dst, "configs", "top.yaml")) + assert.True(t, os.IsNotExist(err), "stale file top.yaml should be removed") + _, err = os.Stat(filepath.Join(dst, "configs", "nested", "base.yaml")) + assert.True(t, os.IsNotExist(err), "stale file base.yaml should be removed") + + // Locally-added file should be preserved. + _, err = os.Stat(filepath.Join(dst, "configs", "nested", "custom.yaml")) + assert.NoError(t, err, "locally-added file custom.yaml should be preserved") + + // Both parent directories should be preserved because nested/ still has content. + _, err = os.Stat(filepath.Join(dst, "configs", "nested")) + assert.NoError(t, err, "nested dir with local files should be preserved") + _, err = os.Stat(filepath.Join(dst, "configs")) + assert.NoError(t, err, "parent dir should be preserved when child dir has content") +} + +func TestRemoveStaleItems_ErrorOnFileRemovePermission(t *testing.T) { + org := t.TempDir() + src := t.TempDir() + dst := t.TempDir() + + // Create a stale file (in org and dst, not in src). + assert.NoError(t, os.WriteFile(filepath.Join(org, "stale.yaml"), []byte("orig"), 0644)) + assert.NoError(t, os.WriteFile(filepath.Join(dst, "stale.yaml"), []byte("orig"), 0644)) + + // Revoke write permission on dst so os.Remove fails. + assert.NoError(t, os.Chmod(dst, 0555)) + t.Cleanup(func() { _ = os.Chmod(dst, 0755) }) + err := RemoveStaleItems(org, src, dst, true, All) assert.Error(t, err) - assert.Contains(t, err.Error(), "directory not empty") + assert.Contains(t, err.Error(), "permission denied") } diff --git a/pkg/lib/update/copy-merge_test.go b/pkg/lib/update/copy-merge_test.go index 63eeca0060..0c176dc268 100644 --- a/pkg/lib/update/copy-merge_test.go +++ b/pkg/lib/update/copy-merge_test.go @@ -411,7 +411,7 @@ func TestCopyMergeDifferentMetadata(t *testing.T) { } } -func TestCopyMergeErrorRemovingFile(t *testing.T) { +func TestCopyMergePreservesLocalFilesInDeletedDir(t *testing.T) { src := t.TempDir() dst := t.TempDir() org := t.TempDir() @@ -424,6 +424,8 @@ func TestCopyMergeErrorRemovingFile(t *testing.T) { assert.NoError(t, os.WriteFile(filePathDst, []byte("content"), 0644)) assert.NoError(t, os.WriteFile(filePathOrg, []byte("content"), 0644)) + // Replace file in dst with a non-empty directory (simulates upstream deleting + // a path that locally became a directory with added files). assert.NoError(t, os.Remove(filePathDst)) assert.NoError(t, os.Mkdir(filePathDst, 0755)) assert.NoError(t, os.WriteFile(filepath.Join(filePathDst, "dummy"), []byte("x"), 0644)) @@ -436,7 +438,13 @@ func TestCopyMergeErrorRemovingFile(t *testing.T) { IsRoot: true, } + // Should succeed — the non-empty directory is preserved with its locally-added file. err := updater.Update(options) - assert.Error(t, err) - assert.Contains(t, err.Error(), "directory not empty") + assert.NoError(t, err) + + // Verify the directory and its contents survive. + _, err = os.Stat(filePathDst) + assert.NoError(t, err, "non-empty directory should be preserved") + _, err = os.Stat(filepath.Join(filePathDst, "dummy")) + assert.NoError(t, err, "locally-added file should be preserved") }