[RFC memory leak?] Minor memory leak fix

2014-03-11 Thread Fredrik Gustafsson 
Strbuf needs to be released even if it's locally declared.

Signed-off-by: Fredrik Gustafsson iv...@iveqy.com
---
 archive.c | 21 +++--
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/archive.c b/archive.c
index 346f3b2..d6d56e6 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);
@@ -126,8 +127,10 @@ 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;
+   if (ATTR_TRUE(check[0].value)) {
+   to_ret = 0;
+   goto cleanup;
+   }
args-convert = ATTR_TRUE(check[1].value);
}
 
@@ -135,14 +138,20 @@ static int write_archive_entry(const unsigned char *sha1, 
const char *base,
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);
+   if (err) {
+   to_ret = err;
+   goto cleanup;
+   }
+   to_ret = (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);
+   goto cleanup;
}
 
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;
 }
 
 int write_archive_entries(struct archiver_args *args,
-- 
1.8.3.1.490.g39d9b24.dirty

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

Re: [RFC memory leak?] Minor memory leak fix

2014-03-11 Thread Duy Nguyen 
On Tue, Mar 11, 2014 at 5:45 PM, Fredrik Gustafsson iv...@iveqy.com wrote:
 Strbuf needs to be released even if it's locally declared.

path is declared static. So yes it's a leak but the leak is minimum.
Your patch would make more sense if static is gone and it's leaked
after every write_archive_entry call.


 Signed-off-by: Fredrik Gustafsson iv...@iveqy.com
 ---
  archive.c | 21 +++--
  1 file changed, 15 insertions(+), 6 deletions(-)

 diff --git a/archive.c b/archive.c
 index 346f3b2..d6d56e6 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);
 @@ -126,8 +127,10 @@ 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;
 +   if (ATTR_TRUE(check[0].value)) {
 +   to_ret = 0;
 +   goto cleanup;
 +   }

to_ret is already 0 so I think goto cleanup; is enough.

 args-convert = ATTR_TRUE(check[1].value);
 }

 @@ -135,14 +138,20 @@ static int write_archive_entry(const unsigned char 
 *sha1, const char *base,
 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);
 +   if (err) {
 +   to_ret = err;
 +   goto cleanup;
 +   }
 +   to_ret = (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);
 +   goto cleanup;

Maybe if (err) to_ret = ...; else to_ret = ...; so we only need one
goto cleanup statement. Going even further:

to_ret = write_entry(...);
if (!to_ret) to_ret = (S_ISDIR(...));
goto cleanup;

 }

 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;
  }

  int write_archive_entries(struct archiver_args *args,
 --
 1.8.3.1.490.g39d9b24.dirty

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



-- 
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC memory leak?] Minor memory leak fix

2014-03-11 Thread Fredrik Gustafsson 
On Tue, Mar 11, 2014 at 06:58:11PM +0700, Duy Nguyen wrote:
 On Tue, Mar 11, 2014 at 5:45 PM, Fredrik Gustafsson iv...@iveqy.com wrote:
  Strbuf needs to be released even if it's locally declared.
 
 path is declared static. So yes it's a leak but the leak is minimum.
 Your patch would make more sense if static is gone and it's leaked
 after every write_archive_entry call.

That's one of the reasons of the RFC. I know Junio thinks that minor
things shouldn't be fixed by themselfes because it takes up review
bandwidth, so it's better to fix them once you touch that part of the
code anyway. (At least that's how I've understood him).

This leak is at about 4.1 kB so it's not huge.

  +   if (ATTR_TRUE(check[0].value)) {
  +   to_ret = 0;
  +   goto cleanup;
  +   }
 
 to_ret is already 0 so I think goto cleanup; is enough.

Agree, fixed in next iteration.

  err = write_entry(args, sha1, path.buf, path.len, mode);
  -   if (err)
  -   return err;
  -   return (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);
  +   if (err) {
  +   to_ret = err;
  +   goto cleanup;
  +   }
  +   to_ret = (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);
  +   goto cleanup;
 
 Maybe if (err) to_ret = ...; else to_ret = ...; so we only need one
 goto cleanup statement. Going even further:
 
 to_ret = write_entry(...);
 if (!to_ret) to_ret = (S_ISDIR(...));
 goto cleanup;

Agree, fixed in next iteration.

-- 
Med vänlig hälsning
Fredrik Gustafsson

tel: 0733-608274
e-post: iv...@iveqy.com
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC memory leak?] Minor memory leak fix

2014-03-11 Thread Junio C Hamano 
Fredrik Gustafsson iv...@iveqy.com writes:

 On Tue, Mar 11, 2014 at 06:58:11PM +0700, Duy Nguyen wrote:
 On Tue, Mar 11, 2014 at 5:45 PM, Fredrik Gustafsson iv...@iveqy.com wrote:
  Strbuf needs to be released even if it's locally declared.
 
 path is declared static. So yes it's a leak but the leak is minimum.
 Your patch would make more sense if static is gone and it's leaked
 after every write_archive_entry call.

 That's one of the reasons of the RFC. I know Junio thinks that minor
 things shouldn't be fixed by themselfes because it takes up review
 bandwidth, so it's better to fix them once you touch that part of the
 code anyway. (At least that's how I've understood him).

Yes, but I at the same time think this static struct strbuf is a
clear statement by the original author that this is not a leak
per-se.  The trade-off, if I am reading the code right, is between
keeping a piece of memory that is large enough to hold the longest
pathname until exit() vs saving repeated allocations and frees for
each of the thousands of paths in the resulting archive.

I tend to think the original strikes a better balance between the
two.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html