Re: [PATCH v3 1/4] alloc: introduce parsed_commits_count

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 12, 2020, 22:00 UTC
Message-ID: <xmqqpna40ws8.fsf@gitster.c.googlers.com>
In-Reply-To: <xmqqtuzg11zv.fsf@gitster.c.googlers.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 24 quoted lines

> Abhishek Kumar <abhishekkumar8222@gmail.com> writes:
>
>>  static unsigned int alloc_commit_index(struct repository *r)
>>  {
>> -	return r->parsed_objects->commit_count++;
>> +	static unsigned int parsed_commits_count = 0;
>> +	r->parsed_objects->commit_count++;
>> +	return parsed_commits_count++;
>>  }
>
> Hmph, ugly.  
>
> The need for this hack, which butchers the function that explicitly
> takes a repository as its parameter so that parsed commits can be
> counted per repository to not count commits per repository, makes
> the whole thing smell fishy.
>
> There shouldn't be a reason why a commit in the superproject and
> another commit in the submodule need to be in the same commit graph
> in the first place.
>
> Instead of breaking the function like this, the right "fix" may be
> to make the commit slab per repository, because the commit index are
> taken from the separate namespace per repository.

Given a commit object (i.e. "struct commit *"), currently there is no way to tell from which repository it came from. So from that point of view, we cannot identify which commit slab to use, unless we add a back-pointer that says from which repository each commit object came from, but that makes the commit object even heavier.

Back when 14ba97f8 (alloc: allow arbitrary repositories for alloc functions, 2018-05-15) made the count per repository (which you are reverting with this patch), there must have been a reason why it did so. We know that commit slab code wants you to count globally and that is why you wrote this patch, but don't other parts of the code expect and rely on the commits being counted per repository? In other words, with this change, who are you breaking to help the commit slab code?

Cc'ing SZEDER who also have touched this and made the function static early last year.

Thanks.