diff --git a/README.md b/README.md --- a/README.md +++ b/README.md @@ -145,13 +145,29 @@ To create and edit your own PRs: ``` -atgc pr create # open a PR from the current branch +atgc pr create # push the branch and open a PR from it + # (--patch-only: the patch alone, no branch) atgc pr resubmit # append a round to one of your PRs atgc pr edit # change a PR's title or body (--title/--body) atgc pr close # close a PR (yours, or one against a repo you own) atgc pr reopen # reopen a closed PR atgc pr merge # land a PR on its target branch (needs push access) ``` + +`pr create` pushes the current branch to the target's knot and then asks the +knot for the patch, which is what the `source: {branch}` it records claims: +Tangled tells a branch-based pull from a patch-based one by that field alone +and never checks it, so a pull naming a branch nobody published gets a repo +link that 404s and a resubmit button that cannot work. Taking the patch from +the knot rather than formatting it here is the other half — it is the same +text the website would have produced, so a pull resubmitted from a browser +and one resubmitted from atgc do not differ by invisible whitespace. + +`--patch-only` is the shape for everything else: no push access on the target, +a branch not worth publishing, a patch assembled from something that is not a +branch. It records **no** source, because a source is exactly the claim that +the branch is there — which also means `pr view` on that branch will not find +the pull, and the command says so with the pull's `at://` URI. To interact with PRs more generally: diff --git a/TODO.md b/TODO.md --- a/TODO.md +++ b/TODO.md @@ -628,71 +628,74 @@ real — it is the only thing matching a pull back to the branch it came off, which `pr view` needs — but the appview reads it as a claim and not as a hint. See the entry below -- [ ] `pr create` pushes the branch by default, with `--patch-only` for the +- [x] `pr create` pushes the branch by default, with `--patch-only` for the shape it has today. Read off Tangled core at master `1adde466` rather - than guessed, so the ground is settled before the work starts. + than guessed, so the ground was settled before the work started. - **There is no patchless pull.** `lexicons/pulls/pull.json` requires - `rounds`, and every round requires `patchBlob`; the ingester refuses a - round without one (`appview/ingester.go`, "missing patchBlob in round - %d") and `Pull.Validate` refuses a pull with no submissions. "Let the - knot diff it" is real, but it names who *computes* the patch, not a - record that lacks one. The patch is still gzipped, uploaded as a blob - and put in the round, exactly as today. + **There is no patchless pull**, and there still isn't: + `lexicons/pulls/pull.json` requires `rounds` and every round requires + `patchBlob`, so the patch is gzipped, uploaded and put in the round + exactly as before. What changed is who computes it and what the record + is then entitled to claim. - **Three shapes, told apart by `source` alone** - (`appview/models/pull.go`: `IsPatchBased`/`IsBranchBased`/ - `IsForkBased`). No `source` is patch-based; a `source` naming this repo - is branch-based; a `source.repo` naming another is fork-based. The - appview never checks a `source` against reality — ingest parses it and - believes it. + The default pushes the branch to the target's knot over SSH, `GET`s + `sh.tangled.repo.compare?repo=&rev1=&rev2=` — + a public query, no scope and no session — refuses an empty comparison + as the appview does, and writes `source: {branch}` over a branch that + is now really there. `--patch-only` keeps the old local + `git format-patch` and writes **no** `source`, because a `source` is + exactly the claim that a branch is on the knot and the appview never + checks it. - **Which is why today's default is a bug and not merely an omission.** - atgc writes `source: {branch}` on every pull, including ones whose - branch was never pushed anywhere, so tangled.org classifies every atgc - pull as branch-based and then acts on it: a `/{repo}/tree/{branch}` - link that 404s (`pages/.../pullHeader.html`), a `resubmitCheck` asking - a knot for the head of a branch it has not got (`pulls/single.go`), and - a web resubmit routed through `resubmitBranch` into a `repo.compare` - that cannot succeed. `pr checkout` already defends itself against - exactly this and says so in its comments; the web UI has no such - defence. atgc is claiming a shape it does not implement. - - **So implement the shape rather than retract the claim.** Default: - push the branch, then write `source`. That makes the claim true, turns - the dead tree link live, and lets a browser resubmit an atgc-opened - pull. `--patch-only` keeps today's behaviour for the cases that need - it — no push access on the target, a branch not worth publishing, a - patch assembled from something that is not a branch — and it must then - write **no `source` at all**, because a `source` is exactly the claim - that a branch is there. - - Steps for the default path, every one against an endpoint that already - exists: push the branch to the target repo's knot over SSH, which is - the access `atgc key add` already grants; `GET /xrpc/ - sh.tangled.repo.compare?repo=&rev1=&rev2=`, - which is a public query needing no scope and no session; refuse an - empty `formatPatch`, as the appview does; gzip `formatPatchRaw`, - upload it, `putRecord` with `source: {branch}`. Taking the patch from - the knot instead of computing it locally is the half worth insisting - on — it is the same text the web UI would have produced, so a pull - resubmitted from a browser and one resubmitted by atgc agree, rather - than differing by whitespace nobody can see until the diff is read. - - Two things deliberately out of scope. **Fork-based pulls** are the - harder shape and want their own entry: `sh.tangled.repo.hiddenRef` on - the *fork's* knot under service auth, push access on the fork, and the - compare run there rather than on the target — and the appview only - recognises a fork it already knows (`db.GetForkByRepoDid`). And - **merge is unaffected either way**: the knot never merges a ref, only - applies a patch (`pulls/merge.go` feeds `CombinedPatch()` to - `repo.merge`), so none of this changes what landing a pull does. - - What it costs: `pr view` matches a branch to a pull through - `source.branch` (`cmd/pr/read.rs`), so under `--patch-only` that match - is gone. Decide where a local branch name lives when it is not a claim - about a knot — a trailer in the patch, beside `Change-Id:`, is the - obvious candidate and costs nothing + Two things learned in the doing, both cheap to get wrong. The knot + serializes `types.RepoFormatPatchResponse` with Go's names, not the + lexicon's: the mailbox arrives as `patch` and the per-commit list as + `format_patch`, and every field is `omitempty`. And a scenario can take + a real push without a git transport on the mock knot — the checkout + carries `url..pushInsteadOf`, which rewrites the push target and + leaves `git remote get-url` alone, so the repo DID is still resolvable + out of the remote path. Plain `insteadOf` would not have worked; + `get-url` applies it +- [ ] `stack create` writes `source: {branch}` on every member, which is the + same untrue claim `pr create` just stopped making, and it cannot be + fixed the same way. A stack member's patch is one commit's + `format-patch` carrying the `Change-Id:` header the appview correlates + rewrites by, and `repo.compare` answers about a *range* — there is no + per-commit compare on a knot to take those bytes from. So the choice is + narrower than it was for `pr create`: push the branch and keep writing + `source` while still formatting each member's patch locally, or drop + `source` from stack members and lose the branch match. Neither is + obvious, and picking one wants the same read of `appview/pulls/` that + settled the entry above +- [ ] `pr resubmit` on a branch-based pull should re-push and re-compare, + and today does neither. Deliberately left out of the `pr create` change + so that one PR did one thing, and it is a real gap rather than a tidy + one: after a resubmit the record's newest round is the local patch + while the branch on the knot is whatever was last pushed, so the tree + link and the diff describe different code. The shape is already there — + `push_and_compare` in `cmd/pr/write.rs` is the whole of it — and the + question the entry has to answer first is how a resubmit knows which + shape it is appending to, since the pull record says (`source` present + and no `source.repo`) but a `--patch-only` pull must not suddenly grow + one +- [ ] the branch a `--patch-only` pull came off is recorded nowhere, so + `pr view`, `browse --pr`, `pr diff` and `pr checkout` cannot find it + from the checkout it was opened in. `pr create --patch-only` says so + and prints the at-uri, which is the honest stopgap and not an answer. + The obvious candidate is still a trailer in the patch beside + `Change-Id:`, and the reason it was not just done is that + `read::for_branch` matches over *listing items*, which hold a blob CID + and not the patch — so matching on a trailer means fetching a blob per + candidate. Worth measuring before building: the candidate set is one + account's pulls on one repo, which is usually small +- [ ] fork-based pulls, the third shape and the one `pr create` deliberately + does not implement. Harder than the branch shape in three separate + ways: `sh.tangled.repo.hiddenRef` on the *fork's* knot under service + auth, push access on the fork rather than on the target, and the + compare run there rather than here — and the appview only recognises a + fork it already knows (`db.GetForkByRepoDid`), so a fork atgc created + by other means does not qualify. `--patch-only` is the answer until + this exists - [ ] Attach commit bodies to the PR body, which used to ride on the end of the entry above and is unrelated to any of it - [x] `pr list` — your own pulls straight from your PDS merged with Bobbin's diff --git a/tests/pr_flows.rs b/tests/pr_flows.rs --- a/tests/pr_flows.rs +++ b/tests/pr_flows.rs @@ -16,6 +16,7 @@ mod support; +use support::world::KNOT_PATCH; use support::{ALICE, BOB, PULL_NSID, PULL_STATUS_NSID, REPO_DID, Scenario}; /// A branch with two commits on it, ready to open a pull from. @@ -83,12 +84,94 @@ assert_eq!(value["source"]["branch"].as_str(), Some("feature")); assert_eq!(world.rounds(ALICE, &rkey), 1); - // The blob the record points at is really there, and really holds this - // branch's patch. A record naming a CID nothing uploaded is a pull that - // renders as an empty diff. + // The branch is on the other side before the record claims it is. That + // ordering is the whole change: `source` is a claim the appview believes + // without checking, so a pull carrying one whose branch was never pushed + // gets a dead tree link and a resubmit button that cannot work. + assert_eq!( + world.checkout.pushed_head("feature"), + Some(world.checkout.head()), + "the branch was recorded as the source but never pushed" + ); + + // The blob the record points at is really there, and holds the *knot's* + // patch rather than a locally formatted one. Same change either way; the + // point is that one producer formats it, so a pull resubmitted from a + // browser and one resubmitted from here do not differ by whitespace + // nobody can see. + assert_eq!(world.round_patch(ALICE, &rkey, 0), KNOT_PATCH); +} + +/// `--patch-only` is the shape for a target you cannot push to: no push, no +/// compare, and — the half that matters — **no `source`**, because a source +/// is exactly the claim that the branch is on the knot. +#[test] +fn patch_only_pushes_nothing_and_records_no_source() { + let world = Scenario::new("pr-create-patch-only"); + feature_branch(&world); + + let created = world + .run(&[ + "pr", + "create", + "--patch-only", + "--title", + "a pull", + "--json", + ]) + .success() + .json(); + assert_eq!(created["source_recorded"], false); + assert_eq!(created["pushed"], false); + + let (rkey, value) = only_pull(&world); + assert!( + value.get("source").is_none_or(serde_json::Value::is_null), + "--patch-only wrote a source: {value:#}" + ); + assert_eq!(world.checkout.pushed_head("feature"), None); + assert!( + world.with(|w| w.calls_to("sh.tangled.repo.compare").is_empty()), + "--patch-only asked the knot to compare" + ); + + // And the patch is the local one, which is the only one there is when + // nothing was published for a knot to diff. let patch = world.round_patch(ALICE, &rkey, 0); assert!(patch.contains("feat: first"), "{patch}"); assert!(patch.contains("feat: second"), "{patch}"); +} + +/// A knot with nothing between the two revisions refuses, in its own words. +/// Reachable even though the local patch was not empty: the knot answers +/// about what it has, and the appview refuses the same case with "No commits +/// between target and source". +#[test] +fn a_compare_with_no_commits_in_it_stops_the_create() { + let world = Scenario::new("pr-create-empty-compare"); + feature_branch(&world); + world.with(|w| w.compare = Ok((0, String::new()))); + + world + .run(&["pr", "create", "--title", "a pull"]) + .refused("finds no commits"); + assert!(world.pulls(ALICE).is_empty(), "a record was written anyway"); +} + +/// A dry run publishes nothing. The push is the first thing that leaves the +/// machine, and `--dry-run` promising to send nothing has to cover it. +#[test] +fn a_dry_run_pushes_no_branch_and_asks_no_knot() { + let world = Scenario::new("pr-create-dry-run"); + feature_branch(&world); + + world + .run(&["pr", "create", "--title", "a pull", "--dry-run"]) + .success(); + + assert_eq!(world.checkout.pushed_head("feature"), None); + assert!(world.with(|w| w.calls_to("sh.tangled.repo.compare").is_empty())); + assert!(world.pulls(ALICE).is_empty()); } /// A pull is written into its author's PDS with its author's credentials, @@ -493,6 +576,9 @@ assert_eq!( knot, [ + // `pr create` opened the pull by pushing and comparing; the two + // merge calls are what `pr merge` itself sends. + "knot sh.tangled.repo.compare", "knot sh.tangled.repo.mergeCheck", "knot sh.tangled.repo.merge" ], diff --git a/src/cmd/agent_notes.txt b/src/cmd/agent_notes.txt --- a/src/cmd/agent_notes.txt +++ b/src/cmd/agent_notes.txt @@ -19,6 +19,11 @@ PRs are created with "rounds" based on submitted patches. Updating a remote branch does not automatically append a new round. You should push the branch, then create a new round, then edit body. + `pr create` pushes the branch for you and takes the patch from the + knot; `--patch-only` skips the push and records no source branch, + which is what to use when you have no push access on the target. + atgc pr create # push the branch, then open the PR + atgc pr create --patch-only # the patch alone, no branch pushed atgc pr resubmit 23 # append a round (a patch revision) atgc pr edit 23 # title and body only atgc pr diff 23 --interdiff # what changed between rounds diff --git a/tests/support/git.rs b/tests/support/git.rs --- a/tests/support/git.rs +++ b/tests/support/git.rs @@ -10,7 +10,8 @@ //! stands alone, and only falls back to following clone redirects when it //! does not. So `/did:plc:…` resolves with no request at all, which is //! both the shape a `repo clone` leaves behind and the reason a scenario -//! needs no git transport. +//! needs no git transport to *read* the remote. Pushing to it is a separate +//! arrangement, and [`Checkout::new`] describes it. //! //! It still points at the mock knot rather than at an invented hostname, //! because a few commands do reach for the remote itself: `pr resubmit` asks @@ -63,7 +64,7 @@ let bare = path.with_file_name("origin.git"); let _ = std::fs::remove_dir_all(&bare); run_git( - std::path::Path::new("."), + Path::new("."), &["init", "-q", "--bare", &bare.to_string_lossy()], ); let checkout = Checkout { path, bare }; diff --git a/src/clients/git/run.rs b/src/clients/git/run.rs --- a/src/clients/git/run.rs +++ b/src/clients/git/run.rs @@ -587,6 +587,40 @@ Ok(()) } +/// Push `branch` to `remote` under its own name, without touching the local +/// branch's upstream. +/// +/// Apart from [`push_upstream`] because the two have different jobs. That one +/// publishes a branch `repo create` has just made and wants tracking set up; +/// this one makes a claim true — a pull recording `source: {branch}` is +/// telling every reader that the branch is on the knot — and has no business +/// rewriting a tracking configuration the person set for themselves. +/// +/// The refspec is spelled in full rather than left to `push.default`, which +/// is a per-machine setting: `matching` would push every branch whose name +/// the remote already has, and `upstream` would push somewhere else entirely +/// on a branch tracking a second remote. `--end-of-options` for the reason +/// [`fetch`] gives — the branch name reaches here from a checkout, and a +/// branch really can be called `--upload-pack=...`. +/// +/// No `--force` and none available. A branch that has diverged from the +/// remote is a question for the person, not something a pull request should +/// settle by overwriting. +pub fn push(remote: &str, branch: &str) -> Result<()> { + // Inherited, like every other push here: progress, host-key questions + // and credential prompts all reach the terminal on their own descriptors. + let refspec = format!("{branch}:refs/heads/{branch}"); + let status = Run::new(&["push", "--end-of-options", remote, &refspec]) + .inherited() + .run() + .context("failed to run git push")? + .status; + if !status.success() { + bail!("git push {remote} {branch} failed"); + } + Ok(()) +} + pub fn push_upstream(remote: &str, branch: &str) -> Result<()> { // Inherit stdio so SSH prompts and progress are visible. let status = Run::new(&["push", "-u", remote, branch]) diff --git a/src/clients/tangled/compare.rs b/src/clients/tangled/compare.rs new file mode 100644 --- /dev/null +++ b/src/clients/tangled/compare.rs @@ -0,0 +1,157 @@ +//! `sh.tangled.repo.compare` — the diff between two revisions, computed by +//! the knot that holds the repo. +//! +//! The odd one out among the knot calls: [`mod@super::knot`] mints a +//! service-auth token for every method it sends, and this one needs none. +//! `sh.tangled.repo.compare` is a public query — the knot resolves both +//! revisions and formats the patch for whoever asks, exactly as it serves a +//! clone — so it costs no OAuth scope and works with no session at all. +//! +//! **Why atgc asks rather than running `git format-patch` itself.** The web +//! UI opens a branch-based pull by calling this and putting the answer in the +//! record (`appview/pulls/create.go`, `handleBranchBasedPull`). A patch atgc +//! computed locally would be the same change and not the same *bytes* — a +//! different `From ` line, a different index abbreviation, a different +//! rename detection — so a pull resubmitted from a browser and one +//! resubmitted from here would differ for no reason a reader could see. One +//! producer of the patch is worth a request. +//! +//! The field names below are the knot's, not the lexicon's: +//! `types.RepoFormatPatchResponse` serializes `FormatPatchRaw` as `patch` and +//! `FormatPatch` as `format_patch`, and every field is `omitempty`, so an +//! absent key is ordinary rather than a malformed answer. + +use anyhow::{Context, Result}; + +/// What a knot answered `sh.tangled.repo.compare` with. +/// +/// Only the parts atgc has a use for. `combined_patch`/`combined_patch_raw` +/// are the squashed form the web UI stores beside the mailbox and no atgc +/// command reads, so they are dropped here rather than carried unused. +#[derive(Debug, PartialEq)] +pub struct Comparison { + /// The commit `rev1` resolved to — the target branch's tip. + pub rev1: String, + /// The commit `rev2` resolved to. This is what the appview records as a + /// submission's `sourceRev`, and it is the knot's answer to "what did I + /// actually diff", which is not necessarily the sha that was pushed a + /// moment ago if somebody else pushed after. + pub rev2: String, + pub merge_base: String, + /// How many commits are in the mailbox. The appview refuses a pull whose + /// `format_patch` is empty ("No commits between target and source"), and + /// so does every caller here. + pub commits: usize, + /// The whole `git format-patch` mailbox, which is what a round's patch + /// blob holds. + pub patch: String, + /// The knot left binary payloads out, so the patch will not apply + /// cleanly. Reported, not refused: the appview logs the same warning and + /// creates the pull anyway. + pub binary_omitted: bool, +} + +/// Compare two revisions on `repo_did`'s knot. +/// +/// `rev1` is the target and `rev2` the source, the same order the web UI +/// passes them in, so the patch reads as "what `rev2` adds on top of `rev1`". +pub async fn compare(knot: &str, repo_did: &str, rev1: &str, rev2: &str) -> Result { + let url = crate::clients::xrpc::endpoint( + &crate::clients::endpoints::knot(knot), + "sh.tangled.repo.compare", + &[("repo", repo_did), ("rev1", rev1), ("rev2", rev2)], + ); + crate::logging::debug::log(format!(">> GET {url}")); + let resp = crate::clients::http::get(&url) + .await + .with_context(|| format!("could not reach knot {knot}"))?; + let status = resp.status(); + let text = crate::clients::http::text_bounded(resp, &url).await?; + crate::logging::debug::log(format!("<< {status} ({} bytes)", text.len())); + if !status.is_success() { + let body: serde_json::Value = serde_json::from_str(&text).unwrap_or_default(); + // Classified off the status, like every other knot refusal: this one + // answers `RevisionNotFound` for a branch the knot has not got, which + // is the failure a caller most needs told apart from "the knot is + // down". + return Err(crate::exit::fail( + crate::exit::from_status(status), + format!( + "knot {knot} refused to compare {rev1}..{rev2} ({status}): {}", + body["message"] + .as_str() + .or(body["error"].as_str()) + .unwrap_or("no message"), + ), + )); + } + let body: serde_json::Value = serde_json::from_str(&text) + .with_context(|| format!("bad sh.tangled.repo.compare response from {knot}"))?; + Ok(parse(&body)) +} + +/// The response shape, apart from the request, so it can be asserted without +/// a socket. Permissive on purpose — every field is `omitempty` on the knot's +/// side, and a missing one is an empty comparison rather than an error. +fn parse(body: &serde_json::Value) -> Comparison { + Comparison { + rev1: body["rev1"].as_str().unwrap_or_default().to_string(), + rev2: body["rev2"].as_str().unwrap_or_default().to_string(), + merge_base: body["merge_base"].as_str().unwrap_or_default().to_string(), + commits: body["format_patch"].as_array().map_or(0, Vec::len), + patch: body["patch"].as_str().unwrap_or_default().to_string(), + binary_omitted: body["binary_omitted"].as_bool().unwrap_or(false), + } +} + +#[cfg(test)] +mod tests { + use super::parse; + use serde_json::json; + + /// The knot's own spelling, which is not the lexicon's: the mailbox + /// arrives under `patch` and the per-commit list under `format_patch`. + /// Reading `formatPatchRaw` — the Go field name — off this would find + /// nothing and record an empty patch. + #[test] + fn the_mailbox_is_read_from_patch_and_the_count_from_format_patch() { + let answer = parse(&json!({ + "rev1": "1111111111111111111111111111111111111111", + "rev2": "2222222222222222222222222222222222222222", + "merge_base": "3333333333333333333333333333333333333333", + "format_patch": [{"commit": {}}, {"commit": {}}], + "patch": "From 2222 Mon Sep 17 00:00:00 2001\n", + "combined_patch_raw": "diff --git a/x b/x\n", + })); + assert_eq!(answer.commits, 2); + assert_eq!(answer.patch, "From 2222 Mon Sep 17 00:00:00 2001\n"); + assert_eq!(answer.rev2, "2222222222222222222222222222222222222222"); + assert!(!answer.binary_omitted); + } + + /// Every field is `omitempty`, so "nothing between these two revisions" + /// arrives as an object with almost nothing in it. That must read as + /// zero commits — the condition callers refuse on — and not as a parse + /// failure, which would be reported as a broken knot. + #[test] + fn an_empty_comparison_is_zero_commits_and_not_an_error() { + let answer = parse(&json!({ + "rev1": "1111111111111111111111111111111111111111", + "rev2": "1111111111111111111111111111111111111111", + })); + assert_eq!(answer.commits, 0); + assert!(answer.patch.is_empty()); + } + + /// The one flag worth carrying up: a patch with the binaries taken out + /// applies to nothing, and the appview warns rather than refusing. + #[test] + fn binary_omitted_is_carried_through() { + let answer = parse(&json!({ + "format_patch": [{}], + "patch": "From 1\n", + "binary_omitted": true, + })); + assert!(answer.binary_omitted); + } +} diff --git a/src/clients/tangled/mod.rs b/src/clients/tangled/mod.rs --- a/src/clients/tangled/mod.rs +++ b/src/clients/tangled/mod.rs @@ -1,5 +1,7 @@ //! Tangled's own services, and the rules that are Tangled's alone. //! +//! - [`mod@compare`] — `sh.tangled.repo.compare` on a knot, the public query +//! that turns two revisions into the patch a branch-based pull carries. //! - [`mod@bobbin`] — the read-only appview. The only thing that can answer a //! question spanning accounts, and the only one that can be hours behind. //! - [`mod@knot`] — authenticated XRPC against the git host, on a service-auth @@ -17,6 +19,7 @@ pub(crate) mod bobbin; pub(crate) mod comments; +pub(crate) mod compare; pub(crate) mod knot; pub(crate) mod ownership; pub(crate) mod resolve; diff --git a/src/cmd/pr/write.rs b/src/cmd/pr/write.rs --- a/src/cmd/pr/write.rs +++ b/src/cmd/pr/write.rs @@ -79,6 +79,10 @@ /// Git remote pointing at the target repo #[arg(long, default_value = "origin")] pub remote: String, + /// Open the pull from the patch alone: don't push the branch, and record + /// no source on the pull + #[arg(long)] + pub patch_only: bool, /// Build and describe the PR without sending anything #[arg(long)] pub dry_run: bool, @@ -121,9 +125,18 @@ pub title: String, pub repo_did: String, pub target_branch: String, - /// The branch the patch came off, which is what `pr view` matches a - /// branch back to a pull by. + /// The branch the patch came off. Local information either way — under + /// `--patch-only` it is *not* on the record, which is what + /// `source_recorded` says. pub source_branch: String, + /// Whether the pull carries `source: {branch}`. False under + /// `--patch-only`, and then `pr view` on this branch will not find this + /// pull, because there is nothing on the record tying the two together. + pub source_recorded: bool, + /// Whether the branch was pushed to `remote` before the record was + /// written. The claim `source_recorded` makes is only true because of + /// this, so the two move together. + pub pushed: bool, pub commits: usize, pub patch_bytes: usize, pub patch_gzip_bytes: usize, @@ -335,8 +348,6 @@ Err(e) => crate::logging::debug::log(format!("stack warning check skipped: {e}")), } - let gzipped = gzip(&patch)?; - // Everything the preview lines below say, as one value the JSON branch // finishes and prints — on a dry run with no identifiers, after the // write with them. @@ -348,36 +359,51 @@ repo_did: repo.did.clone(), target_branch: target_branch.clone(), source_branch: branch.clone(), + source_recorded: !args.patch_only, + pushed: false, commits, patch_bytes: patch.len(), - patch_gzip_bytes: gzipped.len(), + patch_gzip_bytes: 0, images: images.listed(), }; - if !args.json { - println!("target: {} branch {}", repo.did, target_branch); - // Named in the output because it is now recorded, and because it is - // what `pr view` will match on afterwards: if this is not the branch - // you think you are on, that is worth seeing before the record is - // written. - println!("source: branch {branch}"); - println!( - "patch: {} commit(s), {} bytes ({} gzipped)", - commits, - patch.len(), - gzipped.len() - ); - println!("title: {title}"); - for line in images.describe() { - println!("image: {line}"); - } - } if args.dry_run { + // The local patch, which is the only one there is without sending + // anything: on the default path the knot recomputes it after the + // push, and the byte counts can differ. Said so rather than passed + // off as the final numbers. + let gzipped = gzip(&patch)?; + report.patch_gzip_bytes = gzipped.len(); if args.json { return crate::term::jsonout::emit(&report); } + preview_target(&report, &args.remote); + preview_patch(&report, &title, &images); println!("dry run; nothing sent"); return Ok(()); + } + + if !args.json { + preview_target(&report, &args.remote); + } + // The default path makes the claim true before making it. Everything + // before this point was local; from here the branch is on the target's + // knot and the patch is the knot's own, which is the same text the web + // UI would have put in the record. + let patch = match args.patch_only { + true => patch, + false => { + let comparison = push_and_compare(&args.remote, &branch, &target_branch, &repo).await?; + report.pushed = true; + report.commits = comparison.commits; + report.patch_bytes = comparison.patch.len(); + comparison.patch + } + }; + let gzipped = gzip(&patch)?; + report.patch_gzip_bytes = gzipped.len(); + if !args.json { + preview_patch(&report, &title, &images); } let agent = auth::agent_for_did(&selection.did).await?; @@ -406,13 +432,20 @@ title: title.into(), body: body.map(Into::into), target: crate::lexicon::tangled::pull_target(&repo.did, target_branch)?, - // Every `sh.tangled.repo.pull` written by anything other than atgc - // carries `source: {branch}` and nothing else — no `repo`, even - // where the lexicon allows one. The field is not a promise that the - // branch was pushed anywhere; it is the name of the branch the patch - // came off, which is exactly what atgc knows. Writing it is what - // makes `pr view` able to find this pull from this branch again. - source: Some(Source { + // `source` is a claim, not a hint. `appview/models/pull.go` tells the + // three shapes of pull apart by this field alone — no `source` is + // patch-based, a `source` naming this repo is branch-based — and + // nothing ever checks it against reality: ingest parses it and + // believes it. A pull carrying `source: {branch}` therefore gets a + // `/{repo}/tree/{branch}` link, a `resubmitCheck` asking the knot for + // that branch's head, and a web resubmit routed through + // `repo.compare`, all of which need the branch to be on the knot. + // + // So it is written exactly when the push above made it true, and + // `--patch-only` writes no `source` at all rather than a softer + // version of the same claim. No `repo` either: that is the fork-based + // shape, which atgc does not implement — see TODO.md. + source: (!args.patch_only).then(|| Source { branch: branch.clone().into(), repo: None, extra_data: None, @@ -472,7 +505,139 @@ if let Some(note) = link.note() { crate::term::say::note!(Index, "{note}"); } + patch_only_note(args.patch_only, &branch, &uri); Ok(()) +} + +/// Where this is going and where it came from — the half of the summary that +/// is known before anything leaves the machine. +/// +/// Printed ahead of the push so that the first output a person sees explains +/// what the git progress underneath it is for, rather than arriving after it. +fn preview_target(report: &CreatedJson, remote: &str) { + println!( + "target: {} branch {}", + report.repo_did, report.target_branch + ); + // The branch is named either way, because it is the branch this command + // is standing on and "that is not the branch I meant" is the mistake + // worth catching before a record exists. What differs is whether it is + // going anywhere: a recorded source is a claim that the branch is on the + // knot, and `--patch-only` declines to make it. + match report.source_recorded { + true => println!("source: branch {} -> {remote}", report.source_branch), + false => println!( + "source: branch {} (local; --patch-only records no source)", + report.source_branch + ), + } +} + +/// What the record will hold. Printed after the compare, because on the +/// default path the knot computes the patch and the byte counts are not the +/// local ones. +fn preview_patch(report: &CreatedJson, title: &str, images: &crate::cmd::images::Images) { + println!( + "patch: {} commit(s), {} bytes ({} gzipped)", + report.commits, report.patch_bytes, report.patch_gzip_bytes + ); + println!("title: {title}"); + for line in images.describe() { + println!("image: {line}"); + } +} + +/// The one thing `--patch-only` costs, said once, where it is still cheap to +/// act on. +/// +/// `pr view` and `browse --pr` find the pull for the branch they are standing +/// on through `source.branch`, and a patch-based pull has no source to match. +/// That is not a bug to work around here — a source is a claim about a knot, +/// and this pull is deliberately not making it — but it does mean the at-uri +/// printed above is the only handle on this pull from this checkout, and it +/// is worth saying so while it is on the screen. +fn patch_only_note(patch_only: bool, branch: &str, uri: &str) { + if !patch_only { + return; + } + crate::term::say::note!( + Pds, + "no source recorded, so `atgc pr view` on {branch} will not find this pull.\n\ + Name it directly: `atgc pr view {uri}`" + ); +} + +/// Push the branch, then have the target's knot compute the patch. +/// +/// The two are one step because neither is any use alone: the compare asks a +/// knot to diff a branch it has to have, and pushing a branch nothing records +/// is not what was asked for. Doing them in this order is what makes +/// `source: {branch}` true at the moment it is written. +/// +/// The patch comes back from the knot rather than from `git format-patch` +/// here, and [`crate::clients::tangled::compare`] says why: it is the same +/// text the web UI would have produced, so a pull resubmitted from a browser +/// and one resubmitted from atgc differ by nothing invisible. +async fn push_and_compare( + remote: &str, + branch: &str, + target_branch: &str, + repo: &resolve::RepoRef, +) -> Result { + crate::term::say::step!(Git, "pushing {branch} to {remote}..."); + git::push(remote, branch).map_err(|e| { + // The expected failure, and the one the flag exists for: no push + // access on the target. Named here rather than left to git's own + // message, which explains a permission problem and knows nothing + // about there being another way to open the pull. + anyhow::anyhow!( + "{e}\n\ + `atgc pr create --patch-only` opens the pull from the patch alone, \ + with no branch on the knot.\n\ + If the push should have worked, `atgc key add` is what registers \ + an SSH key with a knot." + ) + })?; + + let knot = crate::clients::atproto::did::knot_from_did_doc(&repo.did) + .await + .ok_or_else(|| { + anyhow::anyhow!( + "no knot in {}'s DID document, so there is nothing to ask for the patch\n\ + `atgc pr create --patch-only` opens the pull from the local patch instead", + repo.did + ) + })?; + crate::term::say::step!(Knot, "comparing {target_branch}..{branch} on {knot}..."); + let comparison = + crate::clients::tangled::compare::compare(&knot, &repo.did, target_branch, branch).await?; + + // The appview's own refusal, in its own words: `handleBranchBasedPull` + // stops on an empty `format_patch` with "No commits between target and + // source". Reachable here even though the local patch was not empty — + // the knot is answering about what it has, and a push that raced with + // somebody else's is the ordinary way the two disagree. + if comparison.commits == 0 || comparison.patch.is_empty() { + bail!( + "{knot} finds no commits between {target_branch} and {branch}\n\ + The branch was pushed; the knot has nothing on it that {target_branch} does not." + ); + } + if comparison.binary_omitted { + crate::term::say::warning!( + Knot, + "{knot} left binary payloads out of the patch, so it will not apply cleanly" + ); + } + crate::logging::debug::log(format!( + "compare {target_branch}({})..{branch}({}) merge-base {}: {} commit(s), {} bytes", + comparison.rev1, + comparison.rev2, + comparison.merge_base, + comparison.commits, + comparison.patch.len() + )); + Ok(comparison) } #[derive(clap::Args, Debug)] @@ -3222,6 +3387,8 @@ repo_did: "did:plc:repo".to_string(), target_branch: "main".to_string(), source_branch: "claude/json-flags".to_string(), + source_recorded: true, + pushed: true, commits: 3, patch_bytes: 5145, patch_gzip_bytes: 1200, @@ -3249,8 +3416,10 @@ "images", "patch_bytes", "patch_gzip_bytes", + "pushed", "repo_did", "source_branch", + "source_recorded", "target_branch", "title", "uri",