Fix copy-merge to preserve locally-added files in upstream-deleted directories

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 org) and therefore
+		// intentionally preserved by the 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

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")
 }

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")
 }