Re: [PATCH] packfile: fix perf regression with many packs

Patrick Steinhardt <[email protected]> writes:

>> -void packfile_list_append(struct packfile_list *list, struct packed_git *pack)
>> +void packfile_list_append(struct packfile_list *list, struct packed_git *pack,
>> +			  int is_new)
>>  {
>>  	struct packfile_list_entry *entry;
>>  
>> -	entry = packfile_list_remove_internal(list, pack);
>> +	entry = is_new ? NULL : packfile_list_remove_internal(list, pack);
>>  	if (!entry) {
>>  		entry = xmalloc(sizeof(*entry));
>>  		entry->pack = pack;
>
> I wonder whether we should slightly reformulate this and rename `is_new`
> to `accept_duplicates`. Because ultimately, that is what we're doing
> now: instead of ensuring that the packfile is unique in the list, we
> just don't care and just append the entry to the list.

I had the same thought.  The current callers might have been vetted
thoroughly, but the next caller might not be so careful, and for
that matter, the code paths to reach current caller may change in
the future to break the promise of ever throwing a new pack at
packfile_list.

Is it well understood what bad things it will lead to to have
duplicated entries on a packfile_list (other than it would make it
even less efficient to prove the non-existence of a pack on it, and
possibly a bit more efficient, depending on where duplicates are, to
prove the existence of a pack on it?)