{"thread":{"id":"35566","subject":"[PATCH 3/5] safe_create_leading_directories(): add \"slash\" pointer","startedAt":"2013-12-22T07:14:06Z","lastAt":"2014-01-02T00:53:49Z","messageCount":15,"participants":["Michael Haggerty","Ramsay Jones","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"232347","messageId":"1387696451-32224-1-git-send-email-mhagger@alum.mit.edu","threadId":"35566","inReplyTo":null,"subject":"[PATCH 0/5] Fix two mkdir/rmdir races","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-12-22T07:14:06Z","receivedAt":"2013-12-22T07:14:06Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"There seem to be a lot of mkdir/rmdir races in the Git code. Fix two\nof them.\n\nIt is hard to construct test cases for races without a lot of extra\ntest infrastructure. So I have tested them as best I can using\ninstrumented code (not included here).\n\nMichael Haggerty (5):\n safe_create_leading_directories(): modernize format of \"if\" chaining\n safe_create_leading_directories(): reduce scope of local variable\n safe_create_leading_directories(): add \"slash\" pointer\n safe_create_leading_directories(): fix a mkdir/rmdir race\n rename_ref(): fix a mkdir()/rmdir() race\n\n refs.c | 10 +++++++++-\n sha1_file.c | 56 +++++++++++++++++++++++++++++++++-----------------------\n 2 files changed, 42 insertions(+), 24 deletions(-)\n\n-- \n1.8.5.1\n"},{"id":"232348","messageId":"1387696451-32224-2-git-send-email-mhagger@alum.mit.edu","threadId":"35566","inReplyTo":"1387696451-32224-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 1/5] safe_create_leading_directories(): modernize format of \"if\" chaining","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-12-22T07:14:07Z","receivedAt":"2013-12-22T07:14:07Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Signed-off-by: Michael Haggerty \n---\n sha1_file.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex daacc0c..c9245a6 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -125,8 +125,7 @@ int safe_create_leading_directories(char *path)\n \t\t\t\t*pos = '/';\n \t\t\t\treturn -3;\n \t\t\t}\n-\t\t}\n-\t\telse if (mkdir(path, 0777)) {\n+\t\t} else if (mkdir(path, 0777)) {\n \t\t\tif (errno == EEXIST &&\n \t\t\t !stat(path, &st) && S_ISDIR(st.st_mode)) {\n \t\t\t\t; /* somebody created it since we checked */\n@@ -134,8 +133,7 @@ int safe_create_leading_directories(char *path)\n \t\t\t\t*pos = '/';\n \t\t\t\treturn -1;\n \t\t\t}\n-\t\t}\n-\t\telse if (adjust_shared_perm(path)) {\n+\t\t} else if (adjust_shared_perm(path)) {\n \t\t\t*pos = '/';\n \t\t\treturn -2;\n \t\t}\n-- \n1.8.5.1\n"},{"id":"232344","messageId":"1387696451-32224-3-git-send-email-mhagger@alum.mit.edu","threadId":"35566","inReplyTo":"1387696451-32224-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 2/5] safe_create_leading_directories(): reduce scope of local variable","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-12-22T07:14:08Z","receivedAt":"2013-12-22T07:14:08Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Signed-off-by: Michael Haggerty \n---\n sha1_file.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex c9245a6..cc9957e 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -108,9 +108,10 @@ int mkdir_in_gitdir(const char *path)\n int safe_create_leading_directories(char *path)\n {\n \tchar *pos = path + offset_1st_component(path);\n-\tstruct stat st;\n \n \twhile (pos) {\n+\t\tstruct stat st;\n+\n \t\tpos = strchr(pos, '/');\n \t\tif (!pos)\n \t\t\tbreak;\n-- \n1.8.5.1\n"},{"id":"232343","messageId":"1387696451-32224-4-git-send-email-mhagger@alum.mit.edu","threadId":"35566","inReplyTo":"1387696451-32224-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 3/5] safe_create_leading_directories(): add \"slash\" pointer","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-12-22T07:14:09Z","receivedAt":"2013-12-22T07:14:09Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Keep track of the position of the slash character separately, and\nrestore the slash character at a single place, at the end of the while\nloop. This makes the next change easier to implement.\n\nSigned-off-by: Michael Haggerty \n---\n sha1_file.c | 36 ++++++++++++++++++------------------\n 1 file changed, 18 insertions(+), 18 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex cc9957e..dcfd35a 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -107,40 +107,40 @@ int mkdir_in_gitdir(const char *path)\n \n int safe_create_leading_directories(char *path)\n {\n-\tchar *pos = path + offset_1st_component(path);\n+\tchar *next_component = path + offset_1st_component(path);\n+\tint retval = 0;\n \n-\twhile (pos) {\n+\twhile (!retval && next_component) {\n \t\tstruct stat st;\n+\t\tchar *slash = strchr(next_component, '/');\n \n-\t\tpos = strchr(pos, '/');\n-\t\tif (!pos)\n-\t\t\tbreak;\n-\t\twhile (*++pos == '/')\n-\t\t\t;\n-\t\tif (!*pos)\n-\t\t\tbreak;\n-\t\t*--pos = '\\0';\n+\t\tif (!slash)\n+\t\t\treturn 0;\n+\t\twhile (*(slash + 1) == '/')\n+\t\t\tslash++;\n+\t\tnext_component = slash + 1;\n+\t\tif (!*next_component)\n+\t\t\treturn 0;\n+\n+\t\t*slash = '\\0';\n \t\tif (!stat(path, &st)) {\n \t\t\t/* path exists */\n \t\t\tif (!S_ISDIR(st.st_mode)) {\n-\t\t\t\t*pos = '/';\n-\t\t\t\treturn -3;\n+\t\t\t\tretval = -3;\n \t\t\t}\n \t\t} else if (mkdir(path, 0777)) {\n \t\t\tif (errno == EEXIST &&\n \t\t\t !stat(path, &st) && S_ISDIR(st.st_mode)) {\n \t\t\t\t; /* somebody created it since we checked */\n \t\t\t} else {\n-\t\t\t\t*pos = '/';\n-\t\t\t\treturn -1;\n+\t\t\t\tretval = -1;\n \t\t\t}\n \t\t} else if (adjust_shared_perm(path)) {\n-\t\t\t*pos = '/';\n-\t\t\treturn -2;\n+\t\t\tretval = -2;\n \t\t}\n-\t\t*pos++ = '/';\n+\t\t*slash = '/';\n \t}\n-\treturn 0;\n+\treturn retval;\n }\n \n int safe_create_leading_directories_const(const char *path)\n-- \n1.8.5.1\n"},{"id":"232346","messageId":"1387696451-32224-5-git-send-email-mhagger@alum.mit.edu","threadId":"35566","inReplyTo":"1387696451-32224-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 4/5] safe_create_leading_directories(): fix a mkdir/rmdir race","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-12-22T07:14:10Z","receivedAt":"2013-12-22T07:14:10Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It could be that some other process is trying to clean up empty\ndirectories at the same time that safe_create_leading_directories() is\nattempting to create them. In this case, it could happen that\ndirectory \"a/b\" was present at the end of one iteration of the loop\n(either it was already present or we just created it ourselves), but\nby the time we try to create directory \"a/b/c\", directory \"a/b\" has\nbeen deleted. In fact, directory \"a\" might also have been deleted.\n\nSo, if a call to mkdir() fails with ENOENT, then try checking/making\nall directories again from the beginning. Attempt up to three times\nbefore giving up.\n\nSigned-off-by: Michael Haggerty \n---\n sha1_file.c | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex dcfd35a..abcb56b 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -108,6 +108,7 @@ int mkdir_in_gitdir(const char *path)\n int safe_create_leading_directories(char *path)\n {\n \tchar *next_component = path + offset_1st_component(path);\n+\tint attempts = 3;\n \tint retval = 0;\n \n \twhile (!retval && next_component) {\n@@ -132,6 +133,16 @@ int safe_create_leading_directories(char *path)\n \t\t\tif (errno == EEXIST &&\n \t\t\t !stat(path, &st) && S_ISDIR(st.st_mode)) {\n \t\t\t\t; /* somebody created it since we checked */\n+\t\t\t} else if (errno == ENOENT && --attempts) {\n+\t\t\t\t/*\n+\t\t\t\t * Either mkdir() failed bacause\n+\t\t\t\t * somebody just pruned the containing\n+\t\t\t\t * directory, or stat() failed because\n+\t\t\t\t * the file that was in our way was\n+\t\t\t\t * just removed. Either way, try\n+\t\t\t\t * again from the beginning:\n+\t\t\t\t */\n+\t\t\t\tnext_component = path + offset_1st_component(path);\n \t\t\t} else {\n \t\t\t\tretval = -1;\n \t\t\t}\n-- \n1.8.5.1\n"},{"id":"232345","messageId":"1387696451-32224-6-git-send-email-mhagger@alum.mit.edu","threadId":"35566","inReplyTo":"1387696451-32224-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 5/5] rename_ref(): fix a mkdir()/rmdir() race","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-12-22T07:14:11Z","receivedAt":"2013-12-22T07:14:11Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"When renaming a reflog file, it was possible that an empty directory\nthat we just created using safe_create_leading_directories() might get\ndeleted by another process before we have a chance to move the new\nfile into it.\n\nSo if the rename fails with ENOENT, then retry from the beginning.\nMake up to three attempts before giving up.\n\nIt could theoretically happen that the ENOENT comes from the\ndisappearance of TMP_RENAMED_LOG. In that case three pointless\nattempts will be made to move the nonexistent file, but no other harm\nshould come of it.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 10 +++++++++-\n 1 file changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 3926136..3ab1491 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2516,6 +2516,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \tstruct stat loginfo;\n \tint log = !lstat(git_path(\"logs/%s\", oldrefname), &loginfo);\n \tconst char *symref = NULL;\n+\tint attempts = 3;\n \n \tif (log && S_ISLNK(loginfo.st_mode))\n \t\treturn error(\"reflog for %s is a symlink\", oldrefname);\n@@ -2555,12 +2556,12 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \t\t}\n \t}\n \n+ retry:\n \tif (log && safe_create_leading_directories(git_path(\"logs/%s\", newrefname))) {\n \t\terror(\"unable to create directory for %s\", newrefname);\n \t\tgoto rollback;\n \t}\n \n- retry:\n \tif (log && rename(git_path(TMP_RENAMED_LOG), git_path(\"logs/%s\", newrefname))) {\n \t\tif (errno==EISDIR || errno==ENOTDIR) {\n \t\t\t/*\n@@ -2574,6 +2575,13 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \t\t\t}\n \t\t\tgoto retry;\n \t\t} else {\n+\t\t\tif (errno == ENOENT && --attempts)\n+\t\t\t\t/*\n+\t\t\t\t * Perhaps somebody just pruned the empty\n+\t\t\t\t * directory into which we wanted to move the\n+\t\t\t\t * file.\n+\t\t\t\t */\n+\t\t\t\tgoto retry;\n \t\t\terror(\"unable to move logfile \"TMP_RENAMED_LOG\" to logs/%s: %s\",\n \t\t\t\tnewrefname, strerror(errno));\n \t\t\tgoto rollback;\n-- \n1.8.5.1\n"},{"id":"232358","messageId":"52B76AEF.70200@ramsay1.demon.co.uk","threadId":"35566","inReplyTo":"1387696451-32224-5-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 4/5] safe_create_leading_directories(): fix a mkdir/rmdir race","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-12-22T22:42:55Z","receivedAt":"2013-12-22T22:42:55Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 22/12/13 07:14, Michael Haggerty wrote:\n> It could be that some other process is trying to clean up empty\n> directories at the same time that safe_create_leading_directories() is\n> attempting to create them. In this case, it could happen that\n> directory \"a/b\" was present at the end of one iteration of the loop\n> (either it was already present or we just created it ourselves), but\n> by the time we try to create directory \"a/b/c\", directory \"a/b\" has\n> been deleted. In fact, directory \"a\" might also have been deleted.\n> \n> So, if a call to mkdir() fails with ENOENT, then try checking/making\n> all directories again from the beginning. Attempt up to three times\n> before giving up.\n> \n> Signed-off-by: Michael Haggerty \n> ---\n> sha1_file.c | 11 +++++++++++\n> 1 file changed, 11 insertions(+)\n> \n> diff --git a/sha1_file.c b/sha1_file.c\n> index dcfd35a..abcb56b 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -108,6 +108,7 @@ int mkdir_in_gitdir(const char *path)\n> int safe_create_leading_directories(char *path)\n> {\n> \tchar *next_component = path + offset_1st_component(path);\n> +\tint attempts = 3;\n> \tint retval = 0;\n> \n> \twhile (!retval && next_component) {\n> @@ -132,6 +133,16 @@ int safe_create_leading_directories(char *path)\n> \t\t\tif (errno == EEXIST &&\n> \t\t\t !stat(path, &st) && S_ISDIR(st.st_mode)) {\n> \t\t\t\t; /* somebody created it since we checked */\n> +\t\t\t} else if (errno == ENOENT && --attempts) {\n> +\t\t\t\t/*\n> +\t\t\t\t * Either mkdir() failed bacause\n\ns/bacause/because/\n\n> +\t\t\t\t * somebody just pruned the containing\n> +\t\t\t\t * directory, or stat() failed because\n> +\t\t\t\t * the file that was in our way was\n> +\t\t\t\t * just removed. Either way, try\n> +\t\t\t\t * again from the beginning:\n> +\t\t\t\t */\n> +\t\t\t\tnext_component = path + offset_1st_component(path);\n> \t\t\t} else {\n> \t\t\t\tretval = -1;\n> \t\t\t}\n> \n\nATB,\nRamsay Jones\n"},{"id":"232419","messageId":"20131226215225.GW20443@google.com","threadId":"35566","inReplyTo":"1387696451-32224-2-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 1/5] safe_create_leading_directories(): modernize format of \"if\" chaining","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-26T21:52:25Z","receivedAt":"2013-12-26T21:52:25Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> [Subject: safe_create_leading_directories(): modernize format of \"if\" chaining]\n\nTrivia: it's not so much modernizing as following K&R style, which git\nmore or less followed since day 1. Linux's Documentation/CodingStyle\nexplains:\n\n Note that the closing brace is empty on a line of its own, _except_ in\n the cases where it is followed by a continuation of the same statement,\n ie a \"while\" in a do-statement or an \"else\" in an if-statement, like\n this:\n[...]\n Rationale: K&R.\n\n Also, note that this brace-placement also minimizes the number of empty\n (or almost empty) lines, without any loss of readability. Thus, as the\n supply of new-lines on your screen is not a renewable resource (think\n 25-line terminal screens here), you have more empty lines to put\n comments on.\n\nHere it's especially jarring since the function uses a mix of styles.\nThanks for cleaning it up.\n"},{"id":"232420","messageId":"20131226215559.GX20443@google.com","threadId":"35566","inReplyTo":"1387696451-32224-3-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 2/5] safe_create_leading_directories(): reduce scope of local variable","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-26T21:55:59Z","receivedAt":"2013-12-26T21:55:59Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> Signed-off-by: Michael Haggerty \n> ---\n> sha1_file.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n> \n> diff --git a/sha1_file.c b/sha1_file.c\n> index c9245a6..cc9957e 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -108,9 +108,10 @@ int mkdir_in_gitdir(const char *path)\n> int safe_create_leading_directories(char *path)\n> {\n> \tchar *pos = path + offset_1st_component(path);\n> -\tstruct stat st;\n> \n> \twhile (pos) {\n> +\t\tstruct stat st;\n\nIs this to make it easier to reason about whether 'st' has been\nproperly initialized at any given moment, or is there a more subtle\nreason?\n\nCurious,\nJonathan\n"},{"id":"232421","messageId":"20131226223444.GY20443@google.com","threadId":"35566","inReplyTo":"1387696451-32224-4-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 3/5] safe_create_leading_directories(): add \"slash\" pointer","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-26T22:34:44Z","receivedAt":"2013-12-26T22:34:44Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> [Subject: safe_create_leading_directories(): add \"slash\" pointer]\n\nIs this a cleanup or improving the (internal) functionality of the\nfunction somehow? The above one-liner doesn't sum up for me in an\nobvious way why this is a good change.\n\n> Keep track of the position of the slash character separately, and\n\nSeparately from what?\n\n> restore the slash character at a single place, at the end of the while\n> loop. This makes the next change easier to implement.\n>\n> Signed-off-by: Michael Haggerty \n\nAh, do I understand correctly that this is about cleaning up\nafter the code that scribbles over 'path' in one place, to make\nit harder to forget to do that cleanup as new code paths are\nintroduced?\n\nIt's too bad there's no variant of 'stat' and 'mkdir' that takes\na (buf, len) pair which would avoid the scribbling altogether.\n\n> ---\n> sha1_file.c | 36 ++++++++++++++++++------------------\n> 1 file changed, 18 insertions(+), 18 deletions(-)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index cc9957e..dcfd35a 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -107,40 +107,40 @@ int mkdir_in_gitdir(const char *path)\n> \n> int safe_create_leading_directories(char *path)\n> {\n> -\tchar *pos = path + offset_1st_component(path);\n> +\tchar *next_component = path + offset_1st_component(path);\n\nThis name change is probably worth also mentioning in the commit\nmessage (or lifting into a separate patch) so the reader doesn't get\ndistracted.\n\n> +\tint retval = 0;\n> \n> -\twhile (pos) {\n> +\twhile (!retval && next_component) {\n\nA more usual style would be\n\n\tint ... = 0;\n\n\twhile (pos) {\n\t\t...\n\t\tif (!stat(path, &st)) {\n\t\t\t/* path exists */\n\t\t\tif (!S_ISDIR(st.st_mode)) {\n\t\t\t\t... = -3;\n\t\t\t\tgoto out;\n\t\t\t}\n\t\t} else if (...) {\n\t\t\t...\n\t\t}\n\t\t...\n\t}\n out:\n\t*slash = '/';\n\treturn ...;\n }\n\nwhich makes it more explicit that the slash needs to be written back.\nIn this example, that would look like:\n\n\tchar *slash = NULL;\n\tint ret;\n\n\twhile (pos) {\n\t\t...\n\t\tif (!slash)\n\t\t\tbreak;\n\n\t\t...\n\t\tif (!*pos)\n\t\t\tbreak;\n\n\t\t*slash = '\\0';\n\t\tif (!stat(path, &st)) {\n\t\t\tif (!S_ISDIR(st.st_mode)) {\n\t\t\t\tret = -3;\n\t\t\t\tgoto out;\n\t\t\t}\n\t\t} else if (mkdir(...)) {\n\t\t\tif (errno == EEXIST && ...) {\n\t\t\t\t; /* ok */\n\t\t\t} else {\n\t\t\t\tret = -1;\n\t\t\t\tgoto out;\n\t\t\t}\n\t\t} else if (adjust_shared_perm(...)) {\n\t\t\tret = -2;\n\t\t\tgoto out;\n\t\t}\n\t\t*slash = '/';\n\t}\n\tret = 0;\n out:\n \tif (slash)\n\t\t*slash = '/';\n\treturn ret;\n\nUsing retval for control flow instead makes it eight lines more\nconcise, which is probably worth it.\n\n[...]\n> \t\t\tif (!S_ISDIR(st.st_mode)) {\n> -\t\t\t\t*pos = '/';\n> -\t\t\t\treturn -3;\n> +\t\t\t\tretval = -3;\n> \t\t\t}\n\nNow the 'if' body is one line, so we can drop the braces and save\nanother line. :)\n\nOne more nit: elsewhere in this file, a variable keeping track of the\nreturn value is named 'ret', so it probably makes sense to also use\nthat name here.\n\nThat would mean the following changes to be potentially squashed in\n(keeping 'pos' name to make the patch easier to read, s/retval/ret/,\nremoving unnecessary braces). None of these tweaks are particularly\nimportant. Feel free to skip them --- the only comment I've made that\nmatters is about the commit message.\n\nThanks for a nice cleanup,\nJonathan\n\ndiff --git i/sha1_file.c w/sha1_file.c\nindex dcfd35a..18cb50a 100644\n--- i/sha1_file.c\n+++ w/sha1_file.c\n@@ -107,40 +107,38 @@ int mkdir_in_gitdir(const char *path)\n \n int safe_create_leading_directories(char *path)\n {\n-\tchar *next_component = path + offset_1st_component(path);\n-\tint retval = 0;\n+\tchar *pos = path + offset_1st_component(path);\n+\tint ret = 0;\n \n-\twhile (!retval && next_component) {\n+\twhile (!ret && pos) {\n \t\tstruct stat st;\n-\t\tchar *slash = strchr(next_component, '/');\n+\t\tchar *slash = strchr(pos, '/');\n \n \t\tif (!slash)\n \t\t\treturn 0;\n \t\twhile (*(slash + 1) == '/')\n \t\t\tslash++;\n-\t\tnext_component = slash + 1;\n-\t\tif (!*next_component)\n+\t\tpos = slash + 1;\n+\t\tif (!*pos)\n \t\t\treturn 0;\n \n \t\t*slash = '\\0';\n \t\tif (!stat(path, &st)) {\n \t\t\t/* path exists */\n-\t\t\tif (!S_ISDIR(st.st_mode)) {\n-\t\t\t\tretval = -3;\n-\t\t\t}\n+\t\t\tif (!S_ISDIR(st.st_mode))\n+\t\t\t\tret = -3;\n \t\t} else if (mkdir(path, 0777)) {\n \t\t\tif (errno == EEXIST &&\n-\t\t\t !stat(path, &st) && S_ISDIR(st.st_mode)) {\n+\t\t\t !stat(path, &st) && S_ISDIR(st.st_mode))\n \t\t\t\t; /* somebody created it since we checked */\n-\t\t\t} else {\n-\t\t\t\tretval = -1;\n-\t\t\t}\n+\t\t\telse\n+\t\t\t\tret = -1;\n \t\t} else if (adjust_shared_perm(path)) {\n-\t\t\tretval = -2;\n+\t\t\tret = -2;\n \t\t}\n \t\t*slash = '/';\n \t}\n-\treturn retval;\n+\treturn ret;\n }\n \n int safe_create_leading_directories_const(const char *path)\n"},{"id":"232422","messageId":"20131226230204.GZ20443@google.com","threadId":"35566","inReplyTo":"1387696451-32224-5-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 4/5] safe_create_leading_directories(): fix a mkdir/rmdir race","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-26T23:02:04Z","receivedAt":"2013-12-26T23:02:04Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nMichael Haggerty wrote:\n\n> It could be that some other process is trying to clean up empty\n> directories at the same time that safe_create_leading_directories() is\n> attempting to create them. In this case, it could happen that\n> directory \"a/b\" was present at the end of one iteration of the loop\n> (either it was already present or we just created it ourselves), but\n> by the time we try to create directory \"a/b/c\", directory \"a/b\" has\n> been deleted. In fact, directory \"a\" might also have been deleted.\n\nWhen does this happen in practice? Is this about git racing with\nitself or with some other program?\n\nI fear that the aggressive directory creator fighting the aggressive\ndirectory remover might be waging a losing battle.\n\nIs this about a push that creates a ref racing against a push that\ndeletes a ref from the same hierarchy?\n\n> So, if a call to mkdir() fails with ENOENT, then try checking/making\n> all directories again from the beginning. Attempt up to three times\n> before giving up.\n\nIf we are racing against a ref deletion, then we can retry while our\nrival keeps walking up the directory tree and deleting parent\ndirectories. As soon as we successfully create a directory, we have\nwon the race.\n\nBut what happens if the entire safe_create_leading_directories\noperation succeeds and *then* our racing partner deletes the\ndirectory? No one is putting in a file to reserve the directory for\nthe directory creator.\n\nIf we care enough to retry more than once, I fear this is retrying at\nthe wrong level.\n\n> Signed-off-by: Michael Haggerty \n> ---\n> sha1_file.c | 11 +++++++++++\n> 1 file changed, 11 insertions(+)\n\nTests?\n\nThe code is clear and implements the design correctly.\n\nThanks for some food for thought,\nJonathan\n"},{"id":"232423","messageId":"20131226232023.GA20443@google.com","threadId":"35566","inReplyTo":"1387696451-32224-6-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 5/5] rename_ref(): fix a mkdir()/rmdir() race","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-26T23:20:23Z","receivedAt":"2013-12-26T23:20:23Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> refs.c | 10 +++++++++-\n> 1 file changed, 9 insertions(+), 1 deletion(-)\n\nA test or example reproduction recipe would be nice. (But I can\nunderstand not having one --- races are hard to test.)\n\n[...]\n> --- a/refs.c\n> +++ b/refs.c\n[...]\n> @@ -2574,6 +2575,13 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n> \t\t\t}\n> \t\t\tgoto retry;\n> \t\t} else {\n> +\t\t\tif (errno == ENOENT && --attempts)\n> +\t\t\t\t/*\n> +\t\t\t\t * Perhaps somebody just pruned the empty\n> +\t\t\t\t * directory into which we wanted to move the\n> +\t\t\t\t * file.\n> +\t\t\t\t */\n> +\t\t\t\tgoto retry;\n\nStyle nit: it's easier to read a test of errno when the 'else's\ncascade (i.e., using 'else if' here).\n\nThis patch doesn't depend on any of the others from the series. For\nwhat it's worth, with or without the following squashed in,\n\nReviewed-by: Jonathan Nieder \n\nThanks.\n\ndiff --git i/refs.c w/refs.c\nindex 3ab1491..ea62395 100644\n--- i/refs.c\n+++ w/refs.c\n@@ -2574,14 +2574,14 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \t\t\t\tgoto rollback;\n \t\t\t}\n \t\t\tgoto retry;\n+\t\t} else if (errno == ENOENT && --attempts)\n+\t\t\t/*\n+\t\t\t * Perhaps somebody just pruned the empty\n+\t\t\t * directory into which we wanted to move the\n+\t\t\t * file.\n+\t\t\t */\n+\t\t\tgoto retry;\n \t\t} else {\n-\t\t\tif (errno == ENOENT && --attempts)\n-\t\t\t\t/*\n-\t\t\t\t * Perhaps somebody just pruned the empty\n-\t\t\t\t * directory into which we wanted to move the\n-\t\t\t\t * file.\n-\t\t\t\t */\n-\t\t\t\tgoto retry;\n \t\t\terror(\"unable to move logfile \"TMP_RENAMED_LOG\" to logs/%s: %s\",\n \t\t\t\tnewrefname, strerror(errno));\n \t\t\tgoto rollback;\n"},{"id":"232549","messageId":"52C40C9B.2040001@alum.mit.edu","threadId":"35566","inReplyTo":"20131226215559.GX20443@google.com","subject":"Re: [PATCH 2/5] safe_create_leading_directories(): reduce scope of local variable","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-01T12:39:55Z","receivedAt":"2014-01-01T12:39:55Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/26/2013 10:55 PM, Jonathan Nieder wrote:\n> Michael Haggerty wrote:\n> \n>> Signed-off-by: Michael Haggerty \n>> ---\n>> sha1_file.c | 3 ++-\n>> 1 file changed, 2 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/sha1_file.c b/sha1_file.c\n>> index c9245a6..cc9957e 100644\n>> --- a/sha1_file.c\n>> +++ b/sha1_file.c\n>> @@ -108,9 +108,10 @@ int mkdir_in_gitdir(const char *path)\n>> int safe_create_leading_directories(char *path)\n>> {\n>> \tchar *pos = path + offset_1st_component(path);\n>> -\tstruct stat st;\n>> \n>> \twhile (pos) {\n>> +\t\tstruct stat st;\n> \n> Is this to make it easier to reason about whether 'st' has been\n> properly initialized at any given moment, or is there a more subtle\n> reason?\n\nNo, just the boring reason, the one that makes me reduce the scope of\nvariables whenever possible. I'll buff up the log message.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"232550","messageId":"52C48441.8060605@alum.mit.edu","threadId":"35566","inReplyTo":"20131226223444.GY20443@google.com","subject":"Re: [PATCH 3/5] safe_create_leading_directories(): add \"slash\" pointer","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-01T21:10:25Z","receivedAt":"2014-01-01T21:10:25Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/26/2013 11:34 PM, Jonathan Nieder wrote:\n> Michael Haggerty wrote:\n> \n>> [Subject: safe_create_leading_directories(): add \"slash\" pointer]\n> \n> Is this a cleanup or improving the (internal) functionality of the\n> function somehow? The above one-liner doesn't sum up for me in an\n> obvious way why this is a good change.\n\nIt's hard to make the subject more self-explanatory, given so few\ncharacters. But I will make the rest of the log message better in the\nreroll.\n\n>> Keep track of the position of the slash character separately, and\n> \n> Separately from what?\n> \n>> restore the slash character at a single place, at the end of the while\n>> loop. This makes the next change easier to implement.\n>>\n>> Signed-off-by: Michael Haggerty \n> \n> Ah, do I understand correctly that this is about cleaning up\n> after the code that scribbles over 'path' in one place, to make\n> it harder to forget to do that cleanup as new code paths are\n> introduced?\n\nYes.\n\n> It's too bad there's no variant of 'stat' and 'mkdir' that takes\n> a (buf, len) pair which would avoid the scribbling altogether.\n\nYes.\n\n>> ---\n>> sha1_file.c | 36 ++++++++++++++++++------------------\n>> 1 file changed, 18 insertions(+), 18 deletions(-)\n>>\n>> diff --git a/sha1_file.c b/sha1_file.c\n>> index cc9957e..dcfd35a 100644\n>> --- a/sha1_file.c\n>> +++ b/sha1_file.c\n>> @@ -107,40 +107,40 @@ int mkdir_in_gitdir(const char *path)\n>> \n>> int safe_create_leading_directories(char *path)\n>> {\n>> -\tchar *pos = path + offset_1st_component(path);\n>> +\tchar *next_component = path + offset_1st_component(path);\n> \n> This name change is probably worth also mentioning in the commit\n> message (or lifting into a separate patch) so the reader doesn't get\n> distracted.\n\nOK, I split the renaming into a separate commit.\n\n>> +\tint retval = 0;\n>> \n>> -\twhile (pos) {\n>> +\twhile (!retval && next_component) {\n> \n> A more usual style would be\n> [...]\n> Using retval for control flow instead makes it eight lines more\n> concise, which is probably worth it.\n\nAgreed.\n\n> [...]\n>> \t\t\tif (!S_ISDIR(st.st_mode)) {\n>> -\t\t\t\t*pos = '/';\n>> -\t\t\t\treturn -3;\n>> +\t\t\t\tretval = -3;\n>> \t\t\t}\n> \n> Now the 'if' body is one line, so we can drop the braces and save\n> another line. :)\n\nWill fix.\n\n> One more nit: elsewhere in this file, a variable keeping track of the\n> return value is named 'ret', so it probably makes sense to also use\n> that name here.\n\nOK, will change.\n\n> That would mean the following changes to be potentially squashed in\n> [...]\n\nWhile going over the code again, I noticed another problem in the\noriginal version; namely, that the handling of redundant multiple\nslashes in the input path is not correct. I will fix this problem and\nsplit up the commit into smaller steps in the re-roll.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"232551","messageId":"52C4B89D.5020902@alum.mit.edu","threadId":"35566","inReplyTo":"20131226230204.GZ20443@google.com","subject":"Re: [PATCH 4/5] safe_create_leading_directories(): fix a mkdir/rmdir race","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-02T00:53:49Z","receivedAt":"2014-01-02T00:53:49Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/27/2013 12:02 AM, Jonathan Nieder wrote:\n> Michael Haggerty wrote:\n> \n>> It could be that some other process is trying to clean up empty\n>> directories at the same time that safe_create_leading_directories() is\n>> attempting to create them. In this case, it could happen that\n>> directory \"a/b\" was present at the end of one iteration of the loop\n>> (either it was already present or we just created it ourselves), but\n>> by the time we try to create directory \"a/b/c\", directory \"a/b\" has\n>> been deleted. In fact, directory \"a\" might also have been deleted.\n> \n> When does this happen in practice? Is this about git racing with\n> itself or with some other program?\n\nI think it could be triggered by a reference creation racing with a\nreference packing. See below.\n\n> I fear that the aggressive directory creator fighting the aggressive\n> directory remover might be waging a losing battle.\n\nThat may be so, but it would be strange for a directory remover to be\naggressive. And even if it were, the worst consequence would be that\nthe director creator would try three times before giving up.\n\n> Is this about a push that creates a ref racing against a push that\n> deletes a ref from the same hierarchy?\n\nThe race could be triggered in this scenario but I don't think it would\nresult in a spurious error (at least not if there are only two\nracers...) The reason is that empty loose reference directories are not\ndeleted when a reference is *deleted*, but rather when a new\nd/f-conflicting reference is *created*. E.g., if\n\n git branch foo/bar\n git branch -d foo/bar # this leaves directory foo behind\n # this removes directory foo and creates file foo:\n git branch foo &\n git branch foo/baz\n\nThe last two commands could fight. However, in this scenario one of the\nreference creations would ultimately have to fail anyway, so this patch\ndoesn't really help.\n\nHowever, when packing references, the directories that used to hold the\nold references are deleted right away. So\n\n git branch foo/bar\n git pack-refs --all &\n git branch foo/baz\n\nHere, the last two commands could fight.\n\n>> So, if a call to mkdir() fails with ENOENT, then try checking/making\n>> all directories again from the beginning. Attempt up to three times\n>> before giving up.\n> \n> If we are racing against a ref deletion, then we can retry while our\n> rival keeps walking up the directory tree and deleting parent\n> directories. As soon as we successfully create a directory, we have\n> won the race.\n> \n> But what happens if the entire safe_create_leading_directories\n> operation succeeds and *then* our racing partner deletes the\n> directory? No one is putting in a file to reserve the directory for\n> the directory creator.\n>\n> If we care enough to retry more than once, I fear this is retrying at\n> the wrong level.\n\nI realize that this change doesn't solve the whole problem. But you\nmake a good point, that if the caller is going to retry anyway, then\nthere is no need to retry within this function. It would be sufficient\nfor this function to return a specific error value indicating that\n\"creating the directory failed, but there's a chance of success if you\ntry again\".\n\nOn the other hand, your argument assumes that all callers really *do*\nretry, which isn't the case, and I doubt that all callers are going to\nbe fixed. So there might be some value in retrying within this function\nanyway (it's a game of averages we're playing here anyway).\n\nI'll think some more about it.\n\n> Tests?\n\nI can't think of how to test this short of either instrumenting the code\n(which I did for my own tests, but didn't include the test code in this\nsubmission) or running the test within some kind of malicious virtual\nfilesystem. Ideas?\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"}]}