On Fri, 2016-02-26 at 22:56 -0500, Jeff King wrote:
> On Wed, Feb 24, 2016 at 05:59:00PM -0500, David Turner wrote:
>
> > @@ -1207,6 +1208,29 @@ int git_config_early(config_fn_t fn, void
> > *data, const char *repo_config)
> > }
> >
> > if (repo_config && !access_or_die(repo_config, R_OK, 0)) {
> > + char *storage = NULL;
> > +
> > + /*
> > + * make sure we always read the ref storage config
> > + * from the extensions section on startup
> > + */
> > + ret +=
> > git_config_from_file(ref_storage_backend_config,
> > + repo_config,
> > &storage);
> > +
> > + register_ref_storage_backends();
> > + if (!storage)
> > + storage = xstrdup("");
> > +
> > + if (!*storage ||
> > + !strcmp(storage, "files")) {
> > + /* default backend, nothing to do */
> > + free(storage);
> > + } else {
> > + if
> > (set_ref_storage_backend(ref_storage_backend))
> > + die(_("Unknown ref storage backend
> > %s"),
> > + ref_storage_backend);
> > + }
> > +
>
> Coverity complains that "storage" leaks here, and I think it does in
> the
> case that we non-default storage, and we successfully set up the
> backend. That's a pretty minor point.
>
> However, after looking at this code, I'm rather confused about a few
> things.
>
> One is that we read the config and use ref_storage_backend_config[1]
> to
> store the string value in "storage", and then we check whether that
> is
> non-default, and if it is, I'd expect us to feed it to
> set_ref_storage_backend. But we don't; we feed ref_storage_backend
> instead!
>
> What is that value? It looks like it is the string we set in
> check_repo_format when we load the extensions list there. So why
> are we re-reading the config here at all? Couldn't we just use
> ref_storage_backend in the first place?
>
> My second confusion is why this is happening in git_config_early().
> That
> function is called during the setup of
> check_repository_format_gently(),
> which is why I think you wanted to put the code here. But it's _also_
> called as part of a regular git_config(). Which means we're parsing
> the
> repo config and setting the ref backend all over again, every time we
> look at config for other reasons.
>
> So I think this setup probably should be in
> check_repository_format_gently(), and should be able to trigger off
> of
> the existing ref_storage_backend string we've already saved (and we
> should bail immediately there if we don't know about the backend, as
> it
> means we _don't_ match the repo's extensions and cannot proceed).
We apparently don't always call check_repo_format before calling
git_config_early -- or, more to the point, before doing ref operations.
So I think we need this in git_config_early.
I'll fix up the memory safety/leak issues tho.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html