From 6b2d86ff5af83b8240edecf44cd3a2f2fb09157d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matej=20Jane=C5=BEi=C4=8D?= Date: Wed, 9 Sep 2026 12:03:16 +0000 Subject: [PATCH] fix: make the link store hold what it says it holds --- src/cmd/argv.rs | 13 +-- src/cmd/mod.rs | 4 + src/commands/link.rs | 146 +++++++++++++++++++++++---- src/commands/link/check.rs | 57 ++++++++++- src/commands/link/store.rs | 199 +++++++++++++++++++++++++++++++++---- src/fsops.rs | 43 +++++++- 6 files changed, 413 insertions(+), 49 deletions(-) diff --git a/src/cmd/argv.rs b/src/cmd/argv.rs index 02fe891..e7ae74c 100644 --- a/src/cmd/argv.rs +++ b/src/cmd/argv.rs @@ -151,14 +151,15 @@ impl Argv { Ok(self.command(ctx)?.stdin(stdin).output()?) } + // a probe: it asks a question rather than changing anything, so a dry run + // answers it for real, and the status is the answer instead of an error + pub fn probe_status(&self, ctx: &Ctx) -> Result { + Ok(self.command(ctx)?.stdout(Stdio::null()).spawn()?.wait()?) + } + // whether it succeeded, without failure being an error pub fn quietly_succeeds(&self, ctx: &Ctx) -> Result { - Ok(self - .command(ctx)? - .stdout(Stdio::null()) - .spawn()? - .wait()? - .success()) + Ok(self.probe_status(ctx)?.success()) } fn command(&self, ctx: &Ctx) -> Result { diff --git a/src/cmd/mod.rs b/src/cmd/mod.rs index 638a012..4c05793 100644 --- a/src/cmd/mod.rs +++ b/src/cmd/mod.rs @@ -83,4 +83,8 @@ pub trait Cmd { fn quietly_succeeds(&self, ctx: &Ctx) -> Result { self.argv().quietly_succeeds(ctx) } + + fn probe_status(&self, ctx: &Ctx) -> Result { + self.argv().probe_status(ctx) + } } diff --git a/src/commands/link.rs b/src/commands/link.rs index c5b85be..5a725be 100644 --- a/src/commands/link.rs +++ b/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> { 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> { 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", diff --git a/src/commands/link/check.rs b/src/commands/link/check.rs index 03bf210..7b4f980 100644 --- a/src/commands/link/check.rs +++ b/src/commands/link/check.rs @@ -5,7 +5,7 @@ use std::path::{Path, PathBuf}; use anyhow::{Result, anyhow}; -use super::store::{Repo, resolve, symlink_metadata_opt}; +use super::store::{Leads, Repo, leads, resolve, symlink_metadata_opt}; use crate::cmd::{Cmd, LsFiles}; use crate::ctx::Ctx; use crate::output::{line, text, write_bytes}; @@ -33,6 +33,11 @@ pub fn check( } } + // and says nothing about tracked paths, where a symlink out still counts + for entry in list_tracked(ctx, &repo, &pathspecs)? { + exposed.extend(classify_tracked(&repo, &path_from(&entry))?); + } + exposed.sort_by(|a, b| a.name.cmp(&b.name)); if porcelain || null { @@ -139,6 +144,8 @@ impl Section { const UNTRACKED: char = '?'; const IGNORED: char = '!'; const ELSEWHERE: char = '>'; +// no git equivalent: a tracked path, which only ever appears as a symlink out +const TRACKED: char = 'T'; struct Exposed { mark: char, @@ -178,7 +185,16 @@ fn classify(repo: &Repo, rel: &Path, mark: char) -> Result> { let dest = read_link(&src)?; if dest == repo.store.join(rel) { - return Ok(None); + // it names the store, but what the store holds there can be a symlink of + // its own leading straight back out, which is not being held at all + return Ok(match leads(&src, &repo.store) { + Leads::Inside | Leads::Dangling => None, + Leads::Outside(end) => Some(Exposed { + mark, + name, + dest: Some(end), + }), + }); } Ok(Some(Exposed { @@ -188,6 +204,29 @@ fn classify(repo: &Repo, rel: &Path, mark: char) -> Result> { })) } +// git tracks symlinks, so one can lead out of the repository without ever +// showing up in an untracked or ignored listing; `add` cannot externalize it +// either, so all check can do is say it is there +fn classify_tracked(repo: &Repo, rel: &Path) -> Result> { + let src = repo.root.join(rel); + + let Some(meta) = symlink_metadata_opt(&src)? else { + return Ok(None); + }; + if !meta.is_symlink() { + return Ok(None); + } + + Ok(match leads(&src, &repo.root) { + Leads::Inside | Leads::Dangling => None, + Leads::Outside(end) => Some(Exposed { + mark: TRACKED, + name: rel.to_path_buf(), + dest: Some(end), + }), + }) +} + fn walk(repo: &Repo, rel: &Path, mark: char) -> Result<(usize, Vec)> { let dir = repo.root.join(rel); let mut handled = 0; @@ -210,8 +249,12 @@ fn walk(repo: &Repo, rel: &Path, mark: char) -> Result<(usize, Vec)> { } } - // nothing below is in the store, so collapse to one line - if handled == 0 && !exposed.is_empty() { + // nothing below is in the store, so collapse to one line -- unless some of + // it leads out of the repository, which is a different thing to report and + // carries a destination the one line would drop + let all_content = exposed.iter().all(|item| item.dest.is_none()); + + if handled == 0 && !exposed.is_empty() && all_content { let mut name = rel.as_os_str().to_owned(); name.push("/"); @@ -257,6 +300,12 @@ fn list_others( Ok(split_nul(&listing.capture_bytes(ctx)?)) } +fn list_tracked(ctx: &Ctx, repo: &Repo, pathspecs: &[PathBuf]) -> Result>> { + let listing = LsFiles::tracked(&repo.root).limited_to(pathspecs); + + Ok(split_nul(&listing.capture_bytes(ctx)?)) +} + // every listing check reads is asked for with -z, and kept as the bytes git // wrote: a path is not obliged to be utf-8, and one that is not used to fail // the whole listing rather than the one entry diff --git a/src/commands/link/store.rs b/src/commands/link/store.rs index aa33807..7be74c8 100644 --- a/src/commands/link/store.rs +++ b/src/commands/link/store.rs @@ -1,8 +1,8 @@ use fs_err as fs; use std::env; -use std::ffi::OsString; -use std::os::unix::ffi::OsStringExt; -use std::path::{Path, PathBuf}; +use std::ffi::{OsStr, OsString}; +use std::os::unix::ffi::{OsStrExt, OsStringExt}; +use std::path::{Component, Path, PathBuf}; use anyhow::{Context, Result, anyhow}; @@ -23,10 +23,10 @@ pub(super) struct Repo { impl Repo { pub(super) fn discover(ctx: &Ctx, store: Option<&Path>) -> Result { let root = git_root(ctx)?; - let base = match store { + let base = normalized(match store { Some(store) => store.to_path_buf(), None => store_root()?, - }; + })?; Ok(Self { store: base.join(repo_components(ctx, &root)?), @@ -69,6 +69,28 @@ pub(super) fn resolve(path: &Path) -> Result { Ok(parent.join(name)) } +// absolute, with `.` and `..` folded out. the store path is written into every +// symlink `add` creates, where a relative one would resolve from the link's own +// directory rather than the working one, and it is compared against the +// repository root, which a `..` would slip past +fn normalized(path: PathBuf) -> Result { + let absolute = std::path::absolute(&path) + .with_context(|| format!("resolving absolute path of {}", path.display()))?; + + let mut out = PathBuf::new(); + for part in absolute.components() { + match part { + Component::CurDir => {} + Component::ParentDir => { + out.pop(); + } + part => out.push(part), + } + } + + Ok(out) +} + fn git_root(ctx: &Ctx) -> Result { // bytes, since the checkout can live at a path that is not utf-8, and with // the cause kept: git not being installed and the directory not being a @@ -101,7 +123,7 @@ fn repo_components(ctx: &Ctx, root: &Path) -> Result { let name = root .file_name() .ok_or_else(|| anyhow!("cannot derive a store path for {}", root.display()))?; - Ok(Path::new(LOCAL_NAMESPACE).join(sanitize(&name.to_string_lossy()))) + Ok(Path::new(LOCAL_NAMESPACE).join(sanitize_name(name))) } fn git_origin_url(ctx: &Ctx) -> Option { @@ -141,7 +163,7 @@ fn components_from_remote(url: &str) -> Option { (depth > 0).then_some(components) } -fn sanitize(s: &str) -> String { +fn cleaned(s: &str) -> String { let out: String = s .chars() .map(|c| { @@ -154,10 +176,51 @@ fn sanitize(s: &str) -> String { .collect(); // `.` and `..` are legal characters but not legal components - if out.chars().all(|c| c == '.') { - return "_".repeat(out.len()); + match out.chars().all(|c| c == '.') { + true => "_".repeat(out.len()), + false => out, } - out +} + +fn sanitize(s: &str) -> String { + let out = cleaned(s); + + // every replaced character maps to the same `_`, so `my~api` and `my:api` + // would otherwise share one directory with the plain `my_api`. a component + // that came through untouched keeps its name, so the common remote keeps + // the store path it already has + match out == s { + true => out, + false => format!("{out}-{}", fingerprint(s.as_bytes())), + } +} + +// a checkout name is bytes like any other path. a lossy rendering turns every +// byte it cannot read into the same replacement character, so sanitize would +// see two different names as one and fingerprint them identically: the bytes +// themselves are what has to be fingerprinted +fn sanitize_name(name: &OsStr) -> String { + match name.to_str() { + Some(text) => sanitize(text), + None => format!( + "{}-{}", + cleaned(&name.to_string_lossy()), + fingerprint(name.as_bytes()) + ), + } +} + +// fnv-1a: the store path has to stay put across rust releases, which the hashers +// in std explicitly do not promise +fn fingerprint(bytes: &[u8]) -> String { + let mut hash: u64 = 0xcbf2_9ce4_8422_2325; + + for byte in bytes { + hash ^= u64::from(*byte); + hash = hash.wrapping_mul(0x100_0000_01b3); + } + + format!("{:08x}", hash as u32) } fn store_root() -> Result { @@ -186,10 +249,18 @@ pub(super) fn tracked(ctx: &Ctx, repo: &Repo, rel: &Path) -> Result { Ok(!listed.is_empty()) } -pub(super) fn ignored(ctx: &Ctx, repo: &Repo, rel: &Path) -> bool { - CheckIgnore::new(&repo.root, rel) - .quietly_succeeds(ctx) - .unwrap_or(false) +// check-ignore says 0 for ignored and 1 for not; anything else means it could +// not answer at all, which is not the same as "not ignored". git cannot even be +// asked about a path whose name looks like pathspec magic, since it rejects the +// magic rather than the name, and `:(literal)` is not accepted by this command +pub(super) fn ignored(ctx: &Ctx, repo: &Repo, rel: &Path) -> Result { + let status = CheckIgnore::new(&repo.root, rel).probe_status(ctx)?; + + match status.code() { + Some(0) => Ok(true), + Some(1) => Ok(false), + _ => Err(anyhow!("`git check-ignore` exited with {status}")), + } } pub(super) fn symlink_metadata_opt(path: &Path) -> Result> { @@ -201,9 +272,40 @@ pub(super) fn symlink_metadata_opt(path: &Path) -> Result Leads { + let Ok(end) = fs::canonicalize(path) else { + return Leads::Dangling; + }; + + // the root can be reached through a symlink of its own, so resolve it too + // rather than comparing a resolved path against an unresolved prefix + let root = fs::canonicalize(root).unwrap_or_else(|_| root.to_path_buf()); + + match end.starts_with(&root) { + true => Leads::Inside, + false => Leads::Outside(end), + } +} + #[cfg(test)] mod tests { - use super::{components_from_remote, sanitize}; + use super::{components_from_remote, fingerprint, normalized, sanitize, sanitize_name}; + use std::collections::HashSet; + use std::ffi::OsStr; + use std::os::unix::ffi::OsStrExt; use std::path::PathBuf; #[test] @@ -258,9 +360,70 @@ mod tests { #[test] fn sanitize_never_yields_a_traversal() { - assert_eq!(sanitize(".."), "__"); - assert_eq!(sanitize("."), "_"); - assert_eq!(sanitize("a/b"), "a_b"); + for name in ["..", ".", "a/b", "../..", "a/../b"] { + let out = sanitize(name); + + assert!(!out.contains('/'), "{name} -> {out}"); + assert!(out != "." && out != "..", "{name} -> {out}"); + } + + // a name that needed no replacing keeps the store path it already has assert_eq!(sanitize(".env"), ".env"); + assert_eq!(sanitize("afurnik"), "afurnik"); + assert_eq!(sanitize("git.aflabs.org"), "git.aflabs.org"); + } + + #[test] + fn sanitize_keeps_names_apart_that_replacing_would_collapse() { + // every disallowed character maps to `_`, so without the fingerprint + // these would all share one store directory with a plain `my_api` + let names = ["my~api", "my:api", "my api", "my/api", "my%api"]; + + for name in names { + assert_ne!(sanitize(name), sanitize("my_api"), "name: {name}"); + } + + let distinct: HashSet = names.iter().map(|name| sanitize(name)).collect(); + assert_eq!(distinct.len(), names.len(), "{distinct:?}"); + } + + #[test] + fn a_fingerprint_does_not_drift_with_the_toolchain() { + // std's hashers make no such promise, and a moved store loses the files + assert_eq!(fingerprint(b""), "84222325"); + assert_eq!(fingerprint(b"my~api"), fingerprint(b"my~api")); + assert_ne!(fingerprint(b"my~api"), fingerprint(b"my:api")); + } + + #[test] + fn a_checkout_name_that_is_not_utf_8_keeps_its_own_directory() { + // both render to the same replacement character, so a fingerprint taken + // of the rendering rather than the bytes cannot tell them apart + let one = OsStr::from_bytes(b"proj\xe9"); + let two = OsStr::from_bytes(b"proj\xff"); + + assert_ne!(sanitize_name(one), sanitize_name(two)); + // and a name that is utf-8 is keyed exactly as before + assert_eq!(sanitize_name(OsStr::new("afurnik")), "afurnik"); + } + + #[test] + fn a_store_root_is_absolute_with_the_dots_folded_out() { + // it is written into every symlink add creates, and compared against the + // repository root, so it needs exactly one spelling + let cases = [ + ("/a/b/../c", "/a/c"), + ("/a/./b", "/a/b"), + ("/a/b/../../c", "/c"), + ("/../..", "/"), + ]; + + for (from, want) in cases { + assert_eq!( + normalized(PathBuf::from(from)).unwrap(), + PathBuf::from(want), + "from: {from}" + ); + } } } diff --git a/src/fsops.rs b/src/fsops.rs index 9edbd98..9615028 100644 --- a/src/fsops.rs +++ b/src/fsops.rs @@ -1,7 +1,8 @@ use std::io::Write; +use std::os::unix::fs::DirBuilderExt; use std::path::{Path, PathBuf}; -use anyhow::{Context, Result}; +use anyhow::{Context, Result, anyhow}; // std::fs with the path and the operation already in the error use fs_err as fs; use fs_err::os::unix::fs::symlink; @@ -47,14 +48,52 @@ pub fn place_link(ctx: &Ctx, link_path: &Path, target: &Path) -> Result<()> { return Ok(()); } + // the parent can be missing when the store holds a path the repository does + // not have any more, and symlink would only report the bare ENOENT + ensure_parent(ctx, link_path)?; + // symlink under a temp name and rename over the path: the rename is atomic let tmp = suffixed(link_path, ".ahab-tmp"); - let _ = fs::remove_file(&tmp); + + // only ahab's own leftover is cleared away: anything else here belongs to + // the project, and silently unlinking it would lose it + match fs::symlink_metadata(&tmp) { + Ok(meta) if meta.is_symlink() => fs::remove_file(&tmp)?, + Ok(_) => { + return Err(anyhow!( + "{} is in the way and is not a symlink ahab left behind; \ + move it aside", + tmp.display() + )); + } + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => return Err(e.into()), + } symlink(target, &tmp)?; Ok(fs::rename(&tmp, link_path)?) } +// the store holds what should not be reachable from the repository, so the +// directories it is kept in are the owner's alone. only the tree at or below +// `base` is created here; what is above it is the user's own business +pub fn ensure_private_parent(ctx: &Ctx, base: &Path, target: &Path) -> Result<()> { + if ctx.dry_run { + return Ok(()); + } + + let Some(parent) = target.parent().filter(|dir| dir.starts_with(base)) else { + return Ok(()); + }; + + // a recursive create applies the mode to every directory it makes + std::fs::DirBuilder::new() + .recursive(true) + .mode(0o700) + .create(parent) + .with_context(|| format!("creating {}", parent.display())) +} + pub fn remove_file(ctx: &Ctx, path: &Path) -> Result<()> { if ctx.dry_run { return Ok(());