The `CommitRef` from `invalid_email_or_committer` test cannot be materialized or round-tripped

[ilyagr]

Current behavior 😯

At the latest commit a78bf84...

Update: Got the same result at the actually latest commit 442f800.

I modified the test as follows:

diff --git a/gix-object/tests/object/commit/from_bytes.rs b/gix-object/tests/object/commit/from_bytes.rs
index b89213394..09f6653fd 100644
--- a/gix-object/tests/object/commit/from_bytes.rs
+++ b/gix-object/tests/object/commit/from_bytes.rs
@@ -37,9 +37,17 @@ fn invalid_email_of_committer() {
         email: b"gh <Gregor Hartmann<gh@openoffice.org".as_bstr(),
         time: "1282910542 +0200",
     };
+    let fixture = fixture_name("commit", "invalid-actor.txt");
+    let commit_ref = CommitRef::from_bytes(&fixture).expect("ignore strangely formed actor format");
+    let mut buf = vec![];
+    let write_result = commit_ref.write_to(&mut buf);
+    dbg!(&buf.as_bstr());
+    dbg!(&write_result);
+    assert!(write_result.is_ok(), "write result is not OK");
+    // The assert above fails
+
     assert_eq!(
-        CommitRef::from_bytes(&fixture_name("commit", "invalid-actor.txt"))
-            .expect("ignore strangely formed actor format"),
+        commit_ref,
         CommitRef {
             tree: b"220738fd4199e95a2b244465168366a73ebdf271".as_bstr(),
             parents: [b"209fbe2d632761b30b7b17422914e11b93692833".as_bstr()].into(),

The resulting test fails:

---- commit::from_bytes::invalid_email_of_committer stdout ----
[gix-object/tests/object/commit/from_bytes.rs:44:5] &buf.as_bstr() = "tree 220738fd4199e95a2b244465168366a73ebdf271\nparent 209fbe2d632761b30b7b17422914e11b93692833\nauthor Gregor Hartmann <"
[gix-object/tests/object/commit/from_bytes.rs:45:5] &write_result = Err(
    Custom {
        kind: Other,
        error: IllegalCharacter,
    },
)

thread 'commit::from_bytes::invalid_email_of_committer' (43952483) panicked at gix-object/tests/object/commit/from_bytes.rs:46:5:
write result is not OK

Expected behavior 🤔

The test should pass.

AFAIU, CommitRef should always be possible to materialize. If commits like the one in invalid-actor.txt exist in the wild, I'd want to round-trip them.

Git behavior

No response

Steps to reproduce 🕹

No response

[Byron]

Thanks a lot for reporting!

Indeed, CommitRef should have such properties.
Let me try to get a fix going.

[Byron]

I did take a look (see the PR) and concluded that the major problem is the lossy parsing of the email, which leads to a mild cleanup, but also loss of information.

Further, when writing the email, it expects it to be clear of < and > for good reason, it encloses the string in these characters.

So to fix it, one would have to make parsing work losslessly, and then adjust the writer to detect that case and write emails as is.

[ilyagr]

So to fix it, one would have to make parsing work losslessly, and then adjust the writer to detect that case and write emails as is.

Do you mean the parsing to Signature or the parsing to SignatureRef? How would it look? Should parsing SignatureRef into Signature be more fallible than it is now (currently, only the parsing of the time is fallible, and it's not fallible enough IMO, see Aside 1 below).

You probably already know this, but my main interest is round-tripping such signatures. I don't care how and whether they parse to Signature. The way it currently parses is fine by me. Though, if Git allowed Gregor Hartmann<gh <Gregor Hartmann<gh@openoffice.org as a signature line (without a closing > at all), I'm not sure what would be best to do. Do you know whether such a signature line is possible (wouldn't crash Git)?

Aside 1: Perhaps relatedly, this may or may not deserve its own bug, but in gix-object/tests/fixtures/commit/double-dash-date-offset.txt test case, the parsing from SignatureRef into Signature is lossy, but .time() does not seem to return an error IIUC. So, the only way to tell whether this lost information I could find is to serialize Signature again into SignatureRef.

Aside 2: I'm also curious about the general provenance of the little circus of horrors you have in gix-object/tests/fixtures/commit. Is it true that these commits from real repos, and Git itself is known to handle them? If they are from real repos, morbid historical curiosity makes me wonder where these monsters came from (perhaps when you stumble upon more of them, you could write it down, since to my mind these are not merely tests but also archaeological findings).

[Byron]

Do you mean the parsing to Signature or the parsing to SignatureRef? How would it look? Should parsing SignatureRef into Signature be more fallible than it is now (currently, only the parsing of the time is fallible, and it's not fallible enough IMO, see Aside 1 below).

I understand and we are on the same page there.

You probably already know this, but my main interest is round-tripping such signatures. I don't care how and whether they parse to Signature. The way it currently parses is fine by me. Though, if Git allowed Gregor Hartmann<gh <Gregor Hartmann<gh@openoffice.org as a signature line (without a closing > at all), I'm not sure what would be best to do. Do you know whether such a signature line is possible (wouldn't crash Git)?

I am quite certain that today this wouldn't be allowed anymore, but back then it was.

Aside 1: Perhaps relatedly, this may or may not deserve its own bug, but in gix-object/tests/fixtures/commit/double-dash-date-offset.txt test case, the parsing from SignatureRef into Signature is lossy, but .time() does not seem to return an error IIUC. So, the only way to tell whether this lost information I could find is to serialize Signature again into SignatureRef.

It is also my understanding that the current identity parsing is lossy, which is also the source of the difficulties with making this round-trippable.

Aside 2: I'm also curious about the general provenance of the little circus of horrors you have in gix-object/tests/fixtures/commit. Is it true that these commits from real repos, and Git itself is known to handle them? If they are from real repos, morbid historical curiosity makes me wonder where these monsters came from (perhaps when you stumble upon more of them, you could write it down, since to my mind these are not merely tests but also archaeological findings).

Yes, these are always from real repositories and Git does handle them gracefully. Ideally, gix could be tuned to the point where most of these strange beasts round-trip, and maybe not too much is missing once this issue gets resolved.