Re: [PATCH v2 5/6] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C

> On Thu, Jun 16, 2016 at 9:25 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> Hey Eric,
>>
>> On Fri, Jun 17, 2016 at 12:46 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Thu, Jun 16, 2016 at 3:05 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>>> On Thu, Jun 16, 2016 at 2:44 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>>> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>>>>> Reimplement `is_expected_rev` & `check_expected_revs` shell function in
>>>>>> C and add a `--check-expected-revs` subcommand to `git bisect--helper` to
>>>>>> call it from git-bisect.sh .
>>>>>> [...]
>>>>>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>>>>>> ---
>>>>>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>>>>>> @@ -162,13 +162,44 @@ static int bisect_reset(const char *commit)
>>>>>> +static int is_expected_rev(const char *expected_hex)
>>>>>> +{
>>>>>> +       struct strbuf actual_hex = STRBUF_INIT;
>>>>>> +       int res;
>>>>>> +
>>>>>> +       if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) < 0) {
>>>>>> +               strbuf_release(&actual_hex);
>>>>>> +               return 0;
>>>>>> +       }
>>>>>> +
>>>>>> +       strbuf_trim(&actual_hex);
>>>>>> +       res = !strcmp(actual_hex.buf, expected_hex);
>>>>>> +       strbuf_release(&actual_hex);
>>>>>> +       return res;
>>>>>> +}
>>>>>
>>>>> Not worth a re-roll, but this could be re-structured to avoid having
>>>>> to remember to release the strbuf at all exits:
>>>>>
>>>>>     struct strbuf actual_hex = ...;
>>>>>     int res = 0;
>>>>>
>>>>>     if (strbuf_read_file(...) >= 0) {
>>>>>         strbuf_trim(...);
>>>>>         res = !strcmp(...);
>>>>>     }
>>>>>     strbuf_release(...);
>>>>>     return res;
>>>>>
>>>>> Alternately:
>>>>>
>>>>>     if (strbuf_read_file(...) < 0)
>>>>>         goto done;
>>>>>
>>>>>     strbuf_trim(...);
>>>>>     res = !strcmp(...);
>>>>>
>>>>> done:
>>>>>     strbuf_release(...);
>>>>>     return res;
>>>>>
>>>>> which is a bit less compact.
>>>>
>>>> I will avoid this for the reason that I will have to create a label
>>>> for a lot of functions. If I choose to do this for one function, I
>>>> think it would be more appropriate to do the same for other functions.
>>>> There would be a lot of functions in future which would be in the same
>>>> scenario and creating a separate label for each of them would be quite
>>>> tedious. What do you think?
>>>
>>> Not sure what you're talking about. Label names are not shared across
>>> functions. Anyhow, the first suggestion I presented above is more
>>> concise than the 'goto' version.
>>
>> Yes I am aware of the fact that labels aren't shared across functions.
>> What I meant by "separate label" was that I will have to make a label
>> "fail" in each function. But I recently noticed that its used quite a
>> lot so I think it would be okay to use it. Will re-roll with using
>> labels and goto.
>
> My opinion is that if there is a more concise version without labels
> and gotos, it's better to use it, so I would suggest Eric's first
> suggestion which is:
>
>>     struct strbuf actual_hex = ...;
>>     int res = 0;
>>
>>     if (strbuf_read_file(...) >= 0) {
>>         strbuf_trim(...);
>>         res = !strcmp(...);
>>     }
>>     strbuf_release(...);
>>     return res;