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

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