Re: [PATCH 14/26] sha1_file: allow prepare_alt_odb to handle arbitrary repositories

From: Brandon Williams <bmwill@google.com>
Date: Feb 14, 2018, 18:08 UTC
Message-ID: <20180214180814.GA139458@google.com>
In-Reply-To: <CACsJy8DmXrO9bxv10DsDpSWxm2BiZwwfhso4=ZRQs_xhrpAgfg@mail.gmail.com>

On 02/14, Duy Nguyen wrote:

Show 61 quoted lines

> On Tue, Feb 13, 2018 at 8:22 AM, Stefan Beller <sbeller@google.com> wrote:
> > Signed-off-by: Stefan Beller <sbeller@google.com>
> > ---
> >  object-store.h |  3 +--
> >  sha1_file.c    | 21 +++++++++++----------
> >  2 files changed, 12 insertions(+), 12 deletions(-)
> >
> > diff --git a/object-store.h b/object-store.h
> > index d96a16edd1..add1d4e27c 100644
> > --- a/object-store.h
> > +++ b/object-store.h
> > @@ -61,7 +61,6 @@ struct packed_git {
> >         char pack_name[FLEX_ARRAY]; /* more */
> >  };
> >
> > -#define prepare_alt_odb(r) prepare_alt_odb_##r()
> > -extern void prepare_alt_odb_the_repository(void);
> > +void prepare_alt_odb(struct repository *r);
> >
> >  #endif /* OBJECT_STORE_H */
> > diff --git a/sha1_file.c b/sha1_file.c
> > index d18ce3aeba..f046d560f8 100644
> > --- a/sha1_file.c
> > +++ b/sha1_file.c
> > @@ -677,21 +677,22 @@ int foreach_alt_odb(alt_odb_fn fn, void *cb)
> >         return r;
> >  }
> >
> > -void prepare_alt_odb_the_repository(void)
> > +void prepare_alt_odb(struct repository *r)
> >  {
> > -       const char *alt;
> > -
> > -       if (the_repository->objects.alt_odb_tail)
> > +       if (r->objects.alt_odb_tail)
> >                 return;
> >
> > -       alt = getenv(ALTERNATE_DB_ENVIRONMENT);
> > +       r->objects.alt_odb_tail = &r->objects.alt_odb_list;
> > +
> > +       if (!r->ignore_env) {
> > +               const char *alt = getenv(ALTERNATE_DB_ENVIRONMENT);
> 
> If one day the majority of git moves to use 'struct repository', then
> ALTERNATE_DB_ENVIRONMENT is always ignored because ignore_env is
> always true. I think if you ignore_env, then you still need to get
> this "alt" from  'struct raw_object_store' (or 'struct repository').
> 
> Since you get lots of getenv() in repo_setup_env(), I think this
> getenv(ALTERNATE_DB_ENVIRONMENT) belongs there too. Then here, if
> ignore_env is true, you read r->objects.alt or something instead of
> doing getenv().
> 
> I really want to kill this getenv() in this code, which is basically
> delayed initialization because we haven't done proper init on
> the_repo. I realize that it cannot be done earlier, when
> prepare_alt_odb() does not even have a  'struct repository *' to work
> with. Would it be ok if I contributed a patch on top of your series to
> basically do repo_init(&the_repo) for all builtin/external commands
> (and fix all the bugs that come with it)? Then we would not need
> ignore_env here anymore.

At some point yes we would definitely want the setup code to fully initialize a repository object (in this case the_repository). And I would even like to change the function signatures of all the builtin commands to take a repository object so that they don't implicitly rely on the_repository at all.

When I introduced struct repository I seem to remember there being a couple things which were different about setup that made it difficult to simply call repo_init() on the_repository. If you can fix whatever those issues with setup were (I can't remember all of them) then that would be great :)

Show 24 quoted lines

> 
> > +               if (!alt)
> > +                       alt = "";
> >
> > -       the_repository->objects.alt_odb_tail =
> > -                       &the_repository->objects.alt_odb_list;
> > -       link_alt_odb_entries(the_repository, alt,
> > -                            PATH_SEP, NULL, 0);
> > +               link_alt_odb_entries(r, alt, PATH_SEP, NULL, 0);
> > +       }
> >
> > -       read_info_alternates(the_repository, get_object_directory(), 0);
> > +       read_info_alternates(r, r->objects.objectdir, 0);
> >  }
> >
> >  /* Returns 1 if we have successfully freshened the file, 0 otherwise. */
> > --
> > 2.16.1.73.ga2c3e9663f.dirty
> >
> 
> 
> 
> -- 
> Duy

-- 
Brandon Williams