On Wed, Nov 20, 2024 at 05:48:51PM -0500, Taylor Blau wrote:
> In the instance that this commit addresses, there was a preceding call
> to prepare_packed_git(), which dates all the way back to 660c889e46
> (sha1_file: add for_each iterators for loose and packed objects,
> 2014-10-15) when its caller (for_each_packed_object()) was first
> introduced.
>
> This call could have been removed in 454ea2e4d7, since get_all_packs()
> itself calls prepare_packed_git(). But the translation in 454ea2e4d7 was
> (to the best of my knowledge) a find-and-replace rather than inspecting
> each individual caller.
Yeah, I think that describes what happened.
> Having an extra prepare_packed_git() call here is harmless, since it
> will notice that we have already set the 'packed_git_initialized' field
> and the call will be a noop. So we're only talking about a few dozen CPU
> cycles to set up and tear down the stack frame.
>
> But having a lone prepare_packed_git() call immediately before a call to
> get_all_packs() confused me, so let's remove it as redundant to avoid
> more confusion in the future.
Agreed. I think this is worth doing.
I briefly grepped for other cases. This one confused me:
builtin/gc.c=1272=static off_t get_auto_pack_size(void)
--
builtin/gc.c-1292- reprepare_packed_git(r);
builtin/gc.c:1293: for (p = get_all_packs(r); p; p = p->next) {
It's not a noop because it's calling the reprepare() function, which
will re-check the directory. But why? Are we expecting that something
changed? This is called only when making the midx, so maybe it's trying
to refresh the set of packs after repacking. But that seems like
something that should happen explicitly, not as a side effect of an
otherwise read-only function.
Removing it still passes the tests. So I dunno. It looks superfluous to
me, but it's perhaps more risky than the one you identified.
-Peff
