Re: [PATCH v6 28/32] config: read ref storage config on startup

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:

quoted

@@ -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.