From 8b0a0b94dbace3e551a9e3886147436a7a5d0d66 Mon Sep 17 00:00:00 2001
From: Jonathan Laroche <jonathan.laroche@autodesk.com>
Date: Thu, 12 Dec 2024 12:18:22 -0500
Subject: [PATCH] Fix for not mixing separators and added tests

---
 src/OpenColorIO/ParseUtils.cpp | 49 +++++++++++++++++++++++-----------
 tests/cpu/ParseUtils_tests.cpp | 28 ++++++++++---------
 2 files changed, 48 insertions(+), 29 deletions(-)

diff --git a/src/OpenColorIO/ParseUtils.cpp b/src/OpenColorIO/ParseUtils.cpp
index 61875f76a..b25288403 100644
--- a/src/OpenColorIO/ParseUtils.cpp
+++ b/src/OpenColorIO/ParseUtils.cpp
@@ -693,15 +693,18 @@ bool StrEqualsCaseIgnore(const std::string & a, const std::string & b)
 // Find the end of a name from a list contained in a string.
 // The element of the list are separeted by ":" or ",".
 // The name can be surrounded by quotes to enable name including theses symbols.
-static int findEndOfName(const std::string & s, size_t start)
+static int findEndOfName(const std::string & s, size_t start, char sep)
 {
     int currentPos = start;
     int nameEndPos = currentPos;
     bool isEndFound = false;
     
+    std::string symbols = "\"";
+    symbols += sep;
+
     while( !isEndFound )
     {
-        nameEndPos = s.find_first_of("\",:", currentPos);
+        nameEndPos = s.find_first_of(symbols, currentPos);
         if(nameEndPos == (int)std::string::npos) 
         {
             // We reached the end of the list
@@ -726,8 +729,7 @@ static int findEndOfName(const std::string & s, size_t start)
                 currentPos = nameEndPos + 1;
             }
         }
-        else if( s[nameEndPos] == ',' || 
-                 s[nameEndPos] == ':' )
+        else if( s[nameEndPos] == sep )
         {
             // We found a symbol separating the elements, we stop here
             isEndFound = true;
@@ -746,22 +748,37 @@ StringUtils::StringVec SplitStringEnvStyle(const std::string & str)
     }
 
     StringUtils::StringVec outputvec;
-    int currentPos = 0;
-    while( s.size() > 0 && 
-           currentPos <= (int) s.size() )
+    auto foundComma = s.find_first_of(",");
+    auto foundColon = s.find_first_of(":");
+
+    if( foundComma != std::string::npos || 
+        foundColon != std::string::npos )
     {
-        int nameEndPos = findEndOfName(s, currentPos);
-        if(nameEndPos > currentPos)
+        int currentPos = 0;
+        while( s.size() > 0 && 
+               currentPos <= (int) s.size() )
         {
-            outputvec.push_back(s.substr(currentPos, nameEndPos - currentPos));
-            currentPos = nameEndPos + 1;
-        }
-        else
-        {
-            outputvec.push_back("");
-            currentPos += 1;
+            int nameEndPos = findEndOfName( s, 
+                                            currentPos, 
+                                            foundComma != std::string::npos ? ',' : ':' );
+            if(nameEndPos > currentPos)
+            {
+                outputvec.push_back(s.substr(currentPos, nameEndPos - currentPos));
+                currentPos = nameEndPos + 1;
+            }
+            else
+            {
+                outputvec.push_back("");
+                currentPos += 1;
+            }
         }
     }
+    else
+    {
+        // If there is no colon either, 
+        // we consider the string as a single element
+        outputvec.push_back(s);
+    }
 
     for ( auto & val : outputvec )
     {
diff --git a/tests/cpu/ParseUtils_tests.cpp b/tests/cpu/ParseUtils_tests.cpp
index dfe53792c..3b1045002 100644
--- a/tests/cpu/ParseUtils_tests.cpp
+++ b/tests/cpu/ParseUtils_tests.cpp
@@ -417,11 +417,9 @@ OCIO_ADD_TEST(ParseUtils, split_string_env_style)
     outputvec.clear();
 
     outputvec = OCIO::SplitStringEnvStyle("This:is   ,   a:test  ");
-    OCIO_CHECK_EQUAL(4, outputvec.size());
-    OCIO_CHECK_EQUAL("This", outputvec[0]);
-    OCIO_CHECK_EQUAL("is", outputvec[1]);
-    OCIO_CHECK_EQUAL("a", outputvec[2]);
-    OCIO_CHECK_EQUAL("test", outputvec[3]);
+    OCIO_CHECK_EQUAL(2, outputvec.size());
+    OCIO_CHECK_EQUAL("This:is", outputvec[0]);
+    OCIO_CHECK_EQUAL("a:test", outputvec[1]);
     outputvec.clear();
 
     outputvec = OCIO::SplitStringEnvStyle(",,");
@@ -452,16 +450,13 @@ OCIO_ADD_TEST(ParseUtils, split_string_env_style)
     OCIO_CHECK_EQUAL("test", outputvec[2]);
 
     outputvec = OCIO::SplitStringEnvStyle("   \"This  , is   \":   a:   test  ");
-    OCIO_CHECK_EQUAL(3, outputvec.size());
-    OCIO_CHECK_EQUAL("This  , is   ", outputvec[0]);
-    OCIO_CHECK_EQUAL("a", outputvec[1]);
-    OCIO_CHECK_EQUAL("test", outputvec[2]);
+    OCIO_CHECK_EQUAL(1, outputvec.size());
+    OCIO_CHECK_EQUAL("\"This  , is   \":   a:   test", outputvec[0]);
 
     outputvec = OCIO::SplitStringEnvStyle("   \"This  , is   \":   a,   test  ");
-    OCIO_CHECK_EQUAL(3, outputvec.size());
-    OCIO_CHECK_EQUAL("This  , is   ", outputvec[0]);
-    OCIO_CHECK_EQUAL("a", outputvec[1]);
-    OCIO_CHECK_EQUAL("test", outputvec[2]);
+    OCIO_CHECK_EQUAL(2, outputvec.size());
+    OCIO_CHECK_EQUAL("\"This  , is   \":   a", outputvec[0]);
+    OCIO_CHECK_EQUAL("test", outputvec[1]);
 
     outputvec = OCIO::SplitStringEnvStyle("   This  : is   \":   a:   test  \"");
     OCIO_CHECK_EQUAL(2, outputvec.size());
@@ -497,6 +492,13 @@ OCIO_ADD_TEST(ParseUtils, join_string_env_style)
 
     outputvec = { "This", "is   \":   a:   test" };
     OCIO_CHECK_EQUAL( "This, \"is   \":   a:   test\"", OCIO::JoinStringEnvStyle(outputvec) );
+
+    outputvec = { "This, is, a, string", "this, one, too" };
+    OCIO_CHECK_EQUAL( "\"This, is, a, string\", \"this, one, too\"" , OCIO::JoinStringEnvStyle(outputvec) );
+
+    outputvec = { "This", "is: ", "\"a very good,\"", " fine, helpful, and useful ", "test" };
+    OCIO_CHECK_EQUAL( "This, \"is: \", \"a very good,\", \" fine, helpful, and useful \", test", 
+                      OCIO::JoinStringEnvStyle(outputvec) );
 }
 
 OCIO_ADD_TEST(ParseUtils, intersect_string_vecs_case_ignore)