Patchwork D1080: hgweb: fix logging to use native strings as appropriate

login
register
mail settings
Submitter phabricator
Date Oct. 14, 2017, 8:16 p.m.
Message ID <differential-rev-PHID-DREV-aingnqtfrbersxp3im2g-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/24911/
State Superseded
Headers show

Comments

phabricator - Oct. 14, 2017, 8:16 p.m.
durin42 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Kind of a tangled mess, but now logging works in both Python 2 and 3.
  
  no-check-commit because of the interface required by Python's HTTP
  ==================================================================
  
  server code.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1080

AFFECTED FILES
  mercurial/hgweb/server.py

CHANGE DETAILS




To: durin42, #hg-reviewers
Cc: mercurial-devel
phabricator - Oct. 14, 2017, 9 p.m.
indygreg added a comment.


  I'm not crazy about not logging bytes everywhere. Why is the file-like object taking bytes in Python 2 but Unicode in Python 3? It feels like you stumbled onto an existing bug where the logging mechanism isn't encoding aware?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1080

To: durin42, #hg-reviewers
Cc: indygreg, mercurial-devel
phabricator - Oct. 15, 2017, 1:05 a.m.
durin42 added a comment.


  In https://phab.mercurial-scm.org/D1080#18100, @indygreg wrote:
  
  > I'm not crazy about not logging bytes everywhere. Why is the file-like object taking bytes in Python 2 but Unicode in Python 3? It feels like you stumbled onto an existing bug where the logging mechanism isn't encoding aware?
  
  
  The file-like object to which we're logging is always in bytes mode - we traffic in unicodes from the callsite of log_error and log_request because there's lots of internal wsgi bits that are unicodes, and it's easier to make the log template a native string and bounce it down to bytes on Python 3 than to try and wrap the entire WSGI universe in a bytes blanket. Does that clarify things?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1080

To: durin42, #hg-reviewers
Cc: indygreg, mercurial-devel
phabricator - Oct. 15, 2017, 3:34 a.m.
yuja accepted this revision.
yuja added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> server.py:339
>          raise error.Abort(_("cannot start server at '%s:%d': %s")
> -                         % (address, port, inst.args[1]))
> +                         % (address, port, pycompat.sysbytes(inst.args[1])))

Replaced this with encoding.strtolocal() since socket.error may
contain platform-native string.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1080

To: durin42, #hg-reviewers, yuja
Cc: yuja, indygreg, mercurial-devel

Patch

diff --git a/mercurial/hgweb/server.py b/mercurial/hgweb/server.py
--- a/mercurial/hgweb/server.py
+++ b/mercurial/hgweb/server.py
@@ -67,25 +67,26 @@ 
         httpservermod.basehttprequesthandler.__init__(self, *args, **kargs)
 
     def _log_any(self, fp, format, *args):
-        fp.write("%s - - [%s] %s\n" % (self.client_address[0],
-                                       self.log_date_time_string(),
-                                       format % args))
+        fp.write(pycompat.sysbytes(
+            r"%s - - [%s] %s" % (self.client_address[0],
+                                   self.log_date_time_string(),
+                                   format % args)) + '\n')
         fp.flush()
 
     def log_error(self, format, *args):
         self._log_any(self.server.errorlog, format, *args)
 
     def log_message(self, format, *args):
         self._log_any(self.server.accesslog, format, *args)
 
-    def log_request(self, code='-', size='-'):
+    def log_request(self, code=r'-', size=r'-'):
         xheaders = []
         if util.safehasattr(self, 'headers'):
             xheaders = [h for h in self.headers.items()
-                        if h[0].startswith('x-')]
-        self.log_message('"%s" %s %s%s',
+                        if h[0].startswith(r'x-')]
+        self.log_message(r'"%s" %s %s%s',
                          self.requestline, str(code), str(size),
-                         ''.join([' %s:%s' % h for h in sorted(xheaders)]))
+                         r''.join([r' %s:%s' % h for h in sorted(xheaders)]))
 
     def do_write(self):
         try:
@@ -101,9 +102,13 @@ 
             self._start_response("500 Internal Server Error", [])
             self._write("Internal Server Error")
             self._done()
-            tb = "".join(traceback.format_exception(*sys.exc_info()))
-            self.log_error("Exception happened during processing "
-                           "request '%s':\n%s", self.path, tb)
+            tb = r"".join(traceback.format_exception(*sys.exc_info()))
+            # We need a native-string newline to poke in the log
+            # message, because we won't get a newline when using an
+            # r-string. This is the easy way out.
+            newline = chr(10)
+            self.log_error(r"Exception happened during processing "
+                           r"request '%s':%s%s", self.path, newline, tb)
 
     def do_GET(self):
         self.do_POST()
@@ -331,4 +336,4 @@ 
         return cls(ui, app, (address, port), handler)
     except socket.error as inst:
         raise error.Abort(_("cannot start server at '%s:%d': %s")
-                         % (address, port, inst.args[1]))
+                         % (address, port, pycompat.sysbytes(inst.args[1])))