Patchwork [1,of,2] blackbox: prevent failed I/O from causing hg to abort

login
register
mail settings
Submitter Bryan O'Sullivan
Date March 19, 2013, 11 p.m.
Message ID <9a44c44337c701d62124.1363734037@australite.thefacebook.com>
Download mbox | patch
Permalink /patch/1139/
State Accepted
Commit ed39a8f94e957455ae49faad040fe35154a36dba
Headers show

Comments

Bryan O'Sullivan - March 19, 2013, 11 p.m.
# HG changeset patch
# User Bryan O'Sullivan <bryano@fb.com>
# Date 1363734026 25200
# Node ID 9a44c44337c701d6212454b993caa5f778254f56
# Parent  de0601436fb6156c173abc2d7a6099f0c0b6b7cb
blackbox: prevent failed I/O from causing hg to abort

Instead, we simply print a warning message if opening the blackbox log
file fails, or if writing to it fails.
Durham Goode - March 19, 2013, 11:50 p.m.
On 3/19/13 4:00 PM, "Bryan O'Sullivan" <bos@serpentine.com> wrote:

># HG changeset patch
># User Bryan O'Sullivan <bryano@fb.com>
># Date 1363734026 25200
># Node ID 9a44c44337c701d6212454b993caa5f778254f56
># Parent  de0601436fb6156c173abc2d7a6099f0c0b6b7cb
>blackbox: prevent failed I/O from causing hg to abort
>
>Instead, we simply print a warning message if opening the blackbox log
>file fails, or if writing to it fails.

Overall looks good.  Should we even bother writing out a warning?  I can
imagine the case where the repo is readonly for most users, but still
wants the blackbox on for admin users.  In that case the normal users
would see the warning every time right?
Bryan O'Sullivan - March 20, 2013, 8:23 p.m.
On Tue, Mar 19, 2013 at 4:50 PM, Durham Goode <durham@fb.com> wrote:

> Overall looks good.  Should we even bother writing out a warning?  I can
> imagine the case where the repo is readonly for most users, but still
> wants the blackbox on for admin users.  In that case the normal users
> would see the warning every time right?
>

That's true. I'll change the use of warn to debug, so that if something
goes wrong with logging, you can have *some* hope of finding out.
Bryan O'Sullivan - March 20, 2013, 8:41 p.m.
On Wed, Mar 20, 2013 at 1:23 PM, Bryan O'Sullivan <bos@serpentine.com>wrote:

>
>
I'll change the use of warn to debug, so that if something goes wrong with
> logging, you can have *some* hope of finding out.
>

Patches are now in crew, with your suggested changes. Thanks for the quick
review.

Patch

diff --git a/hgext/blackbox.py b/hgext/blackbox.py
--- a/hgext/blackbox.py
+++ b/hgext/blackbox.py
@@ -57,11 +57,20 @@  def wrapui(ui):
                 date = util.datestr(None, '%Y/%m/%d %H:%M:%S')
                 user = getpass.getuser()
                 formattedmsg = msg[0] % msg[1:]
-                blackbox.write('%s %s> %s' % (date, user, formattedmsg))
+                try:
+                    blackbox.write('%s %s> %s' % (date, user, formattedmsg))
+                except IOError, err:
+                    self.warn(_('warning: cannot write to blackbox.log: %s\n') %
+                              err.strerror)
                 lastblackbox = blackbox
 
         def setrepo(self, repo):
-            self._blackbox = repo.opener('blackbox.log', 'a')
+            try:
+                self._blackbox = repo.opener('blackbox.log', 'a')
+            except IOError, err:
+                self.warn(_('warning: cannot write to blackbox.log: %s\n') %
+                          err.strerror)
+                self._blackbox = None
 
     ui.__class__ = blackboxui
 
diff --git a/tests/test-blackbox.t b/tests/test-blackbox.t
--- a/tests/test-blackbox.t
+++ b/tests/test-blackbox.t
@@ -61,6 +61,30 @@  clone, commit, pull
   1970/01/01 00:00:00 bob> 1 incoming changes - new heads: d02f48003e62
   1970/01/01 00:00:00 bob> pull exited None after * seconds (glob)
 
+we must not cause a failure if we cannot write to the log
+
+  $ hg rollback
+  repository tip rolled back to revision 1 (undo pull)
+  $ chmod 000 .hg/blackbox.log
+  $ hg pull
+  warning: cannot write to blackbox.log: Permission denied
+  pulling from $TESTTMP/blackboxtest
+  searching for changes
+  adding changesets
+  adding manifests
+  adding file changes
+  added 1 changesets with 1 changes to 1 files
+  (run 'hg update' to get a working copy)
+
+a failure reading from the log is fine
+
+  $ hg blackbox -l 3
+  warning: cannot write to blackbox.log: Permission denied
+  abort: Permission denied: $TESTTMP/blackboxtest2/.hg/blackbox.log
+  [255]
+
+  $ chmod 600 .hg/blackbox.log
+
 backup bundles get logged
 
   $ touch d