{"thread":{"id":"62588","subject":"[PATCH 0/3] Performance improvements for repacking non-promisor objects","startedAt":"2024-12-02T20:18:44Z","lastAt":"2024-12-09T23:51:11Z","messageCount":29,"participants":["Jonathan Tan","Josh Steadmon","Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"508457","messageId":"cover.1733170252.git.jonathantanmy@google.com","threadId":"62588","inReplyTo":null,"subject":"[PATCH 0/3] Performance improvements for repacking non-promisor objects","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-12-02T20:18:37Z","receivedAt":"2024-12-02T20:18:44Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This is a follow-up to jt/repack-local-promisor [1] (but these patches\nare based on master, since that branch has already been merged).\n\nThese patches speed up a fetch that takes 7 hours to take under 3\nminutes. More details are in the commit messages, especially that of\npatch 1.\n\nThanks in advance to everyone who reviews. While review is going on,\nwe'll also be testing these at $DAYJOB (I've tested it to work on one\nknown big repo, but there may be others).\n\n[1] https://lore.kernel.org/git/cover.1730491845.git.jonathantanmy@google.com/\n\nJonathan Tan (3):\n index-pack: dedup first during outgoing link check\n index-pack: no blobs during outgoing link check\n index-pack: commit tree during outgoing link check\n\n builtin/index-pack.c | 49 +++++++++++++++++++++++---------------------\n 1 file changed, 26 insertions(+), 23 deletions(-)\n\n-- \n2.47.0.338.g60cca15819-goog\n\n"},{"id":"508458","messageId":"5f0f114dbdf00fe246308490f09b649bd8de242c.1733170252.git.jonathantanmy@google.com","threadId":"62588","inReplyTo":"cover.1733170252.git.jonathantanmy@google.com","subject":"[PATCH 1/3] index-pack: dedup first during outgoing link check","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-12-02T20:18:38Z","receivedAt":"2024-12-02T20:18:46Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Commit c08589efdc (index-pack: repack local links into promisor packs,\n2024-11-01) fixed a bug with what was believed to be a negligible\ndecrease in performance [1] [2]. But at $DAYJOB, with at least one repo,\nit was found that the decrease in performance was very significant.\n\nLooking at the patch, whenever we parse an object in the packfile to\nbe indexed, we check the targets of all its outgoing links for its\nexistence. However, this could be optimized by first collecting all such\ntargets into an oidset (thus deduplicating them) before checking. Teach\nGit to do that.\n\nOn a certain fetch from the aforementioned repo, this improved\nperformance from approximately 7 hours to 24m47.815s. This number will\nbe further reduced in a subsequent patch.\n\n[1] https://lore.kernel.org/git/CAG1j3zGiNMbri8rZNaF0w+yP+6OdMz0T8+8_Wgd1R_p1HzVasg@mail.gmail.com/\n[2] https://lore.kernel.org/git/20241105212849.3759572-1-jonathantanmy@google.com/\n\nSigned-off-by: Jonathan Tan \n---\n builtin/index-pack.c | 44 ++++++++++++++++++++++----------------------\n 1 file changed, 22 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 95babdc5ea..8e7d14c17e 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -155,11 +155,11 @@ static int input_fd, output_fd;\n static const char *curr_pack;\n \n /*\n- * local_links is guarded by read_mutex, and record_local_links is read-only in\n- * a thread.\n+ * outgoing_links is guarded by read_mutex, and record_outgoing_links is\n+ * read-only in a thread.\n */\n-static struct oidset local_links = OIDSET_INIT;\n-static int record_local_links;\n+static struct oidset outgoing_links = OIDSET_INIT;\n+static int record_outgoing_links;\n \n static struct thread_local_data *thread_data;\n static int nr_dispatched;\n@@ -812,18 +812,12 @@ static int check_collison(struct object_entry *entry)\n \treturn 0;\n }\n \n-static void record_if_local_object(const struct object_id *oid)\n+static void record_outgoing_link(const struct object_id *oid)\n {\n-\tstruct object_info info = OBJECT_INFO_INIT;\n-\tif (oid_object_info_extended(the_repository, oid, &info, 0))\n-\t\t/* Missing; assume it is a promisor object */\n-\t\treturn;\n-\tif (info.whence == OI_PACKED && info.u.packed.pack->pack_promisor)\n-\t\treturn;\n-\toidset_insert(&local_links, oid);\n+\toidset_insert(&outgoing_links, oid);\n }\n \n-static void do_record_local_links(struct object *obj)\n+static void do_record_outgoing_links(struct object *obj)\n {\n \tif (obj->type == OBJ_TREE) {\n \t\tstruct tree *tree = (struct tree *)obj;\n@@ -837,16 +831,16 @@ static void do_record_local_links(struct object *obj)\n \t\t\t */\n \t\t\treturn;\n \t\twhile (tree_entry_gently(&desc, &entry))\n-\t\t\trecord_if_local_object(&entry.oid);\n+\t\t\trecord_outgoing_link(&entry.oid);\n \t} else if (obj->type == OBJ_COMMIT) {\n \t\tstruct commit *commit = (struct commit *) obj;\n \t\tstruct commit_list *parents = commit->parents;\n \n \t\tfor (; parents; parents = parents->next)\n-\t\t\trecord_if_local_object(&parents->item->object.oid);\n+\t\t\trecord_outgoing_link(&parents->item->object.oid);\n \t} else if (obj->type == OBJ_TAG) {\n \t\tstruct tag *tag = (struct tag *) obj;\n-\t\trecord_if_local_object(get_tagged_oid(tag));\n+\t\trecord_outgoing_link(get_tagged_oid(tag));\n \t}\n }\n \n@@ -896,7 +890,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\tfree(has_data);\n \t}\n \n-\tif (strict || do_fsck_object || record_local_links) {\n+\tif (strict || do_fsck_object || record_outgoing_links) {\n \t\tread_lock();\n \t\tif (type == OBJ_BLOB) {\n \t\t\tstruct blob *blob = lookup_blob(the_repository, oid);\n@@ -928,8 +922,8 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\t\tdie(_(\"fsck error in packed object\"));\n \t\t\tif (strict && fsck_walk(obj, NULL, &fsck_options))\n \t\t\t\tdie(_(\"Not all child objects of %s are reachable\"), oid_to_hex(&obj->oid));\n-\t\t\tif (record_local_links)\n-\t\t\t\tdo_record_local_links(obj);\n+\t\t\tif (record_outgoing_links)\n+\t\t\t\tdo_record_outgoing_links(obj);\n \n \t\t\tif (obj->type == OBJ_TREE) {\n \t\t\t\tstruct tree *item = (struct tree *) obj;\n@@ -1781,7 +1775,7 @@ static void repack_local_links(void)\n \tstruct object_id *oid;\n \tchar *base_name;\n \n-\tif (!oidset_size(&local_links))\n+\tif (!oidset_size(&outgoing_links))\n \t\treturn;\n \n \tbase_name = mkpathdup(\"%s/pack/pack\", repo_get_object_directory(the_repository));\n@@ -1795,8 +1789,14 @@ static void repack_local_links(void)\n \tif (start_command(&cmd))\n \t\tdie(_(\"could not start pack-objects to repack local links\"));\n \n-\toidset_iter_init(&local_links, &iter);\n+\toidset_iter_init(&outgoing_links, &iter);\n \twhile ((oid = oidset_iter_next(&iter))) {\n+\t\tstruct object_info info = OBJECT_INFO_INIT;\n+\t\tif (oid_object_info_extended(the_repository, oid, &info, 0))\n+\t\t\t/* Missing; assume it is a promisor object */\n+\t\t\tcontinue;\n+\t\tif (info.whence == OI_PACKED && info.u.packed.pack->pack_promisor)\n+\t\t\tcontinue;\n \t\tif (write_in_full(cmd.in, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||\n \t\t write_in_full(cmd.in, \"\\n\", 1) < 0)\n \t\t\tdie(_(\"failed to feed local object to pack-objects\"));\n@@ -1899,7 +1899,7 @@ int cmd_index_pack(int argc,\n \t\t\t} else if (skip_to_optional_arg(arg, \"--keep\", &keep_msg)) {\n \t\t\t\t; /* nothing to do */\n \t\t\t} else if (skip_to_optional_arg(arg, \"--promisor\", &promisor_msg)) {\n-\t\t\t\trecord_local_links = 1;\n+\t\t\t\trecord_outgoing_links = 1;\n \t\t\t} else if (starts_with(arg, \"--threads=\")) {\n \t\t\t\tchar *end;\n \t\t\t\tnr_threads = strtoul(arg+10, &end, 0);\n-- \n2.47.0.338.g60cca15819-goog\n\n"},{"id":"508459","messageId":"300f53b8e39fa1dd55f65924d20f8abd22cbbfc9.1733170252.git.jonathantanmy@google.com","threadId":"62588","inReplyTo":"cover.1733170252.git.jonathantanmy@google.com","subject":"[PATCH 2/3] index-pack: no blobs during outgoing link check","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-12-02T20:18:39Z","receivedAt":"2024-12-02T20:18:48Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"As a follow-up to the parent of this commit, it was found that not\nchecking for the existence of blobs linked from trees sped up the fetch\nfrom 24m47.815s to 2m2.127s. Teach Git to do that.\n\nThe benefit of doing this is as above (fetch speedup), but the drawback\nis that if the packfile to be indexed references a local blob directly\n(that is, not through a local tree), that local blob is in danger of\nbeing garbage collected. Such a situation may arise if we push local\ncommits, including one with a change to a blob in the root tree,\nand then the server incorporates them into its main branch through a\n\"rebase\" or \"squash\" merge strategy, and then we fetch the new main\nbranch from the server.\n\nThis situation has not been observed yet - we have only noticed missing\ncommits, not missing trees or blobs. (In fact, if it were believed that\nonly missing commits are problematic, one could argue that we should\nalso exclude trees during the outgoing link check; but it is safer to\ninclude them.)\n\nDue to the rarity of the situation (it has not been observed to happen\nin real life), and because the \"penalty\" in such a situation is merely\nto refetch the missing blob when it's needed, the tradeoff seems\nworth it.\n\n(Blobs may also be linked from tag objects, but it is impossible to know\nthe type of an object linked from a tag object without looking it up in\nthe object database, so the code for that is untouched.)\n\nSigned-off-by: Jonathan Tan \n---\n builtin/index-pack.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 8e7d14c17e..58d24540dc 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -830,8 +830,10 @@ static void do_record_outgoing_links(struct object *obj)\n \t\t\t * verified, so do not print any here.\n \t\t\t */\n \t\t\treturn;\n-\t\twhile (tree_entry_gently(&desc, &entry))\n-\t\t\trecord_outgoing_link(&entry.oid);\n+\t\twhile (tree_entry_gently(&desc, &entry)) {\n+\t\t\tif (S_ISDIR(entry.mode))\n+\t\t\t\trecord_outgoing_link(&entry.oid);\n+\t\t}\n \t} else if (obj->type == OBJ_COMMIT) {\n \t\tstruct commit *commit = (struct commit *) obj;\n \t\tstruct commit_list *parents = commit->parents;\n-- \n2.47.0.338.g60cca15819-goog\n\n"},{"id":"508460","messageId":"2f2f0db78bf85c14ef132e1924ab5021298aace3.1733170252.git.jonathantanmy@google.com","threadId":"62588","inReplyTo":"cover.1733170252.git.jonathantanmy@google.com","subject":"[PATCH 3/3] index-pack: commit tree during outgoing link check","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-12-02T20:18:40Z","receivedAt":"2024-12-02T20:18:50Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Commit c08589efdc (index-pack: repack local links into promisor packs,\n2024-11-01) seems to contain an oversight in that the tree of a commit\nis not checked. The fix slows down a fetch from a certain repo at\n$DAYJOB from 2m2.127s to 2m45.052s, but in order to make the fetch\ncorrect, it seems worth it.\n\nIn order to test this, we could create server and client repos as\nfollows...\n\n C S\n \\ /\n O\n\n(O and C are commits both on the client and server. S is a commit\nonly on the server. C and S have the same tree but different commit\nmessages.)\n\n...and then, from the client, fetch S from the server.\n\nIn theory, the client declares \"have C\" and the server can use this\ninformation to exclude S's tree (since it knows that the client has C's\ntree, which is the same as S's tree). However, it is also possible for\nthe server to compute that it needs to send S and not O, and proceed\nfrom there; therefore the objects of C are not considered at all when\ndetermining what to send in the packfile. In order to prevent a test of\nclient functionality from having such a dependence on server behavior, I\nhave not included such a test.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/index-pack.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 58d24540dc..338aeeadc8 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -838,6 +838,7 @@ static void do_record_outgoing_links(struct object *obj)\n \t\tstruct commit *commit = (struct commit *) obj;\n \t\tstruct commit_list *parents = commit->parents;\n \n+\t\trecord_outgoing_link(get_commit_tree_oid(commit));\n \t\tfor (; parents; parents = parents->next)\n \t\t\trecord_outgoing_link(&parents->item->object.oid);\n \t} else if (obj->type == OBJ_TAG) {\n-- \n2.47.0.338.g60cca15819-goog\n\n"},{"id":"508471","messageId":"p5ctagmbwo6qp7ocozx4chitw7byy6yjma4unoebbrfr365ywp@jkdlk3g7fchz","threadId":"62588","inReplyTo":"5f0f114dbdf00fe246308490f09b649bd8de242c.1733170252.git.jonathantanmy@google.com","subject":"Re: [PATCH 1/3] index-pack: dedup first during outgoing link check","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-12-02T21:24:52Z","receivedAt":"2024-12-02T21:24:58Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.12.02 12:18, Jonathan Tan wrote:\n> Commit c08589efdc (index-pack: repack local links into promisor packs,\n> 2024-11-01) fixed a bug with what was believed to be a negligible\n> decrease in performance [1] [2]. But at $DAYJOB, with at least one repo,\n> it was found that the decrease in performance was very significant.\n> \n> Looking at the patch, whenever we parse an object in the packfile to\n> be indexed, we check the targets of all its outgoing links for its\n> existence. However, this could be optimized by first collecting all such\n> targets into an oidset (thus deduplicating them) before checking. Teach\n> Git to do that.\n> \n> On a certain fetch from the aforementioned repo, this improved\n> performance from approximately 7 hours to 24m47.815s. This number will\n> be further reduced in a subsequent patch.\n> \n> [1] https://lore.kernel.org/git/CAG1j3zGiNMbri8rZNaF0w+yP+6OdMz0T8+8_Wgd1R_p1HzVasg@mail.gmail.com/\n> [2] https://lore.kernel.org/git/20241105212849.3759572-1-jonathantanmy@google.com/\n> \n> Signed-off-by: Jonathan Tan \n> ---\n> builtin/index-pack.c | 44 ++++++++++++++++++++++----------------------\n> 1 file changed, 22 insertions(+), 22 deletions(-)\n> \n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index 95babdc5ea..8e7d14c17e 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -155,11 +155,11 @@ static int input_fd, output_fd;\n> static const char *curr_pack;\n> \n> /*\n> - * local_links is guarded by read_mutex, and record_local_links is read-only in\n> - * a thread.\n> + * outgoing_links is guarded by read_mutex, and record_outgoing_links is\n> + * read-only in a thread.\n> */\n> -static struct oidset local_links = OIDSET_INIT;\n> -static int record_local_links;\n> +static struct oidset outgoing_links = OIDSET_INIT;\n> +static int record_outgoing_links;\n> \n> static struct thread_local_data *thread_data;\n> static int nr_dispatched;\n\nWe're renaming the oidset and flag because our purpose is now more\ngeneral. OK.\n\n\n> @@ -812,18 +812,12 @@ static int check_collison(struct object_entry *entry)\n> \treturn 0;\n> }\n> \n> -static void record_if_local_object(const struct object_id *oid)\n> +static void record_outgoing_link(const struct object_id *oid)\n> {\n> -\tstruct object_info info = OBJECT_INFO_INIT;\n> -\tif (oid_object_info_extended(the_repository, oid, &info, 0))\n> -\t\t/* Missing; assume it is a promisor object */\n> -\t\treturn;\n> -\tif (info.whence == OI_PACKED && info.u.packed.pack->pack_promisor)\n> -\t\treturn;\n> -\toidset_insert(&local_links, oid);\n> +\toidset_insert(&outgoing_links, oid);\n> }\n\nWe're now unconditionally recording linked objects, and this logic has\nbeen moved below. Looks good.\n\n\n> -static void do_record_local_links(struct object *obj)\n> +static void do_record_outgoing_links(struct object *obj)\n> {\n> \tif (obj->type == OBJ_TREE) {\n> \t\tstruct tree *tree = (struct tree *)obj;\n> @@ -837,16 +831,16 @@ static void do_record_local_links(struct object *obj)\n> \t\t\t */\n> \t\t\treturn;\n> \t\twhile (tree_entry_gently(&desc, &entry))\n> -\t\t\trecord_if_local_object(&entry.oid);\n> +\t\t\trecord_outgoing_link(&entry.oid);\n> \t} else if (obj->type == OBJ_COMMIT) {\n> \t\tstruct commit *commit = (struct commit *) obj;\n> \t\tstruct commit_list *parents = commit->parents;\n> \n> \t\tfor (; parents; parents = parents->next)\n> -\t\t\trecord_if_local_object(&parents->item->object.oid);\n> +\t\t\trecord_outgoing_link(&parents->item->object.oid);\n> \t} else if (obj->type == OBJ_TAG) {\n> \t\tstruct tag *tag = (struct tag *) obj;\n> -\t\trecord_if_local_object(get_tagged_oid(tag));\n> +\t\trecord_outgoing_link(get_tagged_oid(tag));\n> \t}\n> }\n> \n> @@ -896,7 +890,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n> \t\tfree(has_data);\n> \t}\n> \n> -\tif (strict || do_fsck_object || record_local_links) {\n> +\tif (strict || do_fsck_object || record_outgoing_links) {\n> \t\tread_lock();\n> \t\tif (type == OBJ_BLOB) {\n> \t\t\tstruct blob *blob = lookup_blob(the_repository, oid);\n> @@ -928,8 +922,8 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n> \t\t\t\tdie(_(\"fsck error in packed object\"));\n> \t\t\tif (strict && fsck_walk(obj, NULL, &fsck_options))\n> \t\t\t\tdie(_(\"Not all child objects of %s are reachable\"), oid_to_hex(&obj->oid));\n> -\t\t\tif (record_local_links)\n> -\t\t\t\tdo_record_local_links(obj);\n> +\t\t\tif (record_outgoing_links)\n> +\t\t\t\tdo_record_outgoing_links(obj);\n> \n> \t\t\tif (obj->type == OBJ_TREE) {\n> \t\t\t\tstruct tree *item = (struct tree *) obj;\n> @@ -1781,7 +1775,7 @@ static void repack_local_links(void)\n> \tstruct object_id *oid;\n> \tchar *base_name;\n> \n> -\tif (!oidset_size(&local_links))\n> +\tif (!oidset_size(&outgoing_links))\n> \t\treturn;\n> \n> \tbase_name = mkpathdup(\"%s/pack/pack\", repo_get_object_directory(the_repository));\n> @@ -1795,8 +1789,14 @@ static void repack_local_links(void)\n> \tif (start_command(&cmd))\n> \t\tdie(_(\"could not start pack-objects to repack local links\"));\n> \n> -\toidset_iter_init(&local_links, &iter);\n> +\toidset_iter_init(&outgoing_links, &iter);\n> \twhile ((oid = oidset_iter_next(&iter))) {\n> +\t\tstruct object_info info = OBJECT_INFO_INIT;\n> +\t\tif (oid_object_info_extended(the_repository, oid, &info, 0))\n> +\t\t\t/* Missing; assume it is a promisor object */\n> +\t\t\tcontinue;\n> +\t\tif (info.whence == OI_PACKED && info.u.packed.pack->pack_promisor)\n> +\t\t\tcontinue;\n> \t\tif (write_in_full(cmd.in, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||\n> \t\t write_in_full(cmd.in, \"\\n\", 1) < 0)\n> \t\t\tdie(_(\"failed to feed local object to pack-objects\"));\n\nWe've moved our logic to skip promisor objects here, after potential\nobjects have been recorded to the oidset and thus deduped. Now we\n`continue` the loop to skip promisor objects, whereas before we had an\nearly return to avoid recording them in the first place. Seems\nstraightforward.\n\n> @@ -1899,7 +1899,7 @@ int cmd_index_pack(int argc,\n> \t\t\t} else if (skip_to_optional_arg(arg, \"--keep\", &keep_msg)) {\n> \t\t\t\t; /* nothing to do */\n> \t\t\t} else if (skip_to_optional_arg(arg, \"--promisor\", &promisor_msg)) {\n> -\t\t\t\trecord_local_links = 1;\n> +\t\t\t\trecord_outgoing_links = 1;\n> \t\t\t} else if (starts_with(arg, \"--threads=\")) {\n> \t\t\t\tchar *end;\n> \t\t\t\tnr_threads = strtoul(arg+10, &end, 0);\n> -- \n> 2.47.0.338.g60cca15819-goog\n> \n> \n"},{"id":"508472","messageId":"s6adlnfgts43vamiyzdjbjk7bqwy4gvudskclr76uunkgyktbp@pziio343f43z","threadId":"62588","inReplyTo":"cover.1733170252.git.jonathantanmy@google.com","subject":"Re: [PATCH 0/3] Performance improvements for repacking non-promisor objects","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-12-02T21:25:20Z","receivedAt":"2024-12-02T21:25:25Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.12.02 12:18, Jonathan Tan wrote:\n> This is a follow-up to jt/repack-local-promisor [1] (but these patches\n> are based on master, since that branch has already been merged).\n> \n> These patches speed up a fetch that takes 7 hours to take under 3\n> minutes. More details are in the commit messages, especially that of\n> patch 1.\n> \n> Thanks in advance to everyone who reviews. While review is going on,\n> we'll also be testing these at $DAYJOB (I've tested it to work on one\n> known big repo, but there may be others).\n> \n> [1] https://lore.kernel.org/git/cover.1730491845.git.jonathantanmy@google.com/\n> \n> Jonathan Tan (3):\n> index-pack: dedup first during outgoing link check\n> index-pack: no blobs during outgoing link check\n> index-pack: commit tree during outgoing link check\n> \n> builtin/index-pack.c | 49 +++++++++++++++++++++++---------------------\n> 1 file changed, 26 insertions(+), 23 deletions(-)\n> \n> -- \n> 2.47.0.338.g60cca15819-goog\n> \n> \n\nThis series looks good to me, thanks!\n\nReviewed-by: Josh Steadmon \n"},{"id":"508503","messageId":"xmqqa5ddfomb.fsf@gitster.g","threadId":"62588","inReplyTo":"2f2f0db78bf85c14ef132e1924ab5021298aace3.1733170252.git.jonathantanmy@google.com","subject":"Re: [PATCH 3/3] index-pack: commit tree during outgoing link check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-03T03:10:20Z","receivedAt":"2024-12-03T03:10:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Subject: Re: [PATCH 3/3] index-pack: commit tree during outgoing link check\n\n> Commit c08589efdc (index-pack: repack local links into promisor packs,\n> 2024-11-01) seems to contain an oversight in that the tree of a commit\n> is not checked.\n\nI am having a hard time linking the subject with the statement. The\nverb \"commit\" should probably be something else, as you are not\ncreating a tree object while checking, but I am not sure?\n\n> The fix slows down a fetch from a certain repo at\n> $DAYJOB from 2m2.127s to 2m45.052s, but in order to make the fetch\n> correct, it seems worth it.\n\nAnd \"the fix\" is not described so a reader is left wondering. Is\nthe fix for an oversight of not checking merely to check it? IOW,\nis\n\n c08589efdc made outgoing links to be checked for commits, but\n failed to do so for trees. Make sure we check both\n\nwhat is happening?\n\n> In order to test this, we could create server and client repos as\n> follows...\n>\n> C S\n> \\ /\n> O\n>\n> (O and C are commits both on the client and server. S is a commit\n> only on the server. C and S have the same tree but different commit\n> messages.)\n>\n> ...and then, from the client, fetch S from the server.\n>\n> In theory, the client declares \"have C\" and the server can use this\n> information to exclude S's tree (since it knows that the client has C's\n> tree, which is the same as S's tree).\n\nOK.\n\n> However, it is also possible for\n> the server to compute that it needs to send S and not O, and proceed\n> from there;\n\nIf O, C, and S have all identical trees, then wouldn't such a test\nwork well? At that point it does not matter which between O and C \nthe server bases its decision to send S but not S's tree on, no?\n\nIn any case, will queue. Thanks.\n\n\n> therefore the objects of C are not considered at all when\n> determining what to send in the packfile. In order to prevent a test of\n> client functionality from having such a dependence on server behavior, I\n> have not included such a test.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> builtin/index-pack.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index 58d24540dc..338aeeadc8 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -838,6 +838,7 @@ static void do_record_outgoing_links(struct object *obj)\n> \t\tstruct commit *commit = (struct commit *) obj;\n> \t\tstruct commit_list *parents = commit->parents;\n> \n> +\t\trecord_outgoing_link(get_commit_tree_oid(commit));\n> \t\tfor (; parents; parents = parents->next)\n> \t\t\trecord_outgoing_link(&parents->item->object.oid);\n> \t} else if (obj->type == OBJ_TAG) {\n"},{"id":"508507","messageId":"xmqqwmghe7b7.fsf@gitster.g","threadId":"62588","inReplyTo":"s6adlnfgts43vamiyzdjbjk7bqwy4gvudskclr76uunkgyktbp@pziio343f43z","subject":"Re: [PATCH 0/3] Performance improvements for repacking non-promisor objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-03T04:09:32Z","receivedAt":"2024-12-03T04:09:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n>> Thanks in advance to everyone who reviews. While review is going on,\n>> we'll also be testing these at $DAYJOB (I've tested it to work on one\n>> known big repo, but there may be others).\n>> \n>> [1] https://lore.kernel.org/git/cover.1730491845.git.jonathantanmy@google.com/\n>> \n>> Jonathan Tan (3):\n>> index-pack: dedup first during outgoing link check\n>> index-pack: no blobs during outgoing link check\n>> index-pack: commit tree during outgoing link check\n>> \n>> builtin/index-pack.c | 49 +++++++++++++++++++++++---------------------\n>> 1 file changed, 26 insertions(+), 23 deletions(-)\n>> \n>> -- \n>> 2.47.0.338.g60cca15819-goog\n>> \n>> \n>\n> This series looks good to me, thanks!\n>\n> Reviewed-by: Josh Steadmon \n\nThanks.\n"},{"id":"508508","messageId":"xmqqr06pe6vj.fsf@gitster.g","threadId":"62588","inReplyTo":"cover.1733170252.git.jonathantanmy@google.com","subject":"Re: [PATCH 0/3] Performance improvements for repacking non-promisor objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-03T04:18:56Z","receivedAt":"2024-12-03T04:18:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> This is a follow-up to jt/repack-local-promisor [1] (but these patches\n> are based on master, since that branch has already been merged).\n>\n> These patches speed up a fetch that takes 7 hours to take under 3\n> minutes. More details are in the commit messages, especially that of\n> patch 1.\n>\n> Thanks in advance to everyone who reviews. While review is going on,\n> we'll also be testing these at $DAYJOB (I've tested it to work on one\n> known big repo, but there may be others).\n>\n> [1] https://lore.kernel.org/git/cover.1730491845.git.jonathantanmy@google.com/\n>\n> Jonathan Tan (3):\n> index-pack: dedup first during outgoing link check\n> index-pack: no blobs during outgoing link check\n> index-pack: commit tree during outgoing link check\n>\n> builtin/index-pack.c | 49 +++++++++++++++++++++++---------------------\n> 1 file changed, 26 insertions(+), 23 deletions(-)\n\nWhen merged to 'seen', this seems to break quite a many tests, all\nrelated to \"pack\".\n\nI haven't tried running tests on the topic stand-alone.\n\nTest Summary Report\n-------------------\nt5310-pack-bitmaps.sh (Wstat: 256 (exited 1) Tests: 230 Failed: 3)\n Failed tests: 26, 100, 179\n Non-zero exit status: 1\nt5327-multi-pack-bitmaps-rev.sh (Wstat: 256 (exited 1) Tests: 314 Failed: 6)\n Failed tests: 25, 70, 136, 182, 227, 293\n Non-zero exit status: 1\nt5616-partial-clone.sh (Wstat: 256 (exited 1) Tests: 47 Failed: 3)\n Failed tests: 5, 38-39\n Non-zero exit status: 1\nt5326-multi-pack-bitmaps.sh (Wstat: 256 (exited 1) Tests: 357 Failed: 6)\n Failed tests: 25, 70, 146, 199, 244, 320\n Non-zero exit status: 1\nt0410-partial-clone.sh (Wstat: 256 (exited 1) Tests: 38 Failed: 4)\n Failed tests: 11, 14-15, 38\n Non-zero exit status: 1\nt6020-bundle-misc.sh (Wstat: 256 (exited 1) Tests: 30 Failed: 4)\n Failed tests: 16-19\n Non-zero exit status: 1\nt9211-scalar-clone.sh (Wstat: 256 (exited 1) Tests: 13 Failed: 2)\n Failed tests: 4-5\n Non-zero exit status: 1\nFiles=1031, Tests=31721, 324 wallclock secs (11.58 usr 4.57 sys + 674.15 cusr 6136.28 csys = 6826.58 CPU)\nResult: FAIL\ng\n"},{"id":"508509","messageId":"xmqqmshde6sl.fsf@gitster.g","threadId":"62588","inReplyTo":"xmqqr06pe6vj.fsf@gitster.g","subject":"Re: [PATCH 0/3] Performance improvements for repacking non-promisor objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-03T04:20:42Z","receivedAt":"2024-12-03T04:20:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> When merged to 'seen', this seems to break quite a many tests, all\n> related to \"pack\".\n>\n> I haven't tried running tests on the topic stand-alone.\n\nAh, sorry for a false alarm. The breakage may be coming from some\nother topic. Haven't figured out which one yet.\n\n"},{"id":"508511","messageId":"xmqqed2pe5xx.fsf@gitster.g","threadId":"62588","inReplyTo":"xmqqmshde6sl.fsf@gitster.g","subject":"Re: [PATCH 0/3] Performance improvements for repacking non-promisor objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-03T04:39:06Z","receivedAt":"2024-12-03T04:39:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Junio C Hamano writes:\n>\n>> When merged to 'seen', this seems to break quite a many tests, all\n>> related to \"pack\".\n>>\n>> I haven't tried running tests on the topic stand-alone.\n>\n> Ah, sorry for a false alarm. The breakage may be coming from some\n> other topic. Haven't figured out which one yet.\n\nSorry for a double false alarm. The breakages are due to this\ntopic. These do not break without this topic in 'seen', and do\nbreak with this topic in 'seen'.\n"},{"id":"508517","messageId":"Z06ejDgTnC6gWXgx@pks.im","threadId":"62588","inReplyTo":"300f53b8e39fa1dd55f65924d20f8abd22cbbfc9.1733170252.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/3] index-pack: no blobs during outgoing link check","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-03T06:00:44Z","receivedAt":"2024-12-03T06:01:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Dec 02, 2024 at 12:18:39PM -0800, Jonathan Tan wrote:\n> As a follow-up to the parent of this commit, it was found that not\n> checking for the existence of blobs linked from trees sped up the fetch\n> from 24m47.815s to 2m2.127s. Teach Git to do that.\n> \n> The benefit of doing this is as above (fetch speedup), but the drawback\n> is that if the packfile to be indexed references a local blob directly\n> (that is, not through a local tree), that local blob is in danger of\n> being garbage collected. Such a situation may arise if we push local\n> commits, including one with a change to a blob in the root tree,\n> and then the server incorporates them into its main branch through a\n> \"rebase\" or \"squash\" merge strategy, and then we fetch the new main\n> branch from the server.\n\nOkay, so we know that we are basically doing the wrong thing with the\noptimization, but by skipping blobs we can get a significant speedup and\nthe failure mode is that we will re-fetch the object in a later step.\nAnd because we think the situation is rare it shouldn't be a huge issue\nin practice.\n\n> This situation has not been observed yet - we have only noticed missing\n> commits, not missing trees or blobs. (In fact, if it were believed that\n> only missing commits are problematic, one could argue that we should\n> also exclude trees during the outgoing link check; but it is safer to\n> include them.)\n> \n> Due to the rarity of the situation (it has not been observed to happen\n> in real life), and because the \"penalty\" in such a situation is merely\n> to refetch the missing blob when it's needed, the tradeoff seems\n> worth it.\n\nSo is this a one-off event that may happen once per blob, or would we\neventually evict the refetched blob and run into the same situation\nrepeatedly?\n\n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index 8e7d14c17e..58d24540dc 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -830,8 +830,10 @@ static void do_record_outgoing_links(struct object *obj)\n> \t\t\t * verified, so do not print any here.\n> \t\t\t */\n> \t\t\treturn;\n> -\t\twhile (tree_entry_gently(&desc, &entry))\n> -\t\t\trecord_outgoing_link(&entry.oid);\n> +\t\twhile (tree_entry_gently(&desc, &entry)) {\n> +\t\t\tif (S_ISDIR(entry.mode))\n> +\t\t\t\trecord_outgoing_link(&entry.oid);\n> +\t\t}\n\nWithout the context of the commit message this code snippet likely would\nnot make any sense to a reader. The \"correct\" logic would be to record\nall objects, regardless of whether they are an object ID or not. But we\nexplicitly choose not to as a tradeoff between performance and\ncorrectness.\n\nAll to say that we should have a comment here that explains what is\ngoing on.\n\nPatrick\n"},{"id":"508561","messageId":"20241203214000.2032992-1-jonathantanmy@google.com","threadId":"62588","inReplyTo":"Z06ejDgTnC6gWXgx@pks.im","subject":"Re: [PATCH 2/3] index-pack: no blobs during outgoing link check","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-12-03T21:40:00Z","receivedAt":"2024-12-03T21:40:03Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Patrick Steinhardt writes:\n> > This situation has not been observed yet - we have only noticed missing\n> > commits, not missing trees or blobs. (In fact, if it were believed that\n> > only missing commits are problematic, one could argue that we should\n> > also exclude trees during the outgoing link check; but it is safer to\n> > include them.)\n> > \n> > Due to the rarity of the situation (it has not been observed to happen\n> > in real life), and because the \"penalty\" in such a situation is merely\n> > to refetch the missing blob when it's needed, the tradeoff seems\n> > worth it.\n> \n> So is this a one-off event that may happen once per blob, or would we\n> eventually evict the refetched blob and run into the same situation\n> repeatedly?\n\nOne-off, since when refetched, the blob is in a promisor pack (and\nthus won't be GC-ed). I've added this to the code comment that I added\nfollowing your suggestion below.\n\n> > diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> > index 8e7d14c17e..58d24540dc 100644\n> > --- a/builtin/index-pack.c\n> > +++ b/builtin/index-pack.c\n> > @@ -830,8 +830,10 @@ static void do_record_outgoing_links(struct object *obj)\n> > \t\t\t * verified, so do not print any here.\n> > \t\t\t */\n> > \t\t\treturn;\n> > -\t\twhile (tree_entry_gently(&desc, &entry))\n> > -\t\t\trecord_outgoing_link(&entry.oid);\n> > +\t\twhile (tree_entry_gently(&desc, &entry)) {\n> > +\t\t\tif (S_ISDIR(entry.mode))\n> > +\t\t\t\trecord_outgoing_link(&entry.oid);\n> > +\t\t}\n> \n> Without the context of the commit message this code snippet likely would\n> not make any sense to a reader. The \"correct\" logic would be to record\n> all objects, regardless of whether they are an object ID or not. But we\n> explicitly choose not to as a tradeoff between performance and\n> correctness.\n> \n> All to say that we should have a comment here that explains what is\n> going on.\n> \n> Patrick\n\nMakes sense. I had to move almost the entirety of the commit message\ninto a code comment - I don't think putting merely a part here would be\nenough context.\n"},{"id":"508562","messageId":"20241203214209.2033773-1-jonathantanmy@google.com","threadId":"62588","inReplyTo":"xmqqa5ddfomb.fsf@gitster.g","subject":"Re: [PATCH 3/3] index-pack: commit tree during outgoing link check","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-12-03T21:42:09Z","receivedAt":"2024-12-03T21:42:12Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Junio C Hamano writes:\n> > The fix slows down a fetch from a certain repo at\n> > $DAYJOB from 2m2.127s to 2m45.052s, but in order to make the fetch\n> > correct, it seems worth it.\n> \n> And \"the fix\" is not described so a reader is left wondering. Is\n> the fix for an oversight of not checking merely to check it? IOW,\n> is\n> \n> c08589efdc made outgoing links to be checked for commits, but\n> failed to do so for trees. Make sure we check both\n> \n> what is happening?\n\nYes. I was trying to keep to the character limit and in doing so, made\nthe commit message title hard to understand. I think the new title\nshould be easier to understand (and also stated explicitly in the commit\nmessage what is being taught to Git).\n\n> > However, it is also possible for\n> > the server to compute that it needs to send S and not O, and proceed\n> > from there;\n> \n> If O, C, and S have all identical trees, then wouldn't such a test\n> work well? At that point it does not matter which between O and C \n> the server bases its decision to send S but not S's tree on, no?\n> \n> In any case, will queue. Thanks.\n\nO has a different tree from C and S. I will add a note to clarify this.\n"},{"id":"508563","messageId":"cover.1733259949.git.jonathantanmy@google.com","threadId":"62588","inReplyTo":"cover.1733170252.git.jonathantanmy@google.com","subject":"[PATCH v2 0/3] Performance improvements for repacking non-promisor objects","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-12-03T21:43:01Z","receivedAt":"2024-12-03T21:43:08Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks everyone for the reviews.\n\nThe only code change is to invoke the repacking of non-promisor objects\ninto a promisor pack only when the first object that needs repacking\nis detected. If not, there will be an empty pack created, which is the\ncause of the failing tests Junio noticed.\n\nOther than that, there are some code comments and commit message changes\nas requested by reviews.\n\nI've checked that all tests pass before and after merging with \"seen\".\n\nJonathan Tan (3):\n index-pack --promisor: dedup before checking links\n index-pack --promisor: don't check blobs\n index-pack --promisor: also check commits' trees\n\n builtin/index-pack.c | 101 +++++++++++++++++++++++++++++--------------\n 1 file changed, 69 insertions(+), 32 deletions(-)\n\nRange-diff against v1:\n1: 5f0f114dbd ! 1: 7ae21c921f index-pack: dedup first during outgoing link check\n @@ Metadata\n Author: Jonathan Tan \n \n ## Commit message ##\n - index-pack: dedup first during outgoing link check\n + index-pack --promisor: dedup before checking links\n \n Commit c08589efdc (index-pack: repack local links into promisor packs,\n 2024-11-01) fixed a bug with what was believed to be a negligible\n @@ builtin/index-pack.c: static void repack_local_links(void)\n +\tif (!oidset_size(&outgoing_links))\n \t\treturn;\n \n - \tbase_name = mkpathdup(\"%s/pack/pack\", repo_get_object_directory(the_repository));\n -@@ builtin/index-pack.c: static void repack_local_links(void)\n - \tif (start_command(&cmd))\n - \t\tdie(_(\"could not start pack-objects to repack local links\"));\n - \n --\toidset_iter_init(&local_links, &iter);\n +-\tbase_name = mkpathdup(\"%s/pack/pack\", repo_get_object_directory(the_repository));\n +\toidset_iter_init(&outgoing_links, &iter);\n - \twhile ((oid = oidset_iter_next(&iter))) {\n ++\twhile ((oid = oidset_iter_next(&iter))) {\n +\t\tstruct object_info info = OBJECT_INFO_INIT;\n +\t\tif (oid_object_info_extended(the_repository, oid, &info, 0))\n +\t\t\t/* Missing; assume it is a promisor object */\n +\t\t\tcontinue;\n +\t\tif (info.whence == OI_PACKED && info.u.packed.pack->pack_promisor)\n +\t\t\tcontinue;\n + \n +-\tstrvec_push(&cmd.args, \"pack-objects\");\n +-\tstrvec_push(&cmd.args, \"--exclude-promisor-objects-best-effort\");\n +-\tstrvec_push(&cmd.args, base_name);\n +-\tcmd.git_cmd = 1;\n +-\tcmd.in = -1;\n +-\tcmd.out = -1;\n +-\tif (start_command(&cmd))\n +-\t\tdie(_(\"could not start pack-objects to repack local links\"));\n ++\t\tif (!cmd.args.nr) {\n ++\t\t\tbase_name = mkpathdup(\n ++\t\t\t\t\"%s/pack/pack\",\n ++\t\t\t\trepo_get_object_directory(the_repository));\n ++\t\t\tstrvec_push(&cmd.args, \"pack-objects\");\n ++\t\t\tstrvec_push(&cmd.args,\n ++\t\t\t\t \"--exclude-promisor-objects-best-effort\");\n ++\t\t\tstrvec_push(&cmd.args, base_name);\n ++\t\t\tcmd.git_cmd = 1;\n ++\t\t\tcmd.in = -1;\n ++\t\t\tcmd.out = -1;\n ++\t\t\tif (start_command(&cmd))\n ++\t\t\t\tdie(_(\"could not start pack-objects to repack local links\"));\n ++\t\t}\n + \n +-\toidset_iter_init(&local_links, &iter);\n +-\twhile ((oid = oidset_iter_next(&iter))) {\n \t\tif (write_in_full(cmd.in, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||\n \t\t write_in_full(cmd.in, \"\\n\", 1) < 0)\n \t\t\tdie(_(\"failed to feed local object to pack-objects\"));\n + \t}\n ++\n ++\tif (!cmd.args.nr)\n ++\t\treturn;\n ++\n + \tclose(cmd.in);\n + \n + \tout = xfdopen(cmd.out, \"r\");\n @@ builtin/index-pack.c: int cmd_index_pack(int argc,\n \t\t\t} else if (skip_to_optional_arg(arg, \"--keep\", &keep_msg)) {\n \t\t\t\t; /* nothing to do */\n2: 300f53b8e3 ! 2: 5a63c9a5ca index-pack: no blobs during outgoing link check\n @@ Metadata\n Author: Jonathan Tan \n \n ## Commit message ##\n - index-pack: no blobs during outgoing link check\n + index-pack --promisor: don't check blobs\n \n As a follow-up to the parent of this commit, it was found that not\n checking for the existence of blobs linked from trees sped up the fetch\n from 24m47.815s to 2m2.127s. Teach Git to do that.\n \n - The benefit of doing this is as above (fetch speedup), but the drawback\n - is that if the packfile to be indexed references a local blob directly\n - (that is, not through a local tree), that local blob is in danger of\n - being garbage collected. Such a situation may arise if we push local\n - commits, including one with a change to a blob in the root tree,\n - and then the server incorporates them into its main branch through a\n - \"rebase\" or \"squash\" merge strategy, and then we fetch the new main\n - branch from the server.\n -\n - This situation has not been observed yet - we have only noticed missing\n - commits, not missing trees or blobs. (In fact, if it were believed that\n - only missing commits are problematic, one could argue that we should\n - also exclude trees during the outgoing link check; but it is safer to\n - include them.)\n -\n - Due to the rarity of the situation (it has not been observed to happen\n - in real life), and because the \"penalty\" in such a situation is merely\n - to refetch the missing blob when it's needed, the tradeoff seems\n - worth it.\n + The tradeoff of not checking blobs is documented in a code comment.\n \n (Blobs may also be linked from tag objects, but it is impossible to know\n the type of an object linked from a tag object without looking it up in\n @@ Commit message\n Signed-off-by: Jonathan Tan \n \n ## builtin/index-pack.c ##\n +@@ builtin/index-pack.c: static void record_outgoing_link(const struct object_id *oid)\n + \toidset_insert(&outgoing_links, oid);\n + }\n + \n ++static void maybe_record_name_entry(const struct name_entry *entry)\n ++{\n ++\t/*\n ++\t * The benefit of doing this is as above (fetch speedup), but the drawback\n ++is that if the packfile to be indexed references a local blob directly\n ++(that is, not through a local tree), that local blob is in danger of\n ++being garbage collected. Such a situation may arise if we push local\n ++commits, including one with a change to a blob in the root tree,\n ++and then the server incorporates them into its main branch through a\n ++\"rebase\" or \"squash\" merge strategy, and then we fetch the new main\n ++branch from the server.\n ++\n ++This situation has not been observed yet - we have only noticed missing\n ++commits, not missing trees or blobs. (In fact, if it were believed that\n ++only missing commits are problematic, one could argue that we should\n ++also exclude trees during the outgoing link check; but it is safer to\n ++include them.)\n ++\n ++Due to the rarity of the situation (it has not been observed to happen\n ++in real life), and because the \"penalty\" in such a situation is merely\n ++to refetch the missing blob when it's needed, the tradeoff seems\n ++worth it.\n ++\t*/\n ++\tif (S_ISDIR(entry->mode))\n ++\t\trecord_outgoing_link(&entry->oid);\n ++}\n ++\n + static void do_record_outgoing_links(struct object *obj)\n + {\n + \tif (obj->type == OBJ_TREE) {\n @@ builtin/index-pack.c: static void do_record_outgoing_links(struct object *obj)\n - \t\t\t * verified, so do not print any here.\n \t\t\t */\n \t\t\treturn;\n --\t\twhile (tree_entry_gently(&desc, &entry))\n + \t\twhile (tree_entry_gently(&desc, &entry))\n -\t\t\trecord_outgoing_link(&entry.oid);\n -+\t\twhile (tree_entry_gently(&desc, &entry)) {\n -+\t\t\tif (S_ISDIR(entry.mode))\n -+\t\t\t\trecord_outgoing_link(&entry.oid);\n -+\t\t}\n ++\t\t\tmaybe_record_name_entry(&entry);\n \t} else if (obj->type == OBJ_COMMIT) {\n \t\tstruct commit *commit = (struct commit *) obj;\n \t\tstruct commit_list *parents = commit->parents;\n3: 2f2f0db78b ! 3: 8139325bf2 index-pack: commit tree during outgoing link check\n @@ Metadata\n Author: Jonathan Tan \n \n ## Commit message ##\n - index-pack: commit tree during outgoing link check\n + index-pack --promisor: also check commits' trees\n \n Commit c08589efdc (index-pack: repack local links into promisor packs,\n 2024-11-01) seems to contain an oversight in that the tree of a commit\n - is not checked. The fix slows down a fetch from a certain repo at\n - $DAYJOB from 2m2.127s to 2m45.052s, but in order to make the fetch\n - correct, it seems worth it.\n + is not checked. Teach git to check these trees.\n +\n + The fix slows down a fetch from a certain repo at $DAYJOB from 2m2.127s\n + to 2m45.052s, but in order to make the fetch correct, it seems worth it.\n \n In order to test this, we could create server and client repos as\n follows...\n @@ Commit message\n \n (O and C are commits both on the client and server. S is a commit\n only on the server. C and S have the same tree but different commit\n - messages.)\n + messages. The diff between O and C is non-zero.)\n \n ...and then, from the client, fetch S from the server.\n \n-- \n2.47.0.338.g60cca15819-goog\n\n"},{"id":"508564","messageId":"7ae21c921fe367d4b15cd4a299196009c15205d9.1733259949.git.jonathantanmy@google.com","threadId":"62588","inReplyTo":"cover.1733259949.git.jonathantanmy@google.com","subject":"[PATCH v2 1/3] index-pack --promisor: dedup before checking links","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-12-03T21:43:02Z","receivedAt":"2024-12-03T21:43:10Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Commit c08589efdc (index-pack: repack local links into promisor packs,\n2024-11-01) fixed a bug with what was believed to be a negligible\ndecrease in performance [1] [2]. But at $DAYJOB, with at least one repo,\nit was found that the decrease in performance was very significant.\n\nLooking at the patch, whenever we parse an object in the packfile to\nbe indexed, we check the targets of all its outgoing links for its\nexistence. However, this could be optimized by first collecting all such\ntargets into an oidset (thus deduplicating them) before checking. Teach\nGit to do that.\n\nOn a certain fetch from the aforementioned repo, this improved\nperformance from approximately 7 hours to 24m47.815s. This number will\nbe further reduced in a subsequent patch.\n\n[1] https://lore.kernel.org/git/CAG1j3zGiNMbri8rZNaF0w+yP+6OdMz0T8+8_Wgd1R_p1HzVasg@mail.gmail.com/\n[2] https://lore.kernel.org/git/20241105212849.3759572-1-jonathantanmy@google.com/\n\nSigned-off-by: Jonathan Tan \n---\n builtin/index-pack.c | 73 +++++++++++++++++++++++++-------------------\n 1 file changed, 41 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 95babdc5ea..d1c777a6af 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -155,11 +155,11 @@ static int input_fd, output_fd;\n static const char *curr_pack;\n \n /*\n- * local_links is guarded by read_mutex, and record_local_links is read-only in\n- * a thread.\n+ * outgoing_links is guarded by read_mutex, and record_outgoing_links is\n+ * read-only in a thread.\n */\n-static struct oidset local_links = OIDSET_INIT;\n-static int record_local_links;\n+static struct oidset outgoing_links = OIDSET_INIT;\n+static int record_outgoing_links;\n \n static struct thread_local_data *thread_data;\n static int nr_dispatched;\n@@ -812,18 +812,12 @@ static int check_collison(struct object_entry *entry)\n \treturn 0;\n }\n \n-static void record_if_local_object(const struct object_id *oid)\n+static void record_outgoing_link(const struct object_id *oid)\n {\n-\tstruct object_info info = OBJECT_INFO_INIT;\n-\tif (oid_object_info_extended(the_repository, oid, &info, 0))\n-\t\t/* Missing; assume it is a promisor object */\n-\t\treturn;\n-\tif (info.whence == OI_PACKED && info.u.packed.pack->pack_promisor)\n-\t\treturn;\n-\toidset_insert(&local_links, oid);\n+\toidset_insert(&outgoing_links, oid);\n }\n \n-static void do_record_local_links(struct object *obj)\n+static void do_record_outgoing_links(struct object *obj)\n {\n \tif (obj->type == OBJ_TREE) {\n \t\tstruct tree *tree = (struct tree *)obj;\n@@ -837,16 +831,16 @@ static void do_record_local_links(struct object *obj)\n \t\t\t */\n \t\t\treturn;\n \t\twhile (tree_entry_gently(&desc, &entry))\n-\t\t\trecord_if_local_object(&entry.oid);\n+\t\t\trecord_outgoing_link(&entry.oid);\n \t} else if (obj->type == OBJ_COMMIT) {\n \t\tstruct commit *commit = (struct commit *) obj;\n \t\tstruct commit_list *parents = commit->parents;\n \n \t\tfor (; parents; parents = parents->next)\n-\t\t\trecord_if_local_object(&parents->item->object.oid);\n+\t\t\trecord_outgoing_link(&parents->item->object.oid);\n \t} else if (obj->type == OBJ_TAG) {\n \t\tstruct tag *tag = (struct tag *) obj;\n-\t\trecord_if_local_object(get_tagged_oid(tag));\n+\t\trecord_outgoing_link(get_tagged_oid(tag));\n \t}\n }\n \n@@ -896,7 +890,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\tfree(has_data);\n \t}\n \n-\tif (strict || do_fsck_object || record_local_links) {\n+\tif (strict || do_fsck_object || record_outgoing_links) {\n \t\tread_lock();\n \t\tif (type == OBJ_BLOB) {\n \t\t\tstruct blob *blob = lookup_blob(the_repository, oid);\n@@ -928,8 +922,8 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\t\tdie(_(\"fsck error in packed object\"));\n \t\t\tif (strict && fsck_walk(obj, NULL, &fsck_options))\n \t\t\t\tdie(_(\"Not all child objects of %s are reachable\"), oid_to_hex(&obj->oid));\n-\t\t\tif (record_local_links)\n-\t\t\t\tdo_record_local_links(obj);\n+\t\t\tif (record_outgoing_links)\n+\t\t\t\tdo_record_outgoing_links(obj);\n \n \t\t\tif (obj->type == OBJ_TREE) {\n \t\t\t\tstruct tree *item = (struct tree *) obj;\n@@ -1781,26 +1775,41 @@ static void repack_local_links(void)\n \tstruct object_id *oid;\n \tchar *base_name;\n \n-\tif (!oidset_size(&local_links))\n+\tif (!oidset_size(&outgoing_links))\n \t\treturn;\n \n-\tbase_name = mkpathdup(\"%s/pack/pack\", repo_get_object_directory(the_repository));\n+\toidset_iter_init(&outgoing_links, &iter);\n+\twhile ((oid = oidset_iter_next(&iter))) {\n+\t\tstruct object_info info = OBJECT_INFO_INIT;\n+\t\tif (oid_object_info_extended(the_repository, oid, &info, 0))\n+\t\t\t/* Missing; assume it is a promisor object */\n+\t\t\tcontinue;\n+\t\tif (info.whence == OI_PACKED && info.u.packed.pack->pack_promisor)\n+\t\t\tcontinue;\n \n-\tstrvec_push(&cmd.args, \"pack-objects\");\n-\tstrvec_push(&cmd.args, \"--exclude-promisor-objects-best-effort\");\n-\tstrvec_push(&cmd.args, base_name);\n-\tcmd.git_cmd = 1;\n-\tcmd.in = -1;\n-\tcmd.out = -1;\n-\tif (start_command(&cmd))\n-\t\tdie(_(\"could not start pack-objects to repack local links\"));\n+\t\tif (!cmd.args.nr) {\n+\t\t\tbase_name = mkpathdup(\n+\t\t\t\t\"%s/pack/pack\",\n+\t\t\t\trepo_get_object_directory(the_repository));\n+\t\t\tstrvec_push(&cmd.args, \"pack-objects\");\n+\t\t\tstrvec_push(&cmd.args,\n+\t\t\t\t \"--exclude-promisor-objects-best-effort\");\n+\t\t\tstrvec_push(&cmd.args, base_name);\n+\t\t\tcmd.git_cmd = 1;\n+\t\t\tcmd.in = -1;\n+\t\t\tcmd.out = -1;\n+\t\t\tif (start_command(&cmd))\n+\t\t\t\tdie(_(\"could not start pack-objects to repack local links\"));\n+\t\t}\n \n-\toidset_iter_init(&local_links, &iter);\n-\twhile ((oid = oidset_iter_next(&iter))) {\n \t\tif (write_in_full(cmd.in, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||\n \t\t write_in_full(cmd.in, \"\\n\", 1) < 0)\n \t\t\tdie(_(\"failed to feed local object to pack-objects\"));\n \t}\n+\n+\tif (!cmd.args.nr)\n+\t\treturn;\n+\n \tclose(cmd.in);\n \n \tout = xfdopen(cmd.out, \"r\");\n@@ -1899,7 +1908,7 @@ int cmd_index_pack(int argc,\n \t\t\t} else if (skip_to_optional_arg(arg, \"--keep\", &keep_msg)) {\n \t\t\t\t; /* nothing to do */\n \t\t\t} else if (skip_to_optional_arg(arg, \"--promisor\", &promisor_msg)) {\n-\t\t\t\trecord_local_links = 1;\n+\t\t\t\trecord_outgoing_links = 1;\n \t\t\t} else if (starts_with(arg, \"--threads=\")) {\n \t\t\t\tchar *end;\n \t\t\t\tnr_threads = strtoul(arg+10, &end, 0);\n-- \n2.47.0.338.g60cca15819-goog\n\n"},{"id":"508565","messageId":"5a63c9a5cac8088730cc536f33b0af052c90aca1.1733259949.git.jonathantanmy@google.com","threadId":"62588","inReplyTo":"cover.1733259949.git.jonathantanmy@google.com","subject":"[PATCH v2 2/3] index-pack --promisor: don't check blobs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-12-03T21:43:03Z","receivedAt":"2024-12-03T21:43:11Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"As a follow-up to the parent of this commit, it was found that not\nchecking for the existence of blobs linked from trees sped up the fetch\nfrom 24m47.815s to 2m2.127s. Teach Git to do that.\n\nThe tradeoff of not checking blobs is documented in a code comment.\n\n(Blobs may also be linked from tag objects, but it is impossible to know\nthe type of an object linked from a tag object without looking it up in\nthe object database, so the code for that is untouched.)\n\nSigned-off-by: Jonathan Tan \n---\n builtin/index-pack.c | 29 ++++++++++++++++++++++++++++-\n 1 file changed, 28 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex d1c777a6af..57b7888c42 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -817,6 +817,33 @@ static void record_outgoing_link(const struct object_id *oid)\n \toidset_insert(&outgoing_links, oid);\n }\n \n+static void maybe_record_name_entry(const struct name_entry *entry)\n+{\n+\t/*\n+\t * The benefit of doing this is as above (fetch speedup), but the drawback\n+is that if the packfile to be indexed references a local blob directly\n+(that is, not through a local tree), that local blob is in danger of\n+being garbage collected. Such a situation may arise if we push local\n+commits, including one with a change to a blob in the root tree,\n+and then the server incorporates them into its main branch through a\n+\"rebase\" or \"squash\" merge strategy, and then we fetch the new main\n+branch from the server.\n+\n+This situation has not been observed yet - we have only noticed missing\n+commits, not missing trees or blobs. (In fact, if it were believed that\n+only missing commits are problematic, one could argue that we should\n+also exclude trees during the outgoing link check; but it is safer to\n+include them.)\n+\n+Due to the rarity of the situation (it has not been observed to happen\n+in real life), and because the \"penalty\" in such a situation is merely\n+to refetch the missing blob when it's needed, the tradeoff seems\n+worth it.\n+\t*/\n+\tif (S_ISDIR(entry->mode))\n+\t\trecord_outgoing_link(&entry->oid);\n+}\n+\n static void do_record_outgoing_links(struct object *obj)\n {\n \tif (obj->type == OBJ_TREE) {\n@@ -831,7 +858,7 @@ static void do_record_outgoing_links(struct object *obj)\n \t\t\t */\n \t\t\treturn;\n \t\twhile (tree_entry_gently(&desc, &entry))\n-\t\t\trecord_outgoing_link(&entry.oid);\n+\t\t\tmaybe_record_name_entry(&entry);\n \t} else if (obj->type == OBJ_COMMIT) {\n \t\tstruct commit *commit = (struct commit *) obj;\n \t\tstruct commit_list *parents = commit->parents;\n-- \n2.47.0.338.g60cca15819-goog\n\n"},{"id":"508566","messageId":"8139325bf221685793ec40487c201be7103daad6.1733259949.git.jonathantanmy@google.com","threadId":"62588","inReplyTo":"cover.1733259949.git.jonathantanmy@google.com","subject":"[PATCH v2 3/3] index-pack --promisor: also check commits' trees","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-12-03T21:43:04Z","receivedAt":"2024-12-03T21:43:13Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Commit c08589efdc (index-pack: repack local links into promisor packs,\n2024-11-01) seems to contain an oversight in that the tree of a commit\nis not checked. Teach git to check these trees.\n\nThe fix slows down a fetch from a certain repo at $DAYJOB from 2m2.127s\nto 2m45.052s, but in order to make the fetch correct, it seems worth it.\n\nIn order to test this, we could create server and client repos as\nfollows...\n\n C S\n \\ /\n O\n\n(O and C are commits both on the client and server. S is a commit\nonly on the server. C and S have the same tree but different commit\nmessages. The diff between O and C is non-zero.)\n\n...and then, from the client, fetch S from the server.\n\nIn theory, the client declares \"have C\" and the server can use this\ninformation to exclude S's tree (since it knows that the client has C's\ntree, which is the same as S's tree). However, it is also possible for\nthe server to compute that it needs to send S and not O, and proceed\nfrom there; therefore the objects of C are not considered at all when\ndetermining what to send in the packfile. In order to prevent a test of\nclient functionality from having such a dependence on server behavior, I\nhave not included such a test.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/index-pack.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 57b7888c42..2250a410e2 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -863,6 +863,7 @@ static void do_record_outgoing_links(struct object *obj)\n \t\tstruct commit *commit = (struct commit *) obj;\n \t\tstruct commit_list *parents = commit->parents;\n \n+\t\trecord_outgoing_link(get_commit_tree_oid(commit));\n \t\tfor (; parents; parents = parents->next)\n \t\t\trecord_outgoing_link(&parents->item->object.oid);\n \t} else if (obj->type == OBJ_TAG) {\n-- \n2.47.0.338.g60cca15819-goog\n\n"},{"id":"508567","messageId":"cover.1733262661.git.jonathantanmy@google.com","threadId":"62588","inReplyTo":"cover.1733170252.git.jonathantanmy@google.com","subject":"[PATCH v3 0/3] Performance improvements for repacking non-promisor objects","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-12-03T21:52:53Z","receivedAt":"2024-12-03T21:53:01Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Apparently I did not save in my text editor (and didn't notice because\nthe code comment was still valid syntactically, so everything still\ncompiled). Here's a version with the updated and correctly formatted\ncode comment.\n\nJonathan Tan (3):\n index-pack --promisor: dedup before checking links\n index-pack --promisor: don't check blobs\n index-pack --promisor: also check commits' trees\n\n builtin/index-pack.c | 103 +++++++++++++++++++++++++++++--------------\n 1 file changed, 71 insertions(+), 32 deletions(-)\n\nRange-diff against v2:\n1: 7ae21c921f = 1: 7ae21c921f index-pack --promisor: dedup before checking links\n2: 5a63c9a5ca ! 2: a1d2a20203 index-pack --promisor: don't check blobs\n @@ builtin/index-pack.c: static void record_outgoing_link(const struct object_id *o\n +static void maybe_record_name_entry(const struct name_entry *entry)\n +{\n +\t/*\n -+\t * The benefit of doing this is as above (fetch speedup), but the drawback\n -+is that if the packfile to be indexed references a local blob directly\n -+(that is, not through a local tree), that local blob is in danger of\n -+being garbage collected. Such a situation may arise if we push local\n -+commits, including one with a change to a blob in the root tree,\n -+and then the server incorporates them into its main branch through a\n -+\"rebase\" or \"squash\" merge strategy, and then we fetch the new main\n -+branch from the server.\n -+\n -+This situation has not been observed yet - we have only noticed missing\n -+commits, not missing trees or blobs. (In fact, if it were believed that\n -+only missing commits are problematic, one could argue that we should\n -+also exclude trees during the outgoing link check; but it is safer to\n -+include them.)\n -+\n -+Due to the rarity of the situation (it has not been observed to happen\n -+in real life), and because the \"penalty\" in such a situation is merely\n -+to refetch the missing blob when it's needed, the tradeoff seems\n -+worth it.\n ++\t * Checking only trees here results in a significantly faster packfile\n ++\t * indexing, but the drawback is that if the packfile to be indexed\n ++\t * references a local blob only directly (that is, never through a\n ++\t * local tree), that local blob is in danger of being garbage\n ++\t * collected. Such a situation may arise if we push local commits,\n ++\t * including one with a change to a blob in the root tree, and then the\n ++\t * server incorporates them into its main branch through a \"rebase\" or\n ++\t * \"squash\" merge strategy, and then we fetch the new main branch from\n ++\t * the server.\n ++\t *\n ++\t * This situation has not been observed yet - we have only noticed\n ++\t * missing commits, not missing trees or blobs. (In fact, if it were\n ++\t * believed that only missing commits are problematic, one could argue\n ++\t * that we should also exclude trees during the outgoing link check;\n ++\t * but it is safer to include them.)\n ++\t *\n ++\t * Due to the rarity of the situation (it has not been observed to\n ++\t * happen in real life), and because the \"penalty\" in such a situation\n ++\t * is merely to refetch the missing blob when it's needed (and this\n ++\t * happens only once - when refetched, the blob goes into a promisor\n ++\t * pack, so it won't be GC-ed, the tradeoff seems worth it.\n +\t*/\n +\tif (S_ISDIR(entry->mode))\n +\t\trecord_outgoing_link(&entry->oid);\n3: 8139325bf2 = 3: f9f9969a8f index-pack --promisor: also check commits' trees\n-- \n2.47.0.338.g60cca15819-goog\n\n"},{"id":"508568","messageId":"7ae21c921fe367d4b15cd4a299196009c15205d9.1733262662.git.jonathantanmy@google.com","threadId":"62588","inReplyTo":"cover.1733262661.git.jonathantanmy@google.com","subject":"[PATCH v3 1/3] index-pack --promisor: dedup before checking links","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-12-03T21:52:54Z","receivedAt":"2024-12-03T21:53:03Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Commit c08589efdc (index-pack: repack local links into promisor packs,\n2024-11-01) fixed a bug with what was believed to be a negligible\ndecrease in performance [1] [2]. But at $DAYJOB, with at least one repo,\nit was found that the decrease in performance was very significant.\n\nLooking at the patch, whenever we parse an object in the packfile to\nbe indexed, we check the targets of all its outgoing links for its\nexistence. However, this could be optimized by first collecting all such\ntargets into an oidset (thus deduplicating them) before checking. Teach\nGit to do that.\n\nOn a certain fetch from the aforementioned repo, this improved\nperformance from approximately 7 hours to 24m47.815s. This number will\nbe further reduced in a subsequent patch.\n\n[1] https://lore.kernel.org/git/CAG1j3zGiNMbri8rZNaF0w+yP+6OdMz0T8+8_Wgd1R_p1HzVasg@mail.gmail.com/\n[2] https://lore.kernel.org/git/20241105212849.3759572-1-jonathantanmy@google.com/\n\nSigned-off-by: Jonathan Tan \n---\n builtin/index-pack.c | 73 +++++++++++++++++++++++++-------------------\n 1 file changed, 41 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 95babdc5ea..d1c777a6af 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -155,11 +155,11 @@ static int input_fd, output_fd;\n static const char *curr_pack;\n \n /*\n- * local_links is guarded by read_mutex, and record_local_links is read-only in\n- * a thread.\n+ * outgoing_links is guarded by read_mutex, and record_outgoing_links is\n+ * read-only in a thread.\n */\n-static struct oidset local_links = OIDSET_INIT;\n-static int record_local_links;\n+static struct oidset outgoing_links = OIDSET_INIT;\n+static int record_outgoing_links;\n \n static struct thread_local_data *thread_data;\n static int nr_dispatched;\n@@ -812,18 +812,12 @@ static int check_collison(struct object_entry *entry)\n \treturn 0;\n }\n \n-static void record_if_local_object(const struct object_id *oid)\n+static void record_outgoing_link(const struct object_id *oid)\n {\n-\tstruct object_info info = OBJECT_INFO_INIT;\n-\tif (oid_object_info_extended(the_repository, oid, &info, 0))\n-\t\t/* Missing; assume it is a promisor object */\n-\t\treturn;\n-\tif (info.whence == OI_PACKED && info.u.packed.pack->pack_promisor)\n-\t\treturn;\n-\toidset_insert(&local_links, oid);\n+\toidset_insert(&outgoing_links, oid);\n }\n \n-static void do_record_local_links(struct object *obj)\n+static void do_record_outgoing_links(struct object *obj)\n {\n \tif (obj->type == OBJ_TREE) {\n \t\tstruct tree *tree = (struct tree *)obj;\n@@ -837,16 +831,16 @@ static void do_record_local_links(struct object *obj)\n \t\t\t */\n \t\t\treturn;\n \t\twhile (tree_entry_gently(&desc, &entry))\n-\t\t\trecord_if_local_object(&entry.oid);\n+\t\t\trecord_outgoing_link(&entry.oid);\n \t} else if (obj->type == OBJ_COMMIT) {\n \t\tstruct commit *commit = (struct commit *) obj;\n \t\tstruct commit_list *parents = commit->parents;\n \n \t\tfor (; parents; parents = parents->next)\n-\t\t\trecord_if_local_object(&parents->item->object.oid);\n+\t\t\trecord_outgoing_link(&parents->item->object.oid);\n \t} else if (obj->type == OBJ_TAG) {\n \t\tstruct tag *tag = (struct tag *) obj;\n-\t\trecord_if_local_object(get_tagged_oid(tag));\n+\t\trecord_outgoing_link(get_tagged_oid(tag));\n \t}\n }\n \n@@ -896,7 +890,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\tfree(has_data);\n \t}\n \n-\tif (strict || do_fsck_object || record_local_links) {\n+\tif (strict || do_fsck_object || record_outgoing_links) {\n \t\tread_lock();\n \t\tif (type == OBJ_BLOB) {\n \t\t\tstruct blob *blob = lookup_blob(the_repository, oid);\n@@ -928,8 +922,8 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\t\tdie(_(\"fsck error in packed object\"));\n \t\t\tif (strict && fsck_walk(obj, NULL, &fsck_options))\n \t\t\t\tdie(_(\"Not all child objects of %s are reachable\"), oid_to_hex(&obj->oid));\n-\t\t\tif (record_local_links)\n-\t\t\t\tdo_record_local_links(obj);\n+\t\t\tif (record_outgoing_links)\n+\t\t\t\tdo_record_outgoing_links(obj);\n \n \t\t\tif (obj->type == OBJ_TREE) {\n \t\t\t\tstruct tree *item = (struct tree *) obj;\n@@ -1781,26 +1775,41 @@ static void repack_local_links(void)\n \tstruct object_id *oid;\n \tchar *base_name;\n \n-\tif (!oidset_size(&local_links))\n+\tif (!oidset_size(&outgoing_links))\n \t\treturn;\n \n-\tbase_name = mkpathdup(\"%s/pack/pack\", repo_get_object_directory(the_repository));\n+\toidset_iter_init(&outgoing_links, &iter);\n+\twhile ((oid = oidset_iter_next(&iter))) {\n+\t\tstruct object_info info = OBJECT_INFO_INIT;\n+\t\tif (oid_object_info_extended(the_repository, oid, &info, 0))\n+\t\t\t/* Missing; assume it is a promisor object */\n+\t\t\tcontinue;\n+\t\tif (info.whence == OI_PACKED && info.u.packed.pack->pack_promisor)\n+\t\t\tcontinue;\n \n-\tstrvec_push(&cmd.args, \"pack-objects\");\n-\tstrvec_push(&cmd.args, \"--exclude-promisor-objects-best-effort\");\n-\tstrvec_push(&cmd.args, base_name);\n-\tcmd.git_cmd = 1;\n-\tcmd.in = -1;\n-\tcmd.out = -1;\n-\tif (start_command(&cmd))\n-\t\tdie(_(\"could not start pack-objects to repack local links\"));\n+\t\tif (!cmd.args.nr) {\n+\t\t\tbase_name = mkpathdup(\n+\t\t\t\t\"%s/pack/pack\",\n+\t\t\t\trepo_get_object_directory(the_repository));\n+\t\t\tstrvec_push(&cmd.args, \"pack-objects\");\n+\t\t\tstrvec_push(&cmd.args,\n+\t\t\t\t \"--exclude-promisor-objects-best-effort\");\n+\t\t\tstrvec_push(&cmd.args, base_name);\n+\t\t\tcmd.git_cmd = 1;\n+\t\t\tcmd.in = -1;\n+\t\t\tcmd.out = -1;\n+\t\t\tif (start_command(&cmd))\n+\t\t\t\tdie(_(\"could not start pack-objects to repack local links\"));\n+\t\t}\n \n-\toidset_iter_init(&local_links, &iter);\n-\twhile ((oid = oidset_iter_next(&iter))) {\n \t\tif (write_in_full(cmd.in, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||\n \t\t write_in_full(cmd.in, \"\\n\", 1) < 0)\n \t\t\tdie(_(\"failed to feed local object to pack-objects\"));\n \t}\n+\n+\tif (!cmd.args.nr)\n+\t\treturn;\n+\n \tclose(cmd.in);\n \n \tout = xfdopen(cmd.out, \"r\");\n@@ -1899,7 +1908,7 @@ int cmd_index_pack(int argc,\n \t\t\t} else if (skip_to_optional_arg(arg, \"--keep\", &keep_msg)) {\n \t\t\t\t; /* nothing to do */\n \t\t\t} else if (skip_to_optional_arg(arg, \"--promisor\", &promisor_msg)) {\n-\t\t\t\trecord_local_links = 1;\n+\t\t\t\trecord_outgoing_links = 1;\n \t\t\t} else if (starts_with(arg, \"--threads=\")) {\n \t\t\t\tchar *end;\n \t\t\t\tnr_threads = strtoul(arg+10, &end, 0);\n-- \n2.47.0.338.g60cca15819-goog\n\n"},{"id":"508569","messageId":"a1d2a202031e4b932d053b5c216ecd57ec9c728a.1733262662.git.jonathantanmy@google.com","threadId":"62588","inReplyTo":"cover.1733262661.git.jonathantanmy@google.com","subject":"[PATCH v3 2/3] index-pack --promisor: don't check blobs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-12-03T21:52:55Z","receivedAt":"2024-12-03T21:53:05Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"As a follow-up to the parent of this commit, it was found that not\nchecking for the existence of blobs linked from trees sped up the fetch\nfrom 24m47.815s to 2m2.127s. Teach Git to do that.\n\nThe tradeoff of not checking blobs is documented in a code comment.\n\n(Blobs may also be linked from tag objects, but it is impossible to know\nthe type of an object linked from a tag object without looking it up in\nthe object database, so the code for that is untouched.)\n\nSigned-off-by: Jonathan Tan \n---\n builtin/index-pack.c | 31 ++++++++++++++++++++++++++++++-\n 1 file changed, 30 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex d1c777a6af..2e90fe186e 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -817,6 +817,35 @@ static void record_outgoing_link(const struct object_id *oid)\n \toidset_insert(&outgoing_links, oid);\n }\n \n+static void maybe_record_name_entry(const struct name_entry *entry)\n+{\n+\t/*\n+\t * Checking only trees here results in a significantly faster packfile\n+\t * indexing, but the drawback is that if the packfile to be indexed\n+\t * references a local blob only directly (that is, never through a\n+\t * local tree), that local blob is in danger of being garbage\n+\t * collected. Such a situation may arise if we push local commits,\n+\t * including one with a change to a blob in the root tree, and then the\n+\t * server incorporates them into its main branch through a \"rebase\" or\n+\t * \"squash\" merge strategy, and then we fetch the new main branch from\n+\t * the server.\n+\t *\n+\t * This situation has not been observed yet - we have only noticed\n+\t * missing commits, not missing trees or blobs. (In fact, if it were\n+\t * believed that only missing commits are problematic, one could argue\n+\t * that we should also exclude trees during the outgoing link check;\n+\t * but it is safer to include them.)\n+\t *\n+\t * Due to the rarity of the situation (it has not been observed to\n+\t * happen in real life), and because the \"penalty\" in such a situation\n+\t * is merely to refetch the missing blob when it's needed (and this\n+\t * happens only once - when refetched, the blob goes into a promisor\n+\t * pack, so it won't be GC-ed, the tradeoff seems worth it.\n+\t*/\n+\tif (S_ISDIR(entry->mode))\n+\t\trecord_outgoing_link(&entry->oid);\n+}\n+\n static void do_record_outgoing_links(struct object *obj)\n {\n \tif (obj->type == OBJ_TREE) {\n@@ -831,7 +860,7 @@ static void do_record_outgoing_links(struct object *obj)\n \t\t\t */\n \t\t\treturn;\n \t\twhile (tree_entry_gently(&desc, &entry))\n-\t\t\trecord_outgoing_link(&entry.oid);\n+\t\t\tmaybe_record_name_entry(&entry);\n \t} else if (obj->type == OBJ_COMMIT) {\n \t\tstruct commit *commit = (struct commit *) obj;\n \t\tstruct commit_list *parents = commit->parents;\n-- \n2.47.0.338.g60cca15819-goog\n\n"},{"id":"508570","messageId":"f9f9969a8f399fda701b0ccd44dd124e953bf36d.1733262662.git.jonathantanmy@google.com","threadId":"62588","inReplyTo":"cover.1733262661.git.jonathantanmy@google.com","subject":"[PATCH v3 3/3] index-pack --promisor: also check commits' trees","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-12-03T21:52:56Z","receivedAt":"2024-12-03T21:53:06Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Commit c08589efdc (index-pack: repack local links into promisor packs,\n2024-11-01) seems to contain an oversight in that the tree of a commit\nis not checked. Teach git to check these trees.\n\nThe fix slows down a fetch from a certain repo at $DAYJOB from 2m2.127s\nto 2m45.052s, but in order to make the fetch correct, it seems worth it.\n\nIn order to test this, we could create server and client repos as\nfollows...\n\n C S\n \\ /\n O\n\n(O and C are commits both on the client and server. S is a commit\nonly on the server. C and S have the same tree but different commit\nmessages. The diff between O and C is non-zero.)\n\n...and then, from the client, fetch S from the server.\n\nIn theory, the client declares \"have C\" and the server can use this\ninformation to exclude S's tree (since it knows that the client has C's\ntree, which is the same as S's tree). However, it is also possible for\nthe server to compute that it needs to send S and not O, and proceed\nfrom there; therefore the objects of C are not considered at all when\ndetermining what to send in the packfile. In order to prevent a test of\nclient functionality from having such a dependence on server behavior, I\nhave not included such a test.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/index-pack.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 2e90fe186e..1594f2b81d 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -865,6 +865,7 @@ static void do_record_outgoing_links(struct object *obj)\n \t\tstruct commit *commit = (struct commit *) obj;\n \t\tstruct commit_list *parents = commit->parents;\n \n+\t\trecord_outgoing_link(get_commit_tree_oid(commit));\n \t\tfor (; parents; parents = parents->next)\n \t\t\trecord_outgoing_link(&parents->item->object.oid);\n \t} else if (obj->type == OBJ_TAG) {\n-- \n2.47.0.338.g60cca15819-goog\n\n"},{"id":"508574","messageId":"xmqq1pyocszr.fsf@gitster.g","threadId":"62588","inReplyTo":"Z06ejDgTnC6gWXgx@pks.im","subject":"Re: [PATCH 2/3] index-pack: no blobs during outgoing link check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-03T22:16:24Z","receivedAt":"2024-12-03T22:16:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> The benefit of doing this is as above (fetch speedup), but the drawback\n>> is that if the packfile to be indexed references a local blob directly\n>> (that is, not through a local tree), that local blob is in danger of\n>> being garbage collected. Such a situation may arise if we push local\n>> commits, including one with a change to a blob in the root tree,\n>> and then the server incorporates them into its main branch through a\n>> \"rebase\" or \"squash\" merge strategy, and then we fetch the new main\n>> branch from the server.\n>\n> Okay, so we know that we are basically doing the wrong thing with the\n> optimization, but by skipping blobs we can get a significant speedup and\n> the failure mode is that we will re-fetch the object in a later step.\n> And because we think the situation is rare it shouldn't be a huge issue\n> in practice.\n\nThat is how I read it, but the description may want to make the pros\nand cons more explicit. One of the reasons why the users choose to\nuse lazy clone is to avoid grabbing large blobs until they need\nthem, so I am not sure how they feel about having to discard and\nthen fetch again after creating a potentially large blob. As long\nas it does not happen repeatedly for the same blob, it probably is\nOK.\n\n> Without the context of the commit message this code snippet likely would\n> not make any sense to a reader. The \"correct\" logic would be to record\n> all objects, regardless of whether they are an object ID or not. But we\n> explicitly choose not to as a tradeoff between performance and\n> correctness.\n>\n> All to say that we should have a comment here that explains what is\n> going on.\n\nThanks for reviewing and commenting.\n\n"},{"id":"508590","messageId":"xmqq4j3k70x7.fsf@gitster.g","threadId":"62588","inReplyTo":"20241203214209.2033773-1-jonathantanmy@google.com","subject":"Re: [PATCH 3/3] index-pack: commit tree during outgoing link check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-04T00:21:40Z","receivedAt":"2024-12-04T00:21:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Junio C Hamano writes:\n>> > The fix slows down a fetch from a certain repo at\n>> > $DAYJOB from 2m2.127s to 2m45.052s, but in order to make the fetch\n>> > correct, it seems worth it.\n>> \n>> And \"the fix\" is not described so a reader is left wondering. Is\n>> the fix for an oversight of not checking merely to check it? IOW,\n>> is\n>> \n>> c08589efdc made outgoing links to be checked for commits, but\n>> failed to do so for trees. Make sure we check both\n>> \n>> what is happening?\n>\n> Yes. I was trying to keep to the character limit and in doing so, made\n> the commit message title hard to understand. I think the new title\n> should be easier to understand (and also stated explicitly in the commit\n> message what is being taught to Git).\n\nThanks.\n\n>> > However, it is also possible for\n>> > the server to compute that it needs to send S and not O, and proceed\n>> > from there;\n>> \n>> If O, C, and S have all identical trees, then wouldn't such a test\n>> work well? At that point it does not matter which between O and C \n>> the server bases its decision to send S but not S's tree on, no?\n>> \n>> In any case, will queue. Thanks.\n>\n> O has a different tree from C and S. I will add a note to clarify this.\n\nNo, that is not what I meant. \"If you arrange your test so that all\nthree have the same tree, then would't the reason why such a test\nwould not work you cited disappear and make this fix testable?\" is\nwhat I wanted to ask.\n\n"},{"id":"508594","messageId":"xmqq7c8g5gqn.fsf@gitster.g","threadId":"62588","inReplyTo":"cover.1733262661.git.jonathantanmy@google.com","subject":"Re: [PATCH v3 0/3] Performance improvements for repacking non-promisor objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-04T02:22:56Z","receivedAt":"2024-12-04T02:22:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Apparently I did not save in my text editor (and didn't notice because\n> the code comment was still valid syntactically, so everything still\n> compiled). Here's a version with the updated and correctly formatted\n> code comment.\n\nThanks. Will replace.\n"},{"id":"508597","messageId":"xmqqy10w3w0a.fsf@gitster.g","threadId":"62588","inReplyTo":"7ae21c921fe367d4b15cd4a299196009c15205d9.1733262662.git.jonathantanmy@google.com","subject":"Re: [PATCH v3 1/3] index-pack --promisor: dedup before checking links","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-04T04:36:05Z","receivedAt":"2024-12-04T04:36:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> @@ -1781,26 +1775,41 @@ static void repack_local_links(void)\n> \tstruct object_id *oid;\n> \tchar *base_name;\n\nWe may want to give a meaningless NULL initialization to this\nvariable, due to false positive from a compliler.\n\n> -\tif (!oidset_size(&local_links))\n> +\tif (!oidset_size(&outgoing_links))\n> \t\treturn;\n> \n> -\tbase_name = mkpathdup(\"%s/pack/pack\", repo_get_object_directory(the_repository));\n\nIt used to be that it was really obvious that base_name is always\ninitialized. But now due to micro-optimization ...\n\n> +\toidset_iter_init(&outgoing_links, &iter);\n> +\twhile ((oid = oidset_iter_next(&iter))) {\n> +\t\tstruct object_info info = OBJECT_INFO_INIT;\n> +\t\tif (oid_object_info_extended(the_repository, oid, &info, 0))\n> +\t\t\t/* Missing; assume it is a promisor object */\n> +\t\t\tcontinue;\n> +\t\tif (info.whence == OI_PACKED && info.u.packed.pack->pack_promisor)\n> +\t\t\tcontinue;\n> ...\n> +\t\tif (!cmd.args.nr) {\n> +\t\t\tbase_name = mkpathdup(\n> +\t\t\t\t\"%s/pack/pack\",\n> +\t\t\t\trepo_get_object_directory(the_repository));\n\n... we lazily allocate only after we know we will run a command.\n\n> +\t\t\tstrvec_push(&cmd.args, \"pack-objects\");\n> +\t\t\tstrvec_push(&cmd.args,\n> +\t\t\t\t \"--exclude-promisor-objects-best-effort\");\n> +\t\t\tstrvec_push(&cmd.args, base_name);\n> +\t\t\tcmd.git_cmd = 1;\n> +\t\t\tcmd.in = -1;\n> +\t\t\tcmd.out = -1;\n> +\t\t\tif (start_command(&cmd))\n> +\t\t\t\tdie(_(\"could not start pack-objects to repack local links\"));\n> +\t\t}\n\nWe know outgoing_links is not empty, so we know we will enter the\nwhile() loop at least once, but it may be possible that all the\nobjects in the outgoing_links oidset end up to be missing or packed\nin a promisor pack, hitting continue and never running the command.\n\n> -\toidset_iter_init(&local_links, &iter);\n> -\twhile ((oid = oidset_iter_next(&iter))) {\n> \t\tif (write_in_full(cmd.in, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||\n> \t\t write_in_full(cmd.in, \"\\n\", 1) < 0)\n> \t\t\tdie(_(\"failed to feed local object to pack-objects\"));\n> \t}\n> +\n> +\tif (!cmd.args.nr)\n> +\t\treturn;\n\nBut then we have this early return, so from human-reader's point of\nview, we will never hit free(base_name) at the end of this function.\n\nBut GCC used in the macOS build does not seem to realize it.\n\nhttps://github.com/git/git/actions/runs/12152173257/job/33888089229#step:4:380\n\nIt may be safer to give a meaningless NULL as the initial value of\nthe variable.\n\nThanks.\n"},{"id":"508598","messageId":"xmqqr06o3vid.fsf_-_@gitster.g","threadId":"62588","inReplyTo":"cover.1733262661.git.jonathantanmy@google.com","subject":"[PATCH 4/3] index-pack: work around false positive use of uninitialized variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-04T04:46:50Z","receivedAt":"2024-12-04T04:46:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The base_name variable in this function is given a value if cmd.args\narray is not empty (i.e., if we run the pack-objects command), and\nthe function returns when cmd.args is empty before hitting a call to\nfree(base_name) near the end of the function, so to a human reader,\nit can be seen that the variable is not used uninitialized, but to a\nsemi-intelligent compiler it is not so clear.\n\nSquelch a false positive by a meaningless NULL initialization.\n\nSigned-off-by: Junio C Hamano \n---\n\n * Tentatively queued to unblock CI. There may be breakages due to\n other topics in flight, but at least this one is easy to resolve\n (hopefully---I haven't pushed it out).\n\n https://github.com/git/git/actions/runs/12152173257\n\n builtin/index-pack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 1594f2b81d..8e600a58bf 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1803,7 +1803,7 @@ static void repack_local_links(void)\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct oidset_iter iter;\n \tstruct object_id *oid;\n-\tchar *base_name;\n+\tchar *base_name = NULL;\n \n \tif (!oidset_size(&outgoing_links))\n \t\treturn;\n-- \n2.47.1-574-g3b2d6bb55a\n\n"},{"id":"508885","messageId":"20241209202935.799059-1-jonathantanmy@google.com","threadId":"62588","inReplyTo":"xmqq4j3k70x7.fsf@gitster.g","subject":"Re: [PATCH 3/3] index-pack: commit tree during outgoing link check","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-12-09T20:29:35Z","receivedAt":"2024-12-09T20:29:38Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Junio C Hamano writes:\n> >> > However, it is also possible for\n> >> > the server to compute that it needs to send S and not O, and proceed\n> >> > from there;\n> >> \n> >> If O, C, and S have all identical trees, then wouldn't such a test\n> >> work well? At that point it does not matter which between O and C \n> >> the server bases its decision to send S but not S's tree on, no?\n> >> \n> >> In any case, will queue. Thanks.\n> >\n> > O has a different tree from C and S. I will add a note to clarify this.\n> \n> No, that is not what I meant. \"If you arrange your test so that all\n> three have the same tree, then would't the reason why such a test\n> would not work you cited disappear and make this fix testable?\" is\n> what I wanted to ask.\n\nCopying and pasting the diagram for reference:\n\n C S\n \\ /\n O\n\nI looked into this and I don't think that making O, C, and S have the\nsame tree will make this fix testable. If they all had the same tree,\nwhether we check S's tree or not doesn't matter, since that tree is\nalready in a promisor pack (O and its tree was previously fetched from\nthe promisor remote, and thus is in a promisor pack). (We are checking\ntrees in order to repack them into promisor packs if necessary.)\n"},{"id":"508894","messageId":"xmqqseqwmn4j.fsf@gitster.g","threadId":"62588","inReplyTo":"20241209202935.799059-1-jonathantanmy@google.com","subject":"Re: [PATCH 3/3] index-pack: commit tree during outgoing link check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-09T23:51:08Z","receivedAt":"2024-12-09T23:51:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Copying and pasting the diagram for reference:\n>\n> C S\n> \\ /\n> O\n>\n> I looked into this and I don't think that making O, C, and S have the\n> same tree will make this fix testable. If they all had the same tree,\n> whether we check S's tree or not doesn't matter, since that tree is\n> already in a promisor pack (O and its tree was previously fetched from\n> the promisor remote, and thus is in a promisor pack). (We are checking\n> trees in order to repack them into promisor packs if necessary.)\n\nI see; thanks.\n"}]}