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

From: Brandon Williams <bmwill@google.com>
Date: Jul 12, 2017, 18:01 UTC
Message-ID: <20170712180143.GD65927@google.com>
In-Reply-To: <xmqqzica1pli.fsf@gitster.mtv.corp.google.com>

On 07/11, Junio C Hamano wrote:

Show 38 quoted lines

> Brandon Williams <bmwill@google.com> writes:
> 
> > Have the index state which is stored in 'the_repository' be a pointer to
> > the in-core instead 'the_index'.  This makes it easier to begin
> > transitioning more parts of the code base to operate on a 'struct
> > repository'.
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> >  setup.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/setup.c b/setup.c
> > index 860507e1f..b370bf3c1 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -1123,6 +1123,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
> >  			setup_git_env();
> >  		}
> >  	}
> > +	the_repository->index = &the_index;
> >  
> >  	strbuf_release(&dir);
> >  	strbuf_release(&gitdir);
> 
> I would have expected this to be going in the different direction,
> i.e. there is an embedded instance of index_state in a repository
> object, and the_repository.index is defined to be the old the_index,
> i.e.
> 
> 	#define the_index (the_repository.index)
> 
> When a Git command that recurses into submodules in-core using
> the_repository that represents the top-level superproject and
> another repository object tht represents a submodule, don't we want
> the repository object for the submodule also have its own default
> index without having to allocate one and point at it with the index
> field?

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

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

Show 10 quoted lines

> 
> I dunno.  Being able to leave the index field NULL lets you say
> "this is a bare repository and there is no place for the index file
> for it", but even if we never write out the in-core index to an
> index file on disk, being able to populate the in-core index that is
> default for the repository object from a tree-ish and iterating over
> it (e.g.  running in-core merge of trees) is a useful thing to do,
> so I do not consider "index field can be set to NULL to signify a
> bare repository" a very strong plus.
>

I'd probably say that we'd eventually need a bit field in 'struct repository' which indicates if a repository is bare. I think we already have a global variable somewhere which stores this sort of information.

-- 
Brandon Williams