From d2c9a29c0d4bf8f805dbead933156a57ea466d75 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 4 Oct 2019 10:45:18 +0800 Subject: [PATCH 01/10] Remove unnecessary local assign --- osu.Game/Screens/Select/SongSelect.cs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/osu.Game/Screens/Select/SongSelect.cs b/osu.Game/Screens/Select/SongSelect.cs index 5ab49fa2b9..e97fac96c5 100644 --- a/osu.Game/Screens/Select/SongSelect.cs +++ b/osu.Game/Screens/Select/SongSelect.cs @@ -66,11 +66,7 @@ namespace osu.Game.Screens.Select /// protected readonly Container FooterPanels; - protected override BackgroundScreen CreateBackground() - { - var background = new BackgroundScreenBeatmap(); - return background; - } + protected override BackgroundScreen CreateBackground() => new BackgroundScreenBeatmap(); protected readonly BeatmapCarousel Carousel; private readonly BeatmapInfoWedge beatmapInfoWedge; From cb1f7e2dc7840f3016914e5702c575dfb2a8a133 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Thu, 10 Oct 2019 13:48:36 +0200 Subject: [PATCH 02/10] Fix platform dependency in buffered reader test Tests for the line-buffered reader added in 7b1ff38 were subtly dependent on the execution environment due to differing end-of-line markers on Windows and Unix-based systems. Because StreamReader discards all newlines when reading line-by-line, LineBufferedReader used a StringBuilder to patch the peeked lines back together with the remaining contents of the file being read. As StringBuilder.AppendLine uses the environment-specific newline delimiter, the delimiters after the peeked-but-unconsumed lines can therefore be substituted by the platform-specific variants, causing the test failures due to the overly-simplified way they were written. Reformulate the test to avoid such issues from resurfacing again by splitting lines by \r or \n and then testing each line individually. Additionally remove all raw literals in favour of explicitly mixing various line delimiter character sequences for additional coverage. --- .../Beatmaps/IO/LineBufferedReaderTest.cs | 34 +++++++------------ 1 file changed, 13 insertions(+), 21 deletions(-) diff --git a/osu.Game.Tests/Beatmaps/IO/LineBufferedReaderTest.cs b/osu.Game.Tests/Beatmaps/IO/LineBufferedReaderTest.cs index b582ca0a6f..25517ad615 100644 --- a/osu.Game.Tests/Beatmaps/IO/LineBufferedReaderTest.cs +++ b/osu.Game.Tests/Beatmaps/IO/LineBufferedReaderTest.cs @@ -1,6 +1,7 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. +using System; using System.IO; using System.Text; using NUnit.Framework; @@ -14,9 +15,7 @@ namespace osu.Game.Tests.Beatmaps.IO [Test] public void TestReadLineByLine() { - const string contents = @"line 1 -line 2 -line 3"; + const string contents = "line 1\rline 2\nline 3"; using (var stream = new MemoryStream(Encoding.UTF8.GetBytes(contents))) using (var bufferedReader = new LineBufferedReader(stream)) @@ -31,9 +30,7 @@ line 3"; [Test] public void TestPeekLineOnce() { - const string contents = @"line 1 -peek this -line 3"; + const string contents = "line 1\r\npeek this\nline 3"; using (var stream = new MemoryStream(Encoding.UTF8.GetBytes(contents))) using (var bufferedReader = new LineBufferedReader(stream)) @@ -49,9 +46,7 @@ line 3"; [Test] public void TestPeekLineMultipleTimes() { - const string contents = @"peek this once -line 2 -peek this a lot"; + const string contents = "peek this once\nline 2\rpeek this a lot"; using (var stream = new MemoryStream(Encoding.UTF8.GetBytes(contents))) using (var bufferedReader = new LineBufferedReader(stream)) @@ -70,8 +65,7 @@ peek this a lot"; [Test] public void TestPeekLineAtEndOfStream() { - const string contents = @"first line -second line"; + const string contents = "first line\r\nsecond line"; using (var stream = new MemoryStream(Encoding.UTF8.GetBytes(contents))) using (var bufferedReader = new LineBufferedReader(stream)) @@ -100,8 +94,7 @@ second line"; [Test] public void TestReadToEndNoPeeks() { - const string contents = @"first line -second line"; + const string contents = "first line\r\nsecond line"; using (var stream = new MemoryStream(Encoding.UTF8.GetBytes(contents))) using (var bufferedReader = new LineBufferedReader(stream)) @@ -113,20 +106,19 @@ second line"; [Test] public void TestReadToEndAfterReadsAndPeeks() { - const string contents = @"this line is gone -this one shouldn't be -these ones -definitely not"; + const string contents = "this line is gone\rthis one shouldn't be\r\nthese ones\ndefinitely not"; using (var stream = new MemoryStream(Encoding.UTF8.GetBytes(contents))) using (var bufferedReader = new LineBufferedReader(stream)) { Assert.AreEqual("this line is gone", bufferedReader.ReadLine()); Assert.AreEqual("this one shouldn't be", bufferedReader.PeekLine()); - const string ending = @"this one shouldn't be -these ones -definitely not"; - Assert.AreEqual(ending, bufferedReader.ReadToEnd()); + + var endingLines = bufferedReader.ReadToEnd().Split(new[] { '\r', '\n' }, StringSplitOptions.RemoveEmptyEntries); + Assert.AreEqual(3, endingLines.Length); + Assert.AreEqual("this one shouldn't be", endingLines[0]); + Assert.AreEqual("these ones", endingLines[1]); + Assert.AreEqual("definitely not", endingLines[2]); } } } From c8ffc134d47f717e6279d2a3c204b0f7c0951f77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Thu, 10 Oct 2019 22:36:43 +0200 Subject: [PATCH 03/10] Use nameof when instantiating headless game hosts As a purely cosmetic code improvement, substitute string literals in constructor calls of HeadlessGameHost in ImportBeatmapTest for nameof operator usages. --- .../Beatmaps/IO/ImportBeatmapTest.cs | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs index 6da8d8cb71..9cbb5c39c3 100644 --- a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs +++ b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs @@ -29,7 +29,7 @@ namespace osu.Game.Tests.Beatmaps.IO public async Task TestImportWhenClosed() { //unfortunately for the time being we need to reference osu.Framework.Desktop for a game host here. - using (HeadlessGameHost host = new CleanRunHeadlessGameHost("TestImportWhenClosed")) + using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(TestImportWhenClosed))) { try { @@ -46,7 +46,7 @@ namespace osu.Game.Tests.Beatmaps.IO public async Task TestImportThenDelete() { //unfortunately for the time being we need to reference osu.Framework.Desktop for a game host here. - using (HeadlessGameHost host = new CleanRunHeadlessGameHost("TestImportThenDelete")) + using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(TestImportThenDelete))) { try { @@ -67,7 +67,7 @@ namespace osu.Game.Tests.Beatmaps.IO public async Task TestImportThenImport() { //unfortunately for the time being we need to reference osu.Framework.Desktop for a game host here. - using (HeadlessGameHost host = new CleanRunHeadlessGameHost("TestImportThenImport")) + using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(TestImportThenImport))) { try { @@ -94,7 +94,7 @@ namespace osu.Game.Tests.Beatmaps.IO public async Task TestImportCorruptThenImport() { //unfortunately for the time being we need to reference osu.Framework.Desktop for a game host here. - using (HeadlessGameHost host = new CleanRunHeadlessGameHost("TestImportThenImport")) + using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(TestImportCorruptThenImport))) { try { @@ -136,7 +136,7 @@ namespace osu.Game.Tests.Beatmaps.IO public async Task TestRollbackOnFailure() { //unfortunately for the time being we need to reference osu.Framework.Desktop for a game host here. - using (HeadlessGameHost host = new CleanRunHeadlessGameHost("TestRollbackOnFailure")) + using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(TestRollbackOnFailure))) { try { @@ -213,7 +213,7 @@ namespace osu.Game.Tests.Beatmaps.IO public async Task TestImportThenImportDifferentHash() { //unfortunately for the time being we need to reference osu.Framework.Desktop for a game host here. - using (HeadlessGameHost host = new CleanRunHeadlessGameHost("TestImportThenImportDifferentHash")) + using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(TestImportThenImportDifferentHash))) { try { @@ -244,7 +244,7 @@ namespace osu.Game.Tests.Beatmaps.IO public async Task TestImportThenDeleteThenImport() { //unfortunately for the time being we need to reference osu.Framework.Desktop for a game host here. - using (HeadlessGameHost host = new CleanRunHeadlessGameHost("TestImportThenDeleteThenImport")) + using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(TestImportThenDeleteThenImport))) { try { @@ -272,7 +272,7 @@ namespace osu.Game.Tests.Beatmaps.IO public async Task TestImportThenDeleteThenImportWithOnlineIDMismatch(bool set) { //unfortunately for the time being we need to reference osu.Framework.Desktop for a game host here. - using (HeadlessGameHost host = new CleanRunHeadlessGameHost($"TestImportThenDeleteThenImport-{set}")) + using (HeadlessGameHost host = new CleanRunHeadlessGameHost($"{nameof(TestImportThenDeleteThenImportWithOnlineIDMismatch)}-{set}")) { try { @@ -306,7 +306,7 @@ namespace osu.Game.Tests.Beatmaps.IO public async Task TestImportWithDuplicateBeatmapIDs() { //unfortunately for the time being we need to reference osu.Framework.Desktop for a game host here. - using (HeadlessGameHost host = new CleanRunHeadlessGameHost("TestImportWithDuplicateBeatmapID")) + using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(TestImportWithDuplicateBeatmapIDs))) { try { @@ -392,7 +392,7 @@ namespace osu.Game.Tests.Beatmaps.IO [Test] public async Task TestImportWhenFileOpen() { - using (HeadlessGameHost host = new CleanRunHeadlessGameHost("TestImportWhenFileOpen")) + using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(TestImportWhenFileOpen))) { try { @@ -414,7 +414,7 @@ namespace osu.Game.Tests.Beatmaps.IO [Test] public async Task TestImportNestedStructure() { - using (HeadlessGameHost host = new CleanRunHeadlessGameHost("TestImportNestedStructure")) + using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(TestImportNestedStructure))) { try { From 11acd177f18442872e0b06e93703c63c9da35e95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Thu, 10 Oct 2019 23:10:22 +0200 Subject: [PATCH 04/10] Add import test with files to be filtered out Add a test case reproducing the conditions encountered "in the wild" wherein a skin import would be performed incorrectly due to a __MACOSX resource fork directory present next to a directory with the actual skin files in the archive. --- .../Beatmaps/IO/ImportBeatmapTest.cs | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs index 9cbb5c39c3..e96637cae9 100644 --- a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs +++ b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs @@ -456,6 +456,60 @@ namespace osu.Game.Tests.Beatmaps.IO } } + [Test] + public async Task TestImportWithIgnoredDirectoryInArchive() + { + using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(TestImportWithIgnoredDirectoryInArchive))) + { + try + { + var osu = loadOsu(host); + + var temp = TestResources.GetTestBeatmapForImport(); + + string extractedFolder = $"{temp}_extracted"; + string dataFolder = Path.Combine(extractedFolder, "actual_data"); + string resourceForkFolder = Path.Combine(extractedFolder, "__MACOSX"); + string resourceForkFilePath = Path.Combine(resourceForkFolder, ".extracted"); + + Directory.CreateDirectory(dataFolder); + Directory.CreateDirectory(resourceForkFolder); + + using (var resourceForkFile = File.CreateText(resourceForkFilePath)) + { + resourceForkFile.WriteLine("adding content so that it's not empty"); + } + + try + { + using (var zip = ZipArchive.Open(temp)) + zip.WriteToDirectory(dataFolder); + + using (var zip = ZipArchive.Create()) + { + zip.AddAllFromDirectory(extractedFolder); + zip.SaveTo(temp, new ZipWriterOptions(CompressionType.Deflate)); + } + + var imported = await osu.Dependencies.Get().Import(temp); + + ensureLoaded(osu); + + Assert.IsFalse(imported.Files.Any(f => f.Filename.Contains("__MACOSX")), "Files contain resource fork folder, which should be ignored"); + Assert.IsFalse(imported.Files.Any(f => f.Filename.Contains("actual_data")), "Files contain common subfolder"); + } + finally + { + Directory.Delete(extractedFolder, true); + } + } + finally + { + host.Exit(); + } + } + } + public static async Task LoadOszIntoOsu(OsuGameBase osu, string path = null) { var temp = path ?? TestResources.GetTestBeatmapForImport(); From 57bfa18359fe9d0d4fd1221e888405065342635d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Thu, 10 Oct 2019 23:42:02 +0200 Subject: [PATCH 05/10] Filter out OS-generated files from archives Add a filename ignore list to ZipArchiveReader to filter out superfluous OS-generated files from archives during the import process. In addition to decreasing the size of files imported this allows imports of some incorrectly-constructed archives. An example is the case of having a __MACOSX directory next to a single directory with the actual files - filtering out the former at ZipArchiveReader allows the fallback added in #6170 to work. --- osu.Game/IO/Archives/ZipArchiveReader.cs | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/osu.Game/IO/Archives/ZipArchiveReader.cs b/osu.Game/IO/Archives/ZipArchiveReader.cs index d934ac54c4..2233520916 100644 --- a/osu.Game/IO/Archives/ZipArchiveReader.cs +++ b/osu.Game/IO/Archives/ZipArchiveReader.cs @@ -1,15 +1,30 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. +using System; using System.Collections.Generic; using System.IO; using System.Linq; using SharpCompress.Archives.Zip; +using SharpCompress.Common; namespace osu.Game.IO.Archives { public sealed class ZipArchiveReader : ArchiveReader { + /// + /// List of substrings that indicate a file should be ignored during the import process + /// (usually due to representing no useful data and being autogenerated by the OS). + /// + private static readonly string[] filename_ignore_list = + { + // Mac-specific + "__MACOSX", + ".DS_Store", + // Windows-specific + "Thumbs.db" + }; + private readonly Stream archiveStream; private readonly ZipArchive archive; @@ -43,7 +58,9 @@ namespace osu.Game.IO.Archives archiveStream.Dispose(); } - public override IEnumerable Filenames => archive.Entries.Select(e => e.Key).ToArray(); + private static bool canBeIgnored(IEntry entry) => filename_ignore_list.Any(ignoredName => entry.Key.IndexOf(ignoredName, StringComparison.InvariantCultureIgnoreCase) >= 0); + + public override IEnumerable Filenames => archive.Entries.Where(e => !canBeIgnored(e)).Select(e => e.Key).ToArray(); public override Stream GetUnderlyingStream() => archiveStream; } From 9fb5c85a18d2ff4c0f3abb33b96bc490d8cad487 Mon Sep 17 00:00:00 2001 From: "dependabot-preview[bot]" <27856297+dependabot-preview[bot]@users.noreply.github.com> Date: Fri, 11 Oct 2019 05:15:14 +0000 Subject: [PATCH 06/10] Bump ppy.osu.Game.Resources from 2019.913.0 to 2019.1010.0 Bumps [ppy.osu.Game.Resources](https://github.com/ppy/osu-resources) from 2019.913.0 to 2019.1010.0. - [Release notes](https://github.com/ppy/osu-resources/releases) - [Commits](https://github.com/ppy/osu-resources/compare/2019.913.0...2019.1010.0) Signed-off-by: dependabot-preview[bot] --- osu.Android.props | 2 +- osu.Game/osu.Game.csproj | 2 +- osu.iOS.props | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/osu.Android.props b/osu.Android.props index 0a831eceb2..85766665a9 100644 --- a/osu.Android.props +++ b/osu.Android.props @@ -61,7 +61,7 @@ - + diff --git a/osu.Game/osu.Game.csproj b/osu.Game/osu.Game.csproj index c2888c8e21..ab7c40116b 100644 --- a/osu.Game/osu.Game.csproj +++ b/osu.Game/osu.Game.csproj @@ -25,7 +25,7 @@ - + diff --git a/osu.iOS.props b/osu.iOS.props index 8ad7a58e36..925a217a13 100644 --- a/osu.iOS.props +++ b/osu.iOS.props @@ -117,7 +117,7 @@ - + From d44acf20b40b75e9cc92ac2feb8c3d6f2779b1cc Mon Sep 17 00:00:00 2001 From: "dependabot-preview[bot]" <27856297+dependabot-preview[bot]@users.noreply.github.com> Date: Fri, 11 Oct 2019 05:33:15 +0000 Subject: [PATCH 07/10] Bump ppy.osu.Framework.NativeLibs from 2019.1010.0 to 2019.1011.0 Bumps [ppy.osu.Framework.NativeLibs](https://github.com/ppy/osu-framework) from 2019.1010.0 to 2019.1011.0. - [Release notes](https://github.com/ppy/osu-framework/releases) - [Commits](https://github.com/ppy/osu-framework/compare/2019.1010.0...2019.1011.0) Signed-off-by: dependabot-preview[bot] --- osu.iOS.props | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.iOS.props b/osu.iOS.props index 8ad7a58e36..7532bd5100 100644 --- a/osu.iOS.props +++ b/osu.iOS.props @@ -124,6 +124,6 @@ - + From 4f0354bdb63cc6216117db6e6f7148a2cf3a5f84 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 11 Oct 2019 14:56:54 +0900 Subject: [PATCH 08/10] Revert nativelibs update --- osu.iOS.props | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.iOS.props b/osu.iOS.props index a73d40b37b..925a217a13 100644 --- a/osu.iOS.props +++ b/osu.iOS.props @@ -124,6 +124,6 @@ - + From 1faef2f97dbefda23b62dbb39494f0a6fe6b96f7 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 11 Oct 2019 16:17:15 +0900 Subject: [PATCH 09/10] Update fastlane --- Gemfile.lock | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 7df9c46482..ab594aee74 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -18,7 +18,7 @@ GEM unf (>= 0.0.5, < 1.0.0) dotenv (2.7.5) emoji_regex (1.0.1) - excon (0.66.0) + excon (0.67.0) faraday (0.15.4) multipart-post (>= 1.2, < 3) faraday-cookie_jar (0.0.6) @@ -27,7 +27,7 @@ GEM faraday_middleware (0.13.1) faraday (>= 0.7.4, < 1.0) fastimage (2.1.7) - fastlane (2.131.0) + fastlane (2.133.0) CFPropertyList (>= 2.3, < 4.0.0) addressable (>= 2.3, < 3.0.0) babosa (>= 1.0.2, < 2.0.0) @@ -37,9 +37,9 @@ GEM dotenv (>= 2.1.1, < 3.0.0) emoji_regex (>= 0.1, < 2.0) excon (>= 0.45.0, < 1.0.0) - faraday (~> 0.9) + faraday (< 0.16.0) faraday-cookie_jar (~> 0.0.6) - faraday_middleware (~> 0.9) + faraday_middleware (< 0.16.0) fastimage (>= 2.1.0, < 3.0.0) gh_inspector (>= 1.1.2, < 2.0.0) google-api-client (>= 0.21.2, < 0.24.0) @@ -52,7 +52,7 @@ GEM multipart-post (~> 2.0.0) plist (>= 3.1.0, < 4.0.0) public_suffix (~> 2.0.0) - rubyzip (>= 1.2.2, < 2.0.0) + rubyzip (>= 1.3.0, < 2.0.0) security (= 0.1.3) simctl (~> 1.6.3) slack-notifier (>= 2.0.0, < 3.0.0) @@ -102,7 +102,7 @@ GEM memoist (0.16.0) mime-types (3.3) mime-types-data (~> 3.2015) - mime-types-data (3.2019.0904) + mime-types-data (3.2019.1009) mini_magick (4.9.5) mini_portile2 (2.4.0) multi_json (1.13.1) @@ -121,9 +121,9 @@ GEM uber (< 0.2.0) retriable (3.1.2) rouge (2.0.7) - rubyzip (1.2.4) + rubyzip (1.3.0) security (0.1.3) - signet (0.11.0) + signet (0.12.0) addressable (~> 2.3) faraday (~> 0.9) jwt (>= 1.5, < 3.0) From 4b84564f474e1369817815a7bfa8dcb90d99f0c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Fri, 11 Oct 2019 09:24:41 +0200 Subject: [PATCH 10/10] Switch casing comparison mode to ordinal Switch from InvariantCultureIgnoreCase to OrdinalIgnoreCase when checking file paths in archives for substrings indicating the file can be ignored for performance gains. Co-Authored-By: Dan Balasescu --- osu.Game/IO/Archives/ZipArchiveReader.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/IO/Archives/ZipArchiveReader.cs b/osu.Game/IO/Archives/ZipArchiveReader.cs index 2233520916..9033e7529d 100644 --- a/osu.Game/IO/Archives/ZipArchiveReader.cs +++ b/osu.Game/IO/Archives/ZipArchiveReader.cs @@ -58,7 +58,7 @@ namespace osu.Game.IO.Archives archiveStream.Dispose(); } - private static bool canBeIgnored(IEntry entry) => filename_ignore_list.Any(ignoredName => entry.Key.IndexOf(ignoredName, StringComparison.InvariantCultureIgnoreCase) >= 0); + private static bool canBeIgnored(IEntry entry) => filename_ignore_list.Any(ignoredName => entry.Key.IndexOf(ignoredName, StringComparison.OrdinalIgnoreCase) >= 0); public override IEnumerable Filenames => archive.Entries.Where(e => !canBeIgnored(e)).Select(e => e.Key).ToArray();