exchange: raise error.Abort instead of ValueError

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.

Differential Revision: https://phab.mercurial-scm.org/D5972

mercurial/exchange.py

@@ -920,7 +920,7 b' def _pushb2ctx(pushop, bundler):'
                       if v in changegroup.supportedoutgoingversions(
                           pushop.repo)]
         if not cgversions:
-            raise ValueError(_('no common changegroup version'))
+            raise error.Abort(_('no common changegroup version'))
         version = max(cgversions)
     cgstream = changegroup.makestream(pushop.repo, pushop.outgoing, version,
                                       'push')
@@ -2184,7 +2184,7 b' def _getbundlechangegrouppart(bundler, r'
         cgversions = [v for v in cgversions
                       if v in changegroup.supportedoutgoingversions(repo)]
         if not cgversions:
-            raise ValueError(_('no common changegroup version'))
+            raise error.Abort(_('no common changegroup version'))
         version = max(cgversions)
 
     outgoing = _computeoutgoing(repo, heads, common)
@@ -2228,7 +2228,7 b' def _getbundlebookmarkpart(bundler, repo'
     if not kwargs.get(r'bookmarks', False):
         return
     if 'bookmarks' not in b2caps:
-        raise ValueError(_('no common bookmarks exchange method'))
+        raise error.Abort(_('no common bookmarks exchange method'))
     books  = bookmod.listbinbookmarks(repo)
     data = bookmod.binaryencode(books)
     if data:
@@ -2263,7 +2263,7 b' def _getbundlephasespart(bundler, repo, '
     """add phase heads part to the requested bundle"""
     if kwargs.get(r'phases', False):
         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:
             heads = repo.heads()
 

tests/test-lfs-serve.t

@@ -56,11 +56,10 b' make command server magic visible'
   > EOF
 
 Skip the experimental.changegroup3=True config.  Failure to agree on this comes
-first, and causes a "ValueError: no common changegroup version" or "abort:
-HTTP Error 500: Internal Server Error", if the extension is only loaded on one
-side.  If that *is* enabled, the subsequent failure is "abort: missing processor
-for flag '0x2000'!" if the extension is only loaded on one side (possibly also
-masked by the Internal Server Error message).
+first, and causes an "abort: no common changegroup version" if the extension is
+only loaded on one side. If that *is* enabled, the subsequent failure is "abort:
+missing processor for flag '0x2000'!" if the extension is only loaded on one side
+(possibly also masked by the Internal Server Error message).
   $ cat >> $HGRCPATH <<EOF
   > [extensions]
   > debugprocessors = $TESTTMP/debugprocessors.py
@@ -320,7 +319,8 b' TODO: fail more gracefully.'
   $ hg -R $TESTTMP/client4_pull pull http://localhost:$HGPORT
   pulling from http://localhost:$HGPORT/
   requesting all changes
-  abort: HTTP Error 500: Internal Server Error
+  remote: abort: no common changegroup version
+  abort: pull failed on remote
   [255]
   $ grep 'lfs' $TESTTMP/client4_pull/.hg/requires $SERVER_REQUIRES
   $TESTTMP/server/.hg/requires:lfs
@@ -664,10 +664,4 b' Only the files required by diff are pref'
 
   $ "$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
-#endif