diff --git a/src/clients/tangled/resolve.rs b/src/clients/tangled/resolve.rs index aeba2fe..86a481c 100644 --- a/src/clients/tangled/resolve.rs +++ b/src/clients/tangled/resolve.rs @@ -296,7 +296,7 @@ pub fn repo_did_in_path(path: &str) -> Option { /// The `(owner, name)` the appview redirects `repo_did` to, the owner /// spelled as the appview spells it: a handle, usually. The one request -/// behind [`named`]. +/// behind [`named`] and [`repo_ref_by_did`]. /// /// Asked once per process per repo. An issue or pull view resolves the owner /// for its state walk and then names the repo, and a listing does the same @@ -337,6 +337,21 @@ fn owner_and_name_of(location: &str) -> Option<(String, String)> { } } +/// A [`RepoRef`] for a repo known only by its DID, named `owner/name` where +/// the appview's redirect says, and by the DID where it does not. Unlike +/// [`named`], the owner stays as the appview spells it and is not resolved. +/// +/// Best-effort: a lookup that fails costs the name, never the caller. +pub async fn repo_ref_by_did(repo_did: &str) -> RepoRef { + let web_url = match appview_path(repo_did).await { + Ok(Some((owner, name))) => { + format!("{}/{owner}/{name}", crate::clients::endpoints::appview()) + } + Ok(None) | Err(_) => String::new(), + }; + found(repo_did.to_string(), &web_url) +} + /// How many repo paths the process remembers. Repo DIDs come off records /// strangers wrote, so the map has a ceiling; past it lookups still happen /// and are not kept. diff --git a/src/cmd/issue/read.rs b/src/cmd/issue/read.rs index 79eea99..44dc35b 100644 --- a/src/cmd/issue/read.rs +++ b/src/cmd/issue/read.rs @@ -54,6 +54,7 @@ use crate::clients::atproto::record::Key; use crate::clients::git::run as git; use crate::clients::tangled::resolve; use crate::cmd::listing::cuts; +use crate::cmd::pr::read::{Whose, author_label}; use crate::lexicon::tangled::{ISSUE_NSID, ISSUE_STATE_NSID, IssueState}; use crate::term::column::{cell, day, ellipsize, pad_to}; use crate::term::text::one_line; @@ -1041,7 +1042,7 @@ async fn own_issues(did: &str, repo_did: Option<&str>, reach: Reach) -> Result "your issues".to_string(), - false => format!("{did}'s issues"), - }; + let whose = format!("{} issues", whose.possessive()); // "Complete" describes the walk, not the PDS, and saying it over a walk // that stopped at a screenful is the failure this parameter exists for: // a short listing then reads as a short collection. @@ -1236,6 +1234,15 @@ pub(crate) async fn list(args: ListArgs) -> Result<()> { } }; + // Named by the spelling `--author` was given, as `pr list` names it: a + // handle where one was typed, and yourself when it named you. + let whose = match &args.author { + Some(typed) if acting.as_deref() != Some(subject.as_str()) => { + Whose::Named(author_label(typed)) + } + _ => Whose::Mine, + }; + let repo = match args.all { true => None, false => { @@ -1355,8 +1362,8 @@ pub(crate) async fn list(args: ListArgs) -> Result<()> { let why = match (filter, unfiltered) { (Some(state), n) if n > 0 => format!("no {state} issues"), _ => match &repo { - Some(repo) => format!("no issues for {subject} on {}", repo.linked()), - None => format!("no issues for {subject}"), + Some(repo) => format!("{} no issues on {}", whose.has(), repo.linked()), + None => format!("{} no issues", whose.has()), }, }; // The reason goes to stderr under `--json` and to stdout otherwise, @@ -1373,12 +1380,7 @@ pub(crate) async fn list(args: ListArgs) -> Result<()> { false => println!("{why}"), } cuts(evidence, matched, rows.len(), "issue records"); - note_author_scoped( - &subject, - args.author.is_none(), - spans_authors, - evidence.complete(), - ); + note_author_scoped(&whose, spans_authors, evidence.complete()); note_unknown_states(unknown, filter.is_some()); return Ok(()); } @@ -1416,16 +1418,24 @@ pub(crate) async fn list(args: ListArgs) -> Result<()> { .collect(); crate::term::jsonout::emit(&listed)?; cuts(evidence, matched, rows.len(), "issue records"); - note_author_scoped( - &subject, - args.author.is_none(), - spans_authors, - evidence.complete(), - ); + note_author_scoped(&whose, spans_authors, evidence.complete()); note_unknown_states(unknown, filter.is_some()); return Ok(()); } + // One name per repo on the page. The state walk above already asked the + // appview for each PDS row's repo owner, and that answer carries the + // name; only a repo that came from the index alone costs a request. + let mut repo_labels: HashMap = HashMap::new(); + if args.all { + for did in rows.iter().filter_map(Listed::repo_did) { + if !repo_labels.contains_key(did) { + let label = resolve::repo_ref_by_did(did).await.label().to_string(); + repo_labels.insert(did.to_string(), label); + } + } + } + for row in &rows { let number = match numbers.get(&row.uri) { Some(number) => format!("#{number}"), @@ -1456,7 +1466,10 @@ pub(crate) async fn list(args: ListArgs) -> Result<()> { }; match args.all { true => { - let repo = row.repo_did().unwrap_or("?"); + let repo = row + .repo_did() + .and_then(|did| repo_labels.get(did)) + .map_or("?".to_string(), |label| one_line(label)); println!( "{} {} {} {}{author} {} {repo}", pad_to(&number, 5), @@ -1478,16 +1491,11 @@ pub(crate) async fn list(args: ListArgs) -> Result<()> { if let Some(repo) = &repo { println!( "\nview: {}", - crate::term::hyperlink::url(&issues_url(&repo.did)) + crate::term::hyperlink::url(&format!("{}/issues", repo.web_url)) ); } cuts(evidence, matched, rows.len(), "issue records"); - note_author_scoped( - &subject, - args.author.is_none(), - spans_authors, - evidence.complete(), - ); + note_author_scoped(&whose, spans_authors, evidence.complete()); note_unknown_states(unknown, filter.is_some()); Ok(()) } @@ -1698,22 +1706,20 @@ pub(crate) async fn view(args: ViewArgs) -> Result<()> { println!("author: {author}"); println!("state: {}", state.label()); println!("opened: {}", day(issue.created_at().unwrap_or(""))); - if let Some(repo) = issue.repo_did() { - // The DID is what the record holds and the only name available - // without another lookup; the appview 302s it to `owner/name`, so - // linking it costs nothing and lands on the right page. - println!( - "repo: {}", - crate::term::hyperlink::wrap(&repo_url(repo), repo) - ); + // `state_of` asked the appview who owns this repo, and the answer named + // it too, so this is not another request. + let repo = match issue.repo_did() { + Some(did) => Some(resolve::repo_ref_by_did(did).await), + None => None, + }; + if let Some(repo) = &repo { + println!("repo: {}", repo.linked()); } println!("uri: {}", issue.uri); - if let Some(repo) = issue.repo_did() { + if let Some(repo) = &repo { let page = match number { - Some(number) => { - crate::clients::tangled::web::issues::issue_url(&repo_url(repo), number) - } - None => issues_url(repo), + Some(number) => crate::clients::tangled::web::issues::issue_url(&repo.web_url, number), + None => format!("{}/issues", repo.web_url), }; println!("view: {}", crate::term::hyperlink::url(&page)); } diff --git a/src/cmd/pr/read/mod.rs b/src/cmd/pr/read/mod.rs index 3e61a0a..b312981 100644 --- a/src/cmd/pr/read/mod.rs +++ b/src/cmd/pr/read/mod.rs @@ -55,7 +55,7 @@ pub(in crate::cmd) use crate::model::pull::StackRow; pub(super) use labels::target_repo_did; pub(crate) use sources::{Source, pds_pulls}; pub(in crate::cmd) use sources::{ - add_author_rows, branch_pull_record, for_branch, pull_page, repo_rows, + Whose, add_author_rows, branch_pull_record, for_branch, pull_page, repo_rows, }; use crate::clients::atproto::pds::Reach; @@ -68,7 +68,7 @@ use crate::term::text::one_line; use anyhow::Result; use labels::{RepoName, author_did, author_labels, numbers_across_repos, repo_names}; use sources::{ - Ask, Empty, Scope, State, Whose, add_author_pulls, apply_state_filter, classify_empty, gather, + Ask, Empty, Scope, State, add_author_pulls, apply_state_filter, classify_empty, gather, gather_branch_pulls, note_author_scoped, note_unknown_states, own_did, require_branch_pull, unknown_states, warn_stale, }; @@ -516,7 +516,7 @@ pub(super) async fn list(args: ListArgs) -> Result<()> { /// 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 { +pub(in crate::cmd) fn author_label(input: &str) -> String { match crate::lexicon::identity::looks_like_a_did(input) { true => input.to_string(), false => format!("@{}", input.trim_start_matches('@')), diff --git a/src/cmd/pr/read/sources.rs b/src/cmd/pr/read/sources.rs index 5190917..3670baa 100644 --- a/src/cmd/pr/read/sources.rs +++ b/src/cmd/pr/read/sources.rs @@ -872,7 +872,7 @@ pub(super) fn unknown_states(items: &[Listed]) -> usize { /// "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 { +pub(in crate::cmd) enum Whose { /// The selected account: the listing is the reader's own. Mine, /// The account `--author` named, by whatever spelling resolved — a @@ -882,7 +882,7 @@ pub(super) enum Whose { impl Whose { /// "your" / "@bob.example.com's" — the first mention in a sentence. - pub(super) fn possessive(&self) -> String { + pub(in crate::cmd) fn possessive(&self) -> String { match self { Whose::Mine => "your".to_string(), Whose::Named(who) => format!("{who}'s"), @@ -909,7 +909,7 @@ impl Whose { } /// "you have" / "@bob.example.com has". - pub(super) fn has(&self) -> String { + pub(in crate::cmd) fn has(&self) -> String { match self { Whose::Mine => "you have".to_string(), Whose::Named(who) => format!("{who} has"), diff --git a/tests/issue_flows.rs b/tests/issue_flows.rs index 61f2ad8..6333811 100644 --- a/tests/issue_flows.rs +++ b/tests/issue_flows.rs @@ -1230,7 +1230,8 @@ fn an_issue_number_names_the_record_on_its_page() { } let text = world.run(&["issue", "view", "39"]).success(); assert!(text.stdout.contains("#39 a bug"), "{}", text.stdout); - let page = format!("{}/{REPO_DID}/issues/39", world.appview_url()); + // The link is the repo's named page, from the same owner lookup. + let page = format!("{}/{ALICE}/demo/issues/39", world.appview_url()); assert!( text.stdout.contains(&page), "the view line should link the issue's own page:\n{}", @@ -1718,3 +1719,146 @@ fn an_author_can_reopen_what_the_repo_owner_closed() { .json(); assert_eq!(view["state"].as_str(), Some("open"), "{view:#}"); } + +// --------------------------------------------------------------------------- +// names +// --------------------------------------------------------------------------- + +/// Point the appview's `/` redirect at an owner spelled by handle, +/// the way tangled.org spells it. +fn name_repo(world: &Scenario, repo: &str, owner: &str, name: &str) { + world.with(|w| { + w.repo_owners + .insert(repo.to_string(), (owner.to_string(), name.to_string())); + }); +} + +/// How many times the appview was asked where `repo` lives. +fn redirects(world: &Scenario, repo: &str) -> usize { + world.with(|w| w.calls_to(repo).len()) +} + +/// **A repo is named, not identified.** `issue view` printed +/// `repo: did:plc:…`. The state walk already asks the appview who owns the +/// repo, and that redirect names it, so the name costs no request. +#[test] +fn viewing_an_issue_names_its_repo_from_the_owner_lookup() { + let world = Scenario::new("issue-view-names-repo"); + name_repo(&world, REPO_DID, "alice.test", "demo"); + let rkey = open_issue(&world, "a bug"); + + let before = redirects(&world, REPO_DID); + let run = world.run(&["issue", "view", &rkey]).success(); + assert_eq!( + redirects(&world, REPO_DID) - before, + 1, + "naming the repo asked the appview again" + ); + assert!( + run.stdout.contains("repo: alice.test/demo\n"), + "the repo was not named\n--- stdout ---\n{}", + run.stdout + ); + assert!( + run.stdout.contains("/alice.test/demo/issues"), + "the view link does not name the repo\n--- stdout ---\n{}", + run.stdout + ); + + // `--json` is for scripts, and keeps the DID. + let json = world + .run(&["issue", "view", &rkey, "--json"]) + .success() + .json(); + assert_eq!(json["repo_did"].as_str(), Some(REPO_DID)); +} + +/// `issue list --all` names each row's repo, once per repo and from the +/// lookup the state walk made, and keeps the DID only where nothing named it. +#[test] +fn listing_every_repo_names_each_one() { + let world = Scenario::new("issue-list-all-names-repos"); + name_repo(&world, REPO_DID, "alice.test", "demo"); + open_issue(&world, "here"); + open_issue(&world, "also here"); + world.with(|w| { + w.plant( + ALICE, + ISSUE_NSID, + "3zzzelsewhere1", + serde_json::json!({ + "$type": ISSUE_NSID, + "repo": OTHER_REPO, + "title": "somewhere else", + "createdAt": "2026-08-01T00:00:00Z", + }), + ); + }); + + let before = redirects(&world, REPO_DID); + let run = world + .run(&["issue", "list", "--all", "--state", "all"]) + .success(); + assert_eq!(redirects(&world, REPO_DID) - before, 1); + let line = |title: &str| { + run.stdout + .lines() + .find(|l| l.contains(title)) + .unwrap_or_else(|| panic!("no row for {title}\n{}", run.stdout)) + .to_string() + }; + assert!( + line("also here").ends_with(" alice.test/demo"), + "{}", + run.stdout + ); + assert!( + line("somewhere else").ends_with(OTHER_REPO), + "{}", + run.stdout + ); +} + +/// An empty listing says whose issues it read, by the spelling `--author` +/// was given, as `pr list` does. It used to print the subject's DID. +#[test] +fn an_empty_listing_names_whose_issues_it_read() { + let world = Scenario::new("issue-list-empty-names-account"); + + let mine = world.run(&["issue", "list", "--state", "all"]).success(); + assert!( + mine.stdout.starts_with("you have no issues on "), + "{}", + mine.stdout + ); + + let theirs = world + .run(&["issue", "list", "--state", "all", "--author", "bob.test"]) + .success(); + assert!( + theirs.stdout.starts_with("@bob.test has no issues on "), + "{}", + theirs.stdout + ); + assert!( + theirs.stderr.contains("@bob.test's issues"), + "{}", + theirs.stderr + ); + assert!( + !theirs.stdout.contains(BOB) && !theirs.stderr.contains(BOB), + "the account was named by its DID\n--- stdout ---\n{}\n--- stderr ---\n{}", + theirs.stdout, + theirs.stderr + ); + + // Naming yourself reads like the bare listing. + let me = world + .run(&["issue", "list", "--state", "all", "--author", "alice.test"]) + .success(); + assert!( + me.stdout.starts_with("you have no issues on "), + "{}", + me.stdout + ); +}