Re: [PATCH 1/1] pack-write, pack-bitmap-write: register tmp pack files for cleanup

[Patrick Steinhardt <ps@pks.im>]

On Fri, Sep 25, 2026 at 01:56:33PM -0700, Royce Remer wrote:
> diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
> index 1bcb3f98a4..c566419690 100644
> --- a/pack-bitmap-write.c
> +++ b/pack-bitmap-write.c
> @@ -1378,6 +1379,7 @@ void bitmap_writer_finish(struct bitmap_writer *writer,
>  
>  	int fd = odb_mkstemp(writer->repo->objects, &tmp_file,
>  			     "pack/tmp_bitmap_XXXXXX");
> +	struct tempfile *tmp = register_tempfile(tmp_file.buf);
>  
>  	if (writer->pseudo_merges_nr)
>  		options |= BITMAP_OPT_PSEUDO_MERGES;
> @@ -1435,6 +1437,7 @@ void bitmap_writer_finish(struct bitmap_writer *writer,
>  
>  	if (rename(tmp_file.buf, filename))
>  		die_errno("unable to rename temporary bitmap file to '%s'", filename);
> +	delete_tempfile(&tmp);
>  
>  	strbuf_release(&tmp_file);
>  	free(offsets);

The fact that we add calls to `register_tempfile()` to almost every
single callsites that uses `odb_mkstemp()` makes me wonder whether the
interface itself is maybe misdesigned. Like, should it maybe return a
tempfile instead of returning a file descriptor so that callers don't
have to manually register it?

I also wonder whether `odb_mkstemp()` even sits at the right level to
begin with. It's ultimately specific to the "files" backend, as it
assumes that files live in "objects/". Would it be preferable if we
instead made it part of the "tempfile.h" API, where the only difference
to other functions is that it knows to also support leading directories?

We could for example have a new "_d" suffix for `mks_tempfile()`
functions.

Thanks!

Patrick