From ef77658311f2d7471e1bcbc56f252862131e1615 Mon Sep 17 00:00:00 2001
From: Dean Herbert <pe@ppy.sh>
Date: Fri, 11 Sep 2020 16:29:14 +0900
Subject: [PATCH] Add coverage of case where skin.ini doesn't specify
 name/author

---
 osu.Game.Tests/Skins/IO/ImportSkinTest.cs | 26 +++++++++++++++++++++++
 osu.Game/Skinning/SkinManager.cs          | 16 ++++++++++----
 2 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs
index af38d0f3c4..ad1a41a2b3 100644
--- a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs
+++ b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs
@@ -66,6 +66,32 @@ namespace osu.Game.Tests.Skins.IO
             }
         }
 
+        [Test]
+        public async Task TestImportTwiceWithNoMetadata()
+        {
+            using (HeadlessGameHost host = new CleanRunHeadlessGameHost())
+            {
+                try
+                {
+                    var osu = await loadOsu(host);
+
+                    // if a user downloads two skins that do have skin.ini files but don't have any creator metadata in the skin.ini, they should both import separately just for safety.
+                    var imported = await loadIntoOsu(osu, new ZipArchiveReader(createOsk(string.Empty, string.Empty), "download.osk"));
+                    var imported2 = await loadIntoOsu(osu, new ZipArchiveReader(createOsk(string.Empty, string.Empty), "download.osk"));
+
+                    Assert.That(imported2.ID, Is.Not.EqualTo(imported.ID));
+                    Assert.That(osu.Dependencies.Get<SkinManager>().GetAllUserSkins().Count, Is.EqualTo(2));
+
+                    Assert.That(osu.Dependencies.Get<SkinManager>().GetAllUserSkins().First().Files.First().FileInfoID, Is.EqualTo(imported.Files.First().FileInfoID));
+                    Assert.That(osu.Dependencies.Get<SkinManager>().GetAllUserSkins().Last().Files.First().FileInfoID, Is.EqualTo(imported2.Files.First().FileInfoID));
+                }
+                finally
+                {
+                    host.Exit();
+                }
+            }
+        }
+
         [Test]
         public async Task TestImportTwiceWithDifferentMetadata()
         {
diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs
index 46130cbdd4..eacfdaec4a 100644
--- a/osu.Game/Skinning/SkinManager.cs
+++ b/osu.Game/Skinning/SkinManager.cs
@@ -87,11 +87,19 @@ namespace osu.Game.Skinning
 
         protected override SkinInfo CreateModel(ArchiveReader archive) => new SkinInfo { Name = archive.Name };
 
+        private const string unknown_creator_string = "Unknown";
+
         protected override string ComputeHash(SkinInfo item, ArchiveReader reader = null)
         {
-            // this is the optimal way to hash legacy skins, but will need to be reconsidered when we move forward with skin implementation.
-            // likely, the skin should expose a real version (ie. the version of the skin, not the skin.ini version it's targeting).
-            return item.ToString().ComputeSHA2Hash();
+            if (item.Creator != null && item.Creator != unknown_creator_string)
+            {
+                // this is the optimal way to hash legacy skins, but will need to be reconsidered when we move forward with skin implementation.
+                // likely, the skin should expose a real version (ie. the version of the skin, not the skin.ini version it's targeting).
+                return item.ToString().ComputeSHA2Hash();
+            }
+
+            // if there was no creator, the ToString above would give the filename, which along isn't really enough to base any decisions on.
+            return base.ComputeHash(item, reader);
         }
 
         protected override async Task Populate(SkinInfo model, ArchiveReader archive, CancellationToken cancellationToken = default)
@@ -108,7 +116,7 @@ namespace osu.Game.Skinning
             else
             {
                 model.Name = model.Name.Replace(".osk", "");
-                model.Creator ??= "Unknown";
+                model.Creator ??= unknown_creator_string;
             }
         }