sha256convert: don't delete a pre-created target dir on failure cleanup · Entire
sha256convert: don't delete a pre-created target dir on failure cleanup
a550f26→main·
nodo·1mo ago·2 files·+136 added/-16 removed
From PR review: ensureEmptyTarget accepts a directory that already exists as long as it's empty, but the failure-cleanup defer called os.RemoveAll(req.TargetDir), removing the directory itself. If a user pre-created the target -- a mountpoint, or a dir with ownership/ACLs they set up -- any error after PlainInit destroyed the whole directory, contradicting the comment's claim that cleanup "only ever removes content this run created."
ensureEmptyTarget now reports whether it created the directory. cleanupConvertedTarget uses that: remove the tree outright when the run created it, but only strip the contents (leaving the directory) when the user pre-created it. Either way the pre-run state is restored exactly.
Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com
Sessions
940c36562004View transcript
Changes
2
cmd/git-sync/internal/sha256convert
Msha256convert.go+50/-16
Msha256convert_test.go+86
246 unmodified lines
247
248
249
250
250
251
252
253
254
8 unmodified lines
263
264
265
265
266
267
268
269
270
271
272
266
267
268
269
270
271
272
273
274
275
276
277
278
276
279
280
281
282
579 unmodified lines
862
863
864
862
863
865
866
867
868
869
870
871
872
873
868
874
875
870
876
877
872
878
879
880
875
881
882
883
884
885
886
887
888
889
890
891
892
893
894
895
896
897
898
899
900
901
902
903
904
905
906
907
908
909
910
911
912
246 unmodified lines
out = os.Stderr
}
if err := ensureEmptyTarget(req.TargetDir); err != nil {
targetCreated, err := ensureEmptyTarget(req.TargetDir)
if err != nil {
return Result{}, err
}
8 unmodified lines
}
}()
// cleanupTarget fires when set, wiping the SHA256 bare repo we
// initialize below. ensureEmptyTarget already verified the dir was
// empty going in, so a defensive RemoveAll on failure only ever
// removes content this run created. Without it, any error after
// PlainInit leaves config/objects/refs/HEAD behind, and the next
// retry hits ensureEmptyTarget's "not empty" refusal with no
// indication of how to recover. Suppressed by --keep-source-objects
// so users can inspect partial state.
// cleanupTarget fires when set, undoing the SHA256 bare repo we
// initialize below. Without it, any error after PlainInit leaves
// config/objects/refs/HEAD behind, and the next retry hits
// ensureEmptyTarget's "not empty" refusal with no indication of how
// to recover. We restore the exact pre-run state: if this run created
// the target directory, remove it entirely; if the user pre-created it
// (empty — a mountpoint, or a dir whose ownership/ACLs they set up),
// remove only the entries we added and leave the directory in place.
// Suppressed by --keep-source-objects so users can inspect partial
// state.
cleanupTarget := false
defer func() {
if cleanupTarget && !req.KeepSourceObjects {
_ = os.RemoveAll(req.TargetDir)
_ = cleanupConvertedTarget(req.TargetDir, targetCreated)
}
}()
579 unmodified lines
}
// ensureEmptyTarget refuses to init into a non-empty directory so the user
// doesn't quietly accumulate objects into an existing repo.
func ensureEmptyTarget(path string) error {
// doesn't quietly accumulate objects into an existing repo. It reports
// created=true when it had to make the directory, so the caller's failure
// cleanup can tell "remove the whole tree we created" apart from "remove
// only the contents we added to a directory the user pre-created".
func ensureEmptyTarget(path string) (created bool, err error) {
entries, err := os.ReadDir(path)
if err != nil {
if os.IsNotExist(err) {
if mkErr := os.MkdirAll(path, 0o755); mkErr != nil {
return fmt.Errorf("create target dir: %w", mkErr)
return false, fmt.Errorf("create target dir: %w", mkErr)
}
return nil
return true, nil
}
return fmt.Errorf("read target dir: %w", err)
return false, fmt.Errorf("read target dir: %w", err)
}
if len(entries) > 0 {
return fmt.Errorf("target directory %s is not empty", path)
return false, fmt.Errorf("target directory %s is not empty", path)
}
return false, nil
}
// cleanupConvertedTarget restores the target directory to the state it had
// before the run, for use on a failure path. When the run created the
// directory (created=true) it is removed outright. When the user
// pre-created it — ensureEmptyTarget accepts an existing empty directory —
// only the entries the run added are removed, leaving the directory and
// any ownership, permissions, ACLs, or mount the user set up intact.
func cleanupConvertedTarget(path string, created bool) error {
if created {
return os.RemoveAll(path)
}
return removeDirContents(path)
}
// removeDirContents removes every entry inside dir but leaves dir itself
// in place.
func removeDirContents(dir string) error {
entries, err := os.ReadDir(dir)
if err != nil {
return err
}
for _, e := range entries {
if err := os.RemoveAll(filepath.Join(dir, e.Name())); err != nil {
return err
}
}
return nil
}
Mcmd/git-sync/internal/sha256convert/sha256convert.go+50/-16
1262 unmodified lines
1263
1264
1265
1266
1267
1268
1269
1270
1271
1272
1273
1274
1275
1276
1277
1278
1279
1280
1281
1282
1283
1284
1285
1286
1287
1288
1289
1290
1291
1292
1293
1294
1295
1296
1297
1298
1299
1300
1301
1302
1303
1304
1305
1306
1307
1308
1309
1310
1311
1312
1313
1314
1315
1316
1317
1318
1319
1320
1321
1322
1323
1324
1325
1326
1327
1328
1329
1330
1331
1332
1333
1334
1335
1336
1337
1338
1339
1340
1341
1342
1343
1344
1345
1346
1347
1348
1349
1350
1351
1352
1353
1262 unmodified lines
}
// TestEnsureEmptyTarget covers the three cases the failure-cleanup logic
// depends on: an absent directory is created (created=true), a
// pre-existing empty directory is accepted without claiming creation
// (created=false), and a non-empty directory is refused.
func TestEnsureEmptyTarget(t *testing.T) {
t.Run("absent dir is created", func(t *testing.T) {
path := filepath.Join(t.TempDir(), "out")
created, err := ensureEmptyTarget(path)
if err != nil {
t.Fatalf("ensureEmptyTarget: %v", err)
}
if !created {
t.Errorf("created = false, want true for an absent directory")
}
if info, statErr := os.Stat(path); statErr != nil || !info.IsDir() {
t.Errorf("expected %s to be created as a directory; stat err=%v", path, statErr)
}
})
t.Run("pre-existing empty dir is accepted, not claimed as created", func(t *testing.T) {
path := t.TempDir() // already exists and is empty
created, err := ensureEmptyTarget(path)
if err != nil {
t.Fatalf("ensureEmptyTarget: %v", err)
}
if created {
t.Errorf("created = true, want false for a pre-existing directory")
}
})
t.Run("non-empty dir is refused", func(t *testing.T) {
path := t.TempDir()
if err := os.WriteFile(filepath.Join(path, "f"), []byte("x"), 0o644); err != nil {
t.Fatalf("seed file: %v", err)
}
if _, err := ensureEmptyTarget(path); err == nil {
t.Errorf("expected refusal of a non-empty directory, got nil")
}
})
}
// TestCleanupConvertedTarget locks in the invariant the failure path
// claims: a directory the run created is removed outright, but a
// directory the user pre-created is restored to empty — never deleted —
// so a mountpoint or a dir with user-set ownership/ACLs survives a failed
// conversion.
func TestCleanupConvertedTarget(t *testing.T) {
t.Run("run-created dir is removed outright", func(t *testing.T) {
path := filepath.Join(t.TempDir(), "out")
if err := os.MkdirAll(filepath.Join(path, "objects"), 0o755); err != nil {
t.Fatalf("seed: %v", err)
}
if err := cleanupConvertedTarget(path, true); err != nil {
t.Fatalf("cleanupConvertedTarget: %v", err)
}
if _, err := os.Stat(path); !os.IsNotExist(err) {
t.Errorf("expected %s to be removed; stat err=%v", path, err)
}
})
t.Run("pre-existing dir is emptied but kept", func(t *testing.T) {
path := t.TempDir()
// Mimic a half-written bare repo: nested dirs plus a top-level file.
if err := os.MkdirAll(filepath.Join(path, "objects", "pack"), 0o755); err != nil {
t.Fatalf("seed dirs: %v", err)
}
if err := os.WriteFile(filepath.Join(path, "HEAD"), []byte("ref: x\n"), 0o644); err != nil {
t.Fatalf("seed file: %v", err)
}
if err := cleanupConvertedTarget(path, false); err != nil {
t.Fatalf("cleanupConvertedTarget: %v", err)
}
info, err := os.Stat(path)
if err != nil || !info.IsDir() {
t.Fatalf("pre-existing directory must survive cleanup; stat err=%v", err)
}
entries, err := os.ReadDir(path)
if err != nil {
t.Fatalf("read dir: %v", err)
}
if len(entries) != 0 {
t.Errorf("directory should be empty after cleanup, got %d entries", len(entries))
}
})
}
// The default pull-ref exclusions must never trip the branch/tag
// protection guard — they live outside refs/heads/ and refs/tags/.
func TestForeignPullRefPrefixes_NotProtected(t *testing.T) {