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

[Christian Couder <christian.couder@gmail.com>]

On Wed, Jun 8, 2016 at 9:59 AM, 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 ()
>         printf("... not bisecting ...");

As file_size() returns an integer, there is no difference between
"file_size(git_path_bisect_start()) <= 0" and
"file_size(git_path_bisect_start()) < 1".
Or am I missing something?