[PATCH v2 0/8] Fix atomicity and avoid fd exhaustion in ref transactions

[PATCH v2 0/8] Fix atomicity and avoid fd exhaustion in ref transactions

[Michael Haggerty]

This is v2 of [1], formerly called

    Avoid file descriptor exhaustion in ref_transaction_commit()

As a reminder, this patch series fixes some cases where `git
update-ref --stdin` (and, on 2.4+, `git push --atomic`) could fail to
be atomic, and avoids file descriptor exhaustion while carrying out
ref transactions by closing lockfiles promptly.

Thanks to Peff, Stefan, Junio, and Eric for their comments on v1.

The main change is that this version is rebased onto maint-2.2 as per
Junio's wish [2]. This was not trivial because of the changes in the
area between 2.3 and 2.4, especially 'mh/expire-updateref-fixes',
'mh/refs-have-new', and 'mh/reflog-expire'.

Other changes:

* Remove Stefan's

      bc31f46 refs.c: remove lock_fd from struct ref_lock (2015-04-16)

  from the series, as it is not needed for this fix and was causing
  extra conflicts when backporting and merging forward. This patch is
  still a good idea, but there is no need to backport it.

* Change log message of the last patch to describe both of the bugs
  that it fixes.

* Many other small tweaks.

This patch series is available from my GitHub account [3] as branch
'write-refs-sooner'. Please note that the branch there is applied on
top of a cherry-pick of

      f6786c8 http-push: trim trailing newline from remote symref (2015-01-12)

, which was needed to get test t5540 to pass.

The following other branches, also from my GitHub repo, might be
useful:

* 'write-refs-sooner-2.3' -- suggested merge of the change to 'maint'.

* 'write-refs-sooner-master' -- suggested merge of the change to
  'master'.

* 'write-refs-sooner-rebased-2.3' and
  'write-refs-sooner-rebased-master' -- rebases of 'write-refs-sooner'
  onto 'maint' and 'master' respectively, in case anybody is
  interested to see how the individual patches would look if
  implemented natively on these branches.

[1] http://thread.gmane.org/gmane.comp.version-control.git/267735
[2] http://article.gmane.org/gmane.comp.version-control.git/267799
[3] https://github.com/mhagger/git

Michael Haggerty (6):
  write_ref_to_lockfile(): new function, extracted from write_ref_sha1()
  commit_ref_update(): new function, extracted from write_ref_sha1()
  rename_ref(): inline calls to write_ref_sha1() from this function
  ref_transaction_commit(): inline calls to write_ref_sha1()
  ref_transaction_commit(): remove the local flags variable
  ref_transaction_commit(): fix atomicity and avoid fd exhaustion

Stefan Beller (2):
  update-ref: test handling large transactions properly
  t7004: rename ULIMIT test prerequisite to ULIMIT_STACK_SIZE

 refs.c                | 99 +++++++++++++++++++++++++++++++++++++--------------
 t/t1400-update-ref.sh | 28 +++++++++++++++
 t/t7004-tag.sh        |  4 +--
 3 files changed, 103 insertions(+), 28 deletions(-)

-- 
2.1.4

[PATCH v2 1/8] update-ref: test handling large transactions properly

[Michael Haggerty]

From: Stefan Beller <sbeller@google.com>

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t1400-update-ref.sh | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 7b4707b..47d2fe9 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -973,4 +973,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_failure 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_failure 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
-- 
2.1.4

[PATCH v2 2/8] t7004: rename ULIMIT test prerequisite to ULIMIT_STACK_SIZE

[Michael Haggerty]

From: Stefan Beller <sbeller@google.com>

During creation of the patch series our discussion we could have a
more descriptive name for the prerequisite for the test so it stays
unique when other limits of ulimit are introduced.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t7004-tag.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 796e9f7..06b8e0d 100755
--- 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
-- 
2.1.4

Re: [PATCH v2 2/8] t7004: rename ULIMIT test prerequisite to ULIMIT_STACK_SIZE

[Stefan Beller]

On Sat, May 9, 2015 at 7:45 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> From: Stefan Beller <sbeller@google.com>
>
> During creation of the patch series our discussion we could have a
> more descriptive name for the prerequisite for the test so it stays
> unique when other limits of ulimit are introduced.

I must have had a bad day when trying to write this. Either words are missing
(just insert a "revealed" after discussion on the first line) or it
doesn't make any
sense. How about:

In this patch series we introduce another test depending on a different ulimit
so we need to have a more descriptive name for this prerequisite such that
it stays unique when other limits of ulimit are introduced.

>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  t/t7004-tag.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index 796e9f7..06b8e0d 100755
> --- 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
> --
> 2.1.4
>

Re: [PATCH v2 2/8] t7004: rename ULIMIT test prerequisite to ULIMIT_STACK_SIZE

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Sat, May 9, 2015 at 7:45 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> From: Stefan Beller <sbeller@google.com>
>>
>> During creation of the patch series our discussion we could have a
>> more descriptive name for the prerequisite for the test so it stays
>> unique when other limits of ulimit are introduced.
>
> I must have had a bad day when trying to write this. Either words are missing
> (just insert a "revealed" after discussion on the first line) or it

I just did this s//revealed/ while re-reading the v2 series.

Thanks.

[PATCH v2 3/8] write_ref_to_lockfile(): new function, extracted from write_ref_sha1()

[Michael Haggerty]

This is the first step towards separating the checking and writing of
the new reference value to committing the change.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 38 ++++++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 12 deletions(-)

diff --git a/refs.c b/refs.c
index 6664423..9e40c35 100644
--- a/refs.c
+++ b/refs.c
@@ -3048,23 +3048,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",
@@ -3089,6 +3081,28 @@ static int write_ref_sha1(struct ref_lock *lock,
 		errno = save_errno;
 		return -1;
 	}
+	return 0;
+}
+
+/*
+ * Write sha1 into the ref specified by the lock. Make sure that errno
+ * is sane on error.
+ */
+static int write_ref_sha1(struct ref_lock *lock,
+	const unsigned char *sha1, const char *logmsg)
+{
+	if (!lock) {
+		errno = EINVAL;
+		return -1;
+	}
+	if (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {
+		unlock_ref(lock);
+		return 0;
+	}
+
+	if (write_ref_to_lockfile(lock, sha1))
+		return -1;
+
 	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) &&
-- 
2.1.4

[PATCH v2 4/8] commit_ref_update(): new function, extracted from write_ref_sha1()

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 45 +++++++++++++++++++++++++++++----------------
 1 file changed, 29 insertions(+), 16 deletions(-)

diff --git a/refs.c b/refs.c
index 9e40c35..7661db9 100644
--- a/refs.c
+++ b/refs.c
@@ -3085,24 +3085,13 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
 }
 
 /*
- * Write sha1 into the ref specified by the lock. Make sure that errno
- * is sane on error.
+ * 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 write_ref_sha1(struct ref_lock *lock,
-	const unsigned char *sha1, const char *logmsg)
+static int commit_ref_update(struct ref_lock *lock,
+			     const unsigned char *sha1, const char *logmsg)
 {
-	if (!lock) {
-		errno = EINVAL;
-		return -1;
-	}
-	if (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {
-		unlock_ref(lock);
-		return 0;
-	}
-
-	if (write_ref_to_lockfile(lock, sha1))
-		return -1;
-
 	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) &&
@@ -3141,6 +3130,30 @@ static int write_ref_sha1(struct ref_lock *lock,
 	return 0;
 }
 
+/*
+ * Write sha1 as the new value of the reference specified by the
+ * (open) lock. On error, roll back the lockfile and set errno
+ * appropriately.
+ */
+static int write_ref_sha1(struct ref_lock *lock,
+			  const unsigned char *sha1, const char *logmsg)
+{
+	if (!lock) {
+		errno = EINVAL;
+		return -1;
+	}
+	if (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {
+		unlock_ref(lock);
+		return 0;
+	}
+
+	if (write_ref_to_lockfile(lock, sha1) ||
+	    commit_ref_update(lock, sha1, logmsg))
+		return -1;
+
+	return 0;
+}
+
 int create_symref(const char *ref_target, const char *refs_heads_master,
 		  const char *logmsg)
 {
-- 
2.1.4

[PATCH v2 5/8] rename_ref(): inline calls to write_ref_sha1() from this function

[Michael Haggerty]

Most of what it does is unneeded from these call sites.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index 7661db9..18ce8e6 100644
--- a/refs.c
+++ b/refs.c
@@ -2799,8 +2799,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)
 {
@@ -2859,7 +2860,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;
 	}
@@ -2876,7 +2879,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;
 
-- 
2.1.4

[PATCH v2 6/8] ref_transaction_commit(): inline call to write_ref_sha1()

[Michael Haggerty]

And remove the function write_ref_sha1(), as it is no longer used.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 38 ++++++++++----------------------------
 1 file changed, 10 insertions(+), 28 deletions(-)

diff --git a/refs.c b/refs.c
index 18ce8e6..76609bf 100644
--- a/refs.c
+++ b/refs.c
@@ -3134,30 +3134,6 @@ static int commit_ref_update(struct ref_lock *lock,
 	return 0;
 }
 
-/*
- * Write sha1 as the new value of the reference specified by the
- * (open) lock. On error, roll back the lockfile and set errno
- * appropriately.
- */
-static int write_ref_sha1(struct ref_lock *lock,
-			  const unsigned char *sha1, const char *logmsg)
-{
-	if (!lock) {
-		errno = EINVAL;
-		return -1;
-	}
-	if (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {
-		unlock_ref(lock);
-		return 0;
-	}
-
-	if (write_ref_to_lockfile(lock, sha1) ||
-	    commit_ref_update(lock, sha1, logmsg))
-		return -1;
-
-	return 0;
-}
-
 int create_symref(const char *ref_target, const char *refs_heads_master,
 		  const char *logmsg)
 {
@@ -3852,15 +3828,21 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		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->lock->force_write &&
+			    !hashcmp(update->lock->old_sha1, update->new_sha1)) {
+				unlock_ref(update->lock);
+				update->lock = NULL;
+			} else if (write_ref_to_lockfile(update->lock, update->new_sha1) ||
+				   commit_ref_update(update->lock, update->new_sha1, update->msg)) {
+				update->lock = NULL; /* freed by the above calls */
 				strbuf_addf(err, "Cannot update the ref '%s'.",
 					    update->refname);
 				ret = TRANSACTION_GENERIC_ERROR;
 				goto cleanup;
+			} else {
+				/* freed by the above calls: */
+				update->lock = NULL;
 			}
-			update->lock = NULL; /* freed by write_ref_sha1 */
 		}
 	}
 
-- 
2.1.4

[PATCH v2 7/8] ref_transaction_commit(): remove the local flags variable

[Michael Haggerty]

Instead, work directly with update->flags. This has the advantage that
the REF_DELETING bit, set in the first loop, can be read in the second
loop instead of having to be recomputed. Plus, it was potentially
confusing having both update->flags and flags, which sometimes had
different values.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index 76609bf..58b1182 100644
--- a/refs.c
+++ b/refs.c
@@ -3802,16 +3802,15 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	/* Acquire all locks while verifying old values */
 	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)
@@ -3827,7 +3826,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
 
-		if (!is_null_sha1(update->new_sha1)) {
+		if (!(update->flags & REF_DELETING)) {
 			if (!update->lock->force_write &&
 			    !hashcmp(update->lock->old_sha1, update->new_sha1)) {
 				unlock_ref(update->lock);
-- 
2.1.4

[PATCH v2 8/8] ref_transaction_commit(): fix atomicity and avoid fd exhaustion

[Michael Haggerty]

The old code was roughly

    for update in updates:
        acquire locks and check old_sha
    for update in updates:
        if changing value:
            write_ref_to_lockfile()
            commit_ref_update()
    for update in updates:
        if deleting value:
            unlink()
    rewrite packed-refs file
    for update in updates:
        if reference still locked:
            unlock_ref()

This has two problems.

Non-atomic updates
==================

The atomicity of the reference transaction depends on all pre-checks
being done in the first loop, before any changes have started being
committed in the second loop. The problem is that
write_ref_to_lockfile() (previously part of write_ref_sha1()), which
is called from the second loop, contains two more checks:

* It verifies that new_sha1 is a valid object

* If the reference being updated is a branch, it verifies that
  new_sha1 points at a commit object (as opposed to a tag, tree, or
  blob).

If either of these checks fails, the "transaction" is aborted during
the second loop. But this might happen after some reference updates
have already been permanently committed. In other words, the
all-or-nothing promise of "git update-ref --stdin" could be violated.

So these checks have to be moved to the first loop.

File descriptor exhaustion
==========================

The old code locked all of the references in the first loop, leaving
all of the lockfiles open until later loops. Since we might be
updating a lot of references, this could result in file descriptor
exhaustion.

The solution
============

After this patch, the code looks like

    for update in updates:
        acquire locks and check old_sha
        if changing value:
            write_ref_to_lockfile()
        else:
            close_ref()
    for update in updates:
        if changing value:
            commit_ref_update()
    for update in updates:
        if deleting value:
            unlink()
    rewrite packed-refs file
    for update in updates:
        if reference still locked:
            unlock_ref()

This fixes both problems:

1. The pre-checks in write_ref_to_lockfile() are now done in the first
   loop, before any changes have been committed. If any of the checks
   fails, the whole transaction can now be rolled back correctly.

2. All lockfiles are closed in the first loop immediately after they
   are created (either by write_ref_to_lockfile() or by close_ref()).
   This means that there is never more than one open lockfile at a
   time, preventing file descriptor exhaustion.

To simplify the bookkeeping across loops, add a new REF_NEEDS_COMMIT
bit to update->flags, which keeps track of whether the corresponding
lockfile needs to be committed, as opposed to just unlocked. (Since
"struct ref_update" is internal to the refs module, this change is not
visible to external callers.)

This change fixes two tests in t1400.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c                | 55 +++++++++++++++++++++++++++++++++++++++++----------
 t/t1400-update-ref.sh |  4 ++--
 2 files changed, 47 insertions(+), 12 deletions(-)

diff --git a/refs.c b/refs.c
index 58b1182..85c1dcb 100644
--- 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
@@ -3799,7 +3806,12 @@ 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];
 
@@ -3820,26 +3832,49 @@ 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 (!(update->flags & REF_DELETING)) {
-			if (!update->lock->force_write &&
-			    !hashcmp(update->lock->old_sha1, update->new_sha1)) {
-				unlock_ref(update->lock);
+		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;
-			} else if (write_ref_to_lockfile(update->lock, update->new_sha1) ||
-				   commit_ref_update(update->lock, update->new_sha1, update->msg)) {
-				update->lock = NULL; /* freed by the above calls */
 				strbuf_addf(err, "Cannot update the ref '%s'.",
 					    update->refname);
 				ret = TRANSACTION_GENERIC_ERROR;
 				goto cleanup;
 			} else {
-				/* freed by the above calls: */
+				/* freed by the above call: */
 				update->lock = NULL;
 			}
 		}
@@ -3849,7 +3884,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 47d2fe9..c593a1d 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -979,7 +979,7 @@ run_with_limited_open_files () {
 
 test_lazy_prereq ULIMIT_FILE_DESCRIPTORS 'run_with_limited_open_files true'
 
-test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '
+test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '
 (
 	for i in $(test_seq 33)
 	do
@@ -990,7 +990,7 @@ test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches
 )
 '
 
-test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '
+test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '
 (
 	for i in $(test_seq 33)
 	do
-- 
2.1.4

Re: [PATCH v2 0/8] Fix atomicity and avoid fd exhaustion in ref transactions

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> The following other branches, also from my GitHub repo, might be
> useful:
>
> * 'write-refs-sooner-2.3' -- suggested merge of the change to 'maint'.
>
> * 'write-refs-sooner-master' -- suggested merge of the change to
>   'master'.
>
> * 'write-refs-sooner-rebased-2.3' and
>   'write-refs-sooner-rebased-master' -- rebases of 'write-refs-sooner'
>   onto 'maint' and 'master' respectively, in case anybody is
>   interested to see how the individual patches would look if
>   implemented natively on these branches.

Thanks, that indeed is very helpful and instructive.

A mechanical merge of sooner-2.2 to maint trivially gave sooner-2.3,
so I am happy with that one.

Even though I manually resolved it and the resulting tree pretty
much matched with your suggested merge, I am hesitant to record the
change of sooner-2.3 as a single large merge to master.  I am
tempted to record this as somewhat a wicked merge, e.g.

 - apply posted patches on maint-2.2, which is your sooner-2.2;

 - branch sooner-2.3 from maint, merge sooner-2.2;

 - branch sooner-master from v2.4.0, apply the patches in your
   sooner-rebased-master on top, and then merge sooner-2.3, possibly
   with "-s ours"

And then sooner-master would record both "if built naturally on 2.4"
progression, which would explain what was done much better than a
huge merge of sooner-2.3 into 'master', and "what is to be done on
older codebase".

I dunno.

Anyway, these patches looked good both on 2.2 and on 2.4.  Thanks.

Re: [PATCH v2 0/8] Fix atomicity and avoid fd exhaustion in ref transactions

[Michael Haggerty]

On 05/11/2015 06:30 AM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> The following other branches, also from my GitHub repo, might be
>> useful:
>>
>> * 'write-refs-sooner-2.3' -- suggested merge of the change to 'maint'.
>>
>> * 'write-refs-sooner-master' -- suggested merge of the change to
>>   'master'.
>>
>> * 'write-refs-sooner-rebased-2.3' and
>>   'write-refs-sooner-rebased-master' -- rebases of 'write-refs-sooner'
>>   onto 'maint' and 'master' respectively, in case anybody is
>>   interested to see how the individual patches would look if
>>   implemented natively on these branches.
> 
> Thanks, that indeed is very helpful and instructive.
> 
> A mechanical merge of sooner-2.2 to maint trivially gave sooner-2.3,
> so I am happy with that one.
> 
> Even though I manually resolved it and the resulting tree pretty
> much matched with your suggested merge, I am hesitant to record the
> change of sooner-2.3 as a single large merge to master.  I am
> tempted to record this as somewhat a wicked merge, e.g.
> 
>  - apply posted patches on maint-2.2, which is your sooner-2.2;
> 
>  - branch sooner-2.3 from maint, merge sooner-2.2;
> 
>  - branch sooner-master from v2.4.0, apply the patches in your
>    sooner-rebased-master on top, and then merge sooner-2.3, possibly
>    with "-s ours"
> 
> And then sooner-master would record both "if built naturally on 2.4"
> progression, which would explain what was done much better than a
> huge merge of sooner-2.3 into 'master', and "what is to be done on
> older codebase".

This is exactly the kind of case that "rebase with history" [1] was
meant to address. But given that our tooling doesn't support such
complicated histories very well, your plan sounds reasonable.

Michael

[1]
http://softwareswirl.blogspot.de/2009/04/truce-in-merge-vs-rebase-war.html

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v2 0/8] Fix atomicity and avoid fd exhaustion in ref transactions

[Stefan Beller]

On Sun, May 10, 2015 at 11:52 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> This is exactly the kind of case that "rebase with history" [1] was
> meant to address. But given that our tooling doesn't support such
> complicated histories very well, your plan sounds reasonable.

As a side note to your blog post unrelated to the current series:

I think the new proposed history for "rebase-by-merging-a-patch-at-a-time" also
improves bisectability because you have less long running side branches
(as compared to both in traditional rebase and traditional merge), but a finer
meshed DAG where it is easier to split the commit range into half its size.
When going back one step in history you have more merge nodes where
bisect can decide how many commits to chop off of the new range.

>
> Michael
>
> [1]
> http://softwareswirl.blogspot.de/2009/04/truce-in-merge-vs-rebase-war.html
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
>

Re: [PATCH v2 0/8] Fix atomicity and avoid fd exhaustion in ref transactions

[Christian Couder]

On Mon, May 11, 2015 at 7:10 PM, Stefan Beller <sbeller@google.com> wrote:
> On Sun, May 10, 2015 at 11:52 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> This is exactly the kind of case that "rebase with history" [1] was
>> meant to address. But given that our tooling doesn't support such
>> complicated histories very well, your plan sounds reasonable.
>
> As a side note to your blog post unrelated to the current series:
>
> I think the new proposed history for "rebase-by-merging-a-patch-at-a-time" also
> improves bisectability because you have less long running side branches
> (as compared to both in traditional rebase and traditional merge), but a finer
> meshed DAG where it is easier to split the commit range into half its size.
> When going back one step in history you have more merge nodes where
> bisect can decide how many commits to chop off of the new range.

Yeah, this was discussed at the Git Merge 2013 in Berlin, where
Michael gave two presentations (one on the developer day and one on
the user day) about git-imerge. We also discussed the idea of using a
replace ref to be able to switch between different "views" of the
merge, for example one view where you see only one commit for the
whole merge/rebase with history, and one view where you see all the
micro merge commits.

Best,
Christian.

Re: [PATCH v2 0/8] Fix atomicity and avoid fd exhaustion in ref transactions

[Stefan Beller]

On Sun, May 10, 2015 at 9:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> The following other branches, also from my GitHub repo, might be
>> useful:
>>
>> * 'write-refs-sooner-2.3' -- suggested merge of the change to 'maint'.
>>
>> * 'write-refs-sooner-master' -- suggested merge of the change to
>>   'master'.
>>
>> * 'write-refs-sooner-rebased-2.3' and
>>   'write-refs-sooner-rebased-master' -- rebases of 'write-refs-sooner'
>>   onto 'maint' and 'master' respectively, in case anybody is
>>   interested to see how the individual patches would look if
>>   implemented natively on these branches.
>
> Thanks, that indeed is very helpful and instructive.
>
> A mechanical merge of sooner-2.2 to maint trivially gave sooner-2.3,
> so I am happy with that one.
>
> Even though I manually resolved it and the resulting tree pretty
> much matched with your suggested merge, I am hesitant to record the
> change of sooner-2.3 as a single large merge to master.  I am
> tempted to record this as somewhat a wicked merge, e.g.
>
>  - apply posted patches on maint-2.2, which is your sooner-2.2;
>
>  - branch sooner-2.3 from maint, merge sooner-2.2;
>
>  - branch sooner-master from v2.4.0, apply the patches in your
>    sooner-rebased-master on top, and then merge sooner-2.3, possibly
>    with "-s ours"
>
> And then sooner-master would record both "if built naturally on 2.4"
> progression, which would explain what was done much better than a
> huge merge of sooner-2.3 into 'master', and "what is to be done on
> older codebase".
>
> I dunno.
>
> Anyway, these patches looked good both on 2.2 and on 2.4.  Thanks.

The patches from Michael all look good. The one he picked up from me has a
weird commit message, though.

Thanks,
Stefan