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 <mhagger@xxxxxxxxxxxx>
>
> 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
mhagger@xxxxxxxxxxxx
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html