Patchwork blackbox: add an option to customize the name of the log file

login
register
mail settings
Submitter Jun Wu
Date June 13, 2016, 7:32 p.m.
Message ID <9af8429456793d482c67.1465846325@x1c>
Download mbox | patch
Permalink /patch/15477/
State Changes Requested
Headers show

Comments

Jun Wu - June 13, 2016, 7:32 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1465846020 -3600
#      Mon Jun 13 20:27:00 2016 +0100
# Node ID 9af8429456793d482c670b0aa7c829b1513b2025
# Parent  c27dc3c31222c7f74331221a3d25566146feecac
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 9af842945679
blackbox: add an option to customize the name of the log file

In some setups, bots run hg commands frequently and it could flood the blacklog
and make it hard to find commands executed by humans. This patch adds a config
option so the bot can write to a separate file.
timeless - June 14, 2016, 6:12 p.m.
Looks good to me
On Jun 13, 2016 3:42 PM, "Jun Wu" <quark@fb.com> wrote:

> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1465846020 -3600
> #      Mon Jun 13 20:27:00 2016 +0100
> # Node ID 9af8429456793d482c670b0aa7c829b1513b2025
> # Parent  c27dc3c31222c7f74331221a3d25566146feecac
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r
> 9af842945679
> blackbox: add an option to customize the name of the log file
>
> In some setups, bots run hg commands frequently and it could flood the
> blacklog
> and make it hard to find commands executed by humans. This patch adds a
> config
> option so the bot can write to a separate file.
>
> diff --git a/hgext/blackbox.py b/hgext/blackbox.py
> --- a/hgext/blackbox.py
> +++ b/hgext/blackbox.py
> @@ -33,6 +33,9 @@
>    # rotate up to N log files when the current one gets too big
>    maxfiles = 3
>
> +  [blackbox]
> +  # use a different file name: blackbox-bot.log. only a-z and 0-9 can be
> used
> +  filesuffix = bot
>  """
>
>  from __future__ import absolute_import
> @@ -60,15 +63,24 @@
>
>  filehandles = {}
>
> -def _openlog(vfs):
> -    path = vfs.join('blackbox.log')
> +def _getname(ui):
> +    suffix = ui.config('blackbox', 'filesuffix')
> +    if suffix is not None:
> +        suffix = re.sub(r'[^a-z0-9]', '', suffix)
> +    if suffix:
> +        return 'blackbox-%s.log' % suffix
> +    else:
> +        return 'blackbox.log'
> +
> +def _openlog(vfs, name):
> +    path = vfs.join(name)
>      if path in filehandles:
>          return filehandles[path]
> -    filehandles[path] = fp = vfs('blackbox.log', 'a')
> +    filehandles[path] = fp = vfs(name, 'a')
>      return fp
>
> -def _closelog(vfs):
> -    path = vfs.join('blackbox.log')
> +def _closelog(vfs, name):
> +    path = vfs.join(name)
>      fp = filehandles[path]
>      del filehandles[path]
>      fp.close()
> @@ -117,20 +129,21 @@
>                          self.debug("warning: cannot rename '%s' to '%s':
> %s\n" %
>                                     (newpath, oldpath, err.strerror))
>
> -            fp = _openlog(self._bbvfs)
> +            name = _getname(self)
> +            fp = _openlog(self._bbvfs, name)
>              maxsize = self.configbytes('blackbox', 'maxsize', 1048576)
>              if maxsize > 0:
>                  st = self._bbvfs.fstat(fp)
>                  if st.st_size >= maxsize:
>                      path = fp.name
> -                    _closelog(self._bbvfs)
> +                    _closelog(self._bbvfs, name)
>                      maxfiles = self.configint('blackbox', 'maxfiles', 7)
>                      for i in xrange(maxfiles - 1, 1, -1):
>                          rotate(oldpath='%s.%d' % (path, i - 1),
>                                 newpath='%s.%d' % (path, i))
>                      rotate(oldpath=path,
>                             newpath=maxfiles > 0 and path + '.1')
> -                    fp = _openlog(self._bbvfs)
> +                    fp = _openlog(self._bbvfs, name)
>              return fp
>
>          def _bbwrite(self, fmt, *args):
> @@ -229,11 +242,12 @@
>      '''view the recent repository events
>      '''
>
> -    if not repo.vfs.exists('blackbox.log'):
> +    name = _getname(ui)
> +    if not repo.vfs.exists(name):
>          return
>
>      limit = opts.get('limit')
> -    fp = repo.vfs('blackbox.log', 'r')
> +    fp = repo.vfs(name, 'r')
>      lines = fp.read().split('\n')
>
>      count = 0
> diff --git a/tests/test-blackbox.t b/tests/test-blackbox.t
> --- a/tests/test-blackbox.t
> +++ b/tests/test-blackbox.t
> @@ -205,5 +205,20 @@
>    $ hg id --config extensions.x=../r.py --config blackbox.dirty=True
>    45589e459b2e tip
>
> +Test filesuffix
> +
> +  $ cat >> $HGRCPATH <<EOF
> +  > [blackbox]
> +  > filesuffix=foo
> +  > EOF
> +
> +  $ hg status
> +  $ ls .hg/blackbox-foo.log
> +  .hg/blackbox-foo.log
> +  $ hg blackbox
> +  1970/01/01 00:00:00 bob @45589e459b2edfbf3dbde7e01f611d2c1e7453d7
> (5000)> status
> +  1970/01/01 00:00:00 bob @45589e459b2edfbf3dbde7e01f611d2c1e7453d7
> (5000)> status exited 0 after * seconds (glob)
> +  1970/01/01 00:00:00 bob @45589e459b2edfbf3dbde7e01f611d2c1e7453d7
> (5000)> blackbox
> +
>  cleanup
>    $ cd ..
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Yuya Nishihara - June 16, 2016, 1:55 p.m.
On Mon, 13 Jun 2016 20:32:05 +0100, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1465846020 -3600
> #      Mon Jun 13 20:27:00 2016 +0100
> # Node ID 9af8429456793d482c670b0aa7c829b1513b2025
> # Parent  c27dc3c31222c7f74331221a3d25566146feecac
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 9af842945679
> blackbox: add an option to customize the name of the log file

> +  [blackbox]
> +  # use a different file name: blackbox-bot.log. only a-z and 0-9 can be used
> +  filesuffix = bot

It seems confusing that filesuffix isn't a suffix. Is there a reason to
not allow specifying the full file name? Invalid path components such as
'..' will be rejected by pathauditor.

> +def _getname(ui):
> +    suffix = ui.config('blackbox', 'filesuffix')
> +    if suffix is not None:
> +        suffix = re.sub(r'[^a-z0-9]', '', suffix)

Perhaps it would be better to report error than stripping bad characters
silently.
Yuya Nishihara - June 17, 2016, 2:36 p.m.
On Thu, 16 Jun 2016 15:20:33 +0100, Jun Wu wrote:
> Excerpts from Yuya Nishihara's message of 2016-06-16 22:55:45 +0900:
> > It seems confusing that filesuffix isn't a suffix. Is there a reason to
> > not allow specifying the full file name? Invalid path components such as
> > '..' will be rejected by pathauditor.
> 
> I'd like to have flexible full names as well.
> 
> My main concern is names like "requires", "dirstate", "hgrc" are dangerous.
> But if we move them to ".hg/blackbox" and disallow "/", then everything will
> be fine.
> 
> If putting everything under "blackbox" subdirectory (including blackbox.log)
> makes sense, I can prepare the patch. If compatibilities are a concern, I
> can symlink blackbox.log.

Or use the "blackbox" directory only if the option is specified. Perhaps you'll
get better suggestion from your colleagues as they are main users of the
extension?

Either ways, symlink shouldn't be the solution. It doesn't work on Windows as
of Python 2.x.

Patch

diff --git a/hgext/blackbox.py b/hgext/blackbox.py
--- a/hgext/blackbox.py
+++ b/hgext/blackbox.py
@@ -33,6 +33,9 @@ 
   # rotate up to N log files when the current one gets too big
   maxfiles = 3
 
+  [blackbox]
+  # use a different file name: blackbox-bot.log. only a-z and 0-9 can be used
+  filesuffix = bot
 """
 
 from __future__ import absolute_import
@@ -60,15 +63,24 @@ 
 
 filehandles = {}
 
-def _openlog(vfs):
-    path = vfs.join('blackbox.log')
+def _getname(ui):
+    suffix = ui.config('blackbox', 'filesuffix')
+    if suffix is not None:
+        suffix = re.sub(r'[^a-z0-9]', '', suffix)
+    if suffix:
+        return 'blackbox-%s.log' % suffix
+    else:
+        return 'blackbox.log'
+
+def _openlog(vfs, name):
+    path = vfs.join(name)
     if path in filehandles:
         return filehandles[path]
-    filehandles[path] = fp = vfs('blackbox.log', 'a')
+    filehandles[path] = fp = vfs(name, 'a')
     return fp
 
-def _closelog(vfs):
-    path = vfs.join('blackbox.log')
+def _closelog(vfs, name):
+    path = vfs.join(name)
     fp = filehandles[path]
     del filehandles[path]
     fp.close()
@@ -117,20 +129,21 @@ 
                         self.debug("warning: cannot rename '%s' to '%s': %s\n" %
                                    (newpath, oldpath, err.strerror))
 
-            fp = _openlog(self._bbvfs)
+            name = _getname(self)
+            fp = _openlog(self._bbvfs, name)
             maxsize = self.configbytes('blackbox', 'maxsize', 1048576)
             if maxsize > 0:
                 st = self._bbvfs.fstat(fp)
                 if st.st_size >= maxsize:
                     path = fp.name
-                    _closelog(self._bbvfs)
+                    _closelog(self._bbvfs, name)
                     maxfiles = self.configint('blackbox', 'maxfiles', 7)
                     for i in xrange(maxfiles - 1, 1, -1):
                         rotate(oldpath='%s.%d' % (path, i - 1),
                                newpath='%s.%d' % (path, i))
                     rotate(oldpath=path,
                            newpath=maxfiles > 0 and path + '.1')
-                    fp = _openlog(self._bbvfs)
+                    fp = _openlog(self._bbvfs, name)
             return fp
 
         def _bbwrite(self, fmt, *args):
@@ -229,11 +242,12 @@ 
     '''view the recent repository events
     '''
 
-    if not repo.vfs.exists('blackbox.log'):
+    name = _getname(ui)
+    if not repo.vfs.exists(name):
         return
 
     limit = opts.get('limit')
-    fp = repo.vfs('blackbox.log', 'r')
+    fp = repo.vfs(name, 'r')
     lines = fp.read().split('\n')
 
     count = 0
diff --git a/tests/test-blackbox.t b/tests/test-blackbox.t
--- a/tests/test-blackbox.t
+++ b/tests/test-blackbox.t
@@ -205,5 +205,20 @@ 
   $ hg id --config extensions.x=../r.py --config blackbox.dirty=True
   45589e459b2e tip
 
+Test filesuffix
+
+  $ cat >> $HGRCPATH <<EOF
+  > [blackbox]
+  > filesuffix=foo
+  > EOF
+
+  $ hg status
+  $ ls .hg/blackbox-foo.log
+  .hg/blackbox-foo.log
+  $ hg blackbox
+  1970/01/01 00:00:00 bob @45589e459b2edfbf3dbde7e01f611d2c1e7453d7 (5000)> status
+  1970/01/01 00:00:00 bob @45589e459b2edfbf3dbde7e01f611d2c1e7453d7 (5000)> status exited 0 after * seconds (glob)
+  1970/01/01 00:00:00 bob @45589e459b2edfbf3dbde7e01f611d2c1e7453d7 (5000)> blackbox
+
 cleanup
   $ cd ..