[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