[RFC memory leak?] Minor memory leak fix

Strbuf needs to be released even if it's locally declared.

Signed-off-by: Fredrik Gustafsson <redacted>
---
 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

On Tue, Mar 11, 2014 at 5:45 PM, Fredrik Gustafsson [off-list ref] 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.

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

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Duy

Strbuf needs to be released even if it's locally declared.

Signed-off-by: Fredrik Gustafsson <redacted>
---
 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;
 	}
 
 	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

On Tue, Mar 11, 2014 at 06:58:11PM +0700, Duy Nguyen wrote:

On Tue, Mar 11, 2014 at 5:45 PM, Fredrik Gustafsson [off-list ref] wrote:

quoted

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.

quoted

+               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.

quoted

                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: iveqy@iveqy.com

Am 11.03.2014 13:36, schrieb Fredrik Gustafsson:

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,