From cf319efc6dcdc6c54564cb84e13218edbf3643fa Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Tue, 13 Feb 2024 14:52:24 +0000 Subject: feat!: nip34 make pr event optional use first patch as thread root if pr event isn't present. begin renaming pr event to cover letter. fix patch ordering upon creation. patches were in youngest first order which caused: - `PATCH n/t`to be in reverse order - the youngest patch was the marked root - oldest patch replied to the youngest fix finding most recent patch event. when a patch in a set is the most recent it will share a created_at with other patches. previously the first patch recieved from relay in the set would be used. now it finds the first patch with that created_at which isn't also a parent of another patch with the same created_at. --- src/git.rs | 5 +- src/sub_commands/prs/create.rs | 171 +++++++++++++++++++++++--------- src/sub_commands/prs/list.rs | 215 ++++++++++++++++++++++++++--------------- src/sub_commands/pull.rs | 62 ++---------- src/sub_commands/push.rs | 64 ++++-------- 5 files changed, 291 insertions(+), 226 deletions(-) (limited to 'src') diff --git a/src/git.rs b/src/git.rs index 24afe76..cd42724 100644 --- a/src/git.rs +++ b/src/git.rs @@ -1265,6 +1265,7 @@ mod tests { &RepoRef::try_from(generate_repo_ref_event()).unwrap(), None, None, + None, ) } fn test_patch_applies_to_repository(patch_event: nostr::Event) -> Result<()> { @@ -1418,9 +1419,7 @@ mod tests { let git_repo = Repo::from_path(&original_repo.dir)?; let mut events = generate_pr_and_patch_events( - // Some(("test".to_string(), "test".to_string())), - "title", - "description", + Some(("test".to_string(), "test".to_string())), &git_repo, &vec![oid_to_sha1(&oid1), oid_to_sha1(&oid2), oid_to_sha1(&oid3)], &TEST_KEY_1_KEYS, diff --git a/src/sub_commands/prs/create.rs b/src/sub_commands/prs/create.rs index 83a3942..e5a7c1e 100644 --- a/src/sub_commands/prs/create.rs +++ b/src/sub_commands/prs/create.rs @@ -5,6 +5,7 @@ use futures::future::join_all; use indicatif::{MultiProgress, ProgressBar, ProgressStyle}; use nostr::{prelude::sha1::Hash as Sha1Hash, EventBuilder, Marker, Tag, TagKind}; +use super::list::tag_value; #[cfg(not(test))] use crate::client::Client; #[cfg(test)] @@ -32,6 +33,9 @@ pub struct SubCommandArgs { #[clap(long)] /// destination branch (defaults to main or master) to_branch: Option, + /// don't ask about a cover letter + #[arg(long, action)] + no_cover_letter: bool, } #[allow(clippy::too_many_lines)] @@ -42,7 +46,7 @@ pub async fn launch( ) -> Result<()> { let git_repo = Repo::discover().context("cannot find a git repository")?; - let (from_branch, to_branch, ahead, behind) = + let (from_branch, to_branch, mut ahead, behind) = identify_ahead_behind(&git_repo, &args.from_branch, &args.to_branch)?; if ahead.is_empty() { @@ -79,34 +83,44 @@ pub async fn launch( ); } - let title = match &args.title { - Some(t) => t.clone(), - None => Interactor::default() - .input(PromptInputParms::default().with_prompt("title"))? - .clone(), + let title = if args.no_cover_letter { + None + } else { + match &args.title { + Some(t) => Some(t.clone()), + None => { + if Interactor::default().confirm( + PromptConfirmParms::default() + .with_default(false) + .with_prompt("include cover letter?"), + )? { + Some( + Interactor::default() + .input(PromptInputParms::default().with_prompt("title"))? + .clone(), + ) + } else { + None + } + } + } }; - let description = match &args.description { - Some(t) => t.clone(), - None => Interactor::default() - .input(PromptInputParms::default().with_prompt("description (Optional)"))?, + let cover_letter_title_description = if let Some(title) = title { + Some(( + title, + if let Some(t) = &args.description { + t.clone() + } else { + Interactor::default() + .input(PromptInputParms::default().with_prompt("cover letter description"))? + .clone() + }, + )) + } else { + None }; - // let cover_letter_title_description = if let Some(title) = title { - // Some(( - // title, - // if let Some(t) = &args.description { - // t.clone() - // } else { - // Interactor::default() - // .input(PromptInputParms::default().with_prompt("cover letter - // description"))? .clone() - // }, - // )) - // } else { - // None - // }; - #[cfg(not(test))] let mut client = Client::default(); #[cfg(test)] @@ -127,10 +141,11 @@ pub async fn launch( ) .await?; + // oldest first + ahead.reverse(); + let events = generate_pr_and_patch_events( - // cover_letter_title_description, - &title, - &description, + cover_letter_title_description.clone(), &git_repo, &ahead, &keys, @@ -138,8 +153,17 @@ pub async fn launch( )?; println!( - "posting 1 pull request with {} commits...", - events.len() - 1 + "posting {} patches {} a covering letter...", + if cover_letter_title_description.is_none() { + events.len() + } else { + events.len() - 1 + }, + if cover_letter_title_description.is_none() { + "without" + } else { + "with" + } ); send_events( @@ -329,9 +353,7 @@ pub static PR_KIND: u64 = 318; pub static PATCH_KIND: u64 = 1617; pub fn generate_pr_and_patch_events( - title: &str, - description: &str, - // cover_letter_title_description: Option<(String, String)>, + cover_letter_title_description: Option<(String, String)>, git_repo: &Repo, commits: &Vec, keys: &nostr::Keys, @@ -343,8 +365,7 @@ pub fn generate_pr_and_patch_events( let mut events = vec![]; - // if let Some((title, description)) = cover_letter_title_description { - if !title.is_empty() { + if let Some((title, description)) = cover_letter_title_description { events.push(EventBuilder::new( nostr::event::Kind::Custom(PR_KIND), format!( @@ -400,6 +421,15 @@ pub fn generate_pr_and_patch_events( } else { Some(((i + 1).try_into()?, commits.len().try_into()?)) }, + if events.is_empty() { + if let Ok(branch_name) = git_repo.get_checked_out_branch_name() { + Some(branch_name) + } else { + None + } + } else { + None + }, ) .context("failed to generate patch event")?, ); @@ -410,36 +440,72 @@ pub fn generate_pr_and_patch_events( pub struct CoverLetter { pub title: String, pub description: String, - pub branch_name: Option, + pub branch_name: String, } -fn event_is_cover_letter(event: &nostr::Event) -> bool { +pub fn event_is_cover_letter(event: &nostr::Event) -> bool { event.kind.as_u64().eq(&PR_KIND) && event.iter_tags().any(|t| t.as_vec()[1].eq("cover-letter")) } pub fn event_to_cover_letter(event: &nostr::Event) -> Result { - if !event_is_cover_letter(event) { - bail!("event is not a cover letter") + if !event_is_patch_set_root(event) { + bail!("event is not a patch set root event (root patch or cover letter)") } let title_index = event .content .find("] ") - .context("event is not formatted as a cover letter patch")? + .context("event is not formatted as a patch or cover letter")? + 2; let description_index = event.content[title_index..] .find('\n') .unwrap_or(event.content.len() - 1 - title_index) + title_index; + let title = if let Ok(msg) = tag_value(event, "description") { + msg.split('\n').collect::>()[0].to_string() + } else { + event.content[title_index..description_index].to_string() + }; + + // note: if the description field is removed from patch events like in gitstr, + // then this will show entire patch. I'm not sure it is ever displayed though + let description = if let Ok(msg) = tag_value(event, "description") { + if let Some((_before, after)) = msg.split_once('\n') { + after.trim().to_string() + } else { + String::new() + } + } else { + event.content[description_index..].trim().to_string() + }; + Ok(CoverLetter { - title: event.content[title_index..description_index].to_string(), - description: event.content[description_index..].trim().to_string(), - branch_name: event - .iter_tags() - .find(|t| t.as_vec()[0].eq("branch-name")) - .map(|tag| tag.as_vec()[1].clone()), + title: title.clone(), + description, + // TODO should this be prefixed by format!("{}-"e.id.to_string()[..5]?) + branch_name: if let Ok(name) = tag_value(event, "branch-name") { + name + } else { + let s = title + .replace(' ', "-") + .chars() + .map(|c| { + if c.is_ascii_alphanumeric() || c.eq(&'/') { + c + } else { + '-' + } + }) + .collect(); + s + }, }) } +pub fn event_is_patch_set_root(event: &nostr::Event) -> bool { + (event.kind.as_u64().eq(&PR_KIND) || event.kind.as_u64().eq(&PATCH_KIND)) + && event.iter_tags().any(|t| t.as_vec()[1].eq("root")) +} + #[allow(clippy::too_many_arguments)] pub fn generate_patch_event( git_repo: &Repo, @@ -450,6 +516,7 @@ pub fn generate_patch_event( repo_ref: &RepoRef, parent_patch_event_id: Option, series_count: Option<(u64, u64)>, + branch_name: Option, ) -> Result { let commit_parent = git_repo .get_commit_parent(commit) @@ -496,6 +563,18 @@ pub fn generate_patch_event( } else { vec![] }, + if let Some(branch_name) = branch_name { + if thread_event_id.is_none() { + vec![ + Tag::Generic( + TagKind::Custom("branch-name".to_string()), + vec![branch_name.to_string()], + ) + ] + } + else { vec![]} + } + else { vec![]}, // whilst it is in nip34 draft to tag the maintainers // I'm not sure it is a good idea because if they are // interested in all patches then their specialised diff --git a/src/sub_commands/prs/list.rs b/src/sub_commands/prs/list.rs index bc85eed..36cbd02 100644 --- a/src/sub_commands/prs/list.rs +++ b/src/sub_commands/prs/list.rs @@ -1,5 +1,6 @@ use anyhow::{bail, Context, Result}; +use super::create::event_is_patch_set_root; #[cfg(not(test))] use crate::client::Client; #[cfg(test)] @@ -8,8 +9,10 @@ use crate::{ cli_interactor::{Interactor, InteractorPrompt, PromptChoiceParms, PromptConfirmParms}, client::Connect, git::{Repo, RepoActions}, - repo_ref::{self}, - sub_commands::prs::create::{event_to_cover_letter, PATCH_KIND, PR_KIND}, + repo_ref::{self, RepoRef, REPO_REF_KIND}, + sub_commands::prs::create::{ + event_is_cover_letter, event_to_cover_letter, PATCH_KIND, PR_KIND, + }, Cli, }; @@ -51,40 +54,8 @@ pub async fn launch( println!("finding PRs..."); - let pr_events: Vec = client - .get_events( - repo_ref.relays.clone(), - vec![ - nostr::Filter::default() - .kind(nostr::Kind::Custom(PR_KIND)) - .reference(format!("{root_commit}")), - ], - ) - .await? - .iter() - .filter(|e| { - e.kind.as_u64() == PR_KIND - && e.tags - .iter() - .any(|t| t.as_vec().len() > 1 && t.as_vec()[1].eq(&format!("{root_commit}"))) - }) - .map(std::borrow::ToOwned::to_owned) - .collect(); - - // let pr_branch_names: Vec = pr_events - // .iter() - // .map(|e| { - // format!( - // "{}-{}", - // &e.id.to_string()[..5], - // if let Some(t) = e.tags.iter().find(|t| t.as_vec()[0] == - // "branch-name") { t.as_vec()[1].to_string() - // } else { - // "".to_string() - // } // git_repo.get_checked_out_branch_name(), - // ) - // }) - // .collect(); + let pr_events: Vec = + find_pr_events(&client, &repo_ref, &root_commit.to_string()).await?; let selected_index = Interactor::default().choice( PromptChoiceParms::default() @@ -95,6 +66,8 @@ pub async fn launch( .map(|e| { if let Ok(cl) = event_to_cover_letter(e) { cl.title + } else if let Ok(msg) = tag_value(e, "description") { + msg.split('\n').collect::>()[0].to_string() } else { e.id.to_string() } @@ -102,49 +75,20 @@ pub async fn launch( .collect(), ), )?; - // println!("prs:{:?}", &pr_events); println!("finding commits..."); - let commits_events: Vec = client - .get_events( - repo_ref.relays.clone(), - vec![ - nostr::Filter::default() - .kind(nostr::Kind::Custom(PATCH_KIND)) - .event(pr_events[selected_index].id), - ], - ) - .await? - .iter() - .filter(|e| { - e.kind.as_u64() == PATCH_KIND - && e.tags.iter().any(|t| { - t.as_vec().len() > 2 - && t.as_vec()[1].eq(&pr_events[selected_index].id.to_string()) - }) - }) - .map(std::borrow::ToOwned::to_owned) - .collect(); + let commits_events: Vec = + find_commits_for_pr_event(&client, &pr_events[selected_index], &repo_ref).await?; confirm_checkout(&git_repo)?; let most_recent_pr_patch_chain = get_most_recent_patch_with_ancestors(commits_events) .context("cannot get most recent patch for PR")?; - let branch_name: String = if let Ok(cl) = event_to_cover_letter(&pr_events[selected_index]) { - if let Some(name) = cl.branch_name { - name - } else { - cl.title - .replace(' ', "-") - .chars() - .filter(|c| c.is_ascii_alphanumeric() || c.eq(&'/')) - .collect() - } - } else { - bail!("Placeholder not a cover letter") - }; + let branch_name: String = event_to_cover_letter(&pr_events[selected_index]) + .context("cannot assign a branch name as event is not a patch set root")? + .branch_name; let applied = git_repo .apply_patch_chain(&branch_name, most_recent_pr_patch_chain) @@ -193,20 +137,139 @@ pub fn get_most_recent_patch_with_ancestors( ) -> Result> { patches.sort_by_key(|e| e.created_at); - let mut res = vec![]; + let first_patch = patches.first().context("no patches found")?; - let latest_commit_id = tag_value(patches.first().context("no patches found")?, "commit")?; + let patches_with_youngest_created_at: Vec<&nostr::Event> = patches + .iter() + .filter(|p| p.created_at.eq(&first_patch.created_at)) + .collect(); + + let latest_commit_id = tag_value( + // get the first patch which isn't a parent of a patch event created at the same + // time + patches_with_youngest_created_at + .clone() + .iter() + .find(|p| { + if let Ok(commit) = tag_value(p, "commit") { + !patches_with_youngest_created_at.iter().any(|p2| { + if let Ok(parent) = tag_value(p2, "parent-commit") { + commit.eq(&parent) + } else { + false // skip + } + }) + } else { + false // skip + } + }) + .context("cannot find patches_with_youngest_created_at")?, + "commit", + )?; + + let mut res = vec![]; let mut commit_id_to_search = latest_commit_id; while let Some(event) = patches.iter().find(|e| { - tag_value(e, "commit") - .context("patch event doesnt contain commit tag") - .unwrap() - .eq(&commit_id_to_search) + if let Ok(commit) = tag_value(e, "commit") { + commit.eq(&commit_id_to_search) + } else { + false // skip + } }) { res.push(event.clone()); commit_id_to_search = tag_value(event, "parent-commit")?; } Ok(res) } + +pub async fn find_pr_events( + #[cfg(test)] client: &crate::client::MockConnect, + #[cfg(not(test))] client: &Client, + repo_ref: &RepoRef, + root_commit: &str, +) -> Result> { + Ok(client + .get_events( + repo_ref.relays.clone(), + vec![ + nostr::Filter::default() + .kinds(vec![ + nostr::Kind::Custom(PR_KIND), + nostr::Kind::Custom(PATCH_KIND), + ]) + .custom_tag(nostr::Alphabet::T, vec!["root"]) + .identifiers( + repo_ref + .maintainers + .iter() + .map(|m| format!("{REPO_REF_KIND}:{m}:{}", repo_ref.identifier)), + ), + // also pick up prs from the same repo but no target at our maintainers repo events + nostr::Filter::default() + .kinds(vec![ + nostr::Kind::Custom(PR_KIND), + nostr::Kind::Custom(PATCH_KIND), + ]) + .custom_tag(nostr::Alphabet::T, vec!["root"]) + .reference(root_commit), + ], + ) + .await + .context("cannot get pr events")? + .iter() + .filter(|e| { + event_is_patch_set_root(e) + && (e + .tags + .iter() + .any(|t| t.as_vec().len() > 1 && t.as_vec()[1].eq(root_commit)) + || e.tags.iter().any(|t| { + t.as_vec().len() > 1 + && repo_ref + .maintainers + .iter() + .map(|m| format!("{REPO_REF_KIND}:{m}:{}", repo_ref.identifier)) + .any(|d| t.as_vec()[1].eq(&d)) + })) + }) + .map(std::borrow::ToOwned::to_owned) + .collect::>()) +} + +pub async fn find_commits_for_pr_event( + #[cfg(test)] client: &crate::client::MockConnect, + #[cfg(not(test))] client: &Client, + pr_event: &nostr::Event, + repo_ref: &RepoRef, +) -> Result> { + let mut patch_events: Vec = client + .get_events( + repo_ref.relays.clone(), + vec![ + nostr::Filter::default() + .kind(nostr::Kind::Custom(PATCH_KIND)) + // this requires every patch to reference the root event + // this will not pick up v2,v3 patch sets + // TODO: fetch commits for v2.. patch sets + .event(pr_event.id), + ], + ) + .await + .context("cannot fetch patch events")? + .iter() + .filter(|e| { + e.kind.as_u64() == PATCH_KIND + && e.tags + .iter() + .any(|t| t.as_vec().len() > 2 && t.as_vec()[1].eq(&pr_event.id.to_string())) + }) + .map(std::borrow::ToOwned::to_owned) + .collect(); + + if !event_is_cover_letter(pr_event) { + patch_events.push(pr_event.clone()); + } + Ok(patch_events) +} diff --git a/src/sub_commands/pull.rs b/src/sub_commands/pull.rs index f3ae81f..2b20d3d 100644 --- a/src/sub_commands/pull.rs +++ b/src/sub_commands/pull.rs @@ -8,10 +8,8 @@ use crate::{ client::Connect, git::{Repo, RepoActions}, repo_ref, - repo_ref::REPO_REF_KIND, - sub_commands::prs::{ - create::{PATCH_KIND, PR_KIND}, - list::{get_most_recent_patch_with_ancestors, tag_value}, + sub_commands::{ + prs::list::get_most_recent_patch_with_ancestors, push::fetch_pr_and_most_recent_patch_chain, }, }; @@ -46,63 +44,15 @@ pub async fn launch() -> Result<()> { ) .await?; - println!("finding PR event..."); - - let pr_event: nostr::Event = client - .get_events( - repo_ref.relays.clone(), - vec![ - nostr::Filter::default() - .kind(nostr::Kind::Custom(PR_KIND)) - .identifiers( - repo_ref - .maintainers - .iter() - .map(|m| format!("{REPO_REF_KIND}:{m}:{}", repo_ref.identifier)), - ), - ], - ) - .await? - .iter() - .find(|e| { - e.kind.as_u64() == PR_KIND - && e.tags - .iter() - .any(|t| t.as_vec().len() > 1 && t.as_vec()[1].eq(&format!("{root_commit}"))) - && tag_value(e, "branch-name") - .unwrap_or_default() - .eq(&branch_name) - }) - .context("cannot find a PR event associated with the checked out branch name")? - .to_owned(); - - println!("found PR event. finding commits..."); - - let commits_events: Vec = client - .get_events( - repo_ref.relays.clone(), - vec![ - nostr::Filter::default() - .kind(nostr::Kind::Custom(PATCH_KIND)) - .event(pr_event.id), - ], - ) - .await? - .iter() - .filter(|e| { - e.kind.as_u64() == PATCH_KIND - && e.tags - .iter() - .any(|t| t.as_vec().len() > 2 && t.as_vec()[1].eq(&pr_event.id.to_string())) - }) - .map(std::borrow::ToOwned::to_owned) - .collect(); + let (_pr_event, commit_events) = + fetch_pr_and_most_recent_patch_chain(&client, &repo_ref, &root_commit, &branch_name) + .await?; if git_repo.has_outstanding_changes()? { bail!("cannot pull changes when repository is not clean. discard changes and try again."); } - let most_recent_pr_patch_chain = get_most_recent_patch_with_ancestors(commits_events) + let most_recent_pr_patch_chain = get_most_recent_patch_with_ancestors(commit_events) .context("cannot get most recent patch for PR")?; let applied = git_repo diff --git a/src/sub_commands/push.rs b/src/sub_commands/push.rs index 61d5d46..eb42699 100644 --- a/src/sub_commands/push.rs +++ b/src/sub_commands/push.rs @@ -9,10 +9,13 @@ use crate::{ client::Connect, git::{str_to_sha1, Repo, RepoActions}, login, - repo_ref::{self, RepoRef, REPO_REF_KIND}, + repo_ref::{self, RepoRef}, sub_commands::prs::{ - create::{generate_patch_event, send_events, PATCH_KIND, PR_KIND}, - list::{get_most_recent_patch_with_ancestors, tag_value}, + create::{event_to_cover_letter, generate_patch_event, send_events}, + list::{ + find_commits_for_pr_event, find_pr_events, get_most_recent_patch_with_ancestors, + tag_value, + }, }, Cli, }; @@ -111,6 +114,7 @@ pub async fn launch(cli_args: &Cli) -> Result<()> { &repo_ref, patch_events.last().map(nostr::Event::id), None, + None, ) .context("cannot make patch event from commit")?, ); @@ -131,7 +135,7 @@ pub async fn launch(cli_args: &Cli) -> Result<()> { Ok(()) } -async fn fetch_pr_and_most_recent_patch_chain( +pub async fn fetch_pr_and_most_recent_patch_chain( #[cfg(test)] client: &crate::client::MockConnect, #[cfg(not(test))] client: &Client, repo_ref: &RepoRef, @@ -140,54 +144,24 @@ async fn fetch_pr_and_most_recent_patch_chain( ) -> Result<(nostr::Event, Vec)> { println!("finding PR event..."); - let pr_event: nostr::Event = client - .get_events( - repo_ref.relays.clone(), - vec![ - nostr::Filter::default() - .kind(nostr::Kind::Custom(PR_KIND)) - .identifiers( - repo_ref - .maintainers - .iter() - .map(|m| format!("{REPO_REF_KIND}:{m}:{}", repo_ref.identifier)), - ), - ], - ) - .await? + let pr_events: Vec = find_pr_events(client, repo_ref, &root_commit.to_string()) + .await + .context("cannot get pr events for repo")?; + + let pr_event: nostr::Event = pr_events .iter() .find(|e| { - e.kind.as_u64() == PR_KIND - && e.tags - .iter() - .any(|t| t.as_vec().len() > 1 && t.as_vec()[1].eq(&format!("{root_commit}"))) - && tag_value(e, "branch-name") - .unwrap_or_default() - .eq(branch_name) + event_to_cover_letter(e).is_ok_and(|cl| cl.branch_name.eq(branch_name)) + // TODO remove the dependancy on same branch name and replace with + // references stored in .git/ngit }) .context("cannot find a PR event associated with the checked out branch name")? .to_owned(); println!("found PR event. finding commits..."); - let commits_events: Vec = client - .get_events( - repo_ref.relays.clone(), - vec![ - nostr::Filter::default() - .kind(nostr::Kind::Custom(PATCH_KIND)) - .event(pr_event.id), - ], - ) - .await? - .iter() - .filter(|e| { - e.kind.as_u64() == PATCH_KIND - && e.tags - .iter() - .any(|t| t.as_vec().len() > 2 && t.as_vec()[1].eq(&pr_event.id.to_string())) - }) - .map(std::borrow::ToOwned::to_owned) - .collect(); + let commits_events: Vec = + find_commits_for_pr_event(client, &pr_event, repo_ref).await?; + Ok((pr_event, commits_events)) } -- cgit v1.2.3