fix: make the link store hold what it says it holds

src/commands/link.rs

@@ -7,14 +7,18 @@ use fs_err::{read_dir, read_link};
 use std::cell::Cell;
 use std::path::{Path, PathBuf};
 
-use anyhow::{Result, anyhow, bail};
+use anyhow::{Context, Result, anyhow, bail};
 
-use self::store::{Repo, ignored, resolve, symlink_metadata_opt, tracked};
+use self::store::{Leads, Repo, ignored, leads, resolve, symlink_metadata_opt, tracked};
 use crate::ctx::Ctx;
-use crate::fsops::{move_path, place_link, prune_empty, remove_file, rename, suffixed};
+use crate::fsops::{
+    ensure_private_parent, move_path, place_link, prune_empty, remove_file, rename, suffixed,
+};
 use crate::output::{line, note, warning};
 
 const BACKUP_SUFFIX: &str = ".ahab-bak";
+// where the link waits while the payload comes back out of the store
+const RESTORING_SUFFIX: &str = ".ahab-restoring";
 
 // move untracked paths out of the repo and symlink them back
 pub fn add(ctx: &Ctx, paths: &[PathBuf], force: bool, store: Option<&Path>) -> Result<()> {
@@ -126,8 +130,27 @@ fn restore_one(ctx: &Ctx, repo: &Repo, path: &Path, report: &Report) -> Result<(
         bail!("{} is missing from the store", rel.display());
     }
 
-    remove_file(ctx, &src)?;
-    move_path(ctx, &stored, &src)?;
+    // the link is moved aside rather than removed: if the payload cannot come
+    // back out of the store, the repository is left pointing at where it still is
+    let aside = suffixed(&src, RESTORING_SUFFIX);
+    if symlink_metadata_opt(&aside)?.is_some() {
+        bail!("{} is in the way; move it aside", aside.display());
+    }
+
+    rename(ctx, &src, &aside)?;
+
+    if let Err(e) = move_path(ctx, &stored, &src) {
+        rename(ctx, &aside, &src).with_context(|| {
+            format!(
+                "could not put the link at {} back after failing to restore it",
+                src.display()
+            )
+        })?;
+
+        return Err(e);
+    }
+
+    remove_file(ctx, &aside)?;
     prune_empty(ctx, stored.parent(), &repo.base);
 
     report.line("restored", &rel);
@@ -158,9 +181,13 @@ fn stored_paths(repo: &Repo, dir: &Path) -> Result<Vec<(PathBuf, Stored)>> {
 
     for entry in entries {
         let stored = entry?.path();
-        let rel = stored
-            .strip_prefix(&repo.store)
-            .expect("walked out of the store");
+        let rel = stored.strip_prefix(&repo.store).with_context(|| {
+            format!(
+                "{} is not under the store {}",
+                stored.display(),
+                repo.store.display()
+            )
+        })?;
         let src = repo.root.join(rel);
 
         let state = match symlink_metadata_opt(&src)? {
@@ -170,8 +197,12 @@ fn stored_paths(repo: &Repo, dir: &Path) -> Result<Vec<(PathBuf, Stored)>> {
             Some(_) => Stored::Taken,
         };
 
+        // a symlink the store happens to hold is a leaf, never a directory to
+        // walk into: is_dir would follow it and list whatever it points at
+        let holds_dir = symlink_metadata_opt(&stored)?.is_some_and(|meta| meta.is_dir());
+
         // a linked directory is one entry; otherwise the paths inside it are
-        if state == Stored::Linked || !stored.is_dir() {
+        if state == Stored::Linked || !holds_dir {
             found.push((src, state));
         } else {
             found.extend(stored_paths(repo, &stored)?);
@@ -249,22 +280,37 @@ fn link_one(ctx: &Ctx, repo: &Repo, path: &Path, force: bool, report: &Report) -
     let rel = repo.relative(&src)?;
     let target = repo.store.join(&rel);
 
-    // a target inside the repo would be readable from the sandbox anyway
-    if target.starts_with(&repo.root) {
+    // a target inside the repo would be readable from the sandbox anyway. the
+    // base is resolved as well as compared: one symlinked into the checkout
+    // passes a prefix test while landing the file straight back inside it
+    if target.starts_with(&repo.root) || matches!(leads(&repo.base, &repo.root), Leads::Inside) {
         return Err(anyhow!(
-            "target {} is inside the repository; point AHAB_LINK_ROOT elsewhere",
-            target.display()
+            "the store at {} is inside the repository {}; point --store or \
+             AHAB_LINK_ROOT somewhere else",
+            repo.base.display(),
+            repo.root.display()
         ));
     }
 
+    stays_in_store(repo, &rel)?;
+    // before anything is moved in, so the tree it lands in is never briefly
+    // readable by anyone else
+    ensure_private_parent(ctx, &repo.base, &target)?;
+
     if tracked(ctx, repo, &rel)? {
         return Err(anyhow!(
             "{} is tracked by git; only untracked or ignored paths can be externalized",
             rel.display()
         ));
     }
-    if !ignored(ctx, repo, &rel) {
-        warning!("{} is not gitignored", rel.display());
+    match ignored(ctx, repo, &rel) {
+        Ok(true) => {}
+        Ok(false) => warning!("{} is not gitignored", rel.display()),
+        // saying "not gitignored" here would be an answer git never gave
+        Err(e) => warning!(
+            "could not tell whether {} is gitignored: {e:#}",
+            rel.display()
+        ),
     }
 
     let src_meta = symlink_metadata_opt(&src)?;
@@ -288,6 +334,19 @@ fn link_one(ctx: &Ctx, repo: &Repo, path: &Path, force: bool, report: &Report) -
 
             // nothing in the store to adopt, so the symlink itself moves out
             if !target_taken {
+                // moving the link moves the pointer and leaves the contents
+                // where they are, so the store would hold a way back out and
+                // check, seeing a link into the store, would call it clean
+                if let Leads::Outside(end) = leads(&src, &repo.root) {
+                    return Err(anyhow!(
+                        "{} is a symlink to {}, outside the repository; \
+                         externalizing it would move the link and leave its \
+                         contents there, so repoint or remove it instead",
+                        rel.display(),
+                        end.display()
+                    ));
+                }
+
                 if !src.exists() {
                     warning!(
                         "{} is a broken symlink to {}",
@@ -295,8 +354,7 @@ fn link_one(ctx: &Ctx, repo: &Repo, path: &Path, force: bool, report: &Report) -
                         dest.display()
                     );
                 }
-                move_path(ctx, &src, &target)?;
-                place_link(ctx, &src, &target)?;
+                move_and_link(ctx, &src, &target)?;
                 report.line("moved", &rel);
                 return Ok(());
             }
@@ -332,8 +390,7 @@ fn link_one(ctx: &Ctx, repo: &Repo, path: &Path, force: bool, report: &Report) -
         }
 
         Some(_) => {
-            move_path(ctx, &src, &target)?;
-            place_link(ctx, &src, &target)?;
+            move_and_link(ctx, &src, &target)?;
             report.line("moved", &rel);
             Ok(())
         }
@@ -355,6 +412,57 @@ fn link_one(ctx: &Ctx, repo: &Repo, path: &Path, force: bool, report: &Report) -
     }
 }
 
+// the two halves have to end up looking like one step: with the payload moved
+// but no link placed, the store holds a path nothing points at and the working
+// tree has lost it altogether, which is the one outcome worse than failing
+fn move_and_link(ctx: &Ctx, src: &Path, target: &Path) -> Result<()> {
+    move_path(ctx, src, target)?;
+
+    if let Err(e) = place_link(ctx, src, target) {
+        move_path(ctx, target, src).with_context(|| {
+            format!(
+                "could not put {} back after failing to link it to {}",
+                src.display(),
+                target.display()
+            )
+        })?;
+
+        return Err(e);
+    }
+
+    Ok(())
+}
+
+// creating the store directories follows any symlink already standing in them,
+// so a store that holds one would take the move somewhere else entirely; only
+// the components at or below the store are examined, since everything above it
+// is outside by definition
+fn stays_in_store(repo: &Repo, rel: &Path) -> Result<()> {
+    let mut path = repo.store.clone();
+
+    for part in rel.components() {
+        path.push(part);
+
+        match symlink_metadata_opt(&path)? {
+            // nothing here yet, so nothing below it can be followed either
+            None => return Ok(()),
+            Some(meta) if meta.is_symlink() => {
+                if let Leads::Outside(end) = leads(&path, &repo.store) {
+                    return Err(anyhow!(
+                        "the store holds {} as a symlink to {}, outside the store; \
+                         refusing to write through it",
+                        path.display(),
+                        end.display()
+                    ));
+                }
+            }
+            Some(_) => {}
+        }
+    }
+
+    Ok(())
+}
+
 fn needs_force(target: &Path) -> anyhow::Error {
     anyhow!(
         "{} already exists; pass --force to link to it",