{"thread":{"id":"36386","subject":"[PATCH 2/4] refs.c: split delete_ref_loose() into a separate flag-for-deletion and commit phase","startedAt":"2014-04-10T18:30:21Z","lastAt":"2014-04-10T20:08:37Z","messageCount":9,"participants":["Ronnie Sahlberg","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"238633","messageId":"1397154625-11884-1-git-send-email-sahlberg@google.com","threadId":"36386","inReplyTo":null,"subject":"[PATCH 0/4] Make update_refs more atomic V2","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-10T18:30:21Z","receivedAt":"2014-04-10T18:30:21Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"refs.c:update_refs() intermingles doing updates and checks with actually\napplying changes to the refs in loops that abort on error.\nThis is done one ref at a time and means that if an error is detected that\nwill fail the operation after only some of the ref operations have been\nbeen updated on the disk.\n\nThese patches change the update and delete functions to use a three\ncall pattern of\n\n1, lock\n2, update, or flag for deletion\n3, apply on disk\n\nIn the final patch I change update_refs to perform these actions in three\nseparate loops where the final loop to 'apply on disk' all the changes will\nonly be performed if there were no error conditions detected during any of\nprevious loops.\n\nThis should make the changes of refs in update_refs slightly more atomic.\n\n\nThis may overlap with other current patch series for making refs updates\nmore atomic which may mean these patches become obsolete, but I would still\nlike some review and feedback on these changes.\n\nVersion 2:\nUpdates and fixes based on Junio's feedback.\n* Fix the subject line for patches so they comply with the project standard.\n* Redo the update/delete loops so that we maintain the correct order of\n  operations. Perform all updates first, then perform the deletes.\n* Add an additional patch that allows us to do the update/delete in the correct\n  order from within a single loop by first sorting the refs so that deletes\n  are after all non-deletes.\n\n\nRonnie Sahlberg (4):\n  refs.c: split writing and commiting a ref into two separate functions\n  refs.c: split delete_ref_loose() into a separate flag-for-deletion and\n    commit phase\n  refs.c: change update_refs to run the commit loops once all work is\n    finished\n  refs.c: sort the refs by new_sha1 and merge the two update/delete\n    loops into one\n\n branch.c               |  10 ++++-\n builtin/commit.c       |   5 +++\n builtin/fetch.c        |   7 +++-\n builtin/receive-pack.c |   4 ++\n builtin/replace.c      |   6 ++-\n builtin/tag.c          |   6 ++-\n fast-import.c          |   7 +++-\n refs.c                 | 102 +++++++++++++++++++++++++++++++++----------------\n refs.h                 |   6 +++\n sequencer.c            |   4 ++\n walker.c               |   4 ++\n 11 files changed, 123 insertions(+), 38 deletions(-)\n\n-- \n1.9.1.478.ga5a8238.dirty\n"},{"id":"238629","messageId":"1397154625-11884-2-git-send-email-sahlberg@google.com","threadId":"36386","inReplyTo":"1397154625-11884-1-git-send-email-sahlberg@google.com","subject":"[PATCH 1/4] refs.c: split writing and commiting a ref into two separate functions","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-10T18:30:22Z","receivedAt":"2014-04-10T18:30:22Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change the function write_ref_sha1() to just write the ref but not\ncommit the ref or the lockfile.\nAdd a new function commit_ref_lock() that will commit the change done by\na previous write_ref_sha1().\nUpdate all callers of write_ref_sha1() to call commit_ref_lock().\n\nThe new pattern for updating a ref is now :\n\nlock = lock_ref_sha1_basic() (or varient of)\nwrite_ref_sha1(lock)\nunlock_ref(lock) | commit_ref_lock(lock)\n\nOnce write_ref_sha1() returns, the new ref has been written and the lock\nfile has been closed.\nAt that stage we can then either call unlock_ref() which will abort the\nupdate and delete the lock file withouth applying it, or call\ncommit_ref_lock() which will rename the lock file onto the ref file.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n branch.c               | 10 ++++++++--\n builtin/commit.c       |  5 +++++\n builtin/fetch.c        |  7 ++++++-\n builtin/receive-pack.c |  4 ++++\n builtin/replace.c      |  6 +++++-\n builtin/tag.c          |  6 +++++-\n fast-import.c          |  7 ++++++-\n refs.c                 | 39 ++++++++++++++++++++++++++++++---------\n refs.h                 |  4 ++++\n sequencer.c            |  4 ++++\n walker.c               |  4 ++++\n 11 files changed, 81 insertions(+), 15 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 660097b..903ea75 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -304,9 +304,15 @@ void create_branch(const char *head,\n \tif (real_ref && track)\n \t\tsetup_tracking(ref.buf + 11, real_ref, track, quiet);\n \n-\tif (!dont_change_ref)\n-\t\tif (write_ref_sha1(lock, sha1, msg) < 0)\n+\tif (!dont_change_ref) {\n+\t\tif (write_ref_sha1(lock, sha1, msg) < 0) {\n+\t\t\tunlock_ref(lock);\n \t\t\tdie_errno(_(\"Failed to write ref\"));\n+\t\t}\n+\t\tif (commit_ref_lock(lock) < 0) {\n+\t\t\tdie_errno(_(\"Failed to commit ref\"));\n+\t\t}\n+\t}\n \n \tstrbuf_release(&ref);\n \tfree(real_ref);\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex d9550c5..3d8a3a8 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1686,9 +1686,14 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"cannot lock HEAD ref\"));\n \t}\n \tif (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {\n+\t\tunlock_ref(ref_lock);\n \t\trollback_index_files();\n \t\tdie(_(\"cannot update HEAD ref\"));\n \t}\n+\tif (commit_ref_lock(ref_lock) < 0) {\n+\t\trollback_index_files();\n+\t\tdie(_(\"cannot commit HEAD ref\"));\n+\t}\n \n \tunlink(git_path(\"CHERRY_PICK_HEAD\"));\n \tunlink(git_path(\"REVERT_HEAD\"));\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 55f457c..ebfb854 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -388,7 +388,12 @@ static int s_update_ref(const char *action,\n \tif (!lock)\n \t\treturn errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :\n \t\t\t\t\t  STORE_REF_ERROR_OTHER;\n-\tif (write_ref_sha1(lock, ref->new_sha1, msg) < 0)\n+\tif (write_ref_sha1(lock, ref->new_sha1, msg) < 0) {\n+\t\tunlock_ref(lock);\n+\t\treturn errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :\n+\t\t\t\t\t  STORE_REF_ERROR_OTHER;\n+\t}\n+\tif (commit_ref_lock(lock) < 0)\n \t\treturn errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :\n \t\t\t\t\t  STORE_REF_ERROR_OTHER;\n \treturn 0;\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex c323081..4760274 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -587,8 +587,12 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\t\treturn \"failed to lock\";\n \t\t}\n \t\tif (write_ref_sha1(lock, new_sha1, \"push\")) {\n+\t\t\tunlock_ref(lock);\n \t\t\treturn \"failed to write\"; /* error() already called */\n \t\t}\n+\t\tif (commit_ref_lock(lock))\n+\t\t\treturn \"failed to commit\"; /* error() already called */\n+\n \t\treturn NULL; /* good */\n \t}\n }\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex b62420a..c09ff49 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -160,8 +160,12 @@ static int replace_object(const char *object_ref, const char *replace_ref,\n \tlock = lock_any_ref_for_update(ref, prev, 0, NULL);\n \tif (!lock)\n \t\tdie(\"%s: cannot lock the ref\", ref);\n-\tif (write_ref_sha1(lock, repl, NULL) < 0)\n+\tif (write_ref_sha1(lock, repl, NULL) < 0) {\n+\t\tunlock_ref(lock);\n \t\tdie(\"%s: cannot update the ref\", ref);\n+\t}\n+\tif (commit_ref_lock(lock) < 0)\n+\t\tdie(\"%s: cannot commit the ref\", ref);\n \n \treturn 0;\n }\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 40356e3..8653a64 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -644,8 +644,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tlock = lock_any_ref_for_update(ref.buf, prev, 0, NULL);\n \tif (!lock)\n \t\tdie(_(\"%s: cannot lock the ref\"), ref.buf);\n-\tif (write_ref_sha1(lock, object, NULL) < 0)\n+\tif (write_ref_sha1(lock, object, NULL) < 0) {\n+\t\tunlock_ref(lock);\n \t\tdie(_(\"%s: cannot update the ref\"), ref.buf);\n+\t}\n+\tif (commit_ref_lock(lock) < 0)\n+\t\tdie(_(\"%s: cannot commit the ref\"), ref.buf);\n \tif (force && !is_null_sha1(prev) && hashcmp(prev, object))\n \t\tprintf(_(\"Updated tag '%s' (was %s)\\n\"), tag, find_unique_abbrev(prev, DEFAULT_ABBREV));\n \ndiff --git a/fast-import.c b/fast-import.c\nindex fb4738d..65c41ad 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1706,8 +1706,13 @@ static int update_branch(struct branch *b)\n \t\t\treturn -1;\n \t\t}\n \t}\n-\tif (write_ref_sha1(lock, b->sha1, msg) < 0)\n+\tif (write_ref_sha1(lock, b->sha1, msg) < 0) {\n+\t\tunlock_ref(lock);\n \t\treturn error(\"Unable to update %s\", b->name);\n+\t}\n+\tif (commit_ref_lock(lock) < 0) {\n+\t\treturn error(\"Unable to commit %s\", b->name);\n+\t}\n \treturn 0;\n }\n \ndiff --git a/refs.c b/refs.c\nindex 28d5eca..9771237 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2633,9 +2633,14 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \tlock->force_write = 1;\n \thashcpy(lock->old_sha1, orig_sha1);\n \tif (write_ref_sha1(lock, orig_sha1, logmsg)) {\n+\t\tunlock_ref(lock);\n \t\terror(\"unable to write current sha1 into %s\", newrefname);\n \t\tgoto rollback;\n \t}\n+\tif (commit_ref_lock(lock)) {\n+\t\terror(\"unable to commit current sha1 into %s\", newrefname);\n+\t\tgoto rollback;\n+\t}\n \n \treturn 0;\n \n@@ -2649,8 +2654,12 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \tlock->force_write = 1;\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_sha1(lock, orig_sha1, NULL)) {\n+\t\tunlock_ref(lock);\n \t\terror(\"unable to write current sha1 into %s\", oldrefname);\n+\t}\n+\tif (commit_ref_lock(lock))\n+\t\terror(\"unable to commit current sha1 into %s\", oldrefname);\n \tlog_all_ref_updates = flag;\n \n  rollbacklog:\n@@ -2807,34 +2816,30 @@ int write_ref_sha1(struct ref_lock *lock,\n \tif (!lock)\n \t\treturn -1;\n \tif (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {\n-\t\tunlock_ref(lock);\n+\t\tlock->skipped_write = 1;\n \t\treturn 0;\n \t}\n \to = parse_object(sha1);\n \tif (!o) {\n \t\terror(\"Trying to write ref %s with nonexistent object %s\",\n \t\t\tlock->ref_name, sha1_to_hex(sha1));\n-\t\tunlock_ref(lock);\n \t\treturn -1;\n \t}\n \tif (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) {\n \t\terror(\"Trying to write non-commit object %s to branch %s\",\n \t\t\tsha1_to_hex(sha1), lock->ref_name);\n-\t\tunlock_ref(lock);\n \t\treturn -1;\n \t}\n \tif (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 ||\n \t    write_in_full(lock->lock_fd, &term, 1) != 1\n \t\t|| close_ref(lock) < 0) {\n \t\terror(\"Couldn't write %s\", lock->lk->filename);\n-\t\tunlock_ref(lock);\n \t\treturn -1;\n \t}\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 \t     log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) < 0)) {\n-\t\tunlock_ref(lock);\n \t\treturn -1;\n \t}\n \tif (strcmp(lock->orig_ref_name, \"HEAD\") != 0) {\n@@ -2858,7 +2863,12 @@ int write_ref_sha1(struct ref_lock *lock,\n \t\t    !strcmp(head_ref, lock->ref_name))\n \t\t\tlog_ref_write(\"HEAD\", lock->old_sha1, sha1, logmsg);\n \t}\n-\tif (commit_ref(lock)) {\n+\treturn 0;\n+}\n+\n+int commit_ref_lock(struct ref_lock *lock)\n+{\n+        if (!lock->skipped_write && commit_ref(lock)) {\n \t\terror(\"Couldn't set %s\", lock->ref_name);\n \t\tunlock_ref(lock);\n \t\treturn -1;\n@@ -3272,10 +3282,17 @@ int update_ref(const char *action, const char *refname,\n \t       int flags, enum action_on_err onerr)\n {\n \tstruct ref_lock *lock;\n+\tint ret;\n+\n \tlock = update_ref_lock(refname, oldval, flags, NULL, onerr);\n \tif (!lock)\n \t\treturn 1;\n-\treturn update_ref_write(action, refname, sha1, lock, onerr);\n+\tret = update_ref_write(action, refname, sha1, lock, onerr);\n+\tif (ret)\n+\t\tunlock_ref(lock);\n+\telse\n+\t\tret = commit_ref_lock(lock);\n+\treturn ret;\n }\n \n static int ref_update_compare(const void *r1, const void *r2)\n@@ -3351,7 +3368,11 @@ int update_refs(const char *action, const struct ref_update **updates_orig,\n \t\t\t\t\t       updates[i]->ref_name,\n \t\t\t\t\t       updates[i]->new_sha1,\n \t\t\t\t\t       locks[i], onerr);\n-\t\t\tlocks[i] = NULL; /* freed by update_ref_write */\n+\t\t\tif (ret)\n+\t\t\t\tunlock_ref(locks[i]);\n+\t\t\telse\n+\t\t\t\tcommit_ref_lock(locks[i]);\n+\t\t\tlocks[i] = NULL;\n \t\t\tif (ret)\n \t\t\t\tgoto cleanup;\n \t\t}\ndiff --git a/refs.h b/refs.h\nindex 87a1a79..0382128 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -8,6 +8,7 @@ struct ref_lock {\n \tunsigned char old_sha1[20];\n \tint lock_fd;\n \tint force_write;\n+\tint skipped_write;\n };\n \n /**\n@@ -165,6 +166,9 @@ extern void unlock_ref(struct ref_lock *lock);\n /** Writes sha1 into the ref specified by the lock. **/\n extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);\n \n+/** Commit any changes done to the ref specified by the lock. **/\n+extern int commit_ref_lock(struct ref_lock *lock);\n+\n /** Setup reflog before using. **/\n int log_ref_setup(const char *ref_name, char *logfile, int bufsize);\n \ndiff --git a/sequencer.c b/sequencer.c\nindex bde5f04..ffadf82 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -283,6 +283,10 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n \t\t\t\t\t   0, NULL);\n \tstrbuf_addf(&sb, \"%s: fast-forward\", action_name(opts));\n \tret = write_ref_sha1(ref_lock, to, sb.buf);\n+\tif (ret)\n+\t\tunlock_ref(ref_lock);\n+\telse\n+\t\tret |= commit_ref_lock(ref_lock);\n \tstrbuf_release(&sb);\n \treturn ret;\n }\ndiff --git a/walker.c b/walker.c\nindex 1dd86b8..5ce5a1d 100644\n--- a/walker.c\n+++ b/walker.c\n@@ -295,6 +295,10 @@ int walker_fetch(struct walker *walker, int targets, char **target,\n \t\tif (!write_ref || !write_ref[i])\n \t\t\tcontinue;\n \t\tret = write_ref_sha1(lock[i], &sha1[20 * i], msg ? msg : \"fetch (unknown)\");\n+\t\tif (ret)\n+\t\t\tunlock_ref(lock[i]);\n+\t\telse\n+\t\t\tret |= commit_ref_lock(lock[i]);\n \t\tlock[i] = NULL;\n \t\tif (ret)\n \t\t\tgoto unlock_and_fail;\n-- \n1.9.1.478.ga5a8238.dirty\n"},{"id":"238627","messageId":"1397154625-11884-3-git-send-email-sahlberg@google.com","threadId":"36386","inReplyTo":"1397154625-11884-1-git-send-email-sahlberg@google.com","subject":"[PATCH 2/4] refs.c: split delete_ref_loose() into a separate flag-for-deletion and commit phase","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-10T18:30:23Z","receivedAt":"2014-04-10T18:30:23Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change delete_ref_loose()) to just flag that a ref is to be deleted but do\nnot actually unlink the files.\nChange commit_ref_lock() so that it will unlink refs that are flagged for\ndeletion.\nChange all callers of delete_ref_loose() to explicitely call commit_ref_lock()\nto commit the deletion.\n\nThe new pattern for deleting loose refs thus become:\n\nlock = lock_ref_sha1_basic() (or varient of)\ndelete_ref_loose(lock)\nunlock_ref(lock) | commit_ref_lock(lock)\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n refs.c | 31 ++++++++++++++++++++-----------\n refs.h |  2 ++\n 2 files changed, 22 insertions(+), 11 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 9771237..038614d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2484,16 +2484,9 @@ static int repack_without_ref(const char *refname)\n \n static int delete_ref_loose(struct ref_lock *lock, int flag)\n {\n-\tif (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {\n-\t\t/* loose */\n-\t\tint err, i = strlen(lock->lk->filename) - 5; /* .lock */\n-\n-\t\tlock->lk->filename[i] = 0;\n-\t\terr = unlink_or_warn(lock->lk->filename);\n-\t\tlock->lk->filename[i] = '.';\n-\t\tif (err && errno != ENOENT)\n-\t\t\treturn 1;\n-\t}\n+\tlock->delete_ref = 1;\n+\tlock->delete_flag = flag;\n+\n \treturn 0;\n }\n \n@@ -2515,7 +2508,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n \n \tunlink_or_warn(git_path(\"logs/%s\", lock->ref_name));\n \tclear_loose_ref_cache(&ref_cache);\n-\tunlock_ref(lock);\n+\tret |= commit_ref_lock(lock);\n \treturn ret;\n }\n \n@@ -2868,6 +2861,20 @@ int write_ref_sha1(struct ref_lock *lock,\n \n int commit_ref_lock(struct ref_lock *lock)\n {\n+\tif (lock->delete_ref) {\n+\t\tint flag = lock->delete_flag;\n+\n+\t\tif (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {\n+\t\t\t/* loose */\n+\t\t\tint err, i = strlen(lock->lk->filename) - 5; /* .lock */\n+\n+\t\t\tlock->lk->filename[i] = 0;\n+\t\t\terr = unlink_or_warn(lock->lk->filename);\n+\t\t\tlock->lk->filename[i] = '.';\n+\t\t\tif (err && errno != ENOENT)\n+\t\t\t\treturn 1;\n+\t\t}\n+\t} else\n         if (!lock->skipped_write && commit_ref(lock)) {\n \t\terror(\"Couldn't set %s\", lock->ref_name);\n \t\tunlock_ref(lock);\n@@ -3382,6 +3389,8 @@ int update_refs(const char *action, const struct ref_update **updates_orig,\n \t\tif (locks[i]) {\n \t\t\tdelnames[delnum++] = locks[i]->ref_name;\n \t\t\tret |= delete_ref_loose(locks[i], types[i]);\n+\t\t\tret |= commit_ref_lock(locks[i]);\n+\t\t\tlocks[i] = NULL;\n \t\t}\n \tret |= repack_without_refs(delnames, delnum);\n \tfor (i = 0; i < delnum; i++)\ndiff --git a/refs.h b/refs.h\nindex 0382128..2230d67 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -9,6 +9,8 @@ struct ref_lock {\n \tint lock_fd;\n \tint force_write;\n \tint skipped_write;\n+\tint delete_ref;\n+\tint delete_flag;\n };\n \n /**\n-- \n1.9.1.478.ga5a8238.dirty\n"},{"id":"238628","messageId":"1397154625-11884-4-git-send-email-sahlberg@google.com","threadId":"36386","inReplyTo":"1397154625-11884-1-git-send-email-sahlberg@google.com","subject":"[PATCH 3/4] refs.c: change update_refs to run the commit loops once all work is finished","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-10T18:30:24Z","receivedAt":"2014-04-10T18:30:24Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"During update_refs we will both be deleting refs as well as updating/changing\nthe refs.\nSince both delete and update now use the same lock/update/commit pattern\n\n    lock = lock_ref_sha1_basic() (or varient of)\n    write_ref_sha1(lock)/delete_ref_loose(lock)\n    unlock_ref(lock) | commit_ref_lock(lock)\n\nwe can now simplify update_update refs to have one loop that locks all\ninvolved refs.\nA second loop that writes or flags for deletion, but does not commit, all\nthe refs.\nAnd a final third loop that commits all the refs once all the work and\npreparations are complete.\n\nThis makes updating/deleting multiple refs 'more atomic' since we will not start\nthe commit phase until all the preparations have completed successfully.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n refs.c | 38 +++++++++++++++++++++-----------------\n 1 file changed, 21 insertions(+), 17 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 038614d..1678e12 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3368,34 +3368,38 @@ int update_refs(const char *action, const struct ref_update **updates_orig,\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\tif (!is_null_sha1(updates[i]->new_sha1)) {\n+\t/* Go through and prepare all updates and deletes */\n+\tfor (i = 0; i < n; i++) {\n+\t\tif (!is_null_sha1(updates[i]->new_sha1))\n \t\t\tret = update_ref_write(action,\n \t\t\t\t\t       updates[i]->ref_name,\n \t\t\t\t\t       updates[i]->new_sha1,\n \t\t\t\t\t       locks[i], onerr);\n-\t\t\tif (ret)\n-\t\t\t\tunlock_ref(locks[i]);\n-\t\t\telse\n-\t\t\t\tcommit_ref_lock(locks[i]);\n-\t\t\tlocks[i] = NULL;\n-\t\t\tif (ret)\n-\t\t\t\tgoto cleanup;\n+\t\telse {\n+\t\t\tdelnames[delnum++] = locks[i]->ref_name;\n+\t\t\tret = delete_ref_loose(locks[i], types[i]);\n \t\t}\n+\t\tif (ret)\n+\t\t\tgoto cleanup;\n+\t}\n \n-\t/* Perform deletes now that updates are safely completed */\n+\tret = repack_without_refs(delnames, delnum);\n+\tfor (i = 0; i < delnum; i++)\n+\t\tunlink_or_warn(git_path(\"logs/%s\", delnames[i]));\n+\tclear_loose_ref_cache(&ref_cache);\n+\n+\t/* Perform updates first so live commits remain referenced */\n+\tfor (i = 0; i < n; i++)\n+\t\tif (locks[i] && !locks[i]->delete_ref) {\n+\t\t\tret |= commit_ref_lock(locks[i]);\n+\t\t\tlocks[i] = NULL;\n+\t\t}\n+\t/* And finally perform all deletes */\n \tfor (i = 0; i < n; i++)\n \t\tif (locks[i]) {\n-\t\t\tdelnames[delnum++] = locks[i]->ref_name;\n-\t\t\tret |= delete_ref_loose(locks[i], types[i]);\n \t\t\tret |= commit_ref_lock(locks[i]);\n \t\t\tlocks[i] = NULL;\n \t\t}\n-\tret |= repack_without_refs(delnames, delnum);\n-\tfor (i = 0; i < delnum; i++)\n-\t\tunlink_or_warn(git_path(\"logs/%s\", delnames[i]));\n-\tclear_loose_ref_cache(&ref_cache);\n \n cleanup:\n \tfor (i = 0; i < n; i++)\n-- \n1.9.1.478.ga5a8238.dirty\n"},{"id":"238634","messageId":"1397154625-11884-5-git-send-email-sahlberg@google.com","threadId":"36386","inReplyTo":"1397154625-11884-1-git-send-email-sahlberg@google.com","subject":"[PATCH 4/4] refs.c: sort the refs by new_sha1 and merge the two update/delete loops into one","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-10T18:30:25Z","receivedAt":"2014-04-10T18:30:25Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"We want to make sure that we update all refs before we delete any refs\nso that there is no point in time where the tips may not be reachable\nfrom any ref in the system.\nWe currently acheive this by first looping over all updates and applying\nthem before we run a second loop and perform all the deletes.\n\nIf we sort the new sha1 for all the refs so that a deleted ref,\nwith sha1 0{40} comes at the end of the array, then we can just run\na single loop and still guarantee that all updates happen before\nany deletes.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n refs.c | 18 +++++++++++-------\n 1 file changed, 11 insertions(+), 7 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 1678e12..453318e 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3309,6 +3309,15 @@ static int ref_update_compare(const void *r1, const void *r2)\n \treturn strcmp((*u1)->ref_name, (*u2)->ref_name);\n }\n \n+static int ref_delete_compare(const void *r1, const void *r2)\n+{\n+\tconst struct ref_update * const *u1 = r1;\n+\tconst struct ref_update * const *u2 = r2;\n+\n+\t/* -strcmp so that 0{40} sorts to the end */\n+\treturn -strcmp((*u1)->new_sha1, (*u2)->new_sha1);\n+}\n+\n static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n \t\t\t\t\tenum action_on_err onerr)\n {\n@@ -3388,13 +3397,8 @@ int update_refs(const char *action, const struct ref_update **updates_orig,\n \t\tunlink_or_warn(git_path(\"logs/%s\", delnames[i]));\n \tclear_loose_ref_cache(&ref_cache);\n \n-\t/* Perform updates first so live commits remain referenced */\n-\tfor (i = 0; i < n; i++)\n-\t\tif (locks[i] && !locks[i]->delete_ref) {\n-\t\t\tret |= commit_ref_lock(locks[i]);\n-\t\t\tlocks[i] = NULL;\n-\t\t}\n-\t/* And finally perform all deletes */\n+\t/* Sort the array so that we perform all updates before any deletes */\n+\tqsort(updates, n, sizeof(*updates), ref_delete_compare);\n \tfor (i = 0; i < n; i++)\n \t\tif (locks[i]) {\n \t\t\tret |= commit_ref_lock(locks[i]);\n-- \n1.9.1.478.ga5a8238.dirty\n"},{"id":"238637","messageId":"xmqq38hlxbh3.fsf@gitster.dls.corp.google.com","threadId":"36386","inReplyTo":"1397154625-11884-1-git-send-email-sahlberg@google.com","subject":"Re: [PATCH 0/4] Make update_refs more atomic V2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-10T18:53:28Z","receivedAt":"2014-04-10T18:53:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"CC'ing Michael who has been active in this area as an area expert.\n\nRonnie, please make it a habit to run something like\n\n    $ git shortlog --no-merges --since=18.months <affected paths>...\n\nto help you decide who your series may want to be reviewed by,\nbefore sending them.\n\nThanks.\n\nRonnie Sahlberg <sahlberg@google.com> writes:\n\n> refs.c:update_refs() intermingles doing updates and checks with actually\n> applying changes to the refs in loops that abort on error.\n> This is done one ref at a time and means that if an error is detected that\n> will fail the operation after only some of the ref operations have been\n> been updated on the disk.\n>\n> These patches change the update and delete functions to use a three\n> call pattern of\n>\n> 1, lock\n> 2, update, or flag for deletion\n> 3, apply on disk\n>\n> In the final patch I change update_refs to perform these actions in three\n> separate loops where the final loop to 'apply on disk' all the changes will\n> only be performed if there were no error conditions detected during any of\n> previous loops.\n>\n> This should make the changes of refs in update_refs slightly more atomic.\n>\n>\n> This may overlap with other current patch series for making refs updates\n> more atomic which may mean these patches become obsolete, but I would still\n> like some review and feedback on these changes.\n>\n> Version 2:\n> Updates and fixes based on Junio's feedback.\n> * Fix the subject line for patches so they comply with the project standard.\n> * Redo the update/delete loops so that we maintain the correct order of\n>   operations. Perform all updates first, then perform the deletes.\n> * Add an additional patch that allows us to do the update/delete in the correct\n>   order from within a single loop by first sorting the refs so that deletes\n>   are after all non-deletes.\n>\n>\n> Ronnie Sahlberg (4):\n>   refs.c: split writing and commiting a ref into two separate functions\n>   refs.c: split delete_ref_loose() into a separate flag-for-deletion and\n>     commit phase\n>   refs.c: change update_refs to run the commit loops once all work is\n>     finished\n>   refs.c: sort the refs by new_sha1 and merge the two update/delete\n>     loops into one\n>\n>  branch.c               |  10 ++++-\n>  builtin/commit.c       |   5 +++\n>  builtin/fetch.c        |   7 +++-\n>  builtin/receive-pack.c |   4 ++\n>  builtin/replace.c      |   6 ++-\n>  builtin/tag.c          |   6 ++-\n>  fast-import.c          |   7 +++-\n>  refs.c                 | 102 +++++++++++++++++++++++++++++++++----------------\n>  refs.h                 |   6 +++\n>  sequencer.c            |   4 ++\n>  walker.c               |   4 ++\n>  11 files changed, 123 insertions(+), 38 deletions(-)\n"},{"id":"238639","messageId":"xmqqy4zdvwjg.fsf@gitster.dls.corp.google.com","threadId":"36386","inReplyTo":"1397154625-11884-5-git-send-email-sahlberg@google.com","subject":"Re: [PATCH 4/4] refs.c: sort the refs by new_sha1 and merge the two update/delete loops into one","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-10T19:01:23Z","receivedAt":"2014-04-10T19:01:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ronnie Sahlberg <sahlberg@google.com> writes:\n\n> We want to make sure that we update all refs before we delete any refs\n> so that there is no point in time where the tips may not be reachable\n> from any ref in the system.\n> We currently acheive this by first looping over all updates and applying\n> them before we run a second loop and perform all the deletes.\n>\n> If we sort the new sha1 for all the refs so that a deleted ref,\n> with sha1 0{40} comes at the end of the array, then we can just run\n> a single loop and still guarantee that all updates happen before\n> any deletes.\n>\n> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n> ---\n>  refs.c | 18 +++++++++++-------\n>  1 file changed, 11 insertions(+), 7 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 1678e12..453318e 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -3309,6 +3309,15 @@ static int ref_update_compare(const void *r1, const void *r2)\n>  \treturn strcmp((*u1)->ref_name, (*u2)->ref_name);\n>  }\n>  \n> +static int ref_delete_compare(const void *r1, const void *r2)\n> +{\n> +\tconst struct ref_update * const *u1 = r1;\n> +\tconst struct ref_update * const *u2 = r2;\n> +\n> +\t/* -strcmp so that 0{40} sorts to the end */\n> +\treturn -strcmp((*u1)->new_sha1, (*u2)->new_sha1);\n\nTwo glitches:\n\n - Didn't you mean hashcmp()?\n\n - Don't you have an explicit ->delete_ref field that is a more\n   direct way, than relying on the convention \"updating to 0{40}\n   is to delte\", to express this?\n\nIn other words, wouldn't it be more like\n\n\treturn !(*u1)->delete_ref - !(*u2)->delete_ref;\n\nor something (I may be wrong about the sign, tho---I didn't think\ncarefully)?\n\n\n> +}\n> +\n>  static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n>  \t\t\t\t\tenum action_on_err onerr)\n>  {\n> @@ -3388,13 +3397,8 @@ int update_refs(const char *action, const struct ref_update **updates_orig,\n>  \t\tunlink_or_warn(git_path(\"logs/%s\", delnames[i]));\n>  \tclear_loose_ref_cache(&ref_cache);\n>  \n> -\t/* Perform updates first so live commits remain referenced */\n> -\tfor (i = 0; i < n; i++)\n> -\t\tif (locks[i] && !locks[i]->delete_ref) {\n> -\t\t\tret |= commit_ref_lock(locks[i]);\n> -\t\t\tlocks[i] = NULL;\n> -\t\t}\n> -\t/* And finally perform all deletes */\n> +\t/* Sort the array so that we perform all updates before any deletes */\n> +\tqsort(updates, n, sizeof(*updates), ref_delete_compare);\n>  \tfor (i = 0; i < n; i++)\n>  \t\tif (locks[i]) {\n>  \t\t\tret |= commit_ref_lock(locks[i]);\n"},{"id":"238656","messageId":"CAL=YDWnADZmCa3v1gk37bbcQcnnAZ=+hi-OSFVdneWrP4_Lv0A@mail.gmail.com","threadId":"36386","inReplyTo":"xmqqy4zdvwjg.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 4/4] refs.c: sort the refs by new_sha1 and merge the two update/delete loops into one","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-10T19:40:30Z","receivedAt":"2014-04-10T19:40:30Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Thu, Apr 10, 2014 at 12:01 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Ronnie Sahlberg <sahlberg@google.com> writes:\n>\n>> We want to make sure that we update all refs before we delete any refs\n>> so that there is no point in time where the tips may not be reachable\n>> from any ref in the system.\n>> We currently acheive this by first looping over all updates and applying\n>> them before we run a second loop and perform all the deletes.\n>>\n>> If we sort the new sha1 for all the refs so that a deleted ref,\n>> with sha1 0{40} comes at the end of the array, then we can just run\n>> a single loop and still guarantee that all updates happen before\n>> any deletes.\n>>\n>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n>> ---\n>>  refs.c | 18 +++++++++++-------\n>>  1 file changed, 11 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/refs.c b/refs.c\n>> index 1678e12..453318e 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -3309,6 +3309,15 @@ static int ref_update_compare(const void *r1, const void *r2)\n>>       return strcmp((*u1)->ref_name, (*u2)->ref_name);\n>>  }\n>>\n>> +static int ref_delete_compare(const void *r1, const void *r2)\n>> +{\n>> +     const struct ref_update * const *u1 = r1;\n>> +     const struct ref_update * const *u2 = r2;\n>> +\n>> +     /* -strcmp so that 0{40} sorts to the end */\n>> +     return -strcmp((*u1)->new_sha1, (*u2)->new_sha1);\n>\n> Two glitches:\n>\n>  - Didn't you mean hashcmp()?\n>\n>  - Don't you have an explicit ->delete_ref field that is a more\n>    direct way, than relying on the convention \"updating to 0{40}\n>    is to delte\", to express this?\n>\n> In other words, wouldn't it be more like\n>\n>         return !(*u1)->delete_ref - !(*u2)->delete_ref;\n>\n> or something (I may be wrong about the sign, tho---I didn't think\n> carefully)?\n\nI don't have access to delete_ref from 'struct ref_update'.\n\nBut much worse is that the patch is completely bogus.\nSorting and changing the order of the items in the updates[] array\ndoes not affect the ordering of the items in the locks[] array\nso this can not work.\n\nI will delete this bogus patch from any new version of the patch series.\n\n\nregards\nronnie sahlberg\n\n>\n>\n>> +}\n>> +\n>>  static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n>>                                       enum action_on_err onerr)\n>>  {\n>> @@ -3388,13 +3397,8 @@ int update_refs(const char *action, const struct ref_update **updates_orig,\n>>               unlink_or_warn(git_path(\"logs/%s\", delnames[i]));\n>>       clear_loose_ref_cache(&ref_cache);\n>>\n>> -     /* Perform updates first so live commits remain referenced */\n>> -     for (i = 0; i < n; i++)\n>> -             if (locks[i] && !locks[i]->delete_ref) {\n>> -                     ret |= commit_ref_lock(locks[i]);\n>> -                     locks[i] = NULL;\n>> -             }\n>> -     /* And finally perform all deletes */\n>> +     /* Sort the array so that we perform all updates before any deletes */\n>> +     qsort(updates, n, sizeof(*updates), ref_delete_compare);\n>>       for (i = 0; i < n; i++)\n>>               if (locks[i]) {\n>>                       ret |= commit_ref_lock(locks[i]);\n"},{"id":"238657","messageId":"xmqqlhvdvtfe.fsf@gitster.dls.corp.google.com","threadId":"36386","inReplyTo":"xmqq38hlxbh3.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/4] Make update_refs more atomic V2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-10T20:08:37Z","receivedAt":"2014-04-10T20:08:37Z","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> CC'ing Michael who has been active in this area as an area expert.\n>\n> Ronnie, please make it a habit to run something like\n>\n>     $ git shortlog --no-merges --since=18.months <affected paths>...\n>\n> to help you decide who your series may want to be reviewed by,\n> before sending them.\n>\n> Thanks.\n\nAnother tip.  \"git format-patch -v3\" would produce\n\n\tSubject: [PATCH v3 0/4] blah blah blah...\n"}]}