Merge pull request #2349 from libgit2/rb/coverity-fixes

Increase config snapshot usage

11 files changed, 93 insertions, 73 deletions

diff --git a/src/attrcache.c b/src/attrcache.c
index f1bc70467..56c028e60 100644
--- a/src/attrcache.c
+++ b/src/attrcache.c
@@ -349,14 +349,11 @@ int git_attr_cache__do_init(git_repository *repo)
 {
 	int ret = 0;
 	git_attr_cache *cache = git_repository_attr_cache(repo);
-	git_config *cfg;
+	git_config *cfg = NULL;
 
 	if (cache)
 		return 0;
 
-	if ((ret = git_repository_config__weakptr(&cfg, repo)) < 0)
-		return ret;
-
 	cache = git__calloc(1, sizeof(git_attr_cache));
 	GITERR_CHECK_ALLOC(cache);
 
@@ -367,6 +364,9 @@ int git_attr_cache__do_init(git_repository *repo)
 		return -1;
 	}
 
+	if ((ret = git_repository_config_snapshot(&cfg, repo)) < 0)
+		goto cancel;
+
 	/* cache config settings for attributes and ignores */
 	ret = attr_cache__lookup_path(
 		&cache->cfg_attr_file, cfg, GIT_ATTR_CONFIG, GIT_ATTR_FILE_XDG);
@@ -390,11 +390,14 @@ int git_attr_cache__do_init(git_repository *repo)
 	if (cache)
 		goto cancel; /* raced with another thread, free this but no error */
 
+	git_config_free(cfg);
+
 	/* insert default macros */
 	return git_attr_add_macro(repo, "binary", "-diff -crlf -text");
 
 cancel:
 	attr_cache__free(cache);
+	git_config_free(cfg);
 	return ret;
 }
 
diff --git a/src/config.c b/src/config.c
index 757ff0387..4bd27a875 100644
--- a/src/config.c
+++ b/src/config.c
@@ -153,19 +153,19 @@ int git_config_snapshot(git_config **out, git_config *in)
 		git_config_backend *b;
 
 		if ((error = internal->file->snapshot(&b, internal->file)) < 0)
-			goto on_error;
+			break;
 
 		if ((error = git_config_add_backend(config, b, internal->level, 0)) < 0) {
 			b->free(b);
-			goto on_error;
+			break;
 		}
 	}
 
-	*out = config;
-	return error;
+	if (error < 0)
+		git_config_free(config);
+	else
+		*out = config;
 
-on_error:
-	git_config_free(config);
 	return error;
 }
 
diff --git a/src/config.h b/src/config.h
index 00b6063e7..b0dcb49ac 100644
--- a/src/config.h
+++ b/src/config.h
@@ -76,4 +76,10 @@ extern int git_config__get_bool_force(
 extern int git_config__get_int_force(
 	const git_config *cfg, const char *key, int fallback_value);
 
+/* API for repository cvar-style lookups from config - not cached, but
+ * uses cvar value maps and fallbacks
+ */
+extern int git_config__cvar(
+	int *out, git_config *config, git_cvar_cached cvar);
+
 #endif
diff --git a/src/config_cache.c b/src/config_cache.c
index 4bcbf02bf..dca9976f8 100644
--- a/src/config_cache.c
+++ b/src/config_cache.c
@@ -7,11 +7,11 @@
 
 #include "common.h"
 #include "fileops.h"
+#include "repository.h"
 #include "config.h"
 #include "git2/config.h"
 #include "vector.h"
 #include "filter.h"
-#include "repository.h"
 
 struct map_data {
 	const char *cvar_name;
@@ -69,32 +69,38 @@ static struct map_data _cvar_maps[] = {
 	{"core.abbrev", _cvar_map_int, 1, GIT_ABBREV_DEFAULT },
 	{"core.precomposeunicode", NULL, 0, GIT_PRECOMPOSE_DEFAULT },
 	{"core.safecrlf", NULL, 0, GIT_SAFE_CRLF_DEFAULT},
+	{"core.logallrefupdates", NULL, 0, GIT_LOGALLREFUPDATES_DEFAULT },
 };
 
+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__lookup_entry(&entry, config, data->cvar_name, false);
+
+	if (!entry)
+		*out = data->default_value;
+	else if (data->maps)
+		error = git_config_lookup_map_value(
+			out, data->maps, data->map_count, entry->value);
+	else
+		error = git_config_parse_bool(out, entry->value);
+
+	return error;
+}
+
 int git_repository__cvar(int *out, git_repository *repo, git_cvar_cached cvar)
 {
 	*out = repo->cvar_cache[(int)cvar];
 
 	if (*out == GIT_CVAR_NOT_CACHED) {
-		struct map_data *data = &_cvar_maps[(int)cvar];
-		git_config *config;
 		int error;
-		const git_config_entry *entry;
-
-		if ((error = git_repository_config__weakptr(&config, repo)) < 0)
-			return error;
-
-		git_config__lookup_entry(&entry, config, data->cvar_name, false);
-
-		if (!entry)
-			*out = data->default_value;
-		else if (data->maps)
-			error = git_config_lookup_map_value(
-				out, data->maps, data->map_count, entry->value);
-		else
-			error = git_config_parse_bool(out, entry->value);
+		git_config *config;
 
-		if (error < 0)
+		if ((error = git_repository_config__weakptr(&config, repo)) < 0 ||
+			(error = git_config__cvar(out, config, cvar)) < 0)
 			return error;
 
 		repo->cvar_cache[(int)cvar] = *out;
diff --git a/src/config_file.c b/src/config_file.c
index b00d0827d..fdc9bc8a2 100644
--- a/src/config_file.c
+++ b/src/config_file.c
@@ -313,6 +313,7 @@ static int config__refresh(git_config_backend *cfg)
 		goto out;
 
 	reader = git_array_get(b->readers, git_array_size(b->readers) - 1);
+	GITERR_CHECK_ALLOC(reader);
 
 	if ((error = config_parse(values->values, b, reader, b->level, 0)) < 0)
 		goto out;
@@ -327,7 +328,8 @@ static int config__refresh(git_config_backend *cfg)
 
 out:
 	refcounted_strmap_free(values);
-	git_buf_free(&reader->buffer);
+	if (reader)
+		git_buf_free(&reader->buffer);
 	return error;
 }
 
@@ -344,8 +346,8 @@ static int config_refresh(git_config_backend *cfg)
 			&reader->buffer, reader->file_path,
 			&reader->file_mtime, &reader->file_size, &updated);
 
-		if (error < 0)
-			return (error == GIT_ENOTFOUND) ? 0 : error;
+		if (error < 0 && error != GIT_ENOTFOUND)
+			return error;
 
 		if (updated)
 			any_updated = 1;
diff --git a/src/diff.c b/src/diff.c
index b3e36101c..325c599e0 100644
--- a/src/diff.c
+++ b/src/diff.c
@@ -381,7 +381,7 @@ static int diff_list_apply_options(
 	git_diff *diff,
 	const git_diff_options *opts)
 {
-	git_config *cfg;
+	git_config *cfg = NULL;
 	git_repository *repo = diff->repo;
 	git_pool *pool = &diff->pool;
 	int val;
@@ -406,20 +406,20 @@ static int diff_list_apply_options(
 		diff->opts.flags |= GIT_DIFF_INCLUDE_UNTRACKED;
 
 	/* load config values that affect diff behavior */
-	if ((val = git_repository_config__weakptr(&cfg, repo)) < 0)
+	if ((val = git_repository_config_snapshot(&cfg, repo)) < 0)
 		return val;
 
-	if (!git_repository__cvar(&val, repo, GIT_CVAR_SYMLINKS) && val)
+	if (!git_config__cvar(&val, cfg, GIT_CVAR_SYMLINKS) && val)
 		diff->diffcaps = diff->diffcaps | GIT_DIFFCAPS_HAS_SYMLINKS;
 
-	if (!git_repository__cvar(&val, repo, GIT_CVAR_IGNORESTAT) && val)
+	if (!git_config__cvar(&val, cfg, GIT_CVAR_IGNORESTAT) && val)
 		diff->diffcaps = diff->diffcaps | GIT_DIFFCAPS_IGNORE_STAT;
 
 	if ((diff->opts.flags & GIT_DIFF_IGNORE_FILEMODE) == 0 &&
-		!git_repository__cvar(&val, repo, GIT_CVAR_FILEMODE) && val)
+		!git_config__cvar(&val, cfg, GIT_CVAR_FILEMODE) && val)
 		diff->diffcaps = diff->diffcaps | GIT_DIFFCAPS_TRUST_MODE_BITS;
 
-	if (!git_repository__cvar(&val, repo, GIT_CVAR_TRUSTCTIME) && val)
+	if (!git_config__cvar(&val, cfg, GIT_CVAR_TRUSTCTIME) && val)
 		diff->diffcaps = diff->diffcaps | GIT_DIFFCAPS_TRUST_CTIME;
 
 	/* Don't set GIT_DIFFCAPS_USE_DEV - compile time option in core git */
@@ -481,8 +481,6 @@ static int diff_list_apply_options(
 	/* strdup prefix from pool so we're not dependent on external data */
 	diff->opts.old_prefix = diff_strdup_prefix(pool, diff->opts.old_prefix);
 	diff->opts.new_prefix = diff_strdup_prefix(pool, diff->opts.new_prefix);
-	if (!diff->opts.old_prefix || !diff->opts.new_prefix)
-		return -1;
 
 	if (DIFF_FLAG_IS_SET(diff, GIT_DIFF_REVERSE)) {
 		const char *tmp_prefix = diff->opts.old_prefix;
@@ -490,7 +488,10 @@ static int diff_list_apply_options(
 		diff->opts.new_prefix  = tmp_prefix;
 	}
 
-	return 0;
+	git_config_free(cfg);
+
+	/* check strdup results for error */
+	return (!diff->opts.old_prefix || !diff->opts.new_prefix) ? -1 : 0;
 }
 
 static void diff_list_free(git_diff *diff)
diff --git a/src/diff_driver.c b/src/diff_driver.c
index fc1354f36..dc8e79e25 100644
--- a/src/diff_driver.c
+++ b/src/diff_driver.c
@@ -233,17 +233,17 @@ static int git_diff_driver_load(
 		return 0;
 	}
 
+	drv = git__calloc(1, sizeof(git_diff_driver) + namelen + 1);
+	GITERR_CHECK_ALLOC(drv);
+	drv->type = DIFF_DRIVER_AUTO;
+	memcpy(drv->name, driver_name, namelen);
+
 	/* if you can't read config for repo, just use default driver */
 	if (git_repository_config_snapshot(&cfg, repo) < 0) {
 		giterr_clear();
 		goto done;
 	}
 
-	drv = git__calloc(1, sizeof(git_diff_driver) + namelen + 1);
-	GITERR_CHECK_ALLOC(drv);
-	drv->type = DIFF_DRIVER_AUTO;
-	memcpy(drv->name, driver_name, namelen);
-
 	if ((error = git_buf_printf(&name, "diff.%s.binary", driver_name)) < 0)
 		goto done;
 
diff --git a/src/refdb_fs.c b/src/refdb_fs.c
index f9bd4eab5..dd8bf7916 100644
--- a/src/refdb_fs.c
+++ b/src/refdb_fs.c
@@ -927,19 +927,15 @@ static int has_reflog(git_repository *repo, const char *name);
 /* We only write if it's under heads/, remotes/ or notes/ or if it already has a log */
 static int should_write_reflog(int *write, git_repository *repo, const char *name)
 {
-	git_config *config;
-	int error, logall, is_bare;
+	int error, logall;
 
-	/* Defaults to the opposite of the repo being bare */
-	is_bare = git_repository_is_bare(repo);
-	logall = !is_bare;
-
-	if ((error = git_repository_config__weakptr(&config, repo)) < 0)
+	error = git_repository__cvar(&logall, repo, GIT_CVAR_LOGALLREFUPDATES);
+	if (error < 0)
 		return error;
 
-	error = git_config_get_bool(&logall, config, "core.logallrefupdates");
-	if (error < 0 && error != GIT_ENOTFOUND)
-		return error;
+	/* Defaults to the opposite of the repo being bare */
+	if (logall == GIT_LOGALLREFUPDATES_UNSET)
+		logall = !git_repository_is_bare(repo);
 
 	if (!logall) {
 		*write = 0;
diff --git a/src/repository.c b/src/repository.c
index 7d055e28e..b0db5484a 100644
--- a/src/repository.c
+++ b/src/repository.c
@@ -443,7 +443,6 @@ int git_repository_open_ext(
 	int error;
 	git_buf path = GIT_BUF_INIT, parent = GIT_BUF_INIT;
 	git_repository *repo;
-	git_config *config;
 
 	if (repo_ptr)
 		*repo_ptr = NULL;
@@ -458,23 +457,24 @@ int git_repository_open_ext(
 	repo->path_repository = git_buf_detach(&path);
 	GITERR_CHECK_ALLOC(repo->path_repository);
 
-	if ((error = git_repository_config_snapshot(&config, repo)) < 0)
-		return error;
-
 	if ((flags & GIT_REPOSITORY_OPEN_BARE) != 0)
 		repo->is_bare = 1;
-	else if ((error = load_config_data(repo, config)) < 0 ||
-		 (error = load_workdir(repo, config, &parent)) < 0)
-	{
+	else {
+		git_config *config = NULL;
+
+		if ((error = git_repository_config_snapshot(&config, repo)) < 0 ||
+			(error = load_config_data(repo, config)) < 0 ||
+			(error = load_workdir(repo, config, &parent)) < 0)
+			git_repository_free(repo);
+
 		git_config_free(config);
-		git_repository_free(repo);
-		return error;
 	}
 
-	git_config_free(config);
+	if (!error)
+		*repo_ptr = repo;
 	git_buf_free(&parent);
-	*repo_ptr = repo;
-	return 0;
+
+	return error;
 }
 
 int git_repository_open(git_repository **repo_out, const char *path)
diff --git a/src/repository.h b/src/repository.h
index 27eec9dd8..aba16a016 100644
--- a/src/repository.h
+++ b/src/repository.h
@@ -39,6 +39,7 @@ typedef enum {
 	GIT_CVAR_ABBREV,        /* core.abbrev */
 	GIT_CVAR_PRECOMPOSE,    /* core.precomposeunicode */
 	GIT_CVAR_SAFE_CRLF,		/* core.safecrlf */
+	GIT_CVAR_LOGALLREFUPDATES, /* core.logallrefupdates */
 	GIT_CVAR_CACHE_MAX
 } git_cvar_cached;
 
@@ -92,6 +93,9 @@ typedef enum {
 	GIT_PRECOMPOSE_DEFAULT = GIT_CVAR_FALSE,
 	/* core.safecrlf */
 	GIT_SAFE_CRLF_DEFAULT = GIT_CVAR_FALSE,
+	/* core.logallrefupdates */
+	GIT_LOGALLREFUPDATES_UNSET = 2,
+	GIT_LOGALLREFUPDATES_DEFAULT = GIT_LOGALLREFUPDATES_UNSET,
 } git_cvar_value;
 
 /* internal repository init flags */
diff --git a/src/tag.c b/src/tag.c
index 1a4ee1e1c..d7b531d34 100644
--- a/src/tag.c
+++ b/src/tag.c
@@ -363,20 +363,22 @@ int git_tag_create_frombuffer(git_oid *oid, git_repository *repo, const char *bu
 	}
 
 	/* write the buffer */
-	if (git_odb_open_wstream(&stream, odb, strlen(buffer), GIT_OBJ_TAG) < 0)
-		return -1;
+	if ((error = git_odb_open_wstream(
+			&stream, odb, strlen(buffer), GIT_OBJ_TAG)) < 0)
+		return error;
 
-	git_odb_stream_write(stream, buffer, strlen(buffer));
+	if (!(error = git_odb_stream_write(stream, buffer, strlen(buffer))))
+		error = git_odb_stream_finalize_write(oid, stream);
 
-	error = git_odb_stream_finalize_write(oid, stream);
 	git_odb_stream_free(stream);
 
 	if (error < 0) {
 		git_buf_free(&ref_name);
-		return -1;
+		return error;
 	}
 
-	error = git_reference_create(&new_ref, repo, ref_name.ptr, oid, allow_ref_overwrite, NULL, NULL);
+	error = git_reference_create(
+		&new_ref, repo, ref_name.ptr, oid, allow_ref_overwrite, NULL, NULL);
 
 	git_reference_free(new_ref);
 	git_buf_free(&ref_name);