From: Junio C Hamano Date: Thu, 14 Sep 2017 08:40:35 +0000 (+0900) Subject: Merge branch 'jk/incore-lockfile-removal' into next X-Git-Url: https://git.lorimer.id.au/gitweb.git/diff_plain/603dae7fb2d1fe208b734e645ffab8a3c56b03f4?hp=-c Merge branch 'jk/incore-lockfile-removal' into next The long-standing rule that an in-core lockfile instance, once it is used, must not be freed, has been lifted and the lockfile and tempfile APIs have been updated to reduce the chance of programming errors. * jk/incore-lockfile-removal: stop leaking lock structs in some simple cases ref_lock: stop leaking lock_files lockfile: update lifetime requirements in documentation tempfile: auto-allocate tempfiles on heap tempfile: remove deactivated list entries tempfile: use list.h for linked list tempfile: release deactivated strbufs instead of resetting tempfile: robustify cleanup handler tempfile: factor out deactivation tempfile: factor out activation tempfile: replace die("BUG") with BUG() tempfile: handle NULL tempfile pointers gracefully tempfile: prefer is_tempfile_active to bare access lockfile: do not rollback lock on failed close tempfile: do not delete tempfile on failed close always check return value of close_tempfile verify_signed_buffer: prefer close_tempfile() to close() setup_temporary_shallow: move tempfile struct into function setup_temporary_shallow: avoid using inactive tempfile write_index_as_tree: cleanup tempfile on error --- 603dae7fb2d1fe208b734e645ffab8a3c56b03f4 diff --combined refs/files-backend.c index cda790dcbc,f3455609d6..a7cc65d0de --- a/refs/files-backend.c +++ b/refs/files-backend.c @@@ -12,7 -12,7 +12,7 @@@ struct ref_lock { char *ref_name; - struct lock_file *lk; + struct lock_file lk; struct object_id old_oid; }; @@@ -106,6 -106,15 +106,6 @@@ static void files_reflog_path(struct fi struct strbuf *sb, const char *refname) { - if (!refname) { - /* - * FIXME: of course this is wrong in multi worktree - * setting. To be fixed real soon. - */ - strbuf_addf(sb, "%s/logs", refs->gitcommondir); - return; - } - switch (ref_type(refname)) { case REF_TYPE_PER_WORKTREE: case REF_TYPE_PSEUDOREF: @@@ -409,9 -418,7 +409,7 @@@ out 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); + rollback_lock_file(&lock->lk); free(lock->ref_name); free(lock); } @@@ -525,11 -532,8 +523,8 @@@ retry goto error_return; } - if (!lock->lk) - lock->lk = xcalloc(1, sizeof(struct lock_file)); - if (hold_lock_file_for_update_timeout( - lock->lk, ref_file.buf, LOCK_NO_DEREF, + &lock->lk, ref_file.buf, LOCK_NO_DEREF, get_files_ref_lock_timeout_ms()) < 0) { if (errno == ENOENT && --attempts_remaining > 0) { /* @@@ -940,11 -944,9 +935,9 @@@ static struct ref_lock *lock_ref_sha1_b goto error_return; } - lock->lk = xcalloc(1, sizeof(struct lock_file)); - lock->ref_name = xstrdup(refname); - if (raceproof_create_file(ref_file.buf, create_reflock, lock->lk)) { + if (raceproof_create_file(ref_file.buf, create_reflock, &lock->lk)) { last_errno = errno; unable_to_lock_message(ref_file.buf, errno, err); goto error_return; @@@ -1393,16 -1395,16 +1386,16 @@@ static int files_rename_ref(struct ref_ return ret; } - static int close_ref(struct ref_lock *lock) + static int close_ref_gently(struct ref_lock *lock) { - if (close_lock_file(lock->lk)) + if (close_lock_file_gently(&lock->lk)) return -1; return 0; } static int commit_ref(struct ref_lock *lock) { - char *path = get_locked_file_path(lock->lk); + char *path = get_locked_file_path(&lock->lk); struct stat st; if (!lstat(path, &st) && S_ISDIR(st.st_mode)) { @@@ -1426,7 -1428,7 +1419,7 @@@ free(path); } - if (commit_lock_file(lock->lk)) + if (commit_lock_file(&lock->lk)) return -1; return 0; } @@@ -1618,12 -1620,12 +1611,12 @@@ static int write_ref_to_lockfile(struc unlock_ref(lock); return -1; } - fd = get_lock_file_fd(lock->lk); + fd = get_lock_file_fd(&lock->lk); if (write_in_full(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ || write_in_full(fd, &term, 1) != 1 || - close_ref(lock) < 0) { + close_ref_gently(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; } @@@ -1700,7 -1702,7 +1693,7 @@@ static int create_ref_symlink(struct re { int ret = -1; #ifndef NO_SYMLINK_HEAD - char *ref_path = get_locked_file_path(lock->lk); + char *ref_path = get_locked_file_path(&lock->lk); unlink(ref_path); ret = symlink(target, ref_path); free(ref_path); @@@ -1736,14 -1738,14 +1729,14 @@@ static int create_symref_locked(struct return 0; } - if (!fdopen_lock_file(lock->lk, "w")) + if (!fdopen_lock_file(&lock->lk, "w")) return error("unable to fdopen %s: %s", - lock->lk->tempfile.filename.buf, strerror(errno)); + lock->lk.tempfile->filename.buf, strerror(errno)); update_symref_reflog(refs, lock, refname, target, logmsg); /* no error check; commit_ref will check ferror */ - fprintf(lock->lk->tempfile.fp, "ref: %s\n", target); + fprintf(lock->lk.tempfile->fp, "ref: %s\n", target); if (commit_ref(lock) < 0) return error("unable to write symref for %s: %s", refname, strerror(errno)); @@@ -2050,63 -2052,23 +2043,63 @@@ static struct ref_iterator_vtable files files_reflog_iterator_abort }; -static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_store) +static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store, + const char *gitdir) { - struct files_ref_store *refs = - files_downcast(ref_store, REF_STORE_READ, - "reflog_iterator_begin"); struct files_reflog_iterator *iter = xcalloc(1, sizeof(*iter)); struct ref_iterator *ref_iterator = &iter->base; struct strbuf sb = STRBUF_INIT; base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable); - files_reflog_path(refs, &sb, NULL); + strbuf_addf(&sb, "%s/logs", gitdir); iter->dir_iterator = dir_iterator_begin(sb.buf); iter->ref_store = ref_store; strbuf_release(&sb); + return ref_iterator; } +static enum iterator_selection reflog_iterator_select( + struct ref_iterator *iter_worktree, + struct ref_iterator *iter_common, + void *cb_data) +{ + if (iter_worktree) { + /* + * We're a bit loose here. We probably should ignore + * common refs if they are accidentally added as + * per-worktree refs. + */ + return ITER_SELECT_0; + } else if (iter_common) { + if (ref_type(iter_common->refname) == REF_TYPE_NORMAL) + return ITER_SELECT_1; + + /* + * The main ref store may contain main worktree's + * per-worktree refs, which should be ignored + */ + return ITER_SKIP_1; + } else + return ITER_DONE; +} + +static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_store) +{ + struct files_ref_store *refs = + files_downcast(ref_store, REF_STORE_READ, + "reflog_iterator_begin"); + + if (!strcmp(refs->gitdir, refs->gitcommondir)) { + return reflog_iterator_begin(ref_store, refs->gitcommondir); + } else { + return merge_ref_iterator_begin( + reflog_iterator_begin(ref_store, refs->gitdir), + reflog_iterator_begin(ref_store, refs->gitcommondir), + reflog_iterator_select, refs); + } +} + /* * 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. @@@ -2130,10 -2092,11 +2123,10 @@@ static int split_head_update(struct ref /* * First make sure that HEAD is not already in the - * transaction. This insertion is O(N) in the transaction + * transaction. This check is O(lg N) in the transaction * size, but it happens at most once per transaction. */ - item = string_list_insert(affected_refnames, "HEAD"); - if (item->util) { + if (string_list_has_string(affected_refnames, "HEAD")) { /* An entry already existed */ strbuf_addf(err, "multiple updates for 'HEAD' (including one " @@@ -2148,14 -2111,6 +2141,14 @@@ update->new_oid.hash, update->old_oid.hash, update->msg); + /* + * Add "HEAD". This insertion is O(N) in the transaction + * size, but it happens at most once per transaction. + * Add new_update->refname instead of a literal "HEAD". + */ + if (strcmp(new_update->refname, "HEAD")) + BUG("%s unexpectedly not 'HEAD'", new_update->refname); + item = string_list_insert(affected_refnames, new_update->refname); item->util = new_update; return 0; @@@ -2182,12 -2137,13 +2175,12 @@@ static int split_symref_update(struct f /* * First make sure that referent is not already in the - * transaction. This insertion is O(N) in the transaction + * transaction. This check is O(lg 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 */ + if (string_list_has_string(affected_refnames, referent)) { + /* An entry already exists */ strbuf_addf(err, "multiple updates for '%s' (including one " "via symref '%s') are not allowed", @@@ -2222,17 -2178,6 +2215,17 @@@ update->flags |= REF_LOG_ONLY | REF_NODEREF; update->flags &= ~REF_HAVE_OLD; + /* + * Add the referent. This insertion is O(N) in the transaction + * size, but it happens at most once per symref in a + * transaction. Make sure to add new_update->refname, which will + * be valid as long as affected_refnames is in use, and NOT + * referent, which might soon be freed by our caller. + */ + item = string_list_insert(affected_refnames, new_update->refname); + if (item->util) + BUG("%s unexpectedly found in affected_refnames", + new_update->refname); item->util = new_update; return 0; @@@ -2304,7 -2249,7 +2297,7 @@@ static int lock_ref_for_update(struct f struct strbuf referent = STRBUF_INIT; int mustexist = (update->flags & REF_HAVE_OLD) && !is_null_oid(&update->old_oid); - int ret; + int ret = 0; struct ref_lock *lock; files_assert_main_repository(refs, "lock_ref_for_update"); @@@ -2316,7 -2261,7 +2309,7 @@@ ret = split_head_update(update, transaction, head_ref, affected_refnames, err); if (ret) - return ret; + goto out; } ret = lock_raw_ref(refs, update->refname, mustexist, @@@ -2330,7 -2275,7 +2323,7 @@@ strbuf_addf(err, "cannot lock ref '%s': %s", original_update_refname(update), reason); free(reason); - return ret; + goto out; } update->backend_data = lock; @@@ -2349,12 -2294,10 +2342,12 @@@ strbuf_addf(err, "cannot lock ref '%s': " "error reading reference", original_update_refname(update)); - return -1; + ret = TRANSACTION_GENERIC_ERROR; + goto out; } } else if (check_old_oid(update, &lock->old_oid, err)) { - return TRANSACTION_GENERIC_ERROR; + ret = TRANSACTION_GENERIC_ERROR; + goto out; } } else { /* @@@ -2368,15 -2311,13 +2361,15 @@@ referent.buf, transaction, affected_refnames, err); if (ret) - return ret; + goto out; } } else { struct ref_update *parent_update; - if (check_old_oid(update, &lock->old_oid, err)) - return TRANSACTION_GENERIC_ERROR; + if (check_old_oid(update, &lock->old_oid, err)) { + ret = TRANSACTION_GENERIC_ERROR; + goto out; + } /* * If this update is happening indirectly because of a @@@ -2413,8 -2354,7 +2406,8 @@@ "cannot update ref '%s': %s", update->refname, write_err); free(write_err); - return TRANSACTION_GENERIC_ERROR; + ret = TRANSACTION_GENERIC_ERROR; + goto out; } else { update->flags |= REF_NEEDS_COMMIT; } @@@ -2425,17 -2365,13 +2418,17 @@@ * the lockfile is still open. Close it to * free up the file descriptor: */ - if (close_ref(lock)) { + if (close_ref_gently(lock)) { strbuf_addf(err, "couldn't close '%s.lock'", update->refname); - return TRANSACTION_GENERIC_ERROR; + ret = TRANSACTION_GENERIC_ERROR; + goto out; } } - return 0; + +out: + strbuf_release(&referent); + return ret; } /* @@@ -2905,16 -2841,17 +2898,17 @@@ static int files_reflog_expire(struct r !(type & REF_ISSYMREF) && !is_null_oid(&cb.last_kept_oid); - if (close_lock_file(&reflog_lock)) { + if (close_lock_file_gently(&reflog_lock)) { status |= error("couldn't write %s: %s", log_file, strerror(errno)); + rollback_lock_file(&reflog_lock); } else if (update && - (write_in_full(get_lock_file_fd(lock->lk), + (write_in_full(get_lock_file_fd(&lock->lk), oid_to_hex(&cb.last_kept_oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ || - write_str_in_full(get_lock_file_fd(lock->lk), "\n") != 1 || - close_ref(lock) < 0)) { + write_str_in_full(get_lock_file_fd(&lock->lk), "\n") != 1 || + close_ref_gently(lock) < 0)) { status |= error("couldn't write %s", - get_lock_file_path(lock->lk)); + get_lock_file_path(&lock->lk)); rollback_lock_file(&reflog_lock); } else if (commit_lock_file(&reflog_lock)) { status |= error("unable to write reflog '%s' (%s)",