aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorCraig Jennings <c@cjennings.net>2026-08-03 12:57:36 -0500
committerCraig Jennings <c@cjennings.net>2026-08-03 12:57:36 -0500
commit90aa02c5cd7dc78729d4362d4962005627069e60 (patch)
treee2f0d298794c43a24669b3d27b390b31a38693b2
parentfa1e63e4b3e4dbe3841247c8efd2096d2d75390c (diff)
downloaddotemacs-90aa02c5cd7dc78729d4362d4962005627069e60.tar.gz
dotemacs-90aa02c5cd7dc78729d4362d4962005627069e60.zip
fix(music): base an appended m3u path on the playlist, not the music root
cj/music--append-track-to-m3u-file wrote every relative path against cj/music-root wherever the playlist actually sat. Both readers resolve against the playlist's own directory: cj/music--m3u-file-tracks, and EMMS's emms-source-playlist-parse-m3u. So the writer and the readers disagreed whenever those two directories were not the same one. That is not only the playlist-outside-the-music-root case I went looking for. 39 of the 66 m3u files under ~/music are album playlists sitting beside their tracks, storing bare filenames. Appending to any of them wrote a line like Artist/Album/track.flac that resolved to a path which never existed. The fix is the base: (file-name-directory m3u-file). Worth recording how the tests went, because the first three I wrote all passed against the broken writer. I had put cj/music-root and the playlist directory at the same depth, so file-relative-name returned the same string for both bases. The fixture reached the bug's code path and still could not see it. The discriminating cases put the two bases at different depths, and reverting the one-line fix now fails them. Eight existing tests moved with it. They built track paths from the real cj/music-root while the playlist sat in a temp directory, which pinned the old base and also read my actual music root. Rebinding the root to the fixture directory restores every assertion verbatim and makes them hermetic. One thing this does not change: MPD resolves relative playlist entries against music_directory and rejects entries that escape it, so a ../ line is for EMMS rather than MPD. That costs nothing today, since EMMS drives mpv here and MPD is not running.
-rw-r--r--modules/music-config.el10
-rw-r--r--tests/test-music-config--append-track-to-m3u-file.el119
2 files changed, 122 insertions, 7 deletions
diff --git a/modules/music-config.el b/modules/music-config.el
index 92b04782..075ed49b 100644
--- a/modules/music-config.el
+++ b/modules/music-config.el
@@ -677,9 +677,15 @@ M3U-FILE should be an existing, writable M3U file path."
(unless (file-writable-p m3u-file)
(error "M3U file is not writable: %s" m3u-file))
- ;; Convert absolute path to relative path from music root
+ ;; Write the path relative to the playlist's own directory, which is the base
+ ;; `cj/music--m3u-file-tracks' and EMMS both resolve against. This used to be
+ ;; `cj/music-root'. For a playlist inside the music root the two are the same
+ ;; directory, so the disagreement stayed invisible until a playlist lived
+ ;; somewhere else -- then the appended line resolved against the wrong base and
+ ;; pointed at a file that was never there.
(let ((relative-path (if (file-name-absolute-p track-path)
- (file-relative-name track-path cj/music-root)
+ (file-relative-name track-path
+ (file-name-directory m3u-file))
track-path)))
;; Determine if we need a leading newline
(let ((needs-prefix-newline nil)
diff --git a/tests/test-music-config--append-track-to-m3u-file.el b/tests/test-music-config--append-track-to-m3u-file.el
index be0cbd8e..06327b9d 100644
--- a/tests/test-music-config--append-track-to-m3u-file.el
+++ b/tests/test-music-config--append-track-to-m3u-file.el
@@ -39,7 +39,8 @@
"Append to brand new empty M3U file."
(test-music-config--append-track-to-m3u-file-setup)
(unwind-protect
- (let* ((m3u-file (cj/create-temp-test-file "test-playlist-"))
+ (let* ((cj/music-root (cj/create-test-base-dir))
+ (m3u-file (cj/create-temp-test-file "test-playlist-"))
(track-path (expand-file-name "artist/song.mp3" cj/music-root))
(expected-relative "artist/song.mp3"))
(cj/music--append-track-to-m3u-file track-path m3u-file)
@@ -53,6 +54,7 @@
(test-music-config--append-track-to-m3u-file-setup)
(unwind-protect
(let* ((existing-content "first.mp3\n")
+ (cj/music-root (cj/create-test-base-dir))
(m3u-file (cj/create-temp-test-file-with-content existing-content "test-playlist-"))
(track-path (expand-file-name "second.mp3" cj/music-root))
(expected-relative "second.mp3"))
@@ -68,6 +70,7 @@
(test-music-config--append-track-to-m3u-file-setup)
(unwind-protect
(let* ((existing-content "first.mp3")
+ (cj/music-root (cj/create-test-base-dir))
(m3u-file (cj/create-temp-test-file-with-content existing-content "test-playlist-"))
(track-path (expand-file-name "second.mp3" cj/music-root))
(expected-relative "second.mp3"))
@@ -82,7 +85,8 @@
"Multiple appends to same file all succeed (allows duplicates)."
(test-music-config--append-track-to-m3u-file-setup)
(unwind-protect
- (let* ((m3u-file (cj/create-temp-test-file "test-playlist-"))
+ (let* ((cj/music-root (cj/create-test-base-dir))
+ (m3u-file (cj/create-temp-test-file "test-playlist-"))
(track1 (expand-file-name "track1.mp3" cj/music-root))
(track2 (expand-file-name "track2.mp3" cj/music-root))
(track1-duplicate (expand-file-name "track1.mp3" cj/music-root))
@@ -98,13 +102,112 @@
(concat rel1 "\n" rel2 "\n" rel1 "\n"))))))
(test-music-config--append-track-to-m3u-file-teardown)))
+;;; Normal Cases: round-trip with the reader
+
+(ert-deftest test-music-config--append-track-to-m3u-file-normal-round-trips-through-the-reader ()
+ "Normal: the same-directory case round-trips through the reader.
+A positive control only. With the playlist and the music root in one
+directory both candidate bases produce the same string, so this passes
+against the old writer too — the discriminating cases are the two tests
+below, which put the bases at different depths."
+ (test-music-config--append-track-to-m3u-file-setup)
+ (unwind-protect
+ (let* ((base (cj/create-test-base-dir))
+ (cj/music-root base)
+ (m3u-file (cj/create-temp-test-file "test-playlist-"))
+ (track-path (expand-file-name "artist/song.mp3" base)))
+ (cj/music--append-track-to-m3u-file track-path m3u-file)
+ (should (equal (cj/music--m3u-file-tracks m3u-file)
+ (list track-path))))
+ (test-music-config--append-track-to-m3u-file-teardown)))
+
+(ert-deftest test-music-config--append-track-to-m3u-file-normal-round-trips-outside-the-music-root ()
+ "Normal/regression: a playlist living outside `cj/music-root' round-trips.
+This is the case the old writer got wrong. It based every relative path on
+`cj/music-root' wherever the playlist sat, while the reader resolved against
+the playlist's directory. Inside the music root the two coincide, which is
+why the defect stayed invisible until a playlist moved out of it."
+ (test-music-config--append-track-to-m3u-file-setup)
+ (unwind-protect
+ ;; The layout mirrors the real one: playlists/ and audio/ are siblings
+ ;; under mpd/, and the music root is a separate tree at a different depth.
+ ;; The depth difference is load-bearing -- put the music root alongside
+ ;; playlists/ instead and both bases yield the same relative path, so the
+ ;; test passes against the broken writer and proves nothing.
+ (let* ((base (cj/create-test-base-dir))
+ (playlists (expand-file-name "mpd/playlists/" base))
+ (audio (expand-file-name "mpd/audio/" base))
+ (cj/music-root (expand-file-name "music/" base))
+ (m3u-file (expand-file-name "ambience.m3u" playlists))
+ (track-path (expand-file-name "rain-loop.mp3" audio)))
+ (make-directory playlists t)
+ (make-directory audio t)
+ (make-directory cj/music-root t)
+ (with-temp-buffer (write-file m3u-file))
+ (cj/music--append-track-to-m3u-file track-path m3u-file)
+ (should (equal (cj/music--m3u-file-tracks m3u-file)
+ (list track-path))))
+ (test-music-config--append-track-to-m3u-file-teardown)))
+
+(ert-deftest test-music-config--append-track-to-m3u-file-normal-base-is-the-playlist-not-the-music-root ()
+ "Normal: the written line is relative to the playlist's own directory.
+Stated directly as well as via the round-trip, so a reader change could not
+quietly make both sides agree on the wrong base."
+ (test-music-config--append-track-to-m3u-file-setup)
+ (unwind-protect
+ ;; Same depth asymmetry as the round-trip test above, and for the same
+ ;; reason: with the music root alongside playlists/ both bases agree and
+ ;; the assertion holds against the broken writer.
+ (let* ((base (cj/create-test-base-dir))
+ (playlists (expand-file-name "mpd/playlists/" base))
+ (audio (expand-file-name "mpd/audio/" base))
+ (cj/music-root (expand-file-name "music/" base))
+ (m3u-file (expand-file-name "ambience.m3u" playlists))
+ (track-path (expand-file-name "rain-loop.mp3" audio)))
+ (make-directory playlists t)
+ (make-directory audio t)
+ (make-directory cj/music-root t)
+ (with-temp-buffer (write-file m3u-file))
+ (cj/music--append-track-to-m3u-file track-path m3u-file)
+ (with-temp-buffer
+ (insert-file-contents m3u-file)
+ (should (string= (buffer-string) "../audio/rain-loop.mp3\n"))))
+ (test-music-config--append-track-to-m3u-file-teardown)))
+
+(ert-deftest test-music-config--append-track-to-m3u-file-normal-deep-parent-chain-round-trips ()
+ "Normal: a track several levels above the playlist round-trips.
+The single \"../\" the tests above exercise is the shallow case. Once
+playlists and audio can live anywhere, a multi-level chain is the shape most
+likely to be broken by a later \"let's normalize these paths\" edit, and
+nothing else here would catch it."
+ (test-music-config--append-track-to-m3u-file-setup)
+ (unwind-protect
+ (let* ((base (cj/create-test-base-dir))
+ (playlists (expand-file-name "a/b/c/playlists/" base))
+ (cj/music-root (expand-file-name "music/" base))
+ (m3u-file (expand-file-name "deep.m3u" playlists))
+ (track-path (expand-file-name "faraway/song.mp3" base)))
+ (make-directory playlists t)
+ (make-directory (expand-file-name "faraway/" base) t)
+ (make-directory cj/music-root t)
+ (with-temp-buffer (write-file m3u-file))
+ (cj/music--append-track-to-m3u-file track-path m3u-file)
+ (with-temp-buffer
+ (insert-file-contents m3u-file)
+ ;; playlists/ -> c -> b -> a -> base is four hops up.
+ (should (string= (buffer-string) "../../../../faraway/song.mp3\n")))
+ (should (equal (cj/music--m3u-file-tracks m3u-file)
+ (list track-path))))
+ (test-music-config--append-track-to-m3u-file-teardown)))
+
;;; Boundary Cases
(ert-deftest test-music-config--append-track-to-m3u-file-boundary-very-long-path-appends-successfully ()
"Append very long track path without truncation."
(test-music-config--append-track-to-m3u-file-setup)
(unwind-protect
- (let* ((m3u-file (cj/create-temp-test-file "test-playlist-"))
+ (let* ((cj/music-root (cj/create-test-base-dir))
+ (m3u-file (cj/create-temp-test-file "test-playlist-"))
;; Create a relative path that's ~450 chars long
(relative-path (concat (make-string 440 ?a) "/song.mp3"))
(track-path (expand-file-name relative-path cj/music-root)))
@@ -119,7 +222,8 @@
"Append path with unicode characters preserves UTF-8 encoding."
(test-music-config--append-track-to-m3u-file-setup)
(unwind-protect
- (let* ((m3u-file (cj/create-temp-test-file "test-playlist-"))
+ (let* ((cj/music-root (cj/create-test-base-dir))
+ (m3u-file (cj/create-temp-test-file "test-playlist-"))
(relative-path "中文/artist-名前/song🎵.mp3")
(track-path (expand-file-name relative-path cj/music-root)))
(cj/music--append-track-to-m3u-file track-path m3u-file)
@@ -132,7 +236,8 @@
"Append path with spaces and special characters."
(test-music-config--append-track-to-m3u-file-setup)
(unwind-protect
- (let* ((m3u-file (cj/create-temp-test-file "test-playlist-"))
+ (let* ((cj/music-root (cj/create-test-base-dir))
+ (m3u-file (cj/create-temp-test-file "test-playlist-"))
(relative-path "Artist Name/Album (2024)/01 - Song's Title [Remix].mp3")
(track-path (expand-file-name relative-path cj/music-root)))
(cj/music--append-track-to-m3u-file track-path m3u-file)
@@ -146,6 +251,7 @@
(test-music-config--append-track-to-m3u-file-setup)
(unwind-protect
(let* ((existing-content "#EXTM3U\n#EXTINF:-1,Radio Station\nhttp://stream.url/radio\n")
+ (cj/music-root (cj/create-test-base-dir))
(m3u-file (cj/create-temp-test-file-with-content existing-content "test-playlist-"))
(relative-path "local-track.mp3")
(track-path (expand-file-name relative-path cj/music-root)))
@@ -172,6 +278,9 @@
"Signal error when M3U file is read-only."
(test-music-config--append-track-to-m3u-file-setup)
(unwind-protect
+ ;; No `cj/music-root' rebinding here: the writable-p guard signals before
+ ;; any path computation runs, so binding it would imply a dependency the
+ ;; read-only path does not have.
(let* ((m3u-file (cj/create-temp-test-file "test-playlist-"))
(track-path "/home/user/music/song.mp3"))
;; Make file read-only