From 66f2bb5f1a0637baef5908bc25b27d88c5ff22c2 Mon Sep 17 00:00:00 2001 From: Amey Pawar Date: Mon, 13 Jul 2026 00:52:05 +0530 Subject: [PATCH 1/3] feat!: `gix-error` instead of `thiserror` in `gix-lock` Replace the thiserror-derived `acquire::Error` enum with `gix_error::Exn`, where `Failure` is a concrete error that keeps actual lock contention distinguishable from other IO errors, as callers rely on telling these apart. Error message texts are unchanged. --- Cargo.lock | 2 +- gix-lock/Cargo.toml | 2 +- gix-lock/src/acquire.rs | 56 +++++++++++++++++++++++++++-------- gix-lock/src/lib.rs | 3 +- gix-lock/tests/lock/file.rs | 20 +++++++++---- gix-lock/tests/lock/marker.rs | 9 ++++-- 6 files changed, 67 insertions(+), 25 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8eb558c29f8..674ee2158cc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2041,10 +2041,10 @@ version = "0.0.0" name = "gix-lock" version = "23.0.1" dependencies = [ + "gix-error", "gix-tempfile", "gix-utils", "tempfile", - "thiserror 2.0.18", ] [[package]] diff --git a/gix-lock/Cargo.toml b/gix-lock/Cargo.toml index c33b49dc36c..7b570d3342b 100644 --- a/gix-lock/Cargo.toml +++ b/gix-lock/Cargo.toml @@ -18,7 +18,7 @@ test = true [dependencies] gix-utils = { version = "^0.3.3", default-features = false, path = "../gix-utils" } gix-tempfile = { version = "^23.0.0", default-features = false, path = "../gix-tempfile" } -thiserror = "2.0.18" +gix-error = { version = "^0.2.4", path = "../gix-error" } [dev-dependencies] tempfile = "3.26.0" diff --git a/gix-lock/src/acquire.rs b/gix-lock/src/acquire.rs index 669f5e874aa..2f9f84fc19a 100644 --- a/gix-lock/src/acquire.rs +++ b/gix-lock/src/acquire.rs @@ -4,6 +4,7 @@ use std::{ time::Duration, }; +use gix_error::ErrorExt; use gix_tempfile::{AutoRemove, ContainingDirectory}; use crate::{DOT_LOCK_SUFFIX, File, Marker, backoff}; @@ -40,16 +41,14 @@ impl From for Fail { } } -/// The error returned when acquiring a [`File`] or [`Marker`]. -#[derive(Debug, thiserror::Error)] +/// The failure that occurred when acquiring a [`File`] or [`Marker`]. +/// +/// It's a concrete type to let callers tell actual lock contention apart from +/// other IO errors, like path collisions between a lock file and a directory. +#[derive(Debug)] #[allow(missing_docs)] -pub enum Error { - #[error("Another IO error occurred while obtaining the lock")] - Io(#[from] std::io::Error), - #[error( - "The lock for resource '{resource_path}' could not be obtained {mode} after {attempts} attempt(s). The lockfile at '{resource_path}{}' might need manual deletion.", - super::DOT_LOCK_SUFFIX - )] +pub enum Failure { + Io(std::io::Error), PermanentlyLocked { resource_path: PathBuf, mode: Fail, @@ -57,6 +56,36 @@ pub enum Error { }, } +impl fmt::Display for Failure { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Failure::Io(_) => f.write_str("Another IO error occurred while obtaining the lock"), + Failure::PermanentlyLocked { + resource_path, + mode, + attempts, + } => write!( + f, + "The lock for resource '{resource_path}' could not be obtained {mode} after {attempts} attempt(s). The lockfile at '{resource_path}{suffix}' might need manual deletion.", + resource_path = resource_path.display(), + suffix = DOT_LOCK_SUFFIX + ), + } + } +} + +impl std::error::Error for Failure { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match self { + Failure::Io(err) => Some(err), + Failure::PermanentlyLocked { .. } => None, + } + } +} + +/// The error returned when acquiring a [`File`] or [`Marker`]. +pub type Error = gix_error::Exn; + impl File { /// Create a writable lock file with failure `mode` whose content will eventually overwrite the given resource `at_path`. /// @@ -193,7 +222,7 @@ fn lock_with_mode( std::thread::sleep(wait); continue; } - Err(err) => return Err(Error::from(err)), + Err(err) => return Err(Failure::Io(err).raise()), } } try_lock(&lock_path, directory, cleanup) @@ -201,12 +230,13 @@ fn lock_with_mode( } .map(|v| (lock_path, v)) .map_err(|err| match err.kind() { - AlreadyExists => Error::PermanentlyLocked { + AlreadyExists => Failure::PermanentlyLocked { resource_path: resource.into(), mode, attempts, - }, - _ => Error::Io(err), + } + .raise(), + _ => Failure::Io(err).raise(), }) } diff --git a/gix-lock/src/lib.rs b/gix-lock/src/lib.rs index d6c30c63447..bc1c90b9231 100644 --- a/gix-lock/src/lib.rs +++ b/gix-lock/src/lib.rs @@ -28,7 +28,8 @@ //! &resource, //! gix_lock::acquire::Fail::Immediately, //! None, -//! )?; +//! ) +//! .map_err(|err| err.into_error())?; //! lock.write_all(b"new = value\n")?; //! let (resource_path, _) = lock.commit()?; //! diff --git a/gix-lock/tests/lock/file.rs b/gix-lock/tests/lock/file.rs index 369455b9055..5eb0899e19c 100644 --- a/gix-lock/tests/lock/file.rs +++ b/gix-lock/tests/lock/file.rs @@ -10,7 +10,8 @@ mod close { let resource = dir.path().join("resource-existing.ext"); std::fs::write(&resource, b"old state")?; let resource_lock = resource.with_extension("ext.lock"); - let mut file = gix_lock::File::acquire_to_update_resource(&resource, Fail::Immediately, None)?; + let mut file = gix_lock::File::acquire_to_update_resource(&resource, Fail::Immediately, None) + .map_err(|err| err.into_error())?; assert!(resource_lock.is_file()); file.with_mut(|out| out.write_all(b"hello world"))?; let mark = file.close()?; @@ -35,7 +36,8 @@ mod commit { let dir = tempfile::tempdir()?; let resource = dir.path().join("resource-existing.ext"); std::fs::create_dir(&resource)?; - let mark = gix_lock::Marker::acquire_to_hold_resource(&resource, Fail::Immediately, None)?; + let mark = gix_lock::Marker::acquire_to_hold_resource(&resource, Fail::Immediately, None) + .map_err(|err| err.into_error())?; let lock_path = mark.lock_path().to_owned(); assert!(lock_path.is_file(), "the lock is placed"); @@ -57,7 +59,8 @@ mod commit { let dir = tempfile::tempdir()?; let resource = dir.path().join("resource-existing.ext"); std::fs::create_dir(&resource)?; - let file = gix_lock::File::acquire_to_update_resource(&resource, Fail::Immediately, None)?; + let file = gix_lock::File::acquire_to_update_resource(&resource, Fail::Immediately, None) + .map_err(|err| err.into_error())?; let lock_path = file.lock_path().to_owned(); assert!(lock_path.is_file(), "the lock is placed"); @@ -101,7 +104,8 @@ mod acquire { let resource = dir.path().join("a").join("resource-nonexisting"); let resource_lock = resource.with_extension("lock"); let mut file = - gix_lock::File::acquire_to_update_resource(&resource, fail_immediately(), Some(dir.path().into()))?; + gix_lock::File::acquire_to_update_resource(&resource, fail_immediately(), Some(dir.path().into())) + .map_err(|err| err.into_error())?; assert_eq!(file.lock_path(), resource_lock); assert_eq!(file.resource_path(), resource); assert!(resource_lock.is_file()); @@ -131,7 +135,8 @@ mod acquire { let dir = tempfile::tempdir()?; let resource = dir.path().join("resource-nonexisting.ext"); { - let mut file = gix_lock::File::acquire_to_update_resource(&resource, fail_immediately(), None)?; + let mut file = gix_lock::File::acquire_to_update_resource(&resource, fail_immediately(), None) + .map_err(|err| err.into_error())?; file.with_mut(|out| out.write_all(b"probably we will be interrupted"))?; } assert!(!resource.is_file(), "the file wasn't created"); @@ -143,7 +148,10 @@ mod acquire { let dir = tempfile::tempdir()?; let resource = dir.path().join("a").join("resource.ext"); let res = gix_lock::File::acquire_to_update_resource(&resource, fail_immediately(), None); - assert!(matches!(res, Err(acquire::Error::Io(err)) if err.kind() == ErrorKind::NotFound)); + assert!( + matches!(res.map_err(acquire::Error::into_inner), Err(acquire::Failure::Io(err)) if err.kind() == ErrorKind::NotFound), + "the underlying failure is still identifiable after type-erasure" + ); assert!(dir.path().is_dir(), "it won't meddle with the containing directory"); assert!(!resource.is_file(), "the resource is not created"); assert!( diff --git a/gix-lock/tests/lock/marker.rs b/gix-lock/tests/lock/marker.rs index 02ebe0f84fa..a4d1637307c 100644 --- a/gix-lock/tests/lock/marker.rs +++ b/gix-lock/tests/lock/marker.rs @@ -7,7 +7,8 @@ mod acquire { fn fail_mode_immediately_produces_a_descriptive_error() -> crate::Result { let dir = tempfile::tempdir()?; let resource = dir.path().join("the-resource"); - let guard = gix_lock::Marker::acquire_to_hold_resource(&resource, Fail::Immediately, None)?; + let guard = gix_lock::Marker::acquire_to_hold_resource(&resource, Fail::Immediately, None) + .map_err(|err| err.into_error())?; assert!(guard.lock_path().ends_with("the-resource.lock")); assert!(guard.resource_path().ends_with("the-resource")); let err_str = gix_lock::Marker::acquire_to_hold_resource(resource, Fail::Immediately, None) @@ -23,7 +24,8 @@ mod acquire { fn fail_mode_after_duration_fails_after_a_given_duration_or_more() -> crate::Result { let dir = tempfile::tempdir()?; let resource = dir.path().join("the-resource"); - let _guard = gix_lock::Marker::acquire_to_hold_resource(&resource, Fail::Immediately, None)?; + let _guard = gix_lock::Marker::acquire_to_hold_resource(&resource, Fail::Immediately, None) + .map_err(|err| err.into_error())?; let start = Instant::now(); let time_to_wait = Duration::from_millis(50); let err_str = @@ -70,7 +72,8 @@ mod commit { fn fails_for_ordinary_marker_that_was_never_writable() -> crate::Result { let dir = tempfile::tempdir()?; let resource = dir.path().join("the-resource"); - let mark = gix_lock::Marker::acquire_to_hold_resource(resource, Fail::Immediately, None)?; + let mark = gix_lock::Marker::acquire_to_hold_resource(resource, Fail::Immediately, None) + .map_err(|err| err.into_error())?; #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; From a966aac3a9d014114023d0de8d5cad136e73654b Mon Sep 17 00:00:00 2001 From: Amey Pawar Date: Mon, 13 Jul 2026 00:52:05 +0530 Subject: [PATCH 2/3] Adapt to changes in `gix-lock` Consumers that embed the acquisition error in their own `thiserror` enums keep working unchanged thanks to `Exn`s `Deref` to the concrete `Failure`, which keeps `source()` chains intact. Only variant payloads that need `std::error::Error` themselves now hold `Failure` directly, and `?`-conversions in tests and doctests convert via `into_error()`. --- gix-ref/src/store/file/packed.rs | 5 +++-- gix-ref/src/store/file/transaction/prepare.rs | 10 +++++----- gix-shallow/src/lib.rs | 3 ++- tests/tools/src/lib.rs | 3 ++- 4 files changed, 12 insertions(+), 9 deletions(-) diff --git a/gix-ref/src/store/file/packed.rs b/gix-ref/src/store/file/packed.rs index 30d7f015fc0..a72a7c86476 100644 --- a/gix-ref/src/store/file/packed.rs +++ b/gix-ref/src/store/file/packed.rs @@ -9,7 +9,8 @@ impl file::Store { &self, lock_mode: gix_lock::acquire::Fail, ) -> Result { - let lock = gix_lock::File::acquire_to_update_resource(self.packed_refs_path(), lock_mode, None)?; + let lock = gix_lock::File::acquire_to_update_resource(self.packed_refs_path(), lock_mode, None) + .map_err(|err| transaction::Error::TransactionLock(err.into_inner()))?; // We 'steal' the possibly existing packed buffer which may safe time if it's already there and fresh. // If nothing else is happening, nobody will get to see the soon stale buffer either, but if so, they will pay // for reloading it. That seems preferred over always loading up a new one. @@ -72,7 +73,7 @@ pub mod transaction { #[error("An existing pack couldn't be opened or read when preparing a transaction")] BufferOpen(#[from] packed::buffer::open::Error), #[error("The lock for a packed transaction could not be obtained")] - TransactionLock(#[from] gix_lock::acquire::Error), + TransactionLock(#[source] gix_lock::acquire::Failure), } } diff --git a/gix-ref/src/store/file/transaction/prepare.rs b/gix-ref/src/store/file/transaction/prepare.rs index 10e2188a175..574da63c81b 100644 --- a/gix-ref/src/store/file/transaction/prepare.rs +++ b/gix-ref/src/store/file/transaction/prepare.rs @@ -52,8 +52,8 @@ impl Transaction<'_, '_> { /// burying them in [`Error::LockAcquire`], which is reserved for actual contention. // This happens for path collisions where `a` is a ref file, and `a/b` is the lock to be created. fn lock_acquire_error(err: gix_lock::acquire::Error, full_name: &str) -> Error { - match err { - gix_lock::acquire::Error::Io(err) => Error::Io(err), + match err.into_inner() { + gix_lock::acquire::Failure::Io(err) => Error::Io(err), source => Error::LockAcquire { source, full_name: full_name.into(), @@ -360,7 +360,7 @@ impl Transaction<'_, '_> { self.store.precompose_unicode, self.store.namespace.clone(), ) - .map_err(Error::PackedTransactionAcquire) + .map_err(|err| Error::PackedTransactionAcquire(err.into_inner())) }) .transpose()? }; @@ -480,7 +480,7 @@ mod error { #[error("The packed ref buffer could not be loaded")] Packed(#[from] packed::buffer::open::Error), #[error("The lock for the packed-ref file could not be obtained")] - PackedTransactionAcquire(#[source] gix_lock::acquire::Error), + PackedTransactionAcquire(#[source] gix_lock::acquire::Failure), #[error("The packed transaction could not be prepared")] PackedTransactionPrepare(#[from] packed::transaction::prepare::Error), #[error("The packed ref file could not be parsed")] @@ -489,7 +489,7 @@ mod error { PreprocessingFailed(#[source] std::io::Error), #[error("A lock could not be obtained for reference {full_name:?}")] LockAcquire { - source: gix_lock::acquire::Error, + source: gix_lock::acquire::Failure, full_name: BString, }, #[error("An IO error occurred while applying an edit")] diff --git a/gix-shallow/src/lib.rs b/gix-shallow/src/lib.rs index 77e2d6f90e7..66f0470fdd6 100644 --- a/gix-shallow/src/lib.rs +++ b/gix-shallow/src/lib.rs @@ -15,7 +15,8 @@ //! &shallow_file, //! gix_lock::acquire::Fail::Immediately, //! None, -//! )?; +//! ) +//! .map_err(|err| err.into_error())?; //! gix_shallow::write(lock, Some(shallow), &[gix_shallow::Update::Shallow(second)])?; //! //! let ids = gix_shallow::read(&shallow_file)?.unwrap().into_iter().collect::>(); diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 7bdd3b560f4..58477dfedf0 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -1165,7 +1165,8 @@ fn marker_if_needed( None, ) }) - .transpose()?) + .transpose() + .map_err(|err| err.into_error())?) } fn force_and_dir( From 5a1e5c3e922e40416b4db150706faf14ed1f1b99 Mon Sep 17 00:00:00 2001 From: Amey Pawar Date: Mon, 13 Jul 2026 01:30:31 +0530 Subject: [PATCH 3/3] thanks clippy --- gix-lock/tests/lock/file.rs | 10 +++++----- gix-lock/tests/lock/marker.rs | 6 +++--- tests/tools/src/lib.rs | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/gix-lock/tests/lock/file.rs b/gix-lock/tests/lock/file.rs index 5eb0899e19c..bbe51641885 100644 --- a/gix-lock/tests/lock/file.rs +++ b/gix-lock/tests/lock/file.rs @@ -11,7 +11,7 @@ mod close { std::fs::write(&resource, b"old state")?; let resource_lock = resource.with_extension("ext.lock"); let mut file = gix_lock::File::acquire_to_update_resource(&resource, Fail::Immediately, None) - .map_err(|err| err.into_error())?; + .map_err(gix_lock::acquire::Error::into_error)?; assert!(resource_lock.is_file()); file.with_mut(|out| out.write_all(b"hello world"))?; let mark = file.close()?; @@ -37,7 +37,7 @@ mod commit { let resource = dir.path().join("resource-existing.ext"); std::fs::create_dir(&resource)?; let mark = gix_lock::Marker::acquire_to_hold_resource(&resource, Fail::Immediately, None) - .map_err(|err| err.into_error())?; + .map_err(gix_lock::acquire::Error::into_error)?; let lock_path = mark.lock_path().to_owned(); assert!(lock_path.is_file(), "the lock is placed"); @@ -60,7 +60,7 @@ mod commit { let resource = dir.path().join("resource-existing.ext"); std::fs::create_dir(&resource)?; let file = gix_lock::File::acquire_to_update_resource(&resource, Fail::Immediately, None) - .map_err(|err| err.into_error())?; + .map_err(gix_lock::acquire::Error::into_error)?; let lock_path = file.lock_path().to_owned(); assert!(lock_path.is_file(), "the lock is placed"); @@ -105,7 +105,7 @@ mod acquire { let resource_lock = resource.with_extension("lock"); let mut file = gix_lock::File::acquire_to_update_resource(&resource, fail_immediately(), Some(dir.path().into())) - .map_err(|err| err.into_error())?; + .map_err(gix_lock::acquire::Error::into_error)?; assert_eq!(file.lock_path(), resource_lock); assert_eq!(file.resource_path(), resource); assert!(resource_lock.is_file()); @@ -136,7 +136,7 @@ mod acquire { let resource = dir.path().join("resource-nonexisting.ext"); { let mut file = gix_lock::File::acquire_to_update_resource(&resource, fail_immediately(), None) - .map_err(|err| err.into_error())?; + .map_err(gix_lock::acquire::Error::into_error)?; file.with_mut(|out| out.write_all(b"probably we will be interrupted"))?; } assert!(!resource.is_file(), "the file wasn't created"); diff --git a/gix-lock/tests/lock/marker.rs b/gix-lock/tests/lock/marker.rs index a4d1637307c..ebd5fab6379 100644 --- a/gix-lock/tests/lock/marker.rs +++ b/gix-lock/tests/lock/marker.rs @@ -8,7 +8,7 @@ mod acquire { let dir = tempfile::tempdir()?; let resource = dir.path().join("the-resource"); let guard = gix_lock::Marker::acquire_to_hold_resource(&resource, Fail::Immediately, None) - .map_err(|err| err.into_error())?; + .map_err(gix_lock::acquire::Error::into_error)?; assert!(guard.lock_path().ends_with("the-resource.lock")); assert!(guard.resource_path().ends_with("the-resource")); let err_str = gix_lock::Marker::acquire_to_hold_resource(resource, Fail::Immediately, None) @@ -25,7 +25,7 @@ mod acquire { let dir = tempfile::tempdir()?; let resource = dir.path().join("the-resource"); let _guard = gix_lock::Marker::acquire_to_hold_resource(&resource, Fail::Immediately, None) - .map_err(|err| err.into_error())?; + .map_err(gix_lock::acquire::Error::into_error)?; let start = Instant::now(); let time_to_wait = Duration::from_millis(50); let err_str = @@ -73,7 +73,7 @@ mod commit { let dir = tempfile::tempdir()?; let resource = dir.path().join("the-resource"); let mark = gix_lock::Marker::acquire_to_hold_resource(resource, Fail::Immediately, None) - .map_err(|err| err.into_error())?; + .map_err(gix_lock::acquire::Error::into_error)?; #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 58477dfedf0..9fe47b26117 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -1166,7 +1166,7 @@ fn marker_if_needed( ) }) .transpose() - .map_err(|err| err.into_error())?) + .map_err(gix_lock::acquire::Error::into_error)?) } fn force_and_dir(