Re: [PATCH 6/9] streaming_write_entry: propagate streaming errors

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Mar 25, 2013, 21:39 UTC
Message-ID: <20130325213934.GE1414@google.com>
In-Reply-To: <20130325202216.GF16019@sigill.intra.peff.net>

Jeff King wrote:

> When we are streaming an index blob to disk, we store the
> error from stream_blob_to_fd in the "result" variable, and
> then immediately overwrite that with the return value of
> "close".

Good catch.

[...]

Show 12 quoted lines

> --- a/entry.c
> +++ b/entry.c
> @@ -126,8 +126,10 @@ static int streaming_write_entry(struct cache_entry *ce, char *path,
>  	fd = open_output_fd(path, ce, to_tempfile);
>  	if (0 <= fd) {
>  		result = stream_blob_to_fd(fd, ce->sha1, filter, 1);
> -		*fstat_done = fstat_output(fd, state, statbuf);
> -		result = close(fd);
> +		if (!result) {
> +			*fstat_done = fstat_output(fd, state, statbuf);
> +			result = close(fd);
> +		}

Should this do something like

	{
		int fd, result = 0;

		fd = open_output_fd(path, ce, to_tempfile);
		if (fd < 0)
			return -1;

		result = stream_blob_to_fd(fd, ce->sha1, filter, 1);
		if (result)
			goto close_fd;

		*fstat_done = fstat_output(fd, state, statbuf);
	close_fd:
		result |= close(fd);
	unlink_path:
		if (result)
			unlink(path);
		return result;
	}

to avoid leaking the file descriptor?

Show 10 quoted lines

> @@ -31,4 +40,20 @@ test_expect_success 'streaming a corrupt blob fails' '
>  	)
>  '
>  
> +test_expect_success 'read-tree -u detects bit-errors in blobs' '
> +	(
> +		cd bit-error &&
> +		rm content.t &&
> +		test_must_fail git read-tree --reset -u FETCH_HEAD
> +	)

Makes sense. Might make sense to use "rm -f" instead of "rm" to avoid failures if content.t is removed already.

Show 6 quoted lines

> +'
> +
> +test_expect_success 'read-tree -u detects missing objects' '
> +	(
> +		cd missing &&
> +		rm content.t &&

Especially here.

Thanks, Jonathan