{"thread":{"id":"58288","subject":"Partial-clone cause big performance impact on server","startedAt":"2022-08-11T08:10:17Z","lastAt":"2022-09-08T18:42:26Z","messageCount":40,"participants":["程洋","Jonathan Tan","Derrick Stolee","Jeff King","ZheNing Hu","Junio C Hamano","rsbecker@nexbridge.com"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"461059","messageId":"bfa3de4485614badb4a27d8cfba99968@xiaomi.com","threadId":"58288","inReplyTo":null,"subject":"Partial-clone cause big performance impact on server","fromName":"程洋","fromEmail":"chengyang@xiaomi.com","sentAt":"2022-08-11T08:09:56Z","receivedAt":"2022-08-11T08:10:17Z","isPatch":false,"sender":{"key":"chengyang@xiaomi.com","avatar":null},"body":"Hi.\n We observed big disk space save by partial-clone and require all of our users (2000+) to clone repository with partial-clone (filter=blob:none)\n However at busy time, we found it's extremely slow for user to fetch. Here is what we did.\n\n 1. ask all users to fetch with filter=blob:none. And it's remarkable. Now our download size per user decrease from 460G to 180G.\n 2. But at busy time, everyone's fetch become slow. (at idle hours, it takes us 5 minutes to clone a big repositories, but it takes more than 1 hour to clone the same repositories at busy hours)\n 3. with GIT_TRACE_PACKET=1. We found on big repositories (200K+refs, 6m+ objects). Git will sends 40k want.\n 4. And we then track our server(which is gerrit with jgit). We found the server is couting objects. Then we check those 40k objects, most of them are blobs rather than commit. (which means they're not in bitmap)\n 5. We believe that's the root cause of our problem. Git sends too many \"want SHA1\" which are not in bitmap, cause the server to count objects frequently, which then slow down the server.\n\nWhat we want is, download the things we need to checkout to specific commit. But if one commit contain so many objects (like us , 40k+). It takes more time to counting than downloading.\nIs it possible to let git only send \"commit want\" rather than all the objects SHA1 one by one?\n#/******本邮件及其附件含有小米公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！ This e-mail and its attachments contain confidential information from XIAOMI, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by phone or email immediately and delete it!******/#\n"},{"id":"461087","messageId":"20220811172219.2308120-1-jonathantanmy@google.com","threadId":"58288","inReplyTo":"bfa3de4485614badb4a27d8cfba99968@xiaomi.com","subject":"Re: Partial-clone cause big performance impact on server","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-08-11T17:22:19Z","receivedAt":"2022-08-11T17:23:24Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"程洋 writes:\n> 3. with GIT_TRACE_PACKET=1. We found on big repositories (200K+refs, 6m+ objects). Git will sends 40k want.\n> 4. And we then track our server(which is gerrit with jgit). We found the server is couting objects. Then we check those 40k objects, most of them are blobs rather than commit. (which means they're not in bitmap)\n> 5. We believe that's the root cause of our problem. Git sends too many \"want SHA1\" which are not in bitmap, cause the server to count objects frequently, which then slow down the server.\n> \n> What we want is, download the things we need to checkout to specific commit. But if one commit contain so many objects (like us , 40k+). It takes more time to counting than downloading.\n> Is it possible to let git only send \"commit want\" rather than all the objects SHA1 one by one?\n\nOn a technical level, it may be possible - at the point in the Git code\nwhere the batch prefetch occurs, I'm not sure if we have the commit, but\nwe could plumb the commit information there. (We have the tree, but this\ndoesn't help us here because as far as I know, the tree won't be in the\nbitmap so the server would need to count objects anyway, resulting in\nthe same problem.)\n\nHowever, sending only commits as wants would mean that we would be\nfetching more blobs than needed. For example, if we were to clone (with\ncheckout) and then checkout HEAD^, sending a \"commit want\" for the\nlatter checkout would result in all blobs referenced by the commit's\ntree being fetched and not only the blobs that are different.\n\nOne idea that we (at $DAYJOB) had is to supply a commit hint so that the\nserver can first use bitmaps to narrow down the objects that need to be\nchecked. I had a preliminary patch for that [1] but as of now, no one\nhas continued pursuing that idea.\n\n[1] https://lore.kernel.org/git/20201215200207.1083655-1-jonathantanmy@google.com/\n"},{"id":"461114","messageId":"16633d89-6ccd-859d-8533-9861ad831c45@github.com","threadId":"58288","inReplyTo":"bfa3de4485614badb4a27d8cfba99968@xiaomi.com","subject":"Re: Partial-clone cause big performance impact on server","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-12T12:21:54Z","receivedAt":"2022-08-12T12:21:58Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/11/22 4:09 AM, 程洋 wrote:> Hi.\n> We observed big disk space save by partial-clone and require all of our users (2000+) to clone repository with partial-clone (filter=blob:none)\n> However at busy time, we found it's extremely slow for user to fetch. Here is what we did.\n> \n> 1. ask all users to fetch with filter=blob:none. And it's remarkable. Now our download size per user decrease from 460G to 180G.\n\nI hope this includes the blob download during the initial checkout,\nbecause otherwise you have a very strange shape to make your commits and\ntrees take up 180 GB.\n\n> 2. But at busy time, everyone's fetch become slow. (at idle hours, it takes us 5 minutes to clone a big repositories, but it takes more than 1 hour to clone the same repositories at busy hours)\n> 3. with GIT_TRACE_PACKET=1. We found on big repositories (200K+refs, 6m+ objects). Git will sends 40k want.\n\nYou only have six million objects in the repo and yet have that size? It\nmust be some very large blobs.\n\n> 4. And we then track our server(which is gerrit with jgit). We found the server is couting objects. Then we check those 40k objects, most of them are blobs rather than commit. (which means they're not in bitmap)\n\nAre you seeing any commits in these requests? If the Git client is asking\nfor blobs, then they should not be mixed with commit wants. What kind of\noperation are you doing to see these mixed wants?\n\nIf the request was only blobs, then the server should not need a \"Counting\nobjects\" phase. It should jump immediately to preparing the objects (which\nwill likely require parsing deltas, and that can be expensive). I don't\nknow if JGit is doing something different, though.\n\n> 5. We believe that's the root cause of our problem. Git sends too many \"want SHA1\" which are not in bitmap, cause the server to count objects frequently, which then slow down the server.\n> \n> What we want is, download the things we need to checkout to specific commit. But if one commit contain so many objects (like us , 40k+). It takes more time to counting than downloading.\n\nOne thing that the microsoft/git fork uses in its \"git-gvfs-helper\" tool\n(which speaks the GVFS Protocol as a replacement for partial clone when\nusing Azure Repos as a server) is a batched download of missing objects [1].\nThe initial limit is 4000 objects at a time, but that helps keep each\nrequest small enough that it is less likely to fail for scale reasons alone.\n\n[1] https://github.com/microsoft/git/blob/vfs-2.37.1/gvfs-helper.c#L3510-L3520\n\nIt might be interesting to create such batch-downloads for these partial\nclone blob-fetches.\n\nThanks,\n-Stolee\n"},{"id":"461164","messageId":"08dae83ba1b541adac0fd96e2f99b194@xiaomi.com","threadId":"58288","inReplyTo":"20220811172219.2308120-1-jonathantanmy@google.com","subject":"回复: [External Mail]Re: Partial-clone cause big performance impact on server","fromName":"程洋","fromEmail":"chengyang@xiaomi.com","sentAt":"2022-08-13T07:55:28Z","receivedAt":"2022-08-13T07:56:22Z","isPatch":false,"sender":{"key":"chengyang@xiaomi.com","avatar":null},"body":"> > 3. with GIT_TRACE_PACKET=1. We found on big repositories (200K+refs, 6m+ objects). Git will sends 40k want.\n> > 4. And we then track our server(which is gerrit with jgit). We found the server is couting objects. Then we check those 40k objects, most of them are blobs rather than commit. (which means they're not in bitmap)\n> > 5. We believe that's the root cause of our problem. Git sends too many \"want SHA1\" which are not in bitmap, cause the server to count objects frequently, which then slow down the server.\n> >\n> > What we want is, download the things we need to checkout to specific commit. But if one commit contain so many objects (like us , 40k+). It takes more time to counting than downloading.\n> > Is it possible to let git only send \"commit want\" rather than all the objects SHA1 one by one?\n>\n> On a technical level, it may be possible - at the point in the Git code where the\n> batch prefetch occurs, I'm not sure if we have the commit, but we could\n> plumb the commit information there. (We have the tree, but this doesn't help\n> us here because as far as I know, the tree won't be in the bitmap so the server\n> would need to count objects anyway, resulting in the same problem.)\n>\n> However, sending only commits as wants would mean that we would be\n> fetching more blobs than needed. For example, if we were to clone (with\n> checkout) and then checkout HEAD^, sending a \"commit want\" for the latter\n> checkout would result in all blobs referenced by the commit's tree being\n> fetched and not only the blobs that are different.\n\nIt seems your solution require changes from both server side and client side\nWhy not we just add another filter, allow partial-clone always sends commit level want?\nIf we checkout HEAD~1, then client can send \"want HEAD~1 HEAD~2\".\n\n> One idea that we (at $DAYJOB) had is to supply a commit hint so that the\n> server can first use bitmaps to narrow down the objects that need to be\n> checked. I had a preliminary patch for that [1] but as of now, no one has\n> continued pursuing that idea.\n>\n> [1] https://lore.kernel.org/git/20201215200207.1083655-1-jonathantanmy@google.com/\n#/******本邮件及其附件含有小米公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！ This e-mail and its attachments contain confidential information from XIAOMI, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by phone or email immediately and delete it!******/#\n"},{"id":"461172","messageId":"5ad4e3134df444908a548a52f43b90e1@xiaomi.com","threadId":"58288","inReplyTo":"08dae83ba1b541adac0fd96e2f99b194@xiaomi.com","subject":"回复: [External Mail]Re: Partial-clone cause big performance impact on server","fromName":"程洋","fromEmail":"chengyang@xiaomi.com","sentAt":"2022-08-13T11:41:46Z","receivedAt":"2022-08-13T11:41:54Z","isPatch":false,"sender":{"key":"chengyang@xiaomi.com","avatar":null},"body":"Another thing I need to point out is, current partial-clone also have big performance impact on disks. Especially on SMR disks.\nSome of SMR disk has really bad random write performance (like 100kb/s).\nWe found typically it takes us 50 minutes to download whole Android project by partial-clone (2 hours and a half without partial-clone)\nHowever it takes us 5 hours to download with partial-clone on those SMR disks.\n\nThat's also because the big number wants make the download process to be made of random writes rather than sequence writes.\n\n> > > 3. with GIT_TRACE_PACKET=1. We found on big repositories (200K+refs, 6m+ objects). Git will sends 40k want.\n> > > 4. And we then track our server(which is gerrit with jgit). We found the\n> server is couting objects. Then we check those 40k objects, most of them are\n> blobs rather than commit. (which means they're not in bitmap)\n> > > 5. We believe that's the root cause of our problem. Git sends too many \"want SHA1\" which are not in bitmap, cause the server to count objects frequently, which then slow down the server.\n> > >\n> > > What we want is, download the things we need to checkout to specific commit. But if one commit contain so many objects (like us , 40k+). It takes more time to counting than downloading.\n> > > Is it possible to let git only send \"commit want\" rather than all the objects SHA1 one by one?\n> >\n> > On a technical level, it may be possible - at the point in the Git\n> > code where the batch prefetch occurs, I'm not sure if we have the\n> > commit, but we could plumb the commit information there. (We have the\n> > tree, but this doesn't help us here because as far as I know, the tree\n> > won't be in the bitmap so the server would need to count objects\n> > anyway, resulting in the same problem.)\n> >\n> > However, sending only commits as wants would mean that we would be\n> > fetching more blobs than needed. For example, if we were to clone\n> > (with\n> > checkout) and then checkout HEAD^, sending a \"commit want\" for the\n> > latter checkout would result in all blobs referenced by the commit's\n> > tree being fetched and not only the blobs that are different.\n>\n> It seems your solution require changes from both server side and client side\n> Why not we just add another filter, allow partial-clone always sends commit\n> level want?\n> If we checkout HEAD~1, then client can send \"want HEAD~1 HEAD~2\".\n>\n> > One idea that we (at $DAYJOB) had is to supply a commit hint so that\n> > the server can first use bitmaps to narrow down the objects that need\n> > to be checked. I had a preliminary patch for that [1] but as of now,\n> > no one has continued pursuing that idea.\n> >\n> > [1]\n> > https://lore.kernel.org/git/20201215200207.1083655-1-\n> jonathantanmy@goo\n> > gle.com/\n#/******本邮件及其附件含有小米公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！ This e-mail and its attachments contain confidential information from XIAOMI, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by phone or email immediately and delete it!******/#\n"},{"id":"461184","messageId":"YviaoXRctE9fg/mB@coredump.intra.peff.net","threadId":"58288","inReplyTo":"bfa3de4485614badb4a27d8cfba99968@xiaomi.com","subject":"Re: Partial-clone cause big performance impact on server","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-14T06:48:01Z","receivedAt":"2022-08-14T06:48:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 11, 2022 at 08:09:56AM +0000, 程洋 wrote:\n\n> 4. And we then track our server(which is gerrit with jgit). We\n> found the server is couting objects. Then we check those 40k\n> objects, most of them are blobs rather than commit. (which\n> means they're not in bitmap)\n> 5. We believe that's the root cause of our problem. Git sends too\n> many \"want SHA1\" which are not in bitmap, cause the server to\n> count objects frequently, which then slow down the server.\n\nI'd be surprised if bitmaps make a big difference either way here, since\nblobs are very quick in the \"counting\" phase of pack-objects. They can't\nlink to anything else, so we should not be opening the object contents\nat all! We just need to find them on disk, and then in many cases we can\nsend them over the wire without even decompressing (the exception is if\nthey are stored as deltas against an object the client doesn't have).\n\nI didn't generate a test case, but I'm pretty sure that is how git.git's\npack-objects should behave. But you mentioned that the server is jgit;\nit's possible that it isn't as optimized in that area.\n\n-Peff\n"},{"id":"461215","messageId":"CAOLTT8R6hNKWGen4RD2sSU-asjjS6HXnxY2JC4k9SeL4YDzB-g@mail.gmail.com","threadId":"58288","inReplyTo":"08dae83ba1b541adac0fd96e2f99b194@xiaomi.com","subject":"Re: [External Mail]Re: Partial-clone cause big performance impact on server","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2022-08-15T05:16:27Z","receivedAt":"2022-08-15T05:16:42Z","isPatch":false,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"程洋 于2022年8月13日周六 16:00写道：\n>\n> > > 3. with GIT_TRACE_PACKET=1. We found on big repositories (200K+refs, 6m+ objects). Git will sends 40k want.\n> > > 4. And we then track our server(which is gerrit with jgit). We found the server is couting objects. Then we check those 40k objects, most of them are blobs rather than commit. (which means they're not in bitmap)\n> > > 5. We believe that's the root cause of our problem. Git sends too many \"want SHA1\" which are not in bitmap, cause the server to count objects frequently, which then slow down the server.\n> > >\n> > > What we want is, download the things we need to checkout to specific commit. But if one commit contain so many objects (like us , 40k+). It takes more time to counting than downloading.\n> > > Is it possible to let git only send \"commit want\" rather than all the objects SHA1 one by one?\n> >\n> > On a technical level, it may be possible - at the point in the Git code where the\n> > batch prefetch occurs, I'm not sure if we have the commit, but we could\n> > plumb the commit information there. (We have the tree, but this doesn't help\n> > us here because as far as I know, the tree won't be in the bitmap so the server\n> > would need to count objects anyway, resulting in the same problem.)\n> >\n> > However, sending only commits as wants would mean that we would be\n> > fetching more blobs than needed. For example, if we were to clone (with\n> > checkout) and then checkout HEAD^, sending a \"commit want\" for the latter\n> > checkout would result in all blobs referenced by the commit's tree being\n> > fetched and not only the blobs that are different.\n>\n> It seems your solution require changes from both server side and client side\n> Why not we just add another filter, allow partial-clone always sends commit level want?\n> If we checkout HEAD~1, then client can send \"want HEAD~1 HEAD~2\".\n>\n\nI am interesting about this question too, maybe I can try if we can do\nthis.. ;-)\n\nZheNing Hu\n"},{"id":"461224","messageId":"44c62b62ce8f418d8929bdffc894d329@xiaomi.com","threadId":"58288","inReplyTo":"CAOLTT8R6hNKWGen4RD2sSU-asjjS6HXnxY2JC4k9SeL4YDzB-g@mail.gmail.com","subject":"RE: [External Mail]Re: Partial-clone cause big performance impact on server","fromName":"程洋","fromEmail":"chengyang@xiaomi.com","sentAt":"2022-08-15T13:15:07Z","receivedAt":"2022-08-15T13:15:26Z","isPatch":false,"sender":{"key":"chengyang@xiaomi.com","avatar":null},"body":"There is a really easy way to reproduce it\n\ngit clone --filter=blob:none -b master \"https://android.googlesource.com/platform/prebuilts/gradle-plugin\"\n\nEven Google AOSP Gerrit will have this problem. You will find it hang for minutes on checkout\n\n\n> -----Original Message-----\n> From: ZheNing Hu \n> Sent: Monday, August 15, 2022 1:16 PM\n> To: 程洋 \n> Cc: Jonathan Tan ; git@vger.kernel.org; 何浩\n> ; Xin7 Ma 马鑫 ; 石奉兵\n> ; 凡军辉 ; 王汉基\n> \n> Subject: Re: [External Mail]Re: Partial-clone cause big performance impact on\n> server\n>\n> [外部邮件] 此邮件来源于小米公司外部，请谨慎处理。\n>\n> 程洋 于2022年8月13日周六 16:00写道：\n> >\n> > > > 3. with GIT_TRACE_PACKET=1. We found on big repositories\n> (200K+refs, 6m+ objects). Git will sends 40k want.\n> > > > 4. And we then track our server(which is gerrit with jgit). We found\n> the server is couting objects. Then we check those 40k objects, most of them\n> are blobs rather than commit. (which means they're not in bitmap)\n> > > > 5. We believe that's the root cause of our problem. Git sends too\n> many \"want SHA1\" which are not in bitmap, cause the server to count\n> objects frequently, which then slow down the server.\n> > > >\n> > > > What we want is, download the things we need to checkout to specific\n> commit. But if one commit contain so many objects (like us , 40k+). It takes\n> more time to counting than downloading.\n> > > > Is it possible to let git only send \"commit want\" rather than all the\n> objects SHA1 one by one?\n> > >\n> > > On a technical level, it may be possible - at the point in the Git\n> > > code where the batch prefetch occurs, I'm not sure if we have the\n> > > commit, but we could plumb the commit information there. (We have\n> > > the tree, but this doesn't help us here because as far as I know,\n> > > the tree won't be in the bitmap so the server would need to count\n> > > objects anyway, resulting in the same problem.)\n> > >\n> > > However, sending only commits as wants would mean that we would be\n> > > fetching more blobs than needed. For example, if we were to clone\n> > > (with\n> > > checkout) and then checkout HEAD^, sending a \"commit want\" for the\n> > > latter checkout would result in all blobs referenced by the commit's\n> > > tree being fetched and not only the blobs that are different.\n> >\n> > It seems your solution require changes from both server side and\n> > client side Why not we just add another filter, allow partial-clone always\n> sends commit level want?\n> > If we checkout HEAD~1, then client can send \"want HEAD~1 HEAD~2\".\n> >\n>\n> I am interesting about this question too, maybe I can try if we can do this.. ;-)\n>\n> ZheNing Hu\n#/******本邮件及其附件含有小米公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！ This e-mail and its attachments contain confidential information from XIAOMI, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by phone or email immediately and delete it!******/#\n"},{"id":"461225","messageId":"85e6fd08-c741-26d4-1393-4b115133e687@github.com","threadId":"58288","inReplyTo":"YviaoXRctE9fg/mB@coredump.intra.peff.net","subject":"Re: Partial-clone cause big performance impact on server","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-15T13:18:38Z","receivedAt":"2022-08-15T13:18:45Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/14/2022 2:48 AM, Jeff King wrote:\n> On Thu, Aug 11, 2022 at 08:09:56AM +0000, 程洋 wrote:\n> \n>> 4. And we then track our server(which is gerrit with jgit). We\n>> found the server is couting objects. Then we check those 40k\n>> objects, most of them are blobs rather than commit. (which\n>> means they're not in bitmap)\n>> 5. We believe that's the root cause of our problem. Git sends too\n>> many \"want SHA1\" which are not in bitmap, cause the server to\n>> count objects frequently, which then slow down the server.\n> \n> I'd be surprised if bitmaps make a big difference either way here, since\n> blobs are very quick in the \"counting\" phase of pack-objects. They can't\n> link to anything else, so we should not be opening the object contents\n> at all! We just need to find them on disk, and then in many cases we can\n> send them over the wire without even decompressing (the exception is if\n> they are stored as deltas against an object the client doesn't have).\n> \n> I didn't generate a test case, but I'm pretty sure that is how git.git's\n> pack-objects should behave. But you mentioned that the server is jgit;\n> it's possible that it isn't as optimized in that area.\n\nI just remembered that Gerrit specifically has branch-level security,\nwhere some branches are not visible to all users. For that reason, blobs\ncannot be served without first determining if they are reachable from a\nbranch visible to the current user.\n\nI'm not sure if that's the problem in this particular case, but it could\nbe.\n\nThanks,\n-Stolee\n\n"},{"id":"461228","messageId":"68d8bfb7d94b4cd9a1312eddd6740f30@xiaomi.com","threadId":"58288","inReplyTo":"85e6fd08-c741-26d4-1393-4b115133e687@github.com","subject":"RE: [External Mail]Re: Partial-clone cause big performance impact on server","fromName":"程洋","fromEmail":"chengyang@xiaomi.com","sentAt":"2022-08-15T14:50:36Z","receivedAt":"2022-08-15T14:50:46Z","isPatch":false,"sender":{"key":"chengyang@xiaomi.com","avatar":null},"body":"https://gerrit.googlesource.com/jgit/+/refs/heads/master/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java#2037\nIt seems you're right.\nAccording to commnet I found in jgit source code. It seems it is indeed doing what you said!\n\n> On 8/14/2022 2:48 AM, Jeff King wrote:\n> > On Thu, Aug 11, 2022 at 08:09:56AM +0000, 程洋 wrote:\n> >\n> >> 4. And we then track our server(which is gerrit with jgit). We\n> >> found the server is couting objects. Then we check those 40k\n> >> objects, most of them are blobs rather than commit. (which\n> >> means they're not in bitmap)\n> >> 5. We believe that's the root cause of our problem. Git sends too\n> >> many \"want SHA1\" which are not in bitmap, cause the server to\n> >> count objects frequently, which then slow down the server.\n> >\n> > I'd be surprised if bitmaps make a big difference either way here,\n> > since blobs are very quick in the \"counting\" phase of pack-objects.\n> > They can't link to anything else, so we should not be opening the\n> > object contents at all! We just need to find them on disk, and then in\n> > many cases we can send them over the wire without even decompressing\n> > (the exception is if they are stored as deltas against an object the client\n> doesn't have).\n> >\n> > I didn't generate a test case, but I'm pretty sure that is how\n> > git.git's pack-objects should behave. But you mentioned that the\n> > server is jgit; it's possible that it isn't as optimized in that area.\n>\n> I just remembered that Gerrit specifically has branch-level security, where\n> some branches are not visible to all users. For that reason, blobs cannot be\n> served without first determining if they are reachable from a branch visible\n> to the current user.\n>\n> I'm not sure if that's the problem in this particular case, but it could be.\n>\n> Thanks,\n> -Stolee\n\n#/******本邮件及其附件含有小米公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！ This e-mail and its attachments contain confidential information from XIAOMI, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by phone or email immediately and delete it!******/#\n"},{"id":"461360","messageId":"11d7fc5721c541d6bc44bf635517497b@xiaomi.com","threadId":"58288","inReplyTo":"85e6fd08-c741-26d4-1393-4b115133e687@github.com","subject":"RE: [External Mail]Re: Partial-clone cause big performance impact on server","fromName":"程洋","fromEmail":"chengyang@xiaomi.com","sentAt":"2022-08-17T10:22:25Z","receivedAt":"2022-08-17T10:22:34Z","isPatch":false,"sender":{"key":"chengyang@xiaomi.com","avatar":null},"body":"But I still think the protocol still should tell the server which ref the blob is reachable.\nBecause it would be really hard to implement any kind of ACL\nBut git is surely designed for open sources community. It makes senses this request will be rejected.\n\n> -----Original Message-----\n> From: Derrick Stolee \n> Sent: Monday, August 15, 2022 9:19 PM\n> To: Jeff King ; 程洋 \n> Cc: git@vger.kernel.org; 何浩 ; Xin7 Ma 马鑫\n> ; 石奉兵 ; 凡军辉\n> ; 王汉基 \n> Subject: [External Mail]Re: Partial-clone cause big performance impact on\n> server\n>\n> [外部邮件] 此邮件来源于小米公司外部，请谨慎处理。\n>\n> On 8/14/2022 2:48 AM, Jeff King wrote:\n> > On Thu, Aug 11, 2022 at 08:09:56AM +0000, 程洋 wrote:\n> >\n> >> 4. And we then track our server(which is gerrit with jgit). We\n> >> found the server is couting objects. Then we check those 40k\n> >> objects, most of them are blobs rather than commit. (which\n> >> means they're not in bitmap)\n> >> 5. We believe that's the root cause of our problem. Git sends too\n> >> many \"want SHA1\" which are not in bitmap, cause the server to\n> >> count objects frequently, which then slow down the server.\n> >\n> > I'd be surprised if bitmaps make a big difference either way here,\n> > since blobs are very quick in the \"counting\" phase of pack-objects.\n> > They can't link to anything else, so we should not be opening the\n> > object contents at all! We just need to find them on disk, and then in\n> > many cases we can send them over the wire without even decompressing\n> > (the exception is if they are stored as deltas against an object the client\n> doesn't have).\n> >\n> > I didn't generate a test case, but I'm pretty sure that is how\n> > git.git's pack-objects should behave. But you mentioned that the\n> > server is jgit; it's possible that it isn't as optimized in that area.\n>\n> I just remembered that Gerrit specifically has branch-level security, where\n> some branches are not visible to all users. For that reason, blobs cannot be\n> served without first determining if they are reachable from a branch visible\n> to the current user.\n>\n> I'm not sure if that's the problem in this particular case, but it could be.\n>\n> Thanks,\n> -Stolee\n\n#/******本邮件及其附件含有小米公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！ This e-mail and its attachments contain confidential information from XIAOMI, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by phone or email immediately and delete it!******/#\n"},{"id":"461361","messageId":"fc71e91b-7a66-5653-d723-e4df17bf2a9c@github.com","threadId":"58288","inReplyTo":"11d7fc5721c541d6bc44bf635517497b@xiaomi.com","subject":"Re: [External Mail]Re: Partial-clone cause big performance impact on server","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-17T13:41:10Z","receivedAt":"2022-08-17T13:41:20Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/17/2022 6:22 AM, 程洋 wrote:\n> But I still think the protocol still should tell the server which ref\n> the blob is reachable.\n> Because it would be really hard to implement any kind of ACL\n\nI think this idea has merit on its face, but it wouldn't really solve the\nproblem since the reachability query would still need to be done, just\nfrom a smaller set of references at first. If we were able to say \"this\nblob can be found at path X at commit Y\" then the server could do a\ncommit-reachability query and a path traversal, which should be a lot\nfaster.\n\nHowever, it would be extremely difficult to plumb into the partial clone\nmachinery. At the point where Git realizes it is missing a promisor\nobject, that code is very generic and removed from any kind of walk from a\nreference. That is further complicated by the fact that the walk is\nprobably from a local reference, which can be entirely different from the\nremote reference.\n\n> But git is surely designed for open sources community. It makes senses\n> this request will be rejected.\n\nWe try to keep all kinds of users in mind, so the fact that this applies\nto not-completely-open repositories is not a blocker.\n\nOne possible hurdle is the fact that this branch-level security is a\nfeature of Gerrit, not a feature of Git itself. Optimizing Git to that\nspecial case that Git does not itself support is less valuable to the Git\nproject itself.\n\nMy personal take is that the technical complexity required to make this\nfaster paired with the limited scope means that this feature would have a\ndifficult time getting accepted into the Git project. Perhaps a motivated\ncontributor will find ways to overcome these obstacles and find other\ninteresting applications that benefit a larger portion of Git users.\n\nThat's just my expectation. I'd be happy to read any patches that try to\nsolve this problem.\n\nThanks,\n-Stolee\n"},{"id":"461397","messageId":"Yv3S/J/ecYi0slQA@coredump.intra.peff.net","threadId":"58288","inReplyTo":"fc71e91b-7a66-5653-d723-e4df17bf2a9c@github.com","subject":"Re: [External Mail]Re: Partial-clone cause big performance impact on server","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-18T05:49:48Z","receivedAt":"2022-08-18T05:49:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 17, 2022 at 09:41:10AM -0400, Derrick Stolee wrote:\n\n> On 8/17/2022 6:22 AM, 程洋 wrote:\n> > But I still think the protocol still should tell the server which ref\n> > the blob is reachable.\n> > Because it would be really hard to implement any kind of ACL\n> \n> I think this idea has merit on its face, but it wouldn't really solve the\n> problem since the reachability query would still need to be done, just\n> from a smaller set of references at first. If we were able to say \"this\n> blob can be found at path X at commit Y\" then the server could do a\n> commit-reachability query and a path traversal, which should be a lot\n> faster.\n> \n> However, it would be extremely difficult to plumb into the partial clone\n> machinery. At the point where Git realizes it is missing a promisor\n> object, that code is very generic and removed from any kind of walk from a\n> reference. That is further complicated by the fact that the walk is\n> probably from a local reference, which can be entirely different from the\n> remote reference.\n\nAgreed. The client often doesn't know the context of what it's asking\nfor in the first place. Sometimes it's not carried through the code, but\nwe also have commands that might not be invoked with a commit in the\nfirst place! It's valid to run \"git read-tree \", and we should be\nable to fault in blobs from that tree as needed.\n\nI also think that this kind of \"is the blob reachable\" query is\nmostly expensive if you don't have reachability bitmaps at all. If you\ndo, then the cost to ask \"is this object reachable\" is the same for a\ncommit or a blob. If the server has a bitmap of all objects reachable\nfor each branch ACL (even if it has to do some small bit of fill-in\nwalking to bring it up to date), then querying for any object type the\nclient asks for is still just a bit lookup.\n\nNot knowing a lot about gerrit or jgit, it's not clear to me if there\nare configuration knobs that could be tweaked on the server side to make\nthese requests more efficient.\n\n> One possible hurdle is the fact that this branch-level security is a\n> feature of Gerrit, not a feature of Git itself. Optimizing Git to that\n> special case that Git does not itself support is less valuable to the Git\n> project itself.\n\nWe don't have branch-level security per se, but I do think that\neverything is there in Git to do fast \"is this object reachable from\nthese branches\" queries. If you're making a lot of those queries it\nmight influence your decision of which bitmaps to generate, but the\nbitmap concept itself should be sufficient.\n\n-Peff\n"},{"id":"462419","messageId":"b0101e7e0e61496a92c2299454c2696a@xiaomi.com","threadId":"58288","inReplyTo":"YviaoXRctE9fg/mB@coredump.intra.peff.net","subject":"RE: [External Mail]Re: Partial-clone cause big performance impact on server","fromName":"程洋","fromEmail":"chengyang@xiaomi.com","sentAt":"2022-09-01T06:53:01Z","receivedAt":"2022-09-01T06:54:16Z","isPatch":false,"sender":{"key":"chengyang@xiaomi.com","avatar":null},"body":">\n> > 4. And we then track our server(which is gerrit with jgit). We\n> > found the server is couting objects. Then we check those 40k\n> > objects, most of them are blobs rather than commit. (which\n> > means they're not in bitmap)\n> > 5. We believe that's the root cause of our problem. Git sends too\n> > many \"want SHA1\" which are not in bitmap, cause the server to\n> > count objects frequently, which then slow down the server.\n>\n> I'd be surprised if bitmaps make a big difference either way here, since blobs\n> are very quick in the \"counting\" phase of pack-objects. They can't link to\n> anything else, so we should not be opening the object contents at all! We\n> just need to find them on disk, and then in many cases we can send them\n> over the wire without even decompressing (the exception is if they are\n> stored as deltas against an object the client doesn't have).\n>\n> I didn't generate a test case, but I'm pretty sure that is how git.git's pack-\n> objects should behave. But you mentioned that the server is jgit; it's possible\n> that it isn't as optimized in that area.\n\nAt first I also think it's some implementation bugs by jgit. However I can also reproduce it on cgit. Here is the steps, I'm not sure if you can reproduce too.\n\n1. Clone a repository from AOSP to local machine: `git clone \"https://android.googlesource.com/platform/prebuilts/gradle-plugin\"`\n2. try to clone from localhost using cgit server. `GIT_TRACE_PACKET=1 git clone --filter=blob:none -b master user@localhost:/home/user/repositories/gradle-plugin `\n3. During checkout phase, it also takes 15 seconds before actual downloading.\n\nIt's really easy to reproduce, as long as the repository has so many blobs to checkout\n\n>\n> -Peff\n#/******本邮件及其附件含有小米公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！ This e-mail and its attachments contain confidential information from XIAOMI, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by phone or email immediately and delete it!******/#\n"},{"id":"462465","messageId":"YxDbfXyWzgokb1Bq@coredump.intra.peff.net","threadId":"58288","inReplyTo":"b0101e7e0e61496a92c2299454c2696a@xiaomi.com","subject":"Re: [External Mail]Re: Partial-clone cause big performance impact on server","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-01T16:19:09Z","receivedAt":"2022-09-01T16:19:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 01, 2022 at 06:53:01AM +0000, 程洋 wrote:\n\n> At first I also think it's some implementation bugs by jgit. However I\n> can also reproduce it on cgit. Here is the steps, I'm not sure if you\n> can reproduce too.\n> \n> 1. Clone a repository from AOSP to local machine: `git clone\n> \"https://android.googlesource.com/platform/prebuilts/gradle-plugin\"`\n> 2. try to clone from localhost using cgit server.\n> `GIT_TRACE_PACKET=1 git clone --filter=blob:none -b master\n> user@localhost:/home/user/repositories/gradle-plugin `\n> 3. During checkout phase, it also takes 15 seconds before actual downloading.\n\nI don't see that at all. A few things on your reproduction:\n\n - you have to tell the local server repo that filters are OK:\n\n git -C gradle-plugin config uploadpack.allowfilter true\n\n - your example goes over localhost ssh. Is your server configured to\n allow passing the GIT_PROTOCOL environment variable? If not, you're\n using the v0 protocol. In which case you'll have to set a config\n option to allow clients to fetch objects that the server didn't\n advertise.\n\n If you do it with allowReachableSHA1InWant, like this:\n\n git -C gradle-plugin config uploadpack.allowReachableSHA1InWant true\n\n then there will be a short delay while it checks their\n reachability. That check happens via an external rev-list. I think\n it's not clever enough to use bitmaps, though it could. However, in\n this example, the delay only seems to be around 800ms for me (and of\n course we didn't generate bitmaps anyway, so that wouldn't matter).\n\n If you instead do:\n\n git -C gradle-plugin config uploadpack.allowAnySHA1InWant true\n\n then that reachability check goes away.\n\n But on modern servers, most of this should be moot anyway. A\n well-configured server should support protocol v2, which defaults to\n allowAnySHA1InWant.\n\n If you use --no-local to disable local-clone optimizations, then you\n can use --filter without having to go through ssh. That should use\n protocol version 2, as a \"real\" server would.\n\nSo all told, I think a more realistic reproduction is:\n\n $ git clone https://android.googlesource.com/platform/prebuilts/gradle-plugin\n $ git -C gradle-plugin config uploadpack.allowfilter true\n $ git clone --filter=blob:none --no-local -b master grade-plugin foo\n\nwhich takes ~3s for me.\n\nI do think upload-pack spends more time than it needs in this case, as\nit's keen to call parse_object() on oids that the client asks for. Which\nmeans we'll open up those blobs and check their sha1s before sending\nthem out, which isn't strictly necessary.\n\nAll of this seems orthogonal to the original claim that \"Counting\nobjects\" was taking a while, though. The delays here are all inside\nupload-pack, before it spawns pack-objects. And it's pack-objects that\nsays \"Counting objects\".\n\n-Peff\n"},{"id":"462628","messageId":"d5305274b7c24adbaf6ad9ab92ac3b6a@xiaomi.com","threadId":"58288","inReplyTo":"YxDbfXyWzgokb1Bq@coredump.intra.peff.net","subject":"RE: [External Mail]Re: Partial-clone cause big performance impact on server","fromName":"程洋","fromEmail":"chengyang@xiaomi.com","sentAt":"2022-09-05T11:17:21Z","receivedAt":"2022-09-05T11:17:30Z","isPatch":false,"sender":{"key":"chengyang@xiaomi.com","avatar":null},"body":"Sorry, I told you the wrong branch. It should be \"android-t-preview-1\"\ngit clone --filter=blob:none --no-local -b android-t-preview-1 grade-plugin\n\nCan you try this one?\n\n> > At first I also think it's some implementation bugs by jgit. However I\n> > can also reproduce it on cgit. Here is the steps, I'm not sure if you\n> > can reproduce too.\n> >\n> > 1. Clone a repository from AOSP to local machine: `git clone\n> >\n> > \"https://android.googlesource.com/platform/prebuilts/gradle-plugin\"`\n> > 2. try to clone from localhost using cgit server.\n> > `GIT_TRACE_PACKET=1 git clone --filter=blob:none -b master\n> > user@localhost:/home/user/repositories/gradle-plugin ` 3. During\n> > checkout phase, it also takes 15 seconds before actual downloading.\n>\n> I don't see that at all. A few things on your reproduction:\n>\n> - you have to tell the local server repo that filters are OK:\n>\n> git -C gradle-plugin config uploadpack.allowfilter true\n>\n> - your example goes over localhost ssh. Is your server configured to\n> allow passing the GIT_PROTOCOL environment variable? If not, you're\n> using the v0 protocol. In which case you'll have to set a config\n> option to allow clients to fetch objects that the server didn't\n> advertise.\n>\n> If you do it with allowReachableSHA1InWant, like this:\n>\n> git -C gradle-plugin config uploadpack.allowReachableSHA1InWant true\n>\n> then there will be a short delay while it checks their\n> reachability. That check happens via an external rev-list. I think\n> it's not clever enough to use bitmaps, though it could. However, in\n> this example, the delay only seems to be around 800ms for me (and of\n> course we didn't generate bitmaps anyway, so that wouldn't matter).\n>\n> If you instead do:\n>\n> git -C gradle-plugin config uploadpack.allowAnySHA1InWant true\n>\n> then that reachability check goes away.\n>\n> But on modern servers, most of this should be moot anyway. A\n> well-configured server should support protocol v2, which defaults to\n> allowAnySHA1InWant.\n>\n> If you use --no-local to disable local-clone optimizations, then you\n> can use --filter without having to go through ssh. That should use\n> protocol version 2, as a \"real\" server would.\n>\n> So all told, I think a more realistic reproduction is:\n>\n> $ git clone https://android.googlesource.com/platform/prebuilts/gradle-\n> plugin\n> $ git -C gradle-plugin config uploadpack.allowfilter true\n> $ git clone --filter=blob:none --no-local -b master grade-plugin foo\n>\n> which takes ~3s for me.\n>\n> I do think upload-pack spends more time than it needs in this case, as it's\n> keen to call parse_object() on oids that the client asks for. Which means we'll\n> open up those blobs and check their sha1s before sending them out, which\n> isn't strictly necessary.\n>\n> All of this seems orthogonal to the original claim that \"Counting objects\" was\n> taking a while, though. The delays here are all inside upload-pack, before it\n> spawns pack-objects. And it's pack-objects that says \"Counting objects\".\n>\n> -Peff\n#/******本邮件及其附件含有小米公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！ This e-mail and its attachments contain confidential information from XIAOMI, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by phone or email immediately and delete it!******/#\n"},{"id":"462660","messageId":"YxeTsWrjpKo+JGfq@coredump.intra.peff.net","threadId":"58288","inReplyTo":"d5305274b7c24adbaf6ad9ab92ac3b6a@xiaomi.com","subject":"Re: [External Mail]Re: Partial-clone cause big performance impact on server","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-06T18:38:41Z","receivedAt":"2022-09-06T18:38:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 05, 2022 at 11:17:21AM +0000, 程洋 wrote:\n\n> Sorry, I told you the wrong branch. It should be \"android-t-preview-1\"\n> git clone --filter=blob:none --no-local -b android-t-preview-1 grade-plugin\n> \n> Can you try this one?\n\nYes, I see more slow-down there. There are many more blobs there, but I\ndon't think it's really the number of them, but their sizes.\n\nThe problem is that both upload-pack and pack-objects are keen to call\nparse_object() on their inputs. For commits, etc, that is usually\nsensible; we have to parse the object to see what it points to. But for\nblobs, the only thing we do is inflate a ton of bytes in order to check\nthe sha1. That's not really productive here; if there is a bit\ncorruption, the client will notice it on the receiving side.\n\nSo doing this:\n\ndiff --git a/object.c b/object.c\nindex 588b8156f1..6fbf6b2a7e 100644\n--- a/object.c\n+++ b/object.c\n@@ -279,10 +279,6 @@ struct object *parse_object(struct repository *r, const struct object_id *oid)\n \tif ((obj && obj->type == OBJ_BLOB && repo_has_object_file(r, oid)) ||\n \t (!obj && repo_has_object_file(r, oid) &&\n \t oid_object_info(r, oid, NULL) == OBJ_BLOB)) {\n-\t\tif (stream_object_signature(r, repl) < 0) {\n-\t\t\terror(_(\"hash mismatch %s\"), oid_to_hex(oid));\n-\t\t\treturn NULL;\n-\t\t}\n \t\tparse_blob_buffer(lookup_blob(r, oid), NULL, 0);\n \t\treturn lookup_object(r, oid);\n \t}\n\nmakes the follow-up fetch very fast. But there are some parts of the\ncode that rely on this parsing to do the on-disk checks. So I think we'd\nprobably need to take a flag, and plumb it through from the appropriate\nspots.\n\nA sample patch is below, which makes things fast for the example I\nshowed (it might not for yours, because if you're using the v0 protocol\nover ssh, I suspect a different parse_object() call might need to be\ntweaked). I think it should be fairly safe, because in both callsites\nwe're already using the commit-graph to avoid checking the sha1 for\ncommits (actually, I think this may be an existing subtle breakage in\n\"rev-list --verify-objects\", but that should be fixed separately).\n\n---\ndiff --git a/object.c b/object.c\nindex 588b8156f1..97324bc406 100644\n--- a/object.c\n+++ b/object.c\n@@ -263,7 +263,9 @@ struct object *parse_object_or_die(const struct object_id *oid,\n \tdie(_(\"unable to parse object: %s\"), name ? name : oid_to_hex(oid));\n }\n \n-struct object *parse_object(struct repository *r, const struct object_id *oid)\n+struct object *parse_object_with_flags(struct repository *r,\n+\t\t\t\t const struct object_id *oid,\n+\t\t\t\t enum parse_object_flags flags)\n {\n \tunsigned long size;\n \tenum object_type type;\n@@ -279,7 +281,8 @@ struct object *parse_object(struct repository *r, const struct object_id *oid)\n \tif ((obj && obj->type == OBJ_BLOB && repo_has_object_file(r, oid)) ||\n \t (!obj && repo_has_object_file(r, oid) &&\n \t oid_object_info(r, oid, NULL) == OBJ_BLOB)) {\n-\t\tif (stream_object_signature(r, repl) < 0) {\n+\t\tif (!(flags & PARSE_OBJECT_SKIP_HASH_CHECK) &&\n+\t\t stream_object_signature(r, repl) < 0) {\n \t\t\terror(_(\"hash mismatch %s\"), oid_to_hex(oid));\n \t\t\treturn NULL;\n \t\t}\n@@ -289,7 +292,8 @@ struct object *parse_object(struct repository *r, const struct object_id *oid)\n \n \tbuffer = repo_read_object_file(r, oid, &type, &size);\n \tif (buffer) {\n-\t\tif (check_object_signature(r, repl, buffer, size, type) < 0) {\n+\t\tif (!(flags & PARSE_OBJECT_SKIP_HASH_CHECK) &&\n+\t\t check_object_signature(r, repl, buffer, size, type) < 0) {\n \t\t\tfree(buffer);\n \t\t\terror(_(\"hash mismatch %s\"), oid_to_hex(repl));\n \t\t\treturn NULL;\n@@ -304,6 +308,11 @@ struct object *parse_object(struct repository *r, const struct object_id *oid)\n \treturn NULL;\n }\n \n+struct object *parse_object(struct repository *r, const struct object_id *oid)\n+{\n+\treturn parse_object_with_flags(r, oid, 0);\n+}\n+\n struct object_list *object_list_insert(struct object *item,\n \t\t\t\t struct object_list **list_p)\n {\ndiff --git a/object.h b/object.h\nindex 9caef89f1f..cd5b7d6306 100644\n--- a/object.h\n+++ b/object.h\n@@ -137,6 +137,13 @@ struct object *parse_object(struct repository *r, const struct object_id *oid);\n */\n struct object *parse_object_or_die(const struct object_id *oid, const char *name);\n \n+enum parse_object_flags {\n+\tPARSE_OBJECT_SKIP_HASH_CHECK = 1 << 0,\n+};\n+struct object *parse_object_with_flags(struct repository *r,\n+\t\t\t\t const struct object_id *oid,\n+\t\t\t\t enum parse_object_flags flags);\n+\n /* Given the result of read_sha1_file(), returns the object after\n * parsing it. eaten_p indicates if the object has a borrowed copy\n * of buffer and the caller should not free() it.\ndiff --git a/revision.c b/revision.c\nindex ee702e498a..8cbffc0325 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -384,7 +384,8 @@ static struct object *get_reference(struct rev_info *revs, const char *name,\n \tif (commit)\n \t\tobject = &commit->object;\n \telse\n-\t\tobject = parse_object(revs->repo, oid);\n+\t\tobject = parse_object_with_flags(revs->repo, oid,\n+\t\t\t\t\t\t PARSE_OBJECT_SKIP_HASH_CHECK);\n \n \tif (!object) {\n \t\tif (revs->ignore_missing)\ndiff --git a/upload-pack.c b/upload-pack.c\nindex b217a1f469..4bacdf087c 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1420,7 +1420,8 @@ static int parse_want(struct packet_writer *writer, const char *line,\n \t\tif (commit)\n \t\t\to = &commit->object;\n \t\telse\n-\t\t\to = parse_object(the_repository, &oid);\n+\t\t\to = parse_object_with_flags(the_repository, &oid,\n+\t\t\t\t\t\t PARSE_OBJECT_SKIP_HASH_CHECK);\n \n \t\tif (!o) {\n \t\t\tpacket_writer_error(writer,\n"},{"id":"462675","messageId":"YxfQi4qg8uJHs7Gp@coredump.intra.peff.net","threadId":"58288","inReplyTo":"YxeTsWrjpKo+JGfq@coredump.intra.peff.net","subject":"[PATCH 0/3] speeding up on-demand fetch for blobs in partial clone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-06T22:58:19Z","receivedAt":"2022-09-06T22:58:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 06, 2022 at 02:38:41PM -0400, Jeff King wrote:\n\n> On Mon, Sep 05, 2022 at 11:17:21AM +0000, 程洋 wrote:\n> \n> > Sorry, I told you the wrong branch. It should be \"android-t-preview-1\"\n> > git clone --filter=blob:none --no-local -b android-t-preview-1 grade-plugin\n> > \n> > Can you try this one?\n> \n> Yes, I see more slow-down there. There are many more blobs there, but I\n> don't think it's really the number of them, but their sizes.\n> \n> The problem is that both upload-pack and pack-objects are keen to call\n> parse_object() on their inputs. For commits, etc, that is usually\n> sensible; we have to parse the object to see what it points to. But for\n> blobs, the only thing we do is inflate a ton of bytes in order to check\n> the sha1. That's not really productive here; if there is a bit\n> corruption, the client will notice it on the receiving side.\n\nSo here's a cleaned-up series which makes this a lot faster.\n\nThe special sauce is in patch 2, along with timings. The first one is\njust preparing, and the final one is a small cleanup it enables.\n\nI based these directly on the current tip of master. There will be a\nsmall conflict in the test script when merging with the \"rev-list\n--verify-objects\" series I just sent in:\n\n https://lore.kernel.org/git/Yxe0k++LA%2FUfFLF%2F@coredump.intra.peff.net/\n\nThe resolution is to just keep the tests added by both sides.\n\n [1/3]: parse_object(): allow skipping hash check\n [2/3]: upload-pack: skip parse-object re-hashing of \"want\" objects\n [3/3]: parse_object(): check commit-graph when skip_hash set\n\n object.c | 21 ++++++++++++++++++---\n object.h | 6 ++++++\n revision.c | 14 +++-----------\n t/t1450-fsck.sh | 20 ++++++++++++++++++++\n upload-pack.c | 8 ++------\n 5 files changed, 49 insertions(+), 20 deletions(-)\n\n-Peff\n"},{"id":"462676","messageId":"YxfRTubqh7aFvNJs@coredump.intra.peff.net","threadId":"58288","inReplyTo":"YxfQi4qg8uJHs7Gp@coredump.intra.peff.net","subject":"[PATCH 1/3] parse_object(): allow skipping hash check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-06T23:01:34Z","receivedAt":"2022-09-06T23:01:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The parse_object() function checks the object hash of any object it\nparses. This is a nice feature, as it means we may catch bit corruption\nduring normal use, rather than waiting for specific fsck operations.\n\nBut it also can be slow. It's particularly noticeable for blobs, where\nexcept for the hash check, we could return without loading the object\ncontents at all. Now one may wonder what is the point of calling\nparse_object() on a blob in the first place then, but usually it's not\nintentional: we were fed an oid from somewhere, don't know the type, and\nwant an object struct. For commits and trees, the parsing is usually\nhelpful; we're about to look at the contents anyway. But this is less\ntrue for blobs, where we may be collecting them as part of a\nreachability traversal, etc, and don't actually care what's in them. And\nblobs, of course, tend to be larger.\n\nWe don't want to just throw out the hash-checks for blobs, though. We do\ndepend on them in some circumstances (e.g., rev-list --verify-objects\nuses parse_object() to check them). It's only the callers that know\nhow they're going to use the result. And so we can help them by\nproviding a special flag to skip the hash check.\n\nWe could just apply this to blobs, as they're going to be the main\nsource of performance improvement. But if a caller doesn't care about\nchecking the hash, we might as well skip it for other object types, too.\nEven though we can't avoid reading the object contents, we can still\nskip the actual hash computation.\n\nIf this seems like it is making Git a little bit less safe against\ncorruption, it may be. But it's part of a series of tradeoffs we're\nalready making. For instance, \"rev-list --objects\" does not open the\ncontents of blobs it prints. And when a commit graph is present, we skip\nopening most commits entirely. The important thing will be to use this\nflag in cases where it's safe to skip the check. For instance, when\nserving a pack for a fetch, we know the client will fully index the\nobjects and do a connectivity check itself. There's little to be gained\nfrom the server side re-hashing a blob itself. And indeed, most of the\ntime we don't! The revision machinery won't open up a blob reached by\ntraversal, but only one requested directly with a \"want\" line. So\napplied properly, this new feature shouldn't make anything less safe in\npractice.\n\nSigned-off-by: Jeff King \n---\nI'm sorry, I know the argument here is really hand-wavy. But I really\nthink this isn't making anything much less safe.\n\nI was actually tempted to rip out the blob hash-check entirely by\ndefault! Anybody who really cares about checking the bits can do so\nwith read_object_file(). That's what fsck does, and we could pretty\neasily convert \"rev-list --verify-objects\" to do so, too. So this is the\nless extreme version of the patch. ;)\n\n object.c | 15 ++++++++++++---\n object.h | 6 ++++++\n 2 files changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex 588b8156f1..8f6de09078 100644\n--- a/object.c\n+++ b/object.c\n@@ -263,8 +263,11 @@ struct object *parse_object_or_die(const struct object_id *oid,\n \tdie(_(\"unable to parse object: %s\"), name ? name : oid_to_hex(oid));\n }\n \n-struct object *parse_object(struct repository *r, const struct object_id *oid)\n+struct object *parse_object_with_flags(struct repository *r,\n+\t\t\t\t const struct object_id *oid,\n+\t\t\t\t enum parse_object_flags flags)\n {\n+\tint skip_hash = !!(flags & PARSE_OBJECT_SKIP_HASH_CHECK);\n \tunsigned long size;\n \tenum object_type type;\n \tint eaten;\n@@ -279,7 +282,7 @@ struct object *parse_object(struct repository *r, const struct object_id *oid)\n \tif ((obj && obj->type == OBJ_BLOB && repo_has_object_file(r, oid)) ||\n \t (!obj && repo_has_object_file(r, oid) &&\n \t oid_object_info(r, oid, NULL) == OBJ_BLOB)) {\n-\t\tif (stream_object_signature(r, repl) < 0) {\n+\t\tif (!skip_hash && stream_object_signature(r, repl) < 0) {\n \t\t\terror(_(\"hash mismatch %s\"), oid_to_hex(oid));\n \t\t\treturn NULL;\n \t\t}\n@@ -289,7 +292,8 @@ struct object *parse_object(struct repository *r, const struct object_id *oid)\n \n \tbuffer = repo_read_object_file(r, oid, &type, &size);\n \tif (buffer) {\n-\t\tif (check_object_signature(r, repl, buffer, size, type) < 0) {\n+\t\tif (!skip_hash &&\n+\t\t check_object_signature(r, repl, buffer, size, type) < 0) {\n \t\t\tfree(buffer);\n \t\t\terror(_(\"hash mismatch %s\"), oid_to_hex(repl));\n \t\t\treturn NULL;\n@@ -304,6 +308,11 @@ struct object *parse_object(struct repository *r, const struct object_id *oid)\n \treturn NULL;\n }\n \n+struct object *parse_object(struct repository *r, const struct object_id *oid)\n+{\n+\treturn parse_object_with_flags(r, oid, 0);\n+}\n+\n struct object_list *object_list_insert(struct object *item,\n \t\t\t\t struct object_list **list_p)\n {\ndiff --git a/object.h b/object.h\nindex 9caef89f1f..31ebe11458 100644\n--- a/object.h\n+++ b/object.h\n@@ -128,7 +128,13 @@ void *object_as_type(struct object *obj, enum object_type type, int quiet);\n *\n * Returns NULL if the object is missing or corrupt.\n */\n+enum parse_object_flags {\n+\tPARSE_OBJECT_SKIP_HASH_CHECK = 1 << 0,\n+};\n struct object *parse_object(struct repository *r, const struct object_id *oid);\n+struct object *parse_object_with_flags(struct repository *r,\n+\t\t\t\t const struct object_id *oid,\n+\t\t\t\t enum parse_object_flags flags);\n \n /*\n * Like parse_object, but will die() instead of returning NULL. If the\n-- \n2.37.3.1134.gfd534b3986\n\n"},{"id":"462677","messageId":"YxfSRkEiiP4TyZTM@coredump.intra.peff.net","threadId":"58288","inReplyTo":"YxfQi4qg8uJHs7Gp@coredump.intra.peff.net","subject":"[PATCH 2/3] upload-pack: skip parse-object re-hashing of \"want\" objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-06T23:05:42Z","receivedAt":"2022-09-06T23:05:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Imagine we have a history with commit C pointing to a large blob B.\nIf a client asks us for C, we can generally serve both objects to them\nwithout accessing the uncompressed contents of B. In upload-pack, we\nfigure out which commits we have and what the client has, and feed those\ntips to pack-objects. In pack-objects, we traverse the commits and trees\n(or use bitmaps!) to find the set of objects needed, but we never open\nup B. When we serve it to the client, we can often pass the compressed\nbytes directly from the on-disk packfile over the wire.\n\nBut if a client asks us directly for B, perhaps because they are doing\nan on-demand fetch to fill in the missing blob of a partial clone, we\nend up much slower. Upload-pack calls parse_object() on the oid we\nreceive, which opens up the object and re-checks its hash (even though\nif it were a commit, we might skip this parse entirely in favor of the\ncommit graph!). And then we feed the oid directly to pack-objects, which\nagain calls parse_object() and opens the object. And then finally, when\nwe write out the result, we may send bytes straight from disk, but only\nafter having unnecessarily uncompressed and computed the sha1 of the\nobject twice!\n\nThis patch teaches both code paths to use the new SKIP_HASH_CHECK flag\nfor parse_object(). You can see the speed-up in p5600, which does a\nblob:none clone followed by a checkout. The savings for git.git are\nmodest:\n\n Test HEAD^ HEAD\n ----------------------------------------------------------------------\n 5600.3: checkout of result 2.23(4.19+0.24) 1.72(3.79+0.18) -22.9%\n\nBut the savings scale with the number of bytes. So on a repository like\nlinux.git with more files, we see more improvement (in both absolute and\nrelative numbers):\n\n Test HEAD^ HEAD\n ----------------------------------------------------------------------------\n 5600.3: checkout of result 51.62(77.26+2.76) 34.86(61.41+2.63) -32.5%\n\nAnd here's an even more extreme case. This is the android gradle-plugin\nrepository, whose tip checkout has ~3.7GB of files:\n\n Test HEAD^ HEAD\n --------------------------------------------------------------------------\n 5600.3: checkout of result 79.51(90.84+5.55) 40.28(51.88+5.67) -49.3%\n\nKeep in mind that these timings are of the whole checkout operation. So\nthey count the client indexing the pack and actually writing out the\nfiles. If we want to see just the server's view, we can hack up the\nGIT_TRACE_PACKET output from those operations and replay it via\nupload-pack. For the gradle example, that gives me:\n\n Benchmark 1: GIT_PROTOCOL=version=2 git.old upload-pack ../gradle-plugin \n---\n revision.c | 4 +++-\n t/t1450-fsck.sh | 20 ++++++++++++++++++++\n upload-pack.c | 3 ++-\n 3 files changed, 25 insertions(+), 2 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex ee702e498a..786e090785 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -384,7 +384,9 @@ static struct object *get_reference(struct rev_info *revs, const char *name,\n \tif (commit)\n \t\tobject = &commit->object;\n \telse\n-\t\tobject = parse_object(revs->repo, oid);\n+\t\tobject = parse_object_with_flags(revs->repo, oid,\n+\t\t\t\t\t\t revs->verify_objects ? 0 :\n+\t\t\t\t\t\t PARSE_OBJECT_SKIP_HASH_CHECK);\n \n \tif (!object) {\n \t\tif (revs->ignore_missing)\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 53c2aa10b7..6410eff4e0 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -507,6 +507,26 @@ test_expect_success 'rev-list --verify-objects with bad sha1' '\n \ttest_i18ngrep -q \"error: hash mismatch $(dirname $new)$(test_oid ff_2)\" out\n '\n \n+# An actual bit corruption is more likely than swapped commits, but\n+# this provides an easy way to have commits which don't match their purported\n+# hashes, but which aren't so broken we can't read them at all.\n+test_expect_success 'rev-list --verify-objects notices swapped commits' '\n+\tgit init swapped-commits &&\n+\t(\n+\t\tcd swapped-commits &&\n+\t\ttest_commit one &&\n+\t\ttest_commit two &&\n+\t\tone_oid=$(git rev-parse HEAD) &&\n+\t\ttwo_oid=$(git rev-parse HEAD^) &&\n+\t\tone=.git/objects/$(test_oid_to_path $one_oid) &&\n+\t\ttwo=.git/objects/$(test_oid_to_path $two_oid) &&\n+\t\tmv $one tmp &&\n+\t\tmv $two $one &&\n+\t\tmv tmp $two &&\n+\t\ttest_must_fail git rev-list --verify-objects HEAD\n+\t)\n+'\n+\n test_expect_success 'force fsck to ignore double author' '\n \tgit cat-file commit HEAD >basis &&\n \tsed \"s/^author .*/&,&/\" multiple-authors &&\ndiff --git a/upload-pack.c b/upload-pack.c\nindex b217a1f469..4bacdf087c 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1420,7 +1420,8 @@ static int parse_want(struct packet_writer *writer, const char *line,\n \t\tif (commit)\n \t\t\to = &commit->object;\n \t\telse\n-\t\t\to = parse_object(the_repository, &oid);\n+\t\t\to = parse_object_with_flags(the_repository, &oid,\n+\t\t\t\t\t\t PARSE_OBJECT_SKIP_HASH_CHECK);\n \n \t\tif (!o) {\n \t\t\tpacket_writer_error(writer,\n-- \n2.37.3.1134.gfd534b3986\n\n"},{"id":"462678","messageId":"YxfScUATMQw9cB6m@coredump.intra.peff.net","threadId":"58288","inReplyTo":"YxfQi4qg8uJHs7Gp@coredump.intra.peff.net","subject":"[PATCH 3/3] parse_object(): check commit-graph when skip_hash set","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-06T23:06:25Z","receivedAt":"2022-09-06T23:06:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If the caller told us that they don't care about us checking the object\nhash, then we're free to implement any optimizations that get us the\nparsed value more quickly. An obvious one is to check the commit graph\nbefore loading an object from disk. And in fact, both of the callers who\npass in this flag are already doing so before they call parse_object()!\n\nSo we can simplify those callers, as well as any possible future ones,\nby moving the logic into parse_object().\n\nThere are two subtle things to note in the diff, but neither has any\nimpact in practice:\n\n - it seems least-surprising here to do the graph lookup on the\n git-replace'd oid, rather than the original. This is in theory a\n change of behavior from the earlier code, as neither caller did a\n replace lookup itself. But in practice it doesn't matter, as we\n disable the commit graph entirely if there are any replace refs.\n\n - the caller in get_reference() passes the skip_hash flag only if\n revs->verify_objects isn't set, whereas it would look in the commit\n graph unconditionally. In practice this should not matter as we\n should disable the commit graph entirely when using verify_objects\n (and that was done recently in another patch).\n\nSo this should be a pure cleanup with no behavior change.\n\nSigned-off-by: Jeff King \n---\n object.c | 6 ++++++\n revision.c | 16 +++-------------\n upload-pack.c | 9 ++-------\n 3 files changed, 11 insertions(+), 20 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex 8f6de09078..2e4589bae5 100644\n--- a/object.c\n+++ b/object.c\n@@ -279,6 +279,12 @@ struct object *parse_object_with_flags(struct repository *r,\n \tif (obj && obj->parsed)\n \t\treturn obj;\n \n+\tif (skip_hash) {\n+\t\tstruct commit *commit = lookup_commit_in_graph(r, repl);\n+\t\tif (commit)\n+\t\t\treturn &commit->object;\n+\t}\n+\n \tif ((obj && obj->type == OBJ_BLOB && repo_has_object_file(r, oid)) ||\n \t (!obj && repo_has_object_file(r, oid) &&\n \t oid_object_info(r, oid, NULL) == OBJ_BLOB)) {\ndiff --git a/revision.c b/revision.c\nindex 786e090785..a04ebd6139 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -373,20 +373,10 @@ static struct object *get_reference(struct rev_info *revs, const char *name,\n \t\t\t\t unsigned int flags)\n {\n \tstruct object *object;\n-\tstruct commit *commit;\n \n-\t/*\n-\t * If the repository has commit graphs, we try to opportunistically\n-\t * look up the object ID in those graphs. Like this, we can avoid\n-\t * parsing commit data from disk.\n-\t */\n-\tcommit = lookup_commit_in_graph(revs->repo, oid);\n-\tif (commit)\n-\t\tobject = &commit->object;\n-\telse\n-\t\tobject = parse_object_with_flags(revs->repo, oid,\n-\t\t\t\t\t\t revs->verify_objects ? 0 :\n-\t\t\t\t\t\t PARSE_OBJECT_SKIP_HASH_CHECK);\n+\tobject = parse_object_with_flags(revs->repo, oid,\n+\t\t\t\t\t revs->verify_objects ? 0 :\n+\t\t\t\t\t PARSE_OBJECT_SKIP_HASH_CHECK);\n \n \tif (!object) {\n \t\tif (revs->ignore_missing)\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 4bacdf087c..35fe1a3dbb 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1409,19 +1409,14 @@ static int parse_want(struct packet_writer *writer, const char *line,\n \tconst char *arg;\n \tif (skip_prefix(line, \"want \", &arg)) {\n \t\tstruct object_id oid;\n-\t\tstruct commit *commit;\n \t\tstruct object *o;\n \n \t\tif (get_oid_hex(arg, &oid))\n \t\t\tdie(\"git upload-pack: protocol error, \"\n \t\t\t \"expected to get oid, not '%s'\", line);\n \n-\t\tcommit = lookup_commit_in_graph(the_repository, &oid);\n-\t\tif (commit)\n-\t\t\to = &commit->object;\n-\t\telse\n-\t\t\to = parse_object_with_flags(the_repository, &oid,\n-\t\t\t\t\t\t PARSE_OBJECT_SKIP_HASH_CHECK);\n+\t\to = parse_object_with_flags(the_repository, &oid,\n+\t\t\t\t\t PARSE_OBJECT_SKIP_HASH_CHECK);\n \n \t\tif (!o) {\n \t\t\tpacket_writer_error(writer,\n-- \n2.37.3.1134.gfd534b3986\n"},{"id":"462713","messageId":"f79b0ccd-3e36-f447-0dbb-6e40ad547c8d@github.com","threadId":"58288","inReplyTo":"YxfRTubqh7aFvNJs@coredump.intra.peff.net","subject":"Re: [PATCH 1/3] parse_object(): allow skipping hash check","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-07T14:15:37Z","receivedAt":"2022-09-07T14:15:42Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/6/2022 7:01 PM, Jeff King wrote:\n> I'm sorry, I know the argument here is really hand-wavy. But I really\n> think this isn't making anything much less safe.\n\nI agree with you that this is the safest way to move forward here.\n \n> I was actually tempted to rip out the blob hash-check entirely by\n> default! Anybody who really cares about checking the bits can do so\n> with read_object_file(). That's what fsck does, and we could pretty\n> easily convert \"rev-list --verify-objects\" to do so, too. So this is the\n> less extreme version of the patch. ;)\n\nA quick search shows many uses of parse_object() across the codebase.\nIt would certainly be nice if they all suddenly got faster by avoiding\nthis hashing, but I also suppose that most of the calls are using\nparse_object() only because they are unsure if they are parsing a\ncommit or a tag and would never parse a large blob.\n\nI think this approach of making parse_object_with_flags() is the best\nway to incrementally approach things here. If we decide that we need\nthe _with_flags() version specifically to avoid this hash check, then\nwe could probably take the second approach: remove the hash check from\nparse_object() and swap the places that care to use read_object_file()\ninstead. My guess is that in the long term there will be fewer swaps\nto read_object_file() than to parse_object_with_flags().\n\nHowever, this is a good first step to make progress without doing the\ntime-consuming audit of every caller to parse_object().\n\nThanks,\n-Stolee\n"},{"id":"462714","messageId":"6018c526-4641-8374-8802-cfda5be330c3@github.com","threadId":"58288","inReplyTo":"YxfSRkEiiP4TyZTM@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] upload-pack: skip parse-object re-hashing of \"want\" objects","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-07T14:36:14Z","receivedAt":"2022-09-07T14:36:19Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/6/2022 7:05 PM, Jeff King wrote:\n> This patch teaches both code paths to use the new SKIP_HASH_CHECK flag\n> for parse_object(). You can see the speed-up in p5600, which does a\n> blob:none clone followed by a checkout. The savings for git.git are\n> modest:\n> \n> Test HEAD^ HEAD\n> ----------------------------------------------------------------------\n> 5600.3: checkout of result 2.23(4.19+0.24) 1.72(3.79+0.18) -22.9%\n> \n> But the savings scale with the number of bytes. So on a repository like\n> linux.git with more files, we see more improvement (in both absolute and\n> relative numbers):\n> \n> Test HEAD^ HEAD\n> ----------------------------------------------------------------------------\n> 5600.3: checkout of result 51.62(77.26+2.76) 34.86(61.41+2.63) -32.5%\n> \n> And here's an even more extreme case. This is the android gradle-plugin\n> repository, whose tip checkout has ~3.7GB of files:\n> \n> Test HEAD^ HEAD\n> --------------------------------------------------------------------------\n> 5600.3: checkout of result 79.51(90.84+5.55) 40.28(51.88+5.67) -49.3%\n> \n> Keep in mind that these timings are of the whole checkout operation.\n\nThese numbers are _very_ impressive for this user-facing scenario.\n\n> Benchmark 1: GIT_PROTOCOL=version=2 git.old upload-pack ../gradle-plugin Time (mean ± σ): 50.884 s ± 0.239 s [User: 51.450 s, System: 1.726 s]\n> Range (min … max): 50.608 s … 51.025 s 3 runs\n> \n> Benchmark 2: GIT_PROTOCOL=version=2 git.new upload-pack ../gradle-plugin Time (mean ± σ): 9.728 s ± 0.112 s [User: 10.466 s, System: 1.535 s]\n> Range (min … max): 9.618 s … 9.842 s 3 runs\n> \n> Summary\n> 'GIT_PROTOCOL=version=2 git.new upload-pack ../gradle-plugin 5.23 ± 0.07 times faster than 'GIT_PROTOCOL=version=2 git.old upload-pack ../gradle-plugin \n> So a server would see an 80% reduction in CPU serving the initial\n> checkout of a partial clone for this repository.\n\nVery nice!\n\n> In both cases skipping the extra hashing on the server should be pretty\n> safe. The client doesn't trust the server anyway, so it will re-hash all\n> of the objects via index-pack.\n\nDefinitely safe for this target scenario.\n\n> There is one thing to note, though: the\n> change in get_reference() affects not just pack-objects, but rev-list,\n> git-log, etc. We could use a flag to limit to index-pack here, but we\n> may already skip hash checks in this instance. For commits, we'd skip\n> anything we load via the commit-graph. And while before this commit we\n> would check a blob fed directly to rev-list on the command-line, we'd\n> skip checking that same blob if we found it by traversing a tree.\n\nIt's worth also mentioning that since you isolated the change to\nget_reference(), it is only skipping the parse for the requested tips\nof the revision walk. As we follow commits and trees to other objects\nwe do not use parse_object() but instead use the appropriate method\n(parse_commit(), parse_tree(), and do not even parse blobs).\n\nThis is additionally confirmed that p0001-rev-list.sh has no measurable\nchange in results with this commit, even when disabling the commit-graph.\n\n> The exception for both is if --verify-objects is used. In that case,\n> we'll skip this optimization, and the new test makes sure we do this\n> correctly.\n\nThe test for this is clever. Thanks for adding it.\n\nCode and test look good.\n\nThanks,\n-Stolee\n"},{"id":"462726","messageId":"3f901613-72c6-b644-079a-f74e3024a8fe@github.com","threadId":"58288","inReplyTo":"6018c526-4641-8374-8802-cfda5be330c3@github.com","subject":"Re: [PATCH 2/3] upload-pack: skip parse-object re-hashing of \"want\" objects","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-07T14:45:39Z","receivedAt":"2022-09-07T14:45:57Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/7/2022 10:36 AM, Derrick Stolee wrote:\n> On 9/6/2022 7:05 PM, Jeff King wrote:\n\n>> There is one thing to note, though: the\n>> change in get_reference() affects not just pack-objects, but rev-list,\n>> git-log, etc. We could use a flag to limit to index-pack here, but we\n>> may already skip hash checks in this instance. For commits, we'd skip\n>> anything we load via the commit-graph. And while before this commit we\n>> would check a blob fed directly to rev-list on the command-line, we'd\n>> skip checking that same blob if we found it by traversing a tree.\n> \n> It's worth also mentioning that since you isolated the change to\n> get_reference(), it is only skipping the parse for the requested tips\n> of the revision walk. As we follow commits and trees to other objects\n> we do not use parse_object() but instead use the appropriate method\n> (parse_commit(), parse_tree(), and do not even parse blobs).\n\nAfter writing this, it was bothering me that 'rev-list --verify' should\nbe checking for corruption throughout the history, not just at the tips.\n\nThis is done via the condition in builtin/rev-list.c:finish_object():\n\nstatic int finish_object(struct object *obj, const char *name, void *cb_data)\n{\n\tstruct rev_list_info *info = cb_data;\n\tif (oid_object_info_extended(the_repository, &obj->oid, NULL, 0) < 0) {\n\t\tfinish_object__ma(obj);\n\t\treturn 1;\n\t}\n\tif (info->revs->verify_objects && !obj->parsed && obj->type != OBJ_COMMIT)\n\t\tparse_object(the_repository, &obj->oid);\n\treturn 0;\n}\n\nSo this call is the one that is used most-often by the rev-list command,\nbut isn't necessary to update for the purpose of our desired speedup. This\nis another place where we would want to use read_object_file(). (It may even\nbe the _only_ place, since finish_object() should be called even for the\ntip objects.)\n\nIn case you think it is valuable to ensure we test both cases (\"tip\" and\n\"not tip\") I modified your test to have a third commit and test two rev-list\ncalls:\n\n# An actual bit corruption is more likely than swapped commits, but\n# this provides an easy way to have commits which don't match their purported\n# hashes, but which aren't so broken we can't read them at all.\ntest_expect_success 'rev-list --verify-objects notices swapped commits' '\n\tgit init swapped-commits &&\n\t(\n\t\tcd swapped-commits &&\n\t\ttest_commit one &&\n\t\ttest_commit two &&\n\t\ttest_commit three &&\n\t\tone_oid=$(git rev-parse HEAD) &&\n\t\ttwo_oid=$(git rev-parse HEAD^) &&\n\t\tone=.git/objects/$(test_oid_to_path $one_oid) &&\n\t\ttwo=.git/objects/$(test_oid_to_path $two_oid) &&\n\t\tmv $one tmp &&\n\t\tmv $two $one &&\n\t\tmv tmp $two &&\n\t\ttest_must_fail git rev-list --verify-objects HEAD &&\n\t\ttest_must_fail git rev-list --verify-objects HEAD^\n\t)\n'\n\nBut this is likely overkill.\n\nThanks,\n-Stolee\n"},{"id":"462727","messageId":"60a47a3f-e4d5-2339-d79e-48e6a7d4a9f1@github.com","threadId":"58288","inReplyTo":"YxfScUATMQw9cB6m@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] parse_object(): check commit-graph when skip_hash set","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-07T14:46:55Z","receivedAt":"2022-09-07T14:47:16Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/6/2022 7:06 PM, Jeff King wrote:\n> If the caller told us that they don't care about us checking the object\n> hash, then we're free to implement any optimizations that get us the\n> parsed value more quickly. An obvious one is to check the commit graph\n> before loading an object from disk. And in fact, both of the callers who\n> pass in this flag are already doing so before they call parse_object()!\n> \n> So we can simplify those callers, as well as any possible future ones,\n> by moving the logic into parse_object().\n\nNice!\n \n> There are two subtle things to note in the diff, but neither has any\n> impact in practice:\n> \n> - it seems least-surprising here to do the graph lookup on the\n> git-replace'd oid, rather than the original. This is in theory a\n> change of behavior from the earlier code, as neither caller did a\n> replace lookup itself. But in practice it doesn't matter, as we\n> disable the commit graph entirely if there are any replace refs.\n\nI can confirm that this is the case.\n\n> - the caller in get_reference() passes the skip_hash flag only if\n> revs->verify_objects isn't set, whereas it would look in the commit\n> graph unconditionally. In practice this should not matter as we\n> should disable the commit graph entirely when using verify_objects\n> (and that was done recently in another patch).\n> \n> So this should be a pure cleanup with no behavior change.\n\nExcellent, thanks!\n\n-Stolee\n\n"},{"id":"462728","messageId":"fc4ce953-1a3b-8d94-d602-ad3e195f08c9@github.com","threadId":"58288","inReplyTo":"YxfQi4qg8uJHs7Gp@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] speeding up on-demand fetch for blobs in partial clone","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-07T14:48:04Z","receivedAt":"2022-09-07T14:48:13Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/6/2022 6:58 PM, Jeff King wrote:\n> On Tue, Sep 06, 2022 at 02:38:41PM -0400, Jeff King wrote:\n> \n>> On Mon, Sep 05, 2022 at 11:17:21AM +0000, 程洋 wrote:\n>>\n>>> Sorry, I told you the wrong branch. It should be \"android-t-preview-1\"\n>>> git clone --filter=blob:none --no-local -b android-t-preview-1 grade-plugin\n>>>\n>>> Can you try this one?\n>>\n>> Yes, I see more slow-down there. There are many more blobs there, but I\n>> don't think it's really the number of them, but their sizes.\n>>\n>> The problem is that both upload-pack and pack-objects are keen to call\n>> parse_object() on their inputs. For commits, etc, that is usually\n>> sensible; we have to parse the object to see what it points to. But for\n>> blobs, the only thing we do is inflate a ton of bytes in order to check\n>> the sha1. That's not really productive here; if there is a bit\n>> corruption, the client will notice it on the receiving side.\n\nThanks for finding this very subtle issue!\n \n> So here's a cleaned-up series which makes this a lot faster.\n> \n> The special sauce is in patch 2, along with timings. The first one is\n> just preparing, and the final one is a small cleanup it enables.\n\nI carefully read these patches as well as applied them on my machine\nand did some extra digging and performance tests to understand the\nchange.\n\nLGTM.\n\nThanks,\n-Stolee\n"},{"id":"462750","messageId":"xmqq1qsnugsu.fsf@gitster.g","threadId":"58288","inReplyTo":"YxfSRkEiiP4TyZTM@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] upload-pack: skip parse-object re-hashing of \"want\" objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-07T19:26:41Z","receivedAt":"2022-09-07T19:26:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The exception for both is if --verify-objects is used. In that case,\n> we'll skip this optimization, and the new test makes sure we do this\n> correctly.\n\nI wondered if we want to test the change on the \"upload-pack\" side\nby going in to the swapped-commits repository, running upload-pack\nmanually and seeing that it spews unusable output without failing,\nbut it probably is not worth the effort. We have plenty of tests\nthat exercises upload-pack in \"good\" cases. What might be a good\ntest is to try fetching from swapped-commits repository and make\nsure that index-pack on the receiving end notices, but I suspect we\nalready have such a \"fetch/clone from a corrupt repository\" test,\nin which case we do not have to add one.\n\nThanks.\n\n\n"},{"id":"462751","messageId":"xmqqv8pzt20d.fsf@gitster.g","threadId":"58288","inReplyTo":"YxfScUATMQw9cB6m@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] parse_object(): check commit-graph when skip_hash set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-07T19:31:30Z","receivedAt":"2022-09-07T19:31:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If the caller told us that they don't care about us checking the object\n> hash, then we're free to implement any optimizations that get us the\n> parsed value more quickly. An obvious one is to check the commit graph\n> before loading an object from disk. And in fact, both of the callers who\n> pass in this flag are already doing so before they call parse_object()!\n>\n> So we can simplify those callers, as well as any possible future ones,\n> by moving the logic into parse_object().\n\nNicely done.\n"},{"id":"462756","messageId":"YxkAxutS+B8//0WF@coredump.intra.peff.net","threadId":"58288","inReplyTo":"xmqq1qsnugsu.fsf@gitster.g","subject":"Re: [PATCH 2/3] upload-pack: skip parse-object re-hashing of \"want\" objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-07T20:36:22Z","receivedAt":"2022-09-07T20:36:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 07, 2022 at 12:26:41PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > The exception for both is if --verify-objects is used. In that case,\n> > we'll skip this optimization, and the new test makes sure we do this\n> > correctly.\n> \n> I wondered if we want to test the change on the \"upload-pack\" side\n> by going in to the swapped-commits repository, running upload-pack\n> manually and seeing that it spews unusable output without failing,\n> but it probably is not worth the effort. We have plenty of tests\n> that exercises upload-pack in \"good\" cases. What might be a good\n> test is to try fetching from swapped-commits repository and make\n> sure that index-pack on the receiving end notices, but I suspect we\n> already have such a \"fetch/clone from a corrupt repository\" test,\n> in which case we do not have to add one.\n\nThere are some tests in t1060 that cover transfer of corrupted objects.\nBut one of the tricky things about corruption is that it can be somewhat\narbitrary where and when we notice. I.e., whether upload-pack notices\nthe problem and bails or whether it spews an invalid result, we're OK\nwith either outcome, and a test for it can end up rather fragile.\n\nWhat we do care about is that _somebody_ notices the problem. t1060\ncovers that for some cases, though of course there's no partial clone\nthere. If we add one like this:\n\ndiff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh\nindex 5b8e47e346..fd18a8b29e 100755\n--- a/t/t1060-object-corruption.sh\n+++ b/t/t1060-object-corruption.sh\n@@ -139,4 +139,11 @@ test_expect_success 'internal tree objects are not \"missing\"' '\n \t)\n '\n \n+test_expect_success 'partial clone of corrupted reository' '\n+\ttest_config -C bit-error uploadpack.allowFilter true &&\n+\tgit clone --no-local --no-checkout --filter=blob:none \\\n+\t\tbit-error corrupt-partial && \\\n+\ttest_must_fail git -C corrupt-partial checkout --force\n+'\n+\n test_done\n\nwe can see that the initial blob:none clone is OK (since neither side\nlooks at the corrupted blob at all). And then the checkout does barf as\nexpected. But it looks like we catch the error in upload-pack, probably\nbecause the loose object is so corrupted that we cannot even access its\ntype header. I tried moving the corruption to further in the file, but I\nthink it's simply so small that zlib will read the whole input and\ncomplain about the bogus crc.\n\nHmm. Looks like that script has another corrupted repo where an object\nis misnamed. So if we s/bit-error/misnamed/ in the test above, then we\ndo trigger the new code. Before we get:\n\n error: hash mismatch d95f3ad14dee633a758d2e331151e950dd13e4ed\n fatal: git upload-pack: not our ref d95f3ad14dee633a758d2e331151e950dd13e4ed\n fatal: remote error: upload-pack: not our ref d95f3ad14dee633a758d2e331151e950dd13e4ed\n\n(the error is a bit misleading, but I guess the inability to create the\nobject struct means our \"is it our ref\" code gets confused). After my\npatches, we get:\n\n remote: Enumerating objects: 1, done.\n remote: Counting objects: 100% (1/1), done.\n remote: Total 1 (delta 0), reused 0 (delta 0), pack-reused 0\n Receiving objects: 100% (1/1), 49 bytes | 49.00 KiB/s, done.\n fatal: bad revision 'd95f3ad14dee633a758d2e331151e950dd13e4ed'\n error: [...]/misnamed did not send all necessary objects\n\nIs that worth having? I dunno. It's kind of brittle in that a later\nchange could mean we're finding the corruption elsewhere, and not\nchecking exactly what we think we are. OTOH, it probably doesn't hurt to\ncover more cases here, and it's not a very expensive test.\n\n-Peff\n"},{"id":"462757","messageId":"YxkCrAocfstZod8a@coredump.intra.peff.net","threadId":"58288","inReplyTo":"f79b0ccd-3e36-f447-0dbb-6e40ad547c8d@github.com","subject":"Re: [PATCH 1/3] parse_object(): allow skipping hash check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-07T20:44:28Z","receivedAt":"2022-09-07T20:44:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 07, 2022 at 10:15:37AM -0400, Derrick Stolee wrote:\n\n> A quick search shows many uses of parse_object() across the codebase.\n> It would certainly be nice if they all suddenly got faster by avoiding\n> this hashing, but I also suppose that most of the calls are using\n> parse_object() only because they are unsure if they are parsing a\n> commit or a tag and would never parse a large blob.\n\nYeah. I think we use parse_object() as a catch-all for \"somebody gave us\nan oid, and we need to know what it is\". I suspect that most normal uses\nwould not get much faster, because we typically do feed it non-blob\nobjects that are small, and whose contents we need to access anyway to\nparse them. So we're paying only the overhead of sha1 on a buffer we\nalready have in memory.\n\nIn cases where we might not need the parsed contents at all, our best\nbet is to actually remove or delay the parsing entirely. E.g., I think\nupload-pack used to be aggressive about parsing the ref tips that it\nadvertised, but really we can just tell the client about them and only\nparse the ones they ask for.\n\n> I think this approach of making parse_object_with_flags() is the best\n> way to incrementally approach things here. If we decide that we need\n> the _with_flags() version specifically to avoid this hash check, then\n> we could probably take the second approach: remove the hash check from\n> parse_object() and swap the places that care to use read_object_file()\n> instead. My guess is that in the long term there will be fewer swaps\n> to read_object_file() than to parse_object_with_flags().\n> \n> However, this is a good first step to make progress without doing the\n> time-consuming audit of every caller to parse_object().\n\nThe notion that this hash check in parse_object() might be slow is\ncertainly not new. I've been thinking about it for at least a decade. ;)\nBut until this recent case of direct-fetching blobs, I hadn't seen an\ninstance where it really made a significant and measurable difference.\n\nSo I'm definitely not opposed to going to a world where we drop the\nextra hash checks entirely, if that buys us something. The\nincrementalism is conservative, but it also makes it easy to\nconvert specific call-sites to measure the outcomes.\n\n-Peff\n"},{"id":"462758","messageId":"001201d8c2fb$3f1c51b0$bd54f510$@nexbridge.com","threadId":"58288","inReplyTo":"YxkAxutS+B8//0WF@coredump.intra.peff.net","subject":"[BUG] t1800: Fails for error text comparison","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2022-09-07T20:48:49Z","receivedAt":"2022-09-07T20:49:00Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"I am finding an issue with t1800.16 failing as a result of a text compare:\n\n-fatal: cannot run bad-hooks/test-hook: ...\n+fatal: cannot exec 'bad-hooks/test-hook': Permission denied\n\nI don't think this is actually a failure condition but the message text is platform and shell specific.\n\nIs there any clean way around this?\n\nThanks,\nRandall\n--\nBrief whoami: NonStop&UNIX developer since approximately\nUNIX(421664400)\nNonStop(211288444200000000)\n-- In real life, I talk too much.\n\n\n\n"},{"id":"462760","messageId":"YxkEAx/qq5Rmn/Yx@coredump.intra.peff.net","threadId":"58288","inReplyTo":"3f901613-72c6-b644-079a-f74e3024a8fe@github.com","subject":"Re: [PATCH 2/3] upload-pack: skip parse-object re-hashing of \"want\" objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-07T20:50:11Z","receivedAt":"2022-09-07T20:50:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 07, 2022 at 10:45:39AM -0400, Derrick Stolee wrote:\n\n> After writing this, it was bothering me that 'rev-list --verify' should\n> be checking for corruption throughout the history, not just at the tips.\n> \n> This is done via the condition in builtin/rev-list.c:finish_object():\n> \n> static int finish_object(struct object *obj, const char *name, void *cb_data)\n> {\n> \tstruct rev_list_info *info = cb_data;\n> \tif (oid_object_info_extended(the_repository, &obj->oid, NULL, 0) < 0) {\n> \t\tfinish_object__ma(obj);\n> \t\treturn 1;\n> \t}\n> \tif (info->revs->verify_objects && !obj->parsed && obj->type != OBJ_COMMIT)\n> \t\tparse_object(the_repository, &obj->oid);\n> \treturn 0;\n> }\n> \n> So this call is the one that is used most-often by the rev-list command,\n> but isn't necessary to update for the purpose of our desired speedup. This\n> is another place where we would want to use read_object_file(). (It may even\n> be the _only_ place, since finish_object() should be called even for the\n> tip objects.)\n\nYeah, I think this is the only place. At least, if you remove the hash\ncheck entirely, then this parse_object() is the only spots that causes a\nproblem (via the existing test in t1450).\n\n> In case you think it is valuable to ensure we test both cases (\"tip\" and\n> \"not tip\") I modified your test to have a third commit and test two rev-list\n> calls:\n\nI don't think it's important to test here, as we know we just touched\nthe tip code here. But also, that existing \"rev-list --verify-objects\"\ntest in t1450 is covering that case: it corrupts a blob that is\nreachable from an otherwise good commit/tree.\n\nWe should also have tip and non-tip tests for commits, but I posted\nthose in a separate series. ;)\n\n-Peff\n"},{"id":"462762","messageId":"YxkG3A30vNfu55Sx@coredump.intra.peff.net","threadId":"58288","inReplyTo":"YxkAxutS+B8//0WF@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] upload-pack: skip parse-object re-hashing of \"want\" objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-07T21:02:20Z","receivedAt":"2022-09-07T21:02:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 07, 2022 at 04:36:22PM -0400, Jeff King wrote:\n\n> Is that worth having? I dunno. It's kind of brittle in that a later\n> change could mean we're finding the corruption elsewhere, and not\n> checking exactly what we think we are. OTOH, it probably doesn't hurt to\n> cover more cases here, and it's not a very expensive test.\n\nSo here's what it would look like as a patch on top.\n\nIt could also be squashed in, of course, which saves the vague reference\nto \"a recent commit...\". OTOH, it's sufficiently complicated to discuss\nthe testing that it's nice not to overwhelm the already-long commit\nmessage of the original. ;)\n\n-- >8 --\nSubject: [PATCH] t1060: check partial clone of misnamed blob\n\nA recent commit (upload-pack: skip parse-object re-hashing of \"want\"\nobjects, 2022-09-06) loosened the behavior of upload-pack so that it\ndoes not verify the sha1 of objects it receives directly via \"want\"\nrequests. The existing corruption tests in t1060 aren't affected by\nthis: the corruptions are blobs reachable from commits, and the client\nrequests the commits.\n\nThe more interesting case here is a partial clone, where the client will\ndirectly ask for the corrupted blob when it does an on-demand fetch of\nthe filtered object. And that is not covered at all, so let's add a\ntest.\n\nIt's important here that we use the \"misnamed\" corruption and not\n\"bit-error\". The latter is sufficiently corrupted that upload-pack\ncannot even figure out the type of the object, so it bails identically\nboth before and after the recent change. But with \"misnamed\", with the\nhash-checks enabled it sees the problem (though the error messages are a\nbit confusing because of the inability to create a \"struct object\" to\nstore the flags):\n\n error: hash mismatch d95f3ad14dee633a758d2e331151e950dd13e4ed\n fatal: git upload-pack: not our ref d95f3ad14dee633a758d2e331151e950dd13e4ed\n fatal: remote error: upload-pack: not our ref d95f3ad14dee633a758d2e331151e950dd13e4ed\n\nAfter the change to skip the hash check, the server side happily sends\nthe bogus object, but the client correctly realizes that it did not get\nthe necessary data:\n\n remote: Enumerating objects: 1, done.\n remote: Counting objects: 100% (1/1), done.\n remote: Total 1 (delta 0), reused 0 (delta 0), pack-reused 0\n Receiving objects: 100% (1/1), 49 bytes | 49.00 KiB/s, done.\n fatal: bad revision 'd95f3ad14dee633a758d2e331151e950dd13e4ed'\n error: [...]/misnamed did not send all necessary objects\n\nwhich is exactly what we expect to happen.\n\nSigned-off-by: Jeff King \n---\n t/t1060-object-corruption.sh | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh\nindex 5b8e47e346..35261afc9d 100755\n--- a/t/t1060-object-corruption.sh\n+++ b/t/t1060-object-corruption.sh\n@@ -139,4 +139,11 @@ test_expect_success 'internal tree objects are not \"missing\"' '\n \t)\n '\n \n+test_expect_success 'partial clone of corrupted repository' '\n+\ttest_config -C misnamed uploadpack.allowFilter true &&\n+\tgit clone --no-local --no-checkout --filter=blob:none \\\n+\t\tmisnamed corrupt-partial && \\\n+\ttest_must_fail git -C corrupt-partial checkout --force\n+'\n+\n test_done\n-- \n2.37.3.1138.gedcf976b26\n\n"},{"id":"462767","messageId":"xmqqh71isvc8.fsf@gitster.g","threadId":"58288","inReplyTo":"001201d8c2fb$3f1c51b0$bd54f510$@nexbridge.com","subject":"Re: [BUG] t1800: Fails for error text comparison","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-07T21:55:35Z","receivedAt":"2022-09-07T21:55:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":" writes:\n\n> I am finding an issue with t1800.16 failing as a result of a text compare:\n>\n> -fatal: cannot run bad-hooks/test-hook: ...\n> +fatal: cannot exec 'bad-hooks/test-hook': Permission denied\n>\n> I don't think this is actually a failure condition but the message\n> text is platform and shell specific.\n\nIsn't this coming from piece of code in start_command()?\n\n\t\t/*\n\t\t * Attempt to exec using the command and arguments starting at\n\t\t * argv.argv[1]. argv.argv[0] contains SHELL_PATH which will\n\t\t * be used in the event exec failed with ENOEXEC at which point\n\t\t * we will try to interpret the command using 'sh'.\n\t\t */\n\t\texecve(argv.v[1], (char *const *) argv.v + 1,\n\t\t (char *const *) childenv);\n\t\tif (errno == ENOEXEC)\n\t\t\texecve(argv.v[0], (char *const *) argv.v,\n\t\t\t (char *const *) childenv);\n\n\t\tif (errno == ENOENT) {\n\t\t\tif (cmd->silent_exec_failure)\n\t\t\t\tchild_die(CHILD_ERR_SILENT);\n\t\t\tchild_die(CHILD_ERR_ENOENT);\n\t\t} else {\n\t\t\tchild_die(CHILD_ERR_ERRNO);\n\t\t}\n\nThe test apparently expects CHILD_ERR_NOENT, which comes from\nchild_err_spew()\n\n\tcase CHILD_ERR_ENOENT:\n\t\terror_errno(\"cannot run %s\", cmd->args.v[0]);\n\t\tbreak;\n\tcase CHILD_ERR_SILENT:\n\t\tbreak;\n\tcase CHILD_ERR_ERRNO:\n\t\terror_errno(\"cannot exec '%s'\", cmd->args.v[0]);\n\t\tbreak;\n\t}\n\nbut somehow your system fails the execve() with something other than\nENOENT.\n"},{"id":"462768","messageId":"xmqq8rmususl.fsf@gitster.g","threadId":"58288","inReplyTo":"YxkG3A30vNfu55Sx@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] upload-pack: skip parse-object re-hashing of \"want\" objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-07T22:07:22Z","receivedAt":"2022-09-07T22:07:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Subject: [PATCH] t1060: check partial clone of misnamed blob\n>\n> A recent commit (upload-pack: skip parse-object re-hashing of \"want\"\n> objects, 2022-09-06) loosened the behavior of upload-pack so that it\n> does not verify the sha1 of objects it receives directly via \"want\"\n> requests. The existing corruption tests in t1060 aren't affected by\n> this: the corruptions are blobs reachable from commits, and the client\n> requests the commits.\n>\n> The more interesting case here is a partial clone, where the client will\n> directly ask for the corrupted blob when it does an on-demand fetch of\n> the filtered object. And that is not covered at all, so let's add a\n> test.\n>\n> It's important here that we use the \"misnamed\" corruption and not\n> \"bit-error\". The latter is sufficiently corrupted that upload-pack\n> cannot even figure out the type of the object, so it bails identically\n> both before and after the recent change. But with \"misnamed\", with the\n> hash-checks enabled it sees the problem (though the error messages are a\n> bit confusing because of the inability to create a \"struct object\" to\n> store the flags):\n\nMakes sense.\n\n> error: hash mismatch d95f3ad14dee633a758d2e331151e950dd13e4ed\n> fatal: git upload-pack: not our ref d95f3ad14dee633a758d2e331151e950dd13e4ed\n> fatal: remote error: upload-pack: not our ref d95f3ad14dee633a758d2e331151e950dd13e4ed\n>\n> After the change to skip the hash check, the server side happily sends\n> the bogus object, but the client correctly realizes that it did not get\n> the necessary data:\n>\n> remote: Enumerating objects: 1, done.\n> remote: Counting objects: 100% (1/1), done.\n> remote: Total 1 (delta 0), reused 0 (delta 0), pack-reused 0\n> Receiving objects: 100% (1/1), 49 bytes | 49.00 KiB/s, done.\n> fatal: bad revision 'd95f3ad14dee633a758d2e331151e950dd13e4ed'\n> error: [...]/misnamed did not send all necessary objects\n>\n> which is exactly what we expect to happen.\n\nThanks. Let's queue it on top.\n\n> Signed-off-by: Jeff King \n> ---\n> t/t1060-object-corruption.sh | 7 +++++++\n> 1 file changed, 7 insertions(+)\n>\n> diff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh\n> index 5b8e47e346..35261afc9d 100755\n> --- a/t/t1060-object-corruption.sh\n> +++ b/t/t1060-object-corruption.sh\n> @@ -139,4 +139,11 @@ test_expect_success 'internal tree objects are not \"missing\"' '\n> \t)\n> '\n> \n> +test_expect_success 'partial clone of corrupted repository' '\n> +\ttest_config -C misnamed uploadpack.allowFilter true &&\n> +\tgit clone --no-local --no-checkout --filter=blob:none \\\n> +\t\tmisnamed corrupt-partial && \\\n> +\ttest_must_fail git -C corrupt-partial checkout --force\n> +'\n> +\n> test_done\n"},{"id":"462770","messageId":"001a01d8c308$6d9f70f0$48de52d0$@nexbridge.com","threadId":"58288","inReplyTo":"xmqqh71isvc8.fsf@gitster.g","subject":"RE: [BUG] t1800: Fails for error text comparison","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2022-09-07T22:23:11Z","receivedAt":"2022-09-07T22:23:22Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On September 7, 2022 5:56 PM, Junio C Hamano wrote:\n> writes:\n>\n>> I am finding an issue with t1800.16 failing as a result of a text\ncompare:\n>>\n>> -fatal: cannot run bad-hooks/test-hook: ...\n>> +fatal: cannot exec 'bad-hooks/test-hook': Permission denied\n>>\n>> I don't think this is actually a failure condition but the message\n>> text is platform and shell specific.\n>\n>Isn't this coming from piece of code in start_command()?\n>\n>\t\t/*\n>\t\t * Attempt to exec using the command and arguments starting\nat\n>\t\t * argv.argv[1]. argv.argv[0] contains SHELL_PATH which\nwill\n>\t\t * be used in the event exec failed with ENOEXEC at which\npoint\n>\t\t * we will try to interpret the command using 'sh'.\n>\t\t */\n>\t\texecve(argv.v[1], (char *const *) argv.v + 1,\n>\t\t (char *const *) childenv);\n>\t\tif (errno == ENOEXEC)\n>\t\t\texecve(argv.v[0], (char *const *) argv.v,\n>\t\t\t (char *const *) childenv);\n>\n>\t\tif (errno == ENOENT) {\n>\t\t\tif (cmd->silent_exec_failure)\n>\t\t\t\tchild_die(CHILD_ERR_SILENT);\n>\t\t\tchild_die(CHILD_ERR_ENOENT);\n>\t\t} else {\n>\t\t\tchild_die(CHILD_ERR_ERRNO);\n>\t\t}\n>\n>The test apparently expects CHILD_ERR_NOENT, which comes from\n>child_err_spew()\n>\n>\tcase CHILD_ERR_ENOENT:\n>\t\terror_errno(\"cannot run %s\", cmd->args.v[0]);\n>\t\tbreak;\n>\tcase CHILD_ERR_SILENT:\n>\t\tbreak;\n>\tcase CHILD_ERR_ERRNO:\n>\t\terror_errno(\"cannot exec '%s'\", cmd->args.v[0]);\n>\t\tbreak;\n>\t}\n>\n>but somehow your system fails the execve() with something other than\nENOENT.\n\nThe file exists but could not be executed. EPERM was returned. The reason is\nthat test-hooks exists and contains:\n\n#!/bad/path/no/spaces\n\nwhich does not exist. This (correctly IMO) translates to EPERM not ENOENT\nbecause the test-hooks file exists but cannot be executed due to the\nunderlying failure. The underlying ENOENT is hidden.\n\n"},{"id":"462795","messageId":"Yxl37LhGgCC+6J1K@coredump.intra.peff.net","threadId":"58288","inReplyTo":"xmqq8rmususl.fsf@gitster.g","subject":"Re: [PATCH 2/3] upload-pack: skip parse-object re-hashing of \"want\" objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-08T05:04:44Z","receivedAt":"2022-09-08T05:04:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 07, 2022 at 03:07:22PM -0700, Junio C Hamano wrote:\n\n> > Subject: [PATCH] t1060: check partial clone of misnamed blob\n> [...]\n> Thanks. Let's queue it on top.\n\n This fails the leak-check CI job because of existing leaks in\n\"clone --filter\". I think I found the (or perhaps \"a\") bottom of that\nrabbit hole, though:\n\n https://lore.kernel.org/git/Yxl1BNQoy6Drf0Oe@coredump.intra.peff.net/\n\n-Peff\n"},{"id":"462804","messageId":"a87e9eaccf7849fc9eeb10ce7d373db0@xiaomi.com","threadId":"58288","inReplyTo":"xmqqv8pzt20d.fsf@gitster.g","subject":"RE: [External Mail]Re: [PATCH 3/3] parse_object(): check commit-graph when skip_hash set","fromName":"程洋","fromEmail":"chengyang@xiaomi.com","sentAt":"2022-09-08T10:39:08Z","receivedAt":"2022-09-08T10:40:20Z","isPatch":true,"sender":{"key":"chengyang@xiaomi.com","avatar":null},"body":"> If the caller told us that they don't care about us checking the\n> object hash, then we're free to implement any optimizations that get\n> us the parsed value more quickly. An obvious one is to check the\n> commit graph before loading an object from disk. And in fact, both of\n> the callers who pass in this flag are already doing so before they call\n> parse_object()!\n\n> So we can simplify those callers, as well as any possible future ones,\n> by moving the logic into parse_object().\n\nI need to mention that there is serious issue with commit-graph together with partial-clone\nhttps://lore.kernel.org/git/20220709005227.82423-1-hanxin.hx@bytedance.com/\n\nSo actually I told my users to disable commit-graph manually\n#/******本邮件及其附件含有小米公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！ This e-mail and its attachments contain confidential information from XIAOMI, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by phone or email immediately and delete it!******/#\n"},{"id":"462811","messageId":"xmqqv8pxrf7c.fsf@gitster.g","threadId":"58288","inReplyTo":"Yxl37LhGgCC+6J1K@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] upload-pack: skip parse-object re-hashing of \"want\" objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-08T16:41:43Z","receivedAt":"2022-09-08T16:41:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Sep 07, 2022 at 03:07:22PM -0700, Junio C Hamano wrote:\n>\n>> > Subject: [PATCH] t1060: check partial clone of misnamed blob\n>> [...]\n>> Thanks. Let's queue it on top.\n>\n> This fails the leak-check CI job because of existing leaks in\n> \"clone --filter\". I think I found the (or perhaps \"a\") bottom of that\n> rabbit hole, though:\n>\n> https://lore.kernel.org/git/Yxl1BNQoy6Drf0Oe@coredump.intra.peff.net/\n\nThanks.\n\n"},{"id":"462823","messageId":"Yxo3hXg7AwrqzQO8@coredump.intra.peff.net","threadId":"58288","inReplyTo":"a87e9eaccf7849fc9eeb10ce7d373db0@xiaomi.com","subject":"Re: [External Mail]Re: [PATCH 3/3] parse_object(): check commit-graph when skip_hash set","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-08T18:42:13Z","receivedAt":"2022-09-08T18:42:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 08, 2022 at 10:39:08AM +0000, 程洋 wrote:\n\n> > If the caller told us that they don't care about us checking the\n> > object hash, then we're free to implement any optimizations that get\n> > us the parsed value more quickly. An obvious one is to check the\n> > commit graph before loading an object from disk. And in fact, both of\n> > the callers who pass in this flag are already doing so before they call\n> > parse_object()!\n> \n> > So we can simplify those callers, as well as any possible future ones,\n> > by moving the logic into parse_object().\n> \n> I need to mention that there is serious issue with commit-graph together with partial-clone\n> https://lore.kernel.org/git/20220709005227.82423-1-hanxin.hx@bytedance.com/\n\nYes, though I don't think that changes anything with respect to my\npatches. The argument here is simply that if a caller is OK skipping the\nhash check, they are OK with using the commit graph, and vice versa.\nUntil the bug in the linked thread is fixed, it is a good idea to\ndisable the commit graph. But it does not change the intent of the\ncalling code.\n\n-Peff\n"}]}