{"thread":{"id":"51005","subject":"Proposal: object negotiation for partial clones","startedAt":"2019-04-28T15:55:28Z","lastAt":"2019-05-20T18:29:12Z","messageCount":23,"participants":["Matthew DeVore","Jonathan Nieder","Jonathan Tan","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"374613","messageId":"CAMfpvhKYRVwTVNLfRJYcjhHtg=FNLNPbnw8xtY93nJu228v6=g@mail.gmail.com","threadId":"51005","inReplyTo":null,"subject":"Proposal: object negotiation for partial clones","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2019-04-28T15:55:15Z","receivedAt":"2019-04-28T15:55:28Z","isPatch":false,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"Hello,\n\nI'm considering implementing a feature in the Git protocol which would\nenable efficient and accurate object negotiation when the client is a\npartial clone. I'd like to refine and get some validation of my\napproach before I start to write any code, so I've written a proposal\nfor anyone interested to review. Your comments would be appreciated.\n\nRemember this is a publicly-accessible document so be sure to not\ndiscuss any confidential topics in the comments!\n\nTiny URL: http://tinyurl.com/yxz747cy\nFull URL: https://docs.google.com/document/d/1bcDKCgd2Dw5Cl6H9TrNi0ekqzaT8rbyK8EpPE3RcvPA/edit#\n\nThank you,\nMatt\n"},{"id":"374950","messageId":"20190506182504.GA57368@google.com","threadId":"51005","inReplyTo":"CAMfpvhKYRVwTVNLfRJYcjhHtg=FNLNPbnw8xtY93nJu228v6=g@mail.gmail.com","subject":"Re: Proposal: object negotiation for partial clones","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-05-06T18:25:04Z","receivedAt":"2019-05-06T18:25:09Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nMatthew DeVore wrote:\n\n> I'm considering implementing a feature in the Git protocol which would\n> enable efficient and accurate object negotiation when the client is a\n> partial clone. I'd like to refine and get some validation of my\n> approach before I start to write any code, so I've written a proposal\n> for anyone interested to review. Your comments would be appreciated.\n\nYay! Thanks for looking into this, and sorry I didn't respond sooner.\n\nI know the doc has a \"use case\" section, but I suppose I am not sure\nthat I understand the use case yet. Is this about improving the\nfilter syntax to handle features like directory listing? Or is this\nabout being able to make better use of deltas in a partial clone, to\ndecrease bandwidth consumption and overhead that is proportional to\nsize?\n\nThanks,\nJonathan\n"},{"id":"374954","messageId":"20190506192800.213716-1-jonathantanmy@google.com","threadId":"51005","inReplyTo":"CAMfpvhKYRVwTVNLfRJYcjhHtg=FNLNPbnw8xtY93nJu228v6=g@mail.gmail.com","subject":"Re: Proposal: object negotiation for partial clones","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-05-06T19:28:00Z","receivedAt":"2019-05-06T19:28:23Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> I'm considering implementing a feature in the Git protocol which would\n> enable efficient and accurate object negotiation when the client is a\n> partial clone. I'd like to refine and get some validation of my\n> approach before I start to write any code, so I've written a proposal\n> for anyone interested to review. Your comments would be appreciated.\n\nThanks. Let me try to summarize: The issue is that, during a fetch,\nnormally the client can say \"have\" to inform the server that it has a\ncommit and all its referenced objects (barring shallow lines), but we\ncan't do the same if the client is a partial clone (because having a\ncommit doesn't necessarily mean that we have all referenced objects).\nAnd not doing this means that the server sends a lot of unnecessary\nobjects in the sent packfile. The solution is to do the fetch in 2\nparts: one to get the list of objects that would be sent, and after the\nclient filters that, one to get the objects themselves.\n\nIt was unclear to me whether this is meant for (1) fetches directly\ninitiated by the user that fetch commits (e.g. \"git fetch origin\",\nreusing the configured \"core.partialclonefilter\") and/or for (2) lazy\nfetching of missing objects. My assumption is that this is only for (2).\n\nMy main question is: we can get the same list of objects (in the form of\ntree objects) if we fetch with \"blob:none\" filter. Admittedly, we will\nget extra data (file names, etc.) - if the extra bandwidth saving is\nnecessary, this should be called out. (And some of the savings will be\noffset by the fact that we will actually need some of those tree\nobjects.)\n\nAssuming that we do need that bandwidth saving, here's my review of that\ndocument.\n\nThe document describes the 1st request exactly as I envision - a\nspecific parameter sent by the client, and the server responds with a\nlist of object names.\n\nFor the 2nd request, the document describes it as repeating the original\nquery of the 1st request while also giving the full list of objects\nwanted as \"choose-refs\". I'm still not convinced that repeating the\noriginal query is necessary - I would just give the list of objects as\nwants. The rationale given for repeating the original query is:\n\n> The original query is helpful because it means the server only needs\n> to do a single reachability check, rather than many separate ones.\n\nBut this omits the fact that, if doing it the document's way, the server\nneeds to perform an object walk in addition to the \"single reachability\ncheck\", and it is not true that if doing it my way, \"many separate ones\"\nneed to be done because the server can check reachability of all objects\nat once.\n\nAlso, my way means that supporting the 2nd request does not require any\ncode or protocol change - it already works today. Assuming we follow my\napproach, the discussion thus lies in supporting the 1st request.\n\nSome more thoughts:\n\n- Changes in server and client scalability: Currently, the server checks\n reachability of all wants, then enumerates, then sends all objects.\n With this change, the server checks reachability of all wants, then\n enumerates, then sends an object list, then checks reachability of all\n objects in the filtered list, then sends some objects. There is\n additional overhead in the extra reachability check and lists of\n objects being sent twice (once by server and once by client), but\n sending fewer objects means that I/O (server, network, client) and\n disk space usage (client) is reduced.\n\n- Usefulness outside partial clone: If the user ever wants a list of\n objects referenced by an object but without their file names, the user\n could use this, but I can't think of such a scenario.\n"},{"id":"374955","messageId":"20190506194625.GB57368@google.com","threadId":"51005","inReplyTo":"20190506192800.213716-1-jonathantanmy@google.com","subject":"Re: Proposal: object negotiation for partial clones","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-05-06T19:46:25Z","receivedAt":"2019-05-06T19:46:31Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJonathan Tan wrote:\n> Matthew DeVore wrote:\n\n>> I'm considering implementing a feature in the Git protocol which would\n>> enable efficient and accurate object negotiation when the client is a\n>> partial clone. I'd like to refine and get some validation of my\n>> approach before I start to write any code, so I've written a proposal\n>> for anyone interested to review. Your comments would be appreciated.\n>\n> Thanks. Let me try to summarize: The issue is that, during a fetch,\n> normally the client can say \"have\" to inform the server that it has a\n> commit and all its referenced objects (barring shallow lines), but we\n> can't do the same if the client is a partial clone (because having a\n> commit doesn't necessarily mean that we have all referenced objects).\n\nAh, interesting. When this was discussed before, the proposal has been\nthat the client can say \"have\" anyway. They don't have the commit and\nall referenced objects, but they have the commit and a *promise* that\nthey can obtain all referenced objects, which is almost as good.\nThat's what \"git fetch\" currently implements.\n\nBut there's a hitch: when doing the fetch-on-demand for an object\naccess, the client currently does not say \"have\". Sure, even there,\nthey have a *promise* that they can obtain all referenced objects, but\nthis could get out of hand: the first pack may contain a delta against\nan object the client doesn't have, triggering another fetch which\ncontains a delta against another object they don't have, and so on.\nToo many round trips.\n\n> And not doing this means that the server sends a lot of unnecessary\n> objects in the sent packfile. The solution is to do the fetch in 2\n> parts: one to get the list of objects that would be sent, and after the\n> client filters that, one to get the objects themselves.\n\nThis helps with object selection but not with delta base selection.\n\nFor object selection, I think the current approach already works okay,\nat least where tree and blob filters are involved. For commit\nfilters, in the current approach the fetch-on-demand sends way too\nmuch because there's no \"filter=commit:none\" option to pass. Is that\nwhat this proposal aims to address?\n\nFor blob filters, if I ignore the capability advertisements (there's\nan optimization that hasn't yet been implemented to allow\nsingle-round-trip fetches), the current behavior takes the same number\nof round trips as this proposal. Where the current approach has been\nlacking is in delta base selection during fetch-on-demand. Ideas for\nimproving that?\n\nThanks,\nJonathan\n"},{"id":"374962","messageId":"CAMfpvh+jbPcU2waU5n6nToxGEsC29WGOFPLR+ibXbhXL6WBb6w@mail.gmail.com","threadId":"51005","inReplyTo":"20190506192800.213716-1-jonathantanmy@google.com","subject":"Re: Proposal: object negotiation for partial clones","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2019-05-06T22:47:05Z","receivedAt":"2019-05-06T22:47:20Z","isPatch":false,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"On Mon, May 6, 2019 at 12:28 PM Jonathan Tan wrote:\n>\n> > I'm considering implementing a feature in the Git protocol which would\n> > enable efficient and accurate object negotiation when the client is a\n> > partial clone. I'd like to refine and get some validation of my\n> > approach before I start to write any code, so I've written a proposal\n> > for anyone interested to review. Your comments would be appreciated.\n>\n> Thanks. Let me try to summarize: The issue is that, during a fetch,\n> normally the client can say \"have\" to inform the server that it has a\n> commit and all its referenced objects (barring shallow lines), but we\n> can't do the same if the client is a partial clone (because having a\n> commit doesn't necessarily mean that we have all referenced objects).\n> And not doing this means that the server sends a lot of unnecessary\n> objects in the sent packfile. The solution is to do the fetch in 2\n> parts: one to get the list of objects that would be sent, and after the\n> client filters that, one to get the objects themselves.\n>\n> It was unclear to me whether this is meant for (1) fetches directly\n> initiated by the user that fetch commits (e.g. \"git fetch origin\",\n> reusing the configured \"core.partialclonefilter\") and/or for (2) lazy\n> fetching of missing objects. My assumption is that this is only for (2).\nYes, that was my intention. The client doesn't really know anything\nabout the hashes reported, so it can't really make an informed\nselection from the candidate list given by the server after the first\nrequest. I guess if we wanted to just reject *all* objects on the\ninitial clone, this feature would make that possible. But that can\nalso be achieved more embracively with a better filter system.\n\n>\n> My main question is: we can get the same list of objects (in the form of\n> tree objects) if we fetch with \"blob:none\" filter. Admittedly, we will\n> get extra data (file names, etc.) - if the extra bandwidth saving is\n> necessary, this should be called out. (And some of the savings will be\n> offset by the fact that we will actually need some of those tree\n> objects.)\nThat's a very good point. The data the first request gives us is\nbasically the tree objects minus file names and modes. So I think a\nbetter feature to implement would be combining of multiple filters.\nThat way, the client can combine \"tree:\" and\n\"blob:none\" and basically get an \"enumeration\" of available objects.\n\n>\n> Assuming that we do need that bandwidth saving, here's my review of that\n> document.\n>\n> The document describes the 1st request exactly as I envision - a\n> specific parameter sent by the client, and the server responds with a\n> list of object names.\n>\n> For the 2nd request, the document describes it as repeating the original\n> query of the 1st request while also giving the full list of objects\n> wanted as \"choose-refs\". I'm still not convinced that repeating the\n> original query is necessary - I would just give the list of objects as\n> wants. The rationale given for repeating the original query is:\n>\n> > The original query is helpful because it means the server only needs\n> > to do a single reachability check, rather than many separate ones.\n>\n> But this omits the fact that, if doing it the document's way, the server\n> needs to perform an object walk in addition to the \"single reachability\n> check\", and it is not true that if doing it my way, \"many separate ones\"\n> need to be done because the server can check reachability of all objects\n> at once.\nAfter considering more carefully how reachability works (and getting\nyour explanation of it out-of-band), I would assume that my approach\nis no better than marginally faster, and possibly worse, than just\ndoing a plain reachability check of multiple objects using the current\nimplementation. My current priorities preclude this kind of\nbenchmarking+micro-optimization. So I believe what is more important\nto me is to simply enable combining multiple filters.\n\n>\n> Also, my way means that supporting the 2nd request does not require any\n> code or protocol change - it already works today. Assuming we follow my\n> approach, the discussion thus lies in supporting the 1st request.\n>\n> Some more thoughts:\n>\n> - Changes in server and client scalability: Currently, the server checks\n> reachability of all wants, then enumerates, then sends all objects.\n> With this change, the server checks reachability of all wants, then\n> enumerates, then sends an object list, then checks reachability of all\n> objects in the filtered list, then sends some objects. There is\n> additional overhead in the extra reachability check and lists of\n> objects being sent twice (once by server and once by client), but\n> sending fewer objects means that I/O (server, network, client) and\n> disk space usage (client) is reduced.\nAgreed, and this is still true in the new approach we've agreed on.\n\n>\n> - Usefulness outside partial clone: If the user ever wants a list of\n> objects referenced by an object but without their file names, the user\n> could use this, but I can't think of such a scenario.\nNeither can I.\n"},{"id":"374966","messageId":"EA2BD2A8-2641-4B71-8439-66994DE9589F@comcast.net","threadId":"51005","inReplyTo":"20190506194625.GB57368@google.com","subject":"Re: Proposal: object negotiation for partial clones","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-05-06T23:20:19Z","receivedAt":"2019-05-06T23:28:46Z","isPatch":false,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"\n\n> On 2019/05/06, at 12:46, Jonathan Nieder wrote:\n> \n> Hi,\n> \n> Jonathan Tan wrote:\n>> Matthew DeVore wrote:\n> \n>>> I'm considering implementing a feature in the Git protocol which would\n>>> enable efficient and accurate object negotiation when the client is a\n>>> partial clone. I'd like to refine and get some validation of my\n>>> approach before I start to write any code, so I've written a proposal\n>>> for anyone interested to review. Your comments would be appreciated.\n>> \n>> Thanks. Let me try to summarize: The issue is that, during a fetch,\n>> normally the client can say \"have\" to inform the server that it has a\n>> commit and all its referenced objects (barring shallow lines), but we\n>> can't do the same if the client is a partial clone (because having a\n>> commit doesn't necessarily mean that we have all referenced objects).\n> \n> Ah, interesting. When this was discussed before, the proposal has been\n> that the client can say \"have\" anyway. They don't have the commit and\n> all referenced objects, but they have the commit and a *promise* that\n> they can obtain all referenced objects, which is almost as good.\n> That's what \"git fetch\" currently implements.\nDoesn’t that mean the “have” may indicate that the client has the entire repository already, even though it’s only a partial clone? If so, then the client intends to ask for some tree plus trees and blobs 2-3 levels down deeper, how would the server distinguish between those objects the client *really* has and those that were just promised to them? Because the whole purpose of this hypothetical request is to get a bunch of promises fulfilled of which 0-99% are fulfilled already.\n\n> \n> For blob filters, if I ignore the capability advertisements (there's\n> an optimization that hasn't yet been implemented to allow\n> single-round-trip fetches), the current behavior takes the same number\n> of round trips as this proposal. Where the current approach has been\n> lacking is in delta base selection during fetch-on-demand. Ideas for\n> improving that?\n\nMaybe something like this (conceptually based on original proposal) ?\n\n1. Client sends request for an object or objects with an extra flag which means “I can’t really tell you what I already have since it’s a chaotic subset of the object database of the repo”\n\n2. Server responds back with set of objects, represented by deltas if that is how the server has them on disk, along with a list of object-IDs needed in order to resolve the content of all the objects. These object-IDs can go several layers of deltas back, and they go back as far as it takes to get to an object stored in its entirety by the server.\n\n3. Client responds back with another request (this time the extra flag sent from step 1 is not necessary) which has “want”s for every object the server named which the client already has.\n\nVery hand-wavey, but I think you see my idea.\n\n"},{"id":"374970","messageId":"20190507000205.GC57368@google.com","threadId":"51005","inReplyTo":"EA2BD2A8-2641-4B71-8439-66994DE9589F@comcast.net","subject":"Re: Proposal: object negotiation for partial clones","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-05-07T00:02:05Z","receivedAt":"2019-05-07T00:02:10Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Matthew DeVore wrote:\n> On 2019/05/06, at 12:46, Jonathan Nieder wrote:\n\n>> Ah, interesting. When this was discussed before, the proposal has been\n>> that the client can say \"have\" anyway. They don't have the commit and\n>> all referenced objects, but they have the commit and a *promise* that\n>> they can obtain all referenced objects, which is almost as good.\n>> That's what \"git fetch\" currently implements.\n>\n> Doesn’t that mean the “have” may indicate that the client has the\n> entire repository already, even though it’s only a partial clone? If\n> so, then the client intends to ask for some tree plus trees and\n> blobs 2-3 levels down deeper, how would the server distinguish\n> between those objects the client *really* has and those that were\n> just promised to them? Because the whole purpose of this\n> hypothetical request is to get a bunch of promises fulfilled of\n> which 0-99% are fulfilled already.\n\nFor blobs, the answer is simple: the server returns any object\nexplicitly named in a \"want\", even if the client already should have\nit.\n\nFor trees, the current behavior is the same: if you declare that you\n\"have\" everything, then if you \"want\" a tree with filter tree:2, you\nonly get that tree. So here there's already room for improvement.\n\n[...]\n> Maybe something like this (conceptually based on original proposal) ?\n>\n> 1. Client sends request for an object or objects with an extra flag\n> which means “I can’t really tell you what I already have since it’s\n> a chaotic subset of the object database of the repo”\n>\n> 2. Server responds back with set of objects, represented by deltas\n> if that is how the server has them on disk, along with a list of\n> object-IDs needed in order to resolve the content of all the\n> objects. These object-IDs can go several layers of deltas back, and\n> they go back as far as it takes to get to an object stored in its\n> entirety by the server.\n>\n> 3. Client responds back with another request (this time the extra\n> flag sent from step 1 is not necessary) which has “want”s for every\n> object the server named which the client already has.\n>\n> Very hand-wavey, but I think you see my idea.\n\nThe only downside I see is that the list of objects may itself be\nlarge, and the server has to check reachability for each one. But\nmaybe that's fine.\n\nPerhaps after that initial response, instead of sending the list of\nindividual objects the client wants, it could send a list of relevant\nobjects it has (combined with the original set of \"want\"s). That\ncould be a smaller request and it means less work for the server to\ncheck each \"want\" for reachability.\n\nWhat do you think?\n\n[...]\n> That's a very good point. The data the first request gives us is\n> basically the tree objects minus file names and modes. So I think a\n> better feature to implement would be combining of multiple filters.\n> That way, the client can combine \"tree:\" and\n> \"blob:none\" and basically get an \"enumeration\" of available objects.\n\nThis might be simpler.\n\nCombining filters would be useful for other uses, too.\n\nThanks,\nJonathan\n"},{"id":"375048","messageId":"20190507183450.50568-1-jonathantanmy@google.com","threadId":"51005","inReplyTo":"CAMfpvh+jbPcU2waU5n6nToxGEsC29WGOFPLR+ibXbhXL6WBb6w@mail.gmail.com","subject":"Re: Proposal: object negotiation for partial clones","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-05-07T18:34:50Z","receivedAt":"2019-05-07T18:34:56Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > My main question is: we can get the same list of objects (in the form of\n> > tree objects) if we fetch with \"blob:none\" filter. Admittedly, we will\n> > get extra data (file names, etc.) - if the extra bandwidth saving is\n> > necessary, this should be called out. (And some of the savings will be\n> > offset by the fact that we will actually need some of those tree\n> > objects.)\n> That's a very good point. The data the first request gives us is\n> basically the tree objects minus file names and modes. So I think a\n> better feature to implement would be combining of multiple filters.\n> That way, the client can combine \"tree:\" and\n> \"blob:none\" and basically get an \"enumeration\" of available objects.\n\nTo get an enumeration of available objects, don't you need to use only\n\"blob:none\"? Combining filters (once that's implemented) will get all\nobjects only up to a certain depth.\n\nCombining \"tree:\" and \"blob:none\" would allow us to reduce the number\nof trees transmitted, but I would imagine that the savings would be\nsignificant only for very large repositories. Do you have a specific use\ncase in mind that isn't solved by \"blob:none\"?\n"},{"id":"375061","messageId":"A0ADEE11-E3E1-4DE0-81BA-40771C783E4E@comcast.net","threadId":"51005","inReplyTo":"20190507183450.50568-1-jonathantanmy@google.com","subject":"Re: Proposal: object negotiation for partial clones","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-05-07T21:57:32Z","receivedAt":"2019-05-07T21:57:52Z","isPatch":false,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"\n\n> On 2019/05/07, at 11:34, Jonathan Tan wrote:\n> \n> To get an enumeration of available objects, don't you need to use only\n> \"blob:none\"? Combining filters (once that's implemented) will get all\n> objects only up to a certain depth.\n> \n> Combining \"tree:\" and \"blob:none\" would allow us to reduce the number\n> of trees transmitted, but I would imagine that the savings would be\n> significant only for very large repositories. Do you have a specific use\n> case in mind that isn't solved by \"blob:none\"?\n\nI am interested in supporting large repositories. The savings seem to be larger than one may expect. I tried the following command on two huge repos to find out how much it costs to fetch “blob:none” for a single commit:\n\n$ git rev-list --objects --filter=blob:none HEAD: | xargs -n 2 bash -c 'git cat-file -s $1' | awk '{ total += $1; print total }'\n\nNote the “:” after HEAD - this limits it to the current commit.\n\nAnd the results were:\n - Linux: 2 684 054 bytes\n - Chromium: > 16 139 570 bytes (then I got tired of waiting for it to finish)\n\n"},{"id":"375247","messageId":"20190509180022.91700-1-jonathantanmy@google.com","threadId":"51005","inReplyTo":"A0ADEE11-E3E1-4DE0-81BA-40771C783E4E@comcast.net","subject":"Re: Proposal: object negotiation for partial clones","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-05-09T18:00:22Z","receivedAt":"2019-05-09T18:00:32Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > On 2019/05/07, at 11:34, Jonathan Tan wrote:\n> >\n> > To get an enumeration of available objects, don't you need to use only\n> > \"blob:none\"? Combining filters (once that's implemented) will get all\n> > objects only up to a certain depth.\n> >\n> > Combining \"tree:\" and \"blob:none\" would allow us to reduce the number\n> > of trees transmitted, but I would imagine that the savings would be\n> > significant only for very large repositories. Do you have a specific use\n> > case in mind that isn't solved by \"blob:none\"?\n> \n> I am interested in supporting large repositories. The savings seem to be larger than one may expect. I tried the following command on two huge repos to find out how much it costs to fetch “blob:none” for a single commit:\n> \n> $ git rev-list --objects --filter=blob:none HEAD: | xargs -n 2 bash -c 'git cat-file -s $1' | awk '{ total += $1; print total }'\n> \n> Note the “:” after HEAD - this limits it to the current commit.\n> \n> And the results were:\n> - Linux: 2 684 054 bytes\n> - Chromium: > 16 139 570 bytes (then I got tired of waiting for it to finish)\n\nThanks for the numbers. Let me think about it some more, but I'm still\nreluctant to introduce multiple filter support in the protocol and the\nimplementation for the following reasons:\n\n- For large projects like Linux and Chromium, it may be reasonable to\n expect that an infrequent checkout would result in a few-megabyte\n download.\n- (After some in-office discussion) It may be possible to mitigate much\n of that by sending root trees that we have as \"have\" (e.g. by\n consulting the reflog), and that wouldn't need any protocol change.\n- Supporting any combination of filter means that we have more to\n implement and test, especially if we want to support more filters in\n the future. In particular, the different filters (e.g. blob, tree)\n have different code paths now in Git. One way to solve it would be to\n combine everything into one monolith, but I would like to avoid it if\n possible (after having to deal with revision walking a few times...)\n"},{"id":"375441","messageId":"4A3BD0B7-894F-4FB0-A3BA-15675F60046C@comcast.net","threadId":"51005","inReplyTo":"20190509180022.91700-1-jonathantanmy@google.com","subject":"Re: Proposal: object negotiation for partial clones","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-05-14T00:09:00Z","receivedAt":"2019-05-14T00:09:52Z","isPatch":false,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"\n\n> On 2019/05/09, at 11:00, Jonathan Tan wrote:\n> \n> Thanks for the numbers. Let me think about it some more, but I'm still\n> reluctant to introduce multiple filter support in the protocol and the\n> implementation for the following reasons:\n\nCorrection to the original command - I was tweaking it in the middle of running it, and introduced an error that I didn’t notice. Here is one that will work for an entire repo:\n\n$ git rev-list --objects --filter=blob:none HEAD: | awk '{print $1}' | xargs -n 1 git cat-file -s | awk '{ total += $1; print total }'\n\nWhen run to completion, Chromium totaled 17 301 144 bytes.\n\n> \n> - For large projects like Linux and Chromium, it may be reasonable to\n> expect that an infrequent checkout would result in a few-megabyte\n> download.\n\nAnyone developing on Chromium would definitely consider a 17 MB original clone to be an improvement over the status quo, but it is still not ideal.\n\nAnd the 17MB initial download is only incurred once *assuming* the next idea is implemented:\n\n> - (After some in-office discussion) It may be possible to mitigate much\n> of that by sending root trees that we have as \"have\" (e.g. by\n> consulting the reflog), and that wouldn't need any protocol change.\n\nThis would complicate the code - not in Git itself, but in my FUSE-related logic. We would have to explore the reflog and try to find the closest commits in history to the target commit being checked out. This is sounding a bit hacky and round-about, and it assumes that at the FUSE layer we can detect when a checkout is happening cleanly and sufficiently early (rather than when one of the sub-sub-trees is being accessed).\n\n> - Supporting any combination of filter means that we have more to\n> implement and test, especially if we want to support more filters in\n> the future. In particular, the different filters (e.g. blob, tree)\n> have different code paths now in Git. One way to solve it would be to\n> combine everything into one monolith, but I would like to avoid it if\n> possible (after having to deal with revision walking a few times...)\n\nI don’t believe there is any need to introduce monolithic code. The bulk of the filter implementation is in list-objects-filter.c, and I don’t think the file will get much longer with an additional filter that “combines” the existing filter. The new filter is likely simpler than the sparse filter. Once I add the new filter and send out the initial patch set, we can discuss splitting up the file, if it appears to be necessary.\n\nMy idea - if it is not clear already - is to add another OO-like interface to list-objects-filter.c which parallels the 5 that are already there.\n\n"},{"id":"375442","messageId":"20190514001610.GA136746@google.com","threadId":"51005","inReplyTo":"4A3BD0B7-894F-4FB0-A3BA-15675F60046C@comcast.net","subject":"Re: Proposal: object negotiation for partial clones","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-05-14T00:16:10Z","receivedAt":"2019-05-14T00:16:15Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nMatthew DeVore wrote:\n> On 2019/05/09, at 11:00, Jonathan Tan wrote:\n\n>> - Supporting any combination of filter means that we have more to\n>> implement and test, especially if we want to support more filters in\n>> the future. In particular, the different filters (e.g. blob, tree)\n>> have different code paths now in Git. One way to solve it would be to\n>> combine everything into one monolith, but I would like to avoid it if\n>> possible (after having to deal with revision walking a few times...)\n>\n> I don’t believe there is any need to introduce monolithic code. The\n> bulk of the filter implementation is in list-objects-filter.c, and I\n> don’t think the file will get much longer with an additional filter\n> that “combines” the existing filter. The new filter is likely\n> simpler than the sparse filter. Once I add the new filter and send\n> out the initial patch set, we can discuss splitting up the file, if\n> it appears to be necessary.\n>\n> My idea - if it is not clear already - is to add another OO-like\n> interface to list-objects-filter.c which parallels the 5 that are\n> already there.\n\nSounds good to me.\n\nFor what it's worth, my assumption has always been that we would\neventually want the filters to be stackable. So I'm glad you're\nlooking into it.\n\nJonathan's reminder to clean up as you go is a welcome one.\n\nThanks,\nJonathan\n"},{"id":"375733","messageId":"cover.1558030802.git.matvore@google.com","threadId":"51005","inReplyTo":"20190514001610.GA136746@google.com","subject":"[RFC PATCH 0/3] implement composite filters","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2019-05-16T18:56:48Z","receivedAt":"2019-05-16T18:56:58Z","isPatch":true,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"Here is a first stab at composite filters. It does not actually support omits,\nbut the biggest difficulties of the implementation are already addressed. So I\ndecided to send out an early version to give interested people an idea of just\nwhat is needed to implement it, and then give them a chance to change my mind\n(Jonathan T. especially was concerned about code complexity).\n\nThank you,\n\nMatthew DeVore (3):\n list-objects-filter: refactor into a context struct\n list-objects-filter-options: error is localizeable\n list-objects-filter: implement composite filters\n\n Documentation/rev-list-options.txt | 8 +\n contrib/completion/git-completion.bash | 2 +-\n list-objects-filter-options.c | 132 +++++++++++-\n list-objects-filter-options.h | 14 +-\n list-objects-filter.c | 272 ++++++++++++++++---------\n list-objects-filter.h | 31 ++-\n list-objects.c | 45 ++--\n t/t6112-rev-list-filters-objects.sh | 108 +++++++++-\n 8 files changed, 470 insertions(+), 142 deletions(-)\n\n-- \n2.21.0\n\n"},{"id":"375734","messageId":"341bc55d4a3f5438b1523525cf683f96d75e8c3e.1558030802.git.matvore@google.com","threadId":"51005","inReplyTo":"cover.1558030802.git.matvore@google.com","subject":"[RFC PATCH 1/3] list-objects-filter: refactor into a context struct","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2019-05-16T18:56:49Z","receivedAt":"2019-05-16T18:57:21Z","isPatch":true,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"The next patch will create and manage filters in a new way, which means\nthat this bundle of data will have to be managed at a new callsite. Make\nthis bundle of data more manageable by putting it in a struct and\nmaking it part of the list-objects-filter module's API.\n\nSigned-off-by: Matthew DeVore \n---\n list-objects-filter.c | 156 +++++++++++++++---------------------------\n list-objects-filter.h | 31 ++++++---\n list-objects.c | 45 ++++++------\n 3 files changed, 96 insertions(+), 136 deletions(-)\n\ndiff --git a/list-objects-filter.c b/list-objects-filter.c\nindex ee449de3f7..8e8616b9b8 100644\n--- a/list-objects-filter.c\n+++ b/list-objects-filter.c\n@@ -19,82 +19,60 @@\n * FILTER_SHOWN_BUT_REVISIT -- we set this bit on tree objects\n * that have been shown, but should be revisited if they appear\n * in the traversal (until we mark it SEEN). This is a way to\n * let us silently de-dup calls to show() in the caller. This\n * is subtly different from the \"revision.h:SHOWN\" and the\n * \"sha1-name.c:ONELINE_SEEN\" bits. And also different from\n * the non-de-dup usage in pack-bitmap.c\n */\n #define FILTER_SHOWN_BUT_REVISIT (1<<21)\n \n-/*\n- * A filter for list-objects to omit ALL blobs from the traversal.\n- * And to OPTIONALLY collect a list of the omitted OIDs.\n- */\n-struct filter_blobs_none_data {\n-\tstruct oidset *omits;\n-};\n-\n static enum list_objects_filter_result filter_blobs_none(\n \tstruct repository *r,\n \tenum list_objects_filter_situation filter_situation,\n \tstruct object *obj,\n \tconst char *pathname,\n \tconst char *filename,\n-\tvoid *filter_data_)\n+\tstruct filter_context *ctx)\n {\n-\tstruct filter_blobs_none_data *filter_data = filter_data_;\n-\n \tswitch (filter_situation) {\n \tdefault:\n \t\tBUG(\"unknown filter_situation: %d\", filter_situation);\n \n \tcase LOFS_BEGIN_TREE:\n \t\tassert(obj->type == OBJ_TREE);\n \t\t/* always include all tree objects */\n \t\treturn LOFR_MARK_SEEN | LOFR_DO_SHOW;\n \n \tcase LOFS_END_TREE:\n \t\tassert(obj->type == OBJ_TREE);\n \t\treturn LOFR_ZERO;\n \n \tcase LOFS_BLOB:\n \t\tassert(obj->type == OBJ_BLOB);\n \t\tassert((obj->flags & SEEN) == 0);\n \n-\t\tif (filter_data->omits)\n-\t\t\toidset_insert(filter_data->omits, &obj->oid);\n+\t\tif (ctx->omits)\n+\t\t\toidset_insert(ctx->omits, &obj->oid);\n \t\treturn LOFR_MARK_SEEN; /* but not LOFR_DO_SHOW (hard omit) */\n \t}\n }\n \n-static void *filter_blobs_none__init(\n-\tstruct oidset *omitted,\n+static void filter_blobs_none__init(\n \tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn)\n+\tstruct filter_context *ctx)\n {\n-\tstruct filter_blobs_none_data *d = xcalloc(1, sizeof(*d));\n-\td->omits = omitted;\n-\n-\t*filter_fn = filter_blobs_none;\n-\t*filter_free_fn = free;\n-\treturn d;\n+\tctx->filter_fn = filter_blobs_none;\n }\n \n-/*\n- * A filter for list-objects to omit ALL trees and blobs from the traversal.\n- * Can OPTIONALLY collect a list of the omitted OIDs.\n- */\n+/* A filter for list-objects to omit ALL trees and blobs from the traversal. */\n struct filter_trees_depth_data {\n-\tstruct oidset *omits;\n-\n \t/*\n \t * Maps trees to the minimum depth at which they were seen. It is not\n \t * necessary to re-traverse a tree at deeper or equal depths than it has\n \t * already been traversed.\n \t *\n \t * We can't use LOFR_MARK_SEEN for tree objects since this will prevent\n \t * it from being traversed at shallower depths.\n \t */\n \tstruct oidmap seen_at_depth;\n \n@@ -103,41 +81,41 @@ struct filter_trees_depth_data {\n };\n \n struct seen_map_entry {\n \tstruct oidmap_entry base;\n \tsize_t depth;\n };\n \n /* Returns 1 if the oid was in the omits set before it was invoked. */\n static int filter_trees_update_omits(\n \tstruct object *obj,\n-\tstruct filter_trees_depth_data *filter_data,\n+\tstruct filter_context *ctx,\n \tint include_it)\n {\n-\tif (!filter_data->omits)\n+\tif (!ctx->omits)\n \t\treturn 0;\n \n \tif (include_it)\n-\t\treturn oidset_remove(filter_data->omits, &obj->oid);\n+\t\treturn oidset_remove(ctx->omits, &obj->oid);\n \telse\n-\t\treturn oidset_insert(filter_data->omits, &obj->oid);\n+\t\treturn oidset_insert(ctx->omits, &obj->oid);\n }\n \n static enum list_objects_filter_result filter_trees_depth(\n \tstruct repository *r,\n \tenum list_objects_filter_situation filter_situation,\n \tstruct object *obj,\n \tconst char *pathname,\n \tconst char *filename,\n-\tvoid *filter_data_)\n+\tstruct filter_context *ctx)\n {\n-\tstruct filter_trees_depth_data *filter_data = filter_data_;\n+\tstruct filter_trees_depth_data *filter_data = ctx->data;\n \tstruct seen_map_entry *seen_info;\n \tint include_it = filter_data->current_depth <\n \t\tfilter_data->exclude_depth;\n \tint filter_res;\n \tint already_seen;\n \n \t/*\n \t * Note that we do not use _MARK_SEEN in order to allow re-traversal in\n \t * case we encounter a tree or blob again at a shallower depth.\n \t */\n@@ -145,47 +123,47 @@ static enum list_objects_filter_result filter_trees_depth(\n \tswitch (filter_situation) {\n \tdefault:\n \t\tBUG(\"unknown filter_situation: %d\", filter_situation);\n \n \tcase LOFS_END_TREE:\n \t\tassert(obj->type == OBJ_TREE);\n \t\tfilter_data->current_depth--;\n \t\treturn LOFR_ZERO;\n \n \tcase LOFS_BLOB:\n-\t\tfilter_trees_update_omits(obj, filter_data, include_it);\n+\t\tfilter_trees_update_omits(obj, ctx, include_it);\n \t\treturn include_it ? LOFR_MARK_SEEN | LOFR_DO_SHOW : LOFR_ZERO;\n \n \tcase LOFS_BEGIN_TREE:\n \t\tseen_info = oidmap_get(\n \t\t\t&filter_data->seen_at_depth, &obj->oid);\n \t\tif (!seen_info) {\n \t\t\tseen_info = xcalloc(1, sizeof(*seen_info));\n \t\t\toidcpy(&seen_info->base.oid, &obj->oid);\n \t\t\tseen_info->depth = filter_data->current_depth;\n \t\t\toidmap_put(&filter_data->seen_at_depth, seen_info);\n \t\t\talready_seen = 0;\n \t\t} else {\n \t\t\talready_seen =\n \t\t\t\tfilter_data->current_depth >= seen_info->depth;\n \t\t}\n \n \t\tif (already_seen) {\n \t\t\tfilter_res = LOFR_SKIP_TREE;\n \t\t} else {\n \t\t\tint been_omitted = filter_trees_update_omits(\n-\t\t\t\tobj, filter_data, include_it);\n+\t\t\t\tobj, ctx, include_it);\n \t\t\tseen_info->depth = filter_data->current_depth;\n \n \t\t\tif (include_it)\n \t\t\t\tfilter_res = LOFR_DO_SHOW;\n-\t\t\telse if (filter_data->omits && !been_omitted)\n+\t\t\telse if (ctx->omits && !been_omitted)\n \t\t\t\t/*\n \t\t\t\t * Must update omit information of children\n \t\t\t\t * recursively; they have not been omitted yet.\n \t\t\t\t */\n \t\t\t\tfilter_res = LOFR_ZERO;\n \t\t\telse\n \t\t\t\tfilter_res = LOFR_SKIP_TREE;\n \t\t}\n \n \t\tfilter_data->current_depth++;\n@@ -194,55 +172,48 @@ static enum list_objects_filter_result filter_trees_depth(\n }\n \n static void filter_trees_free(void *filter_data) {\n \tstruct filter_trees_depth_data *d = filter_data;\n \tif (!d)\n \t\treturn;\n \toidmap_free(&d->seen_at_depth, 1);\n \tfree(d);\n }\n \n-static void *filter_trees_depth__init(\n-\tstruct oidset *omitted,\n+static void filter_trees_depth__init(\n \tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn)\n+\tstruct filter_context *ctx)\n {\n \tstruct filter_trees_depth_data *d = xcalloc(1, sizeof(*d));\n-\td->omits = omitted;\n \toidmap_init(&d->seen_at_depth, 0);\n \td->exclude_depth = filter_options->tree_exclude_depth;\n \td->current_depth = 0;\n \n-\t*filter_fn = filter_trees_depth;\n-\t*filter_free_fn = filter_trees_free;\n-\treturn d;\n+\tctx->filter_fn = filter_trees_depth;\n+\tctx->free_fn = filter_trees_free;\n+\tctx->data = d;\n }\n \n-/*\n- * A filter for list-objects to omit large blobs.\n- * And to OPTIONALLY collect a list of the omitted OIDs.\n- */\n+/* A filter for list-objects to omit large blobs. */\n struct filter_blobs_limit_data {\n-\tstruct oidset *omits;\n \tunsigned long max_bytes;\n };\n \n static enum list_objects_filter_result filter_blobs_limit(\n \tstruct repository *r,\n \tenum list_objects_filter_situation filter_situation,\n \tstruct object *obj,\n \tconst char *pathname,\n \tconst char *filename,\n-\tvoid *filter_data_)\n+\tstruct filter_context *ctx)\n {\n-\tstruct filter_blobs_limit_data *filter_data = filter_data_;\n+\tstruct filter_blobs_limit_data *filter_data = ctx->data;\n \tunsigned long object_length;\n \tenum object_type t;\n \n \tswitch (filter_situation) {\n \tdefault:\n \t\tBUG(\"unknown filter_situation: %d\", filter_situation);\n \n \tcase LOFS_BEGIN_TREE:\n \t\tassert(obj->type == OBJ_TREE);\n \t\t/* always include all tree objects */\n@@ -263,44 +234,41 @@ static enum list_objects_filter_result filter_blobs_limit(\n \t\t\t * apply the size filter criteria. Be conservative\n \t\t\t * and force show it (and let the caller deal with\n \t\t\t * the ambiguity).\n \t\t\t */\n \t\t\tgoto include_it;\n \t\t}\n \n \t\tif (object_length < filter_data->max_bytes)\n \t\t\tgoto include_it;\n \n-\t\tif (filter_data->omits)\n-\t\t\toidset_insert(filter_data->omits, &obj->oid);\n+\t\tif (ctx->omits)\n+\t\t\toidset_insert(ctx->omits, &obj->oid);\n \t\treturn LOFR_MARK_SEEN; /* but not LOFR_DO_SHOW (hard omit) */\n \t}\n \n include_it:\n-\tif (filter_data->omits)\n-\t\toidset_remove(filter_data->omits, &obj->oid);\n+\tif (ctx->omits)\n+\t\toidset_remove(ctx->omits, &obj->oid);\n \treturn LOFR_MARK_SEEN | LOFR_DO_SHOW;\n }\n \n-static void *filter_blobs_limit__init(\n-\tstruct oidset *omitted,\n+static void filter_blobs_limit__init(\n \tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn)\n+\tstruct filter_context *ctx)\n {\n \tstruct filter_blobs_limit_data *d = xcalloc(1, sizeof(*d));\n-\td->omits = omitted;\n \td->max_bytes = filter_options->blob_limit_value;\n \n-\t*filter_fn = filter_blobs_limit;\n-\t*filter_free_fn = free;\n-\treturn d;\n+\tctx->filter_fn = filter_blobs_limit;\n+\tctx->free_fn = free;\n+\tctx->data = d;\n }\n \n /*\n * A filter driven by a sparse-checkout specification to only\n * include blobs that a sparse checkout would populate.\n *\n * The sparse-checkout spec can be loaded from a blob with the\n * given OID or from a local pathname. We allow an OID because\n * the repo may be bare or we may be doing the filtering on the\n * server.\n@@ -319,36 +287,35 @@ struct frame {\n \t * omitted objects.\n \t *\n \t * 0 if everything (recursively) contained in this directory\n \t * has been explicitly included (SHOWN) in the result and\n \t * the directory may be short-cut later in the traversal.\n \t */\n \tunsigned child_prov_omit : 1;\n };\n \n struct filter_sparse_data {\n-\tstruct oidset *omits;\n \tstruct exclude_list el;\n \n \tsize_t nr, alloc;\n \tstruct frame *array_frame;\n };\n \n static enum list_objects_filter_result filter_sparse(\n \tstruct repository *r,\n \tenum list_objects_filter_situation filter_situation,\n \tstruct object *obj,\n \tconst char *pathname,\n \tconst char *filename,\n-\tvoid *filter_data_)\n+\tstruct filter_context *ctx)\n {\n-\tstruct filter_sparse_data *filter_data = filter_data_;\n+\tstruct filter_sparse_data *filter_data = ctx->data;\n \tint val, dtype;\n \tstruct frame *frame;\n \n \tswitch (filter_situation) {\n \tdefault:\n \t\tBUG(\"unknown filter_situation: %d\", filter_situation);\n \n \tcase LOFS_BEGIN_TREE:\n \t\tassert(obj->type == OBJ_TREE);\n \t\tdtype = DT_DIR;\n@@ -414,128 +381,117 @@ static enum list_objects_filter_result filter_sparse(\n \n \t\tframe = &filter_data->array_frame[filter_data->nr];\n \n \t\tdtype = DT_REG;\n \t\tval = is_excluded_from_list(pathname, strlen(pathname),\n \t\t\t\t\t filename, &dtype, &filter_data->el,\n \t\t\t\t\t r->index);\n \t\tif (val < 0)\n \t\t\tval = frame->defval;\n \t\tif (val > 0) {\n-\t\t\tif (filter_data->omits)\n-\t\t\t\toidset_remove(filter_data->omits, &obj->oid);\n+\t\t\tif (ctx->omits)\n+\t\t\t\toidset_remove(ctx->omits, &obj->oid);\n \t\t\treturn LOFR_MARK_SEEN | LOFR_DO_SHOW;\n \t\t}\n \n \t\t/*\n \t\t * Provisionally omit it. We've already established that\n \t\t * this pathname is not in the sparse-checkout specification\n \t\t * with the CURRENT pathname, so we *WANT* to omit this blob.\n \t\t *\n \t\t * However, a pathname elsewhere in the tree may also\n \t\t * reference this same blob, so we cannot reject it yet.\n \t\t * Leave the LOFR_ bits unset so that if the blob appears\n \t\t * again in the traversal, we will be asked again.\n \t\t */\n-\t\tif (filter_data->omits)\n-\t\t\toidset_insert(filter_data->omits, &obj->oid);\n+\t\tif (ctx->omits)\n+\t\t\toidset_insert(ctx->omits, &obj->oid);\n \n \t\t/*\n \t\t * Remember that at least 1 blob in this tree was\n \t\t * provisionally omitted. This prevents us from short\n \t\t * cutting the tree in future iterations.\n \t\t */\n \t\tframe->child_prov_omit = 1;\n \t\treturn LOFR_ZERO;\n \t}\n }\n \n \n static void filter_sparse_free(void *filter_data)\n {\n \tstruct filter_sparse_data *d = filter_data;\n \t/* TODO free contents of 'd' */\n \tfree(d);\n }\n \n-static void *filter_sparse_oid__init(\n-\tstruct oidset *omitted,\n+static void filter_sparse_oid__init(\n \tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn)\n+\tstruct filter_context *ctx)\n {\n \tstruct filter_sparse_data *d = xcalloc(1, sizeof(*d));\n-\td->omits = omitted;\n \tif (add_excludes_from_blob_to_list(filter_options->sparse_oid_value,\n \t\t\t\t\t NULL, 0, &d->el) < 0)\n \t\tdie(\"could not load filter specification\");\n \n \tALLOC_GROW(d->array_frame, d->nr + 1, d->alloc);\n \td->array_frame[d->nr].defval = 0; /* default to include */\n \td->array_frame[d->nr].child_prov_omit = 0;\n \n-\t*filter_fn = filter_sparse;\n-\t*filter_free_fn = filter_sparse_free;\n-\treturn d;\n+\tctx->filter_fn = filter_sparse;\n+\tctx->free_fn = filter_sparse_free;\n+\tctx->data = d;\n }\n \n-static void *filter_sparse_path__init(\n-\tstruct oidset *omitted,\n+static void filter_sparse_path__init(\n \tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn)\n+\tstruct filter_context *ctx)\n {\n \tstruct filter_sparse_data *d = xcalloc(1, sizeof(*d));\n-\td->omits = omitted;\n \tif (add_excludes_from_file_to_list(filter_options->sparse_path_value,\n \t\t\t\t\t NULL, 0, &d->el, NULL) < 0)\n \t\tdie(\"could not load filter specification\");\n \n \tALLOC_GROW(d->array_frame, d->nr + 1, d->alloc);\n \td->array_frame[d->nr].defval = 0; /* default to include */\n \td->array_frame[d->nr].child_prov_omit = 0;\n \n-\t*filter_fn = filter_sparse;\n-\t*filter_free_fn = filter_sparse_free;\n-\treturn d;\n+\tctx->filter_fn = filter_sparse;\n+\tctx->free_fn = filter_sparse_free;\n+\tctx->data = d;\n }\n \n-typedef void *(*filter_init_fn)(\n-\tstruct oidset *omitted,\n+typedef void (*filter_init_fn)(\n \tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn);\n+\tstruct filter_context *ctx);\n \n /*\n * Must match \"enum list_objects_filter_choice\".\n */\n static filter_init_fn s_filters[] = {\n \tNULL,\n \tfilter_blobs_none__init,\n \tfilter_blobs_limit__init,\n \tfilter_trees_depth__init,\n \tfilter_sparse_oid__init,\n \tfilter_sparse_path__init,\n };\n \n-void *list_objects_filter__init(\n+void list_objects_filter__init(\n \tstruct oidset *omitted,\n \tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn)\n+\tstruct filter_context *ctx)\n {\n \tfilter_init_fn init_fn;\n \n \tassert((sizeof(s_filters) / sizeof(s_filters[0])) == LOFC__COUNT);\n \n \tif (filter_options->choice >= LOFC__COUNT)\n \t\tBUG(\"invalid list-objects filter choice: %d\",\n \t\t filter_options->choice);\n \n+\tmemset(ctx, 0, sizeof(*ctx));\n+\tctx->omits = omitted;\n \tinit_fn = s_filters[filter_options->choice];\n \tif (init_fn)\n-\t\treturn init_fn(omitted, filter_options,\n-\t\t\t filter_fn, filter_free_fn);\n-\t*filter_fn = NULL;\n-\t*filter_free_fn = NULL;\n-\treturn NULL;\n+\t\tinit_fn(filter_options, ctx);\n }\ndiff --git a/list-objects-filter.h b/list-objects-filter.h\nindex 1d45a4ad57..ee807f5d9b 100644\n--- a/list-objects-filter.h\n+++ b/list-objects-filter.h\n@@ -53,37 +53,46 @@ enum list_objects_filter_result {\n \tLOFR_DO_SHOW = 1<<1,\n \tLOFR_SKIP_TREE = 1<<2,\n };\n \n enum list_objects_filter_situation {\n \tLOFS_BEGIN_TREE,\n \tLOFS_END_TREE,\n \tLOFS_BLOB\n };\n \n-typedef enum list_objects_filter_result (*filter_object_fn)(\n-\tstruct repository *r,\n-\tenum list_objects_filter_situation filter_situation,\n-\tstruct object *obj,\n-\tconst char *pathname,\n-\tconst char *filename,\n-\tvoid *filter_data);\n+struct filter_context {\n+\tenum list_objects_filter_result (*filter_fn)(\n+\t\tstruct repository *r,\n+\t\tenum list_objects_filter_situation filter_situation,\n+\t\tstruct object *obj,\n+\t\tconst char *pathname,\n+\t\tconst char *filename,\n+\t\tstruct filter_context *ctx);\n+\tvoid (*free_fn)(void *filter_data);\n \n-typedef void (*filter_free_fn)(void *filter_data);\n+\tstruct oidset *omits;\n+\tvoid *data;\n+};\n \n /*\n * Constructor for the set of defined list-objects filters.\n * Returns a generic \"void *filter_data\".\n *\n * The returned \"filter_fn\" will be used by traverse_commit_list()\n * to filter the results.\n *\n * The returned \"filter_free_fn\" is a destructor for the\n * filter_data.\n */\n-void *list_objects_filter__init(\n+void list_objects_filter__init(\n \tstruct oidset *omitted,\n \tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn);\n+\tstruct filter_context *ctx);\n+\n+static inline void list_objects_filter__release(struct filter_context *ctx) {\n+\tif (ctx->data && ctx->free_fn)\n+\t\tctx->free_fn(ctx->data);\n+\tmemset(ctx, 0, sizeof(*ctx));\n+}\n \n #endif /* LIST_OBJECTS_FILTER_H */\ndiff --git a/list-objects.c b/list-objects.c\nindex b5651ddd5b..7a73f7deee 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -11,22 +11,21 @@\n #include \"list-objects-filter-options.h\"\n #include \"packfile.h\"\n #include \"object-store.h\"\n #include \"trace.h\"\n \n struct traversal_context {\n \tstruct rev_info *revs;\n \tshow_object_fn show_object;\n \tshow_commit_fn show_commit;\n \tvoid *show_data;\n-\tfilter_object_fn filter_fn;\n-\tvoid *filter_data;\n+\tstruct filter_context filter_ctx;\n };\n \n static void process_blob(struct traversal_context *ctx,\n \t\t\t struct blob *blob,\n \t\t\t struct strbuf *path,\n \t\t\t const char *name)\n {\n \tstruct object *obj = &blob->object;\n \tsize_t pathlen;\n \tenum list_objects_filter_result r = LOFR_MARK_SEEN | LOFR_DO_SHOW;\n@@ -47,25 +46,25 @@ static void process_blob(struct traversal_context *ctx,\n \t * may cause the actual filter to report an incomplete list\n \t * of missing objects.\n \t */\n \tif (ctx->revs->exclude_promisor_objects &&\n \t !has_object_file(&obj->oid) &&\n \t is_promisor_object(&obj->oid))\n \t\treturn;\n \n \tpathlen = path->len;\n \tstrbuf_addstr(path, name);\n-\tif ((obj->flags & NOT_USER_GIVEN) && ctx->filter_fn)\n-\t\tr = ctx->filter_fn(ctx->revs->repo,\n-\t\t\t\t LOFS_BLOB, obj,\n-\t\t\t\t path->buf, &path->buf[pathlen],\n-\t\t\t\t ctx->filter_data);\n+\tif ((obj->flags & NOT_USER_GIVEN) && ctx->filter_ctx.filter_fn)\n+\t\tr = ctx->filter_ctx.filter_fn(ctx->revs->repo,\n+\t\t\t\t\t LOFS_BLOB, obj,\n+\t\t\t\t\t path->buf, &path->buf[pathlen],\n+\t\t\t\t\t &ctx->filter_ctx);\n \tif (r & LOFR_MARK_SEEN)\n \t\tobj->flags |= SEEN;\n \tif (r & LOFR_DO_SHOW)\n \t\tctx->show_object(obj, path->buf, ctx->show_data);\n \tstrbuf_setlen(path, pathlen);\n }\n \n /*\n * Processing a gitlink entry currently does nothing, since\n * we do not recurse into the subproject.\n@@ -179,42 +178,42 @@ static void process_tree(struct traversal_context *ctx,\n \t\t */\n \t\tif (revs->exclude_promisor_objects &&\n \t\t is_promisor_object(&obj->oid))\n \t\t\treturn;\n \n \t\tif (!revs->do_not_die_on_missing_tree)\n \t\t\tdie(\"bad tree object %s\", oid_to_hex(&obj->oid));\n \t}\n \n \tstrbuf_addstr(base, name);\n-\tif ((obj->flags & NOT_USER_GIVEN) && ctx->filter_fn)\n-\t\tr = ctx->filter_fn(ctx->revs->repo,\n-\t\t\t\t LOFS_BEGIN_TREE, obj,\n-\t\t\t\t base->buf, &base->buf[baselen],\n-\t\t\t\t ctx->filter_data);\n+\tif ((obj->flags & NOT_USER_GIVEN) && ctx->filter_ctx.filter_fn)\n+\t\tr = ctx->filter_ctx.filter_fn(ctx->revs->repo,\n+\t\t\t\t\t LOFS_BEGIN_TREE, obj,\n+\t\t\t\t\t base->buf, &base->buf[baselen],\n+\t\t\t\t\t &ctx->filter_ctx);\n \tif (r & LOFR_MARK_SEEN)\n \t\tobj->flags |= SEEN;\n \tif (r & LOFR_DO_SHOW)\n \t\tctx->show_object(obj, base->buf, ctx->show_data);\n \tif (base->len)\n \t\tstrbuf_addch(base, '/');\n \n \tif (r & LOFR_SKIP_TREE)\n \t\ttrace_printf(\"Skipping contents of tree %s...\\n\", base->buf);\n \telse if (!failed_parse)\n \t\tprocess_tree_contents(ctx, tree, base);\n \n-\tif ((obj->flags & NOT_USER_GIVEN) && ctx->filter_fn) {\n-\t\tr = ctx->filter_fn(ctx->revs->repo,\n-\t\t\t\t LOFS_END_TREE, obj,\n-\t\t\t\t base->buf, &base->buf[baselen],\n-\t\t\t\t ctx->filter_data);\n+\tif ((obj->flags & NOT_USER_GIVEN) && ctx->filter_ctx.filter_fn) {\n+\t\tr = ctx->filter_ctx.filter_fn(ctx->revs->repo,\n+\t\t\t\t\t LOFS_END_TREE, obj,\n+\t\t\t\t\t base->buf, &base->buf[baselen],\n+\t\t\t\t\t &ctx->filter_ctx);\n \t\tif (r & LOFR_MARK_SEEN)\n \t\t\tobj->flags |= SEEN;\n \t\tif (r & LOFR_DO_SHOW)\n \t\t\tctx->show_object(obj, base->buf, ctx->show_data);\n \t}\n \n \tstrbuf_setlen(base, baselen);\n \tfree_tree_buffer(tree);\n }\n \n@@ -395,38 +394,34 @@ static void do_traverse(struct traversal_context *ctx)\n void traverse_commit_list(struct rev_info *revs,\n \t\t\t show_commit_fn show_commit,\n \t\t\t show_object_fn show_object,\n \t\t\t void *show_data)\n {\n \tstruct traversal_context ctx;\n \tctx.revs = revs;\n \tctx.show_commit = show_commit;\n \tctx.show_object = show_object;\n \tctx.show_data = show_data;\n-\tctx.filter_fn = NULL;\n-\tctx.filter_data = NULL;\n+\tmemset(&ctx.filter_ctx, 0, sizeof(ctx.filter_ctx));\n \tdo_traverse(&ctx);\n }\n \n void traverse_commit_list_filtered(\n \tstruct list_objects_filter_options *filter_options,\n \tstruct rev_info *revs,\n \tshow_commit_fn show_commit,\n \tshow_object_fn show_object,\n \tvoid *show_data,\n \tstruct oidset *omitted)\n {\n \tstruct traversal_context ctx;\n-\tfilter_free_fn filter_free_fn = NULL;\n+\tmemset(&ctx, 0, sizeof(ctx));\n \n \tctx.revs = revs;\n \tctx.show_object = show_object;\n \tctx.show_commit = show_commit;\n \tctx.show_data = show_data;\n-\tctx.filter_fn = NULL;\n \n-\tctx.filter_data = list_objects_filter__init(omitted, filter_options,\n-\t\t\t\t\t\t &ctx.filter_fn, &filter_free_fn);\n+\tlist_objects_filter__init(omitted, filter_options, &ctx.filter_ctx);\n \tdo_traverse(&ctx);\n-\tif (ctx.filter_data && filter_free_fn)\n-\t\tfilter_free_fn(ctx.filter_data);\n+\tlist_objects_filter__release(&ctx.filter_ctx);\n }\n-- \n2.21.0\n\n"},{"id":"375735","messageId":"6f4da02d494323e3ca946b4b20bf78d9dee419e4.1558030802.git.matvore@google.com","threadId":"51005","inReplyTo":"cover.1558030802.git.matvore@google.com","subject":"[RFC PATCH 2/3] list-objects-filter-options: error is localizeable","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2019-05-16T18:56:50Z","receivedAt":"2019-05-16T18:58:00Z","isPatch":true,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"The \"invalid filter-spec\" message is user-facing and not a BUG, so make\nit localizeable.\n\nSigned-off-by: Matthew DeVore \n---\n list-objects-filter-options.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex c0036f7378..e46ea467bc 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -81,21 +81,21 @@ static int gently_parse_list_objects_filter(\n \t\tfilter_options->choice = LOFC_SPARSE_PATH;\n \t\tfilter_options->sparse_path_value = strdup(v0);\n \t\treturn 0;\n \t}\n \t/*\n \t * Please update _git_fetch() in git-completion.bash when you\n \t * add new filters\n \t */\n \n \tif (errbuf)\n-\t\tstrbuf_addf(errbuf, \"invalid filter-spec '%s'\", arg);\n+\t\tstrbuf_addf(errbuf, _(\"invalid filter-spec '%s'\"), arg);\n \n \tmemset(filter_options, 0, sizeof(*filter_options));\n \treturn 1;\n }\n \n int parse_list_objects_filter(struct list_objects_filter_options *filter_options,\n \t\t\t const char *arg)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tif (gently_parse_list_objects_filter(filter_options, arg, &buf))\n-- \n2.21.0\n\n"},{"id":"375736","messageId":"02a8c9b017d8df056d7e90aff907d6e0b5506467.1558030802.git.matvore@google.com","threadId":"51005","inReplyTo":"cover.1558030802.git.matvore@google.com","subject":"[RFC PATCH 3/3] list-objects-filter: implement composite filters","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2019-05-16T18:56:51Z","receivedAt":"2019-05-16T18:58:05Z","isPatch":true,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"Allow combining filters such that only objects accepted by all filters\nare shown. The motivation for this is to allow getting directory\nlistings without also fetching blobs. This can be done by combining\nblob:none with tree:. There are massive repositories that have\nlarger-than-expected trees - even if you include only a single commit.\n\nThe current usage requires passing the filter to rev-list, or sending\nit over the wire, as:\n\n\tcombine:+\n\n(i.e.: git rev-list --filter=combine:tree:2+blob:limit=32k). This is\npotentially awkward because individual filters must be URL-encoded if\nthey contain + or %. This can potentially be improved by supporting a\nrepeated flag syntax, e.g.:\n\n\t$ git rev-list --filter=tree:2 --filter:blob:limit=32k\n\nSuch usage is currently an error, so giving it a meaning is backwards-\ncompatible.\n\nSigned-off-by: Matthew DeVore \n---\n Documentation/rev-list-options.txt | 8 ++\n contrib/completion/git-completion.bash | 2 +-\n list-objects-filter-options.c | 130 +++++++++++++++++++++++++\n list-objects-filter-options.h | 14 ++-\n list-objects-filter.c | 116 ++++++++++++++++++++++\n t/t6112-rev-list-filters-objects.sh | 108 +++++++++++++++++++-\n 6 files changed, 373 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex ddbc1de43f..a2e14d3753 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -730,20 +730,28 @@ specification contained in .\n +\n The form '--filter=tree:' omits all blobs and trees whose depth\n from the root tree is >= (minimum depth if an object is located\n at multiple depths in the commits traversed). =0 will not include\n any trees or blobs unless included explicitly in the command-line (or\n standard input when --stdin is used). =1 will include only the\n tree and blobs which are referenced directly by a commit reachable from\n or an explicitly-given object. =2 is like =1\n while also including trees and blobs one more level removed from an\n explicitly-given commit or tree.\n++\n+The form '--filter=combine:++...' combines\n+several filters. Only objects which are accepted by every filter are\n+included. Filters are joined by \"+\" and individual filters are %-encoded\n+(i.e. URL-encoded). Only the \"%\" and \"+\" characters must be encoded. For\n+instance, 'combine:tree:3+blob:none' and\n+'combine:tree%3A2+blob%3Anone' are equivalent, and result in only tree\n+objects whose depth from the root is >= 3 being included.\n \n --no-filter::\n \tTurn off any previous `--filter=` argument.\n \n --filter-print-omitted::\n \tOnly useful with `--filter=`; prints a list of the objects omitted\n \tby the filter. Object IDs are prefixed with a ``~'' character.\n \n --missing=::\n \tA debug option to help with future \"partial clone\" development.\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 3eefbabdb1..0fd0a10d0c 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1529,21 +1529,21 @@ _git_difftool ()\n __git_fetch_recurse_submodules=\"yes on-demand no\"\n \n _git_fetch ()\n {\n \tcase \"$cur\" in\n \t--recurse-submodules=*)\n \t\t__gitcomp \"$__git_fetch_recurse_submodules\" \"\" \"${cur##--recurse-submodules=}\"\n \t\treturn\n \t\t;;\n \t--filter=*)\n-\t\t__gitcomp \"blob:none blob:limit= sparse:oid= sparse:path=\" \"\" \"${cur##--filter=}\"\n+\t\t__gitcomp \"blob:none blob:limit= sparse:oid= sparse:path= combine: tree:\" \"\" \"${cur##--filter=}\"\n \t\treturn\n \t\t;;\n \t--*)\n \t\t__gitcomp_builtin fetch\n \t\treturn\n \t\t;;\n \tesac\n \t__git_complete_remote_or_refspec\n }\n \ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex e46ea467bc..e9d0de17e0 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -1,19 +1,24 @@\n #include \"cache.h\"\n #include \"commit.h\"\n #include \"config.h\"\n #include \"revision.h\"\n #include \"argv-array.h\"\n #include \"list-objects.h\"\n #include \"list-objects-filter.h\"\n #include \"list-objects-filter-options.h\"\n \n+static int parse_combine_filter(\n+\tstruct list_objects_filter_options *filter_options,\n+\tconst char *arg,\n+\tstruct strbuf *errbuf);\n+\n /*\n * Parse value of the argument to the \"filter\" keyword.\n * On the command line this looks like:\n * --filter=\n * and in the pack protocol as:\n * \"filter\" SP \n *\n * The filter keyword will be used by many commands.\n * See Documentation/rev-list-options.txt for allowed values for .\n *\n@@ -74,33 +79,152 @@ static int gently_parse_list_objects_filter(\n \t\tif (!get_oid_with_context(the_repository, v0, GET_OID_BLOB,\n \t\t\t\t\t &sparse_oid, &oc))\n \t\t\tfilter_options->sparse_oid_value = oiddup(&sparse_oid);\n \t\tfilter_options->choice = LOFC_SPARSE_OID;\n \t\treturn 0;\n \n \t} else if (skip_prefix(arg, \"sparse:path=\", &v0)) {\n \t\tfilter_options->choice = LOFC_SPARSE_PATH;\n \t\tfilter_options->sparse_path_value = strdup(v0);\n \t\treturn 0;\n+\n+\t} else if (skip_prefix(arg, \"combine:\", &v0)) {\n+\t\tint sub_parse_res = parse_combine_filter(\n+\t\t\tfilter_options, v0, errbuf);\n+\t\tif (sub_parse_res)\n+\t\t\treturn sub_parse_res;\n+\t\treturn 0;\n+\n \t}\n \t/*\n \t * Please update _git_fetch() in git-completion.bash when you\n \t * add new filters\n \t */\n \n \tif (errbuf)\n \t\tstrbuf_addf(errbuf, _(\"invalid filter-spec '%s'\"), arg);\n \n \tmemset(filter_options, 0, sizeof(*filter_options));\n \treturn 1;\n }\n \n+static int digit_value(int c, struct strbuf *errbuf) {\n+\tif (c >= '0' && c <= '9')\n+\t\treturn c - '0';\n+\tif (c >= 'a' && c <= 'f')\n+\t\treturn c - 'a' + 10;\n+\tif (c >= 'A' && c <= 'F')\n+\t\treturn c - 'A' + 10;\n+\n+\tif (!errbuf)\n+\t\treturn -1;\n+\n+\tstrbuf_addf(errbuf, _(\"error in filter-spec - \"));\n+\tif (c)\n+\t\tstrbuf_addf(\n+\t\t\terrbuf,\n+\t\t\t_(\"expect two hex digits after %%, but got: '%c'\"),\n+\t\t\tc);\n+\telse\n+\t\tstrbuf_addf(\n+\t\t\terrbuf,\n+\t\t\t_(\"not enough hex digits after %%; expected two\"));\n+\n+\treturn -1;\n+}\n+\n+static int url_decode(struct strbuf *s, struct strbuf *errbuf) {\n+\tchar *dest = s->buf;\n+\tchar *src = s->buf;\n+\twhile (*src) {\n+\t\tint digit_value_0, digit_value_1;\n+\n+\t\tif (src[0] != '%') {\n+\t\t\t*dest++ = *src++;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tsrc++;\n+\n+\t\tdigit_value_0 = digit_value(*src++, errbuf);\n+\t\tif (digit_value_0 < 0)\n+\t\t\treturn 1;\n+\t\tdigit_value_1 = digit_value(*src++, errbuf);\n+\t\tif (digit_value_1 < 0)\n+\t\t\treturn 1;\n+\t\t*dest++ = digit_value_0 * 16 + digit_value_1;\n+\t}\n+\tsize_t new_len = dest - s->buf;\n+\tstrbuf_remove(s, new_len, s->len - new_len);\n+\n+\treturn 0;\n+}\n+\n+static int parse_combine_filter(\n+\tstruct list_objects_filter_options *filter_options,\n+\tconst char *arg,\n+\tstruct strbuf *errbuf)\n+{\n+\tstruct strbuf **sub_specs = strbuf_split_str(arg, '+', 2);\n+\tint result;\n+\n+\tif (!sub_specs[0]) {\n+\t\tif (errbuf)\n+\t\t\tstrbuf_addf(errbuf,\n+\t\t\t\t _(\"expected something after combine:\"));\n+\t\tresult = 1;\n+\t\tgoto cleanup;\n+\t}\n+\n+\t/*\n+\t * Only decode the first sub-filter, since the rest will be decoded on\n+\t * the recursive call.\n+\t */\n+\tresult = url_decode(sub_specs[0], errbuf);\n+\tif (result)\n+\t\tgoto cleanup;\n+\n+\tif (!sub_specs[1]) {\n+\t\t/*\n+\t\t * There is only one sub-filter, so we don't need the\n+\t\t * combine: - just parse it as a non-composite filter.\n+\t\t */\n+\t\tresult = gently_parse_list_objects_filter(\n+\t\t\tfilter_options, sub_specs[0]->buf, errbuf);\n+\t\tgoto cleanup;\n+\t}\n+\n+\t/* Remove trailing \"+\" so we can parse it. */\n+\tassert(sub_specs[0]->buf[sub_specs[0]->len - 1] == '+');\n+\tstrbuf_remove(sub_specs[0], sub_specs[0]->len - 1, 1);\n+\n+\tfilter_options->choice = LOFC_COMBINE;\n+\tfilter_options->lhs = xcalloc(1, sizeof(*filter_options->lhs));\n+\tresult = gently_parse_list_objects_filter(filter_options->lhs,\n+\t\t\t\t\t\t sub_specs[0]->buf,\n+\t\t\t\t\t\t errbuf);\n+\tif (result)\n+\t\tgoto cleanup;\n+\n+\tfilter_options->rhs = xcalloc(1, sizeof(*filter_options->rhs));\n+\tresult = parse_combine_filter(filter_options->rhs,\n+\t\t\t\t sub_specs[1]->buf,\n+\t\t\t\t errbuf);\n+\n+cleanup:\n+\tstrbuf_list_free(sub_specs);\n+\tif (result) {\n+\t\tlist_objects_filter_release(filter_options);\n+\t\tmemset(filter_options, 0, sizeof(*filter_options));\n+\t}\n+\treturn result;\n+}\n+\n int parse_list_objects_filter(struct list_objects_filter_options *filter_options,\n \t\t\t const char *arg)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tif (gently_parse_list_objects_filter(filter_options, arg, &buf))\n \t\tdie(\"%s\", buf.buf);\n \treturn 0;\n }\n \n int opt_parse_list_objects_filter(const struct option *opt,\n@@ -127,23 +251,29 @@ void expand_list_objects_filter_spec(\n \telse if (filter->choice == LOFC_TREE_DEPTH)\n \t\tstrbuf_addf(expanded_spec, \"tree:%lu\",\n \t\t\t filter->tree_exclude_depth);\n \telse\n \t\tstrbuf_addstr(expanded_spec, filter->filter_spec);\n }\n \n void list_objects_filter_release(\n \tstruct list_objects_filter_options *filter_options)\n {\n+\tif (!filter_options)\n+\t\treturn;\n \tfree(filter_options->filter_spec);\n \tfree(filter_options->sparse_oid_value);\n \tfree(filter_options->sparse_path_value);\n+\tlist_objects_filter_release(filter_options->lhs);\n+\tfree(filter_options->lhs);\n+\tlist_objects_filter_release(filter_options->rhs);\n+\tfree(filter_options->rhs);\n \tmemset(filter_options, 0, sizeof(*filter_options));\n }\n \n void partial_clone_register(\n \tconst char *remote,\n \tconst struct list_objects_filter_options *filter_options)\n {\n \t/*\n \t * Record the name of the partial clone remote in the\n \t * config and in the global variable -- the latter is\ndiff --git a/list-objects-filter-options.h b/list-objects-filter-options.h\nindex e3adc78ebf..6c0f0ecd08 100644\n--- a/list-objects-filter-options.h\n+++ b/list-objects-filter-options.h\n@@ -7,20 +7,21 @@\n /*\n * The list of defined filters for list-objects.\n */\n enum list_objects_filter_choice {\n \tLOFC_DISABLED = 0,\n \tLOFC_BLOB_NONE,\n \tLOFC_BLOB_LIMIT,\n \tLOFC_TREE_DEPTH,\n \tLOFC_SPARSE_OID,\n \tLOFC_SPARSE_PATH,\n+\tLOFC_COMBINE,\n \tLOFC__COUNT /* must be last */\n };\n \n struct list_objects_filter_options {\n \t/*\n \t * 'filter_spec' is the raw argument value given on the command line\n \t * or protocol request. (The part after the \"--keyword=\".) For\n \t * commands that launch filtering sub-processes, or for communication\n \t * over the network, don't use this value; use the result of\n \t * expand_list_objects_filter_spec() instead.\n@@ -32,28 +33,35 @@ struct list_objects_filter_options {\n \t * the filtering algorithm to use.\n \t */\n \tenum list_objects_filter_choice choice;\n \n \t/*\n \t * Choice is LOFC_DISABLED because \"--no-filter\" was requested.\n \t */\n \tunsigned int no_filter : 1;\n \n \t/*\n-\t * Parsed values (fields) from within the filter-spec. These are\n-\t * choice-specific; not all values will be defined for any given\n-\t * choice.\n+\t * BEGIN choice-specific parsed values from within the filter-spec. Only\n+\t * some values will be defined for any given choice.\n \t */\n+\n \tstruct object_id *sparse_oid_value;\n \tchar *sparse_path_value;\n \tunsigned long blob_limit_value;\n \tunsigned long tree_exclude_depth;\n+\n+\t/* LOFC_COMBINE values */\n+\tstruct list_objects_filter_options *lhs, *rhs;\n+\n+\t/*\n+\t * END choice-specific parsed values.\n+\t */\n };\n \n /* Normalized command line arguments */\n #define CL_ARG__FILTER \"filter\"\n \n int parse_list_objects_filter(\n \tstruct list_objects_filter_options *filter_options,\n \tconst char *arg);\n \n int opt_parse_list_objects_filter(const struct option *opt,\ndiff --git a/list-objects-filter.c b/list-objects-filter.c\nindex 8e8616b9b8..28496f31f7 100644\n--- a/list-objects-filter.c\n+++ b/list-objects-filter.c\n@@ -453,34 +453,150 @@ static void filter_sparse_path__init(\n \n \tALLOC_GROW(d->array_frame, d->nr + 1, d->alloc);\n \td->array_frame[d->nr].defval = 0; /* default to include */\n \td->array_frame[d->nr].child_prov_omit = 0;\n \n \tctx->filter_fn = filter_sparse;\n \tctx->free_fn = filter_sparse_free;\n \tctx->data = d;\n }\n \n+struct filter_combine_data {\n+\t/* sub[0] corresponds to lhs, sub[1] to rhs. */\n+\tstruct {\n+\t\tstruct filter_context ctx;\n+\t\tstruct oidset seen;\n+\t\tstruct object_id skip_tree;\n+\t\tunsigned is_skipping_tree : 1;\n+\t} sub[2];\n+};\n+\n+static void filter_combine_free(void *filter_data)\n+{\n+\tstruct filter_combine_data *d = filter_data;\n+\tint i;\n+\tfor (i = 0; i < 2; i++) {\n+\t\tlist_objects_filter__release(&d->sub[i].ctx);\n+\t\toidset_clear(&d->sub[i].seen);\n+\t}\n+\tfree(d);\n+}\n+\n+static int should_delegate(enum list_objects_filter_situation filter_situation,\n+\t\t\t struct object *obj,\n+\t\t\t struct filter_combine_data *d,\n+\t\t\t int side)\n+{\n+\tif (!d->sub[side].is_skipping_tree)\n+\t\treturn 1;\n+\tif (filter_situation == LOFS_END_TREE &&\n+\t\toideq(&obj->oid, &d->sub[side].skip_tree)) {\n+\t\td->sub[side].is_skipping_tree = 0;\n+\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n+static enum list_objects_filter_result filter_combine(\n+\tstruct repository *r,\n+\tenum list_objects_filter_situation filter_situation,\n+\tstruct object *obj,\n+\tconst char *pathname,\n+\tconst char *filename,\n+\tstruct filter_context *ctx)\n+{\n+\tstruct filter_combine_data *d = ctx->data;\n+\tenum list_objects_filter_result lhs_result = LOFR_ZERO;\n+\tenum list_objects_filter_result rhs_result = LOFR_ZERO;\n+\tint lhs_already_seen = oidset_contains(&d->sub[0].seen, &obj->oid);\n+\tint rhs_already_seen = oidset_contains(&d->sub[1].seen, &obj->oid);\n+\tint delegate_lhs = !lhs_already_seen &&\n+\t\tshould_delegate(filter_situation, obj, d, 0);\n+\tint delegate_rhs = !rhs_already_seen &&\n+\t\tshould_delegate(filter_situation, obj, d, 1);\n+\tenum list_objects_filter_result combined_result = LOFR_ZERO;\n+\n+\tif (lhs_already_seen && rhs_already_seen)\n+\t\treturn LOFR_ZERO;\n+\n+\tif (delegate_lhs)\n+\t\tlhs_result = d->sub[0].ctx.filter_fn(\n+\t\t\tr, filter_situation, obj, pathname, filename,\n+\t\t\t&d->sub[0].ctx);\n+\tif (delegate_rhs)\n+\t\trhs_result = d->sub[1].ctx.filter_fn(\n+\t\t\tr, filter_situation, obj, pathname, filename,\n+\t\t\t&d->sub[1].ctx);\n+\n+\tif (lhs_result & LOFR_MARK_SEEN)\n+\t\toidset_insert(&d->sub[0].seen, &obj->oid);\n+\n+\tif (rhs_result & LOFR_MARK_SEEN)\n+\t\toidset_insert(&d->sub[1].seen, &obj->oid);\n+\n+\tif (lhs_result & LOFR_SKIP_TREE) {\n+\t\td->sub[0].is_skipping_tree = 1;\n+\t\td->sub[0].skip_tree = obj->oid;\n+\t}\n+\tif (rhs_result & LOFR_SKIP_TREE) {\n+\t\td->sub[1].is_skipping_tree = 1;\n+\t\td->sub[1].skip_tree = obj->oid;\n+\t}\n+\n+\tif ((lhs_result & LOFR_DO_SHOW) && (rhs_result & LOFR_DO_SHOW))\n+\t\tcombined_result |= LOFR_DO_SHOW;\n+\tif (d->sub[0].is_skipping_tree && d->sub[1].is_skipping_tree)\n+\t\tcombined_result |= LOFR_SKIP_TREE;\n+\n+\treturn combined_result;\n+}\n+\n+static void filter_combine__init(\n+\tstruct list_objects_filter_options *filter_options,\n+\tstruct filter_context *ctx)\n+{\n+\tstruct filter_combine_data *d = xcalloc(1, sizeof(*d));\n+\tstruct oidset *lhs_omits = NULL;\n+\tstruct oidset *rhs_omits = NULL;\n+\n+\tif (ctx->omits) {\n+\t\tlhs_omits = xcalloc(1, sizeof(*lhs_omits));\n+\t\toidset_init(lhs_omits, 16);\n+\t\trhs_omits = xcalloc(1, sizeof(*rhs_omits));\n+\t\toidset_init(rhs_omits, 16);\n+\t}\n+\n+\tlist_objects_filter__init(lhs_omits, filter_options->lhs,\n+\t\t\t\t &d->sub[0].ctx);\n+\tlist_objects_filter__init(rhs_omits, filter_options->rhs,\n+\t\t\t\t &d->sub[1].ctx);\n+\n+\tctx->filter_fn = filter_combine;\n+\tctx->free_fn = filter_combine_free;\n+\tctx->data = d;\n+}\n+\n typedef void (*filter_init_fn)(\n \tstruct list_objects_filter_options *filter_options,\n \tstruct filter_context *ctx);\n \n /*\n * Must match \"enum list_objects_filter_choice\".\n */\n static filter_init_fn s_filters[] = {\n \tNULL,\n \tfilter_blobs_none__init,\n \tfilter_blobs_limit__init,\n \tfilter_trees_depth__init,\n \tfilter_sparse_oid__init,\n \tfilter_sparse_path__init,\n+\tfilter_combine__init,\n };\n \n void list_objects_filter__init(\n \tstruct oidset *omitted,\n \tstruct list_objects_filter_options *filter_options,\n \tstruct filter_context *ctx)\n {\n \tfilter_init_fn init_fn;\n \n \tassert((sizeof(s_filters) / sizeof(s_filters[0])) == LOFC__COUNT);\ndiff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh\nindex 9c11427719..63c1524f6f 100755\n--- a/t/t6112-rev-list-filters-objects.sh\n+++ b/t/t6112-rev-list-filters-objects.sh\n@@ -284,21 +284,33 @@ test_expect_success 'verify tree:0 includes trees in \"filtered\" output' '\n # Make sure tree:0 does not iterate through any trees.\n \n test_expect_success 'verify skipping tree iteration when not collecting omits' '\n \tGIT_TRACE=1 git -C r3 rev-list \\\n \t\t--objects --filter=tree:0 HEAD 2>filter_trace &&\n \tgrep \"Skipping contents of tree [.][.][.]\" filter_trace >actual &&\n \t# One line for each commit traversed.\n \ttest_line_count = 2 actual &&\n \n \t# Make sure no other trees were considered besides the root.\n-\t! grep \"Skipping contents of tree [^.]\" filter_trace\n+\t! grep \"Skipping contents of tree [^.]\" filter_trace &&\n+\n+\t# Try this again with \"combine:\". If both sub-filters are skipping\n+\t# trees, the composite filter should also skip trees. This is not\n+\t# important unless the user does combine:tree:X+tree:Y or another filter\n+\t# besides \"tree:\" is implemented in the future which can skip trees.\n+\tGIT_TRACE=1 git -C r3 rev-list \\\n+\t\t--objects --filter=combine:tree:1+tree:3 HEAD 2>filter_trace &&\n+\n+\t# Only skip the dir1/ tree, which is shared between the two commits.\n+\tgrep \"Skipping contents of tree \" filter_trace >actual &&\n+\ttest_write_lines \"Skipping contents of tree dir1/...\" >expected &&\n+\ttest_cmp expected actual\n '\n \n # Test tree:# filters.\n \n expect_has () {\n \tcommit=$1 &&\n \tname=$2 &&\n \n \thash=$(git -C r3 rev-parse $commit:$name) &&\n \tgrep \"^$hash $name$\" actual\n@@ -336,20 +348,114 @@ test_expect_success 'verify tree:3 includes everything expected' '\n \texpect_has HEAD dir1/sparse1 &&\n \texpect_has HEAD dir1/sparse2 &&\n \texpect_has HEAD pattern &&\n \texpect_has HEAD sparse1 &&\n \texpect_has HEAD sparse2 &&\n \n \t# There are also 2 commit objects\n \ttest_line_count = 10 actual\n '\n \n+test_expect_success 'combine:... for a simple combination' '\n+\tgit -C r3 rev-list --objects --filter=combine:tree:2+blob:none HEAD \\\n+\t\t>actual &&\n+\n+\texpect_has HEAD \"\" &&\n+\texpect_has HEAD~1 \"\" &&\n+\texpect_has HEAD dir1 &&\n+\n+\t# There are also 2 commit objects\n+\ttest_line_count = 5 actual\n+'\n+\n+test_expect_success 'combine:... with URL encoding' '\n+\tgit -C r3 rev-list --objects \\\n+\t\t--filter=combine:tree%3a2+blob:%6Eon%65 HEAD >actual &&\n+\n+\texpect_has HEAD \"\" &&\n+\texpect_has HEAD~1 \"\" &&\n+\texpect_has HEAD dir1 &&\n+\n+\t# There are also 2 commit objects\n+\ttest_line_count = 5 actual\n+'\n+\n+expect_invalid_filter_spec () {\n+\tspec=\"$1\" &&\n+\terr=\"$2\" &&\n+\n+\ttest_must_fail git -C r3 rev-list --objects --filter=\"$spec\" HEAD \\\n+\t\t>actual 2>actual_stderr &&\n+\ttest_must_be_empty actual &&\n+\ttest_i18ngrep \"$err\" actual_stderr && cat actual_stderr >/dev/stderr\n+}\n+\n+test_expect_success 'combine:... while URL-encoding things that should not be' '\n+\texpect_invalid_filter_spec combine%3Atree:2+blob:none \\\n+\t\t\"invalid filter-spec\"\n+'\n+\n+test_expect_success 'combine:... with invalid URL-encoded sequences' '\n+\texpect_invalid_filter_spec combine:tree:2+blob:non%a \\\n+\t\t\"error in filter-spec - not enough hex digits after %\" &&\n+\t# Edge cases for non-hex chars: \"Gg/:@`\"\n+\texpect_invalid_filter_spec combine:tree:2+blob%G5none \\\n+\t\t\"error in filter-spec - expect two hex digits .*: .G.\" &&\n+\texpect_invalid_filter_spec combine:tree:2+blob%5/none \\\n+\t\t\"error in filter-spec - expect two hex digits .*: ./.\" &&\n+\texpect_invalid_filter_spec combine:%:5tree:2+blob:none \\\n+\t\t\"error in filter-spec - expect two hex digits .*: .:.\" &&\n+\texpect_invalid_filter_spec combine:tree:2+blob:none%f@ \\\n+\t\t\"error in filter-spec - expect two hex digits .*: .@.\" &&\n+\texpect_invalid_filter_spec combine:tree:2+blob:none%f\\` \\\n+\t\t\"error in filter-spec - expect two hex digits .*: .\\`.\"\n+'\n+\n+test_expect_success 'combine:... with edge-case hex digits: Ff Aa 0 9' '\n+\tgit -C r3 rev-list --objects --filter=\"combine:tree:2+bl%6Fb:n%6fne\" \\\n+\t\tHEAD >actual &&\n+\ttest_line_count = 5 actual &&\n+\tgit -C r3 rev-list --objects --filter=\"combine:tree%3A2+blob%3anone\" \\\n+\t\tHEAD >actual &&\n+\ttest_line_count = 5 actual &&\n+\tgit -C r3 rev-list --objects --filter=\"combine:tree:%30\" HEAD >actual &&\n+\ttest_line_count = 2 actual &&\n+\tgit -C r3 rev-list --objects --filter=\"combine:tree:%39+blob:none\" \\\n+\t\tHEAD >actual &&\n+\ttest_line_count = 5 actual\n+'\n+\n+test_expect_success 'combine:... with more than two sub-filters' '\n+\tgit -C r3 rev-list --objects \\\n+\t\t--filter=combine:tree:3+blob:limit=40+sparse:path=../pattern1 \\\n+\t\tHEAD >actual &&\n+\n+\texpect_has HEAD \"\" &&\n+\texpect_has HEAD~1 \"\" &&\n+\texpect_has HEAD dir1 &&\n+\texpect_has HEAD dir1/sparse1 &&\n+\texpect_has HEAD dir1/sparse2 &&\n+\n+\t# Should also have 2 commits\n+\ttest_line_count = 7 actual &&\n+\n+\t# Try again, this time making sure the last sub-filter is only\n+\t# URL-decoded once.\n+\tcp pattern1 pattern1+renamed% &&\n+\tcp actual expect &&\n+\n+\tgit -C r3 rev-list --objects \\\n+\t\t--filter=combine:tree:3+blob:limit=40+sparse:path=../pattern1%2brenamed%25 \\\n+\t\tHEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n # Test provisional omit collection logic with a repo that has objects appearing\n # at multiple depths - first deeper than the filter's threshold, then shallow.\n \n test_expect_success 'setup r4' '\n \tgit init r4 &&\n \n \techo foo > r4/foo &&\n \tmkdir r4/subdir &&\n \techo bar > r4/subdir/bar &&\n \n-- \n2.21.0\n\n"},{"id":"375756","messageId":"20190516224150.243395-1-jonathantanmy@google.com","threadId":"51005","inReplyTo":"cover.1558030802.git.matvore@google.com","subject":"Re: [RFC PATCH 0/3] implement composite filters","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-05-16T22:41:50Z","receivedAt":"2019-05-16T22:41:57Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Here is a first stab at composite filters. It does not actually support omits,\n> but the biggest difficulties of the implementation are already addressed. So I\n> decided to send out an early version to give interested people an idea of just\n> what is needed to implement it, and then give them a chance to change my mind\n> (Jonathan T. especially was concerned about code complexity).\n\nThanks - seeing these patches reduces my concerns significantly. A\ncomposite filter has LHS and RHS filters, either of which can be\ncomposite themselves, so we support compositing arbitrary numbers of\nfilters. I see that the approach is to run LHS and RHS in lockstep, with\nour own handling of the result flags:\n\n- LOFR_MARK_SEEN is tracked for LHS and RHS separately. To support an\n arbitrary number of filters, we don't use object flags to track this,\n so we use oidsets instead. I don't think that the extra memory usage\n will be a problem (we already allocate more for all the struct\n object). If this is an issue in the future, we can switch to using\n object flags for the first N filters, and oidsets thereafter.\n\n- LOFR_SKIP_TREE is simulated if only one filter wants to skip the tree.\n\n- I haven't fully figured out LOFR_DO_SHOW yet. It seems to me that if\n an object appears twice in the walk, and the LHS says LOFR_DO_SHOW on\n the first occurrence, if the RHS says LOFR_DO_SHOW on the second\n occurrence, the object will be shown twice. But perhaps this isn't a\n problem - as it is, I think that a filter can call LOFR_DO_SHOW\n multiple times on the same object anyway. In any case, if this turns\n out to be a problem, it seems surmountable to me.\n"},{"id":"375768","messageId":"740617F3-9C9F-4F1D-86F7-1A129C7F3F3D@comcast.net","threadId":"51005","inReplyTo":"20190516224150.243395-1-jonathantanmy@google.com","subject":"Re: [RFC PATCH 0/3] implement composite filters","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-05-17T00:01:48Z","receivedAt":"2019-05-17T00:10:13Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"\n\n> On 2019/05/16, at 15:41, Jonathan Tan wrote:\n> \n> Thanks - seeing these patches reduces my concerns significantly. A\n\nThank you for taking a look :)\n\n> \n> - LOFR_MARK_SEEN is tracked for LHS and RHS separately. To support an\n> arbitrary number of filters, we don't use object flags to track this,\n> so we use oidsets instead. I don't think that the extra memory usage\n> will be a problem (we already allocate more for all the struct\n> object). If this is an issue in the future, we can switch to using\n> object flags for the first N filters, and oidsets thereafter.\n\nYup. Another possibility that comes to mind is that when both the lhs and rhs seen sets contain the same object, we promote it to a combined set, and remove it from the individual ones at that time.\n\n> \n> - LOFR_SKIP_TREE is simulated if only one filter wants to skip the tree.\n> \n> - I haven't fully figured out LOFR_DO_SHOW yet. It seems to me that if\n> an object appears twice in the walk, and the LHS says LOFR_DO_SHOW on\n> the first occurrence, if the RHS says LOFR_DO_SHOW on the second\n> occurrence, the object will be shown twice. But perhaps this isn't a\n\nLOFR_DO_SHOW is only propagated upward from the combine: filter if both children indicate LOFR_DO_SHOW for the same object at the same point in the traversal (see the line that has \"combined_result |= LOFR_DO_SHOW\"). In the scenario you draw out, the object won’t be shown at all, since the first occurrence is filtered out for one reason, and the second is filtered out for a separate reason. This may happen when a sparse: filter includes a \"deep\" blob but excludes the same blob at a shallower point in the tree, and a tree: filter does the opposite with the same blob. Just thinking about it now for a moment, this seems intuitive and reasonable.\n\n"},{"id":"375782","messageId":"xmqqwoip3gp0.fsf@gitster-ct.c.googlers.com","threadId":"51005","inReplyTo":"02a8c9b017d8df056d7e90aff907d6e0b5506467.1558030802.git.matvore@google.com","subject":"Re: [RFC PATCH 3/3] list-objects-filter: implement composite filters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-17T03:25:47Z","receivedAt":"2019-05-17T03:25:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthew DeVore writes:\n\n> Allow combining filters such that only objects accepted by all filters\n> are shown. The motivation for this is to allow getting directory\n> listings without also fetching blobs. This can be done by combining\n> blob:none with tree:. There are massive repositories that have\n> larger-than-expected trees - even if you include only a single commit.\n>\n> The current usage requires passing the filter to rev-list, or sending\n> it over the wire, as:\n>\n> \tcombine:+\n>\n> (i.e.: git rev-list --filter=combine:tree:2+blob:limit=32k). This is\n> potentially awkward because individual filters must be URL-encoded if\n> they contain + or %. This can potentially be improved by supporting a\n> repeated flag syntax, e.g.:\n>\n> \t$ git rev-list --filter=tree:2 --filter:blob:limit=32k\n\nShouldn't the second one say \"--filter=blob:limit=32k\" (i.e. the\nfirst colon should be an equal sign)?\n\n> Such usage is currently an error, so giving it a meaning is backwards-\n> compatible.\n\nTwo minor comments. \n\nIf combine means \"must satisfy all of these\", '+' is probably a poor\nchoice (perhaps we want '&' instead). Also, it seems to me that\nhaving to worry about url encoding and parsing encoded data\ncorrectly and securely would be far more work than simply taking\nmultiple command line parameters, accumulating them in a string\nlist, and then at the end of command line parsing, building a\ncombined filter out of all of them at once (a degenerate case may\nend up attempting to build a combined filter that combines a single\nfilter), iow just biting the bullet and do the \"potentially be\nimproved\" step from the beginning.\n\n> +The form '--filter=combine:++...' combines\n> +several filters. Only objects which are accepted by every filter are\n> +included. Filters are joined by \"+\" and individual filters are %-encoded\n> +(i.e. URL-encoded). Only the \"%\" and \"+\" characters must be encoded. For\n> +instance, 'combine:tree:3+blob:none' and\n> +'combine:tree%3A2+blob%3Anone' are equivalent, and result in only tree\n\nI do not quite get this part. The way I read the justification for\nencoding given in the proposed commit log message was because a\nfilter name or filter parameter might want to include the '+' sign,\nso a filter whose name is frotz+nitfol that takes a param may be\ninvoked standalone as \"--filter=frotz+nitfol:param=1\" needs to\nprotect its '+' from getting mistaken when used in the combine\nfilter, it should not be mistaken as a combination of 'frotz' filter\n(with no parameter) and 'nitfol' filter (with param set to one), and\nthe way to do so is to say\n\n\t--filter=\"combine:frotz%2Bnitfol:param=1\"\n\n(this is a degenerate 'combination of a single filter', but you can\nadd another one by following it with '+' and another URLencoded\nstring).\n\nSo why are we allowing %3A there that does not even have to be\nencoded? Shouldn't it be an error?\n\nIn any case, I am not quite convinced that we need to complicate the\nparameters with URLencoding, so I'd skip reviewing large part this\npatch that is about \"decoding\".\n\nOnce the combined filter definition is built in-core, the code that\nevaluates the intersection of all conditions seems to be written\nsanely to me.\n\nThanks.\n"},{"id":"375803","messageId":"1E174CAA-BD57-400B-A83B-4AABFAFBC04B@comcast.net","threadId":"51005","inReplyTo":"xmqqwoip3gp0.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH 3/3] list-objects-filter: implement composite filters","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-05-17T13:17:43Z","receivedAt":"2019-05-17T13:17:48Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"\n\n> On May 16, 2019, at 8:25 PM, Junio C Hamano wrote:\n>> \n>> \t$ git rev-list --filter=tree:2 --filter:blob:limit=32k\n> \n> Shouldn't the second one say \"--filter=blob:limit=32k\" (i.e. the\n> first colon should be an equal sign)?\n\nThat's right. Fixed locally.\n\n> \n>> Such usage is currently an error, so giving it a meaning is backwards-\n>> compatible.\n> \n> Two minor comments. \n> \n> If combine means \"must satisfy all of these\", '+' is probably a poor\n> choice (perhaps we want '&' instead). Also, it seems to me that\n\nI think I agree. & is more intuitive.\n\n> having to worry about url encoding and parsing encoded data\n> correctly and securely would be far more work than simply taking\n> multiple command line parameters, accumulating them in a string\n> list, and then at the end of command line parsing, building a\n> combined filter out of all of them at once (a degenerate case may\n> end up attempting to build a combined filter that combines a single\n> filter), iow just biting the bullet and do the \"potentially be\n> improved\" step from the beginning.\n\nMy intention actually is to support the repeated flag pretty soon, but I only want to write the code if there's agreement on my current approach.\n\nMy justification for the URL-encoding scheme is:\n\n1. The combined filters will eventually have to travel over the wire.\n\n2. The Git protocol will either have repeated \"filter\" lines or it will continue to use a single filter line with an encoding scheme.\n\n3. Continuing to use a single filter line seemed the least disruptive considering both this codebase and Git clones like JGit. Other clones will likely fail saying \"unknown filter combine:\" or something like that until it gets implemented. A paranoid consideration is that clones and proprietary server implementations may currently allow the \"filter\" line to be silently overridden if it is repeated.\n\n4. Assuming we *do* use a single filter line over the wire, it makes sense to allow the user to specify the raw filter line as well as have the more friendly UI of repeating --filter flags.\n\n5. If we use repeated \"filter\" lines over the wire, and later start implementing a more complete DSL for specifying filters (see Mercurial's \"revsets\") the repeated-filter-line feature in the protocol may end up becoming deprecated and we will end up back-pedaling to allow integration of the \"&\" operator with whatever new operators we need.\n\n(I very much doubt I will be the one implementing such a DSL for filters or resets, but I think it's a possibility)\n\n> So why are we allowing %3A there that does not even have to be\n> encoded? Shouldn't it be an error?\n\nWe do have to require the combine operator (& or +) and % be encoded. For other operators, there are three options:\n\n1. Allow anything to be encoded. I chose this because it's how I usually think of URL encoding working. For instance, if I go to https://public-inbox.org/git/?q=cod%65+coverage in Chrome, the browser automatically decodes the %65 to an e in the address bar. Safari does not automatically decode, but the server apparently interprets the %65 as an e. I am not really attached to this choice.\n\n2. Do not allow or require anything else to be encoded.\n\n3. Require encoding of a couple of \"reserved\" characters that don't appear in filters now, and don't typically appear in UNIX path names. This would allow for expansion later. For instance, \"~&%*+|(){}!\\\" plus the ASCII range [0, 0x20] and single and double quotes - do not allow encoding of anything else.\n\n4. Same requirements as 3, but permit encoding of other arbitrary characters.\n\nI kind of like 3 now that I've thought it out more.\n\n> \n> In any case, I am not quite convinced that we need to complicate the\n> parameters with URLencoding, so I'd skip reviewing large part this\n> patch that is about \"decoding\".\n\nIt's fine if we drop the encoding scheme. I intentionally tried to limit the amount of work I stacked on top of it until I got agreement. Please let me know if anything I've said changes your perspective.\n\n> \n> Once the combined filter definition is built in-core, the code that\n> evaluates the intersection of all conditions seems to be written\n> sanely to me.\n\nGreat! I actually did simplify it a bit since I sent the first roll-up.\n\nThanks.\n\n"},{"id":"375865","messageId":"xmqq5zq72qof.fsf@gitster-ct.c.googlers.com","threadId":"51005","inReplyTo":"1E174CAA-BD57-400B-A83B-4AABFAFBC04B@comcast.net","subject":"Re: [RFC PATCH 3/3] list-objects-filter: implement composite filters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-19T01:12:16Z","receivedAt":"2019-05-19T01:12:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthew DeVore writes:\n\n> My justification for the URL-encoding scheme is:\n> ...\n> 3. Continuing to use a single filter line seemed the least\n> disruptive considering both this codebase and Git clones like\n> JGit. Other clones will likely fail saying \"unknown filter\n> combine:\" or something like that until it gets implemented. A\n> paranoid consideration is that clones and proprietary server\n> implementations may currently allow the \"filter\" line to be\n> silently overridden if it is repeated.\n>\n> 4. Assuming we *do* use a single filter line over the wire, it\n> makes sense to allow the user to specify the raw filter line as\n> well as have the more friendly UI of repeating --filter flags.\n>\n> 5. If we use repeated \"filter\" lines over the wire, and later\n> start implementing a more complete DSL for specifying filters (see\n> Mercurial's \"revsets\") the repeated-filter-line feature in the\n> protocol may end up becoming deprecated and we will end up\n> back-pedaling to allow integration of the \"&\" operator with\n> whatever new operators we need.\n\nOK, that's fair.\n\n> 1. Allow anything to be encoded. I chose this because it's how I\n> usually think of URL encoding working. For instance, if I go to\n> https://public-inbox.org/git/?q=cod%65+coverage in Chrome, the\n> browser automatically decodes the %65 to an e in the address\n> bar. Safari does not automatically decode, but the server\n> apparently interprets the %65 as an e. I am not really attached to\n> this choice.\n\nOK, so the rule is \"when you see 'combine:' (this part is never\nencoded), take the rest as a single string, separate it at '+' (these\npluses are never encoded), and URLdecode each part---each of these\nparts is a filter\", which totally makes sense. I somehow didn't see\nthat clearly written in your description.\n\n"},{"id":"375946","messageId":"B379F2FB-77F2-4FAC-B39A-BB3CFE685681@comcast.net","threadId":"51005","inReplyTo":"1E174CAA-BD57-400B-A83B-4AABFAFBC04B@comcast.net","subject":"Re: [RFC PATCH 3/3] list-objects-filter: implement composite filters","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-05-20T18:24:47Z","receivedAt":"2019-05-20T18:25:16Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"\n\n> On 2019/05/17, at 6:17, Matthew DeVore wrote:\n> \n> \n> \n>> On May 16, 2019, at 8:25 PM, Junio C Hamano wrote:\n>>> \n>>> \t$ git rev-list --filter=tree:2 --filter:blob:limit=32k\n>> \n>> Shouldn't the second one say \"--filter=blob:limit=32k\" (i.e. the\n>> first colon should be an equal sign)?\n> \n> That's right. Fixed locally.\n> \n>> \n>>> Such usage is currently an error, so giving it a meaning is backwards-\n>>> compatible.\n>> \n>> Two minor comments. \n>> \n>> If combine means \"must satisfy all of these\", '+' is probably a poor\n>> choice (perhaps we want '&' instead). Also, it seems to me that\n> \n> I think I agree. & is more intuitive.\n\nAfter I tried this in code, I noticed two problems with & which make\nme prefer + again:\n\na. the \"&\" char must be quoted or escaped in the shell, even if it is\n hugged by alphanumeric characters on either side:\n\n\t$ echo a&b\n\t[1] 17083\n\ta\n\t-bash: b: command not found\n\t[1]+ Done echo a\n\t$\n\nb. visually speaking, \"&\" doesn't stand out very well unless it's\n surrounded by whitespace, and currently it must *not* be surrounded\n by whitespace:\n\n\t--filter=combine:blob:none&tree:3&sparse:../foo\n\n\tvs.\n\n\t--filter=combine:blob:none+tree:3+sparse:../foo\n\n> \n>> having to worry about url encoding and parsing encoded data\n>> correctly and securely would be far more work than simply taking\n>> multiple command line parameters, accumulating them in a string\n>> list, and then at the end of command line parsing, building a\n>> combined filter out of all of them at once (a degenerate case may\n>> end up attempting to build a combined filter that combines a single\n>> filter), iow just biting the bullet and do the \"potentially be\n>> improved\" step from the beginning.\n> \n> My intention actually is to support the repeated flag pretty soon, but I only want to write the code if there's agreement on my current approach.\n> \n> My justification for the URL-encoding scheme is:\n> \n> 1. The combined filters will eventually have to travel over the wire.\n> \n> 2. The Git protocol will either have repeated \"filter\" lines or it will continue to use a single filter line with an encoding scheme.\n> \n> 3. Continuing to use a single filter line seemed the least disruptive considering both this codebase and Git clones like JGit. Other clones will likely fail saying \"unknown filter combine:\" or something like that until it gets implemented. A paranoid consideration is that clones and proprietary server implementations may currently allow the \"filter\" line to be silently overridden if it is repeated.\n> \n> 4. Assuming we *do* use a single filter line over the wire, it makes sense to allow the user to specify the raw filter line as well as have the more friendly UI of repeating --filter flags.\n> \n> 5. If we use repeated \"filter\" lines over the wire, and later start implementing a more complete DSL for specifying filters (see Mercurial's \"revsets\") the repeated-filter-line feature in the protocol may end up becoming deprecated and we will end up back-pedaling to allow integration of the \"&\" operator with whatever new operators we need.\n> \n> (I very much doubt I will be the one implementing such a DSL for filters or resets, but I think it's a possibility)\n> \n>> So why are we allowing %3A there that does not even have to be\n>> encoded? Shouldn't it be an error?\n> \n> We do have to require the combine operator (& or +) and % be encoded. For other operators, there are three options:\n> \n> 1. Allow anything to be encoded. I chose this because it's how I usually think of URL encoding working. For instance, if I go to https://public-inbox.org/git/?q=cod%65+coverage in Chrome, the browser automatically decodes the %65 to an e in the address bar. Safari does not automatically decode, but the server apparently interprets the %65 as an e. I am not really attached to this choice.\n> \n> 2. Do not allow or require anything else to be encoded.\n> \n> 3. Require encoding of a couple of \"reserved\" characters that don't appear in filters now, and don't typically appear in UNIX path names. This would allow for expansion later. For instance, \"~&%*+|(){}!\\\" plus the ASCII range [0, 0x20] and single and double quotes - do not allow encoding of anything else.\n> \n> 4. Same requirements as 3, but permit encoding of other arbitrary characters.\n> \n> I kind of like 3 now that I've thought it out more.\n> \n>> \n>> In any case, I am not quite convinced that we need to complicate the\n>> parameters with URLencoding, so I'd skip reviewing large part this\n>> patch that is about \"decoding\".\n> \n> It's fine if we drop the encoding scheme. I intentionally tried to limit the amount of work I stacked on top of it until I got agreement. Please let me know if anything I've said changes your perspective.\n> \n>> \n>> Once the combined filter definition is built in-core, the code that\n>> evaluates the intersection of all conditions seems to be written\n>> sanely to me.\n> \n> Great! I actually did simplify it a bit since I sent the first roll-up.\n> \n> Thanks.\n> \n\n"},{"id":"375947","messageId":"CDE68B7A-C621-4CA8-8039-C432ACBC4FE4@comcast.net","threadId":"51005","inReplyTo":"1E174CAA-BD57-400B-A83B-4AABFAFBC04B@comcast.net","subject":"Re: [RFC PATCH 3/3] list-objects-filter: implement composite filters","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-05-20T18:28:41Z","receivedAt":"2019-05-20T18:29:12Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"\n> On 2019/05/17, at 6:17, Matthew DeVore wrote:\n> \n> We do have to require the combine operator (& or +) and % be encoded. For other operators, there are three options:\n> \n> ...\n> 3. Require encoding of a couple of \"reserved\" characters that don't appear in filters now, and don't typically appear in UNIX path names. This would allow for expansion later. For instance, \"~&%*+|(){}!\\\" plus the ASCII range [0, 0x20] and single and double quotes - do not allow encoding of anything else.\n> \n> 4. Same requirements as 3, but permit encoding of other arbitrary characters.\n\nI tried implementing the reserved characters idea, and considering that\nit was a small amount of code, I think it's worth it:\n\n+static const char *RESERVED_NON_WS = \"\\\"\\\\'[]{}!@#$^~*+\";\n+\n+static int has_reserved_character(\n+ struct strbuf *sub_spec, struct strbuf *errbuf)\n+{\n+ const char *c = sub_spec->buf;\n+ while (*c) {\n+ if (*c <= ' ' || *strchr(RESERVED_NON_WS, *c))\n+ goto found_reserved;\n+ c++;\n+ }\n+\n+ return 0;\n+\n+found_reserved:\n+ if (errbuf)\n+ strbuf_addf(errbuf, \"must escape char in sub-spec: %c\", *c);\n+ return 1;\n+}\n+\n\n"}]}