Re: [PATCH v2 4/5] Make sequencer abort safer

From: Stephan Beyer <s-beyer@gmx.net>
Date: Dec 10, 2016, 20:20 UTC
Message-ID: <102c7dd1-fa70-6a47-3f13-ddafbce4b13b@gmx.net>
In-Reply-To: <20161210200437.ijmahia6e6xifhk6@sigill.intra.peff.net>

On 12/10/2016 09:04 PM, Jeff King wrote:

Show 30 quoted lines

> On Sat, Dec 10, 2016 at 08:56:26PM +0100, Christian Couder wrote:
> 
>>> +static int rollback_is_safe(void)
>>> +{
>>> +       struct strbuf sb = STRBUF_INIT;
>>> +       struct object_id expected_head, actual_head;
>>> +
>>> +       if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) {
>>> +               strbuf_trim(&sb);
>>> +               if (get_oid_hex(sb.buf, &expected_head)) {
>>> +                       strbuf_release(&sb);
>>> +                       die(_("could not parse %s"), git_path_abort_safety_file());
>>> +               }
>>> +               strbuf_release(&sb);
>>> +       }
>>
>> Maybe the following is a bit simpler:
>>
>>        if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) {
>>                int res;
>>                strbuf_trim(&sb);
>>                res = get_oid_hex(sb.buf, &expected_head);
>>                strbuf_release(&sb);
>>                if (res)
>>                    die(_("could not parse %s"), git_path_abort_safety_file());
>>        }
> 
> Is there any point in calling strbuf_release() if we're about to die
> anyway? I could see it if it were "return error()", but it's normal in
> our code base for die() to be abrupt.

The point is that someone "libifies" the function some day; then "die()" becomes "return error()" almost automatically. Chances are high that the resulting memory leak is forgotten. That's one of the reasons why I like being strict about memory leaks.

However, I cannot tell if mine or Christian's variant is really "simpler" (with whatever measure) and I also don't care much.

~Stephan