Re: g_build_path() inconsistency

[Owen Taylor <otaylor redhat com>]

Owen Taylor <otaylor redhat com> writes:

> The other subtlety is the definition of trailing copies of the
> separator. Using the first verison of the algorithm, does:
> 
>  g_build_path ("::", "C", ":::") give 
> 
>   "C" "::" ":" => "C:::" or C "::" ":" "::" => "C::::"
> 
> I think the first one is right - that is, the number of trailing
> copies of the separator in an element is determined after stripping 
> off all leading copies of the separator.

Turns out this is wrong, since it means:

 g_build_path (":", "C", "::") give "C", not the expected "C::".

So, we need to just bite the bullet and take the funny "C::::"
above.

It turns out also, that another special case is needed:

 * However, if there is only a single non-empty element, and there
 * are no characters in not part of the leading or trailing
 * separators, then the result is exactly the original value of
 * that element.

Attached is a patch that implements my current best idea and
updates the docs and test cases. Ugh.

Regards,
                                        Owen

Index: ChangeLog
===================================================================
RCS file: /cvs/gnome/glib/ChangeLog,v
retrieving revision 1.1085.2.39
diff -u -p -r1.1085.2.39 ChangeLog
--- ChangeLog	4 Jul 2002 15:28:20 -0000	1.1085.2.39
+++ ChangeLog	25 Jul 2002 21:44:29 -0000
@@ -1,3 +1,15 @@
+Thu Jul 25 14:23:15 2002  Owen Taylor  <otaylor redhat com>
+
+	* glib/gfileutils.c: Clarify the behavior of g_build_path()
+	for empty elements and for leading and trailing copies
+	of the separator in the docs.
+
+	* glib/gfileutils.c: Fix problems with leading elements
+	consisting only of "/" characters. (#85928, Guillaume Chazarain)
+
+	* tests/strfunc-test.c (main): Add more test cases
+	for g_build_filename().
+
 2002-07-04  Sebastian Wilhelmi  <wilhelmi ira uka de>
 
 	* tests/*.c: Added #undef G_DISABLE_ASSERT and #undef G_LOG_DOMAIN
Index: glib/gfileutils.c
===================================================================
RCS file: /cvs/gnome/glib/glib/gfileutils.c,v
retrieving revision 1.32.2.1
diff -u -p -r1.32.2.1 gfileutils.c
--- glib/gfileutils.c	20 May 2002 19:37:06 -0000	1.32.2.1
+++ glib/gfileutils.c	25 Jul 2002 21:44:29 -0000
@@ -770,7 +770,10 @@ g_build_pathv (const gchar *separator,
   GString *result;
   gint separator_len = strlen (separator);
   gboolean is_first = TRUE;
+  gboolean have_leading = FALSE;
+  const gchar *single_element = NULL;
   const gchar *next_element;
+  const gchar *last_trailing = NULL;
 
   result = g_string_new (NULL);
 
@@ -790,36 +793,69 @@ g_build_pathv (const gchar *separator,
       else
 	break;
 
-      start = element;
+      /* Ignore empty elements */
+      if (!*element)
+	continue;
       
-      if (is_first)
-	is_first = FALSE;
-      else if (separator_len)
+      start = element;
+
+      if (separator_len)
 	{
 	  while (start &&
 		 strncmp (start, separator, separator_len) == 0)
 	    start += separator_len;
-	}
+      	}
 
       end = start + strlen (start);
       
-      if (next_element && separator_len)
+      if (separator_len)
 	{
-	  while (end > start + separator_len &&
+	  while (end >= start + separator_len &&
 		 strncmp (end - separator_len, separator, separator_len) == 0)
 	    end -= separator_len;
+	  
+	  last_trailing = end;
+	  while (last_trailing >= element + separator_len &&
+		 strncmp (last_trailing - separator_len, separator, separator_len) == 0)
+	    last_trailing -= separator_len;
+
+	  if (!have_leading)
+	    {
+	      /* If the leading and trailing separator strings are in the
+	       * same element and overlap, the result is exactly that element
+	       */
+	      if (last_trailing <= start)
+		single_element = element;
+		  
+	      g_string_append_len (result, element, start - element);
+	      have_leading = TRUE;
+	    }
+	  else
+	    single_element = NULL;
 	}
 
-      if (end > start)
-	{
-	  if (result->len > 0)
-	    g_string_append (result, separator);
+      if (end == start)
+	continue;
 
-	  g_string_append_len (result, start, end - start);
-	}
+      if (!is_first)
+	g_string_append (result, separator);
+      
+      g_string_append_len (result, start, end - start);
+      is_first = FALSE;
+    }
+
+  if (single_element)
+    {
+      g_string_free (result, TRUE);
+      return g_strdup (single_element);
     }
+  else
+    {
+      if (last_trailing)
+	g_string_append (result, last_trailing);
   
-  return g_string_free (result, FALSE);
+      return g_string_free (result, FALSE);
+    }
 }
 
 /**
@@ -833,6 +869,28 @@ g_build_pathv (const gchar *separator,
  * any trailing occurrences of separator in the first element, or
  * leading occurrences of separator in the second element are removed
  * and exactly one copy of the separator is inserted.
+ *
+ * Empty elements are ignored.
+ *
+ * The number of leading copies of the separator on the result is
+ * the same as the number of leading copies of the separator on
+ * the first non-empty element.
+ *
+ * The number of trailing copies of the separator on the result is
+ * the same as the number of trailing copies of the separator on
+ * the last non-empty element. (Determination of the number of
+ * trailing copies is done without stripping leading copies, so
+ * if the separator is <literal>ABA</literal>, <literal>ABABA</literal>
+ * has 1 trailing copy.)
+ *
+ * However, if there is only a single non-empty element, and there
+ * are no characters in that element not part of the leading or
+ * trailing separators, then the result is exactly the original value
+ * of that element.
+ *
+ * Other than for determination of the number of leading and trailing
+ * copies of the separator, elements consisting only of copies
+ * of the separator are ignored.
  * 
  * Return value: a newly-allocated string that must be freed with g_free().
  **/
Index: tests/strfunc-test.c
===================================================================
RCS file: /cvs/gnome/glib/tests/strfunc-test.c,v
retrieving revision 1.16.2.1
diff -u -p -r1.16.2.1 strfunc-test.c
--- tests/strfunc-test.c	4 Jul 2002 15:28:22 -0000	1.16.2.1
+++ tests/strfunc-test.c	25 Jul 2002 21:44:29 -0000
@@ -343,25 +343,50 @@ main (int   argc,
   TEST (NULL, str_check (g_build_path (":", ":", NULL), ":"));
   TEST (NULL, str_check (g_build_path (":", ":x", NULL), ":x"));
   TEST (NULL, str_check (g_build_path (":", "x:", NULL), "x:"));
+  TEST (NULL, str_check (g_build_path (":", "", "x", NULL), "x"));
+  TEST (NULL, str_check (g_build_path (":", "", ":x", NULL), ":x"));
+  TEST (NULL, str_check (g_build_path (":", ":", "x", NULL), ":x"));
+  TEST (NULL, str_check (g_build_path (":", "::", "x", NULL), "::x"));
+  TEST (NULL, str_check (g_build_path (":", "x", "", NULL), "x"));
+  TEST (NULL, str_check (g_build_path (":", "x:", "", NULL), "x:"));
+  TEST (NULL, str_check (g_build_path (":", "x", ":", NULL), "x:"));
+  TEST (NULL, str_check (g_build_path (":", "x", "::", NULL), "x::"));
   TEST (NULL, str_check (g_build_path (":", "x", "y",  NULL), "x:y"));
   TEST (NULL, str_check (g_build_path (":", ":x", "y", NULL), ":x:y"));
   TEST (NULL, str_check (g_build_path (":", "x", "y:", NULL), "x:y:"));
   TEST (NULL, str_check (g_build_path (":", ":x:", ":y:", NULL), ":x:y:"));
   TEST (NULL, str_check (g_build_path (":", ":x::", "::y:", NULL), ":x:y:"));
+  TEST (NULL, str_check (g_build_path (":", "x", "","y",  NULL), "x:y"));
+  TEST (NULL, str_check (g_build_path (":", "x", ":", "y",  NULL), "x:y"));
+  TEST (NULL, str_check (g_build_path (":", "x", "::", "y",  NULL), "x:y"));
   TEST (NULL, str_check (g_build_path (":", "x", "y", "z", NULL), "x:y:z"));
   TEST (NULL, str_check (g_build_path (":", ":x:", ":y:", ":z:", NULL), ":x:y:z:"));
   TEST (NULL, str_check (g_build_path (":", "::x::", "::y::", "::z::", NULL), "::x:y:z::"));
 
   TEST (NULL, str_check (g_build_path ("::", NULL), ""));
   TEST (NULL, str_check (g_build_path ("::", "::", NULL), "::"));
+  TEST (NULL, str_check (g_build_path ("::", ":::", NULL), ":::"));
   TEST (NULL, str_check (g_build_path ("::", "::x", NULL), "::x"));
   TEST (NULL, str_check (g_build_path ("::", "x::", NULL), "x::"));
+  TEST (NULL, str_check (g_build_path ("::", "", "x", NULL), "x"));
+  TEST (NULL, str_check (g_build_path ("::", "", "::x", NULL), "::x"));
+  TEST (NULL, str_check (g_build_path ("::", "::", "x", NULL), "::x"));
+  TEST (NULL, str_check (g_build_path ("::", "::::", "x", NULL), "::::x"));
+  TEST (NULL, str_check (g_build_path ("::", "x", "", NULL), "x"));
+  TEST (NULL, str_check (g_build_path ("::", "x::", "", NULL), "x::"));
+  TEST (NULL, str_check (g_build_path ("::", "x", "::", NULL), "x::"));
+  /* This following is weird, but keeps the definition simple */
+  TEST (NULL, str_check (g_build_path ("::", "x", ":::", NULL), "x:::::"));
+  TEST (NULL, str_check (g_build_path ("::", "x", "::::", NULL), "x::::"));
   TEST (NULL, str_check (g_build_path ("::", "x", "y",  NULL), "x::y"));
   TEST (NULL, str_check (g_build_path ("::", "::x", "y", NULL), "::x::y"));
   TEST (NULL, str_check (g_build_path ("::", "x", "y::", NULL), "x::y::"));
   TEST (NULL, str_check (g_build_path ("::", "::x::", "::y::", NULL), "::x::y::"));
   TEST (NULL, str_check (g_build_path ("::", "::x:::", ":::y::", NULL), "::x::::y::"));
   TEST (NULL, str_check (g_build_path ("::", "::x::::", "::::y::", NULL), "::x::y::"));
+  TEST (NULL, str_check (g_build_path ("::", "x", "", "y",  NULL), "x::y"));
+  TEST (NULL, str_check (g_build_path ("::", "x", "::", "y",  NULL), "x::y"));
+  TEST (NULL, str_check (g_build_path ("::", "x", "::::", "y",  NULL), "x::y"));
   TEST (NULL, str_check (g_build_path ("::", "x", "y", "z", NULL), "x::y::z"));
   TEST (NULL, str_check (g_build_path ("::", "::x::", "::y::", "::z::", NULL), "::x::y::z::"));
   TEST (NULL, str_check (g_build_path ("::", ":::x:::", ":::y:::", ":::z:::", NULL), ":::x::::y::::z:::"));
@@ -373,11 +398,22 @@ main (int   argc,
   TEST (NULL, str_check (g_build_filename (S, NULL), S));
   TEST (NULL, str_check (g_build_filename (S"x", NULL), S"x"));
   TEST (NULL, str_check (g_build_filename ("x"S, NULL), "x"S));
+  TEST (NULL, str_check (g_build_filename ("", "x", NULL), "x"));
+  TEST (NULL, str_check (g_build_filename ("", S"x", NULL), S"x"));
+  TEST (NULL, str_check (g_build_filename (S, "x", NULL), S"x"));
+  TEST (NULL, str_check (g_build_filename (S S, "x", NULL), S S"x"));
+  TEST (NULL, str_check (g_build_filename ("x", "", NULL), "x"));
+  TEST (NULL, str_check (g_build_filename ("x"S, "", NULL), "x"S));
+  TEST (NULL, str_check (g_build_filename ("x", S, NULL), "x"S));
+  TEST (NULL, str_check (g_build_filename ("x", S S, NULL), "x"S S));
   TEST (NULL, str_check (g_build_filename ("x", "y",  NULL), "x"S"y"));
   TEST (NULL, str_check (g_build_filename (S"x", "y", NULL), S"x"S"y"));
   TEST (NULL, str_check (g_build_filename ("x", "y"S, NULL), "x"S"y"S));
   TEST (NULL, str_check (g_build_filename (S"x"S, S"y"S, NULL), S"x"S"y"S));
   TEST (NULL, str_check (g_build_filename (S"x"S S, S S"y"S, NULL), S"x"S"y"S));
+  TEST (NULL, str_check (g_build_filename ("x", "", "y",  NULL), "x"S"y"));
+  TEST (NULL, str_check (g_build_filename ("x", S, "y",  NULL), "x"S"y"));
+  TEST (NULL, str_check (g_build_filename ("x", S S, "y",  NULL), "x"S"y"));
   TEST (NULL, str_check (g_build_filename ("x", "y", "z", NULL), "x"S"y"S"z"));
   TEST (NULL, str_check (g_build_filename (S"x"S, S"y"S, S"z"S, NULL), S"x"S"y"S"z"S));
   TEST (NULL, str_check (g_build_filename (S S"x"S S, S S"y"S S, S S"z"S S, NULL), S S"x"S"y"S"z"S S));