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 cause file descriptor
exhaustion.
But there is no reason to keep the lockfiles open after the first
loop:
* For references being deleted or verified, we don't have to write
anything into the lockfile, so we can close it immediately.
* For references being updated, we can write the new SHA-1 into the
lockfile immediately and close the lockfile without renaming it into
place. In the second loop, the already-written contents can be
renamed into place using commit_ref_update().
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.
This change fixes two tests in t1400.
Signed-off-by: Michael Haggerty <mhagger@xxxxxxxxxxxx>
---
refs.c | 68 +++++++++++++++++++++++++++++++++++++--------------
t/t1400-update-ref.sh | 4 +--
2 files changed, 51 insertions(+), 21 deletions(-)
diff --git a/refs.c b/refs.c
index 782e777..2bdd93c 100644
--- a/refs.c
+++ b/refs.c
@@ -36,25 +36,31 @@ static unsigned char refname_disposition[256] = {
* Flag passed to lock_ref_sha1_basic() telling it to tolerate broken
* refs (i.e., because the reference is about to be deleted anyway).
*/
-#define REF_DELETING 0x02
+#define REF_DELETING 0x02
/*
* Used as a flag in ref_update::flags when a loose ref is being
* pruned.
*/
-#define REF_ISPRUNING 0x04
+#define REF_ISPRUNING 0x04
/*
* Used as a flag in ref_update::flags when the reference should be
* updated to new_sha1.
*/
-#define REF_HAVE_NEW 0x08
+#define REF_HAVE_NEW 0x08
/*
* Used as a flag in ref_update::flags when old_sha1 should be
* checked.
*/
-#define REF_HAVE_OLD 0x10
+#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.
@@ -3749,7 +3755,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];
@@ -3770,13 +3781,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
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_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));
@@ -3786,14 +3791,39 @@ int ref_transaction_commit(struct ref_transaction *transaction,
* The reference already has the desired
* value, so we don't need to write it.
*/
- 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 */
+ update->new_sha1)) {
+ update->lock = NULL; /* freed by the above call */
+ 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)) {
+ update->lock = NULL; /* freed by the above call */
strbuf_addf(err, "Cannot update the ref '%s'.",
update->refname);
ret = TRANSACTION_GENERIC_ERROR;
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 7a69f1a..636d3a1 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -1071,7 +1071,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
@@ -1082,7 +1082,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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html