{"thread":{"id":"36433","subject":"[PATCH 00/11] Use ref transactions from most callers","startedAt":"2014-04-17T19:46:16Z","lastAt":"2014-04-25T21:32:10Z","messageCount":24,"participants":["Ronnie Sahlberg","Michael Haggerty","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":11},"messages":[{"id":"239035","messageId":"1397763987-4453-1-git-send-email-sahlberg@google.com","threadId":"36433","inReplyTo":null,"subject":"[PATCH 00/11] Use ref transactions from most callers","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-17T19:46:16Z","receivedAt":"2014-04-17T19:46:16Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"This patch series changes most of the places where the ref functions for\nlocking and writing refs to instead use the new ref transaction API. There\nare still three more places where write_ref_sha1() is called from outside\nof refs.c but those all will require more complex work and review so those\nchanges belong in a different patch series.\n\n\nRonnie Sahlberg (11):\n  refs.c: constify the sha arguments for\n    ref_transaction_create|delete|update\n  refs.c: change ref_transaction_update() to do error checking and\n    return status\n  refs.c: change ref_transaction_create to do error checking and return\n    status\n  refs.c: ref_transaction_delete to check for error and return status\n  tag.c: use ref transactions when doing updates\n  replace.c: use the ref transaction functions for updates\n  commit.c: use ref transactions for updates\n  sequencer.c: use ref transactions for all ref updates\n  fast-import.c: change update_branch to use ref transactions\n  branch.c: use ref transaction for all ref updates\n  walker.c: use ref transaction for ref updates\n\n branch.c             | 16 ++++++++++------\n builtin/commit.c     | 22 ++++++++++++----------\n builtin/replace.c    | 15 +++++++++------\n builtin/tag.c        | 15 +++++++++------\n builtin/update-ref.c | 20 +++++++++++++-------\n fast-import.c        | 23 +++++++++++++++--------\n refs.c               | 44 ++++++++++++++++++++++++++++++--------------\n refs.h               | 25 +++++++++++++------------\n sequencer.c          | 17 +++++++++++++----\n walker.c             | 45 ++++++++++++++++++++-------------------------\n 10 files changed, 144 insertions(+), 98 deletions(-)\n\n-- \n1.9.1.513.gd486896\n"},{"id":"239036","messageId":"1397763987-4453-2-git-send-email-sahlberg@google.com","threadId":"36433","inReplyTo":"1397763987-4453-1-git-send-email-sahlberg@google.com","subject":"[PATCH 01/11] refs.c: constify the sha arguments for ref_transaction_create|delete|update","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-17T19:46:17Z","receivedAt":"2014-04-17T19:46:17Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"ref_transaction_create|delete|update has no need to modify the sha1\narguments passed to it so it should use const unsigned char* instead\nof unsigned char*.\n\nSome functions, such as fast_forward_to(), already have its old/new\nsha1 arguments as consts. This function will at some point need to\nuse ref_transaction_update() in which case this change is required.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n refs.c | 7 ++++---\n refs.h | 7 ++++---\n 2 files changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 728a761..138ab70 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3329,7 +3329,8 @@ static struct ref_update *add_update(struct ref_transaction *transaction,\n \n void ref_transaction_update(struct ref_transaction *transaction,\n \t\t\t    const char *refname,\n-\t\t\t    unsigned char *new_sha1, unsigned char *old_sha1,\n+\t\t\t    const unsigned char *new_sha1,\n+\t\t\t    const unsigned char *old_sha1,\n \t\t\t    int flags, int have_old)\n {\n \tstruct ref_update *update = add_update(transaction, refname);\n@@ -3343,7 +3344,7 @@ void ref_transaction_update(struct ref_transaction *transaction,\n \n void ref_transaction_create(struct ref_transaction *transaction,\n \t\t\t    const char *refname,\n-\t\t\t    unsigned char *new_sha1,\n+\t\t\t    const unsigned char *new_sha1,\n \t\t\t    int flags)\n {\n \tstruct ref_update *update = add_update(transaction, refname);\n@@ -3357,7 +3358,7 @@ void ref_transaction_create(struct ref_transaction *transaction,\n \n void ref_transaction_delete(struct ref_transaction *transaction,\n \t\t\t    const char *refname,\n-\t\t\t    unsigned char *old_sha1,\n+\t\t\t    const unsigned char *old_sha1,\n \t\t\t    int flags, int have_old)\n {\n \tstruct ref_update *update = add_update(transaction, refname);\ndiff --git a/refs.h b/refs.h\nindex 0f08def..892c5b6 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -239,7 +239,8 @@ void ref_transaction_rollback(struct ref_transaction *transaction);\n  */\n void ref_transaction_update(struct ref_transaction *transaction,\n \t\t\t    const char *refname,\n-\t\t\t    unsigned char *new_sha1, unsigned char *old_sha1,\n+\t\t\t    const unsigned char *new_sha1,\n+\t\t\t    const unsigned char *old_sha1,\n \t\t\t    int flags, int have_old);\n \n /*\n@@ -250,7 +251,7 @@ void ref_transaction_update(struct ref_transaction *transaction,\n  */\n void ref_transaction_create(struct ref_transaction *transaction,\n \t\t\t    const char *refname,\n-\t\t\t    unsigned char *new_sha1,\n+\t\t\t    const unsigned char *new_sha1,\n \t\t\t    int flags);\n \n /*\n@@ -260,7 +261,7 @@ void ref_transaction_create(struct ref_transaction *transaction,\n  */\n void ref_transaction_delete(struct ref_transaction *transaction,\n \t\t\t    const char *refname,\n-\t\t\t    unsigned char *old_sha1,\n+\t\t\t    const unsigned char *old_sha1,\n \t\t\t    int flags, int have_old);\n \n /*\n-- \n1.9.1.513.gd486896\n"},{"id":"239037","messageId":"1397763987-4453-3-git-send-email-sahlberg@google.com","threadId":"36433","inReplyTo":"1397763987-4453-1-git-send-email-sahlberg@google.com","subject":"[PATCH 02/11] refs.c: change ref_transaction_update() to do error checking and return status","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-17T19:46:18Z","receivedAt":"2014-04-17T19:46:18Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Update ref_transaction_update() do some basic error checking and return\ntrue on error. Update all callers to check ref_transaction_update() for error.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n builtin/update-ref.c | 11 +++++++----\n refs.c               |  9 +++++++--\n refs.h               | 10 +++++-----\n 3 files changed, 19 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 405267f..12bfacc 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -197,8 +197,10 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next)\n \tif (*next != line_termination)\n \t\tdie(\"update %s: extra input: %s\", refname, next);\n \n-\tref_transaction_update(transaction, refname, new_sha1, old_sha1,\n-\t\t\t       update_flags, have_old);\n+\tif (ref_transaction_update(transaction, refname, new_sha1, old_sha1,\n+\t\t\t\t   update_flags, have_old))\n+\t\tdie(\"failed transaction update for %s\", refname);\n+\n \n \tupdate_flags = 0;\n \tfree(refname);\n@@ -286,8 +288,9 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)\n \tif (*next != line_termination)\n \t\tdie(\"verify %s: extra input: %s\", refname, next);\n \n-\tref_transaction_update(transaction, refname, new_sha1, old_sha1,\n-\t\t\t       update_flags, have_old);\n+\tif (ref_transaction_update(transaction, refname, new_sha1, old_sha1,\n+\t\t\t       update_flags, have_old))\n+\t\tdie(\"failed transaction update for %s\", refname);\n \n \tupdate_flags = 0;\n \tfree(refname);\ndiff --git a/refs.c b/refs.c\nindex 138ab70..da1761d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3327,19 +3327,24 @@ static struct ref_update *add_update(struct ref_transaction *transaction,\n \treturn update;\n }\n \n-void ref_transaction_update(struct ref_transaction *transaction,\n+int ref_transaction_update(struct ref_transaction *transaction,\n \t\t\t    const char *refname,\n \t\t\t    const unsigned char *new_sha1,\n \t\t\t    const unsigned char *old_sha1,\n \t\t\t    int flags, int have_old)\n {\n-\tstruct ref_update *update = add_update(transaction, refname);\n+\tstruct ref_update *update;\n+\n+\tif (have_old && !old_sha1)\n+\t\treturn error(\"have_old is true but old_sha1 is NULL\");\n \n+\tupdate = add_update(transaction, refname);\n \thashcpy(update->new_sha1, new_sha1);\n \tupdate->flags = flags;\n \tupdate->have_old = have_old;\n \tif (have_old)\n \t\thashcpy(update->old_sha1, old_sha1);\n+\treturn 0;\n }\n \n void ref_transaction_create(struct ref_transaction *transaction,\ndiff --git a/refs.h b/refs.h\nindex 892c5b6..00e4f7b 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -237,11 +237,11 @@ void ref_transaction_rollback(struct ref_transaction *transaction);\n  * that the reference should have had before the update, or zeros if\n  * it must not have existed beforehand.\n  */\n-void ref_transaction_update(struct ref_transaction *transaction,\n-\t\t\t    const char *refname,\n-\t\t\t    const unsigned char *new_sha1,\n-\t\t\t    const unsigned char *old_sha1,\n-\t\t\t    int flags, int have_old);\n+int ref_transaction_update(struct ref_transaction *transaction,\n+\t\t\t   const char *refname,\n+\t\t\t   const unsigned char *new_sha1,\n+\t\t\t   const unsigned char *old_sha1,\n+\t\t\t   int flags, int have_old);\n \n /*\n  * Add a reference creation to transaction.  new_sha1 is the value\n-- \n1.9.1.513.gd486896\n"},{"id":"239038","messageId":"1397763987-4453-4-git-send-email-sahlberg@google.com","threadId":"36433","inReplyTo":"1397763987-4453-1-git-send-email-sahlberg@google.com","subject":"[PATCH 03/11] refs.c: change ref_transaction_create to do error checking and return status","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-17T19:46:19Z","receivedAt":"2014-04-17T19:46:19Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Do basic error checking in ref_transaction_create() and make it return\nstatus. Update all callers to check the result of ref_transaction_create()\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n builtin/update-ref.c |  4 +++-\n refs.c               | 17 +++++++++++------\n refs.h               |  8 ++++----\n 3 files changed, 18 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 12bfacc..1b8a087 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -226,7 +226,9 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next)\n \tif (*next != line_termination)\n \t\tdie(\"create %s: extra input: %s\", refname, next);\n \n-\tref_transaction_create(transaction, refname, new_sha1, update_flags);\n+\tif(ref_transaction_create(transaction, refname, new_sha1,\n+\t\t\t\t  update_flags))\n+\t\tdie(\"failed transaction create for %s\", refname);\n \n \tupdate_flags = 0;\n \tfree(refname);\ndiff --git a/refs.c b/refs.c\nindex da1761d..c46249f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3347,18 +3347,23 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \treturn 0;\n }\n \n-void ref_transaction_create(struct ref_transaction *transaction,\n-\t\t\t    const char *refname,\n-\t\t\t    const unsigned char *new_sha1,\n-\t\t\t    int flags)\n+int ref_transaction_create(struct ref_transaction *transaction,\n+\t\t\t   const char *refname,\n+\t\t\t   const unsigned char *new_sha1,\n+\t\t\t   int flags)\n {\n-\tstruct ref_update *update = add_update(transaction, refname);\n+\tstruct ref_update *update;\n+\n+\tif (!new_sha1 || is_null_sha1(new_sha1))\n+\t\treturn error(\"create ref with null new_sha1\");\n+\n+\tupdate = add_update(transaction, refname);\n \n-\tassert(!is_null_sha1(new_sha1));\n \thashcpy(update->new_sha1, new_sha1);\n \thashclr(update->old_sha1);\n \tupdate->flags = flags;\n \tupdate->have_old = 1;\n+\treturn 0;\n }\n \n void ref_transaction_delete(struct ref_transaction *transaction,\ndiff --git a/refs.h b/refs.h\nindex 00e4f7b..8799e15 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -249,10 +249,10 @@ int ref_transaction_update(struct ref_transaction *transaction,\n  * null SHA-1.  It is verified that the reference does not exist\n  * already.\n  */\n-void ref_transaction_create(struct ref_transaction *transaction,\n-\t\t\t    const char *refname,\n-\t\t\t    const unsigned char *new_sha1,\n-\t\t\t    int flags);\n+int ref_transaction_create(struct ref_transaction *transaction,\n+\t\t\t   const char *refname,\n+\t\t\t   const unsigned char *new_sha1,\n+\t\t\t   int flags);\n \n /*\n  * Add a reference deletion to transaction.  If have_old is true, then\n-- \n1.9.1.513.gd486896\n"},{"id":"239039","messageId":"1397763987-4453-5-git-send-email-sahlberg@google.com","threadId":"36433","inReplyTo":"1397763987-4453-1-git-send-email-sahlberg@google.com","subject":"[PATCH 04/11] refs.c: ref_transaction_delete to check for error and return status","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-17T19:46:20Z","receivedAt":"2014-04-17T19:46:20Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change ref_transaction_delete() to do basic error checking and return\nstatus. Update all callers to check the return for ref_transaction_delete()\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n builtin/update-ref.c |  5 +++--\n refs.c               | 15 ++++++++++-----\n refs.h               |  8 ++++----\n 3 files changed, 17 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 1b8a087..6ff8b86 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -258,8 +258,9 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next)\n \tif (*next != line_termination)\n \t\tdie(\"delete %s: extra input: %s\", refname, next);\n \n-\tref_transaction_delete(transaction, refname, old_sha1,\n-\t\t\t       update_flags, have_old);\n+\tif (ref_transaction_delete(transaction, refname, old_sha1,\n+\t\t\t\t   update_flags, have_old))\n+\t\tdie(\"failed transaction delete for %s\", refname);\n \n \tupdate_flags = 0;\n \tfree(refname);\ndiff --git a/refs.c b/refs.c\nindex c46249f..9cbcffa 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3366,19 +3366,24 @@ int ref_transaction_create(struct ref_transaction *transaction,\n \treturn 0;\n }\n \n-void ref_transaction_delete(struct ref_transaction *transaction,\n-\t\t\t    const char *refname,\n-\t\t\t    const unsigned char *old_sha1,\n-\t\t\t    int flags, int have_old)\n+int ref_transaction_delete(struct ref_transaction *transaction,\n+\t\t\t   const char *refname,\n+\t\t\t   const unsigned char *old_sha1,\n+\t\t\t   int flags, int have_old)\n {\n-\tstruct ref_update *update = add_update(transaction, refname);\n+\tstruct ref_update *update;\n \n+\tif (have_old && !old_sha1)\n+\t\treturn error(\"have_old is true but old_sha1 is NULL\");\n+\n+\tupdate = add_update(transaction, refname);\n \tupdate->flags = flags;\n \tupdate->have_old = have_old;\n \tif (have_old) {\n \t\tassert(!is_null_sha1(old_sha1));\n \t\thashcpy(update->old_sha1, old_sha1);\n \t}\n+\treturn 0;\n }\n \n int update_ref(const char *action, const char *refname,\ndiff --git a/refs.h b/refs.h\nindex 8799e15..7050da2 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -259,10 +259,10 @@ int ref_transaction_create(struct ref_transaction *transaction,\n  * old_sha1 holds the value that the reference should have had before\n  * the update (which must not be the null SHA-1).\n  */\n-void ref_transaction_delete(struct ref_transaction *transaction,\n-\t\t\t    const char *refname,\n-\t\t\t    const unsigned char *old_sha1,\n-\t\t\t    int flags, int have_old);\n+int ref_transaction_delete(struct ref_transaction *transaction,\n+\t\t\t   const char *refname,\n+\t\t\t   const unsigned char *old_sha1,\n+\t\t\t   int flags, int have_old);\n \n /*\n  * Commit all of the changes that have been queued in transaction, as\n-- \n1.9.1.513.gd486896\n"},{"id":"239040","messageId":"1397763987-4453-6-git-send-email-sahlberg@google.com","threadId":"36433","inReplyTo":"1397763987-4453-1-git-send-email-sahlberg@google.com","subject":"[PATCH 05/11] tag.c: use ref transactions when doing updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-17T19:46:21Z","receivedAt":"2014-04-17T19:46:21Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change tag.c to use ref transactions for all ref updates.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n builtin/tag.c | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 40356e3..dbeacc5 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -488,7 +488,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tstruct strbuf ref = STRBUF_INIT;\n \tunsigned char object[20], prev[20];\n \tconst char *object_ref, *tag;\n-\tstruct ref_lock *lock;\n \tstruct create_tag_options opt;\n \tchar *cleanup_arg = NULL;\n \tint annotate = 0, force = 0, lines = -1;\n@@ -496,6 +495,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tconst char *msgfile = NULL, *keyid = NULL;\n \tstruct msg_arg msg = { 0, STRBUF_INIT };\n \tstruct commit_list *with_commit = NULL;\n+\tstruct ref_transaction *transaction;\n \tstruct option options[] = {\n \t\tOPT_CMDMODE('l', \"list\", &cmdmode, N_(\"list tag names\"), 'l'),\n \t\t{ OPTION_INTEGER, 'n', NULL, &lines, N_(\"n\"),\n@@ -641,11 +641,14 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tif (annotate)\n \t\tcreate_tag(object, tag, &buf, &opt, prev, object);\n \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-\t\tdie(_(\"%s: cannot update the ref\"), ref.buf);\n+\ttransaction = ref_transaction_begin();\n+\tif (!transaction)\n+\t\tdie(_(\"%s: cannot start transaction\"), ref.buf);\n+\tif (ref_transaction_update(transaction, ref.buf, object, prev,\n+\t\t\t\t   0, !is_null_sha1(prev)))\n+\t\tdie(_(\"%s: cannot update transaction\"), ref.buf);\n+\tif (ref_transaction_commit(transaction, NULL, UPDATE_REFS_DIE_ON_ERR))\n+\t\tdie(_(\"%s: cannot commit transaction\"), 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-- \n1.9.1.513.gd486896\n"},{"id":"239046","messageId":"1397763987-4453-7-git-send-email-sahlberg@google.com","threadId":"36433","inReplyTo":"1397763987-4453-1-git-send-email-sahlberg@google.com","subject":"[PATCH 06/11] replace.c: use the ref transaction functions for updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-17T19:46:22Z","receivedAt":"2014-04-17T19:46:22Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Update replace.c to use ref transactions for updates.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n builtin/replace.c | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex b62420a..d8bd6ee 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -129,7 +129,7 @@ static int replace_object(const char *object_ref, const char *replace_ref,\n \tunsigned char object[20], prev[20], repl[20];\n \tenum object_type obj_type, repl_type;\n \tchar ref[PATH_MAX];\n-\tstruct ref_lock *lock;\n+\tstruct ref_transaction *transaction;\n \n \tif (get_sha1(object_ref, object))\n \t\tdie(\"Failed to resolve '%s' as a valid ref.\", object_ref);\n@@ -157,11 +157,14 @@ static int replace_object(const char *object_ref, const char *replace_ref,\n \telse if (!force)\n \t\tdie(\"replace ref '%s' already exists\", ref);\n \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-\t\tdie(\"%s: cannot update the ref\", ref);\n+\ttransaction = ref_transaction_begin();\n+\tif (!transaction)\n+\t\tdie(_(\"%s: cannot start transaction\"), ref);\n+\tif (ref_transaction_update(transaction, ref, repl, prev,\n+\t\t\t\t   0, !is_null_sha1(prev)))\n+\t\tdie(_(\"%s: cannot update transaction\"), ref);\n+\tif (ref_transaction_commit(transaction, NULL, UPDATE_REFS_DIE_ON_ERR))\n+\t\tdie(_(\"%s: cannot commit transaction\"), ref);\n \n \treturn 0;\n }\n-- \n1.9.1.513.gd486896\n"},{"id":"239041","messageId":"1397763987-4453-8-git-send-email-sahlberg@google.com","threadId":"36433","inReplyTo":"1397763987-4453-1-git-send-email-sahlberg@google.com","subject":"[PATCH 07/11] commit.c: use ref transactions for updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-17T19:46:23Z","receivedAt":"2014-04-17T19:46:23Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change commit.c to use ref transactions for all ref updates.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n builtin/commit.c | 22 ++++++++++++----------\n 1 file changed, 12 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex d9550c5..b8e4389 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1541,11 +1541,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tconst char *index_file, *reflog_msg;\n \tchar *nl;\n \tunsigned char sha1[20];\n-\tstruct ref_lock *ref_lock;\n \tstruct commit_list *parents = NULL, **pptr = &parents;\n \tstruct stat statbuf;\n \tstruct commit *current_head = NULL;\n \tstruct commit_extra_header *extra = NULL;\n+\tstruct ref_transaction *transaction;\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage_with_options(builtin_commit_usage, builtin_commit_options);\n@@ -1667,12 +1667,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tstrbuf_release(&author_ident);\n \tfree_commit_extra_headers(extra);\n \n-\tref_lock = lock_any_ref_for_update(\"HEAD\",\n-\t\t\t\t\t   !current_head\n-\t\t\t\t\t   ? NULL\n-\t\t\t\t\t   : current_head->object.sha1,\n-\t\t\t\t\t   0, NULL);\n-\n \tnl = strchr(sb.buf, '\\n');\n \tif (nl)\n \t\tstrbuf_setlen(&sb, nl + 1 - sb.buf);\n@@ -1681,14 +1675,22 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tstrbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));\n \tstrbuf_insert(&sb, strlen(reflog_msg), \": \", 2);\n \n-\tif (!ref_lock) {\n+\ttransaction = ref_transaction_begin();\n+\tif (!transaction) {\n \t\trollback_index_files();\n-\t\tdie(_(\"cannot lock HEAD ref\"));\n+\t\tdie(_(\"HEAD: cannot start transaction\"));\n \t}\n-\tif (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {\n+\tif (ref_transaction_update(transaction, \"HEAD\", sha1,\n+\t\t\t\t   current_head->object.sha1,\n+\t\t\t\t   0, !!current_head)) {\n \t\trollback_index_files();\n \t\tdie(_(\"cannot update HEAD ref\"));\n \t}\n+\tif (ref_transaction_commit(transaction, sb.buf,\n+\t\t\t\t   UPDATE_REFS_QUIET_ON_ERR)) {\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-- \n1.9.1.513.gd486896\n"},{"id":"239042","messageId":"1397763987-4453-9-git-send-email-sahlberg@google.com","threadId":"36433","inReplyTo":"1397763987-4453-1-git-send-email-sahlberg@google.com","subject":"[PATCH 08/11] sequencer.c: use ref transactions for all ref updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-17T19:46:24Z","receivedAt":"2014-04-17T19:46:24Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change to use ref transactions for all updates to refs.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n sequencer.c | 17 +++++++++++++----\n 1 file changed, 13 insertions(+), 4 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex bde5f04..fa14ac0 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -272,17 +272,26 @@ static int error_dirty_index(struct replay_opts *opts)\n static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n \t\t\tint unborn, struct replay_opts *opts)\n {\n-\tstruct ref_lock *ref_lock;\n+\tstruct ref_transaction *transaction;\n \tstruct strbuf sb = STRBUF_INIT;\n \tint ret;\n \n \tread_cache();\n \tif (checkout_fast_forward(from, to, 1))\n \t\texit(1); /* the callee should have complained already */\n-\tref_lock = lock_any_ref_for_update(\"HEAD\", unborn ? null_sha1 : from,\n-\t\t\t\t\t   0, NULL);\n+\n+\ttransaction = ref_transaction_begin();\n+\tif (!transaction)\n+\t\treturn 1;\n+\tif (ref_transaction_update(transaction, \"HEAD\", to, from,\n+\t\t\t\t   0, !unborn)) {\n+\t\tref_transaction_rollback(transaction);\n+\t\treturn 1;\n+\t}\n+\n \tstrbuf_addf(&sb, \"%s: fast-forward\", action_name(opts));\n-\tret = write_ref_sha1(ref_lock, to, sb.buf);\n+\tret = ref_transaction_commit(transaction, sb.buf,\n+\t\t\t\t     UPDATE_REFS_MSG_ON_ERR);\n \tstrbuf_release(&sb);\n \treturn ret;\n }\n-- \n1.9.1.513.gd486896\n"},{"id":"239043","messageId":"1397763987-4453-10-git-send-email-sahlberg@google.com","threadId":"36433","inReplyTo":"1397763987-4453-1-git-send-email-sahlberg@google.com","subject":"[PATCH 09/11] fast-import.c: change update_branch to use ref transactions","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-17T19:46:25Z","receivedAt":"2014-04-17T19:46:25Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change update_branch() to use ref transactions for updates.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n fast-import.c | 23 +++++++++++++++--------\n 1 file changed, 15 insertions(+), 8 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex fb4738d..466dfe3 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1678,36 +1678,43 @@ found_entry:\n static int update_branch(struct branch *b)\n {\n \tstatic const char *msg = \"fast-import\";\n-\tstruct ref_lock *lock;\n+\tstruct ref_transaction *transaction;\n \tunsigned char old_sha1[20];\n \n \tif (is_null_sha1(b->sha1))\n \t\treturn 0;\n \tif (read_ref(b->name, old_sha1))\n \t\thashclr(old_sha1);\n-\tlock = lock_any_ref_for_update(b->name, old_sha1, 0, NULL);\n-\tif (!lock)\n-\t\treturn error(\"Unable to lock %s\", b->name);\n+\ttransaction = ref_transaction_begin();\n+\tif (!transaction)\n+\t\treturn error(\"Unable to begin transaction for %s\", b->name);\n \tif (!force_update && !is_null_sha1(old_sha1)) {\n \t\tstruct commit *old_cmit, *new_cmit;\n \n \t\told_cmit = lookup_commit_reference_gently(old_sha1, 0);\n \t\tnew_cmit = lookup_commit_reference_gently(b->sha1, 0);\n \t\tif (!old_cmit || !new_cmit) {\n-\t\t\tunlock_ref(lock);\n+\t\t\tref_transaction_rollback(transaction);\n \t\t\treturn error(\"Branch %s is missing commits.\", b->name);\n \t\t}\n \n \t\tif (!in_merge_bases(old_cmit, new_cmit)) {\n-\t\t\tunlock_ref(lock);\n+\t\t\tref_transaction_rollback(transaction);\n \t\t\twarning(\"Not updating %s\"\n \t\t\t\t\" (new tip %s does not contain %s)\",\n \t\t\t\tb->name, sha1_to_hex(b->sha1), sha1_to_hex(old_sha1));\n \t\t\treturn -1;\n \t\t}\n \t}\n-\tif (write_ref_sha1(lock, b->sha1, msg) < 0)\n-\t\treturn error(\"Unable to update %s\", b->name);\n+\tif (ref_transaction_update(transaction, b->name, b->sha1, old_sha1,\n+\t\t\t\t   0, 1)) {\n+\t\tref_transaction_rollback(transaction);\n+\t\treturn error(\"Unable to update transaction for %s\", b->name);\n+\t}\n+\tif (ref_transaction_commit(transaction, msg,\n+\t\t\t\t   UPDATE_REFS_QUIET_ON_ERR))\n+\t\treturn error(\"Unable to commit transaction for %s\", b->name);\n+\n \treturn 0;\n }\n \n-- \n1.9.1.513.gd486896\n"},{"id":"239044","messageId":"1397763987-4453-11-git-send-email-sahlberg@google.com","threadId":"36433","inReplyTo":"1397763987-4453-1-git-send-email-sahlberg@google.com","subject":"[PATCH 10/11] branch.c: use ref transaction for all ref updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-17T19:46:26Z","receivedAt":"2014-04-17T19:46:26Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change branch.c to use ref transactions when doing updates.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n branch.c | 16 ++++++++++------\n 1 file changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 660097b..45c7766 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -226,7 +226,7 @@ void create_branch(const char *head,\n \t\t   int force, int reflog, int clobber_head,\n \t\t   int quiet, enum branch_track track)\n {\n-\tstruct ref_lock *lock = NULL;\n+\tstruct ref_transaction *transaction;\n \tstruct commit *commit;\n \tunsigned char sha1[20];\n \tchar *real_ref, msg[PATH_MAX + 20];\n@@ -286,9 +286,12 @@ void create_branch(const char *head,\n \thashcpy(sha1, commit->object.sha1);\n \n \tif (!dont_change_ref) {\n-\t\tlock = lock_any_ref_for_update(ref.buf, NULL, 0, NULL);\n-\t\tif (!lock)\n-\t\t\tdie_errno(_(\"Failed to lock ref for update\"));\n+\t\ttransaction = ref_transaction_begin();\n+\t\tif (!transaction)\n+\t\t\tdie_errno(_(\"Failed to begin transaction\"));\n+\t\tif (ref_transaction_update(transaction, ref.buf, sha1, NULL,\n+\t\t\t\t   0, 0))\n+\t\t\tdie_errno(_(\"Failed to update transaction\"));\n \t}\n \n \tif (reflog)\n@@ -305,8 +308,9 @@ void create_branch(const char *head,\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-\t\t\tdie_errno(_(\"Failed to write ref\"));\n+\t\tif (ref_transaction_commit(transaction, msg,\n+\t\t\t\t     UPDATE_REFS_DIE_ON_ERR))\n+\t\t\tdie_errno(_(\"Failed to commit transaction\"));\n \n \tstrbuf_release(&ref);\n \tfree(real_ref);\n-- \n1.9.1.513.gd486896\n"},{"id":"239045","messageId":"1397763987-4453-12-git-send-email-sahlberg@google.com","threadId":"36433","inReplyTo":"1397763987-4453-1-git-send-email-sahlberg@google.com","subject":"[PATCH 11/11] walker.c: use ref transaction for ref updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-17T19:46:27Z","receivedAt":"2014-04-17T19:46:27Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Switch to using ref transactions in walker_fetch(). As part of the refactoring\nto use ref transactions we also fix a potential memory leak where in the\noriginal code if write_ref_sha1() would fail we would end up returning from\nthe function without free()ing the msg string.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n walker.c | 45 ++++++++++++++++++++-------------------------\n 1 file changed, 20 insertions(+), 25 deletions(-)\n\ndiff --git a/walker.c b/walker.c\nindex 1dd86b8..fa02d09 100644\n--- a/walker.c\n+++ b/walker.c\n@@ -251,24 +251,16 @@ void walker_targets_free(int targets, char **target, const char **write_ref)\n int walker_fetch(struct walker *walker, int targets, char **target,\n \t\t const char **write_ref, const char *write_ref_log_details)\n {\n-\tstruct ref_lock **lock = xcalloc(targets, sizeof(struct ref_lock *));\n+\tstruct ref_transaction *transaction;\n \tunsigned char *sha1 = xmalloc(targets * 20);\n-\tchar *msg;\n-\tint ret;\n+\tchar *msg = NULL;\n \tint i;\n \n \tsave_commit_buffer = 0;\n \n-\tfor (i = 0; i < targets; i++) {\n-\t\tif (!write_ref || !write_ref[i])\n-\t\t\tcontinue;\n-\n-\t\tlock[i] = lock_ref_sha1(write_ref[i], NULL);\n-\t\tif (!lock[i]) {\n-\t\t\terror(\"Can't lock ref %s\", write_ref[i]);\n-\t\t\tgoto unlock_and_fail;\n-\t\t}\n-\t}\n+\ttransaction = ref_transaction_begin();\n+\tif (!transaction)\n+\t\treturn -1;\n \n \tif (!walker->get_recover)\n \t\tfor_each_ref(mark_complete, NULL);\n@@ -276,14 +268,14 @@ int walker_fetch(struct walker *walker, int targets, char **target,\n \tfor (i = 0; i < targets; i++) {\n \t\tif (interpret_target(walker, target[i], &sha1[20 * i])) {\n \t\t\terror(\"Could not interpret response from server '%s' as something to pull\", target[i]);\n-\t\t\tgoto unlock_and_fail;\n+\t\t\tgoto rollback_and_fail;\n \t\t}\n \t\tif (process(walker, lookup_unknown_object(&sha1[20 * i])))\n-\t\t\tgoto unlock_and_fail;\n+\t\t\tgoto rollback_and_fail;\n \t}\n \n \tif (loop(walker))\n-\t\tgoto unlock_and_fail;\n+\t\tgoto rollback_and_fail;\n \n \tif (write_ref_log_details) {\n \t\tmsg = xmalloc(strlen(write_ref_log_details) + 12);\n@@ -294,19 +286,22 @@ int walker_fetch(struct walker *walker, int targets, char **target,\n \tfor (i = 0; i < targets; i++) {\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\tlock[i] = NULL;\n-\t\tif (ret)\n-\t\t\tgoto unlock_and_fail;\n+\t\tif (ref_transaction_update(transaction, write_ref[i],\n+\t\t\t\t\t   &sha1[20 * i], NULL,\n+\t\t\t\t\t   0, 0))\n+\t\t\tgoto rollback_and_fail;\n \t}\n-\tfree(msg);\n \n+\tif (ref_transaction_commit(transaction, msg ? msg : \"fetch (unknown)\",\n+\t\t\t\t   UPDATE_REFS_QUIET_ON_ERR))\n+\t\tgoto rollback_and_fail;\n+\n+\tfree(msg);\n \treturn 0;\n \n-unlock_and_fail:\n-\tfor (i = 0; i < targets; i++)\n-\t\tif (lock[i])\n-\t\t\tunlock_ref(lock[i]);\n+rollback_and_fail:\n+\tfree(msg);\n+\tref_transaction_rollback(transaction);\n \n \treturn -1;\n }\n-- \n1.9.1.513.gd486896\n"},{"id":"239121","messageId":"5352C696.9010601@alum.mit.edu","threadId":"36433","inReplyTo":"1397763987-4453-3-git-send-email-sahlberg@google.com","subject":"Re: [PATCH 02/11] refs.c: change ref_transaction_update() to do error checking and return status","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-19T18:55:18Z","receivedAt":"2014-04-19T18:55:18Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote:\n> Update ref_transaction_update() do some basic error checking and return\n> true on error. Update all callers to check ref_transaction_update() for error.\n> \n> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n> ---\n>  builtin/update-ref.c | 11 +++++++----\n>  refs.c               |  9 +++++++--\n>  refs.h               | 10 +++++-----\n>  3 files changed, 19 insertions(+), 11 deletions(-)\n> \n> diff --git a/builtin/update-ref.c b/builtin/update-ref.c\n> index 405267f..12bfacc 100644\n> --- a/builtin/update-ref.c\n> +++ b/builtin/update-ref.c\n> @@ -197,8 +197,10 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next)\n>  \tif (*next != line_termination)\n>  \t\tdie(\"update %s: extra input: %s\", refname, next);\n>  \n> -\tref_transaction_update(transaction, refname, new_sha1, old_sha1,\n> -\t\t\t       update_flags, have_old);\n> +\tif (ref_transaction_update(transaction, refname, new_sha1, old_sha1,\n> +\t\t\t\t   update_flags, have_old))\n> +\t\tdie(\"failed transaction update for %s\", refname);\n> +\n>  \n>  \tupdate_flags = 0;\n>  \tfree(refname);\n> @@ -286,8 +288,9 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)\n>  \tif (*next != line_termination)\n>  \t\tdie(\"verify %s: extra input: %s\", refname, next);\n>  \n> -\tref_transaction_update(transaction, refname, new_sha1, old_sha1,\n> -\t\t\t       update_flags, have_old);\n> +\tif (ref_transaction_update(transaction, refname, new_sha1, old_sha1,\n> +\t\t\t       update_flags, have_old))\n> +\t\tdie(\"failed transaction update for %s\", refname);\n>  \n>  \tupdate_flags = 0;\n>  \tfree(refname);\n> diff --git a/refs.c b/refs.c\n> index 138ab70..da1761d 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -3327,19 +3327,24 @@ static struct ref_update *add_update(struct ref_transaction *transaction,\n>  \treturn update;\n>  }\n>  \n> -void ref_transaction_update(struct ref_transaction *transaction,\n> +int ref_transaction_update(struct ref_transaction *transaction,\n>  \t\t\t    const char *refname,\n>  \t\t\t    const unsigned char *new_sha1,\n>  \t\t\t    const unsigned char *old_sha1,\n>  \t\t\t    int flags, int have_old)\n>  {\n> -\tstruct ref_update *update = add_update(transaction, refname);\n> +\tstruct ref_update *update;\n> +\n> +\tif (have_old && !old_sha1)\n> +\t\treturn error(\"have_old is true but old_sha1 is NULL\");\n\nThe function documentation doesn't seem to allow old_sha1 == NULL.  That\nmeans that calling it that way is a bug, not an error.  Therefore, I\nthink this should result in\n\n    die(\"BUG: have_old is true but old_sha1 is NULL\");\n\n>  \n> +\tupdate = add_update(transaction, refname);\n>  \thashcpy(update->new_sha1, new_sha1);\n>  \tupdate->flags = flags;\n>  \tupdate->have_old = have_old;\n>  \tif (have_old)\n>  \t\thashcpy(update->old_sha1, old_sha1);\n> +\treturn 0;\n>  }\n>  \n>  void ref_transaction_create(struct ref_transaction *transaction,\n> diff --git a/refs.h b/refs.h\n> index 892c5b6..00e4f7b 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -237,11 +237,11 @@ void ref_transaction_rollback(struct ref_transaction *transaction);\n>   * that the reference should have had before the update, or zeros if\n>   * it must not have existed beforehand.\n>   */\n> -void ref_transaction_update(struct ref_transaction *transaction,\n> -\t\t\t    const char *refname,\n> -\t\t\t    const unsigned char *new_sha1,\n> -\t\t\t    const unsigned char *old_sha1,\n> -\t\t\t    int flags, int have_old);\n> +int ref_transaction_update(struct ref_transaction *transaction,\n> +\t\t\t   const char *refname,\n> +\t\t\t   const unsigned char *new_sha1,\n> +\t\t\t   const unsigned char *old_sha1,\n> +\t\t\t   int flags, int have_old);\n>  \n>  /*\n>   * Add a reference creation to transaction.  new_sha1 is the value\n> \n\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"239122","messageId":"5352C6CA.7080803@alum.mit.edu","threadId":"36433","inReplyTo":"1397763987-4453-2-git-send-email-sahlberg@google.com","subject":"Re: [PATCH 01/11] refs.c: constify the sha arguments for ref_transaction_create|delete|update","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-19T18:56:10Z","receivedAt":"2014-04-19T18:56:10Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote:\n> ref_transaction_create|delete|update has no need to modify the sha1\n> arguments passed to it so it should use const unsigned char* instead\n> of unsigned char*.\n> \n> Some functions, such as fast_forward_to(), already have its old/new\n> sha1 arguments as consts. This function will at some point need to\n> use ref_transaction_update() in which case this change is required.\n\nGood, thanks.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"239123","messageId":"5352C77D.3090106@alum.mit.edu","threadId":"36433","inReplyTo":"1397763987-4453-4-git-send-email-sahlberg@google.com","subject":"Re: [PATCH 03/11] refs.c: change ref_transaction_create to do error checking and return status","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-19T18:59:09Z","receivedAt":"2014-04-19T18:59:09Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote:\n> Do basic error checking in ref_transaction_create() and make it return\n> status. Update all callers to check the result of ref_transaction_create()\n> \n> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n> ---\n>  builtin/update-ref.c |  4 +++-\n>  refs.c               | 17 +++++++++++------\n>  refs.h               |  8 ++++----\n>  3 files changed, 18 insertions(+), 11 deletions(-)\n> \n> diff --git a/builtin/update-ref.c b/builtin/update-ref.c\n> index 12bfacc..1b8a087 100644\n> --- a/builtin/update-ref.c\n> +++ b/builtin/update-ref.c\n> @@ -226,7 +226,9 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next)\n>  \tif (*next != line_termination)\n>  \t\tdie(\"create %s: extra input: %s\", refname, next);\n>  \n> -\tref_transaction_create(transaction, refname, new_sha1, update_flags);\n> +\tif(ref_transaction_create(transaction, refname, new_sha1,\n> +\t\t\t\t  update_flags))\n\nSpace between \"if\" and opening parenthesis.\n\n> +\t\tdie(\"failed transaction create for %s\", refname);\n>  \n>  \tupdate_flags = 0;\n>  \tfree(refname);\n> diff --git a/refs.c b/refs.c\n> index da1761d..c46249f 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -3347,18 +3347,23 @@ int ref_transaction_update(struct ref_transaction *transaction,\n>  \treturn 0;\n>  }\n>  \n> -void ref_transaction_create(struct ref_transaction *transaction,\n> -\t\t\t    const char *refname,\n> -\t\t\t    const unsigned char *new_sha1,\n> -\t\t\t    int flags)\n> +int ref_transaction_create(struct ref_transaction *transaction,\n> +\t\t\t   const char *refname,\n> +\t\t\t   const unsigned char *new_sha1,\n> +\t\t\t   int flags)\n>  {\n> -\tstruct ref_update *update = add_update(transaction, refname);\n> +\tstruct ref_update *update;\n> +\n> +\tif (!new_sha1 || is_null_sha1(new_sha1))\n> +\t\treturn error(\"create ref with null new_sha1\");\n\nPlease die(\"BUG: ...\").\n\n> +\n> +\tupdate = add_update(transaction, refname);\n>  \n> -\tassert(!is_null_sha1(new_sha1));\n>  \thashcpy(update->new_sha1, new_sha1);\n>  \thashclr(update->old_sha1);\n>  \tupdate->flags = flags;\n>  \tupdate->have_old = 1;\n> +\treturn 0;\n>  }\n>  \n>  void ref_transaction_delete(struct ref_transaction *transaction,\n> diff --git a/refs.h b/refs.h\n> index 00e4f7b..8799e15 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -249,10 +249,10 @@ int ref_transaction_update(struct ref_transaction *transaction,\n>   * null SHA-1.  It is verified that the reference does not exist\n>   * already.\n>   */\n> -void ref_transaction_create(struct ref_transaction *transaction,\n> -\t\t\t    const char *refname,\n> -\t\t\t    const unsigned char *new_sha1,\n> -\t\t\t    int flags);\n> +int ref_transaction_create(struct ref_transaction *transaction,\n> +\t\t\t   const char *refname,\n> +\t\t\t   const unsigned char *new_sha1,\n> +\t\t\t   int flags);\n>  \n>  /*\n>   * Add a reference deletion to transaction.  If have_old is true, then\n> \n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"239124","messageId":"5352C7B9.80405@alum.mit.edu","threadId":"36433","inReplyTo":"1397763987-4453-5-git-send-email-sahlberg@google.com","subject":"Re: [PATCH 04/11] refs.c: ref_transaction_delete to check for error and return status","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-19T19:00:09Z","receivedAt":"2014-04-19T19:00:09Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote:\n> Change ref_transaction_delete() to do basic error checking and return\n> status. Update all callers to check the return for ref_transaction_delete()\n> \n> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n> ---\n>  builtin/update-ref.c |  5 +++--\n>  refs.c               | 15 ++++++++++-----\n>  refs.h               |  8 ++++----\n>  3 files changed, 17 insertions(+), 11 deletions(-)\n> \n> diff --git a/builtin/update-ref.c b/builtin/update-ref.c\n> index 1b8a087..6ff8b86 100644\n> --- a/builtin/update-ref.c\n> +++ b/builtin/update-ref.c\n> @@ -258,8 +258,9 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next)\n>  \tif (*next != line_termination)\n>  \t\tdie(\"delete %s: extra input: %s\", refname, next);\n>  \n> -\tref_transaction_delete(transaction, refname, old_sha1,\n> -\t\t\t       update_flags, have_old);\n> +\tif (ref_transaction_delete(transaction, refname, old_sha1,\n> +\t\t\t\t   update_flags, have_old))\n> +\t\tdie(\"failed transaction delete for %s\", refname);\n>  \n>  \tupdate_flags = 0;\n>  \tfree(refname);\n> diff --git a/refs.c b/refs.c\n> index c46249f..9cbcffa 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -3366,19 +3366,24 @@ int ref_transaction_create(struct ref_transaction *transaction,\n>  \treturn 0;\n>  }\n>  \n> -void ref_transaction_delete(struct ref_transaction *transaction,\n> -\t\t\t    const char *refname,\n> -\t\t\t    const unsigned char *old_sha1,\n> -\t\t\t    int flags, int have_old)\n> +int ref_transaction_delete(struct ref_transaction *transaction,\n> +\t\t\t   const char *refname,\n> +\t\t\t   const unsigned char *old_sha1,\n> +\t\t\t   int flags, int have_old)\n>  {\n> -\tstruct ref_update *update = add_update(transaction, refname);\n> +\tstruct ref_update *update;\n>  \n> +\tif (have_old && !old_sha1)\n> +\t\treturn error(\"have_old is true but old_sha1 is NULL\");\n\nDitto.\n\n> +\n> +\tupdate = add_update(transaction, refname);\n>  \tupdate->flags = flags;\n>  \tupdate->have_old = have_old;\n>  \tif (have_old) {\n>  \t\tassert(!is_null_sha1(old_sha1));\n>  \t\thashcpy(update->old_sha1, old_sha1);\n>  \t}\n> +\treturn 0;\n>  }\n>  \n>  int update_ref(const char *action, const char *refname,\n> diff --git a/refs.h b/refs.h\n> index 8799e15..7050da2 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -259,10 +259,10 @@ int ref_transaction_create(struct ref_transaction *transaction,\n>   * old_sha1 holds the value that the reference should have had before\n>   * the update (which must not be the null SHA-1).\n>   */\n> -void ref_transaction_delete(struct ref_transaction *transaction,\n> -\t\t\t    const char *refname,\n> -\t\t\t    const unsigned char *old_sha1,\n> -\t\t\t    int flags, int have_old);\n> +int ref_transaction_delete(struct ref_transaction *transaction,\n> +\t\t\t   const char *refname,\n> +\t\t\t   const unsigned char *old_sha1,\n> +\t\t\t   int flags, int have_old);\n>  \n>  /*\n>   * Commit all of the changes that have been queued in transaction, as\n> \n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"239126","messageId":"5352CAB2.2000909@alum.mit.edu","threadId":"36433","inReplyTo":"1397763987-4453-6-git-send-email-sahlberg@google.com","subject":"Re: [PATCH 05/11] tag.c: use ref transactions when doing updates","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-19T19:12:50Z","receivedAt":"2014-04-19T19:12:50Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote:\n> Change tag.c to use ref transactions for all ref updates.\n> \n> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n> ---\n>  builtin/tag.c | 15 +++++++++------\n>  1 file changed, 9 insertions(+), 6 deletions(-)\n> \n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index 40356e3..dbeacc5 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -488,7 +488,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>  \tstruct strbuf ref = STRBUF_INIT;\n>  \tunsigned char object[20], prev[20];\n>  \tconst char *object_ref, *tag;\n> -\tstruct ref_lock *lock;\n>  \tstruct create_tag_options opt;\n>  \tchar *cleanup_arg = NULL;\n>  \tint annotate = 0, force = 0, lines = -1;\n> @@ -496,6 +495,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>  \tconst char *msgfile = NULL, *keyid = NULL;\n>  \tstruct msg_arg msg = { 0, STRBUF_INIT };\n>  \tstruct commit_list *with_commit = NULL;\n> +\tstruct ref_transaction *transaction;\n>  \tstruct option options[] = {\n>  \t\tOPT_CMDMODE('l', \"list\", &cmdmode, N_(\"list tag names\"), 'l'),\n>  \t\t{ OPTION_INTEGER, 'n', NULL, &lines, N_(\"n\"),\n> @@ -641,11 +641,14 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>  \tif (annotate)\n>  \t\tcreate_tag(object, tag, &buf, &opt, prev, object);\n>  \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> -\t\tdie(_(\"%s: cannot update the ref\"), ref.buf);\n> +\ttransaction = ref_transaction_begin();\n> +\tif (!transaction)\n> +\t\tdie(_(\"%s: cannot start transaction\"), ref.buf);\n> +\tif (ref_transaction_update(transaction, ref.buf, object, prev,\n> +\t\t\t\t   0, !is_null_sha1(prev)))\n> +\t\tdie(_(\"%s: cannot update transaction\"), ref.buf);\n> +\tif (ref_transaction_commit(transaction, NULL, UPDATE_REFS_DIE_ON_ERR))\n> +\t\tdie(_(\"%s: cannot commit transaction\"), ref.buf);\n\nI wonder whether these error messages are meaningful to the user.  To\nthe user, it's all just a failure to create the tag, right?  Does it\nhelp to tell the user that the error was in the \"start transaction\",\n\"update transaction\", or \"commit transaction\" phase?\n\nIn other words, maybe it would be just as good to do\n\nif (!transaction ||\n    ref_transaction_update(transaction, ref.buf, object, prev,\n\t\t\t   0, !is_null_sha1(prev)) ||\n    ref_transaction_commit(transaction, NULL, UPDATE_REFS_DIE_ON_ERR))\n\tdie(_(\"%s: cannot update the ref\"), ref.buf);\n\nBut then I would also hope that somebody (probably\nref_transaction_commit() now, but later maybe ref_transaction_update())\ntells the user whether the problem was the inability to lock the\nreference, or the consistency check of old_sha1, or whatever.\n\nThe next question is whether the generic error messages that the refs\ncode generates is good enough, or whether the caller would rather learn\nabout the reason for an error and generate its own, more specific error\nmessage.  Please pay attention to this as you rewrite callers: by\nrelying more on centralized code, are we losing out unacceptably on\nerror message specificity?\n\nOh and by the way, given that you pass UPDATE_REFS_DIE_ON_ERR,\nref_transaction_commit() should never return a nonzero value, should it?\n\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> \n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"239127","messageId":"5352CB18.5000007@alum.mit.edu","threadId":"36433","inReplyTo":"1397763987-4453-7-git-send-email-sahlberg@google.com","subject":"Re: [PATCH 06/11] replace.c: use the ref transaction functions for updates","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-19T19:14:32Z","receivedAt":"2014-04-19T19:14:32Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote:\n> Update replace.c to use ref transactions for updates.\n> \n> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n> ---\n>  builtin/replace.c | 15 +++++++++------\n>  1 file changed, 9 insertions(+), 6 deletions(-)\n> \n> diff --git a/builtin/replace.c b/builtin/replace.c\n> index b62420a..d8bd6ee 100644\n> --- a/builtin/replace.c\n> +++ b/builtin/replace.c\n> @@ -129,7 +129,7 @@ static int replace_object(const char *object_ref, const char *replace_ref,\n>  \tunsigned char object[20], prev[20], repl[20];\n>  \tenum object_type obj_type, repl_type;\n>  \tchar ref[PATH_MAX];\n> -\tstruct ref_lock *lock;\n> +\tstruct ref_transaction *transaction;\n>  \n>  \tif (get_sha1(object_ref, object))\n>  \t\tdie(\"Failed to resolve '%s' as a valid ref.\", object_ref);\n> @@ -157,11 +157,14 @@ static int replace_object(const char *object_ref, const char *replace_ref,\n>  \telse if (!force)\n>  \t\tdie(\"replace ref '%s' already exists\", ref);\n>  \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> -\t\tdie(\"%s: cannot update the ref\", ref);\n> +\ttransaction = ref_transaction_begin();\n> +\tif (!transaction)\n> +\t\tdie(_(\"%s: cannot start transaction\"), ref);\n> +\tif (ref_transaction_update(transaction, ref, repl, prev,\n> +\t\t\t\t   0, !is_null_sha1(prev)))\n> +\t\tdie(_(\"%s: cannot update transaction\"), ref);\n> +\tif (ref_transaction_commit(transaction, NULL, UPDATE_REFS_DIE_ON_ERR))\n> +\t\tdie(_(\"%s: cannot commit transaction\"), ref);\n\nThe same comment that I made to 05/11 applies here too (and, I predict,\nto other patches yet to come).\n\n>  \n>  \treturn 0;\n>  }\n> \n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"239134","messageId":"5352CD14.4060808@alum.mit.edu","threadId":"36433","inReplyTo":"1397763987-4453-8-git-send-email-sahlberg@google.com","subject":"Re: [PATCH 07/11] commit.c: use ref transactions for updates","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-19T19:23:00Z","receivedAt":"2014-04-19T19:23:00Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote:\n> Change commit.c to use ref transactions for all ref updates.\n> \n> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n> ---\n>  builtin/commit.c | 22 ++++++++++++----------\n>  1 file changed, 12 insertions(+), 10 deletions(-)\n> \n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index d9550c5..b8e4389 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1541,11 +1541,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>  \tconst char *index_file, *reflog_msg;\n>  \tchar *nl;\n>  \tunsigned char sha1[20];\n> -\tstruct ref_lock *ref_lock;\n>  \tstruct commit_list *parents = NULL, **pptr = &parents;\n>  \tstruct stat statbuf;\n>  \tstruct commit *current_head = NULL;\n>  \tstruct commit_extra_header *extra = NULL;\n> +\tstruct ref_transaction *transaction;\n>  \n>  \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n>  \t\tusage_with_options(builtin_commit_usage, builtin_commit_options);\n> @@ -1667,12 +1667,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>  \tstrbuf_release(&author_ident);\n>  \tfree_commit_extra_headers(extra);\n>  \n> -\tref_lock = lock_any_ref_for_update(\"HEAD\",\n> -\t\t\t\t\t   !current_head\n> -\t\t\t\t\t   ? NULL\n> -\t\t\t\t\t   : current_head->object.sha1,\n> -\t\t\t\t\t   0, NULL);\n\nThe old version, above, contemplates that current_head might be NULL...\n\n> -\n>  \tnl = strchr(sb.buf, '\\n');\n>  \tif (nl)\n>  \t\tstrbuf_setlen(&sb, nl + 1 - sb.buf);\n> @@ -1681,14 +1675,22 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>  \tstrbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));\n>  \tstrbuf_insert(&sb, strlen(reflog_msg), \": \", 2);\n>  \n> -\tif (!ref_lock) {\n> +\ttransaction = ref_transaction_begin();\n> +\tif (!transaction) {\n>  \t\trollback_index_files();\n> -\t\tdie(_(\"cannot lock HEAD ref\"));\n> +\t\tdie(_(\"HEAD: cannot start transaction\"));\n>  \t}\n> -\tif (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {\n> +\tif (ref_transaction_update(transaction, \"HEAD\", sha1,\n> +\t\t\t\t   current_head->object.sha1,\n> +\t\t\t\t   0, !!current_head)) {\n\n...but here you dereference current_head without checking it first.\n\nIt upsets me that the test suite didn't catch this NULL pointer\ndereference.  Either\n\n1. current_head cannot in fact be NULL, in which case the commit message\nshould explain that fact and the code should be simplified\n\nor\n\n2. the test suite is incomplete.  If so, it would be great if you would\nadd a test that exercises this branch of the code (and catches your\nerror), and then fix the error.\n\n>  \t\trollback_index_files();\n>  \t\tdie(_(\"cannot update HEAD ref\"));\n>  \t}\n> +\tif (ref_transaction_commit(transaction, sb.buf,\n> +\t\t\t\t   UPDATE_REFS_QUIET_ON_ERR)) {\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> \n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"239136","messageId":"5352D31A.6000107@alum.mit.edu","threadId":"36433","inReplyTo":"1397763987-4453-12-git-send-email-sahlberg@google.com","subject":"Re: [PATCH 11/11] walker.c: use ref transaction for ref updates","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-19T19:48:42Z","receivedAt":"2014-04-19T19:48:42Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote:\n> Switch to using ref transactions in walker_fetch(). As part of the refactoring\n> to use ref transactions we also fix a potential memory leak where in the\n> original code if write_ref_sha1() would fail we would end up returning from\n> the function without free()ing the msg string.\n\nI don't have time to review this last patch this evening, but one thing\nstruck me.  It seems like the old code went to extra effort to lock all\nthe write_ref references early in the function, whereas your modified\nversion doesn't lock them until later.  Have you verified that you are\nnot opening a possible race condition?  If so, your commit message\nshould justify that it isn't a problem.  In other words, what does the\ncode do between the old time of locking and the new time of locking and\nwhy doesn't it care whether the references are locked?\n\nAside from my other comments, patches 01-10 in the series looked fine.\nThanks!\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"239215","messageId":"CAL=YDW=5Xf4rq11uxA67zbrK0gGPFhs24SL0NCnny3UObtZAfw@mail.gmail.com","threadId":"36433","inReplyTo":"5352CD14.4060808@alum.mit.edu","subject":"Re: [PATCH 07/11] commit.c: use ref transactions for updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-21T18:45:06Z","receivedAt":"2014-04-21T18:45:06Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Sat, Apr 19, 2014 at 12:23 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote:\n>> Change commit.c to use ref transactions for all ref updates.\n>>\n>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n>> ---\n>>  builtin/commit.c | 22 ++++++++++++----------\n>>  1 file changed, 12 insertions(+), 10 deletions(-)\n>>\n>> diff --git a/builtin/commit.c b/builtin/commit.c\n>> index d9550c5..b8e4389 100644\n>> --- a/builtin/commit.c\n>> +++ b/builtin/commit.c\n>> @@ -1541,11 +1541,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>>       const char *index_file, *reflog_msg;\n>>       char *nl;\n>>       unsigned char sha1[20];\n>> -     struct ref_lock *ref_lock;\n>>       struct commit_list *parents = NULL, **pptr = &parents;\n>>       struct stat statbuf;\n>>       struct commit *current_head = NULL;\n>>       struct commit_extra_header *extra = NULL;\n>> +     struct ref_transaction *transaction;\n>>\n>>       if (argc == 2 && !strcmp(argv[1], \"-h\"))\n>>               usage_with_options(builtin_commit_usage, builtin_commit_options);\n>> @@ -1667,12 +1667,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>>       strbuf_release(&author_ident);\n>>       free_commit_extra_headers(extra);\n>>\n>> -     ref_lock = lock_any_ref_for_update(\"HEAD\",\n>> -                                        !current_head\n>> -                                        ? NULL\n>> -                                        : current_head->object.sha1,\n>> -                                        0, NULL);\n>\n> The old version, above, contemplates that current_head might be NULL...\n>\n>> -\n>>       nl = strchr(sb.buf, '\\n');\n>>       if (nl)\n>>               strbuf_setlen(&sb, nl + 1 - sb.buf);\n>> @@ -1681,14 +1675,22 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>>       strbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));\n>>       strbuf_insert(&sb, strlen(reflog_msg), \": \", 2);\n>>\n>> -     if (!ref_lock) {\n>> +     transaction = ref_transaction_begin();\n>> +     if (!transaction) {\n>>               rollback_index_files();\n>> -             die(_(\"cannot lock HEAD ref\"));\n>> +             die(_(\"HEAD: cannot start transaction\"));\n>>       }\n>> -     if (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {\n>> +     if (ref_transaction_update(transaction, \"HEAD\", sha1,\n>> +                                current_head->object.sha1,\n>> +                                0, !!current_head)) {\n>\n> ...but here you dereference current_head without checking it first.\n>\n> It upsets me that the test suite didn't catch this NULL pointer\n> dereference.  Either\n>\n> 1. current_head cannot in fact be NULL, in which case the commit message\n> should explain that fact and the code should be simplified\n>\n> or\n>\n> 2. the test suite is incomplete.  If so, it would be great if you would\n> add a test that exercises this branch of the code (and catches your\n> error), and then fix the error.\n\n\n\nCurrent_head can in fact be NULL here but we never actually\ndereference any pointer in this case since !!current_head is 0.\n\n\ncurrent_head->object.sha1 just computes current_head +\noffsetof(commit, object) + offsetof(object, sha1)\nso we compute and pass a non-NULL garbage pointer into ref_transaction_update()\n\nBut since !!current_head is 0 this mean we never actually dereference\nthis pointer.\nWay to subtle for its own good.\n\n\nI will change ref_transaction_update and friends to use an additional\ntest to detect some subset of this kind of bug :\n\nif (!have_old && old_sha1)\n   die(\"have_old is false but old_sha1 is not NULL\");\n\nregards\nronnie sahlberg\n"},{"id":"239243","messageId":"xmqqioq28jcv.fsf@gitster.dls.corp.google.com","threadId":"36433","inReplyTo":"5352D31A.6000107@alum.mit.edu","subject":"Re: [PATCH 11/11] walker.c: use ref transaction for ref updates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-21T21:26:24Z","receivedAt":"2014-04-21T21:26:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote:\n>> Switch to using ref transactions in walker_fetch(). As part of the refactoring\n>> to use ref transactions we also fix a potential memory leak where in the\n>> original code if write_ref_sha1() would fail we would end up returning from\n>> the function without free()ing the msg string.\n> ...\n> Aside from my other comments, patches 01-10 in the series looked fine.\n> Thanks!\n\nThanks, both.\n\nWhen this is queued on 'pu', it appears that t5550 breaks.  I had to\nfutz with conflicts with the lockfile topic, so a botched conflict\nresolution might be a cause for the breakage, though.\n"},{"id":"239255","messageId":"CAL=YDW=A_L3J6bD+ZXMGoZ0piuQbdGmS1nmTpuCD0yZEnj2BOw@mail.gmail.com","threadId":"36433","inReplyTo":"5352D31A.6000107@alum.mit.edu","subject":"Re: [PATCH 11/11] walker.c: use ref transaction for ref updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-21T22:29:22Z","receivedAt":"2014-04-21T22:29:22Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"I have updated the commit message with some text why I do not think\nthis change is critical for this case.\nI will resend v2 of the patch series in a little while.\n\nOn Sat, Apr 19, 2014 at 12:48 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote:\n>> Switch to using ref transactions in walker_fetch(). As part of the refactoring\n>> to use ref transactions we also fix a potential memory leak where in the\n>> original code if write_ref_sha1() would fail we would end up returning from\n>> the function without free()ing the msg string.\n>\n> I don't have time to review this last patch this evening, but one thing\n> struck me.  It seems like the old code went to extra effort to lock all\n> the write_ref references early in the function, whereas your modified\n> version doesn't lock them until later.  Have you verified that you are\n> not opening a possible race condition?  If so, your commit message\n> should justify that it isn't a problem.  In other words, what does the\n> code do between the old time of locking and the new time of locking and\n> why doesn't it care whether the references are locked?\n>\n> Aside from my other comments, patches 01-10 in the series looked fine.\n> Thanks!\n>\n> Michael\n>\n> --\n> Michael Haggerty\n> mhagger@alum.mit.edu\n> http://softwareswirl.blogspot.com/\n"},{"id":"239712","messageId":"20140425213210.GN15516@google.com","threadId":"36433","inReplyTo":"1397763987-4453-3-git-send-email-sahlberg@google.com","subject":"Re: [PATCH 02/11] refs.c: change ref_transaction_update() to do error checking and return status","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-04-25T21:32:10Z","receivedAt":"2014-04-25T21:32:10Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nRonnie Sahlberg wrote:\n\n> Update ref_transaction_update() do some basic error checking and return\n> true on error. Update all callers to check ref_transaction_update() for error.\n\nMicronit: nonzero, not true.  (true tends to mean '1' while here we\nhave the usual error return of -1.  It's kind of annoying that C\ndoesn't have a nice way to distinguish between the usual int return of\n0 for success and the usual bool return of true for success.)\n\nLooks like a good change.  Some tiny nitpicks below.\n\n[...]\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -237,11 +237,11 @@ void ref_transaction_rollback(struct ref_transaction *transaction);\n>   * that the reference should have had before the update, or zeros if\n>   * it must not have existed beforehand.\n>   */\n> -void ref_transaction_update(struct ref_transaction *transaction,\n> +int ref_transaction_update(struct ref_transaction *transaction,\n\nThe comment above the prototype doesn't tell me:\n\nWhen should the caller expect ref_transaction_update to return an\nerror?  What does an error mean: is it always a sign of a bug in the\ncaller, or can it be due to some other problem?  What guarantees does\nthe caller have about the state after an error --- is it just \"Things\nwill be in a sane state so you can free resources and exit\", or will\nthe ref_transaction_update() have been essentially a no-op allowing\nthe caller to continue?\n\n[...]\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -3327,19 +3327,24 @@ static struct ref_update *add_update(struct ref_transaction *transaction,\n>  \treturn update;\n>  }\n>  \n> -void ref_transaction_update(struct ref_transaction *transaction,\n> +int ref_transaction_update(struct ref_transaction *transaction,\n>  \t\t\t    const char *refname,\n>  \t\t\t    const unsigned char *new_sha1,\n>  \t\t\t    const unsigned char *old_sha1,\n>  \t\t\t    int flags, int have_old)\n>  {\n> -\tstruct ref_update *update = add_update(transaction, refname);\n> +\tstruct ref_update *update;\n> +\n> +\tif (have_old && !old_sha1)\n> +\t\treturn error(\"have_old is true but old_sha1 is NULL\");\n\nI agree with Michael that the error message should start with \"BUG:\"\nso humans encountering this know to contact the list instead of\nblaming themselves.\n\nReturning error instead of die-ing seems like a nice thing that make\nit easier to make git valgrind-clean some day.  Others might disagree\nwith me about whether that's worthwhile, but I think it's a good\nchange. :)\n\n[...]\n> --- a/builtin/update-ref.c\n> +++ b/builtin/update-ref.c\n> @@ -197,8 +197,10 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next)\n>  \tif (*next != line_termination)\n>  \t\tdie(\"update %s: extra input: %s\", refname, next);\n>  \n> -\tref_transaction_update(transaction, refname, new_sha1, old_sha1,\n> -\t\t\t       update_flags, have_old);\n> +\tif (ref_transaction_update(transaction, refname, new_sha1, old_sha1,\n> +\t\t\t\t   update_flags, have_old))\n> +\t\tdie(\"failed transaction update for %s\", refname);\n\nref_transaction_update already printed an error, but of course that's\nno guarantee that bugs in ref_transaction_update will not cause it\nto fail without printing a message in the future.  And the extra\ncontext for the error might be nice (but why not print refname in\nthe message from ref_transaction_update instead?).\n\nIs the plan for ref_transaction_update to be able to fail for\nother reasons some day?  What is the contract --- do we need a\nhuman-readable, translatable message here, or is a \"this can't\nhappen\" BUG message fine?\n\nI'd be fine with\n\n\t\tdie(\"BUG: failed transa...\n\nor\n\n\t\t/* ref_transaction_update already printed a message */\n\t\texit(128)\n\nwith a slight preference for the former, for what it's worth.\n\n[...]\n> @@ -286,8 +288,9 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)\n>  \tif (*next != line_termination)\n>  \t\tdie(\"verify %s: extra input: %s\", refname, next);\n>  \n> -\tref_transaction_update(transaction, refname, new_sha1, old_sha1,\n> -\t\t\t       update_flags, have_old);\n> +\tif (ref_transaction_update(transaction, refname, new_sha1, old_sha1,\n> +\t\t\t       update_flags, have_old))\n> +\t\tdie(\"failed transaction update for %s\", refname);\n\nLikewise.\n\nThanks,\nJonathan\n"}]}