Merge pull request #3168 from libgit2/cmn/config-tx

Locking and transactional/atomic updates for config

CHANGELOG.md

@@ -9,6 +9,11 @@ v0.23 + 1
 
 ### API additions
 
+* `git_config_lock()` has been added, which allow for
+  transactional/atomic complex updates to the configuration, removing
+  the opportunity for concurrent operations and not committing any
+  changes until the unlock.
+
 ### API removals
 
 ### Breaking API changes
@@ -19,6 +24,10 @@ v0.23 + 1
   with the reflog on ref deletion. The file-based backend must delete
   it, a database-backed one may wish to archive it.
 
+* `git_config_backend` has gained two entries. `lock` and `unlock`
+  with which to implement the transactional/atomic semantics for the
+  configuration backend.
+
 v0.23
 ------
 

include/git2/config.h

@@ -689,6 +689,24 @@ GIT_EXTERN(int) git_config_backend_foreach_match(
 	void *payload);
 
 
+/**
+ * Lock the backend with the highest priority
+ *
+ * Locking disallows anybody else from writing to that backend. Any
+ * updates made after locking will not be visible to a reader until
+ * the file is unlocked.
+ *
+ * You can apply the changes by calling `git_transaction_commit()`
+ * before freeing the transaction. Either of these actions will unlock
+ * the config.
+ *
+ * @param tx the resulting transaction, use this to commit or undo the
+ * changes
+ * @param cfg the configuration in which to lock
+ * @return 0 or an error code
+ */
+GIT_EXTERN(int) git_config_lock(git_transaction **tx, git_config *cfg);
+
 /** @} */
 GIT_END_DECL
 #endif

include/git2/sys/config.h

@@ -67,6 +67,20 @@ struct git_config_backend {
 	int (*iterator)(git_config_iterator **, struct git_config_backend *);
 	/** Produce a read-only version of this backend */
 	int (*snapshot)(struct git_config_backend **, struct git_config_backend *);
+	/**
+	 * Lock this backend.
+	 *
+	 * Prevent any writes to the data store backing this
+	 * backend. Any updates must not be visible to any other
+	 * readers.
+	 */
+	int (*lock)(struct git_config_backend *);
+	/**
+	 * Unlock the data store backing this backend. If success is
+	 * true, the changes should be committed, otherwise rolled
+	 * back.
+	 */
+	int (*unlock)(struct git_config_backend *, int success);
 	void (*free)(struct git_config_backend *);
 };
 #define GIT_CONFIG_BACKEND_VERSION 1

src/config.c

@@ -1144,6 +1144,41 @@ int git_config_open_default(git_config **out)
 	return error;
 }
 
+int git_config_lock(git_transaction **out, git_config *cfg)
+{
+	int error;
+	git_config_backend *file;
+	file_internal *internal;
+
+	internal = git_vector_get(&cfg->files, 0);
+	if (!internal || !internal->file) {
+		giterr_set(GITERR_CONFIG, "cannot lock; the config has no backends/files");
+		return -1;
+	}
+	file = internal->file;
+
+	if ((error = file->lock(file)) < 0)
+		return error;
+
+	return git_transaction_config_new(out, cfg);
+}
+
+int git_config_unlock(git_config *cfg, int commit)
+{
+	git_config_backend *file;
+	file_internal *internal;
+
+	internal = git_vector_get(&cfg->files, 0);
+	if (!internal || !internal->file) {
+		giterr_set(GITERR_CONFIG, "cannot lock; the config has no backends/files");
+		return -1;
+	}
+
+	file = internal->file;
+
+	return file->unlock(file, commit);
+}
+
 /***********
  * Parsers
  ***********/

src/config.h

@@ -88,4 +88,19 @@ extern int git_config__cvar(
  */
 int git_config_lookup_map_enum(git_cvar_t *type_out, const char **str_out,
 			       const git_cvar_map *maps, size_t map_n, int enum_val);
+
+/**
+ * Unlock the backend with the highest priority
+ *
+ * Unlocking will allow other writers to updat the configuration
+ * file. Optionally, any changes performed since the lock will be
+ * applied to the configuration.
+ *
+ * @param cfg the configuration
+ * @param commit boolean which indicates whether to commit any changes
+ * done since locking
+ * @return 0 or an error code
+ */
+GIT_EXTERN(int) git_config_unlock(git_config *cfg, int commit);
+
 #endif

src/config_file.c

@@ -105,6 +105,10 @@ typedef struct {
 
 	git_array_t(struct reader) readers;
 
+	bool locked;
+	git_filebuf locked_buf;
+	git_buf locked_content;
+
 	char  *file_path;
 } diskfile_backend;
 
@@ -685,6 +689,42 @@ static int config_snapshot(git_config_backend **out, git_config_backend *in)
 	return git_config_file__snapshot(out, b);
 }
 
+static int config_lock(git_config_backend *_cfg)
+{
+	diskfile_backend *cfg = (diskfile_backend *) _cfg;
+	int error;
+
+	if ((error = git_filebuf_open(&cfg->locked_buf, cfg->file_path, 0, GIT_CONFIG_FILE_MODE)) < 0)
+		return error;
+
+	error = git_futils_readbuffer(&cfg->locked_content, cfg->file_path);
+	if (error < 0 && error != GIT_ENOTFOUND) {
+		git_filebuf_cleanup(&cfg->locked_buf);
+		return error;
+	}
+
+	cfg->locked = true;
+	return 0;
+
+}
+
+static int config_unlock(git_config_backend *_cfg, int success)
+{
+	diskfile_backend *cfg = (diskfile_backend *) _cfg;
+	int error = 0;
+
+	if (success) {
+		git_filebuf_write(&cfg->locked_buf, cfg->locked_content.ptr, cfg->locked_content.size);
+		error = git_filebuf_commit(&cfg->locked_buf);
+	}
+
+	git_filebuf_cleanup(&cfg->locked_buf);
+	git_buf_free(&cfg->locked_content);
+	cfg->locked = false;
+
+	return error;
+}
+
 int git_config_file__ondisk(git_config_backend **out, const char *path)
 {
 	diskfile_backend *backend;
@@ -706,6 +746,8 @@ int git_config_file__ondisk(git_config_backend **out, const char *path)
 	backend->header.parent.del_multivar = config_delete_multivar;
 	backend->header.parent.iterator = config_iterator_new;
 	backend->header.parent.snapshot = config_snapshot;
+	backend->header.parent.lock = config_lock;
+	backend->header.parent.unlock = config_unlock;
 	backend->header.parent.free = backend_free;
 
 	*out = (git_config_backend *)backend;
@@ -750,6 +792,21 @@ static int config_delete_readonly(git_config_backend *cfg, const char *name)
 	return config_error_readonly();
 }
 
+static int config_lock_readonly(git_config_backend *_cfg)
+{
+	GIT_UNUSED(_cfg);
+
+	return config_error_readonly();
+}
+
+static int config_unlock_readonly(git_config_backend *_cfg, int success)
+{
+	GIT_UNUSED(_cfg);
+	GIT_UNUSED(success);
+
+	return config_error_readonly();
+}
+
 static void backend_readonly_free(git_config_backend *_backend)
 {
 	diskfile_backend *backend = (diskfile_backend *)_backend;
@@ -803,6 +860,8 @@ int git_config_file__snapshot(git_config_backend **out, diskfile_backend *in)
 	backend->header.parent.del = config_delete_readonly;
 	backend->header.parent.del_multivar = config_delete_multivar_readonly;
 	backend->header.parent.iterator = config_iterator_new;
+	backend->header.parent.lock = config_lock_readonly;
+	backend->header.parent.unlock = config_unlock_readonly;
 	backend->header.parent.free = backend_readonly_free;
 
 	*out = (git_config_backend *)backend;
@@ -1602,7 +1661,7 @@ static int config_read(git_strmap *values, diskfile_backend *cfg_file, struct re
 	return config_parse(reader, NULL, read_on_variable, NULL, NULL, &parse_data);
 }
 
-static int write_section(git_filebuf *file, const char *key)
+static int write_section(git_buf *fbuf, const char *key)
 {
 	int result;
 	const char *dot;
@@ -1626,7 +1685,7 @@ static int write_section(git_filebuf *file, const char *key)
 	if (git_buf_oom(&buf))
 		return -1;
 
-	result = git_filebuf_write(file, git_buf_cstr(&buf), buf.size);
+	result = git_buf_put(fbuf, git_buf_cstr(&buf), buf.size);
 	git_buf_free(&buf);
 
 	return result;
@@ -1651,7 +1710,7 @@ static const char *quotes_for_value(const char *value)
 }
 
 struct write_data {
-	git_filebuf *file;
+	git_buf *buf;
 	unsigned int in_section : 1,
 		preg_replaced : 1;
 	const char *section;
@@ -1662,10 +1721,10 @@ struct write_data {
 
 static int write_line(struct write_data *write_data, const char *line, size_t line_len)
 {
-	int result = git_filebuf_write(write_data->file, line, line_len);
+	int result = git_buf_put(write_data->buf, line, line_len);
 
 	if (!result && line_len && line[line_len-1] != '\n')
-		result = git_filebuf_printf(write_data->file, "\n");
+		result = git_buf_printf(write_data->buf, "\n");
 
 	return result;
 }
@@ -1676,7 +1735,7 @@ static int write_value(struct write_data *write_data)
 	int result;
 
 	q = quotes_for_value(write_data->value);
-	result = git_filebuf_printf(write_data->file,
+	result = git_buf_printf(write_data->buf,
 		"\t%s = %s%s%s\n", write_data->name, q, write_data->value, q);
 
 	/* If we are updating a single name/value, we're done.  Setting `value`
@@ -1782,7 +1841,7 @@ static int write_on_eof(struct reader **reader, void *data)
 	 * value.
 	 */
 	if ((!write_data->preg || !write_data->preg_replaced) && write_data->value) {
-		if ((result = write_section(write_data->file, write_data->section)) == 0)
+		if ((result = write_section(write_data->buf, write_data->section)) == 0)
 			result = write_value(write_data);
 	}
 
@@ -1797,18 +1856,23 @@ static int config_write(diskfile_backend *cfg, const char *key, const regex_t *p
 	int result;
 	char *section, *name, *ldot;
 	git_filebuf file = GIT_FILEBUF_INIT;
+	git_buf buf = GIT_BUF_INIT;
 	struct reader *reader = git_array_get(cfg->readers, 0);
 	struct write_data write_data;
 
-	/* Lock the file */
+	if (cfg->locked) {
-	if ((result = git_filebuf_open(
+		result = git_buf_puts(&reader->buffer, git_buf_cstr(&cfg->locked_content));
-		&file, cfg->file_path, 0, GIT_CONFIG_FILE_MODE)) < 0) {
+	} else {
+		/* Lock the file */
+		if ((result = git_filebuf_open(
+			     &file, cfg->file_path, 0, GIT_CONFIG_FILE_MODE)) < 0) {
 			git_buf_free(&reader->buffer);
 			return result;
-	}
+		}
 
-	/* We need to read in our own config file */
+		/* We need to read in our own config file */
-	result = git_futils_readbuffer(&reader->buffer, cfg->file_path);
+		result = git_futils_readbuffer(&reader->buffer, cfg->file_path);
+	}
 
 	/* Initialise the reading position */
 	if (result == GIT_ENOTFOUND) {
@@ -1827,7 +1891,7 @@ static int config_write(diskfile_backend *cfg, const char *key, const regex_t *p
 	name = ldot + 1;
 	section = git__strndup(key, ldot - key);
 
-	write_data.file = &file;
+	write_data.buf = &buf;
 	write_data.section = section;
 	write_data.in_section = 0;
 	write_data.preg_replaced = 0;
@@ -1843,13 +1907,22 @@ static int config_write(diskfile_backend *cfg, const char *key, const regex_t *p
 		goto done;
 	}
 
-	/* refresh stats - if this errors, then commit will error too */
+	if (cfg->locked) {
-	(void)git_filebuf_stats(&reader->file_mtime, &reader->file_size, &file);
+		size_t len = buf.asize;
+		/* Update our copy with the modified contents */
+		git_buf_free(&cfg->locked_content);
+		git_buf_attach(&cfg->locked_content, git_buf_detach(&buf), len);
+	} else {
+		git_filebuf_write(&file, git_buf_cstr(&buf), git_buf_len(&buf));
 
-	result = git_filebuf_commit(&file);
+		/* refresh stats - if this errors, then commit will error too */
-	git_buf_free(&reader->buffer);
+		(void)git_filebuf_stats(&reader->file_mtime, &reader->file_size, &file);
+
+		result = git_filebuf_commit(&file);
+	}
 
 done:
+	git_buf_free(&buf);
 	git_buf_free(&reader->buffer);
 	return result;
 }

src/config_file.h

@@ -55,6 +55,16 @@ GIT_INLINE(int) git_config_file_foreach_match(
 	return git_config_backend_foreach_match(cfg, regexp, fn, data);
 }
 
+GIT_INLINE(int) git_config_file_lock(git_config_backend *cfg)
+{
+	return cfg->lock(cfg);
+}
+
+GIT_INLINE(int) git_config_file_unlock(git_config_backend *cfg, int success)
+{
+	return cfg->unlock(cfg, success);
+}
+
 extern int git_config_file_normalize_section(char *start, char *end);
 
 #endif

src/transaction.c

@@ -12,6 +12,7 @@
 #include "pool.h"
 #include "reflog.h"
 #include "signature.h"
+#include "config.h"
 
 #include "git2/transaction.h"
 #include "git2/signature.h"
@@ -20,6 +21,12 @@
 
 GIT__USE_STRMAP
 
+typedef enum {
+	TRANSACTION_NONE,
+	TRANSACTION_REFS,
+	TRANSACTION_CONFIG,
+} transaction_t;
+
 typedef struct {
 	const char *name;
 	void *payload;
@@ -39,13 +46,29 @@ typedef struct {
 } transaction_node;
 
 struct git_transaction {
+	transaction_t type;
 	git_repository *repo;
 	git_refdb *db;
+	git_config *cfg;
 
 	git_strmap *locks;
 	git_pool pool;
 };
 
+int git_transaction_config_new(git_transaction **out, git_config *cfg)
+{
+	git_transaction *tx;
+	assert(out && cfg);
+
+	tx = git__calloc(1, sizeof(git_transaction));
+	GITERR_CHECK_ALLOC(tx);
+
+	tx->type = TRANSACTION_CONFIG;
+	tx->cfg = cfg;
+	*out = tx;
+	return 0;
+}
+
 int git_transaction_new(git_transaction **out, git_repository *repo)
 {
 	int error;
@@ -71,6 +94,7 @@ int git_transaction_new(git_transaction **out, git_repository *repo)
 	if ((error = git_repository_refdb(&tx->db, repo)) < 0)
 		goto on_error;
 
+	tx->type = TRANSACTION_REFS;
 	memcpy(&tx->pool, &pool, sizeof(git_pool));
 	tx->repo = repo;
 	*out = tx;
@@ -305,6 +329,14 @@ int git_transaction_commit(git_transaction *tx)
 
 	assert(tx);
 
+	if (tx->type == TRANSACTION_CONFIG) {
+		error = git_config_unlock(tx->cfg, true);
+		git_config_free(tx->cfg);
+		tx->cfg = NULL;
+
+		return error;
+	}
+
 	for (pos = kh_begin(tx->locks); pos < kh_end(tx->locks); pos++) {
 		if (!git_strmap_has_data(tx->locks, pos))
 			continue;
@@ -332,6 +364,16 @@ void git_transaction_free(git_transaction *tx)
 
 	assert(tx);
 
+	if (tx->type == TRANSACTION_CONFIG) {
+		if (tx->cfg) {
+			git_config_unlock(tx->cfg, false);
+			git_config_free(tx->cfg);
+		}
+
+		git__free(tx);
+		return;
+	}
+
 	/* start by unlocking the ones we've left hanging, if any */
 	for (pos = kh_begin(tx->locks); pos < kh_end(tx->locks); pos++) {
 		if (!git_strmap_has_data(tx->locks, pos))

src/transaction.h

@@ -0,0 +1,14 @@
+/*
+ * Copyright (C) the libgit2 contributors. All rights reserved.
+ *
+ * This file is part of libgit2, distributed under the GNU GPL v2 with
+ * a Linking Exception. For full terms see the included COPYING file.
+ */
+#ifndef INCLUDE_transaction_h__
+#define INCLUDE_transaction_h__
+
+#include "common.h"
+
+int git_transaction_config_new(git_transaction **out, git_config *cfg);
+
+#endif

tests/config/write.c

@@ -1,6 +1,9 @@
 #include "clar_libgit2.h"
 #include "buffer.h"
 #include "fileops.h"
+#include "git2/sys/config.h"
+#include "config_file.h"
+#include "config.h"
 
 void test_config_write__initialize(void)
 {
@@ -630,3 +633,50 @@ void test_config_write__to_file_with_only_comment(void)
 	git_buf_free(&result);
 }
 
+void test_config_write__locking(void)
+{
+	git_config *cfg, *cfg2;
+	git_config_entry *entry;
+	git_transaction *tx;
+	const char *filename = "locked-file";
+
+	/* Open the config and lock it */
+	cl_git_mkfile(filename, "[section]\n\tname = value\n");
+	cl_git_pass(git_config_open_ondisk(&cfg, filename));
+	cl_git_pass(git_config_get_entry(&entry, cfg, "section.name"));
+	cl_assert_equal_s("value", entry->value);
+	git_config_entry_free(entry);
+	cl_git_pass(git_config_lock(&tx, cfg));
+
+	/* Change entries in the locked backend */
+	cl_git_pass(git_config_set_string(cfg, "section.name", "other value"));
+	cl_git_pass(git_config_set_string(cfg, "section2.name3", "more value"));
+
+	/* We can see that the file we read from hasn't changed */
+	cl_git_pass(git_config_open_ondisk(&cfg2, filename));
+	cl_git_pass(git_config_get_entry(&entry, cfg2, "section.name"));
+	cl_assert_equal_s("value", entry->value);
+	git_config_entry_free(entry);
+	cl_git_fail_with(GIT_ENOTFOUND, git_config_get_entry(&entry, cfg2, "section2.name3"));
+	git_config_free(cfg2);
+
+	/* And we also get the old view when we read from the locked config */
+	cl_git_pass(git_config_get_entry(&entry, cfg, "section.name"));
+	cl_assert_equal_s("value", entry->value);
+	git_config_entry_free(entry);
+	cl_git_fail_with(GIT_ENOTFOUND, git_config_get_entry(&entry, cfg, "section2.name3"));
+
+	cl_git_pass(git_transaction_commit(tx));
+	git_transaction_free(tx);
+
+	/* Now that we've unlocked it, we should see both updates */
+	cl_git_pass(git_config_open_ondisk(&cfg, filename));
+	cl_git_pass(git_config_get_entry(&entry, cfg, "section.name"));
+	cl_assert_equal_s("other value", entry->value);
+	git_config_entry_free(entry);
+	cl_git_pass(git_config_get_entry(&entry, cfg, "section2.name3"));
+	cl_assert_equal_s("more value", entry->value);
+	git_config_entry_free(entry);
+
+	git_config_free(cfg);
+}