Re: [RFC/PATCH v11 11/13] bisect--helper: `bisect_next_check` shell function in C

[Pranit Bauva <pranit.bauva@gmail.com>]

Hey Junio,

On Wed, Aug 3, 2016 at 12:47 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Pranit Bauva <pranit.bauva@gmail.com> writes:
>
>> +static int mark_good(const char *refname, const struct object_id *oid,
>> +                  int flag, void *cb_data)
>> +{
>> +     int *m_good = (int *)cb_data;
>> +     *m_good = 0;
>> +     return 0;
>> +}
>
> See below.
>
>> +static int bisect_next_check(const struct bisect_terms *terms,
>> +                          const char *current_term)
>> +{
>> +     int missing_good = 1, missing_bad = 1;
>
> It is somewhat unusual to start with "assume we are OK" and then
> "it turns out that we are not".
>
>> +     char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad.buf);
>> +     char *good_glob = xstrfmt("%s*", terms->term_good.buf);
>
> The original runs
>
>     git for-each-ref "refs/bisect/$TERM_GOOD-*
>
> but this one lacks the final dash.

My bad. Will include it.

>> +     if (ref_exists(bad_ref))
>> +             missing_bad = 0;
>> +     free(bad_ref);
>> +
>> +     for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/",
>> +                          (void *) &missing_good);
>> +     free(good_glob);
>
> The for-each helper does not return until it iterates over all the
> matching refs, but you are only interested in seeing if at least one
> exists.  It may make sense to return 1 from mark_good() to terminate
> the traversal early.

Seems a better way. Thanks!

>> +     if (!missing_good && !missing_bad)
>> +             return 0;
>> +
>> +     if (!current_term)
>> +             return -1;
>> +
>> +     if (missing_good && !missing_bad && current_term &&
>> +         !strcmp(current_term, terms->term_good.buf)) {
>> +             char *yesno;
>> +             /*
>> +              * have bad (or new) but not good (or old). We could bisect
>> +              * although this is less optimum.
>> +              */
>> +             fprintf(stderr, "Warning: bisecting only with a %s commit\n",
>> +                     terms->term_bad.buf);
>
> In the original, this message goes through gettext.

Will do.

>> +             if (!isatty(0))
>> +                     return 0;
>> +             /*
>> +              * TRANSLATORS: Make sure to include [Y] and [n] in your
>> +              * translation. The program will only accept English input
>> +              * at this point.
>> +              */
>> +             yesno = git_prompt(_("Are you sure [Y/n]? "), PROMPT_ECHO);
>> +             if (starts_with(yesno, "N") || starts_with(yesno, "n"))
>> +                     return -1;
>> +             return 0;
>> +     }
>
> When the control falls into the above if(){} block, the function
> will always return.  It will clarify that this is the end of such a
> logical block to have a blank line here.

Will do.

>> +     if (!is_empty_or_missing_file(git_path_bisect_start()))
>> +             return error(_("You need to give me at least one good|old and "
>> +                             "bad|new revision. You can use \"git bisect "
>> +                             "bad|new\" and \"git bisect good|old\" for "
>> +                             "that. \n"));
>> +     else
>> +             return error(_("You need to start by \"git bisect start\". "
>> +                             "You then need to give me at least one good|"
>> +                             "old and bad|new revision. You can use \"git "
>> +                             "bisect bad|new\" and \"git bisect good|old\" "
>> +                             " for that.\n"));
>
> The i18n on these two messages seem to be different from the
> original, which asks bisect_voc to learn what 'bad' and 'good' are
> called and attempts to use these words from the vocabulary.

I have little idea about i18n. Will look more into it.

Regards,
Pranit Bauva