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.
> +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.
> +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?
