From 53a3972171723d5d67178c9eba03112931a86cae Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 6 Jan 2014 14:45:19 +0100 Subject: safe_create_leading_directories(): fix format of "if" chaining Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/sha1_file.c b/sha1_file.c index 760dd60..56b51b0 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -125,8 +125,7 @@ int safe_create_leading_directories(char *path) *pos = '/'; return -3; } - } - else if (mkdir(path, 0777)) { + } else if (mkdir(path, 0777)) { if (errno == EEXIST && !stat(path, &st) && S_ISDIR(st.st_mode)) { ; /* somebody created it since we checked */ @@ -134,8 +133,7 @@ int safe_create_leading_directories(char *path) *pos = '/'; return -1; } - } - else if (adjust_shared_perm(path)) { + } else if (adjust_shared_perm(path)) { *pos = '/'; return -2; } -- cgit v0.10.2-6-g49f6 From f05023324c74bd12e66ac1dd04bbe2692c31dbfb Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 6 Jan 2014 14:45:20 +0100 Subject: safe_create_leading_directories(): reduce scope of local variable This makes it more obvious that values of "st" don't persist across loop iterations. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/sha1_file.c b/sha1_file.c index 56b51b0..e52a003 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -108,9 +108,10 @@ int mkdir_in_gitdir(const char *path) int safe_create_leading_directories(char *path) { char *pos = path + offset_1st_component(path); - struct stat st; while (pos) { + struct stat st; + pos = strchr(pos, '/'); if (!pos) break; -- cgit v0.10.2-6-g49f6 From 831651fde8f8a8d90543917a8783962aa1a534bf Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 6 Jan 2014 14:45:21 +0100 Subject: safe_create_leading_directories(): add explicit "slash" pointer Keep track of the position of the slash character independently of "pos", thereby making the purpose of each variable clearer and working towards other upcoming changes. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/sha1_file.c b/sha1_file.c index e52a003..a2b9e3c 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -111,19 +111,21 @@ int safe_create_leading_directories(char *path) while (pos) { struct stat st; + char *slash = strchr(pos, '/'); - pos = strchr(pos, '/'); - if (!pos) + if (!slash) break; - while (*++pos == '/') - ; + while (*(slash + 1) == '/') + slash++; + pos = slash + 1; if (!*pos) break; - *--pos = '\0'; + + *slash = '\0'; if (!stat(path, &st)) { /* path exists */ if (!S_ISDIR(st.st_mode)) { - *pos = '/'; + *slash = '/'; return -3; } } else if (mkdir(path, 0777)) { @@ -131,14 +133,14 @@ int safe_create_leading_directories(char *path) !stat(path, &st) && S_ISDIR(st.st_mode)) { ; /* somebody created it since we checked */ } else { - *pos = '/'; + *slash = '/'; return -1; } } else if (adjust_shared_perm(path)) { - *pos = '/'; + *slash = '/'; return -2; } - *pos++ = '/'; + *slash = '/'; } return 0; } -- cgit v0.10.2-6-g49f6 From 26c8ae2a577cd283ab9fa8ab5e5f0ced568dc034 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 6 Jan 2014 14:45:22 +0100 Subject: safe_create_leading_directories(): rename local variable Rename "pos" to "next_component", because now it always points at the next component of the path name that has to be processed. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/sha1_file.c b/sha1_file.c index a2b9e3c..4dd16c3 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -107,18 +107,18 @@ int mkdir_in_gitdir(const char *path) int safe_create_leading_directories(char *path) { - char *pos = path + offset_1st_component(path); + char *next_component = path + offset_1st_component(path); - while (pos) { + while (next_component) { struct stat st; - char *slash = strchr(pos, '/'); + char *slash = strchr(next_component, '/'); if (!slash) break; while (*(slash + 1) == '/') slash++; - pos = slash + 1; - if (!*pos) + next_component = slash + 1; + if (!*next_component) break; *slash = '\0'; -- cgit v0.10.2-6-g49f6 From bf10cf70ad0c777dbbbb00bbb741436e285c2181 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 6 Jan 2014 14:45:23 +0100 Subject: safe_create_leading_directories(): split on first of multiple slashes If the input path has multiple slashes between path components (e.g., "foo//bar"), then the old code was breaking the path at the last slash, not the first one. So in the above example, the second slash was overwritten with NUL, resulting in the parent directory being sought as "foo/". When stat() is called on "foo/", it fails with ENOTDIR if "foo" exists but is not a directory. This caused the wrong path to be taken in the subsequent logic. So instead, split path components at the first intercomponent slash rather than the last one. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/sha1_file.c b/sha1_file.c index 4dd16c3..1d9cc1b 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -115,9 +115,10 @@ int safe_create_leading_directories(char *path) if (!slash) break; - while (*(slash + 1) == '/') - slash++; + next_component = slash + 1; + while (*next_component == '/') + next_component++; if (!*next_component) break; -- cgit v0.10.2-6-g49f6 From 9e6f885d146c58b23b166a99b93f115735b7bf22 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 6 Jan 2014 14:45:24 +0100 Subject: safe_create_leading_directories(): always restore slash at end of loop Always restore the slash that we scribbled over at the end of the loop, rather than also fixing it up at each premature exit from the loop. This makes it harder to forget to do the cleanup as new paths are added to the code. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/sha1_file.c b/sha1_file.c index 1d9cc1b..60d6fce 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -108,8 +108,9 @@ int mkdir_in_gitdir(const char *path) int safe_create_leading_directories(char *path) { char *next_component = path + offset_1st_component(path); + int ret = 0; - while (next_component) { + while (!ret && next_component) { struct stat st; char *slash = strchr(next_component, '/'); @@ -125,25 +126,20 @@ int safe_create_leading_directories(char *path) *slash = '\0'; if (!stat(path, &st)) { /* path exists */ - if (!S_ISDIR(st.st_mode)) { - *slash = '/'; - return -3; - } + if (!S_ISDIR(st.st_mode)) + ret = -3; } else if (mkdir(path, 0777)) { if (errno == EEXIST && - !stat(path, &st) && S_ISDIR(st.st_mode)) { + !stat(path, &st) && S_ISDIR(st.st_mode)) ; /* somebody created it since we checked */ - } else { - *slash = '/'; - return -1; - } + else + ret = -1; } else if (adjust_shared_perm(path)) { - *slash = '/'; - return -2; + ret = -2; } *slash = '/'; } - return 0; + return ret; } int safe_create_leading_directories_const(const char *path) -- cgit v0.10.2-6-g49f6 From 0be0521b23f46dac586e1b464fafe8b97027b645 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 6 Jan 2014 14:45:25 +0100 Subject: safe_create_leading_directories(): introduce enum for return values Instead of returning magic integer values (which a couple of callers go to the trouble of distinguishing), return values from an enum. Add a docstring. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/builtin/init-db.c b/builtin/init-db.c index 78aa387..0bc14f3 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -515,10 +515,10 @@ int cmd_init_db(int argc, const char **argv, const char *prefix) saved = shared_repository; shared_repository = 0; switch (safe_create_leading_directories_const(argv[0])) { - case -3: + case SCLD_EXISTS: errno = EEXIST; /* fallthru */ - case -1: + case SCLD_FAILED: die_errno(_("cannot mkdir %s"), argv[0]); break; default: diff --git a/cache.h b/cache.h index ce377e1..c6a4157 100644 --- a/cache.h +++ b/cache.h @@ -736,8 +736,21 @@ enum sharedrepo { }; int git_config_perm(const char *var, const char *value); int adjust_shared_perm(const char *path); -int safe_create_leading_directories(char *path); -int safe_create_leading_directories_const(const char *path); + +/* + * Create the directory containing the named path, using care to be + * somewhat safe against races. Return one of the scld_error values + * to indicate success/failure. + */ +enum scld_error { + SCLD_OK = 0, + SCLD_FAILED = -1, + SCLD_PERMS = -2, + SCLD_EXISTS = -3 +}; +enum scld_error safe_create_leading_directories(char *path); +enum scld_error safe_create_leading_directories_const(const char *path); + int mkdir_in_gitdir(const char *path); extern void home_config_paths(char **global, char **xdg, char *file); extern char *expand_user_path(const char *path); diff --git a/merge-recursive.c b/merge-recursive.c index dbb7104..021e1fc 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -693,7 +693,7 @@ static int make_room_for_path(struct merge_options *o, const char *path) /* Make sure leading directories are created */ status = safe_create_leading_directories_const(path); if (status) { - if (status == -3) { + if (status == SCLD_EXISTS) { /* something else exists */ error(msg, path, _(": perhaps a D/F conflict?")); return -1; diff --git a/sha1_file.c b/sha1_file.c index 60d6fce..2a86912 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -105,12 +105,12 @@ int mkdir_in_gitdir(const char *path) return adjust_shared_perm(path); } -int safe_create_leading_directories(char *path) +enum scld_error safe_create_leading_directories(char *path) { char *next_component = path + offset_1st_component(path); - int ret = 0; + enum scld_error ret = SCLD_OK; - while (!ret && next_component) { + while (ret == SCLD_OK && next_component) { struct stat st; char *slash = strchr(next_component, '/'); @@ -127,26 +127,26 @@ int safe_create_leading_directories(char *path) if (!stat(path, &st)) { /* path exists */ if (!S_ISDIR(st.st_mode)) - ret = -3; + ret = SCLD_EXISTS; } else if (mkdir(path, 0777)) { if (errno == EEXIST && !stat(path, &st) && S_ISDIR(st.st_mode)) ; /* somebody created it since we checked */ else - ret = -1; + ret = SCLD_FAILED; } else if (adjust_shared_perm(path)) { - ret = -2; + ret = SCLD_PERMS; } *slash = '/'; } return ret; } -int safe_create_leading_directories_const(const char *path) +enum scld_error safe_create_leading_directories_const(const char *path) { /* path points to cache entries, so xstrdup before messing with it */ char *buf = xstrdup(path); - int result = safe_create_leading_directories(buf); + enum scld_error result = safe_create_leading_directories(buf); free(buf); return result; } -- cgit v0.10.2-6-g49f6 From f3565c0ca535d3becdcd2266002385709ddfa66c Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 6 Jan 2014 14:45:26 +0100 Subject: cmd_init_db(): when creating directories, handle errors conservatively safe_create_leading_directories_const() returns a non-zero value on error. The old code at this calling site recognized a couple of particular error values, and treated all other return values as success. Instead, be more conservative: recognize the errors we are interested in, but treat any other nonzero values as failures. This is more robust in case somebody adds another possible return value without telling us. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/builtin/init-db.c b/builtin/init-db.c index 0bc14f3..ceeb138 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -515,13 +515,14 @@ int cmd_init_db(int argc, const char **argv, const char *prefix) saved = shared_repository; shared_repository = 0; switch (safe_create_leading_directories_const(argv[0])) { + case SCLD_OK: + case SCLD_PERMS: + break; case SCLD_EXISTS: errno = EEXIST; /* fallthru */ - case SCLD_FAILED: - die_errno(_("cannot mkdir %s"), argv[0]); - break; default: + die_errno(_("cannot mkdir %s"), argv[0]); break; } shared_repository = saved; -- cgit v0.10.2-6-g49f6 From 18d37e860dfb9a98fb93ea7bb517ec3c16f995c4 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 6 Jan 2014 14:45:27 +0100 Subject: safe_create_leading_directories(): add new error value SCLD_VANISHED Add a new possible error result that can be returned by safe_create_leading_directories() and safe_create_leading_directories_const(): SCLD_VANISHED. This value indicates that a file or directory on the path existed at one point (either it already existed or the function created it), but then it disappeared. This probably indicates that another process deleted the directory while we were working. If SCLD_VANISHED is returned, the caller might want to retry the function call, as there is a chance that a new attempt will succeed. Why doesn't safe_create_leading_directories() do the retrying internally? Because an empty directory isn't really ever safe until it holds a file. So even if safe_create_leading_directories() were absolutely sure that the directory existed before it returned, there would be no guarantee that the directory still existed when the caller tried to write something in it. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/cache.h b/cache.h index c6a4157..f34c0a7 100644 --- a/cache.h +++ b/cache.h @@ -741,12 +741,20 @@ int adjust_shared_perm(const char *path); * Create the directory containing the named path, using care to be * somewhat safe against races. Return one of the scld_error values * to indicate success/failure. + * + * SCLD_VANISHED indicates that one of the ancestor directories of the + * path existed at one point during the function call and then + * suddenly vanished, probably because another process pruned the + * directory while we were working. To be robust against this kind of + * race, callers might want to try invoking the function again when it + * returns SCLD_VANISHED. */ enum scld_error { SCLD_OK = 0, SCLD_FAILED = -1, SCLD_PERMS = -2, - SCLD_EXISTS = -3 + SCLD_EXISTS = -3, + SCLD_VANISHED = -4 }; enum scld_error safe_create_leading_directories(char *path); enum scld_error safe_create_leading_directories_const(const char *path); diff --git a/sha1_file.c b/sha1_file.c index 2a86912..ed814e5 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -132,6 +132,17 @@ enum scld_error safe_create_leading_directories(char *path) if (errno == EEXIST && !stat(path, &st) && S_ISDIR(st.st_mode)) ; /* somebody created it since we checked */ + else if (errno == ENOENT) + /* + * Either mkdir() failed because + * somebody just pruned the containing + * directory, or stat() failed because + * the file that was in our way was + * just removed. Either way, inform + * the caller that it might be worth + * trying again: + */ + ret = SCLD_VANISHED; else ret = SCLD_FAILED; } else if (adjust_shared_perm(path)) { -- cgit v0.10.2-6-g49f6 From c4c61c763e700d02344490590d6980ee51031a27 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 18 Jan 2014 23:48:54 +0100 Subject: lock_ref_sha1_basic(): on SCLD_VANISHED, retry If safe_create_leading_directories() fails because a file along the path unexpectedly vanished, try again (up to 3 times). This can occur if another process is deleting directories at the same time as we are trying to make them. For example, "git pack-refs --all" tries to delete the loose refs and any empty directories that are left behind. If a pack-refs process is running, then it might delete a directory that we need to put a new loose reference in. If safe_create_leading_directories() thinks this might have happened, then take its advice and try again (maximum three attempts). Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/refs.c b/refs.c index 5e5a382..2d0faf2 100644 --- a/refs.c +++ b/refs.c @@ -2039,6 +2039,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, int type, lflags; int mustexist = (old_sha1 && !is_null_sha1(old_sha1)); int missing = 0; + int attempts_remaining = 3; lock = xcalloc(1, sizeof(struct ref_lock)); lock->lock_fd = -1; @@ -2093,7 +2094,15 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, if ((flags & REF_NODEREF) && (type & REF_ISSYMREF)) lock->force_write = 1; - if (safe_create_leading_directories(ref_file)) { + retry: + switch (safe_create_leading_directories(ref_file)) { + case SCLD_OK: + break; /* success */ + case SCLD_VANISHED: + if (--attempts_remaining > 0) + goto retry; + /* fall through */ + default: last_errno = errno; error("unable to create directory for %s", ref_file); goto error_return; -- cgit v0.10.2-6-g49f6 From e5c223e98b985d9faa274039aefa4391d2da25a6 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 18 Jan 2014 23:48:55 +0100 Subject: lock_ref_sha1_basic(): if locking fails with ENOENT, retry If hold_lock_file_for_update() fails with errno==ENOENT, it might be because somebody else (for example, a pack-refs process) has just deleted one of the lockfile's ancestor directories. So if this condition is detected, try again (up to 3 times). Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/refs.c b/refs.c index 2d0faf2..a09bbb7 100644 --- a/refs.c +++ b/refs.c @@ -2081,7 +2081,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, lock->lk = xcalloc(1, sizeof(struct lock_file)); - lflags = LOCK_DIE_ON_ERROR; + lflags = 0; if (flags & REF_NODEREF) { refname = orig_refname; lflags |= LOCK_NODEREF; @@ -2109,6 +2109,17 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, } lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags); + if (lock->lock_fd < 0) { + if (errno == ENOENT && --attempts_remaining > 0) + /* + * Maybe somebody just deleted one of the + * directories leading to ref_file. Try + * again: + */ + goto retry; + else + unable_to_lock_index_die(ref_file, errno); + } return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock; error_return: -- cgit v0.10.2-6-g49f6 From ecb2c282c0d6cdd9938ba9cf228316ebc91b397e Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 18 Jan 2014 23:48:56 +0100 Subject: remove_dir_recurse(): tighten condition for removing unreadable dir If opendir() fails on the top-level directory, it makes sense to try to delete it anyway--but only if the failure was due to EACCES. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/dir.c b/dir.c index 23b6de4..11e1520 100644 --- a/dir.c +++ b/dir.c @@ -1476,8 +1476,11 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up) flag &= ~REMOVE_DIR_KEEP_TOPLEVEL; dir = opendir(path->buf); if (!dir) { - /* an empty dir could be removed even if it is unreadble */ - if (!keep_toplevel) + if (errno == EACCES && !keep_toplevel) + /* + * An empty dir could be removable even if it + * is unreadable: + */ return rmdir(path->buf); else return -1; -- cgit v0.10.2-6-g49f6 From 863808cd1a7bd23dc12a3b54e32eb3f25d9487bc Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 18 Jan 2014 23:48:57 +0100 Subject: remove_dir_recurse(): handle disappearing files and directories If a file or directory that we are trying to remove disappears (e.g., because another process has pruned it), do not consider it an error. However, if REMOVE_DIR_KEEP_TOPLEVEL is set, and the toplevel directory is missing, then consider it an error (like before). Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/dir.c b/dir.c index 11e1520..716b613 100644 --- a/dir.c +++ b/dir.c @@ -1476,7 +1476,9 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up) flag &= ~REMOVE_DIR_KEEP_TOPLEVEL; dir = opendir(path->buf); if (!dir) { - if (errno == EACCES && !keep_toplevel) + if (errno == ENOENT) + return keep_toplevel ? -1 : 0; + else if (errno == EACCES && !keep_toplevel) /* * An empty dir could be removable even if it * is unreadable: @@ -1496,13 +1498,21 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up) strbuf_setlen(path, len); strbuf_addstr(path, e->d_name); - if (lstat(path->buf, &st)) - ; /* fall thru */ - else if (S_ISDIR(st.st_mode)) { + if (lstat(path->buf, &st)) { + if (errno == ENOENT) + /* + * file disappeared, which is what we + * wanted anyway + */ + continue; + /* fall thru */ + } else if (S_ISDIR(st.st_mode)) { if (!remove_dir_recurse(path, flag, &kept_down)) continue; /* happy */ - } else if (!only_empty && !unlink(path->buf)) + } else if (!only_empty && + (!unlink(path->buf) || errno == ENOENT)) { continue; /* happy, too */ + } /* path too long, stat fails, or non-directory still exists */ ret = -1; @@ -1512,7 +1522,7 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up) strbuf_setlen(path, original_len); if (!ret && !keep_toplevel && !kept_down) - ret = rmdir(path->buf); + ret = (!rmdir(path->buf) || errno == ENOENT) ? 0 : -1; else if (kept_up) /* * report the uplevel that it is not an error that we -- cgit v0.10.2-6-g49f6 From fa59ae7971498157370d935adbb2bfc28012aa9f Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 18 Jan 2014 23:48:58 +0100 Subject: rename_ref(): extract function rename_tmp_log() It's about to become a bit more complex. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/refs.c b/refs.c index a09bbb7..827e543 100644 --- a/refs.c +++ b/refs.c @@ -2528,6 +2528,35 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt) */ #define TMP_RENAMED_LOG "logs/refs/.tmp-renamed-log" +static int rename_tmp_log(const char *newrefname) +{ + if (safe_create_leading_directories(git_path("logs/%s", newrefname))) { + error("unable to create directory for %s", newrefname); + return -1; + } + + retry: + if (rename(git_path(TMP_RENAMED_LOG), git_path("logs/%s", newrefname))) { + if (errno==EISDIR || errno==ENOTDIR) { + /* + * rename(a, b) when b is an existing + * directory ought to result in ISDIR, but + * Solaris 5.8 gives ENOTDIR. Sheesh. + */ + if (remove_empty_directories(git_path("logs/%s", newrefname))) { + error("Directory not empty: logs/%s", newrefname); + return -1; + } + goto retry; + } else { + error("unable to move logfile "TMP_RENAMED_LOG" to logs/%s: %s", + newrefname, strerror(errno)); + return -1; + } + } + return 0; +} + int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsg) { unsigned char sha1[20], orig_sha1[20]; @@ -2575,30 +2604,9 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms } } - if (log && safe_create_leading_directories(git_path("logs/%s", newrefname))) { - error("unable to create directory for %s", newrefname); + if (log && rename_tmp_log(newrefname)) goto rollback; - } - retry: - if (log && rename(git_path(TMP_RENAMED_LOG), git_path("logs/%s", newrefname))) { - if (errno==EISDIR || errno==ENOTDIR) { - /* - * rename(a, b) when b is an existing - * directory ought to result in ISDIR, but - * Solaris 5.8 gives ENOTDIR. Sheesh. - */ - if (remove_empty_directories(git_path("logs/%s", newrefname))) { - error("Directory not empty: logs/%s", newrefname); - goto rollback; - } - goto retry; - } else { - error("unable to move logfile "TMP_RENAMED_LOG" to logs/%s: %s", - newrefname, strerror(errno)); - goto rollback; - } - } logmoved = log; lock = lock_ref_sha1_basic(newrefname, NULL, 0, NULL); -- cgit v0.10.2-6-g49f6 From ae4a283e3b4cdc99a548d215739539c3a690f290 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 18 Jan 2014 23:48:59 +0100 Subject: rename_tmp_log(): handle a possible mkdir/rmdir race If a directory vanishes while renaming the temporary reflog file, retry (up to 3 times). This could happen if another process deletes the directory created by safe_create_leading_directories() just before we rename the file into the directory. As far as I can tell, this race could not occur internal to git. The only time that a directory under $GIT_DIR/logs is deleted is if room has to be made for a log file for a reference with the same name; for example, in the following sequence: git branch foo/bar # Creates file .git/logs/refs/heads/foo/bar git branch -d foo/bar # Deletes file but leaves .git/logs/refs/heads/foo/ git branch foo # Deletes .git/logs/refs/heads/foo/ But the only reason the last command deletes the directory is because it wants to create a file with the same name. So if another process (e.g., git branch foo/baz ) wants to create that directory, one of the two is doomed to failure anyway because of a D/F conflict. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/refs.c b/refs.c index 827e543..bbcdc88 100644 --- a/refs.c +++ b/refs.c @@ -2530,12 +2530,14 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt) static int rename_tmp_log(const char *newrefname) { + int attempts_remaining = 3; + + retry: if (safe_create_leading_directories(git_path("logs/%s", newrefname))) { error("unable to create directory for %s", newrefname); return -1; } - retry: if (rename(git_path(TMP_RENAMED_LOG), git_path("logs/%s", newrefname))) { if (errno==EISDIR || errno==ENOTDIR) { /* @@ -2548,6 +2550,13 @@ static int rename_tmp_log(const char *newrefname) return -1; } goto retry; + } else if (errno == ENOENT && --attempts_remaining > 0) { + /* + * Maybe another process just deleted one of + * the directories in the path to newrefname. + * Try again from the beginning. + */ + goto retry; } else { error("unable to move logfile "TMP_RENAMED_LOG" to logs/%s: %s", newrefname, strerror(errno)); -- cgit v0.10.2-6-g49f6 From f1e9e9a4dbe2cfb39dcb14ee4f34628ef46d7b15 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 18 Jan 2014 23:49:00 +0100 Subject: rename_tmp_log(): limit the number of remote_empty_directories() attempts This doesn't seem to be a likely error, but we've got the counter anyway, so we might as well use it for an added bit of safety. Please note that the first call to rename() is optimistic, and it is normal for it to fail if there is a directory in the way. So bump the total number of allowed attempts to 4, to be sure that we can still have at least 3 retries in the case of a race. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/refs.c b/refs.c index bbcdc88..fd644f0 100644 --- a/refs.c +++ b/refs.c @@ -2530,7 +2530,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt) static int rename_tmp_log(const char *newrefname) { - int attempts_remaining = 3; + int attempts_remaining = 4; retry: if (safe_create_leading_directories(git_path("logs/%s", newrefname))) { @@ -2539,7 +2539,7 @@ static int rename_tmp_log(const char *newrefname) } if (rename(git_path(TMP_RENAMED_LOG), git_path("logs/%s", newrefname))) { - if (errno==EISDIR || errno==ENOTDIR) { + if ((errno==EISDIR || errno==ENOTDIR) && --attempts_remaining > 0) { /* * rename(a, b) when b is an existing * directory ought to result in ISDIR, but -- cgit v0.10.2-6-g49f6 From 08f555cb82f92797ca0aa0d6ba32e6872f1331e5 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 18 Jan 2014 23:49:01 +0100 Subject: rename_tmp_log(): on SCLD_VANISHED, retry If safe_create_leading_directories() fails because a file along the path unexpectedly vanished, try again from the beginning. Try at most 4 times. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/refs.c b/refs.c index fd644f0..703b5a2 100644 --- a/refs.c +++ b/refs.c @@ -2533,7 +2533,14 @@ static int rename_tmp_log(const char *newrefname) int attempts_remaining = 4; retry: - if (safe_create_leading_directories(git_path("logs/%s", newrefname))) { + switch (safe_create_leading_directories(git_path("logs/%s", newrefname))) { + case SCLD_OK: + break; /* success */ + case SCLD_VANISHED: + if (--attempts_remaining > 0) + goto retry; + /* fall through */ + default: error("unable to create directory for %s", newrefname); return -1; } -- cgit v0.10.2-6-g49f6