Re: [PATCH v3] config: add support for http.<url>.* settings
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 11, 2013, 23:12 UTC
- Message-ID
- <7voba8pu6r.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <9e7edfbc83a7284615af4ca0de39c1b@f74d39fa044aa309eaea14b9f57fe79>
"Kyle J. McKay" <mackyle@gmail.com> writes:
Show 31 quoted lines
> +static size_t http_option_max_matched_len[opt_max];
> +
> static int curl_ssl_verify = -1;
> static int curl_ssl_try;
> static const char *ssl_cert;
> @@ -141,34 +169,122 @@ static void process_curl_messages(void)
> }
> #endif
>
> +static size_t http_options_url_match_prefix(const char *url,
> + const char *url_prefix,
> + size_t url_prefix_len)
> +{
> + /*
> + * url_prefix matches url if url_prefix is an exact match for url or it
> + * is a prefix of url and the match ends on a path component boundary.
> + * Both url and url_prefix are considered to have an implicit '/' on the
> + * end for matching purposes if they do not already.
> + *
> + * url must be NUL terminated. url_prefix_len is the length of
> + * url_prefix which need not be NUL terminated.
> + *
> + * The return value is the length of the match in characters (excluding
> + * any final '/') or 0 for no match. Passing "/" as url_prefix will
> + * always cause 0 to be returned.
> + */
> + size_t url_len;
> + if (url_prefix_len && url_prefix[url_prefix_len - 1] == '/')
> + url_prefix_len--;
> + if (!url_prefix_len || strncmp(url, url_prefix, url_prefix_len))
> + return 0;"URL=http://a.b/c/" vs "URLprefix=http://a.b/c/" would not return here, because we only check up to the length of the prefix after stripping the trailing slash from the prefix..
"URL=http://a.b/cd" vs "URLprefix=http://a.b/c/" would not return here, because we only check up to the length of the prefix after stripping the trailing slash from the prefix.
The latter needs to be rejected in the next condition.
> + url_len = strlen(url); > + if ( (url_len == url_prefix_len) ||
I thought your plan was that you wanted to treat "URL=http://a.b/c/" and "URL=http://a.b/c" the same way; taking strlen(url) and using it for comparison without adjusting for the trailing slash makes it smell somewhat fishy...
> + (url[url_prefix_len - 1] == '/') || > + (url[url_prefix_len] == '/') )
The prefix side no longer has slash at the end, and we know url and the prefix match up to the length of the prefix at this point. Can url[prefix-len - 1] ever be '/'? The latter (e.g. one past the prefix length can be '/' so that the prefix "http://a.b/c" can match against url "http://a.b/c/anything") makes sense, though.
It is not immediately obvious if this function is correct, at least to me.
Show 15 quoted lines
> + return url[url_prefix_len - 1] == '/'
> + ? url_prefix_len - 1 : url_prefix_len;
> + return 0;
> +}
> +
> +static int check_matched_len(enum http_option_type opt, size_t matchlen)
> +{
> + /*
> + * Check the last matched length of option opt against matchlen
> + * and return true if the last matched length is larger (meaning
> + * the config setting should be ignored). Otherwise return false
> + * and record matchlen as the last matched length of option opt.
> + */
> + if (http_option_max_matched_len[opt] > matchlen)
> + return 1;If you had
http."http://a.b/c".opt = A
in your ~/.gitconfig and then
http."http://a.b/c".opt = B
to override it for a particular repository's .git/config, then we read ~/.gitconfig first, remembering "http://a.b/c" has matched, and then we read .git/config, and encounter the same URLprefix. As the comparision is done with ">", not ">=", this allows the latter to override the former.
Which may deserve to be added to the comment, perhaps
... length is larger (meaning the config setting should be ignored). Upon seeing the _same_ key (i.e. new key is the same length as the longest match so far) is not ignored, but overrides the previous settings.
or something. It also would be nice to have a test to make sure this will not be broken in any future change.
Other than that, overall the change looks nice.