"Kyle J. McKay" <mackyle@xxxxxxxxx> writes:
> The <url> value is considered a match to a url if the <url> value
> is either an exact match or a prefix of the url which ends on a
> path component boundary ('/'). So "https://example.com/test"; will
> match "https://example.com/test"; and "https://example.com/test/too";
> but not "https://example.com/testextra";.
>
> Longer matches take precedence over shorter matches with
> environment variable settings taking precedence over all.
>
> With this configuration:
>
> [http]
> useragent = other-agent
> noEPSV = true
> [http "https://example.com";]
> useragent = example-agent
> sslVerify = false
> [http "https://example.com/path";]
> useragent = path-agent
>
> The "https://other.example.com/"; url will have useragent
> "other-agent" and sslVerify will be on.
>
> The "https://example.com/"; url will have useragent
> "example-agent" and sslVerify will be off.
>
> The "https://example.com/path/sub"; url will have useragent
> "path-agent" and sslVerify will be off.
>
> All three of the examples will have noEPSV enabled.
>
> Signed-off-by: Kyle J. McKay <mackyle@xxxxxxxxx>
> Reviewed-by: Junio C Hamano <gitster@xxxxxxxxx>
I haven't reviewed this version (yet).
> ---
>
> The credentials configuration values already support url-specific
> configuration items in the form credential.<url>.*. This patch
> adds similar support for http configuration values.
Let's move the above three lines into the proposed log message and
make it its first paragraph. A log message should say what it is
about (i.e. what does "add support" really mean? what kind of
functionality the new support brings to us?) upfront and then
explain what it does in detail (i.e. the explanation of matching
semantics you have as the first paragraph).
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index b4d4887..3731a3a 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1531,6 +1531,17 @@ http.useragent::
> of common USER_AGENT strings (but not including those like git/1.7.1).
> Can be overridden by the 'GIT_HTTP_USER_AGENT' environment variable.
>
> +http.<url>.*::
> + Any of the http.* options above can be applied selectively to some urls.
> + For example "http.https://example.com.useragent"; would set the user
> + agent only for https connections to example.com. The <url> value
> + matches a url if it is an exact match or a prefix of the url matching
> + at a '/' boundary.
Given Peff's review, I am no longer sure if the "strictly textual
match" is the semantics we want. At least, it is easy to explain
and understand, but it might be too limiting to be useful.
Let's assume it is what we want, for the rest of the review.
> diff --git a/http.c b/http.c
> index 2d086ae..feca70a 100644
> --- a/http.c
> +++ b/http.c
> @@ -30,6 +30,34 @@ static CURL *curl_default;
>
> char curl_errorstr[CURL_ERROR_SIZE];
>
> +enum http_option_type {
> + opt_post_buffer = 0,
Do we need to have "= 0" here?
Is this order meant to match some externally defined order
(e.g. alphabetical, or the value of corresponding constants defined
in the cURL library), other than "opt_max is not a real option but
just has to be there at the end to count all of them"?
I am wondering if it would make it easier to read to move all the #ifdef
at the end immediately before opt-max, or something.
> + opt_min_sessions,
> +#ifdef USE_CURL_MULTI
> + opt_max_requests,
> +#endif
> +...
> + opt_user_agent,
> + opt_passwd_req,
> + opt_max
> +};
> +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). Upon seeing the _same_ key (i.e. new key
> + * has the same match length which is therefore not larger) the new
> + * setting will override the previous setting. Otherwise return false
> + * and record matchlen as the current last matched length of option opt.
> + */
> + if (http_option_max_matched_len[opt] > matchlen)
> + return 1;
> + http_option_max_matched_len[opt] = matchlen;
> + return 0;
> +}
A "check_foo" that returns either 0 or 1 usually mean 1 is good and
0 is not. A "check_foo" that returns either negative or 0 usually
mean negative is an error and 0 is good.
In general, "check_foo" is a bad name for a boolean function. This
checks "seen_longer_match()", and it is up to the caller if it
considers good or bad to have a matching element that is longer or
shorter what it has already seen. See below.
> static int http_options(const char *var, const char *value, void *cb)
> {
> - if (!strcmp("http.sslverify", var)) {
> + const char *url = (const char *)cb;
Cast?
> + if (!strcmp("sslverify", key)) {
> + if (check_matched_len(opt_ssl_verify, matchlen))
> + return 0;
> curl_ssl_verify = git_config_bool(var, value);
> return 0;
> }
At this caller, the name "check_matched_len()" is not immediately
obvious, other than it is checking matchlen, what it does. "check"
needs "what is checked" (i.e. "matchlen"), but it also needs "how it
judges what is checked" (i.e. is longer the better?), but the name
does not convey that. If you rename the function, it becomes a lot
easier to understand what is going on.
if (!strcmp("sslverify", key)) {
if (!seen_longer_match(opt_ssl_verify, matchlen))
curl_ssl_verify = git_config_bool(var, value);
return 0
}
> @@ -344,7 +478,7 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
>
> http_is_verbose = 0;
>
> - git_config(http_options, NULL);
> + git_config(http_options, (void *)url);
Cast?
--
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