{"thread":{"id":"48900","subject":"[RFC] push: add documentation on push v2","startedAt":"2018-07-17T21:09:22Z","lastAt":"2018-08-02T15:18:21Z","messageCount":19,"participants":["Brandon Williams","Stefan Beller","Derrick Stolee","Duy Nguyen","Jeff Hostetler"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"352832","messageId":"20180717210915.139521-1-bmwill@google.com","threadId":"48900","inReplyTo":null,"subject":"[RFC] push: add documentation on push v2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-07-17T21:09:15Z","receivedAt":"2018-07-17T21:09:22Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Signed-off-by: Brandon Williams \n---\n\nSince introducing protocol v2 and enabling fetch I've been thinking\nabout what its inverse 'push' would look like. After talking with a\nnumber of people I have a longish list of things that could be done to\nimprove push and I think I've been able to distill the core features we\nwant in push v2. Thankfully (due to the capability system) most of the\nother features/improvements can be added later with ease.\n\nWhat I've got now is a rough design for a more flexible push, more\nflexible because it allows for the server to do what it wants with the\nrefs that are pushed and has the ability to communicate back what was\ndone to the client. The main motivation for this is to work around\nissues when working with Gerrit and other code-review systems where you\nneed to have Change-Ids in the commit messages (now the server can just\ninsert them for you and send back new commits) and you need to push to\nmagic refs to get around various limitations (now a Gerrit server should\nbe able to communicate that pushing to 'master' doesn't update master\nbut instead creates a refs/changes/ ref).\n\nBefore actually moving to write any code I'm hoping to get some feedback\non if we think this is an acceptable base design for push (other\nfeatures like atomic-push, signed-push, etc can be added as\ncapabilities), so any comments are appreciated.\n\n Documentation/technical/protocol-v2.txt | 76 +++++++++++++++++++++++++\n 1 file changed, 76 insertions(+)\n\ndiff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\nindex 49bda76d23..16c1ce60dd 100644\n--- a/Documentation/technical/protocol-v2.txt\n+++ b/Documentation/technical/protocol-v2.txt\n@@ -403,6 +403,82 @@ header.\n \t\t2 - progress messages\n \t\t3 - fatal error message just before stream aborts\n \n+ push\n+~~~~~~\n+\n+`push` is the command used to push ref-updates and a packfile to a remote\n+server in v2.\n+\n+Additional features not supported in the base command will be advertised\n+as the value of the command in the capability advertisement in the form\n+of a space separated list of features: \"= \"\n+\n+The format of a push request is as follows:\n+\n+ request = *section\n+ section = (ref-updates | packfile)\n+\t (delim-pkt | flush-pkt)\n+\n+ ref-updates = PKT-LINE(\"ref-updates\" LF)\n+\t\t *PKT-Line(update/force-update LF)\n+\n+ update = txn_id SP action SP refname SP old_oid SP new_oid\n+ force-update = txn_id SP \"force\" SP action SP refname SP new_oid\n+ action = (\"create\" | \"delete\" | \"update\")\n+ txn_id = 1*DIGIT\n+\n+ packfile = PKT-LINE(\"packfile\" LF)\n+\t *PKT-LINE(*%x00-ff)\n+\n+ ref-updates section\n+\t* Transaction id's allow for mapping what was requested to what the\n+\t server actually did with the ref-update.\n+\t* Normal ref-updates require that the old value of a ref is supplied so\n+\t that the server can verify that the reference that is being updated\n+\t hasn't changed while the request was being processed.\n+\t* Forced ref-updates only include the new value of a ref as we don't\n+\t care what the old value was.\n+\n+ packfile section\n+\t* A packfile MAY not be included if only delete commands are used or if\n+\t an update only incorperates objects the server already has\n+\n+The server will receive the packfile, unpack it, then validate each ref-update,\n+and it will run any update hooks to make sure that the update is acceptable.\n+If all of that is fine, the server will then update the references.\n+\n+The format of a push response is as follows:\n+\n+ response = *section\n+ section = (unpack-error | ref-update-status | packfile)\n+\t (delim-pkt | flush-pkt)\n+\n+ unpack-error = PKT-LINE(\"ERR\" SP error-msg LF)\n+\n+ ref-update-status = *(update-result | update-error)\n+ update-result = *PKT-LINE(txn_id SP result LF)\n+ result = (\"created\" | \"deleted\" | \"updated\") SP refname SP old_oid SP new_oid\n+ update-error = PKT-LINE(txn_id SP \"error\" SP error-msg LF)\n+\n+ packfile = PKT-LINE(\"packfile\" LF)\n+\t *PKT-LINE(*%x00-ff)\n+\n+ ref-update-status section\n+\t* This section is always included unless there was an error unpacking\n+\t the packfile sent in the request.\n+\t* The server is given the freedom to do what it wants with the\n+\t ref-updates provided in the reqeust. This means that an update sent\n+\t from the server may result in the creation of a ref or rebasing the\n+\t update on the server.\n+\t* If a server creates any new objects due to a ref-update, a packfile\n+\t MUST be sent back in the response.\n+\n+ packfile section\n+\t* This section is included if the server decided to do something with\n+\t the ref-updates that involved creating new objects.\n+\n server-option\n ~~~~~~~~~~~~~~~\n \n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352845","messageId":"CAGZ79kZEpNLkXuEQEiMB_nc-MOOp-KOziHyONmr4SiajA5+F2g@mail.gmail.com","threadId":"48900","inReplyTo":"20180717210915.139521-1-bmwill@google.com","subject":"Re: [RFC] push: add documentation on push v2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-17T23:25:35Z","receivedAt":"2018-07-17T23:25:49Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jul 17, 2018 at 2:09 PM Brandon Williams wrote:\n>\n> Signed-off-by: Brandon Williams \n> ---\n>\n> Since introducing protocol v2 and enabling fetch I've been thinking\n> about what its inverse 'push' would look like. After talking with a\n> number of people I have a longish list of things that could be done to\n> improve push and I think I've been able to distill the core features we\n> want in push v2.\n\nIt would be nice to know which things you want to improve.\n\n> Thankfully (due to the capability system) most of the\n> other features/improvements can be added later with ease.\n>\n> What I've got now is a rough design for a more flexible push, more\n> flexible because it allows for the server to do what it wants with the\n> refs that are pushed and has the ability to communicate back what was\n> done to the client. The main motivation for this is to work around\n> issues when working with Gerrit and other code-review systems where you\n> need to have Change-Ids in the commit messages (now the server can just\n> insert them for you and send back new commits) and you need to push to\n> magic refs to get around various limitations (now a Gerrit server should\n> be able to communicate that pushing to 'master' doesn't update master\n> but instead creates a refs/changes/ ref).\n\nWell Gerrit is our main motivation, but this allows for other workflows as well.\nFor example Facebook uses hg internally and they have a\n\"rebase-on-the-server-after-push\" workflow IIRC as pushing to a single repo\nbrings up quite some contention. The protocol outlined below would allow\nfor such a workflow as well? (This might be an easier sell to the Git\ncommunity as most are not quite familiar with Gerrit)\n\n> Before actually moving to write any code I'm hoping to get some feedback\n> on if we think this is an acceptable base design for push (other\n> features like atomic-push, signed-push, etc can be added as\n> capabilities), so any comments are appreciated.\n>\n> Documentation/technical/protocol-v2.txt | 76 +++++++++++++++++++++++++\n> 1 file changed, 76 insertions(+)\n>\n> diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\n> index 49bda76d23..16c1ce60dd 100644\n> --- a/Documentation/technical/protocol-v2.txt\n> +++ b/Documentation/technical/protocol-v2.txt\n> @@ -403,6 +403,82 @@ header.\n> 2 - progress messages\n> 3 - fatal error message just before stream aborts\n>\n> + push\n> +~~~~~~\n> +\n> +`push` is the command used to push ref-updates and a packfile to a remote\n> +server in v2.\n> +\n> +Additional features not supported in the base command will be advertised\n> +as the value of the command in the capability advertisement in the form\n> +of a space separated list of features: \"= \"\n> +\n> +The format of a push request is as follows:\n> +\n> + request = *section\n> + section = (ref-updates | packfile)\n\nThis reads as if a request consists of sections, which\neach can be a \"ref-updates\" or a packfile, no order given,\nsuch that multiple ref-update sections mixed with packfiles\nare possible.\n\nI would assume we'd only want to allow for ref-updates\nfollowed by the packfile.\n\nGiven the example above for \"rebase-on-push\" though\nit is better to first send the packfile (as that is assumed to\ntake longer) and then send the ref updates, such that the\nrebasing could be faster and has no bottleneck.\n\n> + (delim-pkt | flush-pkt)\n\n\n\n> +\n> + ref-updates = PKT-LINE(\"ref-updates\" LF)\n> + *PKT-Line(update/force-update LF)\n> +\n> + update = txn_id SP action SP refname SP old_oid SP new_oid\n> + force-update = txn_id SP \"force\" SP action SP refname SP new_oid\n\nSo we insert \"force\" after the transaction id if we want to force it.\nWhen adding the atomic capability later we could imagine another insert here\n\n 1 atomic create refs/heads/new-ref <0-hash> \n 1 atomic delete refs/heads/old-ref <0-hash>\n\nwhich would look like a \"rename\" that we could also add instead.\nThe transaction numbers are an interesting concept, how do you\nenvision them to be used? In the example I put them both in the same\ntransaction to demonstrate the \"atomic-ness\", but one could also\nimagine different transactions numbers per ref (i.e. exactly one\nref per txn_id) to have a better understanding of what the server did\nto each individual ref.\n\n> + action = (\"create\" | \"delete\" | \"update\")\n> + txn_id = 1*DIGIT\n> +\n> + packfile = PKT-LINE(\"packfile\" LF)\n> + *PKT-LINE(*%x00-ff)\n> +\n> + ref-updates section\n> + * Transaction id's allow for mapping what was requested to what the\n> + server actually did with the ref-update.\n\nthis would imply the client ought to have at most one ref per transaction id.\nIs the client allowed to put multiple refs per id?\n\nAre new capabilities attached to ref updates or transactions?\nUnlike the example above, stating \"atomic\" on each line, you could just\nsay \"transaction 1 should be atomic\" in another line, that would address\nall refs in that transaction.\n\n> + * Normal ref-updates require that the old value of a ref is supplied so\n> + that the server can verify that the reference that is being updated\n> + hasn't changed while the request was being processed.\n\ncreate/delete assume <00..00> for either old or new ? (We could also\nomit the second hash for create delete, which is more appealing to me)\n\n> + * Forced ref-updates only include the new value of a ref as we don't\n> + care what the old value was.\n\nHow are you implementing force-with-lease then?\n\n> + packfile section\n> + * A packfile MAY not be included if only delete commands are used or if\n> + an update only incorperates objects the server already has\n\nOr rather: \"An empty pack SHALL be omitted\" ?\n\n> +The server will receive the packfile, unpack it, then validate each ref-update,\n> +and it will run any update hooks to make sure that the update is acceptable.\n> +If all of that is fine, the server will then update the references.\n> +\n> +The format of a push response is as follows:\n> +\n> + response = *section\n> + section = (unpack-error | ref-update-status | packfile)\n\nAs above, I assume they ought to go in the order as written,\nor would it make sense to allow for any order?\n\n> + (delim-pkt | flush-pkt)\n> +\n> + unpack-error = PKT-LINE(\"ERR\" SP error-msg LF)\n> +\n> + ref-update-status = *(update-result | update-error)\n> + update-result = *PKT-LINE(txn_id SP result LF)\n> + result = (\"created\" | \"deleted\" | \"updated\") SP refname SP old_oid SP new_oid\n> + update-error = PKT-LINE(txn_id SP \"error\" SP error-msg LF)\n\nCan we unify \"ERR\" and \"error\" ?\n\n> + packfile = PKT-LINE(\"packfile\" LF)\n> + *PKT-LINE(*%x00-ff)\n> +\n> + ref-update-status section\n> + * This section is always included unless there was an error unpacking\n> + the packfile sent in the request.\n> + * The server is given the freedom to do what it wants with the\n> + ref-updates provided in the reqeust. This means that an update sent\n> + from the server may result in the creation of a ref or rebasing the\n> + update on the server.\n> + * If a server creates any new objects due to a ref-update, a packfile\n> + MUST be sent back in the response.\n> +\n> + packfile section\n> + * This section is included if the server decided to do something with\n> + the ref-updates that involved creating new objects.\n> +\n> server-option\n> ~~~~~~~~~~~~~~~\n>\n> --\n> 2.18.0.203.gfac676dfb9-goog\n>\n"},{"id":"352864","messageId":"a7c43308-a388-e307-6bea-47e6df74b65c@gmail.com","threadId":"48900","inReplyTo":"CAGZ79kZEpNLkXuEQEiMB_nc-MOOp-KOziHyONmr4SiajA5+F2g@mail.gmail.com","subject":"Re: [RFC] push: add documentation on push v2","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-07-18T13:31:06Z","receivedAt":"2018-07-18T13:31:12Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/17/2018 7:25 PM, Stefan Beller wrote:\n> On Tue, Jul 17, 2018 at 2:09 PM Brandon Williams wrote:\n>> Signed-off-by: Brandon Williams \n>> ---\n>>\n>> Since introducing protocol v2 and enabling fetch I've been thinking\n>> about what its inverse 'push' would look like. After talking with a\n>> number of people I have a longish list of things that could be done to\n>> improve push and I think I've been able to distill the core features we\n>> want in push v2.\n> It would be nice to know which things you want to improve.\n\nHopefully we can also get others to chime in with things they don't like \nabout the existing protocol. What pain points exist, and what can we do \nto improve at the transport layer before considering new functionality?\n\n>> Thankfully (due to the capability system) most of the\n>> other features/improvements can be added later with ease.\n>>\n>> What I've got now is a rough design for a more flexible push, more\n>> flexible because it allows for the server to do what it wants with the\n>> refs that are pushed and has the ability to communicate back what was\n>> done to the client. The main motivation for this is to work around\n>> issues when working with Gerrit and other code-review systems where you\n>> need to have Change-Ids in the commit messages (now the server can just\n>> insert them for you and send back new commits) and you need to push to\n>> magic refs to get around various limitations (now a Gerrit server should\n>> be able to communicate that pushing to 'master' doesn't update master\n>> but instead creates a refs/changes/ ref).\n> Well Gerrit is our main motivation, but this allows for other workflows as well.\n> For example Facebook uses hg internally and they have a\n> \"rebase-on-the-server-after-push\" workflow IIRC as pushing to a single repo\n> brings up quite some contention. The protocol outlined below would allow\n> for such a workflow as well? (This might be an easier sell to the Git\n> community as most are not quite familiar with Gerrit)\n\nI'm also curious how this \"change commits on push\" would be helpful to \nother scenarios.\n\nSince I'm not familiar with Gerrit: what is preventing you from having a \ncommit hook that inserts (or requests) a Change-Id when not present? How \ncan the server identify the Change-Id automatically when it isn't present?\n\n>> Before actually moving to write any code I'm hoping to get some feedback\n>> on if we think this is an acceptable base design for push (other\n>> features like atomic-push, signed-push, etc can be added as\n>> capabilities), so any comments are appreciated.\n>>\n>> Documentation/technical/protocol-v2.txt | 76 +++++++++++++++++++++++++\n>> 1 file changed, 76 insertions(+)\n>>\n>> diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\n>> index 49bda76d23..16c1ce60dd 100644\n>> --- a/Documentation/technical/protocol-v2.txt\n>> +++ b/Documentation/technical/protocol-v2.txt\n>> @@ -403,6 +403,82 @@ header.\n>> 2 - progress messages\n>> 3 - fatal error message just before stream aborts\n>>\n>> + push\n>> +~~~~~~\n>> +\n>> +`push` is the command used to push ref-updates and a packfile to a remote\n>> +server in v2.\n>> +\n>> +Additional features not supported in the base command will be advertised\n>> +as the value of the command in the capability advertisement in the form\n>> +of a space separated list of features: \"= \"\n>> +\n>> +The format of a push request is as follows:\n>> +\n>> + request = *section\n>> + section = (ref-updates | packfile)\n> This reads as if a request consists of sections, which\n> each can be a \"ref-updates\" or a packfile, no order given,\n> such that multiple ref-update sections mixed with packfiles\n> are possible.\n>\n> I would assume we'd only want to allow for ref-updates\n> followed by the packfile.\n>\n> Given the example above for \"rebase-on-push\" though\n> it is better to first send the packfile (as that is assumed to\n> take longer) and then send the ref updates, such that the\n> rebasing could be faster and has no bottleneck.\n>\n>> + (delim-pkt | flush-pkt)\n>\n>\n>> +\n>> + ref-updates = PKT-LINE(\"ref-updates\" LF)\n>> + *PKT-Line(update/force-update LF)\n>> +\n>> + update = txn_id SP action SP refname SP old_oid SP new_oid\n>> + force-update = txn_id SP \"force\" SP action SP refname SP new_oid\n> So we insert \"force\" after the transaction id if we want to force it.\n> When adding the atomic capability later we could imagine another insert here\n>\n> 1 atomic create refs/heads/new-ref <0-hash> \n> 1 atomic delete refs/heads/old-ref <0-hash>\n>\n> which would look like a \"rename\" that we could also add instead.\n> The transaction numbers are an interesting concept, how do you\n> envision them to be used? In the example I put them both in the same\n> transaction to demonstrate the \"atomic-ness\", but one could also\n> imagine different transactions numbers per ref (i.e. exactly one\n> ref per txn_id) to have a better understanding of what the server did\n> to each individual ref.\n>\n>> + action = (\"create\" | \"delete\" | \"update\")\n>> + txn_id = 1*DIGIT\n>> +\n>> + packfile = PKT-LINE(\"packfile\" LF)\n>> + *PKT-LINE(*%x00-ff)\n>> +\n>> + ref-updates section\n>> + * Transaction id's allow for mapping what was requested to what the\n>> + server actually did with the ref-update.\n> this would imply the client ought to have at most one ref per transaction id.\n> Is the client allowed to put multiple refs per id?\n>\n> Are new capabilities attached to ref updates or transactions?\n> Unlike the example above, stating \"atomic\" on each line, you could just\n> say \"transaction 1 should be atomic\" in another line, that would address\n> all refs in that transaction.\n>\n>> + * Normal ref-updates require that the old value of a ref is supplied so\n>> + that the server can verify that the reference that is being updated\n>> + hasn't changed while the request was being processed.\n> create/delete assume <00..00> for either old or new ? (We could also\n> omit the second hash for create delete, which is more appealing to me)\n>\n>> + * Forced ref-updates only include the new value of a ref as we don't\n>> + care what the old value was.\n> How are you implementing force-with-lease then?\n\nI had the same question.\n\n>\n>> + packfile section\n>> + * A packfile MAY not be included if only delete commands are used or if\n>> + an update only incorperates objects the server already has\n> Or rather: \"An empty pack SHALL be omitted\" ?\n>\n>> +The server will receive the packfile, unpack it, then validate each ref-update,\n>> +and it will run any update hooks to make sure that the update is acceptable.\n>> +If all of that is fine, the server will then update the references.\n>> +\n>> +The format of a push response is as follows:\n>> +\n>> + response = *section\n>> + section = (unpack-error | ref-update-status | packfile)\n> As above, I assume they ought to go in the order as written,\n> or would it make sense to allow for any order?\n>\n>> + (delim-pkt | flush-pkt)\n>> +\n>> + unpack-error = PKT-LINE(\"ERR\" SP error-msg LF)\n>> +\n>> + ref-update-status = *(update-result | update-error)\n>> + update-result = *PKT-LINE(txn_id SP result LF)\n>> + result = (\"created\" | \"deleted\" | \"updated\") SP refname SP old_oid SP new_oid\n>> + update-error = PKT-LINE(txn_id SP \"error\" SP error-msg LF)\n> Can we unify \"ERR\" and \"error\" ?\n>\n>> + packfile = PKT-LINE(\"packfile\" LF)\n>> + *PKT-LINE(*%x00-ff)\n>> +\n>> + ref-update-status section\n>> + * This section is always included unless there was an error unpacking\n>> + the packfile sent in the request.\n>> + * The server is given the freedom to do what it wants with the\n>> + ref-updates provided in the reqeust. This means that an update sent\n>> + from the server may result in the creation of a ref or rebasing the\n>> + update on the server.\n>> + * If a server creates any new objects due to a ref-update, a packfile\n>> + MUST be sent back in the response.\n>> +\n>> + packfile section\n>> + * This section is included if the server decided to do something with\n>> + the ref-updates that involved creating new objects.\n>> +\n>> server-option\n>> ~~~~~~~~~~~~~~~\n>>\n>> --\n>> 2.18.0.203.gfac676dfb9-goog\n>>\n"},{"id":"352911","messageId":"CAGZ79kbLn-uwQOXfqhtO46v0EWevY43Tf4W5Rz9gDD9_qbmX=A@mail.gmail.com","threadId":"48900","inReplyTo":"a7c43308-a388-e307-6bea-47e6df74b65c@gmail.com","subject":"Re: [RFC] push: add documentation on push v2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-18T16:56:20Z","receivedAt":"2018-07-18T16:56:34Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Jul 18, 2018 at 6:31 AM Derrick Stolee wrote:\n>\n> On 7/17/2018 7:25 PM, Stefan Beller wrote:\n> > On Tue, Jul 17, 2018 at 2:09 PM Brandon Williams wrote:\n> >> Signed-off-by: Brandon Williams \n> >> ---\n> >>\n> >> Since introducing protocol v2 and enabling fetch I've been thinking\n> >> about what its inverse 'push' would look like. After talking with a\n> >> number of people I have a longish list of things that could be done to\n> >> improve push and I think I've been able to distill the core features we\n> >> want in push v2.\n> > It would be nice to know which things you want to improve.\n>\n> Hopefully we can also get others to chime in with things they don't like\n> about the existing protocol. What pain points exist, and what can we do\n> to improve at the transport layer before considering new functionality?\n\nAnother thing that I realized last night was the possibility to chunk requests.\nThe web of today is driven by lots of small http(s) requests. I know our server\nteam fights with the internal tools all the time because the communication\ninvolved in git-fetch is usually a large http request (large packfile).\nSo it would be nice to have the possibility of chunking the request.\nBut I think that can be added as a capability? (Not sure how)\n\n> >> Thankfully (due to the capability system) most of the\n> >> other features/improvements can be added later with ease.\n> >>\n> >> What I've got now is a rough design for a more flexible push, more\n> >> flexible because it allows for the server to do what it wants with the\n> >> refs that are pushed and has the ability to communicate back what was\n> >> done to the client. The main motivation for this is to work around\n> >> issues when working with Gerrit and other code-review systems where you\n> >> need to have Change-Ids in the commit messages (now the server can just\n> >> insert them for you and send back new commits) and you need to push to\n> >> magic refs to get around various limitations (now a Gerrit server should\n> >> be able to communicate that pushing to 'master' doesn't update master\n> >> but instead creates a refs/changes/ ref).\n> > Well Gerrit is our main motivation, but this allows for other workflows as well.\n> > For example Facebook uses hg internally and they have a\n> > \"rebase-on-the-server-after-push\" workflow IIRC as pushing to a single repo\n> > brings up quite some contention. The protocol outlined below would allow\n> > for such a workflow as well? (This might be an easier sell to the Git\n> > community as most are not quite familiar with Gerrit)\n>\n> I'm also curious how this \"change commits on push\" would be helpful to\n> other scenarios.\n>\n> Since I'm not familiar with Gerrit: what is preventing you from having a\n> commit hook that inserts (or requests) a Change-Id when not present?\n\nThat is how you do it normally. But what if you just get started or want to\nsend a one-off to the server (I wanted to upload a git patch to our internal\nGerrit once, and as my repository is configured to work with upstream Git\nwhich doesn't carry change ids, I ran into this problem. I had to manually\nadd it to have the server accept it)\n\n> How\n> can the server identify the Change-Id automatically when it isn't present?\n\nThe change id is just a randomly assigned id, which can be made up,\nbut should stay consistent in further revisions. (Put another way:\nchange ids solve the 'linear assignment problem' of range-diff at scale)\n\nSo once the protocol support is in, the client would need to get some UX\nupdate to replace its commits just pushed with the answer from the server\nto work well with server side generated change ids.\n\nBut as said I am not sure how much we want to discuss in that direction,\nbut rather see if we could have other use cases:\nInstead of just rebasing to solve the contention problem server side,\nwe could also offer a \"coding helper as a service\" - server. That would\nwork similar as the change id workflow lines out above:\nYou push to the server, the server performs some action (style formatting\nyour code for example, linting) and you download it back and have it locallly\nagain.\n\nI think that would be pretty cool actually.\n\nThanks,\nStefan\n"},{"id":"352915","messageId":"20180718170846.GA17137@google.com","threadId":"48900","inReplyTo":"CAGZ79kZEpNLkXuEQEiMB_nc-MOOp-KOziHyONmr4SiajA5+F2g@mail.gmail.com","subject":"Re: [RFC] push: add documentation on push v2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-07-18T17:08:46Z","receivedAt":"2018-07-18T17:08:51Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 07/17, Stefan Beller wrote:\n> On Tue, Jul 17, 2018 at 2:09 PM Brandon Williams wrote:\n> >\n> > Signed-off-by: Brandon Williams \n> > ---\n> >\n> > Since introducing protocol v2 and enabling fetch I've been thinking\n> > about what its inverse 'push' would look like. After talking with a\n> > number of people I have a longish list of things that could be done to\n> > improve push and I think I've been able to distill the core features we\n> > want in push v2.\n> \n> It would be nice to know which things you want to improve.\n\nI mean this tackles the main point I want to improve. Others include:\nrebase/merge-on-push, push symrefs, and then the inclusion of other\ncapabilities from v0 like atomic push, signed push, etc.\n\n> \n> > Thankfully (due to the capability system) most of the\n> > other features/improvements can be added later with ease.\n> >\n> > What I've got now is a rough design for a more flexible push, more\n> > flexible because it allows for the server to do what it wants with the\n> > refs that are pushed and has the ability to communicate back what was\n> > done to the client. The main motivation for this is to work around\n> > issues when working with Gerrit and other code-review systems where you\n> > need to have Change-Ids in the commit messages (now the server can just\n> > insert them for you and send back new commits) and you need to push to\n> > magic refs to get around various limitations (now a Gerrit server should\n> > be able to communicate that pushing to 'master' doesn't update master\n> > but instead creates a refs/changes/ ref).\n> \n> Well Gerrit is our main motivation, but this allows for other workflows as well.\n> For example Facebook uses hg internally and they have a\n> \"rebase-on-the-server-after-push\" workflow IIRC as pushing to a single repo\n> brings up quite some contention. The protocol outlined below would allow\n> for such a workflow as well? (This might be an easier sell to the Git\n> community as most are not quite familiar with Gerrit)\n\nYes the idea would be such that we could easily add a \"rebase\" or\n\"merge\" verb later for explicit user controlled workflows like that.\nThis proposal would already make it possible (although the server would\nneed to be configured as such) since the server can do what it wants\nwith the updates a client sends to it.\n\n> \n> > Before actually moving to write any code I'm hoping to get some feedback\n> > on if we think this is an acceptable base design for push (other\n> > features like atomic-push, signed-push, etc can be added as\n> > capabilities), so any comments are appreciated.\n> >\n> > Documentation/technical/protocol-v2.txt | 76 +++++++++++++++++++++++++\n> > 1 file changed, 76 insertions(+)\n> >\n> > diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt\n> > index 49bda76d23..16c1ce60dd 100644\n> > --- a/Documentation/technical/protocol-v2.txt\n> > +++ b/Documentation/technical/protocol-v2.txt\n> > @@ -403,6 +403,82 @@ header.\n> > 2 - progress messages\n> > 3 - fatal error message just before stream aborts\n> >\n> > + push\n> > +~~~~~~\n> > +\n> > +`push` is the command used to push ref-updates and a packfile to a remote\n> > +server in v2.\n> > +\n> > +Additional features not supported in the base command will be advertised\n> > +as the value of the command in the capability advertisement in the form\n> > +of a space separated list of features: \"= \"\n> > +\n> > +The format of a push request is as follows:\n> > +\n> > + request = *section\n> > + section = (ref-updates | packfile)\n> \n> This reads as if a request consists of sections, which\n> each can be a \"ref-updates\" or a packfile, no order given,\n> such that multiple ref-update sections mixed with packfiles\n> are possible.\n> \n> I would assume we'd only want to allow for ref-updates\n> followed by the packfile.\n> \n> Given the example above for \"rebase-on-push\" though\n> it is better to first send the packfile (as that is assumed to\n> take longer) and then send the ref updates, such that the\n> rebasing could be faster and has no bottleneck.\n\nI don't really follow this logic. I don't think it would change\nanything much considering the ref-updates section would usually be\nmuch smaller than the packfile itself, course I don't have any data so\nidk.\n\n> \n> > + (delim-pkt | flush-pkt)\n> \n> \n> \n> > +\n> > + ref-updates = PKT-LINE(\"ref-updates\" LF)\n> > + *PKT-Line(update/force-update LF)\n> > +\n> > + update = txn_id SP action SP refname SP old_oid SP new_oid\n> > + force-update = txn_id SP \"force\" SP action SP refname SP new_oid\n> \n> So we insert \"force\" after the transaction id if we want to force it.\n> When adding the atomic capability later we could imagine another insert here\n> \n> 1 atomic create refs/heads/new-ref <0-hash> \n> 1 atomic delete refs/heads/old-ref <0-hash>\n> \n> which would look like a \"rename\" that we could also add instead.\n> The transaction numbers are an interesting concept, how do you\n> envision them to be used? In the example I put them both in the same\n> transaction to demonstrate the \"atomic-ness\", but one could also\n> imagine different transactions numbers per ref (i.e. exactly one\n> ref per txn_id) to have a better understanding of what the server did\n> to each individual ref.\n\nI believe I outlined their use later. Basically if you give the server\nfree reign to do what it wants with the updates you send it, then you\nneed a way for the client to be able to map the result back to what it\nrequested. Since now i could push to \"master\" but instead of updating\nmaster the server creates a refs/changes/1 ref and puts my changes there\ninstead of updating master. The client needs to know that the ref\nupdate it requested to master is what caused the creation of the\nrefs/changes/1 ref.\n\n> \n> > + action = (\"create\" | \"delete\" | \"update\")\n> > + txn_id = 1*DIGIT\n> > +\n> > + packfile = PKT-LINE(\"packfile\" LF)\n> > + *PKT-LINE(*%x00-ff)\n> > +\n> > + ref-updates section\n> > + * Transaction id's allow for mapping what was requested to what the\n> > + server actually did with the ref-update.\n> \n> this would imply the client ought to have at most one ref per transaction id.\n> Is the client allowed to put multiple refs per id?\n\nNo code has been written yet, so idk. Right now i don't see any reason\nto have multiple updates using the same id but that could change.\n\n> \n> Are new capabilities attached to ref updates or transactions?\n> Unlike the example above, stating \"atomic\" on each line, you could just\n> say \"transaction 1 should be atomic\" in another line, that would address\n> all refs in that transaction.\n\nI haven't thought through \"atomic\" so i have no idea what you'd want\nthat to look like.\n\n> \n> > + * Normal ref-updates require that the old value of a ref is supplied so\n> > + that the server can verify that the reference that is being updated\n> > + hasn't changed while the request was being processed.\n> \n> create/delete assume <00..00> for either old or new ? (We could also\n> omit the second hash for create delete, which is more appealing to me)\n\nWell that depends, in the case of a create you want to ensure that no\nref with that name exists and would want it to fail if one already\nexisted. If you want to force it then you don't care if one existed or\nnot, you just want the ref to have a certain value.\n\n> \n> > + * Forced ref-updates only include the new value of a ref as we don't\n> > + care what the old value was.\n> \n> How are you implementing force-with-lease then?\n\nCurrently force-with-lease/force is implemented 100% on the client side,\nthis proposal extends these two to be implemented on the server as well.\nnon-forced variant are basically the \"with-lease\" case and \"force\" now\nactually forces an update. Right now you can still have a \"forced\"\nupdate fail to update when using a stateless transport because by the\ntime you send the ref-updates they've changed from what you read in the\nref-advertisement (this can happen with projects with high velocity).\n\nIf you just wanted to preserve the existing force-with-lease/force\nbehavior you can simply use the non-force variant of a ref-update.\n\n> \n> > + packfile section\n> > + * A packfile MAY not be included if only delete commands are used or if\n> > + an update only incorperates objects the server already has\n> \n> Or rather: \"An empty pack SHALL be omitted\" ?\n> \n> > +The server will receive the packfile, unpack it, then validate each ref-update,\n> > +and it will run any update hooks to make sure that the update is acceptable.\n> > +If all of that is fine, the server will then update the references.\n> > +\n> > +The format of a push response is as follows:\n> > +\n> > + response = *section\n> > + section = (unpack-error | ref-update-status | packfile)\n> \n> As above, I assume they ought to go in the order as written,\n> or would it make sense to allow for any order?\n> \n> > + (delim-pkt | flush-pkt)\n> > +\n> > + unpack-error = PKT-LINE(\"ERR\" SP error-msg LF)\n> > +\n> > + ref-update-status = *(update-result | update-error)\n> > + update-result = *PKT-LINE(txn_id SP result LF)\n> > + result = (\"created\" | \"deleted\" | \"updated\") SP refname SP old_oid SP new_oid\n> > + update-error = PKT-LINE(txn_id SP \"error\" SP error-msg LF)\n> \n> Can we unify \"ERR\" and \"error\" ?\n\nNo, these are very different. You could have one ref update succeed\nwhile another doesn't for some reason, unless you want everything to be\natomic.\n\n> \n> > + packfile = PKT-LINE(\"packfile\" LF)\n> > + *PKT-LINE(*%x00-ff)\n> > +\n> > + ref-update-status section\n> > + * This section is always included unless there was an error unpacking\n> > + the packfile sent in the request.\n> > + * The server is given the freedom to do what it wants with the\n> > + ref-updates provided in the reqeust. This means that an update sent\n> > + from the server may result in the creation of a ref or rebasing the\n> > + update on the server.\n> > + * If a server creates any new objects due to a ref-update, a packfile\n> > + MUST be sent back in the response.\n> > +\n> > + packfile section\n> > + * This section is included if the server decided to do something with\n> > + the ref-updates that involved creating new objects.\n> > +\n> > server-option\n> > ~~~~~~~~~~~~~~~\n> >\n> > --\n> > 2.18.0.203.gfac676dfb9-goog\n> >\n\n-- \nBrandon Williams\n"},{"id":"352916","messageId":"20180718171127.GB17137@google.com","threadId":"48900","inReplyTo":"a7c43308-a388-e307-6bea-47e6df74b65c@gmail.com","subject":"Re: [RFC] push: add documentation on push v2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-07-18T17:11:27Z","receivedAt":"2018-07-18T17:11:32Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 07/18, Derrick Stolee wrote:\n> On 7/17/2018 7:25 PM, Stefan Beller wrote:\n> > On Tue, Jul 17, 2018 at 2:09 PM Brandon Williams wrote:\n> > > Signed-off-by: Brandon Williams \n> > > ---\n> > > \n> > > Since introducing protocol v2 and enabling fetch I've been thinking\n> > > about what its inverse 'push' would look like. After talking with a\n> > > number of people I have a longish list of things that could be done to\n> > > improve push and I think I've been able to distill the core features we\n> > > want in push v2.\n> > It would be nice to know which things you want to improve.\n> \n> Hopefully we can also get others to chime in with things they don't like\n> about the existing protocol. What pain points exist, and what can we do to\n> improve at the transport layer before considering new functionality?\n> \n> > > Thankfully (due to the capability system) most of the\n> > > other features/improvements can be added later with ease.\n> > > \n> > > What I've got now is a rough design for a more flexible push, more\n> > > flexible because it allows for the server to do what it wants with the\n> > > refs that are pushed and has the ability to communicate back what was\n> > > done to the client. The main motivation for this is to work around\n> > > issues when working with Gerrit and other code-review systems where you\n> > > need to have Change-Ids in the commit messages (now the server can just\n> > > insert them for you and send back new commits) and you need to push to\n> > > magic refs to get around various limitations (now a Gerrit server should\n> > > be able to communicate that pushing to 'master' doesn't update master\n> > > but instead creates a refs/changes/ ref).\n> > Well Gerrit is our main motivation, but this allows for other workflows as well.\n> > For example Facebook uses hg internally and they have a\n> > \"rebase-on-the-server-after-push\" workflow IIRC as pushing to a single repo\n> > brings up quite some contention. The protocol outlined below would allow\n> > for such a workflow as well? (This might be an easier sell to the Git\n> > community as most are not quite familiar with Gerrit)\n> \n> I'm also curious how this \"change commits on push\" would be helpful to other\n> scenarios.\n> \n> Since I'm not familiar with Gerrit: what is preventing you from having a\n> commit hook that inserts (or requests) a Change-Id when not present? How can\n> the server identify the Change-Id automatically when it isn't present?\n\nRight now all Gerrit users have a commit hook installed which inserts\nthe Change-Id. The issue is that if you push to gerrit and you don't\nhave Change-ids, the push fails and you're prompted to blindly run a\ncommand to install the commit-hook. So if we could just have the server\nhandle this completely then the users of gerrit wouldn't ever need to\nhave a hook installed in the first place.\n\n\n-- \nBrandon Williams\n"},{"id":"352919","messageId":"20180718171512.GC17137@google.com","threadId":"48900","inReplyTo":"CAGZ79kbLn-uwQOXfqhtO46v0EWevY43Tf4W5Rz9gDD9_qbmX=A@mail.gmail.com","subject":"Re: [RFC] push: add documentation on push v2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-07-18T17:15:12Z","receivedAt":"2018-07-18T17:15:23Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 07/18, Stefan Beller wrote:\n> On Wed, Jul 18, 2018 at 6:31 AM Derrick Stolee wrote:\n> >\n> > On 7/17/2018 7:25 PM, Stefan Beller wrote:\n> > > On Tue, Jul 17, 2018 at 2:09 PM Brandon Williams wrote:\n> > >> Signed-off-by: Brandon Williams \n> > >> ---\n> > >>\n> > >> Since introducing protocol v2 and enabling fetch I've been thinking\n> > >> about what its inverse 'push' would look like. After talking with a\n> > >> number of people I have a longish list of things that could be done to\n> > >> improve push and I think I've been able to distill the core features we\n> > >> want in push v2.\n> > > It would be nice to know which things you want to improve.\n> >\n> > Hopefully we can also get others to chime in with things they don't like\n> > about the existing protocol. What pain points exist, and what can we do\n> > to improve at the transport layer before considering new functionality?\n> \n> Another thing that I realized last night was the possibility to chunk requests.\n> The web of today is driven by lots of small http(s) requests. I know our server\n> team fights with the internal tools all the time because the communication\n> involved in git-fetch is usually a large http request (large packfile).\n> So it would be nice to have the possibility of chunking the request.\n> But I think that can be added as a capability? (Not sure how)\n\nFetch and push requests/responses are already \"chunked\" when using the\nhttp transport. So I'm not sure what you mean by adding a capability\nbecause the protocol doesn't care about which transport you're using.\nThis is of course unless you're talking about a different \"chunking\"\nfrom what it means to chunk an http request/response.\n\n-- \nBrandon Williams\n"},{"id":"352921","messageId":"CACsJy8DaeUWo1qmgyxZ_9kuKLyRP+m1kgNGkoj6LtOMTknvEYQ@mail.gmail.com","threadId":"48900","inReplyTo":"20180718171127.GB17137@google.com","subject":"Re: [RFC] push: add documentation on push v2","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-07-18T17:19:53Z","receivedAt":"2018-07-18T17:20:22Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jul 18, 2018 at 7:13 PM Brandon Williams wrote:\n> > > > What I've got now is a rough design for a more flexible push, more\n> > > > flexible because it allows for the server to do what it wants with the\n> > > > refs that are pushed and has the ability to communicate back what was\n> > > > done to the client. The main motivation for this is to work around\n> > > > issues when working with Gerrit and other code-review systems where you\n> > > > need to have Change-Ids in the commit messages (now the server can just\n> > > > insert them for you and send back new commits) and you need to push to\n> > > > magic refs to get around various limitations (now a Gerrit server should\n> > > > be able to communicate that pushing to 'master' doesn't update master\n> > > > but instead creates a refs/changes/ ref).\n> > > Well Gerrit is our main motivation, but this allows for other workflows as well.\n> > > For example Facebook uses hg internally and they have a\n> > > \"rebase-on-the-server-after-push\" workflow IIRC as pushing to a single repo\n> > > brings up quite some contention. The protocol outlined below would allow\n> > > for such a workflow as well? (This might be an easier sell to the Git\n> > > community as most are not quite familiar with Gerrit)\n> >\n> > I'm also curious how this \"change commits on push\" would be helpful to other\n> > scenarios.\n> >\n> > Since I'm not familiar with Gerrit: what is preventing you from having a\n> > commit hook that inserts (or requests) a Change-Id when not present? How can\n> > the server identify the Change-Id automatically when it isn't present?\n>\n> Right now all Gerrit users have a commit hook installed which inserts\n> the Change-Id. The issue is that if you push to gerrit and you don't\n> have Change-ids, the push fails and you're prompted to blindly run a\n> command to install the commit-hook. So if we could just have the server\n> handle this completely then the users of gerrit wouldn't ever need to\n> have a hook installed in the first place.\n\nI don't trust the server side to rewrite commits for me. And this is\nbasically rewriting history (e.g. I can push multiple commits to\ngerrit if I remember correctly; if they all don't have change-id, then\nthe history must be rewritten for change-id to be inserted). Don't we\nalready have \"plans\" to push config from server to client? There's\nalso talk about configuring hooks with config file. These should make\nit possible to deal with change-id generation with minimum manual\nintervention.\n-- \nDuy\n"},{"id":"352929","messageId":"20180718174654.GD17137@google.com","threadId":"48900","inReplyTo":"CACsJy8DaeUWo1qmgyxZ_9kuKLyRP+m1kgNGkoj6LtOMTknvEYQ@mail.gmail.com","subject":"Re: [RFC] push: add documentation on push v2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-07-18T17:46:54Z","receivedAt":"2018-07-18T17:46:59Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 07/18, Duy Nguyen wrote:\n> On Wed, Jul 18, 2018 at 7:13 PM Brandon Williams wrote:\n> > > > > What I've got now is a rough design for a more flexible push, more\n> > > > > flexible because it allows for the server to do what it wants with the\n> > > > > refs that are pushed and has the ability to communicate back what was\n> > > > > done to the client. The main motivation for this is to work around\n> > > > > issues when working with Gerrit and other code-review systems where you\n> > > > > need to have Change-Ids in the commit messages (now the server can just\n> > > > > insert them for you and send back new commits) and you need to push to\n> > > > > magic refs to get around various limitations (now a Gerrit server should\n> > > > > be able to communicate that pushing to 'master' doesn't update master\n> > > > > but instead creates a refs/changes/ ref).\n> > > > Well Gerrit is our main motivation, but this allows for other workflows as well.\n> > > > For example Facebook uses hg internally and they have a\n> > > > \"rebase-on-the-server-after-push\" workflow IIRC as pushing to a single repo\n> > > > brings up quite some contention. The protocol outlined below would allow\n> > > > for such a workflow as well? (This might be an easier sell to the Git\n> > > > community as most are not quite familiar with Gerrit)\n> > >\n> > > I'm also curious how this \"change commits on push\" would be helpful to other\n> > > scenarios.\n> > >\n> > > Since I'm not familiar with Gerrit: what is preventing you from having a\n> > > commit hook that inserts (or requests) a Change-Id when not present? How can\n> > > the server identify the Change-Id automatically when it isn't present?\n> >\n> > Right now all Gerrit users have a commit hook installed which inserts\n> > the Change-Id. The issue is that if you push to gerrit and you don't\n> > have Change-ids, the push fails and you're prompted to blindly run a\n> > command to install the commit-hook. So if we could just have the server\n> > handle this completely then the users of gerrit wouldn't ever need to\n> > have a hook installed in the first place.\n> \n> I don't trust the server side to rewrite commits for me. And this is\n\nThat's a fair point. Though I think there are a number of projects\nwhere this would be very beneficial for contributors. The main reason\nfor wanting a feature like this is to make the UX easier for Gerrit\nusers (Having server insert change ids as well as potentially getting\nrid of the weird HEAD:refs/for/master syntax you need to push for\nreview). Also, if you don't control a server yourself, then who ever\ncontrols it can do what it wants with the objects/ref-updates you send\nthem. Of course even if they rewrite history that doesn't mean your\nlocal copy needs to mimic those changes if you don't want them too. So\neven if we move forward with a design like this, there would need to be\nsome config option to actually accept and apply the changes a server\nmakes and sends back to you. This RFC doesn't actually address those\nsorts of UX implications because I expect those are things which can be\nadded and tweaked at some point in the future. I'm just trying to build\nthe foundation for such changes.\n\n> basically rewriting history (e.g. I can push multiple commits to\n> gerrit if I remember correctly; if they all don't have change-id, then\n> the history must be rewritten for change-id to be inserted). Don't we\n> already have \"plans\" to push config from server to client? There's\n\nI know there has been talk about this, but I don't know any of any\ncurrent proposals or work being done in this area.\n\n> also talk about configuring hooks with config file. These should make\n> it possible to deal with change-id generation with minimum manual\n> intervention.\n\n-- \nBrandon Williams\n"},{"id":"352934","messageId":"CACsJy8A=ie_HrBzvvbbOmNVWKisBH8mbYYJYSsE3G+9k47XqdA@mail.gmail.com","threadId":"48900","inReplyTo":"20180718174654.GD17137@google.com","subject":"Re: [RFC] push: add documentation on push v2","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-07-18T17:57:34Z","receivedAt":"2018-07-18T17:58:05Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jul 18, 2018 at 7:46 PM Brandon Williams wrote:\n>\n> On 07/18, Duy Nguyen wrote:\n> > On Wed, Jul 18, 2018 at 7:13 PM Brandon Williams wrote:\n> > > > > > What I've got now is a rough design for a more flexible push, more\n> > > > > > flexible because it allows for the server to do what it wants with the\n> > > > > > refs that are pushed and has the ability to communicate back what was\n> > > > > > done to the client. The main motivation for this is to work around\n> > > > > > issues when working with Gerrit and other code-review systems where you\n> > > > > > need to have Change-Ids in the commit messages (now the server can just\n> > > > > > insert them for you and send back new commits) and you need to push to\n> > > > > > magic refs to get around various limitations (now a Gerrit server should\n> > > > > > be able to communicate that pushing to 'master' doesn't update master\n> > > > > > but instead creates a refs/changes/ ref).\n> > > > > Well Gerrit is our main motivation, but this allows for other workflows as well.\n> > > > > For example Facebook uses hg internally and they have a\n> > > > > \"rebase-on-the-server-after-push\" workflow IIRC as pushing to a single repo\n> > > > > brings up quite some contention. The protocol outlined below would allow\n> > > > > for such a workflow as well? (This might be an easier sell to the Git\n> > > > > community as most are not quite familiar with Gerrit)\n> > > >\n> > > > I'm also curious how this \"change commits on push\" would be helpful to other\n> > > > scenarios.\n> > > >\n> > > > Since I'm not familiar with Gerrit: what is preventing you from having a\n> > > > commit hook that inserts (or requests) a Change-Id when not present? How can\n> > > > the server identify the Change-Id automatically when it isn't present?\n> > >\n> > > Right now all Gerrit users have a commit hook installed which inserts\n> > > the Change-Id. The issue is that if you push to gerrit and you don't\n> > > have Change-ids, the push fails and you're prompted to blindly run a\n> > > command to install the commit-hook. So if we could just have the server\n> > > handle this completely then the users of gerrit wouldn't ever need to\n> > > have a hook installed in the first place.\n> >\n> > I don't trust the server side to rewrite commits for me. And this is\n>\n> That's a fair point. Though I think there are a number of projects\n> where this would be very beneficial for contributors. The main reason\n> for wanting a feature like this is to make the UX easier for Gerrit\n> users (Having server insert change ids as well as potentially getting\n> rid of the weird HEAD:refs/for/master syntax you need to push for\n> review). Also, if you don't control a server yourself, then who ever\n> controls it can do what it wants with the objects/ref-updates you send\n> them. Of course even if they rewrite history that doesn't mean your\n> local copy needs to mimic those changes if you don't want them too. So\n> even if we move forward with a design like this, there would need to be\n> some config option to actually accept and apply the changes a server\n> makes and sends back to you.\n\nThis is the main pain point for me. I almost wrote a follow mail along\nthe line of \"having said that, if we can be transparent what the\nchanges are or have some protection at client side against\n\"dangerous\" changes like tree/blob/commit-header replacement then it's\nprobably ok\".\n\nThere's also other things like signing which will not work well with\nremote updates like this. I guess you can cross it out as \"not\nsupported\" after consideration though.\n\n> This RFC doesn't actually address those\n> sorts of UX implications because I expect those are things which can be\n> added and tweaked at some point in the future. I'm just trying to build\n> the foundation for such changes.\n\nSpeaking of UX, gerrit and this server-side ref-update. My experience\nwith average gerrit users is they tend to stick to a very basic set of\ncommands and if this is not handled well, you just replace the\none-time pain of installing hooks with a new one that happens much\nmore often, potentially more confusing too.\n\n> > basically rewriting history (e.g. I can push multiple commits to\n> > gerrit if I remember correctly; if they all don't have change-id, then\n> > the history must be rewritten for change-id to be inserted). Don't we\n> > already have \"plans\" to push config from server to client? There's\n>\n> I know there has been talk about this, but I don't know any of any\n> current proposals or work being done in this area.\n\nAs far as I know, nobody has worked on it. You're welcome to start of course ;-)\n-- \nDuy\n"},{"id":"352935","messageId":"CAGZ79kZ4HOvo-xaDK=USveJ5zaLdJSddh8XNxsOsFHQuu7KcZQ@mail.gmail.com","threadId":"48900","inReplyTo":"20180718170846.GA17137@google.com","subject":"Re: [RFC] push: add documentation on push v2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-18T18:07:17Z","receivedAt":"2018-07-18T18:07:32Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> > Given the example above for \"rebase-on-push\" though\n> > it is better to first send the packfile (as that is assumed to\n> > take longer) and then send the ref updates, such that the\n> > rebasing could be faster and has no bottleneck.\n>\n> I don't really follow this logic. I don't think it would change\n> anything much considering the ref-updates section would usually be\n> much smaller than the packfile itself, course I don't have any data so\n> idk.\n\nThe server would need to serialize all incoming requests and apply\nthem in order. The receiving of the packfile and the response to the client\nare not part of the critical section that needs to happen serialized but\ncan be spread out to threads. So for that use case it would make\nsense to allow sending the packfile first.\n\n> > > + update = txn_id SP action SP refname SP old_oid SP new_oid\n> > > + force-update = txn_id SP \"force\" SP action SP refname SP new_oid\n> >\n> > So we insert \"force\" after the transaction id if we want to force it.\n> > When adding the atomic capability later we could imagine another insert here\n> >\n> > 1 atomic create refs/heads/new-ref <0-hash> \n> > 1 atomic delete refs/heads/old-ref <0-hash>\n> >\n> > which would look like a \"rename\" that we could also add instead.\n> > The transaction numbers are an interesting concept, how do you\n> > envision them to be used? In the example I put them both in the same\n> > transaction to demonstrate the \"atomic-ness\", but one could also\n> > imagine different transactions numbers per ref (i.e. exactly one\n> > ref per txn_id) to have a better understanding of what the server did\n> > to each individual ref.\n>\n> I believe I outlined their use later. Basically if you give the server\n> free reign to do what it wants with the updates you send it, then you\n> need a way for the client to be able to map the result back to what it\n> requested. Since now i could push to \"master\" but instead of updating\n> master the server creates a refs/changes/1 ref and puts my changes there\n> instead of updating master. The client needs to know that the ref\n> update it requested to master is what caused the creation of the\n> refs/changes/1 ref.\n\nunderstood, the question was more related to how you envision what\nthe client/server SHOULD be doing here, (and I think a one txn_id per\nref is what SHOULD be done is how this is best to implement the\nthoughts above, also the client is ALLOWED to put many refs in one\ntxn, or would we just disallow that already at this stage to not confuse\nthe server?)\n\n>\n> >\n> > Are new capabilities attached to ref updates or transactions?\n> > Unlike the example above, stating \"atomic\" on each line, you could just\n> > say \"transaction 1 should be atomic\" in another line, that would address\n> > all refs in that transaction.\n>\n> I haven't thought through \"atomic\" so i have no idea what you'd want\n> that to look like.\n\nYeah I have not really thought about them either, I just see two ways:\n(A) adding more keywords in each ref-update (like \"force\") or\n(B) adding new subsections somewhere where we talk about the capabilities\n instead.\n\nDepending on why way we want to go this might have impact on the\ndesign how to write the code.\n\n> > > + * Normal ref-updates require that the old value of a ref is supplied so\n> > > + that the server can verify that the reference that is being updated\n> > > + hasn't changed while the request was being processed.\n> >\n> > create/delete assume <00..00> for either old or new ? (We could also\n> > omit the second hash for create delete, which is more appealing to me)\n>\n> Well that depends, in the case of a create you want to ensure that no\n> ref with that name exists and would want it to fail if one already\n> existed. If you want to force it then you don't care if one existed or\n> not, you just want the ref to have a certain value.\n\nWhat I was trying to say is to have\n\n update = txn_id SP (modifier SP) action\n modifier = \"force\" | \"atomic\"\n action = (create | delete | update)\n create = \"create\" SP \n update = \"update\" SP SP \n delete = \"delete\" SP \n\ni.e. only one hash for the create and delete action.\n(I added the \"atomic\" modifier to demonstrate (A) from above, not needed here)\n\n> >\n> > > + * Forced ref-updates only include the new value of a ref as we don't\n> > > + care what the old value was.\n> >\n> > How are you implementing force-with-lease then?\n>\n> Currently force-with-lease/force is implemented 100% on the client side,\n\nUh? That would be bad. Reading 631b5ef219c (push --force-with-lease: tie\nit all together, 2013-07-08) I think that send-pack is done server-side?\n\n> this proposal extends these two to be implemented on the server as well.\n> non-forced variant are basically the \"with-lease\" case and \"force\" now\n> actually forces an update.\n\nI think we have 3 modes:\n(1) standard push, where both client and server check for a fast-forward\n(2) \"force\" that blindly overwrites the ref, but as that has a race condition\n in case multiple people can push to the remove we have\n(3) \"with-lease\", disables the fast forward check both on client and server\n but still takes out a lock on the server to ensure no races happen\n\nNow you propose to have only 2, making (1) and (3) the same, deferring\nthe check to have \"fast forwards only\" to be client only?\nThe server surely wants to ensure that, too (maybe you need\nspecial permission for non-ff; depends on the server implementation).\n\nI am not sure I like it, as on the protocol level this indeed looks the same\nand the server and client need to care in their implementation how/when\nthe ff-check is done. Though it would be nice for the client UX that you\nneed to give a flag to check for ff (both client and server side? or can we rely\non the client alone then?)\n\n> > > + (delim-pkt | flush-pkt)\n> > > +\n> > > + unpack-error = PKT-LINE(\"ERR\" SP error-msg LF)\n> > > +\n> > > + ref-update-status = *(update-result | update-error)\n> > > + update-result = *PKT-LINE(txn_id SP result LF)\n> > > + result = (\"created\" | \"deleted\" | \"updated\") SP refname SP old_oid SP new_oid\n> > > + update-error = PKT-LINE(txn_id SP \"error\" SP error-msg LF)\n> >\n> > Can we unify \"ERR\" and \"error\" ?\n>\n> No, these are very different. You could have one ref update succeed\n> while another doesn't for some reason, unless you want everything to be\n> atomic.\n\nI did not mean to unify them on the semantic level, but on the\nrepresentation level, i.e. have both of them spelled the same,\nas they can still be differentiated by the leading txn id?\n\n\nThanks,\nStefan\n\nP.S.: another feature that just came to mind is localisation of error messages.\nBut that is also easy to do with capabilities (the client sends a capability\nsuch as \"preferred-i18n=DE\" and the server may translate all its errors\nif it can.\n\nThat brings me to another point: do we assume all errors to be read\nby humans? or do we want some markup things in there, too, similar to\nEAGAIN?\n"},{"id":"352937","messageId":"CACsJy8Ccjh+Fma_uYPN+_PxUOfAU0nLQVBbVoTLMonhc5ygKcg@mail.gmail.com","threadId":"48900","inReplyTo":"CAGZ79kZ4HOvo-xaDK=USveJ5zaLdJSddh8XNxsOsFHQuu7KcZQ@mail.gmail.com","subject":"Re: [RFC] push: add documentation on push v2","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-07-18T18:17:36Z","receivedAt":"2018-07-18T18:18:04Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jul 18, 2018 at 8:08 PM Stefan Beller wrote:\n> P.S.: another feature that just came to mind is localisation of error messages.\n> But that is also easy to do with capabilities (the client sends a capability\n> such as \"preferred-i18n=DE\" and the server may translate all its errors\n> if it can.\n>\n> That brings me to another point: do we assume all errors to be read\n> by humans? or do we want some markup things in there, too, similar to\n> EAGAIN?\n\nWe do have several classes of errors (fatal, error, warning) so at\nleast some machine-friendly code should be there. Perhaps just follow\nmany protocols out there (http in particular) and separate error codes\nin groups. Then we can standardize some specific error codes later if\nwe want too but by default, error(), warning() or die() when running\nin server context will have a fixed error code in each group.\n-- \nDuy\n"},{"id":"352939","messageId":"20180718182154.GE17137@google.com","threadId":"48900","inReplyTo":"CAGZ79kZ4HOvo-xaDK=USveJ5zaLdJSddh8XNxsOsFHQuu7KcZQ@mail.gmail.com","subject":"Re: [RFC] push: add documentation on push v2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-07-18T18:21:54Z","receivedAt":"2018-07-18T18:22:00Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 07/18, Stefan Beller wrote:\n> > > Given the example above for \"rebase-on-push\" though\n> > > it is better to first send the packfile (as that is assumed to\n> > > take longer) and then send the ref updates, such that the\n> > > rebasing could be faster and has no bottleneck.\n> >\n> > I don't really follow this logic. I don't think it would change\n> > anything much considering the ref-updates section would usually be\n> > much smaller than the packfile itself, course I don't have any data so\n> > idk.\n> \n> The server would need to serialize all incoming requests and apply\n> them in order. The receiving of the packfile and the response to the client\n> are not part of the critical section that needs to happen serialized but\n> can be spread out to threads. So for that use case it would make\n> sense to allow sending the packfile first.\n\nI would think that a server needs to read the whole request before any\nactions can be done, but maybe I need to think about this a bit more.\n\n> \n> > > > + update = txn_id SP action SP refname SP old_oid SP new_oid\n> > > > + force-update = txn_id SP \"force\" SP action SP refname SP new_oid\n> > >\n> > > So we insert \"force\" after the transaction id if we want to force it.\n> > > When adding the atomic capability later we could imagine another insert here\n> > >\n> > > 1 atomic create refs/heads/new-ref <0-hash> \n> > > 1 atomic delete refs/heads/old-ref <0-hash>\n> > >\n> > > which would look like a \"rename\" that we could also add instead.\n> > > The transaction numbers are an interesting concept, how do you\n> > > envision them to be used? In the example I put them both in the same\n> > > transaction to demonstrate the \"atomic-ness\", but one could also\n> > > imagine different transactions numbers per ref (i.e. exactly one\n> > > ref per txn_id) to have a better understanding of what the server did\n> > > to each individual ref.\n> >\n> > I believe I outlined their use later. Basically if you give the server\n> > free reign to do what it wants with the updates you send it, then you\n> > need a way for the client to be able to map the result back to what it\n> > requested. Since now i could push to \"master\" but instead of updating\n> > master the server creates a refs/changes/1 ref and puts my changes there\n> > instead of updating master. The client needs to know that the ref\n> > update it requested to master is what caused the creation of the\n> > refs/changes/1 ref.\n> \n> understood, the question was more related to how you envision what\n> the client/server SHOULD be doing here, (and I think a one txn_id per\n> ref is what SHOULD be done is how this is best to implement the\n> thoughts above, also the client is ALLOWED to put many refs in one\n> txn, or would we just disallow that already at this stage to not confuse\n> the server?)\n\nOh sorry for misunderstanding. Yeah, as of right now I think having a\none-to-one relationship makes it easier to write/implement but I don't\nknow if there are other workflows which would benefit from multiple per\ntxn_id.\n\n> \n> >\n> > >\n> > > Are new capabilities attached to ref updates or transactions?\n> > > Unlike the example above, stating \"atomic\" on each line, you could just\n> > > say \"transaction 1 should be atomic\" in another line, that would address\n> > > all refs in that transaction.\n> >\n> > I haven't thought through \"atomic\" so i have no idea what you'd want\n> > that to look like.\n> \n> Yeah I have not really thought about them either, I just see two ways:\n> (A) adding more keywords in each ref-update (like \"force\") or\n> (B) adding new subsections somewhere where we talk about the capabilities\n> instead.\n> \n> Depending on why way we want to go this might have impact on the\n> design how to write the code.\n> \n> > > > + * Normal ref-updates require that the old value of a ref is supplied so\n> > > > + that the server can verify that the reference that is being updated\n> > > > + hasn't changed while the request was being processed.\n> > >\n> > > create/delete assume <00..00> for either old or new ? (We could also\n> > > omit the second hash for create delete, which is more appealing to me)\n> >\n> > Well that depends, in the case of a create you want to ensure that no\n> > ref with that name exists and would want it to fail if one already\n> > existed. If you want to force it then you don't care if one existed or\n> > not, you just want the ref to have a certain value.\n> \n> What I was trying to say is to have\n> \n> update = txn_id SP (modifier SP) action\n> modifier = \"force\" | \"atomic\"\n> action = (create | delete | update)\n> create = \"create\" SP \n> update = \"update\" SP SP \n> delete = \"delete\" SP \n> \n> i.e. only one hash for the create and delete action.\n> (I added the \"atomic\" modifier to demonstrate (A) from above, not needed here)\n\nI understood what you were asking, I was just pointing out the rationale\nfor including the zero-id but i guess you're correct in that simply\nomitting it would work just as well for what I outlined above. So I\nthink I'll go with what you suggested here.\n\n> \n> > >\n> > > > + * Forced ref-updates only include the new value of a ref as we don't\n> > > > + care what the old value was.\n> > >\n> > > How are you implementing force-with-lease then?\n> >\n> > Currently force-with-lease/force is implemented 100% on the client side,\n> \n> Uh? That would be bad. Reading 631b5ef219c (push --force-with-lease: tie\n> it all together, 2013-07-08) I think that send-pack is done server-side?\n\nsend-pack.c is the client side of push while receive-pack is the\nserver side. There currently doesn't exist a way for a server to\nunderstand the difference between a force/force-with-lease/non-force\npush.\n\n> \n> > this proposal extends these two to be implemented on the server as well.\n> > non-forced variant are basically the \"with-lease\" case and \"force\" now\n> > actually forces an update.\n> \n> I think we have 3 modes:\n> (1) standard push, where both client and server check for a fast-forward\n> (2) \"force\" that blindly overwrites the ref, but as that has a race condition\n> in case multiple people can push to the remove we have\n> (3) \"with-lease\", disables the fast forward check both on client and server\n> but still takes out a lock on the server to ensure no races happen\n> \n> Now you propose to have only 2, making (1) and (3) the same, deferring\n> the check to have \"fast forwards only\" to be client only?\n> The server surely wants to ensure that, too (maybe you need\n> special permission for non-ff; depends on the server implementation).\n> \n> I am not sure I like it, as on the protocol level this indeed looks the same\n> and the server and client need to care in their implementation how/when\n> the ff-check is done. Though it would be nice for the client UX that you\n> need to give a flag to check for ff (both client and server side? or can we rely\n> on the client alone then?)\n\nIIRC there are no checks done server-side for force, with-lease, or\nfast-forward (well fast-forward can be checked for but this is a config\nand has nothing to do with the protocol). Most of the work is done by\nthe client to ensure these checks are made.\n\n> \n> > > > + (delim-pkt | flush-pkt)\n> > > > +\n> > > > + unpack-error = PKT-LINE(\"ERR\" SP error-msg LF)\n> > > > +\n> > > > + ref-update-status = *(update-result | update-error)\n> > > > + update-result = *PKT-LINE(txn_id SP result LF)\n> > > > + result = (\"created\" | \"deleted\" | \"updated\") SP refname SP old_oid SP new_oid\n> > > > + update-error = PKT-LINE(txn_id SP \"error\" SP error-msg LF)\n> > >\n> > > Can we unify \"ERR\" and \"error\" ?\n> >\n> > No, these are very different. You could have one ref update succeed\n> > while another doesn't for some reason, unless you want everything to be\n> > atomic.\n> \n> I did not mean to unify them on the semantic level, but on the\n> representation level, i.e. have both of them spelled the same,\n> as they can still be differentiated by the leading txn id?\n\nOh I misunderstood again :) yeas we could standardize on ERR.\n\n> \n> \n> Thanks,\n> Stefan\n> \n> P.S.: another feature that just came to mind is localisation of error messages.\n> But that is also easy to do with capabilities (the client sends a capability\n> such as \"preferred-i18n=DE\" and the server may translate all its errors\n> if it can.\n> \n> That brings me to another point: do we assume all errors to be read\n> by humans? or do we want some markup things in there, too, similar to\n> EAGAIN?\n\nThis sort of thing could be added as a protocol-level capability where\nthe client sends LANG= so that those sorts of msgs could\nbe translated server side before sending them.\n\n-- \nBrandon Williams\n"},{"id":"353121","messageId":"1dd6d9aa-0e96-bb8e-f7ae-873f619a2450@jeffhostetler.com","threadId":"48900","inReplyTo":"20180718171512.GC17137@google.com","subject":"Re: [RFC] push: add documentation on push v2","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2018-07-20T13:12:53Z","receivedAt":"2018-07-20T13:12:58Z","isPatch":false,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 7/18/2018 1:15 PM, Brandon Williams wrote:\n> On 07/18, Stefan Beller wrote:\n>> On Wed, Jul 18, 2018 at 6:31 AM Derrick Stolee wrote:\n>>>\n>>> On 7/17/2018 7:25 PM, Stefan Beller wrote:\n>>>> On Tue, Jul 17, 2018 at 2:09 PM Brandon Williams wrote:\n>>>>> Signed-off-by: Brandon Williams \n>>>>> ---\n>>>>>\n>>>>> Since introducing protocol v2 and enabling fetch I've been thinking\n>>>>> about what its inverse 'push' would look like. After talking with a\n>>>>> number of people I have a longish list of things that could be done to\n>>>>> improve push and I think I've been able to distill the core features we\n>>>>> want in push v2.\n>>>> It would be nice to know which things you want to improve.\n>>>\n>>> Hopefully we can also get others to chime in with things they don't like\n>>> about the existing protocol. What pain points exist, and what can we do\n>>> to improve at the transport layer before considering new functionality?\n>>\n>> Another thing that I realized last night was the possibility to chunk requests.\n>> The web of today is driven by lots of small http(s) requests. I know our server\n>> team fights with the internal tools all the time because the communication\n>> involved in git-fetch is usually a large http request (large packfile).\n>> So it would be nice to have the possibility of chunking the request.\n>> But I think that can be added as a capability? (Not sure how)\n> \n> Fetch and push requests/responses are already \"chunked\" when using the\n> http transport. So I'm not sure what you mean by adding a capability\n> because the protocol doesn't care about which transport you're using.\n> This is of course unless you're talking about a different \"chunking\"\n> from what it means to chunk an http request/response.\n> \n\nInternally, we've talked about wanting to have resumable pushes and\nfetches. I realize this is difficult to do when the server is\nreplicated and the repeated request might be talking to a different\nserver instance. And there's a problem with temp files littering the\nserver as it waits for the repeated attempt. But still, the packfile\nsent/received can be large and connections do get dropped.\n\nThat is, if we think about sending 1 large packfile and just using a\nbyte-range-like approach to resuming the transfer.\n\nIf we allowed the request to send a series of packfiles, with each\n\"chunk\" being self-contained and usable. So if a push connection was\ndropped the server could apply the successfully received packfile(s)\n(add the received objects and update the refs to the commits received so\nfar). And ignore the interrupted and unreceived packfile(s) and let the\nclient retry later. When/if the client retried the push, it would\nrenegotiate haves/wants and send a new series of packfile(s). With the\nassumption being that the server would have updated refs from the\nearlier aborted push, so the packfile(s) computed for the second attempt\nwould not repeat the content successfully transmitted in the first\nattempt.\n\nThis would require that the client build an ordered set of packfiles\nfrom oldest to newest so that the server can apply them in-order and\nthe graph remain connected. That may be outside your scope here.\n\nAlso, we might have to add a few messages to the protocol after the\nnegotiation, for the client to say that it is going to send the push\ncontent in 'n' packfiles and send 'n' messages with the intermediate\nref values being updated in each packfile.\n\nJust thinking out loud here.\nJeff\n"},{"id":"353511","messageId":"20180724190003.GB225275@google.com","threadId":"48900","inReplyTo":"1dd6d9aa-0e96-bb8e-f7ae-873f619a2450@jeffhostetler.com","subject":"Re: [RFC] push: add documentation on push v2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-07-24T19:00:03Z","receivedAt":"2018-07-24T19:00:08Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 07/20, Jeff Hostetler wrote:\n> \n> \n> On 7/18/2018 1:15 PM, Brandon Williams wrote:\n> > On 07/18, Stefan Beller wrote:\n> > > On Wed, Jul 18, 2018 at 6:31 AM Derrick Stolee wrote:\n> > > > \n> > > > On 7/17/2018 7:25 PM, Stefan Beller wrote:\n> > > > > On Tue, Jul 17, 2018 at 2:09 PM Brandon Williams wrote:\n> > > > > > Signed-off-by: Brandon Williams \n> > > > > > ---\n> > > > > > \n> > > > > > Since introducing protocol v2 and enabling fetch I've been thinking\n> > > > > > about what its inverse 'push' would look like. After talking with a\n> > > > > > number of people I have a longish list of things that could be done to\n> > > > > > improve push and I think I've been able to distill the core features we\n> > > > > > want in push v2.\n> > > > > It would be nice to know which things you want to improve.\n> > > > \n> > > > Hopefully we can also get others to chime in with things they don't like\n> > > > about the existing protocol. What pain points exist, and what can we do\n> > > > to improve at the transport layer before considering new functionality?\n> > > \n> > > Another thing that I realized last night was the possibility to chunk requests.\n> > > The web of today is driven by lots of small http(s) requests. I know our server\n> > > team fights with the internal tools all the time because the communication\n> > > involved in git-fetch is usually a large http request (large packfile).\n> > > So it would be nice to have the possibility of chunking the request.\n> > > But I think that can be added as a capability? (Not sure how)\n> > \n> > Fetch and push requests/responses are already \"chunked\" when using the\n> > http transport. So I'm not sure what you mean by adding a capability\n> > because the protocol doesn't care about which transport you're using.\n> > This is of course unless you're talking about a different \"chunking\"\n> > from what it means to chunk an http request/response.\n> > \n> \n> Internally, we've talked about wanting to have resumable pushes and\n> fetches. I realize this is difficult to do when the server is\n> replicated and the repeated request might be talking to a different\n> server instance. And there's a problem with temp files littering the\n> server as it waits for the repeated attempt. But still, the packfile\n> sent/received can be large and connections do get dropped.\n> \n> That is, if we think about sending 1 large packfile and just using a\n> byte-range-like approach to resuming the transfer.\n> \n> If we allowed the request to send a series of packfiles, with each\n> \"chunk\" being self-contained and usable. So if a push connection was\n> dropped the server could apply the successfully received packfile(s)\n> (add the received objects and update the refs to the commits received so\n> far). And ignore the interrupted and unreceived packfile(s) and let the\n> client retry later. When/if the client retried the push, it would\n> renegotiate haves/wants and send a new series of packfile(s). With the\n> assumption being that the server would have updated refs from the\n> earlier aborted push, so the packfile(s) computed for the second attempt\n> would not repeat the content successfully transmitted in the first\n> attempt.\n> \n> This would require that the client build an ordered set of packfiles\n> from oldest to newest so that the server can apply them in-order and\n> the graph remain connected. That may be outside your scope here.\n> \n> Also, we might have to add a few messages to the protocol after the\n> negotiation, for the client to say that it is going to send the push\n> content in 'n' packfiles and send 'n' messages with the intermediate\n> ref values being updated in each packfile.\n> \n> Just thinking out loud here.\n> Jeff\n\nWe've talked about working on resumable fetch/push (both of which are\nout of the scope of this work), but we haven't started working on\nanything just yet.\n\nThere's a couple different ways to do this like you've pointed out, we\ncan either have the server redirect the client to fetch from a CDN\n(where its put the packfile) and then the client can use ranged requests\nto fetch until the server decides to remove it from the CDN. This can\nbe tricky because every fetch can produce a unique packfile so maybe you\ndon't want to put a freshly constructed, unique packfile for each client\nrequest up on a CDN somewhere.\n\nBreaking up a response into multiple packfiles and small ref-updates\ncould work, that way as long as some of the smaller packs/updates are\napplied then the client is making headway towards being up to date with\nthe server.\n\n-- \nBrandon Williams\n"},{"id":"353516","messageId":"20180724192811.GC225275@google.com","threadId":"48900","inReplyTo":"20180717210915.139521-1-bmwill@google.com","subject":"Re: [RFC] push: add documentation on push v2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-07-24T19:28:11Z","receivedAt":"2018-07-24T19:28:17Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 07/17, Brandon Williams wrote:\n> Signed-off-by: Brandon Williams \n> ---\n> \n> Since introducing protocol v2 and enabling fetch I've been thinking\n> about what its inverse 'push' would look like. After talking with a\n> number of people I have a longish list of things that could be done to\n> improve push and I think I've been able to distill the core features we\n> want in push v2. Thankfully (due to the capability system) most of the\n> other features/improvements can be added later with ease.\n> \n> What I've got now is a rough design for a more flexible push, more\n> flexible because it allows for the server to do what it wants with the\n> refs that are pushed and has the ability to communicate back what was\n> done to the client. The main motivation for this is to work around\n> issues when working with Gerrit and other code-review systems where you\n> need to have Change-Ids in the commit messages (now the server can just\n> insert them for you and send back new commits) and you need to push to\n> magic refs to get around various limitations (now a Gerrit server should\n> be able to communicate that pushing to 'master' doesn't update master\n> but instead creates a refs/changes/ ref).\n> \n> Before actually moving to write any code I'm hoping to get some feedback\n> on if we think this is an acceptable base design for push (other\n> features like atomic-push, signed-push, etc can be added as\n> capabilities), so any comments are appreciated.\n> \n> Documentation/technical/protocol-v2.txt | 76 +++++++++++++++++++++++++\n> 1 file changed, 76 insertions(+)\n\nPinging this thread again to hopefully reach some more people for\ncommentary. Looking back through the comments so far there are concerns\nthat a server shouldn't be trusted rewriting my local changes, so to\naddress that we could have the be a config option which is defaulted to\nnot take changes from a server.\n\nApart from that I didn't see any other major concerns. I'm hoping to\nget a bit more discussion going before actually beginning work on this.\n\n-- \nBrandon Williams\n"},{"id":"353569","messageId":"CACsJy8C9z14GsxfyPm_pDuGwAQqm6Cdi2dO3bsqiYDE0scVbkQ@mail.gmail.com","threadId":"48900","inReplyTo":"20180724192811.GC225275@google.com","subject":"Re: [RFC] push: add documentation on push v2","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-07-25T15:15:03Z","receivedAt":"2018-07-25T15:15:33Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jul 24, 2018 at 9:29 PM Brandon Williams wrote:\n>\n> On 07/17, Brandon Williams wrote:\n> > Signed-off-by: Brandon Williams \n> > ---\n> >\n> > Since introducing protocol v2 and enabling fetch I've been thinking\n> > about what its inverse 'push' would look like. After talking with a\n> > number of people I have a longish list of things that could be done to\n> > improve push and I think I've been able to distill the core features we\n> > want in push v2. Thankfully (due to the capability system) most of the\n> > other features/improvements can be added later with ease.\n> >\n> > What I've got now is a rough design for a more flexible push, more\n> > flexible because it allows for the server to do what it wants with the\n> > refs that are pushed and has the ability to communicate back what was\n> > done to the client. The main motivation for this is to work around\n> > issues when working with Gerrit and other code-review systems where you\n> > need to have Change-Ids in the commit messages (now the server can just\n> > insert them for you and send back new commits) and you need to push to\n> > magic refs to get around various limitations (now a Gerrit server should\n> > be able to communicate that pushing to 'master' doesn't update master\n> > but instead creates a refs/changes/ ref).\n> >\n> > Before actually moving to write any code I'm hoping to get some feedback\n> > on if we think this is an acceptable base design for push (other\n> > features like atomic-push, signed-push, etc can be added as\n> > capabilities), so any comments are appreciated.\n> >\n> > Documentation/technical/protocol-v2.txt | 76 +++++++++++++++++++++++++\n> > 1 file changed, 76 insertions(+)\n>\n> Pinging this thread again to hopefully reach some more people for\n> commentary.\n\nCould you send a v2 that covers all the push features in pack version\n1? I see some are discussed but it's probably good to summarize in\nthis document too.\n\nA few other comments\n\nIf I remember correctly, we always update the remote refs locally\nafter a push, assuming that the next 'fetch' will do the same anyway.\nThis is not true for special refs (like those from gerrit). Looking\nfrom this I don't think it can say \"yes we have received your pack and\nstored it \"somewhere\" but there's no visible ref created for it\" so\nthat we can skip the local remote ref update?\n\nIs it simpler to tell a push client at the end that \"yes there's new\nstuff now on the server, do another fetch\", sort of like HTTP\nredirect, then the client can switch to fetch protocol to get the new\nstuff that the server has created (e.g. rebase stuff)? I assume we\ncould reuse the same connection for both push and fetch if needed.\nThis way both fetch and push send packs in just one direction.\n-- \nDuy\n"},{"id":"353578","messageId":"20180725174622.GA4850@google.com","threadId":"48900","inReplyTo":"CACsJy8C9z14GsxfyPm_pDuGwAQqm6Cdi2dO3bsqiYDE0scVbkQ@mail.gmail.com","subject":"Re: [RFC] push: add documentation on push v2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-07-25T17:46:22Z","receivedAt":"2018-07-25T17:46:27Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 07/25, Duy Nguyen wrote:\n> On Tue, Jul 24, 2018 at 9:29 PM Brandon Williams wrote:\n> >\n> > On 07/17, Brandon Williams wrote:\n> > > Signed-off-by: Brandon Williams \n> > > ---\n> > >\n> > > Since introducing protocol v2 and enabling fetch I've been thinking\n> > > about what its inverse 'push' would look like. After talking with a\n> > > number of people I have a longish list of things that could be done to\n> > > improve push and I think I've been able to distill the core features we\n> > > want in push v2. Thankfully (due to the capability system) most of the\n> > > other features/improvements can be added later with ease.\n> > >\n> > > What I've got now is a rough design for a more flexible push, more\n> > > flexible because it allows for the server to do what it wants with the\n> > > refs that are pushed and has the ability to communicate back what was\n> > > done to the client. The main motivation for this is to work around\n> > > issues when working with Gerrit and other code-review systems where you\n> > > need to have Change-Ids in the commit messages (now the server can just\n> > > insert them for you and send back new commits) and you need to push to\n> > > magic refs to get around various limitations (now a Gerrit server should\n> > > be able to communicate that pushing to 'master' doesn't update master\n> > > but instead creates a refs/changes/ ref).\n> > >\n> > > Before actually moving to write any code I'm hoping to get some feedback\n> > > on if we think this is an acceptable base design for push (other\n> > > features like atomic-push, signed-push, etc can be added as\n> > > capabilities), so any comments are appreciated.\n> > >\n> > > Documentation/technical/protocol-v2.txt | 76 +++++++++++++++++++++++++\n> > > 1 file changed, 76 insertions(+)\n> >\n> > Pinging this thread again to hopefully reach some more people for\n> > commentary.\n> \n> Could you send a v2 that covers all the push features in pack version\n> 1? I see some are discussed but it's probably good to summarize in\n> this document too.\n\nI can mention the ones we want to implement, but I expect that a push v2\nwould not require that all features in the current push are supported\nout of the box. Some servers may not want to support signed-push, etc.\nAlso I don't want to have to implement every single feature that exists\nbefore getting something merged. This way follow on series can be\nwritten to implement those as new features to push v2.\n\n> \n> A few other comments\n> \n> If I remember correctly, we always update the remote refs locally\n> after a push, assuming that the next 'fetch' will do the same anyway.\n> This is not true for special refs (like those from gerrit). Looking\n> from this I don't think it can say \"yes we have received your pack and\n> stored it \"somewhere\" but there's no visible ref created for it\" so\n> that we can skip the local remote ref update?\n\nThis is one of the pain points for gerrit and one of the reasons why\nthey have this funky push syntax \"push origin HEAD:refs/for/master\".\nBecause its not a remote tracking branch for the current branch, we\ndon't update (or create) a local branch \"for/master\" under the\n\"refs/remotes/origin\" namespace (at least that's how i understand it).\n\nSo in order to support the server doing more things (rebasing, or\ncreating new branches) based on what is pushed the status that the\nserver sends in response needs to be more fluid so that the server can\ndescribe what it did in a way that the client can either: update the\nremote tracking branches, or not (and in this case maybe do what you\nsuggest down below).\n\n> \n> Is it simpler to tell a push client at the end that \"yes there's new\n> stuff now on the server, do another fetch\", sort of like HTTP\n> redirect, then the client can switch to fetch protocol to get the new\n> stuff that the server has created (e.g. rebase stuff)? I assume we\n> could reuse the same connection for both push and fetch if needed.\n> This way both fetch and push send packs in just one direction.\n\nI really, really like this suggestion. Thank you for your input. This\nwould actually make the protocol much simpler by keeping push for\npushing packs and fetch for fetching packs. And since my plan is to\nhave the status-report for push include all the relevant ref changes\nthat the server made, if there are any that don't correspond with what\nwas pushed (either the server rebased a change or created a new ref I\ndidn't) then we can skip the ref-advertisement and go straight to\nfetching those refs. And yes, since protocol v2 is command based we\ncould reuse the existing connection and simply send a \"fetch\" request\nafter our \"push\" one.\n\nThis does mean that it'll be an extra round trip than what I originally\nhad in mind, but it should be simpler.\n\n-- \nBrandon Williams\n"},{"id":"354264","messageId":"CACsJy8CO=ZUgKgnmM--BpNFFgvM-dAyckeXKeZXHrtzZcZiC+g@mail.gmail.com","threadId":"48900","inReplyTo":"20180725174622.GA4850@google.com","subject":"Re: [RFC] push: add documentation on push v2","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-02T15:17:53Z","receivedAt":"2018-08-02T15:18:21Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jul 25, 2018 at 7:46 PM Brandon Williams wrote:\n> > Could you send a v2 that covers all the push features in pack version\n> > 1? I see some are discussed but it's probably good to summarize in\n> > this document too.\n>\n> I can mention the ones we want to implement, but I expect that a push v2\n> would not require that all features in the current push are supported\n> out of the box. Some servers may not want to support signed-push, etc.\n> Also I don't want to have to implement every single feature that exists\n> before getting something merged. This way follow on series can be\n> written to implement those as new features to push v2.\n\nI thought I wrote this mail but apparently I did not for some unknown\nreason. My concern here is painting ourselves in the corner and having\na rough sketch of how current features will be implemented or replaced\nin v2 would help reduce that risk.\n-- \nDuy\n"}]}