diff --git a/crates/batten/tests/it/common/mod.rs b/crates/batten/tests/it/common/mod.rs index 0ea0dea99..0fa5397c9 100644 --- a/crates/batten/tests/it/common/mod.rs +++ b/crates/batten/tests/it/common/mod.rs @@ -807,6 +807,15 @@ pub(crate) fn git_command(dir: &Path, args: &[&str]) -> Command { /// of redundant `git init`s is the wrong trade for a once-per-filesystem cost, /// and a lock file is one more thing to leak. /// +/// # `None` is an answer, not a failure (CLOUD-1832) +/// +/// Every reading of a failed publish that does NOT leave a complete template +/// behind ends here, and the honest report is that this process has no template +/// — not a path it hopes is one. The caller's fallback is the `git init` fork +/// this template exists to avoid, which is a cost rather than a defect, so the +/// suite stays green on a filesystem where the publish cannot land. The state is +/// printed, not swallowed: the next run that hits it says why. +/// /// # The stamp is the `git` binary's own metadata /// /// A hand-bumped version constant would leave a stale template behind whenever @@ -826,35 +835,119 @@ pub(crate) fn git_command(dir: &Path, args: &[&str]) -> Command { /// `.git/config` is the same values by a cheaper route. A fixture that wants a /// DIFFERENT identity still sets it after the copy, and one whose subject is an /// UNSET identity unsets it — `attribution.rs` already does exactly that. -fn git_init_template() -> &'static Path { - static TEMPLATE: std::sync::OnceLock = std::sync::OnceLock::new(); - TEMPLATE.get_or_init(|| { - let published = target_tmp().join(format!("git-init-template-{}", git_stamp())); - if published.join("HEAD").is_file() && published.join("config").is_file() { - return published; - } - let staging = target_tmp().join(format!( - "git-init-template-{}.staging-{}", - git_stamp(), - std::process::id() - )); +fn git_init_template() -> Option<&'static Path> { + static TEMPLATE: std::sync::OnceLock> = std::sync::OnceLock::new(); + TEMPLATE.get_or_init(build_git_init_template).as_deref() +} + +/// Establish the template once per process, or report that this process could +/// not — see [`git_init_template`] for why the answer is an `Option`. +#[allow(clippy::print_stderr)] +fn build_git_init_template() -> Option { + let published = target_tmp().join(format!("git-init-template-{}", git_stamp())); + if is_template(&published) { + return Some(published); + } + let staging = target_tmp().join(format!( + "git-init-template-{}.staging-{}", + git_stamp(), + std::process::id() + )); + let _ = fs::remove_dir_all(&staging); + fs::create_dir_all(&staging).expect("create the template staging directory"); + fork_the_template_into(&staging); + // Publish the `.git` itself rather than the work tree around it: what a + // fixture copies is a repository directory, and lifting it here keeps + // `init_repo` from having to know the template's internal layout. + if let Err(error) = fs::rename(staging.join(".git"), &published) { + // A FAILED RENAME IS NOT A REPORT THAT SOMEBODY ELSE WON (CLOUD-1832). + // That is the LIKELY reading — `rename(2)` onto a non-empty directory + // is `ENOTEMPTY` and the winner's copy is complete — but it is not the + // only one. A `staging/.git` that was never created, a cross-device + // error, a publish interrupted mid-flight: every one of them arrives + // here, and returning the path unasked turns "I could not establish a + // template" into "here is a template". `init_repo` then copies + // whatever is at that path into a fixture, and the first thing to + // notice is a `git add -A` three calls later reporting a directory + // that is not a repository — a could-not-look wearing a result's + // clothes, which is the one thing this repository refuses. + // + // So the loser's branch reports what it could not establish rather + // than assuming it. It does NOT fail the case: the template is a + // COST optimisation over a `git init` this module still knows how to + // fork, so the sound answer is `None` and one fork for this process. + // Failing here would red a suite whose subject is elsewhere for a + // reason that is purely about how the fixture was built — the + // measured CI failure of exactly that shape is why this is a + // fallback and not an assertion. + let listing = fs::read_dir(&published).map_or_else( + |error| format!("unreadable: {error}"), + |entries| { + let mut names: Vec = entries + .filter_map(Result::ok) + .map(|entry| entry.file_name().to_string_lossy().into_owned()) + .collect(); + names.sort(); + format!("[{}]", names.join(" ")) + }, + ); + let staged_git = staging.join(".git").is_dir(); let _ = fs::remove_dir_all(&staging); - fs::create_dir_all(&staging).expect("create the template staging directory"); - git_in(&staging, &["init", "-q"]); - git_in(&staging, &["config", "user.email", "t@example.com"]); - git_in(&staging, &["config", "user.name", "t"]); - // Publish the `.git` itself rather than the work tree around it: what a - // fixture copies is a repository directory, and lifting it here keeps - // `init_repo` from having to know the template's internal layout. - if fs::rename(staging.join(".git"), &published).is_err() { - // Another process published first, which is the whole point of the - // rename. Its copy is complete; ours is not needed. - let _ = fs::remove_dir_all(&staging); - return published; + if is_template(&published) { + return Some(published); } - let _ = fs::remove_dir_all(&staging); - published - }) + // Diagnostics rather than a verdict: this names the state that the + // assertion used to only assert, so the next run that hits it says + // WHY instead of that it happened. + // + // `print_stderr` is allowed here and only here. The lint exists so the + // BINARY never writes an ungoverned line to a channel its output + // contract owns (house-style §6); this is the test harness, whose + // stderr nextest already captures per process and prints on failure. + // The alternative is a fallback that leaves no trace at all — a silent + // change of route, which is the shape that made this defect expensive + // to see. The allowance sits on the function rather than here because + // a statement-scoped one does not reach this macro — clippy reports it + // as an unused attribute. + eprintln!( + "note: the template publish at {} failed ({error}; staged .git \ + present: {staged_git}) and {} is not a complete template \ + (contents: {listing}), so this process forks `git init` per \ + fixture instead — see CLOUD-1832", + staging.display(), + published.display(), + ); + return None; + } + let _ = fs::remove_dir_all(&staging); + Some(published) +} + +/// Build the repository [`git_init_template`] publishes, by forking, in `dir`. +/// +/// The route [`init_repo`] falls back to when no template could be established, +/// and the reason it is a named function rather than three lines inside that +/// `else`: the two `config` calls are not decoration. The template bakes the +/// identity into its own `config`, the binary under test reads it through +/// `git::config_value`, and a fallback that forked `init` alone would hand the +/// fixture an UNSET identity — a difference no fixture asks about and every +/// attribution case would feel. Same commands, same order, same values as the +/// staging build above. +pub(crate) fn fork_the_template_into(dir: &Path) { + git_in(dir, &["init", "-q"]); + git_in(dir, &["config", "user.email", "t@example.com"]); + git_in(dir, &["config", "user.name", "t"]); +} + +/// Whether `dir` is a published template this module may copy from. +/// +/// The same two files [`git_init_template`] checks before it stages, named once +/// so the pre-check and the post-check cannot drift into disagreeing about what +/// "published" means. Deliberately cheap and structural rather than a `git` +/// fork: the question is whether the directory is a repository at all, and a +/// fork per fixture is the cost this template exists to remove. +pub(crate) fn is_template(dir: &Path) -> bool { + dir.join("HEAD").is_file() && dir.join("config").is_file() } /// The resolved `git` binary's length and mtime, as one path-safe token. @@ -931,10 +1024,33 @@ pub(crate) fn init_repo(dir: &Path) { template copy is not, so a second initialisation has to be deliberate", dir.display() ); - if dir.starts_with(target_tmp()) { - copy_tree(git_init_template(), &dir.join(".git")); - } else { + if !dir.starts_with(target_tmp()) { git_in(dir, &["init", "-q"]); + return; + } + let Some(template) = git_init_template() else { + // No template could be established (CLOUD-1832): fork what it stands + // for. + fork_the_template_into(dir); + return; + }; + { + copy_tree(template, &dir.join(".git")); + // CHECK WHAT THE COPY PRODUCED, HERE (CLOUD-1832). `copy_tree` creates + // its destination and copies whatever it finds, so a template that was + // not a repository yields a `.git` that is not one either — silently. + // The failure then surfaces wherever the fixture first runs git, which + // in the measured case was `base_commit`'s `git add -A` reporting + // "not a git repository" in a case about config loading. Asserting at + // the point of creation is the difference between a defect that names + // itself and one that reads as an unrelated test being broken. + assert!( + is_template(&dir.join(".git")), + "copying the template {} into {} did not produce a repository — see \ + CLOUD-1832", + template.display(), + dir.display(), + ); } } diff --git a/crates/batten/tests/it/primitives.rs b/crates/batten/tests/it/primitives.rs index 17d5bf415..806eb1afd 100644 --- a/crates/batten/tests/it/primitives.rs +++ b/crates/batten/tests/it/primitives.rs @@ -1900,3 +1900,112 @@ fn no_case_names_the_template_directory() { `make_empty` would wipe it: {named:?}" ); } + +// THE PREDICATE BOTH TEMPLATE ASSERTIONS REST ON (CLOUD-1832). +// +// `git_init_template` publishes by atomic rename and, when that rename failed, +// used to return the published path unasked — reading "rename failed" as +// "somebody else won and their copy is complete". Every other cause of a failed +// rename arrived at the same `return`, so a process that could not establish a +// template handed one back anyway, and `init_repo` copied it into a fixture. The +// first thing to notice was a `git add -A` several calls later reporting a +// directory that is not a repository: a could-not-look rendered as a result. +// +// Both halves of the repair — the loser's branch, and `init_repo` checking what +// its copy produced — are straight-line uses of `common::is_template`, so this is +// where the discrimination has to be shown. Poisoning the REAL published template +// would be the stronger end-to-end arm and is deliberately not taken: that path +// is shared by every test process under one `CARGO_TARGET_TMPDIR`, so a case that +// corrupted it would red its concurrent siblings — which is precisely the +// cross-process hazard this row is about. +#[test] +fn a_directory_is_a_template_only_when_it_carries_a_repositorys_own_files() { + let root = common::scratch("is-template"); + + assert!( + !common::is_template(&root.join("never-created")), + "a path that does not exist is not a template" + ); + + let empty = root.join("empty"); + std::fs::create_dir_all(&empty).expect("create the empty candidate"); + assert!( + !common::is_template(&empty), + "an empty directory is not a template — that is the shape a failed \ + publish leaves behind, and the one that used to be handed back as \ + though it were a repository" + ); + + let headless = root.join("no-head"); + std::fs::create_dir_all(&headless).expect("create the headless candidate"); + std::fs::write(headless.join("config"), "[core]\n").expect("write config"); + assert!( + !common::is_template(&headless), + "config alone is not a repository" + ); + + let configless = root.join("no-config"); + std::fs::create_dir_all(&configless).expect("create the configless candidate"); + std::fs::write(configless.join("HEAD"), "ref: refs/heads/main\n").expect("write HEAD"); + assert!( + !common::is_template(&configless), + "HEAD alone is not a repository" + ); + + // The positive case comes from a repository this suite actually built, not + // from two files this test wrote: a predicate checked only against hand-made + // directories could agree with itself and disagree with git. + let real = common::Fixture::new("is-template-real") + .file("a.txt", "a\n") + .git() + .base_commit() + .build(); + assert!( + common::is_template(&real.join(".git")), + "a repository this suite just built must satisfy the predicate its own \ + fixtures are checked against" + ); +} + +// CLOUD-1832, the other half: what a process does when it could NOT establish a +// template. +// +// The first repair asserted, and that is what CI then failed on — the assertion +// fired on a cold musl scratch root and reddened a case whose subject was +// elsewhere. A missing template is a COST, not a defect: `common` still knows +// how to fork the repository the template stands for, so the sound answer is to +// fork it. `init_repo` takes that route whenever `git_init_template` returns +// `None`, and this pins what the route has to produce. +// +// Driving it through `init_repo` itself is not available — the template is a +// process-wide `OnceLock` and a case cannot un-establish it without redding its +// concurrent siblings, the same cross-process hazard the case above declines. So +// the fallback's BODY is the named function both routes call, and it is checked +// directly: a repository, with the identity the template bakes into its config. +// A fallback that forked `init` alone would pass `is_template` and still hand +// every fixture an unset identity, so the identity is asserted, not assumed. +#[test] +fn the_route_taken_when_no_template_exists_builds_the_repository_the_template_would_have() { + let dir = common::scratch("template-fallback").join("repo"); + std::fs::create_dir_all(&dir).expect("create the fallback candidate"); + + common::fork_the_template_into(&dir); + + assert!( + common::is_template(&dir.join(".git")), + "the fallback has to produce a repository — it stands in for a template \ + whose whole content is one" + ); + assert_eq!( + common::git_in(&dir, &["config", "user.email"]), + "t@example.com", + "the template bakes the identity into its own config, so a fallback \ + that left it unset would differ from the copy route in a way the \ + binary under test can read" + ); + assert_eq!( + common::git_in(&dir, &["config", "user.name"]), + "t", + "both halves of the identity, for the same reason" + ); +}