[PATCH 2 of 7 stable] bundle1: fix bundle1-denied reporting for push over ssh

Gregory Szorc gregory.szorc at gmail.com
Sat Feb 11 13:28:45 EST 2017



> On Feb 10, 2017, at 15:54, Martin von Zweigbergk via Mercurial-devel <mercurial-devel at mercurial-scm.org> wrote:
> 
> On Fri, Feb 10, 2017 at 9:53 AM, Pierre-Yves David
> <pierre-yves.david at ens-lyon.org> wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david at ens-lyon.org>
>> # Date 1486745819 -3600
>> #      Fri Feb 10 17:56:59 2017 +0100
>> # Branch stable
>> # Node ID f319afe9c93cb0cfeeff58f66b1792eb55130ba4
>> # Parent  a7ded180ddb35dfc0e642e960a59ed475fd9be75
>> # EXP-Topic getbundleerror
>> bundle1: fix bundle1-denied reporting for push over ssh
>> 
>> Changeset b288fb2724bf introduced a config option to have the server deny push
>> using bundle1. The original protocol has not really be design to allow such kind
>> of error reporting so some hack was used. It turned the hack only works on HTTP
>> and that ssh wire peer hangs forever when the same hack is used. After further
>> digging, there is no way to report the error in a unified way. Using 'ooberror'
>> freeze ssh and raising 'Abort' makes HTTP return a HTTP500 without further
>> details. So with sadness we implement a version that dispatch according to the
>> protocol used.
> 
> Could we instead raise a custom exception with the appropriate data (a
> main message and a hint, it seems) that we then could catch in the
> http and ssh handlers and handle appropriately for each?
> 
> Even if that's what we want long-term (I don't know if there's a
> reason not want it long-term), I'm fine with taking your patches for
> now since they're probably less intrusive and therefore more
> appropriate for stable.

Ugh. The SSH wire protocol needs to be replaced.

I agree with Martin about wanting a better error handling mechanism. I also agree that it would be too invasive for stable.

The patches as is correct an obvious problem and LGTM. Refinements can occur on default.

FWIW Mozilla sees a lot of these "stream ended unexpectedly" errors. The errors are next to useless and I really wish we printed something more informational. I still suspect we may be swallowing an exception from a low-level network failure somewhere. What I'm trying to say is there is a lot of room for improvement to error handling in the protocol code. But that's for another series.

> 
>> 
>> We also add a test for pushing over ssh to make sure we won't regress in the
>> future. That test show that the hint is missing, this is another bug fixed in
>> the next changeset.
>> 
>> diff -r a7ded180ddb3 -r f319afe9c93c mercurial/wireproto.py
>> --- a/mercurial/wireproto.py    Fri Feb 10 17:56:47 2017 +0100
>> +++ b/mercurial/wireproto.py    Fri Feb 10 17:56:59 2017 +0100
>> @@ -33,9 +33,10 @@
>> urlerr = util.urlerr
>> urlreq = util.urlreq
>> 
>> -bundle2required = _(
>> -    'incompatible Mercurial client; bundle2 required\n'
>> -    '(see https://www.mercurial-scm.org/wiki/IncompatibleClient)\n')
>> +bundle2requiredmain = _('incompatible Mercurial client; bundle2 required')
>> +bundle2requiredhint = _('see https://www.mercurial-scm.org/wiki/'
>> +                        'IncompatibleClient')
>> +bundle2required = '%s\n(%s)\n' % (bundle2requiredmain, bundle2requiredhint)
>> 
>> class abstractserverproto(object):
>>     """abstract class that summarizes the protocol API
>> @@ -948,7 +949,14 @@
>>             gen = exchange.readbundle(repo.ui, fp, None)
>>             if (isinstance(gen, changegroupmod.cg1unpacker)
>>                 and not bundle1allowed(repo, 'push')):
>> -                return ooberror(bundle2required)
>> +                if proto.name == 'http':
>> +                    # need to special case http because stderr do not get to
>> +                    # the http client on failed push so we need to abuse some
>> +                    # other error type to make sure the message get to the
>> +                    # user.
>> +                    return ooberror(bundle2required)
>> +                raise error.Abort(bundle2requiredmain,
>> +                                  hint=bundle2requiredhint)
>> 
>>             r = exchange.unbundle(repo, gen, their_heads, 'serve',
>>                                   proto._client())
>> diff -r a7ded180ddb3 -r f319afe9c93c tests/test-bundle2-exchange.t
>> --- a/tests/test-bundle2-exchange.t     Fri Feb 10 17:56:47 2017 +0100
>> +++ b/tests/test-bundle2-exchange.t     Fri Feb 10 17:56:59 2017 +0100
>> @@ -1107,6 +1107,14 @@
>>   (see https://www.mercurial-scm.org/wiki/IncompatibleClient)
>>   [255]
>> 
>> +(also check with ssh)
>> +
>> +  $ hg --config devel.legacy.exchange=bundle1 push ssh://user@dummy/bundle2onlyserver
>> +  pushing to ssh://user@dummy/bundle2onlyserver
>> +  searching for changes
>> +  remote: abort: incompatible Mercurial client; bundle2 required
>> +  [1]
>> +
>>   $ hg push
>>   pushing to http://localhost:$HGPORT/
>>   searching for changes
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel at mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel


More information about the Mercurial-devel mailing list