From c53b42311d29a338c93ca6e10a29ab0d18e9358d Mon Sep 17 00:00:00 2001
From: Matthew Duggan <mgithub@guarana.org>
Date: Sat, 31 Dec 2022 17:29:42 +0900
Subject: [PATCH] COMMON: Fix path split+join combinations, add tests for same

This resolves multiple scenarios where a path ends up with a trailing
separator.
---
 common/path.cpp    | 24 ++++++++++------
 common/path.h      |  2 +-
 test/common/path.h | 69 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 85 insertions(+), 10 deletions(-)
 create mode 100644 test/common/path.h

diff --git a/common/path.cpp b/common/path.cpp
index c824b723d3c..7319896a6aa 100644
--- a/common/path.cpp
+++ b/common/path.cpp
@@ -61,13 +61,17 @@ String Path::toString(char separator) const {
 	return res;
 }
 
-size_t Path::findLastSeparator() const {
+size_t Path::findLastSeparator(size_t last) const {
+	if (_str.size() < 2)
+		return String::npos;
+
 	size_t res = String::npos;
-	for (uint i = 0; i + 2 < _str.size(); i++) {
-		if (_str[i] == ESCAPER) {
-			i++;
-			if (_str[i] == ESCAPE_SEPARATOR)
-				res = i - 1;
+	if (last == String::npos || last > _str.size())
+		last = _str.size();
+
+	for (uint i = 0; i < last - 1; i++) {
+		if (_str[i] == ESCAPER && _str[i + 1] == ESCAPE_SEPARATOR) {
+			res = i;
 		}
 	}
 
@@ -77,7 +81,8 @@ size_t Path::findLastSeparator() const {
 Path Path::getParent() const {
 	if (_str.size() < 2)
 		return Path();
-	size_t separatorPos = findLastSeparator();
+	// ignore potential trailing separator
+	size_t separatorPos = findLastSeparator(_str.size() - 1);
 	if (separatorPos == String::npos)
 		return Path();
 	Path ret;
@@ -88,7 +93,8 @@ Path Path::getParent() const {
 Path Path::getLastComponent() const {
 	if (_str.size() < 2)
 		return *this;
-	size_t separatorPos = findLastSeparator();
+	// ignore potential trailing separator
+	size_t separatorPos = findLastSeparator(_str.size() - 1);
 	if (separatorPos == String::npos)
 		return *this;
 	Path ret;
@@ -197,7 +203,7 @@ Path &Path::joinInPlace(const Path &x) {
 		return *this;
 
 	size_t lastSep = findLastSeparator();
-	if (!_str.empty() && (lastSep == String::npos || lastSep != _str.size() - 2) && x._str.hasPrefix(DIR_SEPARATOR))
+	if (!_str.empty() && (lastSep == String::npos || lastSep != _str.size() - 2) && !x._str.hasPrefix(DIR_SEPARATOR))
 		_str += DIR_SEPARATOR;
 
 	_str += x._str;
diff --git a/common/path.h b/common/path.h
index 3d3a5f087e3..7cf5530f2ee 100644
--- a/common/path.h
+++ b/common/path.h
@@ -49,7 +49,7 @@ private:
 	String _str;
 
 	String getIdentifierString() const;
-	size_t findLastSeparator() const;
+	size_t findLastSeparator(size_t last = String::npos) const;
 
 public:
 	/**
diff --git a/test/common/path.h b/test/common/path.h
new file mode 100644
index 00000000000..d7c48f3b869
--- /dev/null
+++ b/test/common/path.h
@@ -0,0 +1,69 @@
+#include <cxxtest/TestSuite.h>
+
+#include "common/path.h"
+
+static const char *TEST_PATH = "parent/dir/file.txt";
+
+class PathTestSuite : public CxxTest::TestSuite
+{
+	public:
+	void test_Path() {
+		Common::Path p;
+		TS_ASSERT_EQUALS(p.toString(), Common::String());
+
+		Common::Path p2(TEST_PATH);
+		TS_ASSERT_EQUALS(p2.toString(), Common::String(TEST_PATH));
+	}
+
+	void test_getLastComponent() {
+		Common::Path p(TEST_PATH);
+		TS_ASSERT_EQUALS(p.getLastComponent().toString(), "file.txt");
+	}
+
+	void test_getParent() {
+		Common::Path p(TEST_PATH);
+		TS_ASSERT_EQUALS(p.getParent().toString(), "parent/dir/");
+		// TODO: should this work?
+		TS_ASSERT_EQUALS(p.getParent().getLastComponent().toString(), "dir/");
+	}
+
+	void test_join() {
+		Common::Path p("dir");
+		Common::Path p2 = p.join("file.txt");
+		TS_ASSERT_EQUALS(p2.toString(), "dir/file.txt");
+
+		Common::Path p3(TEST_PATH);
+		Common::Path p4 = p3.getParent().join("other.txt");
+		TS_ASSERT_EQUALS(p4.toString(), "parent/dir/other.txt");
+	}
+
+	// Ensure we can joinInPlace correctly with leading or trailing separators
+	void test_joinInPlace() {
+		Common::Path p("abc/def");
+		p.joinInPlace("file.txt");
+		TS_ASSERT_EQUALS(p.toString(), "abc/def/file.txt");
+
+		Common::Path p2("xyz/def");
+		p2.joinInPlace(Common::Path("file.txt"));
+		TS_ASSERT_EQUALS(p2.toString(), "xyz/def/file.txt");
+
+		Common::Path p3("ghi/def/");
+		p3.joinInPlace(Common::Path("file.txt"));
+		TS_ASSERT_EQUALS(p3.toString(), "ghi/def/file.txt");
+
+		Common::Path p4("123/def");
+		p4.joinInPlace(Common::Path("/file4.txt"));
+		TS_ASSERT_EQUALS(p4.toString(), "123/def/file4.txt");
+	}
+
+	void test_separator() {
+		Common::Path p(TEST_PATH, '\\');
+		TS_ASSERT_EQUALS(p.getLastComponent().toString(), TEST_PATH);
+		TS_ASSERT_EQUALS(p.getParent().toString(), "");
+
+		Common::Path p2(TEST_PATH, 'e');
+		TS_ASSERT_EQUALS(p2.getLastComponent().toString(), ".txt");
+		TS_ASSERT_EQUALS(p2.getParent().toString('#'), "par#nt/dir/fil#");
+		TS_ASSERT_EQUALS(p2.getParent().getParent().toString('#'), "par#");
+	}
+};