From: "René Scharfe" <l.s.r@web.de>
To: Fredrik Gustafsson <iveqy@iveqy.com>, pclouds@gmail.com
Cc: git@vger.kernel.org
Subject: Re: [[RFC memory leak, v.2]] Minor memory leak fix
Date: Tue, 11 Mar 2014 20:40:07 +0100 [thread overview]
Message-ID: <531F6697.1030703@web.de> (raw)
In-Reply-To: <1394541371-2960-1-git-send-email-iveqy@iveqy.com>
Am 11.03.2014 13:36, schrieb Fredrik Gustafsson:
> Strbuf needs to be released even if it's locally declared.
>
> Signed-off-by: Fredrik Gustafsson <iveqy@iveqy.com>
> ---
> archive.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/archive.c b/archive.c
> index 346f3b2..dfc557d 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -113,6 +113,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
> struct git_attr_check check[2];
> const char *path_without_prefix;
> int err;
> + int to_ret = 0;
>
> args->convert = 0;
> strbuf_reset(&path);
> @@ -127,22 +128,25 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
> setup_archive_check(check);
> if (!git_check_attr(path_without_prefix, ARRAY_SIZE(check), check)) {
> if (ATTR_TRUE(check[0].value))
> - return 0;
> + goto cleanup;
> args->convert = ATTR_TRUE(check[1].value);
> }
>
> if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
> if (args->verbose)
> fprintf(stderr, "%.*s\n", (int)path.len, path.buf);
> - err = write_entry(args, sha1, path.buf, path.len, mode);
> - if (err)
> - return err;
> - return (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);
> + to_ret = write_entry(args, sha1, path.buf, path.len, mode);
> + if (!to_ret)
> + to_ret = (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);
> + goto cleanup;
Why add to_ret when you can use the existing variable err directly? Or
at least remove it when it's not used anymore.
> }
>
> if (args->verbose)
> fprintf(stderr, "%.*s\n", (int)path.len, path.buf);
> - return write_entry(args, sha1, path.buf, path.len, mode);
> + to_ret = write_entry(args, sha1, path.buf, path.len, mode);
> +cleanup:
> + strbuf_release(&path);
> + return to_ret;
If you free the memory of the strbuf at the end of the function then
there is no point in keeping it static anymore. Growing it to PATH_MAX
also doesn't make as much sense as before then.
The patch makes git archive allocate and free the path strbuf once per
archive entry, while before it allocated only once per run and left
freeing to the OS. What's the performance impact of this change?
> }
>
> int write_archive_entries(struct archiver_args *args,
>
next prev parent reply other threads:[~2014-03-11 19:41 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-03-11 10:45 [RFC memory leak?] Minor memory leak fix Fredrik Gustafsson
2014-03-11 11:58 ` Duy Nguyen
2014-03-11 12:36 ` [[RFC memory leak, v.2]] " Fredrik Gustafsson
2014-03-11 19:40 ` René Scharfe [this message]
2014-03-11 12:40 ` [RFC memory leak?] " Fredrik Gustafsson
2014-03-11 19:42 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=531F6697.1030703@web.de \
--to=l.s.r@web.de \
--cc=git@vger.kernel.org \
--cc=iveqy@iveqy.com \
--cc=pclouds@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).