Wire up disc_prefix, which was parsed and never read
The option was accepted from the config file and stored, but nothing ever consulted it: TrackFile always emitted the disc number, so disc_prefix = "never" silently had no effect. The mode is now resolved in the plan and passed through to the name builder. Three values are supported. "always" keeps the current behaviour and stays the default. "never" drops the disc number. "auto" keeps it only when the album has more than one disc, where it is the only thing separating two tracks that share a number. Dropping the prefix across a multi-disc album leaves two files both numbered 01, so that now warns rather than happening quietly. An unrecognised value is rejected when the config is loaded instead of being stored and ignored, which is the same silence that made the option useless to begin with. Closes 94e1e25178234b6210a5113bb8b4a323c61995ea
diff --git a/internal/cli/cli.go b/internal/cli/cli.go
@@ -187,7 +187,7 @@ func (a *App) process(ctx context.Context, al scan.Album) error {
return fmt.Errorf("%s: %s (%s)", al.Name, al.Status, strings.Join(al.Notes, "; "))
}
- names0 := plan.Build(al, a.Cfg.Archive(), a.Cfg.Library(), plan.DirKey{})
+ names0 := plan.Build(al, a.Cfg.Archive(), a.Cfg.Library(), plan.DirKey{}, a.Cfg.DiscPrefix)
var resolved *plan.Plan
var editSession *names.Session
needsNames := nameIssues(names0) || strings.TrimSpace(names0.AlbumArt) == ""
@@ -240,7 +240,7 @@ func (a *App) process(ctx context.Context, al scan.Album) error {
if strings.TrimSpace(chosenCredit) != "" {
key.Credit = chosenCredit
}
- p := plan.BuildWithArtist(al, a.Cfg.Archive(), a.Cfg.Library(), key, resolved, editSession)
+ p := plan.BuildWithArtist(al, a.Cfg.Archive(), a.Cfg.Library(), key, a.Cfg.DiscPrefix, resolved, editSession)
cover := coverChoice{}
if !a.Cfg.NoCover {
diff --git a/internal/cli/file.go b/internal/cli/file.go
@@ -104,7 +104,7 @@ func (a *App) PrepareAlbum(ctx context.Context, dir string) (plan.Plan, error) {
return plan.Plan{}, fmt.Errorf("%s: %s (%s)", al.Name, al.Status, strings.Join(al.Notes, "; "))
}
- names0 := plan.Build(al, a.Cfg.Archive(), a.Cfg.Library(), plan.DirKey{})
+ names0 := plan.Build(al, a.Cfg.Archive(), a.Cfg.Library(), plan.DirKey{}, a.Cfg.DiscPrefix)
credit := names0.CreditFromTags
if strings.TrimSpace(credit) == "" {
credit = names0.AlbumArt
@@ -116,7 +116,7 @@ func (a *App) PrepareAlbum(ctx context.Context, dir string) (plan.Plan, error) {
if strings.TrimSpace(chosenCredit) != "" {
key.Credit = chosenCredit
}
- return plan.BuildWithArtist(al, a.Cfg.Archive(), a.Cfg.Library(), key, nil, nil), nil
+ return plan.BuildWithArtist(al, a.Cfg.Archive(), a.Cfg.Library(), key, a.Cfg.DiscPrefix, nil, nil), nil
}
func (a *App) FileAlbum(ctx context.Context, dir string, logf func(string, ...any)) (FileResult, error) {
diff --git a/internal/config/config.go b/internal/config/config.go
@@ -145,7 +145,13 @@ func (c *Config) set(key, value string) error {
}
c.Quality = n
case "disc_prefix":
- c.DiscPrefix = value
+ v := strings.ToLower(strings.TrimSpace(value))
+ switch v {
+ case "always", "never", "auto":
+ c.DiscPrefix = v
+ default:
+ return fmt.Errorf("disc_prefix: %q is not one of always, never or auto", value)
+ }
case "library_art_px":
n, err := strconv.Atoi(value)
if err != nil {
diff --git a/internal/config/config_test.go b/internal/config/config_test.go
@@ -0,0 +1,33 @@
+package config
+
+import (
+ "os"
+ "path/filepath"
+ "testing"
+)
+
+func write(t *testing.T, body string) string {
+ t.Helper()
+ dir := t.TempDir()
+ p := filepath.Join(dir, "config.toml")
+ if err := os.WriteFile(p, []byte(body), 0o600); err != nil {
+ t.Fatal(err)
+ }
+ return p
+}
+
+// disc_prefix used to be stored verbatim and never read, so a typo or an
+// invented value was accepted and then silently did nothing.
+func TestDiscPrefixRejectsUnknownValues(t *testing.T) {
+ // Load reads the real config path too, so the values are set by file here.
+ for _, good := range []string{"always", "never", "auto", "AUTO", " auto "} {
+ if _, err := Load(write(t, "disc_prefix = \""+good+"\"")); err != nil {
+ t.Errorf("disc_prefix = %q should be accepted: %v", good, err)
+ }
+ }
+ for _, bad := range []string{"sometimes", "", "true"} {
+ if _, err := Load(write(t, "disc_prefix = \""+bad+"\"")); err == nil {
+ t.Errorf("disc_prefix = %q should be rejected", bad)
+ }
+ }
+}
diff --git a/internal/normalize/normalize.go b/internal/normalize/normalize.go
@@ -81,10 +81,16 @@ func embeddedInWord(runes []rune, i int) bool {
return !unicode.IsSpace(runes[i-1]) && !unicode.IsSpace(runes[i+1])
}
-func TrackFile(disc, track, width int, title, ext string) string {
+// TrackFile builds a target file name. withDisc is false when the disc number
+// should be left out, which only makes sense for a single-disc album: two discs
+// sharing a numbering scheme produce two files both called 01_something.
+func TrackFile(disc, track, width int, title, ext string, withDisc bool) string {
if ext != "" && !strings.HasPrefix(ext, ".") {
ext = "." + ext
}
+ if !withDisc {
+ return fmt.Sprintf("%0*d_%s%s", width, track, Name(title), ext)
+ }
return fmt.Sprintf("%d-%0*d_%s%s", disc, width, track, Name(title), ext)
}
diff --git a/internal/normalize/normalize_test.go b/internal/normalize/normalize_test.go
@@ -63,13 +63,13 @@ func TestPadWidth(t *testing.T) {
}
func TestTrackFile(t *testing.T) {
- if got := TrackFile(1, 1, 2, "The Method to Love", ".flac"); got != "1-01_the-method-to-love.flac" {
+ if got := TrackFile(1, 1, 2, "The Method to Love", ".flac", true); got != "1-01_the-method-to-love.flac" {
t.Errorf("2-digit: got %q", got)
}
- if got := TrackFile(1, 7, 3, "Long Player Track 7", ".flac"); got != "1-007_long-player-track-7.flac" {
+ if got := TrackFile(1, 7, 3, "Long Player Track 7", ".flac", true); got != "1-007_long-player-track-7.flac" {
t.Errorf("3-digit: got %q", got)
}
- if got := TrackFile(2, 10, 2, "X", "flac"); got != "2-10_x.flac" {
+ if got := TrackFile(2, 10, 2, "X", "flac", true); got != "2-10_x.flac" {
t.Errorf("ext: got %q", got)
}
}
diff --git a/internal/plan/plan.go b/internal/plan/plan.go
@@ -82,7 +82,7 @@ type DirKey struct {
Source string
}
-func Build(a scan.Album, archiveRoot, libraryRoot string, artistKey DirKey) Plan {
+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 {
p.Issues = append(p.Issues, Issue{Hard: true, Text: "no readable audio tracks"})
@@ -238,10 +238,14 @@ func Build(a scan.Album, archiveRoot, libraryRoot string, artistKey DirKey) Plan
tracksOnDisc[it.disc]++
}
discTotal := len(tracksOnDisc)
+ withDisc := useDiscPrefix(discPrefix, discTotal)
+ if !withDisc && discTotal > 1 {
+ p.Warnings = append(p.Warnings, fmt.Sprintf("disc_prefix is %q but this album has %d discs, so tracks on different discs will share numbers", discPrefix, discTotal))
+ }
var pairs [][2]string
for _, it := range items {
- target := normalize.TrackFile(it.disc, it.track, width, it.title, filepath.Ext(it.src))
+ target := normalize.TrackFile(it.disc, it.track, width, it.title, filepath.Ext(it.src), withDisc)
pairs = append(pairs, [2]string{it.orig, target})
}
for _, c := range normalize.DetectCollisions(pairs) {
@@ -250,7 +254,7 @@ func Build(a scan.Album, archiveRoot, libraryRoot string, artistKey DirKey) Plan
}
for _, it := range items {
- name := normalize.TrackFile(it.disc, it.track, width, it.title, filepath.Ext(it.src))
+ 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 {
p.Warnings = append(p.Warnings, fmt.Sprintf("target name was too long and has been shortened: %s -> %s", name, n))
@@ -472,8 +476,8 @@ func (p *Plan) reassessIssues(albumEdited, artistEdited bool) {
p.Issues = removeIssues(p.Issues, "normalise to the same name")
}
-func BuildWithArtist(a scan.Album, archiveRoot, libraryRoot string, key DirKey, edited *Plan, session *names.Session) Plan {
- p := Build(a, archiveRoot, libraryRoot, key)
+func BuildWithArtist(a scan.Album, archiveRoot, libraryRoot string, key DirKey, discPrefix string, edited *Plan, session *names.Session) Plan {
+ p := Build(a, archiveRoot, libraryRoot, key, discPrefix)
if edited == nil {
return p
}
@@ -659,3 +663,17 @@ func differingArtists(a scan.Album) bool {
}
return false
}
+
+// useDiscPrefix decides whether target names carry the disc number. "auto" only
+// carries it when there is more than one disc, where it is the only thing
+// separating two tracks that share a number.
+func useDiscPrefix(mode string, discs int) bool {
+ switch strings.ToLower(strings.TrimSpace(mode)) {
+ case "never":
+ return false
+ case "auto":
+ return discs > 1
+ default:
+ return true
+ }
+}
diff --git a/internal/plan/plan_test.go b/internal/plan/plan_test.go
@@ -1,6 +1,7 @@
package plan
import (
+ "strings"
"testing"
"intake/internal/audio"
@@ -32,13 +33,15 @@ func itoa(n int) string {
return string(b)
}
-func planFor(tracks []audio.Track) Plan {
+func planFor(tracks []audio.Track) Plan { return planForMode(tracks, "always") }
+
+func planForMode(tracks []audio.Track, mode string) Plan {
return Build(scan.Album{
Dir: "/staging/an-album",
Name: "an-album",
Status: scan.StatusReady,
Tracks: tracks,
- }, "/archive", "/library", DirKey{})
+ }, "/archive", "/library", DirKey{}, mode)
}
func value(t *testing.T, p Plan, i int, field string) string {
@@ -130,6 +133,16 @@ func TestTotalsAreNotReportedAsDropped(t *testing.T) {
// credit makes a track with a given per-track artist, and optionally no
// ALBUMARTIST at all, which is the shape most of these cases care about.
+// TrackDisc returns a copy of a track placed on another disc.
+func TrackDisc(t audio.Track, disc int) audio.Track {
+ for i := range t.Tags {
+ if t.Tags[i].Field == tags.FieldDiscNumber {
+ t.Tags[i].Value = itoa(disc)
+ }
+ }
+ return t
+}
+
func credit(name string, number int, artist string, albumArtist string) audio.Track {
notes := []tags.Note{
{Field: tags.FieldTitle, Value: name},
@@ -226,3 +239,59 @@ func TestVariantAlbumArtistSpellingResolves(t *testing.T) {
t.Errorf("plan is blocked: %v", p.Issues)
}
}
+
+// disc_prefix used to be parsed and then never read, so every album got a 1-
+// prefix whether or not it had more than one disc.
+func TestDiscPrefixModes(t *testing.T) {
+ single := []audio.Track{
+ credit("a", 1, "An Artist", "An Artist"),
+ credit("b", 2, "An Artist", "An Artist"),
+ }
+ multi := []audio.Track{
+ credit("a", 1, "An Artist", "An Artist"),
+ TrackDisc(credit("b", 1, "An Artist", "An Artist"), 2),
+ }
+
+ for _, c := range []struct {
+ mode string
+ tracks []audio.Track
+ wantName string
+ }{
+ {"always", single, "1-01_a.flac"},
+ {"never", single, "01_a.flac"},
+ {"auto", single, "01_a.flac"},
+ {"always", multi, "1-01_a.flac"},
+ {"never", multi, "01_a.flac"},
+ {"auto", multi, "1-01_a.flac"},
+ } {
+ p := planForMode(c.tracks, c.mode)
+ if got := p.Tracks[0].Target; got != c.wantName {
+ t.Errorf("mode %q: first target = %q, want %q", c.mode, got, c.wantName)
+ }
+ }
+}
+
+// Dropping the disc number across a multi-disc album leaves two tracks both
+// numbered 01, so it is worth saying so rather than doing it quietly.
+func TestDiscPrefixNeverWarnsOnMultiDisc(t *testing.T) {
+ multi := []audio.Track{
+ credit("a", 1, "An Artist", "An Artist"),
+ TrackDisc(credit("b", 1, "An Artist", "An Artist"), 2),
+ }
+ p := planForMode(multi, "never")
+ if !hasWarning(p, "disc_prefix") {
+ t.Errorf("expected a warning about dropping the disc prefix, got %v", p.Warnings)
+ }
+ if p := planForMode(multi, "auto"); hasWarning(p, "disc_prefix") {
+ t.Error("auto keeps the prefix on a multi-disc album and should not warn")
+ }
+}
+
+func hasWarning(p Plan, substr string) bool {
+ for _, w := range p.Warnings {
+ if strings.Contains(w, substr) {
+ return true
+ }
+ }
+ return false
+}
diff --git a/internal/tui/tui_test.go b/internal/tui/tui_test.go
@@ -62,7 +62,7 @@ func readyPlan() plan.Plan {
{Field: tags.FieldDiscNumber, Value: "1"},
{Field: tags.FieldDate, Value: "1998"},
}}},
- }, "/archive", "/library", plan.DirKey{})
+ }, "/archive", "/library", plan.DirKey{}, "always")
}
func TestBrowseListsSortedAlbums(t *testing.T) {
@@ -170,7 +170,7 @@ func TestBlockedPlanCannotLeaveMetadata(t *testing.T) {
Tracks: []audio.Track{{Path: "/s/01.flac", Ext: ".flac", Tags: []tags.Note{
{Field: tags.FieldTitle, Value: "Song"},
}}},
- }, "/archive", "/library", plan.DirKey{})
+ }, "/archive", "/library", plan.DirKey{}, "always")
if !blocked.Blocked() {
t.Fatal("fixture should be blocked")
}