From b960986d612f3c5ddd30e47cdc8d2b9450ba9220 Mon Sep 17 00:00:00 2001
From: Michael Carroll <michael@openrobotics.org>
Date: Fri, 4 Mar 2022 18:20:56 -0600
Subject: [PATCH] Fix filesystem::separator, parentPath and basename behavior
 (#308)

* Separator with an emptry string argument should return the preferred
separator for that platform
* Basename should return basename in the case with multiple trailing
slashes

Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>

Co-authored-by: Louise Poubel <louise@openrobotics.org>
---
 src/Filesystem.cc      | 33 +++++++++++++++++--------------
 src/Filesystem_TEST.cc | 45 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 62 insertions(+), 16 deletions(-)

diff --git a/src/Filesystem.cc b/src/Filesystem.cc
index 99cc9094..2b1b26f6 100644
--- a/src/Filesystem.cc
+++ b/src/Filesystem.cc
@@ -99,8 +99,7 @@ bool ignition::common::createDirectories(const std::string &_path)
 /////////////////////////////////////////////////
 std::string const ignition::common::separator(std::string const &_s)
 {
-  fs::path path(_s);
-  return (_s / fs::path("")).string();
+  return _s + std::string{fs::path::preferred_separator};
 }
 
 /////////////////////////////////////////////////
@@ -144,6 +143,11 @@ std::string ignition::common::joinPaths(
   fs::path p1{_path1};
   fs::path p2{_path2};
 
+  if (p1.empty())
+  {
+    p1 = ignition::common::separator("");
+  }
+
   bool is_url = false;
 
   if (_path1.find("://") == std::string::npos)
@@ -202,24 +206,23 @@ bool ignition::common::chdir(const std::string &_dir)
 std::string ignition::common::basename(const std::string &_path)
 {
   fs::path p(_path);
-  // Maintain compatibility with ign-common
-  if (*_path.rbegin() == fs::path::preferred_separator)
-    p = fs::path(_path.substr(0, _path.size()-1));
-  return p.filename().string();
+  p /= "FOO.TXT";
+  p = p.lexically_normal();
+  p = p.parent_path();
+  return p.filename().string().empty() ?
+    std::string{fs::path::preferred_separator} : p.filename().string();
 }
 
 /////////////////////////////////////////////////
 std::string ignition::common::parentPath(const std::string &_path)
 {
-  fs::path p(_path);
-  // Maintain compatibility with ign-common
-  if (*_path.rbegin() == fs::path::preferred_separator)
-    p = fs::path(_path.substr(0, _path.size()-1));
-
-  if (!p.has_parent_path())
-    return _path;
-
-  return p.parent_path().string();
+  std::string result;
+  size_t last_sep = _path.find_last_of(separator(""));
+  // If slash is the last character, find its parent directory
+  if (last_sep == _path.length() - 1)
+    last_sep = _path.substr(0, last_sep).find_last_of(separator(""));
+  result = _path.substr(0, last_sep);
+  return result;
 }
 
 /////////////////////////////////////////////////
diff --git a/src/Filesystem_TEST.cc b/src/Filesystem_TEST.cc
index ebe39d69..2fce0275 100644
--- a/src/Filesystem_TEST.cc
+++ b/src/Filesystem_TEST.cc
@@ -413,38 +413,71 @@ TEST_F(FilesystemTest, IGN_UTILS_TEST_DISABLED_ON_WIN32(cwd_error))
 }
 
 /////////////////////////////////////////////////
-TEST_F(FilesystemTest, basename)
+TEST_F(FilesystemTest, decomposition)
 {
   std::string absolute = joinPaths("", "home", "bob", "foo");
   EXPECT_EQ(basename(absolute), "foo");
+#ifndef _WIN32
+  EXPECT_EQ(parentPath(absolute), "/home/bob");
+#else
+  EXPECT_EQ(parentPath(absolute), "\\home\\bob");
+#endif
 
   std::string relative = joinPaths("baz", "foobar");
   EXPECT_EQ(basename(relative), "foobar");
+  EXPECT_EQ(parentPath(relative), "baz");
 
   std::string bname = "bzzz";
   EXPECT_EQ(basename(bname), "bzzz");
+  EXPECT_EQ(parentPath(bname), "bzzz");
 
   std::string nobase = joinPaths("baz", "");
   EXPECT_EQ(basename(nobase), "baz");
+#ifndef _WIN32
+  EXPECT_EQ(parentPath(nobase), "baz/");
+#else
+  EXPECT_EQ(parentPath(nobase), "baz\\");
+#endif
 
   std::string multiple_slash = separator("baz") + separator("") + separator("")
     + separator("");
   EXPECT_EQ(basename(multiple_slash), "baz");
+#ifndef _WIN32
+  EXPECT_EQ(parentPath(multiple_slash), "baz//");
+#else
+  EXPECT_EQ(parentPath(multiple_slash), "baz\\\\");
+#endif
 
   std::string multiple_slash_middle = separator("") + separator("home")
     + separator("") + separator("") + separator("bob") + separator("foo");
   EXPECT_EQ(basename(multiple_slash_middle), "foo");
+#ifndef _WIN32
+  EXPECT_EQ(parentPath(multiple_slash_middle), "/home///bob");
+#else
+  EXPECT_EQ(parentPath(multiple_slash_middle), "\\home\\\\\\bob");
+#endif
 
   std::string multiple_slash_start = separator("") + separator("")
     + separator("") + separator("home") + separator("bob") + separator("foo");
   EXPECT_EQ(basename(multiple_slash_start), "foo");
+#ifndef _WIN32
+  EXPECT_EQ(parentPath(multiple_slash_start), "///home/bob");
+#else
+  EXPECT_EQ(parentPath(multiple_slash_start), "\\\\\\home\\bob");
+#endif
 
   std::string slash_only = separator("") + separator("");
   EXPECT_EQ(basename(slash_only), separator(""));
+  EXPECT_EQ(parentPath(slash_only), "");
 
   std::string multiple_slash_only = separator("") + separator("")
     + separator("") + separator("");
   EXPECT_EQ(basename(multiple_slash_only), separator(""));
+#ifndef _WIN32
+  EXPECT_EQ(parentPath(multiple_slash_only), "//");
+#else
+  EXPECT_EQ(parentPath(multiple_slash_only), "\\\\");
+#endif
 }
 
 /////////////////////////////////////////////////
@@ -611,6 +644,16 @@ TEST_F(FilesystemTest, uniquePaths)
   EXPECT_TRUE(removeDirectory(dirExistingRt)) << dirExistingRt;
 }
 
+/////////////////////////////////////////////////
+TEST_F(FilesystemTest, separator)
+{
+#ifndef _WIN32
+  EXPECT_EQ("/", ignition::common::separator(""));
+#else
+  EXPECT_EQ("\\", ignition::common::separator(""));
+#endif
+}
+
 /////////////////////////////////////////////////
 /// Main
 int main(int argc, char **argv)