fix: never take a fuzzy artist match on the user's behalf
A fuzzy candidate at or above 0.75 similarity was selected automatically under --yes and written to artists.toml, so every later album by that credit followed it into the same folder. Four of the edge fixtures were filed under the wrong artist that way: "Cue Artist" into dupe-artist, "RTL Artist" into art-artist, "Long Artist" into loose-artist and "Slash Artist" into scan-artist. Nothing downstream catches it, because no check compares a folder name against the ALBUMARTIST tag. A similar name is now only offered, never taken: an unattended run creates the folder the credit names and prints the exact artists.toml line to add if the two really are the same artist. The interactive prompt still lists the candidates, but its default is the new folder rather than the guess. The alias round trip is fixed too. TOMLKey writes keys with %q and the reader only trimmed the outer quotes, so a credit containing a quote or a backslash -- "Weird Al" Yankovic, say -- never matched its own entry. Closes c14e366
diff --git a/internal/artist/artist.go b/internal/artist/artist.go
@@ -7,6 +7,7 @@ import (
"os"
"path/filepath"
"sort"
+ "strconv"
"strings"
"unicode"
@@ -438,7 +439,7 @@ func (ix *Index) SaveAliases(path string) error {
b.WriteString("#\n")
b.WriteString("# Edit freely: the value is used as-is, so it must be a safe directory name.\n\n")
for _, k := range keys {
- fmt.Fprintf(&b, "%s = %q\n", tomlKey(k), ix.Alias[k])
+ fmt.Fprintf(&b, "%s = %q\n", TOMLKey(k), ix.Alias[k])
}
tmp := path + ".tmp"
if err := os.WriteFile(tmp, []byte(b.String()), 0o644); err != nil {
@@ -447,7 +448,7 @@ func (ix *Index) SaveAliases(path string) error {
return os.Rename(tmp, path)
}
-func tomlKey(s string) string {
+func TOMLKey(s string) string {
if s == "" {
return `""`
}
@@ -465,8 +466,15 @@ func tomlKey(s string) string {
return fmt.Sprintf("%q", s)
}
+// unquoteTOML undoes TOMLKey. Trimming the outer quotes is not enough: TOMLKey
+// writes %q, so a credit containing a quote or a backslash -- "Weird Al"
+// Yankovic, or anything with a \u escape in it -- comes back escaped, no longer
+// equals the credit it was written for, and the entry silently never matches.
func unquoteTOML(s string) string {
if len(s) >= 2 && s[0] == '"' && s[len(s)-1] == '"' {
+ if unquoted, err := strconv.Unquote(s); err == nil {
+ return unquoted
+ }
return s[1 : len(s)-1]
}
return s
diff --git a/internal/cli/artist_test.go b/internal/cli/artist_test.go
@@ -0,0 +1,130 @@
+package cli
+
+import (
+ "bytes"
+ "os"
+ "path/filepath"
+ "strings"
+ "testing"
+
+ "intake/internal/artist"
+ "intake/internal/config"
+ "intake/internal/scan"
+)
+
+func artistTestApp(t *testing.T, root string, yes bool) *App {
+ t.Helper()
+ cfg := config.Defaults()
+ cfg.Root = root
+ return &App{Cfg: cfg, Yes: yes, Out: &bytes.Buffer{}, Err: &bytes.Buffer{}}
+}
+
+func existingArtist(t *testing.T, root, dir string) {
+ t.Helper()
+ if err := os.MkdirAll(filepath.Join(root, "archive", dir), 0o755); err != nil {
+ t.Fatal(err)
+ }
+}
+
+func aliasFile(t *testing.T, root string) map[string]string {
+ t.Helper()
+ ix := artist.NewIndex()
+ if err := ix.LoadAliases(artist.AliasPath(root)); err != nil {
+ t.Fatal(err)
+ }
+ return ix.Alias
+}
+
+// "Blur" and "Blue" are 75% alike, which is exactly the fuzzy threshold. Taking
+// the match files Blue's album inside Blur's folder, and nothing downstream ever
+// compares a folder name with the ALBUMARTIST tag, so it is silent. An
+// unattended run has nobody to ask, so it must create the folder the credit
+// names and say what it did.
+func TestUnattendedRunDoesNotFileAnArtistIntoASimilarFolder(t *testing.T) {
+ for _, tc := range []struct{ existing, credit, want string }{
+ {"blur", "Blue", "blue"},
+ {"the-cult", "The Cure", "the-cure"},
+ {"art-artist", "RTL Artist", "rtl-artist"},
+ {"scan-artist", "Slash Artist", "slash-artist"},
+ {"loose-artist", "Long Artist", "long-artist"},
+ } {
+ t.Run(tc.credit, func(t *testing.T) {
+ root := t.TempDir()
+ existingArtist(t, root, tc.existing)
+ a := artistTestApp(t, root, true)
+
+ key, _, err := a.resolveArtist(scan.Album{}, tc.credit, false, "")
+ if err != nil {
+ t.Fatal(err)
+ }
+ if key.Dir != tc.want {
+ t.Errorf("folder = %q, want %q (existing %q is only similar)", key.Dir, tc.want, tc.existing)
+ }
+ out := a.Out.(*bytes.Buffer).String()
+ if !strings.Contains(out, tc.existing) {
+ t.Errorf("the near match %q was not reported:\n%s", tc.existing, out)
+ }
+ if got := aliasFile(t, root); got[tc.credit] != "" {
+ t.Errorf("a guess was written to artists.toml as %q = %q", tc.credit, got[tc.credit])
+ }
+ })
+ }
+}
+
+// The safe path must not have broken the certain one: a recorded alias, or a
+// folder whose name is exactly the credit, is still followed without asking.
+func TestCertainArtistMatchesAreStillUsed(t *testing.T) {
+ root := t.TempDir()
+ existingArtist(t, root, "beatles")
+
+ a := artistTestApp(t, root, true)
+ key, _, err := a.resolveArtist(scan.Album{}, "Beatles", false, "")
+ if err != nil {
+ t.Fatal(err)
+ }
+ if key.Dir != "beatles" {
+ t.Errorf("exact folder match = %q, want beatles", key.Dir)
+ }
+
+ if err := os.WriteFile(artist.AliasPath(root),
+ []byte("\"The Beatles\" = \"beatles\"\n"), 0o644); err != nil {
+ t.Fatal(err)
+ }
+ a = artistTestApp(t, root, true)
+ key, _, err = a.resolveArtist(scan.Album{}, "The Beatles", false, "")
+ if err != nil {
+ t.Fatal(err)
+ }
+ if key.Dir != "beatles" {
+ t.Errorf("recorded alias = %q, want beatles", key.Dir)
+ }
+}
+
+// An alias written by intake has to survive being read back. TOMLKey writes the
+// key with %q, so a credit with a quote in it comes back escaped unless the
+// reader undoes the escaping, and the entry then matches nothing at all.
+func TestAliasRoundTripsThroughQuoting(t *testing.T) {
+ root := t.TempDir()
+ for _, credit := range []string{
+ `"Weird Al" Yankovic`,
+ `RTL \u200fArtist`,
+ "AC/DC",
+ "Straße",
+ `back\slash`,
+ } {
+ t.Run(credit, func(t *testing.T) {
+ ix := artist.NewIndex()
+ ix.SetAlias(credit, "some-folder")
+ if err := ix.SaveAliases(artist.AliasPath(root)); err != nil {
+ t.Fatal(err)
+ }
+ reloaded := artist.NewIndex()
+ if err := reloaded.LoadAliases(artist.AliasPath(root)); err != nil {
+ t.Fatal(err)
+ }
+ if got := reloaded.Alias[credit]; got != "some-folder" {
+ t.Errorf("alias for %q came back as %q", credit, got)
+ }
+ })
+ }
+}
diff --git a/internal/cli/interactive.go b/internal/cli/interactive.go
@@ -6,7 +6,6 @@ import (
"os"
"os/exec"
"path/filepath"
- "sort"
"strconv"
"strings"
@@ -100,6 +99,31 @@ func (a *App) resolveArtist(al scan.Album, credit string, various bool, editorAr
return plan.DirKey{Dir: best.Dir, Credit: primary, Source: describeMatch(best)}, primary, nil
}
+ // A fuzzy match is a guess that two names which merely look alike are the
+ // same artist, and an unattended run must not act on a guess. "Blur" and
+ // "Blue" score 75%; "The Cure" and "The Cult" score 75%; "RTL Artist" and
+ // "Art Artist" score 80%. Taking one of those files the album under a
+ // different artist's folder: nothing downstream notices, because a folder
+ // name is never verified against the ALBUMARTIST tag, and once
+ // rememberAlias has written it down every later album by that artist follows
+ // it there. Creating the folder the credit actually names is visible in a
+ // directory listing and costs one line of artists.toml when the two really
+ // are the same artist, so that is what an unattended run does instead.
+ //
+ // A person sitting in front of the prompt is still shown the candidates and
+ // may pick one; see askArtist, whose default is now the new folder.
+ if best.Fuzzy && a.Yes {
+ fresh := normalize.DirName(primary)
+ if fresh == "" {
+ fresh = "unknown-artist"
+ }
+ a.logf("artist folder: %s (new)", fresh)
+ a.logf("note: %q is %.0f%% similar to the existing folder %q, which was NOT used.", primary, best.Score*100, best.Dir)
+ a.logf(" if they are the same artist, add this line to %s and file again:", artist.AliasPath(a.Cfg.Root))
+ a.logf(" %s = %q", artist.TOMLKey(primary), best.Dir)
+ return plan.DirKey{Dir: fresh, Credit: primary, Source: "new"}, primary, nil
+ }
+
if best.Score == 0 && best.Dir == normalize.DirName(primary) {
a.logf("artist folder: %s (new)", best.Dir)
a.rememberAlias(ix, credit, best.Dir)
@@ -193,6 +217,8 @@ func (a *App) askArtist(m artist.Match) (string, error) {
type row struct {
dir string
count int
+ fuzzy bool
+ score float64
}
var rows []row
seen := map[string]bool{}
@@ -201,35 +227,53 @@ func (a *App) askArtist(m artist.Match) (string, error) {
continue
}
seen[c.Dir] = true
- rows = append(rows, row{c.Dir, a.albumCount(c.Dir)})
+ rows = append(rows, row{c.Dir, a.albumCount(c.Dir), c.Fuzzy, c.Score})
if len(rows) >= 8 {
break
}
}
+ // The default is what a bare Enter takes, so it must not be the one entry
+ // that is only a guess. Listing a fuzzy folder is useful -- the person
+ // reading it may know that "Blur" really is "Blue" -- but choosing it has to
+ // be deliberate, which is also what an unattended run does by never taking
+ // one at all.
+ def := normalize.DirName(m.Credit)
+ for _, r := range rows {
+ if !r.fuzzy {
+ def = r.dir
+ break
+ }
+ }
+ if def == "" {
+ def = rows[0].dir
+ }
for i, r := range rows {
rel := ""
if r.count > 0 {
rel = fmt.Sprintf(" (%d album(s))", r.count)
}
reason := ""
- if i < len(m.Dirs) {
- reason = " " + describeMatch(m.Dirs[i])
+ switch {
+ case !r.fuzzy:
+ reason = " different name, or already decided"
+ default:
+ reason = fmt.Sprintf(" looks %.0f%% alike; only a guess", r.score*100)
}
a.logf(" [%d] %s%s%s", i+1, r.dir, rel, reason)
}
a.logf(" [n] create a new folder")
a.logf(" [t] type a different name")
- fmt.Fprintf(a.Out, "choose [Enter = %s] ", rows[0].dir)
+ fmt.Fprintf(a.Out, "choose [Enter = %s] ", def)
_ = os.Stdout.Sync()
line, err := bufio.NewReader(a.In).ReadString('\n')
if err != nil && line == "" {
- return rows[0].dir, nil
+ return def, nil
}
line = strings.TrimSpace(line)
switch {
case line == "":
- return rows[0].dir, nil
+ return def, nil
case strings.EqualFold(line, "n"):
fmt.Fprintf(a.Out, "new folder name [%s] ", normalize.DirName(m.Credit))
name, _ := bufio.NewReader(a.In).ReadString('\n')
@@ -243,7 +287,7 @@ func (a *App) askArtist(m artist.Match) (string, error) {
name, _ := bufio.NewReader(a.In).ReadString('\n')
name = normalize.DirName(name)
if name == "" {
- return rows[0].dir, nil
+ return def, nil
}
return name, nil
default:
@@ -254,7 +298,7 @@ func (a *App) askArtist(m artist.Match) (string, error) {
return key, nil
}
}
- return rows[0].dir, nil
+ return def, nil
}
func (a *App) albumCount(dir string) int {
@@ -395,34 +439,3 @@ func nameIssues(p plan.Plan) bool {
}
return false
}
-
-func sortedDirs(in []string) []string {
- out := append([]string(nil), in...)
- sort.Strings(out)
- return out
-}
-
-func artistDirOfPlan(p plan.Plan) string {
- if p.ArchiveDir == "" {
- return ""
- }
- return filepath.Base(filepath.Dir(p.ArchiveDir))
-}
-
-func alArtistCredit(credit string) string { return strings.TrimSpace(credit) }
-
-func editorFixesAll(p plan.Plan) bool {
- for _, i := range p.Issues {
- if !i.Hard {
- continue
- }
- t := i.Text
- fixable := strings.Contains(t, "no usable title") ||
- strings.Contains(t, "empty directory name") ||
- strings.Contains(t, "no ASCII folder name")
- if !fixable {
- return false
- }
- }
- return true
-}