{"thread":{"id":"42752","subject":"[PATCH 2/4] receive-pack: implement advertising and receiving push options","startedAt":"2016-06-30T01:08:27Z","lastAt":"2016-07-01T20:11:15Z","messageCount":22,"participants":["Stefan Beller","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"290554","messageId":"20160630005951.7408-3-sbeller@google.com","threadId":"42752","inReplyTo":"20160630005951.7408-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-06-30T00:59:49Z","receivedAt":"2016-06-30T01:08:27Z","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\nFor now we only accept C strings with no new lines.\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 | 44 +++++++++++++++++++++++\n 4 files changed, 64 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 626243f..3b199f9 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 0da6852..68627ed 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@@ -1448,6 +1457,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@@ -1480,6 +1492,35 @@ static struct command *read_head_info(struct sha1_array *shallow)\n \treturn commands;\n }\n \n+static const char *stream_push_options_to_file()\n+{\n+\tstatic const char *fname = \"push-options-XXXXXX\";\n+\tchar *ret = xmallocz(strlen(fname));\n+\tint fd;\n+\tmemcpy(ret, fname, strlen(fname));\n+\tfd = mkstemp(ret);\n+\n+\tfor (;;) {\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 (write_in_full(fd, line, len) < 0 ||\n+\t\t write_in_full(fd, \"\\n\", 1) < 0)\n+\t\t\tgoto fail;\n+\t}\n+\n+\treturn ret;\n+fail:\n+\tclose(fd);\n+\tfree(ret);\n+\treturn NULL;\n+}\n+\n static const char *parse_pack_header(struct pack_header *hdr)\n {\n \tswitch (read_pack_header(0, hdr)) {\n@@ -1767,6 +1808,9 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t\tconst char *unpack_status = NULL;\n \t\tconst char *push_options_file = NULL;\n \n+\t\tif (use_push_options)\n+\t\t\tpush_options_file = stream_push_options_to_file();\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.gdd65b60\n\n"},{"id":"290555","messageId":"20160630005951.7408-5-sbeller@google.com","threadId":"42752","inReplyTo":"20160630005951.7408-1-sbeller@google.com","subject":"[PATCH 4/4] add a test for push options","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-06-30T00:59:51Z","receivedAt":"2016-06-30T01:08:33Z","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 | 85 +++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 85 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..49d5f80\n--- /dev/null\n+++ b/t/t5544-push-options.sh\n@@ -0,0 +1,85 @@\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\tcat $GIT_PUSH_OPTION_FILE >hooks/pre-receive.push_options\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\tcat $GIT_PUSH_OPTION_FILE >hooks/post-receive.push_options\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.gdd65b60\n\n"},{"id":"290556","messageId":"20160630005951.7408-1-sbeller@google.com","threadId":"42752","inReplyTo":null,"subject":"[RFC PATCHv1 0/4] Push options in C Git","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-06-30T00:59:47Z","receivedAt":"2016-06-30T01:26:54Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Allow 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":"290557","messageId":"20160630005951.7408-2-sbeller@google.com","threadId":"42752","inReplyTo":"20160630005951.7408-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-06-30T00:59:48Z","receivedAt":"2016-06-30T01:28:15Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The environment variable GIT_PUSH_OPTION_FILE is set to the push options\nseparated by new line.\n\nThe code is not executed as the push options are set to NULL, nor is the\nnew capability advertised.\n\nThe rationale for keeping the actual options inside a file instead of\nputting them directly into an environment variable has multiple reasons:\n\n1) After a discussion about environment variables and shells, we may not\nwant to put user data into an environment variable (see [1] for example).\n\n2) If a user passes multiple push options, we need to pass them to the\nhooks in a way, the hook can separate them. This could be done via\nmultiple environment variables (e.g. have GIT_PUSH_OPTIONS_COUNT and\nGIT_PUSH_OPTIONS_{0,1,2,...} set), or put it all in one environment\nvariable and choose an appropriate separator. That is hard to parse\nin both ways. For now we'll just put it in a file separated by new line,\nsuch that the hook scripts can pickup the variables as\n\n while read option ; do\n process_push_option() $option\n done < $GIT_PUSH_OPTION_FILE\n\n3) environment variables are messed with in the run-command API depending\non the occurrence of a '=' to set or unset the variable. Having multiple\n'=' is ok, such that we could have it set to a \"key=value\" pair.\n\n4) When putting the options in a file, we need to take care of the race\ncondition of multiple clients pushing. That is actually rather easy.\n\n5) We only inject new lines as separator into the file, so it is possible\nto transmit binaries (\"attach an image to a code review\").\n\n[1] 'Shellshock' https://lwn.net/Articles/614218/\n\nSigned-off-by: Stefan Beller \n---\n Documentation/githooks.txt | 4 ++++\n builtin/receive-pack.c | 41 ++++++++++++++++++++++++++++-------------\n 2 files changed, 32 insertions(+), 13 deletions(-)\n\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex d82e912..dc80574 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -247,6 +247,8 @@ 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 push options are available in the variable GIT_PUSH_OPTION_FILE.\n+\n [[update]]\n update\n ~~~~~~\n@@ -322,6 +324,8 @@ 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 push options are available in the variable GIT_PUSH_OPTION_FILE.\n+\n [[post-update]]\n post-update\n ~~~~~~~~~~~\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 15c323a..0da6852 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 char *push_options_file;\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,9 @@ 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_file)\n+\t\targv_array_pushf(&proc.env_array, \"GIT_PUSH_OPTION_FILE=%s\",\n+\t\t\t\t feed_state->push_options_file);\n \n \tif (use_sideband) {\n \t\tmemset(&muxer, 0, sizeof(muxer));\n@@ -606,12 +617,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 +639,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 char *push_options_file)\n {\n \tstruct receive_hook_feed_state state;\n \tint status;\n@@ -646,6 +653,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_file = push_options_file;\n \tstatus = run_and_feed_hook(hook_name, feed_receive_hook, &state);\n \tstrbuf_release(&state.buf);\n \treturn status;\n@@ -1316,7 +1324,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 char *push_options_file)\n {\n \tstruct command *cmd;\n \tunsigned char sha1[20];\n@@ -1335,7 +1344,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_file)) {\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 +1765,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\tconst char *push_options_file = NULL;\n \n \t\tprepare_shallow_info(&si, &shallow);\n \t\tif (!si.nr_ours && !si.nr_theirs)\n@@ -1764,13 +1774,18 @@ 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_file);\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_file);\n \t\trun_update_post_hook(commands);\n+\t\tif (push_options_file)\n+\t\t\t/* ignore errors */\n+\t\t\tunlink(push_options_file);\n \t\tif (auto_gc) {\n \t\t\tconst char *argv_gc_auto[] = {\n \t\t\t\t\"gc\", \"--auto\", \"--quiet\", NULL,\n-- \n2.9.0.141.gdd65b60\n\n"},{"id":"290558","messageId":"20160630005951.7408-4-sbeller@google.com","threadId":"42752","inReplyTo":"20160630005951.7408-1-sbeller@google.com","subject":"[PATCH 3/4] push: accept push options","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-06-30T00:59:50Z","receivedAt":"2016-06-30T01:31:32Z","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 | 29 +++++++++++++++++++++++++++++\n send-pack.h | 3 +++\n transport.c | 2 ++\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..418f786 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('L', \"push-option\", &push_options, N_(\"server-specific\"), N_(\"options 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 37ee04e..17c30a1 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -261,6 +261,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@@ -279,6 +280,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@@ -371,6 +378,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@@ -393,6 +402,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@@ -419,6 +430,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@@ -427,6 +443,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@@ -513,6 +531,17 @@ 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+\t\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..598bd1f 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)\n@@ -640,6 +641,7 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \tstruct transport *ret = xcalloc(1, sizeof(*ret));\n \n \tret->progress = isatty(2);\n+\tret->push_options = NULL;\n \n \tif (!remote)\n \t\tdie(\"No remote provided to transport_get()\");\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.gdd65b60\n\n"},{"id":"290637","messageId":"20160701071410.GG5358@sigill.intra.peff.net","threadId":"42752","inReplyTo":"20160630005951.7408-2-sbeller@google.com","subject":"Re: [PATCH 1/4] push options: {pre,post}-receive hook learns about push options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-01T07:14:10Z","receivedAt":"2016-07-01T07:14:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 29, 2016 at 05:59:48PM -0700, Stefan Beller wrote:\n\n> The environment variable GIT_PUSH_OPTION_FILE is set to the push options\n> separated by new line.\n> \n> The code is not executed as the push options are set to NULL, nor is the\n> new capability advertised.\n> \n> The rationale for keeping the actual options inside a file instead of\n> putting them directly into an environment variable has multiple reasons:\n\nThanks for including this rationale; my first thought on seeing the\npatch was \"wouldn't this be much more convenient for a hook if each\nvalue had its own environment variable?\".\n\nPutting the data into a file does alleviate some issues. But it also\nmakes parsing annoying, and opens up new issues (like what happens\nto content that has a newline in it?). Wouldn't multiple files be a bit\nmore convenient? Especially for your example of eventually carrying\nlarger binary content like images.\n\n-Peff\n"},{"id":"290638","messageId":"20160701070902.GF5358@sigill.intra.peff.net","threadId":"42752","inReplyTo":"20160630005951.7408-1-sbeller@google.com","subject":"Re: [RFC PATCHv1 0/4] Push options in C Git","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-01T07:09:03Z","receivedAt":"2016-07-01T07:15:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 29, 2016 at 05:59:47PM -0700, Stefan Beller wrote:\n\n> Allow a user to pass information along a push to the pre/post-receive hook\n> on the remote.\n\nSeems like a reasonable thing to want, and related to:\n\n http://thread.gmane.org/gmane.comp.version-control.git/285124\n\nwhich was specifically interested in making \"--quiet\" and \"--force\"\navailable, but stopped short of arbitrary data. (It looks like that\ntopic got overlooked, and Dennis never prodded again).\n\n> When using a remote that is more than just a plain Git host (e.g. Gerrit,\n> Git{hub/lab}, etc) this may become more obvious: The (server backend specific)\n> push options can instruct the server to:\n> * open a pull request\n\nI know this is just a \"for example\", but I don't know if we would ever\nsupport opening a pull request directly via push, if only because a good\npull request has actual parameters that the user needs to fill out (like\nthe cover letter message).\n\nSo you'd probably want some client tool to help the user figure out what\nto put in the PR, and of course that already exists, because GitHub has\nan HTTP API that it can talk to. Shoe-horning it into Git doesn't buy\nmuch.\n\nI'm not sure if your other examples would be better served by just using\nan HTTP API or not. I think Gerrit is funny in this regard because it\neschews branches entirely. E.g., in a GitHub PR, you push to branch\n\"foo\", and then you open a PR using \"foo\" as the source. But with\nGerrit, you push to the magic refs/for/master, and you have no real way\nto cross-reference that submission later.\n\nWhereas in Dennis's patches, it was about specific information directly\nrelated to the act of pushing.\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\nHeh. It _is_ hacks upon hacks, isn't it? ;)\n\n-Peff\n"},{"id":"290682","messageId":"CAGZ79kY7j3r5b0rz=Lb6Ec2s2f8=UBb-hTtZw0H09KM_9H+7=Q@mail.gmail.com","threadId":"42752","inReplyTo":"20160701070902.GF5358@sigill.intra.peff.net","subject":"Re: [RFC PATCHv1 0/4] Push options in C Git","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-01T17:07:10Z","receivedAt":"2016-07-01T17:07:19Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jul 1, 2016 at 12:09 AM, Jeff King wrote:\n> On Wed, Jun 29, 2016 at 05:59:47PM -0700, Stefan Beller wrote:\n>\n>> Allow a user to pass information along a push to the pre/post-receive hook\n>> on the remote.\n>\n> Seems like a reasonable thing to want, and related to:\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/285124\n>\n> which was specifically interested in making \"--quiet\" and \"--force\"\n> available, but stopped short of arbitrary data. (It looks like that\n> topic got overlooked, and Dennis never prodded again).\n\nDoh! I should have searched for such a topic. :/\nI looked at the patches and they differ conceptually in what goes over the wire.\n\nIn Dennis series that is a hook_options= in the capability\nselection line,\nthat is after the first update line, i.e. the hook options are\nrestricted to not contain\nwhite spaces and be of length < ~60k characters IIUC (reserve about 4k for the\nupdate, ref and other selected capabilities)\n\nIn this proposal we only indicate that we want to transmit options in the\nnegotiation phase, and then after the update transmission is done, you can\nhave as many packets as you want excluding flush packets (the flush packet\nindicates we're done transmitting these push/hook options)\n\nSo I might be biased by my code, but I'd claim that is more future proof.\n\n>\n>> When using a remote that is more than just a plain Git host (e.g. Gerrit,\n>> Git{hub/lab}, etc) this may become more obvious: The (server backend specific)\n>> push options can instruct the server to:\n>> * open a pull request\n>\n> I know this is just a \"for example\", but I don't know if we would ever\n> support opening a pull request directly via push, if only because a good\n> pull request has actual parameters that the user needs to fill out (like\n> the cover letter message).\n\nSure, just an example! You could have\n\n \"git push --read-push-option-from-file=coverletter.html\".\n\nas this really only disallows flush packets (i.e. anything goes,\nincluding images)\nand the server must be smart enough to interpret the file.\n\n>\n> So you'd probably want some client tool to help the user figure out what\n> to put in the PR, and of course that already exists, because GitHub has\n> an HTTP API that it can talk to. Shoe-horning it into Git doesn't buy\n> much.\n\nLet's say the example was bad. the reality is that Dennis and I implemented\na needed feature independently of each other and you have the\nout-of-the-box-Github-does-HTTP-too solution. So I think we'd want\n*something* in Git to have information in the receiving hooks available?\n\nIt seems to me there are many other Git-wrappers, such as the repo tool\nor git-review[2], and most of them start with the premise: \"Git doesn't support\nX yet, so while they take their time to figure out how to do it\nproperly upstream,\nwe'll just use this hacky script\" and then eventually you end up with an xml\nparsing beast with thousands lines of python code (The story of the repo\ntool and submodules).\n\n[1] https://code.google.com/p/git-repo/\n[2] http://linux.die.net/man/1/git-review\n\n>\n> I'm not sure if your other examples would be better served by just using\n> an HTTP API or not.\n\nThe decision for the HTTP API would be the decision for a wrapper tool.\nAnd all these wrapper tools differ, so it is not easy to switch from one to\nanother, on the other hand, if you know Git well enough you can easily adapt\nto other work flows as it is not a new tool, but just a new command line option\nor so?\n\n> I think Gerrit is funny in this regard because it\n> eschews branches entirely. E.g., in a GitHub PR, you push to branch\n> \"foo\", and then you open a PR using \"foo\" as the source.\n\nOnce upon a time you could also open a pull request using the sha1?\n(I did that once and then was asked to do some fixes before pulling and\nI had to abandon and reopen a proper branch PR)\n\n> But with\n> Gerrit, you push to the magic refs/for/master, and you have no real way\n> to cross-reference that submission later.\n\nWhat do you mean by cross reference?\n\nI could ask you to look at https://gerrit-review.googlesource.com/#/c/79354/\nand in the upper right corner you'll find a download button that let's you copy\n\"git fetch refs/changes/54/79354/3 && git checkout FETCH_HEAD\".\nThat allows you to inspect that thing alone later (or any of the\nrevisions as the\nstructure is refs/changes/<2 digit sharding>//\n\"revision\" translates to the concept of resending a patch series v2,v3 here)\n\nAlternatively I could ask you to look for a \"Change-Id\" that is a hash in the\nfooter of a commit, so you can find it in the Git-history or on the review site.\n\nGerrits flaws are off topic to this series though (maybe?)\n\n>\n> Whereas in Dennis's patches, it was about specific information directly\n> related to the act of pushing.\n\nBut does it cover all the informations related to pushing? like\n\n \"I am a bot, down-prioritize me, please\"\n \"I am a bot, therefore I do not care about the internal replication\non the server\"\n\nThe last one is interesting as it is very specific to our servers. So you could\nargue we'd want to roll our own fork of Git that also covers such a hook option,\nbut I think it is easier for both the Git community as well as us here to allow\nGit transmit free form text and the server can decide how to act upon it?\n\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> Heh. It _is_ hacks upon hacks, isn't it? ;)\n\nI do not disagree. Now is the time to do it right, but we like the\nconvenience of having just\none tool (Git) that can do it all for us. We don't want to write yet\nanother wrapper script that\ndoes the HTTP API thing. ;)\n\n\nI might have missed the point of this email completely as I seem to\ntry to defend the\npatch series (and Gerrit a bit). So do you think the functionality of\nthis series is overkill\nand we'd rather go with Dennis series?\n\nI just think that is not future proof on the wire as it restricts what\nyou can put on the wire\nas well as not future proof as it specifies what you options you can\nselect over the wire.\n\nGits spirit (Oh no, not this discussion!) is to allow a broad range of\nuses and work flows.\nIt doesn't hinder you from shooting into your feet if that's what\nyou're into, and\nthat is what this series does precisely.\n\n>\n> -Peff\n\nThanks,\nStefan\n"},{"id":"290683","messageId":"xmqqh9c9z3kf.fsf@gitster.mtv.corp.google.com","threadId":"42752","inReplyTo":"20160630005951.7408-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-01T17:11:12Z","receivedAt":"2016-07-01T17:11:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> +static const char *stream_push_options_to_file()\n> +{\n> +\tstatic const char *fname = \"push-options-XXXXXX\";\n> +\tchar *ret = xmallocz(strlen(fname));\n> +\tint fd;\n> +\tmemcpy(ret, fname, strlen(fname));\n> +\tfd = mkstemp(ret);\n\nProbably\n\n\tchar *ret;\n\tint fd;\n\n\tret = xstrdup(\"push-options-XXXXXX\");\n\tfd = xmkstemp(ret);\n\nor use mkstemp but check the return value and \"goto fail\".\n\n> +\tfor (;;) {\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 (write_in_full(fd, line, len) < 0 ||\n> +\t\t write_in_full(fd, \"\\n\", 1) < 0)\n> +\t\t\tgoto fail;\n> +\t}\n> +\n> +\treturn ret;\n> +fail:\n> +\tclose(fd);\n> +\tfree(ret);\n> +\treturn NULL;\n> +}\n> +\n> static const char *parse_pack_header(struct pack_header *hdr)\n> {\n> \tswitch (read_pack_header(0, hdr)) {\n> @@ -1767,6 +1808,9 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n> \t\tconst char *unpack_status = NULL;\n> \t\tconst char *push_options_file = NULL;\n> \n> +\t\tif (use_push_options)\n> +\t\t\tpush_options_file = stream_push_options_to_file();\n> +\n> \t\tprepare_shallow_info(&si, &shallow);\n> \t\tif (!si.nr_ours && !si.nr_theirs)\n> \t\t\tshallow_update = 0;\n"},{"id":"290684","messageId":"CAGZ79kaDCLm3BBURJKfkYWKKvozkFTGCn0wGiQCtspUvtQBd+g@mail.gmail.com","threadId":"42752","inReplyTo":"20160701071410.GG5358@sigill.intra.peff.net","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-01T17:20:32Z","receivedAt":"2016-07-01T17:20:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jul 1, 2016 at 12:14 AM, Jeff King wrote:\n>\n> Putting the data into a file does alleviate some issues. But it also\n> makes parsing annoying, and opens up new issues (like what happens\n> to content that has a newline in it?). Wouldn't multiple files be a bit\n> more convenient? Especially for your example of eventually carrying\n> larger binary content like images.\n\nYeah I did not think about having multiple files. I think that is the\nway to go now.\nIn the environment variable you'd have the paths, such that\n\nprocess_hook_option_from_file()\n{\n ... # do a thing with $1\n}\n\nfor p in $GIT_HOOK_OPTION_FILES ;\ndo\n process_hook_option_from_file $p\ndone\n\nis a viable hook.\n\n\n>> The rationale for keeping the actual options inside a file instead of\n>> putting them directly into an environment variable has multiple reasons:\n>\n> Thanks for including this rationale; my first thought on seeing the\n> patch was \"wouldn't this be much more convenient for a hook if each\n> value had its own environment variable?\".\n\nThat's what I thought as well. Office discussion ensued and I am still\noffended by this solution, but it sucks less than multiple environment variables\n(I tried writing a script to construct and evaluate the environment\nvariables and\nthat doesn't look nice)\n\nIf we did not have a GIT_PUSH_OPTIONS_COUNT and GIT_PUSH_OPTION_\nbut rather GIT_PUSH_OPTIONS_VARIABLES that contains the other variables,\nit may be easier to handle, but whether you read from a file or evaluate the\nenvironment variable is only a minor step, the indirection is there anyway\nand this would be very close to what we have above.\n\nThanks,\nStefan\n\n>\n> -Peff\n"},{"id":"290686","messageId":"CAGZ79kZxqRs=c-Fxe=uyCiPAw_R79gVtpyMn0OYc3OFrdev-hQ@mail.gmail.com","threadId":"42752","inReplyTo":"xmqqh9c9z3kf.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-01T17:24:28Z","receivedAt":"2016-07-01T17:24:34Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jul 1, 2016 at 10:11 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> +static const char *stream_push_options_to_file()\n>> +{\n>> + static const char *fname = \"push-options-XXXXXX\";\n>> + char *ret = xmallocz(strlen(fname));\n>> + int fd;\n>> + memcpy(ret, fname, strlen(fname));\n>> + fd = mkstemp(ret);\n>\n> Probably\n>\n> char *ret;\n> int fd;\n>\n> ret = xstrdup(\"push-options-XXXXXX\");\n> fd = xmkstemp(ret);\n>\n\nThat is way better to read!\nAlso I did not know we also have a wrapper for mkstemp;\nMaybe I should check for any new syscall if we wrap that.\n\n> or use mkstemp but check the return value and \"goto fail\".\n>\n>> + for (;;) {\n>> + char *line;\n>> + int len;\n>> +\n>> + line = packet_read_line(0, &len);\n>> +\n>> + if (!line)\n>> + break;\n>> +\n>> + if (write_in_full(fd, line, len) < 0 ||\n>> + write_in_full(fd, \"\\n\", 1) < 0)\n>> + goto fail;\n>> + }\n>> +\n>> + return ret;\n>> +fail:\n>> + close(fd);\n>> + free(ret);\n>> + return NULL;\n>> +}\n>> +\n>> static const char *parse_pack_header(struct pack_header *hdr)\n>> {\n>> switch (read_pack_header(0, hdr)) {\n>> @@ -1767,6 +1808,9 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n>> const char *unpack_status = NULL;\n>> const char *push_options_file = NULL;\n>>\n>> + if (use_push_options)\n>> + push_options_file = stream_push_options_to_file();\n>> +\n>> prepare_shallow_info(&si, &shallow);\n>> if (!si.nr_ours && !si.nr_theirs)\n>> shallow_update = 0;\n"},{"id":"290687","messageId":"20160701175503.GA16235@sigill.intra.peff.net","threadId":"42752","inReplyTo":"CAGZ79kY7j3r5b0rz=Lb6Ec2s2f8=UBb-hTtZw0H09KM_9H+7=Q@mail.gmail.com","subject":"Re: [RFC PATCHv1 0/4] Push options in C Git","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-01T17:55:04Z","receivedAt":"2016-07-01T17:55:14Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 01, 2016 at 10:07:10AM -0700, Stefan Beller wrote:\n\n> > So you'd probably want some client tool to help the user figure out what\n> > to put in the PR, and of course that already exists, because GitHub has\n> > an HTTP API that it can talk to. Shoe-horning it into Git doesn't buy\n> > much.\n> \n> Let's say the example was bad. the reality is that Dennis and I implemented\n> a needed feature independently of each other and you have the\n> out-of-the-box-Github-does-HTTP-too solution. So I think we'd want\n> *something* in Git to have information in the receiving hooks available?\n\nSure, _if_ it's information that the receiving hooks want to have.\n\nI guess what I'm questioning is exactly what receiving hooks do need to\nhave. Things like \"the user said --force\" seem obviously related to the\npush. Things like auto-creating a PR seem less about the push itself,\nand more about something you can do with the result of the push.\n\nSo a litmus test for me would be something like:\n\n Can you get the same effect with:\n\n git push $remote my-branch &&\n curl https://$remote/api/do-the-thing?q=my-branch\n\n as you would with:\n\n git push --push-data=do-the-thing $remote mybranch\n\nIf so, then it seems like inserting the data through the git connection\nis not really adding functionality, but it _is_ adding complexity to\ngit (that other tools are already handling with much more flexibility).\n\nWe have a similar rule of thumb for hooks; we don't add a hook if\nthere's no reason you couldn't just run the hook content immediately\nbefore or after the command.\n\n> It seems to me there are many other Git-wrappers, such as the repo tool\n> or git-review[2], and most of them start with the premise: \"Git doesn't support\n> X yet, so while they take their time to figure out how to do it\n> properly upstream,\n> we'll just use this hacky script\" and then eventually you end up with an xml\n> parsing beast with thousands lines of python code (The story of the repo\n> tool and submodules).\n\nBut it's exactly this complexity that makes me worried about moving\nthings into git. This patch addresses only the transport problem. If\nyour problem is that something like a review system built on git\ninvolves lot of complicated data, now you have two problems: you still\nhave the complicated data, and now you have to shoe-horn it into Git's\nad-hoc protocol.\n\nI'm also not convinced that it alleviates the need for the tool to have\na separate API. Imagine you can send this data while pushing now. How do\nyou edit it later? How do you retrieve it programmatically? Etc.\n\n> > I think Gerrit is funny in this regard because it\n> > eschews branches entirely. E.g., in a GitHub PR, you push to branch\n> > \"foo\", and then you open a PR using \"foo\" as the source.\n> \n> Once upon a time you could also open a pull request using the sha1?\n> (I did that once and then was asked to do some fixes before pulling and\n> I had to abandon and reopen a proper branch PR)\n> \n> > But with\n> > Gerrit, you push to the magic refs/for/master, and you have no real way\n> > to cross-reference that submission later.\n> \n> What do you mean by cross reference?\n\nI just meant that in my GitHub example, the branch name you chose\n(\"foo\") becomes the key with which you can correlate the two actions:\npushing the data via Git, and later opening a PR (via the web page, or\nthe API, or whatever).\n\nWith Gerrit, how do you refer to the commits you just pushed, when\nmaking a separate action? My impression is that Gerrit assigns\nchange-ids centrally when you push to refs/for/master; how do you figure\nout after the push what the change-id is of the thing you just pushed?\nI can guess maybe it sends something to stderr that the user then reads,\nbut that is purely a guess. I've never really used Gerrit.\n\nYou're right that one could use sha1s for this purpose.\n\n> Gerrits flaws are off topic to this series though (maybe?)\n\nI'm far from qualified to critique Gerrit. I was mostly just observing\nthat your view of the problem space (and thus the solution) is colored\nby Gerrit's way of doing things. That doesn't make it a wrong solution,\nbut it's worth seeing whether other systems would get the same benefit\n(just because part of the \"is it worth it\" equation is how widely used\nthe feature could be).\n\n> > Whereas in Dennis's patches, it was about specific information directly\n> > related to the act of pushing.\n> \n> But does it cover all the informations related to pushing? like\n> \n> \"I am a bot, down-prioritize me, please\"\n> \"I am a bot, therefore I do not care about the internal replication\n> on the server\"\n> \n> The last one is interesting as it is very specific to our servers. So you could\n> argue we'd want to roll our own fork of Git that also covers such a hook option,\n> but I think it is easier for both the Git community as well as us here to allow\n> Git transmit free form text and the server can decide how to act upon it?\n\nNo, I don't think it covers all information. I like the idea of being\nable to pass free-form key/value pairs to hooks, because by definition\nwe don't know what the hooks are doing, or what the user might want to\ncommunicate to them.\n\n> I might have missed the point of this email completely as I seem to\n> try to defend the\n> patch series (and Gerrit a bit). So do you think the functionality of\n> this series is overkill\n> and we'd rather go with Dennis series?\n\nOh, I'm supposed to have a point to my emails? :)\n\nI was mostly just musing. I do see the general idea as a useful thing;\nthere really is data that is related to the push itself, and not other\nactions you may want to perform on the push.\n\nI'm not necessarily sure that we need a complicated data model (like\nmultiple values of the same key), or the ability to handle arbitrarily\nlarge binary data. Those don't necessarily complicate the wire protocol\n(I like the idea of adding a new phase of key/values, followed by a\nflush), but they do complicate the server implementation (how do we\ncommunicate them to the hooks? Should there be limits for safety and DoS\nprotection on the server?)\n\n> Gits spirit (Oh no, not this discussion!) is to allow a broad range of\n> uses and work flows. It doesn't hinder you from shooting into your\n> feet if that's what you're into, and that is what this series does\n> precisely.\n\nI think Git's spirit is also to fit as a tool into the greater\necosystem, and not duplicate functionality that is done elsewhere. So\nfor example there is no authentication in the git:// protocol; that job\nis done by ssh.\n\n-Peff\n"},{"id":"290688","messageId":"20160701175950.GB16235@sigill.intra.peff.net","threadId":"42752","inReplyTo":"CAGZ79kaDCLm3BBURJKfkYWKKvozkFTGCn0wGiQCtspUvtQBd+g@mail.gmail.com","subject":"Re: [PATCH 1/4] push options: {pre,post}-receive hook learns about push options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-01T17:59:50Z","receivedAt":"2016-07-01T17:59:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 01, 2016 at 10:20:32AM -0700, Stefan Beller wrote:\n\n> >> The rationale for keeping the actual options inside a file instead of\n> >> putting them directly into an environment variable has multiple reasons:\n> >\n> > Thanks for including this rationale; my first thought on seeing the\n> > patch was \"wouldn't this be much more convenient for a hook if each\n> > value had its own environment variable?\".\n> \n> That's what I thought as well. Office discussion ensued and I am still\n> offended by this solution, but it sucks less than multiple environment variables\n> (I tried writing a script to construct and evaluate the environment\n> variables and\n> that doesn't look nice)\n\nIf you give up on having multiple incarnations of each variable, then I\nthink:\n\n GIT_PUSH_VAR_foo=value_for_foo\n GIT_PUSH_VAR_bar=value_for_bar\n\nis quite elegant, and easy to use from hooks. It just cannot represent\nmultiple such \"foo\" variables.\n\n> If we did not have a GIT_PUSH_OPTIONS_COUNT and GIT_PUSH_OPTION_\n> but rather GIT_PUSH_OPTIONS_VARIABLES that contains the other variables,\n> it may be easier to handle, but whether you read from a file or evaluate the\n> environment variable is only a minor step, the indirection is there anyway\n> and this would be very close to what we have above.\n\nIt makes the server implementation a bit uglier. You have to create the\ntemporary file, and you have to clean it up. What process is responsible\nfor cleaning up stale files? Obviously receive-pack would try to clean\nup after itself, but what happens when it is \"kill -9\"'d, or the system\ngoes down, etc? We clean up stale tmp files like tmp_obj_* in git-gc; I\nthink we'd want something like that here.\n\n-Peff\n"},{"id":"290690","messageId":"CAPc5daWjSW5KM4uUyEBbb+765t50+dUsewF52uPrCiT1HW=NAQ@mail.gmail.com","threadId":"42752","inReplyTo":"20160701175950.GB16235@sigill.intra.peff.net","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-01T18:03:06Z","receivedAt":"2016-07-01T18:03:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Fri, Jul 1, 2016 at 10:59 AM, Jeff King wrote:\n> If you give up on having multiple incarnations of each variable, then I\n> think:\n>\n> GIT_PUSH_VAR_foo=value_for_foo\n> GIT_PUSH_VAR_bar=value_for_bar\n>\n> is quite elegant, and easy to use from hooks. It just cannot represent\n> multiple such \"foo\" variables.\n>\n>> If we did not have a GIT_PUSH_OPTIONS_COUNT and GIT_PUSH_OPTION_\n>> but rather GIT_PUSH_OPTIONS_VARIABLES that contains the other variables,\n>> it may be easier to handle, but whether you read from a file or evaluate the\n>> environment variable is only a minor step, the indirection is there anyway\n>> and this would be very close to what we have above.\n>\n> It makes the server implementation a bit uglier. You have to create the\n> temporary file, and you have to clean it up. What process is responsible\n> for cleaning up stale files? Obviously receive-pack would try to clean\n> up after itself, but what happens when it is \"kill -9\"'d, or the system\n> goes down, etc? We clean up stale tmp files like tmp_obj_* in git-gc; I\n> think we'd want something like that here.\n\nIt still is not clear to me why the option to pass _COUNT and _VAR_ is\nrejected.\n"},{"id":"290692","messageId":"20160701181102.GA16695@sigill.intra.peff.net","threadId":"42752","inReplyTo":"CAPc5daWjSW5KM4uUyEBbb+765t50+dUsewF52uPrCiT1HW=NAQ@mail.gmail.com","subject":"Re: [PATCH 1/4] push options: {pre,post}-receive hook learns about push options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-01T18:11:02Z","receivedAt":"2016-07-01T18:11:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 01, 2016 at 11:03:06AM -0700, Junio C Hamano wrote:\n\n> On Fri, Jul 1, 2016 at 10:59 AM, Jeff King wrote:\n> > If you give up on having multiple incarnations of each variable, then I\n> > think:\n> >\n> > GIT_PUSH_VAR_foo=value_for_foo\n> > GIT_PUSH_VAR_bar=value_for_bar\n> >\n> > is quite elegant, and easy to use from hooks. It just cannot represent\n> > multiple such \"foo\" variables.\n> >\n> >> If we did not have a GIT_PUSH_OPTIONS_COUNT and GIT_PUSH_OPTION_\n> >> but rather GIT_PUSH_OPTIONS_VARIABLES that contains the other variables,\n> >> it may be easier to handle, but whether you read from a file or evaluate the\n> >> environment variable is only a minor step, the indirection is there anyway\n> >> and this would be very close to what we have above.\n> >\n> > It makes the server implementation a bit uglier. You have to create the\n> > temporary file, and you have to clean it up. What process is responsible\n> > for cleaning up stale files? Obviously receive-pack would try to clean\n> > up after itself, but what happens when it is \"kill -9\"'d, or the system\n> > goes down, etc? We clean up stale tmp files like tmp_obj_* in git-gc; I\n> > think we'd want something like that here.\n> \n> It still is not clear to me why the option to pass _COUNT and _VAR_ is\n> rejected.\n\nIt's a little more unwieldy to parse in a shell hook, but not too bad, I\nguess:\n\n if test -n \"$GIT_PUSH_VAR_COUNT\"; then\n i=0\n while test \"$i\" -lt \"$GIT_PUSH_VAR_COUNT\"; do\n eval \"value=\\$GIT_PUSH_VAR_$i\"\n case \"$value\" in\n force=*)\n\tforce=${value#*=}\n\t;;\n ... and so on ...\n esac\n done\n fi\n\n ...\n if test \"$force\" = true\n ...\n\nCompare to:\n\n if test \"$GIT_PUSH_VAR_force\" = true\n ...\n\nThe \"count\" method gives you the flexibility to parse multiple keys as\nlists, last-one-wins, or whatever scheme you want. But it also gives you\nthe _responsibility_ to do the parsing yourself, which is a pain when\nyou want to do the simple thing.\n\n-Peff\n"},{"id":"290695","messageId":"CAGZ79kZx6=LGjQjzEEjMjZrNbXcVto-+djJbxj9xUCtLMsaL5w@mail.gmail.com","threadId":"42752","inReplyTo":"20160701175503.GA16235@sigill.intra.peff.net","subject":"Re: [RFC PATCHv1 0/4] Push options in C Git","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-01T18:25:40Z","receivedAt":"2016-07-01T18:25:47Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jul 1, 2016 at 10:55 AM, Jeff King wrote:\n> On Fri, Jul 01, 2016 at 10:07:10AM -0700, Stefan Beller wrote:\n>\n>> > So you'd probably want some client tool to help the user figure out what\n>> > to put in the PR, and of course that already exists, because GitHub has\n>> > an HTTP API that it can talk to. Shoe-horning it into Git doesn't buy\n>> > much.\n>>\n>> Let's say the example was bad. the reality is that Dennis and I implemented\n>> a needed feature independently of each other and you have the\n>> out-of-the-box-Github-does-HTTP-too solution. So I think we'd want\n>> *something* in Git to have information in the receiving hooks available?\n>\n> Sure, _if_ it's information that the receiving hooks want to have.\n>\n> I guess what I'm questioning is exactly what receiving hooks do need to\n> have. Things like \"the user said --force\" seem obviously related to the\n> push. Things like auto-creating a PR seem less about the push itself,\n> and more about something you can do with the result of the push.\n>\n> So a litmus test for me would be something like:\n>\n> Can you get the same effect with:\n>\n> git push $remote my-branch &&\n> curl https://$remote/api/do-the-thing?q=my-branch\n>\n> as you would with:\n>\n> git push --push-data=do-the-thing $remote mybranch\n\nPeople are happy when they can use one tool instead of two,\ne.g. \"Deploy with Git\" is a thing[1] and maybe they want to add a\nswitch \"once pushed skip the tests, I know they are broken\".\n\n[1] https://devcenter.heroku.com/articles/git\n\n>\n> If so, then it seems like inserting the data through the git connection\n> is not really adding functionality, but it _is_ adding complexity to\n> git (that other tools are already handling with much more flexibility).\n>\n> We have a similar rule of thumb for hooks; we don't add a hook if\n> there's no reason you couldn't just run the hook content immediately\n> before or after the command.\n>\n>> It seems to me there are many other Git-wrappers, such as the repo tool\n>> or git-review[2], and most of them start with the premise: \"Git doesn't support\n>> X yet, so while they take their time to figure out how to do it\n>> properly upstream,\n>> we'll just use this hacky script\" and then eventually you end up with an xml\n>> parsing beast with thousands lines of python code (The story of the repo\n>> tool and submodules).\n>\n> But it's exactly this complexity that makes me worried about moving\n> things into git. This patch addresses only the transport problem. If\n> your problem is that something like a review system built on git\n> involves lot of complicated data, now you have two problems: you still\n> have the complicated data, and now you have to shoe-horn it into Git's\n> ad-hoc protocol.\n>\n> I'm also not convinced that it alleviates the need for the tool to have\n> a separate API. Imagine you can send this data while pushing now. How do\n> you edit it later? How do you retrieve it programmatically? Etc.\n>\n>> > I think Gerrit is funny in this regard because it\n>> > eschews branches entirely. E.g., in a GitHub PR, you push to branch\n>> > \"foo\", and then you open a PR using \"foo\" as the source.\n>>\n>> Once upon a time you could also open a pull request using the sha1?\n>> (I did that once and then was asked to do some fixes before pulling and\n>> I had to abandon and reopen a proper branch PR)\n>>\n>> > But with\n>> > Gerrit, you push to the magic refs/for/master, and you have no real way\n>> > to cross-reference that submission later.\n>>\n>> What do you mean by cross reference?\n>\n> I just meant that in my GitHub example, the branch name you chose\n> (\"foo\") becomes the key with which you can correlate the two actions:\n> pushing the data via Git, and later opening a PR (via the web page, or\n> the API, or whatever).\n>\n> With Gerrit, how do you refer to the commits you just pushed, when\n> making a separate action? My impression is that Gerrit assigns\n> change-ids centrally when you push to refs/for/master;\n\nactually you install a commit hook to do it decentralized.\n\n> how do you figure\n> out after the push what the change-id is of the thing you just pushed?\n> I can guess maybe it sends something to stderr that the user then reads,\n> but that is purely a guess. I've never really used Gerrit.\n\nI assigns the \"legacy\" sequential numbers like so:\n\n$ git push origin HEAD:refs/for/master\nCounting objects: 18, done.\nDelta compression using up to 12 threads.\nCompressing objects: 100% (18/18), done.\nWriting objects: 100% (18/18), 4.15 KiB | 0 bytes/s, done.\nTotal 18 (delta 13), reused 0 (delta 0)\nremote: Resolving deltas: 100% (13/13)\nremote: Processing changes: new: 3, updated: 2, done\nremote: (W) ba18b08: too many commit message lines longer than 70\ncharacters; manually wrap lines\nremote:\nremote: New Changes:\nremote: https://gerrit-review.googlesource.com/79620 project:\nintroduce hasCommit method\nremote: https://gerrit-review.googlesource.com/79621 project:\nGetSubmodules uses recursive switch for ls-tree\nremote: https://gerrit-review.googlesource.com/79622 sync: add a\nquick flag to ignore up to date projects\nremote:\nremote:\nremote: Updated Changes:\nremote: https://gerrit-review.googlesource.com/79195 manifest_xml:\nmove path parsing before revision parsing\nremote: https://gerrit-review.googlesource.com/79200 Add a\nsuperproject metarepository for atomic syncs\nremote:\nTo https://gerrit.googlesource.com/git-repo\n * [new branch] HEAD -> refs/for/master\n\n\n>\n> You're right that one could use sha1s for this purpose.\n>\n>> Gerrits flaws are off topic to this series though (maybe?)\n>\n> I'm far from qualified to critique Gerrit. I was mostly just observing\n> that your view of the problem space (and thus the solution) is colored\n> by Gerrit's way of doing things. That doesn't make it a wrong solution,\n> but it's worth seeing whether other systems would get the same benefit\n> (just because part of the \"is it worth it\" equation is how widely used\n> the feature could be).\n\n\n>\n>> > Whereas in Dennis's patches, it was about specific information directly\n>> > related to the act of pushing.\n>>\n>> But does it cover all the informations related to pushing? like\n>>\n>> \"I am a bot, down-prioritize me, please\"\n>> \"I am a bot, therefore I do not care about the internal replication\n>> on the server\"\n>>\n>> The last one is interesting as it is very specific to our servers. So you could\n>> argue we'd want to roll our own fork of Git that also covers such a hook option,\n>> but I think it is easier for both the Git community as well as us here to allow\n>> Git transmit free form text and the server can decide how to act upon it?\n>\n> No, I don't think it covers all information. I like the idea of being\n> able to pass free-form key/value pairs to hooks, because by definition\n> we don't know what the hooks are doing, or what the user might want to\n> communicate to them.\n>\n>> I might have missed the point of this email completely as I seem to\n>> try to defend the\n>> patch series (and Gerrit a bit). So do you think the functionality of\n>> this series is overkill\n>> and we'd rather go with Dennis series?\n>\n> Oh, I'm supposed to have a point to my emails? :)\n\nI am used to read high quality emails from you and asked myself what I can\nlearn by reading this email. \"Trying to figure out your point\", nothing more.\n\n>\n> I was mostly just musing. I do see the general idea as a useful thing;\n> there really is data that is related to the push itself, and not other\n> actions you may want to perform on the push.\n>\n> I'm not necessarily sure that we need a complicated data model (like\n> multiple values of the same key), or the ability to handle arbitrarily\n> large binary data. Those don't necessarily complicate the wire protocol\n> (I like the idea of adding a new phase of key/values, followed by a\n> flush),\n\nSo you suggest that we only allow key=value pairs and not \"key\" only?\nI am not sure if that makes it easier, though (what part is easier?) compared\nto completely free form.\n\n> but they do complicate the server implementation (how do we\n> communicate them to the hooks? Should there be limits for safety and DoS\n> protection on the server?)\n\nOh good point! How do you handle DoS protection for pushing large blobs,\nor weird history (that may blow up the before-behind implementation as\nreferenced\nin the \"topological index field for commit objects\" thread)\n\nShouldn't that be server specific as well?\n\nMind that I proposed a \"receive.advertisePushOptions\" config option.\nMaybe that can be fine tuned in a later patch \"receive.PushOptionSizeLimit\",\n\"receive.maxPushOptions\" or such?\n\n>\n>> Gits spirit (Oh no, not this discussion!) is to allow a broad range of\n>> uses and work flows. It doesn't hinder you from shooting into your\n>> feet if that's what you're into, and that is what this series does\n>> precisely.\n>\n> I think Git's spirit is also to fit as a tool into the greater\n> ecosystem, and not duplicate functionality that is done elsewhere. So\n> for example there is no authentication in the git:// protocol; that job\n> is done by ssh.\n\nor https and you have a cookie for that.\n\nTalking about authentication, any tool that we additionally have \"to\njust trigger\nthis one thing remotely\" would need to learn about proper auth, too. so it will\nnot be just a small helper script, but blows up into a large thing. I\nthink we could\navoid that duplicated effort, too.\n\n>\n> -Peff\n"},{"id":"290696","messageId":"CAGZ79kZqB7bs1pyo4sn-_L1sbVRu4hiyFoduYDyZQ62hi9bbCg@mail.gmail.com","threadId":"42752","inReplyTo":"20160701175950.GB16235@sigill.intra.peff.net","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-01T18:40:02Z","receivedAt":"2016-07-01T18:41:33Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jul 1, 2016 at 10:59 AM, Jeff King wrote:\n> On Fri, Jul 01, 2016 at 10:20:32AM -0700, Stefan Beller wrote:\n>\n>> >> The rationale for keeping the actual options inside a file instead of\n>> >> putting them directly into an environment variable has multiple reasons:\n>> >\n>> > Thanks for including this rationale; my first thought on seeing the\n>> > patch was \"wouldn't this be much more convenient for a hook if each\n>> > value had its own environment variable?\".\n>>\n>> That's what I thought as well. Office discussion ensued and I am still\n>> offended by this solution, but it sucks less than multiple environment variables\n>> (I tried writing a script to construct and evaluate the environment\n>> variables and\n>> that doesn't look nice)\n>\n> If you give up on having multiple incarnations of each variable, then I\n> think:\n>\n> GIT_PUSH_VAR_foo=value_for_foo\n> GIT_PUSH_VAR_bar=value_for_bar\n>\n> is quite elegant, and easy to use from hooks. It just cannot represent\n> multiple such \"foo\" variables.\n\nI see! Then you can have a single check for one feature\n\n if $GIT_PUSH_VAR_foo = \"value_for_foo\" ; then\n foo \"bar\"\n elif $GIT_PUSH_VAR_foo != \"\" ; then\n free_form_foo $GIT_PUSH_VAR_foo\n fi\n\nand no worries about parsing.\n\nMind that we now put a user provided thing into the variable again.\nWhich we may be fine with.\n\nThe question I still have is how much of enforcement we want to do?\nDoes the client reject a push option if it doesn't contain a '=', such that\nthe server doesn't try setting a weird \"GIT_PUSH_VAR_bar_baz\".\n\nMind that a different server may not handle these in environment variables,\nbut read it off the wire and handle it in memory.\n\n>\n>> If we did not have a GIT_PUSH_OPTIONS_COUNT and GIT_PUSH_OPTION_\n>> but rather GIT_PUSH_OPTIONS_VARIABLES that contains the other variables,\n>> it may be easier to handle, but whether you read from a file or evaluate the\n>> environment variable is only a minor step, the indirection is there anyway\n>> and this would be very close to what we have above.\n>\n> It makes the server implementation a bit uglier. You have to create the\n> temporary file, and you have to clean it up. What process is responsible\n> for cleaning up stale files? Obviously receive-pack would try to clean\n> up after itself, but what happens when it is \"kill -9\"'d, or the system\n> goes down, etc? We clean up stale tmp files like tmp_obj_* in git-gc; I\n> think we'd want something like that here.\n\nYeah that is one of the weaknesses with the file based solution.\n\n>\n> -Peff\n\n> Jeff writes:\n>> Junio writes:\n>> It still is not clear to me why the option to pass _COUNT and _VAR_ is\n>> rejected.\n> The \"count\" method gives you the flexibility to parse multiple keys as\n> lists, last-one-wins, or whatever scheme you want. But it also gives you\n> the _responsibility_ to do the parsing yourself, which is a pain when\n> you want to do the simple thing.\n\nWe could ship Git with a default parser in the example hook that takes care of\nthe responsibility in an opinionated way (like Git does with \"multiple\noptions are\nallowed\", and \"last option wins\").\n\nIf we do that, then the _COUNT method may be favorable?\n"},{"id":"290700","messageId":"xmqqr3bdxirw.fsf@gitster.mtv.corp.google.com","threadId":"42752","inReplyTo":"20160701181102.GA16695@sigill.intra.peff.net","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-01T19:25:39Z","receivedAt":"2016-07-01T19:25:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Compare to:\n>\n> if test \"$GIT_PUSH_VAR_force\" = true\n> ...\n\nOK. As this is a new feature, I guess it is not too bad if we tell\nusers that they cannot have duplicate keys in their data\nthey ask Git to transport. They can do the key1=val1 key2=val2\nnumbering themselves if that is really needed.\n\n"},{"id":"290701","messageId":"CAGZ79kYB8jej6VMsBACvAG_B-PyWAbe+tBQLXmF2nL8P+zX1KA@mail.gmail.com","threadId":"42752","inReplyTo":"xmqqr3bdxirw.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-01T19:31:23Z","receivedAt":"2016-07-01T19:31:29Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jul 1, 2016 at 12:25 PM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> Compare to:\n>>\n>> if test \"$GIT_PUSH_VAR_force\" = true\n>> ...\n>\n> OK. As this is a new feature, I guess it is not too bad if we tell\n> users that they cannot have duplicate keys in their data\n> they ask Git to transport. They can do the key1=val1 key2=val2\n> numbering themselves if that is really needed.\n>\n\nThe original use case for us was to send out email on your behalf, so\ngit push -o CC=user1@domain -o CC=user2@domain would not work well with\nthis backend approach as the client would already yell at us for duplicate\nkeys.\n\nInstead that has to be `git push -o CC=user1@domain,user2@domain\nand we do need to parse the email addresses apart ourselves.\n\nSo currently I'd favor the _COUNT thing and we deliver a reasonable\n_COUNT -> key/value parser/translator in the example hook.\n"},{"id":"290702","messageId":"20160701193909.GA23141@sigill.intra.peff.net","threadId":"42752","inReplyTo":"xmqqr3bdxirw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/4] push options: {pre,post}-receive hook learns about push options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-01T19:39:09Z","receivedAt":"2016-07-01T19:39:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 01, 2016 at 12:25:39PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Compare to:\n> >\n> > if test \"$GIT_PUSH_VAR_force\" = true\n> > ...\n> \n> OK. As this is a new feature, I guess it is not too bad if we tell\n> users that they cannot have duplicate keys in their data\n> they ask Git to transport. They can do the key1=val1 key2=val2\n> numbering themselves if that is really needed.\n\nThat was sort of my question. _Is it_ too bad or not? It's hard to say\nwithout knowing the use cases.\n\nI'm OK with the more flexible scheme if it's something people really\nwant to use. Though perhaps with your suggestion, we can have our cake\nand eat it, too. If we codify the key1/key2 thing by converting:\n\n git push --push-var=foo=one --push-var=foo=two\n\ninto:\n\n GIT_PUSH_VAR_foo=one\n GIT_PUSH_VAR_foo1=two\n\nthen a hook can either:\n\n - just use $GIT_PUSH_VAR_foo if they only know how to handle a single\n value\n\n - handle a list like:\n\n if test -n \"$GIT_PUSH_VAR_foo\"\n then\n # non-empty list\n echo first value is $GIT_PUSH_VAR_foo\n i=1\n while true; do\n eval \"test -z \\$GIT_PUSH_VAR_foo$i\" && break\n\t eval \"echo value \\$i is \\$GIT_PUSH_VAR_foo$i\"\n done\n fi\n\nThat's ugly, of course, but not really uglier than the parsing required\nfor the COUNT proposal.\n\nI'm assuming that git actually knows about and enforces that things are\n\"key=value\". I'm not sure how you'd get by without that (you'd have to\ninfer the meaning of a parameter by its position, which seems like a\nrecipe for mistakes and incompatibilities).\n\n-Peff\n"},{"id":"290704","messageId":"CAGZ79kaRG7SOcz1LyOkN8W_dpmK_AjwYyyMrZftZCac9fCo+EQ@mail.gmail.com","threadId":"42752","inReplyTo":"20160701193909.GA23141@sigill.intra.peff.net","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-01T19:50:09Z","receivedAt":"2016-07-01T19:50:18Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jul 1, 2016 at 12:39 PM, Jeff King wrote:\n>\n> That was sort of my question. _Is it_ too bad or not? It's hard to say\n> without knowing the use cases.\n>\n...\n>\n> That's ugly, of course, but not really uglier than the parsing required\n> for the COUNT proposal.\n\nok, I'll try to get the COUNT proposal in good enough shape.\n\n>\n> I'm assuming that git actually knows about and enforces that things are\n> \"key=value\". I'm not sure how you'd get by without that (you'd have to\n> infer the meaning of a parameter by its position, which seems like a\n> recipe for mistakes and incompatibilities).\n>\n\nOf course! We could use \"a hack\" just like in the Git config itself.\nIf a boolean parameter is not given, the default is assumed. If it is\ngiven the opposite is assumed. Same for attributes, that can be set\nby just giving the key (with no \"=true\").\n\nSo I think I can be convinced to go only with key/value pairs for now, but\nit would be easier if you'd just have\n\n git push -o draft ...\n"},{"id":"290708","messageId":"20160701200123.GB23141@sigill.intra.peff.net","threadId":"42752","inReplyTo":"CAGZ79kZx6=LGjQjzEEjMjZrNbXcVto-+djJbxj9xUCtLMsaL5w@mail.gmail.com","subject":"Re: [RFC PATCHv1 0/4] Push options in C Git","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-01T20:01:23Z","receivedAt":"2016-07-01T20:11:15Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 01, 2016 at 11:25:40AM -0700, Stefan Beller wrote:\n\n> > So a litmus test for me would be something like:\n> >\n> > Can you get the same effect with:\n> >\n> > git push $remote my-branch &&\n> > curl https://$remote/api/do-the-thing?q=my-branch\n> >\n> > as you would with:\n> >\n> > git push --push-data=do-the-thing $remote mybranch\n> \n> People are happy when they can use one tool instead of two,\n> e.g. \"Deploy with Git\" is a thing[1] and maybe they want to add a\n> switch \"once pushed skip the tests, I know they are broken\".\n> \n> [1] https://devcenter.heroku.com/articles/git\n\nYes, but people are also happy when they can use a flexible and\nstandardized tool to do a thing. I'd be more frustrated when I found out\nthat Git's data-pushing protocol has arbitrary limitations (like, say, I\ncan't push a data item larger than a single 64K pkt-line), which would\neasily just work with something like HTTP POSTs.\n\nI'm not saying my point trumps yours; just that there are a lot of\ntradeoffs in a design like this.\n\n> > With Gerrit, how do you refer to the commits you just pushed, when\n> > making a separate action? My impression is that Gerrit assigns\n> > change-ids centrally when you push to refs/for/master;\n> \n> actually you install a commit hook to do it decentralized.\n\nAh, OK. So you could do roughly the same thing:\n\n git push ... &&\n id=$(\n git cat-file commit HEAD |\n perl -lne '/^Change-Id: (.*)/ and print $1'\n ) &&\n curl https://$remote/api/do-something?id=$id\n\n(Yes, I know Gerrit doesn't necessarily have an API like that; I was\nmostly just wondering how it compared to the GitHub PR example I had\nlaid out).\n\n> I am used to read high quality emails from you and asked myself what I can\n> learn by reading this email. \"Trying to figure out your point\", nothing more.\n\nHeh. Sorry, they can't all be winners. ;)\n\n> So you suggest that we only allow key=value pairs and not \"key\" only?\n> I am not sure if that makes it easier, though (what part is easier?) compared\n> to completely free form.\n\nI had just assumed they would always have a key and a value. If you have\nno key, then you are stuck parsing positionally to know what each value\nmeans. If you have no value, you can treat each element like a boolean\ntrue. But it's not that much worse to turn \"foo\" into \"foo=true\".\n\n> > but they do complicate the server implementation (how do we\n> > communicate them to the hooks? Should there be limits for safety and\n> > DoS protection on the server?)\n> \n> Oh good point! How do you handle DoS protection for pushing large\n> blobs, or weird history (that may blow up the before-behind\n> implementation as referenced in the \"topological index field for\n> commit objects\" thread)\n\nAt GitHub we use a combination of push-time size heuristics and CPU\nrun-time limits.\n\nSo at push time, we reject objects larger than 100MB (uncompressed) in\nindex-pack, and we'll refuse to make a single allocation larger than a\ngigabyte or so (to prevent index-pack blowing up while trying to figure\nout how big the object is). We reject any single pack larger than 2GB.\nRejected objects never even make it to the repo, and we clean up the tmp\npacks immediately. So you can't just stream data at us endlessly and\nchew up disk or RAM (you can chew up plenty of CPU, though). \n\nThat prevents the most egregious DoS problems, and protects us from crap\nlike \"whoops, this tree entry has a 2GB name, and it wraps an integer\nsomewhere deep in git that probably should have been a size_t\".\n\nBut git is complicated enough that you can still get plenty of malicious\ncrap through. So we aggressively limit completion times for most\noperations, and we keep track of the runtime of every single git\noperation and correlate it with the repo, user, and source IP, and apply\nquotas when there's excessive use. That's not nice for a user who has a\nlegitimately big request, but it prevents them from impacting other\nusers (so it's a baseline, and then we try to optimize things that don't\nfit into the timeouts, or that cause users to have quotas applied).\n\nThat was a bit of a tangent, but back to this topic: I'd probably want\nsome kind of hard limit on the number of data items and the size of each\n(though if they are single pkt-lines, that puts a hard limit already).\n\n> Shouldn't that be server specific as well?\n\nPotentially, yes.\n\n> Mind that I proposed a \"receive.advertisePushOptions\" config option.\n> Maybe that can be fine tuned in a later patch \"receive.PushOptionSizeLimit\",\n> \"receive.maxPushOptions\" or such?\n\nYep, I think that is a good approach (presumably with some sane safe-ish\ndefaults).\n\n> Talking about authentication, any tool that we additionally have \"to\n> just trigger this one thing remotely\" would need to learn about proper\n> auth, too. so it will not be just a small helper script, but blows up\n> into a large thing. I think we could avoid that duplicated effort,\n> too.\n\nI think the right tool there is credential helpers. And I don't think\nit's a new problem. You already can log into Gerrit via the web browser,\nbut you have to have separate auth for running git. Hopefully there are\ntools to make that less painful (either pulling the browser cookie, or\ngetting an opaque application token from the web app to feed into git).\n\n-Peff\n"}]}