On 2024.08.07 21:26, brian m. carlson wrote:
> On 2024-08-07 at 18:21:29, Josh Steadmon wrote:
> > 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.
Yeah, that's a known NEEDSWORK that I forgot to mention in the cover
letter. I'm also going to get in touch soon with the gitoxide &
libgit2-rs folks to see if there's any joint work that we can do in
terms of defining nicer Rust APIs.
Semi-relatedly, I was wondering if you might be able to answer a cargo /
crates.io question: for a crate such as cgit-rs, which is not located in
the root of its VCS repo, will cargo balk at downloading the full
git.git worktree? Our build.rs expects the full Git source to be
available at "../.." relative to the crate root. We've currently only
tested consuming cgit-rs in JJ via a local path rather than through
crates.io.
libgit2-rs avoids this issue by including the C source of libgit2 as a
submodule, but I'd prefer for cgit-rs to live in the git.git repo if at
all possible.
> > 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.
Agreed.
> --
> brian m. carlson (they/them or he/him)
> Toronto, Ontario, CA
