{"thread":{"id":"39152","subject":"[PATCH 3/5] write_ref_sha1(): inline function at callers","startedAt":"2015-04-24T11:35:44Z","lastAt":"2015-04-28T04:36:53Z","messageCount":19,"participants":["Michael Haggerty","Jeff King","Stefan Beller","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"259942","messageId":"1429875349-29736-1-git-send-email-mhagger@alum.mit.edu","threadId":"39152","inReplyTo":null,"subject":"[PATCH 0/5] Avoid file descriptor exhaustion in ref_transaction_commit()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-04-24T11:35:44Z","receivedAt":"2015-04-24T11:35:44Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"In ref_transaction_commit(), close the reference lockfiles immediately\nto avoid keeping too many file descriptors open at a time. This is\npretty easy, because in the first loop (where we create the locks) we\nalready know what, if anything, has to be written into the lockfile.\nSo write it and close the lockfile immediately. In the second loop,\nrename the lockfiles for reference updates into place, and in the\ncleanup loop, unlock any references that are still locked (i.e., those\nthat were only being verified or deleted).\n\nI think this is a cleaner solution than Stefan's approach [1] of\nclosing and reopening fds based on an estimate of how many fds we can\nafford to waste--we only need a single file descriptor at a time, and\nwe never have to close then reopen a lockfile. But it is a bit more\nintrusive, so it might still be preferable to use Stefan's approach\nfor release 2.4.0, if indeed any fix for this problem is still being\nconsidered for that release.\n\nThis patch series applies on top of Stefan's\n\n    c1f0ca9 refs.c: remove lock_fd from struct ref_lock (2015-04-16)\n\nand it fixes two tests that Stefan introduced earlier in that series.\n\nIt is also available from my GitHub account:\n\n    https://github.com/mhagger/git branch close-ref-locks-promptly\n\nMichael\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/267548\n\nMichael Haggerty (5):\n  write_ref_to_lockfile(): new function, extracted from write_ref_sha1()\n  commit_ref_update(): new function, extracted from write_ref_sha1()\n  write_ref_sha1(): inline function at callers\n  ref_transaction_commit(): remove the local flags variables\n  ref_transaction_commit(): only keep one lockfile open at a time\n\n refs.c                | 107 ++++++++++++++++++++++++++++++++++----------------\n t/t1400-update-ref.sh |   4 +-\n 2 files changed, 75 insertions(+), 36 deletions(-)\n\n-- \n2.1.4\n"},{"id":"259938","messageId":"1429875349-29736-2-git-send-email-mhagger@alum.mit.edu","threadId":"39152","inReplyTo":"1429875349-29736-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 1/5] write_ref_to_lockfile(): new function, extracted from write_ref_sha1()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-04-24T11:35:45Z","receivedAt":"2015-04-24T11:35:45Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is the first step towards separating the checking and writing of\nthe new reference value to committing the change.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c | 20 ++++++++++++++++----\n 1 file changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 4f495bd..72dae21 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3017,11 +3017,10 @@ int is_branch(const char *refname)\n }\n \n /*\n- * Write sha1 into the ref specified by the lock. Make sure that errno\n- * is sane on error.\n+ * Write sha1 into the open lockfile, then close the lockfile. On\n+ * errors, rollback the lockfile and set errno to reflect the problem.\n  */\n-static int write_ref_sha1(struct ref_lock *lock,\n-\tconst unsigned char *sha1, const char *logmsg)\n+static int write_ref_to_lockfile(struct ref_lock *lock, const unsigned char *sha1)\n {\n \tstatic char term = '\\n';\n \tstruct object *o;\n@@ -3050,6 +3049,19 @@ static int write_ref_sha1(struct ref_lock *lock,\n \t\terrno = save_errno;\n \t\treturn -1;\n \t}\n+\treturn 0;\n+}\n+\n+/*\n+ * Write sha1 into the ref specified by the lock. Make sure that errno\n+ * is sane on error.\n+ */\n+static int write_ref_sha1(struct ref_lock *lock,\n+\tconst unsigned char *sha1, const char *logmsg)\n+{\n+\tif (write_ref_to_lockfile(lock, sha1))\n+\t\treturn -1;\n+\n \tclear_loose_ref_cache(&ref_cache);\n \tif (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||\n \t    (strcmp(lock->ref_name, lock->orig_ref_name) &&\n-- \n2.1.4\n"},{"id":"259939","messageId":"1429875349-29736-3-git-send-email-mhagger@alum.mit.edu","threadId":"39152","inReplyTo":"1429875349-29736-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 2/5] commit_ref_update(): new function, extracted from write_ref_sha1()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-04-24T11:35:46Z","receivedAt":"2015-04-24T11:35:46Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c | 26 +++++++++++++++++---------\n 1 file changed, 17 insertions(+), 9 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 72dae21..9b68aec 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3052,16 +3052,9 @@ static int write_ref_to_lockfile(struct ref_lock *lock, const unsigned char *sha\n \treturn 0;\n }\n \n-/*\n- * Write sha1 into the ref specified by the lock. Make sure that errno\n- * is sane on error.\n- */\n-static int write_ref_sha1(struct ref_lock *lock,\n-\tconst unsigned char *sha1, const char *logmsg)\n+static int commit_ref_update(struct ref_lock *lock,\n+\t\t\t     const unsigned char *sha1, const char *logmsg)\n {\n-\tif (write_ref_to_lockfile(lock, sha1))\n-\t\treturn -1;\n-\n \tclear_loose_ref_cache(&ref_cache);\n \tif (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||\n \t    (strcmp(lock->ref_name, lock->orig_ref_name) &&\n@@ -3100,6 +3093,21 @@ static int write_ref_sha1(struct ref_lock *lock,\n \treturn 0;\n }\n \n+/*\n+ * Write sha1 as the new value of the reference specified by the\n+ * (open) lock. On error, roll back the lockfile and set errno\n+ * appropriately.\n+ */\n+static int write_ref_sha1(struct ref_lock *lock,\n+\tconst unsigned char *sha1, const char *logmsg)\n+{\n+\tif (write_ref_to_lockfile(lock, sha1) ||\n+\t    commit_ref_update(lock, sha1, logmsg))\n+\t\treturn -1;\n+\n+\treturn 0;\n+}\n+\n int create_symref(const char *ref_target, const char *refs_heads_master,\n \t\t  const char *logmsg)\n {\n-- \n2.1.4\n"},{"id":"259937","messageId":"1429875349-29736-4-git-send-email-mhagger@alum.mit.edu","threadId":"39152","inReplyTo":"1429875349-29736-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 3/5] write_ref_sha1(): inline function at callers","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-04-24T11:35:47Z","receivedAt":"2015-04-24T11:35:47Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c | 38 +++++++++++++++-----------------------\n 1 file changed, 15 insertions(+), 23 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 9b68aec..a55d541 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2770,8 +2770,10 @@ static int rename_ref_available(const char *oldname, const char *newname)\n \treturn ret;\n }\n \n-static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,\n-\t\t\t  const char *logmsg);\n+static int write_ref_to_lockfile(struct ref_lock *lock,\n+\t\t\t\t const unsigned char *sha1);\n+static int commit_ref_update(struct ref_lock *lock,\n+\t\t\t     const unsigned char *sha1, const char *logmsg);\n \n int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsg)\n {\n@@ -2829,7 +2831,8 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \t\tgoto rollback;\n \t}\n \thashcpy(lock->old_sha1, orig_sha1);\n-\tif (write_ref_sha1(lock, orig_sha1, logmsg)) {\n+\tif (write_ref_to_lockfile(lock, orig_sha1) ||\n+\t    commit_ref_update(lock, orig_sha1, logmsg)) {\n \t\terror(\"unable to write current sha1 into %s\", newrefname);\n \t\tgoto rollback;\n \t}\n@@ -2845,7 +2848,8 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \n \tflag = log_all_ref_updates;\n \tlog_all_ref_updates = 0;\n-\tif (write_ref_sha1(lock, orig_sha1, NULL))\n+\tif (write_ref_to_lockfile(lock, orig_sha1) ||\n+\t    commit_ref_update(lock, orig_sha1, NULL))\n \t\terror(\"unable to write current sha1 into %s\", oldrefname);\n \tlog_all_ref_updates = flag;\n \n@@ -3093,21 +3097,6 @@ static int commit_ref_update(struct ref_lock *lock,\n \treturn 0;\n }\n \n-/*\n- * Write sha1 as the new value of the reference specified by the\n- * (open) lock. On error, roll back the lockfile and set errno\n- * appropriately.\n- */\n-static int write_ref_sha1(struct ref_lock *lock,\n-\tconst unsigned char *sha1, const char *logmsg)\n-{\n-\tif (write_ref_to_lockfile(lock, sha1) ||\n-\t    commit_ref_update(lock, sha1, logmsg))\n-\t\treturn -1;\n-\n-\treturn 0;\n-}\n-\n int create_symref(const char *ref_target, const char *refs_heads_master,\n \t\t  const char *logmsg)\n {\n@@ -3801,15 +3790,18 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t */\n \t\t\t\tunlock_ref(update->lock);\n \t\t\t\tupdate->lock = NULL;\n-\t\t\t} else if (write_ref_sha1(update->lock, update->new_sha1,\n-\t\t\t\t\t\t  update->msg)) {\n-\t\t\t\tupdate->lock = NULL; /* freed by write_ref_sha1 */\n+\t\t\t} else if (write_ref_to_lockfile(update->lock,\n+\t\t\t\t\t\t\t update->new_sha1) ||\n+\t\t\t\t   commit_ref_update(update->lock,\n+\t\t\t\t\t\t     update->new_sha1,\n+\t\t\t\t\t\t     update->msg)) {\n+\t\t\t\tupdate->lock = NULL; /* freed by the above calls */\n \t\t\t\tstrbuf_addf(err, \"Cannot update the ref '%s'.\",\n \t\t\t\t\t    update->refname);\n \t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n \t\t\t\tgoto cleanup;\n \t\t\t} else {\n-\t\t\t\t/* freed by write_ref_sha1(): */\n+\t\t\t\t/* freed by the above calls: */\n \t\t\t\tupdate->lock = NULL;\n \t\t\t}\n \t\t}\n-- \n2.1.4\n"},{"id":"259940","messageId":"1429875349-29736-5-git-send-email-mhagger@alum.mit.edu","threadId":"39152","inReplyTo":"1429875349-29736-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 4/5] ref_transaction_commit(): remove the local flags variables","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-04-24T11:35:48Z","receivedAt":"2015-04-24T11:35:48Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Instead, work directly with update->flags. This has the advantage that\nthe REF_DELETING bit, set in the first loop, can be read in the third\nloop instead of having to compute the same expression again. Plus, it\nwas kindof confusing having both update->flags and flags, which\nsometimes had different values.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c | 15 ++++++---------\n 1 file changed, 6 insertions(+), 9 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex a55d541..782e777 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3752,16 +3752,15 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t/* Acquire all locks while verifying old values */\n \tfor (i = 0; i < n; i++) {\n \t\tstruct ref_update *update = updates[i];\n-\t\tunsigned int flags = update->flags;\n \n-\t\tif ((flags & REF_HAVE_NEW) && is_null_sha1(update->new_sha1))\n-\t\t\tflags |= REF_DELETING;\n+\t\tif ((update->flags & REF_HAVE_NEW) && is_null_sha1(update->new_sha1))\n+\t\t\tupdate->flags |= REF_DELETING;\n \t\tupdate->lock = lock_ref_sha1_basic(\n \t\t\t\tupdate->refname,\n \t\t\t\t((update->flags & REF_HAVE_OLD) ?\n \t\t\t\t update->old_sha1 : NULL),\n \t\t\t\tNULL,\n-\t\t\t\tflags,\n+\t\t\t\tupdate->flags,\n \t\t\t\t&update->type);\n \t\tif (!update->lock) {\n \t\t\tret = (errno == ENOTDIR)\n@@ -3776,9 +3775,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t/* Perform updates first so live commits remain referenced */\n \tfor (i = 0; i < n; i++) {\n \t\tstruct ref_update *update = updates[i];\n-\t\tint flags = update->flags;\n \n-\t\tif ((flags & REF_HAVE_NEW) && !is_null_sha1(update->new_sha1)) {\n+\t\tif ((update->flags & REF_HAVE_NEW) && !is_null_sha1(update->new_sha1)) {\n \t\t\tint overwriting_symref = ((update->type & REF_ISSYMREF) &&\n \t\t\t\t\t\t  (update->flags & REF_NODEREF));\n \n@@ -3810,15 +3808,14 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t/* Perform deletes now that updates are safely completed */\n \tfor (i = 0; i < n; i++) {\n \t\tstruct ref_update *update = updates[i];\n-\t\tint flags = update->flags;\n \n-\t\tif ((flags & REF_HAVE_NEW) && is_null_sha1(update->new_sha1)) {\n+\t\tif (update->flags & REF_DELETING) {\n \t\t\tif (delete_ref_loose(update->lock, update->type, err)) {\n \t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n \t\t\t\tgoto cleanup;\n \t\t\t}\n \n-\t\t\tif (!(flags & REF_ISPRUNING))\n+\t\t\tif (!(update->flags & REF_ISPRUNING))\n \t\t\t\tstring_list_append(&refs_to_delete,\n \t\t\t\t\t\t   update->lock->ref_name);\n \t\t}\n-- \n2.1.4\n"},{"id":"259941","messageId":"1429875349-29736-6-git-send-email-mhagger@alum.mit.edu","threadId":"39152","inReplyTo":"1429875349-29736-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 5/5] ref_transaction_commit(): only keep one lockfile open at a time","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-04-24T11:35:49Z","receivedAt":"2015-04-24T11:35:49Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The old code locked all of the references in the first loop, leaving\nall of the lockfiles open until later loops. Since we might be\nupdating a lot of references, this could cause file descriptor\nexhaustion.\n\nBut there is no reason to keep the lockfiles open after the first\nloop:\n\n* For references being deleted or verified, we don't have to write\n  anything into the lockfile, so we can close it immediately.\n\n* For references being updated, we can write the new SHA-1 into the\n  lockfile immediately and close the lockfile without renaming it into\n  place. In the second loop, the already-written contents can be\n  renamed into place using commit_ref_update().\n\nTo simplify the bookkeeping across loops, add a new REF_NEEDS_COMMIT\nbit to update->flags, which keeps track of whether the corresponding\nlockfile needs to be committed, as opposed to just unlocked.\n\nThis change fixes two tests in t1400.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c                | 68 +++++++++++++++++++++++++++++++++++++--------------\n t/t1400-update-ref.sh |  4 +--\n 2 files changed, 51 insertions(+), 21 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 782e777..2bdd93c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -36,25 +36,31 @@ static unsigned char refname_disposition[256] = {\n  * Flag passed to lock_ref_sha1_basic() telling it to tolerate broken\n  * refs (i.e., because the reference is about to be deleted anyway).\n  */\n-#define REF_DELETING\t0x02\n+#define REF_DELETING 0x02\n \n /*\n  * Used as a flag in ref_update::flags when a loose ref is being\n  * pruned.\n  */\n-#define REF_ISPRUNING\t0x04\n+#define REF_ISPRUNING 0x04\n \n /*\n  * Used as a flag in ref_update::flags when the reference should be\n  * updated to new_sha1.\n  */\n-#define REF_HAVE_NEW\t0x08\n+#define REF_HAVE_NEW 0x08\n \n /*\n  * Used as a flag in ref_update::flags when old_sha1 should be\n  * checked.\n  */\n-#define REF_HAVE_OLD\t0x10\n+#define REF_HAVE_OLD 0x10\n+\n+/*\n+ * Used as a flag in ref_update::flags when the lockfile needs to be\n+ * committed.\n+ */\n+#define REF_NEEDS_COMMIT 0x20\n \n /*\n  * Try to read one refname component from the front of refname.\n@@ -3749,7 +3755,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\tgoto cleanup;\n \t}\n \n-\t/* Acquire all locks while verifying old values */\n+\t/*\n+\t * Acquire all locks, verify old values if provided, check\n+\t * that new values are valid, and write new values to the\n+\t * lockfiles, ready to be activated. Only keep one lockfile\n+\t * open at a time to avoid running out of file descriptors.\n+\t */\n \tfor (i = 0; i < n; i++) {\n \t\tstruct ref_update *update = updates[i];\n \n@@ -3770,13 +3781,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t    update->refname);\n \t\t\tgoto cleanup;\n \t\t}\n-\t}\n-\n-\t/* Perform updates first so live commits remain referenced */\n-\tfor (i = 0; i < n; i++) {\n-\t\tstruct ref_update *update = updates[i];\n-\n-\t\tif ((update->flags & REF_HAVE_NEW) && !is_null_sha1(update->new_sha1)) {\n+\t\tif ((update->flags & REF_HAVE_NEW) && !(update->flags & REF_DELETING)) {\n \t\t\tint overwriting_symref = ((update->type & REF_ISSYMREF) &&\n \t\t\t\t\t\t  (update->flags & REF_NODEREF));\n \n@@ -3786,14 +3791,39 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t * The reference already has the desired\n \t\t\t\t * value, so we don't need to write it.\n \t\t\t\t */\n-\t\t\t\tunlock_ref(update->lock);\n-\t\t\t\tupdate->lock = NULL;\n \t\t\t} else if (write_ref_to_lockfile(update->lock,\n-\t\t\t\t\t\t\t update->new_sha1) ||\n-\t\t\t\t   commit_ref_update(update->lock,\n-\t\t\t\t\t\t     update->new_sha1,\n-\t\t\t\t\t\t     update->msg)) {\n-\t\t\t\tupdate->lock = NULL; /* freed by the above calls */\n+\t\t\t\t\t\t\t update->new_sha1)) {\n+\t\t\t\tupdate->lock = NULL; /* freed by the above call */\n+\t\t\t\tstrbuf_addf(err, \"Cannot update the ref '%s'.\",\n+\t\t\t\t\t    update->refname);\n+\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\t\t\tgoto cleanup;\n+\t\t\t} else {\n+\t\t\t\tupdate->flags |= REF_NEEDS_COMMIT;\n+\t\t\t}\n+\t\t}\n+\t\tif (!(update->flags & REF_NEEDS_COMMIT)) {\n+\t\t\t/*\n+\t\t\t * We didn't have to write anything to the lockfile.\n+\t\t\t * Close it to free up the file descriptor:\n+\t\t\t */\n+\t\t\tif (close_ref(update->lock)) {\n+\t\t\t\tstrbuf_addf(err, \"Couldn't close %s.lock\",\n+\t\t\t\t\t    update->refname);\n+\t\t\t\tgoto cleanup;\n+\t\t\t}\n+\t\t}\n+\t}\n+\n+\t/* Perform updates first so live commits remain referenced */\n+\tfor (i = 0; i < n; i++) {\n+\t\tstruct ref_update *update = updates[i];\n+\n+\t\tif (update->flags & REF_NEEDS_COMMIT) {\n+\t\t\tif (commit_ref_update(update->lock,\n+\t\t\t\t\t\t    update->new_sha1,\n+\t\t\t\t\t\t    update->msg)) {\n+\t\t\t\tupdate->lock = NULL; /* freed by the above call */\n \t\t\t\tstrbuf_addf(err, \"Cannot update the ref '%s'.\",\n \t\t\t\t\t    update->refname);\n \t\t\t\tret = TRANSACTION_GENERIC_ERROR;\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 7a69f1a..636d3a1 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -1071,7 +1071,7 @@ run_with_limited_open_files () {\n \n test_lazy_prereq ULIMIT_FILE_DESCRIPTORS 'run_with_limited_open_files true'\n \n-test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '\n+test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '\n (\n \tfor i in $(test_seq 33)\n \tdo\n@@ -1082,7 +1082,7 @@ test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches\n )\n '\n \n-test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '\n+test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '\n (\n \tfor i in $(test_seq 33)\n \tdo\n-- \n2.1.4\n"},{"id":"259952","messageId":"20150424172648.GB30592@peff.net","threadId":"39152","inReplyTo":"1429875349-29736-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 0/5] Avoid file descriptor exhaustion in ref_transaction_commit()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-24T17:26:48Z","receivedAt":"2015-04-24T17:26:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 24, 2015 at 01:35:44PM +0200, Michael Haggerty wrote:\n\n> In ref_transaction_commit(), close the reference lockfiles immediately\n> to avoid keeping too many file descriptors open at a time. This is\n> pretty easy, because in the first loop (where we create the locks) we\n> already know what, if anything, has to be written into the lockfile.\n> So write it and close the lockfile immediately. In the second loop,\n> rename the lockfiles for reference updates into place, and in the\n> cleanup loop, unlock any references that are still locked (i.e., those\n> that were only being verified or deleted).\n> \n> I think this is a cleaner solution than Stefan's approach [1] of\n> closing and reopening fds based on an estimate of how many fds we can\n> afford to waste--we only need a single file descriptor at a time, and\n> we never have to close then reopen a lockfile. But it is a bit more\n> intrusive, so it might still be preferable to use Stefan's approach\n> for release 2.4.0, if indeed any fix for this problem is still being\n> considered for that release.\n\nI like this approach much better. It seems like the best of all worlds\n(same performance, and we don't have to worry about whether and when to\nclose lockfiles).\n\nStefan's patch is just in pu at this point, right? I do not think there\nis any rushing/release concern. It is too late for either to be in\nv2.4.0, so the only decision is whether to aim for \"master\" or \"maint\".\nTo me, they both seem to be in the same ballpark as far as risking a\nregression.\n\n-Peff\n"},{"id":"259953","messageId":"20150424173005.GC30592@peff.net","threadId":"39152","inReplyTo":"1429875349-29736-5-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 4/5] ref_transaction_commit(): remove the local flags variables","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-24T17:30:05Z","receivedAt":"2015-04-24T17:30:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 24, 2015 at 01:35:48PM +0200, Michael Haggerty wrote:\n\n> Instead, work directly with update->flags. This has the advantage that\n> the REF_DELETING bit, set in the first loop, can be read in the third\n> loop instead of having to compute the same expression again. Plus, it\n> was kindof confusing having both update->flags and flags, which\n> sometimes had different values.\n\nHmm. I think this is losing the distinction of \"flags the caller has\npassed in to us\" versus \"flags we are using locally only during the\ntransaction_commit routine\". If callers look at the flags in the\nREF_TRANSACTION_CLOSED state, do they care about seeing these new flags?\n\nMy guess is probably not in practice, and \"leaking\" these flags is an\nacceptable tradeoff for keeping the transaction_commit function simpler.\nBut I haven't looked that closely.\n\n-Peff\n"},{"id":"259956","messageId":"CAGZ79kYDPgbD0x82ZREcavqaXxs6aaOo3HXAX6HAi2T+shOACA@mail.gmail.com","threadId":"39152","inReplyTo":"20150424172648.GB30592@peff.net","subject":"Re: [PATCH 0/5] Avoid file descriptor exhaustion in ref_transaction_commit()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-04-24T19:13:39Z","receivedAt":"2015-04-24T19:13:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Apr 24, 2015 at 10:26 AM, Jeff King <peff@peff.net> wrote:\n> On Fri, Apr 24, 2015 at 01:35:44PM +0200, Michael Haggerty wrote:\n>\n>> In ref_transaction_commit(), close the reference lockfiles immediately\n>> to avoid keeping too many file descriptors open at a time. This is\n>> pretty easy, because in the first loop (where we create the locks) we\n>> already know what, if anything, has to be written into the lockfile.\n>> So write it and close the lockfile immediately. In the second loop,\n>> rename the lockfiles for reference updates into place, and in the\n>> cleanup loop, unlock any references that are still locked (i.e., those\n>> that were only being verified or deleted).\n>>\n>> I think this is a cleaner solution than Stefan's approach [1] of\n>> closing and reopening fds based on an estimate of how many fds we can\n>> afford to waste--we only need a single file descriptor at a time, and\n>> we never have to close then reopen a lockfile. But it is a bit more\n>> intrusive, so it might still be preferable to use Stefan's approach\n>> for release 2.4.0, if indeed any fix for this problem is still being\n>> considered for that release.\n>\n> I like this approach much better. It seems like the best of all worlds\n> (same performance, and we don't have to worry about whether and when to\n> close lockfiles).\n\nI would have guessed this approach to take more work to do it right.\nThanks Michael for tackling the problem in an elegant way!\n\n>\n> Stefan's patch is just in pu at this point, right? I do not think there\n> is any rushing/release concern.\n\nYeah it's in pu, so it's easy to remove.\n\n> It is too late for either to be in\n> v2.4.0, so the only decision is whether to aim for \"master\" or \"maint\".\n> To me, they both seem to be in the same ballpark as far as risking a\n> regression.\n>\n> -Peff\n\n\nThanks,\nStefan\n"},{"id":"259963","messageId":"553AB25D.9090201@alum.mit.edu","threadId":"39152","inReplyTo":"20150424173005.GC30592@peff.net","subject":"Re: [PATCH 4/5] ref_transaction_commit(): remove the local flags variables","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-04-24T21:15:09Z","receivedAt":"2015-04-24T21:15:09Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/24/2015 07:30 PM, Jeff King wrote:\n> On Fri, Apr 24, 2015 at 01:35:48PM +0200, Michael Haggerty wrote:\n> \n>> Instead, work directly with update->flags. This has the advantage that\n>> the REF_DELETING bit, set in the first loop, can be read in the third\n>> loop instead of having to compute the same expression again. Plus, it\n>> was kindof confusing having both update->flags and flags, which\n>> sometimes had different values.\n> \n> Hmm. I think this is losing the distinction of \"flags the caller has\n> passed in to us\" versus \"flags we are using locally only during the\n> transaction_commit routine\". If callers look at the flags in the\n> REF_TRANSACTION_CLOSED state, do they care about seeing these new flags?\n> \n> My guess is probably not in practice, and \"leaking\" these flags is an\n> acceptable tradeoff for keeping the transaction_commit function simpler.\n> But I haven't looked that closely.\n\n\"struct ref_update\" is opaque to callers outside of the refs module, and\nref_update::flags is not read anywhere outside of\nref_transaction_commit() (and its value is passed to\nlock_ref_sha1_basic()). So I don't think we have to be shy about storing\nour own internal information there.\n\nIn fact, REF_DELETING, REF_ISPRUNING, REF_HAVE_NEW, and REF_HAVE_OLD are\nalso private to the refs module.\n\nI suppose we could mask out all the \"private\" bits in the flags\nparameter passed by the caller, to make sure that the caller hasn't\naccidentally set other bits. I think that would be more defensive than\nour usual practice, but I don't mind doing it if people think it would\nbe prudent.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"259964","messageId":"20150424211932.GA10809@peff.net","threadId":"39152","inReplyTo":"553AB25D.9090201@alum.mit.edu","subject":"Re: [PATCH 4/5] ref_transaction_commit(): remove the local flags variables","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-24T21:19:33Z","receivedAt":"2015-04-24T21:19:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 24, 2015 at 11:15:09PM +0200, Michael Haggerty wrote:\n\n> > Hmm. I think this is losing the distinction of \"flags the caller has\n> > passed in to us\" versus \"flags we are using locally only during the\n> > transaction_commit routine\". If callers look at the flags in the\n> > REF_TRANSACTION_CLOSED state, do they care about seeing these new flags?\n> > \n> > My guess is probably not in practice, and \"leaking\" these flags is an\n> > acceptable tradeoff for keeping the transaction_commit function simpler.\n> > But I haven't looked that closely.\n> \n> \"struct ref_update\" is opaque to callers outside of the refs module, and\n> ref_update::flags is not read anywhere outside of\n> ref_transaction_commit() (and its value is passed to\n> lock_ref_sha1_basic()). So I don't think we have to be shy about storing\n> our own internal information there.\n> \n> In fact, REF_DELETING, REF_ISPRUNING, REF_HAVE_NEW, and REF_HAVE_OLD are\n> also private to the refs module.\n\nThanks for checking. If nobody is affected (and is not likely to be), I\nagree it's not worth worrying about.\n\n> I suppose we could mask out all the \"private\" bits in the flags\n> parameter passed by the caller, to make sure that the caller hasn't\n> accidentally set other bits. I think that would be more defensive than\n> our usual practice, but I don't mind doing it if people think it would\n> be prudent.\n\nI don't think it's necessary.\n\n-Peff\n"},{"id":"259965","messageId":"CAPig+cS7+NW1L5D1gAwz3xp9jv13u4L9i-u1oFDLvRbr-u_X7w@mail.gmail.com","threadId":"39152","inReplyTo":"1429875349-29736-5-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 4/5] ref_transaction_commit(): remove the local flags variables","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-04-24T21:51:41Z","receivedAt":"2015-04-24T21:51:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Apr 24, 2015 at 7:35 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> Instead, work directly with update->flags. This has the advantage that\n> the REF_DELETING bit, set in the first loop, can be read in the third\n> loop instead of having to compute the same expression again. Plus, it\n> was kindof confusing having both update->flags and flags, which\n\ns/kindof/kind of/\n\n> sometimes had different values.\n>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n"},{"id":"259966","messageId":"553AC22C.2030700@alum.mit.edu","threadId":"39152","inReplyTo":"CAPig+cS7+NW1L5D1gAwz3xp9jv13u4L9i-u1oFDLvRbr-u_X7w@mail.gmail.com","subject":"Re: [PATCH 4/5] ref_transaction_commit(): remove the local flags variables","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-04-24T22:22:36Z","receivedAt":"2015-04-24T22:22:36Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/24/2015 11:51 PM, Eric Sunshine wrote:\n> On Fri, Apr 24, 2015 at 7:35 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>> Instead, work directly with update->flags. This has the advantage that\n>> the REF_DELETING bit, set in the first loop, can be read in the third\n>> loop instead of having to compute the same expression again. Plus, it\n>> was kindof confusing having both update->flags and flags, which\n> \n> s/kindof/kind of/\n> \n>> sometimes had different values.\n>>\n>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n\nHehe thanks for looking over my scribblings. In this case, neither\n\"kindof\" nor \"kind of\" is in fact correct English. I used the slang word\n\"kindof\" (sometimes spelled \"kinda\") to mean \"somewhat\", I guess because\n\"somewhat\" sounded too formal for my slapdash opinion.\n\nBut I suppose it's kindof thoughtless to use slang in commit messages\n:-), especially given that they are also meant for people for whom\nEnglish is a second language (let alone sloppy American English).\n\nI suggest s/kindof/potentially/, at least until I have time to submit a\npatch to the English language to make \"kindof\" a reputable word :-)\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"259976","messageId":"xmqqvbgk25mw.fsf@gitster.dls.corp.google.com","threadId":"39152","inReplyTo":"1429875349-29736-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 0/5] Avoid file descriptor exhaustion in ref_transaction_commit()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-25T04:28:39Z","receivedAt":"2015-04-25T04:28:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> In ref_transaction_commit(), close the reference lockfiles immediately\n> to avoid keeping too many file descriptors open at a time. This is\n> pretty easy, because in the first loop (where we create the locks) we\n> already know what, if anything, has to be written into the lockfile.\n\nNicely analysed and beautifully executed.  I like it.\n\n> This patch series applies on top of Stefan's\n>\n>     c1f0ca9 refs.c: remove lock_fd from struct ref_lock (2015-04-16)\n>\n> and it fixes two tests that Stefan introduced earlier in that series.\n\nThanks.\n"},{"id":"259984","messageId":"553B2F5C.3010007@alum.mit.edu","threadId":"39152","inReplyTo":"1429875349-29736-6-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 5/5] ref_transaction_commit(): only keep one lockfile open at a time","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-04-25T06:08:28Z","receivedAt":"2015-04-25T06:08:28Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/24/2015 01:35 PM, Michael Haggerty wrote:\n> The old code locked all of the references in the first loop, leaving\n> all of the lockfiles open until later loops. Since we might be\n> updating a lot of references, this could cause file descriptor\n> exhaustion.\n> \n> But there is no reason to keep the lockfiles open after the first\n> loop:\n> \n> * For references being deleted or verified, we don't have to write\n>   anything into the lockfile, so we can close it immediately.\n> \n> * For references being updated, we can write the new SHA-1 into the\n>   lockfile immediately and close the lockfile without renaming it into\n>   place. In the second loop, the already-written contents can be\n>   renamed into place using commit_ref_update().\n> \n> To simplify the bookkeeping across loops, add a new REF_NEEDS_COMMIT\n> bit to update->flags, which keeps track of whether the corresponding\n> lockfile needs to be committed, as opposed to just unlocked.\n> \n> This change fixes two tests in t1400.\n\nI realize that I should have highlighted another, perhaps more serious\nbug that is fixed by this change. The old code was roughly\n\n    for update in updates:\n        acquire locks and check old_sha\n    for update in updates:\n        if changing value:\n            write_ref_to_lockfile()\n            commit_ref_update()\n    for update in updates:\n        if deleting value:\n            unlink()\n    for update in updates:\n        if reference still locked:\n            unlock_ref()\n\nThe atomicity of the update depends on all preconditions being checked\nin the first loop, before any changes have start being committed. The\nproblem is that write_ref_to_lockfile() (which used to be part of\nwrite_ref_sha1()), which happens in the second loop, checks some more\npreconditions:\n\n* It verifies that new_sha1 is a valid object\n* If the reference being updated is a branch, it verifies that new_sha1\n  points at a commit object (as opposed to a tag, tree, or blob).\n\nIf either of these preconditions fails, the \"transaction\" might be\naborted after some reference updates have already been permanently\ncommitted. In other words, the all-or-nothing promise of \"git update-ref\n--stdin\" and \"git push --atomic\" would be violated.\n\nI'm sorry that I didn't fully realize this problem earlier.\n\nAfter this patch, the code looks like\n\n    for update in updates:\n        acquire locks and check old_sha\n        if changing value:\n            write_ref_to_lockfile()\n    for update in updates:\n        if changing value:\n            commit_ref_update()\n    for update in updates:\n        if deleting value:\n            unlink()\n    for update in updates:\n        if reference still locked:\n            unlock_ref()\n\nThis has the important effect that the pre-checks in\nwrite_ref_to_lockfile() are carried out before any changes have been\ncommitted, so if any of those checks fail, the transaction can be rolled\nback correctly.\n\nGiven that \"git push --atomic\" is one of the main new features of 2.4.0,\nit would be unfortunate for the release to contain this bug, plus the\nbug that atomic pushes can fail due to file descriptor exhaustion.\n\nIs this problem important enough to justify delaying the release to get\nthe fix in?\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"259986","messageId":"xmqqiock1yq3.fsf@gitster.dls.corp.google.com","threadId":"39152","inReplyTo":"553B2F5C.3010007@alum.mit.edu","subject":"Re: [PATCH 5/5] ref_transaction_commit(): only keep one lockfile open at a time","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-25T06:57:56Z","receivedAt":"2015-04-25T06:57:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> Given that \"git push --atomic\" is one of the main new features of 2.4.0,\n> it would be unfortunate for the release to contain this bug, plus the\n> bug that atomic pushes can fail due to file descriptor exhaustion.\n>\n> Is this problem important enough to justify delaying the release to get\n> the fix in?\n\nI am not too worried about \"push --atomic\", as we can just add a few\nwords to Release Notes and documentation saying \"this is still an\nexperimental broken code that is unusable; don't use the feature in\nproduction\".\n\nI however am more worried about the other one \"update-ref --stdin\";\nthe change will be pure regression for those who want to do many\nupdates and do not care if the update is atomic, no?\n\nUnfortuntely it is pretty late in the game, and even though in\nprinciple I know that the only sensible way forward is to revert the\noriginal breakage, I find it very tempting to patch it up with your\nseries.\n"},{"id":"260003","messageId":"xmqqoamcypy4.fsf@gitster.dls.corp.google.com","threadId":"39152","inReplyTo":"xmqqiock1yq3.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 5/5] ref_transaction_commit(): only keep one lockfile open at a time","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-25T19:21:07Z","receivedAt":"2015-04-25T19:21:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I am not too worried about \"push --atomic\", as we can just add a few\n> words to Release Notes and documentation saying \"this is still an\n> experimental broken code that is unusable; don't use the feature in\n> production\".\n>\n> I however am more worried about the other one \"update-ref --stdin\";\n> the change will be pure regression for those who want to do many\n> updates and do not care if the update is atomic, no?\n\nI should have refrained from touching the keyboard so late at night\nX-<.  This regression was done long time ago (even in v2.1.0 I see\nthat ref_transaction_commit() tries to grab all locks at once).\n\nSo it is only \"push --atomic\".\n\nThe choice is between (1) shipping \"push --atomic\" that is known to\nbe broken, (2) applying your five-patch series which may (a) fix\nboth \"push --atomic\" and \"update-ref --stdin\", or (b) break other\ntransaction users including \"update-ref -stdin\" in unexpected ways.\n\nI dunno.  I am still tempted to go route (2) hoping that it would\nresult in (2-a) not (2-b).\n"},{"id":"260004","messageId":"xmqqk2x0ypmo.fsf@gitster.dls.corp.google.com","threadId":"39152","inReplyTo":"20150424172648.GB30592@peff.net","subject":"Re: [PATCH 0/5] Avoid file descriptor exhaustion in ref_transaction_commit()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-25T19:27:59Z","receivedAt":"2015-04-25T19:27:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Stefan's patch is just in pu at this point, right? I do not think there\n> is any rushing/release concern. It is too late for either to be in\n> v2.4.0, so the only decision is whether to aim for \"master\" or \"maint\".\n> To me, they both seem to be in the same ballpark as far as risking a\n> regression.\n\nThe series builds on c1f0ca9 that is on 2.4.0-rc2, so we would need\nto wiggle things around to apply to older codebase if we want to fix\nthe \"too many open file descriptor\" issue on older maintenance\nreleases for \"update-ref --stdin\".\n\nI personally feel that it is OK to ship v2.4.0 without the fix,\nleaving \"push --atomic\" broken, and fix it in v2.4.1, but I kinda\nprefer that the final fix to be applicable for older maintenance\nreleases, at least to 2.3.x track, if not 2.2.x track.\n"},{"id":"260094","messageId":"20150428043653.GC24580@peff.net","threadId":"39152","inReplyTo":"xmqqoamcypy4.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 5/5] ref_transaction_commit(): only keep one lockfile open at a time","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-28T04:36:53Z","receivedAt":"2015-04-28T04:36:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 25, 2015 at 12:21:07PM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > I am not too worried about \"push --atomic\", as we can just add a few\n> > words to Release Notes and documentation saying \"this is still an\n> > experimental broken code that is unusable; don't use the feature in\n> > production\".\n> >\n> > I however am more worried about the other one \"update-ref --stdin\";\n> > the change will be pure regression for those who want to do many\n> > updates and do not care if the update is atomic, no?\n> \n> I should have refrained from touching the keyboard so late at night\n> X-<.  This regression was done long time ago (even in v2.1.0 I see\n> that ref_transaction_commit() tries to grab all locks at once).\n> \n> So it is only \"push --atomic\".\n> \n> The choice is between (1) shipping \"push --atomic\" that is known to\n> be broken, (2) applying your five-patch series which may (a) fix\n> both \"push --atomic\" and \"update-ref --stdin\", or (b) break other\n> transaction users including \"update-ref -stdin\" in unexpected ways.\n\nI'm not sure \"--atomic\" is foolproof even with these patches.\n\nCertainly there is the obvious problem that the filesystem is not atomic\n(so a power loss halfway through committing the locks will result in a\nnon-atomic push). But there are also subtle locking issues related to\nD/F conflicts.\n\nFor instance, if we do (and I take no credit for this discovery; Michael\nshowed it to me last week):\n\n  {\n    echo \"create refs/heads/branch $sha1\"\n    echo \"create refs/heads/foo/bar $sha1\" &&\n    echo \"create refs/heads/foo $sha1\"\n  } | git update-ref --stdin\n\nwe do not notice the conflict between \"foo\" and \"foo/bar\" until the\ncommit stage. Fortunately our ref-ordering rules mean we will always try\nto write \"foo\" before \"foo/bar\", which will fail because \"foo\" must be a\ndirectory (because we have \"foo/bar.lock\"). But because \"branch\" comes\nalphabetically first, we will have written that ref already, committing\npart of the request.\n\nYou can work around this with code to check for D/F conflicts in a\nsingle transaction, but then you are subject to races with other\nprocesses. E.g., one process writes \"branch\" and \"foo\", the other is\nsimultaneously writing \"foo/bar\". There is no lock contention, but the\nwrite of \"foo\" may fail at the commit stage.\n\nSo I think it is OK to restrict the definition of \"atomic\" here to the\ncommon case of making sure that we can lock and check the $old_sha1 for\neach of the refs. Under that definition, \"push --atomic\" is not broken\nas-is, but there is room for improving its robustness.  And I have no\nproblem with trying to make our storage format as robust as possible in\nthese cases, but I think we need to admit that the filesystem ref\nstorage is never going to be 100% transactional.\n\nI know that sounds a bit like moving the goalposts to say \"eh, this\nfeature is not broken, your definition of broken is just wrong\". But my\npoint is that shipping \"push --atomic\" as-is is still a useful step\nforward, and we can continue to iterate and improve on the concept in\nfuture releases.\n\n-Peff\n"}]}