Merge pull request #3491 from libgit2/cmn/config-checksum

Use checksums to detect config file changes

4 files changed, 59 insertions, 36 deletions

diff --git a/src/config_file.c b/src/config_file.c
index 46f21c0f1..5f5e309e0 100644
--- a/src/config_file.c
+++ b/src/config_file.c
@@ -77,8 +77,7 @@ typedef struct git_config_file_iter {
 		 (iter) = (tmp))
 
 struct reader {
-	time_t file_mtime;
-	size_t file_size;
+	git_oid checksum;
 	char *file_path;
 	git_buf buffer;
 	char *read_ptr;
@@ -285,7 +284,7 @@ static int config_open(git_config_backend *cfg, git_config_level_t level)
 
 	git_buf_init(&reader->buffer, 0);
 	res = git_futils_readbuffer_updated(
-		&reader->buffer, b->file_path, &reader->file_mtime, &reader->file_size, NULL);
+		&reader->buffer, b->file_path, &reader->checksum, NULL);
 
 	/* It's fine if the file doesn't exist */
 	if (res == GIT_ENOTFOUND)
@@ -345,7 +344,7 @@ static int config_refresh(git_config_backend *cfg)
 		reader = git_array_get(b->readers, i);
 		error = git_futils_readbuffer_updated(
 			&reader->buffer, reader->file_path,
-			&reader->file_mtime, &reader->file_size, &updated);
+			&reader->checksum, &updated);
 
 		if (error < 0 && error != GIT_ENOTFOUND)
 			return error;
@@ -1618,7 +1617,7 @@ static int read_on_variable(
 		git_buf_init(&r->buffer, 0);
 
 		result = git_futils_readbuffer_updated(
-			&r->buffer, r->file_path, &r->file_mtime, &r->file_size, NULL);
+			&r->buffer, r->file_path, &r->checksum, NULL);
 
 		if (result == 0) {
 			result = config_read(parse_data->values, parse_data->cfg_file, r, parse_data->level, parse_data->depth+1);
@@ -1894,7 +1893,7 @@ static int config_write(diskfile_backend *cfg, const char *key, const regex_t *p
 	} else {
 		/* Lock the file */
 		if ((result = git_filebuf_open(
-			     &file, cfg->file_path, 0, GIT_CONFIG_FILE_MODE)) < 0) {
+			     &file, cfg->file_path, GIT_FILEBUF_HASH_CONTENTS, GIT_CONFIG_FILE_MODE)) < 0) {
 			git_buf_free(&reader->buffer);
 			return result;
 		}
@@ -1945,10 +1944,6 @@ static int config_write(diskfile_backend *cfg, const char *key, const regex_t *p
 		git_buf_attach(&cfg->locked_content, git_buf_detach(&buf), len);
 	} else {
 		git_filebuf_write(&file, git_buf_cstr(&buf), git_buf_len(&buf));
-
-		/* 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);
 	}
 
diff --git a/src/fileops.c b/src/fileops.c
index 57d2ce9c3..cfc22bb53 100644
--- a/src/fileops.c
+++ b/src/fileops.c
@@ -153,13 +153,15 @@ int git_futils_readbuffer_fd(git_buf *buf, git_file fd, size_t len)
 }
 
 int git_futils_readbuffer_updated(
-	git_buf *buf, const char *path, time_t *mtime, size_t *size, int *updated)
+	git_buf *out, const char *path, git_oid *checksum, int *updated)
 {
+	int error;
 	git_file fd;
 	struct stat st;
-	bool changed = false;
+	git_buf buf = GIT_BUF_INIT;
+	git_oid checksum_new;
 
-	assert(buf && path && *path);
+	assert(out && path && *path);
 
 	if (updated != NULL)
 		*updated = 0;
@@ -178,45 +180,50 @@ int git_futils_readbuffer_updated(
 		return -1;
 	}
 
-	/*
-	 * If we were given a time and/or a size, we only want to read the file
-	 * if it has been modified.
-	 */
-	if (size && *size != (size_t)st.st_size)
-		changed = true;
-	if (mtime && *mtime != (time_t)st.st_mtime)
-		changed = true;
-	if (!size && !mtime)
-		changed = true;
-
-	if (!changed) {
-		return 0;
-	}
-
-	if (mtime != NULL)
-		*mtime = st.st_mtime;
-	if (size != NULL)
-		*size = (size_t)st.st_size;
-
 	if ((fd = git_futils_open_ro(path)) < 0)
 		return fd;
 
-	if (git_futils_readbuffer_fd(buf, fd, (size_t)st.st_size) < 0) {
+	if (git_futils_readbuffer_fd(&buf, fd, (size_t)st.st_size) < 0) {
 		p_close(fd);
 		return -1;
 	}
 
 	p_close(fd);
 
+	if ((error = git_hash_buf(&checksum_new, buf.ptr, buf.size)) < 0) {
+		git_buf_free(&buf);
+		return error;
+	}
+
+	/*
+	 * If we were given a checksum, we only want to use it if it's different
+	 */
+	if (checksum && !git_oid__cmp(checksum, &checksum_new)) {
+		git_buf_free(&buf);
+		if (updated)
+			*updated = 0;
+
+		return 0;
+	}
+
+	/*
+	 * If we're here, the file did change, or the user didn't have an old version
+	 */
+	if (checksum)
+		git_oid_cpy(checksum, &checksum_new);
+
 	if (updated != NULL)
 		*updated = 1;
 
+	git_buf_swap(out, &buf);
+	git_buf_free(&buf);
+
 	return 0;
 }
 
 int git_futils_readbuffer(git_buf *buf, const char *path)
 {
-	return git_futils_readbuffer_updated(buf, path, NULL, NULL, NULL);
+	return git_futils_readbuffer_updated(buf, path, NULL, NULL);
 }
 
 int git_futils_writebuffer(
diff --git a/src/fileops.h b/src/fileops.h
index 572ff01a5..c6db1953e 100644
--- a/src/fileops.h
+++ b/src/fileops.h
@@ -13,6 +13,7 @@
 #include "path.h"
 #include "pool.h"
 #include "strmap.h"
+#include "oid.h"
 
 /**
  * Filebuffer methods
@@ -21,7 +22,7 @@
  */
 extern int git_futils_readbuffer(git_buf *obj, const char *path);
 extern int git_futils_readbuffer_updated(
-	git_buf *obj, const char *path, time_t *mtime, size_t *size, int *updated);
+	git_buf *obj, const char *path, git_oid *checksum, int *updated);
 extern int git_futils_readbuffer_fd(git_buf *obj, git_file fd, size_t len);
 
 extern int git_futils_writebuffer(
diff --git a/tests/config/stress.c b/tests/config/stress.c
index 503f44f03..6e960425c 100644
--- a/tests/config/stress.c
+++ b/tests/config/stress.c
@@ -107,3 +107,23 @@ void test_config_stress__complex(void)
 
 	git_config_free(config);
 }
+
+void test_config_stress__quick_write(void)
+{
+	git_config *config_w, *config_r;
+	const char *path = "./config-quick-write";
+	const char *key = "quick.write";
+	int32_t i;
+
+	/* Create an external writer for one instance with the other one */
+	cl_git_pass(git_config_open_ondisk(&config_w, path));
+	cl_git_pass(git_config_open_ondisk(&config_r, path));
+
+	/* Write and read in the same second (repeat to increase the chance of it happening) */
+	for (i = 0; i < 10; i++) {
+		int32_t val;
+		cl_git_pass(git_config_set_int32(config_w, key, i));
+		cl_git_pass(git_config_get_int32(&val, config_r, key));
+		cl_assert_equal_i(i, val);
+	}
+}