Skip to content

Commit 673193c

Browse files
arwtyxouymzclaude
andcommitted
fix: address CodeRabbit review feedback for CoW copy
- Clean up partial clone output before fallback in Darwin _fast_copy_dir - Cache detect_os result to avoid repeated subshell calls - Use teardown function in copy_safety tests for reliable tmpdir cleanup Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent e806b64 commit 673193c

File tree

2 files changed

+24
-14
lines changed

2 files changed

+24
-14
lines changed

lib/copy.sh

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,17 +79,25 @@ merge_copy_patterns() {
7979
# macOS APFS: cp -cRP (clone); Linux Btrfs/XFS: cp --reflink=auto -RP
8080
# Callers must guard the return value with `if` or `|| true` (set -e safe).
8181
# Usage: _fast_copy_dir src dest
82+
# Cached OS value for _fast_copy_dir; set on first call.
83+
_fast_copy_os=""
84+
8285
_fast_copy_dir() {
8386
local src="$1" dest="$2"
84-
local os
85-
os=$(detect_os)
87+
if [ -z "$_fast_copy_os" ]; then
88+
_fast_copy_os=$(detect_os)
89+
fi
90+
local os="$_fast_copy_os"
8691

8792
case "$os" in
8893
darwin)
8994
# Try CoW clone first; if unsupported, fall back to regular copy
9095
if cp -cRP "$src" "$dest" 2>/dev/null; then
9196
return 0
9297
fi
98+
# Clean up any partial clone output before fallback
99+
local _clone_target="${dest%/}/$(basename "$src")"
100+
[ -e "$_clone_target" ] && rm -rf "$_clone_target"
93101
cp -RP "$src" "$dest"
94102
;;
95103
linux)

tests/copy_safety.bats

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,12 @@ setup() {
66
source "$PROJECT_ROOT/lib/copy.sh"
77
}
88

9+
teardown() {
10+
if [ -n "${_test_tmpdir:-}" ]; then
11+
rm -rf "$_test_tmpdir"
12+
fi
13+
}
14+
915
# --- _is_unsafe_path tests ---
1016

1117
@test "absolute path is unsafe" {
@@ -87,23 +93,22 @@ setup() {
8793
# --- _fast_copy_dir tests ---
8894

8995
@test "_fast_copy_dir copies directory contents" {
90-
local src dst
91-
src=$(mktemp -d)
92-
dst=$(mktemp -d)
96+
_test_tmpdir=$(mktemp -d)
97+
local src="$_test_tmpdir/src" dst="$_test_tmpdir/dst"
98+
mkdir -p "$src" "$dst"
9399
mkdir -p "$src/mydir/sub"
94100
echo "hello" > "$src/mydir/sub/file.txt"
95101

96102
_fast_copy_dir "$src/mydir" "$dst/"
97103

98104
[ -f "$dst/mydir/sub/file.txt" ]
99105
[ "$(cat "$dst/mydir/sub/file.txt")" = "hello" ]
100-
rm -rf "$src" "$dst"
101106
}
102107

103108
@test "_fast_copy_dir preserves symlinks" {
104-
local src dst
105-
src=$(mktemp -d)
106-
dst=$(mktemp -d)
109+
_test_tmpdir=$(mktemp -d)
110+
local src="$_test_tmpdir/src" dst="$_test_tmpdir/dst"
111+
mkdir -p "$src" "$dst"
107112
mkdir -p "$src/mydir"
108113
echo "target" > "$src/mydir/real.txt"
109114
ln -s real.txt "$src/mydir/link.txt"
@@ -112,12 +117,9 @@ setup() {
112117

113118
[ -L "$dst/mydir/link.txt" ]
114119
[ "$(readlink "$dst/mydir/link.txt")" = "real.txt" ]
115-
rm -rf "$src" "$dst"
116120
}
117121

118122
@test "_fast_copy_dir fails on nonexistent source" {
119-
local dst
120-
dst=$(mktemp -d)
121-
! _fast_copy_dir "/nonexistent/path" "$dst/"
122-
rm -rf "$dst"
123+
_test_tmpdir=$(mktemp -d)
124+
! _fast_copy_dir "/nonexistent/path" "$_test_tmpdir/"
123125
}

0 commit comments

Comments
 (0)