[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

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

[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

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

[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

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 3/5] odb/source-files: make the second read an implementation detail

[Patrick Steinhardt]

On Tue, Oct 13, 2026 at 04:28:37PM -0500, Justin Tobler wrote:
> On 26/10/13 09:36AM, Patrick Steinhardt wrote:
> > 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?

I dunno. Whether we check for absence or presence, in either case we use
the flag. So I'm inclined to leave this as-is as it starts to read a bit
awkward otherwise.

Let me know in case you feel strongly though.

> > 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
> > @@ -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.

Yes. But I've got a follow-up patch series that has some fixes for how
we reload alternates, and as part of that I'll start to iterate over
loose object directories that have been added after re-reading
".git/objects/info/alternates". So this is a bit awkward right now, but
makes the follow-up series a bit easier.

Patrick

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

[Justin Tobler]

On 26/10/14 08:55AM, Patrick Steinhardt wrote:
> On Tue, Oct 13, 2026 at 04:28:37PM -0500, Justin Tobler wrote:
> > On 26/10/13 09:36AM, Patrick Steinhardt wrote:
> > > 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?
> 
> I dunno. Whether we check for absence or presence, in either case we use
> the flag. So I'm inclined to leave this as-is as it starts to read a bit
> awkward otherwise.
> 
> Let me know in case you feel strongly though.

I don't feel too strongly either way. When I read it the first time, my
first thought was that we were using the OBJECT_INFO_QUICK flag to mean
the same thing as OBJECT_INFO_SECOND_READ which confused me. Then I
realized it was in fact the exact opposite.

> > > 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
> > > @@ -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.
> 
> Yes. But I've got a follow-up patch series that has some fixes for how
> we reload alternates, and as part of that I'll start to iterate over
> loose object directories that have been added after re-reading
> ".git/objects/info/alternates". So this is a bit awkward right now, but
> makes the follow-up series a bit easier.

Ok, this sounds reasonable to me.

-Justin

[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

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

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

[Patrick Steinhardt]

On Tue, Oct 13, 2026 at 04:48:23PM -0500, Justin Tobler wrote:
> On 26/10/13 09:36AM, Patrick Steinhardt wrote:
> > 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`?

I think I tend to agree. I don't want to do it as part of this commit
though, which is a plain refactoring that shouldn't change semantics.
I'll add a commit on top though.

Patrick

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

[Justin Tobler]

On 26/10/14 08:56AM, Patrick Steinhardt wrote:
> On Tue, Oct 13, 2026 at 04:48:23PM -0500, Justin Tobler wrote:
> > On 26/10/13 09:36AM, Patrick Steinhardt wrote:
> > > 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`?
> 
> I think I tend to agree. I don't want to do it as part of this commit
> though, which is a plain refactoring that shouldn't change semantics.
> I'll add a commit on top though.

Ok, sounds good to me!

-Justin

[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 5/5] odb/source: drop `local` flag

[Justin Tobler]

On 26/10/13 09:36AM, Patrick Steinhardt wrote:
> 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.

I do wish we could also drop ODB_FOR_EACH_OBJECT_LOCAL_ONLY. This flag
also feels rather leaky to me. I guess though we still need a way to
ignore objects from certain sources when iterating over all sources such
as the inmemory source.

Overall, I happy to see the `local` field dropped from the generic ODB
source structure, but IMO is does make each `for_each_object` callback
handling of the ODB_FOR_EACH_OBJECT_LOCAL_ONLY flag a bit ambiguous and
awkward.

Naive question: Do all odb_for_each_object() call sites that use the
ODB_FOR_EACH_OBJECT_LOCAL_ONLY flag have "files" backend specific
reasons to do so?

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
[snip]
> 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))

So for the inmemory source, we don't want to iterate over the objects at
all if we are only interested in "local" objects.

Naive question: Are there any scenarios where we even would iterate over
an inmemory source and only be interested in "local" objects?

>  		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;

Ok, the "loose" backend callback already doesn't need to care about the
ODB_FOR_EACH_OBJECT_LOCAL_ONLY flag because it is handled at the "files"
layer.

>  	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;
[snip]
> 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;

Here the local flag is moved to the "packed" backend...

> +
>  	/*
>  	 * 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
[snip]
> 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;

...and here the local flag is dropped from the generic source layer.

The patch overall looks reasonable and seems to be moving us in the
right direction.

-Justin

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

[Patrick Steinhardt]

On Tue, Oct 13, 2026 at 05:44:11PM -0500, Justin Tobler wrote:
> On 26/10/13 09:36AM, Patrick Steinhardt wrote:
> > 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.
> 
> I do wish we could also drop ODB_FOR_EACH_OBJECT_LOCAL_ONLY. This flag
> also feels rather leaky to me. I guess though we still need a way to
> ignore objects from certain sources when iterating over all sources such
> as the inmemory source.
> 
> Overall, I happy to see the `local` field dropped from the generic ODB
> source structure, but IMO is does make each `for_each_object` callback
> handling of the ODB_FOR_EACH_OBJECT_LOCAL_ONLY flag a bit ambiguous and
> awkward.
> 
> Naive question: Do all odb_for_each_object() call sites that use the
> ODB_FOR_EACH_OBJECT_LOCAL_ONLY flag have "files" backend specific
> reasons to do so?

It does make it awkward, I agree. There's only two callers that use the
flag right now:

  - git-pack-objects(1) with its "--local" flag. I consider this command
    to be too specific to the "files" backend anyway though, so arguably
    we could just let it handle this directly.

  - "reachable.c" via `add_unseen_recent_objects_to_traversal()`. This
    is invoke by git-prune(1) to mark reachable objects that are recent,
    and it's dubious that we only enumerate local objects here.

So we might be able to get rid of that flag altogether. But I'm inclined
to go one step at a time and leave this for a future patch series.

> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> [snip]
> > 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))
> 
> So for the inmemory source, we don't want to iterate over the objects at
> all if we are only interested in "local" objects.
> 
> Naive question: Are there any scenarios where we even would iterate over
> an inmemory source and only be interested in "local" objects?

Not that I'd know of.

Patrick

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

[Justin Tobler]

On 26/10/14 08:56AM, Patrick Steinhardt wrote:
> On Tue, Oct 13, 2026 at 05:44:11PM -0500, Justin Tobler wrote:
> > On 26/10/13 09:36AM, Patrick Steinhardt wrote:
> > > 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.
> > 
> > I do wish we could also drop ODB_FOR_EACH_OBJECT_LOCAL_ONLY. This flag
> > also feels rather leaky to me. I guess though we still need a way to
> > ignore objects from certain sources when iterating over all sources such
> > as the inmemory source.
> > 
> > Overall, I happy to see the `local` field dropped from the generic ODB
> > source structure, but IMO is does make each `for_each_object` callback
> > handling of the ODB_FOR_EACH_OBJECT_LOCAL_ONLY flag a bit ambiguous and
> > awkward.
> > 
> > Naive question: Do all odb_for_each_object() call sites that use the
> > ODB_FOR_EACH_OBJECT_LOCAL_ONLY flag have "files" backend specific
> > reasons to do so?
> 
> It does make it awkward, I agree. There's only two callers that use the
> flag right now:
> 
>   - git-pack-objects(1) with its "--local" flag. I consider this command
>     to be too specific to the "files" backend anyway though, so arguably
>     we could just let it handle this directly.
> 
>   - "reachable.c" via `add_unseen_recent_objects_to_traversal()`. This
>     is invoke by git-prune(1) to mark reachable objects that are recent,
>     and it's dubious that we only enumerate local objects here.
> 
> So we might be able to get rid of that flag altogether. But I'm inclined
> to go one step at a time and leave this for a future patch series.

That's fair. I don't think we need to get rid of this flag as part of
this series, but it seem like it would be something nice to eventually
remove.

Do we want to leave a TODO comment to mention this?

> > > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > > ---
> > [snip]
> > > 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))
> > 
> > So for the inmemory source, we don't want to iterate over the objects at
> > all if we are only interested in "local" objects.
> > 
> > Naive question: Are there any scenarios where we even would iterate over
> > an inmemory source and only be interested in "local" objects?
> 
> Not that I'd know of.

That's what I figured. To me, this is just another reason to remove the
flag. :)

-Justin

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

[Patrick Steinhardt]

On Wed, Oct 14, 2026 at 10:43:23AM -0500, Justin Tobler wrote:
> On 26/10/14 08:56AM, Patrick Steinhardt wrote:
> > On Tue, Oct 13, 2026 at 05:44:11PM -0500, Justin Tobler wrote:
> > > On 26/10/13 09:36AM, Patrick Steinhardt wrote:
> > > > 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.
> > > 
> > > I do wish we could also drop ODB_FOR_EACH_OBJECT_LOCAL_ONLY. This flag
> > > also feels rather leaky to me. I guess though we still need a way to
> > > ignore objects from certain sources when iterating over all sources such
> > > as the inmemory source.
> > > 
> > > Overall, I happy to see the `local` field dropped from the generic ODB
> > > source structure, but IMO is does make each `for_each_object` callback
> > > handling of the ODB_FOR_EACH_OBJECT_LOCAL_ONLY flag a bit ambiguous and
> > > awkward.
> > > 
> > > Naive question: Do all odb_for_each_object() call sites that use the
> > > ODB_FOR_EACH_OBJECT_LOCAL_ONLY flag have "files" backend specific
> > > reasons to do so?
> > 
> > It does make it awkward, I agree. There's only two callers that use the
> > flag right now:
> > 
> >   - git-pack-objects(1) with its "--local" flag. I consider this command
> >     to be too specific to the "files" backend anyway though, so arguably
> >     we could just let it handle this directly.
> > 
> >   - "reachable.c" via `add_unseen_recent_objects_to_traversal()`. This
> >     is invoke by git-prune(1) to mark reachable objects that are recent,
> >     and it's dubious that we only enumerate local objects here.
> > 
> > So we might be able to get rid of that flag altogether. But I'm inclined
> > to go one step at a time and leave this for a future patch series.
> 
> That's fair. I don't think we need to get rid of this flag as part of
> this series, but it seem like it would be something nice to eventually
> remove.
> 
> Do we want to leave a TODO comment to mention this?

I think I'd rather keep it in the hive mind. It's a bit more intricate
to get rid of this, and TODO items often invit rather mindless
approaches to fixes.

Patrick

[PATCH v2 0/6] 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.

Changes in v2:
  - Add another refactoring on top to only fetch objects from a promisor
    remote in case they are missing.
  - Link to v1: https://patch.msgid.link/20261013-pks-odb-files-alternates-cleanups-v1-0-b7030ff2b26f@pks.im

Thanks!

Patrick

---
Patrick Steinhardt (6):
      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: don't try re-fetching corrupt objects
      odb/source: drop `local` flag

 midx.c                |  2 +-
 odb.c                 | 63 +++++++++++++++------------------------------------
 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, 66 insertions(+), 120 deletions(-)

Range-diff versus v1:

1:  c576112f90 = 1:  bd071672be odb/source-files: fix performance when reading objects with alternates
2:  f4065cfc43 = 2:  3092e588e9 odb/source-files: fix performance when streaming objects with alternates
3:  7d10934490 = 3:  2a347fcc0c odb/source-files: make the second read an implementation detail
4:  e4b41bd118 = 4:  b5080890d7 odb: remove read loop for found objects
-:  ---------- > 5:  fd362349fa odb: don't try re-fetching corrupt objects
5:  bafbc979e9 = 6:  8384fcc65b odb/source: drop `local` flag

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

[PATCH v2 1/6] 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

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 8247 bytes --]

Hi Patrick,

On Wed, 14 Oct 2026, 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.
> 
> 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>

Thank you so much for this! This fixes the bug we had to patch
independently in dc305fc33833 (odb: scan all sources’ packfiles before
loose objects), via https://github.com/microsoft/git/pull/975, first
contained in Microsoft Git v2.55.0.vfs.0.7. Would you kindly include (my
colleague) Tyrie Vella's regression test in the next iteration of this
patch series?

-- snip --
From: Tyrie Vella <tyrielv@gmail.com>
Date: Wed, 14 Oct 2026 15:01:45 +0200
Subject: [PATCH] t5615: verify packed alternates precede local loose objects

This regression tests protects the packed-first lookup order required for
decent performance in setups where the majority of Git objects lives in a
separate alternate's packfiles (such as is the case with typical Scalar or
VFS for Git setups).

The test was originally added to Microsoft Git via
https://github.com/microsoft/git/pull/975

Assisted-by: GPT-6.1 Sol
Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t5615-alternate-env.sh | 43 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/t/t5615-alternate-env.sh b/t/t5615-alternate-env.sh
index 9d6aa2187f2..c982e8e95d2 100755
--- a/t/t5615-alternate-env.sh
+++ b/t/t5615-alternate-env.sh
@@ -88,4 +88,47 @@ test_expect_success !MINGW 'broken quoting falls back to interpreting raw' '
 	EOF
 '
 
+test_expect_success 'packs across sources are checked before loose objects' '
+	# Regression test for a performance issue in which an object that
+	# resides in an alternate as a packed object caused a spurious loose
+	# object lookup (a filesystem stat) on the main object store before the
+	# alternate packfile was consulted. Reading such an object must resolve
+	# to the alternate packfile, never to a loose copy in the main store.
+	#
+	# Build an alternate whose object "B" is stored as a delta in a
+	# packfile. git deltifies successive versions of a tracked file, so the
+	# older, shorter blob "B" becomes a delta against the newer, longer
+	# blob "O". A loose object has no delta base, so %(deltabase) tells us
+	# which store answered the read: the alternate pack (O) or a loose copy
+	# (the zero oid).
+	git init alt-src &&
+	test_seq 1 200 >alt-src/file &&
+	git -C alt-src add file &&
+	git -C alt-src commit -q -m base &&
+	B=$(git -C alt-src rev-parse HEAD:file) &&
+	git -C alt-src cat-file blob "$B" >b-content &&
+	test_seq 1 210 >alt-src/file &&
+	git -C alt-src add file &&
+	git -C alt-src commit -q -m more &&
+	O=$(git -C alt-src rev-parse HEAD:file) &&
+	git -C alt-src repack -adf --window=10 --depth=50 &&
+
+	# Precondition: in the alternate, B is a delta based on O.
+	echo "$B" >in &&
+	echo "$O" >expect &&
+	git -C alt-src cat-file --batch-check="%(deltabase)" <in >actual &&
+	test_cmp expect actual &&
+
+	# Main repo: write B as a loose object, before any alternate is active.
+	git init main &&
+	git -C main hash-object -w --stdin <b-content >/dev/null &&
+	test -e "main/.git/objects/$(test_oid_to_path "$B")" &&
+
+	# With the alternate active, B must resolve to the alternate packfile
+	# (deltabase O), not to the main store loose copy (deltabase zero oid).
+	GIT_ALTERNATE_OBJECT_DIRECTORIES="$PWD/alt-src/.git/objects" \
+		git -C main cat-file --batch-check="%(deltabase)" <in >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- snap --

I ran the test on Linux with both SHA-1 and SHA-256. In both formats, the
unfixed base passes the nine existing tests; With vanilla v2.56.0 the
added case fails at its final lookup-order assertion: `%(deltabase)`
returns the zero OID despite the packed-delta precondition passing. Patch
1 (and of course the full patch series) pass the tests.

Ciao,
Johannes

> ---
>  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)
> 
> -- 

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

[Patrick Steinhardt]

On Wed, Oct 14, 2026 at 03:35:56PM +0200, Johannes Schindelin wrote:
> Hi Patrick,
> 
> On Wed, 14 Oct 2026, 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.
> > 
> > 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>
> 
> Thank you so much for this! This fixes the bug we had to patch
> independently in dc305fc33833 (odb: scan all sources’ packfiles before
> loose objects), via https://github.com/microsoft/git/pull/975, first
> contained in Microsoft Git v2.55.0.vfs.0.7. Would you kindly include (my
> colleague) Tyrie Vella's regression test in the next iteration of this
> patch series?

Yeah, makes sense. I'll fold the test into this patch with a Helped-by
trailer. Thanks!

Patrick

[PATCH v2 2/6] 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 v2 3/6] 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 v2 4/6] 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 v2 5/6] odb: don't try re-fetching corrupt objects

[Patrick Steinhardt]

When we failed reading an object in a repository with a promisor remote
we try to fetch it from the promisor and then re-read it. This logic
kicks in not only when the object couldn't be found, but also in the
case where it could be found but the object is corrupt. It feels dubious
though to just paper over corruption and proceed as if nothing has
happened, happily fetching data from the promisor.

Adapt the code so that we only fetch objects in case they are missing.

Suggested-by: Justin Tobler <jltobler@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 odb.c | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/odb.c b/odb.c
index 095183aec0..2c93a9490c 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;
-	bool corrupt = false;
 
 	if (flags & OBJECT_INFO_LOOKUP_REPLACE)
 		real = lookup_replace_object(odb->repo, oid);
@@ -278,38 +277,27 @@ static enum odb_read_status do_oid_object_info_extended(struct object_database *
 
 	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)) {
+	if (ret == ODB_READ_NOT_FOUND &&
+	    odb->repo->fetch_if_missing &&
+	    !(flags & OBJECT_INFO_SKIP_FETCH_OBJECT) &&
+	    repo_has_promisor_remote(odb->repo)) {
+		/* Try to fetch the missing object from a promisor remote. */
 		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;
+						  &corrupt_err);
 	}
 
-	if (flags & OBJECT_INFO_DIE_IF_CORRUPT) {
+	if (ret < 0 && (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 (ret != ODB_READ_NOT_FOUND) {
 			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;
-
-out:
 	strbuf_release(&corrupt_err);
 	return ret;
 }

-- 
2.56.0.212.g129ede20d9.dirty

Re: [PATCH v2 5/6] odb: don't try re-fetching corrupt objects

[Justin Tobler]

On 26/10/14 10:53AM, Patrick Steinhardt wrote:
> When we failed reading an object in a repository with a promisor remote
> we try to fetch it from the promisor and then re-read it. This logic
> kicks in not only when the object couldn't be found, but also in the
> case where it could be found but the object is corrupt. It feels dubious
> though to just paper over corruption and proceed as if nothing has
> happened, happily fetching data from the promisor.

Agreed. If we know the object is corrupt, it makes sense to stop early
and report the finding.

> Adapt the code so that we only fetch objects in case they are missing.
> 
> Suggested-by: Justin Tobler <jltobler@gmail.com>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  odb.c | 28 ++++++++--------------------
>  1 file changed, 8 insertions(+), 20 deletions(-)
> 
> diff --git a/odb.c b/odb.c
> index 095183aec0..2c93a9490c 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;
> -	bool corrupt = false;
>  
>  	if (flags & OBJECT_INFO_LOOKUP_REPLACE)
>  		real = lookup_replace_object(odb->repo, oid);
> @@ -278,38 +277,27 @@ static enum odb_read_status do_oid_object_info_extended(struct object_database *
>  
>  	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)) {
> +	if (ret == ODB_READ_NOT_FOUND &&
> +	    odb->repo->fetch_if_missing &&
> +	    !(flags & OBJECT_INFO_SKIP_FETCH_OBJECT) &&
> +	    repo_has_promisor_remote(odb->repo)) {

Ok, now we only try to fetch a missing object specifically if it was
ODB_READ_NOT_FOUND in the first lookup. This means we now skip this
logic if the object is corrupt. This makes more sense to me and looks
good.

> +		/* Try to fetch the missing object from a promisor remote. */
>  		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;
> +						  &corrupt_err);
>  	}
>  
> -	if (flags & OBJECT_INFO_DIE_IF_CORRUPT) {
> +	if (ret < 0 && (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 (ret != ODB_READ_NOT_FOUND) {
>  			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;
> -
> -out:

The remainder of this patch is functionally identical and looks good.

-Justin

[PATCH v2 6/6] 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

[PATCH v3 0/6] 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.

Changes in v2:
  - Add another refactoring on top to only fetch objects from a promisor
    remote in case they are missing.
  - Link to v1: https://patch.msgid.link/20261013-pks-odb-files-alternates-cleanups-v1-0-b7030ff2b26f@pks.im

Changes in v3:
  - Add a regression test, supplied by Tyrie.
  - Link to v2: https://patch.msgid.link/20261014-pks-odb-files-alternates-cleanups-v2-0-fed9b2caa3be@pks.im

Thanks!

Patrick

---
Patrick Steinhardt (6):
      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: don't try re-fetching corrupt objects
      odb/source: drop `local` flag

 midx.c                   |  2 +-
 odb.c                    | 63 ++++++++++++++----------------------------------
 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 +-
 t/t5615-alternate-env.sh | 43 +++++++++++++++++++++++++++++++++
 13 files changed, 109 insertions(+), 120 deletions(-)

Range-diff versus v2:

1:  3a7f78fc94 ! 1:  dea38443a4 odb/source-files: fix performance when reading objects with alternates
    @@ Commit message
         [1]: <CAFXJcxvpKHoVDwE5mBOd=w-A5vPdUmehqr8SHLUD7qv1qB00rA@mail.gmail.com>
     
         Reported-by: Wolfgang Kritzinger <wkritzinger@atlassian.com>
    +    Helped-by: Tyrie Vella <tyrielv@gmail.com>
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## odb/source-files.c ##
    @@ odb/source-files.c: static enum odb_read_status odb_source_files_read_object_inf
      		ret = odb_source_read_object_info(&dir->loose->base, oid, oi, flags,
      						  status == ODB_READ_NOT_FOUND ? errmsg : NULL);
      		if (!ret)
    +
    + ## t/t5615-alternate-env.sh ##
    +@@ t/t5615-alternate-env.sh: test_expect_success !MINGW 'broken quoting falls back to interpreting raw' '
    + 	EOF
    + '
    + 
    ++test_expect_success 'packs across sources are checked before loose objects' '
    ++	# Regression test for a performance issue in which an object that
    ++	# resides in an alternate as a packed object caused a spurious loose
    ++	# object lookup (a filesystem stat) on the main object store before the
    ++	# alternate packfile was consulted. Reading such an object must resolve
    ++	# to the alternate packfile, never to a loose copy in the main store.
    ++	#
    ++	# Build an alternate whose object "B" is stored as a delta in a
    ++	# packfile. git deltifies successive versions of a tracked file, so the
    ++	# older, shorter blob "B" becomes a delta against the newer, longer
    ++	# blob "O". A loose object has no delta base, so %(deltabase) tells us
    ++	# which store answered the read: the alternate pack (O) or a loose copy
    ++	# (the zero oid).
    ++	git init alt-src &&
    ++	test_seq 1 200 >alt-src/file &&
    ++	git -C alt-src add file &&
    ++	git -C alt-src commit -q -m base &&
    ++	B=$(git -C alt-src rev-parse HEAD:file) &&
    ++	git -C alt-src cat-file blob "$B" >b-content &&
    ++	test_seq 1 210 >alt-src/file &&
    ++	git -C alt-src add file &&
    ++	git -C alt-src commit -q -m more &&
    ++	O=$(git -C alt-src rev-parse HEAD:file) &&
    ++	git -C alt-src repack -adf --window=10 --depth=50 &&
    ++
    ++	# Precondition: in the alternate, B is a delta based on O.
    ++	echo "$B" >in &&
    ++	echo "$O" >expect &&
    ++	git -C alt-src cat-file --batch-check="%(deltabase)" <in >actual &&
    ++	test_cmp expect actual &&
    ++
    ++	# Main repo: write B as a loose object, before any alternate is active.
    ++	git init main &&
    ++	git -C main hash-object -w --stdin <b-content >/dev/null &&
    ++	test_path_is_file "main/.git/objects/$(test_oid_to_path "$B")" &&
    ++
    ++	# With the alternate active, B must resolve to the alternate packfile
    ++	# (deltabase O), not to the main store loose copy (deltabase zero oid).
    ++	GIT_ALTERNATE_OBJECT_DIRECTORIES="$PWD/alt-src/.git/objects" \
    ++		git -C main cat-file --batch-check="%(deltabase)" <in >actual &&
    ++	test_cmp expect actual
    ++'
    ++
    + test_done
2:  23d8dcfcdf = 2:  374cbda799 odb/source-files: fix performance when streaming objects with alternates
3:  406228ce0f = 3:  635d9ce86e odb/source-files: make the second read an implementation detail
4:  c8f4c96b77 = 4:  832852bad5 odb: remove read loop for found objects
5:  e4af3300af = 5:  a0eebb1225 odb: don't try re-fetching corrupt objects
6:  7658903430 = 6:  b4a76c5651 odb/source: drop `local` flag

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

[PATCH v3 1/6] 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>
Helped-by: Tyrie Vella <tyrielv@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 odb/source-files.c       |  5 +++--
 t/t5615-alternate-env.sh | 43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 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)
diff --git a/t/t5615-alternate-env.sh b/t/t5615-alternate-env.sh
index 9d6aa2187f..3259852a0f 100755
--- a/t/t5615-alternate-env.sh
+++ b/t/t5615-alternate-env.sh
@@ -88,4 +88,47 @@ test_expect_success !MINGW 'broken quoting falls back to interpreting raw' '
 	EOF
 '
 
+test_expect_success 'packs across sources are checked before loose objects' '
+	# Regression test for a performance issue in which an object that
+	# resides in an alternate as a packed object caused a spurious loose
+	# object lookup (a filesystem stat) on the main object store before the
+	# alternate packfile was consulted. Reading such an object must resolve
+	# to the alternate packfile, never to a loose copy in the main store.
+	#
+	# Build an alternate whose object "B" is stored as a delta in a
+	# packfile. git deltifies successive versions of a tracked file, so the
+	# older, shorter blob "B" becomes a delta against the newer, longer
+	# blob "O". A loose object has no delta base, so %(deltabase) tells us
+	# which store answered the read: the alternate pack (O) or a loose copy
+	# (the zero oid).
+	git init alt-src &&
+	test_seq 1 200 >alt-src/file &&
+	git -C alt-src add file &&
+	git -C alt-src commit -q -m base &&
+	B=$(git -C alt-src rev-parse HEAD:file) &&
+	git -C alt-src cat-file blob "$B" >b-content &&
+	test_seq 1 210 >alt-src/file &&
+	git -C alt-src add file &&
+	git -C alt-src commit -q -m more &&
+	O=$(git -C alt-src rev-parse HEAD:file) &&
+	git -C alt-src repack -adf --window=10 --depth=50 &&
+
+	# Precondition: in the alternate, B is a delta based on O.
+	echo "$B" >in &&
+	echo "$O" >expect &&
+	git -C alt-src cat-file --batch-check="%(deltabase)" <in >actual &&
+	test_cmp expect actual &&
+
+	# Main repo: write B as a loose object, before any alternate is active.
+	git init main &&
+	git -C main hash-object -w --stdin <b-content >/dev/null &&
+	test_path_is_file "main/.git/objects/$(test_oid_to_path "$B")" &&
+
+	# With the alternate active, B must resolve to the alternate packfile
+	# (deltabase O), not to the main store loose copy (deltabase zero oid).
+	GIT_ALTERNATE_OBJECT_DIRECTORIES="$PWD/alt-src/.git/objects" \
+		git -C main cat-file --batch-check="%(deltabase)" <in >actual &&
+	test_cmp expect actual
+'
+
 test_done

-- 
2.56.0.212.g129ede20d9.dirty

[PATCH v3 2/6] 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 v3 3/6] 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 v3 4/6] 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 v3 5/6] odb: don't try re-fetching corrupt objects

[Patrick Steinhardt]

When we failed reading an object in a repository with a promisor remote
we try to fetch it from the promisor and then re-read it. This logic
kicks in not only when the object couldn't be found, but also in the
case where it could be found but the object is corrupt. It feels dubious
though to just paper over corruption and proceed as if nothing has
happened, happily fetching data from the promisor.

Adapt the code so that we only fetch objects in case they are missing.

Suggested-by: Justin Tobler <jltobler@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 odb.c | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/odb.c b/odb.c
index 095183aec0..2c93a9490c 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;
-	bool corrupt = false;
 
 	if (flags & OBJECT_INFO_LOOKUP_REPLACE)
 		real = lookup_replace_object(odb->repo, oid);
@@ -278,38 +277,27 @@ static enum odb_read_status do_oid_object_info_extended(struct object_database *
 
 	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)) {
+	if (ret == ODB_READ_NOT_FOUND &&
+	    odb->repo->fetch_if_missing &&
+	    !(flags & OBJECT_INFO_SKIP_FETCH_OBJECT) &&
+	    repo_has_promisor_remote(odb->repo)) {
+		/* Try to fetch the missing object from a promisor remote. */
 		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;
+						  &corrupt_err);
 	}
 
-	if (flags & OBJECT_INFO_DIE_IF_CORRUPT) {
+	if (ret < 0 && (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 (ret != ODB_READ_NOT_FOUND) {
 			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;
-
-out:
 	strbuf_release(&corrupt_err);
 	return ret;
 }

-- 
2.56.0.212.g129ede20d9.dirty

[PATCH v3 6/6] 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 v3 0/6] odb/files: cleanups and improvements for alternates

[Justin Tobler]

On 26/10/15 07:27AM, Patrick Steinhardt wrote:
> Changes in v3:
>   - Add a regression test, supplied by Tyrie.
>   - Link to v2: https://patch.msgid.link/20261014-pks-odb-files-alternates-cleanups-v2-0-fed9b2caa3be@pks.im

Thanks. This version of the series looks ready to me.

-Justin