Re: [PATCH 06/31] repo: introduce the repository object

[Brandon Williams <bmwill@google.com>]

On 06/01, Stefan Beller wrote:
> On Wed, May 31, 2017 at 2:43 PM, Brandon Williams <bmwill@google.com> wrote:
> > Introduce the repository object 'struct repo' which can be used hold all
> > state pertaining to a git repository.
> >
> > The aim of object-ifying the repository is to (1) make the code base
> > more readable and easier to reason about and (2) allow for working on
> > multiple repositories, specifically submodules, within the same process.
> >
> > TODO: Add more motivating points for adding a repository object?
> 
> Yes please (or delete this line).
> https://public-inbox.org/git/alpine.DEB.2.21.1.1705221501540.3610@virtualbox/
> 
> > +++ b/repo.c
> > @@ -0,0 +1,124 @@
> > +#include "cache.h"
> > +#include "repo.h"
> > +
> > +/*
> > + * This may be the wrong place for this.
> > + * It may be better to go in env.c or setup for the time being?
> 
> In env.c we say:
> /*
>  * We put all the git config variables in this same object
>  * file, so that programs can link against the config parser
>  * without having to link against all the rest of git.
>  *
>  * In particular, no need to bring in libz etc unless needed,
>  * even if you might want to know where the git directory etc
>  * are.
>  */
> 
> And setup.c only has a few variables that matter there locally.
> So I would think having 'the_repository' in repo.c is acceptable.

And perhaps (far down the road) 'the_repoository' could be removed such
that builtin commands take a pointer to a repo object as a parameter.

> 
> > + */
> > +struct repo the_repository;
> > +
> > +static char *git_path_from_env(const char *envvar, const char *git_dir,
> > +                              const char *path, int fromenv)
> > +{
> > +       if (fromenv) {
> > +               const char *value = getenv(envvar);
> > +               if (value)
> > +                       return xstrdup(value);
> > +       }
> > +
> > +       return xstrfmt("%s/%s", git_dir, path);
> > +}
> > +
> > +static int find_common_dir(struct strbuf *sb, const char *gitdir, int fromenv)
> > +{
> > +       if (fromenv) {
> > +               const char *value = getenv(GIT_COMMON_DIR_ENVIRONMENT);
> > +               if (value) {
> > +                       strbuf_addstr(sb, value);
> > +                       return 1;
> > +               }
> > +       }
> > +
> > +       return get_common_dir_noenv(sb, gitdir);
> > +}
> > +
> > +/* called after setting gitdir */
> > +static void repo_setup_env(struct repo *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->index_file = git_path_from_env(INDEX_ENVIRONMENT, repo->gitdir,
> > +                                            "index", !repo->ignore_env);
> > +       repo->graft_file = git_path_from_env(GRAFT_ENVIRONMENT, repo->commondir,
> > +                                            "info/grafts", !repo->ignore_env);
> > +       repo->namespace = expand_namespace(repo->ignore_env ? NULL :
> > +                                          getenv(GIT_NAMESPACE_ENVIRONMENT));
> > +}
> > +
> > +static void repo_clear_env(struct repo *repo)
> > +{
> > +       free(repo->gitdir);
> > +       repo->gitdir = NULL;
> > +       free(repo->commondir);
> > +       repo->commondir = NULL;
> > +       free(repo->objectdir);
> > +       repo->objectdir = NULL;
> > +       free(repo->index_file);
> > +       repo->index_file = NULL;
> > +       free(repo->graft_file);
> > +       repo->graft_file = NULL;
> > +       free(repo->namespace);
> > +       repo->namespace = NULL;
> 
> I wonder if we can defer the NULL assignments to
> repo_clear, where we would just do a
> memset(repo, 0, sizeof(struct repo));
> 

Yeah perhaps, clearing the env should either be done when setting gitdir
again (so setting up the env will happen again and we don't need to
clear the fields) or clearing the struct as a whole so using memset
would work.

> > +
> > +       repo_set_gitdir(repo, resolved_gitdir);
> > +
> > +       /* NEEDSWORK: Verify repository format version */
> 
> Care to elaborate on this? I do not understand why we would want
> to check the format version here?

When opening up a repository git needs to check if it understands the
repository format and all extensions.  If it doesn't git needs to bail
out and not operate on the repository.  So a part of initializing a repo
object would be to verify that it understands the repository format
version.

> 
> > +
> > +extern void repo_set_gitdir(struct repo *repo, const char *path);
> > +extern int repo_init(struct repo *repo, const char *path);
> > +extern void repo_clear(struct repo *repo);
> 
> The init and clear method seem obvious to me, but what do we need the
> repo_set_gitdir for externally? I would assume the repo auto-discovers its
> gitdir on its own?

Well I didn't completely overhaul the setup code in this series so I
really just hooked into where the setup code already sets the gitdir,
hence why this function is exposed atm.

-- 
Brandon Williams