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

[Michael Haggerty <mhagger@alum.mit.edu>]

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@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?

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@alum.mit.edu
http://softwareswirl.blogspot.com/