Hi,
On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> 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
> +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?
> @@ -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