Patchwork scheme: don't crash on invalid URLs

login
register
mail settings
Submitter Mads Kiilerich
Date April 11, 2013, 8:35 p.m.
Message ID <6f4c6dc72f0f7cb83e26.1365712511@mk-desktop>
Download mbox | patch
Permalink /patch/1278/
State Accepted, archived
Delegated to: Augie Fackler
Headers show

Comments

Mads Kiilerich - April 11, 2013, 8:35 p.m.
# HG changeset patch
# User Mads Kiilerich <madski@unity3d.com>
# Date 1365684082 -7200
#      Thu Apr 11 14:41:22 2013 +0200
# Node ID 6f4c6dc72f0f7cb83e268741c98a61aefbb7e0f4
# Parent  50c464441c03d2c568d8ead7ab4f8a3a7c512d2f
scheme: don't crash on invalid URLs
Pierre-Yves David - April 11, 2013, 11:06 p.m.
On 11 avr. 2013, at 22:35, Mads Kiilerich wrote:

> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1365684082 -7200
> #      Thu Apr 11 14:41:22 2013 +0200
> # Node ID 6f4c6dc72f0f7cb83e268741c98a61aefbb7e0f4
> # Parent  50c464441c03d2c568d8ead7ab4f8a3a7c512d2f
> scheme: don't crash on invalid URLs
> 
> diff --git a/hgext/schemes.py b/hgext/schemes.py
> --- a/hgext/schemes.py
> +++ b/hgext/schemes.py
> @@ -62,7 +62,10 @@
> 
>     def instance(self, ui, url, create):
>         # Should this use the util.url class, or is manual parsing better?
> -        url = url.split('://', 1)[1]
> +        try:
> +            url = url.split('://', 1)[1]
> +        except IndexError:
> +            raise util.Abort(_("no '://' in scheme url '%s'") % url)

What about:

if '://' not in url:
    raise util.Abort(_("no '://' in scheme url '%s'") % url)

Sound clearer to me.
Mads Kiilerich - April 11, 2013, 11:51 p.m.
On 04/12/2013 01:06 AM, Pierre-Yves David wrote:
> On 11 avr. 2013, at 22:35, Mads Kiilerich wrote:
>
>> # HG changeset patch
>> # User Mads Kiilerich <madski@unity3d.com>
>> # Date 1365684082 -7200
>> #      Thu Apr 11 14:41:22 2013 +0200
>> # Node ID 6f4c6dc72f0f7cb83e268741c98a61aefbb7e0f4
>> # Parent  50c464441c03d2c568d8ead7ab4f8a3a7c512d2f
>> scheme: don't crash on invalid URLs
>>
>> diff --git a/hgext/schemes.py b/hgext/schemes.py
>> --- a/hgext/schemes.py
>> +++ b/hgext/schemes.py
>> @@ -62,7 +62,10 @@
>>
>>      def instance(self, ui, url, create):
>>          # Should this use the util.url class, or is manual parsing better?
>> -        url = url.split('://', 1)[1]
>> +        try:
>> +            url = url.split('://', 1)[1]
>> +        except IndexError:
>> +            raise util.Abort(_("no '://' in scheme url '%s'") % url)
> What about:
>
> if '://' not in url:
>      raise util.Abort(_("no '://' in scheme url '%s'") % url)
>
> Sound clearer to me.

It is the same to me. A simple type checker could prove that my version 
wouldn't raise exceptions - it is a lot harder to prove your version. My 
version also avoids a repeated string search and should be a bit faster. 
But not that it matters. Fine.

/Mads
Augie Fackler - April 12, 2013, 8:44 p.m.
On Fri, Apr 12, 2013 at 01:51:51AM +0200, Mads Kiilerich wrote:
> On 04/12/2013 01:06 AM, Pierre-Yves David wrote:
> >On 11 avr. 2013, at 22:35, Mads Kiilerich wrote:
> >
> >># HG changeset patch
> >># User Mads Kiilerich <madski@unity3d.com>
> >># Date 1365684082 -7200
> >>#      Thu Apr 11 14:41:22 2013 +0200
> >># Node ID 6f4c6dc72f0f7cb83e268741c98a61aefbb7e0f4
> >># Parent  50c464441c03d2c568d8ead7ab4f8a3a7c512d2f
> >>scheme: don't crash on invalid URLs
> >>
> >>diff --git a/hgext/schemes.py b/hgext/schemes.py
> >>--- a/hgext/schemes.py
> >>+++ b/hgext/schemes.py
> >>@@ -62,7 +62,10 @@
> >>
> >>     def instance(self, ui, url, create):
> >>         # Should this use the util.url class, or is manual parsing better?
> >>-        url = url.split('://', 1)[1]
> >>+        try:
> >>+            url = url.split('://', 1)[1]
> >>+        except IndexError:
> >>+            raise util.Abort(_("no '://' in scheme url '%s'") % url)
> >What about:
> >
> >if '://' not in url:
> >     raise util.Abort(_("no '://' in scheme url '%s'") % url)
> >
> >Sound clearer to me.
>
> It is the same to me. A simple type checker could prove that my version
> wouldn't raise exceptions - it is a lot harder to prove your version. My
> version also avoids a repeated string search and should be a bit faster. But
> not that it matters. Fine.

I'm fine with the patch as submitted, and it might be a little faster
to not look-before-you leap. Queued.

>
> /Mads
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff --git a/hgext/schemes.py b/hgext/schemes.py
--- a/hgext/schemes.py
+++ b/hgext/schemes.py
@@ -62,7 +62,10 @@ 
 
     def instance(self, ui, url, create):
         # Should this use the util.url class, or is manual parsing better?
-        url = url.split('://', 1)[1]
+        try:
+            url = url.split('://', 1)[1]
+        except IndexError:
+            raise util.Abort(_("no '://' in scheme url '%s'") % url)
         parts = url.split('/', self.parts)
         if len(parts) > self.parts:
             tail = parts[-1]
diff --git a/tests/test-schemes.t b/tests/test-schemes.t
--- a/tests/test-schemes.t
+++ b/tests/test-schemes.t
@@ -14,6 +14,15 @@ 
   $ echo a > a
   $ hg ci -Am initial
   adding a
+
+invalid scheme
+
+  $ hg log -R z:z
+  abort: no '://' in scheme url 'z:z'
+  [255]
+
+http scheme
+
   $ hg serve -n test -p $HGPORT -d --pid-file=hg.pid -A access.log -E errors.log
   $ cat hg.pid >> $DAEMON_PIDS
   $ hg incoming l://