Patchwork D10010: rust: Add a log file rotation utility

login
register
mail settings
Submitter phabricator
Date Feb. 17, 2021, 12:59 p.m.
Message ID <differential-rev-PHID-DREV-a3id2ztar4fudssqmp2k-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/48326/
State Superseded
Headers show

Comments

phabricator - Feb. 17, 2021, 12:59 p.m.
SimonSapin created this revision.
Herald added a reviewer: hg-reviewers.
Herald added a subscriber: mercurial-patches.

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

REPOSITORY
  rHG Mercurial

BRANCH
  default

REVISION DETAIL
  https://phab.mercurial-scm.org/D10010

AFFECTED FILES
  rust/hg-core/src/config/config.rs
  rust/hg-core/src/config/layer.rs
  rust/hg-core/src/errors.rs
  rust/hg-core/src/lib.rs
  rust/hg-core/src/logging.rs
  rust/hg-core/src/repo.rs

CHANGE DETAILS




To: SimonSapin, #hg-reviewers
Cc: mercurial-patches, mercurial-devel

Patch

diff --git a/rust/hg-core/src/repo.rs b/rust/hg-core/src/repo.rs
--- a/rust/hg-core/src/repo.rs
+++ b/rust/hg-core/src/repo.rs
@@ -1,5 +1,5 @@ 
 use crate::config::{Config, ConfigError, ConfigParseError};
-use crate::errors::{HgError, IoResultExt};
+use crate::errors::{HgError, IoErrorContext, IoResultExt};
 use crate::requirements;
 use crate::utils::current_dir;
 use crate::utils::files::get_path_from_bytes;
@@ -38,8 +38,8 @@ 
 
 /// Filesystem access abstraction for the contents of a given "base" diretory
 #[derive(Clone, Copy)]
-pub(crate) struct Vfs<'a> {
-    base: &'a Path,
+pub struct Vfs<'a> {
+    pub(crate) base: &'a Path,
 }
 
 impl Repo {
@@ -196,12 +196,12 @@ 
 
     /// For accessing repository files (in `.hg`), except for the store
     /// (`.hg/store`).
-    pub(crate) fn hg_vfs(&self) -> Vfs<'_> {
+    pub fn hg_vfs(&self) -> Vfs<'_> {
         Vfs { base: &self.dot_hg }
     }
 
     /// 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 }
     }
 
@@ -209,7 +209,7 @@ 
 
     // The undescore prefix silences the "never used" warning. Remove before
     // using.
-    pub(crate) fn _working_directory_vfs(&self) -> Vfs<'_> {
+    pub fn _working_directory_vfs(&self) -> Vfs<'_> {
         Vfs {
             base: &self.working_directory,
         }
@@ -217,26 +217,38 @@ 
 }
 
 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)
     }
 
-    pub(crate) fn read(
+    pub fn read(
         &self,
         relative_path: impl AsRef<Path>,
     ) -> Result<Vec<u8>, HgError> {
         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,
         relative_path: impl AsRef<Path>,
     ) -> Result<Mmap, HgError> {
         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?
-        let mmap = unsafe { MmapOptions::new().map(&file) }.for_file(&path)?;
+        let mmap = unsafe { MmapOptions::new().map(&file) }
+            .when_reading_file(&path)?;
         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 })
+    }
 }
diff --git a/rust/hg-core/src/logging.rs b/rust/hg-core/src/logging.rs
new file mode 100644
--- /dev/null
+++ b/rust/hg-core/src/logging.rs
@@ -0,0 +1,95 @@ 
+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 i in (1..self.max_files).rev() {
+                    self.vfs
+                        .rename(
+                            format!("{}.{}", self.name, i),
+                            format!("{}.{}", self.name, i + 1),
+                        )
+                        .io_not_found_as_none()?;
+                }
+                self.vfs
+                    .rename(self.name, format!("{}.1", self.name))
+                    .io_not_found_as_none()?;
+                // The previously-opened `file` was renamed to "name.1".
+                // Now create a new empty file at "name".
+                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(2)).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());
+}
diff --git a/rust/hg-core/src/lib.rs b/rust/hg-core/src/lib.rs
--- a/rust/hg-core/src/lib.rs
+++ b/rust/hg-core/src/lib.rs
@@ -29,6 +29,7 @@ 
 pub mod revlog;
 pub use revlog::*;
 pub mod config;
+pub mod logging;
 pub mod operations;
 pub mod revset;
 pub mod utils;
diff --git a/rust/hg-core/src/errors.rs b/rust/hg-core/src/errors.rs
--- a/rust/hg-core/src/errors.rs
+++ b/rust/hg-core/src/errors.rs
@@ -41,11 +41,15 @@ 
 }
 
 /// Details about where an I/O error happened
-#[derive(Debug, derive_more::From)]
+#[derive(Debug)]
 pub enum IoErrorContext {
-    /// A filesystem operation for the given file
-    #[from]
-    File(std::path::PathBuf),
+    ReadingFile(std::path::PathBuf),
+    WritingFile(std::path::PathBuf),
+    RemovingFile(std::path::PathBuf),
+    RenamingFile {
+        from: std::path::PathBuf,
+        to: std::path::PathBuf,
+    },
     /// `std::env::current_dir`
     CurrentDir,
     /// `std::env::current_exe`
@@ -109,28 +113,55 @@ 
 impl fmt::Display for IoErrorContext {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match self {
-            IoErrorContext::File(path) => path.display().fmt(f),
-            IoErrorContext::CurrentDir => f.write_str("current directory"),
-            IoErrorContext::CurrentExe => f.write_str("current executable"),
+            IoErrorContext::ReadingFile(path) => {
+                write!(f, "when reading {}", path.display())
+            }
+            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> {
-    /// 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”
-    /// instead of just “File not found”.
+    /// This allows printing something like “File not found when reading
+    /// example.txt” instead of just “File not found”.
     ///
     /// 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> {
-    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 {
             error,
-            context: IoErrorContext::File(path.to_owned()),
+            context: context(),
         })
     }
 }
diff --git a/rust/hg-core/src/config/layer.rs b/rust/hg-core/src/config/layer.rs
--- a/rust/hg-core/src/config/layer.rs
+++ b/rust/hg-core/src/config/layer.rs
@@ -150,7 +150,8 @@ 
                 // `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)?;
+                let data =
+                    std::fs::read(&filename).when_reading_file(&filename)?;
                 layers.push(current_layer);
                 layers.extend(Self::parse(&filename, &data)?);
                 current_layer = Self::new(ConfigOrigin::File(src.to_owned()));
diff --git a/rust/hg-core/src/config/config.rs b/rust/hg-core/src/config/config.rs
--- a/rust/hg-core/src/config/config.rs
+++ b/rust/hg-core/src/config/config.rs
@@ -137,11 +137,11 @@ 
 
     fn add_trusted_dir(&mut self, path: &Path) -> Result<(), ConfigError> {
         if let Some(entries) = std::fs::read_dir(path)
-            .for_file(path)
+            .when_reading_file(path)
             .io_not_found_as_none()?
         {
             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")) {
                     self.add_trusted_file(&file_path)?
                 }
@@ -151,8 +151,9 @@ 
     }
 
     fn add_trusted_file(&mut self, path: &Path) -> Result<(), ConfigError> {
-        if let Some(data) =
-            std::fs::read(path).for_file(path).io_not_found_as_none()?
+        if let Some(data) = std::fs::read(path)
+            .when_reading_file(path)
+            .io_not_found_as_none()?
         {
             self.layers.extend(ConfigLayer::parse(path, &data)?)
         }