Re: [PATCH v15 13/27] bisect--helper: `bisect_start` shell function partially in C

Hey Stephan,

On Wed, Nov 16, 2016 at 4:49 AM, Stephan Beyer [off-list ref] wrote:

Hi,

On 10/14/2016 04:14 PM, Pranit Bauva wrote:

quoted

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

quoted

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

True. I will remove this line.

quoted

+                     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);

quoted

+             } 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 = ...

My bad.

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

Sure!

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

quoted

+             } else if (starts_with(arg, "--") &&
+                      !one_of(arg, "--term-good", "--term-bad", NULL)) {
+                     die(_("unrecognised option: '%s'"), arg);

[...]

quoted

+     /*
+      * 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 ;-)

I will add it back again.

quoted

+             /* 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;

[...]

quoted

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

quoted

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

I think this would be a better choice. Thanks!

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

quoted

+     if (!fp)
+             return -1;

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

quoted

+     goto finish;
+finish:

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

quoted

+     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*

Nice little trick.

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

Regards,
Pranit Bauva