[PATCH 0/5] odb/files: cleanups and improvements for alternates

[PATCH 0/5] odb/files: cleanups and improvements for alternates

[Patrick Steinhardt]

Hi,

this patch series contains a couple of follow-up cleanups and
improvements that are now possible after having migrated alternates into
the "files" backend.

The series is built on top of 6de20f6092 (The 4th batch, 2026-10-06)
with ps/odb-files-alternates at 0ba59c8453 (odb/source: drop
`read_alternates` callback, 2026-10-08) merged into it.

Thanks!

Patrick

---
Patrick Steinhardt (5):
      odb/source-files: fix performance when reading objects with alternates
      odb/source-files: fix performance when streaming objects with alternates
      odb/source-files: make the second read an implementation detail
      odb: remove read loop for found objects
      odb/source: drop `local` flag

 midx.c                |  2 +-
 odb.c                 | 61 +++++++++++++++++++--------------------------------
 odb.h                 | 12 ----------
 odb/source-files.c    | 26 +++++++++++++++++-----
 odb/source-inmemory.c | 10 ++++++---
 odb/source-loose.c    | 17 ++------------
 odb/source-loose.h    |  3 +--
 odb/source-packed.c   | 20 +++++------------
 odb/source-packed.h   |  7 ++++++
 odb/source.c          |  4 +---
 odb/source.h          | 20 +----------------
 repack.c              |  2 +-
 12 files changed, 71 insertions(+), 113 deletions(-)


---
base-commit: ea99757de7d4ef6c7727431bfd7b6726614102e0
change-id: 20261006-pks-odb-files-alternates-cleanups-11f686cc4942

[PATCH 1/5] odb/source-files: fix performance when reading objects with alternates

[Patrick Steinhardt]

With the move towards pluggable object databases we reorganized how we
access alternates. Before the introduction of pluggability, we were
reading objects by first scanning through packfiles from all alternates
before scanning through all loose object directories. With the change
though, we are now iterating through the sources one by one, checking
each of them for whether they contain the object either as a packed or
loose object.

Checking whether an object exists in packfiles is comparatively cheap
because the list of packs is cached, and the pack indices are likely to
be in the page cache. In contrast to that, checking whether an object
exists in loose format always incurs one stat(3p) syscall. So scanning
for objects one alternate at a time led to a performance regression in
in the case where most of the objects are contained in a packfile of an
alternate, as reported in [1].

We could fix that issue by introducing another flag that tells the
backends to only search the optimized storage backend. But that is
adding more complexity to the object database layers that is ultimately
only really required because of the way the "files" backend stores its
data. So for an alternate backend, such new infrastructure would not
really be a good fit.

But since then we have moved handling of alternates into the "files"
backend itself. One of the consequences of this is that this backend is
now having full control over all of its sub-sources, and that also makes
it way easier for us to optimize for this access pattern again.

Do so by first trying to read objects from packfiles located in any of
its object directories before trying to read loose objects.

The following benchmark demonstrates that this significantly improves
performance in some cases. The benchmark uses the Git repository itself
and sets up its object directory as an alternate, while the primary
object directory is simply an empty object directory. It then reads all
commits enumerated via `git rev-list HEAD` with git-cat-file(1):

  Benchmark 1: git cat-file --batch-check --unordered (rev = HEAD~)
    Time (mean ± σ):     179.4 ms ±   2.6 ms    [User: 114.6 ms, System: 61.4 ms]
    Range (min … max):   177.1 ms … 187.7 ms    16 runs

  Benchmark 2: git cat-file --batch-check --unordered (rev = HEAD)
    Time (mean ± σ):      95.0 ms ±   1.5 ms    [User: 79.8 ms, System: 15.0 ms]
    Range (min … max):    93.0 ms …  97.9 ms    30 runs

  Summary
    git cat-file --batch-check --unordered (rev = HEAD) ran
      1.89 ± 0.04 times faster than git cat-file --batch-check --unordered (rev = HEAD~)

[1]: <CAFXJcxvpKHoVDwE5mBOd=w-A5vPdUmehqr8SHLUD7qv1qB00rA@mail.gmail.com>

Reported-by: Wolfgang Kritzinger <wkritzinger@atlassian.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 odb/source-files.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/odb/source-files.c b/odb/source-files.c
index 455f409db0..9268f5aa9a 100644
--- a/odb/source-files.c
+++ b/odb/source-files.c
@@ -426,6 +426,7 @@ static enum odb_read_status odb_source_files_read_object_info(struct odb_source
 {
 	struct odb_source_files *files = odb_source_files_downcast(source);
 	enum odb_read_status status = ODB_READ_NOT_FOUND;
+	enum odb_read_status ret;
 
 	/*
 	 * Reading an object may fail even though the object exists, for
@@ -434,15 +435,15 @@ static enum odb_read_status odb_source_files_read_object_info(struct odb_source
 	 * prefer the first such error in case multiple reads have failed.
 	 */
 	for (struct odb_files_dir *dir = files->dirs; dir; dir = dir->next) {
-		enum odb_read_status ret;
-
 		ret = odb_source_read_object_info(&dir->packed->base, oid, oi, flags,
 						  status == ODB_READ_NOT_FOUND ? errmsg : NULL);
 		if (!ret)
 			return 0;
 		if (ret != ODB_READ_NOT_FOUND && status == ODB_READ_NOT_FOUND)
 			status = ret;
+	}
 
+	for (struct odb_files_dir *dir = files->dirs; dir; dir = dir->next) {
 		ret = odb_source_read_object_info(&dir->loose->base, oid, oi, flags,
 						  status == ODB_READ_NOT_FOUND ? errmsg : NULL);
 		if (!ret)

-- 
2.56.0.212.g129ede20d9.dirty

[PATCH 2/5] odb/source-files: fix performance when streaming objects with alternates

[Patrick Steinhardt]

This patch is essentially the equivalent to the preceding patch, but for
streaming objects instead of just reading them directly.

Exercising performance is a bit more complicated here as streaming only
really kicks in for large objects. Furthermore, I expect that the impact
is not as big as with the last commit, either, because for large objects
the time to stat the file will probably be dwarfed by the time it takes
to read the object itself.

But even though this is probably not as impactful, it makes sense anyway
to do this change to align behaviour across both of these functions.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 odb/source-files.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/odb/source-files.c b/odb/source-files.c
index 9268f5aa9a..54235821d5 100644
--- a/odb/source-files.c
+++ b/odb/source-files.c
@@ -462,8 +462,10 @@ static int odb_source_files_read_object_stream(struct odb_stream **out,
 	struct odb_source_files *files = odb_source_files_downcast(source);
 
 	for (struct odb_files_dir *dir = files->dirs; dir; dir = dir->next)
-		if (!odb_source_read_object_stream(out, &dir->packed->base, oid) ||
-		    !odb_source_read_object_stream(out, &dir->loose->base, oid))
+		if (!odb_source_read_object_stream(out, &dir->packed->base, oid))
+			return 0;
+	for (struct odb_files_dir *dir = files->dirs; dir; dir = dir->next)
+		if (!odb_source_read_object_stream(out, &dir->loose->base, oid))
 			return 0;
 
 	return -1;

-- 
2.56.0.212.g129ede20d9.dirty

[PATCH 3/5] odb/source-files: make the second read an implementation detail

[Patrick Steinhardt]

When looking up an object fails we perform a second read in all sources
with the `OBJECT_INFO_SECOND_READ` flag set. This flag tells sources
that the first lookup has failed and that they should reload any on-disk
state that may have changed in the meantime so that the object may be
surfaced after all. The "packed" source uses this to reprepare its
packfiles, whereas the "loose" source short-circuits the second read
because there are no caches to flush.

This logic lives in the generic object database layer even though it is
highly specific to how the "files" backend handles its reads. But now
that alternates are an implementation detail of that source, we can
easily move that logic into the backend, as well.

Move the logic into the "files" source and drop the flag.

Note that the "packed" source also used this flag to figure out whether
it should try to surface an object via a packfile that should have the
object as determined by a multi-pack index, but where the packfile has
vanished. The multi-pack index is stale in that case, and that scenario
should really only happen when the process is racing with a concurrent
maintenance task.

This code is adapted to instead use `OBJECT_INFO_QUICK`, which means
that the fallback may trigger on the first read already, but not in a
case where callers know to handle false negatives already. In the worst
case this could cause another scan through all packfiles covered by the
multi-pack index. But that scan should be fast enough anyway, and it
only triggers when we hit the race. So the effect thereof should be
limited.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 odb.c               | 15 ---------------
 odb.h               | 12 ------------
 odb/source-files.c  | 13 +++++++++++++
 odb/source-loose.c  | 10 ----------
 odb/source-packed.c | 15 +++------------
 odb/source.h        |  7 -------
 6 files changed, 16 insertions(+), 56 deletions(-)

diff --git a/odb.c b/odb.c
index 98c5c862f5..69a4c203bc 100644
--- a/odb.c
+++ b/odb.c
@@ -285,21 +285,6 @@ static enum odb_read_status do_oid_object_info_extended(struct object_database *
 		if (ret != ODB_READ_NOT_FOUND)
 			corrupt = true;
 
-		/*
-		 * When the object hasn't been found we try a second read and
-		 * tell the sources so. This may cause them to invalidate
-		 * caches or reload on-disk state.
-		 */
-		if (!(flags & OBJECT_INFO_QUICK)) {
-			ret = odb_source_read_object_info(odb->source, real, oi,
-							  flags | OBJECT_INFO_SECOND_READ,
-							  corrupt_err.len ? NULL : &corrupt_err);
-			if (!ret)
-				goto out;
-			if (ret != ODB_READ_NOT_FOUND)
-				corrupt = true;
-		}
-
 		/* Check if it is a missing object */
 		if (odb->repo->fetch_if_missing && repo_has_promisor_remote(odb->repo) &&
 		    !already_retried &&
diff --git a/odb.h b/odb.h
index 0ccc47ee73..dd85b880a7 100644
--- a/odb.h
+++ b/odb.h
@@ -383,18 +383,6 @@ enum object_info_flags {
 	/* Die if object corruption (not just an object being missing) was detected. */
 	OBJECT_INFO_DIE_IF_CORRUPT = (1 << 3),
 
-	/*
-	 * We have already tried reading the object, but it couldn't be found
-	 * via any of the attached sources, and are now doing a second read.
-	 * This second read asks the individual sources to also evaluate
-	 * whether any on-disk state may have changed that may have caused the
-	 * object to appear.
-	 *
-	 * This flag is for internal use, only. The second read only occurs
-	 * when `OBJECT_INFO_QUICK` was not passed.
-	 */
-	OBJECT_INFO_SECOND_READ = (1 << 4),
-
 	/*
 	 * This is meant for bulk prefetching of missing blobs in a partial
 	 * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK.
diff --git a/odb/source-files.c b/odb/source-files.c
index 54235821d5..9788a5adf4 100644
--- a/odb/source-files.c
+++ b/odb/source-files.c
@@ -427,7 +427,9 @@ static enum odb_read_status odb_source_files_read_object_info(struct odb_source
 	struct odb_source_files *files = odb_source_files_downcast(source);
 	enum odb_read_status status = ODB_READ_NOT_FOUND;
 	enum odb_read_status ret;
+	bool retry = false;
 
+retry:
 	/*
 	 * Reading an object may fail even though the object exists, for
 	 * example because it is corrupt. Report this failure to the caller in
@@ -435,6 +437,9 @@ static enum odb_read_status odb_source_files_read_object_info(struct odb_source
 	 * prefer the first such error in case multiple reads have failed.
 	 */
 	for (struct odb_files_dir *dir = files->dirs; dir; dir = dir->next) {
+		if (retry)
+			odb_source_prepare(&dir->packed->base, ODB_PREPARE_FLUSH_CACHES);
+
 		ret = odb_source_read_object_info(&dir->packed->base, oid, oi, flags,
 						  status == ODB_READ_NOT_FOUND ? errmsg : NULL);
 		if (!ret)
@@ -444,6 +449,9 @@ static enum odb_read_status odb_source_files_read_object_info(struct odb_source
 	}
 
 	for (struct odb_files_dir *dir = files->dirs; dir; dir = dir->next) {
+		if (retry)
+			break;
+
 		ret = odb_source_read_object_info(&dir->loose->base, oid, oi, flags,
 						  status == ODB_READ_NOT_FOUND ? errmsg : NULL);
 		if (!ret)
@@ -452,6 +460,11 @@ static enum odb_read_status odb_source_files_read_object_info(struct odb_source
 			status = ret;
 	}
 
+	if (!retry && !(flags & OBJECT_INFO_QUICK)) {
+		retry = true;
+		goto retry;
+	}
+
 	return status;
 }
 
diff --git a/odb/source-loose.c b/odb/source-loose.c
index 0664634938..180ce7403f 100644
--- a/odb/source-loose.c
+++ b/odb/source-loose.c
@@ -222,16 +222,6 @@ static enum odb_read_status odb_source_loose_read_object_info(struct odb_source
 {
 	struct odb_source_loose *loose = odb_source_loose_downcast(source);
 	static struct strbuf buf = STRBUF_INIT;
-
-	/*
-	 * The second read shouldn't cause new loose objects to show up, unless
-	 * there was a race condition with a secondary process. We don't care
-	 * about this case though, so we simply skip reading loose objects a
-	 * second time.
-	 */
-	if (flags & OBJECT_INFO_SECOND_READ)
-		return ODB_READ_NOT_FOUND;
-
 	odb_loose_path(loose, &buf, oid);
 	return read_object_info_from_path(loose, buf.buf, oid, oi, flags, errmsg);
 }
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 2226fbd313..48fe00c2bc 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -52,12 +52,11 @@ static int find_pack_entry(struct odb_source_packed *store,
 	 * packs, and repreparing the on-disk pack set does not reload the
 	 * borrowed, cached MIDX, so scan its packs directly for the survivor.
 	 *
-	 * Do this only on the second read, by which point repreparing packs has
-	 * already had a chance to find an object merely relocated into a new,
-	 * uncovered pack; only a genuine hidden duplicate reaches here.
+	 * Skip the scan for quick lookups, which are expected to accept false
+	 * negatives in favor of speed.
 	 */
 	if (midx_result == MIDX_FILL_OWNER_UNAVAILABLE &&
-	    (flags & OBJECT_INFO_SECOND_READ)) {
+	    !(flags & OBJECT_INFO_QUICK)) {
 		struct multi_pack_index *m = store->midx;
 		uint32_t i;
 
@@ -86,14 +85,6 @@ static enum odb_read_status odb_source_packed_read_object_info(struct odb_source
 	struct pack_entry e;
 	int ret;
 
-	/*
-	 * In case the first read didn't surface the object, we have to reload
-	 * packfiles. This may cause us to discover new packfiles that have
-	 * been added since the last time we have prepared the packfile store.
-	 */
-	if (flags & OBJECT_INFO_SECOND_READ)
-		odb_source_prepare(source, ODB_PREPARE_FLUSH_CACHES);
-
 	if (!find_pack_entry(packed, oid, &e, flags, &bad_pack)) {
 		/*
 		 * The lookup may have failed because the object is known to be
diff --git a/odb/source.h b/odb/source.h
index 1867347dfc..0cfe762376 100644
--- a/odb/source.h
+++ b/odb/source.h
@@ -119,13 +119,6 @@ struct odb_source {
 	 *   - `OBJECT_INFO_QUICK` indicates it is fine to use caches without
 	 *     re-verifying the data.
 	 *
-	 *   - `OBJECT_INFO_SECOND_READ` indicates that the initial object
-	 *     lookup has failed and that the object sources should check
-	 *     whether any of its on-disk state has changed that may have
-	 *     caused the object to appear. Sources are free to ignore the
-	 *     second read in case they know that the first read would have
-	 *     already surfaced the object without reloading any on-disk state.
-	 *
 	 * The callback is expected to return an `enum odb_read_status`. Please
 	 * refer to the individual values that can be returned. In case reading
 	 * the object has failed with a generic error and `errmsg` is non-NULL,

-- 
2.56.0.212.g129ede20d9.dirty

[PATCH 4/5] odb: remove read loop for found objects

[Patrick Steinhardt]

Before moving alternates into the "files" backend we had to handle
alternates in `do_oid_object_info_extended()`. This made the retry logic
somewhat complex, as we had to juggle reloading of the sources, the
second read and fetching missing objects. And to handle the potential
retries we loop around that logic multiple times.

Nowadays though, the reload and retry logic are located in the "files"
backend itself. Consequently, using a loop is a bit on the heavy side
and makes the code harder to reason about.

Simplify the logic by removing the loop. Instead, we explicitly re-read
exactly once after having fetched objects from a promisor remote.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 odb.c | 46 +++++++++++++++++++++++-----------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/odb.c b/odb.c
index 69a4c203bc..095183aec0 100644
--- a/odb.c
+++ b/odb.c
@@ -265,7 +265,6 @@ static enum odb_read_status do_oid_object_info_extended(struct object_database *
 	struct strbuf corrupt_err = STRBUF_INIT;
 	const struct object_id *real = oid;
 	enum odb_read_status ret;
-	int already_retried = 0;
 	bool corrupt = false;
 
 	if (flags & OBJECT_INFO_LOOKUP_REPLACE)
@@ -277,38 +276,39 @@ static enum odb_read_status do_oid_object_info_extended(struct object_database *
 	if (!odb_source_read_object_info(odb->inmemory_objects, oid, oi, flags, NULL))
 		return 0;
 
-	while (1) {
+	ret = odb_source_read_object_info(odb->source, real, oi, flags,
+					  &corrupt_err);
+	if (!ret)
+		goto out;
+	if (ret != ODB_READ_NOT_FOUND)
+		corrupt = true;
+
+	/* Try to fetch the missing object from a promisor remote. */
+	if (odb->repo->fetch_if_missing && repo_has_promisor_remote(odb->repo) &&
+	    !(flags & OBJECT_INFO_SKIP_FETCH_OBJECT)) {
+		promisor_remote_get_direct(odb->repo, real, 1);
+
 		ret = odb_source_read_object_info(odb->source, real, oi, flags,
 						  corrupt_err.len ? NULL : &corrupt_err);
 		if (!ret)
 			goto out;
 		if (ret != ODB_READ_NOT_FOUND)
 			corrupt = true;
+	}
 
-		/* Check if it is a missing object */
-		if (odb->repo->fetch_if_missing && repo_has_promisor_remote(odb->repo) &&
-		    !already_retried &&
-		    !(flags & OBJECT_INFO_SKIP_FETCH_OBJECT)) {
-			promisor_remote_get_direct(odb->repo, real, 1);
-			already_retried = 1;
-			continue;
-		}
-
-		if (flags & OBJECT_INFO_DIE_IF_CORRUPT) {
-			if ((flags & OBJECT_INFO_LOOKUP_REPLACE) && !oideq(real, oid))
-				die(_("replacement %s not found for %s"),
-				    oid_to_hex(real), oid_to_hex(oid));
-			if (corrupt) {
-				if (corrupt_err.len)
-					die("%s", corrupt_err.buf);
-				die(_("object %s is corrupt"), oid_to_hex(real));
-			}
+	if (flags & OBJECT_INFO_DIE_IF_CORRUPT) {
+		if ((flags & OBJECT_INFO_LOOKUP_REPLACE) && !oideq(real, oid))
+			die(_("replacement %s not found for %s"),
+			    oid_to_hex(real), oid_to_hex(oid));
+		if (corrupt) {
+			if (corrupt_err.len)
+				die("%s", corrupt_err.buf);
+			die(_("object %s is corrupt"), oid_to_hex(real));
 		}
-
-		ret = corrupt ? ODB_READ_ERROR : ODB_READ_NOT_FOUND;
-		goto out;
 	}
 
+	ret = corrupt ? ODB_READ_ERROR : ODB_READ_NOT_FOUND;
+
 out:
 	strbuf_release(&corrupt_err);
 	return ret;

-- 
2.56.0.212.g129ede20d9.dirty

[PATCH 5/5] odb/source: drop `local` flag

[Patrick Steinhardt]

The `local` flag indicates whether a given `struct odb_source` is a
local source or not, where "local" is defined as being the object
directory that is directly attached to the current repository. But now
that alternates are handled in the "files" backend, this property is not
something that even makes sense anymore to have on the generic source.

In fact, we already have a partial replacement for this field in `struct
odb_files_dir`. The only other site that requires it is the packed
object database source, because it propagates the bit into the packfiles
it is opening.

Drop the flag from `struct odb_source` and instead move it into the
packed source.

Note that we stop considering the `local` bit when iterating through all
loose objects with the `ODB_FOR_EACH_OBJECT_LOCAL_ONLY` flag. This is
fine though, as we already handle the flag in the "files" backend
itself, where we skip over any object directory that is not marked as
local.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 midx.c                |  2 +-
 odb/source-files.c    |  4 ++--
 odb/source-inmemory.c | 10 +++++++---
 odb/source-loose.c    |  7 ++-----
 odb/source-loose.h    |  3 +--
 odb/source-packed.c   |  5 +++--
 odb/source-packed.h   |  7 +++++++
 odb/source.c          |  4 +---
 odb/source.h          | 13 +------------
 repack.c              |  2 +-
 10 files changed, 26 insertions(+), 31 deletions(-)

diff --git a/midx.c b/midx.c
index dc0c728401..2d4feeeade 100644
--- a/midx.c
+++ b/midx.c
@@ -470,7 +470,7 @@ int prepare_midx_pack(struct multi_pack_index *m,
 	strbuf_addf(&pack_name, "%s/pack/%s", packed->base.path,
 		    m->pack_names[pack_int_id]);
 	p = packfile_store_load_pack(packed,
-				     pack_name.buf, packed->base.local);
+				     pack_name.buf, packed->local);
 	strbuf_release(&pack_name);
 
 	if (!p) {
diff --git a/odb/source-files.c b/odb/source-files.c
index 9788a5adf4..5b4277fbfd 100644
--- a/odb/source-files.c
+++ b/odb/source-files.c
@@ -33,7 +33,7 @@ struct odb_files_dir *odb_files_dir_new(struct object_database *odb,
 	CALLOC_ARRAY(dir, 1);
 	dir->abspath = absolute_pathdup(path);
 	dir->local = local;
-	dir->loose = odb_source_loose_new(odb, path, local);
+	dir->loose = odb_source_loose_new(odb, path);
 	dir->packed = odb_source_packed_new(odb, path, local);
 
 	return dir;
@@ -1265,7 +1265,7 @@ struct odb_source_files *odb_source_files_new(struct object_database *odb,
 		object_dir = xstrfmt("%s/objects", odb->repo->commondir);
 
 	CALLOC_ARRAY(files, 1);
-	odb_source_init(&files->base, odb, ODB_SOURCE_FILES, object_dir, true);
+	odb_source_init(&files->base, odb, ODB_SOURCE_FILES, object_dir);
 
 	hashmap_init(&files->dirs_by_path, odb_files_dir_by_path_cmp, files, 0);
 	files->dirs_paths_icase = -1;
diff --git a/odb/source-inmemory.c b/odb/source-inmemory.c
index 868e723c81..75cf29fa9d 100644
--- a/odb/source-inmemory.c
+++ b/odb/source-inmemory.c
@@ -161,8 +161,12 @@ static int odb_source_inmemory_for_each_object(struct odb_source *source,
 	};
 	struct object_id null_oid = { 0 };
 
-	if ((opts->flags & ODB_FOR_EACH_OBJECT_PROMISOR_ONLY) ||
-	    (opts->flags & ODB_FOR_EACH_OBJECT_LOCAL_ONLY && !source->local))
+	/*
+	 * In-memory objects are neither promisor objects nor do they live in
+	 * the local object directory, so there is nothing to iterate over.
+	 */
+	if (opts->flags & (ODB_FOR_EACH_OBJECT_PROMISOR_ONLY |
+			   ODB_FOR_EACH_OBJECT_LOCAL_ONLY))
 		return 0;
 	if (!inmemory->objects)
 		return 0;
@@ -369,7 +373,7 @@ struct odb_source_inmemory *odb_source_inmemory_new(struct object_database *odb)
 	struct odb_source_inmemory *source;
 
 	CALLOC_ARRAY(source, 1);
-	odb_source_init(&source->base, odb, ODB_SOURCE_INMEMORY, "source", false);
+	odb_source_init(&source->base, odb, ODB_SOURCE_INMEMORY, "source");
 
 	source->base.free = odb_source_inmemory_free;
 	source->base.close = odb_source_inmemory_close;
diff --git a/odb/source-loose.c b/odb/source-loose.c
index 180ce7403f..50cb388c35 100644
--- a/odb/source-loose.c
+++ b/odb/source-loose.c
@@ -464,8 +464,6 @@ static int odb_source_loose_for_each_object(struct odb_source *source,
 	/* There are no loose promisor objects, so we can return immediately. */
 	if ((opts->flags & ODB_FOR_EACH_OBJECT_PROMISOR_ONLY))
 		return 0;
-	if ((opts->flags & ODB_FOR_EACH_OBJECT_LOCAL_ONLY) && !source->local)
-		return 0;
 
 	if (opts->prefix)
 		return oidtree_each(odb_source_loose_cache(loose, opts->prefix),
@@ -1108,13 +1106,12 @@ static int odb_source_loose_fsck(struct odb_source *source,
 }
 
 struct odb_source_loose *odb_source_loose_new(struct object_database *odb,
-					      const char *path,
-					      bool local)
+					      const char *path)
 {
 	struct odb_source_loose *loose;
 
 	CALLOC_ARRAY(loose, 1);
-	odb_source_init(&loose->base, odb, ODB_SOURCE_LOOSE, path, local);
+	odb_source_init(&loose->base, odb, ODB_SOURCE_LOOSE, path);
 
 	loose->base.free = odb_source_loose_free;
 	loose->base.close = odb_source_loose_close;
diff --git a/odb/source-loose.h b/odb/source-loose.h
index 7c7e845fe3..a6ae808662 100644
--- a/odb/source-loose.h
+++ b/odb/source-loose.h
@@ -34,8 +34,7 @@ struct odb_source_loose {
 };
 
 struct odb_source_loose *odb_source_loose_new(struct object_database *odb,
-					      const char *path,
-					      bool local);
+					      const char *path);
 
 /*
  * Cast the given object database source to the loose backend. This will cause
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 48fe00c2bc..7fcbbb4fe9 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -740,7 +740,7 @@ static void prepare_pack(const char *full_name, size_t full_name_len,
 	      midx_contains_pack(data->source->midx, file_name))) {
 		char *trimmed_path = xstrndup(full_name, full_name_len);
 		packfile_store_load_pack(data->source,
-					 trimmed_path, data->source->base.local);
+					 trimmed_path, data->source->local);
 		free(trimmed_path);
 	}
 
@@ -988,8 +988,9 @@ struct odb_source_packed *odb_source_packed_new(struct object_database *odb,
 	struct odb_source_packed *packed;
 
 	CALLOC_ARRAY(packed, 1);
-	odb_source_init(&packed->base, odb, ODB_SOURCE_PACKED, path, local);
+	odb_source_init(&packed->base, odb, ODB_SOURCE_PACKED, path);
 	strmap_init(&packed->packs_by_path);
+	packed->local = local;
 
 	packed->base.free = odb_source_packed_free;
 	packed->base.close = odb_source_packed_close;
diff --git a/odb/source-packed.h b/odb/source-packed.h
index a0f6b5096d..e8a7b08fbb 100644
--- a/odb/source-packed.h
+++ b/odb/source-packed.h
@@ -41,6 +41,13 @@ struct odb_source_packed {
 	 */
 	struct strmap packs_by_path;
 
+	/*
+	 * Whether this store is part of the local object directory of the
+	 * owning repository. Packfiles loaded via this store inherit this
+	 * flag via `pack_local`. Stores for alternates are not local.
+	 */
+	bool local;
+
 	/*
 	 * Whether packfiles have already been populated with this store's
 	 * packs.
diff --git a/odb/source.c b/odb/source.c
index b25ef14df8..4155fc22da 100644
--- a/odb/source.c
+++ b/odb/source.c
@@ -32,12 +32,10 @@ struct odb_source *odb_source_new(struct object_database *odb,
 void odb_source_init(struct odb_source *source,
 		     struct object_database *odb,
 		     enum odb_source_type type,
-		     const char *path,
-		     bool local)
+		     const char *path)
 {
 	source->odb = odb;
 	source->type = type;
-	source->local = local;
 	source->path = xstrdup(path);
 }
 
diff --git a/odb/source.h b/odb/source.h
index 0cfe762376..8b839caa39 100644
--- a/odb/source.h
+++ b/odb/source.h
@@ -58,14 +58,6 @@ struct odb_source {
 	/* The type used by this source. */
 	enum odb_source_type type;
 
-	/*
-	 * Figure out whether this is the local source of the owning
-	 * repository, which would typically be its ".git/objects" directory.
-	 * This local object directory is usually where objects would be
-	 * written to.
-	 */
-	bool local;
-
 	/*
 	 * Path to the source. If this is a relative path, it is relative to
 	 * the current working directory.
@@ -304,8 +296,6 @@ struct odb_source *odb_source_new(struct object_database *odb,
 
 /*
  * Initialize the source for the given object database located at `path`.
- * `local` indicates whether or not the source is the local and thus primary
- * object source of the object database.
  *
  * This function is only supposed to be called by specific object source
  * implementations.
@@ -313,8 +303,7 @@ struct odb_source *odb_source_new(struct object_database *odb,
 void odb_source_init(struct odb_source *source,
 		     struct object_database *odb,
 		     enum odb_source_type type,
-		     const char *path,
-		     bool local);
+		     const char *path);
 
 /*
  * Free the object database source, releasing all associated resources and
diff --git a/repack.c b/repack.c
index 53c34197f3..4c6d1f507d 100644
--- a/repack.c
+++ b/repack.c
@@ -62,7 +62,7 @@ void repack_remove_redundant_pack(struct repository *repo, const char *dir_name,
 	struct odb_source_files *files = odb_source_files_downcast(repo->objects->source);
 	struct multi_pack_index *m = get_multi_pack_index(files->dirs->packed);
 	strbuf_addf(&buf, "%s.pack", base_name);
-	if (m && files->base.local && midx_contains_pack(m, buf.buf)) {
+	if (m && files->dirs->local && midx_contains_pack(m, buf.buf)) {
 		clear_midx_file(repo);
 		if (!wrote_incremental_midx)
 			clear_incremental_midx_files(repo, NULL);

-- 
2.56.0.212.g129ede20d9.dirty

Re: [PATCH 1/5] odb/source-files: fix performance when reading objects with alternates

[Justin Tobler]

On 26/10/13 09:36AM, Patrick Steinhardt wrote:
> With the move towards pluggable object databases we reorganized how we
> access alternates. Before the introduction of pluggability, we were
> reading objects by first scanning through packfiles from all alternates
> before scanning through all loose object directories. With the change
> though, we are now iterating through the sources one by one, checking
> each of them for whether they contain the object either as a packed or
> loose object.

Ok, ideally we want to go back to checking all the packfiles across each
source for a given object before looking through the loose objects. This
would be more performant. Make sense.

> Checking whether an object exists in packfiles is comparatively cheap
> because the list of packs is cached, and the pack indices are likely to
> be in the page cache. In contrast to that, checking whether an object
> exists in loose format always incurs one stat(3p) syscall. So scanning
> for objects one alternate at a time led to a performance regression in
> in the case where most of the objects are contained in a packfile of an
> alternate, as reported in [1].

Ok.

> We could fix that issue by introducing another flag that tells the
> backends to only search the optimized storage backend. But that is
> adding more complexity to the object database layers that is ultimately
> only really required because of the way the "files" backend stores its
> data. So for an alternate backend, such new infrastructure would not
> really be a good fit.

I completely agree. Introducing a special flag here would really only
serve the "files" backend and also make the overall interface more
confusing. It would be best to capture the desired behavior as an
implementation detail of the "files" backend itself.

> But since then we have moved handling of alternates into the "files"
> backend itself. One of the consequences of this is that this backend is
> now having full control over all of its sub-sources, and that also makes
> it way easier for us to optimize for this access pattern again.

Yup, now that alternates are controlled/managed by the "files" backend
itself instead of being discrete sources themselves, it becomes possible
us to update how objects are read internal to the source.

> Do so by first trying to read objects from packfiles located in any of
> its object directories before trying to read loose objects.

Make sense.

> The following benchmark demonstrates that this significantly improves
> performance in some cases. The benchmark uses the Git repository itself
> and sets up its object directory as an alternate, while the primary
> object directory is simply an empty object directory. It then reads all
> commits enumerated via `git rev-list HEAD` with git-cat-file(1):
> 
>   Benchmark 1: git cat-file --batch-check --unordered (rev = HEAD~)
>     Time (mean ± σ):     179.4 ms ±   2.6 ms    [User: 114.6 ms, System: 61.4 ms]
>     Range (min … max):   177.1 ms … 187.7 ms    16 runs
> 
>   Benchmark 2: git cat-file --batch-check --unordered (rev = HEAD)
>     Time (mean ± σ):      95.0 ms ±   1.5 ms    [User: 79.8 ms, System: 15.0 ms]
>     Range (min … max):    93.0 ms …  97.9 ms    30 runs
> 
>   Summary
>     git cat-file --batch-check --unordered (rev = HEAD) ran
>       1.89 ± 0.04 times faster than git cat-file --batch-check --unordered (rev = HEAD~)

Nice.

> [1]: <CAFXJcxvpKHoVDwE5mBOd=w-A5vPdUmehqr8SHLUD7qv1qB00rA@mail.gmail.com>
> 
> Reported-by: Wolfgang Kritzinger <wkritzinger@atlassian.com>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  odb/source-files.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/odb/source-files.c b/odb/source-files.c
> index 455f409db0..9268f5aa9a 100644
> --- a/odb/source-files.c
> +++ b/odb/source-files.c
> @@ -426,6 +426,7 @@ static enum odb_read_status odb_source_files_read_object_info(struct odb_source
>  {
>  	struct odb_source_files *files = odb_source_files_downcast(source);
>  	enum odb_read_status status = ODB_READ_NOT_FOUND;
> +	enum odb_read_status ret;
>  
>  	/*
>  	 * Reading an object may fail even though the object exists, for
> @@ -434,15 +435,15 @@ static enum odb_read_status odb_source_files_read_object_info(struct odb_source
>  	 * prefer the first such error in case multiple reads have failed.
>  	 */
>  	for (struct odb_files_dir *dir = files->dirs; dir; dir = dir->next) {
> -		enum odb_read_status ret;
> -
>  		ret = odb_source_read_object_info(&dir->packed->base, oid, oi, flags,
>  						  status == ODB_READ_NOT_FOUND ? errmsg : NULL);
>  		if (!ret)
>  			return 0;
>  		if (ret != ODB_READ_NOT_FOUND && status == ODB_READ_NOT_FOUND)
>  			status = ret;
> +	}
>  
> +	for (struct odb_files_dir *dir = files->dirs; dir; dir = dir->next) {

Ok, now we iterate through each source to look through all the packs
before looking through the loose objects for each source. This matches
the behavior desribed above and look good.

-Justin

Re: [PATCH 2/5] odb/source-files: fix performance when streaming objects with alternates

[Justin Tobler]

On 26/10/13 09:36AM, Patrick Steinhardt wrote:
> This patch is essentially the equivalent to the preceding patch, but for
> streaming objects instead of just reading them directly.
> 
> Exercising performance is a bit more complicated here as streaming only
> really kicks in for large objects. Furthermore, I expect that the impact
> is not as big as with the last commit, either, because for large objects
> the time to stat the file will probably be dwarfed by the time it takes
> to read the object itself.
> 
> But even though this is probably not as impactful, it makes sense anyway
> to do this change to align behaviour across both of these functions.

I agree that this change is probably not as impactful, but unifying the
behavior makes sense to me and doesn't really have any draw backs. I
think this is a good change.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  odb/source-files.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/odb/source-files.c b/odb/source-files.c
> index 9268f5aa9a..54235821d5 100644
> --- a/odb/source-files.c
> +++ b/odb/source-files.c
> @@ -462,8 +462,10 @@ static int odb_source_files_read_object_stream(struct odb_stream **out,
>  	struct odb_source_files *files = odb_source_files_downcast(source);
>  
>  	for (struct odb_files_dir *dir = files->dirs; dir; dir = dir->next)
> -		if (!odb_source_read_object_stream(out, &dir->packed->base, oid) ||
> -		    !odb_source_read_object_stream(out, &dir->loose->base, oid))
> +		if (!odb_source_read_object_stream(out, &dir->packed->base, oid))
> +			return 0;
> +	for (struct odb_files_dir *dir = files->dirs; dir; dir = dir->next)
> +		if (!odb_source_read_object_stream(out, &dir->loose->base, oid))

Just like the last patch, each source looks through its packfiles first
before looking at loose objects. Looks good.

-Justin

Re: [PATCH 3/5] odb/source-files: make the second read an implementation detail

[Justin Tobler]

On 26/10/13 09:36AM, Patrick Steinhardt wrote:
> When looking up an object fails we perform a second read in all sources
> with the `OBJECT_INFO_SECOND_READ` flag set. This flag tells sources
> that the first lookup has failed and that they should reload any on-disk
> state that may have changed in the meantime so that the object may be
> surfaced after all. The "packed" source uses this to reprepare its
> packfiles, whereas the "loose" source short-circuits the second read
> because there are no caches to flush.
> 
> This logic lives in the generic object database layer even though it is
> highly specific to how the "files" backend handles its reads. But now
> that alternates are an implementation detail of that source, we can
> easily move that logic into the backend, as well.
> 
> Move the logic into the "files" source and drop the flag.

Very happy to see this flag go away. It always felt rather leaky to me.
:)

> Note that the "packed" source also used this flag to figure out whether
> it should try to surface an object via a packfile that should have the
> object as determined by a multi-pack index, but where the packfile has
> vanished. The multi-pack index is stale in that case, and that scenario
> should really only happen when the process is racing with a concurrent
> maintenance task.
> 
> This code is adapted to instead use `OBJECT_INFO_QUICK`, which means
> that the fallback may trigger on the first read already, but not in a
> case where callers know to handle false negatives already. In the worst
> case this could cause another scan through all packfiles covered by the
> multi-pack index. But that scan should be fast enough anyway, and it
> only triggers when we hit the race. So the effect thereof should be
> limited.

Would it be more correct to say that we are now using the absence of the
OBJECT_INFO_QUICK flag trigger the fallback?

> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  odb.c               | 15 ---------------
>  odb.h               | 12 ------------
>  odb/source-files.c  | 13 +++++++++++++
>  odb/source-loose.c  | 10 ----------
>  odb/source-packed.c | 15 +++------------
>  odb/source.h        |  7 -------
>  6 files changed, 16 insertions(+), 56 deletions(-)
> 
> diff --git a/odb.c b/odb.c
> index 98c5c862f5..69a4c203bc 100644
> --- a/odb.c
> +++ b/odb.c
> @@ -285,21 +285,6 @@ static enum odb_read_status do_oid_object_info_extended(struct object_database *
>  		if (ret != ODB_READ_NOT_FOUND)
>  			corrupt = true;
>  
> -		/*
> -		 * When the object hasn't been found we try a second read and
> -		 * tell the sources so. This may cause them to invalidate
> -		 * caches or reload on-disk state.
> -		 */
> -		if (!(flags & OBJECT_INFO_QUICK)) {
> -			ret = odb_source_read_object_info(odb->source, real, oi,
> -							  flags | OBJECT_INFO_SECOND_READ,
> -							  corrupt_err.len ? NULL : &corrupt_err);
> -			if (!ret)
> -				goto out;
> -			if (ret != ODB_READ_NOT_FOUND)
> -				corrupt = true;
> -		}

We no longer perform the second read at the ODB level so this code is
no longer needed. This is also the only callsite that sets the flag.

> -
>  		/* Check if it is a missing object */
>  		if (odb->repo->fetch_if_missing && repo_has_promisor_remote(odb->repo) &&
>  		    !already_retried &&
> diff --git a/odb.h b/odb.h
> index 0ccc47ee73..dd85b880a7 100644
> --- a/odb.h
> +++ b/odb.h
> @@ -383,18 +383,6 @@ enum object_info_flags {
>  	/* Die if object corruption (not just an object being missing) was detected. */
>  	OBJECT_INFO_DIE_IF_CORRUPT = (1 << 3),
>  
> -	/*
> -	 * We have already tried reading the object, but it couldn't be found
> -	 * via any of the attached sources, and are now doing a second read.
> -	 * This second read asks the individual sources to also evaluate
> -	 * whether any on-disk state may have changed that may have caused the
> -	 * object to appear.
> -	 *
> -	 * This flag is for internal use, only. The second read only occurs
> -	 * when `OBJECT_INFO_QUICK` was not passed.
> -	 */
> -	OBJECT_INFO_SECOND_READ = (1 << 4),

Flag is removed from the ODB layer. Looks good.

> -
>  	/*
>  	 * This is meant for bulk prefetching of missing blobs in a partial
>  	 * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK.
> diff --git a/odb/source-files.c b/odb/source-files.c
> index 54235821d5..9788a5adf4 100644
> --- a/odb/source-files.c
> +++ b/odb/source-files.c
> @@ -427,7 +427,9 @@ static enum odb_read_status odb_source_files_read_object_info(struct odb_source
>  	struct odb_source_files *files = odb_source_files_downcast(source);
>  	enum odb_read_status status = ODB_READ_NOT_FOUND;
>  	enum odb_read_status ret;
> +	bool retry = false;
>  
> +retry:
>  	/*
>  	 * Reading an object may fail even though the object exists, for
>  	 * example because it is corrupt. Report this failure to the caller in
> @@ -435,6 +437,9 @@ static enum odb_read_status odb_source_files_read_object_info(struct odb_source
>  	 * prefer the first such error in case multiple reads have failed.
>  	 */
>  	for (struct odb_files_dir *dir = files->dirs; dir; dir = dir->next) {
> +		if (retry)
> +			odb_source_prepare(&dir->packed->base, ODB_PREPARE_FLUSH_CACHES);

Ok, for each subsource in the "files" source we can reload the packfiles
to try to find the object. Makes sense.

> +
>  		ret = odb_source_read_object_info(&dir->packed->base, oid, oi, flags,
>  						  status == ODB_READ_NOT_FOUND ? errmsg : NULL);
>  		if (!ret)
> @@ -444,6 +449,9 @@ static enum odb_read_status odb_source_files_read_object_info(struct odb_source
>  	}
>  
>  	for (struct odb_files_dir *dir = files->dirs; dir; dir = dir->next) {
> +		if (retry)
> +			break;

IIUC, we don't need to bother checking the loose objects a second time
so we exit the look early.

Nit: Mostly just a style preference, but instead of entering the loop
just to break out early for retries, could we just return early prior
the loop starting? It looks like the rest of the function is skipped on
the second read anyways.

> +
>  		ret = odb_source_read_object_info(&dir->loose->base, oid, oi, flags,
>  						  status == ODB_READ_NOT_FOUND ? errmsg : NULL);
>  		if (!ret)
> @@ -452,6 +460,11 @@ static enum odb_read_status odb_source_files_read_object_info(struct odb_source
>  			status = ret;
>  	}
>  
> +	if (!retry && !(flags & OBJECT_INFO_QUICK)) {
> +		retry = true;
> +		goto retry;
> +	}
> +
>  	return status;
>  }
>  
> diff --git a/odb/source-loose.c b/odb/source-loose.c
> index 0664634938..180ce7403f 100644
> --- a/odb/source-loose.c
> +++ b/odb/source-loose.c
> @@ -222,16 +222,6 @@ static enum odb_read_status odb_source_loose_read_object_info(struct odb_source
>  {
>  	struct odb_source_loose *loose = odb_source_loose_downcast(source);
>  	static struct strbuf buf = STRBUF_INIT;
> -
> -	/*
> -	 * The second read shouldn't cause new loose objects to show up, unless
> -	 * there was a race condition with a secondary process. We don't care
> -	 * about this case though, so we simply skip reading loose objects a
> -	 * second time.
> -	 */
> -	if (flags & OBJECT_INFO_SECOND_READ)
> -		return ODB_READ_NOT_FOUND;
> -
>  	odb_loose_path(loose, &buf, oid);
>  	return read_object_info_from_path(loose, buf.buf, oid, oi, flags, errmsg);
>  }
> diff --git a/odb/source-packed.c b/odb/source-packed.c
> index 2226fbd313..48fe00c2bc 100644
> --- a/odb/source-packed.c
> +++ b/odb/source-packed.c
> @@ -52,12 +52,11 @@ static int find_pack_entry(struct odb_source_packed *store,
>  	 * packs, and repreparing the on-disk pack set does not reload the
>  	 * borrowed, cached MIDX, so scan its packs directly for the survivor.
>  	 *
> -	 * Do this only on the second read, by which point repreparing packs has
> -	 * already had a chance to find an object merely relocated into a new,
> -	 * uncovered pack; only a genuine hidden duplicate reaches here.
> +	 * Skip the scan for quick lookups, which are expected to accept false
> +	 * negatives in favor of speed.
>  	 */
>  	if (midx_result == MIDX_FILL_OWNER_UNAVAILABLE &&
> -	    (flags & OBJECT_INFO_SECOND_READ)) {
> +	    !(flags & OBJECT_INFO_QUICK)) {

Ok, instead of performing the fallback when the OBJECT_INFO_SECOND_READ
is provided, we perform it as long as OBJECT_INFO_QUICK isn't. This
likely means that the fallback would happen more frequently, but since
we are already guarded on MIDX_FILL_OWNER_UNAVAILABLE, I'm not sure that
would matter much in practice.

Overall this patch looks good to me.

-Justin

Re: [PATCH 4/5] odb: remove read loop for found objects

[Justin Tobler]

On 26/10/13 09:36AM, Patrick Steinhardt wrote:
> Before moving alternates into the "files" backend we had to handle
> alternates in `do_oid_object_info_extended()`. This made the retry logic
> somewhat complex, as we had to juggle reloading of the sources, the
> second read and fetching missing objects. And to handle the potential
> retries we loop around that logic multiple times.
> 
> Nowadays though, the reload and retry logic are located in the "files"
> backend itself. Consequently, using a loop is a bit on the heavy side
> and makes the code harder to reason about.

The "files" backend is now responsible for retrying a read if it need to
reload its packfiles. IIUC, this means that we should only expect
missing objects from partial clones. Only if we want to fetch these
missing objects from the promisor remote do we need to retry.

> Simplify the logic by removing the loop. Instead, we explicitly re-read
> exactly once after having fetched objects from a promisor remote.

Make sense.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  odb.c | 46 +++++++++++++++++++++++-----------------------
>  1 file changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/odb.c b/odb.c
> index 69a4c203bc..095183aec0 100644
> --- a/odb.c
> +++ b/odb.c
> @@ -265,7 +265,6 @@ static enum odb_read_status do_oid_object_info_extended(struct object_database *
>  	struct strbuf corrupt_err = STRBUF_INIT;
>  	const struct object_id *real = oid;
>  	enum odb_read_status ret;
> -	int already_retried = 0;
>  	bool corrupt = false;
>  
>  	if (flags & OBJECT_INFO_LOOKUP_REPLACE)
> @@ -277,38 +276,39 @@ static enum odb_read_status do_oid_object_info_extended(struct object_database *
>  	if (!odb_source_read_object_info(odb->inmemory_objects, oid, oi, flags, NULL))
>  		return 0;
>  
> -	while (1) {
> +	ret = odb_source_read_object_info(odb->source, real, oi, flags,
> +					  &corrupt_err);

Here is try to read the object, the backend is responsible for handling
any retries internally that it cares about.

> +	if (!ret)
> +		goto out;
> +	if (ret != ODB_READ_NOT_FOUND)
> +		corrupt = true;

If the repsitory is correct, does it even make sense to continue to try
and fetch the missing object? I wonder if we should set `ret =
ODB_READ_ERROR` and `goto out`?

> +
> +	/* Try to fetch the missing object from a promisor remote. */
> +	if (odb->repo->fetch_if_missing && repo_has_promisor_remote(odb->repo) &&
> +	    !(flags & OBJECT_INFO_SKIP_FETCH_OBJECT)) {
> +		promisor_remote_get_direct(odb->repo, real, 1);
> +
>  		ret = odb_source_read_object_info(odb->source, real, oi, flags,
>  						  corrupt_err.len ? NULL : &corrupt_err);

Here we read the missing object after it has been fetched. This is only
case the ODB layer needs to care about retrying itself. Makes sense.

The rest of this patch is functionally the same just without the loop.
Looks good.

-Justin