From: Kyle J. McKay <mackyle@gmail.com>
Date: Thu, 11 Jul 2013 21:49:45 GMT
Subject: Re: [PATCH v2] config: add support for http.<url>.* settings
Message-ID: <072BFEEA-249D-4971-82D3-E76E6C19A303@gmail.com>
In-Reply-To: <7vd2qorj7s.fsf@alter.siamese.dyndns.org>

On Jul 11, 2013, at 12:26, Junio C Hamano wrote:
> "Kyle J. McKay" <mackyle@gmail.com> writes:
>
>> +static int http_option_maxlen[opt_max];
>
> I understand that this is to keep track of the length of the longest
> one that has matched (hence the current candidate).  The name "maxlen"
> captures the "longest" part, but "has matched" is somehow lost.
>
> Can we have a better name here please, or at least a comment to
> clarify what this variable keeps track of.

Will do.  How about "http_option_max_matched_len"?

>> +static int http_options_url_match_prefix(const char *url, const  
>> char *url_prefix)
>> +{
>> +	/*
>> +	 * 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.
>> +	 * url_prefix is considered to have an implicit '/' on the end for
>> +	 * matching purposes if it does not already and it is shorter  
>> than url.
>> +	 * the return value is the length of the match in characters  
>> (excluding
>> +	 * any final '/') or 0 for no match.
>> +	 */
>> +	size_t url_len, url_prefix_len = strlen(url_prefix);
>> +	if (!url_prefix_len || strncmp(url, url_prefix, url_prefix_len))
>> +		return 0;
>
> Should "url=http://git.or.xz/git" match "url_prefix=http://git.or.xz/git/ 
> "?

My initial thought was no.  But your example is persuasive and  
probably should match.  Fix forthcoming.

>> +	url_len = strlen(url);
>> +	if (url_len == url_prefix_len || url[url_prefix_len - 1] == '/'  
>> || url[url_prefix_len] == '/')
>> +		return url[url_prefix_len - 1] == '/' ? url_prefix_len - 1 :  
>> url_prefix_len;
>
> Overlong lines that are somewhat hard to read.

OK.

>> static int http_options(const char *var, const char *value, void *cb)
>> {
>> -	if (!strcmp("http.sslverify", var)) {
>> +/*
>> + * Macro to ignore matches with a match length less than  
>> previously seen
>> + * for the same option type and to remember the largest match  
>> length seen so
>> + * far for each option type
>> + */
>> +#define CHECK_MATCHLEN(opt) \
>> +	if (http_option_maxlen[opt] > matchlen) return 0; \
>> +	else http_option_maxlen[opt] = matchlen
>
> Avoid defining a macro _inside_ a function.  Also if you can make it
> a static function, that would be much easier to read.

Was modeled after the credential_match() function from credential.c  
that defines a CHECK macro inside it and then undefines it at the end  
of the function.  I will change it to a static function.

>> +	if (dot) {
>> +		char *config_url = xmemdupz(key, dot - key);
>> +		matchlen = http_options_url_match_prefix(url, config_url);
>> +		free(config_url);
>
> Yikes.  http_options_url_match_prefix() could take a counted string
> to config_url to avoid this repeated allocation and deallocation,
> no?

Again, modeled after the credential_config_callback() function from  
credential.c that does this exact thing.

I will update http_options_url_match_prefix to take a size_t.