diff --git a/README.md b/README.md index c8497b0..6573708 100644 --- a/README.md +++ b/README.md @@ -186,6 +186,7 @@ To interact with PRs more generally: ``` atgc pr list # list PRs for this repo (--state open|closed|merged|all) +atgc pr list --author # this repo, narrowed to one account atgc pr list --all # your PRs across every repo (--author for someone else's) atgc pr view [pull] # view a PR (defaults to the current branch's) atgc pr diff # print a PR's patch (--round N, --interdiff between rounds) @@ -196,10 +197,14 @@ atgc pr comment # comment on a PR (--body/--body-file, --round N) Every verb here is scoped to the repo you are in, `pr list --all` excepted: that is your own pull requests wherever you filed them, however many repos -that spans, and it needs no checkout. `--author` asks the same of somebody -else, and needs `--all` to have anything to widen; widening is per account -rather than per repo, because a pull record lives in the PDS of whoever wrote -it and there is no listing of everyone's pulls everywhere to narrow. +that spans, and it needs no checkout. Widening is per account rather than per +repo, because a pull record lives in the PDS of whoever wrote it and there is +no listing of everyone's pulls everywhere to narrow. + +`--author` asks either scope about somebody else. Bare it narrows this repo to +one account; with `--all` it is their pulls wherever they filed them. Both read +that account's PDS rather than yours, which takes no token and no permission — +only the DID their handle resolves to. A pull request is a record in its **author's** PDS, so `pr list` and friends read yours straight from there: your own pulls are visible the instant the diff --git a/TODO.md b/TODO.md index be33039..0aadbe9 100644 --- a/TODO.md +++ b/TODO.md @@ -2121,11 +2121,17 @@ and borrows `pr`'s conventions rather than inventing new ones beside them. listing of everyone's pulls everywhere for an author to narrow. A flag that silently meant something else in the narrow case would be worse than a refusal that says so -- [ ] `pr list --author` without `--all`, meaning this repo filtered to one - account. Implementable on the existing gather — `pds_pulls` already - takes a repo filter and a DID, which is what `doctor local`'s index row - uses — and refused today only because it was not part of dissolving the - group +- [x] `pr list --author` without `--all`: this repo, narrowed to one + account. It needed no new gather — a repo listing's complete half was + always one account's PDS, so naming somebody else only changes which + DID is read — plus a post-filter for the index sources, which have + every author in them. The work that was actually in it was the prose: + four sentences about completeness said "your PDS" from a time when + there was only one answer, and a listing of somebody else's pulls that + says "your PDS" is the same class of wrong answer as a stale index. + `Whose` in `cmd/pr/read/sources.rs` is those four sentences' one + source of truth, and `--author` naming the selected account resolves + back to `Mine` so it reads like the bare listing ## report (feedback via userinput.app) - [x] `report` — an `app.userinput.discussion` in the reporter's own PDS, diff --git a/src/cmd/pr/mod.rs b/src/cmd/pr/mod.rs index 843f12e..19ed7bc 100644 --- a/src/cmd/pr/mod.rs +++ b/src/cmd/pr/mod.rs @@ -102,9 +102,11 @@ pub(crate) enum Command { /// records come from your PDS, so that listing is immediate and no index /// can be behind on it. It is complete as well whenever `--state` is /// filtering — that reads the whole collection, since a pull's state is - /// not in its pull record — while `--state all` stops at `--limit`. `--author` asks the same of somebody - /// else and needs `--all`, widening being per account rather than per - /// repo. + /// not in its pull record — while `--state all` stops at `--limit`. + /// `--author` asks either of those about somebody else: bare it narrows + /// this repo to that account, and with `--all` it is their pulls + /// everywhere. It reads their PDS rather than yours, which needs no + /// token and no permission on anything. /// /// `--json` prints an array of objects: the same state, number, round /// count and resolved author handle the table shows, not the raw diff --git a/src/cmd/pr/read/mod.rs b/src/cmd/pr/read/mod.rs index 2a389c2..90d8ee5 100644 --- a/src/cmd/pr/read/mod.rs +++ b/src/cmd/pr/read/mod.rs @@ -66,7 +66,7 @@ use crate::term::column::{day, ellipsize, pad_to}; use anyhow::Result; use labels::{RepoName, author_did, author_handles, numbers_across_repos, repo_names}; use sources::{ - Ask, Empty, Scope, apply_state_filter, classify_empty, gather, gather_branch_pulls, + Ask, Empty, Scope, Whose, apply_state_filter, classify_empty, gather, gather_branch_pulls, note_author_scoped, note_unknown_states, own_did, require_branch_pull, unknown_states, warn_stale, }; @@ -139,8 +139,13 @@ pub(crate) struct ListArgs { /// names somebody else, and it needs no checkout. #[arg(long)] pub all: bool, - /// Show another account's pull requests (handle or DID); needs --all - #[arg(long, requires = "all")] + /// Show another account's pull requests (handle or DID) + /// + /// Bare, this narrows *this repo* to that account. With `--all` it is + /// their pulls wherever they filed them. Either way it reads their PDS + /// rather than yours, which needs no token and no permission — only the + /// DID a handle resolves to. + #[arg(long)] pub author: Option, /// Filter by state #[arg(long, default_value = "open")] @@ -491,13 +496,56 @@ pub(super) async fn list(args: ListArgs) -> Result<()> { } } +/// The spelling `--author` was given, presentable: a handle wears its `@` +/// and a DID is left exactly as typed. +/// +/// Deliberately the *input* and not the DID it resolved to. Somebody who +/// asked about `@bob.example.com` is owed sentences about `@bob.example.com`; +/// answering them about `did:plc:…` is correct and unreadable. +fn author_label(input: &str) -> String { + match input.starts_with("did:") { + true => input.to_string(), + false => format!("@{}", input.trim_start_matches('@')), + } +} + +/// The author DID out of `at:///sh.tangled.repo.pull/`. +/// +/// A pull's authority *is* its author — the record lives in that account's +/// PDS — so the at-uri carries the filter's answer without touching the +/// record. `None` for anything that is not one, which then matches no +/// filter. +fn uri_author(uri: &str) -> Option<&str> { + uri.strip_prefix("at://")?.split('/').next() +} + async fn in_this_repo(args: ListArgs) -> Result<()> { let source = Source::parse_arg(args.source.as_deref())?; let remote_url = git::remote_url(&args.remote)?; let repo = resolve::repo_ref(&remote_url).await?; - let me = match source.uses_pds() { - true => own_did().await, - false => None, + // `--author` narrows this repo's listing to one account, which is the + // whole of what it takes: a repo listing's complete half is one PDS + // already, so naming somebody else only changes *which* PDS. Reading it + // needs no token, only a DID — the same reason `--all --author` works + // with a session that has lapsed. + let author = match &args.author { + Some(who) => Some(crate::config::account::actor_did(who).await?), + None => None, + }; + let me = match (source.uses_pds(), &author) { + (false, _) => None, + (true, Some(did)) => Some(did.clone()), + (true, None) => own_did().await, + }; + // Named by the spelling that was typed rather than by the DID it + // resolved to: somebody who asked for `@bob.example.com` should not be + // answered about `did:plc:…`. Falls back to `Mine` when the account + // named is the selected one, so `--author` on yourself reads like the + // bare listing rather than like a stranger's. + let whose = match (&author, &me) { + (Some(_), Some(did)) if own_did().await.as_deref() == Some(did) => Whose::Mine, + (Some(_), _) => Whose::Named(author_label(args.author.as_deref().unwrap_or_default())), + (None, _) => Whose::Mine, }; // Read before the listing is fetched rather than after, because it is @@ -505,7 +553,7 @@ async fn in_this_repo(args: ListArgs) -> Result<()> { // whose state the walk cannot see. See [`Reach::for_listing`]. let filter = args.state.label(); - let gathered = gather(Ask { + let mut gathered = gather(Ask { source, scope: Scope::Repo, did: me.as_deref(), @@ -525,12 +573,23 @@ async fn in_this_repo(args: ListArgs) -> Result<()> { gathered.pds_total, Scope::Repo, gathered.backfilled, + &whose, ); // Copy, and read before the rows are moved on: it describes the walk, // not the listing, and every sentence below that says what is missing // says it from here. let evidence = gathered.evidence; + + // The PDS half is already one account's; an index source is not, so + // `--author` has to narrow what Bobbin or the web scrape brought back + // too. Done here rather than in `gather` because it is this command's + // filter and not a property of the sources — and after the evidence is + // read, which is about the gather that really ran. + if author.is_some() { + let wanted = author.as_deref(); + gathered.items.retain(|i| uri_author(&i.uri) == wanted); + } let unfiltered = gathered.items.len(); let mut items = apply_state_filter(gathered.items, filter); // What `--limit` is about to cut down, which is one of the two things @@ -568,11 +627,12 @@ async fn in_this_repo(args: ListArgs) -> Result<()> { // honest sentence is about *you*, not about the repo. match (me.is_some(), source.spans_authors()) { (true, false) => { - println!("you have no pull requests on {}", repo.did); + println!("{} no pull requests on {}", whose.has(), repo.did); println!( - "Read from your PDS alone, so this says nothing about anyone \ + "Read from {} PDS alone, so this says nothing about anyone \ else's. Add an index to look: --source bobbin (alpha, and \ - its ingest stalls) or --source web (tangled.org's own, scraped)." + its ingest stalls) or --source web (tangled.org's own, scraped).", + whose.their(), ); } // Every source asked came back empty, and one of them @@ -581,8 +641,9 @@ async fn in_this_repo(args: ListArgs) -> Result<()> { (true, true) => { println!("no pull requests found for {}", repo.did); println!( - "Your PDS has none for it either, so this is probably right \ - rather than index lag." + "{} PDS has none for it either, so this is probably right \ + rather than index lag.", + whose.possessive_leading(), ); } (false, _) => { @@ -603,7 +664,11 @@ async fn in_this_repo(args: ListArgs) -> Result<()> { return Ok(()); } - note_author_scoped(source, me.is_some(), evidence.complete()); + note_author_scoped( + source, + me.is_some() && author.is_none(), + evidence.complete(), + ); // One lookup per unique author DID, run together rather than in turn. let handles = author_handles(&items).await; @@ -730,6 +795,10 @@ async fn across_repos(args: ListArgs) -> Result<()> { gathered.pds_total, Scope::Author, gathered.backfilled, + &match &args.author { + Some(who) => Whose::Named(author_label(who)), + None => Whose::Mine, + }, ); let evidence = gathered.evidence; diff --git a/src/cmd/pr/read/sources.rs b/src/cmd/pr/read/sources.rs index ae36bbb..d48fd87 100644 --- a/src/cmd/pr/read/sources.rs +++ b/src/cmd/pr/read/sources.rs @@ -683,7 +683,13 @@ pub(super) enum Scope { /// `backfilled` is how many other authors' pulls [`crate::clients::tangled::web::backfill`] managed /// to scrape back in. It changes what the incompleteness *means* — the gap is /// patched rather than open — so it changes the sentence. -pub(super) fn warn_stale(missing: &[Missing], pds_total: usize, scope: Scope, backfilled: usize) { +pub(super) fn warn_stale( + missing: &[Missing], + pds_total: usize, + scope: Scope, + backfilled: usize, + whose: &Whose, +) { let Some(newest) = missing.iter().max_by_key(|m| m.created_at.as_str()) else { return; }; @@ -695,26 +701,30 @@ pub(super) fn warn_stale(missing: &[Missing], pds_total: usize, scope: Scope, ba // patched from the web index, and honesty demands saying both that it // was and that the patch is a scrape with limits of its own. Scope::Repo if backfilled > 0 => format!( - "Its index is behind. Your own pulls came from your PDS and are complete;\n\ + "Its index is behind. The pulls it names came from {} PDS and are complete;\n\ {backfilled} other(s) were backfilled by scraping the tangled.org web\n\ - index: a temporary workaround (see TODO.md) that can itself miss pulls." + index: a temporary workaround (see TODO.md) that can itself miss pulls.", + whose.possessive(), ), // The sentence that matters. Merging two sources and printing the // union without this would present a listing as whole when the half // that can only come from Bobbin is demonstrably stale. - Scope::Repo => "Its index is behind. Your own pulls came from your PDS and are \ + Scope::Repo => format!( + "Its index is behind. The pulls it names came from {} PDS and are \ complete;\nother contributors' can only come from Bobbin, so this listing is \ - probably\nmissing theirs too." - .to_string(), + probably\nmissing theirs too.", + whose.possessive(), + ), Scope::Author => { "Its index is behind. The listing came from the PDS and is complete.".to_string() } }; crate::term::say::warning!( Index, - "Bobbin has not indexed {} of the {pds_total} pull request(s) in your PDS \ + "Bobbin has not indexed {} of the {pds_total} pull request(s) in {} PDS \ for this {}.\nnewest missing: {} {}\n{consequence}", missing.len(), + whose.possessive(), match scope { Scope::Repo => "repo", Scope::Author => "account", @@ -758,6 +768,60 @@ pub(super) fn unknown_states(items: &[Listed]) -> usize { items.iter().filter(|i| i.state == State::Unknown).count() } +/// Whose PDS a repo listing's complete half was read from. +/// +/// A repo listing is author-scoped whether or not anybody asked for that — +/// the records come out of one account's PDS — and every sentence this +/// module prints about completeness is a sentence about that account. Until +/// `--author` could narrow a repo listing there was only ever one answer and +/// "your" was hard-coded into all of them; now there are two, and a listing +/// of somebody else's pulls that says "your PDS" is wrong in the one way +/// this module tries hardest not to be. +pub(super) enum Whose { + /// The selected account: the listing is the reader's own. + Mine, + /// The account `--author` named, by whatever spelling resolved — a + /// handle where one resolved, the DID otherwise. + Named(String), +} + +impl Whose { + /// "your" / "@bob.example.com's" — the first mention in a sentence. + pub(super) fn possessive(&self) -> String { + match self { + Whose::Mine => "your".to_string(), + Whose::Named(who) => format!("{who}'s"), + } + } + + /// "your" / "their" — every mention after the first, where repeating the + /// handle reads like a second person. + pub(super) fn their(&self) -> &'static str { + match self { + Whose::Mine => "your", + Whose::Named(_) => "their", + } + } + + /// "Your" / "@bob.example.com's" — the same possessive where it opens a + /// sentence. Only `Mine` has a case to change; a handle carries its `@` + /// and is left alone. + pub(super) fn possessive_leading(&self) -> String { + match self { + Whose::Mine => "Your".to_string(), + Whose::Named(who) => format!("{who}'s"), + } + } + + /// "you have" / "@bob.example.com has". + pub(super) fn has(&self) -> String { + match self { + Whose::Mine => "you have".to_string(), + Whose::Named(who) => format!("{who} has"), + } + } +} + /// The footnote that keeps a repo listing honest about whose pulls it holds. /// /// Without this the change trades one silent wrong answer for another. A @@ -769,7 +833,8 @@ pub(super) fn unknown_states(items: &[Listed]) -> usize { /// /// Only fires when no index was consulted, and only when there was a PDS to /// read — with no account selected the listing has a different problem and -/// the empty-case message says so. +/// the empty-case message says so. `--author` silences it too: a reader who +/// named one account is not being told anything by "this is one account". /// /// `complete` is a claim about the walk and not about the PDS, which is why /// it is a parameter. Printed unconditionally it was false in exactly the @@ -785,7 +850,8 @@ pub(super) fn note_author_scoped(source: Source, had_account: bool, complete: bo "these are your pull requests on this repo, read from your PDS: complete, \ current, and\nsilent about anyone else's. Add an index to see theirs: \ --source bobbin (alpha, its\ningest stalls) or --source web (tangled.org's \ - own, scraped from its pages)." + own, scraped from its pages).\n\ + --author names one other account without an index; --all is yours everywhere." ), false => crate::term::say::note!( Index, @@ -793,7 +859,8 @@ pub(super) fn note_author_scoped(source: Source, had_account: bool, complete: bo current and\nsilent about anyone else's, but not all of your own — under \ --state all the walk\nstops at --limit records. Raise it to see further \ back, or add an index for\nothers': --source bobbin (alpha, its ingest \ - stalls) or --source web." + stalls) or --source web.\n\ + --author names one other account without an index; --all is yours everywhere." ), } } diff --git a/src/main.rs b/src/main.rs index 451244d..48bba93 100644 --- a/src/main.rs +++ b/src/main.rs @@ -653,10 +653,12 @@ mod tests { /// across every repo, so a reader had to be told which was which. A flag /// named for the scope sits among the other filters and says it outright. /// - /// `--author` is pinned to `--all` here because widening is per author - /// and not per repo: a pull record lives in the PDS of whoever wrote it, - /// so "every repo" is answerable for one account and there is no listing - /// of everyone's pulls everywhere for `--author` to narrow. + /// `--author` composes with it rather than depending on it. Widening is + /// still per author and not per repo — a pull record lives in the PDS of + /// whoever wrote it, so "every repo" is answerable for one account and + /// there is no listing of everyone's pulls everywhere. But *narrowing* a + /// repo listing to one account is exactly a repo listing read from that + /// account's PDS, which is what one has always been. #[test] fn your_pulls_across_every_repo_are_pr_list_all() { let cli = Cli::try_parse_from([ @@ -692,12 +694,20 @@ mod tests { _ => panic!("parsed into something other than pr list"), } - // An author with nothing to widen is a usage error, not a listing - // silently scoped to something the flag does not mean. - assert!( - Cli::try_parse_from(["atgc", "pr", "list", "--author", "bob.example.com"]).is_err(), - "--author without --all has no repo-scoped meaning to fall back on" - ); + // And without `--all` it is this repo, narrowed to that account — + // which used to be a usage error for want of an implementation + // rather than for want of a meaning. + let cli = Cli::try_parse_from(["atgc", "pr", "list", "--author", "bob.example.com"]) + .expect("--author narrows this repo"); + match cli.command { + Command::Pr { + command: PrCommand::List(args), + } => { + assert!(!args.all); + assert_eq!(args.author.as_deref(), Some("bob.example.com")); + } + _ => panic!("parsed into something other than pr list"), + } // The verb that used to carry this is gone, both spellings of it. assert!(Cli::try_parse_from(["atgc", "status", "pr"]).is_err()); diff --git a/tests/pr_flows.rs b/tests/pr_flows.rs index 75485e0..a528863 100644 --- a/tests/pr_flows.rs +++ b/tests/pr_flows.rs @@ -912,6 +912,88 @@ fn bobbins_pull_by_bob(world: &Scenario) { }); } +/// `--author` without `--all` narrows this repo to one account, by reading +/// *their* PDS instead of yours. +/// +/// A repo listing's complete half has always been one account's records, so +/// naming somebody else only changes which PDS is read — no index, no token, +/// and no permission on anything. The half worth asserting is the prose: a +/// listing of Bob's pulls that still says "your PDS" is the same class of +/// wrong answer the note exists to prevent. +#[test] +fn author_without_all_lists_this_repo_from_that_accounts_pds() { + let world = Scenario::new("pr-list-author-in-repo"); + feature_branch(&world); + open_pull(&world); + world.checkout.branch("bobs-branch"); + world + .checkout + .commit("bob.txt", "bob\n", "feat: bob's work", None); + world + .run_as(BOB, &["pr", "create", "--title", "bob's pull"]) + .success(); + world.clear_journal(); + + let listed = world + .run(&["pr", "list", "--author", BOB, "--json"]) + .success(); + let rows = listed.json(); + let rows = rows.as_array().expect("pr list --json prints an array"); + + assert_eq!(rows.len(), 1, "not exactly bob's pull: {rows:#?}"); + assert_eq!(rows[0]["title"].as_str(), Some("bob's pull")); + assert_eq!(rows[0]["author_did"].as_str(), Some(BOB)); + world.with(|w| { + assert!( + w.calls("bobbin").is_empty(), + "an index was contacted without being opted in" + ); + }); + // The bare listing's footnote is about the reader's own pulls, and a + // reader who named an account is not being told anything by it. + assert!( + !listed.stderr.contains("your pull requests on this repo"), + "--- stderr ---\n{}", + listed.stderr + ); + + // And the reader's own pull is not in it, which is the filter working + // rather than the account merely being unread. + let titles: Vec<&str> = rows.iter().filter_map(|r| r["title"].as_str()).collect(); + assert!(!titles.contains(&"a pull"), "{titles:?}"); +} + +/// An index source has every author in it, so `--author` has to narrow what +/// the index brought back too — not only which PDS was read. +#[test] +fn author_narrows_an_index_source_as_well_as_the_pds() { + let world = Scenario::new("pr-list-author-with-index"); + feature_branch(&world); + open_pull(&world); + bobbins_pull_by_bob(&world); + world.clear_journal(); + + let rows = world + .run(&[ + "pr", + "list", + "--author", + BOB, + "--source", + "pds,bobbin", + "--json", + ]) + .success() + .json(); + let rows = rows.as_array().expect("pr list --json prints an array"); + + assert_eq!(rows.len(), 1, "not exactly bob's pull: {rows:#?}"); + assert_eq!( + rows[0]["title"].as_str(), + Some("bob's pull, known only to the index") + ); +} + /// The default listing reads one PDS and does not contact the index at all. /// /// Both halves matter. That Bob's pull is absent is the visible half; that