Re: [PATCH v2 5/6] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C
From: Eric Sunshine <hidden>
Date: 2016-06-16 06:55:53
On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva [off-list ref] wrote:
quoted hunk ↗ jump to hunk
Reimplement `is_expected_rev` & `check_expected_revs` shell function in C and add a `--check-expected-revs` subcommand to `git bisect--helper` to call it from git-bisect.sh . [...] Signed-off-by: Pranit Bauva <redacted> ---diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c@@ -162,13 +162,44 @@ static int bisect_reset(const char *commit) +static int is_expected_rev(const char *expected_hex) +{ + struct strbuf actual_hex = STRBUF_INIT; + int res; + + if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) < 0) { + strbuf_release(&actual_hex); + return 0; + } + + strbuf_trim(&actual_hex); + res = !strcmp(actual_hex.buf, expected_hex); + strbuf_release(&actual_hex); + return res; +}
Not worth a re-roll, but this could be re-structured to avoid having
to remember to release the strbuf at all exits:
struct strbuf actual_hex = ...;
int res = 0;
if (strbuf_read_file(...) >= 0) {
strbuf_trim(...);
res = !strcmp(...);
}
strbuf_release(...);
return res;
Alternately:
if (strbuf_read_file(...) < 0)
goto done;
strbuf_trim(...);
res = !strcmp(...);
done:
strbuf_release(...);
return res;
which is a bit less compact.
+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])) {
+ remove_path(git_path_bisect_ancestors_ok());
+ remove_path(git_path_bisect_expected_rev());
+ return 0;
+ }
+ }
+ return 0;
+}Hmm, all execution paths return 0, so it feels a bit pointless to have this function return a value at all. You could also use a 'break' inside the loop rather than 'return' since the return value is the same inside or outside the loop and nothing else happens after the loop.