On Thu, Jun 16, 2016 at 9:25 PM, Pranit Bauva <pranit.bauva@xxxxxxxxx> wrote:
> Hey Eric,
>
> On Fri, Jun 17, 2016 at 12:46 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>> On Thu, Jun 16, 2016 at 3:05 PM, Pranit Bauva <pranit.bauva@xxxxxxxxx> wrote:
>>> On Thu, Jun 16, 2016 at 2:44 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>>>> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@xxxxxxxxx> 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@xxxxxxxxx>
>>>>> ---
>>>>> 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;
Thanks,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html