This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9967

rhg: replace command structs with functions
ClosedPublic

Authored by SimonSapin on Feb 8 2021, 5:43 PM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHG184e46550dc8: rhg: replace command structs with functions
rHGb1921234de5d: rhg: replace command structs with functions
Summary

The Command trait was not used in any generic context,
and the struct where nothing more than holders for values parsed from CLI
arguments to be available to a run method.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

SimonSapin created this revision.Feb 8 2021, 5:43 PM
Alphare accepted this revision.Feb 9 2021, 3:53 AM
Alphare added a subscriber: Alphare.
Alphare added inline comments.
rust/rhg/src/commands/cat.rs
22

I just noticed that this uses values_of instead of values_of_os, so non-UTF8 filenames will cause a panic. We should address that in another patch.

Now that I think about it, we should also have the patch with catch_unwind we've been talking about for a while, to fallback to Python in case we get a panic.

SimonSapin added inline comments.Feb 9 2021, 4:10 AM
rust/rhg/src/commands/cat.rs
22

I’m not thrilled about silently falling back on panics instead of only when we opt to with HgError::unimplemented. Panics indicate that should be fixed and so need to be noticed.

Alphare added inline comments.Feb 9 2021, 4:12 AM
rust/rhg/src/commands/cat.rs
22

You're probably right. The catch_unwind patch would probably be for later to add some user-facing message like what hg does with a link to the bug tracker, etc.

SimonSapin added a commit: rHGb1921234de5d: rhg: replace command structs with functions.Feb 11 2021, 11:36 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHGb1921234de5d: rhg: replace command structs with functions (authored by SimonSapin). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 25501

rust/rhg/src/commands.rs

pub mod cat; pub mod cat;
pub mod debugdata; pub mod debugdata;
pub mod debugrequirements; pub mod debugrequirements;
pub mod files; pub mod files;
pub mod root; pub mod root;
use crate::error::CommandError;
use crate::ui::Ui;
use hg::config::Config;
/// The common trait for rhg commands
///
/// Normalize the interface of the commands provided by rhg
pub trait Command {
fn run(&self, ui: &Ui, config: &Config) -> Result<(), CommandError>;
}

rust/rhg/src/commands/cat.rs

use crate::commands::Command;
use crate::error::CommandError; use crate::error::CommandError;
use crate::ui::Ui; use crate::ui::Ui;
use clap::ArgMatches;
use hg::config::Config; use hg::config::Config;
use hg::operations::cat; use hg::operations::cat;
use hg::repo::Repo; use hg::repo::Repo;
use hg::utils::hg_path::HgPathBuf; use hg::utils::hg_path::HgPathBuf;
use micro_timer::timed; use micro_timer::timed;
use std::convert::TryFrom; use std::convert::TryFrom;
pub const HELP_TEXT: &str = " pub const HELP_TEXT: &str = "
Output the current or given revision of files Output the current or given revision of files
"; ";
pub struct CatCommand<'a> {
rev: Option<&'a str>,
files: Vec<&'a str>,
}
impl<'a> CatCommand<'a> {
pub fn new(rev: Option<&'a str>, files: Vec<&'a str>) -> Self {
Self { rev, files }
}
fn display(&self, ui: &Ui, data: &[u8]) -> Result<(), CommandError> {
ui.write_stdout(data)?;
Ok(())
}
}
impl<'a> Command for CatCommand<'a> {
#[timed] #[timed]
fn run(&self, ui: &Ui, config: &Config) -> Result<(), CommandError> { pub fn run(
ui: &Ui,
config: &Config,
args: &ArgMatches,
) -> Result<(), CommandError> {
let rev = args.value_of("rev");
let file_args = match args.values_of("files") {
AlphareUnsubmitted
Not Done

I just noticed that this uses values_of instead of values_of_os, so non-UTF8 filenames will cause a panic. We should address that in another patch.

Now that I think about it, we should also have the patch with catch_unwind we've been talking about for a while, to fallback to Python in case we get a panic.

Alphare: I just noticed that this uses `values_of` instead of `values_of_os`, so non-UTF8 filenames will…
SimonSapinAuthorUnsubmitted
Done

I’m not thrilled about silently falling back on panics instead of only when we opt to with HgError::unimplemented. Panics indicate that should be fixed and so need to be noticed.

SimonSapin: I’m not thrilled about silently falling back on panics instead of only when we opt to with…
AlphareUnsubmitted
Not Done

You're probably right. The catch_unwind patch would probably be for later to add some user-facing message like what hg does with a link to the bug tracker, etc.

Alphare: You're probably right. The `catch_unwind` patch would probably be for later to add some user…
Some(files) => files.collect(),
None => vec![],
};
let repo = Repo::find(config)?; let repo = Repo::find(config)?;
let cwd = hg::utils::current_dir()?; let cwd = hg::utils::current_dir()?;
let mut files = vec![]; let mut files = vec![];
for file in self.files.iter() { for file in file_args.iter() {
// TODO: actually normalize `..` path segments etc? // TODO: actually normalize `..` path segments etc?
let normalized = cwd.join(&file); let normalized = cwd.join(&file);
let stripped = normalized let stripped = normalized
.strip_prefix(&repo.working_directory_path()) .strip_prefix(&repo.working_directory_path())
// TODO: error message for path arguments outside of the repo // TODO: error message for path arguments outside of the repo
.map_err(|_| CommandError::abort(""))?; .map_err(|_| CommandError::abort(""))?;
let hg_file = HgPathBuf::try_from(stripped.to_path_buf()) let hg_file = HgPathBuf::try_from(stripped.to_path_buf())
.map_err(|e| CommandError::abort(e.to_string()))?; .map_err(|e| CommandError::abort(e.to_string()))?;
files.push(hg_file); files.push(hg_file);
} }
match self.rev { match rev {
Some(rev) => { Some(rev) => {
let data = cat(&repo, rev, &files).map_err(|e| (e, rev))?; let data = cat(&repo, rev, &files).map_err(|e| (e, rev))?;
self.display(ui, &data) ui.write_stdout(&data)?;
Ok(())
} }
None => Err(CommandError::Unimplemented.into()), None => Err(CommandError::Unimplemented.into()),
} }
} }
}

rust/rhg/src/commands/debugdata.rs

use crate::commands::Command;
use crate::error::CommandError; use crate::error::CommandError;
use crate::ui::Ui; use crate::ui::Ui;
use clap::ArgMatches;
use hg::config::Config; use hg::config::Config;
use hg::operations::{debug_data, DebugDataKind}; use hg::operations::{debug_data, DebugDataKind};
use hg::repo::Repo; use hg::repo::Repo;
use micro_timer::timed; use micro_timer::timed;
pub const HELP_TEXT: &str = " pub const HELP_TEXT: &str = "
Dump the contents of a data file revision Dump the contents of a data file revision
"; ";
pub struct DebugDataCommand<'a> { #[timed]
rev: &'a str, pub fn run(
kind: DebugDataKind, ui: &Ui,
} config: &Config,
args: &ArgMatches,
impl<'a> DebugDataCommand<'a> { ) -> Result<(), CommandError> {
pub fn new(rev: &'a str, kind: DebugDataKind) -> Self { let rev = args
DebugDataCommand { rev, kind } .value_of("rev")
.expect("rev should be a required argument");
let kind =
match (args.is_present("changelog"), args.is_present("manifest")) {
(true, false) => DebugDataKind::Changelog,
(false, true) => DebugDataKind::Manifest,
(true, true) => {
unreachable!("Should not happen since options are exclusive")
} }
(false, false) => {
unreachable!("Should not happen since options are required")
} }
};
impl<'a> Command for DebugDataCommand<'a> {
#[timed]
fn run(&self, ui: &Ui, config: &Config) -> Result<(), CommandError> {
let repo = Repo::find(config)?; let repo = Repo::find(config)?;
let data = debug_data(&repo, self.rev, self.kind) let data = debug_data(&repo, rev, kind).map_err(|e| (e, rev))?;
.map_err(|e| (e, self.rev))?;
let mut stdout = ui.stdout_buffer(); let mut stdout = ui.stdout_buffer();
stdout.write_all(&data)?; stdout.write_all(&data)?;
stdout.flush()?; stdout.flush()?;
Ok(()) Ok(())
} }
}

rust/rhg/src/commands/debugrequirements.rs

use crate::commands::Command;
use crate::error::CommandError; use crate::error::CommandError;
use crate::ui::Ui; use crate::ui::Ui;
use clap::ArgMatches;
use hg::config::Config; use hg::config::Config;
use hg::repo::Repo; use hg::repo::Repo;
pub const HELP_TEXT: &str = " pub const HELP_TEXT: &str = "
Print the current repo requirements. Print the current repo requirements.
"; ";
pub struct DebugRequirementsCommand {} pub fn run(
ui: &Ui,
impl DebugRequirementsCommand { config: &Config,
pub fn new() -> Self { _args: &ArgMatches,
DebugRequirementsCommand {} ) -> Result<(), CommandError> {
}
}
impl Command for DebugRequirementsCommand {
fn run(&self, ui: &Ui, config: &Config) -> Result<(), CommandError> {
let repo = Repo::find(config)?; let repo = Repo::find(config)?;
let mut output = String::new(); let mut output = String::new();
let mut requirements: Vec<_> = repo.requirements().iter().collect(); let mut requirements: Vec<_> = repo.requirements().iter().collect();
requirements.sort(); requirements.sort();
for req in requirements { for req in requirements {
output.push_str(req); output.push_str(req);
output.push('\n'); output.push('\n');
} }
ui.write_stdout(output.as_bytes())?; ui.write_stdout(output.as_bytes())?;
Ok(()) Ok(())
} }
}

rust/rhg/src/commands/files.rs

use crate::commands::Command;
use crate::error::CommandError; use crate::error::CommandError;
use crate::ui::Ui; use crate::ui::Ui;
use clap::ArgMatches;
use hg::config::Config; use hg::config::Config;
use hg::operations::list_rev_tracked_files; use hg::operations::list_rev_tracked_files;
use hg::operations::Dirstate; use hg::operations::Dirstate;
use hg::repo::Repo; use hg::repo::Repo;
use hg::utils::files::{get_bytes_from_path, relativize_path}; use hg::utils::files::{get_bytes_from_path, relativize_path};
use hg::utils::hg_path::{HgPath, HgPathBuf}; use hg::utils::hg_path::{HgPath, HgPathBuf};
pub const HELP_TEXT: &str = " pub const HELP_TEXT: &str = "
List tracked files. List tracked files.
Returns 0 on success. Returns 0 on success.
"; ";
pub struct FilesCommand<'a> { pub fn run(
rev: Option<&'a str>, ui: &Ui,
} config: &Config,
args: &ArgMatches,
) -> Result<(), CommandError> {
let rev = args.value_of("rev");
impl<'a> FilesCommand<'a> { let repo = Repo::find(config)?;
pub fn new(rev: Option<&'a str>) -> Self { if let Some(rev) = rev {
FilesCommand { rev } let files =
list_rev_tracked_files(&repo, rev).map_err(|e| (e, rev))?;
display_files(ui, &repo, files.iter())
} else {
let distate = Dirstate::new(&repo)?;
let files = distate.tracked_files()?;
display_files(ui, &repo, files)
}
} }
fn display_files( fn display_files<'a>(
&self,
ui: &Ui, ui: &Ui,
repo: &Repo, repo: &Repo,
files: impl IntoIterator<Item = &'a HgPath>, files: impl IntoIterator<Item = &'a HgPath>,
) -> Result<(), CommandError> { ) -> Result<(), CommandError> {
let cwd = hg::utils::current_dir()?; let cwd = hg::utils::current_dir()?;
let rooted_cwd = cwd let rooted_cwd = cwd
.strip_prefix(repo.working_directory_path()) .strip_prefix(repo.working_directory_path())
.expect("cwd was already checked within the repository"); .expect("cwd was already checked within the repository");
let rooted_cwd = HgPathBuf::from(get_bytes_from_path(rooted_cwd)); let rooted_cwd = HgPathBuf::from(get_bytes_from_path(rooted_cwd));
let mut stdout = ui.stdout_buffer(); let mut stdout = ui.stdout_buffer();
for file in files { for file in files {
stdout.write_all(relativize_path(file, &rooted_cwd).as_ref())?; stdout.write_all(relativize_path(file, &rooted_cwd).as_ref())?;
stdout.write_all(b"\n")?; stdout.write_all(b"\n")?;
} }
stdout.flush()?; stdout.flush()?;
Ok(()) Ok(())
} }
}
impl<'a> Command for FilesCommand<'a> {
fn run(&self, ui: &Ui, config: &Config) -> Result<(), CommandError> {
let repo = Repo::find(config)?;
if let Some(rev) = self.rev {
let files =
list_rev_tracked_files(&repo, rev).map_err(|e| (e, rev))?;
self.display_files(ui, &repo, files.iter())
} else {
let distate = Dirstate::new(&repo)?;
let files = distate.tracked_files()?;
self.display_files(ui, &repo, files)
}
}
}

rust/rhg/src/commands/root.rs

use crate::commands::Command;
use crate::error::CommandError; use crate::error::CommandError;
use crate::ui::Ui; use crate::ui::Ui;
use clap::ArgMatches;
use format_bytes::format_bytes; use format_bytes::format_bytes;
use hg::config::Config; use hg::config::Config;
use hg::repo::Repo; use hg::repo::Repo;
use hg::utils::files::get_bytes_from_path; use hg::utils::files::get_bytes_from_path;
pub const HELP_TEXT: &str = " pub const HELP_TEXT: &str = "
Print the root directory of the current repository. Print the root directory of the current repository.
Returns 0 on success. Returns 0 on success.
"; ";
pub struct RootCommand {} pub fn run(
ui: &Ui,
impl RootCommand { config: &Config,
pub fn new() -> Self { _args: &ArgMatches,
RootCommand {} ) -> Result<(), CommandError> {
}
}
impl Command for RootCommand {
fn run(&self, ui: &Ui, config: &Config) -> Result<(), CommandError> {
let repo = Repo::find(config)?; let repo = Repo::find(config)?;
let bytes = get_bytes_from_path(repo.working_directory_path()); let bytes = get_bytes_from_path(repo.working_directory_path());
ui.write_stdout(&format_bytes!(b"{}\n", bytes.as_slice()))?; ui.write_stdout(&format_bytes!(b"{}\n", bytes.as_slice()))?;
Ok(()) Ok(())
} }
}

rust/rhg/src/main.rs

extern crate log; extern crate log;
use clap::App; use clap::App;
use clap::AppSettings; use clap::AppSettings;
use clap::Arg; use clap::Arg;
use clap::ArgGroup; use clap::ArgGroup;
use clap::ArgMatches; use clap::ArgMatches;
use clap::SubCommand; use clap::SubCommand;
use format_bytes::format_bytes; use format_bytes::format_bytes;
use hg::operations::DebugDataKind;
use std::convert::TryFrom;
mod commands; mod commands;
mod error; mod error;
mod exitcode; mod exitcode;
mod ui; mod ui;
use commands::Command;
use error::CommandError; use error::CommandError;
fn main() { fn main() {
env_logger::init(); env_logger::init();
let app = App::new("rhg") let app = App::new("rhg")
.setting(AppSettings::AllowInvalidUtf8) .setting(AppSettings::AllowInvalidUtf8)
.setting(AppSettings::SubcommandRequired) .setting(AppSettings::SubcommandRequired)
.setting(AppSettings::VersionlessSubcommands) .setting(AppSettings::VersionlessSubcommands)
fn match_subcommand( fn match_subcommand(
matches: ArgMatches, matches: ArgMatches,
ui: &ui::Ui, ui: &ui::Ui,
) -> Result<(), CommandError> { ) -> Result<(), CommandError> {
let config = hg::config::Config::load()?; let config = hg::config::Config::load()?;
match matches.subcommand() { match matches.subcommand() {
("root", _) => commands::root::RootCommand::new().run(&ui, &config), ("root", Some(matches)) => commands::root::run(ui, &config, matches),
("files", Some(matches)) => { ("files", Some(matches)) => commands::files::run(ui, &config, matches),
commands::files::FilesCommand::try_from(matches)?.run(&ui, &config) ("cat", Some(matches)) => commands::cat::run(ui, &config, matches),
}
("cat", Some(matches)) => {
commands::cat::CatCommand::try_from(matches)?.run(&ui, &config)
}
("debugdata", Some(matches)) => { ("debugdata", Some(matches)) => {
commands::debugdata::DebugDataCommand::try_from(matches)? commands::debugdata::run(ui, &config, matches)
.run(&ui, &config)
} }
("debugrequirements", _) => { ("debugrequirements", Some(matches)) => {
commands::debugrequirements::DebugRequirementsCommand::new() commands::debugrequirements::run(ui, &config, matches)
.run(&ui, &config)
} }
_ => unreachable!(), // Because of AppSettings::SubcommandRequired, _ => unreachable!(), // Because of AppSettings::SubcommandRequired,
} }
} }
impl<'a> TryFrom<&'a ArgMatches<'_>> for commands::files::FilesCommand<'a> {
type Error = CommandError;
fn try_from(args: &'a ArgMatches) -> Result<Self, Self::Error> {
let rev = args.value_of("rev");
Ok(commands::files::FilesCommand::new(rev))
}
}
impl<'a> TryFrom<&'a ArgMatches<'_>> for commands::cat::CatCommand<'a> {
type Error = CommandError;
fn try_from(args: &'a ArgMatches) -> Result<Self, Self::Error> {
let rev = args.value_of("rev");
let files = match args.values_of("files") {
Some(files) => files.collect(),
None => vec![],
};
Ok(commands::cat::CatCommand::new(rev, files))
}
}
impl<'a> TryFrom<&'a ArgMatches<'_>>
for commands::debugdata::DebugDataCommand<'a>
{
type Error = CommandError;
fn try_from(args: &'a ArgMatches) -> Result<Self, Self::Error> {
let rev = args
.value_of("rev")
.expect("rev should be a required argument");
let kind = match (
args.is_present("changelog"),
args.is_present("manifest"),
) {
(true, false) => DebugDataKind::Changelog,
(false, true) => DebugDataKind::Manifest,
(true, true) => {
unreachable!("Should not happen since options are exclusive")
}
(false, false) => {
unreachable!("Should not happen since options are required")
}
};
Ok(commands::debugdata::DebugDataCommand::new(rev, kind))
}
}