Subject: Re: [PATCH 2/7] sequencer: release a strbuf used in save_head()

From: Jonathan Nieder <[email-masked]>
Date: Mon, 21 May 2012 23:23:17 -0500

Ramkumar Ramachandra wrote:
> Christian Couder wrote:

>> diff --git a/sequencer.c b/sequencer.c
>> index 762c527..ad1bbea 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -741,6 +741,7 @@ static void save_head(const char *head)
>>                die_errno(_("Could not write to %s"), head_file);
>>        if (commit_lock_file(&head_lock) < 0)
>>                die(_("Error wrapping up %s."), head_file);
>> +       strbuf_release(&buf);
>>  }
>>
>>  static int reset_for_rollback(const unsigned char *sha1)
>
> Thanks.  Shouldn't we be polluting the die() pathways with this
> strbuf_release(&buf) the way save_todo() does it consistently?
> 
> 	if (write_in_full(fd, buf.buf, buf.len) < 0) {
> 		strbuf_release(&buf);
> 		die_errno(_("Could not write to %s"), todo_file);

I don't see why.  Doesn't exit take care of freeing everything?  And
looking at it from the other side, doesn't using exit mean that you
cannot be valgrind-clean anyway, since allocations by functions higher
in the call chain do not get a chance to be freed?

Jonathan