{"thread":{"id":"46983","subject":"[PATCH] Added support for new configuration parameter push.pushOption","startedAt":"2017-10-17T06:32:46Z","lastAt":"2017-10-24T00:59:44Z","messageCount":14,"participants":["Marius Paliga","Junio C Hamano","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"330523","messageId":"CAK7vU=1LGTLrEkRYS=autkDHmJAeq9=qa5jfx=DE+acPLsCqTg@mail.gmail.com","threadId":"46983","inReplyTo":null,"subject":"[PATCH] Added support for new configuration parameter push.pushOption","fromName":"Marius Paliga","fromEmail":"marius.paliga@gmail.com","sentAt":"2017-10-17T06:32:40Z","receivedAt":"2017-10-17T06:32:46Z","isPatch":true,"sender":{"key":"marius.paliga@gmail.com","avatar":"https://avatars.githubusercontent.com/u/54767958?v=4"},"body":"builtin/push.c: add push.pushOption config\n\nCurrently push options need to be given explicitly, via\nthe command line as \"git push --push-option\".\n\nThe UX of Git would be enhanced if push options could be\nconfigured instead of given each time on the command line.\n\nAdd the config option push.pushOption, which is a multi\nstring option, containing push options that are sent by default.\n\nWhen push options are set in the system wide config\n(/etc/gitconfig), they can be unset later in the more specific\nrepository config by setting the string to the empty string.\n\nAdd tests and documentation as well.\n\nSigned-off-by: Marius Paliga <marius.paliga@gmail.com>\n---\n Documentation/git-push.txt |  3 ++\n builtin/push.c             | 12 ++++++++\n t/t5545-push-options.sh    | 77 ++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 92 insertions(+)\n\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex 3e76e99f3..da9b17624 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -161,6 +161,9 @@ already exists on the remote side.\n     Transmit the given string to the server, which passes them to\n     the pre-receive as well as the post-receive hook. The given string\n     must not contain a NUL or LF character.\n+    Default push options can also be specified with configuration\n+    variable `push.pushOption`. String(s) specified here will always\n+    be passed to the server without need to specify it using `--push-option`\n\n --receive-pack=<git-receive-pack>::\n --exec=<git-receive-pack>::\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 2ac810422..10e520c8f 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -32,6 +32,8 @@ static const char **refspec;\n static int refspec_nr;\n static int refspec_alloc;\n\n+static struct string_list push_options_from_config = STRING_LIST_INIT_DUP;\n+\n static void add_refspec(const char *ref)\n {\n     refspec_nr++;\n@@ -503,6 +505,14 @@ static int git_push_config(const char *k, const\nchar *v, void *cb)\n         int val = git_config_bool(k, v) ?\n             RECURSE_SUBMODULES_ON_DEMAND : RECURSE_SUBMODULES_OFF;\n         recurse_submodules = val;\n+    } else if (!strcmp(k, \"push.pushoption\")) {\n+        if (v == NULL)\n+            return config_error_nonbool(k);\n+        else\n+            if (v[0] == '\\0')\n+                string_list_clear(&push_options_from_config, 0);\n+            else\n+                string_list_append(&push_options_from_config, v);\n     }\n\n     return git_default_config(k, v, NULL);\n@@ -562,6 +572,8 @@ int cmd_push(int argc, const char **argv, const\nchar *prefix)\n     packet_trace_identity(\"push\");\n     git_config(git_push_config, &flags);\n     argc = parse_options(argc, argv, prefix, options, push_usage, 0);\n+    if (!push_options.nr)\n+        push_options = push_options_from_config;\n     set_push_cert_flags(&flags, push_cert);\n\n     if (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL |\nTRANSPORT_PUSH_MIRROR))))\ndiff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh\nindex 90a4b0d2f..463783789 100755\n--- a/t/t5545-push-options.sh\n+++ b/t/t5545-push-options.sh\n@@ -140,6 +140,83 @@ test_expect_success 'push options and submodules' '\n     test_cmp expect parent_upstream/.git/hooks/post-receive.push_options\n '\n\n+test_expect_success 'default push option' '\n+    mk_repo_pair &&\n+    git -C upstream config receive.advertisePushOptions true &&\n+    (\n+        cd workbench &&\n+        test_commit one &&\n+        git push --mirror up &&\n+        test_commit two &&\n+        git -c push.pushOption=default push up master\n+    ) &&\n+    test_refs master master &&\n+    echo \"default\" >expect &&\n+    test_cmp expect upstream/.git/hooks/pre-receive.push_options &&\n+    test_cmp expect upstream/.git/hooks/post-receive.push_options\n+'\n+\n+test_expect_success 'two default push options' '\n+    mk_repo_pair &&\n+    git -C upstream config receive.advertisePushOptions true &&\n+    (\n+        cd workbench &&\n+        test_commit one &&\n+        git push --mirror up &&\n+        test_commit two &&\n+        git -c push.pushOption=default1 -c push.pushOption=default2\npush up master\n+    ) &&\n+    test_refs master master &&\n+    printf \"default1\\ndefault2\\n\" >expect &&\n+    test_cmp expect upstream/.git/hooks/pre-receive.push_options &&\n+    test_cmp expect upstream/.git/hooks/post-receive.push_options\n+'\n+\n+test_expect_success 'push option from command line overrides\nfrom-config push option' '\n+    mk_repo_pair &&\n+    git -C upstream config receive.advertisePushOptions true &&\n+    (\n+        cd workbench &&\n+        test_commit one &&\n+        git push --mirror up &&\n+        test_commit two &&\n+        git -c push.pushOption=default push --push-option=manual up master\n+    ) &&\n+    test_refs master master &&\n+    echo \"manual\" >expect &&\n+    test_cmp expect upstream/.git/hooks/pre-receive.push_options &&\n+    test_cmp expect upstream/.git/hooks/post-receive.push_options\n+'\n+\n+test_expect_success 'empty value of push.pushOption in config clears\nthe list' '\n+    mk_repo_pair &&\n+    git -C upstream config receive.advertisePushOptions true &&\n+    (\n+        cd workbench &&\n+        test_commit one &&\n+        git push --mirror up &&\n+        test_commit two &&\n+        git -c push.pushOption=default1 -c push.pushOption= -c\npush.pushOption=default2 push up master\n+    ) &&\n+    test_refs master master &&\n+    echo \"default2\" >expect &&\n+    test_cmp expect upstream/.git/hooks/pre-receive.push_options &&\n+    test_cmp expect upstream/.git/hooks/post-receive.push_options\n+'\n+\n+test_expect_success 'invalid push option in config' '\n+    mk_repo_pair &&\n+    git -C upstream config receive.advertisePushOptions true &&\n+    (\n+        cd workbench &&\n+        test_commit one &&\n+        git push --mirror up &&\n+        test_commit two &&\n+        test_must_fail git -c push.pushOption push up master\n+    ) &&\n+    test_refs master HEAD@{1}\n"},{"id":"330586","messageId":"xmqqvajdl1hn.fsf@gitster.mtv.corp.google.com","threadId":"46983","inReplyTo":"CAK7vU=1LGTLrEkRYS=autkDHmJAeq9=qa5jfx=DE+acPLsCqTg@mail.gmail.com","subject":"Re: [PATCH] Added support for new configuration parameter push.pushOption","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-18T00:54:12Z","receivedAt":"2017-10-18T00:59:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Marius Paliga <marius.paliga@gmail.com> writes:\n\n> builtin/push.c: add push.pushOption config\n\nThis is a good title for this change; it would be perfect if it were\non the Subject: header ;-)\n\n> Currently push options need to be given explicitly, via\n> the command line as \"git push --push-option\".\n>\n> The UX of Git would be enhanced if push options could be\n> configured instead of given each time on the command line.\n>\n> Add the config option push.pushOption, which is a multi\n> string option, containing push options that are sent by default.\n\ns/Currently p/P/ would be sufficient; we describe the status quo in\npresent tense, and give an order to the codebase to \"become like so\"\n(or command a patch monkey to \"make the codebase like so\") by using\nimperative mood.\n\nI think something shorter like this would be sufficient:\n\n    Push options need to be given explicitly, via the command line as \"git\n    push --push-option <option>\".  Add the config option push.pushOption,\n    which is a multi-valued option, containing push options that are sent\n    by default.\n\n    When push options are set in the lower-priority configulation file\n    (e.g. /etc/gitconfig, or $HOME/.gitconfig), they can be unset later in\n    the more specific repository config by the empty string.\n\n> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\n> index 3e76e99f3..da9b17624 100644\n> --- a/Documentation/git-push.txt\n> +++ b/Documentation/git-push.txt\n> @@ -161,6 +161,9 @@ already exists on the remote side.\n>      Transmit the given string to the server, which passes them to\n>      the pre-receive as well as the post-receive hook. The given string\n>      must not contain a NUL or LF character.\n> +    Default push options can also be specified with configuration\n> +    variable `push.pushOption`. String(s) specified here will always\n> +    be passed to the server without need to specify it using `--push-option`\n\n\"will always be passed\" is not a \"Default\".  \n\nIf we really need to say it, say something like\n\n\tWhen no `--push-option <option>` is given from the command\n\tline, the values of this configuration variable are used\n\tinstead.\n\nBut that is what \"Default\" means, so dropping the second sentence\naltogether would be more concise and better.\n\n> diff --git a/builtin/push.c b/builtin/push.c\n> index 2ac810422..10e520c8f 100644\n> --- a/builtin/push.c\n> +++ b/builtin/push.c\n> @@ -32,6 +32,8 @@ static const char **refspec;\n>  static int refspec_nr;\n>  static int refspec_alloc;\n>\n> +static struct string_list push_options_from_config = STRING_LIST_INIT_DUP;\n> +\n>  static void add_refspec(const char *ref)\n>  {\n>      refspec_nr++;\n> @@ -503,6 +505,14 @@ static int git_push_config(const char *k, const\n> char *v, void *cb)\n>          int val = git_config_bool(k, v) ?\n>              RECURSE_SUBMODULES_ON_DEMAND : RECURSE_SUBMODULES_OFF;\n>          recurse_submodules = val;\n> +    } else if (!strcmp(k, \"push.pushoption\")) {\n> +        if (v == NULL)\n> +            return config_error_nonbool(k);\n> +        else\n> +            if (v[0] == '\\0')\n\nMake this \"else if (!*v)\" on a single line and dedent the remainder.\n\n> +                string_list_clear(&push_options_from_config, 0);\n> +            else\n> +                string_list_append(&push_options_from_config, v);\n>      }\n\n> @@ -562,6 +572,8 @@ int cmd_push(int argc, const char **argv, const\n> char *prefix)\n>      packet_trace_identity(\"push\");\n>      git_config(git_push_config, &flags);\n>      argc = parse_options(argc, argv, prefix, options, push_usage, 0);\n> +    if (!push_options.nr)\n> +        push_options = push_options_from_config;\n\nWe encourage our developers to think twice before doing a structure\nassignment.\n\nI think this is a bad idea, primarily because at the point where\nstring_list_clear() is later called on &push_options, it is unclear\nhow to release the resources held by push_options_from_config().  It\nalso is bad to depend on the implementation detail that overwriting\nthe structure do not leak push_options.item when push_options.nr is\nzero, but it is conceivable that STRING_LIST_INIT_* could later be\nmodified to pre-allocate a small array, at which point this will\nbecome a memory leak.\n\n> diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh\n> index 90a4b0d2f..463783789 100755\n> --- a/t/t5545-push-options.sh\n> +++ b/t/t5545-push-options.sh\n> @@ -140,6 +140,83 @@ test_expect_success 'push options and submodules' '\n> ...\n> +        git -c push.pushOption=default1 -c push.pushOption=default2\n> push up master\n> ...\n> +test_expect_success 'push option from command line overrides\n> from-config push option' '\n\nIt appears that your MUA is expanding tabs to spaces and wrapping\nlong lines in your patch?  Please double check and make sure that\nwill not happen.\n\nI couldn't use your patch (because it was broken by your MUA) as a\nbase to show an incremental improvement, but here is how I would do\nthe \"code\" part.\n\n builtin/push.c | 22 ++++++++++++++++++----\n 1 file changed, 18 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 03846e8379..89ef029c67 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -32,6 +32,8 @@ static const char **refspec;\n static int refspec_nr;\n static int refspec_alloc;\n \n+static struct string_list push_options_config = STRING_LIST_INIT_DUP;\n+\n static void add_refspec(const char *ref)\n {\n \trefspec_nr++;\n@@ -503,6 +505,13 @@ static int git_push_config(const char *k, const char *v, void *cb)\n \t\tint val = git_config_bool(k, v) ?\n \t\t\tRECURSE_SUBMODULES_ON_DEMAND : RECURSE_SUBMODULES_OFF;\n \t\trecurse_submodules = val;\n+\t} else if (!strcmp(k, \"push.pushoption\")) {\n+\t\tif (!v)\n+\t\t\treturn config_error_nonbool(k);\n+\t\telse if (!*v)\n+\t\t\tstring_list_clear(&push_options_config, 0);\n+\t\telse\n+\t\t\tstring_list_append(&push_options_config, v);\n \t}\n \n \treturn git_default_config(k, v, NULL);\n@@ -515,7 +524,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-\tstruct string_list push_options = STRING_LIST_INIT_DUP;\n+\tstruct string_list push_options_cmdline = STRING_LIST_INIT_DUP;\n+\tstruct string_list *push_options;\n \tconst struct string_list_item *item;\n \n \tstruct option options[] = {\n@@ -562,6 +572,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \tpacket_trace_identity(\"push\");\n \tgit_config(git_push_config, &flags);\n \targc = parse_options(argc, argv, prefix, options, push_usage, 0);\n+\tpush_options = (push_options_cmdline.nr\n+\t\t\t? &push_options_cmdline\n+\t\t\t: &push_options_config);\n \tset_push_cert_flags(&flags, push_cert);\n \n \tif (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))\n@@ -584,12 +597,13 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\tset_refspecs(argv + 1, argc - 1, repo);\n \t}\n \n-\tfor_each_string_list_item(item, &push_options)\n+\tfor_each_string_list_item(item, push_options)\n \t\tif (strchr(item->string, '\\n'))\n \t\t\tdie(_(\"push options must not have new line characters\"));\n \n-\trc = do_push(repo, flags, &push_options);\n-\tstring_list_clear(&push_options, 0);\n+\trc = do_push(repo, flags, push_options);\n+\tstring_list_clear(&push_options_cmdline, 0);\n+\tstring_list_clear(&push_options_config, 0);\n \tif (rc == -1)\n \t\tusage_with_options(push_usage, options);\n \telse\n\n"},{"id":"330587","messageId":"xmqqmv4pl117.fsf@gitster.mtv.corp.google.com","threadId":"46983","inReplyTo":"xmqqvajdl1hn.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Added support for new configuration parameter push.pushOption","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-18T01:04:04Z","receivedAt":"2017-10-18T01:04:11Z","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> base to show an incremental improvement, but here is how I would do\n> the \"code\" part.\n>\n>  builtin/push.c | 22 ++++++++++++++++++----\n>  1 file changed, 18 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/push.c b/builtin/push.c\n> index 03846e8379..89ef029c67 100644\n> --- a/builtin/push.c\n> +++ b/builtin/push.c\n> @@ -32,6 +32,8 @@ static const char **refspec;\n>  static int refspec_nr;\n>  static int refspec_alloc;\n>  \n> +static struct string_list push_options_config = STRING_LIST_INIT_DUP;\n> +\n>  static void add_refspec(const char *ref)\n>  {\n>  \trefspec_nr++;\n> @@ -503,6 +505,13 @@ static int git_push_config(const char *k, const char *v, void *cb)\n>  \t\tint val = git_config_bool(k, v) ?\n>  \t\t\tRECURSE_SUBMODULES_ON_DEMAND : RECURSE_SUBMODULES_OFF;\n>  \t\trecurse_submodules = val;\n> +\t} else if (!strcmp(k, \"push.pushoption\")) {\n> +\t\tif (!v)\n> +\t\t\treturn config_error_nonbool(k);\n> +\t\telse if (!*v)\n> +\t\t\tstring_list_clear(&push_options_config, 0);\n> +\t\telse\n> +\t\t\tstring_list_append(&push_options_config, v);\n\nHere needs to be\n\n\t\treturn 0;\n\nas there is no point in falling through to call\ngit_default_config().\n\n>  \t}\n>  \n>  \treturn git_default_config(k, v, NULL);\n"},{"id":"330651","messageId":"20171019174715.6577-1-marius.paliga@gmail.com","threadId":"46983","inReplyTo":"xmqqmv4pl117.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] builtin/push.c: add push.pushOption config","fromName":"Marius Paliga","fromEmail":"marius.paliga@gmail.com","sentAt":"2017-10-19T17:47:15Z","receivedAt":"2017-10-19T17:49:04Z","isPatch":true,"sender":{"key":"marius.paliga@gmail.com","avatar":"https://avatars.githubusercontent.com/u/54767958?v=4"},"body":"Push options need to be given explicitly, via the command line as \"git\npush --push-option <option>\".  Add the config option push.pushOption,\nwhich is a multi-valued option, containing push options that are sent\nby default.\n\nWhen push options are set in the lower-priority configulation file\n(e.g. /etc/gitconfig, or $HOME/.gitconfig), they can be unset later in\nthe more specific repository config by the empty string.\n\nAdd tests and update documentation as well.\n\nSigned-off-by: Marius Paliga <marius.paliga@gmail.com>\n---\n Documentation/git-push.txt |  3 ++\n builtin/push.c             | 26 +++++++++++++---\n t/t5545-push-options.sh    | 77 ++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 101 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex 3e76e99f3..aa78057dc 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -161,6 +161,9 @@ already exists on the remote side.\n \tTransmit the given string to the server, which passes them to\n \tthe pre-receive as well as the post-receive hook. The given string\n \tmust not contain a NUL or LF character.\n+\tWhen no `--push-option <option>` is given from the command\n+\tline, the values of configuration variable `push.pushOption`\n+\tare used instead.\n \n --receive-pack=<git-receive-pack>::\n --exec=<git-receive-pack>::\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 2ac810422..1c28427d8 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -32,6 +32,8 @@ static const char **refspec;\n static int refspec_nr;\n static int refspec_alloc;\n \n+static struct string_list push_options_config = STRING_LIST_INIT_DUP;\n+\n static void add_refspec(const char *ref)\n {\n \trefspec_nr++;\n@@ -503,6 +505,15 @@ static int git_push_config(const char *k, const char *v, void *cb)\n \t\tint val = git_config_bool(k, v) ?\n \t\t\tRECURSE_SUBMODULES_ON_DEMAND : RECURSE_SUBMODULES_OFF;\n \t\trecurse_submodules = val;\n+\t} else if (!strcmp(k, \"push.pushoption\")) {\n+\t\tif (!v)\n+\t\t\treturn config_error_nonbool(k);\n+\t\telse\n+\t\t\tif (!*v)\n+\t\t\t\tstring_list_clear(&push_options_config, 0);\n+\t\t\telse\n+\t\t\t\tstring_list_append(&push_options_config, v);\n+\t\treturn 0;\n \t}\n \n \treturn git_default_config(k, v, NULL);\n@@ -515,7 +526,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-\tstruct string_list push_options = STRING_LIST_INIT_DUP;\n+\tstruct string_list push_options_cmdline = STRING_LIST_INIT_DUP;\n+\tstruct string_list *push_options;\n \tconst struct string_list_item *item;\n \n \tstruct option options[] = {\n@@ -551,7 +563,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\t  0, \"signed\", &push_cert, \"yes|no|if-asked\", N_(\"GPG sign the push\"),\n \t\t  PARSE_OPT_OPTARG, option_parse_push_signed },\n \t\tOPT_BIT(0, \"atomic\", &flags, N_(\"request atomic transaction on remote side\"), TRANSPORT_PUSH_ATOMIC),\n-\t\tOPT_STRING_LIST('o', \"push-option\", &push_options, N_(\"server-specific\"), N_(\"option to transmit\")),\n+\t\tOPT_STRING_LIST('o', \"push-option\", &push_options_cmdline, N_(\"server-specific\"), N_(\"option to transmit\")),\n \t\tOPT_SET_INT('4', \"ipv4\", &family, N_(\"use IPv4 addresses only\"),\n \t\t\t\tTRANSPORT_FAMILY_IPV4),\n \t\tOPT_SET_INT('6', \"ipv6\", &family, N_(\"use IPv6 addresses only\"),\n@@ -562,6 +574,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \tpacket_trace_identity(\"push\");\n \tgit_config(git_push_config, &flags);\n \targc = parse_options(argc, argv, prefix, options, push_usage, 0);\n+\tpush_options = (push_options_cmdline.nr\n+\t\t? &push_options_cmdline\n+\t\t: &push_options_config);\n \tset_push_cert_flags(&flags, push_cert);\n \n \tif (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))\n@@ -584,12 +599,13 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\tset_refspecs(argv + 1, argc - 1, repo);\n \t}\n \n-\tfor_each_string_list_item(item, &push_options)\n+\tfor_each_string_list_item(item, push_options)\n \t\tif (strchr(item->string, '\\n'))\n \t\t\tdie(_(\"push options must not have new line characters\"));\n \n-\trc = do_push(repo, flags, &push_options);\n-\tstring_list_clear(&push_options, 0);\n+\trc = do_push(repo, flags, push_options);\n+\tstring_list_clear(&push_options_cmdline, 0);\n+\tstring_list_clear(&push_options_config, 0);\n \tif (rc == -1)\n \t\tusage_with_options(push_usage, options);\n \telse\ndiff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh\nindex 90a4b0d2f..463783789 100755\n--- a/t/t5545-push-options.sh\n+++ b/t/t5545-push-options.sh\n@@ -140,6 +140,83 @@ test_expect_success 'push options and submodules' '\n \ttest_cmp expect parent_upstream/.git/hooks/post-receive.push_options\n '\n \n+test_expect_success 'default push option' '\n+\tmk_repo_pair &&\n+\tgit -C upstream config receive.advertisePushOptions true &&\n+\t(\n+\t\tcd workbench &&\n+\t\ttest_commit one &&\n+\t\tgit push --mirror up &&\n+\t\ttest_commit two &&\n+\t\tgit -c push.pushOption=default push up master\n+\t) &&\n+\ttest_refs master master &&\n+\techo \"default\" >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 'two default push options' '\n+\tmk_repo_pair &&\n+\tgit -C upstream config receive.advertisePushOptions true &&\n+\t(\n+\t\tcd workbench &&\n+\t\ttest_commit one &&\n+\t\tgit push --mirror up &&\n+\t\ttest_commit two &&\n+\t\tgit -c push.pushOption=default1 -c push.pushOption=default2 push up master\n+\t) &&\n+\ttest_refs master master &&\n+\tprintf \"default1\\ndefault2\\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_expect_success 'push option from command line overrides from-config push option' '\n+\tmk_repo_pair &&\n+\tgit -C upstream config receive.advertisePushOptions true &&\n+\t(\n+\t\tcd workbench &&\n+\t\ttest_commit one &&\n+\t\tgit push --mirror up &&\n+\t\ttest_commit two &&\n+\t\tgit -c push.pushOption=default push --push-option=manual up master\n+\t) &&\n+\ttest_refs master master &&\n+\techo \"manual\" >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 'empty value of push.pushOption in config clears the list' '\n+\tmk_repo_pair &&\n+\tgit -C upstream config receive.advertisePushOptions true &&\n+\t(\n+\t\tcd workbench &&\n+\t\ttest_commit one &&\n+\t\tgit push --mirror up &&\n+\t\ttest_commit two &&\n+\t\tgit -c push.pushOption=default1 -c push.pushOption= -c push.pushOption=default2 push up master\n+\t) &&\n+\ttest_refs master master &&\n+\techo \"default2\" >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 'invalid push option in config' '\n+\tmk_repo_pair &&\n+\tgit -C upstream config receive.advertisePushOptions true &&\n+\t(\n+\t\tcd workbench &&\n+\t\ttest_commit one &&\n+\t\tgit push --mirror up &&\n+\t\ttest_commit two &&\n+\t\ttest_must_fail git -c push.pushOption push up master\n+\t) &&\n+\ttest_refs master HEAD@{1}\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.14.1\n\n"},{"id":"330666","messageId":"CAGZ79kaSU+w0=zb61=5pEzhtd4U5Hzae4C2bUgpchNHAL_mzMA@mail.gmail.com","threadId":"46983","inReplyTo":"20171019174715.6577-1-marius.paliga@gmail.com","subject":"Re: [PATCH] builtin/push.c: add push.pushOption config","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-19T19:46:08Z","receivedAt":"2017-10-19T19:46:14Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 19, 2017 at 10:47 AM, Marius Paliga <marius.paliga@gmail.com> wrote:\n> Push options need to be given explicitly, via the command line as \"git\n> push --push-option <option>\".  Add the config option push.pushOption,\n> which is a multi-valued option, containing push options that are sent\n> by default.\n>\n> When push options are set in the lower-priority configulation file\n> (e.g. /etc/gitconfig, or $HOME/.gitconfig), they can be unset later in\n> the more specific repository config by the empty string.\n>\n> Add tests and update documentation as well.\n\nThanks for keeping working on this feature!\n\n\n> @@ -161,6 +161,9 @@ already exists on the remote side.\n>         Transmit the given string to the server, which passes them to\n>         the pre-receive as well as the post-receive hook. The given string\n>         must not contain a NUL or LF character.\n> +       When no `--push-option <option>` is given from the command\n> +       line, the values of configuration variable `push.pushOption`\n> +       are used instead.\n\nWe'd also want to document how push.pushOption works in\nDocumentation/config.txt (that contains all the configs)\n\nSo in the config, we have to explicitly give an empty option to\nclear the previous options, but on the command line we do not need\nthat, but instead we'd have to repeat any push options that we desire\nthat were configured?\n\nExample:\n\n  /etc/gitconfig\n  push.pushoption = a\n  push.pushoption = b\n\n  ~/.gitconfig\n  push.pushoption = c\n\n  repo/.git/config\n  push.pushoption =\n  push.pushoption = b\n\nwill result in only b as a and c are\ncleared.\n\nIf I were to run\n  git -c push.pushOption=d push ... (in repo)\nit would be b and d, but\n  git push --push-option=d\nwould be d only?\n\nAs a user I might have expected it to be the same,\nexpecting\n  git push --push-option='' --push-option=d\nto suppress b.\n\n\n> @@ -584,12 +599,13 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n>                 set_refspecs(argv + 1, argc - 1, repo);\n>         }\n>\n> -       for_each_string_list_item(item, &push_options)\n> +       for_each_string_list_item(item, push_options)\n\nWe have to do the same for _cmdline here, too?\n\nThe tests look good!\n\nThanks,\nStefan\n"},{"id":"330708","messageId":"xmqqbml2imrj.fsf@gitster.mtv.corp.google.com","threadId":"46983","inReplyTo":"CAGZ79kaSU+w0=zb61=5pEzhtd4U5Hzae4C2bUgpchNHAL_mzMA@mail.gmail.com","subject":"Re: [PATCH] builtin/push.c: add push.pushOption config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-20T02:19:44Z","receivedAt":"2017-10-20T02:19:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>> @@ -161,6 +161,9 @@ already exists on the remote side.\n>>         Transmit the given string to the server, which passes them to\n>>         the pre-receive as well as the post-receive hook. The given string\n>>         must not contain a NUL or LF character.\n>> +       When no `--push-option <option>` is given from the command\n>> +       line, the values of configuration variable `push.pushOption`\n>> +       are used instead.\n>\n> We'd also want to document how push.pushOption works in\n> Documentation/config.txt (that contains all the configs)\n\nPerhaps.\n\n> So in the config, we have to explicitly give an empty option to\n> clear the previous options, but on the command line we do not need\n> that, but instead we'd have to repeat any push options that we desire\n> that were configured?\n\nIt is not wrong per-se to phrase it like so, but I think that is\nmaking it unnecessarily confusing by conflating two things.  (1)\nconfigured values are overridden from the command line, just like\nany other --option/config.variable pair and (2) unlike usual single\nvalue variables where \"last one wins\" rule is simple enough to\nexplain,, multi-value variables need a way to \"forget everything we\nsaid so far and start from scratch\" syntax, especially when multiple\ninput files are involved.\n\n> Example:\n>\n>   /etc/gitconfig\n>   push.pushoption = a\n>   push.pushoption = b\n>\n>   ~/.gitconfig\n>   push.pushoption = c\n>\n>   repo/.git/config\n>   push.pushoption =\n>   push.pushoption = b\n>\n> will result in only b as a and c are\n> cleared.\n\nThe above is correct, and it might be worth giving it as an example\nin the doc, because not just \"give an empty entry to clear what has\nbeen accumulated so far\" but a multi-valued option in general is a\nrather rare thing.\n\n> If I were to run\n>   git -c push.pushOption=d push ... (in repo)\n> it would be b and d, but\n>   git push --push-option=d\n> would be d only?\n\n>> @@ -584,12 +599,13 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n>>                 set_refspecs(argv + 1, argc - 1, repo);\n>>         }\n>>\n>> -       for_each_string_list_item(item, &push_options)\n>> +       for_each_string_list_item(item, push_options)\n>\n> We have to do the same for _cmdline here, too?\n\nI do not think so.  The point of these lines that appear before this\nloop:\n\n \tgit_config(git_push_config, &flags);\n \targc = parse_options(argc, argv, prefix, options, push_usage, 0);\n+\tpush_options = (push_options_cmdline.nr\n+\t\t? &push_options_cmdline\n+\t\t: &push_options_config);\n\nis that the command line overrides configured values, just like any\nother configuration.  Adding _cmdline variant here is doubly wrong\nwhen command line options are given in that it (1) duplicates what\nwas obtained from the command line, and (2) does not clear the\nconfigured values.\n"},{"id":"330715","messageId":"xmqq376eibrw.fsf@gitster.mtv.corp.google.com","threadId":"46983","inReplyTo":"xmqqbml2imrj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] builtin/push.c: add push.pushOption config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-20T06:17:07Z","receivedAt":"2017-10-20T06:17:19Z","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>> We'd also want to document how push.pushOption works in\n>> Documentation/config.txt (that contains all the configs)\n>\n> Perhaps.\n\nHere is my attempt.  I have a feeling that the way http.extraheaders\nis described may be much easier to read and we may want to mimick\nits style.  I dunno.\n\n Documentation/config.txt | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 1ac0ae6adb..631ed1172e 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2621,6 +2621,16 @@ push.gpgSign::\n \toverride a value from a lower-priority config file. An explicit\n \tcommand-line flag always overrides this config option.\n \n+push.pushOption::\n+\tWhen no `--push-option=<option>` argument is given from the\n+\tcommand line, `git push` behaves as if each <value> of\n+\tthis variable is given as `--push-option=<value>`.\n++\n+This is a multi-valued variable, and an empty value can be used in a\n+higher priority cofiguration file (e.g. `.git/config` in a\n+repository) to clear the values inherited from a lower priority\n+configuration files (e.g. `$HOME/.gitconfig`).\n+\n push.recurseSubmodules::\n \tMake sure all submodule commits used by the revisions to be pushed\n \tare available on a remote-tracking branch. If the value is 'check'\n"},{"id":"330716","messageId":"xmqqy3o6gx4z.fsf@gitster.mtv.corp.google.com","threadId":"46983","inReplyTo":"xmqq376eibrw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] builtin/push.c: add push.pushOption config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-20T06:18:36Z","receivedAt":"2017-10-20T06:18:48Z","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> Junio C Hamano <gitster@pobox.com> writes:\n>\n>>> We'd also want to document how push.pushOption works in\n>>> Documentation/config.txt (that contains all the configs)\n>>\n>> Perhaps.\n>\n> Here is my attempt.\n\nAnother thing I noticed while we are around the area is that unlike\nall other options in git-push.txt page, this one forgets to say it\nalways takes mandatory string.  Here is a possible fix.\n\n Documentation/git-push.txt | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex aa78057dc5..a8504e0ae3 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -156,12 +156,12 @@ already exists on the remote side.\n \tEither all refs are updated, or on error, no refs are updated.\n \tIf the server does not support atomic pushes the push will fail.\n \n--o::\n---push-option::\n+-o <option>::\n+--push-option=<option>::\n \tTransmit the given string to the server, which passes them to\n \tthe pre-receive as well as the post-receive hook. The given string\n \tmust not contain a NUL or LF character.\n-\tWhen no `--push-option <option>` is given from the command\n+\tWhen no `--push-option=<option>` is given from the command\n \tline, the values of configuration variable `push.pushOption`\n \tare used instead.\n \n"},{"id":"330721","messageId":"CAK7vU=0m6qyFrPb9E7HY79v0Vxwn+So3gyhxJZBPVD1t7vX35A@mail.gmail.com","threadId":"46983","inReplyTo":"xmqqy3o6gx4z.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] builtin/push.c: add push.pushOption config","fromName":"Marius Paliga","fromEmail":"marius.paliga@gmail.com","sentAt":"2017-10-20T08:52:25Z","receivedAt":"2017-10-20T08:52:34Z","isPatch":true,"sender":{"key":"marius.paliga@gmail.com","avatar":"https://avatars.githubusercontent.com/u/54767958?v=4"},"body":"> --o::\n> ---push-option::\n> +-o <option>::\n> +--push-option=<option>::\n>         Transmit the given string to the server, which passes them to\n>         the pre-receive as well as the post-receive hook. The given string\n>         must not contain a NUL or LF character.\n> -       When no `--push-option <option>` is given from the command\n> +       When no `--push-option=<option>` is given from the command\n>         line, the values of configuration variable `push.pushOption`\n>         are used instead.\n\nShould we also mention that this option can take multiple values from\nthe command line?\n\n-o <option>::\n--push-option=<option>::\n        Transmit the given string to the server, which passes them to\n        the pre-receive as well as the post-receive hook. The given string\n        must not contain a NUL or LF character.\n+       Multiple `--push-option=<option>` are allowed on the command line.\n       When no `--push-option=<option>` is given from the command\n        line, the values of configuration variable `push.pushOption`\n        are used instead.\n\n\n\n2017-10-20 8:18 GMT+02:00 Junio C Hamano <gitster@pobox.com>:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Junio C Hamano <gitster@pobox.com> writes:\n>>\n>>>> We'd also want to document how push.pushOption works in\n>>>> Documentation/config.txt (that contains all the configs)\n>>>\n>>> Perhaps.\n>>\n>> Here is my attempt.\n>\n> Another thing I noticed while we are around the area is that unlike\n> all other options in git-push.txt page, this one forgets to say it\n> always takes mandatory string.  Here is a possible fix.\n>\n>  Documentation/git-push.txt | 6 +++---\n>  1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\n> index aa78057dc5..a8504e0ae3 100644\n> --- a/Documentation/git-push.txt\n> +++ b/Documentation/git-push.txt\n> @@ -156,12 +156,12 @@ already exists on the remote side.\n>         Either all refs are updated, or on error, no refs are updated.\n>         If the server does not support atomic pushes the push will fail.\n>\n> --o::\n> ---push-option::\n> +-o <option>::\n> +--push-option=<option>::\n>         Transmit the given string to the server, which passes them to\n>         the pre-receive as well as the post-receive hook. The given string\n>         must not contain a NUL or LF character.\n> -       When no `--push-option <option>` is given from the command\n> +       When no `--push-option=<option>` is given from the command\n>         line, the values of configuration variable `push.pushOption`\n>         are used instead.\n>\n"},{"id":"330739","messageId":"CAGZ79kbEmdJu9a-h07UvPaymMmLJ4Azm+iChSpCBwvvtOW8gog@mail.gmail.com","threadId":"46983","inReplyTo":"xmqqbml2imrj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] builtin/push.c: add push.pushOption config","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-20T19:02:09Z","receivedAt":"2017-10-20T19:02:15Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 19, 2017 at 7:19 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>>> @@ -161,6 +161,9 @@ already exists on the remote side.\n>>>         Transmit the given string to the server, which passes them to\n>>>         the pre-receive as well as the post-receive hook. The given string\n>>>         must not contain a NUL or LF character.\n>>> +       When no `--push-option <option>` is given from the command\n>>> +       line, the values of configuration variable `push.pushOption`\n>>> +       are used instead.\n>>\n>> We'd also want to document how push.pushOption works in\n>> Documentation/config.txt (that contains all the configs)\n>\n> Perhaps.\n>\n>> So in the config, we have to explicitly give an empty option to\n>> clear the previous options, but on the command line we do not need\n>> that, but instead we'd have to repeat any push options that we desire\n>> that were configured?\n>\n> It is not wrong per-se to phrase it like so, but I think that is\n> making it unnecessarily confusing by conflating two things.  (1)\n> configured values are overridden from the command line, just like\n> any other --option/config.variable pair\n\nbecause they are single value options (usually).\n\n> and (2) unlike usual single\n> value variables where \"last one wins\" rule is simple enough to\n> explain,, multi-value variables need a way to \"forget everything we\n> said so far and start from scratch\" syntax, especially when multiple\n> input files are involved.\n\nok, my view of how that should be done is clashing once again\nwith the projects established standards. Sorry for the noise.\n\n>> Example:\n>>\n>>   /etc/gitconfig\n>>   push.pushoption = a\n>>   push.pushoption = b\n>>\n>>   ~/.gitconfig\n>>   push.pushoption = c\n>>\n>>   repo/.git/config\n>>   push.pushoption =\n>>   push.pushoption = b\n>>\n>> will result in only b as a and c are\n>> cleared.\n>\n> The above is correct, and it might be worth giving it as an example\n> in the doc, because not just \"give an empty entry to clear what has\n> been accumulated so far\" but a multi-valued option in general is a\n> rather rare thing.\n>\n>> If I were to run\n>>   git -c push.pushOption=d push ... (in repo)\n>> it would be b and d, but\n>>   git push --push-option=d\n>> would be d only?\n>\n>>> @@ -584,12 +599,13 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n>>>                 set_refspecs(argv + 1, argc - 1, repo);\n>>>         }\n>>>\n>>> -       for_each_string_list_item(item, &push_options)\n>>> +       for_each_string_list_item(item, push_options)\n>>\n>> We have to do the same for _cmdline here, too?\n>\n> I do not think so.  The point of these lines that appear before this\n> loop:\n>\n>         git_config(git_push_config, &flags);\n>         argc = parse_options(argc, argv, prefix, options, push_usage, 0);\n> +       push_options = (push_options_cmdline.nr\n> +               ? &push_options_cmdline\n> +               : &push_options_config);\n>\n> is that the command line overrides configured values, just like any\n> other configuration.  Adding _cmdline variant here is doubly wrong\n> when command line options are given in that it (1) duplicates what\n> was obtained from the command line, and (2) does not clear the\n> configured values.\n\nOh, ok. Sorry for the noise once again.\n\nThanks,\nStefan\n"},{"id":"330758","messageId":"xmqqlgk5gu8t.fsf@gitster.mtv.corp.google.com","threadId":"46983","inReplyTo":"CAK7vU=0m6qyFrPb9E7HY79v0Vxwn+So3gyhxJZBPVD1t7vX35A@mail.gmail.com","subject":"Re: [PATCH] builtin/push.c: add push.pushOption config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-21T01:33:22Z","receivedAt":"2017-10-21T01:33:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Marius Paliga <marius.paliga@gmail.com> writes:\n\n> Should we also mention that this option can take multiple values from\n> the command line?\n>\n> -o <option>::\n> --push-option=<option>::\n>         Transmit the given string to the server, which passes them to\n>         the pre-receive as well as the post-receive hook. The given string\n>         must not contain a NUL or LF character.\n> +       Multiple `--push-option=<option>` are allowed on the command line.\n>        When no `--push-option=<option>` is given from the command\n>         line, the values of configuration variable `push.pushOption`\n>         are used instead.\n\nMy first reaction was \"Do we do that for other options that can be\ngiven multiple times?  If not, perhaps this is not needed.\"\n\nThen my second reaction was \"Do we have that many options that can\nbe given multiple times in the first place?  If not, perhaps a\nchange like this to highlight these oddball options may not be a bad\nidea.\"\n\nAnd my third reaction was \"Well, even if we have many such options,\nand even if most of them are not explicitly marked as usable\nmultiple times in the current documentation, that's not a reason to\nkeep it that way---the readers ought to be able to find out which\nones can be used multiple times and how these multiple instances\ninteract with each other, because the usual rule for single-instance\noptions is 'the last one wins' (e.g. 'git status -uall -uno' should\nbe the same as 'git status -uno') but to these multi-value options\nthat rule does not hold\".\n\nSo, yes, I think it is a good idea.\n\nBut it opens a tangent #leftoverbits.  Among the most commonly used\ncommands listed in \"git --help\", there indeed are some commands that\ntake multiple options of the same kind (even if we do not count an\nobvious exception \"--verbose\", which everybody understands as the\nnumber of times it is given indicates the verbosity level).\nSomebody needs to go over their documentation and add \"this can be\ngiven multiple times from the command line, and here is what happens\nto them\".\n\nIn your suggestion for \"push -o <value1> -o <value2>\", \"here is what\nhappens\" is missing.  Perhaps\n\n\tWhen multiple `--push-option=<option>` are given, they are\n\tall sent to the other side in the order listed on the\n\tcommand line.\n\nor something like that.\n\nThanks.\n"},{"id":"330759","messageId":"xmqqfuadgtcu.fsf@gitster.mtv.corp.google.com","threadId":"46983","inReplyTo":"CAGZ79kbEmdJu9a-h07UvPaymMmLJ4Azm+iChSpCBwvvtOW8gog@mail.gmail.com","subject":"Re: [PATCH] builtin/push.c: add push.pushOption config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-21T01:52:33Z","receivedAt":"2017-10-21T01:52:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>>> So in the config, we have to explicitly give an empty option to\n>>> clear the previous options, but on the command line we do not need\n>>> that, but instead we'd have to repeat any push options that we desire\n>>> that were configured?\n>>\n>> It is not wrong per-se to phrase it like so, but I think that is\n>> making it unnecessarily confusing by conflating two things.  (1)\n>> configured values are overridden from the command line, just like\n>> any other --option/config.variable pair\n>\n> because they are single value options (usually).\n>\n>> and (2) unlike usual single\n>> value variables where \"last one wins\" rule is simple enough to\n>> explain,, multi-value variables need a way to \"forget everything we\n>> said so far and start from scratch\" syntax, especially when multiple\n>> input files are involved.\n>\n> ok, my view of how that should be done is clashing once again\n> with the projects established standards. Sorry for the noise.\n\nI do not think it is a noise.  I was primarily focusing on \"have to\nexplicitly\" part, making it sound as if it was a flaw.  I do think\nit is a good idea to explain a variable and/or an option is\nmulti-valued and how multiple instances of them interact with each\nother.  I think the status quo is:\n\n\tBoth configuration and command line, these multi-valued\n\tthings accumulate.  The \"command line can be used to\n\toverride things from the configuration\" rule applies as any\n\tother config/option pair.\n\n\tIn addition, in the configuration, there is a mechanism to\n\tclear the values read so far with the \"an empty value\n\tclears\" rule, because you may not have control over, or it\n\tmay be cumbersome to modify, lower-priority configuration\n\tfiles.  There is no such thing for command line, as the\n\tentirety of the command line for each invocation is under\n\tyour control.\n\nIf somebody has\n\n\t[alias] mypush = push -ofoo\n\nthen the command line argument for one invocation of \"git mypush\"\nmay *not* be under your control and it might be also convenient if\nthere were a mechanism to clear what has been accumulated from the\ncommand line.  It may be worth considering, but if we were going in\nthat direction, I suspect that it is probably a good idea to first\nstep back a bit and introduce a mechanism to easily define pairs of\noption/config in a more sysmtematic way [*1*].  Once we have such a\nmechanism, the \"clear the previous\" syntax for the command line\nwould be implemented uniformly in there.\n\n\n[Footnote]\n\n*1* E.g. right now, the fact that \"push --push-option\" and\n    \"push.pushOption\" are related, or that \"status -u<mode>\" and\n    \"status.showUntrackedFiles\" are related, is only known to the\n    code and the documentation; I am not sure if it is even a good\n    idea to add config to each and every option that exists, but for\n    the ones that do exist, it would be nicer if there were a more\n    uniform and systematic way for parse-options.c and config.c APIs\n    to help our code, instead of writing git_config() callback and\n    options[] array to give to parse_options(), making sure they\n    refer to the same internal variable, and the latter overrides\n    the former.\n"},{"id":"330832","messageId":"20171023114449.11902-1-marius.paliga@gmail.com","threadId":"46983","inReplyTo":"xmqqfuadgtcu.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] builtin/push.c: add push.pushOption config","fromName":"Marius Paliga","fromEmail":"marius.paliga@gmail.com","sentAt":"2017-10-23T11:44:49Z","receivedAt":"2017-10-23T11:46:22Z","isPatch":true,"sender":{"key":"marius.paliga@gmail.com","avatar":"https://avatars.githubusercontent.com/u/54767958?v=4"},"body":"Push options need to be given explicitly, via the command line as \"git\npush --push-option <option>\".  Add the config option push.pushOption,\nwhich is a multi-valued option, containing push options that are sent\nby default.\n\nWhen push options are set in the lower-priority configulation file\n(e.g. /etc/gitconfig, or $HOME/.gitconfig), they can be unset later in\nthe more specific repository config by the empty string.\n\nAdd tests and update documentation as well.\n\nSigned-off-by: Marius Paliga <marius.paliga@gmail.com>\n---\n Documentation/config.txt   | 29 +++++++++++++++++\n Documentation/git-push.txt | 10 ++++--\n builtin/push.c             | 26 +++++++++++++---\n t/t5545-push-options.sh    | 77 ++++++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 135 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 1ac0ae6ad..0bd46a6ab 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2621,6 +2621,35 @@ push.gpgSign::\n \toverride a value from a lower-priority config file. An explicit\n \tcommand-line flag always overrides this config option.\n \n+push.pushOption::\n+\tWhen no `--push-option=<option>` argument is given from the\n+\tcommand line, `git push` behaves as if each <value> of\n+\tthis variable is given as `--push-option=<value>`.\n++\n+This is a multi-valued variable, and an empty value can be used in a\n+higher priority cofiguration file (e.g. `.git/config` in a\n+repository) to clear the values inherited from a lower priority\n+configuration files (e.g. `$HOME/.gitconfig`).\n++\n+--\n+\n+Example:\n+\n+/etc/gitconfig\n+  push.pushoption = a\n+  push.pushoption = b\n+\n+~/.gitconfig\n+  push.pushoption = c\n+\n+repo/.git/config\n+  push.pushoption =\n+  push.pushoption = b\n+\n+This will result in only b (a and c are cleared).\n+\n+--\n+\n push.recurseSubmodules::\n \tMake sure all submodule commits used by the revisions to be pushed\n \tare available on a remote-tracking branch. If the value is 'check'\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex 3e76e99f3..5b08302fc 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -156,11 +156,17 @@ already exists on the remote side.\n \tEither all refs are updated, or on error, no refs are updated.\n \tIf the server does not support atomic pushes the push will fail.\n \n--o::\n---push-option::\n+-o <option>::\n+--push-option=<option>::\n \tTransmit the given string to the server, which passes them to\n \tthe pre-receive as well as the post-receive hook. The given string\n \tmust not contain a NUL or LF character.\n+\tWhen multiple `--push-option=<option>` are given, they are\n+\tall sent to the other side in the order listed on the\n+\tcommand line.\n+\tWhen no `--push-option=<option>` is given from the command\n+\tline, the values of configuration variable `push.pushOption`\n+\tare used instead.\n \n --receive-pack=<git-receive-pack>::\n --exec=<git-receive-pack>::\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 2ac810422..1c28427d8 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -32,6 +32,8 @@ static const char **refspec;\n static int refspec_nr;\n static int refspec_alloc;\n \n+static struct string_list push_options_config = STRING_LIST_INIT_DUP;\n+\n static void add_refspec(const char *ref)\n {\n \trefspec_nr++;\n@@ -503,6 +505,15 @@ static int git_push_config(const char *k, const char *v, void *cb)\n \t\tint val = git_config_bool(k, v) ?\n \t\t\tRECURSE_SUBMODULES_ON_DEMAND : RECURSE_SUBMODULES_OFF;\n \t\trecurse_submodules = val;\n+\t} else if (!strcmp(k, \"push.pushoption\")) {\n+\t\tif (!v)\n+\t\t\treturn config_error_nonbool(k);\n+\t\telse\n+\t\t\tif (!*v)\n+\t\t\t\tstring_list_clear(&push_options_config, 0);\n+\t\t\telse\n+\t\t\t\tstring_list_append(&push_options_config, v);\n+\t\treturn 0;\n \t}\n \n \treturn git_default_config(k, v, NULL);\n@@ -515,7 +526,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-\tstruct string_list push_options = STRING_LIST_INIT_DUP;\n+\tstruct string_list push_options_cmdline = STRING_LIST_INIT_DUP;\n+\tstruct string_list *push_options;\n \tconst struct string_list_item *item;\n \n \tstruct option options[] = {\n@@ -551,7 +563,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\t  0, \"signed\", &push_cert, \"yes|no|if-asked\", N_(\"GPG sign the push\"),\n \t\t  PARSE_OPT_OPTARG, option_parse_push_signed },\n \t\tOPT_BIT(0, \"atomic\", &flags, N_(\"request atomic transaction on remote side\"), TRANSPORT_PUSH_ATOMIC),\n-\t\tOPT_STRING_LIST('o', \"push-option\", &push_options, N_(\"server-specific\"), N_(\"option to transmit\")),\n+\t\tOPT_STRING_LIST('o', \"push-option\", &push_options_cmdline, N_(\"server-specific\"), N_(\"option to transmit\")),\n \t\tOPT_SET_INT('4', \"ipv4\", &family, N_(\"use IPv4 addresses only\"),\n \t\t\t\tTRANSPORT_FAMILY_IPV4),\n \t\tOPT_SET_INT('6', \"ipv6\", &family, N_(\"use IPv6 addresses only\"),\n@@ -562,6 +574,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \tpacket_trace_identity(\"push\");\n \tgit_config(git_push_config, &flags);\n \targc = parse_options(argc, argv, prefix, options, push_usage, 0);\n+\tpush_options = (push_options_cmdline.nr\n+\t\t? &push_options_cmdline\n+\t\t: &push_options_config);\n \tset_push_cert_flags(&flags, push_cert);\n \n \tif (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))\n@@ -584,12 +599,13 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\tset_refspecs(argv + 1, argc - 1, repo);\n \t}\n \n-\tfor_each_string_list_item(item, &push_options)\n+\tfor_each_string_list_item(item, push_options)\n \t\tif (strchr(item->string, '\\n'))\n \t\t\tdie(_(\"push options must not have new line characters\"));\n \n-\trc = do_push(repo, flags, &push_options);\n-\tstring_list_clear(&push_options, 0);\n+\trc = do_push(repo, flags, push_options);\n+\tstring_list_clear(&push_options_cmdline, 0);\n+\tstring_list_clear(&push_options_config, 0);\n \tif (rc == -1)\n \t\tusage_with_options(push_usage, options);\n \telse\ndiff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh\nindex 90a4b0d2f..463783789 100755\n--- a/t/t5545-push-options.sh\n+++ b/t/t5545-push-options.sh\n@@ -140,6 +140,83 @@ test_expect_success 'push options and submodules' '\n \ttest_cmp expect parent_upstream/.git/hooks/post-receive.push_options\n '\n \n+test_expect_success 'default push option' '\n+\tmk_repo_pair &&\n+\tgit -C upstream config receive.advertisePushOptions true &&\n+\t(\n+\t\tcd workbench &&\n+\t\ttest_commit one &&\n+\t\tgit push --mirror up &&\n+\t\ttest_commit two &&\n+\t\tgit -c push.pushOption=default push up master\n+\t) &&\n+\ttest_refs master master &&\n+\techo \"default\" >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 'two default push options' '\n+\tmk_repo_pair &&\n+\tgit -C upstream config receive.advertisePushOptions true &&\n+\t(\n+\t\tcd workbench &&\n+\t\ttest_commit one &&\n+\t\tgit push --mirror up &&\n+\t\ttest_commit two &&\n+\t\tgit -c push.pushOption=default1 -c push.pushOption=default2 push up master\n+\t) &&\n+\ttest_refs master master &&\n+\tprintf \"default1\\ndefault2\\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_expect_success 'push option from command line overrides from-config push option' '\n+\tmk_repo_pair &&\n+\tgit -C upstream config receive.advertisePushOptions true &&\n+\t(\n+\t\tcd workbench &&\n+\t\ttest_commit one &&\n+\t\tgit push --mirror up &&\n+\t\ttest_commit two &&\n+\t\tgit -c push.pushOption=default push --push-option=manual up master\n+\t) &&\n+\ttest_refs master master &&\n+\techo \"manual\" >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 'empty value of push.pushOption in config clears the list' '\n+\tmk_repo_pair &&\n+\tgit -C upstream config receive.advertisePushOptions true &&\n+\t(\n+\t\tcd workbench &&\n+\t\ttest_commit one &&\n+\t\tgit push --mirror up &&\n+\t\ttest_commit two &&\n+\t\tgit -c push.pushOption=default1 -c push.pushOption= -c push.pushOption=default2 push up master\n+\t) &&\n+\ttest_refs master master &&\n+\techo \"default2\" >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 'invalid push option in config' '\n+\tmk_repo_pair &&\n+\tgit -C upstream config receive.advertisePushOptions true &&\n+\t(\n+\t\tcd workbench &&\n+\t\ttest_commit one &&\n+\t\tgit push --mirror up &&\n+\t\ttest_commit two &&\n+\t\ttest_must_fail git -c push.pushOption push up master\n+\t) &&\n+\ttest_refs master HEAD@{1}\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.14.1\n\n"},{"id":"330879","messageId":"xmqq3769bbt1.fsf@gitster.mtv.corp.google.com","threadId":"46983","inReplyTo":"20171023114449.11902-1-marius.paliga@gmail.com","subject":"Re: [PATCH] builtin/push.c: add push.pushOption config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-24T00:59:38Z","receivedAt":"2017-10-24T00:59:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Marius Paliga <marius.paliga@gmail.com> writes:\n\n> Push options need to be given explicitly, via the command line as \"git\n> push --push-option <option>\".  Add the config option push.pushOption,\n> which is a multi-valued option, containing push options that are sent\n> by default.\n>\n> When push options are set in the lower-priority configulation file\n> (e.g. /etc/gitconfig, or $HOME/.gitconfig), they can be unset later in\n> the more specific repository config by the empty string.\n>\n> Add tests and update documentation as well.\n>\n> Signed-off-by: Marius Paliga <marius.paliga@gmail.com>\n> ---\n\nLooks good.\n\n> +This is a multi-valued variable, and an empty value can be used in a\n> +higher priority cofiguration file (e.g. `.git/config` in a\n\ns/cofig/config/; I think you inherited from me, sorry.\n\nI'll tweak it while queuing; there is not need to resend only to fix\nthis.\n\nThanks.\n"}]}