From 73d829b916d87626f33ea2adead0c48f1d9d737d Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Wed, 18 Feb 2026 21:04:59 +0000 Subject: fix: harden apply_patch_chain when optional patch tags absent - Replace .unwrap() calls in filter closure with pattern-let guards so a missing/invalid mbox commit id conservatively includes the patch - Use the OID returned by create_commit_from_patch as branch tip instead of the tag commit id, which may differ for GPG-signed commits - Add module-level doc comment to mbox_parser explaining design rationale and known limitations around the mbox envelope SHA1 --- src/lib/git/mod.rs | 72 +++++++++++++++++++++++++++++++++++--------------- src/lib/mbox_parser.rs | 62 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 21 deletions(-) (limited to 'src/lib') diff --git a/src/lib/git/mod.rs b/src/lib/git/mod.rs index 57e8403..9f86b5f 100644 --- a/src/lib/git/mod.rs +++ b/src/lib/git/mod.rs @@ -576,15 +576,23 @@ impl RepoActions for Repo { let mut patches_to_apply: Vec = patch_and_ancestors .into_iter() .filter(|e| { - let commit_id = get_commit_id_from_patch(e).unwrap(); - if let Ok(branch_tip) = branch_tip_result { - !branch_tip.to_string().eq(&commit_id) - && !self - .ancestor_of(&branch_tip, &str_to_sha1(&commit_id).unwrap()) - .unwrap() - } else { - true - } + // When the commit tag is absent, the commit id from the mbox envelope + // may not match the reconstructed commit's OID (e.g. GPG-signed commits). + // In that case we conservatively include the patch for application — + // create_commit_from_patch handles idempotency via content-addressed storage. + let Ok(commit_id) = get_commit_id_from_patch(e) else { + return true; + }; + let Ok(branch_tip) = branch_tip_result else { + return true; + }; + let Ok(commit_sha1) = str_to_sha1(&commit_id) else { + // Commit id is not a valid SHA1 (e.g. placeholder from mbox envelope). + // Include conservatively. + return true; + }; + !branch_tip.to_string().eq(&commit_id) + && !self.ancestor_of(&branch_tip, &commit_sha1).unwrap_or(false) }) .collect(); @@ -612,12 +620,26 @@ impl RepoActions for Repo { patches_to_apply.reverse(); for patch in &patches_to_apply { - let commit_id = get_commit_id_from_patch(patch)?; - // only create new commits - otherwise make them the tip - if !self.does_commit_exist(&commit_id)? { - self.create_commit_from_patch(patch, None)?; - } - self.create_branch_at_commit(branch_name, &commit_id)?; + // The commit id from the tag (or mbox envelope) is the authoritative id + // when the optional `commit` nostr tag is present. When it is absent the + // mbox envelope SHA1 is used as a best-effort value — it will often differ + // from the reconstructed commit's actual OID (e.g. GPG-signed commits). + // We therefore always use the OID returned by create_commit_from_patch as + // the branch tip, falling back to the tag commit id only when the commit + // already exists in the repo (meaning it was previously applied correctly). + let tag_commit_id = get_commit_id_from_patch(patch).ok(); + let applied_oid = if let Some(ref id) = tag_commit_id { + if self.does_commit_exist(id)? { + // Commit already exists (e.g. previously fetched), use it directly. + id.clone() + } else { + self.create_commit_from_patch(patch, None)?.to_string() + } + } else { + // No commit id available at all — apply and use the resulting OID. + self.create_commit_from_patch(patch, None)?.to_string() + }; + self.create_branch_at_commit(branch_name, &applied_oid)?; self.checkout(branch_name)?; } Ok(patches_to_apply) @@ -641,8 +663,10 @@ impl RepoActions for Repo { } else { let metadata = crate::mbox_parser::parse_mbox_patch(&patch.content) .context("failed to parse patch for timestamp")?; - let timestamp = metadata.committer_timestamp.unwrap_or(metadata.author_timestamp); - + let timestamp = metadata + .committer_timestamp + .unwrap_or(metadata.author_timestamp); + let best_guess = self .find_best_guess_parent_commit(timestamp) .context("failed to find best guess parent commit")?; @@ -684,8 +708,10 @@ impl RepoActions for Repo { None }; - let author_data = extract_signature_data_with_fallback(&patch.tags, "author", &patch.content)?; - let committer_data = extract_signature_data_with_fallback(&patch.tags, "committer", &patch.content)?; + let author_data = + extract_signature_data_with_fallback(&patch.tags, "author", &patch.content)?; + let committer_data = + extract_signature_data_with_fallback(&patch.tags, "committer", &patch.content)?; let author_sig = author_data.to_signature()?; let committer_sig = committer_data.to_signature()?; @@ -983,7 +1009,9 @@ fn extract_signature_data_from_tags(tags: &Tags, tag_name: &str) -> Result ` envelope line is extracted but +//! **must not be assumed correct**. libgit2 generates this ID from the commit +//! object, but if the original commit was GPG-signed, or if the patch was +//! generated by a different tool, the reconstructed commit (applied via +//! `apply_to_tree` + `commit_create_buffer`) will have a different OID. +//! The `commit` nostr tag is the authoritative source for commit identity when +//! present. + use anyhow::{Context, Result, bail}; use chrono::DateTime; use mailparse::{MailHeaderMap, parse_headers}; @@ -34,6 +88,14 @@ pub fn parse_mbox_patch(content: &str) -> Result { }) } +/// Extract the SHA1 from the mbox `From ` envelope line. +/// +/// **This value should not be assumed correct for the reconstructed commit.** +/// If the original commit was GPG-signed, or the patch was generated by a +/// different tool (e.g. `git format-patch` vs libgit2), the commit recreated +/// by applying this patch via `commit_create_buffer` will have a different OID. +/// Use the `commit` nostr event tag as the authoritative commit identity when +/// present. fn extract_commit_id_from_mbox(content: &str) -> Result { if !content.starts_with("From ") { bail!("patch does not start with 'From ' - not a valid mbox format"); -- cgit v1.2.3