This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9462

chgserver: catch RepoError while loading configuration
ClosedPublic

Authored by pulkit on Nov 30 2020, 4:42 AM.

Details

Reviewers
marmoute
mharbison72
Group Reviewers
hg-reviewers
Commits
rHGaab70f05d6ec: chgserver: catch RepoError while loading configuration
Summary

Recent share safe work introduced functionality to read share source config file
on dispatch. This can result in RepoError while reading config file as the
shared source might not be present.

test-share.t#safe was failing with chg earlier because of this.

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

pulkit created this revision.Nov 30 2020, 4:42 AM
marmoute accepted this revision.Nov 30 2020, 4:48 AM
marmoute added a subscriber: marmoute.

Looks okay. Alternatively, you could have a try: except Exception: with a re-raise. But the current code is "good enough" and we need these tests green.

mharbison72 added a subscriber: mharbison72.Dec 1 2020, 2:03 PM
mharbison72 added inline comments.
mercurial/chgserver.py
513

Does the error need stringutil.forcebytestr()? I see it in some places but not others, and am not sure the difference. Maybe hg errors don't need it but python exceptions do?

515

Needs bytes? (I can fix in flight, I just wanted to confirm).

pulkit updated this revision to Diff 23927.Dec 2 2020, 2:43 AM
pulkit marked 2 inline comments as done.Dec 2 2020, 2:44 AM
pulkit added inline comments.
mercurial/chgserver.py
513

My understanding is hg expections don't need it in general but RepoError can contain path in local encoding (IIUC). Hence added stringutil.forvebytestr().

marmoute accepted this revision.Dec 2 2020, 3:05 AM
mharbison72 accepted this revision.Dec 2 2020, 9:55 AM
This revision is now accepted and ready to land.Dec 2 2020, 9:55 AM
pulkit marked an inline comment as done.Dec 2 2020, 9:58 AM
pulkit added a commit: rHGaab70f05d6ec: chgserver: catch RepoError while loading configuration.
Closed by commit rHGaab70f05d6ec: chgserver: catch RepoError while loading configuration (authored by pulkit). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/chgserver.py (11 lines)

Diff 23938

mercurial/chgserver.py

This may happen if we cannot parse the config. This may happen if we cannot parse the config.
- "reconnect", the client should close the connection and - "reconnect", the client should close the connection and
reconnect. reconnect.
If neither "reconnect" nor "redirect" is included in the instruction If neither "reconnect" nor "redirect" is included in the instruction
list, the client can continue with this server after completing all list, the client can continue with this server after completing all
the instructions. the instructions.
""" """
args = self._readlist() args = self._readlist()
errorraised = False
try: try:
self.ui, lui = _loadnewui(self.ui, args, self.cdebug) self.ui, lui = _loadnewui(self.ui, args, self.cdebug)
except error.RepoError as inst:
# RepoError can be raised while trying to read shared source
# configuration
self.ui.error(_(b"abort: %s\n") % stringutil.forcebytestr(inst))
mharbison72Unsubmitted
Done

Does the error need stringutil.forcebytestr()? I see it in some places but not others, and am not sure the difference. Maybe hg errors don't need it but python exceptions do?

mharbison72: Does the error need `stringutil.forcebytestr()`? I see it in some places but not others, and…
pulkitAuthorUnsubmitted
Done

My understanding is hg expections don't need it in general but RepoError can contain path in local encoding (IIUC). Hence added stringutil.forvebytestr().

pulkit: My understanding is hg expections don't need it in general but `RepoError` can contain path in…
if inst.hint:
self.ui.error(_(b"(%s)\n") % inst.hint)
mharbison72Unsubmitted
Done

Needs bytes? (I can fix in flight, I just wanted to confirm).

mharbison72: Needs bytes? (I can fix in flight, I just wanted to confirm).
errorraised = True
except error.Abort as inst: except error.Abort as inst:
self.ui.error(inst.format()) self.ui.error(inst.format())
errorraised = True
if errorraised:
self.ui.flush() self.ui.flush()
self.cresult.write(b'exit 255') self.cresult.write(b'exit 255')
return return
newhash = hashstate.fromui(lui, self.hashstate.mtimepaths) newhash = hashstate.fromui(lui, self.hashstate.mtimepaths)
insts = [] insts = []
if newhash.mtimehash != self.hashstate.mtimehash: if newhash.mtimehash != self.hashstate.mtimehash:
addr = _hashaddress(self.baseaddress, self.hashstate.confighash) addr = _hashaddress(self.baseaddress, self.hashstate.confighash)
insts.append(b'unlink %s' % addr) insts.append(b'unlink %s' % addr)