Re: [PATCH 1/2] Introduce remove_dir_recursively()

> On Fri, 28 Sep 2007, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 
>> > +int remove_dir_recursively(char *path, int len, int only_empty)
>> > +{
>> > ...
>> > +		namlen = strlen(e->d_name);
>> > +		if (len + namlen > PATH_MAX ||
>> > +				!memcpy(path + len, e->d_name, namlen) ||
>> > +				(path[len + namlen] = '\0') ||
>> > +				lstat(path, &st))
>> > +			; /* fall thru */
>> > +		else if (S_ISDIR(st.st_mode)) {
>> > +			if (!remove_dir_recursively(path, len + namlen,
>> > +						only_empty))
>> > +				continue; /* happy */
>> > +		} else if (!only_empty &&
>> > +				len + namlen + 1 < PATH_MAX &&
>> > +				!unlink(path))
>> > +			continue; /* happy, too */
>> > +
>> > +		/* path too long, stat fails, or non-directory still exists */
>> > +		ret = -1;
>> > +		break;
>> 
>> Is it only me who finds the first if () condition way too
>> convoluted and needs to read three times to convince oneself
>> that it is doing a sane thing?
>> 
>> Please, especially...
>> 
>>  * For $DEITY's sake, memcpy() returns pointer to dst which you
>>    know is not NULL. so !memcpy() is always false here, which
>>    might be _convenient_ for you and the compiler but not for
>>    a human reader of the code who needs to blink twice wondering
>>    if you meant !memcmp().
>> 
>>  * Same for (path[] = '\0'), wondering if it is misspelled
>>    (path[] == '\0').
>
> Okay, will fix (with an evil goto).