{"thread":{"id":"42789","subject":"[PATCHv3 0/4] Push options in C Git","startedAt":"2016-07-07T01:12:44Z","lastAt":"2016-07-11T18:42:50Z","messageCount":31,"participants":["Stefan Beller","Junio C Hamano","Jeff King","Jonathan Nieder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"290921","messageId":"20160707011218.3690-1-sbeller@google.com","threadId":"42789","inReplyTo":null,"subject":"[PATCHv3 0/4] Push options in C Git","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-07T01:12:14Z","receivedAt":"2016-07-07T01:12:44Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This 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":"290922","messageId":"20160707011218.3690-3-sbeller@google.com","threadId":"42789","inReplyTo":"20160707011218.3690-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-07T01:12:16Z","receivedAt":"2016-07-07T01:12:48Z","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 \n---\n Documentation/config.txt | 7 ++-\n Documentation/technical/pack-protocol.txt | 10 ++--\n Documentation/technical/protocol-capabilities.txt | 8 +++\n builtin/receive-pack.c | 59 +++++++++++++++++++++++\n 4 files changed, 79 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex e208af1..df1b314 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2410,7 +2410,12 @@ 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+\tcapability to its clients. If you don't want this capability\n+\tto be advertised, 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 \n receive.autogc::\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..b71eda9 100644\n--- a/Documentation/technical/protocol-capabilities.txt\n+++ b/Documentation/technical/protocol-capabilities.txt\n@@ -253,6 +253,14 @@ 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 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+\n allow-tip-sha1-in-want\n ----------------------\n \ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex edbf81e..e71041a 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 = 1;\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@@ -207,6 +214,8 @@ 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@@ -1454,6 +1463,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@@ -1486,6 +1498,50 @@ static struct command *read_head_info(struct sha1_array *shallow)\n \treturn commands;\n }\n \n+static struct string_list *read_push_options()\n+{\n+\tint i;\n+\tstruct string_list *ret = xmalloc(sizeof(*ret));\n+\tstring_list_init(ret, 1);\n+\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+\tfor (i = 0; i < max_options; i++) {\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\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+\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+\t\t \"to %d push options\", max_options);\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@@ -1773,6 +1829,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.141.gd59d3e9.dirty\n\n"},{"id":"290923","messageId":"20160707011218.3690-4-sbeller@google.com","threadId":"42789","inReplyTo":"20160707011218.3690-1-sbeller@google.com","subject":"[PATCH 3/4] push: accept push options","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-07T01:12:17Z","receivedAt":"2016-07-07T01:12:53Z","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 \n---\n Documentation/git-push.txt | 8 +++++++-\n builtin/push.c | 16 +++++++++++++---\n send-pack.c | 30 ++++++++++++++++++++++++++++++\n send-pack.h | 3 +++\n transport.c | 1 +\n transport.h | 7 +++++++\n 6 files changed, 61 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex 19f46b6..b0b1273 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=]\n \t [--repo=] [-f | --force] [-d | --delete] [--prune] [-v | --verbose]\n-\t [-u | --set-upstream]\n+\t [-u | --set-upstream] [--push-option=]\n \t [--[no-]signed|--sign=(true|false|if-asked)]\n \t [--force-with-lease[=[:]]]\n \t [--no-verify] [ [...]]\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+-L::\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+\n --receive-pack=::\n --exec=::\n \tPath to the 'git-receive-pack' program on the remote\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 4e9e4db..1b5d205 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,8 @@ 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+\n \tstruct option options[] = {\n \t\tOPT__VERBOSITY(&verbosity),\n \t\tOPT_STRING( 0 , \"repo\", &repo, N_(\"repository\"), N_(\"repository\")),\n@@ -533,6 +542,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 +573,7 @@ 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+\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..c943560 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@@ -278,6 +279,12 @@ static int generate_push_cert(struct strbuf *req_buf,\n \t\tstrbuf_addf(&cert, \"nonce %s\\n\", push_cert_nonce);\n \tstrbuf_addstr(&cert, \"\\n\");\n \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+\n \tfor (ref = remote_refs; ref; ref = ref->next) {\n \t\tif (check_to_send_update(ref, args) < 0)\n \t\t\tcontinue;\n@@ -370,6 +377,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 +401,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 +429,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 +442,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 +530,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.141.gd59d3e9.dirty\n\n"},{"id":"290924","messageId":"20160707011218.3690-2-sbeller@google.com","threadId":"42789","inReplyTo":"20160707011218.3690-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-07T01:12:15Z","receivedAt":"2016-07-07T01:12:56Z","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 \n---\n Documentation/githooks.txt | 6 +++++\n builtin/receive-pack.c | 48 +++++++++++++++++++++++++++----------\n templates/hooks--pre-receive.sample | 22 +++++++++++++++++\n 3 files changed, 63 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..c875cde 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -247,6 +247,9 @@ 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+\n [[update]]\n update\n ~~~~~~\n@@ -322,6 +325,9 @@ 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+\n [[post-update]]\n post-update\n ~~~~~~~~~~~\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 15c323a..edbf81e 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,15 @@ 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 && 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}\n \n \tif (use_sideband) {\n \t\tmemset(&muxer, 0, sizeof(muxer));\n@@ -606,12 +623,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 +645,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 +659,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 +1330,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 +1350,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 +1771,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 +1780,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..e4d3edc\n--- /dev/null\n+++ b/templates/hooks--pre-receive.sample\n@@ -0,0 +1,22 @@\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\"; then\n+\ti=0\n+\twhile test \"$i\" -lt \"$GIT_PUSH_OPTION_COUNT\"; do\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.141.gd59d3e9.dirty\n\n"},{"id":"290925","messageId":"20160707011218.3690-5-sbeller@google.com","threadId":"42789","inReplyTo":"20160707011218.3690-1-sbeller@google.com","subject":"[PATCH 4/4] add a test for push options","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-07T01:12:18Z","receivedAt":"2016-07-07T01:12: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 \n---\n t/t5544-push-options.sh | 101 ++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 101 insertions(+)\n create mode 100755 t/t5544-push-options.sh\n\ndiff --git a/t/t5544-push-options.sh b/t/t5544-push-options.sh\nnew file mode 100755\nindex 0000000..8dd3c8e\n--- /dev/null\n+++ b/t/t5544-push-options.sh\n@@ -0,0 +1,101 @@\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+\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+\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.141.gd59d3e9.dirty\n\n"},{"id":"290974","messageId":"xmqqinwhp6pv.fsf@gitster.mtv.corp.google.com","threadId":"42789","inReplyTo":"20160707011218.3690-5-sbeller@google.com","subject":"Re: [PATCH 4/4] add a test for push options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-07T19:51:24Z","receivedAt":"2016-07-07T19:51:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> The functions `mk_repo_pair` as well as `test_refs` are borrowed from\n> t5543-atomic-push, with additional hooks installed.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> t/t5544-push-options.sh | 101 ++++++++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 101 insertions(+)\n> create mode 100755 t/t5544-push-options.sh\n\nFYI:\n\n Applying: add a test for push options\n Test number t5544 already taken\n\n"},{"id":"290975","messageId":"CAPc5daUnUtUK_9caymCkhkdH7DQSEY_UhuyraJmeo9y4Mo86yQ@mail.gmail.com","threadId":"42789","inReplyTo":"xmqqinwhp6pv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/4] add a test for push options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-07T20:01:03Z","receivedAt":"2016-07-07T20:01:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Thu, Jul 7, 2016 at 12:51 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> The functions `mk_repo_pair` as well as `test_refs` are borrowed from\n>> t5543-atomic-push, with additional hooks installed.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> t/t5544-push-options.sh | 101 ++++++++++++++++++++++++++++++++++++++++++++++++\n>> 1 file changed, 101 insertions(+)\n>> create mode 100755 t/t5544-push-options.sh\n>\n> FYI:\n>\n> Applying: add a test for push options\n> Test number t5544 already taken\n>\n\nI'll just move it over to t5545; no need to resend.\n"},{"id":"290979","messageId":"xmqqeg75p5cn.fsf@gitster.mtv.corp.google.com","threadId":"42789","inReplyTo":"20160707011218.3690-2-sbeller@google.com","subject":"Re: [PATCH 1/4] push options: {pre,post}-receive hook learns about push options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-07T20:20:56Z","receivedAt":"2016-07-07T20:21:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> As we first want to focus on getting simple strings to work\n> reliably, we go with the last option for now.\n\nOK.\n\n> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\n> index d82e912..c875cde 100644\n> --- a/Documentation/githooks.txt\n> +++ b/Documentation/githooks.txt\n> @@ -247,6 +247,9 @@ 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> +\n> [[update]]\n> update\n> ~~~~~~\n> @@ -322,6 +325,9 @@ 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> +\n\nThe mention of \"push options\" in these two entries sounded a bit too\nabrupt, at least to me. Perhaps add a short phrase before the\nsentence, e.g.\n\n When 'git push --push-option=...' is used, the number of push\n options are avaiable ...\n\nor\n\n The number of push options given on the command line of \"git\n 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\nWe can read any of the above variants in two ways to describe what\nshould happen when \"git push\" is run without \"--push-option=...\".\nIs GIT_PUSH_OPTION_COUNT unset, or set to 0? Either way, it may be\nbetter to be a bit more explicit.\n\nThe hook driver code does not explicitly clear these environment\nvariables; it is safe to assume that these won't seep in from the\nenvironment, I think, but I am not sure.\n\n> @@ -1756,6 +1771,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 +1780,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,\n> diff --git a/templates/hooks--pre-receive.sample b/templates/hooks--pre-receive.sample\n> new file mode 100644\n> index 0000000..e4d3edc\n> --- /dev/null\n> +++ b/templates/hooks--pre-receive.sample\n> @@ -0,0 +1,22 @@\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\"; then\n> +\ti=0\n> +\twhile test \"$i\" -lt \"$GIT_PUSH_OPTION_COUNT\"; do\n\n\nStyle:\n\n if test -n \"$GIT_PUSH_OPTION_COUNT\"\n then\n\t\t...\n\n\twhile test ...\n do\n\t\t...\n\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\nWhat is suboptimal about the structure of the series is that we\nwon't bisect down to any of the potential bugs in the above code\neven if we ever see any bug in the future. It also does not hint\nwhere push_options is expected to be read in the code in the\nsubsequent patches in the series. If I were doing this series, I\nwould probably have done 2/4 first without plumbing it through\n(i.e. it is sent and accumulated in a string list at the receiver,\nand then cleared and freed without being used), and then added the\nprocessing (i.e. this step) as the second patch.\n"},{"id":"290981","messageId":"xmqqa8htp4kc.fsf@gitster.mtv.corp.google.com","threadId":"42789","inReplyTo":"20160707011218.3690-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-07T20:37:55Z","receivedAt":"2016-07-07T20:38:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> While documenting\n> this, fix a nit in the `receive.advertiseAtomic` wording.\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> +\tcapability to its clients. If you don't want this capability\n> +\tto be advertised, 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\nI think we correcting the nit by avoiding passive voice, i.e.\n\n\tIf you don't want to advertise this capability, set this\n\tvariable to false.\n\nwould make it easier to read.\n\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\nOK.\n\n> +push-options\n> +------------\n> +\n> +If the server sends the 'push-options' capability it is capable to accept\n\nTwo nits:\n\n - A comma would make it easier to read.\n - \"capable\" goes with \"of \", while \"able\" goes with \"to \".\n\ni.e. \"... capability, it is capable of accepting...\"\n\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\nMissing dashes, i.e. \"pre- and post-receive hooks\"?\n\n> @@ -207,6 +214,8 @@ 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\nHmph, was there a good reason to add it in the middle (contrast to\nthe previous addition to the \"only possible values are...\"\nenumeration)?\n\n> +static struct string_list *read_push_options()\n\nstatic struct string_list *read_push_options(void)\n\n> +{\n> +\tint i;\n> +\tstruct string_list *ret = xmalloc(sizeof(*ret));\n> +\tstring_list_init(ret, 1);\n> +\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\nGood NEEDSWORK comments; perhaps also hint that the configuration\nmust not come from the repository level configuration file (i.e.\nPeff's \"scoped configuration\" from jk/upload-pack-hook topic)?\n\n> +\tfor (i = 0; i < max_options; i++) {\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\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> +\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> +\t\t \"to %d push options\", max_options);\n\nWhen not going over ssh://, does the user sees these messages?\n\nMore importantly, if we plan to make this configurable and not make\nthe limit a hardwired constant of the wire protocol, it may be\nbetter to advertise push-options capability with the limit, e.g.\n\"push-options=32\" (or even \"push-options=1024/32\"), so that the\nclient side can count and abort early?\n\nI wondered how well the extra flush works with the extra framing\nsmart-http does to wrap the wire protocol; as I do not see any\nchange to the http side, I'd assume that there is no issue.\n\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> @@ -1773,6 +1829,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\n"},{"id":"290986","messageId":"xmqq60shp3w9.fsf@gitster.mtv.corp.google.com","threadId":"42789","inReplyTo":"20160707011218.3690-4-sbeller@google.com","subject":"Re: [PATCH 3/4] push: accept push options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-07T20:52:22Z","receivedAt":"2016-07-07T20:52:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> +-L::\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\nThis is to affect what happens at the remote end, so I would have\nunderstood \"-R\". I also would have understood \"-P\" as a short-hand\nfor \"--push-option\". What is the justification of \"-L\"?\n\nWhat does \"C strings\" mean? Did you mean to say \"A sequence of\nbytes excluding NUL is passed verbatim\"?\n\nI do not think I saw anything in the code I reviewed so far that\nrequires \"no LF\" limitation.\n\n... Ahh, OK, you want to make sure that push-options are\none-per-line in the push certificate. While I do not think it is\nabsolutely necessary, starting with a possibly tighter than\nnecessary limitation is much better than starting loose and having\nto tighten it later.\n\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\nThe result would be easier to read without the removal of a blank\nthat separates decl/defn and stmt here.\n\n> @@ -533,6 +542,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\nHere it seems to expect \"-o\". If we really want a short option,\n\"-o\" would probably be OK, as I do not think \"git push\" wants to\nhave \"send the output to this file\" option.\n\n> diff --git a/send-pack.c b/send-pack.c\n> index 299d303..c943560 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> @@ -278,6 +279,12 @@ static int generate_push_cert(struct strbuf *req_buf,\n> \t\tstrbuf_addf(&cert, \"nonce %s\\n\", push_cert_nonce);\n> \tstrbuf_addstr(&cert, \"\\n\");\n> \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\nWhy the extra blank?\n\nI would actually have expected to see\n\n\tcertificate version ...\n pusher ...\n \n\tpushee ...\t# optional\n nonce ...\t# optional\n push-option ... # optional\n push-option ... # optional\n\n \n ...\n\nby adding this between the two lines in the pre-context of this\nhunk, i.e.\n\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"},{"id":"290991","messageId":"CAGZ79kbkv5P0wP2kKt9gzmZBe1DjLSB8JpZD66DT_Xd4NKqmKQ@mail.gmail.com","threadId":"42789","inReplyTo":"xmqqa8htp4kc.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-07T21:41:37Z","receivedAt":"2016-07-07T21:41:43Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jul 7, 2016 at 1:37 PM, Junio C Hamano wrote:\n>\n>> @@ -207,6 +214,8 @@ static void show_ref(const char *path, const unsigned char *sha1)\n>> \"report-status delete-refs side-band-64k quiet\");\n>> if (advertise_atomic_push)\n>> strbuf_addstr(&cap, \" atomic\");\n>> + if (advertise_push_options)\n>> + strbuf_addstr(&cap, \" push-options\");\n>> if (prefer_ofs_delta)\n>> strbuf_addstr(&cap, \" ofs-delta\");\n>> if (push_cert_nonce)\n>\n> Hmph, was there a good reason to add it in the middle (contrast to\n> the previous addition to the \"only possible values are...\"\n> enumeration)?\n\nNo, there is no good objective reason. I added it just after the atomic\nflag as that is what I implemented.\n\nIs there a reason for a particular order of capabilities? I always considered\nit a set of strings, i.e. any order is valid and there is no preference in\nwhich way to put it.\n\n>\n>> +static struct string_list *read_push_options()\n>\n> static struct string_list *read_push_options(void)\n>\n>> +{\n>> + int i;\n>> + struct string_list *ret = xmalloc(sizeof(*ret));\n>> + string_list_init(ret, 1);\n>> +\n>> + /* NEEDSWORK: expose the limitations to be configurable. */\n>> + int max_options = 32;\n>> +\n>> + /*\n>> + * NEEDSWORK: expose the limitations to be configurable;\n>> + * Once the limit can be lifted, include a way for payloads\n>> + * larger than one pkt, e.g allow a payload of up to\n>> + * LARGE_PACKET_MAX - 1 only, and reserve the last byte\n>> + * to indicate whether the next pkt continues with this\n>> + * push option.\n>> + */\n>> + int max_size = 1024;\n>\n> Good NEEDSWORK comments; perhaps also hint that the configuration\n> must not come from the repository level configuration file (i.e.\n> Peff's \"scoped configuration\" from jk/upload-pack-hook topic)?\n\nOk, I reviewed that series. It is unclear to me how the attack would\nactually look like in that case.\n\nIn 20b20a22f8f Jeff writes:\n> Because we promise that\n> upload-pack is safe to run in an untrusted repository, we\n> cannot execute arbitrary code or commands found in the\n> repository (neither in hooks/, nor in the config).\n\nI agree on this for all content that can be modified by the user\n(e.g. files in the work tree such as .gitmodules), but the .git/config\nfile cannot be changed remotely. So I wonder how an attack would\nlook like for a hosting provider or anyone else?\nWe still rely on a sane system and trust /etc/gitconfig\nso we do trust the host/admin but not the user?\n\n>\n>> + for (i = 0; i < max_options; i++) {\n>> + char *line;\n>> + int len;\n>> +\n>> + line = packet_read_line(0, &len);\n>> +\n>> + if (!line)\n>> + break;\n>> +\n>> + if (len > max_size)\n>> + die(\"protocol error: server configuration allows push \"\n>> + \"options of size up to %d bytes\", max_size);\n>> +\n>> + len = strcspn(line, \"\\n\");\n>> + line[len] = '\\0';\n>> +\n>> + string_list_append(ret, line);\n>> + }\n>> + if (i == max_options)\n>> + die(\"protocol error: server configuration only allows up \"\n>> + \"to %d push options\", max_options);\n>\n> When not going over ssh://, does the user sees these messages?\n>\n> More importantly, if we plan to make this configurable and not make\n> the limit a hardwired constant of the wire protocol, it may be\n> better to advertise push-options capability with the limit, e.g.\n> \"push-options=32\" (or even \"push-options=1024/32\"), so that the\n> client side can count and abort early?\n\nYeah we may want to start out with a strict format here indicating\nthe parameters used for evaluating the size.\nSo what do these numbers mean?\n\nI assume (and hence I should document that,) that the first (1024)\nis the maximum number of bytes per push option. The second\nnumber (32) is the number of push options (not the number of pkts,\nas one push option may take more than one pkt if the first number is\nlarger than 65k, see the NEEDSWORK comment.)\n\nDo we really need 2 numbers, or could we just have one number\ndescribing the maximum total size in bytes before the remote rejects\nthe connection?\n\n>\n> I wondered how well the extra flush works with the extra framing\n> smart-http does to wrap the wire protocol; as I do not see any\n> change to the http side, I'd assume that there is no issue.\n\nThat's a dangerous assumption of yours, as I did not test the\nhttps side, yet.\n\n>\n>> +\n>> + return ret;\n>> +}\n>> +\n>> static const char *parse_pack_header(struct pack_header *hdr)\n>> {\n>> switch (read_pack_header(0, hdr)) {\n>> @@ -1773,6 +1829,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>> prepare_shallow_info(&si, &shallow);\n>> if (!si.nr_ours && !si.nr_theirs)\n>> shallow_update = 0;\n>\n"},{"id":"290992","messageId":"CAGZ79kb_oHZUv1=8Q0FSV8yVP5PrwStchXAyhZF=mHhv91APHQ@mail.gmail.com","threadId":"42789","inReplyTo":"xmqqeg75p5cn.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/4] push options: {pre,post}-receive hook learns about push options","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-07T21:50:55Z","receivedAt":"2016-07-07T21:51:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jul 7, 2016 at 1:20 PM, Junio C Hamano wrote:\n>\n> What is suboptimal about the structure of the series is that we\n> won't bisect down to any of the potential bugs in the above code\n> even if we ever see any bug in the future.\n> It also does not hint\n> where push_options is expected to be read in the code in the\n> subsequent patches in the series. If I were doing this series, I\n> would probably have done 2/4 first without plumbing it through\n> (i.e. it is sent and accumulated in a string list at the receiver,\n> and then cleared and freed without being used), and then added the\n> processing (i.e. this step) as the second patch.\n\nBut your first patch (2/4) would not yet advertise the capability?\nOr advertise and then just ignoring it?\n\nThat shadows other bugs that would not properly bisect, I'd imagine?\n\nIt is better for documentation purposes in this patch though. It makes\nthe other patch harder as \"it allows transmitting push options, but\nin that patch nothing of value is done with them.\"\n\nSo I'll see if I can reorder easily.\n\nThanks,\nStefan\n"},{"id":"290993","messageId":"CAGZ79ka6mf3Q+cPqseffXvGkxbSYehkTe6qzzd4z-D51FwQXSQ@mail.gmail.com","threadId":"42789","inReplyTo":"CAPc5daUnUtUK_9caymCkhkdH7DQSEY_UhuyraJmeo9y4Mo86yQ@mail.gmail.com","subject":"Re: [PATCH 4/4] add a test for push options","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-07T21:51:46Z","receivedAt":"2016-07-07T21:51:52Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jul 7, 2016 at 1:01 PM, Junio C Hamano wrote:\n> On Thu, Jul 7, 2016 at 12:51 PM, Junio C Hamano wrote:\n>> Stefan Beller writes:\n>>\n>>> The functions `mk_repo_pair` as well as `test_refs` are borrowed from\n>>> t5543-atomic-push, with additional hooks installed.\n>>>\n>>> Signed-off-by: Stefan Beller \n>>> ---\n>>> t/t5544-push-options.sh | 101 ++++++++++++++++++++++++++++++++++++++++++++++++\n>>> 1 file changed, 101 insertions(+)\n>>> create mode 100755 t/t5544-push-options.sh\n>>\n>> FYI:\n>>\n>> Applying: add a test for push options\n>> Test number t5544 already taken\n>>\n>\n> I'll just move it over to t5545; no need to resend.\n\nI'd resend because of the the first three patches anyway,\nso I can include a renamed version of tests, too.\n"},{"id":"290994","messageId":"xmqqlh1dnmhv.fsf@gitster.mtv.corp.google.com","threadId":"42789","inReplyTo":"CAGZ79kb_oHZUv1=8Q0FSV8yVP5PrwStchXAyhZF=mHhv91APHQ@mail.gmail.com","subject":"Re: [PATCH 1/4] push options: {pre,post}-receive hook learns about push options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-07T21:53:32Z","receivedAt":"2016-07-07T21:53:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> But your first patch (2/4) would not yet advertise the capability?\n> Or advertise and then just ignoring it?\n\nAs I wrote...\n\n>> If I were doing this series, I\n>> would probably have done 2/4 first without plumbing it through\n>> (i.e. it is sent and accumulated in a string list at the receiver,\n>> and then cleared and freed without being used), and then added the\n>> processing (i.e. this step) as the second patch.\n\n... I would imagine it would advertise, allow the other side to send,\nreceive and collect, and then discard (properly) without using.\n\n> It is better for documentation purposes in this patch though. It makes\n> the other patch harder as \"it allows transmitting push options, but\n> in that patch nothing of value is done with them.\"\n>\n> So I'll see if I can reorder easily.\n\nIt does not matter too much. Let's not spend too much time on the\nordering.\n"},{"id":"290995","messageId":"20160707215638.GA27627@sigill.intra.peff.net","threadId":"42789","inReplyTo":"CAGZ79kbkv5P0wP2kKt9gzmZBe1DjLSB8JpZD66DT_Xd4NKqmKQ@mail.gmail.com","subject":"Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-07T21:56:38Z","receivedAt":"2016-07-07T21:57:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 07, 2016 at 02:41:37PM -0700, Stefan Beller wrote:\n\n> >> + /* NEEDSWORK: expose the limitations to be configurable. */\n> >> + int max_options = 32;\n> >> +\n> >> + /*\n> >> + * NEEDSWORK: expose the limitations to be configurable;\n> >> + * Once the limit can be lifted, include a way for payloads\n> >> + * larger than one pkt, e.g allow a payload of up to\n> >> + * LARGE_PACKET_MAX - 1 only, and reserve the last byte\n> >> + * to indicate whether the next pkt continues with this\n> >> + * push option.\n> >> + */\n> >> + int max_size = 1024;\n> >\n> > Good NEEDSWORK comments; perhaps also hint that the configuration\n> > must not come from the repository level configuration file (i.e.\n> > Peff's \"scoped configuration\" from jk/upload-pack-hook topic)?\n> \n> Ok, I reviewed that series. It is unclear to me how the attack would\n> actually look like in that case.\n> \n> In 20b20a22f8f Jeff writes:\n> > Because we promise that\n> > upload-pack is safe to run in an untrusted repository, we\n> > cannot execute arbitrary code or commands found in the\n> > repository (neither in hooks/, nor in the config).\n> \n> I agree on this for all content that can be modified by the user\n> (e.g. files in the work tree such as .gitmodules), but the .git/config\n> file cannot be changed remotely. So I wonder how an attack would\n> look like for a hosting provider or anyone else?\n> We still rely on a sane system and trust /etc/gitconfig\n> so we do trust the host/admin but not the user?\n\nThe problem is for hosting sites which serve repositories via git-daemon\nfrom untrusted users who have real shell accounts (e.g., you set up\ngit-daemon to run as the \"daemon\" user serving repositories out of\npeople's home directories; you don't want users to escalate their shell\naccess into running arbitrary code as \"daemon\").\n\nBut I don't think that case applies here. That is about running\nupload-pack on an untrusted repository, but your changes here are part\nof receive-pack. In such a scenario, users should be pushing as\nthemselves via ssh. And if they are not (e.g., the admin set up\npush-over-smart-http centrally), they are already screwed, as a\nmalicious user could just set up a pre-receive hook.\n\nIOW, we promise only that upload-pack is safe to run an untrusted repo,\nbut not receive-pack.\n\n-Peff\n"},{"id":"290996","messageId":"CAGZ79kbrZuDEN58sdBUQEbLeJbqVo5wNzXRjqAouRGqP=mw01A@mail.gmail.com","threadId":"42789","inReplyTo":"20160707215638.GA27627@sigill.intra.peff.net","subject":"Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-07T22:06:31Z","receivedAt":"2016-07-07T22:06:37Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jul 7, 2016 at 2:56 PM, Jeff King wrote:\n> On Thu, Jul 07, 2016 at 02:41:37PM -0700, Stefan Beller wrote:\n>\n>> >> + /* NEEDSWORK: expose the limitations to be configurable. */\n>> >> + int max_options = 32;\n>> >> +\n>> >> + /*\n>> >> + * NEEDSWORK: expose the limitations to be configurable;\n>> >> + * Once the limit can be lifted, include a way for payloads\n>> >> + * larger than one pkt, e.g allow a payload of up to\n>> >> + * LARGE_PACKET_MAX - 1 only, and reserve the last byte\n>> >> + * to indicate whether the next pkt continues with this\n>> >> + * push option.\n>> >> + */\n>> >> + int max_size = 1024;\n>> >\n>> > Good NEEDSWORK comments; perhaps also hint that the configuration\n>> > must not come from the repository level configuration file (i.e.\n>> > Peff's \"scoped configuration\" from jk/upload-pack-hook topic)?\n>>\n>> Ok, I reviewed that series. It is unclear to me how the attack would\n>> actually look like in that case.\n>>\n>> In 20b20a22f8f Jeff writes:\n>> > Because we promise that\n>> > upload-pack is safe to run in an untrusted repository, we\n>> > cannot execute arbitrary code or commands found in the\n>> > repository (neither in hooks/, nor in the config).\n>>\n>> I agree on this for all content that can be modified by the user\n>> (e.g. files in the work tree such as .gitmodules), but the .git/config\n>> file cannot be changed remotely. So I wonder how an attack would\n>> look like for a hosting provider or anyone else?\n>> We still rely on a sane system and trust /etc/gitconfig\n>> so we do trust the host/admin but not the user?\n>\n> The problem is for hosting sites which serve repositories via git-daemon\n> from untrusted users who have real shell accounts (e.g., you set up\n> git-daemon to run as the \"daemon\" user serving repositories out of\n> people's home directories; you don't want users to escalate their shell\n> access into running arbitrary code as \"daemon\").\n\nI think you would want to lock down the\nhosting site as much as possible and not put untrusted users home\ndirectories on there? So it is hard for me to imagine you'd go for such a setup\nin practice.\n\n>\n> But I don't think that case applies here. That is about running\n> upload-pack on an untrusted repository, but your changes here are part\n> of receive-pack. In such a scenario, users should be pushing as\n> themselves via ssh. And if they are not (e.g., the admin set up\n> push-over-smart-http centrally), they are already screwed, as a\n> malicious user could just set up a pre-receive hook.\n\nI hear that as: \"The pre-receive hook itself can do much more\ndamage than an oversized push option payload\".\n\nOK.\n\n>\n> IOW, we promise only that upload-pack is safe to run an untrusted repo,\n> but not receive-pack.\n>\n> -Peff\n\nThanks,\nStefan\n"},{"id":"290997","messageId":"xmqqh9c1nlvm.fsf@gitster.mtv.corp.google.com","threadId":"42789","inReplyTo":"CAGZ79kbkv5P0wP2kKt9gzmZBe1DjLSB8JpZD66DT_Xd4NKqmKQ@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-07T22:06:53Z","receivedAt":"2016-07-07T22:07:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> No, there is no good objective reason. I added it just after the atomic\n> flag as that is what I implemented.\n>\n> Is there a reason for a particular order of capabilities? I always considered\n> it a set of strings, i.e. any order is valid and there is no preference in\n> which way to put it.\n\nThat is correct, but \"there is no inherent order or grouping\" does\nnot lead to \"hence it is OK to put a new thing at a random place in\nthe middle.\"\n\nIf a reviewer sees a new thing at some specific point in a\ncollection, when the collection is understood not to have any\nspecific order or grouping, it makes the reviewer doubt the\nbelief--there must be a reason why this was placed not at the end;\notherwise a new thing wouldn't be placed randomly at the middle.\n\nIf you place a new thing at the end, it still leaves ambiguity; it\nmay be there because there is no inherent order or grouping, or the\nnew one is sorts later than the one that used to be at the last or\nsomehow related to it. \n\n> I agree on this for all content that can be modified by the user\n> (e.g. files in the work tree such as .gitmodules), but the .git/config\n> file cannot be changed remotely. So I wonder how an attack would\n> look like for a hosting provider or anyone else?\n> We still rely on a sane system and trust /etc/gitconfig\n> so we do trust the host/admin but not the user?\n\nIt is not \"we\" do trust; it is \"we let host/admin trust itself while\nmaking sure that they can protect themselves from their users\".\n\n>> More importantly, if we plan to make this configurable and not make\n>> the limit a hardwired constant of the wire protocol, it may be\n>> better to advertise push-options capability with the limit, e.g.\n>> \"push-options=32\" (or even \"push-options=1024/32\"), so that the\n>> client side can count and abort early?\n>\n> Yeah we may want to start out with a strict format here indicating\n> the parameters used for evaluating the size.\n> So what do these numbers mean?\n>\n> I assume (and hence I should document that,) that the first (1024)\n> is the maximum number of bytes per push option. The second\n> number (32) is the number of push options (not the number of pkts,\n> as one push option may take more than one pkt if the first number is\n> larger than 65k, see the NEEDSWORK comment.)\n>\n> Do we really need 2 numbers, or could we just have one number\n> describing the maximum total size in bytes before the remote rejects\n> the connection?\n\nThat's for you to decide. push-options=32 is probably OK but it\nwill prevent a well-behaving \"git push\" from catching a request that\nwill be rejected, if you are basing the receiver side decision on\nthe other number.\n\nThe suggestion was primarily my reaction after seeing the two new\ncalls to die() on the receiver side, whose message I was not sure\nwill be given to the user who is pushing, i.e.\n\n> When not going over ssh://, does the user see these messages?\n\nYour \"git push\" will be collecting the options in a string-list\nwhile parsing the options, so it would be able to check immediately\nupon seeing the advertised capability if it will trigger this\nprotocol error even before making the request, which would be a good\nthing to do, and I am reasonably sure we can give a better error\nmessage if we do that on the local side without having the receiver\nto tell you (for one thing, we can i18n the local side error\nmessage more easily).\n\n"},{"id":"290998","messageId":"20160707220941.GA27779@sigill.intra.peff.net","threadId":"42789","inReplyTo":"CAGZ79kbrZuDEN58sdBUQEbLeJbqVo5wNzXRjqAouRGqP=mw01A@mail.gmail.com","subject":"Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-07T22:09:41Z","receivedAt":"2016-07-07T22:09:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 07, 2016 at 03:06:31PM -0700, Stefan Beller wrote:\n\n> > The problem is for hosting sites which serve repositories via git-daemon\n> > from untrusted users who have real shell accounts (e.g., you set up\n> > git-daemon to run as the \"daemon\" user serving repositories out of\n> > people's home directories; you don't want users to escalate their shell\n> > access into running arbitrary code as \"daemon\").\n> \n> I think you would want to lock down the\n> hosting site as much as possible and not put untrusted users home\n> directories on there? So it is hard for me to imagine you'd go for such a setup\n> in practice.\n\nSure, I think that's a good way to run a hosting site, too. But it\ndoesn't mean people don't have other needs. kernel.org was run as I\nmentioned above for many years.\n\nAnother related case: you have a multi-user server where Alice might run\n\"git fetch server:~bob/repo.git\". Alice does not want to run arbitrary\ncode based on what is in Bob's repo.git. Even if they are in the same\ncompany, it is a poor security practice.\n\n> > But I don't think that case applies here. That is about running\n> > upload-pack on an untrusted repository, but your changes here are part\n> > of receive-pack. In such a scenario, users should be pushing as\n> > themselves via ssh. And if they are not (e.g., the admin set up\n> > push-over-smart-http centrally), they are already screwed, as a\n> > malicious user could just set up a pre-receive hook.\n> \n> I hear that as: \"The pre-receive hook itself can do much more\n> damage than an oversized push option payload\".\n\nExactly. Or more to the point: we promise nothing here except for\nupload-pack, so changes to receive-pack do not have to worry about this\nissue at all.\n\n-Peff\n"},{"id":"291045","messageId":"20160708175821.GA29326@google.com","threadId":"42789","inReplyTo":"xmqqh9c1nlvm.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2016-07-08T17:58:21Z","receivedAt":"2016-07-08T17:58:57Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJunio C Hamano wrote:\n> Stefan Beller writes:\n\n>>> More importantly, if we plan to make this configurable and not make\n>>> the limit a hardwired constant of the wire protocol, it may be\n>>> better to advertise push-options capability with the limit, e.g.\n>>> \"push-options=32\" (or even \"push-options=1024/32\"), so that the\n>>> client side can count and abort early?\n\nSorry to butt into the conversation late, but: I am not yet convinced.\n\nIs the idea that if the push options were very large, this would save\nthe client from the cost of sending them? But this comes with a\ndownside: the server doesn't get to send an error message about where\nthe maximum number of push options can come from (e.g., with a link to\na page where the limit can be adjusted, or with an explanation of when\nclients tend to run into this problem and what they should do\ninstead).\n\nSo I'd like to propose an alternative. What if the client tells the\nserver the number of push options early on (and possibly also a cap on\nthe length of those push options)? That way, the client doesn't have\nto waste bit sending the push options but the server gets an\nopportunity to send a helpful error message on sideband 3.\n\n\tserver> HEAD\\0push-options ...\n\tclient> ... commands ...\n\tclient> push-options 2\n\tclient> my-first-option\n\tclient> my-second-option\n\nThanks,\nJonathan\n"},{"id":"291047","messageId":"xmqqshvkkm86.fsf@gitster.mtv.corp.google.com","threadId":"42789","inReplyTo":"20160708175821.GA29326@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-08T18:39:53Z","receivedAt":"2016-07-08T18:40:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Hi,\n>\n> Junio C Hamano wrote:\n>> Stefan Beller writes:\n>\n>>>> More importantly, if we plan to make this configurable and not make\n>>>> the limit a hardwired constant of the wire protocol, it may be\n>>>> better to advertise push-options capability with the limit, e.g.\n>>>> \"push-options=32\" (or even \"push-options=1024/32\"), so that the\n>>>> client side can count and abort early?\n>\n> Sorry to butt into the conversation late, but: I am not yet convinced.\n>\n> Is the idea that if the push options were very large, this would save\n> the client from the cost of sending them? \n\nNot really. I have no strong opinion on the benefit of limiting\nnumber/size. Stefan limited the number/size at the receiving end\nand made receiving end die with its message. I was merely trying to\ntweak the arrangement so that the sending end can complain with its\nown message, possibly in its own language, especially because it was\nunclear to me if the die() message on the receiving end would always\ngo back to the sending end correctly.\n\n> But this comes with a\n> downside: the server doesn't get to send an error message about\n> where \n> the maximum number of push options can come from (e.g., with a link to\n> a page where the limit can be adjusted, or with an explanation of when\n> clients tend to run into this problem and what they should do\n> instead).\n\nHmm, interesting point. That would be better told by the receiving\nend, as the way to configure it (if offered) would be different from\ninstallation to installation.\n\n> So I'd like to propose an alternative. What if the client tells the\n> server the number of push options early on (and possibly also a cap on\n> the length of those push options)? That way, the client doesn't have\n> to waste bit sending the push options but the server gets an\n> opportunity to send a helpful error message on sideband 3.\n>\n> \tserver> HEAD\\0push-options ...\n> \tclient> ... commands ...\n> \tclient> push-options 2\n> \tclient> my-first-option\n> \tclient> my-second-option\n\n"},{"id":"291050","messageId":"CAGZ79ka8eHOx28Ja+S6DOGEOrV_ijUvi_NnjprhuzQ+3O9BAoQ@mail.gmail.com","threadId":"42789","inReplyTo":"xmqqshvkkm86.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-08T18:57:20Z","receivedAt":"2016-07-08T18:57:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jul 8, 2016 at 11:39 AM, Junio C Hamano wrote:\n> Jonathan Nieder writes:\n>\n>> Hi,\n>>\n>> Junio C Hamano wrote:\n>>> Stefan Beller writes:\n>>\n>>>>> More importantly, if we plan to make this configurable and not make\n>>>>> the limit a hardwired constant of the wire protocol, it may be\n>>>>> better to advertise push-options capability with the limit, e.g.\n>>>>> \"push-options=32\" (or even \"push-options=1024/32\"), so that the\n>>>>> client side can count and abort early?\n>>\n>> Sorry to butt into the conversation late, but: I am not yet convinced.\n>>\n>> Is the idea that if the push options were very large, this would save\n>> the client from the cost of sending them?\n>\n> Not really. I have no strong opinion on the benefit of limiting\n> number/size. Stefan limited the number/size at the receiving end\n> and made receiving end die with its message.\n\nJeff claimed we'd need some sort of DoS protection for this feature,\nso I considered just die-ing enough for an initial implementation.\n\n> I was merely trying to\n> tweak the arrangement so that the sending end can complain with its\n> own message, possibly in its own language, especially because it was\n> unclear to me if the die() message on the receiving end would always\n> go back to the sending end correctly.\n\nI'm currently implementing Jonathans suggestion as it seems to be a reasonable\ntrade off (client hasn't sent a lot of data when it is decided it\ndoesn't go through,\nthe server can complain with a reasonable error message, only downside: no\ni18n localisation support on the client side as the server will\ncurrently report the\nerror in English).\n\nThat method will make heavy use of rp_error that uses the side band for\ncommunicating the actual error message.\n\n>\n>> But this comes with a\n>> downside: the server doesn't get to send an error message about\n>> where\n>> the maximum number of push options can come from (e.g., with a link to\n>> a page where the limit can be adjusted, or with an explanation of when\n>> clients tend to run into this problem and what they should do\n>> instead).\n>\n> Hmm, interesting point. That would be better told by the receiving\n> end, as the way to configure it (if offered) would be different from\n> installation to installation.\n>\n>> So I'd like to propose an alternative. What if the client tells the\n>> server the number of push options early on (and possibly also a cap on\n>> the length of those push options)? That way, the client doesn't have\n>> to waste bit sending the push options but the server gets an\n>> opportunity to send a helpful error message on sideband 3.\n>>\n>> server> HEAD\\0push-options ...\n>> client> ... commands ...\n>> client> push-options 2\n>> client> my-first-option\n>> client> my-second-option\n>\n\nAnother (slightly offtopic) observation:\nIf in the future we'll need to transmit push options >64k, instead of\nsplitting the push option to multiple packets, we could invent \"large\npackets\". The current upper bound for packets is artificailly low, such\nthat the server is able to interleave sideband information with the actual\ndata in a fetch and have the client display the progress in a timely manner.\nWhen pushing to the server we'd not need to have progress information\n(the server doesn't care, and the client knows the size it is pushing).\n\nAs of today a packet consists of 4 bytes (hex characters) to indicate\nthe length and then the payload follows. So instead we could transmit\n\"v\" (that is not a hex character) followed by a variable length integer for\nthe length and then the payload which has no upper bound.\n\nIn the release notes for 2.3 you wrote:\n> * \"git push\" and \"git fetch\" did not communicate an overlong refname\n> correctly. Now it uses 64kB sideband to accommodate longer ones.\n\nThat could also make use of these \"large packets\" instead.\n\nThanks,\nStefan\n"},{"id":"291059","messageId":"20160708214637.GB9820@sigill.intra.peff.net","threadId":"42789","inReplyTo":"CAGZ79ka8eHOx28Ja+S6DOGEOrV_ijUvi_NnjprhuzQ+3O9BAoQ@mail.gmail.com","subject":"Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-08T21:46:38Z","receivedAt":"2016-07-08T21:46:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 08, 2016 at 11:57:20AM -0700, Stefan Beller wrote:\n\n> >> Sorry to butt into the conversation late, but: I am not yet convinced.\n> >>\n> >> Is the idea that if the push options were very large, this would save\n> >> the client from the cost of sending them?\n> >\n> > Not really. I have no strong opinion on the benefit of limiting\n> > number/size. Stefan limited the number/size at the receiving end\n> > and made receiving end die with its message.\n> \n> Jeff claimed we'd need some sort of DoS protection for this feature,\n> so I considered just die-ing enough for an initial implementation.\n\nI do not think we need to worry too much about niceties for these\nlimits. The point is to protect servers from malicious nonsense, like\nsomebody sending gigabytes of push options, or trying to overflow a\nbuffer in a hook with a large value. If people are seeing these in\nroutine use, then the limits are set too low, and this should happen\nroughly as often as a BUG assertion, and IMHO should be treated roughly\nthe same: don't bother with translation, and don't worry about\noptimizing wasted bandwidth for this case. It won't happen enough to\nmatter.\n\n-Peff\n"},{"id":"291061","messageId":"CAGZ79kZOdcJwd0ePMquhfJTv=xZGP_+w4BRV=GyTy3ejb79yrQ@mail.gmail.com","threadId":"42789","inReplyTo":"20160708214637.GB9820@sigill.intra.peff.net","subject":"Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-08T22:17:13Z","receivedAt":"2016-07-08T22:17:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jul 8, 2016 at 2:46 PM, Jeff King wrote:\n> On Fri, Jul 08, 2016 at 11:57:20AM -0700, Stefan Beller wrote:\n>\n>> >> Sorry to butt into the conversation late, but: I am not yet convinced.\n>> >>\n>> >> Is the idea that if the push options were very large, this would save\n>> >> the client from the cost of sending them?\n>> >\n>> > Not really. I have no strong opinion on the benefit of limiting\n>> > number/size. Stefan limited the number/size at the receiving end\n>> > and made receiving end die with its message.\n>>\n>> Jeff claimed we'd need some sort of DoS protection for this feature,\n>> so I considered just die-ing enough for an initial implementation.\n>\n> I do not think we need to worry too much about niceties for these\n> limits. The point is to protect servers from malicious nonsense, like\n> somebody sending gigabytes of push options, or trying to overflow a\n> buffer in a hook with a large value.\n\nAgreed. This would speak for keeping the implementation as is.\n\n>If people are seeing these in\n> routine use, then the limits are set too low, and this should happen\n> roughly as often as a BUG assertion, and IMHO should be treated roughly\n> the same: don't bother with translation, and don't worry about\n> optimizing wasted bandwidth for this case. It won't happen enough to\n> matter.\n\nWell the wasted band width is part of the server protection, no?\nThis would favor the idea Jonathan came up with:\n\n server: I advertise push options\n client: ok I want to use push options\n client: I'll send you 1000 push options with upper bound of 1000M\n server: It's a bit too much, eh?\n * server quits\n\nSo this case only occurs for the (malicious?) corner case, where I\ndo not bother a translation.\n\nBut having the size announcement not in\nthe capability advertisement, but in the actual push options phase makes\nsense to me as we do not want to clutter the capabilities with data that can\ncome later. We would only waste a little bit of band width, (the\ninitial ls-remote\nand command list of the client).\n\n\nSpeaking of this, I can craft a malicious client that sends the\nfollowing command list\n\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000\nrefs/heads/loooooooooooooooooong-ref\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000\nrefs/heads/loooooooooooooooooong-ref\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000\nrefs/heads/loooooooooooooooooong-ref\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000\nrefs/heads/loooooooooooooooooong-ref\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000\nrefs/heads/loooooooooooooooooong-ref\n\n0000\n\nIIUC in the receive-pack code we would queue that up and the error checking\n(two times null sha1? update of the same ref more than once?), is\ndone just after we send out the flush packet, i.e. when all commands\nare received.\n\nThis would also result in sending gigabytes of junk as well as a\nmemory issue on the server\nside?\n\nThe new push options design is actually neat in the way that the\nclient exactly says what it wants\nand the server can reject early, but not cluttering the capability\nadvertisement.\n\nThanks,\nStefan\n"},{"id":"291063","messageId":"20160708222127.GA10756@sigill.intra.peff.net","threadId":"42789","inReplyTo":"CAGZ79kZOdcJwd0ePMquhfJTv=xZGP_+w4BRV=GyTy3ejb79yrQ@mail.gmail.com","subject":"Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-08T22:21:28Z","receivedAt":"2016-07-08T22:21:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 08, 2016 at 03:17:13PM -0700, Stefan Beller wrote:\n\n> >If people are seeing these in\n> > routine use, then the limits are set too low, and this should happen\n> > roughly as often as a BUG assertion, and IMHO should be treated roughly\n> > the same: don't bother with translation, and don't worry about\n> > optimizing wasted bandwidth for this case. It won't happen enough to\n> > matter.\n> \n> Well the wasted band width is part of the server protection, no?\n\nNot if you stop receiving as soon as you hit the limits. Then of course\nthey can send up to the limit each time, but that is not a DoS. That is\nthings working as advertised.\n\n> This would favor the idea Jonathan came up with:\n> \n> server: I advertise push options\n> client: ok I want to use push options\n> client: I'll send you 1000 push options with upper bound of 1000M\n> server: It's a bit too much, eh?\n> * server quits\n> \n> So this case only occurs for the (malicious?) corner case, where I\n> do not bother a translation.\n\nIn the malicious case, the client says \"I'll send you 10 push option\nwith an upper bound of 1024K\", and then sends gigabytes anyway. Either\nway the server has to react to what is sent, not what is promised.\n\n-Peff\n"},{"id":"291064","messageId":"CAGZ79kbu1ec-8LiFrvnXqFqsjFnEofhjZnxcENa1aA1K56m9Fg@mail.gmail.com","threadId":"42789","inReplyTo":"20160708222127.GA10756@sigill.intra.peff.net","subject":"Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-08T22:29:09Z","receivedAt":"2016-07-08T22:29:14Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jul 8, 2016 at 3:21 PM, Jeff King wrote:\n> On Fri, Jul 08, 2016 at 03:17:13PM -0700, Stefan Beller wrote:\n>\n>> >If people are seeing these in\n>> > routine use, then the limits are set too low, and this should happen\n>> > roughly as often as a BUG assertion, and IMHO should be treated roughly\n>> > the same: don't bother with translation, and don't worry about\n>> > optimizing wasted bandwidth for this case. It won't happen enough to\n>> > matter.\n>>\n>> Well the wasted band width is part of the server protection, no?\n>\n> Not if you stop receiving as soon as you hit the limits. Then of course\n> they can send up to the limit each time, but that is not a DoS. That is\n> things working as advertised.\n>\n>> This would favor the idea Jonathan came up with:\n>>\n>> server: I advertise push options\n>> client: ok I want to use push options\n>> client: I'll send you 1000 push options with upper bound of 1000M\n>> server: It's a bit too much, eh?\n>> * server quits\n>>\n>> So this case only occurs for the (malicious?) corner case, where I\n>> do not bother a translation.\n>\n> In the malicious case, the client says \"I'll send you 10 push option\n> with an upper bound of 1024K\", and then sends gigabytes anyway. Either\n> way the server has to react to what is sent, not what is promised.\n\nWell that is what the initial patch did via:\n\n+ for (i = 0; i < max_options; i++) {\n+ char *line;\n+ int len;\n+\n+ line = packet_read_line(0, &len);\n+\n+ if (!line)\n+ break;\n+\n+ if (len > max_size)\n+ die(\"protocol error: server configuration allows push \"\n+ \"options of size up to %d bytes\", max_size);\n+\n+ len = strcspn(line, \"\\n\");\n+ line[len] = '\\0';\n+\n+ string_list_append(ret, line);\n+ }\n+ if (i == max_options)\n+ die(\"protocol error: server configuration only allows up \"\n+ \"to %d push options\", max_options);\n\nI assume the die is an effective way to \"stop receiving\".\n\nThinking further about what you said, I think the initial selections of\nmax_size and max_options is sufficient and we only see those bounds in\nthe malicious case.\n\nThis discussion rather makes me wonder if we want to stick to the initial design\nas it was easy and not overcomplicating things as we assume the abort case\ndoesn't occur often.\n\n\n>\n> -Peff\n"},{"id":"291065","messageId":"20160708223533.GA11056@sigill.intra.peff.net","threadId":"42789","inReplyTo":"CAGZ79kbu1ec-8LiFrvnXqFqsjFnEofhjZnxcENa1aA1K56m9Fg@mail.gmail.com","subject":"Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-08T22:35:33Z","receivedAt":"2016-07-08T22:35:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 08, 2016 at 03:29:09PM -0700, Stefan Beller wrote:\n\n> > In the malicious case, the client says \"I'll send you 10 push option\n> > with an upper bound of 1024K\", and then sends gigabytes anyway. Either\n> > way the server has to react to what is sent, not what is promised.\n> \n> Well that is what the initial patch did via:\n> \n> + for (i = 0; i < max_options; i++) {\n> + char *line;\n> + int len;\n> +\n> + line = packet_read_line(0, &len);\n> +\n> + if (!line)\n> + break;\n> +\n> + if (len > max_size)\n> + die(\"protocol error: server configuration allows push \"\n> + \"options of size up to %d bytes\", max_size);\n> +\n> + len = strcspn(line, \"\\n\");\n> + line[len] = '\\0';\n> +\n> + string_list_append(ret, line);\n> + }\n> + if (i == max_options)\n> + die(\"protocol error: server configuration only allows up \"\n> + \"to %d push options\", max_options);\n> \n> I assume the die is an effective way to \"stop receiving\".\n> \n> Thinking further about what you said, I think the initial selections of\n> max_size and max_options is sufficient and we only see those bounds in\n> the malicious case.\n> \n> This discussion rather makes me wonder if we want to stick to the initial design\n> as it was easy and not overcomplicating things as we assume the abort case\n> doesn't occur often.\n\nYes. I haven't been following the intermediate discussion and patches,\nbut I don't see anything wrong with the general design above. I think\nyou do need to use rp_error() to get the die message to the client for\nnon-ssh cases, though (that is a problem with other protocol-error\nmessages, too; I wonder if we should install a custom die handler, or\nconvert them all to some kind of rp_die()).\n\n-Peff\n"},{"id":"291067","messageId":"CAGZ79kbj8tGb9BsTo_CGqOdfsinJrafHAY5HhQEQDCtAXQqKgw@mail.gmail.com","threadId":"42789","inReplyTo":"20160708223533.GA11056@sigill.intra.peff.net","subject":"Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-08T22:43:27Z","receivedAt":"2016-07-08T22:43:32Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jul 8, 2016 at 3:35 PM, Jeff King wrote:\n>\n> Yes. I haven't been following the intermediate discussion and patches,\n> but I don't see anything wrong with the general design above. I think\n> you do need to use rp_error() to get the die message to the client for\n> non-ssh cases, though (that is a problem with other protocol-error\n> messages, too; I wonder if we should install a custom die handler, or\n> convert them all to some kind of rp_die()).\n\nSome of the rp_error messages do not want to die(), but most seem to\nbe ok when the rp_error would die.\n\nI'll look into that.\n\n>\n> -Peff\n"},{"id":"291068","messageId":"20160708224601.GA11389@sigill.intra.peff.net","threadId":"42789","inReplyTo":"CAGZ79kbj8tGb9BsTo_CGqOdfsinJrafHAY5HhQEQDCtAXQqKgw@mail.gmail.com","subject":"Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-08T22:46:01Z","receivedAt":"2016-07-08T22:46:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 08, 2016 at 03:43:27PM -0700, Stefan Beller wrote:\n\n> On Fri, Jul 8, 2016 at 3:35 PM, Jeff King wrote:\n> >\n> > Yes. I haven't been following the intermediate discussion and patches,\n> > but I don't see anything wrong with the general design above. I think\n> > you do need to use rp_error() to get the die message to the client for\n> > non-ssh cases, though (that is a problem with other protocol-error\n> > messages, too; I wonder if we should install a custom die handler, or\n> > convert them all to some kind of rp_die()).\n> \n> Some of the rp_error messages do not want to die(), but most seem to\n> be ok when the rp_error would die.\n\nSorry, I meant converting die() into:\n\n rp_error(...);\n die(...);\n\npossibly via an rp_die() helper. The existing rp_error() cases would\nremain untouched.\n\nInstalling a die handler could make that work automatically, though I\nsuspect it would lead to corner cases where we break protocol (e.g., if\nwe die() in the middle of writing out a packet).\n\n-Peff\n"},{"id":"291069","messageId":"CAGZ79kbgbuFWni49z5nZuhmPGtkEYkaGYnZonByyhUYJ9=j1+Q@mail.gmail.com","threadId":"42789","inReplyTo":"20160708224601.GA11389@sigill.intra.peff.net","subject":"Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-08T22:51:01Z","receivedAt":"2016-07-08T22:51:10Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jul 8, 2016 at 3:46 PM, Jeff King wrote:\n> Sorry, I meant converting die() into:\n>\n> rp_error(...);\n> die(...);\n>\n> possibly via an rp_die() helper. The existing rp_error() cases would\n> remain untouched.\n\nOh I see!\nThat makes a lot of sense.\n"},{"id":"291071","messageId":"CAGZ79kbkbb=qYUYmKnuKwJCBtidymXua3eAZGANpXDHT56pqGQ@mail.gmail.com","threadId":"42789","inReplyTo":"xmqq60shp3w9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/4] push: accept push options","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-08T22:59:53Z","receivedAt":"2016-07-08T22:59:59Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jul 7, 2016 at 1:52 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> +-L::\n>> +--push-option::\n>> + Transmit the given string to the server, which passes them to\n>> + the pre-receive as well as the post-receive hook. Only C strings\n>> + containing no new lines are allowed.\n>\n> This is to affect what happens at the remote end, so I would have\n> understood \"-R\". I also would have understood \"-P\" as a short-hand\n> for \"--push-option\". What is the justification of \"-L\"?\n\nIt was made up. The actual code took -o for option. Changed that.\n\n>\n> What does \"C strings\" mean? Did you mean to say \"A sequence of\n> bytes excluding NUL is passed verbatim\"?\n\n\n\n>\n> I do not think I saw anything in the code I reviewed so far that\n> requires \"no LF\" limitation.\n\nIt is enforced server side, but an additional\nclient side enforcement may be better indeed.\n\nThe rationale for no enforcement on the client side is an easier way\nforward if we allow it on the server as the client would \"just work\"\nand it's up to the server to complain.\n\nThat makes me wonder if we want to document that, i.e.:\n\n-o::\n--push-option::\n Transmit the given argument to the server, which passes them to\n the pre-receive as well as the post-receive hook. As it is up to the\n server to react on these push options, it may reject push options\n that contain new line or NUL characters. .\n\n\n>\n> ... Ahh, OK, you want to make sure that push-options are\n> one-per-line in the push certificate. While I do not think it is\n> absolutely necessary, starting with a possibly tighter than\n> necessary limitation is much better than starting loose and having\n> to tighten it later.\n\nThis is not what I had in mind, but rather the pain of dealing with multi line\nenvironment variables.\n\n>> transport_get(remote, NULL);\n>> -\n>> + if (flags & TRANSPORT_PUSH_OPTIONS)\n>> + transport->push_options = push_options;\n>\n> The result would be easier to read without the removal of a blank\n> that separates decl/defn and stmt here.\n\nok\n\n>> + OPT_STRING_LIST('o', \"push-option\", &push_options, N_(\"server-specific\"), N_(\"option to transmit\")),\n>\n> Here it seems to expect \"-o\". If we really want a short option,\n> \"-o\" would probably be OK, as I do not think \"git push\" wants to\n> have \"send the output to this file\" option.\n>\n\nOk, will update the documentation.\n\n\n>\n> by adding this between the two lines in the pre-context of this\n> hunk, i.e.\n>\n> if (push_cert_nonce[0])\n> strbuf_addf(&cert, \"nonce %s\\n\", push_cert_nonce);\n> if (args->push_options)\n> for_each_string_list_item(item, args->push_options)\n> strbuf_addf(&cert, \"push-option %s\\n\", item->string);\n> strbuf_addstr(&cert, \"\\n\");\n>\n\nmakes sense.\n"},{"id":"291190","messageId":"xmqqr3b09ftp.fsf@gitster.mtv.corp.google.com","threadId":"42789","inReplyTo":"CAGZ79kbkbb=qYUYmKnuKwJCBtidymXua3eAZGANpXDHT56pqGQ@mail.gmail.com","subject":"Re: [PATCH 3/4] push: accept push options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-11T18:42:42Z","receivedAt":"2016-07-11T18:42:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> This is not what I had in mind, but rather the pain of dealing with multi line\n> environment variables.\n\nHmph, is that painful?\n\n $ cmd \"$VAR\"\n\ninside double quotes would do multi-line just fine, I think.\n\nBut I think it is OK (and probably preferrable) to limit the options\nto be a collection of one-liners.\n"}]}