Re: [PATCH v2 4/7] object-file: remove flags from transaction packfile writes

[Justin Tobler <jltobler@gmail.com>]

On 26/04/01 01:23PM, Patrick Steinhardt wrote:
> On Tue, Mar 31, 2026 at 10:03:12PM -0500, Justin Tobler wrote:
> > diff --git a/object-file.c b/object-file.c
> > index f3038756fc..f317a24ccf 100644
> > --- a/object-file.c
> > +++ b/object-file.c
> > @@ -1412,6 +1411,38 @@ static void prepare_packfile_transaction(struct odb_transaction_files *transacti
> >  		die_errno("unable to write pack header");
> >  }
> >  
> > +static int hash_blob_stream(struct odb_write_stream *stream,
> > +			    const struct git_hash_algo *hash_algo,
> > +			    struct object_id *result_oid, size_t size)
> > +{
> > +	unsigned char buf[16384];
> > +	struct git_hash_ctx ctx;
> > +	unsigned header_len;
> > +	size_t total = 0;
> 
> One nit: I think `total` and `size` don't really give a good sense of
> which variable tracks what. If this was instead `bytes_hashed` and
> `size` it would become a lot more obvious.

That's fair. I'll update the names in the next version.

> > @@ -1666,18 +1683,28 @@ 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);
> > +		struct odb_write_stream stream = { 0 };
> > +		odb_write_stream_from_fd(&stream, fd, xsize_t(st->st_size));
> 
> I would assume that `odb_write_stream_from_fd()` knows to fully
> initialize the stream, so zero-initializing shouldn't be necessary,
> right?

Currently the `is_finished` field is not being initialized, but there
isn't really any reason we couldn't do that in
`odb_write_stream_from_fd()` though. Will update accordingly.

> > diff --git a/odb/streaming.h b/odb/streaming.h
> > index c7861f7e13..e5232cd4d1 100644
> > --- a/odb/streaming.h
> > +++ b/odb/streaming.h
> > @@ -5,6 +5,7 @@
> >  #define STREAMING_H 1
> >  
> >  #include "object.h"
> > +#include "odb.h"
> >  
> >  struct object_database;
> >  struct odb_read_stream;
> > @@ -64,4 +65,11 @@ int odb_stream_blob_to_fd(struct object_database *odb,
> >  			  struct stream_filter *filter,
> >  			  int can_seek);
> >  
> > +/*
> > + * Sets up an ODB write stream that reads from an fd. The caller is expected to
> > + * free the underlying stream data.
> > + */
> 
> Hm. Shouldn't we provide an interface that let's the caller do this
> without having to know about the stream's internals, like
> `odb_write_stream_release()`?

I thought about doint this, but hesitated because the other `struct
odb_write_stream` usage sets up its `data` field differently and
consequently would have no need for a `odb_write_stream_release()`. I'm
probably overthinking this though and it probably makes sense just to
add it.

Thanks,
-Justin