On 2025.01.21 00:00, brian m. carlson wrote: > 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. OK, I'll add a TODO comment in Cargo.toml. > > 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. Fixed for V7. > 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. Hmm, I guess we'd need to do that for libgit-sys as well? Or maybe not, since they're part of the API and thus we should just keep the current `pub type ...` setup? > > +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. ACK, will do for V7. > 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. ACK. > > +#[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. Thanks for the review and advice (both for this round and all the previous ones), it's much appreciated. > -- > brian m. carlson (they/them or he/him) > Toronto, Ontario, CA
