Commit 83bcd3a1911c136b4ace33fecde889c347452718
2017-05-31T22:45:25
config_file: refresh all files if includes were modified Currently, we only re-parse the top-level configuration file when it has changed itself. This can cause problems when an include is changed, as we were not updating all values correctly. Instead of conditionally reparsing only refreshed files, the logic becomes much clearer and easier to follow if we always re-parse the top-level configuration file when either the file itself or one of its included configuration files has changed on disk. This commit implements this logic. Note that this might impact performance in some cases, as we need to re-read all configuration files whenever any of the included files changed. It could increase performance to just re-parse include files which have actually changed, but this would compromise maintainability of the code without much gain. The only case where we will gain anything is when we actually use includes and when only these includes are updated, which will probably be quite an unusual scenario to actually be worthwhile to optimize.
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
diff --git a/src/config_file.c b/src/config_file.c
index 79aea5c..f5770d6 100644
--- a/src/config_file.c
+++ b/src/config_file.c
@@ -317,22 +317,35 @@ static int config_open(git_config_backend *cfg, git_config_level_t level)
return res;
}
-/* The meat of the refresh, as we want to use it in different places */
-static int config__refresh(diskfile_backend *b, struct reader *reader, refcounted_strmap *values)
+static int config_is_modified(int *modified, struct config_file *file)
{
- int error = 0;
+ struct config_file *include;
+ git_buf buf = GIT_BUF_INIT;
+ git_oid hash;
+ uint32_t i;
+ int error;
- if ((error = git_mutex_lock(&b->header.values_mutex)) < 0) {
- giterr_set(GITERR_OS, "failed to lock config backend");
+ *modified = 0;
+
+ if ((error = git_futils_readbuffer(&buf, file->path)) < 0)
goto out;
- }
- if ((error = config_read(values->values, reader, b->level, 0)) < 0)
+ if ((error = git_hash_buf(&hash, buf.ptr, buf.size)) < 0)
goto out;
- git_mutex_unlock(&b->header.values_mutex);
+ if (!git_oid_equal(&hash, &file->checksum)) {
+ *modified = 1;
+ goto out;
+ }
+
+ git_array_foreach(file->includes, i, include) {
+ if ((error = config_is_modified(modified, include)) < 0 || *modified)
+ goto out;
+ }
out:
+ git_buf_free(&buf);
+
return error;
}
@@ -342,46 +355,43 @@ static int config_refresh(git_config_backend *cfg)
refcounted_strmap *values = NULL, *tmp;
struct config_file *include;
struct reader reader;
- int error, updated;
+ int error, modified;
uint32_t i;
+ error = config_is_modified(&modified, &b->file);
+ if (error < 0 && error != GIT_ENOTFOUND)
+ goto out;
+
+ if (!modified)
+ return 0;
+
reader_init(&reader, &b->file);
error = git_futils_readbuffer_updated(
- &reader.buffer, b->file.path, &b->file.checksum, &updated);
+ &reader.buffer, b->file.path, &b->file.checksum, NULL);
- if (error < 0 && error != GIT_ENOTFOUND)
+ if ((error = refcounted_strmap_alloc(&values)) < 0)
goto out;
- if (updated) {
- if ((error = refcounted_strmap_alloc(&values)) < 0)
- goto out;
-
- /* Reparse the current configuration */
- git_array_foreach(b->file.includes, i, include) {
- config_file_clear(include);
- }
+ /* Reparse the current configuration */
+ git_array_foreach(b->file.includes, i, include) {
+ config_file_clear(include);
+ }
+ git_array_clear(b->file.includes);
- git_array_clear(b->file.includes);
+ if ((error = config_read(values->values, &reader, b->level, 0)) < 0)
+ goto out;
- if ((error = config__refresh(b, &reader, values)) < 0)
- goto out;
+ if ((error = git_mutex_lock(&b->header.values_mutex)) < 0) {
+ giterr_set(GITERR_OS, "failed to lock config backend");
+ goto out;
+ }
- tmp = b->header.values;
- b->header.values = values;
- values = tmp;
- } else {
- /* Refresh included configuration files */
- git_array_foreach(b->file.includes, i, include) {
- git_buf_free(&reader.buffer);
- reader_init(&reader, include);
- error = git_futils_readbuffer_updated(&reader.buffer, b->file.path,
- &b->file.checksum, NULL);
+ tmp = b->header.values;
+ b->header.values = values;
+ values = tmp;
- if ((error = config__refresh(b, &reader, b->header.values)) < 0)
- goto out;
- }
- }
+ git_mutex_unlock(&b->header.values_mutex);
out:
refcounted_strmap_free(values);
diff --git a/tests/config/include.c b/tests/config/include.c
index 0696f28..2ce42bd 100644
--- a/tests/config/include.c
+++ b/tests/config/include.c
@@ -123,3 +123,13 @@ void test_config_include__depth2(void)
cl_git_pass(git_config_get_string_buf(&buf, cfg, "foo.bar2"));
cl_assert_equal_s("baz2", git_buf_cstr(&buf));
}
+
+void test_config_include__removing_include_removes_values(void)
+{
+ cl_git_mkfile("top-level", "[include]\npath = included");
+ cl_git_mkfile("included", "[foo]\nbar = value");
+
+ cl_git_pass(git_config_open_ondisk(&cfg, "top-level"));
+ cl_git_mkfile("top-level", "");
+ cl_git_fail(git_config_get_string_buf(&buf, cfg, "foo.bar"));
+}