Re: [PATCH 1/1] gc/repack: release packs when needed

Jeff King [off-list ref] writes:

On Thu, Jan 10, 2019 at 01:01:36PM -0800, Junio C Hamano wrote:

quoted

quoted

diff --git a/builtin/gc.c b/builtin/gc.c
index 871a56f1c5..df90fd7f51 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c

@@ -659,8 +659,10 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 
 	report_garbage = report_pack_garbage;
 	reprepare_packed_git(the_repository);
-	if (pack_garbage.nr > 0)
+	if (pack_garbage.nr > 0) {
+		close_all_packs(the_repository->objects);
 		clean_pack_garbage();
+	}

Closing before removing does make sense, but wouldn't we want to
move reprepare_packed_git() after clean_pack_garbage() while at it?
After all, the logical sequence is that we used the current set of
packs to figure out whihch ones are garbage, then now we are about
to discard.  We close the packs in the current set (i.e. the fix
made in this patch), discard the garbage packs.  It would make sense
to start using the new set (i.e. "reprepare") after all that is
done, no?  Especially, given that the next step (write-commit-graph)
still wants to read quite a lot of data from now the latest set of
packfiles...

I agree that your suggested ordering makes more sense, but I don't think
it matters in practice with the current code. reprepare_packed_git()
never throws away old pack entries (and if they're mmap'd, we might even
continue to use them). So the end result is the same either way.

Yeah, it would not make difference to the machine.  I was trying to
be more helpful to human readers.

In any case, this patch from Dec 15 last year is where my backlog
sweeping is at right now X-<.