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

On 12/26/2013 11:34 PM, Jonathan Nieder wrote:

Michael Haggerty wrote:

quoted

[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.

quoted

Keep track of the position of the slash character separately, and

Separately from what?

quoted

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 <redacted>

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.

quoted

---
 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.

quoted

+	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.

[...]

quoted

			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/