This is an archive of the discontinued Mercurial Phabricator instance.

Differential D2244

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

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

Details

Reviewers
indygreg
Group Reviewers
hg-reviewers
Commits
rHG59affe7e01d4: histedit: binascii.unhexlify (aka node.bin) throws new exception type on py3
rHG88dd15b38bb0: histedit: binascii.unhexlify (aka node.bin) throws new exception type on py3
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
durin42 added a comment.Feb 14 2018, 12:02 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.

indygreg accepted this revision.Feb 14 2018, 12:52 AM
In 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.

This revision is now accepted and ready to land.Feb 14 2018, 12:52 AM
Closed by commit rHG88dd15b38bb0: histedit: binascii.unhexlify (aka node.bin) throws new exception type on py3 (authored by durin42). · Explain WhyFeb 14 2018, 12:54 AM
This revision was automatically updated to reflect the committed changes.
martinvonz added a subscriber: martinvonz.Feb 14 2018, 1:03 AM
In 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 D2244#37233, @indygreg wrote:
In 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 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

indygreg added a comment.Feb 14 2018, 1:05 AM

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

martinvonz added a comment.Feb 14 2018, 1:10 AM
In 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)

Revision Contents
Changeset List

PathPackages
Mhgext/histedit.py (3 lines)

Diff 5688

hgext/histedit.py

[histedit] [histedit]
singletransaction = True singletransaction = True
""" """
from __future__ import absolute_import from __future__ import absolute_import
import binascii
import errno import errno
import os import os
from mercurial.i18n import _ from mercurial.i18n import _
from mercurial import ( from mercurial import (
bundle2, bundle2,
cmdutil, cmdutil,
context, context,
@classmethod @classmethod
def fromrule(cls, state, rule): def fromrule(cls, state, rule):
"""Parses the given rule, returning an instance of the histeditaction. """Parses the given rule, returning an instance of the histeditaction.
""" """
rulehash = rule.strip().split(' ', 1)[0] rulehash = rule.strip().split(' ', 1)[0]
try: try:
rev = node.bin(rulehash) rev = node.bin(rulehash)
except TypeError: except (TypeError, binascii.Error):
raise error.ParseError("invalid changeset %s" % rulehash) raise error.ParseError("invalid changeset %s" % rulehash)
return cls(state, rev) return cls(state, rev)
def verify(self, prev, expected, seen): def verify(self, prev, expected, seen):
""" Verifies semantic correctness of the rule""" """ Verifies semantic correctness of the rule"""
repo = self.repo repo = self.repo
ha = node.hex(self.node) ha = node.hex(self.node)
try: try: