This is an archive of the discontinued Mercurial Phabricator instance.

Differential D3980

contrib/phabricator: Convert description into local
ClosedPublic

Authored by ced on Jul 25 2018, 4:35 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG0dce1297dd01: phabricator: convert description into local
Summary

The description from conduit is a unicode.

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

ced created this revision.Jul 25 2018, 4:35 AM
yuja added a subscriber: yuja.Jul 25 2018, 8:43 AM
newdesc = getdescfromdrev(drev)

+ newdesc = encoding.unitolocal(newdesc)

Perhaps encoding stuff should be handled in getdescfromdrev() before
concatenating received data with bytes.

ced added a comment.Jul 25 2018, 9:08 AM

getdescfromdrev is also used in readpatch which call encoding.unitolocal after.
So it looks like it is designed to be converted after the call.

yuja added a comment.Jul 25 2018, 9:18 AM
getdescfromdrev is also used in readpatch which call encoding.unitolocal after.
So it looks like it is designed to be converted after the call.

Can you fix them all? A general guideline for Mercurial codebase is converting
unicodes to byte strings as early as possible.

ced added a comment.Jul 25 2018, 10:18 AM

I do not know enough the internal of the extensions to know if all other strings in readpatch need to be converted or not.

martinvonz added a subscriber: martinvonz.Jul 25 2018, 3:07 PM
In D3980#61930, @yuja wrote:
getdescfromdrev is also used in readpatch which call encoding.unitolocal after.
So it looks like it is designed to be converted after the call.

Can you fix them all? A general guideline for Mercurial codebase is converting
unicodes to byte strings as early as possible.

Maybe it should even be done where callconduit() returns (i.e. line 216)?

quark added a subscriber: quark.Jul 25 2018, 9:20 PM

Yeah, if only there is a json.loadb function. That could replace json.loads at line 211. I guess it could be done by using a function that recursively convert strings.

yuja added a comment.Jul 26 2018, 8:06 AM

That could replace json.loads at line 211. I guess it could be done by using a function that recursively convert strings.

Could be.

def _maybeunitolocal(u):
    if isinstance(u, pycompat.unicode):
        return encoding.unitolocal(u)
    return u
pycompat.rapply(_maybeunitolocal, json.loads(...))
yuja added a comment.Aug 7 2018, 10:15 AM

I've pushed this with no modification as it should fix a problem, thanks.
Further cleanups are welcome.

Closed by commit rHG0dce1297dd01: phabricator: convert description into local (authored by ced). · Explain WhyAug 7 2018, 10:16 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mcontrib/phabricator.py (1 line)

Diff 10056

contrib/phabricator.py

with repo.wlock(), repo.lock(), repo.transaction(b'phabsend'): with repo.wlock(), repo.lock(), repo.transaction(b'phabsend'):
wnode = unfi[b'.'].node() wnode = unfi[b'.'].node()
mapping = {} # {oldnode: [newnode]} mapping = {} # {oldnode: [newnode]}
for i, rev in enumerate(revs): for i, rev in enumerate(revs):
old = unfi[rev] old = unfi[rev]
drevid = drevids[i] drevid = drevids[i]
drev = [d for d in drevs if int(d[r'id']) == drevid][0] drev = [d for d in drevs if int(d[r'id']) == drevid][0]
newdesc = getdescfromdrev(drev) newdesc = getdescfromdrev(drev)
newdesc = encoding.unitolocal(newdesc)
# Make sure commit message contain "Differential Revision" # Make sure commit message contain "Differential Revision"
if old.description() != newdesc: if old.description() != newdesc:
parents = [ parents = [
mapping.get(old.p1().node(), (old.p1(),))[0], mapping.get(old.p1().node(), (old.p1(),))[0],
mapping.get(old.p2().node(), (old.p2(),))[0], mapping.get(old.p2().node(), (old.p2(),))[0],
] ]
new = context.metadataonlyctx( new = context.metadataonlyctx(
repo, old, parents=parents, text=newdesc, repo, old, parents=parents, text=newdesc,