From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 28 Jan 2025 15:08:48 GMT
Subject: Re: [PATCH v7 2/4] libgit-sys: introduce Rust wrapper for libgit.a
Message-ID: <3559a70c-7ed3-41cd-a45f-390134218c52@gmail.com>
In-Reply-To: <f1502b85901d81889d86597f31eca40e1965d56f.1738023208.git.steadmon@google.com>



On 28/01/2025 00:19, Josh Steadmon wrote:
> Introduce libgit-sys, a Rust wrapper crate that allows Rust code to call
> functions in libgit.a. This initial patch defines build rules and an
> interface that exposes user agent string getter functions as a proof of
> concept. This library can be tested with `cargo test`. 

It's great to see some tests. This is looking good, I've left a couple 
of small comments below.

> +contrib/libgit-sys/partial_symbol_export.o: contrib/libgit-sys/public_symbol_export.o libgit.a reftable/libreftable.a xdiff/lib.a
> +	$(LD) -r $^ -o $@

This is a very long line and the ones below are pretty long - perhaps we 
could put the list of sources in a variable?

> +contrib/libgit-sys/hidden_symbol_export.o: contrib/libgit-sys/partial_symbol_export.o
> +	$(OBJCOPY) --localize-hidden $^ $@
> +
> +contrib/libgit-sys/libgitpub.a: contrib/libgit-sys/hidden_symbol_export.o
> +	$(AR) $(ARFLAGS) $@ $^
 > [...]
> diff --git a/contrib/libgit-sys/public_symbol_export.c b/contrib/libgit-sys/public_symbol_export.c
> new file mode 100644
> index 0000000000..cd1602206e
> --- /dev/null
> +++ b/contrib/libgit-sys/public_symbol_export.c
> @@ -0,0 +1,22 @@
> +/* Shim to publicly export Git symbols. These must be renamed so that the

Style: Multiline comments start with an empty "/*" so this should be

/*
  * Shim ...

> + * original symbols can be hidden. Renaming these with a "libgit_" prefix also
> + * avoids conflicts with other libraries such as libgit2.
> + */

Best Wishes

Phillip

> +#include "git-compat-util.h"
> +#include "contrib/libgit-sys/public_symbol_export.h"
> +#include "version.h"
> +
> +#pragma GCC visibility push(default)
> +
> +const char *libgit_user_agent(void)
> +{
> +	return git_user_agent();
> +}
> +
> +const char *libgit_user_agent_sanitized(void)
> +{
> +	return git_user_agent_sanitized();
> +}
> +
> +#pragma GCC visibility pop
> diff --git a/contrib/libgit-sys/public_symbol_export.h b/contrib/libgit-sys/public_symbol_export.h
> new file mode 100644
> index 0000000000..a3372f93fa
> --- /dev/null
> +++ b/contrib/libgit-sys/public_symbol_export.h
> @@ -0,0 +1,8 @@
> +#ifndef PUBLIC_SYMBOL_EXPORT_H
> +#define PUBLIC_SYMBOL_EXPORT_H
> +
> +const char *libgit_user_agent(void);
> +
> +const char *libgit_user_agent_sanitized(void);
> +
> +#endif /* PUBLIC_SYMBOL_EXPORT_H */
> diff --git a/contrib/libgit-sys/src/lib.rs b/contrib/libgit-sys/src/lib.rs
> new file mode 100644
> index 0000000000..d4853f3074
> --- /dev/null
> +++ b/contrib/libgit-sys/src/lib.rs
> @@ -0,0 +1,46 @@
> +#[cfg(has_std__ffi__c_char)]
> +use std::ffi::c_char;
> +
> +#[cfg(not(has_std__ffi__c_char))]
> +#[allow(non_camel_case_types)]
> +pub type c_char = i8;
> +
> +extern crate libz_sys;
> +
> +extern "C" {
> +    pub fn libgit_user_agent() -> *const c_char;
> +    pub fn libgit_user_agent_sanitized() -> *const c_char;
> +}
> +
> +#[cfg(test)]
> +mod tests {
> +    use std::ffi::CStr;
> +
> +    use super::*;
> +
> +    #[test]
> +    fn user_agent_starts_with_git() {
> +        let c_str = unsafe { CStr::from_ptr(libgit_user_agent()) };
> +        let agent = c_str
> +            .to_str()
> +            .expect("User agent contains invalid UTF-8 data");
> +        assert!(
> +            agent.starts_with("git/"),
> +            r#"Expected user agent to start with "git/", got: {}"#,
> +            agent
> +        );
> +    }
> +
> +    #[test]
> +    fn sanitized_user_agent_starts_with_git() {
> +        let c_str = unsafe { CStr::from_ptr(libgit_user_agent_sanitized()) };
> +        let agent = c_str
> +            .to_str()
> +            .expect("Sanitized user agent contains invalid UTF-8 data");
> +        assert!(
> +            agent.starts_with("git/"),
> +            r#"Expected user agent to start with "git/", got: {}"#,
> +            agent
> +        );
> +    }
> +}
> diff --git a/t/Makefile b/t/Makefile
> index daa5fcae86..53ba01c21b 100644
> --- a/t/Makefile
> +++ b/t/Makefile
> @@ -177,3 +177,13 @@ perf:
>   
>   .PHONY: pre-clean $(T) aggregate-results clean valgrind perf \
>   	check-chainlint clean-chainlint test-chainlint $(UNIT_TESTS)
> +
> +.PHONY: libgit-sys-test
> +libgit-sys-test:
> +	$(QUIET)(\
> +		cd ../contrib/libgit-sys && \
> +		cargo test \
> +	)
> +ifdef INCLUDE_LIBGIT_RS
> +all:: libgit-sys-test
> +endif