[PATCH V2 STABLE] serve: move hg-ssh readonly logic into hg serve

[Durham Goode]

On 5/10/17 7:29 AM, Yuya Nishihara wrote:
> On Tue, 9 May 2017 15:48:50 -0700, Durham Goode wrote:
>> On 4/28/17 6:27 AM, Yuya Nishihara wrote:
>>> On Thu, 27 Apr 2017 10:13:21 -0400, Augie Fackler wrote:
>>>> On Wed, Apr 26, 2017 at 06:24:52PM -0700, Durham Goode wrote:
>>>>> # HG changeset patch
>>>>> # User Durham Goode <durham at fb.com>
>>>>> # Date 1493255976 25200
>>>>> #      Wed Apr 26 18:19:36 2017 -0700
>>>>> # Branch stable
>>>>> # Node ID b1964bbc387fb53b4152f19d6e929309e3f21ac6
>>>>> # Parent  6e0368b6e0bb2aa5210daec091c0200583553a78
>>>>> serve: move hg-ssh readonly logic into hg serve
>>>>
>>>> This looks good, but I'm very hesitant to add a new feature like this
>>>> 4 days before a release. Does anyone feel strongly that this should be
>>>> in 4.2?
>>>
>>> Agreed. The feature seems good, but no need to hurry to pick it up.
>>>
>>>> +        if realcmd == 'serve' and '--read-only' in req.args:
>>>> +            req.args.remove('--read-only')
>>>
>>> This increases the risk of wrong command parsing. We can't say --read-only
>>> is always a flag.
>>>
>>>> +            req.ui.setconfig('hooks', 'pretxnopen.readonlyrejectpush',
>>>> +                             rejectpush, 'dispatch')
>>>> +            req.ui.setconfig('hooks', 'prepushkey.readonlyrejectpush',
>>>> +                             rejectpush, 'dispatch')
>>>
>>> And --read-only won't work as expected in command server since there are write
>>> operations other than push.
>>
>> Are there write operations that wouldn't trigger the pretxnopen hook?
>
> Perhaps, "hg rollback" ?
>
>> Alternatively, I could rename this flag to be
>> --add-hgssh-push-blocking-hooks, remove it from the hg server command
>> definition entirely (just watch for it at the dispatch level and strip
>> it like I currently do). That way we can maintain the current pattern
>> while letting it be a special path for hg-ssh only.
>>
>> And if someone passes --add-hgssh-push-blocking-hooks to hg serve in a
>> position that isn't a flag, well it'll just break for them.
>
> Another idea is adding --unsafe-stdio option so the frontend script may do
> anything on the 'hg serve --stdio' process.

I'm not sure I understand this proposal.  If we have --unsafe-stdio, 
doesn't that imply --stdio is safe and can block write operations? It 
sounds like we don't have a way to block things like rollback, so we 
couldn't guarantee --stdio is safe.