This is an archive of the discontinued Mercurial Phabricator instance.

Differential D12546

rust-revlog: move check for nodemap requirement to caller
Needs ReviewPublic

Authored by martinvonz on Wed, Apr 13, 9:10 AM.

Download Raw Diff

Details

Reviewers

Alphare

Group Reviewers

hg-reviewers

Summary

It's good for both making Revlog testable and reusable to have it
not depend on the higher-level Repo type. This patch is one step in
towards that. Additionally, this change in particular gives the
callers more control over when to use a nodemap.

Diff Detail

Repository

rHG Mercurial

Branch

default

Lint

No Linters Available

Unit

No Unit Test Coverage

Event Timeline

martinvonz created this revision.Wed, Apr 13, 9:10 AM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptWed, Apr 13, 9:10 AM

Herald added a subscriber: mercurial-patches. · View Herald Transcript

martinvonz added a child revision: D12547: rust-revlog: make unaware of `Repo`.Wed, Apr 13, 9:10 AM

While I think you're right about separating the revlogs from the repo, the repeated logic should probably be factored into a function somewhere. Why not in nodemap.rs?

This revision now requires changes to proceed.Thu, Apr 14, 5:03 AM

In D12546#192085, @Alphare wrote:

While I think you're right about separating the revlogs from the repo, the repeated logic should probably be factored into a function somewhere. Why not in nodemap.rs?

Two things:

Do we want to use persistent nodemaps for all types of revlogs? This was what I meant by "Additionally, this change in particular gives the callers more control over when to use a nodemap.". Looking at the Python code, we don't seem to use it for filelogs. I'll add a patch to this series to stop using persistent nodemap for filelogs.
I was thinking of moving the conditions further up the stack, to the repo level. If we do that, then the repo might be a natural place to extract a function. Or maybe I'll put it in nodemap.rs as you said. I'll update this patch a bit later.

martinvonz added a child revision: D12558: rust-filelog: don't use persistent nodemap (to match Python).Thu, Apr 14, 12:13 PM

martinvonz removed a child revision: D12547: rust-revlog: make unaware of `Repo`.

martinvonz updated this revision to Diff 33176.

In D12546#192112, @martinvonz wrote:

Two things:

Do we want to use persistent nodemaps for all types of revlogs? This was what I meant by "Additionally, this change in particular gives the callers more control over when to use a nodemap.". Looking at the Python code, we don't seem to use it for filelogs. I'll add a patch to this series to stop using persistent nodemap for filelogs.

We will probably only want it for changelogs and manifestlogs. Thanks for sending the patch for the filelogs. Maybe that'll change in the future (with unified revlog?), but that can always be added.

I was thinking of moving the conditions further up the stack, to the repo level. If we do that, then the repo might be a natural place to extract a function. Or maybe I'll put it in nodemap.rs as you said. I'll update this patch a bit later.

Either seem fine to me.

In D12546#192136, @Alphare wrote:

In D12546#192112, @martinvonz wrote:

I was thinking of moving the conditions further up the stack, to the repo level. If we do that, then the repo might be a natural place to extract a function. Or maybe I'll put it in nodemap.rs as you said. I'll update this patch a bit later.

Either seem fine to me.

I moved it to Repo in D12562.

Revision Contents
Changeset List

			Path	Packages
M			rust/hg-core/src/operations/debugdata.rs (6 lines)
M			rust/hg-core/src/revlog/changelog.rs (7 lines)
M			rust/hg-core/src/revlog/filelog.rs (7 lines)
M			rust/hg-core/src/revlog/manifest.rs (6 lines)
M			rust/hg-core/src/revlog/revlog.rs (9 lines)

Commit	Parents	Author	Summary	Date
695404f6755b	ea65c742e121	Martin von Zweigbergk		Tue, Apr 12, 2:40 PM

Status	Author	Revision
Needs Review	martinvonz	D12494 [RFC] rust-revlog: create nodemap on the fly if it's not persisted
Needs Review	martinvonz	D12424 rust: upgrade zstd from 0.5.3 to 0.11.1
Accepted	martinvonz	D12548 changelog: avoid copying changeset data into `ChangesetRevisionData`
Accepted	martinvonz	D12486 rust-nodemap: remove unnecessary explicit lifetime
Needs Review	martinvonz	D12562 rust-repo: extract a function for checking nodemap requirement
Needs Review	martinvonz	D12561 rust-revlog: make `Changelog` and `ManifestLog` unaware of `Repo`
Closed	martinvonz	D12442 rust-revlog: add methods for getting parent revs and entries
Accepted	martinvonz	D12547 rust-revlog: make unaware of `Repo`
Accepted	martinvonz	D12558 rust-filelog: don't use persistent nodemap (to match Python)
Needs Review	martinvonz	D12546 rust-revlog: move check for nodemap requirement to caller
Accepted	martinvonz	D12545 rust-nodemap-docket: make unaware of `Repo`
Accepted	martinvonz	D12544 rust-nodemap-docket: move check of nodemap requirement to caller
Closed	martinvonz	D12439 rust-changelog: start parsing changeset data
Closed	martinvonz	D12438 rust-changelog: remove special parsing of empty changelog data for null rev
Closed	martinvonz	D12426 rust-changelog: don't skip empty lines when iterating over changeset lines
Closed	martinvonz	D12425 rust-requirements: allow loading repos with `bookmarksinstore` requirement

Diff 33176

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

	// debugdata.rs			// debugdata.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 crate::repo::Repo;			use crate::repo::Repo;
				use crate::requirements;
	use crate::revlog::revlog::{Revlog, RevlogError};			use crate::revlog::revlog::{Revlog, RevlogError};

	/// Kind of data to debug			/// Kind of data to debug
	#[derive(Debug, Copy, Clone)]			#[derive(Debug, Copy, Clone)]
	pub enum DebugDataKind {			pub enum DebugDataKind {
	Changelog,			Changelog,
	Manifest,			Manifest,
	}			}

	/// Dump the contents data of a revision.			/// Dump the contents data of a revision.
	pub fn debug_data(			pub fn debug_data(
	repo: &Repo,			repo: &Repo,
	revset: &str,			revset: &str,
	kind: DebugDataKind,			kind: DebugDataKind,
	) -> Result<Vec<u8>, RevlogError> {			) -> Result<Vec<u8>, RevlogError> {
	let index_file = match kind {			let index_file = match kind {
	DebugDataKind::Changelog => "00changelog.i",			DebugDataKind::Changelog => "00changelog.i",
	DebugDataKind::Manifest => "00manifest.i",			DebugDataKind::Manifest => "00manifest.i",
	};			};
	let revlog = Revlog::open(repo, index_file, None)?;			let use_nodemap = repo
				.requirements()
				.contains(requirements::NODEMAP_REQUIREMENT);
				let revlog = Revlog::open(repo, index_file, None, use_nodemap)?;
	let rev =			let rev =
	crate::revset::resolve_rev_number_or_hex_prefix(revset, &revlog)?;			crate::revset::resolve_rev_number_or_hex_prefix(revset, &revlog)?;
	let data = revlog.get_rev_data(rev)?;			let data = revlog.get_rev_data(rev)?;
	Ok(data.into_owned())			Ok(data.into_owned())
	}			}

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

	use crate::errors::HgError;			use crate::errors::HgError;
	use crate::repo::Repo;			use crate::repo::Repo;
				use crate::requirements;
	use crate::revlog::revlog::{Revlog, RevlogEntry, RevlogError};			use crate::revlog::revlog::{Revlog, RevlogEntry, RevlogError};
	use crate::revlog::Revision;			use crate::revlog::Revision;
	use crate::revlog::{Node, NodePrefix};			use crate::revlog::{Node, NodePrefix};
	use crate::utils::hg_path::HgPath;			use crate::utils::hg_path::HgPath;
	use itertools::Itertools;			use itertools::Itertools;
	use std::ascii::escape_default;			use std::ascii::escape_default;
	use std::fmt::{Debug, Formatter};			use std::fmt::{Debug, Formatter};

	/// 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.
	pub(crate) revlog: Revlog,			pub(crate) 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(repo: &Repo) -> Result<Self, HgError> {			pub fn open(repo: &Repo) -> Result<Self, HgError> {
	let revlog = Revlog::open(repo, "00changelog.i", None)?;			let use_nodemap = repo
				.requirements()
				.contains(requirements::NODEMAP_REQUIREMENT);
				let revlog = Revlog::open(repo, "00changelog.i", None, use_nodemap)?;
	Ok(Self { revlog })			Ok(Self { revlog })
	}			}

	/// Return the `ChangelogEntry` for the given node ID.			/// Return the `ChangelogEntry` for the given node ID.
	pub fn data_for_node(			pub fn data_for_node(
	&self,			&self,
	node: NodePrefix,			node: NodePrefix,
	) -> Result<ChangelogRevisionData, RevlogError> {			) -> Result<ChangelogRevisionData, RevlogError> {
	&bytes.iter().flat_map(\|b\| escape_default(*b)).collect_vec(),			&bytes.iter().flat_map(\|b\| escape_default(*b)).collect_vec(),
	)			)
	.to_string()			.to_string()
	}			}

	#[cfg(test)]			#[cfg(test)]
	mod tests {			mod tests {
	use super::*;			use super::*;
	use itertools::Itertools;
	use pretty_assertions::assert_eq;			use pretty_assertions::assert_eq;

	#[test]			#[test]
	fn test_create_changelogrevisiondata_invalid() {			fn test_create_changelogrevisiondata_invalid() {
	// Completely empty			// Completely empty
	assert!(ChangelogRevisionData::new(b"abcd".to_vec()).is_err());			assert!(ChangelogRevisionData::new(b"abcd".to_vec()).is_err());
	// No newline after manifest			// No newline after manifest
	assert!(ChangelogRevisionData::new(b"abcd".to_vec()).is_err());			assert!(ChangelogRevisionData::new(b"abcd".to_vec()).is_err());

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

	use crate::errors::HgError;			use crate::errors::HgError;
	use crate::repo::Repo;			use crate::repo::Repo;
				use crate::requirements;
	use crate::revlog::path_encode::path_encode;			use crate::revlog::path_encode::path_encode;
	use crate::revlog::revlog::RevlogEntry;			use crate::revlog::revlog::RevlogEntry;
	use crate::revlog::revlog::{Revlog, RevlogError};			use crate::revlog::revlog::{Revlog, RevlogError};
	use crate::revlog::NodePrefix;			use crate::revlog::NodePrefix;
	use crate::revlog::Revision;			use crate::revlog::Revision;
	use crate::utils::files::get_path_from_bytes;			use crate::utils::files::get_path_from_bytes;
	use crate::utils::hg_path::HgPath;			use crate::utils::hg_path::HgPath;
	use crate::utils::SliceExt;			use crate::utils::SliceExt;
	use std::path::PathBuf;			use std::path::PathBuf;

	/// A specialized `Revlog` to work with file data logs.			/// A specialized `Revlog` to work with file data logs.
	pub struct Filelog {			pub struct Filelog {
	/// The generic `revlog` format.			/// The generic `revlog` format.
	revlog: Revlog,			revlog: Revlog,
	}			}

	impl Filelog {			impl Filelog {
	pub fn open(repo: &Repo, file_path: &HgPath) -> Result<Self, HgError> {			pub fn open(repo: &Repo, file_path: &HgPath) -> Result<Self, HgError> {
	let index_path = store_path(file_path, b".i");			let index_path = store_path(file_path, b".i");
	let data_path = store_path(file_path, b".d");			let data_path = store_path(file_path, b".d");
	let revlog = Revlog::open(repo, index_path, Some(&data_path))?;			let use_nodemap = repo
				.requirements()
				.contains(requirements::NODEMAP_REQUIREMENT);
				let revlog =
				Revlog::open(repo, index_path, Some(&data_path), use_nodemap)?;
	Ok(Self { revlog })			Ok(Self { revlog })
	}			}

	/// The given node ID is that of the file as found in a filelog, not of a			/// The given node ID is that of the file as found in a filelog, not of a
	/// changeset.			/// changeset.
	pub fn data_for_node(			pub fn data_for_node(
	&self,			&self,
	file_node: impl Into<NodePrefix>,			file_node: impl Into<NodePrefix>,

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

	use crate::errors::HgError;			use crate::errors::HgError;
	use crate::repo::Repo;			use crate::repo::Repo;
				use crate::requirements;
	use crate::revlog::revlog::{Revlog, RevlogError};			use crate::revlog::revlog::{Revlog, RevlogError};
	use crate::revlog::Revision;			use crate::revlog::Revision;
	use crate::revlog::{Node, NodePrefix};			use crate::revlog::{Node, NodePrefix};
	use crate::utils::hg_path::HgPath;			use crate::utils::hg_path::HgPath;
	use crate::utils::SliceExt;			use crate::utils::SliceExt;

	/// A specialized `Revlog` to work with `manifest` data format.			/// A specialized `Revlog` to work with `manifest` data format.
	pub struct Manifestlog {			pub struct Manifestlog {
	/// The generic `revlog` format.			/// The generic `revlog` format.
	revlog: Revlog,			revlog: Revlog,
	}			}

	impl Manifestlog {			impl Manifestlog {
	/// Open the `manifest` of a repository given by its root.			/// Open the `manifest` of a repository given by its root.
	pub fn open(repo: &Repo) -> Result<Self, HgError> {			pub fn open(repo: &Repo) -> Result<Self, HgError> {
	let revlog = Revlog::open(repo, "00manifest.i", None)?;			let use_nodemap = repo
				.requirements()
				.contains(requirements::NODEMAP_REQUIREMENT);
				let revlog = Revlog::open(repo, "00manifest.i", None, use_nodemap)?;
	Ok(Self { revlog })			Ok(Self { revlog })
	}			}

	/// Return the `Manifest` for the given node ID.			/// Return the `Manifest` for the given node ID.
	///			///
	/// Note: this is a node ID in the manifestlog, typically found through			/// Note: this is a node ID in the manifestlog, typically found through
	/// `ChangelogEntry::manifest_node`. It is not the node ID of any			/// `ChangelogEntry::manifest_node`. It is not the node ID of any
	/// changeset.			/// changeset.

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

	use super::node::{NodePrefix, NODE_BYTES_LENGTH, NULL_NODE};			use super::node::{NodePrefix, NODE_BYTES_LENGTH, NULL_NODE};
	use super::nodemap;			use super::nodemap;
	use super::nodemap::{NodeMap, NodeMapError};			use super::nodemap::{NodeMap, NodeMapError};
	use super::nodemap_docket::NodeMapDocket;			use super::nodemap_docket::NodeMapDocket;
	use super::patch;			use super::patch;
	use crate::errors::HgError;			use crate::errors::HgError;
	use crate::repo::Repo;			use crate::repo::Repo;
	use crate::revlog::Revision;			use crate::revlog::Revision;
	use crate::{requirements, Node, NULL_REVISION};			use crate::{Node, NULL_REVISION};

	const REVISION_FLAG_CENSORED: u16 = 1 << 15;			const REVISION_FLAG_CENSORED: u16 = 1 << 15;
	const REVISION_FLAG_ELLIPSIS: u16 = 1 << 14;			const REVISION_FLAG_ELLIPSIS: u16 = 1 << 14;
	const REVISION_FLAG_EXTSTORED: u16 = 1 << 13;			const REVISION_FLAG_EXTSTORED: u16 = 1 << 13;
	const REVISION_FLAG_HASCOPIESINFO: u16 = 1 << 12;			const REVISION_FLAG_HASCOPIESINFO: u16 = 1 << 12;

	// Keep this in sync with REVIDX_KNOWN_FLAGS in			// Keep this in sync with REVIDX_KNOWN_FLAGS in
	// mercurial/revlogutils/flagutil.py			// mercurial/revlogutils/flagutil.py
	///			///
	/// 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(			pub fn open(
	repo: &Repo,			repo: &Repo,
	index_path: impl AsRef<Path>,			index_path: impl AsRef<Path>,
	data_path: Option<&Path>,			data_path: Option<&Path>,
				use_nodemap: bool,
	) -> Result<Self, HgError> {			) -> Result<Self, HgError> {
	let index_path = index_path.as_ref();			let index_path = index_path.as_ref();
	let index = {			let index = {
	match repo.store_vfs().mmap_open_opt(&index_path)? {			match repo.store_vfs().mmap_open_opt(&index_path)? {
	None => Index::new(Box::new(vec![])),			None => Index::new(Box::new(vec![])),
	Some(index_mmap) => {			Some(index_mmap) => {
	let index = Index::new(Box::new(index_mmap))?;			let index = Index::new(Box::new(index_mmap))?;
	Ok(index)			Ok(index)
	} else {			} else {
	let data_path = data_path.unwrap_or(&default_data_path);			let data_path = data_path.unwrap_or(&default_data_path);
	let data_mmap = repo.store_vfs().mmap_open(data_path)?;			let data_mmap = repo.store_vfs().mmap_open(data_path)?;
	Some(Box::new(data_mmap))			Some(Box::new(data_mmap))
	};			};

	let nodemap = if index.is_inline() {			let nodemap = if index.is_inline() {
	None			None
	} else if !repo			} else if !use_nodemap {
	.requirements()
	.contains(requirements::NODEMAP_REQUIREMENT)
	{
	// If .hg/requires does not opt it, don’t try to open a nodemap
	None			None
	} else {			} else {
	NodeMapDocket::read_from_file(&repo.store_vfs(), index_path)?.map(			NodeMapDocket::read_from_file(&repo.store_vfs(), index_path)?.map(
	\|(docket, data)\| {			\|(docket, data)\| {
	nodemap::NodeTree::load_bytes(			nodemap::NodeTree::load_bytes(
	Box::new(data),			Box::new(data),
	docket.data_length,			docket.data_length,
	)			)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	33154		Wed, Apr 13, 9:10 AM	★	★
Diff 2	33176		Thu, Apr 14, 12:13 PM	★	★