{"thread":{"id":"47006","subject":"[RFC] protocol version 2","startedAt":"2017-10-20T17:19:29Z","lastAt":"2017-12-13T16:30:51Z","messageCount":47,"participants":["Brandon Williams","Junio C Hamano","Derrick Stolee","Philip Oakley","Jonathan Tan","Johannes Schindelin","Stefan Beller"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"330734","messageId":"20171020171839.4188-1-bmwill@google.com","threadId":"47006","inReplyTo":null,"subject":"[RFC] protocol version 2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-10-20T17:18:39Z","receivedAt":"2017-10-20T17:19:29Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":" Objective\n===========\n\nReplace Git's current wire protocol with a simpler, less wasteful\nprotocol that can evolve over time.\n\n Background\n============\n\nGit's wire protocol is the language used to clone/fetch/push from/to a\nremote git repository. A detailed explanation of the current protocol\nspec can be found\n[here](https://git.kernel.org/pub/scm/git/git.git/tree/Documentation/technical/pack-protocol.txt).\nSome of the pain points with the current protocol spec are:\n\n * The server's initial response is the ref advertisement. This\n advertisement cannot be omitted and can become an issue due to the\n sheer number of refs that can be sent with large repositories. For\n example, when contacting the internal equivalent of\n `https://android.googlesource.com/`, the server will send\n approximately 1 million refs totaling 71MB. This is data that is\n sent during each and every fetch and is not scalable.\n\n * Capabilities were implemented as a hack and are hidden behind a NUL\n byte after the first ref sent from the server during the ref\n advertisement:\n\n\t \\0 \n\n Since they are sent in the context of a pkt-line they are also subject\n to the same length limitations (1k bytes with old clients). While we\n may not be close to hitting this limitation with capabilities alone, it\n has become a problem when trying to abuse capabilities for other\n purposes (e.g. [symrefs](https://public-inbox.org/git/20160816161838.klvjhhoxsftvkfmd@x/)).\n\n * Various other technical debt (e.g. abusing capabilities to\n communicate agent and symref data, service name set using a query\n parameter).\n\n Overview\n==========\n\nThis document presents a specification for a version 2 of Git's wire\nprotocol. Protocol v2 will improve upon v1 in the following ways:\n\n * Instead of multiple service names, multiple commands will be\n supported by a single service\n * Easily extendable as capabilities are moved into their own section\n of the protocol, no longer being hidden behind a NUL byte and\n limited by the size of a pkt-line (as there will be a single\n capability per pkt-line).\n * Separate out other information hidden behind NUL bytes (e.g. agent\n string as a capability and symrefs can be requested using 'ls-ref')\n * Ref advertisement will be omitted unless explicitly requested\n * ls-ref command to explicitly request some refs\n\n Detailed Design\n=================\n\nA client can request to speak protocol v2 by sending `version=2` in the\nside-channel `GIT_PROTOCOL` in the initial request to the server.\n\nIn protocol v2 communication is command oriented. When first contacting\na server a list of capabilities will advertised. Some of these\ncapabilities will be commands which a client can request be executed.\nOnce a command has completed, a client can reuse the connection and\nrequest that other commands be executed\n\n Special Packets\n-----------------\n\nIn protocol v2 these special packets will have the following semantics:\n\n * '0000' Flush Packet (flush-pkt) - indicates the end of a message\n * '0001' End-of-List delimiter (delim-pkt) - indicates the end of a list\n\n Capability Advertisement\n--------------------------\n\nA server which decides to communicate (based on a request from a client)\nusing protocol version 2, notifies the client by sending a version\nstring in its initial response followed by an advertisement of its\ncapabilities. Each capability is a key with an optional value. Clients\nmust ignore all unknown keys. Semantics of unknown values are left to\nthe definition of each key. Some capabilities will describe commands\nwhich can be requested to be executed by the client.\n\n capability-advertisement = protocol-version\n capability-list\n flush-pkt\n\n protocol-version = PKT-LINE(\"version 2\" LF)\n capability-list = *capability\n capability = PKT-LINE(key[=value] LF)\n\n key = 1*CHAR\n value = 1*CHAR\n CHAR = 1*(ALPHA / DIGIT / \"-\" / \"_\")\n\nA client then responds to select the command it wants with any\nparticular capabilities or arguments. There is then an optional section\nwhere the client can provide any command specific parameters or queries.\n\n command-request = command\n capability-list\n delim-pkt\n (command specific parameters)\n flush-pkt\n command = PKT-LINE(\"command=\" key LF)\n\nThe server will then acknowledge the command and requested capabilities\nby echoing them back to the client and then launch into the command.\n\n acknowledge-request = command\n capability-list\n delim-pkt\n execute-command\n execute-command = \n\n\nA particular command can last for as many rounds as are required to\ncomplete the service (multiple for negotiation during fetch and push or\nno additional trips in the case of ls-refs).\n\n\n Commands in v2\n~~~~~~~~~~~~~~~~\n\nServices are the core actions that a client wants to perform (fetch,\npush, etc). Each service has its own set of capabilities and its own\nlanguage of commands (think 'want' lines in fetch). Optionally a\nservice can take in initial parameters or data when a client sends it\nservice request.\n\n Ls-refs\n---------\n\nLs-refs can be looked at as the equivalent of the current ls-remote as\nit is a way to query a remote for the references that it has. Unlike\nthe current ls-remote, the filtering of the output is done on the server\nside by passing a number of parameters to the server-side command\ninstead of the filtering occurring on the client.\n\nLs-ref takes in the following parameters:\n\n --head, --tags: Limit to only refs/heads or refs/tags\n --refs: Do not show peeled tags or pseudorefs like HEAD\n --symref: In addition to the object pointed by it, show the underlying\n ref pointed by it when showing a symbolic ref\n : When specified, only references matching the given patterns\n are displayed.\n\nThe output of ls-refs is as follows:\n\n output = (no-refs / list-of-refs)\n\t *symref\n *shallow\n flush-pkt\n\n no-refs = PKT-LINE(zero-id SP no-refs LF)\n list-of-refs = *ref\n ref = PKT-LINE((tip / peeled) LF)\n tip = obj-id SP refname\n peeled = obj-id SP refname \"^{}\"\n\n symref = PKT-LINE(\"symref\" SP symbolic-ref SP resolved-ref LF)\n shallow = PKT-LINE(\"shallow\" SP obj-id LF)\n\n Fetch\n-------\n\nFetch will need to be a modified version of the v1 fetch protocol. Some\npotential areas for improvement are: Ref-in-want, CDN offloading,\nFetch-options.\n\nSince we'll have an 'ls-ref' service we can eliminate the need of fetch\nto perform a ref-advertisement, instead a client can run the 'ls-refs'\nservice first, in order to find out what refs the server has, and then\nrequest those refs directly using the fetch service.\n\n//TODO Flush out the design\n\n Fetch-object\n--------------\n\nThis service could be used by partial clones in order to request missing\nobjects.\n\n//TODO Flush out the design\n\n Push\n------\n\nPush will need to be a modified version of the v1 push protocol. Some\npotential areas for improvement are: Fix push-options, Negotiation for\nforce push.\n\nOne change that will need to happen is to improve how `push-options` are\nsent to the server (so that they aren't sent twice!!). Also the\nreport-status needs to be better than it currently is in v1 so that\ntools like gerrit can explain what it did with the ref-update the client\nsent to it. Maybe have a push-rebase capability or command?\n\n//TODO Flush out the design\n\n Other Considerations\n======================\n\n * Move away from pkt-line framing?\n * Have responses structured in well known formats (e.g. JSON)\n * Eliminate initial round-trip using 'GIT_PROTOCOL' side-channel\n * Additional commands in a partial clone world (e.g. log, grep)\n"},{"id":"330897","messageId":"xmqqbmkxkpn9.fsf@gitster.mtv.corp.google.com","threadId":"47006","inReplyTo":"20171020171839.4188-1-bmwill@google.com","subject":"Re: [RFC] protocol version 2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-24T06:48:10Z","receivedAt":"2017-10-24T06:48:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> * Capabilities were implemented as a hack and are hidden behind a NUL\n> byte after the first ref sent from the server during the ref\n> advertisement:\n> ...\n>\n> * Various other technical debt (e.g. abusing capabilities to\n> communicate agent and symref data, service name set using a query\n> parameter).\n\nThis sounds like a duplicate of the above.\n\n> Special Packets\n> -----------------\n>\n> In protocol v2 these special packets will have the following semantics:\n>\n> * '0000' Flush Packet (flush-pkt) - indicates the end of a message\n> * '0001' End-of-List delimiter (delim-pkt) - indicates the end of a list\n\nAfter reading the remainder of the document twice, I do not think\nthe reason why we want to introduce a new type delim-pkt is\nexplained and justified well enough. If a command request takes a\ncommand packet, zero or more capability packets, a mandatory\ndelimiter packet, zero or more parameter packets and a mandatory\nflush packet, then you can use the same \"flush\" as delimiter in the\nmiddle. The delimiter will of course become useful if you can omit\nit when not necessary (e.g. after seeing capabilities, you may see a\nflush and you will know there is no parameters and save the need to\nsend one \"delim\").\n\nI actually have a reasonable guess why you want to have a separate\ndelimiter (which has nothing to do with \"optional delim can be\nomitted\"), but I want to see it explained in this document clearly\nby its designer(s).\n\n> command-request = command\n> capability-list\n> delim-pkt\n> (command specific parameters)\n> flush-pkt\n> command = PKT-LINE(\"command=\" key LF)\n>\n> The server will then acknowledge the command and requested capabilities\n> by echoing them back to the client and then launch into the command.\n>\n> acknowledge-request = command\n> capability-list\n> delim-pkt\n> execute-command\n> execute-command = \n\nIt is not quite clear what the value of \"echoing them back\" is,\nespecially if that is done by always echoing verbatim. A reader may\nnaturally expect, when capabilities are exchanged between two\nparties, these are negotiated so that the only ones that are\ncommonly supported by both ends would be used, or something like\nthat.\n\n> A particular command can last for as many rounds as are required to\n> complete the service (multiple for negotiation during fetch and push or\n> no additional trips in the case of ls-refs).\n\nOK.\n\n> Commands in v2\n> ~~~~~~~~~~~~~~~~\n>\n> Services are the core actions that a client wants to perform (fetch,\n> push, etc). Each service has its own set of capabilities and its own\n> language of commands (think 'want' lines in fetch). Optionally a\n> service can take in initial parameters or data when a client sends it\n> service request.\n\nSo a service (like \"fetch\") employ a set of \"command\"s (like \"want\",\n\"have, etc)? In the earlier part of the document, we did not see\nany mention of \"service\" and instead saw only \"command\" mentioned.\nIs the state machine on both ends and the transition between states\nimplicit? E.g. when one side throws \"want\" command and the other\nside acknowledges, they understand implicitly that they are now in a\n\"fetch\" service session, even though there is nothing that said over\nthe wire that they are doing so? One reason I am wondering about\nthis is what we should do if a command verb may be applicable in\nmultiple services.\n\nAfter reading the earlier protocol exchange explanation, I was sort\nof expecting that \"fetch\" would be the command and \"want\", \"have\",\nand \"ack\" lines would be exchanged as \"command specific parameters\",\nso a sudden introduction of \"services\" here was a bit of impedance\nmismatch to me.\n\n> Ls-refs\n> ---------\n>\n> Ls-refs can be looked at as the equivalent of the current ls-remote as\n> it is a way to query a remote for the references that it has. Unlike\n> the current ls-remote, the filtering of the output is done on the server\n> side by passing a number of parameters to the server-side command\n> instead of the filtering occurring on the client.\n>\n> Ls-ref takes in the following parameters:\n>\n> --head, --tags: Limit to only refs/heads or refs/tags\n\nI see no need for the above two as long as \"refs/heads/*\", etc. are\nunderstood.\n\n> --refs: Do not show peeled tags or pseudorefs like HEAD\n\nSo showing peeled tags is the default? Then I can sort-of see why\n\"I am not interested in peeled result\". \n\nI do not see why \"do not show HEAD, MERGE_HEAD, etc.\" is needed,\nthough. It should be sufficient to just ask for refs/* if you are\ninterested only in other things, no?\n\n> --symref: In addition to the object pointed by it, show the underlying\n> ref pointed by it when showing a symbolic ref\n\nSort of OK--it probably is easier to always send this, as symbolic\nrefs are minority anyway, though.\n\n> : When specified, only references matching the given patterns\n> are displayed.\n\nI do not think you meant here.\n\nThe side that is listing what it has has no reason to know what the\nrecipient plans to do with the result, so you must be only sending\nthe LHS of a refspec. If your explanation says \"given patterns\",\nthen replace with . Do not abuse a term that has\nspecific and established meaning for something else.\n\n> The output of ls-refs is as follows:\n>\n> output = (no-refs / list-of-refs)\n> \t *symref\n> *shallow\n> flush-pkt\n>\n> no-refs = PKT-LINE(zero-id SP no-refs LF)\n\nCan't your list-of-refs have 0 element? I do not see why you need\nno-refs here. It's not like you need a dummy line, to the end of\nwhich you need to append NUL plus hidden capabilities ;-)\n\n> list-of-refs = *ref\n> ref = PKT-LINE((tip / peeled) LF)\n> tip = obj-id SP refname\n> peeled = obj-id SP refname \"^{}\"\n>\n> symref = PKT-LINE(\"symref\" SP symbolic-ref SP resolved-ref LF)\n> shallow = PKT-LINE(\"shallow\" SP obj-id LF)\n\nThanks.\n"},{"id":"330932","messageId":"20171024183557.GB79163@google.com","threadId":"47006","inReplyTo":"xmqqbmkxkpn9.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC] protocol version 2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-10-24T18:35:57Z","receivedAt":"2017-10-24T18:36:07Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 10/24, Junio C Hamano wrote:\n> Brandon Williams writes:\n> \n> > * Capabilities were implemented as a hack and are hidden behind a NUL\n> > byte after the first ref sent from the server during the ref\n> > advertisement:\n> > ...\n> >\n> > * Various other technical debt (e.g. abusing capabilities to\n> > communicate agent and symref data, service name set using a query\n> > parameter).\n> \n> This sounds like a duplicate of the above.\n\nYou're right, it mostly is a duplication of that.\n\n> \n> > Special Packets\n> > -----------------\n> >\n> > In protocol v2 these special packets will have the following semantics:\n> >\n> > * '0000' Flush Packet (flush-pkt) - indicates the end of a message\n> > * '0001' End-of-List delimiter (delim-pkt) - indicates the end of a list\n> \n> After reading the remainder of the document twice, I do not think\n> the reason why we want to introduce a new type delim-pkt is\n> explained and justified well enough. If a command request takes a\n> command packet, zero or more capability packets, a mandatory\n> delimiter packet, zero or more parameter packets and a mandatory\n> flush packet, then you can use the same \"flush\" as delimiter in the\n> middle. The delimiter will of course become useful if you can omit\n> it when not necessary (e.g. after seeing capabilities, you may see a\n> flush and you will know there is no parameters and save the need to\n> send one \"delim\").\n> \n> I actually have a reasonable guess why you want to have a separate\n> delimiter (which has nothing to do with \"optional delim can be\n> omitted\"), but I want to see it explained in this document clearly\n> by its designer(s).\n\nJonathan Tan suggested that we tighten flush semantics in a newer\nprotocol so that proxies are easier to work with. Currently proxies\nneed to understand the protocol instead of simply waiting for a flush.\n\nAlso I've been told the smart http code is more complex because of the\ncurrent semantics of flush packets.\n\n> \n> > command-request = command\n> > capability-list\n> > delim-pkt\n> > (command specific parameters)\n> > flush-pkt\n> > command = PKT-LINE(\"command=\" key LF)\n> >\n> > The server will then acknowledge the command and requested capabilities\n> > by echoing them back to the client and then launch into the command.\n> >\n> > acknowledge-request = command\n> > capability-list\n> > delim-pkt\n> > execute-command\n> > execute-command = \n> \n> It is not quite clear what the value of \"echoing them back\" is,\n> especially if that is done by always echoing verbatim. A reader may\n> naturally expect, when capabilities are exchanged between two\n> parties, these are negotiated so that the only ones that are\n> commonly supported by both ends would be used, or something like\n> that.\n\nThe echoing back of the command and requested capabilities may or may\nnot be needed. A client should only ever issue a command-request using\nadvertised capabilities and commands so there really isn't much\nnegotiation happening, just the server saying \"here's what's on the\nmenu\" and the client picking only from that menu.\n\nWhere the echoing back may be useful is if we wanted to (at some point)\neliminate this initial round trip of doing the capability advertisement\nand then subsequent selection of capabilities and instead stuff that\ninformation into the GIT_PROTOCOL side channel in the initial request.\nThat way the client could optimistically send capabilities that it\ndoesn't yet know if the server supports. I thought this might be an\ninteresting idea if we really really didn't want to live with the extra\nround trip.\n\n> \n> > A particular command can last for as many rounds as are required to\n> > complete the service (multiple for negotiation during fetch and push or\n> > no additional trips in the case of ls-refs).\n> \n> OK.\n> \n> > Commands in v2\n> > ~~~~~~~~~~~~~~~~\n> >\n> > Services are the core actions that a client wants to perform (fetch,\n> > push, etc). Each service has its own set of capabilities and its own\n> > language of commands (think 'want' lines in fetch). Optionally a\n> > service can take in initial parameters or data when a client sends it\n> > service request.\n> \n> So a service (like \"fetch\") employ a set of \"command\"s (like \"want\",\n> \"have, etc)? In the earlier part of the document, we did not see\n> any mention of \"service\" and instead saw only \"command\" mentioned.\n> Is the state machine on both ends and the transition between states\n> implicit? E.g. when one side throws \"want\" command and the other\n> side acknowledges, they understand implicitly that they are now in a\n> \"fetch\" service session, even though there is nothing that said over\n> the wire that they are doing so? One reason I am wondering about\n> this is what we should do if a command verb may be applicable in\n> multiple services.\n\nLooks like I missed changing the word \"services\" to \"commands\" here. I\noriginally was using the term 'services' for things like 'fetch', 'push',\n'ls-refs', etc. and decided for some reason to change to using the word\n'commands'. Naming things is hard, especially when you couldn't decide\non a name and end up with two! ;) \n\n> After reading the earlier protocol exchange explanation, I was sort\n> of expecting that \"fetch\" would be the command and \"want\", \"have\",\n> and \"ack\" lines would be exchanged as \"command specific parameters\",\n> so a sudden introduction of \"services\" here was a bit of impedance\n> mismatch to me.\n\nYou are absolutely correct in that i intended 'fetch' to be the command\nand 'want', 'have', and 'ack' lines would be parameters.\n\n> \n> > Ls-refs\n> > ---------\n> >\n> > Ls-refs can be looked at as the equivalent of the current ls-remote as\n> > it is a way to query a remote for the references that it has. Unlike\n> > the current ls-remote, the filtering of the output is done on the server\n> > side by passing a number of parameters to the server-side command\n> > instead of the filtering occurring on the client.\n> >\n> > Ls-ref takes in the following parameters:\n> >\n> > --head, --tags: Limit to only refs/heads or refs/tags\n> \n> I see no need for the above two as long as \"refs/heads/*\", etc. are\n> understood.\n> \n> > --refs: Do not show peeled tags or pseudorefs like HEAD\n> \n> So showing peeled tags is the default? Then I can sort-of see why\n> \"I am not interested in peeled result\". \n> \n> I do not see why \"do not show HEAD, MERGE_HEAD, etc.\" is needed,\n> though. It should be sufficient to just ask for refs/* if you are\n> interested only in other things, no?\n> \n> > --symref: In addition to the object pointed by it, show the underlying\n> > ref pointed by it when showing a symbolic ref\n> \n> Sort of OK--it probably is easier to always send this, as symbolic\n> refs are minority anyway, though.\n> \n> > : When specified, only references matching the given patterns\n> > are displayed.\n> \n> I do not think you meant here.\n> \n> The side that is listing what it has has no reason to know what the\n> recipient plans to do with the result, so you must be only sending\n> the LHS of a refspec. If your explanation says \"given patterns\",\n> then replace with . Do not abuse a term that has\n> specific and established meaning for something else.\n\nYes, you're right i intended that to mean instead so that the\nclient could send \"refs/heads/*\" or some other such pattern and have the\nserver limit its output.\n\nAll of these parameters I just pulled from the current ls-remote command\nthinking that whatever filtering the client currently does can end up\nbeing done on the server. You've illustrated that most of them could\nsimply be done with apattern so maybe i was overthinking it :)\n\n> \n> > The output of ls-refs is as follows:\n> >\n> > output = (no-refs / list-of-refs)\n> > \t *symref\n> > *shallow\n> > flush-pkt\n> >\n> > no-refs = PKT-LINE(zero-id SP no-refs LF)\n> \n> Can't your list-of-refs have 0 element? I do not see why you need\n> no-refs here. It's not like you need a dummy line, to the end of\n> which you need to append NUL plus hidden capabilities ;-)\n\nHaha! good point. Yes a list-of-refs can certainly have 0 elements.\nOne less thing that can be borrowed from the old protocol :)\n\n> \n> > list-of-refs = *ref\n> > ref = PKT-LINE((tip / peeled) LF)\n> > tip = obj-id SP refname\n> > peeled = obj-id SP refname \"^{}\"\n> >\n> > symref = PKT-LINE(\"symref\" SP symbolic-ref SP resolved-ref LF)\n> > shallow = PKT-LINE(\"shallow\" SP obj-id LF)\n> \n> Thanks.\n\n-- \nBrandon Williams\n"},{"id":"330975","messageId":"xmqqwp3kja2m.fsf@gitster.mtv.corp.google.com","threadId":"47006","inReplyTo":"20171024183557.GB79163@google.com","subject":"Re: [RFC] protocol version 2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-25T01:22:09Z","receivedAt":"2017-10-25T01:22:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n>> I actually have a reasonable guess why you want to have a separate\n>> delimiter (which has nothing to do with \"optional delim can be\n>> omitted\"), but I want to see it explained in this document clearly\n>> by its designer(s).\n>\n> Jonathan Tan suggested that we tighten flush semantics in a newer\n> protocol so that proxies are easier to work with. Currently proxies\n> need to understand the protocol instead of simply waiting for a flush.\n>\n> Also I've been told the smart http code is more complex because of the\n> current semantics of flush packets.\n\nI think the above two are the same thing ;-) but yes, \"flush\" in the\noriginal protocol were used for both \"I am truly finished talking;\nnow it is your turn\" and \"I am done with one section of what I need\nto say, and a different section now begins; it is still my turn to\nspeak\". The need to handle the latter makes smart-http quite ugly.\n\nThanks.\n"},{"id":"331017","messageId":"0ba57a04-9321-e765-ed19-3d0e68e1edc0@gmail.com","threadId":"47006","inReplyTo":"20171020171839.4188-1-bmwill@google.com","subject":"Re: [RFC] protocol version 2","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2017-10-25T13:09:12Z","receivedAt":"2017-10-25T13:09:22Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/20/2017 1:18 PM, Brandon Williams wrote:\n> Overview\n> ==========\n>\n> This document presents a specification for a version 2 of Git's wire\n> protocol. Protocol v2 will improve upon v1 in the following ways:\n>\n> * Instead of multiple service names, multiple commands will be\n> supported by a single service\n> * Easily extendable as capabilities are moved into their own section\n> of the protocol, no longer being hidden behind a NUL byte and\n> limited by the size of a pkt-line (as there will be a single\n> capability per pkt-line).\n> * Separate out other information hidden behind NUL bytes (e.g. agent\n> string as a capability and symrefs can be requested using 'ls-ref')\n> * Ref advertisement will be omitted unless explicitly requested\n> * ls-ref command to explicitly request some refs\n\nHi Brandon,\n\nI'm very interested in your protocol as a former server-side dev for the \nVSTS Git server, and understand some of these headaches. We built \nlimited refs specifically to target the problem you are solving with \nls-ref, but it requires knowledge about the authenticated user in order \nto work. I believe your suggestion is a better solution for the Git \nprotocol.\n\nThe \"easily extendable\" part has specifically caught my interest, as we \n(Microsoft) would like to move most of the GVFS protocol into core Git, \nand this is a great way to do it. Even if not all features are accepted \nby upstream, we could use our GVFS-specific fork of Git to communicate \nto our servers without breaking normal users' interactions.\n\nPlease CC me in future versions of this proposal. Let me know if you \nwant to chat directly about the \"TODO\" items below.\n\nSpeaking of TODOs, how much of this concept do you have working in a \nprototype? Do you have code that performs this version 2 handshake and \ncommunicates the ls-refs result?\n\n> Ls-refs\n> ---------\n>\n> Ls-refs can be looked at as the equivalent of the current ls-remote as\n> it is a way to query a remote for the references that it has. Unlike\n> the current ls-remote, the filtering of the output is done on the server\n> side by passing a number of parameters to the server-side command\n> instead of the filtering occurring on the client.\n>\n> Ls-ref takes in the following parameters:\n>\n> --head, --tags: Limit to only refs/heads or refs/tags\n\nNit: It would be better to use \"--heads\" to match refs/heads and your \nuse of \"--tags\" for refs/tags.\n\n> --refs: Do not show peeled tags or pseudorefs like HEAD\n\nAssuming we are in the case where the server has a HEAD ref, why would \nthat ever be advertised? Also, does this imply that without the --refs \noption we would peel annotated tags until we find non-tag OIDs? Neither \nof these functions seem useful as default behavior.\n\n> --symref: In addition to the object pointed by it, show the underlying\n> ref pointed by it when showing a symbolic ref\n> : When specified, only references matching the given patterns\n> are displayed.\n\nCan you be specific about the patterns? For instance, it is not a good \nidea to allow the client to submit a regex for the server to compute. \nInstead, can we limit this pattern-matching to a prefix-set, such as the \nfollowing list of prefixes:\n\n     refs/heads/master\n     refs/releases/*\n     refs/heads/user/me/*\n> Fetch\n> -------\n>\n> Fetch will need to be a modified version of the v1 fetch protocol. Some\n> potential areas for improvement are: Ref-in-want, CDN offloading,\n> Fetch-options.\n>\n> Since we'll have an 'ls-ref' service we can eliminate the need of fetch\n> to perform a ref-advertisement, instead a client can run the 'ls-refs'\n> service first, in order to find out what refs the server has, and then\n> request those refs directly using the fetch service.\n>\n> //TODO Flush out the design\n>\n> Fetch-object\n> --------------\n>\n> This service could be used by partial clones in order to request missing\n> objects.\n>\n> //TODO Flush out the design\n\nAs you flesh our these \"fetch\" and \"fetch-object\" commands, keep in mind \nthat partial clones could mean any of the following:\n\n  * fetch all reachable objects except for blobs.\n\n  * fetch all reachable objects except for blobs above a\n    certain size.\n\n  * fetch all commits, trees, (and blobs?) within a certain\n    \"cone\" of the file system.\n\n> Push\n> ------\n>\n> Push will need to be a modified version of the v1 push protocol. Some\n> potential areas for improvement are: Fix push-options, Negotiation for\n> force push.\n\nNegotiation is something to keep in mind for all pushes, especially in \nan ecosystem full of fork-based workflows. If you are working across \nforks and someone else syncs data between your remotes, you may re-push \na large chunk of objects that are already present in a fork. Adding an \nls-refs step before push would be a step in the right direction.\n> Other Considerations\n> ======================\n>\n> * Move away from pkt-line framing?\n> * Have responses structured in well known formats (e.g. JSON)\n> * Eliminate initial round-trip using 'GIT_PROTOCOL' side-channel\n> * Additional commands in a partial clone world (e.g. log, grep)\n\n[Tangent]\n\nI too have thought about making calls like \"log\" and \"blame\" available \nfor calling remotes. One reason GVFS sends a \"prefetch pack\" of _all_ \ncommits and trees is because one \"git log -- path/to/file\" command would \nstart downloading thousands of objects one at a time as the walk moves \nthrough the history. If the remote can compute the commands that require \nhistorical data, then our partial clones can be more \"pure\" (i.e. only \ncontain objects required for the user's changes).\n\nOne major caveat: if someone runs \"log\" from HEAD, then they may be \nworking over data that is not on the remote, which means they would need \nto start the history walk until reaching commits that are known to be on \nthe remote. If there are merges in the local history, then this could \ninclude multiple independent commits.\n\nFurther complicating this area, the server may not want to allow certain \ntypes of commands (i.e. regexes, expensive history options like \n\"--simplify-merges\").\n\nIn conclusion, I think it is a great idea to have the protocol allow \nthese extensions, especially in a way that is easy to extend without \nbreaking client/server compat scenarios (after both have v2 enabled).\n\n[End Tangent]\n\nThanks,\n-Stolee\n\n"},{"id":"331027","messageId":"20171025181050.GA18778@google.com","threadId":"47006","inReplyTo":"0ba57a04-9321-e765-ed19-3d0e68e1edc0@gmail.com","subject":"Re: [RFC] protocol version 2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-10-25T18:10:50Z","receivedAt":"2017-10-25T18:11:02Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 10/25, Derrick Stolee wrote:\n> On 10/20/2017 1:18 PM, Brandon Williams wrote:\n> > Overview\n> >==========\n> >\n> >This document presents a specification for a version 2 of Git's wire\n> >protocol. Protocol v2 will improve upon v1 in the following ways:\n> >\n> > * Instead of multiple service names, multiple commands will be\n> > supported by a single service\n> > * Easily extendable as capabilities are moved into their own section\n> > of the protocol, no longer being hidden behind a NUL byte and\n> > limited by the size of a pkt-line (as there will be a single\n> > capability per pkt-line).\n> > * Separate out other information hidden behind NUL bytes (e.g. agent\n> > string as a capability and symrefs can be requested using 'ls-ref')\n> > * Ref advertisement will be omitted unless explicitly requested\n> > * ls-ref command to explicitly request some refs\n> \n> Hi Brandon,\n> \n> I'm very interested in your protocol as a former server-side dev for\n> the VSTS Git server, and understand some of these headaches. We\n\nAlways happy to hear that someone is excited about the area I'm working\non :)\n\n> built limited refs specifically to target the problem you are\n> solving with ls-ref, but it requires knowledge about the\n> authenticated user in order to work. I believe your suggestion is a\n> better solution for the Git protocol.\n\nYes one of the big issues we've run into is the ref advertisement, 1\nbecause it makes it difficult to add any additional features to the\nprotocol and 2 because it doesn't scale well because all refs are\nblasted at the client (unless you use a separate system like you\nimplemented to reduce that number). So I'm hoping we can solve (1) by\ndoing some sort of capability advertisement instead of a ref\nadvertisement upfront and (2) by allowing clients to express a way of\nlimiting the ref advertisement server side.\n\n> \n> The \"easily extendable\" part has specifically caught my interest, as\n> we (Microsoft) would like to move most of the GVFS protocol into\n> core Git, and this is a great way to do it. Even if not all features\n> are accepted by upstream, we could use our GVFS-specific fork of Git\n> to communicate to our servers without breaking normal users'\n> interactions.\n\nThis is one thing I'm excited about too and hope there's enough desire\nfor such a capability to extend the protocol. I first was interested in\nbuilding such a system when i looked at some of the previous work done\nby stephan trying to introduce a protocol v2\n(https://public-inbox.org/git/1429658342-5295-1-git-send-email-sbeller@google.com/)\nand this idea was further reinforced when I sat down and talked with\nsome mercurial developers about their protocol and what they did to\nmigrate to a v2. I also discovered that their v2 protocol is\nservice/command oriented and it makes it very simple and easy for\nextensions to be added server and client side which allow for additional\ncommands to be executed during a server/client exchange without breaking\nthe normal fetch/push interaction.\n\nSo I'm hoping that what we decided on for v2 will enable exactly what\nyou want. It may even allow for doing things like server side log and\ngrep when a client has a partial clone or a repo which is so big it\nwould be difficult to do some of those operations locally. Of course\nyou identified some of the issues which such operations below :)\n\n> \n> Please CC me in future versions of this proposal. Let me know if you\n> want to chat directly about the \"TODO\" items below.\n\nOf course, I'll make sure to keep you updated.\n\n> \n> Speaking of TODOs, how much of this concept do you have working in a\n> prototype? Do you have code that performs this version 2 handshake\n> and communicates the ls-refs result?\n\nMost of the TODOs are areas where more thought and design needs to\nhappen. My main goal with this RFC is to see if a modular design like\nthis would have support from the community and if so, to nail down the\ndesign of the exchanges outside of individual commands since that\ninfrastructure would be harder to change after the fact.\n\nAs for the commands themselves, since this design is meant to be module\nwe could implement them separately or one at a time (though a base set\nof commands would need to be implemented and designed before v2 could\nroll out). That being said I've begun working up a rough prototype of\nthe basic initial capability/command exchange and the ls-refs command.\nSince its pretty rough and not integrated into the actual transport code\nyet its no where near ready to be sent out though I wouldn't mind\npushing the WIP code out so people can see/play with what's there.\n\nAnd if you have any comments or suggestions about any parts of the\ndesign or the TODOs I would love to chat about it :)\n\n> \n> > Ls-refs\n> >---------\n> >\n> >Ls-refs can be looked at as the equivalent of the current ls-remote as\n> >it is a way to query a remote for the references that it has. Unlike\n> >the current ls-remote, the filtering of the output is done on the server\n> >side by passing a number of parameters to the server-side command\n> >instead of the filtering occurring on the client.\n> >\n> >Ls-ref takes in the following parameters:\n> >\n\nFor context most of these options i pulled from the existing ls-remote\ncommand and was assuming the existing behavior of the ref advertisement,\nso some of these options may definitely need to be tweaked in addition\nto what the default would be (without any options).\n\n> > --head, --tags: Limit to only refs/heads or refs/tags\n> \n> Nit: It would be better to use \"--heads\" to match refs/heads and\n> your use of \"--tags\" for refs/tags.\n\nThanks for catching this :)\n\n> \n> > --refs: Do not show peeled tags or pseudorefs like HEAD\n> \n> Assuming we are in the case where the server has a HEAD ref, why\n> would that ever be advertised? Also, does this imply that without\n> the --refs option we would peel annotated tags until we find non-tag\n> OIDs? Neither of these functions seem useful as default behavior.\n\nCurrently both are true of the existing ref advertisement, all tags are\npeeled and HEAD is advertised. Maybe we don't need this to be the\ndefault though IIRC upon a clone the advertised HEAD is what is checked\nout by default once the clone completes.\n\n> \n> > --symref: In addition to the object pointed by it, show the underlying\n> > ref pointed by it when showing a symbolic ref\n> > : When specified, only references matching the given patterns\n> > are displayed.\n> \n> Can you be specific about the patterns? For instance, it is not a\n> good idea to allow the client to submit a regex for the server to\n> compute. Instead, can we limit this pattern-matching to a\n> prefix-set, such as the following list of prefixes:\n> \n>     refs/heads/master\n>     refs/releases/*\n>     refs/heads/user/me/*\n\nYou're right we may need to think carefully about what kind of limiting\nwe allow to prevent malicious clients.\n\n> > Fetch\n> >-------\n> >\n> >Fetch will need to be a modified version of the v1 fetch protocol. Some\n> >potential areas for improvement are: Ref-in-want, CDN offloading,\n> >Fetch-options.\n> >\n> >Since we'll have an 'ls-ref' service we can eliminate the need of fetch\n> >to perform a ref-advertisement, instead a client can run the 'ls-refs'\n> >service first, in order to find out what refs the server has, and then\n> >request those refs directly using the fetch service.\n> >\n> >//TODO Flush out the design\n> >\n> > Fetch-object\n> >--------------\n> >\n> >This service could be used by partial clones in order to request missing\n> >objects.\n> >\n> >//TODO Flush out the design\n> \n> As you flesh our these \"fetch\" and \"fetch-object\" commands, keep in\n> mind that partial clones could mean any of the following:\n> \n>  * fetch all reachable objects except for blobs.\n> \n>  * fetch all reachable objects except for blobs above a\n>    certain size.\n> \n>  * fetch all commits, trees, (and blobs?) within a certain\n>    \"cone\" of the file system.\n> \n> > Push\n> >------\n> >\n> >Push will need to be a modified version of the v1 push protocol. Some\n> >potential areas for improvement are: Fix push-options, Negotiation for\n> >force push.\n> \n> Negotiation is something to keep in mind for all pushes, especially\n> in an ecosystem full of fork-based workflows. If you are working\n> across forks and someone else syncs data between your remotes, you\n> may re-push a large chunk of objects that are already present in a\n> fork. Adding an ls-refs step before push would be a step in the\n> right direction.\n> > Other Considerations\n> >======================\n> >\n> > * Move away from pkt-line framing?\n> > * Have responses structured in well known formats (e.g. JSON)\n> > * Eliminate initial round-trip using 'GIT_PROTOCOL' side-channel\n> > * Additional commands in a partial clone world (e.g. log, grep)\n> \n> [Tangent]\n> \n> I too have thought about making calls like \"log\" and \"blame\"\n> available for calling remotes. One reason GVFS sends a \"prefetch\n> pack\" of _all_ commits and trees is because one \"git log --\n> path/to/file\" command would start downloading thousands of objects\n> one at a time as the walk moves through the history. If the remote\n> can compute the commands that require historical data, then our\n> partial clones can be more \"pure\" (i.e. only contain objects\n> required for the user's changes).\n> \n> One major caveat: if someone runs \"log\" from HEAD, then they may be\n> working over data that is not on the remote, which means they would\n> need to start the history walk until reaching commits that are known\n> to be on the remote. If there are merges in the local history, then\n> this could include multiple independent commits.\n\nThese are some of the same points that some of my fellow coworkers have\nalso pointed out and definitely needs to be kept in mind if we ever\nimplement such a command.\n\n> \n> Further complicating this area, the server may not want to allow\n> certain types of commands (i.e. regexes, expensive history options\n> like \"--simplify-merges\").\n> \n> In conclusion, I think it is a great idea to have the protocol allow\n> these extensions, especially in a way that is easy to extend without\n> breaking client/server compat scenarios (after both have v2\n> enabled).\n> \n> [End Tangent]\n> \n> Thanks,\n> -Stolee\n> \n\nThanks for taking a look and giving your feedback!\n\n-- \nBrandon Williams\n"},{"id":"331050","messageId":"xmqqbmkuhgfm.fsf@gitster.mtv.corp.google.com","threadId":"47006","inReplyTo":"20171024183557.GB79163@google.com","subject":"Re: [RFC] protocol version 2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-26T00:59:57Z","receivedAt":"2017-10-26T01:00:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> On 10/24, Junio C Hamano wrote:\n>> Brandon Williams writes:\n>> \n>> > : When specified, only references matching the given patterns\n>> > are displayed.\n>> \n>> I do not think you meant here.\n>> \n>> The side that is listing what it has has no reason to know what the\n>> recipient plans to do with the result, so you must be only sending\n>> the LHS of a refspec. If your explanation says \"given patterns\",\n>> then replace with . Do not abuse a term that has\n>> specific and established meaning for something else.\n>\n> Yes, you're right i intended that to mean instead so that the\n> client could send \"refs/heads/*\" or some other such pattern and have the\n> server limit its output.\n\nSpeaking of limiting the bandwidth consumed by the ref\nadvertisement, I think another trick that came up in past\ndiscussions may be worth considering, which is to allow the\nrequestor to say, \"oh by the way, I made exactly the same request as\nthis one earlier to you, and your response hashed down to this\nvalue\". The responder may choose to give an incremental response\nrelative to the known result the requestor claims to have.\n\nSo for example, a requestor may have made an earlier ls-refs request\nand can still recall that it got:\n\n\trefs/heads/maint\tObjectID A\n\trefs/heads/master\tObjectID B\n\trefs/tags/v1.0\t\tObjectId C\n\nAlso assume that these three lines together (textually) hashes to Z.\n\nWhen the requestor asks about the two hierarchies, it may say \"I know\nyou gave a result that hashes to Z\" with an additional parameter:\n\n\tcommand=ls-ref\n\t\trefspec=refs/heads/*\n\t\trefspec=refs/tags/*\n\t\tknown=Z\n\nIf the current response for refs/heads/* and refs/tags/* when fully\nspelt out were like this (i.e. we updated a ref, gained another, and\nlost one):\n\n\trefs/heads/master\tObjectID D\n\trefs/tags/v1.0\t\tObjectId C\n\trefs/tags/v1.1\t\tObjectID E\n\nthen the responder can send the fully spelt version, or it can\nchoose to say, \"It's good that you know the state Z; relative to\nthat, refs/heads/maint no loner exists, refs/heads/master is now at\nD and refs/tags/v1.1 at E has been added\", if the latter results in\na shorter response (and if it recognises Z and map it back to the\nset of refs and their values it responded with).\n\nThe \"known\" request parameter could further be refined (I do not\nthink this possibility was discussed in the past) to say \"among the\nvalues I received earlier from you, the ones that match this pattern\nhashes to this\", e.g. the earlier example request might become\n\n\tcommand=ls-ref\n\t\trefspec=refs/heads/*\n\t\trefspec=refs/tags/*\n\t\tknown=X for refs/heads/*\n\t\tknown=Y for refs/tags/*\n\n"},{"id":"331247","messageId":"223949CCB658443C972AB1AC1820F0CC@PhilipOakley","threadId":"47006","inReplyTo":"20171020171839.4188-1-bmwill@google.com","subject":"Re: [RFC] protocol version 2","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2017-10-28T22:57:21Z","receivedAt":"2017-10-28T22:57:30Z","isPatch":false,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Brandon Williams\" \nSent: Friday, October 20, 2017 6:18 PM\n> Objective\n> ===========\n>\n> Replace Git's current wire protocol with a simpler, less wasteful\n> protocol that can evolve over time.\n>\n\n\n\n> Capability Advertisement\n> --------------------------\n>\n> A server which decides to communicate (based on a request from a client)\n> using protocol version 2, notifies the client by sending a version\n> string in its initial response followed by an advertisement of its\n> capabilities. Each capability is a key with an optional value. Clients\n> must ignore all unknown keys.\n\n> Semantics of unknown values are left to\n> the definition of each key.\n\nThis sounds odd. If the keys are known then their semantics are known. Or \nthe keys are unknown and they and their values are ignored.\n\nMaybe: Capability keys shall define their response to unknown key values.\n\n> Some capabilities will describe commands\n> which can be requested to be executed by the client.\n>\n\n\n> Ls-refs\n> ---------\n>\n> Ls-refs can be looked at as the equivalent of the current ls-remote as\n> it is a way to query a remote for the references that it has. Unlike\n> the current ls-remote, the filtering of the output is done on the server\n> side by passing a number of parameters to the server-side command\n> instead of the filtering occurring on the client.\n>\n> Ls-ref takes in the following parameters:\n>\n> --head, --tags: Limit to only refs/heads or refs/tags\n> --refs: Do not show peeled tags or pseudorefs like HEAD\n> --symref: In addition to the object pointed by it, show the underlying\n> ref pointed by it when showing a symbolic ref\n> : When specified, only references matching the given patterns\n> are displayed.\n\nDoes the --symref also the pseudorefs?\n\nIsn't there a need somethimes to determine the ref that the remote's HEAD \npoints to. This is an issue with the current clone and bundle code when \nthere is a choice of refs/branches that could be the current HEAD ref and \nthe wrong one is chosen, though this V2 change doesn't affect bundles.\n\n>\n> The output of ls-refs is as follows:\n>\n> output = (no-refs / list-of-refs)\n> *symref\n> *shallow\n> flush-pkt\n>\n> no-refs = PKT-LINE(zero-id SP no-refs LF)\n> list-of-refs = *ref\n> ref = PKT-LINE((tip / peeled) LF)\n> tip = obj-id SP refname\n> peeled = obj-id SP refname \"^{}\"\n>\n> symref = PKT-LINE(\"symref\" SP symbolic-ref SP resolved-ref LF)\n> shallow = PKT-LINE(\"shallow\" SP obj-id LF)\n>\n> Fetch\n> -------\n>\n> Fetch will need to be a modified version of the v1 fetch protocol. Some\n> potential areas for improvement are: Ref-in-want, CDN offloading,\n> Fetch-options.\n>\n> Since we'll have an 'ls-ref' service we can eliminate the need of fetch\n> to perform a ref-advertisement, instead a client can run the 'ls-refs'\n> service first, in order to find out what refs the server has, and then\n> request those refs directly using the fetch service.\n>\n> //TODO Flush out the design\n>\n> Fetch-object\n> --------------\n>\n> This service could be used by partial clones in order to request missing\n> objects.\n>\n> //TODO Flush out the design\n>\n> Push\n> ------\n>\n> Push will need to be a modified version of the v1 push protocol. Some\n> potential areas for improvement are: Fix push-options, Negotiation for\n> force push.\n>\n> One change that will need to happen is to improve how `push-options` are\n> sent to the server (so that they aren't sent twice!!). Also the\n> report-status needs to be better than it currently is in v1 so that\n> tools like gerrit can explain what it did with the ref-update the client\n> sent to it. Maybe have a push-rebase capability or command?\n>\n> //TODO Flush out the design\n>\n> Other Considerations\n> ======================\n>\n> * Move away from pkt-line framing?\n> * Have responses structured in well known formats (e.g. JSON)\n> * Eliminate initial round-trip using 'GIT_PROTOCOL' side-channel\n> * Additional commands in a partial clone world (e.g. log, grep)\n--\nPhilip \n\n"},{"id":"331506","messageId":"20171031184222.GD94048@google.com","threadId":"47006","inReplyTo":"223949CCB658443C972AB1AC1820F0CC@PhilipOakley","subject":"Re: [RFC] protocol version 2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-10-31T18:42:22Z","receivedAt":"2017-10-31T18:42:32Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 10/28, Philip Oakley wrote:\n> From: \"Brandon Williams\" \n> Sent: Friday, October 20, 2017 6:18 PM\n> >Objective\n> >===========\n> >\n> >Replace Git's current wire protocol with a simpler, less wasteful\n> >protocol that can evolve over time.\n> >\n> \n> \n> \n> >Capability Advertisement\n> >--------------------------\n> >\n> >A server which decides to communicate (based on a request from a client)\n> >using protocol version 2, notifies the client by sending a version\n> >string in its initial response followed by an advertisement of its\n> >capabilities. Each capability is a key with an optional value. Clients\n> >must ignore all unknown keys.\n> \n> > Semantics of unknown values are left to\n> >the definition of each key.\n> \n> This sounds odd. If the keys are known then their semantics are\n> known. Or the keys are unknown and they and their values are\n> ignored.\n> \n> Maybe: Capability keys shall define their response to unknown key values.\n\nI'll work to make the language a little clearer.\n\n> \n> > Some capabilities will describe commands\n> >which can be requested to be executed by the client.\n> >\n> \n> \n> >Ls-refs\n> >---------\n> >\n> >Ls-refs can be looked at as the equivalent of the current ls-remote as\n> >it is a way to query a remote for the references that it has. Unlike\n> >the current ls-remote, the filtering of the output is done on the server\n> >side by passing a number of parameters to the server-side command\n> >instead of the filtering occurring on the client.\n> >\n> >Ls-ref takes in the following parameters:\n> >\n> > --head, --tags: Limit to only refs/heads or refs/tags\n> > --refs: Do not show peeled tags or pseudorefs like HEAD\n> > --symref: In addition to the object pointed by it, show the underlying\n> > ref pointed by it when showing a symbolic ref\n> > : When specified, only references matching the given patterns\n> > are displayed.\n> \n> Does the --symref also the pseudorefs?\n> \n> Isn't there a need somethimes to determine the ref that the remote's\n> HEAD points to. This is an issue with the current clone and bundle\n> code when there is a choice of refs/branches that could be the\n> current HEAD ref and the wrong one is chosen, though this V2 change\n> doesn't affect bundles.\n\nYeah, currently the resolution of HEAD is stuffed into the\ncapability line in v1. The intention of this would be to allow for the\nresolution of all symrefs (including HEAD).\n\n> \n> >\n> >The output of ls-refs is as follows:\n> >\n> > output = (no-refs / list-of-refs)\n> > *symref\n> > *shallow\n> > flush-pkt\n> >\n> > no-refs = PKT-LINE(zero-id SP no-refs LF)\n> > list-of-refs = *ref\n> > ref = PKT-LINE((tip / peeled) LF)\n> > tip = obj-id SP refname\n> > peeled = obj-id SP refname \"^{}\"\n> >\n> > symref = PKT-LINE(\"symref\" SP symbolic-ref SP resolved-ref LF)\n> > shallow = PKT-LINE(\"shallow\" SP obj-id LF)\n> >\n> >Fetch\n> >-------\n> >\n> >Fetch will need to be a modified version of the v1 fetch protocol. Some\n> >potential areas for improvement are: Ref-in-want, CDN offloading,\n> >Fetch-options.\n> >\n> >Since we'll have an 'ls-ref' service we can eliminate the need of fetch\n> >to perform a ref-advertisement, instead a client can run the 'ls-refs'\n> >service first, in order to find out what refs the server has, and then\n> >request those refs directly using the fetch service.\n> >\n> >//TODO Flush out the design\n> >\n> >Fetch-object\n> >--------------\n> >\n> >This service could be used by partial clones in order to request missing\n> >objects.\n> >\n> >//TODO Flush out the design\n> >\n> >Push\n> >------\n> >\n> >Push will need to be a modified version of the v1 push protocol. Some\n> >potential areas for improvement are: Fix push-options, Negotiation for\n> >force push.\n> >\n> >One change that will need to happen is to improve how `push-options` are\n> >sent to the server (so that they aren't sent twice!!). Also the\n> >report-status needs to be better than it currently is in v1 so that\n> >tools like gerrit can explain what it did with the ref-update the client\n> >sent to it. Maybe have a push-rebase capability or command?\n> >\n> >//TODO Flush out the design\n> >\n> >Other Considerations\n> >======================\n> >\n> > * Move away from pkt-line framing?\n> > * Have responses structured in well known formats (e.g. JSON)\n> > * Eliminate initial round-trip using 'GIT_PROTOCOL' side-channel\n> > * Additional commands in a partial clone world (e.g. log, grep)\n> --\n> Philip\n> \n\n-- \nBrandon Williams\n"},{"id":"332246","messageId":"20171110121347.1f7c184c543622b60164e9fb@google.com","threadId":"47006","inReplyTo":"20171020171839.4188-1-bmwill@google.com","subject":"Re: [RFC] protocol version 2","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-11-10T20:13:47Z","receivedAt":"2017-11-10T20:13:58Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Fri, 20 Oct 2017 10:18:39 -0700\nBrandon Williams wrote:\n\n> Some of the pain points with the current protocol spec are:\n\nAfter some in-office discussion, I think that the most important pain\npoint is that we have to implement each protocol twice: once for\nHTTP(S), and once for SSH (and friends) that support bidirectional byte\nstreams.\n\nIf it weren't for this, I think that what is discussed in this document\n(e.g. ls-refs, fetch-object) can be less invasively accomplished with\nv1, specifying \"extra parameters\" (explained in this e-mail [1]) to\nmerely tweak the output of upload-pack instead of replacing it nearly\ncompletely, thus acting more as optimizations than changing the mode of\noperation entirely.\n\n[1] https://public-inbox.org/git/20171010193956.168385-1-jonathantanmy@google.com/\n\n> * The server's initial response is the ref advertisement. This\n> advertisement cannot be omitted and can become an issue due to the\n> sheer number of refs that can be sent with large repositories. For\n> example, when contacting the internal equivalent of\n> `https://android.googlesource.com/`, the server will send\n> approximately 1 million refs totaling 71MB. This is data that is\n> sent during each and every fetch and is not scalable.\n\nFor me, this is not a compelling one, because we can provide a ref\nwhitelist as an \"extra parameter\" in v1.\n\n> * Capabilities were implemented as a hack and are hidden behind a NUL\n> byte after the first ref sent from the server during the ref\n> advertisement:\n> \n> \t \\0 \n> \n> Since they are sent in the context of a pkt-line they are also subject\n> to the same length limitations (1k bytes with old clients). While we\n> may not be close to hitting this limitation with capabilities alone, it\n> has become a problem when trying to abuse capabilities for other\n> purposes (e.g. [symrefs](https://public-inbox.org/git/20160816161838.klvjhhoxsftvkfmd@x/)).\n> \n> * Various other technical debt (e.g. abusing capabilities to\n> communicate agent and symref data, service name set using a query\n> parameter).\n\nI think these 2 are the same - I would emphasize the fact that we cannot\nadd more stuff here, rather than the fact that we're putting this behind\nNUL.\n\n> Special Packets\n> -----------------\n> \n> In protocol v2 these special packets will have the following semantics:\n> \n> * '0000' Flush Packet (flush-pkt) - indicates the end of a message\n> * '0001' End-of-List delimiter (delim-pkt) - indicates the end of a list\n\nTo address the pain point of HTTP(S) being different from the others\n(mentioned above), I think the packet semantics should be further\nqualified:\n\n - Communications must be divided up into packets terminated by a\n flush-pkt. Also, each side must be implemented without knowing\n whether packets-in-progress can or cannot be seen by the other side.\n - Each request packet must have a corresponding, possibly empty,\n response packet.\n - A request packet may be sent even if a response packet corresponding\n to a previously sent request packet is awaited. (This allows us to\n retain the existing optimization in fetch-pack wherein, during\n negotiation, the \"have\" request-response packet pairs are\n interleaved.)\n\nThis will allow us to more easily share code between HTTP(S) and the\nothers.\n\nIn summary, I think that we need a big motivation to make the jump from\nv1 to v2, instead of merely making small changes to v1 (and I do think\nthat the proposed new commands, such as \"ls-refs\" and \"fetch-object\",\ncan be implemented merely by small changes). And I think that the\nability to better share code between HTTP(S) and others provides that\nmotivation.\n"},{"id":"334111","messageId":"20171204235823.63299-1-bmwill@google.com","threadId":"47006","inReplyTo":"20171020171839.4188-1-bmwill@google.com","subject":"[WIP 00/15] protocol version 2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-04T23:58:08Z","receivedAt":"2017-12-04T23:59:11Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"A while back I sent out a rough outline for what a protocol version 2 would\nlook like at\nhttps://public-inbox.org/git/20171020171839.4188-1-bmwill@google.com/. After\nhacking at both the server and client code I've managed to get some patches for\nprotocol version 2 which implement listing refs and cloning/fetching working.\nI still need to get the push side of things working before this would be viable\nto accept and apply but I figured this was in a good enough state to start\ngetting some early feedback on the concept. I'm hoping that some comments and\nother pairs of eyes can help me identify deficiencies earlier rather than\nlater.\n\nAs a whole, fetch works fairly similar to the way it works in v1 except I\nremoved the support for shallow clients for the time being in order to make it\neasier to implement at first. I haven't decided if it makes more sense to have\nthe deepening as a separate server command or to keep it in fetch like it is in\nv1, just something to think about.\n\nAny comments or criticism is welcome. Thanks!\n\nBrandon Williams (15):\n pkt-line: introduce packet_read_with_status\n pkt-line: introduce struct packet_reader\n pkt-line: add delim packet support\n upload-pack: convert to a builtin\n upload-pack: factor out processing lines\n transport: use get_refs_via_connect to get refs\n connect: convert get_remote_heads to use struct packet_reader\n connect: discover protocol version outside of get_remote_heads\n transport: store protocol version\n protocol: introduce enum protocol_version value protocol_v2\n serve: introduce git-serve\n ls-refs: introduce ls-refs server command\n connect: request remote refs using v2\n upload_pack: introduce fetch server command\n fetch-pack: perform a fetch using v2\n\n .gitignore | 1 +\n Makefile | 6 +-\n builtin.h | 2 +\n builtin/fetch-pack.c | 19 ++-\n builtin/receive-pack.c | 3 +\n builtin/send-pack.c | 18 ++-\n builtin/serve.c | 25 ++++\n connect.c | 210 +++++++++++++++++++++---------\n connect.h | 3 +\n fetch-pack.c | 237 +++++++++++++++++++++++++++++++++-\n fetch-pack.h | 4 +-\n git.c | 2 +\n ls-refs.c | 96 ++++++++++++++\n ls-refs.h | 9 ++\n pkt-line.c | 133 +++++++++++++++++--\n pkt-line.h | 31 +++++\n protocol.c | 2 +\n protocol.h | 1 +\n remote-curl.c | 21 ++-\n remote.h | 7 +-\n serve.c | 195 ++++++++++++++++++++++++++++\n serve.h | 6 +\n t/t5701-protocol-v2.sh | 54 ++++++++\n transport.c | 84 ++++++++----\n upload-pack.c | 342 +++++++++++++++++++++++++++++++++++++++++++------\n upload-pack.h | 9 ++\n 26 files changed, 1371 insertions(+), 149 deletions(-)\n create mode 100644 builtin/serve.c\n create mode 100644 ls-refs.c\n create mode 100644 ls-refs.h\n create mode 100644 serve.c\n create mode 100644 serve.h\n create mode 100755 t/t5701-protocol-v2.sh\n create mode 100644 upload-pack.h\n\n-- \n2.15.1.424.g9478a66081-goog\n\n"},{"id":"334112","messageId":"20171204235823.63299-2-bmwill@google.com","threadId":"47006","inReplyTo":"20171204235823.63299-1-bmwill@google.com","subject":"[WIP 01/15] pkt-line: introduce packet_read_with_status","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-04T23:58:09Z","receivedAt":"2017-12-04T23:59:18Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"The current pkt-line API encodes the status of a pkt-line read in the\nlength of the read content. An error is indicated with '-1', a flush\nwith '0' (which can be confusing since a return value of '0' can also\nindicate an empty pkt-line), and a positive integer for the length of\nthe read content otherwise. This doesn't leave much room for allowing\nthe addition of additional special packets in the future.\n\nTo solve this introduce 'packet_read_with_status()' which reads a packet\nand returns the status of the read encoded as an 'enum packet_status'\ntype. This allows for easily identifying between special and normal\npackets as well as errors. It also enables easily adding a new special\npacket in the future.\n\nSigned-off-by: Brandon Williams \n---\n pkt-line.c | 55 ++++++++++++++++++++++++++++++++++++++++++-------------\n pkt-line.h | 8 ++++++++\n 2 files changed, 50 insertions(+), 13 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 7006b3587..ac619f05b 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -280,28 +280,33 @@ static int packet_length(const char *linelen)\n \treturn (val < 0) ? val : (val << 8) | hex2chr(linelen + 2);\n }\n \n-int packet_read(int fd, char **src_buf, size_t *src_len,\n-\t\tchar *buffer, unsigned size, int options)\n+enum packet_read_status packet_read_with_status(int fd, char **src_buffer, size_t *src_len,\n+\t\t\t\t\t\tchar *buffer, unsigned size, int *pktlen,\n+\t\t\t\t\t\tint options)\n {\n-\tint len, ret;\n+\tint len;\n \tchar linelen[4];\n \n-\tret = get_packet_data(fd, src_buf, src_len, linelen, 4, options);\n-\tif (ret < 0)\n-\t\treturn ret;\n+\tif (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0)\n+\t\treturn PACKET_READ_ERROR;\n+\n \tlen = packet_length(linelen);\n \tif (len < 0)\n \t\tdie(\"protocol error: bad line length character: %.4s\", linelen);\n-\tif (!len) {\n+\n+\tif (len == 0) {\n \t\tpacket_trace(\"0000\", 4, 0);\n-\t\treturn 0;\n+\t\treturn PACKET_READ_FLUSH;\n+\t} else if (len >= 1 && len <= 3) {\n+\t\tdie(\"protocol error: bad line length character: %.4s\", linelen);\n \t}\n+\n \tlen -= 4;\n-\tif (len >= size)\n+\tif ((len < 0) || ((unsigned)len >= size))\n \t\tdie(\"protocol error: bad line length %d\", len);\n-\tret = get_packet_data(fd, src_buf, src_len, buffer, len, options);\n-\tif (ret < 0)\n-\t\treturn ret;\n+\n+\tif (get_packet_data(fd, src_buffer, src_len, buffer, len, options) < 0)\n+\t\treturn PACKET_READ_ERROR;\n \n \tif ((options & PACKET_READ_CHOMP_NEWLINE) &&\n \t len && buffer[len-1] == '\\n')\n@@ -309,7 +314,31 @@ int packet_read(int fd, char **src_buf, size_t *src_len,\n \n \tbuffer[len] = 0;\n \tpacket_trace(buffer, len, 0);\n-\treturn len;\n+\t*pktlen = len;\n+\treturn PACKET_READ_NORMAL;\n+}\n+\n+int packet_read(int fd, char **src_buffer, size_t *src_len,\n+\t\tchar *buffer, unsigned size, int options)\n+{\n+\tenum packet_read_status status;\n+\tint pktlen;\n+\n+\tstatus = packet_read_with_status(fd, src_buffer, src_len,\n+\t\t\t\t\t buffer, size, &pktlen,\n+\t\t\t\t\t options);\n+\tswitch (status) {\n+\tcase PACKET_READ_ERROR:\n+\t\tpktlen = -1;\n+\t\tbreak;\n+\tcase PACKET_READ_NORMAL:\n+\t\tbreak;\n+\tcase PACKET_READ_FLUSH:\n+\t\tpktlen = 0;\n+\t\tbreak;\n+\t}\n+\n+\treturn pktlen;\n }\n \n static char *packet_read_line_generic(int fd,\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 3dad583e2..f1545929b 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -60,8 +60,16 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);\n * If options contains PACKET_READ_CHOMP_NEWLINE, a trailing newline (if\n * present) is removed from the buffer before returning.\n */\n+enum packet_read_status {\n+\tPACKET_READ_ERROR = -1,\n+\tPACKET_READ_NORMAL,\n+\tPACKET_READ_FLUSH,\n+};\n #define PACKET_READ_GENTLE_ON_EOF (1u<<0)\n #define PACKET_READ_CHOMP_NEWLINE (1u<<1)\n+enum packet_read_status packet_read_with_status(int fd, char **src_buffer, size_t *src_len,\n+\t\t\t\t\t\tchar *buffer, unsigned size, int *pktlen,\n+\t\t\t\t\t\tint options);\n int packet_read(int fd, char **src_buffer, size_t *src_len, char\n \t\t*buffer, unsigned size, int options);\n \n-- \n2.15.1.424.g9478a66081-goog\n\n"},{"id":"334113","messageId":"20171204235823.63299-3-bmwill@google.com","threadId":"47006","inReplyTo":"20171204235823.63299-1-bmwill@google.com","subject":"[WIP 02/15] pkt-line: introduce struct packet_reader","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-04T23:58:10Z","receivedAt":"2017-12-04T23:59:21Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Sometimes it is advantageous to be able to peek the next packet line\nwithout consuming it (e.g. to be able to determine the protocol version\na server is speaking). In order to do that introduce 'struct\npacket_reader' which is an abstraction around the normal packet reading\nlogic. This enables a caller to be able to peek a single line at a time\nusing 'packet_reader_peek()' and having a caller consume a line by\ncalling 'packet_reader_read()'.\n\nSigned-off-by: Brandon Williams \n---\n pkt-line.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n pkt-line.h | 20 ++++++++++++++++++++\n 2 files changed, 81 insertions(+)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex ac619f05b..518109bbe 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -406,3 +406,64 @@ ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out)\n \t}\n \treturn sb_out->len - orig_len;\n }\n+\n+/* Packet Reader Functions */\n+void packet_reader_init(struct packet_reader *reader, int fd,\n+\t\t\tchar *src_buffer, size_t src_len)\n+{\n+\treader->fd = fd;\n+\treader->src_buffer = src_buffer;\n+\treader->src_len = src_len;\n+\treader->buffer = packet_buffer;\n+\treader->buffer_size = sizeof(packet_buffer);\n+\treader->options = PACKET_READ_CHOMP_NEWLINE | PACKET_READ_GENTLE_ON_EOF;\n+\n+\treader->line = NULL;\n+\treader->line_peeked = 0;\n+\treader->pktlen = 0;\n+\treader->status = PACKET_READ_ERROR;\n+}\n+\n+enum packet_read_status packet_reader_read(struct packet_reader *reader)\n+{\n+\tif (reader->line_peeked) {\n+\t\treader->line_peeked = 0;\n+\t\treturn reader->status;\n+\t}\n+\n+\treader->status = packet_read_with_status(reader->fd,\n+\t\t\t\t\t\t &reader->src_buffer,\n+\t\t\t\t\t\t &reader->src_len,\n+\t\t\t\t\t\t reader->buffer,\n+\t\t\t\t\t\t reader->buffer_size,\n+\t\t\t\t\t\t &reader->pktlen,\n+\t\t\t\t\t\t reader->options);\n+\n+\tswitch (reader->status) {\n+\tcase PACKET_READ_ERROR:\n+\t\treader->pktlen = -1;\n+\t\treader->line = NULL;\n+\t\tbreak;\n+\tcase PACKET_READ_NORMAL:\n+\t\treader->line = reader->buffer;\n+\t\tbreak;\n+\tcase PACKET_READ_FLUSH:\n+\t\treader->pktlen = 0;\n+\t\treader->line = NULL;\n+\t\tbreak;\n+\t}\n+\n+\treturn reader->status;\n+}\n+\n+enum packet_read_status packet_reader_peek(struct packet_reader *reader)\n+{\n+\t/* Only allow peeking a single line */\n+\tif (reader->line_peeked)\n+\t\treturn reader->status;\n+\n+\t/* Peek a line by reading it and setting peeked flag */\n+\tpacket_reader_read(reader);\n+\treader->line_peeked = 1;\n+\treturn reader->status;\n+}\ndiff --git a/pkt-line.h b/pkt-line.h\nindex f1545929b..2b5c7cf11 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -104,6 +104,26 @@ char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);\n */\n ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out);\n \n+struct packet_reader {\n+\tint fd;\n+\tchar *src_buffer;\n+\tsize_t src_len;\n+\n+\tchar *buffer;\n+\tunsigned buffer_size;\n+\tint options;\n+\n+\tenum packet_read_status status;\n+\tint pktlen;\n+\tconst char *line;\n+\tint line_peeked;\n+};\n+\n+extern void packet_reader_init(struct packet_reader *reader, int fd,\n+\t\t\t char *src_buffer, size_t src_len);\n+extern enum packet_read_status packet_reader_read(struct packet_reader *reader);\n+extern enum packet_read_status packet_reader_peek(struct packet_reader *reader);\n+\n #define DEFAULT_PACKET_MAX 1000\n #define LARGE_PACKET_MAX 65520\n #define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)\n-- \n2.15.1.424.g9478a66081-goog\n\n"},{"id":"334114","messageId":"20171204235823.63299-4-bmwill@google.com","threadId":"47006","inReplyTo":"20171204235823.63299-1-bmwill@google.com","subject":"[WIP 03/15] pkt-line: add delim packet support","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-04T23:58:11Z","receivedAt":"2017-12-04T23:59:24Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"One of the design goals of protocol-v2 is to improve the semantics of\nflush packets. Currently in protocol-v1, flush packets are used both to\nindicate a break in a list of packet lines as well as an indication that\none side has finished speaking. This makes it particularly difficult\nto implement proxies as a proxy would need to completely understand git\nprotocol instead of simply looking for a flush packet.\n\nTo do this, introduce the special deliminator packet '0001'. A delim\npacket can then be used as a deliminator between lists of packet lines\nwhile flush packets can be reserved to indicate the end of a response.\n\nSigned-off-by: Brandon Williams \n---\n pkt-line.c | 19 ++++++++++++++++++-\n pkt-line.h | 3 +++\n 2 files changed, 21 insertions(+), 1 deletion(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 518109bbe..222e1e310 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -91,6 +91,12 @@ void packet_flush(int fd)\n \twrite_or_die(fd, \"0000\", 4);\n }\n \n+void packet_delim(int fd)\n+{\n+\tpacket_trace(\"0001\", 4, 1);\n+\twrite_or_die(fd, \"0001\", 4);\n+}\n+\n int packet_flush_gently(int fd)\n {\n \tpacket_trace(\"0000\", 4, 1);\n@@ -105,6 +111,12 @@ void packet_buf_flush(struct strbuf *buf)\n \tstrbuf_add(buf, \"0000\", 4);\n }\n \n+void packet_buf_delim(struct strbuf *buf)\n+{\n+\tpacket_trace(\"0001\", 4, 1);\n+\tstrbuf_add(buf, \"0001\", 4);\n+}\n+\n static void set_packet_header(char *buf, const int size)\n {\n \tstatic char hexchar[] = \"0123456789abcdef\";\n@@ -297,7 +309,10 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer, size_\n \tif (len == 0) {\n \t\tpacket_trace(\"0000\", 4, 0);\n \t\treturn PACKET_READ_FLUSH;\n-\t} else if (len >= 1 && len <= 3) {\n+\t} else if (len == 1) {\n+\t\tpacket_trace(\"0001\", 4, 0);\n+\t\treturn PACKET_READ_DELIM;\n+\t} else if (len >= 2 && len <= 3) {\n \t\tdie(\"protocol error: bad line length character: %.4s\", linelen);\n \t}\n \n@@ -333,6 +348,7 @@ int packet_read(int fd, char **src_buffer, size_t *src_len,\n \t\tbreak;\n \tcase PACKET_READ_NORMAL:\n \t\tbreak;\n+\tcase PACKET_READ_DELIM:\n \tcase PACKET_READ_FLUSH:\n \t\tpktlen = 0;\n \t\tbreak;\n@@ -447,6 +463,7 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)\n \tcase PACKET_READ_NORMAL:\n \t\treader->line = reader->buffer;\n \t\tbreak;\n+\tcase PACKET_READ_DELIM:\n \tcase PACKET_READ_FLUSH:\n \t\treader->pktlen = 0;\n \t\treader->line = NULL;\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 2b5c7cf11..49ec80c80 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -20,8 +20,10 @@\n * side can't, we stay with pure read/write interfaces.\n */\n void packet_flush(int fd);\n+void packet_delim(int fd);\n void packet_write_fmt(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n void packet_buf_flush(struct strbuf *buf);\n+void packet_buf_delim(struct strbuf *buf);\n void packet_write(int fd_out, const char *buf, size_t size);\n void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n int packet_flush_gently(int fd);\n@@ -64,6 +66,7 @@ enum packet_read_status {\n \tPACKET_READ_ERROR = -1,\n \tPACKET_READ_NORMAL,\n \tPACKET_READ_FLUSH,\n+\tPACKET_READ_DELIM,\n };\n #define PACKET_READ_GENTLE_ON_EOF (1u<<0)\n #define PACKET_READ_CHOMP_NEWLINE (1u<<1)\n-- \n2.15.1.424.g9478a66081-goog\n\n"},{"id":"334115","messageId":"20171204235823.63299-5-bmwill@google.com","threadId":"47006","inReplyTo":"20171204235823.63299-1-bmwill@google.com","subject":"[WIP 04/15] upload-pack: convert to a builtin","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-04T23:58:12Z","receivedAt":"2017-12-04T23:59:25Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"In order to allow for code sharing with the server-side of fetch in\nprotocol-v2 convert upload-pack to be a builtin.\n\nSigned-off-by: Brandon Williams \n---\n Makefile | 3 ++-\n builtin.h | 1 +\n git.c | 1 +\n upload-pack.c | 2 +-\n 4 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 9ce68cded..86394b69d 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -630,7 +630,6 @@ PROGRAM_OBJS += imap-send.o\n PROGRAM_OBJS += sh-i18n--envsubst.o\n PROGRAM_OBJS += shell.o\n PROGRAM_OBJS += show-index.o\n-PROGRAM_OBJS += upload-pack.o\n PROGRAM_OBJS += remote-testsvn.o\n \n # Binary suffix, set to .exe for Windows builds\n@@ -692,6 +691,7 @@ BUILT_INS += git-merge-subtree$X\n BUILT_INS += git-show$X\n BUILT_INS += git-stage$X\n BUILT_INS += git-status$X\n+BUILT_INS += git-upload-pack$X\n BUILT_INS += git-whatchanged$X\n \n # what 'all' will build and 'install' will install in gitexecdir,\n@@ -890,6 +890,7 @@ LIB_OBJS += tree-diff.o\n LIB_OBJS += tree.o\n LIB_OBJS += tree-walk.o\n LIB_OBJS += unpack-trees.o\n+LIB_OBJS += upload-pack.o\n LIB_OBJS += url.o\n LIB_OBJS += urlmatch.o\n LIB_OBJS += usage.o\ndiff --git a/builtin.h b/builtin.h\nindex 42378f3aa..f332a1257 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -231,6 +231,7 @@ extern int cmd_update_ref(int argc, const char **argv, const char *prefix);\n extern int cmd_update_server_info(int argc, const char **argv, const char *prefix);\n extern int cmd_upload_archive(int argc, const char **argv, const char *prefix);\n extern int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix);\n+extern int cmd_upload_pack(int argc, const char **argv, const char *prefix);\n extern int cmd_var(int argc, const char **argv, const char *prefix);\n extern int cmd_verify_commit(int argc, const char **argv, const char *prefix);\n extern int cmd_verify_tag(int argc, const char **argv, const char *prefix);\ndiff --git a/git.c b/git.c\nindex f31dca696..e32e16f2d 100644\n--- a/git.c\n+++ b/git.c\n@@ -474,6 +474,7 @@ static struct cmd_struct commands[] = {\n \t{ \"update-server-info\", cmd_update_server_info, RUN_SETUP },\n \t{ \"upload-archive\", cmd_upload_archive },\n \t{ \"upload-archive--writer\", cmd_upload_archive_writer },\n+\t{ \"upload-pack\", cmd_upload_pack },\n \t{ \"var\", cmd_var, RUN_SETUP_GENTLY },\n \t{ \"verify-commit\", cmd_verify_commit, RUN_SETUP },\n \t{ \"verify-pack\", cmd_verify_pack },\ndiff --git a/upload-pack.c b/upload-pack.c\nindex ef99a029c..2d16952a3 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1033,7 +1033,7 @@ static int upload_pack_config(const char *var, const char *value, void *unused)\n \treturn parse_hide_refs_config(var, value, \"uploadpack\");\n }\n \n-int cmd_main(int argc, const char **argv)\n+int cmd_upload_pack(int argc, const char **argv, const char *prefix)\n {\n \tconst char *dir;\n \tint strict = 0;\n-- \n2.15.1.424.g9478a66081-goog\n\n"},{"id":"334116","messageId":"20171204235823.63299-8-bmwill@google.com","threadId":"47006","inReplyTo":"20171204235823.63299-1-bmwill@google.com","subject":"[WIP 07/15] connect: convert get_remote_heads to use struct packet_reader","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-04T23:58:15Z","receivedAt":"2017-12-04T23:59:27Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"In order to allow for better control flow when protocol_v2 is introduced\nconvert 'get_remote_heads()' to use 'struct packet_reader' to read\npacket lines. This enables a client to be able to peek the first line\nof a server's response (without consuming it) in order to determine the\nprotocol version its speaking and then passing control to the\nappropriate handler.\n\nThis is needed because the initial response from a server speaking\nprotocol_v0 includes the first ref, while subsequent protocol versions\nrespond with a version line. We want to be able to read this first line\nwithout consuming the first ref sent in the protocol_v0 case so that the\nprotocol version the server is speaking can be determined outside of\n'get_remote_heads()' in a future patch.\n\nSigned-off-by: Brandon Williams \n---\n connect.c | 125 +++++++++++++++++++++++++++++++++++---------------------------\n 1 file changed, 70 insertions(+), 55 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 7fbd396b3..f79ea9179 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -48,6 +48,12 @@ int check_ref_type(const struct ref *ref, int flags)\n \n static void die_initial_contact(int unexpected)\n {\n+\t/*\n+\t * A hang-up after seeing some response from the other end\n+\t * means that it is unexpected, as we know the other end is\n+\t * willing to talk to us. A hang-up before seeing any\n+\t * response does not necessarily mean an ACL problem, though.\n+\t */\n \tif (unexpected)\n \t\tdie(_(\"The remote end hung up upon initial contact\"));\n \telse\n@@ -56,6 +62,41 @@ static void die_initial_contact(int unexpected)\n \t\t \"and the repository exists.\"));\n }\n \n+static enum protocol_version discover_version(struct packet_reader *reader)\n+{\n+\tenum protocol_version version = protocol_unknown_version;\n+\n+\t/*\n+\t * Peek the first line of the server's response to\n+\t * determine the protocol version the server is speaking.\n+\t */\n+\tswitch (packet_reader_peek(reader)) {\n+\tcase PACKET_READ_ERROR:\n+\t\tdie_initial_contact(0);\n+\tcase PACKET_READ_FLUSH:\n+\tcase PACKET_READ_DELIM:\n+\t\tversion = protocol_v0;\n+\t\tbreak;\n+\tcase PACKET_READ_NORMAL:\n+\t\tversion = determine_protocol_version_client(reader->line);\n+\t\tbreak;\n+\t}\n+\n+\t/* Maybe process capabilities here, at least for v2 */\n+\tswitch (version) {\n+\tcase protocol_v1:\n+\t\t/* Read the peeked version line */\n+\t\tpacket_reader_read(reader);\n+\t\tbreak;\n+\tcase protocol_v0:\n+\t\tbreak;\n+\tcase protocol_unknown_version:\n+\t\tBUG(\"ERROR\");\n+\t}\n+\n+\treturn version;\n+}\n+\n static void parse_one_symref_info(struct string_list *symref, const char *val, int len)\n {\n \tchar *sym, *target;\n@@ -109,44 +150,10 @@ static void annotate_refs_with_symref_info(struct ref *ref)\n \tstring_list_clear(&symref, 0);\n }\n \n-/*\n- * Read one line of a server's ref advertisement into packet_buffer.\n- */\n-static int read_remote_ref(int in, char **src_buf, size_t *src_len,\n-\t\t\t int *responded)\n-{\n-\tint len = packet_read(in, src_buf, src_len,\n-\t\t\t packet_buffer, sizeof(packet_buffer),\n-\t\t\t PACKET_READ_GENTLE_ON_EOF |\n-\t\t\t PACKET_READ_CHOMP_NEWLINE);\n-\tconst char *arg;\n-\tif (len < 0)\n-\t\tdie_initial_contact(*responded);\n-\tif (len > 4 && skip_prefix(packet_buffer, \"ERR \", &arg))\n-\t\tdie(\"remote error: %s\", arg);\n-\n-\t*responded = 1;\n-\n-\treturn len;\n-}\n-\n-#define EXPECTING_PROTOCOL_VERSION 0\n-#define EXPECTING_FIRST_REF 1\n-#define EXPECTING_REF 2\n-#define EXPECTING_SHALLOW 3\n-\n-/* Returns 1 if packet_buffer is a protocol version pkt-line, 0 otherwise. */\n-static int process_protocol_version(void)\n-{\n-\tswitch (determine_protocol_version_client(packet_buffer)) {\n-\tcase protocol_v1:\n-\t\treturn 1;\n-\tcase protocol_v0:\n-\t\treturn 0;\n-\tdefault:\n-\t\tdie(\"server is speaking an unknown protocol\");\n-\t}\n-}\n+#define EXPECTING_FIRST_REF 0\n+#define EXPECTING_REF 1\n+#define EXPECTING_SHALLOW 2\n+#define EXPECTING_DONE 3\n \n static void process_capabilities(int *len)\n {\n@@ -230,28 +237,34 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \t\t\t struct oid_array *shallow_points)\n {\n \tstruct ref **orig_list = list;\n+\tint len = 0;\n+\tint state = EXPECTING_FIRST_REF;\n+\tstruct packet_reader reader;\n+\tconst char *arg;\n \n-\t/*\n-\t * A hang-up after seeing some response from the other end\n-\t * means that it is unexpected, as we know the other end is\n-\t * willing to talk to us. A hang-up before seeing any\n-\t * response does not necessarily mean an ACL problem, though.\n-\t */\n-\tint responded = 0;\n-\tint len;\n-\tint state = EXPECTING_PROTOCOL_VERSION;\n+\tpacket_reader_init(&reader, in, src_buf, src_len);\n+\n+\tdiscover_version(&reader);\n \n \t*list = NULL;\n \n-\twhile ((len = read_remote_ref(in, &src_buf, &src_len, &responded))) {\n+\twhile (state != EXPECTING_DONE) {\n+\t\tswitch (packet_reader_read(&reader)) {\n+\t\tcase PACKET_READ_ERROR:\n+\t\t\tdie_initial_contact(1);\n+\t\tcase PACKET_READ_NORMAL:\n+\t\t\tlen = reader.pktlen;\n+\t\t\tif (len > 4 && skip_prefix(packet_buffer, \"ERR \", &arg))\n+\t\t\t\tdie(\"remote error: %s\", arg);\n+\t\t\tbreak;\n+\t\tcase PACKET_READ_FLUSH:\n+\t\t\tstate = EXPECTING_DONE;\n+\t\t\tbreak;\n+\t\tcase PACKET_READ_DELIM:\n+\t\t\tdie(\"invalid packet\\n\");\n+\t\t}\n+\n \t\tswitch (state) {\n-\t\tcase EXPECTING_PROTOCOL_VERSION:\n-\t\t\tif (process_protocol_version()) {\n-\t\t\t\tstate = EXPECTING_FIRST_REF;\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\t\tstate = EXPECTING_FIRST_REF;\n-\t\t\t/* fallthrough */\n \t\tcase EXPECTING_FIRST_REF:\n \t\t\tprocess_capabilities(&len);\n \t\t\tif (process_dummy_ref()) {\n@@ -269,6 +282,8 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \t\t\tif (process_shallow(len, shallow_points))\n \t\t\t\tbreak;\n \t\t\tdie(\"protocol error: unexpected '%s'\", packet_buffer);\n+\t\tcase EXPECTING_DONE:\n+\t\t\tbreak;\n \t\tdefault:\n \t\t\tdie(\"unexpected state %d\", state);\n \t\t}\n-- \n2.15.1.424.g9478a66081-goog\n\n"},{"id":"334117","messageId":"20171204235823.63299-7-bmwill@google.com","threadId":"47006","inReplyTo":"20171204235823.63299-1-bmwill@google.com","subject":"[WIP 06/15] transport: use get_refs_via_connect to get refs","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-04T23:58:14Z","receivedAt":"2017-12-04T23:59:30Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Remove code duplication and use the existing 'get_refs_via_connect()'\nfunction to retrieve a remote's heads in 'fetch_refs_via_pack()' and\n'git_transport_push()'.\n\nSigned-off-by: Brandon Williams \n---\n transport.c | 18 ++++--------------\n 1 file changed, 4 insertions(+), 14 deletions(-)\n\ndiff --git a/transport.c b/transport.c\nindex d75ff0514..7c969f285 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -230,12 +230,8 @@ static int fetch_refs_via_pack(struct transport *transport,\n \targs.cloning = transport->cloning;\n \targs.update_shallow = data->options.update_shallow;\n \n-\tif (!data->got_remote_heads) {\n-\t\tconnect_setup(transport, 0);\n-\t\tget_remote_heads(data->fd[0], NULL, 0, &refs_tmp, 0,\n-\t\t\t\t NULL, &data->shallow);\n-\t\tdata->got_remote_heads = 1;\n-\t}\n+\tif (!data->got_remote_heads)\n+\t\trefs_tmp = get_refs_via_connect(transport, 0);\n \n \trefs = fetch_pack(&args, data->fd, data->conn,\n \t\t\t refs_tmp ? refs_tmp : transport->remote_refs,\n@@ -542,14 +538,8 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n \tstruct send_pack_args args;\n \tint ret;\n \n-\tif (!data->got_remote_heads) {\n-\t\tstruct ref *tmp_refs;\n-\t\tconnect_setup(transport, 1);\n-\n-\t\tget_remote_heads(data->fd[0], NULL, 0, &tmp_refs, REF_NORMAL,\n-\t\t\t\t NULL, &data->shallow);\n-\t\tdata->got_remote_heads = 1;\n-\t}\n+\tif (!data->got_remote_heads)\n+\t\tget_refs_via_connect(transport, 1);\n \n \tmemset(&args, 0, sizeof(args));\n \targs.send_mirror = !!(flags & TRANSPORT_PUSH_MIRROR);\n-- \n2.15.1.424.g9478a66081-goog\n\n"},{"id":"334118","messageId":"20171204235823.63299-9-bmwill@google.com","threadId":"47006","inReplyTo":"20171204235823.63299-1-bmwill@google.com","subject":"[WIP 08/15] connect: discover protocol version outside of get_remote_heads","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-04T23:58:16Z","receivedAt":"2017-12-04T23:59:32Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"In order to prepare for the addition of protocol_v2 push the protocol\nversion discovery outside of 'get_remote_heads()'. This will allow for\nkeeping the logic for processing the reference advertisement for\nprotocol_v1 and protocol_v0 separate from the logic for protocol_v2.\n\nSigned-off-by: Brandon Williams \n---\n builtin/fetch-pack.c | 14 +++++++++++++-\n builtin/send-pack.c | 15 +++++++++++++--\n connect.c | 13 ++++---------\n connect.h | 3 +++\n remote-curl.c | 18 ++++++++++++++++--\n remote.h | 5 +++--\n transport.c | 22 +++++++++++++++++-----\n 7 files changed, 69 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 366b9d13f..4873e9572 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -4,6 +4,7 @@\n #include \"remote.h\"\n #include \"connect.h\"\n #include \"sha1-array.h\"\n+#include \"protocol.h\"\n \n static const char fetch_pack_usage[] =\n \"git fetch-pack [--all] [--stdin] [--quiet | -q] [--keep | -k] [--thin] \"\n@@ -52,6 +53,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \tstruct fetch_pack_args args;\n \tstruct oid_array shallow = OID_ARRAY_INIT;\n \tstruct string_list deepen_not = STRING_LIST_INIT_DUP;\n+\tstruct packet_reader reader;\n \n \tpacket_trace_identity(\"fetch-pack\");\n \n@@ -193,7 +195,17 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\tif (!conn)\n \t\t\treturn args.diag_url ? 0 : 1;\n \t}\n-\tget_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);\n+\n+\tpacket_reader_init(&reader, fd[0], NULL, 0);\n+\n+\tswitch (discover_version(&reader)) {\n+\tcase protocol_v1:\n+\tcase protocol_v0:\n+\t\tget_remote_heads(&reader, &ref, 0, NULL, &shallow);\n+\t\tbreak;\n+\tcase protocol_unknown_version:\n+\t\tBUG(\"unknown protocol version\");\n+\t}\n \n \tref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought,\n \t\t\t &shallow, pack_lockfile_ptr);\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex fc4f0bb5f..9c2ca80c8 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -14,6 +14,7 @@\n #include \"sha1-array.h\"\n #include \"gpg-interface.h\"\n #include \"gettext.h\"\n+#include \"protocol.h\"\n \n static const char * const send_pack_usage[] = {\n \tN_(\"git send-pack [--all | --mirror] [--dry-run] [--force] \"\n@@ -154,6 +155,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \tint progress = -1;\n \tint from_stdin = 0;\n \tstruct push_cas_option cas = {0};\n+\tstruct packet_reader reader;\n \n \tstruct option options[] = {\n \t\tOPT__VERBOSITY(&verbose),\n@@ -256,8 +258,17 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \t\t\targs.verbose ? CONNECT_VERBOSE : 0);\n \t}\n \n-\tget_remote_heads(fd[0], NULL, 0, &remote_refs, REF_NORMAL,\n-\t\t\t &extra_have, &shallow);\n+\tpacket_reader_init(&reader, fd[0], NULL, 0);\n+\n+\tswitch (discover_version(&reader)) {\n+\tcase protocol_v1:\n+\tcase protocol_v0:\n+\t\tget_remote_heads(&reader, &remote_refs, REF_NORMAL,\n+\t\t\t\t &extra_have, &shallow);\n+\t\tbreak;\n+\tcase protocol_unknown_version:\n+\t\tBUG(\"unknown protocol version\");\n+\t}\n \n \ttransport_verify_remote_names(nr_refspecs, refspecs);\n \ndiff --git a/connect.c b/connect.c\nindex f79ea9179..5f7cf05c7 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -62,7 +62,7 @@ static void die_initial_contact(int unexpected)\n \t\t \"and the repository exists.\"));\n }\n \n-static enum protocol_version discover_version(struct packet_reader *reader)\n+enum protocol_version discover_version(struct packet_reader *reader)\n {\n \tenum protocol_version version = protocol_unknown_version;\n \n@@ -231,7 +231,7 @@ static int process_shallow(int len, struct oid_array *shallow_points)\n /*\n * Read all the refs from the other end\n */\n-struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n+struct ref **get_remote_heads(struct packet_reader *reader,\n \t\t\t struct ref **list, unsigned int flags,\n \t\t\t struct oid_array *extra_have,\n \t\t\t struct oid_array *shallow_points)\n@@ -239,21 +239,16 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n \tstruct ref **orig_list = list;\n \tint len = 0;\n \tint state = EXPECTING_FIRST_REF;\n-\tstruct packet_reader reader;\n \tconst char *arg;\n \n-\tpacket_reader_init(&reader, in, src_buf, src_len);\n-\n-\tdiscover_version(&reader);\n-\n \t*list = NULL;\n \n \twhile (state != EXPECTING_DONE) {\n-\t\tswitch (packet_reader_read(&reader)) {\n+\t\tswitch (packet_reader_read(reader)) {\n \t\tcase PACKET_READ_ERROR:\n \t\t\tdie_initial_contact(1);\n \t\tcase PACKET_READ_NORMAL:\n-\t\t\tlen = reader.pktlen;\n+\t\t\tlen = reader->pktlen;\n \t\t\tif (len > 4 && skip_prefix(packet_buffer, \"ERR \", &arg))\n \t\t\t\tdie(\"remote error: %s\", arg);\n \t\t\tbreak;\ndiff --git a/connect.h b/connect.h\nindex 01f14cdf3..cdb8979dc 100644\n--- a/connect.h\n+++ b/connect.h\n@@ -13,4 +13,7 @@ extern int parse_feature_request(const char *features, const char *feature);\n extern const char *server_feature_value(const char *feature, int *len_ret);\n extern int url_is_local_not_ssh(const char *url);\n \n+struct packet_reader;\n+extern enum protocol_version discover_version(struct packet_reader *reader);\n+\n #endif\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 0053b0954..74c6c3049 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n #include \"config.h\"\n #include \"remote.h\"\n+#include \"connect.h\"\n #include \"strbuf.h\"\n #include \"walker.h\"\n #include \"http.h\"\n@@ -13,6 +14,7 @@\n #include \"credential.h\"\n #include \"sha1-array.h\"\n #include \"send-pack.h\"\n+#include \"protocol.h\"\n \n static struct remote *remote;\n /* always ends with a trailing slash */\n@@ -176,8 +178,20 @@ static struct discovery *last_discovery;\n static struct ref *parse_git_refs(struct discovery *heads, int for_push)\n {\n \tstruct ref *list = NULL;\n-\tget_remote_heads(-1, heads->buf, heads->len, &list,\n-\t\t\t for_push ? REF_NORMAL : 0, NULL, &heads->shallow);\n+\tstruct packet_reader reader;\n+\n+\tpacket_reader_init(&reader, -1, heads->buf, heads->len);\n+\n+\tswitch (discover_version(&reader)) {\n+\tcase protocol_v1:\n+\tcase protocol_v0:\n+\t\tget_remote_heads(&reader, &list, for_push ? REF_NORMAL : 0,\n+\t\t\t\t NULL, &heads->shallow);\n+\t\tbreak;\n+\tcase protocol_unknown_version:\n+\t\tBUG(\"unknown protocol version\");\n+\t}\n+\n \treturn list;\n }\n \ndiff --git a/remote.h b/remote.h\nindex 2ecf4c8c7..af34e44a4 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -150,10 +150,11 @@ int check_ref_type(const struct ref *ref, int flags);\n void free_refs(struct ref *ref);\n \n struct oid_array;\n-extern struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n+struct packet_reader;\n+extern struct ref **get_remote_heads(struct packet_reader *reader,\n \t\t\t\t struct ref **list, unsigned int flags,\n \t\t\t\t struct oid_array *extra_have,\n-\t\t\t\t struct oid_array *shallow);\n+\t\t\t\t struct oid_array *shallow_points);\n \n int resolve_remote_symref(struct ref *ref, struct ref *list);\n int ref_newer(const struct object_id *new_oid, const struct object_id *old_oid);\ndiff --git a/transport.c b/transport.c\nindex 7c969f285..28b744ec2 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -17,6 +17,7 @@\n #include \"string-list.h\"\n #include \"sha1-array.h\"\n #include \"sigchain.h\"\n+#include \"protocol.h\"\n \n static void set_upstreams(struct transport *transport, struct ref *refs,\n \tint pretend)\n@@ -190,13 +191,24 @@ static int connect_setup(struct transport *transport, int for_push)\n static struct ref *get_refs_via_connect(struct transport *transport, int for_push)\n {\n \tstruct git_transport_data *data = transport->data;\n-\tstruct ref *refs;\n+\tstruct ref *refs = NULL;\n+\tstruct packet_reader reader;\n \n \tconnect_setup(transport, for_push);\n-\tget_remote_heads(data->fd[0], NULL, 0, &refs,\n-\t\t\t for_push ? REF_NORMAL : 0,\n-\t\t\t &data->extra_have,\n-\t\t\t &data->shallow);\n+\n+\tpacket_reader_init(&reader, data->fd[0], NULL, 0);\n+\n+\tswitch (discover_version(&reader)) {\n+\tcase protocol_v1:\n+\tcase protocol_v0:\n+\t\tget_remote_heads(&reader, &refs,\n+\t\t\t\t for_push ? REF_NORMAL : 0,\n+\t\t\t\t &data->extra_have,\n+\t\t\t\t &data->shallow);\n+\t\tbreak;\n+\tcase protocol_unknown_version:\n+\t\tBUG(\"unknown protocol version\");\n+\t}\n \tdata->got_remote_heads = 1;\n \n \treturn refs;\n-- \n2.15.1.424.g9478a66081-goog\n\n"},{"id":"334119","messageId":"20171204235823.63299-14-bmwill@google.com","threadId":"47006","inReplyTo":"20171204235823.63299-1-bmwill@google.com","subject":"[WIP 13/15] connect: request remote refs using v2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-04T23:58:21Z","receivedAt":"2017-12-04T23:59:35Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Teach the client to be able to request a remote's refs using protocol\nv2. This is done by having a client issue a 'ls-refs' request to a v2\nserver.\n\nSigned-off-by: Brandon Williams \n---\n connect.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++-\n remote.h | 2 ++\n t/t5701-protocol-v2.sh | 28 +++++++++++++++++\n transport.c | 2 +-\n upload-pack.c | 3 +-\n 5 files changed, 117 insertions(+), 3 deletions(-)\n create mode 100755 t/t5701-protocol-v2.sh\n\ndiff --git a/connect.c b/connect.c\nindex 433f08649..b3c933fc1 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -15,6 +15,7 @@\n #include \"protocol.h\"\n \n static char *server_capabilities;\n+static struct argv_array server_capabilities_v2 = ARGV_ARRAY_INIT;\n static const char *parse_feature_value(const char *, const char *, int *);\n \n static int check_ref(const char *name, unsigned int flags)\n@@ -62,6 +63,30 @@ static void die_initial_contact(int unexpected)\n \t\t \"and the repository exists.\"));\n }\n \n+static int server_supports_v2(const char *c)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < server_capabilities_v2.argc; i++) {\n+\t\tconst char *out;\n+\t\tif (skip_prefix(server_capabilities_v2.argv[i], c, &out) &&\n+\t\t (!*out || *out == '='))\n+\t\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n+static void process_capabilities_v2(struct packet_reader *reader)\n+{\n+\twhile (packet_reader_read(reader) == PACKET_READ_NORMAL) {\n+\t\targv_array_push(&server_capabilities_v2, reader->line);\n+\t}\n+\n+\tif (reader->status != PACKET_READ_FLUSH)\n+\t\tdie(\"protocol error\");\n+}\n+\n enum protocol_version discover_version(struct packet_reader *reader)\n {\n \tenum protocol_version version = protocol_unknown_version;\n@@ -85,7 +110,7 @@ enum protocol_version discover_version(struct packet_reader *reader)\n \t/* Maybe process capabilities here, at least for v2 */\n \tswitch (version) {\n \tcase protocol_v2:\n-\t\tdie(\"support for protocol v2 not implemented yet\");\n+\t\tprocess_capabilities_v2(reader);\n \t\tbreak;\n \tcase protocol_v1:\n \t\t/* Read the peeked version line */\n@@ -292,6 +317,64 @@ struct ref **get_remote_heads(struct packet_reader *reader,\n \treturn list;\n }\n \n+static int process_ref_v2(const char *line, struct ref ***list)\n+{\n+\tstruct object_id old_oid;\n+\tconst char *end_of_name;\n+\tstruct ref *ref;\n+\n+\tif (parse_oid_hex(line, &old_oid, &line))\n+\t\treturn 0;\n+\tif (*line != ' ')\n+\t\treturn 0;\n+\tline++;\n+\n+\tend_of_name = strchr(line, ' ');\n+\n+\tif (!end_of_name)\n+\t\tref = alloc_ref(line);\n+\telse {\n+\t\tstruct strbuf name = STRBUF_INIT;\n+\t\t/* symref info */\n+\t\tstrbuf_add(&name, line, end_of_name - line);\n+\t\tref = alloc_ref(name.buf);\n+\t\tref->symref = xstrdup(end_of_name + 1);\n+\n+\t\tstrbuf_release(&name);\n+\t}\n+\n+\toidcpy(&ref->old_oid, &old_oid);\n+\t**list = ref;\n+\t*list = &ref->next;\n+\n+\treturn 1;\n+}\n+struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,\n+\t\t\t struct ref **list, unsigned int flags)\n+{\n+\t*list = NULL;\n+\t/* Check that the server supports the ls-refs command */\n+\tif (!server_supports_v2(\"ls-refs\"))\n+\t\tdie(\"server doesn't support 'ls-refs' command\");\n+\n+\t/* Issue request for ls-refs */\n+\tpacket_write_fmt(fd_out, \"command=ls-refs\");\n+\tpacket_delim(fd_out);\n+\tpacket_write_fmt(fd_out, \"--symrefs\");\n+\tpacket_flush(fd_out);\n+\n+\t/* Process response from server */\n+\twhile (packet_reader_read(reader) == PACKET_READ_NORMAL) {\n+\t\tif (!process_ref_v2(reader->line, &list))\n+\t\t\tdie(\"invalid ls-refs response: %s\", reader->line);\n+\t}\n+\n+\tif (reader->status != PACKET_READ_FLUSH)\n+\t\tdie(\"protocol error\");\n+\n+\treturn list;\n+}\n+\n static const char *parse_feature_value(const char *feature_list, const char *feature, int *lenp)\n {\n \tint len;\ndiff --git a/remote.h b/remote.h\nindex af34e44a4..64d87bf5b 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -155,6 +155,8 @@ extern struct ref **get_remote_heads(struct packet_reader *reader,\n \t\t\t\t struct ref **list, unsigned int flags,\n \t\t\t\t struct oid_array *extra_have,\n \t\t\t\t struct oid_array *shallow_points);\n+extern struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,\n+\t\t\t\t struct ref **list, unsigned int flags);\n \n int resolve_remote_symref(struct ref *ref, struct ref *list);\n int ref_newer(const struct object_id *new_oid, const struct object_id *old_oid);\ndiff --git a/t/t5701-protocol-v2.sh b/t/t5701-protocol-v2.sh\nnew file mode 100755\nindex 000000000..4bf4d61ac\n--- /dev/null\n+++ b/t/t5701-protocol-v2.sh\n@@ -0,0 +1,28 @@\n+#!/bin/sh\n+\n+test_description='test git wire-protocol version 2'\n+\n+TEST_NO_CREATE_REPO=1\n+\n+. ./test-lib.sh\n+\n+# Test protocol v2 with 'file://' transport\n+#\n+test_expect_success 'create repo to be served by file:// transport' '\n+\tgit init file_parent &&\n+\ttest_commit -C file_parent one\n+'\n+\n+test_expect_success 'list refs with file:// using protocol v2' '\n+\tGIT_TRACE_PACKET=1 git -c protocol.version=2 \\\n+\t\tls-remote --symref \"file://$(pwd)/file_parent\" >actual 2>log &&\n+\n+\t# Server responded using protocol v2\n+\tcat log &&\n+\tgrep \"git< version 2\" log &&\n+\n+\tgit ls-remote --symref \"file://$(pwd)/file_parent\" >expect &&\n+\ttest_cmp actual expect\n+'\n+\n+test_done\ndiff --git a/transport.c b/transport.c\nindex 8a3735cf5..ddd0d6174 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -202,7 +202,7 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus\n \tdata->version = discover_version(&reader);\n \tswitch (data->version) {\n \tcase protocol_v2:\n-\t\tdie(\"support for protocol v2 not implemented yet\");\n+\t\tget_remote_refs(data->fd[1], &reader, &refs, for_push ? REF_NORMAL : 0);\n \t\tbreak;\n \tcase protocol_v1:\n \tcase protocol_v0:\ndiff --git a/upload-pack.c b/upload-pack.c\nindex d706175e4..3296831f8 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -19,6 +19,7 @@\n #include \"argv-array.h\"\n #include \"prio-queue.h\"\n #include \"protocol.h\"\n+#include \"serve.h\"\n \n static const char * const upload_pack_usage[] = {\n \tN_(\"git upload-pack [] \"),\n@@ -1106,7 +1107,7 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)\n \n \tswitch (determine_protocol_version_server()) {\n \tcase protocol_v2:\n-\t\tdie(\"support for protocol v2 not implemented yet\");\n+\t\tserve();\n \t\tbreak;\n \tcase protocol_v1:\n \t\t/*\n-- \n2.15.1.424.g9478a66081-goog\n\n"},{"id":"334120","messageId":"20171204235823.63299-10-bmwill@google.com","threadId":"47006","inReplyTo":"20171204235823.63299-1-bmwill@google.com","subject":"[WIP 09/15] transport: store protocol version","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-04T23:58:17Z","receivedAt":"2017-12-04T23:59:37Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Once protocol_v2 is introduced requesting a fetch or a push will need to\nbe handled differently depending on the protocol version. Store the\nprotocol version the serving is speaking in 'struct git_transport_data'\nand use it to determine what to do in the case of a fetch or a push.\n\nSigned-off-by: Brandon Williams \n---\n transport.c | 35 ++++++++++++++++++++++++++---------\n 1 file changed, 26 insertions(+), 9 deletions(-)\n\ndiff --git a/transport.c b/transport.c\nindex 28b744ec2..4160c4167 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -118,6 +118,7 @@ struct git_transport_data {\n \tstruct child_process *conn;\n \tint fd[2];\n \tunsigned got_remote_heads : 1;\n+\tenum protocol_version version;\n \tstruct oid_array extra_have;\n \tstruct oid_array shallow;\n };\n@@ -198,7 +199,8 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus\n \n \tpacket_reader_init(&reader, data->fd[0], NULL, 0);\n \n-\tswitch (discover_version(&reader)) {\n+\tdata->version = discover_version(&reader);\n+\tswitch (data->version) {\n \tcase protocol_v1:\n \tcase protocol_v0:\n \t\tget_remote_heads(&reader, &refs,\n@@ -219,7 +221,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n {\n \tint ret = 0;\n \tstruct git_transport_data *data = transport->data;\n-\tstruct ref *refs;\n+\tstruct ref *refs = NULL;\n \tchar *dest = xstrdup(transport->url);\n \tstruct fetch_pack_args args;\n \tstruct ref *refs_tmp = NULL;\n@@ -245,10 +247,18 @@ static int fetch_refs_via_pack(struct transport *transport,\n \tif (!data->got_remote_heads)\n \t\trefs_tmp = get_refs_via_connect(transport, 0);\n \n-\trefs = fetch_pack(&args, data->fd, data->conn,\n-\t\t\t refs_tmp ? refs_tmp : transport->remote_refs,\n-\t\t\t dest, to_fetch, nr_heads, &data->shallow,\n-\t\t\t &transport->pack_lockfile);\n+\tswitch (data->version) {\n+\tcase protocol_v1:\n+\tcase protocol_v0:\n+\t\trefs = fetch_pack(&args, data->fd, data->conn,\n+\t\t\t\t refs_tmp ? refs_tmp : transport->remote_refs,\n+\t\t\t\t dest, to_fetch, nr_heads, &data->shallow,\n+\t\t\t\t &transport->pack_lockfile);\n+\t\tbreak;\n+\tcase protocol_unknown_version:\n+\t\tBUG(\"unknown protocol version\");\n+\t}\n+\n \tclose(data->fd[0]);\n \tclose(data->fd[1]);\n \tif (finish_connect(data->conn))\n@@ -548,7 +558,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n {\n \tstruct git_transport_data *data = transport->data;\n \tstruct send_pack_args args;\n-\tint ret;\n+\tint ret = 0;\n \n \tif (!data->got_remote_heads)\n \t\tget_refs_via_connect(transport, 1);\n@@ -573,8 +583,15 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n \telse\n \t\targs.push_cert = SEND_PACK_PUSH_CERT_NEVER;\n \n-\tret = send_pack(&args, data->fd, data->conn, remote_refs,\n-\t\t\t&data->extra_have);\n+\tswitch (data->version) {\n+\tcase protocol_v1:\n+\tcase protocol_v0:\n+\t\tret = send_pack(&args, data->fd, data->conn, remote_refs,\n+\t\t\t\t&data->extra_have);\n+\t\tbreak;\n+\tcase protocol_unknown_version:\n+\t\tBUG(\"unknown protocol version\");\n+\t}\n \n \tclose(data->fd[1]);\n \tclose(data->fd[0]);\n-- \n2.15.1.424.g9478a66081-goog\n\n"},{"id":"334121","messageId":"20171204235823.63299-12-bmwill@google.com","threadId":"47006","inReplyTo":"20171204235823.63299-1-bmwill@google.com","subject":"[WIP 11/15] serve: introduce git-serve","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-04T23:58:19Z","receivedAt":"2017-12-04T23:59:41Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Introduce git-serve, the base server for protocol version 2.\n\nWhen connecting to a server supporting protocol version 2, the server\nwill send a list all of its capabilities and then wait for the client to\nsend a command request. Some capabilities advertised are 'commands'\nwhich the client can request (push and fetch are examples of such\ncommands). A command request is comprised of a list of capabilities,\nincluding a command request \"command=\", a delimiter packet,\nfollowed by a list of parameters for the requested command.\n\nAt the end of each command a client can request that another command be\nexecuted or can terminate the connection by sending a flush packet.\n\nSigned-off-by: Brandon Williams \n---\n .gitignore | 1 +\n Makefile | 2 +\n builtin.h | 1 +\n builtin/serve.c | 25 ++++++++\n git.c | 1 +\n serve.c | 185 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n serve.h | 6 ++\n 7 files changed, 221 insertions(+)\n create mode 100644 builtin/serve.c\n create mode 100644 serve.c\n create mode 100644 serve.h\n\ndiff --git a/.gitignore b/.gitignore\nindex 833ef3b0b..2d0450c26 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -140,6 +140,7 @@\n /git-rm\n /git-send-email\n /git-send-pack\n+/git-serve\n /git-sh-i18n\n /git-sh-i18n--envsubst\n /git-sh-setup\ndiff --git a/Makefile b/Makefile\nindex 86394b69d..710672cf4 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -862,6 +862,7 @@ LIB_OBJS += revision.o\n LIB_OBJS += run-command.o\n LIB_OBJS += send-pack.o\n LIB_OBJS += sequencer.o\n+LIB_OBJS += serve.o\n LIB_OBJS += server-info.o\n LIB_OBJS += setup.o\n LIB_OBJS += sha1-array.o\n@@ -995,6 +996,7 @@ BUILTIN_OBJS += builtin/rev-parse.o\n BUILTIN_OBJS += builtin/revert.o\n BUILTIN_OBJS += builtin/rm.o\n BUILTIN_OBJS += builtin/send-pack.o\n+BUILTIN_OBJS += builtin/serve.o\n BUILTIN_OBJS += builtin/shortlog.o\n BUILTIN_OBJS += builtin/show-branch.o\n BUILTIN_OBJS += builtin/show-ref.o\ndiff --git a/builtin.h b/builtin.h\nindex f332a1257..3f3fdfc28 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -215,6 +215,7 @@ extern int cmd_rev_parse(int argc, const char **argv, const char *prefix);\n extern int cmd_revert(int argc, const char **argv, const char *prefix);\n extern int cmd_rm(int argc, const char **argv, const char *prefix);\n extern int cmd_send_pack(int argc, const char **argv, const char *prefix);\n+extern int cmd_serve(int argc, const char **argv, const char *prefix);\n extern int cmd_shortlog(int argc, const char **argv, const char *prefix);\n extern int cmd_show(int argc, const char **argv, const char *prefix);\n extern int cmd_show_branch(int argc, const char **argv, const char *prefix);\ndiff --git a/builtin/serve.c b/builtin/serve.c\nnew file mode 100644\nindex 000000000..2ecaad3b6\n--- /dev/null\n+++ b/builtin/serve.c\n@@ -0,0 +1,25 @@\n+#include \"cache.h\"\n+#include \"builtin.h\"\n+#include \"parse-options.h\"\n+#include \"serve.h\"\n+\n+static char const * const grep_usage[] = {\n+\tN_(\"git serve []\"),\n+\tNULL\n+};\n+\n+int cmd_serve(int argc, const char **argv, const char *prefix)\n+{\n+\n+\tstruct option options[] = {\n+\t\tOPT_END()\n+\t};\n+\n+\t/* ignore all unknown cmdline switches for now */\n+\targc = parse_options(argc, argv, prefix, options, grep_usage,\n+\t\t\t PARSE_OPT_KEEP_DASHDASH |\n+\t\t\t PARSE_OPT_KEEP_UNKNOWN);\n+\tserve();\n+\n+\treturn 0;\n+}\ndiff --git a/git.c b/git.c\nindex e32e16f2d..527086eaf 100644\n--- a/git.c\n+++ b/git.c\n@@ -457,6 +457,7 @@ static struct cmd_struct commands[] = {\n \t{ \"revert\", cmd_revert, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"rm\", cmd_rm, RUN_SETUP },\n \t{ \"send-pack\", cmd_send_pack, RUN_SETUP },\n+\t{ \"serve\", cmd_serve, RUN_SETUP },\n \t{ \"shortlog\", cmd_shortlog, RUN_SETUP_GENTLY | USE_PAGER },\n \t{ \"show\", cmd_show, RUN_SETUP },\n \t{ \"show-branch\", cmd_show_branch, RUN_SETUP },\ndiff --git a/serve.c b/serve.c\nnew file mode 100644\nindex 000000000..476e73b54\n--- /dev/null\n+++ b/serve.c\n@@ -0,0 +1,185 @@\n+#include \"cache.h\"\n+#include \"repository.h\"\n+#include \"config.h\"\n+#include \"pkt-line.h\"\n+#include \"version.h\"\n+#include \"argv-array.h\"\n+#include \"serve.h\"\n+\n+static int agent_advertise(struct repository *r,\n+\t\t\t struct strbuf *value)\n+{\n+\tstrbuf_addstr(value, git_user_agent_sanitized());\n+\treturn 1;\n+}\n+\n+struct protocol_capability {\n+\tconst char *name;\n+\tint advertised; /* capability was advertised */\n+\t/* int advertise(struct strbuf *value, struct repository *r) */\n+\tint (*advertise)(struct repository *r, struct strbuf *value);\n+\t/* int command(struct repository *r, struct argv_array *keys, struct argv_array *args)*/\n+\tint (*command)(struct repository *r,\n+\t\t struct argv_array *keys,\n+\t\t struct argv_array *args);\n+};\n+\n+static struct protocol_capability capabilities[] = {\n+\t{ \"agent\", 0, agent_advertise, NULL },\n+};\n+\n+static void advertise_capabilities(void)\n+{\n+\tstruct strbuf capability = STRBUF_INIT;\n+\tstruct strbuf value = STRBUF_INIT;\n+\tint i;\n+\n+\tfor (i = 0; i < ARRAY_SIZE(capabilities); i++) {\n+\t\tstruct protocol_capability *c = &capabilities[i];\n+\n+\t\tc->advertised = c->advertise(the_repository, &value);\n+\t\tif (c->advertised) {\n+\t\t\tstrbuf_addstr(&capability, c->name);\n+\n+\t\t\tif (value.len) {\n+\t\t\t\tstrbuf_addch(&capability, '=');\n+\t\t\t\tstrbuf_addbuf(&capability, &value);\n+\t\t\t}\n+\n+\t\t\tstrbuf_addch(&capability, '\\n');\n+\t\t\tpacket_write(1, capability.buf, capability.len);\n+\t\t}\n+\n+\t\tstrbuf_reset(&capability);\n+\t\tstrbuf_reset(&value);\n+\t}\n+\n+\tpacket_flush(1);\n+\tstrbuf_release(&capability);\n+\tstrbuf_release(&value);\n+}\n+\n+static struct protocol_capability *get_capability(const char *key)\n+{\n+\tint i;\n+\n+\tif (!key)\n+\t\treturn NULL;\n+\n+\tfor (i = 0; i < ARRAY_SIZE(capabilities); i++) {\n+\t\tstruct protocol_capability *c = &capabilities[i];\n+\t\tconst char *out;\n+\t\tif (skip_prefix(key, c->name, &out) && (!*out || *out == '='))\n+\t\t\treturn c;\n+\t}\n+\n+\treturn NULL;\n+}\n+\n+static int is_valid_capability(const char *key)\n+{\n+\tconst struct protocol_capability *c = get_capability(key);\n+\n+\treturn c && c->advertised;\n+}\n+\n+static int is_command(const char *key, struct protocol_capability **command)\n+{\n+\tconst char *out;\n+\n+\tif (skip_prefix(key, \"command=\", &out)) {\n+\t\tstruct protocol_capability *cmd = get_capability(out);\n+\n+\t\tif (!cmd || !cmd->advertised || !cmd->command)\n+\t\t\tdie(\"invalid cmd '%s'\", out);\n+\t\tif (*command)\n+\t\t\tdie(\"command already requested\");\n+\n+\t\t*command = cmd;\n+\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n+#define PROCESS_REQUEST_KEYS 0\n+#define PROCESS_REQUEST_ARGS 1\n+#define PROCESS_REQUEST_DONE 2\n+\n+static int process_request(void)\n+{\n+\tint state = PROCESS_REQUEST_KEYS;\n+\tstruct packet_reader reader;\n+\tstruct argv_array keys = ARGV_ARRAY_INIT;\n+\tstruct argv_array args = ARGV_ARRAY_INIT;\n+\tstruct protocol_capability *command = NULL;\n+\n+\tpacket_reader_init(&reader, 0, NULL, 0);\n+\n+\twhile (state != PROCESS_REQUEST_DONE) {\n+\t\tswitch (packet_reader_read(&reader)) {\n+\t\tcase PACKET_READ_ERROR:\n+\t\t\tBUG(\"invalid state\");\n+\t\tcase PACKET_READ_NORMAL:\n+\t\t\tbreak;\n+\t\tcase PACKET_READ_FLUSH:\n+\t\t\tstate = PROCESS_REQUEST_DONE;\n+\t\t\tcontinue;\n+\t\tcase PACKET_READ_DELIM:\n+\t\t\tif (state != PROCESS_REQUEST_KEYS)\n+\t\t\t\tdie(\"protocol error\");\n+\t\t\tstate = PROCESS_REQUEST_ARGS;\n+\t\t\t/*\n+\t\t\t * maybe include a check to make sure that a\n+\t\t\t * command/capabilities were given.\n+\t\t\t */\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tswitch (state) {\n+\t\tcase PROCESS_REQUEST_KEYS:\n+\t\t\t/* collect request; a sequence of keys and values */\n+\t\t\tif (is_command(reader.line, &command) ||\n+\t\t\t is_valid_capability(reader.line))\n+\t\t\t\targv_array_push(&keys, reader.line);\n+\t\t\tbreak;\n+\t\tcase PROCESS_REQUEST_ARGS:\n+\t\t\t/* collect arguments for the requested command */\n+\t\t\targv_array_push(&args, reader.line);\n+\t\t\tbreak;\n+\t\tcase PROCESS_REQUEST_DONE:\n+\t\t\tcontinue;\n+\t\tdefault:\n+\t\t\tBUG(\"invalid state\");\n+\t\t}\n+\t}\n+\n+\t/*\n+\t * If no command and no keys were given then the client wanted to\n+\t * terminate the connection.\n+\t */\n+\tif (!keys.argc && !args.argc)\n+\t\treturn 1;\n+\n+\tif (!command)\n+\t\tdie(\"no command requested\");\n+\n+\tcommand->command(the_repository, &keys, &args);\n+\n+\targv_array_clear(&keys);\n+\targv_array_clear(&args);\n+\treturn 0;\n+}\n+\n+/* Main serve loop for protocol version 2 */\n+void serve(void)\n+{\n+\t/* serve by default supports v2 */\n+\tpacket_write_fmt(1, \"version 2\\n\");\n+\n+\tadvertise_capabilities();\n+\n+\tfor (;;)\n+\t\tif (process_request())\n+\t\t\tbreak;\n+}\ndiff --git a/serve.h b/serve.h\nnew file mode 100644\nindex 000000000..1ed9685ca\n--- /dev/null\n+++ b/serve.h\n@@ -0,0 +1,6 @@\n+#ifndef SERVE_H\n+#define SERVE_H\n+\n+extern void serve(void);\n+\n+#endif /* SERVE_H */\n-- \n2.15.1.424.g9478a66081-goog\n\n"},{"id":"334122","messageId":"20171204235823.63299-13-bmwill@google.com","threadId":"47006","inReplyTo":"20171204235823.63299-1-bmwill@google.com","subject":"[WIP 12/15] ls-refs: introduce ls-refs server command","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-04T23:58:20Z","receivedAt":"2017-12-04T23:59:44Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Introduce the ls-refs server command. In protocol v2, the ls-refs\ncommand is used to request the ref advertisement from the server. Since\nit is a command which can be requested (as opposed to manditory in v1),\na clinet can sent a number of parameters in its request to limit the ref\nadvertisement based on provided ref-patterns.\n\nSigned-off-by: Brandon Williams \n---\n Makefile | 1 +\n ls-refs.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n ls-refs.h | 9 ++++++\n serve.c | 8 ++++++\n 4 files changed, 114 insertions(+)\n create mode 100644 ls-refs.c\n create mode 100644 ls-refs.h\n\ndiff --git a/Makefile b/Makefile\nindex 710672cf4..be3c2f98b 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -807,6 +807,7 @@ LIB_OBJS += list-objects.o\n LIB_OBJS += ll-merge.o\n LIB_OBJS += lockfile.o\n LIB_OBJS += log-tree.o\n+LIB_OBJS += ls-refs.o\n LIB_OBJS += mailinfo.o\n LIB_OBJS += mailmap.o\n LIB_OBJS += match-trees.o\ndiff --git a/ls-refs.c b/ls-refs.c\nnew file mode 100644\nindex 000000000..591dd105d\n--- /dev/null\n+++ b/ls-refs.c\n@@ -0,0 +1,96 @@\n+#include \"cache.h\"\n+#include \"repository.h\"\n+#include \"refs.h\"\n+#include \"remote.h\"\n+#include \"argv-array.h\"\n+#include \"ls-refs.h\"\n+#include \"pkt-line.h\"\n+\n+struct ls_refs_data {\n+\tunsigned peel;\n+\tunsigned symrefs;\n+\tstruct argv_array patterns;\n+};\n+\n+/*\n+ * Is there one among the list of patterns that match the tail part\n+ * of the path?\n+ */\n+static int tail_match(const char **pattern, const char *path)\n+{\n+\tconst char *p;\n+\tchar *pathbuf;\n+\n+\tif (!pattern)\n+\t\treturn 1; /* no restriction */\n+\n+\tpathbuf = xstrfmt(\"/%s\", path);\n+\twhile ((p = *(pattern++)) != NULL) {\n+\t\tif (!wildmatch(p, pathbuf, 0)) {\n+\t\t\tfree(pathbuf);\n+\t\t\treturn 1;\n+\t\t}\n+\t}\n+\tfree(pathbuf);\n+\treturn 0;\n+}\n+\n+static int send_ref(const char *refname, const struct object_id *oid,\n+\t\t int flag, void *cb_data)\n+{\n+\tstruct ls_refs_data *data = cb_data;\n+\tconst char *refname_nons = strip_namespace(refname);\n+\tstruct strbuf refline = STRBUF_INIT;\n+\n+\tif (data->patterns.argc && !tail_match(data->patterns.argv, refname))\n+\t\treturn 0;\n+\n+\tstrbuf_addf(&refline, \"%s %s\", oid_to_hex(oid), refname_nons);\n+\tif (data->symrefs && flag & REF_ISSYMREF) {\n+\t\tstruct object_id unused;\n+\t\tconst char *symref_target = resolve_ref_unsafe(refname, 0,\n+\t\t\t\t\t\t\t unused.hash,\n+\t\t\t\t\t\t\t &flag);\n+\n+\t\tif (!symref_target)\n+\t\t\tdie(\"'%s' is a symref but it is not?\", refname);\n+\n+\t\tstrbuf_addf(&refline, \" %s\", symref_target);\n+\t}\n+\n+\tstrbuf_addch(&refline, '\\n');\n+\n+\tpacket_write(1, refline.buf, refline.len);\n+\tif (data->peel) {\n+\t\tstruct object_id peeled;\n+\t\tif (!peel_ref(refname, peeled.hash))\n+\t\t\tpacket_write_fmt(1, \"%s %s^{}\\n\", oid_to_hex(&peeled),\n+\t\t\t\t\t refname_nons);\n+\t}\n+\n+\tstrbuf_release(&refline);\n+\treturn 0;\n+}\n+\n+int ls_refs(struct repository *r, struct argv_array *keys, struct argv_array *args)\n+{\n+\tint i;\n+\tstruct ls_refs_data data = { 0, 0, ARGV_ARRAY_INIT };\n+\n+\tfor (i = 0; i < args->argc; i++) {\n+\t\tif (!strcmp(\"--peeled\", args->argv[i]))\n+\t\t\tdata.peel = 1;\n+\t\telse if (!strcmp(\"--symrefs\", args->argv[i]))\n+\t\t\tdata.symrefs = 1;\n+\t\telse\n+\t\t\t/* Pattern */\n+\t\t\targv_array_pushf(&data.patterns, \"*/%s\", args->argv[i]);\n+\n+\t}\n+\n+\thead_ref_namespaced(send_ref, &data);\n+\tfor_each_namespaced_ref(send_ref, &data);\n+\tpacket_flush(1);\n+\targv_array_clear(&data.patterns);\n+\treturn 0;\n+}\ndiff --git a/ls-refs.h b/ls-refs.h\nnew file mode 100644\nindex 000000000..9e4c57bfe\n--- /dev/null\n+++ b/ls-refs.h\n@@ -0,0 +1,9 @@\n+#ifndef LS_REFS_H\n+#define LS_REFS_H\n+\n+struct repository;\n+struct argv_array;\n+extern int ls_refs(struct repository *r, struct argv_array *keys,\n+\t\t struct argv_array *args);\n+\n+#endif /* LS_REFS_H */\ndiff --git a/serve.c b/serve.c\nindex 476e73b54..36f77c365 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -4,8 +4,15 @@\n #include \"pkt-line.h\"\n #include \"version.h\"\n #include \"argv-array.h\"\n+#include \"ls-refs.h\"\n #include \"serve.h\"\n \n+static int always_advertise(struct repository *r,\n+\t\t\t struct strbuf *value)\n+{\n+\treturn 1;\n+}\n+\n static int agent_advertise(struct repository *r,\n \t\t\t struct strbuf *value)\n {\n@@ -26,6 +33,7 @@ struct protocol_capability {\n \n static struct protocol_capability capabilities[] = {\n \t{ \"agent\", 0, agent_advertise, NULL },\n+\t{ \"ls-refs\", 0, always_advertise, ls_refs },\n };\n \n static void advertise_capabilities(void)\n-- \n2.15.1.424.g9478a66081-goog\n\n"},{"id":"334123","messageId":"20171204235823.63299-16-bmwill@google.com","threadId":"47006","inReplyTo":"20171204235823.63299-1-bmwill@google.com","subject":"[WIP 15/15] fetch-pack: perform a fetch using v2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-04T23:58:23Z","receivedAt":"2017-12-04T23:59:46Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"When communicating with a v2 server, perform a fetch by requesting the\n'fetch' command.\n\nSigned-off-by: Brandon Williams \n---\n builtin/fetch-pack.c | 2 +-\n fetch-pack.c | 237 ++++++++++++++++++++++++++++++++++++++++++++++++-\n fetch-pack.h | 4 +-\n t/t5701-protocol-v2.sh | 26 ++++++\n transport.c | 8 +-\n 5 files changed, 270 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 061c278b4..e02c7761b 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -211,7 +211,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t}\n \n \tref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought,\n-\t\t\t &shallow, pack_lockfile_ptr);\n+\t\t\t &shallow, pack_lockfile_ptr, protocol_v0);\n \tif (pack_lockfile) {\n \t\tprintf(\"lock %s\\n\", pack_lockfile);\n \t\tfflush(stdout);\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 105506e9a..a8ef8f3e5 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1007,6 +1007,232 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \treturn ref;\n }\n \n+static void add_wants(const struct ref *wants, struct strbuf *req_buf)\n+{\n+\tfor ( ; wants ; wants = wants->next) {\n+\t\tconst struct object_id *remote = &wants->old_oid;\n+\t\tconst char *remote_hex;\n+\t\tstruct object *o;\n+\n+\t\t/*\n+\t\t * If that object is complete (i.e. it is an ancestor of a\n+\t\t * local ref), we tell them we have it but do not have to\n+\t\t * tell them about its ancestors, which they already know\n+\t\t * about.\n+\t\t *\n+\t\t * We use lookup_object here because we are only\n+\t\t * interested in the case we *know* the object is\n+\t\t * reachable and we have already scanned it.\n+\t\t */\n+\t\tif (((o = lookup_object(remote->hash)) != NULL) &&\n+\t\t (o->flags & COMPLETE)) {\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tremote_hex = oid_to_hex(remote);\n+\t\tpacket_buf_write(req_buf, \"want %s\\n\", remote_hex);\n+\t}\n+}\n+\n+/* */\n+static int add_haves(struct strbuf *req_buf)\n+{\n+\tint count = 0;\n+\tconst struct object_id *oid;\n+\twhile ((oid = get_rev())) {\n+\t\tpacket_buf_write(req_buf, \"have %s\\n\", oid_to_hex(oid));\n+\t\tif (++count >= INITIAL_FLUSH)\n+\t\t\tbreak;\n+\t};\n+\n+\treturn count;\n+}\n+\n+static int send_fetch_request(int fd_out, const struct fetch_pack_args *args,\n+\t\t\t const struct ref *wants)\n+{\n+\tint ret = 0;\n+\tstruct strbuf req_buf = STRBUF_INIT;\n+\n+\tuse_sideband = 2;\n+\tpacket_buf_write(&req_buf, \"command=fetch\");\n+\tpacket_buf_write(&req_buf, \"agent=%s\", git_user_agent_sanitized());\n+\tpacket_buf_delim(&req_buf);\n+\tif (args->use_thin_pack)\n+\t\tpacket_buf_write(&req_buf, \"thin-pack\");\n+\tif (args->no_progress)\n+\t\tpacket_buf_write(&req_buf, \"no-progress\");\n+\tif (args->include_tag)\n+\t\tpacket_buf_write(&req_buf, \"include-tag\");\n+\tif (prefer_ofs_delta)\n+\t\tpacket_buf_write(&req_buf, \"ofs-delta\");\n+\n+\t/* add wants */\n+\tadd_wants(wants, &req_buf);\n+\n+\t/* Add initial haves */\n+\tret = add_haves(&req_buf);\n+\n+\t/* Send request */\n+\tpacket_buf_flush(&req_buf);\n+\twrite_or_die(fd_out, req_buf.buf, req_buf.len);\n+\n+\tstrbuf_release(&req_buf);\n+\treturn ret;\n+}\n+\n+static enum ack_type process_ack(const char *line, struct object_id *oid)\n+{\n+\tconst char *arg;\n+\n+\tif (!strcmp(line, \"NAK\"))\n+\t\treturn NAK;\n+\tif (skip_prefix(line, \"ACK \", &arg)) {\n+\t\tif (!parse_oid_hex(arg, oid, &arg)) {\n+\t\t\tif (strstr(arg, \"continue\"))\n+\t\t\t\treturn ACK_continue;\n+\t\t\tif (strstr(arg, \"common\"))\n+\t\t\t\treturn ACK_common;\n+\t\t\tif (strstr(arg, \"ready\"))\n+\t\t\t\treturn ACK_ready;\n+\t\t\treturn ACK;\n+\t\t}\n+\t}\n+\tif (skip_prefix(line, \"ERR \", &arg))\n+\t\tdie(_(\"remote error: %s\"), arg);\n+\tdie(_(\"git fetch-pack: expected ACK/NAK, got '%s'\"), line);\n+}\n+\n+static int process_acks(struct packet_reader *reader)\n+{\n+\tint got_ready = 0;\n+\tint got_common = 0;\n+\twhile (packet_reader_read(reader) == PACKET_READ_NORMAL) {\n+\t\tstruct object_id oid;\n+\t\tstruct commit *commit;\n+\t\tenum ack_type ack = process_ack(reader->line, &oid);\n+\n+\t\tswitch (ack) {\n+\t\tcase ACK_ready:\n+\t\t\tclear_prio_queue(&rev_list);\n+\t\t\tgot_ready = 1;\n+\t\t\t/* fallthrough */\n+\t\tcase ACK_common:\n+\t\t\tcommit = lookup_commit(&oid);\n+\t\t\tmark_common(commit, 0, 1);\n+\t\t\tgot_common = 1;\n+\t\t\tbreak;\n+\t\tcase NAK:\n+\t\t\tbreak;\n+\t\tcase ACK:\n+\t\tcase ACK_continue:\n+\t\t\tdie(\"ACK/ACK_continue not supported\");\n+\t\t}\n+\t}\n+\n+\tif (reader->status != PACKET_READ_FLUSH &&\n+\t reader->status != PACKET_READ_DELIM)\n+\t\tdie(\"Error during processing acks: %d\",reader->status);\n+\n+\t/* return 0 if no common, 1 if there are common, or 2 if ready */\n+\treturn got_ready + got_common;\n+}\n+\n+static int send_haves(int fd_out, int *in_vain)\n+{\n+\tint ret = 0;\n+\tstruct strbuf req_buf = STRBUF_INIT;\n+\tint haves_added = add_haves(&req_buf);\n+\n+\t*in_vain += haves_added;\n+\tif (!haves_added || *in_vain >= MAX_IN_VAIN) {\n+\t\t/* Send Done */\n+\t\tpacket_buf_write(&req_buf, \"done\\n\");\n+\t\tret = 1;\n+\t}\n+\n+\t/* Send request */\n+\tpacket_buf_flush(&req_buf);\n+\twrite_or_die(fd_out, req_buf.buf, req_buf.len);\n+\n+\tstrbuf_release(&req_buf);\n+\treturn ret;\n+}\n+\n+#define FETCH_PACK_CHECK_LOCAL 0\n+#define FETCH_PACK_REQUEST 1\n+#define FETCH_PACK_ACKS 2\n+#define FETCH_PACK_HAVES 3\n+#define FETCH_PACK_GET_PACK 4\n+#define FETCH_PACK_DONE 5\n+\n+static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n+\t\t\t\t int fd[2],\n+\t\t\t\t const struct ref *orig_ref,\n+\t\t\t\t struct ref **sought, int nr_sought,\n+\t\t\t\t char **pack_lockfile)\n+{\n+\tstruct ref *ref = copy_ref_list(orig_ref);\n+\tint state = FETCH_PACK_CHECK_LOCAL;\n+\tstruct packet_reader reader;\n+\tint in_vain = 0;\n+\tpacket_reader_init(&reader, fd[0], NULL, 0);\n+\n+\twhile (state != FETCH_PACK_DONE) {\n+\t\tswitch (state) {\n+\t\tcase FETCH_PACK_CHECK_LOCAL:\n+\t\t\tsort_ref_list(&ref, ref_compare_name);\n+\t\t\tQSORT(sought, nr_sought, cmp_ref_by_name);\n+\n+\t\t\t/* Filter 'ref' by 'sought' and those that aren't local */\n+\t\t\tif (everything_local(args, &ref, sought, nr_sought))\n+\t\t\t\tstate = FETCH_PACK_DONE;\n+\t\t\telse\n+\t\t\t\tstate = FETCH_PACK_REQUEST;\n+\t\t\tbreak;\n+\t\tcase FETCH_PACK_REQUEST:\n+\t\t\tif (send_fetch_request(fd[1], args, ref))\n+\t\t\t\tstate = FETCH_PACK_ACKS;\n+\t\t\telse\n+\t\t\t\tstate = FETCH_PACK_GET_PACK;\n+\t\t\tbreak;\n+\t\tcase FETCH_PACK_ACKS:\n+\t\t\t/* Process ACKs/NAKs */\n+\t\t\tswitch (process_acks(&reader)) {\n+\t\t\tcase 2:\n+\t\t\t\tstate = FETCH_PACK_GET_PACK;\n+\t\t\t\tbreak;\n+\t\t\tcase 1:\n+\t\t\t\tin_vain = 0;\n+\t\t\t\t/* fallthrough */\n+\t\t\tdefault:\n+\t\t\t\tstate = FETCH_PACK_HAVES;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t\tbreak;\n+\t\tcase FETCH_PACK_HAVES:\n+\t\t\tif (send_haves(fd[1], &in_vain))\n+\t\t\t\tstate = FETCH_PACK_GET_PACK;\n+\t\t\telse\n+\t\t\t\tstate = FETCH_PACK_ACKS;\n+\t\t\tbreak;\n+\t\tcase FETCH_PACK_GET_PACK:\n+\t\t\t/* get the pack */\n+\t\t\tif (get_pack(args, fd, pack_lockfile))\n+\t\t\t\tdie(_(\"git fetch-pack: fetch failed.\"));\n+\n+\t\t\tstate = FETCH_PACK_DONE;\n+\t\t\tbreak;\n+\t\tcase FETCH_PACK_DONE:\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tdie(\"invalid state\");\n+\t\t}\n+\t}\n+\n+\treturn ref;\n+}\n+\n static void fetch_pack_config(void)\n {\n \tgit_config_get_int(\"fetch.unpacklimit\", &fetch_unpack_limit);\n@@ -1152,7 +1378,8 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\t const char *dest,\n \t\t struct ref **sought, int nr_sought,\n \t\t struct oid_array *shallow,\n-\t\t char **pack_lockfile)\n+\t\t char **pack_lockfile,\n+\t\t enum protocol_version version)\n {\n \tstruct ref *ref_cpy;\n \tstruct shallow_info si;\n@@ -1166,8 +1393,12 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\tdie(_(\"no matching remote head\"));\n \t}\n \tprepare_shallow_info(&si, shallow);\n-\tref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought,\n-\t\t\t\t&si, pack_lockfile);\n+\tif (version == protocol_v2)\n+\t\tref_cpy = do_fetch_pack_v2(args, fd, ref, sought, nr_sought,\n+\t\t\t\t\t pack_lockfile);\n+\telse\n+\t\tref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought,\n+\t\t\t\t\t&si, pack_lockfile);\n \treprepare_packed_git();\n \tupdate_shallow(args, sought, nr_sought, &si);\n \tclear_shallow_info(&si);\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex b6aeb43a8..7afca7305 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -3,6 +3,7 @@\n \n #include \"string-list.h\"\n #include \"run-command.h\"\n+#include \"protocol.h\"\n \n struct oid_array;\n \n@@ -43,7 +44,8 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\t struct ref **sought,\n \t\t int nr_sought,\n \t\t struct oid_array *shallow,\n-\t\t char **pack_lockfile);\n+\t\t char **pack_lockfile,\n+\t\t enum protocol_version version);\n \n /*\n * Print an appropriate error message for each sought ref that wasn't\ndiff --git a/t/t5701-protocol-v2.sh b/t/t5701-protocol-v2.sh\nindex 4bf4d61ac..c788ea597 100755\n--- a/t/t5701-protocol-v2.sh\n+++ b/t/t5701-protocol-v2.sh\n@@ -25,4 +25,30 @@ test_expect_success 'list refs with file:// using protocol v2' '\n \ttest_cmp actual expect\n '\n \n+test_expect_success 'clone with file:// using protocol v2' '\n+\tGIT_TRACE_PACKET=1 git -c protocol.version=2 \\\n+\t\tclone \"file://$(pwd)/file_parent\" file_child 2>log &&\n+\n+\tgit -C file_child log -1 --format=%s >actual &&\n+\tgit -C file_parent log -1 --format=%s >expect &&\n+\ttest_cmp expect actual &&\n+\n+\t# Server responded using protocol v1\n+\tgrep \"clone< version 2\" log\n+'\n+\n+test_expect_success 'fetch with file:// using protocol v2' '\n+\ttest_commit -C file_parent two &&\n+\n+\tGIT_TRACE_PACKET=1 git -C file_child -c protocol.version=2 \\\n+\t\tfetch origin 2>log &&\n+\n+\tgit -C file_child log -1 --format=%s origin/master >actual &&\n+\tgit -C file_parent log -1 --format=%s >expect &&\n+\ttest_cmp expect actual &&\n+\n+\t# Server responded using protocol v1\n+\tgrep \"fetch< version 2\" log\n+'\n+\n test_done\ndiff --git a/transport.c b/transport.c\nindex ddd0d6174..50354abed 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -252,14 +252,18 @@ static int fetch_refs_via_pack(struct transport *transport,\n \n \tswitch (data->version) {\n \tcase protocol_v2:\n-\t\tdie(\"support for protocol v2 not implemented yet\");\n+\t\trefs = fetch_pack(&args, data->fd, data->conn,\n+\t\t\t\t refs_tmp ? refs_tmp : transport->remote_refs,\n+\t\t\t\t dest, to_fetch, nr_heads, &data->shallow,\n+\t\t\t\t &transport->pack_lockfile, data->version);\n+\t\tpacket_flush(data->fd[1]);\n \t\tbreak;\n \tcase protocol_v1:\n \tcase protocol_v0:\n \t\trefs = fetch_pack(&args, data->fd, data->conn,\n \t\t\t\t refs_tmp ? refs_tmp : transport->remote_refs,\n \t\t\t\t dest, to_fetch, nr_heads, &data->shallow,\n-\t\t\t\t &transport->pack_lockfile);\n+\t\t\t\t &transport->pack_lockfile, data->version);\n \t\tbreak;\n \tcase protocol_unknown_version:\n \t\tBUG(\"unknown protocol version\");\n-- \n2.15.1.424.g9478a66081-goog\n\n"},{"id":"334124","messageId":"20171204235823.63299-15-bmwill@google.com","threadId":"47006","inReplyTo":"20171204235823.63299-1-bmwill@google.com","subject":"[WIP 14/15] upload_pack: introduce fetch server command","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-04T23:58:22Z","receivedAt":"2017-12-04T23:59:48Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Introduce the 'fetch' server command.\n\nSigned-off-by: Brandon Williams \n---\n serve.c | 2 +\n upload-pack.c | 223 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n upload-pack.h | 9 +++\n 3 files changed, 234 insertions(+)\n create mode 100644 upload-pack.h\n\ndiff --git a/serve.c b/serve.c\nindex 36f77c365..7823ef0d9 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -6,6 +6,7 @@\n #include \"argv-array.h\"\n #include \"ls-refs.h\"\n #include \"serve.h\"\n+#include \"upload-pack.h\"\n \n static int always_advertise(struct repository *r,\n \t\t\t struct strbuf *value)\n@@ -34,6 +35,7 @@ struct protocol_capability {\n static struct protocol_capability capabilities[] = {\n \t{ \"agent\", 0, agent_advertise, NULL },\n \t{ \"ls-refs\", 0, always_advertise, ls_refs },\n+\t{ \"fetch\", 0, always_advertise, upload_pack_v2 },\n };\n \n static void advertise_capabilities(void)\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 3296831f8..dc4421ab3 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -20,6 +20,7 @@\n #include \"prio-queue.h\"\n #include \"protocol.h\"\n #include \"serve.h\"\n+#include \"upload-pack.h\"\n \n static const char * const upload_pack_usage[] = {\n \tN_(\"git upload-pack [] \"),\n@@ -1041,6 +1042,228 @@ static void upload_pack(void)\n \t}\n }\n \n+static int process_want(const char *line)\n+{\n+\tconst char *arg;\n+\tif (skip_prefix(line, \"want \", &arg)) {\n+\t\tstruct object_id oid;\n+\t\tstruct object *o;\n+\n+\t\tif (get_oid_hex(arg, &oid))\n+\t\t\tdie(\"git upload-pack: protocol error, \"\n+\t\t\t \"expected to get oid, not '%s'\", line);\n+\n+\t\to = parse_object(&oid);\n+\t\tif (!o) {\n+\t\t\tpacket_write_fmt(1,\n+\t\t\t\t\t \"ERR upload-pack: not our ref %s\",\n+\t\t\t\t\t oid_to_hex(&oid));\n+\t\t\tdie(\"git upload-pack: not our ref %s\",\n+\t\t\t oid_to_hex(&oid));\n+\t\t}\n+\n+\t\tif (!(o->flags & WANTED)) {\n+\t\t\to->flags |= WANTED;\n+\t\t\tadd_object_array(o, NULL, &want_obj);\n+\t\t}\n+\n+\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n+static int parse_have(const char *line, struct oid_array *haves)\n+{\n+\tconst char *arg;\n+\tif (skip_prefix(line, \"have \", &arg)) {\n+\t\tstruct object_id oid;\n+\n+\t\tif (get_oid_hex(arg, &oid))\n+\t\t\tdie(\"git upload-pack: expected SHA1 object, got '%s'\", arg);\n+\t\toid_array_append(haves, &oid);\n+\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n+static void process_args(struct argv_array *args, struct oid_array *haves_oid)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < args->argc; i++) {\n+\t\tconst char *arg = args->argv[i];\n+\n+\t\t/* process want */\n+\t\tif (process_want(arg))\n+\t\t\tcontinue;\n+\t\t/* process have line */\n+\t\tif (parse_have(arg, haves_oid))\n+\t\t\tcontinue;\n+\n+\t\t/* process args like thin-pack */\n+\t\tif (!strcmp(arg, \"thin-pack\")) {\n+\t\t\tuse_thin_pack = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (!strcmp(arg, \"ofs-delta\")) {\n+\t\t\tuse_ofs_delta = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (!strcmp(arg, \"no-progress\")) {\n+\t\t\tno_progress = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (!strcmp(arg, \"include-tag\")) {\n+\t\t\tuse_include_tag = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\t/* ignore unknown lines maybe? */\n+\t\tdie(\"unexpect line: '%s'\", arg);\n+\t}\n+}\n+\n+static int process_haves(struct oid_array *haves, struct oid_array *common)\n+{\n+\tint i;\n+\n+\t/* Process haves */\n+\tfor (i = 0; i < haves->nr; i++) {\n+\t\tconst struct object_id *oid = &haves->oid[i];\n+\t\tstruct object *o;\n+\t\tint we_knew_they_have = 0;\n+\n+\t\tif (!has_object_file(oid))\n+\t\t\tcontinue;\n+\n+\t\toid_array_append(common, oid);\n+\n+\t\to = parse_object(oid);\n+\t\tif (!o)\n+\t\t\tdie(\"oops (%s)\", oid_to_hex(oid));\n+\t\tif (o->type == OBJ_COMMIT) {\n+\t\t\tstruct commit_list *parents;\n+\t\t\tstruct commit *commit = (struct commit *)o;\n+\t\t\tif (o->flags & THEY_HAVE)\n+\t\t\t\twe_knew_they_have = 1;\n+\t\t\telse\n+\t\t\t\to->flags |= THEY_HAVE;\n+\t\t\tif (!oldest_have || (commit->date < oldest_have))\n+\t\t\t\toldest_have = commit->date;\n+\t\t\tfor (parents = commit->parents;\n+\t\t\t parents;\n+\t\t\t parents = parents->next)\n+\t\t\t\tparents->item->object.flags |= THEY_HAVE;\n+\t\t}\n+\t\tif (!we_knew_they_have)\n+\t\t\tadd_object_array(o, NULL, &have_obj);\n+\t}\n+\n+\treturn 0;\n+}\n+\n+static int send_acks(struct oid_array *acks, struct strbuf *response)\n+{\n+\tint i;\n+\t/* Send Acks */\n+\tif (!acks->nr)\n+\t\tpacket_buf_write(response, \"NAK\\n\");\n+\n+\tfor (i = 0; i < acks->nr; i++) {\n+\t\tpacket_buf_write(response, \"ACK %s common\\n\",\n+\t\t\t\t oid_to_hex(&acks->oid[i]));\n+\t}\n+\n+\tif (ok_to_give_up()) {\n+\t\t/* Send Ready */\n+\t\tpacket_buf_write(response, \"ACK %s ready\\n\",\n+\t\t\t\t oid_to_hex(&acks->oid[i-1]));\n+\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n+#define COMMON_START 0\n+#define COMMON_DONE 1\n+#define COMMON_READ_HAVES 2\n+#define COMMON_PROCESS_HAVES 3\n+\n+static int get_common_commits_v2(struct oid_array *haves_oid)\n+{\n+\tstruct packet_reader reader;\n+\tint done = 0;\n+\tint state = haves_oid->nr ? COMMON_PROCESS_HAVES : COMMON_DONE;\n+\tstruct oid_array common = OID_ARRAY_INIT;\n+\tstruct strbuf response = STRBUF_INIT;\n+\tpacket_reader_init(&reader, 0, NULL, 0);\n+\n+\twhile (state != COMMON_DONE) {\n+\t\tswitch (state) {\n+\t\tcase COMMON_START:\n+\t\t\tbreak;\n+\t\tcase COMMON_READ_HAVES:\n+\t\t\twhile (packet_reader_read(&reader) == PACKET_READ_NORMAL) {\n+\n+\t\t\t\tif (parse_have(reader.line, haves_oid))\n+\t\t\t\t\tcontinue;\n+\t\t\t\tif (!strcmp(reader.line, \"done\")) {\n+\t\t\t\t\tdone = 1;\n+\t\t\t\t\tcontinue;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t\tif (reader.status != PACKET_READ_FLUSH)\n+\t\t\t\tdie(\"ERROR\");\n+\n+\t\t\tstate = COMMON_PROCESS_HAVES;\n+\t\t\tbreak;\n+\t\tcase COMMON_PROCESS_HAVES:\n+\t\t\tprocess_haves(haves_oid, &common);\n+\t\t\tif (done) {\n+\t\t\t\tstate = COMMON_DONE;\n+\t\t\t} else if (send_acks(&common, &response)) {\n+\t\t\t\tpacket_buf_delim(&response);\n+\t\t\t\tstate = COMMON_DONE;\n+\t\t\t} else {\n+\t\t\t\t/* Add Flush */\n+\t\t\t\tpacket_buf_flush(&response);\n+\t\t\t\tstate = COMMON_READ_HAVES;\n+\t\t\t}\n+\n+\t\t\t/* Send response */\n+\t\t\twrite_or_die(1, response.buf, response.len);\n+\t\t\tstrbuf_reset(&response);\n+\t\t\toid_array_clear(haves_oid);\n+\t\t\toid_array_clear(&common);\n+\t\t\tbreak;\n+\t\tcase COMMON_DONE:\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tBUG(\"invalid state\");\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n+\n+int upload_pack_v2(struct repository *r, struct argv_array *keys,\n+\t\t struct argv_array *args)\n+{\n+\tstruct oid_array haves_oid = OID_ARRAY_INIT;\n+\tuse_sideband = LARGE_PACKET_MAX;\n+\tprocess_args(args, &haves_oid);\n+\n+\tif (want_obj.nr) {\n+\t\t/* Determine Common Commits */\n+\t\tget_common_commits_v2(&haves_oid);\n+\t\t/* create packfile */\n+\t\tcreate_pack_file();\n+\t}\n+\n+\treturn 0;\n+}\n+\n static int upload_pack_config(const char *var, const char *value, void *unused)\n {\n \tif (!strcmp(\"uploadpack.allowtipsha1inwant\", var)) {\ndiff --git a/upload-pack.h b/upload-pack.h\nnew file mode 100644\nindex 000000000..54c429563\n--- /dev/null\n+++ b/upload-pack.h\n@@ -0,0 +1,9 @@\n+#ifndef UPLOAD_PACK_H\n+#define UPLOAD_PACK_H\n+\n+struct repository;\n+struct argv_array;\n+extern int upload_pack_v2(struct repository *r, struct argv_array *keys,\n+\t\t\t struct argv_array *args);\n+\n+#endif /* UPLOAD_PACK_H */\n-- \n2.15.1.424.g9478a66081-goog\n\n"},{"id":"334125","messageId":"20171204235823.63299-11-bmwill@google.com","threadId":"47006","inReplyTo":"20171204235823.63299-1-bmwill@google.com","subject":"[WIP 10/15] protocol: introduce enum protocol_version value protocol_v2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-04T23:58:18Z","receivedAt":"2017-12-04T23:59:51Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Introduce protocol_v2, a new value for 'enum protocol_version'.\nSubsequent patches will fill in the implementation of protocol_v2.\n\nSigned-off-by: Brandon Williams \n---\n builtin/fetch-pack.c | 3 +++\n builtin/receive-pack.c | 3 +++\n builtin/send-pack.c | 3 +++\n connect.c | 3 +++\n protocol.c | 2 ++\n protocol.h | 1 +\n remote-curl.c | 3 +++\n transport.c | 9 +++++++++\n upload-pack.c | 3 +++\n 9 files changed, 30 insertions(+)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 4873e9572..061c278b4 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -199,6 +199,9 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \tpacket_reader_init(&reader, fd[0], NULL, 0);\n \n \tswitch (discover_version(&reader)) {\n+\tcase protocol_v2:\n+\t\tdie(\"support for protocol v2 not implemented yet\");\n+\t\tbreak;\n \tcase protocol_v1:\n \tcase protocol_v0:\n \t\tget_remote_heads(&reader, &ref, 0, NULL, &shallow);\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 839c1462d..4e141d521 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1965,6 +1965,9 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t\tunpack_limit = receive_unpack_limit;\n \n \tswitch (determine_protocol_version_server()) {\n+\tcase protocol_v2:\n+\t\tdie(\"support for protocol v2 not implemented yet\");\n+\t\tbreak;\n \tcase protocol_v1:\n \t\t/*\n \t\t * v1 is just the original protocol with a version string,\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 9c2ca80c8..9441f1eed 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -261,6 +261,9 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \tpacket_reader_init(&reader, fd[0], NULL, 0);\n \n \tswitch (discover_version(&reader)) {\n+\tcase protocol_v2:\n+\t\tdie(\"support for protocol v2 not implemented yet\");\n+\t\tbreak;\n \tcase protocol_v1:\n \tcase protocol_v0:\n \t\tget_remote_heads(&reader, &remote_refs, REF_NORMAL,\ndiff --git a/connect.c b/connect.c\nindex 5f7cf05c7..433f08649 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -84,6 +84,9 @@ enum protocol_version discover_version(struct packet_reader *reader)\n \n \t/* Maybe process capabilities here, at least for v2 */\n \tswitch (version) {\n+\tcase protocol_v2:\n+\t\tdie(\"support for protocol v2 not implemented yet\");\n+\t\tbreak;\n \tcase protocol_v1:\n \t\t/* Read the peeked version line */\n \t\tpacket_reader_read(reader);\ndiff --git a/protocol.c b/protocol.c\nindex 43012b7eb..5e636785d 100644\n--- a/protocol.c\n+++ b/protocol.c\n@@ -8,6 +8,8 @@ static enum protocol_version parse_protocol_version(const char *value)\n \t\treturn protocol_v0;\n \telse if (!strcmp(value, \"1\"))\n \t\treturn protocol_v1;\n+\telse if (!strcmp(value, \"2\"))\n+\t\treturn protocol_v2;\n \telse\n \t\treturn protocol_unknown_version;\n }\ndiff --git a/protocol.h b/protocol.h\nindex 1b2bc94a8..2ad35e433 100644\n--- a/protocol.h\n+++ b/protocol.h\n@@ -5,6 +5,7 @@ enum protocol_version {\n \tprotocol_unknown_version = -1,\n \tprotocol_v0 = 0,\n \tprotocol_v1 = 1,\n+\tprotocol_v2 = 2,\n };\n \n /*\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 74c6c3049..abb6e2ac1 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -183,6 +183,9 @@ static struct ref *parse_git_refs(struct discovery *heads, int for_push)\n \tpacket_reader_init(&reader, -1, heads->buf, heads->len);\n \n \tswitch (discover_version(&reader)) {\n+\tcase protocol_v2:\n+\t\tdie(\"support for protocol v2 not implemented yet\");\n+\t\tbreak;\n \tcase protocol_v1:\n \tcase protocol_v0:\n \t\tget_remote_heads(&reader, &list, for_push ? REF_NORMAL : 0,\ndiff --git a/transport.c b/transport.c\nindex 4160c4167..8a3735cf5 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -201,6 +201,9 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus\n \n \tdata->version = discover_version(&reader);\n \tswitch (data->version) {\n+\tcase protocol_v2:\n+\t\tdie(\"support for protocol v2 not implemented yet\");\n+\t\tbreak;\n \tcase protocol_v1:\n \tcase protocol_v0:\n \t\tget_remote_heads(&reader, &refs,\n@@ -248,6 +251,9 @@ static int fetch_refs_via_pack(struct transport *transport,\n \t\trefs_tmp = get_refs_via_connect(transport, 0);\n \n \tswitch (data->version) {\n+\tcase protocol_v2:\n+\t\tdie(\"support for protocol v2 not implemented yet\");\n+\t\tbreak;\n \tcase protocol_v1:\n \tcase protocol_v0:\n \t\trefs = fetch_pack(&args, data->fd, data->conn,\n@@ -584,6 +590,9 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n \t\targs.push_cert = SEND_PACK_PUSH_CERT_NEVER;\n \n \tswitch (data->version) {\n+\tcase protocol_v2:\n+\t\tdie(\"support for protocol v2 not implemented yet\");\n+\t\tbreak;\n \tcase protocol_v1:\n \tcase protocol_v0:\n \t\tret = send_pack(&args, data->fd, data->conn, remote_refs,\ndiff --git a/upload-pack.c b/upload-pack.c\nindex d2711e4ee..d706175e4 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1105,6 +1105,9 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)\n \tgit_config(upload_pack_config, NULL);\n \n \tswitch (determine_protocol_version_server()) {\n+\tcase protocol_v2:\n+\t\tdie(\"support for protocol v2 not implemented yet\");\n+\t\tbreak;\n \tcase protocol_v1:\n \t\t/*\n \t\t * v1 is just the original protocol with a version string,\n-- \n2.15.1.424.g9478a66081-goog\n\n"},{"id":"334126","messageId":"20171204235823.63299-6-bmwill@google.com","threadId":"47006","inReplyTo":"20171204235823.63299-1-bmwill@google.com","subject":"[WIP 05/15] upload-pack: factor out processing lines","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-04T23:58:13Z","receivedAt":"2017-12-04T23:59:55Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Factor out the logic for processing shallow, deepen, deepen_since, and\ndeepen_not lines into their own functions to simplify the\n'receive_needs()' function in addition to making it easier to reuse some\nof this logic when implementing protocol_v2.\n\nSigned-off-by: Brandon Williams \n---\n upload-pack.c | 113 ++++++++++++++++++++++++++++++++++++++--------------------\n 1 file changed, 74 insertions(+), 39 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 2d16952a3..d2711e4ee 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -731,6 +731,75 @@ static void deepen_by_rev_list(int ac, const char **av,\n \tpacket_flush(1);\n }\n \n+static int process_shallow(const char *line, struct object_array *shallows)\n+{\n+\tconst char *arg;\n+\tif (skip_prefix(line, \"shallow \", &arg)) {\n+\t\tstruct object_id oid;\n+\t\tstruct object *object;\n+\t\tif (get_oid_hex(arg, &oid))\n+\t\t\tdie(\"invalid shallow line: %s\", line);\n+\t\tobject = parse_object(&oid);\n+\t\tif (!object)\n+\t\t\treturn 1;\n+\t\tif (object->type != OBJ_COMMIT)\n+\t\t\tdie(\"invalid shallow object %s\", oid_to_hex(&oid));\n+\t\tif (!(object->flags & CLIENT_SHALLOW)) {\n+\t\t\tobject->flags |= CLIENT_SHALLOW;\n+\t\t\tadd_object_array(object, NULL, shallows);\n+\t\t}\n+\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n+static int process_deepen(const char *line, int *depth)\n+{\n+\tconst char *arg;\n+\tif (skip_prefix(line, \"deepen \", &arg)) {\n+\t\tchar *end = NULL;\n+\t\t*depth = strtol(arg, &end, 0);\n+\t\tif (!end || *end || depth <= 0)\n+\t\t\tdie(\"Invalid deepen: %s\", line);\n+\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n+static int process_deepen_since(const char *line, timestamp_t *deepen_since, int *deepen_rev_list)\n+{\n+\tconst char *arg;\n+\tif (skip_prefix(line, \"deepen-since \", &arg)) {\n+\t\tchar *end = NULL;\n+\t\t*deepen_since = parse_timestamp(arg, &end, 0);\n+\t\tif (!end || *end || !deepen_since ||\n+\t\t /* revisions.c's max_age -1 is special */\n+\t\t *deepen_since == -1)\n+\t\t\tdie(\"Invalid deepen-since: %s\", line);\n+\t\t*deepen_rev_list = 1;\n+\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n+static int process_deepen_not(const char *line, struct string_list *deepen_not, int *deepen_rev_list)\n+{\n+\tconst char *arg;\n+\tif (skip_prefix(line, \"deepen-not \", &arg)) {\n+\t\tchar *ref = NULL;\n+\t\tstruct object_id oid;\n+\t\tif (expand_ref(arg, strlen(arg), oid.hash, &ref) != 1)\n+\t\t\tdie(\"git upload-pack: ambiguous deepen-not: %s\", line);\n+\t\tstring_list_append(deepen_not, ref);\n+\t\tfree(ref);\n+\t\t*deepen_rev_list = 1;\n+\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n static void receive_needs(void)\n {\n \tstruct object_array shallows = OBJECT_ARRAY_INIT;\n@@ -752,49 +821,15 @@ static void receive_needs(void)\n \t\tif (!line)\n \t\t\tbreak;\n \n-\t\tif (skip_prefix(line, \"shallow \", &arg)) {\n-\t\t\tstruct object_id oid;\n-\t\t\tstruct object *object;\n-\t\t\tif (get_oid_hex(arg, &oid))\n-\t\t\t\tdie(\"invalid shallow line: %s\", line);\n-\t\t\tobject = parse_object(&oid);\n-\t\t\tif (!object)\n-\t\t\t\tcontinue;\n-\t\t\tif (object->type != OBJ_COMMIT)\n-\t\t\t\tdie(\"invalid shallow object %s\", oid_to_hex(&oid));\n-\t\t\tif (!(object->flags & CLIENT_SHALLOW)) {\n-\t\t\t\tobject->flags |= CLIENT_SHALLOW;\n-\t\t\t\tadd_object_array(object, NULL, &shallows);\n-\t\t\t}\n+\t\tif (process_shallow(line, &shallows))\n \t\t\tcontinue;\n-\t\t}\n-\t\tif (skip_prefix(line, \"deepen \", &arg)) {\n-\t\t\tchar *end = NULL;\n-\t\t\tdepth = strtol(arg, &end, 0);\n-\t\t\tif (!end || *end || depth <= 0)\n-\t\t\t\tdie(\"Invalid deepen: %s\", line);\n+\t\tif (process_deepen(line, &depth))\n \t\t\tcontinue;\n-\t\t}\n-\t\tif (skip_prefix(line, \"deepen-since \", &arg)) {\n-\t\t\tchar *end = NULL;\n-\t\t\tdeepen_since = parse_timestamp(arg, &end, 0);\n-\t\t\tif (!end || *end || !deepen_since ||\n-\t\t\t /* revisions.c's max_age -1 is special */\n-\t\t\t deepen_since == -1)\n-\t\t\t\tdie(\"Invalid deepen-since: %s\", line);\n-\t\t\tdeepen_rev_list = 1;\n+\t\tif (process_deepen_since(line, &deepen_since, &deepen_rev_list))\n \t\t\tcontinue;\n-\t\t}\n-\t\tif (skip_prefix(line, \"deepen-not \", &arg)) {\n-\t\t\tchar *ref = NULL;\n-\t\t\tstruct object_id oid;\n-\t\t\tif (expand_ref(arg, strlen(arg), oid.hash, &ref) != 1)\n-\t\t\t\tdie(\"git upload-pack: ambiguous deepen-not: %s\", line);\n-\t\t\tstring_list_append(&deepen_not, ref);\n-\t\t\tfree(ref);\n-\t\t\tdeepen_rev_list = 1;\n+\t\tif (process_deepen_not(line, &deepen_not, &deepen_rev_list))\n \t\t\tcontinue;\n-\t\t}\n+\n \t\tif (!skip_prefix(line, \"want \", &arg) ||\n \t\t get_oid_hex(arg, &oid_buf))\n \t\t\tdie(\"git upload-pack: protocol error, \"\n-- \n2.15.1.424.g9478a66081-goog\n\n"},{"id":"334277","messageId":"xmqqr2s7frga.fsf@gitster.mtv.corp.google.com","threadId":"47006","inReplyTo":"20171204235823.63299-5-bmwill@google.com","subject":"Re: [WIP 04/15] upload-pack: convert to a builtin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-06T21:59:33Z","receivedAt":"2017-12-06T21:59:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> In order to allow for code sharing with the server-side of fetch in\n> protocol-v2 convert upload-pack to be a builtin.\n>\n> Signed-off-by: Brandon Williams \n> ---\n\nThis looks obvious and straight-forward to a cursory look.\n\nI vaguely recalled and feared that we on purpose kept this program\nseparate from the rest of the system for a reason, but my mailing\nlist search is coming up empty.\n\n> Makefile | 3 ++-\n> builtin.h | 1 +\n> git.c | 1 +\n> upload-pack.c | 2 +-\n> 4 files changed, 5 insertions(+), 2 deletions(-)\n> ...\n> diff --git a/upload-pack.c b/upload-pack.c\n> index ef99a029c..2d16952a3 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -1033,7 +1033,7 @@ static int upload_pack_config(const char *var, const char *value, void *unused)\n> \treturn parse_hide_refs_config(var, value, \"uploadpack\");\n> }\n> \n> -int cmd_main(int argc, const char **argv)\n> +int cmd_upload_pack(int argc, const char **argv, const char *prefix)\n> {\n> \tconst char *dir;\n> \tint strict = 0;\n\nShouldn't this file be moved to builtin/ directory, though?\n"},{"id":"334282","messageId":"xmqqmv2vfqxh.fsf@gitster.mtv.corp.google.com","threadId":"47006","inReplyTo":"20171204235823.63299-7-bmwill@google.com","subject":"Re: [WIP 06/15] transport: use get_refs_via_connect to get refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-06T22:10:50Z","receivedAt":"2017-12-06T22:10:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> Remove code duplication and use the existing 'get_refs_via_connect()'\n> function to retrieve a remote's heads in 'fetch_refs_via_pack()' and\n> 'git_transport_push()'.\n>\n> Signed-off-by: Brandon Williams \n> ---\n> transport.c | 18 ++++--------------\n> 1 file changed, 4 insertions(+), 14 deletions(-)\n>\n> diff --git a/transport.c b/transport.c\n> index d75ff0514..7c969f285 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -230,12 +230,8 @@ static int fetch_refs_via_pack(struct transport *transport,\n> \targs.cloning = transport->cloning;\n> \targs.update_shallow = data->options.update_shallow;\n> \n> -\tif (!data->got_remote_heads) {\n> -\t\tconnect_setup(transport, 0);\n> -\t\tget_remote_heads(data->fd[0], NULL, 0, &refs_tmp, 0,\n> -\t\t\t\t NULL, &data->shallow);\n> -\t\tdata->got_remote_heads = 1;\n> -\t}\n> +\tif (!data->got_remote_heads)\n> +\t\trefs_tmp = get_refs_via_connect(transport, 0);\n\nThe updated version is equivalent to the original as long as\ntransport->data->extra_have is empty at this point. Were we\ndeliberately sending NULL, instead of &data->extra_have, in the\noriginal, or is it a mere oversight?\n\nThe same comment applies to the other hunk of this patch.\n"},{"id":"334283","messageId":"xmqqh8t3fpm7.fsf@gitster.mtv.corp.google.com","threadId":"47006","inReplyTo":"20171204235823.63299-8-bmwill@google.com","subject":"Re: [WIP 07/15] connect: convert get_remote_heads to use struct packet_reader","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-06T22:39:12Z","receivedAt":"2017-12-06T22:49:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> @@ -56,6 +62,41 @@ static void die_initial_contact(int unexpected)\n> \t\t \"and the repository exists.\"));\n> }\n> \n> +static enum protocol_version discover_version(struct packet_reader *reader)\n> +{\n> +\tenum protocol_version version = protocol_unknown_version;\n> +\n> +\t/*\n> +\t * Peek the first line of the server's response to\n> +\t * determine the protocol version the server is speaking.\n> +\t */\n> +\tswitch (packet_reader_peek(reader)) {\n> +\tcase PACKET_READ_ERROR:\n> +\t\tdie_initial_contact(0);\n> +\tcase PACKET_READ_FLUSH:\n> +\tcase PACKET_READ_DELIM:\n> +\t\tversion = protocol_v0;\n> +\t\tbreak;\n> +\tcase PACKET_READ_NORMAL:\n> +\t\tversion = determine_protocol_version_client(reader->line);\n> +\t\tbreak;\n> +\t}\n> +\n> +\t/* Maybe process capabilities here, at least for v2 */\n> +\tswitch (version) {\n> +\tcase protocol_v1:\n> +\t\t/* Read the peeked version line */\n> +\t\tpacket_reader_read(reader);\n> +\t\tbreak;\n> +\tcase protocol_v0:\n> +\t\tbreak;\n> +\tcase protocol_unknown_version:\n> +\t\tBUG(\"ERROR\");\n> +\t}\n> +\n> +\treturn version;\n> +}\n> +\n\nThis discovers and consumes the \"version\" thing, but for an older\nprotocol that does not have the \"version\" thing, it does not clobber\nthe first \"ref\", thanks to its use of peek. Makes sense.\n\n> +#define EXPECTING_FIRST_REF 0\n> +#define EXPECTING_REF 1\n> +#define EXPECTING_SHALLOW 2\n> +#define EXPECTING_DONE 3\n> \n> static void process_capabilities(int *len)\n> {\n> @@ -230,28 +237,34 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,\n> \t\t\t struct oid_array *shallow_points)\n> {\n> \tstruct ref **orig_list = list;\n> +\tint len = 0;\n> +\tint state = EXPECTING_FIRST_REF;\n> +\tstruct packet_reader reader;\n> +\tconst char *arg;\n> \n> -\t/*\n> -\t * A hang-up after seeing some response from the other end\n> -\t * means that it is unexpected, as we know the other end is\n> -\t * willing to talk to us. A hang-up before seeing any\n> -\t * response does not necessarily mean an ACL problem, though.\n> -\t */\n> -\tint responded = 0;\n> -\tint len;\n> -\tint state = EXPECTING_PROTOCOL_VERSION;\n> +\tpacket_reader_init(&reader, in, src_buf, src_len);\n> +\n> +\tdiscover_version(&reader);\n> \n> \t*list = NULL;\n\nAnd thanks to the \"peeking\" implementation, the version handling is\ncleanly separated from the rest of the exchange, which is good.\n\n> -\twhile ((len = read_remote_ref(in, &src_buf, &src_len, &responded))) {\n> +\twhile (state != EXPECTING_DONE) {\n> +\t\tswitch (packet_reader_read(&reader)) {\n> +\t\tcase PACKET_READ_ERROR:\n> +\t\t\tdie_initial_contact(1);\n> +\t\tcase PACKET_READ_NORMAL:\n> +\t\t\tlen = reader.pktlen;\n> +\t\t\tif (len > 4 && skip_prefix(packet_buffer, \"ERR \", &arg))\n> +\t\t\t\tdie(\"remote error: %s\", arg);\n> +\t\t\tbreak;\n> +\t\tcase PACKET_READ_FLUSH:\n> +\t\t\tstate = EXPECTING_DONE;\n> +\t\t\tbreak;\n> +\t\tcase PACKET_READ_DELIM:\n> +\t\t\tdie(\"invalid packet\\n\");\n> +\t\t}\n> +\n\nEXPECTING_DONE sounded like we are expecting to see 'done' packet\nsent from the other side, but I was mistaken. It is the state\nwhere we are \"done\" expecting anything ;-).\n\nHaving an (unconditional) assignment to 'state' in the above switch\nmakes me feel somewhat uneasy, as the next \"switch (state)\" is what\nis meant as the state machine that would allow us to say things like\n\"from this state, transition to that state is impossible\". When we\nget a flush while we are expecting the first ref, for example, we'd\njust go into the \"done\" state. There is no provision for a future\nupdate to say \"no, getting a flush in this state is an error\".\n\nThat is no different from the current code; when read_remote_ref()\nnotices that it got a flush, it just leaves the loop without even\ntouching 'state' variable. But at least, I find that the current\ncode is more honest---it does not even touch 'state' and allows the\ncode after the loop to inspect it, if needed. From that point of\nvhew, the way the new code uses 'state' to leave the loop upon\nseeing a flush is a regression---it makes it harder to notice and\nreport when we got a flush in a wrong state.\n\nPerhaps getting rid of \"EXPECTING_DONE\" from the enum and then:\n\n\tint got_flush = 0;\n\twhile (1) {\n\t\tswitch (reader_read()) {\n\t\tcase PACKET_READ_FLUSH:\n\t\t\tgot_flush = 1;\n\t\t\tbreak;\n\t\t... other cases ...\n\t\t}\n\n\t\tif (got_flush)\n\t\t\tbreak;\n\n\t\tswitch (state) {\n\t\t... current code ...\n\t\t}\n\t}\n\nwould be an improvement; we can later extend \"if (got_flush)\" part\nto check what state we are in if we wanted to notice and report an\nerror there before breaking out of the loop.\n\n"},{"id":"334328","messageId":"alpine.DEB.2.21.1.1712071658120.4318@virtualbox","threadId":"47006","inReplyTo":"xmqqr2s7frga.fsf@gitster.mtv.corp.google.com","subject":"Re: [WIP 04/15] upload-pack: convert to a builtin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-12-07T16:14:46Z","receivedAt":"2017-12-07T16:15:42Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 6 Dec 2017, Junio C Hamano wrote:\n\n> Brandon Williams writes:\n> \n> > In order to allow for code sharing with the server-side of fetch in\n> > protocol-v2 convert upload-pack to be a builtin.\n> >\n> > Signed-off-by: Brandon Williams \n> > ---\n> \n> This looks obvious and straight-forward to a cursory look.\n> \n> I vaguely recalled and feared that we on purpose kept this program\n> separate from the rest of the system for a reason, but my mailing\n> list search is coming up empty.\n\nI only recall that we kept it in the bin/ directory (as opposed to\nmlibexec/git-core/) to help with fetching via SSH.\n\nCiao,\nDscho\n"},{"id":"334342","messageId":"20171207184009.GA152943@google.com","threadId":"47006","inReplyTo":"xmqqmv2vfqxh.fsf@gitster.mtv.corp.google.com","subject":"Re: [WIP 06/15] transport: use get_refs_via_connect to get refs","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-07T18:40:09Z","receivedAt":"2017-12-07T18:41:43Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 12/06, Junio C Hamano wrote:\n> Brandon Williams writes:\n> \n> > Remove code duplication and use the existing 'get_refs_via_connect()'\n> > function to retrieve a remote's heads in 'fetch_refs_via_pack()' and\n> > 'git_transport_push()'.\n> >\n> > Signed-off-by: Brandon Williams \n> > ---\n> > transport.c | 18 ++++--------------\n> > 1 file changed, 4 insertions(+), 14 deletions(-)\n> >\n> > diff --git a/transport.c b/transport.c\n> > index d75ff0514..7c969f285 100644\n> > --- a/transport.c\n> > +++ b/transport.c\n> > @@ -230,12 +230,8 @@ static int fetch_refs_via_pack(struct transport *transport,\n> > \targs.cloning = transport->cloning;\n> > \targs.update_shallow = data->options.update_shallow;\n> > \n> > -\tif (!data->got_remote_heads) {\n> > -\t\tconnect_setup(transport, 0);\n> > -\t\tget_remote_heads(data->fd[0], NULL, 0, &refs_tmp, 0,\n> > -\t\t\t\t NULL, &data->shallow);\n> > -\t\tdata->got_remote_heads = 1;\n> > -\t}\n> > +\tif (!data->got_remote_heads)\n> > +\t\trefs_tmp = get_refs_via_connect(transport, 0);\n> \n> The updated version is equivalent to the original as long as\n> transport->data->extra_have is empty at this point. Were we\n> deliberately sending NULL, instead of &data->extra_have, in the\n> original, or is it a mere oversight?\n> \n> The same comment applies to the other hunk of this patch.\n\nextra_have is only ever used by the push logic, so they're shouldn't be\nany harm is passing it through on the fetch side, especially since\nupload-pack doesn't send .have lines.\n\nThe push side is what uses the .have lines. From a quick look through\nthe code it seems like get_refs_via_connect is always called before\ngit_transport_push, so the extra check to make sure ref's have been\nretrieved doesn't get executed. But if it ever did get executed, we\nwould silently ignore a server's .have lines.\n\n-- \nBrandon Williams\n"},{"id":"334343","messageId":"xmqq7etye5il.fsf@gitster.mtv.corp.google.com","threadId":"47006","inReplyTo":"20171204235823.63299-9-bmwill@google.com","subject":"Re: [WIP 08/15] connect: discover protocol version outside of get_remote_heads","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-07T18:50:58Z","receivedAt":"2017-12-07T18:51:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> @@ -193,7 +195,17 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n> \t\tif (!conn)\n> \t\t\treturn args.diag_url ? 0 : 1;\n> \t}\n> -\tget_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);\n> +\n> +\tpacket_reader_init(&reader, fd[0], NULL, 0);\n> +\n> +\tswitch (discover_version(&reader)) {\n> +\tcase protocol_v1:\n> +\tcase protocol_v0:\n> +\t\tget_remote_heads(&reader, &ref, 0, NULL, &shallow);\n> +\t\tbreak;\n> +\tcase protocol_unknown_version:\n> +\t\tBUG(\"unknown protocol version\");\n> +\t}\n\nWe see quite a few hunks just like this one appear in this patch.\nThe repetition is a bit disturbing. I wonder if we want a wrapper\naround the \"reader-init && discover-version && return an error if\nthe protocol version is not known\" dance. That would allow us to\nwrite this part of the code like so:\n\n\tstruct packet_reader reader;\n\n\tif (connection_preamble(&reader, fd[0]))\n\t\tdie(\"unknown protocol version\");\n\tget_remote_heads(&reader, &ref, 0, NULL, &shallow);\n\nor something like that.\n\nBy the way, is that really a BUG()? Getting a connection and the\nother end declaring a protocol version you do not yet understand is\nnot a bug in your program---it is a normal runtime error, no?\n"},{"id":"334347","messageId":"20171207190446.GB152943@google.com","threadId":"47006","inReplyTo":"xmqq7etye5il.fsf@gitster.mtv.corp.google.com","subject":"Re: [WIP 08/15] connect: discover protocol version outside of get_remote_heads","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-07T19:04:46Z","receivedAt":"2017-12-07T19:04:54Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 12/07, Junio C Hamano wrote:\n> Brandon Williams writes:\n> \n> > @@ -193,7 +195,17 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n> > \t\tif (!conn)\n> > \t\t\treturn args.diag_url ? 0 : 1;\n> > \t}\n> > -\tget_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);\n> > +\n> > +\tpacket_reader_init(&reader, fd[0], NULL, 0);\n> > +\n> > +\tswitch (discover_version(&reader)) {\n> > +\tcase protocol_v1:\n> > +\tcase protocol_v0:\n> > +\t\tget_remote_heads(&reader, &ref, 0, NULL, &shallow);\n> > +\t\tbreak;\n> > +\tcase protocol_unknown_version:\n> > +\t\tBUG(\"unknown protocol version\");\n> > +\t}\n> \n> We see quite a few hunks just like this one appear in this patch.\n> The repetition is a bit disturbing. I wonder if we want a wrapper\n> around the \"reader-init && discover-version && return an error if\n> the protocol version is not known\" dance. That would allow us to\n> write this part of the code like so:\n> \n> \tstruct packet_reader reader;\n> \n> \tif (connection_preamble(&reader, fd[0]))\n> \t\tdie(\"unknown protocol version\");\n> \tget_remote_heads(&reader, &ref, 0, NULL, &shallow);\n> \n> or something like that.\n> \n> By the way, is that really a BUG()? Getting a connection and the\n> other end declaring a protocol version you do not yet understand is\n> not a bug in your program---it is a normal runtime error, no?\n\nWhile we could wrap the preamble into a function it sort of defeats the\npurpose since you want to be able to call different functions based on\nthe protocol version you're speaking. That way you can have hard\nseparations between the code paths which operate on v0/v1 and v2.\n\nAs for that case being a BUG, yes it should be a BUG. the\ndiscover_version function won't ever return a protocol_unknown_version\nvalue. Its only there to make sure the switch cases are exhaustive and\ncover all the enum values. That does bring up a good point though.\nThis error should be handled as a run-time error and not a BUG in\ndiscover_version, which I'll fix.\n\n\n-- \nBrandon Williams\n"},{"id":"334353","messageId":"xmqqy3mecp4x.fsf@gitster.mtv.corp.google.com","threadId":"47006","inReplyTo":"20171207190446.GB152943@google.com","subject":"Re: [WIP 08/15] connect: discover protocol version outside of get_remote_heads","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-07T19:30:06Z","receivedAt":"2017-12-07T19:30:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> While we could wrap the preamble into a function it sort of defeats the\n> purpose since you want to be able to call different functions based on\n> the protocol version you're speaking. That way you can have hard\n> separations between the code paths which operate on v0/v1 and v2.\n\nAs I envision that the need for different processing across protocol\nversions in real code would become far greater than it would fit in\ncases within a single switch() statement, I imagined that the reader\nstructure would have a field that says which version of the protocol\nit is, so that the caller of the preamble thing can inspect it later\nto switch on it.\n\n"},{"id":"334366","messageId":"CAGZ79kbV07BsKde1P_3EHjnSj7APTMQk+bbGx79X0nvpU2LD+Q@mail.gmail.com","threadId":"47006","inReplyTo":"20171204235823.63299-2-bmwill@google.com","subject":"Re: [WIP 01/15] pkt-line: introduce packet_read_with_status","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-07T20:53:47Z","receivedAt":"2017-12-07T20:54:03Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Dec 4, 2017 at 3:58 PM, Brandon Williams wrote:\n\n> diff --git a/pkt-line.h b/pkt-line.h\n> index 3dad583e2..f1545929b 100644\n> --- a/pkt-line.h\n> +++ b/pkt-line.h\n> @@ -60,8 +60,16 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);\n> * If options contains PACKET_READ_CHOMP_NEWLINE, a trailing newline (if\n> * present) is removed from the buffer before returning.\n> */\n> +enum packet_read_status {\n> + PACKET_READ_ERROR = -1,\n> + PACKET_READ_NORMAL,\n> + PACKET_READ_FLUSH,\n> +};\n> #define PACKET_READ_GENTLE_ON_EOF (1u<<0)\n> #define PACKET_READ_CHOMP_NEWLINE (1u<<1)\n> +enum packet_read_status packet_read_with_status(int fd, char **src_buffer, size_t *src_len,\n> + char *buffer, unsigned size, int *pktlen,\n> + int options);\n> int packet_read(int fd, char **src_buffer, size_t *src_len, char\n> *buffer, unsigned size, int options);\n\nThe documentation that is preceding these lines is very specific to\npacket_read, e.g.\n\n If options does contain PACKET_READ_GENTLE_ON_EOF,\n we will not die on condition 4 (truncated input), but instead return -1\n\nwhich doesn't hold true for the _status version. Can you adapt the comment\nto explain both read functions?\n"},{"id":"334384","messageId":"CAGZ79kY5dCo1qO-4W=xdZQMjoJs1ZqJdywUsqX0CjyXHu80hCw@mail.gmail.com","threadId":"47006","inReplyTo":"20171204235823.63299-3-bmwill@google.com","subject":"Re: [WIP 02/15] pkt-line: introduce struct packet_reader","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-07T22:01:58Z","receivedAt":"2017-12-07T22:02:04Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Dec 4, 2017 at 3:58 PM, Brandon Williams wrote:\n> Sometimes it is advantageous to be able to peek the next packet line\n> without consuming it (e.g. to be able to determine the protocol version\n> a server is speaking). In order to do that introduce 'struct\n> packet_reader' which is an abstraction around the normal packet reading\n> logic. This enables a caller to be able to peek a single line at a time\n> using 'packet_reader_peek()' and having a caller consume a line by\n> calling 'packet_reader_read()'.\n>\n> Signed-off-by: Brandon Williams \n> ---\n> pkt-line.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n> pkt-line.h | 20 ++++++++++++++++++++\n> 2 files changed, 81 insertions(+)\n>\n> diff --git a/pkt-line.c b/pkt-line.c\n> index ac619f05b..518109bbe 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -406,3 +406,64 @@ ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out)\n> }\n> return sb_out->len - orig_len;\n> }\n> +\n> +/* Packet Reader Functions */\n> +void packet_reader_init(struct packet_reader *reader, int fd,\n> + char *src_buffer, size_t src_len)\n> +{\n> + reader->fd = fd;\n> + reader->src_buffer = src_buffer;\n> + reader->src_len = src_len;\n> + reader->buffer = packet_buffer;\n> + reader->buffer_size = sizeof(packet_buffer);\n> + reader->options = PACKET_READ_CHOMP_NEWLINE | PACKET_READ_GENTLE_ON_EOF;\n\nI assume the future user of this packet reader will need exactly\nthese options coincidentally. ;)\n\nI think it might be ok for now and later we can extend the reader if needed\nto also take the flags as args. However given this set of args, this is a gentle\npacket reader, as it corresponds to the _gently version of reading\npackets AFAICT.\n\nUnlike the pkt_read function this constructor of a packet reader doesn't need\nthe arguments for its buffer (packet_buffer and sizeof thereof), which\npacket_read\nunfortunately needs. We pass in packet_buffer all the time except in\nbuiltin/receive-pack\nfor obtaining the gpg cert. I think that's ok here.\n\n> +enum packet_read_status packet_reader_read(struct packet_reader *reader)\n> +{\n> + if (reader->line_peeked) {\n> + reader->line_peeked = 0;\n> + return reader->status;\n> + }\n> +\n> + reader->status = packet_read_with_status(reader->fd,\n> + &reader->src_buffer,\n> + &reader->src_len,\n> + reader->buffer,\n> + reader->buffer_size,\n> + &reader->pktlen,\n> + reader->options);\n> +\n> + switch (reader->status) {\n> + case PACKET_READ_ERROR:\n> + reader->pktlen = -1;\n\nIn case of error the read function might not\nhave assigned the pktlen as requested, so we assign\nit to -1/NULL here. Though the caller ought to already know\nthat they handle bogus, as the state is surely the first thing\nthey'd inspect?\n\n> + reader->line = NULL;\n> + break;\n> + case PACKET_READ_NORMAL:\n> + reader->line = reader->buffer;\n> + break;\n> + case PACKET_READ_FLUSH:\n> + reader->pktlen = 0;\n> + reader->line = NULL;\n> + break;\n> + }\n\nOh, this gives an interesting interface for someone who is\njust inspecting the len/line instead of the state, so it might be\nworth keeping it this way.\n\nCan we have an API documentation in the header file,\nexplaining what to expect in each field given the state\nof the (read, peaked) packet?\n"},{"id":"334387","messageId":"CAGZ79kYw3y3grehEbFeoA2r7pEra079M7zVpcVwxcoK5BPN1Dw@mail.gmail.com","threadId":"47006","inReplyTo":"20171204235823.63299-4-bmwill@google.com","subject":"Re: [WIP 03/15] pkt-line: add delim packet support","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-07T22:30:33Z","receivedAt":"2017-12-07T22:30:42Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Dec 4, 2017 at 3:58 PM, Brandon Williams wrote:\n> One of the design goals of protocol-v2 is to improve the semantics of\n> flush packets. Currently in protocol-v1, flush packets are used both to\n> indicate a break in a list of packet lines as well as an indication that\n> one side has finished speaking. This makes it particularly difficult\n> to implement proxies as a proxy would need to completely understand git\n> protocol instead of simply looking for a flush packet.\n>\n> To do this, introduce the special deliminator packet '0001'. A delim\n> packet can then be used as a deliminator between lists of packet lines\n> while flush packets can be reserved to indicate the end of a response.\n>\n> Signed-off-by: Brandon Williams \n\nI presume the update for Documentation/technical/* comes at a later patch in the\nseries, clarifying the exact semantic difference between the packet types?\n"},{"id":"334391","messageId":"xmqq8teeaywm.fsf@gitster.mtv.corp.google.com","threadId":"47006","inReplyTo":"20171204235823.63299-12-bmwill@google.com","subject":"Re: [WIP 11/15] serve: introduce git-serve","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-07T23:42:01Z","receivedAt":"2017-12-07T23:42:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> +static struct protocol_capability *get_capability(const char *key)\n> +{\n> +\tint i;\n> +\n> +\tif (!key)\n> +\t\treturn NULL;\n> +\n> +\tfor (i = 0; i < ARRAY_SIZE(capabilities); i++) {\n> +\t\tstruct protocol_capability *c = &capabilities[i];\n> +\t\tconst char *out;\n> +\t\tif (skip_prefix(key, c->name, &out) && (!*out || *out == '='))\n> +\t\t\treturn c;\n\nLooks familiar and resembles what was recently discussed on list ;-)\n\n> +int cmd_serve(int argc, const char **argv, const char *prefix)\n> +{\n> +\n> +\tstruct option options[] = {\n> +\t\tOPT_END()\n> +\t};\n> +\n> +\t/* ignore all unknown cmdline switches for now */\n> +\targc = parse_options(argc, argv, prefix, options, grep_usage,\n> +\t\t\t PARSE_OPT_KEEP_DASHDASH |\n> +\t\t\t PARSE_OPT_KEEP_UNKNOWN);\n> +\tserve();\n> +\n> +\treturn 0;\n> +}\n> ...\n> +/* Main serve loop for protocol version 2 */\n> +void serve(void)\n> +{\n> +\t/* serve by default supports v2 */\n> +\tpacket_write_fmt(1, \"version 2\\n\");\n> +\n> +\tadvertise_capabilities();\n> +\n> +\tfor (;;)\n> +\t\tif (process_request())\n> +\t\t\tbreak;\n> +}\n\nI am guessing that this would be run just like upload-pack,\ni.e. invoked via ssh or via git-daemon, and that is why it can just\nassume that fd#0/fd#1 are already connected to the other end. It\nmay be helpful to document somewhere how we envision to invoke this\nprogram.\n\n"},{"id":"334471","messageId":"20171208180334.GF152943@google.com","threadId":"47006","inReplyTo":"CAGZ79kbV07BsKde1P_3EHjnSj7APTMQk+bbGx79X0nvpU2LD+Q@mail.gmail.com","subject":"Re: [WIP 01/15] pkt-line: introduce packet_read_with_status","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-08T18:03:34Z","receivedAt":"2017-12-08T18:03:42Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 12/07, Stefan Beller wrote:\n> On Mon, Dec 4, 2017 at 3:58 PM, Brandon Williams wrote:\n> \n> > diff --git a/pkt-line.h b/pkt-line.h\n> > index 3dad583e2..f1545929b 100644\n> > --- a/pkt-line.h\n> > +++ b/pkt-line.h\n> > @@ -60,8 +60,16 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);\n> > * If options contains PACKET_READ_CHOMP_NEWLINE, a trailing newline (if\n> > * present) is removed from the buffer before returning.\n> > */\n> > +enum packet_read_status {\n> > + PACKET_READ_ERROR = -1,\n> > + PACKET_READ_NORMAL,\n> > + PACKET_READ_FLUSH,\n> > +};\n> > #define PACKET_READ_GENTLE_ON_EOF (1u<<0)\n> > #define PACKET_READ_CHOMP_NEWLINE (1u<<1)\n> > +enum packet_read_status packet_read_with_status(int fd, char **src_buffer, size_t *src_len,\n> > + char *buffer, unsigned size, int *pktlen,\n> > + int options);\n> > int packet_read(int fd, char **src_buffer, size_t *src_len, char\n> > *buffer, unsigned size, int options);\n> \n> The documentation that is preceding these lines is very specific to\n> packet_read, e.g.\n> \n> If options does contain PACKET_READ_GENTLE_ON_EOF,\n> we will not die on condition 4 (truncated input), but instead return -1\n> \n> which doesn't hold true for the _status version. Can you adapt the comment\n> to explain both read functions?\n\nGood point, I'll makes changes and document the _status version\nseparately.\n\n-- \nBrandon Williams\n"},{"id":"334473","messageId":"20171208181143.GG152943@google.com","threadId":"47006","inReplyTo":"CAGZ79kY5dCo1qO-4W=xdZQMjoJs1ZqJdywUsqX0CjyXHu80hCw@mail.gmail.com","subject":"Re: [WIP 02/15] pkt-line: introduce struct packet_reader","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-08T18:11:43Z","receivedAt":"2017-12-08T18:11:53Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 12/07, Stefan Beller wrote:\n> On Mon, Dec 4, 2017 at 3:58 PM, Brandon Williams wrote:\n> > Sometimes it is advantageous to be able to peek the next packet line\n> > without consuming it (e.g. to be able to determine the protocol version\n> > a server is speaking). In order to do that introduce 'struct\n> > packet_reader' which is an abstraction around the normal packet reading\n> > logic. This enables a caller to be able to peek a single line at a time\n> > using 'packet_reader_peek()' and having a caller consume a line by\n> > calling 'packet_reader_read()'.\n> >\n> > Signed-off-by: Brandon Williams \n> > ---\n> > pkt-line.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n> > pkt-line.h | 20 ++++++++++++++++++++\n> > 2 files changed, 81 insertions(+)\n> >\n> > diff --git a/pkt-line.c b/pkt-line.c\n> > index ac619f05b..518109bbe 100644\n> > --- a/pkt-line.c\n> > +++ b/pkt-line.c\n> > @@ -406,3 +406,64 @@ ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out)\n> > }\n> > return sb_out->len - orig_len;\n> > }\n> > +\n> > +/* Packet Reader Functions */\n> > +void packet_reader_init(struct packet_reader *reader, int fd,\n> > + char *src_buffer, size_t src_len)\n> > +{\n> > + reader->fd = fd;\n> > + reader->src_buffer = src_buffer;\n> > + reader->src_len = src_len;\n> > + reader->buffer = packet_buffer;\n> > + reader->buffer_size = sizeof(packet_buffer);\n> > + reader->options = PACKET_READ_CHOMP_NEWLINE | PACKET_READ_GENTLE_ON_EOF;\n> \n> I assume the future user of this packet reader will need exactly\n> these options coincidentally. ;)\n> \n> I think it might be ok for now and later we can extend the reader if needed\n> to also take the flags as args. However given this set of args, this is a gentle\n> packet reader, as it corresponds to the _gently version of reading\n> packets AFAICT.\n> \n> Unlike the pkt_read function this constructor of a packet reader doesn't need\n> the arguments for its buffer (packet_buffer and sizeof thereof), which\n> packet_read\n> unfortunately needs. We pass in packet_buffer all the time except in\n> builtin/receive-pack\n> for obtaining the gpg cert. I think that's ok here.\n\nYeah, all of the callers I wanted to migrate all passed the same flags\nand same buffer. I figured there may be a point in the future where\nwe'd want to extend this so instead of hard coding the flags in the read\nfunctions, I did so in the constructor. That way if we wanted to extend\nit in the future, it would be a little bit easier.\n\n> \n> > +enum packet_read_status packet_reader_read(struct packet_reader *reader)\n> > +{\n> > + if (reader->line_peeked) {\n> > + reader->line_peeked = 0;\n> > + return reader->status;\n> > + }\n> > +\n> > + reader->status = packet_read_with_status(reader->fd,\n> > + &reader->src_buffer,\n> > + &reader->src_len,\n> > + reader->buffer,\n> > + reader->buffer_size,\n> > + &reader->pktlen,\n> > + reader->options);\n> > +\n> > + switch (reader->status) {\n> > + case PACKET_READ_ERROR:\n> > + reader->pktlen = -1;\n> \n> In case of error the read function might not\n> have assigned the pktlen as requested, so we assign\n> it to -1/NULL here. Though the caller ought to already know\n> that they handle bogus, as the state is surely the first thing\n> they'd inspect?\n\nExactly, I thought it would be easier to ensure that pktlen and line\nwere had consistent state no matter what the output of the read was.\nBut yes, callers should be looking at the status to determine what to\ndo.\n\n> \n> > + reader->line = NULL;\n> > + break;\n> > + case PACKET_READ_NORMAL:\n> > + reader->line = reader->buffer;\n> > + break;\n> > + case PACKET_READ_FLUSH:\n> > + reader->pktlen = 0;\n> > + reader->line = NULL;\n> > + break;\n> > + }\n> \n> Oh, this gives an interesting interface for someone who is\n> just inspecting the len/line instead of the state, so it might be\n> worth keeping it this way.\n> \n> Can we have an API documentation in the header file,\n> explaining what to expect in each field given the state\n> of the (read, peaked) packet?\n\nYep I can write some better API docs for these functions.\n\n-- \nBrandon Williams\n"},{"id":"334484","messageId":"20171208200850.GA140529@google.com","threadId":"47006","inReplyTo":"CAGZ79kYw3y3grehEbFeoA2r7pEra079M7zVpcVwxcoK5BPN1Dw@mail.gmail.com","subject":"Re: [WIP 03/15] pkt-line: add delim packet support","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-08T20:08:50Z","receivedAt":"2017-12-08T20:09:01Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 12/07, Stefan Beller wrote:\n> On Mon, Dec 4, 2017 at 3:58 PM, Brandon Williams wrote:\n> > One of the design goals of protocol-v2 is to improve the semantics of\n> > flush packets. Currently in protocol-v1, flush packets are used both to\n> > indicate a break in a list of packet lines as well as an indication that\n> > one side has finished speaking. This makes it particularly difficult\n> > to implement proxies as a proxy would need to completely understand git\n> > protocol instead of simply looking for a flush packet.\n> >\n> > To do this, introduce the special deliminator packet '0001'. A delim\n> > packet can then be used as a deliminator between lists of packet lines\n> > while flush packets can be reserved to indicate the end of a response.\n> >\n> > Signed-off-by: Brandon Williams \n> \n> I presume the update for Documentation/technical/* comes at a later patch in the\n> series, clarifying the exact semantic difference between the packet types?\n\nYeah, currently there isn't a use for the delim packet but there will be\none when v2 is introduced.\n\n-- \nBrandon Williams\n"},{"id":"334486","messageId":"20171208201108.GB140529@google.com","threadId":"47006","inReplyTo":"xmqqy3mecp4x.fsf@gitster.mtv.corp.google.com","subject":"Re: [WIP 08/15] connect: discover protocol version outside of get_remote_heads","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-08T20:11:08Z","receivedAt":"2017-12-08T20:11:15Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 12/07, Junio C Hamano wrote:\n> Brandon Williams writes:\n> \n> > While we could wrap the preamble into a function it sort of defeats the\n> > purpose since you want to be able to call different functions based on\n> > the protocol version you're speaking. That way you can have hard\n> > separations between the code paths which operate on v0/v1 and v2.\n> \n> As I envision that the need for different processing across protocol\n> versions in real code would become far greater than it would fit in\n> cases within a single switch() statement, I imagined that the reader\n> structure would have a field that says which version of the protocol\n> it is, so that the caller of the preamble thing can inspect it later\n> to switch on it.\n> \n\nYes, patch 9 changes this so that the protocol version is stored in the\ntransport structure. This way the fetch and push code can know what to\ndo in v2.\n\n-- \nBrandon Williams\n"},{"id":"334487","messageId":"20171208201230.GC140529@google.com","threadId":"47006","inReplyTo":"xmqqr2s7frga.fsf@gitster.mtv.corp.google.com","subject":"Re: [WIP 04/15] upload-pack: convert to a builtin","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-08T20:12:30Z","receivedAt":"2017-12-08T20:12:38Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 12/06, Junio C Hamano wrote:\n> Brandon Williams writes:\n> \n> > In order to allow for code sharing with the server-side of fetch in\n> > protocol-v2 convert upload-pack to be a builtin.\n> >\n> > Signed-off-by: Brandon Williams \n> > ---\n> \n> This looks obvious and straight-forward to a cursory look.\n> \n> I vaguely recalled and feared that we on purpose kept this program\n> separate from the rest of the system for a reason, but my mailing\n> list search is coming up empty.\n> \n> > Makefile | 3 ++-\n> > builtin.h | 1 +\n> > git.c | 1 +\n> > upload-pack.c | 2 +-\n> > 4 files changed, 5 insertions(+), 2 deletions(-)\n> > ...\n> > diff --git a/upload-pack.c b/upload-pack.c\n> > index ef99a029c..2d16952a3 100644\n> > --- a/upload-pack.c\n> > +++ b/upload-pack.c\n> > @@ -1033,7 +1033,7 @@ static int upload_pack_config(const char *var, const char *value, void *unused)\n> > \treturn parse_hide_refs_config(var, value, \"uploadpack\");\n> > }\n> > \n> > -int cmd_main(int argc, const char **argv)\n> > +int cmd_upload_pack(int argc, const char **argv, const char *prefix)\n> > {\n> > \tconst char *dir;\n> > \tint strict = 0;\n> \n> Shouldn't this file be moved to builtin/ directory, though?\n\nI can definitely move the file to builtin if you would prefer.\n\n-- \nBrandon Williams\n"},{"id":"334489","messageId":"20171208201930.GD140529@google.com","threadId":"47006","inReplyTo":"xmqqh8t3fpm7.fsf@gitster.mtv.corp.google.com","subject":"Re: [WIP 07/15] connect: convert get_remote_heads to use struct packet_reader","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-08T20:19:30Z","receivedAt":"2017-12-08T20:19:39Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 12/06, Junio C Hamano wrote:\n> Brandon Williams writes:\n> \n> \n> EXPECTING_DONE sounded like we are expecting to see 'done' packet\n> sent from the other side, but I was mistaken. It is the state\n> where we are \"done\" expecting anything ;-).\n> \n> Having an (unconditional) assignment to 'state' in the above switch\n> makes me feel somewhat uneasy, as the next \"switch (state)\" is what\n> is meant as the state machine that would allow us to say things like\n> \"from this state, transition to that state is impossible\". When we\n> get a flush while we are expecting the first ref, for example, we'd\n> just go into the \"done\" state. There is no provision for a future\n> update to say \"no, getting a flush in this state is an error\".\n\nI believe this is accepted behavior, receiving a flush in that state.\nAnd I don't think there is ever an instance during the ref advertisement\nwhere a flush would be an error. It just indicates that the\nadvertisement is finished.\n\n> \n> That is no different from the current code; when read_remote_ref()\n> notices that it got a flush, it just leaves the loop without even\n> touching 'state' variable. But at least, I find that the current\n> code is more honest---it does not even touch 'state' and allows the\n> code after the loop to inspect it, if needed. From that point of\n> vhew, the way the new code uses 'state' to leave the loop upon\n> seeing a flush is a regression---it makes it harder to notice and\n> report when we got a flush in a wrong state.\n> \n> Perhaps getting rid of \"EXPECTING_DONE\" from the enum and then:\n> \n> \tint got_flush = 0;\n> \twhile (1) {\n> \t\tswitch (reader_read()) {\n> \t\tcase PACKET_READ_FLUSH:\n> \t\t\tgot_flush = 1;\n> \t\t\tbreak;\n> \t\t... other cases ...\n> \t\t}\n> \n> \t\tif (got_flush)\n> \t\t\tbreak;\n> \n> \t\tswitch (state) {\n> \t\t... current code ...\n> \t\t}\n> \t}\n> \n> would be an improvement; we can later extend \"if (got_flush)\" part\n> to check what state we are in if we wanted to notice and report an\n> error there before breaking out of the loop.\n> \n\nI don't really see how this is any clearer from what this patch does\nthough. I thought it made it easier to read as you no longer have an\ninfinite loop, but rather know when it will end (when you move to the\ndone state).\n\n-- \nBrandon Williams\n"},{"id":"334491","messageId":"20171208202530.GE140529@google.com","threadId":"47006","inReplyTo":"xmqq8teeaywm.fsf@gitster.mtv.corp.google.com","subject":"Re: [WIP 11/15] serve: introduce git-serve","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-08T20:25:30Z","receivedAt":"2017-12-08T20:25:39Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 12/07, Junio C Hamano wrote:\n> Brandon Williams writes:\n> \n> > +static struct protocol_capability *get_capability(const char *key)\n> > +{\n> > +\tint i;\n> > +\n> > +\tif (!key)\n> > +\t\treturn NULL;\n> > +\n> > +\tfor (i = 0; i < ARRAY_SIZE(capabilities); i++) {\n> > +\t\tstruct protocol_capability *c = &capabilities[i];\n> > +\t\tconst char *out;\n> > +\t\tif (skip_prefix(key, c->name, &out) && (!*out || *out == '='))\n> > +\t\t\treturn c;\n> \n> Looks familiar and resembles what was recently discussed on list ;-)\n> \n> > +int cmd_serve(int argc, const char **argv, const char *prefix)\n> > +{\n> > +\n> > +\tstruct option options[] = {\n> > +\t\tOPT_END()\n> > +\t};\n> > +\n> > +\t/* ignore all unknown cmdline switches for now */\n> > +\targc = parse_options(argc, argv, prefix, options, grep_usage,\n> > +\t\t\t PARSE_OPT_KEEP_DASHDASH |\n> > +\t\t\t PARSE_OPT_KEEP_UNKNOWN);\n> > +\tserve();\n> > +\n> > +\treturn 0;\n> > +}\n\nI assume that at some point we may want to have a new endpoint that just\ndoes v2 without needing the side channel to tell it to do so. Maybe for\nbrand new server commands, like a remote grep or a remote object-stat or\nsomething that don't have a v1 equivalent that can be fallen back to.\nThat's why I included a builtin/serve.c \n\n> > ...\n> > +/* Main serve loop for protocol version 2 */\n> > +void serve(void)\n> > +{\n> > +\t/* serve by default supports v2 */\n> > +\tpacket_write_fmt(1, \"version 2\\n\");\n> > +\n> > +\tadvertise_capabilities();\n> > +\n> > +\tfor (;;)\n> > +\t\tif (process_request())\n> > +\t\t\tbreak;\n> > +}\n> \n> I am guessing that this would be run just like upload-pack,\n> i.e. invoked via ssh or via git-daemon, and that is why it can just\n> assume that fd#0/fd#1 are already connected to the other end. It\n> may be helpful to document somewhere how we envision to invoke this\n> program.\n> \n\nThis function I was planning to just be executed by upload-pack and\nreceive-pack when a client requests protocol v2. But yes the idea would\nbe that fd#0/fd#1 would be already setup like they are for upload-pack\nand receive-pack.\n\n-- \nBrandon Williams\n"},{"id":"334492","messageId":"xmqqvahh6k6a.fsf@gitster.mtv.corp.google.com","threadId":"47006","inReplyTo":"alpine.DEB.2.21.1.1712071658120.4318@virtualbox","subject":"Re: [WIP 04/15] upload-pack: convert to a builtin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-08T20:26:05Z","receivedAt":"2017-12-08T20:26:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Wed, 6 Dec 2017, Junio C Hamano wrote:\n> ...\n>> I vaguely recalled and feared that we on purpose kept this program\n>> separate from the rest of the system for a reason, but my mailing\n>> list search is coming up empty.\n>\n> I only recall that we kept it in the bin/ directory (as opposed to\n> mlibexec/git-core/) to help with fetching via SSH.\n\nYes, that is about where it is installed (i.e. on $PATH), which is a\ndifferent issue. \n\nMy vague recollection was about what is (and what is not) included\nin and linked into the program built, with some reason that is\ndifferent from but similar to the reason why remote helpers that\nlink to curl and openssl libraries are excluded from the builtin\ndeliberately. I know we exclude remote-helpers from builtin in\norder to save the start-up overhead for other more important\nbuilt-in commands by not having to link these heavyweight libs. I\nsuspect there was some valid reason why we didn't make upload-pack\na built-in, but am failing to recall what the reason was.\n\n\n"},{"id":"334758","messageId":"63304896830043BF9F076F507000D454@PhilipOakley","threadId":"47006","inReplyTo":"20171204235823.63299-13-bmwill@google.com","subject":"Re: [WIP 12/15] ls-refs: introduce ls-refs server command","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2017-12-13T16:30:51Z","isPatch":false,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Brandon Williams\" \nSent: Monday, December 04, 2017 11:58 PM\n> Introduce the ls-refs server command. In protocol v2, the ls-refs\n> command is used to request the ref advertisement from the server. Since\n> it is a command which can be requested (as opposed to manditory in v1),\n> a clinet can sent a number of parameters in its request to limit the ref\n\ns/clinet/client/\n\n> advertisement based on provided ref-patterns.\n> \n> Signed-off-by: Brandon Williams \n> ---\nPhilip\n"}]}