Repository: beetbox/beets
Pull request: feat(import): Add support for reading skipped paths from logfile
## Description

Fixes #4379.

## To Do

- [X] Documentation. (If you've add a new command-line flag, for example, find the appropriate page under `docs/` to describe it.)
- [X] Changelog. (Add an entry to `docs/changelog.rst` near the top of the document.)
- [X] Test. (Encouraged but not strictly required.)

Problem statement:
Allow importing from logfile
When importing large collections, it makes sense to do the automated import first, then do the manual part afterwards:

    $ beet import -l skipped.log -q /path/to/library

It would be great if it was possible to continue the import using the logfile instead of a directory:

    $ beet import skipped.log

Beets should be able to parse the logfile and call import on each directory that was skipped.

Hints from issue discussion:
My current workaround:

```
$ grep -v "^import" missing.log | sed "s/^[a-z-]\+ //" | python <(cat << EOF
import sys
import os.path
for line in sys.stdin:
    print(os.path.commonpath(line.rstrip("\n").split("; ")))
EOF
) | xargs -d $'\n' beet import 
```
I use a sricpt sh for that:

```
date=$(date '+%Y%m%d' | cut -c 3-)

grep skip ~/.config/beets/import.log | sed 's/skip //' > ~/.config/beets/skipped_import.log

mv ~/.config/beets/import.log ~/.config/beets/import.log."$date"

manual_import() {
until [ ! -s ~/.config/beets/skipped_import.log ]
do
  chek=$(head -n 1 ~/.config/beets/skipped_import.log)
  beet import -CW "$chek" &&
  sed -i '1d' ~/.config/beets/skipped_import.log
done
}

manual_import
```
I'm pretty sure that a feature like this has been discussed before; but I can't find the issue right now.
Well, it looks like the log file format is intended to be machine-readable, so it's not really far-fetched to assume that such a feature was considered at some point.

Commit message: feat(import): Add support for reading skipped paths from logfile

Fixes #4379.

Patch to review:
diff --git a/beets/ui/commands.py b/beets/ui/commands.py
index 3a33740133..b0921728dd 100755
--- a/beets/ui/commands.py
+++ b/beets/ui/commands.py
@@ -79,6 +79,26 @@ def _do_query(lib, query, album, also_items=True):
     return items, albums
 
 
+def _paths_from_logfile(path):
+    """Parse the logfile and yield skipped paths to pass to the `import`
+    command.
+    """
+    with open(path, mode="r", encoding="utf-8") as fp:
+        for line in fp:
+            verb, sep, paths = line.rstrip("\n").partition(" ")
+            if not sep:
+                raise ValueError("malformed line in logfile")
+
+            # Ignore informational lines that don't need to be re-imported.
+            if verb in {"import", "duplicate-keep", "duplicate-replace"}:
+                continue
+
+            if verb not in {"asis", "skip", "duplicate-skip"}:
+                raise ValueError(f"unknown verb {verb} found in logfile")
+
+            yield os.path.commonpath(paths.split("; "))
+
+
 # fields: Shows a list of available fields for queries and format strings.
 
 def _print_keys(query):
@@ -914,11 +934,30 @@ def import_files(lib, paths, query):
     query.
     """
     # Check the user-specified directories.
+    paths_to_import = []
     for path in paths:
-        if not os.path.exists(syspath(normpath(path))):
+        normalized_path = syspath(normpath(path))
+        if not os.path.exists(normalized_path):
             raise ui.UserError('no such file or directory: {}'.format(
                 displayable_path(path)))
 
+        # Read additional paths from logfile.
+        if os.path.isfile(normalized_path):
+            try:
+                paths_to_import.extend(list(
+                    _paths_from_logfile(normalized_path)))
+            except ValueError:
+                # We could raise an error here, but it's possible that the user
+                # tried to import a media file instead of a logfile. In that
+                # case, logging a warning would be confusing, so we skip this
+                # here.
+                pass
+            else:
+                # Don't re-add the log file to the list of paths to import.
+                continue
+
+        paths_to_import.append(paths)
+
     # Check parameter consistency.
     if config['import']['quiet'] and config['import']['timid']:
         raise ui.UserError("can't be both quiet and timid")
@@ -939,11 +978,11 @@ def import_files(lib, paths, query):
             config['import']['quiet']:
         config['import']['resume'] = False
 
-    session = TerminalImportSession(lib, loghandler, paths, query)
+    session = TerminalImportSession(lib, loghandler, paths_to_import, query)
     session.run()
 
     # Emit event.
-    plugins.send('import', lib=lib, paths=paths)
+    plugins.send('import', lib=lib, paths=paths_to_import)
 
 
 def import_func(lib, opts, args):
diff --git a/docs/changelog.rst b/docs/changelog.rst
index dbee127648..a5086b6b9f 100644
--- a/docs/changelog.rst
+++ b/docs/changelog.rst
@@ -29,6 +29,9 @@ New features:
   :bug:`1840` :bug:`4302`
 * Added a ``-P`` (or ``--disable-plugins``) flag to specify one/multiple plugin(s) to be
   disabled at startup.
+* :ref:`import-options`: Add support for re-running the importer on paths in
+  log files that were created with the ``-l`` (or ``--logfile``) argument.
+  :bug:`4379` :bug:`4387`
 
 Bug fixes:
 
diff --git a/docs/guides/tagger.rst b/docs/guides/tagger.rst
index d890f5c08a..5184f433fd 100644
--- a/docs/guides/tagger.rst
+++ b/docs/guides/tagger.rst
@@ -80,6 +80,8 @@ all of these limitations.
 
 Now that that's out of the way, let's tag some music.
 
+.. _import-options:
+
 Options
 -------
 
@@ -101,7 +103,8 @@ command-line options you should know:
 * ``beet import -l LOGFILE``: write a message to ``LOGFILE`` every time you skip
   an album or choose to take its tags "as-is" (see below) or the album is
   skipped as a duplicate; this lets you come back later and reexamine albums
-  that weren't tagged successfully
+  that weren't tagged successfully. Run ``beet import LOGFILE`` rerun the
+  importer on such paths from the logfile.
 
 * ``beet import -q``: quiet mode. Never prompt for input and, instead,
   conservatively skip any albums that need your opinion. The ``-ql`` combination


Code review comments:
diff_hunk: @@ -79,6 +79,26 @@ def _do_query(lib, query, album, also_items=True):
     return items, albums
 
 
+def _paths_from_logfile(path):
+    """Parse the logfile and yield skipped paths to pass to the `import`
+    command.
+    """
+    with open(path, mode="r", encoding="utf-8") as fp:
+        for line in fp:
+            verb, sep, paths = line.rstrip("\n").partition(" ")
+            if not sep:
+                raise ValueError("malformed line in logfile")
path: beets/ui/commands.py
text: @user1:
I'm not sure `ValueError` is the right thing here. I believe we want to surface this kind of error to the user (assuming we change the interface to be explicit about the file type), so maybe `ui.UserError` is more appropriate—or a new subclass thereof?

@author:
I thought that at this point the error is "internal". `os.path.commonpath` might raise a `ValueError` too. The error is caught in the calling code and could be used to raise a `ui.UserError`. I didn't do that yet because without an additional flag we can't  distinguish logfiles from audio files yet.

@author:
error handling is here: https://github.com/beetbox/beets/pull/4387/commits/5128bcd73cd67e046f2d24c2e3b1de2f7859f176#diff-e324b20657a7d6b43b8b7aeb5754b96774f5062294b5ba7f1e3062845e9e7044R984-R991

@user1:
Got it; this makes sense to me!