From: Jeff King <peff@peff.net>
Date: Mon, 26 Sep 2011 21:32:16 GMT
Subject: Re: [PATCH/RFC 1/2] is_url: Remove redundant assignment
Message-ID: <20110926213216.GA5415@sigill.intra.peff.net>
In-Reply-To: <7vsjnj455l.fsf@alter.siamese.dyndns.org>

[+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@gmail.com> writes:
> 
> > On Sun, Sep 25, 2011 at 1:06 PM, Pang Yan Han <pangyanhan@gmail.com> wrote:
> >> Signed-off-by: Pang Yan Han <pangyanhan@gmail.com>
> >> ---
> >>  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