Merge branch 'mh/write-refs-sooner-2.4'
authorJunio C Hamano <gitster@pobox.com>
Fri, 22 May 2015 19:41:52 +0000 (12:41 -0700)
committerJunio C Hamano <gitster@pobox.com>
Fri, 22 May 2015 19:41:52 +0000 (12:41 -0700)
Multi-ref transaction support we merged a few releases ago
unnecessarily kept many file descriptors open, risking to fail with
resource exhaustion. This is for 2.4.x track.

* mh/write-refs-sooner-2.4:
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
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

1  2 
refs.c
diff --combined refs.c
index b5189f4a574c555ced4f24bc0e12b2702ae2e5b4,96f50467125dad661a79c60e13ecf5b389c4ef10..825a1f6847ccefb3aba29e489c05c96382fff907
--- 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,6 -57,12 +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
@@@ -343,6 -350,8 +349,6 @@@ static struct ref_entry *create_ref_ent
        if (check_name &&
            check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
                die("Reference has invalid format: '%s'", refname);
 -      if (!check_name && !refname_is_safe(refname))
 -              die("Reference has invalid name: '%s'", refname);
        len = strlen(refname) + 1;
        ref = xmalloc(sizeof(struct ref_entry) + len);
        hashcpy(ref->u.value.sha1, sha1);
@@@ -1175,8 -1184,6 +1181,8 @@@ static void read_packed_refs(FILE *f, s
                        int flag = REF_ISPACKED;
  
                        if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
 +                              if (!refname_is_safe(refname))
 +                                      die("packed refname is dangerous: %s", refname);
                                hashclr(sha1);
                                flag |= REF_BAD_NAME | REF_ISBROKEN;
                        }
@@@ -1322,8 -1329,6 +1328,8 @@@ static void read_loose_refs(const char 
                        }
                        if (check_refname_format(refname.buf,
                                                 REFNAME_ALLOW_ONELEVEL)) {
 +                              if (!refname_is_safe(refname.buf))
 +                                      die("loose refname is dangerous: %s", refname.buf);
                                hashclr(sha1);
                                flag |= REF_BAD_NAME | REF_ISBROKEN;
                        }
@@@ -1383,7 -1388,7 +1389,7 @@@ static int resolve_gitlink_ref_recursiv
  {
        int fd, len;
        char buffer[128], *p;
 -      char *path;
 +      const char *path;
  
        if (recursion > MAXDEPTH || strlen(refname) > MAXREFLEN)
                return -1;
@@@ -1476,11 -1481,7 +1482,11 @@@ static int resolve_missing_loose_ref(co
  }
  
  /* This function needs to return a meaningful errno on failure */
 -const char *resolve_ref_unsafe(const char *refname, int resolve_flags, unsigned char *sha1, int *flags)
 +static const char *resolve_ref_unsafe_1(const char *refname,
 +                                      int resolve_flags,
 +                                      unsigned char *sha1,
 +                                      int *flags,
 +                                      struct strbuf *sb_path)
  {
        int depth = MAXDEPTH;
        ssize_t len;
                bad_name = 1;
        }
        for (;;) {
 -              char path[PATH_MAX];
 +              const char *path;
                struct stat st;
                char *buf;
                int fd;
                        return NULL;
                }
  
 -              git_snpath(path, sizeof(path), "%s", refname);
 +              strbuf_reset(sb_path);
 +              strbuf_git_path(sb_path, "%s", refname);
 +              path = sb_path->buf;
  
                /*
                 * We might have to loop back here to avoid a race
        }
  }
  
 +const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
 +                             unsigned char *sha1, int *flags)
 +{
 +      struct strbuf sb_path = STRBUF_INIT;
 +      const char *ret = resolve_ref_unsafe_1(refname, resolve_flags,
 +                                             sha1, flags, &sb_path);
 +      strbuf_release(&sb_path);
 +      return ret;
 +}
 +
  char *resolve_refdup(const char *ref, int resolve_flags, unsigned char *sha1, int *flags)
  {
        return xstrdup_or_null(resolve_ref_unsafe(ref, resolve_flags, sha1, flags));
@@@ -2291,7 -2280,7 +2297,7 @@@ static struct ref_lock *lock_ref_sha1_b
                                            const struct string_list *skip,
                                            unsigned int flags, int *type_p)
  {
 -      char *ref_file;
 +      const char *ref_file;
        const char *orig_refname = refname;
        struct ref_lock *lock;
        int last_errno = 0;
        int attempts_remaining = 3;
  
        lock = xcalloc(1, sizeof(struct ref_lock));
 -      lock->lock_fd = -1;
  
        if (mustexist)
                resolve_flags |= RESOLVE_REF_READING;
        ref_file = git_path("%s", refname);
  
   retry:
 -      switch (safe_create_leading_directories(ref_file)) {
 +      switch (safe_create_leading_directories_const(ref_file)) {
        case SCLD_OK:
                break; /* success */
        case SCLD_VANISHED:
                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)
                        /*
@@@ -2736,7 -2727,7 +2742,7 @@@ static int rename_tmp_log(const char *n
        int attempts_remaining = 4;
  
   retry:
 -      switch (safe_create_leading_directories(git_path("logs/%s", newrefname))) {
 +      switch (safe_create_leading_directories_const(git_path("logs/%s", newrefname))) {
        case SCLD_OK:
                break; /* success */
        case SCLD_VANISHED:
@@@ -2788,8 -2779,9 +2794,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)
  {
                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;
        }
  
        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;
  
@@@ -2883,6 -2878,7 +2893,6 @@@ static int close_ref(struct ref_lock *l
  {
        if (close_lock_file(lock->lk))
                return -1;
 -      lock->lock_fd = -1;
        return 0;
  }
  
@@@ -2890,6 -2886,7 +2900,6 @@@ static int commit_ref(struct ref_lock *
  {
        if (commit_lock_file(lock->lk))
                return -1;
 -      lock->lock_fd = -1;
        return 0;
  }
  
@@@ -2920,15 -2917,11 +2930,15 @@@ static int copy_msg(char *buf, const ch
  }
  
  /* This function must set a meaningful errno on failure */
 -int log_ref_setup(const char *refname, char *logfile, int bufsize)
 +int log_ref_setup(const char *refname, struct strbuf *sb_logfile)
  {
        int logfd, oflags = O_APPEND | O_WRONLY;
 +      char *logfile;
  
 -      git_snpath(logfile, bufsize, "logs/%s", refname);
 +      strbuf_git_path(sb_logfile, "logs/%s", refname);
 +      logfile = sb_logfile->buf;
 +      /* make sure the rest of the function can't change "logfile" */
 +      sb_logfile = NULL;
        if (log_all_ref_updates &&
            (starts_with(refname, "refs/heads/") ||
             starts_with(refname, "refs/remotes/") ||
@@@ -2999,22 -2992,18 +3009,22 @@@ static int log_ref_write_fd(int fd, con
        return 0;
  }
  
 -static int log_ref_write(const char *refname, const unsigned char *old_sha1,
 -                       const unsigned char *new_sha1, const char *msg)
 +static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
 +                         const unsigned char *new_sha1, const char *msg,
 +                         struct strbuf *sb_log_file)
  {
        int logfd, result, oflags = O_APPEND | O_WRONLY;
 -      char log_file[PATH_MAX];
 +      char *log_file;
  
        if (log_all_ref_updates < 0)
                log_all_ref_updates = !is_bare_repository();
  
 -      result = log_ref_setup(refname, log_file, sizeof(log_file));
 +      result = log_ref_setup(refname, sb_log_file);
        if (result)
                return result;
 +      log_file = sb_log_file->buf;
 +      /* make sure the rest of the function can't change "log_file" */
 +      sb_log_file = NULL;
  
        logfd = open(log_file, oflags);
        if (logfd < 0)
        return 0;
  }
  
 +static int log_ref_write(const char *refname, const unsigned char *old_sha1,
 +                       const unsigned char *new_sha1, const char *msg)
 +{
 +      struct strbuf sb = STRBUF_INIT;
 +      int ret = log_ref_write_1(refname, old_sha1, new_sha1, msg, &sb);
 +      strbuf_release(&sb);
 +      return ret;
 +}
 +
  int is_branch(const char *refname)
  {
        return !strcmp(refname, "HEAD") || starts_with(refname, "refs/heads/");
  }
  
  /*
-  * 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) &&
@@@ -3775,19 -3766,24 +3796,24 @@@ int ref_transaction_commit(struct ref_t
                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->flags,
                                &update->type);
                if (!update->lock) {
                        ret = (errno == ENOTDIR)
                                    update->refname);
                        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;
+                               strbuf_addf(err, "Cannot update the ref '%s'.",
+                                           update->refname);
+                               ret = TRANSACTION_GENERIC_ERROR;
+                               goto cleanup;
+                       } else {
+                               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;
-                       } 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(): */
+                               /* 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);
                }
@@@ -4114,9 -4135,9 +4165,9 @@@ int reflog_expire(const char *refname, 
                        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);