Merge pull request #2188 from libgit2/cmn/config-snapshot

Configuration snapshotting

include/git2/config.h

@@ -226,6 +226,22 @@ GIT_EXTERN(int) git_config_open_level(
  */
 GIT_EXTERN(int) git_config_open_global(git_config **out, git_config *config);
 
+/**
+ * Create a snapshot of the configuration
+ *
+ * Create a snapshot of the current state of a configuration, which
+ * allows you to look into a consistent view of the configuration for
+ * looking up complex values (e.g. a remote, submodule).
+ *
+ * The string returned when querying such a config object is valid
+ * until it is freed.
+ *
+ * @param out pointer in which to store the snapshot config object
+ * @param config configuration to snapshot
+ * @return 0 or an error code
+ */
+GIT_EXTERN(int) git_config_snapshot(git_config **out, git_config *config);
+
 
 /**
  * Reload changed config files
@@ -312,7 +328,8 @@ GIT_EXTERN(int) git_config_get_bool(int *out, const git_config *cfg, const char
  * Get the value of a string config variable.
  *
  * The string is owned by the variable and should not be freed by the
- * user.
+ * user. The pointer will be valid until the next operation on this
+ * config object.
  *
  * All config files will be looked into, in the order of their
  * defined level. A higher level means a higher priority. The
@@ -353,6 +370,9 @@ GIT_EXTERN(int) git_config_multivar_iterator_new(git_config_iterator **out, cons
 /**
  * Return the current entry and advance the iterator
  *
+ * The pointers returned by this function are valid until the iterator
+ * is freed.
+ *
  * @param entry pointer to store the entry
  * @param iter the iterator
  * @return 0 or an error code. GIT_ITEROVER if the iteration has completed
@@ -451,6 +471,9 @@ GIT_EXTERN(int) git_config_delete_multivar(git_config *cfg, const char *name, co
  * If the callback returns a non-zero value, the function stops iterating
  * and returns that value to the caller.
  *
+ * The pointers passed to the callback are only valid as long as the
+ * iteration is ongoing.
+ *
  * @param cfg where to get the variables from
  * @param callback the function to call on each variable
  * @param payload the data to pass to the callback
@@ -491,6 +514,9 @@ GIT_EXTERN(int) git_config_iterator_glob_new(git_config_iterator **out, const gi
  * regular expression that filters which config keys are passed to the
  * callback.
  *
+ * The pointers passed to the callback are only valid as long as the
+ * iteration is ongoing.
+ *
  * @param cfg where to get the variables from
  * @param regexp regular expression to match against config names
  * @param callback the function to call on each variable

include/git2/repository.h

@@ -408,13 +408,25 @@ GIT_EXTERN(int) git_repository_is_bare(git_repository *repo);
  * The configuration file must be freed once it's no longer
  * being used by the user.
  *
- * @param out Pointer to store the loaded config file
+ * @param out Pointer to store the loaded configuration
  * @param repo A repository object
  * @return 0, or an error code
  */
 GIT_EXTERN(int) git_repository_config(git_config **out, git_repository *repo);
 
 /**
+ * Get a snapshot of the repository's configuration
+ *
+ * Convenience function to take a snapshot from the repository's
+ * configuration.
+ *
+ * @param out Pointer to store the loaded configuration
+ * @param repo the repository
+ * @return 0, or an error code
+ */
+GIT_EXTERN(int) git_repository_config_snapshot(git_config **out, git_repository *repo);
+
+/**
  * Get the Object Database for this repository.
  *
  * If a custom ODB has not been set, the default

include/git2/sys/config.h

@@ -57,13 +57,15 @@ struct git_config_backend {
 
 	/* Open means open the file/database and parse if necessary */
 	int (*open)(struct git_config_backend *, git_config_level_t level);
-	int (*get)(const struct git_config_backend *, const char *key, const git_config_entry **entry);
+	int (*get)(struct git_config_backend *, const char *key, const git_config_entry **entry);
 	int (*set)(struct git_config_backend *, const char *key, const char *value);
 	int (*set_multivar)(git_config_backend *cfg, const char *name, const char *regexp, const char *value);
 	int (*del)(struct git_config_backend *, const char *key);
 	int (*del_multivar)(struct git_config_backend *, const char *key, const char *regexp);
 	int (*iterator)(git_config_iterator **, struct git_config_backend *);
 	int (*refresh)(struct git_config_backend *);
+	/** Produce a read-only version of this backend */
+	int (*snapshot)(struct git_config_backend **, struct git_config_backend *);
 	void (*free)(struct git_config_backend *);
 };
 #define GIT_CONFIG_BACKEND_VERSION 1

src/branch.c

@@ -306,17 +306,13 @@ int git_branch_name(
 
 static int retrieve_upstream_configuration(
 	const char **out,
-	git_repository *repo,
+	const git_config *config,
 	const char *canonical_branch_name,
 	const char *format)
 {
-	git_config *config;
 	git_buf buf = GIT_BUF_INIT;
 	int error;
 
-	if (git_repository_config__weakptr(&config, repo) < 0)
-		return -1;
-
 	if (git_buf_printf(&buf, format,
 		canonical_branch_name + strlen(GIT_REFS_HEADS_DIR)) < 0)
 			return -1;
@@ -336,6 +332,7 @@ int git_branch_upstream_name(
 	int error = -1;
 	git_remote *remote = NULL;
 	const git_refspec *refspec;
+	git_config *config;
 
 	assert(out && refname);
 
@@ -344,12 +341,15 @@ int git_branch_upstream_name(
 	if (!git_reference__is_branch(refname))
 		return not_a_local_branch(refname);
 
+	if ((error = git_repository_config_snapshot(&config, repo)) < 0)
+		return error;
+
 	if ((error = retrieve_upstream_configuration(
-		&remote_name, repo, refname, "branch.%s.remote")) < 0)
+		&remote_name, config, refname, "branch.%s.remote")) < 0)
 			goto cleanup;
 
 	if ((error = retrieve_upstream_configuration(
-		&merge_name, repo, refname, "branch.%s.merge")) < 0)
+		&merge_name, config, refname, "branch.%s.merge")) < 0)
 			goto cleanup;
 
 	if (!*remote_name || !*merge_name) {
@@ -378,6 +378,7 @@ int git_branch_upstream_name(
 	error = git_buf_set(out, git_buf_cstr(&buf), git_buf_len(&buf));
 
 cleanup:
+	git_config_free(config);
 	git_remote_free(remote);
 	git_buf_free(&buf);
 	return error;

src/config.c

@@ -137,6 +137,38 @@ int git_config_open_ondisk(git_config **out, const char *path)
 	return error;
 }
 
+int git_config_snapshot(git_config **out, git_config *in)
+{
+	int error;
+	size_t i;
+	file_internal *internal;
+	git_config *config;
+
+	*out = NULL;
+
+	if (git_config_new(&config) < 0)
+		return -1;
+
+	git_vector_foreach(&in->files, i, internal) {
+		git_config_backend *b;
+
+		if ((error = internal->file->snapshot(&b, internal->file)) < 0)
+			goto on_error;
+
+		if ((error = git_config_add_backend(config, b, internal->level, 0)) < 0) {
+			b->free(b);
+			goto on_error;
+		}
+	}
+
+	*out = config;
+	return error;
+
+on_error:
+	git_config_free(config);
+	return error;
+}
+
 static int find_internal_file_by_level(
 	file_internal **internal_out,
 	const git_config *cfg,

src/diff_driver.c

@@ -219,7 +219,7 @@ static int git_diff_driver_load(
 	git_diff_driver *drv = NULL;
 	size_t namelen = strlen(driver_name);
 	khiter_t pos;
-	git_config *cfg;
+	git_config *cfg, *repo_cfg;
 	git_buf name = GIT_BUF_INIT;
 	const git_config_entry *ce;
 	bool found_driver = false;
@@ -234,7 +234,7 @@ static int git_diff_driver_load(
 	}
 
 	/* if you can't read config for repo, just use default driver */
-	if (git_repository_config__weakptr(&cfg, repo) < 0) {
+	if (git_repository_config_snapshot(&cfg, repo) < 0) {
 		giterr_clear();
 		goto done;
 	}
@@ -321,6 +321,7 @@ static int git_diff_driver_load(
 
 done:
 	git_buf_free(&name);
+	git_config_free(cfg);
 
 	if (!*out) {
 		int error2 = git_diff_driver_builtin(out, reg, driver_name);

src/pack-objects.c

@@ -90,8 +90,8 @@ static int packbuilder_config(git_packbuilder *pb)
 	int ret;
 	int64_t val;
 
-	if (git_repository_config__weakptr(&config, pb->repo) < 0)
-		return -1;
+	if ((ret = git_repository_config_snapshot(&config, pb->repo)) < 0)
+		return ret;
 
 #define config_get(KEY,DST,DFLT) do { \
 	ret = git_config_get_int64(&val, config, KEY); \
@@ -109,6 +109,8 @@ static int packbuilder_config(git_packbuilder *pb)
 
 #undef config_get
 
+	git_config_free(config);
+
 	return 0;
 }
 

src/remote.c

@@ -356,8 +356,8 @@ int git_remote_load(git_remote **out, git_repository *repo, const char *name)
 	if ((error = ensure_remote_name_is_valid(name)) < 0)
 		return error;
 
-	if (git_repository_config__weakptr(&config, repo) < 0)
-		return -1;
+	if ((error = git_repository_config_snapshot(&config, repo)) < 0)
+		return error;
 
 	remote = git__malloc(sizeof(git_remote));
 	GITERR_CHECK_ALLOC(remote);
@@ -437,6 +437,7 @@ int git_remote_load(git_remote **out, git_repository *repo, const char *name)
 	*out = remote;
 
 cleanup:
+	git_config_free(config);
 	git_buf_free(&buf);
 
 	if (error < 0)

src/repository.c

@@ -169,13 +169,9 @@ int git_repository_new(git_repository **out)
 	return 0;
 }
 
-static int load_config_data(git_repository *repo)
+static int load_config_data(git_repository *repo, const git_config *config)
 {
 	int is_bare;
-	git_config *config;
-
-	if (git_repository_config__weakptr(&config, repo) < 0)
-		return -1;
 
 	/* Try to figure out if it's bare, default to non-bare if it's not set */
 	if (git_config_get_bool(&is_bare, config, "core.bare") < 0)
@@ -186,19 +182,15 @@ static int load_config_data(git_repository *repo)
 	return 0;
 }
 
-static int load_workdir(git_repository *repo, git_buf *parent_path)
+static int load_workdir(git_repository *repo, git_config *config, git_buf *parent_path)
 {
 	int         error;
-	git_config *config;
 	const git_config_entry *ce;
 	git_buf     worktree = GIT_BUF_INIT;
 
 	if (repo->is_bare)
 		return 0;
 
-	if ((error = git_repository_config__weakptr(&config, repo)) < 0)
-		return error;
-
 	if ((error = git_config__lookup_entry(
 			&ce, config, "core.worktree", false)) < 0)
 		return error;
@@ -451,6 +443,7 @@ 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;
@@ -465,15 +458,20 @@ 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)) < 0 ||
-		(error = load_workdir(repo, &parent)) < 0)
+	else if ((error = load_config_data(repo, config)) < 0 ||
+		 (error = load_workdir(repo, config, &parent)) < 0)
 	{
+		git_config_free(config);
 		git_repository_free(repo);
 		return error;
 	}
 
+	git_config_free(config);
 	git_buf_free(&parent);
 	*repo_ptr = repo;
 	return 0;
@@ -627,6 +625,16 @@ int git_repository_config(git_config **out, git_repository *repo)
 	return 0;
 }
 
+int git_repository_config_snapshot(git_config **out, git_repository *repo)
+{
+	git_config *weak;
+
+	if (git_repository_config__weakptr(&weak, repo) < 0)
+		return -1;
+
+	return git_config_snapshot(out, weak);
+}
+
 void git_repository_set_config(git_repository *repo, git_config *config)
 {
 	assert(repo && config);

src/signature.c

@@ -144,7 +144,7 @@ int git_signature_default(git_signature **out, git_repository *repo)
 	git_config *cfg;
 	const char *user_name, *user_email;
 
-	if ((error = git_repository_config(&cfg, repo)) < 0)
+	if ((error = git_repository_config_snapshot(&cfg, repo)) < 0)
 		return error;
 
 	if (!(error = git_config_get_string(&user_name, cfg, "user.name")) &&

tests/config/multivar.c

@@ -231,13 +231,13 @@ void test_config_multivar__delete(void)
 
 	n = 0;
 	cl_git_pass(git_config_get_multivar_foreach(cfg, _name, NULL, cb, &n));
-	cl_assert(n == 2);
+	cl_assert_equal_i(2, n);
 
 	cl_git_pass(git_config_delete_multivar(cfg, _name, "github"));
 
 	n = 0;
 	cl_git_pass(git_config_get_multivar_foreach(cfg, _name, NULL, cb, &n));
-	cl_assert(n == 1);
+	cl_assert_equal_i(1, n);
 
 	git_config_free(cfg);
 
@@ -245,7 +245,7 @@ void test_config_multivar__delete(void)
 
 	n = 0;
 	cl_git_pass(git_config_get_multivar_foreach(cfg, _name, NULL, cb, &n));
-	cl_assert(n == 1);
+	cl_assert_equal_i(1, n);
 
 	git_config_free(cfg);
 }

tests/config/refresh.c

@@ -26,9 +26,6 @@ void test_config_refresh__update_value(void)
 
 	cl_git_rewritefile(TEST_FILE, "[section]\n\tvalue = 10\n\n");
 
-	cl_git_pass(git_config_get_int32(&v, cfg, "section.value"));
-	cl_assert_equal_i(1, v);
-
 	cl_git_pass(git_config_refresh(cfg));
 
 	cl_git_pass(git_config_get_int32(&v, cfg, "section.value"));
@@ -53,9 +50,9 @@ void test_config_refresh__delete_value(void)
 
 	cl_git_rewritefile(TEST_FILE, "[section]\n\tnewval = 10\n\n");
 
-	cl_git_pass(git_config_get_int32(&v, cfg, "section.value"));
-	cl_assert_equal_i(1, v);
-	cl_git_fail(git_config_get_int32(&v, cfg, "section.newval"));
+	cl_git_fail_with(GIT_ENOTFOUND, git_config_get_int32(&v, cfg, "section.value"));
+
+	cl_git_pass(git_config_get_int32(&v, cfg, "section.newval"));
 
 	cl_git_pass(git_config_refresh(cfg));
 

tests/config/snapshot.c

+#include "clar_libgit2.h"
+
+void test_config_snapshot__create_snapshot(void)
+{
+	int32_t tmp;
+	git_config *cfg, *snapshot;
+	const char *filename = "config-ext-change";
+
+	cl_git_mkfile(filename, "[old]\nvalue = 5\n");
+
+	cl_git_pass(git_config_open_ondisk(&cfg, filename));
+
+	cl_git_pass(git_config_get_int32(&tmp, cfg, "old.value"));
+	cl_assert_equal_i(5, tmp);
+
+	cl_git_pass(git_config_snapshot(&snapshot, cfg));
+
+	/* Change the value on the file itself (simulate external process) */
+	cl_git_mkfile(filename, "[old]\nvalue = 56\n");
+
+	cl_git_pass(git_config_get_int32(&tmp, cfg, "old.value"));
+	cl_assert_equal_i(56, tmp);
+
+	cl_git_pass(git_config_get_int32(&tmp, snapshot, "old.value"));
+	cl_assert_equal_i(5, tmp);
+	
+	git_config_free(snapshot);
+	git_config_free(cfg);
+}
+
+static int count_me(const git_config_entry *entry, void *payload)
+{
+	int *n = (int *) payload;
+
+	GIT_UNUSED(entry);
+
+	(*n)++;
+
+	return 0;
+}
+
+void test_config_snapshot__multivar(void)
+{
+	int count = 0;
+	git_config *cfg, *snapshot;
+	const char *filename = "config-file";
+
+	cl_git_mkfile(filename, "[old]\nvalue = 5\nvalue = 6\n");
+
+	cl_git_pass(git_config_open_ondisk(&cfg, filename));
+	cl_git_pass(git_config_get_multivar_foreach(cfg, "old.value", NULL, count_me, &count));
+
+	cl_assert_equal_i(2, count);
+
+	cl_git_pass(git_config_snapshot(&snapshot, cfg));
+	git_config_free(cfg);
+
+	count = 0;
+	cl_git_pass(git_config_get_multivar_foreach(snapshot, "old.value", NULL, count_me, &count));
+
+	cl_assert_equal_i(2, count);
+
+	git_config_free(snapshot);
+}

tests/config/write.c

@@ -304,3 +304,30 @@ void test_config_write__updating_a_locked_config_file_returns_ELOCKED(void)
 	git_config_free(cfg);
 }
 
+void test_config_write__outside_change(void)
+{
+	int32_t tmp;
+	git_config *cfg;
+	const char *filename = "config-ext-change";
+
+	cl_git_mkfile(filename, "[old]\nvalue = 5\n");
+
+	cl_git_pass(git_config_open_ondisk(&cfg, filename));
+
+	cl_git_pass(git_config_get_int32(&tmp, cfg, "old.value"));
+
+	/* Change the value on the file itself (simulate external process) */
+	cl_git_mkfile(filename, "[old]\nvalue = 6\n");
+
+	cl_git_pass(git_config_set_int32(cfg, "new.value", 7));
+
+	cl_git_pass(git_config_get_int32(&tmp, cfg, "old.value"));
+	cl_assert_equal_i(6, tmp);
+
+	cl_git_pass(git_config_refresh(cfg));
+
+	cl_git_pass(git_config_get_int32(&tmp, cfg, "old.value"));
+	cl_assert_equal_i(6, tmp);
+
+	git_config_free(cfg);
+}