Skip to content

Commit 83d4060

Browse files
peffpeff
committed
refdb_fs: refresh packed refs after loose during iteration
There's a possible race between loose and packed refs when iterating over refs. It works like this: 0. A loose ref X exists. 1. libgit2 reads the packed-refs file, which does not mention X. 2. A simultaneously-running `git pack-refs` commands packs X and prunes the loose version. 3. libgit2 reads the loose directory, which no longer has X. At this point, libgit2 claims that X does not exist, even though it does. We can solve this by reading loose refs before packed. This works because refs always migrate from loose to packed, but not the other way around (technically an update to a packed ref creates a new loose file, but it doesn't _delete_ the packed one, so we're guaranteed to always see either the old or the new version). See also git/git@98eeb09, which fixed this same race in upstream Git, and discusses the race in more detail. Note that just naively flipping the order of packed_load() and iter_load_loose_paths() doesn't quite work. The latter expects the packed refs to already be loaded, so that it can mark them with PACKREF_SHADOWED. When doing it in reverse order, we have two options: 1. packed_reload() should take the "loose" vector and check to see if refs it loads are shadowed. Doing this efficiently requires the loose vector to be sorted, which might impact the output. 2. After loading both, take an additional pass over the loose vector and mark shadowed packrefs. This is slightly less efficient than (1) because of the extra pass, but it's big-O identical. This patch does (2), since it seems less the risky route, and the efficiency difference should be minimal. If we really care about efficiency, then we should scrap the current PACKREF_SHADOWED bit entirely, and instead do a sorted merge of the two lists. Note that in this patch we do leave the existing PACKREF_SHADOWED code in iter_load_loose_paths(). If the packed-refs file has already been loaded, then that code would kick in (and our subsequent packed-load would be a noop, as we only reload the file if the timestamp indicates it has changed).
1 parent e191637 commit 83d4060

1 file changed

Lines changed: 25 additions & 4 deletions

File tree

‎src/refdb_fs.c‎

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -666,18 +666,32 @@ static int refdb_fs_backend__iterator_next_name(
666666
return error;
667667
}
668668

669+
static void mark_shadowed_packrefs(git_vector *loose, git_sortedcache *packed)
670+
{
671+
size_t i;
672+
const char *name;
673+
674+
git_vector_foreach(loose, i, name) {
675+
struct packref *ref;
676+
677+
git_sortedcache_rlock(packed);
678+
ref = git_sortedcache_lookup(packed, name);
679+
if (ref)
680+
ref->flags |= PACKREF_SHADOWED;
681+
git_sortedcache_runlock(packed);
682+
}
683+
}
684+
669685
static int refdb_fs_backend__iterator(
670686
git_reference_iterator **out, git_refdb_backend *_backend, const char *glob)
671687
{
672688
int error;
689+
int ret = -1;
673690
refdb_fs_iter *iter;
674691
refdb_fs_backend *backend = (refdb_fs_backend *)_backend;
675692

676693
assert(backend);
677694

678-
if ((error = packed_reload(backend)) < 0)
679-
return error;
680-
681695
iter = git__calloc(1, sizeof(refdb_fs_iter));
682696
GIT_ERROR_CHECK_ALLOC(iter);
683697

@@ -697,12 +711,19 @@ static int refdb_fs_backend__iterator(
697711
if (iter_load_loose_paths(backend, iter) < 0)
698712
goto fail;
699713

714+
if ((error = packed_reload(backend)) < 0) {
715+
ret = error;
716+
goto fail;
717+
}
718+
719+
mark_shadowed_packrefs(&iter->loose, backend->refcache);
720+
700721
*out = (git_reference_iterator *)iter;
701722
return 0;
702723

703724
fail:
704725
refdb_fs_backend__iterator_free((git_reference_iterator *)iter);
705-
return -1;
726+
return ret;
706727
}
707728

708729
static bool ref_is_available(

0 commit comments

Comments
 (0)