Merge branch 'error'

Byron · Byron · commit c372321dd6ea · 2023-11-11T12:31:37.000+01:00
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/gix-command/Cargo.toml b/gix-command/Cargo.toml
@@ -13,6 +13,8 @@ include = ["src/lib.rs", "LICENSE-*"]
 doctest = false
 
 [dependencies]
+gix-trace = { version = "^0.1.3", path = "../gix-trace" }
+
 bstr = { version = "1.5.0", default-features = false, features = ["std"] }
 
 [dev-dependencies]
diff --git a/gix-command/src/lib.rs b/gix-command/src/lib.rs
@@ -93,7 +93,9 @@ mod prepare {
     impl Prepare {
         /// Spawn the command as configured.
         pub fn spawn(self) -> std::io::Result<std::process::Child> {
-            Command::from(self).spawn()
+            let mut cmd = Command::from(self);
+            gix_trace::debug!(cmd = ?cmd);
+            cmd.spawn()
         }
     }
 
diff --git a/gix-credentials/Cargo.toml b/gix-credentials/Cargo.toml
@@ -23,6 +23,7 @@ gix-path = { version = "^0.10.0", path = "../gix-path" }
 gix-command = { version = "^0.2.10", path = "../gix-command" }
 gix-config-value = { version = "^0.14.0", path = "../gix-config-value" }
 gix-prompt = { version = "^0.7.0", path = "../gix-prompt" }
+gix-trace = { version = "^0.1.3", path = "../gix-trace" }
 
 thiserror = "1.0.32"
 serde = { version = "1.0.114", optional = true, default-features = false, features = ["derive"] }
diff --git a/gix-credentials/src/program/mod.rs b/gix-credentials/src/program/mod.rs
@@ -106,6 +106,7 @@ impl Program {
                 Stdio::null()
             })
             .stderr(if self.stderr { Stdio::inherit() } else { Stdio::null() });
+        gix_trace::debug!(cmd = ?cmd, "launching credential helper");
         let mut child = cmd.spawn()?;
         let stdin = child.stdin.take().expect("stdin to be configured");
         let stdout = child.stdout.take();
diff --git a/gix-object/src/commit/mod.rs b/gix-object/src/commit/mod.rs
@@ -1,6 +1,7 @@
 use std::ops::Range;
 
 use bstr::{BStr, BString, ByteSlice};
+use winnow::prelude::*;
 
 use crate::{Commit, CommitRef, TagRef};
 
@@ -60,7 +61,11 @@ mod write;
 impl<'a> CommitRef<'a> {
     /// Deserialize a commit from the given `data` bytes while avoiding most allocations.
     pub fn from_bytes(mut data: &'a [u8]) -> Result<CommitRef<'a>, crate::decode::Error> {
-        decode::commit(&mut data).map_err(crate::decode::Error::with_err)
+        let input = &mut data;
+        match decode::commit.parse_next(input) {
+            Ok(tag) => Ok(tag),
+            Err(err) => Err(crate::decode::Error::with_err(err, input)),
+        }
     }
 }
 
diff --git a/gix-object/src/commit/ref_iter.rs b/gix-object/src/commit/ref_iter.rs
@@ -154,44 +154,42 @@ fn missing_field() -> crate::decode::Error {
 
 impl<'a> CommitRefIter<'a> {
     #[inline]
-    fn next_inner(i: &'a [u8], state: &mut State) -> Result<(&'a [u8], Token<'a>), crate::decode::Error> {
-        Self::next_inner_(i, state).map_err(crate::decode::Error::with_err)
+    fn next_inner(mut i: &'a [u8], state: &mut State) -> Result<(&'a [u8], Token<'a>), crate::decode::Error> {
+        let input = &mut i;
+        match Self::next_inner_(input, state) {
+            Ok(token) => Ok((*input, token)),
+            Err(err) => Err(crate::decode::Error::with_err(err, input)),
+        }
     }
 
     fn next_inner_(
-        mut i: &'a [u8],
+        input: &mut &'a [u8],
         state: &mut State,
-    ) -> Result<(&'a [u8], Token<'a>), winnow::error::ErrMode<crate::decode::ParseError>> {
+    ) -> Result<Token<'a>, winnow::error::ErrMode<crate::decode::ParseError>> {
         use State::*;
         Ok(match state {
             Tree => {
                 let tree = (|i: &mut _| parse::header_field(i, b"tree", parse::hex_hash))
                     .context(StrContext::Expected("tree <40 lowercase hex char>".into()))
-                    .parse_next(&mut i)?;
+                    .parse_next(input)?;
                 *state = State::Parents;
-                (
-                    i,
-                    Token::Tree {
-                        id: ObjectId::from_hex(tree).expect("parsing validation"),
-                    },
-                )
+                Token::Tree {
+                    id: ObjectId::from_hex(tree).expect("parsing validation"),
+                }
             }
             Parents => {
                 let parent = opt(|i: &mut _| parse::header_field(i, b"parent", parse::hex_hash))
                     .context(StrContext::Expected("commit <40 lowercase hex char>".into()))
-                    .parse_next(&mut i)?;
+                    .parse_next(input)?;
                 match parent {
-                    Some(parent) => (
-                        i,
-                        Token::Parent {
-                            id: ObjectId::from_hex(parent).expect("parsing validation"),
-                        },
-                    ),
+                    Some(parent) => Token::Parent {
+                        id: ObjectId::from_hex(parent).expect("parsing validation"),
+                    },
                     None => {
                         *state = State::Signature {
                             of: SignatureKind::Author,
                         };
-                        return Self::next_inner_(i, state);
+                        Self::next_inner_(input, state)?
                     }
                 }
             }
@@ -209,23 +207,20 @@ impl<'a> CommitRefIter<'a> {
                 };
                 let signature = (|i: &mut _| parse::header_field(i, field_name, parse::signature))
                     .context(StrContext::Expected(err_msg.into()))
-                    .parse_next(&mut i)?;
-                (
-                    i,
-                    match who {
-                        SignatureKind::Author => Token::Author { signature },
-                        SignatureKind::Committer => Token::Committer { signature },
-                    },
-                )
+                    .parse_next(input)?;
+                match who {
+                    SignatureKind::Author => Token::Author { signature },
+                    SignatureKind::Committer => Token::Committer { signature },
+                }
             }
             Encoding => {
                 let encoding = opt(|i: &mut _| parse::header_field(i, b"encoding", take_till1(NL)))
                     .context(StrContext::Expected("encoding <encoding>".into()))
-                    .parse_next(&mut i)?;
+                    .parse_next(input)?;
                 *state = State::ExtraHeaders;
                 match encoding {
-                    Some(encoding) => (i, Token::Encoding(encoding.as_bstr())),
-                    None => return Self::next_inner_(i, state),
+                    Some(encoding) => Token::Encoding(encoding.as_bstr()),
+                    None => Self::next_inner_(input, state)?,
                 }
             }
             ExtraHeaders => {
@@ -237,22 +232,22 @@ impl<'a> CommitRefIter<'a> {
                     },
                 )))
                 .context(StrContext::Expected("<field> <single-line|multi-line>".into()))
-                .parse_next(&mut i)?;
+                .parse_next(input)?;
                 match extra_header {
-                    Some(extra_header) => (i, Token::ExtraHeader(extra_header)),
+                    Some(extra_header) => Token::ExtraHeader(extra_header),
                     None => {
                         *state = State::Message;
-                        return Self::next_inner_(i, state);
+                        Self::next_inner_(input, state)?
                     }
                 }
             }
             Message => {
-                let message = terminated(decode::message, eof).parse_next(&mut i)?;
+                let message = terminated(decode::message, eof).parse_next(input)?;
                 debug_assert!(
-                    i.is_empty(),
+                    input.is_empty(),
                     "we should have consumed all data - otherwise iter may go forever"
                 );
-                return Ok((i, Token::Message(message)));
+                Token::Message(message)
             }
         })
     }
diff --git a/gix-object/src/lib.rs b/gix-object/src/lib.rs
@@ -267,6 +267,7 @@ pub mod decode {
         pub(crate) fn empty_error() -> Error {
             Error {
                 inner: winnow::error::ContextError::new(),
+                remaining: Default::default(),
             }
         }
 
@@ -275,19 +276,27 @@ pub mod decode {
         pub struct Error {
             /// The actual error
             pub inner: ParseError,
+            /// Where the error occurred
+            pub remaining: Vec<u8>,
         }
 
         impl Error {
-            pub(crate) fn with_err(err: winnow::error::ErrMode<ParseError>) -> Self {
+            pub(crate) fn with_err(err: winnow::error::ErrMode<ParseError>, remaining: &[u8]) -> Self {
                 Self {
                     inner: err.into_inner().expect("we don't have streaming parsers"),
+                    remaining: remaining.to_owned(),
                 }
             }
         }
 
         impl std::fmt::Display for Error {
             fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-                self.inner.fmt(f)
+                write!(f, "object parsing failed at `{}`", bstr::BStr::new(&self.remaining))?;
+                if self.inner.context().next().is_some() {
+                    writeln!(f)?;
+                    self.inner.fmt(f)?;
+                }
+                Ok(())
             }
         }
 
@@ -316,7 +325,7 @@ pub mod decode {
         }
 
         impl Error {
-            pub(crate) fn with_err(err: winnow::error::ErrMode<ParseError>) -> Self {
+            pub(crate) fn with_err(err: winnow::error::ErrMode<ParseError>, _remaining: &[u8]) -> Self {
                 Self {
                     inner: err.into_inner().expect("we don't have streaming parsers"),
                 }
diff --git a/gix-object/src/tag/mod.rs b/gix-object/src/tag/mod.rs
@@ -1,3 +1,5 @@
+use winnow::prelude::*;
+
 use crate::TagRef;
 
 mod decode;
@@ -11,7 +13,11 @@ pub mod ref_iter;
 impl<'a> TagRef<'a> {
     /// Deserialize a tag from `data`.
     pub fn from_bytes(mut data: &'a [u8]) -> Result<TagRef<'a>, crate::decode::Error> {
-        decode::git_tag(&mut data).map_err(crate::decode::Error::with_err)
+        let input = &mut data;
+        match decode::git_tag.parse_next(input) {
+            Ok(tag) => Ok(tag),
+            Err(err) => Err(crate::decode::Error::with_err(err, input)),
+        }
     }
     /// The object this tag points to as `Id`.
     pub fn target(&self) -> gix_hash::ObjectId {
diff --git a/gix-object/src/tag/ref_iter.rs b/gix-object/src/tag/ref_iter.rs
@@ -59,58 +59,59 @@ fn missing_field() -> crate::decode::Error {
 
 impl<'a> TagRefIter<'a> {
     #[inline]
-    fn next_inner(i: &'a [u8], state: &mut State) -> Result<(&'a [u8], Token<'a>), crate::decode::Error> {
-        Self::next_inner_(i, state).map_err(crate::decode::Error::with_err)
+    fn next_inner(mut i: &'a [u8], state: &mut State) -> Result<(&'a [u8], Token<'a>), crate::decode::Error> {
+        let input = &mut i;
+        match Self::next_inner_(input, state) {
+            Ok(token) => Ok((*input, token)),
+            Err(err) => Err(crate::decode::Error::with_err(err, input)),
+        }
     }
 
     fn next_inner_(
-        mut i: &'a [u8],
+        input: &mut &'a [u8],
         state: &mut State,
-    ) -> Result<(&'a [u8], Token<'a>), winnow::error::ErrMode<crate::decode::ParseError>> {
+    ) -> Result<Token<'a>, winnow::error::ErrMode<crate::decode::ParseError>> {
         use State::*;
         Ok(match state {
             Target => {
                 let target = (|i: &mut _| parse::header_field(i, b"object", parse::hex_hash))
                     .context(StrContext::Expected("object <40 lowercase hex char>".into()))
-                    .parse_next(&mut i)?;
+                    .parse_next(input)?;
                 *state = TargetKind;
-                (
-                    i,
-                    Token::Target {
-                        id: ObjectId::from_hex(target).expect("parsing validation"),
-                    },
-                )
+                Token::Target {
+                    id: ObjectId::from_hex(target).expect("parsing validation"),
+                }
             }
             TargetKind => {
                 let kind = (|i: &mut _| parse::header_field(i, b"type", take_while(1.., AsChar::is_alpha)))
                     .context(StrContext::Expected("type <object kind>".into()))
-                    .parse_next(&mut i)?;
+                    .parse_next(input)?;
                 let kind = Kind::from_bytes(kind)
-                    .map_err(|_| winnow::error::ErrMode::from_error_kind(&i, winnow::error::ErrorKind::Verify))?;
+                    .map_err(|_| winnow::error::ErrMode::from_error_kind(input, winnow::error::ErrorKind::Verify))?;
                 *state = Name;
-                (i, Token::TargetKind(kind))
+                Token::TargetKind(kind)
             }
             Name => {
                 let tag_version = (|i: &mut _| parse::header_field(i, b"tag", take_while(1.., |b| b != NL[0])))
                     .context(StrContext::Expected("tag <version>".into()))
-                    .parse_next(&mut i)?;
+                    .parse_next(input)?;
                 *state = Tagger;
-                (i, Token::Name(tag_version.as_bstr()))
+                Token::Name(tag_version.as_bstr())
             }
             Tagger => {
                 let signature = opt(|i: &mut _| parse::header_field(i, b"tagger", parse::signature))
                     .context(StrContext::Expected("tagger <signature>".into()))
-                    .parse_next(&mut i)?;
+                    .parse_next(input)?;
                 *state = Message;
-                (i, Token::Tagger(signature))
+                Token::Tagger(signature)
             }
             Message => {
-                let (message, pgp_signature) = terminated(decode::message, eof).parse_next(&mut i)?;
+                let (message, pgp_signature) = terminated(decode::message, eof).parse_next(input)?;
                 debug_assert!(
-                    i.is_empty(),
+                    input.is_empty(),
                     "we should have consumed all data - otherwise iter may go forever"
                 );
-                return Ok((i, Token::Body { message, pgp_signature }));
+                Token::Body { message, pgp_signature }
             }
         })
     }
diff --git a/gix-object/src/tree/ref_iter.rs b/gix-object/src/tree/ref_iter.rs
@@ -2,6 +2,7 @@ use std::convert::TryFrom;
 
 use bstr::BStr;
 use winnow::error::ParserError;
+use winnow::prelude::*;
 
 use crate::{tree, tree::EntryRef, TreeRef, TreeRefIter};
 
@@ -15,7 +16,11 @@ impl<'a> TreeRefIter<'a> {
 impl<'a> TreeRef<'a> {
     /// Deserialize a Tree from `data`.
     pub fn from_bytes(mut data: &'a [u8]) -> Result<TreeRef<'a>, crate::decode::Error> {
-        decode::tree(&mut data).map_err(crate::decode::Error::with_err)
+        let input = &mut data;
+        match decode::tree.parse_next(input) {
+            Ok(tag) => Ok(tag),
+            Err(err) => Err(crate::decode::Error::with_err(err, input)),
+        }
     }
 
     /// Find an entry named `name` knowing if the entry is a directory or not, using a binary search.
@@ -73,6 +78,7 @@ impl<'a> Iterator for TreeRefIter<'a> {
                 #[allow(clippy::unit_arg)]
                 Some(Err(crate::decode::Error::with_err(
                     winnow::error::ErrMode::from_error_kind(&failing, winnow::error::ErrorKind::Verify),
+                    failing,
                 )))
             }
         }
diff --git a/gix-object/tests/commit/mod.rs b/gix-object/tests/commit/mod.rs
@@ -169,7 +169,7 @@ fn invalid() {
     assert_eq!(
         CommitRef::from_bytes(partial_commit).unwrap_err().to_string(),
         if cfg!(feature = "verbose-object-parsing-errors") {
-            "expected `<timestamp>`, `<name> <<email>> <timestamp> <+|-><HHMM>`, `author <signature>`"
+            "object parsing failed at `1`\nexpected `<timestamp>`, `<name> <<email>> <timestamp> <+|-><HHMM>`, `author <signature>`"
         } else {
             "object parsing failed"
         }
diff --git a/gix-object/tests/tag/mod.rs b/gix-object/tests/tag/mod.rs
@@ -123,7 +123,7 @@ fn invalid() {
     assert_eq!(
         TagRef::from_bytes(partial_tag).unwrap_err().to_string(),
         if cfg!(feature = "verbose-object-parsing-errors") {
-            ""
+            "object parsing failed at `tagger Sebasti`"
         } else {
             "object parsing failed"
         }
diff --git a/gix-object/tests/tree/mod.rs b/gix-object/tests/tree/mod.rs
diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs
diff --git a/gix-path/src/env/mod.rs b/gix-path/src/env/mod.rs
diff --git a/gix-transport/src/client/blocking_io/ssh/mod.rs b/gix-transport/src/client/blocking_io/ssh/mod.rs
diff --git a/gix/Cargo.toml b/gix/Cargo.toml

Original file line number	Diff line number	Diff line change
`@@ -93,7 +93,9 @@ mod prepare {`
`93`	`93`	`impl Prepare {`
`94`	`94`	`/// Spawn the command as configured.`
`95`	`95`	`pub fn spawn(self) -> std::io::Result<std::process::Child> {`
`96`		`- Command::from(self).spawn()`
	`96`	`+ let mut cmd = Command::from(self);`
	`97`	`+ gix_trace::debug!(cmd = ?cmd);`
	`98`	`+ cmd.spawn()`
`97`	`99`	`}`
`98`	`100`	`}`
`99`	`101`
Original file line number	Diff line number	Diff line change
`@@ -267,6 +267,7 @@ pub mod decode {`
`267`	`267`	`pub(crate) fn empty_error() -> Error {`
`268`	`268`	`Error {`
`269`	`269`	`inner: winnow::error::ContextError::new(),`
	`270`	`+ remaining: Default::default(),`
`270`	`271`	`}`
`271`	`272`	`}`
`272`	`273`
`@@ -275,19 +276,27 @@ pub mod decode {`
`275`	`276`	`pub struct Error {`
`276`	`277`	`/// The actual error`
`277`	`278`	`pub inner: ParseError,`
	`279`	`+ /// Where the error occurred`
	`280`	`+ pub remaining: Vec<u8>,`
`278`	`281`	`}`
`279`	`282`
`280`	`283`	`impl Error {`
`281`		`- pub(crate) fn with_err(err: winnow::error::ErrMode<ParseError>) -> Self {`
	`284`	`+ pub(crate) fn with_err(err: winnow::error::ErrMode<ParseError>, remaining: &[u8]) -> Self {`
`282`	`285`	`Self {`
`283`	`286`	`inner: err.into_inner().expect("we don't have streaming parsers"),`
	`287`	`+ remaining: remaining.to_owned(),`
`284`	`288`	`}`
`285`	`289`	`}`
`286`	`290`	`}`
`287`	`291`
`288`	`292`	`impl std::fmt::Display for Error {`
`289`	`293`	`fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {`
`290`		`- self.inner.fmt(f)`
	`294`	+ write!(f, "object parsing failed at `{}`", bstr::BStr::new(&self.remaining))?;
	`295`	`+ if self.inner.context().next().is_some() {`
	`296`	`+ writeln!(f)?;`
	`297`	`+ self.inner.fmt(f)?;`
	`298`	`+ }`
	`299`	`+ Ok(())`
`291`	`300`	`}`
`292`	`301`	`}`
`293`	`302`
`@@ -316,7 +325,7 @@ pub mod decode {`
`316`	`325`	`}`
`317`	`326`
`318`	`327`	`impl Error {`
`319`		`- pub(crate) fn with_err(err: winnow::error::ErrMode<ParseError>) -> Self {`
	`328`	`+ pub(crate) fn with_err(err: winnow::error::ErrMode<ParseError>, _remaining: &[u8]) -> Self {`
`320`	`329`	`Self {`
`321`	`330`	`inner: err.into_inner().expect("we don't have streaming parsers"),`
`322`	`331`	`}`