a73x

b2db9cf4

Stop the server rewriting the repositories it serves

a73x   2026-08-13 09:23

Commit message
Stop the server rewriting the repositories it serves

`state::list_patches` called `migrate_patch_layout`, and the server calls
`list_patches` on every repo page. So rendering wrote to the repository
being rendered: any anonymous visitor mutated it, with no lock, able to
race a concurrent request or an in-flight `receive-pack`.

The crux is not "stop calling migrate" — it is that the read path had no
way to see an unmigrated patch at all. `patch_refs_under` globbed
`<id>/events`, which a bare `refs/collab/patches/<id>` does not match, so
without the migration the patch was simply invisible and the page would
have rendered an empty list instead of writing. Reads now tolerate both
layouts: the bare ref *is* the DAG and is walked where it lies. The two
shapes cannot coexist for one id — git will not have `<id>` be both a ref
and a directory — so there is nothing to disambiguate.

Migration moves to the callers that own their repository. The CLI does it
once at its entry point, because writes genuinely need it:
`<id>/rev/<oid>` and a bare `<id>` are a directory/file conflict git
refuses. `sync` keeps the call it already had. The server has none.

And an operator gets `git-collab-server migrate`, which is what makes the
sequencing work: once e5096ffc strips legacy support an unmigrated repo
will fail to read, and this is how you convert a roster *before* the
upgrade rather than from the wreckage after it. It walks `repos_dir` by
the server's own discovery rules, reports every repository including the
ones with nothing to do, and has a dry run.

It also fixes the diagnostic the old path never gave. Failures were
printed per patch on stderr with no repository name and nothing returned,
so a read-only mount produced noise and no verdict; now each repository is
probed for writability first — by writing, since a read-only mount leaves
the mode bits saying 0755 — and reported by name, with the run exiting
non-zero if anything was left behind.

Locking: each repository is migrated under its own `SyncLock`, the
advisory lock `git-collab sync` already takes, so a concurrent sync is
refused rather than raced. That is not a guarantee against `git
receive-pack`, which takes no such lock; what covers a push is narrower
and comes from git itself, since every ref written here goes through git2
and takes the per-ref lockfile. A push landing mid-run can still leave the
report stale, which is what the dry run is for.

Tests: an old-layout patch served over real HTTP across every page that
renders patches, with all of `refs/collab/**` byte-identical afterwards —
and the same patch asserted present on the list, the overview and its
detail page, because not writing is worthless if it means not reading.
Plus migrate's own behaviour: it migrates, it is idempotent, it walks
nested repos and skips dot-prefixed paths, it names a repository it cannot
write without letting that stop the others, and its dry run changes
nothing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
src/lib.rs
Old New
@@ -190,6 +190,21 @@ fn report(
190 } 190 }
191 191
192 pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> { 192 pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
193 // The one place the CLI brings a legacy patch layout up to date, and the
194 // reason it is here rather than inside `list_patches`: the CLI owns the
195 // repository it was invoked in, so a write is its to make, whereas the
196 // server does not own the repositories it serves and must never write
197 // while rendering. Reads tolerate both layouts either way; writes cannot,
198 // because `refs/collab/patches/<id>/rev/<oid>` and a bare
199 // `refs/collab/patches/<id>` are a directory/file conflict git refuses.
200 //
201 // Excluded: the `commit-msg` hook, which runs inside `git commit` and has
202 // one job — not to give git a reason to abort, and not to spray migration
203 // warnings into the middle of a commit.
204 if !matches!(cli.command, Commands::Hooks(HookCmd::RunCommitMsg { .. })) {
205 state::migrate_patch_layout(repo);
206 }
207
193 let is_write = cli.command.is_write(); 208 let is_write = cli.command.is_write();
194 match cli.command { 209 match cli.command {
195 Commands::Init => sync::init(repo), 210 Commands::Init => sync::init(repo),
src/server/main.rs
Old New
@@ -1,11 +1,12 @@
1 use std::path::PathBuf; 1 use std::path::PathBuf;
2 2
3 use clap::Parser; 3 use clap::{CommandFactory, Parser, Subcommand};
4 use tracing::info; 4 use tracing::info;
5 5
6 mod config; 6 mod config;
7 mod governance; 7 mod governance;
8 mod http; 8 mod http;
9 mod migrate;
9 mod releases; 10 mod releases;
10 mod repos; 11 mod repos;
11 mod ssh; 12 mod ssh;
@@ -17,7 +18,11 @@ mod ssh;
17 about = "Minimal git hosting server" 18 about = "Minimal git hosting server"
18 )] 19 )]
19 struct Args { 20 struct Args {
20 #[arg(short, long, required_unless_present = "governance_hook")] 21 // Not `required_unless_present`: a subcommand is not an arg, so clap
22 // cannot express "unless one was given" here. Checked by hand at the one
23 // point that reaches for it, with the same clap error it would have
24 // produced.
25 #[arg(short, long)]
21 config: Option<PathBuf>, 26 config: Option<PathBuf>,
22 27
23 /// Internal: run as the `update` hook this server installs in the 28 /// Internal: run as the `update` hook this server installs in the
@@ -32,6 +37,28 @@ struct Args {
32 conflicts_with = "config" 37 conflicts_with = "config"
33 )] 38 )]
34 governance_hook: Option<Vec<String>>, 39 governance_hook: Option<Vec<String>>,
40
41 #[command(subcommand)]
42 command: Option<Command>,
43 }
44
45 #[derive(Subcommand)]
46 enum Command {
47 /// Bring hosted repositories' collab refs up to the current layout.
48 ///
49 /// Serving a repository never migrates it — rendering a page must not
50 /// write to the repository being rendered — so this is where an operator
51 /// converts a roster, deliberately, before an upgrade that drops support
52 /// for the old layout.
53 Migrate {
54 /// The same config the server runs with; only `repos_dir` is read.
55 #[arg(short, long)]
56 config: PathBuf,
57
58 /// Report what would change, and change nothing.
59 #[arg(long)]
60 dry_run: bool,
61 },
35 } 62 }
36 63
37 #[tokio::main] 64 #[tokio::main]
@@ -49,10 +76,25 @@ async fn main() {
49 return; 76 return;
50 } 77 }
51 78
52 // `required_unless_present` above leaves exactly one way to get here. 79 if let Some(Command::Migrate { config, dry_run }) = args.command {
53 let config_path = args 80 let config = match config::ServerConfig::from_file(&config) {
54 .config 81 Ok(c) => c,
55 .expect("--config is required without --governance-hook"); 82 Err(e) => {
83 eprintln!("Failed to load config from {:?}: {}", config, e);
84 std::process::exit(1);
85 }
86 };
87 std::process::exit(migrate::run(&config.repos_dir, dry_run));
88 }
89
90 let Some(config_path) = args.config else {
91 Args::command()
92 .error(
93 clap::error::ErrorKind::MissingRequiredArgument,
94 "the following required arguments were not provided:\n --config <CONFIG>",
95 )
96 .exit();
97 };
56 98
57 let config = match config::ServerConfig::from_file(&config_path) { 99 let config = match config::ServerConfig::from_file(&config_path) {
58 Ok(c) => c, 100 Ok(c) => c,
src/server/migrate.rs
Old New
@@ -0,0 +1,209 @@
1 //! `git-collab-server migrate`: bring hosted repositories' collab refs up to
2 //! the current layout, deliberately.
3 //!
4 //! Migration used to happen as a side effect of somebody loading a page, which
5 //! meant an anonymous HTTP request rewrote the repository being served (issue
6 //! 5174338f). Reading now tolerates the pre-migration layout without touching
7 //! it, which leaves migration needing an owner — and the owner is the operator,
8 //! here, at a moment of their choosing.
9 //!
10 //! Sequencing is the point. Once legacy support is stripped, an unmigrated
11 //! repository will not render at all; an operator needs to convert the roster
12 //! *before* upgrading rather than discovering the problem from a 500.
13 //!
14 //! ## Locking
15 //!
16 //! Each repository is migrated under its own `SyncLock` — `.git/collab/
17 //! sync.lock`, the advisory lock `git-collab sync` already takes. That is a
18 //! real guarantee against a concurrent `git-collab sync` in the same
19 //! repository, and it makes the tool refuse rather than race.
20 //!
21 //! It is *not* a guarantee against a concurrent `git receive-pack`: git takes
22 //! no such lock, and nothing this process does could make it. What protects
23 //! that case is narrower and comes from git itself — every ref this migration
24 //! writes goes through git2, which takes the per-ref lockfile, so an individual
25 //! update cannot interleave with a push's update of the same ref. A push
26 //! landing mid-run can still leave the run's *report* stale. An operator who
27 //! wants the stronger property should stop the service first, and the dry run
28 //! is there to tell them whether it is worth the downtime.
29
30 use std::path::Path;
31
32 use git_collab::state::{self, MigrationReport};
33 use git_collab::sync_lock::SyncLock;
34
35 use crate::repos;
36
37 /// What happened to one repository.
38 enum Outcome {
39 /// Nothing to do.
40 Current,
41 /// Changed, or — under `--dry-run` — would be.
42 Changed(MigrationReport),
43 /// Not attempted, or attempted and stopped before it could start, and why.
44 Blocked(String),
45 }
46
47 /// Migrate every repository under `repos_dir`. Returns the process exit code:
48 /// non-zero if anything at all could not be migrated, because a sweep run
49 /// before an upgrade is only useful if a partial result is loud.
50 pub fn run(repos_dir: &Path, dry_run: bool) -> i32 {
51 let entries = match repos::discover(repos_dir) {
52 Ok(entries) => entries,
53 Err(e) => {
54 eprintln!("error: cannot read {}: {}", repos_dir.display(), e);
55 return 1;
56 }
57 };
58
59 if entries.is_empty() {
60 println!("No repositories found under {}.", repos_dir.display());
61 return 0;
62 }
63
64 if dry_run {
65 println!("dry run: nothing will be written.");
66 }
67
68 let mut current = 0usize;
69 let mut changed = 0usize;
70 let mut blocked = 0usize;
71
72 for entry in &entries {
73 let outcome = migrate_one(entry, dry_run);
74 match &outcome {
75 Outcome::Current => current += 1,
76 Outcome::Changed(report) if report.failed.is_empty() => changed += 1,
77 // A report carrying failures counts as both: something moved, and
78 // something did not. It is the second half the exit code is about.
79 Outcome::Changed(_) => {
80 changed += 1;
81 blocked += 1;
82 }
83 Outcome::Blocked(_) => blocked += 1,
84 }
85 report(&entry.name, &outcome, dry_run);
86 }
87
88 println!(
89 "\n{} repositor{} scanned: {} {}, {} already current, {} could not be migrated.",
90 entries.len(),
91 if entries.len() == 1 { "y" } else { "ies" },
92 changed,
93 if dry_run { "to migrate" } else { "migrated" },
94 current,
95 blocked,
96 );
97
98 // A dry run reports; it does not judge. Failing it on work still to do
99 // would make "is there anything to migrate?" indistinguishable from "did
100 // the migration break?", and the answer to the first is routinely yes.
101 if blocked > 0 && !dry_run {
102 1
103 } else {
104 0
105 }
106 }
107
108 fn migrate_one(entry: &repos::RepoEntry, dry_run: bool) -> Outcome {
109 let repo = match repos::open(entry) {
110 Ok(repo) => repo,
111 Err(e) => return Outcome::Blocked(format!("cannot open the repository: {}", e)),
112 };
113
114 // Before anything else, and in the dry run too: whether the repository can
115 // be written at all. This is the diagnostic the old path never produced —
116 // on a read-only mount it failed once per patch on stderr and told the
117 // caller nothing — and an operator planning an upgrade needs it *before*
118 // the migration, not from its wreckage.
119 if let Err(reason) = probe_writable(&repo) {
120 return Outcome::Blocked(format!("the repository is not writable: {}", reason));
121 }
122
123 if dry_run {
124 let plan = state::plan_patch_layout_migration(&repo);
125 return if plan.is_current() {
126 Outcome::Current
127 } else {
128 Outcome::Changed(plan)
129 };
130 }
131
132 // See the module note on locking: this stops a concurrent `git-collab
133 // sync`, and says so rather than racing it.
134 let _lock = match SyncLock::acquire(&repo) {
135 Ok(lock) => lock,
136 Err(e) => return Outcome::Blocked(format!("{}", e)),
137 };
138
139 let report = state::migrate_patch_layout_reporting(&repo);
140 if report.is_current() {
141 Outcome::Current
142 } else {
143 Outcome::Changed(report)
144 }
145 }
146
147 /// Whether the repository can be written, answered by writing.
148 ///
149 /// Mode bits are not the question: a read-only bind mount leaves them saying
150 /// `0755` and still refuses every write, which is exactly the deployment this
151 /// check exists for. So the probe creates a file in the git directory — where
152 /// git's own ref lockfiles go — and deletes it on the spot. It leaves nothing
153 /// behind, which is what lets the dry run use it too.
154 fn probe_writable(repo: &git2::Repository) -> Result<(), String> {
155 tempfile::Builder::new()
156 .prefix(".git-collab-migrate-probe")
157 .tempfile_in(repo.path())
158 .map(|_| ())
159 .map_err(|e| e.to_string())
160 }
161
162 fn report(name: &str, outcome: &Outcome, dry_run: bool) {
163 match outcome {
164 Outcome::Current => println!("{}: already current", name),
165 Outcome::Blocked(reason) => println!("{}: cannot migrate — {}", name, reason),
166 Outcome::Changed(report) => {
167 let mut parts = Vec::new();
168 if !report.migrated.is_empty() {
169 parts.push(format!(
170 "{} {}",
171 if dry_run { "would migrate" } else { "migrated" },
172 plural(report.migrated.len(), "patch", "patches")
173 ));
174 }
175 if !report.repinned.is_empty() {
176 parts.push(format!(
177 "{} numbered revision refs on {}",
178 if dry_run { "would re-pin" } else { "re-pinned" },
179 plural(report.repinned.len(), "patch", "patches")
180 ));
181 }
182 if !report.resumed.is_empty() {
183 parts.push(format!(
184 "{} {} left by an interrupted migration",
185 if dry_run { "would finish" } else { "finished" },
186 plural(report.resumed.len(), "patch", "patches")
187 ));
188 }
189 if !report.failed.is_empty() {
190 parts.push(format!(
191 "{} could not be migrated",
192 plural(report.failed.len(), "patch", "patches")
193 ));
194 }
195 println!("{}: {}", name, parts.join(", "));
196 // Per-patch reasons, indented under the repository they belong to.
197 // The old path put these on stderr with no repository name at all,
198 // which on a multi-repo sweep named nothing an operator could act
199 // on.
200 for (_, message) in &report.failed {
201 println!(" {}", message);
202 }
203 }
204 }
205 }
206
207 fn plural(n: usize, singular: &str, plural: &str) -> String {
208 format!("{} {}", n, if n == 1 { singular } else { plural })
209 }
src/state.rs
Old New
@@ -1637,21 +1637,37 @@ fn refs_under(
1637 } 1637 }
1638 1638
1639 /// Enumerate the event DAG ref of every patch under a prefix, returning 1639 /// Enumerate the event DAG ref of every patch under a prefix, returning
1640 /// (ref_name, id) pairs. Only `<id>/events` counts as a patch; the revision 1640 /// (ref_name, id) pairs.
1641 /// refs beside it are commits, not DAGs. 1641 ///
1642 /// Both layouts, and reading only. In the current one the DAG is at
1643 /// `<id>/events` and the refs beside it — `rev/<oid>`, or `r/<n>` from the
1644 /// draft that preceded it — are commits, not DAGs. In the pre-migration one
1645 /// the bare `<id>` ref *is* the DAG, and it is read where it lies.
1646 ///
1647 /// Tolerating the old shape here rather than migrating on the way past is what
1648 /// lets the server serve an unmigrated repository without writing to it. The
1649 /// two cannot coexist for one id — git will not have `<id>` be both a ref and a
1650 /// directory — so there is nothing to disambiguate, and everything downstream
1651 /// takes the ref name it is handed.
1642 fn patch_refs_under( 1652 fn patch_refs_under(
1643 repo: &Repository, 1653 repo: &Repository,
1644 prefix: &str, 1654 prefix: &str,
1645 ) -> Result<Vec<(String, String)>, crate::error::Error> { 1655 ) -> Result<Vec<(String, String)>, crate::error::Error> {
1646 let refs = repo.references_glob(&format!("{}*/events", prefix))?; 1656 let refs = repo.references_glob(&format!("{}*", prefix))?;
1647 let mut result = Vec::new(); 1657 let mut result = Vec::new();
1648 for r in refs { 1658 for r in refs {
1649 let r = r?; 1659 let r = r?;
1650 let ref_name = r.name().unwrap_or_default().to_string(); 1660 let ref_name = r.name().unwrap_or_default().to_string();
1651 if let Some((id, "events")) = split_patch_ref(&ref_name, prefix) { 1661 let id = match split_patch_ref(&ref_name, prefix) {
1652 let id = id.to_string(); 1662 Some((id, "events")) => id.to_string(),
1653 result.push((ref_name, id)); 1663 // A revision ref, not a patch.
1664 Some(_) => continue,
1665 None => ref_name.strip_prefix(prefix).unwrap_or_default().to_string(),
1666 };
1667 if id.is_empty() {
1668 continue;
1654 } 1669 }
1670 result.push((ref_name, id));
1655 } 1671 }
1656 Ok(result) 1672 Ok(result)
1657 } 1673 }
@@ -1766,7 +1782,6 @@ pub fn list_issues(repo: &Repository) -> Result<Vec<IssueState>, crate::error::E
1766 1782
1767 /// List active patch refs, excluding any that also have an archived ref. 1783 /// List active patch refs, excluding any that also have an archived ref.
1768 pub fn list_patches(repo: &Repository) -> Result<Vec<PatchState>, crate::error::Error> { 1784 pub fn list_patches(repo: &Repository) -> Result<Vec<PatchState>, crate::error::Error> {
1769 migrate_patch_layout(repo);
1770 let archived_ids: std::collections::HashSet<String> = collab_archive_refs(repo, "patches")? 1785 let archived_ids: std::collections::HashSet<String> = collab_archive_refs(repo, "patches")?
1771 .into_iter() 1786 .into_iter()
1772 .map(|(_, id)| id) 1787 .map(|(_, id)| id)
@@ -1816,7 +1831,6 @@ pub fn list_issues_with_archived(
1816 pub fn list_patches_with_archived( 1831 pub fn list_patches_with_archived(
1817 repo: &Repository, 1832 repo: &Repository,
1818 ) -> Result<Vec<PatchState>, crate::error::Error> { 1833 ) -> Result<Vec<PatchState>, crate::error::Error> {
1819 migrate_patch_layout(repo);
1820 let mut seen = std::collections::HashSet::new(); 1834 let mut seen = std::collections::HashSet::new();
1821 let mut items = Vec::new(); 1835 let mut items = Vec::new();
1822 1836
@@ -2003,7 +2017,7 @@ const MIGRATION_PARK_PREFIX: &str = "refs/collab/local/migrating/patches/";
2003 /// was dropped — leaves the park behind as litter, and possibly a patch whose 2017 /// was dropped — leaves the park behind as litter, and possibly a patch whose
2004 /// revisions were never pinned. Both come out of one loop, and the park is only 2018 /// revisions were never pinned. Both come out of one loop, and the park is only
2005 /// dropped once the patch is whole. 2019 /// dropped once the patch is whole.
2006 fn resume_interrupted_migrations(repo: &Repository) { 2020 fn resume_interrupted_migrations(repo: &Repository, report: &mut MigrationReport) {
2007 let Ok(refs) = repo.references_glob(&format!("{}*", MIGRATION_PARK_PREFIX)) else { 2021 let Ok(refs) = repo.references_glob(&format!("{}*", MIGRATION_PARK_PREFIX)) else {
2008 return; 2022 return;
2009 }; 2023 };
@@ -2032,30 +2046,123 @@ fn resume_interrupted_migrations(repo: &Repository) {
2032 None => finish_migration(repo, PATCH_PREFIX, &id, &park_ref), 2046 None => finish_migration(repo, PATCH_PREFIX, &id, &park_ref),
2033 }; 2047 };
2034 if let Err(e) = outcome { 2048 if let Err(e) = outcome {
2035 eprintln!( 2049 report.failed.push((
2036 "warning: could not resume interrupted migration of patch {:.8}: {}", 2050 id.clone(),
2037 id, e 2051 format!(
2038 ); 2052 "could not resume interrupted migration of patch {:.8}: {}",
2053 id, e
2054 ),
2055 ));
2039 continue; 2056 continue;
2040 } 2057 }
2041 // Only now is the patch whole; the park has nothing left to protect. 2058 // Only now is the patch whole; the park has nothing left to protect.
2042 if let Ok(mut r) = repo.find_reference(&park_ref) { 2059 if let Ok(mut r) = repo.find_reference(&park_ref) {
2043 let _ = r.delete(); 2060 let _ = r.delete();
2044 } 2061 }
2062 report.resumed.push(id);
2045 } 2063 }
2046 } 2064 }
2047 2065
2066 /// What a patch-layout migration found in a repository, and what it managed to
2067 /// do about it.
2068 ///
2069 /// The old path printed warnings and returned nothing, which is fine for a CLI
2070 /// speaking to the person who ran it and useless to an operator sweeping a
2071 /// server: a read-only mount produced a line per patch on stderr and no way for
2072 /// the caller to know the repository had been left behind. This carries the
2073 /// same facts back as data.
2074 #[derive(Debug, Default, Clone, PartialEq, Eq)]
2075 pub struct MigrationReport {
2076 /// Patch ids brought from the bare `<id>` layout to `<id>/events`.
2077 pub migrated: Vec<String>,
2078 /// Patch ids whose numbered `r/<n>` refs were re-pinned by OID.
2079 pub repinned: Vec<String>,
2080 /// Patch ids whose interrupted migration was resumed or swept.
2081 pub resumed: Vec<String>,
2082 /// `(id, message)` for everything that could not be done, and why.
2083 pub failed: Vec<(String, String)>,
2084 }
2085
2086 impl MigrationReport {
2087 /// Nothing found and nothing to do: the repository is already current.
2088 pub fn is_current(&self) -> bool {
2089 self.migrated.is_empty()
2090 && self.repinned.is_empty()
2091 && self.resumed.is_empty()
2092 && self.failed.is_empty()
2093 }
2094 }
2095
2096 /// What [`migrate_patch_layout`] would do to this repository, without doing any
2097 /// of it.
2098 ///
2099 /// A scan of ref *names* only — the same three scans the migration itself runs,
2100 /// with the writes left out — so it is safe against a repository nobody wants
2101 /// touched yet. It cannot predict a failure, because a failure is something
2102 /// only the attempt discovers; `failed` is always empty here.
2103 pub fn plan_patch_layout_migration(repo: &Repository) -> MigrationReport {
2104 let mut report = MigrationReport::default();
2105
2106 if let Ok(refs) = repo.references_glob(&format!("{}*", MIGRATION_PARK_PREFIX)) {
2107 for r in refs.flatten() {
2108 if let Some(id) = r.name().and_then(|n| n.strip_prefix(MIGRATION_PARK_PREFIX)) {
2109 report.resumed.push(id.to_string());
2110 }
2111 }
2112 }
2113
2114 for prefix in [PATCH_PREFIX, ARCHIVE_PATCH_PREFIX] {
2115 let Ok(refs) = repo.references_glob(&format!("{}*", prefix)) else {
2116 continue;
2117 };
2118 for r in refs.flatten() {
2119 let Some(name) = r.name() else { continue };
2120 match split_patch_ref(name, prefix) {
2121 // The new layout always has a suffix; the old one never does.
2122 None => report
2123 .migrated
2124 .push(name.strip_prefix(prefix).unwrap_or_default().to_string()),
2125 Some((id, suffix)) if suffix.starts_with("r/") => {
2126 report.repinned.push(id.to_string())
2127 }
2128 Some(_) => {}
2129 }
2130 }
2131 }
2132
2133 report.migrated.sort();
2134 report.migrated.dedup();
2135 report.repinned.sort();
2136 report.repinned.dedup();
2137 report.resumed.sort();
2138 report.resumed.dedup();
2139 report
2140 }
2141
2048 /// Bring patches written in the pre-revision-refs layout — a single ref at 2142 /// Bring patches written in the pre-revision-refs layout — a single ref at
2049 /// `refs/collab/patches/<id>` — up to the current one. Called from every entry 2143 /// `refs/collab/patches/<id>` — up to the current one, printing a warning per
2050 /// point that enumerates or resolves a patch, so an old repository migrates on 2144 /// patch it could not convert.
2051 /// first use rather than breaking. 2145 ///
2146 /// This writes, so only a caller that owns the repository may run it: the CLI,
2147 /// at its entry point, and `sync`. **Not the server.** Reading tolerates both
2148 /// layouts (see [`patch_refs_under`]) precisely so that rendering a page never
2149 /// has to reach for this; an operator asks for it by hand with
2150 /// `git-collab-server migrate`.
2052 /// 2151 ///
2053 /// Failures are reported and skipped rather than propagated: one unmigratable 2152 /// Failures are reported and skipped rather than propagated: one unmigratable
2054 /// patch must not make the whole list unreadable. 2153 /// patch must not make the whole list unreadable.
2055 pub fn migrate_patch_layout(repo: &Repository) { 2154 pub fn migrate_patch_layout(repo: &Repository) {
2155 for (_, message) in migrate_patch_layout_reporting(repo).failed {
2156 eprintln!("warning: {}", message);
2157 }
2158 }
2159
2160 /// [`migrate_patch_layout`], with what it did handed back instead of printed.
2161 pub fn migrate_patch_layout_reporting(repo: &Repository) -> MigrationReport {
2162 let mut report = MigrationReport::default();
2056 // Before anything else: a patch stranded by an earlier interrupted run is 2163 // Before anything else: a patch stranded by an earlier interrupted run is
2057 // invisible to the scan below, because its old ref is already gone. 2164 // invisible to the scan below, because its old ref is already gone.
2058 resume_interrupted_migrations(repo); 2165 resume_interrupted_migrations(repo, &mut report);
2059 2166
2060 for prefix in [PATCH_PREFIX, ARCHIVE_PATCH_PREFIX] { 2167 for prefix in [PATCH_PREFIX, ARCHIVE_PATCH_PREFIX] {
2061 let Ok(refs) = repo.references_glob(&format!("{}*", prefix)) else { 2168 let Ok(refs) = repo.references_glob(&format!("{}*", prefix)) else {
@@ -2075,8 +2182,12 @@ pub fn migrate_patch_layout(repo: &Repository) {
2075 .collect(); 2182 .collect();
2076 for old_ref in old_layout { 2183 for old_ref in old_layout {
2077 let id = old_ref.strip_prefix(prefix).unwrap_or_default().to_string(); 2184 let id = old_ref.strip_prefix(prefix).unwrap_or_default().to_string();
2078 if let Err(e) = migrate_one_patch(repo, prefix, &id, &old_ref) { 2185 match migrate_one_patch(repo, prefix, &id, &old_ref) {
2079 eprintln!("warning: could not migrate patch {:.8}: {}", id, e); 2186 Ok(()) => report.migrated.push(id),
2187 Err(e) => report.failed.push((
2188 id.clone(),
2189 format!("could not migrate patch {:.8}: {}", id, e),
2190 )),
2080 } 2191 }
2081 } 2192 }
2082 2193
@@ -2096,14 +2207,19 @@ pub fn migrate_patch_layout(repo: &Repository) {
2096 ids.sort(); 2207 ids.sort();
2097 ids.dedup(); 2208 ids.dedup();
2098 for id in ids { 2209 for id in ids {
2099 if let Err(e) = retire_numbered_revision_refs(repo, prefix, &id) { 2210 match retire_numbered_revision_refs(repo, prefix, &id) {
2100 eprintln!( 2211 Ok(()) => report.repinned.push(id),
2101 "warning: could not convert numbered revision refs for patch {:.8}: {}", 2212 Err(e) => report.failed.push((
2102 id, e 2213 id.clone(),
2103 ); 2214 format!(
2215 "could not convert numbered revision refs for patch {:.8}: {}",
2216 id, e
2217 ),
2218 )),
2104 } 2219 }
2105 } 2220 }
2106 } 2221 }
2222 report
2107 } 2223 }
2108 2224
2109 /// Replace a patch's `r/<n>` refs with OID-named ones. 2225 /// Replace a patch's `r/<n>` refs with OID-named ones.
@@ -2244,7 +2360,6 @@ pub fn resolve_patch_ref(
2244 repo: &Repository, 2360 repo: &Repository,
2245 prefix: &str, 2361 prefix: &str,
2246 ) -> Result<(String, String), crate::error::Error> { 2362 ) -> Result<(String, String), crate::error::Error> {
2247 migrate_patch_layout(repo);
2248 resolve_ref(repo, "patches", "patch", prefix) 2363 resolve_ref(repo, "patches", "patch", prefix)
2249 } 2364 }
2250 2365
src/timeline.rs
Old New
@@ -41,12 +41,11 @@
41 //! # Reads do not write 41 //! # Reads do not write
42 //! 42 //!
43 //! Building a timeline appends no event and moves no ref. It resolves the 43 //! Building a timeline appends no event and moves no ref. It resolves the
44 //! patch's ref, walks it, and folds it. (`resolve_patch_ref` runs the one-time 44 //! patch's ref, walks it, and folds it — including a patch still in the
45 //! `migrate_patch_layout`, exactly as `patch show` and `patch log` already do; 45 //! pre-revision-refs layout, which resolves and walks where it lies rather
46 //! that is a pre-existing ref-layout migration, not a collab event, and this 46 //! than being migrated on the way past. (Note `patch show` additionally moves
47 //! view adds nothing to it. Note `patch show` additionally moves a 47 //! a `refs/collab/local/seen/` read-marker — the timeline deliberately does
48 //! `refs/collab/local/seen/` read-marker — the timeline deliberately does *not* 48 //! *not* adopt that pattern and marks nothing read.)
49 //! adopt that pattern and marks nothing read.)
50 49
51 use git2::Repository; 50 use git2::Repository;
52 use serde::Serialize; 51 use serde::Serialize;
tests/server_migrate_test.rs
Old New
@@ -0,0 +1,458 @@
1 //! Serving a repository is a read, and migrating one is a decision.
2 //!
3 //! Two halves of the same defect (5174338f). `state::list_patches` used to call
4 //! `migrate_patch_layout`, and the server calls `list_patches` on every page —
5 //! so an unauthenticated GET rewrote the repository being served, with no lock,
6 //! able to race a concurrent request or an in-flight `receive-pack`.
7 //!
8 //! The fix has to hold both ends up at once: the read path must tolerate the
9 //! pre-migration layout *without writing*, or "stop migrating on read" just
10 //! means "stop showing old patches". Hence a pair of tests over one live
11 //! server: the refs are byte-identical afterwards, and the patch is on the page.
12 //!
13 //! The other half is `git-collab-server migrate`, the command an operator runs
14 //! deliberately. It matters for sequencing: once legacy support is stripped an
15 //! unmigrated repository will simply fail to read, and the operator needs to
16 //! migrate *before* upgrading rather than discovering it afterwards.
17
18 mod common;
19
20 use common::{ServerHarness, TestRepo};
21 use std::path::{Path, PathBuf};
22 use std::process::{Command, Output};
23 use tempfile::TempDir;
24
25 // ---------------------------------------------------------------------------
26 // Fixtures
27 // ---------------------------------------------------------------------------
28
29 fn git_in(dir: &Path, args: &[&str]) -> String {
30 let output = Command::new("git")
31 .args(args)
32 .current_dir(dir)
33 .output()
34 .expect("failed to run git");
35 assert!(
36 output.status.success(),
37 "git {:?} in {:?} failed: {}",
38 args,
39 dir,
40 String::from_utf8_lossy(&output.stderr)
41 );
42 String::from_utf8(output.stdout).unwrap()
43 }
44
45 /// Every collab ref in `bare`, as sorted `<name> <oid>` lines.
46 ///
47 /// All of `refs/collab/**`, not just the patch refs: a migration parks tips
48 /// under `refs/collab/local/migrating/` and pins revisions under `<id>/rev/`,
49 /// and a snapshot narrower than the namespace would miss both.
50 fn collab_refs(bare: &Path) -> String {
51 let mut lines: Vec<String> = git_in(
52 bare,
53 &[
54 "for-each-ref",
55 "--format=%(refname) %(objectname)",
56 "refs/collab/",
57 ],
58 )
59 .lines()
60 .map(str::to_string)
61 .collect();
62 lines.sort();
63 lines.join("\n")
64 }
65
66 /// The full id of the one patch in `bare`, in either layout.
67 fn only_patch_id(bare: &Path) -> String {
68 let refs = git_in(bare, &["for-each-ref", "--format=%(refname)", "refs/collab/patches/"]);
69 let ids: Vec<String> = refs
70 .lines()
71 .filter_map(|name| {
72 let rest = name.strip_prefix("refs/collab/patches/")?;
73 Some(rest.split('/').next()?.to_string())
74 })
75 .collect();
76 let mut ids = ids;
77 ids.sort();
78 ids.dedup();
79 assert_eq!(ids.len(), 1, "expected exactly one patch, got {:?}", ids);
80 ids.into_iter().next().unwrap()
81 }
82
83 /// Put the patch back into the layout git-collab wrote before revision refs
84 /// existed: one bare ref at `refs/collab/patches/<id>` holding the event DAG,
85 /// and nothing beside it.
86 fn demote_to_old_layout(bare: &Path, id: &str) {
87 let events = format!("refs/collab/patches/{}/events", id);
88 let tip = git_in(bare, &["rev-parse", &events]).trim().to_string();
89 let names = git_in(
90 bare,
91 &[
92 "for-each-ref",
93 "--format=%(refname)",
94 &format!("refs/collab/patches/{}/", id),
95 ],
96 );
97 for name in names.lines() {
98 git_in(bare, &["update-ref", "-d", name]);
99 }
100 git_in(bare, &["update-ref", &format!("refs/collab/patches/{}", id), &tip]);
101 }
102
103 /// A `repos_dir` with a `server.toml` beside it, and no server running: enough
104 /// for `git-collab-server migrate`, which needs the config only to find the
105 /// repositories.
106 struct Fixture {
107 root: TempDir,
108 repos_dir: PathBuf,
109 config: PathBuf,
110 }
111
112 impl Fixture {
113 fn new() -> Self {
114 let root = TempDir::new().unwrap();
115 let repos_dir = root.path().join("repos");
116 std::fs::create_dir_all(&repos_dir).unwrap();
117 let authorized_keys = root.path().join("authorized_keys");
118 std::fs::write(&authorized_keys, "").unwrap();
119 let config = root.path().join("server.toml");
120 std::fs::write(
121 &config,
122 format!(
123 "repos_dir = {:?}\nauthorized_keys = {:?}\n",
124 repos_dir, authorized_keys
125 ),
126 )
127 .unwrap();
128 Fixture {
129 root,
130 repos_dir,
131 config,
132 }
133 }
134
135 /// A hosted bare repository at `<repos_dir>/<relative>.git` holding one
136 /// patch in the pre-migration layout. Returns (bare path, patch id).
137 fn seed_old_layout(&self, relative: &str, title: &str) -> (PathBuf, String) {
138 let (bare, id) = self.seed_current_layout(relative, title);
139 demote_to_old_layout(&bare, &id);
140 (bare, id)
141 }
142
143 /// The same, left in the current layout: a repository `migrate` has
144 /// nothing to do to.
145 fn seed_current_layout(&self, relative: &str, title: &str) -> (PathBuf, String) {
146 let bare = self.repos_dir.join(format!("{relative}.git"));
147 std::fs::create_dir_all(bare.parent().unwrap()).unwrap();
148 git_in(
149 self.root.path(),
150 &["init", "-q", "--bare", "-b", "main", bare.to_str().unwrap()],
151 );
152
153 let work = TestRepo::new("Alice", "alice@example.com");
154 work.patch_create(title);
155 work.git(&["push", "-q", bare.to_str().unwrap(), "main:main"]);
156 work.git(&[
157 "push",
158 "-q",
159 bare.to_str().unwrap(),
160 "refs/collab/*:refs/collab/*",
161 ]);
162
163 let id = only_patch_id(&bare);
164 (bare, id)
165 }
166
167 fn migrate(&self, extra: &[&str]) -> Output {
168 let mut args = vec!["migrate", "--config", self.config.to_str().unwrap()];
169 args.extend_from_slice(extra);
170 Command::new(env!("CARGO_BIN_EXE_git-collab-server"))
171 .args(&args)
172 .output()
173 .expect("failed to run git-collab-server migrate")
174 }
175 }
176
177 fn combined(output: &Output) -> String {
178 format!(
179 "{}{}",
180 String::from_utf8_lossy(&output.stdout),
181 String::from_utf8_lossy(&output.stderr)
182 )
183 }
184
185 // ---------------------------------------------------------------------------
186 // The server must not migrate
187 // ---------------------------------------------------------------------------
188
189 /// The regression. One old-layout patch, every page that renders patches, and
190 /// the ref store byte-identical afterwards.
191 #[test]
192 fn serving_an_old_layout_repository_writes_nothing() {
193 let harness = ServerHarness::new("readonly-render");
194 harness
195 .work_repo()
196 .patch_create("A patch from before revision refs");
197 harness.push_head();
198 harness.push_collab_refs();
199
200 let bare = harness.repos_dir().join("readonly-render.git");
201 let id = only_patch_id(&bare);
202 demote_to_old_layout(&bare, &id);
203
204 let before = collab_refs(&bare);
205 let name = harness.repo_name();
206 for path in [
207 "/".to_string(),
208 format!("/{name}"),
209 format!("/{name}/patches"),
210 format!("/{name}/patches?filter=all"),
211 format!("/{name}/patches?filter=closed"),
212 format!("/{name}/patches?filter=merged"),
213 format!("/{name}/patches/{id}"),
214 format!("/{name}/issues"),
215 format!("/{name}/issues?filter=all"),
216 format!("/{name}/commits"),
217 ] {
218 harness.get_ok(&path);
219 }
220 let after = collab_refs(&bare);
221
222 assert_eq!(
223 before, after,
224 "serving pages rewrote the hosted repository's collab refs"
225 );
226 }
227
228 /// Not writing is worthless if it means not reading: the unmigrated patch has
229 /// to be on the page, in the list and on its own detail page.
230 #[test]
231 fn an_old_layout_patch_renders_without_being_migrated() {
232 let title = "A patch from before revision refs";
233 let harness = ServerHarness::new("readonly-visible");
234 harness.work_repo().patch_create(title);
235 harness.push_head();
236 harness.push_collab_refs();
237
238 let bare = harness.repos_dir().join("readonly-visible.git");
239 let id = only_patch_id(&bare);
240 demote_to_old_layout(&bare, &id);
241
242 let name = harness.repo_name();
243 let list = harness.get_ok(&format!("/{name}/patches?filter=all")).body;
244 assert!(
245 list.contains(title),
246 "the unmigrated patch is missing from the patch list:\n{list}"
247 );
248
249 let overview = harness.get_ok(&format!("/{name}")).body;
250 assert!(
251 overview.contains(title),
252 "the unmigrated patch is missing from the overview:\n{overview}"
253 );
254
255 let detail = harness.get_ok(&format!("/{name}/patches/{id}")).body;
256 assert!(
257 detail.contains(title),
258 "the unmigrated patch's detail page does not show it:\n{detail}"
259 );
260
261 // And still nothing was written to reach it.
262 assert!(
263 git_in(&bare, &["for-each-ref", "--format=%(refname)", "refs/collab/patches/"])
264 .lines()
265 .all(|name| name == format!("refs/collab/patches/{id}")),
266 "rendering the patch migrated it after all"
267 );
268 }
269
270 // ---------------------------------------------------------------------------
271 // git-collab-server migrate
272 // ---------------------------------------------------------------------------
273
274 #[test]
275 fn migrate_brings_an_old_layout_repository_up_to_date() {
276 let fixture = Fixture::new();
277 let (bare, id) = fixture.seed_old_layout("waystty", "An old patch");
278
279 let out = fixture.migrate(&[]);
280 let text = combined(&out);
281 assert!(
282 out.status.success(),
283 "migrate should succeed:\n{text}"
284 );
285 assert!(
286 text.contains("waystty") && text.contains("migrated 1 patch"),
287 "migrate did not report what it did to waystty:\n{text}"
288 );
289
290 let refs = collab_refs(&bare);
291 assert!(
292 refs.contains(&format!("refs/collab/patches/{id}/events")),
293 "the events ref was not created:\n{refs}"
294 );
295 assert!(
296 !refs.lines().any(|l| l.starts_with(&format!("refs/collab/patches/{id} "))),
297 "the old bare ref survived the migration:\n{refs}"
298 );
299 }
300
301 /// Nesting and dot-prefixed skips are the server's discovery rules, and
302 /// `migrate` has to walk by exactly the same ones or it will quietly leave a
303 /// repository behind for the upgrade to break.
304 #[test]
305 fn migrate_walks_nested_repositories_and_skips_dot_prefixed_ones() {
306 let fixture = Fixture::new();
307 let (nested, nested_id) = fixture.seed_old_layout("agents/claude-a", "A nested old patch");
308 let (hidden, hidden_id) = fixture.seed_old_layout(".private/secret", "A hidden old patch");
309
310 let out = fixture.migrate(&[]);
311 let text = combined(&out);
312 assert!(out.status.success(), "{text}");
313 assert!(
314 text.contains("agents/claude-a"),
315 "the nested repository was not reported by its full name:\n{text}"
316 );
317 assert!(
318 collab_refs(&nested).contains(&format!("refs/collab/patches/{nested_id}/events")),
319 "the nested repository was not migrated"
320 );
321 assert!(
322 !collab_refs(&hidden).contains(&format!("refs/collab/patches/{hidden_id}/events")),
323 "a dot-prefixed path is not a hosted repository and must not be touched"
324 );
325 }
326
327 #[test]
328 fn migrate_is_idempotent() {
329 let fixture = Fixture::new();
330 let (bare, _) = fixture.seed_old_layout("waystty", "An old patch");
331
332 assert!(fixture.migrate(&[]).status.success());
333 let after_first = collab_refs(&bare);
334
335 let out = fixture.migrate(&[]);
336 let text = combined(&out);
337 assert!(out.status.success(), "a second migrate should succeed:\n{text}");
338 assert!(
339 text.contains("already current"),
340 "a repository with nothing to do should say so:\n{text}"
341 );
342 assert_eq!(
343 after_first,
344 collab_refs(&bare),
345 "a second migrate changed the refs"
346 );
347 }
348
349 #[test]
350 fn migrate_reports_a_repository_it_cannot_write() {
351 let fixture = Fixture::new();
352 let (writable, writable_id) = fixture.seed_old_layout("writable", "A migratable patch");
353 let (blocked, blocked_id) = fixture.seed_old_layout("blocked", "An unmigratable patch");
354
355 make_read_only(&blocked);
356 if is_still_writable(&blocked) {
357 // Running as root, where mode bits mean nothing. Nothing to assert.
358 restore_write(&blocked);
359 eprintln!("skipping: this user can write a mode-0555 directory");
360 return;
361 }
362
363 let out = fixture.migrate(&[]);
364 let text = combined(&out);
365 restore_write(&blocked);
366
367 assert!(
368 !out.status.success(),
369 "migrate must exit non-zero when a repository could not be migrated:\n{text}"
370 );
371 assert!(
372 text.contains("blocked"),
373 "the unmigratable repository was not named:\n{text}"
374 );
375 assert!(
376 text.contains("not writable"),
377 "migrate did not say why the repository could not be migrated:\n{text}"
378 );
379 // A repository it cannot write must not stop the ones it can.
380 assert!(
381 collab_refs(&writable).contains(&format!("refs/collab/patches/{writable_id}/events")),
382 "one unwritable repository stopped the rest of the run"
383 );
384 let untouched = collab_refs(&blocked);
385 assert!(
386 !untouched.contains(&format!("refs/collab/patches/{blocked_id}/")),
387 "the unwritable repository was changed anyway:\n{untouched}"
388 );
389 }
390
391 #[test]
392 fn migrate_dry_run_changes_nothing() {
393 let fixture = Fixture::new();
394 let (bare, _) = fixture.seed_old_layout("waystty", "An old patch");
395 let before = collab_refs(&bare);
396
397 let out = fixture.migrate(&["--dry-run"]);
398 let text = combined(&out);
399 assert!(out.status.success(), "{text}");
400 assert!(
401 text.contains("would migrate 1 patch"),
402 "dry run did not say what it would change:\n{text}"
403 );
404 assert!(
405 text.contains("dry run"),
406 "dry run did not say it was a dry run:\n{text}"
407 );
408 assert_eq!(
409 before,
410 collab_refs(&bare),
411 "a dry run changed the repository"
412 );
413 }
414
415 /// A repository already in the current layout is reported, not skipped in
416 /// silence: an operator running this before an upgrade wants the whole roster
417 /// accounted for.
418 #[test]
419 fn migrate_reports_repositories_with_nothing_to_do() {
420 let fixture = Fixture::new();
421 fixture.seed_current_layout("current", "A current patch");
422
423 let out = fixture.migrate(&[]);
424 let text = combined(&out);
425 assert!(out.status.success(), "{text}");
426 assert!(
427 text.contains("current: already current"),
428 "a repository with nothing to do was not reported:\n{text}"
429 );
430 }
431
432 // ---------------------------------------------------------------------------
433 // Read-only mount simulation
434 // ---------------------------------------------------------------------------
435
436 fn make_read_only(dir: &Path) {
437 set_mode(dir, 0o555);
438 }
439
440 fn restore_write(dir: &Path) {
441 set_mode(dir, 0o755);
442 }
443
444 fn set_mode(dir: &Path, mode: u32) {
445 use std::os::unix::fs::PermissionsExt;
446 std::fs::set_permissions(dir, std::fs::Permissions::from_mode(mode)).unwrap();
447 }
448
449 fn is_still_writable(dir: &Path) -> bool {
450 let probe = dir.join(".write-probe");
451 match std::fs::write(&probe, b"x") {
452 Ok(()) => {
453 let _ = std::fs::remove_file(&probe);
454 true
455 }
456 Err(_) => false,
457 }
458 }