{"thread":{"id":"35614","subject":"[PATCH v2 00/17] Fix some mkdir/rmdir races","startedAt":"2014-01-06T13:45:18Z","lastAt":"2014-01-19T20:31:20Z","messageCount":29,"participants":["Michael Haggerty","Junio C Hamano","Sebastian Schuberth"],"isPatch":true,"patchVersion":2,"patchTotal":17},"messages":[{"id":"232694","messageId":"1389015935-21936-1-git-send-email-mhagger@alum.mit.edu","threadId":"35614","inReplyTo":null,"subject":"[PATCH v2 00/17] Fix some mkdir/rmdir races","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-06T13:45:18Z","receivedAt":"2014-01-06T13:45:18Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is v2 of changes [1] to remove some mkdir/rmdir races around\nsafe_create_leading_directories() and a couple of its callers.  Thanks\nto Jonathan Nieder for his thorough review of v1 and to Ramsay Jones\nfor pointing out a typo.  I think I have addressed all of their\nsuggestions.\n\nThis patch series has a bigger scope than v1.  The main differences is\nthat I took Jonathan's (implicit?) suggestion to move the retrying to\na level higher, where files are actually created in the directories\nand therefore the directories are definitely no longer subject to\npruning.\n\nDifferences from v1:\n\n* Improve some commit messages\n\n* Break up some changes to safe_create_leading_directories() into\n  smaller steps.\n\n* Fix a problem in safe_create_leading_directories() when handling\n  paths with multiple slashes (e.g., \"foo//bar\").  (I noticed this\n  pre-existing problem while making the other changes.)\n\n* Change how retrying is done:\n\n  * safe_create_leading_directories() doesn't retry internally;\n    instead, its return value is turned into an enum with a new value,\n    SCLD_VANISHED, that indicates that a directory along the path\n    vanished while it was working.  This return value is an indication\n    that its caller might want to try calling the function again.\n\n  * Change rename_ref() to retry if either\n    safe_create_leading_directories() returns SCLD_VANISHED or if its\n    own call to rename() returns ENOENT.\n\n* Fix a similar mkdir/rmdir race in lock_ref_sha1_basic().  This one\n  is actually more interesting than the one in rename_ref() because it\n  can be triggered internal to git.\n\n* Fix a race in remove_dir_recurse(): if somebody else deletes a file\n  or directory that the function wanted to delete anyway, don't treat\n  it as an error.\n\nThe main git-internal race that I know to be fixed by these changes is\nwhen pack-refs is trying to delete empty directories at the same time\nas another process is trying to create a new reference.  It can be\nreproduced by this script:\n\n    BRANCHES=\"foo/bar/xyzzy foo/bar/plugh\"\n    HEADS=\"h1 h2\"\n\n    rm -rf test-repo\n    $GIT init test-repo\n    cd test-repo\n    $GIT config user.name 'Lou User'\n    $GIT config user.email 'luser@example.com'\n\n    for h in $HEADS\n    do\n        $GIT commit --allow-empty -m $h\n        $GIT branch $h\n    done\n\n    (\n        while true\n        do\n    \tfor b in $BRANCHES\n    \tdo\n    \t    for h in $HEADS\n    \t    do\n    \t\t$GIT update-ref refs/heads/$b $h\n    \t    done\n    \tdone\n        done\n    ) &\n    pid1=$!\n\n    (\n        while true\n        do\n    \t$GIT pack-refs --all\n        done\n    ) &\n    pid2=$!\n\nThe script has to fail if update-ref and pack-refs try to lock the\nsame reference at the same time, because we don't handle lock\ncontention:\n\n    fatal: Unable to create '/home/mhagger/tmp/test-repo/.git/refs/heads/foo/bar/plugh.lock': File exists.\n\n    If no other git process is currently running, this probably means a\n    git process crashed in this repository earlier. Make sure no other git\n    process is running and remove the file manually to continue.\n\nAlso, if update-ref changes the value of a reference while pack-refs\nis running, then pack-refs emits a warning and leaves the new value of\nthe loose reference in place:\n\n    error: Ref refs/heads/foo/bar/xyzzy is at 003a9cedc3ec79b8f589b158ffe91177f0a611b3 but expected 34bf1e34d27a639732a01fef0a791e58c2c0882f\n\nBut it used to have other unnecessary failures, too, related to\nmkdir/rmdir races between the two programs:\n\n    error: unable to create directory for .git/refs/heads/foo/bar/plugh\n\nand\n\n    fatal: Unable to create '/home/mhagger/tmp/test-repo/.git/refs/heads/foo/bar/plugh.lock': No such file or directory\n\nThis patch series eliminates the last two types of failures.  Here are\ntallies of failures from running the above script for 60 seconds\nbefore and after this change.  Before:\n\n    Total updates: 46900\n    Unable to create '*.lock': File exists: 1992\n    * is at * but expected *: 940\n    unable to create directory: 187\n    Unable to create '*.lock': No such file or directory: 197\n\nAfter:\n\n    Total updates: 47892\n    Unable to create '*.lock': File exists: 2105\n    * is at * but expected *: 835\n    unable to create directory: 0\n    Unable to create '*.lock': No such file or directory: 0\n\nClearly, more work is still needed in this area.  For example, the \"*\nis at * but expected *\" errors are not really errors at all and should\nnot be reported as such.  And we might want to consider adding a short\ndelay and then a retry when acquiring locks.  If I run the same test\nscript with the following patch, then most or all of the \"Unable to\ncreate '*.lock': File exists\" errors go away too.\n\n======================================================================\ndiff --git a/refs.c b/refs.c\nindex 810f802..b3ff1f5 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2117,7 +2117,10 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n                         * again:\n                         */\n                        goto retry;\n-               else\n+               else if (errno == EEXIST && --attempts > 0) {\n+                       usleep(1000);\n+                       goto retry;\n+               } else\n                        unable_to_lock_index_die(ref_file, errno);\n        }\n        return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;\n======================================================================\n\nI hope to do some more work here in the near future.\n\n[1] Version 1: http://thread.gmane.org/gmane.comp.version-control.git/239638\n\nMichael Haggerty (17):\n  safe_create_leading_directories(): fix format of \"if\" chaining\n  safe_create_leading_directories(): reduce scope of local variable\n  safe_create_leading_directories(): add explicit \"slash\" pointer\n  safe_create_leading_directories(): rename local variable\n  safe_create_leading_directories(): split on first of multiple slashes\n  safe_create_leading_directories(): always restore slash at end of loop\n  safe_create_leading_directories(): introduce enum for return values\n  cmd_init_db(): when creating directories, handle errors conservatively\n  safe_create_leading_directories(): add new error value SCLD_VANISHED\n  lock_ref_sha1_basic(): on SCLD_VANISHED, retry\n  lock_ref_sha1_basic(): if locking fails with ENOENT, retry\n  remove_dir_recurse(): tighten condition for removing unreadable dir\n  remove_dir_recurse(): handle disappearing files and directories\n  rename_ref(): extract function rename_tmp_log()\n  rename_tmp_log(): handle a possible mkdir/rmdir race\n  rename_tmp_log(): limit the number of remote_empty_directories()\n    attempts\n  rename_tmp_log(): on SCLD_VANISHED, retry\n\n builtin/init-db.c |  9 +++---\n cache.h           | 25 +++++++++++++--\n dir.c             | 27 +++++++++++-----\n merge-recursive.c |  2 +-\n refs.c            | 92 ++++++++++++++++++++++++++++++++++++++++---------------\n sha1_file.c       | 67 ++++++++++++++++++++++------------------\n 6 files changed, 155 insertions(+), 67 deletions(-)\n\n-- \n1.8.5.2\n"},{"id":"232697","messageId":"1389015935-21936-2-git-send-email-mhagger@alum.mit.edu","threadId":"35614","inReplyTo":"1389015935-21936-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 01/17] safe_create_leading_directories(): fix format of \"if\" chaining","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-06T13:45:19Z","receivedAt":"2014-01-06T13:45:19Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\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.2\n"},{"id":"232696","messageId":"1389015935-21936-3-git-send-email-mhagger@alum.mit.edu","threadId":"35614","inReplyTo":"1389015935-21936-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 02/17] safe_create_leading_directories(): reduce scope of local variable","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-06T13:45:20Z","receivedAt":"2014-01-06T13:45:20Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This makes it more obvious that values of \"st\" don't persist across\nloop iterations.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\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.2\n"},{"id":"232707","messageId":"1389015935-21936-4-git-send-email-mhagger@alum.mit.edu","threadId":"35614","inReplyTo":"1389015935-21936-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 03/17] safe_create_leading_directories(): add explicit \"slash\" pointer","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-06T13:45:21Z","receivedAt":"2014-01-06T13:45:21Z","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 independently of\n\"pos\", thereby making the purpose of each variable clearer and\nworking towards other upcoming changes.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n sha1_file.c | 20 +++++++++++---------\n 1 file changed, 11 insertions(+), 9 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex cc9957e..197766d 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -111,19 +111,21 @@ int safe_create_leading_directories(char *path)\n \n \twhile (pos) {\n \t\tstruct stat st;\n+\t\tchar *slash = strchr(pos, '/');\n \n-\t\tpos = strchr(pos, '/');\n-\t\tif (!pos)\n+\t\tif (!slash)\n \t\t\tbreak;\n-\t\twhile (*++pos == '/')\n-\t\t\t;\n+\t\twhile (*(slash + 1) == '/')\n+\t\t\tslash++;\n+\t\tpos = slash + 1;\n \t\tif (!*pos)\n \t\t\tbreak;\n-\t\t*--pos = '\\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\t*slash = '/';\n \t\t\t\treturn -3;\n \t\t\t}\n \t\t} else if (mkdir(path, 0777)) {\n@@ -131,14 +133,14 @@ int safe_create_leading_directories(char *path)\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\t*slash = '/';\n \t\t\t\treturn -1;\n \t\t\t}\n \t\t} else if (adjust_shared_perm(path)) {\n-\t\t\t*pos = '/';\n+\t\t\t*slash = '/';\n \t\t\treturn -2;\n \t\t}\n-\t\t*pos++ = '/';\n+\t\t*slash = '/';\n \t}\n \treturn 0;\n }\n-- \n1.8.5.2\n"},{"id":"232704","messageId":"1389015935-21936-5-git-send-email-mhagger@alum.mit.edu","threadId":"35614","inReplyTo":"1389015935-21936-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 04/17] safe_create_leading_directories(): rename local variable","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-06T13:45:22Z","receivedAt":"2014-01-06T13:45:22Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Rename \"pos\" to \"next_component\", because now it always points at the\nnext component of the path name that has to be processed.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n sha1_file.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 197766d..54202e8 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -107,18 +107,18 @@ 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-\twhile (pos) {\n+\twhile (next_component) {\n \t\tstruct stat st;\n-\t\tchar *slash = strchr(pos, '/');\n+\t\tchar *slash = strchr(next_component, '/');\n \n \t\tif (!slash)\n \t\t\tbreak;\n \t\twhile (*(slash + 1) == '/')\n \t\t\tslash++;\n-\t\tpos = slash + 1;\n-\t\tif (!*pos)\n+\t\tnext_component = slash + 1;\n+\t\tif (!*next_component)\n \t\t\tbreak;\n \n \t\t*slash = '\\0';\n-- \n1.8.5.2\n"},{"id":"232695","messageId":"1389015935-21936-6-git-send-email-mhagger@alum.mit.edu","threadId":"35614","inReplyTo":"1389015935-21936-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 05/17] safe_create_leading_directories(): split on first of multiple slashes","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-06T13:45:23Z","receivedAt":"2014-01-06T13:45:23Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If the input path has multiple slashes between path components (e.g.,\n\"foo//bar\"), then the old code was breaking the path at the last\nslash, not the first one.  So in the above example, the second slash\nwas overwritten with NUL, resulting in the parent directory being\nsought as \"foo/\".\n\nWhen stat() is called on \"foo/\", it fails with ENOTDIR if \"foo\" exists\nbut is not a directory.  This caused the wrong path to be taken in the\nsubsequent logic.\n\nSo instead, split path components at the first intercomponent slash\nrather than the last one.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n sha1_file.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 54202e8..f3190c6 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -115,9 +115,10 @@ int safe_create_leading_directories(char *path)\n \n \t\tif (!slash)\n \t\t\tbreak;\n-\t\twhile (*(slash + 1) == '/')\n-\t\t\tslash++;\n+\n \t\tnext_component = slash + 1;\n+\t\twhile (*next_component == '/')\n+\t\t\tnext_component++;\n \t\tif (!*next_component)\n \t\t\tbreak;\n \n-- \n1.8.5.2\n"},{"id":"232709","messageId":"1389015935-21936-7-git-send-email-mhagger@alum.mit.edu","threadId":"35614","inReplyTo":"1389015935-21936-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 06/17] safe_create_leading_directories(): always restore slash at end of loop","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-06T13:45:24Z","receivedAt":"2014-01-06T13:45:24Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Always restore the slash that we scribbled over at the end of the\nloop, rather than also fixing it up at each premature exit from the\nloop.  This makes it harder to forget to do the cleanup as new paths\nare added to the code.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n sha1_file.c | 22 +++++++++-------------\n 1 file changed, 9 insertions(+), 13 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex f3190c6..4513cbb 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -108,8 +108,9 @@ 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 ret = 0;\n \n-\twhile (next_component) {\n+\twhile (!ret && next_component) {\n \t\tstruct stat st;\n \t\tchar *slash = strchr(next_component, '/');\n \n@@ -125,25 +126,20 @@ int safe_create_leading_directories(char *path)\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*slash = '/';\n-\t\t\t\treturn -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\t*slash = '/';\n-\t\t\t\treturn -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\t*slash = '/';\n-\t\t\treturn -2;\n+\t\t\tret = -2;\n \t\t}\n \t\t*slash = '/';\n \t}\n-\treturn 0;\n+\treturn ret;\n }\n \n int safe_create_leading_directories_const(const char *path)\n-- \n1.8.5.2\n"},{"id":"232708","messageId":"1389015935-21936-8-git-send-email-mhagger@alum.mit.edu","threadId":"35614","inReplyTo":"1389015935-21936-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 07/17] safe_create_leading_directories(): introduce enum for return values","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-06T13:45:25Z","receivedAt":"2014-01-06T13:45:25Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Instead of returning magic integer values (which a couple of callers\ngo to the trouble of distinguishing), return values from an enum.  Add\na docstring.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n builtin/init-db.c |  4 ++--\n cache.h           | 17 +++++++++++++++--\n merge-recursive.c |  2 +-\n sha1_file.c       | 16 ++++++++--------\n 4 files changed, 26 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex b3f03cf..6f69593 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -515,10 +515,10 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n \t\t\t\tsaved = shared_repository;\n \t\t\t\tshared_repository = 0;\n \t\t\t\tswitch (safe_create_leading_directories_const(argv[0])) {\n-\t\t\t\tcase -3:\n+\t\t\t\tcase SCLD_EXISTS:\n \t\t\t\t\terrno = EEXIST;\n \t\t\t\t\t/* fallthru */\n-\t\t\t\tcase -1:\n+\t\t\t\tcase SCLD_FAILED:\n \t\t\t\t\tdie_errno(_(\"cannot mkdir %s\"), argv[0]);\n \t\t\t\t\tbreak;\n \t\t\t\tdefault:\ndiff --git a/cache.h b/cache.h\nindex ce377e1..c6a4157 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -736,8 +736,21 @@ enum sharedrepo {\n };\n int git_config_perm(const char *var, const char *value);\n int adjust_shared_perm(const char *path);\n-int safe_create_leading_directories(char *path);\n-int safe_create_leading_directories_const(const char *path);\n+\n+/*\n+ * Create the directory containing the named path, using care to be\n+ * somewhat safe against races.  Return one of the scld_error values\n+ * to indicate success/failure.\n+ */\n+enum scld_error {\n+\tSCLD_OK = 0,\n+\tSCLD_FAILED = -1,\n+\tSCLD_PERMS = -2,\n+\tSCLD_EXISTS = -3\n+};\n+enum scld_error safe_create_leading_directories(char *path);\n+enum scld_error safe_create_leading_directories_const(const char *path);\n+\n int mkdir_in_gitdir(const char *path);\n extern void home_config_paths(char **global, char **xdg, char *file);\n extern char *expand_user_path(const char *path);\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex a18bd15..8400a8e 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -693,7 +693,7 @@ static int make_room_for_path(struct merge_options *o, const char *path)\n \t/* Make sure leading directories are created */\n \tstatus = safe_create_leading_directories_const(path);\n \tif (status) {\n-\t\tif (status == -3) {\n+\t\tif (status == SCLD_EXISTS) {\n \t\t\t/* something else exists */\n \t\t\terror(msg, path, _(\": perhaps a D/F conflict?\"));\n \t\t\treturn -1;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 4513cbb..5594f11 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -105,12 +105,12 @@ int mkdir_in_gitdir(const char *path)\n \treturn adjust_shared_perm(path);\n }\n \n-int safe_create_leading_directories(char *path)\n+enum scld_error safe_create_leading_directories(char *path)\n {\n \tchar *next_component = path + offset_1st_component(path);\n-\tint ret = 0;\n+\tenum scld_error ret = SCLD_OK;\n \n-\twhile (!ret && next_component) {\n+\twhile (ret == SCLD_OK && next_component) {\n \t\tstruct stat st;\n \t\tchar *slash = strchr(next_component, '/');\n \n@@ -127,26 +127,26 @@ int safe_create_leading_directories(char *path)\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\tret = -3;\n+\t\t\t\tret = SCLD_EXISTS;\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\telse\n-\t\t\t\tret = -1;\n+\t\t\t\tret = SCLD_FAILED;\n \t\t} else if (adjust_shared_perm(path)) {\n-\t\t\tret = -2;\n+\t\t\tret = SCLD_PERMS;\n \t\t}\n \t\t*slash = '/';\n \t}\n \treturn ret;\n }\n \n-int safe_create_leading_directories_const(const char *path)\n+enum scld_error safe_create_leading_directories_const(const char *path)\n {\n \t/* path points to cache entries, so xstrdup before messing with it */\n \tchar *buf = xstrdup(path);\n-\tint result = safe_create_leading_directories(buf);\n+\tenum scld_error result = safe_create_leading_directories(buf);\n \tfree(buf);\n \treturn result;\n }\n-- \n1.8.5.2\n"},{"id":"232699","messageId":"1389015935-21936-9-git-send-email-mhagger@alum.mit.edu","threadId":"35614","inReplyTo":"1389015935-21936-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 08/17] cmd_init_db(): when creating directories, handle errors conservatively","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-06T13:45:26Z","receivedAt":"2014-01-06T13:45:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"safe_create_leading_directories_const() returns a non-zero value on\nerror.  The old code at this calling site recognized a couple of\nparticular error values, and treated all other return values as\nsuccess.  Instead, be more conservative: recognize the errors we are\ninterested in, but treat any other nonzero values as failures.  This\nis more robust in case somebody adds another possible return value\nwithout telling us.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n builtin/init-db.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 6f69593..c7c76bb 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -515,13 +515,14 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n \t\t\t\tsaved = shared_repository;\n \t\t\t\tshared_repository = 0;\n \t\t\t\tswitch (safe_create_leading_directories_const(argv[0])) {\n+\t\t\t\tcase SCLD_OK:\n+\t\t\t\tcase SCLD_PERMS:\n+\t\t\t\t\tbreak;\n \t\t\t\tcase SCLD_EXISTS:\n \t\t\t\t\terrno = EEXIST;\n \t\t\t\t\t/* fallthru */\n-\t\t\t\tcase SCLD_FAILED:\n-\t\t\t\t\tdie_errno(_(\"cannot mkdir %s\"), argv[0]);\n-\t\t\t\t\tbreak;\n \t\t\t\tdefault:\n+\t\t\t\t\tdie_errno(_(\"cannot mkdir %s\"), argv[0]);\n \t\t\t\t\tbreak;\n \t\t\t\t}\n \t\t\t\tshared_repository = saved;\n-- \n1.8.5.2\n"},{"id":"232698","messageId":"1389015935-21936-10-git-send-email-mhagger@alum.mit.edu","threadId":"35614","inReplyTo":"1389015935-21936-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 09/17] safe_create_leading_directories(): add new error value SCLD_VANISHED","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-06T13:45:27Z","receivedAt":"2014-01-06T13:45:27Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Add a new possible error result that can be returned by\nsafe_create_leading_directories() and\nsafe_create_leading_directories_const(): SCLD_VANISHED.  This value\nindicates that a file or directory on the path existed at one point\n(either it already existed or the function created it), but then it\ndisappeared.  This probably indicates that another process deleted the\ndirectory while we were working.  If SCLD_VANISHED is returned, the\ncaller might want to retry the function call, as there is a chance\nthat a new attempt will succeed.\n\nWhy doesn't safe_create_leading_directories() do the retrying\ninternally?  Because an empty directory isn't really ever safe until\nit holds a file.  So even if safe_create_leading_directories() were\nabsolutely sure that the directory existed before it returned, there\nwould be no guarantee that the directory still existed when the caller\ntried to write something in it.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n cache.h     | 10 +++++++++-\n sha1_file.c | 11 +++++++++++\n 2 files changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/cache.h b/cache.h\nindex c6a4157..f34c0a7 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -741,12 +741,20 @@ int adjust_shared_perm(const char *path);\n  * Create the directory containing the named path, using care to be\n  * somewhat safe against races.  Return one of the scld_error values\n  * to indicate success/failure.\n+ *\n+ * SCLD_VANISHED indicates that one of the ancestor directories of the\n+ * path existed at one point during the function call and then\n+ * suddenly vanished, probably because another process pruned the\n+ * directory while we were working.  To be robust against this kind of\n+ * race, callers might want to try invoking the function again when it\n+ * returns SCLD_VANISHED.\n  */\n enum scld_error {\n \tSCLD_OK = 0,\n \tSCLD_FAILED = -1,\n \tSCLD_PERMS = -2,\n-\tSCLD_EXISTS = -3\n+\tSCLD_EXISTS = -3,\n+\tSCLD_VANISHED = -4\n };\n enum scld_error safe_create_leading_directories(char *path);\n enum scld_error safe_create_leading_directories_const(const char *path);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 5594f11..69a4e95 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -132,6 +132,17 @@ enum scld_error 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\telse if (errno == ENOENT)\n+\t\t\t\t/*\n+\t\t\t\t * Either mkdir() failed because\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, inform\n+\t\t\t\t * the caller that it might be worth\n+\t\t\t\t * trying again:\n+\t\t\t\t */\n+\t\t\t\tret = SCLD_VANISHED;\n \t\t\telse\n \t\t\t\tret = SCLD_FAILED;\n \t\t} else if (adjust_shared_perm(path)) {\n-- \n1.8.5.2\n"},{"id":"293745","messageId":"1389015935-21936-11-git-send-email-mhagger@alum.mit.edu","threadId":"35614","inReplyTo":"1389015935-21936-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 10/17] lock_ref_sha1_basic(): on SCLD_VANISHED, retry","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-06T13:45:28Z","receivedAt":"2014-01-06T13:45:28Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If safe_create_leading_directories() fails because a file along the\npath unexpectedly vanished, try again (up to 3 times).\n\nThis can occur if another process is deleting directories at the same\ntime as we are trying to make them.  For example, \"git pack-refs\n--all\" tries to delete the loose refs and any empty directories that\nare left behind.  If a pack-refs process is running, then it might\ndelete a directory that we need to put a new loose reference in.\n\nIf safe_create_leading_directories() thinks this might have happened,\nthen take its advice and try again (maximum three attempts).\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c | 11 ++++++++++-\n 1 file changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 3926136..6eb8a02 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2039,6 +2039,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \tint type, lflags;\n \tint mustexist = (old_sha1 && !is_null_sha1(old_sha1));\n \tint missing = 0;\n+\tint attempts = 3;\n \n \tlock = xcalloc(1, sizeof(struct ref_lock));\n \tlock->lock_fd = -1;\n@@ -2093,7 +2094,15 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \tif ((flags & REF_NODEREF) && (type & REF_ISSYMREF))\n \t\tlock->force_write = 1;\n \n-\tif (safe_create_leading_directories(ref_file)) {\n+ retry:\n+\tswitch (safe_create_leading_directories(ref_file)) {\n+\tcase SCLD_OK:\n+\t\tbreak; /* success */\n+\tcase SCLD_VANISHED:\n+\t\tif (--attempts > 0)\n+\t\t\tgoto retry;\n+\t\t/* fall through */\n+\tdefault:\n \t\tlast_errno = errno;\n \t\terror(\"unable to create directory for %s\", ref_file);\n \t\tgoto error_return;\n-- \n1.8.5.2\n"},{"id":"232700","messageId":"1389015935-21936-12-git-send-email-mhagger@alum.mit.edu","threadId":"35614","inReplyTo":"1389015935-21936-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 11/17] lock_ref_sha1_basic(): if locking fails with ENOENT, retry","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-06T13:45:29Z","receivedAt":"2014-01-06T13:45:29Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If hold_lock_file_for_update() fails with errno==ENOENT, it might be\nbecause somebody else (for example, a pack-refs process) has just\ndeleted one of the lockfile's ancestor directories.  So if this\ncondition is detected, try again (up to 3 times).\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c | 13 ++++++++++++-\n 1 file changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 6eb8a02..8a3d3ea 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2081,7 +2081,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \n \tlock->lk = xcalloc(1, sizeof(struct lock_file));\n \n-\tlflags = LOCK_DIE_ON_ERROR;\n+\tlflags = 0;\n \tif (flags & REF_NODEREF) {\n \t\trefname = orig_refname;\n \t\tlflags |= LOCK_NODEREF;\n@@ -2109,6 +2109,17 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t}\n \n \tlock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);\n+\tif (lock->lock_fd < 0) {\n+\t\tif (errno == ENOENT && --attempts > 0)\n+\t\t\t/*\n+\t\t\t * Maybe somebody just deleted one of the\n+\t\t\t * directories leading to ref_file.  Try\n+\t\t\t * again:\n+\t\t\t */\n+\t\t\tgoto retry;\n+\t\telse\n+\t\t\tunable_to_lock_index_die(ref_file, errno);\n+\t}\n \treturn old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;\n \n  error_return:\n-- \n1.8.5.2\n"},{"id":"232702","messageId":"1389015935-21936-13-git-send-email-mhagger@alum.mit.edu","threadId":"35614","inReplyTo":"1389015935-21936-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 12/17] remove_dir_recurse(): tighten condition for removing unreadable dir","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-06T13:45:30Z","receivedAt":"2014-01-06T13:45:30Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If opendir() fails on the top-level directory, it makes sense to try\nto delete it anyway--but only if the failure was due to EACCES.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n dir.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 23b6de4..11e1520 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1476,8 +1476,11 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n \tflag &= ~REMOVE_DIR_KEEP_TOPLEVEL;\n \tdir = opendir(path->buf);\n \tif (!dir) {\n-\t\t/* an empty dir could be removed even if it is unreadble */\n-\t\tif (!keep_toplevel)\n+\t\tif (errno == EACCES && !keep_toplevel)\n+\t\t\t/*\n+\t\t\t * An empty dir could be removable even if it\n+\t\t\t * is unreadable:\n+\t\t\t */\n \t\t\treturn rmdir(path->buf);\n \t\telse\n \t\t\treturn -1;\n-- \n1.8.5.2\n"},{"id":"232701","messageId":"1389015935-21936-14-git-send-email-mhagger@alum.mit.edu","threadId":"35614","inReplyTo":"1389015935-21936-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 13/17] remove_dir_recurse(): handle disappearing files and directories","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-06T13:45:31Z","receivedAt":"2014-01-06T13:45:31Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If a file or directory that we are trying to remove disappears (e.g.,\nbecause another process has pruned it), do not consider it an error.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n dir.c | 22 ++++++++++++++++------\n 1 file changed, 16 insertions(+), 6 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 11e1520..716b613 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1476,7 +1476,9 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n \tflag &= ~REMOVE_DIR_KEEP_TOPLEVEL;\n \tdir = opendir(path->buf);\n \tif (!dir) {\n-\t\tif (errno == EACCES && !keep_toplevel)\n+\t\tif (errno == ENOENT)\n+\t\t\treturn keep_toplevel ? -1 : 0;\n+\t\telse if (errno == EACCES && !keep_toplevel)\n \t\t\t/*\n \t\t\t * An empty dir could be removable even if it\n \t\t\t * is unreadable:\n@@ -1496,13 +1498,21 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n \n \t\tstrbuf_setlen(path, len);\n \t\tstrbuf_addstr(path, e->d_name);\n-\t\tif (lstat(path->buf, &st))\n-\t\t\t; /* fall thru */\n-\t\telse if (S_ISDIR(st.st_mode)) {\n+\t\tif (lstat(path->buf, &st)) {\n+\t\t\tif (errno == ENOENT)\n+\t\t\t\t/*\n+\t\t\t\t * file disappeared, which is what we\n+\t\t\t\t * wanted anyway\n+\t\t\t\t */\n+\t\t\t\tcontinue;\n+\t\t\t/* fall thru */\n+\t\t} else if (S_ISDIR(st.st_mode)) {\n \t\t\tif (!remove_dir_recurse(path, flag, &kept_down))\n \t\t\t\tcontinue; /* happy */\n-\t\t} else if (!only_empty && !unlink(path->buf))\n+\t\t} else if (!only_empty &&\n+\t\t\t   (!unlink(path->buf) || errno == ENOENT)) {\n \t\t\tcontinue; /* happy, too */\n+\t\t}\n \n \t\t/* path too long, stat fails, or non-directory still exists */\n \t\tret = -1;\n@@ -1512,7 +1522,7 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n \n \tstrbuf_setlen(path, original_len);\n \tif (!ret && !keep_toplevel && !kept_down)\n-\t\tret = rmdir(path->buf);\n+\t\tret = (!rmdir(path->buf) || errno == ENOENT) ? 0 : -1;\n \telse if (kept_up)\n \t\t/*\n \t\t * report the uplevel that it is not an error that we\n-- \n1.8.5.2\n"},{"id":"232710","messageId":"1389015935-21936-15-git-send-email-mhagger@alum.mit.edu","threadId":"35614","inReplyTo":"1389015935-21936-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 14/17] rename_ref(): extract function rename_tmp_log()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-06T13:45:32Z","receivedAt":"2014-01-06T13:45:32Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It's about to become a bit more complex.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c | 52 ++++++++++++++++++++++++++++++----------------------\n 1 file changed, 30 insertions(+), 22 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 8a3d3ea..5bc01a7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2528,6 +2528,35 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n  */\n #define TMP_RENAMED_LOG  \"logs/refs/.tmp-renamed-log\"\n \n+static int rename_tmp_log(const char *newrefname)\n+{\n+\tif (safe_create_leading_directories(git_path(\"logs/%s\", newrefname))) {\n+\t\terror(\"unable to create directory for %s\", newrefname);\n+\t\treturn -1;\n+\t}\n+\n+ retry:\n+\tif (rename(git_path(TMP_RENAMED_LOG), git_path(\"logs/%s\", newrefname))) {\n+\t\tif (errno==EISDIR || errno==ENOTDIR) {\n+\t\t\t/*\n+\t\t\t * rename(a, b) when b is an existing\n+\t\t\t * directory ought to result in ISDIR, but\n+\t\t\t * Solaris 5.8 gives ENOTDIR.  Sheesh.\n+\t\t\t */\n+\t\t\tif (remove_empty_directories(git_path(\"logs/%s\", newrefname))) {\n+\t\t\t\terror(\"Directory not empty: logs/%s\", newrefname);\n+\t\t\t\treturn -1;\n+\t\t\t}\n+\t\t\tgoto retry;\n+\t\t} else {\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\treturn -1;\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n+\n int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsg)\n {\n \tunsigned char sha1[20], orig_sha1[20];\n@@ -2575,30 +2604,9 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \t\t}\n \t}\n \n-\tif (log && safe_create_leading_directories(git_path(\"logs/%s\", newrefname))) {\n-\t\terror(\"unable to create directory for %s\", newrefname);\n+\tif (log && rename_tmp_log(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-\t\t\t * rename(a, b) when b is an existing\n-\t\t\t * directory ought to result in ISDIR, but\n-\t\t\t * Solaris 5.8 gives ENOTDIR.  Sheesh.\n-\t\t\t */\n-\t\t\tif (remove_empty_directories(git_path(\"logs/%s\", newrefname))) {\n-\t\t\t\terror(\"Directory not empty: logs/%s\", newrefname);\n-\t\t\t\tgoto rollback;\n-\t\t\t}\n-\t\t\tgoto retry;\n-\t\t} else {\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-\t\t}\n-\t}\n \tlogmoved = log;\n \n \tlock = lock_ref_sha1_basic(newrefname, NULL, 0, NULL);\n-- \n1.8.5.2\n"},{"id":"232711","messageId":"1389015935-21936-16-git-send-email-mhagger@alum.mit.edu","threadId":"35614","inReplyTo":"1389015935-21936-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 15/17] rename_tmp_log(): handle a possible mkdir/rmdir race","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-06T13:45:33Z","receivedAt":"2014-01-06T13:45:33Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If a directory vanishes while renaming the temporary reflog file,\nretry (up to 3 times).  This could happen if another process deletes\nthe directory created by safe_create_leading_directories() just before\nwe rename the file into the directory.\n\nAs far as I can tell, this race could not occur internal to git.  The\nonly time that a directory under $GIT_DIR/logs is deleted is if room\nhas to be made for a log file for a reference with the same name;\nfor example, in the following sequence:\n\n    git branch foo/bar    # Creates file .git/logs/refs/heads/foo/bar\n    git branch -d foo/bar # Deletes file but leaves .git/logs/refs/heads/foo/\n    git branch foo        # Deletes .git/logs/refs/heads/foo/\n\nBut the only reason the last command deletes the directory is because\nit wants to create a file with the same name.  So if another process\n(e.g.,\n\n    git branch foo/baz\n\n) wants to create that directory, one of the two is doomed to failure\nanyway because of a D/F conflict.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c | 11 ++++++++++-\n 1 file changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 5bc01a7..8de636e 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2530,12 +2530,14 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n \n static int rename_tmp_log(const char *newrefname)\n {\n+\tint attempts = 3;\n+\n+ retry:\n \tif (safe_create_leading_directories(git_path(\"logs/%s\", newrefname))) {\n \t\terror(\"unable to create directory for %s\", newrefname);\n \t\treturn -1;\n \t}\n \n- retry:\n \tif (rename(git_path(TMP_RENAMED_LOG), git_path(\"logs/%s\", newrefname))) {\n \t\tif (errno==EISDIR || errno==ENOTDIR) {\n \t\t\t/*\n@@ -2548,6 +2550,13 @@ static int rename_tmp_log(const char *newrefname)\n \t\t\t\treturn -1;\n \t\t\t}\n \t\t\tgoto retry;\n+\t\t} else if (errno == ENOENT && --attempts > 0) {\n+\t\t\t/*\n+\t\t\t * Maybe another process just deleted one of\n+\t\t\t * the directories in the path to newrefname.\n+\t\t\t * Try again from the beginning.\n+\t\t\t */\n+\t\t\tgoto retry;\n \t\t} else {\n \t\t\terror(\"unable to move logfile \"TMP_RENAMED_LOG\" to logs/%s: %s\",\n \t\t\t\tnewrefname, strerror(errno));\n-- \n1.8.5.2\n"},{"id":"232703","messageId":"1389015935-21936-17-git-send-email-mhagger@alum.mit.edu","threadId":"35614","inReplyTo":"1389015935-21936-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 16/17] rename_tmp_log(): limit the number of remote_empty_directories() attempts","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-06T13:45:34Z","receivedAt":"2014-01-06T13:45:34Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This doesn't seem to be a likely error, but we've got the counter\nanyway, so we might as well use it for an added bit of safety.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 8de636e..490525a 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2530,7 +2530,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n \n static int rename_tmp_log(const char *newrefname)\n {\n-\tint attempts = 3;\n+\tint attempts = 4;\n \n  retry:\n \tif (safe_create_leading_directories(git_path(\"logs/%s\", newrefname))) {\n@@ -2539,7 +2539,7 @@ static int rename_tmp_log(const char *newrefname)\n \t}\n \n \tif (rename(git_path(TMP_RENAMED_LOG), git_path(\"logs/%s\", newrefname))) {\n-\t\tif (errno==EISDIR || errno==ENOTDIR) {\n+\t\tif ((errno==EISDIR || errno==ENOTDIR) && --attempts > 0) {\n \t\t\t/*\n \t\t\t * rename(a, b) when b is an existing\n \t\t\t * directory ought to result in ISDIR, but\n-- \n1.8.5.2\n"},{"id":"232705","messageId":"1389015935-21936-18-git-send-email-mhagger@alum.mit.edu","threadId":"35614","inReplyTo":"1389015935-21936-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 17/17] rename_tmp_log(): on SCLD_VANISHED, retry","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-06T13:45:35Z","receivedAt":"2014-01-06T13:45:35Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If safe_create_leading_directories() fails because a file along the\npath unexpectedly vanished, try again from the beginning.  Try at most\n3 times.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 490525a..810f802 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2533,7 +2533,14 @@ static int rename_tmp_log(const char *newrefname)\n \tint attempts = 4;\n \n  retry:\n-\tif (safe_create_leading_directories(git_path(\"logs/%s\", newrefname))) {\n+\tswitch (safe_create_leading_directories(git_path(\"logs/%s\", newrefname))) {\n+\tcase SCLD_OK:\n+\t\tbreak; /* success */\n+\tcase SCLD_VANISHED:\n+\t\tif (--attempts > 0)\n+\t\t\tgoto retry;\n+\t\t/* fall through */\n+\tdefault:\n \t\terror(\"unable to create directory for %s\", newrefname);\n \t\treturn -1;\n \t}\n-- \n1.8.5.2\n"},{"id":"232741","messageId":"xmqq4n5hj8ry.fsf@gitster.dls.corp.google.com","threadId":"35614","inReplyTo":"1389015935-21936-11-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 10/17] lock_ref_sha1_basic(): on SCLD_VANISHED, retry","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-06T17:54:25Z","receivedAt":"2014-01-06T17:54:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> If safe_create_leading_directories() fails because a file along the\n> path unexpectedly vanished, try again (up to 3 times).\n>\n> This can occur if another process is deleting directories at the same\n> time as we are trying to make them.  For example, \"git pack-refs\n> --all\" tries to delete the loose refs and any empty directories that\n> are left behind.  If a pack-refs process is running, then it might\n> delete a directory that we need to put a new loose reference in.\n>\n> If safe_create_leading_directories() thinks this might have happened,\n> then take its advice and try again (maximum three attempts).\n>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n>  refs.c | 11 ++++++++++-\n>  1 file changed, 10 insertions(+), 1 deletion(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 3926136..6eb8a02 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2039,6 +2039,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n>  \tint type, lflags;\n>  \tint mustexist = (old_sha1 && !is_null_sha1(old_sha1));\n>  \tint missing = 0;\n> +\tint attempts = 3;\n>  \n>  \tlock = xcalloc(1, sizeof(struct ref_lock));\n>  \tlock->lock_fd = -1;\n> @@ -2093,7 +2094,15 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n>  \tif ((flags & REF_NODEREF) && (type & REF_ISSYMREF))\n>  \t\tlock->force_write = 1;\n>  \n> -\tif (safe_create_leading_directories(ref_file)) {\n> + retry:\n> +\tswitch (safe_create_leading_directories(ref_file)) {\n> +\tcase SCLD_OK:\n> +\t\tbreak; /* success */\n> +\tcase SCLD_VANISHED:\n> +\t\tif (--attempts > 0)\n> +\t\t\tgoto retry;\n> +\t\t/* fall through */\n\nHmph.\n\nHaving no backoff/sleep at all might be OK here as long as the other\nside that removes does not retry (and I do not think the other side\nwould, even though I haven't read through the series to the end yet\n;-)).\n\nThis may be just a style thing, but I find that the variable name\n\"attempts\" that starts out as 3 quite misleading, as its value is\nnot \"the number of attempts made\" but \"the remaining number of\nattempts allowed.\"  Starting it from 0 and then\n\n\tif (attempts++ < MAX_ATTEMPTS)\n\t\tgoto retry;\n\nwould be one way to clarify it.  Renaming it to remaining_attempts\nwould be another.\n\n> +\tdefault:\n>  \t\tlast_errno = errno;\n>  \t\terror(\"unable to create directory for %s\", ref_file);\n>  \t\tgoto error_return;\n"},{"id":"232742","messageId":"xmqqy52tht2l.fsf@gitster.dls.corp.google.com","threadId":"35614","inReplyTo":"1389015935-21936-14-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 13/17] remove_dir_recurse(): handle disappearing files and directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-06T18:18:58Z","receivedAt":"2014-01-06T18:18:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> If a file or directory that we are trying to remove disappears (e.g.,\n> because another process has pruned it), do not consider it an error.\n>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n>  dir.c | 22 ++++++++++++++++------\n>  1 file changed, 16 insertions(+), 6 deletions(-)\n>\n> diff --git a/dir.c b/dir.c\n> index 11e1520..716b613 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -1476,7 +1476,9 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n>  \tflag &= ~REMOVE_DIR_KEEP_TOPLEVEL;\n>  \tdir = opendir(path->buf);\n>  \tif (!dir) {\n> -\t\tif (errno == EACCES && !keep_toplevel)\n> +\t\tif (errno == ENOENT)\n> +\t\t\treturn keep_toplevel ? -1 : 0;\n\nIf we were told that \"foo/bar must go, but do not bother removing\nfoo/\", is it an error if foo itself did not exist?  I think this\nstep does not change the behaviour from the original (we used to say\n\"oh, we were told to keep_toplevel, and the top-level cannot be read\nfor whatever reason, so it is an error\"), but because this series is\ngiving us a finer grained error diagnosis, we may want to think\nabout it further, perhaps as a follow-up.\n\nI also wonder why the keep-toplevel logic is in this \"recurse\" part\nof the callchain. If everything in \"foo/bar/\" can be removed but\n\"foo/bar/\" is unreadable, it is OK, when opendir(\"foo/bar\") failed\nwith EACCESS, to attempt to rmdir(\"foo/bar\") whether we were told\nnot to attempt removing \"foo/\", no?\n\n> +\t\telse if (errno == EACCES && !keep_toplevel)\n\nThat is, I am wondering why this part just checks !keep_toplevel,\nnot\n\n\t(!keep_toplevel || has_dir_separator(path->buf))\n\nor something.\n\n>  \t\t\t/*\n>  \t\t\t * An empty dir could be removable even if it\n>  \t\t\t * is unreadable:\n> @@ -1496,13 +1498,21 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n>  \n>  \t\tstrbuf_setlen(path, len);\n>  \t\tstrbuf_addstr(path, e->d_name);\n> -\t\tif (lstat(path->buf, &st))\n> -\t\t\t; /* fall thru */\n> -\t\telse if (S_ISDIR(st.st_mode)) {\n> +\t\tif (lstat(path->buf, &st)) {\n> +\t\t\tif (errno == ENOENT)\n> +\t\t\t\t/*\n> +\t\t\t\t * file disappeared, which is what we\n> +\t\t\t\t * wanted anyway\n> +\t\t\t\t */\n> +\t\t\t\tcontinue;\n> +\t\t\t/* fall thru */\n> +\t\t} else if (S_ISDIR(st.st_mode)) {\n>  \t\t\tif (!remove_dir_recurse(path, flag, &kept_down))\n>  \t\t\t\tcontinue; /* happy */\n> -\t\t} else if (!only_empty && !unlink(path->buf))\n> +\t\t} else if (!only_empty &&\n> +\t\t\t   (!unlink(path->buf) || errno == ENOENT)) {\n>  \t\t\tcontinue; /* happy, too */\n> +\t\t}\n>  \n>  \t\t/* path too long, stat fails, or non-directory still exists */\n>  \t\tret = -1;\n> @@ -1512,7 +1522,7 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n>  \n>  \tstrbuf_setlen(path, original_len);\n>  \tif (!ret && !keep_toplevel && !kept_down)\n> -\t\tret = rmdir(path->buf);\n> +\t\tret = (!rmdir(path->buf) || errno == ENOENT) ? 0 : -1;\n>  \telse if (kept_up)\n>  \t\t/*\n>  \t\t * report the uplevel that it is not an error that we\n"},{"id":"232743","messageId":"xmqqtxdhhsyq.fsf@gitster.dls.corp.google.com","threadId":"35614","inReplyTo":"1389015935-21936-18-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 17/17] rename_tmp_log(): on SCLD_VANISHED, retry","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-06T18:21:17Z","receivedAt":"2014-01-06T18:21:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> If safe_create_leading_directories() fails because a file along the\n> path unexpectedly vanished, try again from the beginning.  Try at most\n> 3 times.\n\nAs the previous step bumped it from 3 to 4 without explanation, the\nabove no longer reflects reality ;-)\n\nThe series mostly looked sane from a cursory read.\n\nWill re-queue.  Thanks.\n\n\n>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n>  refs.c | 9 ++++++++-\n>  1 file changed, 8 insertions(+), 1 deletion(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 490525a..810f802 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2533,7 +2533,14 @@ static int rename_tmp_log(const char *newrefname)\n>  \tint attempts = 4;\n>  \n>   retry:\n> -\tif (safe_create_leading_directories(git_path(\"logs/%s\", newrefname))) {\n> +\tswitch (safe_create_leading_directories(git_path(\"logs/%s\", newrefname))) {\n> +\tcase SCLD_OK:\n> +\t\tbreak; /* success */\n> +\tcase SCLD_VANISHED:\n> +\t\tif (--attempts > 0)\n> +\t\t\tgoto retry;\n> +\t\t/* fall through */\n> +\tdefault:\n>  \t\terror(\"unable to create directory for %s\", newrefname);\n>  \t\treturn -1;\n>  \t}\n"},{"id":"232744","messageId":"xmqqppo5hsf9.fsf@gitster.dls.corp.google.com","threadId":"35614","inReplyTo":"1389015935-21936-4-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 03/17] safe_create_leading_directories(): add explicit \"slash\" pointer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-06T18:32:58Z","receivedAt":"2014-01-06T18:32:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> Keep track of the position of the slash character independently of\n> \"pos\", thereby making the purpose of each variable clearer and\n> working towards other upcoming changes.\n>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n\nThis step has an interaction with $gmane/239878 where Windows folks\nwant it to pay attention to is_dir_sep()---over there, a backslash\ncould separate directory path components.\n\nAFAIK, the function was meant to be used only on paths we internally\ngenerate, and the paths we internally generate all are slash\nseparated, so it could be argued that feeding a path, whose path\ncomponents are separated by backslashes, that we obtained from the\nend user without converting it to the internal form in some\ncodepaths (e.g. \"$there\" in \"git clone $url $there\") are bugs we\nacquired over time that need to be fixed, but it is easy enough to\nuse is_dir_sep() here to work it around, and doing so will\nnot negatively affect\n\n 1. UNIX-only projects by forbidding use of a byte with backslash in\n    it as a path component character (yes, I am imagining using\n    Shift-JIS that can use a backslash as the second byte of\n    two-byte character in the pathname on UNIX); and\n\n 2. UNIX-and-Windows mixed projects, as you cannot sanely use such a\n    pathname with backslash as part of a path component if its tree\n    needs to be checked out on Windows.\n\n\n>  sha1_file.c | 20 +++++++++++---------\n>  1 file changed, 11 insertions(+), 9 deletions(-)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index cc9957e..197766d 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -111,19 +111,21 @@ int safe_create_leading_directories(char *path)\n>  \n>  \twhile (pos) {\n>  \t\tstruct stat st;\n> +\t\tchar *slash = strchr(pos, '/');\n>  \n> -\t\tpos = strchr(pos, '/');\n> -\t\tif (!pos)\n> +\t\tif (!slash)\n>  \t\t\tbreak;\n> -\t\twhile (*++pos == '/')\n> -\t\t\t;\n> +\t\twhile (*(slash + 1) == '/')\n> +\t\t\tslash++;\n> +\t\tpos = slash + 1;\n>  \t\tif (!*pos)\n>  \t\t\tbreak;\n> -\t\t*--pos = '\\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\t*slash = '/';\n>  \t\t\t\treturn -3;\n>  \t\t\t}\n>  \t\t} else if (mkdir(path, 0777)) {\n> @@ -131,14 +133,14 @@ int safe_create_leading_directories(char *path)\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\t*slash = '/';\n>  \t\t\t\treturn -1;\n>  \t\t\t}\n>  \t\t} else if (adjust_shared_perm(path)) {\n> -\t\t\t*pos = '/';\n> +\t\t\t*slash = '/';\n>  \t\t\treturn -2;\n>  \t\t}\n> -\t\t*pos++ = '/';\n> +\t\t*slash = '/';\n>  \t}\n>  \treturn 0;\n>  }\n"},{"id":"232802","messageId":"52CBC854.2060602@alum.mit.edu","threadId":"35614","inReplyTo":"xmqqppo5hsf9.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 03/17] safe_create_leading_directories(): add explicit \"slash\" pointer","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-07T09:26:44Z","receivedAt":"2014-01-07T09:26:44Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 01/06/2014 07:32 PM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n>> Keep track of the position of the slash character independently of\n>> \"pos\", thereby making the purpose of each variable clearer and\n>> working towards other upcoming changes.\n>>\n>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n>> ---\n> \n> This step has an interaction with $gmane/239878 where Windows folks\n> want it to pay attention to is_dir_sep()---over there, a backslash\n> could separate directory path components.\n> \n> AFAIK, the function was meant to be used only on paths we internally\n> generate, and the paths we internally generate all are slash\n> separated, so it could be argued that feeding a path, whose path\n> components are separated by backslashes, that we obtained from the\n> end user without converting it to the internal form in some\n> codepaths (e.g. \"$there\" in \"git clone $url $there\") are bugs we\n> acquired over time that need to be fixed, but it is easy enough to\n> use is_dir_sep() here to work it around, and doing so will\n> not negatively affect\n> \n>  1. UNIX-only projects by forbidding use of a byte with backslash in\n>     it as a path component character (yes, I am imagining using\n>     Shift-JIS that can use a backslash as the second byte of\n>     two-byte character in the pathname on UNIX); and\n> \n>  2. UNIX-and-Windows mixed projects, as you cannot sanely use such a\n>     pathname with backslash as part of a path component if its tree\n>     needs to be checked out on Windows.\n\nI agree that it would be reasonable to use is_dir_sep() in the\nimplementation of this function, at least unless/until somebody does the\nwork to figure out whether callers should really only be passing it\nforward-slash-normalized paths.\n\nPlease be careful, though, because I don't think this function is\ncapable of handling arbitrary Windows paths, like for example\n//host/path format, either before or after my change.\n\nLet me know if you would like me to merge or rebase the is_dir_sep()\nchanges into this patch series.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"232803","messageId":"52CBD1D5.5080701@alum.mit.edu","threadId":"35614","inReplyTo":"xmqqy52tht2l.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 13/17] remove_dir_recurse(): handle disappearing files and directories","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-07T10:07:17Z","receivedAt":"2014-01-07T10:07:17Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 01/06/2014 07:18 PM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n>> If a file or directory that we are trying to remove disappears (e.g.,\n>> because another process has pruned it), do not consider it an error.\n>>\n>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n>> ---\n>>  dir.c | 22 ++++++++++++++++------\n>>  1 file changed, 16 insertions(+), 6 deletions(-)\n>>\n>> diff --git a/dir.c b/dir.c\n>> index 11e1520..716b613 100644\n>> --- a/dir.c\n>> +++ b/dir.c\n>> @@ -1476,7 +1476,9 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n>>  \tflag &= ~REMOVE_DIR_KEEP_TOPLEVEL;\n>>  \tdir = opendir(path->buf);\n>>  \tif (!dir) {\n>> -\t\tif (errno == EACCES && !keep_toplevel)\n>> +\t\tif (errno == ENOENT)\n>> +\t\t\treturn keep_toplevel ? -1 : 0;\n> \n> If we were told that \"foo/bar must go, but do not bother removing\n> foo/\", is it an error if foo itself did not exist?  I think this\n> step does not change the behaviour from the original (we used to say\n> \"oh, we were told to keep_toplevel, and the top-level cannot be read\n> for whatever reason, so it is an error\"), but because this series is\n> giving us a finer grained error diagnosis, we may want to think\n> about it further, perhaps as a follow-up.\n\nIndeed, this is a design choice that I should have explained.  I\nimplemented it this way to keep the behavior the same as the old code in\nthis situation, and because I thought that if the caller explicitly asks\nfor the toplevel path to be kept, and that path doesn't even exist at\nthe entrance to the function, then something is screwy and it is better\nto return an error than to keep going.\n\nIt would even be possible to argue that if keep_toplevel is set but path\nis missing, then this function should call\nsafe_create_leading_directories(path).\n\nChanging this behavior would require an audit to see which behavior\nwould be most useful to the callers.  I think that is out of the scope\nof this patch series.\n\n> I also wonder why the keep-toplevel logic is in this \"recurse\" part\n> of the callchain. If everything in \"foo/bar/\" can be removed but\n> \"foo/bar/\" is unreadable, it is OK, when opendir(\"foo/bar\") failed\n> with EACCESS, to attempt to rmdir(\"foo/bar\") whether we were told\n> not to attempt removing \"foo/\", no?\n> \n>> +\t\telse if (errno == EACCES && !keep_toplevel)\n> \n> That is, I am wondering why this part just checks !keep_toplevel,\n> not\n> \n> \t(!keep_toplevel || has_dir_separator(path->buf))\n> \n> or something.\n\nI'm not sure I understand your point.  Please note that the\nREMOVE_DIR_KEEP_TOPLEVEL bit is cleared from flags before this function\nrecurses.  So in recursive invocations, keep_toplevel will always be\nfalse, and the rmdir(path->buf) codepath will be permitted.  Does that\nanswer your question?\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"232804","messageId":"52CBD622.3010805@alum.mit.edu","threadId":"35614","inReplyTo":"xmqq4n5hj8ry.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 10/17] lock_ref_sha1_basic(): on SCLD_VANISHED, retry","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-07T10:25:38Z","receivedAt":"2014-01-07T10:25:38Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 01/06/2014 06:54 PM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n>> If safe_create_leading_directories() fails because a file along the\n>> path unexpectedly vanished, try again (up to 3 times).\n>>\n>> This can occur if another process is deleting directories at the same\n>> time as we are trying to make them.  For example, \"git pack-refs\n>> --all\" tries to delete the loose refs and any empty directories that\n>> are left behind.  If a pack-refs process is running, then it might\n>> delete a directory that we need to put a new loose reference in.\n>>\n>> If safe_create_leading_directories() thinks this might have happened,\n>> then take its advice and try again (maximum three attempts).\n>>\n>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n>> ---\n>>  refs.c | 11 ++++++++++-\n>>  1 file changed, 10 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/refs.c b/refs.c\n>> index 3926136..6eb8a02 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -2039,6 +2039,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n>>  \tint type, lflags;\n>>  \tint mustexist = (old_sha1 && !is_null_sha1(old_sha1));\n>>  \tint missing = 0;\n>> +\tint attempts = 3;\n>>  \n>>  \tlock = xcalloc(1, sizeof(struct ref_lock));\n>>  \tlock->lock_fd = -1;\n>> @@ -2093,7 +2094,15 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n>>  \tif ((flags & REF_NODEREF) && (type & REF_ISSYMREF))\n>>  \t\tlock->force_write = 1;\n>>  \n>> -\tif (safe_create_leading_directories(ref_file)) {\n>> + retry:\n>> +\tswitch (safe_create_leading_directories(ref_file)) {\n>> +\tcase SCLD_OK:\n>> +\t\tbreak; /* success */\n>> +\tcase SCLD_VANISHED:\n>> +\t\tif (--attempts > 0)\n>> +\t\t\tgoto retry;\n>> +\t\t/* fall through */\n> \n> Hmph.\n> \n> Having no backoff/sleep at all might be OK here as long as the other\n> side that removes does not retry (and I do not think the other side\n> would, even though I haven't read through the series to the end yet\n> ;-)).\n\nremove_dir_recurse() only tries deleting directories once (I haven't\nchanged that).  And from a broader perspective, it would be pretty silly\nfor any tidy-up-directories function to try deleting things more than\nonce.  So I don't think it is a problem.  But even in the worst case,\nthis function only tries three times before giving up, so it shouldn't\nbe a disaster.\n\n> This may be just a style thing, but I find that the variable name\n> \"attempts\" that starts out as 3 quite misleading, as its value is\n> not \"the number of attempts made\" but \"the remaining number of\n> attempts allowed.\"  Starting it from 0 and then\n> \n> \tif (attempts++ < MAX_ATTEMPTS)\n> \t\tgoto retry;\n> \n> would be one way to clarify it.  Renaming it to remaining_attempts\n> would be another.\n\nI just renamed the variable to attempts_remaining.  (I thought I was\nfollowing your suggestion, but now I see that I put the words in the\nopposite order; oh well, I think it's fine either way.)\n\nThanks for your review!  I will wait a day or so for any additional\ncomments, and then send a v3.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"232805","messageId":"52CBDBD9.7020607@alum.mit.edu","threadId":"35614","inReplyTo":"xmqqtxdhhsyq.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 17/17] rename_tmp_log(): on SCLD_VANISHED, retry","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-01-07T10:50:01Z","receivedAt":"2014-01-07T10:50:01Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 01/06/2014 07:21 PM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n>> If safe_create_leading_directories() fails because a file along the\n>> path unexpectedly vanished, try again from the beginning.  Try at most\n>> 3 times.\n> \n> As the previous step bumped it from 3 to 4 without explanation, the\n> above no longer reflects reality ;-)\n\nGood catch.  The increment 3 -> 4 was because the first call to rename()\nis optimistic, and can fail once even if there is no race.  I will\nchange the commit message of 16/17 to explain this point, and of 17/17\nto match reality.\n\n> The series mostly looked sane from a cursory read.\n> \n> Will re-queue.  Thanks.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"232814","messageId":"xmqq8uurg0sm.fsf@gitster.dls.corp.google.com","threadId":"35614","inReplyTo":"52CBD1D5.5080701@alum.mit.edu","subject":"Re: [PATCH v2 13/17] remove_dir_recurse(): handle disappearing files and directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-07T17:27:21Z","receivedAt":"2014-01-07T17:27:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> I'm not sure I understand your point.  Please note that the\n> REMOVE_DIR_KEEP_TOPLEVEL bit is cleared from flags before this function\n> recurses.  So in recursive invocations, keep_toplevel will always be\n> false, and the rmdir(path->buf) codepath will be permitted.  Does that\n> answer your question?\n\nYes; thanks.\n"},{"id":"232815","messageId":"xmqq4n5fg05g.fsf@gitster.dls.corp.google.com","threadId":"35614","inReplyTo":"52CBC854.2060602@alum.mit.edu","subject":"Re: [PATCH v2 03/17] safe_create_leading_directories(): add explicit \"slash\" pointer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-07T17:41:15Z","receivedAt":"2014-01-07T17:41:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> I agree that it would be reasonable to use is_dir_sep() in the\n> implementation of this function, at least unless/until somebody does the\n> work to figure out whether callers should really only be passing it\n> forward-slash-normalized paths.\n>\n> Please be careful, though, because I don't think this function is\n> capable of handling arbitrary Windows paths, like for example\n> //host/path format, either before or after my change.\n>\n> Let me know if you would like me to merge or rebase the is_dir_sep()\n> changes into this patch series.\n\nI'd want SSchuberth and windows folks to be at least aware of this\nseries and preferrably see that they offer inputs to this series,\nmaking their is_dir_sep() change just one step in this series.  That\nway I have one less series to worry about ;-)\n\nThanks.\n"},{"id":"233390","messageId":"CAHGBnuMdEtRMg4apeRAt0QY9dMm6p3MB_DZ5axBG-L=R-vh_ww@mail.gmail.com","threadId":"35614","inReplyTo":"xmqq4n5fg05g.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 03/17] safe_create_leading_directories(): add explicit \"slash\" pointer","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2014-01-19T20:31:20Z","receivedAt":"2014-01-19T20:31:20Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Tue, Jan 7, 2014 at 6:41 PM, Junio C Hamano <gitster@pobox.com> wrote:\n\n>> Let me know if you would like me to merge or rebase the is_dir_sep()\n>> changes into this patch series.\n>\n> I'd want SSchuberth and windows folks to be at least aware of this\n> series and preferrably see that they offer inputs to this series,\n> making their is_dir_sep() change just one step in this series.  That\n> way I have one less series to worry about ;-)\n\nThanks Junio for pointing out Michael's series to me and resolving the\ninitial merge conflict. However, as written in my reply to Michael's\nmail of today, I'd prefer to take Michael's patch that applies cleanly\non top of v3 of his mh/safe-create-leading-directories instead of your\nmerge conflict resolution.\n\n-- \nSebastian Schuberth\n"}]}