Re: [PATCH v2 6/6] bisect--helper: `bisect_write` shell function in C

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 16, 2016, 18:55 UTC
Message-ID: <CAPig+cQV3FTGJBvS0Kuc3CNPiwZMZtApT6r8+Ojhw3y7O2VTiw@mail.gmail.com>
In-Reply-To: <20160615140026.10519-7-pranit.bauva@gmail.com>

On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:

Show 14 quoted lines

> Reimplement the `bisect_write` shell function in C and add a
> `bisect-write` subcommand to `git bisect--helper` to call it from
> git-bisect.sh
>
> Using `--bisect-write` subcommand is a temporary measure to port shell
> function in C so as to use the existing test suite. As more functions
> are ported, this subcommand will be retired and will be called by some
> other methods.
>
> Note: bisect_write() uses two variables namely TERM_GOOD and TERM_BAD
> from the global shell script thus we need to pass it to the subcommand
> using the arguments. After the whole conversion, we can remove the extra
> arguments and make the method use the two variables from the global scope
> within the C code.

You could do this now rather than waiting for later. Instead of passing these arguments to bisect_write(), create global variables in this patch and assign them in the BISECT_WRITE case of cmd_bisect__helper() before calling bisect_write().

Not necessarily worth a re-roll, but would save you the effort of having to explain it here and then change it in some later patch.

Show 26 quoted lines

> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> @@ -192,6 +193,55 @@ static int check_expected_revs(const char **revs, int rev_nr)
> +static int bisect_write(const char *state, const char *rev,
> +                       const char *term_good, const char *term_bad,
> +                       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;
> +
> +       if (!strcmp(state, term_bad))
> +               strbuf_addf(&tag, "refs/bisect/%s", state);
> +       else if(one_of(state, term_good, "skip", NULL))
> +               strbuf_addf(&tag, "refs/bisect/%s-%s", state, rev);
> +       else
> +               return error(_("Bad bisect_write argument: %s"), state);
> +
> +       if (get_oid(rev, &oid)) {
> +               strbuf_release(&tag);
> +               return error(_("couldn't get the oid of the rev '%s'"), rev);
> +       }

Minor: If you move the get_oid() conditional before the one above it,
then you won't have to worry about releasing 'strbuf tag' at this
point.

Show 5 quoted lines

> +       if (update_ref(NULL, tag.buf, oid.hash, NULL, 0,
> +                      UPDATE_REFS_MSG_ON_ERR)) {
> +               strbuf_release(&tag);
> +               return -1;
> +       }

If you release 'strbuf tag' right here, after it's final use, then you won't have to worry about releasing it anywhere below (particularly in the error cases).

Show 24 quoted lines

> +       fp = fopen(git_path_bisect_log(), "a");
> +       if (!fp) {
> +               strbuf_release(&tag);
> +               return error_errno(_("couldn't open the file '%s'"), git_path_bisect_log());
> +       }
> +
> +       commit = lookup_commit_reference(oid.hash);
> +       format_commit_message(commit, "%s", &commit_name, &pp);
> +       fprintf(fp, "# %s: [%s] %s\n", state, sha1_to_hex(oid.hash),
> +               commit_name.buf);
> +
> +       if (!nolog)
> +               fprintf(fp, "git bisect %s %s\n", state, rev);
> +
> +       strbuf_release(&commit_name);
> +       strbuf_release(&tag);
> +       fclose(fp);
> +       return 0;
> +}
> @@ -241,6 +295,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> +       case BISECT_WRITE:
> +               if (argc != 4 && argc != 5)
> +                       die(_("--bisect-write requires either 4 or 5 arguments"));
> +               nolog = (argc == 5) && !strcmp(argv[4], "nolog");

This is minor and won't matter in the long run when this code goes away later in the C conversion, but this differs from the shell code which only cared that a (non-empty) fifth argument was provided but didn't care about the actual value, whereas this code expects the argument to be exactly "nolog".

> +               return bisect_write(argv[0], argv[1], argv[2], argv[3], nolog);
>         default:
>                 die("BUG: unknown subcommand '%d'", cmdmode);
>         }