Re: [PATCH 1/2] bisect--helper: `bisect_log` shell function in C

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

Hey Eric,

On Mon, May 16, 2016 at 1:06 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Fri, May 13, 2016 at 4:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> bisect--helper: `bisect_log` shell function in C
>
> Do you need to insert "rewrite" or "reimplement" in the subject?
>
>> Reimplement the `bisect_log` shell function in C and add a
>> `--bisect-log` subcommand to `git bisect--helper` to call it from
>> git-bisect.sh .
>>
>> Using `--bisect-log` subcommand is a temporary measure to port shell
>> function to 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 method.
>>
>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>> ---
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> @@ -79,11 +80,26 @@ int write_terms(const char *bad, const char *good)
>> +int bisect_log(void)
>
> s/^/static/

Will include this in the re-roll

>> +{
>> +       struct strbuf buf = STRBUF_INIT;
>> +
>> +       if (strbuf_read_file(&buf, ".git/BISECT_LOG", 256) < 0)
>
> As mentioned in my review of the "write-terms" rewrite, hardcoding
> ".git/" here is wrong for a variety of reasons. See get_git_dir(),
> get_git_common_dir(), etc. in cache.h instead.

Thanks. I will have a look into this.

>> +               return error(_("We are not bisecting."));
>> +
>> +       printf("%s", buf.buf);
>> +       strbuf_release(&buf);
>> +
>> +       return 0;
>> +}
>> +
>>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>>  {
>> @@ -109,6 +127,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>>                 if (argc != 2)
>>                         die(_("--write-terms requires two arguments"));
>>                 return write_terms(argv[0], argv[1]);
>> +       case BISECT_LOG:
>
> Shouldn't you be die()ing here with an appropriate error message if
> argc is not 0?

I think it would be better to check for argc != 0 and die
appropriately. Will include this in the re-roll.

>> +               return bisect_log();
>>         default:
>>                 die("BUG: unknown subcommand '%d'", cmdmode);
>>         }

Regards,
Pranit Bauva