From 864ca656c09d772cd465b518f7a857b8282e55f0 Mon Sep 17 00:00:00 2001 From: Fintan Halpenny Date: Fri, 8 Mar 2024 16:02:32 +0000 Subject: [PATCH] cli: add Unknown variant for NotificationKind When the `rad inbox` command would come across a reference name that it did not recognise as a `Cob` or `Branch`, then it would fail with an `Unknown` error. Instead, add an `Unknown` variant to `NotificationKind` which can be displayed in the `inbox` CLI, by using the `--show-unknown` flag. Otherwise, it will be skipped and the inbox will work as usual. While refactoring, the `TypedId` type is used for the `Cob` variant to simplify some of its uses. Signed-off-by: Fintan Halpenny X-Clacks-Overhead: GNU Terry Pratchett --- radicle-cli/src/commands/inbox.rs | 327 ++++++++++++++++-------- radicle/src/node/notifications.rs | 51 ++-- radicle/src/node/notifications/store.rs | 6 +- 3 files changed, 253 insertions(+), 131 deletions(-) diff --git a/radicle-cli/src/commands/inbox.rs b/radicle-cli/src/commands/inbox.rs index ad314ef9..b6e589a4 100644 --- a/radicle-cli/src/commands/inbox.rs +++ b/radicle-cli/src/commands/inbox.rs @@ -4,15 +4,17 @@ use std::process; use anyhow::anyhow; +use git_ref_format::Qualified; use localtime::LocalTime; +use radicle::cob::TypedId; use radicle::identity::Identity; use radicle::issue::cache::Issues as _; use radicle::node::notifications; use radicle::node::notifications::*; use radicle::patch::cache::Patches as _; use radicle::prelude::{Profile, RepoId}; -use radicle::storage::{ReadRepository, ReadStorage}; -use radicle::{cob, Storage}; +use radicle::storage::{BranchName, ReadRepository, ReadStorage}; +use radicle::{cob, git, Storage}; use term::Element as _; @@ -48,6 +50,7 @@ Options --repo Operate on the given repository (default: rad .) --sort-by Sort by `id` or `timestamp` (default: timestamp) --reverse, -r Reverse the list + --show-unknown Show any updates that were not recognized --help Print help "#, }; @@ -79,6 +82,7 @@ pub struct Options { op: Operation, mode: Mode, sort_by: SortBy, + show_unknown: bool, } impl Args for Options { @@ -91,6 +95,7 @@ impl Args for Options { let mut ids = Vec::new(); let mut reverse = None; let mut field = None; + let mut show_unknown = false; while let Some(arg) = parser.next()? { match arg { @@ -103,6 +108,9 @@ impl Args for Options { Long("reverse") | Short('r') => { reverse = Some(true); } + Long("show-unknown") => { + show_unknown = true; + } Long("sort-by") => { let val = parser.value()?; @@ -154,7 +162,15 @@ impl Args for Options { } }; - Ok((Options { op, mode, sort_by }, vec![])) + Ok(( + Options { + op, + mode, + sort_by, + show_unknown, + }, + vec![], + )) } } @@ -162,10 +178,22 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> { let profile = ctx.profile()?; let storage = &profile.storage; let mut notifs = profile.notifications_mut()?; - let Options { op, mode, sort_by } = options; + let Options { + op, + mode, + sort_by, + show_unknown, + } = options; match op { - Operation::List => list(mode, sort_by, ¬ifs.read_only(), storage, &profile), + Operation::List => list( + mode, + sort_by, + show_unknown, + ¬ifs.read_only(), + storage, + &profile, + ), Operation::Clear => clear(mode, &mut notifs), Operation::Show => show(mode, &mut notifs, storage, &profile), } @@ -174,6 +202,7 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> { fn list( mode: Mode, sort_by: SortBy, + show_unknown: bool, notifs: ¬ifications::StoreReader, storage: &Storage, profile: &Profile, @@ -181,17 +210,17 @@ fn list( let repos: Vec> = match mode { Mode::Contextual => { if let Ok((_, rid)) = radicle::rad::cwd() { - list_repo(rid, sort_by, notifs, storage, profile)? + list_repo(rid, sort_by, show_unknown, notifs, storage, profile)? .into_iter() .collect() } else { - list_all(sort_by, notifs, storage, profile)? + list_all(sort_by, show_unknown, notifs, storage, profile)? } } - Mode::ByRepo(rid) => list_repo(rid, sort_by, notifs, storage, profile)? + Mode::ByRepo(rid) => list_repo(rid, sort_by, show_unknown, notifs, storage, profile)? .into_iter() .collect(), - Mode::All => list_all(sort_by, notifs, storage, profile)?, + Mode::All => list_all(sort_by, show_unknown, notifs, storage, profile)?, Mode::ById(_) => anyhow::bail!("the `list` command does not take IDs"), }; @@ -207,6 +236,7 @@ fn list( fn list_all<'a>( sort_by: SortBy, + show_unknown: bool, notifs: ¬ifications::StoreReader, storage: &Storage, profile: &Profile, @@ -216,7 +246,7 @@ fn list_all<'a>( let mut vstacks = Vec::new(); for repo in repos { - let vstack = list_repo(repo.rid, sort_by, notifs, storage, profile)?; + let vstack = list_repo(repo.rid, sort_by, show_unknown, notifs, storage, profile)?; vstacks.extend(vstack.into_iter()); } Ok(vstacks) @@ -225,6 +255,7 @@ fn list_all<'a>( fn list_repo<'a, R: ReadStorage>( rid: RepoId, sort_by: SortBy, + show_unknown: bool, notifs: ¬ifications::StoreReader, storage: &R, profile: &Profile, @@ -257,86 +288,6 @@ where } else { term::format::tertiary(String::from("●")).into() }; - let (category, summary, state, name) = match n.kind { - NotificationKind::Branch { name } => { - let commit = if let Some(head) = n.update.new() { - repo.commit(head)?.summary().unwrap_or_default().to_owned() - } else { - String::new() - }; - - let state = match n - .update - .new() - .map(|oid| repo.is_ancestor_of(oid, head)) - .transpose() - { - Ok(Some(true)) => { - term::Paint::::from(term::format::secondary("merged")) - } - Ok(Some(false)) | Ok(None) => term::format::ref_update(n.update).into(), - Err(e) => return Err(e.into()), - } - .to_owned(); - - ( - "branch".to_string(), - commit, - state, - term::format::default(name.to_string()), - ) - } - NotificationKind::Cob { type_name, id } => { - let (category, summary, state) = if type_name == *cob::issue::TYPENAME { - let Some(issue) = issues.get(&id)? else { - // Issue could have been deleted after notification was created. - continue; - }; - ( - String::from("issue"), - issue.title().to_owned(), - term::format::issue::state(issue.state()), - ) - } else if type_name == *cob::patch::TYPENAME { - let Some(patch) = patches.get(&id)? else { - // Patch could have been deleted after notification was created. - continue; - }; - ( - String::from("patch"), - patch.title().to_owned(), - term::format::patch::state(patch.state()), - ) - } else if type_name == *cob::identity::TYPENAME { - let Ok(identity) = Identity::get(&id, &repo) else { - log::error!( - target: "cli", - "Error retrieving identity {id} for notification {}", n.id - ); - continue; - }; - let Some(rev) = n.update.new().and_then(|id| identity.revision(&id)) else { - log::error!( - target: "cli", - "Error retrieving identity revision for notification {}", n.id - ); - continue; - }; - ( - String::from("id"), - rev.title.clone(), - term::format::identity::state(&rev.state), - ) - } else { - ( - type_name.to_string(), - "".to_owned(), - term::format::default(String::new()), - ) - }; - (category, summary, state, term::format::cob(&id)) - } - }; let author = n .remote .map(|r| { @@ -344,15 +295,39 @@ where alias }) .unwrap_or_default(); + let notification_id = term::format::dim(format!("{:-03}", n.id)).into(); + let timestamp = term::format::italic(term::format::timestamp(n.timestamp)).into(); + + let NotificationRow { + category, + summary, + state, + name, + } = match &n.kind { + NotificationKind::Branch { name } => NotificationRow::branch(name, head, &n, &repo)?, + NotificationKind::Cob { typed_id } => { + match NotificationRow::cob(typed_id, &n, &issues, &patches, &repo)? { + Some(row) => row, + None => continue, + } + } + NotificationKind::Unknown { refname } => { + if show_unknown { + NotificationRow::unknown(refname, &n, &repo)? + } else { + continue; + } + } + }; table.push([ - term::format::dim(format!("{:-03}", n.id)).into(), + notification_id, seen, - term::format::tertiary(name).into(), + name.into(), summary.into(), - term::format::dim(category).into(), + category.into(), state.into(), author, - term::format::italic(term::format::timestamp(n.timestamp)).into(), + timestamp, ]); } @@ -369,6 +344,149 @@ where } } +struct NotificationRow { + category: term::Paint, + summary: term::Paint, + state: term::Paint, + name: term::Paint>, +} + +impl NotificationRow { + fn new( + category: String, + summary: String, + state: term::Paint, + name: term::Paint, + ) -> Self { + Self { + category: term::format::dim(category), + summary: term::Paint::new(summary), + state, + name: term::format::tertiary(name), + } + } + + fn branch( + name: &BranchName, + head: git::Oid, + n: &Notification, + repo: &S, + ) -> anyhow::Result + where + S: ReadRepository, + { + let commit = if let Some(head) = n.update.new() { + repo.commit(head)?.summary().unwrap_or_default().to_owned() + } else { + String::new() + }; + + let state = match n + .update + .new() + .map(|oid| repo.is_ancestor_of(oid, head)) + .transpose() + { + Ok(Some(true)) => term::Paint::::from(term::format::secondary("merged")), + Ok(Some(false)) | Ok(None) => term::format::ref_update(&n.update).into(), + Err(e) => return Err(e.into()), + } + .to_owned(); + + Ok(Self::new( + "branch".to_string(), + commit, + state, + term::format::default(name.to_string()), + )) + } + + fn cob( + typed_id: &TypedId, + n: &Notification, + issues: &I, + patches: &P, + repo: &S, + ) -> anyhow::Result> + where + S: ReadRepository + cob::Store, + I: cob::issue::cache::Issues, + P: cob::patch::cache::Patches, + { + let TypedId { id, .. } = typed_id; + let (category, summary, state) = if typed_id.is_issue() { + let Some(issue) = issues.get(id)? else { + // Issue could have been deleted after notification was created. + return Ok(None); + }; + ( + String::from("issue"), + issue.title().to_owned(), + term::format::issue::state(issue.state()), + ) + } else if typed_id.is_patch() { + let Some(patch) = patches.get(id)? else { + // Patch could have been deleted after notification was created. + return Ok(None); + }; + ( + String::from("patch"), + patch.title().to_owned(), + term::format::patch::state(patch.state()), + ) + } else if typed_id.is_identity() { + let Ok(identity) = Identity::get(id, repo) else { + log::error!( + target: "cli", + "Error retrieving identity {id} for notification {}", n.id + ); + return Ok(None); + }; + let Some(rev) = n.update.new().and_then(|id| identity.revision(&id)) else { + log::error!( + target: "cli", + "Error retrieving identity revision for notification {}", n.id + ); + return Ok(None); + }; + ( + String::from("id"), + rev.title.clone(), + term::format::identity::state(&rev.state), + ) + } else { + ( + typed_id.type_name.to_string(), + "".to_owned(), + term::format::default(String::new()), + ) + }; + Ok(Some(Self::new( + category, + summary, + state, + term::format::cob(id), + ))) + } + + fn unknown(refname: &Qualified<'static>, n: &Notification, repo: &S) -> anyhow::Result + where + S: ReadRepository, + { + let commit = if let Some(head) = n.update.new() { + repo.commit(head)?.summary().unwrap_or_default().to_owned() + } else { + String::new() + }; + Ok(Self::new( + "unknown".to_string(), + commit, + "".into(), + term::format::default(refname.to_string()), + )) + } +} + fn clear(mode: Mode, notifs: &mut notifications::StoreWriter) -> anyhow::Result<()> { let cleared = match mode { Mode::All => notifs.clear_all()?, @@ -412,20 +530,25 @@ fn show( let repo = storage.repository(n.repo)?; match n.kind { - NotificationKind::Cob { type_name, id } if type_name == *cob::issue::TYPENAME => { + NotificationKind::Cob { typed_id } if typed_id.is_issue() => { let issues = profile.issues(&repo)?; - let issue = issues.get(&id)?.unwrap(); + let issue = issues.get(&typed_id.id)?.unwrap(); - term::issue::show(&issue, &id, term::issue::Format::default(), profile)?; + term::issue::show( + &issue, + &typed_id.id, + term::issue::Format::default(), + profile, + )?; } - NotificationKind::Cob { type_name, id } if type_name == *cob::patch::TYPENAME => { + NotificationKind::Cob { typed_id } if typed_id.is_patch() => { let patches = profile.patches(&repo)?; - let patch = patches.get(&id)?.unwrap(); + let patch = patches.get(&typed_id.id)?.unwrap(); - term::patch::show(&patch, &id, false, &repo, None, profile)?; + term::patch::show(&patch, &typed_id.id, false, &repo, None, profile)?; } - NotificationKind::Cob { type_name, id } if type_name == *cob::identity::TYPENAME => { - let identity = Identity::get(&id, &repo)?; + NotificationKind::Cob { typed_id } if typed_id.is_identity() => { + let identity = Identity::get(&typed_id.id, &repo)?; term::json::to_pretty(&identity.doc, Path::new("radicle.json"))?.print(); } diff --git a/radicle/src/node/notifications.rs b/radicle/src/node/notifications.rs index a1ae9055..cbea534a 100644 --- a/radicle/src/node/notifications.rs +++ b/radicle/src/node/notifications.rs @@ -5,8 +5,8 @@ use serde::Serialize; use sqlite as sql; use thiserror::Error; -use crate::cob::object::ParseObjectId; -use crate::cob::{ObjectId, TypeName, TypeNameParse}; +use crate::cob; +use crate::cob::TypedId; use crate::git::{BranchName, Qualified}; use crate::prelude::RepoId; use crate::storage::{RefUpdate, RemoteId}; @@ -57,47 +57,44 @@ pub struct Notification { #[derive(Debug, PartialEq, Eq, Clone, Serialize)] pub enum NotificationKind { /// A COB changed. - Cob { type_name: TypeName, id: ObjectId }, + Cob { + #[serde(flatten)] + typed_id: TypedId, + }, /// A source branch changed. Branch { name: BranchName }, + /// Unknown reference. + Unknown { refname: Qualified<'static> }, } #[derive(Error, Debug)] pub enum NotificationKindError { - /// Invalid COB type name. - #[error("invalid type name: {0}")] - TypeName(#[from] TypeNameParse), - /// Invalid COB object id. - #[error("invalid object id: {0}")] - ObjectId(#[from] ParseObjectId), + #[error("invalid cob identifier: {0}")] + TypedId(#[from] cob::ParseIdentifierError), /// Invalid Git ref format. #[error("invalid ref format: {0}")] RefFormat(#[from] radicle_git_ext::ref_format::Error), - /// Unknown notification kind. - #[error("unknown notification kind {0:?}")] - Unknown(Qualified<'static>), } impl<'a> TryFrom> for NotificationKind { type Error = NotificationKindError; fn try_from(value: Qualified) -> Result { - let kind = match value.non_empty_iter() { - ("refs", "heads", head, rest) => NotificationKind::Branch { - name: [head] - .into_iter() - .chain(rest) - .collect::>() - .join("/") - .try_into()?, + let kind = match TypedId::from_qualified(&value)? { + Some(typed_id) => Self::Cob { typed_id }, + None => match value.non_empty_iter() { + ("refs", "heads", head, rest) => Self::Branch { + name: [head] + .into_iter() + .chain(rest) + .collect::>() + .join("/") + .try_into()?, + }, + _ => Self::Unknown { + refname: value.to_owned(), + }, }, - ("refs", "cobs", type_name, id) => NotificationKind::Cob { - type_name: type_name.parse()?, - id: id.collect::().parse()?, - }, - _ => { - return Err(NotificationKindError::Unknown(value.to_owned())); - } }; Ok(kind) } diff --git a/radicle/src/node/notifications/store.rs b/radicle/src/node/notifications/store.rs index a8748c0d..eb6f240f 100644 --- a/radicle/src/node/notifications/store.rs +++ b/radicle/src/node/notifications/store.rs @@ -621,8 +621,10 @@ mod test { qualified, update, kind: NotificationKind::Cob { - type_name: cob::issue::TYPENAME.clone(), - id: "d87dcfe8c2b3200e78b128d9b959cfdf7063fefe".parse().unwrap(), + typed_id: cob::TypedId { + type_name: cob::issue::TYPENAME.clone(), + id: "d87dcfe8c2b3200e78b128d9b959cfdf7063fefe".parse().unwrap(), + }, }, status: NotificationStatus::Unread, timestamp,