[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH 5/5] Split out the actual commit creation from the
From:       Kristian =?ISO-8859-1?Q?H=F8gsberg?= <krh () redhat ! com>
Date:       2007-07-31 14:11:49
Message-ID: 1185891109.11086.28.camel () hinata ! boston ! redhat ! com
[Download RAW message or body]

On Mon, 2007-07-30 at 21:43 -0700, Junio C Hamano wrote:
> Kristian Høgsberg <krh@redhat.com> writes:
> 
> > @@ -85,40 +129,20 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix)
> >  			parents++;
> >  	}
> >  
> > -	/* Not having i18n.commitencoding is the same as having utf-8 */
> > -	encoding_is_utf8 = is_encoding_utf8(git_commit_encoding);
> > +	buffer = NULL;
> > +	if (read_fd(0, &buffer, &len))
> > +		die("Could not read commit message from standard input");
> >  
> > -	strbuf_init(&sb);
> > -	strbuf_printf(&sb, "tree %s\n", sha1_to_hex(tree_sha1));
> > +	commit_sha1 = create_commit(tree_sha1,
> > +				    parent_sha1, parents,
> > +				    xstrdup(git_author_info(1)),
> > +				    xstrdup(git_committer_info(1)),
> > +				    buffer, len);
> 
> Hmph, the series was so nice so far, but here we have a few new
> leak, presumably so small per process invocation that we do not
> care about?

There's number of buffers that don't get freed: the strbuf, the commit
message buffer, and the strdup'ed author and committer info.  All the
leaks are not critical since the process exits immediately.  As for the
strbuf leak, I was thinking about renaming strbuf_begin to strbuf_reset
and making it public[1], which will then be used for freeing up strbuf
memory.  The message buffer leak should be fixed by adding a
strbuf_read_fd() that just reads it straight into the strbuf.  The
xstrdup's are necessary because fmt_ident uses a static buffer (thanks,
test case :).  We could add rotating static buffers for fmt_ident like
git_path and avoid the strdups, but again, the leaks are not critical.

Kristian

[1] strbuf_begin() is a good name the way it's used in strbuf.c where
it's balanced by strbuf_end(), but as a general purpose reset function
it's better name strbuf_reset(), I think

-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]