Re: [PATCH 2/3] setup: have the_repository use the_index

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 12, 2017, 20:38 UTC
Message-ID: <xmqqmv891jct.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170712180143.GD65927@google.com>

Brandon Williams <bmwill@google.com> writes:

> For all intents and purposes the index struct that is stored in 'struct
> repository' is an embedded instance, its just stored as a pointer
> instead of being a direct part of the struct itself.

The question really is this. In order to realize the intents and purposes to have an embeded instance, the most natural way to implement it is to actually embed an instance. There must be some advantage of substituting that most natural implementation with a pointer that needs to point at a separately allocated memory; otherwise the implementation chosen is a poor imitation of the real thing. One downside of not actually embedding an instance is that the code needs to do something different from what it has always done to answer "is the index already populated?". In the normal codepath, it would look at istate.initialized but for the index state that is emulatedly-embedded in the repository, it would also have to see if the pointer is NULL.

And I do not see why a pointer to an allocated struct was chosen, and what advantage we wanted to extract from that design decision.

Show 5 quoted lines

> As far as
> submodules are concerned, thats why 'repo_read_index' exists since it
> will allocate the index struct if needed and then populate it with the
> index file associated with that repository.  So the 'struct repository'
> owns that instance of 'struct index_state'.

All of the above merely makes an excuse why you can work around the fact that the index field is a pointer. It does not say why it is better not to embed the real thing at all.

> Since it is a pointer then using a '#define' to replace 'the_index'
> (which is not a pointer) would be a little more challenging.

The above is merely realizing another downside that stems from the earlier design decision that the index field is not a real embedded structure, but is a pointer. It does not explain why it is better to have a pointer to an allocated structure in the first place.

I am not (yet) telling you to fix the design to have a pointer "index" by replacing it with an embedded structure. I may actually do so later, but I am first trying to find out if it is a right design decision with some advantage. If there is some advantage to have it as a pointer to an allocated structure, then perhaps we may even want to do the conversion the other way by declaring the_index is always a pointer to an allocated structure that _could_ be NULL. We can even lose the istate.initialized bit if we did so, as the way to answer "is the index already populated?" in the new world order would be to see if it is NULL.

But if there isn't any advantage, then I _would_ tell you that the design to have it as a pointer in the repository structure _is_ a mistake. But I do not think I was given sufficient information to decide it yet.

Thanks.