Re: [PATCH 01/10] pack-write.c: plug a leak in stage_tmp_packfiles()

From: Jeff King <peff@peff.net>
Date: Apr 18, 2023, 10:30 UTC
Message-ID: <20230418103005.GC508219@coredump.intra.peff.net>
In-Reply-To: <65ac7ed9b843a83f7b4cc0acd7b4e70d98ca20aa.1681764848.git.me@ttaylorr.com>

On Mon, Apr 17, 2023 at 04:54:15PM -0400, Taylor Blau wrote:

Show 11 quoted lines

> The function `stage_tmp_packfiles()` generates a filename to use for
> staging the contents of what will become the pack's ".mtimes" file.
> 
> The name is generated in `write_mtimes_file()` and the result is
> returned back to `stage_tmp_packfiles()` which uses it to rename the
> temporary file into place via `rename_tmp_packfiles()`.
> 
> `write_mtimes_file()` returns a `const char *`, indicating that callers
> are not expected to free its result (similar to, e.g., `oid_to_hex()`).
> But callers are expected to free its result, so this return type is
> incorrect.

Makes sense. I do think in the long run that it might make sense for all of these pack-write tmpfiles to tempfile.[ch] (either directly, or via register_tempfile() after creating the file). And then it would be safe to pass around the tempfile struct itself, which contains the filename, without worrying so much about ownership issues.

But that's a much bigger change that shouldn't be part of your series.

-Peff