From bc45906ea38adb82a7179cb6b92f7bc34b7e0371 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 5 Sep 2022 19:11:53 +0800 Subject: [PATCH 01/23] replace `quick-error` with `thiserror` (#450) --- Cargo.lock | 2 +- git-chunk/Cargo.toml | 2 +- git-chunk/src/file/decode.rs | 54 +++++++++++++++--------------------- git-chunk/src/file/index.rs | 23 ++++++--------- 4 files changed, 33 insertions(+), 48 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 192999c021b..d3c932f0a9f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1134,7 +1134,7 @@ dependencies = [ name = "git-chunk" version = "0.3.1" dependencies = [ - "quick-error", + "thiserror", ] [[package]] diff --git a/git-chunk/Cargo.toml b/git-chunk/Cargo.toml index 48b905b3ff2..ce509cf5b35 100644 --- a/git-chunk/Cargo.toml +++ b/git-chunk/Cargo.toml @@ -14,4 +14,4 @@ doctest = false test = false [dependencies] -quick-error = "2.0.0" +thiserror = "1.0.34" diff --git a/git-chunk/src/file/decode.rs b/git-chunk/src/file/decode.rs index a6976ef7d3d..1543ac9be41 100644 --- a/git-chunk/src/file/decode.rs +++ b/git-chunk/src/file/decode.rs @@ -1,38 +1,30 @@ use std::{convert::TryInto, ops::Range}; -pub use error::Error; - mod error { - use quick_error::quick_error; - quick_error! { - /// The value returned by [crate::FileRef::from_bytes() - #[derive(Debug)] - #[allow(missing_docs)] - pub enum Error { - EarlySentinelValue { - display("Sentinel value encountered while still processing chunks.") - } - MissingSentinelValue { actual: crate::Id } { - display("Sentinel value wasn't found, saw {:?}", std::str::from_utf8(actual.as_ref()).unwrap_or("")) - } - ChunkSizeOutOfBounds { offset: crate::file::Offset, file_length: u64 } { - display("The chunk offset {} went past the file of length {} - was it truncated?", offset, file_length) - } - NonIncrementalChunkOffsets { - display("All chunk offsets must be incrementing.") - } - DuplicateChunk(kind: crate::Id) { - display("The chunk of kind {:?} was encountered more than once", std::str::from_utf8(kind.as_ref()).unwrap_or("")) - } - TocTooSmall { actual: usize, expected: usize } { - display("The table of contents would be {} bytes, but got only {}", expected, actual) - } - Empty { - display("Empty chunk indices are not allowed as the point of chunked files is to have chunks.") - } - } + /// The value returned by [crate::FileRef::from_bytes() + #[derive(Debug, thiserror::Error)] + #[allow(missing_docs)] + pub enum Error { + #[error("Sentinel value encountered while still processing chunks.")] + EarlySentinelValue, + #[error("Sentinel value wasn't found, saw {:?}", std::str::from_utf8(actual.as_ref()).unwrap_or(""))] + MissingSentinelValue { actual: crate::Id }, + #[error("The chunk offset {offset} went past the file of length {file_length} - was it truncated?")] + ChunkSizeOutOfBounds { + offset: crate::file::Offset, + file_length: u64, + }, + #[error("All chunk offsets must be incrementing.")] + NonIncrementalChunkOffsets, + #[error("The chunk of kind {:?} was encountered more than once", std::str::from_utf8(kind.as_ref()).unwrap_or(""))] + DuplicateChunk { kind: crate::Id }, + #[error("The table of contents would be {expected} bytes, but got only {actual}")] + TocTooSmall { actual: usize, expected: usize }, + #[error("Empty chunk indices are not allowed as the point of chunked files is to have chunks.")] + Empty, } } +pub use error::Error; use crate::{file, file::index}; @@ -62,7 +54,7 @@ impl file::Index { return Err(Error::EarlySentinelValue); } if chunks.iter().any(|c: &index::Entry| c.kind == kind) { - return Err(Error::DuplicateChunk(kind)); + return Err(Error::DuplicateChunk { kind }); } let offset = be_u64(offset); diff --git a/git-chunk/src/file/index.rs b/git-chunk/src/file/index.rs index adbb4fe6159..5b59f6767a3 100644 --- a/git-chunk/src/file/index.rs +++ b/git-chunk/src/file/index.rs @@ -28,21 +28,14 @@ pub mod offset_by_kind { /// pub mod data_by_kind { - use quick_error::quick_error; - quick_error! { - /// The error returned by [Index::data_by_kind()][super::Index::data_by_id()]. - #[derive(Debug)] - #[allow(missing_docs)] - pub enum Error { - NotFound(err: super::offset_by_kind::Error) { - display("The chunk wasn't found in the file index") - from() - source(err) - } - FileTooLarge { - display("The offsets into the file couldn't be represented by usize") - } - } + /// The error returned by [Index::data_by_kind()][super::Index::data_by_id()]. + #[derive(Debug, thiserror::Error)] + #[allow(missing_docs)] + pub enum Error { + #[error("The chunk wasn't found in the file index")] + NotFound(#[from] super::offset_by_kind::Error), + #[error("The offsets into the file couldn't be represented by usize")] + FileTooLarge, } } From 5a74999f853215feb33140997c4a0dc62e49df66 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 7 Sep 2022 09:35:51 +0800 Subject: [PATCH 02/23] chore!: replace quick-error with thiserror (#450) Many of the error definitions changed from tuple types to structs. --- Cargo.lock | 2 +- git-packetline/Cargo.toml | 2 +- git-packetline/src/decode.rs | 76 +++++++++++------------- git-packetline/src/encode/async_io.rs | 8 ++- git-packetline/src/encode/blocking_io.rs | 4 +- git-packetline/src/encode/mod.rs | 22 +++---- git-packetline/src/line/mod.rs | 2 +- 7 files changed, 54 insertions(+), 62 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d3c932f0a9f..c31805bae28 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1457,8 +1457,8 @@ dependencies = [ "hex", "maybe-async", "pin-project-lite", - "quick-error", "serde", + "thiserror", ] [[package]] diff --git a/git-packetline/Cargo.toml b/git-packetline/Cargo.toml index caefa0345ed..a7981dd3e5a 100644 --- a/git-packetline/Cargo.toml +++ b/git-packetline/Cargo.toml @@ -39,7 +39,7 @@ required-features = ["blocking-io", "maybe-async/is_sync"] [dependencies] serde = { version = "1.0.114", optional = true, default-features = false, features = ["std", "derive"]} -quick-error = "2.0.0" +thiserror = "1.0.34" hex = "0.4.2" bstr = { version = "0.2.13", default-features = false, features = ["std"] } # async support diff --git a/git-packetline/src/decode.rs b/git-packetline/src/decode.rs index 19321361b4f..ae24b68f4e3 100644 --- a/git-packetline/src/decode.rs +++ b/git-packetline/src/decode.rs @@ -1,49 +1,35 @@ use bstr::BString; -use quick_error::quick_error; use crate::{PacketLineRef, DELIMITER_LINE, FLUSH_LINE, MAX_DATA_LEN, MAX_LINE_LEN, RESPONSE_END_LINE, U16_HEX_BYTES}; -quick_error! { - /// The error used in the [`decode`][crate::decode] module - #[derive(Debug)] - #[allow(missing_docs)] - pub enum Error { - HexDecode(err: String) { - display("Failed to decode the first four hex bytes indicating the line length: {}", err) - } - DataLengthLimitExceeded(length_in_bytes: usize) { - display("The data received claims to be larger than than the maximum allowed size: got {}, exceeds {}", length_in_bytes, MAX_DATA_LEN) - } - DataIsEmpty { - display("Received an invalid empty line") - } - InvalidLineLength { - display("Received an invalid line of length 3") - } - Line(data: BString, bytes_consumed: usize) { - display("{}", data) - } - NotEnoughData(bytes_needed: usize) { - display("Needing {} additional bytes to decode the line successfully", bytes_needed) - } - } +/// The error used in the [`decode`][crate::decode] module +#[derive(Debug, thiserror::Error)] +#[allow(missing_docs)] +pub enum Error { + #[error("Failed to decode the first four hex bytes indicating the line length: {err}")] + HexDecode { err: String }, + #[error("The data received claims to be larger than than the maximum allowed size: got {length_in_bytes}, exceeds {MAX_DATA_LEN}")] + DataLengthLimitExceeded { length_in_bytes: usize }, + #[error("Received an invalid empty line")] + DataIsEmpty, + #[error("Received an invalid line of length 3")] + InvalidLineLength, + #[error("{data:?} - consumed {bytes_consumed} bytes")] + Line { data: BString, bytes_consumed: usize }, + #[error("Needing {bytes_needed} additional bytes to decode the line successfully")] + NotEnoughData { bytes_needed: usize }, } /// pub mod band { - use quick_error::quick_error; - quick_error! { - /// The error used in [`PacketLineRef::decode_band()`]. - #[derive(Debug)] - #[allow(missing_docs)] - pub enum Error { - InvalidSideBand(band: u8) { - display("attempt to decode a non-side channel line or input was malformed: {}", band) - } - NonDataLine { - display("attempt to decode a non-data line into a side-channel band") - } - } + /// The error used in [`PacketLineRef::decode_band()`]. + #[derive(Debug, thiserror::Error)] + #[allow(missing_docs)] + pub enum Error { + #[error("attempt to decode a non-side channel line or input was malformed: {band_id}")] + InvalidSideBand { band_id: u8 }, + #[error("attempt to decode a non-data line into a side-channel band")] + NonDataLine, } } @@ -86,7 +72,7 @@ pub fn hex_prefix(four_bytes: &[u8]) -> Result, Error } let mut buf = [0u8; U16_HEX_BYTES / 2]; - hex::decode_to_slice(four_bytes, &mut buf).map_err(|err| Error::HexDecode(err.to_string()))?; + hex::decode_to_slice(four_bytes, &mut buf).map_err(|err| Error::HexDecode { err: err.to_string() })?; let wanted_bytes = u16::from_be_bytes(buf); if wanted_bytes == 3 { @@ -105,7 +91,9 @@ pub fn hex_prefix(four_bytes: &[u8]) -> Result, Error /// Obtain a `PacketLine` from `data` after assuring `data` is small enough to fit. pub fn to_data_line(data: &[u8]) -> Result, Error> { if data.len() > MAX_LINE_LEN { - return Err(Error::DataLengthLimitExceeded(data.len())); + return Err(Error::DataLengthLimitExceeded { + length_in_bytes: data.len(), + }); } Ok(PacketLineRef::Data(data)) @@ -129,7 +117,9 @@ pub fn streaming(data: &[u8]) -> Result, Error> { } } + U16_HEX_BYTES; if wanted_bytes > MAX_LINE_LEN { - return Err(Error::DataLengthLimitExceeded(wanted_bytes)); + return Err(Error::DataLengthLimitExceeded { + length_in_bytes: wanted_bytes, + }); } if data_len < wanted_bytes { return Ok(Stream::Incomplete { @@ -150,6 +140,8 @@ pub fn streaming(data: &[u8]) -> Result, Error> { pub fn all_at_once(data: &[u8]) -> Result, Error> { match streaming(data)? { Stream::Complete { line, .. } => Ok(line), - Stream::Incomplete { bytes_needed } => Err(Error::NotEnoughData(bytes_needed)), + Stream::Incomplete { bytes_needed } => Err(Error::NotEnoughData { + bytes_needed: bytes_needed, + }), } } diff --git a/git-packetline/src/encode/async_io.rs b/git-packetline/src/encode/async_io.rs index 8279dc7d0d6..01487e8d4e9 100644 --- a/git-packetline/src/encode/async_io.rs +++ b/git-packetline/src/encode/async_io.rs @@ -61,7 +61,9 @@ impl AsyncWrite for LineWriter<'_, W> { State::Idle => { let data_len = this.prefix.len() + data.len() + this.suffix.len(); if data_len > MAX_DATA_LEN { - return Poll::Ready(Err(into_io_err(Error::DataLengthLimitExceeded(data_len)))); + return Poll::Ready(Err(into_io_err(Error::DataLengthLimitExceeded { + length_in_bytes: data_len, + }))); } if data.is_empty() { return Poll::Ready(Err(into_io_err(Error::DataIsEmpty))); @@ -146,7 +148,9 @@ async fn prefixed_and_suffixed_data_to_write( ) -> io::Result { let data_len = prefix.len() + data.len() + suffix.len(); if data_len > MAX_DATA_LEN { - return Err(into_io_err(Error::DataLengthLimitExceeded(data_len))); + return Err(into_io_err(Error::DataLengthLimitExceeded { + length_in_bytes: data_len, + })); } if data.is_empty() { return Err(into_io_err(Error::DataIsEmpty)); diff --git a/git-packetline/src/encode/blocking_io.rs b/git-packetline/src/encode/blocking_io.rs index e8134452da8..41b705e0b99 100644 --- a/git-packetline/src/encode/blocking_io.rs +++ b/git-packetline/src/encode/blocking_io.rs @@ -52,7 +52,9 @@ fn prefixed_and_suffixed_data_to_write( if data_len > MAX_DATA_LEN { return Err(io::Error::new( io::ErrorKind::Other, - Error::DataLengthLimitExceeded(data_len), + Error::DataLengthLimitExceeded { + length_in_bytes: data_len, + }, )); } if data.is_empty() { diff --git a/git-packetline/src/encode/mod.rs b/git-packetline/src/encode/mod.rs index 3d3b9ac8518..b5a277c8c8f 100644 --- a/git-packetline/src/encode/mod.rs +++ b/git-packetline/src/encode/mod.rs @@ -1,19 +1,13 @@ -use quick_error::quick_error; - use crate::MAX_DATA_LEN; -quick_error! { - /// The error returned by most functions in the [`encode`][crate::encode] module - #[derive(Debug)] - #[allow(missing_docs)] - pub enum Error { - DataLengthLimitExceeded(length_in_bytes: usize) { - display("Cannot encode more than {} bytes, got {}", MAX_DATA_LEN, length_in_bytes) - } - DataIsEmpty { - display("Empty lines are invalid") - } - } +/// The error returned by most functions in the [`encode`][crate::encode] module +#[derive(Debug, thiserror::Error)] +#[allow(missing_docs)] +pub enum Error { + #[error("Cannot encode more than {MAX_DATA_LEN} bytes, got {length_in_bytes}")] + DataLengthLimitExceeded { length_in_bytes: usize }, + #[error("Empty lines are invalid")] + DataIsEmpty, } #[cfg(all(not(feature = "blocking-io"), feature = "async-io"))] diff --git a/git-packetline/src/line/mod.rs b/git-packetline/src/line/mod.rs index 15604d891e2..e3777812807 100644 --- a/git-packetline/src/line/mod.rs +++ b/git-packetline/src/line/mod.rs @@ -59,7 +59,7 @@ impl<'a> PacketLineRef<'a> { 1 => BandRef::Data(&d[1..]), 2 => BandRef::Progress(&d[1..]), 3 => BandRef::Error(&d[1..]), - band => return Err(decode::band::Error::InvalidSideBand(band)), + band => return Err(decode::band::Error::InvalidSideBand { band_id: band }), }) } } From 9c8a36f35b1e8dad66befb102e5a8670b4a6c2b2 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 7 Sep 2022 09:53:54 +0800 Subject: [PATCH 03/23] chore!: remove quick-error in favor of thiserror (#450) Some errors change shape which makes this a breaking change. --- Cargo.lock | 2 +- git-object/Cargo.toml | 2 +- git-object/src/data.rs | 19 ++++++------ git-object/src/encode.rs | 21 ++++++------- git-object/src/kind.rs | 19 +++++------- git-object/src/lib.rs | 56 +++++++++++++++------------------- git-object/src/object/mod.rs | 16 +++++----- git-object/src/tag/write.rs | 23 +++++--------- git-object/src/tree/write.rs | 26 ++++++++-------- git-object/tests/encode/mod.rs | 14 ++++----- 10 files changed, 86 insertions(+), 112 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c31805bae28..685a20d933b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1385,9 +1385,9 @@ dependencies = [ "itoa 1.0.3", "nom", "pretty_assertions", - "quick-error", "serde", "smallvec", + "thiserror", ] [[package]] diff --git a/git-object/Cargo.toml b/git-object/Cargo.toml index 9b7b2ac7ab7..5b23f0d470b 100644 --- a/git-object/Cargo.toml +++ b/git-object/Cargo.toml @@ -28,7 +28,7 @@ git-actor = { version = "^0.11.3", path = "../git-actor" } btoi = "0.4.2" itoa = "1.0.1" -quick-error = "2.0.0" +thiserror = "1.0.34" hex = "0.4.2" bstr = { version = "0.2.13", default-features = false, features = ["std", "unicode"] } nom = { version = "7", default-features = false, features = ["std"]} diff --git a/git-object/src/data.rs b/git-object/src/data.rs index 7abdada0852..493836edc3a 100644 --- a/git-object/src/data.rs +++ b/git-object/src/data.rs @@ -51,17 +51,16 @@ impl<'a> Data<'a> { /// Types supporting object hash verification pub mod verify { - use quick_error::quick_error; - quick_error! { - /// Returned by [`crate::Data::verify_checksum()`] - #[derive(Debug)] - #[allow(missing_docs)] - pub enum Error { - ChecksumMismatch {desired: git_hash::ObjectId, actual: git_hash::ObjectId} { - display("Object expected to have id {}, but actual id was {}", desired, actual) - } - } + /// Returned by [`crate::Data::verify_checksum()`] + #[derive(Debug, thiserror::Error)] + #[allow(missing_docs)] + pub enum Error { + #[error("Object expected to have id {desired}, but actual id was {actual}")] + ChecksumMismatch { + desired: git_hash::ObjectId, + actual: git_hash::ObjectId, + }, } impl crate::Data<'_> { diff --git a/git-object/src/encode.rs b/git-object/src/encode.rs index b3b30328015..b7d5117a862 100644 --- a/git-object/src/encode.rs +++ b/git-object/src/encode.rs @@ -2,18 +2,15 @@ use std::io::{self, Write}; use bstr::{BString, ByteSlice}; -use quick_error::quick_error; -quick_error! { - #[derive(Debug)] - enum Error { - NewlineInHeaderValue(value: BString) { - display("Newlines are not allowed in header values: {:?}", value) - } - EmptyValue { - display("Header values must not be empty") - } - } +/// An error returned when object encoding fails. +#[derive(Debug, thiserror::Error)] +#[allow(missing_docs)] +pub enum Error { + #[error("Newlines are not allowed in header values: {value:?}")] + NewlineInHeaderValue { value: BString }, + #[error("Header values must not be empty")] + EmptyValue, } macro_rules! check { @@ -78,7 +75,7 @@ pub(crate) fn header_field(name: &[u8], value: &[u8], out: impl io::Write) -> io return Err(Error::EmptyValue.into()); } if value.find(NL).is_some() { - return Err(Error::NewlineInHeaderValue(value.into()).into()); + return Err(Error::NewlineInHeaderValue { value: value.into() }.into()); } trusted_header_field(name, value, out) } diff --git a/git-object/src/kind.rs b/git-object/src/kind.rs index 864ab7318f0..86df251bf4e 100644 --- a/git-object/src/kind.rs +++ b/git-object/src/kind.rs @@ -1,18 +1,13 @@ use std::fmt; -use quick_error::quick_error; - use crate::Kind; -quick_error! { - /// The Error used in [`Kind::from_bytes()`]. - #[derive(Debug, Clone)] - #[allow(missing_docs)] - pub enum Error { - InvalidObjectKind(kind: crate::BString) { - display("Unknown object kind: {:?}", kind) - } - } +/// The Error used in [`Kind::from_bytes()`]. +#[derive(Debug, Clone, thiserror::Error)] +#[allow(missing_docs)] +pub enum Error { + #[error("Unknown object kind: {kind:?}")] + InvalidObjectKind { kind: bstr::BString }, } impl Kind { @@ -23,7 +18,7 @@ impl Kind { b"blob" => Kind::Blob, b"commit" => Kind::Commit, b"tag" => Kind::Tag, - _ => return Err(Error::InvalidObjectKind(s.into())), + _ => return Err(Error::InvalidObjectKind { kind: s.into() }), }) } diff --git a/git-object/src/lib.rs b/git-object/src/lib.rs index 212e20f45b4..6264e861b6f 100644 --- a/git-object/src/lib.rs +++ b/git-object/src/lib.rs @@ -336,28 +336,20 @@ pub mod decode { pub use _decode::{Error, ParseError, ParseErrorOwned}; impl std::error::Error for Error {} - use quick_error::quick_error; - quick_error! { - /// Returned by [`loose_header()`] - #[derive(Debug)] - #[allow(missing_docs)] - pub enum LooseHeaderDecodeError { - ParseIntegerError( - source: btoi::ParseIntegerError, - message: &'static str, - number: Vec - ) { - display("{}: {:?}", message, std::str::from_utf8(number)) - } - InvalidHeader(s: &'static str) { - display("{}", s) - } - ObjectHeader(err: super::kind::Error) { - display("The object header contained an unknown object kind.") - from() - source(err) - } - } + /// Returned by [`loose_header()`] + #[derive(Debug, thiserror::Error)] + #[allow(missing_docs)] + pub enum LooseHeaderDecodeError { + #[error("{message}: {number:?}")] + ParseIntegerError { + source: btoi::ParseIntegerError, + message: &'static str, + number: bstr::BString, + }, + #[error("{message}")] + InvalidHeader { message: &'static str }, + #[error("The object header contained an unknown object kind.")] + ObjectHeader(#[from] super::kind::Error), } use bstr::ByteSlice; @@ -367,18 +359,18 @@ pub mod decode { /// `size` is the uncompressed size of the payload in bytes. pub fn loose_header(input: &[u8]) -> Result<(super::Kind, usize, usize), LooseHeaderDecodeError> { use LooseHeaderDecodeError::*; - let kind_end = input.find_byte(0x20).ok_or(InvalidHeader("Expected ' '"))?; + let kind_end = input.find_byte(0x20).ok_or(InvalidHeader { + message: "Expected ' '", + })?; let kind = super::Kind::from_bytes(&input[..kind_end])?; - let size_end = input - .find_byte(0x0) - .ok_or(InvalidHeader("Did not find 0 byte in header"))?; + let size_end = input.find_byte(0x0).ok_or(InvalidHeader { + message: "Did not find 0 byte in header", + })?; let size_bytes = &input[kind_end + 1..size_end]; - let size = btoi::btoi(size_bytes).map_err(|source| { - ParseIntegerError( - source, - "Object size in header could not be parsed", - size_bytes.to_owned(), - ) + let size = btoi::btoi(size_bytes).map_err(|source| ParseIntegerError { + source, + message: "Object size in header could not be parsed", + number: size_bytes.into(), })?; Ok((kind, size, size_end + 1)) } diff --git a/git-object/src/object/mod.rs b/git-object/src/object/mod.rs index 710356d626e..2da1e3d9df5 100644 --- a/git-object/src/object/mod.rs +++ b/git-object/src/object/mod.rs @@ -162,20 +162,18 @@ impl Object { } } -use quick_error::quick_error; - use crate::{ decode::{loose_header, Error as DecodeError, LooseHeaderDecodeError}, BlobRef, CommitRef, Kind, ObjectRef, TagRef, TreeRef, }; -quick_error! { - #[derive(Debug)] - #[allow(missing_docs)] - pub enum LooseDecodeError { - InvalidHeader(err: LooseHeaderDecodeError) { from() } - InvalidContent(err: DecodeError) { from() } - } +#[derive(Debug, thiserror::Error)] +#[allow(missing_docs)] +pub enum LooseDecodeError { + #[error(transparent)] + InvalidHeader(#[from] LooseHeaderDecodeError), + #[error(transparent)] + InvalidContent(#[from] DecodeError), } impl<'a> ObjectRef<'a> { diff --git a/git-object/src/tag/write.rs b/git-object/src/tag/write.rs index 3405d4af5ce..911347807ea 100644 --- a/git-object/src/tag/write.rs +++ b/git-object/src/tag/write.rs @@ -1,24 +1,17 @@ use std::io; use bstr::BStr; -use quick_error::quick_error; use crate::{encode, encode::NL, Kind, Tag, TagRef}; -quick_error! { - /// An Error used in [`Tag::write_to()`]. - #[derive(Debug)] - #[allow(missing_docs)] - pub enum Error { - StartsWithDash { - display("Tags must not start with a dash: '-'") - } - InvalidRefName(err: git_validate::tag::name::Error) { - display("The tag name was no valid reference name") - from() - source(err) - } - } +/// An Error used in [`Tag::write_to()`]. +#[derive(Debug, thiserror::Error)] +#[allow(missing_docs)] +pub enum Error { + #[error("Tags must not start with a dash: '-'")] + StartsWithDash, + #[error("The tag name was no valid reference name")] + InvalidRefName(#[from] git_validate::tag::name::Error), } impl From for io::Error { diff --git a/git-object/src/tree/write.rs b/git-object/src/tree/write.rs index 8c19b944773..3de3cd08f01 100644 --- a/git-object/src/tree/write.rs +++ b/git-object/src/tree/write.rs @@ -1,7 +1,6 @@ use std::io; use bstr::{BString, ByteSlice}; -use quick_error::quick_error; use crate::{ encode::SPACE, @@ -9,15 +8,12 @@ use crate::{ Kind, Tree, TreeRef, }; -quick_error! { - /// The Error used in [`Tree::write_to()`]. - #[derive(Debug)] - #[allow(missing_docs)] - pub enum Error { - NewlineInFilename(name: BString) { - display("Newlines are invalid in file paths: {:?}", name) - } - } +/// The Error used in [`Tree::write_to()`]. +#[derive(Debug, thiserror::Error)] +#[allow(missing_docs)] +pub enum Error { + #[error("Newlines are invalid in file paths: {name:?}")] + NewlineInFilename { name: BString }, } impl From for io::Error { @@ -44,7 +40,10 @@ impl crate::WriteTo for Tree { out.write_all(SPACE)?; if filename.find_byte(b'\n').is_some() { - return Err(Error::NewlineInFilename((*filename).to_owned()).into()); + return Err(Error::NewlineInFilename { + name: (*filename).to_owned(), + } + .into()); } out.write_all(filename)?; out.write_all(&[b'\0'])?; @@ -84,7 +83,10 @@ impl<'a> crate::WriteTo for TreeRef<'a> { out.write_all(SPACE)?; if filename.find_byte(b'\n').is_some() { - return Err(Error::NewlineInFilename((*filename).to_owned()).into()); + return Err(Error::NewlineInFilename { + name: (*filename).to_owned(), + } + .into()); } out.write_all(filename)?; out.write_all(&[b'\0'])?; diff --git a/git-object/tests/encode/mod.rs b/git-object/tests/encode/mod.rs index f524880b37b..ec6d4b64fdb 100644 --- a/git-object/tests/encode/mod.rs +++ b/git-object/tests/encode/mod.rs @@ -1,11 +1,9 @@ -use quick_error::quick_error; -quick_error! { - /// Because the TryFrom implementations don't return proper errors - /// on failure - #[derive(Debug)] - enum Error { - TryFromError {} - } +/// Because the TryFrom implementations don't return proper errors +/// on failure +#[derive(Debug, thiserror::Error)] +enum Error { + #[error("")] + TryFromError, } macro_rules! round_trip { From 9aadac3341bb778e8f8b0800847b574338e16a05 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 7 Sep 2022 09:54:21 +0800 Subject: [PATCH 04/23] adjust to changes in git-object (#450) --- git-repository/tests/revision/spec/from_bytes/mod.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/git-repository/tests/revision/spec/from_bytes/mod.rs b/git-repository/tests/revision/spec/from_bytes/mod.rs index 4dceb0106e7..a26b257ea10 100644 --- a/git-repository/tests/revision/spec/from_bytes/mod.rs +++ b/git-repository/tests/revision/spec/from_bytes/mod.rs @@ -1,11 +1,10 @@ use git_repository::{prelude::ObjectIdExt, revision::Spec}; use git_testtools::hex_to_id; - -mod util; pub use util::*; mod ambiguous; mod regex; +mod util; mod reflog; mod traverse; @@ -93,7 +92,7 @@ fn bad_objects_are_valid_until_they_are_actually_read_from_the_odb() { ); assert_eq!( format!("{:?}", parse_spec("e328^{object}", &repo).unwrap_err()), - r#"FindObject(Find(Loose(Decode(ObjectHeader(InvalidObjectKind("bad"))))))"#, + r#"FindObject(Find(Loose(Decode(ObjectHeader(InvalidObjectKind { kind: "bad" })))))"#, "Now we enforce the object to exist and be valid, as ultimately it wants to match with a certain type" ); } From 4d6ce803da12298b5780af920479679a39d923a0 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 7 Sep 2022 09:59:34 +0800 Subject: [PATCH 05/23] chore!: replace `quick-error` with `thiserror` (#450) --- Cargo.lock | 2 +- git-validate/Cargo.toml | 2 +- git-validate/src/reference.rs | 39 ++++++----------- git-validate/src/tag.rs | 55 ++++++++++-------------- git-validate/tests/validate/reference.rs | 4 +- git-validate/tests/validate/tagname.rs | 2 +- 6 files changed, 42 insertions(+), 62 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 685a20d933b..66335681fab 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1760,7 +1760,7 @@ version = "0.5.5" dependencies = [ "bstr", "git-testtools", - "quick-error", + "thiserror", ] [[package]] diff --git a/git-validate/Cargo.toml b/git-validate/Cargo.toml index bf35506b6f5..cc68ae8bc7f 100644 --- a/git-validate/Cargo.toml +++ b/git-validate/Cargo.toml @@ -13,7 +13,7 @@ doctest = false test = true [dependencies] -quick-error = "2.0.0" +thiserror = "1.0.34" bstr = { version = "0.2.13", default-features = false, features = ["std"] } [dev-dependencies] diff --git a/git-validate/src/reference.rs b/git-validate/src/reference.rs index d3e0d4c48a3..eb1f25a812d 100644 --- a/git-validate/src/reference.rs +++ b/git-validate/src/reference.rs @@ -2,31 +2,20 @@ pub mod name { use std::convert::Infallible; - use quick_error::quick_error; - - quick_error! { - /// The error used in [name()][super::name()] and [name_partial()][super::name_partial()] - #[allow(missing_docs)] - #[derive(Debug)] - pub enum Error { - Tag(err: crate::tag::name::Error) { - display("A reference must be a valid tag name as well") - from() - source(err) - } - SomeLowercase { - display("Standalone references must be all uppercased, like 'HEAD'") - } - StartsWithSlash { - display("A reference name must not start with a slash '/'") - } - RepeatedSlash { - display("Multiple slashes in a row are not allowed as they may change the reference's meaning") - } - SingleDot { - display("Names must not be a single '.', but may contain it.") - } - } + /// The error used in [name()][super::name()] and [name_partial()][super::name_partial()] + #[derive(Debug, thiserror::Error)] + #[allow(missing_docs)] + pub enum Error { + #[error("A reference must be a valid tag name as well")] + Tag(#[from] crate::tag::name::Error), + #[error("Standalone references must be all uppercased, like 'HEAD'")] + SomeLowercase, + #[error("A reference name must not start with a slash '/'")] + StartsWithSlash, + #[error("Multiple slashes in a row are not allowed as they may change the reference's meaning")] + RepeatedSlash, + #[error("Names must not be a single '.', but may contain it.")] + SingleDot, } impl From for Error { diff --git a/git-validate/src/tag.rs b/git-validate/src/tag.rs index 46004ec8357..09e965499c8 100644 --- a/git-validate/src/tag.rs +++ b/git-validate/src/tag.rs @@ -3,38 +3,27 @@ use bstr::BStr; /// pub mod name { use bstr::BString; - use quick_error::quick_error; - quick_error! { - /// The error returned by [`name()`] - #[derive(Debug)] - #[allow(missing_docs)] - pub enum Error { - InvalidByte(name: BString) { - display("A ref must not contain invalid bytes or ascii control characters: '{}'", name) - } - DoubleDot { - display("A ref must not contain '..' as it may be mistaken for a range") - } - LockFileSuffix { - display("A ref must not end with '.lock'") - } - ReflogPortion { - display("A ref must not contain '@{{' which is a part of a ref-log") - } - Asterisk { - display("A ref must not contain '*' character") - } - StartsWithDot { - display("A ref must not start with a '.'") - } - EndsWithSlash { - display("A ref must not end with a '/'") - } - Empty { - display("A ref must not be empty") - } - } + /// The error returned by [`name()`] + #[derive(Debug, thiserror::Error)] + #[allow(missing_docs)] + pub enum Error { + #[error("A ref must not contain invalid bytes or ascii control characters: '{name}'")] + InvalidByte { name: BString }, + #[error("A ref must not contain '..' as it may be mistaken for a range")] + DoubleDot, + #[error("A ref must not end with '.lock'")] + LockFileSuffix, + #[error("A ref must not contain '@{{' which is a part of a ref-log")] + ReflogPortion, + #[error("A ref must not contain '*' character")] + Asterisk, + #[error("A ref must not start with a '.'")] + StartsWithDot, + #[error("A ref must not end with a '/'")] + EndsWithSlash, + #[error("A ref must not be empty")] + Empty, } } @@ -51,7 +40,9 @@ pub fn name(bytes: &BStr) -> Result<&BStr, name::Error> { for byte in bytes.iter() { match byte { b'\\' | b'^' | b':' | b'[' | b'?' | b' ' | b'~' | b'\0'..=b'\x1F' | b'\x7F' => { - return Err(name::Error::InvalidByte((&[*byte][..]).into())) + return Err(name::Error::InvalidByte { + name: (&[*byte][..]).into(), + }) } b'*' => return Err(name::Error::Asterisk), b'.' if previous == b'.' => return Err(name::Error::DoubleDot), diff --git a/git-validate/tests/validate/reference.rs b/git-validate/tests/validate/reference.rs index 3bd35b8ceb8..620572b12ea 100644 --- a/git-validate/tests/validate/reference.rs +++ b/git-validate/tests/validate/reference.rs @@ -73,12 +73,12 @@ mod name_partial { mktest!( path_with_spaces, b"refs//heads/name with spaces", - RefError::Tag(TagError::InvalidByte(_)) + RefError::Tag(TagError::InvalidByte { .. }) ); mktest!( path_with_backslashes, b"refs\\heads/name with spaces", - RefError::Tag(TagError::InvalidByte(_)) + RefError::Tag(TagError::InvalidByte { .. }) ); } } diff --git a/git-validate/tests/validate/tagname.rs b/git-validate/tests/validate/tagname.rs index 408e5009e6c..5523e2f2181 100644 --- a/git-validate/tests/validate/tagname.rs +++ b/git-validate/tests/validate/tagname.rs @@ -41,7 +41,7 @@ mod invalid { #[test] fn $name() { match git_validate::tag::name($input.as_bstr()) { - Err(git_validate::tag::name::Error::InvalidByte(_)) => {} + Err(git_validate::tag::name::Error::InvalidByte { .. }) => {} got => panic!("Wanted {}, got {:?}", stringify!($expected), got), } } From 4eac45f89d4581a7be8eedcc931512cd52c255a9 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 7 Sep 2022 10:00:48 +0800 Subject: [PATCH 06/23] adjust to changes in `git-validate` (#450) --- git-ref/src/name.rs | 7 +++++-- git-validate/src/tag.rs | 6 +++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/git-ref/src/name.rs b/git-ref/src/name.rs index 69b22acd68b..c8ace78af93 100644 --- a/git-ref/src/name.rs +++ b/git-ref/src/name.rs @@ -130,8 +130,11 @@ impl<'a> convert::TryFrom<&'a OsStr> for &'a PartialNameRef { type Error = Error; fn try_from(v: &'a OsStr) -> Result { - let v = git_path::os_str_into_bstr(v) - .map_err(|_| Error::Tag(git_validate::tag::name::Error::InvalidByte("".into())))?; + let v = git_path::os_str_into_bstr(v).map_err(|_| { + Error::Tag(git_validate::tag::name::Error::InvalidByte { + name: "".into(), + }) + })?; Ok(PartialNameRef::new_unchecked(git_validate::reference::name_partial( v.as_bstr(), )?)) diff --git a/git-validate/src/tag.rs b/git-validate/src/tag.rs index 09e965499c8..8e02f2b3e92 100644 --- a/git-validate/src/tag.rs +++ b/git-validate/src/tag.rs @@ -8,8 +8,8 @@ pub mod name { #[derive(Debug, thiserror::Error)] #[allow(missing_docs)] pub enum Error { - #[error("A ref must not contain invalid bytes or ascii control characters: '{name}'")] - InvalidByte { name: BString }, + #[error("A ref must not contain invalid bytes or ascii control characters: {byte:?}")] + InvalidByte { byte: BString }, #[error("A ref must not contain '..' as it may be mistaken for a range")] DoubleDot, #[error("A ref must not end with '.lock'")] @@ -41,7 +41,7 @@ pub fn name(bytes: &BStr) -> Result<&BStr, name::Error> { match byte { b'\\' | b'^' | b':' | b'[' | b'?' | b' ' | b'~' | b'\0'..=b'\x1F' | b'\x7F' => { return Err(name::Error::InvalidByte { - name: (&[*byte][..]).into(), + byte: (&[*byte][..]).into(), }) } b'*' => return Err(name::Error::Asterisk), From 725210dc401406fe9450eae9d375b0238d645027 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 7 Sep 2022 10:41:23 +0800 Subject: [PATCH 07/23] chore!: replace `quick-error` with `thiserror` (#450) --- Cargo.lock | 2 +- git-ref/Cargo.toml | 2 +- git-ref/src/name.rs | 2 +- git-ref/src/peel.rs | 40 +++---- git-ref/src/store/file/find.rs | 80 +++++-------- git-ref/src/store/file/log/iter.rs | 25 ++--- git-ref/src/store/file/log/line.rs | 16 +-- .../src/store/file/loose/reference/decode.rs | 36 +++--- git-ref/src/store/file/loose/reflog.rs | 71 +++++------- git-ref/src/store/file/overlay_iter.rs | 41 +++---- git-ref/src/store/file/packed.rs | 25 ++--- git-ref/src/store/file/raw_ext.rs | 10 +- git-ref/src/store/file/transaction/commit.rs | 52 +++------ git-ref/src/store/file/transaction/prepare.rs | 106 ++++++++---------- git-ref/src/store/general/handle/find.rs | 50 +++------ git-ref/src/store/general/init.rs | 19 +--- git-ref/src/store/packed/buffer.rs | 31 ++--- git-ref/src/store/packed/find.rs | 47 +++----- git-ref/src/store/packed/iter.rs | 21 ++-- git-ref/src/store/packed/transaction.rs | 57 +++------- git-ref/tests/file/store/find.rs | 2 +- git-ref/tests/file/store/iter.rs | 4 +- .../prepare_and_commit/create_or_update.rs | 4 +- .../transaction/prepare_and_commit/delete.rs | 12 +- git-ref/tests/namespace/mod.rs | 2 +- git-ref/tests/packed/iter.rs | 4 +- 26 files changed, 292 insertions(+), 469 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 66335681fab..49202d8db74 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1551,9 +1551,9 @@ dependencies = [ "git-validate", "memmap2", "nom", - "quick-error", "serde", "tempfile", + "thiserror", ] [[package]] diff --git a/git-ref/Cargo.toml b/git-ref/Cargo.toml index 19a21e7fc83..f501eb17692 100644 --- a/git-ref/Cargo.toml +++ b/git-ref/Cargo.toml @@ -32,7 +32,7 @@ git-actor = { version = "^0.11.3", path = "../git-actor" } git-lock = { version = "^2.0.0", path = "../git-lock" } git-tempfile = { version = "^2.0.0", path = "../git-tempfile" } -quick-error = "2.0.0" +thiserror = "1.0.34" nom = { version = "7", default-features = false, features = ["std"]} serde = { version = "1.0.114", optional = true, default-features = false, features = ["derive"]} diff --git a/git-ref/src/name.rs b/git-ref/src/name.rs index c8ace78af93..4162a69fa31 100644 --- a/git-ref/src/name.rs +++ b/git-ref/src/name.rs @@ -132,7 +132,7 @@ impl<'a> convert::TryFrom<&'a OsStr> for &'a PartialNameRef { fn try_from(v: &'a OsStr) -> Result { let v = git_path::os_str_into_bstr(v).map_err(|_| { Error::Tag(git_validate::tag::name::Error::InvalidByte { - name: "".into(), + byte: "".into(), }) })?; Ok(PartialNameRef::new_unchecked(git_validate::reference::name_partial( diff --git a/git-ref/src/peel.rs b/git-ref/src/peel.rs index 361c7d4d7b6..ef0211368c6 100644 --- a/git-ref/src/peel.rs +++ b/git-ref/src/peel.rs @@ -11,34 +11,22 @@ pub mod to_id { use std::path::PathBuf; use git_object::bstr::BString; - use quick_error::quick_error; use crate::file; - quick_error! { - /// The error returned by [`crate::file::ReferenceExt::peel_to_id_in_place()`]. - #[derive(Debug)] - #[allow(missing_docs)] - pub enum Error { - Follow(err: file::find::existing::Error) { - display("Could not follow a single level of a symbolic reference") - from() - source(err) - } - Cycle(start_absolute: PathBuf){ - display("Aborting due to reference cycle with first seen path being '{}'", start_absolute.display()) - } - DepthLimitExceeded { max_depth: usize } { - display("Refusing to follow more than {} levels of indirection", max_depth) - } - Find(err: Box) { - display("An error occurred when trying to resolve an object a reference points to") - from() - source(&**err) - } - NotFound{oid: git_hash::ObjectId, name: BString} { - display("Object {} as referred to by '{}' could not be found", oid, name) - } - } + /// The error returned by [`crate::file::ReferenceExt::peel_to_id_in_place()`]. + #[derive(Debug, thiserror::Error)] + #[allow(missing_docs)] + pub enum Error { + #[error("Could not follow a single level of a symbolic reference")] + Follow(#[from] file::find::existing::Error), + #[error("Aborting due to reference cycle with first seen path being {start_absolute:?}")] + Cycle { start_absolute: PathBuf }, + #[error("Refusing to follow more than {max_depth} levels of indirection")] + DepthLimitExceeded { max_depth: usize }, + #[error("An error occurred when trying to resolve an object a reference points to")] + Find(#[from] Box), + #[error("Object {oid} as referred to by {name:?} could not be found")] + NotFound { oid: git_hash::ObjectId, name: BString }, } } diff --git a/git-ref/src/store/file/find.rs b/git-ref/src/store/file/find.rs index 4f246ab9379..5fb0d5f5e35 100644 --- a/git-ref/src/store/file/find.rs +++ b/git-ref/src/store/file/find.rs @@ -111,7 +111,7 @@ impl file::Store { let add_refs_prefix = matches!(transform, Transform::EnforceRefsPrefix); let full_name = partial_name.construct_full_name_ref(add_refs_prefix, inbetween, path_buf); let content_buf = self.ref_contents(full_name).map_err(|err| Error::ReadFileContents { - err, + source: err, path: self.reference_path(full_name), })?; @@ -148,7 +148,7 @@ impl file::Store { r }) .map_err(|err| Error::ReferenceCreation { - err, + source: err, relative_path: full_name.to_path().to_owned(), })?, )), @@ -296,7 +296,9 @@ pub mod existing { .map_err(|err| Error::Find(find::Error::RefnameValidation(err.into())))?; match self.find_one_with_verified_input(path, packed) { Ok(Some(r)) => Ok(r), - Ok(None) => Err(Error::NotFound(path.to_partial_path().to_owned())), + Ok(None) => Err(Error::NotFound { + name: path.to_partial_path().to_owned(), + }), Err(err) => Err(err.into()), } } @@ -305,24 +307,16 @@ pub mod existing { mod error { use std::path::PathBuf; - use quick_error::quick_error; - use crate::store_impl::file::find; - quick_error! { - /// The error returned by [file::Store::find_existing()][crate::file::Store::find_existing()]. - #[derive(Debug)] - #[allow(missing_docs)] - pub enum Error { - Find(err: find::Error) { - display("An error occurred while trying to find a reference") - from() - source(err) - } - NotFound(name: PathBuf) { - display("The ref partially named '{}' could not be found", name.display()) - } - } + /// The error returned by [file::Store::find_existing()][crate::file::Store::find_existing()]. + #[derive(Debug, thiserror::Error)] + #[allow(missing_docs)] + pub enum Error { + #[error("An error occurred while trying to find a reference")] + Find(#[from] find::Error), + #[error("The ref partially named {name:?} could not be found")] + NotFound { name: PathBuf }, } } } @@ -330,39 +324,25 @@ pub mod existing { mod error { use std::{convert::Infallible, io, path::PathBuf}; - use quick_error::quick_error; - use crate::{file, store_impl::packed}; - quick_error! { - /// The error returned by [file::Store::find()]. - #[derive(Debug)] - #[allow(missing_docs)] - pub enum Error { - RefnameValidation(err: crate::name::Error) { - display("The ref name or path is not a valid ref name") - from() - source(err) - } - ReadFileContents{err: io::Error, path: PathBuf} { - display("The ref file '{}' could not be read in full", path.display()) - source(err) - } - ReferenceCreation{ err: file::loose::reference::decode::Error, relative_path: PathBuf } { - display("The reference at '{}' could not be instantiated", relative_path.display()) - source(err) - } - PackedRef(err: packed::find::Error) { - display("A packed ref lookup failed") - from() - source(err) - } - PackedOpen(err: packed::buffer::open::Error) { - display("Could not open the packed refs buffer when trying to find references.") - from() - source(err) - } - } + /// The error returned by [file::Store::find()]. + #[derive(Debug, thiserror::Error)] + #[allow(missing_docs)] + pub enum Error { + #[error("The ref name or path is not a valid ref name")] + RefnameValidation(#[from] crate::name::Error), + #[error("The ref file {path:?} could not be read in full")] + ReadFileContents { source: io::Error, path: PathBuf }, + #[error("The reference at {relative_path:?} could not be instantiated")] + ReferenceCreation { + source: file::loose::reference::decode::Error, + relative_path: PathBuf, + }, + #[error("A packed ref lookup failed")] + PackedRef(#[from] packed::find::Error), + #[error("Could not open the packed refs buffer when trying to find references.")] + PackedOpen(#[from] packed::buffer::open::Error), } impl From for Error { diff --git a/git-ref/src/store/file/log/iter.rs b/git-ref/src/store/file/log/iter.rs index 991d22c6182..d03e1adf89c 100644 --- a/git-ref/src/store/file/log/iter.rs +++ b/git-ref/src/store/file/log/iter.rs @@ -144,26 +144,17 @@ where /// pub mod reverse { - use quick_error::quick_error; use super::decode; - quick_error! { - /// The error returned by the [`Reverse`][super::Reverse] iterator - #[derive(Debug)] - #[allow(missing_docs)] - pub enum Error { - Io(err: std::io::Error) { - display("The buffer could not be filled to make more lines available") - from() - source(err) - } - Decode(err: decode::Error) { - display("Could not decode log line") - from() - source(err) - } - } + /// The error returned by the [`Reverse`][super::Reverse] iterator + #[derive(Debug, thiserror::Error)] + #[allow(missing_docs)] + pub enum Error { + #[error("The buffer could not be filled to make more lines available")] + Io(#[from] std::io::Error), + #[error("Could not decode log line")] + Decode(#[from] decode::Error), } } diff --git a/git-ref/src/store/file/log/line.rs b/git-ref/src/store/file/log/line.rs index a8d53f6c284..fce4c239514 100644 --- a/git-ref/src/store/file/log/line.rs +++ b/git-ref/src/store/file/log/line.rs @@ -13,19 +13,15 @@ mod write { use std::io; use git_object::bstr::{BStr, ByteSlice}; - use quick_error::quick_error; use crate::log::Line; - quick_error! { - /// The Error produced by [`Line::write_to()`] (but wrapped in an io error). - #[derive(Debug)] - #[allow(missing_docs)] - enum Error { - IllegalCharacter { - display("Messages must not contain newlines\\n") - } - } + /// The Error produced by [`Line::write_to()`] (but wrapped in an io error). + #[derive(Debug, thiserror::Error)] + #[allow(missing_docs)] + enum Error { + #[error("Messages must not contain newlines\\n")] + IllegalCharacter, } impl From for io::Error { diff --git a/git-ref/src/store/file/loose/reference/decode.rs b/git-ref/src/store/file/loose/reference/decode.rs index 274b278ea57..a5c12a4922d 100644 --- a/git-ref/src/store/file/loose/reference/decode.rs +++ b/git-ref/src/store/file/loose/reference/decode.rs @@ -8,7 +8,6 @@ use nom::{ sequence::terminated, IResult, }; -use quick_error::quick_error; use crate::{ parse::{hex_hash, newline}, @@ -21,19 +20,17 @@ enum MaybeUnsafeState { UnvalidatedPath(BString), } -quick_error! { - /// The error returned by [`Reference::try_from_path()`]. - #[derive(Debug)] - #[allow(missing_docs)] - pub enum Error { - Parse(content: BString) { - display("{:?} could not be parsed", content) - } - RefnameValidation{err: git_validate::reference::name::Error, path: BString} { - display("The path to a symbolic reference within a ref file is invalid") - source(err) - } - } +/// The error returned by [`Reference::try_from_path()`]. +#[derive(Debug, thiserror::Error)] +#[allow(missing_docs)] +pub enum Error { + #[error("{content:?} could not be parsed")] + Parse { content: BString }, + #[error("The path {path:?} to a symbolic reference within a ref file is invalid")] + RefnameValidation { + source: git_validate::reference::name::Error, + path: BString, + }, } impl TryFrom for Target { @@ -44,7 +41,12 @@ impl TryFrom for Target { MaybeUnsafeState::Id(id) => Target::Peeled(id), MaybeUnsafeState::UnvalidatedPath(name) => Target::Symbolic(match git_validate::refname(name.as_ref()) { Ok(_) => FullName(name), - Err(err) => return Err(Error::RefnameValidation { err, path: name }), + Err(err) => { + return Err(Error::RefnameValidation { + source: err, + path: name, + }) + } }), }) } @@ -57,7 +59,9 @@ impl Reference { Ok(Reference { name, target: parse(path_contents) - .map_err(|_| Error::Parse(path_contents.into()))? + .map_err(|_| Error::Parse { + content: path_contents.into(), + })? .1 .try_into()?, }) diff --git a/git-ref/src/store/file/loose/reflog.rs b/git-ref/src/store/file/loose/reflog.rs index 9d0d6223169..4ca90c7dcbe 100644 --- a/git-ref/src/store/file/loose/reflog.rs +++ b/git-ref/src/store/file/loose/reflog.rs @@ -122,7 +122,7 @@ pub mod create_or_update { let parent_dir = log_path.parent().expect("always with parent directory"); git_tempfile::create_dir::all(parent_dir, Default::default()).map_err(|err| { Error::CreateLeadingDirectories { - err, + source: err, reflog_directory: parent_dir.to_owned(), } })?; @@ -139,12 +139,12 @@ pub mod create_or_update { .and_then(|_| options.open(&log_path)) .map(Some) .map_err(|_| Error::Append { - err, + source: err, reflog_path: self.reflog_path(name), })? } else { return Err(Error::Append { - err, + source: err, reflog_path: log_path, }); } @@ -162,7 +162,7 @@ pub mod create_or_update { } }) .map_err(|err| Error::Append { - err, + source: err, reflog_path: self.reflog_path(name), })?; } @@ -205,25 +205,22 @@ pub mod create_or_update { mod error { use std::path::PathBuf; - use quick_error::quick_error; - - quick_error! { - /// The error returned when creating or appending to a reflog - #[derive(Debug)] - #[allow(missing_docs)] - pub enum Error { - CreateLeadingDirectories { err: std::io::Error, reflog_directory: PathBuf } { - display("Could create one or more directories in '{}' to contain reflog file", reflog_directory.display()) - source(err) - } - Append { err: std::io::Error, reflog_path: PathBuf } { - display("Could not open reflog file at '{}' for appending", reflog_path.display()) - source(err) - } - MessageWithNewlines { - display("tbd") - } - } + /// The error returned when creating or appending to a reflog + #[derive(Debug, thiserror::Error)] + #[allow(missing_docs)] + pub enum Error { + #[error("Could create one or more directories in {reflog_directory:?} to contain reflog file")] + CreateLeadingDirectories { + source: std::io::Error, + reflog_directory: PathBuf, + }, + #[error("Could not open reflog file at {reflog_path:?} for appending")] + Append { + source: std::io::Error, + reflog_path: PathBuf, + }, + #[error("reflog message must not contain newlines")] + MessageWithNewlines, } } pub use error::Error; @@ -232,26 +229,14 @@ pub mod create_or_update { } mod error { - use std::io; - - use quick_error::quick_error; - - quick_error! { - /// The error returned by [crate::file::Store::reflog_iter()]. - #[derive(Debug)] - #[allow(missing_docs)] - pub enum Error { - RefnameValidation(err: crate::name::Error) { - display("The reflog name or path is not a valid ref name") - from() - source(err) - } - Io(err: io::Error) { - display("The reflog file could not read") - from() - source(err) - } - } + /// The error returned by [crate::file::Store::reflog_iter()]. + #[derive(Debug, thiserror::Error)] + #[allow(missing_docs)] + pub enum Error { + #[error("The reflog name or path is not a valid ref name")] + RefnameValidation(#[from] crate::name::Error), + #[error("The reflog file could not read")] + Io(#[from] std::io::Error), } } pub use error::Error; diff --git a/git-ref/src/store/file/overlay_iter.rs b/git-ref/src/store/file/overlay_iter.rs index 798023380a9..782fca0df94 100644 --- a/git-ref/src/store/file/overlay_iter.rs +++ b/git-ref/src/store/file/overlay_iter.rs @@ -89,7 +89,7 @@ impl<'p, 's> LooseThenPacked<'p, 's> { f.read_to_end(&mut self.buf) }) .map_err(|err| Error::ReadFileContents { - err, + source: err, path: refpath.to_owned(), })?; loose::Reference::try_from_path(name, &self.buf) @@ -103,7 +103,7 @@ impl<'p, 's> LooseThenPacked<'p, 's> { }) .expect("one of our bases contains the path"); Error::ReferenceCreation { - err, + source: err, relative_path: relative_path.into(), } }) @@ -409,31 +409,24 @@ mod error { use std::{io, path::PathBuf}; use git_object::bstr::BString; - use quick_error::quick_error; use crate::store_impl::file; - quick_error! { - /// The error returned by the [`LooseThenPacked`][super::LooseThenPacked] iterator. - #[derive(Debug)] - #[allow(missing_docs)] - pub enum Error { - Traversal(err: io::Error) { - display("The file system could not be traversed") - source(err) - } - ReadFileContents{err: io::Error, path: PathBuf} { - display("The ref file '{}' could not be read in full", path.display()) - source(err) - } - ReferenceCreation{ err: file::loose::reference::decode::Error, relative_path: PathBuf } { - display("The reference at '{}' could not be instantiated", relative_path.display()) - source(err) - } - PackedReference { invalid_line: BString, line_number: usize } { - display("Invalid reference in line {}: '{}'", line_number, invalid_line) - } - } + /// The error returned by the [`LooseThenPacked`][super::LooseThenPacked] iterator. + #[derive(Debug, thiserror::Error)] + #[allow(missing_docs)] + pub enum Error { + #[error("The file system could not be traversed")] + Traversal(#[source] io::Error), + #[error("The ref file {path:?} could not be read in full")] + ReadFileContents { source: io::Error, path: PathBuf }, + #[error("The reference at {relative_path:?} could not be instantiated")] + ReferenceCreation { + source: file::loose::reference::decode::Error, + relative_path: PathBuf, + }, + #[error("Invalid reference in line {line_number}: {invalid_line:?}")] + PackedReference { invalid_line: BString, line_number: usize }, } } pub use error::Error; diff --git a/git-ref/src/store/file/packed.rs b/git-ref/src/store/file/packed.rs index 697e2866c41..5da1a13227c 100644 --- a/git-ref/src/store/file/packed.rs +++ b/git-ref/src/store/file/packed.rs @@ -48,26 +48,17 @@ impl file::Store { /// pub mod transaction { - use quick_error::quick_error; use crate::store_impl::packed; - quick_error! { - /// The error returned by [`file::Store::packed_transaction`][crate::file::Store::packed_transaction()]. - #[derive(Debug)] - #[allow(missing_docs)] - pub enum Error { - BufferOpen(err: packed::buffer::open::Error) { - display("An existing pack couldn't be opened or read when preparing a transaction") - source(err) - from() - } - TransactionLock(err: git_lock::acquire::Error) { - display("The lock for a packed transaction could not be obtained") - source(err) - from() - } - } + /// The error returned by [`file::Store::packed_transaction`][crate::file::Store::packed_transaction()]. + #[derive(Debug, thiserror::Error)] + #[allow(missing_docs)] + pub enum Error { + #[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] git_lock::acquire::Error), } } diff --git a/git-ref/src/store/file/raw_ext.rs b/git-ref/src/store/file/raw_ext.rs index a68e5217178..c22e8784c55 100644 --- a/git-ref/src/store/file/raw_ext.rs +++ b/git-ref/src/store/file/raw_ext.rs @@ -95,7 +95,9 @@ impl ReferenceExt for Reference { while let Some(next) = cursor.follow_packed(store, packed) { let next = next?; if seen.contains(&next.name) { - return Err(peel::to_id::Error::Cycle(store.reference_path(cursor.name.as_ref()))); + return Err(peel::to_id::Error::Cycle { + start_absolute: store.reference_path(cursor.name.as_ref()), + }); } *cursor = next; seen.insert(cursor.name.clone()); @@ -161,9 +163,9 @@ impl ReferenceExt for Reference { Target::Peeled(_) => None, Target::Symbolic(full_name) => match store.try_find_packed(full_name.as_ref(), packed) { Ok(Some(next)) => Some(Ok(next)), - Ok(None) => Some(Err(file::find::existing::Error::NotFound( - full_name.to_path().to_owned(), - ))), + Ok(None) => Some(Err(file::find::existing::Error::NotFound { + name: full_name.to_path().to_owned(), + })), Err(err) => Some(Err(file::find::existing::Error::Find(err))), }, }, diff --git a/git-ref/src/store/file/transaction/commit.rs b/git-ref/src/store/file/transaction/commit.rs index cfb096bcd7e..f76ccb7f538 100644 --- a/git-ref/src/store/file/transaction/commit.rs +++ b/git-ref/src/store/file/transaction/commit.rs @@ -86,7 +86,7 @@ impl<'s> Transaction<'s> { if let Some(err) = err { return Err(Error::LockCommit { - err, + source: err, full_name: change.name(), }); } @@ -108,7 +108,7 @@ impl<'s> Transaction<'s> { if let Err(err) = std::fs::remove_file(&reflog_path) { if err.kind() != std::io::ErrorKind::NotFound { return Err(Error::DeleteReflog { - err, + source: err, full_name: change.name(), }); } @@ -157,41 +157,25 @@ impl<'s> Transaction<'s> { } mod error { use git_object::bstr::BString; - use quick_error::quick_error; use crate::store_impl::{file, packed}; - quick_error! { - /// The error returned by various [`Transaction`][super::Transaction] methods. - #[derive(Debug)] - #[allow(missing_docs)] - pub enum Error { - PackedTransactionCommit(err: packed::transaction::commit::Error) { - display("The packed-ref transaction could not be committed") - source(err) - } - PreprocessingFailed(err: std::io::Error) { - display("Edit preprocessing failed with error: {}", err.to_string()) - source(err) - } - LockCommit{err: std::io::Error, full_name: BString} { - display("THe change for reference {} could not be committed", full_name) - source(err) - } - DeleteReference{ full_name: BString, err: std::io::Error } { - display("The reference '{}' could not be deleted", full_name) - source(err) - } - DeleteReflog{ full_name: BString, err: std::io::Error } { - display("The reflog of reference '{}' could not be deleted", full_name) - source(err) - } - CreateOrUpdateRefLog(err: file::log::create_or_update::Error) { - display("The reflog could not be created or updated") - from() - source(err) - } - } + /// The error returned by various [`Transaction`][super::Transaction] methods. + #[derive(Debug, thiserror::Error)] + #[allow(missing_docs)] + pub enum Error { + #[error("The packed-ref transaction could not be committed")] + PackedTransactionCommit(#[source] packed::transaction::commit::Error), + #[error("Edit preprocessing failed with error")] + PreprocessingFailed { source: std::io::Error }, + #[error("The change for reference {full_name:?} could not be committed")] + LockCommit { source: std::io::Error, full_name: BString }, + #[error("The reference {full_name} could not be deleted")] + DeleteReference { full_name: BString, err: std::io::Error }, + #[error("The reflog of reference {full_name:?} could not be deleted")] + DeleteReflog { full_name: BString, source: std::io::Error }, + #[error("The reflog could not be created or updated")] + CreateOrUpdateRefLog(#[from] file::log::create_or_update::Error), } } pub use error::Error; diff --git a/git-ref/src/store/file/transaction/prepare.rs b/git-ref/src/store/file/transaction/prepare.rs index d0b7b950b43..bb402f2a120 100644 --- a/git-ref/src/store/file/transaction/prepare.rs +++ b/git-ref/src/store/file/transaction/prepare.rs @@ -58,7 +58,7 @@ impl<'s> Transaction<'s> { Some(base.into_owned()), ) .map_err(|err| Error::LockAcquire { - err, + source: err, full_name: "borrowchk won't allow change.name()".into(), })?; let existing_ref = existing_ref?; @@ -104,7 +104,7 @@ impl<'s> Transaction<'s> { Some(base.into_owned()), ) .map_err(|err| Error::LockAcquire { - err, + source: err, full_name: "borrowchk won't allow change.name() and this will be corrected by caller".into(), })?; @@ -307,8 +307,11 @@ impl<'s> Transaction<'s> { change, ) { let err = match err { - Error::LockAcquire { err, full_name: _bogus } => Error::LockAcquire { - err, + Error::LockAcquire { + source, + full_name: _bogus, + } => Error::LockAcquire { + source, full_name: { let mut cursor = change.parent_index; let mut ref_name = change.name(); @@ -367,68 +370,51 @@ fn possibly_adjust_name_for_prefixes(name: &FullNameRef) -> Option { mod error { use git_object::bstr::BString; - use quick_error::quick_error; use crate::{ store_impl::{file, packed}, Target, }; - quick_error! { - /// The error returned by various [`Transaction`][super::Transaction] methods. - #[derive(Debug)] - #[allow(missing_docs)] - pub enum Error { - Packed(err: packed::buffer::open::Error) { - display("The packed ref buffer could not be loaded") - from() - source(err) - } - PackedTransactionAcquire(err: git_lock::acquire::Error) { - display("The lock for the packed-ref file could not be obtained") - source(err) - } - PackedTransactionPrepare(err: packed::transaction::prepare::Error) { - display("The packed transaction could not be prepared") - from() - source(err) - } - PackedFind(err: packed::find::Error) { - display("The packed ref file could not be parsed") - source(err) - from() - } - PreprocessingFailed(err: std::io::Error) { - display("Edit preprocessing failed with error: {}", err.to_string()) - source(err) - } - LockAcquire{err: git_lock::acquire::Error, full_name: BString} { - display("A lock could not be obtained for reference {}", full_name) - source(err) - } - Io(err: std::io::Error) { - display("An IO error occurred while applying an edit") - from() - source(err) - } - DeleteReferenceMustExist { full_name: BString } { - display("The reference '{}' for deletion did not exist or could not be parsed", full_name) - } - MustNotExist { full_name: BString, actual: Target, new: Target } { - display("Reference '{}' was not supposed to exist when writing it with value {}, but actual content was {}", full_name, new, actual) - } - MustExist { full_name: BString, expected: Target } { - display("Reference '{}' was supposed to exist with value {}, but didn't.", full_name, expected) - } - ReferenceOutOfDate { full_name: BString, expected: Target, actual: Target } { - display("The reference '{}' should have content {}, actual content was {}", full_name, expected, actual) - } - ReferenceDecode(err: file::loose::reference::decode::Error) { - display("Could not read reference") - from() - source(err) - } - } + /// The error returned by various [`Transaction`][super::Transaction] methods. + #[derive(Debug, thiserror::Error)] + #[allow(missing_docs)] + pub enum 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] git_lock::acquire::Error), + #[error("The packed transaction could not be prepared")] + PackedTransactionPrepare(#[from] packed::transaction::prepare::Error), + #[error("The packed ref file could not be parsed")] + PackedFind(#[from] packed::find::Error), + #[error("Edit preprocessing failed with an error")] + PreprocessingFailed(#[source] std::io::Error), + #[error("A lock could not be obtained for reference {full_name:?}")] + LockAcquire { + source: git_lock::acquire::Error, + full_name: BString, + }, + #[error("An IO error occurred while applying an edit")] + Io(#[from] std::io::Error), + #[error("The reference {full_name:?} for deletion did not exist or could not be parsed")] + DeleteReferenceMustExist { full_name: BString }, + #[error("Reference {full_name:?} was not supposed to exist when writing it with value {new:?}, but actual content was {actual:?}")] + MustNotExist { + full_name: BString, + actual: Target, + new: Target, + }, + #[error("Reference {full_name:?} was supposed to exist with value {expected}, but didn't.")] + MustExist { full_name: BString, expected: Target }, + #[error("The reference {full_name:?} should have content {expected}, actual content was {actual}")] + ReferenceOutOfDate { + full_name: BString, + expected: Target, + actual: Target, + }, + #[error("Could not read reference")] + ReferenceDecode(#[from] file::loose::reference::decode::Error), } } diff --git a/git-ref/src/store/general/handle/find.rs b/git-ref/src/store/general/handle/find.rs index aba691f7d79..f775e995fc4 100644 --- a/git-ref/src/store/general/handle/find.rs +++ b/git-ref/src/store/general/handle/find.rs @@ -5,24 +5,14 @@ use crate::{store, PartialNameRef, Reference}; mod error { use std::convert::Infallible; - use quick_error::quick_error; - - quick_error! { - /// The error returned by [crate::Store::find()]. - #[derive(Debug)] - #[allow(missing_docs)] - pub enum Error { - Loose(err: crate::file::find::Error) { - display("An error occurred while finding a reference in the loose file database") - from() - source(err) - } - RefnameValidation(err: crate::name::Error) { - display("The ref name or path is not a valid ref name") - from() - source(err) - } - } + /// The error returned by [crate::Store::find()]. + #[derive(Debug, thiserror::Error)] + #[allow(missing_docs)] + pub enum Error { + #[error("An error occurred while finding a reference in the loose file database")] + Loose(#[from] crate::file::find::Error), + #[error("The ref name or path is not a valid ref name")] + RefnameValidation(#[from] crate::name::Error), } impl From for Error { @@ -56,22 +46,14 @@ mod existing { mod error { use std::path::PathBuf; - use quick_error::quick_error; - - quick_error! { - /// The error returned by [file::Store::find_existing()][crate::file::Store::find_existing()]. - #[derive(Debug)] - #[allow(missing_docs)] - pub enum Error { - Find(err: crate::store::find::Error) { - display("An error occurred while finding a reference in the database") - from() - source(err) - } - NotFound(name: PathBuf) { - display("The ref partially named '{}' could not be found", name.display()) - } - } + /// The error returned by [file::Store::find_existing()][crate::file::Store::find_existing()]. + #[derive(Debug, thiserror::Error)] + #[allow(missing_docs)] + pub enum Error { + #[error("An error occurred while finding a reference in the database")] + Find(#[from] crate::store::find::Error), + #[error("The ref partially named {name:?} could not be found")] + NotFound { name: PathBuf }, } } diff --git a/git-ref/src/store/general/init.rs b/git-ref/src/store/general/init.rs index 147ed1fcca2..dfd7d3a7f14 100644 --- a/git-ref/src/store/general/init.rs +++ b/git-ref/src/store/general/init.rs @@ -3,19 +3,12 @@ use std::path::PathBuf; use crate::store::WriteReflog; mod error { - use quick_error::quick_error; - - quick_error! { - /// The error returned by [crate::Store::at()]. - #[derive(Debug)] - #[allow(missing_docs)] - pub enum Error { - Io(err: std::io::Error) { - display("There was an error accessing the store's directory") - from() - source(err) - } - } + /// The error returned by [crate::Store::at()]. + #[derive(Debug, thiserror::Error)] + #[allow(missing_docs)] + pub enum Error { + #[error("There was an error accessing the store's directory")] + Io(#[from] std::io::Error), } } diff --git a/git-ref/src/store/packed/buffer.rs b/git-ref/src/store/packed/buffer.rs index 78debd3c93b..6786e4a9f66 100644 --- a/git-ref/src/store/packed/buffer.rs +++ b/git-ref/src/store/packed/buffer.rs @@ -85,29 +85,18 @@ pub mod open { } mod error { - use quick_error::quick_error; - use crate::packed; - quick_error! { - /// The error returned by [`open()`][super::packed::Buffer::open()]. - #[derive(Debug)] - #[allow(missing_docs)] - pub enum Error { - Iter(err: packed::iter::Error) { - display("The packed-refs file did not have a header or wasn't sorted and could not be iterated") - from() - source(err) - } - HeaderParsing { - display("The header could not be parsed, even though first line started with '#'") - } - Io(err: std::io::Error) { - display("The buffer could not be opened or read") - from() - source(err) - } - } + /// The error returned by [`open()`][super::packed::Buffer::open()]. + #[derive(Debug, thiserror::Error)] + #[allow(missing_docs)] + pub enum Error { + #[error("The packed-refs file did not have a header or wasn't sorted and could not be iterated")] + Iter(#[from] packed::iter::Error), + #[error("The header could not be parsed, even though first line started with '#'")] + HeaderParsing, + #[error("The buffer could not be opened or read")] + Io(#[from] std::io::Error), } } pub use error::Error; diff --git a/git-ref/src/store/packed/find.rs b/git-ref/src/store/packed/find.rs index 1a0184328c7..1e50fe73d39 100644 --- a/git-ref/src/store/packed/find.rs +++ b/git-ref/src/store/packed/find.rs @@ -107,22 +107,14 @@ impl packed::Buffer { mod error { use std::convert::Infallible; - use quick_error::quick_error; - - quick_error! { - /// The error returned by [`find()`][super::packed::Buffer::find()] - #[derive(Debug)] - #[allow(missing_docs)] - pub enum Error { - RefnameValidation(err: crate::name::Error) { - display("The ref name or path is not a valid ref name") - from() - source(err) - } - Parse { - display("The reference could not be parsed") - } - } + /// The error returned by [`find()`][super::packed::Buffer::find()] + #[derive(Debug, thiserror::Error)] + #[allow(missing_docs)] + pub enum Error { + #[error("The ref name or path is not a valid ref name")] + RefnameValidation(#[from] crate::name::Error), + #[error("The reference could not be parsed")] + Parse, } impl From for Error { @@ -135,22 +127,15 @@ pub use error::Error; /// pub mod existing { - use quick_error::quick_error; - quick_error! { - /// The error returned by [`find_existing()`][super::packed::Buffer::find_existing()] - #[derive(Debug)] - #[allow(missing_docs)] - pub enum Error { - Find(err: super::Error) { - display("The find operation failed") - from() - source(err) - } - NotFound { - display("The reference did not exist even though that was expected") - } - } + /// The error returned by [`find_existing()`][super::packed::Buffer::find_existing()] + #[derive(Debug, thiserror::Error)] + #[allow(missing_docs)] + pub enum Error { + #[error("The find operation failed")] + Find(#[from] super::Error), + #[error("The reference did not exist even though that was expected")] + NotFound, } } diff --git a/git-ref/src/store/packed/iter.rs b/git-ref/src/store/packed/iter.rs index acac50b909c..8029b5ddb53 100644 --- a/git-ref/src/store/packed/iter.rs +++ b/git-ref/src/store/packed/iter.rs @@ -102,20 +102,15 @@ impl<'a> packed::Iter<'a> { mod error { use git_object::bstr::BString; - use quick_error::quick_error; - quick_error! { - /// The error returned by [`Iter::new(…)`][super::Iter::new()], - #[derive(Debug)] - #[allow(missing_docs)] - pub enum Error { - Header { invalid_first_line: BString } { - display("The header existed but could not be parsed: '{}'", invalid_first_line) - } - Reference { invalid_line: BString, line_number: usize } { - display("Invalid reference in line {}: '{}'", line_number, invalid_line) - } - } + /// The error returned by [`Iter::new(…)`][super::Iter::new()], + #[derive(Debug, thiserror::Error)] + #[allow(missing_docs)] + pub enum Error { + #[error("The header existed but could not be parsed: {invalid_first_line:?}")] + Header { invalid_first_line: BString }, + #[error("Invalid reference in line {line_number}: {invalid_line:?}")] + Reference { invalid_line: BString, line_number: usize }, } } diff --git a/git-ref/src/store/packed/transaction.rs b/git-ref/src/store/packed/transaction.rs index 1c861c48fe9..1470647eb1e 100644 --- a/git-ref/src/store/packed/transaction.rs +++ b/git-ref/src/store/packed/transaction.rs @@ -229,51 +229,30 @@ pub(crate) fn buffer_into_transaction( /// pub mod prepare { - use quick_error::quick_error; - quick_error! { - /// The error used in [`Transaction::prepare(…)`][super::packed::Transaction::prepare()]. - #[derive(Debug)] - #[allow(missing_docs)] - pub enum Error { - CloseLock(err: std::io::Error) { - display("Could not close a lock which won't ever be committed") - source(err) - } - Resolve(err: Box) { - display("The lookup of an object failed while peeling it") - from() - source(&**err) - } - } + /// The error used in [`Transaction::prepare(…)`][super::packed::Transaction::prepare()]. + #[derive(Debug, thiserror::Error)] + #[allow(missing_docs)] + pub enum Error { + #[error("Could not close a lock which won't ever be committed")] + CloseLock(#[from] std::io::Error), + #[error("The lookup of an object failed while peeling it")] + Resolve(#[from] Box), } } /// pub mod commit { - use quick_error::quick_error; - use crate::store_impl::packed; - quick_error! { - /// The error used in [`Transaction::commit(…)`][super::packed::Transaction::commit()]. - #[derive(Debug)] - #[allow(missing_docs)] - pub enum Error { - Commit(err: git_lock::commit::Error) { - display("Changes to the resource could not be committed") - from() - source(err) - } - Iteration(err: packed::iter::Error) { - display("Some references in the packed refs buffer could not be parsed") - from() - source(err) - } - Io(err: std::io::Error) { - display("Failed to write a ref line to the packed ref file") - from() - source(err) - } - } + /// The error used in [`Transaction::commit(…)`][super::packed::Transaction::commit()]. + #[derive(Debug, thiserror::Error)] + #[allow(missing_docs)] + pub enum Error { + #[error("Changes to the resource could not be committed")] + Commit(#[from] git_lock::commit::Error), + #[error("Some references in the packed refs buffer could not be parsed")] + Iteration(#[from] packed::iter::Error), + #[error("Failed to write a ref line to the packed ref file")] + Io(#[from] std::io::Error), } } diff --git a/git-ref/tests/file/store/find.rs b/git-ref/tests/file/store/find.rs index c1c982556c5..097b64bf732 100644 --- a/git-ref/tests/file/store/find.rs +++ b/git-ref/tests/file/store/find.rs @@ -107,7 +107,7 @@ mod loose { Some(expected_path) => assert_eq!(reference?.name.as_bstr(), expected_path), None => match reference { Ok(_) => panic!("Expected error"), - Err(git_ref::file::find::existing::Error::NotFound(name)) => { + Err(git_ref::file::find::existing::Error::NotFound { name }) => { assert_eq!(name, Path::new(*partial_name)); } Err(err) => panic!("Unexpected err: {:?}", err), diff --git a/git-ref/tests/file/store/iter.rs b/git-ref/tests/file/store/iter.rs index 5566d6aebb2..682cb477716 100644 --- a/git-ref/tests/file/store/iter.rs +++ b/git-ref/tests/file/store/iter.rs @@ -261,9 +261,9 @@ fn loose_iter_with_broken_refs() -> crate::Result { "there is exactly one invalid item, and it didn't abort the iterator most importantly" ); #[cfg(not(windows))] - let msg = "The reference at 'refs/broken' could not be instantiated"; + let msg = "The reference at \"refs/broken\" could not be instantiated"; #[cfg(windows)] - let msg = "The reference at 'refs\\broken' could not be instantiated"; + let msg = "The reference at \"refs\\broken\" could not be instantiated"; assert_eq!( actual[first_error].as_ref().expect_err("unparsable ref").to_string(), msg diff --git a/git-ref/tests/file/transaction/prepare_and_commit/create_or_update.rs b/git-ref/tests/file/transaction/prepare_and_commit/create_or_update.rs index 9efaf76b7c5..666f3517069 100644 --- a/git-ref/tests/file/transaction/prepare_and_commit/create_or_update.rs +++ b/git-ref/tests/file/transaction/prepare_and_commit/create_or_update.rs @@ -55,10 +55,10 @@ fn reference_with_equally_named_empty_or_non_empty_directory_already_in_place_ca } else { match edits { #[cfg_attr(target_os = "windows", allow(unused_variables))] - Err(transaction::commit::Error::LockCommit { err, full_name }) => { + Err(transaction::commit::Error::LockCommit { source, full_name }) => { assert_eq!(full_name, "HEAD"); #[cfg(not(windows))] - assert_eq!(err.to_string(), "Directory not empty"); + assert_eq!(source.to_string(), "Directory not empty"); } _ => unreachable!("other errors shouldn't happen here"), }; diff --git a/git-ref/tests/file/transaction/prepare_and_commit/delete.rs b/git-ref/tests/file/transaction/prepare_and_commit/delete.rs index a7f85885456..1eecd5fb7f8 100644 --- a/git-ref/tests/file/transaction/prepare_and_commit/delete.rs +++ b/git-ref/tests/file/transaction/prepare_and_commit/delete.rs @@ -52,7 +52,7 @@ fn delete_a_ref_which_is_gone_but_must_exist_fails() -> crate::Result { Ok(_) => unreachable!("must exist, but it doesn't actually exist"), Err(err) => assert_eq!( err.to_string(), - "The reference 'DOES_NOT_EXIST' for deletion did not exist or could not be parsed" + "The reference \"DOES_NOT_EXIST\" for deletion did not exist or could not be parsed" ), } Ok(()) @@ -121,7 +121,7 @@ fn delete_ref_with_incorrect_previous_value_fails() -> crate::Result { match res { Err(err) => { - assert_eq!(err.to_string(), "The reference 'refs/heads/main' should have content ref: refs/heads/main, actual content was 02a7a22d90d7c02fb494ed25551850b868e634f0"); + assert_eq!(err.to_string(), "The reference \"refs/heads/main\" should have content ref: refs/heads/main, actual content was 02a7a22d90d7c02fb494ed25551850b868e634f0"); } Ok(_) => unreachable!("must be err"), } @@ -223,7 +223,7 @@ fn delete_broken_ref_that_must_exist_fails_as_it_is_no_valid_ref() -> crate::Res Err(err) => { assert_eq!( err.to_string(), - "The reference 'HEAD' for deletion did not exist or could not be parsed" + "The reference \"HEAD\" for deletion did not exist or could not be parsed" ); } Ok(_) => unreachable!("expected error"), @@ -359,7 +359,7 @@ fn packed_refs_are_consulted_when_determining_previous_value_of_ref_to_be_delete name: "refs/heads/main".try_into()?, deref: false, }), - git_lock::acquire::Fail::Immediately, + Fail::Immediately, )? .commit(committer().to_ref())?; @@ -392,7 +392,7 @@ fn a_loose_ref_with_old_value_check_and_outdated_packed_refs_value_deletes_both_ name: branch.name, deref: false, }), - git_lock::acquire::Fail::Immediately, + Fail::Immediately, )? .commit(committer().to_ref())?; @@ -431,7 +431,7 @@ fn all_contained_references_deletes_the_packed_ref_file_too() -> crate::Result { deref: false, } }), - git_lock::acquire::Fail::Immediately, + Fail::Immediately, )? .commit(committer().to_ref())?; diff --git a/git-ref/tests/namespace/mod.rs b/git-ref/tests/namespace/mod.rs index a785ab64696..a205c03dc46 100644 --- a/git-ref/tests/namespace/mod.rs +++ b/git-ref/tests/namespace/mod.rs @@ -32,7 +32,7 @@ mod expand { assert!(matches!( git_ref::namespace::expand("foo\\bar").expect_err("empty invalid"), git_validate::refname::Error::Tag( - git_validate::tag::name::Error::InvalidByte(byte) + git_validate::tag::name::Error::InvalidByte{byte} ) if byte == "\\" )); } diff --git a/git-ref/tests/packed/iter.rs b/git-ref/tests/packed/iter.rs index 86a5f8ffa5d..c6b82a21c61 100644 --- a/git-ref/tests/packed/iter.rs +++ b/git-ref/tests/packed/iter.rs @@ -123,7 +123,7 @@ buggy-hash refs/wrong .expect("second ref") .expect_err("an error is produced") .to_string(), - "Invalid reference in line 2: 'buggy-hash refs/wrong'", + "Invalid reference in line 2: \"buggy-hash refs/wrong\"", "second line is invalid", ); assert_eq!( @@ -131,7 +131,7 @@ buggy-hash refs/wrong .expect("third ref") .expect_err("an error is produced") .to_string(), - "Invalid reference in line 3: '^buggy-hash-too'", + "Invalid reference in line 3: \"^buggy-hash-too\"", "third line is invalid", ); assert!(iter.next().expect("last ref").is_ok(), "last line is valid"); From 1d8556461a585188616683d3b9998ec07936d5fc Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 7 Sep 2022 10:42:04 +0800 Subject: [PATCH 08/23] thanks clippy --- git-packetline/src/decode.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/git-packetline/src/decode.rs b/git-packetline/src/decode.rs index ae24b68f4e3..d82bc5e2dfa 100644 --- a/git-packetline/src/decode.rs +++ b/git-packetline/src/decode.rs @@ -140,8 +140,6 @@ pub fn streaming(data: &[u8]) -> Result, Error> { pub fn all_at_once(data: &[u8]) -> Result, Error> { match streaming(data)? { Stream::Complete { line, .. } => Ok(line), - Stream::Incomplete { bytes_needed } => Err(Error::NotEnoughData { - bytes_needed: bytes_needed, - }), + Stream::Incomplete { bytes_needed } => Err(Error::NotEnoughData { bytes_needed }), } } From 296f23fea43f543e10448fc0c948f629f241561e Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 7 Sep 2022 10:47:25 +0800 Subject: [PATCH 09/23] adjust to changes in `git-ref` (#450) --- git-repository/tests/reference/mod.rs | 2 +- git-repository/tests/repository/object.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/git-repository/tests/reference/mod.rs b/git-repository/tests/reference/mod.rs index 3515b375c56..70e901396e8 100644 --- a/git-repository/tests/reference/mod.rs +++ b/git-repository/tests/reference/mod.rs @@ -81,7 +81,7 @@ fn set_target_id() { .set_target_id(prev_id, "fails") .unwrap_err() .to_string() - .starts_with("Reference 'refs/heads/main' was supposed to exist")); + .starts_with("Reference \"refs/heads/main\" was supposed to exist")); } mod remote; diff --git a/git-repository/tests/repository/object.rs b/git-repository/tests/repository/object.rs index 5b08cbbd0ec..27d05821f1b 100644 --- a/git-repository/tests/repository/object.rs +++ b/git-repository/tests/repository/object.rs @@ -167,7 +167,7 @@ mod commit { .unwrap_err(); assert_eq!( err.to_string(), - "Reference 'refs/heads/main' was supposed to exist with value 4b825dc642cb6eb9a060e54bf8d69288fbee4904, but didn't.", + "Reference \"refs/heads/main\" was supposed to exist with value 4b825dc642cb6eb9a060e54bf8d69288fbee4904, but didn't.", "cannot provide parent id in initial commit" ); } From dad9cbeb853c0cc5128360b05c04b5a3da7ec75e Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 7 Sep 2022 10:57:08 +0800 Subject: [PATCH 10/23] fix docs (#450) --- git-object/src/tag/write.rs | 2 +- git-object/src/tree/write.rs | 2 +- git-packetline/src/decode.rs | 4 ++-- git-ref/src/store/file/find.rs | 2 +- git-ref/src/store/file/packed.rs | 2 +- git-ref/src/store/general/handle/find.rs | 2 +- git-ref/src/store/packed/find.rs | 2 +- git-ref/src/store/packed/iter.rs | 2 +- git-ref/src/store/packed/transaction.rs | 4 ++-- git-validate/src/tag.rs | 2 +- 10 files changed, 12 insertions(+), 12 deletions(-) diff --git a/git-object/src/tag/write.rs b/git-object/src/tag/write.rs index 911347807ea..e82569a1241 100644 --- a/git-object/src/tag/write.rs +++ b/git-object/src/tag/write.rs @@ -4,7 +4,7 @@ use bstr::BStr; use crate::{encode, encode::NL, Kind, Tag, TagRef}; -/// An Error used in [`Tag::write_to()`]. +/// An Error used in [`Tag::write_to()`][crate::WriteTo::write_to()]. #[derive(Debug, thiserror::Error)] #[allow(missing_docs)] pub enum Error { diff --git a/git-object/src/tree/write.rs b/git-object/src/tree/write.rs index 3de3cd08f01..1e8edc024e0 100644 --- a/git-object/src/tree/write.rs +++ b/git-object/src/tree/write.rs @@ -8,7 +8,7 @@ use crate::{ Kind, Tree, TreeRef, }; -/// The Error used in [`Tree::write_to()`]. +/// The Error used in [`Tree::write_to()`][crate::WriteTo::write_to()]. #[derive(Debug, thiserror::Error)] #[allow(missing_docs)] pub enum Error { diff --git a/git-packetline/src/decode.rs b/git-packetline/src/decode.rs index d82bc5e2dfa..bb3b0a2aa24 100644 --- a/git-packetline/src/decode.rs +++ b/git-packetline/src/decode.rs @@ -2,7 +2,7 @@ use bstr::BString; use crate::{PacketLineRef, DELIMITER_LINE, FLUSH_LINE, MAX_DATA_LEN, MAX_LINE_LEN, RESPONSE_END_LINE, U16_HEX_BYTES}; -/// The error used in the [`decode`][crate::decode] module +/// The error used in the [`decode`][mod@crate::decode] module #[derive(Debug, thiserror::Error)] #[allow(missing_docs)] pub enum Error { @@ -22,7 +22,7 @@ pub enum Error { /// pub mod band { - /// The error used in [`PacketLineRef::decode_band()`]. + /// The error used in [`PacketLineRef::decode_band()`][super::PacketLineRef::decode_band()]. #[derive(Debug, thiserror::Error)] #[allow(missing_docs)] pub enum Error { diff --git a/git-ref/src/store/file/find.rs b/git-ref/src/store/file/find.rs index 5fb0d5f5e35..6a84ec80aa2 100644 --- a/git-ref/src/store/file/find.rs +++ b/git-ref/src/store/file/find.rs @@ -309,7 +309,7 @@ pub mod existing { use crate::store_impl::file::find; - /// The error returned by [file::Store::find_existing()][crate::file::Store::find_existing()]. + /// The error returned by [file::Store::find_existing()][crate::file::Store::find()]. #[derive(Debug, thiserror::Error)] #[allow(missing_docs)] pub enum Error { diff --git a/git-ref/src/store/file/packed.rs b/git-ref/src/store/file/packed.rs index 5da1a13227c..51bb87cbe9b 100644 --- a/git-ref/src/store/file/packed.rs +++ b/git-ref/src/store/file/packed.rs @@ -51,7 +51,7 @@ pub mod transaction { use crate::store_impl::packed; - /// The error returned by [`file::Store::packed_transaction`][crate::file::Store::packed_transaction()]. + /// The error returned by [`file::Transaction::prepare()`][crate::file::Transaction::prepare()]. #[derive(Debug, thiserror::Error)] #[allow(missing_docs)] pub enum Error { diff --git a/git-ref/src/store/general/handle/find.rs b/git-ref/src/store/general/handle/find.rs index f775e995fc4..9792b9b7d59 100644 --- a/git-ref/src/store/general/handle/find.rs +++ b/git-ref/src/store/general/handle/find.rs @@ -5,7 +5,7 @@ use crate::{store, PartialNameRef, Reference}; mod error { use std::convert::Infallible; - /// The error returned by [crate::Store::find()]. + /// The error returned by [crate::file::Store::find_loose()]. #[derive(Debug, thiserror::Error)] #[allow(missing_docs)] pub enum Error { diff --git a/git-ref/src/store/packed/find.rs b/git-ref/src/store/packed/find.rs index 1e50fe73d39..1cce1eac3cb 100644 --- a/git-ref/src/store/packed/find.rs +++ b/git-ref/src/store/packed/find.rs @@ -128,7 +128,7 @@ pub use error::Error; /// pub mod existing { - /// The error returned by [`find_existing()`][super::packed::Buffer::find_existing()] + /// The error returned by [`find_existing()`][super::packed::Buffer::find()] #[derive(Debug, thiserror::Error)] #[allow(missing_docs)] pub enum Error { diff --git a/git-ref/src/store/packed/iter.rs b/git-ref/src/store/packed/iter.rs index 8029b5ddb53..e2b2dbe400b 100644 --- a/git-ref/src/store/packed/iter.rs +++ b/git-ref/src/store/packed/iter.rs @@ -103,7 +103,7 @@ impl<'a> packed::Iter<'a> { mod error { use git_object::bstr::BString; - /// The error returned by [`Iter::new(…)`][super::Iter::new()], + /// The error returned by [`Iter`][super::packed::Iter], #[derive(Debug, thiserror::Error)] #[allow(missing_docs)] pub enum Error { diff --git a/git-ref/src/store/packed/transaction.rs b/git-ref/src/store/packed/transaction.rs index 1470647eb1e..19f942a629c 100644 --- a/git-ref/src/store/packed/transaction.rs +++ b/git-ref/src/store/packed/transaction.rs @@ -229,7 +229,7 @@ pub(crate) fn buffer_into_transaction( /// pub mod prepare { - /// The error used in [`Transaction::prepare(…)`][super::packed::Transaction::prepare()]. + /// The error used in [`Transaction::prepare(…)`][crate::file::Transaction::prepare()]. #[derive(Debug, thiserror::Error)] #[allow(missing_docs)] pub enum Error { @@ -244,7 +244,7 @@ pub mod prepare { pub mod commit { use crate::store_impl::packed; - /// The error used in [`Transaction::commit(…)`][super::packed::Transaction::commit()]. + /// The error used in [`Transaction::commit(…)`][crate::file::Transaction::commit()]. #[derive(Debug, thiserror::Error)] #[allow(missing_docs)] pub enum Error { diff --git a/git-validate/src/tag.rs b/git-validate/src/tag.rs index 8e02f2b3e92..9ae4c3cd0b4 100644 --- a/git-validate/src/tag.rs +++ b/git-validate/src/tag.rs @@ -4,7 +4,7 @@ use bstr::BStr; pub mod name { use bstr::BString; - /// The error returned by [`name()`] + /// The error returned by [`name()`][super::name()]. #[derive(Debug, thiserror::Error)] #[allow(missing_docs)] pub enum Error { From 5ac6fee366953988103ac4f566bc1825e872593b Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 7 Sep 2022 11:05:06 +0800 Subject: [PATCH 11/23] fix journey tests (#450) --- .../porcelain/estimate-hours/invalid-branch-name-failure | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/snapshots/porcelain/estimate-hours/invalid-branch-name-failure b/tests/snapshots/porcelain/estimate-hours/invalid-branch-name-failure index 7242c2b493c..8a6045c0553 100644 --- a/tests/snapshots/porcelain/estimate-hours/invalid-branch-name-failure +++ b/tests/snapshots/porcelain/estimate-hours/invalid-branch-name-failure @@ -1 +1 @@ -Error: The ref partially named 'foobar' could not be found \ No newline at end of file +Error: The ref partially named "foobar" could not be found \ No newline at end of file From 140e6903d9bd9d9b393b717988f00e42c52a4d36 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 7 Sep 2022 11:22:27 +0800 Subject: [PATCH 12/23] fix windows tests (#450) Turns out backslashes are escaped, which can make it into the error and look strange there. --- git-ref/src/store/file/find.rs | 2 +- git-ref/src/store/file/overlay_iter.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/git-ref/src/store/file/find.rs b/git-ref/src/store/file/find.rs index 6a84ec80aa2..e9c9efedc5f 100644 --- a/git-ref/src/store/file/find.rs +++ b/git-ref/src/store/file/find.rs @@ -334,7 +334,7 @@ mod error { RefnameValidation(#[from] crate::name::Error), #[error("The ref file {path:?} could not be read in full")] ReadFileContents { source: io::Error, path: PathBuf }, - #[error("The reference at {relative_path:?} could not be instantiated")] + #[error("The reference at \"{relative_path}\" could not be instantiated")] ReferenceCreation { source: file::loose::reference::decode::Error, relative_path: PathBuf, diff --git a/git-ref/src/store/file/overlay_iter.rs b/git-ref/src/store/file/overlay_iter.rs index 782fca0df94..a971f19f747 100644 --- a/git-ref/src/store/file/overlay_iter.rs +++ b/git-ref/src/store/file/overlay_iter.rs @@ -420,7 +420,7 @@ mod error { Traversal(#[source] io::Error), #[error("The ref file {path:?} could not be read in full")] ReadFileContents { source: io::Error, path: PathBuf }, - #[error("The reference at {relative_path:?} could not be instantiated")] + #[error("The reference at \"{relative_path}\" could not be instantiated")] ReferenceCreation { source: file::loose::reference::decode::Error, relative_path: PathBuf, From 0f0bca3652f0e46dbebcb46956aa9c32e8812abe Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 11 Sep 2022 09:42:57 +0800 Subject: [PATCH 13/23] change!: Remove `Access` without replacement. (#450) It's a clear case of over-engineering and it didn't prove to be useful at all. --- Makefile | 3 +- git-sec/src/lib.rs | 135 ++++++++++++--------------------------------- 2 files changed, 37 insertions(+), 101 deletions(-) diff --git a/Makefile b/Makefile index e81fdf3dbf3..d13f4d52c7d 100644 --- a/Makefile +++ b/Makefile @@ -73,7 +73,8 @@ check: ## Build all code in suitable configurations && cargo check --features verbose-object-parsing-errors cd git-index && cargo check --features serde1 cd git-credentials && cargo check --features serde1 - cd git-sec && cargo check --features serde1 + cd git-sec && cargo check --features serde1 \ + cargo check --features thiserror cd git-revision && cargo check --features serde1 cd git-attributes && cargo check --features serde1 cd git-glob && cargo check --features serde1 diff --git a/git-sec/src/lib.rs b/git-sec/src/lib.rs index 81dcb8f6eed..e6f6c36bc2e 100644 --- a/git-sec/src/lib.rs +++ b/git-sec/src/lib.rs @@ -9,11 +9,7 @@ // `unsafe_code` not forbidden because we need to interact with the libc #![deny(missing_docs, rust_2018_idioms, unsafe_code)] -use std::{ - fmt::{Debug, Display, Formatter}, - marker::PhantomData, - ops::Deref, -}; +use std::fmt::{Display, Formatter}; /// A way to specify how 'safe' we feel about a resource, typically about a git repository. #[derive(Copy, Clone, Ord, PartialOrd, PartialEq, Eq, Debug, Hash)] @@ -85,47 +81,8 @@ pub mod trust { /// pub mod permission { - use std::fmt::{Debug, Display}; - - use crate::Access; - - /// A marker trait to signal tags for permissions. - pub trait Tag: Debug + Clone {} - - /// A tag indicating that a permission is applying to the contents of a configuration file. - #[derive(Debug, Clone)] - pub struct Config; - impl Tag for Config {} - - /// A tag indicating that a permission is applying to the resource itself. - #[derive(Debug, Clone)] - pub struct Resource; - impl Tag for Resource {} - - impl Access { - /// Create a permission for values contained in git configuration files. - /// - /// This applies permissions to values contained inside of these files. - pub fn config(permission: P) -> Self { - Access { - permission, - _data: Default::default(), - } - } - } - - impl Access { - /// Create a permission a file or directory itself. - /// - /// This applies permissions to a configuration file itself and whether it can be used at all, or to a directory - /// to read from or write to. - pub fn resource(permission: P) -> Self { - Access { - permission, - _data: Default::default(), - } - } - } + use crate::Permission; + use std::fmt::{Display, Formatter}; /// An error to use if an operation cannot proceed due to insufficient permissions. /// @@ -134,12 +91,42 @@ pub mod permission { #[cfg(feature = "thiserror")] #[derive(Debug, thiserror::Error)] #[error("Not allowed to handle resource {:?}: permission {}", .resource, .permission)] - pub struct Error { + pub struct Error { /// The resource which cannot be used. pub resource: R, /// The permission causing it to be disallowed. pub permission: P, } + + impl Permission { + /// Check this permissions and produce a reply to indicate if the `resource` can be used and in which way. + /// + /// Only if this permission is set to `Allow` will the resource be usable. + #[cfg(feature = "thiserror")] + pub fn check(&self, resource: R) -> Result, Error> { + match self { + Permission::Allow => Ok(Some(resource)), + Permission::Deny => Ok(None), + Permission::Forbid => Err(Error { + resource, + permission: *self, + }), + } + } + } + + impl Display for Permission { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + Display::fmt( + match self { + Permission::Allow => "allowed", + Permission::Deny => "denied", + Permission::Forbid => "forbidden", + }, + f, + ) + } + } } /// Allow, deny or forbid using a resource or performing an action. @@ -154,36 +141,6 @@ pub enum Permission { Allow, } -impl Permission { - /// Check this permissions and produce a reply to indicate if the `resource` can be used and in which way. - /// - /// Only if this permission is set to `Allow` will the resource be usable. - #[cfg(feature = "thiserror")] - pub fn check(&self, resource: R) -> Result, permission::Error> { - match self { - Permission::Allow => Ok(Some(resource)), - Permission::Deny => Ok(None), - Permission::Forbid => Err(permission::Error { - resource, - permission: *self, - }), - } - } -} - -impl Display for Permission { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - Display::fmt( - match self { - Permission::Allow => "allowed", - Permission::Deny => "denied", - Permission::Forbid => "forbidden", - }, - f, - ) - } -} - bitflags::bitflags! { /// Whether something can be read or written. #[cfg_attr(feature = "serde1", derive(serde::Serialize, serde::Deserialize))] @@ -197,29 +154,7 @@ bitflags::bitflags! { impl Display for ReadWrite { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - Debug::fmt(self, f) - } -} - -/// A container to define tagged access permissions, rendering the permission read-only. -#[derive(Debug, Clone)] -pub struct Access { - /// The access permission itself. - permission: P, - _data: PhantomData, -} - -impl Display for Access { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - Display::fmt(&self.permission, f) - } -} - -impl Deref for Access { - type Target = P; - - fn deref(&self) -> &Self::Target { - &self.permission + std::fmt::Debug::fmt(self, f) } } From 129bc87cadf2d69d565ccd643ea7a4c0d51e9737 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 11 Sep 2022 09:44:18 +0800 Subject: [PATCH 14/23] refactor (#450) --- git-sec/src/lib.rs | 110 ++------------------------------------ git-sec/src/permission.rs | 46 ++++++++++++++++ git-sec/src/trust.rs | 54 +++++++++++++++++++ 3 files changed, 104 insertions(+), 106 deletions(-) create mode 100644 git-sec/src/permission.rs create mode 100644 git-sec/src/trust.rs diff --git a/git-sec/src/lib.rs b/git-sec/src/lib.rs index e6f6c36bc2e..2f35d98cf7f 100644 --- a/git-sec/src/lib.rs +++ b/git-sec/src/lib.rs @@ -22,112 +22,7 @@ pub enum Trust { } /// -pub mod trust { - use crate::Trust; - - impl Trust { - /// Derive `Full` trust if `path` is owned by the user executing the current process, or `Reduced` trust otherwise. - pub fn from_path_ownership(path: impl AsRef) -> std::io::Result { - Ok(crate::identity::is_path_owned_by_current_user(path.as_ref())? - .then(|| Trust::Full) - .unwrap_or(Trust::Reduced)) - } - } - - /// A trait to help creating default values based on a trust level. - pub trait DefaultForLevel { - /// Produce a default value for the given trust `level`. - fn default_for_level(level: Trust) -> Self; - } - - /// Associate instructions for how to deal with various `Trust` levels as they are encountered in the wild. - pub struct Mapping { - /// The value for fully trusted resources. - pub full: T, - /// The value for resources with reduced trust. - pub reduced: T, - } - - impl Default for Mapping - where - T: DefaultForLevel, - { - fn default() -> Self { - Mapping { - full: T::default_for_level(Trust::Full), - reduced: T::default_for_level(Trust::Reduced), - } - } - } - - impl Mapping { - /// Obtain the value for the given trust `level`. - pub fn by_level(&self, level: Trust) -> &T { - match level { - Trust::Full => &self.full, - Trust::Reduced => &self.reduced, - } - } - - /// Obtain the value for the given `level` once. - pub fn into_value_by_level(self, level: Trust) -> T { - match level { - Trust::Full => self.full, - Trust::Reduced => self.reduced, - } - } - } -} - -/// -pub mod permission { - use crate::Permission; - use std::fmt::{Display, Formatter}; - - /// An error to use if an operation cannot proceed due to insufficient permissions. - /// - /// It's up to the implementation to decide which permission is required for an operation, and which one - /// causes errors. - #[cfg(feature = "thiserror")] - #[derive(Debug, thiserror::Error)] - #[error("Not allowed to handle resource {:?}: permission {}", .resource, .permission)] - pub struct Error { - /// The resource which cannot be used. - pub resource: R, - /// The permission causing it to be disallowed. - pub permission: P, - } - - impl Permission { - /// Check this permissions and produce a reply to indicate if the `resource` can be used and in which way. - /// - /// Only if this permission is set to `Allow` will the resource be usable. - #[cfg(feature = "thiserror")] - pub fn check(&self, resource: R) -> Result, Error> { - match self { - Permission::Allow => Ok(Some(resource)), - Permission::Deny => Ok(None), - Permission::Forbid => Err(Error { - resource, - permission: *self, - }), - } - } - } - - impl Display for Permission { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - Display::fmt( - match self { - Permission::Allow => "allowed", - Permission::Deny => "denied", - Permission::Forbid => "forbidden", - }, - f, - ) - } - } -} +pub mod trust; /// Allow, deny or forbid using a resource or performing an action. #[derive(Debug, Copy, Clone, PartialOrd, PartialEq, Ord, Eq, Hash)] @@ -141,6 +36,9 @@ pub enum Permission { Allow, } +/// +pub mod permission; + bitflags::bitflags! { /// Whether something can be read or written. #[cfg_attr(feature = "serde1", derive(serde::Serialize, serde::Deserialize))] diff --git a/git-sec/src/permission.rs b/git-sec/src/permission.rs new file mode 100644 index 00000000000..945341af5a7 --- /dev/null +++ b/git-sec/src/permission.rs @@ -0,0 +1,46 @@ +use crate::Permission; +use std::fmt::{Display, Formatter}; + +/// An error to use if an operation cannot proceed due to insufficient permissions. +/// +/// It's up to the implementation to decide which permission is required for an operation, and which one +/// causes errors. +#[cfg(feature = "thiserror")] +#[derive(Debug, thiserror::Error)] +#[error("Not allowed to handle resource {:?}: permission {}", .resource, .permission)] +pub struct Error { + /// The resource which cannot be used. + pub resource: R, + /// The permission causing it to be disallowed. + pub permission: P, +} + +impl Permission { + /// Check this permissions and produce a reply to indicate if the `resource` can be used and in which way. + /// + /// Only if this permission is set to `Allow` will the resource be usable. + #[cfg(feature = "thiserror")] + pub fn check(&self, resource: R) -> Result, Error> { + match self { + Permission::Allow => Ok(Some(resource)), + Permission::Deny => Ok(None), + Permission::Forbid => Err(Error { + resource, + permission: *self, + }), + } + } +} + +impl Display for Permission { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + Display::fmt( + match self { + Permission::Allow => "allowed", + Permission::Deny => "denied", + Permission::Forbid => "forbidden", + }, + f, + ) + } +} diff --git a/git-sec/src/trust.rs b/git-sec/src/trust.rs new file mode 100644 index 00000000000..cee480d7661 --- /dev/null +++ b/git-sec/src/trust.rs @@ -0,0 +1,54 @@ +use crate::Trust; + +impl Trust { + /// Derive `Full` trust if `path` is owned by the user executing the current process, or `Reduced` trust otherwise. + pub fn from_path_ownership(path: impl AsRef) -> std::io::Result { + Ok(crate::identity::is_path_owned_by_current_user(path.as_ref())? + .then(|| Trust::Full) + .unwrap_or(Trust::Reduced)) + } +} + +/// A trait to help creating default values based on a trust level. +pub trait DefaultForLevel { + /// Produce a default value for the given trust `level`. + fn default_for_level(level: Trust) -> Self; +} + +/// Associate instructions for how to deal with various `Trust` levels as they are encountered in the wild. +pub struct Mapping { + /// The value for fully trusted resources. + pub full: T, + /// The value for resources with reduced trust. + pub reduced: T, +} + +impl Default for Mapping +where + T: DefaultForLevel, +{ + fn default() -> Self { + Mapping { + full: T::default_for_level(Trust::Full), + reduced: T::default_for_level(Trust::Reduced), + } + } +} + +impl Mapping { + /// Obtain the value for the given trust `level`. + pub fn by_level(&self, level: Trust) -> &T { + match level { + Trust::Full => &self.full, + Trust::Reduced => &self.reduced, + } + } + + /// Obtain the value for the given `level` once. + pub fn into_value_by_level(self, level: Trust) -> T { + match level { + Trust::Full => self.full, + Trust::Reduced => self.reduced, + } + } +} From ac3823d7e0dcd1e51a373c9adca1a7476ca79003 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 11 Sep 2022 09:52:02 +0800 Subject: [PATCH 15/23] change!: remove `thiserror` optional feature. (#450) It's now included by default even though it's only used for a single error type. The reasoning is that `git-sec` is used within a tree that uses `thiserror` anyway, so no need to optimize compile times for the case where it doesn't. --- git-sec/Cargo.toml | 2 +- git-sec/src/permission.rs | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/git-sec/Cargo.toml b/git-sec/Cargo.toml index 3ef790a9b8a..d9deb21eb9b 100644 --- a/git-sec/Cargo.toml +++ b/git-sec/Cargo.toml @@ -18,7 +18,7 @@ serde1 = [ "serde" ] [dependencies] serde = { version = "1.0.114", optional = true, default-features = false, features = ["std", "derive"] } bitflags = "1.3.2" -thiserror = { version = "1.0.26", optional = true } +thiserror = "1.0.26" document-features = { version = "0.2.1", optional = true } diff --git a/git-sec/src/permission.rs b/git-sec/src/permission.rs index 945341af5a7..ef34c2e88fc 100644 --- a/git-sec/src/permission.rs +++ b/git-sec/src/permission.rs @@ -5,7 +5,6 @@ use std::fmt::{Display, Formatter}; /// /// It's up to the implementation to decide which permission is required for an operation, and which one /// causes errors. -#[cfg(feature = "thiserror")] #[derive(Debug, thiserror::Error)] #[error("Not allowed to handle resource {:?}: permission {}", .resource, .permission)] pub struct Error { @@ -19,7 +18,6 @@ impl Permission { /// Check this permissions and produce a reply to indicate if the `resource` can be used and in which way. /// /// Only if this permission is set to `Allow` will the resource be usable. - #[cfg(feature = "thiserror")] pub fn check(&self, resource: R) -> Result, Error> { match self { Permission::Allow => Ok(Some(resource)), From 515e52145b2bd0e484af232de4cd8450a1e7cbf2 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 11 Sep 2022 09:53:00 +0800 Subject: [PATCH 16/23] feat: `Permissions::check_opt()` for those who don't need an error case. (#450) --- git-sec/src/permission.rs | 9 +++++++++ git-sec/tests/sec.rs | 18 ++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/git-sec/src/permission.rs b/git-sec/src/permission.rs index ef34c2e88fc..ac8daae71d3 100644 --- a/git-sec/src/permission.rs +++ b/git-sec/src/permission.rs @@ -28,6 +28,15 @@ impl Permission { }), } } + + /// Like [`check()`][Self::check()], but degenerates the type to an option to make it more useful in cases where + /// `Forbid` shoudn't abort the entire operation. + pub fn check_opt(&self, resource: R) -> Option { + match self { + Permission::Allow => Some(resource), + Permission::Deny | Permission::Forbid => None, + } + } } impl Display for Permission { diff --git a/git-sec/tests/sec.rs b/git-sec/tests/sec.rs index 245e02b5e4a..129247aa731 100644 --- a/git-sec/tests/sec.rs +++ b/git-sec/tests/sec.rs @@ -9,4 +9,22 @@ mod trust { } } +mod permission { + use git_sec::Permission; + + #[test] + fn check() { + assert_eq!(Permission::Allow.check("hi").unwrap(), Some("hi")); + assert_eq!(Permission::Deny.check("hi").unwrap(), None); + assert!(Permission::Forbid.check("hi").is_err()); + } + + #[test] + fn check_opt() { + assert_eq!(Permission::Allow.check_opt("hi"), Some("hi")); + assert_eq!(Permission::Deny.check_opt("hi"), None); + assert_eq!(Permission::Forbid.check_opt("hi"), None); + } +} + mod identity; From 0ce21b1e95e00f2d4cebc1c62349e0dc66a1c705 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 11 Sep 2022 09:53:55 +0800 Subject: [PATCH 17/23] adapt to changes in `git-sec` (#450) --- git-discover/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git-discover/Cargo.toml b/git-discover/Cargo.toml index ed6e5e800cf..817a5d0e1f3 100644 --- a/git-discover/Cargo.toml +++ b/git-discover/Cargo.toml @@ -12,7 +12,7 @@ include = ["src/**/*", "CHANGELOG.md"] doctest = false [dependencies] -git-sec = { version = "^0.3.1", path = "../git-sec", features = ["thiserror"] } +git-sec = { version = "^0.3.1", path = "../git-sec" } git-path = { version = "^0.4.2", path = "../git-path" } git-ref = { version = "^0.15.2", path = "../git-ref" } git-hash = { version = "^0.9.9", path = "../git-hash" } From 51c721cccc4754e55ec9cb30344f75d7b07fc2a7 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 11 Sep 2022 09:59:34 +0800 Subject: [PATCH 18/23] change!: `permission::Error` with only one generic parameter (#450) As this error is only used for `Permission`, it's clear that it contains a `Permission` instance. --- git-sec/src/permission.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/git-sec/src/permission.rs b/git-sec/src/permission.rs index ac8daae71d3..b9968e2f42d 100644 --- a/git-sec/src/permission.rs +++ b/git-sec/src/permission.rs @@ -7,18 +7,18 @@ use std::fmt::{Display, Formatter}; /// causes errors. #[derive(Debug, thiserror::Error)] #[error("Not allowed to handle resource {:?}: permission {}", .resource, .permission)] -pub struct Error { +pub struct Error { /// The resource which cannot be used. pub resource: R, /// The permission causing it to be disallowed. - pub permission: P, + pub permission: Permission, } impl Permission { /// Check this permissions and produce a reply to indicate if the `resource` can be used and in which way. /// /// Only if this permission is set to `Allow` will the resource be usable. - pub fn check(&self, resource: R) -> Result, Error> { + pub fn check(&self, resource: R) -> Result, Error> { match self { Permission::Allow => Ok(Some(resource)), Permission::Deny => Ok(None), From b42a64dededae3a24dedc3bb42a097208c76afaa Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 11 Sep 2022 10:03:47 +0800 Subject: [PATCH 19/23] Manually implement `permission::Error` to save on `thiserror` dependency. (#450) This also removes the need for the `Permission` field as it's clear that only `Forbid` variants can produce it. --- git-sec/Cargo.toml | 1 - git-sec/src/permission.rs | 27 ++++++++++++++++++--------- 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/git-sec/Cargo.toml b/git-sec/Cargo.toml index d9deb21eb9b..72cce761967 100644 --- a/git-sec/Cargo.toml +++ b/git-sec/Cargo.toml @@ -18,7 +18,6 @@ serde1 = [ "serde" ] [dependencies] serde = { version = "1.0.114", optional = true, default-features = false, features = ["std", "derive"] } bitflags = "1.3.2" -thiserror = "1.0.26" document-features = { version = "0.2.1", optional = true } diff --git a/git-sec/src/permission.rs b/git-sec/src/permission.rs index b9968e2f42d..b6da1010d1d 100644 --- a/git-sec/src/permission.rs +++ b/git-sec/src/permission.rs @@ -1,19 +1,31 @@ use crate::Permission; -use std::fmt::{Display, Formatter}; +use std::fmt::{Debug, Display, Formatter}; /// An error to use if an operation cannot proceed due to insufficient permissions. /// /// It's up to the implementation to decide which permission is required for an operation, and which one /// causes errors. -#[derive(Debug, thiserror::Error)] -#[error("Not allowed to handle resource {:?}: permission {}", .resource, .permission)] +#[derive(Debug)] pub struct Error { /// The resource which cannot be used. pub resource: R, - /// The permission causing it to be disallowed. - pub permission: Permission, } +impl Display for Error +where + R: std::fmt::Debug, +{ + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + write!( + f, + "Not allowed to handle resource {:?}: permission denied", + self.resource + ) + } +} + +impl std::error::Error for Error where R: std::fmt::Debug {} + impl Permission { /// Check this permissions and produce a reply to indicate if the `resource` can be used and in which way. /// @@ -22,10 +34,7 @@ impl Permission { match self { Permission::Allow => Ok(Some(resource)), Permission::Deny => Ok(None), - Permission::Forbid => Err(Error { - resource, - permission: *self, - }), + Permission::Forbid => Err(Error { resource }), } } From 33f8b9292946329ca2b24c5f0b2877db9afa2a15 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 11 Sep 2022 10:04:59 +0800 Subject: [PATCH 20/23] change!: remove `Perrmission::fmt()` (Display) (#450) It was somewhat specific to being printed in the error scenario, and not general purpose at all. --- git-sec/src/permission.rs | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/git-sec/src/permission.rs b/git-sec/src/permission.rs index b6da1010d1d..c6c2eed3a6f 100644 --- a/git-sec/src/permission.rs +++ b/git-sec/src/permission.rs @@ -47,16 +47,3 @@ impl Permission { } } } - -impl Display for Permission { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - Display::fmt( - match self { - Permission::Allow => "allowed", - Permission::Deny => "denied", - Permission::Forbid => "forbidden", - }, - f, - ) - } -} From fe24b41bae244884f1f2cea43af11ab27976b9bc Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 11 Sep 2022 10:09:19 +0800 Subject: [PATCH 21/23] feat: `Permission::is_allowed()` as convenience method (#450) --- git-sec/src/permission.rs | 4 ++++ git-sec/tests/sec.rs | 7 +++++++ 2 files changed, 11 insertions(+) diff --git a/git-sec/src/permission.rs b/git-sec/src/permission.rs index c6c2eed3a6f..a4d2617ac6f 100644 --- a/git-sec/src/permission.rs +++ b/git-sec/src/permission.rs @@ -27,6 +27,10 @@ where impl std::error::Error for Error where R: std::fmt::Debug {} impl Permission { + /// Return true if this instance is `Permission::Allow`. + pub fn is_allowed(&self) -> bool { + matches!(self, Permission::Allow) + } /// Check this permissions and produce a reply to indicate if the `resource` can be used and in which way. /// /// Only if this permission is set to `Allow` will the resource be usable. diff --git a/git-sec/tests/sec.rs b/git-sec/tests/sec.rs index 129247aa731..ce9c24122ab 100644 --- a/git-sec/tests/sec.rs +++ b/git-sec/tests/sec.rs @@ -25,6 +25,13 @@ mod permission { assert_eq!(Permission::Deny.check_opt("hi"), None); assert_eq!(Permission::Forbid.check_opt("hi"), None); } + + #[test] + fn is_allowed() { + assert!(Permission::Allow.is_allowed()); + assert!(!Permission::Deny.is_allowed()); + assert!(!Permission::Forbid.is_allowed()); + } } mod identity; From d6ef2ce9d0a7883d7d3b5ddb0010c8ab3d26bb78 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 11 Sep 2022 10:11:30 +0800 Subject: [PATCH 22/23] adjust to changes in `git-sec` (#450) --- Cargo.lock | 1 - git-repository/Cargo.toml | 2 +- git-repository/src/config/cache/init.rs | 2 +- git-repository/src/config/cache/mod.rs | 4 +-- git-repository/src/config/mod.rs | 8 +++--- .../src/config/snapshot/credential_helpers.rs | 4 +-- git-repository/src/lib.rs | 6 +--- .../src/remote/url/scheme_permission.rs | 7 ++--- git-repository/src/repository/identity.rs | 4 +-- git-repository/src/repository/permissions.rs | 28 +++++++++---------- git-repository/src/worktree/mod.rs | 2 +- git-repository/tests/remote/connect.rs | 2 +- .../tests/repository/config/identity.rs | 6 ++-- 13 files changed, 33 insertions(+), 43 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 49202d8db74..6e60fd29c92 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1649,7 +1649,6 @@ dependencies = [ "libc", "serde", "tempfile", - "thiserror", "windows 0.37.0", ] diff --git a/git-repository/Cargo.toml b/git-repository/Cargo.toml index 3297a66c7d3..f184b8a6c09 100644 --- a/git-repository/Cargo.toml +++ b/git-repository/Cargo.toml @@ -82,7 +82,7 @@ git-discover = { version = "^0.4.2", path = "../git-discover" } git-tempfile = { version = "^2.0.0", path = "../git-tempfile" } git-lock = { version = "^2.0.0", path = "../git-lock" } git-validate = { version = "^0.5.5", path = "../git-validate" } -git-sec = { version = "^0.3.1", path = "../git-sec", features = ["thiserror"] } +git-sec = { version = "^0.3.1", path = "../git-sec" } git-date = { version = "^0.1.0", path = "../git-date" } git-refspec = { version = "^0.1.1", path = "../git-refspec" } diff --git a/git-repository/src/config/cache/init.rs b/git-repository/src/config/cache/init.rs index aa184cb3b79..9350ec7cffb 100644 --- a/git-repository/src/config/cache/init.rs +++ b/git-repository/src/config/cache/init.rs @@ -171,7 +171,7 @@ impl Cache { pub fn xdg_config_path( &self, resource_file_name: &str, - ) -> Result, git_sec::permission::Error> { + ) -> Result, git_sec::permission::Error> { std::env::var_os("XDG_CONFIG_HOME") .map(|path| (path, &self.xdg_config_home_env)) .or_else(|| std::env::var_os("HOME").map(|path| (path, &self.home_env))) diff --git a/git-repository/src/config/cache/mod.rs b/git-repository/src/config/cache/mod.rs index 40e92f5289f..eeae0c20426 100644 --- a/git-repository/src/config/cache/mod.rs +++ b/git-repository/src/config/cache/mod.rs @@ -16,7 +16,7 @@ impl std::fmt::Debug for Cache { impl Cache { pub(crate) fn personas(&self) -> &identity::Personas { self.personas - .get_or_init(|| identity::Personas::from_config_and_env(&self.resolved, &self.git_prefix)) + .get_or_init(|| identity::Personas::from_config_and_env(&self.resolved, self.git_prefix)) } pub(crate) fn url_rewrite(&self) -> &remote::url::Rewrite { @@ -29,7 +29,7 @@ impl Cache { &self, ) -> Result<&remote::url::SchemePermission, remote::url::scheme_permission::init::Error> { self.url_scheme.get_or_try_init(|| { - remote::url::SchemePermission::from_config(&self.resolved, &self.git_prefix, self.filter_config_section) + remote::url::SchemePermission::from_config(&self.resolved, self.git_prefix, self.filter_config_section) }) } } diff --git a/git-repository/src/config/mod.rs b/git-repository/src/config/mod.rs index f328c6c31a8..ef3e30bc439 100644 --- a/git-repository/src/config/mod.rs +++ b/git-repository/src/config/mod.rs @@ -1,7 +1,7 @@ pub use git_config::*; use git_features::threading::OnceCell; -use crate::{bstr::BString, permission, remote, repository::identity, revision::spec, Repository}; +use crate::{bstr::BString, remote, repository::identity, revision::spec, Repository}; pub(crate) mod cache; mod snapshot; @@ -84,10 +84,10 @@ pub(crate) struct Cache { /// The path to the user-level excludes file to ignore certain files in the worktree. pub excludes_file: Option, /// Define how we can use values obtained with `xdg_config(…)` and its `XDG_CONFIG_HOME` variable. - xdg_config_home_env: permission::env_var::Resource, + xdg_config_home_env: git_sec::Permission, /// Define how we can use values obtained with `xdg_config(…)`. and its `HOME` variable. - home_env: permission::env_var::Resource, + home_env: git_sec::Permission, /// How to use git-prefixed environment variables - git_prefix: permission::env_var::Resource, + git_prefix: git_sec::Permission, // TODO: make core.precomposeUnicode available as well. } diff --git a/git-repository/src/config/snapshot/credential_helpers.rs b/git-repository/src/config/snapshot/credential_helpers.rs index 4429c7361f4..0414dd3757e 100644 --- a/git-repository/src/config/snapshot/credential_helpers.rs +++ b/git-repository/src/config/snapshot/credential_helpers.rs @@ -120,8 +120,8 @@ impl Snapshot<'_> { } } - let allow_git_env = *self.repo.options.permissions.env.git_prefix == git_sec::Permission::Allow; - let allow_ssh_env = *self.repo.options.permissions.env.ssh_prefix == git_sec::Permission::Allow; + let allow_git_env = self.repo.options.permissions.env.git_prefix.is_allowed(); + let allow_ssh_env = self.repo.options.permissions.env.ssh_prefix.is_allowed(); let prompt_options = git_prompt::Options::default().apply_environment(allow_git_env, allow_ssh_env, allow_git_env); Ok(( diff --git a/git-repository/src/lib.rs b/git-repository/src/lib.rs index 548da38d407..8c957e1fdfc 100644 --- a/git-repository/src/lib.rs +++ b/git-repository/src/lib.rs @@ -234,14 +234,10 @@ pub fn open_opts(directory: impl Into, options: open::Option pub mod permission { /// pub mod env_var { - use git_sec::{permission, Access}; - - /// A permission to control access to the resource pointed to an environment variable. - pub type Resource = Access; /// pub mod resource { /// - pub type Error = git_sec::permission::Error; + pub type Error = git_sec::permission::Error; } } } diff --git a/git-repository/src/remote/url/scheme_permission.rs b/git-repository/src/remote/url/scheme_permission.rs index b3d09c3c0da..fa81b5aa7fc 100644 --- a/git-repository/src/remote/url/scheme_permission.rs +++ b/git-repository/src/remote/url/scheme_permission.rs @@ -2,10 +2,7 @@ use std::{borrow::Cow, collections::BTreeMap, convert::TryFrom}; -use crate::{ - bstr::{BStr, BString, ByteSlice}, - permission, -}; +use crate::bstr::{BStr, BString, ByteSlice}; /// pub mod init { @@ -62,7 +59,7 @@ pub(crate) struct SchemePermission { impl SchemePermission { pub fn from_config( config: &git_config::File<'static>, - git_prefix: &permission::env_var::Resource, + git_prefix: git_sec::Permission, mut filter: fn(&git_config::file::Metadata) -> bool, ) -> Result { let allow: Option = config diff --git a/git-repository/src/repository/identity.rs b/git-repository/src/repository/identity.rs index db2e3662099..98759a13de2 100644 --- a/git-repository/src/repository/identity.rs +++ b/git-repository/src/repository/identity.rs @@ -1,6 +1,6 @@ use std::{borrow::Cow, time::SystemTime}; -use crate::{bstr::BString, permission}; +use crate::bstr::BString; /// Identity handling. impl crate::Repository { @@ -98,7 +98,7 @@ pub(crate) struct Personas { } impl Personas { - pub fn from_config_and_env(config: &git_config::File<'_>, git_env: &permission::env_var::Resource) -> Self { + pub fn from_config_and_env(config: &git_config::File<'_>, git_env: git_sec::Permission) -> Self { fn env_var(name: &str) -> Option { std::env::var_os(name).map(|value| git_path::into_bstr(Cow::Owned(value.into())).into_owned()) } diff --git a/git-repository/src/repository/permissions.rs b/git-repository/src/repository/permissions.rs index 079d3fb399b..8cfbcf26269 100644 --- a/git-repository/src/repository/permissions.rs +++ b/git-repository/src/repository/permissions.rs @@ -1,6 +1,4 @@ -use git_sec::{Access, Trust}; - -use crate::permission; +use git_sec::Trust; /// Permissions associated with various resources of a git repository #[derive(Debug, Clone)] @@ -67,23 +65,23 @@ pub struct Environment { /// Control whether resources pointed to by `XDG_CONFIG_HOME` can be used when looking up common configuration values. /// /// Note that [`git_sec::Permission::Forbid`] will cause the operation to abort if a resource is set via the XDG config environment. - pub xdg_config_home: permission::env_var::Resource, + pub xdg_config_home: git_sec::Permission, /// Control the way resources pointed to by the home directory (similar to `xdg_config_home`) may be used. - pub home: permission::env_var::Resource, + pub home: git_sec::Permission, /// Control if resources pointed to by `GIT_*` prefixed environment variables can be used. - pub git_prefix: permission::env_var::Resource, + pub git_prefix: git_sec::Permission, /// Control if resources pointed to by `SSH_*` prefixed environment variables can be used (like `SSH_ASKPASS`) - pub ssh_prefix: permission::env_var::Resource, + pub ssh_prefix: git_sec::Permission, } impl Environment { /// Allow access to the entire environment. pub fn all() -> Self { Environment { - xdg_config_home: Access::resource(git_sec::Permission::Allow), - home: Access::resource(git_sec::Permission::Allow), - git_prefix: Access::resource(git_sec::Permission::Allow), - ssh_prefix: Access::resource(git_sec::Permission::Allow), + xdg_config_home: git_sec::Permission::Allow, + home: git_sec::Permission::Allow, + git_prefix: git_sec::Permission::Allow, + ssh_prefix: git_sec::Permission::Allow, } } } @@ -122,11 +120,11 @@ impl Permissions { includes: false, }, env: { - let deny = permission::env_var::Resource::resource(git_sec::Permission::Deny); + let deny = git_sec::Permission::Deny; Environment { - xdg_config_home: deny.clone(), - home: deny.clone(), - ssh_prefix: deny.clone(), + xdg_config_home: deny, + home: deny, + ssh_prefix: deny, git_prefix: deny, } }, diff --git a/git-repository/src/worktree/mod.rs b/git-repository/src/worktree/mod.rs index 49d73e8d962..1213aa04cca 100644 --- a/git-repository/src/worktree/mod.rs +++ b/git-repository/src/worktree/mod.rs @@ -111,7 +111,7 @@ pub mod excludes { #[error("Could not read repository exclude.")] Io(#[from] std::io::Error), #[error(transparent)] - EnvironmentPermission(#[from] git_sec::permission::Error), + EnvironmentPermission(#[from] git_sec::permission::Error), } impl<'repo> crate::Worktree<'repo> { diff --git a/git-repository/tests/remote/connect.rs b/git-repository/tests/remote/connect.rs index 791ce976bd5..265682804c2 100644 --- a/git-repository/tests/remote/connect.rs +++ b/git-repository/tests/remote/connect.rs @@ -32,7 +32,7 @@ mod blocking_io { remote::repo("protocol_file_user").git_dir(), git::open::Options::isolated().permissions(git::Permissions { env: git::permissions::Environment { - git_prefix: git_sec::Access::resource(git_sec::Permission::Allow), + git_prefix: git_sec::Permission::Allow, ..git::permissions::Environment::all() }, ..git::Permissions::isolated() diff --git a/git-repository/tests/repository/config/identity.rs b/git-repository/tests/repository/config/identity.rs index 43a601878d4..54db5182c29 100644 --- a/git-repository/tests/repository/config/identity.rs +++ b/git-repository/tests/repository/config/identity.rs @@ -1,7 +1,7 @@ use std::path::Path; use git_repository as git; -use git_sec::{Access, Permission}; +use git_sec::Permission; use git_testtools::Env; use serial_test::serial; @@ -28,8 +28,8 @@ fn author_and_committer_and_fallback() { repo.git_dir(), repo.open_options().clone().with(trust).permissions(git::Permissions { env: git::permissions::Environment { - xdg_config_home: Access::resource(Permission::Deny), - home: Access::resource(Permission::Deny), + xdg_config_home: Permission::Deny, + home: Permission::Deny, ..git::permissions::Environment::all() }, ..Default::default() From 9e4e4c4fbe0189951eeac0ed8cbdcfcd409a9f6c Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 11 Sep 2022 10:13:59 +0800 Subject: [PATCH 23/23] refactor (#450) --- Makefile | 3 +-- git-repository/src/config/cache/init.rs | 4 ++-- git-repository/src/open.rs | 2 +- git-repository/src/remote/url/scheme_permission.rs | 2 +- 4 files changed, 5 insertions(+), 6 deletions(-) diff --git a/Makefile b/Makefile index d13f4d52c7d..e81fdf3dbf3 100644 --- a/Makefile +++ b/Makefile @@ -73,8 +73,7 @@ check: ## Build all code in suitable configurations && cargo check --features verbose-object-parsing-errors cd git-index && cargo check --features serde1 cd git-credentials && cargo check --features serde1 - cd git-sec && cargo check --features serde1 \ - cargo check --features thiserror + cd git-sec && cargo check --features serde1 cd git-revision && cargo check --features serde1 cd git-attributes && cargo check --features serde1 cd git-glob && cargo check --features serde1 diff --git a/git-repository/src/config/cache/init.rs b/git-repository/src/config/cache/init.rs index 9350ec7cffb..4e5ab68203c 100644 --- a/git-repository/src/config/cache/init.rs +++ b/git-repository/src/config/cache/init.rs @@ -80,7 +80,7 @@ impl Cache { "HOME" => Some(home_env), _ => None, } - .and_then(|perm| std::env::var_os(name).and_then(|val| perm.check(val).ok().flatten())) + .and_then(|perm| std::env::var_os(name).and_then(|val| perm.check_opt(val))) }) .map(|p| (source, p.into_owned())) } @@ -189,6 +189,6 @@ impl Cache { pub fn home_dir(&self) -> Option { std::env::var_os("HOME") .map(PathBuf::from) - .and_then(|path| self.home_env.check(path).ok().flatten()) + .and_then(|path| self.home_env.check_opt(path)) } } diff --git a/git-repository/src/open.rs b/git-repository/src/open.rs index db8a877fad9..a3c7048082c 100644 --- a/git-repository/src/open.rs +++ b/git-repository/src/open.rs @@ -356,7 +356,7 @@ impl ThreadSafeRepository { let git_install_dir = crate::path::install_dir().ok(); let home = std::env::var_os("HOME") .map(PathBuf::from) - .and_then(|home| env.home.check(home).ok().flatten()); + .and_then(|home| env.home.check_opt(home)); let mut filter_config_section = filter_config_section.unwrap_or(config::section::is_trusted); let config = config::Cache::from_stage_one( diff --git a/git-repository/src/remote/url/scheme_permission.rs b/git-repository/src/remote/url/scheme_permission.rs index fa81b5aa7fc..4493d175dbf 100644 --- a/git-repository/src/remote/url/scheme_permission.rs +++ b/git-repository/src/remote/url/scheme_permission.rs @@ -103,7 +103,7 @@ impl SchemePermission { let user_allowed = saw_user.then(|| { std::env::var_os("GIT_PROTOCOL_FROM_USER") - .and_then(|val| git_prefix.check(val).ok().flatten()) + .and_then(|val| git_prefix.check_opt(val)) .map_or(true, |val| val == "1") }); Ok(SchemePermission {