"Ezekiel Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +	if (new_capacity == 0) {
> +		free(self->ptr);
> +		self->ptr = NULL;

	if (!new_capacity)
		FREE_AND_NULL(self->ptr);
	else
		...;

> +void ivec_free(void *self_)
> +{
> +	struct IVec_c_void *self = self_;
> +
> +	free(self->ptr);
> +	self->ptr = NULL;

Likewise.  Otherwise the code will fail coccicheck.

> +	self->length = 0;
> +	self->capacity = 0;
> +	// DO NOT MODIFY element_size!!!

	/* A single-liner comment in our codebase looks like this */

On Sat, Jan 3, 2026 at 10:32 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Ezekiel Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > +     if (new_capacity == 0) {
> > +             free(self->ptr);
> > +             self->ptr = NULL;
>
>         if (!new_capacity)
>                 FREE_AND_NULL(self->ptr);
>         else
>                 ...;
>
> > +void ivec_free(void *self_)
> > +{
> > +     struct IVec_c_void *self = self_;
> > +
> > +     free(self->ptr);
> > +     self->ptr = NULL;
>
> Likewise.  Otherwise the code will fail coccicheck.
>
> > +     self->length = 0;
> > +     self->capacity = 0;
> > +     // DO NOT MODIFY element_size!!!
>
>         /* A single-liner comment in our codebase looks like this */
>

I will make these changes.

Hi Ezekiel

On 02/01/2026 18:52, Ezekiel Newren via GitGitGadget wrote:
> From: Ezekiel Newren <ezekielnewren@gmail.com>
> > Trying to use Rust's Vec in C, or git's ALLOC_GROW() macros (via
> wrapper functions) in Rust is painful because:
> >    * C doesn't define its own vector type, and even though Rust does
>      have Vec its painful to use on the C side (more on that below).
>      However its still not viable to use Rust's Vec type because Git
>      needs to be able to compile without Rust. So ivec was created
>      expressley to be interoperable between C and Rust without needing
>      Rust.
>    * C doing vector things the Rust way would require wrapper functions,
>      and Rust doing vector things the C way would require wrapper
>      functions, so ivec was created to ensure a consistent contract
>      between the 2 languages for how to manipulate a vector.
>    * Currently, Rust defines its own 'Vec' type that is generic, but its
>      memory allocator and struct layout weren't designed for
>      interoperability with C (or any language for that matter), meaning
>      that the C side cannot push to or expand a 'Vec' without defining
>      wrapper functions in Rust that C can call. Without special care,
>      the two languages might use different allocators (malloc/free on
>      the C side, and possibly something else in Rust), which would make
>      it difficult for a function in one language to free elements
>      allocated by a call from a function in the other language.
>    * Similarly, git defines ALLOC_GROW() and related macros in
>      git-compat-util.h. While we could add functions allowing Rust to
>      invoke something similar to those macros, passing three variables
>      (pointer, length, allocated_size) instead of a single variable
>      (vector) across the language boundary requires more cognitive
>      overhead for readers to keep track of and makes it easier to make
>      mistakes. Further, for low-level components that we want to
>      eventually convert to pure Rust, such triplets would feel very out
>      of place.
> > 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.)  This new
> type is specifically designed for FFI purposes, so that both languages
> handle the vector in the same way, though it could be used on either
> side independently. This type is designed such that it can easily be
> replaced by a Rust 'Vec' once interoperability is no longer a concern.
> > One particular item to note is that Git's macros to handle vec
> operations infer the amount that a vec needs to grow from the size of
> a pointer, but that makes it somewhat specific to the macros used in C.
> To avoid defining every ivec function as a macro I opted to also
> include an element_size field that allows concrete functions like
> push() to know how much to grow the memory. This element_size also
> helps in verifying that the ivec is correct when passing from C to
> Rust.

I've left some comments below but I think this is a sensible direction.

> diff --git a/compat/ivec.c b/compat/ivec.c
> new file mode 100644
> index 0000000000..0a777e78dc
> --- /dev/null
> +++ b/compat/ivec.c
> @@ -0,0 +1,113 @@
> +#include "ivec.h"
> +
> +struct IVec_c_void {

We normally use all lower case names for structs but as this is shared with rust it maybe makes sense to use CamelCase so the names are the same in both languages.

> +	void *ptr;
> +	size_t length;
> +	size_t capacity;
> +	size_t element_size;
> +};
> +
> +static void _set_capacity(void *self_, size_t new_capacity)
> +{
> +	struct IVec_c_void *self = self_;

Passing any of the ivec variants defined below to this function invokes undefined behavior because we're not casting the pointer back to the orginal type. However I think on the platforms we care about sizeof(void*) == sizeof(T*) for all T so maybe we can look the other way.

> +
> +	if (new_capacity == self->capacity) {
> +		return;
> +	}
> +	if (new_capacity == 0) {
> +		free(self->ptr);
> +		self->ptr = NULL;
> +	} else {
> +		self->ptr = realloc(self->ptr, new_capacity * self->element_size);
> +	}
> +	self->capacity = new_capacity;

Not if realloc() returns NULL. We should check for that, probably by using xrealloc().

> +void ivec_zero(void *self_, size_t capacity)
> +{
> +	struct IVec_c_void *self = self_;
> +
> +	self->ptr = calloc(capacity, self->element_size);

We should be handling allocation failures here probably by using xcalloc().

> +void ivec_reserve(void *self_, size_t additional)
> +{
> +	struct IVec_c_void *self = self_;
> +
> +	size_t growby = 128;
> +	if (self->capacity > growby)
> +		growby = self->capacity;
> +	if (additional > growby)
> +		growby = additional;

This growth strategy differs from both ALLOC_GROW() and XDL_ALLOC_GROW(), if there isn't a good reason for that we should perhaps just use ALLOC_GROW() here.

> +void ivec_push(void *self_, const void *value)
> +{
> +	struct IVec_c_void *self = self_;
> +	void *dst = NULL;
> +
> +	if (self->length == self->capacity)
> +		ivec_reserve(self, 1);
> +
> +	dst = (uint8_t*)self->ptr + self->length * self->element_size;
> +	memcpy(dst, value, self->element_size);

If self->element_size was a compile time constant the compiler could easily optimize this call away. I'm not sure that is easy to achieve though.

> +	self->length++;
> +}
> +
> +void ivec_free(void *self_)

Normally we'd call a like this that free the allocations and re-initializes the members ivec_clear()

> +{
> +	struct IVec_c_void *self = self_;
> +
> +	free(self->ptr);
> +	self->ptr = NULL;
> +	self->length = 0;
> +	self->capacity = 0;
> +	// DO NOT MODIFY element_size!!!
> +}
> +
> +void ivec_move(void *src_, void *dst_)
> +{
> +	struct IVec_c_void *src = src_;
> +	struct IVec_c_void *dst = dst_;

Maybe we should add

	if (src->element_size != dst->element_size)
		BUG("moving incompatible arrays");
> +
> +	ivec_free(dst);
> +	dst->ptr = src->ptr;
> +	dst->length = src->length;
> +	dst->capacity = src->capacity;
> +	// DO NOT MODIFY element_size!!!

As the element sizes must match maybe *dst = *src would be clearer?

> +
> +	src->ptr = NULL;
> +	src->length = 0;
> +	src->capacity = 0;
> +	// DO NOT MODIFY element_size!!!
> +}
> diff --git a/compat/ivec.h b/compat/ivec.h
> new file mode 100644
> index 0000000000..654a05c506
> --- /dev/null
> +++ b/compat/ivec.h
> @@ -0,0 +1,52 @@
> +#ifndef IVEC_H
> +#define IVEC_H
> +
> +#include <git-compat-util.h>

It would be nice to have some documentation in this header, see the examples in strvec.h and hashmap.h

> +#define IVEC_INIT(variable) ivec_init(&(variable), sizeof(*(variable).ptr))

This is a bit cumbersome to use compared to our usual *_INIT macros. I'm struggling to see how we can make it nicer though as DEFINE_IVEC_TYPE cannot define a per-type initializer macro and I we cannot initialize the element size without knowing the type.

> +
> +#ifndef CBINDGEN
> +#define DEFINE_IVEC_TYPE(type, suffix) \
> +struct IVec_##suffix { \
> +	type* ptr; \
> +	size_t length; \
> +	size_t capacity; \
> +	size_t element_size; \
> +}

I wonder if we want to define type safe inline safe wrappers for the ivec_* functions here. I think the only functions where the element type matters are ivec_move() and ivec_push(), for the others like ivec_zero(), ivec_reserve() and ivec_free() the element type does not matter. ivec_push() would certainly be easier to use with a wrapper as means we can avoid forcing the caller to take the address of the value.

static inline ivec_##suffix##_push(struct IVec_##suffix *self, type value) { \
	const void *ptr = &value; \
	ivec_push(self, ptr); \
}

I'll try and take a look at the rest of this series next week

Thanks

Phillip

> +
> +DEFINE_IVEC_TYPE(bool, bool);
> +
> +DEFINE_IVEC_TYPE(uint8_t, u8);
> +DEFINE_IVEC_TYPE(uint16_t, u16);
> +DEFINE_IVEC_TYPE(uint32_t, u32);
> +DEFINE_IVEC_TYPE(uint64_t, u64);
> +
> +DEFINE_IVEC_TYPE(int8_t, i8);
> +DEFINE_IVEC_TYPE(int16_t, i16);
> +DEFINE_IVEC_TYPE(int32_t, i32);
> +DEFINE_IVEC_TYPE(int64_t, i64);
> +
> +DEFINE_IVEC_TYPE(float, f32);
> +DEFINE_IVEC_TYPE(double, f64);
> +
> +DEFINE_IVEC_TYPE(size_t, usize);
> +DEFINE_IVEC_TYPE(ssize_t, isize);
> +#endif
> +
> +void ivec_init(void *self_, size_t element_size);
> +
> +void ivec_zero(void *self_, size_t capacity);
> +
> +void ivec_reserve_exact(void *self_, size_t additional);
> +
> +void ivec_reserve(void *self_, size_t additional);
> +
> +void ivec_shrink_to_fit(void *self_);
> +
> +void ivec_push(void *self_, const void *value);
> +
> +void ivec_free(void *self_);
> +
> +void ivec_move(void *src, void *dst);
> +
> +#endif /* IVEC_H */
> diff --git a/meson.build b/meson.build
> index dd52efd1c8..42ac0c8c42 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -302,6 +302,7 @@ libgit_sources = [
>     'commit.c',
>     'common-exit.c',
>     'common-init.c',
> +  'compat/ivec.c',
>     'compat/nonblock.c',
>     'compat/obstack.c',
>     'compat/open.c',

On Thu, Jan 8, 2026 at 7:34 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> > diff --git a/compat/ivec.c b/compat/ivec.c
> > new file mode 100644
> > index 0000000000..0a777e78dc
> > --- /dev/null
> > +++ b/compat/ivec.c
> > @@ -0,0 +1,113 @@
> > +#include "ivec.h"
> > +
> > +struct IVec_c_void {
>
> We normally use all lower case names for structs but as this is shared
> with rust it maybe makes sense to use CamelCase so the names are the
> same in both languages.

My preference would be all lowercase, but cbindgen insists on using
the same casing as was used in Rust. I don't think there's a way to
make cbindgen use all lowercase for structs.

> > +     void *ptr;
> > +     size_t length;
> > +     size_t capacity;
> > +     size_t element_size;
> > +};
> > +
> > +static void _set_capacity(void *self_, size_t new_capacity)
> > +{
> > +     struct IVec_c_void *self = self_;
>
> Passing any of the ivec variants defined below to this function invokes
> undefined behavior because we're not casting the pointer back to the
> orginal type. However I think on the platforms we care about
> sizeof(void*) == sizeof(T*) for all T so maybe we can look the other way.

If someone finds that this code does not work because of this
assumption I'd like to know. But I can't fathom a case where it
wouldn't work.

> > +
> > +     if (new_capacity == self->capacity) {
> > +             return;
> > +     }
> > +     if (new_capacity == 0) {
> > +             free(self->ptr);
> > +             self->ptr = NULL;
> > +     } else {
> > +             self->ptr = realloc(self->ptr, new_capacity * self->element_size);
> > +     }
> > +     self->capacity = new_capacity;
>
> Not if realloc() returns NULL. We should check for that, probably by
> using xrealloc().
>
> > +void ivec_zero(void *self_, size_t capacity)
> > +{
> > +     struct IVec_c_void *self = self_;
> > +
> > +     self->ptr = calloc(capacity, self->element_size);
>
> We should be handling allocation failures here probably by using xcalloc().

I've changed it to xrealloc() similar for the calloc() call.


> > +void ivec_reserve(void *self_, size_t additional)
> > +{
> > +     struct IVec_c_void *self = self_;
> > +
> > +     size_t growby = 128;
> > +     if (self->capacity > growby)
> > +             growby = self->capacity;
> > +     if (additional > growby)
> > +             growby = additional;
>
> This growth strategy differs from both ALLOC_GROW() and
> XDL_ALLOC_GROW(), if there isn't a good reason for that we should
> perhaps just use ALLOC_GROW() here.

XDL_ALLOW_GROW() can't be used because the pointer is always a void*
in this function.

> > +void ivec_push(void *self_, const void *value)
> > +{
> > +     struct IVec_c_void *self = self_;
> > +     void *dst = NULL;
> > +
> > +     if (self->length == self->capacity)
> > +             ivec_reserve(self, 1);
> > +
> > +     dst = (uint8_t*)self->ptr + self->length * self->element_size;
> > +     memcpy(dst, value, self->element_size);
>
> If self->element_size was a compile time constant the compiler could
> easily optimize this call away. I'm not sure that is easy to achieve though.

The problem is that I didn't want all of ivec to be macros that looked
like function calls. I wanted to minimize use of macros so that it was
easier to port and verify that the Rust implementation matches the
behavior of the C implementation.

> > +void ivec_free(void *self_)
>
> Normally we'd call a like this that free the allocations and
> re-initializes the members ivec_clear()

In Rust Vec.clear() means to set length to zero, but leaves the
allocation alone. The reason why I'm zeroing the struct is to help
avoid FFI issues. If not zero then what should the members be set to,
to indicate that using the struct is not valid anymore? In Rust an
object is freed when it goes out of scope and _cannot_ be accessed
afterward.

> > +{
> > +     struct IVec_c_void *self = self_;
> > +
> > +     free(self->ptr);
> > +     self->ptr = NULL;
> > +     self->length = 0;
> > +     self->capacity = 0;
> > +     // DO NOT MODIFY element_size!!!
> > +}
> > +
> > +void ivec_move(void *src_, void *dst_)
> > +{
> > +     struct IVec_c_void *src = src_;
> > +     struct IVec_c_void *dst = dst_;
>
> Maybe we should add
>
>         if (src->element_size != dst->element_size)
>                 BUG("moving incompatible arrays");

I'll do that.

> > +
> > +     ivec_free(dst);
> > +     dst->ptr = src->ptr;
> > +     dst->length = src->length;
> > +     dst->capacity = src->capacity;
> > +     // DO NOT MODIFY element_size!!!
>
> As the element sizes must match maybe *dst = *src would be clearer?

That seems fine.

> > +
> > +     src->ptr = NULL;
> > +     src->length = 0;
> > +     src->capacity = 0;
> > +     // DO NOT MODIFY element_size!!!
> > +}
> > diff --git a/compat/ivec.h b/compat/ivec.h
> > new file mode 100644
> > index 0000000000..654a05c506
> > --- /dev/null
> > +++ b/compat/ivec.h
> > @@ -0,0 +1,52 @@
> > +#ifndef IVEC_H
> > +#define IVEC_H
> > +
> > +#include <git-compat-util.h>
>
> It would be nice to have some documentation in this header, see the
> examples in strvec.h and hashmap.h
>
> > +#define IVEC_INIT(variable) ivec_init(&(variable), sizeof(*(variable).ptr))
>
> This is a bit cumbersome to use compared to our usual *_INIT macros. I'm
> struggling to see how we can make it nicer though as DEFINE_IVEC_TYPE
> cannot define a per-type initializer macro and I we cannot initialize
> the element size without knowing the type.

I don't see what's cumbersome about it. Maybe an example use case
would clarify things.

```
DEFINE_IVEC_TYPE(xrecord_t, xrecord);

void some_function() {
    struct IVec_xrecord rec;
    IVEC_INIT(rec);  // i.e. ivec_init(&rec, sizeof(*rec.ptr);

    // use concrete functions to manipulate vector or access the array
directly via ptr
}
```

IVEC_INIT() should be used on the concrete type.

> > +
> > +#ifndef CBINDGEN
> > +#define DEFINE_IVEC_TYPE(type, suffix) \
> > +struct IVec_##suffix { \
> > +     type* ptr; \
> > +     size_t length; \
> > +     size_t capacity; \
> > +     size_t element_size; \
> > +}
>
> I wonder if we want to define type safe inline safe wrappers for the
> ivec_* functions here. I think the only functions where the element type
> matters are ivec_move() and ivec_push(), for the others like
> ivec_zero(), ivec_reserve() and ivec_free() the element type does not
> matter. ivec_push() would certainly be easier to use with a wrapper as
> means we can avoid forcing the caller to take the address of the value.
>
> static inline ivec_##suffix##_push(struct IVec_##suffix *self, type
> value) { \
>         const void *ptr = &value; \
>         ivec_push(self, ptr); \
> }

I turned ivec_push() into a macro, but the rest will remain as
concrete functions.

I've Cc'd Peff and René for a second opinion if you have time please.

On 15/01/2026 15:55, Ezekiel Newren wrote:
> On Thu, Jan 8, 2026 at 7:34 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
>>> +static void _set_capacity(void *self_, size_t new_capacity)
>>> +{
>>> +     struct IVec_c_void *self = self_;
>>
>> Passing any of the ivec variants defined below to this function invokes
>> undefined behavior because we're not casting the pointer back to the
>> orginal type. However I think on the platforms we care about
>> sizeof(void*) == sizeof(T*) for all T so maybe we can look the other way.
> > If someone finds that this code does not work because of this
> assumption I'd like to know. But I can't fathom a case where it
> wouldn't work.

So we have two different structs

struct IVec_c_void {
	void *ptr;
	size_t length;
	size_t capacity;
	size_t element_size;
}

and

struct Ivec_u8 {
	uint8_t *ptr;
	size_t length;
	size_t capacity;
	size_t element_size;
}

One the platforms we care about they will have the same memory layout as all pointers have the same representation. However I don't think they are "compatible types" in the language of the C standard because the type of the "ptr" member differs. That means casting IVec_u8* to IVec_c_void* either directly or via void* is undefined and so

	struct IVec_u8 vec;
	ivec_init(&vec, sizeof(*vec.ptr));

is undefined. For the compiler to see the undefined cast it needs to look across translation units because the implementation of ivec_init() will be in a separate file to where it is called. Maybe that and the fact they have the same memory layout saves us from having to worry too much though I'm always nervous of undefined behavior.

An alternative would be to pass the individual struct members as function parameters

	void ivec_init(void **vec, size_t &length, size_t &capacity,
		       size_t &element_size_, size_t element_size)
	{
		*vec = NULL;
		*length = 0;
		*capacity = 0;
		*element_size_ = element_size;
	}

and have DEFINE_IVEC_TYPE create typesafe wrappers

	static inline void ivec_u8_init(struct IVec_u8 *vec)
	{
		void *ptr = vec->ptr;
		ivec_init(&ptr, &v->length, &v->capacity,
			  &v->element_size, sizeof(*(v->ptr));
		vec->ptr = ptr;
	}

That's safe because we cast the "ptr" member to "void*" and then back to the original type. On the rust side the implementation of IVec<T> would also need to split out the individual struct members when it calls ivec_init() etc. It's all a bit more effort but the benefit is that we don't have any undefined behavior and we have a nice typesafe C interface to 'struct IVec_*'.

Thanks

Phillip

On 1/16/26 11:39 AM, Phillip Wood wrote:
> I've Cc'd Peff and René for a second opinion if you have time please.
> 
> On 15/01/2026 15:55, Ezekiel Newren wrote:
>> On Thu, Jan 8, 2026 at 7:34 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>>>> +static void _set_capacity(void *self_, size_t new_capacity)
>>>> +{
>>>> +     struct IVec_c_void *self = self_;
>>>
>>> Passing any of the ivec variants defined below to this function invokes
>>> undefined behavior because we're not casting the pointer back to the
>>> orginal type. However I think on the platforms we care about
>>> sizeof(void*) == sizeof(T*) for all T so maybe we can look the other way.
>>
>> If someone finds that this code does not work because of this
>> assumption I'd like to know. But I can't fathom a case where it
>> wouldn't work.
> 
> So we have two different structs
> 
> struct IVec_c_void {
>     void *ptr;
>     size_t length;
>     size_t capacity;
>     size_t element_size;
> }
> 
> and
> 
> struct Ivec_u8 {
>     uint8_t *ptr;
>     size_t length;
>     size_t capacity;
>     size_t element_size;
> }
> 
> One the platforms we care about they will have the same memory
> layout as all pointers have the same representation. However I don't
> think they are "compatible types" in the language of the C standard
> because the type of the "ptr" member differs. That means casting
> IVec_u8* to IVec_c_void* either directly or via void* is undefined
> and so
> 
>     struct IVec_u8 vec;
>     ivec_init(&vec, sizeof(*vec.ptr));
> 
> is undefined. For the compiler to see the undefined cast it needs to
> look across translation units because the implementation of
> ivec_init() will be in a separate file to where it is called. Maybe
> that and the fact they have the same memory layout saves us from
> having to worry too much though I'm always nervous of undefined
> behavior.

True.  The GCC docs give a fun example of what a compiler might do
when using different struct types to access the same memory:

https://www.gnu.org/software/c-intro-and-ref/manual/html_node/Aliasing-Type-Rules.html

Not sure it applies to this case, but the point is that compilers
can and will do terrifying things when they smell UB, with little
concern for safety or original intent.

> An alternative would be to pass the individual struct members as function parameters
> 
>     void ivec_init(void **vec, size_t &length, size_t &capacity,
>                size_t &element_size_, size_t element_size)
>     {
>         *vec = NULL;
>         *length = 0;
>         *capacity = 0;
>         *element_size_ = element_size;
>     }

The ampersands (&) should be asterisks (*), right?

> and have DEFINE_IVEC_TYPE create typesafe wrappers
> 
>     static inline void ivec_u8_init(struct IVec_u8 *vec)
>     {
>         void *ptr = vec->ptr;
>         ivec_init(&ptr, &v->length, &v->capacity,
>               &v->element_size, sizeof(*(v->ptr));
>         vec->ptr = ptr;
>     }

Mixes "v" and "vec", misses a closing parenthesis.  Looks viable,
though, and this method should be applicable to the rest of the
functions as well (on the C side).

I guess this doesn't require an element_size member anymore as
each wrapper can pass in the sizeof value.

> That's safe because we cast the "ptr" member to "void*" and then
> back to the original type. On the rust side the implementation of
> IVec<T> would also need to split out the individual struct members
> when it calls ivec_init() etc. It's all a bit more effort but the
> benefit is that we don't have any undefined behavior and we have a
> nice typesafe C interface to 'struct IVec_*'.
Right.  No idea how ugly this would be on the Rust side, though.

René

On 16/01/2026 20:19, René Scharfe wrote:
> On 1/16/26 11:39 AM, Phillip Wood wrote:
>> I've Cc'd Peff and René for a second opinion if you have time please.
>>
>> On 15/01/2026 15:55, Ezekiel Newren wrote:
>>> On Thu, Jan 8, 2026 at 7:34 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>>
>>>>> +static void _set_capacity(void *self_, size_t new_capacity)
>>>>> +{
>>>>> +     struct IVec_c_void *self = self_;
>>>>
>>>> Passing any of the ivec variants defined below to this function invokes
>>>> undefined behavior because we're not casting the pointer back to the
>>>> orginal type. However I think on the platforms we care about
>>>> sizeof(void*) == sizeof(T*) for all T so maybe we can look the other way.
>>>
>>> If someone finds that this code does not work because of this
>>> assumption I'd like to know. But I can't fathom a case where it
>>> wouldn't work.
>>
>> So we have two different structs
>>
>> struct IVec_c_void {
>>      void *ptr;
>>      size_t length;
>>      size_t capacity;
>>      size_t element_size;
>> }
>>
>> and
>>
>> struct Ivec_u8 {
>>      uint8_t *ptr;
>>      size_t length;
>>      size_t capacity;
>>      size_t element_size;
>> }
>>
>> One the platforms we care about they will have the same memory
>> layout as all pointers have the same representation. However I don't
>> think they are "compatible types" in the language of the C standard
>> because the type of the "ptr" member differs. That means casting
>> IVec_u8* to IVec_c_void* either directly or via void* is undefined
>> and so
>>
>>      struct IVec_u8 vec;
>>      ivec_init(&vec, sizeof(*vec.ptr));
>>
>> is undefined. For the compiler to see the undefined cast it needs to
>> look across translation units because the implementation of
>> ivec_init() will be in a separate file to where it is called. Maybe
>> that and the fact they have the same memory layout saves us from
>> having to worry too much though I'm always nervous of undefined
>> behavior.
> > True.  The GCC docs give a fun example of what a compiler might do
> when using different struct types to access the same memory:
> > https://www.gnu.org/software/c-intro-and-ref/manual/html_node/Aliasing-Type-Rules.html

Thanks for the link

> Not sure it applies to this case, but the point is that compilers
> can and will do terrifying things when they smell UB, with little
> concern for safety or original intent.
> >> An alternative would be to pass the individual struct members as function parameters
>>
>>      void ivec_init(void **vec, size_t &length, size_t &capacity,
>>                 size_t &element_size_, size_t element_size)
>>      {
>>          *vec = NULL;
>>          *length = 0;
>>          *capacity = 0;
>>          *element_size_ = element_size;
>>      }
> > The ampersands (&) should be asterisks (*), right?

Indeed, that's embarrassing - I must have been thinking of the caller.

>> and have DEFINE_IVEC_TYPE create typesafe wrappers
>>
>>      static inline void ivec_u8_init(struct IVec_u8 *vec)
>>      {
>>          void *ptr = vec->ptr;
>>          ivec_init(&ptr, &v->length, &v->capacity,
>>                &v->element_size, sizeof(*(v->ptr));
>>          vec->ptr = ptr;
>>      }
> > Mixes "v" and "vec", misses a closing parenthesis.  Looks viable,
> though, and this method should be applicable to the rest of the
> functions as well (on the C side).
> > I guess this doesn't require an element_size member anymore as
> each wrapper can pass in the sizeof value.

Good point

>> That's safe because we cast the "ptr" member to "void*" and then
>> back to the original type. On the rust side the implementation of
>> IVec<T> would also need to split out the individual struct members
>> when it calls ivec_init() etc. It's all a bit more effort but the
>> benefit is that we don't have any undefined behavior and we have a
>> nice typesafe C interface to 'struct IVec_*'.
> Right.  No idea how ugly this would be on the Rust side, though.

I'm hoping it's not too bad and `impl IVec<T>` just contains the equivalent of the wrappers generated by DEFINE_IVEC_TYPE()

Thanks

Phillip
> > René
> 

On Wed, Jan 21, 2026 at 02:00:15PM -0700, Ezekiel Newren wrote:

> What about adding clar unit tests to make sure that different ivec
> types have the same size and layout? e.g. sizeof(IVec_c_void) ==
> sizeof(IVec_u8);
> sizeof(IVec_c_void) == sizeof(IVec_u16);
> sizeof(IVec_c_void) == sizeof(IVec_u32);
> sizeof(IVec_c_void) == sizeof(IVec_u64);
> ...
> 
> As well as other tests for ivec.

I'm a little hesitant in general to have run-time tests for properties
around undefined behavior, just because the compiler is allowed to do a
lot of tricky things when we get into that territory. Plus it is not
really _solving_ the problem, but perhaps just alerting us slightly
sooner than the production code itself crashing and burning.

You'd also need to check the pointer field sizes directly due to
padding. I don't think it's sufficient, due to padding. If one pointer
is 4 bytes and another is 8 (for example), but the element afterwards
requires 8-byte alignment, then the compiler will have to insert 4 bytes
of padding. And the resulting struct size will be the same. You'd have
to more directly check that sizeof(uint_t*) == sizeof(void *), I think.

So I dunno. I am not a compiler expert, nor a rust expert, nor really
know anything about rust/C ABI boundaries. There might be no problem at
all here, and I'm only commenting on what I know is possible (albeit
unlikely) from the C side.

-Peff

Jeff King <peff@peff.net> writes:

> On Wed, Jan 21, 2026 at 02:00:15PM -0700, Ezekiel Newren wrote:
>
>> What about adding clar unit tests to make sure that different ivec
>> types have the same size and layout? e.g. sizeof(IVec_c_void) ==
>> sizeof(IVec_u8);
>> sizeof(IVec_c_void) == sizeof(IVec_u16);
>> sizeof(IVec_c_void) == sizeof(IVec_u32);
>> sizeof(IVec_c_void) == sizeof(IVec_u64);
>> ...
>> 
>> As well as other tests for ivec.
>
> I'm a little hesitant in general to have run-time tests for properties
> around undefined behavior, just because the compiler is allowed to do a
> lot of tricky things when we get into that territory. Plus it is not
> really _solving_ the problem, but perhaps just alerting us slightly
> sooner than the production code itself crashing and burning.

Yup, by definition, testing undefined behaviour with code is more or
less pointless.  Implementation defined behaviour, maybe, but not
undefined ones, please.

I thought you already gave them that having different possibilities
in a union would work correctly, but perhaps I was reading a
different thread?  I dunno...

On Tue, Jan 20, 2026 at 7:06 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Ezekiel
>
> On 15/01/2026 15:55, Ezekiel Newren wrote:
> > On Thu, Jan 8, 2026 at 7:34 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> >>> +void ivec_reserve(void *self_, size_t additional)
> >>> +{
> >>> +     struct IVec_c_void *self = self_;
> >>> +
> >>> +     size_t growby = 128;
> >>> +     if (self->capacity > growby)
> >>> +             growby = self->capacity;
> >>> +     if (additional > growby)
> >>> +             growby = additional;
> >>
> >> This growth strategy differs from both ALLOC_GROW() and
> >> XDL_ALLOC_GROW(), if there isn't a good reason for that we should
> >> perhaps just use ALLOC_GROW() here.
> >
> > XDL_ALLOW_GROW() can't be used because the pointer is always a void*
> > in this function.
>
> Oh right. I'm not sure that's not a reason to use a different growth
> strategy though. The minimum size of 128 elements is probably good for
> the xdiff code that creates arrays with one element per line but if this
> is supposed to be for general use it is going to waste space when we're
> allocating a lot of small arrays. ALLOC_GROW() uses alloc_nr() to
> calculate the new side so perhaps we could use that here?

If ivec_reserve() isn't suitable then ivec_reserve_exact() should be
used instead.

> >>> +void ivec_push(void *self_, const void *value)
> >>> +{
> >>> +     struct IVec_c_void *self = self_;
> >>> +     void *dst = NULL;
> >>> +
> >>> +     if (self->length == self->capacity)
> >>> +             ivec_reserve(self, 1);
> >>> +
> >>> +     dst = (uint8_t*)self->ptr + self->length * self->element_size;
> >>> +     memcpy(dst, value, self->element_size);
> >>
> >> If self->element_size was a compile time constant the compiler could
> >> easily optimize this call away. I'm not sure that is easy to achieve though.
> >
> > The problem is that I didn't want all of ivec to be macros that looked
> > like function calls. I wanted to minimize use of macros so that it was
> > easier to port and verify that the Rust implementation matches the
> > behavior of the C implementation.
>
> I think that's a reasonable concern. So is the plan to have a parallel
> rust implementation of these functions rather than call the C
> implementation from rust?

Yes, the Rust implementation will be independent of the C
implementation, but will behave the same way. That's why I'm calling
it an interoperable vec as opposed to a compatible vec. Rust can't
call the C ivec functions and C can't call the Rust ivec functions,
but they'll behave the same way.

> >>> +void ivec_free(void *self_)
> >>
> >> Normally we'd call a like this that free the allocations and
> >> re-initializes the members ivec_clear()
> >
> > In Rust Vec.clear() means to set length to zero, but leaves the
> > allocation alone. The reason why I'm zeroing the struct is to help
> > avoid FFI issues. If not zero then what should the members be set to,
> > to indicate that using the struct is not valid anymore? In Rust an
> > object is freed when it goes out of scope and _cannot_ be accessed
> > afterward.

Maybe I should call this ivec_drop(). Though the notion of explicitly
freeing an object in Rust is _almost_ nonsense. The way you free
something in Rust is to let it go out of scope.

> I'm aware that Vec::clear() has different semantics (it does what
> strbuf_reset() does). That's unfortunate but this function has different
> semantics to all the other *_free() functions in git. Our coding
> guidelines say
>
>   - There are several common idiomatic names for functions performing
>     specific tasks on a structure `S`:
>
>      - `S_init()` initializes a structure without allocating the
>        structure itself.
>
>      - `S_release()` releases a structure's contents without freeing the
>        structure.
>
>      - `S_clear()` is equivalent to `S_release()` followed by `S_init()`
>        such that the structure is directly usable after clearing it. When
>        `S_clear()` is provided, `S_init()` shall not allocate resources
>        that need to be released again.
>
>      - `S_free()` releases a structure's contents and frees the
>        structure.
>
> As we write more rust code and so wrap more of our existing structs
> we're going to be wrapping C code that uses the definitions above so I
> think we should do the same with struct IVec_*.

I disagree. IVec isn't a wrapper around an existing struct. ivec is
meant to very closely mimic Rust's Vec while guaranteeing
interoperability. For things like strbuf I haven't conceived of a
solution for that yet. Making ivec diverge from Rust's Vec will result
in POLA violations due to different behavior when refactoring an
IVec<your_type_here> to Vec<your_type_here>.

> >>> diff --git a/compat/ivec.h b/compat/ivec.h
> >>> new file mode 100644
> >>> index 0000000000..654a05c506
> >>> --- /dev/null
> >>> +++ b/compat/ivec.h
> >>> @@ -0,0 +1,52 @@
> >>> +#ifndef IVEC_H
> >>> +#define IVEC_H
> >>> +
> >>> +#include <git-compat-util.h>
> >>
> >> It would be nice to have some documentation in this header, see the
> >> examples in strvec.h and hashmap.h
> >>
> >>> +#define IVEC_INIT(variable) ivec_init(&(variable), sizeof(*(variable).ptr))
> >>
> >> This is a bit cumbersome to use compared to our usual *_INIT macros. I'm
> >> struggling to see how we can make it nicer though as DEFINE_IVEC_TYPE
> >> cannot define a per-type initializer macro and I we cannot initialize
> >> the element size without knowing the type.
> >
> > I don't see what's cumbersome about it. Maybe an example use case
> > would clarify things.
>
> It is cumbersome because it separates the initialization from the
> declaration. Normally our *_INIT macros are initializer lists so we can
> write
>
>         struct strbuf = STRBUF_INIT;
>
> which keeps the declaration and initialization together. Although
> they're on adjacent lines in your example in real code the
> initialization likely to be separated from the declaration by other
> variable declarations.

Ah I see what you mean now. I'll experiment with making IVEC_INIT()
work like that. One wrinkle is that STRBUF_INIT is a single concrete
type whereas IVEC_INIT() is meant for generic types.

On Wed, Jan 21, 2026 at 2:31 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Jeff King <peff@peff.net> writes:
>
> > On Wed, Jan 21, 2026 at 02:00:15PM -0700, Ezekiel Newren wrote:
> >
> >> What about adding clar unit tests to make sure that different ivec
> >> types have the same size and layout? e.g. sizeof(IVec_c_void) ==
> >> sizeof(IVec_u8);
> >> sizeof(IVec_c_void) == sizeof(IVec_u16);
> >> sizeof(IVec_c_void) == sizeof(IVec_u32);
> >> sizeof(IVec_c_void) == sizeof(IVec_u64);
> >> ...
> >>
> >> As well as other tests for ivec.
> >
> > I'm a little hesitant in general to have run-time tests for properties
> > around undefined behavior, just because the compiler is allowed to do a
> > lot of tricky things when we get into that territory. Plus it is not
> > really _solving_ the problem, but perhaps just alerting us slightly
> > sooner than the production code itself crashing and burning.
>
> Yup, by definition, testing undefined behaviour with code is more or
> less pointless.  Implementation defined behaviour, maybe, but not
> undefined ones, please.
>
> I thought you already gave them that having different possibilities
> in a union would work correctly, but perhaps I was reading a
> different thread?  I dunno...
>

In my opinion the proper solution to this is to document that any
platform with different size pointers for different types is not
supported by Git. Which would make using Git on those platforms "use
at your own risk".

On 21/01/2026 21:39, Ezekiel Newren wrote:
> On Tue, Jan 20, 2026 at 7:06 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> Hi Ezekiel
>>
>> On 15/01/2026 15:55, Ezekiel Newren wrote:
>>> On Thu, Jan 8, 2026 at 7:34 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>>>> +void ivec_reserve(void *self_, size_t additional)
>>>>> +{
>>>>> +     struct IVec_c_void *self = self_;
>>>>> +
>>>>> +     size_t growby = 128;
>>>>> +     if (self->capacity > growby)
>>>>> +             growby = self->capacity;
>>>>> +     if (additional > growby)
>>>>> +             growby = additional;
>>>>
>>>> This growth strategy differs from both ALLOC_GROW() and
>>>> XDL_ALLOC_GROW(), if there isn't a good reason for that we should
>>>> perhaps just use ALLOC_GROW() here.
>>>
>>> XDL_ALLOW_GROW() can't be used because the pointer is always a void*
>>> in this function.
>>
>> Oh right. I'm not sure that's not a reason to use a different growth
>> strategy though. The minimum size of 128 elements is probably good for
>> the xdiff code that creates arrays with one element per line but if this
>> is supposed to be for general use it is going to waste space when we're
>> allocating a lot of small arrays. ALLOC_GROW() uses alloc_nr() to
>> calculate the new side so perhaps we could use that here?
> > If ivec_reserve() isn't suitable then ivec_reserve_exact() should be
> used instead.

If some C code that pushes one element at a time to an array using ALLOC_GROW() is converted to use an ivec then we don't want to change the code behaves - that means it should grow the array in the same way. I don't see how the suggestion to use ivec_reserve_exact() helps in that situation. What is the advantage in having a different growth characteristic?

>>>>> +void ivec_push(void *self_, const void *value)
>>>>> +{
>>>>> +     struct IVec_c_void *self = self_;
>>>>> +     void *dst = NULL;
>>>>> +
>>>>> +     if (self->length == self->capacity)
>>>>> +             ivec_reserve(self, 1);
>>>>> +
>>>>> +     dst = (uint8_t*)self->ptr + self->length * self->element_size;
>>>>> +     memcpy(dst, value, self->element_size);
>>>>
>>>> If self->element_size was a compile time constant the compiler could
>>>> easily optimize this call away. I'm not sure that is easy to achieve though.
>>>
>>> The problem is that I didn't want all of ivec to be macros that looked
>>> like function calls. I wanted to minimize use of macros so that it was
>>> easier to port and verify that the Rust implementation matches the
>>> behavior of the C implementation.
>>
>> I think that's a reasonable concern. So is the plan to have a parallel
>> rust implementation of these functions rather than call the C
>> implementation from rust?
> > Yes, the Rust implementation will be independent of the C
> implementation, but will behave the same way. That's why I'm calling
> it an interoperable vec as opposed to a compatible vec. Rust can't
> call the C ivec functions and C can't call the Rust ivec functions,
> but they'll behave the same way.

Interesting - I'm curious what the advantage of that is over having rust call the C implementation? I can see you wouldn't want to be calling into C for each ivec.push() call, but checking if there is room to push the new element in rust and calling into C to extend the vector if not should be reasonable and then you don't have to re-implement everything in rust.

>>>>> +void ivec_free(void *self_)
>>>>
>>>> Normally we'd call a like this that free the allocations and
>>>> re-initializes the members ivec_clear()
>>>
>>> In Rust Vec.clear() means to set length to zero, but leaves the
>>> allocation alone. The reason why I'm zeroing the struct is to help
>>> avoid FFI issues. If not zero then what should the members be set to,
>>> to indicate that using the struct is not valid anymore? In Rust an
>>> object is freed when it goes out of scope and _cannot_ be accessed
>>> afterward.
> > Maybe I should call this ivec_drop(). Though the notion of explicitly
> freeing an object in Rust is _almost_ nonsense. The way you free
> something in Rust is to let it go out of scope.

Indeed - which means this wont be a public function in rust and so why do we worry about naming it ivec_clear()? At least ivec_drop() does not conflict with any of the standard function suffixes that we're already using in git.

>> I'm aware that Vec::clear() has different semantics (it does what
>> strbuf_reset() does). That's unfortunate but this function has different
>> semantics to all the other *_free() functions in git. Our coding
>> guidelines say
>>
>>    - There are several common idiomatic names for functions performing
>>      specific tasks on a structure `S`:
>>
>>       - `S_init()` initializes a structure without allocating the
>>         structure itself.
>>
>>       - `S_release()` releases a structure's contents without freeing the
>>         structure.
>>
>>       - `S_clear()` is equivalent to `S_release()` followed by `S_init()`
>>         such that the structure is directly usable after clearing it. When
>>         `S_clear()` is provided, `S_init()` shall not allocate resources
>>         that need to be released again.
>>
>>       - `S_free()` releases a structure's contents and frees the
>>         structure.
>>
>> As we write more rust code and so wrap more of our existing structs
>> we're going to be wrapping C code that uses the definitions above so I
>> think we should do the same with struct IVec_*.
> > I disagree. IVec isn't a wrapper around an existing struct.

So just because it is a new stuct it shouldn't have to follow the existing naming conventions?

> ivec is
> meant to very closely mimic Rust's Vec while guaranteeing
> interoperability. For things like strbuf I haven't conceived of a
> solution for that yet. Making ivec diverge from Rust's Vec will result
> in POLA violations due to different behavior when refactoring an
> IVec<your_type_here> to Vec<your_type_here>.

On the other hand, vec.reset() does not exist so you'd get a compiler error if you forgot to rename those calls when changing from IVec to Vec and the rust code wouldn't be calling ivec.clear(). I'm not sure citing POLA concerns is very convincing as ivec_free() in C is a POLA violation for anyone familiar with git's code base so it's not like there's a choice that avoids that concern.

Thanks

Phillip

>>>>> diff --git a/compat/ivec.h b/compat/ivec.h
>>>>> new file mode 100644
>>>>> index 0000000000..654a05c506
>>>>> --- /dev/null
>>>>> +++ b/compat/ivec.h
>>>>> @@ -0,0 +1,52 @@
>>>>> +#ifndef IVEC_H
>>>>> +#define IVEC_H
>>>>> +
>>>>> +#include <git-compat-util.h>
>>>>
>>>> It would be nice to have some documentation in this header, see the
>>>> examples in strvec.h and hashmap.h
>>>>
>>>>> +#define IVEC_INIT(variable) ivec_init(&(variable), sizeof(*(variable).ptr))
>>>>
>>>> This is a bit cumbersome to use compared to our usual *_INIT macros. I'm
>>>> struggling to see how we can make it nicer though as DEFINE_IVEC_TYPE
>>>> cannot define a per-type initializer macro and I we cannot initialize
>>>> the element size without knowing the type.
>>>
>>> I don't see what's cumbersome about it. Maybe an example use case
>>> would clarify things.
>>
>> It is cumbersome because it separates the initialization from the
>> declaration. Normally our *_INIT macros are initializer lists so we can
>> write
>>
>>          struct strbuf = STRBUF_INIT;
>>
>> which keeps the declaration and initialization together. Although
>> they're on adjacent lines in your example in real code the
>> initialization likely to be separated from the declaration by other
>> variable declarations.
> > Ah I see what you mean now. I'll experiment with making IVEC_INIT()
> work like that. One wrinkle is that STRBUF_INIT is a single concrete
> type whereas IVEC_INIT() is meant for generic types.

If you can get it to work that would be great, but I can't think of a way of getting it to work for a generic type.

Thanks

Phillip

On 1/2/26 7:52 PM, Ezekiel Newren via GitGitGadget wrote:
> diff --git a/compat/ivec.c b/compat/ivec.c
> new file mode 100644
> index 0000000000..0a777e78dc
> --- /dev/null
> +++ b/compat/ivec.c
> @@ -0,0 +1,113 @@
> +#include "ivec.h"
> +
> +struct IVec_c_void {
> +	void *ptr;
> +	size_t length;
> +	size_t capacity;
> +	size_t element_size;
> +};
> +
> +static void _set_capacity(void *self_, size_t new_capacity)
> +{
> +	struct IVec_c_void *self = self_;
> +
> +	if (new_capacity == self->capacity) {
> +		return;
> +	}
> +	if (new_capacity == 0) {
> +		free(self->ptr);
> +		self->ptr = NULL;
> +	} else {
> +		self->ptr = realloc(self->ptr, new_capacity * self->element_size);
> +	}
> +	self->capacity = new_capacity;
> +}
> +
> +
> +void ivec_init(void *self_, size_t element_size)
> +{
> +	struct IVec_c_void *self = self_;
> +
> +	self->ptr = NULL;
> +	self->length = 0;
> +	self->capacity = 0;
> +	self->element_size = element_size;
> +}
> +
> +void ivec_zero(void *self_, size_t capacity)
> +{
> +	struct IVec_c_void *self = self_;
> +
> +	self->ptr = calloc(capacity, self->element_size);
> +	self->length = capacity;
> +	self->capacity = capacity;
> +	// DO NOT MODIFY element_size!!!
> +}
> +
> +void ivec_reserve_exact(void *self_, size_t additional)
> +{
> +	struct IVec_c_void *self = self_;
> +
> +	_set_capacity(self, self->capacity + additional);
> +}
> +
> +void ivec_reserve(void *self_, size_t additional)
> +{
> +	struct IVec_c_void *self = self_;
> +
> +	size_t growby = 128;
> +	if (self->capacity > growby)
> +		growby = self->capacity;
> +	if (additional > growby)
> +		growby = additional;
> +
> +	_set_capacity(self, self->capacity + growby);
> +}

Constant growth steps like these cause linear growth and quadratic
complexity.  ALLOC_GROW does exponential growth with factor 1.5 to
get linear complexity.  Here's an old plea to do the same:
https://blog.mozilla.org/nnethercote/2014/11/04/please-grow-your-buffers-exponentially/

René

On Fri, Jan 16, 2026 at 1:19 PM René Scharfe <l.s.r@web.de> wrote:
>
> On 1/2/26 7:52 PM, Ezekiel Newren via GitGitGadget wrote:
> > diff --git a/compat/ivec.c b/compat/ivec.c
> > new file mode 100644
> > index 0000000000..0a777e78dc
> > --- /dev/null
> > +++ b/compat/ivec.c
> > @@ -0,0 +1,113 @@
> > +#include "ivec.h"
> > +
> > +struct IVec_c_void {
> > +     void *ptr;
> > +     size_t length;
> > +     size_t capacity;
> > +     size_t element_size;
> > +};
> > +
> > +static void _set_capacity(void *self_, size_t new_capacity)
> > +{
> > +     struct IVec_c_void *self = self_;
> > +
> > +     if (new_capacity == self->capacity) {
> > +             return;
> > +     }
> > +     if (new_capacity == 0) {
> > +             free(self->ptr);
> > +             self->ptr = NULL;
> > +     } else {
> > +             self->ptr = realloc(self->ptr, new_capacity * self->element_size);
> > +     }
> > +     self->capacity = new_capacity;
> > +}
> > +
> > +
> > +void ivec_init(void *self_, size_t element_size)
> > +{
> > +     struct IVec_c_void *self = self_;
> > +
> > +     self->ptr = NULL;
> > +     self->length = 0;
> > +     self->capacity = 0;
> > +     self->element_size = element_size;
> > +}
> > +
> > +void ivec_zero(void *self_, size_t capacity)
> > +{
> > +     struct IVec_c_void *self = self_;
> > +
> > +     self->ptr = calloc(capacity, self->element_size);
> > +     self->length = capacity;
> > +     self->capacity = capacity;
> > +     // DO NOT MODIFY element_size!!!
> > +}
> > +
> > +void ivec_reserve_exact(void *self_, size_t additional)
> > +{
> > +     struct IVec_c_void *self = self_;
> > +
> > +     _set_capacity(self, self->capacity + additional);
> > +}
> > +
> > +void ivec_reserve(void *self_, size_t additional)
> > +{
> > +     struct IVec_c_void *self = self_;
> > +
> > +     size_t growby = 128;
> > +     if (self->capacity > growby)
> > +             growby = self->capacity;
> > +     if (additional > growby)
> > +             growby = additional;
> > +
> > +     _set_capacity(self, self->capacity + growby);
> > +}
>
> Constant growth steps like these cause linear growth and quadratic
> complexity.  ALLOC_GROW does exponential growth with factor 1.5 to
> get linear complexity.  Here's an old plea to do the same:
> https://blog.mozilla.org/nnethercote/2014/11/04/please-grow-your-buffers-exponentially/
>
> René

It _is_ exponential. ivec_reserve(&vec, 1) means grow by _at least_ 1.
I'm not using typical memory management as defined in
git-compat-util.h because I'm trying to get ivec to behave very
similarly to Rust's Vec so that when Rust is introduced into the code,
C programmers will already be familiar with how Vec operates _and_ so
that converting from IVec to Vec is as simple as refactoring IVec
declarations to Vec.

Since C does not support generics there is no _proper_ solution. What
I have come up with on the C side for ivec is my best effort
compromise.

On 1/17/26 4:58 PM, Ezekiel Newren wrote:
> On Fri, Jan 16, 2026 at 1:19 PM René Scharfe <l.s.r@web.de> wrote:
>>
>>> +void ivec_reserve(void *self_, size_t additional)
>>> +{
>>> +     struct IVec_c_void *self = self_;
>>> +
>>> +     size_t growby = 128;
>>> +     if (self->capacity > growby)
>>> +             growby = self->capacity;
>>> +     if (additional > growby)
>>> +             growby = additional;
>>> +
>>> +     _set_capacity(self, self->capacity + growby);
>>> +}
>>
>> Constant growth steps like these cause linear growth and quadratic
>> complexity.  ALLOC_GROW does exponential growth with factor 1.5 to
>> get linear complexity.  Here's an old plea to do the same:
>> https://blog.mozilla.org/nnethercote/2014/11/04/please-grow-your-buffers-exponentially/
>>
>> René
> 
> It _is_ exponential. ivec_reserve(&vec, 1) means grow by _at least_ 1.
D'oh!  Right, it grows with factor 2, as growby is at least as big as
->capacity.  I can't read.

René

On 1/2/26 7:52 PM, Ezekiel Newren via GitGitGadget wrote:
> @@ -253,22 +250,44 @@ static bool xdl_clean_mmatch(uint8_t const *action, long i, long s, long e) {
>  	return rpdis1 * XDL_KPDIS_RUN < (rpdis1 + rdis1);
>  }
>  
> +struct xoccurrence
> +{
> +	size_t file1, file2;
> +};
> +
> +
> +DEFINE_IVEC_TYPE(struct xoccurrence, xoccurrence);
> +
>  
>  /*
>   * Try to reduce the problem complexity, discard records that have no
>   * matches on the other file. Also, lines that have multiple matches
>   * might be potentially discarded if they appear in a run of discardable.
>   */
> -static int xdl_cleanup_records(xdlclassifier_t *cf, xdfenv_t *xe) {
> -	long i, nm, mlim;
> +static int xdl_cleanup_records(xdfenv_t *xe, uint64_t flags) {
> +	long i;
> +	size_t nm, mlim;
>  	xrecord_t *recs;
> -	xdlclass_t *rcrec;
>  	uint8_t *action1 = NULL, *action2 = NULL;
> -	bool need_min = !!(cf->flags & XDF_NEED_MINIMAL);
> +	struct IVec_xoccurrence occ;
> +	bool need_min = !!(flags & XDF_NEED_MINIMAL);
>  	int ret = 0;
>  	ptrdiff_t dend1 = xe->xdf1.nrec - 1 - xe->delta_end;
>  	ptrdiff_t dend2 = xe->xdf2.nrec - 1 - xe->delta_end;
>  
> +	IVEC_INIT(occ);
> +	ivec_zero(&occ, xe->mph_size);

This array is presized here.  It is neither grown nor shrunken.
CALLOC_ARRAY would work just as well, at least at this point, no?

> +
> +	for (size_t j = 0; j < xe->xdf1.nrec; j++) {
> +		size_t mph1 = xe->xdf1.recs[j].minimal_perfect_hash;
> +		occ.ptr[mph1].file1 += 1;
> +	}
> +
> +	for (size_t j = 0; j < xe->xdf2.nrec; j++) {
> +		size_t mph2 = xe->xdf2.recs[j].minimal_perfect_hash;
> +		occ.ptr[mph2].file2 += 1;
> +	}
> +
>  	/*
>  	 * Create temporary arrays that will help us decide if
>  	 * changed[i] should remain false, or become true.
> @@ -288,16 +307,14 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfenv_t *xe) {
>  	if ((mlim = xdl_bogosqrt((long)xe->xdf1.nrec)) > XDL_MAX_EQLIMIT)
>  		mlim = XDL_MAX_EQLIMIT;
>  	for (i = xe->delta_start, recs = &xe->xdf1.recs[xe->delta_start]; i <= dend1; i++, recs++) {
> -		rcrec = cf->rcrecs[recs->minimal_perfect_hash];
> -		nm = rcrec ? rcrec->len2 : 0;
> +		nm = occ.ptr[recs->minimal_perfect_hash].file2;
>  		action1[i] = (nm == 0) ? DISCARD: (nm >= mlim && !need_min) ? INVESTIGATE: KEEP;
>  	}
>  
>  	if ((mlim = xdl_bogosqrt((long)xe->xdf2.nrec)) > XDL_MAX_EQLIMIT)
>  		mlim = XDL_MAX_EQLIMIT;
>  	for (i = xe->delta_start, recs = &xe->xdf2.recs[xe->delta_start]; i <= dend2; i++, recs++) {
> -		rcrec = cf->rcrecs[recs->minimal_perfect_hash];
> -		nm = rcrec ? rcrec->len1 : 0;
> +		nm = occ.ptr[recs->minimal_perfect_hash].file1;
>  		action2[i] = (nm == 0) ? DISCARD: (nm >= mlim && !need_min) ? INVESTIGATE: KEEP;
>  	}
>  
> @@ -332,6 +349,7 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfenv_t *xe) {
>  cleanup:
>  	xdl_free(action1);
>  	xdl_free(action2);
> +	ivec_free(&occ);
>  
>  	return ret;
>  }

On Fri, Jan 16, 2026 at 1:19 PM René Scharfe <l.s.r@web.de> wrote:
>
> On 1/2/26 7:52 PM, Ezekiel Newren via GitGitGadget wrote:
> > @@ -253,22 +250,44 @@ static bool xdl_clean_mmatch(uint8_t const *action, long i, long s, long e) {
> >       return rpdis1 * XDL_KPDIS_RUN < (rpdis1 + rdis1);
> >  }
> >
> > +struct xoccurrence
> > +{
> > +     size_t file1, file2;
> > +};
> > +
> > +
> > +DEFINE_IVEC_TYPE(struct xoccurrence, xoccurrence);
> > +
> >
> >  /*
> >   * Try to reduce the problem complexity, discard records that have no
> >   * matches on the other file. Also, lines that have multiple matches
> >   * might be potentially discarded if they appear in a run of discardable.
> >   */
> > -static int xdl_cleanup_records(xdlclassifier_t *cf, xdfenv_t *xe) {
> > -     long i, nm, mlim;
> > +static int xdl_cleanup_records(xdfenv_t *xe, uint64_t flags) {
> > +     long i;
> > +     size_t nm, mlim;
> >       xrecord_t *recs;
> > -     xdlclass_t *rcrec;
> >       uint8_t *action1 = NULL, *action2 = NULL;
> > -     bool need_min = !!(cf->flags & XDF_NEED_MINIMAL);
> > +     struct IVec_xoccurrence occ;
> > +     bool need_min = !!(flags & XDF_NEED_MINIMAL);
> >       int ret = 0;
> >       ptrdiff_t dend1 = xe->xdf1.nrec - 1 - xe->delta_end;
> >       ptrdiff_t dend2 = xe->xdf2.nrec - 1 - xe->delta_end;
> >
> > +     IVEC_INIT(occ);
> > +     ivec_zero(&occ, xe->mph_size);
>
> This array is presized here.  It is neither grown nor shrunken.
> CALLOC_ARRAY would work just as well, at least at this point, no?
>
> > +
> > +     for (size_t j = 0; j < xe->xdf1.nrec; j++) {
> > +             size_t mph1 = xe->xdf1.recs[j].minimal_perfect_hash;
> > +             occ.ptr[mph1].file1 += 1;
> > +     }
> > +
> > +     for (size_t j = 0; j < xe->xdf2.nrec; j++) {
> > +             size_t mph2 = xe->xdf2.recs[j].minimal_perfect_hash;
> > +             occ.ptr[mph2].file2 += 1;
> > +     }
> > +
> >       /*
> >        * Create temporary arrays that will help us decide if
> >        * changed[i] should remain false, or become true.
> > @@ -288,16 +307,14 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfenv_t *xe) {
> >       if ((mlim = xdl_bogosqrt((long)xe->xdf1.nrec)) > XDL_MAX_EQLIMIT)
> >               mlim = XDL_MAX_EQLIMIT;
> >       for (i = xe->delta_start, recs = &xe->xdf1.recs[xe->delta_start]; i <= dend1; i++, recs++) {
> > -             rcrec = cf->rcrecs[recs->minimal_perfect_hash];
> > -             nm = rcrec ? rcrec->len2 : 0;
> > +             nm = occ.ptr[recs->minimal_perfect_hash].file2;
> >               action1[i] = (nm == 0) ? DISCARD: (nm >= mlim && !need_min) ? INVESTIGATE: KEEP;
> >       }
> >
> >       if ((mlim = xdl_bogosqrt((long)xe->xdf2.nrec)) > XDL_MAX_EQLIMIT)
> >               mlim = XDL_MAX_EQLIMIT;
> >       for (i = xe->delta_start, recs = &xe->xdf2.recs[xe->delta_start]; i <= dend2; i++, recs++) {
> > -             rcrec = cf->rcrecs[recs->minimal_perfect_hash];
> > -             nm = rcrec ? rcrec->len1 : 0;
> > +             nm = occ.ptr[recs->minimal_perfect_hash].file1;
> >               action2[i] = (nm == 0) ? DISCARD: (nm >= mlim && !need_min) ? INVESTIGATE: KEEP;
> >       }
> >
> > @@ -332,6 +349,7 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfenv_t *xe) {
> >  cleanup:
> >       xdl_free(action1);
> >       xdl_free(action2);
> > +     ivec_free(&occ);
> >
> >       return ret;
> >  }

In Rust the memory management macros defined in git-compat-util.h will
not be available. ivec was built expressly to bridge the gap between C
and Rust. I'm avoiding using those macros because I'm trying to get C
programmers familiar with how Rust's Vec operates without forcing them
to read and write in Rust. Also, it makes converting from IVec to Vec
super easy.

ivec_zero() also sets length and capacity. Also CALLOC_ARRAY needs to
know the type of the pointer which ivec_zero() does not have access
to. This is one of the few ivec functions that does not have a direct
equivalent in Rust's Vec, but is faster than what is logically
equivalent in Rust.

In Rust the closest safe equivalent would look like:

let size = 35;
let mut vec = Vec::<u64>::new();
vec.reserve_exact(size);
vec.fill(0);  // requires that T implements the `Copy` trait

The unsafe version would look like:
let size = 35;
let mut vec = Vec::<u64>::new();
vec.reserve_exact(size);
unsafe {
    std::ptr::write_bytes(vec.as_mut_ptr(), 0, size * size_of::<u64>());
}

On 1/17/26 5:34 PM, Ezekiel Newren wrote:
> On Fri, Jan 16, 2026 at 1:19 PM René Scharfe <l.s.r@web.de> wrote:
>>
>> On 1/2/26 7:52 PM, Ezekiel Newren via GitGitGadget wrote:
>>> @@ -253,22 +250,44 @@ static bool xdl_clean_mmatch(uint8_t const *action, long i, long s, long e) {
>>>       return rpdis1 * XDL_KPDIS_RUN < (rpdis1 + rdis1);
>>>  }
>>>
>>> +struct xoccurrence
>>> +{
>>> +     size_t file1, file2;
>>> +};
>>> +
>>> +
>>> +DEFINE_IVEC_TYPE(struct xoccurrence, xoccurrence);
>>> +
>>>
>>>  /*
>>>   * Try to reduce the problem complexity, discard records that have no
>>>   * matches on the other file. Also, lines that have multiple matches
>>>   * might be potentially discarded if they appear in a run of discardable.
>>>   */
>>> -static int xdl_cleanup_records(xdlclassifier_t *cf, xdfenv_t *xe) {
>>> -     long i, nm, mlim;
>>> +static int xdl_cleanup_records(xdfenv_t *xe, uint64_t flags) {
>>> +     long i;
>>> +     size_t nm, mlim;
>>>       xrecord_t *recs;
>>> -     xdlclass_t *rcrec;
>>>       uint8_t *action1 = NULL, *action2 = NULL;
>>> -     bool need_min = !!(cf->flags & XDF_NEED_MINIMAL);
>>> +     struct IVec_xoccurrence occ;
>>> +     bool need_min = !!(flags & XDF_NEED_MINIMAL);
>>>       int ret = 0;
>>>       ptrdiff_t dend1 = xe->xdf1.nrec - 1 - xe->delta_end;
>>>       ptrdiff_t dend2 = xe->xdf2.nrec - 1 - xe->delta_end;
>>>
>>> +     IVEC_INIT(occ);
>>> +     ivec_zero(&occ, xe->mph_size);
>>
>> This array is presized here.  It is neither grown nor shrunken.
>> CALLOC_ARRAY would work just as well, at least at this point, no?
>>
>>> +
>>> +     for (size_t j = 0; j < xe->xdf1.nrec; j++) {
>>> +             size_t mph1 = xe->xdf1.recs[j].minimal_perfect_hash;
>>> +             occ.ptr[mph1].file1 += 1;
>>> +     }
>>> +
>>> +     for (size_t j = 0; j < xe->xdf2.nrec; j++) {
>>> +             size_t mph2 = xe->xdf2.recs[j].minimal_perfect_hash;
>>> +             occ.ptr[mph2].file2 += 1;
>>> +     }
>>> +
>>>       /*
>>>        * Create temporary arrays that will help us decide if
>>>        * changed[i] should remain false, or become true.
>>> @@ -288,16 +307,14 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfenv_t *xe) {
>>>       if ((mlim = xdl_bogosqrt((long)xe->xdf1.nrec)) > XDL_MAX_EQLIMIT)
>>>               mlim = XDL_MAX_EQLIMIT;
>>>       for (i = xe->delta_start, recs = &xe->xdf1.recs[xe->delta_start]; i <= dend1; i++, recs++) {
>>> -             rcrec = cf->rcrecs[recs->minimal_perfect_hash];
>>> -             nm = rcrec ? rcrec->len2 : 0;
>>> +             nm = occ.ptr[recs->minimal_perfect_hash].file2;
>>>               action1[i] = (nm == 0) ? DISCARD: (nm >= mlim && !need_min) ? INVESTIGATE: KEEP;
>>>       }
>>>
>>>       if ((mlim = xdl_bogosqrt((long)xe->xdf2.nrec)) > XDL_MAX_EQLIMIT)
>>>               mlim = XDL_MAX_EQLIMIT;
>>>       for (i = xe->delta_start, recs = &xe->xdf2.recs[xe->delta_start]; i <= dend2; i++, recs++) {
>>> -             rcrec = cf->rcrecs[recs->minimal_perfect_hash];
>>> -             nm = rcrec ? rcrec->len1 : 0;
>>> +             nm = occ.ptr[recs->minimal_perfect_hash].file1;
>>>               action2[i] = (nm == 0) ? DISCARD: (nm >= mlim && !need_min) ? INVESTIGATE: KEEP;
>>>       }
>>>
>>> @@ -332,6 +349,7 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfenv_t *xe) {
>>>  cleanup:
>>>       xdl_free(action1);
>>>       xdl_free(action2);
>>> +     ivec_free(&occ);
>>>
>>>       return ret;
>>>  }
> 
> In Rust the memory management macros defined in git-compat-util.h will
> not be available. ivec was built expressly to bridge the gap between C
> and Rust. I'm avoiding using those macros because I'm trying to get C
> programmers familiar with how Rust's Vec operates without forcing them
> to read and write in Rust. Also, it makes converting from IVec to Vec
> super easy.
> 
> ivec_zero() also sets length and capacity. Also CALLOC_ARRAY needs to
> know the type of the pointer which ivec_zero() does not have access
> to. This is one of the few ivec functions that does not have a direct
> equivalent in Rust's Vec, but is faster than what is logically
> equivalent in Rust.
> 
> In Rust the closest safe equivalent would look like:
> 
> let size = 35;
> let mut vec = Vec::<u64>::new();
> vec.reserve_exact(size);
> vec.fill(0);  // requires that T implements the `Copy` trait
> 
> The unsafe version would look like:
> let size = 35;
> let mut vec = Vec::<u64>::new();
> vec.reserve_exact(size);
> unsafe {
>     std::ptr::write_bytes(vec.as_mut_ptr(), 0, size * size_of::<u64>());
> }

I was being unclear and made a few assumptions here.  My point was just
that this is a fixed-size array and doesn't need to be stored in a
variable-sized container.  This is the first Ivec user, and I would have
expected it to exercise the push function.  I assume accessing a
fixed-size array via FFI would be a lot easier since allocation and
growth are out of the picture.

René

On 02/01/2026 18:52, Ezekiel Newren via GitGitGadget wrote:
> From: Ezekiel Newren <ezekielnewren@gmail.com>
> > Later patches will prepare xdl_cleanup_records() to be moved into xdiffi.c
> since only the classic diff uses that function.

I assume that's to make it easier to covert the myers implementation to rust without affecting the rest of the code? If so it would be nice to say that.

> Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com>

> +int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
> +		xdfenv_t *xe) {
> +	int res;
> +
> +	if (xdl_prepare_env(mf1, mf2, xpp, xe) < 0)
> +		return -1;
> +
> +	if (XDF_DIFF_ALG(xpp->flags) == XDF_PATIENCE_DIFF) {
> +		res = xdl_do_patience_diff(xpp, xe);
> +		goto out;
> +	}
> +
> +	if (XDF_DIFF_ALG(xpp->flags) == XDF_HISTOGRAM_DIFF) {
> +		res = xdl_do_histogram_diff(xpp, xe);
> +		goto out;
> +	}
> +
> +	res = xdl_do_classic_diff(xe, xpp->flags);

This might be clearer that we're calling only one of the three functions if we wrote this as

	if (XDF_DIFF_ALG(xpp->flags) == XDIF_PATIENCE_DIFF)
		res = xdl_do_patience_diff(xpp, xe);
	else if (XDF_DIFF_ALG(xpp->flags) == XDF_HISTOGRAM_DIFF)
		res = xdl_do_histogram_diff(xpp, xe);
	else
		res = xdl_do_classic_diff(xe, xpp->flags);

and then we can drop the out: label

Thanks

Phillip

>    out:
>   	if (res < 0)
>   		xdl_free_env(xe);
> diff --git a/xdiff/xdiffi.h b/xdiff/xdiffi.h
> index 49e52c67f9..8bf4c20373 100644
> --- a/xdiff/xdiffi.h
> +++ b/xdiff/xdiffi.h
> @@ -42,6 +42,7 @@ typedef struct s_xdchange {
>   int xdl_recs_cmp(xdfile_t *xdf1, long off1, long lim1,
>   		 xdfile_t *xdf2, long off2, long lim2,
>   		 long *kvdf, long *kvdb, int need_min, xdalgoenv_t *xenv);
> +int xdl_do_classic_diff(xdfenv_t *xe, uint64_t flags);
>   int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
>   		xdfenv_t *xe);
>   int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags);

On Tue, Jan 20, 2026 at 8:01 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> On 02/01/2026 18:52, Ezekiel Newren via GitGitGadget wrote:
> > From: Ezekiel Newren <ezekielnewren@gmail.com>
> >
> > Later patches will prepare xdl_cleanup_records() to be moved into xdiffi.c
> > since only the classic diff uses that function.
>
> I assume that's to make it easier to covert the myers implementation to
> rust without affecting the rest of the code? If so it would be nice to
> say that.

Making it easier to port to Rust is a side effect. The primary goal is
to simplify the job of xprepare to only parsing and hashing lines in a
file. xdl_cleanup_records() is only used by classic diff
(myers/minimal) which means it doesn't belong in xprepare because it's
part of a diff algorithm and isn't relevant to preparing the file for
a diff algorithm. Perhaps xdl_trim_ends() should be moved into
xdl_do_diff() too...

> > Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com>
>
> > +int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
> > +             xdfenv_t *xe) {
> > +     int res;
> > +
> > +     if (xdl_prepare_env(mf1, mf2, xpp, xe) < 0)
> > +             return -1;
> > +
> > +     if (XDF_DIFF_ALG(xpp->flags) == XDF_PATIENCE_DIFF) {
> > +             res = xdl_do_patience_diff(xpp, xe);
> > +             goto out;
> > +     }
> > +
> > +     if (XDF_DIFF_ALG(xpp->flags) == XDF_HISTOGRAM_DIFF) {
> > +             res = xdl_do_histogram_diff(xpp, xe);
> > +             goto out;
> > +     }
> > +
> > +     res = xdl_do_classic_diff(xe, xpp->flags);
>
> This might be clearer that we're calling only one of the three functions
> if we wrote this as
>
>         if (XDF_DIFF_ALG(xpp->flags) == XDIF_PATIENCE_DIFF)
>                 res = xdl_do_patience_diff(xpp, xe);
>         else if (XDF_DIFF_ALG(xpp->flags) == XDF_HISTOGRAM_DIFF)
>                 res = xdl_do_histogram_diff(xpp, xe);
>         else
>                 res = xdl_do_classic_diff(xe, xpp->flags);
>
> and then we can drop the out: label

In a later cleanup, I make this exact change :)

On 02/01/2026 18:52, Ezekiel Newren via GitGitGadget wrote:
> From: Ezekiel Newren <ezekielnewren@gmail.com>
> > All lines must be read anyway, so classify them after they're read in.
> Also move the memset() into xdl_init_classifier().

So instead of looping over the input lines one and a bit times (the bit being from xdl_guess_lines) we now loop over them twice as we split them first and then classify them in a separate loop. It does save some work not to call xdl_guess_lines but it is unclear if that offsets classifying them in a separate loop.

> +	for (size_t i = 0; i < xe->xdf1.nrec; i++) {
> +		xrecord_t *rec = &xe->xdf1.recs[i];
> +		xdl_classify_record(1, &cf, rec);

We seem to have lost the error handling if xdl_classify_record() fails.

Thanks

Phillip

> +	}
> +
> +	for (size_t i = 0; i < xe->xdf2.nrec; i++) {
> +		xrecord_t *rec = &xe->xdf2.recs[i];
> +		xdl_classify_record(2, &cf, rec);
>   	}
>   >   	if ((XDF_DIFF_ALG(xpp->flags) != XDF_PATIENCE_DIFF) &&
> diff --git a/xdiff/xutils.c b/xdiff/xutils.c
> index 77ee1ad9c8..b3d51197c1 100644
> --- a/xdiff/xutils.c
> +++ b/xdiff/xutils.c
> @@ -118,26 +118,6 @@ void *xdl_cha_alloc(chastore_t *cha) {
>   	return data;
>   }
>   > -long xdl_guess_lines(mmfile_t *mf, long sample) {
> -	long nl = 0, size, tsize = 0;
> -	char const *data, *cur, *top;
> -
> -	if ((cur = data = xdl_mmfile_first(mf, &size))) {
> -		for (top = data + size; nl < sample && cur < top; ) {
> -			nl++;
> -			if (!(cur = memchr(cur, '\n', top - cur)))
> -				cur = top;
> -			else
> -				cur++;
> -		}
> -		tsize += (long) (cur - data);
> -	}
> -
> -	if (nl && tsize)
> -		nl = xdl_mmfile_size(mf) / (tsize / nl);
> -
> -	return nl + 1;
> -}
>   >   int xdl_blankline(const char *line, long size, long flags)
>   {
> diff --git a/xdiff/xutils.h b/xdiff/xutils.h
> index 615b4a9d35..d800840dd0 100644
> --- a/xdiff/xutils.h
> +++ b/xdiff/xutils.h
> @@ -31,7 +31,6 @@ int xdl_emit_diffrec(char const *rec, long size, char const *pre, long psize,
>   int xdl_cha_init(chastore_t *cha, long isize, long icount);
>   void xdl_cha_free(chastore_t *cha);
>   void *xdl_cha_alloc(chastore_t *cha);
> -long xdl_guess_lines(mmfile_t *mf, long sample);
>   int xdl_blankline(const char *line, long size, long flags);
>   int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags);
>   uint64_t xdl_hash_record_verbatim(uint8_t const **data, uint8_t const *top);

On Tue, Jan 20, 2026 at 8:02 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> On 02/01/2026 18:52, Ezekiel Newren via GitGitGadget wrote:
> > From: Ezekiel Newren <ezekielnewren@gmail.com>
> >
> > All lines must be read anyway, so classify them after they're read in.
> > Also move the memset() into xdl_init_classifier().
>
> So instead of looping over the input lines one and a bit times (the bit
> being from xdl_guess_lines) we now loop over them twice as we split them
> first and then classify them in a separate loop. It does save some work
> not to call xdl_guess_lines but it is unclear if that offsets
> classifying them in a separate loop.
>
> > +     for (size_t i = 0; i < xe->xdf1.nrec; i++) {
> > +             xrecord_t *rec = &xe->xdf1.recs[i];
> > +             xdl_classify_record(1, &cf, rec);
>
> We seem to have lost the error handling if xdl_classify_record() fails.

The error handling was not "lost" it was deliberately removed. The
only way in which xdl_classify_record() could fail is by a failed
memory allocation. On the Rust side this would result in a panic
(panic means something different in Rust vs C) in which case C could
not possibly recover. Also for operations like Vec.push() in Rust it's
assumed that memory management functions will never fail and if they
do they crash the program with no chance of recovery (unless you
account for panic unwinding which is really ugly). It seems a lot of
arguments about ivec and my xdiff cleanups are "We don't do things
this way in Git/C" I'm aware of many of these arguments and I'm trying
to address them with a more specific answer of "Yes, but that's not
how things are done in Rust and all of this is to prepare the code for
conversion to Rust and some things shouldn't, or even, cannot be done
the C way in Rust."

On 21/01/2026 21:12, Ezekiel Newren wrote:
> On Tue, Jan 20, 2026 at 8:02 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> On 02/01/2026 18:52, Ezekiel Newren via GitGitGadget wrote:
>>> From: Ezekiel Newren <ezekielnewren@gmail.com>
>>>
>>> All lines must be read anyway, so classify them after they're read in.
>>> Also move the memset() into xdl_init_classifier().
>>
>> So instead of looping over the input lines one and a bit times (the bit
>> being from xdl_guess_lines) we now loop over them twice as we split them
>> first and then classify them in a separate loop. It does save some work
>> not to call xdl_guess_lines but it is unclear if that offsets
>> classifying them in a separate loop.
>>
>>> +     for (size_t i = 0; i < xe->xdf1.nrec; i++) {
>>> +             xrecord_t *rec = &xe->xdf1.recs[i];
>>> +             xdl_classify_record(1, &cf, rec);
>>
>> We seem to have lost the error handling if xdl_classify_record() fails.
> > The error handling was not "lost" it was deliberately removed. That's the sort of thing that needs to be explained in the commit message.

> The
> only way in which xdl_classify_record() could fail is by a failed
> memory allocation. On the Rust side this would result in a panic
> (panic means something different in Rust vs C) in which case C could
> not possibly recover.

There is no rust code in xdiff at the moment so we don't panic on failure. In git we'll die() because xdl_malloc() and friends are defined as xmalloc() etc. which die on allocation failure. However anyone else picking up this code and using a different allocator that does not die on allocation failure will expect the error to be propagated.

If you want to stop supporting other allocators then you should propose a patch to do so, not silently slip the change into this patch.

Thanks

Phillip

> Also for operations like Vec.push() in Rust it's
> assumed that memory management functions will never fail and if they
> do they crash the program with no chance of recovery (unless you
> account for panic unwinding which is really ugly). It seems a lot of
> arguments about ivec and my xdiff cleanups are "We don't do things
> this way in Git/C" I'm aware of many of these arguments and I'm trying
> to address them with a more specific answer of "Yes, but that's not
> how things are done in Rust and all of this is to prepare the code for
> conversion to Rust and some things shouldn't, or even, cannot be done
> the C way in Rust."

On 02/01/2026 18:52, Ezekiel Newren via GitGitGadget wrote:
> From: Ezekiel Newren <ezekielnewren@gmail.com>
> > The patience diff is set up the exact same way as histogram, see
> xdl_do_historgram_diff() in xhistogram.c. xdl_optimize_ctxs() is
> redundant now, delete it.

Does this change the output? The patience diff looks for unique context lines and builds the context out from those. For files that look like

Old	New
A	A
B	B
C	A
B	B
A	C
	B
	A

That will give a hunk

@@ -1,3 +0,5 @@
+A
+B
 A
 B
 C

but trimming the common prefix first would give

@@ -1,5 +1,7
 A
 B
+A
+B
 C
 B
 A

Though it seems like the diff silder causes us to output the same diff in both cases for that simple test so maybe it is not an issue. It would certainly be helpful to comment on any possible changes in the commit message as it could have been a deliberate choice not to trim the ends for those algorithms.

> -static int xdl_optimize_ctxs(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xdf2) {
> -
> -	if (xdl_trim_ends(xdf1, xdf2) < 0 ||
> -	    xdl_cleanup_records(cf, xdf1, xdf2) < 0) {
> -
> -		return -1;
> -	}
> -
> -	return 0;
> -}
> -
>   int xdl_prepare_env(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
>   		    xdfenv_t *xe) {
>   	xdlclassifier_t cf;
> @@ -404,9 +393,10 @@ int xdl_prepare_env(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
>   		xdl_classify_record(2, &cf, rec);
>   	}
>   > +	xdl_trim_ends(&xe->xdf1, &xe->xdf2);

It would be clear that this was safe if you changed the function signature to return void as the way it is called in xdl_optimize_ctxs() makes it look like it can return an error.

Thanks

Phillip

>   	if ((XDF_DIFF_ALG(xpp->flags) != XDF_PATIENCE_DIFF) &&
>   	    (XDF_DIFF_ALG(xpp->flags) != XDF_HISTOGRAM_DIFF) &&
> -	    xdl_optimize_ctxs(&cf, &xe->xdf1, &xe->xdf2) < 0) {
> +	    xdl_cleanup_records(&cf, &xe->xdf1, &xe->xdf2) < 0) {
>   >   		xdl_free_ctx(&xe->xdf2);
>   		xdl_free_ctx(&xe->xdf1);

On 20/01/2026 15:02, Phillip Wood wrote:
> On 02/01/2026 18:52, Ezekiel Newren via GitGitGadget wrote:
>> From: Ezekiel Newren <ezekielnewren@gmail.com>
>>
>> The patience diff is set up the exact same way as histogram, see
>> xdl_do_historgram_diff() in xhistogram.c. xdl_optimize_ctxs() is
>> redundant now, delete it.
> > Does this change the output? The patience diff looks for unique context > lines and builds the context out from those. For files that look like
> > Old    New
> A    A
> B    B
> C    A
> B    B
> A    C
>      B
>      A
> > That will give a hunk
> > @@ -1,3 +0,5 @@
> +A
> +B
>   A
>   B
>   C
> > but trimming the common prefix first would give
> > @@ -1,5 +1,7
>   A
>   B
> +A
> +B
>   C
>   B
>   A
> > Though it seems like the diff silder causes us to output the same diff > in both cases for that simple test so maybe it is not an issue.

It does change larger diffs. If you run

git show --diff-algorithm=patience --diff-merges=first-parent f406b89552

You get a different diff with this series applied.

Thanks

Phillip

> It would > certainly be helpful to comment on any possible changes in the commit > message as it could have been a deliberate choice not to trim the ends > for those algorithms.
> >> -static int xdl_optimize_ctxs(xdlclassifier_t *cf, xdfile_t *xdf1, >> xdfile_t *xdf2) {
>> -
>> -    if (xdl_trim_ends(xdf1, xdf2) < 0 ||
>> -        xdl_cleanup_records(cf, xdf1, xdf2) < 0) {
>> -
>> -        return -1;
>> -    }
>> -
>> -    return 0;
>> -}
>> -
>>   int xdl_prepare_env(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
>>               xdfenv_t *xe) {
>>       xdlclassifier_t cf;
>> @@ -404,9 +393,10 @@ int xdl_prepare_env(mmfile_t *mf1, mmfile_t *mf2, >> xpparam_t const *xpp,
>>           xdl_classify_record(2, &cf, rec);
>>       }
>> +    xdl_trim_ends(&xe->xdf1, &xe->xdf2);
> > It would be clear that this was safe if you changed the function > signature to return void as the way it is called in xdl_optimize_ctxs() > makes it look like it can return an error.
> > Thanks
> > Phillip
> >>       if ((XDF_DIFF_ALG(xpp->flags) != XDF_PATIENCE_DIFF) &&
>>           (XDF_DIFF_ALG(xpp->flags) != XDF_HISTOGRAM_DIFF) &&
>> -        xdl_optimize_ctxs(&cf, &xe->xdf1, &xe->xdf2) < 0) {
>> +        xdl_cleanup_records(&cf, &xe->xdf1, &xe->xdf2) < 0) {
>>           xdl_free_ctx(&xe->xdf2);
>>           xdl_free_ctx(&xe->xdf1);
> > 

On 02/01/2026 18:52, Ezekiel Newren via GitGitGadget wrote:
> From: Ezekiel Newren <ezekielnewren@gmail.com>
> > View with --color-words. Prepare these functions to use the fields:
> delta_start, delta_end. A future patch will add these fields to
> xdfenv_t.

I'm afraid this message doesn't make much sense to me. What are these new fields? I think it would help to explain what this up comming change is going to do and why.

Oh, having read patch 7 we're removing dstart and dend from xdfile_t and replacing them with delta_start and delta_end in xdfenv_t. It would be useful to say that here.
> -static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xdf2) {
> +static int xdl_cleanup_records(xdlclassifier_t *cf, xdfenv_t *xe) {

Maybe we could add xdf1 and xdf2 as local variables to avoid having to change the code that accesses the members of xdfile_t that are not going to be moved to xdfenv_t.

Thanks

Phillip

>   	long i, nm, mlim;
>   	xrecord_t *recs;
>   	xdlclass_t *rcrec;
> @@ -273,11 +273,11 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd
>   	 * Create temporary arrays that will help us decide if
>   	 * changed[i] should remain false, or become true.
>   	 */
> -	if (!XDL_CALLOC_ARRAY(action1, xdf1->nrec + 1)) {
> +	if (!XDL_CALLOC_ARRAY(action1, xe->xdf1.nrec + 1)) {
>   		ret = -1;
>   		goto cleanup;
>   	}
> -	if (!XDL_CALLOC_ARRAY(action2, xdf2->nrec + 1)) {
> +	if (!XDL_CALLOC_ARRAY(action2, xe->xdf2.nrec + 1)) {
>   		ret = -1;
>   		goto cleanup;
>   	}
> @@ -285,17 +285,17 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd
>   	/*
>   	 * Initialize temporary arrays with DISCARD, KEEP, or INVESTIGATE.
>   	 */
> -	if ((mlim = xdl_bogosqrt((long)xdf1->nrec)) > XDL_MAX_EQLIMIT)
> +	if ((mlim = xdl_bogosqrt((long)xe->xdf1.nrec)) > XDL_MAX_EQLIMIT)
>   		mlim = XDL_MAX_EQLIMIT;
> -	for (i = xdf1->dstart, recs = &xdf1->recs[xdf1->dstart]; i <= xdf1->dend; i++, recs++) {
> +	for (i = xe->xdf1.dstart, recs = &xe->xdf1.recs[xe->xdf1.dstart]; i <= xe->xdf1.dend; i++, recs++) {
>   		rcrec = cf->rcrecs[recs->minimal_perfect_hash];
>   		nm = rcrec ? rcrec->len2 : 0;
>   		action1[i] = (nm == 0) ? DISCARD: (nm >= mlim && !need_min) ? INVESTIGATE: KEEP;
>   	}
>   > -	if ((mlim = xdl_bogosqrt((long)xdf2->nrec)) > XDL_MAX_EQLIMIT)
> +	if ((mlim = xdl_bogosqrt((long)xe->xdf2.nrec)) > XDL_MAX_EQLIMIT)
>   		mlim = XDL_MAX_EQLIMIT;
> -	for (i = xdf2->dstart, recs = &xdf2->recs[xdf2->dstart]; i <= xdf2->dend; i++, recs++) {
> +	for (i = xe->xdf2.dstart, recs = &xe->xdf2.recs[xe->xdf2.dstart]; i <= xe->xdf2.dend; i++, recs++) {
>   		rcrec = cf->rcrecs[recs->minimal_perfect_hash];
>   		nm = rcrec ? rcrec->len1 : 0;
>   		action2[i] = (nm == 0) ? DISCARD: (nm >= mlim && !need_min) ? INVESTIGATE: KEEP;
> @@ -305,27 +305,27 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd
>   	 * Use temporary arrays to decide if changed[i] should remain
>   	 * false, or become true.
>   	 */
> -	xdf1->nreff = 0;
> -	for (i = xdf1->dstart, recs = &xdf1->recs[xdf1->dstart];
> -	     i <= xdf1->dend; i++, recs++) {
> +	xe->xdf1.nreff = 0;
> +	for (i = xe->xdf1.dstart, recs = &xe->xdf1.recs[xe->xdf1.dstart];
> +	     i <= xe->xdf1.dend; i++, recs++) {
>   		if (action1[i] == KEEP ||
> -		    (action1[i] == INVESTIGATE && !xdl_clean_mmatch(action1, i, xdf1->dstart, xdf1->dend))) {
> -			xdf1->reference_index[xdf1->nreff++] = i;
> +		    (action1[i] == INVESTIGATE && !xdl_clean_mmatch(action1, i, xe->xdf1.dstart, xe->xdf1.dend))) {
> +			xe->xdf1.reference_index[xe->xdf1.nreff++] = i;
>   			/* changed[i] remains false, i.e. keep */
>   		} else
> -			xdf1->changed[i] = true;
> +			xe->xdf1.changed[i] = true;
>   			/* i.e. discard */
>   	}
>   > -	xdf2->nreff = 0;
> -	for (i = xdf2->dstart, recs = &xdf2->recs[xdf2->dstart];
> -	     i <= xdf2->dend; i++, recs++) {
> +	xe->xdf2.nreff = 0;
> +	for (i = xe->xdf2.dstart, recs = &xe->xdf2.recs[xe->xdf2.dstart];
> +	     i <= xe->xdf2.dend; i++, recs++) {
>   		if (action2[i] == KEEP ||
> -		    (action2[i] == INVESTIGATE && !xdl_clean_mmatch(action2, i, xdf2->dstart, xdf2->dend))) {
> -			xdf2->reference_index[xdf2->nreff++] = i;
> +		    (action2[i] == INVESTIGATE && !xdl_clean_mmatch(action2, i, xe->xdf2.dstart, xe->xdf2.dend))) {
> +			xe->xdf2.reference_index[xe->xdf2.nreff++] = i;
>   			/* changed[i] remains false, i.e. keep */
>   		} else
> -			xdf2->changed[i] = true;
> +			xe->xdf2.changed[i] = true;
>   			/* i.e. discard */
>   	}
>   > @@ -340,27 +340,27 @@ cleanup:
>   /*
>    * Early trim initial and terminal matching records.
>    */
> -static int xdl_trim_ends(xdfile_t *xdf1, xdfile_t *xdf2) {
> +static int xdl_trim_ends(xdfenv_t *xe) {
>   	long i, lim;
>   	xrecord_t *recs1, *recs2;
>   > -	recs1 = xdf1->recs;
> -	recs2 = xdf2->recs;
> -	for (i = 0, lim = (long)XDL_MIN(xdf1->nrec, xdf2->nrec); i < lim;
> +	recs1 = xe->xdf1.recs;
> +	recs2 = xe->xdf2.recs;
> +	for (i = 0, lim = (long)XDL_MIN(xe->xdf1.nrec, xe->xdf2.nrec); i < lim;
>   	     i++, recs1++, recs2++)
>   		if (recs1->minimal_perfect_hash != recs2->minimal_perfect_hash)
>   			break;
>   > -	xdf1->dstart = xdf2->dstart = i;
> +	xe->xdf1.dstart = xe->xdf2.dstart = i;
>   > -	recs1 = xdf1->recs + xdf1->nrec - 1;
> -	recs2 = xdf2->recs + xdf2->nrec - 1;
> +	recs1 = xe->xdf1.recs + xe->xdf1.nrec - 1;
> +	recs2 = xe->xdf2.recs + xe->xdf2.nrec - 1;
>   	for (lim -= i, i = 0; i < lim; i++, recs1--, recs2--)
>   		if (recs1->minimal_perfect_hash != recs2->minimal_perfect_hash)
>   			break;
>   > -	xdf1->dend = (long)xdf1->nrec - i - 1;
> -	xdf2->dend = (long)xdf2->nrec - i - 1;
> +	xe->xdf1.dend = (long)xe->xdf1.nrec - i - 1;
> +	xe->xdf2.dend = (long)xe->xdf2.nrec - i - 1;
>   >   	return 0;
>   }
> @@ -393,10 +393,10 @@ int xdl_prepare_env(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
>   		xdl_classify_record(2, &cf, rec);
>   	}
>   > -	xdl_trim_ends(&xe->xdf1, &xe->xdf2);
> +	xdl_trim_ends(xe);
>   	if ((XDF_DIFF_ALG(xpp->flags) != XDF_PATIENCE_DIFF) &&
>   	    (XDF_DIFF_ALG(xpp->flags) != XDF_HISTOGRAM_DIFF) &&
> -	    xdl_cleanup_records(&cf, &xe->xdf1, &xe->xdf2) < 0) {
> +	    xdl_cleanup_records(&cf, xe) < 0) {
>   >   		xdl_free_ctx(&xe->xdf2);
>   		xdl_free_ctx(&xe->xdf1);