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

Johannes Schindelin [off-list ref] writes:

On Fri, 28 Sep 2007, Junio C Hamano wrote:

quoted

Johannes Schindelin [off-list ref] writes:

quoted

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

Uh, C has a comma operator.  And inverting the first condition makes
for a nicer flow, no goto.

		namlen = strlen(e->d_name);
		if (len + namlen <= PATH_MAX
			&& (memcpy(path + len, e->d_name, namlen),
			    path[len + namlen] = '\0',
			    lstat(path, &st) == 0)
		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;


-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum