Re: [PATCH] repack: respect --keep-pack with geometric repack

From: Junio C Hamano <gitster@pobox.com>
Date: May 20, 2022, 20:41 UTC
Message-ID: <xmqqr14oey2a.fsf@gitster.g>
In-Reply-To: <YofJLv8+x5J7yPmf@nand.local>

Taylor Blau <me@ttaylorr.com> writes:

Show 8 quoted lines

>>  	*geometry_p = xcalloc(1, sizeof(struct pack_geometry));
>>  	geometry = *geometry_p;
>>
>> +	string_list_sort(existing_kept_packs);
>
> Would it be worth sorting this as early as in collect_pack_filenames()?
> For our purposes in this patch, this works as-is, but it may be
> defensive to try and minimize the time that list has unsorted contents.

It does not matter too much but after reading the latest one before coming back to this original thread, I actually thought that sorting it too early was questionable. Nobody other than this code cares about the list being sorted, and if it turns out that the look-up in the loop need to be optimized, it is plausible that we do not need this list sorted when we populate a hashmap out of its contents, for example.

Show 15 quoted lines

>>  	for (p = get_all_packs(the_repository); p; p = p->next) {
>> -		if (!pack_kept_objects && p->pack_keep)
>> -			continue;
>> +		if (!pack_kept_objects) {
>> +			if (p->pack_keep)
>> +				continue;
>
> (You mentioned this to me off-list, but I'll repeat it here since it
> wasn't obvious to me on first read): this check for `p->pack_keep` isn't
> strictly necessary, since any packs that have their `pack_keep` bit set
> will appear in the `existing_kept_packs` list.
>
> But it does give us a fast path to avoid having to check that list, so
> it's worth checking that bit to avoid a slightly more expensive check
> where possible.

That invites a related but different question. Doesn't p->pack_keep and string_list_has_string(existing_kept_packs, name_of_pack(p)) mirror each other? Can a pack appear on the existing_kept_packs list without having .pack_keep bit set and under what condition?

It is answered by the comment below, I presume?

Show 5 quoted lines

>> +			/*
>> +			 * The pack may be kept via the --keep-pack option;
>> +			 * check 'existing_kept_packs' to determine whether to
>> +			 * ignore it.
>> +			 */

OK. So there are two classes of packs we want to exclude from the geometry repacking. Those that already have .pack_keep bit set, and those that are _are_ newly making into kept packs that do not yet have .pack_keep bit set. Makes sense.