On Wed, Aug 28, 2024 at 10:55:20AM -0700, Junio C Hamano wrote: > With -Wunused, the compiler notices that the midx_name parameter is > unused. In this case, it is truly unused, the function signature is > not constrained externally, so we can simply drop the parameter from > the definition of the function and its sole caller. > > This comes from 01a2cbab (midx: implement writing incremental MIDX > bitmaps, 2024-08-15), so I'll squash the following to that commit. Well, that didn't take long for this to come up again. :) I've been fixing them progressively as they hit 'next' (since I don't usually build 'seen'), but this one isn't there yet. I'm always curious in these cases why we have the parameter at all if it's unnecessary (i.e., is it a bug or leftover cruft). In this case, it was present before that commit, but refactoring meant that we no longer write to $name-$hash.bitmap, but instead use get_midx_filename_ext(), or get_split_midx_filename_ext() in incremental mode. Is that right, though? It looks like the caller might pass in a tempfile name like .../pack/multi-pack-index.d/tmp_midx_XXXXXX, if we're in incremental mode. But we'll write directly to "multi-pack-index-$hash.bitmap" in the same directory. I'm not sure to what degree it matters, since that's the name we want in the long run. But would we possibly overwrite an active-in-use file rather than doing the atomic rename-into-place if we happened to generate the same midx? It feels like we should still respect the name the caller is using for tempfiles, and then rename it into the correct spot at the end. -Peff
