4 ms·
This seems like a consequence of using thin wrappers of C libraries in rust, or comparably using a java API from scala, where the error handling methodology is
by klysm 7y ago
This seems like a consequence of using thin wrappers of C libraries in rust, or comparably using a java API from scala, where the error handling methodology is significantly different.
In this case, the git2 crate (https://docs.rs/git2/ https://docs.rs/git2/) is a thin wrapping over libgit2 (https://libgit2.org 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.
- kbenson 7y ago> 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. 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.
- stouset 7y agoAgreed! This is how I've always developed FFI code, for both Rust and Ruby. A thin wrapper that does little more than FFI and maybe mapping onto native memory-management techniques, and then a separate higher-level wrapper that provides as idiomatic an interface on top of the library as possible.
- rabidferret 7y agoYeah, unfortunately there's not really much of an alternative here besides dropping libgit2 and subshelling out to git (which we honestly should consider, it just hasn't been a priority).
- klysm 7y agoShelling out is arguably a worse scheme as it is again a thin wrapper, but with significantly worse error semantics and performance. Parsing the output of CLI tools is tedious very brittle, even in the case when the tool provides nicely formatted and consistent output, not to mention the shitty performance. 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.
- tedunangst 7y agoIsn't the root cause choosing to default to returning Ok() and only maybe changing that if something goes wrong?
- klysm 7y agoWith the api provided by libgit2 (and therefore git2), it's not possible to get the error without closing over mutable state somehow.
- anonsivalley652 7y agoThat's an unnecessary rabbit-hole to go down. 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.
- klysm 7y agoI encourage you to look at the patch (it’s short). The callbacks are not between git2 and libgit2 so I think you are misunderstanding the nature of the problem. I agree though that a pure rust implementation of libgit2 would be ideal.
- steveklabnik 7y agoSome fun history: https://github.com/libgit2/libgit2/pull/3540 https://github.com/libgit2/libgit2/pull/3540
- 7y ago