From 19dd7d06e5d2c58895dd101025c013404025e192 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Thu, 5 May 2016 13:22:23 +0200 Subject: t1404: demonstrate a bug resolving references Add some tests checking that it is possible to work with a reference even if there is an empty directory where the loose ref would be stored. One of the new tests demonstrates a bug that has been with us since at least 2.5.0--single reference lookup gives up when it sees the directory, even if the reference exists as a packed ref. This probably hasn't been reported before because Git usually cleans up empty directories when packing references. This bug will be fixed shortly. Signed-off-by: Michael Haggerty --- t/t1404-update-ref-df-conflicts.sh | 76 +++++++++++++++++++++++++++++++++++++- 1 file changed, 75 insertions(+), 1 deletion(-) diff --git a/t/t1404-update-ref-df-conflicts.sh b/t/t1404-update-ref-df-conflicts.sh index 66bafb5cf..16752a999 100755 --- a/t/t1404-update-ref-df-conflicts.sh +++ b/t/t1404-update-ref-df-conflicts.sh @@ -28,7 +28,9 @@ Q="'" test_expect_success 'setup' ' git commit --allow-empty -m Initial && - C=$(git rev-parse HEAD) + C=$(git rev-parse HEAD) && + git commit --allow-empty -m Second && + D=$(git rev-parse HEAD) ' @@ -104,4 +106,76 @@ test_expect_success 'one new ref is a simple prefix of another' ' ' +test_expect_failure 'empty directory should not fool rev-parse' ' + prefix=refs/e-rev-parse && + git update-ref $prefix/foo $C && + git pack-refs --all && + mkdir -p .git/$prefix/foo/bar/baz && + echo "$C" >expected && + git rev-parse $prefix/foo >actual && + test_cmp expected actual +' + +test_expect_success 'empty directory should not fool for-each-ref' ' + prefix=refs/e-for-each-ref && + git update-ref $prefix/foo $C && + git for-each-ref $prefix >expected && + git pack-refs --all && + mkdir -p .git/$prefix/foo/bar/baz && + git for-each-ref $prefix >actual && + test_cmp expected actual +' + +test_expect_success 'empty directory should not fool create' ' + prefix=refs/e-create && + mkdir -p .git/$prefix/foo/bar/baz && + printf "create %s $C\n" $prefix/foo | + git update-ref --stdin +' + +test_expect_success 'empty directory should not fool verify' ' + prefix=refs/e-verify && + git update-ref $prefix/foo $C && + git pack-refs --all && + mkdir -p .git/$prefix/foo/bar/baz && + printf "verify %s $C\n" $prefix/foo | + git update-ref --stdin +' + +test_expect_success 'empty directory should not fool 1-arg update' ' + prefix=refs/e-update-1 && + git update-ref $prefix/foo $C && + git pack-refs --all && + mkdir -p .git/$prefix/foo/bar/baz && + printf "update %s $D\n" $prefix/foo | + git update-ref --stdin +' + +test_expect_success 'empty directory should not fool 2-arg update' ' + prefix=refs/e-update-2 && + git update-ref $prefix/foo $C && + git pack-refs --all && + mkdir -p .git/$prefix/foo/bar/baz && + printf "update %s $D $C\n" $prefix/foo | + git update-ref --stdin +' + +test_expect_success 'empty directory should not fool 0-arg delete' ' + prefix=refs/e-delete-0 && + git update-ref $prefix/foo $C && + git pack-refs --all && + mkdir -p .git/$prefix/foo/bar/baz && + printf "delete %s\n" $prefix/foo | + git update-ref --stdin +' + +test_expect_success 'empty directory should not fool 1-arg delete' ' + prefix=refs/e-delete-1 && + git update-ref $prefix/foo $C && + git pack-refs --all && + mkdir -p .git/$prefix/foo/bar/baz && + printf "delete %s $C\n" $prefix/foo | + git update-ref --stdin +' + test_done -- cgit v1.2.1 From 5387c0d8839e366c44838c808ccc20eb7f9bd358 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Thu, 5 May 2016 15:33:03 +0200 Subject: commit_ref(): if there is an empty dir in the way, delete it Part of the bug revealed in the last commit is that resolve_ref_unsafe() incorrectly returns EISDIR if it finds a directory in the place where it is looking for a loose reference, even if the corresponding packed reference exists. lock_ref_sha1_basic() notices the bogus EISDIR, and use it as an indication that it should call remove_empty_directories() and call resolve_ref_unsafe() again. But resolve_ref_unsafe() shouldn't report EISDIR in this case. If we would simply make that change, then remove_empty_directories() wouldn't get called anymore, and the empty directory would get in the way when commit_ref() calls commit_lock_file() to rename the lockfile into place. So instead of relying on lock_ref_sha1_basic() to delete empty directories, teach commit_ref(), just before calling commit_lock_file(), to check whether a directory is in the way, and if so, try to delete it. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/refs/files-backend.c b/refs/files-backend.c index 1f3807641..ad9cd8645 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2457,6 +2457,30 @@ static int close_ref(struct ref_lock *lock) static int commit_ref(struct ref_lock *lock) { + char *path = get_locked_file_path(lock->lk); + struct stat st; + + if (!lstat(path, &st) && S_ISDIR(st.st_mode)) { + /* + * There is a directory at the path we want to rename + * the lockfile to. Hopefully it is empty; try to + * delete it. + */ + size_t len = strlen(path); + struct strbuf sb_path = STRBUF_INIT; + + strbuf_attach(&sb_path, path, len, len); + + /* + * If this fails, commit_lock_file() will also fail + * and will report the problem. + */ + remove_empty_directories(&sb_path); + strbuf_release(&sb_path); + } else { + free(path); + } + if (commit_lock_file(lock->lk)) return -1; return 0; -- cgit v1.2.1 From e167a5673e25b960dce118fb967d54da30b69def Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Thu, 5 May 2016 14:09:41 +0200 Subject: read_raw_ref(): don't get confused by an empty directory Even if there is an empty directory where we look for the loose version of a reference, check for a packed reference before giving up. This fixes the failing test that was introduced two commits ago. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 11 ++++++++++- t/t1404-update-ref-df-conflicts.sh | 2 +- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index ad9cd8645..0cc116d67 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1477,7 +1477,16 @@ stat_ref: /* Is it a directory? */ if (S_ISDIR(st.st_mode)) { - errno = EISDIR; + /* + * Even though there is a directory where the loose + * ref is supposed to be, there could still be a + * packed ref: + */ + if (resolve_missing_loose_ref(refname, sha1, flags)) { + errno = EISDIR; + goto out; + } + ret = 0; goto out; } diff --git a/t/t1404-update-ref-df-conflicts.sh b/t/t1404-update-ref-df-conflicts.sh index 16752a999..6d869d128 100755 --- a/t/t1404-update-ref-df-conflicts.sh +++ b/t/t1404-update-ref-df-conflicts.sh @@ -106,7 +106,7 @@ test_expect_success 'one new ref is a simple prefix of another' ' ' -test_expect_failure 'empty directory should not fool rev-parse' ' +test_expect_success 'empty directory should not fool rev-parse' ' prefix=refs/e-rev-parse && git update-ref $prefix/foo $C && git pack-refs --all && -- cgit v1.2.1 From e95792e532bde75fd4a1e91aecfcf9a28ba23955 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sun, 24 Apr 2016 04:34:12 +0200 Subject: safe_create_leading_directories(): improve docstring Document the difference between this function and safe_create_leading_directories_const(), and that the former restores path before returning. Signed-off-by: Michael Haggerty --- cache.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cache.h b/cache.h index 2711048ca..4134f648e 100644 --- a/cache.h +++ b/cache.h @@ -993,6 +993,11 @@ int adjust_shared_perm(const char *path); * 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. + * + * safe_create_leading_directories() temporarily changes path while it + * is working but restores it before returning. + * safe_create_leading_directories_const() doesn't modify path, even + * temporarily. */ enum scld_error { SCLD_OK = 0, -- cgit v1.2.1 From 728af2832c3e58222965521682414adb9a80932b Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sun, 24 Apr 2016 07:07:59 +0200 Subject: remove_dir_recursively(): add docstring Add a docstring for the remove_dir_recursively() function and the REMOVE_DIR_* flags that can be passed to it. Signed-off-by: Michael Haggerty --- dir.h | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/dir.h b/dir.h index 301b737a3..5f19acc27 100644 --- a/dir.h +++ b/dir.h @@ -262,9 +262,32 @@ extern int is_empty_dir(const char *dir); extern void setup_standard_excludes(struct dir_struct *dir); + +/* Constants for remove_dir_recursively: */ + +/* + * If a non-directory is found within path, stop and return an error. + * (In this case some empty directories might already have been + * removed.) + */ #define REMOVE_DIR_EMPTY_ONLY 01 + +/* + * If any Git work trees are found within path, skip them without + * considering it an error. + */ #define REMOVE_DIR_KEEP_NESTED_GIT 02 + +/* Remove the contents of path, but leave path itself. */ #define REMOVE_DIR_KEEP_TOPLEVEL 04 + +/* + * Remove path and its contents, recursively. flags is a combination + * of the above REMOVE_DIR_* constants. Return 0 on success. + * + * This function uses path as temporary scratch space, but restores it + * before returning. + */ extern int remove_dir_recursively(struct strbuf *path, int flag); /* tries to remove the path with empty directories along it, ignores ENOENT */ -- cgit v1.2.1 From 39950fef8bb45e944655e48393ee04c0b33211f5 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 27 Apr 2016 12:39:11 +0200 Subject: refname_is_safe(): use skip_prefix() Signed-off-by: Michael Haggerty --- refs.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/refs.c b/refs.c index 87dc82f1d..578915274 100644 --- a/refs.c +++ b/refs.c @@ -120,17 +120,19 @@ int check_refname_format(const char *refname, int flags) int refname_is_safe(const char *refname) { - if (starts_with(refname, "refs/")) { + const char *rest; + + if (skip_prefix(refname, "refs/", &rest)) { char *buf; int result; - buf = xmallocz(strlen(refname)); /* * Does the refname try to escape refs/? * For example: refs/foo/../bar is safe but refs/foo/../../bar * is not. */ - result = !normalize_path_copy(buf, refname + strlen("refs/")); + buf = xmallocz(strlen(rest)); + result = !normalize_path_copy(buf, rest); free(buf); return result; } -- cgit v1.2.1 From 35db25c65f6f77c153ef2b1183ea7821236201c8 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 27 Apr 2016 12:42:27 +0200 Subject: refname_is_safe(): don't allow the empty string Signed-off-by: Michael Haggerty --- refs.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/refs.c b/refs.c index 578915274..ca0280f7e 100644 --- a/refs.c +++ b/refs.c @@ -136,11 +136,12 @@ int refname_is_safe(const char *refname) free(buf); return result; } - while (*refname) { + + do { if (!isupper(*refname) && *refname != '_') return 0; refname++; - } + } while (*refname); return 1; } -- cgit v1.2.1 From e40f3557f7e767bd2be2a824bc3bc2379aa69931 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 27 Apr 2016 12:40:39 +0200 Subject: refname_is_safe(): insist that the refname already be normalized The reference name is going to be compared to other reference names, so it should be in its normalized form. Signed-off-by: Michael Haggerty --- refs.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/refs.c b/refs.c index ca0280f7e..b18d9959a 100644 --- a/refs.c +++ b/refs.c @@ -125,14 +125,19 @@ int refname_is_safe(const char *refname) if (skip_prefix(refname, "refs/", &rest)) { char *buf; int result; + size_t restlen = strlen(rest); + + /* rest must not be empty, or start or end with "/" */ + if (!restlen || *rest == '/' || rest[restlen - 1] == '/') + return 0; /* * Does the refname try to escape refs/? * For example: refs/foo/../bar is safe but refs/foo/../../bar * is not. */ - buf = xmallocz(strlen(rest)); - result = !normalize_path_copy(buf, rest); + buf = xmallocz(restlen); + result = !normalize_path_copy(buf, rest) && !strcmp(buf, rest); free(buf); return result; } -- cgit v1.2.1 From 76fc394d50efef8f1308a0f0d56087f502dac689 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Tue, 26 Apr 2016 09:09:51 +0200 Subject: commit_ref_update(): write error message to *err, not stderr Signed-off-by: Michael Haggerty --- refs/files-backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 0cc116d67..2d3a8c669 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2719,7 +2719,7 @@ static int commit_ref_update(struct ref_lock *lock, } } if (commit_ref(lock)) { - error("Couldn't set %s", lock->ref_name); + strbuf_addf(err, "Couldn't set %s", lock->ref_name); unlock_ref(lock); return -1; } -- cgit v1.2.1 From e711b1af2ead2ffad5c510aadbbc387c7d8aa4c7 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Thu, 21 Apr 2016 23:42:19 +0200 Subject: rename_ref(): remove unneeded local variable Signed-off-by: Michael Haggerty --- refs/files-backend.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 2d3a8c669..80d346fd3 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2360,20 +2360,17 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms struct ref_lock *lock; struct stat loginfo; int log = !lstat(git_path("logs/%s", oldrefname), &loginfo); - const char *symref = NULL; struct strbuf err = STRBUF_INIT; if (log && S_ISLNK(loginfo.st_mode)) return error("reflog for %s is a symlink", oldrefname); - symref = resolve_ref_unsafe(oldrefname, RESOLVE_REF_READING, - orig_sha1, &flag); + if (!resolve_ref_unsafe(oldrefname, RESOLVE_REF_READING, orig_sha1, &flag)) + return error("refname %s not found", oldrefname); + if (flag & REF_ISSYMREF) return error("refname %s is a symbolic ref, renaming it is not supported", oldrefname); - if (!symref) - return error("refname %s not found", oldrefname); - if (!rename_ref_available(oldrefname, newrefname)) return 1; -- cgit v1.2.1 From efe472813d60befd72d6e2797934c90b22a26c93 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 22 Apr 2016 00:02:50 +0200 Subject: ref_transaction_commit(): remove local variables n and updates These microoptimizations don't make a significant difference in speed. And they cause problems if somebody ever wants to modify the function to add updates to a transaction as part of processing it, as will happen shortly. Make the same changes in initial_ref_transaction_commit(). Signed-off-by: Michael Haggerty --- refs/files-backend.c | 42 ++++++++++++++++++++---------------------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 80d346fd3..814e230fa 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -3076,8 +3076,6 @@ int ref_transaction_commit(struct ref_transaction *transaction, struct strbuf *err) { int ret = 0, i; - int n = transaction->nr; - struct ref_update **updates = transaction->updates; struct string_list refs_to_delete = STRING_LIST_INIT_NODUP; struct string_list_item *ref_to_delete; struct string_list affected_refnames = STRING_LIST_INIT_NODUP; @@ -3087,14 +3085,15 @@ int ref_transaction_commit(struct ref_transaction *transaction, if (transaction->state != REF_TRANSACTION_OPEN) die("BUG: commit called for transaction that is not open"); - if (!n) { + if (!transaction->nr) { transaction->state = REF_TRANSACTION_CLOSED; return 0; } /* Fail if a refname appears more than once in the transaction: */ - for (i = 0; i < n; i++) - string_list_append(&affected_refnames, updates[i]->refname); + for (i = 0; i < transaction->nr; i++) + string_list_append(&affected_refnames, + transaction->updates[i]->refname); string_list_sort(&affected_refnames); if (ref_update_reject_duplicates(&affected_refnames, err)) { ret = TRANSACTION_GENERIC_ERROR; @@ -3107,8 +3106,8 @@ int ref_transaction_commit(struct ref_transaction *transaction, * lockfiles, ready to be activated. Only keep one lockfile * open at a time to avoid running out of file descriptors. */ - for (i = 0; i < n; i++) { - struct ref_update *update = updates[i]; + for (i = 0; i < transaction->nr; i++) { + struct ref_update *update = transaction->updates[i]; if ((update->flags & REF_HAVE_NEW) && is_null_sha1(update->new_sha1)) @@ -3178,8 +3177,8 @@ int ref_transaction_commit(struct ref_transaction *transaction, } /* Perform updates first so live commits remain referenced */ - for (i = 0; i < n; i++) { - struct ref_update *update = updates[i]; + for (i = 0; i < transaction->nr; i++) { + struct ref_update *update = transaction->updates[i]; if (update->flags & REF_NEEDS_COMMIT) { if (commit_ref_update(update->lock, @@ -3197,8 +3196,8 @@ int ref_transaction_commit(struct ref_transaction *transaction, } /* Perform deletes now that updates are safely completed */ - for (i = 0; i < n; i++) { - struct ref_update *update = updates[i]; + for (i = 0; i < transaction->nr; i++) { + struct ref_update *update = transaction->updates[i]; if (update->flags & REF_DELETING) { if (delete_ref_loose(update->lock, update->type, err)) { @@ -3223,9 +3222,9 @@ int ref_transaction_commit(struct ref_transaction *transaction, cleanup: transaction->state = REF_TRANSACTION_CLOSED; - for (i = 0; i < n; i++) - if (updates[i]->lock) - unlock_ref(updates[i]->lock); + for (i = 0; i < transaction->nr; i++) + if (transaction->updates[i]->lock) + unlock_ref(transaction->updates[i]->lock); string_list_clear(&refs_to_delete, 0); string_list_clear(&affected_refnames, 0); return ret; @@ -3243,8 +3242,6 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction, struct strbuf *err) { int ret = 0, i; - int n = transaction->nr; - struct ref_update **updates = transaction->updates; struct string_list affected_refnames = STRING_LIST_INIT_NODUP; assert(err); @@ -3253,8 +3250,9 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction, die("BUG: commit called for transaction that is not open"); /* Fail if a refname appears more than once in the transaction: */ - for (i = 0; i < n; i++) - string_list_append(&affected_refnames, updates[i]->refname); + for (i = 0; i < transaction->nr; i++) + string_list_append(&affected_refnames, + transaction->updates[i]->refname); string_list_sort(&affected_refnames); if (ref_update_reject_duplicates(&affected_refnames, err)) { ret = TRANSACTION_GENERIC_ERROR; @@ -3276,8 +3274,8 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction, if (for_each_rawref(ref_present, &affected_refnames)) die("BUG: initial ref transaction called with existing refs"); - for (i = 0; i < n; i++) { - struct ref_update *update = updates[i]; + for (i = 0; i < transaction->nr; i++) { + struct ref_update *update = transaction->updates[i]; if ((update->flags & REF_HAVE_OLD) && !is_null_sha1(update->old_sha1)) @@ -3297,8 +3295,8 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction, goto cleanup; } - for (i = 0; i < n; i++) { - struct ref_update *update = updates[i]; + for (i = 0; i < transaction->nr; i++) { + struct ref_update *update = transaction->updates[i]; if ((update->flags & REF_HAVE_NEW) && !is_null_sha1(update->new_sha1)) -- cgit v1.2.1 From 3a0b6b9aba844075e802a6dc4c24622b34ab535b Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Tue, 26 Apr 2016 03:06:23 +0200 Subject: read_raw_ref(): rename flags argument to type This will hopefully reduce confusion with the "flags" arguments that are used in many functions in this module as an input parameter to choose how the function should operate. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 18 +++++++++--------- refs/refs-internal.h | 2 +- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 814e230fa..1657ce751 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1395,18 +1395,18 @@ static int resolve_missing_loose_ref(const char *refname, * * If the ref is symbolic, fill in *symref with the referrent * (e.g. "refs/heads/master") and return 0. The caller is responsible - * for validating the referrent. Set REF_ISSYMREF in flags. + * for validating the referrent. Set REF_ISSYMREF in type. * * If the ref doesn't exist, set errno to ENOENT and return -1. * * If the ref exists but is neither a symbolic ref nor a sha1, it is - * broken. Set REF_ISBROKEN in flags, set errno to EINVAL, and return + * broken. Set REF_ISBROKEN in type, set errno to EINVAL, and return * -1. * * If there is another error reading the ref, set errno appropriately and * return -1. * - * Backend-specific flags might be set in flags as well, regardless of + * Backend-specific flags might be set in type as well, regardless of * outcome. * * sb_path is workspace: the caller should allocate and free it. @@ -1419,7 +1419,7 @@ static int resolve_missing_loose_ref(const char *refname, * refname will still be valid and unchanged. */ int read_raw_ref(const char *refname, unsigned char *sha1, - struct strbuf *symref, unsigned int *flags) + struct strbuf *symref, unsigned int *type) { struct strbuf sb_contents = STRBUF_INIT; struct strbuf sb_path = STRBUF_INIT; @@ -1448,7 +1448,7 @@ stat_ref: if (lstat(path, &st) < 0) { if (errno != ENOENT) goto out; - if (resolve_missing_loose_ref(refname, sha1, flags)) { + if (resolve_missing_loose_ref(refname, sha1, type)) { errno = ENOENT; goto out; } @@ -1469,7 +1469,7 @@ stat_ref: if (starts_with(sb_contents.buf, "refs/") && !check_refname_format(sb_contents.buf, 0)) { strbuf_swap(&sb_contents, symref); - *flags |= REF_ISSYMREF; + *type |= REF_ISSYMREF; ret = 0; goto out; } @@ -1482,7 +1482,7 @@ stat_ref: * ref is supposed to be, there could still be a * packed ref: */ - if (resolve_missing_loose_ref(refname, sha1, flags)) { + if (resolve_missing_loose_ref(refname, sha1, type)) { errno = EISDIR; goto out; } @@ -1519,7 +1519,7 @@ stat_ref: strbuf_reset(symref); strbuf_addstr(symref, buf); - *flags |= REF_ISSYMREF; + *type |= REF_ISSYMREF; ret = 0; goto out; } @@ -1530,7 +1530,7 @@ stat_ref: */ if (get_sha1_hex(buf, sha1) || (buf[40] != '\0' && !isspace(buf[40]))) { - *flags |= REF_ISBROKEN; + *type |= REF_ISBROKEN; errno = EINVAL; goto out; } diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 3a4f634cb..0b047f67b 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -210,6 +210,6 @@ int do_for_each_ref(const char *submodule, const char *base, each_ref_fn fn, int trim, int flags, void *cb_data); int read_raw_ref(const char *refname, unsigned char *sha1, - struct strbuf *symref, unsigned int *flags); + struct strbuf *symref, unsigned int *type); #endif /* REFS_REFS_INTERNAL_H */ -- cgit v1.2.1 From fa96ea1b883bf83fc488f999c58396bbab1860c1 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 22 Apr 2016 01:11:17 +0200 Subject: read_raw_ref(): clear *type at start of function This is more convenient and less error-prone for callers. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 1 + 1 file changed, 1 insertion(+) diff --git a/refs/files-backend.c b/refs/files-backend.c index 1657ce751..3c99eef42 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1430,6 +1430,7 @@ int read_raw_ref(const char *refname, unsigned char *sha1, int ret = -1; int save_errno; + *type = 0; strbuf_reset(&sb_path); strbuf_git_path(&sb_path, "%s", refname); path = sb_path.buf; -- cgit v1.2.1 From 92b380931ee8beacb0c09635432b38a02b9fcc7e Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 22 Apr 2016 01:11:17 +0200 Subject: read_raw_ref(): rename symref argument to referent After all, it doesn't hold the symbolic reference, but rather the reference referred to. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 21 +++++++++++---------- refs/refs-internal.h | 2 +- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 3c99eef42..a23ade4ff 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1393,9 +1393,10 @@ static int resolve_missing_loose_ref(const char *refname, * * If the ref is a sha1, fill in sha1 and return 0. * - * If the ref is symbolic, fill in *symref with the referrent - * (e.g. "refs/heads/master") and return 0. The caller is responsible - * for validating the referrent. Set REF_ISSYMREF in type. + * If the ref is symbolic, fill in *referent with the name of the + * branch to which it refers (e.g. "refs/heads/master") and return 0. + * The caller is responsible for validating the referent. Set + * REF_ISSYMREF in type. * * If the ref doesn't exist, set errno to ENOENT and return -1. * @@ -1411,15 +1412,15 @@ static int resolve_missing_loose_ref(const char *refname, * * sb_path is workspace: the caller should allocate and free it. * - * It is OK for refname to point into symref. In this case: - * - if the function succeeds with REF_ISSYMREF, symref will be + * It is OK for refname to point into referent. In this case: + * - if the function succeeds with REF_ISSYMREF, referent will be * overwritten and the memory pointed to by refname might be changed * or even freed. - * - in all other cases, symref will be untouched, and therefore + * - in all other cases, referent will be untouched, and therefore * refname will still be valid and unchanged. */ int read_raw_ref(const char *refname, unsigned char *sha1, - struct strbuf *symref, unsigned int *type) + struct strbuf *referent, unsigned int *type) { struct strbuf sb_contents = STRBUF_INIT; struct strbuf sb_path = STRBUF_INIT; @@ -1469,7 +1470,7 @@ stat_ref: } if (starts_with(sb_contents.buf, "refs/") && !check_refname_format(sb_contents.buf, 0)) { - strbuf_swap(&sb_contents, symref); + strbuf_swap(&sb_contents, referent); *type |= REF_ISSYMREF; ret = 0; goto out; @@ -1518,8 +1519,8 @@ stat_ref: while (isspace(*buf)) buf++; - strbuf_reset(symref); - strbuf_addstr(symref, buf); + strbuf_reset(referent); + strbuf_addstr(referent, buf); *type |= REF_ISSYMREF; ret = 0; goto out; diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 0b047f67b..37a1a37ea 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -210,6 +210,6 @@ int do_for_each_ref(const char *submodule, const char *base, each_ref_fn fn, int trim, int flags, void *cb_data); int read_raw_ref(const char *refname, unsigned char *sha1, - struct strbuf *symref, unsigned int *type); + struct strbuf *referent, unsigned int *type); #endif /* REFS_REFS_INTERNAL_H */ -- cgit v1.2.1 From bb462b00286902f6cdbb66bb418c59b5c7894e0d Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sun, 24 Apr 2016 08:10:30 +0200 Subject: read_raw_ref(): improve docstring Among other things, document the (important!) requirement that input refname be checked for safety before calling this function. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 41 ++++++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index a23ade4ff..c41cf432d 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1389,33 +1389,40 @@ static int resolve_missing_loose_ref(const char *refname, } /* - * Read a raw ref from the filesystem or packed refs file. + * Read the specified reference from the filesystem or packed refs + * file, non-recursively. Set type to describe the reference, and: * - * If the ref is a sha1, fill in sha1 and return 0. + * - If refname is the name of a normal reference, fill in sha1 + * (leaving referent unchanged). * - * If the ref is symbolic, fill in *referent with the name of the - * branch to which it refers (e.g. "refs/heads/master") and return 0. - * The caller is responsible for validating the referent. Set - * REF_ISSYMREF in type. + * - If refname is the name of a symbolic reference, write the full + * name of the reference to which it refers (e.g. + * "refs/heads/master") to referent and set the REF_ISSYMREF bit in + * type (leaving sha1 unchanged). The caller is responsible for + * validating that referent is a valid reference name. * - * If the ref doesn't exist, set errno to ENOENT and return -1. + * WARNING: refname might be used as part of a filename, so it is + * important from a security standpoint that it be safe in the sense + * of refname_is_safe(). Moreover, for symrefs this function sets + * referent to whatever the repository says, which might not be a + * properly-formatted or even safe reference name. NEITHER INPUT NOR + * OUTPUT REFERENCE NAMES ARE VALIDATED WITHIN THIS FUNCTION. * - * If the ref exists but is neither a symbolic ref nor a sha1, it is - * broken. Set REF_ISBROKEN in type, set errno to EINVAL, and return - * -1. - * - * If there is another error reading the ref, set errno appropriately and - * return -1. + * Return 0 on success. If the ref doesn't exist, set errno to ENOENT + * and return -1. If the ref exists but is neither a symbolic ref nor + * a sha1, it is broken; set REF_ISBROKEN in type, set errno to + * EINVAL, and return -1. If there is another error reading the ref, + * set errno appropriately and return -1. * * Backend-specific flags might be set in type as well, regardless of * outcome. * - * sb_path is workspace: the caller should allocate and free it. + * It is OK for refname to point into referent. If so: * - * It is OK for refname to point into referent. In this case: * - if the function succeeds with REF_ISSYMREF, referent will be - * overwritten and the memory pointed to by refname might be changed - * or even freed. + * overwritten and the memory formerly pointed to by it might be + * changed or even freed. + * * - in all other cases, referent will be untouched, and therefore * refname will still be valid and unchanged. */ -- cgit v1.2.1 From cf596442c6a18268f3f0d95cf7615a613102746f Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 6 May 2016 17:25:31 +0200 Subject: read_raw_ref(): move docstring to header file Signed-off-by: Michael Haggerty --- refs/files-backend.c | 38 -------------------------------------- refs/refs-internal.h | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 38 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index c41cf432d..c2bd7b831 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1388,44 +1388,6 @@ static int resolve_missing_loose_ref(const char *refname, return -1; } -/* - * Read the specified reference from the filesystem or packed refs - * file, non-recursively. Set type to describe the reference, and: - * - * - If refname is the name of a normal reference, fill in sha1 - * (leaving referent unchanged). - * - * - If refname is the name of a symbolic reference, write the full - * name of the reference to which it refers (e.g. - * "refs/heads/master") to referent and set the REF_ISSYMREF bit in - * type (leaving sha1 unchanged). The caller is responsible for - * validating that referent is a valid reference name. - * - * WARNING: refname might be used as part of a filename, so it is - * important from a security standpoint that it be safe in the sense - * of refname_is_safe(). Moreover, for symrefs this function sets - * referent to whatever the repository says, which might not be a - * properly-formatted or even safe reference name. NEITHER INPUT NOR - * OUTPUT REFERENCE NAMES ARE VALIDATED WITHIN THIS FUNCTION. - * - * Return 0 on success. If the ref doesn't exist, set errno to ENOENT - * and return -1. If the ref exists but is neither a symbolic ref nor - * a sha1, it is broken; set REF_ISBROKEN in type, set errno to - * EINVAL, and return -1. If there is another error reading the ref, - * set errno appropriately and return -1. - * - * Backend-specific flags might be set in type as well, regardless of - * outcome. - * - * It is OK for refname to point into referent. If so: - * - * - if the function succeeds with REF_ISSYMREF, referent will be - * overwritten and the memory formerly pointed to by it might be - * changed or even freed. - * - * - in all other cases, referent will be untouched, and therefore - * refname will still be valid and unchanged. - */ int read_raw_ref(const char *refname, unsigned char *sha1, struct strbuf *referent, unsigned int *type) { diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 37a1a37ea..de7722e32 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -209,6 +209,44 @@ int rename_ref_available(const char *oldname, const char *newname); int do_for_each_ref(const char *submodule, const char *base, each_ref_fn fn, int trim, int flags, void *cb_data); +/* + * Read the specified reference from the filesystem or packed refs + * file, non-recursively. Set type to describe the reference, and: + * + * - If refname is the name of a normal reference, fill in sha1 + * (leaving referent unchanged). + * + * - If refname is the name of a symbolic reference, write the full + * name of the reference to which it refers (e.g. + * "refs/heads/master") to referent and set the REF_ISSYMREF bit in + * type (leaving sha1 unchanged). The caller is responsible for + * validating that referent is a valid reference name. + * + * WARNING: refname might be used as part of a filename, so it is + * important from a security standpoint that it be safe in the sense + * of refname_is_safe(). Moreover, for symrefs this function sets + * referent to whatever the repository says, which might not be a + * properly-formatted or even safe reference name. NEITHER INPUT NOR + * OUTPUT REFERENCE NAMES ARE VALIDATED WITHIN THIS FUNCTION. + * + * Return 0 on success. If the ref doesn't exist, set errno to ENOENT + * and return -1. If the ref exists but is neither a symbolic ref nor + * a sha1, it is broken; set REF_ISBROKEN in type, set errno to + * EINVAL, and return -1. If there is another error reading the ref, + * set errno appropriately and return -1. + * + * Backend-specific flags might be set in type as well, regardless of + * outcome. + * + * It is OK for refname to point into referent. If so: + * + * - if the function succeeds with REF_ISSYMREF, referent will be + * overwritten and the memory formerly pointed to by it might be + * changed or even freed. + * + * - in all other cases, referent will be untouched, and therefore + * refname will still be valid and unchanged. + */ int read_raw_ref(const char *refname, unsigned char *sha1, struct strbuf *referent, unsigned int *type); -- cgit v1.2.1 From bcb497d0f83f9c3e60f00fd2cc87130923329ed9 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 22 Apr 2016 09:13:00 +0200 Subject: lock_ref_sha1_basic(): remove unneeded local variable resolve_ref_unsafe() can cope with being called with NULL passed to its flags argument. So lock_ref_sha1_basic() can just hand its own type parameter through. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index c2bd7b831..dc247e0e7 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1738,7 +1738,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, const unsigned char *old_sha1, const struct string_list *extras, const struct string_list *skip, - unsigned int flags, int *type_p, + unsigned int flags, int *type, struct strbuf *err) { struct strbuf ref_file = STRBUF_INIT; @@ -1746,7 +1746,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, const char *orig_refname = refname; struct ref_lock *lock; int last_errno = 0; - int type; int lflags = 0; int mustexist = (old_sha1 && !is_null_sha1(old_sha1)); int resolve_flags = 0; @@ -1766,7 +1765,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, } refname = resolve_ref_unsafe(refname, resolve_flags, - lock->old_oid.hash, &type); + lock->old_oid.hash, type); if (!refname && errno == EISDIR) { /* * we are trying to lock foo but we used to @@ -1784,10 +1783,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, goto error_return; } refname = resolve_ref_unsafe(orig_refname, resolve_flags, - lock->old_oid.hash, &type); + lock->old_oid.hash, type); } - if (type_p) - *type_p = type; if (!refname) { last_errno = errno; if (last_errno != ENOTDIR || -- cgit v1.2.1 From 0568c8e9dce2aa0dd18f41f23e3465f3639e371e Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 27 Apr 2016 15:21:36 +0200 Subject: refs: make error messages more consistent * Always start error messages with a lower-case letter. * Always enclose reference names in single quotes. Signed-off-by: Michael Haggerty --- refs.c | 8 ++++---- refs/files-backend.c | 32 ++++++++++++++++---------------- t/t1400-update-ref.sh | 4 ++-- 3 files changed, 22 insertions(+), 22 deletions(-) diff --git a/refs.c b/refs.c index b18d9959a..ba1410595 100644 --- a/refs.c +++ b/refs.c @@ -504,7 +504,7 @@ static int write_pseudoref(const char *pseudoref, const unsigned char *sha1, filename = git_path("%s", pseudoref); fd = hold_lock_file_for_update(&lock, filename, LOCK_DIE_ON_ERROR); if (fd < 0) { - strbuf_addf(err, "Could not open '%s' for writing: %s", + strbuf_addf(err, "could not open '%s' for writing: %s", filename, strerror(errno)); return -1; } @@ -515,14 +515,14 @@ static int write_pseudoref(const char *pseudoref, const unsigned char *sha1, if (read_ref(pseudoref, actual_old_sha1)) die("could not read ref '%s'", pseudoref); if (hashcmp(actual_old_sha1, old_sha1)) { - strbuf_addf(err, "Unexpected sha1 when writing %s", pseudoref); + strbuf_addf(err, "unexpected sha1 when writing '%s'", pseudoref); rollback_lock_file(&lock); goto done; } } if (write_in_full(fd, buf.buf, buf.len) != buf.len) { - strbuf_addf(err, "Could not write to '%s'", filename); + strbuf_addf(err, "could not write to '%s'", filename); rollback_lock_file(&lock); goto done; } @@ -792,7 +792,7 @@ int ref_transaction_update(struct ref_transaction *transaction, if (new_sha1 && !is_null_sha1(new_sha1) && check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { - strbuf_addf(err, "refusing to update ref with bad name %s", + strbuf_addf(err, "refusing to update ref with bad name '%s'", refname); return -1; } diff --git a/refs/files-backend.c b/refs/files-backend.c index dc247e0e7..c978fe49c 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1701,7 +1701,7 @@ static int verify_lock(struct ref_lock *lock, lock->old_oid.hash, NULL)) { if (old_sha1) { int save_errno = errno; - strbuf_addf(err, "can't verify ref %s", lock->ref_name); + strbuf_addf(err, "can't verify ref '%s'", lock->ref_name); errno = save_errno; return -1; } else { @@ -1710,7 +1710,7 @@ static int verify_lock(struct ref_lock *lock, } } if (old_sha1 && hashcmp(lock->old_oid.hash, old_sha1)) { - strbuf_addf(err, "ref %s is at %s but expected %s", + strbuf_addf(err, "ref '%s' is at %s but expected %s", lock->ref_name, sha1_to_hex(lock->old_oid.hash), sha1_to_hex(old_sha1)); @@ -1790,7 +1790,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, if (last_errno != ENOTDIR || !verify_refname_available_dir(orig_refname, extras, skip, get_loose_refs(&ref_cache), err)) - strbuf_addf(err, "unable to resolve reference %s: %s", + strbuf_addf(err, "unable to resolve reference '%s': %s", orig_refname, strerror(last_errno)); goto error_return; @@ -1828,7 +1828,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, /* fall through */ default: last_errno = errno; - strbuf_addf(err, "unable to create directory for %s", + strbuf_addf(err, "unable to create directory for '%s'", ref_file.buf); goto error_return; } @@ -2473,7 +2473,7 @@ 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: " + strbuf_addf(err, "unable to create directory for '%s': " "%s", logfile->buf, strerror(errno)); return -1; } @@ -2487,7 +2487,7 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str if (errno == EISDIR) { if (remove_empty_directories(logfile)) { - strbuf_addf(err, "There are still logs under " + strbuf_addf(err, "there are still logs under " "'%s'", logfile->buf); return -1; } @@ -2495,7 +2495,7 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str } if (logfd < 0) { - strbuf_addf(err, "unable to append to %s: %s", + strbuf_addf(err, "unable to append to '%s': %s", logfile->buf, strerror(errno)); return -1; } @@ -2564,13 +2564,13 @@ 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, + strbuf_addf(err, "unable to append to '%s': %s", logfile->buf, strerror(errno)); close(logfd); return -1; } if (close(logfd)) { - strbuf_addf(err, "unable to append to %s: %s", logfile->buf, + strbuf_addf(err, "unable to append to '%s': %s", logfile->buf, strerror(errno)); return -1; } @@ -2611,14 +2611,14 @@ static int write_ref_to_lockfile(struct ref_lock *lock, o = parse_object(sha1); if (!o) { strbuf_addf(err, - "Trying to write ref %s with nonexistent object %s", + "trying to write ref '%s' with nonexistent object %s", lock->ref_name, sha1_to_hex(sha1)); unlock_ref(lock); return -1; } if (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) { strbuf_addf(err, - "Trying to write non-commit object %s to branch %s", + "trying to write non-commit object %s to branch '%s'", sha1_to_hex(sha1), lock->ref_name); unlock_ref(lock); return -1; @@ -2628,7 +2628,7 @@ static int write_ref_to_lockfile(struct ref_lock *lock, write_in_full(fd, &term, 1) != 1 || close_ref(lock) < 0) { strbuf_addf(err, - "Couldn't write %s", get_lock_file_path(lock->lk)); + "couldn't write '%s'", get_lock_file_path(lock->lk)); unlock_ref(lock); return -1; } @@ -2649,7 +2649,7 @@ static int commit_ref_update(struct ref_lock *lock, (strcmp(lock->ref_name, lock->orig_ref_name) && log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg, flags, err) < 0)) { char *old_msg = strbuf_detach(err, NULL); - strbuf_addf(err, "Cannot update the ref '%s': %s", + strbuf_addf(err, "cannot update the ref '%s': %s", lock->ref_name, old_msg); free(old_msg); unlock_ref(lock); @@ -2684,7 +2684,7 @@ static int commit_ref_update(struct ref_lock *lock, } } if (commit_ref(lock)) { - strbuf_addf(err, "Couldn't set %s", lock->ref_name); + strbuf_addf(err, "couldn't set '%s'", lock->ref_name); unlock_ref(lock); return -1; } @@ -3033,7 +3033,7 @@ static int ref_update_reject_duplicates(struct string_list *refnames, for (i = 1; i < n; i++) if (!strcmp(refnames->items[i - 1].string, refnames->items[i].string)) { strbuf_addf(err, - "Multiple updates for ref '%s' not allowed.", + "multiple updates for ref '%s' not allowed.", refnames->items[i].string); return 1; } @@ -3137,7 +3137,7 @@ int ref_transaction_commit(struct ref_transaction *transaction, * Close it to free up the file descriptor: */ if (close_ref(update->lock)) { - strbuf_addf(err, "Couldn't close %s.lock", + strbuf_addf(err, "couldn't close '%s.lock'", update->refname); goto cleanup; } diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index af1b20dd5..40b0ccedf 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -479,7 +479,7 @@ test_expect_success 'stdin fails with duplicate refs' ' create $a $m EOF test_must_fail git update-ref --stdin err && - grep "fatal: Multiple updates for ref '"'"'$a'"'"' not allowed." err + grep "fatal: multiple updates for ref '"'"'$a'"'"' not allowed." err ' test_expect_success 'stdin create ref works' ' @@ -880,7 +880,7 @@ test_expect_success 'stdin -z fails option with unknown name' ' test_expect_success 'stdin -z fails with duplicate refs' ' printf $F "create $a" "$m" "create $b" "$m" "create $a" "$m" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: Multiple updates for ref '"'"'$a'"'"' not allowed." err + grep "fatal: multiple updates for ref '"'"'$a'"'"' not allowed." err ' test_expect_success 'stdin -z create ref works' ' -- cgit v1.2.1 From c52ce248d63a185eb0a616b361d1fd72c5c66451 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sun, 24 Apr 2016 09:48:26 +0200 Subject: ref_transaction_create(): disallow recursive pruning It is nonsensical (and a little bit dangerous) to use REF_ISPRUNING without REF_NODEREF. Forbid it explicitly. Change the one REF_ISPRUNING caller to pass REF_NODEREF too. Signed-off-by: Michael Haggerty --- refs.c | 3 +++ refs/files-backend.c | 2 +- refs/refs-internal.h | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/refs.c b/refs.c index ba1410595..5dc2473fb 100644 --- a/refs.c +++ b/refs.c @@ -790,6 +790,9 @@ int ref_transaction_update(struct ref_transaction *transaction, if (transaction->state != REF_TRANSACTION_OPEN) die("BUG: update called for transaction that is not open"); + if ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF)) + die("BUG: REF_ISPRUNING set without REF_NODEREF"); + if (new_sha1 && !is_null_sha1(new_sha1) && check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { strbuf_addf(err, "refusing to update ref with bad name '%s'", diff --git a/refs/files-backend.c b/refs/files-backend.c index c978fe49c..35d37ce58 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2087,7 +2087,7 @@ static void prune_ref(struct ref_to_prune *r) transaction = ref_transaction_begin(&err); if (!transaction || ref_transaction_delete(transaction, r->name, r->sha1, - REF_ISPRUNING, NULL, &err) || + REF_ISPRUNING | REF_NODEREF, NULL, &err) || ref_transaction_commit(transaction, &err)) { ref_transaction_free(transaction); error("%s", err.buf); diff --git a/refs/refs-internal.h b/refs/refs-internal.h index de7722e32..1f94f7a26 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -15,7 +15,7 @@ /* * Used as a flag in ref_update::flags when a loose ref is being - * pruned. + * pruned. This flag must only be used when REF_NODEREF is set. */ #define REF_ISPRUNING 0x04 -- cgit v1.2.1 From 5a563d4ad17a66aabeacfd0f221ac45c07bc4ee8 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 25 Apr 2016 11:58:23 +0200 Subject: ref_transaction_commit(): correctly report close_ref() failure Signed-off-by: Michael Haggerty --- refs/files-backend.c | 1 + 1 file changed, 1 insertion(+) diff --git a/refs/files-backend.c b/refs/files-backend.c index 35d37ce58..85e1e1c75 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -3139,6 +3139,7 @@ int ref_transaction_commit(struct ref_transaction *transaction, if (close_ref(update->lock)) { strbuf_addf(err, "couldn't close '%s.lock'", update->refname); + ret = TRANSACTION_GENERIC_ERROR; goto cleanup; } } -- cgit v1.2.1 From 8bb0455367a17bd7428e02f835e3f55c8cd168da Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 25 Apr 2016 10:42:19 +0200 Subject: delete_branches(): use resolve_refdup() The return value of resolve_ref_unsafe() is not guaranteed to stay around as long as we need it, so use resolve_refdup() instead. Signed-off-by: Michael Haggerty --- builtin/branch.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index 0adba629d..ae5568845 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -212,7 +212,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, die(_("Couldn't look up commit object for HEAD")); } for (i = 0; i < argc; i++, strbuf_release(&bname)) { - const char *target; + char *target = NULL; int flags = 0; strbuf_branchname(&bname, argv[i]); @@ -231,11 +231,11 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, } } - target = resolve_ref_unsafe(name, - RESOLVE_REF_READING - | RESOLVE_REF_NO_RECURSE - | RESOLVE_REF_ALLOW_BAD_NAME, - sha1, &flags); + target = resolve_refdup(name, + RESOLVE_REF_READING + | RESOLVE_REF_NO_RECURSE + | RESOLVE_REF_ALLOW_BAD_NAME, + sha1, &flags); if (!target) { error(remote_branch ? _("remote-tracking branch '%s' not found.") @@ -248,7 +248,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, check_branch_commit(bname.buf, name, sha1, head_rev, kinds, force)) { ret = 1; - continue; + goto next; } if (delete_ref(name, is_null_sha1(sha1) ? NULL : sha1, @@ -258,7 +258,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, : _("Error deleting branch '%s'"), bname.buf); ret = 1; - continue; + goto next; } if (!quiet) { printf(remote_branch @@ -270,6 +270,9 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, : find_unique_abbrev(sha1, DEFAULT_ABBREV)); } delete_branch_config(bname.buf); + + next: + free(target); } free(name); -- cgit v1.2.1 From d99aa884dff33d48d5aab8c9cf989a25c779fd70 Mon Sep 17 00:00:00 2001 From: David Turner Date: Wed, 24 Feb 2016 17:58:50 -0500 Subject: refs: allow log-only updates The refs infrastructure learns about log-only ref updates, which only update the reflog. Later, we will use this to separate symbolic reference resolution from ref updating. Signed-off-by: David Turner Signed-off-by: Junio C Hamano Signed-off-by: Michael Haggerty --- refs/files-backend.c | 16 ++++++++++------ refs/refs-internal.h | 7 +++++++ 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 85e1e1c75..2f98eebe9 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2683,7 +2683,7 @@ static int commit_ref_update(struct ref_lock *lock, } } } - if (commit_ref(lock)) { + if (!(flags & REF_LOG_ONLY) && commit_ref(lock)) { strbuf_addf(err, "couldn't set '%s'", lock->ref_name); unlock_ref(lock); return -1; @@ -3101,7 +3101,8 @@ int ref_transaction_commit(struct ref_transaction *transaction, goto cleanup; } if ((update->flags & REF_HAVE_NEW) && - !(update->flags & REF_DELETING)) { + !(update->flags & REF_DELETING) && + !(update->flags & REF_LOG_ONLY)) { int overwriting_symref = ((update->type & REF_ISSYMREF) && (update->flags & REF_NODEREF)); @@ -3133,8 +3134,9 @@ int ref_transaction_commit(struct ref_transaction *transaction, } if (!(update->flags & REF_NEEDS_COMMIT)) { /* - * We didn't have to write anything to the lockfile. - * Close it to free up the file descriptor: + * We didn't call write_ref_to_lockfile(), so + * the lockfile is still open. Close it to + * free up the file descriptor: */ if (close_ref(update->lock)) { strbuf_addf(err, "couldn't close '%s.lock'", @@ -3149,7 +3151,8 @@ int ref_transaction_commit(struct ref_transaction *transaction, for (i = 0; i < transaction->nr; i++) { struct ref_update *update = transaction->updates[i]; - if (update->flags & REF_NEEDS_COMMIT) { + if (update->flags & REF_NEEDS_COMMIT || + update->flags & REF_LOG_ONLY) { if (commit_ref_update(update->lock, update->new_sha1, update->msg, update->flags, err)) { @@ -3168,7 +3171,8 @@ int ref_transaction_commit(struct ref_transaction *transaction, for (i = 0; i < transaction->nr; i++) { struct ref_update *update = transaction->updates[i]; - if (update->flags & REF_DELETING) { + if (update->flags & REF_DELETING && + !(update->flags & REF_LOG_ONLY)) { if (delete_ref_loose(update->lock, update->type, err)) { ret = TRANSACTION_GENERIC_ERROR; goto cleanup; diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 1f94f7a26..85f4650ab 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -42,6 +42,13 @@ * value to ref_update::flags */ +/* + * Used as a flag in ref_update::flags when we want to log a ref + * update but not actually perform it. This is used when a symbolic + * ref update is split up. + */ +#define REF_LOG_ONLY 0x80 + /* * Return true iff refname is minimally safe. "Safe" here means that * deleting a loose reference by this name will not do any damage, for -- cgit v1.2.1 From 12fd3496d19c33c6401c5fdc7558944d46124a0f Mon Sep 17 00:00:00 2001 From: David Turner Date: Wed, 24 Feb 2016 17:58:51 -0500 Subject: refs: don't dereference on rename When renaming refs, don't dereference either the origin or the destination before renaming. The origin does not need to be dereferenced because it is presently forbidden to rename symbolic refs. Not dereferencing the destination fixes a bug where renaming on top of a broken symref would use the pointed-to ref name for the moved reflog. Add a test for the reflog bug. Signed-off-by: David Turner Signed-off-by: Junio C Hamano Signed-off-by: Michael Haggerty --- refs/files-backend.c | 21 ++++++++++++++++----- t/t3200-branch.sh | 9 +++++++++ 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 2f98eebe9..15ba456d6 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2333,7 +2333,8 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms if (log && S_ISLNK(loginfo.st_mode)) return error("reflog for %s is a symlink", oldrefname); - if (!resolve_ref_unsafe(oldrefname, RESOLVE_REF_READING, orig_sha1, &flag)) + if (!resolve_ref_unsafe(oldrefname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, + orig_sha1, &flag)) return error("refname %s not found", oldrefname); if (flag & REF_ISSYMREF) @@ -2351,8 +2352,16 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms goto rollback; } - if (!read_ref_full(newrefname, RESOLVE_REF_READING, sha1, NULL) && - delete_ref(newrefname, sha1, REF_NODEREF)) { + /* + * Since we are doing a shallow lookup, sha1 is not the + * correct value to pass to delete_ref as old_sha1. But that + * doesn't matter, because an old_sha1 check wouldn't add to + * the safety anyway; we want to delete the reference whatever + * its current value. + */ + if (!read_ref_full(newrefname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, + sha1, NULL) && + delete_ref(newrefname, NULL, REF_NODEREF)) { if (errno==EISDIR) { struct strbuf path = STRBUF_INIT; int result; @@ -2376,7 +2385,8 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms logmoved = log; - lock = lock_ref_sha1_basic(newrefname, NULL, NULL, NULL, 0, NULL, &err); + lock = lock_ref_sha1_basic(newrefname, NULL, NULL, NULL, REF_NODEREF, + NULL, &err); if (!lock) { error("unable to rename '%s' to '%s': %s", oldrefname, newrefname, err.buf); strbuf_release(&err); @@ -2394,7 +2404,8 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms return 0; rollback: - lock = lock_ref_sha1_basic(oldrefname, NULL, NULL, NULL, 0, NULL, &err); + lock = lock_ref_sha1_basic(oldrefname, NULL, NULL, NULL, REF_NODEREF, + NULL, &err); if (!lock) { error("unable to lock %s for rollback: %s", oldrefname, err.buf); strbuf_release(&err); diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index f3e3b6cf2..42811604b 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -79,6 +79,15 @@ test_expect_success 'git branch -m dumps usage' ' test_i18ngrep "branch name required" err ' +test_expect_success 'git branch -m m broken_symref should work' ' + test_when_finished "git branch -D broken_symref" && + git branch -l m && + git symbolic-ref refs/heads/broken_symref refs/heads/i_am_broken && + git branch -m m broken_symref && + git reflog exists refs/heads/broken_symref && + test_must_fail git reflog exists refs/heads/i_am_broken +' + test_expect_success 'git branch -m m m/m should work' ' git branch -l m && git branch -m m m/m && -- cgit v1.2.1 From 3a8af7be8f977cbf393dc77884a9ee6dfd611d95 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 25 Apr 2016 11:20:08 +0200 Subject: verify_refname_available(): adjust constness in declaration The two string_list arguments can be const. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 4 ++-- refs/refs-internal.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 15ba456d6..1d1b72d57 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2299,8 +2299,8 @@ out: } int verify_refname_available(const char *newname, - struct string_list *extras, - struct string_list *skip, + const struct string_list *extras, + const struct string_list *skip, struct strbuf *err) { struct ref_dir *packed_refs = get_packed_refs(&ref_cache); diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 85f4650ab..9686e6094 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -116,8 +116,8 @@ enum peel_status peel_object(const unsigned char *name, unsigned char *sha1); * extras and skip must be sorted. */ int verify_refname_available(const char *newname, - struct string_list *extras, - struct string_list *skip, + const struct string_list *extras, + const struct string_list *skip, struct strbuf *err); /* -- cgit v1.2.1 From 71564516deccafba0a58129bd7d3851e28fdb4bb Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 25 Apr 2016 11:39:54 +0200 Subject: add_update(): initialize the whole ref_update Change add_update() to initialize all of the fields in the new ref_update object. Rename the function to ref_transaction_add_update(), and increase its visibility to all of the refs-related code. All of this makes the function more useful for other future callers. Signed-off-by: Michael Haggerty --- refs.c | 48 ++++++++++++++++++++++++++---------------------- refs/refs-internal.h | 14 ++++++++++++++ 2 files changed, 40 insertions(+), 22 deletions(-) diff --git a/refs.c b/refs.c index 5dc2473fb..7c4eeb1c9 100644 --- a/refs.c +++ b/refs.c @@ -766,13 +766,33 @@ void ref_transaction_free(struct ref_transaction *transaction) free(transaction); } -static struct ref_update *add_update(struct ref_transaction *transaction, - const char *refname) +struct ref_update *ref_transaction_add_update( + struct ref_transaction *transaction, + const char *refname, unsigned int flags, + const unsigned char *new_sha1, + const unsigned char *old_sha1, + const char *msg) { struct ref_update *update; + + if (transaction->state != REF_TRANSACTION_OPEN) + die("BUG: update called for transaction that is not open"); + + if ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF)) + die("BUG: REF_ISPRUNING set without REF_NODEREF"); + FLEX_ALLOC_STR(update, refname, refname); ALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc); transaction->updates[transaction->nr++] = update; + + update->flags = flags; + + if (flags & REF_HAVE_NEW) + hashcpy(update->new_sha1, new_sha1); + if (flags & REF_HAVE_OLD) + hashcpy(update->old_sha1, old_sha1); + if (msg) + update->msg = xstrdup(msg); return update; } @@ -783,16 +803,8 @@ int ref_transaction_update(struct ref_transaction *transaction, unsigned int flags, const char *msg, struct strbuf *err) { - struct ref_update *update; - assert(err); - if (transaction->state != REF_TRANSACTION_OPEN) - die("BUG: update called for transaction that is not open"); - - if ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF)) - die("BUG: REF_ISPRUNING set without REF_NODEREF"); - if (new_sha1 && !is_null_sha1(new_sha1) && check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { strbuf_addf(err, "refusing to update ref with bad name '%s'", @@ -800,18 +812,10 @@ int ref_transaction_update(struct ref_transaction *transaction, return -1; } - update = add_update(transaction, refname); - if (new_sha1) { - hashcpy(update->new_sha1, new_sha1); - flags |= REF_HAVE_NEW; - } - if (old_sha1) { - hashcpy(update->old_sha1, old_sha1); - flags |= REF_HAVE_OLD; - } - update->flags = flags; - if (msg) - update->msg = xstrdup(msg); + flags |= (new_sha1 ? REF_HAVE_NEW : 0) | (old_sha1 ? REF_HAVE_OLD : 0); + + ref_transaction_add_update(transaction, refname, flags, + new_sha1, old_sha1, msg); return 0; } diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 9686e6094..babdf2769 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -157,6 +157,20 @@ struct ref_update { const char refname[FLEX_ARRAY]; }; +/* + * Add a ref_update with the specified properties to transaction, and + * return a pointer to the new object. This function does not verify + * that refname is well-formed. new_sha1 and old_sha1 are only + * dereferenced if the REF_HAVE_NEW and REF_HAVE_OLD bits, + * respectively, are set in flags. + */ +struct ref_update *ref_transaction_add_update( + struct ref_transaction *transaction, + const char *refname, unsigned int flags, + const unsigned char *new_sha1, + const unsigned char *old_sha1, + const char *msg); + /* * Transaction states. * OPEN: The transaction is in a valid state and can accept new updates. -- cgit v1.2.1 From 165056b2fc065e27e4077a11ed2bf1589207b997 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sun, 24 Apr 2016 08:58:41 +0200 Subject: lock_ref_for_update(): new function Extract a new function, lock_ref_for_update(), from ref_transaction_commit(). Signed-off-by: Michael Haggerty --- refs/files-backend.c | 152 ++++++++++++++++++++++++++++----------------------- 1 file changed, 85 insertions(+), 67 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 1d1b72d57..85751977f 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -3051,6 +3051,88 @@ static int ref_update_reject_duplicates(struct string_list *refnames, return 0; } +/* + * Acquire all locks, verify old values if provided, check + * that new values are valid, and write new values to the + * lockfiles, ready to be activated. Only keep one lockfile + * open at a time to avoid running out of file descriptors. + */ +static int lock_ref_for_update(struct ref_update *update, + struct ref_transaction *transaction, + struct string_list *affected_refnames, + struct strbuf *err) +{ + int ret; + + if ((update->flags & REF_HAVE_NEW) && + is_null_sha1(update->new_sha1)) + update->flags |= REF_DELETING; + update->lock = lock_ref_sha1_basic( + update->refname, + ((update->flags & REF_HAVE_OLD) ? + update->old_sha1 : NULL), + affected_refnames, NULL, + update->flags, + &update->type, + err); + if (!update->lock) { + char *reason; + + ret = (errno == ENOTDIR) + ? TRANSACTION_NAME_CONFLICT + : TRANSACTION_GENERIC_ERROR; + reason = strbuf_detach(err, NULL); + strbuf_addf(err, "cannot lock ref '%s': %s", + update->refname, reason); + free(reason); + return ret; + } + if ((update->flags & REF_HAVE_NEW) && + !(update->flags & REF_DELETING) && + !(update->flags & REF_LOG_ONLY)) { + int overwriting_symref = ((update->type & REF_ISSYMREF) && + (update->flags & REF_NODEREF)); + + if (!overwriting_symref && + !hashcmp(update->lock->old_oid.hash, update->new_sha1)) { + /* + * The reference already has the desired + * value, so we don't need to write it. + */ + } else if (write_ref_to_lockfile(update->lock, + update->new_sha1, + err)) { + char *write_err = strbuf_detach(err, NULL); + + /* + * The lock was freed upon failure of + * write_ref_to_lockfile(): + */ + update->lock = NULL; + strbuf_addf(err, + "cannot update the ref '%s': %s", + update->refname, write_err); + free(write_err); + return TRANSACTION_GENERIC_ERROR; + } else { + update->flags |= REF_NEEDS_COMMIT; + } + } + if (!(update->flags & REF_NEEDS_COMMIT)) { + /* + * We didn't call write_ref_to_lockfile(), so + * the lockfile is still open. Close it to + * free up the file descriptor: + */ + if (close_ref(update->lock)) { + strbuf_addf(err, "couldn't close '%s.lock'", + update->refname); + return TRANSACTION_GENERIC_ERROR; + } + } + return 0; +} + int ref_transaction_commit(struct ref_transaction *transaction, struct strbuf *err) { @@ -3088,74 +3170,10 @@ int ref_transaction_commit(struct ref_transaction *transaction, for (i = 0; i < transaction->nr; i++) { struct ref_update *update = transaction->updates[i]; - if ((update->flags & REF_HAVE_NEW) && - is_null_sha1(update->new_sha1)) - update->flags |= REF_DELETING; - update->lock = lock_ref_sha1_basic( - update->refname, - ((update->flags & REF_HAVE_OLD) ? - update->old_sha1 : NULL), - &affected_refnames, NULL, - update->flags, - &update->type, - err); - if (!update->lock) { - char *reason; - - ret = (errno == ENOTDIR) - ? TRANSACTION_NAME_CONFLICT - : TRANSACTION_GENERIC_ERROR; - reason = strbuf_detach(err, NULL); - strbuf_addf(err, "cannot lock ref '%s': %s", - update->refname, reason); - free(reason); + ret = lock_ref_for_update(update, transaction, + &affected_refnames, err); + if (ret) goto cleanup; - } - if ((update->flags & REF_HAVE_NEW) && - !(update->flags & REF_DELETING) && - !(update->flags & REF_LOG_ONLY)) { - int overwriting_symref = ((update->type & REF_ISSYMREF) && - (update->flags & REF_NODEREF)); - - if (!overwriting_symref && - !hashcmp(update->lock->old_oid.hash, update->new_sha1)) { - /* - * The reference already has the desired - * value, so we don't need to write it. - */ - } else if (write_ref_to_lockfile(update->lock, - update->new_sha1, - err)) { - char *write_err = strbuf_detach(err, NULL); - - /* - * The lock was freed upon failure of - * write_ref_to_lockfile(): - */ - update->lock = NULL; - strbuf_addf(err, - "cannot update the ref '%s': %s", - update->refname, write_err); - free(write_err); - ret = TRANSACTION_GENERIC_ERROR; - goto cleanup; - } else { - update->flags |= REF_NEEDS_COMMIT; - } - } - if (!(update->flags & REF_NEEDS_COMMIT)) { - /* - * We didn't call write_ref_to_lockfile(), so - * the lockfile is still open. Close it to - * free up the file descriptor: - */ - if (close_ref(update->lock)) { - strbuf_addf(err, "couldn't close '%s.lock'", - update->refname); - ret = TRANSACTION_GENERIC_ERROR; - goto cleanup; - } - } } /* Perform updates first so live commits remain referenced */ -- cgit v1.2.1 From 8415d24746b97a479fe5aec9845bfc150cda2d14 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sun, 24 Apr 2016 08:11:37 +0200 Subject: unlock_ref(): move definition higher in the file This avoids the need for a forward declaration in the next patch. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 85751977f..dc0bde05b 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1516,6 +1516,16 @@ out: return ret; } +static void unlock_ref(struct ref_lock *lock) +{ + /* Do not free lock->lk -- atexit() still looks at them */ + if (lock->lk) + rollback_lock_file(lock->lk); + free(lock->ref_name); + free(lock->orig_ref_name); + free(lock); +} + /* * Peel the entry (if possible) and return its new peel_status. If * repeel is true, re-peel the entry even if there is an old peeled @@ -1674,16 +1684,6 @@ int do_for_each_ref(const char *submodule, const char *base, return do_for_each_entry(refs, base, do_one_ref, &data); } -static void unlock_ref(struct ref_lock *lock) -{ - /* Do not free lock->lk -- atexit() still looks at them */ - if (lock->lk) - rollback_lock_file(lock->lk); - free(lock->ref_name); - free(lock->orig_ref_name); - free(lock); -} - /* * Verify that the reference locked by lock has the value old_sha1. * Fail if the reference doesn't exist and mustexist is set. Return 0 -- cgit v1.2.1 From 8a679de6f1a4bd077f828273f75eea46947b5b73 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 27 Apr 2016 15:54:45 +0200 Subject: ref_transaction_update(): check refname_is_safe() at a minimum If the user has asked that a new value be set for a reference, we use check_refname_format() to verify that the reference name satisfies all of the rules. But in other cases, at least check that refname_is_safe(). Signed-off-by: Michael Haggerty --- refs.c | 5 +++-- t/t1400-update-ref.sh | 2 +- t/t1430-bad-ref-name.sh | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/refs.c b/refs.c index 7c4eeb1c9..842c5c7b0 100644 --- a/refs.c +++ b/refs.c @@ -805,8 +805,9 @@ int ref_transaction_update(struct ref_transaction *transaction, { assert(err); - if (new_sha1 && !is_null_sha1(new_sha1) && - check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { + if ((new_sha1 && !is_null_sha1(new_sha1)) ? + check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) : + !refname_is_safe(refname)) { strbuf_addf(err, "refusing to update ref with bad name '%s'", refname); return -1; diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 40b0ccedf..08bd8fd8d 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -23,7 +23,7 @@ test_expect_success setup ' m=refs/heads/master n_dir=refs/heads/gu n=$n_dir/fixes -outside=foo +outside=refs/foo test_expect_success \ "create $m" \ diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh index 25ddab4e9..8937e25e4 100755 --- a/t/t1430-bad-ref-name.sh +++ b/t/t1430-bad-ref-name.sh @@ -285,7 +285,7 @@ test_expect_success 'update-ref -d cannot delete non-ref in .git dir' ' echo precious >expect && test_must_fail git update-ref -d my-private-file >output 2>error && test_must_be_empty output && - test_i18ngrep -e "cannot lock .*: unable to resolve reference" error && + test_i18ngrep -e "refusing to update ref with bad name" error && test_cmp expect .git/my-private-file ' -- cgit v1.2.1 From 92b1551b1d407065f961ffd1d972481063a0edcc Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 25 Apr 2016 15:56:07 +0200 Subject: refs: resolve symbolic refs first Before committing ref updates, split symbolic ref updates into two parts: an update to the underlying ref, and a log-only update to the symbolic ref. This ensures that both references are locked correctly during the transaction, including while their reflogs are updated. Similarly, if the reference pointed to by HEAD is modified directly, add a separate log-only update to HEAD, rather than leaving the job of updating HEAD's reflog to commit_ref_update(). This change ensures that HEAD is locked correctly while its reflog is being modified, as well as being cheaper (HEAD only needs to be resolved once). This makes use of a new function, lock_raw_ref(), which is analogous to read_raw_ref(), but acquires a lock on the reference before reading it. This change still has two problems: * There are redundant read_ref_full() reference lookups. * It is still possible to get incorrect reflogs for symbolic references if there is a concurrent update by another process, since the old_oid of a symref is determined before the lock on the pointed-to ref is held. Both problems will soon be fixed. Signed-off-by: David Turner Signed-off-by: Junio C Hamano Signed-off-by: Michael Haggerty WIP --- refs/files-backend.c | 508 ++++++++++++++++++++++++++++++++++++++++++++++---- refs/refs-internal.h | 11 +- t/t1400-update-ref.sh | 35 ++++ 3 files changed, 514 insertions(+), 40 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index dc0bde05b..ffc30fd23 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1526,6 +1526,233 @@ static void unlock_ref(struct ref_lock *lock) free(lock); } +/* + * Lock refname, without following symrefs, and set *lock_p to point + * at a newly-allocated lock object. Fill in lock->old_oid, referent, + * and type similarly to read_raw_ref(). + * + * The caller must verify that refname is a "safe" reference name (in + * the sense of refname_is_safe()) before calling this function. + * + * If the reference doesn't already exist, verify that refname doesn't + * have a D/F conflict with any existing references. extras and skip + * are passed to verify_refname_available_dir() for this check. + * + * If mustexist is not set and the reference is not found or is + * broken, lock the reference anyway but clear sha1. + * + * Return 0 on success. On failure, write an error message to err and + * return TRANSACTION_NAME_CONFLICT or TRANSACTION_GENERIC_ERROR. + * + * Implementation note: This function is basically + * + * lock reference + * read_raw_ref() + * + * but it includes a lot more code to + * - Deal with possible races with other processes + * - Avoid calling verify_refname_available_dir() when it can be + * avoided, namely if we were successfully able to read the ref + * - Generate informative error messages in the case of failure + */ +static int lock_raw_ref(const char *refname, int mustexist, + const struct string_list *extras, + const struct string_list *skip, + struct ref_lock **lock_p, + struct strbuf *referent, + unsigned int *type, + struct strbuf *err) +{ + struct ref_lock *lock; + struct strbuf ref_file = STRBUF_INIT; + int attempts_remaining = 3; + int ret = TRANSACTION_GENERIC_ERROR; + + assert(err); + *type = 0; + + /* First lock the file so it can't change out from under us. */ + + *lock_p = lock = xcalloc(1, sizeof(*lock)); + + lock->ref_name = xstrdup(refname); + lock->orig_ref_name = xstrdup(refname); + strbuf_git_path(&ref_file, "%s", refname); + +retry: + switch (safe_create_leading_directories(ref_file.buf)) { + case SCLD_OK: + break; /* success */ + case SCLD_EXISTS: + /* + * Suppose refname is "refs/foo/bar". We just failed + * to create the containing directory, "refs/foo", + * because there was a non-directory in the way. This + * indicates a D/F conflict, probably because of + * another reference such as "refs/foo". There is no + * reason to expect this error to be transitory. + */ + if (verify_refname_available(refname, extras, skip, err)) { + if (mustexist) { + /* + * To the user the relevant error is + * that the "mustexist" reference is + * missing: + */ + strbuf_reset(err); + strbuf_addf(err, "unable to resolve reference '%s'", + refname); + } else { + /* + * The error message set by + * verify_refname_available_dir() is OK. + */ + ret = TRANSACTION_NAME_CONFLICT; + } + } else { + /* + * The file that is in the way isn't a loose + * reference. Report it as a low-level + * failure. + */ + strbuf_addf(err, "unable to create lock file %s.lock; " + "non-directory in the way", + ref_file.buf); + } + goto error_return; + case SCLD_VANISHED: + /* Maybe another process was tidying up. Try again. */ + if (--attempts_remaining > 0) + goto retry; + /* fall through */ + default: + strbuf_addf(err, "unable to create directory for %s", + ref_file.buf); + goto error_return; + } + + if (!lock->lk) + lock->lk = xcalloc(1, sizeof(struct lock_file)); + + if (hold_lock_file_for_update(lock->lk, ref_file.buf, LOCK_NO_DEREF) < 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_message(ref_file.buf, errno, err); + goto error_return; + } + } + + /* + * Now we hold the lock and can read the reference without + * fear that its value will change. + */ + + if (read_raw_ref(refname, lock->old_oid.hash, referent, type)) { + if (errno == ENOENT) { + if (mustexist) { + /* Garden variety missing reference. */ + strbuf_addf(err, "unable to resolve reference '%s'", + refname); + goto error_return; + } else { + /* + * Reference is missing, but that's OK. We + * know that there is not a conflict with + * another loose reference because + * (supposing that we are trying to lock + * reference "refs/foo/bar"): + * + * - We were successfully able to create + * the lockfile refs/foo/bar.lock, so we + * know there cannot be a loose reference + * named "refs/foo". + * + * - We got ENOENT and not EISDIR, so we + * know that there cannot be a loose + * reference named "refs/foo/bar/baz". + */ + } + } else if (errno == EISDIR) { + /* + * There is a directory in the way. It might have + * contained references that have been deleted. If + * we don't require that the reference already + * exists, try to remove the directory so that it + * doesn't cause trouble when we want to rename the + * lockfile into place later. + */ + if (mustexist) { + /* Garden variety missing reference. */ + strbuf_addf(err, "unable to resolve reference '%s'", + refname); + goto error_return; + } else if (remove_dir_recursively(&ref_file, + REMOVE_DIR_EMPTY_ONLY)) { + if (verify_refname_available_dir( + refname, extras, skip, + get_loose_refs(&ref_cache), + err)) { + /* + * The error message set by + * verify_refname_available() is OK. + */ + ret = TRANSACTION_NAME_CONFLICT; + goto error_return; + } else { + /* + * We can't delete the directory, + * but we also don't know of any + * references that it should + * contain. + */ + strbuf_addf(err, "there is a non-empty directory '%s' " + "blocking reference '%s'", + ref_file.buf, refname); + goto error_return; + } + } + } else if (errno == EINVAL && (*type & REF_ISBROKEN)) { + strbuf_addf(err, "unable to resolve reference '%s': " + "reference broken", refname); + goto error_return; + } else { + strbuf_addf(err, "unable to resolve reference '%s': %s", + refname, strerror(errno)); + goto error_return; + } + + /* + * If the ref did not exist and we are creating it, + * make sure there is no existing packed ref whose + * name begins with our refname, nor a packed ref + * whose name is a proper prefix of our refname. + */ + if (verify_refname_available_dir( + refname, extras, skip, + get_packed_refs(&ref_cache), + err)) { + goto error_return; + } + } + + ret = 0; + goto out; + +error_return: + unlock_ref(lock); + *lock_p = NULL; + +out: + strbuf_release(&ref_file); + return ret; +} + /* * Peel the entry (if possible) and return its new peel_status. If * repeel is true, re-peel the entry even if there is an old peeled @@ -3052,55 +3279,202 @@ static int ref_update_reject_duplicates(struct string_list *refnames, } /* - * Acquire all locks, verify old values if provided, check - * that new values are valid, and write new values to the - * lockfiles, ready to be activated. Only keep one lockfile - * open at a time to avoid running out of file descriptors. + * If update is a direct update of head_ref (the reference pointed to + * by HEAD), then add an extra REF_LOG_ONLY update for HEAD. + */ +static int split_head_update(struct ref_update *update, + struct ref_transaction *transaction, + const char *head_ref, + struct string_list *affected_refnames, + struct strbuf *err) +{ + struct string_list_item *item; + struct ref_update *new_update; + + if ((update->flags & REF_LOG_ONLY) || + (update->flags & REF_ISPRUNING) || + (update->flags & REF_UPDATE_VIA_HEAD)) + return 0; + + if (strcmp(update->refname, head_ref)) + return 0; + + /* + * First make sure that HEAD is not already in the + * transaction. This insertion is O(N) in the transaction + * size, but it happens at most once per transaction. + */ + item = string_list_insert(affected_refnames, "HEAD"); + if (item->util) { + /* An entry already existed */ + strbuf_addf(err, + "multiple updates for 'HEAD' (including one " + "via its referent '%s') are not allowed", + update->refname); + return TRANSACTION_NAME_CONFLICT; + } + + new_update = ref_transaction_add_update( + transaction, "HEAD", + update->flags | REF_LOG_ONLY | REF_NODEREF, + update->new_sha1, update->old_sha1, + update->msg); + + item->util = new_update; + + return 0; +} + +/* + * update is for a symref that points at referent and doesn't have + * REF_NODEREF set. Split it into two updates: + * - The original update, but with REF_LOG_ONLY and REF_NODEREF set + * - A new, separate update for the referent reference + * Note that the new update will itself be subject to splitting when + * the iteration gets to it. + */ +static int split_symref_update(struct ref_update *update, + const char *referent, + struct ref_transaction *transaction, + struct string_list *affected_refnames, + struct strbuf *err) +{ + struct string_list_item *item; + struct ref_update *new_update; + unsigned int new_flags; + + /* + * First make sure that referent is not already in the + * transaction. This insertion is O(N) in the transaction + * size, but it happens at most once per symref in a + * transaction. + */ + item = string_list_insert(affected_refnames, referent); + if (item->util) { + /* An entry already existed */ + strbuf_addf(err, + "multiple updates for '%s' (including one " + "via symref '%s') are not allowed", + referent, update->refname); + return TRANSACTION_NAME_CONFLICT; + } + + new_flags = update->flags; + if (!strcmp(update->refname, "HEAD")) { + /* + * Record that the new update came via HEAD, so that + * when we process it, split_head_update() doesn't try + * to add another reflog update for HEAD. Note that + * this bit will be propagated if the new_update + * itself needs to be split. + */ + new_flags |= REF_UPDATE_VIA_HEAD; + } + + new_update = ref_transaction_add_update( + transaction, referent, new_flags, + update->new_sha1, update->old_sha1, + update->msg); + + /* Change the symbolic ref update to log only: */ + update->flags |= REF_LOG_ONLY | REF_NODEREF; + + item->util = new_update; + + return 0; +} + +/* + * Prepare for carrying out update: + * - Lock the reference referred to by update. + * - Read the reference under lock. + * - Check that its old SHA-1 value (if specified) is correct, and in + * any case record it in update->lock->old_oid for later use when + * writing the reflog. + * - If it is a symref update without REF_NODEREF, split it up into a + * REF_LOG_ONLY update of the symref and add a separate update for + * the referent to transaction. + * - If it is an update of head_ref, add a corresponding REF_LOG_ONLY + * update of HEAD. */ static int lock_ref_for_update(struct ref_update *update, struct ref_transaction *transaction, + const char *head_ref, struct string_list *affected_refnames, struct strbuf *err) { + struct strbuf referent = STRBUF_INIT; + int mustexist = (update->flags & REF_HAVE_OLD) && + !is_null_sha1(update->old_sha1); int ret; + struct ref_lock *lock; - if ((update->flags & REF_HAVE_NEW) && - is_null_sha1(update->new_sha1)) + if ((update->flags & REF_HAVE_NEW) && is_null_sha1(update->new_sha1)) update->flags |= REF_DELETING; - update->lock = lock_ref_sha1_basic( - update->refname, - ((update->flags & REF_HAVE_OLD) ? - update->old_sha1 : NULL), - affected_refnames, NULL, - update->flags, - &update->type, - err); - if (!update->lock) { + + if (head_ref) { + ret = split_head_update(update, transaction, head_ref, + affected_refnames, err); + if (ret) + return ret; + } + + ret = lock_raw_ref(update->refname, mustexist, + affected_refnames, NULL, + &update->lock, &referent, + &update->type, err); + + if (ret) { char *reason; - ret = (errno == ENOTDIR) - ? TRANSACTION_NAME_CONFLICT - : TRANSACTION_GENERIC_ERROR; reason = strbuf_detach(err, NULL); strbuf_addf(err, "cannot lock ref '%s': %s", update->refname, reason); free(reason); return ret; } + + lock = update->lock; + + if (read_ref_full(update->refname, + mustexist ? RESOLVE_REF_READING : 0, + lock->old_oid.hash, NULL)) { + if (update->flags & REF_HAVE_OLD) { + strbuf_addf(err, "cannot lock ref '%s': can't resolve old value", + update->refname); + return TRANSACTION_GENERIC_ERROR; + } else { + hashclr(lock->old_oid.hash); + } + } + if ((update->flags & REF_HAVE_OLD) && + hashcmp(lock->old_oid.hash, update->old_sha1)) { + strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s", + update->refname, + sha1_to_hex(lock->old_oid.hash), + sha1_to_hex(update->old_sha1)); + return TRANSACTION_GENERIC_ERROR; + } + + if (update->type & REF_ISSYMREF) { + if (!(update->flags & REF_NODEREF)) { + ret = split_symref_update(update, referent.buf, transaction, + affected_refnames, err); + if (ret) + return ret; + } + } + if ((update->flags & REF_HAVE_NEW) && !(update->flags & REF_DELETING) && !(update->flags & REF_LOG_ONLY)) { - int overwriting_symref = ((update->type & REF_ISSYMREF) && - (update->flags & REF_NODEREF)); - - if (!overwriting_symref && - !hashcmp(update->lock->old_oid.hash, update->new_sha1)) { + if (!(update->type & REF_ISSYMREF) && + !hashcmp(lock->old_oid.hash, update->new_sha1)) { /* * The reference already has the desired * value, so we don't need to write it. */ - } else if (write_ref_to_lockfile(update->lock, - update->new_sha1, + } else if (write_ref_to_lockfile(lock, update->new_sha1, err)) { char *write_err = strbuf_detach(err, NULL); @@ -3124,7 +3498,7 @@ static int lock_ref_for_update(struct ref_update *update, * the lockfile is still open. Close it to * free up the file descriptor: */ - if (close_ref(update->lock)) { + if (close_ref(lock)) { strbuf_addf(err, "couldn't close '%s.lock'", update->refname); return TRANSACTION_GENERIC_ERROR; @@ -3140,6 +3514,9 @@ int ref_transaction_commit(struct ref_transaction *transaction, struct string_list refs_to_delete = STRING_LIST_INIT_NODUP; struct string_list_item *ref_to_delete; struct string_list affected_refnames = STRING_LIST_INIT_NODUP; + char *head_ref = NULL; + int head_type; + struct object_id head_oid; assert(err); @@ -3151,16 +3528,57 @@ int ref_transaction_commit(struct ref_transaction *transaction, return 0; } - /* Fail if a refname appears more than once in the transaction: */ - for (i = 0; i < transaction->nr; i++) - string_list_append(&affected_refnames, - transaction->updates[i]->refname); + /* + * Fail if a refname appears more than once in the + * transaction. (If we end up splitting up any updates using + * split_symref_update() or split_head_update(), those + * functions will check that the new updates don't have the + * same refname as any existing ones.) + */ + for (i = 0; i < transaction->nr; i++) { + struct ref_update *update = transaction->updates[i]; + struct string_list_item *item = + string_list_append(&affected_refnames, update->refname); + + /* + * We store a pointer to update in item->util, but at + * the moment we never use the value of this field + * except to check whether it is non-NULL. + */ + item->util = update; + } string_list_sort(&affected_refnames); if (ref_update_reject_duplicates(&affected_refnames, err)) { ret = TRANSACTION_GENERIC_ERROR; goto cleanup; } + /* + * Special hack: If a branch is updated directly and HEAD + * points to it (may happen on the remote side of a push + * for example) then logically the HEAD reflog should be + * updated too. + * + * A generic solution would require reverse symref lookups, + * but finding all symrefs pointing to a given branch would be + * rather costly for this rare event (the direct update of a + * branch) to be worth it. So let's cheat and check with HEAD + * only, which should cover 99% of all usage scenarios (even + * 100% of the default ones). + * + * So if HEAD is a symbolic reference, then record the name of + * the reference that it points to. If we see an update of + * head_ref within the transaction, then split_head_update() + * arranges for the reflog of HEAD to be updated, too. + */ + head_ref = resolve_refdup("HEAD", RESOLVE_REF_NO_RECURSE, + head_oid.hash, &head_type); + + if (head_ref && !(head_type & REF_ISSYMREF)) { + free(head_ref); + head_ref = NULL; + } + /* * Acquire all locks, verify old values if provided, check * that new values are valid, and write new values to the @@ -3170,7 +3588,7 @@ int ref_transaction_commit(struct ref_transaction *transaction, for (i = 0; i < transaction->nr; i++) { struct ref_update *update = transaction->updates[i]; - ret = lock_ref_for_update(update, transaction, + ret = lock_ref_for_update(update, transaction, head_ref, &affected_refnames, err); if (ret) goto cleanup; @@ -3179,23 +3597,35 @@ int ref_transaction_commit(struct ref_transaction *transaction, /* Perform updates first so live commits remain referenced */ for (i = 0; i < transaction->nr; i++) { struct ref_update *update = transaction->updates[i]; + struct ref_lock *lock = update->lock; if (update->flags & REF_NEEDS_COMMIT || update->flags & REF_LOG_ONLY) { - if (commit_ref_update(update->lock, - update->new_sha1, update->msg, - update->flags, err)) { - /* freed by commit_ref_update(): */ + if (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", + lock->ref_name, old_msg); + free(old_msg); + unlock_ref(lock); update->lock = NULL; ret = TRANSACTION_GENERIC_ERROR; goto cleanup; - } else { - /* freed by commit_ref_update(): */ + } + } + if (update->flags & REF_NEEDS_COMMIT) { + clear_loose_ref_cache(&ref_cache); + if (commit_ref(lock)) { + strbuf_addf(err, "couldn't set '%s'", lock->ref_name); + unlock_ref(lock); update->lock = NULL; + ret = TRANSACTION_GENERIC_ERROR; + goto cleanup; } } } - /* Perform deletes now that updates are safely completed */ for (i = 0; i < transaction->nr; i++) { struct ref_update *update = transaction->updates[i]; @@ -3228,7 +3658,9 @@ cleanup: if (transaction->updates[i]->lock) unlock_ref(transaction->updates[i]->lock); string_list_clear(&refs_to_delete, 0); + free(head_ref); string_list_clear(&affected_refnames, 0); + return ret; } diff --git a/refs/refs-internal.h b/refs/refs-internal.h index babdf2769..cccd76b28 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -49,6 +49,12 @@ */ #define REF_LOG_ONLY 0x80 +/* + * Internal flag, meaning that the containing ref_update was via an + * update to HEAD. + */ +#define REF_UPDATE_VIA_HEAD 0x100 + /* * Return true iff refname is minimally safe. "Safe" here means that * deleting a loose reference by this name will not do any damage, for @@ -148,11 +154,12 @@ struct ref_update { unsigned char old_sha1[20]; /* * One or more of REF_HAVE_NEW, REF_HAVE_OLD, REF_NODEREF, - * REF_DELETING, and REF_ISPRUNING: + * REF_DELETING, REF_ISPRUNING, REF_LOG_ONLY, and + * REF_UPDATE_VIA_HEAD: */ unsigned int flags; struct ref_lock *lock; - int type; + unsigned int type; char *msg; const char refname[FLEX_ARRAY]; }; diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 08bd8fd8d..d22693041 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -1102,6 +1102,41 @@ test_expect_success 'stdin -z delete refs works with packed and loose refs' ' test_must_fail git rev-parse --verify -q $c ' +test_expect_success 'fails with duplicate HEAD update' ' + git branch target1 $A && + git checkout target1 && + cat >stdin <<-EOF && + update refs/heads/target1 $C + option no-deref + update HEAD $B + EOF + test_must_fail git update-ref --stdin err && + grep "fatal: multiple updates for '\''HEAD'\'' (including one via its referent .refs/heads/target1.) are not allowed" err && + echo "refs/heads/target1" >expect && + git symbolic-ref HEAD >actual && + test_cmp expect actual && + echo "$A" >expect && + git rev-parse refs/heads/target1 >actual && + test_cmp expect actual +' + +test_expect_success 'fails with duplicate ref update via symref' ' + git branch target2 $A && + git symbolic-ref refs/heads/symref2 refs/heads/target2 && + cat >stdin <<-EOF && + update refs/heads/target2 $C + update refs/heads/symref2 $B + EOF + test_must_fail git update-ref --stdin err && + grep "fatal: multiple updates for '\''refs/heads/target2'\'' (including one via symref .refs/heads/symref2.) are not allowed" err && + echo "refs/heads/target2" >expect && + git symbolic-ref refs/heads/symref2 >actual && + test_cmp expect actual && + echo "$A" >expect && + git rev-parse refs/heads/target2 >actual && + test_cmp expect actual +' + run_with_limited_open_files () { (ulimit -n 32 && "$@") } -- cgit v1.2.1 From 8169d0d06ad721aa54d95f044f4b097d79151ea2 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 25 Apr 2016 17:38:35 +0200 Subject: lock_ref_for_update(): don't re-read non-symbolic references Before the previous patch, our first read of the reference happened before the reference was locked, so we couldn't trust its value and had to read it again. But now that our first read of the reference happens after acquiring the lock, there is no need to read it a second time. So move the read_ref_full() call into the (update->type & REF_ISSYMREF) block. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 48 ++++++++++++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 18 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index ffc30fd23..7bc18322a 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -3436,33 +3436,45 @@ static int lock_ref_for_update(struct ref_update *update, lock = update->lock; - if (read_ref_full(update->refname, - mustexist ? RESOLVE_REF_READING : 0, - lock->old_oid.hash, NULL)) { - if (update->flags & REF_HAVE_OLD) { - strbuf_addf(err, "cannot lock ref '%s': can't resolve old value", - update->refname); + if (update->type & REF_ISSYMREF) { + if (read_ref_full(update->refname, + mustexist ? RESOLVE_REF_READING : 0, + lock->old_oid.hash, NULL)) { + if (update->flags & REF_HAVE_OLD) { + strbuf_addf(err, "cannot lock ref '%s': can't resolve old value", + update->refname); + return TRANSACTION_GENERIC_ERROR; + } else { + hashclr(lock->old_oid.hash); + } + } + if ((update->flags & REF_HAVE_OLD) && + hashcmp(lock->old_oid.hash, update->old_sha1)) { + strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s", + update->refname, + sha1_to_hex(lock->old_oid.hash), + sha1_to_hex(update->old_sha1)); return TRANSACTION_GENERIC_ERROR; - } else { - hashclr(lock->old_oid.hash); } - } - if ((update->flags & REF_HAVE_OLD) && - hashcmp(lock->old_oid.hash, update->old_sha1)) { - strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s", - update->refname, - sha1_to_hex(lock->old_oid.hash), - sha1_to_hex(update->old_sha1)); - return TRANSACTION_GENERIC_ERROR; - } - if (update->type & REF_ISSYMREF) { if (!(update->flags & REF_NODEREF)) { ret = split_symref_update(update, referent.buf, transaction, affected_refnames, err); if (ret) return ret; } + } else if ((update->flags & REF_HAVE_OLD) && + hashcmp(lock->old_oid.hash, update->old_sha1)) { + if (is_null_sha1(update->old_sha1)) + strbuf_addf(err, "cannot lock ref '%s': reference already exists", + update->refname); + else + strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s", + update->refname, + sha1_to_hex(lock->old_oid.hash), + sha1_to_hex(update->old_sha1)); + + return TRANSACTION_GENERIC_ERROR; } if ((update->flags & REF_HAVE_NEW) && -- cgit v1.2.1 From 6e30b2f652d0a6748e2041dee5b5612cafca29b2 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 25 Apr 2016 17:48:32 +0200 Subject: lock_ref_for_update(): don't resolve symrefs If a transaction includes a non-NODEREF update to a symbolic reference, we don't have to look it up in lock_ref_for_update(). The reference will be dereferenced anyway when the split-off update is processed. This change requires that we store a backpointer from the split-off update to its parent update, for two reasons: * We still want to report the original reference name in error messages. So if an error occurs when checking the split-off update's old_sha1, walk the parent_update pointers back to find the original reference name, and report that one. * We still need to write the old_sha1 of the symref to its reflog. So after we read the split-off update's reference value, walk the parent_update pointers back and fill in their old_sha1 fields. Aside from eliminating unnecessary reads, this change fixes a subtle (though not very serious) race condition: in the old code, the old_sha1 of the symref was resolved before the reference that it pointed at was locked. So it was possible that the old_sha1 value logged to the symref's reflog could be wrong if another process changed the downstream reference before it was locked. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 108 +++++++++++++++++++++++++++++++++++++-------------- refs/refs-internal.h | 17 ++++++++ 2 files changed, 95 insertions(+), 30 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 7bc18322a..a78351343 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -3376,14 +3376,32 @@ static int split_symref_update(struct ref_update *update, update->new_sha1, update->old_sha1, update->msg); - /* Change the symbolic ref update to log only: */ + new_update->parent_update = update; + + /* + * Change the symbolic ref update to log only. Also, it + * doesn't need to check its old SHA-1 value, as that will be + * done when new_update is processed. + */ update->flags |= REF_LOG_ONLY | REF_NODEREF; + update->flags &= ~REF_HAVE_OLD; item->util = new_update; return 0; } +/* + * Return the refname under which update was originally requested. + */ +static const char *original_update_refname(struct ref_update *update) +{ + while (update->parent_update) + update = update->parent_update; + + return update->refname; +} + /* * Prepare for carrying out update: * - Lock the reference referred to by update. @@ -3437,44 +3455,74 @@ static int lock_ref_for_update(struct ref_update *update, lock = update->lock; if (update->type & REF_ISSYMREF) { - if (read_ref_full(update->refname, - mustexist ? RESOLVE_REF_READING : 0, - lock->old_oid.hash, NULL)) { - if (update->flags & REF_HAVE_OLD) { - strbuf_addf(err, "cannot lock ref '%s': can't resolve old value", - update->refname); + if (update->flags & REF_NODEREF) { + /* + * We won't be reading the referent as part of + * the transaction, so we have to read it here + * to record and possibly check old_sha1: + */ + if (read_ref_full(update->refname, + mustexist ? RESOLVE_REF_READING : 0, + lock->old_oid.hash, NULL)) { + if (update->flags & REF_HAVE_OLD) { + strbuf_addf(err, "cannot lock ref '%s': " + "can't resolve old value", + update->refname); + return TRANSACTION_GENERIC_ERROR; + } else { + hashclr(lock->old_oid.hash); + } + } + if ((update->flags & REF_HAVE_OLD) && + hashcmp(lock->old_oid.hash, update->old_sha1)) { + strbuf_addf(err, "cannot lock ref '%s': " + "is at %s but expected %s", + update->refname, + sha1_to_hex(lock->old_oid.hash), + sha1_to_hex(update->old_sha1)); return TRANSACTION_GENERIC_ERROR; - } else { - hashclr(lock->old_oid.hash); } - } - if ((update->flags & REF_HAVE_OLD) && - hashcmp(lock->old_oid.hash, update->old_sha1)) { - strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s", - update->refname, - sha1_to_hex(lock->old_oid.hash), - sha1_to_hex(update->old_sha1)); - return TRANSACTION_GENERIC_ERROR; - } - if (!(update->flags & REF_NODEREF)) { + } else { + /* + * Create a new update for the reference this + * symref is pointing at. Also, we will record + * and verify old_sha1 for this update as part + * of processing the split-off update, so we + * don't have to do it here. + */ ret = split_symref_update(update, referent.buf, transaction, affected_refnames, err); if (ret) return ret; } - } else if ((update->flags & REF_HAVE_OLD) && - hashcmp(lock->old_oid.hash, update->old_sha1)) { - if (is_null_sha1(update->old_sha1)) - strbuf_addf(err, "cannot lock ref '%s': reference already exists", - update->refname); - else - strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s", - update->refname, - sha1_to_hex(lock->old_oid.hash), - sha1_to_hex(update->old_sha1)); + } else { + struct ref_update *parent_update; + + /* + * If this update is happening indirectly because of a + * symref update, record the old SHA-1 in the parent + * update: + */ + for (parent_update = update->parent_update; + parent_update; + parent_update = parent_update->parent_update) { + oidcpy(&parent_update->lock->old_oid, &lock->old_oid); + } - return TRANSACTION_GENERIC_ERROR; + if ((update->flags & REF_HAVE_OLD) && + hashcmp(lock->old_oid.hash, update->old_sha1)) { + if (is_null_sha1(update->old_sha1)) + strbuf_addf(err, "cannot lock ref '%s': reference already exists", + original_update_refname(update)); + else + strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s", + original_update_refname(update), + sha1_to_hex(lock->old_oid.hash), + sha1_to_hex(update->old_sha1)); + + return TRANSACTION_GENERIC_ERROR; + } } if ((update->flags & REF_HAVE_NEW) && diff --git a/refs/refs-internal.h b/refs/refs-internal.h index cccd76b28..1bb3d87dc 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -143,24 +143,41 @@ int should_autocreate_reflog(const char *refname); * not exist before update. */ struct ref_update { + /* * If (flags & REF_HAVE_NEW), set the reference to this value: */ unsigned char new_sha1[20]; + /* * If (flags & REF_HAVE_OLD), check that the reference * previously had this value: */ unsigned char old_sha1[20]; + /* * One or more of REF_HAVE_NEW, REF_HAVE_OLD, REF_NODEREF, * REF_DELETING, REF_ISPRUNING, REF_LOG_ONLY, and * REF_UPDATE_VIA_HEAD: */ unsigned int flags; + struct ref_lock *lock; unsigned int type; char *msg; + + /* + * If this ref_update was split off of a symref update via + * split_symref_update(), then this member points at that + * update. This is used for two purposes: + * 1. When reporting errors, we report the refname under which + * the update was originally requested. + * 2. When we read the old value of this reference, we + * propagate it back to its parent update for recording in + * the latter's reflog. + */ + struct ref_update *parent_update; + const char refname[FLEX_ARRAY]; }; -- cgit v1.2.1 From 5d9b2de4ef5a6b0cc38bbb3affcc614a66c663d7 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 22 Apr 2016 14:38:56 +0200 Subject: commit_ref_update(): remove the flags parameter commit_ref_update() is now only called with flags=0. So remove the flags parameter entirely. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index a78351343..7f9949352 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2546,7 +2546,7 @@ static int write_ref_to_lockfile(struct ref_lock *lock, const unsigned char *sha1, struct strbuf *err); static int commit_ref_update(struct ref_lock *lock, const unsigned char *sha1, const char *logmsg, - int flags, struct strbuf *err); + struct strbuf *err); int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsg) { @@ -2622,7 +2622,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms hashcpy(lock->old_oid.hash, orig_sha1); if (write_ref_to_lockfile(lock, orig_sha1, &err) || - commit_ref_update(lock, orig_sha1, logmsg, 0, &err)) { + commit_ref_update(lock, orig_sha1, logmsg, &err)) { error("unable to write current sha1 into %s: %s", newrefname, err.buf); strbuf_release(&err); goto rollback; @@ -2642,7 +2642,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms flag = log_all_ref_updates; log_all_ref_updates = 0; if (write_ref_to_lockfile(lock, orig_sha1, &err) || - commit_ref_update(lock, orig_sha1, NULL, 0, &err)) { + commit_ref_update(lock, orig_sha1, NULL, &err)) { error("unable to write current sha1 into %s: %s", oldrefname, err.buf); strbuf_release(&err); } @@ -2880,12 +2880,12 @@ static int write_ref_to_lockfile(struct ref_lock *lock, */ static int commit_ref_update(struct ref_lock *lock, const unsigned char *sha1, const char *logmsg, - int flags, struct strbuf *err) + struct strbuf *err) { clear_loose_ref_cache(&ref_cache); - if (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, flags, err) < 0 || + if (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, 0, err) < 0 || (strcmp(lock->ref_name, lock->orig_ref_name) && - log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg, flags, err) < 0)) { + log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg, 0, err) < 0)) { char *old_msg = strbuf_detach(err, NULL); strbuf_addf(err, "cannot update the ref '%s': %s", lock->ref_name, old_msg); @@ -2921,7 +2921,7 @@ static int commit_ref_update(struct ref_lock *lock, } } } - if (!(flags & REF_LOG_ONLY) && commit_ref(lock)) { + if (commit_ref(lock)) { strbuf_addf(err, "couldn't set '%s'", lock->ref_name); unlock_ref(lock); return -1; -- cgit v1.2.1 From 7a418f3a17b95746eb94cfd55f4fe0385d058777 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Fri, 22 Apr 2016 15:25:25 +0200 Subject: lock_ref_sha1_basic(): only handle REF_NODEREF mode Now lock_ref_sha1_basic() is only called with flags==REF_NODEREF. So we don't have to handle other cases anymore. This enables several simplifications, the most interesting of which come from the fact that ref_lock::orig_ref_name is now always the same as ref_lock::ref_name: * Remove ref_lock::orig_ref_name * Remove local variable orig_refname from lock_ref_sha1_basic() * ref_name can be initialize once and its value reused * commit_ref_update() never has to write to the reflog for lock->orig_ref_name Signed-off-by: Michael Haggerty --- refs/files-backend.c | 54 +++++++++++++++++++--------------------------------- 1 file changed, 20 insertions(+), 34 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 7f9949352..bbf96ad83 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -7,7 +7,6 @@ struct ref_lock { char *ref_name; - char *orig_ref_name; struct lock_file *lk; struct object_id old_oid; }; @@ -1522,7 +1521,6 @@ static void unlock_ref(struct ref_lock *lock) if (lock->lk) rollback_lock_file(lock->lk); free(lock->ref_name); - free(lock->orig_ref_name); free(lock); } @@ -1576,7 +1574,6 @@ static int lock_raw_ref(const char *refname, int mustexist, *lock_p = lock = xcalloc(1, sizeof(*lock)); lock->ref_name = xstrdup(refname); - lock->orig_ref_name = xstrdup(refname); strbuf_git_path(&ref_file, "%s", refname); retry: @@ -1969,14 +1966,13 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, struct strbuf *err) { struct strbuf ref_file = STRBUF_INIT; - struct strbuf orig_ref_file = STRBUF_INIT; - const char *orig_refname = refname; struct ref_lock *lock; int last_errno = 0; - int lflags = 0; + int lflags = LOCK_NO_DEREF; int mustexist = (old_sha1 && !is_null_sha1(old_sha1)); - int resolve_flags = 0; + int resolve_flags = RESOLVE_REF_NO_RECURSE; int attempts_remaining = 3; + int resolved; assert(err); @@ -1986,46 +1982,39 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, resolve_flags |= RESOLVE_REF_READING; if (flags & REF_DELETING) resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME; - if (flags & REF_NODEREF) { - resolve_flags |= RESOLVE_REF_NO_RECURSE; - lflags |= LOCK_NO_DEREF; - } - refname = resolve_ref_unsafe(refname, resolve_flags, - lock->old_oid.hash, type); - if (!refname && errno == EISDIR) { + strbuf_git_path(&ref_file, "%s", refname); + resolved = !!resolve_ref_unsafe(refname, resolve_flags, + lock->old_oid.hash, type); + if (!resolved && errno == EISDIR) { /* * we are trying to lock foo but we used to * have foo/bar which now does not exist; * it is normal for the empty directory 'foo' * to remain. */ - strbuf_git_path(&orig_ref_file, "%s", orig_refname); - if (remove_empty_directories(&orig_ref_file)) { + if (remove_empty_directories(&ref_file)) { last_errno = errno; - if (!verify_refname_available_dir(orig_refname, extras, skip, + if (!verify_refname_available_dir(refname, extras, skip, get_loose_refs(&ref_cache), err)) strbuf_addf(err, "there are still refs under '%s'", - orig_refname); + refname); goto error_return; } - refname = resolve_ref_unsafe(orig_refname, resolve_flags, - lock->old_oid.hash, type); + resolved = !!resolve_ref_unsafe(refname, resolve_flags, + lock->old_oid.hash, type); } - if (!refname) { + if (!resolved) { last_errno = errno; if (last_errno != ENOTDIR || - !verify_refname_available_dir(orig_refname, extras, skip, + !verify_refname_available_dir(refname, extras, skip, get_loose_refs(&ref_cache), err)) strbuf_addf(err, "unable to resolve reference '%s': %s", - orig_refname, strerror(last_errno)); + refname, strerror(last_errno)); goto error_return; } - if (flags & REF_NODEREF) - refname = orig_refname; - /* * If the ref did not exist and we are creating it, make sure * there is no existing packed ref whose name begins with our @@ -2042,8 +2031,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, lock->lk = xcalloc(1, sizeof(struct lock_file)); lock->ref_name = xstrdup(refname); - lock->orig_ref_name = xstrdup(orig_refname); - strbuf_git_path(&ref_file, "%s", refname); retry: switch (safe_create_leading_directories_const(ref_file.buf)) { @@ -2086,7 +2073,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, out: strbuf_release(&ref_file); - strbuf_release(&orig_ref_file); errno = last_errno; return lock; } @@ -2883,9 +2869,7 @@ static int commit_ref_update(struct ref_lock *lock, struct strbuf *err) { clear_loose_ref_cache(&ref_cache); - if (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, 0, err) < 0 || - (strcmp(lock->ref_name, lock->orig_ref_name) && - log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg, 0, err) < 0)) { + if (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); @@ -2893,7 +2877,8 @@ static int commit_ref_update(struct ref_lock *lock, unlock_ref(lock); return -1; } - if (strcmp(lock->orig_ref_name, "HEAD") != 0) { + + if (strcmp(lock->ref_name, "HEAD") != 0) { /* * Special hack: If a branch is updated directly and HEAD * points to it (may happen on the remote side of a push @@ -2909,6 +2894,7 @@ static int commit_ref_update(struct ref_lock *lock, unsigned char head_sha1[20]; int head_flag; const char *head_ref; + head_ref = resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, head_sha1, &head_flag); if (head_ref && (head_flag & REF_ISSYMREF) && @@ -2921,6 +2907,7 @@ static int commit_ref_update(struct ref_lock *lock, } } } + if (commit_ref(lock)) { strbuf_addf(err, "couldn't set '%s'", lock->ref_name); unlock_ref(lock); @@ -3026,7 +3013,6 @@ int set_worktree_head_symref(const char *gitdir, const char *target) lock = xcalloc(1, sizeof(struct ref_lock)); lock->lk = &head_lock; lock->ref_name = xstrdup(head_rel); - lock->orig_ref_name = xstrdup(head_rel); ret = create_symref_locked(lock, head_rel, target, NULL); -- cgit v1.2.1