From 8dece735d178a1a9df509de08421227a60d622e0 Mon Sep 17 00:00:00 2001
From: Julien Calixte <julien.calixte@theodo.com>
Date: Tue, 7 Jul 2026 22:48:17 +0200
Subject: [PATCH] fix(firmware): delete repo dir via path-based recursion, not
 remove_dir_all

std::fs::remove_dir_all deletes race-free via the openat/unlinkat/fdopendir
family; esp-idf's path-based FATFS VFS doesn't implement those, so it fails
with EACCES on the first entry. That looked like a read-only-attribute
problem but wasn't. A plain read_dir -> remove_file/remove_dir walk
(remove_tree) uses only what FATFS supports.

Hardware-verified: a diagnostic recursive delete removed the whole
/spiflash/repo (every entry reported writable, ro=false) then re-cloned +
pushed, twice. Wire remove_tree into both the RECLONE_EACH_BOOT reset and
the stale-clone recovery.

The p_open/p_creat shim (39e1155) stays: it keeps libgit2's objects writable
(ro=false), so neither this delete nor libgit2's own gc/repack/fetch-prune
is ever blocked by a read-only file on FAT.
---
 firmware/src/bin/git_sync.rs | 51 +++++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 18 deletions(-)

diff --git a/firmware/src/bin/git_sync.rs b/firmware/src/bin/git_sync.rs
index 5635fda..9abd850 100644
--- a/firmware/src/bin/git_sync.rs
+++ b/firmware/src/bin/git_sync.rs
@@ -246,8 +246,7 @@ fn publish() -> Result<String> {
 fn open_or_clone() -> Result<(Repository, bool)> {
     if RECLONE_EACH_BOOT && Path::new(REPO_DIR).exists() {
         log::warn!("RECLONE_EACH_BOOT — removing {REPO_DIR} to re-clone from scratch");
-        fs::remove_dir_all(REPO_DIR)
-            .context("RECLONE_EACH_BOOT wipe — deletes libgit2 objects, needs the RO-delete fix")?;
+        remove_tree(Path::new(REPO_DIR)).context("RECLONE_EACH_BOOT wipe")?;
     }
     match Repository::open(REPO_DIR) {
         Ok(repo) => {
@@ -256,24 +255,12 @@ fn open_or_clone() -> Result<(Repository, bool)> {
         }
         Err(_) => {
             // A leftover at REPO_DIR is a partial clone (libgit2 refuses to clone
-            // into a non-empty dir, code=Exists). Remove it and re-clone. NOTE:
-            // this only succeeds if the leftover is writable — libgit2 marks
-            // objects/packs read-only, FATFS won't f_unlink a read-only file
-            // (EACCES), and POSIX chmod does NOT clear the attribute on esp-idf's
-            // FATFS VFS (verified on hardware). So a clone that got far enough to
-            // write objects can't be cleared from here yet; the proper fix is an
-            // AM_RDO-clearing unlink shim in esp_stubs.c (increment B, needed for
-            // fetch/repack too). Until then the fallback is a one-time wipe:
-            // `espflash erase-region 0x310000 0x400000`.
+            // into a non-empty dir, code=Exists). Remove it and re-clone via
+            // remove_tree (path-based) — std::fs::remove_dir_all EACCESes on
+            // esp-idf's FATFS VFS (see remove_tree). Hardware-verified.
             if Path::new(REPO_DIR).exists() {
                 log::warn!("{REPO_DIR} exists but is not a valid repo — removing stale clone");
-                fs::remove_dir_all(REPO_DIR).with_context(|| {
-                    format!(
-                        "removing stale {REPO_DIR} — if this is EACCES (read-only files on \
-                         FATFS), erase the storage partition once: \
-                         `espflash erase-region 0x310000 0x400000`"
-                    )
-                })?;
+                remove_tree(Path::new(REPO_DIR)).context("removing stale clone")?;
             }
             log::info!("cloning {REMOTE_URL} → {REPO_DIR}");
             let mut fo = FetchOptions::new();
@@ -365,6 +352,34 @@ fn fetch_and_integrate(repo: &Repository, branch: &str) -> Result<()> {
     bail!("origin/{branch} diverged from local — a real merge commit is needed (increment B, deferred)")
 }
 
+/// Recursively remove a directory tree by PATH — deliberately NOT
+/// `std::fs::remove_dir_all`. std's version deletes race-free via the
+/// openat/unlinkat/fdopendir family, which esp-idf's path-based FATFS VFS does
+/// not implement → it fails with EACCES on the first entry. A plain
+/// read_dir → remove_file/remove_dir walk uses only what FATFS supports.
+/// (Hardware-diagnosed: std::remove_dir_all EACCES'd where this succeeds, and
+/// every entry reported writable — so read-only was never the blocker; the
+/// esp_stubs p_open/p_creat shim keeps objects writable so this — and libgit2's
+/// own gc/repack/fetch-prune deletes — never hit a read-only file on FAT.)
+/// Reads each directory fully before recursing so no readdir handle is open
+/// during removal.
+fn remove_tree(path: &Path) -> Result<()> {
+    let meta = fs::symlink_metadata(path).with_context(|| format!("stat {}", path.display()))?;
+    if meta.is_dir() {
+        let children: Vec<_> = fs::read_dir(path)
+            .with_context(|| format!("read_dir {}", path.display()))?
+            .filter_map(|e| e.ok())
+            .map(|e| e.path())
+            .collect();
+        for child in children {
+            remove_tree(&child)?;
+        }
+        fs::remove_dir(path).with_context(|| format!("rmdir {}", path.display()))
+    } else {
+        fs::remove_file(path).with_context(|| format!("unlink {}", path.display()))
+    }
+}
+
 /// Auth + cert callbacks shared by clone, fetch, and push. Captures only the
 /// baked consts, so a fresh set can be built per operation (RemoteCallbacks is
 /// consumed by each). The PAT is handed to libgit2 here and never logged.