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(+) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 1f38076411..ad9cd86457 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.3 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(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index ad9cd86457..0cc116d67c 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 16752a999f..6d869d1285 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.3 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(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 0cc116d67c..2d3a8c669a 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.3 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(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 2d3a8c669a..80d346fd38 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.3 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(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 80d346fd38..814e230fa3 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.3 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(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 814e230fa3..1657ce7512 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 3a4f634cb4..0b047f67ba 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.3 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(+) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 1657ce7512..3c99eef42a 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.3 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(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 3c99eef42a..a23ade4ffb 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 0b047f67ba..37a1a37ea0 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.3 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(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index a23ade4ffb..c41cf432d1 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.3 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(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index c41cf432d1..c2bd7b8317 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 37a1a37ea0..de7722e323 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.3 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(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index c2bd7b8317..dc247e0e7b 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.3 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(-) (limited to 'refs/files-backend.c') diff --git a/refs.c b/refs.c index b18d9959af..ba14105959 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 dc247e0e7b..c978fe49c9 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 af1b20dd5c..40b0ccedfc 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.3 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(-) (limited to 'refs/files-backend.c') diff --git a/refs.c b/refs.c index ba14105959..5dc2473fbb 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 c978fe49c9..35d37ce58b 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 de7722e323..1f94f7a262 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.3 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(+) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 35d37ce58b..85e1e1c759 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.3 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(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 85e1e1c759..2f98eebe94 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 1f94f7a262..85f4650ab8 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.3 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(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 2f98eebe94..15ba456d68 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 f3e3b6cf2e..42811604bb 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.3 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(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 15ba456d68..1d1b72d57d 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 85f4650ab8..9686e60942 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.3 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(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 1d1b72d57d..85751977f6 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.3 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(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 85751977f6..dc0bde05b8 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.3 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(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index dc0bde05b8..ffc30fd232 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 babdf2769f..cccd76b28c 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 08bd8fd8d6..d226930412 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.3 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(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index ffc30fd232..7bc18322aa 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.3 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(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 7bc18322aa..a783513435 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 cccd76b28c..1bb3d87dc5 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.3 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(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index a783513435..7f9949352b 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.3 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(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 7f9949352b..bbf96ad83a 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.3 From 067622b0e8afde8b39884e221b7a56299036f6d9 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 18 Jun 2016 06:15:08 +0200 Subject: do_for_each_ref(): move docstring to the header file Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/files-backend.c | 9 --------- refs/refs-internal.h | 10 +++++++++- 2 files changed, 9 insertions(+), 10 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index bbf96ad83a..8fa897b3b8 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1878,15 +1878,6 @@ static int do_for_each_entry(struct ref_cache *refs, const char *base, return retval; } -/* - * Call fn for each reference in the specified ref_cache for which the - * refname begins with base. If trim is non-zero, then trim that many - * characters off the beginning of each refname before passing the - * refname to fn. flags can be DO_FOR_EACH_INCLUDE_BROKEN to include - * broken references in the iteration. If fn ever returns a non-zero - * value, stop the iteration and return that value; otherwise, return - * 0. - */ int do_for_each_ref(const char *submodule, const char *base, each_ref_fn fn, int trim, int flags, void *cb_data) { diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 1bb3d87dc5..b4dd5453ea 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -249,7 +249,15 @@ int rename_ref_available(const char *oldname, const char *newname); #define DO_FOR_EACH_INCLUDE_BROKEN 0x01 /* - * The common backend for the for_each_*ref* functions + * Call fn for each reference in the specified submodule for which the + * refname begins with base. If trim is non-zero, then trim that many + * characters off the beginning of each refname before passing the + * refname to fn. flags can be DO_FOR_EACH_INCLUDE_BROKEN to include + * broken references in the iteration. If fn ever returns a non-zero + * value, stop the iteration and return that value; otherwise, return + * 0. + * + * This is the common backend for the for_each_*ref* functions. */ int do_for_each_ref(const char *submodule, const char *base, each_ref_fn fn, int trim, int flags, void *cb_data); -- cgit v1.2.3 From 4633a846f5cf085bc11de702d0e78177aa44a907 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 18 Jun 2016 06:15:09 +0200 Subject: refs: use name "prefix" consistently In the context of the for_each_ref() functions, call the prefix that references must start with "prefix". (In some places it was called "base".) This is clearer, and also prevents confusion with another planned use of the word "base". Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/files-backend.c | 24 ++++++++++++------------ refs/refs-internal.h | 14 +++++++------- 2 files changed, 19 insertions(+), 19 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 8fa897b3b8..d5c4789619 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -542,7 +542,7 @@ static struct ref_entry *current_ref; typedef int each_ref_entry_fn(struct ref_entry *entry, void *cb_data); struct ref_entry_cb { - const char *base; + const char *prefix; int trim; int flags; each_ref_fn *fn; @@ -559,7 +559,7 @@ static int do_one_ref(struct ref_entry *entry, void *cb_data) struct ref_entry *old_current_ref; int retval; - if (!starts_with(entry->name, data->base)) + if (!starts_with(entry->name, data->prefix)) return 0; if (!(data->flags & DO_FOR_EACH_INCLUDE_BROKEN) && @@ -1824,12 +1824,12 @@ int peel_ref(const char *refname, unsigned char *sha1) /* * Call fn for each reference in the specified ref_cache, omitting - * references not in the containing_dir of base. fn is called for all - * references, including broken ones. If fn ever returns a non-zero + * references not in the containing_dir of prefix. Call fn for all + * references, including broken ones. If fn ever returns a non-zero * value, stop the iteration and return that value; otherwise, return * 0. */ -static int do_for_each_entry(struct ref_cache *refs, const char *base, +static int do_for_each_entry(struct ref_cache *refs, const char *prefix, each_ref_entry_fn fn, void *cb_data) { struct packed_ref_cache *packed_ref_cache; @@ -1846,8 +1846,8 @@ static int do_for_each_entry(struct ref_cache *refs, const char *base, * disk. */ loose_dir = get_loose_refs(refs); - if (base && *base) { - loose_dir = find_containing_dir(loose_dir, base, 0); + if (prefix && *prefix) { + loose_dir = find_containing_dir(loose_dir, prefix, 0); } if (loose_dir) prime_ref_dir(loose_dir); @@ -1855,8 +1855,8 @@ static int do_for_each_entry(struct ref_cache *refs, const char *base, packed_ref_cache = get_packed_ref_cache(refs); acquire_packed_ref_cache(packed_ref_cache); packed_dir = get_packed_ref_dir(packed_ref_cache); - if (base && *base) { - packed_dir = find_containing_dir(packed_dir, base, 0); + if (prefix && *prefix) { + packed_dir = find_containing_dir(packed_dir, prefix, 0); } if (packed_dir && loose_dir) { @@ -1878,14 +1878,14 @@ static int do_for_each_entry(struct ref_cache *refs, const char *base, return retval; } -int do_for_each_ref(const char *submodule, const char *base, +int do_for_each_ref(const char *submodule, const char *prefix, each_ref_fn fn, int trim, int flags, void *cb_data) { struct ref_entry_cb data; struct ref_cache *refs; refs = get_ref_cache(submodule); - data.base = base; + data.prefix = prefix; data.trim = trim; data.flags = flags; data.fn = fn; @@ -1896,7 +1896,7 @@ int do_for_each_ref(const char *submodule, const char *base, if (ref_paranoia) data.flags |= DO_FOR_EACH_INCLUDE_BROKEN; - return do_for_each_entry(refs, base, do_one_ref, &data); + return do_for_each_entry(refs, prefix, do_one_ref, &data); } /* diff --git a/refs/refs-internal.h b/refs/refs-internal.h index b4dd5453ea..8ad02d84df 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -250,16 +250,16 @@ int rename_ref_available(const char *oldname, const char *newname); /* * Call fn for each reference in the specified submodule for which the - * refname begins with base. If trim is non-zero, then trim that many - * characters off the beginning of each refname before passing the - * refname to fn. flags can be DO_FOR_EACH_INCLUDE_BROKEN to include - * broken references in the iteration. If fn ever returns a non-zero - * value, stop the iteration and return that value; otherwise, return - * 0. + * refname begins with prefix. If trim is non-zero, then trim that + * many characters off the beginning of each refname before passing + * the refname to fn. flags can be DO_FOR_EACH_INCLUDE_BROKEN to + * include broken references in the iteration. If fn ever returns a + * non-zero value, stop the iteration and return that value; + * otherwise, return 0. * * This is the common backend for the for_each_*ref* functions. */ -int do_for_each_ref(const char *submodule, const char *base, +int do_for_each_ref(const char *submodule, const char *prefix, each_ref_fn fn, int trim, int flags, void *cb_data); /* -- cgit v1.2.3 From c5f04dddb6cf5f76adfe145de3565411711255b8 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 18 Jun 2016 06:15:10 +0200 Subject: delete_refs(): add a flags argument This will be useful for passing REF_NODEREF through. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/fetch.c | 2 +- builtin/remote.c | 4 ++-- refs.h | 5 +++-- refs/files-backend.c | 4 ++-- 4 files changed, 8 insertions(+), 7 deletions(-) (limited to 'refs/files-backend.c') diff --git a/builtin/fetch.c b/builtin/fetch.c index f8455bde7a..b55c83c468 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -806,7 +806,7 @@ static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map, for (ref = stale_refs; ref; ref = ref->next) string_list_append(&refnames, ref->name); - result = delete_refs(&refnames); + result = delete_refs(&refnames, 0); string_list_clear(&refnames, 0); } diff --git a/builtin/remote.c b/builtin/remote.c index fda5c2e53d..1bbf9b44df 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -788,7 +788,7 @@ static int rm(int argc, const char **argv) strbuf_release(&buf); if (!result) - result = delete_refs(&branches); + result = delete_refs(&branches, 0); string_list_clear(&branches, 0); if (skipped.nr) { @@ -1303,7 +1303,7 @@ static int prune_remote(const char *remote, int dry_run) string_list_sort(&refs_to_prune); if (!dry_run) - result |= delete_refs(&refs_to_prune); + result |= delete_refs(&refs_to_prune, 0); for_each_string_list_item(item, &states.stale) { const char *refname = item->util; diff --git a/refs.h b/refs.h index 21874f023b..6d515a4671 100644 --- a/refs.h +++ b/refs.h @@ -274,9 +274,10 @@ int delete_ref(const char *refname, const unsigned char *old_sha1, /* * Delete the specified references. If there are any problems, emit * errors but attempt to keep going (i.e., the deletes are not done in - * an all-or-nothing transaction). + * an all-or-nothing transaction). flags is passed through to + * ref_transaction_delete(). */ -int delete_refs(struct string_list *refnames); +int delete_refs(struct string_list *refnames, unsigned int flags); /** Delete a reflog */ int delete_reflog(const char *refname); diff --git a/refs/files-backend.c b/refs/files-backend.c index d5c4789619..e15f7ae68b 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2403,7 +2403,7 @@ static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err) return 0; } -int delete_refs(struct string_list *refnames) +int delete_refs(struct string_list *refnames, unsigned int flags) { struct strbuf err = STRBUF_INIT; int i, result = 0; @@ -2432,7 +2432,7 @@ int delete_refs(struct string_list *refnames) for (i = 0; i < refnames->nr; i++) { const char *refname = refnames->items[i].string; - if (delete_ref(refname, NULL, 0)) + if (delete_ref(refname, NULL, flags)) result |= error(_("could not remove reference %s"), refname); } -- cgit v1.2.3 From 2eed2780f0e36e4a9038d65d9125cde5d5e1d2c6 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 18 Jun 2016 06:15:12 +0200 Subject: get_ref_cache(): only create an instance if there is a submodule If there is not a nonbare repository where a submodule is supposedly located, then don't instantiate a ref_cache for it. The analogous check can be removed from resolve_gitlink_ref(). Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/files-backend.c | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index e15f7ae68b..b563a7e875 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -954,15 +954,26 @@ static struct ref_cache *lookup_ref_cache(const char *submodule) /* * Return a pointer to a ref_cache for the specified submodule. For - * the main repository, use submodule==NULL. The returned structure - * will be allocated and initialized but not necessarily populated; it - * should not be freed. + * the main repository, use submodule==NULL; such a call cannot fail. + * For a submodule, the submodule must exist and be a nonbare + * repository, otherwise return NULL. + * + * The returned structure will be allocated and initialized but not + * necessarily populated; it should not be freed. */ static struct ref_cache *get_ref_cache(const char *submodule) { struct ref_cache *refs = lookup_ref_cache(submodule); - if (!refs) - refs = create_ref_cache(submodule); + + if (!refs) { + struct strbuf submodule_sb = STRBUF_INIT; + + strbuf_addstr(&submodule_sb, submodule); + if (is_nonbare_repository_dir(&submodule_sb)) + refs = create_ref_cache(submodule); + strbuf_release(&submodule_sb); + } + return refs; } @@ -1341,13 +1352,10 @@ int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *sh return -1; strbuf_add(&submodule, path, len); - refs = lookup_ref_cache(submodule.buf); + refs = get_ref_cache(submodule.buf); if (!refs) { - if (!is_nonbare_repository_dir(&submodule)) { - strbuf_release(&submodule); - return -1; - } - refs = create_ref_cache(submodule.buf); + strbuf_release(&submodule); + return -1; } strbuf_release(&submodule); @@ -1885,6 +1893,9 @@ int do_for_each_ref(const char *submodule, const char *prefix, struct ref_cache *refs; refs = get_ref_cache(submodule); + if (!refs) + return 0; + data.prefix = prefix; data.trim = trim; data.flags = flags; -- cgit v1.2.3 From ffeef642310f00bc43341bcb9eeadf06e0fd5904 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 18 Jun 2016 06:15:13 +0200 Subject: entry_resolves_to_object(): rename function from ref_resolves_to_object() Free up the old name for a more general purpose. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/files-backend.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index b563a7e875..c24a78ea8e 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -517,7 +517,7 @@ static void sort_ref_dir(struct ref_dir *dir) * an object in the database. Emit a warning if the referred-to * object does not exist. */ -static int ref_resolves_to_object(struct ref_entry *entry) +static int entry_resolves_to_object(struct ref_entry *entry) { if (entry->flag & REF_ISBROKEN) return 0; @@ -563,7 +563,7 @@ static int do_one_ref(struct ref_entry *entry, void *cb_data) return 0; if (!(data->flags & DO_FOR_EACH_INCLUDE_BROKEN) && - !ref_resolves_to_object(entry)) + !entry_resolves_to_object(entry)) return 0; /* Store the old value, in case this is a recursive call: */ @@ -2228,7 +2228,7 @@ static int pack_if_possible_fn(struct ref_entry *entry, void *cb_data) return 0; /* Do not pack symbolic or broken refs: */ - if ((entry->flag & REF_ISSYMREF) || !ref_resolves_to_object(entry)) + if ((entry->flag & REF_ISSYMREF) || !entry_resolves_to_object(entry)) return 0; /* Add a packed ref cache entry equivalent to the loose entry. */ -- cgit v1.2.3 From a873924483ec6ec548e94d723fe62875b1a4ec93 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 18 Jun 2016 06:15:14 +0200 Subject: ref_resolves_to_object(): new function Extract new function ref_resolves_to_object() from entry_resolves_to_object(). It can be used even if there is no ref_entry at hand. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/files-backend.c | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index c24a78ea8e..62280b5c70 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -513,21 +513,34 @@ static void sort_ref_dir(struct ref_dir *dir) } /* - * Return true iff the reference described by entry can be resolved to - * an object in the database. Emit a warning if the referred-to - * object does not exist. + * Return true if refname, which has the specified oid and flags, can + * be resolved to an object in the database. If the referred-to object + * does not exist, emit a warning and return false. */ -static int entry_resolves_to_object(struct ref_entry *entry) +static int ref_resolves_to_object(const char *refname, + const struct object_id *oid, + unsigned int flags) { - if (entry->flag & REF_ISBROKEN) + if (flags & REF_ISBROKEN) return 0; - if (!has_sha1_file(entry->u.value.oid.hash)) { - error("%s does not point to a valid object!", entry->name); + if (!has_sha1_file(oid->hash)) { + error("%s does not point to a valid object!", refname); return 0; } return 1; } +/* + * Return true if the reference described by entry can be resolved to + * an object in the database; otherwise, emit a warning and return + * false. + */ +static int entry_resolves_to_object(struct ref_entry *entry) +{ + return ref_resolves_to_object(entry->name, + &entry->u.value.oid, entry->flag); +} + /* * current_ref is a performance hack: when iterating over references * using the for_each_ref*() functions, current_ref is set to the -- cgit v1.2.3 From 3bc581b9406e1e9a3f879d379106ee1e3bc48f5c Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 18 Jun 2016 06:15:15 +0200 Subject: refs: introduce an iterator interface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently, the API for iterating over references is via a family of for_each_ref()-type functions that invoke a callback function for each selected reference. All of these eventually call do_for_each_ref(), which knows how to do one thing: iterate in parallel through two ref_caches, one for loose and one for packed refs, giving loose references precedence over packed refs. This is rather complicated code, and is quite specialized to the files backend. It also requires callers to encapsulate their work into a callback function, which often means that they have to define and use a "cb_data" struct to manage their context. The current design is already bursting at the seams, and will become even more awkward in the upcoming world of multiple reference storage backends: * Per-worktree vs. shared references are currently handled via a kludge in git_path() rather than iterating over each part of the reference namespace separately and merging the results. This kludge will cease to work when we have multiple reference storage backends. * The current scheme is inflexible. What if we sometimes want to bypass the ref_cache, or use it only for packed or only for loose refs? What if we want to store symbolic refs in one type of storage backend and non-symbolic ones in another? In the future, each reference backend will need to define its own way of iterating over references. The crux of the problem with the current design is that it is impossible to compose for_each_ref()-style iterations, because the flow of control is owned by the for_each_ref() function. There is nothing that a caller can do but iterate through all references in a single burst, so there is no way for it to interleave references from multiple backends and present the result to the rest of the world as a single compound backend. This commit introduces a new iteration primitive for references: a ref_iterator. A ref_iterator is a polymorphic object that a reference storage backend can be asked to instantiate. There are three functions that can be applied to a ref_iterator: * ref_iterator_advance(): move to the next reference in the iteration * ref_iterator_abort(): end the iteration before it is exhausted * ref_iterator_peel(): peel the reference currently being looked at Iterating using a ref_iterator leaves the flow of control in the hands of the caller, which means that ref_iterators from multiple sources (e.g., loose and packed refs) can be composed and presented to the world as a single compound ref_iterator. It also means that the backend code for implementing reference iteration will sometimes be more complicated. For example, the cache_ref_iterator (which iterates over a ref_cache) can't use the C stack to recurse; instead, it must manage its own stack internally as explicit data structures. There is also a lot of boilerplate connected with object-oriented programming in C. Eventually, end-user callers will be able to be written in a more natural way—managing their own flow of control rather than having to work via callbacks. Since there will only be a few reference backends but there are many consumers of this API, this is a good tradeoff. More importantly, we gain composability, and especially the possibility of writing interchangeable parts that can work with any ref_iterator. For example, merge_ref_iterator implements a generic way of merging the contents of any two ref_iterators. It is used to merge loose + packed refs as part of the implementation of the files_ref_iterator. But it will also be possible to use it to merge other pairs of reference sources (e.g., per-worktree vs. shared refs). Another example is prefix_ref_iterator, which can be used to trim a prefix off the front of reference names before presenting them to the caller (e.g., "refs/heads/master" -> "master"). In this patch, we introduce the iterator abstraction and many utilities, and implement a reference iterator for the files ref storage backend. (I've written several other obvious utilities, for example a generic way to filter references being iterated over. These will probably be useful in the future. But they are not needed for this patch series, so I am not including them at this time.) In a moment we will rewrite do_for_each_ref() to work via reference iterators (allowing some special-purpose code to be discarded), and do something similar for reflogs. In future patch series, we will expose the ref_iterator abstraction in the public refs API so that callers can use it directly. Implementation note: I tried abstracting this a layer further to allow generic iterators (over arbitrary types of objects) and generic utilities like a generic merge_iterator. But the implementation in C was very cumbersome, involving (in my opinion) too much boilerplate and too much unsafe casting, some of which would have had to be done on the caller side. However, I did put a few iterator-related constants in a top-level header file, iterator.h, as they will be useful in a moment to implement iteration over directory trees and possibly other types of iterators in the future. Signed-off-by: Ramsay Jones Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- Makefile | 1 + iterator.h | 81 ++++++++++++ refs.h | 4 +- refs/files-backend.c | 281 ++++++++++++++++++++++++++++++++++++++++ refs/iterator.c | 355 +++++++++++++++++++++++++++++++++++++++++++++++++++ refs/refs-internal.h | 194 ++++++++++++++++++++++++++++ 6 files changed, 915 insertions(+), 1 deletion(-) create mode 100644 iterator.h create mode 100644 refs/iterator.c (limited to 'refs/files-backend.c') diff --git a/Makefile b/Makefile index a83e322f46..ac8f365d04 100644 --- a/Makefile +++ b/Makefile @@ -786,6 +786,7 @@ LIB_OBJS += read-cache.o LIB_OBJS += reflog-walk.o LIB_OBJS += refs.o LIB_OBJS += refs/files-backend.o +LIB_OBJS += refs/iterator.o LIB_OBJS += ref-filter.o LIB_OBJS += remote.o LIB_OBJS += replace_object.o diff --git a/iterator.h b/iterator.h new file mode 100644 index 0000000000..0f6900e43a --- /dev/null +++ b/iterator.h @@ -0,0 +1,81 @@ +#ifndef ITERATOR_H +#define ITERATOR_H + +/* + * Generic constants related to iterators. + */ + +/* + * The attempt to advance the iterator was successful; the iterator + * reflects the new current entry. + */ +#define ITER_OK 0 + +/* + * The iterator is exhausted and has been freed. + */ +#define ITER_DONE -1 + +/* + * The iterator experienced an error. The iteration has been aborted + * and the iterator has been freed. + */ +#define ITER_ERROR -2 + +/* + * Return values for selector functions for merge iterators. The + * numerical values of these constants are important and must be + * compatible with ITER_DONE and ITER_ERROR. + */ +enum iterator_selection { + /* End the iteration without an error: */ + ITER_SELECT_DONE = ITER_DONE, + + /* Report an error and abort the iteration: */ + ITER_SELECT_ERROR = ITER_ERROR, + + /* + * The next group of constants are masks that are useful + * mainly internally. + */ + + /* The LSB selects whether iter0/iter1 is the "current" iterator: */ + ITER_CURRENT_SELECTION_MASK = 0x01, + + /* iter0 is the "current" iterator this round: */ + ITER_CURRENT_SELECTION_0 = 0x00, + + /* iter1 is the "current" iterator this round: */ + ITER_CURRENT_SELECTION_1 = 0x01, + + /* Yield the value from the current iterator? */ + ITER_YIELD_CURRENT = 0x02, + + /* Discard the value from the secondary iterator? */ + ITER_SKIP_SECONDARY = 0x04, + + /* + * The constants that a selector function should usually + * return. + */ + + /* Yield the value from iter0: */ + ITER_SELECT_0 = ITER_CURRENT_SELECTION_0 | ITER_YIELD_CURRENT, + + /* Yield the value from iter0 and discard the one from iter1: */ + ITER_SELECT_0_SKIP_1 = ITER_SELECT_0 | ITER_SKIP_SECONDARY, + + /* Discard the value from iter0 without yielding anything this round: */ + ITER_SKIP_0 = ITER_CURRENT_SELECTION_1 | ITER_SKIP_SECONDARY, + + /* Yield the value from iter1: */ + ITER_SELECT_1 = ITER_CURRENT_SELECTION_1 | ITER_YIELD_CURRENT, + + /* Yield the value from iter1 and discard the one from iter0: */ + ITER_SELECT_1_SKIP_0 = ITER_SELECT_1 | ITER_SKIP_SECONDARY, + + /* Discard the value from iter1 without yielding anything this round: */ + ITER_SKIP_1 = ITER_CURRENT_SELECTION_0 | ITER_SKIP_SECONDARY +}; + +#endif /* ITERATOR_H */ diff --git a/refs.h b/refs.h index 6d515a4671..442c1a5db8 100644 --- a/refs.h +++ b/refs.h @@ -141,7 +141,9 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **ref); struct ref_transaction; /* - * Bit values set in the flags argument passed to each_ref_fn(): + * Bit values set in the flags argument passed to each_ref_fn() and + * stored in ref_iterator::flags. Other bits are for internal use + * only: */ /* Reference is a symbolic reference. */ diff --git a/refs/files-backend.c b/refs/files-backend.c index 62280b5c70..395056b675 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1,6 +1,7 @@ #include "../cache.h" #include "../refs.h" #include "refs-internal.h" +#include "../iterator.h" #include "../lockfile.h" #include "../object.h" #include "../dir.h" @@ -704,6 +705,153 @@ static void prime_ref_dir(struct ref_dir *dir) } } +/* + * A level in the reference hierarchy that is currently being iterated + * through. + */ +struct cache_ref_iterator_level { + /* + * The ref_dir being iterated over at this level. The ref_dir + * is sorted before being stored here. + */ + struct ref_dir *dir; + + /* + * The index of the current entry within dir (which might + * itself be a directory). If index == -1, then the iteration + * hasn't yet begun. If index == dir->nr, then the iteration + * through this level is over. + */ + int index; +}; + +/* + * Represent an iteration through a ref_dir in the memory cache. The + * iteration recurses through subdirectories. + */ +struct cache_ref_iterator { + struct ref_iterator base; + + /* + * The number of levels currently on the stack. This is always + * at least 1, because when it becomes zero the iteration is + * ended and this struct is freed. + */ + size_t levels_nr; + + /* The number of levels that have been allocated on the stack */ + size_t levels_alloc; + + /* + * A stack of levels. levels[0] is the uppermost level that is + * being iterated over in this iteration. (This is not + * necessary the top level in the references hierarchy. If we + * are iterating through a subtree, then levels[0] will hold + * the ref_dir for that subtree, and subsequent levels will go + * on from there.) + */ + struct cache_ref_iterator_level *levels; +}; + +static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator) +{ + struct cache_ref_iterator *iter = + (struct cache_ref_iterator *)ref_iterator; + + while (1) { + struct cache_ref_iterator_level *level = + &iter->levels[iter->levels_nr - 1]; + struct ref_dir *dir = level->dir; + struct ref_entry *entry; + + if (level->index == -1) + sort_ref_dir(dir); + + if (++level->index == level->dir->nr) { + /* This level is exhausted; pop up a level */ + if (--iter->levels_nr == 0) + return ref_iterator_abort(ref_iterator); + + continue; + } + + entry = dir->entries[level->index]; + + if (entry->flag & REF_DIR) { + /* push down a level */ + ALLOC_GROW(iter->levels, iter->levels_nr + 1, + iter->levels_alloc); + + level = &iter->levels[iter->levels_nr++]; + level->dir = get_ref_dir(entry); + level->index = -1; + } else { + iter->base.refname = entry->name; + iter->base.oid = &entry->u.value.oid; + iter->base.flags = entry->flag; + return ITER_OK; + } + } +} + +static enum peel_status peel_entry(struct ref_entry *entry, int repeel); + +static int cache_ref_iterator_peel(struct ref_iterator *ref_iterator, + struct object_id *peeled) +{ + struct cache_ref_iterator *iter = + (struct cache_ref_iterator *)ref_iterator; + struct cache_ref_iterator_level *level; + struct ref_entry *entry; + + level = &iter->levels[iter->levels_nr - 1]; + + if (level->index == -1) + die("BUG: peel called before advance for cache iterator"); + + entry = level->dir->entries[level->index]; + + if (peel_entry(entry, 0)) + return -1; + hashcpy(peeled->hash, entry->u.value.peeled.hash); + return 0; +} + +static int cache_ref_iterator_abort(struct ref_iterator *ref_iterator) +{ + struct cache_ref_iterator *iter = + (struct cache_ref_iterator *)ref_iterator; + + free(iter->levels); + base_ref_iterator_free(ref_iterator); + return ITER_DONE; +} + +static struct ref_iterator_vtable cache_ref_iterator_vtable = { + cache_ref_iterator_advance, + cache_ref_iterator_peel, + cache_ref_iterator_abort +}; + +static struct ref_iterator *cache_ref_iterator_begin(struct ref_dir *dir) +{ + struct cache_ref_iterator *iter; + struct ref_iterator *ref_iterator; + struct cache_ref_iterator_level *level; + + iter = xcalloc(1, sizeof(*iter)); + ref_iterator = &iter->base; + base_ref_iterator_init(ref_iterator, &cache_ref_iterator_vtable); + ALLOC_GROW(iter->levels, 10, iter->levels_alloc); + + iter->levels_nr = 1; + level = &iter->levels[0]; + level->index = -1; + level->dir = dir; + + return ref_iterator; +} + struct nonmatching_ref_data { const struct string_list *skip; const char *conflicting_refname; @@ -1843,6 +1991,139 @@ int peel_ref(const char *refname, unsigned char *sha1) return peel_object(base, sha1); } +struct files_ref_iterator { + struct ref_iterator base; + + struct packed_ref_cache *packed_ref_cache; + struct ref_iterator *iter0; + unsigned int flags; +}; + +static int files_ref_iterator_advance(struct ref_iterator *ref_iterator) +{ + struct files_ref_iterator *iter = + (struct files_ref_iterator *)ref_iterator; + int ok; + + while ((ok = ref_iterator_advance(iter->iter0)) == ITER_OK) { + if (!(iter->flags & DO_FOR_EACH_INCLUDE_BROKEN) && + !ref_resolves_to_object(iter->iter0->refname, + iter->iter0->oid, + iter->iter0->flags)) + continue; + + iter->base.refname = iter->iter0->refname; + iter->base.oid = iter->iter0->oid; + iter->base.flags = iter->iter0->flags; + return ITER_OK; + } + + iter->iter0 = NULL; + if (ref_iterator_abort(ref_iterator) != ITER_DONE) + ok = ITER_ERROR; + + return ok; +} + +static int files_ref_iterator_peel(struct ref_iterator *ref_iterator, + struct object_id *peeled) +{ + struct files_ref_iterator *iter = + (struct files_ref_iterator *)ref_iterator; + + return ref_iterator_peel(iter->iter0, peeled); +} + +static int files_ref_iterator_abort(struct ref_iterator *ref_iterator) +{ + struct files_ref_iterator *iter = + (struct files_ref_iterator *)ref_iterator; + int ok = ITER_DONE; + + if (iter->iter0) + ok = ref_iterator_abort(iter->iter0); + + release_packed_ref_cache(iter->packed_ref_cache); + base_ref_iterator_free(ref_iterator); + return ok; +} + +static struct ref_iterator_vtable files_ref_iterator_vtable = { + files_ref_iterator_advance, + files_ref_iterator_peel, + files_ref_iterator_abort +}; + +struct ref_iterator *files_ref_iterator_begin( + const char *submodule, + const char *prefix, unsigned int flags) +{ + struct ref_cache *refs = get_ref_cache(submodule); + struct ref_dir *loose_dir, *packed_dir; + struct ref_iterator *loose_iter, *packed_iter; + struct files_ref_iterator *iter; + struct ref_iterator *ref_iterator; + + if (!refs) + return empty_ref_iterator_begin(); + + if (ref_paranoia < 0) + ref_paranoia = git_env_bool("GIT_REF_PARANOIA", 0); + if (ref_paranoia) + flags |= DO_FOR_EACH_INCLUDE_BROKEN; + + iter = xcalloc(1, sizeof(*iter)); + ref_iterator = &iter->base; + base_ref_iterator_init(ref_iterator, &files_ref_iterator_vtable); + + /* + * We must make sure that all loose refs are read before + * accessing the packed-refs file; this avoids a race + * condition if loose refs are migrated to the packed-refs + * file by a simultaneous process, but our in-memory view is + * from before the migration. We ensure this as follows: + * First, we call prime_ref_dir(), which pre-reads the loose + * references for the subtree into the cache. (If they've + * already been read, that's OK; we only need to guarantee + * that they're read before the packed refs, not *how much* + * before.) After that, we call get_packed_ref_cache(), which + * internally checks whether the packed-ref cache is up to + * date with what is on disk, and re-reads it if not. + */ + + loose_dir = get_loose_refs(refs); + + if (prefix && *prefix) + loose_dir = find_containing_dir(loose_dir, prefix, 0); + + if (loose_dir) { + prime_ref_dir(loose_dir); + loose_iter = cache_ref_iterator_begin(loose_dir); + } else { + /* There's nothing to iterate over. */ + loose_iter = empty_ref_iterator_begin(); + } + + iter->packed_ref_cache = get_packed_ref_cache(refs); + acquire_packed_ref_cache(iter->packed_ref_cache); + packed_dir = get_packed_ref_dir(iter->packed_ref_cache); + + if (prefix && *prefix) + packed_dir = find_containing_dir(packed_dir, prefix, 0); + + if (packed_dir) { + packed_iter = cache_ref_iterator_begin(packed_dir); + } else { + /* There's nothing to iterate over. */ + packed_iter = empty_ref_iterator_begin(); + } + + iter->iter0 = overlay_ref_iterator_begin(loose_iter, packed_iter); + iter->flags = flags; + + return ref_iterator; +} + /* * Call fn for each reference in the specified ref_cache, omitting * references not in the containing_dir of prefix. Call fn for all diff --git a/refs/iterator.c b/refs/iterator.c new file mode 100644 index 0000000000..93ba472f91 --- /dev/null +++ b/refs/iterator.c @@ -0,0 +1,355 @@ +/* + * Generic reference iterator infrastructure. See refs-internal.h for + * documentation about the design and use of reference iterators. + */ + +#include "cache.h" +#include "refs.h" +#include "refs/refs-internal.h" +#include "iterator.h" + +int ref_iterator_advance(struct ref_iterator *ref_iterator) +{ + return ref_iterator->vtable->advance(ref_iterator); +} + +int ref_iterator_peel(struct ref_iterator *ref_iterator, + struct object_id *peeled) +{ + return ref_iterator->vtable->peel(ref_iterator, peeled); +} + +int ref_iterator_abort(struct ref_iterator *ref_iterator) +{ + return ref_iterator->vtable->abort(ref_iterator); +} + +void base_ref_iterator_init(struct ref_iterator *iter, + struct ref_iterator_vtable *vtable) +{ + iter->vtable = vtable; + iter->refname = NULL; + iter->oid = NULL; + iter->flags = 0; +} + +void base_ref_iterator_free(struct ref_iterator *iter) +{ + /* Help make use-after-free bugs fail quickly: */ + iter->vtable = NULL; + free(iter); +} + +struct empty_ref_iterator { + struct ref_iterator base; +}; + +static int empty_ref_iterator_advance(struct ref_iterator *ref_iterator) +{ + return ref_iterator_abort(ref_iterator); +} + +static int empty_ref_iterator_peel(struct ref_iterator *ref_iterator, + struct object_id *peeled) +{ + die("BUG: peel called for empty iterator"); +} + +static int empty_ref_iterator_abort(struct ref_iterator *ref_iterator) +{ + base_ref_iterator_free(ref_iterator); + return ITER_DONE; +} + +static struct ref_iterator_vtable empty_ref_iterator_vtable = { + empty_ref_iterator_advance, + empty_ref_iterator_peel, + empty_ref_iterator_abort +}; + +struct ref_iterator *empty_ref_iterator_begin(void) +{ + struct empty_ref_iterator *iter = xcalloc(1, sizeof(*iter)); + struct ref_iterator *ref_iterator = &iter->base; + + base_ref_iterator_init(ref_iterator, &empty_ref_iterator_vtable); + return ref_iterator; +} + +int is_empty_ref_iterator(struct ref_iterator *ref_iterator) +{ + return ref_iterator->vtable == &empty_ref_iterator_vtable; +} + +struct merge_ref_iterator { + struct ref_iterator base; + + struct ref_iterator *iter0, *iter1; + + ref_iterator_select_fn *select; + void *cb_data; + + /* + * A pointer to iter0 or iter1 (whichever is supplying the + * current value), or NULL if advance has not yet been called. + */ + struct ref_iterator **current; +}; + +static int merge_ref_iterator_advance(struct ref_iterator *ref_iterator) +{ + struct merge_ref_iterator *iter = + (struct merge_ref_iterator *)ref_iterator; + int ok; + + if (!iter->current) { + /* Initialize: advance both iterators to their first entries */ + if ((ok = ref_iterator_advance(iter->iter0)) != ITER_OK) { + iter->iter0 = NULL; + if (ok == ITER_ERROR) + goto error; + } + if ((ok = ref_iterator_advance(iter->iter1)) != ITER_OK) { + iter->iter1 = NULL; + if (ok == ITER_ERROR) + goto error; + } + } else { + /* + * Advance the current iterator past the just-used + * entry: + */ + if ((ok = ref_iterator_advance(*iter->current)) != ITER_OK) { + *iter->current = NULL; + if (ok == ITER_ERROR) + goto error; + } + } + + /* Loop until we find an entry that we can yield. */ + while (1) { + struct ref_iterator **secondary; + enum iterator_selection selection = + iter->select(iter->iter0, iter->iter1, iter->cb_data); + + if (selection == ITER_SELECT_DONE) { + return ref_iterator_abort(ref_iterator); + } else if (selection == ITER_SELECT_ERROR) { + ref_iterator_abort(ref_iterator); + return ITER_ERROR; + } + + if ((selection & ITER_CURRENT_SELECTION_MASK) == 0) { + iter->current = &iter->iter0; + secondary = &iter->iter1; + } else { + iter->current = &iter->iter1; + secondary = &iter->iter0; + } + + if (selection & ITER_SKIP_SECONDARY) { + if ((ok = ref_iterator_advance(*secondary)) != ITER_OK) { + *secondary = NULL; + if (ok == ITER_ERROR) + goto error; + } + } + + if (selection & ITER_YIELD_CURRENT) { + iter->base.refname = (*iter->current)->refname; + iter->base.oid = (*iter->current)->oid; + iter->base.flags = (*iter->current)->flags; + return ITER_OK; + } + } + +error: + ref_iterator_abort(ref_iterator); + return ITER_ERROR; +} + +static int merge_ref_iterator_peel(struct ref_iterator *ref_iterator, + struct object_id *peeled) +{ + struct merge_ref_iterator *iter = + (struct merge_ref_iterator *)ref_iterator; + + if (!iter->current) { + die("BUG: peel called before advance for merge iterator"); + } + return ref_iterator_peel(*iter->current, peeled); +} + +static int merge_ref_iterator_abort(struct ref_iterator *ref_iterator) +{ + struct merge_ref_iterator *iter = + (struct merge_ref_iterator *)ref_iterator; + int ok = ITER_DONE; + + if (iter->iter0) { + if (ref_iterator_abort(iter->iter0) != ITER_DONE) + ok = ITER_ERROR; + } + if (iter->iter1) { + if (ref_iterator_abort(iter->iter1) != ITER_DONE) + ok = ITER_ERROR; + } + base_ref_iterator_free(ref_iterator); + return ok; +} + +static struct ref_iterator_vtable merge_ref_iterator_vtable = { + merge_ref_iterator_advance, + merge_ref_iterator_peel, + merge_ref_iterator_abort +}; + +struct ref_iterator *merge_ref_iterator_begin( + struct ref_iterator *iter0, struct ref_iterator *iter1, + ref_iterator_select_fn *select, void *cb_data) +{ + struct merge_ref_iterator *iter = xcalloc(1, sizeof(*iter)); + struct ref_iterator *ref_iterator = &iter->base; + + /* + * We can't do the same kind of is_empty_ref_iterator()-style + * optimization here as overlay_ref_iterator_begin() does, + * because we don't know the semantics of the select function. + * It might, for example, implement "intersect" by passing + * references through only if they exist in both iterators. + */ + + base_ref_iterator_init(ref_iterator, &merge_ref_iterator_vtable); + iter->iter0 = iter0; + iter->iter1 = iter1; + iter->select = select; + iter->cb_data = cb_data; + iter->current = NULL; + return ref_iterator; +} + +/* + * A ref_iterator_select_fn that overlays the items from front on top + * of those from back (like loose refs over packed refs). See + * overlay_ref_iterator_begin(). + */ +static enum iterator_selection overlay_iterator_select( + struct ref_iterator *front, struct ref_iterator *back, + void *cb_data) +{ + int cmp; + + if (!back) + return front ? ITER_SELECT_0 : ITER_SELECT_DONE; + else if (!front) + return ITER_SELECT_1; + + cmp = strcmp(front->refname, back->refname); + + if (cmp < 0) + return ITER_SELECT_0; + else if (cmp > 0) + return ITER_SELECT_1; + else + return ITER_SELECT_0_SKIP_1; +} + +struct ref_iterator *overlay_ref_iterator_begin( + struct ref_iterator *front, struct ref_iterator *back) +{ + /* + * Optimization: if one of the iterators is empty, return the + * other one rather than incurring the overhead of wrapping + * them. + */ + if (is_empty_ref_iterator(front)) { + ref_iterator_abort(front); + return back; + } else if (is_empty_ref_iterator(back)) { + ref_iterator_abort(back); + return front; + } + + return merge_ref_iterator_begin(front, back, + overlay_iterator_select, NULL); +} + +struct prefix_ref_iterator { + struct ref_iterator base; + + struct ref_iterator *iter0; + char *prefix; + int trim; +}; + +static int prefix_ref_iterator_advance(struct ref_iterator *ref_iterator) +{ + struct prefix_ref_iterator *iter = + (struct prefix_ref_iterator *)ref_iterator; + int ok; + + while ((ok = ref_iterator_advance(iter->iter0)) == ITER_OK) { + if (!starts_with(iter->iter0->refname, iter->prefix)) + continue; + + iter->base.refname = iter->iter0->refname + iter->trim; + iter->base.oid = iter->iter0->oid; + iter->base.flags = iter->iter0->flags; + return ITER_OK; + } + + iter->iter0 = NULL; + if (ref_iterator_abort(ref_iterator) != ITER_DONE) + return ITER_ERROR; + return ok; +} + +static int prefix_ref_iterator_peel(struct ref_iterator *ref_iterator, + struct object_id *peeled) +{ + struct prefix_ref_iterator *iter = + (struct prefix_ref_iterator *)ref_iterator; + + return ref_iterator_peel(iter->iter0, peeled); +} + +static int prefix_ref_iterator_abort(struct ref_iterator *ref_iterator) +{ + struct prefix_ref_iterator *iter = + (struct prefix_ref_iterator *)ref_iterator; + int ok = ITER_DONE; + + if (iter->iter0) + ok = ref_iterator_abort(iter->iter0); + free(iter->prefix); + base_ref_iterator_free(ref_iterator); + return ok; +} + +static struct ref_iterator_vtable prefix_ref_iterator_vtable = { + prefix_ref_iterator_advance, + prefix_ref_iterator_peel, + prefix_ref_iterator_abort +}; + +struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0, + const char *prefix, + int trim) +{ + struct prefix_ref_iterator *iter; + struct ref_iterator *ref_iterator; + + if (!*prefix && !trim) + return iter0; /* optimization: no need to wrap iterator */ + + iter = xcalloc(1, sizeof(*iter)); + ref_iterator = &iter->base; + + base_ref_iterator_init(ref_iterator, &prefix_ref_iterator_vtable); + + iter->iter0 = iter0; + iter->prefix = xstrdup(prefix); + iter->trim = trim; + + return ref_iterator; +} diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 8ad02d84df..fc2088ba57 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -248,6 +248,200 @@ int rename_ref_available(const char *oldname, const char *newname); /* Include broken references in a do_for_each_ref*() iteration: */ #define DO_FOR_EACH_INCLUDE_BROKEN 0x01 +/* + * Reference iterators + * + * A reference iterator encapsulates the state of an in-progress + * iteration over references. Create an instance of `struct + * ref_iterator` via one of the functions in this module. + * + * A freshly-created ref_iterator doesn't yet point at a reference. To + * advance the iterator, call ref_iterator_advance(). If successful, + * this sets the iterator's refname, oid, and flags fields to describe + * the next reference and returns ITER_OK. The data pointed at by + * refname and oid belong to the iterator; if you want to retain them + * after calling ref_iterator_advance() again or calling + * ref_iterator_abort(), you must make a copy. When the iteration has + * been exhausted, ref_iterator_advance() releases any resources + * assocated with the iteration, frees the ref_iterator object, and + * returns ITER_DONE. If you want to abort the iteration early, call + * ref_iterator_abort(), which also frees the ref_iterator object and + * any associated resources. If there was an internal error advancing + * to the next entry, ref_iterator_advance() aborts the iteration, + * frees the ref_iterator, and returns ITER_ERROR. + * + * The reference currently being looked at can be peeled by calling + * ref_iterator_peel(). This function is often faster than peel_ref(), + * so it should be preferred when iterating over references. + * + * Putting it all together, a typical iteration looks like this: + * + * int ok; + * struct ref_iterator *iter = ...; + * + * while ((ok = ref_iterator_advance(iter)) == ITER_OK) { + * if (want_to_stop_iteration()) { + * ok = ref_iterator_abort(iter); + * break; + * } + * + * // Access information about the current reference: + * if (!(iter->flags & REF_ISSYMREF)) + * printf("%s is %s\n", iter->refname, oid_to_hex(&iter->oid)); + * + * // If you need to peel the reference: + * ref_iterator_peel(iter, &oid); + * } + * + * if (ok != ITER_DONE) + * handle_error(); + */ +struct ref_iterator { + struct ref_iterator_vtable *vtable; + const char *refname; + const struct object_id *oid; + unsigned int flags; +}; + +/* + * Advance the iterator to the first or next item and return ITER_OK. + * If the iteration is exhausted, free the resources associated with + * the ref_iterator and return ITER_DONE. On errors, free the iterator + * resources and return ITER_ERROR. It is a bug to use ref_iterator or + * call this function again after it has returned ITER_DONE or + * ITER_ERROR. + */ +int ref_iterator_advance(struct ref_iterator *ref_iterator); + +/* + * If possible, peel the reference currently being viewed by the + * iterator. Return 0 on success. + */ +int ref_iterator_peel(struct ref_iterator *ref_iterator, + struct object_id *peeled); + +/* + * End the iteration before it has been exhausted, freeing the + * reference iterator and any associated resources and returning + * ITER_DONE. If the abort itself failed, return ITER_ERROR. + */ +int ref_iterator_abort(struct ref_iterator *ref_iterator); + +/* + * An iterator over nothing (its first ref_iterator_advance() call + * returns ITER_DONE). + */ +struct ref_iterator *empty_ref_iterator_begin(void); + +/* + * Return true iff ref_iterator is an empty_ref_iterator. + */ +int is_empty_ref_iterator(struct ref_iterator *ref_iterator); + +/* + * A callback function used to instruct merge_ref_iterator how to + * interleave the entries from iter0 and iter1. The function should + * return one of the constants defined in enum iterator_selection. It + * must not advance either of the iterators itself. + * + * The function must be prepared to handle the case that iter0 and/or + * iter1 is NULL, which indicates that the corresponding sub-iterator + * has been exhausted. Its return value must be consistent with the + * current states of the iterators; e.g., it must not return + * ITER_SKIP_1 if iter1 has already been exhausted. + */ +typedef enum iterator_selection ref_iterator_select_fn( + struct ref_iterator *iter0, struct ref_iterator *iter1, + void *cb_data); + +/* + * Iterate over the entries from iter0 and iter1, with the values + * interleaved as directed by the select function. The iterator takes + * ownership of iter0 and iter1 and frees them when the iteration is + * over. + */ +struct ref_iterator *merge_ref_iterator_begin( + struct ref_iterator *iter0, struct ref_iterator *iter1, + ref_iterator_select_fn *select, void *cb_data); + +/* + * An iterator consisting of the union of the entries from front and + * back. If there are entries common to the two sub-iterators, use the + * one from front. Each iterator must iterate over its entries in + * strcmp() order by refname for this to work. + * + * The new iterator takes ownership of its arguments and frees them + * when the iteration is over. As a convenience to callers, if front + * or back is an empty_ref_iterator, then abort that one immediately + * and return the other iterator directly, without wrapping it. + */ +struct ref_iterator *overlay_ref_iterator_begin( + struct ref_iterator *front, struct ref_iterator *back); + +/* + * Wrap iter0, only letting through the references whose names start + * with prefix. If trim is set, set iter->refname to the name of the + * reference with that many characters trimmed off the front; + * otherwise set it to the full refname. The new iterator takes over + * ownership of iter0 and frees it when iteration is over. It makes + * its own copy of prefix. + * + * As an convenience to callers, if prefix is the empty string and + * trim is zero, this function returns iter0 directly, without + * wrapping it. + */ +struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0, + const char *prefix, + int trim); + +/* + * Iterate over the packed and loose references in the specified + * submodule that are within find_containing_dir(prefix). If prefix is + * NULL or the empty string, iterate over all references in the + * submodule. + */ +struct ref_iterator *files_ref_iterator_begin(const char *submodule, + const char *prefix, + unsigned int flags); + +/* Internal implementation of reference iteration: */ + +/* + * Base class constructor for ref_iterators. Initialize the + * ref_iterator part of iter, setting its vtable pointer as specified. + * This is meant to be called only by the initializers of derived + * classes. + */ +void base_ref_iterator_init(struct ref_iterator *iter, + struct ref_iterator_vtable *vtable); + +/* + * Base class destructor for ref_iterators. Destroy the ref_iterator + * part of iter and shallow-free the object. This is meant to be + * called only by the destructors of derived classes. + */ +void base_ref_iterator_free(struct ref_iterator *iter); + +/* Virtual function declarations for ref_iterators: */ + +typedef int ref_iterator_advance_fn(struct ref_iterator *ref_iterator); + +typedef int ref_iterator_peel_fn(struct ref_iterator *ref_iterator, + struct object_id *peeled); + +/* + * Implementations of this function should free any resources specific + * to the derived class, then call base_ref_iterator_free() to clean + * up and free the ref_iterator object. + */ +typedef int ref_iterator_abort_fn(struct ref_iterator *ref_iterator); + +struct ref_iterator_vtable { + ref_iterator_advance_fn *advance; + ref_iterator_peel_fn *peel; + ref_iterator_abort_fn *abort; +}; + /* * Call fn for each reference in the specified submodule for which the * refname begins with prefix. If trim is non-zero, then trim that -- cgit v1.2.3 From 4c4de89573fa29b7f97e7a9a3d0674dbdb6f2a28 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 18 Jun 2016 06:15:16 +0200 Subject: do_for_each_ref(): reimplement using reference iteration Use the reference iterator interface to implement do_for_each_ref(). Delete a bunch of code supporting the old for_each_ref() implementation. And now that do_for_each_ref() is generic code (it is no longer tied to the files backend), move it to refs.c. The implementation is via a new function, do_for_each_ref_iterator(), which takes a reference iterator as argument and calls a callback function for each of the references in the iterator. This change requires the current_ref performance hack for peel_ref() to be implemented via ref_iterator_peel() rather than peel_entry() because we don't have a ref_entry handy (it is hidden under three layers: file_ref_iterator, merge_ref_iterator, and cache_ref_iterator). So: * do_for_each_ref_iterator() records the active iterator in current_ref_iter while it is running. * peel_ref() checks whether current_ref_iter is pointing at the requested reference. If so, it asks the iterator to peel the reference (which it can do efficiently via its "peel" virtual function). For extra safety, we do the optimization only if the refname *addresses* are the same, not only if the refname *strings* are the same, to forestall possible mixups between refnames that come from different ref_iterators. Please note that this optimization of peel_ref() is only available when iterating via do_for_each_ref_iterator() (including all of the for_each_ref() functions, which call it indirectly). It would be complicated to implement a similar optimization when iterating directly using a reference iterator, because multiple reference iterators can be in use at the same time, with interleaved calls to ref_iterator_advance(). (In fact we do exactly that in merge_ref_iterator.) But that is not necessary. peel_ref() is only called while iterating over references. Callers who iterate using the for_each_ref() functions benefit from the optimization described above. Callers who iterate using reference iterators directly have access to the ref_iterator, so they can call ref_iterator_peel() themselves to get an analogous optimization in a more straightforward manner. If we rewrite all callers to use the reference iteration API, then we can remove the current_ref_iter hack permanently. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 20 +++++ refs/files-backend.c | 206 ++------------------------------------------------- refs/iterator.c | 29 ++++++++ refs/refs-internal.h | 35 ++++++--- 4 files changed, 77 insertions(+), 213 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs.c b/refs.c index 842c5c7b05..814cad3163 100644 --- a/refs.c +++ b/refs.c @@ -1120,6 +1120,26 @@ int head_ref(each_ref_fn fn, void *cb_data) return head_ref_submodule(NULL, fn, cb_data); } +/* + * Call fn for each reference in the specified submodule for which the + * refname begins with prefix. If trim is non-zero, then trim that + * many characters off the beginning of each refname before passing + * the refname to fn. flags can be DO_FOR_EACH_INCLUDE_BROKEN to + * include broken references in the iteration. If fn ever returns a + * non-zero value, stop the iteration and return that value; + * otherwise, return 0. + */ +static int do_for_each_ref(const char *submodule, const char *prefix, + each_ref_fn fn, int trim, int flags, void *cb_data) +{ + struct ref_iterator *iter; + + iter = files_ref_iterator_begin(submodule, prefix, flags); + iter = prefix_ref_iterator_begin(iter, prefix, trim); + + return do_for_each_ref_iterator(iter, fn, cb_data); +} + int for_each_ref(each_ref_fn fn, void *cb_data) { return do_for_each_ref(NULL, "", fn, 0, 0, cb_data); diff --git a/refs/files-backend.c b/refs/files-backend.c index 395056b675..4232da8cb7 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -542,53 +542,8 @@ static int entry_resolves_to_object(struct ref_entry *entry) &entry->u.value.oid, entry->flag); } -/* - * current_ref is a performance hack: when iterating over references - * using the for_each_ref*() functions, current_ref is set to the - * current reference's entry before calling the callback function. If - * the callback function calls peel_ref(), then peel_ref() first - * checks whether the reference to be peeled is the current reference - * (it usually is) and if so, returns that reference's peeled version - * if it is available. This avoids a refname lookup in a common case. - */ -static struct ref_entry *current_ref; - typedef int each_ref_entry_fn(struct ref_entry *entry, void *cb_data); -struct ref_entry_cb { - const char *prefix; - int trim; - int flags; - each_ref_fn *fn; - void *cb_data; -}; - -/* - * Handle one reference in a do_for_each_ref*()-style iteration, - * calling an each_ref_fn for each entry. - */ -static int do_one_ref(struct ref_entry *entry, void *cb_data) -{ - struct ref_entry_cb *data = cb_data; - struct ref_entry *old_current_ref; - int retval; - - if (!starts_with(entry->name, data->prefix)) - return 0; - - if (!(data->flags & DO_FOR_EACH_INCLUDE_BROKEN) && - !entry_resolves_to_object(entry)) - return 0; - - /* Store the old value, in case this is a recursive call: */ - old_current_ref = current_ref; - current_ref = entry; - retval = data->fn(entry->name + data->trim, &entry->u.value.oid, - entry->flag, data->cb_data); - current_ref = old_current_ref; - return retval; -} - /* * Call fn for each reference in dir that has index in the range * offset <= index < dir->nr. Recurse into subdirectories that are in @@ -617,78 +572,6 @@ static int do_for_each_entry_in_dir(struct ref_dir *dir, int offset, return 0; } -/* - * Call fn for each reference in the union of dir1 and dir2, in order - * by refname. Recurse into subdirectories. If a value entry appears - * in both dir1 and dir2, then only process the version that is in - * dir2. The input dirs must already be sorted, but subdirs will be - * sorted as needed. fn is called for all references, including - * broken ones. - */ -static int do_for_each_entry_in_dirs(struct ref_dir *dir1, - struct ref_dir *dir2, - each_ref_entry_fn fn, void *cb_data) -{ - int retval; - int i1 = 0, i2 = 0; - - assert(dir1->sorted == dir1->nr); - assert(dir2->sorted == dir2->nr); - while (1) { - struct ref_entry *e1, *e2; - int cmp; - if (i1 == dir1->nr) { - return do_for_each_entry_in_dir(dir2, i2, fn, cb_data); - } - if (i2 == dir2->nr) { - return do_for_each_entry_in_dir(dir1, i1, fn, cb_data); - } - e1 = dir1->entries[i1]; - e2 = dir2->entries[i2]; - cmp = strcmp(e1->name, e2->name); - if (cmp == 0) { - if ((e1->flag & REF_DIR) && (e2->flag & REF_DIR)) { - /* Both are directories; descend them in parallel. */ - struct ref_dir *subdir1 = get_ref_dir(e1); - struct ref_dir *subdir2 = get_ref_dir(e2); - sort_ref_dir(subdir1); - sort_ref_dir(subdir2); - retval = do_for_each_entry_in_dirs( - subdir1, subdir2, fn, cb_data); - i1++; - i2++; - } else if (!(e1->flag & REF_DIR) && !(e2->flag & REF_DIR)) { - /* Both are references; ignore the one from dir1. */ - retval = fn(e2, cb_data); - i1++; - i2++; - } else { - die("conflict between reference and directory: %s", - e1->name); - } - } else { - struct ref_entry *e; - if (cmp < 0) { - e = e1; - i1++; - } else { - e = e2; - i2++; - } - if (e->flag & REF_DIR) { - struct ref_dir *subdir = get_ref_dir(e); - sort_ref_dir(subdir); - retval = do_for_each_entry_in_dir( - subdir, 0, fn, cb_data); - } else { - retval = fn(e, cb_data); - } - } - if (retval) - return retval; - } -} - /* * Load all of the refs from the dir into our in-memory cache. The hard work * of loading loose refs is done by get_ref_dir(), so we just need to recurse @@ -1959,11 +1842,12 @@ int peel_ref(const char *refname, unsigned char *sha1) int flag; unsigned char base[20]; - if (current_ref && (current_ref->name == refname - || !strcmp(current_ref->name, refname))) { - if (peel_entry(current_ref, 0)) + if (current_ref_iter && current_ref_iter->refname == refname) { + struct object_id peeled; + + if (ref_iterator_peel(current_ref_iter, &peeled)) return -1; - hashcpy(sha1, current_ref->u.value.peeled.hash); + hashcpy(sha1, peeled.hash); return 0; } @@ -2124,86 +2008,6 @@ struct ref_iterator *files_ref_iterator_begin( return ref_iterator; } -/* - * Call fn for each reference in the specified ref_cache, omitting - * references not in the containing_dir of prefix. Call fn for all - * references, including broken ones. If fn ever returns a non-zero - * value, stop the iteration and return that value; otherwise, return - * 0. - */ -static int do_for_each_entry(struct ref_cache *refs, const char *prefix, - each_ref_entry_fn fn, void *cb_data) -{ - struct packed_ref_cache *packed_ref_cache; - struct ref_dir *loose_dir; - struct ref_dir *packed_dir; - int retval = 0; - - /* - * We must make sure that all loose refs are read before accessing the - * packed-refs file; this avoids a race condition in which loose refs - * are migrated to the packed-refs file by a simultaneous process, but - * our in-memory view is from before the migration. get_packed_ref_cache() - * takes care of making sure our view is up to date with what is on - * disk. - */ - loose_dir = get_loose_refs(refs); - if (prefix && *prefix) { - loose_dir = find_containing_dir(loose_dir, prefix, 0); - } - if (loose_dir) - prime_ref_dir(loose_dir); - - packed_ref_cache = get_packed_ref_cache(refs); - acquire_packed_ref_cache(packed_ref_cache); - packed_dir = get_packed_ref_dir(packed_ref_cache); - if (prefix && *prefix) { - packed_dir = find_containing_dir(packed_dir, prefix, 0); - } - - if (packed_dir && loose_dir) { - sort_ref_dir(packed_dir); - sort_ref_dir(loose_dir); - retval = do_for_each_entry_in_dirs( - packed_dir, loose_dir, fn, cb_data); - } else if (packed_dir) { - sort_ref_dir(packed_dir); - retval = do_for_each_entry_in_dir( - packed_dir, 0, fn, cb_data); - } else if (loose_dir) { - sort_ref_dir(loose_dir); - retval = do_for_each_entry_in_dir( - loose_dir, 0, fn, cb_data); - } - - release_packed_ref_cache(packed_ref_cache); - return retval; -} - -int do_for_each_ref(const char *submodule, const char *prefix, - each_ref_fn fn, int trim, int flags, void *cb_data) -{ - struct ref_entry_cb data; - struct ref_cache *refs; - - refs = get_ref_cache(submodule); - if (!refs) - return 0; - - data.prefix = prefix; - data.trim = trim; - data.flags = flags; - data.fn = fn; - data.cb_data = cb_data; - - if (ref_paranoia < 0) - ref_paranoia = git_env_bool("GIT_REF_PARANOIA", 0); - if (ref_paranoia) - data.flags |= DO_FOR_EACH_INCLUDE_BROKEN; - - return do_for_each_entry(refs, prefix, do_one_ref, &data); -} - /* * 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 diff --git a/refs/iterator.c b/refs/iterator.c index 93ba472f91..bce1f192f7 100644 --- a/refs/iterator.c +++ b/refs/iterator.c @@ -353,3 +353,32 @@ struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0, return ref_iterator; } + +struct ref_iterator *current_ref_iter = NULL; + +int do_for_each_ref_iterator(struct ref_iterator *iter, + each_ref_fn fn, void *cb_data) +{ + int retval = 0, ok; + struct ref_iterator *old_ref_iter = current_ref_iter; + + current_ref_iter = iter; + while ((ok = ref_iterator_advance(iter)) == ITER_OK) { + retval = fn(iter->refname, iter->oid, iter->flags, cb_data); + if (retval) { + /* + * If ref_iterator_abort() returns ITER_ERROR, + * we ignore that error in deference to the + * callback function's return value. + */ + ref_iterator_abort(iter); + goto out; + } + } + +out: + current_ref_iter = old_ref_iter; + if (ok == ITER_ERROR) + return -1; + return retval; +} diff --git a/refs/refs-internal.h b/refs/refs-internal.h index fc2088ba57..f73e022f0e 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -443,18 +443,29 @@ struct ref_iterator_vtable { }; /* - * Call fn for each reference in the specified submodule for which the - * refname begins with prefix. If trim is non-zero, then trim that - * many characters off the beginning of each refname before passing - * the refname to fn. flags can be DO_FOR_EACH_INCLUDE_BROKEN to - * include broken references in the iteration. If fn ever returns a - * non-zero value, stop the iteration and return that value; - * otherwise, return 0. - * - * This is the common backend for the for_each_*ref* functions. - */ -int do_for_each_ref(const char *submodule, const char *prefix, - each_ref_fn fn, int trim, int flags, void *cb_data); + * current_ref_iter is a performance hack: when iterating over + * references using the for_each_ref*() functions, current_ref_iter is + * set to the reference iterator before calling the callback function. + * If the callback function calls peel_ref(), then peel_ref() first + * checks whether the reference to be peeled is the one referred to by + * the iterator (it usually is) and if so, asks the iterator for the + * peeled version of the reference if it is available. This avoids a + * refname lookup in a common case. current_ref_iter is set to NULL + * when the iteration is over. + */ +extern struct ref_iterator *current_ref_iter; + +/* + * The common backend for the for_each_*ref* functions. Call fn for + * each reference in iter. If the iterator itself ever returns + * ITER_ERROR, return -1. If fn ever returns a non-zero value, stop + * the iteration and return that value. Otherwise, return 0. In any + * case, free the iterator when done. This function is basically an + * adapter between the callback style of reference iteration and the + * iterator style. + */ +int do_for_each_ref_iterator(struct ref_iterator *iter, + each_ref_fn fn, void *cb_data); /* * Read the specified reference from the filesystem or packed refs -- cgit v1.2.3 From d24b21e9fcaa9ed4b7966275a8d82406f578577d Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 18 Jun 2016 06:15:17 +0200 Subject: for_each_reflog(): don't abort for bad references If there is a file under "$GIT_DIR/logs" with no corresponding reference, the old code was emitting an error message, aborting the reflog iteration, and returning -1. But * None of the callers was checking the exit value * The callers all want to find all legitimate reflogs (sometimes for the purpose of determining object reachability!) and wouldn't benefit from a truncated iteration anyway. So instead, emit an error message and skip the "broken" reflog, but continue with the iteration. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/files-backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 4232da8cb7..ab40db3326 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -3324,7 +3324,7 @@ static int do_for_each_reflog(struct strbuf *name, each_ref_fn fn, void *cb_data struct object_id oid; if (read_ref_full(name->buf, 0, oid.hash, NULL)) - retval = error("bad ref for %s", name->buf); + error("bad ref for %s", name->buf); else retval = fn(name->buf, &oid, 0, cb_data); } -- cgit v1.2.3 From 2880d16f09635f9d43247b27fd7e6508b992e599 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sat, 18 Jun 2016 06:15:19 +0200 Subject: for_each_reflog(): reimplement using iterators Allow references with reflogs to be iterated over using a ref_iterator. The latter is implemented as a files_reflog_iterator, which in turn uses dir_iterator to read the "logs" directory. Note that reflog iteration doesn't correctly handle per-worktree reflogs (either before or after this patch). Signed-off-by: Ramsay Jones Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/files-backend.c | 113 ++++++++++++++++++++++++++++++++------------------- refs/refs-internal.h | 7 ++++ 2 files changed, 78 insertions(+), 42 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index ab40db3326..a9ca003bae 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2,6 +2,7 @@ #include "../refs.h" #include "refs-internal.h" #include "../iterator.h" +#include "../dir-iterator.h" #include "../lockfile.h" #include "../object.h" #include "../dir.h" @@ -3291,60 +3292,88 @@ int for_each_reflog_ent(const char *refname, each_reflog_ent_fn fn, void *cb_dat strbuf_release(&sb); return ret; } -/* - * Call fn for each reflog in the namespace indicated by name. name - * must be empty or end with '/'. Name will be used as a scratch - * space, but its contents will be restored before return. - */ -static int do_for_each_reflog(struct strbuf *name, each_ref_fn fn, void *cb_data) -{ - DIR *d = opendir(git_path("logs/%s", name->buf)); - int retval = 0; - struct dirent *de; - int oldlen = name->len; - if (!d) - return name->len ? errno : 0; +struct files_reflog_iterator { + struct ref_iterator base; - while ((de = readdir(d)) != NULL) { - struct stat st; + struct dir_iterator *dir_iterator; + struct object_id oid; +}; - if (de->d_name[0] == '.') +static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator) +{ + struct files_reflog_iterator *iter = + (struct files_reflog_iterator *)ref_iterator; + struct dir_iterator *diter = iter->dir_iterator; + int ok; + + while ((ok = dir_iterator_advance(diter)) == ITER_OK) { + int flags; + + if (!S_ISREG(diter->st.st_mode)) continue; - if (ends_with(de->d_name, ".lock")) + if (diter->basename[0] == '.') + continue; + if (ends_with(diter->basename, ".lock")) continue; - strbuf_addstr(name, de->d_name); - if (stat(git_path("logs/%s", name->buf), &st) < 0) { - ; /* silently ignore */ - } else { - if (S_ISDIR(st.st_mode)) { - strbuf_addch(name, '/'); - retval = do_for_each_reflog(name, fn, cb_data); - } else { - struct object_id oid; - if (read_ref_full(name->buf, 0, oid.hash, NULL)) - error("bad ref for %s", name->buf); - else - retval = fn(name->buf, &oid, 0, cb_data); - } - if (retval) - break; + if (read_ref_full(diter->relative_path, 0, + iter->oid.hash, &flags)) { + error("bad ref for %s", diter->path.buf); + continue; } - strbuf_setlen(name, oldlen); + + iter->base.refname = diter->relative_path; + iter->base.oid = &iter->oid; + iter->base.flags = flags; + return ITER_OK; } - closedir(d); - return retval; + + iter->dir_iterator = NULL; + if (ref_iterator_abort(ref_iterator) == ITER_ERROR) + ok = ITER_ERROR; + return ok; +} + +static int files_reflog_iterator_peel(struct ref_iterator *ref_iterator, + struct object_id *peeled) +{ + die("BUG: ref_iterator_peel() called for reflog_iterator"); +} + +static int files_reflog_iterator_abort(struct ref_iterator *ref_iterator) +{ + struct files_reflog_iterator *iter = + (struct files_reflog_iterator *)ref_iterator; + int ok = ITER_DONE; + + if (iter->dir_iterator) + ok = dir_iterator_abort(iter->dir_iterator); + + base_ref_iterator_free(ref_iterator); + return ok; +} + +static struct ref_iterator_vtable files_reflog_iterator_vtable = { + files_reflog_iterator_advance, + files_reflog_iterator_peel, + files_reflog_iterator_abort +}; + +struct ref_iterator *files_reflog_iterator_begin(void) +{ + struct files_reflog_iterator *iter = xcalloc(1, sizeof(*iter)); + struct ref_iterator *ref_iterator = &iter->base; + + base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable); + iter->dir_iterator = dir_iterator_begin(git_path("logs")); + return ref_iterator; } int for_each_reflog(each_ref_fn fn, void *cb_data) { - int retval; - struct strbuf name; - strbuf_init(&name, PATH_MAX); - retval = do_for_each_reflog(&name, fn, cb_data); - strbuf_release(&name); - return retval; + return do_for_each_ref_iterator(files_reflog_iterator_begin(), + fn, cb_data); } static int ref_update_reject_duplicates(struct string_list *refnames, diff --git a/refs/refs-internal.h b/refs/refs-internal.h index f73e022f0e..efe584701b 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -404,6 +404,13 @@ struct ref_iterator *files_ref_iterator_begin(const char *submodule, const char *prefix, unsigned int flags); +/* + * Iterate over the references in the main ref_store that have a + * reflog. The paths within a directory are iterated over in arbitrary + * order. + */ +struct ref_iterator *files_reflog_iterator_begin(void); + /* Internal implementation of reference iteration: */ /* -- cgit v1.2.3 From e3f510393c9d373f2969badc2b8afe179803a0fa Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Tue, 7 Jun 2016 09:29:23 +0200 Subject: lock_ref_for_update(): make error handling more uniform To aid the effort, extract a new function, check_old_oid(), and use it in the two places where the read value of the reference has to be checked against update->old_sha1. Update tests to reflect the improvements. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/files-backend.c | 74 +++++++++++++++++++++++++------------------- t/t1404-update-ref-errors.sh | 14 ++++----- 2 files changed, 49 insertions(+), 39 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index bbf96ad83a..6f8fecd024 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -3388,6 +3388,38 @@ static const char *original_update_refname(struct ref_update *update) return update->refname; } +/* + * Check whether the REF_HAVE_OLD and old_oid values stored in update + * are consistent with oid, which is the reference's current value. If + * everything is OK, return 0; otherwise, write an error message to + * err and return -1. + */ +static int check_old_oid(struct ref_update *update, struct object_id *oid, + struct strbuf *err) +{ + if (!(update->flags & REF_HAVE_OLD) || + !hashcmp(oid->hash, update->old_sha1)) + return 0; + + if (is_null_sha1(update->old_sha1)) + strbuf_addf(err, "cannot lock ref '%s': " + "reference already exists", + original_update_refname(update)); + else if (is_null_oid(oid)) + strbuf_addf(err, "cannot lock ref '%s': " + "reference is missing but expected %s", + original_update_refname(update), + sha1_to_hex(update->old_sha1)); + else + strbuf_addf(err, "cannot lock ref '%s': " + "is at %s but expected %s", + original_update_refname(update), + oid_to_hex(oid), + sha1_to_hex(update->old_sha1)); + + return -1; +} + /* * Prepare for carrying out update: * - Lock the reference referred to by update. @@ -3433,7 +3465,7 @@ static int lock_ref_for_update(struct ref_update *update, reason = strbuf_detach(err, NULL); strbuf_addf(err, "cannot lock ref '%s': %s", - update->refname, reason); + original_update_refname(update), reason); free(reason); return ret; } @@ -3447,28 +3479,17 @@ static int lock_ref_for_update(struct ref_update *update, * 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, + if (read_ref_full(update->refname, 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); + "error reading reference", + original_update_refname(update)); + return -1; } - } - 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)); + } else if (check_old_oid(update, &lock->old_oid, err)) { return TRANSACTION_GENERIC_ERROR; } - } else { /* * Create a new update for the reference this @@ -3485,6 +3506,9 @@ static int lock_ref_for_update(struct ref_update *update, } else { struct ref_update *parent_update; + if (check_old_oid(update, &lock->old_oid, err)) + return TRANSACTION_GENERIC_ERROR; + /* * If this update is happening indirectly because of a * symref update, record the old SHA-1 in the parent @@ -3495,20 +3519,6 @@ static int lock_ref_for_update(struct ref_update *update, parent_update = parent_update->parent_update) { oidcpy(&parent_update->lock->old_oid, &lock->old_oid); } - - 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) && @@ -3530,7 +3540,7 @@ static int lock_ref_for_update(struct ref_update *update, */ update->lock = NULL; strbuf_addf(err, - "cannot update the ref '%s': %s", + "cannot update ref '%s': %s", update->refname, write_err); free(write_err); return TRANSACTION_GENERIC_ERROR; diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh index 5234b41f14..c34ece48f5 100755 --- a/t/t1404-update-ref-errors.sh +++ b/t/t1404-update-ref-errors.sh @@ -237,7 +237,7 @@ test_expect_success 'missing old value blocks indirect update' ' prefix=refs/missing-indirect-update && git symbolic-ref $prefix/symref $prefix/foo && cat >expected <<-EOF && - fatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q + fatal: cannot lock ref $Q$prefix/symref$Q: unable to resolve reference $Q$prefix/foo$Q EOF printf "%s\n" "update $prefix/symref $E $D" | test_must_fail git update-ref --stdin 2>output.err && @@ -284,7 +284,7 @@ test_expect_success 'missing old value blocks indirect no-deref update' ' prefix=refs/missing-noderef-update && git symbolic-ref $prefix/symref $prefix/foo && cat >expected <<-EOF && - fatal: cannot lock ref $Q$prefix/symref$Q: can${Q}t resolve old value + fatal: cannot lock ref $Q$prefix/symref$Q: reference is missing but expected $D EOF printf "%s\n" "option no-deref" "update $prefix/symref $E $D" | test_must_fail git update-ref --stdin 2>output.err && @@ -303,7 +303,7 @@ test_expect_success 'incorrect old value blocks indirect no-deref update' ' test_cmp expected output.err ' -test_expect_failure 'existing old value blocks indirect no-deref create' ' +test_expect_success 'existing old value blocks indirect no-deref create' ' prefix=refs/existing-noderef-create && git symbolic-ref $prefix/symref $prefix/foo && git update-ref $prefix/foo $C && @@ -372,13 +372,13 @@ test_expect_success 'non-empty directory blocks indirect create' ' : >.git/$prefix/foo/bar/baz.lock && test_when_finished "rm -f .git/$prefix/foo/bar/baz.lock" && cat >expected <<-EOF && - fatal: cannot lock ref $Q$prefix/foo$Q: there is a non-empty directory $Q.git/$prefix/foo$Q blocking reference $Q$prefix/foo$Q + fatal: cannot lock ref $Q$prefix/symref$Q: there is a non-empty directory $Q.git/$prefix/foo$Q blocking reference $Q$prefix/foo$Q EOF printf "%s\n" "update $prefix/symref $C" | test_must_fail git update-ref --stdin 2>output.err && test_cmp expected output.err && cat >expected <<-EOF && - fatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q + fatal: cannot lock ref $Q$prefix/symref$Q: unable to resolve reference $Q$prefix/foo$Q EOF printf "%s\n" "update $prefix/symref $D $C" | test_must_fail git update-ref --stdin 2>output.err && @@ -391,13 +391,13 @@ test_expect_success 'broken reference blocks indirect create' ' echo "gobbledigook" >.git/$prefix/foo && test_when_finished "rm -f .git/$prefix/foo" && cat >expected <<-EOF && - fatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q: reference broken + fatal: cannot lock ref $Q$prefix/symref$Q: unable to resolve reference $Q$prefix/foo$Q: reference broken EOF printf "%s\n" "update $prefix/symref $C" | test_must_fail git update-ref --stdin 2>output.err && test_cmp expected output.err && cat >expected <<-EOF && - fatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q: reference broken + fatal: cannot lock ref $Q$prefix/symref$Q: unable to resolve reference $Q$prefix/foo$Q: reference broken EOF printf "%s\n" "update $prefix/symref $D $C" | test_must_fail git update-ref --stdin 2>output.err && -- cgit v1.2.3 From 841caad903f2b160e9f5ff05f961d20ad9085ddc Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Tue, 7 Jun 2016 09:32:08 +0200 Subject: lock_ref_for_update(): avoid a symref resolution If we're overwriting a symref with a SHA-1, we need to resolve the value of the symref (1) to check against update->old_sha1 and (2) to write to its reflog. However, we've already read the symref itself and know its referent. So there is no need to read the symref's value through the symref; we can read the referent directly. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/files-backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 6f8fecd024..769e5c4100 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -3479,7 +3479,7 @@ static int lock_ref_for_update(struct ref_update *update, * the transaction, so we have to read it here * to record and possibly check old_sha1: */ - if (read_ref_full(update->refname, 0, + if (read_ref_full(referent.buf, 0, lock->old_oid.hash, NULL)) { if (update->flags & REF_HAVE_OLD) { strbuf_addf(err, "cannot lock ref '%s': " -- cgit v1.2.3 From c368dde9245fa3d50b7e01e1ff4f9e5c12c718da Mon Sep 17 00:00:00 2001 From: "brian m. carlson" Date: Fri, 24 Jun 2016 23:09:22 +0000 Subject: coccinelle: apply object_id Coccinelle transformations Apply the set of semantic patches from contrib/coccinelle to convert some leftover places using struct object_id's hash member to instead use the wrapper functions that take struct object_id natively. Signed-off-by: brian m. carlson Signed-off-by: Junio C Hamano --- bisect.c | 2 +- builtin/merge.c | 13 ++++++------- refs/files-backend.c | 4 ++-- 3 files changed, 9 insertions(+), 10 deletions(-) (limited to 'refs/files-backend.c') diff --git a/bisect.c b/bisect.c index 6d93edbcb9..ff147589c7 100644 --- a/bisect.c +++ b/bisect.c @@ -754,7 +754,7 @@ static void handle_bad_merge_base(void) static void handle_skipped_merge_base(const unsigned char *mb) { char *mb_hex = sha1_to_hex(mb); - char *bad_hex = sha1_to_hex(current_bad_oid->hash); + char *bad_hex = oid_to_hex(current_bad_oid); char *good_hex = join_sha1_array_hex(&good_revs, ' '); warning("the merge base between %s and [%s] " diff --git a/builtin/merge.c b/builtin/merge.c index a9b99c9ffc..f66d06ce25 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -501,7 +501,7 @@ static void merge_name(const char *remote, struct strbuf *msg) if (ref_exists(truname.buf)) { strbuf_addf(msg, "%s\t\tbranch '%s'%s of .\n", - sha1_to_hex(remote_head->object.oid.hash), + oid_to_hex(&remote_head->object.oid), truname.buf + 11, (early ? " (early part)" : "")); strbuf_release(&truname); @@ -515,7 +515,7 @@ static void merge_name(const char *remote, struct strbuf *msg) desc = merge_remote_util(remote_head); if (desc && desc->obj && desc->obj->type == OBJ_TAG) { strbuf_addf(msg, "%s\t\t%s '%s'\n", - sha1_to_hex(desc->obj->oid.hash), + oid_to_hex(&desc->obj->oid), typename(desc->obj->type), remote); goto cleanup; @@ -523,7 +523,7 @@ static void merge_name(const char *remote, struct strbuf *msg) } strbuf_addf(msg, "%s\t\tcommit '%s'\n", - sha1_to_hex(remote_head->object.oid.hash), remote); + oid_to_hex(&remote_head->object.oid), remote); cleanup: strbuf_release(&buf); strbuf_release(&bname); @@ -1366,7 +1366,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) for (p = remoteheads; p; p = p->next) { struct commit *commit = p->item; strbuf_addf(&buf, "GITHEAD_%s", - sha1_to_hex(commit->object.oid.hash)); + oid_to_hex(&commit->object.oid)); setenv(buf.buf, merge_remote_util(commit)->name, 1); strbuf_reset(&buf); if (fast_forward != FF_ONLY && @@ -1425,7 +1425,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) goto done; } else if (fast_forward != FF_NO && !remoteheads->next && !common->next && - !hashcmp(common->item->object.oid.hash, head_commit->object.oid.hash)) { + !oidcmp(&common->item->object.oid, &head_commit->object.oid)) { /* Again the most common case of merging one remote. */ struct strbuf msg = STRBUF_INIT; struct commit *commit; @@ -1499,8 +1499,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) * HEAD^^" would be missed. */ common_one = get_merge_bases(head_commit, j->item); - if (hashcmp(common_one->item->object.oid.hash, - j->item->object.oid.hash)) { + if (oidcmp(&common_one->item->object.oid, &j->item->object.oid)) { up_to_date = 0; break; } diff --git a/refs/files-backend.c b/refs/files-backend.c index 1f38076411..dac3a22297 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1725,14 +1725,14 @@ static int verify_lock(struct ref_lock *lock, errno = save_errno; return -1; } else { - hashclr(lock->old_oid.hash); + oidclr(&lock->old_oid); return 0; } } if (old_sha1 && hashcmp(lock->old_oid.hash, old_sha1)) { strbuf_addf(err, "ref %s is at %s but expected %s", lock->ref_name, - sha1_to_hex(lock->old_oid.hash), + oid_to_hex(&lock->old_oid), sha1_to_hex(old_sha1)); errno = EBUSY; return -1; -- cgit v1.2.3