From 80d2fdc3aba91949d98d30dc97158b416558a316 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sat, 10 Oct 2026 17:29:42 -0400 Subject: [PATCH] fix: under the album's own name, matching track titles are enough to match it; a refusal says which track differed and how (M502 #5569) The first run on the library refused singles whose one track has the album's title ("0 of 1 agree") and near-complete albums (11 of 14), because every track's length had to fit as well. Lengths now only decide between groups whose names merely overlap. The reason for a refusal names the first track that differed, with both titles and lengths, so what is left can be judged. Migration 0087 clears the refusals so those albums are matched again. Co-Authored-By: Claude Opus 5.5 --- .../migrations/0087_library_rematch.down.sql | 2 + .../db/migrations/0087_library_rematch.up.sql | 9 ++ internal/wanted/library_test.go | 14 ++- internal/wanted/match.go | 88 +++++++++++++------ 4 files changed, 83 insertions(+), 30 deletions(-) create mode 100644 internal/db/migrations/0087_library_rematch.down.sql create mode 100644 internal/db/migrations/0087_library_rematch.up.sql diff --git a/internal/db/migrations/0087_library_rematch.down.sql b/internal/db/migrations/0087_library_rematch.down.sql new file mode 100644 index 00000000..66b5e525 --- /dev/null +++ b/internal/db/migrations/0087_library_rematch.down.sql @@ -0,0 +1,2 @@ +-- Nothing to undo: the refused albums are matched again either way. +SELECT 1; diff --git a/internal/db/migrations/0087_library_rematch.up.sql b/internal/db/migrations/0087_library_rematch.up.sql new file mode 100644 index 00000000..7347fff3 --- /dev/null +++ b/internal/db/migrations/0087_library_rematch.up.sql @@ -0,0 +1,9 @@ +-- The first matching (#5569) required every track's length to fit as well +-- as its title, and refused singles and near-complete albums the library +-- plainly holds. Matching now takes the titles alone under the album's own +-- name: the albums it refused are matched again. +DELETE FROM library_album_matches WHERE outcome = 'unmatched'; + +UPDATE acquisition_settings + SET library_listed_at = NULL, library_unlisted = 0 + WHERE library_listing_started_at IS NOT NULL; diff --git a/internal/wanted/library_test.go b/internal/wanted/library_test.go index eb865d1a..76410b7e 100644 --- a/internal/wanted/library_test.go +++ b/internal/wanted/library_test.go @@ -252,10 +252,22 @@ func TestTrackAgreement(t *testing.T) { {"the right title at the wrong length", []libraryTrack{{Number: 2, Title: "Song", LengthMs: 400000}}, 0}, {"a track counted once", []libraryTrack{{Title: "Song", LengthMs: 200000}, {Title: "Song", LengthMs: 200000}}, 1}, } { - if got := trackAgreement(c.tracks, edition); got != c.want { + if got, _ := trackAgreement(c.tracks, edition, false); got != c.want { t.Errorf("%s: %d agree, want %d", c.name, got, c.want) } } + // Under the album's own name the titles are enough; the reason a track + // did not agree names both sides. + edit := []libraryTrack{{Number: 2, Title: "Song (Radio Edit)", LengthMs: 150000}} + if n, miss := trackAgreement(edit, edition, false); n != 0 || miss != `"Song (Radio Edit)" runs 2:30 here; MusicBrainz's track 2 is "Song", 3:20` { + t.Errorf("by length: %d, %q", n, miss) + } + if n, _ := trackAgreement(edit, edition, true); n != 1 { + t.Errorf("by title alone: %d agree", n) + } + if _, miss := trackAgreement([]libraryTrack{{Title: "Other", LengthMs: 61000}}, edition, true); miss != `no track of its edition is like "Other" (1:01)` { + t.Errorf("unplaced: %q", miss) + } if agrees(3, 4) || !agrees(4, 5) || agrees(0, 0) { t.Error("agrees: 80% of the album's tracks, and an album has tracks") } diff --git a/internal/wanted/match.go b/internal/wanted/match.go index bcdf3ea8..04c7527e 100644 --- a/internal/wanted/match.go +++ b/internal/wanted/match.go @@ -101,10 +101,14 @@ func tracksToMatch(rows []dbq.ListAlbumTracksToMatchRow) []libraryTrack { } // trackAgreement counts the album's tracks an edition agrees with: the same -// recording, or a length that fits and a title that agrees. A track is -// paired by its disc and number first, then by title, so an album missing -// some tracks still pairs the ones it has. -func trackAgreement(tracks []libraryTrack, edition []editionTrack) int { +// recording, or a title that agrees at a length that fits. When the group is +// called just what the album is called, the titles alone are enough: the +// lengths are there to tell apart albums whose names only overlap, and a +// rip's single may be the edit MusicBrainz lists at another length. A track +// is paired by its disc and number first, then by title, so an album missing +// some tracks still pairs the ones it has. miss says how the first track +// that did not agree differs, for the reason a match was refused. +func trackAgreement(tracks []libraryTrack, edition []editionTrack, titlesEnough bool) (n int, miss string) { byPos := make(map[[2]int]int, len(edition)) for i, e := range edition { byPos[[2]int{max(e.Medium, 1), e.Position}] = i @@ -113,27 +117,47 @@ func trackAgreement(tracks []libraryTrack, edition []editionTrack) int { if t.Recording != "" && strings.EqualFold(t.Recording, e.Recording) { return true } - return release.LengthFits(t.LengthMs, e.LengthMs) && release.TitleAgrees(t.Title, e.Title) + return release.TitleAgrees(t.Title, e.Title) && (titlesEnough || release.LengthFits(t.LengthMs, e.LengthMs)) } used := make([]bool, len(edition)) - n := 0 for _, t := range tracks { + at, placed := -1, false if t.Number > 0 { - if i, ok := byPos[[2]int{max(t.Disc, 1), t.Number}]; ok && !used[i] && agree(t, edition[i]) { - used[i] = true - n++ - continue + if i, ok := byPos[[2]int{max(t.Disc, 1), t.Number}]; ok { + at = i + placed = !used[i] && agree(t, edition[i]) + if placed { + used[i] = true + } } } for i, e := range edition { - if !used[i] && agree(t, e) { - used[i] = true - n++ - break + if !placed && !used[i] && agree(t, e) { + used[i], placed = true, true } } + switch { + case placed: + n++ + case miss != "": + case at >= 0: + e := edition[at] + miss = fmt.Sprintf("%q runs %s here; MusicBrainz's track %d is %q, %s", + t.Title, minutes(t.LengthMs), e.Position, e.Title, minutes(e.LengthMs)) + default: + miss = fmt.Sprintf("no track of its edition is like %q (%s)", t.Title, minutes(t.LengthMs)) + } } - return n + return n, miss +} + +// minutes writes a length as m:ss, or "no length". +func minutes(ms int) string { + if ms <= 0 { + return "no length" + } + sec := (ms + 500) / 1000 + return fmt.Sprintf("%d:%02d", sec/60, sec%60) } // agrees: enough of the album's tracks agree for the edition to be it. @@ -173,14 +197,14 @@ func (s *Service) matchAlbum(ctx context.Context, a dbq.ListAlbumsToMatchRow, tr // discography first, then a search. tried := map[string]bool{} var best albumMatch - bestN, bestEdition, considered := -1, 0, 0 + bestN, bestEdition, considered, bestMiss := -1, 0, 0, "" check := func(cands []candidate) error { for _, c := range cands { if tried[c.Group] || len(tried) >= matchCandidates { continue } tried[c.Group] = true - n, size, err := s.agreementWith(ctx, c.Group, tracks) + n, size, miss, err := s.agreementWith(ctx, c.Group, tracks, sameTitle(c.Title, a.Title)) if err != nil { return err } @@ -189,7 +213,7 @@ func (s *Service) matchAlbum(ctx context.Context, a dbq.ListAlbumsToMatchRow, tr // one; then the edition nearest the album's length. st, bt := sameTitle(c.Title, a.Title), sameTitle(best.Title, a.Title) if n > bestN || (n == bestN && ((st && !bt) || (st == bt && abs(size-len(tracks)) < abs(bestEdition-len(tracks))))) { - best, bestN, bestEdition = albumMatch{candidate: c}, n, size + best, bestN, bestEdition, bestMiss = albumMatch{candidate: c}, n, size, miss } } return nil @@ -217,9 +241,15 @@ func (s *Service) matchAlbum(ctx context.Context, a dbq.ListAlbumsToMatchRow, tr if considered == 0 { return albumMatch{Detail: fmt.Sprintf("MusicBrainz has no album called %q by %s.", a.Title, a.ArtistName)}, nil } - return albumMatch{Detail: fmt.Sprintf( - "MusicBrainz has %d %s called %q by %s, but none has its tracks: the closest, %q, agrees on %d of its %d.", - considered, plural(considered, "album", "albums"), a.Title, a.ArtistName, best.Title, max(bestN, 0), len(tracks))}, nil + detail := fmt.Sprintf("MusicBrainz has %d %s called %q by %s, but none has its tracks: the closest, %q, agrees on %d of its %d", + considered, plural(considered, "album", "albums"), a.Title, a.ArtistName, best.Title, max(bestN, 0), len(tracks)) + switch { + case bestN < 0: + detail += " (MusicBrainz lists no edition of it)" + case bestMiss != "": + detail += ": " + bestMiss + } + return albumMatch{Detail: detail + "."}, nil } // groupCandidate reads a group the tags name. A group MusicBrainz does not @@ -296,15 +326,15 @@ func (s *Service) searchCandidates(ctx context.Context, a dbq.ListAlbumsToMatchR } // agreementWith is how many of the album's tracks the group's best edition -// agrees with, and that edition's length. A group MusicBrainz no longer has -// agrees with none. -func (s *Service) agreementWith(ctx context.Context, group string, tracks []libraryTrack) (n, size int, err error) { +// agrees with, that edition's length, and how it differs. A group +// MusicBrainz no longer has, or lists no edition of, agrees with none (-1). +func (s *Service) agreementWith(ctx context.Context, group string, tracks []libraryTrack, titlesEnough bool) (n, size int, miss string, err error) { eds, err := s.cat.Releases(ctx, group) switch { case errors.Is(err, catalogue.ErrNotFound): - return -1, 0, nil + return -1, 0, "", nil case err != nil: - return 0, 0, fmt.Errorf("%w: %v", errMusicBrainzAway, err) + return 0, 0, "", fmt.Errorf("%w: %v", errMusicBrainzAway, err) } n = -1 for _, e := range eds.Editions { @@ -313,11 +343,11 @@ func (s *Service) agreementWith(ctx context.Context, group string, tracks []libr edition = append(edition, editionTrack{Medium: int(t.MediumPosition), Position: int(t.Position), Title: t.Title, LengthMs: int(t.LengthMs), Recording: t.RecordingMbid}) } - if got := trackAgreement(tracks, edition); got > n || (got == n && abs(len(edition)-len(tracks)) < abs(size-len(tracks))) { - n, size = got, len(edition) + if got, m := trackAgreement(tracks, edition, titlesEnough); got > n || (got == n && abs(len(edition)-len(tracks)) < abs(size-len(tracks))) { + n, size, miss = got, len(edition), m } } - return n, size, nil + return n, size, miss, nil } // creditedArtist is the artist a want is filed under: the library's own