Jeff King <peff@xxxxxxxx> writes:
> On Thu, Jan 10, 2019 at 01:01:36PM -0800, Junio C Hamano wrote:
>
>> > 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-<.
