Re: [PATCH 3/5] safe_create_leading_directories(): add "slash" pointer
- From
Michael Haggerty <mhagger@alum.mit.edu>
- Date
- Jan 1, 2014, 21:10 UTC
- Message-ID
- <52C48441.8060605@alum.mit.edu>
- In-Reply-To
- <20131226223444.GY20443@google.com>
On 12/26/2013 11:34 PM, Jonathan Nieder wrote:
Show 7 quoted lines
> 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.
Show 13 quoted lines
>> 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.
Show 18 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.OK, I split the renaming into a separate commit.
Show 9 quoted lines
>> + 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.
Show 9 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. :)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/