diff --git a/crates/signed_git/src/lib.rs b/crates/signed_git/src/lib.rs index 86974df..298ce69 100644 --- a/crates/signed_git/src/lib.rs +++ b/crates/signed_git/src/lib.rs @@ -44,7 +44,11 @@ impl GitCache { } /// Open the existing clone, fetching it first. - pub fn ensure_clone(&self, addr: &RepoAddr, clone_urls: &[String]) -> Result { + pub fn ensure_clone>( + &self, + addr: &RepoAddr, + clone_urls: &[U], + ) -> Result { let path = self.repo_path(addr); if let Some(repo) = self.open(addr)? { @@ -122,7 +126,7 @@ pub fn find_git_repos(root: &Path) -> Vec { /// Clone into `path` from the first working URL in `clone_urls`. /// /// Unlike [`GitCache::ensure_clone`], the clone is not kept in any cache. -pub fn clone_repo(clone_urls: &[String], path: &Path) -> Result<()> { +pub fn clone_repo>(clone_urls: &[U], path: &Path) -> Result<()> { if path.exists() { bail!("destination {} already exists", path.display()); } @@ -347,14 +351,14 @@ fn transport_url(url: &str) -> String { /// /// Returns the last error wrapped in `failed to {verb} from any mirror`, /// or `no clone URLs provided` when the list is empty. -fn try_each_url(urls: &[String], verb: &str, mut attempt: F) -> Result<()> +fn try_each_url, F>(urls: &[U], verb: &str, mut attempt: F) -> Result<()> where F: FnMut(&str) -> Result<()>, { let mut last_err: Option = None; for url in urls { - match attempt(url) { + match attempt(url.as_ref()) { Ok(()) => return Ok(()), Err(e) => last_err = Some(e), } @@ -698,7 +702,7 @@ fn edit_local_config( /// When no URL works, the last error is returned. /// /// Never touches the checked-out refs or the worktree. -pub fn fetch_repo_refs(repo_path: &Path, urls: &[String], refspec: &str) -> Result<()> { +pub fn fetch_repo_refs>(repo_path: &Path, urls: &[U], refspec: &str) -> Result<()> { let repo = gix::open(repo_path)?; let refspec = gix::refspec::parse( gix::bstr::BStr::new(refspec), @@ -2950,9 +2954,10 @@ mod tests { #[test] fn working_copy_cloned_from_the_mirror_matches_head_and_origin() { - // The mirror is a freshly initialized repository. - // Its `origin` points at the grasp server. - // `Backend::create_repository` leaves it in the GitCache. + // The mirror is a freshly initialized repository, standing in for + // the grasp server. Its `origin` points at the (fake) grasp server. + // `GitCache::ensure_clone` lazily clones from a URL shaped like this + // the first time a repository is opened. let dir = tempfile::tempdir().expect("tempdir"); let mirror = dir.path().join("mirror"); let commit = init_repository(&mirror, "My Repo", "Does things.").expect("init"); @@ -3166,7 +3171,8 @@ mod tests { assert!(err.to_string().contains("failed to fetch")); // Without any URL there is nothing to try. - let err = fetch_repo_refs(dir, &[], "+refs/heads/*:refs/fork/x/*").expect_err("no URLs"); + let err = fetch_repo_refs(dir, &[] as &[String], "+refs/heads/*:refs/fork/x/*") + .expect_err("no URLs"); assert!(err.to_string().contains("no clone URLs")); } diff --git a/crates/signed_state/src/backend.rs b/crates/signed_state/src/backend.rs index 4168202..c34edcd 100644 --- a/crates/signed_state/src/backend.rs +++ b/crates/signed_state/src/backend.rs @@ -5,14 +5,14 @@ use std::str::FromStr; use std::sync::{Arc, Mutex}; use std::time::Duration; -use anyhow::{Context as AnyhowContext, Error, anyhow, bail}; +use anyhow::{Error, anyhow, bail}; use bitcoin_hashes::sha1::Hash as Sha1Hash; use gpui::{App, AppContext, Context, Entity, EventEmitter, Global, Task}; use nostr::event::IntoEventBuilder; use nostr_connect::prelude::*; use nostr_sdk::client::SyncSummary; use nostr_sdk::prelude::*; -use signed_core::{Announcement, RepoAddr, build_state, filters, identifier_from_name, repo_addr}; +use signed_core::{Announcement, RepoAddr, build_state, filters, identifier_from_name}; use signed_nostr::{SignedAuthUrlHandler, UniversalSigner, Update}; use crate::git_store::GitStore; @@ -420,18 +420,27 @@ impl Backend { ))); } - let addr = repo_addr(public_key, repo_id.clone()); - let cache = GitStore::global(cx).cache().clone(); - let path = cache.repo_path(&addr); let owner = public_key.to_bech32().unwrap(); let servers = grasp_servers.clone(); let client = self.client.clone(); + // Initialize directly at the user's chosen destination. + // No mirror is pre-populated: `GitCache::ensure_clone` lazily clones + // from the grasp server the first time the repo detail view needs it, + // exactly like every other repository. + let destination = { + let dir_name = signed_git::sanitize_path_component(&name); + let dir_name = if dir_name.is_empty() { + "repository".to_owned() + } else { + dir_name + }; + folder.join(dir_name) + }; + cx.spawn(async move |this, cx| { - // Initialize the local clone and create the user's working copy from it. let work = cx.background_spawn({ - let path = path.clone(); - let folder = folder.clone(); + let destination = destination.clone(); let name = name.clone(); let description = description.clone(); let owner = owner.clone(); @@ -439,53 +448,22 @@ impl Backend { let servers = servers.clone(); async move { - let parent = path - .parent() - .ok_or_else(|| anyhow!("invalid repository path"))?; - std::fs::create_dir_all(parent)?; - let commit = signed_git::init_repository(&path, &name, &description)?; - - // Point `origin` at the first grasp server. - // Later fetches and pushes have a target, like ngit. - if let Some(base) = servers.first().and_then(grasp_base_url) { - let url = format!("{base}/{owner}/{repo_id}.git"); - signed_git::ensure_origin(&path, &url).ok(); + if destination.exists() { + bail!("destination {} already exists", destination.display()); } - // A working copy at `/`, like the header's Clone action. - // Cloned from the mirror above so it shares the announced history. - // `origin` is set to the first grasp server, not the mirror path. - let destination = { - let dir_name = signed_git::sanitize_path_component(&name); - let dir_name = if dir_name.is_empty() { - "repository".to_owned() - } else { - dir_name - }; - folder.join(dir_name) - }; - - let mirror_url = Url::from_file_path(&path) - .map_err(|_| anyhow!("invalid mirror path"))? - .to_string(); - - signed_git::clone_repo(&[mirror_url], &destination).with_context(|| { - format!( - "failed to create the working copy at {}", - destination.display() - ) - })?; + let commit = signed_git::init_repository(&destination, &name, &description)?; if let Some(base) = servers.first().and_then(grasp_base_url) { let url = format!("{base}/{owner}/{repo_id}.git"); signed_git::set_origin(&destination, &url)?; } - Ok::<_, Error>((commit, destination)) + Ok::<_, Error>(commit) } }); - let (commit, checkout_path) = work.await?; + let commit = work.await?; let commit_sha = Sha1Hash::from_str(&commit).map_err(|_| anyhow!("invalid id"))?; // The nostr client queues events until each relay is connected. @@ -525,7 +503,7 @@ impl Backend { let push = cx.background_spawn({ let client = client.clone(); let signer = signer.clone(); - let path = path.clone(); + let destination = destination.clone(); let owner = owner.clone(); let repo_id = repo_id.clone(); let servers = servers.clone(); @@ -537,7 +515,7 @@ impl Backend { &repo_id, &refs, Some("main"), - &path, + &destination, &owner, &servers, signed_git::push_main, @@ -577,7 +555,7 @@ impl Backend { let announcement = Announcement::from_event(&event) .ok_or_else(|| anyhow!("failed to parse announcement"))?; - Ok((announcement, checkout_path)) + Ok((announcement, destination)) }) } diff --git a/crates/signed_state/src/repo.rs b/crates/signed_state/src/repo.rs index d516228..a5c052d 100644 --- a/crates/signed_state/src/repo.rs +++ b/crates/signed_state/src/repo.rs @@ -999,10 +999,10 @@ impl RepoStore { let cache = GitStore::global(cx).cache().clone(); let addr = self.addr.clone(); - let clone_urls: Vec = self + let clone_urls: Vec = self .announcement .as_ref() - .map(|a| a.clone.iter().map(ToString::to_string).collect()) + .map(|a| a.clone.clone()) .unwrap_or_default(); let patch = pull_request_patch(root, self.patches.iter()); @@ -1222,7 +1222,7 @@ impl RepoStore { return self.action_error("Repository announcement is not loaded yet", cx); }; - let clone_urls: Vec = announcement.clone.iter().map(ToString::to_string).collect(); + let clone_urls = announcement.clone.clone(); let addr = self.addr.clone(); self.cloning = true; diff --git a/crates/workspace/src/views/repo_detail/mod.rs b/crates/workspace/src/views/repo_detail/mod.rs index 2201f06..8f4f4dc 100644 --- a/crates/workspace/src/views/repo_detail/mod.rs +++ b/crates/workspace/src/views/repo_detail/mod.rs @@ -24,7 +24,7 @@ use gpui_component::{ ActiveTheme, Colorize, Icon, IconName, Sizable, StyledExt, ThemeStyled, VirtualListScrollHandle, h_flex, v_flex, }; -use nostr::prelude::{RelayUrl, ToBech32}; +use nostr::prelude::{RelayUrl, ToBech32, Url}; use signed_core::{Announcement, RepoAddr, RepoStatus, filters}; use signed_git::{CommitList, FileCommit}; use signed_state::{ @@ -390,7 +390,7 @@ impl RepoDetailView { let cache = GitStore::global(cx).cache().clone(); let addr = initial.addr(); - let clone_urls: Vec = initial.clone.iter().map(ToString::to_string).collect(); + let clone_urls: Vec = initial.clone.clone(); // Captured before the loads start. // A branch/tag switch bumps the generation, discarding the refresh below. diff --git a/crates/workspace/src/views/repo_detail/new_pull_request.rs b/crates/workspace/src/views/repo_detail/new_pull_request.rs index 643a01d..94e4918 100644 --- a/crates/workspace/src/views/repo_detail/new_pull_request.rs +++ b/crates/workspace/src/views/repo_detail/new_pull_request.rs @@ -591,14 +591,14 @@ impl NewPullRequestView { let cache = GitStore::global(cx).cache().clone(); let mirror_path = cache.repo_path(&base); let namespace = fork_namespace(&announcement); - let clone_urls: Vec = announcement.clone.iter().map(ToString::to_string).collect(); + let clone_urls = announcement.clone.clone(); - let base_clone_urls: Vec = self + let base_clone_urls: Vec = self .store .read(cx) .announcement .as_ref() - .map(|a| a.clone.iter().map(ToString::to_string).collect()) + .map(|a| a.clone.clone()) .unwrap_or_default(); // Keep the current compare and base when the fork is already applied. diff --git a/crates/workspace/src/views/repo_detail/pull_request_detail.rs b/crates/workspace/src/views/repo_detail/pull_request_detail.rs index 73c4755..2827c63 100644 --- a/crates/workspace/src/views/repo_detail/pull_request_detail.rs +++ b/crates/workspace/src/views/repo_detail/pull_request_detail.rs @@ -20,7 +20,7 @@ use gpui_component::{ ActiveTheme, Sizable, StyledExt, VirtualListScrollHandle, WindowExt, h_flex, v_flex, v_virtual_list, }; -use nostr::prelude::{Event, EventId, Kind, Nip34Tag}; +use nostr::prelude::{Event, EventId, Kind, Nip34Tag, Url}; use signed_core::{activity_subject, pull_request_patch}; use signed_git::{FileCommit, patch_commits, patch_diffs}; use signed_state::{Backend, GitStore, ProfileStore, RepoStore}; @@ -139,12 +139,8 @@ impl PullRequestDetailView { .and_then(merge_base_of) .or_else(|| merge_base_of(root)); - let clone_urls = clone_urls_of(root).or_else(|| { - store - .announcement - .as_ref() - .map(|a| a.clone.iter().map(ToString::to_string).collect()) - }); + let clone_urls = clone_urls_of(root) + .or_else(|| store.announcement.as_ref().map(|a| a.clone.clone())); ( root.content.clone(), @@ -730,12 +726,12 @@ fn merge_base_of(event: &Event) -> Option { /// The `clone` tag of a PR event. /// /// URLs where the proposed branch can be fetched, or `None` if the PR has none. -fn clone_urls_of(event: &Event) -> Option> { +fn clone_urls_of(event: &Event) -> Option> { event .tags .iter() .find_map(|tag| match Nip34Tag::parse(tag.as_slice()) { - Ok(Nip34Tag::Clone(urls)) => Some(urls.iter().map(ToString::to_string).collect()), + Ok(Nip34Tag::Clone(urls)) => Some(urls), _ => None, }) } diff --git a/docs/backend-rearchitecture.md b/docs/backend-rearchitecture.md index 0c9fc78..850582b 100644 --- a/docs/backend-rearchitecture.md +++ b/docs/backend-rearchitecture.md @@ -691,6 +691,24 @@ Delete `Backend::add_relays` entirely once both call sites are inlined. ## 9. `create_repository`'s flow is backwards: it inits a mirror, then clones it into the real destination +> **Status: done.** `Backend::create_repository` now computes `destination` +> (`folder.join(dir_name)`) up front and calls `signed_git::init_repository` +> directly on it — no mirror path, no `Url::from_file_path`, no `clone_repo` +> call, no double `origin` setup. An explicit `destination.exists()` check +> (mirroring what `clone_repo` used to guard for free) replaces the removed +> clone step's own guard. The push at the end now runs against `destination` +> instead of the mirror path, and the task returns `(announcement, +> destination)` as before — no caller-visible signature change. This also +> dropped `GitStore`/`GitCache::repo_path`/`repo_addr` usage from the +> function entirely, since no mirror is created there anymore; `repo_addr` +> and `Context as AnyhowContext` became unused imports in `backend.rs` and +> were removed. Updated a stale comment in `signed_git`'s +> `working_copy_cloned_from_the_mirror_matches_head_and_origin` test, which +> referenced this flow by name even though it's a generic `clone_repo` +> fixture unrelated to `Backend::create_repository`. `cargo check --workspace`, +> `cargo clippy --workspace`, and `cargo test --workspace` (signed_git 67, +> signed_state 24, workspace 14) all pass. + This is a real business-logic flaw, not just a style issue. Today (`backend.rs:437-501`): @@ -1046,6 +1064,21 @@ already exists once in the same crate. ## 15. `Vec` → `Vec` conversion sprawl — fix the 3 `signed_git` signatures, not the 8 call sites +> **Status: done.** `try_each_url`, `clone_repo`, `GitCache::ensure_clone` +> and `fetch_repo_refs` are now generic over `U: AsRef`. All 7 call +> sites (`repo.rs::merge_pull_request`/`clone_to_folder`, +> `repo_detail/mod.rs::load_repo`, `new_pull_request.rs::choose_fork` x2, +> `pull_request_detail.rs::load`/`clone_urls_of`) now pass the +> `Vec`/`Vec`-derived value straight through with a plain +> `.clone()` of the field, no `.iter().map(ToString::to_string).collect()` +> anywhere left in non-test code. One test in `signed_git` passed an empty +> untyped `&[]` literal to `fetch_repo_refs`, which lost its type-inference +> anchor once the function went generic — fixed with an explicit +> `&[] as &[String]` annotation. `cargo check --workspace`, +> `cargo clippy -p signed_state -p workspace -p signed_git --all-targets`, +> and `cargo test --workspace` (signed_git 67, signed_state 24, workspace 14) +> all pass. + `Announcement::clone` is `Vec` (`signed_core/src/model.rs`, `Url` being `nostr`'s re-export of the `url` crate's `Url`, `nostr/src/types/url.rs:15`, `pub use url::*;`). Every call site that needs to hand those URLs to @@ -1300,17 +1333,23 @@ method. `latest_grasp_list_servers`. Done: see §1. `fetch_events` no longer appears anywhere in the workspace. -5. **Generalize the 3 `signed_git` URL-list signatures** to `&[impl AsRef]` +5. ✅ **Generalize the 3 `signed_git` URL-list signatures** to `&[impl AsRef]` (§15), then delete the now-redundant `.map(ToString::to_string).collect()` at all 7 call sites. Self-contained to `signed_git`'s public API plus a one-line change per call site; re-run `signed_git`'s existing tests (`clone_repo`/`fetch_repo_refs` already have coverage). -6. **Fix `create_repository`'s init/clone ordering** (§9): initialize and + + Done: see §15 for the full list of call sites and verification notes. +6. ✅ **Fix `create_repository`'s init/clone ordering** (§9): initialize and push directly at the user's chosen destination, drop the mirror pre-population entirely and let `ensure_clone` populate it lazily like every other repo. Self-contained to one function; verify against this crate's existing `init_repository`/push tests plus a manual create-repository-then-open-detail-view pass. + + Done: see §9. Manual create-repository-then-open-detail-view pass still + recommended before shipping, since it depends on the grasp push actually + succeeding end-to-end against a live server. 7. **Merge `local_repos.rs` and `repo_list.rs` into one file** (§13), keeping both stores as independent entities. Purely organizational, zero call-site changes, safe to do any time.