Merge branch 'mh/write-refs-sooner-2.2' into mh/write-refs-sooner-2.3

* 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

diff --git a/refs.c b/refs.c
index 3a26ad4e65b92bc9398766169f9a236e4be0d08d..c689af6ccf6c0540adc322847b593309ed751b4f 100644 (file)
--- a/refs.c
+++ b/refs.c
@@ -30,6 +30,13 @@ static unsigned char refname_disposition[256] = {
  * 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
@@ -2747,8 +2754,9 @@ static int rename_ref_available(const char *oldname, const char *newname)
        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)
 {
@@ -2807,7 +2815,9 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
        }
        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;
        }
@@ -2824,7 +2834,8 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
        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 +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",
@@ -3037,6 +3040,17 @@ static int write_ref_sha1(struct ref_lock *lock,
                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) &&
@@ -3738,19 +3752,23 @@ int ref_transaction_commit(struct ref_transaction *transaction,
                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)
@@ -3760,22 +3778,51 @@ int ref_transaction_commit(struct ref_transaction *transaction,
                                    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 */
                }
        }
 
@@ -3783,7 +3830,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
        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;

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 6805b9e6bb5b7e6a6b6b9546e96023ebeb557bd0..636d3a167c4aa17bbb8f8b58d6a66142d90d1af2 100755 (executable)
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -1065,4 +1065,32 @@ test_expect_success 'stdin -z delete refs works with packed and loose refs' '
        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 --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 35c805a44e817c637c667b1d1d14d97b4ee46803..5446060ffb75302f6c4c876299a0c7cfa6cedaf4 100755 (executable)
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -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