diff --git a/docs/machine-readable-output.md b/docs/machine-readable-output.md index 40c13c3..f330a25 100644 --- a/docs/machine-readable-output.md +++ b/docs/machine-readable-output.md @@ -110,6 +110,12 @@ before and after, the fields an edit would change — are *not* notes under rule 2. They are the answer, so `--json` prints them once, as fields, rather than moving them to stderr. +`pr checkout --json` is the one shape in neither group: it writes no record, +so it takes no `--dry-run` and carries no `dry_run`. What it did is local git +— a branch, and under `--worktree` a directory — and those are what it +prints. Both would otherwise be readable only out of the prose it prints for +a person, which is the case for `--json` in the first place. + ## Examples ```bash @@ -174,6 +180,7 @@ serializes, which are in the same rustdoc build as this page: - [`crate::cmd::key::KeyJson`] — `key list` - [`crate::cmd::browse::BrowseJson`] — `browse` - [`crate::cmd::doctor::DoctorJson`] — `doctor` +- [`crate::cmd::pr::review::CheckoutJson`] — `pr checkout` and, for the writing commands, the `…Json` type beside each one in [`crate::cmd::pr::write`], [`crate::cmd::stack::write`], [`crate::cmd::repo`], diff --git a/docs/module-layout.md b/docs/module-layout.md index 5788da2..be66d2b 100644 --- a/docs/module-layout.md +++ b/docs/module-layout.md @@ -118,7 +118,7 @@ a network service or a subprocess. clients/ ├── http.rs the one bounded reqwest client; every client below uses it ├── endpoints.rs the five hostnames compiled in, and their overrides -├── git/ run.rs config.rs patch.rs ssh.rs +├── git/ run.rs config.rs patch.rs ssh.rs worktree.rs ├── atproto/ pds.rs record.rs did.rs oauth/ └── tangled/ bobbin.rs knot.rs resolve.rs scope.rs web/ ``` @@ -148,6 +148,12 @@ implemented here and called from `cmd/`. No exceptions outside `testutil.rs`. change-ids, commit ranges. - `ssh.rs` — key material on this machine and matching it against registered keys. +- `worktree.rs` — `git worktree add` and `remove`, plus the check that a path + is somewhere a worktree may go. Two commands want the same three things of + it and want them differently: `pr diff --interdiff` builds a detached + scratch tree and destroys it, `pr checkout --worktree` builds one on a + branch and leaves it. What they share is the git invocation, so that is + what is shared; the lifetime is each caller's own. `clients/atproto/` is the protocol with nothing Tangled in it. `clients/tangled/` is Tangled's services and the rules that are Tangled's diff --git a/docs/pull-requests.md b/docs/pull-requests.md index f6a2057..3e110ed 100644 --- a/docs/pull-requests.md +++ b/docs/pull-requests.md @@ -850,6 +850,69 @@ status` to see it, `git add` plus `git am --continue` to finish, `git am --skip` to drop a commit, or `git am --abort` followed by `git checkout -` and `git branch -D` to undo the whole thing. +### Somewhere else entirely: `--worktree` + +```console +atgc pr checkout 23 --worktree ../review-23 +``` + +Everything above happens in a `git worktree` created at that path instead of +in the checkout you ran it from — same branch naming, same fetch and +corroboration, same `git am --3way --empty=keep`, same refusal to overwrite a +branch, and the same stopped `git am` left stopped, with its four ways out +rewritten to name the tree they apply to. + +**The dirty-tree refusal does not apply**, and that is the reason the flag +exists rather than a relaxation of the rule it skips. The rule is about +`git am` writing into a working tree that has uncommitted work in it; here it +writes into a tree this command has just made. Nothing on this path reads, +moves or writes the caller's working tree, index or `HEAD` — `git worktree +add` does not, and the fetch and the corroborating `git diff base...FETCH_HEAD` +that run in the original checkout are reads of refs and objects, not of the +tree. So a reviewer with half a feature in progress can look at somebody's +pull request without stashing it, which is the normal state of a checkout in +a project whose contributors work out of `.claude/worktrees/`. + +`--branch` composes with it: one names the branch, the other the directory +it is checked out in. Neither takes precedence over the other because they +do not overlap. + +Three things are refused before anything is created — a path that already +holds something (an existing *empty* directory is fine, as it is to git), a +path inside a **different** repository, and an empty path. The middle one is +a trap rather than a preference: a worktree under another repo is untracked +content over there, where a commit can swallow it and `git clean -fdx` can +delete it while the repo that registered it still lists it. Inside *this* +repo is allowed, since it is how this project itself works, but a path that +`.gitignore` does not cover gets a note saying so. All three are checked +first because `git worktree add` validates the path only after creating the +branch, so a refusal from git leaves a branch behind that nothing is using. + +The worktree shares the repo's object database — no clone, no second fetch — +and, unless something has set `extensions.worktreeConfig`, its `.git/config`. +That second one is what makes the identity work: the `[user]` handle and DID +and the `core.sshCommand` that `repo clone` and `repo configure` wrote are +one file, shared, so commits made in the new worktree are attributed to the +same account with nothing written twice. + +On success the path is printed with the way in and the way out: + +```console +$ atgc pr checkout 23 --worktree ../review-23 +… +checked out pr/permadeath.com/3msg7wllcqc2b-r1 +built on origin/main; `git log origin/main..` shows what the pull request adds + +in a worktree at /home/you/review-23 — this checkout was not touched +enter it: cd /home/you/review-23 +remove it: git worktree remove --force /home/you/review-23 && git branch -D pr/… +``` + +`--json` prints [`crate::cmd::pr::review::CheckoutJson`] instead: the branch, +the absolute `worktree` path (`null` when the pull was checked out here), the +base, and `mode`, which is `branch` or `patch` according to which of the two +paths above ran. + ## Commenting: `pr comment` ```console diff --git a/src/clients/git/mod.rs b/src/clients/git/mod.rs index 5aea7f4..ee74eb9 100644 --- a/src/clients/git/mod.rs +++ b/src/clients/git/mod.rs @@ -4,6 +4,9 @@ //! subprocess from stopping on a question nobody will answer. //! - [`mod@ssh`] — the keys this machine can offer, matched against the ones //! the account has registered. +//! - [`mod@worktree`] — making and removing a second working tree on the same +//! object database, which `pr diff --interdiff` and `pr checkout +//! --worktree` both need. //! //! Every interaction with git belongs in this folder: running a command, //! parsing its output, reading or writing git config. A command calls in; it @@ -14,3 +17,4 @@ pub(crate) mod config; pub(crate) mod patch; pub(crate) mod run; pub(crate) mod ssh; +pub(crate) mod worktree; diff --git a/src/clients/git/worktree.rs b/src/clients/git/worktree.rs new file mode 100644 index 0000000..1b36b65 --- /dev/null +++ b/src/clients/git/worktree.rs @@ -0,0 +1,371 @@ +//! `git worktree`: a second working tree on one object database. +//! +//! Two callers, and they want the same three things — make one, put it +//! somewhere sensible, take it away again: +//! +//! - `pr diff --interdiff` builds a detached scratch worktree, applies two +//! rounds in it and removes it on every path including the failures. +//! - `pr checkout --worktree ` builds one on a new branch and *keeps* +//! it, so a pull can be reviewed without the caller's own checkout moving. +//! +//! The reason a worktree is the right tool for both is the same: it shares +//! the repo's object database, so it costs no clone and no fetch, and the +//! commits written in it are visible from the original checkout the moment +//! they exist. It also shares `.git/config`, which is why a checkout +//! `repo clone` configured — the `[user]` identity that attributes commits to +//! an ATProto account, and the `core.sshCommand` that picks the key — carries +//! into a worktree with nothing written twice. (Only until somebody sets +//! `extensions.worktreeConfig`; nothing in atgc does.) +//! +//! What is *not* shared is the working tree, the index and `HEAD`. That is +//! the whole point of [`add`] for `pr checkout`: the caller's checkout is not +//! read, not moved and not required to be clean. + +use anyhow::{Result, bail}; +use std::path::{Path, PathBuf}; + +/// Create a worktree at `dir`, on a new `branch` or detached, starting at +/// `start`. +/// +/// `root` is the checkout to run this from — any working tree of the repo +/// will do, since worktrees are registered against the common git directory +/// rather than against whichever tree asked for one. +/// +/// `--` before the path because both remaining arguments are strings this +/// process did not choose: the path is the caller's word, and `start` is +/// built from a remote name and a branch out of somebody else's pull request +/// record. Past the marker git reads them as a path and a commit-ish or it +/// errors, rather than as options. +/// +/// Callers are expected to have validated the path with [`plan`] first. Git +/// checks it too, but only *after* creating the branch, so a refusal from git +/// leaves a branch behind that nothing is using. +pub(crate) fn add( + root: &Path, + dir: &Path, + branch: Option<&str>, + start: &str, + quiet: bool, +) -> Result<()> { + let path = dir.to_string_lossy(); + let mut args: Vec<&str> = vec!["worktree", "add"]; + match branch { + Some(name) => args.extend_from_slice(&["-b", name]), + None => args.push("--detach"), + } + if quiet { + args.push("--quiet"); + } + args.extend_from_slice(&["--", &path, start]); + if !super::run::status_in(root, &args)? { + bail!("could not create a worktree at {}", dir.display()); + } + Ok(()) +} + +/// Remove a worktree and its directory, reporting whether git managed it. +/// +/// `--force` because the caller is throwing the tree away deliberately and a +/// scratch worktree with a stopped `git am` in it is exactly the state git +/// would otherwise decline to remove. Failure is returned rather than raised: +/// this runs on drop paths where there is nothing useful to do about it. +pub(crate) fn remove(root: &Path, dir: &Path) -> bool { + super::run::status_in( + root, + &[ + "worktree", + "remove", + "--force", + "--", + &dir.to_string_lossy(), + ], + ) + .unwrap_or(false) +} + +/// Where a requested worktree path really is, once it is checked. +/// +/// Answers with an absolute path, which is what should then be handed to +/// [`add`] and printed: a relative path in the output of a command whose +/// whole purpose is to send you somewhere else is a path relative to a +/// directory the reader is about to leave. Making it absolute also removes +/// the last way the path could reach git looking like an option. +/// +/// Three refusals, in the order they are cheapest to check: +/// +/// 1. **An empty path.** Git would take it as the current directory. +/// 2. **A path that already holds something.** Git refuses this too, but +/// only after it has created the branch, so checking here is what keeps a +/// failed `pr checkout --worktree` from leaving a stray branch behind. An +/// existing *empty* directory is fine and is accepted, as git accepts it. +/// 3. **A path inside a different repository's working tree.** Git allows +/// this and it is a trap: the new worktree shows up as untracked content +/// in the other repo, where a commit can swallow it and `git clean -fdx` +/// can delete it out from under the repo that registered it — leaving a +/// worktree that git still lists and cannot find. Inside *this* repo is +/// allowed, because it is how this project itself works +/// (`.claude/worktrees/`, which `.gitignore` covers). +pub(crate) fn plan(root: &Path, requested: &str) -> Result { + let requested = requested.trim(); + if requested.is_empty() { + return Err(crate::exit::fail( + crate::exit::Exit::Usage, + "--worktree needs a path to create the worktree at", + )); + } + + let path = match Path::new(requested).is_absolute() { + true => PathBuf::from(requested), + false => std::env::current_dir() + .map(|cwd| cwd.join(requested)) + .unwrap_or_else(|_| PathBuf::from(requested)), + }; + let path = settle(&path); + + if let Ok(entries) = std::fs::read_dir(&path) { + if entries.count() > 0 { + bail!( + "{} already exists and is not empty\n\ + pick a path that does not exist yet; `pr checkout --worktree` creates it, \ + and will not write a pull request into a directory that holds something else", + path.display() + ); + } + } else if path.exists() { + bail!( + "{} already exists and is not a directory\n\ + `pr checkout --worktree` creates the directory it is given", + path.display() + ); + } + + // The nearest ancestor that exists is the one to ask about, since the + // path itself usually does not yet. `plan` is the answer to "where would + // this land", and it lands inside whatever that directory is inside. + if let Some(existing) = path.ancestors().skip(1).find(|p| p.is_dir()) + && let Some(other) = common_dir(existing) + { + if common_dir(root).is_none_or(|ours| ours != other) { + bail!( + "{} is inside another git repository ({})\n\ + a worktree there is untracked content in that repo: a commit can swallow it \ + and `git clean -fdx` can delete it while git still lists it here\n\ + pick a path outside it", + path.display(), + other.display() + ); + } + // Inside our own repo is allowed and is how this project works, but + // only because `.gitignore` covers `.claude/worktrees`. Somewhere + // that is not ignored, the same hazard applies to the caller's own + // repo — so it is said rather than refused, since it is their tree + // and one line in `.gitignore` fixes it. + if !super::run::quiet_in( + existing, + &["check-ignore", "-q", "--", &path.to_string_lossy()], + ) { + crate::term::say::note!( + Git, + "{} is inside this repo and not ignored, so the worktree will show as \ + untracked content; add it to .gitignore", + path.display() + ); + } + } + + Ok(path) +} + +/// One spelling of one place: `.` and `..` folded out, and the part of the +/// path that already exists resolved through any symlinks. +/// +/// Worth doing because this path is *printed*, twice, as somewhere to go — +/// and `--worktree ../review-23` joined onto the current directory produces +/// `/home/you/atgc/../review-23`, which is a correct path and an unreadable +/// instruction. Symlinks are resolved only as far as the directory that is +/// there, since the path itself is not yet: that is enough for the two things +/// resolution is for here, comparing this location against a repository's and +/// showing the reader the same string twice. +fn settle(path: &Path) -> PathBuf { + let mut lexical = PathBuf::new(); + for component in path.components() { + match component { + std::path::Component::CurDir => {} + // A leading `..` has nothing above it to cancel, so it is kept: + // dropping it would change which directory is named. + std::path::Component::ParentDir => { + if !lexical.pop() { + lexical.push(".."); + } + } + other => lexical.push(other), + } + } + let Some(existing) = lexical.ancestors().find(|p| p.exists()) else { + return lexical; + }; + match ( + std::fs::canonicalize(existing), + lexical.strip_prefix(existing), + ) { + // The whole path already exists — an empty directory, the one such + // path this accepts. Joining the empty remainder onto it would leave + // a trailing separator, and this string is printed. + (Ok(real), Ok(rest)) if rest.as_os_str().is_empty() => real, + (Ok(real), Ok(rest)) => real.join(rest), + _ => lexical, + } +} + +/// The repository `dir` belongs to, named by its common git directory — the +/// one thing every worktree of a repo agrees on, and so the only honest way +/// to ask whether two paths are in the *same* repo rather than merely each in +/// some repo. `None` when `dir` is in no repository at all, which is the +/// ordinary case and not a failure. +fn common_dir(dir: &Path) -> Option { + let found = super::run::git_in( + dir, + &["rev-parse", "--path-format=absolute", "--git-common-dir"], + ) + .ok()?; + // Symlinked temp directories (/tmp -> /private/tmp, and this project's + // own scratch paths) make two spellings of one directory, and a textual + // comparison of them would refuse a path that is in fact our own repo. + let path = PathBuf::from(found); + Some(std::fs::canonicalize(&path).unwrap_or(path)) +} + +#[cfg(test)] +mod tests { + use super::{add, plan, remove}; + use crate::testutil::TempRepo; + use std::path::Path; + + /// The refusals, and the one shape that is allowed through: a path that + /// does not exist yet, and an existing but empty directory — which is + /// what a shell's tab-completion or an `mkdir -p` leaves behind, and + /// which git itself accepts. + #[test] + fn plans_a_path_that_is_free_and_refuses_one_that_is_not() { + let repo = TempRepo::new("worktree-plan"); + repo.commit("a.txt", "one\n", "Add a"); + let here = std::env::current_dir().expect("inside the temp repo"); + let root = Path::new("."); + + // Relative in, absolute out: the path is printed to somebody who is + // about to `cd` somewhere else, so it cannot stay relative. + let planned = plan(root, "review/pr-1").expect("a path that does not exist"); + assert!(planned.is_absolute(), "got {}", planned.display()); + assert!(planned.ends_with("review/pr-1")); + + // And with no `..` left in the middle of it. `--worktree ../review` + // is the ordinary way to ask for one, and the answer is printed as + // somewhere to `cd` to. + let sibling = plan(root, "../review-23").expect("a sibling of the checkout"); + assert!( + !sibling.to_string_lossy().contains(".."), + "got {}", + sibling.display() + ); + assert_eq!(sibling.parent(), here.parent()); + assert!(sibling.ends_with("review-23")); + + std::fs::create_dir_all(here.join("empty")).expect("an empty directory"); + assert!(plan(root, "empty").is_ok(), "an empty directory is free"); + + std::fs::create_dir_all(here.join("full")).expect("a directory"); + std::fs::write(here.join("full/x"), "x").expect("something in it"); + let err = plan(root, "full").unwrap_err().to_string(); + assert!(err.contains("not empty"), "got: {err}"); + + std::fs::write(here.join("afile"), "x").expect("a file"); + let err = plan(root, "afile").unwrap_err().to_string(); + assert!(err.contains("not a directory"), "got: {err}"); + + assert!( + plan(root, " ").is_err(), + "an empty path is the cwd to git" + ); + } + + /// A worktree inside *another* repo is untracked content over there, so + /// a commit or a `git clean -fdx` in that repo can take it away while + /// this one still lists it. Inside our own repo is fine — it is how this + /// project works (`.claude/worktrees/`). + #[test] + fn plan_refuses_a_path_inside_a_different_repo() { + let repo = TempRepo::new("worktree-nested"); + repo.commit("a.txt", "one\n", "Add a"); + let here = std::env::current_dir().expect("inside the temp repo"); + let root = Path::new("."); + + assert!( + plan(root, "inside/our/own/repo").is_ok(), + "a path inside the repo asking for the worktree is allowed" + ); + + let other = here.join("other"); + std::fs::create_dir_all(&other).expect("a directory"); + crate::clients::git::run::git_in(&other, &["init", "-q"]).expect("a second repo"); + let err = plan(root, "other/wt").unwrap_err().to_string(); + assert!(err.contains("another git repository"), "got: {err}"); + } + + /// The claim `pr checkout --worktree` rests on: making one does not read, + /// move or dirty the checkout it was asked from — including when that + /// checkout has uncommitted work in it, which is the case the dirty-tree + /// refusal exists for and which this path is exempt from. + #[test] + fn adding_a_worktree_leaves_the_origin_checkout_alone() { + let repo = TempRepo::new("worktree-add"); + repo.commit("a.txt", "one\n", "Add a"); + let here = std::env::current_dir().expect("inside the temp repo"); + let root = here.as_path(); + + // Uncommitted work, of both kinds git reports. + std::fs::write(here.join("a.txt"), "one\ntwo\n").expect("modify a tracked file"); + std::fs::write(here.join("untracked.txt"), "x").expect("add an untracked file"); + let before_head = repo.git(&["rev-parse", "HEAD"]); + let before_branch = repo.git(&["rev-parse", "--abbrev-ref", "HEAD"]); + let before_status = repo.git(&["status", "--porcelain"]); + + // Beside the repo rather than inside it. A worktree *inside* the + // caller's own repo is untracked content there — which is exactly + // what the note in `plan` is about — and it would show in the + // porcelain compared below, hiding the thing this test is asking. + let outside = here.with_file_name(format!( + "{}-wt", + here.file_name() + .expect("a temp repo name") + .to_string_lossy() + )); + let dir = plan(root, &outside.to_string_lossy()).expect("a free path"); + add(root, &dir, Some("pr/somebody/3ms-r1"), "HEAD", true).expect("a worktree"); + + assert!(dir.join("a.txt").exists(), "the worktree was populated"); + assert_eq!( + std::fs::read_to_string(dir.join("a.txt")).unwrap(), + "one\n", + "the worktree holds the committed content, not the caller's edit" + ); + assert_eq!( + crate::clients::git::run::git_in(&dir, &["rev-parse", "--abbrev-ref", "HEAD"]).unwrap(), + "pr/somebody/3ms-r1" + ); + + assert_eq!(repo.git(&["rev-parse", "HEAD"]), before_head); + assert_eq!( + repo.git(&["rev-parse", "--abbrev-ref", "HEAD"]), + before_branch + ); + assert_eq!(repo.git(&["status", "--porcelain"]), before_status); + assert_eq!( + std::fs::read_to_string(here.join("a.txt")).unwrap(), + "one\ntwo\n", + "the caller's uncommitted edit survived" + ); + + assert!(remove(root, &dir), "a worktree is removable again"); + assert!(!dir.exists()); + } +} diff --git a/src/cmd/pr/mod.rs b/src/cmd/pr/mod.rs index fcef684..cfc9794 100644 --- a/src/cmd/pr/mod.rs +++ b/src/cmd/pr/mod.rs @@ -152,6 +152,18 @@ pub(crate) enum Command { /// target branch, or fetches the source branch when the pull request is /// branch-based. Refuses to run on a dirty working tree, and refuses to /// overwrite a branch that already exists. + /// + /// `--worktree ` creates a `git worktree` there and checks the + /// pull out in it instead, leaving this checkout on the branch it is on + /// and its uncommitted work where it is — so there is nothing to stash + /// and no dirty-tree refusal. The worktree shares this repo's objects + /// and its `.git/config`, so it costs no clone and commits in it are + /// attributed to the same account. + /// + /// Examples: + /// atgc pr checkout 23 + /// atgc pr checkout 23 --worktree ../review-23 + /// atgc pr checkout 23 --worktree ../review-23 --json | jq -r .worktree #[command(verbatim_doc_comment)] Checkout(review::CheckoutArgs), /// Merge a pull request into its target branch diff --git a/src/cmd/pr/review.rs b/src/cmd/pr/review.rs index e9c402a..edc543a 100644 --- a/src/cmd/pr/review.rs +++ b/src/cmd/pr/review.rs @@ -40,6 +40,7 @@ //! `atgc pr diff 23 > fix.patch` do what they look like they do. use crate::clients::git::run as git; +use crate::clients::git::worktree; use crate::clients::tangled::resolve; use crate::clients::tangled::web::pulls::{pull_number_from_url, pull_uri_from_appview}; use crate::config::account; @@ -842,6 +843,12 @@ async fn interdiff(pull: &Pull, rounds: &[RoundRef], args: &DiffArgs, author: &s /// runs `git worktree remove --force`, including on the error paths, so a /// failed interdiff does not leave a scratch directory or a registered /// worktree behind. +/// +/// The two git commands underneath are +/// [`crate::clients::git::worktree`]'s, shared with `pr checkout --worktree`. +/// What is *not* shared is anything above them: this is a throwaway that +/// exists for the length of one command, and that one is a place somebody is +/// about to work in. struct Scratch { root: PathBuf, dir: PathBuf, @@ -857,20 +864,7 @@ impl Scratch { .unwrap_or(0); let dir = std::env::temp_dir().join(format!("atgc-interdiff-{}-{stamp}", std::process::id())); - let ok = git::status_in( - root, - &[ - "worktree", - "add", - "--detach", - "--quiet", - &dir.to_string_lossy(), - base_commit, - ], - )?; - if !ok { - bail!("could not create a scratch worktree at {}", dir.display()); - } + worktree::add(root, &dir, None, base_commit, true)?; Ok(Self { root: root.to_path_buf(), dir, @@ -979,10 +973,7 @@ impl Scratch { impl Drop for Scratch { fn drop(&mut self) { - let _ = git::status_in( - &self.root, - &["worktree", "remove", "--force", &self.dir.to_string_lossy()], - ); + worktree::remove(&self.root, &self.dir); } } @@ -1010,6 +1001,13 @@ pub(crate) struct CheckoutArgs { /// Branch to create (defaults to `pr//-r`) #[arg(long, value_name = "NAME")] pub branch: Option, + /// Create a `git worktree` at this path and check the pull out there, + /// leaving this checkout on the branch it is on + /// + /// The path must not exist, or must be an empty directory. Composes with + /// `--branch`, which names the branch the worktree is put on. + #[arg(long, value_name = "PATH")] + pub worktree: Option, /// Whose pull request it is, when a bare record key is ambiguous #[arg(long, value_name = "HANDLE|DID")] pub author: Option, @@ -1028,6 +1026,109 @@ pub(crate) struct CheckoutArgs { /// Refuse a patch that decompresses past this many bytes #[arg(long, value_name = "BYTES", default_value_t = DEFAULT_MAX_BYTES)] pub max_bytes: u64, + /// Print one JSON object: the branch, the worktree path, the base and + /// whether a branch or a patch was used + #[arg(long)] + pub json: bool, +} + +/// What `pr checkout` put where. +/// +/// The command's answer used to be four lines of prose ending in a branch +/// name, which is the shape that makes a caller parse stdout: the two things +/// worth having afterwards — where to `cd` and what to `git log` — were +/// readable only by a person. Every field here is one of the lines that were +/// already printed. +#[derive(serde::Serialize, Debug, PartialEq)] +pub(crate) struct CheckoutJson { + /// The pull request's record, which is the only identifier it really has. + pub uri: String, + pub title: String, + /// The author's handle with no leading `@`, or `null` when the DID + /// document named none — the DID beside it always resolves. + pub author_handle: Option, + pub author_did: String, + /// 1-based, as `--round` counts and as `pr view --json` reports. + pub round: usize, + /// The branch created, whether in this checkout or in a worktree. + pub branch: String, + /// The absolute path of the worktree created, or `null` when the pull was + /// checked out into the caller's own checkout. + pub worktree: Option, + /// The commit the branch was started at, as it was named — `origin/main` + /// rather than a sha, since that is what `git log ..` takes. + pub base: String, + /// `branch` when the pull request's source branch was fetched and + /// corroborated against the round's patch, `patch` when the round's patch + /// was applied with `git am`. + pub mode: &'static str, + /// How many pulls beneath this one in its stack were applied first. `0` + /// for an unstacked pull and under `--only`. + pub stack: usize, +} + +/// Where a checked-out pull request is put. +/// +/// [`Destination::Here`] is what `pr checkout` has always done: `git checkout +/// -b` in the caller's own checkout, which moves them onto the pull and is +/// why that path refuses a dirty working tree. +/// +/// [`Destination::Worktree`] does the same work somewhere else entirely. It +/// is not a convenience over the first: it is the only shape that works when +/// the reviewer has their own work in progress, which — in a project whose +/// contributors and agents all live in `.claude/worktrees/` — is the normal +/// state of a checkout rather than an exception. Nothing on this path reads +/// or writes the caller's working tree, index or `HEAD`, so the dirty-tree +/// refusal does not apply to it and is not made. +enum Destination { + Here, + Worktree(PathBuf), +} + +impl Destination { + /// Create `branch` at `start`, with a working tree on it. + /// + /// One method rather than two call sites because this is the only step + /// the two destinations do differently: everything before it (resolving + /// the pull, naming the branch, fetching, corroborating) and everything + /// after it (`git am`) is the same work in a different directory. + fn start(&self, root: &Path, branch: &str, start: &str, quiet: bool) -> Result<()> { + let ok = match self { + Destination::Here => { + let mut args = vec!["checkout"]; + if quiet { + args.push("-q"); + } + args.extend_from_slice(&["-b", branch, start]); + git::status_in(root, &args)? + } + // Always quiet: `git worktree add` narrates in two lines that + // say less than the path this command prints itself. + Destination::Worktree(dir) => { + worktree::add(root, dir, Some(branch), start, true)?; + true + } + }; + if !ok { + bail!("could not create branch {branch} at {start}"); + } + Ok(()) + } + + /// The working tree the patch is applied in. + fn dir<'a>(&'a self, root: &'a Path) -> &'a Path { + match self { + Destination::Here => root, + Destination::Worktree(dir) => dir, + } + } + + fn path(&self) -> Option<&Path> { + match self { + Destination::Here => None, + Destination::Worktree(dir) => Some(dir), + } + } } impl CheckoutArgs { @@ -1213,33 +1314,65 @@ fn corroborates(patch: &str, branch_diff: &str) -> bool { } pub(crate) async fn checkout(args: CheckoutArgs) -> Result<()> { + crate::term::jsonout::init(args.json); let root = git::repo_root()?; + // Checked before anything is resolved or fetched, so a path that was + // never going to work costs no request — and, more importantly, so that + // no branch is created before it is known there is somewhere to put one. + // `git worktree add` validates the path itself, but only after minting + // the branch, and a refusal that leaves a branch behind is a refusal that + // did something. + let destination = match &args.worktree { + Some(path) => Destination::Worktree(worktree::plan(&root, path)?), + None => Destination::Here, + }; + // The same check git makes before it will move you between branches, and // for the same reason: `git am` writes to the working tree and the index, // and doing that on top of uncommitted work makes the two indivisible. - let dirty = git::git_in(&root, &["status", "--porcelain"])?; - if !dirty.is_empty() { - bail!( - "the working tree has uncommitted changes\n\ - commit or stash them first; `pr checkout` applies a patch onto a new branch" - ); + // + // Not asked under `--worktree`, and that is the point of the flag rather + // than a relaxation of a rule: the patch is applied in a tree this + // command just created, so there is no uncommitted work anywhere near it + // and the caller's tree is never written to. + if destination.path().is_none() { + let dirty = git::git_in(&root, &["status", "--porcelain"])?; + if !dirty.is_empty() { + bail!( + "the working tree has uncommitted changes\n\ + commit or stash them first; `pr checkout` applies a patch onto a new branch\n\ + or leave this checkout alone: `pr checkout --worktree ` puts the pull \ + request in a worktree of its own" + ); + } } let pull = resolve_pull(args.pull(), args.author.as_deref(), &args.remote).await?; let patches = read_patches(&pull.value)?; - let author = author_label(&pull.did).await; + // One DID document read for both spellings: `@handle` for a person, the + // bare handle for a branch name and for `--json`. + let handle = crate::clients::atproto::did::handle_from_did_doc(&pull.did).await; + let author = match &handle { + Some(handle) => format!("@{handle}"), + None => pull.did.clone(), + }; let round_number = match &patches { Patches::Rounds(rounds) => pick_round(rounds, args.round)?.number, Patches::Inline(_) => 1, }; - let handle = crate::clients::atproto::did::handle_from_did_doc(&pull.did) - .await - .unwrap_or_else(|| pull.did.clone()); + // `--branch` and `--worktree` compose: one names the branch, the other + // names the directory it is checked out in, and wanting both is the + // ordinary case rather than a contradiction. There is nothing for clap to + // refuse and nothing here to give precedence to. let branch = match &args.branch { Some(b) => b.clone(), - None => branch_name(&handle, &pull.rkey, round_number), + None => branch_name( + handle.as_deref().unwrap_or(&pull.did), + &pull.rkey, + round_number, + ), }; // Let git rule on the name rather than trusting `sanitize` to have // thought of everything. @@ -1262,9 +1395,11 @@ pub(crate) async fn checkout(args: CheckoutArgs) -> Result<()> { ); } - println!("{}", pull.title()); - println!("author: {author}"); - println!("uri: {}", pull.uri); + if !args.json { + println!("{}", pull.title()); + println!("author: {author}"); + println!("uri: {}", pull.uri); + } // A branch-based pull request names a branch that really exists on a // knot, and fetching it gets the author's actual commits — hashes, @@ -1289,7 +1424,9 @@ pub(crate) async fn checkout(args: CheckoutArgs) -> Result<()> { (Some(_), None) => false, }; if same_repo { - println!("source: branch {source} on {}", args.remote); + if !args.json { + println!("source: branch {source} on {}", args.remote); + } // `--end-of-options` for the reason `git::fetch` gives: `source` // is a string out of somebody else's pull request record, and // git would otherwise read one beginning with a dash as an @@ -1331,11 +1468,27 @@ pub(crate) async fn checkout(args: CheckoutArgs) -> Result<()> { remote = args.remote, ); } - if !git::status_in(&root, &["checkout", "-b", &branch, "FETCH_HEAD"])? { - bail!("could not create branch {branch} at the fetched head"); + // The fetch and the diff above both ran in `root`, and + // deliberately: `FETCH_HEAD` is per-worktree, so the ref this + // resolves has to be read where it was written. + destination.start(&root, &branch, "FETCH_HEAD", args.json)?; + if args.json { + return crate::term::jsonout::emit(&CheckoutJson { + uri: pull.uri.clone(), + title: pull.title().to_string(), + author_handle: handle, + author_did: pull.did.clone(), + round: round_number, + branch, + worktree: destination.path().map(|p| p.display().to_string()), + base, + mode: "branch", + stack: 0, + }); } println!("\nchecked out {branch} at {}/{source}", args.remote); println!("corroborated: matches round {round_number}'s patch"); + report_destination(&destination, &branch); return Ok(()); } crate::term::say::note!( @@ -1358,7 +1511,9 @@ pub(crate) async fn checkout(args: CheckoutArgs) -> Result<()> { Patches::Inline(patch) => patch.clone(), Patches::Rounds(rounds) => { let round = pick_round(rounds, args.round)?; - println!("round: {} of {}", round.number, rounds.len()); + if !args.json { + println!("round: {} of {}", round.number, rounds.len()); + } fetch_patch(&pull.did, round, args.max_bytes).await? } }; @@ -1373,9 +1528,11 @@ pub(crate) async fn checkout(args: CheckoutArgs) -> Result<()> { let combined = match below.is_empty() { true => patch.clone(), false => { - println!("stack: applying {} pull(s) below first:", below.len()); - for (title, _) in &below { - println!(" {title}"); + if !args.json { + println!("stack: applying {} pull(s) below first:", below.len()); + for (title, _) in &below { + println!(" {title}"); + } } let mut all: Vec<&str> = below.iter().map(|(_, p)| p.as_str()).collect(); all.push(&patch); @@ -1384,11 +1541,13 @@ pub(crate) async fn checkout(args: CheckoutArgs) -> Result<()> { }; let base = base_ref(&pull, &args.remote, args.base.as_deref())?; - println!("base: {base}"); - if !git::status_in(&root, &["checkout", "-q", "-b", &branch, &base])? { - bail!("could not create branch {branch} at {base}"); + if !args.json { + println!("base: {base}"); + } + destination.start(&root, &branch, &base, true)?; + if !args.json { + println!(); } - println!(); // `--empty=keep`, everywhere. It was `--empty=drop` for stacks, and a // stack whose member was an empty commit checked out with that member @@ -1398,33 +1557,101 @@ pub(crate) async fn checkout(args: CheckoutArgs) -> Result<()> { // moved base that is not the problem), and a member already merged into // the target three-way-resolves to an empty commit that says on its // face it brought nothing new. - let am: &[&str] = &["am", "--3way", "--empty=keep"]; - if !git::stdin_in(&root, am, combined.as_bytes())? { - // Deliberately not cleaned up. A stopped `git am` is a working state - // with the conflict in the tree, which is the thing worth looking at, - // and throwing it away to be tidy would destroy the only artefact the - // user came for. What is owed is a way out, spelled out — the state - // is otherwise invisible and `git status` is the only hint git gives. + let mut am: Vec<&str> = vec!["am", "--3way", "--empty=keep"]; + if args.json { + // Rule 1 of `--json`: stdout carries one document. `git am` narrates + // an `Applying: ` line per commit on stdout, and it is the + // one thing here that would land in the middle of it. + am.push("--quiet"); + } + if !git::stdin_in(destination.dir(&root), &am, combined.as_bytes())? { + // Deliberately not cleaned up — the worktree included. A stopped + // `git am` is a working state with the conflict in the tree, which is + // the thing worth looking at, and throwing it away to be tidy would + // destroy the only artefact the user came for. What is owed is a way + // out, spelled out — the state is otherwise invisible and `git + // status` is the only hint git gives. + // + // Where that state *is* differs, so the four ways out do too: under + // `--worktree` the caller is not standing in it, so every command + // below has to say which tree it means, and undoing it is removing + // the worktree rather than walking back out of a branch. + let stopped = match destination.path() { + None => format!( + "you are on {branch}, mid-apply, with the conflict in the working tree\n\ + \n\ + to look at it: git status\n\ + to resolve it: fix the files, `git add` them, then `git am --continue`\n\ + to skip one commit: git am --skip\n\ + to undo all of it: git am --abort, then `git checkout -` and \ + `git branch -D {branch}`" + ), + Some(path) => { + let path = path.display(); + format!( + "the worktree at {path} is on {branch}, mid-apply, with the conflict in \ + it; this checkout was not touched\n\ + \n\ + to look at it: cd {path} && git status\n\ + to resolve it: fix the files there, `git add` them, then \ + `git am --continue`\n\ + to skip one commit: git -C {path} am --skip\n\ + to undo all of it: git -C {path} am --abort, then \ + `git worktree remove --force {path}` and `git branch -D {branch}`" + ) + } + }; bail!( "`git am` stopped partway through applying this patch onto {base}\n\ - you are on {branch}, mid-apply, with the conflict in the working tree\n\ - \n\ - to look at it: git status\n\ - to resolve it: fix the files, `git add` them, then `git am --continue`\n\ - to skip one commit: git am --skip\n\ - to undo all of it: git am --abort, then `git checkout -` and \ - `git branch -D {branch}`\n\ + {stopped}\n\ \n\ usually {base} has moved since the round was written; `--base ` \ applies it where it was meant to go" ); } + if args.json { + return crate::term::jsonout::emit(&CheckoutJson { + uri: pull.uri.clone(), + title: pull.title().to_string(), + author_handle: handle, + author_did: pull.did.clone(), + round: round_number, + branch, + worktree: destination.path().map(|p| p.display().to_string()), + base, + mode: "patch", + stack: below.len(), + }); + } println!("\nchecked out {branch}"); println!("built on {base}; `git log {base}..` shows what the pull request adds"); + report_destination(&destination, &branch); Ok(()) } +/// Where the pull request went, when it did not go here. +/// +/// Nothing is printed for [`Destination::Here`]: the caller is standing in +/// the answer. For a worktree there are three things they do not have — the +/// path, the way in, and the way to be rid of it again — and the last of +/// those matters most, because a worktree left registered and then deleted by +/// hand is a state git complains about and most people have never had to +/// clear. +/// +/// The path is printed twice, bare and inside the `cd`, because those are two +/// different things to want: one to read or capture, one to paste. +fn report_destination(destination: &Destination, branch: &str) { + let Some(path) = destination.path() else { + return; + }; + let path = path.display(); + println!(); + println!("in a worktree at {path} — this checkout was not touched"); + println!("enter it: cd {path}"); + println!("remove it: git worktree remove --force {path} && git branch -D {branch}"); +} + #[cfg(test)] mod tests { use super::{ diff --git a/src/main.rs b/src/main.rs index 007e843..9d8432d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -540,6 +540,48 @@ mod tests { ); } + /// `--branch` and `--worktree` both say something about the branch + /// `pr checkout` creates, and the codebase's habit with two flags that + /// overlap is to have clap refuse the pair outright (`browse --pr` + /// against its section, `pr view --pr` against its positional). These are + /// the other case and must not be declared that way: they name different + /// halves of one act — what the branch is called, and which directory it + /// is checked out in — so `--worktree ../review --branch theirs` is an + /// ordinary thing to type, not a contradiction. Nothing gives precedence + /// to either at runtime, and this is what stops someone adding a + /// `conflicts_with` for symmetry's sake. + #[test] + fn pr_checkout_takes_a_branch_name_and_a_worktree_together() { + let cli = Cli::try_parse_from([ + "atgc", + "pr", + "checkout", + "23", + "--worktree", + "../review-23", + "--branch", + "theirs", + ]) + .expect("--worktree and --branch name different things"); + match cli.command { + Command::Pr { + command: PrCommand::Checkout(args), + } => { + assert_eq!(args.pull(), Some("23")); + assert_eq!(args.worktree.as_deref(), Some("../review-23")); + assert_eq!(args.branch.as_deref(), Some("theirs")); + } + _ => panic!("parsed into something other than pr checkout"), + } + // The flag takes a path; it is not a bare switch onto some directory + // atgc would pick, because where a worktree lands is the caller's + // decision and a wrong guess is a directory in the wrong place. + assert!( + Cli::try_parse_from(["atgc", "pr", "checkout", "23", "--worktree"]).is_err(), + "--worktree needs the path to create" + ); + } + /// The kind rides in front like a subcommand (`report bug`), because /// that is what the board's tag *is*, but it must stay an optional /// positional — which kinds exist is the board's data, not this enum's,