This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8639

py3: fix broken man page generation, it was generating `(default: NUL*)`
ClosedPublic

Authored by spectral on Jun 17 2020, 8:15 PM.

Details

Reviewers
mjacob
pulkit
Group Reviewers
hg-reviewers
Commits
rHG1a4b9b602e54: py3: fix broken man page generation, it was generating `(default: NUL*)`
Summary

bytes(default) was producing things like (default: \x00) when handed
non-bytes values such as 1, 10, or True. The man page generation would
apparently ignore these bytes and produce man pages that had the string
(default: ).

Test Plan
  • Ran cd doc; python3 gendoc.py "hg.1.gendoc" and grepped for bad output
  • Ran make deb, extracted the deb, manually inspected hg.1 file.

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

spectral created this revision.Jun 17 2020, 8:15 PM
mjacob added a subscriber: mjacob.Jun 18 2020, 6:15 AM
mjacob added inline comments.
doc/gendoc.py
91

Isn’t bool(defaulttmpl) always true?

93

I think it’s safer to use 'ascii' here.

On Python 2, repr(default) is likely to return bytes and therefore will attempt to decode it with 'ascii' before encoding it with 'latin1'.

In general, I don’t think it is guaranteed that the output is expected to be latin1-encoded. If we ever have non-ASCII default values, we should fail early instead of producing mojibake.

An alternative to your code would be to use pycompat.bytestr. I don’t have a preference. An theoretical advantage is that is works correctly with bytearray, if we ever use it for default values. ;)

spectral updated this revision to Diff 21663.Jun 18 2020, 1:50 PM
spectral added inline comments.Jun 18 2020, 1:51 PM
doc/gendoc.py
91

I assumed it wasn't and that this was a way of checking whether the translation had the string. But now that I'm reading it again, I think it's a different way of writing desc += _(b" (default: %s)") % bytes(default) if default else b"", so I've made this non-conditional.

93

Yeah, now that I look closer, I was confused by the use of latin1 above. Switched this to stringutil.forcebytestr.

mjacob accepted this revision.Jun 18 2020, 9:43 PM
mjacob added inline comments.
doc/gendoc.py
93

Sounds reasonable.

pulkit accepted this revision.Jun 19 2020, 4:32 AM
This revision is now accepted and ready to land.Jun 19 2020, 4:32 AM

Revision Contents
Changeset List

PathPackages
Mdoc/gendoc.py (5 lines)

Diff 21666

doc/gendoc.py

minirst, minirst,
pycompat, pycompat,
ui as uimod, ui as uimod,
) )
from mercurial.i18n import ( from mercurial.i18n import (
gettext, gettext,
_, _,
) )
from mercurial.utils import stringutil
table = commands.table table = commands.table
globalopts = commands.globalopts globalopts = commands.globalopts
helptable = help.helptable helptable = help.helptable
loaddoc = help.loaddoc loaddoc = help.loaddoc
def get_desc(docstr): def get_desc(docstr):
allopts.append(b"--%s" % longopt) allopts.append(b"--%s" % longopt)
if isinstance(default, list): if isinstance(default, list):
allopts[-1] += b" <%s[+]>" % optlabel allopts[-1] += b" <%s[+]>" % optlabel
elif (default is not None) and not isinstance(default, bool): elif (default is not None) and not isinstance(default, bool):
allopts[-1] += b" <%s>" % optlabel allopts[-1] += b" <%s>" % optlabel
if b'\n' in desc: if b'\n' in desc:
# only remove line breaks and indentation # only remove line breaks and indentation
desc = b' '.join(l.lstrip() for l in desc.split(b'\n')) desc = b' '.join(l.lstrip() for l in desc.split(b'\n'))
desc += default and _(b" (default: %s)") % bytes(default) or b"" if default:
default = stringutil.forcebytestr(default)
desc += _(b" (default: %s)") % default
mjacobUnsubmitted
Not Done

Isn’t bool(defaulttmpl) always true?

mjacob: Isn’t `bool(defaulttmpl)` always true?
spectralAuthorUnsubmitted
Done

I assumed it wasn't and that this was a way of checking whether the translation had the string. But now that I'm reading it again, I think it's a different way of writing desc += _(b" (default: %s)") % bytes(default) if default else b"", so I've made this non-conditional.

spectral: I assumed it wasn't and that this was a way of checking whether the translation had the string.
yield (b", ".join(allopts), desc) yield (b", ".join(allopts), desc)
mjacobUnsubmitted
Not Done

I think it’s safer to use 'ascii' here.

On Python 2, repr(default) is likely to return bytes and therefore will attempt to decode it with 'ascii' before encoding it with 'latin1'.

In general, I don’t think it is guaranteed that the output is expected to be latin1-encoded. If we ever have non-ASCII default values, we should fail early instead of producing mojibake.

An alternative to your code would be to use pycompat.bytestr. I don’t have a preference. An theoretical advantage is that is works correctly with bytearray, if we ever use it for default values. ;)

mjacob: I think it’s safer to use 'ascii' here. On Python 2, `repr(default)` is likely to return bytes…
spectralAuthorUnsubmitted
Done

Yeah, now that I look closer, I was confused by the use of latin1 above. Switched this to stringutil.forcebytestr.

spectral: Yeah, now that I look closer, I was confused by the use of latin1 above. Switched this to…
mjacobUnsubmitted
Not Done

Sounds reasonable.

mjacob: Sounds reasonable.
def get_cmd(cmd, cmdtable): def get_cmd(cmd, cmdtable):
d = {} d = {}
attr = cmdtable[cmd] attr = cmdtable[cmd]
cmds = cmd.lstrip(b"^").split(b"|") cmds = cmd.lstrip(b"^").split(b"|")
d[b'cmd'] = cmds[0] d[b'cmd'] = cmds[0]
d[b'aliases'] = cmd.split(b"|")[1:] d[b'aliases'] = cmd.split(b"|")[1:]