Re: [PATCH 3/3] packfile: close_all_packs to close_object_store

From: Derrick Stolee <stolee@gmail.com>
Date: May 20, 2019, 11:55 UTC
Message-ID: <0c13a91f-e45e-d177-758a-30e9517a664f@gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.1905201141000.46@tvgsbejvaqbjf.bet>

On 5/20/2019 6:01 AM, Johannes Schindelin wrote:

> Hi Stolee,
> 
> *really* minor nit: the commit subject probably wants to have a "rename"
> after the colon ;-)

I did put that there, but then the subject line was too long. I'm not
opposed to putting it back.

> The patch looks sensible to me. Since Junio asked for a sanity check
> whether all of the call sites of `close_all_packs()` actually want to
> close the MIDX and the commit graph, too, I'll do the "speak out loud"
> type of patch review here (spoiler: all of them check out):

Thanks for the detail here!

Show 38 quoted lines

>> diff --git a/builtin/repack.c b/builtin/repack.c
>> index 67f8978043..4de8b6600c 100644
>> --- a/builtin/repack.c
>> +++ b/builtin/repack.c
>> @@ -419,7 +419,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>>  	if (!names.nr && !po_args.quiet)
>>  		printf_ln(_("Nothing new to pack."));
>>
>> -	close_all_packs(the_repository->objects);
>> +	close_object_store(the_repository->objects);
>>
>>  	/*
>>  	 * Ok we have prepared all new packfiles.
> 
> Ah, the joys of un-dynamic patch review. What you, dear reader, cannot see
> in this hunk is that the code comment at the end continues thusly:
> 
>          * First see if there are packs of the same name and if so
>          * if we can move them out of the way (this can happen if we
>          * repacked immediately after packing fully.
>          */
> 
> Meaning: we're about to rename some pack files. So the pack file handles
> need to be closed, all right, but what about the other object store
> handles? There is no mention of the commit graph (more on that below), but
> the loop following the code comment contains this:
> 
>                         if (!midx_cleared) {
>                                 clear_midx_file(the_repository);
>                                 midx_cleared = 1;
>                         }
> 
> So yes, I would give this a check.
> 
> It does puzzle me, I have to admit, that there is no (opt-in) code block
> to re-write the commit graph. After all, the commit graph references the
> pack files, right? So if they are repacked, it would at least be
> invalidated at this point...

The commit-graph does not directly reference the packs. The file will still be valid, except if we GC'd some commits that it references. We have the ability to rewrite the graph in 'git gc'.

The MIDX does reference packs by name, so it needs to be cleared before we delete packs. This _could_ be done with more care: we only need to delete it if a pack it references is queued for deletion. However, you can do that using the 'git multi-pack-index expire|repack' pattern currently cooking.

Thanks, -Stolee