fix: survive over-long folder names, lossy sources, and a disputed album tag
Three ways a plausible album could not be filed. Album and artist folder names had no length limit. A long album name made the temporary directory exceed the filesystem's 255 byte component cap, so the album failed with "file name too long" after being fully prepared. They are shortened to the same 200 bytes track names already used, and the shortening is reported. An album containing anything that is not flac was accepted by the plan and then failed deep inside metaflac with "does not appear to be a FLAC file", after the files had been copied. The archive holds lossless masters written by flac tools, so a lossy source is now refused up front, in terms of what to do about it. Tracks that disagreed on ALBUM were treated as having no album name at all. One mistyped tag out of fifteen stopped the album. A clear majority is now used and the disagreement is reported. Also: a cue sheet beside a single audio file warns, because intake does not split on cue sheets and filing it silently would leave a wrong track list.
diff --git a/internal/plan/plan.go b/internal/plan/plan.go
@@ -82,6 +82,12 @@ type DirKey struct {
Source string
}
+// maxNameBytes is the longest name intake writes for a file or a directory.
+// Filesystems cap a single path component at 255 bytes, and the archive is built
+// inside a temporary directory whose name is the target's with a prefix and a
+// random suffix, so the target has to leave room for that.
+const maxNameBytes = 200
+
func Build(a scan.Album, archiveRoot, libraryRoot string, artistKey DirKey, discPrefix string) Plan {
p := Plan{Album: a, ArchiveRoot: archiveRoot, LibraryRoot: libraryRoot}
if len(a.Tracks) == 0 {
@@ -94,6 +100,22 @@ func Build(a scan.Album, archiveRoot, libraryRoot string, artistKey DirKey, disc
artist, artistOK := commonValue(a, tags.FieldArtist)
yearRaw, _ := commonValue(a, tags.FieldDate)
+ // Tracks that disagree on an album-level value are treated as not having
+ // agreed at all, but one mistagged track out of fifteen should not stop the
+ // album: a clear majority is used, and said out loud.
+ if !albumNameOK {
+ if v, n, total := dominantValue(a, tags.FieldAlbum); n*2 > total {
+ albumName, albumNameOK = v, true
+ p.Warnings = append(p.Warnings, fmt.Sprintf("tracks disagree on ALBUM; %d of %d say %q, using that", n, total, v))
+ }
+ }
+ if !albumArtistOK {
+ if v, n, total := dominantValue(a, tags.FieldAlbumArtist); n*2 > total {
+ albumArtist, albumArtistOK = v, true
+ p.Warnings = append(p.Warnings, fmt.Sprintf("tracks disagree on ALBUMARTIST; %d of %d say %q, using that", n, total, v))
+ }
+ }
+
if !albumArtistOK {
switch {
case artistOK:
@@ -132,6 +154,35 @@ func Build(a scan.Album, archiveRoot, libraryRoot string, artistKey DirKey, disc
p.Issues = append(p.Issues, Issue{Hard: true, Text: "album name is unknown: no ALBUM tag and the directory name gives no hint"})
}
+ // The archive holds lossless masters: every file intake writes is FLAC, and
+ // the tagging and artwork tools it drives are FLAC tools. A lossy source is a
+ // design decision rather than a half-working fallback, so it is refused here
+ // with something the user can act on, instead of failing much later inside
+ // metaflac with "does not appear to be a FLAC file".
+ notFLAC := map[string]bool{}
+ for _, t := range a.Tracks {
+ if ext := strings.ToLower(filepath.Ext(t.Path)); ext != ".flac" {
+ notFLAC[strings.TrimPrefix(ext, ".")] = true
+ }
+ }
+ if len(notFLAC) > 0 {
+ kinds := make([]string, 0, len(notFLAC))
+ for k := range notFLAC {
+ kinds = append(kinds, k)
+ }
+ sort.Strings(kinds)
+ p.Issues = append(p.Issues, Issue{Hard: true, Text: fmt.Sprintf("the archive holds flac only, and this album contains %s; convert it to flac first, or file it by hand", strings.Join(kinds, ", "))})
+ }
+
+ // One audio file beside a cue sheet is usually a whole album the cue
+ // describes as many tracks. intake does not split on cue sheets, so filing it
+ // silently would leave the user with an album whose track list is wrong.
+ if len(a.Tracks) == 1 && a.Dir != "" {
+ if cues, _ := filepath.Glob(filepath.Join(a.Dir, "*.cue")); len(cues) > 0 {
+ p.Warnings = append(p.Warnings, fmt.Sprintf("a cue sheet is present (%s) and there is only one audio file; intake does not split on cue sheets, so this will be filed as a single track", filepath.Base(cues[0])))
+ }
+ }
+
p.AlbumName = albumName
p.AlbumTag = albumName
p.AlbumArt = albumArtist
@@ -187,6 +238,12 @@ func Build(a scan.Album, archiveRoot, libraryRoot string, artistKey DirKey, disc
if disc == 0 && fDisc > 0 {
disc = fDisc
}
+ if disc == 0 && t.DiscHint > 0 {
+ // Last resort, and only when the tags and the file name both said
+ // nothing: the folder the file sits in, for rips arranged as CD1,
+ // CD2 with no disc tags at all.
+ disc = t.DiscHint
+ }
if disc == 0 {
disc = 1
}
@@ -256,7 +313,7 @@ func Build(a scan.Album, archiveRoot, libraryRoot string, artistKey DirKey, disc
for _, it := range items {
name := normalize.TrackFile(it.disc, it.track, width, it.title, filepath.Ext(it.src), withDisc)
truncated := false
- if n, cut := normalize.Truncate(name, 200); cut {
+ if n, cut := normalize.Truncate(name, maxNameBytes); cut {
p.Warnings = append(p.Warnings, fmt.Sprintf("target name was too long and has been shortened: %s -> %s", name, n))
name, truncated = n, true
}
@@ -295,6 +352,10 @@ func Build(a scan.Album, archiveRoot, libraryRoot string, artistKey DirKey, disc
if albumDir == "" {
albumDir = normalize.Name(p.AlbumName)
}
+ if short, cut := normalize.Truncate(albumDir, maxNameBytes); cut {
+ p.Warnings = append(p.Warnings, fmt.Sprintf("album folder name was too long and has been shortened: %s -> %s", albumDir, short))
+ albumDir = short
+ }
if albumDir == "" {
p.Issues = append(p.Issues, Issue{Hard: true, Text: fmt.Sprintf("album name %q has no ASCII folder name; open the editor to supply one", p.AlbumName)})
}
@@ -303,6 +364,10 @@ func Build(a scan.Album, archiveRoot, libraryRoot string, artistKey DirKey, disc
if artistDir == "" {
artistDir = normalize.DirName(p.AlbumArt)
}
+ if short, cut := normalize.Truncate(artistDir, maxNameBytes); cut {
+ p.Warnings = append(p.Warnings, fmt.Sprintf("artist folder name was too long and has been shortened: %s -> %s", artistDir, short))
+ artistDir = short
+ }
p.ArtistDir = artistDir
if artistDir == "" {
p.Issues = append(p.Issues, Issue{Hard: true, Text: fmt.Sprintf("album artist %q has no ASCII folder name; open the editor to supply one", p.AlbumArt)})
@@ -624,10 +689,17 @@ func commonValue(a scan.Album, field string) (string, bool) {
// album with two features still has a clear majority. Splitting credits on a
// separator would mean guessing whether it is "+", "&", "feat." or "with".
func dominantArtist(a scan.Album) (string, int, int) {
+ return dominantValue(a, tags.FieldArtist)
+}
+
+// dominantValue returns the most common non-empty value of a field, how many
+// tracks carry it, and how many carry any value at all. Ties break
+// alphabetically so the result does not depend on map order.
+func dominantValue(a scan.Album, field string) (string, int, int) {
counts := map[string]int{}
total := 0
for _, t := range a.Tracks {
- v, ok := tags.Get(t.Tags, tags.FieldArtist)
+ v, ok := tags.Get(t.Tags, field)
v = strings.TrimSpace(v)
if !ok || v == "" {
continue
diff --git a/internal/plan/plan_test.go b/internal/plan/plan_test.go
@@ -1,6 +1,8 @@
package plan
import (
+ "os"
+ "path/filepath"
"strings"
"testing"
@@ -295,3 +297,132 @@ func hasWarning(p Plan, substr string) bool {
}
return false
}
+
+// Filesystems cap one path component at 255 bytes, and the archive is assembled
+// inside a temporary directory named after the target. An over-long album name
+// therefore failed the whole album with "file name too long" rather than being
+// shortened, which real rips hit often: scene names, deluxe editions and format
+// tags all push past it.
+func TestOverlongFolderNamesAreShortened(t *testing.T) {
+ long := strings.Repeat("Very Long Album Name ", 20)
+ tracks := []audio.Track{{
+ Path: "/staging/a.flac", Ext: ".flac",
+ Tags: []tags.Note{
+ {Field: tags.FieldTitle, Value: "A Song"},
+ {Field: tags.FieldAlbum, Value: long},
+ {Field: tags.FieldArtist, Value: strings.Repeat("Long Artist ", 30)},
+ {Field: tags.FieldAlbumArtist, Value: strings.Repeat("Long Artist ", 30)},
+ {Field: tags.FieldTrackNumber, Value: "1"},
+ {Field: tags.FieldDiscNumber, Value: "1"},
+ },
+ }}
+ p := Build(scan.Album{Dir: "/staging/x", Name: "x", Status: scan.StatusReady, Tracks: tracks},
+ "/archive", "/library", DirKey{}, "always")
+
+ if len(p.AlbumDir) > maxNameBytes {
+ t.Errorf("album folder is %d bytes, over the %d cap", len(p.AlbumDir), maxNameBytes)
+ }
+ if len(p.ArtistDir) > maxNameBytes {
+ t.Errorf("artist folder is %d bytes, over the %d cap", len(p.ArtistDir), maxNameBytes)
+ }
+ if !hasWarning(p, "shortened") {
+ t.Errorf("shortening happened without saying so: %v", p.Warnings)
+ }
+ if p.Blocked() {
+ t.Errorf("a long name should be shortened, not refused: %v", p.Issues)
+ }
+}
+
+// The archive holds flac only, and the tools that write it are flac tools. A
+// lossy source used to be accepted by the plan and then fail deep inside
+// metaflac with "does not appear to be a FLAC file", after the album had already
+// been copied into place.
+func TestNonFLACTracksAreRefusedEarly(t *testing.T) {
+ mp3 := credit("lossy", 2, "An Artist", "An Artist")
+ mp3.Path = "/staging/lossy.mp3"
+ mp3.Ext = ".mp3"
+
+ p := planFor([]audio.Track{credit("lossless", 1, "An Artist", "An Artist"), mp3})
+ if !p.Blocked() {
+ t.Fatal("an album containing an mp3 should be refused")
+ }
+ found := false
+ for _, i := range p.Issues {
+ if strings.Contains(i.Text, "flac only") {
+ found = true
+ }
+ }
+ if !found {
+ t.Errorf("the refusal should say what is wrong and what to do about it: %v", p.Issues)
+ }
+ // A flac-only album is unaffected.
+ if planFor([]audio.Track{credit("a", 1, "An Artist", "An Artist")}).Blocked() {
+ t.Error("a flac album should not be refused")
+ }
+}
+
+// A single file plus a cue sheet is how a lot of rips arrive, and the cue
+// usually describes an album's worth of tracks. intake does not split on cue
+// sheets, so the user has to be told rather than handed a one-track album.
+func TestCueSheetBesideASingleFileWarns(t *testing.T) {
+ dir := t.TempDir()
+ if err := os.WriteFile(filepath.Join(dir, "album.cue"), []byte("FILE \"whole.flac\" WAVE\n"), 0o600); err != nil {
+ t.Fatal(err)
+ }
+ one := []audio.Track{credit("whole", 1, "An Artist", "An Artist")}
+ p := Build(scan.Album{Dir: dir, Name: "x", Status: scan.StatusReady, Tracks: one},
+ "/archive", "/library", DirKey{}, "always")
+ if !hasWarning(p, "cue sheet") {
+ t.Errorf("a cue sheet beside one file should be reported: %v", p.Warnings)
+ }
+
+ // An album of separate files with a cue alongside is ordinary, and silent.
+ many := []audio.Track{
+ credit("a", 1, "An Artist", "An Artist"),
+ credit("b", 2, "An Artist", "An Artist"),
+ }
+ if p := Build(scan.Album{Dir: dir, Name: "x", Status: scan.StatusReady, Tracks: many},
+ "/archive", "/library", DirKey{}, "always"); hasWarning(p, "cue sheet") {
+ t.Error("a cue beside an already-split album should not warn")
+ }
+}
+
+// Tracks that disagree are treated as not having agreed, but one mistyped tag out
+// of many should not stop an album. A clear majority wins and says so.
+func TestDisputedAlbumValueFallsBackToTheMajority(t *testing.T) {
+ tracks := []audio.Track{
+ credit("a", 1, "An Artist", "An Artist"),
+ credit("b", 2, "An Artist", "An Artist"),
+ credit("c", 3, "An Artist", "An Artist"),
+ }
+ // mistype one, the way a bad rip does
+ for i := range tracks[1].Tags {
+ if tracks[1].Tags[i].Field == tags.FieldAlbum {
+ tracks[1].Tags[i].Value = "Rong Album"
+ }
+ }
+ p := planFor(tracks)
+ if p.AlbumName != "An Album" {
+ t.Errorf("album = %q, want the majority value", p.AlbumName)
+ }
+ if !hasWarning(p, "disagree") {
+ t.Errorf("using the majority should be reported: %v", p.Warnings)
+ }
+ if p.Blocked() {
+ t.Errorf("a single mistyped tag should not stop the album: %v", p.Issues)
+ }
+
+ // With no majority there is nothing to go on.
+ split := []audio.Track{
+ credit("a", 1, "An Artist", "An Artist"),
+ credit("b", 2, "An Artist", "An Artist"),
+ }
+ for i := range split[1].Tags {
+ if split[1].Tags[i].Field == tags.FieldAlbum {
+ split[1].Tags[i].Value = "Another Album"
+ }
+ }
+ if p := planFor(split); !p.Blocked() {
+ t.Error("a one-all split has no majority and should be refused")
+ }
+}