This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9960

rust: Remove unnecessary check for absolute path before joining
ClosedPublic

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

Details

Reviewers
Alphare
pulkit
Group Reviewers
hg-reviewers
Commits
rHG39128182f04e: rust: Remove unnecessary check for absolute path before joining
Summary

Path::join does the right thing if its argument is absolute.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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

I think we should always try to use expect instead of unwrap unless it's *really* repetitive or obvious, just as a best practice.

SimonSapin updated this revision to Diff 25497.Feb 8 2021, 2:00 PM
Alphare accepted this revision.Feb 9 2021, 3:33 AM
pulkit accepted this revision.Feb 10 2021, 7:05 AM
This revision is now accepted and ready to land.Feb 10 2021, 7:05 AM

Revision Contents
Changeset List

PathPackages
Mrust/hg-core/src/config/layer.rs (30 lines)

Diff 25527

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

// layer.rs // layer.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 crate::errors::{HgError, IoResultExt}; use crate::errors::{HgError, IoResultExt};
use crate::utils::files::{get_bytes_from_path, get_path_from_bytes}; use crate::utils::files::{get_bytes_from_path, get_path_from_bytes};
use format_bytes::format_bytes; use format_bytes::format_bytes;
use lazy_static::lazy_static; use lazy_static::lazy_static;
use regex::bytes::Regex; use regex::bytes::Regex;
use std::collections::HashMap; use std::collections::HashMap;
use std::io;
use std::path::{Path, PathBuf}; use std::path::{Path, PathBuf};
lazy_static! { lazy_static! {
static ref SECTION_RE: Regex = make_regex(r"^\[([^\[]+)\]"); static ref SECTION_RE: Regex = make_regex(r"^\[([^\[]+)\]");
static ref ITEM_RE: Regex = make_regex(r"^([^=\s][^=]*?)\s*=\s*((.*\S)?)"); static ref ITEM_RE: Regex = make_regex(r"^([^=\s][^=]*?)\s*=\s*((.*\S)?)");
/// Continuation whitespace /// Continuation whitespace
static ref CONT_RE: Regex = make_regex(r"^\s+(\S|\S.*\S)\s*$"); static ref CONT_RE: Regex = make_regex(r"^\s+(\S|\S.*\S)\s*$");
static ref EMPTY_RE: Regex = make_regex(r"^(;|#|\s*$)"); static ref EMPTY_RE: Regex = make_regex(r"^(;|#|\s*$)");
let mut lines_iter = let mut lines_iter =
data.split(|b| *b == b'\n').enumerate().peekable(); data.split(|b| *b == b'\n').enumerate().peekable();
let mut section = b"".to_vec(); let mut section = b"".to_vec();
while let Some((index, bytes)) = lines_iter.next() { while let Some((index, bytes)) = lines_iter.next() {
if let Some(m) = INCLUDE_RE.captures(&bytes) { if let Some(m) = INCLUDE_RE.captures(&bytes) {
let filename_bytes = &m[1]; let filename_bytes = &m[1];
let filename_to_include = get_path_from_bytes(&filename_bytes); // `Path::parent` only fails for the root directory,
let (include_src, result) = // which `src` can’t be since we’ve managed to open it as a file.
AlphareUnsubmitted
Not Done

I think we should always try to use expect instead of unwrap unless it's *really* repetitive or obvious, just as a best practice.

Alphare: I think we should always try to use `expect` instead of `unwrap` unless it's *really*…
read_include(&src, &filename_to_include); let dir = src
let data = result.for_file(filename_to_include)?; .parent()
.expect("Path::parent fail on a file we’ve read");
// `Path::join` with an absolute argument correctly ignores the base path
let filename = dir.join(&get_path_from_bytes(&filename_bytes));
let data = std::fs::read(&filename).for_file(&filename)?;
layers.push(current_layer); layers.push(current_layer);
layers.extend(Self::parse(&include_src, &data)?); layers.extend(Self::parse(&filename, &data)?);
current_layer = Self::new(ConfigOrigin::File(src.to_owned())); current_layer = Self::new(ConfigOrigin::File(src.to_owned()));
} else if let Some(_) = EMPTY_RE.captures(&bytes) { } else if let Some(_) = EMPTY_RE.captures(&bytes) {
} else if let Some(m) = SECTION_RE.captures(&bytes) { } else if let Some(m) = SECTION_RE.captures(&bytes) {
section = m[1].to_vec(); section = m[1].to_vec();
} else if let Some(m) = ITEM_RE.captures(&bytes) { } else if let Some(m) = ITEM_RE.captures(&bytes) {
let item = m[1].to_vec(); let item = m[1].to_vec();
let mut value = m[2].to_vec(); let mut value = m[2].to_vec();
loop { loop {
pub enum ConfigError { pub enum ConfigError {
Parse(ConfigParseError), Parse(ConfigParseError),
Other(HgError), Other(HgError),
} }
fn make_regex(pattern: &'static str) -> Regex { fn make_regex(pattern: &'static str) -> Regex {
Regex::new(pattern).expect("expected a valid regex") Regex::new(pattern).expect("expected a valid regex")
} }
/// Includes are relative to the file they're defined in, unless they're
/// absolute.
fn read_include(
old_src: &Path,
new_src: &Path,
) -> (PathBuf, io::Result<Vec<u8>>) {
if new_src.is_absolute() {
(new_src.to_path_buf(), std::fs::read(&new_src))
} else {
let dir = old_src.parent().unwrap();
let new_src = dir.join(&new_src);
(new_src.to_owned(), std::fs::read(&new_src))
}
}