Re: [PATCH 5/7] odb/streaming: consolidate read and write streams

On 26/08/04 09:25AM, Patrick Steinhardt wrote:
> The `struct odb_read_stream` and `struct odb_write_stream` both provide
> the same functionality: they allow a caller to read object data from an
> arbitrary source. Historically, the only difference was that the read
> stream was used to read data out of the object database, whereas the
> write stream was used to write data into the object database, but the
> interfaces were mostly the same.

Ok.

> Over the preceding commits we have refactored the write stream to have
> almost exactly the same interface as the read stream. With these
> refactorings we can now easily merge those two streams into a single
> interface that's used for both use cases.

Nice.

> While most of the changes are mechanical, there are two sites that need
> special mention:
> 
>   - "builtin/unpack-objects.c" creates a write stream from compressed
>     object data.
> 
>   - "odb/streaming.c" creates a write stream from a file descriptor.
> 
> Adapting these sites to yield the new stream type requires a couple more
> changes. Most importantly, instead of embedding the pointer to the data
> in `struct odb_write_stream`, we now allocate a structure that wraps the
> new `struct odb_stream` base. Other than that though, the changes are
> rather straight forward.

Ok, creating wrapper stream types for these sounds reasonable.

> 
> Signed-off-by: Patrick Steinhardt <[email protected]>
> ---
>  builtin/unpack-objects.c      | 31 ++++++++++++++++---------------
>  object-file.c                 | 25 ++++++++++++-------------
>  odb.c                         |  2 +-
>  odb.h                         |  4 ++--
>  odb/source-files.c            |  2 +-
>  odb/source-inmemory.c         |  4 ++--
>  odb/source-loose.c            |  6 +++---
>  odb/source-packed.c           |  2 +-
>  odb/source.h                  |  4 ++--
>  odb/streaming.c               | 35 ++++++++++++++++-------------------
>  odb/streaming.h               | 31 +++----------------------------
>  odb/transaction.c             |  2 +-
>  odb/transaction.h             |  4 ++--
>  t/unit-tests/u-odb-inmemory.c |  6 +++---
>  14 files changed, 65 insertions(+), 93 deletions(-)
> 
> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
> index 7439ec53be..05a2d48011 100644
> --- a/builtin/unpack-objects.c
> +++ b/builtin/unpack-objects.c
> @@ -359,20 +359,21 @@ static void unpack_non_delta_entry(enum object_type type, unsigned long size,
>  }
>  
>  struct input_zstream_data {
> +	struct odb_stream base;
>  	git_zstream *zstream;
>  	int status;
>  };

Ok, as mentioned in the commit message, we now embed the stream instead
storing a pointer to the extra data. Should we also update the struct
name here now that `input_zstream_data` is really itself a stream?

> -static ssize_t feed_input_zstream(struct odb_write_stream *in_stream,
> -				  unsigned char *buf, size_t buf_len)
> +static ssize_t feed_input_zstream(struct odb_stream *in_stream,
> +				  char *buf, size_t buf_len)
>  {
> -	struct input_zstream_data *data = in_stream->data;
> +	struct input_zstream_data *data = container_of(in_stream, struct input_zstream_data, base);

Callback is updated to fetch data from the base stream.

>  	git_zstream *zstream = data->zstream;
>  
>  	if (data->status != Z_OK)
>  		return 0;
>  
> -	zstream->next_out = buf;
> +	zstream->next_out = (unsigned char *) buf;
>  	zstream->avail_out = buf_len;
>  
>  	while (data->status == Z_OK && zstream->avail_out == buf_len) {
> @@ -388,24 +389,24 @@ static ssize_t feed_input_zstream(struct odb_write_stream *in_stream,
>  static void stream_blob(unsigned long size, unsigned nr)
>  {
>  	git_zstream zstream = { 0 };
> -	struct input_zstream_data data = { 0 };
> -	struct odb_write_stream in_stream = {
> -		.read = feed_input_zstream,
> -		.data = &data,
> -		.size = size,
> -		.type = OBJ_BLOB,
> +	struct input_zstream_data in_stream = {
> +		.base = {
> +			.read = feed_input_zstream,
> +			.size = size,
> +			.type = OBJ_BLOB,
> +		},
> +		.zstream = &zstream,
> +		.status = Z_OK,
>  	};
>  	struct obj_info *info = &obj_list[nr];
>  
> -	data.zstream = &zstream;
> -	data.status = Z_OK;
>  	git_inflate_init(&zstream);
>  
> -	if (odb_write_object_stream(the_repository->objects, &in_stream, &info->oid))
> +	if (odb_write_object_stream(the_repository->objects, &in_stream.base, &info->oid))
>  		die(_("failed to write object in stream"));
>  
> -	if (data.status != Z_STREAM_END)
> -		die(_("inflate returned (%d)"), data.status);
> +	if (in_stream.status != Z_STREAM_END)
> +		die(_("inflate returned (%d)"), in_stream.status);
>  	git_inflate_end(&zstream);
>  
>  	if (strict) {

Stream set up is now updated to use the wrapper stream. Looks good.

[snip]
> @@ -299,14 +289,15 @@ int odb_stream_blob_to_fd(struct object_database *odb,
>  }
>  
>  struct read_object_fd_data {
> +	struct odb_stream base;
>  	int fd;
>  	size_t remaining;
>  };

`read_object_fd_data` is also now set up as a wrapper stream. Should we
also rename it accordingly?

> -static ssize_t read_object_fd(struct odb_write_stream *stream,
> -			      unsigned char *buf, size_t len)
> +static ssize_t read_object_fd(struct odb_stream *stream,
> +			      char *buf, size_t len)
>  {
> -	struct read_object_fd_data *data = stream->data;
> +	struct read_object_fd_data *data = container_of(stream, struct read_object_fd_data, base);
>  	ssize_t read_result;
>  	size_t count;
>  
> @@ -323,17 +314,23 @@ static ssize_t read_object_fd(struct odb_write_stream *stream,
>  	return read_result;
>  }
>  
> -void odb_write_stream_from_fd(struct odb_write_stream *stream, int fd,
> -			      size_t size, enum object_type type)
> +static int close_object_fd(struct odb_stream *stream UNUSED)
> +{
> +	/* The file descriptor is owned by the caller for now. */
> +	return 0;
> +}
> +
> +struct odb_stream *odb_write_stream_from_fd(int fd, size_t size, enum object_type type)

Should we also update the name of this function?

The rest of this patch is just renames and call site updates to
consolidate the two stream types. Looks good.

-Justin