| Message ID | 20240906222116.270196-3-calvinwan@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce libgit-rs, a Rust wrapper around libgit.a | expand |
On Fri, Sep 6, 2024 at 6:21 PM Calvin Wan <calvinwan@google.com> wrote: > Wrap a few repo setup and config access functions in libgit-sys. These > were selected as proof-of-concept items to show that we can access local > config from Rust. > > Co-authored-by: Josh Steadmon <steadmon@google.com> > Signed-off-by: Calvin Wan <calvinwan@google.com> Josh's sign-off is missing? > --- > diff --git a/contrib/libgit-rs/libgit-sys/src/lib.rs b/contrib/libgit-rs/libgit-sys/src/lib.rs > @@ -1,8 +1,19 @@ > 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; Perhaps add a comment above libgit_setup_git_directory() stating its origin, as you do for the other functions mentioned here? // From setup.c pub fn libgit_setup_git_directory() -> *const c_char; (Nit: I would probably drop the word "From" from these comments, as it doesn't seem to add value and ends up being noise. Even better, drop the comments altogether since they don't really add value and can easily become outdated if code is ever moved around.) > + > + // From common-init.c > + pub fn libgit_init_git(argv: *const *const c_char); > + > + // From parse.c > + pub fn libgit_parse_maybe_bool(val: *const c_char) -> c_int;
Calvin Wan <calvinwan@google.com> writes: > Wrap a few repo setup and config access functions in libgit-sys. These > were selected as proof-of-concept items to show that we can access local > config from Rust. > > Co-authored-by: Josh Steadmon <steadmon@google.com> > Signed-off-by: Calvin Wan <calvinwan@google.com> > Change-Id: I6dd886af8c63e1f0f3251064cd8903aecdf768bb > --- Common to all other steps, I suspect that you meant to but forgot to strip out the Change-Id: thing here? Also there are a few whitespace breakages in this step. Applying: libgit-sys: add repo initialization and config access .git/rebase-apply/patch:129: trailing whitespace. * for the existence of a key rather than a specific value .git/rebase-apply/patch:138: trailing whitespace. unsafe { .git/rebase-apply/patch:143: trailing whitespace. unsafe { warning: 3 lines add whitespace errors. There is another one in a later step. Applying: libgit: add higher-level libgit crate .git/rebase-apply/patch:325: trailing whitespace. // ConfigSet retrieves correct value warning: 1 line adds whitespace errors.
On Fri, Sep 06, 2024 at 10:21:13PM +0000, Calvin Wan wrote: > diff --git a/contrib/libgit-rs/libgit-sys/public_symbol_export.c b/contrib/libgit-rs/libgit-sys/public_symbol_export.c > index 39c27d9c1a..65d1620d28 100644 > --- a/contrib/libgit-rs/libgit-sys/public_symbol_export.c > +++ b/contrib/libgit-rs/libgit-sys/public_symbol_export.c > @@ -2,11 +2,37 @@ > // original symbols can be hidden. Renaming these with a "libgit_" prefix also > // avoid conflicts with other libraries such as libgit2. > > +#include "git-compat-util.h" > #include "contrib/libgit-rs/libgit-sys/public_symbol_export.h" > +#include "common-init.h" > +#include "config.h" > +#include "setup.h" > #include "version.h" > > +extern struct repository *the_repository; > + > #pragma GCC visibility push(default) > > +const char *libgit_setup_git_directory(void) > +{ > + return setup_git_directory(); > +} > + > +int libgit_config_get_int(const char *key, int *dest) > +{ > + return repo_config_get_int(the_repository, key, dest); > +} > + > +void libgit_init_git(const char **argv) > +{ > + init_git(argv); > +} > + > +int libgit_parse_maybe_bool(const char *val) > +{ > + return git_parse_maybe_bool(val); > +} > + I don't quite get why we expose functionality that is inherently not libified. Exposing the current state to library users certainly does not feel right to me, doubly so because `the_repository` is deprecated and will eventually go away. So we already know that we'll have to break the API here once that has happened. I'd rather want to see that introducing the library makes us double down on providing properly encapsulated interfaces from hereon. Also, we already have ways to initialize a repository and read their config without relying on `the_repository`. So shouldn't we expose that instead? Patrick
On 2024.09.06 18:53, Eric Sunshine wrote: > On Fri, Sep 6, 2024 at 6:21 PM Calvin Wan <calvinwan@google.com> wrote: > > Wrap a few repo setup and config access functions in libgit-sys. These > > were selected as proof-of-concept items to show that we can access local > > config from Rust. > > > > Co-authored-by: Josh Steadmon <steadmon@google.com> > > Signed-off-by: Calvin Wan <calvinwan@google.com> > > Josh's sign-off is missing? > > > --- > > diff --git a/contrib/libgit-rs/libgit-sys/src/lib.rs b/contrib/libgit-rs/libgit-sys/src/lib.rs > > @@ -1,8 +1,19 @@ > > 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; > > Perhaps add a comment above libgit_setup_git_directory() stating its > origin, as you do for the other functions mentioned here? > > // From setup.c > pub fn libgit_setup_git_directory() -> *const c_char; > > (Nit: I would probably drop the word "From" from these comments, as it > doesn't seem to add value and ends up being noise. Even better, drop > the comments altogether since they don't really add value and can > easily become outdated if code is ever moved around.) Removed in V4. > > + > > + // From common-init.c > > + pub fn libgit_init_git(argv: *const *const c_char); > > + > > + // From parse.c > > + pub fn libgit_parse_maybe_bool(val: *const c_char) -> c_int;
On 2024.09.06 16:45, Junio C Hamano wrote: > Calvin Wan <calvinwan@google.com> writes: > > > Wrap a few repo setup and config access functions in libgit-sys. These > > were selected as proof-of-concept items to show that we can access local > > config from Rust. > > > > Co-authored-by: Josh Steadmon <steadmon@google.com> > > Signed-off-by: Calvin Wan <calvinwan@google.com> > > Change-Id: I6dd886af8c63e1f0f3251064cd8903aecdf768bb > > --- > > > Common to all other steps, I suspect that you meant to but forgot to > strip out the Change-Id: thing here? > > Also there are a few whitespace breakages in this step. > > Applying: libgit-sys: add repo initialization and config access > .git/rebase-apply/patch:129: trailing whitespace. > * for the existence of a key rather than a specific value > .git/rebase-apply/patch:138: trailing whitespace. > unsafe { > .git/rebase-apply/patch:143: trailing whitespace. > unsafe { > warning: 3 lines add whitespace errors. > > > There is another one in a later step. > > Applying: libgit: add higher-level libgit crate > .git/rebase-apply/patch:325: trailing whitespace. > // ConfigSet retrieves correct value > warning: 1 line adds whitespace errors. Fixed the whitespace errors in V4.
On 2024.09.10 08:42, Patrick Steinhardt wrote: > On Fri, Sep 06, 2024 at 10:21:13PM +0000, Calvin Wan wrote: > > diff --git a/contrib/libgit-rs/libgit-sys/public_symbol_export.c b/contrib/libgit-rs/libgit-sys/public_symbol_export.c > > index 39c27d9c1a..65d1620d28 100644 > > --- a/contrib/libgit-rs/libgit-sys/public_symbol_export.c > > +++ b/contrib/libgit-rs/libgit-sys/public_symbol_export.c > > @@ -2,11 +2,37 @@ > > // original symbols can be hidden. Renaming these with a "libgit_" prefix also > > // avoid conflicts with other libraries such as libgit2. > > > > +#include "git-compat-util.h" > > #include "contrib/libgit-rs/libgit-sys/public_symbol_export.h" > > +#include "common-init.h" > > +#include "config.h" > > +#include "setup.h" > > #include "version.h" > > > > +extern struct repository *the_repository; > > + > > #pragma GCC visibility push(default) > > > > +const char *libgit_setup_git_directory(void) > > +{ > > + return setup_git_directory(); > > +} > > + > > +int libgit_config_get_int(const char *key, int *dest) > > +{ > > + return repo_config_get_int(the_repository, key, dest); > > +} > > + > > +void libgit_init_git(const char **argv) > > +{ > > + init_git(argv); > > +} > > + > > +int libgit_parse_maybe_bool(const char *val) > > +{ > > + return git_parse_maybe_bool(val); > > +} > > + > > I don't quite get why we expose functionality that is inherently not > libified. Exposing the current state to library users certainly does not > feel right to me, doubly so because `the_repository` is deprecated and > will eventually go away. So we already know that we'll have to break the > API here once that has happened. I'd rather want to see that introducing > the library makes us double down on providing properly encapsulated > interfaces from hereon. > > Also, we already have ways to initialize a repository and read their > config without relying on `the_repository`. So shouldn't we expose that > instead? > > Patrick Specifically in this case, yeah, we should have started with the libified version. This is an artifact due to the way we are figuring out C/Rust interop as we go, and it was easier to convert it this way without making the Rust side care about handling repository pointers. But you're right, and I'll fix this soon for V4. In general though, we're treating the initial version of libgit-rs as a proof-of-concept that we can call from JJ into Git without blowing things up. We might not always have the option of exposing fully-libified code paths, and for our $DAYJOB purposes, we may have to deal with non-ideal interfaces for the early versions. However, we do want to use this as a way to motivate development of better, libified APIs when we find real-world use cases for them. We've said before (even before we started libgit-rs) that we're not going to be able to make guarantees about early libified APIs, because we're learning as we go along. I don't want to use that as an excuse to cover up bad design, but I do want to be upfront that we can't commit to fully libifying any given code path before we expose it through the shim library or through libgit-rs.
On 2024.10.07 14:21, Josh Steadmon wrote: > On 2024.09.10 08:42, Patrick Steinhardt wrote: > > I don't quite get why we expose functionality that is inherently not > > libified. Exposing the current state to library users certainly does not > > feel right to me, doubly so because `the_repository` is deprecated and > > will eventually go away. So we already know that we'll have to break the > > API here once that has happened. I'd rather want to see that introducing > > the library makes us double down on providing properly encapsulated > > interfaces from hereon. > > > > Also, we already have ways to initialize a repository and read their > > config without relying on `the_repository`. So shouldn't we expose that > > instead? > > > > Patrick > > Specifically in this case, yeah, we should have started with the > libified version. This is an artifact due to the way we are figuring out > C/Rust interop as we go, and it was easier to convert it this way > without making the Rust side care about handling repository pointers. > But you're right, and I'll fix this soon for V4. > > In general though, we're treating the initial version of libgit-rs as a > proof-of-concept that we can call from JJ into Git without blowing > things up. We might not always have the option of exposing > fully-libified code paths, and for our $DAYJOB purposes, we may have to > deal with non-ideal interfaces for the early versions. However, we do > want to use this as a way to motivate development of better, libified > APIs when we find real-world use cases for them. > > We've said before (even before we started libgit-rs) that we're not > going to be able to make guarantees about early libified APIs, because > we're learning as we go along. I don't want to use that as an excuse to > cover up bad design, but I do want to be upfront that we can't commit to > fully libifying any given code path before we expose it through the shim > library or through libgit-rs. In fact, since the JJ proof-of-concept doesn't rely on repository initialization or repository-level config access, I think we can just drop this patch for now and worry about getting a better interface for repo initialization later.
diff --git a/contrib/libgit-rs/libgit-sys/public_symbol_export.c b/contrib/libgit-rs/libgit-sys/public_symbol_export.c index 39c27d9c1a..65d1620d28 100644 --- a/contrib/libgit-rs/libgit-sys/public_symbol_export.c +++ b/contrib/libgit-rs/libgit-sys/public_symbol_export.c @@ -2,11 +2,37 @@ // original symbols can be hidden. Renaming these with a "libgit_" prefix also // avoid conflicts with other libraries such as libgit2. +#include "git-compat-util.h" #include "contrib/libgit-rs/libgit-sys/public_symbol_export.h" +#include "common-init.h" +#include "config.h" +#include "setup.h" #include "version.h" +extern struct repository *the_repository; + #pragma GCC visibility push(default) +const char *libgit_setup_git_directory(void) +{ + return setup_git_directory(); +} + +int libgit_config_get_int(const char *key, int *dest) +{ + return repo_config_get_int(the_repository, key, dest); +} + +void libgit_init_git(const char **argv) +{ + init_git(argv); +} + +int libgit_parse_maybe_bool(const char *val) +{ + return git_parse_maybe_bool(val); +} + const char *libgit_user_agent(void) { return git_user_agent(); diff --git a/contrib/libgit-rs/libgit-sys/public_symbol_export.h b/contrib/libgit-rs/libgit-sys/public_symbol_export.h index a3372f93fa..64332f30de 100644 --- a/contrib/libgit-rs/libgit-sys/public_symbol_export.h +++ b/contrib/libgit-rs/libgit-sys/public_symbol_export.h @@ -1,6 +1,14 @@ #ifndef PUBLIC_SYMBOL_EXPORT_H #define PUBLIC_SYMBOL_EXPORT_H +const char *libgit_setup_git_directory(void); + +int libgit_config_get_int(const char *key, int *dest); + +void libgit_init_git(const char **argv); + +int libgit_parse_maybe_bool(const char *val); + const char *libgit_user_agent(void); const char *libgit_user_agent_sanitized(void); diff --git a/contrib/libgit-rs/libgit-sys/src/lib.rs b/contrib/libgit-rs/libgit-sys/src/lib.rs index 346b26083d..9b2e85dff8 100644 --- a/contrib/libgit-rs/libgit-sys/src/lib.rs +++ b/contrib/libgit-rs/libgit-sys/src/lib.rs @@ -1,8 +1,19 @@ -use std::ffi::c_char; +use std::ffi::{c_char, c_int}; extern crate libz_sys; 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 common-init.c + pub fn libgit_init_git(argv: *const *const c_char); + + // 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; @@ -10,7 +21,7 @@ extern "C" { #[cfg(test)] mod tests { - use std::ffi::CStr; + use std::ffi::{CStr, CString}; use super::*; @@ -39,4 +50,46 @@ mod tests { agent ); } + + #[test] + fn parse_bools_from_strings() { + let arg = CString::new("true").unwrap(); + assert_eq!(unsafe { libgit_parse_maybe_bool(arg.as_ptr()) }, 1); + + let arg = CString::new("yes").unwrap(); + assert_eq!(unsafe { libgit_parse_maybe_bool(arg.as_ptr()) }, 1); + + let arg = CString::new("false").unwrap(); + assert_eq!(unsafe { libgit_parse_maybe_bool(arg.as_ptr()) }, 0); + + let arg = CString::new("no").unwrap(); + assert_eq!(unsafe { libgit_parse_maybe_bool(arg.as_ptr()) }, 0); + + let arg = CString::new("maybe").unwrap(); + assert_eq!(unsafe { libgit_parse_maybe_bool(arg.as_ptr()) }, -1); + } + + #[test] + fn access_configs() { + /* + * Since we can't easily set a config for the local repo, test + * for the existence of a key rather than a specific value + */ + let fake_argv = [std::ptr::null::<c_char>()]; + unsafe { + libgit_init_git(fake_argv.as_ptr()); + libgit_setup_git_directory(); + } + let mut dest: c_int = 0; + let key = CString::new("core.repositoryformatversion").unwrap(); + unsafe { + let val = libgit_config_get_int(key.as_ptr(), &mut dest as *mut i32); + assert_eq!(val, 0) + }; + let missing_key = CString::new("foo.bar").unwrap(); + unsafe { + let val = libgit_config_get_int(missing_key.as_ptr(), &mut dest as *mut i32); + assert_eq!(val, 1) + }; + } }
Wrap a few repo setup and config access functions in libgit-sys. These were selected as proof-of-concept items to show that we can access local config from Rust. Co-authored-by: Josh Steadmon <steadmon@google.com> Signed-off-by: Calvin Wan <calvinwan@google.com> Change-Id: I6dd886af8c63e1f0f3251064cd8903aecdf768bb --- .../libgit-sys/public_symbol_export.c | 26 +++++++++ .../libgit-sys/public_symbol_export.h | 8 +++ contrib/libgit-rs/libgit-sys/src/lib.rs | 57 ++++++++++++++++++- 3 files changed, 89 insertions(+), 2 deletions(-)