Commit 9f1af7f279e9f5cbd03c9a9d78fcba77a1c2c64c

Edward Thomson 2015-08-13T10:22:50

Merge pull request #3168 from libgit2/cmn/config-tx Locking and transactional/atomic updates for config

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378
379
380
381
382
383
384
385
386
387
388
389
390
391
392
393
394
395
396
397
398
399
400
401
402
403
404
405
406
407
408
409
410
411
412
413
414
415
416
417
418
419
420
421
422
423
424
425
426
427
428
429
430
431
432
433
434
435
436
437
438
439
440
441
442
443
444
445
446
447
448
449
450
451
452
453
454
455
456
457
458
459
460
461
462
463
464
465
466
467
468
469
470
471
472
473
474
475
476
477
478
479
480
481
482
483
484
485
486
487
488
489
490
491
492
493
494
495
496
497
498
499
500
501
502
503
504
505
506
507
508
509
510
511
512
513
514
515
516
517
518
519
520
521
522
523
524
525
526
527
528
529
530
531
532
533
534
535
536
537
538
539
540
541
542
543
544
545
546
547
548
549
550
551
552
553
554
555
556
557
558
559
560
561
562
563
564
565
566
567
568
569
570
571
572
573
574
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 243b696..5dd4b34 100644
--- a/CHANGELOG.md
+++ b/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
 ------
 
diff --git a/include/git2/config.h b/include/git2/config.h
index 6d3fdb0..05c3ad6 100644
--- a/include/git2/config.h
+++ b/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
diff --git a/include/git2/sys/config.h b/include/git2/sys/config.h
index 044e344..4dad6da 100644
--- a/include/git2/sys/config.h
+++ b/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
diff --git a/src/config.c b/src/config.c
index 77cf573..2df1fc8 100644
--- a/src/config.c
+++ b/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
  ***********/
diff --git a/src/config.h b/src/config.h
index f257cc9..ba74533 100644
--- a/src/config.h
+++ b/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
diff --git a/src/config_file.c b/src/config_file.c
index 52a5376..a3fec1b 100644
--- a/src/config_file.c
+++ b/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 ((result = git_filebuf_open(
-		&file, cfg->file_path, 0, GIT_CONFIG_FILE_MODE)) < 0) {
+	if (cfg->locked) {
+		result = git_buf_puts(&reader->buffer, git_buf_cstr(&cfg->locked_content));
+	} 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 */
-	result = git_futils_readbuffer(&reader->buffer, cfg->file_path);
+		/* We need to read in our own config file */
+		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 */
-	(void)git_filebuf_stats(&reader->file_mtime, &reader->file_size, &file);
+	if (cfg->locked) {
+		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);
-	git_buf_free(&reader->buffer);
+		/* refresh stats - if this errors, then commit will error too */
+		(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;
 }
diff --git a/src/config_file.h b/src/config_file.h
index 0d8bf74..1c52892 100644
--- a/src/config_file.h
+++ b/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
diff --git a/src/transaction.c b/src/transaction.c
index e833189..e9639bf 100644
--- a/src/transaction.c
+++ b/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))
diff --git a/src/transaction.h b/src/transaction.h
new file mode 100644
index 0000000..780c068
--- /dev/null
+++ b/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
diff --git a/tests/config/write.c b/tests/config/write.c
index 2e7b818..3d9b1a1 100644
--- a/tests/config/write.c
+++ b/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);
+}