Re: [PATCH v15 09/27] bisect--helper: `bisect_write` shell function in C

> 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.