| 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
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.
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
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://