On 06/20, Jonathan Tan wrote:
> On Tue, 20 Jun 2017 12:19:35 -0700
> Brandon Williams <bmw...@google.com> 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
