This is an archive of the discontinued Mercurial Phabricator instance.

Differential D10010

rust: Add a log file rotation utility
ClosedPublic

Authored by SimonSapin on Feb 17 2021, 7:58 AM.

Download Raw Diff

Details

Reviewers

Alphare

Group Reviewers

hg-reviewers

Commits

rHG1f55cd5b292f: rust: Add a log file rotation utility
rHG289cbad841bc: rust: Add a log file rotation utility

Summary

This is ported to Rust from mercurial/loggingutil.py.

The "builder" pattern is used to make it visible at call sites what the two
numeric parameters mean. In Python they might simply by keyword arguments.

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 17 2021, 7:58 AM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptFeb 17 2021, 7:58 AM

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

SimonSapin added a child revision: D10011: blackbox: Remove misleading quotes in config example.Feb 17 2021, 7:58 AM

Alphare added a subscriber: Alphare.Feb 18 2021, 4:52 AM

Alphare added inline comments.

rust/hg-core/src/logging.rs
60	Should be `>=`, even though it's not really important.
61	Shouldn't it be `(1..=self.max_files)`?

SimonSapin marked 2 inline comments as done.Feb 19 2021, 3:18 AM

SimonSapin added inline comments.

rust/hg-core/src/logging.rs
60	Done.
61	It could be if we renamed `{name}.{i - 1}` to `{name}.{i}` instead of `{name}.{i}` to `{name}.{i + 1}`.

SimonSapin marked 2 inline comments as done.Feb 19 2021, 3:31 AM

SimonSapin updated this revision to Diff 25660.

Alphare accepted this revision.Feb 19 2021, 4:01 AM

Alphare added inline comments.

rust/hg-core/src/logging.rs
61	Right, doesn't really matter. I wasn't sure of the behavior of the range operator when doing n..n.

/!\ CI failed for this change after you last updated.

As a note to Simon: you should rely on Baymax to re-send your patches after the initial phabsend. Hopefully we can keep the CI happy and that works well. :)

This revision now requires changes to proceed.Feb 19 2021, 5:41 AM

Alright, let’s try this: https://foss.heptapod.net/octobus/mercurial-devel/-/pipelines/18143

What’s a good workflow when I also have additional patches to add on top of the stack?

In D10010#151796, @SimonSapin wrote:

Alright, let’s try this: https://foss.heptapod.net/octobus/mercurial-devel/-/pipelines/18143
What’s a good workflow when I also have additional patches to add on top of the stack?

Wait for the CI to finish, and phabsend your stack. No-ops will not trigger notifications. In an ideal world: don't use Phabricator... but that's still a ways out. ;)

This revision now requires review to proceed.Feb 19 2021, 6:10 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

Alphare accepted this revision.Feb 19 2021, 7:56 AM

SimonSapin added a commit: rHG289cbad841bc: rust: Add a log file rotation utility.Feb 24 2021, 11:10 AM

This revision was not accepted when it landed; it landed in state Needs Review.

Closed by commit rHG289cbad841bc: rust: Add a log file rotation utility (authored by SimonSapin). · Explain Why

This revision was automatically updated to reflect the committed changes.

SimonSapin added a commit: rHG1f55cd5b292f: rust: Add a log file rotation utility.Feb 24 2021, 12:11 PM

Revision Contents
Changeset List

		Path
M		rust/hg-core/src/config/config.rs (9 lines)
M		rust/hg-core/src/config/layer.rs (3 lines)
M		rust/hg-core/src/errors.rs (57 lines)
M		rust/hg-core/src/lib.rs (1 line)
A	M	rust/hg-core/src/logging.rs (101 lines)
M		rust/hg-core/src/repo.rs (36 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	25647		Feb 17 2021, 7:58 AM	★	★
Diff 2	25660		Feb 19 2021, 3:30 AM	★	★
Diff 3	25678		Feb 19 2021, 6:29 AM	★	★
Diff 4	25877	rHG289cbad841bcf9bf29aa5503068ca4bd03696705	Feb 11 2021, 9:51 AM	★	★

Status	Author	Revision
Closed	SimonSapin	D10022 rust: Add some unit tests for parse_byte_size in config
Closed	SimonSapin	D10021 rust: Move config value parsing functions to a new module
Closed	SimonSapin	D10012 rhg: Add support for the blackbox extension
Closed	SimonSapin	D10011 blackbox: Remove misleading quotes in config example
Closed	SimonSapin	D10010 rust: Add a log file rotation utility
Closed	SimonSapin	D10009 rust: Add a `ConfigValueParseError` variant to common errors
Closed	SimonSapin	D10008 rust: Add config parsing support for more value types
Closed	SimonSapin	D10007 rust: Introduce a get_bytes_from_os_str utility function
Closed	SimonSapin	D10006 rust: Make `DirstateParents`’s fields typed `Node`s
Closed	SimonSapin	D10005 rust: Rewrite dirstate parsing usin the `bytes-cast` crate
Closed	SimonSapin	D10004 rhg: Move `Repo` object creation into `main()`
Closed	SimonSapin	D10003 rhg: Group values passed to every sub-command into a struct
Closed	SimonSapin	D10002 rhg: Remove error message on unsupported CLI arguments

Diff 25877

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

	if let Some(layer) = ConfigLayer::parse_cli_args(cli_config_args)? {			if let Some(layer) = ConfigLayer::parse_cli_args(cli_config_args)? {
	config.layers.push(layer)			config.layers.push(layer)
	}			}
	Ok(config)			Ok(config)
	}			}

	fn add_trusted_dir(&mut self, path: &Path) -> Result<(), ConfigError> {			fn add_trusted_dir(&mut self, path: &Path) -> Result<(), ConfigError> {
	if let Some(entries) = std::fs::read_dir(path)			if let Some(entries) = std::fs::read_dir(path)
	.for_file(path)			.when_reading_file(path)
	.io_not_found_as_none()?			.io_not_found_as_none()?
	{			{
	for entry in entries {			for entry in entries {
	let file_path = entry.for_file(path)?.path();			let file_path = entry.when_reading_file(path)?.path();
	if file_path.extension() == Some(std::ffi::OsStr::new("rc")) {			if file_path.extension() == Some(std::ffi::OsStr::new("rc")) {
	self.add_trusted_file(&file_path)?			self.add_trusted_file(&file_path)?
	}			}
	}			}
	}			}
	Ok(())			Ok(())
	}			}

	fn add_trusted_file(&mut self, path: &Path) -> Result<(), ConfigError> {			fn add_trusted_file(&mut self, path: &Path) -> Result<(), ConfigError> {
	if let Some(data) =			if let Some(data) = std::fs::read(path)
	std::fs::read(path).for_file(path).io_not_found_as_none()?			.when_reading_file(path)
				.io_not_found_as_none()?
	{			{
	self.layers.extend(ConfigLayer::parse(path, &data)?)			self.layers.extend(ConfigLayer::parse(path, &data)?)
	}			}
	Ok(())			Ok(())
	}			}

	fn add_for_environment_variable(			fn add_for_environment_variable(
	&mut self,			&mut self,

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

	// which `src` can’t be since we’ve managed to open it as a			// which `src` can’t be since we’ve managed to open it as a
	// file.			// file.
	let dir = src			let dir = src
	.parent()			.parent()
	.expect("Path::parent fail on a file we’ve read");			.expect("Path::parent fail on a file we’ve read");
	// `Path::join` with an absolute argument correctly ignores the			// `Path::join` with an absolute argument correctly ignores the
	// base path			// base path
	let filename = dir.join(&get_path_from_bytes(&filename_bytes));			let filename = dir.join(&get_path_from_bytes(&filename_bytes));
	let data = std::fs::read(&filename).for_file(&filename)?;			let data =
				std::fs::read(&filename).when_reading_file(&filename)?;
	layers.push(current_layer);			layers.push(current_layer);
	layers.extend(Self::parse(&filename, &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();

rust/hg-core/src/errors.rs

	/// These errors can happen in many places in the code because values are			/// These errors can happen in many places in the code because values are
	/// parsed lazily as the file-level parser does not know the expected type			/// parsed lazily as the file-level parser does not know the expected type
	/// and syntax of each value.			/// and syntax of each value.
	#[from]			#[from]
	ConfigValueParseError(ConfigValueParseError),			ConfigValueParseError(ConfigValueParseError),
	}			}

	/// Details about where an I/O error happened			/// Details about where an I/O error happened
	#[derive(Debug, derive_more::From)]			#[derive(Debug)]
	pub enum IoErrorContext {			pub enum IoErrorContext {
	/// A filesystem operation for the given file			ReadingFile(std::path::PathBuf),
	#[from]			WritingFile(std::path::PathBuf),
	File(std::path::PathBuf),			RemovingFile(std::path::PathBuf),
				RenamingFile {
				from: std::path::PathBuf,
				to: std::path::PathBuf,
				},
	/// `std::env::current_dir`			/// `std::env::current_dir`
	CurrentDir,			CurrentDir,
	/// `std::env::current_exe`			/// `std::env::current_exe`
	CurrentExe,			CurrentExe,
	}			}

	impl HgError {			impl HgError {
	pub fn corrupted(explanation: impl Into<String>) -> Self {			pub fn corrupted(explanation: impl Into<String>) -> Self {
	}			}
	}			}
	}			}

	// TODO: use `DisplayBytes` instead to show non-Unicode filenames losslessly?			// TODO: use `DisplayBytes` instead to show non-Unicode filenames losslessly?
	impl fmt::Display for IoErrorContext {			impl fmt::Display for IoErrorContext {
	fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {			fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
	match self {			match self {
	IoErrorContext::File(path) => path.display().fmt(f),			IoErrorContext::ReadingFile(path) => {
	IoErrorContext::CurrentDir => f.write_str("current directory"),			write!(f, "when reading {}", path.display())
	IoErrorContext::CurrentExe => f.write_str("current executable"),			}
				IoErrorContext::WritingFile(path) => {
				write!(f, "when writing {}", path.display())
				}
				IoErrorContext::RemovingFile(path) => {
				write!(f, "when removing {}", path.display())
				}
				IoErrorContext::RenamingFile { from, to } => write!(
				f,
				"when renaming {} to {}",
				from.display(),
				to.display()
				),
				IoErrorContext::CurrentDir => write!(f, "current directory"),
				IoErrorContext::CurrentExe => write!(f, "current executable"),
	}			}
	}			}
	}			}

	pub trait IoResultExt<T> {			pub trait IoResultExt<T> {
	/// Annotate a possible I/O error as related to a file at the given path.			/// Annotate a possible I/O error as related to a reading a file at the
				/// given path.
	///			///
	/// This allows printing something like “File not found: example.txt”			/// This allows printing something like “File not found when reading
	/// instead of just “File not found”.			/// example.txt” instead of just “File not found”.
	///			///
	/// Converts a `Result` with `std::io::Error` into one with `HgError`.			/// Converts a `Result` with `std::io::Error` into one with `HgError`.
	fn for_file(self, path: &std::path::Path) -> Result<T, HgError>;			fn when_reading_file(self, path: &std::path::Path) -> Result<T, HgError>;

				fn with_context(
				self,
				context: impl FnOnce() -> IoErrorContext,
				) -> Result<T, HgError>;
	}			}

	impl<T> IoResultExt<T> for std::io::Result<T> {			impl<T> IoResultExt<T> for std::io::Result<T> {
	fn for_file(self, path: &std::path::Path) -> Result<T, HgError> {			fn when_reading_file(self, path: &std::path::Path) -> Result<T, HgError> {
				self.with_context(\|\| IoErrorContext::ReadingFile(path.to_owned()))
				}

				fn with_context(
				self,
				context: impl FnOnce() -> IoErrorContext,
				) -> Result<T, HgError> {
	self.map_err(\|error\| HgError::IoError {			self.map_err(\|error\| HgError::IoError {
	error,			error,
	context: IoErrorContext::File(path.to_owned()),			context: context(),
	})			})
	}			}
	}			}

	pub trait HgResultExt<T> {			pub trait HgResultExt<T> {
	/// Handle missing files separately from other I/O error cases.			/// Handle missing files separately from other I/O error cases.
	///			///
	/// Wraps the `Ok` type in an `Option`:			/// Wraps the `Ok` type in an `Option`:

rust/hg-core/src/lib.rs

	};			};
	pub mod copy_tracing;			pub mod copy_tracing;
	mod filepatterns;			mod filepatterns;
	pub mod matchers;			pub mod matchers;
	pub mod repo;			pub mod repo;
	pub mod revlog;			pub mod revlog;
	pub use revlog::*;			pub use revlog::*;
	pub mod config;			pub mod config;
				pub mod logging;
	pub mod operations;			pub mod operations;
	pub mod revset;			pub mod revset;
	pub mod utils;			pub mod utils;

	use crate::utils::hg_path::{HgPathBuf, HgPathError};			use crate::utils::hg_path::{HgPathBuf, HgPathError};
	pub use filepatterns::{			pub use filepatterns::{
	parse_pattern_syntax, read_pattern_file, IgnorePattern,			parse_pattern_syntax, read_pattern_file, IgnorePattern,
	PatternFileWarning, PatternSyntax,			PatternFileWarning, PatternSyntax,

rust/hg-core/src/logging.rs

This file was added.

				use crate::errors::{HgError, HgResultExt, IoErrorContext, IoResultExt};
				use crate::repo::Vfs;
				use std::io::Write;

				/// An utility to append to a log file with the given name, and optionally
				/// rotate it after it reaches a certain maximum size.
				///
				/// Rotation works by renaming "example.log" to "example.log.1", after renaming
				/// "example.log.1" to "example.log.2" etc up to the given maximum number of
				/// files.
				pub struct LogFile<'a> {
				vfs: Vfs<'a>,
				name: &'a str,
				max_size: Option<u64>,
				max_files: u32,
				}

				impl<'a> LogFile<'a> {
				pub fn new(vfs: Vfs<'a>, name: &'a str) -> Self {
				Self {
				vfs,
				name,
				max_size: None,
				max_files: 0,
				}
				}

				/// Rotate before writing to a log file that was already larger than the
				/// given size, in bytes. `None` disables rotation.
				pub fn max_size(mut self, value: Option<u64>) -> Self {
				self.max_size = value;
				self
				}

				/// Keep this many rotated files `{name}.1` up to `{name}.{max}`, in
				/// addition to the original `{name}` file.
				pub fn max_files(mut self, value: u32) -> Self {
				self.max_files = value;
				self
				}

				/// Append the given `bytes` as-is to the log file, after rotating if
				/// needed.
				///
				/// No trailing newline is added. Make sure to include one in `bytes` if
				/// desired.
				pub fn write(&self, bytes: &[u8]) -> Result<(), HgError> {
				let path = self.vfs.join(self.name);
				let context = \|\| IoErrorContext::WritingFile(path.clone());
				let open = \|\| {
				std::fs::OpenOptions::new()
				.create(true)
				.append(true)
				.open(&path)
				.with_context(context)
				};
				let mut file = open()?;
				if let Some(max_size) = self.max_size {
				if file.metadata().with_context(context)?.len() >= max_size {
				// For example with `max_files == 5`, the first iteration of
				AlphareUnsubmitted Done Should be `>=`, even though it's not really important. Alphare: Should be `>=`, even though it's not really important.
				SimonSapinAuthorUnsubmitted Done Done. SimonSapin: Done.
				// this loop has `i == 4` and renames `{name}.4` to `{name}.5`.
				AlphareUnsubmitted Done Shouldn't it be `(1..=self.max_files)`? Alphare: Shouldn't it be `(1..=self.max_files)`?
				SimonSapinAuthorUnsubmitted Done It could be if we renamed `{name}.{i - 1}` to `{name}.{i}` instead of `{name}.{i}` to `{name}.{i + 1}`. SimonSapin: It could be if we renamed `{name}.{i - 1}` to `{name}.{i}` instead of `{name}.{i}` to `{name}.
				AlphareUnsubmitted Not Done Right, doesn't really matter. I wasn't sure of the behavior of the range operator when doing n..n. Alphare: Right, doesn't really matter. I wasn't sure of the behavior of the range operator when doing n..
				// The last iteration renames `{name}.1` to
				// `{name}.2`
				for i in (1..self.max_files).rev() {
				self.vfs
				.rename(
				format!("{}.{}", self.name, i),
				format!("{}.{}", self.name, i + 1),
				)
				.io_not_found_as_none()?;
				}
				// Then rename `{name}` to `{name}.1`. This is the
				// previously-opened `file`.
				self.vfs
				.rename(self.name, format!("{}.1", self.name))
				.io_not_found_as_none()?;
				// Finally, create a new `{name}` file and replace our `file`
				// handle.
				file = open()?;
				}
				}
				file.write_all(bytes).with_context(context)?;
				file.sync_all().with_context(context)
				}
				}

				#[test]
				fn test_rotation() {
				let temp = tempfile::tempdir().unwrap();
				let vfs = Vfs { base: temp.path() };
				let logger = LogFile::new(vfs, "log").max_size(Some(3)).max_files(2);
				logger.write(b"one\n").unwrap();
				logger.write(b"two\n").unwrap();
				logger.write(b"3\n").unwrap();
				logger.write(b"four\n").unwrap();
				logger.write(b"five\n").unwrap();
				assert_eq!(vfs.read("log").unwrap(), b"five\n");
				assert_eq!(vfs.read("log.1").unwrap(), b"3\nfour\n");
				assert_eq!(vfs.read("log.2").unwrap(), b"two\n");
				assert!(vfs.read("log.3").io_not_found_as_none().unwrap().is_none());
				}

rust/hg-core/src/repo.rs

	use crate::config::{Config, ConfigError, ConfigParseError};			use crate::config::{Config, ConfigError, ConfigParseError};
	use crate::errors::{HgError, IoResultExt};			use crate::errors::{HgError, IoErrorContext, IoResultExt};
	use crate::requirements;			use crate::requirements;
	use crate::utils::current_dir;			use crate::utils::current_dir;
	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
	ConfigError::Parse(error) => error.into(),			ConfigError::Parse(error) => error.into(),
	ConfigError::Other(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 struct Vfs<'a> {
	base: &'a Path,			pub(crate) base: &'a Path,
	}			}

	impl Repo {			impl Repo {
	/// Find a repository, either at the given path (which must contain a `.hg`			/// Find a repository, either at the given path (which must contain a `.hg`
	/// sub-directory) or by searching the current directory and its			/// sub-directory) or by searching the current directory and its
	/// ancestors.			/// ancestors.
	///			///
	/// A method with two very different "modes" like this usually a code smell			/// A method with two very different "modes" like this usually a code smell
	}			}

	pub fn config(&self) -> &Config {			pub fn config(&self) -> &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 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 fn store_vfs(&self) -> Vfs<'_> {
	Vfs { base: &self.store }			Vfs { base: &self.store }
	}			}

	/// For accessing the working copy			/// For accessing the working copy

	// The undescore prefix silences the "never used" warning. Remove before			// The undescore prefix silences the "never used" warning. Remove before
	// using.			// using.
	pub(crate) fn _working_directory_vfs(&self) -> Vfs<'_> {			pub fn _working_directory_vfs(&self) -> Vfs<'_> {
	Vfs {			Vfs {
	base: &self.working_directory,			base: &self.working_directory,
	}			}
	}			}
	}			}

	impl Vfs<'_> {			impl Vfs<'_> {
	pub(crate) fn join(&self, relative_path: impl AsRef<Path>) -> PathBuf {			pub fn join(&self, relative_path: impl AsRef<Path>) -> PathBuf {
	self.base.join(relative_path)			self.base.join(relative_path)
	}			}

	pub(crate) fn read(			pub fn read(
	&self,			&self,
	relative_path: impl AsRef<Path>,			relative_path: impl AsRef<Path>,
	) -> Result<Vec<u8>, HgError> {			) -> Result<Vec<u8>, HgError> {
	let path = self.join(relative_path);			let path = self.join(relative_path);
	std::fs::read(&path).for_file(&path)			std::fs::read(&path).when_reading_file(&path)
	}			}

	pub(crate) fn mmap_open(			pub fn mmap_open(
	&self,			&self,
	relative_path: impl AsRef<Path>,			relative_path: impl AsRef<Path>,
	) -> Result<Mmap, HgError> {			) -> Result<Mmap, HgError> {
	let path = self.base.join(relative_path);			let path = self.base.join(relative_path);
	let file = std::fs::File::open(&path).for_file(&path)?;			let file = std::fs::File::open(&path).when_reading_file(&path)?;
	// TODO: what are the safety requirements here?			// TODO: what are the safety requirements here?
	let mmap = unsafe { MmapOptions::new().map(&file) }.for_file(&path)?;			let mmap = unsafe { MmapOptions::new().map(&file) }
				.when_reading_file(&path)?;
	Ok(mmap)			Ok(mmap)
	}			}

				pub fn rename(
				&self,
				relative_from: impl AsRef<Path>,
				relative_to: impl AsRef<Path>,
				) -> Result<(), HgError> {
				let from = self.join(relative_from);
				let to = self.join(relative_to);
				std::fs::rename(&from, &to)
				.with_context(\|\| IoErrorContext::RenamingFile { from, to })
				}
	}			}