Merge branch 'mh/lockfile-retry'
authorJunio C Hamano <gitster@pobox.com>
Fri, 22 May 2015 19:41:55 +0000 (12:41 -0700)
committerJunio C Hamano <gitster@pobox.com>
Fri, 22 May 2015 19:41:55 +0000 (12:41 -0700)
Instead of dying immediately upon failing to obtain a lock, retry
after a short while with backoff.

* mh/lockfile-retry:
lock_packed_refs(): allow retries when acquiring the packed-refs lock
lockfile: allow file locking to be retried with a timeout

1  2 
Documentation/config.txt
refs.c
diff --combined Documentation/config.txt
index 54cbf6b7ea8c6e21a4ffc5c767fc15c630ccd33c,863057b25f198de17548ca4b2ea1a7b6eb48f3e8..0f668bbe11a5b439fca53ca7aa52a43782e30b79
@@@ -624,6 -624,12 +624,12 @@@ core.commentChar:
  If set to "auto", `git-commit` would select a character that is not
  the beginning character of any line in existing commit messages.
  
+ core.packedRefsTimeout::
+       The length of time, in milliseconds, to retry when trying to
+       lock the `packed-refs` file. Value 0 means not to retry at
+       all; -1 means to try indefinitely. Default is 1000 (i.e.,
+       retry for 1 second).
  sequence.editor::
        Text editor used by `git rebase -i` for editing the rebase instruction file.
        The value is meant to be interpreted by the shell when it is used.
@@@ -1569,19 -1575,6 +1575,19 @@@ http.saveCookies:
        If set, store cookies received during requests to the file specified by
        http.cookieFile. Has no effect if http.cookieFile is unset.
  
 +http.sslCipherList::
 +  A list of SSL ciphers to use when negotiating an SSL connection.
 +  The available ciphers depend on whether libcurl was built against
 +  NSS or OpenSSL and the particular configuration of the crypto
 +  library in use.  Internally this sets the 'CURLOPT_SSL_CIPHER_LIST'
 +  option; see the libcurl documentation for more details on the format
 +  of this list.
 ++
 +Can be overridden by the 'GIT_SSL_CIPHER_LIST' environment variable.
 +To force git to use libcurl's default cipher list and ignore any
 +explicit http.sslCipherList option, set 'GIT_SSL_CIPHER_LIST' to the
 +empty string.
 +
  http.sslVerify::
        Whether to verify the SSL certificate when fetching or pushing
        over HTTPS. Can be overridden by the 'GIT_SSL_NO_VERIFY' environment
@@@ -2292,18 -2285,18 +2298,18 @@@ remote.<name>.skipFetchAll:
  
  remote.<name>.receivepack::
        The default program to execute on the remote side when pushing.  See
 -      option \--receive-pack of linkgit:git-push[1].
 +      option --receive-pack of linkgit:git-push[1].
  
  remote.<name>.uploadpack::
        The default program to execute on the remote side when fetching.  See
 -      option \--upload-pack of linkgit:git-fetch-pack[1].
 +      option --upload-pack of linkgit:git-fetch-pack[1].
  
  remote.<name>.tagOpt::
 -      Setting this value to \--no-tags disables automatic tag following when
 -      fetching from remote <name>. Setting it to \--tags will fetch every
 +      Setting this value to --no-tags disables automatic tag following when
 +      fetching from remote <name>. Setting it to --tags will fetch every
        tag from remote <name>, even if they are not reachable from remote
        branch heads. Passing these flags directly to linkgit:git-fetch[1] can
 -      override this setting. See options \--tags and \--no-tags of
 +      override this setting. See options --tags and --no-tags of
        linkgit:git-fetch[1].
  
  remote.<name>.vcs::
diff --combined refs.c
index f704ee285cdff46e94d9bd47de5f39a7cc6927ee,8a4a9dd99c558355f50017021c9f514b9099a436..8480d8dbf5c78c28d895c9c1a2ac7aa5305f4e5e
--- 1/refs.c
--- 2/refs.c
+++ b/refs.c
@@@ -11,6 -11,7 +11,6 @@@ struct ref_lock 
        char *orig_ref_name;
        struct lock_file *lk;
        unsigned char old_sha1[20];
 -      int lock_fd;
  };
  
  /*
@@@ -56,12 -57,6 +56,12 @@@ static unsigned char refname_dispositio
   */
  #define REF_HAVE_OLD  0x10
  
 +/*
 + * Used as a flag in ref_update::flags when the lockfile needs to be
 + * committed.
 + */
 +#define REF_NEEDS_COMMIT 0x20
 +
  /*
   * Try to read one refname component from the front of refname.
   * Return the length of the component found, or -1 if the component is
@@@ -268,7 -263,7 +268,7 @@@ struct ref_dir 
   * presence of an empty subdirectory does not block the creation of a
   * similarly-named reference.  (The fact that reference names with the
   * same leading components can conflict *with each other* is a
 - * separate issue that is regulated by is_refname_available().)
 + * separate issue that is regulated by verify_refname_available().)
   *
   * Please note that the name field contains the fully-qualified
   * reference (or subdirectory) name.  Space could be saved by only
@@@ -844,181 -839,121 +844,181 @@@ static void prime_ref_dir(struct ref_di
        }
  }
  
 -static int entry_matches(struct ref_entry *entry, const struct string_list *list)
 -{
 -      return list && string_list_has_string(list, entry->name);
 -}
 -
  struct nonmatching_ref_data {
        const struct string_list *skip;
 -      struct ref_entry *found;
 +      const char *conflicting_refname;
  };
  
  static int nonmatching_ref_fn(struct ref_entry *entry, void *vdata)
  {
        struct nonmatching_ref_data *data = vdata;
  
 -      if (entry_matches(entry, data->skip))
 +      if (data->skip && string_list_has_string(data->skip, entry->name))
                return 0;
  
 -      data->found = entry;
 +      data->conflicting_refname = entry->name;
        return 1;
  }
  
 -static void report_refname_conflict(struct ref_entry *entry,
 -                                  const char *refname)
 -{
 -      error("'%s' exists; cannot create '%s'", entry->name, refname);
 -}
 -
  /*
 - * Return true iff a reference named refname could be created without
 - * conflicting with the name of an existing reference in dir.  If
 - * skip is non-NULL, ignore potential conflicts with refs in skip
 - * (e.g., because they are scheduled for deletion in the same
 - * operation).
 + * Return 0 if a reference named refname could be created without
 + * conflicting with the name of an existing reference in dir.
 + * Otherwise, return a negative value and write an explanation to err.
 + * If extras is non-NULL, it is a list of additional refnames with
 + * which refname is not allowed to conflict. If skip is non-NULL,
 + * ignore potential conflicts with refs in skip (e.g., because they
 + * are scheduled for deletion in the same operation). Behavior is
 + * undefined if the same name is listed in both extras and skip.
   *
   * Two reference names conflict if one of them exactly matches the
 - * leading components of the other; e.g., "foo/bar" conflicts with
 - * both "foo" and with "foo/bar/baz" but not with "foo/bar" or
 - * "foo/barbados".
 + * leading components of the other; e.g., "refs/foo/bar" conflicts
 + * with both "refs/foo" and with "refs/foo/bar/baz" but not with
 + * "refs/foo/bar" or "refs/foo/barbados".
   *
 - * skip must be sorted.
 + * extras and skip must be sorted.
   */
 -static int is_refname_available(const char *refname,
 -                              const struct string_list *skip,
 -                              struct ref_dir *dir)
 +static int verify_refname_available(const char *refname,
 +                                  const struct string_list *extras,
 +                                  const struct string_list *skip,
 +                                  struct ref_dir *dir,
 +                                  struct strbuf *err)
  {
        const char *slash;
 -      size_t len;
        int pos;
 -      char *dirname;
 +      struct strbuf dirname = STRBUF_INIT;
 +      int ret = -1;
 +
 +      /*
 +       * For the sake of comments in this function, suppose that
 +       * refname is "refs/foo/bar".
 +       */
 +
 +      assert(err);
  
 +      strbuf_grow(&dirname, strlen(refname) + 1);
        for (slash = strchr(refname, '/'); slash; slash = strchr(slash + 1, '/')) {
 +              /* Expand dirname to the new prefix, not including the trailing slash: */
 +              strbuf_add(&dirname, refname + dirname.len, slash - refname - dirname.len);
 +
                /*
 -               * We are still at a leading dir of the refname; we are
 -               * looking for a conflict with a leaf entry.
 -               *
 -               * If we find one, we still must make sure it is
 -               * not in "skip".
 +               * We are still at a leading dir of the refname (e.g.,
 +               * "refs/foo"; if there is a reference with that name,
 +               * it is a conflict, *unless* it is in skip.
                 */
 -              pos = search_ref_dir(dir, refname, slash - refname);
 -              if (pos >= 0) {
 -                      struct ref_entry *entry = dir->entries[pos];
 -                      if (entry_matches(entry, skip))
 -                              return 1;
 -                      report_refname_conflict(entry, refname);
 -                      return 0;
 +              if (dir) {
 +                      pos = search_ref_dir(dir, dirname.buf, dirname.len);
 +                      if (pos >= 0 &&
 +                          (!skip || !string_list_has_string(skip, dirname.buf))) {
 +                              /*
 +                               * We found a reference whose name is
 +                               * a proper prefix of refname; e.g.,
 +                               * "refs/foo", and is not in skip.
 +                               */
 +                              strbuf_addf(err, "'%s' exists; cannot create '%s'",
 +                                          dirname.buf, refname);
 +                              goto cleanup;
 +                      }
                }
  
 +              if (extras && string_list_has_string(extras, dirname.buf) &&
 +                  (!skip || !string_list_has_string(skip, dirname.buf))) {
 +                      strbuf_addf(err, "cannot process '%s' and '%s' at the same time",
 +                                  refname, dirname.buf);
 +                      goto cleanup;
 +              }
  
                /*
                 * Otherwise, we can try to continue our search with
 -               * the next component; if we come up empty, we know
 -               * there is nothing under this whole prefix.
 +               * the next component. So try to look up the
 +               * directory, e.g., "refs/foo/". If we come up empty,
 +               * we know there is nothing under this whole prefix,
 +               * but even in that case we still have to continue the
 +               * search for conflicts with extras.
                 */
 -              pos = search_ref_dir(dir, refname, slash + 1 - refname);
 -              if (pos < 0)
 -                      return 1;
 -
 -              dir = get_ref_dir(dir->entries[pos]);
 +              strbuf_addch(&dirname, '/');
 +              if (dir) {
 +                      pos = search_ref_dir(dir, dirname.buf, dirname.len);
 +                      if (pos < 0) {
 +                              /*
 +                               * There was no directory "refs/foo/",
 +                               * so there is nothing under this
 +                               * whole prefix. So there is no need
 +                               * to continue looking for conflicting
 +                               * references. But we need to continue
 +                               * looking for conflicting extras.
 +                               */
 +                              dir = NULL;
 +                      } else {
 +                              dir = get_ref_dir(dir->entries[pos]);
 +                      }
 +              }
        }
  
        /*
 -       * We are at the leaf of our refname; we want to
 -       * make sure there are no directories which match it.
 +       * We are at the leaf of our refname (e.g., "refs/foo/bar").
 +       * There is no point in searching for a reference with that
 +       * name, because a refname isn't considered to conflict with
 +       * itself. But we still need to check for references whose
 +       * names are in the "refs/foo/bar/" namespace, because they
 +       * *do* conflict.
         */
 -      len = strlen(refname);
 -      dirname = xmallocz(len + 1);
 -      sprintf(dirname, "%s/", refname);
 -      pos = search_ref_dir(dir, dirname, len + 1);
 -      free(dirname);
 +      strbuf_addstr(&dirname, refname + dirname.len);
 +      strbuf_addch(&dirname, '/');
 +
 +      if (dir) {
 +              pos = search_ref_dir(dir, dirname.buf, dirname.len);
  
 -      if (pos >= 0) {
 +              if (pos >= 0) {
 +                      /*
 +                       * We found a directory named "$refname/"
 +                       * (e.g., "refs/foo/bar/"). It is a problem
 +                       * iff it contains any ref that is not in
 +                       * "skip".
 +                       */
 +                      struct nonmatching_ref_data data;
 +
 +                      data.skip = skip;
 +                      data.conflicting_refname = NULL;
 +                      dir = get_ref_dir(dir->entries[pos]);
 +                      sort_ref_dir(dir);
 +                      if (do_for_each_entry_in_dir(dir, 0, nonmatching_ref_fn, &data)) {
 +                              strbuf_addf(err, "'%s' exists; cannot create '%s'",
 +                                          data.conflicting_refname, refname);
 +                              goto cleanup;
 +                      }
 +              }
 +      }
 +
 +      if (extras) {
                /*
 -               * We found a directory named "refname". It is a
 -               * problem iff it contains any ref that is not
 -               * in "skip".
 +               * Check for entries in extras that start with
 +               * "$refname/". We do that by looking for the place
 +               * where "$refname/" would be inserted in extras. If
 +               * there is an entry at that position that starts with
 +               * "$refname/" and is not in skip, then we have a
 +               * conflict.
                 */
 -              struct ref_entry *entry = dir->entries[pos];
 -              struct ref_dir *dir = get_ref_dir(entry);
 -              struct nonmatching_ref_data data;
 +              for (pos = string_list_find_insert_index(extras, dirname.buf, 0);
 +                   pos < extras->nr; pos++) {
 +                      const char *extra_refname = extras->items[pos].string;
  
 -              data.skip = skip;
 -              sort_ref_dir(dir);
 -              if (!do_for_each_entry_in_dir(dir, 0, nonmatching_ref_fn, &data))
 -                      return 1;
 +                      if (!starts_with(extra_refname, dirname.buf))
 +                              break;
  
 -              report_refname_conflict(data.found, refname);
 -              return 0;
 +                      if (!skip || !string_list_has_string(skip, extra_refname)) {
 +                              strbuf_addf(err, "cannot process '%s' and '%s' at the same time",
 +                                          refname, extra_refname);
 +                              goto cleanup;
 +                      }
 +              }
        }
  
 -      /*
 -       * There is no point in searching for another leaf
 -       * node which matches it; such an entry would be the
 -       * ref we are looking for, not a conflict.
 -       */
 -      return 1;
 +      /* No conflicts were found */
 +      ret = 0;
 +
 +cleanup:
 +      strbuf_release(&dirname);
 +      return ret;
  }
  
  struct packed_ref_cache {
@@@ -2354,10 -2289,8 +2354,10 @@@ 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 *extras,
                                            const struct string_list *skip,
 -                                          unsigned int flags, int *type_p)
 +                                          unsigned int flags, int *type_p,
 +                                          struct strbuf *err)
  {
        const char *ref_file;
        const char *orig_refname = refname;
        int resolve_flags = 0;
        int attempts_remaining = 3;
  
 +      assert(err);
 +
        lock = xcalloc(1, sizeof(struct ref_lock));
 -      lock->lock_fd = -1;
  
        if (mustexist)
                resolve_flags |= RESOLVE_REF_READING;
                ref_file = git_path("%s", orig_refname);
                if (remove_empty_directories(ref_file)) {
                        last_errno = errno;
 -                      error("there are still refs under '%s'", orig_refname);
 +
 +                      if (!verify_refname_available(orig_refname, extras, skip,
 +                                                    get_loose_refs(&ref_cache), err))
 +                              strbuf_addf(err, "there are still refs under '%s'",
 +                                          orig_refname);
 +
                        goto error_return;
                }
                refname = resolve_ref_unsafe(orig_refname, resolve_flags,
            *type_p = type;
        if (!refname) {
                last_errno = errno;
 -              error("unable to resolve reference %s: %s",
 -                      orig_refname, strerror(errno));
 +              if (last_errno != ENOTDIR ||
 +                  !verify_refname_available(orig_refname, extras, skip,
 +                                            get_loose_refs(&ref_cache), err))
 +                      strbuf_addf(err, "unable to resolve reference %s: %s",
 +                                  orig_refname, strerror(last_errno));
 +
                goto error_return;
        }
        /*
         * our refname.
         */
        if (is_null_sha1(lock->old_sha1) &&
 -           !is_refname_available(refname, skip, get_packed_refs(&ref_cache))) {
 +          verify_refname_available(refname, extras, skip,
 +                                   get_packed_refs(&ref_cache), err)) {
                last_errno = ENOTDIR;
                goto error_return;
        }
                /* fall through */
        default:
                last_errno = errno;
 -              error("unable to create directory for %s", ref_file);
 +              strbuf_addf(err, "unable to create directory for %s", ref_file);
                goto error_return;
        }
  
 -      lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);
 -      if (lock->lock_fd < 0) {
 +      if (hold_lock_file_for_update(lock->lk, ref_file, lflags) < 0) {
                last_errno = errno;
                if (errno == ENOENT && --attempts_remaining > 0)
                        /*
                         */
                        goto retry;
                else {
 -                      struct strbuf err = STRBUF_INIT;
 -                      unable_to_lock_message(ref_file, errno, &err);
 -                      error("%s", err.buf);
 -                      strbuf_release(&err);
 +                      unable_to_lock_message(ref_file, errno, err);
                        goto error_return;
                }
        }
@@@ -2505,9 -2431,19 +2505,19 @@@ static int write_packed_entry_fn(struc
  /* This should return a meaningful errno on failure */
  int lock_packed_refs(int flags)
  {
+       static int timeout_configured = 0;
+       static int timeout_value = 1000;
        struct packed_ref_cache *packed_ref_cache;
  
-       if (hold_lock_file_for_update(&packlock, git_path("packed-refs"), flags) < 0)
+       if (!timeout_configured) {
+               git_config_get_int("core.packedrefstimeout", &timeout_value);
+               timeout_configured = 1;
+       }
+       if (hold_lock_file_for_update_timeout(
+                           &packlock, git_path("packed-refs"),
+                           flags, timeout_value) < 0)
                return -1;
        /*
         * Get the current packed-refs while holding the lock.  If the
@@@ -2856,25 -2792,17 +2866,25 @@@ static int rename_tmp_log(const char *n
  static int rename_ref_available(const char *oldname, const char *newname)
  {
        struct string_list skip = STRING_LIST_INIT_NODUP;
 +      struct strbuf err = STRBUF_INIT;
        int ret;
  
        string_list_insert(&skip, oldname);
 -      ret = is_refname_available(newname, &skip, get_packed_refs(&ref_cache))
 -          && is_refname_available(newname, &skip, get_loose_refs(&ref_cache));
 +      ret = !verify_refname_available(newname, NULL, &skip,
 +                                      get_packed_refs(&ref_cache), &err)
 +              && !verify_refname_available(newname, NULL, &skip,
 +                                           get_loose_refs(&ref_cache), &err);
 +      if (!ret)
 +              error("%s", err.buf);
 +
        string_list_clear(&skip, 0);
 +      strbuf_release(&err);
        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)
  {
        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);
  
        logmoved = log;
  
 -      lock = lock_ref_sha1_basic(newrefname, NULL, NULL, 0, NULL);
 +      lock = lock_ref_sha1_basic(newrefname, NULL, NULL, NULL, 0, NULL, &err);
        if (!lock) {
 -              error("unable to lock %s for update", newrefname);
 +              error("unable to rename '%s' to '%s': %s", oldrefname, newrefname, err.buf);
 +              strbuf_release(&err);
                goto rollback;
        }
        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;
        }
        return 0;
  
   rollback:
 -      lock = lock_ref_sha1_basic(oldrefname, NULL, NULL, 0, NULL);
 +      lock = lock_ref_sha1_basic(oldrefname, NULL, NULL, NULL, 0, NULL, &err);
        if (!lock) {
 -              error("unable to lock %s for rollback", oldrefname);
 +              error("unable to lock %s for rollback: %s", oldrefname, err.buf);
 +              strbuf_release(&err);
                goto rollbacklog;
        }
  
        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;
  
@@@ -2974,6 -2896,7 +2984,6 @@@ static int close_ref(struct ref_lock *l
  {
        if (close_lock_file(lock->lk))
                return -1;
 -      lock->lock_fd = -1;
        return 0;
  }
  
@@@ -2981,6 -2904,7 +2991,6 @@@ static int commit_ref(struct ref_lock *
  {
        if (commit_lock_file(lock->lk))
                return -1;
 -      lock->lock_fd = -1;
        return 0;
  }
  
@@@ -3143,11 -3067,11 +3153,11 @@@ 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;
                errno = EINVAL;
                return -1;
        }
 -      if (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 ||
 -          write_in_full(lock->lock_fd, &term, 1) != 1 ||
 +      if (write_in_full(lock->lk->fd, sha1_to_hex(sha1), 40) != 40 ||
 +          write_in_full(lock->lk->fd, &term, 1) != 1 ||
            close_ref(lock) < 0) {
                int save_errno = errno;
                error("Couldn't write %s", lock->lk->filename.buf);
                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) &&
@@@ -3827,18 -3740,25 +3837,18 @@@ int update_ref(const char *msg, const c
        return 0;
  }
  
 -static int ref_update_compare(const void *r1, const void *r2)
 -{
 -      const struct ref_update * const *u1 = r1;
 -      const struct ref_update * const *u2 = r2;
 -      return strcmp((*u1)->refname, (*u2)->refname);
 -}
 -
 -static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 +static int ref_update_reject_duplicates(struct string_list *refnames,
                                        struct strbuf *err)
  {
 -      int i;
 +      int i, n = refnames->nr;
  
        assert(err);
  
        for (i = 1; i < n; i++)
 -              if (!strcmp(updates[i - 1]->refname, updates[i]->refname)) {
 +              if (!strcmp(refnames->items[i - 1].string, refnames->items[i].string)) {
                        strbuf_addf(err,
                                    "Multiple updates for ref '%s' not allowed.",
 -                                  updates[i]->refname);
 +                                  refnames->items[i].string);
                        return 1;
                }
        return 0;
@@@ -3852,7 -3772,6 +3862,7 @@@ int ref_transaction_commit(struct ref_t
        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;
  
        assert(err);
  
                return 0;
        }
  
 -      /* Copy, sort, and reject duplicate refs */
 -      qsort(updates, n, sizeof(*updates), ref_update_compare);
 -      if (ref_update_reject_duplicates(updates, n, err)) {
 +      /* 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);
 +      string_list_sort(&affected_refnames);
 +      if (ref_update_reject_duplicates(&affected_refnames, err)) {
                ret = TRANSACTION_GENERIC_ERROR;
                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];
 -              unsigned int flags = update->flags;
  
 -              if ((flags & REF_HAVE_NEW) && is_null_sha1(update->new_sha1))
 -                      flags |= REF_DELETING;
 +              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),
 -                              NULL,
 -                              flags,
 -                              &update->type);
 +                              &affected_refnames, NULL,
 +                              update->flags,
 +                              &update->type,
 +                              err);
                if (!update->lock) {
 +                      char *reason;
 +
                        ret = (errno == ENOTDIR)
                                ? TRANSACTION_NAME_CONFLICT
                                : TRANSACTION_GENERIC_ERROR;
 -                      strbuf_addf(err, "Cannot lock the ref '%s'.",
 -                                  update->refname);
 +                      reason = strbuf_detach(err, NULL);
 +                      strbuf_addf(err, "Cannot lock ref '%s': %s",
 +                                  update->refname, reason);
 +                      free(reason);
                        goto cleanup;
                }
 -      }
 -
 -      /* 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 ((flags & REF_HAVE_NEW) && !is_null_sha1(update->new_sha1)) {
 +              if ((update->flags & REF_HAVE_NEW) &&
 +                  !(update->flags & REF_DELETING)) {
                        int overwriting_symref = ((update->type & REF_ISSYMREF) &&
                                                  (update->flags & REF_NODEREF));
  
 -                      if (!overwriting_symref
 -                          && !hashcmp(update->lock->old_sha1, update->new_sha1)) {
 +                      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);
 +                      } else if (write_ref_to_lockfile(update->lock,
 +                                                       update->new_sha1)) {
 +                              /*
 +                               * The lock was freed upon failure of
 +                               * write_ref_to_lockfile():
 +                               */
                                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->flags |= REF_NEEDS_COMMIT;
 +                      }
 +              }
 +              if (!(update->flags & REF_NEEDS_COMMIT)) {
 +                      /*
 +                       * 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 (update->flags & REF_NEEDS_COMMIT) {
 +                      if (commit_ref_update(update->lock,
 +                                            update->new_sha1, update->msg)) {
 +                              /* 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 commit_ref_update(): */
                                update->lock = NULL;
                        }
                }
        /* 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 ((flags & REF_HAVE_NEW) && is_null_sha1(update->new_sha1)) {
 +              if (update->flags & REF_DELETING) {
                        if (delete_ref_loose(update->lock, update->type, err)) {
                                ret = TRANSACTION_GENERIC_ERROR;
                                goto cleanup;
                        }
  
 -                      if (!(flags & REF_ISPRUNING))
 +                      if (!(update->flags & REF_ISPRUNING))
                                string_list_append(&refs_to_delete,
                                                   update->lock->ref_name);
                }
@@@ -3995,7 -3877,6 +4005,7 @@@ cleanup
                if (updates[i]->lock)
                        unlock_ref(updates[i]->lock);
        string_list_clear(&refs_to_delete, 0);
 +      string_list_clear(&affected_refnames, 0);
        return ret;
  }
  
@@@ -4185,7 -4066,6 +4195,7 @@@ int reflog_expire(const char *refname, 
        char *log_file;
        int status = 0;
        int type;
 +      struct strbuf err = STRBUF_INIT;
  
        memset(&cb, 0, sizeof(cb));
        cb.flags = flags;
         * reference itself, plus we might need to update the
         * reference if --updateref was specified:
         */
 -      lock = lock_ref_sha1_basic(refname, sha1, NULL, 0, &type);
 -      if (!lock)
 -              return error("cannot lock ref '%s'", refname);
 +      lock = lock_ref_sha1_basic(refname, sha1, NULL, NULL, 0, &type, &err);
 +      if (!lock) {
 +              error("cannot lock ref '%s': %s", refname, err.buf);
 +              strbuf_release(&err);
 +              return -1;
 +      }
        if (!reflog_exists(refname)) {
                unlock_ref(lock);
                return 0;
                        status |= error("couldn't write %s: %s", log_file,
                                        strerror(errno));
                } else if (update &&
 -                      (write_in_full(lock->lock_fd,
 +                         (write_in_full(lock->lk->fd,
                                sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
 -                       write_str_in_full(lock->lock_fd, "\n") != 1 ||
 +                       write_str_in_full(lock->lk->fd, "\n") != 1 ||
                         close_ref(lock) < 0)) {
                        status |= error("couldn't write %s",
                                        lock->lk->filename.buf);