Re: [PATCH 3/5] safe_create_leading_directories(): add "slash" pointer

[Jonathan Nieder <jrnieder@gmail.com>]

Michael Haggerty wrote:

> [Subject: safe_create_leading_directories(): add "slash" pointer]

Is this a cleanup or improving the (internal) functionality of the
function somehow?  The above one-liner doesn't sum up for me in an
obvious way why this is a good change.

> Keep track of the position of the slash character separately, and

Separately from what?

> restore the slash character at a single place, at the end of the while
> loop.  This makes the next change easier to implement.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

Ah, do I understand correctly that this is about cleaning up
after the code that scribbles over 'path' in one place, to make
it harder to forget to do that cleanup as new code paths are
introduced?

It's too bad there's no variant of 'stat' and 'mkdir' that takes
a (buf, len) pair which would avoid the scribbling altogether.

> ---
>  sha1_file.c | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index cc9957e..dcfd35a 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -107,40 +107,40 @@ int mkdir_in_gitdir(const char *path)
>  
>  int safe_create_leading_directories(char *path)
>  {
> -	char *pos = path + offset_1st_component(path);
> +	char *next_component = path + offset_1st_component(path);

This name change is probably worth also mentioning in the commit
message (or lifting into a separate patch) so the reader doesn't get
distracted.

> +	int retval = 0;
>  
> -	while (pos) {
> +	while (!retval && next_component) {

A more usual style would be

	int ... = 0;

	while (pos) {
		...
		if (!stat(path, &st)) {
			/* path exists */
			if (!S_ISDIR(st.st_mode)) {
				... = -3;
				goto out;
			}
		} else if (...) {
			...
		}
		...
	}
 out:
	*slash = '/';
	return ...;
 }

which makes it more explicit that the slash needs to be written back.
In this example, that would look like:

	char *slash = NULL;
	int ret;

	while (pos) {
		...
		if (!slash)
			break;

		...
		if (!*pos)
			break;

		*slash = '\0';
		if (!stat(path, &st)) {
			if (!S_ISDIR(st.st_mode)) {
				ret = -3;
				goto out;
			}
		} else if (mkdir(...)) {
			if (errno == EEXIST && ...) {
				; /* ok */
			} else {
				ret = -1;
				goto out;
			}
		} else if (adjust_shared_perm(...)) {
			ret = -2;
			goto out;
		}
		*slash = '/';
	}
	ret = 0;
 out:
 	if (slash)
		*slash = '/';
	return ret;

Using retval for control flow instead makes it eight lines more
concise, which is probably worth it.

[...]
> 			if (!S_ISDIR(st.st_mode)) {
> -				*pos = '/';
> -				return -3;
> +				retval = -3;
> 			}

Now the 'if' body is one line, so we can drop the braces and save
another line. :)

One more nit: elsewhere in this file, a variable keeping track of the
return value is named 'ret', so it probably makes sense to also use
that name here.

That would mean the following changes to be potentially squashed in
(keeping 'pos' name to make the patch easier to read, s/retval/ret/,
removing unnecessary braces).  None of these tweaks are particularly
important.  Feel free to skip them --- the only comment I've made that
matters is about the commit message.

Thanks for a nice cleanup,
Jonathan

diff --git i/sha1_file.c w/sha1_file.c
index dcfd35a..18cb50a 100644
--- i/sha1_file.c
+++ w/sha1_file.c
@@ -107,40 +107,38 @@ int mkdir_in_gitdir(const char *path)
 
 int safe_create_leading_directories(char *path)
 {
-	char *next_component = path + offset_1st_component(path);
-	int retval = 0;
+	char *pos = path + offset_1st_component(path);
+	int ret = 0;
 
-	while (!retval && next_component) {
+	while (!ret && pos) {
 		struct stat st;
-		char *slash = strchr(next_component, '/');
+		char *slash = strchr(pos, '/');
 
 		if (!slash)
 			return 0;
 		while (*(slash + 1) == '/')
 			slash++;
-		next_component = slash + 1;
-		if (!*next_component)
+		pos = slash + 1;
+		if (!*pos)
 			return 0;
 
 		*slash = '\0';
 		if (!stat(path, &st)) {
 			/* path exists */
-			if (!S_ISDIR(st.st_mode)) {
-				retval = -3;
-			}
+			if (!S_ISDIR(st.st_mode))
+				ret = -3;
 		} else if (mkdir(path, 0777)) {
 			if (errno == EEXIST &&
-			    !stat(path, &st) && S_ISDIR(st.st_mode)) {
+			    !stat(path, &st) && S_ISDIR(st.st_mode))
 				; /* somebody created it since we checked */
-			} else {
-				retval = -1;
-			}
+			else
+				ret = -1;
 		} else if (adjust_shared_perm(path)) {
-			retval = -2;
+			ret = -2;
 		}
 		*slash = '/';
 	}
-	return retval;
+	return ret;
 }
 
 int safe_create_leading_directories_const(const char *path)