On Tue, Dec 08, 2009 at 03:35:17PM -0800, Junio C Hamano wrote:
> Ilari Liusvaara <ilari.liusvaara@xxxxxxxxxxx> writes:
>
> > +const char *remove_ext_force(const char *url)
> > +{
> > + const char *url2 = url;
> > + const char *first_colon = NULL;
> > +
> > + if (!url)
> > + return NULL;
> > +
> > + while (*url2 && !first_colon)
> > + if (*url2 == ':')
> > + first_colon = url2;
> > + else
> > + url2++;
> > +
> > + if (first_colon && first_colon[1] == ':')
> > + return first_colon + 2;
> > + else
> > + return url;
> > +}
>
> Sorry, I am slow today, but is this any different from:
>
> if (url) {
> const char *colon = strchr(url, ':');
> if (colon && colon[1] == ':')
> return url2 + 2;
> }
> return url;
Undefined variable url2 (you mean colon?). Changed.
> > diff --git a/transport.c b/transport.c
> > index 3eea836..e42a82b 100644
> > --- a/transport.c
> > +++ b/transport.c
> > @@ -780,6 +780,58 @@ static int is_file(const char *url)
> > return S_ISREG(buf.st_mode);
> > }
> >
> > +static const char *strchrc(const char *str, int c)
> > +{
> > + while (*str)
> > + if (*str == c)
> > + return str;
> > + else
> > + str++;
> > + return NULL;
> > +}
>
> I cannot spot how this is different from strchr()...
Return type. Not that useful. Removed.
> > +static int is_url(const char *url)
> > +{
> > + const char *url2, *first_slash;
> > +
> > + if (!url)
> > + return 0;
> > + url2 = url;
> > + first_slash = strchrc(url, '/');
> > +
> > + /* Input with no slash at all or slash first can't be URL. */
> > + if (!first_slash || first_slash == url)
> > + return 0;
> > + /* Character before must be : and next must be /. */
> > + if (first_slash[-1] != ':' || first_slash[1] != '/')
> > + return 0;
> > + /* There must be something before the :// */
> > + if (first_slash == url + 1)
> > + return 0;
> > + /*
> > + * Check all characters up to first slash. Only alpha, num and
> > + * colon (":") are allowed. ":" must be followed by ":" or "/".
> > + */
I tightened this a bit, only checking (exclusive) character before first
"/" and requiring all-alphanum. The previous rules were apparently
leftover from times the "specify remote helper by url" patch didn't
exist.
Considering context at call site, it is equivalent, since any ':' must be
followed by '/' or is_url will never be called, and first ':/' must be
exactly at last_slash - 1 (by earlier checks).
> Hmm, so "a::::bc:://" is ok?
is_url never gets such thing. Due to "::" it bypasses URL validation. And
yes, it is ok.
> Is the reason this does not to check the string up to (first_slash-1) for
> a valid syntax for <scheme> (as in "<scheme>://") because this potentially
> has "<helper>::" in front of it?
It doesn't have '<helper>::' in front of it.
> I cannot tell if you want to allow "<helper1>::<helper2>::<scheme>://..."
> by reading this code.
Allowing or denying that is not up to this function. And one can't chain
helpers that way. It invokes <helper1>
> > +static int external_specification_len(const char *url)
> > +{
> > + return strchrc(url, ':') - url;
> > +}
>
> Does the caller guarantee url has at least one colon in it?
Anything passing is_url() does have ':' in it.
> > + } else if (!is_url(url)
> > + || !prefixcmp(url, "file://")
> > + || !prefixcmp(url, "git://")
> > + || !prefixcmp(url, "ssh://")
> > + || !prefixcmp(url, "git+ssh://")
> > + || !prefixcmp(url, "ssh+git://")) {
> > + /* These are builtin smart transports. */
>
> Hmm, what is !is_url(url) at the beginning for, if this lists "builtin"
> smart transports?
It catches the scp and path syntaxes as not URLs.
> > + } else {
> > + /* Unknown protocol in URL. Pass to external handler. */
> > + int len = external_specification_len(url);
> > + char *handler = xmalloc(len + 1);
> > + handler[len] = 0;
> > + strncpy(handler, url, len);
> > + transport_helper_init(ret, handler);
>
> Hmph, external_specification_len() may get passed a colon-less string
> after all, I think.
Nope, it can't. Else if above snarfs everything that doesn't pass
is_url, and string can't pass it without having ':' in it.
-Ilari
--
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