From e9b79fb6d0436511bf91746fdf40c22665348538 Mon Sep 17 00:00:00 2001 From: cloudhead Date: Mon, 8 Apr 2024 22:14:08 +0200 Subject: [PATCH] cli: Improve UX for cloning/fetching private repos When `--private` is passed to `clone`, we force a fetch from nodes that aren't known seeds. When it isn't specified, we skip nodes that aren't known seeds, even if they are in the preferred seeds list. Previously, we would still fetch from them, which would cause errors. --- .../examples/rad-init-private-clone-seed.md | 2 +- radicle-cli/examples/rad-init-private-clone.md | 2 +- radicle-cli/src/commands/clone.rs | 16 ++++++++-------- radicle-cli/src/commands/sync.rs | 10 ++++++---- radicle-cli/src/node.rs | 14 ++++++++++++-- 5 files changed, 28 insertions(+), 16 deletions(-) diff --git a/radicle-cli/examples/rad-init-private-clone-seed.md b/radicle-cli/examples/rad-init-private-clone-seed.md index 946b511c..46e06e5b 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 --seed z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi --timeout 1 +$ rad clone rad:z2ug5mwNKZB8KGpBDRTrWHAMbvHCu --private --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 b2a35737..f4a6ca5e 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 --seed z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi --timeout 1 +$ rad clone rad:z2ug5mwNKZB8KGpBDRTrWHAMbvHCu --private --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/src/commands/clone.rs b/radicle-cli/src/commands/clone.rs index b708ae4a..90a88ebf 100644 --- a/radicle-cli/src/commands/clone.rs +++ b/radicle-cli/src/commands/clone.rs @@ -41,13 +41,14 @@ 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, the `--seed` option can be passed to clone directly - from a known seed in the privacy set. + For private repositories, use the `--private` and `--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 @@ -64,8 +65,6 @@ pub struct Options { scope: Scope, /// Sync settings. sync: SyncSettings, - /// Fetch timeout. - timeout: time::Duration, } impl Args for Options { @@ -76,7 +75,6 @@ impl Args for Options { let mut id: Option = None; let mut scope = Scope::All; let mut sync = SyncSettings::default(); - let mut timeout = time::Duration::from_secs(9); let mut directory = None; while let Some(arg) = parser.next()? { @@ -93,11 +91,14 @@ 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)?; - timeout = time::Duration::from_secs(secs as u64); + sync.timeout = time::Duration::from_secs(secs as u64); } Long("no-confirm") => { // We keep this flag here for consistency though it doesn't have any effect, @@ -129,7 +130,6 @@ impl Args for Options { directory, scope, sync, - timeout, }, vec![], )) @@ -151,7 +151,7 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> { options.id, options.directory.clone(), options.scope, - options.sync.with_profile(&profile).timeout(options.timeout), + options.sync.with_profile(&profile), &mut node, &signer, &profile.storage, diff --git a/radicle-cli/src/commands/sync.rs b/radicle-cli/src/commands/sync.rs index 702b9580..ed9c768a 100644 --- a/radicle-cli/src/commands/sync.rs +++ b/radicle-cli/src/commands/sync.rs @@ -435,14 +435,13 @@ pub fn fetch( let replicas = settings .replicas .min(seeds.iter().filter(|s| s.nid != local).count()); - let sessions = node.sessions()?; let mut results = FetchResults::default(); let (connected, mut disconnected) = seeds.partition(); - // Fetch from specified seeds, plus our preferred seeds. + // Fetch from specified seeds. for nid in &settings.seeds { - if !sessions.iter().any(|s| &s.nid == nid) { - term::warning(format!("node {nid} is not connected.. skipping")); + if !seeds.is_connected(nid) && !settings.force { + term::warning(format!("node {nid} is not connected or seeding.. skipping")); continue; } let result = fetch_from(rid, nid, settings.timeout, node)?; @@ -463,6 +462,9 @@ pub fn fetch( let result = fetch_from(rid, &nid, settings.timeout, node)?; results.push(nid, result); } + if results.success().count() >= replicas { + return Ok(results); + } // Try to connect to disconnected seeds and fetch from them. while results.success().count() < replicas { diff --git a/radicle-cli/src/node.rs b/radicle-cli/src/node.rs index b6f37ac9..c4c9ece2 100644 --- a/radicle-cli/src/node.rs +++ b/radicle-cli/src/node.rs @@ -22,6 +22,9 @@ 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, } @@ -33,6 +36,7 @@ impl SyncSettings { Self { replicas: seeds.len(), seeds, + force: false, timeout: DEFAULT_SYNC_TIMEOUT, } } @@ -51,17 +55,22 @@ impl SyncSettings { self } + /// Set the 'force' option. + pub fn force(mut self, force: bool) -> Self { + self.force = force; + self + } + /// Use profile to populate sync settings, by adding preferred seeds if no seeds are specified, /// and removing the local node from the set. pub fn with_profile(mut self, profile: &Profile) -> Self { - // If no seeds were specified, add up to `replica` seeds from the preferred seeds. + // If no seeds were specified, add the preferred seeds. if self.seeds.is_empty() { self.seeds = profile .config .preferred_seeds .iter() .map(|p| p.id) - .take(self.replicas) .collect(); } // Remove our local node from the seed set just in case it was added by mistake. @@ -75,6 +84,7 @@ impl Default for SyncSettings { Self { replicas: 3, seeds: BTreeSet::new(), + force: false, timeout: DEFAULT_SYNC_TIMEOUT, } }