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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 8, 2016, 17:50 UTC
Message-ID: <CAPig+cSWJjS6X=Bmet0bWQmWV9kQJS9_qGMM7hu+Rj+7_Ctf=w@mail.gmail.com>
In-Reply-To: <CAP8UFD2nfcMu7Gmh=asyu+WZh5X2UjhyYSC2Sbp-qsQVjEULcw@mail.gmail.com>

On Wed, Jun 8, 2016 at 5:51 AM, Christian Couder <christian.couder@gmail.com> wrote:

Show 17 quoted lines

> On Wed, Jun 8, 2016 at 9:59 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> +       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?

No, you're right. I misread the code as:

    file_size(...) < 0

rather than what it really says:

    file_size(...) < 1

Sorry for the noise.

That it was so easy to misread the code, however, may be a good argument for making a more special-purpose function, such as file_empty_or_missing() (or file_not_empty()) as suggested in the patch 3/4 thread.