Add a regexp to verify the host parameter.

Add a regexp to verify the host parameter.

If the host parameter is invalid, like having a trailing backslash, it will
result on HTTP 400 later which is much harder to diagnose.

[techtonik, 2010-11-23 16:33:32]

Is it possible to improve error message instead of embedding cryptic assertions?

[M-A, 2010-11-23 16:37:36]

I could use ErrorExit() instead, you prefer that?

[techtonik, 2010-11-23 17:21:20]

On 2010/11/23 16:37:36, M-A wrote:
> I could use ErrorExit() instead, you prefer that?

If HTTP 400 error is shown when server endpoint is wrong, maybe it is possible
to explicitly check if hostname is valid? For example, provide an "echo" service
and if its response is wrong, show error message about invalid hostname.

[M-A, 2010-11-23 17:36:04]

On 2010/11/23 17:21:20, techtonik wrote:
> On 2010/11/23 16:37:36, M-A wrote:
> > I could use ErrorExit() instead, you prefer that?
> 
> If HTTP 400 error is shown when server endpoint is wrong, maybe it is possible
> to explicitly check if hostname is valid? For example, provide an "echo"
service
> and if its response is wrong, show error message about invalid hostname.

I think setting up a echo service is overdesigning a simple typo verification
helper. I don't control google.com BTW. I don't mind how it errors out, as long
as it errors out early enough with a message about 'host %s is wrong'.

[Ilia Mirkin, 2010-12-05 16:34:17]

On 2010/11/23 17:36:04, M-A wrote:
> I think setting up a echo service is overdesigning a simple typo verification
> helper. I don't control http://google.com BTW. I don't mind how it errors out,
as long
> as it errors out early enough with a message about 'host %s is wrong'.

100% agree. I've also found upload.py errors to be fairly undecipherable in the
cases where "something funny" goes wrong. What error do you get now with an
invalid host? Would it be reasonable/beneficial to improve the error you
currently get instead of adding potentially bad parsing to the host?
(Invariably, someone's setting it to like user@password:[::1]:75 or something
insane like that.)

[M-A, 2010-12-05 18:31:08]

On 2010/12/05 16:34:17, Ilia Mirkin wrote:
> On 2010/11/23 17:36:04, M-A wrote:
> > I think setting up a echo service is overdesigning a simple typo
verification
> > helper. I don't control http://google.com BTW. I don't mind how it errors
out,
> as long
> > as it errors out early enough with a message about 'host %s is wrong'.
> 
> 100% agree. I've also found upload.py errors to be fairly undecipherable in
the
> cases where "something funny" goes wrong. What error do you get now with an
> invalid host? Would it be reasonable/beneficial to improve the error you
> currently get instead of adding potentially bad parsing to the host?
> (Invariably, someone's setting it to like user@password:[::1]:75 or something
> insane like that.)

The error in that case was a trailing slash, e.g.
http://codereview.chromium.org/ . I agree improving the error at the exception
site would be more interesting but even there it wouldn't be easy to figure out
what's wrong.

Note that using upload.py -s http://codereview.chromium.org/ doesn't reproduce
the issue, only using the interface directly did reproduce the bug.

In any case this change is low priority for me so I don't mind dropping it.

[imirkin_alum.mit.edu, 2010-12-05 18:36:21]

On Sun, Dec 5, 2010 at 1:31 PM,  <maruel@chromium.org> wrote:
> On 2010/12/05 16:34:17, Ilia Mirkin wrote:
> The error in that case was a trailing slash, e.g.
> http://codereview.chromium.org/ . I agree improving the error at the
> exception site would be more interesting but even there it wouldn't be
> easy to figure out what's wrong.

Well perhaps something on the order of "Could not connect to $url"
would be sufficient? I presume the issue was that something else
appends an extra /, and seeing a double-/ shoould be a dead give-away
as to the issue.

> Note that using upload.py -s http://codereview.chromium.org/ doesn't
> reproduce the issue, only using the interface directly did reproduce the
> bug.
>
> In any case this change is low priority for me so I don't mind dropping
> it.

OK. I do think that the error reporting needs to be improved. But you
don't have to be the one doing it. :)

-- 
Ilia Mirkin
imirkin@alum.mit.edu

[techtonik, 2010-12-12 00:53:04]

On 2010/12/05 18:31:08, M-A wrote:
> The error in that case was a trailing slash, e.g.
> http://codereview.chromium.org/ . I agree improving the error at the exception
> site would be more interesting but even there it wouldn't be easy to figure
out
> what's wrong.

Will this fix work? http://codereview.appspot.com/3591041/
 
> Note that using upload.py -s http://codereview.chromium.org/ doesn't reproduce
> the issue, only using the interface directly did reproduce the bug.

I can't reproduce the issue no matter how many trailing slashes I add to server
specified with -s option. What do you mean by "using interface directly"? I've
tried setting DEFAULT_REVIEW_SERVER, but it doesn't error either.

[M-A, 2010-12-13 15:33:42]

I'm cancelling this change.

[techtonik, 2010-12-14 09:33:07]

Ok. I hope it won't discourage from further contributions.