From a457ade9207be48f6bfaa519046d7acea390ff1b Mon Sep 17 00:00:00 2001 From: Craig Jennings Date: Mon, 3 Aug 2026 14:38:20 -0500 Subject: feat(music): write an m3u path absolute when it leaves the playlist's directory A track under the playlist's own directory still writes relative. Anything outside it now writes the absolute path instead of a ../ chain. Playlists in the mpd directory point into ~/music, so the relative form there was four levels of .. and broke the moment anything moved. I took the trade knowingly. ambience.m3u is the only playlist anywhere using ../ lines, and it reaches its audio through a sibling directory. So a newly appended track there writes absolute while its hand-written lines stay relative, and its header comment needs a matching edit. The alternative was an exception for tracks inside the deployed tree, which would be a rule serving exactly one file. The comment I wrote for the earlier half of this argued one direction only. Absolute survives the playlist moving, which I said. Relative survives the library moving, which I didn't, and for a track in the same tree the ../ form survives strictly more moves rather than fewer. Rewritten to name the trade instead of implying there isn't one. The boundary case is a directory whose name begins with two dots. The check looks for a leading "../", so a real subdirectory called ..hidden sits under the playlist and must stay relative. It has a test now, because loosening that check to ".." would break that case and nothing else. --- modules/music-config.el | 29 +++++--- .../test-music-config--append-track-to-m3u-file.el | 77 +++++++++++++++++----- 2 files changed, 80 insertions(+), 26 deletions(-) diff --git a/modules/music-config.el b/modules/music-config.el index 075ed49b..0834aae8 100644 --- a/modules/music-config.el +++ b/modules/music-config.el @@ -677,16 +677,25 @@ 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)) - ;; 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 - (file-name-directory m3u-file)) - track-path))) + ;; Relative when the track sits under the playlist's own directory, absolute + ;; otherwise. + ;; + ;; The base is the playlist rather than `cj/music-root' because that is what + ;; both readers resolve against -- `cj/music--m3u-file-tracks' and EMMS's + ;; `emms-source-playlist-parse-m3u'. Inside the music root the two are the + ;; same directory, which is why basing on the root went unnoticed: it only + ;; wrote an unresolvable line once a playlist lived somewhere else. + ;; + ;; Falling back to absolute keeps a cross-tree reference readable, and it + ;; survives the playlist being moved again. A playlist in the mpd directory + ;; pointing into ~/music would otherwise carry a four-level ../ chain that + ;; breaks the moment anything moves. + (let* ((dir (file-name-directory m3u-file)) + (relative-path + (if (not (file-name-absolute-p track-path)) + track-path + (let ((rel (file-relative-name track-path dir))) + (if (string-prefix-p "../" rel) track-path rel))))) ;; Determine if we need a leading newline (let ((needs-prefix-newline nil) (file-size (file-attribute-size (file-attributes m3u-file)))) 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 06327b9d..9e5d0578 100644 --- a/tests/test-music-config--append-track-to-m3u-file.el +++ b/tests/test-music-config--append-track-to-m3u-file.el @@ -149,15 +149,35 @@ why the defect stayed invisible until a playlist moved out of it." (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." +(ert-deftest test-music-config--append-track-to-m3u-file-normal-under-playlist-dir-is-relative () + "Normal: a track under the playlist's directory is written relative to it. +The music root sits at a different depth on purpose. Put it alongside the +playlist directory instead and both candidate bases produce the same string, +so the assertion would hold against a writer using either one." + (test-music-config--append-track-to-m3u-file-setup) + (unwind-protect + (let* ((base (cj/create-test-base-dir)) + (playlists (expand-file-name "mpd/playlists/" base)) + (cj/music-root (expand-file-name "music/" base)) + (m3u-file (expand-file-name "album.m3u" playlists)) + (track-path (expand-file-name "sub/song.mp3" playlists))) + (make-directory (expand-file-name "sub/" playlists) 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) "sub/song.mp3\n"))) + (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-sibling-dir-is-absolute () + "Normal: a track outside the playlist's directory is written absolute. +A sibling would otherwise come out as \"../audio/x.mp3\". Absolute is the +convention for cross-tree references here, and it survives the playlist being +moved again later, which a ../ chain does not." (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)) @@ -171,15 +191,15 @@ quietly make both sides agree on the wrong base." (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")))) + (should (string= (buffer-string) (concat track-path "\n")))) + (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-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." +(ert-deftest test-music-config--append-track-to-m3u-file-normal-deep-parent-chain-goes-absolute () + "Normal: a track several levels away is written absolute, not as a ../ chain. +This is the case the absolute fallback exists for. A four-level chain is +unreadable and breaks the moment the playlist moves, so distance from the +playlist is exactly when an absolute path earns its keep." (test-music-config--append-track-to-m3u-file-setup) (unwind-protect (let* ((base (cj/create-test-base-dir)) @@ -194,14 +214,39 @@ nothing else here would catch it." (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"))) + ;; Four hops up (playlists -> c -> b -> a -> base) would be the + ;; relative form; the writer declines it and emits the absolute path. + (should (string= (buffer-string) (concat track-path "\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-dotdot-named-dir-stays-relative () + "Boundary: a directory whose name merely begins with two dots stays relative. +This is the input the relative-vs-absolute test actually turns on. The check +looks for a leading \"../\", so a real subdirectory named \"..hidden\" is under +the playlist and must not be mistaken for an escape. Loosening the check to +\"..\" would break exactly this case and nothing else in the suite 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 "mpd/playlists/" base)) + (cj/music-root (expand-file-name "music/" base)) + (m3u-file (expand-file-name "p.m3u" playlists)) + (track-path (expand-file-name "..hidden/song.mp3" playlists))) + (make-directory (expand-file-name "..hidden/" playlists) 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) "..hidden/song.mp3\n"))) + (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-boundary-very-long-path-appends-successfully () "Append very long track path without truncation." (test-music-config--append-track-to-m3u-file-setup) -- cgit v1.2.3