aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorCraig Jennings <c@cjennings.net>2026-08-03 15:27:17 -0500
committerCraig Jennings <c@cjennings.net>2026-08-03 15:27:17 -0500
commit6f90ec5b660c6a2552ac7284a3d9d963771fbbc2 (patch)
tree90bcdaafd27fabdd7dfcee7484b8646e2bc6923b
parenta457ade9207be48f6bfaa519046d7acea390ff1b (diff)
downloaddotemacs-6f90ec5b660c6a2552ac7284a3d9d963771fbbc2.tar.gz
dotemacs-6f90ec5b660c6a2552ac7284a3d9d963771fbbc2.zip
fix(music): size a playlist by its content, not by its symlink
cj/music--append-track-to-m3u-file decided whether to prepend a newline by seeking to a byte offset from file-attributes, which doesn't follow symlinks. On a stow-deployed playlist that measures the link string instead of the file. Two failure modes, and I only went looking for the second. All 100 symlinked playlists read the wrong byte, so a file already ending in a newline looked unterminated and gained a blank line on every append. On the 31 whose link string is longer than their content the range fell outside the file entirely. Nothing was inserted, and char-after handed nil to a numeric comparison, so the append died with a wrong-type error. The probe now reads the file and checks its last character. That takes file-attributes out of the path, so this class can't come back. The largest real playlist is 11 KB and the read costs 0.11 ms, so being clever about offsets bought nothing. A test per mode, because one fixture can't show both. Seed the blank-line case with content shorter than the link string and it lands on the error path instead, going red for the wrong reason and proving nothing. The cheap patch was guarding char-after against nil. That would have silenced the crash on 31 playlists and left the blank line live on all 100.
-rw-r--r--modules/music-config.el22
-rw-r--r--tests/test-music-config--append-track-to-m3u-file.el67
2 files changed, 81 insertions, 8 deletions
diff --git a/modules/music-config.el b/modules/music-config.el
index 0834aae8..47863e41 100644
--- a/modules/music-config.el
+++ b/modules/music-config.el
@@ -696,14 +696,20 @@ M3U-FILE should be an existing, writable M3U file 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))))
- (when (> file-size 0)
- ;; Read the last character of the file to check if it ends with newline
- (with-temp-buffer
- (insert-file-contents m3u-file nil (max 0 (1- file-size)) file-size)
- (setq needs-prefix-newline (not (= (char-after (point-min)) ?\n)))))
+ ;; Does the file need a separating newline first? Read the content and look
+ ;; at its last character, rather than seeking to a byte offset derived from
+ ;; `file-attributes'. That call does not follow symlinks, so on a
+ ;; stow-deployed playlist it measures the link string instead of the file:
+ ;; every symlinked playlist read the wrong byte and gained a blank line per
+ ;; append, and where the link string was the longer of the two the range fell
+ ;; outside the file entirely and the append died on a nil `char-after'.
+ ;; Playlists are small text files, so reading one is cheaper than being
+ ;; clever about offsets.
+ (let ((needs-prefix-newline
+ (with-temp-buffer
+ (insert-file-contents m3u-file)
+ (and (> (buffer-size) 0)
+ (/= (char-before (point-max)) ?\n)))))
;; Append the track with proper newline handling
(with-temp-buffer
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 9e5d0578..cc40438c 100644
--- a/tests/test-music-config--append-track-to-m3u-file.el
+++ b/tests/test-music-config--append-track-to-m3u-file.el
@@ -307,6 +307,73 @@ it."
(concat existing-content relative-path "\n")))))
(test-music-config--append-track-to-m3u-file-teardown)))
+;;; Boundary Cases: symlinked playlists
+
+(defun test-music-config--append--make-symlinked-playlist (base content link-depth)
+ "Create a playlist whose deployed path is a symlink, and return that path.
+CONTENT is written to the real file. LINK-DEPTH controls how long the link
+string is, which is the whole point: `file-attributes' does not follow
+symlinks, so a writer sizing the file that way reads the length of the link
+rather than the content."
+ (let* ((deployed (expand-file-name "deployed/" base))
+ (deep (expand-file-name (mapconcat #'identity
+ (make-list link-depth "longdirname")
+ "/")
+ base))
+ (real (expand-file-name "p.m3u" deep))
+ (link (expand-file-name "p.m3u" deployed)))
+ (make-directory deep t)
+ (make-directory deployed t)
+ (with-temp-buffer (insert content) (write-file real))
+ (make-symbolic-link (file-relative-name real deployed) link t)
+ link))
+
+(ert-deftest test-music-config--append-track-to-m3u-file-boundary-symlink-longer-than-content ()
+ "Boundary: appending to a symlinked playlist whose link string is longer than
+its content must not signal. Sizing the file with `file-attributes' returns
+the link's length, so the read range falls outside the file, nothing is
+inserted, and `char-after' hands nil to a numeric comparison. Measured on the
+real deployed set: 31 of 100 symlinked playlists are in this state."
+ (test-music-config--append-track-to-m3u-file-setup)
+ (unwind-protect
+ (let* ((base (cj/create-test-base-dir))
+ (m3u-file (test-music-config--append--make-symlinked-playlist
+ base "https://example.com/s.mp3\n" 8))
+ (track-path (expand-file-name "song.mp3" (file-name-directory m3u-file))))
+ (should (> (file-attribute-size (file-attributes m3u-file))
+ (file-attribute-size (file-attributes (file-truename m3u-file)))))
+ (cj/music--append-track-to-m3u-file track-path m3u-file)
+ ;; The seeded line is a stream URL, which the reader passes through, so
+ ;; both entries come back.
+ (should (equal (cj/music--m3u-file-tracks m3u-file)
+ (list "https://example.com/s.mp3" track-path))))
+ (test-music-config--append-track-to-m3u-file-teardown)))
+
+(ert-deftest test-music-config--append-track-to-m3u-file-boundary-symlink-no-spurious-blank-line ()
+ "Boundary: a symlinked playlist already ending in a newline gains no blank line.
+The trailing-newline probe reads a byte chosen from the wrong size, so it
+misreads a terminated file as unterminated and prepends a newline. All 100
+symlinked playlists in the deployed set read the wrong byte this way."
+ (test-music-config--append-track-to-m3u-file-setup)
+ (unwind-protect
+ ;; Content deliberately longer than the link string, so the misread byte
+ ;; still lands inside the file. That separates this from the sibling test
+ ;; above: here the probe reads a valid but wrong byte and silently
+ ;; misjudges, rather than reading past the end and signalling.
+ (let* ((base (cj/create-test-base-dir))
+ (content (mapconcat (lambda (i) (format "track-%03d-with-a-longish-name.mp3" i))
+ (number-sequence 1 12) "\n"))
+ (m3u-file (test-music-config--append--make-symlinked-playlist
+ base (concat content "\n") 2))
+ (track-path (expand-file-name "second.mp3" (file-name-directory m3u-file))))
+ (should (< (file-attribute-size (file-attributes m3u-file))
+ (file-attribute-size (file-attributes (file-truename 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) (concat content "\nsecond.mp3\n")))))
+ (test-music-config--append-track-to-m3u-file-teardown)))
+
;;; Error Cases
(ert-deftest test-music-config--append-track-to-m3u-file-error-nonexistent-file-signals-error ()