fix tree parsing and error reporting

gitoxide-core/src/repository/revision/resolve.rs

@@ -4,6 +4,12 @@ pub struct Options {
     pub format: OutputFormat,
     pub explain: bool,
     pub cat_file: bool,
+    pub tree_mode: TreeMode,
+}
+
+pub enum TreeMode {
+    Raw,
+    Pretty,
 }
 
 pub(crate) mod function {
@@ -13,6 +19,7 @@ pub(crate) mod function {
     use gix::revision::Spec;
 
     use super::Options;
+    use crate::repository::revision::resolve::TreeMode;
     use crate::{repository::revision, OutputFormat};
 
     pub fn resolve(
@@ -23,6 +30,7 @@ pub(crate) mod function {
             format,
             explain,
             cat_file,
+            tree_mode,
         }: Options,
     ) -> anyhow::Result<()> {
         repo.object_cache_size_if_unset(1024 * 1024);
@@ -36,7 +44,7 @@ pub(crate) mod function {
                     let spec = gix::path::os_str_into_bstr(&spec)?;
                     let spec = repo.rev_parse(spec)?;
                     if cat_file {
-                        return display_object(spec, out);
+                        return display_object(spec, tree_mode, out);
                     }
                     writeln!(out, "{spec}", spec = spec.detach())?;
                 }
@@ -63,11 +71,11 @@ pub(crate) mod function {
         Ok(())
     }
 
-    fn display_object(spec: Spec<'_>, mut out: impl std::io::Write) -> anyhow::Result<()> {
+    fn display_object(spec: Spec<'_>, tree_mode: TreeMode, mut out: impl std::io::Write) -> anyhow::Result<()> {
         let id = spec.single().context("rev-spec must resolve to a single object")?;
         let object = id.object()?;
         match object.kind {
-            gix::object::Kind::Tree => {
+            gix::object::Kind::Tree if matches!(tree_mode, TreeMode::Pretty) => {
                 for entry in object.into_tree().iter() {
                     writeln!(out, "{}", entry?)?;
                 }

gix-object/Cargo.toml

@@ -25,7 +25,7 @@ serde = ["dep:serde", "bstr/serde", "smallvec/serde", "gix-hash/serde", "gix-act
 ## details information about the error location will be collected.
 ## Use it in applications which expect broken or invalid objects or for debugging purposes. Incorrectly formatted objects aren't at all
 ## common otherwise.
-verbose-object-parsing-errors = []
+verbose-object-parsing-errors = ["winnow/std"]
 
 [dependencies]
 gix-features = { version = "^0.36.0", path = "../gix-features", features = ["rustsha1", "progress"] }

gix-object/benches/decode_objects.rs

@@ -19,10 +19,10 @@ fn parse_tag(c: &mut Criterion) {
 }
 
 fn parse_tree(c: &mut Criterion) {
-    c.bench_function("TreeRef(sig)", |b| {
+    c.bench_function("TreeRef()", |b| {
         b.iter(|| black_box(gix_object::TreeRef::from_bytes(TREE)).unwrap())
     });
-    c.bench_function("TreeRefIter(sig)", |b| {
+    c.bench_function("TreeRefIter()", |b| {
         b.iter(|| black_box(gix_object::TreeRefIter::from_bytes(TREE).count()))
     });
 }

gix-object/src/lib.rs

@@ -290,6 +290,12 @@ pub mod decode {
                 self.inner.fmt(f)
             }
         }
+
+        impl std::error::Error for Error {
+            fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+                self.inner.cause().map(|v| v as &(dyn std::error::Error + 'static))
+            }
+        }
     }
 
     ///
@@ -318,14 +324,15 @@ pub mod decode {
         }
 
         impl std::fmt::Display for Error {
-            fn fmt(&self, _f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-                Ok(())
+            fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+                f.write_str("object parsing failed")
             }
         }
+
+        impl std::error::Error for Error {}
     }
     pub(crate) use _decode::empty_error;
     pub use _decode::{Error, ParseError};
-    impl std::error::Error for Error {}
 
     /// Returned by [`loose_header()`]
     #[derive(Debug, thiserror::Error)]

gix-object/src/tree/ref_iter.rs

@@ -67,11 +67,11 @@ impl<'a> Iterator for TreeRefIter<'a> {
                 Some(Ok(entry))
             }
             None => {
+                let failing = self.data;
                 self.data = &[];
-                let empty = &[] as &[u8];
                 #[allow(clippy::unit_arg)]
                 Some(Err(crate::decode::Error::with_err(
-                    winnow::error::ErrMode::from_error_kind(&empty, winnow::error::ErrorKind::Verify),
+                    winnow::error::ErrMode::from_error_kind(&failing, winnow::error::ErrorKind::Verify),
                 )))
             }
         }
@@ -116,17 +116,9 @@ mod decode {
     use std::convert::TryFrom;
 
     use bstr::ByteSlice;
-    use winnow::{
-        combinator::{eof, repeat, terminated},
-        error::ParserError,
-        prelude::*,
-        stream::AsChar,
-        token::{take, take_while},
-    };
+    use winnow::{error::ParserError, prelude::*};
 
-    use crate::{parse::SPACE, tree, tree::EntryRef, TreeRef};
-
-    const NULL: &[u8] = b"\0";
+    use crate::{tree, tree::EntryRef, TreeRef};
 
     pub fn fast_entry(i: &[u8]) -> Option<(&[u8], EntryRef<'_>)> {
         let mut mode = 0u32;
@@ -157,24 +149,20 @@ mod decode {
         ))
     }
 
-    pub fn entry<'a, E: ParserError<&'a [u8]>>(i: &mut &'a [u8]) -> PResult<EntryRef<'a>, E> {
-        (
-            terminated(take_while(5..=6, AsChar::is_dec_digit), SPACE)
-                .verify_map(|mode| tree::EntryMode::try_from(mode).ok()),
-            terminated(take_while(1.., |b| b != NULL[0]), NULL),
-            take(20u8), // TODO(SHA256): make this compatible with other hash lengths
-        )
-            .map(|(mode, filename, oid): (_, &[u8], _)| EntryRef {
-                mode,
-                filename: filename.as_bstr(),
-                oid: gix_hash::oid::try_from_bytes(oid).expect("we counted exactly 20 bytes"),
-            })
-            .parse_next(i)
-    }
-
     pub fn tree<'a, E: ParserError<&'a [u8]>>(i: &mut &'a [u8]) -> PResult<TreeRef<'a>, E> {
-        terminated(repeat(0.., entry), eof)
-            .map(|entries| TreeRef { entries })
-            .parse_next(i)
+        let mut out = Vec::new();
+        let mut i = &**i;
+        while !i.is_empty() {
+            let Some((rest, entry)) = fast_entry(i) else {
+                #[allow(clippy::unit_arg)]
+                return Err(winnow::error::ErrMode::from_error_kind(
+                    &i,
+                    winnow::error::ErrorKind::Verify,
+                ));
+            };
+            i = rest;
+            out.push(entry);
+        }
+        Ok(TreeRef { entries: out })
     }
 }

gix-object/tests/commit/mod.rs

@@ -1,3 +1,6 @@
+use crate::fixture_name;
+use gix_object::{CommitRef, CommitRefIter};
+
 const SIGNATURE: & [u8; 487] = b"-----BEGIN PGP SIGNATURE-----\n\niQEzBAABCAAdFiEEdjYp/sh4j8NRKLX27gKdHl60AwAFAl7q2DsACgkQ7gKdHl60\nAwDvewgAkL5UjEztzeVXlzceom0uCrAkCw9wSGLTmYcMKW3JwEaTRgQ4FX+sDuFT\nLZ8DoPu3UHUP0QnKrUwHulTTlKcOAvsczHbVPIKtXCxo6QpUfhsJQwz/J29kiE4L\nsOd+lqKGnn4oati/de2xwqNGi081fO5KILX75z6KfsAe7Qz7R3jxRF4uzHI033O+\nJc2Y827XeaELxW40SmzoLanWgEcdreXf3PstXEWW77CAu0ozXmvYj56vTviVybxx\nG7bc8lwc+SSKVe2VVB+CCfVbs0i541gmghUpZfMhUgaqttcCH8ysrUJDhne1BLG8\nCrOJIWTwAeEDtomV1p76qrMeqr1GFg==\n=qlSN\n-----END PGP SIGNATURE-----";
 
 const LONG_MESSAGE: &str = "Merge tag 'thermal-v5.8-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux
@@ -159,6 +162,27 @@ mod method {
     }
 }
 
+#[test]
+fn invalid() {
+    let fixture = fixture_name("commit", "unsigned.txt");
+    let partial_commit = &fixture[..fixture.len() / 2];
+    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>`"
+        } else {
+            "object parsing failed"
+        }
+    );
+    assert_eq!(
+        CommitRefIter::from_bytes(partial_commit)
+            .take_while(Result::is_ok)
+            .count(),
+        1,
+        "we can decode some fields before failing"
+    );
+}
+
 mod from_bytes;
 mod iter;
 mod message;

gix-object/tests/tag/mod.rs

@@ -1,5 +1,6 @@
+use crate::fixture_name;
 use gix_date::time::Sign;
-use gix_object::{bstr::ByteSlice, Kind, TagRef};
+use gix_object::{bstr::ByteSlice, Kind, TagRef, TagRefIter};
 
 mod method {
     use gix_object::TagRef;
@@ -115,6 +116,25 @@ KLMHist5yj0sw1E4hDTyQa0=
     }
 }
 
+#[test]
+fn invalid() {
+    let fixture = fixture_name("tag", "whitespace.txt");
+    let partial_tag = &fixture[..fixture.len() / 2];
+    assert_eq!(
+        TagRef::from_bytes(partial_tag).unwrap_err().to_string(),
+        if cfg!(feature = "verbose-object-parsing-errors") {
+            ""
+        } else {
+            "object parsing failed"
+        }
+    );
+    assert_eq!(
+        TagRefIter::from_bytes(partial_tag).take_while(Result::is_ok).count(),
+        4,
+        "we can decode some fields before failing"
+    );
+}
+
 mod from_bytes {
     use gix_object::{bstr::ByteSlice, Kind, TagRef};
 

gix-object/tests/tree/mod.rs

@@ -56,7 +56,7 @@ mod iter {
 }
 
 mod from_bytes {
-    use gix_object::{bstr::ByteSlice, tree, tree::EntryRef, TreeRef};
+    use gix_object::{bstr::ByteSlice, tree, tree::EntryRef, TreeRef, TreeRefIter};
 
     use crate::{fixture_name, hex_to_id};
 
@@ -108,24 +108,46 @@ mod from_bytes {
     }
 
     #[test]
-    fn maybe_special() -> crate::Result {
+    fn invalid() {
+        let fixture = fixture_name("tree", "definitely-special.tree");
+        let partial_tree = &fixture[..fixture.len() / 2];
         assert_eq!(
-            TreeRef::from_bytes(&fixture_name("tree", "maybe-special.tree"))?
-                .entries
-                .len(),
-            160
+            TreeRef::from_bytes(partial_tree).unwrap_err().to_string(),
+            if cfg!(feature = "verbose-object-parsing-errors") {
+                ""
+            } else {
+                "object parsing failed"
+            }
+        );
+        assert_eq!(
+            TreeRefIter::from_bytes(partial_tree).take_while(Result::is_ok).count(),
+            9,
+            "we can decode about half of it before failing"
         );
-        Ok(())
     }
 
     #[test]
-    fn definitely_special() -> crate::Result {
-        assert_eq!(
-            TreeRef::from_bytes(&fixture_name("tree", "definitely-special.tree"))?
-                .entries
-                .len(),
-            19
-        );
+    fn special_trees() -> crate::Result {
+        for (name, expected_entry_count) in [
+            ("maybe-special", 160),
+            ("definitely-special", 19),
+            ("special-1", 5),
+            ("special-2", 18),
+            ("special-3", 5),
+            ("special-4", 18),
+        ] {
+            let fixture = fixture_name("tree", &format!("{name}.tree"));
+            assert_eq!(
+                TreeRef::from_bytes(&fixture)?.entries.len(),
+                expected_entry_count,
+                "{name}"
+            );
+            assert_eq!(
+                TreeRefIter::from_bytes(&fixture).map(Result::unwrap).count(),
+                expected_entry_count,
+                "{name}"
+            );
+        }
         Ok(())
     }
 }

src/plumbing/main.rs

@@ -925,6 +925,7 @@ pub fn main() -> Result<()> {
                 specs,
                 explain,
                 cat_file,
+                tree_mode,
             } => prepare_and_run(
                 "revision-parse",
                 trace,
@@ -941,6 +942,12 @@ pub fn main() -> Result<()> {
                             format,
                             explain,
                             cat_file,
+                            tree_mode: match tree_mode.unwrap_or_default() {
+                                revision::resolve::TreeMode::Raw => core::repository::revision::resolve::TreeMode::Raw,
+                                revision::resolve::TreeMode::Pretty => {
+                                    core::repository::revision::resolve::TreeMode::Pretty
+                                }
+                            },
                         },
                     )
                 },

src/plumbing/options/mod.rs

@@ -595,6 +595,16 @@ pub mod commitgraph {
 }
 
 pub mod revision {
+    pub mod resolve {
+        #[derive(Default, Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, clap::ValueEnum)]
+        pub enum TreeMode {
+            /// Show the raw bytes - only useful for piping into files for use with tooling.
+            Raw,
+            /// Display a tree in human-readable form.
+            #[default]
+            Pretty,
+        }
+    }
     #[derive(Debug, clap::Subcommand)]
     #[clap(visible_alias = "rev", visible_alias = "r")]
     pub enum Subcommands {
@@ -625,6 +635,8 @@ pub mod revision {
             /// Show the first resulting object similar to how `git cat-file` would, but don't show the resolved spec.
             #[clap(short = 'c', long, conflicts_with = "explain")]
             cat_file: bool,
+            #[clap(short = 't', long)]
+            tree_mode: Option<resolve::TreeMode>,
             /// rev-specs like `@`, `@~1` or `HEAD^2`.
             #[clap(required = true)]
             specs: Vec<std::ffi::OsString>,