Avoid duplication of / in joinPaths (Windows) (#201)

* Avoid duplication of / in joinPaths (Windows)

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Remove / in Linux and MacOS

Signed-off-by: ahcorde <ahcorde@gmail.com>

* add test and doc

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

* Fixed test

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Improved joinPaths method

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Add quick fix

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Add quick fix

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Make linters happy

Signed-off-by: ahcorde <ahcorde@gmail.com>

Co-authored-by: Louise Poubel <louise@openrobotics.org>

include/ignition/common/Filesystem.hh

@@ -126,7 +126,8 @@ namespace ignition
     /// \param[in] _path1 the left portion of the path
     /// \param[in] _path2 the right portion of the path
     /// \return Joined path. The function can do simplifications such as
-    /// elimination of ../ (but is not guaranteed to do so).
+    /// elimination of ../ and removal of duplicate // (but is not guaranteed to
+    /// do so).
     std::string IGNITION_COMMON_VISIBLE joinPaths(const std::string &_path1,
                                                   const std::string &_path2);
 

src/Filesystem.cc

@@ -257,19 +257,40 @@ std::string checkWindowsPath(const std::string _path)
 std::string ignition::common::joinPaths(const std::string &_path1,
                                         const std::string &_path2)
 {
+  // avoid duplicated '/' at the beginning/end of the string
+  auto sanitizeSlashes = [](const std::string &_path, bool is_windows = false)
+  {
+    std::string result = _path;
+    if (is_windows && !result.empty() &&
+      (result[0] == '\\' || result[0] == '/'))
+    {
+        result.erase(0, 1);
+    }
+    if (!result.empty() &&
+      (result[result.length()-1] == '/' || result[result.length()-1] == '\\'))
+    {
+        result.erase(result.length()-1, 1);
+    }
+    return result;
+  };
+  std::string path;
 #ifndef _WIN32
-  return separator(_path1) + _path2;
+  path = sanitizeSlashes(separator(sanitizeSlashes(_path1))
+    + sanitizeSlashes(_path2));
 #else  // _WIN32
   // std::string path1 = checkWindowsPath(_path1);
   // std::string path2 = checkWindowsPath(_path2);
   // +1 for directory separator, +1 for the ending \0 character
-  std::vector<CHAR> combined(_path1.length() + _path2.length() + 2);
+  std::string path1 = sanitizeSlashes(_path1, true);
+  std::string path2 = sanitizeSlashes(_path2, true);
+  std::vector<CHAR> combined(path1.length() + path2.length() + 2);
   // TODO(anyone): Switch to PathAllocCombine once switched to wide strings
-  if (::PathCombineA(combined.data(), _path1.c_str(), _path2.c_str()) != NULL)
-    return checkWindowsPath(std::string(combined.data()));
+  if (::PathCombineA(combined.data(), path1.c_str(), path2.c_str()) != NULL)
+    path = sanitizeSlashes(checkWindowsPath(std::string(combined.data())));
   else
-    return checkWindowsPath(separator(_path1) + _path1);
+    path = sanitizeSlashes(checkWindowsPath(separator(path1) + path2));
 #endif  // _WIN32
+  return path;
 }
 
 /////////////////////////////////////////////////

src/Filesystem_TEST.cc

@@ -408,6 +408,14 @@ TEST_F(FilesystemTest, append)
   EXPECT_EQ(path, separator("tmp") +
             separator("hello") +
             separator("there") + "again");
+
+  path = joinPaths("base", "/before", "after/");
+
+#ifndef _WIN32
+  EXPECT_EQ(path, "base//before/after");
+#else
+  EXPECT_EQ(path, "base\\before\\after");
+#endif
 }
 
 /////////////////////////////////////////////////