Unfortunately common. That said, I think the best route is usually a thin wrapper module, and then a module that attempts to map that to something more idiomatic. At least then there's a clear place where changes/additions in the underlying library should be handled. I've lived the problem of multiple modules that each try to deal with mapping the underlying library itself to an idiomatic interface. They're good for a while, but they always seem much more susceptible to drift in the underlying library causing problems over time.
I think there are two alternatives of increasing difficulty: 1. Write a thicker wrapping around git2 (possibly in a different crate) that achieves an idiomatic API. 2. Write an equivalent to libgit2 in rust.
It’s true that 2 has some significant downsides and probably isn’t worth the effort but it certainly would’ve prevented this bug.
The root cause was likely Github returning 200 when they should've returned a 5xx error.
A better solution in this case is to verify the push worked by looking at the remote for a ref in the push before returning success. I would never just depend on git2 to libgit2 callbacks succeeding as "proof" that it worked, because there's too much to go wrong.
Also, the crates.io patch should be upstreamed into git2 by wrapping the block. And, the ultimate fix is to address libgit2 to make sure it's handling errors correctly. It's not a very good fix, but it's better than what exists now.
Furthermore, it seems like a pure Rust git would be better than problematic wrapper layers stacked one on of top of the other.
I agree though that a pure rust implementation of libgit2 would be ideal.
I didn’t realize how complicated the backend tooling is to achieve the same portability as C - it seems so much simpler as a consumer of the toolchain. IIRC the bootstrapping problem is still something distros are fighting with today, but hopefully one day we can have rustc bundled with all the other standard build tools.
In this case, the git2 crate (https://docs.rs/git2/) is a thin wrapping over libgit2 (https://libgit2.org). To ensure a push went through properly you have to watch what happens inside a callback. The docs state the following:
Note that you'll likely want to use RemoteCallbacks and set push_update_reference to test whether all the references were pushed successfully.
It looks like the only sensible way to utilize this is by closing over mutable state and then checking that imperatively, which is far from idiomatic (this is what they do in the patch). If you were pushing multiple refs it would be even worse!
Usually, when you use some rust code, it's pretty easy to tell what can fail simply by looking at the types - this is a huge win. Thin wrappers can subtly violate this intuition: sometimes errors are handled idiomatically and sometimes they aren't, leading to a sort of false sense of security.
The takeaway here is to use an abundance of caution when utilizing a thin wrapper and to understand just how thick it is, especially with errors.