Patchwork D2244: histedit: binascii.unhexlify (aka node.bin) throws new exception type on py3

login
register
mail settings
Submitter phabricator
Date Feb. 14, 2018, 12:13 a.m.
Message ID <differential-rev-PHID-DREV-ontozzqj5wvizthzmry5-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/27862/
State Superseded
Headers show

Comments

phabricator - Feb. 14, 2018, 12:13 a.m.
durin42 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Lucky for us, the exception type exists on 2.7, so we can include it
  in the except block without any extra work.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2244

AFFECTED FILES
  hgext/histedit.py

CHANGE DETAILS




To: durin42, #hg-reviewers
Cc: mercurial-devel
phabricator - Feb. 14, 2018, 4:59 a.m.
indygreg requested changes to this revision.
indygreg added a comment.
This revision now requires changes to proceed.


  I'm not super thrilled about this. Presumably we'll have this class of failure throughout the code base. What do you think about having `node.bin()` catch `binascii.Error` and re-raise as `TypeError`?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2244

To: durin42, #hg-reviewers, indygreg
Cc: indygreg, mercurial-devel
phabricator - Feb. 14, 2018, 5:02 a.m.
durin42 added a comment.


  I could go either way. A casual grep for "except TypeError" suggests we're looking at 4 or 5 locations *total*, so my bias is to do it at the callsites and avoid the annoyingly-large overhead of a wrapper function for something that's so fundamental.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2244

To: durin42, #hg-reviewers, indygreg
Cc: indygreg, mercurial-devel
phabricator - Feb. 14, 2018, 5:52 a.m.
indygreg accepted this revision.
indygreg added a comment.
This revision is now accepted and ready to land.


  In https://phab.mercurial-scm.org/D2244#37169, @durin42 wrote:
  
  > I could go either way. A casual grep for "except TypeError" suggests we're looking at 4 or 5 locations *total*, so my bias is to do it at the callsites and avoid the annoyingly-large overhead of a wrapper function for something that's so fundamental.
  
  
  If it's only 4 or 5 call sites, then I'm not worried. I assumed it would be more widespread than that.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2244

To: durin42, #hg-reviewers, indygreg
Cc: indygreg, mercurial-devel
phabricator - Feb. 14, 2018, 6:03 a.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D2244#37169, @durin42 wrote:
  
  > I could go either way. A casual grep for "except TypeError" suggests we're looking at 4 or 5 locations *total*, so my bias is to do it at the callsites and avoid the annoyingly-large overhead of a wrapper function for something that's so fundamental.
  
  
  
  
  In https://phab.mercurial-scm.org/D2244#37233, @indygreg wrote:
  
  > In https://phab.mercurial-scm.org/D2244#37169, @durin42 wrote:
  >
  > > I could go either way. A casual grep for "except TypeError" suggests we're looking at 4 or 5 locations *total*, so my bias is to do it at the callsites and avoid the annoyingly-large overhead of a wrapper function for something that's so fundamental.
  >
  >
  > If it's only 4 or 5 call sites, then I'm not worried. I assumed it would be more widespread than that.
  
  
  I found a few more (at https://phab.mercurial-scm.org/rHGeefabd9ed3e1c8ed8ea023a0a76a76335185e2b3):
  
  lfcommands.py:332
  histedit.py:428
  shelve.py:194
  bookmarks.py:80
  context.py:528
  debugcommands.py:1541
  revlog.py:1360
  revlog.py:1407
  revset.py:1326
  tags.py:295

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2244

To: durin42, #hg-reviewers, indygreg
Cc: martinvonz, indygreg, mercurial-devel
phabricator - Feb. 14, 2018, 6:05 a.m.
indygreg added a comment.


  Now I'm second guessing accepting/committing this...

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2244

To: durin42, #hg-reviewers, indygreg
Cc: martinvonz, indygreg, mercurial-devel
phabricator - Feb. 14, 2018, 6:10 a.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D2244#37261, @indygreg wrote:
  
  > Now I'm second guessing accepting/committing this...
  
  
  Some thoughts:
  
  - This is probably not very well tested code (because it's an error path)
  - As long as we grep for TypeError and add binascii.Error in all those places, we're probably fine
  - Extensions need to be fixed too
  
  I think I'd prefer to define our own node.bin() with the old contract (IOW to wrap it)

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2244

To: durin42, #hg-reviewers, indygreg
Cc: martinvonz, indygreg, mercurial-devel

Patch

diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -183,6 +183,7 @@ 
 
 from __future__ import absolute_import
 
+import binascii
 import errno
 import os
 
@@ -425,7 +426,7 @@ 
         rulehash = rule.strip().split(' ', 1)[0]
         try:
             rev = node.bin(rulehash)
-        except TypeError:
+        except (TypeError, binascii.Error):
             raise error.ParseError("invalid changeset %s" % rulehash)
         return cls(state, rev)