{"thread":{"id":"42850","subject":"[PATCHv6 2/4] receive-pack: implement advertising and receiving push options","startedAt":"2016-07-14T21:25:06Z","lastAt":"2016-07-14T21:41:38Z","messageCount":3,"participants":["Stefan Beller","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"291481","messageId":"20160714212454.16643-1-sbeller@google.com","threadId":"42850","inReplyTo":null,"subject":"[PATCHv6 2/4] receive-pack: implement advertising and receiving push options","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-14T21:24:54Z","receivedAt":"2016-07-14T21:25:06Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The pre/post receive hook may be interested in more information from the\nuser. This information can be transmitted when both client and server\nsupport the \"push-options\" capability, which when used is a phase directly\nafter update commands ended by a flush pkt.\n\nSimilar to the atomic option, the server capability can be disabled via\nthe `receive.advertisePushOptions` config variable. While documenting\nthis, fix a nit in the `receive.advertiseAtomic` wording.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n Junio,\n please replace v5 2/4 with this patch (I only resend this single patch\n as the other 3 remain as is).\n\n This only changes read_push_options to not care at all about any\n limitations.\n \n Thanks,\n Stefan\n\n# interdiff to v5:\n# diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n# index 917ac18..9bb9afc 100644\n# --- a/builtin/receive-pack.c\n# +++ b/builtin/receive-pack.c\n# @@ -1505,7 +1505,7 @@ static struct string_list *read_push_options(void)\n#  \tstring_list_init(ret, 1);\n#  \n#  \twhile (1) {\n# -\t\tchar *line, *lf;\n# +\t\tchar *line;\n#  \t\tint len;\n#  \n#  \t\tline = packet_read_line(0, &len);\n# @@ -1513,30 +1513,6 @@ static struct string_list *read_push_options(void)\n#  \t\tif (!line)\n#  \t\t\tbreak;\n#  \n# -\t\t/*\n# -\t\t* NEEDSWORK: expose the limitations to be configurable;\n# -\t\t* Once the limit can be lifted, include a way for payloads\n# -\t\t* larger than one pkt, e.g use last byte to indicate if\n# -\t\t* the push option continues in the next packet or implement\n# -\t\t* larger packets.\n# -\t\t*/\n# -\t\tif (len > LARGE_PACKET_MAX - 1) {\n# -\t\t\t/*\n# -\t\t\t * NEEDSWORK: The error message in die(..) is not\n# -\t\t\t * transmitted in call cases, so ideally all die(..)\n# -\t\t\t * calls are prefixed with rp_error and then we can\n# -\t\t\t * combine rp_error && die into one helper function.\n# -\t\t\t */\n# -\t\t\trp_error(\"protocol error: server configuration allows push \"\n# -\t\t\t\t \"options of size up to %d bytes\",\n# -\t\t\t\t LARGE_PACKET_MAX - 1);\n# -\t\t\tdie(\"protocol error: push options too large\");\n# -\t\t}\n# -\n# -\t\tlf = strchr(line, '\\n');\n# -\t\tif (lf)\n# -\t\t\t*lf = '\\0';\n# -\n#  \t\tstring_list_append(ret, line);\n#  \t}\n\n\n Documentation/config.txt                          |  9 ++++--\n Documentation/technical/pack-protocol.txt         | 10 ++++---\n Documentation/technical/protocol-capabilities.txt |  9 ++++++\n builtin/receive-pack.c                            | 35 +++++++++++++++++++++++\n 4 files changed, 57 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 16dc22d..0bb6daa 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2417,8 +2417,13 @@ rebase.instructionFormat\n \n receive.advertiseAtomic::\n \tBy default, git-receive-pack will advertise the atomic push\n-\tcapability to its clients. If you don't want to this capability\n-\tto be advertised, set this variable to false.\n+\tcapability to its clients. If you don't want to advertise this\n+\tcapability, set this variable to false.\n+\n+receive.advertisePushOptions::\n+\tBy default, git-receive-pack will advertise the push options\n+\tcapability to its clients. If you don't want to advertise this\n+\tcapability, set this variable to false.\n \n receive.autogc::\n \tBy default, git-receive-pack will run \"git-gc --auto\" after\ndiff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\nindex 8b36343..7a2ed30 100644\n--- a/Documentation/technical/pack-protocol.txt\n+++ b/Documentation/technical/pack-protocol.txt\n@@ -454,7 +454,8 @@ The reference discovery phase is done nearly the same way as it is in the\n fetching protocol. Each reference obj-id and name on the server is sent\n in packet-line format to the client, followed by a flush-pkt.  The only\n real difference is that the capability listing is different - the only\n-possible values are 'report-status', 'delete-refs' and 'ofs-delta'.\n+possible values are 'report-status', 'delete-refs', 'ofs-delta' and\n+'push-options'.\n \n Reference Update Request and Packfile Transfer\n ----------------------------------------------\n@@ -465,9 +466,10 @@ that it wants to update, it sends a line listing the obj-id currently on\n the server, the obj-id the client would like to update it to and the name\n of the reference.\n \n-This list is followed by a flush-pkt and then the packfile that should\n-contain all the objects that the server will need to complete the new\n-references.\n+This list is followed by a flush-pkt. Then the push options are transmitted\n+one per packet followed by another flush-pkt. After that the packfile that\n+should contain all the objects that the server will need to complete the new\n+references will be sent.\n \n ----\n   update-request    =  *shallow ( command-list | push-cert ) [packfile]\ndiff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\nindex eaab6b4..4c28d3a 100644\n--- a/Documentation/technical/protocol-capabilities.txt\n+++ b/Documentation/technical/protocol-capabilities.txt\n@@ -253,6 +253,15 @@ atomic pushes. If the pushing client requests this capability, the server\n will update the refs in one atomic transaction. Either all refs are\n updated or none.\n \n+push-options\n+------------\n+\n+If the server sends the 'push-options' capability it is able to accept\n+push options after the update commands have been sent, but before the\n+packfile is streamed. If the pushing client requests this capability,\n+the server will pass the options to the pre- and post- receive hooks\n+that process this push request.\n+\n allow-tip-sha1-in-want\n ----------------------\n \ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 236417e..9bb9afc 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -44,10 +44,12 @@ static struct strbuf fsck_msg_types = STRBUF_INIT;\n static int receive_unpack_limit = -1;\n static int transfer_unpack_limit = -1;\n static int advertise_atomic_push = 1;\n+static int advertise_push_options;\n static int unpack_limit = 100;\n static int report_status;\n static int use_sideband;\n static int use_atomic;\n+static int use_push_options;\n static int quiet;\n static int prefer_ofs_delta = 1;\n static int auto_update_server_info;\n@@ -193,6 +195,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (strcmp(var, \"receive.advertisepushoptions\") == 0) {\n+\t\tadvertise_push_options = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \treturn git_default_config(var, value, cb);\n }\n \n@@ -211,6 +218,8 @@ static void show_ref(const char *path, const unsigned char *sha1)\n \t\t\tstrbuf_addstr(&cap, \" ofs-delta\");\n \t\tif (push_cert_nonce)\n \t\t\tstrbuf_addf(&cap, \" push-cert=%s\", push_cert_nonce);\n+\t\tif (advertise_push_options)\n+\t\t\tstrbuf_addstr(&cap, \" push-options\");\n \t\tstrbuf_addf(&cap, \" agent=%s\", git_user_agent_sanitized());\n \t\tpacket_write(1, \"%s %s%c%s\\n\",\n \t\t\t     sha1_to_hex(sha1), path, 0, cap.buf);\n@@ -1455,6 +1464,9 @@ static struct command *read_head_info(struct sha1_array *shallow)\n \t\t\tif (advertise_atomic_push\n \t\t\t    && parse_feature_request(feature_list, \"atomic\"))\n \t\t\t\tuse_atomic = 1;\n+\t\t\tif (advertise_push_options\n+\t\t\t    && parse_feature_request(feature_list, \"push-options\"))\n+\t\t\t\tuse_push_options = 1;\n \t\t}\n \n \t\tif (!strcmp(line, \"push-cert\")) {\n@@ -1487,6 +1499,26 @@ static struct command *read_head_info(struct sha1_array *shallow)\n \treturn commands;\n }\n \n+static struct string_list *read_push_options(void)\n+{\n+\tstruct string_list *ret = xmalloc(sizeof(*ret));\n+\tstring_list_init(ret, 1);\n+\n+\twhile (1) {\n+\t\tchar *line;\n+\t\tint len;\n+\n+\t\tline = packet_read_line(0, &len);\n+\n+\t\tif (!line)\n+\t\t\tbreak;\n+\n+\t\tstring_list_append(ret, line);\n+\t}\n+\n+\treturn ret;\n+}\n+\n static const char *parse_pack_header(struct pack_header *hdr)\n {\n \tswitch (read_pack_header(0, hdr)) {\n@@ -1774,6 +1806,9 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t\tconst char *unpack_status = NULL;\n \t\tstruct string_list *push_options = NULL;\n \n+\t\tif (use_push_options)\n+\t\t\tpush_options = read_push_options();\n+\n \t\tprepare_shallow_info(&si, &shallow);\n \t\tif (!si.nr_ours && !si.nr_theirs)\n \t\t\tshallow_update = 0;\n-- \n2.9.0.247.gf748855.dirty\n\n"},{"id":"291483","messageId":"20160714213534.GB20645@sigill.intra.peff.net","threadId":"42850","inReplyTo":"20160714212454.16643-1-sbeller@google.com","subject":"Re: [PATCHv6 2/4] receive-pack: implement advertising and receiving push options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-14T21:35:35Z","receivedAt":"2016-07-14T21:35:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 14, 2016 at 02:24:54PM -0700, Stefan Beller wrote:\n\n> # interdiff to v5:\n> [...giant deletion...]\n\nMuch nicer. :)\n\n> +static struct string_list *read_push_options(void)\n> +{\n> +\tstruct string_list *ret = xmalloc(sizeof(*ret));\n\nThis struck me as a little non-idiomatic for our code base. The usual\ntechnique is to take a pointer to a stack-allocated struct, and write\ninto that.\n\nI guess that here:\n\n> @@ -1774,6 +1806,9 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n>  \t\tconst char *unpack_status = NULL;\n>  \t\tstruct string_list *push_options = NULL;\n>  \n> +\t\tif (use_push_options)\n> +\t\t\tpush_options = read_push_options();\n> +\n\nYou will want to later check whether push_options is NULL. But you can\nalso just check push_options.nr.\n\n-Peff\n"},{"id":"291484","messageId":"CAGZ79kbu8H9=SX-KKupaNnLUu_VseNQmm5BTsOP4EopMwbqWuA@mail.gmail.com","threadId":"42850","inReplyTo":"20160714213534.GB20645@sigill.intra.peff.net","subject":"Re: [PATCHv6 2/4] receive-pack: implement advertising and receiving push options","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-14T21:40:57Z","receivedAt":"2016-07-14T21:41:38Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jul 14, 2016 at 2:35 PM, Jeff King <peff@peff.net> wrote:\n> On Thu, Jul 14, 2016 at 02:24:54PM -0700, Stefan Beller wrote:\n>\n>> # interdiff to v5:\n>> [...giant deletion...]\n>\n> Much nicer. :)\n>\n>> +static struct string_list *read_push_options(void)\n>> +{\n>> +     struct string_list *ret = xmalloc(sizeof(*ret));\n>\n> This struck me as a little non-idiomatic for our code base. The usual\n> technique is to take a pointer to a stack-allocated struct, and write\n> into that.\n\nOh, right! :(\n\n    That's what you get when you grow up with object orientation all along.\n    read_push_options() mentally mapped to \"create a push options object if any\"\n    such that I can see if it is NULL or not.\n\nI'll reroll a more idiomatic thing. Thanks,\nStefan\n\n>\n> I guess that here:\n>\n>> @@ -1774,6 +1806,9 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n>>               const char *unpack_status = NULL;\n>>               struct string_list *push_options = NULL;\n>>\n>> +             if (use_push_options)\n>> +                     push_options = read_push_options();\n>> +\n>\n> You will want to later check whether push_options is NULL. But you can\n> also just check push_options.nr.\n>\n> -Peff\n"}]}