Re: [PATCH v16 Part II 1/8] bisect--helper: `bisect_reset` shell function in C

From: Stephan Beyer <s-beyer@gmx.net>
Date: Oct 30, 2017, 13:22 UTC
Message-ID: <7d978e14-2eec-23b9-c25f-e6b361101b03@gmx.net>
In-Reply-To: <0102015f5e5ee171-f30f4868-886f-47a1-a4e4-b4936afc545d-000000@eu-west-1.amazonses.com>

Hi Pranit,

On 10/27/2017 05:06 PM, Pranit Bauva wrote:

> A big thanks to Stephan and Ramsay for patiently reviewing my series and
> helping me get this merged.

You're welcome. ;)

In addition to the things mentioned by the other reviewers, only a minor thing:

> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 35d2105f941c6..12754448f7b6a 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c

[...]> @@ -106,13 +112,48 @@ static void check_expected_revs(const char **revs, int rev_nr)

Show 10 quoted lines

>  	}
>  }
>  
> +static int bisect_reset(const char *commit)
> +{
> +	struct strbuf branch = STRBUF_INIT;
> +
> +	if (!commit) {
> +		if (strbuf_read_file(&branch, git_path_bisect_start(), 0) < 1)
> +			return !printf(_("We are not bisecting.\n"));

This is weird; I had to look up the return value of printf first before I could understand what you are doing ;) I think that it is meant as a shortcut for

	printf(_("We are not bisecting.\n"));
	return 0;

but please also express it with these two lines. (Or what is the point of returning a non-zero value only in the case when nothing could be printed?)

Best, Stephan