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

[Junio C Hamano <gitster@pobox.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.

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