josh-project / josh

Just One Single History
https://josh-project.github.io/josh/
MIT License
1.48k stars 55 forks source link

Fix round-tripping when commit contains non-standard headers #1387

Closed bjeanes closed 1 month ago

bjeanes commented 1 month ago

Fixes #1377 Closes #1389

This takes a similar strategy as GitButler uses to insert its custom headers in the first place. In essence, we build the bytes of the commit object ourselves, starting with the bytes from the original commit, and then write it using git2::odb::write.

The only thing I am a little unhappy with is how committer/author handling has to "know" the Git format for timestamps. Without using unsafe {}, I'm not sure how to leverage libgit2's authoritative implementation for these parts in a clean way, and even with unsafe {} I'm not sure it's possible. I think this is good enough though, but I am interested in others' thoughts.

NB: I am familiar enough with Rust to have made this work, but I am far from a "good" Rust programmer, so I am happy to take direction to tweak this implementation if you feel there are specific improvements (e.g. such as standardising BString/String/&str).

bjeanes commented 1 month ago

Some further thoughts:

Looking forward to the thoughts of maintainers and happy to throw up a version using gix (if I'm successful) if there's interest. Also happy to merge this and follow up with that work or to make minor changes to what I have already done, as I'm pretty motivated to help get JOSH working for the repository I want to import into monorepo.

bjeanes commented 1 month ago

Thanks for unblocking the tests. The only failure seems to be:

   $ curl -s http://localhost:8002/version
-  Version: r*.*.* (glob)
+  Version: v24.08.14-5-g5a26dd2

I didn't change the version format, so I'm not sure why it's failing.

LMG commented 1 month ago

Yes, I saw that. This is very curious. I don't have a lot of time to look at it right now though, and I also would like a review from @christian-schilling before merging

bjeanes commented 1 month ago

I just realised that it should be possible to use gix_object::Commit/gix_object::CommitRef without using the rest of Gix or introducing the IO concerns raised in #1337. This would avoid needing to introduce this bespoke CommitBuffer and keep the implementation of custom header handling to a more purpose built location.

I'm going to have a little look-see if I can make an improved version.

Edit: https://github.com/josh-project/josh/pull/1387

bjeanes commented 1 month ago

Yes, I saw that. This is very curious. I don't have a lot of time to look at it right now

It looks like it's just because the most recent release tag changed schemes (possibly unintentionally):

❯ git tag
r21.03.07
r21.03.08
[ ... snip ... ]
r23.02.14
r23.12.04
v0.1.0
v0.2.0
v24.08.14       <---

I added ebbb32eb57f7587efa83317afeb33c1fb6b46eea but I imagine that instead the maintainers may wish to change the tag back to the r-prefixed scheme.

bjeanes commented 1 month ago

Closing in lieu of #1389