{"thread":{"id":"40083","subject":"[PATCH 3/7] Documentation/git-send-pack.txt: Document --signed","startedAt":"2015-08-13T19:00:44Z","lastAt":"2015-08-19T15:18:02Z","messageCount":32,"participants":["Dave Borowitz","Chris Packham","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"268016","messageId":"1439492451-11233-1-git-send-email-dborowitz@google.com","threadId":"40083","inReplyTo":null,"subject":"[PATCH 0/7] Flags and config to sign pushes by default","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-08-13T19:00:44Z","receivedAt":"2015-08-13T19:00:44Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"Remembering to pass --signed to git push on every push is extra typing that is\neasy to forget, and just leads to annoyance if the remote has a hook that makes\nsigned pushes required. Add a config option push.gpgSign, analogous to\ncommit.gpgSign, allowing users to set this flag by default.\n\nSince --signed push will simply fail on any remote that does not advertise a\npush cert nonce, actually setting this to true is not very useful (except for\nthe super-paranoid who would never want to push to a server that does not\nsupport signed pushes). So, add a third state to this boolean, \"if-possible\",\nto sign the push if and only if supported by the server. To keep parity between\nthe config and command line options, add a --signed-if-possible flag to git\npush as well.\n\nThe \"if-possible\" name and weird tri-state boolean is basically a straw man,\nand I am happy to change if someone has a clearer suggestion.\n\nDave Borowitz (7):\n  Documentation/git-push.txt: Document when --signed may fail\n  Documentation/git-send-pack.txt: Flow long synopsis line\n  Documentation/git-send-pack.txt: Document --signed\n  gitremote-helpers.txt: Document pushcert option\n  transport: Remove git_transport_options.push_cert\n  Support signing pushes iff the server supports it\n  Add a config option push.gpgSign for default signed pushes\n\n Documentation/config.txt            |  8 ++++++++\n Documentation/git-push.txt          | 11 +++++++++--\n Documentation/git-send-pack.txt     | 17 ++++++++++++++++-\n Documentation/gitremote-helpers.txt |  3 +++\n builtin/push.c                      | 26 +++++++++++++++++++++++++-\n builtin/send-pack.c                 | 33 +++++++++++++++++++++++++++++++--\n remote-curl.c                       | 14 ++++++++++----\n send-pack.c                         | 18 +++++++++++++++---\n send-pack.h                         |  8 +++++++-\n transport-helper.c                  | 34 +++++++++++++++++-----------------\n transport.c                         | 11 +++++++----\n transport.h                         |  6 +++---\n 12 files changed, 151 insertions(+), 38 deletions(-)\n\n-- \n2.5.0.276.gf5e568e\n"},{"id":"268013","messageId":"1439492451-11233-2-git-send-email-dborowitz@google.com","threadId":"40083","inReplyTo":"1439492451-11233-1-git-send-email-dborowitz@google.com","subject":"[PATCH 1/7] Documentation/git-push.txt: Document when --signed may fail","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-08-13T19:00:45Z","receivedAt":"2015-08-13T19:00:45Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"Like --atomic, --signed will fail if the server does not advertise the\nnecessary capability. In addition, it requires gpg on the client side.\n\nSigned-off-by: Dave Borowitz <dborowitz@google.com>\n---\n Documentation/git-push.txt | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex 135d810..f8b8b8b 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -137,7 +137,9 @@ already exists on the remote side.\n \tGPG-sign the push request to update refs on the receiving\n \tside, to allow it to be checked by the hooks and/or be\n \tlogged.  See linkgit:git-receive-pack[1] for the details\n-\ton the receiving end.\n+\ton the receiving end.  If the `gpg` executable is not available,\n+\tor if the server does not support signed pushes, the push will\n+\tfail.\n \n --[no-]atomic::\n \tUse an atomic transaction on the remote side if available.\n-- \n2.5.0.276.gf5e568e\n"},{"id":"268017","messageId":"1439492451-11233-3-git-send-email-dborowitz@google.com","threadId":"40083","inReplyTo":"1439492451-11233-1-git-send-email-dborowitz@google.com","subject":"[PATCH 2/7] Documentation/git-send-pack.txt: Flow long synopsis line","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-08-13T19:00:46Z","receivedAt":"2015-08-13T19:00:46Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"Signed-off-by: Dave Borowitz <dborowitz@google.com>\n---\n Documentation/git-send-pack.txt | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt\nindex b5d09f7..6affff6 100644\n--- a/Documentation/git-send-pack.txt\n+++ b/Documentation/git-send-pack.txt\n@@ -9,7 +9,8 @@ git-send-pack - Push objects over Git protocol to another repository\n SYNOPSIS\n --------\n [verse]\n-'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=<git-receive-pack>] [--verbose] [--thin] [--atomic] [<host>:]<directory> [<ref>...]\n+'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=<git-receive-pack>]\n+\t\t[--verbose] [--thin] [--atomic] [<host>:]<directory> [<ref>...]\n \n DESCRIPTION\n -----------\n-- \n2.5.0.276.gf5e568e\n"},{"id":"268010","messageId":"1439492451-11233-4-git-send-email-dborowitz@google.com","threadId":"40083","inReplyTo":"1439492451-11233-1-git-send-email-dborowitz@google.com","subject":"[PATCH 3/7] Documentation/git-send-pack.txt: Document --signed","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-08-13T19:00:47Z","receivedAt":"2015-08-13T19:00:47Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"Signed-off-by: Dave Borowitz <dborowitz@google.com>\n---\n Documentation/git-send-pack.txt | 11 ++++++++++-\n 1 file changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt\nindex 6affff6..dde13b0 100644\n--- a/Documentation/git-send-pack.txt\n+++ b/Documentation/git-send-pack.txt\n@@ -10,7 +10,8 @@ SYNOPSIS\n --------\n [verse]\n 'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=<git-receive-pack>]\n-\t\t[--verbose] [--thin] [--atomic] [<host>:]<directory> [<ref>...]\n+\t\t[--verbose] [--thin] [--atomic] [--signed]\n+\t\t[<host>:]<directory> [<ref>...]\n \n DESCRIPTION\n -----------\n@@ -68,6 +69,14 @@ be in a separate packet, and the list must end with a flush packet.\n \tfails to update then the entire push will fail without changing any\n \trefs.\n \n+--signed::\n+\tGPG-sign the push request to update refs on the receiving\n+\tside, to allow it to be checked by the hooks and/or be\n+\tlogged.  See linkgit:git-receive-pack[1] for the details\n+\ton the receiving end.  If the `gpg` executable is not available,\n+\tor if the server does not support signed pushes, the push will\n+\tfail.\n+\n <host>::\n \tA remote host to house the repository.  When this\n \tpart is specified, 'git-receive-pack' is invoked via\n-- \n2.5.0.276.gf5e568e\n"},{"id":"268014","messageId":"1439492451-11233-5-git-send-email-dborowitz@google.com","threadId":"40083","inReplyTo":"1439492451-11233-1-git-send-email-dborowitz@google.com","subject":"[PATCH 4/7] gitremote-helpers.txt: Document pushcert option","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-08-13T19:00:48Z","receivedAt":"2015-08-13T19:00:48Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"Signed-off-by: Dave Borowitz <dborowitz@google.com>\n---\n Documentation/gitremote-helpers.txt | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt\nindex 82e2d15..78e0b27 100644\n--- a/Documentation/gitremote-helpers.txt\n+++ b/Documentation/gitremote-helpers.txt\n@@ -448,6 +448,9 @@ set by Git if the remote helper has the 'option' capability.\n 'option update-shallow {'true'|'false'}::\n \tAllow to extend .git/shallow if the new refs require it.\n \n+'option pushcert {'true'|'false'}::\n+\tGPG sign pushes.\n+\n SEE ALSO\n --------\n linkgit:git-remote[1]\n-- \n2.5.0.276.gf5e568e\n"},{"id":"268015","messageId":"1439492451-11233-6-git-send-email-dborowitz@google.com","threadId":"40083","inReplyTo":"1439492451-11233-1-git-send-email-dborowitz@google.com","subject":"[PATCH 5/7] transport: Remove git_transport_options.push_cert","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-08-13T19:00:49Z","receivedAt":"2015-08-13T19:00:49Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"This field was set in transport_set_option, but never read in the push\ncode. The push code basically ignores the smart_options field\nentirely, and derives its options from the flags arguments to the\npush* callbacks. Note that in git_transport_push there are already\nseveral args set from flags that have no corresponding field in\ngit_transport_options; after this change, push_cert is just like\nthose.\n\nSigned-off-by: Dave Borowitz <dborowitz@google.com>\n---\n transport.c | 3 ---\n transport.h | 1 -\n 2 files changed, 4 deletions(-)\n\ndiff --git a/transport.c b/transport.c\nindex 40692f8..3dd6e30 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -476,9 +476,6 @@ static int set_git_option(struct git_transport_options *opts,\n \t\t\t\tdie(\"transport: invalid depth option '%s'\", value);\n \t\t}\n \t\treturn 0;\n-\t} else if (!strcmp(name, TRANS_OPT_PUSH_CERT)) {\n-\t\topts->push_cert = !!value;\n-\t\treturn 0;\n \t}\n \treturn 1;\n }\ndiff --git a/transport.h b/transport.h\nindex 18d2cf8..79190df 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -12,7 +12,6 @@ struct git_transport_options {\n \tunsigned check_self_contained_and_connected : 1;\n \tunsigned self_contained_and_connected : 1;\n \tunsigned update_shallow : 1;\n-\tunsigned push_cert : 1;\n \tint depth;\n \tconst char *uploadpack;\n \tconst char *receivepack;\n-- \n2.5.0.276.gf5e568e\n"},{"id":"268011","messageId":"1439492451-11233-7-git-send-email-dborowitz@google.com","threadId":"40083","inReplyTo":"1439492451-11233-1-git-send-email-dborowitz@google.com","subject":"[PATCH 6/7] Support signing pushes iff the server supports it","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-08-13T19:00:50Z","receivedAt":"2015-08-13T19:00:50Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"Add a new flag --signed-if-possible to push and send-pack that sends a\npush certificate if and only if the server advertised a push cert\nnonce. If not, at least warn the user that their push may not be as\nsecure as they thought.\n\nSigned-off-by: Dave Borowitz <dborowitz@google.com>\n---\n Documentation/git-push.txt      |  9 +++++++--\n Documentation/git-send-pack.txt |  9 +++++++--\n builtin/push.c                  |  4 +++-\n builtin/send-pack.c             |  6 +++++-\n remote-curl.c                   | 14 ++++++++++----\n send-pack.c                     | 18 +++++++++++++++---\n send-pack.h                     |  8 +++++++-\n transport-helper.c              | 34 +++++++++++++++++-----------------\n transport.c                     |  8 +++++++-\n transport.h                     |  5 +++--\n 10 files changed, 81 insertions(+), 34 deletions(-)\n\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex f8b8b8b..fcfdf73 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n [verse]\n 'git push' [--all | --mirror | --tags] [--follow-tags] [--atomic] [-n | --dry-run] [--receive-pack=<git-receive-pack>]\n \t   [--repo=<repository>] [-f | --force] [--prune] [-v | --verbose]\n-\t   [-u | --set-upstream] [--signed]\n+\t   [-u | --set-upstream] [--signed] [--signed-if-possible]\n \t   [--force-with-lease[=<refname>[:<expect>]]]\n \t   [--no-verify] [<repository> [<refspec>...]]\n \n@@ -139,7 +139,12 @@ already exists on the remote side.\n \tlogged.  See linkgit:git-receive-pack[1] for the details\n \ton the receiving end.  If the `gpg` executable is not available,\n \tor if the server does not support signed pushes, the push will\n-\tfail.\n+\tfail. Takes precedence over --signed-if-possible.\n+\n+--signed-if-possible::\n+\tLike --signed, but only if the server supports signed pushes. If\n+\tthe server supports signed pushes but the `gpg` is not available,\n+\tthe push will fail.\n \n --[no-]atomic::\n \tUse an atomic transaction on the remote side if available.\ndiff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt\nindex dde13b0..5789208 100644\n--- a/Documentation/git-send-pack.txt\n+++ b/Documentation/git-send-pack.txt\n@@ -10,7 +10,7 @@ SYNOPSIS\n --------\n [verse]\n 'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=<git-receive-pack>]\n-\t\t[--verbose] [--thin] [--atomic] [--signed]\n+\t\t[--verbose] [--thin] [--atomic] [--signed] [--signed-if-possible]\n \t\t[<host>:]<directory> [<ref>...]\n \n DESCRIPTION\n@@ -75,7 +75,12 @@ be in a separate packet, and the list must end with a flush packet.\n \tlogged.  See linkgit:git-receive-pack[1] for the details\n \ton the receiving end.  If the `gpg` executable is not available,\n \tor if the server does not support signed pushes, the push will\n-\tfail.\n+\tfail. Takes precedence over --signed-if-possible.\n+\n+--signed-if-possible::\n+\tLike --signed, but only if the server supports signed pushes. If\n+\tthe server supports signed pushes but the `gpg` is not available,\n+\tthe push will fail.\n \n <host>::\n \tA remote host to house the repository.  When this\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 57c138b..95a67c5 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -526,7 +526,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT(0, \"no-verify\", &flags, N_(\"bypass pre-push hook\"), TRANSPORT_PUSH_NO_HOOK),\n \t\tOPT_BIT(0, \"follow-tags\", &flags, N_(\"push missing but relevant tags\"),\n \t\t\tTRANSPORT_PUSH_FOLLOW_TAGS),\n-\t\tOPT_BIT(0, \"signed\", &flags, N_(\"GPG sign the push\"), TRANSPORT_PUSH_CERT),\n+\t\tOPT_BIT(0, \"signed\", &flags, N_(\"GPG sign the push\"), TRANSPORT_PUSH_CERT_ALWAYS),\n+\t\tOPT_BIT(0, \"signed-if-possible\", &flags, N_(\"GPG sign the push, if supported by the server\"),\n+\t\t\tTRANSPORT_PUSH_CERT_IF_POSSIBLE),\n \t\tOPT_BIT(0, \"atomic\", &flags, N_(\"request atomic transaction on remote side\"), TRANSPORT_PUSH_ATOMIC),\n \t\tOPT_END()\n \t};\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 23b2962..8eebbf4 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -158,7 +158,11 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--signed\")) {\n-\t\t\t\targs.push_cert = 1;\n+\t\t\t\targs.push_cert = SEND_PACK_PUSH_CERT_ALWAYS;\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\tif (!strcmp(arg, \"--signed-if-possible\")) {\n+\t\t\t\targs.push_cert = SEND_PACK_PUSH_CERT_IF_POSSIBLE;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--progress\")) {\ndiff --git a/remote-curl.c b/remote-curl.c\nindex af7b678..f049de8 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -11,6 +11,7 @@\n #include \"argv-array.h\"\n #include \"credential.h\"\n #include \"sha1-array.h\"\n+#include \"send-pack.h\"\n \n static struct remote *remote;\n /* always ends with a trailing slash */\n@@ -26,7 +27,8 @@ struct options {\n \t\tfollowtags : 1,\n \t\tdry_run : 1,\n \t\tthin : 1,\n-\t\tpush_cert : 1;\n+\t\t/* One of the SEND_PACK_PUSH_CERT_* constants. */\n+\t\tpush_cert : 2;\n };\n static struct options options;\n static struct string_list cas_options = STRING_LIST_INIT_DUP;\n@@ -109,9 +111,11 @@ static int set_option(const char *name, const char *value)\n \t\treturn 0;\n \t} else if (!strcmp(name, \"pushcert\")) {\n \t\tif (!strcmp(value, \"true\"))\n-\t\t\toptions.push_cert = 1;\n+\t\t\toptions.push_cert = SEND_PACK_PUSH_CERT_ALWAYS;\n \t\telse if (!strcmp(value, \"false\"))\n-\t\t\toptions.push_cert = 0;\n+\t\t\toptions.push_cert = SEND_PACK_PUSH_CERT_NEVER;\n+\t\telse if (!strcmp(value, \"if-possible\"))\n+\t\t\toptions.push_cert = SEND_PACK_PUSH_CERT_IF_POSSIBLE;\n \t\telse\n \t\t\treturn -1;\n \t\treturn 0;\n@@ -880,8 +884,10 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)\n \t\targv_array_push(&args, \"--thin\");\n \tif (options.dry_run)\n \t\targv_array_push(&args, \"--dry-run\");\n-\tif (options.push_cert)\n+\tif (options.push_cert == SEND_PACK_PUSH_CERT_ALWAYS)\n \t\targv_array_push(&args, \"--signed\");\n+\telse if (options.push_cert == SEND_PACK_PUSH_CERT_IF_POSSIBLE)\n+\t\targv_array_push(&args, \"--signed-if-possible\");\n \tif (options.verbosity == 0)\n \t\targv_array_push(&args, \"--quiet\");\n \telse if (options.verbosity > 1)\ndiff --git a/send-pack.c b/send-pack.c\nindex 2a64fec..6ae9f45 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -370,7 +370,7 @@ 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 (args->push_cert) {\n+\tif (args->push_cert == SEND_PACK_PUSH_CERT_ALWAYS) {\n \t\tint len;\n \n \t\tpush_cert_nonce = server_feature_value(\"push-cert\", &len);\n@@ -379,6 +379,18 @@ int send_pack(struct send_pack_args *args,\n \t\treject_invalid_nonce(push_cert_nonce, len);\n \t\tpush_cert_nonce = xmemdupz(push_cert_nonce, len);\n \t}\n+\tif (args->push_cert == SEND_PACK_PUSH_CERT_IF_POSSIBLE) {\n+\t\tint len;\n+\n+\t\tpush_cert_nonce = server_feature_value(\"push-cert\", &len);\n+\t\tif (push_cert_nonce) {\n+\t\t\treject_invalid_nonce(push_cert_nonce, len);\n+\t\t\tpush_cert_nonce = xmemdupz(push_cert_nonce, len);\n+\t\t} else\n+\t\t\twarning(_(\"not sending a push certificate since the\"\n+\t\t\t\t  \" receiving end does not support --signed\"\n+\t\t\t\t  \" push\"));\n+\t}\n \n \tif (!remote_refs) {\n \t\tfprintf(stderr, \"No refs in common and none specified; doing nothing.\\n\"\n@@ -413,7 +425,7 @@ int send_pack(struct send_pack_args *args,\n \tif (!args->dry_run)\n \t\tadvertise_shallow_grafts_buf(&req_buf);\n \n-\tif (!args->dry_run && args->push_cert)\n+\tif (!args->dry_run && push_cert_nonce)\n \t\tcmds_sent = generate_push_cert(&req_buf, remote_refs, args,\n \t\t\t\t\t       cap_buf.buf, push_cert_nonce);\n \n@@ -452,7 +464,7 @@ int send_pack(struct send_pack_args *args,\n \tfor (ref = remote_refs; ref; ref = ref->next) {\n \t\tchar *old_hex, *new_hex;\n \n-\t\tif (args->dry_run || args->push_cert)\n+\t\tif (args->dry_run || push_cert_nonce)\n \t\t\tcontinue;\n \n \t\tif (check_to_send_update(ref, args) < 0)\ndiff --git a/send-pack.h b/send-pack.h\nindex b664648..5042b65 100644\n--- a/send-pack.h\n+++ b/send-pack.h\n@@ -1,6 +1,11 @@\n #ifndef SEND_PACK_H\n #define SEND_PACK_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_POSSIBLE 1\n+#define SEND_PACK_PUSH_CERT_ALWAYS 2\n+\n struct send_pack_args {\n \tconst char *url;\n \tunsigned verbose:1,\n@@ -12,7 +17,8 @@ struct send_pack_args {\n \t\tuse_thin_pack:1,\n \t\tuse_ofs_delta:1,\n \t\tdry_run:1,\n-\t\tpush_cert:1,\n+\t\t/* One of the SEND_PACK_PUSH_CERT_* constants. */\n+\t\tpush_cert:2,\n \t\tstateless_rpc:1,\n \t\tatomic:1;\n };\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 5d99a6b..bf4dd22 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -257,7 +257,6 @@ static const char *boolean_options[] = {\n \tTRANS_OPT_THIN,\n \tTRANS_OPT_KEEP,\n \tTRANS_OPT_FOLLOWTAGS,\n-\tTRANS_OPT_PUSH_CERT\n \t};\n \n static int set_helper_option(struct transport *transport,\n@@ -763,6 +762,21 @@ static int push_update_refs_status(struct helper_data *data,\n \treturn ret;\n }\n \n+static void set_common_push_options(struct transport *transport,\n+\t\t\t\t   const char *name, int flags)\n+{\n+\tif (flags & TRANSPORT_PUSH_DRY_RUN) {\n+\t\tif (set_helper_option(transport, \"dry-run\", \"true\") != 0)\n+\t\t\tdie(\"helper %s does not support dry-run\", name);\n+\t} else if (flags & TRANSPORT_PUSH_CERT_ALWAYS) {\n+\t\tif (set_helper_option(transport, TRANS_OPT_PUSH_CERT, \"true\") != 0)\n+\t\t\tdie(\"helper %s does not support --signed\", name);\n+\t} else if (flags & TRANSPORT_PUSH_CERT_IF_POSSIBLE) {\n+\t\tif (set_helper_option(transport, TRANS_OPT_PUSH_CERT, \"if-possible\") != 0)\n+\t\t\tdie(\"helper %s does not support --signed-if-possible\", name);\n+\t}\n+}\n+\n static int push_refs_with_push(struct transport *transport,\n \t\t\t       struct ref *remote_refs, int flags)\n {\n@@ -830,14 +844,7 @@ static int push_refs_with_push(struct transport *transport,\n \n \tfor_each_string_list_item(cas_option, &cas_options)\n \t\tset_helper_option(transport, \"cas\", cas_option->string);\n-\n-\tif (flags & TRANSPORT_PUSH_DRY_RUN) {\n-\t\tif (set_helper_option(transport, \"dry-run\", \"true\") != 0)\n-\t\t\tdie(\"helper %s does not support dry-run\", data->name);\n-\t} else if (flags & TRANSPORT_PUSH_CERT) {\n-\t\tif (set_helper_option(transport, TRANS_OPT_PUSH_CERT, \"true\") != 0)\n-\t\t\tdie(\"helper %s does not support --signed\", data->name);\n-\t}\n+\tset_common_push_options(transport, data->name, flags);\n \n \tstrbuf_addch(&buf, '\\n');\n \tsendline(data, &buf);\n@@ -858,14 +865,7 @@ static int push_refs_with_export(struct transport *transport,\n \tif (!data->refspecs)\n \t\tdie(\"remote-helper doesn't support push; refspec needed\");\n \n-\tif (flags & TRANSPORT_PUSH_DRY_RUN) {\n-\t\tif (set_helper_option(transport, \"dry-run\", \"true\") != 0)\n-\t\t\tdie(\"helper %s does not support dry-run\", data->name);\n-\t} else if (flags & TRANSPORT_PUSH_CERT) {\n-\t\tif (set_helper_option(transport, TRANS_OPT_PUSH_CERT, \"true\") != 0)\n-\t\t\tdie(\"helper %s does not support --signed\", data->name);\n-\t}\n-\n+\tset_common_push_options(transport, data->name, flags);\n \tif (flags & TRANSPORT_PUSH_FORCE) {\n \t\tif (set_helper_option(transport, \"force\", \"true\") != 0)\n \t\t\twarning(\"helper %s does not support 'force'\", data->name);\ndiff --git a/transport.c b/transport.c\nindex 3dd6e30..66cd2d3 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -826,10 +826,16 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n \targs.progress = transport->progress;\n \targs.dry_run = !!(flags & TRANSPORT_PUSH_DRY_RUN);\n \targs.porcelain = !!(flags & TRANSPORT_PUSH_PORCELAIN);\n-\targs.push_cert = !!(flags & TRANSPORT_PUSH_CERT);\n \targs.atomic = !!(flags & TRANSPORT_PUSH_ATOMIC);\n \targs.url = transport->url;\n \n+\tif (flags & TRANSPORT_PUSH_CERT_ALWAYS)\n+\t\targs.push_cert = SEND_PACK_PUSH_CERT_ALWAYS;\n+\telse if (flags & TRANSPORT_PUSH_CERT_IF_POSSIBLE)\n+\t\targs.push_cert = SEND_PACK_PUSH_CERT_IF_POSSIBLE;\n+\telse\n+\t\targs.push_cert = SEND_PACK_PUSH_CERT_NEVER;\n+\n \tret = send_pack(&args, data->fd, data->conn, remote_refs,\n \t\t\t&data->extra_have);\n \ndiff --git a/transport.h b/transport.h\nindex 79190df..49f3e44 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -123,8 +123,9 @@ struct transport {\n #define TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND 256\n #define TRANSPORT_PUSH_NO_HOOK 512\n #define TRANSPORT_PUSH_FOLLOW_TAGS 1024\n-#define TRANSPORT_PUSH_CERT 2048\n-#define TRANSPORT_PUSH_ATOMIC 4096\n+#define TRANSPORT_PUSH_CERT_ALWAYS 2048\n+#define TRANSPORT_PUSH_CERT_IF_POSSIBLE 4096\n+#define TRANSPORT_PUSH_ATOMIC 8192\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.5.0.276.gf5e568e\n"},{"id":"268012","messageId":"1439492451-11233-8-git-send-email-dborowitz@google.com","threadId":"40083","inReplyTo":"1439492451-11233-1-git-send-email-dborowitz@google.com","subject":"[PATCH 7/7] Add a config option push.gpgSign for default signed pushes","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-08-13T19:00:51Z","receivedAt":"2015-08-13T19:00:51Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"---\n Documentation/config.txt |  8 ++++++++\n builtin/push.c           | 22 ++++++++++++++++++++++\n builtin/send-pack.c      | 27 ++++++++++++++++++++++++++-\n 3 files changed, 56 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 016f6e9..6804f5b 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2178,6 +2178,14 @@ push.followTags::\n \tmay override this configuration at time of push by specifying\n \t'--no-follow-tags'.\n \n+push.gpgSign::\n+\tMay be set to a boolean value, or the string 'if-possible'. A\n+\ttrue value causes all pushes to be GPG signed, as if '--signed'\n+\tis passed to linkgit:git-push[1]. The string 'if-possible'\n+\tcauses pushes to be signed if the server supports it, as if\n+\t'--signed-if-possible' is passed to 'git push'. A false value\n+\tmay override a value from a lower-priority config file. An\n+\texplicit command-line flag always overrides this config option.\n \n rebase.stat::\n \tWhether to show a diffstat of what changed upstream since the last\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 95a67c5..8972193 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -491,6 +491,26 @@ static int git_push_config(const char *k, const char *v, void *cb)\n \treturn git_default_config(k, v, NULL);\n }\n \n+static void set_push_cert_flags_from_config(int *flags)\n+{\n+\tconst char *value;\n+\t/* Ignore config if flags were set from command line. */\n+\tif (*flags & (TRANSPORT_PUSH_CERT_ALWAYS | TRANSPORT_PUSH_CERT_IF_POSSIBLE))\n+\t\treturn;\n+\tif (!git_config_get_value(\"push.gpgsign\", &value)) {\n+\t\tswitch (git_config_maybe_bool(\"push.gpgsign\", value)) {\n+\t\tcase 1:\n+\t\t\t*flags |= TRANSPORT_PUSH_CERT_ALWAYS;\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tif (value && !strcmp(value, \"if-possible\"))\n+\t\t\t\t*flags |= TRANSPORT_PUSH_CERT_IF_POSSIBLE;\n+\t\t\telse\n+\t\t\t\tdie(_(\"Invalid value for 'push.gpgsign'\"));\n+\t\t}\n+\t}\n+}\n+\n int cmd_push(int argc, const char **argv, const char *prefix)\n {\n \tint flags = 0;\n@@ -537,6 +557,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \tgit_config(git_push_config, &flags);\n \targc = parse_options(argc, argv, prefix, options, push_usage, 0);\n \n+\tset_push_cert_flags_from_config(&flags);\n+\n \tif (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))\n \t\tdie(_(\"--delete is incompatible with --all, --mirror and --tags\"));\n \tif (deleterefs && argc < 2)\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 8eebbf4..9c8b7de 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -92,6 +92,31 @@ static void print_helper_status(struct ref *ref)\n \tstrbuf_release(&buf);\n }\n \n+static int send_pack_config(const char *k, const char *v, void *cb)\n+{\n+\tgit_gpg_config(k, v, NULL);\n+\n+\tif (!strcmp(k, \"push.gpgsign\")) {\n+\t\tconst char *value;\n+\t\tif (!git_config_get_value(\"push.gpgsign\", &value)) {\n+\t\t\tswitch (git_config_maybe_bool(\"push.gpgsign\", value)) {\n+\t\t\tcase 0:\n+\t\t\t\targs.push_cert = SEND_PACK_PUSH_CERT_NEVER;\n+\t\t\t\tbreak;\n+\t\t\tcase 1:\n+\t\t\t\targs.push_cert = SEND_PACK_PUSH_CERT_ALWAYS;\n+\t\t\t\tbreak;\n+\t\t\tdefault:\n+\t\t\t\tif (value && !strcasecmp(value, \"if-possible\"))\n+\t\t\t\t\targs.push_cert = SEND_PACK_PUSH_CERT_IF_POSSIBLE;\n+\t\t\t\telse\n+\t\t\t\t\treturn error(\"Invalid value for '%s'\", k);\n+\t\t\t}\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n+\n int cmd_send_pack(int argc, const char **argv, const char *prefix)\n {\n \tint i, nr_refspecs = 0;\n@@ -114,7 +139,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \tint from_stdin = 0;\n \tstruct push_cas_option cas = {0};\n \n-\tgit_config(git_gpg_config, NULL);\n+\tgit_config(send_pack_config, NULL);\n \n \targv++;\n \tfor (i = 1; i < argc; i++, argv++) {\n-- \n2.5.0.276.gf5e568e\n"},{"id":"268035","messageId":"CAFOYHZAjvhOJQ1jLvVgOtbWLXfhn-XfKsFuKGR-68_E2D=ASRA@mail.gmail.com","threadId":"40083","inReplyTo":"1439492451-11233-1-git-send-email-dborowitz@google.com","subject":"Re: [PATCH 0/7] Flags and config to sign pushes by default","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2015-08-14T11:47:16Z","receivedAt":"2015-08-14T11:47:16Z","isPatch":true,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"Bike shedding a little (I've never used the signed push functionality)\n\nOn Fri, Aug 14, 2015 at 7:00 AM, Dave Borowitz <dborowitz@google.com> wrote:\n> The \"if-possible\" name and weird tri-state boolean is basically a straw man,\n> and I am happy to change if someone has a clearer suggestion.\n\nwhat about git push --signed={always|if-possible} defaulting to\n\"always\" to be backwards compatible. You might also want to add\n--no-signed or --signed=no to override your new config option\n"},{"id":"268057","messageId":"xmqqbne9ivry.fsf@gitster.dls.corp.google.com","threadId":"40083","inReplyTo":"1439492451-11233-1-git-send-email-dborowitz@google.com","subject":"Re: [PATCH 0/7] Flags and config to sign pushes by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-14T18:12:49Z","receivedAt":"2015-08-14T18:12:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Borowitz <dborowitz@google.com> writes:\n\n> Remembering to pass --signed to git push on every push is extra typing that is\n> easy to forget, and just leads to annoyance if the remote has a hook that makes\n> signed pushes required. Add a config option push.gpgSign, analogous to\n> commit.gpgSign, allowing users to set this flag by default.\n>\n> Since --signed push will simply fail on any remote that does not advertise a\n> push cert nonce, actually setting this to true is not very useful (except for\n> the super-paranoid who would never want to push to a server that does not\n> support signed pushes). So, add a third state to this boolean, \"if-possible\",\n> to sign the push if and only if supported by the server. To keep parity between\n> the config and command line options, add a --signed-if-possible flag to git\n> push as well.\n>\n> The \"if-possible\" name and weird tri-state boolean is basically a straw man,\n> and I am happy to change if someone has a clearer suggestion.\n\nYes, it looks somewhat strange.  Let me go on a slight tangent to\nexplain why I think it is OK for \"push --signed\".\n\nFirst imagine the case where we were talking an optional setting for\n\"git tag -a\" and what our reaction would be.\n\nBecause the reason \"git tag -s\" would fail when \"git tag -a\" can\nsucceed can only be because you do not have a working GPG set-up\n(i.e. correctly built and installed GPG with a usable key of your\nown to sign), I would say \"please sign this tag if I can, but do not\nbother failing, an unsigned annotated tag is also OK for me\" is not\na sensible request.  In such a case, you'd better get your act\ntogether and make yourself ready to sign before doing the \"tag -s\"\nthing.  Otherwise you'd never get around to do it.  So I'd say \"git\ntag --sign-if-possible\" would not make sense.\n\nBut \"push --signed\" can fail even if you have a perfectly good\nGPG set-up.  It will not succeed until the receiving end becomes\nready to accept a signed push, and often you would not be in control\nof the receiving end.\n\nMore importantly, the meaning and the purose of the GPG signature in\nsigned tags and signed pushes are vastly different.  In the former\ncase, you are attesting that the signed objects were made by you to\nhelp yourself.  If somebody else created a tag or a commit and\nclaimed it is from you, you can say \"that signature does not match,\nit is not mine\".\n\nBut \"signed push\" is not about helping you.  It is about helping the\nreceiving end by allowing them to be more credible when they say\n\"This is what David Borowitz said he wanted to put at the tip of\nthis branch\" to other people.  Currently, they can only make a weak\nclaim \"Well, you know, the push was made after we authenticated a\npusher with our own authentication methond, and here is the log that\nsays the pusher was David\".  The log entries could be faked, and the\ngeneral public cannot audit.  With a signed push, they can say \"Here\nis the push certificate, dated and signed by David Borowitz\", and\nthe general public can check without trusting the receiving end\n(i.e. hosting site).  If the receiving end does not offer signed\npushes, it just means that they are not ready to be helped by you,\nand you should have the option of pushing without helping them,\nwhich is what your \"if-possible\" is about.\n\nBecause of the above reasoning, I think a weaker \"I want to do a\nsigned push if the recipient is capable of accepting one, but\notherwise just pushing there is OK\" is a perfectly reasonable\nrequest.\n\nSo I am fine as long as \"if-possible\" turns a failure to make signed\npush into a success _only_ when the reason of the failure is because\nwe did not see the capability supported by the receiving end.  If\nthe reason why you cannot do a signed push is because you cannot\nsign push certificate, \"if-possible\" should still fail.\n\nThanks.\n"},{"id":"268073","messageId":"CAD0k6qRAG96a=zhTpw7nta6QjK7gEYTfYvMvCeob35LLzKkKYw@mail.gmail.com","threadId":"40083","inReplyTo":"xmqqbne9ivry.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/7] Flags and config to sign pushes by default","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-08-14T20:29:16Z","receivedAt":"2015-08-14T20:29:16Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Fri, Aug 14, 2015 at 2:12 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> So I am fine as long as \"if-possible\" turns a failure to make signed\n> push into a success _only_ when the reason of the failure is because\n> we did not see the capability supported by the receiving end.  If\n> the reason why you cannot do a signed push is because you cannot\n> sign push certificate, \"if-possible\" should still fail.\n\nI completely agree with your reasoning, and that's exactly how I\nimplemented and documented --signed-if-possible.\n\n>From patch 6/7:\n+--signed-if-possible::\n+       Like --signed, but only if the server supports signed pushes. If\n+       the server supports signed pushes but the `gpg` is not available,\n+       the push will fail.\n"},{"id":"268075","messageId":"CAD0k6qSjZW-5eMw-OOHP0cGdj08PesdKVgE9OAFvESwCueyH6w@mail.gmail.com","threadId":"40083","inReplyTo":"xmqqbne9ivry.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/7] Flags and config to sign pushes by default","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-08-14T20:31:50Z","receivedAt":"2015-08-14T20:31:50Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Fri, Aug 14, 2015 at 2:12 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> The \"if-possible\" name and weird tri-state boolean is basically a straw man,\n>> and I am happy to change if someone has a clearer suggestion.\n>\n> Yes, it looks somewhat strange.  Let me go on a slight tangent to\n> explain why I think it is OK for \"push --signed\".\n\nI think we agree that there are three possible behaviors for push and\nwe should allow the user to specify any of the three. The straw-man\nstrangeness is that two of them are the traditional boolean values\n\"true/false\" and the third is \"file not found^W^W^Wif-possible\" :)\n"},{"id":"268077","messageId":"xmqqwpwxha4r.fsf@gitster.dls.corp.google.com","threadId":"40083","inReplyTo":"CAD0k6qSjZW-5eMw-OOHP0cGdj08PesdKVgE9OAFvESwCueyH6w@mail.gmail.com","subject":"Re: [PATCH 0/7] Flags and config to sign pushes by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-14T20:45:40Z","receivedAt":"2015-08-14T20:45:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Borowitz <dborowitz@google.com> writes:\n\n> On Fri, Aug 14, 2015 at 2:12 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Yes, it looks somewhat strange.\n> ... The straw-man\n> strangeness is that two of them are the traditional boolean values\n> \"true/false\" and the third is \"file not found^W^W^Wif-possible\" :)\n\nIt actually is not uncommon for a Git configuration variable to\nstart its life as a boolean and then later become tristate (or more)\nas we gain experience with the system, so don't worry about it being\n\"strange\".  A tristate, among whose choices two of them are true and\nfalse, is not \"strange\" around here.\n\nBy \"strange\", I was referring to the possible perception issue on\nhaving a choice other than yes/no for a configuration that allows\nyou to express your security preference.\n\nThanks.\n"},{"id":"268085","messageId":"CAD0k6qR2HkHHYu8429mvdvN1bkLeTpD-5EbO4Mt+o69rC+P6aQ@mail.gmail.com","threadId":"40083","inReplyTo":"xmqqwpwxha4r.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/7] Flags and config to sign pushes by default","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-08-14T20:55:03Z","receivedAt":"2015-08-14T20:55:03Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Fri, Aug 14, 2015 at 4:45 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Dave Borowitz <dborowitz@google.com> writes:\n>\n>> On Fri, Aug 14, 2015 at 2:12 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> Yes, it looks somewhat strange.\n>> ... The straw-man\n>> strangeness is that two of them are the traditional boolean values\n>> \"true/false\" and the third is \"file not found^W^W^Wif-possible\" :)\n>\n> It actually is not uncommon for a Git configuration variable to\n> start its life as a boolean and then later become tristate (or more)\n> as we gain experience with the system, so don't worry about it being\n> \"strange\".  A tristate, among whose choices two of them are true and\n> false, is not \"strange\" around here.\n\nOk, so let us bikeshed a bit further.\n\nBikeshed 1.\nOption A: --signed/--no-signed--signed-if-possible\nOption B: --signed=true|false|if-possible, \"--signed\" alone implies \"=true\".\n\nBikeshed 2.\n\nOption A: if-possible\n\nThe possibly confusing thing is one might interpret missing \"gpg\" to\nmean \"impossible\", i.e. \"if gpg is not installed don't attempt to\nsign\", which is not the behavior we want.\n\nI don't have another succinct way of saying this.\n\"if-server-supported\" is a mouthful. I think Jonathan mentioned\n\"opportunistic\", which is fairly opaque.\n\n\n> By \"strange\", I was referring to the possible perception issue on\n> having a choice other than yes/no for a configuration that allows\n> you to express your security preference.\n>\n> Thanks.\n"},{"id":"268088","messageId":"xmqqk2sxh9ax.fsf@gitster.dls.corp.google.com","threadId":"40083","inReplyTo":"CAD0k6qR2HkHHYu8429mvdvN1bkLeTpD-5EbO4Mt+o69rC+P6aQ@mail.gmail.com","subject":"Re: [PATCH 0/7] Flags and config to sign pushes by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-14T21:03:34Z","receivedAt":"2015-08-14T21:03:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Borowitz <dborowitz@google.com> writes:\n\n> Ok, so let us bikeshed a bit further.\n>\n> Bikeshed 1.\n> Option A: --signed/--no-signed--signed-if-possible\n> Option B: --signed=true|false|if-possible, \"--signed\" alone implies \"=true\".\n>\n> Bikeshed 2.\n>\n> Option A: if-possible\n>\n> The possibly confusing thing is one might interpret missing \"gpg\" to\n> mean \"impossible\", i.e. \"if gpg is not installed don't attempt to\n> sign\", which is not the behavior we want.\n>\n> I don't have another succinct way of saying this.\n> \"if-server-supported\" is a mouthful. I think Jonathan mentioned\n> \"opportunistic\", which is fairly opaque.\n\nI would call what we agreed is a good behaviour during this\ndiscussion \"--sign-if-asked\".\n"},{"id":"268108","messageId":"xmqqr3n5fovm.fsf@gitster.dls.corp.google.com","threadId":"40083","inReplyTo":"1439492451-11233-2-git-send-email-dborowitz@google.com","subject":"Re: [PATCH 1/7] Documentation/git-push.txt: Document when --signed may fail","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-14T23:10:05Z","receivedAt":"2015-08-14T23:10:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Borowitz <dborowitz@google.com> writes:\n\n> Like --atomic, --signed will fail if the server does not advertise the\n> necessary capability. In addition, it requires gpg on the client side.\n>\n> Signed-off-by: Dave Borowitz <dborowitz@google.com>\n> ---\n>  Documentation/git-push.txt | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\n> index 135d810..f8b8b8b 100644\n> --- a/Documentation/git-push.txt\n> +++ b/Documentation/git-push.txt\n> @@ -137,7 +137,9 @@ already exists on the remote side.\n>  \tGPG-sign the push request to update refs on the receiving\n>  \tside, to allow it to be checked by the hooks and/or be\n>  \tlogged.  See linkgit:git-receive-pack[1] for the details\n> -\ton the receiving end.\n> +\ton the receiving end.  If the `gpg` executable is not available,\n> +\tor if the server does not support signed pushes, the push will\n> +\tfail.\n\nLooks good.\n\nI am wondering if another mode of failure is worth mentioning: `gpg`\navailable, you have _some_ keys, but signingkey configured does not\nmatch any of the keys.\n\nNote that I said \"am wondering\", which is very different from \"I\nthink we should also describe\".\n\nThanks.\n"},{"id":"268109","messageId":"xmqqmvxtfoo9.fsf@gitster.dls.corp.google.com","threadId":"40083","inReplyTo":"1439492451-11233-6-git-send-email-dborowitz@google.com","subject":"Re: [PATCH 5/7] transport: Remove git_transport_options.push_cert","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-14T23:14:30Z","receivedAt":"2015-08-14T23:14:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Borowitz <dborowitz@google.com> writes:\n\n> This field was set in transport_set_option, but never read in the push\n> code. The push code basically ignores the smart_options field\n> entirely, and derives its options from the flags arguments to the\n> push* callbacks. Note that in git_transport_push there are already\n> several args set from flags that have no corresponding field in\n> git_transport_options; after this change, push_cert is just like\n> those.\n>\n> Signed-off-by: Dave Borowitz <dborowitz@google.com>\n> ---\n\nThanks for cleaning up my mess.\n\nHonestly, to me, the smart transport is always a second-class\ncitizen (and http walkers are not even citizens ;-)) and any support\nof new feature is added as an after-thought once the feature starts\nworking with the native transport, and that development pattern\nclearly shows in a place like this.\n\n>  transport.c | 3 ---\n>  transport.h | 1 -\n>  2 files changed, 4 deletions(-)\n>\n> diff --git a/transport.c b/transport.c\n> index 40692f8..3dd6e30 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -476,9 +476,6 @@ static int set_git_option(struct git_transport_options *opts,\n>  \t\t\t\tdie(\"transport: invalid depth option '%s'\", value);\n>  \t\t}\n>  \t\treturn 0;\n> -\t} else if (!strcmp(name, TRANS_OPT_PUSH_CERT)) {\n> -\t\topts->push_cert = !!value;\n> -\t\treturn 0;\n>  \t}\n>  \treturn 1;\n>  }\n> diff --git a/transport.h b/transport.h\n> index 18d2cf8..79190df 100644\n> --- a/transport.h\n> +++ b/transport.h\n> @@ -12,7 +12,6 @@ struct git_transport_options {\n>  \tunsigned check_self_contained_and_connected : 1;\n>  \tunsigned self_contained_and_connected : 1;\n>  \tunsigned update_shallow : 1;\n> -\tunsigned push_cert : 1;\n>  \tint depth;\n>  \tconst char *uploadpack;\n>  \tconst char *receivepack;\n"},{"id":"268110","messageId":"xmqqio8hfobk.fsf@gitster.dls.corp.google.com","threadId":"40083","inReplyTo":"1439492451-11233-7-git-send-email-dborowitz@google.com","subject":"Re: [PATCH 6/7] Support signing pushes iff the server supports it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-14T23:22:07Z","receivedAt":"2015-08-14T23:22:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Borowitz <dborowitz@google.com> writes:\n\n> diff --git a/send-pack.c b/send-pack.c\n> index 2a64fec..6ae9f45 100644\n> --- a/send-pack.c\n> +++ b/send-pack.c\n> @@ -370,7 +370,7 @@ 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 (args->push_cert) {\n> +\tif (args->push_cert == SEND_PACK_PUSH_CERT_ALWAYS) {\n>  \t\tint len;\n>  \n>  \t\tpush_cert_nonce = server_feature_value(\"push-cert\", &len);\n> @@ -379,6 +379,18 @@ int send_pack(struct send_pack_args *args,\n>  \t\treject_invalid_nonce(push_cert_nonce, len);\n>  \t\tpush_cert_nonce = xmemdupz(push_cert_nonce, len);\n>  \t}\n> +\tif (args->push_cert == SEND_PACK_PUSH_CERT_IF_POSSIBLE) {\n> +\t\tint len;\n> +\n> +\t\tpush_cert_nonce = server_feature_value(\"push-cert\", &len);\n> +\t\tif (push_cert_nonce) {\n> +\t\t\treject_invalid_nonce(push_cert_nonce, len);\n> +\t\t\tpush_cert_nonce = xmemdupz(push_cert_nonce, len);\n> +\t\t} else\n> +\t\t\twarning(_(\"not sending a push certificate since the\"\n> +\t\t\t\t  \" receiving end does not support --signed\"\n> +\t\t\t\t  \" push\"));\n> +\t}\n\nI wonder if the bodies of these two if statements can be a bit\nbetter organized to avoid duplication (I suspect you have tried\nand you may already know that the above is the most readable\nversion, but I haven't tried to do so myself, so...).\n"},{"id":"268191","messageId":"xmqqy4h9et2m.fsf@gitster.dls.corp.google.com","threadId":"40083","inReplyTo":"1439492451-11233-8-git-send-email-dborowitz@google.com","subject":"Re: [PATCH 7/7] Add a config option push.gpgSign for default signed pushes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-17T17:13:53Z","receivedAt":"2015-08-17T17:13:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Borowitz <dborowitz@google.com> writes:\n\n> ---\n\nDoes the lack of sign-off indicate something (like \"this is just a\n'what do people think?' weatherbaloon not yet a serious submission\")?\n\n> +push.gpgSign::\n> +\tMay be set to a boolean value, or the string 'if-possible'. A\n> +\ttrue value causes all pushes to be GPG signed, as if '--signed'\n> +\tis passed to linkgit:git-push[1]. The string 'if-possible'\n> +\tcauses pushes to be signed if the server supports it, as if\n> +\t'--signed-if-possible' is passed to 'git push'. A false value\n> +\tmay override a value from a lower-priority config file. An\n> +\texplicit command-line flag always overrides this config option.\n\n> diff --git a/builtin/push.c b/builtin/push.c\n> index 95a67c5..8972193 100644\n> --- a/builtin/push.c\n> +++ b/builtin/push.c\n> @@ -491,6 +491,26 @@ static int git_push_config(const char *k, const char *v, void *cb)\n>  \treturn git_default_config(k, v, NULL);\n>  }\n>  \n> +static void set_push_cert_flags_from_config(int *flags)\n> +{\n> +\tconst char *value;\n> +\t/* Ignore config if flags were set from command line. */\n> +\tif (*flags & (TRANSPORT_PUSH_CERT_ALWAYS | TRANSPORT_PUSH_CERT_IF_POSSIBLE))\n> +\t\treturn;\n\nThis looks somewhat strange.  Usually we read from config first and\nthen from options, so a git_config() callback shouldn't have to\nworry about what command line option parser did (because it hasn't\nhappened yet).  Why isn't the addition to support this new variable\ndone inside existing git_push_config() callback function?\n\n> +\tif (!git_config_get_value(\"push.gpgsign\", &value)) {\n> +\t\tswitch (git_config_maybe_bool(\"push.gpgsign\", value)) {\n> +\t\tcase 1:\n> +\t\t\t*flags |= TRANSPORT_PUSH_CERT_ALWAYS;\n> +\t\t\tbreak;\n> +\t\tdefault:\n> +\t\t\tif (value && !strcmp(value, \"if-possible\"))\n> +\t\t\t\t*flags |= TRANSPORT_PUSH_CERT_IF_POSSIBLE;\n> +\t\t\telse\n> +\t\t\t\tdie(_(\"Invalid value for 'push.gpgsign'\"));\n> +\t\t}\n> +\t}\n> +}\n> +\n\nmaybe_bool() returns 0 for \"false\" (and its various spellings), 1\nfor \"true\" (and its various spellings) and -1 for \"that's not a\nbool\".\n\nFor \"A false value may override a value\" to be true, we'd need\n\n\t\tcase 0:\n\t\t\t*flags &= ~TRANSPORT_PUSH_CERT_ALWAYS;\n\t\t\tbreak;\n\nor something?\n"},{"id":"268192","messageId":"xmqqtwrxesqa.fsf@gitster.dls.corp.google.com","threadId":"40083","inReplyTo":"CAD0k6qR2HkHHYu8429mvdvN1bkLeTpD-5EbO4Mt+o69rC+P6aQ@mail.gmail.com","subject":"Re: [PATCH 0/7] Flags and config to sign pushes by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-17T17:21:17Z","receivedAt":"2015-08-17T17:21:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Borowitz <dborowitz@google.com> writes:\n\n> Ok, so let us bikeshed a bit further.\n>\n> Bikeshed 1.\n> Option A: --signed/--no-signed--signed-if-possible\n> Option B: --signed=true|false|if-possible, \"--signed\" alone implies \"=true\".\n>\n> Bikeshed 2.\n>\n> Option A: if-possible\n>\n> The possibly confusing thing is one might interpret missing \"gpg\" to\n> mean \"impossible\", i.e. \"if gpg is not installed don't attempt to\n> sign\", which is not the behavior we want.\n>\n> I don't have another succinct way of saying this.\n> \"if-server-supported\" is a mouthful. I think Jonathan mentioned\n> \"opportunistic\", which is fairly opaque.\n>\n>> By \"strange\", I was referring to the possible perception issue on\n>> having a choice other than yes/no for a configuration that allows\n>> you to express your security preference.\n\nMy preference on Bikeshed 1. would probably be to add\n\n    --sign=yes/no/if-asked\n\nand to keep --[no-]signed for \"no\" and \"yes\" for existing users.\n\nRegarding Bikeshed 2., I do not have a strong opinion myself.\n\nThanks.\n"},{"id":"268196","messageId":"CAD0k6qS4A0zWWr1oNLaaps0qD08pkGTcj7yGo_tkPGKMxKWGhQ@mail.gmail.com","threadId":"40083","inReplyTo":"xmqqr3n5fovm.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/7] Documentation/git-push.txt: Document when --signed may fail","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-08-17T18:11:44Z","receivedAt":"2015-08-17T18:11:44Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Fri, Aug 14, 2015 at 7:10 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Dave Borowitz <dborowitz@google.com> writes:\n>\n>> Like --atomic, --signed will fail if the server does not advertise the\n>> necessary capability. In addition, it requires gpg on the client side.\n>>\n>> Signed-off-by: Dave Borowitz <dborowitz@google.com>\n>> ---\n>>  Documentation/git-push.txt | 4 +++-\n>>  1 file changed, 3 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\n>> index 135d810..f8b8b8b 100644\n>> --- a/Documentation/git-push.txt\n>> +++ b/Documentation/git-push.txt\n>> @@ -137,7 +137,9 @@ already exists on the remote side.\n>>       GPG-sign the push request to update refs on the receiving\n>>       side, to allow it to be checked by the hooks and/or be\n>>       logged.  See linkgit:git-receive-pack[1] for the details\n>> -     on the receiving end.\n>> +     on the receiving end.  If the `gpg` executable is not available,\n>> +     or if the server does not support signed pushes, the push will\n>> +     fail.\n>\n> Looks good.\n>\n> I am wondering if another mode of failure is worth mentioning: `gpg`\n> available, you have _some_ keys, but signingkey configured does not\n> match any of the keys.\n>\n> Note that I said \"am wondering\", which is very different from \"I\n> think we should also describe\".\n\nI think we don't need to go down the path of enumerating all possible\nways the operation can fail. There is probably a reasonably concise\nway to include more possibilities. How about:\n\n\"If the attempt to sign with `gpg` fails, or if the server does not\nsupport signed pushes, the push will fail.\"\n\nThis should cover gpg not being found, gpg being fatally\nmisconfigured, crazy unexpected pipe closures, etc.\n\n> Thanks.\n"},{"id":"268198","messageId":"CAD0k6qTnJPc+Nh6dck0_Zx9vnyn5YVMCmy3E=7vr8bTpRSppAA@mail.gmail.com","threadId":"40083","inReplyTo":"xmqqy4h9et2m.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 7/7] Add a config option push.gpgSign for default signed pushes","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-08-17T18:22:55Z","receivedAt":"2015-08-17T18:22:55Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Mon, Aug 17, 2015 at 1:13 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Dave Borowitz <dborowitz@google.com> writes:\n>\n>> ---\n>\n> Does the lack of sign-off indicate something (like \"this is just a\n> 'what do people think?' weatherbaloon not yet a serious submission\")?\n>\n>> +push.gpgSign::\n>> +     May be set to a boolean value, or the string 'if-possible'. A\n>> +     true value causes all pushes to be GPG signed, as if '--signed'\n>> +     is passed to linkgit:git-push[1]. The string 'if-possible'\n>> +     causes pushes to be signed if the server supports it, as if\n>> +     '--signed-if-possible' is passed to 'git push'. A false value\n>> +     may override a value from a lower-priority config file. An\n>> +     explicit command-line flag always overrides this config option.\n>\n>> diff --git a/builtin/push.c b/builtin/push.c\n>> index 95a67c5..8972193 100644\n>> --- a/builtin/push.c\n>> +++ b/builtin/push.c\n>> @@ -491,6 +491,26 @@ static int git_push_config(const char *k, const char *v, void *cb)\n>>       return git_default_config(k, v, NULL);\n>>  }\n>>\n>> +static void set_push_cert_flags_from_config(int *flags)\n>> +{\n>> +     const char *value;\n>> +     /* Ignore config if flags were set from command line. */\n>> +     if (*flags & (TRANSPORT_PUSH_CERT_ALWAYS | TRANSPORT_PUSH_CERT_IF_POSSIBLE))\n>> +             return;\n>\n> This looks somewhat strange.  Usually we read from config first and\n> then from options, so a git_config() callback shouldn't have to\n> worry about what command line option parser did (because it hasn't\n> happened yet).  Why isn't the addition to support this new variable\n> done inside existing git_push_config() callback function?\n\nThe issue is that if both _ALWAYS and _IF_POSSIBLE are set,\ngit_transport_push interprets it as _ALWAYS. But, we are also supposed\nto prefer explicit command-line options to config values.\n\nSuppose we parsed config first, then options. If the user has\npush.signed = always and and passes --signed-if-possible, then the end\nresult is (_ALWAYS | _IF_POSSIBLE), aka always, and we've violated\n\"prefer command line options to config values\".\n\nI guess the alternative is to have --signed just clear the\n_IF_POSSIBLE bit in addition to setting the _ALWAYS bit, and vice\nversa for --signed-if-possible. I am not sure what the end result\nwould be if the user passed a combination of various --signed and\n--signed-if-possible flags on the command line; maybe that's not worth\nworrying about.\n\n>> +     if (!git_config_get_value(\"push.gpgsign\", &value)) {\n>> +             switch (git_config_maybe_bool(\"push.gpgsign\", value)) {\n>> +             case 1:\n>> +                     *flags |= TRANSPORT_PUSH_CERT_ALWAYS;\n>> +                     break;\n>> +             default:\n>> +                     if (value && !strcmp(value, \"if-possible\"))\n>> +                             *flags |= TRANSPORT_PUSH_CERT_IF_POSSIBLE;\n>> +                     else\n>> +                             die(_(\"Invalid value for 'push.gpgsign'\"));\n>> +             }\n>> +     }\n>> +}\n>> +\n>\n> maybe_bool() returns 0 for \"false\" (and its various spellings), 1\n> for \"true\" (and its various spellings) and -1 for \"that's not a\n> bool\".\n>\n> For \"A false value may override a value\" to be true, we'd need\n>\n>                 case 0:\n>                         *flags &= ~TRANSPORT_PUSH_CERT_ALWAYS;\n>                         break;\n>\n> or something?\n\nYes, except unsetting both flags? ~(TRANSPORT_PUSH_CERT_ALWAYS |\nTRANSPORT_CERT_IF_POSSIBLE)\n"},{"id":"268201","messageId":"CAD0k6qTWojeWT10xw_Dc5=Fw5r3rP0PUQOyqO7JAz6Vu+tV54w@mail.gmail.com","threadId":"40083","inReplyTo":"xmqqtwrxesqa.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/7] Flags and config to sign pushes by default","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-08-17T18:32:03Z","receivedAt":"2015-08-17T18:32:03Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Mon, Aug 17, 2015 at 1:21 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Dave Borowitz <dborowitz@google.com> writes:\n>\n>> Ok, so let us bikeshed a bit further.\n>>\n>> Bikeshed 1.\n>> Option A: --signed/--no-signed--signed-if-possible\n>> Option B: --signed=true|false|if-possible, \"--signed\" alone implies \"=true\".\n>>\n>> Bikeshed 2.\n>>\n>> Option A: if-possible\n>>\n>> The possibly confusing thing is one might interpret missing \"gpg\" to\n>> mean \"impossible\", i.e. \"if gpg is not installed don't attempt to\n>> sign\", which is not the behavior we want.\n>>\n>> I don't have another succinct way of saying this.\n>> \"if-server-supported\" is a mouthful. I think Jonathan mentioned\n>> \"opportunistic\", which is fairly opaque.\n>>\n>>> By \"strange\", I was referring to the possible perception issue on\n>>> having a choice other than yes/no for a configuration that allows\n>>> you to express your security preference.\n>\n> My preference on Bikeshed 1. would probably be to add\n>\n>     --sign=yes/no/if-asked\n>\n> and to keep --[no-]signed for \"no\" and \"yes\" for existing users.\n\nIncidentally, I just looked up incidence of true/false vs. yes/no in\ncommand line options, and the results are decidedly undecided:\n\n$ grep -e '--[^ ]*=[^ ]*true' Documentation/*.txt\nDocumentation/git-init.txt:--shared[=(false|true|umask|group|all|world|everybody|0xxx)]::\nDocumentation/git-pull.txt:--rebase[=false|true|preserve]::\nDocumentation/git-svn.txt:--shared[=(false|true|umask|group|all|world|everybody)]::\n$ grep -e '--[^ ]*=[^ ]*yes' Documentation/*.txt\nDocumentation/fetch-options.txt:--recurse-submodules[=yes|on-demand|no]::\nDocumentation/fetch-options.txt:--recurse-submodules-default=[yes|on-demand]::\nDocumentation/git-pull.txt:--[no-]recurse-submodules[=yes|on-demand|no]::\n\nConsistency is hard.\n\nI am inclined to stick with yes/no in this case because\n--recurse-submodules at least feels like a more modern option that we\nshould emulate, but don't feel strongly either way.\n\n> Regarding Bikeshed 2., I do not have a strong opinion myself.\n\nAlthough it sounds like you already expressed an opinion for if-asked\n> if-possible, which is stronger than my own :)\n\n> Thanks.\n"},{"id":"268203","messageId":"xmqq614deoq8.fsf@gitster.dls.corp.google.com","threadId":"40083","inReplyTo":"CAD0k6qTWojeWT10xw_Dc5=Fw5r3rP0PUQOyqO7JAz6Vu+tV54w@mail.gmail.com","subject":"Re: [PATCH 0/7] Flags and config to sign pushes by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-17T18:47:43Z","receivedAt":"2015-08-17T18:47:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Borowitz <dborowitz@google.com> writes:\n\n> On Mon, Aug 17, 2015 at 1:21 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> My preference on Bikeshed 1. would probably be to add\n>>\n>>     --sign=yes/no/if-asked\n>>\n>> and to keep --[no-]signed for \"no\" and \"yes\" for existing users.\n>\n> Incidentally, I just looked up incidence of true/false vs. yes/no in\n> command line options,...\n\nMy yes/no was a short-hand for \"yes\" (and various other ways to\nspell \"true\") and \"no\" (and various other ways to spell \"false\").  I\nwas NOT bikeshedding to say \"I do not like true/false but favor\nyes/no\".\n\nI actually was expecting a short discussion on sign vs signed,\nthough.  As \"tag --sign\" is not \"tag --signed\" even though we call\nthe resulting object a \"signed tag\", \"push --sign\" may be a good\nenough way to spell \"signed push\".  I _think_ signed pushes are\nrecent enough that we still have time to deprecate --signed form,\nbut I do not think it is worth it.\n\nSo an updated suggestion would be that we'd take (this is a pretty\nmuch exhaustive enumeration) these:\n\n    --no-signed\n    --signed\n    --signed=if-asked\n    --signed=yes/true/on/1/2...\n    --signed=no/false/off/0\n\nWe might want to throw in 'always' and 'never' as synonyms for\n'true' and 'false', but again I do not think it is worth the\nconfusion factor, as 'always' and 'true' already mean different\nthings in some other contexts.\n\nThanks.\n"},{"id":"268204","messageId":"CAD0k6qRPxkdOgAo=0+_f8bcFoL70MSvLDJ_OjrFtVMKtcqVV_A@mail.gmail.com","threadId":"40083","inReplyTo":"xmqq614deoq8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/7] Flags and config to sign pushes by default","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-08-17T18:54:09Z","receivedAt":"2015-08-17T18:54:09Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Mon, Aug 17, 2015 at 2:47 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Dave Borowitz <dborowitz@google.com> writes:\n>\n>> On Mon, Aug 17, 2015 at 1:21 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>>\n>>> My preference on Bikeshed 1. would probably be to add\n>>>\n>>>     --sign=yes/no/if-asked\n>>>\n>>> and to keep --[no-]signed for \"no\" and \"yes\" for existing users.\n>>\n>> Incidentally, I just looked up incidence of true/false vs. yes/no in\n>> command line options,...\n>\n> My yes/no was a short-hand for \"yes\" (and various other ways to\n> spell \"true\") and \"no\" (and various other ways to spell \"false\").  I\n> was NOT bikeshedding to say \"I do not like true/false but favor\n> yes/no\".\n>\n> I actually was expecting a short discussion on sign vs signed,\n> though.  As \"tag --sign\" is not \"tag --signed\" even though we call\n> the resulting object a \"signed tag\", \"push --sign\" may be a good\n> enough way to spell \"signed push\".  I _think_ signed pushes are\n> recent enough that we still have time to deprecate --signed form,\n> but I do not think it is worth it.\n\nI agree that \"push --sign\" would be better than \"push --signed\" for\nconsistency with tag, but will defer to you as to whether it's worth\nit to do the deprecation.\n\n> So an updated suggestion would be that we'd take (this is a pretty\n> much exhaustive enumeration) these:\n>\n>     --no-signed\n>     --signed\n>     --signed=if-asked\n>     --signed=yes/true/on/1/2...\n>     --signed=no/false/off/0\n\nFine by me. Would you expect those to all be documented, or just\n--[no-]signed|--signed=(yes|no|if-asked) and silently accept the rest?\n\nIs there a common utility function that does what we want? Basically\ngit_config_maybe_bool but not specifically about configs.\n\n> We might want to throw in 'always' and 'never' as synonyms for\n> 'true' and 'false', but again I do not think it is worth the\n> confusion factor, as 'always' and 'true' already mean different\n> things in some other contexts.\n>\n> Thanks.\n>\n>\n>\n"},{"id":"268214","messageId":"xmqqk2std7lt.fsf@gitster.dls.corp.google.com","threadId":"40083","inReplyTo":"CAD0k6qTnJPc+Nh6dck0_Zx9vnyn5YVMCmy3E=7vr8bTpRSppAA@mail.gmail.com","subject":"Re: [PATCH 7/7] Add a config option push.gpgSign for default signed pushes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-17T19:42:54Z","receivedAt":"2015-08-17T19:42:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Borowitz <dborowitz@google.com> writes:\n\n> The issue is that if both _ALWAYS and _IF_POSSIBLE are set,\n> git_transport_push interprets it as _ALWAYS. But, we are also supposed\n> to prefer explicit command-line options to config values.\n>\n> Suppose we parsed config first, then options. If the user has\n> push.signed = always and and passes --signed-if-possible, then the end\n> result is (_ALWAYS | _IF_POSSIBLE), aka always,...\n\nDoesn't that merely suggest that the option parsing is implemented\nincorrectly?  Why is --signed-if-possible just ORing its bits into\nthe flag, instead of clearing and setting?\n"},{"id":"268215","messageId":"xmqqfv3hd7ea.fsf@gitster.dls.corp.google.com","threadId":"40083","inReplyTo":"xmqqk2std7lt.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 7/7] Add a config option push.gpgSign for default signed pushes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-17T19:47:25Z","receivedAt":"2015-08-17T19:47:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Dave Borowitz <dborowitz@google.com> writes:\n>\n>> The issue is that if both _ALWAYS and _IF_POSSIBLE are set,\n>> git_transport_push interprets it as _ALWAYS. But, we are also supposed\n>> to prefer explicit command-line options to config values.\n>>\n>> Suppose we parsed config first, then options. If the user has\n>> push.signed = always and and passes --signed-if-possible, then the end\n>> result is (_ALWAYS | _IF_POSSIBLE), aka always,...\n>\n> Doesn't that merely suggest that the option parsing is implemented\n> incorrectly?  Why is --signed-if-possible just ORing its bits into\n> the flag, instead of clearing and setting?\n\nThat is, \"git config alias.myp push --sign=if-asked\" followed by\n\n    $ git myp --sign=no\n\nwould internally expand to\n\n    $ git push --sign=if-asked --sign=no\n\nand the result should follow the usual \"last one wins\" rule.\n"},{"id":"268217","messageId":"CAD0k6qRa+exs-Qf+iaRqOhcP4dYvS3Pt3AArLXx_XZucD28c=w@mail.gmail.com","threadId":"40083","inReplyTo":"xmqqk2std7lt.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 7/7] Add a config option push.gpgSign for default signed pushes","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-08-17T19:49:31Z","receivedAt":"2015-08-17T19:49:31Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Mon, Aug 17, 2015 at 3:42 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Dave Borowitz <dborowitz@google.com> writes:\n>\n> > The issue is that if both _ALWAYS and _IF_POSSIBLE are set,\n> > git_transport_push interprets it as _ALWAYS. But, we are also supposed\n> > to prefer explicit command-line options to config values.\n> >\n> > Suppose we parsed config first, then options. If the user has\n> > push.signed = always and and passes --signed-if-possible, then the end\n> > result is (_ALWAYS | _IF_POSSIBLE), aka always,...\n>\n> Doesn't that merely suggest that the option parsing is implemented\n> incorrectly?  Why is --signed-if-possible just ORing its bits into\n> the flag, instead of clearing and setting?\n\nYes, that would be incorrect, but the actual implementation in my\npatch is not incorrect, it just solves the problem a different way.\n\nThe alternative I suggested is another correct implementation, and it\nsounds like it's more in line with the convention you expect. I'll go\nwith that.\n"},{"id":"268218","messageId":"xmqq7fotd71o.fsf@gitster.dls.corp.google.com","threadId":"40083","inReplyTo":"CAD0k6qRPxkdOgAo=0+_f8bcFoL70MSvLDJ_OjrFtVMKtcqVV_A@mail.gmail.com","subject":"Re: [PATCH 0/7] Flags and config to sign pushes by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-17T19:54:59Z","receivedAt":"2015-08-17T19:54:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Borowitz <dborowitz@google.com> writes:\n\n> Is there a common utility function that does what we want? Basically\n> git_config_maybe_bool but not specifically about configs.\n\nInteresting.  git_config_maybe_bool() and its friends take the usual\n(name, value) and pretend to be part of the \"config\" family, primarily\nbecause that was where they came from.\n\nBut they do not really care about \"name\", which is used for error\nreporting and that is what makes them look very specific to the\nconfig subsystem.\n\nI did a quick grep of git_config_maybe_bool() and I _think_ all\ncallers are prepared to handle errors themselves, so it might be a\ngood direction to go in the longer term to drop \"name\" and rename\nthe function to git_parse_maybe_bool() or something, and make these\ncallers use that.\n\nIn the shorter term, at least we should be able to introduce\ngit_parse_maybe_bool() that does not take \"name\", use that as a\nhelper to implement git_config_maybe_bool(), so that the existing\ncallers of git_config_maybe_bool() does not have to change.  And\nthat new helper can be used as your \"Basically it, but not\nspecifically about configs\".\n"},{"id":"268219","messageId":"CAD0k6qS9qA2vrrxF6SQJ-RsX01rryCuZ0zPn4k+OP__TOPR2gg@mail.gmail.com","threadId":"40083","inReplyTo":"xmqq7fotd71o.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/7] Flags and config to sign pushes by default","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-08-17T20:00:52Z","receivedAt":"2015-08-17T20:00:52Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Mon, Aug 17, 2015 at 3:54 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> In the shorter term, at least we should be able to introduce\n> git_parse_maybe_bool() that does not take \"name\", use that as a\n> helper to implement git_config_maybe_bool(), so that the existing\n> callers of git_config_maybe_bool() does not have to change.  And\n> that new helper can be used as your \"Basically it, but not\n> specifically about configs\".\n\nWill do, thanks for the suggestion.\n\nSlight digression for a question that came up during reworking the\nseries: would it be reasonable to rewrite option parsing in\nbuiltin/send-pack.c to use the options API? That way we can easily\nreuse the option callback from builtin/push.c. (It would have some\nside effects like making --no-* variants work where they did not\nbefore; I assume that's a good thing, but it's marginally inconsistent\nwith some other plumbing commands like receive-pack.)\n"},{"id":"268223","messageId":"xmqqy4h9bqml.fsf@gitster.dls.corp.google.com","threadId":"40083","inReplyTo":"CAD0k6qS9qA2vrrxF6SQJ-RsX01rryCuZ0zPn4k+OP__TOPR2gg@mail.gmail.com","subject":"Re: [PATCH 0/7] Flags and config to sign pushes by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-17T20:34:58Z","receivedAt":"2015-08-17T20:34:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Borowitz <dborowitz@google.com> writes:\n\n> Slight digression for a question that came up during reworking the\n> series: would it be reasonable to rewrite option parsing in\n> builtin/send-pack.c to use the options API?\n\nSurely.  The part of the system whose option parsing predates\nparse-options may not have been converted to do so yet, but as long\nas the result is correct, why not.  After all APIs were invented to\nbe used.\n\n> That way we can easily\n> reuse the option callback from builtin/push.c. (It would have some\n> side effects like making --no-* variants work where they did not\n> before; I assume that's a good thing, but it's marginally inconsistent\n> with some other plumbing commands like receive-pack.)\n\nAs long as people are not deliberately feeding --no-something and\nrelying on it to fail, we'd be ok ;-).\n"},{"id":"268307","messageId":"CAD0k6qRPmJAF4-LT1jZTCjTiGMpfS4+gt2C36izt5qe3P1pJig@mail.gmail.com","threadId":"40083","inReplyTo":"xmqqio8hfobk.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 6/7] Support signing pushes iff the server supports it","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-08-19T15:18:02Z","receivedAt":"2015-08-19T15:18:02Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Fri, Aug 14, 2015 at 7:22 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Dave Borowitz <dborowitz@google.com> writes:\n>\n>> diff --git a/send-pack.c b/send-pack.c\n>> index 2a64fec..6ae9f45 100644\n>> --- a/send-pack.c\n>> +++ b/send-pack.c\n>> @@ -370,7 +370,7 @@ int send_pack(struct send_pack_args *args,\n>>               args->use_thin_pack = 0;\n>>       if (server_supports(\"atomic\"))\n>>               atomic_supported = 1;\n>> -     if (args->push_cert) {\n>> +     if (args->push_cert == SEND_PACK_PUSH_CERT_ALWAYS) {\n>>               int len;\n>>\n>>               push_cert_nonce = server_feature_value(\"push-cert\", &len);\n>> @@ -379,6 +379,18 @@ int send_pack(struct send_pack_args *args,\n>>               reject_invalid_nonce(push_cert_nonce, len);\n>>               push_cert_nonce = xmemdupz(push_cert_nonce, len);\n>>       }\n>> +     if (args->push_cert == SEND_PACK_PUSH_CERT_IF_POSSIBLE) {\n>> +             int len;\n>> +\n>> +             push_cert_nonce = server_feature_value(\"push-cert\", &len);\n>> +             if (push_cert_nonce) {\n>> +                     reject_invalid_nonce(push_cert_nonce, len);\n>> +                     push_cert_nonce = xmemdupz(push_cert_nonce, len);\n>> +             } else\n>> +                     warning(_(\"not sending a push certificate since the\"\n>> +                               \" receiving end does not support --signed\"\n>> +                               \" push\"));\n>> +     }\n>\n> I wonder if the bodies of these two if statements can be a bit\n> better organized to avoid duplication (I suspect you have tried\n> and you may already know that the above is the most readable\n> version, but I haven't tried to do so myself, so...).\n\nFound a slightly less repetitious way.\n"}]}