From: Junio C Hamano Date: Wed, 13 May 2015 04:26:09 +0000 (-0700) Subject: Merge branch 'mh/write-refs-sooner-2.2' into mh/write-refs-sooner-2.3 X-Git-Tag: v2.4.3~13^2^2 X-Git-Url: https://git.lorimer.id.au/gitweb.git/diff_plain/4ec6591dd7bb055a74f259ad4c360ecd8ded5f35?hp=-c Merge branch 'mh/write-refs-sooner-2.2' into mh/write-refs-sooner-2.3 * mh/write-refs-sooner-2.2: ref_transaction_commit(): fix atomicity and avoid fd exhaustion ref_transaction_commit(): remove the local flags variable ref_transaction_commit(): inline call to write_ref_sha1() rename_ref(): inline calls to write_ref_sha1() from this function commit_ref_update(): new function, extracted from write_ref_sha1() write_ref_to_lockfile(): new function, extracted from write_ref_sha1() t7004: rename ULIMIT test prerequisite to ULIMIT_STACK_SIZE update-ref: test handling large transactions properly --- 4ec6591dd7bb055a74f259ad4c360ecd8ded5f35 diff --combined refs.c index 3a26ad4e65,85c1dcb017..c689af6ccf --- a/refs.c +++ b/refs.c @@@ -30,6 -30,13 +30,13 @@@ static unsigned char refname_dispositio * pruned. */ #define REF_ISPRUNING 0x0100 + + /* + * Used as a flag in ref_update::flags when the lockfile needs to be + * committed. + */ + #define REF_NEEDS_COMMIT 0x0200 + /* * Try to read one refname component from the front of refname. * Return the length of the component found, or -1 if the component is @@@ -1618,7 -1625,8 +1625,7 @@@ const char *resolve_ref_unsafe(const ch char *resolve_refdup(const char *ref, int resolve_flags, unsigned char *sha1, int *flags) { - const char *ret = resolve_ref_unsafe(ref, resolve_flags, sha1, flags); - return ret ? xstrdup(ret) : NULL; + return xstrdup_or_null(resolve_ref_unsafe(ref, resolve_flags, sha1, flags)); } /* The argument to filter_refs */ @@@ -1907,11 -1915,6 +1914,11 @@@ static int do_for_each_ref(struct ref_c 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, base, do_one_ref, &data); } @@@ -2326,7 -2329,6 +2333,7 @@@ static struct ref_lock *lock_ref_sha1_b lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags); if (lock->lock_fd < 0) { + last_errno = errno; if (errno == ENOENT && --attempts_remaining > 0) /* * Maybe somebody just deleted one of the @@@ -2334,13 -2336,8 +2341,13 @@@ * again: */ goto retry; - else - unable_to_lock_die(ref_file, errno); + else { + struct strbuf err = STRBUF_INIT; + unable_to_lock_message(ref_file, errno, &err); + error("%s", err.buf); + strbuf_release(&err); + goto error_return; + } } return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock; @@@ -2596,24 -2593,79 +2603,24 @@@ int pack_refs(unsigned int flags return 0; } -/* - * If entry is no longer needed in packed-refs, add it to the string - * list pointed to by cb_data. Reasons for deleting entries: - * - * - Entry is broken. - * - Entry is overridden by a loose ref. - * - Entry does not point at a valid object. - * - * In the first and third cases, also emit an error message because these - * are indications of repository corruption. - */ -static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data) -{ - struct string_list *refs_to_delete = cb_data; - - if (entry->flag & REF_ISBROKEN) { - /* This shouldn't happen to packed refs. */ - error("%s is broken!", entry->name); - string_list_append(refs_to_delete, entry->name); - return 0; - } - if (!has_sha1_file(entry->u.value.sha1)) { - unsigned char sha1[20]; - int flags; - - if (read_ref_full(entry->name, 0, sha1, &flags)) - /* We should at least have found the packed ref. */ - die("Internal error"); - if ((flags & REF_ISSYMREF) || !(flags & REF_ISPACKED)) { - /* - * This packed reference is overridden by a - * loose reference, so it is OK that its value - * is no longer valid; for example, it might - * refer to an object that has been garbage - * collected. For this purpose we don't even - * care whether the loose reference itself is - * invalid, broken, symbolic, etc. Silently - * remove the packed reference. - */ - string_list_append(refs_to_delete, entry->name); - return 0; - } - /* - * There is no overriding loose reference, so the fact - * that this reference doesn't refer to a valid object - * indicates some kind of repository corruption. - * Report the problem, then omit the reference from - * the output. - */ - error("%s does not point to a valid object!", entry->name); - string_list_append(refs_to_delete, entry->name); - return 0; - } - - return 0; -} - -int repack_without_refs(const char **refnames, int n, struct strbuf *err) +int repack_without_refs(struct string_list *refnames, struct strbuf *err) { struct ref_dir *packed; - struct string_list refs_to_delete = STRING_LIST_INIT_DUP; - struct string_list_item *ref_to_delete; - int i, ret, removed = 0; + struct string_list_item *refname; + int ret, needs_repacking = 0, removed = 0; assert(err); /* Look for a packed ref */ - for (i = 0; i < n; i++) - if (get_packed_ref(refnames[i])) + for_each_string_list_item(refname, refnames) { + if (get_packed_ref(refname->string)) { + needs_repacking = 1; break; + } + } /* Avoid locking if we have nothing to do */ - if (i == n) + if (!needs_repacking) return 0; /* no refname exists in packed refs */ if (lock_packed_refs(0)) { @@@ -2623,8 -2675,8 +2630,8 @@@ packed = get_packed_refs(&ref_cache); /* Remove refnames from the cache */ - for (i = 0; i < n; i++) - if (remove_entry(packed, refnames[i]) != -1) + for_each_string_list_item(refname, refnames) + if (remove_entry(packed, refname->string) != -1) removed = 1; if (!removed) { /* @@@ -2635,6 -2687,13 +2642,6 @@@ return 0; } - /* Remove any other accumulated cruft */ - do_for_each_entry_in_dir(packed, 0, curate_packed_ref_fn, &refs_to_delete); - for_each_string_list_item(ref_to_delete, &refs_to_delete) { - if (remove_entry(packed, ref_to_delete->string) == -1) - die("internal error"); - } - /* Write what remains */ ret = commit_packed_refs(); if (ret) @@@ -2747,8 -2806,9 +2754,9 @@@ static int rename_ref_available(const c return ret; } - static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, - const char *logmsg); + static int write_ref_to_lockfile(struct ref_lock *lock, const unsigned char *sha1); + static int commit_ref_update(struct ref_lock *lock, + const unsigned char *sha1, const char *logmsg); int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsg) { @@@ -2807,7 -2867,9 +2815,9 @@@ } lock->force_write = 1; hashcpy(lock->old_sha1, orig_sha1); - if (write_ref_sha1(lock, orig_sha1, logmsg)) { + + if (write_ref_to_lockfile(lock, orig_sha1) || + commit_ref_update(lock, orig_sha1, logmsg)) { error("unable to write current sha1 into %s", newrefname); goto rollback; } @@@ -2824,7 -2886,8 +2834,8 @@@ lock->force_write = 1; flag = log_all_ref_updates; log_all_ref_updates = 0; - if (write_ref_sha1(lock, orig_sha1, NULL)) + if (write_ref_to_lockfile(lock, orig_sha1) || + commit_ref_update(lock, orig_sha1, NULL)) error("unable to write current sha1 into %s", oldrefname); log_all_ref_updates = flag; @@@ -2996,23 -3059,15 +3007,15 @@@ int is_branch(const char *refname } /* - * Write sha1 into the ref specified by the lock. Make sure that errno - * is sane on error. + * Write sha1 into the open lockfile, then close the lockfile. On + * errors, rollback the lockfile and set errno to reflect the problem. */ - static int write_ref_sha1(struct ref_lock *lock, - const unsigned char *sha1, const char *logmsg) + static int write_ref_to_lockfile(struct ref_lock *lock, + const unsigned char *sha1) { static char term = '\n'; struct object *o; - if (!lock) { - errno = EINVAL; - return -1; - } - if (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) { - unlock_ref(lock); - return 0; - } o = parse_object(sha1); if (!o) { error("Trying to write ref %s with nonexistent object %s", @@@ -3037,6 -3092,17 +3040,17 @@@ errno = save_errno; return -1; } + return 0; + } + + /* + * Commit a change to a loose reference that has already been written + * to the loose reference lockfile. Also update the reflogs if + * necessary, using the specified lockmsg (which can be NULL). + */ + static int commit_ref_update(struct ref_lock *lock, + const unsigned char *sha1, const char *logmsg) + { clear_loose_ref_cache(&ref_cache); if (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 || (strcmp(lock->ref_name, lock->orig_ref_name) && @@@ -3715,11 -3781,10 +3729,11 @@@ static int ref_update_reject_duplicates int ref_transaction_commit(struct ref_transaction *transaction, struct strbuf *err) { - int ret = 0, delnum = 0, i; - const char **delnames; + 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; assert(err); @@@ -3731,6 -3796,9 +3745,6 @@@ return 0; } - /* Allocate work space */ - delnames = xmalloc(sizeof(*delnames) * n); - /* Copy, sort, and reject duplicate refs */ qsort(updates, n, sizeof(*updates), ref_update_compare); if (ref_update_reject_duplicates(updates, n, err)) { @@@ -3738,19 -3806,23 +3752,23 @@@ goto cleanup; } - /* Acquire all locks while verifying old values */ + /* + * 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. + */ for (i = 0; i < n; i++) { struct ref_update *update = updates[i]; - int flags = update->flags; if (is_null_sha1(update->new_sha1)) - flags |= REF_DELETING; + update->flags |= REF_DELETING; update->lock = lock_ref_sha1_basic(update->refname, (update->have_old ? update->old_sha1 : NULL), NULL, - flags, + update->flags, &update->type); if (!update->lock) { ret = (errno == ENOTDIR) @@@ -3760,22 -3832,51 +3778,51 @@@ update->refname); goto cleanup; } + if (!(update->flags & REF_DELETING) && + (update->lock->force_write || + hashcmp(update->lock->old_sha1, update->new_sha1))) { + if (write_ref_to_lockfile(update->lock, update->new_sha1)) { + /* + * The lock was freed upon failure of + * write_ref_to_lockfile(): + */ + update->lock = NULL; + strbuf_addf(err, "Cannot update the ref '%s'.", + update->refname); + ret = TRANSACTION_GENERIC_ERROR; + goto cleanup; + } + update->flags |= REF_NEEDS_COMMIT; + } else { + /* + * We didn't have to write anything to the lockfile. + * Close it to free up the file descriptor: + */ + if (close_ref(update->lock)) { + strbuf_addf(err, "Couldn't close %s.lock", + update->refname); + goto cleanup; + } + } } /* Perform updates first so live commits remain referenced */ for (i = 0; i < n; i++) { struct ref_update *update = updates[i]; - if (!is_null_sha1(update->new_sha1)) { - if (write_ref_sha1(update->lock, update->new_sha1, - update->msg)) { - update->lock = NULL; /* freed by write_ref_sha1 */ + if (update->flags & REF_NEEDS_COMMIT) { + if (commit_ref_update(update->lock, + update->new_sha1, update->msg)) { + /* The lock was freed by commit_ref_update(): */ + update->lock = NULL; strbuf_addf(err, "Cannot update the ref '%s'.", update->refname); ret = TRANSACTION_GENERIC_ERROR; goto cleanup; + } else { + /* freed by the above call: */ + update->lock = NULL; } - update->lock = NULL; /* freed by write_ref_sha1 */ } } @@@ -3783,24 -3884,23 +3830,24 @@@ for (i = 0; i < n; i++) { struct ref_update *update = updates[i]; - if (update->lock) { + if (update->flags & REF_DELETING) { if (delete_ref_loose(update->lock, update->type, err)) { ret = TRANSACTION_GENERIC_ERROR; goto cleanup; } if (!(update->flags & REF_ISPRUNING)) - delnames[delnum++] = update->lock->ref_name; + string_list_append(&refs_to_delete, + update->lock->ref_name); } } - if (repack_without_refs(delnames, delnum, err)) { + if (repack_without_refs(&refs_to_delete, err)) { ret = TRANSACTION_GENERIC_ERROR; goto cleanup; } - for (i = 0; i < delnum; i++) - unlink_or_warn(git_path("logs/%s", delnames[i])); + for_each_string_list_item(ref_to_delete, &refs_to_delete) + unlink_or_warn(git_path("logs/%s", ref_to_delete->string)); clear_loose_ref_cache(&ref_cache); cleanup: @@@ -3809,7 -3909,7 +3856,7 @@@ for (i = 0; i < n; i++) if (updates[i]->lock) unlock_ref(updates[i]->lock); - free(delnames); + string_list_clear(&refs_to_delete, 0); return ret; } diff --combined t/t1400-update-ref.sh index 6805b9e6bb,c593a1df20..636d3a167c --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@@ -619,52 -619,6 +619,52 @@@ test_expect_success 'stdin update/creat test_must_fail git rev-parse --verify -q $c ' +test_expect_success 'stdin verify succeeds for correct value' ' + git rev-parse $m >expect && + echo "verify $m $m" >stdin && + git update-ref --stdin actual && + test_cmp expect actual +' + +test_expect_success 'stdin verify succeeds for missing reference' ' + echo "verify refs/heads/missing $Z" >stdin && + git update-ref --stdin stdin && + git update-ref --stdin expect && + echo "verify $m $m~1" >stdin && + test_must_fail git update-ref --stdin actual && + test_cmp expect actual +' + +test_expect_success 'stdin verify fails for mistaken null value' ' + git rev-parse $m >expect && + echo "verify $m $Z" >stdin && + test_must_fail git update-ref --stdin actual && + test_cmp expect actual +' + +test_expect_success 'stdin verify fails for mistaken empty value' ' + M=$(git rev-parse $m) && + test_when_finished "git update-ref $m $M" && + git rev-parse $m >expect && + echo "verify $m" >stdin && + test_must_fail git update-ref --stdin actual && + test_cmp expect actual +' + test_expect_success 'stdin update refs works with identity updates' ' cat >stdin <<-EOF && update $a $m $m @@@ -984,52 -938,6 +984,52 @@@ test_expect_success 'stdin -z update/cr test_must_fail git rev-parse --verify -q $c ' +test_expect_success 'stdin -z verify succeeds for correct value' ' + git rev-parse $m >expect && + printf $F "verify $m" "$m" >stdin && + git update-ref -z --stdin actual && + test_cmp expect actual +' + +test_expect_success 'stdin -z verify succeeds for missing reference' ' + printf $F "verify refs/heads/missing" "$Z" >stdin && + git update-ref -z --stdin stdin && + git update-ref -z --stdin expect && + printf $F "verify $m" "$m~1" >stdin && + test_must_fail git update-ref -z --stdin actual && + test_cmp expect actual +' + +test_expect_success 'stdin -z verify fails for mistaken null value' ' + git rev-parse $m >expect && + printf $F "verify $m" "$Z" >stdin && + test_must_fail git update-ref -z --stdin actual && + test_cmp expect actual +' + +test_expect_success 'stdin -z verify fails for mistaken empty value' ' + M=$(git rev-parse $m) && + test_when_finished "git update-ref $m $M" && + git rev-parse $m >expect && + printf $F "verify $m" "" >stdin && + test_must_fail git update-ref -z --stdin actual && + test_cmp expect actual +' + test_expect_success 'stdin -z update refs works with identity updates' ' printf $F "update $a" "$m" "$m" "update $b" "$m" "$m" "update $c" "$Z" "" >stdin && git update-ref -z --stdin large_input && + run_with_limited_open_files git update-ref --stdin large_input && + run_with_limited_open_files git update-ref --stdin expect echo "RFC1991 signed tag" >>expect echo '-----BEGIN PGP MESSAGE-----' >>expect -test_expect_success GPG \ +test_expect_success GPG,RFC1991 \ 'creating a signed tag with rfc1991' ' echo "rfc1991" >gpghome/gpg.conf && git tag -s -m "RFC1991 signed tag" rfc1991-signed-tag $commit && @@@ -1095,7 -1095,7 +1095,7 @@@ cp "$1" actua EOF chmod +x fakeeditor -test_expect_success GPG \ +test_expect_success GPG,RFC1991 \ 'reediting a signed tag body omits signature' ' echo "rfc1991" >gpghome/gpg.conf && echo "RFC1991 signed tag" >expect && @@@ -1103,13 -1103,13 +1103,13 @@@ test_cmp expect actual ' -test_expect_success GPG \ +test_expect_success GPG,RFC1991 \ 'verifying rfc1991 signature' ' echo "rfc1991" >gpghome/gpg.conf && git tag -v rfc1991-signed-tag ' -test_expect_success GPG \ +test_expect_success GPG,RFC1991 \ 'list tag with rfc1991 signature' ' echo "rfc1991" >gpghome/gpg.conf && echo "rfc1991-signed-tag RFC1991 signed tag" >expect && @@@ -1123,12 -1123,12 +1123,12 @@@ rm -f gpghome/gpg.conf -test_expect_success GPG \ +test_expect_success GPG,RFC1991 \ 'verifying rfc1991 signature without --rfc1991' ' git tag -v rfc1991-signed-tag ' -test_expect_success GPG \ +test_expect_success GPG,RFC1991 \ 'list tag with rfc1991 signature without --rfc1991' ' echo "rfc1991-signed-tag RFC1991 signed tag" >expect && git tag -l -n1 rfc1991-signed-tag >actual && @@@ -1139,7 -1139,7 +1139,7 @@@ test_cmp expect actual ' -test_expect_success GPG \ +test_expect_success GPG,RFC1991 \ 'reediting a signed tag body omits signature' ' echo "RFC1991 signed tag" >expect && GIT_EDITOR=./fakeeditor git tag -f -s rfc1991-signed-tag $commit && @@@ -1463,10 -1463,10 +1463,10 @@@ run_with_limited_stack () (ulimit -s 128 && "$@") } - test_lazy_prereq ULIMIT 'run_with_limited_stack true' + test_lazy_prereq ULIMIT_STACK_SIZE 'run_with_limited_stack true' # we require ulimit, this excludes Windows - test_expect_success ULIMIT '--contains works in a deep repo' ' + test_expect_success ULIMIT_STACK_SIZE '--contains works in a deep repo' ' >expect && i=1 && while test $i -lt 8000