diff --git a/TODO.md b/TODO.md index 1e16946..6a24c71 100644 --- a/TODO.md +++ b/TODO.md @@ -577,13 +577,83 @@ out and `repo clone` won't parse one either, so this is a loud error where it used to be a silently wrong DID. Fixing it properly means asking the knot or the appview what repo that path names -- [x] `pr create` — records `source: {branch}`, the shape every pull request - written by anything other than atgc carries. It is not a claim the - branch was pushed; it is the only thing that can match a pull request - back to the branch it came off, which `pr view` needs -- [ ] `pr create` — branch-based PRs proper (push the branch, and let - `source` mean what a knot would mean by it); attach commit bodies to - the PR body +- [x] `pr create` — records `source: {branch}`. The reasoning that stood + here was wrong on a checkable fact, so it is corrected rather than + quietly dropped: this is **not** "the shape every pull request written + by anything other than atgc carries". Tangled's own web UI passes + `pullSource = nil` for a patch-based pull + (`appview/pulls/create.go`, `handlePatchBasedPull`), and only its two + branch shapes write a `source` at all. What the field buys atgc is + 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 + shape it has today. Read off Tangled core at master `1adde466` rather + than guessed, so the ground is settled before the work starts. + + **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. + + **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. + + **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 +- [ ] 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 sh.tangled.repo.listPulls (api.tangled.org, ATGC_BOBBIN to override); --state/--limit/--remote and --source auto|pds|bobbin; tolerates old