Re: [PATCH v15 08/27] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C

From: Stephan Beyer <s-beyer@gmx.net>
Date: Dec 17, 2016, 19:55 UTC
Message-ID: <6a8aef40-9ae9-7e7a-12b1-b28039fc6554@gmx.net>
In-Reply-To: <CAFZEwPMwviof5jNvQnxFZYdw324XpbLSQ9mzEQjrCW-K4A+GCA@mail.gmail.com>

Hi Pranit,

On 12/16/2016 08:35 PM, Pranit Bauva wrote:

Show 38 quoted lines

> On Thu, Nov 17, 2016 at 5:17 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
>> On 10/14/2016 04:14 PM, Pranit Bauva wrote:
>>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>>> index d84ba86..c542e8b 100644
>>> --- a/builtin/bisect--helper.c
>>> +++ b/builtin/bisect--helper.c
>>> @@ -123,13 +123,40 @@ static int bisect_reset(const char *commit)
>>>       return bisect_clean_state();
>>>  }
>>>
>>> +static int is_expected_rev(const char *expected_hex)
>>> +{
>>> +     struct strbuf actual_hex = STRBUF_INIT;
>>> +     int res = 0;
>>> +     if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) >= 40) {
>>> +             strbuf_trim(&actual_hex);
>>> +             res = !strcmp(actual_hex.buf, expected_hex);
>>> +     }
>>> +     strbuf_release(&actual_hex);
>>> +     return res;
>>> +}
>>
>> I am not sure it does what it should.
>>
>> I would expect the following behavior from this function:
>>  - file does not exist (or is "broken") => return 0
>>  - actual_hex != expected_hex => return 0
>>  - otherwise return 1
>>
>> If I am not wrong, the code does the following instead:
>>  - file does not exist (or is "broken") => return 0
>>  - actual_hex != expected_hex => return 1
>>  - otherwise => return 0
> 
> It seems that I didn't carefully see what the shell code is (or
> apparently did a mistake in understanding it ;)). I think the C
> version does exactly what the shell version does. Can you confirm it
> once again, please?

I check again...

The shell code does the following:
 - file does not exist or is not a regular file => return false
 - actual_hex != expected_hex => return false
 - otherwise => return true

(false means a value != 0, true means 0)

Your code does the following:
 - file cannot be read or is broken => return 0
 - actual_hex != expected_hex => return 0
 - otherwise => return 1

So you are very right, it seems I had a weird thinko (I probably missed the "!" in front of strcmp or something) :)

Sorry for bothering,
  Stephan