Re: [PATCH 4/4] bisect--helper: `bisect_reset` shell function in C

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> Reimplement `bisect_reset` shell function in C and add a `--bisect-reset`
> subcommand to `git bisect--helper` to call it from git-bisect.sh .
>
> Using `bisect_reset` subcommand is a temporary measure to port shell
> functions to C so as to use the existing test suite. As more functions
> are ported, this subcommand would be retired and will be called by some
> other method.
>
> Note: --bisect-clean-state subcommand has not been retired as there are
> still a function namely `bisect_start()` which still uses this
> subcommand.
>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> @@ -118,12 +122,51 @@ int bisect_clean_state(void)
> +int bisect_reset(const char *commit)

s/^/static/

> +{
> +       struct strbuf branch = STRBUF_INIT;
> +       int status = 0;
> +
> +       if (file_size(git_path_bisect_start()) < 1) {

This doesn't even care about the size of the file, only if it
encountered an error while stat()'ing it. Why not just use
file_exists() instead (which you already use elsewhere in this
function)? Alternately, if you're trying to be faithful to the shell
code, then you *do* need to check that the file has non-zero size
before issuing the "not bisecting" diagnostic, so:

    if (file_size(git_path_bisect_start()) <= 0)
        printf("... not bisecting ...");

A different approach would be to invoke strbuf_read_file()
unconditionally, rather than performing this separate check. If
strbuf_read_file() returns -1, then the file didn't exist or couldn't
be read; if it returns 0, then the file has no content:

    if (strbuf_read_file(&branch, ..., 0) <= 0)
        printf("... not bisecting ...");

> +               printf("We are not bisecting.\n");
> +               return 0;
> +       }
> +
> +       if (!commit) {
> +               strbuf_read_file(&branch, git_path_bisect_start(), 0);

Shouldn't you be checking the return value of strbuf_read_file()?

> +               strbuf_rtrim(&branch);
> +       } else {
> +               struct object_id oid;
> +               if (get_oid(commit, &oid))
> +                       return error(_("'%s' is not a valid commit"), commit);
> +               strbuf_addf(&branch, "%s", commit);
> +       }
> +
> +       if (!file_exists(git_path_bisect_head())) {
> +               struct argv_array argv = ARGV_ARRAY_INIT;
> +               argv_array_pushl(&argv, "checkout", branch.buf, "--", NULL);
> +               status = run_command_v_opt(argv.argv, RUN_GIT_CMD);
> +               argv_array_clear(&argv);
> +       }
> +
> +       if (status) {

What's the purpose of having this 'status' conditional outside of the
above !file_exists() conditional, which is the only place that
'status' gets assigned. Likewise, 'status' itself could be declared
within the scope of that conditional block.

> +               error(_("Could not check out original HEAD '%s'. "
> +                               "Try 'git bisect reset <commit>'."), branch.buf);
> +               strbuf_release(&branch);
> +               return -1;
> +       }
> +
> +       strbuf_release(&branch);
> +       return bisect_clean_state();
> +}