max / makenotwork
- Co-Authored-By
- Claude Opus 5 (1M context) <noreply@anthropic.com>
10 files changed,
+843 insertions,
-5 deletions
| @@ -126,6 +126,29 @@ | |||
| 126 | 126 | || echo "[$(date -u +%FT%TZ)] FAILED issues/process-push exit=$?" | |
| 127 | 127 | ) & | |
| 128 | 128 | ;; | |
| 129 | + | refs/mnw/notes-inbox/*) | |
| 130 | + | # The one arm that is NOT backgrounded. Builds and issue links are | |
| 131 | + | # things the pusher learns about later; a notes merge is the answer | |
| 132 | + | # to the push itself, and post-receive stdout is the only channel | |
| 133 | + | # back to them. The timeouts bound what that costs: a push cannot | |
| 134 | + | # hang on a server that is not answering. | |
| 135 | + | echo "[$(date -u +%FT%TZ)] notes-push $OWNER/$REPO_NAME ref=$refname" >>"$LOG" | |
| 136 | + | RESULT="$(curl -sf --connect-timeout 5 --max-time 30 -X POST \ | |
| 137 | + | -H "Authorization: Bearer __HMAC__" \ | |
| 138 | + | -H "Content-Type: application/json" \ | |
| 139 | + | -d "{\"repo_owner\": \"$OWNER\", \"repo_name\": \"$REPO_NAME\", \"ref_name\": \"$refname\"}" \ | |
| 140 | + | "http://localhost:3000/api/internal/notes/merge-inbox" 2>>"$LOG")" | |
| 141 | + | if [ -n "$RESULT" ]; then | |
| 142 | + | echo "notes: merged into refs/notes/${refname#refs/mnw/notes-inbox/}" | |
| 143 | + | echo "[$(date -u +%FT%TZ)] notes-merged $RESULT" >>"$LOG" | |
| 144 | + | else | |
| 145 | + | # The notes are in the inbox ref either way, so nothing is lost; | |
| 146 | + | # the next push to the namespace merges them. Say so rather than | |
| 147 | + | # letting the push look like it did nothing. | |
| 148 | + | echo "notes: received, merge deferred (the server did not answer)" | |
| 149 | + | echo "[$(date -u +%FT%TZ)] FAILED notes/merge-inbox" >>"$LOG" | |
| 150 | + | fi | |
| 151 | + | ;; | |
| 129 | 152 | esac | |
| 130 | 153 | done | |
| 131 | 154 | "#; |
| @@ -190,6 +190,26 @@ | |||
| 190 | 190 | "http://localhost:3000/api/internal/issues/process-push" \ | |
| 191 | 191 | >/dev/null 2>&1 & | |
| 192 | 192 | ;; | |
| 193 | + | refs/mnw/notes-inbox/*) | |
| 194 | + | # Not backgrounded, unlike the two above. A notes merge is the | |
| 195 | + | # answer to this push, and post-receive stdout is the only way back | |
| 196 | + | # to the person who made it. The timeouts stop a quiet server from | |
| 197 | + | # hanging a push. | |
| 198 | + | REPO_PATH="$(cd "$(dirname "$0")/.." && pwd)" | |
| 199 | + | REPO_NAME="$(basename "$REPO_PATH" .git)" | |
| 200 | + | OWNER="$(basename "$(dirname "$REPO_PATH")")" | |
| 201 | + | if curl -sf --connect-timeout 5 --max-time 30 -X POST \ | |
| 202 | + | -H "Authorization: Bearer __HMAC__" \ | |
| 203 | + | -H "Content-Type: application/json" \ | |
| 204 | + | -d "{\"repo_owner\": \"$OWNER\", \"repo_name\": \"$REPO_NAME\", \"ref_name\": \"$refname\"}" \ | |
| 205 | + | "http://localhost:3000/api/internal/notes/merge-inbox" >/dev/null 2>&1; then | |
| 206 | + | echo "notes: merged into refs/notes/${refname#refs/mnw/notes-inbox/}" | |
| 207 | + | else | |
| 208 | + | # The notes are in the inbox ref regardless, so nothing is lost | |
| 209 | + | # and the next push merges them. | |
| 210 | + | echo "notes: received, merge deferred (the server did not answer)" | |
| 211 | + | fi | |
| 212 | + | ;; | |
| 193 | 213 | esac | |
| 194 | 214 | done | |
| 195 | 215 | "#; |
| @@ -7,7 +7,7 @@ | |||
| 7 | 7 | //! decision that cannot be taken back once a repository is cloned, so it is | |
| 8 | 8 | //! asserted against the object on disk rather than against a redirect. | |
| 9 | 9 | ||
| 10 | - | use crate::harness::TestHarness; | |
| 10 | + | use crate::harness::{BuildOptions, TestHarness}; | |
| 11 | 11 | ||
| 12 | 12 | /// A bare repo at `{dir}/testowner/testrepo.git` with one commit on `main`. | |
| 13 | 13 | fn make_repo(dir: &std::path::Path) { | |
| @@ -22,7 +22,14 @@ | |||
| 22 | 22 | ||
| 23 | 23 | async fn setup(tmp: &tempfile::TempDir) -> (TestHarness, String) { | |
| 24 | 24 | make_repo(tmp.path()); | |
| 25 | - | let mut h = TestHarness::with_git_repos(tmp.path().to_str().unwrap().to_string()).await; | |
| 25 | + | // The build trigger token is what the post-receive hook's per-repo HMAC is | |
| 26 | + | // derived from, so the inbox endpoint needs it configured. | |
| 27 | + | let mut h = TestHarness::build(BuildOptions { | |
| 28 | + | git_repos_path: Some(tmp.path().to_str().unwrap().to_string()), | |
| 29 | + | build_trigger_token: Some("test-trigger-secret".to_string()), | |
| 30 | + | ..Default::default() | |
| 31 | + | }) | |
| 32 | + | .await; | |
| 26 | 33 | h.signup("testowner", "testowner@example.com", "password123") | |
| 27 | 34 | .await; | |
| 28 | 35 | // The first browse auto-registers the on-disk repo in the database, which | |
| @@ -72,6 +79,188 @@ | |||
| 72 | 79 | (meta.committer.name, meta.committer.email) | |
| 73 | 80 | } | |
| 74 | 81 | ||
| 82 | + | /// The per-repo HMAC the post-receive hook carries. | |
| 83 | + | fn push_token(owner: &str, repo: &str) -> String { | |
| 84 | + | makenotwork::build_runner::repo_hmac("test-trigger-secret", owner, repo) | |
| 85 | + | } | |
| 86 | + | ||
| 87 | + | /// Write a notes commit onto `full_ref`, the way a client's push leaves it. | |
| 88 | + | fn push_notes(tmp: &tempfile::TempDir, full_ref: &str, target: &str, body: &str) { | |
| 89 | + | use makenotwork::git::notes::{self, GixEngine, NoteObjects, NoteWrites, Oid, Signature}; | |
| 90 | + | ||
| 91 | + | let repo = gix::open(tmp.path().join("testowner").join("testrepo.git")).unwrap(); | |
| 92 | + | let engine = GixEngine::new(&repo); | |
| 93 | + | let who = Signature { | |
| 94 | + | name: "Pusher".into(), | |
| 95 | + | email: "pusher@example.com".into(), | |
| 96 | + | time: chrono::Utc::now(), | |
| 97 | + | }; | |
| 98 | + | ||
| 99 | + | let existing = engine.resolve_ref(full_ref).unwrap(); | |
| 100 | + | let root = existing.map(|tip| engine.read_commit(tip).unwrap().tree); | |
| 101 | + | let blob = engine.write_blob(body.as_bytes()).unwrap(); | |
| 102 | + | let tree = notes::splice_note( | |
| 103 | + | &engine, | |
| 104 | + | root, | |
| 105 | + | Oid::from_hex(target.as_bytes()).unwrap(), | |
| 106 | + | Some(blob), | |
| 107 | + | ) | |
| 108 | + | .unwrap() | |
| 109 | + | .expect("the push changes something"); | |
| 110 | + | let commit = engine | |
| 111 | + | .write_commit(tree, existing.as_slice(), &who, &who, "notes: pushed\n") | |
| 112 | + | .unwrap(); | |
| 113 | + | engine.update_ref_cas(full_ref, existing, commit).unwrap(); | |
| 114 | + | } | |
| 115 | + | ||
| 116 | + | fn ref_exists(tmp: &tempfile::TempDir, full_ref: &str) -> bool { | |
| 117 | + | use makenotwork::git::notes::{GixEngine, NoteObjects}; | |
| 118 | + | ||
| 119 | + | let repo = gix::open(tmp.path().join("testowner").join("testrepo.git")).unwrap(); | |
| 120 | + | GixEngine::new(&repo) | |
| 121 | + | .resolve_ref(full_ref) | |
| 122 | + | .unwrap() | |
| 123 | + | .is_some() | |
| 124 | + | } | |
| 125 | + | ||
| 126 | + | #[tokio::test] | |
| 127 | + | async fn a_pushed_inbox_is_merged_into_the_namespace_and_then_deleted() { | |
| 128 | + | let tmp = tempfile::TempDir::new().unwrap(); | |
| 129 | + | let (mut h, sha) = setup(&tmp).await; | |
| 130 | + | ||
| 131 | + | // Someone annotated through the browser; someone else annotated the same | |
| 132 | + | // commit offline and pushed. Plain git rejects the second as a | |
| 133 | + | // non-fast-forward and tells them to run a notes merge by hand. | |
| 134 | + | h.login("testowner", "password123").await; | |
| 135 | + | h.client | |
| 136 | + | .post_form( | |
| 137 | + | &format!("/git/testowner/testrepo/commit/{sha}/notes"), | |
| 138 | + | "namespace=commits&content=from+the+browser", | |
| 139 | + | ) | |
| 140 | + | .await; | |
| 141 | + | push_notes( | |
| 142 | + | &tmp, | |
| 143 | + | "refs/mnw/notes-inbox/commits", | |
| 144 | + | &sha, | |
| 145 | + | "from a laptop\n", | |
| 146 | + | ); | |
| 147 | + | ||
| 148 | + | h.client | |
| 149 | + | .set_bearer_token(&push_token("testowner", "testrepo")); | |
| 150 | + | let resp = h | |
| 151 | + | .client | |
| 152 | + | .post_json( | |
| 153 | + | "/api/internal/notes/merge-inbox", | |
| 154 | + | &serde_json::json!({ | |
| 155 | + | "repo_owner": "testowner", | |
| 156 | + | "repo_name": "testrepo", | |
| 157 | + | "ref_name": "refs/mnw/notes-inbox/commits", | |
| 158 | + | }) | |
| 159 | + | .to_string(), | |
| 160 | + | ) | |
| 161 | + | .await; | |
| 162 | + | assert_eq!(resp.status, 200, "{}", resp.text); | |
| 163 | + | assert!(resp.text.contains("\"merged\":true"), "{}", resp.text); | |
| 164 | + | ||
| 165 | + | // Neither writer lost anything, and the inbox is gone so the next push | |
| 166 | + | // starts clean. | |
| 167 | + | let note = note_in_repo(&tmp, "commits", &sha).expect("a merged note"); | |
| 168 | + | assert!(note.contains("from the browser"), "{note}"); | |
| 169 | + | assert!(note.contains("from a laptop"), "{note}"); | |
| 170 | + | assert!(!ref_exists(&tmp, "refs/mnw/notes-inbox/commits")); | |
| 171 | + | } | |
| 172 | + | ||
| 173 | + | #[tokio::test] | |
| 174 | + | async fn merging_an_inbox_that_is_already_gone_is_not_an_error() { | |
| 175 | + | let tmp = tempfile::TempDir::new().unwrap(); | |
| 176 | + | let (mut h, _sha) = setup(&tmp).await; | |
| 177 | + | ||
| 178 | + | // The hook can fire twice for one push, and a retry after a timeout is the | |
| 179 | + | // expected recovery. Neither may fail: the push already landed. | |
| 180 | + | h.client | |
| 181 | + | .set_bearer_token(&push_token("testowner", "testrepo")); | |
| 182 | + | let resp = h | |
| 183 | + | .client | |
| 184 | + | .post_json( | |
| 185 | + | "/api/internal/notes/merge-inbox", | |
| 186 | + | &serde_json::json!({ | |
| 187 | + | "repo_owner": "testowner", | |
| 188 | + | "repo_name": "testrepo", | |
| 189 | + | "ref_name": "refs/mnw/notes-inbox/commits", | |
| 190 | + | }) | |
| 191 | + | .to_string(), | |
| 192 | + | ) | |
| 193 | + | .await; | |
| 194 | + | assert_eq!(resp.status, 200, "{}", resp.text); | |
| 195 | + | assert!(resp.text.contains("\"merged\":false"), "{}", resp.text); | |
| 196 | + | } | |
| 197 | + | ||
| 198 | + | #[tokio::test] | |
| 199 | + | async fn the_inbox_endpoint_refuses_a_bad_token_and_the_reserved_namespace() { | |
| 200 | + | let tmp = tempfile::TempDir::new().unwrap(); | |
| 201 | + | let (mut h, sha) = setup(&tmp).await; | |
| 202 | + | push_notes(&tmp, "refs/mnw/notes-inbox/mnw/builds", &sha, "not yours\n"); | |
| 203 | + | ||
| 204 | + | let body = serde_json::json!({ | |
| 205 | + | "repo_owner": "testowner", | |
| 206 | + | "repo_name": "testrepo", | |
| 207 | + | "ref_name": "refs/mnw/notes-inbox/mnw/builds", | |
| 208 | + | }) | |
| 209 | + | .to_string(); | |
| 210 | + | ||
| 211 | + | // The HMAC is per repo, so one minted for a different repository must not | |
| 212 | + | // work here. | |
| 213 | + | h.client | |
| 214 | + | .set_bearer_token(&push_token("testowner", "otherrepo")); | |
| 215 | + | let resp = h | |
| 216 | + | .client | |
| 217 | + | .post_json("/api/internal/notes/merge-inbox", &body) | |
| 218 | + | .await; | |
| 219 | + | assert_eq!(resp.status, 403, "{}", resp.text); | |
| 220 | + | ||
| 221 | + | // With the right token the namespace policy still applies: pushing is not | |
| 222 | + | // a way around the server-owned prefix. | |
| 223 | + | h.client | |
| 224 | + | .set_bearer_token(&push_token("testowner", "testrepo")); | |
| 225 | + | let resp = h | |
| 226 | + | .client | |
| 227 | + | .post_json("/api/internal/notes/merge-inbox", &body) | |
| 228 | + | .await; | |
| 229 | + | assert_eq!(resp.status, 422, "{}", resp.text); | |
| 230 | + | assert_eq!(note_in_repo(&tmp, "mnw/builds", &sha), None); | |
| 231 | + | } | |
| 232 | + | ||
| 233 | + | #[test] | |
| 234 | + | fn both_copies_of_the_post_receive_hook_handle_the_inbox() { | |
| 235 | + | // The hook template exists twice, in server/src/build_runner.rs and in | |
| 236 | + | // mnw-cli/src/ssh/git.rs, and nothing but this makes them agree. A repo | |
| 237 | + | // created by one and re-hooked by the other would otherwise silently stop | |
| 238 | + | // merging notes pushes. | |
| 239 | + | let cli = std::fs::read_to_string( | |
| 240 | + | std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("../mnw-cli/src/ssh/git.rs"), | |
| 241 | + | ) | |
| 242 | + | .expect("mnw-cli lives beside the server in this repo"); | |
| 243 | + | let server = std::fs::read_to_string( | |
| 244 | + | std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("src/build_runner.rs"), | |
| 245 | + | ) | |
| 246 | + | .unwrap(); | |
| 247 | + | ||
| 248 | + | for (name, source) in [("mnw-cli", &cli), ("server", &server)] { | |
| 249 | + | assert!( | |
| 250 | + | source.contains("refs/mnw/notes-inbox/*)"), | |
| 251 | + | "{name}'s hook does not case on the notes inbox" | |
| 252 | + | ); | |
| 253 | + | assert!( | |
| 254 | + | source.contains("/api/internal/notes/merge-inbox"), | |
| 255 | + | "{name}'s hook does not call the merge endpoint" | |
| 256 | + | ); | |
| 257 | + | assert!( | |
| 258 | + | source.contains("--max-time"), | |
| 259 | + | "{name}'s inbox arm is synchronous and must bound how long a push can wait" | |
| 260 | + | ); | |
| 261 | + | } | |
| 262 | + | } | |
| 263 | + | ||
| 75 | 264 | #[tokio::test] | |
| 76 | 265 | async fn the_owner_can_add_edit_and_remove_a_note() { | |
| 77 | 266 | let tmp = tempfile::TempDir::new().unwrap(); |
| @@ -251,6 +251,17 @@ | |||
| 251 | 251 | /// Append a blob's bytes to `out`. The caller owns the buffer so a batch | |
| 252 | 252 | /// read can reuse one allocation. | |
| 253 | 253 | fn read_blob_into(&self, oid: Oid, out: &mut Vec<u8>) -> Result<(), NotesError>; | |
| 254 | + | ||
| 255 | + | /// The best common ancestor of two commits, or `None` when they share no | |
| 256 | + | /// history. | |
| 257 | + | /// | |
| 258 | + | /// A history question rather than a tree one, and the only reason it is on | |
| 259 | + | /// this trait: a three-way merge cannot be done without it, and every | |
| 260 | + | /// engine worth implementing over answers it already. `None` is an ordinary | |
| 261 | + | /// answer, not a failure — the notes inbox pushes a deliberately unrelated | |
| 262 | + | /// tip, and two notes refs that were never fetched from each other have no | |
| 263 | + | /// base either. | |
| 264 | + | fn merge_base(&self, one: Oid, two: Oid) -> Result<Option<Oid>, NotesError>; | |
| 254 | 265 | } | |
| 255 | 266 | ||
| 256 | 267 | /// The write half of the engine boundary. | |
| @@ -298,6 +309,14 @@ | |||
| 298 | 309 | expected: Option<Oid>, | |
| 299 | 310 | new: Oid, | |
| 300 | 311 | ) -> Result<(), NotesError>; | |
| 312 | + | ||
| 313 | + | /// Delete `full_name`, but only if it currently holds `expected`. | |
| 314 | + | /// | |
| 315 | + | /// Compare-and-swap here too, and `expected` is not optional: the inbox | |
| 316 | + | /// this exists for is deleted after its contents are merged, and an | |
| 317 | + | /// unconditional delete would silently discard a push that arrived while | |
| 318 | + | /// the merge was running. | |
| 319 | + | fn delete_ref_cas(&self, full_name: &str, expected: Oid) -> Result<(), NotesError>; | |
| 301 | 320 | } | |
| 302 | 321 | ||
| 303 | 322 | #[cfg(test)] |
| @@ -183,6 +183,17 @@ | |||
| 183 | 183 | Ok(()) | |
| 184 | 184 | } | |
| 185 | 185 | ||
| 186 | + | fn merge_base(&self, one: Oid, two: Oid) -> Result<Option<Oid>, NotesError> { | |
| 187 | + | // gix reports "no common ancestor" as an error variant. Here that is an | |
| 188 | + | // ordinary answer: the notes inbox is an unrelated tip on purpose, so | |
| 189 | + | // every failure to find a base reads as `None` rather than propagating. | |
| 190 | + | // A genuinely broken object graph surfaces on the next read instead. | |
| 191 | + | let Ok(base) = self.repo.merge_base(to_gix_oid(one)?, to_gix_oid(two)?) else { | |
| 192 | + | return Ok(None); | |
| 193 | + | }; | |
| 194 | + | to_oid(&base).map(Some) | |
| 195 | + | } | |
| 196 | + | ||
| 186 | 197 | fn read_blob_into(&self, oid: Oid, out: &mut Vec<u8>) -> Result<(), NotesError> { | |
| 187 | 198 | let object = self | |
| 188 | 199 | .repo | |
| @@ -261,6 +272,35 @@ | |||
| 261 | 272 | to_oid(&id) | |
| 262 | 273 | } | |
| 263 | 274 | ||
| 275 | + | fn delete_ref_cas(&self, full_name: &str, expected: Oid) -> Result<(), NotesError> { | |
| 276 | + | let edit = RefEdit { | |
| 277 | + | change: Change::Delete { | |
| 278 | + | expected: PreviousValue::MustExistAndMatch(gix::refs::Target::Object(to_gix_oid( | |
| 279 | + | expected, | |
| 280 | + | )?)), | |
| 281 | + | log: RefLog::AndReference, | |
| 282 | + | }, | |
| 283 | + | name: full_name.try_into().map_err(|e| { | |
| 284 | + | NotesError::Malformed(format!("{full_name} is not a ref name: {e}")) | |
| 285 | + | })?, | |
| 286 | + | deref: false, | |
| 287 | + | }; | |
| 288 | + | ||
| 289 | + | let Err(err) = self.repo.edit_reference(edit) else { | |
| 290 | + | return Ok(()); | |
| 291 | + | }; | |
| 292 | + | // Same reasoning as the update: the ref still holding what we expected | |
| 293 | + | // means the failure was the repository's, and anything else means we | |
| 294 | + | // lost the race and the caller should leave the ref alone. | |
| 295 | + | if self.resolve_ref(full_name)? == Some(expected) { | |
| 296 | + | Err(NotesError::Backend(format!( | |
| 297 | + | "cannot delete {full_name}: {err}" | |
| 298 | + | ))) | |
| 299 | + | } else { | |
| 300 | + | Err(NotesError::Raced) | |
| 301 | + | } | |
| 302 | + | } | |
| 303 | + | ||
| 264 | 304 | fn update_ref_cas( | |
| 265 | 305 | &self, | |
| 266 | 306 | full_name: &str, |
| @@ -20,7 +20,7 @@ | |||
| 20 | 20 | ||
| 21 | 21 | use std::collections::{HashMap, HashSet}; | |
| 22 | 22 | ||
| 23 | - | use super::engine::{NoteObjects, NoteWrites, NotesError, Oid}; | |
| 23 | + | use super::engine::{NoteObjects, NoteWrites, NotesError, Oid, Signature}; | |
| 24 | 24 | use super::{flatten, splice_note}; | |
| 25 | 25 | ||
| 26 | 26 | /// How to resolve a target both sides changed. | |
| @@ -127,6 +127,101 @@ | |||
| 127 | 127 | }) | |
| 128 | 128 | } | |
| 129 | 129 | ||
| 130 | + | /// How many times [`merge_and_publish`] reloads and retries after losing the | |
| 131 | + | /// ref race. Same reasoning as the write path's bound: every retry means | |
| 132 | + | /// another writer got there first, and a ref under enough contention to exhaust | |
| 133 | + | /// this is one to report rather than spin on. | |
| 134 | + | const MAX_PUBLISH_ATTEMPTS: usize = 5; | |
| 135 | + | ||
| 136 | + | /// What publishing a merge left behind. | |
| 137 | + | #[derive(Debug, Clone)] | |
| 138 | + | pub struct Published { | |
| 139 | + | /// The namespace's tip after the merge. `None` only when both sides were | |
| 140 | + | /// empty. | |
| 141 | + | pub tip: Option<Oid>, | |
| 142 | + | /// Targets left alone under [`MergeStrategy::Manual`]. | |
| 143 | + | pub conflicts: Vec<Oid>, | |
| 144 | + | /// Whether a commit was made. A merge that changed nothing must not add one. | |
| 145 | + | pub changed: bool, | |
| 146 | + | } | |
| 147 | + | ||
| 148 | + | /// Merge the notes commit `incoming` into the namespace at `full_ref` and | |
| 149 | + | /// publish the result, retrying against whatever a concurrent writer left. | |
| 150 | + | /// | |
| 151 | + | /// This is what makes a note push able to promise it never fails. The incoming | |
| 152 | + | /// tip is whatever a client pushed, related to the namespace or not; the merge | |
| 153 | + | /// base decides which, and no caller has to know in advance. | |
| 154 | + | /// | |
| 155 | + | /// The published commit takes both tips as parents, local first. That is what | |
| 156 | + | /// makes the merge a merge rather than an assertion: the incoming history stays | |
| 157 | + | /// reachable, so what was pushed can still be read afterwards, and running the | |
| 158 | + | /// same merge again is a no-op instead of a second interpretation. | |
| 159 | + | pub fn merge_and_publish<E: NoteObjects + NoteWrites>( | |
| 160 | + | engine: &E, | |
| 161 | + | full_ref: &str, | |
| 162 | + | incoming: Oid, | |
| 163 | + | strategy: MergeStrategy, | |
| 164 | + | who: &Signature, | |
| 165 | + | ) -> Result<Published, NotesError> { | |
| 166 | + | let incoming_tree = engine.read_commit(incoming)?.tree; | |
| 167 | + | ||
| 168 | + | for _ in 0..MAX_PUBLISH_ATTEMPTS { | |
| 169 | + | let ours_tip = engine.resolve_ref(full_ref)?; | |
| 170 | + | let ours_tree = match ours_tip { | |
| 171 | + | Some(tip) => Some(engine.read_commit(tip)?.tree), | |
| 172 | + | None => None, | |
| 173 | + | }; | |
| 174 | + | ||
| 175 | + | // No local ref, or no shared history, both mean no base — and mean the | |
| 176 | + | // same thing to the merge, which is why neither is special-cased. | |
| 177 | + | let base_tree = match ours_tip { | |
| 178 | + | Some(ours_tip) => match engine.merge_base(ours_tip, incoming)? { | |
| 179 | + | Some(base) => Some(engine.read_commit(base)?.tree), | |
| 180 | + | None => None, | |
| 181 | + | }, | |
| 182 | + | None => None, | |
| 183 | + | }; | |
| 184 | + | ||
| 185 | + | let merged = merge_notes(engine, ours_tree, Some(incoming_tree), base_tree, strategy)?; | |
| 186 | + | let Some(tree) = merged.tree.filter(|_| merged.changed) else { | |
| 187 | + | return Ok(Published { | |
| 188 | + | tip: ours_tip, | |
| 189 | + | conflicts: merged.conflicts, | |
| 190 | + | changed: false, | |
| 191 | + | }); | |
| 192 | + | }; | |
| 193 | + | ||
| 194 | + | let mut parents = Vec::with_capacity(2); | |
| 195 | + | parents.extend(ours_tip); | |
| 196 | + | parents.push(incoming); | |
| 197 | + | ||
| 198 | + | let commit = engine.write_commit( | |
| 199 | + | tree, | |
| 200 | + | &parents, | |
| 201 | + | who, | |
| 202 | + | who, | |
| 203 | + | &format!( | |
| 204 | + | "notes: merge {} into {full_ref}\n", | |
| 205 | + | incoming.to_short_hex(12) | |
| 206 | + | ), | |
| 207 | + | )?; | |
| 208 | + | ||
| 209 | + | match engine.update_ref_cas(full_ref, ours_tip, commit) { | |
| 210 | + | Ok(()) => { | |
| 211 | + | return Ok(Published { | |
| 212 | + | tip: Some(commit), | |
| 213 | + | conflicts: merged.conflicts, | |
| 214 | + | changed: true, | |
| 215 | + | }); | |
| 216 | + | } | |
| 217 | + | Err(NotesError::Raced) => {} | |
| 218 | + | Err(other) => return Err(other), | |
| 219 | + | } | |
| 220 | + | } | |
| 221 | + | ||
| 222 | + | Err(NotesError::Raced) | |
| 223 | + | } | |
| 224 | + | ||
| 130 | 225 | enum Resolution { | |
| 131 | 226 | /// The local side already says the right thing. | |
| 132 | 227 | Keep, |
| @@ -30,7 +30,7 @@ | |||
| 30 | 30 | ||
| 31 | 31 | pub use engine::{CommitMeta, NewEntry, NoteObjects, NoteWrites, NotesError, Oid, Signature}; | |
| 32 | 32 | pub use gix_engine::GixEngine; | |
| 33 | - | pub use merge::{MergeStrategy, Merged, merge_notes}; | |
| 33 | + | pub use merge::{MergeStrategy, Merged, Published, merge_and_publish, merge_notes}; | |
| 34 | 34 | ||
| 35 | 35 | use engine::{EntryKind, TreeEntry, Walk}; | |
| 36 | 36 |
| @@ -962,6 +962,222 @@ | |||
| 962 | 962 | ); | |
| 963 | 963 | } | |
| 964 | 964 | ||
| 965 | + | // ── Merging a pushed ref into a namespace ── | |
| 966 | + | ||
| 967 | + | /// Commit `tree` onto `full_ref`, the way a client's push would leave it. | |
| 968 | + | /// | |
| 969 | + | /// `parent` is the commit's parent, which is not the same as what the ref | |
| 970 | + | /// currently holds: an inbox push builds on the notes history it fetched while | |
| 971 | + | /// the inbox ref itself does not exist yet. The ref's expected value is read | |
| 972 | + | /// here rather than assumed. | |
| 973 | + | fn publish(engine: &GixEngine<'_>, full_ref: &str, tree: Option<Oid>, parent: Option<Oid>) -> Oid { | |
| 974 | + | let who = signature("Pusher"); | |
| 975 | + | let tree = tree.unwrap_or_else(|| engine.write_tree(&[]).unwrap()); | |
| 976 | + | let commit = engine | |
| 977 | + | .write_commit(tree, parent.as_slice(), &who, &who, "notes: pushed\n") | |
| 978 | + | .unwrap(); | |
| 979 | + | let expected = engine.resolve_ref(full_ref).unwrap(); | |
| 980 | + | engine | |
| 981 | + | .update_ref_cas(full_ref, expected, commit) | |
| 982 | + | .expect("the fixture publishes without contention"); | |
| 983 | + | commit | |
| 984 | + | } | |
| 985 | + | ||
| 986 | + | const INBOX: &str = "refs/mnw/notes-inbox/commits"; | |
| 987 | + | const NOTES: &str = "refs/notes/commits"; | |
| 988 | + | ||
| 989 | + | #[test] | |
| 990 | + | fn an_unrelated_push_merges_instead_of_being_rejected() { | |
| 991 | + | let (_tmp, repo) = init_bare(); | |
| 992 | + | let engine = GixEngine::new(&repo); | |
| 993 | + | ||
| 994 | + | // The shape a rejected non-fast-forward push has: two tips, no shared | |
| 995 | + | // history, both with something to say about the same commit. Git refuses | |
| 996 | + | // this and tells the pusher to run a notes merge by hand. Here it is the | |
| 997 | + | // ordinary path. | |
| 998 | + | publish( | |
| 999 | + | &engine, | |
| 1000 | + | NOTES, | |
| 1001 | + | tree_of(&engine, &[(T1, "from the server\n")]), | |
| 1002 | + | None, | |
| 1003 | + | ); | |
| 1004 | + | let incoming = publish( | |
| 1005 | + | &engine, | |
| 1006 | + | INBOX, | |
| 1007 | + | tree_of(&engine, &[(T1, "from a laptop\n"), (T3, "and another\n")]), | |
| 1008 | + | None, | |
| 1009 | + | ); | |
| 1010 | + | ||
| 1011 | + | let published = merge_and_publish( | |
| 1012 | + | &engine, | |
| 1013 | + | NOTES, | |
| 1014 | + | incoming, | |
| 1015 | + | MergeStrategy::CatSortUniq, | |
| 1016 | + | &signature("MNW"), | |
| 1017 | + | ) | |
| 1018 | + | .unwrap(); | |
| 1019 | + | ||
| 1020 | + | assert!(published.changed); | |
| 1021 | + | let tip = published.tip.unwrap(); | |
| 1022 | + | assert_eq!( | |
| 1023 | + | note_for(&engine, tip, oid(T1)) | |
| 1024 | + | .unwrap() | |
| 1025 | + | .unwrap() | |
| 1026 | + | .content_lossy(), | |
| 1027 | + | "from a laptop\nfrom the server\n", | |
| 1028 | + | "neither side may be dropped" | |
| 1029 | + | ); | |
| 1030 | + | assert_eq!( | |
| 1031 | + | note_for(&engine, tip, oid(T3)) | |
| 1032 | + | .unwrap() | |
| 1033 | + | .unwrap() | |
| 1034 | + | .content_lossy(), | |
| 1035 | + | "and another\n" | |
| 1036 | + | ); | |
| 1037 | + | ||
| 1038 | + | // Both histories stay reachable, so what was pushed can still be read and a | |
| 1039 | + | // second run of the same merge has something to recognize. | |
| 1040 | + | let parents = engine.read_commit(tip).unwrap().parents; | |
| 1041 | + | assert_eq!(parents.len(), 2); | |
| 1042 | + | assert_eq!(parents[1], incoming); | |
| 1043 | + | } | |
| 1044 | + | ||
| 1045 | + | #[test] | |
| 1046 | + | fn a_related_push_uses_the_merge_base_and_does_not_resurrect_a_deletion() { | |
| 1047 | + | let (_tmp, repo) = init_bare(); | |
| 1048 | + | let engine = GixEngine::new(&repo); | |
| 1049 | + | ||
| 1050 | + | // A client that fetched, removed a note, and pushed. The removal is not a | |
| 1051 | + | // conflict, because only one side moved away from the base — and getting | |
| 1052 | + | // this wrong would make every delete come back on the next push. | |
| 1053 | + | let base = publish( | |
| 1054 | + | &engine, | |
| 1055 | + | NOTES, | |
| 1056 | + | tree_of(&engine, &[(T1, "one\n"), (T3, "three\n")]), | |
| 1057 | + | None, | |
| 1058 | + | ); | |
| 1059 | + | let incoming = publish( | |
| 1060 | + | &engine, | |
| 1061 | + | INBOX, | |
| 1062 | + | tree_of(&engine, &[(T3, "three\n")]), | |
| 1063 | + | Some(base), | |
| 1064 | + | ); | |
| 1065 | + | ||
| 1066 | + | let published = merge_and_publish( | |
| 1067 | + | &engine, | |
| 1068 | + | NOTES, | |
| 1069 | + | incoming, | |
| 1070 | + | MergeStrategy::CatSortUniq, | |
| 1071 | + | &signature("MNW"), | |
| 1072 | + | ) | |
| 1073 | + | .unwrap(); | |
| 1074 | + | ||
| 1075 | + | let tip = published.tip.unwrap(); | |
| 1076 | + | assert!( | |
| 1077 | + | note_for(&engine, tip, oid(T1)).unwrap().is_none(), | |
| 1078 | + | "a delete the server never contested came back" | |
| 1079 | + | ); | |
| 1080 | + | assert!(note_for(&engine, tip, oid(T3)).unwrap().is_some()); | |
| 1081 | + | } | |
| 1082 | + | ||
| 1083 | + | #[test] | |
| 1084 | + | fn a_push_of_what_is_already_there_is_not_a_commit() { | |
| 1085 | + | let (_tmp, repo) = init_bare(); | |
| 1086 | + | let engine = GixEngine::new(&repo); | |
| 1087 | + | ||
| 1088 | + | let tree = tree_of(&engine, &[(T1, "same\n")]); | |
| 1089 | + | let ours = publish(&engine, NOTES, tree, None); | |
| 1090 | + | let incoming = publish(&engine, INBOX, tree, None); | |
| 1091 | + | ||
| 1092 | + | // Re-pushing unchanged notes is the common case for a client that syncs on | |
| 1093 | + | // a timer. It must not add a commit per attempt. | |
| 1094 | + | let published = merge_and_publish( | |
| 1095 | + | &engine, | |
| 1096 | + | NOTES, | |
| 1097 | + | incoming, | |
| 1098 | + | MergeStrategy::CatSortUniq, | |
| 1099 | + | &signature("MNW"), | |
| 1100 | + | ) | |
| 1101 | + | .unwrap(); | |
| 1102 | + | assert!(!published.changed); | |
| 1103 | + | assert_eq!(published.tip, Some(ours)); | |
| 1104 | + | assert_eq!(engine.resolve_ref(NOTES).unwrap(), Some(ours)); | |
| 1105 | + | } | |
| 1106 | + | ||
| 1107 | + | #[test] | |
| 1108 | + | fn a_push_into_a_namespace_nobody_has_written_lands_whole() { | |
| 1109 | + | let (_tmp, repo) = init_bare(); | |
| 1110 | + | let engine = GixEngine::new(&repo); | |
| 1111 | + | ||
| 1112 | + | let incoming = publish(&engine, INBOX, tree_of(&engine, &[(T1, "first\n")]), None); | |
| 1113 | + | let published = merge_and_publish( | |
| 1114 | + | &engine, | |
| 1115 | + | NOTES, | |
| 1116 | + | incoming, | |
| 1117 | + | MergeStrategy::CatSortUniq, | |
| 1118 | + | &signature("MNW"), | |
| 1119 | + | ) | |
| 1120 | + | .unwrap(); | |
| 1121 | + | ||
| 1122 | + | assert!(published.changed); | |
| 1123 | + | let tip = published.tip.unwrap(); | |
| 1124 | + | assert_eq!( | |
| 1125 | + | note_for(&engine, tip, oid(T1)) | |
| 1126 | + | .unwrap() | |
| 1127 | + | .unwrap() | |
| 1128 | + | .content_lossy(), | |
| 1129 | + | "first\n" | |
| 1130 | + | ); | |
| 1131 | + | // A root commit: there was no local tip to build on. | |
| 1132 | + | assert_eq!(engine.read_commit(tip).unwrap().parents, vec![incoming]); | |
| 1133 | + | } | |
| 1134 | + | ||
| 1135 | + | #[test] | |
| 1136 | + | fn deleting_the_inbox_refuses_to_discard_a_push_that_arrived_since() { | |
| 1137 | + | let (_tmp, repo) = init_bare(); | |
| 1138 | + | let engine = GixEngine::new(&repo); | |
| 1139 | + | ||
| 1140 | + | let first = publish(&engine, INBOX, tree_of(&engine, &[(T1, "first\n")]), None); | |
| 1141 | + | // A second push lands while the first is being merged. Deleting the inbox | |
| 1142 | + | // unconditionally here would throw the second one away without ever having | |
| 1143 | + | // read it, which is the one way this design could lose a note. | |
| 1144 | + | let second = publish( | |
| 1145 | + | &engine, | |
| 1146 | + | INBOX, | |
| 1147 | + | tree_of(&engine, &[(T1, "first\n"), (T3, "second\n")]), | |
| 1148 | + | Some(first), | |
| 1149 | + | ); | |
| 1150 | + | ||
| 1151 | + | let raced = engine.delete_ref_cas(INBOX, first); | |
| 1152 | + | assert!(matches!(raced, Err(NotesError::Raced)), "{raced:?}"); | |
| 1153 | + | assert_eq!(engine.resolve_ref(INBOX).unwrap(), Some(second)); | |
| 1154 | + | ||
| 1155 | + | // Against the tip it actually merged, the delete goes through. | |
| 1156 | + | engine.delete_ref_cas(INBOX, second).unwrap(); | |
| 1157 | + | assert_eq!(engine.resolve_ref(INBOX).unwrap(), None); | |
| 1158 | + | } | |
| 1159 | + | ||
| 1160 | + | #[test] | |
| 1161 | + | fn merging_the_same_push_twice_changes_nothing_the_second_time() { | |
| 1162 | + | let (_tmp, repo) = init_bare(); | |
| 1163 | + | let engine = GixEngine::new(&repo); | |
| 1164 | + | ||
| 1165 | + | publish(&engine, NOTES, tree_of(&engine, &[(T1, "ours\n")]), None); | |
| 1166 | + | let incoming = publish(&engine, INBOX, tree_of(&engine, &[(T1, "theirs\n")]), None); | |
| 1167 | + | let who = signature("MNW"); | |
| 1168 | + | ||
| 1169 | + | let first = merge_and_publish(&engine, NOTES, incoming, MergeStrategy::Union, &who).unwrap(); | |
| 1170 | + | assert!(first.changed); | |
| 1171 | + | ||
| 1172 | + | // The hook can fire twice for one push, and a retry must be a no-op rather | |
| 1173 | + | // than a second union that doubles the text. Recording the incoming tip as | |
| 1174 | + | // a parent is what makes the merge base of the retry the incoming tip | |
| 1175 | + | // itself, so the second run has nothing left to take. | |
| 1176 | + | let second = merge_and_publish(&engine, NOTES, incoming, MergeStrategy::Union, &who).unwrap(); | |
| 1177 | + | assert!(!second.changed, "a repeated merge must not re-apply"); | |
| 1178 | + | assert_eq!(second.tip, first.tip); | |
| 1179 | + | } | |
| 1180 | + | ||
| 965 | 1181 | // ── Publishing under contention ── | |
| 966 | 1182 | ||
| 967 | 1183 | /// An engine that lets somebody else publish first. | |
| @@ -1047,6 +1263,9 @@ | |||
| 1047 | 1263 | fn read_blob_into(&self, oid: Oid, out: &mut Vec<u8>) -> Result<(), NotesError> { | |
| 1048 | 1264 | self.inner.read_blob_into(oid, out) | |
| 1049 | 1265 | } | |
| 1266 | + | fn merge_base(&self, one: Oid, two: Oid) -> Result<Option<Oid>, NotesError> { | |
| 1267 | + | self.inner.merge_base(one, two) | |
| 1268 | + | } | |
| 1050 | 1269 | } | |
| 1051 | 1270 | ||
| 1052 | 1271 | impl NoteWrites for Contended<'_> { | |
| @@ -1067,6 +1286,9 @@ | |||
| 1067 | 1286 | self.inner | |
| 1068 | 1287 | .write_commit(tree, parents, author, committer, message) | |
| 1069 | 1288 | } | |
| 1289 | + | fn delete_ref_cas(&self, full_name: &str, expected: Oid) -> Result<(), NotesError> { | |
| 1290 | + | self.inner.delete_ref_cas(full_name, expected) | |
| 1291 | + | } | |
| 1070 | 1292 | fn update_ref_cas( | |
| 1071 | 1293 | &self, | |
| 1072 | 1294 | full_name: &str, |
| @@ -1,6 +1,7 @@ | |||
| 1 | 1 | //! Git source browser routes; public browsing of bare repos on disk. | |
| 2 | 2 | ||
| 3 | 3 | mod browsing; | |
| 4 | + | mod notes_inbox; | |
| 4 | 5 | pub mod notes_view; | |
| 5 | 6 | mod notes_write; | |
| 6 | 7 | mod raw; | |
| @@ -20,7 +21,7 @@ | |||
| 20 | 21 | AppState, | |
| 21 | 22 | config::Config, | |
| 22 | 23 | constants, | |
| 23 | - | csrf::{CsrfRouter, post_csrf}, | |
| 24 | + | csrf::{CsrfRouter, post_csrf, post_csrf_skip}, | |
| 24 | 25 | db::{self, DbGitRepo, DbUser, UserId, Username}, | |
| 25 | 26 | error::{AppError, Result}, | |
| 26 | 27 | git, | |
| @@ -102,6 +103,13 @@ | |||
| 102 | 103 | "/git/{owner}/{repo}/commit/{oid}/notes/delete", | |
| 103 | 104 | post_csrf(notes_write::note_delete), | |
| 104 | 105 | ) | |
| 106 | + | .route( | |
| 107 | + | "/api/internal/notes/merge-inbox", | |
| 108 | + | post_csrf_skip( | |
| 109 | + | "internal git push hook, HMAC bearer", | |
| 110 | + | notes_inbox::merge_inbox, | |
| 111 | + | ), | |
| 112 | + | ) | |
| 105 | 113 | } | |
| 106 | 114 | ||
| 107 | 115 | // Helpers |