"Kyle J. McKay" <mackyle@xxxxxxxxx> writes:
> +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.
> + 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html