Re: [PATCH]: first take at cleanup of #include, xmalloc / xrealloc, git status report usage.

From: Klaus Robert Suetterlin <robert@mpe.mpg.de>
Date: Apr 29, 2005, 17:07 UTC
Message-ID: <20050429170743.GD93323@xdt04.mpe-garching.mpg.de>
In-Reply-To: <20050429182407.5f6afd15.froese@gmx.de>

Thanks for reviewing this lengthy patch!

On Fri, Apr 29, 2005 at 06:24:07PM +0200, Edgar Toernig wrote:

Show 23 quoted lines

> Robert S?tterlin wrote:
> >[...]
> > +static int
> > +create_directories(const char *path)
> >   {
> > -	int len = strlen(path);
> > -	char *buf = xmalloc(len + 1);
> > -	const char *slash = path;
> > +	char *buf = (char *)path;
> > +	char *slash = buf;
> > 
> >   	while ((slash = strchr(slash+1, '/')) != NULL) {
> > -		len = slash - path;
> > -		memcpy(buf, path, len);
> > -		buf[len] = 0;
> > -		mkdir(buf, 0755);
> > +		*slash = '\0';
> > +		if (0 != mkdir(buf, 0755))
> > +			return error("Unable to mkdir(``%s'', 0755)", buf);
> > +		*slash = '/';
> 
> You need the temp buffer.  Simply casting the const away may
> shut up the compiler but it's not correct.

Ok, I see! Someone will pass a const char * (e.g. static string, readonly mmap, ...). In that case I would rather change the signature of the function ;). My am I a smart ass, ain't I.

What I wanted to achieve was getting rid of xmalloc that would just die("horribly") in case we cannot allocate the memory. Or some other hacked up malloc / realloc alternative.

Aside: I really do not like the current habit of die("if anything does
   not work out.") in git code all that much.  People seem to take
   the die() as cast in stone, and do not free resources in code
   pathes that lead to a die(), currently.  Of course this die()
   might change in the near future.  And we will have to examine
   every die() very carefully to make sure Joe Lazy Programmer
   didn't leave any garbage lying around.
   
   IMHO, the right thing for die() would be to dump core instead of just
   exiting.  As die() should be called in the ``can't happen'' or rather
   ``isn't resolved correctly'' cases only.  And programmers would be
   able to use the core to identify these cases quickly.

Unfortunately what You say is true and I cannot see a way around some kind of working copy (except for forcing the caller to provide a non-const char*). Maybe I will put ``char scratchpath[MAXPATH + 1];'' and ``#include <sys/param.h>'' in cache.h :).

Show 5 quoted lines

> 
> > -		if (errno != EEXIST)
> > +		if (EEXIST != errno)
> 
> Too much Star Wars?  Joda-speak?

No.  This is my prefered style for two reasons:

1) Putting the non-l-value on the left hand side protects against
   my most common typo: "=" instead of "==" or "!=".  No matter which
   compiler or warning level.

2) More often than not the variable part will be longer than
   ``errno''.  So putting EEXIST and the comparator front helps seeing
   the condition I test against.  Just compare:

while (-1 != (ch = getopt(argc, argv, "abcde:fg:h:ijkl:mnopq:r:stuvwx:y:z"))) while ((ch = getopt(argc, argv, "abcde:fg:h:ijkl:mnopq:r:stuvwx:y:z")) != -1)

Show 5 quoted lines

> 
> Ciao, ET.
> 
> 
> PS: the mkdir mode should be 0777 ...

Thanks! I did just a literal copy of what was there before. I added the check for the return value, to get the right diagnostic output.

Kind regards,

--Robert Suetterlin (robert@mpe.mpg.de) phone: (+49)89 / 30000-3546 fax: (+49)89 / 30000-3950