Subject: Re: [PATCH v3 04/20] repository: introduce the repository object

From: Brandon Williams <[email-masked]>
Date: Tue, 20 Jun 2017 14:56:10 -0700

On 06/20, Jonathan Tan wrote:
> On Tue, 20 Jun 2017 12:19:35 -0700
> Brandon Williams <[email-masked]> wrote:
> 
> > Introduce the repository object 'struct repository' which can be used to
> > hold all state pertaining to a git repository.
> > 
> > Some of the benefits of object-ifying a repository are:
> > 
> >   1. Make the code base more readable and easier to reason about.
> > 
> >   2. Allow for working on multiple repositories, specifically
> >      submodules, within the same process.  Currently the process for
> >      working on a submodule involves setting up an argv_array of options
> >      for a particular command and then launching a child process to
> >      execute the command in the context of the submodule.  This is
> >      clunky and can require lots of little hacks in order to ensure
> >      correctness.  Ideally it would be nice to simply pass a repository
> >      and an options struct to a command.
> > 
> >   3. Eliminating reliance on global state will make it easier to
> >      enable the use of threading to improve performance.
> 
> These would indeed be nice to have.
> 
> > +/* called after setting gitdir */
> > +static void repo_setup_env(struct repository *repo)
> > +{
> > +	struct strbuf sb = STRBUF_INIT;
> > +
> > +	if (!repo->gitdir)
> > +		BUG("gitdir wasn't set before setting up the environment");
> > +
> > +	repo->different_commondir = find_common_dir(&sb, repo->gitdir,
> > +						    !repo->ignore_env);
> > +	repo->commondir = strbuf_detach(&sb, NULL);
> > +	repo->objectdir = git_path_from_env(DB_ENVIRONMENT, repo->commondir,
> > +					    "objects", !repo->ignore_env);
> > +	repo->graft_file = git_path_from_env(GRAFT_ENVIRONMENT, repo->commondir,
> > +					     "info/grafts", !repo->ignore_env);
> > +	repo->index_file = git_path_from_env(INDEX_ENVIRONMENT, repo->gitdir,
> > +					     "index", !repo->ignore_env);
> > +}
> 
> It seems that this function is only used once in repo_set_gitdir() -
> could this be inlined there instead? Then you wouldn't need the comment
> and the !repo->gitdir check.

I'd like to keep them separate as it makes things a little clearer in my
opinion, smaller functions and the like.

> 
> > +static int verify_repo_format(struct repository_format *format,
> > +			      const char *commondir)
> > +{
> > +	int ret = 0;
> > +	struct strbuf sb = STRBUF_INIT;
> > +
> > +	strbuf_addf(&sb, "%s/config", commondir);
> > +	read_repository_format(format, sb.buf);
> > +	strbuf_reset(&sb);
> > +
> > +	if (verify_repository_format(format, &sb) < 0) {
> > +		warning("%s", sb.buf);
> > +		ret = -1;
> > +	}
> > +
> > +	strbuf_release(&sb);
> > +	return ret;
> > +}
> 
> This function is confusingly named - firstly, there is already a
> verify_repository_format(), and secondly, this function both reads and
> verifies it.

This one is static to the file, and i don't think its named confusingly
as it does just what it says it does. I'm trying to limit how much other
code I change so I had to make this function.

> 
> > +void repo_clear(struct repository *repo)
> > +{
> > +	free(repo->gitdir);
> > +	repo->gitdir = NULL;
> > +	free(repo->commondir);
> > +	repo->commondir = NULL;
> > +	free(repo->objectdir);
> > +	repo->objectdir = NULL;
> > +	free(repo->graft_file);
> > +	repo->graft_file = NULL;
> > +	free(repo->index_file);
> > +	repo->index_file = NULL;
> > +	free(repo->worktree);
> > +	repo->worktree = NULL;
> > +
> > +	memset(repo, 0, sizeof(*repo));
> > +}
> 
> If you're going to memset, you probably don't need to set everything to
> NULL.
> 
> > +	/* Configurations */
> > +	/*
> > +	 * Bit used during initialization to indicate if repository state (like
> > +	 * the location of the 'objectdir') should be read from the
> > +	 * environment.  By default this bit will be set at the begining of
> > +	 * 'repo_init()' so that all repositories will ignore the environment.
> > +	 * The exception to this is 'the_repository', which doesn't go through
> > +	 * the normal 'repo_init()' process.
> > +	 */
> > +	unsigned ignore_env:1;
> 
> If this is only used during initialization, could this be passed as a
> separate parameter internally instead of having it here?

It would feel a little wonky to be a parameter as 'the_repository' is
initialized differently than a repository would normally be.  Theres
been a lot of cleanup to the setup logic but it would need to be cleaned
up even more to have this be a param.

-- 
Brandon Williams