From: Junio C Hamano Date: Tue, 10 Mar 2015 20:52:39 +0000 (-0700) Subject: Merge branch 'mh/expire-updateref-fixes' X-Git-Tag: v2.4.0-rc0~51 X-Git-Url: https://git.lorimer.id.au/gitweb.git/diff_plain/82b7e651990dce7016a903cb020e3dc0eba2ecfe?ds=inline;hp=-c Merge branch 'mh/expire-updateref-fixes' Various issues around "reflog expire", e.g. using --updateref when expiring a reflog for a symbolic reference, have been corrected and/or made saner. * mh/expire-updateref-fixes: reflog_expire(): never update a reference to null_sha1 reflog_expire(): ignore --updateref for symbolic references reflog: improve and update documentation struct ref_lock: delete the force_write member lock_ref_sha1_basic(): do not set force_write for missing references write_ref_sha1(): move write elision test to callers write_ref_sha1(): remove check for lock == NULL --- 82b7e651990dce7016a903cb020e3dc0eba2ecfe diff --combined refs.c index 8d46b08055,05a4be0c06..e23542b386 --- a/refs.c +++ b/refs.c @@@ -12,7 -12,6 +12,6 @@@ struct ref_lock struct lock_file *lk; unsigned char old_sha1[20]; int lock_fd; - int force_write; }; /* @@@ -35,29 -34,10 +34,29 @@@ static unsigned char refname_dispositio }; /* - * Used as a flag to ref_transaction_delete when a loose ref is being + * Flag passed to lock_ref_sha1_basic() telling it to tolerate broken + * refs (i.e., because the reference is about to be deleted anyway). + */ +#define REF_DELETING 0x02 + +/* + * Used as a flag in ref_update::flags when a loose ref is being * pruned. */ -#define REF_ISPRUNING 0x0100 +#define REF_ISPRUNING 0x04 + +/* + * Used as a flag in ref_update::flags when the reference should be + * updated to new_sha1. + */ +#define REF_HAVE_NEW 0x08 + +/* + * Used as a flag in ref_update::flags when old_sha1 should be + * checked. + */ +#define REF_HAVE_OLD 0x10 + /* * Try to read one refname component from the front of refname. * Return the length of the component found, or -1 if the component is @@@ -1646,7 -1626,8 +1645,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 */ @@@ -2268,7 -2249,7 +2267,7 @@@ int dwim_log(const char *str, int len, static struct ref_lock *lock_ref_sha1_basic(const char *refname, const unsigned char *old_sha1, const struct string_list *skip, - int flags, int *type_p) + unsigned int flags, int *type_p) { char *ref_file; const char *orig_refname = refname; @@@ -2277,7 -2258,6 +2276,6 @@@ int type, lflags; int mustexist = (old_sha1 && !is_null_sha1(old_sha1)); int resolve_flags = 0; - int missing = 0; int attempts_remaining = 3; lock = xcalloc(1, sizeof(struct ref_lock)); @@@ -2316,13 -2296,13 +2314,13 @@@ orig_refname, strerror(errno)); goto error_return; } - missing = is_null_sha1(lock->old_sha1); - /* When the ref did not exist and we are creating it, - * make sure there is no existing ref that is packed - * whose name begins with our refname, nor a ref whose - * name is a proper prefix of our 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 + * refname, nor a packed ref whose name is a proper prefix of + * our refname. */ - if (missing && + if (is_null_sha1(lock->old_sha1) && !is_refname_available(refname, skip, get_packed_refs(&ref_cache))) { last_errno = ENOTDIR; goto error_return; @@@ -2338,10 -2318,6 +2336,6 @@@ lock->ref_name = xstrdup(refname); lock->orig_ref_name = xstrdup(orig_refname); ref_file = git_path("%s", refname); - if (missing) - lock->force_write = 1; - if ((flags & REF_NODEREF) && (type & REF_ISSYMREF)) - lock->force_write = 1; retry: switch (safe_create_leading_directories(ref_file)) { @@@ -2582,7 -2558,7 +2576,7 @@@ static void prune_ref(struct ref_to_pru transaction = ref_transaction_begin(&err); if (!transaction || ref_transaction_delete(transaction, r->name, r->sha1, - REF_ISPRUNING, 1, NULL, &err) || + REF_ISPRUNING, NULL, &err) || ref_transaction_commit(transaction, &err)) { ref_transaction_free(transaction); error("%s", err.buf); @@@ -2752,16 -2728,15 +2746,16 @@@ static int delete_ref_loose(struct ref_ return 0; } -int delete_ref(const char *refname, const unsigned char *sha1, int delopt) +int delete_ref(const char *refname, const unsigned char *sha1, unsigned int flags) { struct ref_transaction *transaction; struct strbuf err = STRBUF_INIT; transaction = ref_transaction_begin(&err); if (!transaction || - ref_transaction_delete(transaction, refname, sha1, delopt, - sha1 && !is_null_sha1(sha1), NULL, &err) || + ref_transaction_delete(transaction, refname, + (sha1 && !is_null_sha1(sha1)) ? sha1 : NULL, + flags, NULL, &err) || ref_transaction_commit(transaction, &err)) { error("%s", err.buf); ref_transaction_free(transaction); @@@ -2897,7 -2872,6 +2891,6 @@@ int rename_ref(const char *oldrefname, error("unable to lock %s for update", newrefname); goto rollback; } - lock->force_write = 1; hashcpy(lock->old_sha1, orig_sha1); if (write_ref_sha1(lock, orig_sha1, logmsg)) { error("unable to write current sha1 into %s", newrefname); @@@ -2913,7 -2887,6 +2906,6 @@@ goto rollbacklog; } - lock->force_write = 1; flag = log_all_ref_updates; log_all_ref_updates = 0; if (write_ref_sha1(lock, orig_sha1, NULL)) @@@ -3099,14 -3072,6 +3091,6 @@@ static int write_ref_sha1(struct ref_lo 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", @@@ -3576,27 -3541,16 +3560,27 @@@ int for_each_reflog(each_ref_fn fn, voi } /** - * Information needed for a single ref update. Set new_sha1 to the - * new value or to zero to delete the ref. To check the old value - * while locking the ref, set have_old to 1 and set old_sha1 to the - * value or to zero to ensure the ref does not exist before update. + * Information needed for a single ref update. Set new_sha1 to the new + * value or to null_sha1 to delete the ref. To check the old value + * while the ref is locked, set (flags & REF_HAVE_OLD) and set + * old_sha1 to the old value, or to null_sha1 to ensure the ref does + * 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]; - int flags; /* REF_NODEREF? */ - int have_old; /* 1 if old_sha1 is valid, 0 otherwise */ + /* + * One or more of REF_HAVE_NEW, REF_HAVE_OLD, REF_NODEREF, + * REF_DELETING, and REF_ISPRUNING: + */ + unsigned int flags; struct ref_lock *lock; int type; char *msg; @@@ -3668,7 -3622,7 +3652,7 @@@ int ref_transaction_update(struct ref_t const char *refname, const unsigned char *new_sha1, const unsigned char *old_sha1, - int flags, int have_old, const char *msg, + unsigned int flags, const char *msg, struct strbuf *err) { struct ref_update *update; @@@ -3678,7 -3632,10 +3662,7 @@@ if (transaction->state != REF_TRANSACTION_OPEN) die("BUG: update called for transaction that is not open"); - if (have_old && !old_sha1) - die("BUG: have_old is true but old_sha1 is NULL"); - - if (!is_null_sha1(new_sha1) && + 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", refname); @@@ -3686,15 -3643,11 +3670,15 @@@ } update = add_update(transaction, refname); - hashcpy(update->new_sha1, new_sha1); - update->flags = flags; - update->have_old = have_old; - if (have_old) + if (new_sha1) { + hashcpy(update->new_sha1, new_sha1); + flags |= REF_HAVE_NEW; + } + if (old_sha1) { hashcpy(update->old_sha1, old_sha1); + flags |= REF_HAVE_OLD; + } + update->flags = flags; if (msg) update->msg = xstrdup(msg); return 0; @@@ -3703,52 -3656,34 +3687,52 @@@ int ref_transaction_create(struct ref_transaction *transaction, const char *refname, const unsigned char *new_sha1, - int flags, const char *msg, + unsigned int flags, const char *msg, struct strbuf *err) { + if (!new_sha1 || is_null_sha1(new_sha1)) + die("BUG: create called without valid new_sha1"); return ref_transaction_update(transaction, refname, new_sha1, - null_sha1, flags, 1, msg, err); + null_sha1, flags, msg, err); } int ref_transaction_delete(struct ref_transaction *transaction, const char *refname, const unsigned char *old_sha1, - int flags, int have_old, const char *msg, + unsigned int flags, const char *msg, struct strbuf *err) { - return ref_transaction_update(transaction, refname, null_sha1, - old_sha1, flags, have_old, msg, err); + if (old_sha1 && is_null_sha1(old_sha1)) + die("BUG: delete called with old_sha1 set to zeros"); + return ref_transaction_update(transaction, refname, + null_sha1, old_sha1, + flags, msg, err); } -int update_ref(const char *action, const char *refname, - const unsigned char *sha1, const unsigned char *oldval, - int flags, enum action_on_err onerr) +int ref_transaction_verify(struct ref_transaction *transaction, + const char *refname, + const unsigned char *old_sha1, + unsigned int flags, + struct strbuf *err) +{ + if (!old_sha1) + die("BUG: verify called with old_sha1 set to NULL"); + return ref_transaction_update(transaction, refname, + NULL, old_sha1, + flags, NULL, err); +} + +int update_ref(const char *msg, const char *refname, + const unsigned char *new_sha1, const unsigned char *old_sha1, + unsigned int flags, enum action_on_err onerr) { struct ref_transaction *t; struct strbuf err = STRBUF_INIT; t = ref_transaction_begin(&err); if (!t || - ref_transaction_update(t, refname, sha1, oldval, flags, - !!oldval, action, &err) || + ref_transaction_update(t, refname, new_sha1, old_sha1, + flags, msg, &err) || ref_transaction_commit(t, &err)) { const char *str = "update_ref failed for ref '%s': %s"; @@@ -3824,17 -3759,17 +3808,17 @@@ int ref_transaction_commit(struct ref_t /* Acquire all locks while verifying old values */ for (i = 0; i < n; i++) { struct ref_update *update = updates[i]; - int flags = update->flags; + unsigned int flags = update->flags; - if (is_null_sha1(update->new_sha1)) + if ((flags & REF_HAVE_NEW) && is_null_sha1(update->new_sha1)) flags |= REF_DELETING; - update->lock = lock_ref_sha1_basic(update->refname, - (update->have_old ? - update->old_sha1 : - NULL), - NULL, - flags, - &update->type); + update->lock = lock_ref_sha1_basic( + update->refname, + ((update->flags & REF_HAVE_OLD) ? + update->old_sha1 : NULL), + NULL, + flags, + &update->type); if (!update->lock) { ret = (errno == ENOTDIR) ? TRANSACTION_NAME_CONFLICT @@@ -3848,33 -3783,44 +3832,46 @@@ /* Perform updates first so live commits remain referenced */ for (i = 0; i < n; i++) { struct ref_update *update = updates[i]; + int flags = update->flags; - if (!is_null_sha1(update->new_sha1)) { + if ((flags & REF_HAVE_NEW) && !is_null_sha1(update->new_sha1)) { - if (write_ref_sha1(update->lock, update->new_sha1, - update->msg)) { + int overwriting_symref = ((update->type & REF_ISSYMREF) && + (update->flags & REF_NODEREF)); + + if (!overwriting_symref + && !hashcmp(update->lock->old_sha1, update->new_sha1)) { + /* + * The reference already has the desired + * value, so we don't need to write it. + */ + unlock_ref(update->lock); + update->lock = NULL; + } else if (write_ref_sha1(update->lock, update->new_sha1, + update->msg)) { update->lock = NULL; /* freed by write_ref_sha1 */ strbuf_addf(err, "Cannot update the ref '%s'.", update->refname); ret = TRANSACTION_GENERIC_ERROR; goto cleanup; + } else { + /* freed by write_ref_sha1(): */ + update->lock = NULL; } - update->lock = NULL; /* freed by write_ref_sha1 */ } } /* Perform deletes now that updates are safely completed */ for (i = 0; i < n; i++) { struct ref_update *update = updates[i]; + int flags = update->flags; - if (update->lock) { + if ((flags & REF_HAVE_NEW) && is_null_sha1(update->new_sha1)) { if (delete_ref_loose(update->lock, update->type, err)) { ret = TRANSACTION_GENERIC_ERROR; goto cleanup; } - if (!(update->flags & REF_ISPRUNING)) + if (!(flags & REF_ISPRUNING)) string_list_append(&refs_to_delete, update->lock->ref_name); } @@@ -4083,6 -4029,7 +4080,7 @@@ int reflog_expire(const char *refname, struct ref_lock *lock; char *log_file; int status = 0; + int type; memset(&cb, 0, sizeof(cb)); cb.flags = flags; @@@ -4094,7 -4041,7 +4092,7 @@@ * reference itself, plus we might need to update the * reference if --updateref was specified: */ - lock = lock_ref_sha1_basic(refname, sha1, NULL, 0, NULL); + lock = lock_ref_sha1_basic(refname, sha1, NULL, 0, &type); if (!lock) return error("cannot lock ref '%s'", refname); if (!reflog_exists(refname)) { @@@ -4131,10 -4078,21 +4129,21 @@@ (*cleanup_fn)(cb.policy_cb); if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) { + /* + * It doesn't make sense to adjust a reference pointed + * to by a symbolic ref based on expiring entries in + * the symbolic reference's reflog. Nor can we update + * a reference if there are no remaining reflog + * entries. + */ + int update = (flags & EXPIRE_REFLOGS_UPDATE_REF) && + !(type & REF_ISSYMREF) && + !is_null_sha1(cb.last_kept_sha1); + if (close_lock_file(&reflog_lock)) { status |= error("couldn't write %s: %s", log_file, strerror(errno)); - } else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) && + } else if (update && (write_in_full(lock->lock_fd, sha1_to_hex(cb.last_kept_sha1), 40) != 40 || write_str_in_full(lock->lock_fd, "\n") != 1 || @@@ -4145,7 -4103,7 +4154,7 @@@ } else if (commit_lock_file(&reflog_lock)) { status |= error("unable to commit reflog '%s' (%s)", log_file, strerror(errno)); - } else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) && commit_ref(lock)) { + } else if (update && commit_ref(lock)) { status |= error("couldn't set %s", lock->ref_name); } }