Re: [PATCH 3/5] safe_create_leading_directories(): add "slash" pointer
> [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
> 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.
Show 14 quoted lines
> ---
> 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.
[...]
Show 5 quoted lines
> 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
Show changes to diff +13 −15
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)
Michael Haggerty
