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);
> +}
I wondered if we could call this function to replace other spots that
close the descriptors. But I don't think so. There is only one such
spot, in finalize_hashfile(), and it does extra work to make sure that
the close succeeds. So it wouldn't make sense to convert, and nobody
else (until now) bothered to clean up a discarded hashfile.
> diff --git a/read-cache.c b/read-cache.c
> index 48bf24f87c..d96a2cb8cf 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2963,7 +2963,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
>
> if (err) {
> free(ieot);
> - return err;
> + goto discard_hashfile_and_return;
> }
>
> offset = hashfile_total(f);
> @@ -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.
> @@ -3106,6 +3158,11 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
> istate->cache_nr);
>
> return 0;
> +
> +discard_hashfile_and_return:
> + if (f)
> + discard_hashfile(f);
> + return err;
> }
OK, so here's our cleanup label. We check that "f" is valid. I notice we
don't initialize it to NULL, but assigning to it from hashfd() is the
very first thing we do, so it will never be uninitialized. Good.
That made me wonder when it would ever be NULL, and the answer is that
it becomes so after we finalize it:
> @@ -3085,13 +3134,16 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
>
> finalize_hashfile(f, istate->oid.hash, FSYNC_COMPONENT_INDEX,
> CSUM_HASH_IN_STREAM | csum_fsync_flag);
> + f = NULL;
which makes sense. Arguably finalize_hashfile() could take a
pointer-to-pointer and set it to NULL itself for safety/simplicity, but
it's probably OK to let the caller deal with it (we do that trick in the
tempfile API, but not elsewhere).
One thing I did notice. The full hunk above is:
> @@ -3085,13 +3134,16 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
>
> finalize_hashfile(f, istate->oid.hash, FSYNC_COMPONENT_INDEX,
> CSUM_HASH_IN_STREAM | csum_fsync_flag);
> + f = NULL;
>
> if (close_tempfile_gently(tempfile)) {
> - error(_("could not close '%s'"), get_tempfile_path(tempfile));
> - return -1;
> + err = error(_("could not close '%s'"), get_tempfile_path(tempfile));
> + goto discard_hashfile_and_return;
> + }
> + if (stat(get_tempfile_path(tempfile), &st)) {
> + err = error_errno(_("could not stat '%s'"), get_tempfile_path(tempfile));
> + goto discard_hashfile_and_return;
> }
> - if (stat(get_tempfile_path(tempfile), &st))
> - return -1;
We know that "f" is always NULL at this point, so the new code at the
discard_hashfile_and_return label will never actually free anything.
We could continue to just "return -1" here and be fine. I am OK with it
to keep the general "jump to the cleanup label and return" pattern
consistent within the function, but it would make more sense if the
label had a less specific name. ;)
-Peff
