Patchwork [04,of,12] hgweb: simplify wsgirequest header handling

login
register
mail settings
Submitter Mads Kiilerich
Date Jan. 11, 2013, 11:32 p.m.
Message ID <eb88facfdf98896c7597.1357947168@mk-desktop>
Download mbox | patch
Permalink /patch/564/
State Accepted
Commit 764a758780b66dcff7986de1b4dd6e241b969076
Headers show

Comments

Mads Kiilerich - Jan. 11, 2013, 11:32 p.m.
# HG changeset patch
# User Mads Kiilerich <madski@unity3d.com>
# Date 1357947109 -3600
# Node ID eb88facfdf98896c759734bb0bafd31371e76611
# Parent  3de07b9123f7babdbd11958e4ddca94a91e9a228
hgweb: simplify wsgirequest header handling

Remove leaky header abstraction and prepare for other encodings.
Augie Fackler - Jan. 14, 2013, 7:41 p.m.
On Sat, Jan 12, 2013 at 12:32:48AM +0100, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@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?)

> +            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@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Mads Kiilerich - Jan. 14, 2013, 11:39 p.m.
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@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@selenic.com
>> http://selenic.com/mailman/listinfo/mercurial-devel

Patch

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")
+            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)