From e98acf8c5da5033b20aee8510485a8b4a4f4a56e Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Wed, 22 Apr 2026 13:00:17 +0000 Subject: fix: prevent fatal clone/fetch errors when PR git data unavailable Only advertise `refs/heads/pr/*` branches once their tip OIDs are confirmed present locally; prevents `fatal: bad object` / `remote did not send all necessary objects` errors during clone/fetch when a PR tip lives on a different git server than the one that won the bulk prefetch race. After the bulk prefetch, collect remaining missing PR tip OIDs and do one batch fetch per repo git server using only the OIDs that server has advertised; break early once all are satisfied and skip servers that carry none. Avoids batch-poisoning (a server rejects the whole request if any single OID is absent) and redundant connections. Restrict mop-up fetches and run_fetch to the repo's declared git servers; do not fetch from clone-tag URLs in PR events - they are submitter-supplied and could let a malicious or slow server stall every clone/fetch operation. Also apply rustfmt and fix clippy warnings. --- CHANGELOG.md | 6 +++ src/bin/git_remote_nostr/fetch.rs | 65 ++++++++----------------------- src/bin/git_remote_nostr/list.rs | 82 ++++++++++++++++++++++++++++++--------- 3 files changed, 86 insertions(+), 67 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2995cd3..b3a93bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [2.4.1] - 2026-04-22 + +### Fixed + +- `fatal` errors during clone/fetch when an open PR's git data isn't available on the repository's specified git servers + ## [2.4.0] - 2026-04-10 ### Added diff --git a/src/bin/git_remote_nostr/fetch.rs b/src/bin/git_remote_nostr/fetch.rs index dd80f0f..7847777 100644 --- a/src/bin/git_remote_nostr/fetch.rs +++ b/src/bin/git_remote_nostr/fetch.rs @@ -1,17 +1,11 @@ use core::str; -use std::{ - collections::{HashMap, HashSet}, - io::Stdin, -}; +use std::{collections::HashMap, io::Stdin}; use anyhow::{Context, Result, bail}; use ngit::{ fetch::fetch_from_git_server, git::{Repo, RepoActions}, - git_events::{ - KIND_PULL_REQUEST, KIND_PULL_REQUEST_UPDATE, - identify_clone_urls_for_oids_from_pr_pr_update_events, - }, + git_events::{KIND_PULL_REQUEST, KIND_PULL_REQUEST_UPDATE}, login::get_curent_user, repo_ref::{RepoRef, is_grasp_server_in_list}, utils::{ @@ -37,56 +31,29 @@ pub async fn run_fetch( .map(|(_, oid)| oid.clone()) .collect::>(); - let pr_oid_clone_url_map = identify_clone_urls_for_oids_from_pr_pr_update_events( - fetch_batch.values().collect::>(), - git_repo, - repo_ref, - ) - .await?; - - let oids_to_fetch_from_git_servers = [ - oids_from_state.clone(), - pr_oid_clone_url_map - .keys() - .cloned() - .collect::>(), - ] - .concat(); - - let git_servers = { - let mut seen: HashSet = HashSet::new(); - let mut out: Vec = vec![]; - for server in &repo_ref.git_server { - if seen.insert(server.clone()) { - out.push(server.clone()); - } - } - for url in pr_oid_clone_url_map.values().flatten() { - if seen.insert(url.clone()) { - out.push(url.clone()); - } - } - out - }; - let mut errors = vec![]; let term = console::Term::stderr(); - for git_server_url in &git_servers { - let oids_to_fetch_from_server = oids_to_fetch_from_git_servers - .clone() - .into_iter() + // Only repo git servers are tried here. PR tip OIDs are guaranteed local by the + // list phase (see get_open_and_draft_proposals_state), so run_fetch only needs + // to resolve state OIDs that may have been missed by the bulk prefetch. + // We intentionally do not fall back to git-server URLs in PR event `clone` + // tags; see the note in get_open_and_draft_proposals_state for the + // rationale. + for git_server_url in &repo_ref.git_server { + let missing = oids_from_state + .iter() .filter(|oid| !git_repo.does_commit_exist(oid).unwrap_or(false)) + .cloned() .collect::>(); - if oids_to_fetch_from_server.is_empty() { - continue; + if missing.is_empty() { + break; } - let term = console::Term::stderr(); if let Err(error) = fetch_from_git_server( git_repo, - &oids_from_state, + &missing, git_server_url, &repo_ref.to_nostr_git_url(&None), &term, @@ -98,7 +65,7 @@ pub async fn run_fetch( if oids_from_state .iter() - .any(|oid| !git_repo.does_commit_exist(oid).unwrap()) + .any(|oid| !git_repo.does_commit_exist(oid).unwrap_or(false)) && !errors.is_empty() { bail!( diff --git a/src/bin/git_remote_nostr/list.rs b/src/bin/git_remote_nostr/list.rs index a32ed67..23b3b98 100644 --- a/src/bin/git_remote_nostr/list.rs +++ b/src/bin/git_remote_nostr/list.rs @@ -32,13 +32,14 @@ pub async fn run_list( if is_verbose() { term.write_line("git servers: listing refs...")?; } + let nostr_git_url = repo_ref.to_nostr_git_url(&None); // nostr_state is passed to list_from_remotes only for the sync-status // display; the actual ref state we advertise is determined below. let remote_states = list_from_remotes( &term, git_repo, &repo_ref.git_server, - &repo_ref.to_nostr_git_url(&None), + &nostr_git_url, nostr_state.as_ref(), ) .await; @@ -134,6 +135,7 @@ pub async fn run_list( /// fetches branches and tags from git servers so patch parent commits can be /// used to build patches with correct commit ids +#[allow(clippy::too_many_lines)] async fn get_open_and_draft_proposals_state( term: &console::Term, git_repo: &Repo, @@ -156,8 +158,6 @@ async fn get_open_and_draft_proposals_state( .filter(|v| !v.starts_with("ref: ")) .cloned() .collect::>(), - // TODO we could fetch the oids of Pull Requests and Pull Request Updates to prevent - // having repeat fetching during the git remote helper fetch phase git_server_url, &repo_ref.to_nostr_git_url(&None), term, @@ -169,8 +169,62 @@ async fn get_open_and_draft_proposals_state( } } - let mut state = HashMap::new(); let open_and_draft_proposals = get_open_or_draft_proposals(git_repo, repo_ref).await?; + + // Collect PR/PR-update tip OIDs that are still missing after the bulk prefetch. + // We borrow proposals here so we can move them in the state-building loop + // below. + let mut missing_pr_oids: Vec = open_and_draft_proposals + .values() + .filter_map(|(_, events)| { + events + .iter() + .find(|e| e.kind.eq(&KIND_PULL_REQUEST) || e.kind.eq(&KIND_PULL_REQUEST_UPDATE)) + .and_then(|e| tag_value(e, "c").ok()) + }) + .filter(|tip| !git_repo.does_commit_exist(tip).unwrap_or(false)) + .collect(); + + // For each repo git server, batch-fetch the PR tip OIDs it carries that are + // still missing locally. Only OIDs the server has advertised are included in + // each batch (avoids all-or-nothing batch-poisoning). We mop up across servers + // until all missing OIDs are satisfied or all servers are exhausted. + // + // NOTE: we intentionally restrict mop-up to the repo's declared git servers + // (remote_states) and do NOT try the git-server URL carried in the PR event's + // `clone` tag. A PR submitter could include an arbitrary server URL there; + // fetching from it unconditionally would let a malicious or slow server + // delay every clone/fetch. If we later want to support PR-supplied servers, + // it should be opt-in (e.g. an explicit `--include-pr-servers` flag) so + // users consciously accept the trust/performance trade-off. PRs whose tip + // OID isn't carried by any repo git server will simply not be advertised as + // `refs/heads/pr/*` refs; they are still accessible via their patch events. + if !missing_pr_oids.is_empty() { + for (server_url, (server_state, is_grasp)) in remote_states { + let batch: Vec = missing_pr_oids + .iter() + .filter(|oid| server_state.values().any(|v| v == *oid)) + .cloned() + .collect(); + if batch.is_empty() { + continue; + } + let _ = fetch_from_git_server( + git_repo, + &batch, + server_url, + &repo_ref.to_nostr_git_url(&None), + term, + *is_grasp, + ); + missing_pr_oids.retain(|oid| !git_repo.does_commit_exist(oid).unwrap_or(false)); + if missing_pr_oids.is_empty() { + break; + } + } + } + + let mut state = HashMap::new(); let current_user = get_curent_user(git_repo)?; for (_, (proposal, events_to_apply)) in open_and_draft_proposals { if let Ok(cl) = event_to_cover_letter(&proposal) { @@ -192,17 +246,9 @@ async fn get_open_and_draft_proposals_state( { match tag_value(pr_or_pr_update, "c") { Ok(tip) => { - // only list Pull Requests as refs/heads/pr/* if data is commit is - // advertised as tip of a ref on a repo git server or - // available locally. Otherwise the standard cmd: - // `git clone nostr://` will fail as it assumes all /refs/heads - // returned by list are accessable - let tip_oid_is_on_a_repo_git_server = - remote_states.iter().any(|(_url, (state, _is_grasp))| { - state.iter().any(|(_, oid)| tip == *oid) - }) || git_repo.does_commit_exist(&tip).is_ok_and(|r| r); - - if tip_oid_is_on_a_repo_git_server { + // Only advertise once confirmed locally available — this + // guarantees the subsequent fetch phase can serve the object. + if git_repo.does_commit_exist(&tip).is_ok_and(|r| r) { state.insert(format!("refs/heads/{branch_name}"), tip); } } @@ -254,8 +300,8 @@ async fn get_all_proposals_state( let mut state = HashMap::new(); let all_proposals = get_all_proposals(git_repo, repo_ref).await?; let current_user = get_curent_user(git_repo)?; - for (_, (proposal, events_to_apply)) in all_proposals { - if let Ok(cl) = event_to_cover_letter(&proposal) { + for (proposal, events_to_apply) in all_proposals.values() { + if let Ok(cl) = event_to_cover_letter(proposal) { if let Ok(mut branch_name) = cl.get_branch_name_with_pr_prefix_and_shorthand_id() { branch_name = if let Some(public_key) = current_user { if proposal.pubkey.eq(&public_key) { @@ -275,7 +321,7 @@ async fn get_all_proposals_state( state.insert(format!("refs/pr/{}/head", proposal.id), tip); } } else if let Ok(tip) = - make_commits_for_proposal(git_repo, repo_ref, &events_to_apply) + make_commits_for_proposal(git_repo, repo_ref, events_to_apply) { state.insert(format!("refs/{branch_name}"), tip.clone()); state.insert(format!("refs/pr/{}/head", proposal.id), tip); -- cgit v1.2.3