Fix RemoveLastPathComponent edge case with absolute paths

src/filesys.cpp

@@ -610,14 +610,12 @@ bool RecursiveDeleteContent(const std::string &path)
 
 bool CreateAllDirs(const std::string &path)
 {
-
 	std::vector<std::string> tocreate;
-	std::string basepath = path;
-	while(!PathExists(basepath))
-	{
+	std::string basepath = path, removed;
+	while (!PathExists(basepath)) {
 		tocreate.push_back(basepath);
-		basepath = RemoveLastPathComponent(basepath);
-		if(basepath.empty())
+		basepath = RemoveLastPathComponent(basepath, &removed);
+		if (removed.empty())
 			break;
 	}
 	for(int i=tocreate.size()-1;i>=0;i--)
@@ -748,25 +746,31 @@ std::string RemoveLastPathComponent(const std::string &path,
 
 	size_t remaining = path.size();
 
-	for(int i = 0; i < count; ++i){
-		// strip a dir delimiter
-		while(remaining != 0 && IsDirDelimiter(path[remaining-1]))
+	for (int i = 0; i < count; ++i) {
+		// strip a dir delimiter, unless the path is empty
+		// because "" and "/" are not the same
+		// FIXME: same problem probably exists on win32 with "C:"
+		while (remaining > 1 && IsDirDelimiter(path[remaining-1]))
 			remaining--;
 		// strip a path component
 		size_t component_end = remaining;
-		while(remaining != 0 && !IsDirDelimiter(path[remaining-1]))
+		while (remaining != 0 && !IsDirDelimiter(path[remaining-1]))
 			remaining--;
 		size_t component_start = remaining;
-		// strip a dir delimiter
-		while(remaining != 0 && IsDirDelimiter(path[remaining-1]))
+		// strip another delimiter
+		while (remaining > 1 && IsDirDelimiter(path[remaining-1]))
 			remaining--;
-		if(removed){
+		if (component_start == component_end)
+			break; // could not remove anything
+		if (removed) {
 			std::string component = path.substr(component_start,
 					component_end - component_start);
-			if(i)
-				*removed = component + DIR_DELIM + *removed;
-			else
-				*removed = component;
+			if (i) {
+				removed->insert(0, DIR_DELIM);
+				removed->insert(0, component);
+			} else {
+				*removed = std::move(component);
+			}
 		}
 	}
 	return path.substr(0, remaining);
@@ -804,7 +808,7 @@ std::string RemoveRelativePathComponents(std::string path)
 			while (pos != 0 && IsDirDelimiter(path[pos-1]))
 				pos--;
 			if (component_start == 0) {
-				// We need to remove the delemiter too
+				// We need to remove the delimiter too
 				path = path.substr(component_with_delim_end, std::string::npos);
 			} else {
 				path = path.substr(0, pos) + DIR_DELIM +

src/filesys.h

@@ -116,12 +116,13 @@ bool MoveDir(const std::string &source, const std::string &target);
 // Ignores case differences and '/' vs. '\\' on Windows
 bool PathStartsWith(const std::string &path, const std::string &prefix);
 
-// Remove last path component and the dir delimiter before and/or after it,
-// returns "" if there is only one path component.
-// removed: If non-NULL, receives the removed component(s).
+// Remove last path component and the dir delimiter before and/or after it.
+// If there's only one path component it will refuse to remove it (if absolute)
+// or return "" (if relative).
+// removed: If non-NULL, receives the removed components
 // count: Number of components to remove
 std::string RemoveLastPathComponent(const std::string &path,
-		std::string *removed = NULL, int count = 1);
+		std::string *removed = nullptr, int count = 1);
 
 // Remove "." and ".." path components and for every ".." removed, remove
 // the last normal path component before it. Unlike AbsolutePath,

src/unittest/test_filesys.cpp

@@ -49,6 +49,12 @@ void TestFileSys::runTests(IGameDef *gamedef)
 
 ////////////////////////////////////////////////////////////////////////////////
 
+#if defined(_WIN32)
+static constexpr bool win32 = true;
+#else
+static constexpr bool win32 = false;
+#endif
+
 // adjusts a POSIX path to system-specific conventions
 // -> changes '/' to DIR_DELIM
 // -> absolute paths start with "C:\\" on windows
@@ -61,10 +67,10 @@ static std::string p(std::string path)
 		}
 	}
 
-	#ifdef _WIN32
+#ifdef _WIN32
 	if (path[0] == '\\')
-		path = "C:" + path;
-	#endif
+		path.insert(0, "C:");
+#endif
 
 	return path;
 }
@@ -75,11 +81,7 @@ void TestFileSys::testIsDirDelimiter()
 	UASSERT(fs::IsDirDelimiter('/') == true);
 	UASSERT(fs::IsDirDelimiter('A') == false);
 	UASSERT(fs::IsDirDelimiter(0) == false);
-#ifdef _WIN32
-	UASSERT(fs::IsDirDelimiter('\\') == true);
-#else
-	UASSERT(fs::IsDirDelimiter('\\') == false);
-#endif
+	UASSERT(fs::IsDirDelimiter('\\') == win32);
 }
 
 
@@ -127,33 +129,17 @@ void TestFileSys::testPathStartsWith()
 
 	for (int i = 0; i < numpaths; i++)
 	for (int j = 0; j < numpaths; j++){
-		/*verbosestream<<"testing fs::PathStartsWith(\""
-			<<paths[i]<<"\", \""
-			<<paths[j]<<"\")"<<std::endl;*/
 		bool starts = fs::PathStartsWith(paths[i], paths[j]);
 		int expected = expected_results[i][j];
 		if(expected == 0){
 			UASSERT(starts == false);
-		}
-		else if(expected == 1){
+		} else if(expected == 1) {
 			UASSERT(starts == true);
-		}
-		#ifdef _WIN32
-		else if(expected == 2){
-			UASSERT(starts == false);
-		}
-		else if(expected == 3){
-			UASSERT(starts == true);
-		}
-		#else
-		else if(expected == 2){
-			UASSERT(starts == true);
-		}
-		else if(expected == 3){
-			UASSERT(starts == false);
-		}
-		#endif
-		else if(expected == 4){
+		} else if(expected == 2) {
+			UASSERT(starts == !win32);
+		} else if(expected == 3) {
+			UASSERT(starts == win32);
+		} else if(expected == 4) {
 			UASSERT(starts == (bool)FILESYS_CASE_INSENSITIVE);
 		}
 	}
@@ -165,6 +151,7 @@ void TestFileSys::testRemoveLastPathComponent()
 	std::string path, result, removed;
 
 	UASSERT(fs::RemoveLastPathComponent("") == "");
+
 	path = p("/home/user/minetest/bin/..//worlds/world1");
 	result = fs::RemoveLastPathComponent(path, &removed, 0);
 	UASSERT(result == path);
@@ -188,12 +175,30 @@ void TestFileSys::testRemoveLastPathComponent()
 	UASSERT(result == p("/home"));
 	UASSERT(removed == p("user/minetest/bin/../worlds/world1"));
 	result = fs::RemoveLastPathComponent(path, &removed, 7);
-#ifdef _WIN32
-	UASSERT(result == "C:");
-#else
-	UASSERT(result == "");
-#endif
+	UASSERTEQ(auto, result, win32 ? "C:" : "/");
 	UASSERT(removed == p("home/user/minetest/bin/../worlds/world1"));
+
+	path = p("./README.txt");
+	result = fs::RemoveLastPathComponent(path, &removed);
+	UASSERT(result == ".");
+	UASSERT(removed == "README.txt");
+
+#ifdef __unix__
+	path = "/README.txt";
+	result = fs::RemoveLastPathComponent(path, &removed);
+	UASSERT(result == "/");
+	UASSERT(removed == "README.txt");
+
+	path = "README.txt";
+	result = fs::RemoveLastPathComponent(path, &removed);
+	UASSERT(result == ""); // working directory
+	UASSERT(removed == "README.txt");
+
+	path = "///";
+	result = fs::RemoveLastPathComponent(path, &removed);
+	UASSERT(result == "/");
+	UASSERT(removed == "");
+#endif
 }
 
 
@@ -224,11 +229,7 @@ void TestFileSys::testRemoveLastPathComponentWithTrailingDelimiter()
 	UASSERT(result == p("/home"));
 	UASSERT(removed == p("user/minetest/bin/../worlds/world1"));
 	result = fs::RemoveLastPathComponent(path, &removed, 7);
-#ifdef _WIN32
-	UASSERT(result == "C:");
-#else
-	UASSERT(result == "");
-#endif
+	UASSERTEQ(auto, result, win32 ? "C:" : "/");
 	UASSERT(removed == p("home/user/minetest/bin/../worlds/world1"));
 }