* [PATCH 0/2] odb/source-loose: fix failing existence checks caused by stale cache
@ 2026-11-04 12:16 Patrick Steinhardt
2026-11-04 12:16 ` [PATCH 1/2] odb/source-loose: extract logic to retrieve subdirectory bitmap Patrick Steinhardt
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Patrick Steinhardt @ 2026-11-04 12:16 UTC (permalink / raw)
To: git; +Cc: Patrick Ohly
Hi,
this small patch series fixes the bug reported in [1].
In summary, when git-merge(1) writes merged blobs it may be unable to
read them back because the loose object cache does not get updated.
Consequently, we may think that the blobs do not exist locally, and thus
in a promisor remote we may perform a backfill fetch. But as the remote
doesn't know those objects we'll fail.
Thanks!
Patrick
[1]: yrjh1p92qc7w.fsf@pohly-mobl1.fritz.box
---
Patrick Steinhardt (2):
odb/source-loose: extract logic to retrieve subdirectory bitmap
odb/source-loose: fix existence checks for newly written objects
odb/source-loose.c | 54 +++++++++++++++++++++++++++++++++++++++---------
t/t5616-partial-clone.sh | 43 ++++++++++++++++++++++++++++++++++++++
2 files changed, 87 insertions(+), 10 deletions(-)
---
base-commit: 750f5de7dfe9a4062f95088212b063f189d50e91
change-id: 20261104-pks-odb-loose-stale-cache-c16107987290
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/2] odb/source-loose: extract logic to retrieve subdirectory bitmap 2026-11-04 12:16 [PATCH 0/2] odb/source-loose: fix failing existence checks caused by stale cache Patrick Steinhardt @ 2026-11-04 12:16 ` Patrick Steinhardt 2026-11-04 12:16 ` [PATCH 2/2] odb/source-loose: fix existence checks for newly written objects Patrick Steinhardt 2026-11-04 12:57 ` [PATCH 0/2] odb/source-loose: fix failing existence checks caused by stale cache Kristofer Karlsson 2 siblings, 0 replies; 9+ messages in thread From: Patrick Steinhardt @ 2026-11-04 12:16 UTC (permalink / raw) To: git; +Cc: Patrick Ohly The loose object cache tracks which sharding directories have already been scanned via a bitmap. The logic to retrieve the bitmap and mask for a given object ID is inlined into the function that populates the cache, but we're about to add a second function that will need to perform the same logic. Extract the logic into a separate function to prepare for this. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- odb/source-loose.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/odb/source-loose.c b/odb/source-loose.c index 50cb388c352..da8f9767257 100644 --- a/odb/source-loose.c +++ b/odb/source-loose.c @@ -26,29 +26,39 @@ static int append_loose_object(const struct object_id *oid, return 0; } -static struct oidtree *odb_source_loose_cache(struct odb_source_loose *loose, - const struct object_id *oid) +static uint32_t *subdir_seen_bitmap(struct odb_source_loose *loose, + const struct object_id *oid, + uint32_t *mask) { int subdir_nr = oid->hash[0]; - struct strbuf buf = STRBUF_INIT; size_t word_bits = bitsizeof(loose->subdir_seen[0]); size_t word_index = subdir_nr / word_bits; - size_t mask = (size_t)1u << (subdir_nr % word_bits); - uint32_t *bitmap; if (subdir_nr < 0 || (size_t) subdir_nr >= bitsizeof(loose->subdir_seen)) BUG("subdir_nr out of range"); - bitmap = &loose->subdir_seen[word_index]; + *mask = (size_t)1u << (subdir_nr % word_bits); + + return &loose->subdir_seen[word_index]; +} + +static struct oidtree *odb_source_loose_cache(struct odb_source_loose *loose, + const struct object_id *oid) +{ + struct strbuf buf = STRBUF_INIT; + uint32_t *bitmap, mask; + + bitmap = subdir_seen_bitmap(loose, oid, &mask); if (*bitmap & mask) return loose->cache; + if (!loose->cache) { ALLOC_ARRAY(loose->cache, 1); oidtree_init(loose->cache); } strbuf_addstr(&buf, loose->base.path); - for_each_file_in_obj_subdir(subdir_nr, &buf, + for_each_file_in_obj_subdir(oid->hash[0], &buf, loose->base.odb->repo->hash_algo, append_loose_object, NULL, NULL, -- 2.56.0.500.g5d06dc02cd.dirty ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] odb/source-loose: fix existence checks for newly written objects 2026-11-04 12:16 [PATCH 0/2] odb/source-loose: fix failing existence checks caused by stale cache Patrick Steinhardt 2026-11-04 12:16 ` [PATCH 1/2] odb/source-loose: extract logic to retrieve subdirectory bitmap Patrick Steinhardt @ 2026-11-04 12:16 ` Patrick Steinhardt 2026-11-04 17:47 ` Junio C Hamano 2026-11-05 19:43 ` Jeff King 2026-11-04 12:57 ` [PATCH 0/2] odb/source-loose: fix failing existence checks caused by stale cache Kristofer Karlsson 2 siblings, 2 replies; 9+ messages in thread From: Patrick Steinhardt @ 2026-11-04 12:16 UTC (permalink / raw) To: git; +Cc: Patrick Ohly The loose object source maintains a cache of object IDs that it uses to answer existence checks fast. This cache is lazily populated: whenever we are asked about a specific object ID, we scan the whole sharding directory it would live in and populate the cache with the objects we find. Subsequent existence checks are then answered from the cache without hitting the filesystem. But when writing a new loose object we don't update that cache. That's fine in the case where the cache for its sharding directory has not yet been populated, because we would then populate it on a subsequent read and thus surface the newly written object. But when it _is_ populated already, then any subsequent existence check will claim that the object does not exist at all until the cache is flushed. This issue can cause git-merge(1) to die in a partial clone because we try to fetch an object that we have just written, as reported in [1]: 1. `prefetch_for_content_merges()` performs quick existence checks for all blobs that need a content merge to figure out which ones need to be fetched from the promisor remote. These lookups populate the loose object cache for the respective sharding directories. 2. We lazily fetch the missing blobs, which writes a new packfile. 3. We perform the merge and write the resulting blobs as loose objects. The cache is not updated, so for all blobs whose sharding directory was populated in (1), the object existence checks will fail. 4. When checking out the merge result, `prefetch_cache_entries()` performs existence checks for all index entries that need to be written to the worktree. But due to the stale cache, the merged blobs are reported as missing, and consequently we try to fetch them from the promisor remote. This fetch fails because the remote will most likely not have them. This issue bisects to 5946a564cdd (odb/source: make `read_object_info()` function pluggable, 2026-03-05), because in that commit we stopped flushing the loose object cache when we try to read a missing object. That was done intentionally though: a failing read is only retried when it is a non-QUICK read, whereas the cache only affects QUICK reads. So flushing the cache for a non-QUICK read wouldn't ever cause us to find that object. So the logic described here only ever worked by chance, as the cache was incoherent after writes even before that commit. It only used to work because of the interleaving sequence of QUICK and non-QUICK reads: when we read the lazily-fetched blobs in (3) via a non-QUICK read, the first read fails because the new packfile has not yet been added to the "packed" source. On the retry we then used to flush the loose object cache, and that in turn means that the subsequent existence check in (4) rescanned the sharding directories and found the merged blobs. We could in theory fix this by flushing all the loose object caches again, like we used to do before 5946a564cdd. But that wouldn't address the root cause: the cache would still be stale unless somebody does a non-QUICK read for a missing object. Instead, fix the bug by updating the cache whenever we write a new loose object. [1]: <yrjh1p92qc7w.fsf@pohly-mobl1.fritz.box> Reported-by: Patrick Ohly <patrick.ohly@intel.com> Signed-off-by: Patrick Steinhardt <ps@pks.im> --- odb/source-loose.c | 30 +++++++++++++++++++++++++++--- t/t5616-partial-clone.sh | 43 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 3 deletions(-) diff --git a/odb/source-loose.c b/odb/source-loose.c index da8f9767257..bcbd29ce3a0 100644 --- a/odb/source-loose.c +++ b/odb/source-loose.c @@ -68,6 +68,19 @@ static struct oidtree *odb_source_loose_cache(struct odb_source_loose *loose, return loose->cache; } +/* + * Mark an object as seen in the loose cache. Note that we only do this in case + * the particular subdirectory was scanned already. If it wasn't, then the next + * read would populate it anyway. + */ +static void loose_cache_mark_seen(struct odb_source_loose *loose, + const struct object_id *oid) +{ + uint32_t mask, *bitmap = subdir_seen_bitmap(loose, oid, &mask); + if (*bitmap & mask) + oidtree_insert(loose->cache, oid, NULL); +} + static int quick_has_loose(struct odb_source_loose *loose, const struct object_id *oid) { @@ -824,8 +837,13 @@ static int write_loose_object(struct odb_source_loose *loose, warning_errno(_("failed utime() on %s"), tmp_file.buf); } - return finalize_object_file_flags(loose->base.odb->repo, tmp_file.buf, filename.buf, - FOF_SKIP_COLLISION_CHECK); + ret = finalize_object_file_flags(loose->base.odb->repo, tmp_file.buf, filename.buf, + FOF_SKIP_COLLISION_CHECK); + if (ret < 0) + return ret; + + loose_cache_mark_seen(loose, oid); + return 0; } static int odb_source_loose_write_object(struct odb_source *source, @@ -967,8 +985,14 @@ static int odb_source_loose_write_object_stream(struct odb_source *source, err = finalize_object_file_flags(loose->base.odb->repo, tmp_file.buf, filename.buf, FOF_SKIP_COLLISION_CHECK); - if (!err && compat) + if (err < 0) + goto cleanup; + + loose_cache_mark_seen(loose, oid); + + if (compat) err = repo_add_loose_object_map(loose, oid, &compat_oid); + cleanup: strbuf_release(&tmp_file); strbuf_release(&filename); diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh index 7cc6f58a31f..9636652362f 100755 --- a/t/t5616-partial-clone.sh +++ b/t/t5616-partial-clone.sh @@ -747,6 +747,49 @@ test_expect_success 'after fetching descendants of non-promisor commits, gc work git -C partial gc --prune=now ' +write_many_mergeable_files () { + for i in $(test_seq 50) + do + { + echo "$1 $i" && + test_seq 8 | sed "s/^/line /" && + echo "$2 $i" + } >"$3/file-$i" || return 1 + done +} + +test_expect_success 'merge does not lazy-fetch locally written objects' ' + test_when_finished "rm -rf server client merged" && + git init server && + ( + cd server && + git config set uploadpack.allowfilter 1 && + + write_many_mergeable_files top-A bottom-A . && + git add . && + git commit -m A && + + write_many_mergeable_files top-B bottom-A . && + git commit -a -m B && + + git checkout -b C main~1 && + write_many_mergeable_files top-A bottom-C . && + echo C >only-in-C && + git add . && + git commit -m C + ) && + + git clone --filter=blob:none --branch=main "file://$(pwd)/server" client && + git -C client merge origin/C && + + mkdir merged && + write_many_mergeable_files top-B bottom-C merged && + for m in merged/* + do + test_cmp "$m" client/"$(basename "$m")" || return 1 + done && + test_cmp server/only-in-C client/only-in-C +' . "$TEST_DIRECTORY"/lib-httpd.sh start_httpd -- 2.56.0.500.g5d06dc02cd.dirty ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] odb/source-loose: fix existence checks for newly written objects 2026-11-04 12:16 ` [PATCH 2/2] odb/source-loose: fix existence checks for newly written objects Patrick Steinhardt @ 2026-11-04 17:47 ` Junio C Hamano 2026-11-05 19:43 ` Jeff King 1 sibling, 0 replies; 9+ messages in thread From: Junio C Hamano @ 2026-11-04 17:47 UTC (permalink / raw) To: Patrick Steinhardt; +Cc: git, Patrick Ohly Patrick Steinhardt <ps@pks.im> writes: > @@ -824,8 +837,13 @@ static int write_loose_object(struct odb_source_loose *loose, > warning_errno(_("failed utime() on %s"), tmp_file.buf); > } > > - return finalize_object_file_flags(loose->base.odb->repo, tmp_file.buf, filename.buf, > - FOF_SKIP_COLLISION_CHECK); > + ret = finalize_object_file_flags(loose->base.odb->repo, tmp_file.buf, filename.buf, > + FOF_SKIP_COLLISION_CHECK); > + if (ret < 0) > + return ret; > + > + loose_cache_mark_seen(loose, oid); > + return 0; > } > > static int odb_source_loose_write_object(struct odb_source *source, > @@ -967,8 +985,14 @@ static int odb_source_loose_write_object_stream(struct odb_source *source, > > err = finalize_object_file_flags(loose->base.odb->repo, tmp_file.buf, filename.buf, > FOF_SKIP_COLLISION_CHECK); > - if (!err && compat) > + if (err < 0) > + goto cleanup; > + > + loose_cache_mark_seen(loose, oid); > + > + if (compat) > err = repo_add_loose_object_map(loose, oid, &compat_oid); > + > cleanup: > strbuf_release(&tmp_file); > strbuf_release(&filename); Not about this change but the lack of "if (compat)" stuff in the previous hunk puzzled me. It turns out that the logic to write a single loose object is so big that write_loose_object(), which is the subject of the previous hunk, helps another function, odb_source_loose_write_object(), by being its helper function, and this caller has the corresponding "if (compat)" logic to produce the hash interoperability data. Looking very good. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] odb/source-loose: fix existence checks for newly written objects 2026-11-04 12:16 ` [PATCH 2/2] odb/source-loose: fix existence checks for newly written objects Patrick Steinhardt 2026-11-04 17:47 ` Junio C Hamano @ 2026-11-05 19:43 ` Jeff King 2026-11-06 11:16 ` Patrick Steinhardt 1 sibling, 1 reply; 9+ messages in thread From: Jeff King @ 2026-11-05 19:43 UTC (permalink / raw) To: Patrick Steinhardt; +Cc: git, Patrick Ohly On Wed, Nov 04, 2026 at 01:16:35PM +0100, Patrick Steinhardt wrote: > This issue can cause git-merge(1) to die in a partial clone because we > try to fetch an object that we have just written, as reported in [1]: > > 1. `prefetch_for_content_merges()` performs quick existence checks for > all blobs that need a content merge to figure out which ones need > to be fetched from the promisor remote. These lookups populate the > loose object cache for the respective sharding directories. > > 2. We lazily fetch the missing blobs, which writes a new packfile. > > 3. We perform the merge and write the resulting blobs as loose > objects. The cache is not updated, so for all blobs whose sharding > directory was populated in (1), the object existence checks will > fail. > > 4. When checking out the merge result, `prefetch_cache_entries()` > performs existence checks for all index entries that need to be > written to the worktree. But due to the stale cache, the merged > blobs are reported as missing, and consequently we try to fetch > them from the promisor remote. This fetch fails because the remote > will most likely not have them. I don't doubt this sequence of events, but it feels like it is pointing to a separate underlying bug. Anybody who is passing QUICK should be prepared to get a false negative and recover reasonably. That's what QUICK is for. And anybody who is not passing QUICK will not use the cache (as you note, they do not use the cache at all, but it would also be reasonable to use it and fall back to a refresh). So it sounds like somebody is using QUICK who should not be in this path. At first glance, it makes sense for prefetch_cache_entries() to be QUICK: it is just a prefetch, so it's happy to get a wrong answer. But as we've seen, a false negative here can be a problem because it actually tries the fetch. The most obvious issue is the one we saw here: if the fetch fails because the object is in fact local-only, then rather than say "oh well, I guess it is not something we can pre-fetch" we barf at the lower level, in promisor_remote_get_direct(). It feels like that low-level function should be informed that the caller is in "false negatives are OK" QUICK mode and quietly return failure. But even that is tricky. Our fetch request is all-or-nothing, so one missing local object means we can't prefetch anything. Not great. Plus, even a quietly-discarded fetch attempt is expensive to do, when we could much more cheaply get the object by just looking again at the filesystem. So I kind of wonder if prefetch_cache_entries(), along with any caller doing an OBJECT_INFO_FOR_PREFETCH check, ought to omit the QUICK flag. It would mean that the normal case of having missing partial objects (about to be fetched) would cause us to re-scan the local pack list, but that's not _that_ expensive, especially compared to the fetch we're about to do. Possibly we'd want some logic to avoid doing the re-scan for every object (so if you are about to prefetch for a million objects, you don't want to trigger a million SECOND_READ re-scans, but rather do it once). That would require some refactoring. But it would fix this problem, along with any other related ones. In particular, I think your fix here is leaving lots of cases with the same problem. Missing our own loose write in the cache is an obvious problem, but it's not the only reason we might fail to see a local object: it could be in a pack now, or it could have been written as a loose object by some _other_ process. So I suspect you could cause the same problem racily by running "git log" on some local-only commits while somebody else is repacking, with a sequence like: 1. git-log scans the odb and builds an internal packlist. 2. git-repack runs in another process, migrating loose objects to packs. git-log doesn't know about our new pack. 3. git-log tries to diff a commit. In a non-partial clone, the diff code eventually tries to read that object, realizes we can't find it, and does a SECOND_READ. But in a partial clone, we try to prefetch via diff_queued_diff_prefetch(). That uses the same FOR_PREFETCH flag and will try to fetch it. I'd guess most such cases are going to be racy, though you can imagine ones that aren't. E.g., if mktree actually did pre-fetching, then something like: for fn in $files do oid=$(git hash-object -w $fn) printf "100644 blob $oid\t$fn" done | git mktree might run into similar problems. It needs to know about newly written local loose objects, but they were written externally. You might even be able to find a similar case where git-merge ends up writing the objects via a helper process, but I didn't check. So I dunno. This is fixing a real case and not making anything worse, so maybe it is reasonable in the interim. But it's the tip of the iceberg, I think. > We could in theory fix this by flushing all the loose object caches > again, like we used to do before 5946a564cdd. But that wouldn't address > the root cause: the cache would still be stale unless somebody does a > non-QUICK read for a missing object. Instead, fix the bug by updating > the cache whenever we write a new loose object. I agree that the object cache flushing was only fixing this problem by chance. But I think it might be worth reconsidering the change from 5946a564cdd. Flushing the cache (or not) should always be an optimization problem, not a correctness one (assuming the rest of the code is bug-free and using QUICK sensibly). But a SECOND_READ fallback is a good indication that information about our odb is stale, and it's a good time (from an optimization perspective) to flush them. Side note: I do think there is a correctness problem with caches going the other way, with false positives. We might say "we have object X" because we saw that it was loose earlier and cached the result, but a simultaneous maintenance job then pruned it. So imagine something like a fetch, where we opportunistically optimize by saying "oh, we already have X". Later, when it turns out we don't, the fetch fails, even though it _could_ have succeeded if we realized from the start that we didn't have it. But that is a general problem with racy pruning in Git, and it affects both packs and loose objects. Nor would flushing the caches solve it (though it could help reduce the race window in some cases). So not something to solve here, but I just wanted to qualify my "cache flushing should never affect correctness" claim. :) -Peff ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] odb/source-loose: fix existence checks for newly written objects 2026-11-05 19:43 ` Jeff King @ 2026-11-06 11:16 ` Patrick Steinhardt 0 siblings, 0 replies; 9+ messages in thread From: Patrick Steinhardt @ 2026-11-06 11:16 UTC (permalink / raw) To: Jeff King; +Cc: git, Patrick Ohly On Thu, Nov 05, 2026 at 02:43:14PM -0500, Jeff King wrote: > On Wed, Nov 04, 2026 at 01:16:35PM +0100, Patrick Steinhardt wrote: > > > This issue can cause git-merge(1) to die in a partial clone because we > > try to fetch an object that we have just written, as reported in [1]: > > > > 1. `prefetch_for_content_merges()` performs quick existence checks for > > all blobs that need a content merge to figure out which ones need > > to be fetched from the promisor remote. These lookups populate the > > loose object cache for the respective sharding directories. > > > > 2. We lazily fetch the missing blobs, which writes a new packfile. > > > > 3. We perform the merge and write the resulting blobs as loose > > objects. The cache is not updated, so for all blobs whose sharding > > directory was populated in (1), the object existence checks will > > fail. > > > > 4. When checking out the merge result, `prefetch_cache_entries()` > > performs existence checks for all index entries that need to be > > written to the worktree. But due to the stale cache, the merged > > blobs are reported as missing, and consequently we try to fetch > > them from the promisor remote. This fetch fails because the remote > > will most likely not have them. > > I don't doubt this sequence of events, but it feels like it is pointing > to a separate underlying bug. > > Anybody who is passing QUICK should be prepared to get a false negative > and recover reasonably. That's what QUICK is for. And anybody who is not > passing QUICK will not use the cache (as you note, they do not use the > cache at all, but it would also be reasonable to use it and fall back to > a refresh). > > So it sounds like somebody is using QUICK who should not be in this > path. At first glance, it makes sense for prefetch_cache_entries() to be > QUICK: it is just a prefetch, so it's happy to get a wrong answer. > > But as we've seen, a false negative here can be a problem because it > actually tries the fetch. The most obvious issue is the one we saw here: > if the fetch fails because the object is in fact local-only, then rather > than say "oh well, I guess it is not something we can pre-fetch" we barf > at the lower level, in promisor_remote_get_direct(). It feels like that > low-level function should be informed that the caller is in "false > negatives are OK" QUICK mode and quietly return failure. > > But even that is tricky. Our fetch request is all-or-nothing, so one > missing local object means we can't prefetch anything. Not great. Plus, > even a quietly-discarded fetch attempt is expensive to do, when we could > much more cheaply get the object by just looking again at the > filesystem. > > So I kind of wonder if prefetch_cache_entries(), along with any caller > doing an OBJECT_INFO_FOR_PREFETCH check, ought to omit the QUICK flag. > It would mean that the normal case of having missing partial objects > (about to be fetched) would cause us to re-scan the local pack list, but > that's not _that_ expensive, especially compared to the fetch we're > about to do. Yeah, that's a valid point. Honestly, by now I'm almost urged to just drop the QUICK flag altogther. It's a source of bugs, and the longer we look at it the more cases we find where the existing users thereof are buggy in specific edge cases. We just recently removed one user of QUICK, and this here is another dubious case. I wouldn't be surprised if we could find edge cases for the other, remaining users, as well. > Possibly we'd want some logic to avoid doing the re-scan for every > object (so if you are about to prefetch for a million objects, you don't > want to trigger a million SECOND_READ re-scans, but rather do it once). > That would require some refactoring. > > But it would fix this problem, along with any other related ones. In > particular, I think your fix here is leaving lots of cases with the same > problem. Missing our own loose write in the cache is an obvious problem, > but it's not the only reason we might fail to see a local object: it > could be in a pack now, or it could have been written as a loose object > by some _other_ process. True. I think it's a valuable fix regardless as it ensures that our cache reflects what we ourselves just did, so it's a simple improvement. But there's definitely more edge cases to this whole thing. > So I suspect you could cause the same problem racily by running "git > log" on some local-only commits while somebody else is repacking, with a > sequence like: > > 1. git-log scans the odb and builds an internal packlist. > > 2. git-repack runs in another process, migrating loose objects to > packs. git-log doesn't know about our new pack. > > 3. git-log tries to diff a commit. In a non-partial clone, the diff > code eventually tries to read that object, realizes we can't find > it, and does a SECOND_READ. But in a partial clone, we try to > prefetch via diff_queued_diff_prefetch(). That uses the same > FOR_PREFETCH flag and will try to fetch it. > > I'd guess most such cases are going to be racy, though you can imagine > ones that aren't. E.g., if mktree actually did pre-fetching, then > something like: > > for fn in $files > do > oid=$(git hash-object -w $fn) > printf "100644 blob $oid\t$fn" > done | > git mktree > > might run into similar problems. It needs to know about newly written > local loose objects, but they were written externally. You might even be > able to find a similar case where git-merge ends up writing the objects > via a helper process, but I didn't check. > > So I dunno. This is fixing a real case and not making anything worse, so > maybe it is reasonable in the interim. But it's the tip of the iceberg, > I think. No arguing there. I'd propose to still move ahead with this particular fix here as it addresses the original bug report and addresses the regression. But I agree that we may want to invest some more time into this problem space after it lands. > > We could in theory fix this by flushing all the loose object caches > > again, like we used to do before 5946a564cdd. But that wouldn't address > > the root cause: the cache would still be stale unless somebody does a > > non-QUICK read for a missing object. Instead, fix the bug by updating > > the cache whenever we write a new loose object. > > I agree that the object cache flushing was only fixing this problem by > chance. But I think it might be worth reconsidering the change from > 5946a564cdd. Flushing the cache (or not) should always be an > optimization problem, not a correctness one (assuming the rest of the > code is bug-free and using QUICK sensibly). But a SECOND_READ fallback > is a good indication that information about our odb is stale, and it's a > good time (from an optimization perspective) to flush them. It may be, but it may just as well not be. In some cases we may truly not have a specific object, and the caller would want to know about that. And in that case we should avoid flushing to only those cases where it's really necessary. Patrick ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] odb/source-loose: fix failing existence checks caused by stale cache 2026-11-04 12:16 [PATCH 0/2] odb/source-loose: fix failing existence checks caused by stale cache Patrick Steinhardt 2026-11-04 12:16 ` [PATCH 1/2] odb/source-loose: extract logic to retrieve subdirectory bitmap Patrick Steinhardt 2026-11-04 12:16 ` [PATCH 2/2] odb/source-loose: fix existence checks for newly written objects Patrick Steinhardt @ 2026-11-04 12:57 ` Kristofer Karlsson 2026-11-04 16:27 ` Derrick Stolee 2026-11-05 0:09 ` Junio C Hamano 2 siblings, 2 replies; 9+ messages in thread From: Kristofer Karlsson @ 2026-11-04 12:57 UTC (permalink / raw) To: Patrick Steinhardt; +Cc: git, Patrick Ohly On Wed, 4 Nov 2026 at 13:17, Patrick Steinhardt <ps@pks.im> wrote: > > Hi, > > this small patch series fixes the bug reported in [1]. > > In summary, when git-merge(1) writes merged blobs it may be unable to > read them back because the loose object cache does not get updated. > Consequently, we may think that the blobs do not exist locally, and thus > in a promisor remote we may perform a backfill fetch. But as the remote > doesn't know those objects we'll fail. > > Thanks! > > Patrick The fix looks good to me, and I verified that the test fails without it. My own local fix is logically equivalent, which is a good sign, though perhaps that biased my review. Thanks, Kristofer ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] odb/source-loose: fix failing existence checks caused by stale cache 2026-11-04 12:57 ` [PATCH 0/2] odb/source-loose: fix failing existence checks caused by stale cache Kristofer Karlsson @ 2026-11-04 16:27 ` Derrick Stolee 2026-11-05 0:09 ` Junio C Hamano 1 sibling, 0 replies; 9+ messages in thread From: Derrick Stolee @ 2026-11-04 16:27 UTC (permalink / raw) To: git On 11/4/2026 7:57 AM, Kristofer Karlsson wrote: > On Wed, 4 Nov 2026 at 13:17, Patrick Steinhardt <ps@pks.im> wrote: >> >> Hi, >> >> this small patch series fixes the bug reported in [1]. >> >> In summary, when git-merge(1) writes merged blobs it may be unable to >> read them back because the loose object cache does not get updated. >> Consequently, we may think that the blobs do not exist locally, and thus >> in a promisor remote we may perform a backfill fetch. But as the remote >> doesn't know those objects we'll fail. >> >> Thanks! >> >> Patrick > > The fix looks good to me, and I verified that the test fails > without it. My own local fix is logically equivalent, > which is a good sign, though perhaps that biased my review. I also think the fix looks good. (I haven't verified it locally like Kristofer did, but the test is good.) Thanks, -Stolee ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] odb/source-loose: fix failing existence checks caused by stale cache 2026-11-04 12:57 ` [PATCH 0/2] odb/source-loose: fix failing existence checks caused by stale cache Kristofer Karlsson 2026-11-04 16:27 ` Derrick Stolee @ 2026-11-05 0:09 ` Junio C Hamano 1 sibling, 0 replies; 9+ messages in thread From: Junio C Hamano @ 2026-11-05 0:09 UTC (permalink / raw) To: Kristofer Karlsson; +Cc: Patrick Steinhardt, git, Patrick Ohly Kristofer Karlsson <krka@spotify.com> writes: > On Wed, 4 Nov 2026 at 13:17, Patrick Steinhardt <ps@pks.im> wrote: >> >> Hi, >> >> this small patch series fixes the bug reported in [1]. >> >> In summary, when git-merge(1) writes merged blobs it may be unable to >> read them back because the loose object cache does not get updated. >> Consequently, we may think that the blobs do not exist locally, and thus >> in a promisor remote we may perform a backfill fetch. But as the remote >> doesn't know those objects we'll fail. >> >> Thanks! >> >> Patrick > > The fix looks good to me, and I verified that the test fails > without it. My own local fix is logically equivalent, > which is a good sign, though perhaps that biased my review. Thanks, both. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2026-11-06 11:16 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-11-04 12:16 [PATCH 0/2] odb/source-loose: fix failing existence checks caused by stale cache Patrick Steinhardt 2026-11-04 12:16 ` [PATCH 1/2] odb/source-loose: extract logic to retrieve subdirectory bitmap Patrick Steinhardt 2026-11-04 12:16 ` [PATCH 2/2] odb/source-loose: fix existence checks for newly written objects Patrick Steinhardt 2026-11-04 17:47 ` Junio C Hamano 2026-11-05 19:43 ` Jeff King 2026-11-06 11:16 ` Patrick Steinhardt 2026-11-04 12:57 ` [PATCH 0/2] odb/source-loose: fix failing existence checks caused by stale cache Kristofer Karlsson 2026-11-04 16:27 ` Derrick Stolee 2026-11-05 0:09 ` Junio C Hamano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.