Hi,

On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 6a5878c..1d3e17f 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -24,6 +27,8 @@ static const char * const git_bisect_helper_usage[] = {
>       N_("git bisect--helper --bisect-check-and-set-terms <command> 
> <TERM_GOOD> <TERM_BAD>"),
>       N_("git bisect--helper --bisect-next-check [<term>] <TERM_GOOD> 
> <TERM_BAD"),
>       N_("git bisect--helper --bisect-terms [--term-good | --term-old | 
> --term-bad | --term-new]"),
> +     N_("git bisect--helper --bisect start [--term-{old,good}=<term> 
> --term-{new,bad}=<term>]"
> +                                           "[--no-checkout] [<bad> 
> [<good>...]] [--] [<paths>...]"),

Typo: "--bisect start" with space instead of "-"

> @@ -403,6 +408,205 @@ static int bisect_terms(struct bisect_terms *terms, 
> const char **argv, int argc)
>       return 0;
>  }
>  
> +static int bisect_start(struct bisect_terms *terms, int no_checkout,
> +                     const char **argv, int argc)
> +{
> +     int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
> +     int flags, pathspec_pos, retval = 0;
> +     struct string_list revs = STRING_LIST_INIT_DUP;
> +     struct string_list states = STRING_LIST_INIT_DUP;
> +     struct strbuf start_head = STRBUF_INIT;
> +     struct strbuf bisect_names = STRBUF_INIT;
> +     struct strbuf orig_args = STRBUF_INIT;
> +     const char *head;
> +     unsigned char sha1[20];
> +     FILE *fp = NULL;
> +     struct object_id oid;
> +
> +     if (is_bare_repository())
> +             no_checkout = 1;
> +
> +     for (i = 0; i < argc; i++) {
> +             if (!strcmp(argv[i], "--")) {
> +                     has_double_dash = 1;
> +                     break;
> +             }
> +     }
> +
> +     for (i = 0; i < argc; i++) {
> +             const char *commit_id = xstrfmt("%s^{commit}", argv[i]);
> +             const char *arg = argv[i];
> +             if (!strcmp(argv[i], "--")) {
> +                     has_double_dash = 1;

This is without effect since has_double_dash is already set to 1 by the
loop above. I think you can remove this line.

> +                     break;
> +             } else if (!strcmp(arg, "--no-checkout")) {
> +                     no_checkout = 1;
> +             } else if (!strcmp(arg, "--term-good") ||
> +                      !strcmp(arg, "--term-old")) {
> +                     must_write_terms = 1;
> +                     terms->term_good = xstrdup(argv[++i]);
> +             } else if (skip_prefix(arg, "--term-good=", &arg)) {
> +                     must_write_terms = 1;
> +                     terms->term_good = xstrdup(arg);
> +             } else if (skip_prefix(arg, "--term-old=", &arg)) {
> +                     must_write_terms = 1;
> +                     terms->term_good = xstrdup(arg);

I think you can join the last two branches:

+               } else if (skip_prefix(arg, "--term-good=", &arg) ||
+                          skip_prefix(arg, "--term-old=", &arg)) {
+                       must_write_terms = 1;
+                       terms->term_good = xstrdup(arg);

> +             } else if (!strcmp(arg, "--term-bad") ||
> +                      !strcmp(arg, "--term-new")) {
> +                     must_write_terms = 1;
> +                     terms->term_bad = xstrdup(argv[++i]);
> +             } else if (skip_prefix(arg, "--term-bad=", &arg)) {
> +                     must_write_terms = 1;
> +                     terms->term_bad = xstrdup(arg);
> +             } else if (skip_prefix(arg, "--term-new=", &arg)) {
> +                     must_write_terms = 1;
> +                     terms->term_good = xstrdup(arg);

This has to be terms->term_bad = ...

Also, you can join the last two branches, again, ie,

+               } else if (skip_prefix(arg, "--term-bad=", &arg) ||
+                          skip_prefix(arg, "--term-new=", &arg)) {
+                       must_write_terms = 1;
+                       terms->term_bad = xstrdup(arg);

> +             } else if (starts_with(arg, "--") &&
> +                      !one_of(arg, "--term-good", "--term-bad", NULL)) {
> +                     die(_("unrecognised option: '%s'"), arg);
[...]
> +     /*
> +      * Verify HEAD
> +      */
> +     head = resolve_ref_unsafe("HEAD", 0, sha1, &flags);
> +     if (!head)
> +             if (get_sha1("HEAD", sha1))
> +                     die(_("Bad HEAD - I need a HEAD"));
> +
> +     if (!is_empty_or_missing_file(git_path_bisect_start())) {

You were so eager to re-use the comments from the shell script, but you
forgot the "Check if we are bisecting." comment above this line ;-)

> +             /* Reset to the rev from where we started */
> +             strbuf_read_file(&start_head, git_path_bisect_start(), 0);
> +             strbuf_trim(&start_head);
> +             if (!no_checkout) {
> +                     struct argv_array argv = ARGV_ARRAY_INIT;
[...]
> +     if (must_write_terms)
> +             if (write_terms(terms->term_bad, terms->term_good)) {
> +                     retval = -1;
> +                     goto finish;
> +             }
> +

bisect_start() is a pretty big function.
I think it can easily be decomposed in some smaller parts, for example,
the following lines ...

> +     fp = fopen(git_path_bisect_log(), "a");
> +     if (!fp)
> +             return -1;
> +
> +     if (fprintf(fp, "git bisect start") < 1) {
> +             retval = -1;
> +             goto finish;
> +     }
> +
> +     sq_quote_argv(&orig_args, argv, 0);
> +     if (fprintf(fp, "%s", orig_args.buf) < 0) {
> +             retval = -1;
> +             goto finish;
> +     }
> +     if (fprintf(fp, "\n") < 1) {
> +             retval = -1;
> +             goto finish;
> +     }

... could be in a function like

static int bisect_append_log(const char **argv)
{
        FILE *fp = fopen(git_path_bisect_log(), "a");
        struct strbuf orig_args = STRBUF_INIT;
        if (!fp)
                return -1;

        if (fprintf(fp, "git bisect start") < 1) {
                retval = -1;
                goto finish;
        }

        sq_quote_argv(&orig_args, argv, 0);
        if (fprintf(fp, "%s", orig_args.buf) < 0 ||
            fprintf(fp, "\n") < 1) {
                retval = -1;
                goto finish;
        }

finish:
        if (fp)
                fclose(fp);
        strbuf_release(&orig_args);

        return retval;
}

and then simply call

        retval = bisect_append_log(argv);

in bisect_start()... (This is totally untested.)

If you do not want that for some reason, you should at least fix

> +     if (!fp)
> +             return -1;

to retval = 1; goto finish; such that the other lists and strings are
released.

> +     goto finish;
> +finish:

The "goto finish" right above the "finish" label is unnecessary.

> +     if (fp)
> +             fclose(fp);
> +     string_list_clear(&revs, 0);
> +     string_list_clear(&states, 0);
> +     strbuf_release(&start_head);
> +     strbuf_release(&bisect_names);
> +     strbuf_release(&orig_args);
> +     return retval;
> +}
> +
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
>       enum {

By the way, there are two spaghetti-ish ways to get rid of the

        retval = -1;
        goto finish;

line pair:

        goto fail;

and below the "return retval;" add

fail:
        retval = -1;
        goto finish;

and you can feel the touch of His Noodly Appendage. *scnr*

The other way is to keep the "goto finish" I deemed unnecessary (right
above the label), and expand it to:

        goto finish;
fail:
        retval = -1;
finish:
        ...

Cheers
  Stephan

Reply via email to