config: borrow refcounted referencescmn/config-borrow-entry

This changes the get_entry() method to return a refcounted version of
the config entry, which you have to free when you're done.

This allows us to avoid freeing the memory in which the entry is stored
on a refresh, which may happen at any time for a live config.

For this reason, get_string() has been forbidden on live configs and a
new function get_string_buf() has been added, which stores the string in
a git_buf which the user then owns.

The functions which parse the string value takea advantage of the
borrowing to parse safely and then release the entry.

12 files changed, 165 insertions, 68 deletions

diff --git a/src/attrcache.c b/src/attrcache.c
index 018fa4874..5bc260460 100644
--- a/src/attrcache.c
+++ b/src/attrcache.c
@@ -276,7 +276,7 @@ static int attr_cache__lookup_path(
 {
 	git_buf buf = GIT_BUF_INIT;
 	int error;
-	const git_config_entry *entry = NULL;
+	git_config_entry *entry = NULL;
 
 	*out = NULL;
 
@@ -296,6 +296,7 @@ static int attr_cache__lookup_path(
 	else if (!git_sysdir_find_xdg_file(&buf, fallback))
 		*out = git_buf_detach(&buf);
 
+	git_config_entry_free(entry);
 	git_buf_free(&buf);
 
 	return error;
diff --git a/src/branch.c b/src/branch.c
index 4e9460f36..06602c525 100644
--- a/src/branch.c
+++ b/src/branch.c
@@ -301,7 +301,7 @@ int git_branch_name(
 }
 
 static int retrieve_upstream_configuration(
-	const char **out,
+	git_buf *out,
 	const git_config *config,
 	const char *canonical_branch_name,
 	const char *format)
@@ -313,7 +313,7 @@ static int retrieve_upstream_configuration(
 		canonical_branch_name + strlen(GIT_REFS_HEADS_DIR)) < 0)
 			return -1;
 
-	error = git_config_get_string(out, config, git_buf_cstr(&buf));
+	error = git_config_get_string_buf(out, config, git_buf_cstr(&buf));
 	git_buf_free(&buf);
 	return error;
 }
@@ -323,7 +323,8 @@ int git_branch_upstream_name(
 	git_repository *repo,
 	const char *refname)
 {
-	const char *remote_name, *merge_name;
+	git_buf remote_name = GIT_BUF_INIT;
+	git_buf merge_name = GIT_BUF_INIT;
 	git_buf buf = GIT_BUF_INIT;
 	int error = -1;
 	git_remote *remote = NULL;
@@ -348,27 +349,27 @@ int git_branch_upstream_name(
 		&merge_name, config, refname, "branch.%s.merge")) < 0)
 			goto cleanup;
 
-	if (!*remote_name || !*merge_name) {
+	if (git_buf_len(&remote_name) == 0 || git_buf_len(&merge_name) == 0) {
 		giterr_set(GITERR_REFERENCE,
 			"branch '%s' does not have an upstream", refname);
 		error = GIT_ENOTFOUND;
 		goto cleanup;
 	}
 
-	if (strcmp(".", remote_name) != 0) {
-		if ((error = git_remote_lookup(&remote, repo, remote_name)) < 0)
+	if (strcmp(".", git_buf_cstr(&remote_name)) != 0) {
+		if ((error = git_remote_lookup(&remote, repo, git_buf_cstr(&remote_name))) < 0)
 			goto cleanup;
 
-		refspec = git_remote__matching_refspec(remote, merge_name);
+		refspec = git_remote__matching_refspec(remote, git_buf_cstr(&merge_name));
 		if (!refspec) {
 			error = GIT_ENOTFOUND;
 			goto cleanup;
 		}
 
-		if (git_refspec_transform(&buf, refspec, merge_name) < 0)
+		if (git_refspec_transform(&buf, refspec, git_buf_cstr(&merge_name)) < 0)
 			goto cleanup;
 	} else
-		if (git_buf_sets(&buf, merge_name) < 0)
+		if (git_buf_set(&buf, git_buf_cstr(&merge_name), git_buf_len(&merge_name)) < 0)
 			goto cleanup;
 
 	error = git_buf_set(out, git_buf_cstr(&buf), git_buf_len(&buf));
@@ -376,6 +377,8 @@ int git_branch_upstream_name(
 cleanup:
 	git_config_free(config);
 	git_remote_free(remote);
+	git_buf_free(&remote_name);
+	git_buf_free(&merge_name);
 	git_buf_free(&buf);
 	return error;
 }
@@ -383,29 +386,25 @@ cleanup:
 int git_branch_upstream_remote(git_buf *buf, git_repository *repo, const char *refname)
 {
 	int error;
-	const char *str;
 	git_config *cfg;
 
 	if (!git_reference__is_branch(refname))
 		return not_a_local_branch(refname);
 
-	git_buf_sanitize(buf);
-	if ((error = git_repository_config_snapshot(&cfg, repo)) < 0)
+	if ((error = git_repository_config__weakptr(&cfg, repo)) < 0)
 		return error;
 
-	if ((error = retrieve_upstream_configuration(&str, cfg, refname, "branch.%s.remote")) < 0)
-		goto cleanup;
+	git_buf_sanitize(buf);
 
-	if (!*str) {
+	if ((error = retrieve_upstream_configuration(buf, cfg, refname, "branch.%s.remote")) < 0)
+		return error;
+
+	if (git_buf_len(buf) == 0) {
 		giterr_set(GITERR_REFERENCE, "branch '%s' does not have an upstream remote", refname);
 		error = GIT_ENOTFOUND;
-		goto cleanup;
+		git_buf_clear(buf);
 	}
 
-	error = git_buf_puts(buf, str);
-
-cleanup:
-	git_config_free(cfg);
 	return error;
 }
 
diff --git a/src/checkout.c b/src/checkout.c
index c06928138..b106c110c 100644
--- a/src/checkout.c
+++ b/src/checkout.c
@@ -2390,25 +2390,27 @@ static int checkout_data_init(
 
 	if ((data->opts.checkout_strategy &
 		(GIT_CHECKOUT_CONFLICT_STYLE_MERGE | GIT_CHECKOUT_CONFLICT_STYLE_DIFF3)) == 0) {
-		const char *conflict_style;
+		git_config_entry *conflict_style = NULL;
 		git_config *cfg = NULL;
 
 		if ((error = git_repository_config__weakptr(&cfg, repo)) < 0 ||
-			(error = git_config_get_string(&conflict_style, cfg, "merge.conflictstyle")) < 0 ||
+			(error = git_config_get_entry(&conflict_style, cfg, "merge.conflictstyle")) < 0 ||
 			error == GIT_ENOTFOUND)
 			;
 		else if (error)
 			goto cleanup;
-		else if (strcmp(conflict_style, "merge") == 0)
+		else if (strcmp(conflict_style->value, "merge") == 0)
 			data->opts.checkout_strategy |= GIT_CHECKOUT_CONFLICT_STYLE_MERGE;
-		else if (strcmp(conflict_style, "diff3") == 0)
+		else if (strcmp(conflict_style->value, "diff3") == 0)
 			data->opts.checkout_strategy |= GIT_CHECKOUT_CONFLICT_STYLE_DIFF3;
 		else {
 			giterr_set(GITERR_CHECKOUT, "unknown style '%s' given for 'merge.conflictstyle'",
 				conflict_style);
 			error = -1;
+			git_config_entry_free(conflict_style);
 			goto cleanup;
 		}
+		git_config_entry_free(conflict_style);
 	}
 
 	if ((error = git_vector_init(&data->removes, 0, git__strcmp_cb)) < 0 ||
diff --git a/src/config.c b/src/config.c
index f80770138..d116a9d80 100644
--- a/src/config.c
+++ b/src/config.c
@@ -19,6 +19,14 @@
 
 #include <ctype.h>
 
+void git_config_entry_free(git_config_entry *entry)
+{
+	if (!entry)
+		return;
+
+	entry->free(entry);
+}
+
 typedef struct {
 	git_refcount rc;
 
@@ -638,7 +646,7 @@ int git_config__update_entry(
 	bool only_if_existing)
 {
 	int error = 0;
-	const git_config_entry *ce = NULL;
+	git_config_entry *ce = NULL;
 
 	if ((error = git_config__lookup_entry(&ce, config, key, false)) < 0)
 		return error;
@@ -657,6 +665,7 @@ int git_config__update_entry(
 	else
 		error = git_config_set_string(config, key, value);
 
+	git_config_entry_free(ce);
 	return error;
 }
 
@@ -677,7 +686,7 @@ enum {
 };
 
 static int get_entry(
-	const git_config_entry **out,
+	git_config_entry **out,
 	const git_config *cfg,
 	const char *name,
 	bool normalize_name,
@@ -721,13 +730,13 @@ cleanup:
 }
 
 int git_config_get_entry(
-	const git_config_entry **out, const git_config *cfg, const char *name)
+	git_config_entry **out, const git_config *cfg, const char *name)
 {
 	return get_entry(out, cfg, name, true, GET_ALL_ERRORS);
 }
 
 int git_config__lookup_entry(
-	const git_config_entry **out,
+	git_config_entry **out,
 	const git_config *cfg,
 	const char *key,
 	bool no_errors)
@@ -743,87 +752,154 @@ int git_config_get_mapped(
 	const git_cvar_map *maps,
 	size_t map_n)
 {
-	const git_config_entry *entry;
+	git_config_entry *entry;
 	int ret;
 
 	if ((ret = get_entry(&entry, cfg, name, true, GET_ALL_ERRORS)) < 0)
 		return ret;
 
-	return git_config_lookup_map_value(out, maps, map_n, entry->value);
+	ret = git_config_lookup_map_value(out, maps, map_n, entry->value);
+	git_config_entry_free(entry);
+
+	return ret;
 }
 
 int git_config_get_int64(int64_t *out, const git_config *cfg, const char *name)
 {
-	const git_config_entry *entry;
+	git_config_entry *entry;
 	int ret;
 
 	if ((ret = get_entry(&entry, cfg, name, true, GET_ALL_ERRORS)) < 0)
 		return ret;
 
-	return git_config_parse_int64(out, entry->value);
+	ret = git_config_parse_int64(out, entry->value);
+	git_config_entry_free(entry);
+
+	return ret;
 }
 
 int git_config_get_int32(int32_t *out, const git_config *cfg, const char *name)
 {
-	const git_config_entry *entry;
+	git_config_entry *entry;
 	int ret;
 
 	if ((ret = get_entry(&entry, cfg, name, true, GET_ALL_ERRORS)) < 0)
 		return ret;
 
-	return git_config_parse_int32(out, entry->value);
+	ret = git_config_parse_int32(out, entry->value);
+	git_config_entry_free(entry);
+
+	return ret;
 }
 
 int git_config_get_bool(int *out, const git_config *cfg, const char *name)
 {
-	const git_config_entry *entry;
+	git_config_entry *entry;
 	int ret;
 
 	if ((ret = get_entry(&entry, cfg, name, true, GET_ALL_ERRORS)) < 0)
 		return ret;
 
-	return git_config_parse_bool(out, entry->value);
+	ret = git_config_parse_bool(out, entry->value);
+	git_config_entry_free(entry);
+
+	return ret;
+}
+
+static int is_readonly(const git_config *cfg)
+{
+	size_t i;
+	file_internal *internal;
+
+	git_vector_foreach(&cfg->files, i, internal) {
+		if (!internal || !internal->file)
+			continue;
+
+		if (!internal->file->readonly)
+			return 0;
+	}
+
+	return 1;
 }
 
 int git_config_get_path(git_buf *out, const git_config *cfg, const char *name)
 {
-	const git_config_entry *entry;
+	git_config_entry *entry;
 	int error;
 
 	if ((error = get_entry(&entry, cfg, name, true, GET_ALL_ERRORS)) < 0)
 		return error;
 
-	return git_config_parse_path(out, entry->value);
+	 error = git_config_parse_path(out, entry->value);
+	 git_config_entry_free(entry);
+
+	 return error;
 }
 
 int git_config_get_string(
 	const char **out, const git_config *cfg, const char *name)
 {
-	const git_config_entry *entry;
-	int ret = get_entry(&entry, cfg, name, true, GET_ALL_ERRORS);
+	git_config_entry *entry;
+	int ret;
+
+	if (!is_readonly(cfg)) {
+		giterr_set(GITERR_CONFIG, "get_string called on a live config object");
+		return -1;
+	}
+
+	ret = get_entry(&entry, cfg, name, true, GET_ALL_ERRORS);
 	*out = !ret ? (entry->value ? entry->value : "") : NULL;
+
+	git_config_entry_free(entry);
+
 	return ret;
 }
 
-const char *git_config__get_string_force(
+int git_config_get_string_buf(
+	git_buf *out, const git_config *cfg, const char *name)
+{
+	git_config_entry *entry;
+	int ret;
+	const char *str;
+
+	git_buf_sanitize(out);
+
+	ret  = get_entry(&entry, cfg, name, true, GET_ALL_ERRORS);
+	str = !ret ? (entry->value ? entry->value : "") : NULL;
+
+	if (str)
+		ret = git_buf_puts(out, str);
+
+	git_config_entry_free(entry);
+
+	return ret;
+}
+
+char *git_config__get_string_force(
 	const git_config *cfg, const char *key, const char *fallback_value)
 {
-	const git_config_entry *entry;
+	git_config_entry *entry;
+	char *ret;
+
 	get_entry(&entry, cfg, key, false, GET_NO_ERRORS);
-	return (entry && entry->value) ? entry->value : fallback_value;
+	ret = (entry && entry->value) ? git__strdup(entry->value) : fallback_value ? git__strdup(fallback_value) : NULL;
+	git_config_entry_free(entry);
+
+	return ret;
 }
 
 int git_config__get_bool_force(
 	const git_config *cfg, const char *key, int fallback_value)
 {
 	int val = fallback_value;
-	const git_config_entry *entry;
+	git_config_entry *entry;
 
 	get_entry(&entry, cfg, key, false, GET_NO_ERRORS);
 
 	if (entry && git_config_parse_bool(&val, entry->value) < 0)
 		giterr_clear();
 
+	git_config_entry_free(entry);
 	return val;
 }
 
@@ -831,13 +907,14 @@ int git_config__get_int_force(
 	const git_config *cfg, const char *key, int fallback_value)
 {
 	int32_t val = (int32_t)fallback_value;
-	const git_config_entry *entry;
+	git_config_entry *entry;
 
 	get_entry(&entry, cfg, key, false, GET_NO_ERRORS);
 
 	if (entry && git_config_parse_int32(&val, entry->value) < 0)
 		giterr_clear();
 
+	git_config_entry_free(entry);
 	return (int)val;
 }
 
diff --git a/src/config.h b/src/config.h
index b0dcb49ac..691868b1d 100644
--- a/src/config.h
+++ b/src/config.h
@@ -48,7 +48,7 @@ extern int git_config__normalize_name(const char *in, char **out);
 
 /* internal only: does not normalize key and sets out to NULL if not found */
 extern int git_config__lookup_entry(
-	const git_config_entry **out,
+	git_config_entry **out,
 	const git_config *cfg,
 	const char *key,
 	bool no_errors);
@@ -67,7 +67,7 @@ extern int git_config__update_entry(
  * failures occur while trying to access the value.
  */
 
-extern const char *git_config__get_string_force(
+extern char *git_config__get_string_force(
 	const git_config *cfg, const char *key, const char *fallback_value);
 
 extern int git_config__get_bool_force(
diff --git a/src/config_cache.c b/src/config_cache.c
index d397a4bab..c859ec148 100644
--- a/src/config_cache.c
+++ b/src/config_cache.c
@@ -84,7 +84,7 @@ int git_config__cvar(int *out, git_config *config, git_cvar_cached cvar)
 {
 	int error = 0;
 	struct map_data *data = &_cvar_maps[(int)cvar];
-	const git_config_entry *entry;
+	git_config_entry *entry;
 
 	git_config__lookup_entry(&entry, config, data->cvar_name, false);
 
@@ -96,6 +96,7 @@ int git_config__cvar(int *out, git_config *config, git_cvar_cached cvar)
 	else
 		error = git_config_parse_bool(out, entry->value);
 
+	git_config_entry_free(entry);
 	return error;
 }
 
diff --git a/src/config_file.c b/src/config_file.c
index 168dd5483..732705687 100644
--- a/src/config_file.c
+++ b/src/config_file.c
@@ -96,7 +96,6 @@ typedef struct {
 	/* mutex to coordinate accessing the values */
 	git_mutex values_mutex;
 	refcounted_strmap *values;
-	int readonly;
 } diskfile_header;
 
 typedef struct {
@@ -504,19 +503,26 @@ out:
 	return ret;
 }
 
+/* release the map containing the entry as an equivalent to freeing it */
+static void release_map(git_config_entry *entry)
+{
+	refcounted_strmap *map = (refcounted_strmap *) entry->payload;
+	refcounted_strmap_free(map);
+}
+
 /*
  * Internal function that actually gets the value in string form
  */
-static int config_get(git_config_backend *cfg, const char *key, const git_config_entry **out)
+static int config_get(git_config_backend *cfg, const char *key, git_config_entry **out)
 {
 	diskfile_header *h = (diskfile_header *)cfg;
 	refcounted_strmap *map;
 	git_strmap *values;
 	khiter_t pos;
 	cvar_t *var;
-	int error;
+	int error = 0;
 
-	if (!h->readonly && ((error = config_refresh(cfg)) < 0))
+	if (!h->parent.readonly && ((error = config_refresh(cfg)) < 0))
 		return error;
 
 	map = refcounted_strmap_take(h);
@@ -534,9 +540,11 @@ static int config_get(git_config_backend *cfg, const char *key, const git_config
 	while (var->next)
 		var = var->next;
 
-	refcounted_strmap_free(map);
 	*out = var->entry;
-	return 0;
+	(*out)->free = release_map;
+	(*out)->payload = map;
+
+	return error;
 }
 
 static int config_set_multivar(
@@ -763,7 +771,7 @@ static int config_readonly_open(git_config_backend *cfg, git_config_level_t leve
 	refcounted_strmap *src_map;
 	int error;
 
-	if (!src_header->readonly && (error = config_refresh(&src_header->parent)) < 0)
+	if (!src_header->parent.readonly && (error = config_refresh(&src_header->parent)) < 0)
 		return error;
 
 	/* We're just copying data, don't care about the level */
@@ -787,7 +795,7 @@ int git_config_file__snapshot(git_config_backend **out, diskfile_backend *in)
 
 	backend->snapshot_from = in;
 
-	backend->header.readonly = 1;
+	backend->header.parent.readonly = 1;
 	backend->header.parent.version = GIT_CONFIG_BACKEND_VERSION;
 	backend->header.parent.open = config_readonly_open;
 	backend->header.parent.get = config_get;
diff --git a/src/config_file.h b/src/config_file.h
index fcccbd5cc..0d8bf740f 100644
--- a/src/config_file.h
+++ b/src/config_file.h
@@ -21,7 +21,7 @@ GIT_INLINE(void) git_config_file_free(git_config_backend *cfg)
 }
 
 GIT_INLINE(int) git_config_file_get_string(
-	const git_config_entry **out, git_config_backend *cfg, const char *name)
+	git_config_entry **out, git_config_backend *cfg, const char *name)
 {
 	return cfg->get(cfg, name, out);
 }
diff --git a/src/diff.c b/src/diff.c
index 815351b21..9432b0467 100644
--- a/src/diff.c
+++ b/src/diff.c
@@ -461,12 +461,13 @@ static int diff_list_apply_options(
 
 	/* if ignore_submodules not explicitly set, check diff config */
 	if (diff->opts.ignore_submodules <= 0) {
-		const git_config_entry *entry;
+		 git_config_entry *entry;
 		git_config__lookup_entry(&entry, cfg, "diff.ignoresubmodules", true);
 
 		if (entry && git_submodule_parse_ignore(
 				&diff->opts.ignore_submodules, entry->value) < 0)
 			giterr_clear();
+		git_config_entry_free(entry);
 	}
 
 	/* if either prefix is not set, figure out appropriate value */
diff --git a/src/diff_driver.c b/src/diff_driver.c
index 049e6ef2a..69eef0f7a 100644
--- a/src/diff_driver.c
+++ b/src/diff_driver.c
@@ -242,7 +242,7 @@ static int git_diff_driver_load(
 	khiter_t pos;
 	git_config *cfg = NULL;
 	git_buf name = GIT_BUF_INIT;
-	const git_config_entry *ce;
+	git_config_entry *ce = NULL;
 	bool found_driver = false;
 
 	if ((reg = git_repository_driver_registry(repo)) == NULL)
@@ -341,6 +341,7 @@ static int git_diff_driver_load(
 	*out = drv;
 
 done:
+	git_config_entry_free(ce);
 	git_buf_free(&name);
 	git_config_free(cfg);
 
diff --git a/src/remote.c b/src/remote.c
index bc6d8a2c4..4924bf83a 100644
--- a/src/remote.c
+++ b/src/remote.c
@@ -52,7 +52,7 @@ static int add_refspec(git_remote *remote, const char *string, bool is_fetch)
 
 static int download_tags_value(git_remote *remote, git_config *cfg)
 {
-	const git_config_entry *ce;
+	git_config_entry *ce;
 	git_buf buf = GIT_BUF_INIT;
 	int error;
 
@@ -70,6 +70,7 @@ static int download_tags_value(git_remote *remote, git_config *cfg)
 			remote->download_tags = GIT_REMOTE_DOWNLOAD_TAGS_ALL;
 	}
 
+	git_config_entry_free(ce);
 	return error;
 }
 
@@ -548,7 +549,7 @@ int git_remote_save(const git_remote *remote)
 	git_config *cfg;
 	const char *tagopt = NULL;
 	git_buf buf = GIT_BUF_INIT;
-	const git_config_entry *existing;
+	git_config_entry *existing = NULL;
 
 	assert(remote);
 
@@ -618,6 +619,7 @@ int git_remote_save(const git_remote *remote)
 		cfg, git_buf_cstr(&buf), tagopt, true, false);
 
 cleanup:
+	git_config_entry_free(existing);
 	git_buf_free(&buf);
 	return error;
 }
@@ -753,7 +755,7 @@ int git_remote_ls(const git_remote_head ***out, size_t *size, git_remote *remote
 int git_remote__get_http_proxy(git_remote *remote, bool use_ssl, char **proxy_url)
 {
 	git_config *cfg;
-	const git_config_entry *ce;
+	git_config_entry *ce = NULL;
 	const char *val = NULL;
 	int error;
 
@@ -805,6 +807,7 @@ found:
 		*proxy_url = git__strdup(val);
 		GITERR_CHECK_ALLOC(*proxy_url);
 	}
+	git_config_entry_free(ce);
 
 	return 0;
 }
diff --git a/src/repository.c b/src/repository.c
index 778cdefd7..43d3aeaea 100644
--- a/src/repository.c
+++ b/src/repository.c
@@ -191,7 +191,7 @@ static int load_config_data(git_repository *repo, const git_config *config)
 static int load_workdir(git_repository *repo, git_config *config, git_buf *parent_path)
 {
 	int         error;
-	const git_config_entry *ce;
+	git_config_entry *ce;
 	git_buf     worktree = GIT_BUF_INIT;
 
 	if (repo->is_bare)
@@ -204,7 +204,7 @@ static int load_workdir(git_repository *repo, git_config *config, git_buf *paren
 	if (ce && ce->value) {
 		if ((error = git_path_prettify_dir(
 				&worktree, ce->value, repo->path_repository)) < 0)
-			return error;
+			goto cleanup;
 
 		repo->workdir = git_buf_detach(&worktree);
 	}
@@ -212,14 +212,18 @@ static int load_workdir(git_repository *repo, git_config *config, git_buf *paren
 		repo->workdir = git_buf_detach(parent_path);
 	else {
 		if (git_path_dirname_r(&worktree, repo->path_repository) < 0 ||
-			git_path_to_dir(&worktree) < 0)
-			return -1;
+		    git_path_to_dir(&worktree) < 0) {
+			error = -1;
+			goto cleanup;
+		}
 
 		repo->workdir = git_buf_detach(&worktree);
 	}
 
 	GITERR_CHECK_ALLOC(repo->workdir);
-	return 0;
+cleanup:
+	git_config_entry_free(ce);
+	return error;
 }
 
 /*