From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 18 Feb 2014 23:56:21 GMT
Subject: Re: [PATCH 5/5] streaming: simplify attaching a filter
Message-ID: <xmqqd2ikasbe.fsf@gitster.dls.corp.google.com>
In-Reply-To: <0120f99a7cccc118782cda63f478d7d3f528ee3f.1392565571.git.john@keeping.me.uk>

John Keeping <john@keeping.me.uk> writes:

> We are guaranteed that 'nst' is non-null because it is allocated with
> xmalloc(), and in fact we rely on this three lines later by
> unconditionally dereferencing it.

The intent of the original code is for attach_stream_filter() to
detect an error condition and return NULL, in which case it closes
the istream it allocated and signal error to the caller, I think,
and falling thru to use st->anything and return st when that happens
is *not* a guarantee that a-s-f will not detect an error ever, but
rather is a bug in the error codepath.

>
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
>  streaming.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/streaming.c b/streaming.c
> index d7c9f32..8a7135d 100644
> --- a/streaming.c
> +++ b/streaming.c
> @@ -151,10 +151,7 @@ struct git_istream *open_istream(const unsigned char *sha1,
>  	}
>  	if (filter) {
>  		/* Add "&& !is_null_stream_filter(filter)" for performance */
> -		struct git_istream *nst = attach_stream_filter(st, filter);
> -		if (!nst)
> -			close_istream(st);
> -		st = nst;
> +		st = attach_stream_filter(st, filter);
>  	}
>  
>  	*size = st->size;