Re: [PATCH 5/5] Split out the actual commit creation from the option parsing etc.
- From
Kristian Høgsberg <krh@redhat.com>
- Date
- Jul 31, 2007, 14:11 UTC
- Message-ID
- <1185891109.11086.28.camel@hinata.boston.redhat.com>
- In-Reply-To
- <7v8x8xgp3s.fsf@assigned-by-dhcp.cox.net>
On Mon, 2007-07-30 at 21:43 -0700, Junio C Hamano wrote:
Show 23 quoted lines
> 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