Skip to content

Conversation

@schacon
Copy link
Member

@schacon schacon commented Feb 9, 2026

runs gix clone with some basic output and stats at the end.

@schacon schacon requested a review from krlvi as a code owner February 9, 2026 06:03
Copilot AI review requested due to automatic review settings February 9, 2026 06:03
@vercel
Copy link

vercel bot commented Feb 9, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
gitbutler-web Ignored Ignored Preview Feb 9, 2026 6:03am

Request Review

@github-actions github-actions bot added rust Pull requests that update Rust code CLI The command-line program `but` labels Feb 9, 2026
Copy link
Contributor

Copilot AI left a 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 added protocol and host subcommands. 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()
            )?;
        }

Comment on lines +77 to +181
/// 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();
})
Copy link

Copilot AI Feb 9, 2026

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.

Copilot uses AI. Check for mistakes.
Comment on lines +314 to +332
/// 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")
}
Copy link

Copilot AI Feb 9, 2026

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.

Copilot uses AI. Check for mistakes.
Comment on lines +554 to +568
/// 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(())
Copy link

Copilot AI Feb 9, 2026

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.

Copilot uses AI. Check for mistakes.
/// 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.
Copy link

Copilot AI Feb 9, 2026

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).

Suggested change
/// 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).

Copilot uses AI. Check for mistakes.
Comment on lines +239 to +296
/// 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)
}
Copy link

Copilot AI Feb 9, 2026

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.

Copilot generated this review using guidance from repository custom instructions.
Comment on lines +18 to +28
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())
Copy link

Copilot AI Feb 9, 2026

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.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLI The command-line program `but` rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant