On Tue, Mar 11, 2014 at 06:58:11PM +0700, Duy Nguyen wrote:
> On Tue, Mar 11, 2014 at 5:45 PM, Fredrik Gustafsson <iveqy@xxxxxxxxx> 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@xxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html