Re: [PATCH 1/7] xdiff: introduce rust
From: Taylor Blau <hidden>
Date: 2025-07-17 22:38:23
On Thu, Jul 17, 2025 at 08:32:18PM +0000, Ezekiel Newren via GitGitGadget wrote:
From: Ezekiel Newren <redacted> Upcoming patches will accelerate and simplify xdiff, while also porting parts of it to Rust. In preparation, add some stubs and setup the Rust build. For now, it is easier to let cargo build rust and have make or meson merely link against the static library that cargo builds. In line with ongoing libification efforts, use multiple crates to allow more modularity on the Rust side. xdiff is the crate that this series will focus on, but we also introduce the interop crate for future patch series. In order to facilitate interoperability between C and Rust, introduce C definitions for Rust primitive types in git-compat-util.h.
Exciting ;-).
quoted hunk ↗ jump to hunk
diff --git a/Makefile b/Makefile index 70d1543b6b86..db39e6e1c28e 100644 --- a/Makefile +++ b/Makefile@@ -919,6 +919,11 @@ TEST_SHELL_PATH = $(SHELL_PATH) LIB_FILE = libgit.a XDIFF_LIB = xdiff/lib.a +ifeq ($(DEBUG), 1) +RUST_LIB = rust/target/debug/libxdiff.a +else +RUST_LIB = rust/target/release/libxdiff.a +endif
We do have a DEBUG variable in our Makefile introduced via dce7d29551 (msvc: support building Git using MS Visual C++, 2019-06-25), but I don't think that it is very widely used. Perhaps that is because I don't build Git with MSVC, but I suspect that this is generally true. Much more common is the DEVELOPER=1 setting, which adds more compiler warnings and similar. I am not sure whether or not it would be appropriate to use DEVELOPER here to determine which libxdiff.a to use. In any event, our convention would be to treat the defined-ness of DEBUG the same way that this patch treats DEBUG=1, so I might suggest replacing your "ifeq" with "ifdef DEBUG".
quoted hunk ↗ jump to hunk
REFTABLE_LIB = reftable/libreftable.a GENERATED_H += command-list.h@@ -1392,6 +1397,8 @@ UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/lib-reftable.o GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(LIB_FILE) EXTLIBS = +GITLIBS += $(RUST_LIB) + GIT_USER_AGENT = git/$(GIT_VERSION) ifeq ($(wildcard sha1collisiondetection/lib/sha1.h),sha1collisiondetection/lib/sha1.h)@@ -2925,6 +2932,14 @@ $(LIB_FILE): $(LIB_OBJS) $(XDIFF_LIB): $(XDIFF_OBJS) $(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^ +.PHONY: $(RUST_LIB) +$(RUST_LIB): +ifeq ($(DEBUG), 1) + cd rust && RUSTFLAGS="-Aunused_imports -Adead_code" cargo build --verbose
A few thoughts here: - Does "cargo" support a flag similar to our -C? If so, I wonder if it might be worth writing "cargo -C rust build ..." instead of "cd rust && ...". - This conditional on DEBUG passes the "--verbose" option in both cases. Should we only pass the "--verbose" option when we have "V=1"? - Regardless of whether or not we condition passing "--release" (or not) on "DEBUG", this line should also be "ifdef DEBUG" similar to above.
quoted hunk ↗ jump to hunk
+else + cd rust && RUSTFLAGS="-Aunused_imports -Adead_code" cargo build --verbose --release +endif + $(REFTABLE_LIB): $(REFTABLE_OBJS) $(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^@@ -3756,7 +3771,10 @@ cocciclean: $(RM) -r .build/contrib/coccinelle $(RM) contrib/coccinelle/*.cocci.patch -clean: profile-clean coverage-clean cocciclean +rustclean:
I'm nitpicking, and we don't *really* have a convention here between separating the clean target from "clean", as we have both "profile-clean" and "cocciclean". I prefer the former, and think that it would be nice to use that convention, but this is pretty much textbook bike-shedding and not something that I really care about ;-).
+ cd rust && cargo clean
Same question here about whether or not this could be written as "cargo -C clean".
quoted hunk ↗ jump to hunk
diff --git a/git-compat-util.h b/git-compat-util.h index 4678e21c4cb8..82dc99764ac0 100644 --- a/git-compat-util.h +++ b/git-compat-util.h@@ -196,6 +196,23 @@ static inline int is_xplatform_dir_sep(int c) #include "compat/msvc.h" #endif +/* rust types */ +typedef uint8_t u8; +typedef uint16_t u16; +typedef uint32_t u32; +typedef uint64_t u64; + +typedef int8_t i8; +typedef int16_t i16; +typedef int32_t i32; +typedef int64_t i64; + +typedef float f32; +typedef double f64; + +typedef size_t usize; +typedef ptrdiff_t isize; +
Makes sense. Should we also have "bool" here (assuming that the series declaring the "use bool" experiment a success lands)? I guess maybe not either way, <stdbool.h> defines "bool" as the type name, identically to Rust. Thanks, Taylor