Re: [PATCH v15 08/27] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C

From: Stephan Beyer <s-beyer@gmx.net>
Date: Nov 16, 2016, 23:47 UTC
Message-ID: <a4c7fec8-0e84-eb53-ca22-c369ce3facfa@gmx.net>
In-Reply-To: <01020157c38b1ab6-bda8420e-9a63-47d7-9b99-47465b6333d9-000000@eu-west-1.amazonses.com>

Hi,

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

Show 19 quoted lines

> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index d84ba86..c542e8b 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -123,13 +123,40 @@ static int bisect_reset(const char *commit)
>  	return bisect_clean_state();
>  }
>  
> +static int is_expected_rev(const char *expected_hex)
> +{
> +	struct strbuf actual_hex = STRBUF_INIT;
> +	int res = 0;
> +	if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) >= 40) {
> +		strbuf_trim(&actual_hex);
> +		res = !strcmp(actual_hex.buf, expected_hex);
> +	}
> +	strbuf_release(&actual_hex);
> +	return res;
> +}

I am not sure it does what it should.

I would expect the following behavior from this function:
 - file does not exist (or is "broken") => return 0
 - actual_hex != expected_hex => return 0
 - otherwise return 1

If I am not wrong, the code does the following instead:
 - file does not exist (or is "broken") => return 0
 - actual_hex != expected_hex => return 1
 - otherwise => return 0

Show 13 quoted lines

> +static int check_expected_revs(const char **revs, int rev_nr)
> +{
> +	int i;
> +
> +	for (i = 0; i < rev_nr; i++) {
> +		if (!is_expected_rev(revs[i])) {
> +			unlink_or_warn(git_path_bisect_ancestors_ok());
> +			unlink_or_warn(git_path_bisect_expected_rev());
> +			return 0;
> +		}
> +	}
> +	return 0;
> +}

Here I am not sure what the function *should* do. However, I see that it basically mimics the behavior of the shell function (assuming is_expected_rev() is implemented correctly).

I don't understand why the return value is int and not void. To avoid a "return 0;" line when calling this function?

Show 6 quoted lines

> @@ -167,6 +196,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  		if (argc > 1)
>  			die(_("--bisect-reset requires either zero or one arguments"));
>  		return bisect_reset(argc ? argv[0] : NULL);
> +	case CHECK_EXPECTED_REVS:
> +		return check_expected_revs(argv, argc);

I note that you check the correct number of arguments for some subcommands and you do not check it for some other subcommands like this one. (I don't care, I just want to mention it.)

>  	default:
>  		die("BUG: unknown subcommand '%d'", cmdmode);
>  	}

~Stephan