{"thread":{"id":"34416","subject":"[PATCH v4] config: add support for http.<url>.* settings","startedAt":"2013-07-12T11:20:04Z","lastAt":"2013-07-12T18:48:08Z","messageCount":3,"participants":["Kyle J. McKay","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"223160","messageId":"a2ef80fd50c466e5c69726a1968f8ad@f74d39fa044aa309eaea14b9f57fe79","threadId":"34416","inReplyTo":null,"subject":"[PATCH v4] config: add support for http.<url>.* settings","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2013-07-12T11:20:04Z","receivedAt":"2013-07-12T11:20:04Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"The <url> value is considered a match to a url if the <url> value\nis either an exact match or a prefix of the url which ends on a\npath component boundary ('/').  So \"https://example.com/test\" will\nmatch \"https://example.com/test\" and \"https://example.com/test/too\"\nbut not \"https://example.com/testextra\".\n\nLonger matches take precedence over shorter matches with\nenvironment variable settings taking precedence over all.\n\nWith this configuration:\n\n[http]\n        useragent = other-agent\n        noEPSV = true\n[http \"https://example.com\"]\n        useragent = example-agent\n        sslVerify = false\n[http \"https://example.com/path\"]\n        useragent = path-agent\n\nThe \"https://other.example.com/\" url will have useragent\n\"other-agent\" and sslVerify will be on.\n\nThe \"https://example.com/\" url will have useragent\n\"example-agent\" and sslVerify will be off.\n\nThe \"https://example.com/path/sub\" url will have useragent\n\"path-agent\" and sslVerify will be off.\n\nAll three of the examples will have noEPSV enabled.\n\nSigned-off-by: Kyle J. McKay <mackyle@gmail.com>\nReviewed-by: Junio C Hamano <gitster@pobox.com>\n---\n\nThe credentials configuration values already support url-specific\nconfiguration items in the form credential.<url>.*.  This patch\nadds similar support for http configuration values.\n\nThis version of the patch addresses the following feedback:\n\n  * The url[url_prefix_len-1] test can now never succeed.  It is removed.\n\n  * More text added to check_matched_len to clarify behavior when two\n    options have the same key.\n\n Documentation/config.txt |  11 ++++\n http.c                   | 168 ++++++++++++++++++++++++++++++++++++++++++-----\n 2 files changed, 162 insertions(+), 17 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex b4d4887..3731a3a 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1531,6 +1531,17 @@ http.useragent::\n \tof common USER_AGENT strings (but not including those like git/1.7.1).\n \tCan be overridden by the 'GIT_HTTP_USER_AGENT' environment variable.\n \n+http.<url>.*::\n+\tAny of the http.* options above can be applied selectively to some urls.\n+\tFor example \"http.https://example.com.useragent\" would set the user\n+\tagent only for https connections to example.com.  The <url> value\n+\tmatches a url if it is an exact match or a prefix of the url matching\n+\tat a '/' boundary.  Longer <url> matches take precedence over shorter\n+\tones with the environment variable settings taking precedence over all.\n+\tNote that <url> must match the url exactly (other than possibly being a\n+\tprefix).  This means any user, password and/or port setting that appears\n+\tin a url must also appear in <url> to have a successful match.\n+\n i18n.commitEncoding::\n \tCharacter encoding the commit messages are stored in; Git itself\n \tdoes not care per se, but this information is necessary e.g. when\ndiff --git a/http.c b/http.c\nindex 2d086ae..feca70a 100644\n--- a/http.c\n+++ b/http.c\n@@ -30,6 +30,34 @@ static CURL *curl_default;\n \n char curl_errorstr[CURL_ERROR_SIZE];\n \n+enum http_option_type {\n+\topt_post_buffer = 0,\n+\topt_min_sessions,\n+#ifdef USE_CURL_MULTI\n+\topt_max_requests,\n+#endif\n+\topt_ssl_verify,\n+\topt_ssl_try,\n+\topt_ssl_cert,\n+#if LIBCURL_VERSION_NUM >= 0x070903\n+\topt_ssl_key,\n+#endif\n+#if LIBCURL_VERSION_NUM >= 0x070908\n+\topt_ssl_capath,\n+#endif\n+\topt_ssl_cainfo,\n+\topt_low_speed,\n+\topt_low_time,\n+\topt_no_epsv,\n+\topt_http_proxy,\n+\topt_cookie_file,\n+\topt_user_agent,\n+\topt_passwd_req,\n+\topt_max\n+};\n+\n+static size_t http_option_max_matched_len[opt_max];\n+\n static int curl_ssl_verify = -1;\n static int curl_ssl_try;\n static const char *ssl_cert;\n@@ -141,34 +169,121 @@ static void process_curl_messages(void)\n }\n #endif\n \n+static size_t http_options_url_match_prefix(const char *url,\n+\t\t\t\t\t    const char *url_prefix,\n+\t\t\t\t\t    size_t url_prefix_len)\n+{\n+\t/*\n+\t * url_prefix matches url if url_prefix is an exact match for url or it\n+\t * is a prefix of url and the match ends on a path component boundary.\n+\t * Both url and url_prefix are considered to have an implicit '/' on the\n+\t * end for matching purposes if they do not already.\n+\t *\n+\t * url must be NUL terminated.  url_prefix_len is the length of\n+\t * url_prefix which need not be NUL terminated.\n+\t *\n+\t * The return value is the length of the match in characters (excluding\n+\t * any final '/') or 0 for no match.  Passing \"/\" as url_prefix will\n+\t * always cause 0 to be returned.\n+\t */\n+\tsize_t url_len;\n+\tif (url_prefix_len && url_prefix[url_prefix_len - 1] == '/')\n+\t\turl_prefix_len--;\n+\tif (!url_prefix_len || strncmp(url, url_prefix, url_prefix_len))\n+\t\treturn 0;\n+\turl_len = strlen(url);\n+\tif ((url_len == url_prefix_len) || (url[url_prefix_len] == '/'))\n+\t\treturn url_prefix_len;\n+\treturn 0;\n+}\n+\n+static int check_matched_len(enum http_option_type opt, size_t matchlen)\n+{\n+\t/*\n+\t * Check the last matched length of option opt against matchlen and\n+\t * return true if the last matched length is larger (meaning the config\n+\t * setting should be ignored).  Upon seeing the _same_ key (i.e. new key\n+\t * has the same match length which is therefore not larger) the new\n+\t * setting will override the previous setting.  Otherwise return false\n+\t * and record matchlen as the current last matched length of option opt.\n+\t */\n+\tif (http_option_max_matched_len[opt] > matchlen)\n+\t\treturn 1;\n+\thttp_option_max_matched_len[opt] = matchlen;\n+\treturn 0;\n+}\n+\n static int http_options(const char *var, const char *value, void *cb)\n {\n-\tif (!strcmp(\"http.sslverify\", var)) {\n+\tconst char *url = (const char *)cb;\n+\tconst char *key, *dot;\n+\tsize_t matchlen = 0;\n+\n+\tkey = skip_prefix(var, \"http.\");\n+\tif (!key)\n+\t\treturn git_default_config(var, value, cb);\n+\n+\t/*\n+\t * If the part following the leading \"http.\" contains a '.' then check\n+\t * to see if the part between \"http.\" and the last '.' is a match to\n+\t * url.  If it's not then ignore the setting.  Otherwise set key to\n+\t * point to the option which is the part after the final '.' and\n+\t * use key in subsequent comparisons to determine the option type.\n+\t */\n+\tdot = strrchr(key, '.');\n+\tif (dot) {\n+\t\tmatchlen = http_options_url_match_prefix(url, key, dot - key);\n+\t\tif (!matchlen)\n+\t\t\treturn 0;\n+\t\tkey = dot + 1;\n+\t}\n+\n+\tif (!strcmp(\"sslverify\", key)) {\n+\t\tif (check_matched_len(opt_ssl_verify, matchlen))\n+\t\t\treturn 0;\n \t\tcurl_ssl_verify = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n-\tif (!strcmp(\"http.sslcert\", var))\n+\tif (!strcmp(\"sslcert\", key)) {\n+\t\tif (check_matched_len(opt_ssl_cert, matchlen))\n+\t\t\treturn 0;\n \t\treturn git_config_string(&ssl_cert, var, value);\n+\t}\n #if LIBCURL_VERSION_NUM >= 0x070903\n-\tif (!strcmp(\"http.sslkey\", var))\n+\tif (!strcmp(\"sslkey\", key)) {\n+\t\tif (check_matched_len(opt_ssl_key, matchlen))\n+\t\t\treturn 0;\n \t\treturn git_config_string(&ssl_key, var, value);\n+\t}\n #endif\n #if LIBCURL_VERSION_NUM >= 0x070908\n-\tif (!strcmp(\"http.sslcapath\", var))\n+\tif (!strcmp(\"sslcapath\", key)) {\n+\t\tif (check_matched_len(opt_ssl_capath, matchlen))\n+\t\t\treturn 0;\n \t\treturn git_config_string(&ssl_capath, var, value);\n+\t}\n #endif\n-\tif (!strcmp(\"http.sslcainfo\", var))\n+\tif (!strcmp(\"sslcainfo\", key)) {\n+\t\tif (check_matched_len(opt_ssl_cainfo, matchlen))\n+\t\t\treturn 0;\n \t\treturn git_config_string(&ssl_cainfo, var, value);\n-\tif (!strcmp(\"http.sslcertpasswordprotected\", var)) {\n+\t}\n+\tif (!strcmp(\"sslcertpasswordprotected\", key)) {\n+\t\tif (check_matched_len(opt_passwd_req, matchlen))\n+\t\t\treturn 0;\n \t\tif (git_config_bool(var, value))\n \t\t\tssl_cert_password_required = 1;\n \t\treturn 0;\n \t}\n-\tif (!strcmp(\"http.ssltry\", var)) {\n+\tif (!strcmp(\"ssltry\", key)) {\n+\t\tif (check_matched_len(opt_ssl_try, matchlen))\n+\t\t\treturn 0;\n \t\tcurl_ssl_try = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n-\tif (!strcmp(\"http.minsessions\", var)) {\n+\tif (!strcmp(\"minsessions\", key)) {\n+\t\tif (check_matched_len(opt_min_sessions, matchlen))\n+\t\t\treturn 0;\n \t\tmin_curl_sessions = git_config_int(var, value);\n #ifndef USE_CURL_MULTI\n \t\tif (min_curl_sessions > 1)\n@@ -177,39 +292,58 @@ static int http_options(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n #ifdef USE_CURL_MULTI\n-\tif (!strcmp(\"http.maxrequests\", var)) {\n+\tif (!strcmp(\"maxrequests\", key)) {\n+\t\tif (check_matched_len(opt_max_requests, matchlen))\n+\t\t\treturn 0;\n \t\tmax_requests = git_config_int(var, value);\n \t\treturn 0;\n \t}\n #endif\n-\tif (!strcmp(\"http.lowspeedlimit\", var)) {\n+\tif (!strcmp(\"lowspeedlimit\", key)) {\n+\t\tif (check_matched_len(opt_low_speed, matchlen))\n+\t\t\treturn 0;\n \t\tcurl_low_speed_limit = (long)git_config_int(var, value);\n \t\treturn 0;\n \t}\n-\tif (!strcmp(\"http.lowspeedtime\", var)) {\n+\tif (!strcmp(\"lowspeedtime\", key)) {\n+\t\tif (check_matched_len(opt_low_time, matchlen))\n+\t\t\treturn 0;\n \t\tcurl_low_speed_time = (long)git_config_int(var, value);\n \t\treturn 0;\n \t}\n \n-\tif (!strcmp(\"http.noepsv\", var)) {\n+\tif (!strcmp(\"noepsv\", key)) {\n+\t\tif (check_matched_len(opt_no_epsv, matchlen))\n+\t\t\treturn 0;\n \t\tcurl_ftp_no_epsv = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n-\tif (!strcmp(\"http.proxy\", var))\n+\tif (!strcmp(\"proxy\", key)) {\n+\t\tif (check_matched_len(opt_http_proxy, matchlen))\n+\t\t\treturn 0;\n \t\treturn git_config_string(&curl_http_proxy, var, value);\n+\t}\n \n-\tif (!strcmp(\"http.cookiefile\", var))\n+\tif (!strcmp(\"cookiefile\", key)) {\n+\t\tif (check_matched_len(opt_cookie_file, matchlen))\n+\t\t\treturn 0;\n \t\treturn git_config_string(&curl_cookie_file, var, value);\n+\t}\n \n-\tif (!strcmp(\"http.postbuffer\", var)) {\n+\tif (!strcmp(\"postbuffer\", key)) {\n+\t\tif (check_matched_len(opt_post_buffer, matchlen))\n+\t\t\treturn 0;\n \t\thttp_post_buffer = git_config_int(var, value);\n \t\tif (http_post_buffer < LARGE_PACKET_MAX)\n \t\t\thttp_post_buffer = LARGE_PACKET_MAX;\n \t\treturn 0;\n \t}\n \n-\tif (!strcmp(\"http.useragent\", var))\n+\tif (!strcmp(\"useragent\", key)) {\n+\t\tif (check_matched_len(opt_user_agent, matchlen))\n+\t\t\treturn 0;\n \t\treturn git_config_string(&user_agent, var, value);\n+\t}\n \n \t/* Fall back on the default ones */\n \treturn git_default_config(var, value, cb);\n@@ -344,7 +478,7 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)\n \n \thttp_is_verbose = 0;\n \n-\tgit_config(http_options, NULL);\n+\tgit_config(http_options, (void *)url);\n \n \tcurl_global_init(CURL_GLOBAL_ALL);\n \n-- \n1.8.3\n"},{"id":"223199","messageId":"7vfvvjliya.fsf@alter.siamese.dyndns.org","threadId":"34416","inReplyTo":"a2ef80fd50c466e5c69726a1968f8ad@f74d39fa044aa309eaea14b9f57fe79","subject":"Re: [PATCH v4] config: add support for http.<url>.* settings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-12T18:41:17Z","receivedAt":"2013-07-12T18:41:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle J. McKay\" <mackyle@gmail.com> writes:\n\n> The <url> value is considered a match to a url if the <url> value\n> is either an exact match or a prefix of the url which ends on a\n> path component boundary ('/').  So \"https://example.com/test\" will\n> match \"https://example.com/test\" and \"https://example.com/test/too\"\n> but not \"https://example.com/testextra\".\n>\n> Longer matches take precedence over shorter matches with\n> environment variable settings taking precedence over all.\n>\n> With this configuration:\n>\n> [http]\n>         useragent = other-agent\n>         noEPSV = true\n> [http \"https://example.com\"]\n>         useragent = example-agent\n>         sslVerify = false\n> [http \"https://example.com/path\"]\n>         useragent = path-agent\n>\n> The \"https://other.example.com/\" url will have useragent\n> \"other-agent\" and sslVerify will be on.\n>\n> The \"https://example.com/\" url will have useragent\n> \"example-agent\" and sslVerify will be off.\n>\n> The \"https://example.com/path/sub\" url will have useragent\n> \"path-agent\" and sslVerify will be off.\n>\n> All three of the examples will have noEPSV enabled.\n>\n> Signed-off-by: Kyle J. McKay <mackyle@gmail.com>\n> Reviewed-by: Junio C Hamano <gitster@pobox.com>\n\nI haven't reviewed this version (yet).\n\n> ---\n>\n> The credentials configuration values already support url-specific\n> configuration items in the form credential.<url>.*.  This patch\n> adds similar support for http configuration values.\n\nLet's move the above three lines into the proposed log message and\nmake it its first paragraph.  A log message should say what it is\nabout (i.e. what does \"add support\" really mean?  what kind of\nfunctionality the new support brings to us?) upfront and then\nexplain what it does in detail (i.e. the explanation of matching\nsemantics you have as the first paragraph).\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index b4d4887..3731a3a 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1531,6 +1531,17 @@ http.useragent::\n>  \tof common USER_AGENT strings (but not including those like git/1.7.1).\n>  \tCan be overridden by the 'GIT_HTTP_USER_AGENT' environment variable.\n>  \n> +http.<url>.*::\n> +\tAny of the http.* options above can be applied selectively to some urls.\n> +\tFor example \"http.https://example.com.useragent\" would set the user\n> +\tagent only for https connections to example.com.  The <url> value\n> +\tmatches a url if it is an exact match or a prefix of the url matching\n> +\tat a '/' boundary.\n\nGiven Peff's review, I am no longer sure if the \"strictly textual\nmatch\" is the semantics we want.  At least, it is easy to explain\nand understand, but it might be too limiting to be useful.\n\nLet's assume it is what we want, for the rest of the review.\n\n> diff --git a/http.c b/http.c\n> index 2d086ae..feca70a 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -30,6 +30,34 @@ static CURL *curl_default;\n>  \n>  char curl_errorstr[CURL_ERROR_SIZE];\n>  \n> +enum http_option_type {\n> +\topt_post_buffer = 0,\n\nDo we need to have \"= 0\" here?\n\nIs this order meant to match some externally defined order\n(e.g. alphabetical, or the value of corresponding constants defined\nin the cURL library), other than \"opt_max is not a real option but\njust has to be there at the end to count all of them\"?\n\nI am wondering if it would make it easier to read to move all the #ifdef\nat the end immediately before opt-max, or something.\n\n> +\topt_min_sessions,\n> +#ifdef USE_CURL_MULTI\n> +\topt_max_requests,\n> +#endif\n> +...\n> +\topt_user_agent,\n> +\topt_passwd_req,\n> +\topt_max\n> +};\n\n> +static int check_matched_len(enum http_option_type opt, size_t matchlen)\n> +{\n> +\t/*\n> +\t * Check the last matched length of option opt against matchlen and\n> +\t * return true if the last matched length is larger (meaning the config\n> +\t * setting should be ignored).  Upon seeing the _same_ key (i.e. new key\n> +\t * has the same match length which is therefore not larger) the new\n> +\t * setting will override the previous setting.  Otherwise return false\n> +\t * and record matchlen as the current last matched length of option opt.\n> +\t */\n> +\tif (http_option_max_matched_len[opt] > matchlen)\n> +\t\treturn 1;\n> +\thttp_option_max_matched_len[opt] = matchlen;\n> +\treturn 0;\n> +}\n\nA \"check_foo\" that returns either 0 or 1 usually mean 1 is good and\n0 is not.  A \"check_foo\" that returns either negative or 0 usually\nmean negative is an error and 0 is good.\n\nIn general, \"check_foo\" is a bad name for a boolean function.  This\nchecks \"seen_longer_match()\", and it is up to the caller if it\nconsiders good or bad to have a matching element that is longer or\nshorter what it has already seen.  See below.\n\n>  static int http_options(const char *var, const char *value, void *cb)\n>  {\n> -\tif (!strcmp(\"http.sslverify\", var)) {\n> +\tconst char *url = (const char *)cb;\n\nCast?\n\n> +\tif (!strcmp(\"sslverify\", key)) {\n> +\t\tif (check_matched_len(opt_ssl_verify, matchlen))\n> +\t\t\treturn 0;\n>  \t\tcurl_ssl_verify = git_config_bool(var, value);\n>  \t\treturn 0;\n>  \t}\n\nAt this caller, the name \"check_matched_len()\" is not immediately\nobvious, other than it is checking matchlen, what it does.  \"check\"\nneeds \"what is checked\" (i.e. \"matchlen\"), but it also needs \"how it\njudges what is checked\" (i.e. is longer the better?), but the name\ndoes not convey that.  If you rename the function, it becomes a lot\neasier to understand what is going on.\n\n\tif (!strcmp(\"sslverify\", key)) {\n\t\tif (!seen_longer_match(opt_ssl_verify, matchlen))\n\t\t\tcurl_ssl_verify = git_config_bool(var, value);\n\t\treturn 0\n\t}\n\n> @@ -344,7 +478,7 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)\n>  \n>  \thttp_is_verbose = 0;\n>  \n> -\tgit_config(http_options, NULL);\n> +\tgit_config(http_options, (void *)url);\n\nCast?\n"},{"id":"223200","messageId":"7vbo67limv.fsf@alter.siamese.dyndns.org","threadId":"34416","inReplyTo":"a2ef80fd50c466e5c69726a1968f8ad@f74d39fa044aa309eaea14b9f57fe79","subject":"Re: [PATCH v4] config: add support for http.<url>.* settings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-12T18:48:08Z","receivedAt":"2013-07-12T18:48:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle J. McKay\" <mackyle@gmail.com> writes:\n\n> +\tif (!strcmp(\"sslcertpasswordprotected\", key)) {\n> +\t\tif (check_matched_len(opt_passwd_req, matchlen))\n> +\t\t\treturn 0;\n>  \t\tif (git_config_bool(var, value))\n>  \t\t\tssl_cert_password_required = 1;\n>  \t\treturn 0;\n>  \t}\n\nThis is not a new problem, but I think the existing code is wrong.\nThere is no way to countermand an earlier\n\n\t[http]\n        \tsslcertpasswordprotected\n\nin a more generic configuration file with\n\n\n\t[http]\n        \tsslcertpasswordprotected = no\n\nin a repository specific configuration file.\n\nPerhaps we should fix it as a preparatory patch (1/2) before the\nmain \"feature addition\" patch.\n\n> -\tif (!strcmp(\"http.ssltry\", var)) {\n> +\tif (!strcmp(\"ssltry\", key)) {\n> +\t\tif (check_matched_len(opt_ssl_try, matchlen))\n> +\t\t\treturn 0;\n>  \t\tcurl_ssl_try = git_config_bool(var, value);\n>  \t\treturn 0;\n>  \t}\n"}]}