packfile.c: remove unnecessary prepare_packed_git() call

Commit Message

Taylor Blau Nov. 20, 2024, 10:48 p.m. UTC

In 454ea2e4d7 (treewide: use get_all_packs, 2018-08-20) we converted
existing calls to both:

  - get_packed_git(), as well as
  - the_repository->objects->packed_git

, to instead use the new get_all_packs() function.

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.

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.

Signed-off-by: Taylor Blau <[e-mail]>
---
 packfile.c | 1 -
 1 file changed, 1 deletion(-)


base-commit: 78e4bc3efc49ee4c9ec1ce14117c2e7d99999234
--
This applies on top of Karthik's series, and was something small I
noticed while reading it.

2.47.0.237.gc601277f4c4

Comments

Jeff King Nov. 21, 2024, 9:13 a.m. UTC | #1

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

Patch

diff --git a/packfile.c b/packfile.c
index 724ce8e977..5585075023 100644
--- a/packfile.c
+++ b/packfile.c
@@ -2223,7 +2223,6 @@  int for_each_packed_object(struct repository *repo, each_packed_object_fn cb,
 	int r = 0;
 	int pack_errors = 0;

-	prepare_packed_git(repo);
 	for (p = get_all_packs(repo); p; p = p->next) {
 		if ((flags & FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local)
 			continue;