Re: [PATCH v4 15/20] repository: add index_state to struct repo
- From
- Brandon Williams <bmwill@google.com>
- Date
- Jun 22, 2017, 20:35 UTC
- Message-ID
- <20170622203511.GB124061@google.com>
- In-Reply-To
- <xmqqshirixrt.fsf@gitster.mtv.corp.google.com>
On 06/22, Junio C Hamano wrote:
Show 53 quoted lines
> Brandon Williams <bmwill@google.com> 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