On Mon, Mar 06 2023, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <derrickstolee@xxxxxxxxxx> > > When an object is not found in a repository's object store, we sometimes > call reprepare_packed_git() to see if the object was temporarily moved > into a new pack-file (and its old pack-file or loose object was > deleted). This process does a scan of each pack directory within each > odb, but does not reevaluate if the odb list needs updating. > > Create a new reprepare_alt_odb() method that is a similar wrapper around > prepare_alt_odb(). Call it from reprepare_packed_git() under the object > read lock to avoid readers from interacting with a potentially > incomplete odb being added to the odb list. > > prepare_alt_odb() already avoids adding duplicate odbs to the list > during its progress, so it is safe to call it again from > reprepare_alt_odb() without worrying about duplicate odbs. > > This change is specifically for concurrent changes to the repository, so > it is difficult to create a test that guarantees this behavior is > correct. I manually verified by introducing a reprepare_packed_git() call > into get_revision() and stepped into that call in a debugger with a > parent 'git log' process. Multiple runs of reprepare_alt_odb() kept > the_repository->objects->odb as a single-item chain until I added a > .git/objects/info/alternates file in a different process. The next run > added the new odb to the chain and subsequent runs did not add to the > chain. I found this a bit hard to read, as one migh think from just this explanation that you're adding ODB locking as a new concept here, adding "existing" in front of "read lock" above might help. But in fact we've been doing the locking since 6c307626f1e (grep: protect packed_git [re-]initialization, 2020-01-15). So the only thing that really needs justification here is that putting the alternates re-reading under the same lock There is a really interesting potential caveat here which you're not discussing, which is... > Signed-off-by: Derrick Stolee <derrickstolee@xxxxxxxxxx> > --- > object-file: reprepare alternates when necessary > > This subtlety was notice by Michael Haggerty due to how alternates are > used server-side at $DAYJOB. Moving pack-files from a repository to the > alternate occasionally causes failures because processes that start > before the alternate exists don't know how to find that alternate at > run-time. > > Thanks, > > * Stolee > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1490%2Fderrickstolee%2Fstolee%2Freprepare-alternates-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1490/derrickstolee/stolee/reprepare-alternates-v1 > Pull-Request: https://github.com/gitgitgadget/git/pull/1490 > > object-file.c | 6 ++++++ > object-store.h | 1 + > packfile.c | 1 + > 3 files changed, 8 insertions(+) > > diff --git a/object-file.c b/object-file.c > index 939865c1ae0..22acc7fd8e9 100644 > --- a/object-file.c > +++ b/object-file.c > @@ -944,6 +944,12 @@ void prepare_alt_odb(struct repository *r) > r->objects->loaded_alternates = 1; > } > > +void reprepare_alt_odb(struct repository *r) > +{ > + r->objects->loaded_alternates = 0; > + prepare_alt_odb(r); > +} > + > /* Returns 1 if we have successfully freshened the file, 0 otherwise. */ > static int freshen_file(const char *fn) > { > diff --git a/object-store.h b/object-store.h > index 1a713d89d7c..750c29daa54 100644 > --- a/object-store.h > +++ b/object-store.h > @@ -56,6 +56,7 @@ KHASH_INIT(odb_path_map, const char * /* key: odb_path */, > struct object_directory *, 1, fspathhash, fspatheq) > > void prepare_alt_odb(struct repository *r); > +void reprepare_alt_odb(struct repository *r); > char *compute_alternate_path(const char *path, struct strbuf *err); > struct object_directory *find_odb(struct repository *r, const char *obj_dir); > typedef int alt_odb_fn(struct object_directory *, void *); > diff --git a/packfile.c b/packfile.c > index 79e21ab18e7..2b28918a05e 100644 > --- a/packfile.c > +++ b/packfile.c > @@ -1008,6 +1008,7 @@ void reprepare_packed_git(struct repository *r) > struct object_directory *odb; > > obj_read_lock(); > + reprepare_alt_odb(r); > for (odb = r->objects->odb; odb; odb = odb->next) > odb_clear_loose_cache(odb); > > > base-commit: d15644fe0226af7ffc874572d968598564a230dd This seems reasonable, but wouldn't this do the same without introducing an API function just for this one use-case? That's of course a nit, and you seem to have been adding this for consistency with reprepare_packed_git(), but it already "owns" the "approximate_object_count_valid" and "packed_git_initialized" flags in "struct raw_object_store". So as we'll only need this from reprepare_packed_git() isn't it better to declare that "loaded_alternates" is another such flag? Perhaps not, but as the resulting patch is much shorter it seems worth considering. ...but to continue the above, the *really* important thing here (and correct me if I'm wrong) is that we really need to *first* prepare the alternates, and *then* do the rest, as our new alternates might point to new loose objects and packs. So with both of the above (the same could be done with your new helper) something like this would IMO make that much clearer: diff --git a/packfile.c b/packfile.c index 79e21ab18e7..50cb46ca4b7 100644 --- a/packfile.c +++ b/packfile.c @@ -1008,6 +1008,13 @@ void reprepare_packed_git(struct repository *r) struct object_directory *odb; obj_read_lock(); + /* + * New alternates might point to new loose & pack dirs, so we + * must first read those. + */ + r->objects->loaded_alternates = 0; + prepare_alt_odb(r); + for (odb = r->objects->odb; odb; odb = odb->next) odb_clear_loose_cache(odb); And, I think this is an exsiting edge case, but we're only locking the ODB of the "parent" repository in this case, so if we have alternates in play aren't we going to racily compute the rest here, the loose objects and packs of the alternates we're about to consult aren't under the same lock?
