Re: [RFC PATCH 4/6] contrib/cgit-rs: add repo initialization and config access

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Aug 7, 2024, 21:26 UTC
Message-ID: <ZrPmmBqK3IIiQutV@tapette.crustytoothpaste.net>
In-Reply-To: <b84a8210a05c2358dc29f24a56adcbeaa90c8654.1723054623.git.steadmon@google.com>

On 2024-08-07 at 18:21:29, Josh Steadmon wrote:

Show 23 quoted lines

> diff --git a/contrib/cgit-rs/src/lib.rs b/contrib/cgit-rs/src/lib.rs
> index dc46e7ff42..df350e758f 100644
> --- a/contrib/cgit-rs/src/lib.rs
> +++ b/contrib/cgit-rs/src/lib.rs
> @@ -1,6 +1,17 @@
> -use libc::c_char;
> +use libc::{c_char, c_int};
>  
>  extern "C" {
> +    pub fn libgit_setup_git_directory() -> *const c_char;
> +
> +    // From config.c
> +    pub fn libgit_config_get_int(key: *const c_char, dest: *mut c_int) ->c_int;
> +
> +    // From repository.c
> +    pub fn libgit_initialize_the_repository();
> +
> +    // From parse.c
> +    pub fn libgit_parse_maybe_bool(val: *const c_char) -> c_int;
> +
>      // From version.c
>      pub fn libgit_user_agent() -> *const c_char;
>      pub fn libgit_user_agent_sanitized() -> *const c_char;

I think it would be helpful if we didn't expose the raw C API in Rust. Nobody is going to want to write code that uses that in Rust.

If we _do_ expose that, it should be in a separate cgit-sys crate, which is the customary naming, that exposes only that and then we can offer nicer wrappers around it.

Show 36 quoted lines

> diff --git a/contrib/cgit-rs/src/main.rs b/contrib/cgit-rs/src/main.rs
> index 1794e3f43e..c5f8644fca 100644
> --- a/contrib/cgit-rs/src/main.rs
> +++ b/contrib/cgit-rs/src/main.rs
> @@ -1,4 +1,4 @@
> -use std::ffi::CStr;
> +use std::ffi::{CStr, CString};
>  
>  fn main() {
>      println!("Let's print some strings provided by Git");
> @@ -7,4 +7,25 @@ fn main() {
>      println!("git_user_agent() = {:?}", c_str);
>      println!("git_user_agent_sanitized() = {:?}",
>          unsafe { CStr::from_ptr(cgit::libgit_user_agent_sanitized()) });
> +
> +    println!("\nNow try passing args");
> +    let test_arg = CString::new("test_arg").unwrap();
> +    println!("git_parse_maybe_bool(...) = {:?}",
> +        unsafe { cgit::libgit_parse_maybe_bool(test_arg.as_ptr()) });
> +
> +    println!("\nCan we get an int out of our config??");
> +    unsafe {
> +        cgit::libgit_initialize_the_repository();
> +        cgit::libgit_setup_git_directory();
> +        let mut val: libc::c_int = 0;
> +        let key = CString::new("trace2.eventNesting").unwrap();
> +        cgit::libgit_config_get_int(
> +            key.as_ptr(),
> +            &mut val as *mut i32
> +        );
> +        println!(
> +            "git_config_get_int(\"trace2.eventNesting\") -> {:?}",
> +            val
> +        );
> +    };
>  }

It seems like this code wants to be a set of tests and maybe it would be helpful to write it as a few instead. For example, we can assume that our user-agent starts with `git/` assuming it hasn't been overridden, so maybe we could write that as a test, or at least that we got a valid C string.

-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA