Merge branch 'mh/write-refs-sooner-2.2' into mh/write-refs-sooner-2.3
authorJunio C Hamano <gitster@pobox.com>
Wed, 13 May 2015 04:26:09 +0000 (21:26 -0700)
committerJunio C Hamano <gitster@pobox.com>
Wed, 13 May 2015 04:26:09 +0000 (21:26 -0700)
* mh/write-refs-sooner-2.2:
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
t/t1400-update-ref.sh
t/t7004-tag.sh
diff --combined refs.c
index 3a26ad4e65b92bc9398766169f9a236e4be0d08d,85c1dcb017e13be7fd3e9baac753f92773d544bb..c689af6ccf6c0540adc322847b593309ed751b4f
--- 1/refs.c
--- 2/refs.c
+++ b/refs.c
@@@ -30,6 -30,13 +30,13 @@@ static unsigned char refname_dispositio
   * pruned.
   */
  #define REF_ISPRUNING 0x0100
+ /*
+  * Used as a flag in ref_update::flags when the lockfile needs to be
+  * committed.
+  */
+ #define REF_NEEDS_COMMIT 0x0200
  /*
   * Try to read one refname component from the front of refname.
   * Return the length of the component found, or -1 if the component is
@@@ -1618,7 -1625,8 +1625,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 */
@@@ -1907,11 -1915,6 +1914,11 @@@ static int do_for_each_ref(struct ref_c
        data.fn = fn;
        data.cb_data = cb_data;
  
 +      if (ref_paranoia < 0)
 +              ref_paranoia = git_env_bool("GIT_REF_PARANOIA", 0);
 +      if (ref_paranoia)
 +              data.flags |= DO_FOR_EACH_INCLUDE_BROKEN;
 +
        return do_for_each_entry(refs, base, do_one_ref, &data);
  }
  
@@@ -2326,7 -2329,6 +2333,7 @@@ static struct ref_lock *lock_ref_sha1_b
  
        lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);
        if (lock->lock_fd < 0) {
 +              last_errno = errno;
                if (errno == ENOENT && --attempts_remaining > 0)
                        /*
                         * Maybe somebody just deleted one of the
                         * again:
                         */
                        goto retry;
 -              else
 -                      unable_to_lock_die(ref_file, errno);
 +              else {
 +                      struct strbuf err = STRBUF_INIT;
 +                      unable_to_lock_message(ref_file, errno, &err);
 +                      error("%s", err.buf);
 +                      strbuf_release(&err);
 +                      goto error_return;
 +              }
        }
        return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;
  
@@@ -2596,24 -2593,79 +2603,24 @@@ int pack_refs(unsigned int flags
        return 0;
  }
  
 -/*
 - * If entry is no longer needed in packed-refs, add it to the string
 - * list pointed to by cb_data.  Reasons for deleting entries:
 - *
 - * - Entry is broken.
 - * - Entry is overridden by a loose ref.
 - * - Entry does not point at a valid object.
 - *
 - * In the first and third cases, also emit an error message because these
 - * are indications of repository corruption.
 - */
 -static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)
 -{
 -      struct string_list *refs_to_delete = cb_data;
 -
 -      if (entry->flag & REF_ISBROKEN) {
 -              /* This shouldn't happen to packed refs. */
 -              error("%s is broken!", entry->name);
 -              string_list_append(refs_to_delete, entry->name);
 -              return 0;
 -      }
 -      if (!has_sha1_file(entry->u.value.sha1)) {
 -              unsigned char sha1[20];
 -              int flags;
 -
 -              if (read_ref_full(entry->name, 0, sha1, &flags))
 -                      /* We should at least have found the packed ref. */
 -                      die("Internal error");
 -              if ((flags & REF_ISSYMREF) || !(flags & REF_ISPACKED)) {
 -                      /*
 -                       * This packed reference is overridden by a
 -                       * loose reference, so it is OK that its value
 -                       * is no longer valid; for example, it might
 -                       * refer to an object that has been garbage
 -                       * collected.  For this purpose we don't even
 -                       * care whether the loose reference itself is
 -                       * invalid, broken, symbolic, etc.  Silently
 -                       * remove the packed reference.
 -                       */
 -                      string_list_append(refs_to_delete, entry->name);
 -                      return 0;
 -              }
 -              /*
 -               * There is no overriding loose reference, so the fact
 -               * that this reference doesn't refer to a valid object
 -               * indicates some kind of repository corruption.
 -               * Report the problem, then omit the reference from
 -               * the output.
 -               */
 -              error("%s does not point to a valid object!", entry->name);
 -              string_list_append(refs_to_delete, entry->name);
 -              return 0;
 -      }
 -
 -      return 0;
 -}
 -
 -int repack_without_refs(const char **refnames, int n, struct strbuf *err)
 +int repack_without_refs(struct string_list *refnames, struct strbuf *err)
  {
        struct ref_dir *packed;
 -      struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
 -      struct string_list_item *ref_to_delete;
 -      int i, ret, removed = 0;
 +      struct string_list_item *refname;
 +      int ret, needs_repacking = 0, removed = 0;
  
        assert(err);
  
        /* Look for a packed ref */
 -      for (i = 0; i < n; i++)
 -              if (get_packed_ref(refnames[i]))
 +      for_each_string_list_item(refname, refnames) {
 +              if (get_packed_ref(refname->string)) {
 +                      needs_repacking = 1;
                        break;
 +              }
 +      }
  
        /* Avoid locking if we have nothing to do */
 -      if (i == n)
 +      if (!needs_repacking)
                return 0; /* no refname exists in packed refs */
  
        if (lock_packed_refs(0)) {
        packed = get_packed_refs(&ref_cache);
  
        /* Remove refnames from the cache */
 -      for (i = 0; i < n; i++)
 -              if (remove_entry(packed, refnames[i]) != -1)
 +      for_each_string_list_item(refname, refnames)
 +              if (remove_entry(packed, refname->string) != -1)
                        removed = 1;
        if (!removed) {
                /*
                return 0;
        }
  
 -      /* Remove any other accumulated cruft */
 -      do_for_each_entry_in_dir(packed, 0, curate_packed_ref_fn, &refs_to_delete);
 -      for_each_string_list_item(ref_to_delete, &refs_to_delete) {
 -              if (remove_entry(packed, ref_to_delete->string) == -1)
 -                      die("internal error");
 -      }
 -
        /* Write what remains */
        ret = commit_packed_refs();
        if (ret)
@@@ -2747,8 -2806,9 +2754,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)
  {
        }
        lock->force_write = 1;
        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;
        }
        lock->force_write = 1;
        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;
  
@@@ -2996,23 -3059,15 +3007,15 @@@ 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;
  
-       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",
                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) &&
@@@ -3715,11 -3781,10 +3729,11 @@@ static int ref_update_reject_duplicates
  int ref_transaction_commit(struct ref_transaction *transaction,
                           struct strbuf *err)
  {
 -      int ret = 0, delnum = 0, i;
 -      const char **delnames;
 +      int ret = 0, i;
        int n = transaction->nr;
        struct ref_update **updates = transaction->updates;
 +      struct string_list refs_to_delete = STRING_LIST_INIT_NODUP;
 +      struct string_list_item *ref_to_delete;
  
        assert(err);
  
                return 0;
        }
  
 -      /* Allocate work space */
 -      delnames = xmalloc(sizeof(*delnames) * n);
 -
        /* Copy, sort, and reject duplicate refs */
        qsort(updates, n, sizeof(*updates), ref_update_compare);
        if (ref_update_reject_duplicates(updates, n, err)) {
                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];
-               int flags = update->flags;
  
                if (is_null_sha1(update->new_sha1))
-                       flags |= REF_DELETING;
+                       update->flags |= REF_DELETING;
                update->lock = lock_ref_sha1_basic(update->refname,
                                                   (update->have_old ?
                                                    update->old_sha1 :
                                                    NULL),
                                                   NULL,
-                                                  flags,
+                                                  update->flags,
                                                   &update->type);
                if (!update->lock) {
                        ret = (errno == ENOTDIR)
                                    update->refname);
                        goto cleanup;
                }
+               if (!(update->flags & REF_DELETING) &&
+                   (update->lock->force_write ||
+                    hashcmp(update->lock->old_sha1, update->new_sha1))) {
+                       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;
+                       }
+                       update->flags |= REF_NEEDS_COMMIT;
+               } else {
+                       /*
+                        * 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 (!is_null_sha1(update->new_sha1)) {
-                       if (write_ref_sha1(update->lock, update->new_sha1,
-                                          update->msg)) {
-                               update->lock = NULL; /* freed by write_ref_sha1 */
+               if (update->flags & REF_NEEDS_COMMIT) {
+                       if (commit_ref_update(update->lock,
+                                             update->new_sha1, update->msg)) {
+                               /* The lock was 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 the above call: */
+                               update->lock = NULL;
                        }
-                       update->lock = NULL; /* freed by write_ref_sha1 */
                }
        }
  
        for (i = 0; i < n; i++) {
                struct ref_update *update = updates[i];
  
-               if (update->lock) {
+               if (update->flags & REF_DELETING) {
                        if (delete_ref_loose(update->lock, update->type, err)) {
                                ret = TRANSACTION_GENERIC_ERROR;
                                goto cleanup;
                        }
  
                        if (!(update->flags & REF_ISPRUNING))
 -                              delnames[delnum++] = update->lock->ref_name;
 +                              string_list_append(&refs_to_delete,
 +                                                 update->lock->ref_name);
                }
        }
  
 -      if (repack_without_refs(delnames, delnum, err)) {
 +      if (repack_without_refs(&refs_to_delete, err)) {
                ret = TRANSACTION_GENERIC_ERROR;
                goto cleanup;
        }
 -      for (i = 0; i < delnum; i++)
 -              unlink_or_warn(git_path("logs/%s", delnames[i]));
 +      for_each_string_list_item(ref_to_delete, &refs_to_delete)
 +              unlink_or_warn(git_path("logs/%s", ref_to_delete->string));
        clear_loose_ref_cache(&ref_cache);
  
  cleanup:
        for (i = 0; i < n; i++)
                if (updates[i]->lock)
                        unlock_ref(updates[i]->lock);
 -      free(delnames);
 +      string_list_clear(&refs_to_delete, 0);
        return ret;
  }
  
diff --combined t/t1400-update-ref.sh
index 6805b9e6bb5b7e6a6b6b9546e96023ebeb557bd0,c593a1df208aad2f07459a614fd8310b8b26ada0..636d3a167c4aa17bbb8f8b58d6a66142d90d1af2
@@@ -619,52 -619,6 +619,52 @@@ test_expect_success 'stdin update/creat
        test_must_fail git rev-parse --verify -q $c
  '
  
 +test_expect_success 'stdin verify succeeds for correct value' '
 +      git rev-parse $m >expect &&
 +      echo "verify $m $m" >stdin &&
 +      git update-ref --stdin <stdin &&
 +      git rev-parse $m >actual &&
 +      test_cmp expect actual
 +'
 +
 +test_expect_success 'stdin verify succeeds for missing reference' '
 +      echo "verify refs/heads/missing $Z" >stdin &&
 +      git update-ref --stdin <stdin &&
 +      test_must_fail git rev-parse --verify -q refs/heads/missing
 +'
 +
 +test_expect_success 'stdin verify treats no value as missing' '
 +      echo "verify refs/heads/missing" >stdin &&
 +      git update-ref --stdin <stdin &&
 +      test_must_fail git rev-parse --verify -q refs/heads/missing
 +'
 +
 +test_expect_success 'stdin verify fails for wrong value' '
 +      git rev-parse $m >expect &&
 +      echo "verify $m $m~1" >stdin &&
 +      test_must_fail git update-ref --stdin <stdin &&
 +      git rev-parse $m >actual &&
 +      test_cmp expect actual
 +'
 +
 +test_expect_success 'stdin verify fails for mistaken null value' '
 +      git rev-parse $m >expect &&
 +      echo "verify $m $Z" >stdin &&
 +      test_must_fail git update-ref --stdin <stdin &&
 +      git rev-parse $m >actual &&
 +      test_cmp expect actual
 +'
 +
 +test_expect_success 'stdin verify fails for mistaken empty value' '
 +      M=$(git rev-parse $m) &&
 +      test_when_finished "git update-ref $m $M" &&
 +      git rev-parse $m >expect &&
 +      echo "verify $m" >stdin &&
 +      test_must_fail git update-ref --stdin <stdin &&
 +      git rev-parse $m >actual &&
 +      test_cmp expect actual
 +'
 +
  test_expect_success 'stdin update refs works with identity updates' '
        cat >stdin <<-EOF &&
        update $a $m $m
@@@ -984,52 -938,6 +984,52 @@@ test_expect_success 'stdin -z update/cr
        test_must_fail git rev-parse --verify -q $c
  '
  
 +test_expect_success 'stdin -z verify succeeds for correct value' '
 +      git rev-parse $m >expect &&
 +      printf $F "verify $m" "$m" >stdin &&
 +      git update-ref -z --stdin <stdin &&
 +      git rev-parse $m >actual &&
 +      test_cmp expect actual
 +'
 +
 +test_expect_success 'stdin -z verify succeeds for missing reference' '
 +      printf $F "verify refs/heads/missing" "$Z" >stdin &&
 +      git update-ref -z --stdin <stdin &&
 +      test_must_fail git rev-parse --verify -q refs/heads/missing
 +'
 +
 +test_expect_success 'stdin -z verify treats no value as missing' '
 +      printf $F "verify refs/heads/missing" "" >stdin &&
 +      git update-ref -z --stdin <stdin &&
 +      test_must_fail git rev-parse --verify -q refs/heads/missing
 +'
 +
 +test_expect_success 'stdin -z verify fails for wrong value' '
 +      git rev-parse $m >expect &&
 +      printf $F "verify $m" "$m~1" >stdin &&
 +      test_must_fail git update-ref -z --stdin <stdin &&
 +      git rev-parse $m >actual &&
 +      test_cmp expect actual
 +'
 +
 +test_expect_success 'stdin -z verify fails for mistaken null value' '
 +      git rev-parse $m >expect &&
 +      printf $F "verify $m" "$Z" >stdin &&
 +      test_must_fail git update-ref -z --stdin <stdin &&
 +      git rev-parse $m >actual &&
 +      test_cmp expect actual
 +'
 +
 +test_expect_success 'stdin -z verify fails for mistaken empty value' '
 +      M=$(git rev-parse $m) &&
 +      test_when_finished "git update-ref $m $M" &&
 +      git rev-parse $m >expect &&
 +      printf $F "verify $m" "" >stdin &&
 +      test_must_fail git update-ref -z --stdin <stdin &&
 +      git rev-parse $m >actual &&
 +      test_cmp expect actual
 +'
 +
  test_expect_success 'stdin -z update refs works with identity updates' '
        printf $F "update $a" "$m" "$m" "update $b" "$m" "$m" "update $c" "$Z" "" >stdin &&
        git update-ref -z --stdin <stdin &&
@@@ -1065,4 -973,32 +1065,32 @@@ test_expect_success 'stdin -z delete re
        test_must_fail git rev-parse --verify -q $c
  '
  
+ run_with_limited_open_files () {
+       (ulimit -n 32 && "$@")
+ }
+ test_lazy_prereq ULIMIT_FILE_DESCRIPTORS 'run_with_limited_open_files true'
+ test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '
+ (
+       for i in $(test_seq 33)
+       do
+               echo "create refs/heads/$i HEAD"
+       done >large_input &&
+       run_with_limited_open_files git update-ref --stdin <large_input &&
+       git rev-parse --verify -q refs/heads/33
+ )
+ '
+ test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '
+ (
+       for i in $(test_seq 33)
+       do
+               echo "delete refs/heads/$i HEAD"
+       done >large_input &&
+       run_with_limited_open_files git update-ref --stdin <large_input &&
+       test_must_fail git rev-parse --verify -q refs/heads/33
+ )
+ '
  test_done
diff --combined t/t7004-tag.sh
index 35c805a44e817c637c667b1d1d14d97b4ee46803,06b8e0def185d3c6f99909ca37b05d26f51044c5..5446060ffb75302f6c4c876299a0c7cfa6cedaf4
@@@ -1081,7 -1081,7 +1081,7 @@@ test_expect_success GPG 
  get_tag_header rfc1991-signed-tag $commit commit $time >expect
  echo "RFC1991 signed tag" >>expect
  echo '-----BEGIN PGP MESSAGE-----' >>expect
 -test_expect_success GPG \
 +test_expect_success GPG,RFC1991 \
        'creating a signed tag with rfc1991' '
        echo "rfc1991" >gpghome/gpg.conf &&
        git tag -s -m "RFC1991 signed tag" rfc1991-signed-tag $commit &&
@@@ -1095,7 -1095,7 +1095,7 @@@ cp "$1" actua
  EOF
  chmod +x fakeeditor
  
 -test_expect_success GPG \
 +test_expect_success GPG,RFC1991 \
        'reediting a signed tag body omits signature' '
        echo "rfc1991" >gpghome/gpg.conf &&
        echo "RFC1991 signed tag" >expect &&
        test_cmp expect actual
  '
  
 -test_expect_success GPG \
 +test_expect_success GPG,RFC1991 \
        'verifying rfc1991 signature' '
        echo "rfc1991" >gpghome/gpg.conf &&
        git tag -v rfc1991-signed-tag
  '
  
 -test_expect_success GPG \
 +test_expect_success GPG,RFC1991 \
        'list tag with rfc1991 signature' '
        echo "rfc1991" >gpghome/gpg.conf &&
        echo "rfc1991-signed-tag RFC1991 signed tag" >expect &&
  
  rm -f gpghome/gpg.conf
  
 -test_expect_success GPG \
 +test_expect_success GPG,RFC1991 \
        'verifying rfc1991 signature without --rfc1991' '
        git tag -v rfc1991-signed-tag
  '
  
 -test_expect_success GPG \
 +test_expect_success GPG,RFC1991 \
        'list tag with rfc1991 signature without --rfc1991' '
        echo "rfc1991-signed-tag RFC1991 signed tag" >expect &&
        git tag -l -n1 rfc1991-signed-tag >actual &&
        test_cmp expect actual
  '
  
 -test_expect_success GPG \
 +test_expect_success GPG,RFC1991 \
        'reediting a signed tag body omits signature' '
        echo "RFC1991 signed tag" >expect &&
        GIT_EDITOR=./fakeeditor git tag -f -s rfc1991-signed-tag $commit &&
@@@ -1463,10 -1463,10 +1463,10 @@@ run_with_limited_stack () 
        (ulimit -s 128 && "$@")
  }
  
- test_lazy_prereq ULIMIT 'run_with_limited_stack true'
+ test_lazy_prereq ULIMIT_STACK_SIZE 'run_with_limited_stack true'
  
  # we require ulimit, this excludes Windows
- test_expect_success ULIMIT '--contains works in a deep repo' '
+ test_expect_success ULIMIT_STACK_SIZE '--contains works in a deep repo' '
        >expect &&
        i=1 &&
        while test $i -lt 8000