-
Notifications
You must be signed in to change notification settings - Fork 825
but clone command
#12295
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
but clone command
#12295
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds a new legacy but clone CLI command that clones a repository via gix, shows basic progress/stats, and then performs GitButler project setup. It also introduces configurable defaults for shorthand clone URL expansion (protocol + host) stored in forge settings.
Changes:
- Introduce
but clone(legacy) implementation with progress rendering, stats summary, and post-clone setup. - Add forge settings + CLI config subcommands to view/set clone shorthand defaults (
protocol,host). - Refactor legacy setup flow to support a “quiet” setup path used by
but clone.
Reviewed changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/but/src/command/legacy/clone.rs | New legacy clone command implementation (clone + progress/stats + setup) |
| crates/but/src/args/mod.rs | Adds Clone subcommand to CLI (legacy-gated) |
| crates/but/src/lib.rs | Wires Subcommands::Clone into command dispatch |
| crates/but/src/command/legacy/setup.rs | Adds repo_quiet() and refactors setup output gating |
| crates/but/src/command/config.rs | Adds but config forge protocol/host + shows clone defaults |
| crates/but/src/args/config.rs | Adds clap subcommands for forge clone defaults |
| crates/but-forge-storage/src/settings.rs | Persists clone_protocol / clone_host in forge settings |
| crates/but-forge-storage/src/controller.rs | Adds controller getters/setters for clone defaults |
| crates/but/src/utils/metrics.rs | Includes Clone in legacy “Unknown” metrics bucket |
| crates/but/src/command/legacy/mod.rs | Exposes new clone module |
| crates/but/Cargo.toml | Adds but-forge-storage, enables additional gix features, adds prodash |
| Cargo.toml / Cargo.lock | Adds workspace dependency on prodash |
| crates/but/tests/but/snapshots/...status01.stdout.term.svg | Updates snapshot output |
Comments suppressed due to low confidence (1)
crates/but/src/command/config.rs:449
- The
forge_show_overview()human output prints an “Available commands” list, but it doesn’t include the newly addedprotocolandhostsubcommands. This makes the new functionality hard to discover from the overview output; add entries for these commands (and keep the list in sync when new subcommands are added).
writeln!(out, "{}:", "Available commands".dimmed())?;
writeln!(
out,
" {} - Authenticate with a forge",
"but config forge auth".blue().dimmed()
)?;
writeln!(
out,
" {} - Forget an authenticated account",
"but config forge forget [username]".blue().dimmed()
)?;
}
| /// Spawn a thread that renders a single updating progress bar to stderr. | ||
| /// The bar is cleared when the thread exits. | ||
| fn spawn_progress_renderer(progress: &Arc<prodash::tree::Root>, stop: Arc<AtomicBool>) -> std::thread::JoinHandle<()> { | ||
| let progress = Arc::downgrade(progress); | ||
| std::thread::spawn(move || { | ||
| let mut snapshot = Vec::new(); | ||
|
|
||
| while !stop.load(Ordering::Relaxed) { | ||
| std::thread::sleep(Duration::from_millis(150)); | ||
| let Some(progress) = progress.upgrade() else { | ||
| break; | ||
| }; | ||
| progress.sorted_snapshot(&mut snapshot); | ||
|
|
||
| // Find the best task to display and any byte counter | ||
| let mut best_name = String::new(); | ||
| let mut best_current: usize = 0; | ||
| let mut best_total: Option<usize> = None; | ||
| let mut bytes_received: Option<usize> = None; | ||
|
|
||
| for (_key, task) in &snapshot { | ||
| if let Some(ref prog) = task.progress { | ||
| let current = prog.step.load(Ordering::Relaxed); | ||
| if current == 0 && prog.done_at.is_none() { | ||
| continue; | ||
| } | ||
| let is_bytes = prog | ||
| .unit | ||
| .as_ref() | ||
| .map(|u: &prodash::unit::Unit| format!("{}", u.display(1, None, None)).contains('B')) | ||
| .unwrap_or(false); | ||
|
|
||
| if is_bytes { | ||
| bytes_received = Some(current); | ||
| } else if prog.done_at.is_some() { | ||
| best_name = task.name.clone(); | ||
| best_current = current; | ||
| best_total = prog.done_at; | ||
| } else if best_total.is_none() { | ||
| best_name = task.name.clone(); | ||
| best_current = current; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if best_name.is_empty() && bytes_received.is_none() { | ||
| continue; | ||
| } | ||
|
|
||
| let width = terminal_size::terminal_size().map(|(w, _)| w.0 as usize).unwrap_or(80); | ||
|
|
||
| // Build suffix: " 45678/232966, 65.20 MiB" | ||
| let mut suffix = String::new(); | ||
| if let Some(total) = best_total { | ||
| suffix.push_str(&format!(" {best_current}/{total}")); | ||
| } else if best_current > 0 { | ||
| suffix.push_str(&format!(" {best_current}")); | ||
| } | ||
| if let Some(b) = bytes_received { | ||
| if !suffix.is_empty() { | ||
| suffix.push_str(", "); | ||
| } else { | ||
| suffix.push(' '); | ||
| } | ||
| suffix.push_str(&format_bytes(b as u64)); | ||
| } | ||
|
|
||
| let label = if best_name.is_empty() { | ||
| "Fetching".to_string() | ||
| } else { | ||
| capitalize(&best_name) | ||
| }; | ||
| let colored_label = format!("{}", label.bold().green()); | ||
| let label_width = label.len(); | ||
|
|
||
| let chrome = label_width + 2 + 1 + suffix.len(); | ||
| let bar_width = if width > chrome + 5 { width - chrome - 1 } else { 20 }; | ||
|
|
||
| let bar = if let Some(total) = best_total { | ||
| let fraction = if total > 0 { | ||
| (best_current as f64 / total as f64).min(1.0) | ||
| } else { | ||
| 0.0 | ||
| }; | ||
| let filled = (fraction * bar_width as f64) as usize; | ||
| let arrow = if filled < bar_width { ">" } else { "=" }; | ||
| let empty = bar_width.saturating_sub(filled).saturating_sub(1); | ||
| format!( | ||
| "{}{}{}", | ||
| "=".repeat(filled), | ||
| if filled < bar_width { arrow } else { "" }, | ||
| " ".repeat(empty) | ||
| ) | ||
| } else { | ||
| " ".repeat(bar_width) | ||
| }; | ||
|
|
||
| let _ = write!(std::io::stderr(), "\x1b[2K\r{colored_label} [{}]{suffix}", bar.cyan()); | ||
| let _ = std::io::stderr().flush(); | ||
| } | ||
|
|
||
| // Clear the progress line | ||
| let _ = write!(std::io::stderr(), "\x1b[2K\r"); | ||
| let _ = std::io::stderr().flush(); | ||
| }) |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The progress renderer writes ANSI control codes directly to stderr whenever out.for_human() is set, even if stderr is redirected/non-TTY. This can pollute logs/CI output; OutputChannel::ProgressChannel elsewhere gates progress output on stderr.is_terminal(). Consider using out.progress_channel() (or at least checking std::io::stderr().is_terminal()) before spawning the renderer.
| /// Run the GitButler setup on an already-cloned repository. | ||
| /// This mirrors the `but setup` flow: register the project first, | ||
| /// then open the repo from the registered project's git dir. | ||
| fn run_setup(repo_path: &Path, out: &mut OutputChannel) -> anyhow::Result<()> { | ||
| let repo = match but_api::legacy::projects::add_project_best_effort(repo_path.to_path_buf())? { | ||
| gitbutler_project::AddProjectOutcome::Added(project) | ||
| | gitbutler_project::AddProjectOutcome::AlreadyExists(project) => gix::open(project.git_dir())?, | ||
| _ => { | ||
| anyhow::bail!( | ||
| "Could not register '{}' as a GitButler project. Run 'but setup' manually.", | ||
| repo_path.display() | ||
| ); | ||
| } | ||
| }; | ||
| let mut ctx = but_ctx::Context::from_repo(repo)?; | ||
| let mut guard = ctx.exclusive_worktree_access(); | ||
| super::setup::repo_quiet(&mut ctx, repo_path, out, guard.write_permission()) | ||
| .context("Failed to set up GitButler project") | ||
| } |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
run_setup() registers the project via add_project_best_effort() and then calls setup::repo_quiet(), which registers the project again. This is redundant I/O and can lead to slightly different error behavior between but clone and but setup. Consider opening the freshly cloned repo directly (e.g. gix::open(repo_path)) and letting repo_quiet() perform the single registration step, or refactor repo_quiet() to accept an already-registered project.
| /// View or set the default clone host. | ||
| fn forge_clone_host(value: Option<String>, out: &mut OutputChannel) -> Result<()> { | ||
| let storage = forge_storage()?; | ||
| if let Some(value) = value { | ||
| storage.set_clone_host(Some(value.clone()))?; | ||
| if let Some(out) = out.for_human() { | ||
| writeln!(out, "Clone host set to: {}", value.green())?; | ||
| } | ||
| } else { | ||
| let current = storage.clone_host()?.unwrap_or_else(|| "github".to_string()); | ||
| if let Some(out) = out.for_human() { | ||
| writeln!(out, "Clone host: {}", current.green())?; | ||
| } | ||
| } | ||
| Ok(()) |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forge_clone_host persists whatever string the user provides, but but clone’s shorthand expansion treats this value as a domain (it string-interpolates into https://{host}/{owner}/{repo}). If a user sets host to a full URL like https://gitlab.com (which the settings comment currently implies is acceptable), shorthand expansion will generate malformed URLs. Add validation/normalization (strip scheme/path, or reject non-domain inputs) and align the help text/comments accordingly.
| /// Default protocol for clone shorthand expansion: "https" (default) or "ssh". | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub clone_protocol: Option<String>, | ||
| /// Default host for clone shorthand expansion: "github" (default), "gitlab", or a custom base URL. |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ForgeSettings.clone_host is documented as allowing a “custom base URL”, but the clone shorthand expansion code treats the stored value as a plain host/domain (no scheme/path). Update the doc comment to match the actual accepted format (or update the expansion logic to accept full base URLs).
| /// Default host for clone shorthand expansion: "github" (default), "gitlab", or a custom base URL. | |
| /// Default host for clone shorthand expansion: "github" (default), "gitlab", or a custom host/domain (no scheme or path). |
| /// Clone a repository from `url` into `path` and then run GitButler setup. | ||
| pub fn run(url: String, path: Option<PathBuf>, out: &mut OutputChannel) -> anyhow::Result<()> { | ||
| let (protocol, host) = load_clone_defaults(); | ||
| let url = expand_shorthand(&url, &protocol, &host); | ||
| let target_dir = match path { | ||
| Some(p) => p, | ||
| None => PathBuf::from(directory_from_url(&url)?), | ||
| }; | ||
|
|
||
| if target_dir.exists() { | ||
| anyhow::bail!("Destination path '{}' already exists.", target_dir.display()); | ||
| } | ||
|
|
||
| if let Some(out) = out.for_human() { | ||
| writeln!(out, "{}", format!("Cloning into '{}'...", target_dir.display()).cyan())?; | ||
| } | ||
|
|
||
| let start = std::time::Instant::now(); | ||
| let should_interrupt = AtomicBool::new(false); | ||
|
|
||
| let is_human = out.for_human().is_some(); | ||
| let progress = Arc::new(prodash::tree::Root::new()); | ||
| let stop_renderer = Arc::new(AtomicBool::new(false)); | ||
| let render_thread = if is_human { | ||
| Some(spawn_progress_renderer(&progress, Arc::clone(&stop_renderer))) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| let mut clone_progress = progress.add_child("clone"); | ||
|
|
||
| let (mut checkout, fetch_outcome) = gix::prepare_clone(url.as_str(), &target_dir)? | ||
| .fetch_then_checkout(&mut clone_progress, &should_interrupt) | ||
| .context("Failed to fetch repository")?; | ||
|
|
||
| let (_repo, _) = checkout | ||
| .main_worktree(&mut clone_progress, &should_interrupt) | ||
| .context("Failed to checkout worktree")?; | ||
|
|
||
| let elapsed = start.elapsed(); | ||
|
|
||
| // Stop the progress renderer (clear the line) | ||
| stop_renderer.store(true, Ordering::Relaxed); | ||
| if let Some(handle) = render_thread { | ||
| let _ = handle.join(); | ||
| } | ||
| drop(clone_progress); | ||
| drop(progress); | ||
|
|
||
| // Print stats summary | ||
| print_stats(&fetch_outcome, elapsed, out)?; | ||
|
|
||
| // Use the canonicalized target_dir for setup, matching how `but setup` uses | ||
| // args.current_dir. This ensures path consistency for project registration. | ||
| let repo_path = std::fs::canonicalize(&target_dir).unwrap_or_else(|_| target_dir.clone()); | ||
|
|
||
| run_setup(&repo_path, out) | ||
| } |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but clone introduces a new end-to-end flow (clone + setup + output/progress behavior), but there’s no CLI-level test covering the happy path. Given the existing crates/but/tests/but/command/* suite, add a minimal integration test that clones from a local fixture repository (no network) and asserts the expected setup side effects/output.
| fn directory_from_url(url: &str) -> anyhow::Result<String> { | ||
| let name = url | ||
| .rsplit('/') | ||
| .next() | ||
| .or_else(|| url.rsplit(':').next()) | ||
| .context("Could not derive directory name from URL")?; | ||
| let name = name.strip_suffix(".git").unwrap_or(name); | ||
| if name.is_empty() { | ||
| anyhow::bail!("Could not derive directory name from URL '{url}'"); | ||
| } | ||
| Ok(name.to_string()) |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
directory_from_url() fails for URLs with a trailing slash (e.g. https://github.com/user/repo/) because rsplit('/').next() returns an empty segment, causing an error even though the repo name is derivable. Consider trimming trailing slashes or skipping empty segments when extracting the last path component, and add a test case for the trailing-slash form.
runs gix clone with some basic output and stats at the end.