From 583643248a28c5d3f678fdd042a63e5d1c48f39e Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 6 Jan 2017 17:22:21 +0100 Subject: files_rename_ref(): tidy up whitespace Signed-off-by: Michael Haggerty Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- refs/files-backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index f9023939d5..be4ffdca24 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2631,7 +2631,7 @@ static int files_rename_ref(struct ref_store *ref_store, if (!read_ref_full(newrefname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, sha1, NULL) && delete_ref(newrefname, NULL, REF_NODEREF)) { - if (errno==EISDIR) { + if (errno == EISDIR) { struct strbuf path = STRBUF_INIT; int result; -- cgit v1.2.1 From 15ee2c72e76fa79e957990b43277a4591371e70e Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 6 Jan 2017 17:22:22 +0100 Subject: refname_is_safe(): correct docstring The behavior of refname_is_safe() was changed in e40f355 "refname_is_safe(): insist that the refname already be normalized", 2016-04-27 without a corresponding update to its docstring. The function is in fact stricter than documented, because it now insists that the result of normalizing the part of a refname following "refs/" is identical to that part of the original refname. Fix the docstring. Signed-off-by: Michael Haggerty Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- refs/refs-internal.h | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) (limited to 'refs') diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 708b26082a..dc81acc902 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -62,11 +62,12 @@ * This function does not check that the reference name is legal; for * that, use check_refname_format(). * - * We consider a refname that starts with "refs/" to be safe as long - * as any ".." components that it might contain do not escape "refs/". - * Names that do not start with "refs/" are considered safe iff they - * consist entirely of upper case characters and '_' (like "HEAD" and - * "MERGE_HEAD" but not "config" or "FOO/BAR"). + * A refname that starts with "refs/" is considered safe iff it + * doesn't contain any "." or ".." components or consecutive '/' + * characters, end with '/', or (on Windows) contain any '\' + * characters. Names that do not start with "refs/" are considered + * safe iff they consist entirely of upper case characters and '_' + * (like "HEAD" and "MERGE_HEAD" but not "config" or "FOO/BAR"). */ int refname_is_safe(const char *refname); -- cgit v1.2.1 From e5007a6820e509c2b8611874dc758e4cfe9fabec Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 6 Jan 2017 17:22:27 +0100 Subject: lock_ref_sha1_basic(): inline constant `lflags` is set a single time then never changed, so just inline it. Signed-off-by: Michael Haggerty Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- refs/files-backend.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index be4ffdca24..7d4658aa94 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2000,7 +2000,6 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs, struct strbuf ref_file = STRBUF_INIT; struct ref_lock *lock; int last_errno = 0; - int lflags = LOCK_NO_DEREF; int mustexist = (old_sha1 && !is_null_sha1(old_sha1)); int resolve_flags = RESOLVE_REF_NO_RECURSE; int attempts_remaining = 3; @@ -2083,7 +2082,7 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs, goto error_return; } - if (hold_lock_file_for_update(lock->lk, ref_file.buf, lflags) < 0) { + if (hold_lock_file_for_update(lock->lk, ref_file.buf, LOCK_NO_DEREF) < 0) { last_errno = errno; if (errno == ENOENT && --attempts_remaining > 0) /* -- cgit v1.2.1 From 3b5d3c9848aaa6e15d814730cfac0efe3257eb46 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 6 Jan 2017 17:22:28 +0100 Subject: lock_ref_sha1_basic(): use raceproof_create_file() Instead of coding the retry loop inline, use raceproof_create_file() to make lock acquisition safe against directory creation/deletion races. Signed-off-by: Michael Haggerty Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- refs/files-backend.c | 35 +++++++++-------------------------- 1 file changed, 9 insertions(+), 26 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index 7d4658aa94..74de289f42 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1985,6 +1985,13 @@ static int remove_empty_directories(struct strbuf *path) return remove_dir_recursively(path, REMOVE_DIR_EMPTY_ONLY); } +static int create_reflock(const char *path, void *cb) +{ + struct lock_file *lk = cb; + + return hold_lock_file_for_update(lk, path, LOCK_NO_DEREF) < 0 ? -1 : 0; +} + /* * Locks a ref returning the lock on success and NULL on failure. * On failure errno is set to something meaningful. @@ -2002,7 +2009,6 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs, int last_errno = 0; int mustexist = (old_sha1 && !is_null_sha1(old_sha1)); int resolve_flags = RESOLVE_REF_NO_RECURSE; - int attempts_remaining = 3; int resolved; assert_main_repository(&refs->base, "lock_ref_sha1_basic"); @@ -2067,35 +2073,12 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs, lock->ref_name = xstrdup(refname); - retry: - switch (safe_create_leading_directories_const(ref_file.buf)) { - case SCLD_OK: - break; /* success */ - case SCLD_VANISHED: - if (--attempts_remaining > 0) - goto retry; - /* fall through */ - default: + if (raceproof_create_file(ref_file.buf, create_reflock, lock->lk)) { last_errno = errno; - strbuf_addf(err, "unable to create directory for '%s'", - ref_file.buf); + unable_to_lock_message(ref_file.buf, errno, err); goto error_return; } - if (hold_lock_file_for_update(lock->lk, ref_file.buf, LOCK_NO_DEREF) < 0) { - last_errno = errno; - 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_message(ref_file.buf, errno, err); - goto error_return; - } - } if (verify_lock(lock, old_sha1, mustexist, err)) { last_errno = errno; goto error_return; -- cgit v1.2.1 From 6a7f3631709cb68cdea4403c480a1a5d93100c47 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 6 Jan 2017 17:22:29 +0100 Subject: rename_tmp_log(): use raceproof_create_file() Besides shortening the code, this saves an unnecessary call to safe_create_leading_directories_const() in almost all cases. Signed-off-by: Michael Haggerty Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- refs/files-backend.c | 73 +++++++++++++++++++++------------------------------- 1 file changed, 30 insertions(+), 43 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index 74de289f42..3f18a01b4a 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2489,55 +2489,42 @@ out: */ #define TMP_RENAMED_LOG "logs/refs/.tmp-renamed-log" -static int rename_tmp_log(const char *newrefname) +static int rename_tmp_log_callback(const char *path, void *cb) { - int attempts_remaining = 4; - struct strbuf path = STRBUF_INIT; - int ret = -1; + int *true_errno = cb; - retry: - strbuf_reset(&path); - strbuf_git_path(&path, "logs/%s", newrefname); - switch (safe_create_leading_directories_const(path.buf)) { - 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); - goto out; + if (rename(git_path(TMP_RENAMED_LOG), path)) { + /* + * rename(a, b) when b is an existing directory ought + * to result in ISDIR, but Solaris 5.8 gives ENOTDIR. + * Sheesh. Record the true errno for error reporting, + * but report EISDIR to raceproof_create_file() so + * that it knows to retry. + */ + *true_errno = errno; + if (errno == ENOTDIR) + errno = EISDIR; + return -1; + } else { + return 0; } +} - if (rename(git_path(TMP_RENAMED_LOG), path.buf)) { - if ((errno==EISDIR || errno==ENOTDIR) && --attempts_remaining > 0) { - /* - * 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(&path)) { - error("Directory not empty: logs/%s", newrefname); - goto out; - } - 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 { +static int rename_tmp_log(const char *newrefname) +{ + char *path = git_pathdup("logs/%s", newrefname); + int ret, true_errno; + + ret = raceproof_create_file(path, rename_tmp_log_callback, &true_errno); + if (ret) { + if (errno == EISDIR) + error("Directory not empty: %s", path); + else error("unable to move logfile "TMP_RENAMED_LOG" to logs/%s: %s", - newrefname, strerror(errno)); - goto out; - } + newrefname, strerror(true_errno)); } - ret = 0; -out: - strbuf_release(&path); + + free(path); return ret; } -- cgit v1.2.1 From 990c98d2bd3a46df9ab873947a53b55d29a6d16f Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 6 Jan 2017 17:22:30 +0100 Subject: rename_tmp_log(): improve error reporting * Don't capitalize error strings * Report true paths of affected files Signed-off-by: Michael Haggerty Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- refs/files-backend.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index 3f18a01b4a..49a119ce4c 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2518,10 +2518,11 @@ static int rename_tmp_log(const char *newrefname) ret = raceproof_create_file(path, rename_tmp_log_callback, &true_errno); if (ret) { if (errno == EISDIR) - error("Directory not empty: %s", path); + error("directory not empty: %s", path); else - error("unable to move logfile "TMP_RENAMED_LOG" to logs/%s: %s", - newrefname, strerror(true_errno)); + error("unable to move logfile %s to %s: %s", + git_path(TMP_RENAMED_LOG), path, + strerror(true_errno)); } free(path); -- cgit v1.2.1 From 81b1b6d4ff5dea059c2120819f5cffcd915dff2e Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 6 Jan 2017 17:22:31 +0100 Subject: log_ref_write(): inline function This function doesn't do anything beyond call files_log_ref_write(), so replace it with the latter at its call sites. Signed-off-by: Michael Haggerty Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- refs/files-backend.c | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index 49a119ce4c..fd8a751e10 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2832,14 +2832,6 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1, return 0; } -static int log_ref_write(const char *refname, const unsigned char *old_sha1, - const unsigned char *new_sha1, const char *msg, - int flags, struct strbuf *err) -{ - return files_log_ref_write(refname, old_sha1, new_sha1, msg, flags, - err); -} - int files_log_ref_write(const char *refname, const unsigned char *old_sha1, const unsigned char *new_sha1, const char *msg, int flags, struct strbuf *err) @@ -2903,7 +2895,8 @@ static int commit_ref_update(struct files_ref_store *refs, assert_main_repository(&refs->base, "commit_ref_update"); clear_loose_ref_cache(refs); - if (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, 0, err)) { + if (files_log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, + logmsg, 0, err)) { char *old_msg = strbuf_detach(err, NULL); strbuf_addf(err, "cannot update the ref '%s': %s", lock->ref_name, old_msg); @@ -2934,7 +2927,7 @@ static int commit_ref_update(struct files_ref_store *refs, if (head_ref && (head_flag & REF_ISSYMREF) && !strcmp(head_ref, lock->ref_name)) { struct strbuf log_err = STRBUF_INIT; - if (log_ref_write("HEAD", lock->old_oid.hash, sha1, + if (files_log_ref_write("HEAD", lock->old_oid.hash, sha1, logmsg, 0, &log_err)) { error("%s", log_err.buf); strbuf_release(&log_err); @@ -2973,7 +2966,8 @@ static void update_symref_reflog(struct ref_lock *lock, const char *refname, struct strbuf err = STRBUF_INIT; unsigned char new_sha1[20]; if (logmsg && !read_ref(target, new_sha1) && - log_ref_write(refname, lock->old_oid.hash, new_sha1, logmsg, 0, &err)) { + files_log_ref_write(refname, lock->old_oid.hash, new_sha1, + logmsg, 0, &err)) { error("%s", err.buf); strbuf_release(&err); } @@ -3748,9 +3742,11 @@ static int files_transaction_commit(struct ref_store *ref_store, if (update->flags & REF_NEEDS_COMMIT || update->flags & REF_LOG_ONLY) { - if (log_ref_write(lock->ref_name, lock->old_oid.hash, - update->new_sha1, - update->msg, update->flags, err)) { + if (files_log_ref_write(lock->ref_name, + lock->old_oid.hash, + update->new_sha1, + update->msg, update->flags, + err)) { char *old_msg = strbuf_detach(err, NULL); strbuf_addf(err, "cannot update the ref '%s': %s", -- cgit v1.2.1 From 854bda6b4f7d6bc6087966bfa74a59c232eceac6 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 6 Jan 2017 17:22:32 +0100 Subject: log_ref_setup(): separate code for create vs non-create The behavior of this function (especially how it handles errors) is quite different depending on whether we are willing to create the reflog vs. whether we are only trying to open an existing reflog. So separate the code paths. This also simplifies the next steps. Signed-off-by: Michael Haggerty Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- refs/files-backend.c | 59 ++++++++++++++++++++++++++++++++++------------------ 1 file changed, 39 insertions(+), 20 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index fd8a751e10..c8f6d82e6d 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2718,45 +2718,64 @@ static int commit_ref(struct ref_lock *lock) */ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err, int force_create) { - int logfd, oflags = O_APPEND | O_WRONLY; + int logfd; strbuf_git_path(logfile, "logs/%s", refname); + if (force_create || should_autocreate_reflog(refname)) { if (safe_create_leading_directories(logfile->buf) < 0) { strbuf_addf(err, "unable to create directory for '%s': " "%s", logfile->buf, strerror(errno)); return -1; } - oflags |= O_CREAT; - } - - logfd = open(logfile->buf, oflags, 0666); - if (logfd < 0) { - if (!(oflags & O_CREAT) && (errno == ENOENT || errno == EISDIR)) - return 0; + logfd = open(logfile->buf, O_APPEND | O_WRONLY | O_CREAT, 0666); + if (logfd < 0) { + if (errno == EISDIR) { + /* + * The directory that is in the way might be + * empty. Try to remove it. + */ + if (remove_empty_directories(logfile)) { + strbuf_addf(err, "there are still logs under " + "'%s'", logfile->buf); + return -1; + } + logfd = open(logfile->buf, O_APPEND | O_WRONLY | O_CREAT, 0666); + } - if (errno == EISDIR) { - if (remove_empty_directories(logfile)) { - strbuf_addf(err, "there are still logs under " - "'%s'", logfile->buf); + if (logfd < 0) { + strbuf_addf(err, "unable to append to '%s': %s", + logfile->buf, strerror(errno)); return -1; } - logfd = open(logfile->buf, oflags, 0666); } - + } else { + logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666); if (logfd < 0) { - strbuf_addf(err, "unable to append to '%s': %s", - logfile->buf, strerror(errno)); - return -1; + if (errno == ENOENT || errno == EISDIR) { + /* + * The logfile doesn't already exist, + * but that is not an error; it only + * means that we won't write log + * entries to it. + */ + ; + } else { + strbuf_addf(err, "unable to append to '%s': %s", + logfile->buf, strerror(errno)); + return -1; + } } } - adjust_shared_perm(logfile->buf); - close(logfd); + if (logfd >= 0) { + adjust_shared_perm(logfile->buf); + close(logfd); + } + return 0; } - static int files_create_reflog(struct ref_store *ref_store, const char *refname, int force_create, struct strbuf *err) -- cgit v1.2.1 From 1fb0c809859252f037286d32f56432444d65bd38 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 6 Jan 2017 17:22:33 +0100 Subject: log_ref_setup(): improve robustness against races Change log_ref_setup() to use raceproof_create_file() to create the new logfile. This makes it more robust against a race against another process that might be trying to clean up empty directories while we are trying to create a new logfile. This also means that it will only call create_leading_directories() if open() fails, which should be a net win. Even in the cases where we are willing to create a new logfile, it will usually be the case that the logfile already exists, or if not then that the directory containing the logfile already exists. In such cases, we will save some work that was previously done unconditionally. Signed-off-by: Michael Haggerty Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- refs/files-backend.c | 41 ++++++++++++++++++----------------------- 1 file changed, 18 insertions(+), 23 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index c8f6d82e6d..27d4fd3f96 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2710,6 +2710,14 @@ static int commit_ref(struct ref_lock *lock) return 0; } +static int open_or_create_logfile(const char *path, void *cb) +{ + int *fd = cb; + + *fd = open(path, O_APPEND | O_WRONLY | O_CREAT, 0666); + return (*fd < 0) ? -1 : 0; +} + /* * Create a reflog for a ref. If force_create = 0, the reflog will * only be created for certain refs (those for which @@ -2723,31 +2731,18 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str strbuf_git_path(logfile, "logs/%s", refname); if (force_create || should_autocreate_reflog(refname)) { - if (safe_create_leading_directories(logfile->buf) < 0) { - strbuf_addf(err, "unable to create directory for '%s': " - "%s", logfile->buf, strerror(errno)); - return -1; - } - logfd = open(logfile->buf, O_APPEND | O_WRONLY | O_CREAT, 0666); - if (logfd < 0) { - if (errno == EISDIR) { - /* - * The directory that is in the way might be - * empty. Try to remove it. - */ - if (remove_empty_directories(logfile)) { - strbuf_addf(err, "there are still logs under " - "'%s'", logfile->buf); - return -1; - } - logfd = open(logfile->buf, O_APPEND | O_WRONLY | O_CREAT, 0666); - } - - if (logfd < 0) { + if (raceproof_create_file(logfile->buf, open_or_create_logfile, &logfd)) { + if (errno == ENOENT) + strbuf_addf(err, "unable to create directory for '%s': " + "%s", logfile->buf, strerror(errno)); + else if (errno == EISDIR) + strbuf_addf(err, "there are still logs under '%s'", + logfile->buf); + else strbuf_addf(err, "unable to append to '%s': %s", logfile->buf, strerror(errno)); - return -1; - } + + return -1; } } else { logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666); -- cgit v1.2.1 From e404f459fdc0d42e9ea83084cb1acdd241c14de3 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 6 Jan 2017 17:22:34 +0100 Subject: log_ref_setup(): pass the open file descriptor back to the caller This function will most often be called by log_ref_write_1(), which wants to append to the reflog file. In that case, it is silly to close the file only for the caller to reopen it immediately. So, in the case that the file was opened, pass the open file descriptor back to the caller. Signed-off-by: Michael Haggerty Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- refs/files-backend.c | 39 ++++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 17 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index 27d4fd3f96..f723834d9b 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2719,19 +2719,23 @@ static int open_or_create_logfile(const char *path, void *cb) } /* - * Create a reflog for a ref. If force_create = 0, the reflog will - * only be created for certain refs (those for which - * should_autocreate_reflog returns non-zero. Otherwise, create it - * regardless of the ref name. Fill in *err and return -1 on failure. + * Create a reflog for a ref. Store its path to *logfile. If + * force_create = 0, only create the reflog for certain refs (those + * for which should_autocreate_reflog returns non-zero). Otherwise, + * create it regardless of the reference name. If the logfile already + * existed or was created, return 0 and set *logfd to the file + * descriptor opened for appending to the file. If no logfile exists + * and we decided not to create one, return 0 and set *logfd to -1. On + * failure, fill in *err, set *logfd to -1, and return -1. */ -static int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err, int force_create) +static int log_ref_setup(const char *refname, + struct strbuf *logfile, int *logfd, + struct strbuf *err, int force_create) { - int logfd; - strbuf_git_path(logfile, "logs/%s", refname); if (force_create || should_autocreate_reflog(refname)) { - if (raceproof_create_file(logfile->buf, open_or_create_logfile, &logfd)) { + if (raceproof_create_file(logfile->buf, open_or_create_logfile, logfd)) { if (errno == ENOENT) strbuf_addf(err, "unable to create directory for '%s': " "%s", logfile->buf, strerror(errno)); @@ -2745,8 +2749,8 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str return -1; } } else { - logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666); - if (logfd < 0) { + *logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666); + if (*logfd < 0) { if (errno == ENOENT || errno == EISDIR) { /* * The logfile doesn't already exist, @@ -2763,10 +2767,8 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str } } - if (logfd >= 0) { + if (*logfd >= 0) adjust_shared_perm(logfile->buf); - close(logfd); - } return 0; } @@ -2777,11 +2779,14 @@ static int files_create_reflog(struct ref_store *ref_store, { int ret; struct strbuf sb = STRBUF_INIT; + int fd; /* Check validity (but we don't need the result): */ files_downcast(ref_store, 0, "create_reflog"); - ret = log_ref_setup(refname, &sb, err, force_create); + ret = log_ref_setup(refname, &sb, &fd, err, force_create); + if (fd >= 0) + close(fd); strbuf_release(&sb); return ret; } @@ -2817,17 +2822,17 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1, struct strbuf *logfile, int flags, struct strbuf *err) { - int logfd, result, oflags = O_APPEND | O_WRONLY; + int logfd, result; if (log_all_ref_updates < 0) log_all_ref_updates = !is_bare_repository(); - result = log_ref_setup(refname, logfile, err, flags & REF_FORCE_CREATE_REFLOG); + result = log_ref_setup(refname, logfile, &logfd, err, + flags & REF_FORCE_CREATE_REFLOG); if (result) return result; - logfd = open(logfile->buf, oflags); if (logfd < 0) return 0; result = log_ref_write_fd(logfd, old_sha1, new_sha1, -- cgit v1.2.1 From 87b21e05ed869806f865ada8f5d977ce99f18f20 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 6 Jan 2017 17:22:35 +0100 Subject: log_ref_write_1(): don't depend on logfile argument It's unnecessary to pass a strbuf holding the reflog path up and down the call stack now that it is hardly needed by the callers. Remove the places where log_ref_write_1() uses it, in preparation for making it internal to log_ref_setup(). Signed-off-by: Michael Haggerty Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- refs/files-backend.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index f723834d9b..9c5e804191 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2838,14 +2838,18 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1, result = log_ref_write_fd(logfd, old_sha1, new_sha1, git_committer_info(0), msg); if (result) { - strbuf_addf(err, "unable to append to '%s': %s", logfile->buf, - strerror(errno)); + int save_errno = errno; + + strbuf_addf(err, "unable to append to '%s': %s", + git_path("logs/%s", refname), strerror(save_errno)); close(logfd); return -1; } if (close(logfd)) { - strbuf_addf(err, "unable to append to '%s': %s", logfile->buf, - strerror(errno)); + int save_errno = errno; + + strbuf_addf(err, "unable to append to '%s': %s", + git_path("logs/%s", refname), strerror(save_errno)); return -1; } return 0; -- cgit v1.2.1 From 4533e5343bb1171b932f787e345d6cff920a4e70 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 6 Jan 2017 17:22:36 +0100 Subject: log_ref_setup(): manage the name of the reflog file internally Instead of writing the name of the reflog file into a strbuf that is supplied by the caller but not needed there, write it into a local temporary buffer and remove the strbuf parameter entirely. And while we're adjusting the function signature, reorder the arguments to move the input parameters before the output parameters. Signed-off-by: Michael Haggerty Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- refs/files-backend.c | 69 ++++++++++++++++++++++++++-------------------------- 1 file changed, 34 insertions(+), 35 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index 9c5e804191..846380f38d 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2719,37 +2719,36 @@ static int open_or_create_logfile(const char *path, void *cb) } /* - * Create a reflog for a ref. Store its path to *logfile. If - * force_create = 0, only create the reflog for certain refs (those - * for which should_autocreate_reflog returns non-zero). Otherwise, - * create it regardless of the reference name. If the logfile already - * existed or was created, return 0 and set *logfd to the file - * descriptor opened for appending to the file. If no logfile exists - * and we decided not to create one, return 0 and set *logfd to -1. On - * failure, fill in *err, set *logfd to -1, and return -1. + * Create a reflog for a ref. If force_create = 0, only create the + * reflog for certain refs (those for which should_autocreate_reflog + * returns non-zero). Otherwise, create it regardless of the reference + * name. If the logfile already existed or was created, return 0 and + * set *logfd to the file descriptor opened for appending to the file. + * If no logfile exists and we decided not to create one, return 0 and + * set *logfd to -1. On failure, fill in *err, set *logfd to -1, and + * return -1. */ -static int log_ref_setup(const char *refname, - struct strbuf *logfile, int *logfd, - struct strbuf *err, int force_create) +static int log_ref_setup(const char *refname, int force_create, + int *logfd, struct strbuf *err) { - strbuf_git_path(logfile, "logs/%s", refname); + char *logfile = git_pathdup("logs/%s", refname); if (force_create || should_autocreate_reflog(refname)) { - if (raceproof_create_file(logfile->buf, open_or_create_logfile, logfd)) { + if (raceproof_create_file(logfile, open_or_create_logfile, logfd)) { if (errno == ENOENT) strbuf_addf(err, "unable to create directory for '%s': " - "%s", logfile->buf, strerror(errno)); + "%s", logfile, strerror(errno)); else if (errno == EISDIR) strbuf_addf(err, "there are still logs under '%s'", - logfile->buf); + logfile); else strbuf_addf(err, "unable to append to '%s': %s", - logfile->buf, strerror(errno)); + logfile, strerror(errno)); - return -1; + goto error; } } else { - *logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666); + *logfd = open(logfile, O_APPEND | O_WRONLY, 0666); if (*logfd < 0) { if (errno == ENOENT || errno == EISDIR) { /* @@ -2761,34 +2760,39 @@ static int log_ref_setup(const char *refname, ; } else { strbuf_addf(err, "unable to append to '%s': %s", - logfile->buf, strerror(errno)); - return -1; + logfile, strerror(errno)); + goto error; } } } if (*logfd >= 0) - adjust_shared_perm(logfile->buf); + adjust_shared_perm(logfile); + free(logfile); return 0; + +error: + free(logfile); + return -1; } static int files_create_reflog(struct ref_store *ref_store, const char *refname, int force_create, struct strbuf *err) { - int ret; - struct strbuf sb = STRBUF_INIT; int fd; /* Check validity (but we don't need the result): */ files_downcast(ref_store, 0, "create_reflog"); - ret = log_ref_setup(refname, &sb, &fd, err, force_create); + if (log_ref_setup(refname, force_create, &fd, err)) + return -1; + if (fd >= 0) close(fd); - strbuf_release(&sb); - return ret; + + return 0; } static int log_ref_write_fd(int fd, const unsigned char *old_sha1, @@ -2819,16 +2823,15 @@ static int log_ref_write_fd(int fd, const unsigned char *old_sha1, static int log_ref_write_1(const char *refname, const unsigned char *old_sha1, const unsigned char *new_sha1, const char *msg, - struct strbuf *logfile, int flags, - struct strbuf *err) + int flags, struct strbuf *err) { int logfd, result; if (log_all_ref_updates < 0) log_all_ref_updates = !is_bare_repository(); - result = log_ref_setup(refname, logfile, &logfd, err, - flags & REF_FORCE_CREATE_REFLOG); + result = log_ref_setup(refname, flags & REF_FORCE_CREATE_REFLOG, + &logfd, err); if (result) return result; @@ -2859,11 +2862,7 @@ int files_log_ref_write(const char *refname, const unsigned char *old_sha1, const unsigned char *new_sha1, const char *msg, int flags, struct strbuf *err) { - struct strbuf sb = STRBUF_INIT; - int ret = log_ref_write_1(refname, old_sha1, new_sha1, msg, &sb, flags, - err); - strbuf_release(&sb); - return ret; + return log_ref_write_1(refname, old_sha1, new_sha1, msg, flags, err); } /* -- cgit v1.2.1 From fc31955294a1baf4845558451494ce42ee70444b Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 6 Jan 2017 17:22:37 +0100 Subject: log_ref_write_1(): inline function Now files_log_ref_write() doesn't do anything beyond call log_ref_write_1(), so inline the latter into the former. Signed-off-by: Michael Haggerty Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- refs/files-backend.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index 846380f38d..39d6f5b782 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2821,9 +2821,9 @@ static int log_ref_write_fd(int fd, const unsigned char *old_sha1, return 0; } -static int log_ref_write_1(const char *refname, const unsigned char *old_sha1, - const unsigned char *new_sha1, const char *msg, - int flags, struct strbuf *err) +int files_log_ref_write(const char *refname, const unsigned char *old_sha1, + const unsigned char *new_sha1, const char *msg, + int flags, struct strbuf *err) { int logfd, result; @@ -2858,13 +2858,6 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1, return 0; } -int files_log_ref_write(const char *refname, const unsigned char *old_sha1, - const unsigned char *new_sha1, const char *msg, - int flags, struct strbuf *err) -{ - return log_ref_write_1(refname, old_sha1, new_sha1, msg, flags, err); -} - /* * Write sha1 into the open lockfile, then close the lockfile. On * errors, rollback the lockfile, fill in *err and -- cgit v1.2.1 From 0e81d016f11ce93494b73d68220f59cf4b5a1da2 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 6 Jan 2017 17:22:38 +0100 Subject: delete_ref_loose(): derive loose reference path from lock It is simpler to derive the path to the file that must be deleted from "lock->ref_name" than from the lock_file object. Signed-off-by: Michael Haggerty Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- refs/files-backend.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index 39d6f5b782..4d5536458b 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2430,10 +2430,7 @@ static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err) * loose. The loose file name is the same as the * lockfile name, minus ".lock": */ - char *loose_filename = get_locked_file_path(lock->lk); - int res = unlink_or_msg(loose_filename, err); - free(loose_filename); - if (res) + if (unlink_or_msg(git_path("%s", lock->ref_name), err)) return 1; } return 0; -- cgit v1.2.1 From ce0af24de01114552e37ec87dbb948013454f3e1 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 6 Jan 2017 17:22:39 +0100 Subject: delete_ref_loose(): inline function It was hardly doing anything anymore, and had only one caller. Signed-off-by: Michael Haggerty Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- refs/files-backend.c | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index 4d5536458b..9abd7c39cd 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2421,21 +2421,6 @@ static int repack_without_refs(struct files_ref_store *refs, return ret; } -static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err) -{ - assert(err); - - if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) { - /* - * loose. The loose file name is the same as the - * lockfile name, minus ".lock": - */ - if (unlink_or_msg(git_path("%s", lock->ref_name), err)) - return 1; - } - return 0; -} - static int files_delete_refs(struct ref_store *ref_store, struct string_list *refnames, unsigned int flags) { @@ -3788,9 +3773,13 @@ static int files_transaction_commit(struct ref_store *ref_store, if (update->flags & REF_DELETING && !(update->flags & REF_LOG_ONLY)) { - if (delete_ref_loose(lock, update->type, err)) { - ret = TRANSACTION_GENERIC_ERROR; - goto cleanup; + if (!(update->type & REF_ISPACKED) || + update->type & REF_ISSYMREF) { + /* It is a loose reference. */ + if (unlink_or_msg(git_path("%s", lock->ref_name), err)) { + ret = TRANSACTION_GENERIC_ERROR; + goto cleanup; + } } if (!(update->flags & REF_ISPRUNING)) -- cgit v1.2.1 From 730e0342869744e33a2db76fda5f3acf28fc889c Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 6 Jan 2017 17:22:40 +0100 Subject: try_remove_empty_parents(): rename parameter "name" -> "refname" This is the standard nomenclature. Signed-off-by: Michael Haggerty Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- refs/files-backend.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index 9abd7c39cd..92a9d99ba5 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2282,13 +2282,13 @@ static int pack_if_possible_fn(struct ref_entry *entry, void *cb_data) /* * Remove empty parents, but spare refs/ and immediate subdirs. - * Note: munges *name. + * Note: munges *refname. */ -static void try_remove_empty_parents(char *name) +static void try_remove_empty_parents(char *refname) { char *p, *q; int i; - p = name; + p = refname; for (i = 0; i < 2; i++) { /* refs/{heads,tags,...}/ */ while (*p && *p != '/') p++; @@ -2306,7 +2306,7 @@ static void try_remove_empty_parents(char *name) if (q == p) break; *q = '\0'; - if (rmdir(git_path("%s", name))) + if (rmdir(git_path("%s", refname))) break; } } -- cgit v1.2.1 From 8bdaecb402cc024ff1ce7fc8856e4ee87f9102f1 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 6 Jan 2017 17:22:41 +0100 Subject: try_remove_empty_parents(): don't trash argument contents It's bad manners and surprising and therefore error-prone. Signed-off-by: Michael Haggerty Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- refs/files-backend.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index 92a9d99ba5..88f8c7aa4b 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2282,13 +2282,15 @@ static int pack_if_possible_fn(struct ref_entry *entry, void *cb_data) /* * Remove empty parents, but spare refs/ and immediate subdirs. - * Note: munges *refname. */ -static void try_remove_empty_parents(char *refname) +static void try_remove_empty_parents(const char *refname) { + struct strbuf buf = STRBUF_INIT; char *p, *q; int i; - p = refname; + + strbuf_addstr(&buf, refname); + p = buf.buf; for (i = 0; i < 2; i++) { /* refs/{heads,tags,...}/ */ while (*p && *p != '/') p++; @@ -2296,8 +2298,7 @@ static void try_remove_empty_parents(char *refname) while (*p == '/') p++; } - for (q = p; *q; q++) - ; + q = buf.buf + buf.len; while (1) { while (q > p && *q != '/') q--; @@ -2305,10 +2306,11 @@ static void try_remove_empty_parents(char *refname) q--; if (q == p) break; - *q = '\0'; - if (rmdir(git_path("%s", refname))) + strbuf_setlen(&buf, q - buf.buf); + if (rmdir(git_path("%s", buf.buf))) break; } + strbuf_release(&buf); } /* make sure nobody touched the ref, and unlink */ -- cgit v1.2.1 From a8f0db2d99552d786fd1428a06d5fb8e0425dca8 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 6 Jan 2017 17:22:42 +0100 Subject: try_remove_empty_parents(): teach to remove parents of reflogs, too Add a new "flags" parameter that tells the function whether to remove empty parent directories of the loose reference file, of the reflog file, or both. The new functionality is not yet used. Signed-off-by: Michael Haggerty Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- refs/files-backend.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index 88f8c7aa4b..bce0022345 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2280,10 +2280,18 @@ static int pack_if_possible_fn(struct ref_entry *entry, void *cb_data) return 0; } +enum { + REMOVE_EMPTY_PARENTS_REF = 0x01, + REMOVE_EMPTY_PARENTS_REFLOG = 0x02 +}; + /* - * Remove empty parents, but spare refs/ and immediate subdirs. + * Remove empty parent directories associated with the specified + * reference and/or its reflog, but spare [logs/]refs/ and immediate + * subdirs. flags is a combination of REMOVE_EMPTY_PARENTS_REF and/or + * REMOVE_EMPTY_PARENTS_REFLOG. */ -static void try_remove_empty_parents(const char *refname) +static void try_remove_empty_parents(const char *refname, unsigned int flags) { struct strbuf buf = STRBUF_INIT; char *p, *q; @@ -2299,7 +2307,7 @@ static void try_remove_empty_parents(const char *refname) p++; } q = buf.buf + buf.len; - while (1) { + while (flags & (REMOVE_EMPTY_PARENTS_REF | REMOVE_EMPTY_PARENTS_REFLOG)) { while (q > p && *q != '/') q--; while (q > p && *(q-1) == '/') @@ -2307,8 +2315,12 @@ static void try_remove_empty_parents(const char *refname) if (q == p) break; strbuf_setlen(&buf, q - buf.buf); - if (rmdir(git_path("%s", buf.buf))) - break; + if ((flags & REMOVE_EMPTY_PARENTS_REF) && + rmdir(git_path("%s", buf.buf))) + flags &= ~REMOVE_EMPTY_PARENTS_REF; + if ((flags & REMOVE_EMPTY_PARENTS_REFLOG) && + rmdir(git_path("logs/%s", buf.buf))) + flags &= ~REMOVE_EMPTY_PARENTS_REFLOG; } strbuf_release(&buf); } @@ -2334,7 +2346,7 @@ static void prune_ref(struct ref_to_prune *r) } ref_transaction_free(transaction); strbuf_release(&err); - try_remove_empty_parents(r->name); + try_remove_empty_parents(r->name, REMOVE_EMPTY_PARENTS_REF); } static void prune_refs(struct ref_to_prune *r) -- cgit v1.2.1 From 446397774ad3b9f2ae81bcfbe0628e99c3db7e49 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 6 Jan 2017 17:22:43 +0100 Subject: files_transaction_commit(): clean up empty directories When deleting/pruning references, remove any directories that are made empty by the deletion of loose references or of reflogs. Otherwise such empty directories can survive forever and accumulate over time. (Even 'pack-refs', which is smart enough to remove the parent directories of loose references that it prunes, leaves directories that were already empty.) And now that files_transaction_commit() takes care of deleting the parent directories of loose references that it prunes, we don't have to do that in prune_ref() anymore. This change would be unwise if the *creation* of these directories could race with our deletion of them. But the earlier changes in this patch series made the creation paths robust against races, so now it is safe to tidy them up more aggressively. Signed-off-by: Michael Haggerty Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- refs/files-backend.c | 34 ++++++++++++++++++++++++++++------ refs/refs-internal.h | 11 +++++++++-- 2 files changed, 37 insertions(+), 8 deletions(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index bce0022345..c426575fe9 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2346,7 +2346,6 @@ static void prune_ref(struct ref_to_prune *r) } ref_transaction_free(transaction); strbuf_release(&err); - try_remove_empty_parents(r->name, REMOVE_EMPTY_PARENTS_REF); } static void prune_refs(struct ref_to_prune *r) @@ -3794,6 +3793,7 @@ static int files_transaction_commit(struct ref_store *ref_store, ret = TRANSACTION_GENERIC_ERROR; goto cleanup; } + update->flags |= REF_DELETED_LOOSE; } if (!(update->flags & REF_ISPRUNING)) @@ -3806,16 +3806,38 @@ static int files_transaction_commit(struct ref_store *ref_store, ret = TRANSACTION_GENERIC_ERROR; goto cleanup; } - for_each_string_list_item(ref_to_delete, &refs_to_delete) - unlink_or_warn(git_path("logs/%s", ref_to_delete->string)); + + /* Delete the reflogs of any references that were deleted: */ + for_each_string_list_item(ref_to_delete, &refs_to_delete) { + if (!unlink_or_warn(git_path("logs/%s", ref_to_delete->string))) + try_remove_empty_parents(ref_to_delete->string, + REMOVE_EMPTY_PARENTS_REFLOG); + } + clear_loose_ref_cache(refs); cleanup: transaction->state = REF_TRANSACTION_CLOSED; - for (i = 0; i < transaction->nr; i++) - if (transaction->updates[i]->backend_data) - unlock_ref(transaction->updates[i]->backend_data); + for (i = 0; i < transaction->nr; i++) { + struct ref_update *update = transaction->updates[i]; + struct ref_lock *lock = update->backend_data; + + if (lock) + unlock_ref(lock); + + if (update->flags & REF_DELETED_LOOSE) { + /* + * The loose reference was deleted. Delete any + * empty parent directories. (Note that this + * can only work because we have already + * removed the lockfile.) + */ + try_remove_empty_parents(update->refname, + REMOVE_EMPTY_PARENTS_REF); + } + } + string_list_clear(&refs_to_delete, 0); free(head_ref); string_list_clear(&affected_refnames, 0); diff --git a/refs/refs-internal.h b/refs/refs-internal.h index dc81acc902..15d5a1e088 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -55,6 +55,12 @@ */ #define REF_UPDATE_VIA_HEAD 0x100 +/* + * Used as a flag in ref_update::flags when the loose reference has + * been deleted. + */ +#define REF_DELETED_LOOSE 0x200 + /* * Return true iff refname is minimally safe. "Safe" here means that * deleting a loose reference by this name will not do any damage, for @@ -158,8 +164,9 @@ struct ref_update { /* * One or more of REF_HAVE_NEW, REF_HAVE_OLD, REF_NODEREF, - * REF_DELETING, REF_ISPRUNING, REF_LOG_ONLY, and - * REF_UPDATE_VIA_HEAD: + * REF_DELETING, REF_ISPRUNING, REF_LOG_ONLY, + * REF_UPDATE_VIA_HEAD, REF_NEEDS_COMMIT, and + * REF_DELETED_LOOSE: */ unsigned int flags; -- cgit v1.2.1