| hg-reviewers |
Raising ValueError results in an uncaught exception and a traceback
being printed. In the context of servers, it can result in an HTTP
500 and an exception being logged in the error log.
I don't think this is proper behavior.
The bundle2 code paths have a mechanism for translating an
error.Abort into an error message reported to the clients. I
think we should use that instead.
This commit replaces some ValueError with Abort so that
servers can error more gracefully.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
| Path | Packages | |||
|---|---|---|---|---|
| M | mercurial/exchange.py (8 lines) | |||
| M | tests/test-lfs-serve.t (18 lines) |
| b2caps = bundle2.bundle2caps(pushop.remote) | b2caps = bundle2.bundle2caps(pushop.remote) | ||||
| version = '01' | version = '01' | ||||
| cgversions = b2caps.get('changegroup') | cgversions = b2caps.get('changegroup') | ||||
| if cgversions: # 3.1 and 3.2 ship with an empty value | if cgversions: # 3.1 and 3.2 ship with an empty value | ||||
| cgversions = [v for v in cgversions | cgversions = [v for v in cgversions | ||||
| if v in changegroup.supportedoutgoingversions( | if v in changegroup.supportedoutgoingversions( | ||||
| pushop.repo)] | pushop.repo)] | ||||
| if not cgversions: | if not cgversions: | ||||
| raise ValueError(_('no common changegroup version')) | raise error.Abort(_('no common changegroup version')) | ||||
| version = max(cgversions) | version = max(cgversions) | ||||
| cgstream = changegroup.makestream(pushop.repo, pushop.outgoing, version, | cgstream = changegroup.makestream(pushop.repo, pushop.outgoing, version, | ||||
| 'push') | 'push') | ||||
| cgpart = bundler.newpart('changegroup', data=cgstream) | cgpart = bundler.newpart('changegroup', data=cgstream) | ||||
| if cgversions: | if cgversions: | ||||
| cgpart.addparam('version', version) | cgpart.addparam('version', version) | ||||
| if 'treemanifest' in pushop.repo.requirements: | if 'treemanifest' in pushop.repo.requirements: | ||||
| cgpart.addparam('treemanifest', '1') | cgpart.addparam('treemanifest', '1') | ||||
| return | return | ||||
| version = '01' | version = '01' | ||||
| cgversions = b2caps.get('changegroup') | cgversions = b2caps.get('changegroup') | ||||
| if cgversions: # 3.1 and 3.2 ship with an empty value | if cgversions: # 3.1 and 3.2 ship with an empty value | ||||
| cgversions = [v for v in cgversions | cgversions = [v for v in cgversions | ||||
| if v in changegroup.supportedoutgoingversions(repo)] | if v in changegroup.supportedoutgoingversions(repo)] | ||||
| if not cgversions: | if not cgversions: | ||||
| raise ValueError(_('no common changegroup version')) | raise error.Abort(_('no common changegroup version')) | ||||
| version = max(cgversions) | version = max(cgversions) | ||||
| outgoing = _computeoutgoing(repo, heads, common) | outgoing = _computeoutgoing(repo, heads, common) | ||||
| if not outgoing.missing: | if not outgoing.missing: | ||||
| return | return | ||||
| if kwargs.get(r'narrow', False): | if kwargs.get(r'narrow', False): | ||||
| include = sorted(filter(bool, kwargs.get(r'includepats', []))) | include = sorted(filter(bool, kwargs.get(r'includepats', []))) | ||||
| @getbundle2partsgenerator('bookmarks') | @getbundle2partsgenerator('bookmarks') | ||||
| def _getbundlebookmarkpart(bundler, repo, source, bundlecaps=None, | def _getbundlebookmarkpart(bundler, repo, source, bundlecaps=None, | ||||
| b2caps=None, **kwargs): | b2caps=None, **kwargs): | ||||
| """add a bookmark part to the requested bundle""" | """add a bookmark part to the requested bundle""" | ||||
| if not kwargs.get(r'bookmarks', False): | if not kwargs.get(r'bookmarks', False): | ||||
| return | return | ||||
| if 'bookmarks' not in b2caps: | if 'bookmarks' not in b2caps: | ||||
| raise ValueError(_('no common bookmarks exchange method')) | raise error.Abort(_('no common bookmarks exchange method')) | ||||
| books = bookmod.listbinbookmarks(repo) | books = bookmod.listbinbookmarks(repo) | ||||
| data = bookmod.binaryencode(books) | data = bookmod.binaryencode(books) | ||||
| if data: | if data: | ||||
| bundler.newpart('bookmarks', data=data) | bundler.newpart('bookmarks', data=data) | ||||
| @getbundle2partsgenerator('listkeys') | @getbundle2partsgenerator('listkeys') | ||||
| def _getbundlelistkeysparts(bundler, repo, source, bundlecaps=None, | def _getbundlelistkeysparts(bundler, repo, source, bundlecaps=None, | ||||
| b2caps=None, **kwargs): | b2caps=None, **kwargs): | ||||
| bundle2.buildobsmarkerspart(bundler, markers) | bundle2.buildobsmarkerspart(bundler, markers) | ||||
| @getbundle2partsgenerator('phases') | @getbundle2partsgenerator('phases') | ||||
| def _getbundlephasespart(bundler, repo, source, bundlecaps=None, | def _getbundlephasespart(bundler, repo, source, bundlecaps=None, | ||||
| b2caps=None, heads=None, **kwargs): | b2caps=None, heads=None, **kwargs): | ||||
| """add phase heads part to the requested bundle""" | """add phase heads part to the requested bundle""" | ||||
| if kwargs.get(r'phases', False): | if kwargs.get(r'phases', False): | ||||
| if not 'heads' in b2caps.get('phases'): | if not 'heads' in b2caps.get('phases'): | ||||
| raise ValueError(_('no common phases exchange method')) | raise error.Abort(_('no common phases exchange method')) | ||||
| if heads is None: | if heads is None: | ||||
| heads = repo.heads() | heads = repo.heads() | ||||
| headsbyphase = collections.defaultdict(set) | headsbyphase = collections.defaultdict(set) | ||||
| if repo.publishing(): | if repo.publishing(): | ||||
| headsbyphase[phases.public] = heads | headsbyphase[phases.public] = heads | ||||
| else: | else: | ||||
| # find the appropriate heads to move | # find the appropriate heads to move | ||||
| > opts[b'manifest'] = False | > opts[b'manifest'] = False | ||||
| > opts[b'dir'] = False | > opts[b'dir'] = False | ||||
| > rl = cmdutil.openrevlog(repo, b'debugprocessors', file_, opts) | > rl = cmdutil.openrevlog(repo, b'debugprocessors', file_, opts) | ||||
| > for flag, proc in rl._flagprocessors.items(): | > for flag, proc in rl._flagprocessors.items(): | ||||
| > ui.status(b"registered processor '%#x'\n" % (flag)) | > ui.status(b"registered processor '%#x'\n" % (flag)) | ||||
| > EOF | > EOF | ||||
| Skip the experimental.changegroup3=True config. Failure to agree on this comes | Skip the experimental.changegroup3=True config. Failure to agree on this comes | ||||
| first, and causes a "ValueError: no common changegroup version" or "abort: | first, and causes an "abort: no common changegroup version" if the extension is | ||||
| HTTP Error 500: Internal Server Error", if the extension is only loaded on one | only loaded on one side. If that *is* enabled, the subsequent failure is "abort: | ||||
| side. If that *is* enabled, the subsequent failure is "abort: missing processor | missing processor for flag '0x2000'!" if the extension is only loaded on one side | ||||
| for flag '0x2000'!" if the extension is only loaded on one side (possibly also | (possibly also masked by the Internal Server Error message). | ||||
| masked by the Internal Server Error message). | |||||
| $ cat >> $HGRCPATH <<EOF | $ cat >> $HGRCPATH <<EOF | ||||
| > [extensions] | > [extensions] | ||||
| > debugprocessors = $TESTTMP/debugprocessors.py | > debugprocessors = $TESTTMP/debugprocessors.py | ||||
| > [experimental] | > [experimental] | ||||
| > lfs.disableusercache = True | > lfs.disableusercache = True | ||||
| > [lfs] | > [lfs] | ||||
| > threshold=10 | > threshold=10 | ||||
| > [web] | > [web] | ||||
| [2] | [2] | ||||
| TODO: fail more gracefully. | TODO: fail more gracefully. | ||||
| $ hg init $TESTTMP/client4_pull | $ hg init $TESTTMP/client4_pull | ||||
| $ hg -R $TESTTMP/client4_pull pull http://localhost:$HGPORT | $ hg -R $TESTTMP/client4_pull pull http://localhost:$HGPORT | ||||
| pulling from http://localhost:$HGPORT/ | pulling from http://localhost:$HGPORT/ | ||||
| requesting all changes | requesting all changes | ||||
| abort: HTTP Error 500: Internal Server Error | remote: abort: no common changegroup version | ||||
| abort: pull failed on remote | |||||
| [255] | [255] | ||||
| $ grep 'lfs' $TESTTMP/client4_pull/.hg/requires $SERVER_REQUIRES | $ grep 'lfs' $TESTTMP/client4_pull/.hg/requires $SERVER_REQUIRES | ||||
| $TESTTMP/server/.hg/requires:lfs | $TESTTMP/server/.hg/requires:lfs | ||||
| $ hg identify http://localhost:$HGPORT | $ hg identify http://localhost:$HGPORT | ||||
| 03b080fa9d93 | 03b080fa9d93 | ||||
| -------------------------------------------------------------------------------- | -------------------------------------------------------------------------------- | ||||
| +++ b/lfspair2.bin Thu Jan 01 00:00:00 1970 +0000 | +++ b/lfspair2.bin Thu Jan 01 00:00:00 1970 +0000 | ||||
| @@ -0,0 +1,1 @@ | @@ -0,0 +1,1 @@ | ||||
| +this is an lfs file too | +this is an lfs file too | ||||
| #endif | #endif | ||||
| $ "$PYTHON" $TESTDIR/killdaemons.py $DAEMON_PIDS | $ "$PYTHON" $TESTDIR/killdaemons.py $DAEMON_PIDS | ||||
| #if lfsremote-on | |||||
| $ cat $TESTTMP/errors.log | grep '^[A-Z]' | |||||
| Traceback (most recent call last): | |||||
| ValueError: no common changegroup version | |||||
| #else | |||||
| $ cat $TESTTMP/errors.log | $ cat $TESTTMP/errors.log | ||||
| #endif | |||||