Re: [PATCH v15 11/27] bisect--helper: `bisect_next_check` & bisect_voc shell function in C

>
> Hi Pranit,
>
> On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> > Also reimplement `bisect_voc` shell function in C and call it from
> > `bisect_next_check` implementation in C.
>
> Please don't! ;D
>
> > +static char *bisect_voc(char *revision_type)
> > +{
> > +     if (!strcmp(revision_type, "bad"))
> > +             return "bad|new";
> > +     if (!strcmp(revision_type, "good"))
> > +             return "good|old";
> > +
> > +     return NULL;
> > +}
>
> Why not simply use something like this:
>
> static const char *voc[] = {
>         "bad|new",
>         "good|old",
> };
>
> Then...

> > +static int bisect_next_check(const struct bisect_terms *terms,
> > +                          const char *current_term)
> > +{
> > +     int missing_good = 1, missing_bad = 1, retval = 0;
> > +     char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
> > +     char *good_glob = xstrfmt("%s-*", terms->term_good);
> > +     char *bad_syn, *good_syn;
>
> ...you don't need bad_syn and good_syn...
>
> > +     bad_syn = xstrdup(bisect_voc("bad"));
> > +     good_syn = xstrdup(bisect_voc("good"));
>
> ...and hence not these two lines...
>
> > +     if (!is_empty_or_missing_file(git_path_bisect_start())) {
> > +             error(_("You need to give me at least one %s and "
> > +                     "%s revision. You can use \"git bisect %s\" "
> > +                     "and \"git bisect %s\" for that. \n"),
> > +                     bad_syn, good_syn, bad_syn, good_syn);
>
> ...and write
>                         voc[0], voc[1], voc[0], voc[1]);
> instead...
>
> > +             retval = -1;
> > +             goto finish;
> > +     }
> > +     else {
> > +             error(_("You need to start by \"git bisect start\". You "
> > +                     "then need to give me at least one %s and %s "
> > +                     "revision. You can use \"git bisect %s\" and "
> > +                     "\"git bisect %s\" for that.\n"),
> > +                     good_syn, bad_syn, bad_syn, good_syn);
>
> ...and here
>                         voc[1], voc[0], voc[0], voc[1]);
> ...
>
> > +             retval = -1;
> > +             goto finish;
> > +     }
> > +     goto finish;
> > +finish:
> > +     if (!bad_ref)
> > +             free(bad_ref);
> > +     if (!good_glob)
> > +             free(good_glob);
> > +     if (!bad_syn)
> > +             free(bad_syn);
> > +     if (!good_syn)
> > +             free(good_syn);
>
> ...and you can remove the 4 lines above.
>
> > +     return retval;
> > +}
>
> Besides that, there are again some things that I've already mentioned
> and that can be applied here, too, for example, not capitalizing
> TERM_GOOD and TERM_BAD, the goto fail simplification, the terms memory leak.