Re: [PATCH v15 22/27] bisect--helper: `bisect_log` shell function in C

[Stephan Beyer <s-beyer@xxxxxxx>]

Hi,

On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 493034c..c18ca07 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -858,6 +858,23 @@ static int bisect_state(struct bisect_terms *terms, const char **argv,
>  	return -1;
>  }
>  
> +static int bisect_log(void)
> +{
> +	int fd, status;
> +	fd = open(git_path_bisect_log(), O_RDONLY);
> +	if (fd < 0)
> +		return -1;
> +
> +	status = copy_fd(fd, 1);

Perhaps

	status = copy_fd(fd, STDOUT_FILENO);

> +	if (status) {
> +		close(fd);
> +		return -1;
> +	}
> +
> +	close(fd);
> +	return status;
> +}

That's weird.
Either get rid of the if() and actually use status:

	status = copy_fd(fd, STDOUT_FILENO);

	close(fd);
	return status ? -1 : 0;

or get rid of status and use the if:

	if (copy_fd(fd, STDOUT_FILENO)) {
		close(fd);
		return -1;
	}

	close(fd);
	return 0;

I'd recommend the shorter variant ;)

~Stephan