Subject: [PATCH v2] object-file: reprepare alternates when necessary

From: "Derrick Stolee via GitGitGadget" <[email-masked]>
Date: Wed, 08 Mar 2023 18:47:32 +0000

From: Derrick Stolee <[email-masked]>

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.

Extend reprepare_packed_git() to also reprepate the alternate odb list
by setting loaded_alternates to zero and calling prepare_alt_odb(). This
will add newly-discoverd odbs to the linked list, but will not duplicate
existing ones nor will it remove existing ones that are no longer listed
in the alternates file. Do this under the object read lock to avoid
readers from interacting with a potentially incomplete odb being added
to the odb list.

If the alternates file was edited to _remove_ some alternates during the
course of the Git process, Git will continue to see alternates that were
ever valid for that repository. ODBs are not removed from the list, the
same as the existing behavior before this change. Git already has
protections against an alternate directory disappearing from the
filesystem during the lifetime of a process, and those are still in
effect.

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

Signed-off-by: Derrick Stolee <[email-masked]>
---
    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.

    Update in v2
    ============
    
     * Instead of creating a new public reprepare_alt_odb() method, inline
       its implementation inside reprepare_packed_git().
     * Update commit message to be explicit about behavior with alternates
       being removed from the alternates file or from disk.
    
    Thanks,
    
     * Stolee

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1490%2Fderrickstolee%2Fstolee%2Freprepare-alternates-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1490/derrickstolee/stolee/reprepare-alternates-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1490

Range-diff vs v1:

 1:  3f42c9cdef7 ! 1:  9a5e1d9a9da object-file: reprepare alternates when necessary
     @@ Commit message
          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.
     +    Extend reprepare_packed_git() to also reprepate the alternate odb list
     +    by setting loaded_alternates to zero and calling prepare_alt_odb(). This
     +    will add newly-discoverd odbs to the linked list, but will not duplicate
     +    existing ones nor will it remove existing ones that are no longer listed
     +    in the alternates file. Do this under the object read lock to avoid
     +    readers from interacting with a potentially incomplete odb being added
     +    to the odb list.
     +
     +    If the alternates file was edited to _remove_ some alternates during the
     +    course of the Git process, Git will continue to see alternates that were
     +    ever valid for that repository. ODBs are not removed from the list, the
     +    same as the existing behavior before this change. Git already has
     +    protections against an alternate directory disappearing from the
     +    filesystem during the lifetime of a process, and those are still in
     +    effect.
      
          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
     +    parent 'git log' process. Multiple runs of prepare_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
     @@ Commit message
      
          Signed-off-by: Derrick Stolee <[email-masked]>
      
     - ## object-file.c ##
     -@@ object-file.c: 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)
     - {
     -
     - ## object-store.h ##
     -@@ object-store.h: 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 *);
     -
       ## packfile.c ##
      @@ packfile.c: void reprepare_packed_git(struct repository *r)
       	struct object_directory *odb;
       
       	obj_read_lock();
     -+	reprepare_alt_odb(r);
     ++
     ++	/*
     ++	 * Reprepare alt odbs, in case the alternates file was modified
     ++	 * during the course of this process. This only _adds_ odbs to
     ++	 * the linked list, so existing odbs will continue to exist for
     ++	 * the lifetime of the process.
     ++	 */
     ++	r->objects->loaded_alternates = 0;
     ++	prepare_alt_odb(r);
     ++
       	for (odb = r->objects->odb; odb; odb = odb->next)
       		odb_clear_loose_cache(odb);

 packfile.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/packfile.c b/packfile.c
index 79e21ab18e7..06419c8e8ca 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1008,6 +1008,16 @@ void reprepare_packed_git(struct repository *r)
 	struct object_directory *odb;
 
 	obj_read_lock();
+
+	/*
+	 * Reprepare alt odbs, in case the alternates file was modified
+	 * during the course of this process. This only _adds_ odbs to
+	 * the linked list, so existing odbs will continue to exist for
+	 * the lifetime of the process.
+	 */
+	r->objects->loaded_alternates = 0;
+	prepare_alt_odb(r);
+
 	for (odb = r->objects->odb; odb; odb = odb->next)
 		odb_clear_loose_cache(odb);

base-commit: d15644fe0226af7ffc874572d968598564a230dd
-- 
gitgitgadget