On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@xxxxxxxxx> wrote: > 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 <pranit.bauva@xxxxxxxxx> > --- > 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html