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

[Patrick Steinhardt <ps@pks.im>]

On Mon, Mar 30, 2026 at 10:38:34PM -0500, Justin Tobler wrote:
> diff --git a/object-file.c b/object-file.c
> index 1de2244ac5..4c797d6498 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1453,24 +1453,19 @@ static void stream_blob_to_pack(struct transaction_packfile *state,
>  	s.avail_out = sizeof(obuf) - hdrlen;
>  
>  	while (status != Z_STREAM_END) {
> -		if (size && !s.avail_in) {
> -			size_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);
> -			ssize_t read_result = read_in_full(fd, ibuf, rsize);
> -			if (read_result < 0)
> -				die_errno("failed to read from '%s'", path);
> -			if ((size_t)read_result != rsize)
> -				die("failed to read %u bytes from '%s'",
> -				    (unsigned)rsize, path);
> +		if (!stream->is_finished && !s.avail_in) {
> +			unsigned long rsize;
> +			unsigned const char *buf = stream->read(stream, &rsize);
>  
>  			if (rsize)
> -				git_hash_update(ctx, ibuf, rsize);
> +				git_hash_update(ctx, buf, rsize);
>  
> -			s.next_in = ibuf;
> +			s.next_in = (unsigned char *)buf;

A bit ugly that we have to cast away the constness, but oh, well.

> @@ -1490,6 +1485,10 @@ static void stream_blob_to_pack(struct transaction_packfile *state,
>  			die("unexpected deflate failure: %d", status);
>  		}
>  	}
> +
> +	if (total != size)
> +		die("unexpected number of bytes read");

Do we want to mention the expected and actual number of bytes?

> @@ -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.

> +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.

> +	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.

Patrick