mab122
/
radicle-heartwood-lfs
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

cli: move rad review to rad patch 

It is more natural for the review command to exist as a subcommand of
patch, since the reviews are for patches.

Remove the `rad review` command, moving its functionality to the `rad
patch` command. The rad-patch.md and rad-review-by-hunk.md tests are
modified to use the `rad patch review` command.

Signed-off-by: Fintan Halpenny <fintan.halpenny@gmail.com>
X-Clacks-Overhead: GNU Terry Pratchett
This commit is contained in:
Fintan Halpenny 2023-12-01 12:02:38 +00:00 committed by cloudhead
parent 6a3a66ae9c
commit ee9ee69130
No known key found for this signature in database
9 changed files with 253 additions and 317 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
radicle-cli/examples/rad-patch.md
View File

@ -154,7 +154,7 @@ $ rad patch checkout 6ff4f09
We can also add a review verdict as such:
```
$ rad review 6ff4f09 --accept --no-message --no-sync
$ rad patch review 6ff4f09 --accept --no-message
✓ Patch 6ff4f09 accepted
```

14
radicle-cli/examples/rad-review-by-hunk.md
View File

@ -70,7 +70,7 @@ Finally, we do a review of the patch by hunk. The output of this command should
match `git diff master -W100% -U5 --patience`:
```
$ rad review --no-sync --patch -U5 52da8356aa9beec08e6943cb3c4063fa37f3241b
$ rad patch review --patch -U5 52da8356aa9beec08e6943cb3c4063fa37f3241b
diff --git a/.gitignore b/.gitignore
deleted file mode 100644
index 7937fb3..0000000
@ -116,7 +116,7 @@ rename to notes/INSTRUCTIONS.txt
Now let's accept these hunks one by one..
```
$ rad review --no-sync --patch --accept --hunk 1 52da8356aa9beec08e6943cb3c4063fa37f3241b
$ rad patch review --patch --accept --hunk 1 52da8356aa9beec08e6943cb3c4063fa37f3241b
✓ Loaded existing review ([..]) for patch 52da8356aa9beec08e6943cb3c4063fa37f3241b
diff --git a/.gitignore b/.gitignore
deleted file mode 100644
@ -127,7 +127,7 @@ index 7937fb3..0000000
-*.draft
```
```
$ rad review --no-sync --patch --accept --hunk 1 52da8356aa9beec08e6943cb3c4063fa37f3241b
$ rad patch review --patch --accept --hunk 1 52da8356aa9beec08e6943cb3c4063fa37f3241b
✓ Loaded existing review ([..]) for patch 52da8356aa9beec08e6943cb3c4063fa37f3241b
diff --git a/DISCLAIMER.txt b/DISCLAIMER.txt
new file mode 100644
@ -138,7 +138,7 @@ index 0000000..2b5bd86
+All food is served as-is, with no warranty!
```
```
$ rad review --no-sync --patch --accept -U3 --hunk 1 52da8356aa9beec08e6943cb3c4063fa37f3241b
$ rad patch review --patch --accept -U3 --hunk 1 52da8356aa9beec08e6943cb3c4063fa37f3241b
✓ Loaded existing review ([..]) for patch 52da8356aa9beec08e6943cb3c4063fa37f3241b
diff --git a/MENU.txt b/MENU.txt
index 867958c..3af9741 100644
@ -153,7 +153,7 @@ index 867958c..3af9741 100644
[..]
```
```
$ rad review --no-sync --patch --accept -U3 --hunk 1 52da8356aa9beec08e6943cb3c4063fa37f3241b
$ rad patch review --patch --accept -U3 --hunk 1 52da8356aa9beec08e6943cb3c4063fa37f3241b
✓ Loaded existing review ([..]) for patch 52da8356aa9beec08e6943cb3c4063fa37f3241b
diff --git a/MENU.txt b/MENU.txt
index 4e2e828..3af9741 100644
@ -169,7 +169,7 @@ index 4e2e828..3af9741 100644
```
```
$ rad review --no-sync --patch --accept --hunk 1 52da8356aa9beec08e6943cb3c4063fa37f3241b
$ rad patch review --patch --accept --hunk 1 52da8356aa9beec08e6943cb3c4063fa37f3241b
✓ Loaded existing review ([..]) for patch 52da8356aa9beec08e6943cb3c4063fa37f3241b
diff --git a/INSTRUCTIONS.txt b/notes/INSTRUCTIONS.txt
similarity index 100%
@ -178,7 +178,7 @@ rename to notes/INSTRUCTIONS.txt
```
```
$ rad review --no-sync --patch --accept --hunk 1 52da8356aa9beec08e6943cb3c4063fa37f3241b
$ rad patch review --patch --accept --hunk 1 52da8356aa9beec08e6943cb3c4063fa37f3241b
✓ Loaded existing review ([..]) for patch 52da8356aa9beec08e6943cb3c4063fa37f3241b
✓ All hunks have been reviewed
```

2
radicle-cli/src/commands.rs
View File

@ -36,8 +36,6 @@ pub mod rad_path;
pub mod rad_publish;
#[path = "commands/remote.rs"]
pub mod rad_remote;
#[path = "commands/review.rs"]
pub mod rad_review;
#[path = "commands/self.rs"]
pub mod rad_self;
#[path = "commands/sync.rs"]

1
radicle-cli/src/commands/help.rs
View File

@ -26,7 +26,6 @@ const COMMANDS: &[Help] = &[
rad_node::HELP,
rad_patch::HELP,
rad_path::HELP,
rad_review::HELP,
rad_clean::HELP,
rad_self::HELP,
rad_track::HELP,

104
radicle-cli/src/commands/patch.rs
View File

@ -20,6 +20,8 @@ mod list;
mod ready;
#[path = "patch/redact.rs"]
mod redact;
#[path = "patch/review.rs"]
mod review;
#[path = "patch/show.rs"]
mod show;
#[path = "patch/update.rs"]
@ -53,6 +55,7 @@ Usage
rad patch archive <patch-id> [<option>...]
rad patch update <patch-id> [<option>...]
rad patch checkout <patch-id> [<option>...]
rad patch review <patch-id> [--accept | --reject] [-m [<string>]] [-d | --delete] [<option>...]
rad patch delete <patch-id> [<option>...]
rad patch redact <revision-id> [<option>...]
rad patch assign <revision-id> [--add <did>] [--delete <did>] [<option>...]
@ -75,6 +78,18 @@ Edit options
-m, --message [<string>] Provide a comment message to the patch or revision (default: prompt)
Review options
--revision <id> Review the given revision of the patch
-p, --patch Review by patch hunks
--hunk <index> Only review a specific hunk
--accept Accept a patch or set of hunks
--reject Reject a patch or set of hunks
-U, --unified <n> Generate diffs with <n> lines of context instead of the usual three
-d, --delete Delete a review draft
-r, --revision Review a patch revision
-m, --message [<string>] Provide a comment with the review (default: prompt)
Assign options
-a, --add <did> Add an assignee to the patch (may be specified multiple times).
@ -134,6 +149,7 @@ pub enum OperationName {
Checkout,
Comment,
Ready,
Review,
Label,
#[default]
List,
@ -205,6 +221,11 @@ pub enum Operation {
message: Message,
reply_to: Option<Rev>,
},
Review {
patch_id: Rev,
revision_id: Option<Rev>,
opts: review::Options,
},
Assign {
patch_id: Rev,
opts: AssignOptions,
@ -261,6 +282,7 @@ impl Args for Options {
let mut checkout_opts = checkout::Options::default();
let mut assign_opts = AssignOptions::default();
let mut label_opts = LabelOptions::default();
let mut review_op = review::Operation::default();
while let Some(arg) = parser.next()? {
match arg {
@ -314,6 +336,65 @@ impl Args for Options {
reply_to = Some(rev);
}
// Review options.
Long("revision") if op == Some(OperationName::Review) => {
let val = parser.value()?;
let rev = term::args::oid(&val)?;
revision_id = Some(rev);
}
Long("patch") | Short('p') if op == Some(OperationName::Review) => {
if let review::Operation::Review { by_hunk, .. } = &mut review_op {
*by_hunk = true;
} else {
return Err(arg.unexpected().into());
}
}
Long("unified") | Short('U') if op == Some(OperationName::Review) => {
if let review::Operation::Review { unified, .. } = &mut review_op {
let val = parser.value()?;
*unified = term::args::number(&val)?;
} else {
return Err(arg.unexpected().into());
}
}
Long("hunk") if op == Some(OperationName::Review) => {
if let review::Operation::Review { hunk, .. } = &mut review_op {
let val = parser.value()?;
let val = term::args::number(&val)
.map_err(|e| anyhow!("invalid hunk value: {e}"))?;
*hunk = Some(val);
} else {
return Err(arg.unexpected().into());
}
}
Long("delete") | Short('d') if op == Some(OperationName::Review) => {
review_op = review::Operation::Delete;
}
Long("accept") if op == Some(OperationName::Review) => {
if let review::Operation::Review {
verdict: verdict @ None,
..
} = &mut review_op
{
*verdict = Some(patch::Verdict::Accept);
} else {
return Err(arg.unexpected().into());
}
}
Long("reject") if op == Some(OperationName::Review) => {
if let review::Operation::Review {
verdict: verdict @ None,
..
} = &mut review_op
{
*verdict = Some(patch::Verdict::Reject);
} else {
return Err(arg.unexpected().into());
}
}
// Checkout options
Long("revision") if op == Some(OperationName::Checkout) => {
let val = parser.value()?;
@ -409,6 +490,7 @@ impl Args for Options {
"assign" => op = Some(OperationName::Assign),
"label" => op = Some(OperationName::Label),
"comment" => op = Some(OperationName::Comment),
"review" => op = Some(OperationName::Review),
"set" => op = Some(OperationName::Set),
unknown => anyhow::bail!("unknown operation '{}'", unknown),
},
@ -426,6 +508,7 @@ impl Args for Options {
Some(OperationName::Ready),
Some(OperationName::Checkout),
Some(OperationName::Comment),
Some(OperationName::Review),
Some(OperationName::Edit),
Some(OperationName::Set),
Some(OperationName::Assign),
@ -467,6 +550,15 @@ impl Args for Options {
message,
reply_to,
},
OperationName::Review => Operation::Review {
patch_id: patch_id
.ok_or_else(|| anyhow!("a patch or revision must be provided"))?,
revision_id,
opts: review::Options {
message,
op: review_op,
},
},
OperationName::Ready => Operation::Ready {
patch_id: patch_id.ok_or_else(|| anyhow!("a patch must be provided"))?,
undo,
@ -585,6 +677,18 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> {
&profile,
)?;
}
Operation::Review {
patch_id,
revision_id,
opts,
} => {
let patch_id = patch_id.resolve(&repository.backend)?;
let revision_id = revision_id
.map(|rev| rev.resolve::<radicle::git::Oid>(&repository.backend))
.transpose()?
.map(patch::RevisionId::from);
review::run(patch_id, revision_id, opts, &profile, &repository)?;
}
Operation::Edit { patch_id, message } => {
let patch_id = patch_id.resolve(&repository.backend)?;
edit::run(&patch_id, message, &profile, &repository)?;

141
radicle-cli/src/commands/patch/review.rs Normal file
View File

@ -0,0 +1,141 @@
#[path = "review/builder.rs"]
mod builder;
use anyhow::{anyhow, Context};
use radicle::cob::patch::{PatchId, Patches, RevisionId, Verdict};
use radicle::git;
use radicle::prelude::*;
use radicle::storage::git::Repository;
use crate::terminal as term;
use crate::terminal::patch::Message;
/// Review help message.
pub const REVIEW_HELP_MSG: &str = r#"
<!--
You may enter a review comment here. If you leave this blank,
no comment will be attached to your review.
Markdown supported.
-->
"#;
#[derive(Debug, PartialEq, Eq)]
pub enum Operation {
Delete,
Review {
by_hunk: bool,
unified: usize,
hunk: Option<usize>,
verdict: Option<Verdict>,
},
}
impl Default for Operation {
fn default() -> Self {
Self::Review {
by_hunk: false,
unified: 3,
hunk: None,
verdict: None,
}
}
}
#[derive(Debug, Default)]
pub struct Options {
pub message: Message,
pub op: Operation,
}
pub fn run(
patch_id: PatchId,
revision_id: Option<RevisionId>,
options: Options,
profile: &Profile,
repository: &Repository,
) -> anyhow::Result<()> {
let signer = term::signer(profile)?;
let _project = repository.identity_doc().context(format!(
"couldn't load project {} from local state",
repository.id
))?;
let mut patches = Patches::open(repository)?;
let mut patch = patches
.get_mut(&patch_id)
.context(format!("couldn't find patch {patch_id} locally"))?;
let (revision_id, revision) = match revision_id {
Some(id) => (
id,
patch
.revision(&id)
.ok_or_else(|| anyhow!("Patch revision `{id}` not found"))?,
),
None => patch.latest(),
};
let patch_id_pretty = term::format::tertiary(term::format::cob(&patch_id));
match options.op {
Operation::Review {
verdict,
by_hunk,
unified,
hunk,
} if by_hunk => {
let mut opts = git::raw::DiffOptions::new();
opts.patience(true)
.minimal(true)
.context_lines(unified as u32);
builder::ReviewBuilder::new(patch_id, *profile.id(), repository)
.hunk(hunk)
.verdict(verdict)
.run(revision, &mut opts)?;
}
Operation::Review { verdict, .. } => {
let message = options.message.get(REVIEW_HELP_MSG)?;
let message = message.replace(REVIEW_HELP_MSG.trim(), "");
let message = if message.is_empty() {
None
} else {
Some(message)
};
patch.review(revision_id, verdict, message, vec![], &signer)?;
match verdict {
Some(Verdict::Accept) => {
term::success!(
"Patch {} {}",
patch_id_pretty,
term::format::highlight("accepted")
);
}
Some(Verdict::Reject) => {
term::success!(
"Patch {} {}",
patch_id_pretty,
term::format::negative("rejected")
);
}
None => {
term::success!("Patch {} reviewed", patch_id_pretty);
}
}
}
Operation::Delete => {
let name = git::refs::storage::draft::review(profile.id(), &patch_id);
match repository.backend.find_reference(&name) {
Ok(mut r) => r.delete()?,
Err(e) => {
anyhow::bail!("Couldn't delete review reference '{name}': {e}");
}
}
}
}
Ok(())
}

0
radicle-cli/src/commands/review/builder.rs → radicle-cli/src/commands/patch/review/builder.rs
View File

299
radicle-cli/src/commands/review.rs
View File

@ -1,299 +0,0 @@
#[path = "review/builder.rs"]
mod builder;
use std::ffi::OsString;
use anyhow::{anyhow, Context};
use radicle::cob::patch::{PatchId, Patches, RevisionId, Verdict};
use radicle::prelude::*;
use radicle::{git, rad};
use crate::git::Rev;
use crate::terminal as term;
use crate::terminal::args::{string, Args, Error, Help};
use crate::terminal::patch::Message;
pub const HELP: Help = Help {
name: "review",
description: "Approve or reject a patch",
version: env!("CARGO_PKG_VERSION"),
usage: r#"
Usage
rad review [<patch-id>] [--accept | --reject] [-m [<string>]] [<option>...]
rad review [<patch-id>] [-d | --delete]
To specify a patch to review, use the fully qualified patch id
or an unambiguous prefix of it.
In scripting contexts, patch mode can be used non-interactively,
by passing eg. the `--hunk` and `--accept` options.
Options
-p, --patch Review by patch hunks
--hunk <index> Only review a specific hunk
--accept Accept a patch or set of hunks
--reject Reject a patch or set of hunks
-U, --unified <n> Generate diffs with <n> lines of context instead of the usual three
-d, --delete Delete a review draft
-r, --revision Review a patch revision
--[no-]sync Sync review to seed (default: sync)
-m, --message [<string>] Provide a comment with the review (default: prompt)
--help Print help
"#,
};
/// Review help message.
pub const REVIEW_HELP_MSG: &str = r#"
<!--
You may enter a review comment here. If you leave this blank,
no comment will be attached to your review.
Markdown supported.
-->
"#;
#[derive(Debug, PartialEq, Eq)]
pub enum Operation {
Delete,
Review {
by_hunk: bool,
unified: usize,
hunk: Option<usize>,
verdict: Option<Verdict>,
},
}
impl Default for Operation {
fn default() -> Self {
Self::Review {
by_hunk: false,
unified: 3,
hunk: None,
verdict: None,
}
}
}
#[derive(Debug)]
pub struct Options {
pub id: Rev,
pub revision: bool,
pub message: Message,
pub sync: bool,
pub verbose: bool,
pub op: Operation,
}
impl Args for Options {
fn from_args(args: Vec<OsString>) -> anyhow::Result<(Self, Vec<OsString>)> {
use lexopt::prelude::*;
let mut parser = lexopt::Parser::from_args(args);
let mut id: Option<Rev> = None;
let mut revision = false;
let mut message = Message::default();
let mut sync = true;
let mut verbose = false;
let mut op = Operation::default();
while let Some(arg) = parser.next()? {
match arg {
Long("help") | Short('h') => {
return Err(Error::Help.into());
}
Long("revision") | Short('r') => {
revision = true;
}
Long("sync") => {
// Skipping due the `no-sync` flag precedence.
}
Long("no-sync") => {
sync = false;
}
Long("message") | Short('m') => {
if message != Message::Blank {
let txt: String = parser.value()?.to_string_lossy().into();
message.append(&txt);
}
}
Long("no-message") => {
message = Message::Blank;
}
Long("patch") | Short('p') => {
if let Operation::Review { by_hunk, .. } = &mut op {
*by_hunk = true;
} else {
return Err(arg.unexpected().into());
}
}
Long("unified") | Short('U') => {
if let Operation::Review { unified, .. } = &mut op {
let val = parser.value()?;
*unified = term::args::number(&val)?;
} else {
return Err(arg.unexpected().into());
}
}
Long("hunk") => {
if let Operation::Review { hunk, .. } = &mut op {
let val = parser.value()?;
let val = term::args::number(&val)
.map_err(|e| anyhow!("invalid hunk value: {e}"))?;
*hunk = Some(val);
} else {
return Err(arg.unexpected().into());
}
}
Long("delete") | Short('d') => {
op = Operation::Delete;
}
Long("verbose") | Short('v') => {
verbose = true;
}
Long("accept") => {
if let Operation::Review {
verdict: verdict @ None,
..
} = &mut op
{
*verdict = Some(Verdict::Accept);
} else {
return Err(arg.unexpected().into());
}
}
Long("reject") => {
if let Operation::Review {
verdict: verdict @ None,
..
} = &mut op
{
*verdict = Some(Verdict::Reject);
} else {
return Err(arg.unexpected().into());
}
}
Value(val) => {
id = Some(Rev::from(string(&val)));
}
_ => return Err(anyhow::anyhow!(arg.unexpected())),
}
}
Ok((
Options {
id: id.ok_or_else(|| anyhow!("a patch to review must be provided"))?,
message,
sync,
revision,
verbose,
op,
},
vec![],
))
}
}
pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> {
let (_, id) =
rad::cwd().map_err(|_| anyhow!("this command must be run in the context of a project"))?;
let profile = ctx.profile()?;
let signer = term::signer(&profile)?;
let repository = profile.storage.repository(id)?;
let _project = repository
.identity_doc()
.context(format!("couldn't load project {id} from local state"))?;
let mut patches = Patches::open(&repository)?;
let (patch_id, revision) = if options.revision {
let id = options.id.resolve::<git::Oid>(&repository.backend)?;
let (patch_id, _, rev_id, rev) = patches
.find_by_revision(&RevisionId::from(id))?
.ok_or_else(|| anyhow!("revision {} does not exist", id))?;
(patch_id, Some((rev_id, rev)))
} else {
let id = options.id.resolve::<PatchId>(&repository.backend)?;
(id, None)
};
let mut patch = patches
.get_mut(&patch_id)
.context(format!("couldn't find patch {patch_id} locally"))?;
let (revision_id, revision) = if let Some(v) = revision {
v
} else {
let (id, r) = patch.latest();
(id, r.clone())
};
let patch_id_pretty = term::format::tertiary(term::format::cob(&patch_id));
match options.op {
Operation::Review {
verdict,
by_hunk,
unified,
hunk,
} if by_hunk => {
let mut opts = git::raw::DiffOptions::new();
opts.patience(true)
.minimal(true)
.context_lines(unified as u32);
builder::ReviewBuilder::new(patch_id, *profile.id(), &repository)
.hunk(hunk)
.verdict(verdict)
.run(&revision, &mut opts)?;
}
Operation::Review { verdict, .. } => {
let message = options.message.get(REVIEW_HELP_MSG)?;
let message = message.replace(REVIEW_HELP_MSG.trim(), "");
let message = if message.is_empty() {
None
} else {
Some(message)
};
patch.review(revision_id, verdict, message, vec![], &signer)?;
match verdict {
Some(Verdict::Accept) => {
term::success!(
"Patch {} {}",
patch_id_pretty,
term::format::highlight("accepted")
);
}
Some(Verdict::Reject) => {
term::success!(
"Patch {} {}",
patch_id_pretty,
term::format::negative("rejected")
);
}
None => {
term::success!("Patch {} reviewed", patch_id_pretty);
}
}
}
Operation::Delete => {
let name = git::refs::storage::draft::review(profile.id(), &patch_id);
match repository.backend.find_reference(&name) {
Ok(mut r) => r.delete()?,
Err(e) => {
anyhow::bail!("Couldn't delete review reference '{name}': {e}");
}
}
}
}
if options.sync {
term::warning("the `--sync` option is not yet supported");
}
Ok(())
}

7
radicle-cli/src/main.rs
View File

@ -208,13 +208,6 @@ fn run_other(exe: &str, args: &[OsString]) -> Result<(), Option<anyhow::Error>>
args.to_vec(),
);
}
"review" => {
term::run_command_args::<rad_review::Options, _>(
rad_review::HELP,
rad_review::run,
args.to_vec(),
);
}
"clean" => {
term::run_command_args::<rad_clean::Options, _>(
rad_clean::HELP,