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
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
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
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
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
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
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)