Re: [PATCH 3/6] object-file: remove flags from transaction packfile writes

[Justin Tobler <jltobler@gmail.com>]

On 26/03/31 09:48AM, Patrick Steinhardt wrote:
> On Mon, Mar 30, 2026 at 10:38:32PM -0500, Justin Tobler wrote:
> > diff --git a/object-file.c b/object-file.c
> > index bfbb632cf8..493173eaf4 100644
> > --- a/object-file.c
> > +++ b/object-file.c
> > @@ -1405,6 +1404,34 @@ static void prepare_packfile_transaction(struct odb_transaction_files *transacti
> >  		die_errno("unable to write pack header");
> >  }
> >  
> > +static int hash_blob_stream(const struct git_hash_algo *hash_algo,
> > +			    struct object_id *result_oid, int fd, size_t size)
> 
> We could of course change the interface while at it to also receive the
> object type. I'll leave it to you though whether you want to go there,
> we don't have any use case for it right now anyway.

Hmmm, this particular helper is always reads the object from an fd. We
could of course also update this interface to use a `struct
odb_writes_stream` to provide the object instead. For now though, since
we don't have a use case, I may just leave it as-is.

> > +{
> > +	unsigned char buf[16384];
> > +	struct git_hash_ctx ctx;
> > +	unsigned header_len;
> > +
> > +	header_len = format_object_header((char *)buf, sizeof(buf),
> > +					  OBJ_BLOB, size);
> > +	hash_algo->init_fn(&ctx);
> > +	git_hash_update(&ctx, buf, header_len);
> > +
> > +	while (size) {
> > +		size_t rsize = size < sizeof(buf) ? size : sizeof(buf);
> > +		ssize_t read_result = read_in_full(fd, buf, rsize);
> > +
> > +		if ((size_t)read_result != rsize)
> > +			return -1;
> 
> It would be a bit cleaner to first check whether `read_result < 0`
> before casting.
> 
>     if (read_result < 0 || (size_t) read_result != rsize)
>         return -1;
> 
> Doesn't make a difference in practice though.

That's fair, I can update in the next version to this.

> > +		git_hash_update(&ctx, buf, rsize);
> > +		size -= read_result;
> > +	}
> > +
> > +	git_hash_final_oid(result_oid, &ctx);
> > +
> > +	return 0;
> > +}
> 
> Overall, this function really is simple enough to pull out, even if it
> duplicates a tiny amount of logic. Also, it has the benefit that we can
> easily skip deflating the data, which we used to do even if we didn't
> ultimately write the data to disk, so it was just pointless busywork.
> 
> > @@ -1642,7 +1655,7 @@ int index_fd(struct index_state *istate, struct object_id *oid,
> >  	     int fd, struct stat *st,
> >  	     enum object_type type, const char *path, unsigned flags)
> >  {
> > -	int ret;
> > +	int ret = 0;
> >  
> >  	/*
> >  	 * Call xsize_t() only when needed to avoid potentially unnecessary
> 
> In practice this doesn't have to be zero-initialized

Ah, yes. An earlier iteration of this didn't have `hash_blob_stream()`
returning error codes. Will update accordingly. Thanks.

> > @@ -1659,18 +1672,23 @@ int index_fd(struct index_state *istate, struct object_id *oid,
> >  		ret = index_core(istate, oid, fd, xsize_t(st->st_size),
> >  				 type, path, flags);
> >  	} else {
> > -		struct object_database *odb = the_repository->objects;
> > -		struct odb_transaction_files *files_transaction;
> > -		struct odb_transaction *transaction;
> > -
> > -		transaction = odb_transaction_begin(odb);
> > -		files_transaction = container_of(odb->transaction,
> > -						 struct odb_transaction_files,
> > -						 base);
> > -		ret = index_blob_packfile_transaction(files_transaction, oid, fd,
> > -						      xsize_t(st->st_size),
> > -						      path, flags);
> > -		odb_transaction_commit(transaction);
> > +		if (flags & INDEX_WRITE_OBJECT) {
> > +			struct object_database *odb = the_repository->objects;
> > +			struct odb_transaction_files *files_transaction;
> > +			struct odb_transaction *transaction;
> > +
> > +			transaction = odb_transaction_begin(odb);
> > +			files_transaction = container_of(odb->transaction,
> > +							 struct odb_transaction_files,
> > +							 base);
> > +			ret = index_blob_packfile_transaction(files_transaction, oid, fd,
> > +						      xsize_t(st->st_size), path);
> > +			odb_transaction_commit(transaction);
> 
> Okay. It's a bit sad that we have to reach into the files backend here,
> but we already did beforehand, and maybe a subsequent commit will fix
> this? Reading on.

Yes. :)

> On another note, it's somewhat curious that the commit doesn't return an
> error code. Probably something we should fix eventually.

Ya, the error handling for the ODB transaction functions really should
bubble up any errors instead of just "die()"ing. I plan to improve this
in a future series.

Thanks,
-Justin