This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9121

hg-core: fix path encoding usage
ClosedPublic

Authored by acezar on Sep 29 2020, 11:58 AM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHG7252f5237352: hg-core: fix path encoding usage
rHGc02776792a40: hg-core: fix path encoding usage
Summary
  1. Hash encoded path are in .hg/store/dh instead of .hg/store/data.
  1. Path encoded index and data files may not have the same parent path. It is not just about replacing .i by .d

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

acezar created this revision.Sep 29 2020, 11:58 AM
acezar updated this revision to Diff 22923.Sep 29 2020, 1:31 PM
Alphare requested changes to this revision.Sep 30 2020, 4:07 AM
Alphare added a subscriber: Alphare.
Alphare added inline comments.
rust/hg-core/src/operations/cat.rs
162

I don't like the use of format! instead of get_path_from_bytes, no need for UTF8 lossy conversion, that just gives the wrong impression. Maybe that wouldn't work on some platforms, even.

This revision now requires changes to proceed.Sep 30 2020, 4:07 AM
acezar updated this revision to Diff 23267.Oct 20 2020, 6:06 AM
acezar marked an inline comment as done.Oct 20 2020, 6:07 AM
durin42 added a subscriber: durin42.Oct 28 2020, 9:56 PM

@Alphare are you happy with this now?

Alphare accepted this revision.Oct 29 2020, 4:27 AM

Yes, sorry I forgot about this even though I talked about it with Antoine.

Antoine cezar <acezar@chwitlabs.fr> added a commit: rHGc02776792a40: hg-core: fix path encoding usage.Oct 30 2020, 2:41 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHGc02776792a40: hg-core: fix path encoding usage (authored by Antoine cezar <acezar@chwitlabs.fr>). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 23383

rust/hg-core/src/operations/cat.rs

// list_tracked_files.rs // list_tracked_files.rs
// //
// Copyright 2020 Antoine Cezar <antoine.cezar@octobus.net> // Copyright 2020 Antoine Cezar <antoine.cezar@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 std::convert::From; use std::convert::From;
use std::path::PathBuf; use std::path::{Path, PathBuf};
use crate::revlog::changelog::Changelog; use crate::revlog::changelog::Changelog;
use crate::revlog::manifest::{Manifest, ManifestEntry}; use crate::revlog::manifest::{Manifest, ManifestEntry};
use crate::revlog::path_encode::path_encode; use crate::revlog::path_encode::path_encode;
use crate::revlog::revlog::Revlog; use crate::revlog::revlog::Revlog;
use crate::revlog::revlog::RevlogError; use crate::revlog::revlog::RevlogError;
use crate::revlog::Revision; use crate::revlog::Revision;
use crate::utils::hg_path::HgPathBuf; use crate::utils::files::get_path_from_bytes;
use crate::utils::hg_path::{HgPath, HgPathBuf};
const METADATA_DELIMITER: [u8; 2] = [b'\x01', b'\n']; const METADATA_DELIMITER: [u8; 2] = [b'\x01', b'\n'];
/// Kind of error encountered by `CatRev` /// Kind of error encountered by `CatRev`
#[derive(Debug)] #[derive(Debug)]
pub enum CatRevErrorKind { pub enum CatRevErrorKind {
/// Error when reading a `revlog` file. /// Error when reading a `revlog` file.
IoError(std::io::Error), IoError(std::io::Error),
if let Some(ref manifest_entry) = self.manifest_entry { if let Some(ref manifest_entry) = self.manifest_entry {
let mut bytes = vec![]; let mut bytes = vec![];
for (manifest_file, node_bytes) in for (manifest_file, node_bytes) in
manifest_entry.files_with_nodes() manifest_entry.files_with_nodes()
{ {
for cat_file in self.files.iter() { for cat_file in self.files.iter() {
if cat_file.as_bytes() == manifest_file.as_bytes() { if cat_file.as_bytes() == manifest_file.as_bytes() {
let encoded_bytes = let index_path =
path_encode(manifest_file.as_bytes()); store_path(self.root, manifest_file, b".i");
let revlog_index_string = format!( let data_path =
".hg/store/data/{}.i", store_path(self.root, manifest_file, b".d");
String::from_utf8_lossy(&encoded_bytes),
); let file_log =
let revlog_index_path = Revlog::open(&index_path, Some(&data_path))?;
self.root.join(&revlog_index_string);
let file_log = Revlog::open(&revlog_index_path)?;
let file_node = hex::decode(&node_bytes) let file_node = hex::decode(&node_bytes)
.map_err(|_| CatRevErrorKind::CorruptedRevlog)?; .map_err(|_| CatRevErrorKind::CorruptedRevlog)?;
let file_rev = file_log.get_node_rev(&file_node)?; let file_rev = file_log.get_node_rev(&file_node)?;
let data = file_log.get_rev_data(file_rev)?; let data = file_log.get_rev_data(file_rev)?;
if data.starts_with(&METADATA_DELIMITER) { if data.starts_with(&METADATA_DELIMITER) {
let end_delimiter_position = data let end_delimiter_position = data
[METADATA_DELIMITER.len()..] [METADATA_DELIMITER.len()..]
.windows(METADATA_DELIMITER.len()) .windows(METADATA_DELIMITER.len())
} }
Ok(bytes) Ok(bytes)
} else { } else {
unreachable!("manifest_entry should have been stored"); unreachable!("manifest_entry should have been stored");
} }
} }
} }
fn store_path(root: &Path, hg_path: &HgPath, suffix: &[u8]) -> PathBuf {
let encoded_bytes =
path_encode(&[b"data/", hg_path.as_bytes(), suffix].concat());
[
AlphareUnsubmitted
Done

I don't like the use of format! instead of get_path_from_bytes, no need for UTF8 lossy conversion, that just gives the wrong impression. Maybe that wouldn't work on some platforms, even.

Alphare: I don't like the use of `format!` instead of `get_path_from_bytes`, no need for UTF8 lossy…
root,
&Path::new(".hg/store/"),
get_path_from_bytes(&encoded_bytes),
]
.iter()
.collect()
}

rust/hg-core/src/operations/debugdata.rs

.parse::<Revision>() .parse::<Revision>()
.or(Err(DebugDataErrorKind::InvalidRevision))?; .or(Err(DebugDataErrorKind::InvalidRevision))?;
let root = find_root::FindRoot::new().run()?; let root = find_root::FindRoot::new().run()?;
let index_file = match self.kind { let index_file = match self.kind {
DebugDataKind::Changelog => root.join(".hg/store/00changelog.i"), DebugDataKind::Changelog => root.join(".hg/store/00changelog.i"),
DebugDataKind::Manifest => root.join(".hg/store/00manifest.i"), DebugDataKind::Manifest => root.join(".hg/store/00manifest.i"),
}; };
let revlog = Revlog::open(&index_file)?; let revlog = Revlog::open(&index_file, None)?;
let data = revlog.get_rev_data(rev)?; let data = revlog.get_rev_data(rev)?;
Ok(data) Ok(data)
} }
} }

rust/hg-core/src/revlog/changelog.rs

use crate::revlog::revlog::{Revlog, RevlogError}; use crate::revlog::revlog::{Revlog, RevlogError};
use crate::revlog::Revision; use crate::revlog::Revision;
use std::path::PathBuf; use std::path::PathBuf;
/// A specialized `Revlog` to work with `changelog` data format. /// A specialized `Revlog` to work with `changelog` data format.
pub struct Changelog { pub struct Changelog {
/// The generic `revlog` format. /// The generic `revlog` format.
revlog: Revlog, revlog: Revlog,
} }
impl Changelog { impl Changelog {
/// Open the `changelog` of a repository given by its root. /// Open the `changelog` of a repository given by its root.
pub fn open(root: &PathBuf) -> Result<Self, RevlogError> { pub fn open(root: &PathBuf) -> Result<Self, RevlogError> {
let index_file = root.join(".hg/store/00changelog.i"); let index_file = root.join(".hg/store/00changelog.i");
let revlog = Revlog::open(&index_file)?; let revlog = Revlog::open(&index_file, None)?;
Ok(Self { revlog }) Ok(Self { revlog })
} }
/// Return the `ChangelogEntry` a given node id. /// Return the `ChangelogEntry` a given node id.
pub fn get_node( pub fn get_node(
&self, &self,
node: &[u8], node: &[u8],
) -> Result<ChangelogEntry, RevlogError> { ) -> Result<ChangelogEntry, RevlogError> {

rust/hg-core/src/revlog/manifest.rs

use crate::revlog::revlog::{Revlog, RevlogError}; use crate::revlog::revlog::{Revlog, RevlogError};
use crate::revlog::Revision; use crate::revlog::Revision;
use crate::utils::hg_path::HgPath; use crate::utils::hg_path::HgPath;
use std::path::PathBuf; use std::path::PathBuf;
/// A specialized `Revlog` to work with `manifest` data format. /// A specialized `Revlog` to work with `manifest` data format.
pub struct Manifest { pub struct Manifest {
/// The generic `revlog` format. /// The generic `revlog` format.
revlog: Revlog, revlog: Revlog,
} }
impl Manifest { impl Manifest {
/// Open the `manifest` of a repository given by its root. /// Open the `manifest` of a repository given by its root.
pub fn open(root: &PathBuf) -> Result<Self, RevlogError> { pub fn open(root: &PathBuf) -> Result<Self, RevlogError> {
let index_file = root.join(".hg/store/00manifest.i"); let index_file = root.join(".hg/store/00manifest.i");
let revlog = Revlog::open(&index_file)?; let revlog = Revlog::open(&index_file, None)?;
Ok(Self { revlog }) Ok(Self { revlog })
} }
/// Return the `ManifestEntry` of a given node id. /// Return the `ManifestEntry` of a given node id.
pub fn get_node(&self, node: &[u8]) -> Result<ManifestEntry, RevlogError> { pub fn get_node(&self, node: &[u8]) -> Result<ManifestEntry, RevlogError> {
let rev = self.revlog.get_node_rev(node)?; let rev = self.revlog.get_node_rev(node)?;
self.get_rev(rev) self.get_rev(rev)
} }

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

} }
impl Revlog { impl Revlog {
/// Open a revlog index file. /// Open a revlog index file.
/// ///
/// It will also open the associated data file if index and data are not /// It will also open the associated data file if index and data are not
/// interleaved. /// interleaved.
#[timed] #[timed]
pub fn open(index_path: &Path) -> Result<Self, RevlogError> { pub fn open(
index_path: &Path,
data_path: Option<&Path>,
) -> Result<Self, RevlogError> {
let index_mmap = let index_mmap =
mmap_open(&index_path).map_err(RevlogError::IoError)?; mmap_open(&index_path).map_err(RevlogError::IoError)?;
let version = get_version(&index_mmap); let version = get_version(&index_mmap);
if version != 1 { if version != 1 {
return Err(RevlogError::UnsuportedVersion(version)); return Err(RevlogError::UnsuportedVersion(version));
} }
let index = Index::new(Box::new(index_mmap))?; let index = Index::new(Box::new(index_mmap))?;
// TODO load data only when needed // let default_data_path = index_path.with_extension("d");
// type annotation required // type annotation required
// won't recognize Mmap as Deref<Target = [u8]> // won't recognize Mmap as Deref<Target = [u8]>
let data_bytes: Option<Box<dyn Deref<Target = [u8]> + Send>> = let data_bytes: Option<Box<dyn Deref<Target = [u8]> + Send>> =
if index.is_inline() { if index.is_inline() {
None None
} else { } else {
let data_path = index_path.with_extension("d"); let data_path = data_path.unwrap_or(&default_data_path);
let data_mmap = let data_mmap =
mmap_open(&data_path).map_err(RevlogError::IoError)?; mmap_open(data_path).map_err(RevlogError::IoError)?;
Some(Box::new(data_mmap)) Some(Box::new(data_mmap))
}; };
Ok(Revlog { index, data_bytes }) Ok(Revlog { index, data_bytes })
} }
/// Return number of entries of the `Revlog`. /// Return number of entries of the `Revlog`.
pub fn len(&self) -> usize { pub fn len(&self) -> usize {