[PATCH 04 of 12] hgweb: simplify wsgirequest header handling

Mads Kiilerich mads at kiilerich.com
Mon Jan 14 17:39:51 CST 2013


Augie Fackler wrote, On 01/14/2013 08:41 PM:
> On Sat, Jan 12, 2013 at 12:32:48AM +0100, Mads Kiilerich wrote:
>> # HG changeset patch
>> # User Mads Kiilerich <madski at unity3d.com>
>> # Date 1357947109 -3600
>> # Node ID eb88facfdf98896c759734bb0bafd31371e76611
>> # Parent  3de07b9123f7babdbd11958e4ddca94a91e9a228
>> hgweb: simplify wsgirequest header handling
>>
>> Remove leaky header abstraction and prepare for other encodings.
>>
>> diff --git a/mercurial/hgweb/request.py b/mercurial/hgweb/request.py
>> --- a/mercurial/hgweb/request.py
>> +++ b/mercurial/hgweb/request.py
>> @@ -72,17 +72,22 @@
>>
>>       def respond(self, status, type=None, filename=None, length=None):
>>           if self._start_response is not None:
>> -
>> -            self.httphdr(type, filename, length)
>> -            if not self.headers:
>> -                raise RuntimeError("request.write called before headers sent")
> I worry a little about removing this defense - it's definitely saved
> me when I've been doing stupid things (or is this no longer necessary
> for a reason I'm missing?)

Ok, I could keep the check ... even though I don't see much value in it.

Admittedly several things are going on here. The change moves to 
something that IMO makes sense - but I do not fully understand where we 
are coming from and can thus not split it up in steps that makes sense.

The error message was weird. A WSGI-ish application should send headers 
_when_ the write is called. I guess it should have been "error: 
responding without headers"?

But httphdr would usually add a Content-Type header - that is when 
'type' is specified. The only place where it isn't specified is in 
webcommands.archive ... but it could easily be given there and 'type' 
could become mandatory and the warning here wouldn't make sense. But ok, 
that haven't been done. Is the purpose of the check to verify that a 
Content-Type has been specified? Do you consider it equally good to do 
that refactoring?

/Mads

>
>> +            if type is not None:
>> +                self.headers.append(('Content-Type', type))
>> +            if filename:
>> +                filename = (filename.split('/')[-1]
>> +                            .replace('\\', '\\\\').replace('"', '\\"'))
>> +                self.headers.append(('Content-Disposition',
>> +                                     'inline; filename="%s"' % filename))
>> +            if length is not None:
>> +                self.headers.append(('Content-Length', str(length)))
>>
>>               for k, v in self.headers:
>>                   if not isinstance(v, str):
>> -                    raise TypeError('header value must be string: %r' % v)
>> +                    raise TypeError('header value must be string: %r' % (v,))
>>
>>               if isinstance(status, ErrorResponse):
>> -                self.header(status.headers)
>> +                self.headers.extend(status.headers)
>>                   if status.code == HTTP_NOT_MODIFIED:
>>                       # RFC 2616 Section 10.3.5: 304 Not Modified has cases where
>>                       # it MUST NOT include any headers other than these and no
>> @@ -122,22 +127,6 @@
>>       def close(self):
>>           return None
>>
>> -    def header(self, headers=[('Content-Type','text/html')]):
>> -        self.headers.extend(headers)
>> -
>> -    def httphdr(self, type=None, filename=None, length=None, headers={}):
>> -        headers = headers.items()
>> -        if type is not None:
>> -            headers.append(('Content-Type', type))
>> -        if filename:
>> -            filename = (filename.split('/')[-1]
>> -                        .replace('\\', '\\\\').replace('"', '\\"'))
>> -            headers.append(('Content-Disposition',
>> -                            'inline; filename="%s"' % filename))
>> -        if length is not None:
>> -            headers.append(('Content-Length', str(length)))
>> -        self.header(headers)
>> -
>>   def wsgiapplication(app_maker):
>>       '''For compatibility with old CGI scripts. A plain hgweb() or hgwebdir()
>>       can and should now be used as a WSGI application.'''
>> diff --git a/mercurial/hgweb/webcommands.py b/mercurial/hgweb/webcommands.py
>> --- a/mercurial/hgweb/webcommands.py
>> +++ b/mercurial/hgweb/webcommands.py
>> @@ -805,7 +805,7 @@
>>       ]
>>       if encoding:
>>           headers.append(('Content-Encoding', encoding))
>> -    req.header(headers)
>> +    req.headers.extend(headers)
>>       req.respond(HTTP_OK)
>>
>>       ctx = webutil.changectx(web.repo, req)
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel at selenic.com
>> http://selenic.com/mailman/listinfo/mercurial-devel



More information about the Mercurial-devel mailing list