[+cc Ilari, who wrote the code originally]
On Mon, Sep 26, 2011 at 09:52:54AM -0700, Junio C Hamano wrote:
> Tay Ray Chuan <rctay89@xxxxxxxxx> writes:
>
> > On Sun, Sep 25, 2011 at 1:06 PM, Pang Yan Han <pangyanhan@xxxxxxxxx> wrote:
> >> Signed-off-by: Pang Yan Han <pangyanhan@xxxxxxxxx>
> >> ---
> >> url.c | 1 -
> >> 1 files changed, 0 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/url.c b/url.c
> >> index 3e06fd3..d2e17e6 100644
> >> --- a/url.c
> >> +++ b/url.c
> >> @@ -22,7 +22,6 @@ int is_url(const char *url)
> >>
> >> if (!url)
> >> return 0;
> >> - url2 = url;
> >> first_slash = strchr(url, '/');
> >>
> >> /* Input with no slash at all or slash first can't be URL. */
> >
> > Looks correct. Perhaps you could mention in the patch message that
> >
> > There are no operations on url2 until another assignment to it later
> > at line 41.
>
> The looks correct, so I'll queue it, but it looks like that the function
> is implemented in an overly complicated way.
>
> Why aren't we checking from left to right in a single pass, perhaps like
> this?
>
> /* Make sure it is of form "scheme://something" */
> int is_url(const char *url)
> {
> /* Is "scheme" part reasonable? */
> if (!url || !is_urlschemechar(1, *url++))
> return 0;
> while (*url && *url != ':') {
> if (!is_urlschemechar(0, *url++))
> return 0;
> }
> /* We've seen "scheme"; we want colon-slash-slash */
> return (url[0] == ':' && url[1] == '/' && url[2] == '/');
> }
That looks right to me, and is way more readable.
-Peff
--
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