{"thread":{"id":"59348","subject":"[PATCH] object-file: reprepare alternates when necessary","startedAt":"2023-03-06T20:59:40Z","lastAt":"2023-03-11T03:09:30Z","messageCount":19,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","Taylor Blau","Ævar Arnfjörð Bjarmason","Derrick Stolee","Jeff King","Eric Wong","Jonathan Tan"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"473084","messageId":"pull.1490.git.1678136369387.gitgitgadget@gmail.com","threadId":"59348","inReplyTo":null,"subject":"[PATCH] object-file: reprepare alternates when necessary","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-03-06T20:59:29Z","receivedAt":"2023-03-06T20:59:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <derrickstolee@github.com>\n\nWhen an object is not found in a repository's object store, we sometimes\ncall reprepare_packed_git() to see if the object was temporarily moved\ninto a new pack-file (and its old pack-file or loose object was\ndeleted). This process does a scan of each pack directory within each\nodb, but does not reevaluate if the odb list needs updating.\n\nCreate a new reprepare_alt_odb() method that is a similar wrapper around\nprepare_alt_odb(). Call it from reprepare_packed_git() under the object\nread lock to avoid readers from interacting with a potentially\nincomplete odb being added to the odb list.\n\nprepare_alt_odb() already avoids adding duplicate odbs to the list\nduring its progress, so it is safe to call it again from\nreprepare_alt_odb() without worrying about duplicate odbs.\n\nThis change is specifically for concurrent changes to the repository, so\nit is difficult to create a test that guarantees this behavior is\ncorrect. I manually verified by introducing a reprepare_packed_git() call\ninto get_revision() and stepped into that call in a debugger with a\nparent 'git log' process. Multiple runs of reprepare_alt_odb() kept\nthe_repository->objects->odb as a single-item chain until I added a\n.git/objects/info/alternates file in a different process. The next run\nadded the new odb to the chain and subsequent runs did not add to the\nchain.\n\nSigned-off-by: Derrick Stolee <derrickstolee@github.com>\n---\n    object-file: reprepare alternates when necessary\n    \n    This subtlety was notice by Michael Haggerty due to how alternates are\n    used server-side at $DAYJOB. Moving pack-files from a repository to the\n    alternate occasionally causes failures because processes that start\n    before the alternate exists don't know how to find that alternate at\n    run-time.\n    \n    Thanks,\n    \n     * Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1490%2Fderrickstolee%2Fstolee%2Freprepare-alternates-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1490/derrickstolee/stolee/reprepare-alternates-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1490\n\n object-file.c  | 6 ++++++\n object-store.h | 1 +\n packfile.c     | 1 +\n 3 files changed, 8 insertions(+)\n\ndiff --git a/object-file.c b/object-file.c\nindex 939865c1ae0..22acc7fd8e9 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -944,6 +944,12 @@ void prepare_alt_odb(struct repository *r)\n \tr->objects->loaded_alternates = 1;\n }\n \n+void reprepare_alt_odb(struct repository *r)\n+{\n+\tr->objects->loaded_alternates = 0;\n+\tprepare_alt_odb(r);\n+}\n+\n /* Returns 1 if we have successfully freshened the file, 0 otherwise. */\n static int freshen_file(const char *fn)\n {\ndiff --git a/object-store.h b/object-store.h\nindex 1a713d89d7c..750c29daa54 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -56,6 +56,7 @@ KHASH_INIT(odb_path_map, const char * /* key: odb_path */,\n \tstruct object_directory *, 1, fspathhash, fspatheq)\n \n void prepare_alt_odb(struct repository *r);\n+void reprepare_alt_odb(struct repository *r);\n char *compute_alternate_path(const char *path, struct strbuf *err);\n struct object_directory *find_odb(struct repository *r, const char *obj_dir);\n typedef int alt_odb_fn(struct object_directory *, void *);\ndiff --git a/packfile.c b/packfile.c\nindex 79e21ab18e7..2b28918a05e 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1008,6 +1008,7 @@ void reprepare_packed_git(struct repository *r)\n \tstruct object_directory *odb;\n \n \tobj_read_lock();\n+\treprepare_alt_odb(r);\n \tfor (odb = r->objects->odb; odb; odb = odb->next)\n \t\todb_clear_loose_cache(odb);\n \n\nbase-commit: d15644fe0226af7ffc874572d968598564a230dd\n-- \ngitgitgadget\n"},{"id":"473093","messageId":"xmqqy1o97apj.fsf@gitster.g","threadId":"59348","inReplyTo":"pull.1490.git.1678136369387.gitgitgadget@gmail.com","subject":"Re: [PATCH] object-file: reprepare alternates when necessary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-06T22:54:00Z","receivedAt":"2023-03-06T22:54:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> @@ -1008,6 +1008,7 @@ void reprepare_packed_git(struct repository *r)\n>  \tstruct object_directory *odb;\n>  \n>  \tobj_read_lock();\n> +\treprepare_alt_odb(r);\n>  \tfor (odb = r->objects->odb; odb; odb = odb->next)\n>  \t\todb_clear_loose_cache(odb);\n\nHmph, if there was an old alternate ODB from which we took some\nloose object from and cached, and if that ODB no longer is on the\nupdated alternate list, would we now fail to clear the loose objects\ncache for the ODB?  Or are we only prepared for seeing \"more\"\nalternates and assume no existing alternates go away?\n\nOther than that, looking quite well reasoned.\n\n"},{"id":"473097","messageId":"ZAaFQJm6UGYH4YIi@nand.local","threadId":"59348","inReplyTo":"xmqqy1o97apj.fsf@gitster.g","subject":"Re: [PATCH] object-file: reprepare alternates when necessary","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-03-07T00:28:48Z","receivedAt":"2023-03-07T00:28:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Mar 06, 2023 at 02:54:00PM -0800, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> > @@ -1008,6 +1008,7 @@ void reprepare_packed_git(struct repository *r)\n> >  \tstruct object_directory *odb;\n> >\n> >  \tobj_read_lock();\n> > +\treprepare_alt_odb(r);\n> >  \tfor (odb = r->objects->odb; odb; odb = odb->next)\n> >  \t\todb_clear_loose_cache(odb);\n>\n> Hmph, if there was an old alternate ODB from which we took some\n> loose object from and cached, and if that ODB no longer is on the\n> updated alternate list, would we now fail to clear the loose objects\n> cache for the ODB?  Or are we only prepared for seeing \"more\"\n> alternates and assume no existing alternates go away?\n\nBased on my understanding of the patch, we are only prepared to see\n\"more\" alternates, rather than some existing alternate going away.\n\nThat being said, I am not certain that is how it works. Perhaps an\nalternate \"goes away\", but does not actually get removed from the list\nof alternate ODBs. If that's the case, any object lookup in that\nnow-missing ODB would fail, but any subsequent ODBs which were added\nafter calling reprepare_alt_odb() would succeed on that object lookup.\n\nSo, I don't know. I don't have the implementation details of the\nalternates ODB mechanism paged in enough to say for sure. Hopefully\nStolee can point us in the right direction.\n\n> Other than that, looking quite well reasoned.\n\nAgreed.\n\nThanks,\nTaylor\n"},{"id":"473117","messageId":"230307.861qm0235d.gmgdl@evledraar.gmail.com","threadId":"59348","inReplyTo":"pull.1490.git.1678136369387.gitgitgadget@gmail.com","subject":"Re: [PATCH] object-file: reprepare alternates when necessary","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-03-07T11:28:44Z","receivedAt":"2023-03-07T11:50:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Mar 06 2023, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee <derrickstolee@github.com>\n>\n> When an object is not found in a repository's object store, we sometimes\n> call reprepare_packed_git() to see if the object was temporarily moved\n> into a new pack-file (and its old pack-file or loose object was\n> deleted). This process does a scan of each pack directory within each\n> odb, but does not reevaluate if the odb list needs updating.\n>\n> Create a new reprepare_alt_odb() method that is a similar wrapper around\n> prepare_alt_odb(). Call it from reprepare_packed_git() under the object\n> read lock to avoid readers from interacting with a potentially\n> incomplete odb being added to the odb list.\n>\n> prepare_alt_odb() already avoids adding duplicate odbs to the list\n> during its progress, so it is safe to call it again from\n> reprepare_alt_odb() without worrying about duplicate odbs.\n>\n> This change is specifically for concurrent changes to the repository, so\n> it is difficult to create a test that guarantees this behavior is\n> correct. I manually verified by introducing a reprepare_packed_git() call\n> into get_revision() and stepped into that call in a debugger with a\n> parent 'git log' process. Multiple runs of reprepare_alt_odb() kept\n> the_repository->objects->odb as a single-item chain until I added a\n> .git/objects/info/alternates file in a different process. The next run\n> added the new odb to the chain and subsequent runs did not add to the\n> chain.\n\nI found this a bit hard to read, as one migh think from just this\nexplanation that you're adding ODB locking as a new concept here, adding\n\"existing\" in front of \"read lock\" above might help.\n\nBut in fact we've been doing the locking since 6c307626f1e (grep:\nprotect packed_git [re-]initialization, 2020-01-15). So the only thing\nthat really needs justification here is that putting the alternates\nre-reading under the same lock\n\nThere is a really interesting potential caveat here which you're not\ndiscussing, which is...\n\n> Signed-off-by: Derrick Stolee <derrickstolee@github.com>\n> ---\n>     object-file: reprepare alternates when necessary\n>     \n>     This subtlety was notice by Michael Haggerty due to how alternates are\n>     used server-side at $DAYJOB. Moving pack-files from a repository to the\n>     alternate occasionally causes failures because processes that start\n>     before the alternate exists don't know how to find that alternate at\n>     run-time.\n>     \n>     Thanks,\n>     \n>      * Stolee\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1490%2Fderrickstolee%2Fstolee%2Freprepare-alternates-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1490/derrickstolee/stolee/reprepare-alternates-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1490\n>\n>  object-file.c  | 6 ++++++\n>  object-store.h | 1 +\n>  packfile.c     | 1 +\n>  3 files changed, 8 insertions(+)\n>\n> diff --git a/object-file.c b/object-file.c\n> index 939865c1ae0..22acc7fd8e9 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -944,6 +944,12 @@ void prepare_alt_odb(struct repository *r)\n>  \tr->objects->loaded_alternates = 1;\n>  }\n>  \n> +void reprepare_alt_odb(struct repository *r)\n> +{\n> +\tr->objects->loaded_alternates = 0;\n> +\tprepare_alt_odb(r);\n> +}\n> +\n>  /* Returns 1 if we have successfully freshened the file, 0 otherwise. */\n>  static int freshen_file(const char *fn)\n>  {\n> diff --git a/object-store.h b/object-store.h\n> index 1a713d89d7c..750c29daa54 100644\n> --- a/object-store.h\n> +++ b/object-store.h\n> @@ -56,6 +56,7 @@ KHASH_INIT(odb_path_map, const char * /* key: odb_path */,\n>  \tstruct object_directory *, 1, fspathhash, fspatheq)\n>  \n>  void prepare_alt_odb(struct repository *r);\n> +void reprepare_alt_odb(struct repository *r);\n>  char *compute_alternate_path(const char *path, struct strbuf *err);\n>  struct object_directory *find_odb(struct repository *r, const char *obj_dir);\n>  typedef int alt_odb_fn(struct object_directory *, void *);\n> diff --git a/packfile.c b/packfile.c\n> index 79e21ab18e7..2b28918a05e 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -1008,6 +1008,7 @@ void reprepare_packed_git(struct repository *r)\n>  \tstruct object_directory *odb;\n>  \n>  \tobj_read_lock();\n> +\treprepare_alt_odb(r);\n>  \tfor (odb = r->objects->odb; odb; odb = odb->next)\n>  \t\todb_clear_loose_cache(odb);\n>  \n>\n> base-commit: d15644fe0226af7ffc874572d968598564a230dd\n\nThis seems reasonable, but wouldn't this do the same without introducing\nan API function just for this one use-case?\n\nThat's of course a nit, and you seem to have been adding this for\nconsistency with reprepare_packed_git(), but it already \"owns\" the\n\"approximate_object_count_valid\" and \"packed_git_initialized\" flags in\n\"struct raw_object_store\".\n\nSo as we'll only need this from reprepare_packed_git() isn't it better\nto declare that \"loaded_alternates\" is another such flag?\n\nPerhaps not, but as the resulting patch is much shorter it seems worth\nconsidering.\n\n...but to continue the above, the *really* important thing here (and\ncorrect me if I'm wrong) is that we really need to *first* prepare the\nalternates, and *then* do the rest, as our new alternates might point to\nnew loose objects and packs.\n\nSo with both of the above (the same could be done with your new helper)\nsomething like this would IMO make that much clearer:\n\n\tdiff --git a/packfile.c b/packfile.c\n\tindex 79e21ab18e7..50cb46ca4b7 100644\n\t--- a/packfile.c\n\t+++ b/packfile.c\n\t@@ -1008,6 +1008,13 @@ void reprepare_packed_git(struct repository *r)\n\t \tstruct object_directory *odb;\n\t \n\t \tobj_read_lock();\n\t+\t/*\n\t+\t * New alternates might point to new loose & pack dirs, so we\n\t+\t * must first read those.\n\t+\t */\n\t+\tr->objects->loaded_alternates = 0;\n\t+\tprepare_alt_odb(r);\n\t+\n\t \tfor (odb = r->objects->odb; odb; odb = odb->next)\n\t \t\todb_clear_loose_cache(odb);\n\nAnd, I think this is an exsiting edge case, but we're only locking the\nODB of the \"parent\" repository in this case, so if we have alternates in\nplay aren't we going to racily compute the rest here, the loose objects\nand packs of the alternates we're about to consult aren't under the same\nlock?\n\n\n"},{"id":"473122","messageId":"901299ac-e543-b7e5-0a1a-c90e667a947d@github.com","threadId":"59348","inReplyTo":"ZAaFQJm6UGYH4YIi@nand.local","subject":"Re: [PATCH] object-file: reprepare alternates when necessary","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-03-07T14:52:19Z","receivedAt":"2023-03-07T15:03:31Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/6/2023 7:28 PM, Taylor Blau wrote:\n> On Mon, Mar 06, 2023 at 02:54:00PM -0800, Junio C Hamano wrote:\n>> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>>\n>>> @@ -1008,6 +1008,7 @@ void reprepare_packed_git(struct repository *r)\n>>>  \tstruct object_directory *odb;\n>>>\n>>>  \tobj_read_lock();\n>>> +\treprepare_alt_odb(r);\n>>>  \tfor (odb = r->objects->odb; odb; odb = odb->next)\n>>>  \t\todb_clear_loose_cache(odb);\n>>\n>> Hmph, if there was an old alternate ODB from which we took some\n>> loose object from and cached, and if that ODB no longer is on the\n>> updated alternate list, would we now fail to clear the loose objects\n>> cache for the ODB?  Or are we only prepared for seeing \"more\"\n>> alternates and assume no existing alternates go away?\n> \n> Based on my understanding of the patch, we are only prepared to see\n> \"more\" alternates, rather than some existing alternate going away.\n> \n> That being said, I am not certain that is how it works. Perhaps an\n> alternate \"goes away\", but does not actually get removed from the list\n> of alternate ODBs. If that's the case, any object lookup in that\n> now-missing ODB would fail, but any subsequent ODBs which were added\n> after calling reprepare_alt_odb() would succeed on that object lookup.\n> \n> So, I don't know. I don't have the implementation details of the\n> alternates ODB mechanism paged in enough to say for sure. Hopefully\n> Stolee can point us in the right direction.\n\nThe prepare_alt_odb() call only _adds_ to the linked odb list. It\nwill not remove any existing ODBs. Adding this reprepare_*() method\nmakes it such that we can use the union of the alternates available\nacross the lifetime of the process.\n\nThanks,\n-Stolee\n\n"},{"id":"473127","messageId":"xmqq4jqw32jh.fsf@gitster.g","threadId":"59348","inReplyTo":"901299ac-e543-b7e5-0a1a-c90e667a947d@github.com","subject":"Re: [PATCH] object-file: reprepare alternates when necessary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-07T17:16:18Z","receivedAt":"2023-03-07T17:21:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <derrickstolee@github.com> writes:\n\n> The prepare_alt_odb() call only _adds_ to the linked odb list.\n\nAh, thanks.  That was what I missed and led to the question.\n"},{"id":"473129","messageId":"xmqqttyw1ndr.fsf@gitster.g","threadId":"59348","inReplyTo":"230307.861qm0235d.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] object-file: reprepare alternates when necessary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-07T17:29:04Z","receivedAt":"2023-03-07T17:33:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> But in fact we've been doing the locking since 6c307626f1e (grep:\n> protect packed_git [re-]initialization, 2020-01-15). So the only thing\n> that really needs justification here is that putting the alternates\n> re-reading under the same lock\n>\n> There is a really interesting potential caveat here which you're not\n> discussing, which is...\n>> ...\n>> +void reprepare_alt_odb(struct repository *r)\n>> +{\n>> +\tr->objects->loaded_alternates = 0;\n>> +\tprepare_alt_odb(r);\n>> +}\n>> +\n>> ...\n> This seems reasonable, but wouldn't this do the same without introducing\n> an API function just for this one use-case?\n>\n> That's of course a nit, and you seem to have been adding this for\n> consistency with reprepare_packed_git(), but it already \"owns\" the\n> \"approximate_object_count_valid\" and \"packed_git_initialized\" flags in\n> \"struct raw_object_store\".\n>\n> So as we'll only need this from reprepare_packed_git() isn't it better\n> to declare that \"loaded_alternates\" is another such flag?\n\nI am not sure I got what you want to say 100%, but if you are saying\nthat this \"drop the 'loaded' flag and force prepare_*() function to\nredo its thing\" must not be done only in reprepare_packed_git(), and\nthat inlining the code there without introducing a helper function\nthat anybody can casually call without thinking its consequenced\nthrough, then I tend to agree in principle.  But it is just as easy\nto lift two lines of code from the rewritten/inlined code to a new\nplace---to ensure people follow the obj_read_lock() rule, the\ncomment before it may have to be a bit stronger, I wonder?\n\n> Perhaps not, but as the resulting patch is much shorter it seems worth\n> considering.\n>\n> ...but to continue the above, the *really* important thing here (and\n> correct me if I'm wrong) is that we really need to *first* prepare the\n> alternates, and *then* do the rest, as our new alternates might point to\n> new loose objects and packs.\n\nYes, and as Derrick explained in another message, we only have to\nworry about new ones getting added, not existing ones going away.\n\n> So with both of the above (the same could be done with your new helper)\n> something like this would IMO make that much clearer:\n>\n> \tdiff --git a/packfile.c b/packfile.c\n> \tindex 79e21ab18e7..50cb46ca4b7 100644\n> \t--- a/packfile.c\n> \t+++ b/packfile.c\n> \t@@ -1008,6 +1008,13 @@ void reprepare_packed_git(struct repository *r)\n> \t \tstruct object_directory *odb;\n> \t \n> \t \tobj_read_lock();\n> \t+\t/*\n> \t+\t * New alternates might point to new loose & pack dirs, so we\n> \t+\t * must first read those.\n> \t+\t */\n> \t+\tr->objects->loaded_alternates = 0;\n> \t+\tprepare_alt_odb(r);\n> \t+\n> \t \tfor (odb = r->objects->odb; odb; odb = odb->next)\n> \t \t\todb_clear_loose_cache(odb);\n>\n> And, I think this is an exsiting edge case, but we're only locking the\n> ODB of the \"parent\" repository in this case, so if we have alternates in\n> play aren't we going to racily compute the rest here, the loose objects\n> and packs of the alternates we're about to consult aren't under the same\n> lock?\n"},{"id":"473143","messageId":"xmqqjzzszaph.fsf@gitster.g","threadId":"59348","inReplyTo":"xmqqttyw1ndr.fsf@gitster.g","subject":"Re: [PATCH] object-file: reprepare alternates when necessary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-07T18:18:50Z","receivedAt":"2023-03-07T18:23:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> redo its thing\" must not be done only in reprepare_packed_git(), and\n\nOops, \"must be done only\", of course.  Sorry.\n"},{"id":"473192","messageId":"64903e15-014f-a81e-26dc-83e16b632b2b@github.com","threadId":"59348","inReplyTo":"xmqqttyw1ndr.fsf@gitster.g","subject":"Re: [PATCH] object-file: reprepare alternates when necessary","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-03-08T13:29:18Z","receivedAt":"2023-03-08T13:31:12Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/7/2023 12:29 PM, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n> \n>> But in fact we've been doing the locking since 6c307626f1e (grep:\n>> protect packed_git [re-]initialization, 2020-01-15). So the only thing\n>> that really needs justification here is that putting the alternates\n>> re-reading under the same lock\n>>\n>> There is a really interesting potential caveat here which you're not\n>> discussing, which is...\n>>> ...\n>>> +void reprepare_alt_odb(struct repository *r)\n>>> +{\n>>> +\tr->objects->loaded_alternates = 0;\n>>> +\tprepare_alt_odb(r);\n>>> +}\n>>> +\n>>> ...\n>> This seems reasonable, but wouldn't this do the same without introducing\n>> an API function just for this one use-case?\n>>\n>> That's of course a nit, and you seem to have been adding this for\n>> consistency with reprepare_packed_git(), but it already \"owns\" the\n>> \"approximate_object_count_valid\" and \"packed_git_initialized\" flags in\n>> \"struct raw_object_store\".\n>>\n>> So as we'll only need this from reprepare_packed_git() isn't it better\n>> to declare that \"loaded_alternates\" is another such flag?\n> \n> I am not sure I got what you want to say 100%, but if you are saying\n> that this \"drop the 'loaded' flag and force prepare_*() function to\n> redo its thing\" must not be done only in reprepare_packed_git(), and\n> that inlining the code there without introducing a helper function\n> that anybody can casually call without thinking its consequenced\n> through, then I tend to agree in principle.  But it is just as easy\n> to lift two lines of code from the rewritten/inlined code to a new\n> place---to ensure people follow the obj_read_lock() rule, the\n> comment before it may have to be a bit stronger, I wonder?\n\nThe fact that we do it in a lock in reprepare_packed_git() in the\nonly current caller does raise suspicion that someone could call it\nlater and not realize that the lock could be helpful. Inlining the\nchange within reprepare_packed_git() makes the most sense here\ninstead of mimicking the pattern.\n \n>> Perhaps not, but as the resulting patch is much shorter it seems worth\n>> considering.\n\nThe shortness of the patch is metric of quality, to me. The other\nreason (we might introduce a footgun) is more interesting.\n\n>> ...but to continue the above, the *really* important thing here (and\n>> correct me if I'm wrong) is that we really need to *first* prepare the\n>> alternates, and *then* do the rest, as our new alternates might point to\n>> new loose objects and packs.\n> \n> Yes, and as Derrick explained in another message, we only have to\n> worry about new ones getting added, not existing ones going away.\n\nI'll be sure to clarify that in my next version.\n\n>> So with both of the above (the same could be done with your new helper)\n>> something like this would IMO make that much clearer:\n>>\n>> \tdiff --git a/packfile.c b/packfile.c\n>> \tindex 79e21ab18e7..50cb46ca4b7 100644\n>> \t--- a/packfile.c\n>> \t+++ b/packfile.c\n>> \t@@ -1008,6 +1008,13 @@ void reprepare_packed_git(struct repository *r)\n>> \t \tstruct object_directory *odb;\n>> \t \n>> \t \tobj_read_lock();\n>> \t+\t/*\n>> \t+\t * New alternates might point to new loose & pack dirs, so we\n>> \t+\t * must first read those.\n>> \t+\t */\n>> \t+\tr->objects->loaded_alternates = 0;\n>> \t+\tprepare_alt_odb(r);\n>> \t+\n>> \t \tfor (odb = r->objects->odb; odb; odb = odb->next)\n>> \t \t\todb_clear_loose_cache(odb);\n>>\n>> And, I think this is an exsiting edge case, but we're only locking the\n>> ODB of the \"parent\" repository in this case, so if we have alternates in\n>> play aren't we going to racily compute the rest here, the loose objects\n>> and packs of the alternates we're about to consult aren't under the same\n>> lock?\n\nI don't understand what you are saying here. odb_read_lock() does not\nspecify a repository and is instead a global lock on reading from any\nobject database.\n\nHere is its implementation:\n\nextern int obj_read_use_lock;\nextern pthread_mutex_t obj_read_mutex;\n\nstatic inline void obj_read_lock(void)\n{\n\tif(obj_read_use_lock)\n\t\tpthread_mutex_lock(&obj_read_mutex);\n}\n\nstatic inline void obj_read_unlock(void)\n{\n\tif(obj_read_use_lock)\n\t\tpthread_mutex_unlock(&obj_read_mutex);\n}\n\nSo I don't believe that your proposed edge case exists.\n\nThanks,\n-Stolee\n"},{"id":"473198","messageId":"ZAiv2I17+/IBF8pl@nand.local","threadId":"59348","inReplyTo":"901299ac-e543-b7e5-0a1a-c90e667a947d@github.com","subject":"Re: [PATCH] object-file: reprepare alternates when necessary","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-03-08T15:55:04Z","receivedAt":"2023-03-08T15:56:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Mar 07, 2023 at 09:52:19AM -0500, Derrick Stolee wrote:\n> On 3/6/2023 7:28 PM, Taylor Blau wrote:\n> > On Mon, Mar 06, 2023 at 02:54:00PM -0800, Junio C Hamano wrote:\n> >> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> >>\n> >>> @@ -1008,6 +1008,7 @@ void reprepare_packed_git(struct repository *r)\n> >>>  \tstruct object_directory *odb;\n> >>>\n> >>>  \tobj_read_lock();\n> >>> +\treprepare_alt_odb(r);\n> >>>  \tfor (odb = r->objects->odb; odb; odb = odb->next)\n> >>>  \t\todb_clear_loose_cache(odb);\n> >>\n> >> Hmph, if there was an old alternate ODB from which we took some\n> >> loose object from and cached, and if that ODB no longer is on the\n> >> updated alternate list, would we now fail to clear the loose objects\n> >> cache for the ODB?  Or are we only prepared for seeing \"more\"\n> >> alternates and assume no existing alternates go away?\n> >\n> > Based on my understanding of the patch, we are only prepared to see\n> > \"more\" alternates, rather than some existing alternate going away.\n> >\n> > That being said, I am not certain that is how it works. Perhaps an\n> > alternate \"goes away\", but does not actually get removed from the list\n> > of alternate ODBs. If that's the case, any object lookup in that\n> > now-missing ODB would fail, but any subsequent ODBs which were added\n> > after calling reprepare_alt_odb() would succeed on that object lookup.\n> >\n> > So, I don't know. I don't have the implementation details of the\n> > alternates ODB mechanism paged in enough to say for sure. Hopefully\n> > Stolee can point us in the right direction.\n>\n> The prepare_alt_odb() call only _adds_ to the linked odb list. It\n> will not remove any existing ODBs. Adding this reprepare_*() method\n> makes it such that we can use the union of the alternates available\n> across the lifetime of the process.\n\nRight, that matches my understanding. What I am asking is: since we only\nadd ODBs to the list, what happens if we can no longer access an\n*existing* alternate at the time we call reprepare_alt_odb()?\n\nIt's clear that that now-inaccessible alternate remains in our list of\nalternate ODBs, but do all object lookups hitting that ODB fail-over to\nthe new ODB? I believe so, but it isn't totally clear to me.\n\nThanks,\nTaylor\n"},{"id":"473202","messageId":"87ecf011-415f-20d8-e781-18b97bfdca44@github.com","threadId":"59348","inReplyTo":"ZAiv2I17+/IBF8pl@nand.local","subject":"Re: [PATCH] object-file: reprepare alternates when necessary","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-03-08T17:13:03Z","receivedAt":"2023-03-08T17:13:13Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/8/2023 10:55 AM, Taylor Blau wrote:\n> On Tue, Mar 07, 2023 at 09:52:19AM -0500, Derrick Stolee wrote:\n\n>> The prepare_alt_odb() call only _adds_ to the linked odb list. It\n>> will not remove any existing ODBs. Adding this reprepare_*() method\n>> makes it such that we can use the union of the alternates available\n>> across the lifetime of the process.\n> \n> Right, that matches my understanding. What I am asking is: since we only\n> add ODBs to the list, what happens if we can no longer access an\n> *existing* alternate at the time we call reprepare_alt_odb()?\n> \n> It's clear that that now-inaccessible alternate remains in our list of\n> alternate ODBs, but do all object lookups hitting that ODB fail-over to\n> the new ODB? I believe so, but it isn't totally clear to me.\n\nIt's the same as the pack-file list: if we fail to load something\nfrom one, then we continue to the next one. If an alternate dir\nis completely removed during the process, then looking for pack-\nfiles again will fail to see any and continue without error.\n\nThis is already possible by deleting an alternate directory\nwhile a Git process is running and might try to open files in it.\nGit already recovers from this scenario.\n\nIf you're instead talking about the .git/objects/info/alternates\nfile being modified to remove an alternate from the list, then\nGit's current behavior is to keep that alternate around for the\nlife of the process, and I recommend continuing that behavior.\n\nThere's nothing special that we are adding here that doesn't\nalready exist as protections when files are removed beneath the\nGit process.\n\nThanks,\n-Stolee\n"},{"id":"473209","messageId":"pull.1490.v2.git.1678301252360.gitgitgadget@gmail.com","threadId":"59348","inReplyTo":"pull.1490.git.1678136369387.gitgitgadget@gmail.com","subject":"[PATCH v2] object-file: reprepare alternates when necessary","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-03-08T18:47:32Z","receivedAt":"2023-03-08T18:47:39Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <derrickstolee@github.com>\n\nWhen an object is not found in a repository's object store, we sometimes\ncall reprepare_packed_git() to see if the object was temporarily moved\ninto a new pack-file (and its old pack-file or loose object was\ndeleted). This process does a scan of each pack directory within each\nodb, but does not reevaluate if the odb list needs updating.\n\nExtend reprepare_packed_git() to also reprepate the alternate odb list\nby setting loaded_alternates to zero and calling prepare_alt_odb(). This\nwill add newly-discoverd odbs to the linked list, but will not duplicate\nexisting ones nor will it remove existing ones that are no longer listed\nin the alternates file. Do this under the object read lock to avoid\nreaders from interacting with a potentially incomplete odb being added\nto the odb list.\n\nIf the alternates file was edited to _remove_ some alternates during the\ncourse of the Git process, Git will continue to see alternates that were\never valid for that repository. ODBs are not removed from the list, the\nsame as the existing behavior before this change. Git already has\nprotections against an alternate directory disappearing from the\nfilesystem during the lifetime of a process, and those are still in\neffect.\n\nThis change is specifically for concurrent changes to the repository, so\nit is difficult to create a test that guarantees this behavior is\ncorrect. I manually verified by introducing a reprepare_packed_git() call\ninto get_revision() and stepped into that call in a debugger with a\nparent 'git log' process. Multiple runs of prepare_alt_odb() kept\nthe_repository->objects->odb as a single-item chain until I added a\n.git/objects/info/alternates file in a different process. The next run\nadded the new odb to the chain and subsequent runs did not add to the\nchain.\n\nSigned-off-by: Derrick Stolee <derrickstolee@github.com>\n---\n    object-file: reprepare alternates when necessary\n    \n    This subtlety was notice by Michael Haggerty due to how alternates are\n    used server-side at $DAYJOB. Moving pack-files from a repository to the\n    alternate occasionally causes failures because processes that start\n    before the alternate exists don't know how to find that alternate at\n    run-time.\n    \n    \n    Update in v2\n    ============\n    \n     * Instead of creating a new public reprepare_alt_odb() method, inline\n       its implementation inside reprepare_packed_git().\n     * Update commit message to be explicit about behavior with alternates\n       being removed from the alternates file or from disk.\n    \n    Thanks,\n    \n     * Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1490%2Fderrickstolee%2Fstolee%2Freprepare-alternates-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1490/derrickstolee/stolee/reprepare-alternates-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1490\n\nRange-diff vs v1:\n\n 1:  3f42c9cdef7 ! 1:  9a5e1d9a9da object-file: reprepare alternates when necessary\n     @@ Commit message\n          deleted). This process does a scan of each pack directory within each\n          odb, but does not reevaluate if the odb list needs updating.\n      \n     -    Create a new reprepare_alt_odb() method that is a similar wrapper around\n     -    prepare_alt_odb(). Call it from reprepare_packed_git() under the object\n     -    read lock to avoid readers from interacting with a potentially\n     -    incomplete odb being added to the odb list.\n     -\n     -    prepare_alt_odb() already avoids adding duplicate odbs to the list\n     -    during its progress, so it is safe to call it again from\n     -    reprepare_alt_odb() without worrying about duplicate odbs.\n     +    Extend reprepare_packed_git() to also reprepate the alternate odb list\n     +    by setting loaded_alternates to zero and calling prepare_alt_odb(). This\n     +    will add newly-discoverd odbs to the linked list, but will not duplicate\n     +    existing ones nor will it remove existing ones that are no longer listed\n     +    in the alternates file. Do this under the object read lock to avoid\n     +    readers from interacting with a potentially incomplete odb being added\n     +    to the odb list.\n     +\n     +    If the alternates file was edited to _remove_ some alternates during the\n     +    course of the Git process, Git will continue to see alternates that were\n     +    ever valid for that repository. ODBs are not removed from the list, the\n     +    same as the existing behavior before this change. Git already has\n     +    protections against an alternate directory disappearing from the\n     +    filesystem during the lifetime of a process, and those are still in\n     +    effect.\n      \n          This change is specifically for concurrent changes to the repository, so\n          it is difficult to create a test that guarantees this behavior is\n          correct. I manually verified by introducing a reprepare_packed_git() call\n          into get_revision() and stepped into that call in a debugger with a\n     -    parent 'git log' process. Multiple runs of reprepare_alt_odb() kept\n     +    parent 'git log' process. Multiple runs of prepare_alt_odb() kept\n          the_repository->objects->odb as a single-item chain until I added a\n          .git/objects/info/alternates file in a different process. The next run\n          added the new odb to the chain and subsequent runs did not add to the\n     @@ Commit message\n      \n          Signed-off-by: Derrick Stolee <derrickstolee@github.com>\n      \n     - ## object-file.c ##\n     -@@ object-file.c: void prepare_alt_odb(struct repository *r)\n     - \tr->objects->loaded_alternates = 1;\n     - }\n     - \n     -+void reprepare_alt_odb(struct repository *r)\n     -+{\n     -+\tr->objects->loaded_alternates = 0;\n     -+\tprepare_alt_odb(r);\n     -+}\n     -+\n     - /* Returns 1 if we have successfully freshened the file, 0 otherwise. */\n     - static int freshen_file(const char *fn)\n     - {\n     -\n     - ## object-store.h ##\n     -@@ object-store.h: KHASH_INIT(odb_path_map, const char * /* key: odb_path */,\n     - \tstruct object_directory *, 1, fspathhash, fspatheq)\n     - \n     - void prepare_alt_odb(struct repository *r);\n     -+void reprepare_alt_odb(struct repository *r);\n     - char *compute_alternate_path(const char *path, struct strbuf *err);\n     - struct object_directory *find_odb(struct repository *r, const char *obj_dir);\n     - typedef int alt_odb_fn(struct object_directory *, void *);\n     -\n       ## packfile.c ##\n      @@ packfile.c: void reprepare_packed_git(struct repository *r)\n       \tstruct object_directory *odb;\n       \n       \tobj_read_lock();\n     -+\treprepare_alt_odb(r);\n     ++\n     ++\t/*\n     ++\t * Reprepare alt odbs, in case the alternates file was modified\n     ++\t * during the course of this process. This only _adds_ odbs to\n     ++\t * the linked list, so existing odbs will continue to exist for\n     ++\t * the lifetime of the process.\n     ++\t */\n     ++\tr->objects->loaded_alternates = 0;\n     ++\tprepare_alt_odb(r);\n     ++\n       \tfor (odb = r->objects->odb; odb; odb = odb->next)\n       \t\todb_clear_loose_cache(odb);\n       \n\n\n packfile.c | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/packfile.c b/packfile.c\nindex 79e21ab18e7..06419c8e8ca 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1008,6 +1008,16 @@ void reprepare_packed_git(struct repository *r)\n \tstruct object_directory *odb;\n \n \tobj_read_lock();\n+\n+\t/*\n+\t * Reprepare alt odbs, in case the alternates file was modified\n+\t * during the course of this process. This only _adds_ odbs to\n+\t * the linked list, so existing odbs will continue to exist for\n+\t * the lifetime of the process.\n+\t */\n+\tr->objects->loaded_alternates = 0;\n+\tprepare_alt_odb(r);\n+\n \tfor (odb = r->objects->odb; odb; odb = odb->next)\n \t\todb_clear_loose_cache(odb);\n \n\nbase-commit: d15644fe0226af7ffc874572d968598564a230dd\n-- \ngitgitgadget\n"},{"id":"473216","messageId":"xmqqedpzt4t1.fsf@gitster.g","threadId":"59348","inReplyTo":"pull.1490.v2.git.1678301252360.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] object-file: reprepare alternates when necessary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-08T19:35:06Z","receivedAt":"2023-03-08T19:35:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Derrick Stolee <derrickstolee@github.com>\n>\n> When an object is not found in a repository's object store, we sometimes\n> call reprepare_packed_git() to see if the object was temporarily moved\n> into a new pack-file (and its old pack-file or loose object was\n> deleted). This process does a scan of each pack directory within each\n> odb, but does not reevaluate if the odb list needs updating.\n\nVery well explained, except I do not know what is meant by\n\"temporarily\", which to me implies what was moved is moved back\nlater.  Without \"temporarily\", it reads perfectly well.  While I do\nnot think of a good word to use to tell the readers that the moving\nis done by somebody else while we are trying to find the object, if\nthere were one, that would replace \"temporarily\" very well.\n\nBut other than that tiny nit that does not have to be addressed at\nall, everything in this patch looks good.\n\nThanks.\n"},{"id":"473222","messageId":"ZAj0f906l/7xGFAl@nand.local","threadId":"59348","inReplyTo":"xmqqedpzt4t1.fsf@gitster.g","subject":"Re: [PATCH v2] object-file: reprepare alternates when necessary","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-03-08T20:47:59Z","receivedAt":"2023-03-08T20:48:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Mar 08, 2023 at 11:35:06AM -0800, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> > From: Derrick Stolee <derrickstolee@github.com>\n> >\n> > When an object is not found in a repository's object store, we sometimes\n> > call reprepare_packed_git() to see if the object was temporarily moved\n> > into a new pack-file (and its old pack-file or loose object was\n> > deleted). This process does a scan of each pack directory within each\n> > odb, but does not reevaluate if the odb list needs updating.\n>\n> Very well explained, except I do not know what is meant by\n> \"temporarily\", which to me implies what was moved is moved back\n> later.  Without \"temporarily\", it reads perfectly well.  While I do\n> not think of a good word to use to tell the readers that the moving\n> is done by somebody else while we are trying to find the object, if\n> there were one, that would replace \"temporarily\" very well.\n>\n> But other than that tiny nit that does not have to be addressed at\n> all, everything in this patch looks good.\n\ns/temporarily/concurrently?\n\nI think that concurrently conveys that another process was doing the\nmoving. But I agree that it doesn't matter here to aid in the overall\nunderstanding of what's happening in this patch.\n\nSo I'd be fine with this patch as-is, with s/temporarily/concurrently,\nor with s/temporarily//.\n\nThanks,\nTaylor\n"},{"id":"473265","messageId":"ZAmJtnLgwimRBGTb@coredump.intra.peff.net","threadId":"59348","inReplyTo":"pull.1490.v2.git.1678301252360.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] object-file: reprepare alternates when necessary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-09T07:24:38Z","receivedAt":"2023-03-09T07:24:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 08, 2023 at 06:47:32PM +0000, Derrick Stolee via GitGitGadget wrote:\n\n> Extend reprepare_packed_git() to also reprepate the alternate odb list\n\ns/reprepate/reprepare/\n\nUnless you were inventing a new word. :)\n\n> This change is specifically for concurrent changes to the repository, so\n> it is difficult to create a test that guarantees this behavior is\n> correct. I manually verified by introducing a reprepare_packed_git() call\n> into get_revision() and stepped into that call in a debugger with a\n> parent 'git log' process. Multiple runs of prepare_alt_odb() kept\n> the_repository->objects->odb as a single-item chain until I added a\n> .git/objects/info/alternates file in a different process. The next run\n> added the new odb to the chain and subsequent runs did not add to the\n> chain.\n\nOne thing that test wouldn't cover is loading alternates from\n$GIT_ALTERNATE_OBJECT_DIRECTORIES. Once upon a time, I think we were\npretty inconsistent in finding duplicates. But these days it looks like\nit's all done centrally in link_alt_odb_entry(), via alt_odb_usable().\nSo it should be good.\n\n>     object-file: reprepare alternates when necessary\n>     \n>     This subtlety was notice by Michael Haggerty due to how alternates are\n>     used server-side at $DAYJOB. Moving pack-files from a repository to the\n>     alternate occasionally causes failures because processes that start\n>     before the alternate exists don't know how to find that alternate at\n>     run-time.\n\nYeah, I don't think there's any real reason not to do this. It simply\ndoesn't come up in the same way that packfile-repreparing does, because\nthere it is routine for a concurrent gc to remove an existing packfile\nand add the objects elsewhere.\n\nWhereas modifying alternates was never seen as something that would\nhappen often or automatically. I guess in GitHub's case it is from\nconverting a repository from stand-alone into a fork, migrating its\nobjects to a shared one, and adding an alternate.\n\nThe only downside might be performance. For sane cases, I think scanning\nthe new alternates is OK. I know Eric (cc'd) has some crazy\n100k-alternate setup (from 407532f82d, etc), but I'd expect a reprepare\nthere is already expensive (we already have to re-scan every one of\nthose directories for packfiles, and throw out any loose object caches).\n\nThe patch itself looks good to me (and I agree with the sentiment to\njust inline the lines rather than adding a function).\n\n-Peff\n"},{"id":"473267","messageId":"20230309090608.M92573@dcvr","threadId":"59348","inReplyTo":"ZAmJtnLgwimRBGTb@coredump.intra.peff.net","subject":"Re: [PATCH v2] object-file: reprepare alternates when necessary","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2023-03-09T09:06:08Z","receivedAt":"2023-03-09T09:07:16Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jeff King <peff@peff.net> wrote:\n> The only downside might be performance. For sane cases, I think scanning\n> the new alternates is OK. I know Eric (cc'd) has some crazy\n> 100k-alternate setup (from 407532f82d, etc), but I'd expect a reprepare\n> there is already expensive (we already have to re-scan every one of\n> those directories for packfiles, and throw out any loose object caches).\n\nI'm not sure if that 100k alternate thing is happening, yet...\n(initial specs called for ~30k, but I figured it might grow)\n\nIf it does, I'm thinking about enhancing --batch-command, to support\n`add-alternate' to dynamically add alternates while running cat-file.\n\nRight now, my biggest use case is only 250 alternates or so.\n"},{"id":"473384","messageId":"20230310212916.4138690-1-jonathantanmy@google.com","threadId":"59348","inReplyTo":"pull.1490.v2.git.1678301252360.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] object-file: reprepare alternates when necessary","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2023-03-10T21:29:16Z","receivedAt":"2023-03-10T21:30:18Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>     Update in v2\n>     ============\n>     \n>      * Instead of creating a new public reprepare_alt_odb() method, inline\n>        its implementation inside reprepare_packed_git().\n\n[snip]\n\n> diff --git a/packfile.c b/packfile.c\n> index 79e21ab18e7..06419c8e8ca 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -1008,6 +1008,16 @@ void reprepare_packed_git(struct repository *r)\n>  \tstruct object_directory *odb;\n>  \n>  \tobj_read_lock();\n> +\n> +\t/*\n> +\t * Reprepare alt odbs, in case the alternates file was modified\n> +\t * during the course of this process. This only _adds_ odbs to\n> +\t * the linked list, so existing odbs will continue to exist for\n> +\t * the lifetime of the process.\n> +\t */\n> +\tr->objects->loaded_alternates = 0;\n> +\tprepare_alt_odb(r);\n\nI understand that the change to inline what was originally\nreprepare_alt_odb() was in response to a reviewer comment, but I would\nprefer the original version, since this assumes that prepare_alt_odb()\nonly adds to what's there instead of first clearing the odb linked list\nand odb_by_path hashmap.\n\nBut I guess clearing a linked list and hashmap can be a bit cumbersome\nin C, so maybe it would be reasonable to assume that this behavior\nwould not change. In any case, maybe a comment should be added\nto prepare_alt_odb() saying that if an update of the alternates\nis needed, one can do so by clearing loaded_alternates and re-\ninvoking prepare_alt_odb() (at least so that a developer changing\nprepare_alt_odb() can see the comment and understand what behaviors this\nfunction needs to preserve).\n\nEverything else (the new functionality, the commit message etc.) looks\ngood, of course.\n\n"},{"id":"473393","messageId":"xmqqilf8yx3k.fsf@gitster.g","threadId":"59348","inReplyTo":"20230310212916.4138690-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] object-file: reprepare alternates when necessary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-11T00:01:51Z","receivedAt":"2023-03-11T00:01:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> But I guess clearing a linked list and hashmap can be a bit cumbersome\n> in C, so maybe it would be reasonable to assume that this behavior\n> would not change.\n\nI think the original reason why we did not clear was because we\nnever knew (and we do not know) what is still in use.  Can anybody\nexplain why it would be a safe thing to do to rebuild the alt-odb\nlist from scratch?  Surely we can have a big central lock to do the\n\"list manipulation\" part safely, but is it safe to access the\nobjects we obtained from one of the odb's that no longer is listed\non the alt-odb list, for example?  The code that this patch touches\nclears the loose object cache after updating the odb list, so loose\nobject cache of any odb that goes away will not be cleared here,\nwhich means that the code that reconstruts alt-odb list would need\nto clear the loose object cache automatically?  What should we do to\npackfiles that are mmaped from these alt-odbs that goes away?  Etc.\netc.\n\n> In any case, maybe a comment should be added to prepare_alt_odb()\n> saying that if an update of the alternates is needed, one can do\n> so by clearing loaded_alternates and re- invoking\n> prepare_alt_odb() (at least so that a developer changing\n> prepare_alt_odb() can see the comment and understand what\n> behaviors this function needs to preserve).\n\nSounds good.\n\nThanks.\n\n"},{"id":"473396","messageId":"20230311030923.4171623-1-jonathantanmy@google.com","threadId":"59348","inReplyTo":"xmqqilf8yx3k.fsf@gitster.g","subject":"Re: [PATCH v2] object-file: reprepare alternates when necessary","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2023-03-11T03:09:23Z","receivedAt":"2023-03-11T03:09:30Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> writes:\n> Jonathan Tan <jonathantanmy@google.com> writes:\n> \n> > But I guess clearing a linked list and hashmap can be a bit cumbersome\n> > in C, so maybe it would be reasonable to assume that this behavior\n> > would not change.\n> \n> I think the original reason why we did not clear was because we\n> never knew (and we do not know) what is still in use.  \n\nAh, I meant that prepare_alt_odb() assumes that the data structures\nare as they were originally initialized. So I can think of at least\n2 ways that we could implement prepare_alt_odb():\n\n (a) Repeatedly modify r->objects->odb_tail and repeatedly assign to\n     kh_value(r->objects->odb_by_path, pos).\n (b) Call make_odb(), which returns a pointer that we assign to\n     r->objects->odb, and call make_odb_hashmap(), which returns a\n     pointer that we assign to r->objects->odb_by_path.\n\nRemember that prepare_alt_odb() is meant to start from scratch, so it is\nreasonable to do either.\n\nIn Java, for example, it would be very reasonable to do (b). But\nthis patch assumes that we have implemented it as (a). Indeed we\nhave implemented it as (a) (although we have never documented it as\nsuch), and my quoted statement above is meant to say that it might be\nreasonable to keep assuming that it would be (a).\n"}]}