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

> 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?