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.

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
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.

Alphare requested changes to this revision.Feb 19 2021, 5:41 AM

/!\ 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
SimonSapin added a comment.Feb 19 2021, 6:04 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?

Alphare accepted this revision.Feb 19 2021, 6:10 AM
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
baymax updated this revision to Diff 25678.Feb 19 2021, 6:29 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.

Revision Contents
Changeset List

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