Add cache busting to attribute cache

This makes the git attributes and git ignores cache check
stat information before using the file contents from the
cache.  For cached files from the index, it checks the SHA
of the file instead.  This should reduce the need to ever
call `git_attr_cache_flush()` in most situations.

This commit also fixes the `git_status_should_ignore` API
to use the libgit2 standard parameter ordering.

include/git2/status.h

@@ -139,13 +139,13 @@ GIT_EXTERN(int) git_status_file(unsigned int *status_flags, git_repository *repo
  * would be ignored regardless of whether the file is already in the index
  * or in the repository.
  *
+ * @param ignored boolean returning 0 if the file is not ignored, 1 if it is
  * @param repo a repository object
  * @param path the file to check ignores for, rooted at the repo's workdir
- * @param ignored boolean returning 0 if the file is not ignored, 1 if it is
  * @return GIT_SUCCESS if the ignore rules could be processed for the file
  *		(regardless of whether it exists or not), or an error < 0 if they could not.
  */
-GIT_EXTERN(int) git_status_should_ignore(git_repository *repo, const char *path, int *ignored);
+GIT_EXTERN(int) git_status_should_ignore(int *ignored, git_repository *repo, const char *path);
 
 /** @} */
 GIT_END_DECL

src/attr.c

@@ -235,31 +235,91 @@ bool git_attr_cache__is_cached(
 	return rval;
 }
 
-static int load_attr_file(const char *filename, const char **data)
+static int load_attr_file(
+	const char **data,
+	git_attr_file_stat_sig *sig,
+	const char *filename)
 {
 	int error;
 	git_buf content = GIT_BUF_INIT;
+	struct stat st;
 
-	error = git_futils_readbuffer(&content, filename);
-	*data = error ? NULL : git_buf_detach(&content);
+	if (p_stat(filename, &st) < 0)
+		return GIT_ENOTFOUND;
 
+	if (sig != NULL &&
+		(git_time_t)st.st_mtime == sig->seconds &&
+		(git_off_t)st.st_size == sig->size &&
+		(unsigned int)st.st_ino == sig->ino)
+		return GIT_ENOTFOUND;
+
+	error = git_futils_readbuffer_updated(&content, filename, NULL, NULL);
+	if (error < 0)
 		return error;
+
+	if (sig != NULL) {
+		sig->seconds = (git_time_t)st.st_mtime;
+		sig->size    = (git_off_t)st.st_size;
+		sig->ino     = (unsigned int)st.st_ino;
+	}
+
+	*data = git_buf_detach(&content);
+
+	return 0;
 }
 
 static int load_attr_blob_from_index(
-	git_repository *repo, const char *filename, git_blob **blob)
+	const char **content,
+	git_blob **blob,
+	git_repository *repo,
+	const git_oid *old_oid,
+	const char *relfile)
 {
 	int error;
 	git_index *index;
 	git_index_entry *entry;
 
 	if ((error = git_repository_index__weakptr(&index, repo)) < 0 ||
-		(error = git_index_find(index, filename)) < 0)
+		(error = git_index_find(index, relfile)) < 0)
 		return error;
 
 	entry = git_index_get(index, error);
 
-	return git_blob_lookup(blob, repo, &entry->oid);
+	if (old_oid && git_oid_cmp(old_oid, &entry->oid) == 0)
+		return GIT_ENOTFOUND;
+
+	if ((error = git_blob_lookup(blob, repo, &entry->oid)) < 0)
+		return error;
+
+	*content = git_blob_rawcontent(*blob);
+	return 0;
+}
+
+static int load_attr_from_cache(
+	git_attr_file **file,
+	git_attr_cache *cache,
+	git_attr_file_source source,
+	const char *relative_path)
+{
+	git_buf  cache_key = GIT_BUF_INIT;
+	khiter_t cache_pos;
+
+	*file = NULL;
+
+	if (!cache || !cache->files)
+		return 0;
+
+	if (git_buf_printf(&cache_key, "%d#%s", (int)source, relative_path) < 0)
+		return -1;
+
+	cache_pos = git_strmap_lookup_index(cache->files, cache_key.ptr);
+
+	git_buf_free(&cache_key);
+
+	if (git_strmap_valid_index(cache->files, cache_pos))
+		*file = git_strmap_value_at(cache->files, cache_pos);
+
+	return 0;
 }
 
 int git_attr_cache__internal_file(
@@ -301,6 +361,7 @@ int git_attr_cache__push_file(
 	git_attr_cache *cache = git_repository_attr_cache(repo);
 	git_attr_file *file = NULL;
 	git_blob *blob = NULL;
+	git_attr_file_stat_sig st;
 
 	assert(filename && stack);
 
@@ -316,29 +377,22 @@ int git_attr_cache__push_file(
 		relfile += strlen(workdir);
 
 	/* check cache */
-	if (cache && cache->files) {
-		git_buf  cache_key = GIT_BUF_INIT;
-		khiter_t cache_pos;
-
-		if (git_buf_printf(&cache_key, "%d#%s", (int)source, relfile) < 0)
+	if (load_attr_from_cache(&file, cache, source, relfile) < 0)
 		return -1;
 
-		cache_pos = git_strmap_lookup_index(cache->files, cache_key.ptr);
-
-		git_buf_free(&cache_key);
-
-		if (git_strmap_valid_index(cache->files, cache_pos)) {
-			file = git_strmap_value_at(cache->files, cache_pos);
-			goto finish;
-		}
-	}
-
 	/* if not in cache, load data, parse, and cache */
 
-	if (source == GIT_ATTR_FILE_FROM_FILE)
-		error = load_attr_file(filename, &content);
+	if (source == GIT_ATTR_FILE_FROM_FILE) {
+		if (file)
+			memcpy(&st, &file->cache_data.st, sizeof(st));
 		else
-		error = load_attr_blob_from_index(repo, relfile, &blob);
+			memset(&st, 0, sizeof(st));
+
+		error = load_attr_file(&content, &st, filename);
+	} else {
+		error = load_attr_blob_from_index(&content, &blob,
+			repo, file ? &file->cache_data.oid : NULL, relfile);
+	}
 
 	if (error) {
 		/* not finding a file is not an error for this function */
@@ -349,10 +403,8 @@ int git_attr_cache__push_file(
 		goto finish;
 	}
 
-	if (blob)
-		content = git_blob_rawcontent(blob);
-
-	if ((error = git_attr_file__new(&file, source, relfile, &cache->pool)) < 0)
+	if (!file &&
+		(error = git_attr_file__new(&file, source, relfile, &cache->pool)) < 0)
 		goto finish;
 
 	if (parse && (error = parse(repo, content, file)) < 0)
@@ -362,6 +414,12 @@ int git_attr_cache__push_file(
 	if (error > 0)
 		error = 0;
 
+	/* remember "cache buster" file signature */
+	if (blob)
+		git_oid_cpy(&file->cache_data.oid, git_object_id((git_object *)blob));
+	else
+		memcpy(&file->cache_data.st, &st, sizeof(st));
+
 finish:
 	/* push file onto vector if we found one*/
 	if (!error && file != NULL)

src/attr_file.h

@@ -48,10 +48,20 @@ typedef struct {
 } git_attr_assignment;
 
 typedef struct {
+	git_time_t seconds;
+	git_off_t  size;
+	unsigned int ino;
+} git_attr_file_stat_sig;
+
+typedef struct {
 	char *key;				/* cache "source#path" this was loaded from */
 	git_vector rules;		/* vector of <rule*> or <fnmatch*> */
 	git_pool *pool;
 	bool pool_is_allocated;
+	union {
+		git_oid oid;
+		git_attr_file_stat_sig st;
+	} cache_data;
 } git_attr_file;
 
 typedef struct {

src/fileops.c

@@ -185,9 +185,6 @@ int git_futils_readbuffer_updated(git_buf *buf, const char *path, time_t *mtime,
 
 	p_close(fd);
 
-	if (mtime != NULL)
-		*mtime = st.st_mtime;
-
 	if (updated != NULL)
 		*updated = 1;
 

src/status.c

@@ -400,7 +400,8 @@ cleanup:
 	return error;
 }
 
-int git_status_should_ignore(git_repository *repo, const char *path, int *ignored)
+int git_status_should_ignore(
+	int *ignored, git_repository *repo, const char *path)
 {
 	int error;
 	git_ignores ignores;

tests-clar/status/ignore.c

@@ -2,12 +2,12 @@
 #include "fileops.h"
 #include "git2/attr.h"
 #include "attr.h"
+#include "status_helpers.h"
 
 static git_repository *g_repo = NULL;
 
 void test_status_ignore__initialize(void)
 {
-	g_repo = cl_git_sandbox_init("attr");
 }
 
 void test_status_ignore__cleanup(void)
@@ -40,9 +40,11 @@ void test_status_ignore__0(void)
 		{ NULL, 0 }
 	}, *one_test;
 
+	g_repo = cl_git_sandbox_init("attr");
+
 	for (one_test = test_cases; one_test->path != NULL; one_test++) {
 		int ignored;
-		cl_git_pass(git_status_should_ignore(g_repo, one_test->path, &ignored));
+		cl_git_pass(git_status_should_ignore(&ignored, g_repo, one_test->path));
 		cl_assert_(ignored == one_test->expected, one_test->path);
 	}
 
@@ -56,25 +58,76 @@ void test_status_ignore__1(void)
 {
 	int ignored;
 
+	g_repo = cl_git_sandbox_init("attr");
+
 	cl_git_rewritefile("attr/.gitignore", "/*.txt\n/dir/\n");
 	git_attr_cache_flush(g_repo);
 
-	cl_git_pass(git_status_should_ignore(g_repo, "root_test4.txt", &ignored));
+	cl_git_pass(git_status_should_ignore(&ignored, g_repo, "root_test4.txt"));
 	cl_assert(ignored);
 
-	cl_git_pass(git_status_should_ignore(g_repo, "sub/subdir_test2.txt", &ignored));
+	cl_git_pass(git_status_should_ignore(&ignored, g_repo, "sub/subdir_test2.txt"));
 	cl_assert(!ignored);
 
-	cl_git_pass(git_status_should_ignore(g_repo, "dir", &ignored));
+	cl_git_pass(git_status_should_ignore(&ignored, g_repo, "dir"));
 	cl_assert(ignored);
 
-	cl_git_pass(git_status_should_ignore(g_repo, "dir/", &ignored));
+	cl_git_pass(git_status_should_ignore(&ignored, g_repo, "dir/"));
 	cl_assert(ignored);
 
-	cl_git_pass(git_status_should_ignore(g_repo, "sub/dir", &ignored));
+	cl_git_pass(git_status_should_ignore(&ignored, g_repo, "sub/dir"));
+	cl_assert(!ignored);
+
+	cl_git_pass(git_status_should_ignore(&ignored, g_repo, "sub/dir/"));
+	cl_assert(!ignored);
+}
+
+
+void test_status_ignore__empty_repo_with_gitignore_rewrite(void)
+{
+	status_entry_single st;
+	int ignored;
+
+	g_repo = cl_git_sandbox_init("empty_standard_repo");
+
+	cl_git_mkfile(
+		"empty_standard_repo/look-ma.txt", "I'm going to be ignored!");
+
+	memset(&st, 0, sizeof(st));
+	cl_git_pass(git_status_foreach(g_repo, cb_status__single, &st));
+	cl_assert(st.count == 1);
+	cl_assert(st.status == GIT_STATUS_WT_NEW);
+
+	cl_git_pass(git_status_file(&st.status, g_repo, "look-ma.txt"));
+	cl_assert(st.status == GIT_STATUS_WT_NEW);
+
+	cl_git_pass(git_status_should_ignore(&ignored, g_repo, "look-ma.txt"));
 	cl_assert(!ignored);
 
-	cl_git_pass(git_status_should_ignore(g_repo, "sub/dir/", &ignored));
+	cl_git_rewritefile("empty_standard_repo/.gitignore", "*.nomatch\n");
+
+	memset(&st, 0, sizeof(st));
+	cl_git_pass(git_status_foreach(g_repo, cb_status__single, &st));
+	cl_assert(st.count == 2);
+	cl_assert(st.status == GIT_STATUS_WT_NEW);
+
+	cl_git_pass(git_status_file(&st.status, g_repo, "look-ma.txt"));
+	cl_assert(st.status == GIT_STATUS_WT_NEW);
+
+	cl_git_pass(git_status_should_ignore(&ignored, g_repo, "look-ma.txt"));
 	cl_assert(!ignored);
+
+	cl_git_rewritefile("empty_standard_repo/.gitignore", "*.txt\n");
+
+	memset(&st, 0, sizeof(st));
+	cl_git_pass(git_status_foreach(g_repo, cb_status__single, &st));
+	cl_assert(st.count == 2);
+	cl_assert(st.status == GIT_STATUS_IGNORED);
+
+	cl_git_pass(git_status_file(&st.status, g_repo, "look-ma.txt"));
+	cl_assert(st.status == GIT_STATUS_IGNORED);
+
+	cl_git_pass(git_status_should_ignore(&ignored, g_repo, "look-ma.txt"));
+	cl_assert(ignored);
 }
 

tests-clar/status/status_data.h

-
-struct status_entry_counts {
-	size_t wrong_status_flags_count;
-	size_t wrong_sorted_path;
-	size_t entry_count;
-	const unsigned int* expected_statuses;
-	const char** expected_paths;
-	size_t expected_entry_count;
-};
+#include "status_helpers.h"
 
 /* entries for a plain copy of tests/resources/status */
 

tests-clar/status/status_helpers.c

+#include "clar_libgit2.h"
+#include "status_helpers.h"
+
+int cb_status__normal(
+	const char *path, unsigned int status_flags, void *payload)
+{
+	status_entry_counts *counts = payload;
+
+	if (counts->entry_count >= counts->expected_entry_count) {
+		counts->wrong_status_flags_count++;
+		goto exit;
+	}
+
+	if (strcmp(path, counts->expected_paths[counts->entry_count])) {
+		counts->wrong_sorted_path++;
+		goto exit;
+	}
+
+	if (status_flags != counts->expected_statuses[counts->entry_count])
+		counts->wrong_status_flags_count++;
+
+exit:
+	counts->entry_count++;
+	return 0;
+}
+
+int cb_status__count(const char *p, unsigned int s, void *payload)
+{
+	volatile int *count = (int *)payload;
+
+	GIT_UNUSED(p);
+	GIT_UNUSED(s);
+
+	(*count)++;
+
+	return 0;
+}
+
+int cb_status__single(const char *p, unsigned int s, void *payload)
+{
+	status_entry_single *data = (status_entry_single *)payload;
+
+	GIT_UNUSED(p);
+
+	data->count++;
+	data->status = s;
+
+	return 0;
+}

tests-clar/status/status_helpers.h

+#ifndef INCLUDE_cl_status_helpers_h__
+#define INCLUDE_cl_status_helpers_h__
+
+typedef struct {
+	size_t wrong_status_flags_count;
+	size_t wrong_sorted_path;
+	size_t entry_count;
+	const unsigned int* expected_statuses;
+	const char** expected_paths;
+	size_t expected_entry_count;
+} status_entry_counts;
+
+/* cb_status__normal takes payload of "status_entry_counts *" */
+
+extern int cb_status__normal(
+	const char *path, unsigned int status_flags, void *payload);
+
+
+/* cb_status__count takes payload of "int *" */
+
+extern int cb_status__count(const char *p, unsigned int s, void *payload);
+
+
+typedef struct {
+	int count;
+	unsigned int status;
+} status_entry_single;
+
+/* cb_status__single takes payload of "status_entry_single *" */
+
+extern int cb_status__single(const char *p, unsigned int s, void *payload);
+
+#endif

tests-clar/status/submodules.c

@@ -2,6 +2,7 @@
 #include "buffer.h"
 #include "path.h"
 #include "posix.h"
+#include "status_helpers.h"
 
 static git_repository *g_repo = NULL;
 
@@ -43,19 +44,6 @@ void test_status_submodules__api(void)
 	cl_assert_equal_s("testrepo", sm->path);
 }
 
-static int
-cb_status__submodule_count(const char *p, unsigned int s, void *payload)
-{
-	volatile int *count = (int *)payload;
-
-	GIT_UNUSED(p);
-	GIT_UNUSED(s);
-
-	(*count)++;
-
-	return 0;
-}
-
 void test_status_submodules__0(void)
 {
 	int counts = 0;
@@ -65,7 +53,7 @@ void test_status_submodules__0(void)
 	cl_assert(git_path_isfile("submodules/.gitmodules"));
 
 	cl_git_pass(
-		git_status_foreach(g_repo, cb_status__submodule_count, &counts)
+		git_status_foreach(g_repo, cb_status__count, &counts)
 	);
 
 	cl_assert(counts == 6);

tests-clar/status/worktree.c

@@ -7,45 +7,6 @@
 #include "path.h"
 
 /**
- * Auxiliary methods
- */
-static int
-cb_status__normal( const char *path, unsigned int status_flags, void *payload)
-{
-	struct status_entry_counts *counts = payload;
-
-	if (counts->entry_count >= counts->expected_entry_count) {
-		counts->wrong_status_flags_count++;
-		goto exit;
-	}
-
-	if (strcmp(path, counts->expected_paths[counts->entry_count])) {
-		counts->wrong_sorted_path++;
-		goto exit;
-	}
-
-	if (status_flags != counts->expected_statuses[counts->entry_count])
-		counts->wrong_status_flags_count++;
-
-exit:
-	counts->entry_count++;
-	return 0;
-}
-
-static int
-cb_status__count(const char *p, unsigned int s, void *payload)
-{
-	volatile int *count = (int *)payload;
-
-	GIT_UNUSED(p);
-	GIT_UNUSED(s);
-
-	(*count)++;
-
-	return 0;
-}
-
-/**
  * Initializer
  *
  * Not all of the tests in this file use the same fixtures, so we allow each
@@ -72,10 +33,10 @@ void test_status_worktree__cleanup(void)
 /* this test is equivalent to t18-status.c:statuscb0 */
 void test_status_worktree__whole_repository(void)
 {
-	struct status_entry_counts counts;
+	status_entry_counts counts;
 	git_repository *repo = cl_git_sandbox_init("status");
 
-	memset(&counts, 0x0, sizeof(struct status_entry_counts));
+	memset(&counts, 0x0, sizeof(status_entry_counts));
 	counts.expected_entry_count = entry_count0;
 	counts.expected_paths = entry_paths0;
 	counts.expected_statuses = entry_statuses0;
@@ -120,7 +81,7 @@ static int remove_file_cb(void *data, git_buf *file)
 /* this test is equivalent to t18-status.c:statuscb2 */
 void test_status_worktree__purged_worktree(void)
 {
-	struct status_entry_counts counts;
+	status_entry_counts counts;
 	git_repository *repo = cl_git_sandbox_init("status");
 	git_buf workdir = GIT_BUF_INIT;
 
@@ -130,7 +91,7 @@ void test_status_worktree__purged_worktree(void)
 	git_buf_free(&workdir);
 
 	/* now get status */
-	memset(&counts, 0x0, sizeof(struct status_entry_counts));
+	memset(&counts, 0x0, sizeof(status_entry_counts));
 	counts.expected_entry_count = entry_count2;
 	counts.expected_paths = entry_paths2;
 	counts.expected_statuses = entry_statuses2;
@@ -147,7 +108,7 @@ void test_status_worktree__purged_worktree(void)
 /* this test is similar to t18-status.c:statuscb3 */
 void test_status_worktree__swap_subdir_and_file(void)
 {
-	struct status_entry_counts counts;
+	status_entry_counts counts;
 	git_repository *repo = cl_git_sandbox_init("status");
 	git_status_options opts;
 
@@ -161,7 +122,7 @@ void test_status_worktree__swap_subdir_and_file(void)
 	cl_git_mkfile("status/README.md", "dummy");
 
 	/* now get status */
-	memset(&counts, 0x0, sizeof(struct status_entry_counts));
+	memset(&counts, 0x0, sizeof(status_entry_counts));
 	counts.expected_entry_count = entry_count3;
 	counts.expected_paths = entry_paths3;
 	counts.expected_statuses = entry_statuses3;
@@ -182,7 +143,7 @@ void test_status_worktree__swap_subdir_and_file(void)
 
 void test_status_worktree__swap_subdir_with_recurse_and_pathspec(void)
 {
-	struct status_entry_counts counts;
+	status_entry_counts counts;
 	git_repository *repo = cl_git_sandbox_init("status");
 	git_status_options opts;
 
@@ -196,7 +157,7 @@ void test_status_worktree__swap_subdir_with_recurse_and_pathspec(void)
 	cl_git_mkfile("status/zzz_new_file", "dummy");
 
 	/* now get status */
-	memset(&counts, 0x0, sizeof(struct status_entry_counts));
+	memset(&counts, 0x0, sizeof(status_entry_counts));
 	counts.expected_entry_count = entry_count4;
 	counts.expected_paths = entry_paths4;
 	counts.expected_statuses = entry_statuses4;
@@ -286,18 +247,18 @@ void test_status_worktree__ignores(void)
 
 	for (i = 0; i < (int)entry_count0; i++) {
 		cl_git_pass(
-			git_status_should_ignore(repo, entry_paths0[i], &ignored)
+			git_status_should_ignore(&ignored, repo, entry_paths0[i])
 		);
 		cl_assert(ignored == (entry_statuses0[i] == GIT_STATUS_IGNORED));
 	}
 
 	cl_git_pass(
-		git_status_should_ignore(repo, "nonexistent_file", &ignored)
+		git_status_should_ignore(&ignored, repo, "nonexistent_file")
 	);
 	cl_assert(!ignored);
 
 	cl_git_pass(
-		git_status_should_ignore(repo, "ignored_nonexistent_file", &ignored)
+		git_status_should_ignore(&ignored, repo, "ignored_nonexistent_file")
 	);
 	cl_assert(ignored);
 }
@@ -402,24 +363,6 @@ void test_status_worktree__cannot_retrieve_the_status_of_a_bare_repository(void)
 	git_repository_free(repo);
 }
 
-typedef struct {
-	int count;
-	unsigned int status;
-} status_entry_single;
-
-static int
-cb_status__single(const char *p, unsigned int s, void *payload)
-{
-	status_entry_single *data = (status_entry_single *)payload;
-
-	GIT_UNUSED(p);
-
-	data->count++;
-	data->status = s;
-
-	return 0;
-}
-
 void test_status_worktree__first_commit_in_progress(void)
 {
 	git_repository *repo;