Re: [PATCH v6 4/5] libgit: add higher-level libgit crate

> On 2025-01-15 at 20:05:43, Josh Steadmon wrote:
> > From: Calvin Wan <calvinwan@google.com>
> > 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.

> > +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.

> > +#[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.