On 2025-01-15 at 20:05:43, Josh Steadmon wrote: > From: Calvin Wan <calvinwan@xxxxxxxxxx> > diff --git a/contrib/libgit-rs/Cargo.lock b/contrib/libgit-rs/Cargo.lock > new file mode 100644 > index 0000000000..a30c7c8d33 > --- /dev/null > +++ b/contrib/libgit-rs/Cargo.lock > @@ -0,0 +1,77 @@ > +# This file is automatically @generated by Cargo. > +# It is not intended for manual editing. > +version = 3 > + > +[[package]] > +name = "autocfg" > +version = "1.4.0" > +source = "registry+https://github.com/rust-lang/crates.io-index"; > +checksum = "ace50bade8e6234aa140d9a2f552bbee1db4d353f69b8217bc503490fc1a9f26" > + > +[[package]] > +name = "cc" > +version = "1.1.15" > +source = "registry+https://github.com/rust-lang/crates.io-index"; > +checksum = "57b6a275aa2903740dc87da01c62040406b8812552e97129a63ea8850a17c6e6" > +dependencies = [ > + "shlex", > +] > + > +[[package]] > +name = "libc" > +version = "0.2.158" > +source = "registry+https://github.com/rust-lang/crates.io-index"; > +checksum = "d8adc4bb1803a324070e64a98ae98f38934d91957a99cfb3a43dcbc01bc56439" > + > +[[package]] > +name = "libgit" > +version = "0.1.0" > +dependencies = [ > + "autocfg", > + "libgit-sys", > +] > + > +[[package]] > +name = "libgit-sys" > +version = "0.1.0" > +dependencies = [ > + "autocfg", > + "libz-sys", > + "make-cmd", > +] > + > +[[package]] > +name = "libz-sys" > +version = "1.1.20" > +source = "registry+https://github.com/rust-lang/crates.io-index"; > +checksum = "d2d16453e800a8cf6dd2fc3eb4bc99b786a9b90c663b8559a5b1a041bf89e472" > +dependencies = [ > + "cc", > + "libc", > + "pkg-config", > + "vcpkg", > +] > + > +[[package]] > +name = "make-cmd" > +version = "0.1.0" > +source = "registry+https://github.com/rust-lang/crates.io-index"; > +checksum = "a8ca8afbe8af1785e09636acb5a41e08a765f5f0340568716c18a8700ba3c0d3" > + > +[[package]] > +name = "pkg-config" > +version = "0.3.30" > +source = "registry+https://github.com/rust-lang/crates.io-index"; > +checksum = "d231b230927b5e4ad203db57bbcbee2802f6bce620b1e4a9024a07d94e2907ec" > + > +[[package]] > +name = "shlex" > +version = "1.3.0" > +source = "registry+https://github.com/rust-lang/crates.io-index"; > +checksum = "0fda2ff0d084019ba4d7c6f371c95d8fd75ce3524c3cb8fb653a3023f6323e64" > + > +[[package]] > +name = "vcpkg" > +version = "0.2.15" > +source = "registry+https://github.com/rust-lang/crates.io-index"; > +checksum = "accd4ea62f7bb7a82fe23066fb0957d48ef677f6eeb8215f372f52e48bb32426" There are two possibilities here. The first is to check in the Cargo.lock, in which case all users will have to use these versions. That produces a more stable and reliable approach, but it has some downsides. Say, for instance, that a cool new platform or architecture is added to libc and we'd like to support it, but that version of libc requires a newer version of Rust. We then would have to hold off supporting that new platform due to compatibility reasons. But if we omitted the Cargo.lock, users could install any version that meets their needs. I believe Rust just got the ability to install only versions that honour the rust-version directive in 1.84, whereas older versions will try to use the latest version, even if that fails. So I think it's okay for now to use Cargo.lock, because that means that things will be better out of the box for users on older Rust. But we may want to drop it once 1.84 is older than our supported version. > diff --git a/contrib/libgit-rs/src/lib.rs b/contrib/libgit-rs/src/lib.rs > new file mode 100644 > index 0000000000..27b6fd63f1 > --- /dev/null > +++ b/contrib/libgit-rs/src/lib.rs > @@ -0,0 +1,95 @@ > +use std::ffi::{c_void, CStr, CString}; > +use std::path::Path; > + > +#[cfg(has_std__ffi__c_char)] > +use std::ffi::{c_char, c_int}; > + > +#[cfg(not(has_std__ffi__c_char))] > +#[allow(non_camel_case_types)] > +pub type c_char = i8; > + > +#[cfg(not(has_std__ffi__c_char))] > +#[allow(non_camel_case_types)] > +pub type c_int = i32; By making these `pub`, you're exporting them. We probably do not want to do that, since they are not part of our API. If we need them more generally in the code, let's put them in a module called `ffi` or such that's `pub(crate)`, and then use them from there. > +use libgit_sys::*; > + > +pub struct ConfigSet(*mut libgit_config_set); > +impl ConfigSet { I would suggest we place these in a module, such as `config`. We should expect to have a lot more things in our crate in the future and putting a little thought into this now will make it easier for users in the future. I'd also, of course, suggest documentation comments, since if we ever upload this to crates.io, people are overwhelmingly going to read only the docs and not the source, and right now we've said nothing about how this works or should work. > +#[cfg(test)] > +mod tests { > + use super::*; > + > + #[test] > + fn load_configs_via_configset() { > + let mut cs = ConfigSet::new(); > + cs.add_files(&[ > + Path::new("testdata/config1"), > + Path::new("testdata/config2"), > + Path::new("testdata/config3"), > + ]); > + // ConfigSet retrieves correct value > + assert_eq!(cs.get_int("trace2.eventTarget"), Some(1)); > + // ConfigSet respects last config value set > + assert_eq!(cs.get_int("trace2.eventNesting"), Some(3)); > + // ConfigSet returns None for missing key > + assert_eq!(cs.get_string("foo.bar"), None); > + } > +} I am, of course, delighted to see tests. This is a nice improvement in our code that we can take advantage of, and we can test both the C code and Rust code at the same time. And if, in the future, we decide that we'd like to implement a Rust-based version of this API to replace the C one, we've already written tests. -- brian m. carlson (they/them or he/him) Toronto, Ontario, CA
Attachment:
signature.asc
Description: PGP signature
