contrib/phabricator: Convert description into local
ClosedPublic

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

Details

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

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.

This revision was automatically updated to reflect the committed changes.