xffm changes

[edscott wilson garcia]

El lun, 27-01-2003 a las 07:59, Jens Luedicke escribió:
> On Mon, Jan 27, 2003 at 06:54:44AM -0600, edscott wilson garcia wrote:
> 
> > > I attached a patch of my new old changes. They include the backup
> > > feature. I tested them and they work for me.

Some comments:

1- It's a "good thing" to keep variables localized. For example, putting
wastebackup into the if block means it will not be pushed up to the
stack if not needed, and the call to g_free can be avoided. This
eliminates unnecesary code from being executed.

2- By using heap memory instead of the original stack memory for
variables wastepath and wastename, you have to put in g_free's all over
the place to avoid memory leaks (when  "access(wastepath, F_OK)&&mkdir
(wastepath, 0xFFFF) < 0" or when "access(wastename,F_OK) == 0 &&
rename(wastename,wastebackup)<0" or when "rename (path, wastename) < 0".

3- Do not include a trailing "/" within path variables. Doing so causes
many bugs and confusion in xftree and should be avoided in xffm. The
only path that ends with a slash should be the root dir.

I'm attaching a patch with the corrections made to your last patch, and
as you can see the code losses simplicity and is much longer (KISS). In
the original code, using 256 character strings on the stack might seem
overkill, but it's really a tradeoff to cut out a lot of code. POSIX
says that paths are no more than 255, so there is really no segv
posibility here.

What I just committed, based on your patch, is a small modification
which simplifies the code.



Edscott





> > 
> > If you use xfdiff to create your patch files (or create them in unified
> > format), I could actually use xfdiff to see what changes are implied,
> > line by line. The way you are sending them makes it more difficult to
> > read and I need more time to do so
> > 
> 
> I recreated and attached the patch using 'cvs diff -u remove.c > ...'
> 
> 
> -- 
> Jens Luedicke <jens at irs-net.com>
> 
> "Never offend people with style when you can offend them with substance."
> --Sam Brown
> 
> ----
> 

> Index: remove.c
> ===================================================================
> RCS file: /cvsroot/xfce/xfce-devel/xffm/src/remove.c,v
> retrieving revision 1.14
> diff -u -r1.14 remove.c
> --- remove.c	27 Jan 2003 03:40:45 -0000	1.14
> +++ remove.c	27 Jan 2003 13:54:53 -0000
> @@ -144,32 +144,59 @@
>  }
>  
>  
> -gboolean wasteit(char *path){
> -	char *directory=g_strdup(path);
> -	char wastepath[256];
> -	char wastename[256];
> -	if (!strchr(path,'/')) g_assert_not_reached();
> -	if (strlen(path)==1) return FALSE;
> -	*(strrchr(directory,'/'))=0;
> -	sprintf(wastepath,"%s/..Wastebasket",directory);
> -	g_free(directory);
> -	/*printf("DBG:wastepath=%s\n",wastepath);*/
> -	if (access(wastepath,F_OK) != 0){
> -		if (mkdir (wastepath, 0xFFFF) < 0) return FALSE;
> -		
> -	}
> -	sprintf(wastename,"%s/%s",wastepath,strrchr(path,'/')+1);
> -	/*printf("DBG:wastename=%s path=%s\n",wastename,path);*/
> -	if (access(wastename,F_OK) == 0) {
> -	   /* too harsh: if (!unlinkit(wastename)) return FALSE;*/
> -	   char wastebackup[256];
> -	   sprintf(wastebackup,"%s/%s",
> -		wastepath,new_name(wastepath,strrchr(wastename,'/')+1));
> -	   /*printf("DBG:wastebackup=%s\n",wastebackup);*/
> -	   if (rename(wastename,wastebackup)<0) return FALSE;
> -	}
> -	if (rename(path,wastename)<0) return FALSE;
> -	return TRUE;
> +gboolean
> +wasteit (char *path)
> +{
> +  gchar *filepath = NULL;
> +  gchar *filename = NULL;
> +  gchar *wastepath = NULL;
> +  gchar *wastename = NULL;
> +  gchar *wastebackup = NULL;
> +
> +  if (!strchr (path, '/'))
> +    g_assert_not_reached ();
> +
> +  if (strlen (path) == 1)
> +    return FALSE;
> +
> +  filepath = g_path_get_dirname (path);
> +  filename = g_path_get_basename (path);
> +
> +  wastepath = g_strconcat (filepath, "/..Wastebasket/", NULL);
> +  wastename = g_strconcat (wastepath, filename, NULL);
> +
> +  /*
> +     printf("DBG:wastepath=%s\n",wastepath);
> +     printf("DBG:wastename=%s path=%s\n",wastename,path);
> +   */
> +
> +  if (access (wastepath, F_OK) != 0)
> +    {
> +      if (mkdir (wastepath, 0xFFFF) < 0)
> +	return FALSE;
> +    }
> +
> +  if (access(wastename,F_OK) == 0) {
> +	/* too harsh: if (!unlinkit(wastename)) return FALSE;*/
> +	wastebackup = g_strconcat(wastepath, new_name(wastepath, filename));
> +	/*
> +	printf("DBG:wastebackup=%s\n",wastebackup);
> +	*/
> +
> +	if (rename(wastename,wastebackup)<0)
> +	    return FALSE;
> +  }
> +
> +  if (rename (path, wastename) < 0)
> +    return FALSE;
> +
> +  g_free (filepath);
> +  g_free (filename);
> +  g_free (wastepath);
> +  g_free (wastename);
> +  g_free (wastebackup);
> +
> +  return TRUE;
>  }
>  
>  

-------------- next part --------------
--- /home/edscott/CVS/sourceforge/xfce/xfce-devel/xffm/src/remove.c-0	2003-01-27 10:11:38.000000000 -0600
+++ /home/edscott/CVS/sourceforge/xfce/xfce-devel/xffm/src/remove.c	2003-01-27 10:54:34.000000000 -0600
@@ -144,32 +144,73 @@
 }
 
 
-gboolean wasteit(char *path){
-	char *directory=g_strdup(path);
-	char wastepath[256];
-	char wastename[256];
-	if (!strchr(path,'/')) g_assert_not_reached();
-	if (strlen(path)==1) return FALSE;
-	*(strrchr(directory,'/'))=0;
-	sprintf(wastepath,"%s/..Wastebasket",directory);
-	g_free(directory);
-	/*printf("DBG:wastepath=%s\n",wastepath);*/
-	if (access(wastepath,F_OK) != 0){
-		if (mkdir (wastepath, 0xFFFF) < 0) return FALSE;
-		
-	}
-	sprintf(wastename,"%s/%s",wastepath,strrchr(path,'/')+1);
-	/*printf("DBG:wastename=%s path=%s\n",wastename,path);*/
-	if (access(wastename,F_OK) == 0) {
-	   /* too harsh: if (!unlinkit(wastename)) return FALSE;*/
-	   char wastebackup[256];
-	   sprintf(wastebackup,"%s/%s",
-		wastepath,new_name(wastepath,strrchr(wastename,'/')+1));
-	   /*printf("DBG:wastebackup=%s\n",wastebackup);*/
-	   if (rename(wastename,wastebackup)<0) return FALSE;
+gboolean
+wasteit (char *path)
+{
+  gchar *filepath = NULL;
+  gchar *filename = NULL;
+  gchar *wastepath = NULL;
+  gchar *wastename = NULL;
+
+  if (!strchr (path, '/'))
+    g_assert_not_reached ();
+
+  if (strlen (path) == 1)
+    return FALSE;
+
+  filepath = g_path_get_dirname (path);
+  filename = g_path_get_basename (path);
+
+  wastepath = g_strconcat (filepath, "/..Wastebasket","/", NULL);
+  wastename = g_strconcat (wastepath, filename, NULL);
+
+  /*
+     printf("DBG:wastepath=%s\n",wastepath);
+     printf("DBG:wastename=%s path=%s\n",wastename,path);
+   */
+
+  if (access (wastepath, F_OK) != 0)
+    {
+      if (mkdir (wastepath, 0xFFFF) < 0) {
+  	g_free (filepath);
+  	g_free (filename);
+  	g_free (wastepath);
+  	g_free (wastename);
+	return FALSE:
+    }
+
+  if (access(wastename,F_OK) == 0) {
+  	gchar *wastebackup = NULL;
+	/* too harsh: if (!unlinkit(wastename)) return FALSE;*/
+	wastebackup = g_strconcat(wastepath, new_name(wastepath,"/",filename));
+	/*
+	printf("DBG:wastebackup=%s\n",wastebackup);
+	*/
+
+	if (rename(wastename,wastebackup)<0){
+   	  g_free (filepath);
+  	  g_free (filename);
+  	  g_free (wastepath);
+  	  g_free (wastename);
+  	  g_free (wastebackup);
+	  return FALSE:
 	}
-	if (rename(path,wastename)<0) return FALSE;
-	return TRUE;
+  	g_free (wastebackup);
+  }
+
+  if (rename (path, wastename) < 0){
+    g_free (filepath);
+    g_free (filename);
+    g_free (wastepath);
+    g_free (wastename);
+    return FALSE;
+  } else {
+    g_free (filepath);
+    g_free (filename);
+    g_free (wastepath);
+    g_free (wastename);
+    return TRUE;
+  }
 }