On Sat, Aug 23, 2025 at 12:05 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
> > To address these issue, introduce a new type, ivec -- short for
> > interoperable vector. (We refer to it as 'ivec' generally, though on
> > the Rust side the struct is called IVec to match Rust style.)
>
> I however was hoping by now Rust getting used more widely, somebody
> has already created a generic "this is how you make C-array and Rust
> vectors interoperate" wrapper that latecomer projects like us can
> use without inventing our own.
I've looked for code that will do what Git needs, but as far as I know
nothing can do everything that my ivec can.
> > +INTEROP_OBJS += interop/ivec.o
> > +.PHONY: interop-objs
> > +interop-objs: $(INTEROP_OBJS)
>
> What is this phony target used for? No other targets seem to depend
> on this one (I am wondering if we need the latter two lines).
You are correct. I will remove those lines.
> > diff --git a/interop/ivec.c b/interop/ivec.c
> > new file mode 100644
> > index 000000000000..9bc2258c04ad
> > --- /dev/null
> > +++ b/interop/ivec.c
>
> I am wondering if this needs a new hierarchy "interop"; shouldn't
> the existing "compat" be a good fit enough? I dunno.
I had considered compat/, but I thought it didn’t fit. I thought it
meant that the same API would exist everywhere, as opposed to “We
speak different languages, but we’ve agreed on a translator or common
protocol”. In particular, an example from ivec, a function on both
the C and Rust sides:
void ivec_extend_from_slice(void *_self, void const *ptr, usize size);
pub fn extend_from_slice(&mut self, slice: &[T]) where T: Clone,
The Rust side uses a slice or “fat” pointer, where the C side uses two
arguments (a pointer and a size) in its place. The API is different,
even if semantically they are the same and they are interoperable.
Was I reading too much into the meaning of compat/? Do folks object
to using interop/?
> Even though this is a shim to somebody else's code, it still is a
> part of our codebase, so our CodingGuidelines for C programs should
> apply.
Sorry I missed those. I will fix them up.
> > @@ -0,0 +1,151 @@
> > +#include "ivec.h"
> > +
> > +static void ivec_set_capacity(void* self, usize new_capacity) {
> > + struct rawivec *this = self;
>
> - Asterisk sticks to the variable, not type.
>
> - The opening and closing {braces} for the function body are
> written at the leftmost column on its own line.
>
> - There should be a blank line between the declarations and the
> first statement.
I will make those changes.
> > + if (new_capacity == 0)
> > + FREE_AND_NULL(this->ptr);
> > + else
> > + this->ptr = xrealloc(this->ptr, new_capacity * this->element_size);
> > + this->capacity = new_capacity;
> > +}
> > +
> > +void ivec_init(void* self, usize element_size) {
> > + struct rawivec *this = self;
> > + this->ptr = NULL;
> > + this->length = 0;
> > + this->capacity = 0;
> > + this->element_size = element_size;
> > +}
>
> I notice that this reintroduces a variable named "this", which was
> eradicated in 585c0e2e (diff: rename 'this' variables, 2018-02-14).
>
> I do not think those who want to use C++ compilers on our C code
> would not mind "self", so how about doing something like...
>
>
> void ivec_init(void *self_, usize element_size)
> {
> struct rawivec *self = self;
>
> self->ptr = NULL;
> self->len = 0;
> self->capacity = 0;
> self->element_size = element_size;
> }
>
> ... perhaps?
That sounds good. I will make those changes.
> > diff --git a/interop/ivec.h b/interop/ivec.h
> > new file mode 100644
> > index 000000000000..98be4bbeb54a
> > --- /dev/null
> > +++ b/interop/ivec.h
> > @@ -0,0 +1,52 @@
> > +#ifndef IVEC_H
> > +#define IVEC_H
> > +
> > +#include "../git-compat-util.h"
>
> As we use -I. on the command line, there is no need to add "../"
> here; just writing
>
> #include <git-compat-util.h>
>
> should be enough. Also, if this file does not depend on the
> services compat-util header provides (and I do not think it does
> from a brief look at its contents), it is better not to include it.
This file actually does depend on git-compat-util.h, particularly the
Rust primitive definitions (e.g. usize, u64, etc...).
I'll use the include style you mentioned.
> > diff --git a/rust/xdiff/src/lib.rs b/rust/xdiff/src/lib.rs
> > index e69de29bb2d1..8b137891791f 100644
> > --- a/rust/xdiff/src/lib.rs
> > +++ b/rust/xdiff/src/lib.rs
> > @@ -0,0 +1 @@
> > +
>
> If this empty line in an otherwise empty file is absolutely
> necessary to make Rust work, then please arrange .gitattributes to
> tell git that this file is excempt from the usual blank-at-eof
> whitespace rule we use. If not, remove that unnecessary empty line.
>
> Or perhaps remove the file altogether if nobody looks at it???
>
> In any case, given that our top-level .gitattributes file starts
> with
>
> * whitespace=!indent,trail,space
> *.[ch] whitespace=indent,trail,space diff=cpp
> *.sh whitespace=indent,trail,space text eol=lf
> ...
> *.bat text eol=crlf
> CODE_OF_CONDUCT.md -whitespace
> ...
>
> I think a new rule to cover "*.rs" and perhaps *.toml files right
> before rules for each specific file begin would be in order.
This is only a problem for empty files, and we only have empty .rs
files to setup the basic Rust build in this commit. It's not a problem
later in this series and shouldn't be a problem in the future. I will
remove those blank lines from those commits.
