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

[Martin von Zweigbergk]

On Sat, Feb 11, 2017 at 10:28 AM, Gregory Szorc <gregory.szorc at gmail.com> wrote:
>
>
>> 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.

Series queued for stable. Thanks to Pierre-Yves for patches and thanks
to Greg for review.

>
>>
>>>
>>> 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