fix(remote): add resilience to `prs`

make poorly formatted patches fail silently. we stop trusting that the
`commit` tag in the latest patch can be produced by apply the patches.
to achieve this we must recreate the commit during the list command,
which require fetching the parent oids.

support patches without optional `commit` and `parent-commit` tags.

5 files changed, 207 insertions, 96 deletions

diff --git a/src/bin/git_remote_nostr/fetch.rs b/src/bin/git_remote_nostr/fetch.rs
index 2e16297..46e7ad3 100644
--- a/src/bin/git_remote_nostr/fetch.rs
+++ b/src/bin/git_remote_nostr/fetch.rs
@@ -1,11 +1,12 @@
 use core::str;
 use std::{
+    collections::HashMap,
     io::Stdin,
     sync::{Arc, Mutex},
     time::Instant,
 };
 
-use anyhow::{anyhow, bail, Result};
+use anyhow::{anyhow, bail, Context, Result};
 use auth_git2::GitAuthenticator;
 use git2::{Progress, Repository};
 use ngit::{
@@ -19,7 +20,7 @@ use ngit::{
     repo_ref::RepoRef,
 };
 use nostr::nips::nip19;
-use nostr_sdk::ToBech32;
+use nostr_sdk::{Event, ToBech32};
 
 use crate::utils::{
     count_lines_per_msg_vec, fetch_or_list_error_is_not_authentication_failure,
@@ -78,65 +79,97 @@ pub async fn run_fetch(
 
     fetch_batch.retain(|refstr, _| refstr.contains("refs/heads/pr/"));
 
-    if !fetch_batch.is_empty() {
+    fetch_proposals(git_repo, &term, repo_ref, &fetch_batch).await?;
+    term.flush()?;
+    println!();
+    Ok(())
+}
+
+pub fn make_commits_for_proposal(
+    git_repo: &Repo,
+    repo_ref: &RepoRef,
+    patches_ancestor_last: &[Event],
+) -> Result<String> {
+    let patches_ancestor_first: Vec<&Event> = patches_ancestor_last.iter().rev().collect();
+    let mut tip_commit_id = if let Ok(parent_commit) = tag_value(
+        patches_ancestor_first
+            .first()
+            .context("proposal should have at least one patch")?,
+        "parent-commit",
+    ) {
+        parent_commit
+    } else {
+        // TODO choose most recent commit on master before patch timestamp so it doesnt
+        // constantly get rebased
+        let (_, hash) = git_repo.get_main_or_master_branch()?;
+        hash.to_string()
+    };
+
+    for patch in &patches_ancestor_first {
+        let commit_id = git_repo
+            .create_commit_from_patch(patch, Some(tip_commit_id.clone()))
+            .context(format!(
+                "cannot create commit for patch {}",
+                nip19::Nip19Event {
+                    event_id: patch.id(),
+                    author: Some(patch.author()),
+                    kind: Some(patch.kind()),
+                    relays: if let Some(relay) = repo_ref.relays.first() {
+                        vec![relay.to_string()]
+                    } else {
+                        vec![]
+                    },
+                }
+                .to_bech32()
+                .unwrap_or_default()
+            ))?;
+        tip_commit_id = commit_id.to_string();
+    }
+    Ok(tip_commit_id)
+}
+
+async fn fetch_proposals(
+    git_repo: &Repo,
+    term: &console::Term,
+    repo_ref: &RepoRef,
+    proposal_refs: &HashMap<String, String>,
+) -> Result<()> {
+    if !proposal_refs.is_empty() {
         let open_proposals = get_open_proposals(git_repo, repo_ref).await?;
 
         let current_user = get_curent_user(git_repo)?;
 
-        for (refstr, oid) in fetch_batch {
+        for refstr in proposal_refs.keys() {
             if let Some((_, (_, patches))) =
-                find_proposal_and_patches_by_branch_name(&refstr, &open_proposals, &current_user)
+                find_proposal_and_patches_by_branch_name(refstr, &open_proposals, &current_user)
             {
-                if !git_repo.does_commit_exist(&oid)? {
-                    let mut patches_ancestor_first = patches.clone();
-                    patches_ancestor_first.reverse();
-                    if git_repo.does_commit_exist(&tag_value(
-                        patches_ancestor_first.first().unwrap(),
-                        "parent-commit",
-                    )?)? {
-                        for patch in &patches_ancestor_first {
-                            if let Err(error) = git_repo.create_commit_from_patch(patch) {
-                                term.write_line(
-                                    format!(
-                                        "WARNING: cannot create branch for {refstr}, error: {error} for patch {}",
-                                        nip19::Nip19Event {
-                                            event_id: patch.id(),
-                                            author: Some(patch.author()),
-                                            kind: Some(patch.kind()),
-                                            relays: if let Some(relay) = repo_ref.relays.first() {
-                                                vec![relay.to_string()]
-                                            } else { vec![]},
-                                        }.to_bech32().unwrap_or_default()
-                                    )
-                                    .as_str(),
-                                )?;
-                                break;
-                            }
-                        }
-                    } else {
-                        term.write_line(
-                            format!("WARNING: cannot find parent commit for {refstr}").as_str(),
-                        )?;
-                    }
+                if let Err(error) = make_commits_for_proposal(git_repo, repo_ref, patches) {
+                    term.write_line(
+                        format!("WARNING: cannot create branch for {refstr}, error: {error}",)
+                            .as_str(),
+                    )?;
+                    break;
                 }
-            } else {
-                term.write_line(format!("WARNING: cannot find proposal for {refstr}").as_str())?;
             }
         }
     }
-
-    term.flush()?;
-    println!();
     Ok(())
 }
 
-fn fetch_from_git_server(
+pub fn fetch_from_git_server(
     git_repo: &Repo,
     oids: &[String],
     git_server_url: &str,
     decoded_nostr_url: &NostrUrlDecoded,
     term: &console::Term,
 ) -> Result<()> {
+    let already_have_oids = oids
+        .iter()
+        .all(|oid| git_repo.does_commit_exist(oid).is_ok_and(|outcome| outcome));
+    if already_have_oids {
+        return Ok(());
+    }
+
     let server_url = git_server_url.parse::<CloneUrl>()?;
 
     let protocols_to_attempt = get_read_protocols_to_try(git_repo, &server_url, decoded_nostr_url);
diff --git a/src/bin/git_remote_nostr/list.rs b/src/bin/git_remote_nostr/list.rs
index 959b8c8..378a124 100644
--- a/src/bin/git_remote_nostr/list.rs
+++ b/src/bin/git_remote_nostr/list.rs
@@ -5,14 +5,13 @@ use anyhow::{anyhow, Context, Result};
 use auth_git2::GitAuthenticator;
 use client::get_state_from_cache;
 use git::RepoActions;
-use git_events::{event_to_cover_letter, get_commit_id_from_patch};
 use ngit::{
     client,
     git::{
         self,
         nostr_url::{CloneUrl, NostrUrlDecoded, ServerProtocol},
     },
-    git_events,
+    git_events::event_to_cover_letter,
     login::get_curent_user,
     repo_ref,
 };
@@ -20,6 +19,7 @@ use nostr_sdk::hashes::sha1::Hash as Sha1Hash;
 use repo_ref::RepoRef;
 
 use crate::{
+    fetch::{fetch_from_git_server, make_commits_for_proposal},
     git::Repo,
     utils::{
         fetch_or_list_error_is_not_authentication_failure, get_open_proposals,
@@ -88,6 +88,61 @@ pub async fn run_list(
 
     state.retain(|k, _| !k.starts_with("refs/heads/pr/"));
 
+    let proposals_state =
+        get_open_proposals_state(&term, git_repo, repo_ref, decoded_nostr_url, &remote_states)
+            .await?;
+
+    state.extend(proposals_state);
+
+    // TODO 'for push' should we check with the git servers to see if any of them
+    // allow push from the user?
+    for (name, value) in state {
+        if value.starts_with("ref: ") {
+            if !for_push {
+                println!("{} {name}", value.replace("ref: ", "@"));
+            }
+        } else {
+            println!("{value} {name}");
+        }
+    }
+
+    println!();
+    Ok(remote_states)
+}
+
+async fn get_open_proposals_state(
+    term: &console::Term,
+    git_repo: &Repo,
+    repo_ref: &RepoRef,
+    decoded_nostr_url: &NostrUrlDecoded,
+    remote_states: &HashMap<String, HashMap<String, String>>,
+) -> Result<HashMap<String, String>> {
+    // we cannot use commit_id in the latest patch in a proposal because:
+    // 1) the `commit` tag is optional
+    // 2) if the commit tag is wrong, it will cause errors which stop clone from
+    //    working
+
+    // without trusting commit_id we must apply each patch which requires the oid of
+    // the parent so we much do a fetch
+    for (git_server_url, oids_from_git_servers) in remote_states {
+        if fetch_from_git_server(
+            git_repo,
+            &oids_from_git_servers
+                .values()
+                .filter(|v| !v.starts_with("ref: "))
+                .cloned()
+                .collect::<Vec<String>>(),
+            git_server_url,
+            decoded_nostr_url,
+            term,
+        )
+        .is_ok()
+        {
+            break;
+        }
+    }
+
+    let mut state = HashMap::new();
     let open_proposals = get_open_proposals(git_repo, repo_ref).await?;
     let current_user = get_curent_user(git_repo)?;
     for (_, (proposal, patches)) in open_proposals {
@@ -102,32 +157,21 @@ pub async fn run_list(
                 } else {
                     branch_name
                 };
-                if let Some(patch) = patches.first() {
-                    // TODO this isn't resilient because the commit id stated may not be correct
-                    // we will need to check whether the commit id exists in the repo or apply the
-                    // proposal and each patch to check
-                    if let Ok(commit_id) = get_commit_id_from_patch(patch) {
-                        state.insert(format!("refs/heads/{branch_name}"), commit_id);
+                match make_commits_for_proposal(git_repo, repo_ref, &patches) {
+                    Ok(tip) => {
+                        state.insert(format!("refs/heads/{branch_name}"), tip);
                     }
-                }
-            }
-        }
-    }
-
-    // TODO 'for push' should we check with the git servers to see if any of them
-    // allow push from the user?
-    for (name, value) in state {
-        if value.starts_with("ref: ") {
-            if !for_push {
-                println!("{} {name}", value.replace("ref: ", "@"));
+                    Err(error) => {
+                        let _ = term.write_line(
+                            format!("WARNING: cannot fetch branch {branch_name} error: {error}")
+                                .as_str(),
+                        );
+                    }
+                };
             }
-        } else {
-            println!("{value} {name}");
         }
     }
-
-    println!();
-    Ok(remote_states)
+    Ok(state)
 }
 
 pub fn list_from_remotes(
diff --git a/src/lib/git/mod.rs b/src/lib/git/mod.rs
index 7ebdefd..05186b0 100644
--- a/src/lib/git/mod.rs
+++ b/src/lib/git/mod.rs
@@ -79,7 +79,11 @@ pub trait RepoActions {
         branch_name: &str,
         patch_and_ancestors: Vec<nostr::Event>,
     ) -> Result<Vec<nostr::Event>>;
-    fn create_commit_from_patch(&self, patch: &nostr::Event) -> Result<Oid>;
+    fn create_commit_from_patch(
+        &self,
+        patch: &nostr::Event,
+        parent_commit_id_override: Option<String>,
+    ) -> Result<Oid>;
     fn parse_starting_commits(&self, starting_commits: &str) -> Result<Vec<Sha1Hash>>;
     fn ancestor_of(&self, decendant: &Sha1Hash, ancestor: &Sha1Hash) -> Result<bool>;
     fn get_git_config_item(&self, item: &str, global: Option<bool>) -> Result<Option<String>>;
@@ -540,19 +544,30 @@ impl RepoActions for Repo {
             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)?;
+                self.create_commit_from_patch(patch, None)?;
             }
             self.create_branch_at_commit(branch_name, &commit_id)?;
             self.checkout(branch_name)?;
         }
         Ok(patches_to_apply)
     }
-    fn create_commit_from_patch(&self, patch: &nostr::Event) -> Result<Oid> {
-        let commit_id = get_commit_id_from_patch(patch)?;
-        if self.does_commit_exist(&commit_id)? {
-            return Ok(Oid::from_str(&commit_id)?);
+    fn create_commit_from_patch(
+        &self,
+        patch: &nostr::Event,
+        parent_commit_id_override: Option<String>,
+    ) -> Result<Oid> {
+        let commit_id = get_commit_id_from_patch(patch);
+        if let Ok(commit_id) = &commit_id {
+            if self.does_commit_exist(commit_id).unwrap_or(false) {
+                return Ok(Oid::from_str(commit_id)?);
+            }
         }
-        let parent_commit_id = tag_value(patch, "parent-commit")?;
+
+        let parent_commit_id = if let Some(commit_id) = parent_commit_id_override.clone() {
+            commit_id
+        } else {
+            tag_value(patch, "parent-commit")?
+        };
 
         let parent_commit = self
             .git_repo
@@ -604,25 +619,38 @@ impl RepoActions for Repo {
         // were identical when in a scenario when they should be different but I dont
         // think we have a test case for it. surely we should be using the
         // extract_sig_from_patch_tags outputs to address this?
-        if !applied_oid.to_string().eq(&commit_id) {
-            let commit = self.git_repo.find_commit(applied_oid)?;
-            applied_oid = commit
-                .amend(
-                    None,
-                    Some(&commit.author()),
-                    Some(&commit.committer()),
-                    None,
-                    None,
-                    None,
-                )
-                .context("cannot amend commit to produce new oid")?;
-        }
-        if !applied_oid.to_string().eq(&commit_id) {
-            bail!(
-                "when applied the patch commit id ({}) doesn't match the one specified in the event tag ({})",
-                applied_oid.to_string(),
-                get_commit_id_from_patch(patch)?,
-            );
+        let custom_parent = if let Some(ovderride_parent) = parent_commit_id_override {
+            if let Ok(tag_parent) = tag_value(patch, "parent-commit") {
+                ovderride_parent != tag_parent
+            } else {
+                true
+            }
+        } else {
+            false
+        };
+        if !custom_parent {
+            if let Ok(commit_id) = &commit_id {
+                if !applied_oid.to_string().eq(commit_id) {
+                    let commit = self.git_repo.find_commit(applied_oid)?;
+                    applied_oid = commit
+                        .amend(
+                            None,
+                            Some(&commit.author()),
+                            Some(&commit.committer()),
+                            None,
+                            None,
+                            None,
+                        )
+                        .context("cannot amend commit to produce new oid")?;
+                }
+                if !applied_oid.to_string().eq(commit_id) {
+                    bail!(
+                        "when applied the patch commit id ({}) doesn't match the one specified in the event tag ({})",
+                        applied_oid.to_string(),
+                        get_commit_id_from_patch(patch)?,
+                    );
+                }
+            }
         }
         self.git_repo.set_index(&mut existing_index)?;
         Ok(applied_oid)
@@ -1651,7 +1679,7 @@ mod tests {
             test_repo.populate()?;
             let git_repo = Repo::from_path(&test_repo.dir)?;
             println!("{:?}", &patch_event);
-            git_repo.create_commit_from_patch(&patch_event)?;
+            git_repo.create_commit_from_patch(&patch_event, None)?;
             let commit_id = tag_value(&patch_event, "commit")?;
             // does commit with id exist?
             assert!(git_repo.does_commit_exist(&commit_id)?);
diff --git a/tests/git_remote_nostr/fetch.rs b/tests/git_remote_nostr/fetch.rs
index 26d0b5b..17fd6d4 100644
--- a/tests/git_remote_nostr/fetch.rs
+++ b/tests/git_remote_nostr/fetch.rs
@@ -140,8 +140,7 @@ mod when_first_git_server_fails_ {
 #[tokio::test]
 #[serial]
 async fn creates_commits_from_open_proposal_with_no_warnings_printed() -> Result<()> {
-    let (events, source_git_repo) = prep_source_repo_and_events_including_proposals().await?;
-    let source_path = source_git_repo.dir.to_str().unwrap().to_string();
+    let (events, _) = prep_source_repo_and_events_including_proposals().await?;
 
     let (mut r51, mut r52, mut r53, mut r55, mut r56, mut r57) = (
         Relay::new(8051, None, None),
@@ -166,7 +165,6 @@ async fn creates_commits_from_open_proposal_with_no_warnings_printed() -> Result
         let mut p = cli_tester_after_fetch(&git_repo)?;
         p.send_line(format!("fetch {proposal_tip} refs/heads/{branch_name}").as_str())?;
         p.send_line("")?;
-        p.expect(format!("fetching {source_path} over filesystem...\r\n").as_str())?;
         // expect no errors
         p.expect_after_whitespace("\r\n")?;
         p.exit()?;
diff --git a/tests/git_remote_nostr/list.rs b/tests/git_remote_nostr/list.rs
index d173cc7..37cff30 100644
--- a/tests/git_remote_nostr/list.rs
+++ b/tests/git_remote_nostr/list.rs
@@ -52,6 +52,10 @@ mod without_state_announcement {
             assert_eq!(
                 res.split("\r\n")
                     .map(|e| e.to_string())
+                    .filter(|s| !s.contains("remote: ")
+                        && !s.contains("Receiving objects")
+                        && !s.contains("Resolving deltas")
+                        && !s.contains("fetching /"))
                     .collect::<HashSet<String>>(),
                 HashSet::from([
                     "@refs/heads/main HEAD".to_string(),
@@ -220,6 +224,10 @@ mod with_state_announcement {
                 assert_eq!(
                     res.split("\r\n")
                         .map(|e| e.to_string())
+                        .filter(|s| !s.contains("remote: ")
+                            && !s.contains("Receiving objects")
+                            && !s.contains("Resolving deltas")
+                            && !s.contains("fetching /"))
                         .collect::<HashSet<String>>(),
                     HashSet::from([
                         "@refs/heads/main HEAD".to_string(),