From 56b3c149df70af5d441e8527ec1225e5038bde8e Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Wed, 18 Feb 2026 22:00:35 +0000 Subject: fix: remove outdated patch_supports gate and fix fetch parent fallback Remove the patch_supports_commit_ids gates in checkout.rs and list.rs that pre-dated the mbox fallback logic. apply_patch_chain already handles all fallback cases. Also replace the main-branch TODO fallback in make_commits_for_proposal with get_parent_commit_from_patch, which uses timestamp-based best-guess when the parent-commit tag is absent. --- src/bin/ngit/sub_commands/checkout.rs | 61 ++++--------- src/bin/ngit/sub_commands/init.rs | 12 +-- src/bin/ngit/sub_commands/list.rs | 158 ++++++++++------------------------ 3 files changed, 65 insertions(+), 166 deletions(-) (limited to 'src/bin/ngit') diff --git a/src/bin/ngit/sub_commands/checkout.rs b/src/bin/ngit/sub_commands/checkout.rs index 87f1ff2..67447ae 100644 --- a/src/bin/ngit/sub_commands/checkout.rs +++ b/src/bin/ngit/sub_commands/checkout.rs @@ -24,7 +24,7 @@ use nostr_sdk::{EventId, FromBech32}; use crate::{ client::{Client, Connect, fetching_with_report, get_repo_ref_from_cache}, git::{Repo, RepoActions, str_to_sha1}, - git_events::{event_to_cover_letter, get_parent_commit_from_patch, patch_supports_commit_ids}, + git_events::event_to_cover_letter, repo_ref::get_repo_coordinates_when_remote_unknown, }; @@ -261,32 +261,7 @@ fn checkout_patch( most_recent_proposal_patch_chain_or_pr_or_pr_update: &[nostr::Event], nostr_remote_name: Option<&str>, ) -> Result<()> { - let no_support_for_patches_as_branch = most_recent_proposal_patch_chain_or_pr_or_pr_update - .iter() - .any(|event| !patch_supports_commit_ids(event)); - - if no_support_for_patches_as_branch { - bail!( - "this proposal cannot be checked out as a branch because some patches do not have a parent commit.\n\ - Try `ngit apply --stdout` to apply patches to the current branch, or use `ngit list` for interactive options." - ); - } - - let last_patch = most_recent_proposal_patch_chain_or_pr_or_pr_update - .last() - .context("there should be at least one patch")?; - - let proposal_base_commit = str_to_sha1(&get_parent_commit_from_patch(last_patch, Some(git_repo))?) - .context("failed to get valid parent commit id from patch")?; - - let (main_branch_name, _master_tip) = git_repo.get_main_or_master_branch()?; - - if !git_repo.does_commit_exist(&proposal_base_commit.to_string())? { - bail!( - "the proposal parent commit doesn't exist in your local repository.\n\ - Try running `git pull` on '{main_branch_name}' first, or use `ngit apply --stdout` to apply patches to the current branch." - ); - } + let (_, _master_tip) = git_repo.get_main_or_master_branch()?; if git_repo.has_outstanding_changes()? { bail!("working directory is not clean. Discard or stash (un)staged changes and try again."); @@ -322,23 +297,25 @@ fn checkout_patch( let local_branch_tip = git_repo.get_tip_of_branch(&branch_name)?; - let proposal_tip = str_to_sha1( - &get_commit_id_from_patch( - most_recent_proposal_patch_chain_or_pr_or_pr_update - .first() - .context("there should be at least one patch")?, - ) - .context("failed to get valid commit_id from patch")?, - ) - .context("failed to get valid commit_id from patch")?; - - if proposal_tip.eq(&local_branch_tip) { - git_repo.checkout(&branch_name)?; - println!("branch '{branch_name}' checked out and up-to-date"); - return Ok(()); + // If we can reliably determine the proposal tip commit, use it to skip + // re-applying when already up-to-date. If the commit tag is absent or + // unreliable, skip this check and let apply_patch_chain handle idempotency. + if let Ok(proposal_tip_str) = get_commit_id_from_patch( + most_recent_proposal_patch_chain_or_pr_or_pr_update + .first() + .context("there should be at least one patch")?, + ) { + if let Ok(proposal_tip) = str_to_sha1(&proposal_tip_str) { + if proposal_tip.eq(&local_branch_tip) { + git_repo.checkout(&branch_name)?; + println!("branch '{branch_name}' checked out and up-to-date"); + return Ok(()); + } + } } - git_repo.create_branch_at_commit(&branch_name, &proposal_base_commit.to_string())?; + // Branch exists but may need updating — re-apply the chain. + // apply_patch_chain handles already-applied commits idempotently. git_repo.checkout(&branch_name)?; let _ = git_repo .apply_patch_chain( diff --git a/src/bin/ngit/sub_commands/init.rs b/src/bin/ngit/sub_commands/init.rs index 75306d1..5dd6157 100644 --- a/src/bin/ngit/sub_commands/init.rs +++ b/src/bin/ngit/sub_commands/init.rs @@ -1724,10 +1724,7 @@ struct PollContext { reveal_state: Arc, } -fn create_server_bars( - clone_urls: &[String], - detail_multi: &MultiProgress, -) -> Vec { +fn create_server_bars(clone_urls: &[String], detail_multi: &MultiProgress) -> Vec { let waiting_style = ProgressStyle::with_template(" {spinner} {msg}") .unwrap() .tick_chars("⠁⠂⠄⡀⢀⠠⠐⠈"); @@ -1779,12 +1776,7 @@ fn spawn_expand_timer( }) } -fn finalize_spinner( - all_ready: bool, - spinner_pb: &ProgressBar, - final_ready: u64, - total: u64, -) { +fn finalize_spinner(all_ready: bool, spinner_pb: &ProgressBar, final_ready: u64, total: u64) { if all_ready { spinner_pb.finish_and_clear(); } else { diff --git a/src/bin/ngit/sub_commands/list.rs b/src/bin/ngit/sub_commands/list.rs index 133ac83..3d5e876 100644 --- a/src/bin/ngit/sub_commands/list.rs +++ b/src/bin/ngit/sub_commands/list.rs @@ -35,7 +35,7 @@ use crate::{ git::{Repo, RepoActions, str_to_sha1}, git_events::{ commit_msg_from_patch_oneliner, event_is_revision_root, event_to_cover_letter, - get_parent_commit_from_patch, patch_supports_commit_ids, + get_parent_commit_from_patch, }, repo_ref::get_repo_coordinates_when_remote_unknown, }; @@ -623,72 +623,6 @@ async fn launch_interactive() -> Result<()> { } } - let binding_patch_text_ref = format!( - "{} commits", - most_recent_proposal_patch_chain_or_pr_or_pr_update.len() - ); - let patch_text_ref = if most_recent_proposal_patch_chain_or_pr_or_pr_update - .len() - .gt(&1) - { - binding_patch_text_ref.as_str() - } else { - "1 commit" - }; - - let no_support_for_patches_as_branch = most_recent_proposal_patch_chain_or_pr_or_pr_update - .iter() - .any(|event| !patch_supports_commit_ids(event)); - - if no_support_for_patches_as_branch { - println!("{patch_text_ref}"); - return match Interactor::default().choice( - PromptChoiceParms::default() - .with_default(0) - .with_choices(vec![ - "learn why this proposals can't be checked out".to_string(), - format!("apply to current branch with `git am`"), - format!("download to ./patches"), - "back".to_string(), - ]), - )? { - 0 => { - println!( - "Some proposals are posted as patch without listing a parent commit\n" - ); - println!( - "they are not anchored against a particular state of the code base like a standard patch or a pull request can be\n" - ); - println!( - "they are designed to reviewed by studying the diff (in a tool like gitworkshop.dev) and if acceptable by a maintainer, applied to the latest version of master with any conflicts resolved as the do so\n" - ); - println!( - "this has proven to be a smoother workflow for large scale projects with a high frequency of changes, even when patches are exchanged via email\n" - ); - println!( - "by default ngit posts proposals with a parent commit so either workflow can be used" - ); - Interactor::default().choice( - PromptChoiceParms::default() - .with_default(0) - .with_choices(vec!["back".to_string()]), - )?; - continue; - } - 1 => { - launch_git_am_with_patches(most_recent_proposal_patch_chain_or_pr_or_pr_update) - } - 2 => save_patches_to_dir( - most_recent_proposal_patch_chain_or_pr_or_pr_update, - &git_repo, - ), - 3 => continue, - _ => { - bail!("unexpected choice") - } - }; - } - let branch_exists = git_repo .get_local_branch_names() .context("gitlib2 will not show a list of local branch names")? @@ -705,35 +639,36 @@ async fn launch_interactive() -> Result<()> { let last_patch = most_recent_proposal_patch_chain_or_pr_or_pr_update .last() - .context( - "there should be at least one patch as we have already checked for this", - )?; + .context("there should be at least one patch as we have already checked for this")?; - let proposal_base_commit = str_to_sha1(&get_parent_commit_from_patch(last_patch, Some(&git_repo))?) - .context("failed to get valid parent commit id from patch")?; + let proposal_base_commit = get_parent_commit_from_patch(last_patch, Some(&git_repo)) + .ok() + .and_then(|s| str_to_sha1(&s).ok()); let (main_branch_name, master_tip) = git_repo.get_main_or_master_branch()?; - if !git_repo.does_commit_exist(&proposal_base_commit.to_string())? { - println!("your '{main_branch_name}' branch may not be up-to-date."); - println!("the proposal parent commit doesnt exist in your local repository."); - return match Interactor::default().choice(PromptChoiceParms::default().with_default(0).with_choices( - vec![ - format!( - "manually run `git pull` on '{main_branch_name}' and select proposal again" - ), - format!("apply to current branch with `git am`"), - format!("download to ./patches"), - "back".to_string(), - ], - ))? { - 0 | 3 => continue, - 1 => launch_git_am_with_patches(most_recent_proposal_patch_chain_or_pr_or_pr_update), - 2 => save_patches_to_dir(most_recent_proposal_patch_chain_or_pr_or_pr_update, &git_repo), - _ => { - bail!("unexpected choice") - } - }; + if let Some(ref base_commit) = proposal_base_commit { + if !git_repo.does_commit_exist(&base_commit.to_string())? { + println!("your '{main_branch_name}' branch may not be up-to-date."); + println!("the proposal parent commit doesnt exist in your local repository."); + return match Interactor::default().choice(PromptChoiceParms::default().with_default(0).with_choices( + vec![ + format!( + "manually run `git pull` on '{main_branch_name}' and select proposal again" + ), + format!("apply to current branch with `git am`"), + format!("download to ./patches"), + "back".to_string(), + ], + ))? { + 0 | 3 => continue, + 1 => launch_git_am_with_patches(most_recent_proposal_patch_chain_or_pr_or_pr_update), + 2 => save_patches_to_dir(most_recent_proposal_patch_chain_or_pr_or_pr_update, &git_repo), + _ => { + bail!("unexpected choice") + } + }; + } } let proposal_tip = str_to_sha1( @@ -748,8 +683,14 @@ async fn launch_interactive() -> Result<()> { ) .context("failed to get valid commit_id from patch")?; - let (_, proposal_behind_main) = - git_repo.get_commits_ahead_behind(&master_tip, &proposal_base_commit)?; + let proposal_behind_main_len = if let Some(ref base_commit) = proposal_base_commit { + git_repo + .get_commits_ahead_behind(&master_tip, base_commit) + .map(|(_, behind)| behind.len()) + .unwrap_or(0) + } else { + 0 + }; // branch doesnt exist if !branch_exists { @@ -758,7 +699,7 @@ async fn launch_interactive() -> Result<()> { format!( "create and checkout proposal branch ({} ahead {} behind '{main_branch_name}')", most_recent_proposal_patch_chain_or_pr_or_pr_update.len(), - proposal_behind_main.len(), + proposal_behind_main_len, ), format!("apply to current branch with `git am`"), format!("download to ./patches"), @@ -815,7 +756,7 @@ async fn launch_interactive() -> Result<()> { format!( "checkout proposal branch ({} ahead {} behind '{main_branch_name}')", most_recent_proposal_patch_chain_or_pr_or_pr_update.len(), - proposal_behind_main.len(), + proposal_behind_main_len, ), format!("apply to current branch with `git am`"), format!("download to ./patches"), @@ -913,7 +854,7 @@ async fn launch_interactive() -> Result<()> { println!( "updated proposal available ({} ahead {} behind '{main_branch_name}'). existing version is {} ahead {} behind '{main_branch_name}'", most_recent_proposal_patch_chain_or_pr_or_pr_update.len(), - proposal_behind_main.len(), + proposal_behind_main_len, local_ahead_of_main.len(), local_beind_main.len(), ); @@ -930,13 +871,6 @@ async fn launch_interactive() -> Result<()> { )? { 0 => { check_clean(&git_repo)?; - git_repo.create_branch_at_commit( - &cover_letter.get_branch_name_with_pr_prefix_and_shorthand_id()?, - &proposal_base_commit.to_string(), - )?; - git_repo.checkout( - &cover_letter.get_branch_name_with_pr_prefix_and_shorthand_id()?, - )?; let chain_length = most_recent_proposal_patch_chain_or_pr_or_pr_update.len(); let _ = git_repo .apply_patch_chain( @@ -947,7 +881,7 @@ async fn launch_interactive() -> Result<()> { println!( "checked out new version of proposal ({} ahead {} behind '{main_branch_name}'), replacing old version ({} ahead {} behind '{main_branch_name}')", chain_length, - proposal_behind_main.len(), + proposal_behind_main_len, local_ahead_of_main.len(), local_beind_main.len(), ); @@ -991,7 +925,7 @@ async fn launch_interactive() -> Result<()> { "local proposal branch exists with {} unpublished commits on top of the most up-to-date version of the proposal ({} ahead {} behind '{main_branch_name}')", local_ahead_of_proposal.len(), local_ahead_of_main.len(), - proposal_behind_main.len(), + proposal_behind_main_len, ); return match Interactor::default().choice( PromptChoiceParms::default() @@ -1012,7 +946,7 @@ async fn launch_interactive() -> Result<()> { "checked out proposal branch with {} unpublished commits ({} ahead {} behind '{main_branch_name}')", local_ahead_of_proposal.len(), local_ahead_of_main.len(), - proposal_behind_main.len(), + proposal_behind_main_len, ); Ok(()) } @@ -1032,7 +966,7 @@ async fn launch_interactive() -> Result<()> { println!( "you have previously applied the latest version of the proposal ({} ahead {} behind '{main_branch_name}') but your local proposal branch has amended or rebased it ({} ahead {} behind '{main_branch_name}')", most_recent_proposal_patch_chain_or_pr_or_pr_update.len(), - proposal_behind_main.len(), + proposal_behind_main_len, local_ahead_of_main.len(), local_beind_main.len(), ); @@ -1045,7 +979,7 @@ async fn launch_interactive() -> Result<()> { local_ahead_of_main.len(), local_beind_main.len(), most_recent_proposal_patch_chain_or_pr_or_pr_update.len(), - proposal_behind_main.len(), + proposal_behind_main_len, ); println!( @@ -1085,10 +1019,6 @@ async fn launch_interactive() -> Result<()> { } 1 => { check_clean(&git_repo)?; - git_repo.create_branch_at_commit( - &cover_letter.get_branch_name_with_pr_prefix_and_shorthand_id()?, - &proposal_base_commit.to_string(), - )?; let chain_length = most_recent_proposal_patch_chain_or_pr_or_pr_update.len(); let _ = git_repo .apply_patch_chain( @@ -1102,7 +1032,7 @@ async fn launch_interactive() -> Result<()> { println!( "checked out latest version of proposal ({} ahead {} behind '{main_branch_name}'), replacing unpublished version ({} ahead {} behind '{main_branch_name}')", chain_length, - proposal_behind_main.len(), + proposal_behind_main_len, local_ahead_of_main.len(), local_beind_main.len(), ); -- cgit v1.2.3