From a8cca57c0af3c9ba90cd8eee840eda1519a2e524 Mon Sep 17 00:00:00 2001 From: Fintan Halpenny Date: Wed, 3 Jan 2024 14:39:22 +0000 Subject: [PATCH] cli: inline patch revision updates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of having a timeline of a patch be global and sorted by timestamp, organise it via revisions. Each revision has an entry in the timeline, sorted by timestamp. Under each revision, a series of updates are inlined, also sorted by their timestamps. Currently, these updates can either be reviews or merges. For example: ``` ├────────────────────────────────────────────────────────────────────────────┤ │ ● opened by fintohaps (you) 1 week ago │ │ └── ✓ accepted by fintohaps (you) 1 week ago │ │ ↑ updated to 5f2a123e3f2aa60f2e03815df283563495b73085 (a1c3c0f) 1 week ago │ │ └── ✓ accepted by fintohaps (you) 1 week ago │ │ └── ✓ merged by cloudhead (z6Mkt67…v4N1tRk) now │ ╰────────────────────────────────────────────────────────────────────────────╯ ``` Signed-off-by: Fintan Halpenny X-Clacks-Overhead: GNU Terry Pratchett --- radicle-cli/examples/rad-merge-via-push.md | 15 + radicle-cli/examples/rad-patch.md | 4 +- .../workflow/5-patching-maintainer.md | 8 +- radicle-cli/src/commands/patch/list.rs | 394 +++++++++++++----- radicle-cli/src/commands/patch/show.rs | 2 +- radicle/src/cob/patch.rs | 5 + 6 files changed, 329 insertions(+), 99 deletions(-) diff --git a/radicle-cli/examples/rad-merge-via-push.md b/radicle-cli/examples/rad-merge-via-push.md index 01917f0f..e50924ce 100644 --- a/radicle-cli/examples/rad-merge-via-push.md +++ b/radicle-cli/examples/rad-merge-via-push.md @@ -75,6 +75,21 @@ $ rad patch --merged │ ✔ [ ... ] Second change alice (you) daf349f +0 -0 now │ │ ✔ [ ... ] First change alice (you) 20aa5dd +0 -0 now │ ╰────────────────────────────────────────────────────────────────────╯ +$ rad patch show 8357a9f1d61e80309d314491aa754969d9f47d77 +╭────────────────────────────────────────────────────────────────╮ +│ Title Second change │ +│ Patch 8357a9f1d61e80309d314491aa754969d9f47d77 │ +│ Author alice (you) │ +│ Head daf349ff76bedf48c5f292290b682ee7be0683cf │ +│ Branches feature/2 │ +│ Commits ahead 0, behind 2 │ +│ Status merged │ +├────────────────────────────────────────────────────────────────┤ +│ daf349f Second change │ +├────────────────────────────────────────────────────────────────┤ +│ ● opened by alice (you) now │ +│ └─ ✓ merged by alice (you) at revision 8357a9f (d6399c7) now │ +╰────────────────────────────────────────────────────────────────╯ ``` We can verify that the remote tracking branches were also deleted: diff --git a/radicle-cli/examples/rad-patch.md b/radicle-cli/examples/rad-patch.md index dfebb6b6..13754a4c 100644 --- a/radicle-cli/examples/rad-patch.md +++ b/radicle-cli/examples/rad-patch.md @@ -179,7 +179,7 @@ $ rad patch show 6ff4f09 ├─────────────────────────────────────────────────────────────────────┤ │ ● opened by z6MknSL…StBU8Vi (you) now │ │ ↑ updated to e0fd9f00b51e10e1ca88868e68e46e859ed371d7 (27857ec) now │ -│ ✓ accepted by z6MknSL…StBU8Vi (you) now │ +│ └─ ✓ accepted by z6MknSL…StBU8Vi (you) now │ ╰─────────────────────────────────────────────────────────────────────╯ ``` @@ -205,6 +205,6 @@ $ rad patch show 6ff4f09 ├─────────────────────────────────────────────────────────────────────┤ │ ● opened by z6MknSL…StBU8Vi (you) now │ │ ↑ updated to e0fd9f00b51e10e1ca88868e68e46e859ed371d7 (27857ec) now │ -│ ✓ accepted by z6MknSL…StBU8Vi (you) now │ +│ └─ ✓ accepted by z6MknSL…StBU8Vi (you) now │ ╰─────────────────────────────────────────────────────────────────────╯ ``` diff --git a/radicle-cli/examples/workflow/5-patching-maintainer.md b/radicle-cli/examples/workflow/5-patching-maintainer.md index 09ed26b6..46ffe478 100644 --- a/radicle-cli/examples/workflow/5-patching-maintainer.md +++ b/radicle-cli/examples/workflow/5-patching-maintainer.md @@ -60,9 +60,12 @@ To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkE * [new branch] patch/3581e83 -> patches/3581e83ad18f5cdd806ab50fa11cfd5dd4e8ae1c ``` -Great, all fixed up, lets merge the code. +Great, all fixed up, lets accept and merge the code. ``` +$ rad patch review 3581e83 --revision abb0360 --accept +✓ Patch 3581e83 accepted +✓ Synced with 1 node(s) $ git checkout master Your branch is up to date with 'rad/master'. $ git merge patch/3581e83 @@ -103,7 +106,8 @@ $ rad patch show 3581e83 │ ● opened by bob z6Mkt67…v4N1tRk now │ │ ↑ updated to 6de8527cdf51f96e12649c7278efe1dccfdee885 (27857ec) now │ │ * revised by alice (you) in abb0360 (f567f69) now │ -│ ✓ merged by alice (you) at revision abb0360 (f567f69) now │ +│ └─ ✓ accepted by alice (you) now │ +│ └─ ✓ merged by alice (you) at revision abb0360 (f567f69) now │ ╰─────────────────────────────────────────────────────────────────────╯ ``` diff --git a/radicle-cli/src/commands/patch/list.rs b/radicle-cli/src/commands/patch/list.rs index 1765f1c2..e7b4bbea 100644 --- a/radicle-cli/src/commands/patch/list.rs +++ b/radicle-cli/src/commands/patch/list.rs @@ -1,18 +1,21 @@ use std::collections::BTreeSet; +use std::iter; +use radicle::cob; use radicle::cob::patch; use radicle::cob::patch::{Patch, PatchId, Patches, Verdict}; +use radicle::git; +use radicle::patch::{Merge, Review, Revision, RevisionId}; use radicle::prelude::*; use radicle::profile::Profile; use radicle::storage::git::Repository; -use radicle::storage::RemoteRepository as _; -use crate::terminal as term; use term::format::Author; use term::table::{Table, TableOptions}; use term::Element as _; use super::common; +use crate::terminal as term; /// List patches. pub fn run( @@ -126,111 +129,314 @@ pub fn row( ]) } -pub fn timeline( - profile: &Profile, - patch: &Patch, - repository: &Repository, -) -> anyhow::Result> { - let open = term::Line::spaced([ - term::format::positive("●").into(), - term::format::default("opened by").into(), - ]) - .space() - .extend(Author::new(patch.author().id(), profile).line()); +pub fn timeline<'a>( + profile: &'a Profile, + patch: &'a Patch, +) -> impl Iterator + 'a { + Timeline::build(profile, patch).into_lines(profile) +} - let mut timeline = vec![(patch.timestamp(), open)]; - let (root, _) = patch.root(); +/// The timeline of a [`Patch`]. +/// +/// A `Patch` will always have opened with a root revision and may +/// have a series of revisions that update the patch. +/// +/// The function, [`timeline`], builds a `Timeline` and converts it +/// into a series of [`term::Line`]s. +struct Timeline<'a> { + opened: Opened<'a>, + revisions: Vec>, +} - for (revision_id, revision) in patch.revisions() { - // Don't show an "update" line for the first revision. - if revision_id != root { - if revision.author() == patch.author() { - timeline.push(( +impl<'a> Timeline<'a> { + fn build(profile: &Profile, patch: &'a Patch) -> Self { + let opened = Opened::from_patch(patch, profile); + let mut revisions = patch + .revisions() + .skip(1) // skip the root revision since it's handled in `Opened::from_patch` + .map(|(id, revision)| { + ( revision.timestamp(), - term::Line::spaced( - [ - term::format::tertiary("↑").into(), - term::format::default("updated to").into(), - term::format::dim(revision_id).into(), - term::format::parens(term::format::secondary(term::format::oid( - revision.head(), - ))) - .into(), - ] - .into_iter(), - ), - )); + RevisionEntry::from_revision(patch, id, revision, profile), + ) + }) + .collect::>(); + revisions.sort_by_key(|(t, _)| *t); + Timeline { + opened, + revisions: revisions.into_iter().map(|(_, e)| e).collect(), + } + } + + fn into_lines(self, profile: &'a Profile) -> impl Iterator + 'a { + self.opened.into_lines(profile).chain( + self.revisions + .into_iter() + .flat_map(|r| r.into_lines(profile)), + ) + } +} + +/// The root `Revision` of the `Patch`. +struct Opened<'a> { + /// The `Author` of the patch. + author: Author<'a>, + /// When the patch was created. + timestamp: cob::Timestamp, + /// Any updates performed on the root `Revision`. + updates: Vec>, +} + +impl<'a> Opened<'a> { + fn from_patch(patch: &'a Patch, profile: &Profile) -> Self { + let (root, revision) = patch.root(); + let mut updates = Vec::new(); + updates.extend(revision.reviews().map(|(_, review)| { + ( + review.timestamp(), + Update::Reviewed { + review: review.clone(), + }, + ) + })); + updates.extend(patch.merges().filter_map(|(_, merge)| { + if merge.revision == root { + Some(( + merge.timestamp, + Update::Merged { + author: Author::new(&revision.author().id, profile), + merge: merge.clone(), + }, + )) } else { - let (alias, nid) = Author::new(revision.author().id(), profile).labels(); - - timeline.push(( - revision.timestamp(), - term::Line::spaced( - [ - term::format::tertiary("*").into(), - term::format::default("revised by").into(), - alias.clone(), - nid.clone(), - term::format::default("in").into(), - term::format::dim(term::format::oid(revision_id)).into(), - term::format::parens(term::format::secondary(term::format::oid( - revision.head(), - ))) - .into(), - ] - .into_iter(), - ), - )); + None } + })); + updates.sort_by_key(|(t, _)| *t); + Opened { + author: Author::new(&patch.author().id, profile), + timestamp: patch.timestamp(), + updates: updates.into_iter().map(|(_, up)| up).collect(), } + } - for (nid, merge) in patch.merges().filter(|(_, m)| m.revision == revision_id) { - let peer = repository.remote(nid)?; - let (alias, nid) = Author::new(&peer.id, profile).labels(); - let line = term::Line::spaced([ - term::format::primary("✓").bold().into(), - term::format::default("merged by").into(), - alias, - nid, - term::format::default("at revision").into(), - term::format::dim(term::format::oid(merge.revision)).into(), - term::format::parens(term::format::secondary(term::format::oid(merge.commit))) - .into(), - ]); - timeline.push((merge.timestamp, line)); - } - for (reviewer, review) in revision.reviews() { - let verdict = review.verdict(); - let verdict_symbol = match verdict { - Some(Verdict::Accept) => term::format::positive("✓"), - Some(Verdict::Reject) => term::format::negative("✗"), - None => term::format::dim("⋄"), - }; - let verdict_verb = match verdict { - Some(Verdict::Accept) => term::format::default("accepted"), - Some(Verdict::Reject) => term::format::default("rejected"), - None => term::format::default("reviewed"), - }; - let peer = repository.remote(reviewer)?; - let line = term::Line::spaced([ - verdict_symbol.into(), - verdict_verb.into(), - term::format::default("by").into(), + fn into_lines(self, profile: &'a Profile) -> impl Iterator + 'a { + iter::once( + term::Line::spaced([ + term::format::positive("●").into(), + term::format::default("opened by").into(), ]) .space() - .extend(Author::new(&peer.id, profile).line()); + .extend(self.author.line()) + .space() + .extend([term::format::dim(term::format::timestamp(self.timestamp)).into()]), + ) + .chain(self.updates.into_iter().map(|up| { + term::Line::spaced([term::Label::space(), term::Label::from("└─ ")]) + .extend(up.into_line(profile)) + })) + } +} - timeline.push((review.timestamp(), line)); +/// A revision entry in the [`Timeline`]. +enum RevisionEntry<'a> { + /// An `Updated` entry means that the original author of the + /// `Patch` created a new revision. + Updated { + /// When the `Revision` was created. + timestamp: cob::Timestamp, + /// The id of the `Revision`. + id: RevisionId, + /// The commit head of the `Revision`. + head: git::Oid, + /// All [`Update`]s that occurred on the `Revision`. + updates: Vec>, + }, + /// A `Revised` entry means that an author other than the original + /// author of the `Patch` created a new revision. + Revised { + /// The `Author` that created the `Revision` (that is not the + /// `Patch` author). + author: Author<'a>, + /// When the `Revision` was created. + timestamp: cob::Timestamp, + /// The id of the `Revision`. + id: RevisionId, + /// The commit head of the `Revision`. + head: git::Oid, + /// All [`Update`]s that occurred on the `Revision`. + updates: Vec>, + }, +} + +impl<'a> RevisionEntry<'a> { + fn from_revision( + patch: &Patch, + id: RevisionId, + revision: &'a Revision, + profile: &Profile, + ) -> Self { + let mut updates = Vec::new(); + updates.extend(revision.reviews().map(|(_, review)| { + ( + review.timestamp(), + Update::Reviewed { + review: review.clone(), + }, + ) + })); + updates.extend(patch.merges().filter_map(|(_, merge)| { + if merge.revision == id { + Some(( + merge.timestamp, + Update::Merged { + author: Author::new(&revision.author().id, profile), + merge: merge.clone(), + }, + )) + } else { + None + } + })); + updates.sort_by_key(|(t, _)| *t); + + if revision.author() == patch.author() { + RevisionEntry::Updated { + timestamp: revision.timestamp(), + id, + head: revision.head(), + updates: updates.into_iter().map(|(_, up)| up).collect(), + } + } else { + RevisionEntry::Revised { + author: Author::new(&revision.author().id, profile), + timestamp: revision.timestamp(), + id, + head: revision.head(), + updates: updates.into_iter().map(|(_, up)| up).collect(), + } } } - timeline.sort_by_key(|(t, _)| *t); - let mut lines = Vec::new(); - for (time, mut line) in timeline.into_iter() { - line.push(term::Label::space()); - line.push(term::format::dim(term::format::timestamp(time))); - lines.push(line); + fn into_lines(self, profile: &'a Profile) -> Vec { + match self { + RevisionEntry::Updated { + timestamp, + id, + head, + updates, + } => Self::updated(profile, timestamp, id, head, updates).collect(), + RevisionEntry::Revised { + author, + timestamp, + id, + head, + updates, + } => Self::revised(profile, author, timestamp, id, head, updates).collect(), + } } - Ok(lines) + fn updated( + profile: &'a Profile, + timestamp: cob::Timestamp, + id: RevisionId, + head: git::Oid, + updates: Vec>, + ) -> impl Iterator + 'a { + iter::once(term::Line::spaced([ + term::format::tertiary("↑").into(), + term::format::default("updated to").into(), + term::format::dim(id).into(), + term::format::parens(term::format::secondary(term::format::oid(head))).into(), + term::format::dim(term::format::timestamp(timestamp)).into(), + ])) + .chain(updates.into_iter().map(|up| { + term::Line::spaced([term::Label::space(), term::Label::from("└─ ")]) + .extend(up.into_line(profile)) + })) + } + + fn revised( + profile: &'a Profile, + author: Author<'a>, + timestamp: cob::Timestamp, + id: RevisionId, + head: git::Oid, + updates: Vec>, + ) -> impl Iterator + 'a { + let (alias, nid) = author.labels(); + iter::once(term::Line::spaced([ + term::format::tertiary("*").into(), + term::format::default("revised by").into(), + alias, + nid, + term::format::default("in").into(), + term::format::dim(term::format::oid(id)).into(), + term::format::parens(term::format::secondary(term::format::oid(head))).into(), + term::format::dim(term::format::timestamp(timestamp)).into(), + ])) + .chain(updates.into_iter().map(|up| { + term::Line::spaced([term::Label::space(), term::Label::from("└─ ")]) + .extend(up.into_line(profile)) + })) + } +} + +/// An update in the [`Patch`]'s timeline. +enum Update<'a> { + /// A revision of the patch was reviewed. + Reviewed { review: Review }, + /// A revision of the patch was merged. + Merged { author: Author<'a>, merge: Merge }, +} + +impl<'a> Update<'a> { + fn timestamp(&self) -> cob::Timestamp { + match self { + Update::Reviewed { review } => review.timestamp(), + Update::Merged { merge, .. } => merge.timestamp, + } + } + + fn into_line(self, profile: &Profile) -> term::Line { + let timestamp = self.timestamp(); + let mut line = match self { + Update::Reviewed { review } => { + let verdict = review.verdict(); + let verdict_symbol = match verdict { + Some(Verdict::Accept) => term::format::positive("✓"), + Some(Verdict::Reject) => term::format::negative("✗"), + None => term::format::dim("⋄"), + }; + let verdict_verb = match verdict { + Some(Verdict::Accept) => term::format::default("accepted"), + Some(Verdict::Reject) => term::format::default("rejected"), + None => term::format::default("reviewed"), + }; + term::Line::spaced([ + verdict_symbol.into(), + verdict_verb.into(), + term::format::default("by").into(), + ]) + .space() + .extend(Author::new(&review.author().id.into(), profile).line()) + } + Update::Merged { author, merge } => { + let (alias, nid) = author.labels(); + term::Line::spaced([ + term::format::primary("✓").bold().into(), + term::format::default("merged by").into(), + alias, + nid, + term::format::default("at revision").into(), + term::format::dim(term::format::oid(merge.revision)).into(), + term::format::parens(term::format::secondary(term::format::oid(merge.commit))) + .into(), + ]) + } + }; + line.push(term::Label::space()); + line.push(term::format::dim(term::format::timestamp(timestamp))); + line + } } diff --git a/radicle-cli/src/commands/patch/show.rs b/radicle-cli/src/commands/patch/show.rs index e18ee6b9..fef09291 100644 --- a/radicle-cli/src/commands/patch/show.rs +++ b/radicle-cli/src/commands/patch/show.rs @@ -152,7 +152,7 @@ pub fn run( .children(commits.into_iter().map(|l| l.boxed())) .divider(); - for line in list::timeline(profile, &patch, stored)? { + for line in list::timeline(profile, &patch) { widget.push(line); } widget.print(); diff --git a/radicle/src/cob/patch.rs b/radicle/src/cob/patch.rs index 4da82886..c2cbec04 100644 --- a/radicle/src/cob/patch.rs +++ b/radicle/src/cob/patch.rs @@ -1490,6 +1490,11 @@ impl Review { } } + /// Review author. + pub fn author(&self) -> &Author { + &self.author + } + /// Review verdict. pub fn verdict(&self) -> Option { self.verdict