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(),