Hi,
I've only got some minors to mention here ;)
On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index c542e8b..3f19b68 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -19,9 +19,15 @@ static const char * const git_bisect_helper_usage[] = {
> N_("git bisect--helper --write-terms <bad_term> <good_term>"),
> N_("git bisect--helper --bisect-clean-state"),
> N_("git bisect--helper --bisect-reset [<commit>]"),
> + N_("git bisect--helper --bisect-write <state> <revision> <TERM_GOOD> <TERM_BAD> [<nolog>]"),
> NULL
> };
I wouldn't write "<TERM_GOOD <TERM_BAD>" in capital letters. I'd use
something like "<good_term> <bad_term>" as you have used for
--write-terms. Note that "git bisect --help" uses "<term-old>
<term-new>" in that context.
> @@ -149,6 +155,63 @@ static int check_expected_revs(const char **revs, int rev_nr)
> return 0;
> }
>
> +static int bisect_write(const char *state, const char *rev,
> + const struct bisect_terms *terms, int nolog)
> +{
> + struct strbuf tag = STRBUF_INIT;
> + struct strbuf commit_name = STRBUF_INIT;
> + struct object_id oid;
> + struct commit *commit;
> + struct pretty_print_context pp = {0};
> + FILE *fp = NULL;
> + int retval = 0;
> +
> + if (!strcmp(state, terms->term_bad))
> + strbuf_addf(&tag, "refs/bisect/%s", state);
> + else if (one_of(state, terms->term_good, "skip", NULL))
> + strbuf_addf(&tag, "refs/bisect/%s-%s", state, rev);
> + else {
> + error(_("Bad bisect_write argument: %s"), state);
> + retval = -1;
> + goto finish;
> + }
> +
> + if (get_oid(rev, &oid)) {
> + error(_("couldn't get the oid of the rev '%s'"), rev);
> + retval = -1;
> + goto finish;
> + }
> +
> + if (update_ref(NULL, tag.buf, oid.hash, NULL, 0,
> + UPDATE_REFS_MSG_ON_ERR)) {
> + retval = -1;
> + goto finish;
> + }
I'd like to mention that the "goto fail;" trick could apply in this
function, too.
> @@ -156,9 +219,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> WRITE_TERMS,
> BISECT_CLEAN_STATE,
> BISECT_RESET,
> - CHECK_EXPECTED_REVS
> + CHECK_EXPECTED_REVS,
> + BISECT_WRITE
> } cmdmode = 0;
> - int no_checkout = 0;
> + int no_checkout = 0, res = 0;
Why do you do this "direct return" -> "set res and return res" transition?
You don't need it in this patch, you do not need it in the subsequent
patches (you always set "res" exactly once after the initialization),
and you don't need cleanup code in this function.
> struct option options[] = {
> OPT_CMDMODE(0, "next-all", &cmdmode,
> N_("perform 'git bisect next'"), NEXT_ALL),
> @@ -170,10 +234,13 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> N_("reset the bisection state"), BISECT_RESET),
> OPT_CMDMODE(0, "check-expected-revs", &cmdmode,
> N_("check for expected revs"), CHECK_EXPECTED_REVS),
> + OPT_CMDMODE(0, "bisect-write", &cmdmode,
> + N_("write out the bisection state in BISECT_LOG"), BISECT_WRITE),
That info text is confusing, especially considering that there is a
"nolog" option. I think the action of bisect-write is two-fold: (1)
update the refs, (2) log.
> @@ -182,24 +249,37 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> usage_with_options(git_bisect_helper_usage, options);
>
> switch (cmdmode) {
> + int nolog;
> case NEXT_ALL:
> return bisect_next_all(prefix, no_checkout);
> case WRITE_TERMS:
> if (argc != 2)
> die(_("--write-terms requires two arguments"));
> - return write_terms(argv[0], argv[1]);
> + res = write_terms(argv[0], argv[1]);
> + break;
As indicated above, I think the direct "return ...;" is cleaner.
~Stephan