Re: [PATCH 1/7] xdiff: introduce rust

[Taylor Blau <me@ttaylorr.com>]

On Thu, Jul 17, 2025 at 08:32:18PM +0000, Ezekiel Newren via GitGitGadget wrote:
> From: Ezekiel Newren <ezekielnewren@gmail.com>
>
> 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 ;-).

> 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".

>  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.

> +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".

> 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