{"thread":{"id":"42847","subject":"[PATCHv5 0/4] Push options","startedAt":"2016-07-14T17:39:44Z","lastAt":"2016-07-14T20:07:59Z","messageCount":13,"participants":["Stefan Beller","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"291441","messageId":"20160714173920.22001-1-sbeller@google.com","threadId":"42847","inReplyTo":null,"subject":"[PATCHv5 0/4] Push options","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-14T17:39:16Z","receivedAt":"2016-07-14T17:39:44Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Jeff wrote:\n> Junio wrote:\n>> I think those extra knobs can come later.  If we are not going to\n>> limit with max_options in the end, however, wouldn't it be more\n>> natural for the initial iteration without any configuration not to\n>> have hard-coded max_options at all?\n>\n> Yeah, I am OK with adding restrictive knobs later as a separate topic.\n> As Stefan notes, upstream does not have the other knobs anyway, and IIRC\n> the push-options feature is not even enabled by default.\n>\n> -Peff\n\n* now it actually is not a default. ;)\n* removed knobs, but instead we only reject at > LARGE_PACKET_MAX - 1,\n\nThanks,\nStefan\n\nv5:\ngit diff origin/sb/push-options:\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 4d8041a..917ac18 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -44,7 +44,7 @@ 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 = 1;\n+static int advertise_push_options;\n static int unpack_limit = 100;\n static int report_status;\n static int use_sideband;\n@@ -1501,24 +1501,11 @@ static struct command *read_head_info(struct sha1_array *shallow)\n \n static struct string_list *read_push_options(void)\n {\n-\tint i;\n \tstruct string_list *ret = xmalloc(sizeof(*ret));\n-\t/* NEEDSWORK: expose the limitations to be configurable. */\n-\tint max_options = 32;\n-\n-\t/*\n-\t * NEEDSWORK: expose the limitations to be configurable;\n-\t * Once the limit can be lifted, include a way for payloads\n-\t * larger than one pkt, e.g allow a payload of up to\n-\t * LARGE_PACKET_MAX - 1 only, and reserve the last byte\n-\t * to indicate whether the next pkt continues with this\n-\t * push option.\n-\t */\n-\tint max_size = 1024;\n-\n \tstring_list_init(ret, 1);\n-\tfor (i = 0; i < max_options; i++) {\n-\t\tchar *line;\n+\n+\twhile (1) {\n+\t\tchar *line, *lf;\n \t\tint len;\n \n \t\tline = packet_read_line(0, &len);\n@@ -1526,7 +1513,14 @@ static struct string_list *read_push_options(void)\n \t\tif (!line)\n \t\t\tbreak;\n \n-\t\tif (len > max_size) {\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@@ -1534,20 +1528,17 @@ static struct string_list *read_push_options(void)\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\", max_size);\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\tlen = strcspn(line, \"\\n\");\n-\t\tline[len] = '\\0';\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-\tif (i == max_options) {\n-\t\trp_error(\"protocol error: server configuration only allows up \"\n-\t\t    \"to %d push options\", max_options);\n-\t\tdie(\"protocol error: push options too large\");\n-\t}\n \n \treturn ret;\n }\ndiff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh\nindex 8dd3c8e..ea813b9 100755\n--- a/t/t5545-push-options.sh\n+++ b/t/t5545-push-options.sh\n@@ -57,6 +57,7 @@ test_refs () {\n \n test_expect_success 'one push option works for a single branch' '\n \tmk_repo_pair &&\n+\tgit -C upstream config receive.advertisePushOptions true &&\n \t(\n \t\tcd workbench &&\n \t\ttest_commit one &&\n@@ -85,6 +86,7 @@ test_expect_success 'push option denied by remote' '\n \n test_expect_success 'two push options work' '\n \tmk_repo_pair &&\n+\tgit -C upstream config receive.advertisePushOptions true &&\n \t(\n \t\tcd workbench &&\n \t\ttest_commit one &&\n\n\n\ncover letter v4:\n\nThanks Junio, Jeff, Jonathan for discussion and feedback!\n\nI went over the emails again and we seem to agree that the initial design (in v3)\nwas sane and the error messages and reporting for corner cases were to be\ndismissed as \"it happens as often as 'BUG:' messages appear, so let's not care\ntoo deeply now\".\n\nThanks,\nStefan\n\nThis is a diff against a modified v3 (it's actually origin/sb/push-options):\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex df1b314..25b5db1 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2410,13 +2410,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 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 capability\n-\tto its clients. If you don't want this capability\n-\tto be advertised, set this variable to false.\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/git-push.txt b/Documentation/git-push.txt\nindex b0b1273..e960258 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -156,11 +156,11 @@ already exists on the remote side.\n \tEither all refs are updated, or on error, no refs are updated.\n \tIf the server does not support atomic pushes the push will fail.\n \n--L::\n+-o::\n --push-option::\n \tTransmit the given string to the server, which passes them to\n-\tthe pre-receive as well as the post-receive hook. Only C strings\n-\tcontaining no new lines are allowed.\n+\tthe pre-receive as well as the post-receive hook. The given string\n+\tmust not contain a NUL or LF character.\n \n --receive-pack=<git-receive-pack>::\n --exec=<git-receive-pack>::\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex c875cde..9565dc3 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -247,8 +247,14 @@ Both standard output and standard error output are forwarded to\n 'git send-pack' on the other end, so you can simply `echo` messages\n for the user.\n \n-The number of push options are available in the variable GIT_PUSH_OPTION_COUNT\n-and the options itself are in GIT_PUSH_OPTION_{0,1,...}.\n+The number of push options given on the command line of\n+`git push --push-option=...` can be read from the environment\n+variable `GIT_PUSH_OPTION_COUNT`, and the options themselves are\n+found in `GIT_PUSH_OPTION_0`, `GIT_PUSH_OPTION_1`,...\n+If it is negotiated to not use the push options phase, the\n+environment variables will not be set. If the client selects\n+to use push options, but doesn't transmit any, the count variable\n+will be set to zero, `GIT_PUSH_OPTION_COUNT=0`.\n \n [[update]]\n update\n@@ -325,8 +331,14 @@ a sample script `post-receive-email` provided in the `contrib/hooks`\n directory in Git distribution, which implements sending commit\n emails.\n \n-The number of push options are available in the variable GIT_PUSH_OPTION_COUNT\n-and the options itself are in GIT_PUSH_OPTION_{0,1,...}.\n+The number of push options given on the command line of\n+`git push --push-option=...` can be read from the environment\n+variable `GIT_PUSH_OPTION_COUNT`, and the options themselves are\n+found in `GIT_PUSH_OPTION_0`, `GIT_PUSH_OPTION_1`,...\n+If it is negotiated to not use the push options phase, the\n+environment variables will not be set. If the client selects\n+to use push options, but doesn't transmit any, the count variable\n+will be set to zero, `GIT_PUSH_OPTION_COUNT=0`.\n \n [[post-update]]\n post-update\ndiff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt\nindex b71eda9..4c28d3a 100644\n--- a/Documentation/technical/protocol-capabilities.txt\n+++ b/Documentation/technical/protocol-capabilities.txt\n@@ -256,10 +256,11 @@ updated or none.\n push-options\n ------------\n \n-If the server sends the 'push-options' capability it is capable to accept\n-push options after the update commands have been sent. If the pushing client\n-requests this capability, the server will pass the options to the pre and post\n-receive hooks that process this push request.\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 ----------------------\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 1b5d205..3bb9d6b 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -508,6 +508,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \tint rc;\n \tconst char *repo = NULL;\t/* default repository */\n \tstatic struct string_list push_options = STRING_LIST_INIT_DUP;\n+\tstatic struct string_list_item *item;\n \n \tstruct option options[] = {\n \t\tOPT__VERBOSITY(&verbosity),\n@@ -573,6 +574,10 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\tset_refspecs(argv + 1, argc - 1, repo);\n \t}\n \n+\tfor_each_string_list_item(item, &push_options)\n+\t\tif (strchr(item->string, '\\n'))\n+\t\t\tdie(_(\"push options must not have new line characters\"));\n+\n \trc = do_push(repo, flags, &push_options);\n \tif (rc == -1)\n \t\tusage_with_options(push_usage, options);\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex e71041a..754db6e 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -214,12 +214,12 @@ static void show_ref(const char *path, const unsigned char *sha1)\n \t\t\t      \"report-status delete-refs side-band-64k quiet\");\n \t\tif (advertise_atomic_push)\n \t\t\tstrbuf_addstr(&cap, \" atomic\");\n-\t\tif (advertise_push_options)\n-\t\t\tstrbuf_addstr(&cap, \" push-options\");\n \t\tif (prefer_ofs_delta)\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@@ -584,7 +584,7 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,\n \tproc.argv = argv;\n \tproc.in = -1;\n \tproc.stdout_to_stderr = 1;\n-\tif (feed_state && feed_state->push_options) {\n+\tif (feed_state->push_options) {\n \t\tint i;\n \t\tfor (i = 0; i < feed_state->push_options->nr; i++)\n \t\t\targv_array_pushf(&proc.env_array,\n@@ -592,7 +592,8 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,\n \t\t\t\tfeed_state->push_options->items[i].string);\n \t\targv_array_pushf(&proc.env_array, \"GIT_PUSH_OPTION_COUNT=%d\",\n \t\t\t\t feed_state->push_options->nr);\n-\t}\n+\t} else\n+\t\targv_array_pushf(&proc.env_array, \"GIT_PUSH_OPTION_COUNT\");\n \n \tif (use_sideband) {\n \t\tmemset(&muxer, 0, sizeof(muxer));\n@@ -1498,7 +1499,7 @@ static struct command *read_head_info(struct sha1_array *shallow)\n \treturn commands;\n }\n \n-static struct string_list *read_push_options()\n+static struct string_list *read_push_options(void)\n {\n \tint i;\n \tstruct string_list *ret = xmalloc(sizeof(*ret));\n@@ -1526,18 +1527,28 @@ static struct string_list *read_push_options()\n \t\tif (!line)\n \t\t\tbreak;\n \n-\t\tif (len > max_size)\n-\t\t\tdie(\"protocol error: server configuration allows push \"\n-\t\t\t    \"options of size up to %d bytes\", max_size);\n+\t\tif (len > max_size) {\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\", max_size);\n+\t\t\tdie(\"protocol error: push options too large\");\n+\t\t}\n \n \t\tlen = strcspn(line, \"\\n\");\n \t\tline[len] = '\\0';\n \n \t\tstring_list_append(ret, line);\n \t}\n-\tif (i == max_options)\n-\t\tdie(\"protocol error: server configuration only allows up \"\n+\tif (i == max_options) {\n+\t\trp_error(\"protocol error: server configuration only allows up \"\n \t\t    \"to %d push options\", max_options);\n+\t\tdie(\"protocol error: push options too large\");\n+\t}\n \n \treturn ret;\n }\ndiff --git a/send-pack.c b/send-pack.c\nindex c943560..3a842ac 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -277,13 +277,10 @@ static int generate_push_cert(struct strbuf *req_buf,\n \t}\n \tif (push_cert_nonce[0])\n \t\tstrbuf_addf(&cert, \"nonce %s\\n\", push_cert_nonce);\n-\tstrbuf_addstr(&cert, \"\\n\");\n-\n-\tif (args->push_options) {\n+\tif (args->push_options)\n \t\tfor_each_string_list_item(item, args->push_options)\n \t\t\tstrbuf_addf(&cert, \"push-option %s\\n\", item->string);\n-\t\tstrbuf_addstr(&cert, \"\\n\");\n-\t}\n+\tstrbuf_addstr(&cert, \"\\n\");\n \n \tfor (ref = remote_refs; ref; ref = ref->next) {\n \t\tif (check_to_send_update(ref, args) < 0)\ndiff --git a/templates/hooks--pre-receive.sample b/templates/hooks--pre-receive.sample\nindex e4d3edc..a1fd29e 100644\n--- a/templates/hooks--pre-receive.sample\n+++ b/templates/hooks--pre-receive.sample\n@@ -6,13 +6,15 @@\n #\n # To enable this hook, rename this file to \"pre-receive\".\n \n-if test -n \"$GIT_PUSH_OPTION_COUNT\"; then\n+if test -n \"$GIT_PUSH_OPTION_COUNT\"\n+then\n \ti=0\n-\twhile test \"$i\" -lt \"$GIT_PUSH_OPTION_COUNT\"; do\n+\twhile test \"$i\" -lt \"$GIT_PUSH_OPTION_COUNT\"\n+\tdo\n \t\teval \"value=\\$GIT_PUSH_OPTION_$i\"\n \t\tcase \"$value\" in\n \t\techoback=*)\n-\t\t\techo \"echo from the pre-receive-hook ${value#*=}\" >&2\n+\t\t\techo \"echo from the pre-receive-hook: ${value#*=}\" >&2\n \t\t\t;;\n \t\treject)\n \t\t\texit 1\n\n\nCover letter v3:\n================\n\nThis is not marked for RFC any more, as I do not recall any open points\nleft for discussion. This addresses the only reply from Eric Wong on patch 3:\n\ndiff to v2:\ndiff --git a/send-pack.c b/send-pack.c\nindex e328276..c943560 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -536,7 +536,8 @@ int send_pack(struct send_pack_args *args,\n \n                for_each_string_list_item(item, args->push_options)\n                        packet_buf_write(&sb, \"%s\", item->string);\n-                       write_or_die(out, sb.buf, sb.len);\n+\n+               write_or_die(out, sb.buf, sb.len);\n                packet_flush(out);\n                strbuf_release(&sb);\n        }\ndiff --git a/transport.c b/transport.c\nindex 598bd1f..0298be1 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -641,7 +641,6 @@ struct transport *transport_get(struct remote *remote, const char *url)\n        struct transport *ret = xcalloc(1, sizeof(*ret));\n \n        ret->progress = isatty(2);\n-       ret->push_options = NULL;\n \n        if (!remote)\n                die(\"No remote provided to transport_get()\");\n\nCover letter v2:\n================\n\nAllow a user to pass information along a push to the pre/post-receive hook\non the remote.\n\nJeff writes on v1:\n> Whereas in Dennis's patches, it was about specific information directly\n> related to the act of pushing.\n\nThis allows to transmit arbitrary information as the backends of $VENDOR\nmay have different options available related to the direct act of pushing.\n\nThanks,\nStefan\n\nCover letter v1:\n================\n\nAllow a user to pass information along a push to the pre/post-receive hook\non the remote.\n\nWhen using a remote that is more than just a plain Git host (e.g. Gerrit,\nGit{hub/lab}, etc) this may become more obvious: The (server backend specific)\npush options can instruct the server to:\n* open a pull request\n* send out emails asking for review\n* (un)trigger continuous integration\n* set priority for continuous integration (i.e. bots pushing may ask to be\n  treated with lower priority compared to humans)\n* ... \n\nMost of these actions can be done on the client side as well,\nbut in these remote-centric workflows it is easier to do that on the remote,\nwhich is why we need to transport the information there.\n\nMore concrete examples:\n* When you want a change in Gerrit to be submitted to refs/heads/master, you\n  push instead to a magic branch refs/for/master and Gerrit will create a change\n  for you (similar to a pull request). Instead we could imagine that you push\n  to a magical refs/heads/master with a push option \"create-change\".\n  \n* When pushing to Gerrit you can already attach some of this information by\n  adding a '%' followed by the parameter to the ref, i.e. when interacting with\n  Gerrit it is possible to do things like[1]:\n    \n    git push origin HEAD:refs/for/master%draft%topic=example%cc=jon.doe@example.org\n  \n  This is not appealing to our users as it looks like hacks upon hacks to make\n  it work. It would read better if it was spelled as:\n  \n  git push origin HEAD:refs/for/master \\\n      --push-option draft \\\n      --push-option topic=example \\\n      --push-option cc=jon.doe@example.org\n      \n  (with a short form that is even easier to type,\n   but this is is more intuitive already)\n\nThis is a patch series to Git core, which is developed at the same time\nas a change is proposed to JGit by Dan Wang, see [2].\n\nThis code is also available at [3].\n\nThanks,\nStefan\n\n[1] Not all Gerrit '%' options are documented, so here is a link to source code instead :(\nhttps://gerrit.googlesource.com/gerrit/+/refs/heads/master/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java#1141\n\n[2] https://git.eclipse.org/r/#/c/74570/ \n \n[3] https://github.com/stefanbeller/git/tree/pushoptions\n\nStefan Beller (4):\n  push options: {pre,post}-receive hook learns about push options\n  receive-pack: implement advertising and receiving push options\n  push: accept push options\n  add a test for push options\n\n Documentation/config.txt                          |  7 +-\n Documentation/git-push.txt                        |  8 ++-\n Documentation/githooks.txt                        |  4 ++\n Documentation/technical/pack-protocol.txt         | 10 +--\n Documentation/technical/protocol-capabilities.txt |  8 +++\n builtin/push.c                                    | 16 ++++-\n builtin/receive-pack.c                            | 85 +++++++++++++++++++----\n send-pack.c                                       | 29 ++++++++\n send-pack.h                                       |  3 +\n t/t5544-push-options.sh                           | 85 +++++++++++++++++++++++\n transport.c                                       |  2 +\n transport.h                                       |  7 ++\n 12 files changed, 242 insertions(+), 22 deletions(-)\n create mode 100755 t/t5544-push-options.sh\n\n-- \n2.9.0.141.gdd65b60\n\n"},{"id":"291442","messageId":"20160714173920.22001-2-sbeller@google.com","threadId":"42847","inReplyTo":"20160714173920.22001-1-sbeller@google.com","subject":"[PATCH 1/4] push options: {pre,post}-receive hook learns about push options","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-14T17:39:17Z","receivedAt":"2016-07-14T17:39:49Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The environment variable GIT_PUSH_OPTION_COUNT is set to the number of\npush options sent, and GIT_PUSH_OPTION_{0,1,..} is set to the transmitted\noption.\n\nThe code is not executed as the push options are set to NULL, nor is the\nnew capability advertised.\n\nThere was some discussion back and forth how to present these push options\nto the user as there are some ways to do it:\n\nKeep all options in one environment variable\n============================================\n+ easiest way to implement in Git\n- This would make things hard to parse correctly in the hook.\n\nPut the options in files instead,\nfilenames are in GIT_PUSH_OPTION_FILES\n======================================\n+ After a discussion about environment variables and shells, we may not\n  want to put user data into an environment variable (see [1] for example).\n+ We could transmit binaries, i.e. we're not bound to C strings as\n  we are when using environment variables to the user.\n+ Maybe easier to parse than constructing environment variable names\n  GIT_PUSH_OPTION_{0,1,..} yourself\n- cleanup of the temporary files is hard to do reliably\n- we have race conditions with multiple clients pushing, hence we'd need\n  to use mkstemp. That's not too bad, but still.\n\nUse environment variables, but restrict to key/value pairs\n==========================================================\n(When the user pushes a push option `foo=bar`, we'd\nGIT_PUSH_OPTION_foo=bar)\n+ very easy to parse for a simple model of push options\n- it's not sufficient for more elaborate models, e.g.\n  it doesn't allow doubles (e.g. cc=reviewer@email)\n\nPresent the options in different environment variables\n======================================================\n(This is implemented)\n* harder to parse as a user, but we have a sample hook for that.\n- doesn't allow binary files\n+ allows the same option twice, i.e. is not restrictive about\n  options, except for binary files.\n+ doesn't clutter a remote directory with (possibly stale)\n  temporary files\n\nAs we first want to focus on getting simple strings to work\nreliably, we go with the last option for now. If we want to\ndo transmission of binaries later, we can just attach a\n'side-channel', e.g. \"any push option that contains a '\\0' is\nput into a file instead of the environment variable and we'd\nhave new GIT_PUSH_OPTION_FILES, GIT_PUSH_OPTION_FILENAME_{0,1,..}\nenvironment variables\".\n\nWe limit the push options for now\n* to not exceed an arbitrary count, and\n* to not exceed an arbitrary size.\n\nThis serves two purposes:\n* DoS protection (i.e. one connection can add no more than 32kB\n  now)\n* We need to figure out how to handle large (>64kB). Jeff wrote:\n  > Yes, but people are also happy when they can use a flexible and\n  > standardized tool to do a thing. I'd be more frustrated when I found out\n  > that Git's data-pushing protocol has arbitrary limitations (like, say, I\n  > can't push a data item larger than a single 64K pkt-line), which would\n  > easily just work with something like HTTP POSTs.\n  So to keep a way open in the future to deal with large pay loads,\n  the size is restricted for now.\n\n[1] 'Shellshock' https://lwn.net/Articles/614218/\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n Documentation/githooks.txt          | 18 ++++++++++++++\n builtin/receive-pack.c              | 49 +++++++++++++++++++++++++++----------\n templates/hooks--pre-receive.sample | 24 ++++++++++++++++++\n 3 files changed, 78 insertions(+), 13 deletions(-)\n create mode 100644 templates/hooks--pre-receive.sample\n\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex d82e912..9565dc3 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -247,6 +247,15 @@ Both standard output and standard error output are forwarded to\n 'git send-pack' on the other end, so you can simply `echo` messages\n for the user.\n \n+The number of push options given on the command line of\n+`git push --push-option=...` can be read from the environment\n+variable `GIT_PUSH_OPTION_COUNT`, and the options themselves are\n+found in `GIT_PUSH_OPTION_0`, `GIT_PUSH_OPTION_1`,...\n+If it is negotiated to not use the push options phase, the\n+environment variables will not be set. If the client selects\n+to use push options, but doesn't transmit any, the count variable\n+will be set to zero, `GIT_PUSH_OPTION_COUNT=0`.\n+\n [[update]]\n update\n ~~~~~~\n@@ -322,6 +331,15 @@ a sample script `post-receive-email` provided in the `contrib/hooks`\n directory in Git distribution, which implements sending commit\n emails.\n \n+The number of push options given on the command line of\n+`git push --push-option=...` can be read from the environment\n+variable `GIT_PUSH_OPTION_COUNT`, and the options themselves are\n+found in `GIT_PUSH_OPTION_0`, `GIT_PUSH_OPTION_1`,...\n+If it is negotiated to not use the push options phase, the\n+environment variables will not be set. If the client selects\n+to use push options, but doesn't transmit any, the count variable\n+will be set to zero, `GIT_PUSH_OPTION_COUNT=0`.\n+\n [[post-update]]\n post-update\n ~~~~~~~~~~~\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 15c323a..236417e 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -550,8 +550,16 @@ static void prepare_push_cert_sha1(struct child_process *proc)\n \t}\n }\n \n+struct receive_hook_feed_state {\n+\tstruct command *cmd;\n+\tint skip_broken;\n+\tstruct strbuf buf;\n+\tconst struct string_list *push_options;\n+};\n+\n typedef int (*feed_fn)(void *, const char **, size_t *);\n-static int run_and_feed_hook(const char *hook_name, feed_fn feed, void *feed_state)\n+static int run_and_feed_hook(const char *hook_name, feed_fn feed,\n+\t\t\t     struct receive_hook_feed_state *feed_state)\n {\n \tstruct child_process proc = CHILD_PROCESS_INIT;\n \tstruct async muxer;\n@@ -567,6 +575,16 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed, void *feed_sta\n \tproc.argv = argv;\n \tproc.in = -1;\n \tproc.stdout_to_stderr = 1;\n+\tif (feed_state->push_options) {\n+\t\tint i;\n+\t\tfor (i = 0; i < feed_state->push_options->nr; i++)\n+\t\t\targv_array_pushf(&proc.env_array,\n+\t\t\t\t\"GIT_PUSH_OPTION_%d=%s\", i,\n+\t\t\t\tfeed_state->push_options->items[i].string);\n+\t\targv_array_pushf(&proc.env_array, \"GIT_PUSH_OPTION_COUNT=%d\",\n+\t\t\t\t feed_state->push_options->nr);\n+\t} else\n+\t\targv_array_pushf(&proc.env_array, \"GIT_PUSH_OPTION_COUNT\");\n \n \tif (use_sideband) {\n \t\tmemset(&muxer, 0, sizeof(muxer));\n@@ -606,12 +624,6 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed, void *feed_sta\n \treturn finish_command(&proc);\n }\n \n-struct receive_hook_feed_state {\n-\tstruct command *cmd;\n-\tint skip_broken;\n-\tstruct strbuf buf;\n-};\n-\n static int feed_receive_hook(void *state_, const char **bufp, size_t *sizep)\n {\n \tstruct receive_hook_feed_state *state = state_;\n@@ -634,8 +646,10 @@ static int feed_receive_hook(void *state_, const char **bufp, size_t *sizep)\n \treturn 0;\n }\n \n-static int run_receive_hook(struct command *commands, const char *hook_name,\n-\t\t\t    int skip_broken)\n+static int run_receive_hook(struct command *commands,\n+\t\t\t    const char *hook_name,\n+\t\t\t    int skip_broken,\n+\t\t\t    const struct string_list *push_options)\n {\n \tstruct receive_hook_feed_state state;\n \tint status;\n@@ -646,6 +660,7 @@ static int run_receive_hook(struct command *commands, const char *hook_name,\n \tif (feed_receive_hook(&state, NULL, NULL))\n \t\treturn 0;\n \tstate.cmd = commands;\n+\tstate.push_options = push_options;\n \tstatus = run_and_feed_hook(hook_name, feed_receive_hook, &state);\n \tstrbuf_release(&state.buf);\n \treturn status;\n@@ -1316,7 +1331,8 @@ cleanup:\n \n static void execute_commands(struct command *commands,\n \t\t\t     const char *unpacker_error,\n-\t\t\t     struct shallow_info *si)\n+\t\t\t     struct shallow_info *si,\n+\t\t\t     const struct string_list *push_options)\n {\n \tstruct command *cmd;\n \tunsigned char sha1[20];\n@@ -1335,7 +1351,7 @@ static void execute_commands(struct command *commands,\n \n \treject_updates_to_hidden(commands);\n \n-\tif (run_receive_hook(commands, \"pre-receive\", 0)) {\n+\tif (run_receive_hook(commands, \"pre-receive\", 0, push_options)) {\n \t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n \t\t\tif (!cmd->error_string)\n \t\t\t\tcmd->error_string = \"pre-receive hook declined\";\n@@ -1756,6 +1772,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \n \tif ((commands = read_head_info(&shallow)) != NULL) {\n \t\tconst char *unpack_status = NULL;\n+\t\tstruct string_list *push_options = NULL;\n \n \t\tprepare_shallow_info(&si, &shallow);\n \t\tif (!si.nr_ours && !si.nr_theirs)\n@@ -1764,13 +1781,19 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t\t\tunpack_status = unpack_with_sideband(&si);\n \t\t\tupdate_shallow_info(commands, &si, &ref);\n \t\t}\n-\t\texecute_commands(commands, unpack_status, &si);\n+\t\texecute_commands(commands, unpack_status, &si,\n+\t\t\t\t push_options);\n \t\tif (pack_lockfile)\n \t\t\tunlink_or_warn(pack_lockfile);\n \t\tif (report_status)\n \t\t\treport(commands, unpack_status);\n-\t\trun_receive_hook(commands, \"post-receive\", 1);\n+\t\trun_receive_hook(commands, \"post-receive\", 1,\n+\t\t\t\t push_options);\n \t\trun_update_post_hook(commands);\n+\t\tif (push_options) {\n+\t\t\tstring_list_clear(push_options, 0);\n+\t\t\tfree(push_options);\n+\t\t}\n \t\tif (auto_gc) {\n \t\t\tconst char *argv_gc_auto[] = {\n \t\t\t\t\"gc\", \"--auto\", \"--quiet\", NULL,\ndiff --git a/templates/hooks--pre-receive.sample b/templates/hooks--pre-receive.sample\nnew file mode 100644\nindex 0000000..a1fd29e\n--- /dev/null\n+++ b/templates/hooks--pre-receive.sample\n@@ -0,0 +1,24 @@\n+#!/bin/sh\n+#\n+# An example hook script to make use of push options.\n+# The example simply echoes all push options that start with 'echoback='\n+# and rejects all pushes when the \"reject\" push option is used.\n+#\n+# To enable this hook, rename this file to \"pre-receive\".\n+\n+if test -n \"$GIT_PUSH_OPTION_COUNT\"\n+then\n+\ti=0\n+\twhile test \"$i\" -lt \"$GIT_PUSH_OPTION_COUNT\"\n+\tdo\n+\t\teval \"value=\\$GIT_PUSH_OPTION_$i\"\n+\t\tcase \"$value\" in\n+\t\techoback=*)\n+\t\t\techo \"echo from the pre-receive-hook: ${value#*=}\" >&2\n+\t\t\t;;\n+\t\treject)\n+\t\t\texit 1\n+\t\tesac\n+\t\ti=$((i + 1))\n+\tdone\n+fi\n-- \n2.9.0.247.gf748855.dirty\n\n"},{"id":"291443","messageId":"20160714173920.22001-3-sbeller@google.com","threadId":"42847","inReplyTo":"20160714173920.22001-1-sbeller@google.com","subject":"[PATCH 2/4] receive-pack: implement advertising and receiving push options","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-14T17:39:18Z","receivedAt":"2016-07-14T17:39:54Z","isPatch":true,"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 Documentation/config.txt                          |  9 +++-\n Documentation/technical/pack-protocol.txt         | 10 ++--\n Documentation/technical/protocol-capabilities.txt |  9 ++++\n builtin/receive-pack.c                            | 59 +++++++++++++++++++++++\n 4 files changed, 81 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex e208af1..25b5db1 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2410,8 +2410,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..917ac18 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,50 @@ 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, *lf;\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\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+\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 +1830,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":"291444","messageId":"20160714173920.22001-4-sbeller@google.com","threadId":"42847","inReplyTo":"20160714173920.22001-1-sbeller@google.com","subject":"[PATCH 3/4] push: accept push options","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-14T17:39:19Z","receivedAt":"2016-07-14T17:39:58Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This implements everything that is required on the client side to make use\nof push options from the porcelain push command.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n Documentation/git-push.txt |  8 +++++++-\n builtin/push.c             | 21 ++++++++++++++++++---\n send-pack.c                | 27 +++++++++++++++++++++++++++\n send-pack.h                |  3 +++\n transport.c                |  1 +\n transport.h                |  7 +++++++\n 6 files changed, 63 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex 19f46b6..e960258 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n [verse]\n 'git push' [--all | --mirror | --tags] [--follow-tags] [--atomic] [-n | --dry-run] [--receive-pack=<git-receive-pack>]\n \t   [--repo=<repository>] [-f | --force] [-d | --delete] [--prune] [-v | --verbose]\n-\t   [-u | --set-upstream]\n+\t   [-u | --set-upstream] [--push-option=<string>]\n \t   [--[no-]signed|--sign=(true|false|if-asked)]\n \t   [--force-with-lease[=<refname>[:<expect>]]]\n \t   [--no-verify] [<repository> [<refspec>...]]\n@@ -156,6 +156,12 @@ already exists on the remote side.\n \tEither all refs are updated, or on error, no refs are updated.\n \tIf the server does not support atomic pushes the push will fail.\n \n+-o::\n+--push-option::\n+\tTransmit the given string to the server, which passes them to\n+\tthe pre-receive as well as the post-receive hook. The given string\n+\tmust not contain a NUL or LF character.\n+\n --receive-pack=<git-receive-pack>::\n --exec=<git-receive-pack>::\n \tPath to the 'git-receive-pack' program on the remote\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 4e9e4db..3bb9d6b 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -353,7 +353,8 @@ static int push_with_options(struct transport *transport, int flags)\n \treturn 1;\n }\n \n-static int do_push(const char *repo, int flags)\n+static int do_push(const char *repo, int flags,\n+\t\t   const struct string_list *push_options)\n {\n \tint i, errs;\n \tstruct remote *remote = pushremote_get(repo);\n@@ -376,6 +377,9 @@ static int do_push(const char *repo, int flags)\n \tif (remote->mirror)\n \t\tflags |= (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE);\n \n+\tif (push_options->nr)\n+\t\tflags |= TRANSPORT_PUSH_OPTIONS;\n+\n \tif ((flags & TRANSPORT_PUSH_ALL) && refspec) {\n \t\tif (!strcmp(*refspec, \"refs/tags/*\"))\n \t\t\treturn error(_(\"--all and --tags are incompatible\"));\n@@ -406,13 +410,16 @@ static int do_push(const char *repo, int flags)\n \t\tfor (i = 0; i < url_nr; i++) {\n \t\t\tstruct transport *transport =\n \t\t\t\ttransport_get(remote, url[i]);\n+\t\t\tif (flags & TRANSPORT_PUSH_OPTIONS)\n+\t\t\t\ttransport->push_options = push_options;\n \t\t\tif (push_with_options(transport, flags))\n \t\t\t\terrs++;\n \t\t}\n \t} else {\n \t\tstruct transport *transport =\n \t\t\ttransport_get(remote, NULL);\n-\n+\t\tif (flags & TRANSPORT_PUSH_OPTIONS)\n+\t\t\ttransport->push_options = push_options;\n \t\tif (push_with_options(transport, flags))\n \t\t\terrs++;\n \t}\n@@ -500,6 +507,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \tint push_cert = -1;\n \tint rc;\n \tconst char *repo = NULL;\t/* default repository */\n+\tstatic struct string_list push_options = STRING_LIST_INIT_DUP;\n+\tstatic struct string_list_item *item;\n+\n \tstruct option options[] = {\n \t\tOPT__VERBOSITY(&verbosity),\n \t\tOPT_STRING( 0 , \"repo\", &repo, N_(\"repository\"), N_(\"repository\")),\n@@ -533,6 +543,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\t  0, \"signed\", &push_cert, \"yes|no|if-asked\", N_(\"GPG sign the push\"),\n \t\t  PARSE_OPT_OPTARG, option_parse_push_signed },\n \t\tOPT_BIT(0, \"atomic\", &flags, N_(\"request atomic transaction on remote side\"), TRANSPORT_PUSH_ATOMIC),\n+\t\tOPT_STRING_LIST('o', \"push-option\", &push_options, N_(\"server-specific\"), N_(\"option to transmit\")),\n \t\tOPT_SET_INT('4', \"ipv4\", &family, N_(\"use IPv4 addresses only\"),\n \t\t\t\tTRANSPORT_FAMILY_IPV4),\n \t\tOPT_SET_INT('6', \"ipv6\", &family, N_(\"use IPv6 addresses only\"),\n@@ -563,7 +574,11 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\tset_refspecs(argv + 1, argc - 1, repo);\n \t}\n \n-\trc = do_push(repo, flags);\n+\tfor_each_string_list_item(item, &push_options)\n+\t\tif (strchr(item->string, '\\n'))\n+\t\t\tdie(_(\"push options must not have new line characters\"));\n+\n+\trc = do_push(repo, flags, &push_options);\n \tif (rc == -1)\n \t\tusage_with_options(push_usage, options);\n \telse\ndiff --git a/send-pack.c b/send-pack.c\nindex 299d303..3a842ac 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -260,6 +260,7 @@ static int generate_push_cert(struct strbuf *req_buf,\n \t\t\t      const char *push_cert_nonce)\n {\n \tconst struct ref *ref;\n+\tstruct string_list_item *item;\n \tchar *signing_key = xstrdup(get_signing_key());\n \tconst char *cp, *np;\n \tstruct strbuf cert = STRBUF_INIT;\n@@ -276,6 +277,9 @@ static int generate_push_cert(struct strbuf *req_buf,\n \t}\n \tif (push_cert_nonce[0])\n \t\tstrbuf_addf(&cert, \"nonce %s\\n\", push_cert_nonce);\n+\tif (args->push_options)\n+\t\tfor_each_string_list_item(item, args->push_options)\n+\t\t\tstrbuf_addf(&cert, \"push-option %s\\n\", item->string);\n \tstrbuf_addstr(&cert, \"\\n\");\n \n \tfor (ref = remote_refs; ref; ref = ref->next) {\n@@ -370,6 +374,8 @@ int send_pack(struct send_pack_args *args,\n \tint agent_supported = 0;\n \tint use_atomic = 0;\n \tint atomic_supported = 0;\n+\tint use_push_options = 0;\n+\tint push_options_supported = 0;\n \tunsigned cmds_sent = 0;\n \tint ret;\n \tstruct async demux;\n@@ -392,6 +398,8 @@ int send_pack(struct send_pack_args *args,\n \t\targs->use_thin_pack = 0;\n \tif (server_supports(\"atomic\"))\n \t\tatomic_supported = 1;\n+\tif (server_supports(\"push-options\"))\n+\t\tpush_options_supported = 1;\n \n \tif (args->push_cert != SEND_PACK_PUSH_CERT_NEVER) {\n \t\tint len;\n@@ -418,6 +426,11 @@ int send_pack(struct send_pack_args *args,\n \n \tuse_atomic = atomic_supported && args->atomic;\n \n+\tif (args->push_options && !push_options_supported)\n+\t\tdie(_(\"the receiving end does not support push options\"));\n+\n+\tuse_push_options = push_options_supported && args->push_options;\n+\n \tif (status_report)\n \t\tstrbuf_addstr(&cap_buf, \" report-status\");\n \tif (use_sideband)\n@@ -426,6 +439,8 @@ int send_pack(struct send_pack_args *args,\n \t\tstrbuf_addstr(&cap_buf, \" quiet\");\n \tif (use_atomic)\n \t\tstrbuf_addstr(&cap_buf, \" atomic\");\n+\tif (use_push_options)\n+\t\tstrbuf_addstr(&cap_buf, \" push-options\");\n \tif (agent_supported)\n \t\tstrbuf_addf(&cap_buf, \" agent=%s\", git_user_agent_sanitized());\n \n@@ -512,6 +527,18 @@ int send_pack(struct send_pack_args *args,\n \tstrbuf_release(&req_buf);\n \tstrbuf_release(&cap_buf);\n \n+\tif (use_push_options) {\n+\t\tstruct string_list_item *item;\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\n+\t\tfor_each_string_list_item(item, args->push_options)\n+\t\t\tpacket_buf_write(&sb, \"%s\", item->string);\n+\n+\t\twrite_or_die(out, sb.buf, sb.len);\n+\t\tpacket_flush(out);\n+\t\tstrbuf_release(&sb);\n+\t}\n+\n \tif (use_sideband && cmds_sent) {\n \t\tmemset(&demux, 0, sizeof(demux));\n \t\tdemux.proc = sideband_demux;\ndiff --git a/send-pack.h b/send-pack.h\nindex 57f222a..67fc40f 100644\n--- a/send-pack.h\n+++ b/send-pack.h\n@@ -1,6 +1,8 @@\n #ifndef SEND_PACK_H\n #define SEND_PACK_H\n \n+#include \"string-list.h\"\n+\n /* Possible values for push_cert field in send_pack_args. */\n #define SEND_PACK_PUSH_CERT_NEVER 0\n #define SEND_PACK_PUSH_CERT_IF_ASKED 1\n@@ -21,6 +23,7 @@ struct send_pack_args {\n \t\tpush_cert:2,\n \t\tstateless_rpc:1,\n \t\tatomic:1;\n+\tconst struct string_list *push_options;\n };\n \n struct option;\ndiff --git a/transport.c b/transport.c\nindex 095e61f..0298be1 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -510,6 +510,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n \targs.dry_run = !!(flags & TRANSPORT_PUSH_DRY_RUN);\n \targs.porcelain = !!(flags & TRANSPORT_PUSH_PORCELAIN);\n \targs.atomic = !!(flags & TRANSPORT_PUSH_ATOMIC);\n+\targs.push_options = transport->push_options;\n \targs.url = transport->url;\n \n \tif (flags & TRANSPORT_PUSH_CERT_ALWAYS)\ndiff --git a/transport.h b/transport.h\nindex c681408..6fe3485 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -48,6 +48,12 @@ struct transport {\n \t */\n \tunsigned cloning : 1;\n \n+\t/*\n+\t * These strings will be passed to the {pre, post}-receive hook,\n+\t * on the remote side, if both sides support the push options capability.\n+\t */\n+\tconst struct string_list *push_options;\n+\n \t/**\n \t * Returns 0 if successful, positive if the option is not\n \t * recognized or is inapplicable, and negative if the option\n@@ -134,6 +140,7 @@ struct transport {\n #define TRANSPORT_PUSH_CERT_ALWAYS 2048\n #define TRANSPORT_PUSH_CERT_IF_ASKED 4096\n #define TRANSPORT_PUSH_ATOMIC 8192\n+#define TRANSPORT_PUSH_OPTIONS 16384\n \n #define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)\n #define TRANSPORT_SUMMARY(x) (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)\n-- \n2.9.0.247.gf748855.dirty\n\n"},{"id":"291445","messageId":"20160714173920.22001-5-sbeller@google.com","threadId":"42847","inReplyTo":"20160714173920.22001-1-sbeller@google.com","subject":"[PATCH 4/4] add a test for push options","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-14T17:39:20Z","receivedAt":"2016-07-14T17:39:59Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The functions `mk_repo_pair` as well as `test_refs` are borrowed from\nt5543-atomic-push, with additional hooks installed.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n t/t5545-push-options.sh | 103 ++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 103 insertions(+)\n create mode 100755 t/t5545-push-options.sh\n\ndiff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh\nnew file mode 100755\nindex 0000000..ea813b9\n--- /dev/null\n+++ b/t/t5545-push-options.sh\n@@ -0,0 +1,103 @@\n+#!/bin/sh\n+\n+test_description='pushing to a repository using push options'\n+\n+. ./test-lib.sh\n+\n+mk_repo_pair () {\n+\trm -rf workbench upstream &&\n+\ttest_create_repo upstream &&\n+\ttest_create_repo workbench &&\n+\t(\n+\t\tcd upstream &&\n+\t\tgit config receive.denyCurrentBranch warn &&\n+\t\tmkdir -p .git/hooks &&\n+\t\tcat >.git/hooks/pre-receive <<-'EOF' &&\n+\t\t#!/bin/sh\n+\t\tif test -n \"$GIT_PUSH_OPTION_COUNT\"; then\n+\t\t\ti=0\n+\t\t\t>hooks/pre-receive.push_options\n+\t\t\twhile test \"$i\" -lt \"$GIT_PUSH_OPTION_COUNT\"; do\n+\t\t\t\teval \"value=\\$GIT_PUSH_OPTION_$i\"\n+\t\t\t\techo $value >>hooks/pre-receive.push_options\n+\t\t\t\ti=$((i + 1))\n+\t\t\tdone\n+\t\tfi\n+\t\tEOF\n+\t\tchmod u+x .git/hooks/pre-receive\n+\n+\t\tcat >.git/hooks/post-receive <<-'EOF' &&\n+\t\t#!/bin/sh\n+\t\tif test -n \"$GIT_PUSH_OPTION_COUNT\"; then\n+\t\t\ti=0\n+\t\t\t>hooks/post-receive.push_options\n+\t\t\twhile test \"$i\" -lt \"$GIT_PUSH_OPTION_COUNT\"; do\n+\t\t\t\teval \"value=\\$GIT_PUSH_OPTION_$i\"\n+\t\t\t\techo $value >>hooks/post-receive.push_options\n+\t\t\t\ti=$((i + 1))\n+\t\t\tdone\n+\t\tfi\n+\t\tEOF\n+\t\tchmod u+x .git/hooks/post-receive\n+\t) &&\n+\t(\n+\t\tcd workbench &&\n+\t\tgit remote add up ../upstream\n+\t)\n+}\n+\n+# Compare the ref ($1) in upstream with a ref value from workbench ($2)\n+# i.e. test_refs second HEAD@{2}\n+test_refs () {\n+\ttest $# = 2 &&\n+\tgit -C upstream rev-parse --verify \"$1\" >expect &&\n+\tgit -C workbench rev-parse --verify \"$2\" >actual &&\n+\ttest_cmp expect actual\n+}\n+\n+test_expect_success 'one push option works for a single branch' '\n+\tmk_repo_pair &&\n+\tgit -C upstream config receive.advertisePushOptions true &&\n+\t(\n+\t\tcd workbench &&\n+\t\ttest_commit one &&\n+\t\tgit push --mirror up &&\n+\t\ttest_commit two &&\n+\t\tgit push --push-option=asdf up master\n+\t) &&\n+\ttest_refs master master &&\n+\techo \"asdf\" >expect &&\n+\ttest_cmp expect upstream/.git/hooks/pre-receive.push_options &&\n+\ttest_cmp expect upstream/.git/hooks/post-receive.push_options\n+'\n+\n+test_expect_success 'push option denied by remote' '\n+\tmk_repo_pair &&\n+\tgit -C upstream config receive.advertisePushOptions false &&\n+\t(\n+\t\tcd workbench &&\n+\t\ttest_commit one &&\n+\t\tgit push --mirror up &&\n+\t\ttest_commit two &&\n+\t\ttest_must_fail git push --push-option=asdf up master\n+\t) &&\n+\ttest_refs master HEAD@{1}\n+'\n+\n+test_expect_success 'two push options work' '\n+\tmk_repo_pair &&\n+\tgit -C upstream config receive.advertisePushOptions true &&\n+\t(\n+\t\tcd workbench &&\n+\t\ttest_commit one &&\n+\t\tgit push --mirror up &&\n+\t\ttest_commit two &&\n+\t\tgit push --push-option=asdf --push-option=\"more structured text\" up master\n+\t) &&\n+\ttest_refs master master &&\n+\tprintf \"asdf\\nmore structured text\\n\" >expect &&\n+\ttest_cmp expect upstream/.git/hooks/pre-receive.push_options &&\n+\ttest_cmp expect upstream/.git/hooks/post-receive.push_options\n+'\n+\n+test_done\n-- \n2.9.0.247.gf748855.dirty\n\n"},{"id":"291452","messageId":"xmqqk2goqd4l.fsf@gitster.mtv.corp.google.com","threadId":"42847","inReplyTo":"20160714173920.22001-3-sbeller@google.com","subject":"Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-14T18:38:02Z","receivedAt":"2016-07-14T18:38:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\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, *lf;\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\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\npacket_read_line() calls packet_read_line_generic() that calls\npacket_read() with the fixed packet_buffer[] that is sized to be\nLARGE_PACKET_MAX.\n\nCan this check even trigger?\n\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\npacket_read_line() -> packet_read_line_generic() calls packet_read()\nwith PACKET_READ_CHOMP_NEWLINE flag bit; do we need this check?\n\n> +\n> +\t\tstring_list_append(ret, line);\n> +\t}\n> +\n> +\treturn ret;\n> +}\n\nOther than that, looks good to me.\n\nThanks.\n"},{"id":"291454","messageId":"20160714184146.GE16497@sigill.intra.peff.net","threadId":"42847","inReplyTo":"20160714173920.22001-1-sbeller@google.com","subject":"Re: [PATCHv5 0/4] Push options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-14T18:41:46Z","receivedAt":"2016-07-14T18:41:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 14, 2016 at 10:39:16AM -0700, Stefan Beller wrote:\n\n> Jeff wrote:\n> > Junio wrote:\n> >> I think those extra knobs can come later.  If we are not going to\n> >> limit with max_options in the end, however, wouldn't it be more\n> >> natural for the initial iteration without any configuration not to\n> >> have hard-coded max_options at all?\n> >\n> > Yeah, I am OK with adding restrictive knobs later as a separate topic.\n> > As Stefan notes, upstream does not have the other knobs anyway, and IIRC\n> > the push-options feature is not even enabled by default.\n> \n> * now it actually is not a default. ;)\n\nHmm. So that is a downside for people who have implemented separate DoS\nprotection only in that upgrading git will open a new \"hole\" that they\nneed to plug. But that is their problem, not upstream git's.\n\n-Peff\n"},{"id":"291456","messageId":"CAGZ79kaj2vfvS3RXCWqkZNdF7UkjAuv_J00kENPjz7SNTUV7BQ@mail.gmail.com","threadId":"42847","inReplyTo":"20160714184146.GE16497@sigill.intra.peff.net","subject":"Re: [PATCHv5 0/4] Push options","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-14T18:48:58Z","receivedAt":"2016-07-14T18:49:04Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jul 14, 2016 at 11:41 AM, Jeff King <peff@peff.net> wrote:\n> On Thu, Jul 14, 2016 at 10:39:16AM -0700, Stefan Beller wrote:\n>\n>> Jeff wrote:\n>> > Junio wrote:\n>> >> I think those extra knobs can come later.  If we are not going to\n>> >> limit with max_options in the end, however, wouldn't it be more\n>> >> natural for the initial iteration without any configuration not to\n>> >> have hard-coded max_options at all?\n>> >\n>> > Yeah, I am OK with adding restrictive knobs later as a separate topic.\n>> > As Stefan notes, upstream does not have the other knobs anyway, and IIRC\n>> > the push-options feature is not even enabled by default.\n>>\n>> * now it actually is not a default. ;)\n>\n> Hmm. So that is a downside for people who have implemented separate DoS\n> protection only in that upgrading git will open a new \"hole\" that they\n> need to plug. But that is their problem, not upstream git's.\n>\n> -Peff\n\nBut this is not specific to DoS protection, but like most features.\nJust look at the vendors\nusing linux that think carrying patches out of tree for their special\nsnowflake hardware\nis cheaper than getting it upstream.... (This is not to be read as a\nsnarky comment, but\nrather as a pointer to the similarity of the mechanisms involved).\nAlthough I cannot tell\noffhand another feature that would easily break downstream customization.\n\nStefan\n"},{"id":"291459","messageId":"20160714185037.GG16497@sigill.intra.peff.net","threadId":"42847","inReplyTo":"CAGZ79kaj2vfvS3RXCWqkZNdF7UkjAuv_J00kENPjz7SNTUV7BQ@mail.gmail.com","subject":"Re: [PATCHv5 0/4] Push options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-14T18:50:37Z","receivedAt":"2016-07-14T18:50:43Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 14, 2016 at 11:48:58AM -0700, Stefan Beller wrote:\n\n> > Hmm. So that is a downside for people who have implemented separate\n> > DoS protection only in that upgrading git will open a new \"hole\"\n> > that they need to plug. But that is their problem, not upstream\n> > git's.\n> >\n> > -Peff\n> \n> But this is not specific to DoS protection, but like most features.\n> Just look at the vendors using linux that think carrying patches out\n> of tree for their special snowflake hardware is cheaper than getting\n> it upstream.... (This is not to be read as a snarky comment, but\n> rather as a pointer to the similarity of the mechanisms involved).\n> Although I cannot tell offhand another feature that would easily break\n> downstream customization.\n\nRight. I was serious when I said \"their problem, not git's\".\n\nMost features are a little better off in that they are not a possible\nnegative for somebody downstream to receive them. But again, I don't\nthink that needs to restrict what git releases.\n\n-Peff\n"},{"id":"291460","messageId":"CAGZ79kZgJ_6fxZ19Hx6zd6KoTAd_+oMjb4fj1twY2ZGUHP7wHA@mail.gmail.com","threadId":"42847","inReplyTo":"xmqqk2goqd4l.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-14T19:00:48Z","receivedAt":"2016-07-14T19:00:53Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> packet_read_line() calls packet_read_line_generic() that calls\n> packet_read() with the fixed packet_buffer[] that is sized to be\n> LARGE_PACKET_MAX.\n\n>\n> Can this check even trigger?\n\nI thought when len == LARGE_PACKET_MAX,\nthis could trigger. Though on inspection of packet_read,\nwe already reject packets that have size LARGE_PACKET_MAX,\nand the largest size allowed is LARGE_PACKET_MAX - 1.\n\nI guess we can remove it altogether and still be future proof.\nIf we ever want to allow larger push options we either need to\nhave larger (variable sized) packets or a capability push-options-v2,\nso I'll rip this check out.\n\n>\n>> +             lf = strchr(line, '\\n');\n>> +             if (lf)\n>> +                     *lf = '\\0';\n>\n> packet_read_line() -> packet_read_line_generic() calls packet_read()\n> with PACKET_READ_CHOMP_NEWLINE flag bit; do we need this check?\n\nThis check was not about \"option with lf at end\\n\", but rather we want to chop\noff \"option\\nover\\nmultiple\\nlines\" ?\n\nAlthough as you remarked in another email, this would not pose a problem for\nthe shell variable, so we could also drop it to allow multi line\noptions. will do.\n\n>\n>> +\n>> +             string_list_append(ret, line);\n>> +     }\n>> +\n>> +     return ret;\n>> +}\n>\n> Other than that, looks good to me.\n>\n> Thanks.\n"},{"id":"291461","messageId":"xmqqfurcqbrw.fsf@gitster.mtv.corp.google.com","threadId":"42847","inReplyTo":"CAGZ79kZgJ_6fxZ19Hx6zd6KoTAd_+oMjb4fj1twY2ZGUHP7wHA@mail.gmail.com","subject":"Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-14T19:07:15Z","receivedAt":"2016-07-14T19:07:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>>> +             lf = strchr(line, '\\n');\n>>> +             if (lf)\n>>> +                     *lf = '\\0';\n>>\n>> packet_read_line() -> packet_read_line_generic() calls packet_read()\n>> with PACKET_READ_CHOMP_NEWLINE flag bit; do we need this check?\n>\n> This check was not about \"option with lf at end\\n\", but rather we want to chop\n> off \"option\\nover\\nmultiple\\nlines\" ?\n\nAhh, I did misread the check.\n\n> Although as you remarked in another email, this would not pose a problem for\n> the shell variable, so we could also drop it to allow multi line\n> options. will do.\n\nOne thing to note is that I do not think there is a guarantee that\npacket_buf[] is NUL-terminated, and when len == LAGE_PACKET_MAX, you\ndo not have room to NUL-terminate it yourself.\n\nstring_list_append(ret, line) that assumes the \"string\" is NUL\nterminated may become an issue that you need to solve by appending\nthe result of xmemdupz() into a non-duping string list.\n\n\n"},{"id":"291466","messageId":"20160714194522.GA17798@sigill.intra.peff.net","threadId":"42847","inReplyTo":"xmqqfurcqbrw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-14T19:45:22Z","receivedAt":"2016-07-14T19:45:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 14, 2016 at 12:07:15PM -0700, Junio C Hamano wrote:\n\n> > Although as you remarked in another email, this would not pose a problem for\n> > the shell variable, so we could also drop it to allow multi line\n> > options. will do.\n> \n> One thing to note is that I do not think there is a guarantee that\n> packet_buf[] is NUL-terminated, and when len == LAGE_PACKET_MAX, you\n> do not have room to NUL-terminate it yourself.\n\npacket_read() does NUL-terminate for you. It gets the extra bytes\nbecause it doesn't store the 4-byte size in the output (whereas the\nclient does not ever send anything over LARGE_PACKET_MAX, _including_\nthose bytes, so we always have room to store its result in our\nLARGE_PACKET_MAX buffer, plus the NUL, with 3 bytes to spare).\n\n-Peff\n"},{"id":"291469","messageId":"xmqqk2gooueg.fsf@gitster.mtv.corp.google.com","threadId":"42847","inReplyTo":"20160714194522.GA17798@sigill.intra.peff.net","subject":"Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-14T20:07:51Z","receivedAt":"2016-07-14T20:07:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> packet_read() does NUL-terminate for you. It gets the extra bytes\n> because it doesn't store the 4-byte size in the output (whereas the\n> client does not ever send anything over LARGE_PACKET_MAX, _including_\n> those bytes, so we always have room to store its result in our\n> LARGE_PACKET_MAX buffer, plus the NUL, with 3 bytes to spare).\n\nGood; then the loop will further be simplified ;-)\n\nThanks.\n"}]}