Re: [PATCH 1/1] pack-write, pack-bitmap-write: register tmp pack files for cleanup
flat view
From: Patrick Steinhardt <hidden>
Date: 2026-09-28 07:58:47
On Fri, Sep 25, 2026 at 01:56:33PM -0700, Royce Remer wrote:
quoted hunk ↗ jump to hunk
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