| Message ID | 20240906222116.270196-5-calvinwan@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce libgit-rs, a Rust wrapper around libgit.a | expand |
Calvin Wan <calvinwan@google.com> writes: > diff --git a/contrib/libgit-rs/libgit-sys/public_symbol_export.c b/contrib/libgit-rs/libgit-sys/public_symbol_export.c > index 65d1620d28..07d6bfdd84 100644 > --- a/contrib/libgit-rs/libgit-sys/public_symbol_export.c > +++ b/contrib/libgit-rs/libgit-sys/public_symbol_export.c > @@ -33,6 +33,36 @@ int libgit_parse_maybe_bool(const char *val) > return git_parse_maybe_bool(val); > } > > +struct libgit_config_set *libgit_configset_alloc(void) > +{ > + return git_configset_alloc(); > +} git_configset_alloc() returns "struct config_set *" while this thing returns an incompatible pointer. Sent out an outdated version or something? This wouldn't have passed even a compile test, I suspect. $ make contrib/libgit-rs/libgit-sys/public_symbol_export.o CC contrib/libgit-rs/libgit-sys/public_symbol_export.o contrib/libgit-rs/libgit-sys/public_symbol_export.c: In function 'libgit_configset_alloc': contrib/libgit-rs/libgit-sys/public_symbol_export.c:38:16: error: returning 'struct config_set *' from a function with incompatible return type 'struct libgit_config_set *' [-Werror=incompatible-pointer-types] 38 | return git_configset_alloc(); | ^~~~~~~~~~~~~~~~~~~~~ contrib/libgit-rs/libgit-sys/public_symbol_export.c: In function 'libgit_configset_clear_and_free': contrib/libgit-rs/libgit-sys/public_symbol_export.c:43:38: error: passing argument 1 of 'git_configset_clear_and_free' from incompatible pointer type [-Werror=incompatible-pointer-types] 43 | git_configset_clear_and_free(cs); | ^~ | | | struct libgit_config_set * In file included from contrib/libgit-rs/libgit-sys/public_symbol_export.c:8: ./config.h:543:54: note: expected 'struct config_set *' but argument is of type 'struct libgit_config_set *' 543 | void git_configset_clear_and_free(struct config_set *cs); | ~~~~~~~~~~~~~~~~~~~^~ contrib/libgit-rs/libgit-sys/public_symbol_export.c: In function 'libgit_configset_init': contrib/libgit-rs/libgit-sys/public_symbol_export.c:48:28: error: passing argument 1 of 'git_configset_init' from incompatible pointer type [-Werror=incompatible-pointer-types] 48 | git_configset_init(cs); | ^~ | | | struct libgit_config_set * ./config.h:495:44: note: expected 'struct config_set *' but argument is of type 'struct libgit_config_set *' 495 | void git_configset_init(struct config_set *cs); | ~~~~~~~~~~~~~~~~~~~^~ contrib/libgit-rs/libgit-sys/public_symbol_export.c: In function 'libgit_configset_add_file': contrib/libgit-rs/libgit-sys/public_symbol_export.c:53:39: error: passing argument 1 of 'git_configset_add_file' from incompatible pointer type [-Werror=incompatible-pointer-types] 53 | return git_configset_add_file(cs, filename); | ^~ | | | struct libgit_config_set * ./config.h:504:47: note: expected 'struct config_set *' but argument is of type 'struct libgit_config_set *' 504 | int git_configset_add_file(struct config_set *cs, const char *filename); | ~~~~~~~~~~~~~~~~~~~^~ contrib/libgit-rs/libgit-sys/public_symbol_export.c: In function 'libgit_configset_get_int': contrib/libgit-rs/libgit-sys/public_symbol_export.c:58:38: error: passing argument 1 of 'git_configset_get_int' from incompatible pointer type [-Werror=incompatible-pointer-types] 58 | return git_configset_get_int(cs, key, dest); | ^~ | | | struct libgit_config_set * ./config.h:568:46: note: expected 'struct config_set *' but argument is of type 'struct libgit_config_set *' 568 | int git_configset_get_int(struct config_set *cs, const char *key, int *dest); | ~~~~~~~~~~~~~~~~~~~^~ contrib/libgit-rs/libgit-sys/public_symbol_export.c: In function 'libgit_configset_get_string': contrib/libgit-rs/libgit-sys/public_symbol_export.c:63:41: error: passing argument 1 of 'git_configset_get_string' from incompatible pointer type [-Werror=incompatible-pointer-types] 63 | return git_configset_get_string(cs, key, dest); | ^~ | | | struct libgit_config_set * ./config.h:567:49: note: expected 'struct config_set *' but argument is of type 'struct libgit_config_set *' 567 | int git_configset_get_string(struct config_set *cs, const char *key, char **dest); | ~~~~~~~~~~~~~~~~~~~^~ cc1: all warnings being treated as errors gmake: *** [Makefile:2802: contrib/libgit-rs/libgit-sys/public_symbol_export.o] Error 1
Junio C Hamano <gitster@pobox.com> writes: >> +struct libgit_config_set *libgit_configset_alloc(void) >> +{ >> + return git_configset_alloc(); >> +} > > git_configset_alloc() returns "struct config_set *" while this thing > returns an incompatible pointer. > > Sent out an outdated version or something? This wouldn't have > passed even a compile test, I suspect. The "shim" layer should hide the details of interfacing to the git proper from callers, as well as it should hide the callers' from the git proper. So if you really want to hide "struct config_set" from your library callers, you may need to do something like the attached, perhaps? At least this does pass compilation test. .../libgit-rs/libgit-sys/public_symbol_export.h | 11 +++++++++ .../libgit-rs/libgit-sys/public_symbol_export.c | 26 ++++++++++++---------- 2 files changed, 25 insertions(+), 12 deletions(-) diff --git c/contrib/libgit-rs/libgit-sys/public_symbol_export.h w/contrib/libgit-rs/libgit-sys/public_symbol_export.h index 3933698976..70701ca63e 100644 --- c/contrib/libgit-rs/libgit-sys/public_symbol_export.h +++ w/contrib/libgit-rs/libgit-sys/public_symbol_export.h @@ -1,6 +1,17 @@ #ifndef PUBLIC_SYMBOL_EXPORT_H #define PUBLIC_SYMBOL_EXPORT_H +/* shim */ +struct libgit_config_set { + struct config_set cs; + /* + * the shim layer may want to add more members below, but then + * it may need to wrap underlying config_set differently, + * e.g., store a pointer of an allocated config_set in this + * shim structure. + */ +}; + const char *libgit_setup_git_directory(void); int libgit_config_get_int(const char *key, int *dest); diff --git c/contrib/libgit-rs/libgit-sys/public_symbol_export.c w/contrib/libgit-rs/libgit-sys/public_symbol_export.c index 07d6bfdd84..6f5eb3b249 100644 --- c/contrib/libgit-rs/libgit-sys/public_symbol_export.c +++ w/contrib/libgit-rs/libgit-sys/public_symbol_export.c @@ -3,12 +3,13 @@ // 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" +#include "contrib/libgit-rs/libgit-sys/public_symbol_export.h" + extern struct repository *the_repository; #pragma GCC visibility push(default) @@ -35,32 +36,33 @@ int libgit_parse_maybe_bool(const char *val) struct libgit_config_set *libgit_configset_alloc(void) { - return git_configset_alloc(); + void *cs = git_configset_alloc(); + return (struct libgit_config_set *)cs; } -void libgit_configset_clear_and_free(struct libgit_config_set *cs) +void libgit_configset_clear_and_free(struct libgit_config_set *lgcs) { - git_configset_clear_and_free(cs); + git_configset_clear_and_free(&lgcs->cs); } -void libgit_configset_init(struct libgit_config_set *cs) +void libgit_configset_init(struct libgit_config_set *lgcs) { - git_configset_init(cs); + git_configset_init(&lgcs->cs); } -int libgit_configset_add_file(struct libgit_config_set *cs, const char *filename) +int libgit_configset_add_file(struct libgit_config_set *lgcs, const char *filename) { - return git_configset_add_file(cs, filename); + return git_configset_add_file(&lgcs->cs, filename); } -int libgit_configset_get_int(struct libgit_config_set *cs, const char *key, int *dest) +int libgit_configset_get_int(struct libgit_config_set *lgcs, const char *key, int *dest) { - return git_configset_get_int(cs, key, dest); + return git_configset_get_int(&lgcs->cs, key, dest); } -int libgit_configset_get_string(struct libgit_config_set *cs, const char *key, char **dest) +int libgit_configset_get_string(struct libgit_config_set *lgcs, const char *key, char **dest) { - return git_configset_get_string(cs, key, dest); + return git_configset_get_string(&lgcs->cs, key, dest); } const char *libgit_user_agent(void)
On Mon, Sep 9, 2024 at 1:47 PM Junio C Hamano <gitster@pobox.com> wrote: > > Junio C Hamano <gitster@pobox.com> writes: > > >> +struct libgit_config_set *libgit_configset_alloc(void) > >> +{ > >> + return git_configset_alloc(); > >> +} > > > > git_configset_alloc() returns "struct config_set *" while this thing > > returns an incompatible pointer. > > > > Sent out an outdated version or something? This wouldn't have > > passed even a compile test, I suspect. > > The "shim" layer should hide the details of interfacing to the git > proper from callers, as well as it should hide the callers' from the > git proper. So if you really want to hide "struct config_set" from > your library callers, you may need to do something like the > attached, perhaps? At least this does pass compilation test. You're correct that this is supposed to be the shim layer, where callers aren't supposed to be able to directly interface with the config_set struct. From Rust's perspective, all pointers that come back from C code are effectively void* so the naming doesn't make a functional difference for our use case (and consequently these warnings did not show up from the build.rs script nor did the tests fail). However, I agree that the public interface should pass the compilation test and your approach does that -- will reroll with those changes and I believe that we should also fix the build.rs so that warnings also show up during cargo build.
Calvin Wan <calvinwan@google.com> writes: > However, I agree that the public interface should pass the > compilation test and your approach does that -- will reroll with those > changes and I believe that we should also fix the build.rs so that > warnings also show up during cargo build. Thanks. I couldn't quite tell if *.c was supposed to be compilable into *.o directly (if not, then Makefile needs fixing), and I am OK, if the shim layer is only internally used, if the public.h defined all pointers as "void *". I wasn't happy to cast "struct config_set *" between "struct libgit_config_set *" merely because one embeds the other without adding any other member. If they have to be bit-for-bit identical, shouldn't we just use the real name of the struct everywhere? The situation might become different if you address others' comments like "why do we want allocation-only interface exposed? shoudln't it also initialize?" etc. Thanks.
On 2024.09.10 12:04, Calvin Wan wrote: > On Mon, Sep 9, 2024 at 1:47 PM Junio C Hamano <gitster@pobox.com> wrote: > > > > Junio C Hamano <gitster@pobox.com> writes: > > > > >> +struct libgit_config_set *libgit_configset_alloc(void) > > >> +{ > > >> + return git_configset_alloc(); > > >> +} > > > > > > git_configset_alloc() returns "struct config_set *" while this thing > > > returns an incompatible pointer. > > > > > > Sent out an outdated version or something? This wouldn't have > > > passed even a compile test, I suspect. > > > > The "shim" layer should hide the details of interfacing to the git > > proper from callers, as well as it should hide the callers' from the > > git proper. So if you really want to hide "struct config_set" from > > your library callers, you may need to do something like the > > attached, perhaps? At least this does pass compilation test. > > You're correct that this is supposed to be the shim layer, where > callers aren't supposed to be able to directly interface with the > config_set struct. From Rust's perspective, all pointers that come > back from C code are effectively void* so the naming doesn't make a > functional difference for our use case (and consequently these > warnings did not show up from the build.rs script nor did the tests > fail). However, I agree that the public interface should pass the > compilation test and your approach does that -- will reroll with those > changes and I believe that we should also fix the build.rs so that > warnings also show up during cargo build. Yeah, cargo will complain if DEVELOPER=1 is set. We should probably enforce that in build.rs.
On 2024.09.10 12:14, Junio C Hamano wrote: > Calvin Wan <calvinwan@google.com> writes: > > > However, I agree that the public interface should pass the > > compilation test and your approach does that -- will reroll with those > > changes and I believe that we should also fix the build.rs so that > > warnings also show up during cargo build. > > Thanks. I couldn't quite tell if *.c was supposed to be compilable > into *.o directly (if not, then Makefile needs fixing), and I am OK, > if the shim layer is only internally used, if the public.h defined > all pointers as "void *". > > I wasn't happy to cast "struct config_set *" between "struct > libgit_config_set *" merely because one embeds the other without > adding any other member. If they have to be bit-for-bit identical, > shouldn't we just use the real name of the struct everywhere? We want to namespace types as well as functions, as Phillip pointed out in 47b18fa4-f01b-4f42-8d04-9e145515ccc1@gmail.com. Is there a reason why we need the shim struct from your xmqqcylcpnah.fsf@gitster.g and can't just cast directly like so: contrib/libgit-rs/libgit-sys/public_symbol_export.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/contrib/libgit-rs/libgit-sys/public_symbol_export.c b/contrib/libgit-rs/libgit-sys/public_symbol_export.c index 07d6bfdd84..c96fa15ab6 100644 --- a/contrib/libgit-rs/libgit-sys/public_symbol_export.c +++ b/contrib/libgit-rs/libgit-sys/public_symbol_export.c @@ -3,12 +3,13 @@ // 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" +#include "contrib/libgit-rs/libgit-sys/public_symbol_export.h" + extern struct repository *the_repository; #pragma GCC visibility push(default) @@ -35,32 +36,32 @@ int libgit_parse_maybe_bool(const char *val) struct libgit_config_set *libgit_configset_alloc(void) { - return git_configset_alloc(); + return (struct libgit_config_set *) git_configset_alloc(); } void libgit_configset_clear_and_free(struct libgit_config_set *cs) { - git_configset_clear_and_free(cs); + git_configset_clear_and_free((struct config_set *) cs); } void libgit_configset_init(struct libgit_config_set *cs) { - git_configset_init(cs); + git_configset_init((struct config_set *) cs); } int libgit_configset_add_file(struct libgit_config_set *cs, const char *filename) { - return git_configset_add_file(cs, filename); + return git_configset_add_file((struct config_set *) cs, filename); } int libgit_configset_get_int(struct libgit_config_set *cs, const char *key, int *dest) { - return git_configset_get_int(cs, key, dest); + return git_configset_get_int((struct config_set *) cs, key, dest); } int libgit_configset_get_string(struct libgit_config_set *cs, const char *key, char **dest) { - return git_configset_get_string(cs, key, dest); + return git_configset_get_string((struct config_set *) cs, key, dest); } const char *libgit_user_agent(void)
Josh Steadmon <steadmon@google.com> writes: > We want to namespace types as well as functions, as Phillip pointed out > in 47b18fa4-f01b-4f42-8d04-9e145515ccc1@gmail.com. > > Is there a reason why we need the shim struct from your > xmqqcylcpnah.fsf@gitster.g and can't just cast directly like so: > ... > int libgit_configset_get_string(struct libgit_config_set *cs, const char *key, char **dest) > { > - return git_configset_get_string(cs, key, dest); > + return git_configset_get_string((struct config_set *) cs, key, dest); > } Not at all. I just didn't see your intentions in the patch if all you wanted to do was merely to flip names, or wanted to leave the possibility to allow the wrapped ones to optionally have different shape (e.g. for bookkeeping purposes for either the host environment or the shim layer). If it is merely "we do not want to expose these names but we want bit-for-bit identical data", then you do not need extra logic at all---the casts would be suffficient[*]. PS. I am not feeling well today, so please expect delayed and/or sparse responses. [Footnote] * Building objects that go to libgit.a, partially linking them to resolve internal references, and then rewriting the symbol table of the resulting relocatable object file to expose only the entry points and data you want to show to the rust world to whatever names you want, would be a less gross solution, I would imagine. You only then need to write a (fake) public_symbol_export.h file that will never has to be seen on the C side, but to be seen as the header to describe that C library to the rust side (and obviously you do not need public_symbol_export.c file only to keep these casts).
On 2024.09.18 09:34, Junio C Hamano wrote: > Josh Steadmon <steadmon@google.com> writes: > > > We want to namespace types as well as functions, as Phillip pointed out > > in 47b18fa4-f01b-4f42-8d04-9e145515ccc1@gmail.com. > > > > Is there a reason why we need the shim struct from your > > xmqqcylcpnah.fsf@gitster.g and can't just cast directly like so: > > ... > > int libgit_configset_get_string(struct libgit_config_set *cs, const char *key, char **dest) > > { > > - return git_configset_get_string(cs, key, dest); > > + return git_configset_get_string((struct config_set *) cs, key, dest); > > } > > Not at all. I just didn't see your intentions in the patch if all > you wanted to do was merely to flip names, or wanted to leave the > possibility to allow the wrapped ones to optionally have different > shape (e.g. for bookkeeping purposes for either the host environment > or the shim layer). If it is merely "we do not want to expose these > names but we want bit-for-bit identical data", then you do not need > extra logic at all---the casts would be suffficient[*]. > > PS. I am not feeling well today, so please expect delayed and/or > sparse responses. > > > [Footnote] > > * Building objects that go to libgit.a, partially linking them to > resolve internal references, and then rewriting the symbol table > of the resulting relocatable object file to expose only the entry > points and data you want to show to the rust world to whatever > names you want, would be a less gross solution, I would imagine. > You only then need to write a (fake) public_symbol_export.h file > that will never has to be seen on the C side, but to be seen as > the header to describe that C library to the rust side (and > obviously you do not need public_symbol_export.c file only to keep > these casts). I don't think I'll have time to experiment with this approach for V4, but I'll put it on our list of future work. For now I'm just keeping public_symbol_export.{c,h} as-is and casting to avoid exposing the unmangled struct name.
diff --git a/.gitignore b/.gitignore index dfd72820fb..0a42f27117 100644 --- a/.gitignore +++ b/.gitignore @@ -248,4 +248,5 @@ Release/ /git.VC.db *.dSYM /contrib/buildsystems/out +/contrib/libgit-rs/target /contrib/libgit-rs/libgit-sys/target diff --git a/Makefile b/Makefile index 0090514e55..abeee01d9e 100644 --- a/Makefile +++ b/Makefile @@ -3723,7 +3723,7 @@ clean: profile-clean coverage-clean cocciclean $(RM) $(htmldocs).tar.gz $(manpages).tar.gz $(MAKE) -C Documentation/ clean $(RM) Documentation/GIT-EXCLUDED-PROGRAMS - $(RM) -r contrib/libgit-rs/libgit-sys/target + $(RM) -r contrib/libgit-rs/target contrib/libgit-rs/libgit-sys/target ifndef NO_PERL $(RM) -r perl/build/ endif diff --git a/contrib/libgit-rs/Cargo.lock b/contrib/libgit-rs/Cargo.lock new file mode 100644 index 0000000000..187176f5df --- /dev/null +++ b/contrib/libgit-rs/Cargo.lock @@ -0,0 +1,62 @@ +# This file is automatically @generated by Cargo. +# It is not intended for manual editing. +version = 3 + +[[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 = [ + "libgit-sys", +] + +[[package]] +name = "libgit-sys" +version = "0.1.0" +dependencies = [ + "libz-sys", +] + +[[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 = "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" diff --git a/contrib/libgit-rs/Cargo.toml b/contrib/libgit-rs/Cargo.toml new file mode 100644 index 0000000000..b8914517e0 --- /dev/null +++ b/contrib/libgit-rs/Cargo.toml @@ -0,0 +1,10 @@ +[package] +name = "libgit" +version = "0.1.0" +edition = "2021" + +[lib] +path = "src/lib.rs" + +[dependencies] +libgit-sys = { version = "0.1.0", path = "libgit-sys" } diff --git a/contrib/libgit-rs/libgit-sys/public_symbol_export.c b/contrib/libgit-rs/libgit-sys/public_symbol_export.c index 65d1620d28..07d6bfdd84 100644 --- a/contrib/libgit-rs/libgit-sys/public_symbol_export.c +++ b/contrib/libgit-rs/libgit-sys/public_symbol_export.c @@ -33,6 +33,36 @@ int libgit_parse_maybe_bool(const char *val) return git_parse_maybe_bool(val); } +struct libgit_config_set *libgit_configset_alloc(void) +{ + return git_configset_alloc(); +} + +void libgit_configset_clear_and_free(struct libgit_config_set *cs) +{ + git_configset_clear_and_free(cs); +} + +void libgit_configset_init(struct libgit_config_set *cs) +{ + git_configset_init(cs); +} + +int libgit_configset_add_file(struct libgit_config_set *cs, const char *filename) +{ + return git_configset_add_file(cs, filename); +} + +int libgit_configset_get_int(struct libgit_config_set *cs, const char *key, int *dest) +{ + return git_configset_get_int(cs, key, dest); +} + +int libgit_configset_get_string(struct libgit_config_set *cs, const char *key, char **dest) +{ + return git_configset_get_string(cs, key, dest); +} + 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 64332f30de..3933698976 100644 --- a/contrib/libgit-rs/libgit-sys/public_symbol_export.h +++ b/contrib/libgit-rs/libgit-sys/public_symbol_export.h @@ -9,6 +9,18 @@ void libgit_init_git(const char **argv); int libgit_parse_maybe_bool(const char *val); +struct libgit_config_set *libgit_configset_alloc(void); + +void libgit_configset_clear_and_free(struct libgit_config_set *cs); + +void libgit_configset_init(struct libgit_config_set *cs); + +int libgit_configset_add_file(struct libgit_config_set *cs, const char *filename); + +int libgit_configset_get_int(struct libgit_config_set *cs, const char *key, int *dest); + +int libgit_configset_get_string(struct libgit_config_set *cs, const char *key, char **dest); + 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 9b2e85dff8..f3eb44a238 100644 --- a/contrib/libgit-rs/libgit-sys/src/lib.rs +++ b/contrib/libgit-rs/libgit-sys/src/lib.rs @@ -1,8 +1,17 @@ -use std::ffi::{c_char, c_int}; +use std::ffi::{c_char, c_int, c_void}; + +#[allow(non_camel_case_types)] +#[repr(C)] +pub struct libgit_config_set { + _data: [u8; 0], + _marker: core::marker::PhantomData<(*mut u8, core::marker::PhantomPinned)>, +} extern crate libz_sys; extern "C" { + pub fn free(ptr: *mut c_void); + pub fn libgit_setup_git_directory() -> *const c_char; // From config.c @@ -17,6 +26,26 @@ extern "C" { // From version.c pub fn libgit_user_agent() -> *const c_char; pub fn libgit_user_agent_sanitized() -> *const c_char; + + pub fn libgit_configset_alloc() -> *mut libgit_config_set; + pub fn libgit_configset_clear_and_free(cs: *mut libgit_config_set); + + pub fn libgit_configset_init(cs: *mut libgit_config_set); + + pub fn libgit_configset_add_file(cs: *mut libgit_config_set, filename: *const c_char) -> c_int; + + pub fn libgit_configset_get_int( + cs: *mut libgit_config_set, + key: *const c_char, + int: *mut c_int, + ) -> c_int; + + pub fn libgit_configset_get_string( + cs: *mut libgit_config_set, + key: *const c_char, + dest: *mut *mut c_char, + ) -> c_int; + } #[cfg(test)] diff --git a/contrib/libgit-rs/src/lib.rs b/contrib/libgit-rs/src/lib.rs new file mode 100644 index 0000000000..43aae09656 --- /dev/null +++ b/contrib/libgit-rs/src/lib.rs @@ -0,0 +1,85 @@ +use std::ffi::{c_char, c_int, c_void, CStr, CString}; +use std::path::Path; + +use libgit_sys::*; + +pub struct ConfigSet(*mut libgit_config_set); +impl ConfigSet { + pub fn new() -> Self { + unsafe { + let ptr = libgit_configset_alloc(); + libgit_configset_init(ptr); + ConfigSet(ptr) + } + } + + pub fn add_files(&mut self, files: &[&Path]) { + for file in files { + let pstr: &str = file.to_str().expect("Invalid UTF-8"); + let rs = CString::new(&*pstr).expect("Couldn't convert to CString"); + unsafe { + libgit_configset_add_file(self.0, rs.as_ptr()); + } + } + } + + pub fn get_int(&mut self, key: &str) -> Option<c_int> { + let key = CString::new(key).expect("Couldn't convert to CString"); + let mut val: c_int = 0; + unsafe { + if libgit_configset_get_int(self.0, key.as_ptr(), &mut val as *mut c_int) != 0 { + return None; + } + } + + Some(val) + } + + pub fn get_str(&mut self, key: &str) -> Option<CString> { + let key = CString::new(key).expect("Couldn't convert to CString"); + let mut val: *mut c_char = std::ptr::null_mut(); + unsafe { + if libgit_configset_get_string(self.0, key.as_ptr(), &mut val as *mut *mut c_char) != 0 + { + return None; + } + let borrowed_str = CStr::from_ptr(val); + let owned_str = CString::from_vec_with_nul(borrowed_str.to_bytes_with_nul().to_vec()); + free(val as *mut c_void); // Free the xstrdup()ed pointer from the C side + Some(owned_str.unwrap()) + } + } +} + +impl Default for ConfigSet { + fn default() -> Self { + Self::new() + } +} + +impl Drop for ConfigSet { + fn drop(&mut self) { + unsafe { + libgit_configset_clear_and_free(self.0); + } + } +} + +#[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_str("foo.bar"), None); + } +} diff --git a/contrib/libgit-rs/testdata/config1 b/contrib/libgit-rs/testdata/config1 new file mode 100644 index 0000000000..4e9a9d25d1 --- /dev/null +++ b/contrib/libgit-rs/testdata/config1 @@ -0,0 +1,2 @@ +[trace2] + eventNesting = 1 diff --git a/contrib/libgit-rs/testdata/config2 b/contrib/libgit-rs/testdata/config2 new file mode 100644 index 0000000000..b8d1eca423 --- /dev/null +++ b/contrib/libgit-rs/testdata/config2 @@ -0,0 +1,2 @@ +[trace2] + eventTarget = 1 diff --git a/contrib/libgit-rs/testdata/config3 b/contrib/libgit-rs/testdata/config3 new file mode 100644 index 0000000000..ca7b9a7c38 --- /dev/null +++ b/contrib/libgit-rs/testdata/config3 @@ -0,0 +1,2 @@ +[trace2] + eventNesting = 3
Wrap `struct config_set` and a few of its associated functions in libgit-sys. Also introduce a higher-level "libgit" crate which provides a more Rust-friendly interface to config_set structs. Co-authored-by: Josh Steadmon <steadmon@google.com> Signed-off-by: Calvin Wan <calvinwan@google.com> Change-Id: If85e77809434f0ef05e2db35132ecf84621e10ef --- .gitignore | 1 + Makefile | 2 +- contrib/libgit-rs/Cargo.lock | 62 ++++++++++++++ contrib/libgit-rs/Cargo.toml | 10 +++ .../libgit-sys/public_symbol_export.c | 30 +++++++ .../libgit-sys/public_symbol_export.h | 12 +++ contrib/libgit-rs/libgit-sys/src/lib.rs | 31 ++++++- contrib/libgit-rs/src/lib.rs | 85 +++++++++++++++++++ contrib/libgit-rs/testdata/config1 | 2 + contrib/libgit-rs/testdata/config2 | 2 + contrib/libgit-rs/testdata/config3 | 2 + 11 files changed, 237 insertions(+), 2 deletions(-) create mode 100644 contrib/libgit-rs/Cargo.lock create mode 100644 contrib/libgit-rs/Cargo.toml create mode 100644 contrib/libgit-rs/src/lib.rs create mode 100644 contrib/libgit-rs/testdata/config1 create mode 100644 contrib/libgit-rs/testdata/config2 create mode 100644 contrib/libgit-rs/testdata/config3