From: Fredrik Gustafsson Date: Tue, 11 Mar 2014 12:40:09 GMT Subject: Re: [RFC memory leak?] Minor memory leak fix Message-ID: <20140311124009.GI14327@paksenarrion.iveqy.com> In-Reply-To: On Tue, Mar 11, 2014 at 06:58:11PM +0700, Duy Nguyen wrote: > On Tue, Mar 11, 2014 at 5:45 PM, Fredrik Gustafsson 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: iveqy@iveqy.com