Merge branch 'jk/incore-lockfile-removal'
authorJunio C Hamano <gitster@pobox.com>
Tue, 19 Sep 2017 01:47:53 +0000 (10:47 +0900)
committerJunio C Hamano <gitster@pobox.com>
Tue, 19 Sep 2017 01:47:53 +0000 (10:47 +0900)
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

1  2 
refs/files-backend.c
diff --combined refs/files-backend.c
index cda790dcbc9389121093077059eafce514eda23a,f3455609d658a55b20ca47026236cec17533ecca..a7cc65d0dee2dc5e8d3af6e447fc337e5958930f
@@@ -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)) {
                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 "
                        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",
        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");
                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,
                strbuf_addf(err, "cannot lock ref '%s': %s",
                            original_update_refname(update), reason);
                free(reason);
 -              return ret;
 +              goto out;
        }
  
        update->backend_data = lock;
                                        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 {
                        /*
                                                  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
                                    "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;
                }
                 * 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)",