Re: [PATCH] csum-file: introduce discard_hashfile()

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 26, 2024, 14:41 UTC
Message-ID: <xmqqplr0z051.fsf@gitster.g>
In-Reply-To: <20240726044216.GA642208@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 5 quoted lines

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

These localized and independent strbuf instances were indeed what discouraged me from moving other clean-up to a central place.

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

Yup, I wobbled between a more generic "cleanup" and "hashfile is the only thing that needs special clean-up right now", and it does show, as you noticed, how the error code paths after calling finalize looks like.

I think I'll rename the label to "cleaup".

Thanks.