Repository: beetbox/beets

Issue:
hook: Interpolate paths (and other bytestrings) correctly into commands
### Problem

I have the following configuration for the Hook plugin in my config.yaml:

```
hook:
    hooks:
        - event: album_imported
          command: /usr/bin/ls -l "{album.path}"
```

This is just a test to see how beets presents the path values.  It appears that the paths are returned as bytes objects rather than strings.  This is problematic when using the path values as arguments for external shell scripts.  As can be seen below, the shell is unable to use the value provided by {album.path}.


```sh
$ beet -vv import /tmp/music/new/Al\ Di\ Meola\ -\ Elegant\ Gypsy/
```

Led to this problem:

```
hook: running command "/usr/bin/ls -l b'/tmp/music/FLAC/Al Di Meola/Elegant Gypsy'" for event album_imported
/usr/bin/ls: cannot access "b'/tmp/music/FLAC/Al Di Meola/Elegant Gypsy'": No such file or directory

```

The path "/tmp/music/FLAC/Al Di Meola/Elegant Gypsy" does exist on the filesystem after the import is complete.



### Setup

* OS: Arch Linux
* Python version: 3.4.5
* beets version: 1.4.7
* Turning off plugins made problem go away (yes/no): This issue is related to a plugin, so I didn't turn them off

My configuration (output of `beet config`) is:

```yaml
plugins: inline convert badfiles info missing lastgenre fetchart mbsync scrub smartplaylist hook
directory: /tmp/music/FLAC
library: ~/.config/beets/library.db

import:
    copy: yes
    write: yes
    log: ~/.config/beets/import.log
    languages: en
per_disc_numbering: yes

paths:
    default: $albumartist/$album%aunique{}/$disc_and_track - $title
    comp: $albumartist/$album%aunique{}/$disc_and_track - $title
item_fields:
    disc_and_track: u'%01i-%02i' % (disc, track) if disctotal > 1 else u'%02i' % (track)

ui:
    color: yes

match:
    ignored: missing_tracks unmatched_tracks
convert:
    copy_album_art: yes
    dest: /tmp/music/ogg
    embed: yes
    never_convert_lossy_files: yes
    format: ogg
    formats:
        ogg:
            command: oggenc -Q -q 4 -o $dest $source
            extension: ogg
        aac:
            command: ffmpeg -i $source -y -vn -acodec aac -aq 1 $dest
            extension: m4a
        alac:
            command: ffmpeg -i $source -y -vn -acodec alac $dest
            extension: m4a
        flac: ffmpeg -i $source -y -vn -acodec flac $dest
        mp3: ffmpeg -i $source -y -vn -aq 2 $dest
        opus: ffmpeg -i $source -y -vn -acodec libopus -ab 96k $dest
        wma: ffmpeg -i $source -y -vn -acodec wmav2 -vn $dest
    pretend: no
    threads: 4
    max_bitrate: 500
    auto: no
    tmpdir:
    quiet: no

    paths: {}
    no_convert: ''
    album_art_maxwidth: 0
lastgenre:
    force: yes
    prefer_specific: no
    min_weight: 20
    count: 4
    separator: '; '
    whitelist: yes
    fallback:
    canonical: no
    source: album
    auto: yes
fetchart:
    sources: filesystem coverart amazon albumart
    auto: yes
    minwidth: 0
    maxwidth: 0
    enforce_ratio: no
    cautious: no
    cover_names:
    - cover
    - front
    - art
    - album
    - folder
    google_key: REDACTED
    google_engine: 001442825323518660753:hrh5ch1gjzm
    fanarttv_key: REDACTED
    store_source: no
hook:
    hooks: [{event: album_imported, command: '/usr/bin/ls -l "{album.path}"'}]
pathfields: {}
album_fields: {}
scrub:
    auto: yes
missing:
    count: no
    total: no
    album: no
smartplaylist:
    relative_to:
    playlist_dir: .
    auto: yes
    playlists: []
```

I created a Python 2 virtual environment, installed beets and any dependencies in to that virtualenv, cleaned my test library, and imported the same files using the same config.yaml.  This time the shell  was able to use the path value returned by the hook configuration:

```
hook: running command "/usr/bin/ls -l /tmp/music/FLAC/Al Di Meola/Elegant Gypsy" for event album_imported
total 254944
-rw-r--r-- 1 mike mike 50409756 Jun 24 13:46 01 - Flight Over Rio.flac
-rw-r--r-- 1 mike mike 43352354 Jun 24 13:46 02 - Midnight Tango.flac
-rw-r--r-- 1 mike mike  7726389 Jun 24 13:46 03 - Percussion Intro.flac
-rw-r--r-- 1 mike mike 32184646 Jun 24 13:46 04 - Mediterranean Sundance.flac
-rw-r--r-- 1 mike mike 45770796 Jun 24 13:46 05 - Race With Devil on Spanish Highway.flac
-rw-r--r-- 1 mike mike 10421006 Jun 24 13:46 06 - Lady of Rome, Sister of Brazil.flac
-rw-r--r-- 1 mike mike 65807504 Jun 24 13:46 07 - Elegant Gypsy Suite.flac
-rw-r--r-- 1 mike mike  5366515 Jun 24 13:46 cover.jpg
```
I'm guessing this is due to a data type difference between Python 2 and Python 3.

Discussion:
jackwilsdon: Please ignore the failed `continuous-integration/appveyor/branch` and `continuous-integration/travis-ci/push` tests - I've upset Travis and AppVeyor by accidentally pushing the branch to this repository instead of my fork and then deleting the branch I pushed here.
sampsyo: Looks perfect; thank you!!

Could you please add a quick changelog entry? It also looks like `.test_hook.py.swp` got committed by mistake—you might want to add `.*.swp` to your global `.gitignore`.
jackwilsdon: Oops! Thought I'd already removed that 😖 rookie mistake.

Added a changelog entry and removed the swapfile 👍 

Patch:
diff --git a/beetsplug/hook.py b/beetsplug/hook.py
index de44c1b81..ac0c4acad 100644
--- a/beetsplug/hook.py
+++ b/beetsplug/hook.py
@@ -18,7 +18,6 @@ from __future__ import division, absolute_import, print_function
 
 import string
 import subprocess
-import six
 
 from beets.plugins import BeetsPlugin
 from beets.util import shlex_split, arg_encoding
@@ -46,10 +45,8 @@ class CodingFormatter(string.Formatter):
 
         See str.format and string.Formatter.format.
         """
-        try:
+        if isinstance(format_string, bytes):
             format_string = format_string.decode(self._coding)
-        except UnicodeEncodeError:
-            pass
 
         return super(CodingFormatter, self).format(format_string, *args,
                                                    **kwargs)
@@ -96,10 +93,7 @@ class HookPlugin(BeetsPlugin):
                 return
 
             # Use a string formatter that works on Unicode strings.
-            if six.PY2:
-                formatter = CodingFormatter(arg_encoding())
-            else:
-                formatter = string.Formatter()
+            formatter = CodingFormatter(arg_encoding())
 
             command_pieces = shlex_split(command)
 
diff --git a/docs/changelog.rst b/docs/changelog.rst
index f311571d5..7e98a8360 100644
--- a/docs/changelog.rst
+++ b/docs/changelog.rst
@@ -150,6 +150,8 @@ Fixes:
 * :doc:`/plugins/badfiles`: Avoid a crash when the underlying tool emits
   undecodable output.
   :bug:`3165`
+* :doc:`/plugins/hook`: Fix byte string interpolation in hook commands.
+  :bug:`2967` :bug:`3167`
 
 .. _python-itunes: https://github.com/ocelma/python-itunes
 


Test patch:
diff --git a/test/test_hook.py b/test/test_hook.py
index 39fd08959..81363c73c 100644
--- a/test/test_hook.py
+++ b/test/test_hook.py
@@ -110,6 +110,25 @@ class HookTest(_common.TestCase, TestHelper):
             self.assertTrue(os.path.isfile(path))
             os.remove(path)
 
+    def test_hook_bytes_interpolation(self):
+        temporary_paths = [
+            get_temporary_path().encode('utf-8')
+            for i in range(self.TEST_HOOK_COUNT)
+        ]
+
+        for index, path in enumerate(temporary_paths):
+            self._add_hook('test_bytes_event_{0}'.format(index),
+                           'touch "{path}"')
+
+        self.load_plugins('hook')
+
+        for index, path in enumerate(temporary_paths):
+            plugins.send('test_bytes_event_{0}'.format(index), path=path)
+
+        for path in temporary_paths:
+            self.assertTrue(os.path.isfile(path))
+            os.remove(path)
+
 
 def suite():
     return unittest.TestLoader().loadTestsFromName(__name__)