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
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

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 25497

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))
}
}