Subject: Re: [PATCH v4 15/20] repository: add index_state to struct repo

From: Brandon Williams <[email-masked]>
Date: Thu, 22 Jun 2017 13:35:11 -0700

On 06/22, Junio C Hamano wrote:
> Brandon Williams <[email-masked]> writes:
> 
> >  struct repository {
> >  	/* Environment */
> > @@ -49,6 +50,12 @@ struct repository {
> >  	 */
> >  	struct config_set *config;
> >  
> > +	/*
> > +	 * Repository's in-memory index.
> > +	 * 'repo_read_index()' can be used to populate 'index'.
> > +	 */
> > +	struct index_state *index;
> > +
> >  	/* Configurations */
> >  	/*
> >  	 * Bit used during initialization to indicate if repository state (like
> > @@ -71,4 +78,6 @@ extern void repo_set_worktree(struct repository *repo, const char *path);
> >  extern int repo_init(struct repository *repo, const char *gitdir, const char *worktree);
> >  extern void repo_clear(struct repository *repo);
> >  
> > +extern int repo_read_index(struct repository *repo);
> > +
> >  #endif /* REPOSITORY_H */
> 
> While you are working on a simple read-only operation like
> "ls-files", you can get away with just a singleton (the equivalent
> to "the_index" in the repository object world) in-core index
> instance, and having a way to tell the system to read things into
> the default thing without having to name what that default thing is
> is a very useful thing to have.  It is a good thing that this only
> needs "struct repository *" and no "struct index_state *" parameter.
> 
> But you will need a way to read, update, write it out as a tree,
> etc. to a non-default in-core index, once you start doing more
> complex things.  The function signature of the lowest level
> primitive helper for them will be:
> 
>     extern int (*)(struct repository *, struct index_state *);
> 
> And you would want to reserve "index" suffix for such a function,
> following the "if you use the default in-core thing, you do not have
> to pass it as a parameter---just call _cache() convenience macro;
> but you can explicitly pass it to the underlying _index() function"
> convention we established long time ago.
> 
> So I'd suggest renaming the above one that uses the default in-core
> index instance to
> 
> 	extern int repo_read_cache(struct repository *);
> 
> or you will regret later.
> 

That makes sense.  While at it, would it make sense to ensure that the
'struct index_state *' which is stored in 'the_repository.index' be
'&the_index'?

-- 
Brandon Williams