On Wed, Apr 19, 2023 at 03:00:48PM -0700, Junio C Hamano wrote: > Taylor Blau <me@xxxxxxxxxxxx> writes: > > > 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. > > Indeed the string that holds the name of the file returned by > write_mtimes_file() is leaking. Does the same logic apply to the > returned filename from write_rev_file() and stored in rev_tmp_name > that is not freed in stage_tmp_packfiles() in another topic? They are similar, but unfortunately different. Here, our temporary name is generated by `write_mtimes_file()` which *always* comes up with a new name from scratch. So we know that it should always be free()'d at the end of `stage_tmp_packfiles()`. But in the case of `write_rev_file()`, it only *sometimes* generates a new name from scratch. The first parameter can be NULL, in which case `write_rev_file()` will generate a new name. Or it can be non-NULL, in which case that name will be used instead. So in that topic, it's less clear on what the ultimate right path forward is. But in this topic, changing `mtimes_tmp_name` and the return type of `write_mtimes_file()` to be a non-const `char *` (and free()ing it, of course ;-)) is the right thing to do. Thanks, Taylor
