removal of duplicated code in gnome-vfs-uri

[Christophe Fergeau <teuf users sourceforge net>]

Hi,

The attached patch removes lots of duplicated code from
gnome-vfs-utils.c and gnome-vfs-uri.c. It changes
gnome_vfs_uri_make_full_from_relative,
gnome_vfs_make_uri_full_from_relative and
gnome_vfs_uri_resolve_relative.

gnome_vfs_make_uri_full_from_relative is deprecated and calls
gnome_vfs_uri_make_full_from_relative (both contained the exact same
code).
I also noticed that gnome_vfs_uri_make_full_from_relative and
gnome_vfs_uri_resolve_relative were doing mostly the same thing, the
only difference being that gnome_vfs_uri_resolve_relative takes a
GnomeVFSURI, and the other takes a string as parameter. Since they were
duplicating code doing the same thing, I factored it in
make_full_uri_from_relative. There is still a slight difference between
the 2 functions: gnome_vfs_uri_resolve_relative will return NULL if the
relative_uri argument is NULL, while
gnome_vfs_uri_make_full_from_relative will return the base_uri arg. The
gnome_vfs_uri_resolve_relative behaviour seems flawed in that case, I'd
like to change it to behave the same as
gnome_vfs_uri_make_full_from_relative (that would also allow to
factorize some more code).

For now, the attached patch shouldn't modify at all the behaviour of
these 3 functions. If something changed, that's unexpected and that's a
bug. I'd prefer to commit this patch first, it only moves some code so
it should be safe, but it's quite big, so I'm not really comfortable
with it, a review would be nice if someone has some time :)

Then I'll change gnome_vfs_uri_resolve_relative behaviour if we agree
that it's a bit buggy, and I'll try to fix #105094. One possibility to
fix this bug is to remove the make_full_from_relative code from
gnome-vfs and to use xmlBuildURI from libxml2 which implements section
5.2 of the RFC 2396 as does make_full_from_relative. However, that
change would need to be carefully tested since gnome-vfs parsing code
may not be 100% spec-compliant, and some apps may rely on that, so a
less invasive fix would probably be better :)

Christophe


? diff
? duplicated.diff
? test1
? test2
? test3
? uri
? util
Index: gnome-vfs-uri.c
===================================================================
RCS file: /cvs/gnome/gnome-vfs/libgnomevfs/gnome-vfs-uri.c,v
retrieving revision 1.110
diff -u -r1.110 gnome-vfs-uri.c
--- gnome-vfs-uri.c	7 Jan 2003 15:11:20 -0000	1.110
+++ gnome-vfs-uri.c	7 Feb 2003 12:39:35 -0000
@@ -577,6 +577,7 @@
 	return  !(':' == *current);
 }
 
+
 /*
  * Remove "./" segments
  * Compact "../" segments inside the URI
@@ -668,134 +669,119 @@
 {
 	char *result = NULL;
 
-	g_return_val_if_fail (base_uri != NULL, g_strdup (uri));
-	g_return_val_if_fail (uri != NULL, NULL);
-
-	/* See section 5.2 in RFC 2396 */
-
-	/* FIXME bugzilla.eazel.com 4413: This function does not take
-	 * into account a BASE tag in an HTML document, so its
-	 * functionality differs from what Mozilla itself would do.
+	char *mutable_base_uri;
+	char *mutable_uri;
+	
+	char *uri_current;
+	gsize base_uri_length;
+	char *separator;
+	
+	/* We may need one extra character
+	 * to append a "/" to uri's that have no "/"
+	 * (such as help:)
 	 */
 
-	if (is_uri_relative (uri)) {
-		char *mutable_base_uri;
-		char *mutable_uri;
-
-		char *uri_current;
-		gsize base_uri_length;
-		char *separator;
-
-		/* We may need one extra character
-		 * to append a "/" to uri's that have no "/"
-		 * (such as help:)
-		 */
-
-		mutable_base_uri = g_malloc(strlen(base_uri)+2);
-		strcpy (mutable_base_uri, base_uri);
+	mutable_base_uri = g_malloc(strlen(base_uri)+2);
+	strcpy (mutable_base_uri, base_uri);
 		
-		uri_current = mutable_uri = g_strdup (uri);
+	uri_current = mutable_uri = g_strdup (uri);
 
-		/* Chew off Fragment and Query from the base_url */
+	/* Chew off Fragment and Query from the base_url */
 
-		separator = strrchr (mutable_base_uri, '#'); 
+	separator = strrchr (mutable_base_uri, '#'); 
 
-		if (separator) {
-			*separator = '\0';
-		}
+	if (separator) {
+		*separator = '\0';
+	}
 
-		separator = strrchr (mutable_base_uri, '?');
+	separator = strrchr (mutable_base_uri, '?');
 
-		if (separator) {
-			*separator = '\0';
-		}
+	if (separator) {
+		*separator = '\0';
+	}
 
-		if ('/' == uri_current[0] && '/' == uri_current [1]) {
-			/* Relative URI's beginning with the authority
-			 * component inherit only the scheme from their parents
-			 */
+	if ('/' == uri_current[0] && '/' == uri_current [1]) {
+		/* Relative URI's beginning with the authority
+		 * component inherit only the scheme from their parents
+		 */
 
-			separator = strchr (mutable_base_uri, ':');
+		separator = strchr (mutable_base_uri, ':');
 
-			if (separator) {
-				separator[1] = '\0';
-			}			  
-		} else if ('/' == uri_current[0]) {
-			/* Relative URI's beginning with '/' absolute-path based
-			 * at the root of the base uri
-			 */
+		if (separator) {
+			separator[1] = '\0';
+		}			  
+	} else if ('/' == uri_current[0]) {
+		/* Relative URI's beginning with '/' absolute-path based
+		 * at the root of the base uri
+		 */
 
-			separator = strchr (mutable_base_uri, ':');
+		separator = strchr (mutable_base_uri, ':');
 
-			/* g_assert (separator), really */
-			if (separator) {
-				/* If we start with //, skip past the authority section */
-				if ('/' == separator[1] && '/' == separator[2]) {
-					separator = strchr (separator + 3, '/');
-					if (separator) {
-						separator[0] = '\0';
-					}
-				} else {
-				/* If there's no //, just assume the scheme is the root */
-					separator[1] = '\0';
+		/* g_assert (separator), really */
+		if (separator) {
+			/* If we start with //, skip past the authority section */
+			if ('/' == separator[1] && '/' == separator[2]) {
+				separator = strchr (separator + 3, '/');
+				if (separator) {
+					separator[0] = '\0';
 				}
+			} else {
+				/* If there's no //, just assume the scheme is the root */
+				separator[1] = '\0';
 			}
-		} else if ('#' != uri_current[0]) {
-			/* Handle the ".." convention for relative uri's */
+		}
+	} else if ('#' != uri_current[0]) {
+		/* Handle the ".." convention for relative uri's */
 
-			/* If there's a trailing '/' on base_url, treat base_url
-			 * as a directory path.
-			 * Otherwise, treat it as a file path, and chop off the filename
-			 */
+		/* If there's a trailing '/' on base_url, treat base_url
+		 * as a directory path.
+		 * Otherwise, treat it as a file path, and chop off the filename
+		 */
 
-			base_uri_length = strlen (mutable_base_uri);
-			if ('/' == mutable_base_uri[base_uri_length-1]) {
-				/* Trim off '/' for the operation below */
-				mutable_base_uri[base_uri_length-1] = 0;
-			} else {
-				separator = strrchr (mutable_base_uri, '/');
-				if (separator) {
-					*separator = '\0';
-				}
+		base_uri_length = strlen (mutable_base_uri);
+		if ('/' == mutable_base_uri[base_uri_length-1]) {
+			/* Trim off '/' for the operation below */
+			mutable_base_uri[base_uri_length-1] = 0;
+		} else {
+			separator = strrchr (mutable_base_uri, '/');
+			if (separator) {
+				*separator = '\0';
 			}
+		}
 
-			remove_internal_relative_components (uri_current);
+		remove_internal_relative_components (uri_current);
 
-			/* handle the "../"'s at the beginning of the relative URI */
-			while (0 == strncmp ("../", uri_current, 3)) {
-				uri_current += 3;
-				separator = strrchr (mutable_base_uri, '/');
-				if (separator) {
-					*separator = '\0';
-				} else {
-					/* <shrug> */
-					break;
-				}
+		/* handle the "../"'s at the beginning of the relative URI */
+		while (0 == strncmp ("../", uri_current, 3)) {
+			uri_current += 3;
+			separator = strrchr (mutable_base_uri, '/');
+			if (separator) {
+				*separator = '\0';
+			} else {
+				/* <shrug> */
+				break;
 			}
+		}
 
-			/* handle a ".." at the end */
-			if (uri_current[0] == '.' && uri_current[1] == '.' 
-			    && uri_current[2] == '\0') {
+		/* handle a ".." at the end */
+		if (uri_current[0] == '.' && uri_current[1] == '.' 
+		    && uri_current[2] == '\0') {
 
-			    	uri_current += 2;
-				separator = strrchr (mutable_base_uri, '/');
-				if (separator) {
-					*separator = '\0';
-				}
+			uri_current += 2;
+			separator = strrchr (mutable_base_uri, '/');
+			if (separator) {
+				*separator = '\0';
 			}
-
-			/* Re-append the '/' */
-			mutable_base_uri [strlen(mutable_base_uri)+1] = '\0';
-			mutable_base_uri [strlen(mutable_base_uri)] = '/';
 		}
 
-		result = g_strconcat (mutable_base_uri, uri_current, NULL);
-		g_free (mutable_base_uri); 
-		g_free (mutable_uri); 
-
-	} else {
-		result = g_strdup (uri);
+		/* Re-append the '/' */
+		mutable_base_uri [strlen(mutable_base_uri)+1] = '\0';
+		mutable_base_uri [strlen(mutable_base_uri)] = '/';
 	}
+
+	result = g_strconcat (mutable_base_uri, uri_current, NULL);
+	g_free (mutable_base_uri); 
+	g_free (mutable_uri); 
 	
 	return result;
 }
@@ -822,8 +808,16 @@
 	} else {
 		text_base = gnome_vfs_uri_to_string (base, 0);
 	}
-	text_new = make_full_uri_from_relative (text_base, relative_reference);
 
+	if (relative_reference == NULL) {
+		text_new = NULL;
+	} else if (is_uri_relative (relative_reference)) {
+		text_new = make_full_uri_from_relative (text_base, 
+							relative_reference);
+	} else {
+		text_new = g_strdup (relative_reference);
+	}
+	
 	uri = gnome_vfs_uri_new (text_new);
 
 	g_free (text_base);
@@ -1882,26 +1876,6 @@
 	g_list_free (gnome_vfs_uri_list_unref (list));
 }
 
-
-static gboolean
-is_uri_partial (const char *uri)
-{
-	const char *current;
-
-	/* RFC 2396 section 3.1 */
-	for (current = uri ; 
-		*current
-		&& 	((*current >= 'a' && *current <= 'z')
-			 || (*current >= 'A' && *current <= 'Z')
-			 || (*current >= '0' && *current <= '9')
-			 || ('-' == *current)
-			 || ('+' == *current)
-			 || ('.' == *current)) ;
-	     current++);
-
-	return  !(':' == *current);
-}
-
 /**
  * gnome_vfs_uri_make_full_from_relative:
  * @base_uri: a string representing the base URI
@@ -1926,115 +1900,10 @@
 		result = g_strdup (relative_uri);
 	} else if (relative_uri == NULL) {
 		result = g_strdup (base_uri);
-	} else if (!is_uri_partial (relative_uri)) {
-		result = g_strdup (relative_uri);
+	} else if (is_uri_relative (relative_uri)) {
+		result = make_full_uri_from_relative (base_uri, relative_uri);
 	} else {
-		char *mutable_base_uri;
-		char *mutable_uri;
-
-		char *uri_current;
-		size_t base_uri_length;
-		char *separator;
-
-		mutable_base_uri = g_strdup (base_uri);
-		uri_current = mutable_uri = g_strdup (relative_uri);
-
-		/* Chew off Fragment and Query from the base_url */
-
-		separator = strrchr (mutable_base_uri, '#'); 
-
-		if (separator) {
-			*separator = '\0';
-		}
-
-		separator = strrchr (mutable_base_uri, '?');
-
-		if (separator) {
-			*separator = '\0';
-		}
-
-		if ('/' == uri_current[0] && '/' == uri_current [1]) {
-			/* Relative URI's beginning with the authority
-			 * component inherit only the scheme from their parents
-			 */
-
-			separator = strchr (mutable_base_uri, ':');
-
-			if (separator) {
-				separator[1] = '\0';
-			}			  
-		} else if ('/' == uri_current[0]) {
-			/* Relative URI's beginning with '/' absolute-path based
-			 * at the root of the base uri
-			 */
-
-			separator = strchr (mutable_base_uri, ':');
-
-			/* g_assert (separator), really */
-			if (separator) {
-				/* If we start with //, skip past the authority section */
-				if ('/' == separator[1] && '/' == separator[2]) {
-					separator = strchr (separator + 3, '/');
-					if (separator) {
-						separator[0] = '\0';
-					}
-				} else {
-				/* If there's no //, just assume the scheme is the root */
-					separator[1] = '\0';
-				}
-			}
-		} else if ('#' != uri_current[0]) {
-			/* Handle the ".." convention for relative uri's */
-
-			/* If there's a trailing '/' on base_url, treat base_url
-			 * as a directory path.
-			 * Otherwise, treat it as a file path, and chop off the filename
-			 */
-
-			base_uri_length = strlen (mutable_base_uri);
-			if ('/' == mutable_base_uri[base_uri_length-1]) {
-				/* Trim off '/' for the operation below */
-				mutable_base_uri[base_uri_length-1] = 0;
-			} else {
-				separator = strrchr (mutable_base_uri, '/');
-				if (separator) {
-					*separator = '\0';
-				}
-			}
-
-			remove_internal_relative_components (uri_current);
-
-			/* handle the "../"'s at the beginning of the relative URI */
-			while (0 == strncmp ("../", uri_current, 3)) {
-				uri_current += 3;
-				separator = strrchr (mutable_base_uri, '/');
-				if (separator) {
-					*separator = '\0';
-				} else {
-					/* <shrug> */
-					break;
-				}
-			}
-
-			/* handle a ".." at the end */
-			if (uri_current[0] == '.' && uri_current[1] == '.' 
-			    && uri_current[2] == '\0') {
-
-			    	uri_current += 2;
-				separator = strrchr (mutable_base_uri, '/');
-				if (separator) {
-					*separator = '\0';
-				}
-			}
-
-			/* Re-append the '/' */
-			mutable_base_uri [strlen(mutable_base_uri)+1] = '\0';
-			mutable_base_uri [strlen(mutable_base_uri)] = '/';
-		}
-
-		result = g_strconcat (mutable_base_uri, uri_current, NULL);
-		g_free (mutable_base_uri); 
-		g_free (mutable_uri); 
+		result = g_strdup (relative_uri);
 	}
 	
 	return result;
Index: gnome-vfs-utils.c
===================================================================
RCS file: /cvs/gnome/gnome-vfs/libgnomevfs/gnome-vfs-utils.c,v
retrieving revision 1.61
diff -u -r1.61 gnome-vfs-utils.c
--- gnome-vfs-utils.c	19 Dec 2002 17:57:19 -0000	1.61
+++ gnome-vfs-utils.c	7 Feb 2003 12:39:36 -0000
@@ -1670,7 +1670,7 @@
 
 	location_escaped = gnome_vfs_escape_path_string (location);
 
-	uri = gnome_vfs_make_uri_full_from_relative (base_uri_slash, location_escaped);
+	uri = gnome_vfs_uri_make_full_from_relative (base_uri_slash, location_escaped);
 
 	g_free (location_escaped);
 	g_free (base_uri_slash);
@@ -1720,101 +1720,7 @@
 	return uri;
 }
 
-static gboolean
-is_uri_partial (const char *uri)
-{
-	const char *current;
-
-	/* RFC 2396 section 3.1 */
-	for (current = uri ; 
-		*current
-		&& 	((*current >= 'a' && *current <= 'z')
-			 || (*current >= 'A' && *current <= 'Z')
-			 || (*current >= '0' && *current <= '9')
-			 || ('-' == *current)
-			 || ('+' == *current)
-			 || ('.' == *current)) ;
-	     current++);
-
-	return  !(':' == *current);
-}
-
-/*
- * FIXME this is not the simplest or most time-efficent way
- * to do this.  Probably a far more clear way of doing this processing
- * is to split the path into segments, rather than doing the processing
- * in place.
- */
-static void
-remove_internal_relative_components (char *uri_current)
-{
-	char *segment_prev, *segment_cur;
-	size_t len_prev, len_cur;
-
-	len_prev = len_cur = 0;
-	segment_prev = NULL;
-
-	g_return_if_fail (uri_current != NULL);
-
-	segment_cur = uri_current;
-
-	while (*segment_cur) {
-		len_cur = strcspn (segment_cur, "/");
-
-		if (len_cur == 1 && segment_cur[0] == '.') {
-			/* Remove "." 's */
-			if (segment_cur[1] == '\0') {
-				segment_cur[0] = '\0';
-				break;
-			} else {
-				memmove (segment_cur, segment_cur + 2, strlen (segment_cur + 2) + 1);
-				continue;
-			}
-		} else if (len_cur == 2 && segment_cur[0] == '.' && segment_cur[1] == '.' ) {
-			/* Remove ".."'s (and the component to the left of it) that aren't at the
-			 * beginning or to the right of other ..'s
-			 */
-			if (segment_prev) {
-				if (! (len_prev == 2
-				       && segment_prev[0] == '.'
-				       && segment_prev[1] == '.')) {
-				       	if (segment_cur[2] == '\0') {
-						segment_prev[0] = '\0';
-						break;
-				       	} else {
-						memmove (segment_prev, segment_cur + 3, strlen (segment_cur + 3) + 1);
-
-						segment_cur = segment_prev;
-						len_cur = len_prev;
-
-						/* now we find the previous segment_prev */
-						if (segment_prev == uri_current) {
-							segment_prev = NULL;
-						} else if (segment_prev - uri_current >= 2) {
-							segment_prev -= 2;
-							for ( ; segment_prev > uri_current && segment_prev[0] != '/' 
-							      ; segment_prev-- );
-							if (segment_prev[0] == '/') {
-								segment_prev++;
-							}
-						}
-						continue;
-					}
-				}
-			}
-		}
-
-		/*Forward to next segment */
-
-		if (segment_cur [len_cur] == '\0') {
-			break;
-		}
-		 
-		segment_prev = segment_cur;
-		len_prev = len_cur;
-		segment_cur += len_cur + 1;	
-	}	
-}
+#ifndef GNOME_VFS_DISABLE_DEPRECATED
 
 /**
  * gnome_vfs_make_uri_full_from_relative:
@@ -1822,6 +1728,9 @@
  * Returns a full URI given a full base URI, and a secondary URI which may
  * be relative.
  *
+ * This function is deprecated, please use 
+ * gnome_vfs_uri_make_full_from_relative from gnome-vfs-uri.h
+ *
  * Return value: the URI (NULL for some bad errors).
  *
  * Since: 2.2
@@ -1830,129 +1739,10 @@
 char *
 gnome_vfs_make_uri_full_from_relative (const char *base_uri, const char *relative_uri)
 {
-	char *result = NULL;
-
-	/* See section 5.2 in RFC 2396 */
-
-	if (base_uri == NULL && relative_uri == NULL) {
-		result = NULL;
-	} else if (base_uri == NULL) {
-		result = g_strdup (relative_uri);
-	} else if (relative_uri == NULL) {
-		result = g_strdup (base_uri);
-	} else if (!is_uri_partial (relative_uri)) {
-		result = g_strdup (relative_uri);
-	} else {
-		char *mutable_base_uri;
-		char *mutable_uri;
-
-		char *uri_current;
-		size_t base_uri_length;
-		char *separator;
-
-		mutable_base_uri = g_strdup (base_uri);
-		uri_current = mutable_uri = g_strdup (relative_uri);
-
-		/* Chew off Fragment and Query from the base_url */
-
-		separator = strrchr (mutable_base_uri, '#'); 
-
-		if (separator) {
-			*separator = '\0';
-		}
-
-		separator = strrchr (mutable_base_uri, '?');
-
-		if (separator) {
-			*separator = '\0';
-		}
-
-		if ('/' == uri_current[0] && '/' == uri_current [1]) {
-			/* Relative URI's beginning with the authority
-			 * component inherit only the scheme from their parents
-			 */
-
-			separator = strchr (mutable_base_uri, ':');
-
-			if (separator) {
-				separator[1] = '\0';
-			}			  
-		} else if ('/' == uri_current[0]) {
-			/* Relative URI's beginning with '/' absolute-path based
-			 * at the root of the base uri
-			 */
-
-			separator = strchr (mutable_base_uri, ':');
-
-			/* g_assert (separator), really */
-			if (separator) {
-				/* If we start with //, skip past the authority section */
-				if ('/' == separator[1] && '/' == separator[2]) {
-					separator = strchr (separator + 3, '/');
-					if (separator) {
-						separator[0] = '\0';
-					}
-				} else {
-				/* If there's no //, just assume the scheme is the root */
-					separator[1] = '\0';
-				}
-			}
-		} else if ('#' != uri_current[0]) {
-			/* Handle the ".." convention for relative uri's */
-
-			/* If there's a trailing '/' on base_url, treat base_url
-			 * as a directory path.
-			 * Otherwise, treat it as a file path, and chop off the filename
-			 */
-
-			base_uri_length = strlen (mutable_base_uri);
-			if ('/' == mutable_base_uri[base_uri_length-1]) {
-				/* Trim off '/' for the operation below */
-				mutable_base_uri[base_uri_length-1] = 0;
-			} else {
-				separator = strrchr (mutable_base_uri, '/');
-				if (separator) {
-					*separator = '\0';
-				}
-			}
-
-			remove_internal_relative_components (uri_current);
-
-			/* handle the "../"'s at the beginning of the relative URI */
-			while (0 == strncmp ("../", uri_current, 3)) {
-				uri_current += 3;
-				separator = strrchr (mutable_base_uri, '/');
-				if (separator) {
-					*separator = '\0';
-				} else {
-					/* <shrug> */
-					break;
-				}
-			}
-
-			/* handle a ".." at the end */
-			if (uri_current[0] == '.' && uri_current[1] == '.' 
-			    && uri_current[2] == '\0') {
-
-			    	uri_current += 2;
-				separator = strrchr (mutable_base_uri, '/');
-				if (separator) {
-					*separator = '\0';
-				}
-			}
-
-			/* Re-append the '/' */
-			mutable_base_uri [strlen(mutable_base_uri)+1] = '\0';
-			mutable_base_uri [strlen(mutable_base_uri)] = '/';
-		}
-
-		result = g_strconcat (mutable_base_uri, uri_current, NULL);
-		g_free (mutable_base_uri); 
-		g_free (mutable_uri); 
-	}
-	
-	return result;
+	return gnome_vfs_uri_make_full_from_relative (base_uri, relative_uri);
 }
+#endif /* GNOME_VFS_DISABLE_DEPRECATED */
+
 
 GnomeVFSResult
 _gnome_vfs_uri_resolve_all_symlinks_uri (GnomeVFSURI *uri,
Index: gnome-vfs-utils.h
===================================================================
RCS file: /cvs/gnome/gnome-vfs/libgnomevfs/gnome-vfs-utils.h,v
retrieving revision 1.37
diff -u -r1.37 gnome-vfs-utils.h
--- gnome-vfs-utils.h	5 Dec 2002 09:42:29 -0000	1.37
+++ gnome-vfs-utils.h	7 Feb 2003 12:39:36 -0000
@@ -148,8 +148,11 @@
 gboolean gnome_vfs_uris_match (const char *uri_1, const char *uri_2);
 char * gnome_vfs_get_uri_scheme (const char *uri);
 char * gnome_vfs_make_uri_from_shell_arg (const char *uri);
+
+#ifndef GNOME_VFS_DISABLE_DEPRECATED
 char * gnome_vfs_make_uri_full_from_relative (const char *base_uri,
 					      const char *relative_uri);
+#endif /* GNOME_VFS_DISABLE_DEPRECATED */
 
 G_END_DECLS