{"thread":{"id":"36411","subject":"[PATCH v4 2/3] refs.c: split delete_ref_loose() into a separate flag-for-deletion and commit phase","startedAt":"2014-04-14T18:29:20Z","lastAt":"2014-04-16T21:51:09Z","messageCount":16,"participants":["Ronnie Sahlberg","Junio C Hamano","Michael Haggerty"],"isPatch":true,"patchVersion":4,"patchTotal":3},"messages":[{"id":"238856","messageId":"1397500163-7617-1-git-send-email-sahlberg@google.com","threadId":"36411","inReplyTo":null,"subject":"[PATCH v4 0/3] Make update refs more atomic","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-14T18:29:20Z","receivedAt":"2014-04-14T18:29:20Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"refs.c:ref_transaction_commit() intermingles doing updates and checks with\nactually applying 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 partway through the list of refs to update we\nwill end up with some changes applied to disk and others not.\n\nWithout having transaction support from the filesystem, it is hard to\nmake an update that involves multiple refs to guarantee atomicity, but we\ncan do a somewhat better than we currently do.\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  (rename() or unlink())\n\nWhen a transaction is commited we first do all the locking, preparations\nand most of the error checking before we actually start applying any changes\nto the filesystem store.\n\nThis means that more of the error cases that will fail the commit\nwill trigger before we start doing any changes to the actual files.\n\n\nThis should make the changes of refs in refs_transaction_commit slightly\nmore atomic.\n\n\nVersion 4:\n* Fix a bug in fast-import.c:dump_tags and make sure no tests fail\n\nVersion 3:\n* Rebased onto mhagger/ref-transactions.\n* Removed the patch to do update/delete from a single loop.\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 (3):\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 ref_transaction_commit to run the commit loops once all\n    work is finished\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          | 18 ++++++++--\n refs.c                 | 98 +++++++++++++++++++++++++++++++++-----------------\n refs.h                 |  6 ++++\n sequencer.c            |  4 +++\n walker.c               |  4 +++\n 11 files changed, 129 insertions(+), 39 deletions(-)\n\n-- \n1.9.1.505.gd05696d\n"},{"id":"238858","messageId":"1397500163-7617-2-git-send-email-sahlberg@google.com","threadId":"36411","inReplyTo":"1397500163-7617-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 1/3] refs.c: split writing and commiting a ref into two separate functions","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-14T18:29:21Z","receivedAt":"2014-04-14T18:29:21Z","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          | 18 ++++++++++++++++--\n refs.c                 | 41 +++++++++++++++++++++++++++++++----------\n refs.h                 |  4 ++++\n sequencer.c            |  4 ++++\n walker.c               |  4 ++++\n 11 files changed, 92 insertions(+), 17 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..f732bfb 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 \n@@ -1732,8 +1737,17 @@ static void dump_tags(void)\n \tfor (t = first_tag; t; t = t->next_tag) {\n \t\tsprintf(ref_name, \"tags/%s\", t->name);\n \t\tlock = lock_ref_sha1(ref_name, NULL);\n-\t\tif (!lock || write_ref_sha1(lock, t->sha1, msg) < 0)\n+\t\tif (!lock) {\n+\t\t\tfailure |= error(\"Unable to lock %s\", ref_name);\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (write_ref_sha1(lock, t->sha1, msg) < 0) {\n \t\t\tfailure |= error(\"Unable to update %s\", ref_name);\n+\t\t\tunlock_ref(lock);\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (commit_ref_lock(lock) < 0)\n+\t\t\tfailure |= error(\"Unable to commit %s\", ref_name);\n \t}\n }\n \ndiff --git a/refs.c b/refs.c\nindex 728a761..646afd7 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+\tif (!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@@ -3375,10 +3385,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@@ -3453,7 +3470,11 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t\t       update->refname,\n \t\t\t\t\t       update->new_sha1,\n \t\t\t\t\t       update->lock, onerr);\n-\t\t\tupdate->lock = NULL; /* freed by update_ref_write */\n+\t\t\tif (ret)\n+\t\t\t\tunlock_ref(update->lock);\n+\t\t\telse\n+\t\t\t\tcommit_ref_lock(update->lock);\n+\t\t\tupdate->lock = NULL;\n \t\t\tif (ret)\n \t\t\t\tgoto cleanup;\n \t\t}\n@@ -3464,7 +3485,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\tstruct ref_update *update = updates[i];\n \n \t\tif (update->lock) {\n-\t\t\tdelnames[delnum++] = update->lock->ref_name;\n+\t\t\tdelnames[delnum++] = update->refname;\n \t\t\tret |= delete_ref_loose(update->lock, update->type);\n \t\t}\n \t}\ndiff --git a/refs.h b/refs.h\nindex 0f08def..f14a417 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 struct ref_transaction;\n@@ -153,6 +154,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 *refname, 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.505.gd05696d\n"},{"id":"238853","messageId":"1397500163-7617-3-git-send-email-sahlberg@google.com","threadId":"36411","inReplyTo":"1397500163-7617-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 2/3] 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-14T18:29:22Z","receivedAt":"2014-04-14T18:29:22Z","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 | 32 ++++++++++++++++++++------------\n refs.h |  2 ++\n 2 files changed, 22 insertions(+), 12 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 646afd7..a14addb 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,7 +2861,20 @@ int write_ref_sha1(struct ref_lock *lock,\n \n int commit_ref_lock(struct ref_lock *lock)\n {\n-\tif (!lock->skipped_write && commit_ref(lock)) {\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 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@@ -3487,6 +3493,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\tif (update->lock) {\n \t\t\tdelnames[delnum++] = update->refname;\n \t\t\tret |= delete_ref_loose(update->lock, update->type);\n+\t\t\tret |= commit_ref_lock(update->lock);\n+\t\t\tupdate->lock = NULL;\n \t\t}\n \t}\n \ndiff --git a/refs.h b/refs.h\nindex f14a417..223be30 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 struct ref_transaction;\n-- \n1.9.1.505.gd05696d\n"},{"id":"238855","messageId":"1397500163-7617-4-git-send-email-sahlberg@google.com","threadId":"36411","inReplyTo":"1397500163-7617-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 3/3] refs.c: change ref_transaction_commit to run the commit loops once all work is finished","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-14T18:29:23Z","receivedAt":"2014-04-14T18:29:23Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"During a transaction commit we will both update and delete refs.\nSince both update and delete now use the same 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 ref_transaction_commit 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 | 39 ++++++++++++++++++++++-----------------\n 1 file changed, 22 insertions(+), 17 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex a14addb..87193c7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3467,42 +3467,47 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t}\n \t}\n \n-\t/* Perform updates first so live commits remain referenced */\n+\t/* Prepare all the updates/deletes */\n \tfor (i = 0; i < n; i++) {\n \t\tstruct ref_update *update = updates[i];\n \n-\t\tif (!is_null_sha1(update->new_sha1)) {\n+\t\tif (!is_null_sha1(update->new_sha1))\n \t\t\tret = update_ref_write(msg,\n \t\t\t\t\t       update->refname,\n \t\t\t\t\t       update->new_sha1,\n \t\t\t\t\t       update->lock, onerr);\n-\t\t\tif (ret)\n-\t\t\t\tunlock_ref(update->lock);\n-\t\t\telse\n-\t\t\t\tcommit_ref_lock(update->lock);\n-\t\t\tupdate->lock = NULL;\n-\t\t\tif (ret)\n-\t\t\t\tgoto cleanup;\n+\t\telse {\n+\t\t\tdelnames[delnum++] = update->refname;\n+\t\t\tret = delete_ref_loose(update->lock, update->type);\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\tstruct ref_update *update = updates[i];\n+\n+\t\tif (update->lock && !update->lock->delete_ref) {\n+\t\t\tret |= commit_ref_lock(update->lock);\n+\t\t\tupdate->lock = NULL;\n+\t\t}\n+\t}\n+\t/* And finally perform all deletes */\n \tfor (i = 0; i < n; i++) {\n \t\tstruct ref_update *update = updates[i];\n \n \t\tif (update->lock) {\n-\t\t\tdelnames[delnum++] = update->refname;\n-\t\t\tret |= delete_ref_loose(update->lock, update->type);\n \t\t\tret |= commit_ref_lock(update->lock);\n \t\t\tupdate->lock = NULL;\n \t\t}\n \t}\n \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 \t\tif (updates[i]->lock)\n-- \n1.9.1.505.gd05696d\n"},{"id":"238863","messageId":"xmqqeh0zoe19.fsf@gitster.dls.corp.google.com","threadId":"36411","inReplyTo":"1397500163-7617-1-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v4 0/3] Make update refs more atomic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-14T20:24:18Z","receivedAt":"2014-04-14T20:24:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks; will queue.\n"},{"id":"238876","messageId":"534CD376.7080108@alum.mit.edu","threadId":"36411","inReplyTo":"1397500163-7617-1-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v4 0/3] Make update refs more atomic","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-15T06:36:38Z","receivedAt":"2014-04-15T06:36:38Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/14/2014 08:29 PM, Ronnie Sahlberg wrote:\n> refs.c:ref_transaction_commit() intermingles doing updates and checks with\n> actually 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 partway through the list of refs to update we\n> will end up with some changes applied to disk and others not.\n> \n> Without having transaction support from the filesystem, it is hard to\n> make an update that involves multiple refs to guarantee atomicity, but we\n> can do a somewhat better than we currently do.\n\nIt took me a moment to understand what you were talking about here,\nbecause the code for ref_transaction_commit() already seems\nsuperficially to do reference modifications in phases.  The problem is\nthat write_ref_sha1() internally contains additional checks that can\nfail in \"normal\" circumstances.  So the most important part of this\npatch series is allowing those checks to be done before committing anything.\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  (rename() or unlink())\n> \n> When a transaction is commited we first do all the locking, preparations\n> and most of the error checking before we actually start applying any changes\n> to the filesystem store.\n> \n> This means that more of the error cases that will fail the commit\n> will trigger before we start doing any changes to the actual files.\n> \n> \n> This should make the changes of refs in refs_transaction_commit slightly\n> more atomic.\n> [...]\n\nYes, this is a good and important goal.\n\nI wonder, however, whether your approach of changing callers from\n\n    lock = lock_ref_sha1_basic() (or varient of)\n    write_ref_sha1(lock)\n\nto\n\n    lock = lock_ref_sha1_basic() (or varient of)\n    write_ref_sha1(lock)\n    unlock_ref(lock) | commit_ref_lock(lock)\n\nis not doing work that we will soon need to rework.  Would it be jumping\nthe gun to change the callers to\n\n    transaction = ref_transaction_begin();\n    ref_transaction_{update,delete,etc}(transaction, ...);\n    ref_transaction_{commit,rollback}(transaction, ...);\n\ninstead?  Then we could bury the details of calling write_ref_sha1() and\ncommit_lock_ref() inside ref_transaction_commit() rather than having to\nexpose them in the public API.\n\nI suspect that the answer is \"no, ref transactions are not yet powerful\nenough to do everything that the callers need\".  But then I would\nsuggest that we *make* them powerful enough and *then* make the change\nat the callers.\n\nI'm not saying that we shouldn't accept your change as a first step [1]\nand do the next step later, but wanted to get your reaction about making\nthe first step a bit more ambitious.\n\nMichael\n\n[1] Though I still need to review your patch series in detail.\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238886","messageId":"534D152C.7090607@alum.mit.edu","threadId":"36411","inReplyTo":"1397500163-7617-2-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v4 1/3] refs.c: split writing and commiting a ref into two separate functions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-15T11:17:00Z","receivedAt":"2014-04-15T11:17:00Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"See my comment to your cover letter where I suggest using ref\ntransactions instead of making callers deal with even more of the\ndetails of updating references.  But I will comment on these patches\nanyway, in case you'd rather leave them closer to the current form.\n\nOn 04/14/2014 08:29 PM, Ronnie Sahlberg wrote:\n> Change the function write_ref_sha1() to just write the ref but not\n> commit the ref or the lockfile.\n> Add a new function commit_ref_lock() that will commit the change done by\n> a previous write_ref_sha1().\n> Update all callers of write_ref_sha1() to call commit_ref_lock().\n> \n> The new pattern for updating a ref is now :\n> \n> lock = lock_ref_sha1_basic() (or varient of)\n\ns/varient/variant/\n\n> write_ref_sha1(lock)\n> unlock_ref(lock) | commit_ref_lock(lock)\n> \n> Once write_ref_sha1() returns, the new ref has been written and the lock\n> file has been closed.\n> At that stage we can then either call unlock_ref() which will abort the\n> update and delete the lock file withouth applying it, or call\n\nYou need a comma after \"unlock_ref()\".\n\ns/withouth/without/\n\n> commit_ref_lock() which will rename the lock file onto the ref file.\n\n\nThe commit message would be easier to read with better formatting; maybe\n\n---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---\nrefs.c: split writing and commiting a ref into two separate functions\n\n* Change the function write_ref_sha1() to just write the ref but not\n  commit the ref or the lockfile.\n\n* Add a new function commit_ref_lock() that will commit the change done by\n  a previous write_ref_sha1().\n\n* Update all callers of write_ref_sha1() to call commit_ref_lock().\n\nThe new pattern for updating a ref is now :\n\n    lock = lock_ref_sha1_basic() (or variant of)\n    write_ref_sha1(lock)\n    unlock_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. At that stage we can then either call unlock_ref(),\nwhich will abort the update and delete the lock file without applying it,\nor call commit_ref_lock() which will rename the lock file onto the ref\nfile.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---\n\n\n> Signed-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          | 18 ++++++++++++++++--\n>  refs.c                 | 41 +++++++++++++++++++++++++++++++----------\n>  refs.h                 |  4 ++++\n>  sequencer.c            |  4 ++++\n>  walker.c               |  4 ++++\n>  11 files changed, 92 insertions(+), 17 deletions(-)\n> \n> diff --git a/branch.c b/branch.c\n> index 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);\n\nThere are a lot of changes like this with similar duplicated error\nhandling.  Why not define a helper function like\nwrite_ref_sha1_and_commit() that does what the old write_ref_sha1() used\nto do (for the callers who don't care about updating multiple references\nat once)?\n\nIn fact, I would recommend renaming the function in a preparatory\ncommit, to reduce the amount of code churn in the second commit where\nyou extract the two new separate functions.\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 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\"));\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 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;\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 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>  }\n> diff --git a/builtin/replace.c b/builtin/replace.c\n> index 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>  }\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index 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>  \n> diff --git a/fast-import.c b/fast-import.c\n> index fb4738d..f732bfb 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>  \n> @@ -1732,8 +1737,17 @@ static void dump_tags(void)\n>  \tfor (t = first_tag; t; t = t->next_tag) {\n>  \t\tsprintf(ref_name, \"tags/%s\", t->name);\n>  \t\tlock = lock_ref_sha1(ref_name, NULL);\n> -\t\tif (!lock || write_ref_sha1(lock, t->sha1, msg) < 0)\n> +\t\tif (!lock) {\n> +\t\t\tfailure |= error(\"Unable to lock %s\", ref_name);\n> +\t\t\tcontinue;\n> +\t\t}\n> +\t\tif (write_ref_sha1(lock, t->sha1, msg) < 0) {\n>  \t\t\tfailure |= error(\"Unable to update %s\", ref_name);\n> +\t\t\tunlock_ref(lock);\n> +\t\t\tcontinue;\n> +\t\t}\n> +\t\tif (commit_ref_lock(lock) < 0)\n> +\t\t\tfailure |= error(\"Unable to commit %s\", ref_name);\n>  \t}\n>  }\n>  \n> diff --git a/refs.c b/refs.c\n> index 728a761..646afd7 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> +\tif (!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> @@ -3375,10 +3385,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> @@ -3453,7 +3470,11 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n>  \t\t\t\t\t       update->refname,\n>  \t\t\t\t\t       update->new_sha1,\n>  \t\t\t\t\t       update->lock, onerr);\n> -\t\t\tupdate->lock = NULL; /* freed by update_ref_write */\n> +\t\t\tif (ret)\n> +\t\t\t\tunlock_ref(update->lock);\n> +\t\t\telse\n> +\t\t\t\tcommit_ref_lock(update->lock);\n> +\t\t\tupdate->lock = NULL;\n>  \t\t\tif (ret)\n>  \t\t\t\tgoto cleanup;\n>  \t\t}\n> @@ -3464,7 +3485,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n>  \t\tstruct ref_update *update = updates[i];\n>  \n>  \t\tif (update->lock) {\n> -\t\t\tdelnames[delnum++] = update->lock->ref_name;\n> +\t\t\tdelnames[delnum++] = update->refname;\n\nIsn't this hunk orthogonal to the main point of this commit?  If so,\nplease split it into a separate commit.\n\n>  \t\t\tret |= delete_ref_loose(update->lock, update->type);\n>  \t\t}\n>  \t}\n> diff --git a/refs.h b/refs.h\n> index 0f08def..f14a417 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>  struct ref_transaction;\n> @@ -153,6 +154,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 *refname, char *logfile, int bufsize);\n>  \n> diff --git a/sequencer.c b/sequencer.c\n> index 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\n\"|=\" could be changed to \"=\" here and in the next hunk.\n\n>  \tstrbuf_release(&sb);\n>  \treturn ret;\n>  }\n> diff --git a/walker.c b/walker.c\n> index 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> \n\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238889","messageId":"CAL=YDWmm1pDtNuibs5CrPTDkaxT9PUvZscXFicoNsNpXVXJv1A@mail.gmail.com","threadId":"36411","inReplyTo":"534CD376.7080108@alum.mit.edu","subject":"Re: [PATCH v4 0/3] Make update refs more atomic","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-15T16:33:59Z","receivedAt":"2014-04-15T16:33:59Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Mon, Apr 14, 2014 at 11:36 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> On 04/14/2014 08:29 PM, Ronnie Sahlberg wrote:\n>> refs.c:ref_transaction_commit() intermingles doing updates and checks with\n>> actually 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 partway through the list of refs to update we\n>> will end up with some changes applied to disk and others not.\n>>\n>> Without having transaction support from the filesystem, it is hard to\n>> make an update that involves multiple refs to guarantee atomicity, but we\n>> can do a somewhat better than we currently do.\n>\n> It took me a moment to understand what you were talking about here,\n> because the code for ref_transaction_commit() already seems\n> superficially to do reference modifications in phases.  The problem is\n> that write_ref_sha1() internally contains additional checks that can\n> fail in \"normal\" circumstances.  So the most important part of this\n> patch series is allowing those checks to be done before committing anything.\n\nYes.\nThis patch series is mainly focused on making ref_transaction_commit()\nmore atomic\nso that it does more checks for failures before it starts doing\nirreversible changes to the underlying files.\n\nThese patches change ref_transaction_commit() to do even more checks\nbefore it starts applying the\nactual changes to disk.\n\n\nThere are also changes to other users of write_ref_sha1() too but\nthose are mainly just to\nreflect that this function no longer actually does the changes to the\nunderlying files any more.\n\n\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  (rename() or unlink())\n>>\n>> When a transaction is commited we first do all the locking, preparations\n>> and most of the error checking before we actually start applying any changes\n>> to the filesystem store.\n>>\n>> This means that more of the error cases that will fail the commit\n>> will trigger before we start doing any changes to the actual files.\n>>\n>>\n>> This should make the changes of refs in refs_transaction_commit slightly\n>> more atomic.\n>> [...]\n>\n> Yes, this is a good and important goal.\n>\n> I wonder, however, whether your approach of changing callers from\n>\n>     lock = lock_ref_sha1_basic() (or varient of)\n>     write_ref_sha1(lock)\n>\n> to\n>\n>     lock = lock_ref_sha1_basic() (or varient of)\n>     write_ref_sha1(lock)\n>     unlock_ref(lock) | commit_ref_lock(lock)\n>\n> is not doing work that we will soon need to rework.  Would it be jumping\n> the gun to change the callers to\n>\n>     transaction = ref_transaction_begin();\n>     ref_transaction_{update,delete,etc}(transaction, ...);\n>     ref_transaction_{commit,rollback}(transaction, ...);\n>\n> instead?  Then we could bury the details of calling write_ref_sha1() and\n> commit_lock_ref() inside ref_transaction_commit() rather than having to\n> expose them in the public API.\n>\n\nI think you are right.\n\nLets put this patch series on the backburner for now and start by\nmaking all callers use transactions\nand remove write_ref_sha1() from the public API thar refs.c exports.\n\nOnce everything is switched over to transactions I can rework this\npatchseries for ref_transaction_commit()\nand resubmit to the mailing list.\n\n\nLets start preparing patches to change all external callers to use\ntransactions instead.\nI am happy to help preparing patches for this. How do we ensure that\nwe do not create duplicate work\nand work on the same functions?\n\n\nregards\nronnie sahlberg\n\n\n> I suspect that the answer is \"no, ref transactions are not yet powerful\n> enough to do everything that the callers need\".  But then I would\n> suggest that we *make* them powerful enough and *then* make the change\n> at the callers.\n>\n> I'm not saying that we shouldn't accept your change as a first step [1]\n> and do the next step later, but wanted to get your reaction about making\n> the first step a bit more ambitious.\n>\n> Michael\n>\n> [1] Though I still need to review your patch series in detail.\n>\n> --\n> Michael Haggerty\n> mhagger@alum.mit.edu\n> http://softwareswirl.blogspot.com/\n"},{"id":"238890","messageId":"CAL=YDWnWc3r8s2p_SCRDQ+UA9Y9DMRJzG+jcbku2kX0GevC4Dw@mail.gmail.com","threadId":"36411","inReplyTo":"xmqqeh0zoe19.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 0/3] Make update refs more atomic","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-15T16:41:31Z","receivedAt":"2014-04-15T16:41:31Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Mon, Apr 14, 2014 at 1:24 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Thanks; will queue.\n\nJunio,\nPlease defer queuing for now.\n\nI think we should convert more of the external callers to use\ntransactions first.\nOnce that is done and everything uses transactions I will re-send an\nupdated version of this patch series.\n\n\nregards\nronnie sahlberg\n"},{"id":"238894","messageId":"534D6A05.8040609@alum.mit.edu","threadId":"36411","inReplyTo":"1397500163-7617-3-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v4 2/3] refs.c: split delete_ref_loose() into a separate flag-for-deletion and commit phase","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-15T17:19:01Z","receivedAt":"2014-04-15T17:19:01Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/14/2014 08:29 PM, Ronnie Sahlberg wrote:\n> Change delete_ref_loose()) to just flag that a ref is to be deleted but do\n> not actually unlink the files.\n> Change commit_ref_lock() so that it will unlink refs that are flagged for\n> deletion.\n> Change all callers of delete_ref_loose() to explicitely call commit_ref_lock()\n\ns/explicitely/explicitly/\n\n> to commit the deletion.\n> \n> The new pattern for deleting loose refs thus become:\n> \n> lock = lock_ref_sha1_basic() (or varient of)\n\ns/varient/variant/\n\n> delete_ref_loose(lock)\n> unlock_ref(lock) | commit_ref_lock(lock)\n\nFormatting: sentences should be flowed together if they are within a\nparagraph, or separated with blank lines if they constitute separate\nparagraphs, or have \"bullet\" characters and be indented if they are a\nbullet list.\n\nCode should be indented to set it off from the prose.\n\n> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n> ---\n>  refs.c | 32 ++++++++++++++++++++------------\n>  refs.h |  2 ++\n>  2 files changed, 22 insertions(+), 12 deletions(-)\n> \n> diff --git a/refs.c b/refs.c\n> index 646afd7..a14addb 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\nBefore this patch, the sequence for deleting a reference was\n\n    acquire lock on loose ref file\n    delete loose ref file\n    acquire lock on packed-refs file\n    rewrite packed-refs file, omitting ref\n    activate packed-refs file and release its lock\n    release lock on loose ref file\n\nAnother process that tries to read the reference's value between steps 2\nand 4 sees some old value of the reference from the packed-refs file\nrather than seeing either its recent value or seeing it undefined.  The\nvalue that it sees can be arbitrarily old, and might even point at an\nobject that has long-since been garbage-collected.  If the packed-refs\nlock acquisition fails, then the old value can be left in the\npacked-refs file and becomes the value of the reference permanently.  So\nthis is not correct (it's a known problem).\n\nAfter the patch, it is\n\n    acquire lock on loose ref file\n    acquire lock on packed-refs file\n    rewrite packed-refs file, omitting ref\n    activate packed-refs file and release its lock\n    delete loose ref file          <-- now this happens later\n    release lock on loose ref file\n\nA pack-refs process that runs between steps 4 and 5 might be able to\nacquire the packed-refs file lock, see the doomed loose value of the\nreference, and pack it, with the end effect that the reference is not\ndeleted after all.  But this is less bad than what can happen now.  Can\nanybody think of any new races that the new sequence would open?\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238899","messageId":"534D9741.3010404@alum.mit.edu","threadId":"36411","inReplyTo":"CAL=YDWmm1pDtNuibs5CrPTDkaxT9PUvZscXFicoNsNpXVXJv1A@mail.gmail.com","subject":"Re: [PATCH v4 0/3] Make update refs more atomic","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-15T20:32:01Z","receivedAt":"2014-04-15T20:32:01Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/15/2014 06:33 PM, Ronnie Sahlberg wrote:\n> On Mon, Apr 14, 2014 at 11:36 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>> [...]\n>> I wonder, however, whether your approach of changing callers from\n>>\n>>     lock = lock_ref_sha1_basic() (or varient of)\n>>     write_ref_sha1(lock)\n>>\n>> to\n>>\n>>     lock = lock_ref_sha1_basic() (or varient of)\n>>     write_ref_sha1(lock)\n>>     unlock_ref(lock) | commit_ref_lock(lock)\n>>\n>> is not doing work that we will soon need to rework.  Would it be jumping\n>> the gun to change the callers to\n>>\n>>     transaction = ref_transaction_begin();\n>>     ref_transaction_{update,delete,etc}(transaction, ...);\n>>     ref_transaction_{commit,rollback}(transaction, ...);\n>>\n>> instead?  Then we could bury the details of calling write_ref_sha1() and\n>> commit_lock_ref() inside ref_transaction_commit() rather than having to\n>> expose them in the public API.\n> \n> I think you are right.\n> \n> Lets put this patch series on the backburner for now and start by\n> making all callers use transactions\n> and remove write_ref_sha1() from the public API thar refs.c exports.\n> \n> Once everything is switched over to transactions I can rework this\n> patchseries for ref_transaction_commit()\n> and resubmit to the mailing list.\n\nSounds good.  Rewriting callers to use transactions would be a great\nnext step.  Please especially keep track of what new features the\ntransactions API still needs.  More flexible error handling?  The\nability to have steps in the transaction that are \"best-effort\" (i.e.,\ndon't abort the transaction if they fail)?  Different reflog messages\nfor different updates within the same transaction rather than one reflog\nmessage for all updates?  Etc.\n\nAnd some callers who currently change multiple references one at a time\nmight be able to be rewritten to update the references in a single\ntransaction.\n\n> Lets start preparing patches to change all external callers to use\n> transactions instead.\n> I am happy to help preparing patches for this. How do we ensure that\n> we do not create duplicate work\n> and work on the same functions?\n\nI have a few loose ends to take care of on my lockfile patch series, and\nthere are a few things I would like to tidy up internal to the\ntransactions implementation, so I think if you are working on the caller\nside then we won't step on each other's toes too much in the near future.\n\nI suggest we use IRC (mhagger@freenode) or XMPP (mhagger@jabber.org) for\nsmall-scale coordination.  I also have a GitHub repo\n(http://github.com/mhagger/git) to which I often push intermediate\nresults; I will try to push to that more regularly (warning: I often\nrebase feature branches even after they are pushed to GitHub).  I think\nyou are in Pacific Time whereas I am in Berlin, so we will tend to work\nin serial rather than in parallel; that should help.  It would be a good\nhabit to shoot each short status emails at the end of each working day.\n\nOf course we should only use one-on-one communication for early work; as\nsoon as something is getting ripe we should make sure our technical\ndiscussions take place here on the mailing list.\n\nSound OK?\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238948","messageId":"CAL=YDW=g=jkm4yhBvnZXSvLLm-6ZGhJORKv_evg66v0U=E71FA@mail.gmail.com","threadId":"36411","inReplyTo":"534D9741.3010404@alum.mit.edu","subject":"Re: [PATCH v4 0/3] Make update refs more atomic","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-16T17:11:21Z","receivedAt":"2014-04-16T17:11:21Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Tue, Apr 15, 2014 at 1:32 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> On 04/15/2014 06:33 PM, Ronnie Sahlberg wrote:\n>> On Mon, Apr 14, 2014 at 11:36 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>>> [...]\n>>> I wonder, however, whether your approach of changing callers from\n>>>\n>>>     lock = lock_ref_sha1_basic() (or varient of)\n>>>     write_ref_sha1(lock)\n>>>\n>>> to\n>>>\n>>>     lock = lock_ref_sha1_basic() (or varient of)\n>>>     write_ref_sha1(lock)\n>>>     unlock_ref(lock) | commit_ref_lock(lock)\n>>>\n>>> is not doing work that we will soon need to rework.  Would it be jumping\n>>> the gun to change the callers to\n>>>\n>>>     transaction = ref_transaction_begin();\n>>>     ref_transaction_{update,delete,etc}(transaction, ...);\n>>>     ref_transaction_{commit,rollback}(transaction, ...);\n>>>\n>>> instead?  Then we could bury the details of calling write_ref_sha1() and\n>>> commit_lock_ref() inside ref_transaction_commit() rather than having to\n>>> expose them in the public API.\n>>\n>> I think you are right.\n>>\n>> Lets put this patch series on the backburner for now and start by\n>> making all callers use transactions\n>> and remove write_ref_sha1() from the public API thar refs.c exports.\n>>\n>> Once everything is switched over to transactions I can rework this\n>> patchseries for ref_transaction_commit()\n>> and resubmit to the mailing list.\n>\n> Sounds good.  Rewriting callers to use transactions would be a great\n> next step.  Please especially keep track of what new features the\n> transactions API still needs.  More flexible error handling?  The\n> ability to have steps in the transaction that are \"best-effort\" (i.e.,\n> don't abort the transaction if they fail)?  Different reflog messages\n> for different updates within the same transaction rather than one reflog\n> message for all updates?  Etc.\n>\n> And some callers who currently change multiple references one at a time\n> might be able to be rewritten to update the references in a single\n> transaction.\n\nAs an experiment I rewrite most of the callers to use transactions yesterday.\nMost callers would translate just fine, but some callers, such as walker_fetch()\ndoes not yet fit well with the current transaction code.\n\nFor example that code does want to first take out locks on all refs\nbefore it does a\nlot of processing, with the locks held, before it writes and updates the refs.\n\n\nSome of my thoughts after going over the callers :\n\nCurrently any locking of refs in a transaction only happens during the commit\nphase. I think it would be useful to have a mechanism where you could\noptionally take out locks for the involved refs early during the transaction.\nSo that simple callers could continue using\nref_transaction_begin()\nref_transaction_create|update|delete()*\nref_transaction_commit()\n\nbut, if a caller such as walker_fetch() could opt to do\nref_transaction_begin()\nref_transaction_lock_ref()*\n...do stuff...\nref_transaction_create|update|delete()*\nref_transaction_commit()\n\nIn this second case ref_transaction_commit() would only take out any locks that\nare missing during the 'lock the refs\" loop.\n\nSuggestion 1: Add a ref_transaction_lock_ref() to allow locking a ref\nearly during\na transaction.\n\n\nA second idea is to change the signatures for\nref_transaction_create|update|delete()\nslightly and allow them to return errors early.\nWe can check for things like add_update() failing, check that the\nref-name looks sane,\ncheck some of the flags, like if has_old==true then old sha1 should\nnot be NULL or 0{40}, etc.\n\nAdditionally for robustness, if any of these functions detect an error\nwe can flag this in the\ntransaction structure and take action during ref_transaction_commit().\nI.e. if a ref_transaction_update had a hard failure, do not allow\nref_transaction_commit()\nto succeed.\n\nSuggestion 2: Change ref_transaction_create|delete|update() to return an int.\nAll callers that use these functions should check the function for error.\n\n\nSuggestion 3: remove the qsort and check for duplicates in\nref_transaction_commit()\nSince we are already taking out a lock for each ref we are updating\nduring the transaction\nany duplicate refs will fail the second attempt to lock the same ref which will\nimplicitly make sure that a transaction will not change the same ref twice.\n\nThere are only two caveats I see with this third suggestion.\n1, The second lock attempt will always cause a die() since we\neventually would end up\nin lock_ref_sha1_basic() and this function will always unconditionally\ndie() if the lock failed.\nBut your locking changes are addressing this, right?\n(all callers to lock_ref_sha1() or lock_any_ref_for_update() do check\nfor and handle if the lock\n failed, so that change to not die() should be safe)\n\n2, We would need to take care when a lock fails here to print the\nproper error message\nso that we still show \"Multiple updates for ref '%s' not allowed.\" if\nthe lock failed because\nthe transaction had already locked this file.\n\n\n\n\n>\n>> Lets start preparing patches to change all external callers to use\n>> transactions instead.\n>> I am happy to help preparing patches for this. How do we ensure that\n>> we do not create duplicate work\n>> and work on the same functions?\n>\n> I have a few loose ends to take care of on my lockfile patch series, and\n> there are a few things I would like to tidy up internal to the\n> transactions implementation, so I think if you are working on the caller\n> side then we won't step on each other's toes too much in the near future.\n>\n> I suggest we use IRC (mhagger@freenode) or XMPP (mhagger@jabber.org) for\n> small-scale coordination.  I also have a GitHub repo\n> (http://github.com/mhagger/git) to which I often push intermediate\n> results; I will try to push to that more regularly (warning: I often\n> rebase feature branches even after they are pushed to GitHub).  I think\n> you are in Pacific Time whereas I am in Berlin, so we will tend to work\n> in serial rather than in parallel; that should help.  It would be a good\n> habit to shoot each short status emails at the end of each working day.\n>\n> Of course we should only use one-on-one communication for early work; as\n> soon as something is getting ripe we should make sure our technical\n> discussions take place here on the mailing list.\n>\n> Sound OK?\n> Michael\n>\n> --\n> Michael Haggerty\n> mhagger@alum.mit.edu\n> http://softwareswirl.blogspot.com/\n"},{"id":"238978","messageId":"xmqq1twxgjge.fsf@gitster.dls.corp.google.com","threadId":"36411","inReplyTo":"CAL=YDW=g=jkm4yhBvnZXSvLLm-6ZGhJORKv_evg66v0U=E71FA@mail.gmail.com","subject":"Re: [PATCH v4 0/3] Make update refs more atomic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-16T19:31:13Z","receivedAt":"2014-04-16T19:31:13Z","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> Currently any locking of refs in a transaction only happens during the commit\n> phase. I think it would be useful to have a mechanism where you could\n> optionally take out locks for the involved refs early during the transaction.\n> So that simple callers could continue using\n> ref_transaction_begin()\n> ref_transaction_create|update|delete()*\n> ref_transaction_commit()\n>\n> but, if a caller such as walker_fetch() could opt to do\n> ref_transaction_begin()\n> ref_transaction_lock_ref()*\n> ...do stuff...\n> ref_transaction_create|update|delete()*\n> ref_transaction_commit()\n>\n> In this second case ref_transaction_commit() would only take out any locks that\n> are missing during the 'lock the refs\" loop.\n>\n> Suggestion 1: Add a ref_transaction_lock_ref() to allow locking a ref\n> early during\n> a transaction.\n\nHmph.\n\nI am not sure if that is the right way to go, or instead change all\ncreate/update/delete to take locks without adding a new primitive.\n\n> A second idea is to change the signatures for\n> ref_transaction_create|update|delete()\n> slightly and allow them to return errors early.\n> We can check for things like add_update() failing, check that the\n> ref-name looks sane,\n> check some of the flags, like if has_old==true then old sha1 should\n> not be NULL or 0{40}, etc.\n>\n> Additionally for robustness, if any of these functions detect an error\n> we can flag this in the\n> transaction structure and take action during ref_transaction_commit().\n> I.e. if a ref_transaction_update had a hard failure, do not allow\n> ref_transaction_commit()\n> to succeed.\n>\n> Suggestion 2: Change ref_transaction_create|delete|update() to return an int.\n> All callers that use these functions should check the function for error.\n\nI think that is a very sensible thing to do.\n\nThe details of determining \"this cannot possibly succeed\" may change\n(for example, if we have them take locks at the point of\ncreate/delete/update, a failure to lock may count as an early\nerror).\n\nIs there any reason why this should be conditional (i.e. you said\n\"allow them to\", implying that the early failure is optional)?\n\n> Suggestion 3: remove the qsort and check for duplicates in\n> ref_transaction_commit()\n> Since we are already taking out a lock for each ref we are updating\n> during the transaction\n> any duplicate refs will fail the second attempt to lock the same ref which will\n> implicitly make sure that a transaction will not change the same ref twice.\n\nI do not know if I care about the implementation detail of \"do we\nhave a unique set of update requests?\".  While I do not see a strong\nneed for one transaction to touch the same ref twice (e.g. create to\npoint at commit A and update it to point at commit B), I do not see\nwhy we should forbid such a use in the future.\n\nJust some of my knee-jerk reactions.\n"},{"id":"238982","messageId":"CAL=YDWnHtPedxYmpycgSybZA=CmdD55XQAFdA-Bs_42bk2Z0Tg@mail.gmail.com","threadId":"36411","inReplyTo":"xmqq1twxgjge.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 0/3] Make update refs more atomic","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-16T21:31:17Z","receivedAt":"2014-04-16T21:31:17Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Wed, Apr 16, 2014 at 12:31 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Ronnie Sahlberg <sahlberg@google.com> writes:\n>\n>> Currently any locking of refs in a transaction only happens during the commit\n>> phase. I think it would be useful to have a mechanism where you could\n>> optionally take out locks for the involved refs early during the transaction.\n>> So that simple callers could continue using\n>> ref_transaction_begin()\n>> ref_transaction_create|update|delete()*\n>> ref_transaction_commit()\n>>\n>> but, if a caller such as walker_fetch() could opt to do\n>> ref_transaction_begin()\n>> ref_transaction_lock_ref()*\n>> ...do stuff...\n>> ref_transaction_create|update|delete()*\n>> ref_transaction_commit()\n>>\n>> In this second case ref_transaction_commit() would only take out any locks that\n>> are missing during the 'lock the refs\" loop.\n>>\n>> Suggestion 1: Add a ref_transaction_lock_ref() to allow locking a ref\n>> early during\n>> a transaction.\n>\n> Hmph.\n>\n> I am not sure if that is the right way to go, or instead change all\n> create/update/delete to take locks without adding a new primitive.\n\nack.\n\n>\n>> A second idea is to change the signatures for\n>> ref_transaction_create|update|delete()\n>> slightly and allow them to return errors early.\n>> We can check for things like add_update() failing, check that the\n>> ref-name looks sane,\n>> check some of the flags, like if has_old==true then old sha1 should\n>> not be NULL or 0{40}, etc.\n>>\n>> Additionally for robustness, if any of these functions detect an error\n>> we can flag this in the\n>> transaction structure and take action during ref_transaction_commit().\n>> I.e. if a ref_transaction_update had a hard failure, do not allow\n>> ref_transaction_commit()\n>> to succeed.\n>>\n>> Suggestion 2: Change ref_transaction_create|delete|update() to return an int.\n>> All callers that use these functions should check the function for error.\n>\n> I think that is a very sensible thing to do.\n>\n> The details of determining \"this cannot possibly succeed\" may change\n> (for example, if we have them take locks at the point of\n> create/delete/update, a failure to lock may count as an early\n> error).\n>\n> Is there any reason why this should be conditional (i.e. you said\n> \"allow them to\", implying that the early failure is optional)?\n\nIt was poor wording on my side. Checking for the ref_transaction_*()\nreturn for error should be mandatory (modulo bugs).\n\nBut a caller could be buggy and fail to check properly.\nIt would be very cheap to detect this condition in\nref_transaction_commit() which could then do\n\n  die(\"transaction commit called for errored transaction\");\n\nwhich would make it easy to spot this kind of bugs.\n\n\n>\n>> Suggestion 3: remove the qsort and check for duplicates in\n>> ref_transaction_commit()\n>> Since we are already taking out a lock for each ref we are updating\n>> during the transaction\n>> any duplicate refs will fail the second attempt to lock the same ref which will\n>> implicitly make sure that a transaction will not change the same ref twice.\n>\n> I do not know if I care about the implementation detail of \"do we\n> have a unique set of update requests?\".  While I do not see a strong\n> need for one transaction to touch the same ref twice (e.g. create to\n> point at commit A and update it to point at commit B), I do not see\n> why we should forbid such a use in the future.\n>\n\nack.\n"},{"id":"238983","messageId":"xmqqtx9teytq.fsf@gitster.dls.corp.google.com","threadId":"36411","inReplyTo":"CAL=YDWnHtPedxYmpycgSybZA=CmdD55XQAFdA-Bs_42bk2Z0Tg@mail.gmail.com","subject":"Re: [PATCH v4 0/3] Make update refs more atomic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-16T21:42:09Z","receivedAt":"2014-04-16T21:42:09Z","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>> I am not sure if that is the right way to go, or instead change all\n>> create/update/delete to take locks without adding a new primitive.\n>\n> ack.\n\nHmph.  When I say \"I am not sure\", \"I dunno\", etc., I do mean it.\n\nDid you mean by \"Ack\" \"I do not know, either\", or \"I think it is\nbetter to take locks early everywhere\"?\n"},{"id":"238984","messageId":"534EFB4D.2070500@alum.mit.edu","threadId":"36411","inReplyTo":"xmqq1twxgjge.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 0/3] Make update refs more atomic","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-16T21:51:09Z","receivedAt":"2014-04-16T21:51:09Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/16/2014 09:31 PM, Junio C Hamano wrote:\n> Ronnie Sahlberg <sahlberg@google.com> writes:\n> \n>> Currently any locking of refs in a transaction only happens during the commit\n>> phase. I think it would be useful to have a mechanism where you could\n>> optionally take out locks for the involved refs early during the transaction.\n>> So that simple callers could continue using\n>> ref_transaction_begin()\n>> ref_transaction_create|update|delete()*\n>> ref_transaction_commit()\n>>\n>> but, if a caller such as walker_fetch() could opt to do\n>> ref_transaction_begin()\n>> ref_transaction_lock_ref()*\n>> ...do stuff...\n>> ref_transaction_create|update|delete()*\n>> ref_transaction_commit()\n>>\n>> In this second case ref_transaction_commit() would only take out any locks that\n>> are missing during the 'lock the refs\" loop.\n>>\n>> Suggestion 1: Add a ref_transaction_lock_ref() to allow locking a ref\n>> early during\n>> a transaction.\n> \n> Hmph.\n> \n> I am not sure if that is the right way to go, or instead change all\n> create/update/delete to take locks without adding a new primitive.\n\nJunio's suggestion seems like a good idea to me.  Obviously, as soon as\nwe take out a lock we could also do any applicable old_sha1 check and\npossibly fail fast.\n\nDoes a \"verify\" operation require holding a lock?  If not, when is the\nverification done--early, or during the commit, or both?  (I realize\nthat we don't yet *have* a verify operation at the API level, but we\nshould!)\n\nWe also need to think about for what period of time we have to hold the\npacked-refs lock.\n\nFinally, we shouldn't forget that currently the reflog files are locked\nby holding the lock on the corresponding loose reference file.  Do we\nneed to integrate these files into our system any more than they\ncurrently are?\n\n[By the way, I noticed the other day that the command\n\n    git reflog expire --stale-fix --expire-unreachable=now --all\n\ncan hold loose reference locks for a *long* time (like 10s of minutes),\nespecially in weird cases like the repository having 9000 packfiles for\nsome reason or another :-)  The command execution time grows strongly\nwith the length of the reference's log, or maybe even the square of the\nlength assuming the history is roughly linear and reachability is\ncomputed separately for each SHA-1.  This is just an empirical\nobservation so far; I haven't looked into the code yet.]\n\n>> A second idea is to change the signatures for\n>> ref_transaction_create|update|delete()\n>> slightly and allow them to return errors early.\n>> We can check for things like add_update() failing, check that the\n>> ref-name looks sane,\n>> check some of the flags, like if has_old==true then old sha1 should\n>> not be NULL or 0{40}, etc.\n>>\n>> Additionally for robustness, if any of these functions detect an error\n>> we can flag this in the\n>> transaction structure and take action during ref_transaction_commit().\n>> I.e. if a ref_transaction_update had a hard failure, do not allow\n>> ref_transaction_commit()\n>> to succeed.\n>>\n>> Suggestion 2: Change ref_transaction_create|delete|update() to return an int.\n>> All callers that use these functions should check the function for error.\n> \n> I think that is a very sensible thing to do.\n> \n> The details of determining \"this cannot possibly succeed\" may change\n> (for example, if we have them take locks at the point of\n> create/delete/update, a failure to lock may count as an early\n> error).\n> \n> Is there any reason why this should be conditional (i.e. you said\n> \"allow them to\", implying that the early failure is optional)?\n\nAlso a good suggestion.  We should make it clear in the documentation\nthat the create/delete/update functions are not *obliged* to return an\nerror (for example that the current value of the reference does not\nagree with old_sha1) because future alternate ref backends might\npossibly not be able to make such checks until the commit phase.  For\nexample, checking old_sha1 might involve a round-trip to a database or\nremote repository, in which case it might be preferable to check all\nvalues in a single round-trip upon commit.\n\nSo, callers might be informed early of problems, or they might only\nlearn about problems when they try to commit the transaction.  They have\nto be able to handle either type of error reporting.\n\nSo then the question arises (and maybe this is what Ronnie was getting\nat by suggesting that the checks might be conditional): are callers\n*obliged* to check the return values from create/delete/update, or are\nthey allowed to just keep throwing everything into the transaction,\nignoring errors, and only check the result of ref_transaction_commit()?\n\nI don't feel strongly one way or the other about this question.  It\nmight be nice to be able to write callers sloppily, but it would cost a\nbit more code in the reference backends.  Though maybe it wouldn't even\nbe much extra code, given that we would probably want to put consistency\nchecks in there anyway.\n\n>> Suggestion 3: remove the qsort and check for duplicates in\n>> ref_transaction_commit()\n>> Since we are already taking out a lock for each ref we are updating\n>> during the transaction\n>> any duplicate refs will fail the second attempt to lock the same ref which will\n>> implicitly make sure that a transaction will not change the same ref twice.\n> \n> I do not know if I care about the implementation detail of \"do we\n> have a unique set of update requests?\".  While I do not see a strong\n> need for one transaction to touch the same ref twice (e.g. create to\n> point at commit A and update it to point at commit B), I do not see\n> why we should forbid such a use in the future.\n\nI agree.  For example, we might very well want to allow multiple updates\nto a single reference, as long as they are mutually consistent, to be\ncoalesced into a single update.  I also expect that the error message in\nthe current code is more illuminating than the generic error message\nthat would result from the inability to lock a reference (because it is\nalready locked by an earlier update in the same transaction!)  Let's\nleave this aspect the way it is now and revisit the topic later when we\nhave learned more.\n\nI see, though, that this will be a little tricky to implement early\nlocking.  Currently we sort all of the updates and look for duplicates\n*before* we acquire any locks.  But if we want to acquire locks\nimmediately, then (since the list isn't sorted yet) it will be expensive\nto look for duplicates before attempting to lock.  I can see two\npossibilities:\n\n1. Use a different data structure, like a hash map, for storing updates,\nso that we have a cheap way to lookup whether there is already an update\ninvolving a reference.\n\n2. Try to lock the reference, and if the lock fails *then* look back\nthrough the list to see if we're the ones holding the lock already.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"}]}