From: "Kyle J. McKay" <mackyle@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, "David Aguilar" <davvid@gmail.com>,
"Petr Baudis" <pasky@ucw.cz>, "Jeff King" <peff@peff.net>,
"Daniel Knittl-Frank" <knittl89@googlemail.com>,
"Jan Krüger" <jk@jk.gs>, "Alejandro Mery" <amery@geeks.cl>
Subject: Re: [PATCH v2] config: add support for http.<url>.* settings
Date: Thu, 11 Jul 2013 14:49:45 -0700 [thread overview]
Message-ID: <072BFEEA-249D-4971-82D3-E76E6C19A303@gmail.com> (raw)
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.
prev parent reply other threads:[~2013-07-11 21:49 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-07-11 16:47 [PATCH v2] config: add support for http.<url>.* settings Kyle J. McKay
2013-07-11 19:26 ` Junio C Hamano
2013-07-11 21:49 ` Kyle J. McKay [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=072BFEEA-249D-4971-82D3-E76E6C19A303@gmail.com \
--to=mackyle@gmail.com \
--cc=amery@geeks.cl \
--cc=davvid@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jk@jk.gs \
--cc=knittl89@googlemail.com \
--cc=pasky@ucw.cz \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).