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

From: Stephan Beyer <s-beyer@gmx.net>
Date: Nov 17, 2016, 21:47 UTC
Message-ID: <c49a6151-0827-cae5-0569-8f05515be0f9@gmx.net>
In-Reply-To: <01020157c38b1b18-b81203b0-122f-4244-bfb2-9fac8ae71767-000000@eu-west-1.amazonses.com>

Hi,

On 10/14/2016 04:14 PM, Pranit Bauva wrote:

Show 16 quoted lines

> 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);

Show 8 quoted lines

> +	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