Re: [PATCH 3/5] safe_create_leading_directories(): add "slash" pointer
On 12/26/2013 11:34 PM, Jonathan Nieder wrote:
> 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.
It's hard to make the subject more self-explanatory, given so few
characters. But I will make the rest of the log message better in the
reroll.
>> 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
>
> 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?
Yes.
> It's too bad there's no variant of 'stat' and 'mkdir' that takes
> a (buf, len) pair which would avoid the scribbling altogether.
Yes.
>> ---
>> 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.
OK, I split the renaming into a separate commit.
>> +int retval = 0;
>>
>> -while (pos) {
>> +while (!retval && next_component) {
>
> A more usual style would be
> [...]
> Using retval for control flow instead makes it eight lines more
> concise, which is probably worth it.
Agreed.
> [...]
>> 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. :)
Will fix.
> 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.
OK, will change.
> That would mean the following changes to be potentially squashed in
> [...]
While going over the code again, I noticed another problem in the
original version; namely, that the handling of redundant multiple
slashes in the input path is not correct. I will fix this problem and
split up the commit into smaller steps in the re-roll.
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] safe_create_leading_directories(): add "slash" pointer
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
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)
[PATCH 3/5] safe_create_leading_directories(): add "slash" pointer
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
---
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);
+ int retval = 0;
- while (pos) {
+ while (!retval && next_component) {
struct stat st;
+ char *slash = strchr(next_component, '/');
- pos = strchr(pos, '/');
- if (!pos)
- break;
- while (*++pos == '/')
- ;
- if (!*pos)
- break;
- *--pos = '\0';
+ if (!slash)
+ return 0;
+ while (*(slash + 1) == '/')
+ slash++;
+ next_component = slash + 1;
+ if (!*next_component)
+ return 0;
+
+ *slash = '\0';
if (!stat(path, &st)) {
/* path exists */
if (!S_ISDIR(st.st_mode)) {
- *pos = '/';
- return -3;
+ retval = -3;
}
} else if (mkdir(path, 0777)) {
if (errno == EEXIST &&
!stat(path, &st) && S_ISDIR(st.st_mode)) {
; /* somebody created it since we checked */
} else {
- *pos = '/';
- return -1;
+ retval = -1;
}
} else if (adjust_shared_perm(path)) {
- *pos = '/';
- return -2;
+ retval = -2;
}
- *pos++ = '/';
+ *slash = '/';
}
- return 0;
+ return retval;
}
int safe_create_leading_directories_const(const char *path)
--
1.8.5.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
