Re: [PATCH 2/7] odb/streaming: drop `is_finished` field

On 26/08/04 09:25AM, Patrick Steinhardt wrote:
> The `is_finished` field is used to track whether a write stream is done
> writing all of its data. Tracking this field as part of the stream
> itself shouldn't be required though: callers will already know when the
> stream is done when the stream's read function returns zero bytes, same
> as when reading from a file descriptor.
> 
> There is one exception where it gets a bit more complicated: when
> consuming data in "builtin/unpack-objects.c" it may happen that we don't
> yield any new bytes after reading from the pipe. This is addressed by
> looping until we have produced at least a single byte of output.

Addressing this one outlier sounds reasonable.

> Drop the field from `struct odb_write_stream`. Again, same as in the
> preceding commit, this brings the structure a bit closer to its sibling
> `struct odb_read_stream`.

This also makes the overal interface a bit simpler. Callers can trust
that when `odb_write_stream_read()` returns zero, it is actually
finished without having to inspect further.

> Signed-off-by: Patrick Steinhardt <[email protected]>
> ---
>  builtin/unpack-objects.c      | 15 ++++++++-------
>  object-file.c                 | 13 ++++++++-----
>  odb/source-inmemory.c         |  9 ++++++++-
>  odb/source-loose.c            | 12 ++++++++----
>  odb/streaming.c               |  5 +----
>  odb/streaming.h               |  1 -
>  t/unit-tests/u-odb-inmemory.c |  5 +++--
>  7 files changed, 36 insertions(+), 24 deletions(-)
> 
> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
> index f3e0b504f4..b7c486ea94 100644
> --- a/builtin/unpack-objects.c
> +++ b/builtin/unpack-objects.c
> @@ -368,20 +368,20 @@ static ssize_t feed_input_zstream(struct odb_write_stream *in_stream,
>  {
>  	struct input_zstream_data *data = in_stream->data;
>  	git_zstream *zstream = data->zstream;
> -	void *in = fill(1);
>  
> -	if (in_stream->is_finished)
> +	if (data->status != Z_OK)
>  		return 0;
>  
>  	zstream->next_out = buf;
>  	zstream->avail_out = buf_len;
> -	zstream->next_in = in;
> -	zstream->avail_in = len;
>  
> -	data->status = git_inflate(zstream, 0);
> +	while (data->status == Z_OK && zstream->avail_out == buf_len) {
> +		zstream->next_in = fill(1);
> +		zstream->avail_in = len;
> +		data->status = git_inflate(zstream, 0);
> +		use(len - zstream->avail_in);
> +	}

Ok, now we call `git_inflate()` in a loop until there is an error or we
get some data back. This makes it so we can trust that returning zero
does mean that the stream is finished. Previously, it was the callers
responsibility to check the `is_finished` stream field to be certain.

I was curious if we needed to update any code documentation with this
change, but it looks like the comments for `odb_write_stream_read()`
already made it sound like this was the current behavior.

[snip]
> diff --git a/odb/streaming.h b/odb/streaming.h
> index 4d7d31b5aa..5e8e6e532e 100644
> --- a/odb/streaming.h
> +++ b/odb/streaming.h
> @@ -56,7 +56,6 @@ struct odb_write_stream {
>  	ssize_t (*read)(struct odb_write_stream *, unsigned char *, size_t);
>  	void *data;
>  	size_t size;
> -	int is_finished;

The field is dropped. Nice.

The rest of this patch looks good.

-Justin