This is an archive of the discontinued Mercurial Phabricator instance.

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

Authored by durin42 on Feb 13 2018, 7:13 PM.

Details

Summary

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

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

durin42 created this revision.Feb 13 2018, 7:13 PM
indygreg requested changes to this revision.Feb 13 2018, 11:59 PM
indygreg added a subscriber: indygreg.

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?

This revision now requires changes to proceed.Feb 13 2018, 11:59 PM

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.

indygreg accepted this revision.Feb 14 2018, 12:52 AM

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.

This revision is now accepted and ready to land.Feb 14 2018, 12:52 AM
This revision was automatically updated to reflect the committed changes.

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.

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

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

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

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)