Fix broken logic for attr cache invalidation

The checks to see if files were out of date in the attibute cache
was wrong because the cache-breaker data wasn't getting stored
correctly.  Additionally, when the cache-breaker triggered, the
old file data was being leaked.

CMakeLists.txt

@@ -305,6 +305,11 @@ ELSE ()
 	ENDIF ()
 	IF (APPLE) # Apple deprecated OpenSSL
 		SET(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wno-deprecated-declarations")
+
+		# With clang, disable some annoying extra warnings
+		IF (NOT CMAKE_COMPILER_IS_GNUCC)
+			SET(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wno-unused-const-variable -Wno-unused-function")
+		ENDIF()
 	ENDIF ()
 	IF (PROFILE)
 		SET(CMAKE_C_FLAGS "-pg ${CMAKE_C_FLAGS}")

src/attr.c

@@ -260,26 +260,26 @@ typedef struct {
 } attr_walk_up_info;
 
 static int attr_decide_sources(
-	uint32_t flags, bool has_wd, bool has_index, git_attr_cache_source *srcs)
+	uint32_t flags, bool has_wd, bool has_index, git_attr_file_source *srcs)
 {
 	int count = 0;
 
 	switch (flags & 0x03) {
 	case GIT_ATTR_CHECK_FILE_THEN_INDEX:
 		if (has_wd)
-			srcs[count++] = GIT_ATTR_CACHE__FROM_FILE;
+			srcs[count++] = GIT_ATTR_FILE__FROM_FILE;
 		if (has_index)
-			srcs[count++] = GIT_ATTR_CACHE__FROM_INDEX;
+			srcs[count++] = GIT_ATTR_FILE__FROM_INDEX;
 		break;
 	case GIT_ATTR_CHECK_INDEX_THEN_FILE:
 		if (has_index)
-			srcs[count++] = GIT_ATTR_CACHE__FROM_INDEX;
+			srcs[count++] = GIT_ATTR_FILE__FROM_INDEX;
 		if (has_wd)
-			srcs[count++] = GIT_ATTR_CACHE__FROM_FILE;
+			srcs[count++] = GIT_ATTR_FILE__FROM_FILE;
 		break;
 	case GIT_ATTR_CHECK_INDEX_ONLY:
 		if (has_index)
-			srcs[count++] = GIT_ATTR_CACHE__FROM_INDEX;
+			srcs[count++] = GIT_ATTR_FILE__FROM_INDEX;
 		break;
 	}
 
@@ -289,7 +289,7 @@ static int attr_decide_sources(
 static int push_attr_file(
 	git_repository *repo,
 	git_vector *list,
-	git_attr_cache_source source,
+	git_attr_file_source source,
 	const char *base,
 	const char *filename)
 {
@@ -297,7 +297,7 @@ static int push_attr_file(
 	git_attr_file *file = NULL;
 
 	error = git_attr_cache__get(
-		&file, repo, source, base, filename, git_attr_file__parse_buffer, NULL);
+		&file, repo, source, base, filename, git_attr_file__parse_buffer);
 	if (error < 0)
 		return error;
 
@@ -313,7 +313,7 @@ static int push_one_attr(void *ref, git_buf *path)
 {
 	int error = 0, n_src, i;
 	attr_walk_up_info *info = (attr_walk_up_info *)ref;
-	git_attr_cache_source src[2];
+	git_attr_file_source src[2];
 
 	n_src = attr_decide_sources(
 		info->flags, info->workdir != NULL, info->index != NULL, src);
@@ -367,7 +367,7 @@ static int collect_attr_files(
 	 */
 
 	error = push_attr_file(
-		repo, files, GIT_ATTR_CACHE__FROM_FILE,
+		repo, files, GIT_ATTR_FILE__FROM_FILE,
 		git_repository_path(repo), GIT_ATTR_FILE_INREPO);
 	if (error < 0)
 		goto cleanup;
@@ -385,7 +385,7 @@ static int collect_attr_files(
 
 	if (git_repository_attr_cache(repo)->cfg_attr_file != NULL) {
 		error = push_attr_file(
-			repo, files, GIT_ATTR_CACHE__FROM_FILE,
+			repo, files, GIT_ATTR_FILE__FROM_FILE,
 			NULL, git_repository_attr_cache(repo)->cfg_attr_file);
 		if (error < 0)
 			goto cleanup;
@@ -395,7 +395,7 @@ static int collect_attr_files(
 		error = git_sysdir_find_system_file(&dir, GIT_ATTR_FILE_SYSTEM);
 		if (!error)
 			error = push_attr_file(
-				repo, files, GIT_ATTR_CACHE__FROM_FILE, NULL, dir.ptr);
+				repo, files, GIT_ATTR_FILE__FROM_FILE, NULL, dir.ptr);
 		else if (error == GIT_ENOTFOUND) {
 			giterr_clear();
 			error = 0;

src/attr_file.c

@@ -2,6 +2,7 @@
 #include "repository.h"
 #include "filebuf.h"
 #include "attr_file.h"
+#include "attrcache.h"
 #include "git2/blob.h"
 #include "git2/tree.h"
 #include "index.h"
@@ -22,8 +23,8 @@ static void attr_file_free(git_attr_file *file)
 
 int git_attr_file__new(
 	git_attr_file **out,
-	git_attr_cache_entry *ce,
-	git_attr_cache_source source)
+	git_attr_file_entry *entry,
+	git_attr_file_source source)
 {
 	git_attr_file *attrs = git__calloc(1, sizeof(git_attr_file));
 	GITERR_CHECK_ALLOC(attrs);
@@ -40,7 +41,7 @@ int git_attr_file__new(
 	}
 
 	GIT_REFCOUNT_INC(attrs);
-	attrs->ce = ce;
+	attrs->entry  = entry;
 	attrs->source = source;
 	*out = attrs;
 	return 0;
@@ -95,42 +96,77 @@ static int attr_file_oid_from_index(
 int git_attr_file__load(
 	git_attr_file **out,
 	git_repository *repo,
-	git_attr_cache_entry *ce,
-	git_attr_cache_source source,
-	git_attr_cache_parser parser,
-	void *payload)
+	git_attr_file_entry *entry,
+	git_attr_file_source source,
+	git_attr_file_parser parser)
 {
 	int error = 0;
 	git_blob *blob = NULL;
 	git_buf content = GIT_BUF_INIT;
 	const char *data = NULL;
 	git_attr_file *file;
+	struct stat st;
 
 	*out = NULL;
 
-	if (source == GIT_ATTR_CACHE__FROM_INDEX) {
+	switch (source) {
+	case GIT_ATTR_FILE__IN_MEMORY:
+		/* in-memory attribute file doesn't need data */
+		break;
+	case GIT_ATTR_FILE__FROM_INDEX: {
 		git_oid id;
 
-		if ((error = attr_file_oid_from_index(&id, repo, ce->path)) < 0 ||
+		if ((error = attr_file_oid_from_index(&id, repo, entry->path)) < 0 ||
 			(error = git_blob_lookup(&blob, repo, &id)) < 0)
 			return error;
 
 		data = git_blob_rawcontent(blob);
-	} else {
-		if ((error = git_futils_readbuffer(&content, ce->fullpath)) < 0)
-			/* always return ENOTFOUND so item will just be skipped */
-			/* TODO: issue a warning once warnings API is available */
+		break;
+	}
+	case GIT_ATTR_FILE__FROM_FILE: {
+		int fd;
+
+		if (p_stat(entry->fullpath, &st) < 0)
+			return git_path_set_error(errno, entry->fullpath, "stat");
+		if (S_ISDIR(st.st_mode))
 			return GIT_ENOTFOUND;
+
+		/* For open or read errors, return ENOTFOUND to skip item */
+		/* TODO: issue warning when warning API is available */
+
+		if ((fd = git_futils_open_ro(entry->fullpath)) < 0)
+			return GIT_ENOTFOUND;
+
+		error = git_futils_readbuffer_fd(&content, fd, (size_t)st.st_size);
+		p_close(fd);
+
+		if (error < 0)
+			return GIT_ENOTFOUND;
+
 		data = content.ptr;
+		break;
+	}
+	default:
+		giterr_set(GITERR_INVALID, "Unknown file source %d", source);
+		return -1;
 	}
 
-	if ((error = git_attr_file__new(&file, ce, source)) < 0)
+	if ((error = git_attr_file__new(&file, entry, source)) < 0)
 		goto cleanup;
 
-	if (parser && (error = parser(repo, file, data, payload)) < 0)
+	if (parser && (error = parser(repo, file, data)) < 0) {
 		git_attr_file__free(file);
-	else
-		*out = file;
+		goto cleanup;
+	}
+
+	/* write cache breaker */
+	if (source == GIT_ATTR_FILE__FROM_INDEX)
+		git_oid_cpy(&file->cache_data.oid, git_blob_id(blob));
+	else if (source == GIT_ATTR_FILE__FROM_FILE)
+		git_futils_filestamp_set_from_stat(&file->cache_data.stamp, &st);
+	/* else always cacheable */
+
+	*out = file;
 
 cleanup:
 	git_blob_free(blob);
@@ -144,18 +180,29 @@ int git_attr_file__out_of_date(git_repository *repo, git_attr_file *file)
 	if (!file)
 		return 1;
 
-	if (file->source == GIT_ATTR_CACHE__FROM_INDEX) {
+	switch (file->source) {
+	case GIT_ATTR_FILE__IN_MEMORY:
+		return 0;
+
+	case GIT_ATTR_FILE__FROM_FILE:
+		return git_futils_filestamp_check(
+			&file->cache_data.stamp, file->entry->fullpath);
+
+	case GIT_ATTR_FILE__FROM_INDEX: {
 		int error;
 		git_oid id;
 
-		if ((error = attr_file_oid_from_index(&id, repo, file->ce->path)) < 0)
+		if ((error = attr_file_oid_from_index(
+				&id, repo, file->entry->path)) < 0)
 			return error;
 
 		return (git_oid__cmp(&file->cache_data.oid, &id) != 0);
 	}
 
-	return git_futils_filestamp_check(
-		&file->cache_data.stamp, file->ce->fullpath);
+	default:
+		giterr_set(GITERR_INVALID, "Invalid file type %d", file->source);
+		return -1;
+	}
 }
 
 static int sort_by_hash_and_name(const void *a_raw, const void *b_raw);
@@ -166,22 +213,17 @@ static bool parse_optimized_patterns(
 	const char *pattern);
 
 int git_attr_file__parse_buffer(
-	git_repository *repo,
-	git_attr_file *attrs,
-	const char *data,
-	void *payload)
+	git_repository *repo, git_attr_file *attrs, const char *data)
 {
 	int error = 0;
 	const char *scan = data, *context = NULL;
 	git_attr_rule *rule = NULL;
 
-	GIT_UNUSED(payload);
-
 	/* if subdir file path, convert context for file paths */
-	if (attrs->ce &&
-		git_path_root(attrs->ce->path) < 0 &&
-		!git__suffixcmp(attrs->ce->path, "/" GIT_ATTR_FILE))
-		context = attrs->ce->path;
+	if (attrs->entry &&
+		git_path_root(attrs->entry->path) < 0 &&
+		!git__suffixcmp(attrs->entry->path, "/" GIT_ATTR_FILE))
+		context = attrs->entry->path;
 
 	if (git_mutex_lock(&attrs->lock) < 0) {
 		giterr_set(GITERR_OS, "Failed to lock attribute file");
@@ -268,19 +310,18 @@ int git_attr_file__lookup_one(
 	return 0;
 }
 
-int git_attr_file__load_standalone(
-	git_attr_file **out,
-	const char *path)
+int git_attr_file__load_standalone(git_attr_file **out, const char *path)
 {
 	int error;
 	git_attr_file *file;
 	git_buf content = GIT_BUF_INIT;
 
-	error = git_attr_file__new(&file, NULL, GIT_ATTR_CACHE__FROM_FILE);
+	error = git_attr_file__new(&file, NULL, GIT_ATTR_FILE__FROM_FILE);
 	if (error < 0)
 		return error;
 
-	error = git_attr_cache_entry__new(&file->ce, NULL, path, &file->pool);
+	error = git_attr_cache__alloc_file_entry(
+		&file->entry, NULL, path, &file->pool);
 	if (error < 0) {
 		git_attr_file__free(file);
 		return error;
@@ -290,7 +331,7 @@ int git_attr_file__load_standalone(
 	 */
 
 	if (!(error = git_futils_readbuffer(&content, path))) {
-		error = git_attr_file__parse_buffer(NULL, file, content.ptr, NULL);
+		error = git_attr_file__parse_buffer(NULL, file, content.ptr);
 		git_buf_free(&content);
 	}
 

src/attr_file.h

@@ -13,7 +13,6 @@
 #include "pool.h"
 #include "buffer.h"
 #include "fileops.h"
-#include "attrcache.h"
 
 #define GIT_ATTR_FILE			".gitattributes"
 #define GIT_ATTR_FILE_INREPO	"info/attributes"
@@ -36,6 +35,14 @@
 	(GIT_ATTR_FNMATCH_ALLOWSPACE | \
 	 GIT_ATTR_FNMATCH_ALLOWNEG | GIT_ATTR_FNMATCH_ALLOWMACRO)
 
+typedef enum {
+	GIT_ATTR_FILE__IN_MEMORY  = 0,
+	GIT_ATTR_FILE__FROM_FILE  = 1,
+	GIT_ATTR_FILE__FROM_INDEX = 2,
+
+	GIT_ATTR_FILE_NUM_SOURCES = 3
+} git_attr_file_source;
+
 extern const char *git_attr__true;
 extern const char *git_attr__false;
 extern const char *git_attr__unset;
@@ -46,10 +53,10 @@ typedef struct {
 	unsigned int flags;
 } git_attr_fnmatch;
 
-struct git_attr_rule {
+typedef struct {
 	git_attr_fnmatch match;
 	git_vector assigns;		/* vector of <git_attr_assignment*> */
-};
+} git_attr_rule;
 
 typedef struct {
 	git_refcount unused;
@@ -64,19 +71,32 @@ typedef struct {
 	const char *value;
 } git_attr_assignment;
 
-struct git_attr_file {
+typedef struct git_attr_file_entry git_attr_file_entry;
+
+typedef struct {
 	git_refcount rc;
 	git_mutex lock;
-	git_attr_cache_entry *ce;
-	git_attr_cache_source source;
+	git_attr_file_entry *entry;
+	git_attr_file_source source;
 	git_vector rules;			/* vector of <rule*> or <fnmatch*> */
 	git_pool pool;
 	union {
 		git_oid oid;
 		git_futils_filestamp stamp;
 	} cache_data;
+} git_attr_file;
+
+struct git_attr_file_entry {
+	git_attr_file *file[GIT_ATTR_FILE_NUM_SOURCES];
+	const char *path; /* points into fullpath */
+	char fullpath[GIT_FLEX_ARRAY];
 };
 
+typedef int (*git_attr_file_parser)(
+	git_repository *repo,
+	git_attr_file *file,
+	const char *data);
+
 typedef struct {
 	git_buf  full;
 	char    *path;
@@ -90,31 +110,26 @@ typedef struct {
 
 int git_attr_file__new(
 	git_attr_file **out,
-	git_attr_cache_entry *ce,
-	git_attr_cache_source source);
+	git_attr_file_entry *entry,
+	git_attr_file_source source);
 
 void git_attr_file__free(git_attr_file *file);
 
 int git_attr_file__load(
 	git_attr_file **out,
 	git_repository *repo,
-	git_attr_cache_entry *ce,
-	git_attr_cache_source source,
-	git_attr_cache_parser parser,
-	void *payload);
+	git_attr_file_entry *ce,
+	git_attr_file_source source,
+	git_attr_file_parser parser);
 
 int git_attr_file__load_standalone(
-	git_attr_file **out,
-	const char *path);
+	git_attr_file **out, const char *path);
 
 int git_attr_file__out_of_date(
 	git_repository *repo, git_attr_file *file);
 
 int git_attr_file__parse_buffer(
-	git_repository *repo,
-	git_attr_file *attrs,
-	const char *data,
-	void *payload);
+	git_repository *repo, git_attr_file *attrs, const char *data);
 
 int git_attr_file__clear_rules(
 	git_attr_file *file, bool need_lock);

src/attrcache.c

@@ -24,7 +24,7 @@ GIT_INLINE(void) attr_cache_unlock(git_attr_cache *cache)
 	git_mutex_unlock(&cache->lock);
 }
 
-GIT_INLINE(git_attr_cache_entry *) attr_cache_lookup_entry(
+GIT_INLINE(git_attr_file_entry *) attr_cache_lookup_entry(
 	git_attr_cache *cache, const char *path)
 {
 	khiter_t pos = git_strmap_lookup_index(cache->files, path);
@@ -35,15 +35,15 @@ GIT_INLINE(git_attr_cache_entry *) attr_cache_lookup_entry(
 		return NULL;
 }
 
-int git_attr_cache_entry__new(
-	git_attr_cache_entry **out,
+int git_attr_cache__alloc_file_entry(
+	git_attr_file_entry **out,
 	const char *base,
 	const char *path,
 	git_pool *pool)
 {
 	size_t baselen = 0, pathlen = strlen(path);
-	size_t cachesize = sizeof(git_attr_cache_entry) + pathlen + 1;
-	git_attr_cache_entry *ce;
+	size_t cachesize = sizeof(git_attr_file_entry) + pathlen + 1;
+	git_attr_file_entry *ce;
 
 	if (base != NULL && git_path_root(path) < 0) {
 		baselen = strlen(base);
@@ -72,41 +72,41 @@ int git_attr_cache_entry__new(
 
 /* call with attrcache locked */
 static int attr_cache_make_entry(
-	git_attr_cache_entry **out, git_repository *repo, const char *path)
+	git_attr_file_entry **out, git_repository *repo, const char *path)
 {
 	int error = 0;
 	git_attr_cache *cache = git_repository_attr_cache(repo);
-	git_attr_cache_entry *ce = NULL;
+	git_attr_file_entry *entry = NULL;
 
-	error = git_attr_cache_entry__new(
-		&ce, git_repository_workdir(repo), path, &cache->pool);
+	error = git_attr_cache__alloc_file_entry(
+		&entry, git_repository_workdir(repo), path, &cache->pool);
 
 	if (!error) {
-		git_strmap_insert(cache->files, ce->path, ce, error);
+		git_strmap_insert(cache->files, entry->path, entry, error);
 		if (error > 0)
 			error = 0;
 	}
 
-	*out = ce;
+	*out = entry;
 	return error;
 }
 
 /* insert entry or replace existing if we raced with another thread */
 static int attr_cache_upsert(git_attr_cache *cache, git_attr_file *file)
 {
-	git_attr_cache_entry *ce;
+	git_attr_file_entry *entry;
 	git_attr_file *old;
 
 	if (attr_cache_lock(cache) < 0)
 		return -1;
 
-	ce = attr_cache_lookup_entry(cache, file->ce->path);
-
-	old = ce->file[file->source];
+	entry = attr_cache_lookup_entry(cache, file->entry->path);
 
-	GIT_REFCOUNT_OWN(file, ce);
+	GIT_REFCOUNT_OWN(file, entry);
 	GIT_REFCOUNT_INC(file);
-	ce->file[file->source] = file;
+
+	old = git__compare_and_swap(
+		&entry->file[file->source], entry->file[file->source], file);
 
 	if (old) {
 		GIT_REFCOUNT_OWN(old, NULL);
@@ -120,7 +120,7 @@ static int attr_cache_upsert(git_attr_cache *cache, git_attr_file *file)
 static int attr_cache_remove(git_attr_cache *cache, git_attr_file *file)
 {
 	int error = 0;
-	git_attr_cache_entry *ce;
+	git_attr_file_entry *entry;
 	bool found = false;
 
 	if (!file)
@@ -128,27 +128,29 @@ static int attr_cache_remove(git_attr_cache *cache, git_attr_file *file)
 	if ((error = attr_cache_lock(cache)) < 0)
 		return error;
 
-	if ((ce = attr_cache_lookup_entry(cache, file->ce->path)) != NULL &&
-		ce->file[file->source] == file)
-	{
-		ce->file[file->source] = NULL;
-		GIT_REFCOUNT_OWN(file, NULL);
-		found = true;
-	}
+	if ((entry = attr_cache_lookup_entry(cache, file->entry->path)) != NULL)
+		file = git__compare_and_swap(&entry->file[file->source], file, NULL);
 
 	attr_cache_unlock(cache);
 
-	if (found)
+	if (found) {
+		GIT_REFCOUNT_OWN(file, NULL);
 		git_attr_file__free(file);
+	}
 
 	return error;
 }
 
+/* Look up cache entry and file.
+ * - If entry is not present, create it while the cache is locked.
+ * - If file is present, increment refcount before returning it, so the
+ *   cache can be unlocked and it won't go away.
+ */
 static int attr_cache_lookup(
 	git_attr_file **out_file,
-	git_attr_cache_entry **out_ce,
+	git_attr_file_entry **out_entry,
 	git_repository *repo,
-	git_attr_cache_source source,
+	git_attr_file_source source,
 	const char *base,
 	const char *filename)
 {
@@ -156,7 +158,7 @@ static int attr_cache_lookup(
 	git_buf path = GIT_BUF_INIT;
 	const char *wd = git_repository_workdir(repo), *relfile;
 	git_attr_cache *cache = git_repository_attr_cache(repo);
-	git_attr_cache_entry *ce = NULL;
+	git_attr_file_entry *entry = NULL;
 	git_attr_file *file = NULL;
 
 	/* join base and path as needed */
@@ -174,20 +176,20 @@ static int attr_cache_lookup(
 	if ((error = attr_cache_lock(cache)) < 0)
 		goto cleanup;
 
-	ce = attr_cache_lookup_entry(cache, relfile);
-	if (!ce) {
-		if ((error = attr_cache_make_entry(&ce, repo, relfile)) < 0)
+	entry = attr_cache_lookup_entry(cache, relfile);
+	if (!entry) {
+		if ((error = attr_cache_make_entry(&entry, repo, relfile)) < 0)
 			goto cleanup;
-	} else if (ce->file[source] != NULL) {
-		file = ce->file[source];
+	} else if (entry->file[source] != NULL) {
+		file = entry->file[source];
 		GIT_REFCOUNT_INC(file);
 	}
 
 	attr_cache_unlock(cache);
 
 cleanup:
-	*out_file = file;
-	*out_ce = ce;
+	*out_file  = file;
+	*out_entry = entry;
 
 	git_buf_free(&path);
 	return error;
@@ -196,29 +198,26 @@ cleanup:
 int git_attr_cache__get(
 	git_attr_file **out,
 	git_repository *repo,
-	git_attr_cache_source source,
+	git_attr_file_source source,
 	const char *base,
 	const char *filename,
-	git_attr_cache_parser parser,
-	void *payload)
+	git_attr_file_parser parser)
 {
 	int error = 0;
 	git_attr_cache *cache = git_repository_attr_cache(repo);
-	git_attr_cache_entry *ce = NULL;
+	git_attr_file_entry *entry = NULL;
 	git_attr_file *file = NULL;
 
-	if ((error = attr_cache_lookup(&file, &ce, repo, source, base, filename)) < 0)
+	if ((error = attr_cache_lookup(
+			&file, &entry, repo, source, base, filename)) < 0)
 		goto cleanup;
 
-	/* if this is not a file backed entry, just create a new empty one */
-	if (!parser) {
-		if (!file && !(error = git_attr_file__new(&file, ce, source)))
-			error = attr_cache_upsert(cache, file);
-	}
-	/* otherwise load and/or reload as needed */
-	else if (!file || (error = git_attr_file__out_of_date(repo, file)) > 0) {
-		if (!(error = git_attr_file__load(
-				&file, repo, ce, source, parser, payload)))
+	/* if file not found or out of date, load up-to-date data and replace */
+	if (!file || (error = git_attr_file__out_of_date(repo, file)) > 0) {
+		/* decrement refcount (if file was found) b/c we will not return it */
+		git_attr_file__free(file);
+
+		if (!(error = git_attr_file__load(&file, repo, entry, source, parser)))
 			error = attr_cache_upsert(cache, file);
 	}
 
@@ -245,13 +244,13 @@ cleanup:
 
 bool git_attr_cache__is_cached(
 	git_repository *repo,
-	git_attr_cache_source source,
+	git_attr_file_source source,
 	const char *filename)
 {
 	git_attr_cache *cache = git_repository_attr_cache(repo);
 	git_strmap *files;
 	khiter_t pos;
-	git_attr_cache_entry *ce;
+	git_attr_file_entry *entry;
 
 	if (!(cache = git_repository_attr_cache(repo)) ||
 		!(files = cache->files))
@@ -261,9 +260,9 @@ bool git_attr_cache__is_cached(
 	if (!git_strmap_valid_index(files, pos))
 		return false;
 
-	ce = git_strmap_value_at(files, pos);
+	entry = git_strmap_value_at(files, pos);
 
-	return ce && (ce->file[source] != NULL);
+	return entry && (entry->file[source] != NULL);
 }
 
 
@@ -307,14 +306,15 @@ static void attr_cache__free(git_attr_cache *cache)
 	unlock = (git_mutex_lock(&cache->lock) == 0);
 
 	if (cache->files != NULL) {
-		git_attr_cache_entry *ce;
+		git_attr_file_entry *entry;
+		git_attr_file *file;
 		int i;
 
-		git_strmap_foreach_value(cache->files, ce, {
-			for (i = 0; i < GIT_ATTR_CACHE_NUM_SOURCES; ++i) {
-				if (ce->file[i]) {
-					GIT_REFCOUNT_OWN(ce->file[i], NULL);
-					git_attr_file__free(ce->file[i]);
+		git_strmap_foreach_value(cache->files, entry, {
+			for (i = 0; i < GIT_ATTR_FILE_NUM_SOURCES; ++i) {
+				if ((file = git__swap(entry->file[i], NULL)) != NULL) {
+					GIT_REFCOUNT_OWN(file, NULL);
+					git_attr_file__free(file);
 				}
 			}
 		});
@@ -345,7 +345,7 @@ static void attr_cache__free(git_attr_cache *cache)
 	git__free(cache);
 }
 
-int git_attr_cache__init(git_repository *repo)
+int git_attr_cache__do_init(git_repository *repo)
 {
 	int ret = 0;
 	git_attr_cache *cache = git_repository_attr_cache(repo);

src/attrcache.h

@@ -7,9 +7,8 @@
 #ifndef INCLUDE_attrcache_h__
 #define INCLUDE_attrcache_h__
 
-#include "pool.h"
+#include "attr_file.h"
 #include "strmap.h"
-#include "buffer.h"
 
 #define GIT_ATTR_CONFIG       "core.attributesfile"
 #define GIT_IGNORE_CONFIG     "core.excludesfile"
@@ -23,55 +22,35 @@ typedef struct {
 	git_pool  pool;
 } git_attr_cache;
 
-extern int git_attr_cache__init(git_repository *repo);
+extern int git_attr_cache__do_init(git_repository *repo);
 
-typedef enum {
-	GIT_ATTR_CACHE__FROM_FILE = 0,
-	GIT_ATTR_CACHE__FROM_INDEX = 1,
-
-	GIT_ATTR_CACHE_NUM_SOURCES = 2
-} git_attr_cache_source;
-
-typedef struct git_attr_file git_attr_file;
-typedef struct git_attr_rule git_attr_rule;
-
-typedef struct {
-	git_attr_file *file[GIT_ATTR_CACHE_NUM_SOURCES];
-	const char *path; /* points into fullpath */
-	char fullpath[GIT_FLEX_ARRAY];
-} git_attr_cache_entry;
-
-typedef int (*git_attr_cache_parser)(
-	git_repository *repo,
-	git_attr_file *file,
-	const char *data,
-	void *payload);
+#define git_attr_cache__init(REPO) \
+	(git_repository_attr_cache(REPO) ? 0 : git_attr_cache__do_init(REPO))
 
 /* get file - loading and reload as needed */
 extern int git_attr_cache__get(
 	git_attr_file **file,
 	git_repository *repo,
-	git_attr_cache_source source,
+	git_attr_file_source source,
 	const char *base,
 	const char *filename,
-	git_attr_cache_parser parser,
-	void *payload);
+	git_attr_file_parser parser);
 
 extern bool git_attr_cache__is_cached(
 	git_repository *repo,
-	git_attr_cache_source source,
+	git_attr_file_source source,
 	const char *path);
 
+extern int git_attr_cache__alloc_file_entry(
+	git_attr_file_entry **out,
+	const char *base,
+	const char *path,
+	git_pool *pool);
+
 extern int git_attr_cache__insert_macro(
 	git_repository *repo, git_attr_rule *macro);
 
 extern git_attr_rule *git_attr_cache__lookup_macro(
 	git_repository *repo, const char *name);
 
-extern int git_attr_cache_entry__new(
-	git_attr_cache_entry **out,
-	const char *base,
-	const char *path,
-	git_pool *pool);
-
 #endif

src/fileops.c

@@ -132,6 +132,7 @@ int git_futils_readbuffer_fd(git_buf *buf, git_file fd, size_t len)
 
 	if (read_size != (ssize_t)len) {
 		giterr_set(GITERR_OS, "Failed to read descriptor");
+		git_buf_free(buf);
 		return -1;
 	}
 
@@ -829,3 +830,16 @@ void git_futils_filestamp_set(
 	else
 		memset(target, 0, sizeof(*target));
 }
+
+
+void git_futils_filestamp_set_from_stat(
+	git_futils_filestamp *stamp, struct stat *st)
+{
+	if (st) {
+		stamp->mtime = (git_time_t)st->st_mtime;
+		stamp->size  = (git_off_t)st->st_size;
+		stamp->ino   = (unsigned int)st->st_ino;
+	} else {
+		memset(stamp, 0, sizeof(*stamp));
+	}
+}

src/fileops.h

@@ -317,4 +317,10 @@ extern int git_futils_filestamp_check(
 extern void git_futils_filestamp_set(
 	git_futils_filestamp *tgt, const git_futils_filestamp *src);
 
+/**
+ * Set file stamp data from stat structure
+ */
+extern void git_futils_filestamp_set_from_stat(
+	git_futils_filestamp *stamp, struct stat *st);
+
 #endif /* INCLUDE_fileops_h__ */

src/ignore.c

 #include "git2/ignore.h"
 #include "common.h"
 #include "ignore.h"
-#include "attr_file.h"
+#include "attrcache.h"
 #include "path.h"
 #include "config.h"
 
@@ -10,30 +10,24 @@
 #define GIT_IGNORE_DEFAULT_RULES ".\n..\n.git\n"
 
 static int parse_ignore_file(
-	git_repository *repo,
-	git_attr_file *attrs,
-	const char *data,
-	void *payload)
+	git_repository *repo, git_attr_file *attrs, const char *data)
 {
 	int error = 0;
 	int ignore_case = false;
 	const char *scan = data, *context = NULL;
 	git_attr_fnmatch *match = NULL;
 
-	/* either read ignore_case from ignores structure or use repo config */
-	if (payload != NULL)
-		ignore_case = ((git_ignores *)payload)->ignore_case;
-	else if (git_repository__cvar(&ignore_case, repo, GIT_CVAR_IGNORECASE) < 0)
+	if (git_repository__cvar(&ignore_case, repo, GIT_CVAR_IGNORECASE) < 0)
 		giterr_clear();
 
 	/* if subdir file path, convert context for file paths */
-	if (attrs->ce &&
-		git_path_root(attrs->ce->path) < 0 &&
-		!git__suffixcmp(attrs->ce->path, "/" GIT_IGNORE_FILE))
-		context = attrs->ce->path;
+	if (attrs->entry &&
+		git_path_root(attrs->entry->path) < 0 &&
+		!git__suffixcmp(attrs->entry->path, "/" GIT_IGNORE_FILE))
+		context = attrs->entry->path;
 
 	if (git_mutex_lock(&attrs->lock) < 0) {
-		giterr_set(GITERR_OS, "Failed to lock attribute file");
+		giterr_set(GITERR_OS, "Failed to lock ignore file");
 		return -1;
 	}
 
@@ -84,8 +78,8 @@ static int push_ignore_file(
 	git_attr_file *file = NULL;
 
 	error = git_attr_cache__get(
-		&file, ignores->repo, GIT_ATTR_CACHE__FROM_FILE,
-		base, filename, parse_ignore_file, ignores);
+		&file, ignores->repo, GIT_ATTR_FILE__FROM_FILE,
+		base, filename, parse_ignore_file);
 	if (error < 0)
 		return error;
 
@@ -111,14 +105,12 @@ static int get_internal_ignores(git_attr_file **out, git_repository *repo)
 	if ((error = git_attr_cache__init(repo)) < 0)
 		return error;
 
-	/* get with NULL parser, gives existing or empty git_attr_file */
 	error = git_attr_cache__get(
-		out, repo, GIT_ATTR_CACHE__FROM_FILE,
-		NULL, GIT_IGNORE_INTERNAL, NULL, NULL);
+		out, repo, GIT_ATTR_FILE__IN_MEMORY, NULL, GIT_IGNORE_INTERNAL, NULL);
 
 	/* if internal rules list is empty, insert default rules */
 	if (!error && !(*out)->rules.length)
-		error = parse_ignore_file(repo, *out, GIT_IGNORE_DEFAULT_RULES, NULL);
+		error = parse_ignore_file(repo, *out, GIT_IGNORE_DEFAULT_RULES);
 
 	return error;
 }
@@ -199,7 +191,7 @@ int git_ignore__pop_dir(git_ignores *ign)
 {
 	if (ign->ign_path.length > 0) {
 		git_attr_file *file = git_vector_last(&ign->ign_path);
-		const char *start = file->ce->path, *end;
+		const char *start = file->entry->path, *end;
 
 		/* - ign->dir looks something like "/home/user/a/b/" (or "a/b/c/d/")
 		 * - file->path looks something like "a/b/.gitignore
@@ -302,35 +294,33 @@ cleanup:
 	return 0;
 }
 
-int git_ignore_add_rule(
-	git_repository *repo,
-	const char *rules)
+int git_ignore_add_rule(git_repository *repo, const char *rules)
 {
 	int error;
 	git_attr_file *ign_internal = NULL;
 
-	if (!(error = get_internal_ignores(&ign_internal, repo))) {
-		error = parse_ignore_file(repo, ign_internal, rules, NULL);
-		git_attr_file__free(ign_internal);
-	}
+	if ((error = get_internal_ignores(&ign_internal, repo)) < 0)
+		return error;
+
+	error = parse_ignore_file(repo, ign_internal, rules);
+	git_attr_file__free(ign_internal);
 
 	return error;
 }
 
-int git_ignore_clear_internal_rules(
-	git_repository *repo)
+int git_ignore_clear_internal_rules(git_repository *repo)
 {
 	int error;
 	git_attr_file *ign_internal;
 
-	if (!(error = get_internal_ignores(&ign_internal, repo))) {
-		if (!(error = git_attr_file__clear_rules(ign_internal, true)))
-			error = parse_ignore_file(
-				repo, ign_internal, GIT_IGNORE_DEFAULT_RULES, NULL);
+	if ((error = get_internal_ignores(&ign_internal, repo)) < 0)
+		return error;
 
-		git_attr_file__free(ign_internal);
-	}
+	if (!(error = git_attr_file__clear_rules(ign_internal, true)))
+		error = parse_ignore_file(
+			repo, ign_internal, GIT_IGNORE_DEFAULT_RULES);
 
+	git_attr_file__free(ign_internal);
 	return error;
 }
 

tests/attr/file.c

@@ -13,7 +13,7 @@ void test_attr_file__simple_read(void)
 
 	cl_git_pass(git_attr_file__load_standalone(&file, cl_fixture("attr/attr0")));
 
-	cl_assert_equal_s(cl_fixture("attr/attr0"), file->ce->path);
+	cl_assert_equal_s(cl_fixture("attr/attr0"), file->entry->path);
 	cl_assert(file->rules.length == 1);
 
 	rule = get_rule(0);
@@ -39,7 +39,7 @@ void test_attr_file__match_variants(void)
 
 	cl_git_pass(git_attr_file__load_standalone(&file, cl_fixture("attr/attr1")));
 
-	cl_assert_equal_s(cl_fixture("attr/attr1"), file->ce->path);
+	cl_assert_equal_s(cl_fixture("attr/attr1"), file->entry->path);
 	cl_assert(file->rules.length == 10);
 
 	/* let's do a thorough check of this rule, then just verify
@@ -123,7 +123,7 @@ void test_attr_file__assign_variants(void)
 
 	cl_git_pass(git_attr_file__load_standalone(&file, cl_fixture("attr/attr2")));
 
-	cl_assert_equal_s(cl_fixture("attr/attr2"), file->ce->path);
+	cl_assert_equal_s(cl_fixture("attr/attr2"), file->entry->path);
 	cl_assert(file->rules.length == 11);
 
 	check_one_assign(file, 0, 0, "pat0", "simple", EXPECT_TRUE, NULL);
@@ -188,7 +188,7 @@ void test_attr_file__check_attr_examples(void)
 	git_attr_assignment *assign;
 
 	cl_git_pass(git_attr_file__load_standalone(&file, cl_fixture("attr/attr3")));
-	cl_assert_equal_s(cl_fixture("attr/attr3"), file->ce->path);
+	cl_assert_equal_s(cl_fixture("attr/attr3"), file->entry->path);
 	cl_assert(file->rules.length == 3);
 
 	rule = get_rule(0);

tests/attr/lookup.c

@@ -10,7 +10,7 @@ void test_attr_lookup__simple(void)
 	const char *value = NULL;
 
 	cl_git_pass(git_attr_file__load_standalone(&file, cl_fixture("attr/attr0")));
-	cl_assert_equal_s(cl_fixture("attr/attr0"), file->ce->path);
+	cl_assert_equal_s(cl_fixture("attr/attr0"), file->entry->path);
 	cl_assert(file->rules.length == 1);
 
 	cl_git_pass(git_attr_path__init(&path, "test", NULL));
@@ -130,7 +130,7 @@ void test_attr_lookup__match_variants(void)
 	};
 
 	cl_git_pass(git_attr_file__load_standalone(&file, cl_fixture("attr/attr1")));
-	cl_assert_equal_s(cl_fixture("attr/attr1"), file->ce->path);
+	cl_assert_equal_s(cl_fixture("attr/attr1"), file->entry->path);
 	cl_assert(file->rules.length == 10);
 
 	cl_git_pass(git_attr_path__init(&path, "/testing/for/pat0", NULL));
@@ -252,7 +252,7 @@ void test_attr_lookup__from_buffer(void)
 
 	cl_git_pass(git_attr_file__new(&file, NULL, 0));
 
-	cl_git_pass(git_attr_file__parse_buffer(NULL, file, "a* foo\nabc bar\n* baz", NULL));
+	cl_git_pass(git_attr_file__parse_buffer(NULL, file, "a* foo\nabc bar\n* baz"));
 
 	cl_assert(file->rules.length == 3);
 

tests/attr/repo.c

@@ -68,9 +68,12 @@ void test_attr_repo__get_one(void)
 		attr_check_expected(scan->expected, scan->expected_str, scan->attr, value);
 	}
 
-	cl_assert(git_attr_cache__is_cached(g_repo, 0, ".git/info/attributes"));
-	cl_assert(git_attr_cache__is_cached(g_repo, 0, ".gitattributes"));
-	cl_assert(git_attr_cache__is_cached(g_repo, 0, "sub/.gitattributes"));
+	cl_assert(git_attr_cache__is_cached(
+		g_repo, GIT_ATTR_FILE__FROM_FILE, ".git/info/attributes"));
+	cl_assert(git_attr_cache__is_cached(
+		g_repo, GIT_ATTR_FILE__FROM_FILE, ".gitattributes"));
+	cl_assert(git_attr_cache__is_cached(
+		g_repo, GIT_ATTR_FILE__FROM_FILE, "sub/.gitattributes"));
 }
 
 void test_attr_repo__get_many(void)

tests/status/ignore.c

@@ -54,8 +54,10 @@ void test_status_ignore__0(void)
 	}
 
 	/* confirm that ignore files were cached */
-	cl_assert(git_attr_cache__is_cached(g_repo, 0, ".git/info/exclude"));
-	cl_assert(git_attr_cache__is_cached(g_repo, 0, ".gitignore"));
+	cl_assert(git_attr_cache__is_cached(
+		g_repo, GIT_ATTR_FILE__FROM_FILE, ".git/info/exclude"));
+	cl_assert(git_attr_cache__is_cached(
+		g_repo, GIT_ATTR_FILE__FROM_FILE, ".gitignore"));
 }