Re: [PATCH 7/7] odb: use size_t for object_info.sizep and the size APIs

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Patrick,

On Mon, 15 Jun 2026, Patrick Steinhardt wrote:

> On Thu, Jun 04, 2026 at 10:51:12AM +0000, Johannes Schindelin via GitGitGadget wrote:
> > diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> > index fa45f774d7..fa6e396ddc 100644
> > --- a/builtin/cat-file.c
> > +++ b/builtin/cat-file.c
> > @@ -120,7 +120,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
> >  	struct object_id oid;
> >  	enum object_type type;
> >  	char *buf;
> > -	unsigned long size;
> > +	size_t size;
> >  	struct object_context obj_context = {0};
> >  	struct object_info oi = OBJECT_INFO_INIT;
> >  	unsigned flags = OBJECT_INFO_LOOKUP_REPLACE;
> > @@ -166,7 +166,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
> >  		if (use_mailmap && (type == OBJ_COMMIT || type == OBJ_TAG)) {
> >  			size_t s = size;
> >  			buf = replace_idents_using_mailmap(buf, &s);
> > -			size = cast_size_t_to_ulong(s);
> > +			size = s;
> >  		}
> >  
> >  		printf("%"PRIuMAX"\n", (uintmax_t)size);
> 
> Can't we drop this local variable completely and instead supply `&size`
> directly?

Well spotted!

> > @@ -219,7 +225,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
> >  		if (use_mailmap) {
> >  			size_t s = size;
> >  			buf = replace_idents_using_mailmap(buf, &s);
> > -			size = cast_size_t_to_ulong(s);
> > +			size = s;
> >  		}
> >  
> >  		/* otherwise just spit out the data */
> > @@ -266,7 +272,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
> >  		if (use_mailmap) {
> >  			size_t s = size;
> >  			buf = replace_idents_using_mailmap(buf, &s);
> > -			size = cast_size_t_to_ulong(s);
> > +			size = s;
> >  		}
> >  		break;
> >  	}
> > @@ -446,7 +455,7 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
> >  		if (use_mailmap) {
> >  			size_t s = size;
> >  			contents = replace_idents_using_mailmap(contents, &s);
> > -			size = cast_size_t_to_ulong(s);
> > +			size = s;
> >  		}
> >  
> >  		if (type != data->type)
> 
> Likewise for these three instances.

I totally agree.

> > @@ -555,7 +564,7 @@ static void batch_object_write(const char *obj_name,
> >  			if (!buf)
> >  				die(_("unable to read %s"), oid_to_hex(&data->oid));
> >  			buf = replace_idents_using_mailmap(buf, &s);
> > -			data->size = cast_size_t_to_ulong(s);
> > +			data->size = s;
> >  
> >  			free(buf);
> >  		}
> 
> And I think this site here can be adapted, as well.

Indeed!

> > diff --git a/diff.c b/diff.c
> > index 5a584fa1d5..816b89dc6c 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -4594,8 +4594,9 @@ int diff_populate_filespec(struct repository *r,
> >  		}
> >  	}
> >  	else {
> > +		size_t size_st = 0;
> >  		struct object_info info = {
> > -			.sizep = &s->size
> > +			.sizep = &size_st
> >  		};
> >  
> >  		if (!(size_only || check_binary))
> > @@ -4617,6 +4618,7 @@ int diff_populate_filespec(struct repository *r,
> >  			die("unable to read %s", oid_to_hex(&s->oid));
> >  
> >  object_read:
> > +		s->size = cast_size_t_to_ulong(size_st);
> >  		if (size_only || check_binary) {
> >  			if (size_only)
> >  				return 0;
> > @@ -4631,6 +4633,7 @@ object_read:
> >  			if (odb_read_object_info_extended(r->objects, &s->oid, &info,
> >  							  OBJECT_INFO_LOOKUP_REPLACE))
> >  				die("unable to read %s", oid_to_hex(&s->oid));
> > +			s->size = cast_size_t_to_ulong(size_st);
> >  		}
> >  		s->should_free = 1;
> >  	}
> 
> The flow in this function is quite weird if you ask me, but that's a
> preexisting issue. This does look correct to me, even if it's awkward.

Yes, on all four accounts.

Ciao,
Johannes