Re: [PATCH 5/6] object-file: generalize packfile writes to use odb_write_stream

[Justin Tobler <jltobler@gmail.com>]

On 26/03/31 09:48AM, Patrick Steinhardt wrote:
> On Mon, Mar 30, 2026 at 10:38:34PM -0500, Justin Tobler wrote:
> > +
> > +	if (total != size)
> > +		die("unexpected number of bytes read");
> 
> Do we want to mention the expected and actual number of bytes?

Ya, that sounds reasonable. Will update.

> > @@ -1543,6 +1542,40 @@ static void flush_packfile_transaction(struct odb_transaction_files *transaction
> >  	odb_reprepare(repo->objects);
> >  }
> >  
> > +struct read_object_fd_data {
> > +	int fd;
> > +	size_t size;
> > +	unsigned char buf[16384];
> > +};
> 
> This interface feels generally useful to me, not just in this subsystem
> here. Would it make sense to instead expose it in "odb/transaction.h"
> as a new `odb_write_stream_from_fd()` function? No need to expose the
> structure itself, I guess.

Hmmm, exposing an `odb_write_stream_from_fd()` function could probably
be useful. Would it be better for it to be put in "odb/streaming.h"
though? Maybe the its use case would always be related to transactions?

> > +static const void *read_object_fd(struct odb_write_stream *stream,
> > +				  unsigned long *len)
> > +{
> > +	struct read_object_fd_data *data = stream->data;
> > +	ssize_t read_result;
> > +	size_t rsize;
> > +
> > +	if (stream->is_finished) {
> > +		*len = 0;
> > +		return NULL;
> > +	}
> > +
> > +	rsize = data->size < sizeof(data->buf) ? data->size : sizeof(data->buf);
> > +	read_result = read_in_full(data->fd, data->buf, rsize);
> > +	if (read_result < 0)
> > +		die_errno("failed to read blob data");
> 
> It's a bit unfortunate that we die here, but we don't have an easy way
> to return errors. I wonder whether we should refactor the interface a
> bit to maybe take a pointer to a buffer as well as the buffer's length
> and then return an `ssize_t`.
> 
>     static ssize_t *read_object_fd(struct odb_write_stream *stream,
>                                    unsigned char *buf,
>                                    size_t buf_len);
> 
> That'd also avoid having to cast away the const-ness, and it allows the
> caller to control how many bytes they want to read at once.

I think the above suggestion would work. I believe there is only a
single other usage of `struct odb_write_stream` so updating shouldn't be
much churn.

> > +	if ((size_t)read_result != rsize)
> > +		die("failed to read %u bytes of blob data", (unsigned)rsize);
> > +
> > +	data->size -= rsize;
> 
> I feel like `data->size` is misleadingly named now, as it doesn't
> reflect the overall size but rather the number of remaining bytes that
> we expect.

That's fair. The variable starts off as the initial size of the object
being read, but really is just the number of unread bytes. Will update.

Thanks,
-Justin