D10090: rhg: Make configuration available as early as possible in main()

SimonSapin phabricator at mercurial-scm.org
Tue Mar 2 19:58:27 UTC 2021


SimonSapin created this revision.
Herald added a reviewer: hg-reviewers.
Herald added a subscriber: mercurial-patches.

REPOSITORY
  rHG Mercurial

BRANCH
  default

REVISION DETAIL
  https://phab.mercurial-scm.org/D10090

AFFECTED FILES
  rust/rhg/src/blackbox.rs
  rust/rhg/src/commands/config.rs
  rust/rhg/src/main.rs

CHANGE DETAILS

diff --git a/rust/rhg/src/main.rs b/rust/rhg/src/main.rs
--- a/rust/rhg/src/main.rs
+++ b/rust/rhg/src/main.rs
@@ -7,7 +7,10 @@
 use format_bytes::format_bytes;
 use hg::config::Config;
 use hg::repo::{Repo, RepoError};
-use std::path::{Path, PathBuf};
+use hg::utils::files::{get_bytes_from_os_str, get_path_from_bytes};
+use hg::utils::SliceExt;
+use std::ffi::OsString;
+use std::path::PathBuf;
 
 mod blackbox;
 mod error;
@@ -16,10 +19,11 @@
 use error::CommandError;
 
 fn main_with_result(
+    process_start_time: &blackbox::ProcessStartTime,
     ui: &ui::Ui,
-    process_start_time: &blackbox::ProcessStartTime,
+    repo: Result<&Repo, &NoRepoInCwdError>,
+    config: &Config,
 ) -> Result<(), CommandError> {
-    env_logger::init();
     let app = App::new("rhg")
         .global_setting(AppSettings::AllowInvalidUtf8)
         .setting(AppSettings::SubcommandRequired)
@@ -57,29 +61,11 @@
     let subcommand_args = subcommand_matches
         .expect("no subcommand arguments from clap despite AppSettings::SubcommandRequired");
 
-    let config_args = matches
-        .values_of_os("config")
-        // Turn `Option::None` into an empty iterator:
-        .into_iter()
-        .flatten()
-        .map(hg::utils::files::get_bytes_from_os_str);
-    let non_repo_config = &hg::config::Config::load(config_args)?;
-
-    let repo_path = matches.value_of_os("repository").map(Path::new);
-    let repo = match Repo::find(non_repo_config, repo_path) {
-        Ok(repo) => Ok(repo),
-        Err(RepoError::NotFound { at }) if repo_path.is_none() => {
-            // Not finding a repo is not fatal yet, if `-R` was not given
-            Err(NoRepoInCwdError { cwd: at })
-        }
-        Err(error) => return Err(error.into()),
-    };
-
     let invocation = CliInvocation {
         ui,
         subcommand_args,
-        non_repo_config,
-        repo: repo.as_ref(),
+        config,
+        repo,
     };
     let blackbox = blackbox::Blackbox::new(&invocation, process_start_time)?;
     blackbox.log_command_start();
@@ -94,17 +80,36 @@
     // measurements. Reading config files can be slow if they’re on NFS.
     let process_start_time = blackbox::ProcessStartTime::now();
 
+    env_logger::init();
     let ui = ui::Ui::new();
 
-    let result = main_with_result(&ui, &process_start_time);
-    if let Err(CommandError::Abort { message }) = &result {
-        if !message.is_empty() {
-            // Ignore errors when writing to stderr, we’re already exiting
-            // with failure code so there’s not much more we can do.
-            let _ = ui.write_stderr(&format_bytes!(b"abort: {}\n", message));
+    let early_args = EarlyArgs::parse(std::env::args_os());
+    let non_repo_config = Config::load(early_args.config)
+        .unwrap_or_else(|error| exit(&ui, Err(error.into())));
+
+    let repo_path = early_args.repo.as_deref().map(get_path_from_bytes);
+    let repo_result = match Repo::find(&non_repo_config, repo_path) {
+        Ok(repo) => Ok(repo),
+        Err(RepoError::NotFound { at }) if repo_path.is_none() => {
+            // Not finding a repo is not fatal yet, if `-R` was not given
+            Err(NoRepoInCwdError { cwd: at })
         }
-    }
-    std::process::exit(exit_code(&result))
+        Err(error) => exit(&ui, Err(error.into())),
+    };
+
+    let config = if let Ok(repo) = &repo_result {
+        repo.config()
+    } else {
+        &non_repo_config
+    };
+
+    let result = main_with_result(
+        &process_start_time,
+        &ui,
+        repo_result.as_ref(),
+        config,
+    );
+    exit(&ui, result)
 }
 
 fn exit_code(result: &Result<(), CommandError>) -> i32 {
@@ -118,6 +123,17 @@
     }
 }
 
+fn exit(ui: &Ui, result: Result<(), CommandError>) -> ! {
+    if let Err(CommandError::Abort { message }) = &result {
+        if !message.is_empty() {
+            // Ignore errors when writing to stderr, we’re already exiting
+            // with failure code so there’s not much more we can do.
+            let _ = ui.write_stderr(&format_bytes!(b"abort: {}\n", message));
+        }
+    }
+    std::process::exit(exit_code(&result))
+}
+
 macro_rules! subcommands {
     ($( $command: ident )+) => {
         mod commands {
@@ -157,7 +173,7 @@
 pub struct CliInvocation<'a> {
     ui: &'a Ui,
     subcommand_args: &'a ArgMatches<'a>,
-    non_repo_config: &'a Config,
+    config: &'a Config,
     /// References inside `Result` is a bit peculiar but allow
     /// `invocation.repo?` to work out with `&CliInvocation` since this
     /// `Result` type is `Copy`.
@@ -168,12 +184,45 @@
     cwd: PathBuf,
 }
 
-impl CliInvocation<'_> {
-    fn config(&self) -> &Config {
-        if let Ok(repo) = self.repo {
-            repo.config()
-        } else {
-            self.non_repo_config
+/// CLI arguments to be parsed "early" in order to be able to read
+/// configuration before using Clap. Ideally we would also use Clap for this,
+/// see <https://github.com/clap-rs/clap/discussions/2366>.
+///
+/// These arguments are still declared when we do use Clap later, so that Clap
+/// does not return an error for their presence.
+struct EarlyArgs {
+    /// Values of all `--config` arguments. (Possibly none)
+    config: Vec<Vec<u8>>,
+    /// Value of the `-R` or `--repository` argument, if any.
+    repo: Option<Vec<u8>>,
+}
+
+impl EarlyArgs {
+    fn parse(args: impl IntoIterator<Item = OsString>) -> Self {
+        let mut args = args.into_iter().map(get_bytes_from_os_str);
+        let mut config = Vec::new();
+        let mut repo = None;
+        // Use `while let` instead of `for` so that we can also call
+        // `args.next()` inside the loop.
+        while let Some(arg) = args.next() {
+            if arg == b"--config" {
+                if let Some(value) = args.next() {
+                    config.push(value)
+                }
+            } else if let Some(value) = arg.drop_prefix(b"--config=") {
+                config.push(value.to_owned())
+            }
+
+            if arg == b"--repository" || arg == b"-R" {
+                if let Some(value) = args.next() {
+                    repo = Some(value)
+                }
+            } else if let Some(value) = arg.drop_prefix(b"--repository=") {
+                repo = Some(value.to_owned())
+            } else if let Some(value) = arg.drop_prefix(b"-R") {
+                repo = Some(value.to_owned())
+            }
         }
+        Self { config, repo }
     }
 }
diff --git a/rust/rhg/src/commands/config.rs b/rust/rhg/src/commands/config.rs
--- a/rust/rhg/src/commands/config.rs
+++ b/rust/rhg/src/commands/config.rs
@@ -29,7 +29,7 @@
         .split_2(b'.')
         .ok_or_else(|| HgError::abort(""))?;
 
-    let value = invocation.config().get(section, name).unwrap_or(b"");
+    let value = invocation.config.get(section, name).unwrap_or(b"");
 
     invocation.ui.write_stdout(&format_bytes!(b"{}\n", value))?;
     Ok(())
diff --git a/rust/rhg/src/blackbox.rs b/rust/rhg/src/blackbox.rs
--- a/rust/rhg/src/blackbox.rs
+++ b/rust/rhg/src/blackbox.rs
@@ -52,20 +52,22 @@
         process_start_time: &'a ProcessStartTime,
     ) -> Result<Self, HgError> {
         let configured = if let Ok(repo) = invocation.repo {
-            let config = invocation.config();
-            if config.get(b"extensions", b"blackbox").is_none() {
+            if invocation.config.get(b"extensions", b"blackbox").is_none() {
                 // The extension is not enabled
                 None
             } else {
                 Some(ConfiguredBlackbox {
                     repo,
-                    max_size: config
+                    max_size: invocation
+                        .config
                         .get_byte_size(b"blackbox", b"maxsize")?
                         .unwrap_or(DEFAULT_MAX_SIZE),
-                    max_files: config
+                    max_files: invocation
+                        .config
                         .get_u32(b"blackbox", b"maxfiles")?
                         .unwrap_or(DEFAULT_MAX_FILES),
-                    date_format: config
+                    date_format: invocation
+                        .config
                         .get_str(b"blackbox", b"date-format")?
                         .unwrap_or(DEFAULT_DATE_FORMAT),
                 })



To: SimonSapin, #hg-reviewers
Cc: mercurial-patches, mercurial-devel


More information about the Mercurial-devel mailing list