On 12/10/2016 09:04 PM, Jeff King wrote:
> 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