{"thread":{"id":"54178","subject":"[PATCH] fetch: do not look for submodule changes in unchanged refs","startedAt":"2020-09-02T14:27:08Z","lastAt":"2020-09-07T15:50:12Z","messageCount":4,"participants":["Orgad Shaneh via GitGitGadget","Junio C Hamano","Orgad Shaneh"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"404915","messageId":"pull.720.git.1599056635276.gitgitgadget@gmail.com","threadId":"54178","inReplyTo":null,"subject":"[PATCH] fetch: do not look for submodule changes in unchanged refs","fromName":"Orgad Shaneh via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-09-02T14:23:55Z","receivedAt":"2020-09-02T14:27:08Z","isPatch":true,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"From: Orgad Shaneh \n\nThis operation is very expensive, as it scans all the refs using\nsetup_revisions, which resolves each ref, including checking if it\nis ambiguous, or if it is a file name etc.\n\nThere is no reason to do all that for refs that haven't changed in this\nfetch.\n\nReported here:\nhttps://public-inbox.org/git/CAGHpTBKSUJzFSWc=uznSu2zB33qCSmKXM-iAjxRCpqNK5bnhRg@mail.gmail.com/\n\nAmends commit be76c2128234d94b47f7087152ee55d08bb65d88.\n\nSigned-off-by: Orgad Shaneh \n---\n fetch: do not look for submodule changes in unchanged refs\n \n This operation is very expensive, as it scans all the refs using\n setup_revisions, which resolves each ref, including checking if it is\n ambiguous, or if it is a file name etc.\n \n There is no reason to do all that for refs that hasn't changed in this\n fetch.\n \n Reported here:\n https://public-inbox.org/git/CAGHpTBKSUJzFSWc=uznSu2zB33qCSmKXM-iAjxRCpqNK5bnhRg@mail.gmail.com/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-720%2Forgads%2Ffetch-less-submodules-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-720/orgads/fetch-less-submodules-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/720\n\n builtin/fetch.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 0f23dd4b8c..d3f922fc89 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -958,8 +958,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t\tref->force = rm->peer_ref->force;\n \t\t\t}\n \n-\t\t\tif (recurse_submodules != RECURSE_SUBMODULES_OFF)\n+\t\t\tif (recurse_submodules != RECURSE_SUBMODULES_OFF &&\n+\t\t\t (!rm->peer_ref || !oideq(&ref->old_oid, &ref->new_oid))) {\n \t\t\t\tcheck_for_new_submodule_commits(&rm->old_oid);\n+\t\t\t}\n \n \t\t\tif (!strcmp(rm->name, \"HEAD\")) {\n \t\t\t\tkind = \"\";\n\nbase-commit: e19713638985533ce461db072b49112da5bd2042\n-- \ngitgitgadget\n"},{"id":"404940","messageId":"xmqq3640apsl.fsf@gitster.c.googlers.com","threadId":"54178","inReplyTo":"pull.720.git.1599056635276.gitgitgadget@gmail.com","subject":"Re: [PATCH] fetch: do not look for submodule changes in unchanged refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-02T20:26:18Z","receivedAt":"2020-09-02T20:26:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Orgad Shaneh via GitGitGadget\" writes:\n\n> From: Orgad Shaneh \n>\n> This operation is very expensive, as it scans all the refs using\n> setup_revisions, which resolves each ref, including checking if it\n> is ambiguous, or if it is a file name etc.\n\nNobody can tell what \"This operation\" is without looking at the\npatch/diff text. Our commit message typically gives minimum\nexplanation of the situation and the problem it tries to solve first\nto make it self sufficient. And then we go on to order the code\nbase to be in a better shape. Something along the lines of ...\n\n When fetching recursively with submodules, for each ref in the\n superproject, we call check_for_new_submodule_commits() to\n figure out X and Y for the object the ref was pointing at before\n the fetch in the superproject, in order to ensure Z. This is\n expensive because of A, B and C, but it unnecessary if the fetch\n in the superproject did not update the ref (i.e. the objects\n that are required to exist in the submodule did not change).\n\n Check if we are making any change to the ref, and skip the check\n if we aren't.\n\n... but I didn't fill the most important bits in the above, as by\nnow you, as the person who encountered the issue and figured out a\ngood way to solve it, would know what to fill the placeholders with\nfar better than I would ;-)\n\n\n> There is no reason to do all that for refs that haven't changed in this\n> fetch.\n>\n> Reported here:\n> https://public-inbox.org/git/CAGHpTBKSUJzFSWc=uznSu2zB33qCSmKXM-iAjxRCpqNK5bnhRg@mail.gmail.com/\n>\n> Amends commit be76c2128234d94b47f7087152ee55d08bb65d88.\n\nI am not sure what this reference is trying to achieve. Fixing a\nbug in be76c212 (fetch: ensure submodule objects fetched,\n2018-12-06)? If so, please say so more directly, perhaps like\n\n be76c212 (fetch: ensure submodule objects fetched, 2018-12-06)\n tried to do what we are trying to do here, but it botched the\n exectuion by forgetting the fact that ...\n\nor somesuch. The cited commit says\n\n The submodule checks were done only when a ref in the\n superproject changed,...\n\nso it is not clear what we are really fixing with this patch,\nthough. Is the assertion \"checks were done only when changed\"\nit made incorrect and instead we were doing unnecessary check\nalways?\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 0f23dd4b8c..d3f922fc89 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -958,8 +958,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n> \t\t\t\tref->force = rm->peer_ref->force;\n> \t\t\t}\n> \n> -\t\t\tif (recurse_submodules != RECURSE_SUBMODULES_OFF)\n> +\t\t\tif (recurse_submodules != RECURSE_SUBMODULES_OFF &&\n> +\t\t\t (!rm->peer_ref || !oideq(&ref->old_oid, &ref->new_oid))) {\n> \t\t\t\tcheck_for_new_submodule_commits(&rm->old_oid);\n> +\t\t\t}\n\nThe original before be76c212 fed ref->new_oid to the check\nfunction. Now that we are using ref->{old,new}_oid in the\ncondition, would it make more sense to pass ref->new_oid\nlike we did before the commit, or is that an object that is\ndifferent from rm->old_oid?\n\nThanks.\n\n> \t\t\tif (!strcmp(rm->name, \"HEAD\")) {\n> \t\t\t\tkind = \"\";\n>\n> base-commit: e19713638985533ce461db072b49112da5bd2042\n"},{"id":"405036","messageId":"pull.720.v2.git.1599227449987.gitgitgadget@gmail.com","threadId":"54178","inReplyTo":"pull.720.git.1599056635276.gitgitgadget@gmail.com","subject":"[PATCH v2] fetch: do not look for submodule changes in unchanged refs","fromName":"Orgad Shaneh via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-09-04T13:50:49Z","receivedAt":"2020-09-04T13:51:36Z","isPatch":true,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"From: Orgad Shaneh \n\nWhen fetching recursively with submodules, for each ref in the\nsuperproject, we call check_for_new_submodule_commits() which collects all\nthe objects that have to be checked for submodule changes on\ncalculate_changed_submodule_paths(). On the first call, it also collects all\nthe existing refs for excluding them from the scan.\n\ncalculate_changed_submodule_paths() creates an argument array with all the\ncollected new objects, followed by --not and all the old objects. This argv\nis passed to setup_revisions, which parses each argument, converts it back\nto an oid and resolves the object. The parsing itself also does redundant\nwork, because it is treated like user input, while in fact it is a full\noid. So it needlessly attempts to look it up as ref (checks if it has ^, ~\netc.), checks if it is a file name etc.\n\nFor a repository with many refs, all of this is expensive. But if the fetch\nin the superproject did not update the ref (i.e. the objects that are\nrequired to exist in the submodule did not change), there is no need to\ninclude it in the list.\n\nBefore commit be76c212 (fetch: ensure submodule objects fetched,\n2018-12-06), submodule reference changes were only detected for refs that\nwere changed, but not for new refs. This commit covered also this case, but\nwhat it did was to just include every ref.\n\nThis change should reduce the number of scanned refs by about half (except\nthe case of a no-op fetch, which will not scan any ref), because all the\nexisting refs will still be listed after --not.\n\nThe regression was reported here:\nhttps://public-inbox.org/git/CAGHpTBKSUJzFSWc=uznSu2zB33qCSmKXM-\niAjxRCpqNK5bnhRg@mail.gmail.com/\n\nSigned-off-by: Orgad Shaneh \n---\n fetch: do not look for submodule changes in unchanged refs\n \n This operation is very expensive, as it scans all the refs using\n setup_revisions, which resolves each ref, including checking if it is\n ambiguous, or if it is a file name etc.\n \n There is no reason to do all that for refs that hasn't changed in this\n fetch.\n \n Reported here:\n https://public-inbox.org/git/CAGHpTBKSUJzFSWc=uznSu2zB33qCSmKXM-iAjxRCpqNK5bnhRg@mail.gmail.com/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-720%2Forgads%2Ffetch-less-submodules-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-720/orgads/fetch-less-submodules-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/720\n\nRange-diff vs v1:\n\n 1: 0f978f487d ! 1: 5348cd7ec8 fetch: do not look for submodule changes in unchanged refs\n @@ Metadata\n ## Commit message ##\n fetch: do not look for submodule changes in unchanged refs\n \n - This operation is very expensive, as it scans all the refs using\n - setup_revisions, which resolves each ref, including checking if it\n - is ambiguous, or if it is a file name etc.\n + When fetching recursively with submodules, for each ref in the\n + superproject, we call check_for_new_submodule_commits() which collects all\n + the objects that have to be checked for submodule changes on\n + calculate_changed_submodule_paths(). On the first call, it also collects all\n + the existing refs for excluding them from the scan.\n \n - There is no reason to do all that for refs that haven't changed in this\n - fetch.\n + calculate_changed_submodule_paths() creates an argument array with all the\n + collected new objects, followed by --not and all the old objects. This argv\n + is passed to setup_revisions, which parses each argument, converts it back\n + to an oid and resolves the object. The parsing itself also does redundant\n + work, because it is treated like user input, while in fact it is a full\n + oid. So it needlessly attempts to look it up as ref (checks if it has ^, ~\n + etc.), checks if it is a file name etc.\n \n - Reported here:\n - https://public-inbox.org/git/CAGHpTBKSUJzFSWc=uznSu2zB33qCSmKXM-iAjxRCpqNK5bnhRg@mail.gmail.com/\n + For a repository with many refs, all of this is expensive. But if the fetch\n + in the superproject did not update the ref (i.e. the objects that are\n + required to exist in the submodule did not change), there is no need to\n + include it in the list.\n \n - Amends commit be76c2128234d94b47f7087152ee55d08bb65d88.\n + Before commit be76c212 (fetch: ensure submodule objects fetched,\n + 2018-12-06), submodule reference changes were only detected for refs that\n + were changed, but not for new refs. This commit covered also this case, but\n + what it did was to just include every ref.\n +\n + This change should reduce the number of scanned refs by about half (except\n + the case of a no-op fetch, which will not scan any ref), because all the\n + existing refs will still be listed after --not.\n +\n + The regression was reported here:\n + https://public-inbox.org/git/CAGHpTBKSUJzFSWc=uznSu2zB33qCSmKXM-\n + iAjxRCpqNK5bnhRg@mail.gmail.com/\n \n Signed-off-by: Orgad Shaneh \n \n\n\n builtin/fetch.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 0f23dd4b8c..d3f922fc89 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -958,8 +958,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t\tref->force = rm->peer_ref->force;\n \t\t\t}\n \n-\t\t\tif (recurse_submodules != RECURSE_SUBMODULES_OFF)\n+\t\t\tif (recurse_submodules != RECURSE_SUBMODULES_OFF &&\n+\t\t\t (!rm->peer_ref || !oideq(&ref->old_oid, &ref->new_oid))) {\n \t\t\t\tcheck_for_new_submodule_commits(&rm->old_oid);\n+\t\t\t}\n \n \t\t\tif (!strcmp(rm->name, \"HEAD\")) {\n \t\t\t\tkind = \"\";\n\nbase-commit: e19713638985533ce461db072b49112da5bd2042\n-- \ngitgitgadget\n"},{"id":"405125","messageId":"CAGHpTB+6spDbsXKpUYXhxbAST4qd-3f9Czga=XmAdRhm4Z-XnA@mail.gmail.com","threadId":"54178","inReplyTo":"xmqq3640apsl.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] fetch: do not look for submodule changes in unchanged refs","fromName":"Orgad Shaneh","fromEmail":"orgads@gmail.com","sentAt":"2020-09-07T15:49:20Z","receivedAt":"2020-09-07T15:50:12Z","isPatch":true,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"Hi Junio,\n\nThanks for the detailed review. I posted a new commit message.\n\nOn Wed, Sep 2, 2020 at 11:26 PM Junio C Hamano wrote:\n>\n> \"Orgad Shaneh via GitGitGadget\" writes:\n>\n> > From: Orgad Shaneh \n> >\n> > This operation is very expensive, as it scans all the refs using\n> > setup_revisions, which resolves each ref, including checking if it\n> > is ambiguous, or if it is a file name etc.\n>\n> Nobody can tell what \"This operation\" is without looking at the\n> patch/diff text. Our commit message typically gives minimum\n> explanation of the situation and the problem it tries to solve first\n> to make it self sufficient. And then we go on to order the code\n> base to be in a better shape. Something along the lines of ...\n>\n> When fetching recursively with submodules, for each ref in the\n> superproject, we call check_for_new_submodule_commits() to\n> figure out X and Y for the object the ref was pointing at before\n> the fetch in the superproject, in order to ensure Z. This is\n> expensive because of A, B and C, but it unnecessary if the fetch\n> in the superproject did not update the ref (i.e. the objects\n> that are required to exist in the submodule did not change).\n>\n> Check if we are making any change to the ref, and skip the check\n> if we aren't.\n>\n> ... but I didn't fill the most important bits in the above, as by\n> now you, as the person who encountered the issue and figured out a\n> good way to solve it, would know what to fill the placeholders with\n> far better than I would ;-)\n\nThat was very helpful. Thanks.\n\n> [... snip ...]\n> > diff --git a/builtin/fetch.c b/builtin/fetch.c\n> > index 0f23dd4b8c..d3f922fc89 100644\n> > --- a/builtin/fetch.c\n> > +++ b/builtin/fetch.c\n> > @@ -958,8 +958,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n> > ref->force = rm->peer_ref->force;\n> > }\n> >\n> > - if (recurse_submodules != RECURSE_SUBMODULES_OFF)\n> > + if (recurse_submodules != RECURSE_SUBMODULES_OFF &&\n> > + (!rm->peer_ref || !oideq(&ref->old_oid, &ref->new_oid))) {\n> > check_for_new_submodule_commits(&rm->old_oid);\n> > + }\n>\n> The original before be76c212 fed ref->new_oid to the check\n> function. Now that we are using ref->{old,new}_oid in the\n> condition, would it make more sense to pass ref->new_oid\n> like we did before the commit, or is that an object that is\n> different from rm->old_oid?\n\nI think that was the whole point of this commit, to cover the case\nof !rm->peer_ref, for newly fetched refs. On this case, ref is NULL.\n\n> Thanks.\n>\n> > if (!strcmp(rm->name, \"HEAD\")) {\n> > kind = \"\";\n> >\n> > base-commit: e19713638985533ce461db072b49112da5bd2042\n\n- Orgad\n"}]}