Hey Stephan,
On Thu, Nov 17, 2016 at 3:10 PM, Stephan Beyer <s-beyer@xxxxxxx> wrote:
> 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.
Keeping it in small does give less strain to the eye ;)
>> @@ -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.
Sure!
>> @@ -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.
Initially I was using strbuf but then I switched to const char *
according to Junio's suggestion. It seems that in this version I have
forgot to free the terms.
>> 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.
I agree that it is confusing. I couldn't find a better way to describe
it and since this would be gone after the whole conversion, I didn't
bother putting more efforts there.
Regards,
Pranit Bauva