Merge pull request #1817 from libgit2/ntk/fix/backend/honor_refresh_capabilities

Of backends and refreshers...

include/git2/sys/odb_backend.h

@@ -64,6 +64,16 @@ struct git_odb_backend {
 	int (* exists)(
 		git_odb_backend *, const git_oid *);
 
+	/**
+	 * If the backend implements a refreshing mechanism, it should be exposed
+	 * through this endpoint. Each call to `git_odb_refresh()` will invoke it.
+	 *
+	 * However, the backend implementation should try to stay up-to-date as much
+	 * as possible by itself as libgit2 will not automatically invoke
+	 * `git_odb_refresh()`. For instance, a potential strategy for the backend
+	 * implementation to achieve this could be to internally invoke this
+	 * endpoint on failed lookups (ie. `exists()`, `read()`, `read_header()`).
+	 */
 	int (* refresh)(git_odb_backend *);
 
 	int (* foreach)(

src/odb.c

@@ -608,7 +608,6 @@ int git_odb_exists(git_odb *db, const git_oid *id)
 	git_odb_object *object;
 	size_t i;
 	bool found = false;
-	bool refreshed = false;
 
 	assert(db && id);
 
@@ -617,7 +616,6 @@ int git_odb_exists(git_odb *db, const git_oid *id)
 		return (int)true;
 	}
 
-attempt_lookup:
 	for (i = 0; i < db->backends.length && !found; ++i) {
 		backend_internal *internal = git_vector_get(&db->backends, i);
 		git_odb_backend *b = internal->backend;
@@ -626,16 +624,6 @@ attempt_lookup:
 			found = b->exists(b, id);
 	}
 
-	if (!found && !refreshed) {
-		if (git_odb_refresh(db) < 0) {
-			giterr_clear();
-			return (int)false;
-		}
-
-		refreshed = true;
-		goto attempt_lookup;
-	}
-
 	return (int)found;
 }
 
@@ -700,7 +688,6 @@ int git_odb_read(git_odb_object **out, git_odb *db, const git_oid *id)
 {
 	size_t i, reads = 0;
 	int error;
-	bool refreshed = false;
 	git_rawobj raw;
 	git_odb_object *object;
 
@@ -710,7 +697,6 @@ int git_odb_read(git_odb_object **out, git_odb *db, const git_oid *id)
 	if (*out != NULL)
 		return 0;
 
-attempt_lookup:
 	error = GIT_ENOTFOUND;
 
 	for (i = 0; i < db->backends.length && error < 0; ++i) {
@@ -723,14 +709,6 @@ attempt_lookup:
 		}
 	}
 
-	if (error == GIT_ENOTFOUND && !refreshed) {
-		if ((error = git_odb_refresh(db)) < 0)
-			return error;
-
-		refreshed = true;
-		goto attempt_lookup;
-	}
-
 	if (error && error != GIT_PASSTHROUGH) {
 		if (!reads)
 			return git_odb__error_notfound("no match for id", id);
@@ -752,7 +730,7 @@ int git_odb_read_prefix(
 	git_oid found_full_oid = {{0}};
 	git_rawobj raw;
 	void *data = NULL;
-	bool found = false, refreshed = false;
+	bool found = false;
 	git_odb_object *object;
 
 	assert(out && db);
@@ -769,7 +747,6 @@ int git_odb_read_prefix(
 			return 0;
 	}
 
-attempt_lookup:
 	for (i = 0; i < db->backends.length; ++i) {
 		backend_internal *internal = git_vector_get(&db->backends, i);
 		git_odb_backend *b = internal->backend;
@@ -796,14 +773,6 @@ attempt_lookup:
 		}
 	}
 
-	if (!found && !refreshed) {
-		if ((error = git_odb_refresh(db)) < 0)
-			return error;
-
-		refreshed = true;
-		goto attempt_lookup;
-	}
-
 	if (!found)
 		return git_odb__error_notfound("no match for prefix", short_id);
 

src/odb_pack.c

@@ -342,8 +342,9 @@ static int pack_backend__refresh(git_odb_backend *_backend)
 	return 0;
 }
 
-
-static int pack_backend__read_header(size_t *len_p, git_otype *type_p, struct git_odb_backend *backend, const git_oid *oid)
+static int pack_backend__read_header_internal(
+	size_t *len_p, git_otype *type_p,
+	struct git_odb_backend *backend, const git_oid *oid)
 {
 	struct git_pack_entry e;
 	int error;
@@ -356,7 +357,26 @@ static int pack_backend__read_header(size_t *len_p, git_otype *type_p, struct gi
 	return git_packfile_resolve_header(len_p, type_p, e.p, e.offset);
 }
 
-static int pack_backend__read(void **buffer_p, size_t *len_p, git_otype *type_p, git_odb_backend *backend, const git_oid *oid)
+static int pack_backend__read_header(
+	size_t *len_p, git_otype *type_p,
+	struct git_odb_backend *backend, const git_oid *oid)
+{
+	int error;
+
+	error = pack_backend__read_header_internal(len_p, type_p, backend, oid);
+
+	if (error != GIT_ENOTFOUND)
+		return error;
+
+	if ((error = pack_backend__refresh(backend)) < 0)
+		return error;
+
+	return pack_backend__read_header_internal(len_p, type_p, backend, oid);
+}
+
+static int pack_backend__read_internal(
+	void **buffer_p, size_t *len_p, git_otype *type_p,
+	git_odb_backend *backend, const git_oid *oid)
 {
 	struct git_pack_entry e;
 	git_rawobj raw;
@@ -373,7 +393,24 @@ static int pack_backend__read(void **buffer_p, size_t *len_p, git_otype *type_p,
 	return 0;
 }
 
-static int pack_backend__read_prefix(
+static int pack_backend__read(
+	void **buffer_p, size_t *len_p, git_otype *type_p,
+	git_odb_backend *backend, const git_oid *oid)
+{
+	int error;
+
+	error = pack_backend__read_internal(buffer_p, len_p, type_p, backend, oid);
+
+	if (error != GIT_ENOTFOUND)
+		return error;
+
+	if ((error = pack_backend__refresh(backend)) < 0)
+		return error;
+
+	return pack_backend__read_internal(buffer_p, len_p, type_p, backend, oid);
+}
+
+static int pack_backend__read_prefix_internal(
 	git_oid *out_oid,
 	void **buffer_p,
 	size_t *len_p,
@@ -410,9 +447,45 @@ static int pack_backend__read_prefix(
 	return error;
 }
 
+static int pack_backend__read_prefix(
+	git_oid *out_oid,
+	void **buffer_p,
+	size_t *len_p,
+	git_otype *type_p,
+	git_odb_backend *backend,
+	const git_oid *short_oid,
+	size_t len)
+{
+	int error;
+
+	error = pack_backend__read_prefix_internal(
+		out_oid, buffer_p, len_p, type_p, backend, short_oid, len);
+
+	if (error != GIT_ENOTFOUND)
+		return error;
+
+	if ((error = pack_backend__refresh(backend)) < 0)
+		return error;
+
+	return pack_backend__read_prefix_internal(
+		out_oid, buffer_p, len_p, type_p, backend, short_oid, len);
+}
+
 static int pack_backend__exists(git_odb_backend *backend, const git_oid *oid)
 {
 	struct git_pack_entry e;
+	int error;
+
+	error = pack_entry_find(&e, (struct pack_backend *)backend, oid);
+
+	if (error != GIT_ENOTFOUND)
+		return error == 0;
+
+	if ((error = pack_backend__refresh(backend)) < 0) {
+		giterr_clear();
+		return (int)false;
+	}
+
 	return pack_entry_find(&e, (struct pack_backend *)backend, oid) == 0;
 }
 

tests-clar/odb/backend/nonrefreshing.c

+#include "clar_libgit2.h"
+#include "git2/sys/odb_backend.h"
+#include "repository.h"
+
+typedef struct fake_backend {
+	git_odb_backend parent;
+
+	git_error_code error_code;
+
+	int exists_calls;
+	int read_calls;
+	int read_header_calls;
+	int read_prefix_calls;
+} fake_backend;
+
+static git_repository *_repo;
+static fake_backend *_fake;
+static git_oid _oid;
+
+static int fake_backend__exists(git_odb_backend *backend, const git_oid *oid)
+{
+	fake_backend *fake;
+
+	GIT_UNUSED(oid);
+
+	fake = (fake_backend *)backend;
+
+	fake->exists_calls++;
+
+	return (fake->error_code == GIT_OK);
+}
+
+static int fake_backend__read(
+	void **buffer_p, size_t *len_p, git_otype *type_p,
+	git_odb_backend *backend, const git_oid *oid)
+{
+	fake_backend *fake;
+
+	GIT_UNUSED(buffer_p);
+	GIT_UNUSED(len_p);
+	GIT_UNUSED(type_p);
+	GIT_UNUSED(oid);
+
+	fake = (fake_backend *)backend;
+
+	fake->read_calls++;
+
+	*len_p = 0;
+	*buffer_p = NULL;
+	*type_p = GIT_OBJ_BLOB;
+
+	return fake->error_code;
+}
+
+static int fake_backend__read_header(
+	size_t *len_p, git_otype *type_p,
+	git_odb_backend *backend, const git_oid *oid)
+{
+	fake_backend *fake;
+
+	GIT_UNUSED(len_p);
+	GIT_UNUSED(type_p);
+	GIT_UNUSED(oid);
+
+	fake = (fake_backend *)backend;
+
+	fake->read_header_calls++;
+
+	*len_p = 0;
+	*type_p = GIT_OBJ_BLOB;
+
+	return fake->error_code;
+}
+
+static int fake_backend__read_prefix(
+	git_oid *out_oid, void **buffer_p, size_t *len_p, git_otype *type_p,
+	git_odb_backend *backend, const git_oid *short_oid,	size_t len)
+{
+	fake_backend *fake;
+
+	GIT_UNUSED(out_oid);
+	GIT_UNUSED(buffer_p);
+	GIT_UNUSED(len_p);
+	GIT_UNUSED(type_p);
+	GIT_UNUSED(short_oid);
+	GIT_UNUSED(len);
+
+	fake = (fake_backend *)backend;
+
+	fake->read_prefix_calls++;
+
+	*len_p = 0;
+	*buffer_p = NULL;
+	*type_p = GIT_OBJ_BLOB;
+
+	return fake->error_code;
+}
+
+static void fake_backend__free(git_odb_backend *_backend)
+{
+	fake_backend *backend;
+
+	backend = (fake_backend *)_backend;
+
+	git__free(backend);
+}
+
+static int build_fake_backend(
+	git_odb_backend **out,
+	git_error_code error_code)
+{
+	fake_backend *backend;
+
+	backend = git__calloc(1, sizeof(fake_backend));
+	GITERR_CHECK_ALLOC(backend);
+
+	backend->parent.version = GIT_ODB_BACKEND_VERSION;
+
+	backend->parent.refresh = NULL;
+	backend->error_code = error_code;
+
+	backend->parent.read = fake_backend__read;
+	backend->parent.read_prefix = fake_backend__read_prefix;
+	backend->parent.read_header = fake_backend__read_header;
+	backend->parent.exists = fake_backend__exists;
+	backend->parent.free = &fake_backend__free;
+
+	*out = (git_odb_backend *)backend;
+
+	return 0;
+}
+
+static void setup_repository_and_backend(git_error_code error_code)
+{
+	git_odb *odb;
+	git_odb_backend *backend;
+
+	_repo = cl_git_sandbox_init("testrepo.git");
+
+	cl_git_pass(build_fake_backend(&backend, error_code));
+
+	cl_git_pass(git_repository_odb__weakptr(&odb, _repo));
+	cl_git_pass(git_odb_add_backend(odb, backend, 10));
+
+	_fake = (fake_backend *)backend;
+
+	cl_git_pass(git_oid_fromstr(&_oid, "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
+}
+
+void test_odb_backend_nonrefreshing__cleanup(void)
+{
+	cl_git_sandbox_cleanup();
+}
+
+void test_odb_backend_nonrefreshing__exists_is_invoked_once_on_failure(void)
+{
+	git_odb *odb;
+
+	setup_repository_and_backend(GIT_ENOTFOUND);
+
+	cl_git_pass(git_repository_odb__weakptr(&odb, _repo));
+	cl_assert_equal_b(false, git_odb_exists(odb, &_oid));
+
+	cl_assert_equal_i(1, _fake->exists_calls);
+}
+
+void test_odb_backend_nonrefreshing__read_is_invoked_once_on_failure(void)
+{
+	git_object *obj;
+
+	setup_repository_and_backend(GIT_ENOTFOUND);
+
+	cl_git_fail_with(
+		git_object_lookup(&obj, _repo, &_oid, GIT_OBJ_ANY),
+		GIT_ENOTFOUND);
+
+	cl_assert_equal_i(1, _fake->read_calls);
+}
+
+void test_odb_backend_nonrefreshing__readprefix_is_invoked_once_on_failure(void)
+{
+	git_object *obj;
+
+	setup_repository_and_backend(GIT_ENOTFOUND);
+
+	cl_git_fail_with(
+		git_object_lookup_prefix(&obj, _repo, &_oid, 7, GIT_OBJ_ANY),
+		GIT_ENOTFOUND);
+
+	cl_assert_equal_i(1, _fake->read_prefix_calls);
+}
+
+void test_odb_backend_nonrefreshing__readheader_is_invoked_once_on_failure(void)
+{
+	git_odb *odb;
+	size_t len;
+	git_otype type;
+
+	setup_repository_and_backend(GIT_ENOTFOUND);
+
+	cl_git_pass(git_repository_odb__weakptr(&odb, _repo));
+
+	cl_git_fail_with(
+		git_odb_read_header(&len, &type, odb, &_oid),
+		GIT_ENOTFOUND);
+
+	cl_assert_equal_i(1, _fake->read_header_calls);
+}
+
+void test_odb_backend_nonrefreshing__exists_is_invoked_once_on_success(void)
+{
+	git_odb *odb;
+
+	setup_repository_and_backend(GIT_OK);
+
+	cl_git_pass(git_repository_odb__weakptr(&odb, _repo));
+	cl_assert_equal_b(true, git_odb_exists(odb, &_oid));
+
+	cl_assert_equal_i(1, _fake->exists_calls);
+}
+
+void test_odb_backend_nonrefreshing__read_is_invoked_once_on_success(void)
+{
+	git_object *obj;
+
+	setup_repository_and_backend(GIT_OK);
+
+	cl_git_pass(git_object_lookup(&obj, _repo, &_oid, GIT_OBJ_ANY));
+
+	cl_assert_equal_i(1, _fake->read_calls);
+
+	git_object_free(obj);
+}
+
+void test_odb_backend_nonrefreshing__readprefix_is_invoked_once_on_success(void)
+{
+	git_object *obj;
+
+	setup_repository_and_backend(GIT_OK);
+
+	cl_git_pass(git_object_lookup_prefix(&obj, _repo, &_oid, 7, GIT_OBJ_ANY));
+
+	cl_assert_equal_i(1, _fake->read_prefix_calls);
+
+	git_object_free(obj);
+}
+
+void test_odb_backend_nonrefreshing__readheader_is_invoked_once_on_success(void)
+{
+	git_odb *odb;
+	size_t len;
+	git_otype type;
+
+	setup_repository_and_backend(GIT_OK);
+
+	cl_git_pass(git_repository_odb__weakptr(&odb, _repo));
+
+	cl_git_pass(git_odb_read_header(&len, &type, odb, &_oid));
+
+	cl_assert_equal_i(1, _fake->read_header_calls);
+}