On Fri, Jul 26, 2024 at 12:42:16AM -0400, Jeff King wrote:
> On Thu, Jul 25, 2024 at 04:07:28PM -0700, Junio C Hamano wrote:
>
> > Introduce discard_hashfile() API function to allow them to release
> > the resources held by a hashfile structure the callers want to
> > dispose of, and use that in read-cache.c:do_write_index(), which is
> > a central place that writes the index file.
>
> Nicely explained, and the patch looks good to me.
>
> A few small comments (that probably do not need any changes):
>
> > +void discard_hashfile(struct hashfile *f)
> > +{
> > + if (0 <= f->check_fd)
> > + close(f->check_fd);
> > + if (0 <= f->fd)
> > + close(f->fd);
> > + free_hashfile(f);
> > +}
Are we sure that this is always correct? A valid file descriptor may
have a zero value, and we wouldn't end up closing it here.
> > @@ -2992,8 +2992,14 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
> > hashwrite(f, sb.buf, sb.len);
> > strbuf_release(&sb);
> > free(ieot);
> > - if (err)
> > - return -1;
> > + /*
> > + * NEEDSWORK: write_index_ext_header() never returns a failure,
> > + * and this part may want to be simplified.
> > + */
> > + if (err) {
> > + err = -1;
> > + goto discard_hashfile_and_return;
> > + }
> > }
>
> There's other repeated cleanup happening here, like free(ieot) and
> strbuf_release(), which made wonder if we could bump it down to the
> cleanup label at the end of the function to simplify things. But
> probably not, as we are often doing that cleanup even in the non-error
> case. And of course the "sb" strbuf is local to a lot of blocks.
>
> So even if we did want to do it, I think it would come as a separate
> patch. But mostly I wondered whether the label should be a more generic
> "cleanup" than "discard_hashfile". We could probably worry about that
> later, though, if that separate patch ever materializes.
Indeed, I wanted to say the same. I've got a patch series sitting around
locally where I do this. I guess I should send out my memory leak fixes
sooner rather than later to avoid duplicated work :)
Patrick
Attachment:
signature.asc
Description: PGP signature
