a73x

ae48e4fe

Strip pre-release compatibility, keeping the one unknown that is real

a73x   2026-08-14 06:11

Commit message
Strip pre-release compatibility, keeping the one unknown that is real

Every shape below was confirmed absent before it was removed, by
`git-collab refs` over this clone (legacy: 0) and a field-level audit of
all 271 event.json blobs under refs/collab/**: zero `commit: ""`, zero
reviews without a revision, zero `patch.revise`/`PatchCreate`/`head_commit`,
and zero cached states holding a singular `relates_to` or missing `labels`.

Removed:

- The ref-layout migration entire — migrate_patch_layout,
  resume_interrupted_migrations, plan_patch_layout_migration, the park
  namespace, and the `git-collab-server migrate` subcommand that existed
  only to drive it.
- The `''`-means-unknown convention for Revision.commit and the dedup
  guard that exempted it.
- event.rs legacy round-tripping: the `PatchCreate`/`patch.revise`
  variant aliases, the `head_commit` alias, and the empty-string defaults
  for commit/tree.
- PatchReview.revision as Option, and with it the attribution that
  recovered a review's revision from its position in the DAG — the one
  place derived state depended on DAG shape rather than signed content.
- The singular-string relates_to deserializer and PatchState.labels
  serde(default); cache::CACHE_FORMAT_VERSION was always the real guard
  for both, and is bumped to 9.
- resolve_head's fallback to refs/heads/<branch> and to an OID stored in
  the branch field. All 95 patch refs here resolve from a recorded
  revision commit; the fallback was the old addressing scheme, not a
  representation of an unknown head, and it let a patch with missing
  objects silently stand on whatever the branch of that name pointed at.

Kept, and re-documented as permanent rather than transitional:
Revision.base as Option and PatchState::effective_base. A revision
written before the field existed genuinely has no base, no migration can
invent one, and recomputing against the current tip yields the head
itself for a merged patch. Five revisions in this repository depend on
it. The RSA key-format handling in server/ssh/auth.rs is untouched — it
is OpenSSH interop, not old git-collab data.

A repository still holding a superseded shape now fails loudly instead of
being silently converted: the reader names the ref, says the layout is no
longer read, and points at `git-collab refs`, which deliberately still
classifies both shapes and is the one command that keeps working. sync
takes the same check before its first ref write, because it otherwise
reached git first and produced "could not remove directory ...: parent is
not directory" — naming no patch, no layout and no remedy.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
README.md
Old New
@@ -548,10 +548,12 @@ patch revision 9f8e7d6c5b4a refs/collab/patches/<id>/rev/9f8e7d6c5b4a...
548 2 collab refs: 1 patch, 1 revision. 548 2 collab refs: 1 patch, 1 revision.
549 ``` 549 ```
550 550
551 It takes `--json`, and it is the one command that never migrates the repository 551 It takes `--json`. Nothing reads the ref layouts written by pre-release
552 on the way past — so it can be trusted to report a layout an older version 552 versions any more: a command that meets one refuses and names it, and this is
553 wrote rather than quietly converting it. `git-collab-server refs --config 553 the command it points at — the one thing that still classifies those shapes, so
554 server.toml` answers the same question across a server's repositories. 554 you can see what a repository holds before deciding whether to re-fetch it or
555 delete the stale refs. `git-collab-server refs --config server.toml` answers the
556 same question across a server's repositories.
555 557
556 ## Status 558 ## Status
557 559
src/cache.rs
Old New
@@ -46,7 +46,13 @@ fn sanitize_ref_name(ref_name: &str) -> String {
46 /// that ref, which is exactly the "has unanswered feedback" signal the feature 46 /// that ref, which is exactly the "has unanswered feedback" signal the feature
47 /// exists to provide, served backwards. Same reasoning as v7; bump rather than 47 /// exists to provide, served backwards. Same reasoning as v7; bump rather than
48 /// trust the default. 48 /// trust the default.
49 const CACHE_FORMAT_VERSION: u32 = 8; 49 /// v9: pre-release compatibility was removed (issue `e5096ffc`). `Review::
50 /// revision` is no longer optional and `PatchState::labels` no longer defaults,
51 /// so an entry written before this would fail to deserialize rather than
52 /// deserialize wrongly — but the version is bumped anyway, because a hard
53 /// error on a cache hit is a worse way to discover a shape change than simply
54 /// not taking the hit.
55 const CACHE_FORMAT_VERSION: u32 = 9;
50 56
51 /// Cache entry stored on disk: the tip OID at cache time + serialized state. 57 /// Cache entry stored on disk: the tip OID at cache time + serialized state.
52 #[derive(serde::Serialize, serde::Deserialize)] 58 #[derive(serde::Serialize, serde::Deserialize)]
src/event.rs
Old New
@@ -52,30 +52,22 @@ pub enum Action {
52 IssueCommitLink { 52 IssueCommitLink {
53 commit: String, 53 commit: String,
54 }, 54 },
55 /// Patches created before revisions recorded their commit and tree omit 55 /// A patch, and the commit and tree revision 1 stands at.
56 /// both fields; an older shape still calls the whole variant `PatchCreate`
57 /// and puts the head in `head_commit` (sometimes a branch name, sometimes
58 /// a raw OID — `PatchState::resolve_head` handles either). Defaulting
59 /// `commit`/`tree` to empty keeps those patches readable; an empty commit
60 /// means "not recorded", never "the null OID".
61 /// 56 ///
62 /// Skipping the empty case on the way out matters as much as defaulting it 57 /// Pre-release shapes omitted `commit`/`tree` entirely, named the variant
63 /// on the way in: signatures are checked by re-serializing the event, so 58 /// `PatchCreate`, and put the head in `head_commit`. All three are gone
64 /// writing back a field the signer never wrote would invalidate every 59 /// (issue `e5096ffc`): every event in every repository we host was checked
65 /// legacy signature and `sync` would reject the ref. Nothing current code 60 /// for them first, and `commit` is now required, so a missing one is a
66 /// writes is ever empty, so this never fires for a new event. 61 /// deserialize error rather than a silent empty string meaning "unknown".
67 #[serde(rename = "patch.create", alias = "PatchCreate")] 62 #[serde(rename = "patch.create")]
68 PatchCreate { 63 PatchCreate {
69 title: String, 64 title: String,
70 body: String, 65 body: String,
71 base_ref: String, 66 base_ref: String,
72 #[serde(alias = "head_commit")]
73 branch: String, 67 branch: String,
74 #[serde(default, skip_serializing_if = "Option::is_none")] 68 #[serde(default, skip_serializing_if = "Option::is_none")]
75 fixes: Option<String>, 69 fixes: Option<String>,
76 #[serde(default, skip_serializing_if = "String::is_empty")]
77 commit: String, 70 commit: String,
78 #[serde(default, skip_serializing_if = "String::is_empty")]
79 tree: String, 71 tree: String,
80 /// Where revision 1 branched off `base_ref`: the merge-base of the 72 /// Where revision 1 branched off `base_ref`: the merge-base of the
81 /// base branch and `commit`, recorded when the patch is created. Named 73 /// base branch and `commit`, recorded when the patch is created. Named
@@ -84,42 +76,55 @@ pub enum Action {
84 #[serde(default, skip_serializing_if = "Option::is_none")] 76 #[serde(default, skip_serializing_if = "Option::is_none")]
85 base_commit: Option<String>, 77 base_commit: Option<String>,
86 }, 78 },
87 /// Historically written as `patch.revise` carrying only a note, before 79 /// A new revision of a patch: the commit and tree it now stands at.
88 /// revisions recorded the commit and tree they pointed at. Note that the 80 ///
89 /// variant rename is not reversible on serialization, so these events 81 /// Once written as `patch.revise` carrying only a note, before revisions
90 /// cannot round-trip for signature purposes however `commit`/`tree` are 82 /// recorded what they pointed at. That alias is gone (issue `e5096ffc`)
91 /// handled — see issue 2a79b3ab. 83 /// along with the empty-string default for `commit`/`tree`, having been
92 #[serde(rename = "patch.revision", alias = "patch.revise")] 84 /// confirmed absent everywhere first.
85 #[serde(rename = "patch.revision")]
93 PatchRevision { 86 PatchRevision {
94 #[serde(default, skip_serializing_if = "String::is_empty")]
95 commit: String, 87 commit: String,
96 #[serde(default, skip_serializing_if = "String::is_empty")]
97 tree: String, 88 tree: String,
98 #[serde(default, skip_serializing_if = "Option::is_none")] 89 #[serde(default, skip_serializing_if = "Option::is_none")]
99 body: Option<String>, 90 body: Option<String>,
100 /// Merge-base of the base branch and `commit` when this revision was 91 /// Merge-base of the base branch and `commit` when this revision was
101 /// recorded. `None` on revisions written before it was stored, and 92 /// recorded, or `None` for a revision that records no base.
102 /// skipped on the way out so those events still round-trip byte for 93 ///
103 /// byte for signature verification. 94 /// **Permanent, and not a migration leftover.** A revision written
95 /// before this field existed genuinely has no base: the merge-base was
96 /// never computed, and nothing can recover it after the fact —
97 /// recomputing against the branch as it stands now yields the head
98 /// itself for an already-merged patch, which reads as a base that never
99 /// moved. So "no recorded base" is a state this type has to be able to
100 /// express, five revisions in this project's own history are in it, and
101 /// `PatchState::effective_base` is how a reader copes.
102 ///
103 /// Skipped on the way out so an event that never carried the key still
104 /// re-serializes byte for byte, which is what signature verification
105 /// compares.
104 #[serde(default, skip_serializing_if = "Option::is_none")] 106 #[serde(default, skip_serializing_if = "Option::is_none")]
105 base: Option<String>, 107 base: Option<String>,
106 }, 108 },
107 /// `revision` is `None` on reviews written before reviews were scoped to a 109 /// A review, always scoped to the revision it reviewed.
108 /// revision. It is not the same as revision 1: `PatchState` attributes 110 ///
109 /// such a review to whichever revision was current when it was written, 111 /// `revision` was once optional, on reviews written before reviews were
110 /// which is what the vote-per-revision rule needs to keep successive 112 /// revision-scoped, and a reader recovered it from the event's position in
111 /// review rounds by one author from superseding each other. 113 /// the DAG. That was a weaker guarantee than the rest of the event carried
114 /// — position is not signed, and two clones holding the same events joined
115 /// in different parent orders could attribute the same review to different
116 /// revisions, which fed the vote-supersession rule and could drop a vote
117 /// rather than merely mislabel one (issue 33b5e541).
112 /// 118 ///
113 /// A `Some` revision is part of the signed payload. A `None` one is 119 /// Required now (issue `e5096ffc`), confirmed after checking every review
114 /// recovered from the event's position in the DAG, which is a weaker 120 /// event in every repository we host: the revision is part of the signed
115 /// guarantee — see the invariant documented at the attribution site in 121 /// payload, so attribution no longer depends on anything a rewrite could
116 /// `PatchState::from_ref_uncached`, and issue 33b5e541. 122 /// change.
117 #[serde(rename = "patch.review")] 123 #[serde(rename = "patch.review")]
118 PatchReview { 124 PatchReview {
119 verdict: ReviewVerdict, 125 verdict: ReviewVerdict,
120 body: String, 126 body: String,
121 #[serde(default, skip_serializing_if = "Option::is_none")] 127 revision: u32,
122 revision: Option<u32>,
123 }, 128 },
124 #[serde(rename = "patch.label")] 129 #[serde(rename = "patch.label")]
125 PatchLabel { label: String }, 130 PatchLabel { label: String },
src/lib.rs
Old New
@@ -209,29 +209,12 @@ fn report(
209 } 209 }
210 210
211 pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> { 211 pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
212 // The one place the CLI brings a legacy patch layout up to date, and the 212 // Every command used to convert a pre-release patch ref layout on the way
213 // reason it is here rather than inside `list_patches`: the CLI owns the 213 // past. Nothing does now (issue `e5096ffc`): the shapes were confirmed gone
214 // repository it was invoked in, so a write is its to make, whereas the 214 // from every repository we host, and a reader that meets one refuses and
215 // server does not own the repositories it serves and must never write 215 // says so rather than silently converting or silently skipping. `refs` is
216 // while rendering. Reads tolerate both layouts either way; writes cannot, 216 // the exception that still reports them, which is what the refusal points
217 // because `refs/collab/patches/<id>/rev/<oid>` and a bare 217 // at.
218 // `refs/collab/patches/<id>` are a directory/file conflict git refuses.
219 //
220 // Excluded: the `commit-msg` hook, which runs inside `git commit` and has
221 // one job — not to give git a reason to abort, and not to spray migration
222 // warnings into the middle of a commit.
223 //
224 // Excluded too, and for a different reason: `refs`, whose entire purpose is
225 // to report what shapes this repository holds. Migrating first would make
226 // it answer its own question — a repository with a legacy ref would be
227 // converted by the act of asking, and the answer would always be "none".
228 if !matches!(
229 cli.command,
230 Commands::Hooks(HookCmd::RunCommitMsg { .. }) | Commands::Refs { .. }
231 ) {
232 state::migrate_patch_layout(repo);
233 }
234
235 let is_write = cli.command.is_write(); 218 let is_write = cli.command.is_write();
236 match cli.command { 219 match cli.command {
237 Commands::Init => sync::init(repo), 220 Commands::Init => sync::init(repo),
@@ -766,11 +749,23 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
766 ); 749 );
767 } 750 }
768 } 751 }
769 Err(_) => { 752 Err(ref e) => {
770 println!("Head: unknown -> {}", p.base_ref); 753 println!("Head: unknown -> {}", p.base_ref);
771 println!("Branch: {}", p.branch); 754 println!("Branch: {}", p.branch);
755 // Said here, beside the line it explains, rather than
756 // only at the end: "unknown" on its own is what this
757 // used to print when the head silently resolved to
758 // whatever branch happened to share the patch's name.
759 eprintln!("warning: {}", e);
772 } 760 }
773 } 761 }
762 // Everything below reads from the DAG and is unaffected by a
763 // missing head, so it is printed either way — a patch whose
764 // objects were never fetched still has reviews and comments
765 // worth reading. The failure is carried to the exit code
766 // instead, so a script cannot mistake a partial answer for a
767 // whole one.
768 let head_failure = p.resolve_head(repo).err();
774 // The commit that landed the patch, printed next to the head it 769 // The commit that landed the patch, printed next to the head it
775 // replaced. For a squash there is no path from `Head` to the 770 // replaced. For a squash there is no path from `Head` to the
776 // base branch at all, so this line is the only thing that says 771 // base branch at all, so this line is the only thing that says
@@ -826,7 +821,7 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
826 let reviews: Vec<_> = if let Some(rev) = revision { 821 let reviews: Vec<_> = if let Some(rev) = revision {
827 p.reviews 822 p.reviews
828 .iter() 823 .iter()
829 .filter(|r| r.revision == Some(rev)) 824 .filter(|r| r.revision == rev)
830 .collect() 825 .collect()
831 } else { 826 } else {
832 p.reviews.iter().collect() 827 p.reviews.iter().collect()
@@ -834,8 +829,7 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
834 if !reviews.is_empty() { 829 if !reviews.is_empty() {
835 println!("\n--- Reviews ---"); 830 println!("\n--- Reviews ---");
836 for r in &reviews { 831 for r in &reviews {
837 let rev_label = 832 let rev_label = format!(" (r{})", r.revision);
838 r.revision.map(|n| format!(" (r{})", n)).unwrap_or_default();
839 println!( 833 println!(
840 "\n{} ({}) - {}{}{} [{:.8}]:\n{}", 834 "\n{} ({}) - {}{}{} [{:.8}]:\n{}",
841 r.author.name, 835 r.author.name,
@@ -949,7 +943,10 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
949 ); 943 );
950 } 944 }
951 } 945 }
952 Ok(()) 946 match head_failure {
947 Some(e) => Err(e),
948 None => Ok(()),
949 }
953 } 950 }
954 PatchCmd::Diff { 951 PatchCmd::Diff {
955 id, 952 id,
src/patch.rs
Old New
@@ -369,7 +369,7 @@ pub fn list_json(
369 /// revision at all, so filtering them by one would silently hide the whole 369 /// revision at all, so filtering them by one would silently hide the whole
370 /// discussion. The plain renderer makes the same choice. 370 /// discussion. The plain renderer makes the same choice.
371 fn only_revision(patch: &mut PatchState, revision: u32) { 371 fn only_revision(patch: &mut PatchState, revision: u32) {
372 patch.reviews.retain(|r| r.revision == Some(revision)); 372 patch.reviews.retain(|r| r.revision == revision);
373 patch 373 patch
374 .inline_comments 374 .inline_comments
375 .retain(|c| c.revision == Some(revision)); 375 .retain(|c| c.revision == Some(revision));
@@ -952,7 +952,7 @@ pub fn review(
952 // A different verdict is allowed and supersedes the previous vote. 952 // A different verdict is allowed and supersedes the previous vote.
953 if verdict.is_vote() { 953 if verdict.is_vote() {
954 let duplicate = patch.reviews.iter().any(|r| { 954 let duplicate = patch.reviews.iter().any(|r| {
955 r.verdict == verdict && r.author.email == author.email && r.revision == Some(rev) 955 r.verdict == verdict && r.author.email == author.email && r.revision == rev
956 }); 956 });
957 if duplicate { 957 if duplicate {
958 return Err(Error::Cmd(format!( 958 return Err(Error::Cmd(format!(
@@ -968,7 +968,7 @@ pub fn review(
968 action: Action::PatchReview { 968 action: Action::PatchReview {
969 verdict, 969 verdict,
970 body: body.to_string(), 970 body: body.to_string(),
971 revision: Some(rev), 971 revision: rev,
972 }, 972 },
973 clock: 0, 973 clock: 0,
974 }; 974 };
src/refs.rs
Old New
@@ -14,12 +14,17 @@
14 //! 14 //!
15 //! ## This is a read 15 //! ## This is a read
16 //! 16 //!
17 //! Nothing here writes, and the CLI excludes `refs` from the layout migration 17 //! Nothing here writes. That mattered when every other command converted a
18 //! it runs on entry to every other command. That is not tidiness. The question 18 //! pre-release layout on the way past — a command that migrated before it
19 //! this command exists to answer — *does this repository still hold a 19 //! looked could not answer *does this repository still hold a pre-migration
20 //! pre-migration shape?* — is unanswerable by a command that migrates before it 20 //! shape?* — and a server-side read that wrote to the repository it was reading
21 //! looks, and a server-side read that wrote to the repository it was reading was 21 //! was already one real bug (issue 5174338f).
22 //! already one real bug (issue 5174338f). 22 //!
23 //! The conversion is gone (issue `e5096ffc`), which leaves this module holding
24 //! the only description of those shapes that still exists. A reader that meets
25 //! one now refuses and names this command, so `refs` has to keep working
26 //! exactly where everything else stops: it is a read, it takes no lock, and it
27 //! never touches what it reports.
23 28
24 use std::collections::BTreeMap; 29 use std::collections::BTreeMap;
25 use std::io::Write; 30 use std::io::Write;
@@ -88,10 +93,13 @@ impl RefKind {
88 } 93 }
89 } 94 }
90 95
91 /// Whether this shape is one a migration converts. 96 /// Whether this shape is one written by a pre-release git-collab.
92 /// 97 ///
93 /// The single question blocking the removal of the compatibility code 98 /// Classifying these outlived the code that converted them, and that is the
94 /// (issue e5096ffc) is whether any repository anywhere still answers yes. 99 /// point: the migration was removed in issue `e5096ffc` once every
100 /// repository we host was confirmed clear of them, so nothing reads these
101 /// shapes any more. This is the only thing that still recognises one, which
102 /// makes it the diagnostic every refusal elsewhere points at.
95 pub fn is_legacy(self) -> bool { 103 pub fn is_legacy(self) -> bool {
96 matches!( 104 matches!(
97 self, 105 self,
@@ -308,8 +316,10 @@ pub fn render(refs: &[CollabRef], out: &mut impl Write) -> std::io::Result<()> {
308 if legacy > 0 { 316 if legacy > 0 {
309 writeln!( 317 writeln!(
310 out, 318 out,
311 "{} in a superseded layout ({}) — any other git-collab command in this \ 319 "{} in a superseded layout ({}) — no version still standing reads these, \
312 repository converts them.", 320 and every other git-collab command in this repository will refuse. \
321 Fetch the patch from a remote holding the current layout, or delete the \
322 stale ref if it exists nowhere else.",
313 legacy, 323 legacy,
314 legacy_breakdown(refs) 324 legacy_breakdown(refs)
315 )?; 325 )?;
src/server/http/repo/patches.rs
Old New
@@ -338,7 +338,7 @@ pub async fn patch_detail(
338 verdict: r.verdict.as_str().to_string(), 338 verdict: r.verdict.as_str().to_string(),
339 body: r.body, 339 body: r.body,
340 timestamp: Timestamp::new(r.timestamp), 340 timestamp: Timestamp::new(r.timestamp),
341 revision: r.revision, 341 revision: Some(r.revision),
342 edited: r.edited, 342 edited: r.edited,
343 }) 343 })
344 .collect(), 344 .collect(),
src/server/main.rs
Old New
@@ -6,7 +6,6 @@ use tracing::info;
6 mod config; 6 mod config;
7 mod governance; 7 mod governance;
8 mod http; 8 mod http;
9 mod migrate;
10 mod refs; 9 mod refs;
11 mod releases; 10 mod releases;
12 mod repos; 11 mod repos;
@@ -46,22 +45,6 @@ struct Args {
46 45
47 #[derive(Subcommand)] 46 #[derive(Subcommand)]
48 enum Command { 47 enum Command {
49 /// Bring hosted repositories' collab refs up to the current layout.
50 ///
51 /// Serving a repository never migrates it — rendering a page must not
52 /// write to the repository being rendered — so this is where an operator
53 /// converts a roster, deliberately, before an upgrade that drops support
54 /// for the old layout.
55 Migrate {
56 /// The same config the server runs with; only `repos_dir` is read.
57 #[arg(short, long)]
58 config: PathBuf,
59
60 /// Report what would change, and change nothing.
61 #[arg(long)]
62 dry_run: bool,
63 },
64
65 /// Report what collab refs the hosted repositories hold. 48 /// Report what collab refs the hosted repositories hold.
66 /// 49 ///
67 /// A read, and only a read: no lock, no writes, and a repository that 50 /// A read, and only a read: no lock, no writes, and a repository that
@@ -136,7 +119,6 @@ async fn main() {
136 // to serve. 119 // to serve.
137 if let Some(command) = args.command { 120 if let Some(command) = args.command {
138 let (path, dry_run, json) = match &command { 121 let (path, dry_run, json) = match &command {
139 Command::Migrate { config, dry_run } => (config, *dry_run, false),
140 Command::Refs { config, json } => (config, false, *json), 122 Command::Refs { config, json } => (config, false, *json),
141 Command::Setup { 123 Command::Setup {
142 config, dry_run, .. 124 config, dry_run, ..
@@ -150,7 +132,6 @@ async fn main() {
150 } 132 }
151 }; 133 };
152 std::process::exit(match command { 134 std::process::exit(match command {
153 Command::Migrate { .. } => migrate::run(&config.repos_dir, dry_run),
154 Command::Refs { .. } => refs::run(&config.repos_dir, json), 135 Command::Refs { .. } => refs::run(&config.repos_dir, json),
155 Command::Setup { 136 Command::Setup {
156 admin_key, 137 admin_key,
src/server/migrate.rs
Old New
@@ -1,209 +0,0 @@
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
@@ -52,27 +52,6 @@ fn deserialize_verdict<'de, D: serde::Deserializer<'de>>(d: D) -> Result<ReviewV
52 s.parse().map_err(serde::de::Error::custom) 52 s.parse().map_err(serde::de::Error::custom)
53 } 53 }
54 54
55 /// `relates_to` used to be a single `Option<String>`, written only at issue
56 /// creation. It is now a `Vec<String>` so an issue can relate to more than
57 /// one other issue. Cached state (and any other JSON on disk) written by an
58 /// older git-collab version still has the old shape, so this accepts a bare
59 /// string (-> one-element vec), `null`/absent (-> empty vec, via `#[serde(default)]`
60 /// on the field), or the current array shape.
61 #[derive(Deserialize)]
62 #[serde(untagged)]
63 enum RelatesToShape {
64 Many(Vec<String>),
65 One(String),
66 }
67
68 fn deserialize_relates_to<'de, D: serde::Deserializer<'de>>(d: D) -> Result<Vec<String>, D::Error> {
69 match Option::<RelatesToShape>::deserialize(d)? {
70 None => Ok(Vec::new()),
71 Some(RelatesToShape::Many(v)) => Ok(v),
72 Some(RelatesToShape::One(s)) => Ok(vec![s]),
73 }
74 }
75
76 /// One `BodyEdit` or `CommentDelete`, pending application. 55 /// One `BodyEdit` or `CommentDelete`, pending application.
77 struct BodyOverride { 56 struct BodyOverride {
78 /// `(clock, oid_hex)` of the requesting event. The same total order that 57 /// `(clock, oid_hex)` of the requesting event. The same total order that
@@ -289,7 +268,12 @@ pub struct IssueState {
289 #[serde(default)] 268 #[serde(default)]
290 pub last_updated: String, 269 pub last_updated: String,
291 pub author: Author, 270 pub author: Author,
292 #[serde(default, deserialize_with = "deserialize_relates_to")] 271 /// The issues this one relates to. Once a single `Option<String>` written
272 /// only at creation; the deserializer that still accepted that shape from a
273 /// stale fold cache went with issue `e5096ffc`, since
274 /// `cache::CACHE_FORMAT_VERSION` already rejects every entry old enough to
275 /// hold it.
276 #[serde(default)]
293 pub relates_to: Vec<String>, 277 pub relates_to: Vec<String>,
294 } 278 }
295 279
@@ -303,8 +287,11 @@ pub struct Review {
303 pub verdict: ReviewVerdict, 287 pub verdict: ReviewVerdict,
304 pub body: String, 288 pub body: String,
305 pub timestamp: String, 289 pub timestamp: String,
306 #[serde(default)] 290 /// The revision this review was cast against. Not optional: the event
307 pub revision: Option<u32>, 291 /// carries it, so every review has one (issue `e5096ffc`). It was optional
292 /// while reviews written before revision-scoping had to be attributed from
293 /// their position in the DAG — an attribution nothing signed.
294 pub revision: u32,
308 /// The OID of the `PatchReview` event, so the body can be corrected. A 295 /// The OID of the `PatchReview` event, so the body can be corrected. A
309 /// review's *verdict* is never edited — a reviewer changes their mind by 296 /// review's *verdict* is never edited — a reviewer changes their mind by
310 /// submitting a new review, which supersedes the old vote through the 297 /// submitting a new review, which supersedes the old vote through the
@@ -352,19 +339,29 @@ impl fmt::Display for PatchStatus {
352 #[derive(Debug, Clone, Serialize, Deserialize)] 339 #[derive(Debug, Clone, Serialize, Deserialize)]
353 pub struct Revision { 340 pub struct Revision {
354 pub number: u32, 341 pub number: u32,
355 /// Commit OID, or `""` when none was recorded — patches created before 342 /// Commit OID. Always recorded: the `""`-means-unknown convention that used
356 /// revisions carried one. Consumers of the JSON output must treat the 343 /// to sit here — for patches created before revisions carried a commit —
357 /// empty string as "unknown", not as an OID: it will not parse as one. 344 /// was removed in issue `e5096ffc` after confirming no event anywhere still
345 /// used it.
358 pub commit: String, 346 pub commit: String,
359 /// Tree OID, or `""` alongside an unrecorded `commit`. 347 /// Tree OID, recorded alongside `commit`.
360 pub tree: String, 348 pub tree: String,
361 pub body: Option<String>, 349 pub body: Option<String>,
362 pub timestamp: String, 350 pub timestamp: String,
363 /// Merge-base of the patch's base branch and `commit` when this revision 351 /// Merge-base of the patch's base branch and `commit` when this revision
364 /// was recorded. `None` for revisions written before it was stored; those 352 /// was recorded, or `None` for a revision that records no base.
365 /// fall back to recomputing it against the base branch as it stands now, 353 ///
366 /// which is what every patch used to do. "Did the author rebase between 354 /// **A real state, not legacy debt.** A revision written before this field
367 /// these two revisions" is exactly "did this change". 355 /// existed has no base and no migration can invent one: recomputing it
356 /// against the branch as it stands now yields the head itself once the
357 /// patch is merged, which reads as a base that never moved. Unknown is
358 /// therefore a permanent thing this type must be able to say — five
359 /// revisions in this project's own history say it — and
360 /// [`PatchState::effective_base`] is how a reader copes, by searching back
361 /// for the newest revision that did record one.
362 ///
363 /// "Did the author rebase between these two revisions" is exactly "did this
364 /// change", which is why an unknown base cannot simply be filled in.
368 #[serde(default)] 365 #[serde(default)]
369 pub base: Option<String>, 366 pub base: Option<String>,
370 /// The OID of the event that recorded this revision — the `PatchRevision`, 367 /// The OID of the event that recorded this revision — the `PatchRevision`,
@@ -530,19 +527,29 @@ pub struct PatchState {
530 pub base_ref: String, 527 pub base_ref: String,
531 pub fixes: Option<String>, 528 pub fixes: Option<String>,
532 /// The branch the patch was created from, recorded for provenance only. 529 /// The branch the patch was created from, recorded for provenance only.
533 /// Nothing resolves through it any more: a patch is addressed by its own 530 ///
534 /// revision refs, so an ephemeral or rewritten branch costs it nothing. 531 /// Nothing *resolves* through it: a patch is addressed by its own revision
532 /// refs, so an ephemeral or rewritten branch costs it nothing. That was
533 /// aspirational until issue `e5096ffc` removed `resolve_head`'s fallback to
534 /// `refs/heads/<branch>`; it is now literally true.
535 ///
536 /// Still read, so this is provenance and not dead weight: the `commit-msg`
537 /// hook matches HEAD's branch against open patches to decide what to stamp,
538 /// and `patch checkout` names the branch it creates. Both want "what was
539 /// this called", which is exactly what a provenance field is for — neither
540 /// treats it as an address. (Issue `659f0350` tracks the hook wanting a
541 /// better signal than a branch name now that patches are not addressed by
542 /// one.)
535 pub branch: String, 543 pub branch: String,
536 /// Postdates patch labelling: absent on any `PatchState` serialized 544 /// The labels folded onto this patch.
537 /// before this field existed, which must load as an empty vec. This 545 ///
538 /// `#[serde(default)]` is what makes such JSON deserialize at all; it is 546 /// The `#[serde(default)]` that let a `PatchState` serialized before this
539 /// not what keeps the on-disk fold cache correct. A stale cache entry 547 /// field existed load as an empty vec went with issue `e5096ffc`: it was
540 /// missing this key would deserialize just as cleanly whether or not it 548 /// never what kept the fold cache correct — an entry missing the key
541 /// predates a `patch.label` event actually folded into that ref -- so 549 /// deserializes just as cleanly whether or not it predates a `patch.label`
542 /// the cache still needs `cache::CACHE_FORMAT_VERSION` bumped alongside 550 /// event folded into that ref, which is why
543 /// this field to force a refold instead of quietly serving a 551 /// `cache::CACHE_FORMAT_VERSION` was bumped alongside the field in the
544 /// labels-empty hit. 552 /// first place. The version check is the real guard, and it is still there.
545 #[serde(default)]
546 pub labels: Vec<String>, 553 pub labels: Vec<String>,
547 pub comments: Vec<Comment>, 554 pub comments: Vec<Comment>,
548 pub inline_comments: Vec<InlineComment>, 555 pub inline_comments: Vec<InlineComment>,
@@ -1026,28 +1033,43 @@ impl PatchState {
1026 1033
1027 /// The commit the patch currently stands at: the latest revision whose 1034 /// The commit the patch currently stands at: the latest revision whose
1028 /// commit was recorded and whose objects are still present. A revision ref 1035 /// commit was recorded and whose objects are still present. A revision ref
1029 /// keeps those objects reachable, so this no longer depends on any 1036 /// keeps those objects reachable, so this does not depend on any
1030 /// `refs/heads/*` ref surviving a rebase. 1037 /// `refs/heads/*` ref surviving a rebase.
1031 /// 1038 ///
1032 /// The branch fallback is for patches old enough to have recorded no 1039 /// A patch is addressed by its own revision refs and by nothing else. This
1033 /// commits at all — either a branch name or, in the oldest shape, a raw OID 1040 /// used to fall back to resolving `refs/heads/<branch>` — and, in an older
1034 /// stored where the branch name now lives. 1041 /// shape still, a raw OID stored where the branch name now lives — because
1042 /// patches were once *addressed* by branch. That model is gone (issue
1043 /// `e5096ffc`), and the fallback with it: it is not a representation of an
1044 /// unknown head, it is the old addressing scheme, and leaving it in meant a
1045 /// patch whose objects were missing silently resolved to whatever the
1046 /// branch of that name happened to point at — a different commit than the
1047 /// patch ever recorded, and no way to tell from the output.
1048 ///
1049 /// So a head that cannot be found is an error and never a substitute. The
1050 /// message names the revision it could not resolve, because the cause is
1051 /// almost always objects that were never fetched rather than anything wrong
1052 /// with the patch.
1035 pub fn resolve_head(&self, repo: &Repository) -> Result<Oid, crate::error::Error> { 1053 pub fn resolve_head(&self, repo: &Repository) -> Result<Oid, crate::error::Error> {
1036 if let Some(oid) = self.latest_usable_commit(repo) { 1054 if let Some(oid) = self.latest_usable_commit(repo) {
1037 return Ok(oid); 1055 return Ok(oid);
1038 } 1056 }
1039 if let Ok(oid) = Oid::from_str(&self.branch) { 1057 let recorded = self
1040 if repo.find_commit(oid).is_ok() { 1058 .revisions
1041 return Ok(oid); 1059 .last()
1060 .map(|r| r.commit.as_str())
1061 .unwrap_or_default();
1062 Err(crate::error::Error::Cmd(format!(
1063 "patch {:.8} records revision commit {} but its objects are not in this \
1064 repository — fetch the patch's revision refs, or the commit it stands on, \
1065 before reading it",
1066 self.id,
1067 if recorded.is_empty() {
1068 "(none)"
1069 } else {
1070 recorded
1042 } 1071 }
1043 } 1072 )))
1044 let ref_name = format!("refs/heads/{}", self.branch);
1045 repo.refname_to_id(&ref_name).map_err(|e| {
1046 crate::error::Error::Cmd(format!(
1047 "patch has no recorded revision commit and branch '{}' was not found: {}",
1048 self.branch, e
1049 ))
1050 })
1051 } 1073 }
1052 1074
1053 /// Index of the newest revision whose commit was recorded and whose objects 1075 /// Index of the newest revision whose commit was recorded and whose objects
@@ -1070,13 +1092,23 @@ impl PatchState {
1070 /// The base the patch currently stands on, searching back from the revision 1092 /// The base the patch currently stands on, searching back from the revision
1071 /// the head came from for the newest one that recorded a base. 1093 /// the head came from for the newest one that recorded a base.
1072 /// 1094 ///
1073 /// The search matters because `base` is only written by revisions recorded 1095 /// **Permanent, not a migration workaround.** `Revision::base` is `None` on
1074 /// after this change: a migrated patch typically has one on its `PatchCreate` 1096 /// revisions written before it was stored, and that is a fact about those
1075 /// and none on the `PatchRevision` events after it. Reading only the newest 1097 /// revisions rather than a state something could convert them out of: the
1076 /// revision would call the base unknown for exactly those patches, and an 1098 /// merge-base was never computed and cannot be recovered afterwards.
1077 /// unknown base cannot be recovered by recomputing a merge-base against the 1099 /// Recomputing against the tip as it stands now is not a substitute —
1078 /// tip as it stands now — once the patch is merged that yields the head 1100 /// once the patch is merged it yields the head itself, which reads as a
1079 /// itself, which looks like a base that never moved. 1101 /// base that never moved, and that is the commonest case for a
1102 /// single-commit patch.
1103 ///
1104 /// So the search back is the answer, not a stopgap. A patch typically has a
1105 /// base on its `PatchCreate` and none on some `PatchRevision` events after
1106 /// it; reading only the newest revision would call the base unknown for
1107 /// exactly those. Five revisions in this project's own history depend on
1108 /// this, which is why the surrounding pre-release compatibility could be
1109 /// removed in issue `e5096ffc` and this could not.
1110 ///
1111 /// `None` only when no revision up to the head recorded a base at all.
1080 pub fn effective_base(&self, repo: &Repository) -> Option<Oid> { 1112 pub fn effective_base(&self, repo: &Repository) -> Option<Oid> {
1081 let end = self.latest_usable_index(repo)?; 1113 let end = self.latest_usable_index(repo)?;
1082 self.revisions[..=end] 1114 self.revisions[..=end]
@@ -1279,12 +1311,12 @@ impl PatchState {
1279 base, 1311 base,
1280 } => { 1312 } => {
1281 if let Some(ref mut s) = state { 1313 if let Some(ref mut s) = state {
1282 // Dedup by commit OID — skip if already seen. Legacy 1314 // Dedup by commit OID — skip if already seen. The guard
1283 // revisions recorded no commit, and collapsing those 1315 // that used to exempt an empty commit went with the
1284 // into one another would also collapse the review 1316 // `""`-means-unknown convention (issue `e5096ffc`);
1285 // rounds that sat between them. 1317 // every revision records a commit, so every revision
1286 let already_seen = 1318 // can be compared by it.
1287 !commit.is_empty() && s.revisions.iter().any(|r| r.commit == commit); 1319 let already_seen = s.revisions.iter().any(|r| r.commit == commit);
1288 if !already_seen { 1320 if !already_seen {
1289 overrides.note_owner(oid, &event.author); 1321 overrides.note_owner(oid, &event.author);
1290 let number = s.revisions.len() as u32 + 1; 1322 let number = s.revisions.len() as u32 + 1;
@@ -1308,42 +1340,24 @@ impl PatchState {
1308 revision, 1340 revision,
1309 } => { 1341 } => {
1310 if let Some(ref mut s) = state { 1342 if let Some(ref mut s) = state {
1311 // Reviews written before reviews were revision-scoped 1343 // The revision comes off the event, which means it is
1312 // carry no revision. Attribute them to the revision 1344 // covered by the event's signature.
1313 // that was current at this point in the walk, which is
1314 // where the reviewer was in fact looking.
1315 // 1345 //
1316 // INVARIANT: this is only well-defined because every 1346 // It used to be recoverable from the event's position
1317 // event that can reach this branch sits in a linear 1347 // in the walk when absent, for reviews written before
1318 // prefix of the DAG. Current code always writes 1348 // reviews were revision-scoped. That was the one place
1319 // `Some(n)`, so no new event extends the region where 1349 // where derived state depended on the DAG's *shape*
1320 // position matters, and no collab ref in the wild has 1350 // rather than its contents: signatures cover event
1321 // a merge commit. Walk order over a *forked* DAG is a 1351 // content but not parent links, and `Sort::TOPOLOGICAL`
1322 // partial order that `Sort::TOPOLOGICAL` completes by 1352 // completes a forked DAG's partial order by an internal
1323 // an internal tiebreak, so two clients holding the 1353 // tiebreak, so two clones holding the same events could
1324 // same events joined in different parent orders could 1354 // attribute one review to different revisions. Because
1325 // attribute the same review to different revisions. 1355 // attribution feeds the vote-supersession rule just
1326 // That is worse than a display bug: attribution feeds 1356 // below, that divergence could *drop* a vote rather
1327 // the vote-supersession rule below, so a divergence 1357 // than merely relabel it. Removing the fallback (issue
1328 // can drop a review rather than relabel one. For the 1358 // `e5096ffc`) removes that whole class of divergence;
1329 // same reason, note that signatures cover event 1359 // issue 33b5e541 tracks the remaining ordering work.
1330 // content but not parent links — a chain rewrite
1331 // (already unauthenticated) can now re-attribute
1332 // someone else's legacy review and delete a vote
1333 // through that same rule. The robust fix is to fold in
1334 // a total order over `(clock, oid)`, which is
1335 // content-derived and therefore client-independent, as
1336 // the status fold below already does: issue 33b5e541.
1337 // 1360 //
1338 // `state` is `Some` only after a PatchCreate, which
1339 // always seeds revision 1, so the list is never empty
1340 // here; `max(1)` guards a malformed DAG only.
1341 debug_assert!(
1342 !s.revisions.is_empty(),
1343 "PatchCreate always seeds revision 1"
1344 );
1345 let revision =
1346 revision.unwrap_or_else(|| (s.revisions.len() as u32).max(1));
1347 // A reviewer holds one current vote per revision: a new 1361 // A reviewer holds one current vote per revision: a new
1348 // vote supersedes their previous one. Comment-verdict 1362 // vote supersedes their previous one. Comment-verdict
1349 // reviews are not votes and accumulate. 1363 // reviews are not votes and accumulate.
@@ -1351,7 +1365,7 @@ impl PatchState {
1351 s.reviews.retain(|r| { 1365 s.reviews.retain(|r| {
1352 !(r.verdict.is_vote() 1366 !(r.verdict.is_vote()
1353 && r.author.email == event.author.email 1367 && r.author.email == event.author.email
1354 && r.revision == Some(revision)) 1368 && r.revision == revision)
1355 }); 1369 });
1356 } 1370 }
1357 overrides.note_owner(oid, &event.author); 1371 overrides.note_owner(oid, &event.author);
@@ -1360,7 +1374,7 @@ impl PatchState {
1360 verdict, 1374 verdict,
1361 body, 1375 body,
1362 timestamp: event.timestamp.clone(), 1376 timestamp: event.timestamp.clone(),
1363 revision: Some(revision), 1377 revision,
1364 commit_id: oid, 1378 commit_id: oid,
1365 edited: false, 1379 edited: false,
1366 }); 1380 });
@@ -1639,16 +1653,23 @@ fn refs_under(
1639 /// Enumerate the event DAG ref of every patch under a prefix, returning 1653 /// Enumerate the event DAG ref of every patch under a prefix, returning
1640 /// (ref_name, id) pairs. 1654 /// (ref_name, id) pairs.
1641 /// 1655 ///
1642 /// Both layouts, and reading only. In the current one the DAG is at 1656 /// One layout, and reading only: the DAG is at `<id>/events`, and the
1643 /// `<id>/events` and the refs beside it — `rev/<oid>`, or `r/<n>` from the 1657 /// `<id>/rev/<oid>` refs beside it are commits, not DAGs.
1644 /// draft that preceded it — are commits, not DAGs. In the pre-migration one 1658 ///
1645 /// the bare `<id>` ref *is* the DAG, and it is read where it lies. 1659 /// Two shapes written by pre-release versions used to be read here as well —
1660 /// the bare `<id>` ref that *was* the DAG, and the interim `<id>/r/<n>`
1661 /// revision numbering — and a migration converted them on the way past. Both
1662 /// are gone (issue `e5096ffc`), verified absent from every repository we host
1663 /// before the code was removed, so meeting one now is not a shape to
1664 /// interpret: it is a repository this version cannot read.
1646 /// 1665 ///
1647 /// Tolerating the old shape here rather than migrating on the way past is what 1666 /// It has to *say* so. The failure mode worth naming is not breakage but
1648 /// lets the server serve an unmigrated repository without writing to it. The 1667 /// silence: a reader that simply stopped recognising these names would skip
1649 /// two cannot coexist for one id — git will not have `<id>` be both a ref and a 1668 /// them and report a repository full of patches as empty, which is
1650 /// directory — so there is nothing to disambiguate, and everything downstream 1669 /// indistinguishable from a clean clone and impossible to diagnose from the
1651 /// takes the ref name it is handed. 1670 /// output. So this refuses, names the ref, and points at `git-collab refs` —
1671 /// which deliberately still classifies both shapes by name, and is the only
1672 /// command that can still be run here.
1652 fn patch_refs_under( 1673 fn patch_refs_under(
1653 repo: &Repository, 1674 repo: &Repository,
1654 prefix: &str, 1675 prefix: &str,
@@ -1660,9 +1681,18 @@ fn patch_refs_under(
1660 let ref_name = r.name().unwrap_or_default().to_string(); 1681 let ref_name = r.name().unwrap_or_default().to_string();
1661 let id = match split_patch_ref(&ref_name, prefix) { 1682 let id = match split_patch_ref(&ref_name, prefix) {
1662 Some((id, "events")) => id.to_string(), 1683 Some((id, "events")) => id.to_string(),
1684 // The interim revision numbering, from the draft the OID naming
1685 // replaced.
1686 Some((_, suffix)) if suffix.starts_with("r/") => {
1687 return Err(superseded_layout(&ref_name))
1688 }
1663 // A revision ref, not a patch. 1689 // A revision ref, not a patch.
1664 Some(_) => continue, 1690 Some(_) => continue,
1665 None => ref_name.strip_prefix(prefix).unwrap_or_default().to_string(), 1691 // The pre-migration layout: no suffix at all.
1692 None if !ref_name.strip_prefix(prefix).unwrap_or_default().is_empty() => {
1693 return Err(superseded_layout(&ref_name))
1694 }
1695 None => continue,
1666 }; 1696 };
1667 if id.is_empty() { 1697 if id.is_empty() {
1668 continue; 1698 continue;
@@ -1672,6 +1702,46 @@ fn patch_refs_under(
1672 Ok(result) 1702 Ok(result)
1673 } 1703 }
1674 1704
1705 /// Refuse early if this repository holds a patch ref in a superseded layout.
1706 ///
1707 /// [`patch_refs_under`] already refuses when something enumerates patches, but
1708 /// `sync` reaches git before it reaches that: it fetches into
1709 /// `refs/collab/sync/…` and then writes `<id>/events`, which against a local
1710 /// bare `<id>` is a directory-vs-file conflict git reports as
1711 /// `could not remove directory …: parent is not directory`. That names no
1712 /// patch, no layout and no remedy, and it is the error a contributor would
1713 /// actually have hit — so the check has to run *before* the first ref write,
1714 /// not merely somewhere on the read path.
1715 ///
1716 /// Both namespaces, because `close` moves a patch's whole subtree into
1717 /// `archive/` and a superseded shape moves with it.
1718 pub fn ensure_no_superseded_patch_refs(repo: &Repository) -> Result<(), crate::error::Error> {
1719 for prefix in [PATCH_PREFIX, ARCHIVE_PATCH_PREFIX] {
1720 patch_refs_under(repo, prefix)?;
1721 }
1722 Ok(())
1723 }
1724
1725 /// The error for a ref in a layout no version still standing can read.
1726 ///
1727 /// Written as advice rather than as a diagnosis because there is nothing the
1728 /// tool can do about it: the conversion that used to run here was removed once
1729 /// every repository we host had been through it, so the only ways out are from
1730 /// outside — take the current layout from a remote that has it, or drop the
1731 /// ref. Both are named, since which one applies depends on whether the patch
1732 /// exists anywhere else, and only the operator knows that.
1733 fn superseded_layout(ref_name: &str) -> crate::error::Error {
1734 crate::error::Error::Cmd(format!(
1735 "{} is in a patch ref layout written by an older git-collab, which this \
1736 version no longer reads.\n\
1737 Run `git-collab refs` to list every ref in a superseded layout — it is a \
1738 read and still works here.\n\
1739 To recover: fetch the patch from a remote holding the current layout, or \
1740 delete the stale ref if it exists nowhere else.",
1741 ref_name
1742 ))
1743 }
1744
1675 fn collab_refs( 1745 fn collab_refs(
1676 repo: &Repository, 1746 repo: &Repository,
1677 kind: &str, 1747 kind: &str,
@@ -1999,311 +2069,6 @@ pub fn delete_patch_refs(repo: &Repository, id: &str) -> Result<(), crate::error
1999 Ok(()) 2069 Ok(())
2000 } 2070 }
2001 2071
2002 /// Where a migration parks a patch's tip while it swaps the old ref for the
2003 /// new subtree. Outside the patch namespace so it cannot collide with either
2004 /// layout, and under `local/` so it is never pushed.
2005 const MIGRATION_PARK_PREFIX: &str = "refs/collab/local/migrating/patches/";
2006
2007 /// Finish, or clean up after, any migration that was interrupted partway.
2008 ///
2009 /// A migration deletes the old ref before it can create the events ref — git
2010 /// will not let `<id>` be both a ref and a directory, so the two cannot
2011 /// overlap. A kill in that window leaves the patch reachable only from the
2012 /// parked ref, where nothing else would ever look for it again: gone from
2013 /// `patch list`, `patch show` and the TUI, permanently, with its objects alive
2014 /// but unfindable. This is the only thing that makes parking worth anything.
2015 ///
2016 /// The later window — killed after the events ref exists but before the park
2017 /// was dropped — leaves the park behind as litter, and possibly a patch whose
2018 /// revisions were never pinned. Both come out of one loop, and the park is only
2019 /// dropped once the patch is whole.
2020 fn resume_interrupted_migrations(repo: &Repository, report: &mut MigrationReport) {
2021 let Ok(refs) = repo.references_glob(&format!("{}*", MIGRATION_PARK_PREFIX)) else {
2022 return;
2023 };
2024 let parked: Vec<(String, String)> = refs
2025 .filter_map(|r| {
2026 let name = r.ok()?.name()?.to_string();
2027 let id = name.strip_prefix(MIGRATION_PARK_PREFIX)?.to_string();
2028 Some((name, id))
2029 })
2030 .collect();
2031
2032 for (park_ref, id) in parked {
2033 // The park carries no namespace, so look for the patch in both.
2034 let existing = [PATCH_PREFIX, ARCHIVE_PATCH_PREFIX]
2035 .into_iter()
2036 .find(|p| repo.refname_to_id(&format!("{}{}/events", p, id)).is_ok());
2037
2038 let outcome = match existing {
2039 // The events ref landed, so only the revisions might be missing.
2040 // Pinning is idempotent, so re-running it costs nothing and covers
2041 // an interruption between the two steps.
2042 Some(prefix) => pin_dag_revisions(repo, &format!("{}{}/events", prefix, id), &id),
2043 // Resume from the parked tip. The active namespace is the right
2044 // home: a patch is only archived by an explicit close, and a close
2045 // moves the whole subtree, so nothing half-migrated belongs there.
2046 None => finish_migration(repo, PATCH_PREFIX, &id, &park_ref),
2047 };
2048 if let Err(e) = outcome {
2049 report.failed.push((
2050 id.clone(),
2051 format!(
2052 "could not resume interrupted migration of patch {:.8}: {}",
2053 id, e
2054 ),
2055 ));
2056 continue;
2057 }
2058 // Only now is the patch whole; the park has nothing left to protect.
2059 if let Ok(mut r) = repo.find_reference(&park_ref) {
2060 let _ = r.delete();
2061 }
2062 report.resumed.push(id);
2063 }
2064 }
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
2142 /// Bring patches written in the pre-revision-refs layout — a single ref at
2143 /// `refs/collab/patches/<id>` — up to the current one, printing a warning per
2144 /// patch it could not convert.
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`.
2151 ///
2152 /// Failures are reported and skipped rather than propagated: one unmigratable
2153 /// patch must not make the whole list unreadable.
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();
2163 // Before anything else: a patch stranded by an earlier interrupted run is
2164 // invisible to the scan below, because its old ref is already gone.
2165 resume_interrupted_migrations(repo, &mut report);
2166
2167 for prefix in [PATCH_PREFIX, ARCHIVE_PATCH_PREFIX] {
2168 let Ok(refs) = repo.references_glob(&format!("{}*", prefix)) else {
2169 continue;
2170 };
2171 // Collect first: the migration rewrites refs, and mutating the ref
2172 // store while iterating it is not sound.
2173 let old_layout: Vec<String> = refs
2174 .filter_map(|r| {
2175 let name = r.ok()?.name()?.to_string();
2176 // The new layout always has a suffix; the old one never does.
2177 match split_patch_ref(&name, prefix) {
2178 Some(_) => None,
2179 None => Some(name),
2180 }
2181 })
2182 .collect();
2183 for old_ref in old_layout {
2184 let id = old_ref.strip_prefix(prefix).unwrap_or_default().to_string();
2185 match migrate_one_patch(repo, prefix, &id, &old_ref) {
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 )),
2191 }
2192 }
2193
2194 // The interim shape: revision refs named by number, from the draft this
2195 // design replaced. Re-pin from the DAG under the OID names and retire
2196 // the numbered ones.
2197 let Ok(numbered) = repo.references_glob(&format!("{}*/r/*", prefix)) else {
2198 continue;
2199 };
2200 let mut ids: Vec<String> = numbered
2201 .filter_map(|r| {
2202 let name = r.ok()?.name()?.to_string();
2203 let (id, suffix) = split_patch_ref(&name, prefix)?;
2204 suffix.starts_with("r/").then(|| id.to_string())
2205 })
2206 .collect();
2207 ids.sort();
2208 ids.dedup();
2209 for id in ids {
2210 match retire_numbered_revision_refs(repo, prefix, &id) {
2211 Ok(()) => report.repinned.push(id),
2212 Err(e) => report.failed.push((
2213 id.clone(),
2214 format!(
2215 "could not convert numbered revision refs for patch {:.8}: {}",
2216 id, e
2217 ),
2218 )),
2219 }
2220 }
2221 }
2222 report
2223 }
2224
2225 /// Replace a patch's `r/<n>` refs with OID-named ones.
2226 ///
2227 /// The new refs are written before the old ones are dropped, so no commit is
2228 /// left unreferenced in between. A numbered ref pointing at a commit the DAG
2229 /// does not list is dropped too, and said so: it is exactly the wedge the OID
2230 /// naming removes — a published number nobody can renegotiate — and the DAG is
2231 /// the only thing that decides what a revision is.
2232 fn retire_numbered_revision_refs(
2233 repo: &Repository,
2234 prefix: &str,
2235 id: &str,
2236 ) -> Result<(), crate::error::Error> {
2237 let events_ref = format!("{}{}/events", prefix, id);
2238 if repo.refname_to_id(&events_ref).is_err() {
2239 return Err(git2::Error::from_str("no events ref beside numbered revision refs").into());
2240 }
2241 pin_dag_revisions(repo, &events_ref, id)?;
2242
2243 for (ref_name, suffix) in patch_subtree(repo, prefix, id)? {
2244 if !suffix.starts_with("r/") {
2245 continue;
2246 }
2247 if let Ok(oid) = repo.refname_to_id(&ref_name) {
2248 let pinned = patch_revision_ref(&events_ref, oid);
2249 if repo.refname_to_id(&pinned).is_err() {
2250 eprintln!(
2251 "warning: dropping {} — commit {:.8} is not a revision of patch {:.8}",
2252 ref_name, oid, id
2253 );
2254 }
2255 }
2256 repo.find_reference(&ref_name)?.delete()?;
2257 }
2258 Ok(())
2259 }
2260
2261 fn migrate_one_patch(
2262 repo: &Repository,
2263 prefix: &str,
2264 id: &str,
2265 old_ref: &str,
2266 ) -> Result<(), crate::error::Error> {
2267 let oid = repo.refname_to_id(old_ref)?;
2268
2269 // Read the revision list off the old ref before touching anything, so a
2270 // patch whose DAG will not materialize is left exactly as it was found.
2271 PatchState::from_ref_uncached(repo, old_ref, id)?;
2272
2273 // The old ref and the new subtree cannot coexist: git refuses to have
2274 // `<id>` be both a ref and a directory. Park the tip outside the patch
2275 // namespace first — `resume_interrupted_migrations` reads it back, which is
2276 // what makes the delete-then-create window survivable.
2277 let parked = format!("{}{}", MIGRATION_PARK_PREFIX, id);
2278 repo.reference(&parked, oid, false, "migrate patch: park tip")?;
2279 repo.find_reference(old_ref)?.delete()?;
2280
2281 finish_migration(repo, prefix, id, &parked)?;
2282
2283 repo.find_reference(&parked)?.delete()?;
2284 Ok(())
2285 }
2286
2287 /// Create the events ref from a parked tip and pin every recoverable revision.
2288 /// Callers must have established that no events ref exists yet; the caller that
2289 /// resumes an interrupted migration checks exactly that, and pins directly when
2290 /// one does.
2291 fn finish_migration(
2292 repo: &Repository,
2293 prefix: &str,
2294 id: &str,
2295 parked: &str,
2296 ) -> Result<(), crate::error::Error> {
2297 let oid = repo.refname_to_id(parked)?;
2298 let events_ref = format!("{}{}/events", prefix, id);
2299 repo.reference(&events_ref, oid, false, "migrate patch: events")?;
2300
2301 // Give every revision whose commit is still present a ref of its own.
2302 // Revisions whose objects were already lost to a force-push cannot be
2303 // recovered and keep the `""`-means-unknown convention.
2304 pin_dag_revisions(repo, &events_ref, id)
2305 }
2306
2307 /// Pin `commit` so the objects behind it stay reachable. The ref's name is its 2072 /// Pin `commit` so the objects behind it stay reachable. The ref's name is its
2308 /// content, so this is write-once for free: the only ref it can collide with is 2073 /// content, so this is write-once for free: the only ref it can collide with is
2309 /// one already pointing at the same commit. 2074 /// one already pointing at the same commit.
@@ -2367,10 +2132,12 @@ pub fn resolve_patch_ref(
2367 mod tests { 2132 mod tests {
2368 use super::*; 2133 use super::*;
2369 2134
2370 // `relates_to` used to be a single `Option<String>`. Cached state and any 2135 // `relates_to` was once a single `Option<String>`, and `IssueState` used to
2371 // other on-disk JSON written by older git-collab versions can still hold 2136 // accept that shape from on-disk JSON. That reader went with issue
2372 // that shape, so `IssueState` must keep reading it even though it now 2137 // `e5096ffc`: `cache::CACHE_FORMAT_VERSION` already rejects every entry old
2373 // materializes a `Vec<String>`. 2138 // enough to hold it, so the only thing the deserializer still had to read
2139 // is the current array — plus an absent field, which is not a legacy shape
2140 // but the ordinary "this issue relates to nothing".
2374 2141
2375 fn issue_json_with_relates_to(relates_to_json: &str) -> String { 2142 fn issue_json_with_relates_to(relates_to_json: &str) -> String {
2376 format!( 2143 format!(
@@ -2394,17 +2161,16 @@ mod tests {
2394 } 2161 }
2395 2162
2396 #[test] 2163 #[test]
2397 fn deserializes_legacy_singular_string_relates_to_as_one_element_vec() { 2164 fn the_superseded_singular_relates_to_is_no_longer_read() {
2165 // Asserted rather than merely deleted: silently accepting the old
2166 // shape again would resurrect a reader nothing else in the codebase
2167 // expects, and the cache version is what is supposed to be keeping
2168 // those entries out.
2398 let json = issue_json_with_relates_to(r#""def456""#); 2169 let json = issue_json_with_relates_to(r#""def456""#);
2399 let issue: IssueState = serde_json::from_str(&json).unwrap(); 2170 assert!(
2400 assert_eq!(issue.relates_to, vec!["def456".to_string()]); 2171 serde_json::from_str::<IssueState>(&json).is_err(),
2401 } 2172 "a singular relates_to is a pre-release shape and must not deserialize"
2402 2173 );
2403 #[test]
2404 fn deserializes_null_relates_to_as_empty_vec() {
2405 let json = issue_json_with_relates_to("null");
2406 let issue: IssueState = serde_json::from_str(&json).unwrap();
2407 assert_eq!(issue.relates_to, Vec::<String>::new());
2408 } 2174 }
2409 2175
2410 #[test] 2176 #[test]
@@ -2439,11 +2205,13 @@ mod tests {
2439 assert_eq!(issue.relates_to, Vec::<String>::new()); 2205 assert_eq!(issue.relates_to, Vec::<String>::new());
2440 } 2206 }
2441 2207
2442 // `labels` on `PatchState` postdates patch labelling: cached state (and 2208 // `labels` on `PatchState` postdates patch labelling, and used to default
2443 // any other on-disk JSON) written before this feature existed has no 2209 // so that state serialized before the feature existed still loaded. That
2444 // `labels` field at all. It must still deserialize, as an empty vec. 2210 // default went with issue `e5096ffc`; `cache::CACHE_FORMAT_VERSION` is what
2211 // actually keeps such entries out, and always was — the default could not
2212 // tell "no labels" from "labels folded in since this was cached".
2445 #[test] 2213 #[test]
2446 fn deserializes_missing_labels_field_as_empty_vec_on_patch_state() { 2214 fn a_patch_state_without_labels_is_no_longer_read() {
2447 let json = r#"{ 2215 let json = r#"{
2448 "id": "abc123", 2216 "id": "abc123",
2449 "title": "t", 2217 "title": "t",
@@ -2460,7 +2228,9 @@ mod tests {
2460 "last_updated": "", 2228 "last_updated": "",
2461 "author": {"name": "A", "email": "a@example.com"} 2229 "author": {"name": "A", "email": "a@example.com"}
2462 }"#; 2230 }"#;
2463 let patch: PatchState = serde_json::from_str(json).unwrap(); 2231 assert!(
2464 assert_eq!(patch.labels, Vec::<String>::new()); 2232 serde_json::from_str::<PatchState>(json).is_err(),
2233 "a PatchState with no labels field predates the feature and must not deserialize"
2234 );
2465 } 2235 }
2466 } 2236 }
src/sync.rs
Old New
@@ -627,6 +627,10 @@ fn run_local_scans(
627 /// stopping. The instruction still goes out — nothing recorded here is being 627 /// stopping. The instruction still goes out — nothing recorded here is being
628 /// shared with anyone — but as a closing note, not as a refusal. 628 /// shared with anyone — but as a closing note, not as a refusal.
629 fn sync_local_only(repo: &Repository) -> Result<(), Error> { 629 fn sync_local_only(repo: &Repository) -> Result<(), Error> {
630 // Same guard as the remote path: the local scans append events against
631 // patches, so a superseded layout stops this too, and says so.
632 state::ensure_no_superseded_patch_refs(repo)?;
633
630 // The scans append events, so this path takes the same advisory lock the 634 // The scans append events, so this path takes the same advisory lock the
631 // remote path does. Two syncs racing to record the same merge is exactly 635 // remote path does. Two syncs racing to record the same merge is exactly
632 // the case the lock exists for, and having no remote does not change it. 636 // the case the lock exists for, and having no remote does not change it.
@@ -637,11 +641,6 @@ fn sync_local_only(repo: &Repository) -> Result<(), Error> {
637 let author = get_author(repo)?; 641 let author = get_author(repo)?;
638 let sk = signing::load_signing_key(&signing::signing_key_dir()?)?; 642 let sk = signing::load_signing_key(&signing::signing_key_dir()?)?;
639 643
640 // Same reason the remote path migrates: a bare `<id>` patch ref predating
641 // the `<id>/events` layout is invisible to everything that folds patches,
642 // so a merge scan over it would silently find no patches to record against.
643 state::migrate_patch_layout(repo);
644
645 run_local_scans(repo, &author, &sk); 644 run_local_scans(repo, &author, &sk);
646 645
647 outln!("Local scan complete. Add a remote and run `git-collab init` to share these events."); 646 outln!("Local scan complete. Add a remote and run `git-collab init` to share these events.");
@@ -693,6 +692,11 @@ fn sync_remote(
693 remote_name: &str, 692 remote_name: &str,
694 warned: &mut TrustWarning, 693 warned: &mut TrustWarning,
695 ) -> Result<(), Error> { 694 ) -> Result<(), Error> {
695 // Before the lock and before any ref is touched: a repository still holding
696 // a superseded patch layout cannot be synced, and the raw git error it
697 // would otherwise produce names neither the layout nor the way out.
698 state::ensure_no_superseded_patch_refs(repo)?;
699
696 // Acquire advisory lock — held until _lock is dropped (RAII) 700 // Acquire advisory lock — held until _lock is dropped (RAII)
697 let _lock = SyncLock::acquire(repo)?; 701 let _lock = SyncLock::acquire(repo)?;
698 702
@@ -737,15 +741,6 @@ fn sync_remote(
737 // directory, which is the correct argument to `Repository::open`. 741 // directory, which is the correct argument to `Repository::open`.
738 let repo = Repository::open(repo.path())?; 742 let repo = Repository::open(repo.path())?;
739 743
740 // Migrate before reconciling or pushing. `sync` is precisely the command a
741 // contributor runs on a repo they have not otherwise touched, so it cannot
742 // rely on some earlier read having migrated: an incoming `<id>/events`
743 // would collide with a local bare `<id>` as a directory-vs-file lock
744 // conflict that aborts the whole patches reconcile, and the bare ref would
745 // be pushed outbound where a migrated remote must reject it for the same
746 // reason, with nothing to tell the user why.
747 state::migrate_patch_layout(&repo);
748
749 let sk = signing::load_signing_key(&signing::signing_key_dir()?)?; 744 let sk = signing::load_signing_key(&signing::signing_key_dir()?)?;
750 reconcile_refs(&repo, "issues", &author, &sk, warned)?; 745 reconcile_refs(&repo, "issues", &author, &sk, warned)?;
751 reconcile_refs(&repo, "patches", &author, &sk, warned)?; 746 reconcile_refs(&repo, "patches", &author, &sk, warned)?;
src/timeline.rs
Old New
@@ -280,7 +280,7 @@ pub fn build(repo: &Repository, id_prefix: &str) -> Result<(PatchState, Vec<Entr
280 body: r.body.clone(), 280 body: r.body.clone(),
281 edited: r.edited, 281 edited: r.edited,
282 }, 282 },
283 r.revision, 283 Some(r.revision),
284 ) 284 )
285 } 285 }
286 Action::PatchLabel { label } => ( 286 Action::PatchLabel { label } => (
src/tui/mod.rs
Old New
@@ -425,7 +425,7 @@ mod tests {
425 let action = Action::PatchReview { 425 let action = Action::PatchReview {
426 verdict: ReviewVerdict::Approve, 426 verdict: ReviewVerdict::Approve,
427 body: "lgtm".to_string(), 427 body: "lgtm".to_string(),
428 revision: Some(1), 428 revision: 1,
429 }; 429 };
430 assert_eq!(action_type_label(&action), "Patch Review"); 430 assert_eq!(action_type_label(&action), "Patch Review");
431 } 431 }
@@ -497,7 +497,7 @@ mod tests {
497 action: Action::PatchReview { 497 action: Action::PatchReview {
498 verdict: ReviewVerdict::Approve, 498 verdict: ReviewVerdict::Approve,
499 body: "Looks good!".to_string(), 499 body: "Looks good!".to_string(),
500 revision: Some(1), 500 revision: 1,
501 }, 501 },
502 clock: 0, 502 clock: 0,
503 }; 503 };
@@ -1129,7 +1129,7 @@ mod tests {
1129 verdict: ReviewVerdict::Approve, 1129 verdict: ReviewVerdict::Approve,
1130 body: "LGTM".into(), 1130 body: "LGTM".into(),
1131 timestamp: "2026-01-04T00:00:00Z".into(), 1131 timestamp: "2026-01-04T00:00:00Z".into(),
1132 revision: Some(2), 1132 revision: 2,
1133 commit_id: Oid::from_str("dddddddddddddddddddddddddddddddddddddddd").unwrap(), 1133 commit_id: Oid::from_str("dddddddddddddddddddddddddddddddddddddddd").unwrap(),
1134 edited: false, 1134 edited: false,
1135 }], 1135 }],
src/tui/widgets.rs
Old New
@@ -1131,10 +1131,7 @@ pub(crate) fn build_patch_detail_rows(app: &App) -> Vec<DetailRow> {
1131 ReviewVerdict::Comment => Color::White, 1131 ReviewVerdict::Comment => Color::White,
1132 ReviewVerdict::Reject => Color::Red, 1132 ReviewVerdict::Reject => Color::Red,
1133 }; 1133 };
1134 let rev_label = review 1134 let rev_label = format!(" (r{})", review.revision);
1135 .revision
1136 .map(|r| format!(" (r{})", r))
1137 .unwrap_or_default();
1138 rows.push(Line::from(vec![ 1135 rows.push(Line::from(vec![
1139 Span::styled( 1136 Span::styled(
1140 review.author.name.clone(), 1137 review.author.name.clone(),
tests/body_edit_test.rs
Old New
@@ -852,7 +852,7 @@ fn a_review_edit_by_a_different_author_is_ignored_by_the_fold() {
852 Action::PatchReview { 852 Action::PatchReview {
853 verdict: ReviewVerdict::Comment, 853 verdict: ReviewVerdict::Comment,
854 body: "Alice's review".to_string(), 854 body: "Alice's review".to_string(),
855 revision: Some(1), 855 revision: 1,
856 }, 856 },
857 ); 857 );
858 append( 858 append(
tests/collab_test.rs
Old New
@@ -660,10 +660,14 @@ fn add_commit_on_branch(
660 .unwrap() 660 .unwrap()
661 } 661 }
662 662
663 /// Create a branch-based patch using DAG primitives. The recorded commit is a 663 /// Create a patch on `branch` using DAG primitives, recording the branch's
664 /// placeholder that is not in the object database, so these patches exercise 664 /// current tip as revision 1's commit.
665 /// `resolve_head`'s legacy branch fallback — the path taken by patches old 665 ///
666 /// enough to have recorded no usable revision commit. 666 /// The recorded commit used to be a placeholder deliberately absent from the
667 /// object database, so that every patch built here exercised `resolve_head`'s
668 /// fallback to `refs/heads/<branch>`. That fallback is gone (issue
669 /// `e5096ffc`) — a patch is addressed by its own revision refs — so the commit
670 /// has to be real, and these patches now exercise the ordinary path.
667 fn create_branch_patch( 671 fn create_branch_patch(
668 repo: &git2::Repository, 672 repo: &git2::Repository,
669 author: &Author, 673 author: &Author,
@@ -672,6 +676,10 @@ fn create_branch_patch(
672 base_ref: &str, 676 base_ref: &str,
673 ) -> (String, String) { 677 ) -> (String, String) {
674 let sk = test_signing_key(); 678 let sk = test_signing_key();
679 let tip = repo
680 .refname_to_id(&format!("refs/heads/{}", branch))
681 .unwrap_or_else(|e| panic!("branch {} must exist: {}", branch, e));
682 let tree = repo.find_commit(tip).unwrap().tree().unwrap().id();
675 let event = Event { 683 let event = Event {
676 timestamp: now(), 684 timestamp: now(),
677 author: author.clone(), 685 author: author.clone(),
@@ -681,22 +689,12 @@ fn create_branch_patch(
681 base_ref: base_ref.to_string(), 689 base_ref: base_ref.to_string(),
682 branch: branch.to_string(), 690 branch: branch.to_string(),
683 fixes: None, 691 fixes: None,
684 commit: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa".to_string(), 692 commit: tip.to_string(),
685 tree: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb".to_string(), 693 tree: tree.to_string(),
686 base_commit: None, 694 base_commit: None,
687 }, 695 },
688 clock: 0, 696 clock: 0,
689 }; 697 };
690 // Load-bearing for test_resolve_head_branch_based and
691 // test_resolve_head_deleted_branch_error: they only test the legacy
692 // fallback because this commit cannot be resolved. Assert it rather than
693 // just documenting it, so an edit here cannot make both tests pass
694 // vacuously by giving them a real commit to find.
695 assert!(
696 repo.find_commit(git2::Oid::from_str("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa").unwrap())
697 .is_err(),
698 "the placeholder commit must not be in the object database"
699 );
700 let oid = dag::create_root_event(repo, &event, &sk).unwrap(); 698 let oid = dag::create_root_event(repo, &event, &sk).unwrap();
701 let id = oid.to_string(); 699 let id = oid.to_string();
702 let patch_ref = git_collab::state::patch_events_ref(&id); 700 let patch_ref = git_collab::state::patch_events_ref(&id);
@@ -710,16 +708,17 @@ fn create_branch_patch(
710 // --------------------------------------------------------------------------- 708 // ---------------------------------------------------------------------------
711 709
712 #[test] 710 #[test]
713 fn test_resolve_head_branch_based() { 711 fn test_resolve_head_follows_the_recorded_revision_not_the_branch() {
714 // Legacy fallback: with no usable revision commit recorded, the head is 712 // The contract that replaced the branch fallback: the head is the commit
715 // still read off `refs/heads/<branch>` and still follows it. 713 // the patch recorded, and moving the branch of the same name does not drag
714 // it. Under the old fallback a patch with no usable revision commit
715 // followed `refs/heads/<branch>` wherever it went, so a patch could silently
716 // come to stand on work it never contained.
716 let tmp = TempDir::new().unwrap(); 717 let tmp = TempDir::new().unwrap();
717 let repo = init_repo(tmp.path(), &alice()); 718 let repo = init_repo(tmp.path(), &alice());
718 719
719 // Create main branch and feature branch
720 make_initial_commit(&repo, "main"); 720 make_initial_commit(&repo, "main");
721 let feature_tip = add_commit_on_branch(&repo, "main", "feature.rs", b"fn feature() {}"); 721 let feature_tip = add_commit_on_branch(&repo, "main", "feature.rs", b"fn feature() {}");
722 // Create the feature branch at the current tip
723 repo.branch( 722 repo.branch(
724 "feature/test", 723 "feature/test",
725 &repo.find_commit(feature_tip).unwrap(), 724 &repo.find_commit(feature_tip).unwrap(),
@@ -730,21 +729,26 @@ fn test_resolve_head_branch_based() {
730 let (patch_ref, id) = 729 let (patch_ref, id) =
731 create_branch_patch(&repo, &alice(), "Test patch", "feature/test", "main"); 730 create_branch_patch(&repo, &alice(), "Test patch", "feature/test", "main");
732 let state = PatchState::from_ref(&repo, &patch_ref, &id).unwrap(); 731 let state = PatchState::from_ref(&repo, &patch_ref, &id).unwrap();
732 assert_eq!(state.resolve_head(&repo).unwrap(), feature_tip);
733 733
734 // resolve_head should return the branch tip 734 // Advance the branch. The patch recorded no new revision, so its head must
735 let resolved = state.resolve_head(&repo).unwrap(); 735 // not move with it.
736 assert_eq!(resolved, feature_tip);
737
738 // Now add a commit to the branch — resolve_head should return the new tip
739 let new_tip = add_commit_on_branch(&repo, "feature/test", "more.rs", b"more code"); 736 let new_tip = add_commit_on_branch(&repo, "feature/test", "more.rs", b"more code");
740 let resolved2 = state.resolve_head(&repo).unwrap(); 737 assert_ne!(new_tip, feature_tip);
741 assert_eq!(resolved2, new_tip); 738 let state = PatchState::from_ref(&repo, &patch_ref, &id).unwrap();
739 assert_eq!(
740 state.resolve_head(&repo).unwrap(),
741 feature_tip,
742 "the head is the recorded revision commit, not wherever the branch has got to"
743 );
742 } 744 }
743 745
744 #[test] 746 #[test]
745 fn test_resolve_head_deleted_branch_error() { 747 fn test_resolve_head_survives_the_branch_being_deleted() {
746 // Only for patches on the legacy fallback: one with a revision ref has 748 // The other half of the same change, and the reason it is an improvement:
747 // somewhere else to look, and no longer cares that the branch is gone. 749 // a patch used to become unreadable when its branch was deleted, because
750 // the branch was the only way to find its head. An ephemeral worktree
751 // branch now costs the patch nothing.
748 let tmp = TempDir::new().unwrap(); 752 let tmp = TempDir::new().unwrap();
749 let repo = init_repo(tmp.path(), &alice()); 753 let repo = init_repo(tmp.path(), &alice());
750 754
@@ -756,17 +760,16 @@ fn test_resolve_head_deleted_branch_error() {
756 let (patch_ref, id) = 760 let (patch_ref, id) =
757 create_branch_patch(&repo, &alice(), "Ephemeral patch", "ephemeral", "main"); 761 create_branch_patch(&repo, &alice(), "Ephemeral patch", "ephemeral", "main");
758 762
759 // Delete the branch 763 repo.find_branch("ephemeral", git2::BranchType::Local)
760 let mut branch = repo 764 .unwrap()
761 .find_branch("ephemeral", git2::BranchType::Local) 765 .delete()
762 .unwrap(); 766 .unwrap();
763 branch.delete().unwrap();
764 767
765 let state = PatchState::from_ref(&repo, &patch_ref, &id).unwrap(); 768 let state = PatchState::from_ref(&repo, &patch_ref, &id).unwrap();
766 let result = state.resolve_head(&repo); 769 assert_eq!(
767 assert!( 770 state.resolve_head(&repo).unwrap(),
768 result.is_err(), 771 tip,
769 "resolve_head should error when branch is deleted" 772 "the recorded revision commit outlives the branch it was made on"
770 ); 773 );
771 } 774 }
772 775
@@ -947,8 +950,11 @@ fn test_patch_show_outdated_staleness() {
947 // --------------------------------------------------------------------------- 950 // ---------------------------------------------------------------------------
948 951
949 #[test] 952 #[test]
950 fn test_patch_create_with_head_commit_field_deserializes() { 953 fn test_patch_create_with_head_commit_field_is_no_longer_read() {
951 // Events created with "head_commit" instead of "branch" should still work 954 // `head_commit` was the pre-release spelling of `branch`. The alias that
955 // accepted it went with issue `e5096ffc`, so such an event no longer
956 // deserializes at all — asserted rather than deleted, because silently
957 // accepting it again would reintroduce a shape nothing else expects.
952 let json = r#"{ 958 let json = r#"{
953 "timestamp": "2026-03-21T00:00:00+00:00", 959 "timestamp": "2026-03-21T00:00:00+00:00",
954 "author": {"name": "agent", "email": "agent@test"}, 960 "author": {"name": "agent", "email": "agent@test"},
@@ -962,13 +968,10 @@ fn test_patch_create_with_head_commit_field_deserializes() {
962 "tree": "def456" 968 "tree": "def456"
963 } 969 }
964 }"#; 970 }"#;
965 let event: Event = serde_json::from_str(json).unwrap(); 971 assert!(
966 match event.action { 972 serde_json::from_str::<Event>(json).is_err(),
967 Action::PatchCreate { branch, .. } => { 973 "head_commit is a pre-release spelling and must not deserialize"
968 assert_eq!(branch, "abc123def456"); 974 );
969 }
970 _ => panic!("expected PatchCreate"),
971 }
972 } 975 }
973 976
974 #[test] 977 #[test]
@@ -996,15 +999,19 @@ fn test_patch_create_with_branch_field_still_works() {
996 } 999 }
997 1000
998 #[test] 1001 #[test]
999 fn test_resolve_head_with_oid_string() { 1002 fn test_an_oid_in_the_branch_field_is_not_resolved_as_a_head() {
1000 // If branch field contains a hex OID, resolve_head should try to parse it as an OID 1003 // The oldest shape of all put a raw OID where the branch name now lives,
1004 // and `resolve_head` used to parse it and use it. That went with the branch
1005 // fallback (issue `e5096ffc`): `branch` is provenance, never an address, so
1006 // a patch whose recorded revision commit is missing is an error rather than
1007 // an excuse to go looking in a second place.
1001 let tmp = TempDir::new().unwrap(); 1008 let tmp = TempDir::new().unwrap();
1002 let repo = init_repo(tmp.path(), &alice()); 1009 let repo = init_repo(tmp.path(), &alice());
1003 make_initial_commit(&repo, "main"); 1010 make_initial_commit(&repo, "main");
1004 let tip = add_commit_on_branch(&repo, "main", "f.rs", b"content"); 1011 let tip = add_commit_on_branch(&repo, "main", "f.rs", b"content");
1005 1012
1006 // Create a patch where "branch" is actually a commit OID string
1007 let sk = test_signing_key(); 1013 let sk = test_signing_key();
1014 let absent = "a".repeat(40);
1008 let event = Event { 1015 let event = Event {
1009 timestamp: now(), 1016 timestamp: now(),
1010 author: alice(), 1017 author: alice(),
@@ -1012,10 +1019,11 @@ fn test_resolve_head_with_oid_string() {
1012 title: "OID-based patch".to_string(), 1019 title: "OID-based patch".to_string(),
1013 body: "".to_string(), 1020 body: "".to_string(),
1014 base_ref: "main".to_string(), 1021 base_ref: "main".to_string(),
1022 // A real commit, in the field that is no longer an address.
1015 branch: tip.to_string(), 1023 branch: tip.to_string(),
1016 fixes: None, 1024 fixes: None,
1017 commit: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa".to_string(), 1025 commit: absent.clone(),
1018 tree: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb".to_string(), 1026 tree: "b".repeat(40),
1019 base_commit: None, 1027 base_commit: None,
1020 }, 1028 },
1021 clock: 0, 1029 clock: 0,
@@ -1026,8 +1034,15 @@ fn test_resolve_head_with_oid_string() {
1026 repo.reference(&ref_name, oid, false, "test").unwrap(); 1034 repo.reference(&ref_name, oid, false, "test").unwrap();
1027 1035
1028 let state = PatchState::from_ref(&repo, &ref_name, &id).unwrap(); 1036 let state = PatchState::from_ref(&repo, &ref_name, &id).unwrap();
1029 let resolved = state.resolve_head(&repo).unwrap(); 1037 let err = state
1030 assert_eq!(resolved, tip); 1038 .resolve_head(&repo)
1039 .expect_err("an OID in `branch` must not stand in for a missing revision commit");
1040 let message = err.to_string();
1041 assert!(
1042 message.contains(&absent),
1043 "the error must name the revision commit it could not find, got: {}",
1044 message
1045 );
1031 } 1046 }
1032 1047
1033 // --------------------------------------------------------------------------- 1048 // ---------------------------------------------------------------------------
tests/common/mod.rs
Old New
@@ -430,11 +430,11 @@ pub fn init_repo(dir: &Path, author: &Author) -> Repository {
430 /// `parent` as its only parent (`None` produces an orphan root commit). 430 /// `parent` as its only parent (`None` produces an orphan root commit).
431 /// Returns the new commit OID. The caller owns the ref. 431 /// Returns the new commit OID. The caller owns the ref.
432 /// 432 ///
433 /// Historical on-disk shapes — `patch.revise`, reviews with no `revision`, 433 /// For events that cannot be produced by serializing today's `Action` at all:
434 /// creates with no `commit`/`tree` — can no longer be produced by serializing 434 /// pre-release shapes such as `patch.revise` or a `patch.create` with no
435 /// today's `Action`, so tests covering them write the JSON directly. The 435 /// `commit`, and events pinned to refs a test wants to place by hand. The
436 /// commit tree and its ed25519 signature are built exactly as `dag` builds 436 /// commit tree and its ed25519 signature are built exactly as `dag` builds
437 /// them, so the result is indistinguishable from a real historical event. 437 /// them, so the result is indistinguishable from a real event.
438 pub fn write_raw_event( 438 pub fn write_raw_event(
439 repo: &Repository, 439 repo: &Repository,
440 parent: Option<git2::Oid>, 440 parent: Option<git2::Oid>,
@@ -615,7 +615,7 @@ pub fn add_review_on(
615 action: Action::PatchReview { 615 action: Action::PatchReview {
616 verdict, 616 verdict,
617 body: "review comment".to_string(), 617 body: "review comment".to_string(),
618 revision: Some(revision), 618 revision,
619 }, 619 },
620 clock: 0, 620 clock: 0,
621 }; 621 };
@@ -1119,7 +1119,7 @@ impl ServerHarness {
1119 } 1119 }
1120 1120
1121 /// The config file the running server was started with, for subcommands 1121 /// The config file the running server was started with, for subcommands
1122 /// an operator runs against the same server (`migrate`, `setup`). 1122 /// an operator runs against the same server (`refs`, `setup`).
1123 pub fn config_path(&self) -> PathBuf { 1123 pub fn config_path(&self) -> PathBuf {
1124 self.root.path().join("server.toml") 1124 self.root.path().join("server.toml")
1125 } 1125 }
tests/fixtures/legacy_events/patch_create_head_commit_variant.json
Old New
@@ -1,14 +0,0 @@
1 {
2 "timestamp": "2026-03-21T15:42:16.382345234+00:00",
3 "author": {
4 "name": "a73x",
5 "email": "dev@a73x.sh"
6 },
7 "action": {
8 "type": "PatchCreate",
9 "title": "Add --json output flag to list/show commands",
10 "body": "Fixes 79125c77. Adds --json flag to issue list, issue show, patch list, patch show for machine-parseable output in agent workflows.",
11 "base_ref": "main",
12 "head_commit": "db059e663d7a435f288bd2832bee4e9f374474d2"
13 }
14 }
14 \ No newline at end of file \ No newline at end of file
tests/fixtures/legacy_events/patch_create_missing_commit_tree.json
Old New
@@ -1,16 +0,0 @@
1 {
2 "timestamp": "2026-03-21T19:08:48.630385084+00:00",
3 "author": {
4 "name": "a73x",
5 "email": "dev@a73x.sh"
6 },
7 "action": {
8 "type": "patch.create",
9 "title": "Avoid double DAG walk after auto_detect_revision",
10 "body": "",
11 "base_ref": "main",
12 "branch": "worktree-agent-aef11cfd",
13 "fixes": "53e698d5"
14 },
15 "clock": 1
16 }
16 \ No newline at end of file \ No newline at end of file
tests/fixtures/legacy_events/patch_review_missing_revision.json
Old New
@@ -1,13 +0,0 @@
1 {
2 "timestamp": "2026-03-21T19:09:05.299717785+00:00",
3 "author": {
4 "name": "a73x",
5 "email": "dev@a73x.sh"
6 },
7 "action": {
8 "type": "patch.review",
9 "verdict": "RequestChanges",
10 "body": "Good approach returning Option<Revision> instead of u32 — that's the right move. But you've introduced the same 3-line pattern in comment(), review(), and merge():\n\n let mut patch = patch;\n if let Some(rev) = auto_detect_revision(repo, &ref_name, &patch, &sk)? {\n patch.revisions.push(rev);\n }\n\nExtract that into a small helper, e.g. `auto_detect_and_update(repo, ref_name, &mut patch, &sk)?` that does the detect + push in one call. That way callers just need one line. Also avoids the awkward `let mut patch = patch;` rebinding."
11 },
12 "clock": 2
13 }
13 \ No newline at end of file \ No newline at end of file
tests/fixtures/legacy_events/patch_review_with_revision.json
Old New
@@ -1,14 +0,0 @@
1 {
2 "timestamp": "2026-03-22T10:03:14.967814484+00:00",
3 "author": {
4 "name": "a73x",
5 "email": "dev@a73x.sh"
6 },
7 "action": {
8 "type": "patch.review",
9 "verdict": "Approve",
10 "body": "LGTM — confirmed no stale .clone() calls remain. Good to merge.",
11 "revision": 2
12 },
13 "clock": 5
14 }
14 \ No newline at end of file \ No newline at end of file
tests/fixtures/legacy_events/patch_revise_body_only.json
Old New
@@ -1,12 +0,0 @@
1 {
2 "timestamp": "2026-03-21T19:10:36.869240870+00:00",
3 "author": {
4 "name": "a73x",
5 "email": "dev@a73x.sh"
6 },
7 "action": {
8 "type": "patch.revise",
9 "body": "Address review: extract auto_detect_and_update helper to eliminate 3x repeated pattern"
10 },
11 "clock": 3
12 }
12 \ No newline at end of file \ No newline at end of file
tests/interdiff_test.rs
Old New
@@ -332,7 +332,12 @@ fn patch_diff_survives_the_patch_being_merged() {
332 } 332 }
333 333
334 // =========================================================================== 334 // ===========================================================================
335 // Migration: revisions with no recorded base degrade, they do not fail 335 // Revisions with no recorded base degrade, they do not fail
336 //
337 // Not a migration case and never was: a revision written before `base` was
338 // stored genuinely has none, nothing can recover it, and "unknown" is a state
339 // this tool has to be able to express and say out loud. Issue `e5096ffc`
340 // stripped the pre-release compatibility around it and deliberately kept this.
336 // =========================================================================== 341 // ===========================================================================
337 342
338 #[test] 343 #[test]
@@ -374,8 +379,15 @@ fn revisions_without_a_recorded_base_still_diff_and_say_the_base_is_unknown() {
374 2, 379 2,
375 ); 380 );
376 let id = root.to_string(); 381 let id = root.to_string();
377 repo.reference(&format!("refs/collab/patches/{}", id), tip, false, "legacy") 382 // The current ref layout. What makes this fixture the case under test is
378 .unwrap(); 383 // that neither event carries a `base` key — not the shape of its refs.
384 repo.reference(
385 &format!("refs/collab/patches/{}/events", id),
386 tip,
387 false,
388 "events",
389 )
390 .unwrap();
379 drop(repo); 391 drop(repo);
380 392
381 let out = work.run_ok(&["patch", "diff", &id[..8], "--between", "1", "2"]); 393 let out = work.run_ok(&["patch", "diff", &id[..8], "--between", "1", "2"]);
tests/legacy_patch_test.rs
Old New
@@ -1,424 +1,257 @@
1 //! Recovery of patches written by older versions of git-collab. 1 //! What happens to a repository still holding a pre-release shape.
2 //! 2 //!
3 //! Between them, the historical shapes drop `commit`/`tree` from 3 //! Until issue `e5096ffc` this file asserted that such repositories were read
4 //! `patch.create`, name the revision event `patch.revise`, and omit 4 //! and silently converted. Every shape it covered — the bare
5 //! `revision` from `patch.review`. Every event JSON asserted on here was 5 //! `refs/collab/patches/<id>` layout, the interim `<id>/r/<n>` revision
6 //! captured verbatim from a real patch in this repository's own collab refs 6 //! numbering, `patch.revise`, `head_commit`, reviews with no `revision`,
7 //! (`tests/fixtures/legacy_events/`). 7 //! revisions with no `commit` — is now gone from the reader, confirmed absent
8 //! from every repository we host before it was removed.
9 //!
10 //! So the contract this file asserts is the opposite one, and it is the whole
11 //! reason the removal is safe to ship: a repository that *does* still hold one
12 //! of those shapes must say so, name the shape, and say what to do about it.
13 //! The failure mode being guarded against is not breakage — breakage is
14 //! intended — it is a silent empty list, or an error about something else.
15 //! `git-collab refs` deliberately still classifies these shapes by name, and is
16 //! the diagnostic every message here points at.
8 17
9 mod common; 18 mod common;
10 19
11 use std::path::PathBuf; 20 use common::{write_raw_event, TestRepo};
12
13 use common::{test_signing_key, write_raw_event, ServerHarness, TestRepo};
14 use git_collab::event::{Action, Event, ReviewVerdict};
15 use git_collab::signing::{self, DetachedSignature, VerifyStatus};
16 use git_collab::state::{PatchState, PatchStatus};
17 use serde_json::json; 21 use serde_json::json;
18 22
19 fn fixture(name: &str) -> String { 23 /// Create a patch on a fresh branch holding one commit. Returns its full id.
20 let path = PathBuf::from(env!("CARGO_MANIFEST_DIR")) 24 fn patch_on_branch(repo: &TestRepo, branch: &str, file: &str) -> String {
21 .join("tests/fixtures/legacy_events") 25 repo.git(&["checkout", "-b", branch]);
22 .join(name); 26 repo.commit_file(file, "v1", &format!("add {}", file));
23 std::fs::read_to_string(&path) 27 let out = repo.run_ok(&["patch", "create", "-t", branch, "-B", branch]);
24 .unwrap_or_else(|e| panic!("reading fixture {}: {}", path.display(), e)) 28 let short = out
25 } 29 .trim()
26 30 .strip_prefix("Created patch ")
27 /// Sign a fixture's bytes as its original author would have, then verify that 31 .unwrap_or_else(|| panic!("unexpected create output: {}", out));
28 /// signature against the event as deserialized by current code. `Valid` means 32 for name in repo
29 /// the event survives the deserialize/re-serialize round trip byte for byte, 33 .git(&["for-each-ref", "--format=%(refname)", "refs/collab/"])
30 /// which is what decides whether `sync` accepts a ref carrying it. 34 .lines()
31 fn round_trip_status(name: &str) -> VerifyStatus { 35 {
32 use base64::engine::general_purpose::STANDARD; 36 if let Some(rest) = name.strip_prefix("refs/collab/patches/") {
33 use base64::Engine; 37 let id = rest.split('/').next().unwrap_or_default();
34 use ed25519_dalek::Signer; 38 if id.starts_with(short) {
35 39 return id.to_string();
36 let raw = fixture(name); 40 }
37 let original: serde_json::Value = serde_json::from_str(&raw).unwrap();
38 let canonical = serde_json::to_string(&original).unwrap();
39
40 let sk = test_signing_key();
41 let detached = DetachedSignature {
42 signature: STANDARD.encode(sk.sign(canonical.as_bytes()).to_bytes()),
43 pubkey: STANDARD.encode(sk.verifying_key().to_bytes()),
44 };
45
46 let event: Event = serde_json::from_str(&raw).unwrap();
47 signing::verify_detached(&event, &detached).unwrap()
48 }
49
50 // ===========================================================================
51 // Deserialization of real historical events
52 // ===========================================================================
53
54 #[test]
55 fn legacy_patch_create_without_commit_or_tree_deserializes() {
56 let event: Event = serde_json::from_str(&fixture("patch_create_missing_commit_tree.json"))
57 .expect("legacy patch.create must deserialize");
58
59 match event.action {
60 Action::PatchCreate {
61 title,
62 base_ref,
63 branch,
64 fixes,
65 commit,
66 tree,
67 base_commit,
68 ..
69 } => {
70 assert_eq!(title, "Avoid double DAG walk after auto_detect_revision");
71 assert_eq!(base_ref, "main");
72 assert_eq!(branch, "worktree-agent-aef11cfd");
73 assert_eq!(fixes.as_deref(), Some("53e698d5"));
74 assert!(commit.is_empty(), "unrecorded commit reads as empty");
75 assert!(tree.is_empty(), "unrecorded tree reads as empty");
76 assert!(base_commit.is_none());
77 }
78 other => panic!("expected PatchCreate, got {:?}", other),
79 }
80 }
81
82 #[test]
83 fn legacy_patch_create_with_head_commit_deserializes() {
84 let event: Event = serde_json::from_str(&fixture("patch_create_head_commit_variant.json"))
85 .expect("legacy PatchCreate must deserialize");
86
87 match event.action {
88 Action::PatchCreate {
89 title,
90 branch,
91 commit,
92 tree,
93 ..
94 } => {
95 assert_eq!(title, "Add --json output flag to list/show commands");
96 // This generation recorded the head SHA in a field named
97 // `head_commit`, which is aliased onto `branch`.
98 assert_eq!(branch, "db059e663d7a435f288bd2832bee4e9f374474d2");
99 assert!(commit.is_empty());
100 assert!(tree.is_empty());
101 }
102 other => panic!("expected PatchCreate, got {:?}", other),
103 }
104 assert_eq!(event.clock, 0, "this generation had no clock field");
105 }
106
107 #[test]
108 fn legacy_patch_review_without_revision_deserializes() {
109 let event: Event = serde_json::from_str(&fixture("patch_review_missing_revision.json"))
110 .expect("legacy patch.review must deserialize");
111
112 match event.action {
113 Action::PatchReview {
114 verdict,
115 body,
116 revision,
117 } => {
118 assert_eq!(verdict, ReviewVerdict::RequestChanges);
119 assert!(body.starts_with("Good approach returning Option<Revision>"));
120 assert_eq!(revision, None, "no revision was recorded");
121 }
122 other => panic!("expected PatchReview, got {:?}", other),
123 }
124 }
125
126 #[test]
127 fn current_patch_review_keeps_its_revision() {
128 let event: Event = serde_json::from_str(&fixture("patch_review_with_revision.json")).unwrap();
129
130 match event.action {
131 Action::PatchReview {
132 verdict, revision, ..
133 } => {
134 assert_eq!(verdict, ReviewVerdict::Approve);
135 assert_eq!(revision, Some(2));
136 }
137 other => panic!("expected PatchReview, got {:?}", other),
138 }
139 }
140
141 #[test]
142 fn legacy_patch_revise_deserializes_as_a_revision() {
143 let event: Event = serde_json::from_str(&fixture("patch_revise_body_only.json"))
144 .expect("legacy patch.revise must deserialize");
145
146 match event.action {
147 Action::PatchRevision {
148 commit,
149 tree,
150 body,
151 base,
152 } => {
153 assert!(commit.is_empty());
154 assert!(tree.is_empty());
155 assert!(base.is_none(), "this generation recorded no base");
156 assert_eq!(
157 body.as_deref(),
158 Some("Address review: extract auto_detect_and_update helper to eliminate 3x repeated pattern")
159 );
160 } 41 }
161 other => panic!("expected PatchRevision, got {:?}", other),
162 } 42 }
43 panic!("no patch ref matching {}", short);
163 } 44 }
164 45
165 // =========================================================================== 46 /// Write a patch in the pre-migration layout: one bare `refs/collab/patches/<id>`
166 // Signature round-tripping (decides whether `sync` accepts these refs) 47 /// ref that *is* the event DAG.
167 // =========================================================================== 48 ///
168 49 /// By hand, because no version of the tool that can still be built writes this
169 #[test] 50 /// shape — and a repository somewhere may still hold it, which is the case
170 fn legacy_patch_create_signature_survives_the_round_trip() { 51 /// under test.
171 // Deserializing adds no `commit`/`tree` back, so the bytes the signer 52 fn legacy_bare_patch(repo: &TestRepo) -> String {
172 // signed are the bytes we re-serialize. 53 let git_repo = git2::Repository::open(repo.dir.path()).unwrap();
173 assert_eq!( 54 let head = git_repo.head().unwrap().target().unwrap();
174 round_trip_status("patch_create_missing_commit_tree.json"), 55 let tree = git_repo.find_commit(head).unwrap().tree().unwrap().id();
175 VerifyStatus::Valid
176 );
177 }
178
179 #[test]
180 fn legacy_patch_review_signature_survives_the_round_trip() {
181 assert_eq!(
182 round_trip_status("patch_review_missing_revision.json"),
183 VerifyStatus::Valid
184 );
185 }
186
187 #[test]
188 fn current_event_signature_survives_the_round_trip() {
189 assert_eq!(
190 round_trip_status("patch_review_with_revision.json"),
191 VerifyStatus::Valid
192 );
193 }
194
195 #[test]
196 fn renamed_legacy_shapes_still_cannot_round_trip() {
197 // `patch.revise` re-serializes as `patch.revision`, and the older
198 // `PatchCreate`/`head_commit` spelling normalizes to the current one, so
199 // no serde attribute can make these byte-identical. Their refs stay
200 // unsyncable until verification compares stored bytes — issue 2a79b3ab.
201 // Asserted so the day that changes, this test says so.
202 assert_eq!(
203 round_trip_status("patch_revise_body_only.json"),
204 VerifyStatus::Invalid
205 );
206 assert_eq!(
207 round_trip_status("patch_create_head_commit_variant.json"),
208 VerifyStatus::Invalid
209 );
210 }
211
212 // ===========================================================================
213 // Materializing a legacy DAG into PatchState
214 // ===========================================================================
215
216 /// Build the event chain of patch 05e5fa7c ("Avoid double DAG walk after
217 /// auto_detect_revision"): create, then three review rounds interleaved with
218 /// two revises, then a merge. Returns (ref_name, id).
219 fn write_legacy_review_history(repo: &git2::Repository) -> (String, String) {
220 let root = write_raw_event( 56 let root = write_raw_event(
221 repo, 57 &git_repo,
222 None, 58 None,
223 json!({ 59 json!({
224 "type": "patch.create", 60 "type": "patch.create",
225 "title": "Avoid double DAG walk after auto_detect_revision", 61 "title": "Written by an older version",
226 "body": "", 62 "body": "",
227 "base_ref": "main", 63 "base_ref": "main",
228 "branch": "worktree-agent-aef11cfd", 64 "branch": "old",
229 "fixes": "53e698d5", 65 "commit": head.to_string(),
66 "tree": tree.to_string(),
230 }), 67 }),
231 1, 68 1,
232 ); 69 );
233 let mut tip = write_raw_event( 70 let id = root.to_string();
234 repo, 71 git_repo
235 Some(root), 72 .reference(
236 json!({ 73 &format!("refs/collab/patches/{}", id),
237 "type": "patch.review", 74 root,
238 "verdict": "RequestChanges", 75 false,
239 "body": "Extract that into a small helper.", 76 "old layout",
240 }), 77 )
241 2, 78 .unwrap();
242 ); 79 id
243 tip = write_raw_event( 80 }
244 repo, 81
245 Some(tip), 82 /// Every message about a superseded shape has to do three things, or it is not
246 json!({ 83 /// the diagnostic this removal promised: name the ref, say the layout is not
247 "type": "patch.revise", 84 /// read any more, and point at the command that lists them.
248 "body": "Address review: extract auto_detect_and_update helper", 85 fn assert_actionable(stderr: &str, ref_name: &str) {
249 }), 86 assert!(
250 3, 87 stderr.contains(ref_name),
251 ); 88 "the message must name the offending ref, got: {}",
252 tip = write_raw_event( 89 stderr
253 repo,
254 Some(tip),
255 json!({
256 "type": "patch.review",
257 "verdict": "RequestChanges",
258 "body": "One remaining nit: the let mut p = p rebinding.",
259 }),
260 4,
261 ); 90 );
262 tip = write_raw_event( 91 assert!(
263 repo, 92 stderr.contains("git-collab refs"),
264 Some(tip), 93 "the message must point at the diagnostic command, got: {}",
265 json!({ 94 stderr
266 "type": "patch.revise",
267 "body": "Address r2 feedback: remove let mut p = p rebinding",
268 }),
269 5,
270 ); 95 );
271 tip = write_raw_event( 96 let lowered = stderr.to_lowercase();
272 repo, 97 assert!(
273 Some(tip), 98 lowered.contains("layout") || lowered.contains("older version"),
274 json!({ 99 "the message must say what is wrong with the ref, got: {}",
275 "type": "patch.review", 100 stderr
276 "verdict": "Approve",
277 "body": "Clean. Ship it.",
278 }),
279 6,
280 ); 101 );
281 tip = write_raw_event(repo, Some(tip), json!({ "type": "patch.merge" }), 7);
282
283 let id = root.to_string();
284 let ref_name = format!("refs/collab/patches/{}", id);
285 repo.reference(&ref_name, tip, false, "legacy patch")
286 .unwrap();
287 (ref_name, id)
288 } 102 }
289 103
104 // ===========================================================================
105 // Superseded ref layouts are refused, loudly
106 // ===========================================================================
107
290 #[test] 108 #[test]
291 fn legacy_patch_with_review_history_loads_intact() { 109 fn a_bare_patch_ref_is_refused_and_says_what_to_do() {
292 let work = TestRepo::new("Alice", "alice@example.com"); 110 let repo = TestRepo::new("Alice", "alice@example.com");
293 let repo = git2::Repository::open(work.dir.path()).unwrap(); 111 let id = legacy_bare_patch(&repo);
294 let (ref_name, id) = write_legacy_review_history(&repo);
295 112
296 let patch = PatchState::from_ref_uncached(&repo, &ref_name, &id) 113 let stderr = repo.run_err(&["patch", "list"]);
297 .expect("legacy patch must materialize"); 114 assert_actionable(&stderr, &format!("refs/collab/patches/{}", id));
115 }
298 116
299 assert_eq!( 117 #[test]
300 patch.title, 118 fn a_bare_patch_ref_does_not_produce_an_empty_list() {
301 "Avoid double DAG walk after auto_detect_revision" 119 // The specific regression this replaces: before, the reader tolerated the
120 // shape; a reader that merely stopped recognising it would list nothing
121 // and exit zero, which reads as "this repository has no patches".
122 let repo = TestRepo::new("Alice", "alice@example.com");
123 legacy_bare_patch(&repo);
124
125 let output = repo.run(&["patch", "list"]);
126 assert!(
127 !output.status.success(),
128 "a repository holding a superseded layout must fail, not report emptiness: {}",
129 String::from_utf8_lossy(&output.stdout)
302 ); 130 );
303 assert_eq!(patch.branch, "worktree-agent-aef11cfd"); 131 }
304 assert_eq!(patch.status, PatchStatus::Merged);
305 132
306 // Each revise is its own revision even though none of them recorded a 133 #[test]
307 // commit — collapsing them would also collapse the reviews between them. 134 fn a_numbered_revision_ref_is_refused_and_says_what_to_do() {
308 let numbers: Vec<u32> = patch.revisions.iter().map(|r| r.number).collect(); 135 let repo = TestRepo::new("Alice", "alice@example.com");
309 assert_eq!(numbers, vec![1, 2, 3]); 136 let id = patch_on_branch(&repo, "feat", "a.txt");
310 assert!(patch.revisions.iter().all(|r| r.commit.is_empty())); 137 let tip = repo.git(&["rev-parse", "HEAD"]).trim().to_string();
311 assert_eq!( 138
312 patch.revisions[1].body.as_deref(), 139 // The interim shape: a revision named by number rather than by the OID it
313 Some("Address review: extract auto_detect_and_update helper") 140 // pins. It can only exist beside an events ref.
314 ); 141 repo.git(&[
315 assert_eq!( 142 "update-ref",
316 patch.revisions[2].body.as_deref(), 143 &format!("refs/collab/patches/{}/r/1", id),
317 Some("Address r2 feedback: remove let mut p = p rebinding") 144 &tip,
318 ); 145 ]);
146
147 let stderr = repo.run_err(&["patch", "list"]);
148 assert_actionable(&stderr, &format!("refs/collab/patches/{}/r/1", id));
149 }
319 150
320 // All three reviews survive: they are attributed to the revision that was 151 #[test]
321 // current when each was written, so the one-vote-per-revision rule does 152 fn refs_still_reports_the_shape_the_reader_refuses() {
322 // not treat them as one author revoting on the same revision. 153 // `git-collab refs` is the one command that must keep working here: it is
323 let verdicts: Vec<ReviewVerdict> = patch.reviews.iter().map(|r| r.verdict).collect(); 154 // what the refusal message tells the operator to run, so if it failed the
324 assert_eq!( 155 // same way the advice would be a loop.
325 verdicts, 156 let repo = TestRepo::new("Alice", "alice@example.com");
326 vec![ 157 let id = legacy_bare_patch(&repo);
327 ReviewVerdict::RequestChanges, 158
328 ReviewVerdict::RequestChanges, 159 let out = repo.run_ok(&["refs"]);
329 ReviewVerdict::Approve 160 let line = out
330 ] 161 .lines()
162 .find(|l| l.contains(&format!("refs/collab/patches/{}", id)))
163 .unwrap_or_else(|| panic!("refs must still list the bare ref, got:\n{}", out));
164 assert!(
165 line.contains("legacy"),
166 "refs must still classify the superseded layout by name: {:?}",
167 line
331 ); 168 );
332 let review_revs: Vec<Option<u32>> = patch.reviews.iter().map(|r| r.revision).collect();
333 assert_eq!(review_revs, vec![Some(1), Some(2), Some(3)]);
334 } 169 }
335 170
336 #[test] 171 #[test]
337 fn legacy_patch_appears_in_patch_list() { 172 fn nothing_migrates_the_shape_out_from_under_the_operator() {
338 let work = TestRepo::new("Alice", "alice@example.com"); 173 // The removal's other half: a failed read must leave the evidence in
339 let repo = git2::Repository::open(work.dir.path()).unwrap(); 174 // place. A command that "helpfully" deleted or rewrote the ref on the way
340 write_legacy_review_history(&repo); 175 // to failing would destroy the thing the operator was told to go look at.
341 drop(repo); 176 let repo = TestRepo::new("Alice", "alice@example.com");
342 177 let id = legacy_bare_patch(&repo);
343 let out = work.run_ok(&["patch", "list", "--all"]); 178 let before = repo.git(&["rev-parse", &format!("refs/collab/patches/{}", id)]);
344 assert!( 179
345 out.contains("Avoid double DAG walk"), 180 let _ = repo.run(&["patch", "list"]);
346 "legacy patch missing from list output: {}", 181 let _ = repo.run(&["refs"]);
347 out 182
183 let after = repo.git(&["rev-parse", &format!("refs/collab/patches/{}", id)]);
184 assert_eq!(
185 before.trim(),
186 after.trim(),
187 "the superseded ref must survive being refused"
348 ); 188 );
349 } 189 }
350 190
351 // =========================================================================== 191 // ===========================================================================
352 // Writes against a legacy patch 192 // A head that cannot be resolved
353 // =========================================================================== 193 // ===========================================================================
354 194
355 #[test] 195 #[test]
356 fn commenting_on_a_legacy_patch_does_not_invent_a_revision() { 196 fn a_patch_whose_revision_commit_is_missing_fails_clearly() {
357 let work = TestRepo::new("Alice", "alice@example.com"); 197 // Patches used to be addressed by branch, so a head that could not be
358 let head = work.git(&["rev-parse", "HEAD"]).trim().to_string(); 198 // found from a recorded revision fell back to `refs/heads/<branch>`. That
359 let repo = git2::Repository::open(work.dir.path()).unwrap(); 199 // fallback is gone: a patch is addressed by its own revision refs. What
200 // must not happen is the old silent no-op — the caller has to be told the
201 // objects are missing, and which patch is affected.
202 let repo = TestRepo::new("Alice", "alice@example.com");
203 let git_repo = git2::Repository::open(repo.dir.path()).unwrap();
204 let head = git_repo.head().unwrap().target().unwrap();
205 let tree = git_repo.find_commit(head).unwrap().tree().unwrap().id();
206
207 // A recorded commit that is a well-formed OID but is not in the object
208 // database — a revision whose objects were never fetched.
209 let absent = "a".repeat(40);
210 assert!(
211 git_repo
212 .find_commit(git2::Oid::from_str(&absent).unwrap())
213 .is_err(),
214 "the placeholder commit must not be in the object database"
215 );
360 216
361 // The oldest patches stored the head SHA where `branch` now lives, so the
362 // head still resolves even though revision 1 recorded no commit.
363 let root = write_raw_event( 217 let root = write_raw_event(
364 &repo, 218 &git_repo,
365 None, 219 None,
366 json!({ 220 json!({
367 "type": "patch.create", 221 "type": "patch.create",
368 "title": "Legacy patch with resolvable head", 222 "title": "Objects never fetched",
369 "body": "", 223 "body": "",
370 "base_ref": "main", 224 "base_ref": "main",
371 "head_commit": head, 225 "branch": "main",
226 "commit": absent,
227 "tree": tree.to_string(),
228 "base_commit": head.to_string(),
372 }), 229 }),
373 1, 230 1,
374 ); 231 );
375 let id = root.to_string(); 232 let id = root.to_string();
376 repo.reference( 233 git_repo
377 &format!("refs/collab/patches/{}", id), 234 .reference(
378 root, 235 &format!("refs/collab/patches/{}/events", id),
379 false, 236 root,
380 "legacy", 237 false,
381 ) 238 "events",
382 .unwrap(); 239 )
383 drop(repo); 240 .unwrap();
384
385 work.run_ok(&["patch", "comment", &id[..8], "-b", "a comment"]);
386
387 let out = work.run_ok(&["patch", "show", &id[..8], "--json"]);
388 let shown: serde_json::Value = serde_json::from_str(&out).unwrap();
389 assert_eq!(
390 shown["revisions"].as_array().unwrap().len(),
391 1,
392 "auto-detection must not turn an unrecorded commit into a new revision: {}",
393 out
394 );
395 }
396
397 // ===========================================================================
398 // Server rendering
399 // ===========================================================================
400
401 #[test]
402 fn patch_detail_page_renders_a_revision_with_no_commit() {
403 let harness = ServerHarness::new("legacy-patch-detail");
404 harness.push_head();
405
406 let repo = git2::Repository::open(harness.work_repo().dir.path()).unwrap();
407 let (_ref_name, id) = write_legacy_review_history(&repo);
408 drop(repo);
409 harness.push_collab_refs();
410 241
411 let page = harness.get_ok(&format!("/{}/patches/{}", harness.repo_name(), id)); 242 // `main` exists and is a perfectly good branch name on this patch. Under
412 assert!(page.body.contains("Avoid double DAG walk")); 243 // the old fallback that alone would have silently resolved the head to
244 // main's tip — a different commit than the patch ever recorded.
245 let stderr = repo.run_err(&["patch", "show", &id[..8]]);
246 let lowered = stderr.to_lowercase();
413 assert!( 247 assert!(
414 page.body.contains(">unknown<"), 248 lowered.contains("commit") || lowered.contains("revision"),
415 "revisions with no commit should say so, in the same words as the CLI: {}", 249 "the failure must say the recorded revision commit is what is missing, got: {}",
416 page.body 250 stderr
417 ); 251 );
418 assert!( 252 assert!(
419 !page.body.contains("/diff/\""), 253 stderr.contains(&absent[..8]) || lowered.contains("missing") || lowered.contains("not "),
420 "a revision with no commit must not link to an empty diff: {}", 254 "the failure must identify what could not be resolved, got: {}",
421 page.body 255 stderr
422 ); 256 );
423 assert!(page.body.contains("Ship it."), "reviews should render");
424 } 257 }
tests/refs_test.rs
Old New
@@ -102,9 +102,14 @@ fn line_for<'a>(output: &'a str, needle: &str) -> &'a str {
102 } 102 }
103 103
104 /// A repository holding one of each shape that matters: a current patch with a 104 /// A repository holding one of each shape that matters: a current patch with a
105 /// pinned revision, an issue, and — written after every CLI call, so nothing 105 /// pinned revision, an issue, and a bare pre-migration patch ref plus an
106 /// migrates it out from under the test — a bare pre-migration patch ref and an
107 /// interim numbered revision ref. 106 /// interim numbered revision ref.
107 ///
108 /// The superseded shapes are written after every CLI call, and the ordering is
109 /// still load-bearing — for the opposite reason it used to be. It was to stop
110 /// the layout migration converting them out from under the test; the migration
111 /// is gone (issue `e5096ffc`), and now every command except `refs` refuses to
112 /// run at all once one of these exists.
108 fn mixed_layout_repo() -> (TestRepo, String, String, String) { 113 fn mixed_layout_repo() -> (TestRepo, String, String, String) {
109 let repo = TestRepo::new("Alice", "alice@example.com"); 114 let repo = TestRepo::new("Alice", "alice@example.com");
110 let (modern, tip) = patch_on_branch(&repo, "feat", "a.txt"); 115 let (modern, tip) = patch_on_branch(&repo, "feat", "a.txt");
tests/review_test.rs
Old New
@@ -53,7 +53,7 @@ fn review_event(
53 action: Action::PatchReview { 53 action: Action::PatchReview {
54 verdict, 54 verdict,
55 body: body.to_string(), 55 body: body.to_string(),
56 revision: Some(revision), 56 revision,
57 }, 57 },
58 clock: 0, 58 clock: 0,
59 } 59 }
tests/revision_refs_test.rs
Old New
@@ -445,9 +445,16 @@ fn tree_of(repo: &TestRepo, commit: &str) -> String {
445 tree 445 tree
446 } 446 }
447 447
448 /// A patch in the shape migration produces: a `PatchCreate` carrying 448 /// A patch whose first revision records a base and whose second does not: a
449 /// `base_commit`, followed by a `PatchRevision` from before revisions recorded 449 /// `PatchCreate` carrying `base_commit`, followed by a `PatchRevision` written
450 /// a base of their own. Returns the full id. 450 /// before revisions recorded a base of their own. Returns the full id.
451 ///
452 /// This is the shape `PatchState::effective_base` exists for, and it is a
453 /// permanent one — the merge-base was never computed for that second revision
454 /// and nothing can recover it. Issue `e5096ffc` removed the pre-release
455 /// compatibility around it and deliberately kept this, so the refs below are
456 /// the *current* layout: what makes the fixture legacy is the missing `base`
457 /// key, not the shape of its refs.
451 fn patch_with_baseless_revision( 458 fn patch_with_baseless_revision(
452 repo: &TestRepo, 459 repo: &TestRepo,
453 r1: &str, 460 r1: &str,
@@ -484,17 +491,17 @@ fn patch_with_baseless_revision(
484 let id = root.to_string(); 491 let id = root.to_string();
485 git_repo 492 git_repo
486 .reference( 493 .reference(
487 &format!("refs/collab/patches/{}", id), 494 &format!("refs/collab/patches/{}/events", id),
488 tip, 495 tip,
489 false, 496 false,
490 "old layout", 497 "events",
491 ) 498 )
492 .unwrap(); 499 .unwrap();
493 id 500 id
494 } 501 }
495 502
496 #[test] 503 #[test]
497 fn a_migrated_patch_merged_by_exact_fast_forward_is_detected() { 504 fn a_patch_with_a_baseless_revision_merged_by_exact_fast_forward_is_detected() {
498 // The latest revision predates `base`, which is the normal shape of a 505 // The latest revision predates `base`, which is the normal shape of a
499 // migrated patch. In the exact fast-forward case — main fast-forwarded onto 506 // migrated patch. In the exact fast-forward case — main fast-forwarded onto
500 // the patch head, so base tip == head — recomputing a merge-base yields the 507 // the patch head, so base tip == head — recomputing a merge-base yields the
@@ -521,7 +528,7 @@ fn a_migrated_patch_merged_by_exact_fast_forward_is_detected() {
521 } 528 }
522 529
523 #[test] 530 #[test]
524 fn a_migrated_patch_that_was_not_merged_stays_open() { 531 fn a_patch_with_a_baseless_revision_that_was_not_merged_stays_open() {
525 let repo = TestRepo::new("Alice", "alice@example.com"); 532 let repo = TestRepo::new("Alice", "alice@example.com");
526 let base = repo.git(&["rev-parse", "main"]).trim().to_string(); 533 let base = repo.git(&["rev-parse", "main"]).trim().to_string();
527 repo.git(&["checkout", "-b", "feat"]); 534 repo.git(&["checkout", "-b", "feat"]);
@@ -567,10 +574,10 @@ fn a_patch_created_on_the_base_branch_is_not_reported_merged() {
567 let id = root.to_string(); 574 let id = root.to_string();
568 git_repo 575 git_repo
569 .reference( 576 .reference(
570 &format!("refs/collab/patches/{}", id), 577 &format!("refs/collab/patches/{}/events", id),
571 root, 578 root,
572 false, 579 false,
573 "old layout", 580 "events",
574 ) 581 )
575 .unwrap(); 582 .unwrap();
576 drop(git_repo); 583 drop(git_repo);
@@ -629,10 +636,10 @@ fn merge_detection_reads_the_base_of_the_revision_it_resolved_the_head_from() {
629 let id = root.to_string(); 636 let id = root.to_string();
630 git_repo 637 git_repo
631 .reference( 638 .reference(
632 &format!("refs/collab/patches/{}", id), 639 &format!("refs/collab/patches/{}/events", id),
633 tip, 640 tip,
634 false, 641 false,
635 "old layout", 642 "events",
636 ) 643 )
637 .unwrap(); 644 .unwrap();
638 drop(git_repo); 645 drop(git_repo);
@@ -770,364 +777,3 @@ fn deleting_a_patch_removes_every_ref_in_its_namespace() {
770 refs 777 refs
771 ); 778 );
772 } 779 }
773
774 // ===========================================================================
775 // Migration from the single-ref layout
776 // ===========================================================================
777
778 /// Rewrite a patch back into the pre-revision-refs layout: one ref at
779 /// `refs/collab/patches/<id>` and no revision refs at all.
780 fn demote_to_old_layout(repo: &TestRepo, id: &str) {
781 let events = format!("refs/collab/patches/{}/events", id);
782 let tip = ref_target(repo, &events).expect("events ref");
783 for name in patch_refs(repo) {
784 if name.starts_with(&format!("refs/collab/patches/{}/", id)) {
785 repo.git(&["update-ref", "-d", &name]);
786 }
787 }
788 repo.git(&["update-ref", &format!("refs/collab/patches/{}", id), &tip]);
789 }
790
791 /// Reproduce the state a migration killed midway leaves behind: the tip parked
792 /// outside the patch namespace, the old ref already deleted, and no events ref
793 /// yet. Nothing enumerates the patch in this state, so only a resume can find
794 /// it again.
795 fn interrupt_migration_after_delete(repo: &TestRepo, id: &str) {
796 let events = format!("refs/collab/patches/{}/events", id);
797 let tip = ref_target(repo, &events).expect("events ref");
798 for name in patch_refs(repo) {
799 if name.starts_with(&format!("refs/collab/patches/{}/", id)) {
800 repo.git(&["update-ref", "-d", &name]);
801 }
802 }
803 repo.git(&[
804 "update-ref",
805 &format!("refs/collab/local/migrating/patches/{}", id),
806 &tip,
807 ]);
808 }
809
810 #[test]
811 fn migration_resumes_after_an_interrupted_run() {
812 // A kill between deleting the old ref and creating the events ref used to
813 // lose the patch from every listing permanently — the objects survived but
814 // nothing could find them. The parked ref only makes that recoverable if
815 // something reads it back.
816 let repo = TestRepo::new("Alice", "alice@example.com");
817 let (short, id, r1) = patch_on_branch(&repo, "feat", "a.txt");
818 repo.commit_file("b.txt", "v2", "second commit");
819 repo.run_ok(&["patch", "revise", &short]);
820 let r2 = repo.git(&["rev-parse", "HEAD"]).trim().to_string();
821
822 interrupt_migration_after_delete(&repo, &id);
823 assert!(
824 patch_refs(&repo)
825 .iter()
826 .all(|r| !r.starts_with(&format!("refs/collab/patches/{}", id))),
827 "precondition: the patch is unreachable from the patch namespace"
828 );
829
830 let out = repo.run_ok(&["patch", "list"]);
831 assert!(out.contains(&short), "the patch must come back: {}", out);
832
833 let refs = patch_refs(&repo);
834 assert!(
835 refs.contains(&format!("refs/collab/patches/{}/events", id)),
836 "{:?}",
837 refs
838 );
839 let mut expected = vec![r1, r2];
840 expected.sort();
841 assert_eq!(pinned_commits(&repo, &id), expected);
842 assert!(
843 ref_target(&repo, &format!("refs/collab/local/migrating/patches/{}", id)).is_none(),
844 "the park must be cleared once the patch is whole"
845 );
846 }
847
848 #[test]
849 fn migration_resumes_between_the_events_ref_and_the_pins() {
850 // The narrower window: the events ref landed but the revisions were never
851 // pinned. The patch lists fine, so nothing else would notice — only the
852 // park says the migration never finished, and dropping it on sight would
853 // leave the revisions unpinned for good.
854 let repo = TestRepo::new("Alice", "alice@example.com");
855 let (short, id, r1) = patch_on_branch(&repo, "feat", "a.txt");
856 let r2 = repo.commit_file("b.txt", "v2", "second commit");
857 repo.run_ok(&["patch", "revise", &short]);
858
859 let tip = ref_target(&repo, &format!("refs/collab/patches/{}/events", id)).unwrap();
860 for name in patch_refs(&repo) {
861 if name.starts_with(&format!("refs/collab/patches/{}/rev/", id)) {
862 repo.git(&["update-ref", "-d", &name]);
863 }
864 }
865 repo.git(&[
866 "update-ref",
867 &format!("refs/collab/local/migrating/patches/{}", id),
868 &tip,
869 ]);
870 assert!(pinned_commits(&repo, &id).is_empty(), "precondition");
871
872 repo.run_ok(&["patch", "list"]);
873
874 let mut expected = vec![r1, r2];
875 expected.sort();
876 assert_eq!(
877 pinned_commits(&repo, &id),
878 expected,
879 "the resume must pin what the interrupted run did not"
880 );
881 assert!(
882 ref_target(&repo, &format!("refs/collab/local/migrating/patches/{}", id)).is_none(),
883 "and only then drop the park"
884 );
885 }
886
887 #[test]
888 fn migration_clears_a_park_left_by_a_late_interruption() {
889 // The other half of the same window: killed after the events ref was
890 // created but before the park was deleted. The patch is fine; the park is
891 // litter that would otherwise never be collected.
892 let repo = TestRepo::new("Alice", "alice@example.com");
893 let (short, id, _r1) = patch_on_branch(&repo, "feat", "a.txt");
894 let tip = ref_target(&repo, &format!("refs/collab/patches/{}/events", id)).unwrap();
895 repo.git(&[
896 "update-ref",
897 &format!("refs/collab/local/migrating/patches/{}", id),
898 &tip,
899 ]);
900
901 repo.run_ok(&["patch", "list"]);
902
903 assert!(
904 ref_target(&repo, &format!("refs/collab/local/migrating/patches/{}", id)).is_none(),
905 "a park whose patch already has an events ref must be swept"
906 );
907 let json = show_json(&repo, &short);
908 assert_eq!(json["revisions"].as_array().unwrap().len(), 1);
909 }
910
911 #[test]
912 fn an_old_layout_patch_is_migrated_on_first_use() {
913 let repo = TestRepo::new("Alice", "alice@example.com");
914 let (short, id, r1) = patch_on_branch(&repo, "feat", "a.txt");
915 repo.commit_file("b.txt", "v2", "second commit");
916 repo.run_ok(&["patch", "revise", &short]);
917 let r2 = repo.git(&["rev-parse", "HEAD"]).trim().to_string();
918 demote_to_old_layout(&repo, &id);
919
920 // Any read is "first use".
921 let out = repo.run_ok(&["patch", "list"]);
922 assert!(out.contains(&short), "{}", out);
923
924 let refs = patch_refs(&repo);
925 assert!(
926 refs.contains(&format!("refs/collab/patches/{}/events", id)),
927 "{:?}",
928 refs
929 );
930 assert!(
931 !refs.contains(&format!("refs/collab/patches/{}", id)),
932 "the old ref must be gone: {:?}",
933 refs
934 );
935 let mut expected = vec![r1, r2];
936 expected.sort();
937 assert_eq!(pinned_commits(&repo, &id), expected);
938
939 // The migrated patch still diffs.
940 let diff = repo.run_ok(&["patch", "diff", &short, "--revision", "1"]);
941 assert!(diff.contains("a.txt"), "{}", diff);
942 }
943
944 #[test]
945 fn migration_skips_a_revision_whose_commit_is_already_lost() {
946 // Revisions whose objects were stripped by a force-push before the patch
947 // was migrated cannot be recovered. Migration must leave them without a
948 // ref rather than failing and taking the whole patch down with it.
949 let repo = TestRepo::new("Alice", "alice@example.com");
950 let head = repo.git(&["rev-parse", "HEAD"]).trim().to_string();
951 let git_repo = git2::Repository::open(repo.dir.path()).unwrap();
952 let tree = git_repo
953 .find_commit(git2::Oid::from_str(&head).unwrap())
954 .unwrap()
955 .tree()
956 .unwrap()
957 .id()
958 .to_string();
959 let missing = "1111111111111111111111111111111111111111";
960
961 let root = write_raw_event(
962 &git_repo,
963 None,
964 json!({
965 "type": "patch.create",
966 "title": "Patch whose r1 was force-pushed away",
967 "body": "",
968 "base_ref": "main",
969 "branch": "gone",
970 "commit": missing,
971 "tree": tree,
972 }),
973 1,
974 );
975 let tip = write_raw_event(
976 &git_repo,
977 Some(root),
978 json!({
979 "type": "patch.revision",
980 "commit": head,
981 "tree": tree,
982 "body": "still here",
983 }),
984 2,
985 );
986 let id = root.to_string();
987 git_repo
988 .reference(
989 &format!("refs/collab/patches/{}", id),
990 tip,
991 false,
992 "old layout",
993 )
994 .unwrap();
995 drop(git_repo);
996
997 let out = repo.run_ok(&["patch", "list", "--all"]);
998 assert!(out.contains("force-pushed away"), "{}", out);
999
1000 let refs = patch_refs(&repo);
1001 assert!(
1002 refs.contains(&format!("refs/collab/patches/{}/events", id)),
1003 "{:?}",
1004 refs
1005 );
1006 assert!(
1007 !refs.contains(&rev_ref(&id, missing)),
1008 "a lost commit must not get a ref: {:?}",
1009 refs
1010 );
1011 assert_eq!(
1012 pinned_commits(&repo, &id),
1013 vec![head],
1014 "only the surviving revision is pinned"
1015 );
1016 }
1017
1018 /// Rewrite a patch into the interim numbered layout: the events ref stays, but
1019 /// revisions are pinned by `r/<n>` instead of `rev/<oid>`. This shape is not
1020 /// hypothetical — it is what a repo migrated by the previous draft is in.
1021 fn demote_to_numbered_revision_refs(repo: &TestRepo, id: &str) {
1022 let prefix = format!("refs/collab/patches/{}/rev/", id);
1023 let mut n = 0;
1024 for name in patch_refs(repo) {
1025 let Some(commit) = name.strip_prefix(&prefix) else {
1026 continue;
1027 };
1028 n += 1;
1029 repo.git(&[
1030 "update-ref",
1031 &format!("refs/collab/patches/{}/r/{}", id, n),
1032 commit,
1033 ]);
1034 repo.git(&["update-ref", "-d", &name]);
1035 }
1036 assert!(n > 0, "nothing to demote");
1037 }
1038
1039 #[test]
1040 fn numbered_revision_refs_are_converted_to_oid_names() {
1041 let repo = TestRepo::new("Alice", "alice@example.com");
1042 let (short, id, r1) = patch_on_branch(&repo, "feat", "a.txt");
1043 let r2 = repo.commit_file("b.txt", "v2", "second commit");
1044 repo.run_ok(&["patch", "revise", &short]);
1045 demote_to_numbered_revision_refs(&repo, &id);
1046
1047 repo.run_ok(&["patch", "list"]);
1048
1049 let mut expected = vec![r1, r2];
1050 expected.sort();
1051 assert_eq!(
1052 pinned_commits(&repo, &id),
1053 expected,
1054 "every revision must be pinned under its OID name"
1055 );
1056 assert!(
1057 patch_refs(&repo)
1058 .iter()
1059 .all(|r| !r.starts_with(&format!("refs/collab/patches/{}/r/", id))),
1060 "the numbered refs must be retired, or they keep being pushed"
1061 );
1062 }
1063
1064 #[test]
1065 fn converting_numbered_refs_drops_one_the_dag_does_not_vouch_for() {
1066 // A numbered ref pointing at a commit no event lists is exactly the wedge
1067 // OID naming removes: a published number nobody can renegotiate. It goes,
1068 // and the revisions that are real stay.
1069 let repo = TestRepo::new("Alice", "alice@example.com");
1070 let (short, id, r1) = patch_on_branch(&repo, "feat", "a.txt");
1071 demote_to_numbered_revision_refs(&repo, &id);
1072 let stray = repo.git(&["rev-parse", "main"]).trim().to_string();
1073 repo.git(&[
1074 "update-ref",
1075 &format!("refs/collab/patches/{}/r/7", id),
1076 &stray,
1077 ]);
1078
1079 repo.run_ok(&["patch", "list"]);
1080
1081 assert_eq!(pinned_commits(&repo, &id), vec![r1]);
1082 assert!(
1083 patch_refs(&repo)
1084 .iter()
1085 .all(|r| !r.starts_with(&format!("refs/collab/patches/{}/r/", id))),
1086 "the stray numbered ref must go too"
1087 );
1088 // The patch itself is untouched.
1089 assert_eq!(show_json(&repo, &short)["revisions"].as_array().unwrap().len(), 1);
1090 }
1091
1092 #[test]
1093 fn a_fully_legacy_patch_with_no_recorded_commits_still_materializes() {
1094 let repo = TestRepo::new("Alice", "alice@example.com");
1095 let git_repo = git2::Repository::open(repo.dir.path()).unwrap();
1096 let root = write_raw_event(
1097 &git_repo,
1098 None,
1099 json!({
1100 "type": "patch.create",
1101 "title": "Ancient patch",
1102 "body": "",
1103 "base_ref": "main",
1104 "branch": "long-gone",
1105 }),
1106 1,
1107 );
1108 let id = root.to_string();
1109 git_repo
1110 .reference(
1111 &format!("refs/collab/patches/{}", id),
1112 root,
1113 false,
1114 "old layout",
1115 )
1116 .unwrap();
1117 drop(git_repo);
1118
1119 let out = repo.run_ok(&["patch", "list", "--all"]);
1120 assert!(out.contains("Ancient patch"), "{}", out);
1121
1122 let refs = patch_refs(&repo);
1123 assert!(
1124 refs.contains(&format!("refs/collab/patches/{}/events", id)),
1125 "{:?}",
1126 refs
1127 );
1128 assert!(
1129 pinned_commits(&repo, &id).is_empty(),
1130 "no commits were ever recorded, so there is nothing to pin: {:?}",
1131 refs
1132 );
1133 }
tests/server_migrate_test.rs
Old New
@@ -1,458 +0,0 @@
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 }
tests/sync_test.rs
Old New
@@ -819,7 +819,7 @@ fn test_patch_review_across_repos() {
819 action: Action::PatchReview { 819 action: Action::PatchReview {
820 verdict: ReviewVerdict::Approve, 820 verdict: ReviewVerdict::Approve,
821 body: "LGTM!".to_string(), 821 body: "LGTM!".to_string(),
822 revision: Some(1), 822 revision: 1,
823 }, 823 },
824 clock: 0, 824 clock: 0,
825 }; 825 };
@@ -884,7 +884,7 @@ fn test_concurrent_review_and_revise() {
884 action: Action::PatchReview { 884 action: Action::PatchReview {
885 verdict: ReviewVerdict::RequestChanges, 885 verdict: ReviewVerdict::RequestChanges,
886 body: "Needs work".to_string(), 886 body: "Needs work".to_string(),
887 revision: Some(1), 887 revision: 1,
888 }, 888 },
889 clock: 0, 889 clock: 0,
890 }; 890 };
@@ -2059,18 +2059,19 @@ fn commit_on_branch(
2059 } 2059 }
2060 2060
2061 #[test] 2061 #[test]
2062 fn sync_migrates_an_old_layout_repo_before_reconciling() { 2062 fn sync_refuses_a_repo_still_holding_the_old_layout() {
2063 // sync is precisely the command a contributor runs on a repo they have not 2063 // sync used to migrate here, because it is precisely the command a
2064 // otherwise touched, so it cannot rely on some earlier read having 2064 // contributor runs on a repo they have not otherwise touched. The
2065 // migrated. Left unmigrated, an incoming <id>/events collides with the 2065 // migration is gone (issue `e5096ffc`), confirmed unnecessary against every
2066 // local bare <id> as a directory-vs-file lock conflict that aborts the 2066 // repository we host, so what matters now is that sync *stops* — and says
2067 // whole patches reconcile, and the bare ref is pushed outbound where a 2067 // which ref and what to do. Left to carry on it would push the bare ref
2068 // migrated remote must reject it for the same reason. 2068 // outbound, where a remote holding the current layout rejects it as a
2069 // directory-vs-file conflict with nothing to explain why.
2069 let cluster = TestCluster::new(); 2070 let cluster = TestCluster::new();
2070 let alice_repo = cluster.alice_repo(); 2071 let alice_repo = cluster.alice_repo();
2071 2072
2072 let base = make_commit_with_message(&alice_repo, "base"); 2073 let base = make_commit_with_message(&alice_repo, "base");
2073 let feat = commit_on_branch(&alice_repo, "feat", base, "feature.txt", b"work"); 2074 commit_on_branch(&alice_repo, "feat", base, "feature.txt", b"work");
2074 cluster.run_collab_ok( 2075 cluster.run_collab_ok(
2075 cluster.alice_dir.path(), 2076 cluster.alice_dir.path(),
2076 &["patch", "create", "-t", "Old layout", "-B", "feat"], 2077 &["patch", "create", "-t", "Old layout", "-B", "feat"],
@@ -2093,34 +2094,31 @@ fn sync_migrates_an_old_layout_repo_before_reconciling() {
2093 for name in subtree { 2094 for name in subtree {
2094 alice_repo.find_reference(&name).unwrap().delete().unwrap(); 2095 alice_repo.find_reference(&name).unwrap().delete().unwrap();
2095 } 2096 }
2097 let bare = format!("refs/collab/patches/{}", id);
2096 alice_repo 2098 alice_repo
2097 .reference(&format!("refs/collab/patches/{}", id), tip, false, "demote") 2099 .reference(&bare, tip, false, "demote")
2098 .unwrap(); 2100 .unwrap();
2099 2101
2100 // sync must migrate before it reconciles or pushes. 2102 let err = sync::sync(&alice_repo, "origin")
2101 sync::sync(&alice_repo, "origin").unwrap(); 2103 .expect_err("sync must refuse a repository in the superseded layout");
2102 2104 let message = err.to_string();
2103 let alice_repo = Repository::open(cluster.alice_dir.path()).unwrap();
2104 assert!( 2105 assert!(
2105 alice_repo.refname_to_id(&events).is_ok(), 2106 message.contains(&bare),
2106 "sync must migrate the local layout" 2107 "the refusal must name the offending ref, got: {}",
2108 message
2107 ); 2109 );
2108 assert_eq!( 2110 assert!(
2109 alice_repo 2111 message.contains("git-collab refs"),
2110 .refname_to_id(&format!("refs/collab/patches/{}/rev/{}", id, feat)) 2112 "the refusal must point at the diagnostic command, got: {}",
2111 .ok(), 2113 message
2112 Some(feat)
2113 ); 2114 );
2114 2115
2115 // And the migrated layout is what reached the remote. 2116 // And it must not have quietly fixed or destroyed the evidence.
2116 let bob_repo = cluster.bob_repo(); 2117 let alice_repo = Repository::open(cluster.alice_dir.path()).unwrap();
2117 sync::sync(&bob_repo, "origin").unwrap();
2118 let bob_repo = Repository::open(cluster.bob_dir.path()).unwrap();
2119 assert_eq!( 2118 assert_eq!(
2120 bob_repo 2119 alice_repo.refname_to_id(&bare).ok(),
2121 .refname_to_id(&format!("refs/collab/patches/{}/rev/{}", id, feat)) 2120 Some(tip),
2122 .ok(), 2121 "a refused sync leaves the repository exactly as it found it"
2123 Some(feat)
2124 ); 2122 );
2125 } 2123 }
2126 2124