Re: [PATCH v15 08/27] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C
flat view
From: Stephan Beyer <hidden>
Date: 2016-11-16 23:47:31
Hi, On 10/14/2016 04:14 PM, Pranit Bauva wrote:
quoted hunk ↗ jump to hunk
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
quoted hunk ↗ jump to hunk
+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?
quoted hunk ↗ jump to hunk
@@ -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