{"thread":{"id":"49773","subject":"[PATCH 0/1] send-pack: set core.warnAmbiguousRefs=false","startedAt":"2018-11-06T19:13:50Z","lastAt":"2018-11-07T09:21:34Z","messageCount":12,"participants":["Derrick Stolee via GitGitGadget","Jeff King","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"362600","messageId":"pull.68.git.gitgitgadget@gmail.com","threadId":"49773","inReplyTo":null,"subject":"[PATCH 0/1] send-pack: set core.warnAmbiguousRefs=false","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-06T19:13:47Z","receivedAt":"2018-11-06T19:13:50Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"I've been looking into the performance of git push for very large repos. Our\nusers are reporting that 60-80% of git push time is spent during the\n\"Enumerating objects\" phase of git pack-objects.\n\nA git push process runs several processes during its run, but one includes \ngit send-pack which calls git pack-objects and passes the known have/wants\ninto stdin using object ids. However, the default setting for \ncore.warnAmbiguousRefs requires git pack-objects to check for ref names\nmatching the ref_rev_parse_rules array in refs.c. This means that every\nobject is triggering at least six \"file exists?\" queries.\n\nWhen there are a lot of refs, this can add up significantly! My PerfView\ntrace for a simple push measured 3 seconds spent checking these paths.\n\nThe fix for this is simple: set core.warnAmbiguousRefs to false for this\nspecific call of git pack-objects coming from git send-pack. We don't want\nto default it to false for all calls to git pack-objects, as it is valid to\npass ref names instead of object ids. This helps regain these seconds during\na push.\n\nIn addition to this patch submission, we are looking into merging it into\nour fork sooner [1].\n\n[1] https://github.com/Microsoft/git/pull/67\n\nDerrick Stolee (1):\n  send-pack: set core.warnAmbiguousRefs=false\n\n send-pack.c | 2 ++\n 1 file changed, 2 insertions(+)\n\n\nbase-commit: cae598d9980661a978e2df4fb338518f7bf09572\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-68%2Fderrickstolee%2Fsend-pack-config-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-68/derrickstolee/send-pack-config-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/68\n-- \ngitgitgadget\n"},{"id":"362601","messageId":"1ef2c51550c57bddc64426081eac2dde3349fb05.1541531625.git.gitgitgadget@gmail.com","threadId":"49773","inReplyTo":"pull.68.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] send-pack: set core.warnAmbiguousRefs=false","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-06T19:13:48Z","receivedAt":"2018-11-06T19:13:51Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nDuring a 'git push' command, we run 'git send-pack' inside of our\ntransport helper. This creates a 'git pack-objects' process and\npasses a list of object ids. If this list is large, then the\npack-objects process can spend a lot of time checking the possible\nrefs these strings could represent.\n\nRemove this extra check by setting core.warnAmbiguousRefs to false\nas we call 'git pack-objects'.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n send-pack.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex e920ca57df..5055150fe1 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -64,6 +64,8 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *extra, struc\n \tint i;\n \tint rc;\n \n+\targv_array_push(&po.args, \"-c\");\n+\targv_array_push(&po.args, \"core.warnAmbiguousRefs=false\");\n \targv_array_push(&po.args, \"pack-objects\");\n \targv_array_push(&po.args, \"--all-progress-implied\");\n \targv_array_push(&po.args, \"--revs\");\n-- \ngitgitgadget\n"},{"id":"362603","messageId":"20181106194442.GB8902@sigill.intra.peff.net","threadId":"49773","inReplyTo":"pull.68.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/1] send-pack: set core.warnAmbiguousRefs=false","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-06T19:44:43Z","receivedAt":"2018-11-06T19:44:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 06, 2018 at 11:13:47AM -0800, Derrick Stolee via GitGitGadget wrote:\n\n> I've been looking into the performance of git push for very large repos. Our\n> users are reporting that 60-80% of git push time is spent during the\n> \"Enumerating objects\" phase of git pack-objects.\n> \n> A git push process runs several processes during its run, but one includes \n> git send-pack which calls git pack-objects and passes the known have/wants\n> into stdin using object ids. However, the default setting for \n> core.warnAmbiguousRefs requires git pack-objects to check for ref names\n> matching the ref_rev_parse_rules array in refs.c. This means that every\n> object is triggering at least six \"file exists?\" queries.\n> \n> When there are a lot of refs, this can add up significantly! My PerfView\n> trace for a simple push measured 3 seconds spent checking these paths.\n\nSome of this might be useful in the commit message. :)\n\n> The fix for this is simple: set core.warnAmbiguousRefs to false for this\n> specific call of git pack-objects coming from git send-pack. We don't want\n> to default it to false for all calls to git pack-objects, as it is valid to\n> pass ref names instead of object ids. This helps regain these seconds during\n> a push.\n\nI don't think you actually care about the ambiguity check between refs\nhere; you just care about avoiding the ref check when we've seen (and\nare mostly expecting) a 40-hex sha1. We have a more specific flag for\nthat: warn_on_object_refname_ambiguity.\n\nAnd I think it would be OK to enable that all the time for pack-objects,\nwhich is plumbing that does typically expect object names. See prior art\nin 25fba78d36 (cat-file: disable object/refname ambiguity check for\nbatch mode, 2013-07-12) and 4c30d50402 (rev-list: disable object/refname\nambiguity check with --stdin, 2014-03-12).\n\n> Derrick Stolee (1):\n>   send-pack: set core.warnAmbiguousRefs=false\n> \n>  send-pack.c | 2 ++\n>  1 file changed, 2 insertions(+)\n\nWhenever I see a change like this to the pack-objects invocation for\nsend-pack, it makes me wonder if upload-pack would want the same thing.\n\nIt's a moot point if we just set the flag directly in inside\npack-objects, though.\n\n-Peff\n"},{"id":"362604","messageId":"20181106195131.GA5289@sigill.intra.peff.net","threadId":"49773","inReplyTo":"20181106194442.GB8902@sigill.intra.peff.net","subject":"Re: [PATCH 0/1] send-pack: set core.warnAmbiguousRefs=false","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-06T19:51:32Z","receivedAt":"2018-11-06T19:51:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 06, 2018 at 02:44:42PM -0500, Jeff King wrote:\n\n> > The fix for this is simple: set core.warnAmbiguousRefs to false for this\n> > specific call of git pack-objects coming from git send-pack. We don't want\n> > to default it to false for all calls to git pack-objects, as it is valid to\n> > pass ref names instead of object ids. This helps regain these seconds during\n> > a push.\n> \n> I don't think you actually care about the ambiguity check between refs\n> here; you just care about avoiding the ref check when we've seen (and\n> are mostly expecting) a 40-hex sha1. We have a more specific flag for\n> that: warn_on_object_refname_ambiguity.\n> \n> And I think it would be OK to enable that all the time for pack-objects,\n> which is plumbing that does typically expect object names. See prior art\n> in 25fba78d36 (cat-file: disable object/refname ambiguity check for\n> batch mode, 2013-07-12) and 4c30d50402 (rev-list: disable object/refname\n> ambiguity check with --stdin, 2014-03-12).\n\nI'd probably do it here:\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex e50c6cd1ff..d370638a5d 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3104,6 +3104,7 @@ static void get_object_list(int ac, const char **av)\n \tstruct rev_info revs;\n \tchar line[1000];\n \tint flags = 0;\n+\tint save_warning;\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n \tsave_commit_buffer = 0;\n@@ -3112,6 +3113,9 @@ static void get_object_list(int ac, const char **av)\n \t/* make sure shallows are read */\n \tis_repository_shallow(the_repository);\n \n+\tsave_warning = warn_on_object_refname_ambiguity;\n+\twarn_on_object_refname_ambiguity = 0;\n+\n \twhile (fgets(line, sizeof(line), stdin) != NULL) {\n \t\tint len = strlen(line);\n \t\tif (len && line[len - 1] == '\\n')\n@@ -3138,6 +3142,8 @@ static void get_object_list(int ac, const char **av)\n \t\t\tdie(_(\"bad revision '%s'\"), line);\n \t}\n \n+\twarn_on_object_refname_ambiguity = save_warning;\n+\n \tif (use_bitmap_index && !get_object_list_from_bitmap(&revs))\n \t\treturn;\n \n\nBut I'll leave it to you to wrap that up in a patch, since you probably\nshould re-check your timings (which it would be interesting to include\nin the commit message, if you have reproducible timings).\n\n-Peff\n"},{"id":"362605","messageId":"1e92f560-a5c0-2d58-5a4c-8530598d1025@gmail.com","threadId":"49773","inReplyTo":"20181106194442.GB8902@sigill.intra.peff.net","subject":"Re: [PATCH 0/1] send-pack: set core.warnAmbiguousRefs=false","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-11-06T20:00:58Z","receivedAt":"2018-11-06T20:01:00Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/6/2018 2:44 PM, Jeff King wrote:\n> On Tue, Nov 06, 2018 at 11:13:47AM -0800, Derrick Stolee via GitGitGadget wrote:\n>\n>> I've been looking into the performance of git push for very large repos. Our\n>> users are reporting that 60-80% of git push time is spent during the\n>> \"Enumerating objects\" phase of git pack-objects.\n>>\n>> A git push process runs several processes during its run, but one includes\n>> git send-pack which calls git pack-objects and passes the known have/wants\n>> into stdin using object ids. However, the default setting for\n>> core.warnAmbiguousRefs requires git pack-objects to check for ref names\n>> matching the ref_rev_parse_rules array in refs.c. This means that every\n>> object is triggering at least six \"file exists?\" queries.\n>>\n>> When there are a lot of refs, this can add up significantly! My PerfView\n>> trace for a simple push measured 3 seconds spent checking these paths.\n> Some of this might be useful in the commit message. :)\n>\n>> The fix for this is simple: set core.warnAmbiguousRefs to false for this\n>> specific call of git pack-objects coming from git send-pack. We don't want\n>> to default it to false for all calls to git pack-objects, as it is valid to\n>> pass ref names instead of object ids. This helps regain these seconds during\n>> a push.\n> I don't think you actually care about the ambiguity check between refs\n> here; you just care about avoiding the ref check when we've seen (and\n> are mostly expecting) a 40-hex sha1. We have a more specific flag for\n> that: warn_on_object_refname_ambiguity.\n>\n> And I think it would be OK to enable that all the time for pack-objects,\n> which is plumbing that does typically expect object names. See prior art\n> in 25fba78d36 (cat-file: disable object/refname ambiguity check for\n> batch mode, 2013-07-12) and 4c30d50402 (rev-list: disable object/refname\n> ambiguity check with --stdin, 2014-03-12).\nThanks for these pointers. Helps to know there is precedent for shutting \ndown the\nbehavior without relying on \"-c\" flags.\n\n> Whenever I see a change like this to the pack-objects invocation for\n> send-pack, it makes me wonder if upload-pack would want the same thing.\n>\n> It's a moot point if we just set the flag directly in inside\n> pack-objects, though.\nI'll send a v2 that does just that.\n\nThanks,\n-Stolee\n"},{"id":"362609","messageId":"f2ff23be-9f26-6296-f7a4-b95f9f2ec1f6@gmail.com","threadId":"49773","inReplyTo":"20181106195131.GA5289@sigill.intra.peff.net","subject":"Re: [PATCH 0/1] send-pack: set core.warnAmbiguousRefs=false","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-11-06T20:16:35Z","receivedAt":"2018-11-06T20:16:37Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/6/2018 2:51 PM, Jeff King wrote:\n> On Tue, Nov 06, 2018 at 02:44:42PM -0500, Jeff King wrote:\n>\n>>> The fix for this is simple: set core.warnAmbiguousRefs to false for this\n>>> specific call of git pack-objects coming from git send-pack. We don't want\n>>> to default it to false for all calls to git pack-objects, as it is valid to\n>>> pass ref names instead of object ids. This helps regain these seconds during\n>>> a push.\n>> I don't think you actually care about the ambiguity check between refs\n>> here; you just care about avoiding the ref check when we've seen (and\n>> are mostly expecting) a 40-hex sha1. We have a more specific flag for\n>> that: warn_on_object_refname_ambiguity.\n>>\n>> And I think it would be OK to enable that all the time for pack-objects,\n>> which is plumbing that does typically expect object names. See prior art\n>> in 25fba78d36 (cat-file: disable object/refname ambiguity check for\n>> batch mode, 2013-07-12) and 4c30d50402 (rev-list: disable object/refname\n>> ambiguity check with --stdin, 2014-03-12).\n> I'd probably do it here:\n>\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index e50c6cd1ff..d370638a5d 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -3104,6 +3104,7 @@ static void get_object_list(int ac, const char **av)\n\nScoping the change into get_object_list does make sense. I was doing it \na level higher, which is not worth it. I'll reproduce your change here.\n\n>   \tstruct rev_info revs;\n>   \tchar line[1000];\n>   \tint flags = 0;\n> +\tint save_warning;\n>   \n>   \trepo_init_revisions(the_repository, &revs, NULL);\n>   \tsave_commit_buffer = 0;\n> @@ -3112,6 +3113,9 @@ static void get_object_list(int ac, const char **av)\n>   \t/* make sure shallows are read */\n>   \tis_repository_shallow(the_repository);\n>   \n> +\tsave_warning = warn_on_object_refname_ambiguity;\n> +\twarn_on_object_refname_ambiguity = 0;\n> +\n>   \twhile (fgets(line, sizeof(line), stdin) != NULL) {\n>   \t\tint len = strlen(line);\n>   \t\tif (len && line[len - 1] == '\\n')\n> @@ -3138,6 +3142,8 @@ static void get_object_list(int ac, const char **av)\n>   \t\t\tdie(_(\"bad revision '%s'\"), line);\n>   \t}\n>   \n> +\twarn_on_object_refname_ambiguity = save_warning;\n> +\n>   \tif (use_bitmap_index && !get_object_list_from_bitmap(&revs))\n>   \t\treturn;\n>   \n>\n> But I'll leave it to you to wrap that up in a patch, since you probably\n> should re-check your timings (which it would be interesting to include\n> in the commit message, if you have reproducible timings).\n\nThe timings change a lot depending on the disk cache and the remote \nrefs, which is unfortunate, but I have measured a three-second improvement.\n\nThanks,\n-Stolee\n"},{"id":"362610","messageId":"pull.68.v2.git.gitgitgadget@gmail.com","threadId":"49773","inReplyTo":"pull.68.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/1] send-pack: set core.warnAmbiguousRefs=false","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-06T20:34:46Z","receivedAt":"2018-11-06T20:34:49Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"I've been looking into the performance of git push for very large repos. Our\nusers are reporting that 60-80% of git push time is spent during the\n\"Enumerating objects\" phase of git pack-objects.\n\nA git push process runs several processes during its run, but one includes \ngit send-pack which calls git pack-objects and passes the known have/wants\ninto stdin using object ids. However, the default setting for \ncore.warnAmbiguousRefs requires git pack-objects to check for ref names\nmatching the ref_rev_parse_rules array in refs.c. This means that every\nobject is triggering at least six \"file exists?\" queries.\n\nWhen there are a lot of refs, this can add up significantly! My PerfView\ntrace for a simple push measured 3 seconds spent checking these paths.\n\nThe fix is to set the global warn_on_object_refname_ambiguity to 0 for the\nsection that is performing these object reads.\n\nIn addition to this patch submission, we are looking into merging it into\nour fork sooner [1].\n\n[1] https://github.com/Microsoft/git/pull/67\n\nChanges in V2: Instead of using the \"-c\" flag from send-pack, just set the\nglobal. I left the name of the cover letter the same to not confuse anyone\nviewing the message without threading.\n\nDerrick Stolee (1):\n  pack-objects: ignore ambiguous object warnings\n\n builtin/pack-objects.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\n\nbase-commit: cae598d9980661a978e2df4fb338518f7bf09572\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-68%2Fderrickstolee%2Fsend-pack-config-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-68/derrickstolee/send-pack-config-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/68\n\nRange-diff vs v1:\n\n 1:  1ef2c51550 < -:  ---------- send-pack: set core.warnAmbiguousRefs=false\n -:  ---------- > 1:  002868ee6b pack-objects: ignore ambiguous object warnings\n\n-- \ngitgitgadget\n"},{"id":"362611","messageId":"002868ee6bec3dac38749d0f05bf2db8da0969a5.1541536484.git.gitgitgadget@gmail.com","threadId":"49773","inReplyTo":"pull.68.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/1] pack-objects: ignore ambiguous object warnings","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-06T20:34:47Z","receivedAt":"2018-11-06T20:35:03Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nA git push process runs several processes during its run, but one\nincludes git send-pack which calls git pack-objects and passes\nthe known have/wants into stdin using object ids. However, the\ndefault setting for core.warnAmbiguousRefs requires git pack-objects\nto check for ref names matching the ref_rev_parse_rules array in\nrefs.c. This means that every object is triggering at least six\n\"file exists?\" queries.  When there are a lot of refs, this can\nadd up significantly! I observed a simple push spending three\nseconds checking these paths.\n\nThe fix here is similar to 4c30d50 \"rev-list: disable object/refname\nambiguity check with --stdin\". Save the value of the global\nwarn_on_object_refname_ambiguity variable (which is usually set to\nthe boolean config variable core.warnAmbiguousRefs) and change the\nstate to false. Do this only during the get_object_list() method\nwhich reads the objects from stdin.\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n builtin/pack-objects.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex d1144a8f7e..f703e6df9b 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2988,6 +2988,7 @@ static void get_object_list(int ac, const char **av)\n \tstruct rev_info revs;\n \tchar line[1000];\n \tint flags = 0;\n+\tint save_warning;\n \n \tinit_revisions(&revs, NULL);\n \tsave_commit_buffer = 0;\n@@ -2996,6 +2997,9 @@ static void get_object_list(int ac, const char **av)\n \t/* make sure shallows are read */\n \tis_repository_shallow(the_repository);\n \n+\tsave_warning = warn_on_object_refname_ambiguity;\n+\twarn_on_object_refname_ambiguity = 0;\n+\n \twhile (fgets(line, sizeof(line), stdin) != NULL) {\n \t\tint len = strlen(line);\n \t\tif (len && line[len - 1] == '\\n')\n@@ -3022,6 +3026,8 @@ static void get_object_list(int ac, const char **av)\n \t\t\tdie(_(\"bad revision '%s'\"), line);\n \t}\n \n+\twarn_on_object_refname_ambiguity = save_warning;\n+\n \tif (use_bitmap_index && !get_object_list_from_bitmap(&revs))\n \t\treturn;\n \n-- \ngitgitgadget\n"},{"id":"362612","messageId":"20181106211234.GA8383@sigill.intra.peff.net","threadId":"49773","inReplyTo":"002868ee6bec3dac38749d0f05bf2db8da0969a5.1541536484.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/1] pack-objects: ignore ambiguous object warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-06T21:12:35Z","receivedAt":"2018-11-06T21:12:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 06, 2018 at 12:34:47PM -0800, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee <dstolee@microsoft.com>\n> \n> A git push process runs several processes during its run, but one\n> includes git send-pack which calls git pack-objects and passes\n> the known have/wants into stdin using object ids. However, the\n> default setting for core.warnAmbiguousRefs requires git pack-objects\n> to check for ref names matching the ref_rev_parse_rules array in\n> refs.c. This means that every object is triggering at least six\n> \"file exists?\" queries.  When there are a lot of refs, this can\n> add up significantly! I observed a simple push spending three\n> seconds checking these paths.\n\nThanks, this fills out the details from the cover letter a bit better.\n\n> The fix here is similar to 4c30d50 \"rev-list: disable object/refname\n> ambiguity check with --stdin\". Save the value of the global\n> warn_on_object_refname_ambiguity variable (which is usually set to\n> the boolean config variable core.warnAmbiguousRefs) and change the\n> state to false. Do this only during the get_object_list() method\n> which reads the objects from stdin.\n\nI think this parenthetical isn't quite right. We never change the value\nof warn_on_object_refname_ambiguity based on user config. It's just that\nif warn_ambiguous_refs is off, we do not even bother looking at the\nobject_refname variant.\n\nSo we'd never expect to see anything except \"1\" in our save_warning\nvariable. Doing a save/restore is just about code hygiene and\nmaintainability.\n\nOther than that minor nit, this whole thing looks good to me.\n\n-Peff\n"},{"id":"362628","messageId":"xmqqftwdd54p.fsf@gitster-ct.c.googlers.com","threadId":"49773","inReplyTo":"20181106211234.GA8383@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/1] pack-objects: ignore ambiguous object warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-07T01:54:14Z","receivedAt":"2018-11-07T01:54:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So we'd never expect to see anything except \"1\" in our save_warning\n> variable. Doing a save/restore is just about code hygiene and\n> maintainability.\n\nHere is what I plan to queue.  Thanks, both.\n\n-- >8 --\nFrom: Derrick Stolee <dstolee@microsoft.com>\nDate: Tue, 6 Nov 2018 12:34:47 -0800\nSubject: [PATCH] pack-objects: ignore ambiguous object warnings\n\nA git push process runs several processes during its run, but one\nincludes git send-pack which calls git pack-objects and passes\nthe known have/wants into stdin using object ids. However, the\ndefault setting for core.warnAmbiguousRefs requires git pack-objects\nto check for ref names matching the ref_rev_parse_rules array in\nrefs.c. This means that every object is triggering at least six\n\"file exists?\" queries.  When there are a lot of refs, this can\nadd up significantly! I observed a simple push spending three\nseconds checking these paths.\n\nThe fix here is similar to 4c30d50 \"rev-list: disable object/refname\nambiguity check with --stdin\".  While the get_object_list() method\nreads the objects from stdin, turn warn_on_object_refname_ambiguity\nflag (which is usually true) to false.  Just for code hygiene, save\naway the original at the beginning and restore it once we are done.\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/pack-objects.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex d1144a8f7e..f703e6df9b 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2988,6 +2988,7 @@ static void get_object_list(int ac, const char **av)\n \tstruct rev_info revs;\n \tchar line[1000];\n \tint flags = 0;\n+\tint save_warning;\n \n \tinit_revisions(&revs, NULL);\n \tsave_commit_buffer = 0;\n@@ -2996,6 +2997,9 @@ static void get_object_list(int ac, const char **av)\n \t/* make sure shallows are read */\n \tis_repository_shallow(the_repository);\n \n+\tsave_warning = warn_on_object_refname_ambiguity;\n+\twarn_on_object_refname_ambiguity = 0;\n+\n \twhile (fgets(line, sizeof(line), stdin) != NULL) {\n \t\tint len = strlen(line);\n \t\tif (len && line[len - 1] == '\\n')\n@@ -3022,6 +3026,8 @@ static void get_object_list(int ac, const char **av)\n \t\t\tdie(_(\"bad revision '%s'\"), line);\n \t}\n \n+\twarn_on_object_refname_ambiguity = save_warning;\n+\n \tif (use_bitmap_index && !get_object_list_from_bitmap(&revs))\n \t\treturn;\n \n-- \n2.19.1-856-g8858448bb4\n\n"},{"id":"362633","messageId":"xmqq7ehpclsa.fsf@gitster-ct.c.googlers.com","threadId":"49773","inReplyTo":"xmqqftwdd54p.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/1] pack-objects: ignore ambiguous object warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-07T08:52:05Z","receivedAt":"2018-11-07T08:52:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> The fix here is similar to 4c30d50 \"rev-list: disable object/refname\n> ambiguity check with --stdin\".  While the get_object_list() method\n> reads the objects from stdin, turn warn_on_object_refname_ambiguity\n> flag (which is usually true) to false.  Just for code hygiene, save\n> away the original at the beginning and restore it once we are done.\n\nI actually think this is a bit too broad.  The calling process of\nthis program does know that it is feeding list of raw object names\n(prefixed with ^ for negative ones), but the codepath this patch\ntouches does not know who is calling it with what.  It would be\nsafer to introduce a mechanism for the caller to tell this codepath\nnot to bother checking refnames, as it knows it is feeding the raw\nobject names and not refnames.\n\nAfter all, you can feed things like \"refs/heads/master\" from the\nstandard input of \"git pack-objects --revs\", no?\n\n> Helped-by: Jeff King <peff@peff.net>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  builtin/pack-objects.c | 6 ++++++\n>  1 file changed, 6 insertions(+)\n>\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index d1144a8f7e..f703e6df9b 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -2988,6 +2988,7 @@ static void get_object_list(int ac, const char **av)\n>  \tstruct rev_info revs;\n>  \tchar line[1000];\n>  \tint flags = 0;\n> +\tint save_warning;\n>  \n>  \tinit_revisions(&revs, NULL);\n>  \tsave_commit_buffer = 0;\n> @@ -2996,6 +2997,9 @@ static void get_object_list(int ac, const char **av)\n>  \t/* make sure shallows are read */\n>  \tis_repository_shallow(the_repository);\n>  \n> +\tsave_warning = warn_on_object_refname_ambiguity;\n> +\twarn_on_object_refname_ambiguity = 0;\n> +\n>  \twhile (fgets(line, sizeof(line), stdin) != NULL) {\n>  \t\tint len = strlen(line);\n>  \t\tif (len && line[len - 1] == '\\n')\n> @@ -3022,6 +3026,8 @@ static void get_object_list(int ac, const char **av)\n>  \t\t\tdie(_(\"bad revision '%s'\"), line);\n>  \t}\n>  \n> +\twarn_on_object_refname_ambiguity = save_warning;\n> +\n>  \tif (use_bitmap_index && !get_object_list_from_bitmap(&revs))\n>  \t\treturn;\n"},{"id":"362635","messageId":"20181107092129.GA8671@sigill.intra.peff.net","threadId":"49773","inReplyTo":"xmqq7ehpclsa.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/1] pack-objects: ignore ambiguous object warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-07T09:21:29Z","receivedAt":"2018-11-07T09:21:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 07, 2018 at 05:52:05PM +0900, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > The fix here is similar to 4c30d50 \"rev-list: disable object/refname\n> > ambiguity check with --stdin\".  While the get_object_list() method\n> > reads the objects from stdin, turn warn_on_object_refname_ambiguity\n> > flag (which is usually true) to false.  Just for code hygiene, save\n> > away the original at the beginning and restore it once we are done.\n> \n> I actually think this is a bit too broad.  The calling process of\n> this program does know that it is feeding list of raw object names\n> (prefixed with ^ for negative ones), but the codepath this patch\n> touches does not know who is calling it with what.  It would be\n> safer to introduce a mechanism for the caller to tell this codepath\n> not to bother checking refnames, as it knows it is feeding the raw\n> object names and not refnames.\n> \n> After all, you can feed things like \"refs/heads/master\" from the\n> standard input of \"git pack-objects --revs\", no?\n\nKeep in mind that this is not disallowing \"refs/heads/master\", nor even\ndisabling the ambiguity checking if we feed a \"foo\" that exists as both\na tag and a branch.\n\nIt is only disabling the check that when the caller feeds a 40-hex sha1,\nwe do not double-check to make sure that there is not a ref of the same\nname (which is not even really an ambiguity, but just an informational\nmessage for the user; the object id has always and must always take\nprecedence).\n\nSo yes, the caller does know better \"I am only going to feed object\nids\". But you can likewise look from the callee's perspective: \"I am\ngoing to take a lot of inputs, and spending time for an informational\nmessage for each one is not worth doing\".\n\nSo I think it's justifiable from that point of view. And from a\npractical point of view, it's much simpler: we do not have teach every\ncaller to specify its input, or risk being slowed down by a low-value\ninformational check.\n\n-Peff\n"}]}