From: Klaus Robert Suetterlin <robert@mpe.mpg.de>
Date: Fri, 29 Apr 2005 17:07:43 GMT
Subject: Re: [PATCH]: first take at cleanup of #include, xmalloc / xrealloc, git status report usage.
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:
> 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 :).

> 
> > -		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)


> 
> 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