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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 28, 2007, 09:05 UTC
Message-ID: <7v8x6rqhwy.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <Pine.LNX.4.64.0709280606350.28395@racer.site>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 21 quoted lines

> +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').