From a75a1441b7c1cec93ebc0cb796c21360abbc5573 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Fri, 23 May 2025 21:53:35 +0100 Subject: feat(push): avoid out of sync issues for ngit relay we need to be careful with git servers with their own permissions so a ngit user doesn't inadvertantly push changes on top of a another user who pushed directly to the git server without using the force flag. We dont have this problem with ngit-relay so we can always force push, even if the user didnt as nostr is the authority of state. --- src/bin/git_remote_nostr/list.rs | 30 +++++++++++++----------------- src/bin/git_remote_nostr/push.rs | 36 ++++++++++++++++++++++++++---------- 2 files changed, 39 insertions(+), 27 deletions(-) (limited to 'src/bin') diff --git a/src/bin/git_remote_nostr/list.rs b/src/bin/git_remote_nostr/list.rs index bf16f89..ea29531 100644 --- a/src/bin/git_remote_nostr/list.rs +++ b/src/bin/git_remote_nostr/list.rs @@ -31,7 +31,7 @@ pub async fn run_list( git_repo: &Repo, repo_ref: &RepoRef, for_push: bool, -) -> Result>> { +) -> Result, bool)>> { let nostr_state = (get_state_from_cache(Some(git_repo.get_path()?), repo_ref).await).ok(); let term = console::Term::stderr(); @@ -46,7 +46,7 @@ pub async fn run_list( let mut state = if let Some(nostr_state) = nostr_state { for (name, value) in &nostr_state.state { - for (url, remote_state) in &remote_states { + for (url, (remote_state, _is_ngit_relay)) in &remote_states { let remote_name = get_short_git_server_name(git_repo, url); if let Some(remote_value) = remote_state.get(name) { if value.ne(remote_value) { @@ -74,15 +74,16 @@ pub async fn run_list( } nostr_state.state } else { - repo_ref + let (state, _is_ngit_relay) = repo_ref .git_server .iter() .filter_map(|server| remote_states.get(server)) .cloned() - .collect::>>() + .collect::, bool)>>() .first() .context("failed to get refs from git server")? - .clone() + .clone(); + state }; state.retain(|k, _| !k.starts_with("refs/heads/pr/")); @@ -112,7 +113,7 @@ async fn get_open_and_draft_proposals_state( term: &console::Term, git_repo: &Repo, repo_ref: &RepoRef, - remote_states: &HashMap>, + remote_states: &HashMap, bool)>, ) -> Result> { // we cannot use commit_id in the latest patch in a proposal because: // 1) the `commit` tag is optional @@ -121,7 +122,7 @@ async fn get_open_and_draft_proposals_state( // 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 { + for (git_server_url, (oids_from_git_servers, is_ngit_relay)) in remote_states { if fetch_from_git_server( git_repo, &oids_from_git_servers @@ -132,7 +133,7 @@ async fn get_open_and_draft_proposals_state( git_server_url, &repo_ref.to_nostr_git_url(&None), term, - is_ngit_relay(git_server_url, &repo_ref.ngit_relays()), + *is_ngit_relay, ) .is_ok() { @@ -178,22 +179,17 @@ pub fn list_from_remotes( git_servers: &Vec, decoded_nostr_url: &NostrUrlDecoded, ngit_relays: &[String], -) -> HashMap> { +) -> HashMap, bool)> { let mut remote_states = HashMap::new(); let mut errors = HashMap::new(); for url in git_servers { - match list_from_remote( - term, - git_repo, - url, - decoded_nostr_url, - is_ngit_relay(url, ngit_relays), - ) { + let is_ngit_relay = is_ngit_relay(url, ngit_relays); + match list_from_remote(term, git_repo, url, decoded_nostr_url, is_ngit_relay) { Err(error) => { errors.insert(url, error); } Ok(state) => { - remote_states.insert(url.to_string(), state); + remote_states.insert(url.to_string(), (state, is_ngit_relay)); } } } diff --git a/src/bin/git_remote_nostr/push.rs b/src/bin/git_remote_nostr/push.rs index a34d729..c4953f8 100644 --- a/src/bin/git_remote_nostr/push.rs +++ b/src/bin/git_remote_nostr/push.rs @@ -50,13 +50,14 @@ use crate::{ }; #[allow(clippy::too_many_lines)] +#[allow(clippy::type_complexity)] pub async fn run_push( git_repo: &Repo, repo_ref: &RepoRef, stdin: &Stdin, initial_refspec: &str, client: &Client, - list_outputs: Option>>, + list_outputs: Option, bool)>>, ) -> Result<()> { let refspecs = get_refspecs_from_push_batch(stdin, initial_refspec)?; @@ -93,7 +94,8 @@ pub async fn run_push( .iter() .find(|&url| list_outputs.contains_key(url)) { - list_outputs.get(url).unwrap().to_owned() + let (state, _is_ngit_relay) = list_outputs.get(url).unwrap().to_owned(); + state } else { bail!( "failed to connect to git servers: {}", @@ -706,13 +708,14 @@ fn create_rejected_refspecs_and_remotes_refspecs( git_repo: &Repo, refspecs: &Vec, nostr_state: &HashMap, - list_outputs: &HashMap>, + list_outputs: &HashMap, bool)>, ) -> Result<(HashMapUrlRefspecs, HashMapUrlRefspecs)> { let mut refspecs_for_remotes = HashMap::new(); let mut rejected_refspecs: HashMapUrlRefspecs = HashMap::new(); - for (url, remote_state) in list_outputs { + for (url, (remote_state, is_ngit_relay)) in list_outputs { + let is_ngit_relay = is_ngit_relay.to_owned(); let short_name = get_short_git_server_name(git_repo, url); let mut refspecs_for_remote = vec![]; for refspec in refspecs { @@ -747,12 +750,7 @@ fn create_rejected_refspecs_and_remotes_refspecs( if is_remote_tip_ancestor_of_commit { refspecs_for_remote.push(refspec.clone()); } else { - // this is a force push so we need to force push to git server too - if refspec.starts_with('+') { - refspecs_for_remote.push(refspec.clone()); - } else { - refspecs_for_remote.push(format!("+{refspec}")); - } + refspecs_for_remote.push(ensure_force_push_refspec(refspec)); } } else if let Ok(remote_value_tip) = git_repo.get_commit_or_tip_of_reference(remote_value) @@ -778,6 +776,9 @@ fn create_rejected_refspecs_and_remotes_refspecs( if ahead_of_nostr.is_empty() { // ancestor of nostr and we are force pushing anyway... refspecs_for_remote.push(refspec.clone()); + } else if is_ngit_relay { + // an ngit-relay can only be pushed to via nostr so can force push + refspecs_for_remote.push(ensure_force_push_refspec(refspec)); } else { rejected_refspecs .entry(refspec.to_string()) @@ -794,6 +795,8 @@ fn create_rejected_refspecs_and_remotes_refspecs( )?; } } + } else if is_ngit_relay { + refspecs_for_remote.push(ensure_force_push_refspec(refspec)); } else { // remote_value oid is not present locally // TODO can we download the remote reference? @@ -827,6 +830,8 @@ fn create_rejected_refspecs_and_remotes_refspecs( if ahead.is_empty() { // can soft push refspecs_for_remote.push(refspec.clone()); + } else if is_ngit_relay { + refspecs_for_remote.push(ensure_force_push_refspec(refspec)); } else { // cant soft push rejected_refspecs @@ -841,6 +846,8 @@ fn create_rejected_refspecs_and_remotes_refspecs( ).as_str(), )?; } + } else if is_ngit_relay { + refspecs_for_remote.push(ensure_force_push_refspec(refspec)); } else { // havn't fetched oid from remote // TODO fetch oid from remote @@ -878,6 +885,15 @@ fn create_rejected_refspecs_and_remotes_refspecs( Ok((rejected_refspecs, remotes_refspecs_without_rejected)) } +fn ensure_force_push_refspec(refspec: &str) -> String { + // Check if the refspec starts with '+' or ':' + if refspec.starts_with('+') || refspec.starts_with(':') { + refspec.to_string() // Return as is + } else { + format!("+{refspec}") // Add '+' prefix + } +} + fn generate_updated_state( git_repo: &Repo, existing_state: &HashMap, -- cgit v1.2.3