From: Pranit Bauva <pranit.bauva@gmail.com>
Date: Thu, 16 Jun 2016 19:25:54 GMT
Subject: Re: [PATCH v2 5/6] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C
Message-ID: <CAFZEwPPBTd6o7qg96cet8RAp5wY1shzQ=jU8dEK1irXSiMAxQg@mail.gmail.com>
In-Reply-To: <CAPig+cS1=bv588H6yrOy1oFvArjJNbetdTS7+ZhfGn-GtVp4qw@mail.gmail.com>

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.

Regards,
Pranit Bauva