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

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

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

Show 8 quoted lines

> 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?

I did a bit more digging, as I had a bit of time after pushing today's integration result out, and it turns out that in the today's code, c->index is the only thing that uses this counter. It is set in init_commit_node() function, which is called from object.c when an in-core commit object is created. The callchain looks like this:

object_as_type(struct repository *, struct object *, enum object_type, int quiet)
  -> init_commit_node(struct repository *, struct commit *)
       -> alloc_commit_index(struct repository *)

What is interesting is that object_as_type() takes the parameter "struct repository *" ONLY BECAUSE it needs to pass something to init_commit_node(), which in turn takes it ONLY BECAUSE the alloc_commit_index() wants one to be passed.

Since the ONLY reason why there needs a monotorically increasing counter of in-core commit objects is because we need to be able to index into the commit slab, and because we cannot make commit slabs per-repository due to the lack of backpointer from an in-core commit object to the repository it belongs to, reverting 14ba97f8 (alloc: allow arbitrary repositories for alloc functions, 2018-05-15) is a reasonable thing to do, but then since the only reason the above three functions in the callchain take "struct repository *" is because the bottom-most alloc_commit_index() wants it WHEN IT DOES NOT USE IT, it would be good to get rid of the "struct repository" parameter from the functions involved in the callchain altogether.

Then we won't have to ask the "who are we breaking with this change" question. At that point it would be clear that everybody would be OK with a single counter to ensure uniqueness of in-core commit across all the in-core repository instances.

Thanks.