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

[Pranit Bauva <pranit.bauva@gmail.com>]

Hey Eric,

On Wed, Jun 8, 2016 at 1:29 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> 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 ...");

Umm, I think that for all x belonging to integers,
            x <= 0    <=>      x < 1

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

strbuf_read_file() opens the file and then reads its contents. Well
this much of computation isn't really required. By using stat we can
get the file size without actually reading the file. Are there any
benefits which I have missed of using strbuf_read_file() over stat() ?

>     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()?

The shell script didn't report any error but I guess this doesn't have
to continue. Its probably better to add error handling code while
rewriting.

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

I wanted to avoid nesting. In a few other parts of the code also,
nesting is avoided as much as possible.

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

Regards,
Pranit Bauva