From 8402b85ad9e646d96efe31d7f6b16d76f7131219 Mon Sep 17 00:00:00 2001 From: cloudhead Date: Wed, 15 May 2024 21:54:51 +0200 Subject: [PATCH] cli: Simplify private repo fetching * Remove `rad sync --force` flag * Remove `rad clone --private` flag These flags are no longer necessary. Simplify specifying which seed to fetch from with `--seed` is enough. --- radicle-cli/examples/rad-clone-connect.md | 4 +- .../examples/rad-clone-partial-fail.md | 2 +- radicle-cli/examples/rad-id-threshold.md | 2 +- .../examples/rad-init-private-clone-seed.md | 2 +- .../examples/rad-init-private-clone.md | 2 +- radicle-cli/examples/rad-init-private-seed.md | 15 +--- radicle-cli/examples/rad-sync.md | 7 ++ radicle-cli/src/commands/clone.rs | 6 +- radicle-cli/src/commands/sync.rs | 74 +++++++------------ radicle-cli/src/node.rs | 29 +------- radicle-node/src/service.rs | 8 +- radicle/src/node.rs | 5 ++ 12 files changed, 55 insertions(+), 101 deletions(-) diff --git a/radicle-cli/examples/rad-clone-connect.md b/radicle-cli/examples/rad-clone-connect.md index 4d824774..d453de5c 100644 --- a/radicle-cli/examples/rad-clone-connect.md +++ b/radicle-cli/examples/rad-clone-connect.md @@ -4,10 +4,10 @@ automatically connect to the necessary seeds. ``` $ rad clone rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji ✓ Seeding policy updated for rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji with scope 'all' -✓ Connecting to z6Mkt67…v4N1tRk@[..] -✓ Fetching rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji from z6Mkt67…v4N1tRk.. ✓ Connecting to z6MknSL…StBU8Vi@[..] ✓ Fetching rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji from z6MknSL…StBU8Vi.. +✓ Connecting to z6Mkt67…v4N1tRk@[..] +✓ Fetching rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji from z6Mkt67…v4N1tRk.. ✓ Creating checkout in ./heartwood.. ✓ Remote alice@z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi added ✓ Remote-tracking branch alice@z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi/master created for z6MknSL…StBU8Vi diff --git a/radicle-cli/examples/rad-clone-partial-fail.md b/radicle-cli/examples/rad-clone-partial-fail.md index c0270700..d3120584 100644 --- a/radicle-cli/examples/rad-clone-partial-fail.md +++ b/radicle-cli/examples/rad-clone-partial-fail.md @@ -16,8 +16,8 @@ still returns successfully. ``` $ rad clone rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji --timeout 3 ✓ Seeding policy updated for rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji with scope 'all' -✗ Fetching rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji from z6Mkt67…v4N1tRk.. error: failed to perform fetch handshake ✓ Fetching rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji from z6MknSL…StBU8Vi.. +✗ Fetching rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji from z6Mkt67…v4N1tRk.. error: failed to perform fetch handshake ✗ Connecting to z6MksFq…bS9wzpT@[..].. error: connection reset ✓ Creating checkout in ./heartwood.. ✓ Remote alice@z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi added diff --git a/radicle-cli/examples/rad-id-threshold.md b/radicle-cli/examples/rad-id-threshold.md index 54175c8c..9682ce0e 100644 --- a/radicle-cli/examples/rad-id-threshold.md +++ b/radicle-cli/examples/rad-id-threshold.md @@ -169,8 +169,8 @@ sync` and fetch his references: ``` ~bob $ rad clone rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji ✓ Seeding policy updated for rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji with scope 'all' -✓ Fetching rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji from z6MknSL…StBU8Vi.. ✓ Fetching rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji from z6Mkux1…nVhib7Z.. +✓ Fetching rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji from z6MknSL…StBU8Vi.. ✓ Creating checkout in ./heartwood.. ✓ Remote alice@z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi added ✓ Remote-tracking branch alice@z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi/master created for z6MknSL…StBU8Vi diff --git a/radicle-cli/examples/rad-init-private-clone-seed.md b/radicle-cli/examples/rad-init-private-clone-seed.md index 46e06e5b..946b511c 100644 --- a/radicle-cli/examples/rad-init-private-clone-seed.md +++ b/radicle-cli/examples/rad-init-private-clone-seed.md @@ -29,7 +29,7 @@ $ rad inspect --identity ``` ~bob $ rad ls --all --private -$ rad clone rad:z2ug5mwNKZB8KGpBDRTrWHAMbvHCu --private --seed z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi --timeout 1 +$ rad clone rad:z2ug5mwNKZB8KGpBDRTrWHAMbvHCu --seed z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi --timeout 1 ✓ Seeding policy updated for rad:z2ug5mwNKZB8KGpBDRTrWHAMbvHCu with scope 'all' ✓ Fetching rad:z2ug5mwNKZB8KGpBDRTrWHAMbvHCu from z6MknSL…StBU8Vi.. ✓ Creating checkout in ./heartwood.. diff --git a/radicle-cli/examples/rad-init-private-clone.md b/radicle-cli/examples/rad-init-private-clone.md index 599bfc23..fe57bcfc 100644 --- a/radicle-cli/examples/rad-init-private-clone.md +++ b/radicle-cli/examples/rad-init-private-clone.md @@ -5,7 +5,7 @@ Bob tries to fetch it, and even though he's connected to Alice, it fails. $ rad ls ``` ``` ~bob (fail) -$ rad clone rad:z2ug5mwNKZB8KGpBDRTrWHAMbvHCu --private --seed z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi --timeout 1 +$ rad clone rad:z2ug5mwNKZB8KGpBDRTrWHAMbvHCu --seed z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi --timeout 1 ✓ Seeding policy updated for rad:z2ug5mwNKZB8KGpBDRTrWHAMbvHCu with scope 'all' ✗ Fetching rad:z2ug5mwNKZB8KGpBDRTrWHAMbvHCu from z6MknSL…StBU8Vi.. error: failed to perform fetch handshake ✗ Error: repository rad:z2ug5mwNKZB8KGpBDRTrWHAMbvHCu not found diff --git a/radicle-cli/examples/rad-init-private-seed.md b/radicle-cli/examples/rad-init-private-seed.md index 1d99d2fd..0300faca 100644 --- a/radicle-cli/examples/rad-init-private-seed.md +++ b/radicle-cli/examples/rad-init-private-seed.md @@ -20,19 +20,10 @@ $ rad sync rad:z2ug5mwNKZB8KGpBDRTrWHAMbvHCu --fetch ✗ Error: no seeds found for rad:z2ug5mwNKZB8KGpBDRTrWHAMbvHCu ``` -If he specifies a seed that isn't in his routing table, he gets a warning and -an error: +He has to specify a seed that isn't in his routing table: ``` ~bob $ rad sync rad:z2ug5mwNKZB8KGpBDRTrWHAMbvHCu --fetch --seed z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi -! Warning: node z6MknSL…StBU8Vi is not connected or seeding.. skipping -✗ Error: no seeds found for rad:z2ug5mwNKZB8KGpBDRTrWHAMbvHCu -``` - -For this to work, Bob has to force the sync: - -``` ~bob -$ rad sync rad:z2ug5mwNKZB8KGpBDRTrWHAMbvHCu --fetch --seed z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi --force ✓ Fetching rad:z2ug5mwNKZB8KGpBDRTrWHAMbvHCu from z6MknSL…StBU8Vi.. ✓ Fetched repository from 1 seed(s) ``` @@ -50,8 +41,8 @@ Note that if multiple seeds are specified, the command succeeds as long as one seed succeeds. ``` ~bob -$ rad sync rad:z2ug5mwNKZB8KGpBDRTrWHAMbvHCu --fetch --seed z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi --seed z6MkwPUeUS2fJMfc2HZN1RQTQcTTuhw4HhPySB8JeUg2mVvx --force +$ rad sync rad:z2ug5mwNKZB8KGpBDRTrWHAMbvHCu --fetch --seed z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi --seed z6MkwPUeUS2fJMfc2HZN1RQTQcTTuhw4HhPySB8JeUg2mVvx ✓ Fetching rad:z2ug5mwNKZB8KGpBDRTrWHAMbvHCu from z6MknSL…StBU8Vi.. -✗ Fetching rad:z2ug5mwNKZB8KGpBDRTrWHAMbvHCu from z6MkwPU…Ug2mVvx.. error: session does not exist; cannot initiate fetch +✗ Fetching rad:z2ug5mwNKZB8KGpBDRTrWHAMbvHCu from z6MkwPU…Ug2mVvx.. error: peer is not connected; cannot initiate fetch ✓ Fetched repository from 1 seed(s) ``` diff --git a/radicle-cli/examples/rad-sync.md b/radicle-cli/examples/rad-sync.md index 02a237df..af3fc925 100644 --- a/radicle-cli/examples/rad-sync.md +++ b/radicle-cli/examples/rad-sync.md @@ -110,6 +110,13 @@ $ rad sync rad:z39mP9rQAaGmERfUMPULfPUi473tY ✗ Error: nothing to announce, repository rad:z39mP9rQAaGmERfUMPULfPUi473tY is not available locally ``` +Or when trying to fetch from a seed you aren't connected to, using `--seed`: +``` +$ rad sync --fetch rad:z39mP9rQAaGmERfUMPULfPUi473tY --seed z6MkjM3HpqNVV4ZsL5s3RAd8ThVG3VG98YsDCjHBNnGMq5o7 +✗ Fetching rad:z39mP9rQAaGmERfUMPULfPUi473tY from z6MkjM3…nGMq5o7.. error: peer is not connected; cannot initiate fetch +✗ Error: repository fetch from 1 seed(s) failed +``` + Also note that you cannot sync an unseeded repo: ``` $ rad unseed rad:z39mP9rQAaGmERfUMPULfPUi473tY diff --git a/radicle-cli/src/commands/clone.rs b/radicle-cli/src/commands/clone.rs index 90a88ebf..7c2b9215 100644 --- a/radicle-cli/src/commands/clone.rs +++ b/radicle-cli/src/commands/clone.rs @@ -41,14 +41,13 @@ Usage The `clone` command will use your local node's routing table to find seeds from which it can clone the repository. - For private repositories, use the `--private` and `--seed` options, to clone directly + For private repositories, use the `--seed` options, to clone directly from known seeds in the privacy set. Options --scope Follow scope: `followed` or `all` (default: all) -s, --seed Clone from this seed (may be specified multiple times) - --private Clone a private repository --timeout Timeout for fetching repository (default: 9) --help Print help @@ -91,9 +90,6 @@ impl Args for Options { scope = term::args::parse_value("scope", value)?; } - Long("private") => { - sync.force = true; - } Long("timeout") => { let value = parser.value()?; let secs = term::args::number(&value)?; diff --git a/radicle-cli/src/commands/sync.rs b/radicle-cli/src/commands/sync.rs index ad1eaa30..2b3de7ba 100644 --- a/radicle-cli/src/commands/sync.rs +++ b/radicle-cli/src/commands/sync.rs @@ -43,9 +43,6 @@ Usage When `--fetch` is specified, any number of seeds may be given using the `--seed` option, eg. `--seed @:`. - To force a fetch even if there is no route to a seed (as is the case for - private repositories), `--force` can be used. - When `--replicas` is specified, the given replication factor will try to be matched. For example, `--replicas 5` will sync with 5 seeds. @@ -65,7 +62,6 @@ Options -f, --fetch Turn on fetching (default: true) -a, --announce Turn on ref announcing (default: true) -i, --inventory Turn on inventory announcing (default: false) - --force Force fetches from unknown seeds (default: false) --timeout How many seconds to wait while syncing --seed Sync with the given node (may be specified multiple times) -r, --replicas Sync with a specific number of seeds @@ -149,7 +145,6 @@ impl Args for Options { let mut fetch = false; let mut announce = false; let mut inventory = false; - let mut force = false; let mut debug = false; let mut replicas = None; let mut seeds = BTreeSet::new(); @@ -167,9 +162,6 @@ impl Args for Options { Long("fetch") | Short('f') => { fetch = true; } - Long("force") => { - force = true; - } Long("replicas") | Short('r') => { let val = parser.value()?; let count = term::args::number(&val)?; @@ -218,10 +210,8 @@ impl Args for Options { } } - let sync = if inventory && (fetch || announce || force) { - anyhow::bail!( - "`--inventory` cannot be used with `--fetch` or `--announce` or `--force`" - ); + let sync = if inventory && (fetch || announce) { + anyhow::bail!("`--inventory` cannot be used with `--fetch` or `--announce`"); } else if inventory { SyncMode::Inventory } else { @@ -230,17 +220,14 @@ impl Args for Options { (true, false) => SyncDirection::Fetch, (false, true) => SyncDirection::Announce, }; - if direction == SyncDirection::Announce && force { - anyhow::bail!("`--force` cannot be used without `--fetch`"); - } - let settings = if seeds.is_empty() { - SyncSettings::from_replicas(replicas.unwrap_or(3)) - } else { - SyncSettings::from_seeds(seeds) - } - .timeout(timeout) - .force(force); + let mut settings = SyncSettings::default().timeout(timeout); + if let Some(r) = replicas { + settings.replicas = r; + } + if !seeds.is_empty() { + settings.seeds = seeds; + } SyncMode::Repo { settings, direction, @@ -446,33 +433,25 @@ pub fn fetch( let local = node.nid()?; // Get seeds. This consults the local routing table only. let seeds = node.seeds(rid)?; - // Target replicas, clamped by the maximum replicas possible, - // unless `force` is true. - let replicas = if settings.force { - settings.replicas - } else { - settings - .replicas - .min(seeds.iter().filter(|s| s.nid != local).count()) - }; + let replicas = settings.replicas; let mut results = FetchResults::default(); let (connected, mut disconnected) = seeds.partition(); // Fetch from specified seeds. for nid in &settings.seeds { - if !seeds.is_connected(nid) && !settings.force { - term::warning(format!( - "node {} is not connected or seeding.. skipping", - term::format::node(nid) - )); - continue; - } - let result = fetch_from(rid, nid, settings.timeout, node)?; - results.push(*nid, result); - + fetch_from(rid, nid, settings.timeout, &mut results, node)?; + // We are done when we either hit our replica count, + // or fetch from all the specified seeds. if results.success().count() >= replicas { return Ok(results); } + if settings + .seeds + .iter() + .all(|nid| results.get(nid).map_or(false, |r| r.is_success())) + { + return Ok(results); + } } // Fetch from connected seeds. @@ -486,8 +465,7 @@ pub fn fetch( let Some(nid) = connected.pop_front() else { break; }; - let result = fetch_from(rid, &nid, settings.timeout, node)?; - results.push(nid, result); + fetch_from(rid, &nid, settings.timeout, &mut results, node)?; } // Try to connect to disconnected seeds and fetch from them. @@ -505,8 +483,7 @@ pub fn fetch( settings.timeout, node, ) { - let result = fetch_from(rid, &seed.nid, settings.timeout, node)?; - results.push(seed.nid, result); + fetch_from(rid, &seed.nid, settings.timeout, &mut results, node)?; } } @@ -557,8 +534,9 @@ fn fetch_from( rid: RepoId, seed: &NodeId, timeout: time::Duration, + results: &mut FetchResults, node: &mut Node, -) -> Result { +) -> Result<(), node::Error> { let spinner = term::spinner(format!( "Fetching {} from {}..", term::format::tertiary(rid), @@ -574,7 +552,9 @@ fn fetch_from( spinner.error(reason); } } - Ok(result) + results.push(*seed, result); + + Ok(()) } fn sort_seeds_by(local: NodeId, seeds: &mut [Seed], aliases: &impl AliasStore, sort_by: &SortBy) { diff --git a/radicle-cli/src/node.rs b/radicle-cli/src/node.rs index c4c9ece2..c7f7e85e 100644 --- a/radicle-cli/src/node.rs +++ b/radicle-cli/src/node.rs @@ -22,42 +22,20 @@ pub struct SyncSettings { pub replicas: usize, /// Sync with the given list of seeds. pub seeds: BTreeSet, - /// Sync with the given seeds even if they aren't in our routing table. - /// Can be used to fetch private repositories, for example. - pub force: bool, /// How long to wait for syncing to complete. pub timeout: time::Duration, } impl SyncSettings { - /// Create a [`SyncSettings`] from a list of seeds. - pub fn from_seeds(seeds: impl IntoIterator) -> Self { - let seeds = BTreeSet::from_iter(seeds); - Self { - replicas: seeds.len(), - seeds, - force: false, - timeout: DEFAULT_SYNC_TIMEOUT, - } - } - - /// Create a [`SyncSettings`] from a replica count. - pub fn from_replicas(replicas: usize) -> Self { - Self { - replicas, - ..Self::default() - } - } - /// Set sync timeout. Defaults to [`DEFAULT_SYNC_TIMEOUT`]. pub fn timeout(mut self, timeout: time::Duration) -> Self { self.timeout = timeout; self } - /// Set the 'force' option. - pub fn force(mut self, force: bool) -> Self { - self.force = force; + /// Set replicas. + pub fn replicas(mut self, replicas: usize) -> Self { + self.replicas = replicas; self } @@ -84,7 +62,6 @@ impl Default for SyncSettings { Self { replicas: 3, seeds: BTreeSet::new(), - force: false, timeout: DEFAULT_SYNC_TIMEOUT, } } diff --git a/radicle-node/src/service.rs b/radicle-node/src/service.rs index e9668e54..9be8c60f 100644 --- a/radicle-node/src/service.rs +++ b/radicle-node/src/service.rs @@ -242,11 +242,9 @@ pub enum CommandError { enum TryFetchError<'a> { #[error("ongoing fetch for repository exists")] AlreadyFetching(&'a mut FetchState), - #[error("session does not exist; cannot initiate fetch")] - SessionNotFound, - #[error("session is not connected; cannot initiate fetch")] + #[error("peer is not connected; cannot initiate fetch")] SessionNotConnected, - #[error("session fetch capacity reached; cannot initiate fetch")] + #[error("peer fetch capacity reached; cannot initiate fetch")] SessionCapacityReached, #[error(transparent)] Namespaces(#[from] NamespacesError), @@ -997,7 +995,7 @@ where ) -> Result<&mut FetchState, TryFetchError> { let from = *from; let Some(session) = self.sessions.get_mut(&from) else { - return Err(TryFetchError::SessionNotFound); + return Err(TryFetchError::SessionNotConnected); }; let fetching = self.fetching.entry(rid); diff --git a/radicle/src/node.rs b/radicle/src/node.rs index 15d77889..0bd78673 100644 --- a/radicle/src/node.rs +++ b/radicle/src/node.rs @@ -775,6 +775,11 @@ impl FetchResults { self.0.iter().any(|(n, _)| n == nid) } + /// Get a node's result. + pub fn get(&self, nid: &NodeId) -> Option<&FetchResult> { + self.0.iter().find(|(n, _)| n == nid).map(|(_, r)| r) + } + /// Iterate over all fetch results. pub fn iter(&self) -> impl Iterator { self.0.iter().map(|(nid, r)| (nid, r))