{"thread":{"id":"34406","subject":"[PATCH v3] config: add support for http.<url>.* settings","startedAt":"2013-07-11T21:50:03Z","lastAt":"2013-07-15T09:50:48Z","messageCount":11,"participants":["Kyle J. McKay","Junio C Hamano","Jeff King","Aaron Schrab"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"223098","messageId":"9e7edfbc83a7284615af4ca0de39c1b@f74d39fa044aa309eaea14b9f57fe79","threadId":"34406","inReplyTo":null,"subject":"[PATCH v3] config: add support for http.<url>.* settings","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2013-07-11T21:50:03Z","receivedAt":"2013-07-11T21:50:03Z","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>\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 some initial feedback.\n\n Documentation/config.txt |  11 +++\n http.c                   | 169 ++++++++++++++++++++++++++++++++++++++++++-----\n 2 files changed, 163 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..d6d3507 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,122 @@ 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)      ||\n+\t     (url[url_prefix_len - 1] == '/') ||\n+\t     (url[url_prefix_len] == '/')     )\n+\t\treturn url[url_prefix_len - 1] == '/'\n+\t\t       ? url_prefix_len - 1 : 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\n+\t * and return true if the last matched length is larger (meaning\n+\t * the config setting should be ignored).  Otherwise return false\n+\t * and record matchlen as the 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 +293,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 +479,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":"223116","messageId":"7voba8pu6r.fsf@alter.siamese.dyndns.org","threadId":"34406","inReplyTo":"9e7edfbc83a7284615af4ca0de39c1b@f74d39fa044aa309eaea14b9f57fe79","subject":"Re: [PATCH v3] config: add support for http.<url>.* settings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-11T23:12:44Z","receivedAt":"2013-07-11T23:12:44Z","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> +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,122 @@ 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\n\"URL=http://a.b/c/\" vs \"URLprefix=http://a.b/c/\" would not return\nhere, because we only check up to the length of the prefix after\nstripping the trailing slash from the prefix..\n\n\"URL=http://a.b/cd\" vs \"URLprefix=http://a.b/c/\" would not return\nhere, because we only check up to the length of the prefix after\nstripping the trailing slash from the prefix.\n\nThe latter needs to be rejected in the next condition.\n\n> +\turl_len = strlen(url);\n> +\tif ( (url_len == url_prefix_len)      ||\n\nI thought your plan was that you wanted to treat \"URL=http://a.b/c/\"\nand \"URL=http://a.b/c\" the same way; taking strlen(url) and using it\nfor comparison without adjusting for the trailing slash makes it\nsmell somewhat fishy...\n\n> +\t     (url[url_prefix_len - 1] == '/') ||\n> +\t     (url[url_prefix_len] == '/')     )\n\nThe prefix side no longer has slash at the end, and we know url and\nthe prefix match up to the length of the prefix at this point.  Can\nurl[prefix-len - 1] ever be '/'?  The latter (e.g. one past the\nprefix length can be '/' so that the prefix \"http://a.b/c\" can\nmatch against url \"http://a.b/c/anything\") makes sense, though.\n\nIt is not immediately obvious if this function is correct, at\nleast to me.\n\n> +\t\treturn url[url_prefix_len - 1] == '/'\n> +\t\t       ? url_prefix_len - 1 : 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\n> +\t * and return true if the last matched length is larger (meaning\n> +\t * the config setting should be ignored).  Otherwise return false\n> +\t * and record matchlen as the last matched length of option opt.\n> +\t */\n> +\tif (http_option_max_matched_len[opt] > matchlen)\n> +\t\treturn 1;\n\nIf you had\n\n\thttp.\"http://a.b/c\".opt = A\n\nin your ~/.gitconfig and then\n\n\thttp.\"http://a.b/c\".opt = B\n\nto override it for a particular repository's .git/config, then we\nread ~/.gitconfig first, remembering \"http://a.b/c\" has matched, and\nthen we read .git/config, and encounter the same URLprefix.  As the\ncomparision is done with \">\", not \">=\", this allows the latter to\noverride the former.\n\nWhich may deserve to be added to the comment, perhaps\n\n\t... length is larger (meaning the config setting should be\n\tignored).  Upon seeing the _same_ key (i.e. new key is the\n\tsame length as the longest match so far) is not ignored, but\n\toverrides the previous settings.\n\nor something.  It also would be nice to have a test to make sure\nthis will not be broken in any future change.\n\nOther than that, overall the change looks nice.\n"},{"id":"223156","messageId":"20130712095923.GA4695@sigill.intra.peff.net","threadId":"34406","inReplyTo":"9e7edfbc83a7284615af4ca0de39c1b@f74d39fa044aa309eaea14b9f57fe79","subject":"Re: [PATCH v3] config: add support for http.<url>.* settings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-12T09:59:23Z","receivedAt":"2013-07-12T09:59:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 11, 2013 at 02:50:03PM -0700, Kyle J. McKay wrote:\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\nI like the general direction you are going, and especially the path\nprefix matching is something I had always meant to implement for the\ncredential code.\n\nA few comments on the implementation:\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\nWe usually put enum fields in ALL_CAPS to mark them as constants (though\nyou can find few exceptions in the code).\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\nIt looks like you're matching the URLs as raw strings, and I don't see\nany canonicalization going on. What happens if I have\n\"https://example.com/foo+bar\" in my config, but then I visit\n\"https://example.comfoo%20bar\"?\n\nOr what about optional components? If I have \"https://example.com\" in my\nconfig, but I am visiting \"https://peff@example.com/\", shouldn't it\nmatch? The config spec is more general than my specific URL; you would\nnot want it to match in the opposite direction, though.\n\nI think you would want to break down the URL into fields, canonicalize\neach field by undoing any encoding, and then compare the broken-down\nURLs, as credential_match does (adding in your prefix/boundary matching to\nthe path instead of a straight string comparison).\n\nI think you could mostly reuse the code there by doing:\n\n  1. Create a \"struct url\" that contains the broken-down form; \"struct\n     credential\" would contain a url.\n\n  2. Pull out credential_from_url into \"int url_parse(struct url *,\n     const char *)\".\n\n  3. Pull out credential_match into \"int url_match(struct url *, struct\n     url *)\".\n\n  4. Parse and compare URLs from \"http.$URL.*\" during the config read\n     (similar to credential_config_callback).\n\nThat does make your by-length ordering impossible, but I don't think you\ncan do it in the general case. If I have:\n\n  [http \"http://peff@example.com\"] foo = 1\n  [http \"http://example.com:8080\"] foo = 2\n\nand I visit \"http://peff@example.com:8080\", which one is the winner? I\ndon't think there is an unambiguous definition. I'd suggest instead just\nusing the usual \"last one wins\" strategy that our config uses. It has\nthe unfortunate case that:\n\n  [http \"http://example.com\"]\n     foo = 1\n  [http]\n     foo = 2\n\nwill always choose http.foo=2, but I don't think that is a big problem\nin practice. You often only set one or the other, and in the cases where\nyou want to have a non-standard default and then override it with\nanother host-specific case, the \"last one wins\" rule makes it simple to\nexplain that you need to specify keys in most-general to most-specific\norder.\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\nNo need to cast from void, this isn't C++. :)\n\nThe rest of the http_options() changes look like what I'd expect.\n\n> @@ -344,7 +479,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\nNo need to cast again. :)\n\nSo this is the URL that we got on the command line. Which means that if\nwe get a redirect, we will not re-examine the options. I think that's\nOK; we do not even _see_ the redirect, as it happens internally within\ncurl. The credential code has the same problem, but it is not a security\nissue because curl clears the credentials on redirect.\n\nHowever, it may be worth mentioning in the documentation that the config\noptions operate on the URL you give git, _not_ necessarily on the actual\nURL you are hitting at any given moment (the gitcredentials(7) page\nshould probably make the same distinction).\n\n-Peff\n"},{"id":"223164","messageId":"F5272E14-188E-4199-9523-D2ED66574D91@gmail.com","threadId":"34406","inReplyTo":"20130712095923.GA4695@sigill.intra.peff.net","subject":"Re: [PATCH v3] config: add support for http.<url>.* settings","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2013-07-12T13:07:35Z","receivedAt":"2013-07-12T13:07:35Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Jul 12, 2013, at 02:59, Jeff King wrote:\n> On Thu, Jul 11, 2013 at 02:50:03PM -0700, Kyle J. McKay wrote:\n>\n> A few comments on the implementation:\n>\n>> +enum http_option_type {\n>> +\topt_post_buffer = 0,\n>> +\topt_min_sessions,\n>\n> We usually put enum fields in ALL_CAPS to mark them as constants  \n> (though\n> you can find few exceptions in the code).\n\nOK.\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>\n> It looks like you're matching the URLs as raw strings, and I don't see\n> any canonicalization going on. What happens if I have\n> \"https://example.com/foo+bar\" in my config, but then I visit\n> \"https://example.comfoo%20bar\"?\n\nThe documentation for http.<url>.* says:\n\n>> +http.<url>.*::\n> [snip]\n>> +       Note that <url> must match the url exactly (other than  \n>> possibly being a\n>> +       prefix).  This means any user, password and/or port setting  \n>> that appears\n>> +       in a url must also appear in <url> to have a successful  \n>> match.\n>> +\n\n> Or what about optional components? If I have \"https://example.com\"  \n> in my\n> config, but I am visiting \"https://peff@example.com/\", shouldn't it\n> match? The config spec is more general than my specific URL; you would\n> not want it to match in the opposite direction, though.\n\nThey have to be included to match.  Any URL-encoding present in the  \nURL given to git needs to be duplicated in the URL in the config file  \nexactly or there will not be a match.\n\n> That does make your by-length ordering impossible, but I don't think  \n> you\n> can do it in the general case. If I have:\n>\n>  [http \"http://peff@example.com\"] foo = 1\n>  [http \"http://example.com:8080\"] foo = 2\n>\n> and I visit \"http://peff@example.com:8080\", which one is the winner?\n\nIf I were to spilt everything out, then I would only have the second  \none match.  The first config is on a different port, quite possibly an  \nentirely different service.  It shouldn't match.  Consider if you were  \nport forwarding with ssh, services from many different machines would  \nall be on localhost on different ports.  The second one is on the same  \nport and matches because it's slightly more general (missing the user  \nname), but it's clearly talking to the same service.\n\nAs the patch stands now, neither of them would match since the  \ndocumentation requires an exact match (except for possibly being a  \nprefix).\n\nI don't think it's necessary to split the URL apart though.  Whatever  \nURL the user gave to git on the command line (at some point even if  \nit's now stored as a remote setting in config) complete with URL- \nencoding, user names, port names, etc. is the same url, possibly  \nshortened, that needs to be used for the http.<url>.option setting.\n\nI think that's simple and very easy to explain and avoids user  \nconfusion and surprise while still allowing a default to be set for a  \nsite but easily overridden for a portion of that site without needing  \nto worry about the order config files are processed or the order of  \nthe [http \"<url>\"] sections within them.\n\n> I don't think there is an unambiguous definition. I'd suggest  \n> instead just\n> using the usual \"last one wins\" strategy that our config uses. It has\n> the unfortunate case that:\n>\n>  [http \"http://example.com\"]\n>     foo = 1\n>  [http]\n>     foo = 2\n>\n> will always choose http.foo=2, but I don't think that is a big problem\n> in practice.\n\nI think that violates the principle of least surprise.  In this case:\n\n[http \"https://cacert.org\"]\n   sslVerify = false\n[http]\n   sslVerify = true\n\nthe intention here is very clear -- for cacert.org only, sslVerify  \nshould be false.  To have the [http] setting step on the [http \"https://cacert.org \n\"] setting seems completely surprising and unexpected.\n\nThe \"last one wins\" is still in effect though for the same paths.  If  \nyou have:\n\n# System config\n[http \"https://cacert.org\"]\n   sslVerify = false\n\n# Global config\n[http \"https://cacert.org\"]\n   sslVerify = true\n\nThe setting from the Global config still wins.\n\n> You often only set one or the other, and in the cases where\n> you want to have a non-standard default and then override it with\n> another host-specific case, the \"last one wins\" rule makes it simple  \n> to\n> explain that you need to specify keys in most-general to most-specific\n> order.\n\nUnless, of course, different config files are involved and that's not  \npossible without duplicating entries into the later-processed config  \nfile.\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>\n> No need to cast from void, this isn't C++. :)\n\nIndeed.\n\n> The rest of the http_options() changes look like what I'd expect.\n>\n>> @@ -344,7 +479,7 @@ void http_init(struct remote *remote, const  \n>> 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> No need to cast again. :)\n\nAh, but there is in order to avoid a warning:\n\nhttp.c: In function ‘http_init’:\nhttp.c:481: warning: passing argument 2 of ‘git_config’ discards  \nqualifiers from pointer target type\n\nurl is a const char * and git_config takes a void *.\n\n> However, it may be worth mentioning in the documentation that the  \n> config\n> options operate on the URL you give git, _not_ necessarily on the  \n> actual\n> URL you are hitting at any given moment (the gitcredentials(7) page\n> should probably make the same distinction).\n\nCan do that.  Will have to think about the wording for a while and  \nmention the %XX escapes as well."},{"id":"223219","messageId":"20130712205843.GJ4604@pug.qqx.org","threadId":"34406","inReplyTo":"F5272E14-188E-4199-9523-D2ED66574D91@gmail.com","subject":"Re: [PATCH v3] config: add support for http.<url>.* settings","fromName":"Aaron Schrab","fromEmail":"aaron@schrab.com","sentAt":"2013-07-12T20:58:44Z","receivedAt":"2013-07-12T20:58:44Z","isPatch":true,"sender":{"key":"aaron@schrab.com","avatar":"https://avatars.githubusercontent.com/u/39620?v=4"},"body":"At 06:07 -0700 12 Jul 2013, \"Kyle J. McKay\" <mackyle@gmail.com> wrote:\n>I don't think it's necessary to split the URL apart though.  Whatever \n>URL the user gave to git on the command line (at some point even if \n>it's now stored as a remote setting in config) complete with URL-\n>encoding, user names, port names, etc. is the same url, possibly \n>shortened, that needs to be used for the http.<url>.option setting.\n\nThis seems to be assuming that the configuration is done after the URL \nis entered and that URLs are always entered manually.  I don't think \neither of those assumptions is valid.  A user may want to specify http \nsettings for all repositories on a specified host and so add settings \nfor that host to ~/.gitconfig expecting those settings to be used later.  \nA URL in a slightly different format may later be copy+pasted without \nthe user realizing that it won't use that config due to one of the \nversions being in a non-canonical form.\n\n>I think that's simple and very easy to explain and avoids user \n>confusion and surprise while still allowing a default to be set for a \n>site but easily overridden for a portion of that site without needing \n>to worry about the order config files are processed or the order of the \n>[http \"<url>\"] sections within them.\n\nI agree that the method is easy to explain, but I think a user may very \nwell be surprised and confused in a scenario like I described above.  \nAnd having the order not matter (in some cases) for these configuration \nitems deviates from how other configuration values are handled.\n"},{"id":"223251","messageId":"47B58075-3FDD-48E5-9047-8650F7FC5E3B@gmail.com","threadId":"34406","inReplyTo":"7v4nbyic57.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] config: add support for http.<url>.* settings","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2013-07-13T19:46:17Z","receivedAt":"2013-07-13T19:46:17Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Jul 12, 2013, at 13:58, Aaron Schrab wrote:\n> At 06:07 -0700 12 Jul 2013, \"Kyle J. McKay\" <mackyle@gmail.com> wrote:\n>> I don't think it's necessary to split the URL apart though.   \n>> Whatever URL the user gave to git on the command line (at some  \n>> point even if it's now stored as a remote setting in config)  \n>> complete with URL-\n>> encoding, user names, port names, etc. is the same url, possibly  \n>> shortened, that needs to be used for the http.<url>.option setting.\n>\n> This seems to be assuming that the configuration is done after the  \n> URL is entered and that URLs are always entered manually.  I don't  \n> think either of those assumptions is valid.  A user may want to  \n> specify http settings for all repositories on a specified host and  \n> so add settings for that host to ~/.gitconfig expecting those  \n> settings to be used later.  A URL in a slightly different format may  \n> later be copy+pasted without the user realizing that it won't use  \n> that config due to one of the versions being in a non-canonical form.\n\nThat seems like a very reasonable expectation to me.\n\n>> I think that's simple and very easy to explain and avoids user  \n>> confusion and surprise while still allowing a default to be set for  \n>> a site but easily overridden for a portion of that site without  \n>> needing to worry about the order config files are processed or the  \n>> order of the [http \"<url>\"] sections within them.\n>\n> I agree that the method is easy to explain, but I think a user may  \n> very well be surprised and confused in a scenario like I described  \n> above.  And having the order not matter (in some cases) for these  \n> configuration items deviates from how other configuration values are  \n> handled.\n\n\nOn Jul 13, 2013, at 10:48, Junio C Hamano wrote:\n> The only remaining issue is if matching strictly at the textual\n> level is too limiting.  I personally have no strong opinion myself\n> on it, and if we start with a limited form, we can always loosen it\n> later, so...\n\n\nThe full on everything is to split the URL into all its pieces,  \ncanonizing (according to RFC 1738) the pieces and probably allowing  \nomitted parts to act like wildcards.  I'm not opposed to doing this  \nwork if that's the consensus.\n\nHowever, there's probably a shortcut to all that work that will  \naddress Aaron's concern.\n\nI expect it will be easier just to normalize the URL without  \nsplitting.  That is, lowercase the parts that are case-insensitive  \n(scheme and host name) and adjust the URL-escaping to remove URL  \nescaping (%xx) from characters that don't need it but add it to any  \nfor which it is required that are not escaped (according to RFC 1738).\n\nBasically a url_normalize function added to url.{h,c}.  It can take a  \nconst char * and return a char * that needs to be free'd.  (Perhaps it  \nshould live in http.c until some other file needs to use it and  \nmigrate then?)\n\nThis should guarantee a match in the scenario Aaron proposes above and  \nstill has pretty much the same easy explanation to the user.\n\nShall I go ahead and add that to the next patch version?\n\nOr proceed with what's there right now (there are a few pending  \nupdates from reviewers) and then, as Junio says above, adjust it later  \nif needed?\n"},{"id":"223369","messageId":"7v8v18fp2s.fsf@alter.siamese.dyndns.org","threadId":"34406","inReplyTo":"47B58075-3FDD-48E5-9047-8650F7FC5E3B@gmail.com","subject":"Re: [PATCH v3] config: add support for http.<url>.* settings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-15T04:02:19Z","receivedAt":"2013-07-15T04:02:19Z","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> On Jul 12, 2013, at 13:58, Aaron Schrab wrote:\n> ...\n> This should guarantee a match in the scenario Aaron proposes above and\n> still has pretty much the same easy explanation to the user.\n>\n> Shall I go ahead and add that to the next patch version?\n>\n> Or proceed with what's there right now (there are a few pending\n> updates from reviewers) and then, as Junio says above, adjust it later\n> if needed?\n\nI have been assuming that \"strictly textual match\" will be a subset\nof the matching semantics Aaron and Peff suggested.  That is, if we\ninclude your version in the upcoming release, the user writes the\nhttp.<URLpattern>.<variable> configuration so that the entries match\nwhat they want them to match, the enhanced URL matcher Aaron and\nPeff suggested will still make them match.\n\nAm I mistaken?  Will there be some <URLpattern> that will not match\nwith the same URL literally?\n\nAssuming that Aaron and Peff's enhancement will not be a backward\nincompatible update, my preference is to take the posted matching\nsemantics as-is (you may have some other changes that does not\nchange the \"strictly textual match\" semantics).\n\nI do not have strong opinion but Aaron and Peff seem to know what\nthey are talking about, so they will be a better guide to work with\nyou and polish such enhancement on top of what you have now, as a\nseparate change that may need more time to mature.\n"},{"id":"223376","messageId":"20130715044337.GA21127@sigill.intra.peff.net","threadId":"34406","inReplyTo":"F5272E14-188E-4199-9523-D2ED66574D91@gmail.com","subject":"Re: [PATCH v3] config: add support for http.<url>.* settings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-15T04:43:38Z","receivedAt":"2013-07-15T04:43:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 12, 2013 at 06:07:35AM -0700, Kyle J. McKay wrote:\n\n> >It looks like you're matching the URLs as raw strings, and I don't see\n> >any canonicalization going on. What happens if I have\n> >\"https://example.com/foo+bar\" in my config, but then I visit\n> >\"https://example.comfoo%20bar\"?\n> \n> The documentation for http.<url>.* says:\n\nYes, I know. My question was more rhetorical, as in \"what _should_\nhappen\". It seems unfriendly not to do at least some basic\ncanonicalization with respect to encodings.\n\n> >That does make your by-length ordering impossible, but I don't\n> >think you\n> >can do it in the general case. If I have:\n> >\n> > [http \"http://peff@example.com\"] foo = 1\n> > [http \"http://example.com:8080\"] foo = 2\n> >\n> >and I visit \"http://peff@example.com:8080\", which one is the winner?\n> \n> If I were to spilt everything out, then I would only have the second\n> one match.  The first config is on a different port, quite possibly\n> an entirely different service.  It shouldn't match.\n\nYeah, sorry, that was a bad example, because a missing port is not\n\"do not care\", but rather \"default port\". A better example is:\n\n  [http \"http://peff@example.com/\"] foo = 1\n  [http \"http://example.com/path/\"] foo = 2\n\nIf we see the URL \"http://peff@example.com/path/foo\", I would expect the\nsecond one to match (it does not under a pure-prefix scheme). If we were\nto make it match, you cannot say which of the two entries is \"more\nspecific\" they are both specific in different ways.\n\n> I don't think it's necessary to split the URL apart though.  Whatever\n> URL the user gave to git on the command line (at some point even if\n> it's now stored as a remote setting in config) complete with URL-\n> encoding, user names, port names, etc. is the same url, possibly\n> shortened, that needs to be used for the http.<url>.option setting.\n\nI'm not sure I agree with this. The URL handed to git is not always\ntyped directly by the user. E.g., it might be cut-and-paste from an\nemail or website, or it may even be fed by a script (e.g., the \"repo\"\ntool will pull URLs from its manifest file).\n\n> I think that's simple and very easy to explain and avoids user\n> confusion and surprise while still allowing a default to be set for a\n> site but easily overridden for a portion of that site without needing\n> to worry about the order config files are processed or the order of\n> the [http \"<url>\"] sections within them.\n\nBut the user has to worry about last-one-wins anyway for same-length\nprefixes, as you noted below.\n\n> >using the usual \"last one wins\" strategy that our config uses. It has\n> >the unfortunate case that:\n> >\n> > [http \"http://example.com\"]\n> >    foo = 1\n> > [http]\n> >    foo = 2\n> >\n> >will always choose http.foo=2, but I don't think that is a big problem\n> >in practice.\n> \n> I think that violates the principle of least surprise.  In this case:\n> \n> [http \"https://cacert.org\"]\n>   sslVerify = false\n> [http]\n>   sslVerify = true\n\nSure, I think yours is less surprising in this case. But it is more\nsurprising in other cases, like ones where URL encoding or optional\ncomponents are involved.  E.g., imagine your two options above were\nflipped (you do not usually verify ssl, but it is very important for you\nto do so against cacert.org). An automated tool like repo tries to clone\nfrom https://user@cacert.org, and you might be surprised that SSL\nverification is not turned on.\n\n> >>-\tgit_config(http_options, NULL);\n> >>+\tgit_config(http_options, (void *)url);\n> >\n> >No need to cast again. :)\n> \n> Ah, but there is in order to avoid a warning:\n\nAh, you're right. Sorry for the noise.\n\n-Peff\n"},{"id":"223378","messageId":"20130715050654.GB21127@sigill.intra.peff.net","threadId":"34406","inReplyTo":"47B58075-3FDD-48E5-9047-8650F7FC5E3B@gmail.com","subject":"Re: [PATCH v3] config: add support for http.<url>.* settings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-15T05:06:54Z","receivedAt":"2013-07-15T05:06:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 13, 2013 at 12:46:17PM -0700, Kyle J. McKay wrote:\n\n> I expect it will be easier just to normalize the URL without\n> splitting.  That is, lowercase the parts that are case-insensitive\n> (scheme and host name) and adjust the URL-escaping to remove URL\n> escaping (%xx) from characters that don't need it but add it to any\n> for which it is required that are not escaped (according to RFC\n> 1738).\n\nI think you are suggesting doing better than this, but just to be clear,\nwe cannot treat the URL as a simple string and just decode and\nre-encode.\n\nOne of the things that gets encoded are the delimiting characters. So if\nI have the URL:\n\n  https://foo%3abar@example.com\n\nyou would \"canonicalize\" it into:\n\n  https://foo:bar@example.com\n\nBut those are two different URLs entirely; the first has the username\n\"foo:bar\", and the second has the username \"foo\" and the password \"bar\".\n\nI admit that these are unlikely to come up in practice, but I am worried\nthat there is some room for mischief here. For example:\n\n  https://example.com%2ftricky.host/repo.git\n\nIf we canonicalize that into:\n\n  https://example.com/tricky.host/repo.git\n\nand do a lookup, we think we are hitting example.com, but we are\nactually hitting example.comtricky.host (i.e., that is how curl will\ninterpret it).  If we were deciding to use a stored credential based on\nthat information, it would be quite bad (we would leak credentials to\nthe owner of comtricky.host). I know your patch does not impact the\ncredential lookup behavior, but it would be nice in the long run if the\ntwo lookups followed the same rules.\n\nSo I think the three options are basically:\n\n  1. No decoding, require the user to use a consistent prefix between\n     config and other uses of the URL. I.e., your current patch. The\n     downside is that it doesn't handle any variation of input.\n\n  2. Full decoding into constituent parts. This handles canonicalization\n     of encoding, and also allows \"wildcard\" components (e.g., a URL\n     with username can match the generic \"https://example.com\" in the\n     config). The downside is that you cannot do a \"longest prefix wins\"\n     rule for overriding.\n\n  3. Full decoding as in (2), but then re-assemble into a canonicalized\n     encoded URL. The upside is that you get to do \"longest prefix\n     wins\", but you can no longer have wildcard components. I think this\n     is what you are suggesting in your mail.\n\nI'm still in favor of (2), because I think the wildcard components are\nimportant (and while I agree that the \"longest prefix wins\" is nicer, we\nalready have \"last one wins\" for the rest of the config, including the\ncredential URL matcher). But I certainly think (3) is better than (1).\n\n-Peff\n"},{"id":"223379","messageId":"20130715051233.GC21127@sigill.intra.peff.net","threadId":"34406","inReplyTo":"7v8v18fp2s.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] config: add support for http.<url>.* settings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-15T05:12:33Z","receivedAt":"2013-07-15T05:12:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 14, 2013 at 09:02:19PM -0700, Junio C Hamano wrote:\n\n> > Or proceed with what's there right now (there are a few pending\n> > updates from reviewers) and then, as Junio says above, adjust it later\n> > if needed?\n> \n> I have been assuming that \"strictly textual match\" will be a subset\n> of the matching semantics Aaron and Peff suggested.  That is, if we\n> include your version in the upcoming release, the user writes the\n> http.<URLpattern>.<variable> configuration so that the entries match\n> what they want them to match, the enhanced URL matcher Aaron and\n> Peff suggested will still make them match.\n> \n> Am I mistaken?  Will there be some <URLpattern> that will not match\n> with the same URL literally?\n\nI think we need to decide now, because the two schemes are not\ncompatible, and switching will break setups. Yes, the matcher that Aaron\nand I suggest is a strict superset (i.e., we will not stop matching\nthings that used to match), which is good. But we would not be able to\nimplement \"longest prefix wins\" overriding anymore, which would change\nthe meaning of cases like:\n\n  [http \"https://example.com\"] foo = 1\n  [http] foo = 2\n\n(under Kyle's scheme it is \"1\", and under ours \"2\"). We can probably\ncome up with some clever rules for overriding a broken-down URL that\nwould stay backwards compatible. E.g., longest-prefix-match if there are\nno wildcarded components, and last-one-wins if there are. But that is\nnot a rule I would want readers to have to puzzle out in the\ndocumentation.\n\nSo I think we are much better off to decide the semantics now.\n\n-Peff\n"},{"id":"223408","messageId":"CDCD6E5C-A829-43A1-94F6-93670A339A35@gmail.com","threadId":"34406","inReplyTo":"20130715051233.GC21127@sigill.intra.peff.net","subject":"Re: [PATCH v3] config: add support for http.<url>.* settings","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2013-07-15T09:50:48Z","receivedAt":"2013-07-15T09:50:48Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"(I'm attempting to combine the various separate email replies into a  \nsingle response here, please forgive me if I mangle something up.)\n\n\n\nOn Jul 14, 2013, at 22:12, Jeff King wrote:\n> On Sun, Jul 14, 2013 at 09:02:19PM -0700, Junio C Hamano wrote:\n>\n>>> Or proceed with what's there right now (there are a few pending\n>>> updates from reviewers) and then, as Junio says above, adjust it  \n>>> later\n>>> if needed?\n>>\n>> I have been assuming that \"strictly textual match\" will be a subset\n>> of the matching semantics Aaron and Peff suggested.  That is, if we\n>> include your version in the upcoming release, the user writes the\n>> http.<URLpattern>.<variable> configuration so that the entries match\n>> what they want them to match, the enhanced URL matcher Aaron and\n>> Peff suggested will still make them match.\n>>\n>> Am I mistaken?  Will there be some <URLpattern> that will not match\n>> with the same URL literally?\n>\n> I think we need to decide now, because the two schemes are not\n> compatible, and switching will break setups. Yes, the matcher that  \n> Aaron\n> and I suggest is a strict superset (i.e., we will not stop matching\n> things that used to match), which is good. But we would not be able to\n> implement \"longest prefix wins\" overriding anymore, which would change\n> the meaning of cases like:\n>\n>  [http \"https://example.com\"] foo = 1\n>  [http] foo = 2\n>\n> (under Kyle's scheme it is \"1\", and under ours \"2\"). We can probably\n> come up with some clever rules for overriding a broken-down URL that\n> would stay backwards compatible. E.g., longest-prefix-match if there  \n> are\n> no wildcarded components, and last-one-wins if there are. But that is\n> not a rule I would want readers to have to puzzle out in the\n> documentation.\n>\n> So I think we are much better off to decide the semantics now.\n\nYes.  Consider these two commands:\n\n> git config http.foo = 2\n> git config http.https://example.com/.foo = 1\n\nI am proposing that this means https://example.com has foo set to 1  \n(assuming there are no other http*.foo configurations).\n\nThe other proposal probably means that, but it might not.\n\nGiven this sequence:\n\n> git config http.foo = 2\n> git config http.https://example.com/.foo = 1\n>\n\nor this sequence:\n\n> git config http.https://example.com/.foo = 1\n> git config http.foo = 2\n\nwhat actually happens when using the other proposal will depend on  \nwhether or not the user has previously configured any other \"http.*\"  \nsetting or any other \"http.https://example.com/.*\" setting since doing  \nso would have established such a section in the config file and it  \nwill affect the order the directives are processed since they will be  \nadded to the existing section rather than creating a new same-named  \nsection at the end of the file.\n\nFor this reason the order the above two \"git config\" commands are  \ngiven cannot be relied upon to determine what setting http.foo will  \nhave when a https://example.com/ url is accessed.\n\nSince git config does not have a \"git config --placing-after-section  \nsection-name http.foo 2\" option it seems to me that the only way to be  \nsure what you would end up with using this method is to examine the  \ncreated config file (git config -l would be sufficient although  \nprobably harder to read than viewing the config file itself).\n\nFor an individual .git/config file I don't expect this to be an  \nissue.  However for the --global config file, I believe quite some  \ntime could go by between setting one http.* option and another so it  \nseems quite likely to me that the user may not remember or have ever  \nbeen aware of what order the various [http*] sections are currently in.\n\nThis is why I originally proposed longest-match-wins semantics, but  \nplease read on.\n\n\n\nOn Jul 14, 2013, at 22:06, Jeff King wrote:\n> I admit that these are unlikely to come up in practice, but I am  \n> worried\n> that there is some room for mischief here. For example:\n>\n>  https://example.com%2ftricky.host/repo.git\n\nThis is actually an invalid URL.  Host names may not contain '%'  \ncharacters and can only be followed by optionally ':port' and then one  \nof '/', '?', or '#'.  A URL parser would actually die when it sees the  \n'%' there as there's no way to match that to the URL grammar rules.   \n(It wouldn't actually be taken as part of the host name even though it  \nmay appear to be.)\n\n> If we canonicalize that into:\n>\n>  https://example.com/tricky.host/repo.git\n\nI don't think this will be a concern since that is an invalid  \nnormalization.  Perhaps there is another example that is also  \nconcerning that needs to be addressed?\n\n\n> One of the things that gets encoded are the delimiting characters.  \n> So if\n> I have the URL:\n>\n>  https://foo%3abar@example.com\n>\n> you would \"canonicalize\" it into:\n>\n>  https://foo:bar@example.com\n>\n> But those are two different URLs entirely; the first has the username\n> \"foo:bar\", and the second has the username \"foo\" and the password  \n> \"bar\".\n\nThat would be an incorrect normalization according to RFC 3986.  '@',  \n'/' and ':' must be escaped in the user:password portion and decoding  \nthem there is verboten (prohibited).  And delimiters in general may  \nnot have their escaping changed during normalization.\n\n> So I think the three options are basically:\n>\n>  1. No decoding, require the user to use a consistent prefix between\n>     config and other uses of the URL. I.e., your current patch. The\n>     downside is that it doesn't handle any variation of input.\n\nI have previously agreed with Aaron about this and am no longer  \nproposing this.\n\n>\n>  2. Full decoding into constituent parts. This handles  \n> canonicalization\n>     of encoding, and also allows \"wildcard\" components (e.g., a URL\n>     with username can match the generic \"https://example.com\" in the\n>     config). The downside is that you cannot do a \"longest prefix  \n> wins\"\n>     rule for overriding.\n>\n>  3. Full decoding as in (2), but then re-assemble into a canonicalized\n>     encoded URL. The upside is that you get to do \"longest prefix\n>     wins\", but you can no longer have wildcard components. I think  \n> this\n>     is what you are suggesting in your mail.\n>\n> I'm still in favor of (2), because I think the wildcard components are\n> important (and while I agree that the \"longest prefix wins\" is  \n> nicer, we\n> already have \"last one wins\" for the rest of the config, including the\n> credential URL matcher). But I certainly think (3) is better than (1).\n\nAIUI, currently \"last one wins\" only applies to same-named options.   \nThat is, if I have these:\n\n> [svn-remote \"svn\"]\n> \tuseSvnsyncProps = true\n> [svn]\n> \tuseSvnsyncProps = false\n\nWhen fetching using git-svn, useSvnsyncProps will be true since \"svn- \nremote.svn.useSvnsyncProps\" and \"svn.useSvnsyncProps\" are different  \nproperty names.\n\n\n\nI think we've agreed that any matches must match:\n\n1) the scheme (http:,https:,...)\n\n2) the host name\n\n3) the port number\n\n4) at least a prefix of the path (that does not seem to be in  \nquestion, the question seems to be whether or not longest match wins  \nor last seen when processed by git_config() wins)\n\nIn that case the only wildcard in question is the user name.  (I don't  \nthink anyone is proposing matching on the user password when it has  \nbeen included directly in the URL.)\n\n\n\nI propose that:\n\n1) URLs be normalized before attempting any matching (RFC 3986 rules  \nhere)\n\n2) Allow a config <url> without a user to match a given url with a  \nuser (otherwise, the user part must match exactly)\n\n2) Longest length path match wins (to avoid users needing to be  \ncognizant of the actual order sections appear in their config file)\n\n3) If there are multiple same-path-length matches, the last one  \nencountered wins except that exact user matches are preferred over  \nmatches where the url contains a user but the config <url> does not.\n\n\n\nOn Jul 14, 2013, at 21:02, Junio C Hamano wrote:\n> \"Kyle J. McKay\" <mackyle@gmail.com> writes:\n>\n>> On Jul 12, 2013, at 13:58, Aaron Schrab wrote:\n>> ...\n>> This should guarantee a match in the scenario Aaron proposes above  \n>> and\n>> still has pretty much the same easy explanation to the user.\n>>\n>> Shall I go ahead and add that to the next patch version?\n>>\n>> Or proceed with what's there right now (there are a few pending\n>> updates from reviewers) and then, as Junio says above, adjust it  \n>> later\n>> if needed?\n>\n> I have been assuming that \"strictly textual match\" will be a subset\n> of the matching semantics Aaron and Peff suggested.  That is, if we\n> include your version in the upcoming release, the user writes the\n> http.<URLpattern>.<variable> configuration so that the entries match\n> what they want them to match, the enhanced URL matcher Aaron and\n> Peff suggested will still make them match.\n\nYes.  Normalizing the URLs will only create more matches, it will not  \ncause any current matches to stop matching.\n\n> Am I mistaken?  Will there be some <URLpattern> that will not match\n> with the same URL literally?\n\nI believe you are correct.\n\n> Assuming that Aaron and Peff's enhancement will not be a backward\n> incompatible update, my preference is to take the posted matching\n> semantics as-is (you may have some other changes that does not\n> change the \"strictly textual match\" semantics).\n\n\n\nSo along these lines I have prepared a forthcoming [PATCH v5] that  \nadds url normalization before the comparisons.  The url normalization  \nis added as a separate patch in the patch series on top of the textual  \nmatching with a test on top of that (it also includes the \"fix parsing  \nof http.sslCertPasswordProtected\" change as a preparatory patch).\n\nThe forthcoming patch does not include wildcard user matching, but the  \nsame principle applies in that adding wildcard user matching in the  \nfuture will only create more matches without causing any current  \nmatches to stop matching.\n\nI appreciate the time you reviewers have spent looking at these  \npatches and sending feedback.  I would like to see this recognized in  \nthe patch with a suitable annotation (Reviewed-By: or Feedback-From:  \nor whatever's appropriate).  I am unclear on how to make this happen  \nsince, as has been pointed out, I cannot actually include a 'Reviewed- \nBy:' line in a new patch I send out since such a new patch has not yet  \nactually been reviewed no matter the content.\n\nThanks for your help,\nKyle\n"}]}