Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 5 commits into from

Conversation

bjeanes
Copy link
Contributor

@bjeanes bjeanes commented Sep 14, 2024

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
Copy link
Contributor Author

bjeanes commented Sep 15, 2024

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
Copy link
Contributor Author

bjeanes commented Sep 17, 2024

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
Copy link
Collaborator

LMG commented Sep 17, 2024

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
Copy link
Contributor Author

bjeanes commented Sep 17, 2024

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: #1387

@bjeanes
Copy link
Contributor Author

bjeanes commented Sep 17, 2024

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 ebbb32e but I imagine that instead the maintainers may wish to change the tag back to the r-prefixed scheme.

@bjeanes
Copy link
Contributor Author

bjeanes commented Sep 18, 2024

Closing in lieu of #1389

@bjeanes bjeanes closed this Sep 18, 2024
@bjeanes bjeanes deleted the roundtrip-custom-headers branch September 18, 2024 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Roundtripping broken when raw commit includes custom metadata (e.g. as GitButler adds)
2 participants