| Message ID | cover.1738101256.git.steadmon@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce libgit-rs, a Rust wrapper around libgit.a | expand |
Hi Josh On 28/01/2025 22:01, Josh Steadmon wrote: Thanks for re-rolling, the range-diff looks good to me apart from > +void libgit_configset_free(struct libgit_config_set *cs) > +{ > -+ git_configset_clear((struct config_set *) cs); > -+ free((struct config_set *) cs); > ++ git_configset_clear(&cs->cs); > ++ free(&cs->cs); Which I think should be "free(cs)". In practice it does not matter because we pass the same value to free() but it seems a bit odd to pass the address of the first member of the struct rather than the address of the struct itself. I'm looking forward to seeing this merged soon Best Wishes Phillip
On 2025.01.29 15:24, Phillip Wood wrote: > Hi Josh > > On 28/01/2025 22:01, Josh Steadmon wrote: > > Thanks for re-rolling, the range-diff looks good to me apart from > > > +void libgit_configset_free(struct libgit_config_set *cs) > > +{ > > -+ git_configset_clear((struct config_set *) cs); > > -+ free((struct config_set *) cs); > > ++ git_configset_clear(&cs->cs); > > ++ free(&cs->cs); > > Which I think should be "free(cs)". In practice it does not matter because > we pass the same value to free() but it seems a bit odd to pass the address > of the first member of the struct rather than the address of the struct > itself. Yep sorry, got a bit careless with search-and-replace. Thanks for the catch! > I'm looking forward to seeing this merged soon > > Best Wishes > > Phillip >
This series provides two small Rust wrapper libraries around parts of Git: "libgit-sys", which exposes a few functions from libgit.a, and "libgit", which provides a more Rust-friendly interface to some of those functions. In addition to included unit tests, at $DAYJOB we have tested building JJ[1] with our library and used it to replace some of the libgit2-rs uses. [1] https://github.com/jj-vcs/jj There is known NEEDSWORK, but I feel that they can be addressed in follow-up changes, rather than in this series. If you feel otherwise, please let me know: * Investigate alternative methods of managing symbol visibility & renaming. * Figure out symbol versioning Changes in V8: * Define a private libgit_config_set struct to avoid excessive casting in public_symbol_export.c. * Style fixes: merge some Makefile rules, limit rule line length by defining intermediate variables, add initial empty comment line, add linebreaks in function definitions. Changes in V7: * Moved the ConfigSet implementation in libgit-rs to a `config` module. * Added doc comments for ConfigSet and its methods. * Fix meson builds by adding new object files to `libgit_sources` * Moved INCLUDE_LIBGIT_RS Makefile changes earlier in the series, so that we can make it optional to compile some of the libgitpub sources. Squashed V6 patch 5/5 into this series' patch 4/4. * Don't publicly export FFI types in libgit-rs. * Removed extraneous `-r` argument to $(RM) in the clean rules. * Added TODO reminder in Cargo.toml about removing Cargo.lock once we hit a certain minimum supported Rust version. * Style cleanup in public_symbol_export.c Changes in V6: * Rebased onto current master, since V5 was several months old. * Move libgit-sys out of libgit-rs; while this sort of nesting is common in Rust crates with standalone repositories, it doesn't make as much sense when they're contained in the larger Git project's repo. * Standardize the naming of some of the Makefile targets to always include a dash in the "-rs" or "-sys" suffixes. * Clean up READMEs and crate descriptions in preparation for uploading to crates.io. Changes in V5: * When building with INCLUDE_LIBGIT_RS defined, add "-fvisibility=hidden" to CFLAGS. This allows us to manage symbol visibility in libgitpub.a without causing `make all` to rebuild from scratch due to changing CFLAGS. * Avoid using c_int in the higher-level Rust API. * Remove libgitpub.a and intermediate files with `make clean`. Changes in V4: * Drop V3 patch #3, which added wrappers around repository initialization and config access. These are not well-libified, and they are not necessary for JJ's proof-of-concept use case, so let's avoid exporting them for now. * Set a minimum supported Rust version of 1.63. Autodetect whether our Rust version has c_int and c_char types; if not, define them ourselves. * When building libgitpub.a via build.rs, set DEVELOPER=1 to catch additional errors at build time. * In build.rs, use the make_cmd crate to portable select the correct invocation of GNU Make. * Follow naming standards for _alloc() and _free() functions. * Use String instead of CString in higher-level API. * Move libgit_configset_alloc() and libgit_configset_free() out of upstream Git, to the libgitpub shim library. * In libgitpub, initialize libgit_config_set structs in the _alloc() function rather than with a separate _init() function. * Remove unnecessary comments in libgit-sys showing where the wrapped functions were originally defined. * Fix clippy lint: don't reborrow configfile path references. * Various typo fixes and `cargo fmt` fixes. Changes in V3: * Renamed cgit-rs to libgit-rs and cgit-sys to libgit-sys * Makefile cleanup, particularly adding config.mak options that developers can set to run Rust builds and tests by default (Patch 6) * Provide testdata configs for unit tests * ConfigSet API now uses &Path instead of &str -- more ergonomic for Rust users to pass in and errors out if the path string isn't UTF-8 * Fixed unresolved dependency on libz in Cargo.toml Calvin Wan (1): libgit: add higher-level libgit crate Josh Steadmon (3): common-main: split init and exit code into new files libgit-sys: introduce Rust wrapper for libgit.a libgit-sys: also export some config_set functions .gitignore | 2 + Makefile | 49 ++++++++++ common-exit.c | 26 ++++++ common-init.c | 63 +++++++++++++ common-init.h | 6 ++ common-main.c | 83 +---------------- contrib/libgit-rs/Cargo.lock | 77 ++++++++++++++++ contrib/libgit-rs/Cargo.toml | 17 ++++ contrib/libgit-rs/README.md | 13 +++ contrib/libgit-rs/build.rs | 4 + contrib/libgit-rs/src/config.rs | 106 ++++++++++++++++++++++ contrib/libgit-rs/src/lib.rs | 1 + contrib/libgit-rs/testdata/config1 | 2 + contrib/libgit-rs/testdata/config2 | 2 + contrib/libgit-rs/testdata/config3 | 2 + contrib/libgit-sys/Cargo.lock | 69 ++++++++++++++ contrib/libgit-sys/Cargo.toml | 19 ++++ contrib/libgit-sys/README.md | 4 + contrib/libgit-sys/build.rs | 35 +++++++ contrib/libgit-sys/public_symbol_export.c | 59 ++++++++++++ contrib/libgit-sys/public_symbol_export.h | 18 ++++ contrib/libgit-sys/src/lib.rs | 79 ++++++++++++++++ meson.build | 2 + t/Makefile | 15 +++ 24 files changed, 672 insertions(+), 81 deletions(-) create mode 100644 common-exit.c create mode 100644 common-init.c create mode 100644 common-init.h create mode 100644 contrib/libgit-rs/Cargo.lock create mode 100644 contrib/libgit-rs/Cargo.toml create mode 100644 contrib/libgit-rs/README.md create mode 100644 contrib/libgit-rs/build.rs create mode 100644 contrib/libgit-rs/src/config.rs 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 create mode 100644 contrib/libgit-sys/Cargo.lock create mode 100644 contrib/libgit-sys/Cargo.toml create mode 100644 contrib/libgit-sys/README.md create mode 100644 contrib/libgit-sys/build.rs create mode 100644 contrib/libgit-sys/public_symbol_export.c create mode 100644 contrib/libgit-sys/public_symbol_export.h create mode 100644 contrib/libgit-sys/src/lib.rs Range-diff against v7: 1: cd0cb9aa04 = 1: cd0cb9aa04 common-main: split init and exit code into new files 2: f1502b8590 ! 2: 3588a3c3fc libgit-sys: introduce Rust wrapper for libgit.a @@ Makefile: $(CLAR_TEST_PROG): $(UNIT_TEST_DIR)/clar.suite $(CLAR_TEST_OBJS) $(GIT +all:: libgit-sys +endif + -+contrib/libgit-sys/partial_symbol_export.o: contrib/libgit-sys/public_symbol_export.o libgit.a reftable/libreftable.a xdiff/lib.a ++LIBGIT_PUB_OBJS = contrib/libgit-sys/public_symbol_export.o ++LIBGIT_PUB_OBJS += libgit.a ++LIBGIT_PUB_OBJS += reftable/libreftable.a ++LIBGIT_PUB_OBJS += xdiff/lib.a ++ ++LIBGIT_PARTIAL_EXPORT = contrib/libgit-sys/partial_symbol_export.o ++ ++LIBGIT_HIDDEN_EXPORT = contrib/libgit-sys/hidden_symbol_export.o ++ ++$(LIBGIT_PARTIAL_EXPORT): $(LIBGIT_PUB_OBJS) + $(LD) -r $^ -o $@ + -+contrib/libgit-sys/hidden_symbol_export.o: contrib/libgit-sys/partial_symbol_export.o ++$(LIBGIT_HIDDEN_EXPORT): $(LIBGIT_PARTIAL_EXPORT) + $(OBJCOPY) --localize-hidden $^ $@ + -+contrib/libgit-sys/libgitpub.a: contrib/libgit-sys/hidden_symbol_export.o ++contrib/libgit-sys/libgitpub.a: $(LIBGIT_HIDDEN_EXPORT) + $(AR) $(ARFLAGS) $@ $^ ## contrib/libgit-sys/Cargo.lock (new) ## @@ contrib/libgit-sys/build.rs (new) ## contrib/libgit-sys/public_symbol_export.c (new) ## @@ -+/* Shim to publicly export Git symbols. These must be renamed so that the ++/* ++ * Shim to publicly export Git symbols. These must be renamed so that the + * original symbols can be hidden. Renaming these with a "libgit_" prefix also + * avoids conflicts with other libraries such as libgit2. + */ 3: d67d3648d1 ! 3: f4452fffe6 libgit-sys: also export some config_set functions @@ contrib/libgit-sys/public_symbol_export.c #pragma GCC visibility push(default) ++struct libgit_config_set { ++ struct config_set cs; ++}; ++ +struct libgit_config_set *libgit_configset_alloc(void) +{ -+ struct config_set *cs = xmalloc(sizeof(struct config_set)); -+ git_configset_init(cs); -+ return (struct libgit_config_set *) cs; ++ struct libgit_config_set *cs = ++ xmalloc(sizeof(struct libgit_config_set)); ++ git_configset_init(&cs->cs); ++ return cs; +} + +void libgit_configset_free(struct libgit_config_set *cs) +{ -+ git_configset_clear((struct config_set *) cs); -+ free((struct config_set *) cs); ++ git_configset_clear(&cs->cs); ++ free(&cs->cs); +} + +int libgit_configset_add_file(struct libgit_config_set *cs, const char *filename) +{ -+ return git_configset_add_file((struct config_set *) cs, filename); ++ return git_configset_add_file(&cs->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 *cs, const char *key, ++ int *dest) +{ -+ return git_configset_get_int((struct config_set *) cs, key, dest); ++ return git_configset_get_int(&cs->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 *cs, const char *key, ++ char **dest) +{ -+ return git_configset_get_string((struct config_set *) cs, key, dest); ++ return git_configset_get_string(&cs->cs, key, dest); +} + const char *libgit_user_agent(void) 4: 88425bb0b1 ! 4: ada9fc0a13 libgit: add higher-level libgit crate @@ Makefile: build-unit-tests: $(UNIT_TEST_PROGS) $(CLAR_TEST_PROG) $(MAKE) -C t/ unit-tests -.PHONY: libgit-sys +-libgit-sys: +.PHONY: libgit-sys libgit-rs - libgit-sys: ++libgit-sys libgit-rs: $(QUIET)(\ - cd contrib/libgit-sys && \ +- cd contrib/libgit-sys && \ ++ cd contrib/$@ && \ cargo build \ ) -+libgit-rs: -+ $(QUIET)(\ -+ cd contrib/libgit-rs && \ -+ cargo build \ -+ ) ifdef INCLUDE_LIBGIT_RS -all:: libgit-sys +all:: libgit-sys libgit-rs endif - contrib/libgit-sys/partial_symbol_export.o: contrib/libgit-sys/public_symbol_export.o libgit.a reftable/libreftable.a xdiff/lib.a + LIBGIT_PUB_OBJS = contrib/libgit-sys/public_symbol_export.o ## contrib/libgit-rs/Cargo.lock (new) ## @@ base-commit: 757161efcca150a9a96b312d9e780a071e601a03