This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9964

rhg: Parse per-repository configuration
ClosedPublic

Authored by SimonSapin on Feb 5 2021, 4:26 AM.

Details

Reviewers
Alphare
pulkit
Group Reviewers
hg-reviewers
Commits
rHGd7685105e504: rhg: Parse per-repository configuration

Diff Detail

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

Event Timeline

SimonSapin created this revision.Feb 5 2021, 4:26 AM
Alphare added a subscriber: Alphare.Feb 8 2021, 9:38 AM
Alphare added inline comments.
rust/hg-core/src/config/config.rs
29

I'm not a big fan of adding a cache that can become potentially fragile for what I'm assuming to be so little performance gained. Maybe iterating in reverse through the layers until you hit one that is not from the CLI would be good enough?

SimonSapin added inline comments.Feb 8 2021, 11:38 AM
rust/hg-core/src/config/config.rs
29

I was not at all thinking of this as a cache. Do you mean we could filter based on ConfigOrigin? I had not considered that but yeah it’s probably less fragile. Or, what do you think of having three Vecs instead of one? System+user, repo, and CLI layers kept separately.

Alphare added inline comments.Feb 8 2021, 11:46 AM
rust/hg-core/src/config/config.rs
29

Currently, the ordering of layers gives us an easy answer to precedence that we lose if we split into multiple Vec. I say we keep it this way until we feel that this compromise is not worth it anymore and we need another abstraction layer.

SimonSapin added inline comments.Feb 8 2021, 11:52 AM
rust/hg-core/src/config/config.rs
29

With three vecs the precedence logic would be centralized in a method that returns impl Iterator<Item=&ConfigLayer>.

Which do you mean by keep it this way? With RangeFrom<usize>?

Alphare added inline comments.Feb 8 2021, 11:56 AM
rust/hg-core/src/config/config.rs
29

With three vecs the precedence logic would be centralized in a method that returns impl Iterator<Item=&ConfigLayer>.

Right, while it's not particularly harder, I didn't feel like it was needed when adding the config logic. If you think that it'll be frequent enough to query the separate "groups" of layers then by all means, go ahead. :)

Which do you mean by keep it this way? With RangeFrom<usize>?

I meant using a single Vec. I am still -1 on using RangeFrom<usize>.

SimonSapin added inline comments.Feb 8 2021, 12:48 PM
rust/hg-core/src/config/config.rs
29

It’s not frequent, combine_with_repo is the only thing that needs handle these hypothetically-separate vecs separately. And the separation would be purely to make the code simpler and less fragile.

A single vec, do you by with combine_with_repo filtering based on ConfigOrigin?

SimonSapin updated this revision to Diff 25507.Feb 9 2021, 3:07 AM
Alphare added inline comments.Feb 9 2021, 3:41 AM
rust/hg-core/src/config/config.rs
29

Yes, that's how I would do it.

Alphare accepted this revision.Feb 9 2021, 3:42 AM

I saw that you updated the code, looks good to me!

pulkit accepted this revision.Feb 10 2021, 7:14 AM
This revision is now accepted and ready to land.Feb 10 2021, 7:14 AM
SimonSapin added a commit: rHGd7685105e504: rhg: Parse per-repository configuration.Feb 10 2021, 8:08 AM
Closed by commit rHGd7685105e504: rhg: Parse per-repository configuration (authored by SimonSapin). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 25494

rust/hg-core/src/config/config.rs

// config.rs // config.rs
// //
// Copyright 2020 // Copyright 2020
// Valentin Gatien-Baron, // Valentin Gatien-Baron,
// Raphaël Gomès <rgomes@octobus.net> // Raphaël Gomès <rgomes@octobus.net>
// //
// This software may be used and distributed according to the terms of the // This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version. // GNU General Public License version 2 or any later version.
use super::layer; use super::layer;
use crate::config::layer::{ use crate::config::layer::{
ConfigError, ConfigLayer, ConfigParseError, ConfigValue, ConfigError, ConfigLayer, ConfigParseError, ConfigValue,
}; };
use crate::utils::files::get_bytes_from_path; use crate::utils::files::get_bytes_from_path;
use std::env; use std::env;
use std::ops::RangeFrom;
use std::path::{Path, PathBuf}; use std::path::{Path, PathBuf};
use crate::errors::{HgResultExt, IoResultExt}; use crate::errors::{HgResultExt, IoResultExt};
use crate::repo::Repo;
/// Holds the config values for the current repository /// Holds the config values for the current repository
/// TODO update this docstring once we support more sources /// TODO update this docstring once we support more sources
pub struct Config { pub struct Config {
layers: Vec<layer::ConfigLayer>, layers: Vec<layer::ConfigLayer>,
/// Which layers if any are for `--config` command-line arguments.
/// This matters for pre-repository config which has less precedence than
/// CLI arguments but more than everything else.
cli_layers_indices: Option<RangeFrom<usize>>,
AlphareUnsubmitted
Not Done

I'm not a big fan of adding a cache that can become potentially fragile for what I'm assuming to be so little performance gained. Maybe iterating in reverse through the layers until you hit one that is not from the CLI would be good enough?

Alphare: I'm not a big fan of adding a cache that can become potentially fragile for what I'm assuming…
SimonSapinAuthorUnsubmitted
Done

I was not at all thinking of this as a cache. Do you mean we could filter based on ConfigOrigin? I had not considered that but yeah it’s probably less fragile. Or, what do you think of having three Vecs instead of one? System+user, repo, and CLI layers kept separately.

SimonSapin: I was not at all thinking of this as a cache. Do you mean we could filter based on…
AlphareUnsubmitted
Not Done

Currently, the ordering of layers gives us an easy answer to precedence that we lose if we split into multiple Vec. I say we keep it this way until we feel that this compromise is not worth it anymore and we need another abstraction layer.

Alphare: Currently, the ordering of layers gives us an easy answer to precedence that we lose if we…
SimonSapinAuthorUnsubmitted
Done

With three vecs the precedence logic would be centralized in a method that returns impl Iterator<Item=&ConfigLayer>.

Which do you mean by keep it this way? With RangeFrom<usize>?

SimonSapin: With three vecs the precedence logic would be centralized in a method that returns `impl…
AlphareUnsubmitted
Not Done

With three vecs the precedence logic would be centralized in a method that returns impl Iterator<Item=&ConfigLayer>.

Right, while it's not particularly harder, I didn't feel like it was needed when adding the config logic. If you think that it'll be frequent enough to query the separate "groups" of layers then by all means, go ahead. :)

Which do you mean by keep it this way? With RangeFrom<usize>?

I meant using a single Vec. I am still -1 on using RangeFrom<usize>.

Alphare: > With three vecs the precedence logic would be centralized in a method that returns impl…
SimonSapinAuthorUnsubmitted
Done

It’s not frequent, combine_with_repo is the only thing that needs handle these hypothetically-separate vecs separately. And the separation would be purely to make the code simpler and less fragile.

A single vec, do you by with combine_with_repo filtering based on ConfigOrigin?

SimonSapin: It’s not frequent, `combine_with_repo` is the only thing that needs handle these hypothetically…
AlphareUnsubmitted
Not Done

Yes, that's how I would do it.

Alphare: Yes, that's how I would do it.
} }
impl std::fmt::Debug for Config { impl std::fmt::Debug for Config {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
for (index, layer) in self.layers.iter().rev().enumerate() { for (index, layer) in self.layers.iter().rev().enumerate() {
write!( write!(
f, f,
"==== Layer {} (trusted: {}) ====\n{:?}", "==== Layer {} (trusted: {}) ====\n{:?}",
impl Config { impl Config {
/// Load system and user configuration from various files. /// Load system and user configuration from various files.
/// ///
/// This is also affected by some environment variables. /// This is also affected by some environment variables.
/// ///
/// TODO: add a parameter for `--config` CLI arguments /// TODO: add a parameter for `--config` CLI arguments
pub fn load() -> Result<Self, ConfigError> { pub fn load() -> Result<Self, ConfigError> {
let mut config = Self { layers: Vec::new() }; let mut config = Self {
layers: Vec::new(),
cli_layers_indices: None,
};
let opt_rc_path = env::var_os("HGRCPATH"); let opt_rc_path = env::var_os("HGRCPATH");
// HGRCPATH replaces system config // HGRCPATH replaces system config
if opt_rc_path.is_none() { if opt_rc_path.is_none() {
config.add_system_config()? config.add_system_config()?
} }
config.add_for_environment_variable("EDITOR", b"ui", b"editor"); config.add_for_environment_variable("EDITOR", b"ui", b"editor");
config.add_for_environment_variable("VISUAL", b"ui", b"editor"); config.add_for_environment_variable("VISUAL", b"ui", b"editor");
config.add_for_environment_variable("PAGER", b"pager", b"pager"); config.add_for_environment_variable("PAGER", b"pager", b"pager");
Err(_) => continue, // same as the python code Err(_) => continue, // same as the python code
Ok(data) => data, Ok(data) => data,
}; };
layers.extend(ConfigLayer::parse(&c, &data)?) layers.extend(ConfigLayer::parse(&c, &data)?)
} }
} }
} }
Ok(Config { layers }) Ok(Config {
layers,
cli_layers_indices: None,
})
} }
/// Loads the local config. In a future version, this will also load the /// Loads the per-repository config into a new `Config` which is combined
/// `$HOME/.hgrc` and more to mirror the Python implementation. /// with `self`.
pub fn load_for_repo(repo: &Repo) -> Result<Self, ConfigError> { pub(crate) fn combine_with_repo(
Ok(Self::load_from_explicit_sources(vec![ &self,
ConfigSource::AbsPath(repo.hg_vfs().join("hgrc")), repo_config_files: &[PathBuf],
])?) ) -> Result<Self, ConfigError> {
let cli_layers_start = match self.cli_layers_indices {
Some(RangeFrom { start }) => start,
None => self.layers.len(),
};
let (non_cli_layers, cli_layers) =
self.layers.split_at(cli_layers_start);
let mut repo_config = Self {
layers: non_cli_layers.iter().cloned().collect::<Vec<_>>(),
cli_layers_indices: None,
};
for path in repo_config_files {
// TODO: check if this file should be trusted:
// `mercurial/ui.py:427`
repo_config.add_trusted_file(path)?;
}
repo_config.cli_layers_indices = Some(repo_config.layers.len()..);
repo_config.layers.extend(cli_layers.iter().cloned());
Ok(repo_config)
} }
/// Returns an `Err` if the first value found is not a valid boolean. /// Returns an `Err` if the first value found is not a valid boolean.
/// Otherwise, returns an `Ok(option)`, where `option` is the boolean if /// Otherwise, returns an `Ok(option)`, where `option` is the boolean if
/// found, or `None`. /// found, or `None`.
pub fn get_option( pub fn get_option(
&self, &self,
section: &[u8], section: &[u8],

rust/hg-core/src/repo.rs

use crate::config::Config; use crate::config::{Config, ConfigError, ConfigParseError};
use crate::errors::{HgError, IoResultExt}; use crate::errors::{HgError, IoResultExt};
use crate::requirements; use crate::requirements;
use crate::utils::files::get_path_from_bytes; use crate::utils::files::get_path_from_bytes;
use memmap::{Mmap, MmapOptions}; use memmap::{Mmap, MmapOptions};
use std::collections::HashSet; use std::collections::HashSet;
use std::path::{Path, PathBuf}; use std::path::{Path, PathBuf};
/// A repository on disk /// A repository on disk
pub struct Repo { pub struct Repo {
working_directory: PathBuf, working_directory: PathBuf,
dot_hg: PathBuf, dot_hg: PathBuf,
store: PathBuf, store: PathBuf,
requirements: HashSet<String>, requirements: HashSet<String>,
config: Config,
} }
#[derive(Debug, derive_more::From)] #[derive(Debug, derive_more::From)]
pub enum RepoFindError { pub enum RepoError {
NotFoundInCurrentDirectoryOrAncestors { NotFound {
current_directory: PathBuf, current_directory: PathBuf,
}, },
#[from] #[from]
ConfigParseError(ConfigParseError),
#[from]
Other(HgError), Other(HgError),
} }
impl From<ConfigError> for RepoError {
fn from(error: ConfigError) -> Self {
match error {
ConfigError::Parse(error) => error.into(),
ConfigError::Other(error) => error.into(),
}
}
}
/// Filesystem access abstraction for the contents of a given "base" diretory /// Filesystem access abstraction for the contents of a given "base" diretory
#[derive(Clone, Copy)] #[derive(Clone, Copy)]
pub(crate) struct Vfs<'a> { pub(crate) struct Vfs<'a> {
base: &'a Path, base: &'a Path,
} }
impl Repo { impl Repo {
/// Search the current directory and its ancestores for a repository: /// Search the current directory and its ancestores for a repository:
/// a working directory that contains a `.hg` sub-directory. /// a working directory that contains a `.hg` sub-directory.
pub fn find(config: &Config) -> Result<Self, RepoFindError> { pub fn find(config: &Config) -> Result<Self, RepoError> {
let current_directory = crate::utils::current_dir()?; let current_directory = crate::utils::current_dir()?;
// ancestors() is inclusive: it first yields `current_directory` as-is. // ancestors() is inclusive: it first yields `current_directory` as-is.
for ancestor in current_directory.ancestors() { for ancestor in current_directory.ancestors() {
if ancestor.join(".hg").is_dir() { if ancestor.join(".hg").is_dir() {
return Ok(Self::new_at_path(ancestor.to_owned(), config)?); return Ok(Self::new_at_path(ancestor.to_owned(), config)?);
} }
} }
Err(RepoFindError::NotFoundInCurrentDirectoryOrAncestors { Err(RepoError::NotFound { current_directory })
current_directory,
})
} }
/// To be called after checking that `.hg` is a sub-directory /// To be called after checking that `.hg` is a sub-directory
fn new_at_path( fn new_at_path(
working_directory: PathBuf, working_directory: PathBuf,
config: &Config, config: &Config,
) -> Result<Self, HgError> { ) -> Result<Self, RepoError> {
let dot_hg = working_directory.join(".hg"); let dot_hg = working_directory.join(".hg");
let mut repo_config_files = Vec::new();
repo_config_files.push(dot_hg.join("hgrc"));
repo_config_files.push(dot_hg.join("hgrc-not-shared"));
let hg_vfs = Vfs { base: &dot_hg }; let hg_vfs = Vfs { base: &dot_hg };
let mut reqs = requirements::load_if_exists(hg_vfs)?; let mut reqs = requirements::load_if_exists(hg_vfs)?;
let relative = let relative =
reqs.contains(requirements::RELATIVE_SHARED_REQUIREMENT); reqs.contains(requirements::RELATIVE_SHARED_REQUIREMENT);
let shared = let shared =
reqs.contains(requirements::SHARED_REQUIREMENT) || relative; reqs.contains(requirements::SHARED_REQUIREMENT) || relative;
// From `mercurial/localrepo.py`: // From `mercurial/localrepo.py`:
let mut shared_path = get_path_from_bytes(&bytes).to_owned(); let mut shared_path = get_path_from_bytes(&bytes).to_owned();
if relative { if relative {
shared_path = dot_hg.join(shared_path) shared_path = dot_hg.join(shared_path)
} }
if !shared_path.is_dir() { if !shared_path.is_dir() {
return Err(HgError::corrupted(format!( return Err(HgError::corrupted(format!(
".hg/sharedpath points to nonexistent directory {}", ".hg/sharedpath points to nonexistent directory {}",
shared_path.display() shared_path.display()
))); ))
.into());
} }
store_path = shared_path.join("store"); store_path = shared_path.join("store");
let source_is_share_safe = let source_is_share_safe =
requirements::load(Vfs { base: &shared_path })? requirements::load(Vfs { base: &shared_path })?
.contains(requirements::SHARESAFE_REQUIREMENT); .contains(requirements::SHARESAFE_REQUIREMENT);
if share_safe && !source_is_share_safe { if share_safe && !source_is_share_safe {
return Err(match config.get(b"safe-mismatch", b"source-not-safe") { return Err(match config
.get(b"safe-mismatch", b"source-not-safe")
{
Some(b"abort") | None => HgError::abort( Some(b"abort") | None => HgError::abort(
"share source does not support share-safe requirement" "share source does not support share-safe requirement",
), ),
_ => HgError::unsupported("share-safe downgrade") _ => HgError::unsupported("share-safe downgrade"),
}); }
.into());
} else if source_is_share_safe && !share_safe { } else if source_is_share_safe && !share_safe {
return Err( return Err(
match config.get(b"safe-mismatch", b"source-safe") { match config.get(b"safe-mismatch", b"source-safe") {
Some(b"abort") | None => HgError::abort( Some(b"abort") | None => HgError::abort(
"version mismatch: source uses share-safe \ "version mismatch: source uses share-safe \
functionality while the current share does not", functionality while the current share does not",
), ),
_ => HgError::unsupported("share-safe upgrade"), _ => HgError::unsupported("share-safe upgrade"),
}, }
.into(),
); );
} }
if share_safe {
repo_config_files.insert(0, shared_path.join("hgrc"))
}
} }
let repo_config = config.combine_with_repo(&repo_config_files)?;
let repo = Self { let repo = Self {
requirements: reqs, requirements: reqs,
working_directory, working_directory,
store: store_path, store: store_path,
dot_hg, dot_hg,
config: repo_config,
}; };
requirements::check(&repo)?; requirements::check(&repo)?;
Ok(repo) Ok(repo)
} }
pub fn working_directory_path(&self) -> &Path { pub fn working_directory_path(&self) -> &Path {
&self.working_directory &self.working_directory
} }
pub fn requirements(&self) -> &HashSet<String> { pub fn requirements(&self) -> &HashSet<String> {
&self.requirements &self.requirements
} }
pub fn config(&self) -> &Config {
&self.config
}
/// For accessing repository files (in `.hg`), except for the store /// For accessing repository files (in `.hg`), except for the store
/// (`.hg/store`). /// (`.hg/store`).
pub(crate) fn hg_vfs(&self) -> Vfs<'_> { pub(crate) fn hg_vfs(&self) -> Vfs<'_> {
Vfs { base: &self.dot_hg } Vfs { base: &self.dot_hg }
} }
/// For accessing repository store files (in `.hg/store`) /// For accessing repository store files (in `.hg/store`)
pub(crate) fn store_vfs(&self) -> Vfs<'_> { pub(crate) fn store_vfs(&self) -> Vfs<'_> {

rust/rhg/src/error.rs

use crate::ui::utf8_to_local; use crate::ui::utf8_to_local;
use crate::ui::UiError; use crate::ui::UiError;
use format_bytes::format_bytes; use format_bytes::format_bytes;
use hg::config::{ConfigError, ConfigParseError}; use hg::config::{ConfigError, ConfigParseError};
use hg::errors::HgError; use hg::errors::HgError;
use hg::repo::RepoFindError; use hg::repo::RepoError;
use hg::revlog::revlog::RevlogError; use hg::revlog::revlog::RevlogError;
use hg::utils::files::get_bytes_from_path; use hg::utils::files::get_bytes_from_path;
use std::convert::From; use std::convert::From;
/// The kind of command error /// The kind of command error
#[derive(Debug)] #[derive(Debug)]
pub enum CommandError { pub enum CommandError {
/// Exit with an error message and "standard" failure exit code. /// Exit with an error message and "standard" failure exit code.
fn from(_error: UiError) -> Self { fn from(_error: UiError) -> Self {
// If we already failed writing to stdout or stderr, // If we already failed writing to stdout or stderr,
// writing an error message to stderr about it would be likely to fail // writing an error message to stderr about it would be likely to fail
// too. // too.
CommandError::abort("") CommandError::abort("")
} }
} }
impl From<RepoFindError> for CommandError { impl From<RepoError> for CommandError {
fn from(error: RepoFindError) -> Self { fn from(error: RepoError) -> Self {
match error { match error {
RepoFindError::NotFoundInCurrentDirectoryOrAncestors { RepoError::NotFound { current_directory } => CommandError::Abort {
current_directory,
} => CommandError::Abort {
message: format_bytes!( message: format_bytes!(
b"no repository found in '{}' (.hg not found)!", b"no repository found in '{}' (.hg not found)!",
get_bytes_from_path(current_directory) get_bytes_from_path(current_directory)
), ),
}, },
RepoFindError::Other(error) => error.into(), RepoError::ConfigParseError(error) => error.into(),
RepoError::Other(error) => error.into(),
} }
} }
} }
impl From<ConfigError> for CommandError { impl From<ConfigError> for CommandError {
fn from(error: ConfigError) -> Self { fn from(error: ConfigError) -> Self {
match error { match error {
ConfigError::Parse(ConfigParseError { ConfigError::Parse(error) => error.into(),
ConfigError::Other(error) => error.into(),
}
}
}
impl From<ConfigParseError> for CommandError {
fn from(error: ConfigParseError) -> Self {
let ConfigParseError {
origin, origin,
line, line,
bytes, bytes,
}) => { } = error;
let line_message = if let Some(line_number) = line { let line_message = if let Some(line_number) = line {
format_bytes!( format_bytes!(b" at line {}", line_number.to_string().into_bytes())
b" at line {}",
line_number.to_string().into_bytes()
)
} else { } else {
Vec::new() Vec::new()
}; };
CommandError::Abort { CommandError::Abort {
message: format_bytes!( message: format_bytes!(
b"config parse error in {}{}: '{}'", b"config parse error in {}{}: '{}'",
origin.to_bytes(), origin.to_bytes(),
line_message, line_message,
bytes bytes
), ),
} }
} }
ConfigError::Other(error) => error.into(),
}
}
} }
impl From<(RevlogError, &str)> for CommandError { impl From<(RevlogError, &str)> for CommandError {
fn from((err, rev): (RevlogError, &str)) -> CommandError { fn from((err, rev): (RevlogError, &str)) -> CommandError {
match err { match err {
RevlogError::InvalidRevision => CommandError::abort(format!( RevlogError::InvalidRevision => CommandError::abort(format!(
"invalid revision identifier {}", "invalid revision identifier {}",
rev rev