{"thread":{"id":"36467","subject":"[PATCH v2 00/13] Use ref transactions from most callers","startedAt":"2014-04-21T22:53:18Z","lastAt":"2014-04-22T22:46:41Z","messageCount":19,"participants":["Ronnie Sahlberg","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":13},"messages":[{"id":"239259","messageId":"1398120811-20284-1-git-send-email-sahlberg@google.com","threadId":"36467","inReplyTo":null,"subject":"[PATCH v2 00/13] Use ref transactions from most callers","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-21T22:53:18Z","receivedAt":"2014-04-21T22:53:18Z","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\nVersion 2:\n - Add a patch to ref_transaction_commit to make it honor onerr even if the\n   error triggered in ref_Transaction_commit itself rather than in a call\n   to other functions (that already honor onerr).\n - Add a patch to make the update_ref() helper function use transactions\n   internally.\n - Change ref_transaction_update to die() instead of error() if we pass\n   if a NULL old_sha1 but have have_old == true.\n - Change ref_transaction_create to die() instead of error() if new_sha1\n   is false but we pass it a null_sha1.\n - Change ref_transaction_delete die() instead of error() if we pass\n   if a NULL old_sha1 but have have_old == true.\n - Change several places to do  if(!transaction || ref_transaction_update()\n   || ref_Transaction_commit()) die(generic-message) instead of checking each\n   step separately and having a different message for each failure.\n   Most users are likely not interested in what step of the transaction\n   failed and only whether it failed or not.\n - Change commit.c to only pass a pointer to ref_transaction_update\n   iff current_head is non-NULL.\n   The previous patch used to compute a garbage pointer for\n   current_head->object.sha1 and relied on the fact that ref_transaction_update\n   would not try to dereference this pointer if !!current_head was 0.\n - Updated commit message for the walker_fetch change to try to justify why\n   the change in locking semantics should not be harmful.\n\n\nRonnie Sahlberg (13):\n  refs.c: constify the sha arguments for\n    ref_transaction_create|delete|update\n  refs.c: use a single exit path from transaction commit and handle\n    onerr\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  refs.c: change update_ref to use a transaction\n\n branch.c             | 16 ++++++----\n builtin/commit.c     | 23 +++++++-------\n builtin/replace.c    | 13 ++++----\n builtin/tag.c        | 11 +++----\n builtin/update-ref.c | 19 +++++++-----\n fast-import.c        | 23 +++++++++-----\n refs.c               | 84 +++++++++++++++++++++++++++++++++++++++-------------\n refs.h               | 25 ++++++++--------\n sequencer.c          | 17 ++++++++---\n walker.c             | 45 +++++++++++++---------------\n 10 files changed, 172 insertions(+), 104 deletions(-)\n\n-- \n1.9.1.515.g3b87021\n"},{"id":"239261","messageId":"1398120811-20284-2-git-send-email-sahlberg@google.com","threadId":"36467","inReplyTo":"1398120811-20284-1-git-send-email-sahlberg@google.com","subject":"[PATCH v2 01/13] refs.c: constify the sha arguments for ref_transaction_create|delete|update","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-21T22:53:19Z","receivedAt":"2014-04-21T22:53:19Z","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.515.g3b87021\n"},{"id":"239263","messageId":"1398120811-20284-3-git-send-email-sahlberg@google.com","threadId":"36467","inReplyTo":"1398120811-20284-1-git-send-email-sahlberg@google.com","subject":"[PATCH v2 02/13] refs.c: use a single exit path from transaction commit and handle onerr","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-21T22:53:20Z","receivedAt":"2014-04-21T22:53:20Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Update ref_transaction_commit to have a single exit path and process onerr\nif an error occured during hte commit. This does mean that in case of an error\noccuring for UPDATE_REFS_MSG_ON_ERR during the calls to update_ref_lock or\nupdate_ref_write we will log errors from both those functions as well as a\ngeneric message from ref_transaction_commit.\n\nI thought a while to make the MSG_ON_ERR message in ref_transaction_commit\nconditional to only trigger if the error was not triggered by the two functions\nwe call that also take onerr, and which would already have logger an error\nalready for this case, but the code would just look too awful. I think it\nis acceptable to log two error messages for those two cases than to badify\nthe commit code.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n refs.c | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 138ab70..9daf89e 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3414,12 +3414,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t   const char *msg, enum action_on_err onerr)\n {\n \tint ret = 0, delnum = 0, i;\n-\tconst char **delnames;\n+\tconst char **delnames = NULL;\n \tint n = transaction->nr;\n \tstruct ref_update **updates = transaction->updates;\n \n \tif (!n)\n-\t\treturn 0;\n+\t\tgoto cleanup;\n \n \t/* Allocate work space */\n \tdelnames = xmalloc(sizeof(*delnames) * n);\n@@ -3481,6 +3481,14 @@ cleanup:\n \t\t\tunlock_ref(updates[i]->lock);\n \tfree(delnames);\n \tref_transaction_free(transaction);\n+\tif (ret) {\n+\t\tconst char *str = \"Cannot commit transaction.\";\n+\t\tswitch (onerr) {\n+\t\tcase UPDATE_REFS_MSG_ON_ERR: error(str); break;\n+\t\tcase UPDATE_REFS_DIE_ON_ERR: die(str); break;\n+\t\tcase UPDATE_REFS_QUIET_ON_ERR: break;\n+\t\t}\n+\t}\n \treturn ret;\n }\n \n-- \n1.9.1.515.g3b87021\n"},{"id":"239262","messageId":"1398120811-20284-4-git-send-email-sahlberg@google.com","threadId":"36467","inReplyTo":"1398120811-20284-1-git-send-email-sahlberg@google.com","subject":"[PATCH v2 03/13] refs.c: change ref_transaction_update() to do error checking and return status","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-21T22:53:21Z","receivedAt":"2014-04-21T22:53:21Z","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 | 10 ++++++----\n refs.c               |  9 +++++++--\n refs.h               | 10 +++++-----\n 3 files changed, 18 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 405267f..811e0b0 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -197,8 +197,9 @@ 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 \tupdate_flags = 0;\n \tfree(refname);\n@@ -286,8 +287,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\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 9daf89e..8c02779 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\tdie(\"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.515.g3b87021\n"},{"id":"239260","messageId":"1398120811-20284-5-git-send-email-sahlberg@google.com","threadId":"36467","inReplyTo":"1398120811-20284-1-git-send-email-sahlberg@google.com","subject":"[PATCH v2 04/13] refs.c: change ref_transaction_create to do error checking and return status","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-21T22:53:22Z","receivedAt":"2014-04-21T22:53:22Z","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 811e0b0..7d1e0c0 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -225,7 +225,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 8c02779..8c728c6 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\tdie(\"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.515.g3b87021\n"},{"id":"239264","messageId":"1398120811-20284-6-git-send-email-sahlberg@google.com","threadId":"36467","inReplyTo":"1398120811-20284-1-git-send-email-sahlberg@google.com","subject":"[PATCH v2 05/13] refs.c: ref_transaction_delete to check for error and return status","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-21T22:53:23Z","receivedAt":"2014-04-21T22:53:23Z","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 7d1e0c0..b65445d 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -257,8 +257,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 8c728c6..878948a 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\tdie(\"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.515.g3b87021\n"},{"id":"239266","messageId":"1398120811-20284-7-git-send-email-sahlberg@google.com","threadId":"36467","inReplyTo":"1398120811-20284-1-git-send-email-sahlberg@google.com","subject":"[PATCH v2 06/13] tag.c: use ref transactions when doing updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-21T22:53:24Z","receivedAt":"2014-04-21T22:53:24Z","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 | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 40356e3..3c40957 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,10 +641,11 @@ 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+\ttransaction = ref_transaction_begin();\n+\tif (!transaction ||\n+\t    ref_transaction_update(transaction, ref.buf, object, prev,\n+\t\t\t\t   0, !is_null_sha1(prev)) ||\n+\t    ref_transaction_commit(transaction, NULL, UPDATE_REFS_DIE_ON_ERR))\n \t\tdie(_(\"%s: cannot update 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-- \n1.9.1.515.g3b87021\n"},{"id":"239265","messageId":"1398120811-20284-8-git-send-email-sahlberg@google.com","threadId":"36467","inReplyTo":"1398120811-20284-1-git-send-email-sahlberg@google.com","subject":"[PATCH v2 07/13] replace.c: use the ref transaction functions for updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-21T22:53:25Z","receivedAt":"2014-04-21T22:53:25Z","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 | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex b62420a..b91e550 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,12 @@ 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    ref_transaction_update(transaction, ref, repl, prev,\n+\t\t\t\t   0, !is_null_sha1(prev)) ||\n+\t    ref_transaction_commit(transaction, NULL, UPDATE_REFS_DIE_ON_ERR))\n+\t\tdie(_(\"%s: failed to replace ref\"), ref);\n \n \treturn 0;\n }\n-- \n1.9.1.515.g3b87021\n"},{"id":"239267","messageId":"1398120811-20284-9-git-send-email-sahlberg@google.com","threadId":"36467","inReplyTo":"1398120811-20284-1-git-send-email-sahlberg@google.com","subject":"[PATCH v2 08/13] commit.c: use ref transactions for updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-21T22:53:26Z","receivedAt":"2014-04-21T22:53:26Z","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.\nMake sure we pass a NULL pointer to ref_transaction_update if have_old\nis false.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n builtin/commit.c | 23 +++++++++++++----------\n 1 file changed, 13 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex d9550c5..1b3c764 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,23 @@ 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 ? \n+\t\t\t\t   current_head->object.sha1 : NULL,\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.515.g3b87021\n"},{"id":"239268","messageId":"1398120811-20284-10-git-send-email-sahlberg@google.com","threadId":"36467","inReplyTo":"1398120811-20284-1-git-send-email-sahlberg@google.com","subject":"[PATCH v2 09/13] sequencer.c: use ref transactions for all ref updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-21T22:53:27Z","receivedAt":"2014-04-21T22:53:27Z","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.515.g3b87021\n"},{"id":"239269","messageId":"1398120811-20284-11-git-send-email-sahlberg@google.com","threadId":"36467","inReplyTo":"1398120811-20284-1-git-send-email-sahlberg@google.com","subject":"[PATCH v2 10/13] fast-import.c: change update_branch to use ref transactions","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-21T22:53:28Z","receivedAt":"2014-04-21T22:53:28Z","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.515.g3b87021\n"},{"id":"239270","messageId":"1398120811-20284-12-git-send-email-sahlberg@google.com","threadId":"36467","inReplyTo":"1398120811-20284-1-git-send-email-sahlberg@google.com","subject":"[PATCH v2 11/13] branch.c: use ref transaction for all ref updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-21T22:53:29Z","receivedAt":"2014-04-21T22:53:29Z","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.515.g3b87021\n"},{"id":"239272","messageId":"1398120811-20284-13-git-send-email-sahlberg@google.com","threadId":"36467","inReplyTo":"1398120811-20284-1-git-send-email-sahlberg@google.com","subject":"[PATCH v2 12/13] walker.c: use ref transaction for ref updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-21T22:53:30Z","receivedAt":"2014-04-21T22:53:30Z","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\nThis changes the locking slightly for walker_fetch. Previously the code would\nlock all refs before writing them but now we do not lock the refs until the\ncommit stage. There is thus a very short window where changes could be done\nlocally during the fetch which would be overwritten when the fetch completes\nand commits its transaction. But this window should be reasonably short.\nEven if this race does trigger, since both the old code and the new code\njust overwrites the refs to the new values without checking or comparing\nthem with the previous value, this is not too dissimilar to a similar scenario\nwhere you first do a ref change locally and then later do a fetch that\noverwrites the local change. With this in mind I do not see the change in\nlocking semantics to be critical.\n\nNote that this function is only called when fetching from a remote HTTP\nrepository onto the local (most of the time single-user) repository which\nlikely means that the type of collissions that the previous locking would\nprotect against and cause the fetch to fail for to be even more rare.\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.515.g3b87021\n"},{"id":"239271","messageId":"1398120811-20284-14-git-send-email-sahlberg@google.com","threadId":"36467","inReplyTo":"1398120811-20284-1-git-send-email-sahlberg@google.com","subject":"[PATCH v2 13/13] refs.c: change update_ref to use a transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-21T22:53:31Z","receivedAt":"2014-04-21T22:53:31Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change the update_ref helper function to use a ref transaction internally.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n refs.c | 28 +++++++++++++++++++++++-----\n 1 file changed, 23 insertions(+), 5 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 878948a..e52b6bf 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3390,11 +3390,29 @@ int update_ref(const char *action, const char *refname,\n \t       const unsigned char *sha1, const unsigned char *oldval,\n \t       int flags, enum action_on_err onerr)\n {\n-\tstruct ref_lock *lock;\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+\tstruct ref_transaction *t;\n+\tconst char *str = \"update_ref failed for ref '%s'.\";\n+\n+\tt = ref_transaction_begin();\n+\tif (!t)\n+\t\tgoto error_return;\n+\tif (ref_transaction_update(t, refname, sha1, oldval, flags,\n+\t\t\t\t   !!oldval)) {\n+\t\tref_transaction_rollback(t);\n+\t\tgoto error_return;\n+\t}\n+\tif (ref_transaction_commit(t, action, onerr))\n+\t\tgoto error_return;\n+\n+\treturn 0;\n+\n+error_return:\n+\tswitch (onerr) {\n+\tcase UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;\n+\tcase UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;\n+\tcase UPDATE_REFS_QUIET_ON_ERR: break;\n+\t}\n+\treturn 1;\n }\n \n static int ref_update_compare(const void *r1, const void *r2)\n-- \n1.9.1.515.g3b87021\n"},{"id":"239368","messageId":"xmqqbnvt5ge2.fsf@gitster.dls.corp.google.com","threadId":"36467","inReplyTo":"1398120811-20284-3-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v2 02/13] refs.c: use a single exit path from transaction commit and handle onerr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-22T19:11:01Z","receivedAt":"2014-04-22T19:11:01Z","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> diff --git a/refs.c b/refs.c\n> index 138ab70..9daf89e 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -3414,12 +3414,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n>  \t\t\t   const char *msg, enum action_on_err onerr)\n> ...\n> +\tif (ret) {\n> +\t\tconst char *str = \"Cannot commit transaction.\";\n> +\t\tswitch (onerr) {\n> +\t\tcase UPDATE_REFS_MSG_ON_ERR: error(str); break;\n> +\t\tcase UPDATE_REFS_DIE_ON_ERR: die(str); break;\n> +\t\tcase UPDATE_REFS_QUIET_ON_ERR: break;\n> +\t\t}\n> +\t}\n>  \treturn ret;\n>  }\n\nI think this is a response to Michael's earlier review \"do different\ncallers want to give different error messages more suitable for the\nsituation?\".  I suspect that the ideal endgame may end up all\ncallers passing QUIET_ON_ERR and doing the error message themselves,\ne.g. branch.c::craete_branch() may end something like this:\n\n    ...\n    if (!dont_change_ref)\n        if (ref_transaction_commit(..., UPDATE_REFS_QUIET_ON_ERR))\n                die_errno(_(\"Failed to write branch '%s'\"),\n                          skip_prefix(ref.buf, \"refs/heads/));\n\nAnd in the meantime until the callers are converted, we may end up\nshowing the fallback \"Cannot commit transaction.\" but that would be\nOK during the development and polishing of this series.\n"},{"id":"239370","messageId":"xmqq7g6h5g7m.fsf@gitster.dls.corp.google.com","threadId":"36467","inReplyTo":"1398120811-20284-3-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v2 02/13] refs.c: use a single exit path from transaction commit and handle onerr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-22T19:14:53Z","receivedAt":"2014-04-22T19:14:53Z","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> @@ -3481,6 +3481,14 @@ cleanup:\n>  \t\t\tunlock_ref(updates[i]->lock);\n>  \tfree(delnames);\n>  \tref_transaction_free(transaction);\n> +\tif (ret) {\n> +\t\tconst char *str = \"Cannot commit transaction.\";\n> +\t\tswitch (onerr) {\n> +\t\tcase UPDATE_REFS_MSG_ON_ERR: error(str); break;\n> +\t\tcase UPDATE_REFS_DIE_ON_ERR: die(str); break;\n> +\t\tcase UPDATE_REFS_QUIET_ON_ERR: break;\n> +\t\t}\n> +\t}\n>  \treturn ret;\n>  }\n\nAlso on top of this part:\n\n - avoid complier warning for printf-like functions getting a non\n   literal format string as their format argument;\n\n - style: case label and each statement on its own line.\n\n - Allow localizing the error message.\n\ndiff --git a/refs.c b/refs.c\nindex e52b6bf..35ce61a 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3515,11 +3515,16 @@ cleanup:\n \tfree(delnames);\n \tref_transaction_free(transaction);\n \tif (ret) {\n-\t\tconst char *str = \"Cannot commit transaction.\";\n+\t\tconst char *str = _(\"Cannot commit transaction.\");\n \t\tswitch (onerr) {\n-\t\tcase UPDATE_REFS_MSG_ON_ERR: error(str); break;\n-\t\tcase UPDATE_REFS_DIE_ON_ERR: die(str); break;\n-\t\tcase UPDATE_REFS_QUIET_ON_ERR: break;\n+\t\tcase UPDATE_REFS_MSG_ON_ERR:\n+\t\t\terror(\"%s\", str);\n+\t\t\tbreak;\n+\t\tcase UPDATE_REFS_DIE_ON_ERR:\n+\t\t\tdie(\"%s\", str);\n+\t\t\tbreak;\n+\t\tcase UPDATE_REFS_QUIET_ON_ERR:\n+\t\t\tbreak;\n \t\t}\n \t}\n \treturn ret;\n-- \n2.0.0-rc0-187-g5842ffa\n"},{"id":"239372","messageId":"xmqqzjjd40q3.fsf@gitster.dls.corp.google.com","threadId":"36467","inReplyTo":"1398120811-20284-1-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v2 00/13] Use ref transactions from most callers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-22T19:34:44Z","receivedAt":"2014-04-22T19:34:44Z","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> This patch series changes most of the places where the ref functions for\n> locking and writing refs to instead use the new ref transaction API. There\n> are still three more places where write_ref_sha1() is called from outside\n> of refs.c but those all will require more complex work and review so those\n> changes belong in a different patch series.\n>\n> Version 2:\n>  - Add a patch to ref_transaction_commit to make it honor onerr even if the\n>    error triggered in ref_Transaction_commit itself rather than in a call\n>    to other functions (that already honor onerr).\n>  - Add a patch to make the update_ref() helper function use transactions\n>    internally.\n>  - Change ref_transaction_update to die() instead of error() if we pass\n>    if a NULL old_sha1 but have have_old == true.\n>  - Change ref_transaction_create to die() instead of error() if new_sha1\n>    is false but we pass it a null_sha1.\n>  - Change ref_transaction_delete die() instead of error() if we pass\n>    if a NULL old_sha1 but have have_old == true.\n>  - Change several places to do  if(!transaction || ref_transaction_update()\n>    || ref_Transaction_commit()) die(generic-message) instead of checking each\n>    step separately and having a different message for each failure.\n>    Most users are likely not interested in what step of the transaction\n>    failed and only whether it failed or not.\n>  - Change commit.c to only pass a pointer to ref_transaction_update\n>    iff current_head is non-NULL.\n>    The previous patch used to compute a garbage pointer for\n>    current_head->object.sha1 and relied on the fact that ref_transaction_update\n>    would not try to dereference this pointer if !!current_head was 0.\n>  - Updated commit message for the walker_fetch change to try to justify why\n>    the change in locking semantics should not be harmful.\n\nWill queue, but when applied on top of mh/ref-transaction and tested\nstandalone, it appears to fail test #14 of t5550-http-fetch-dumb.sh\nfor me.\n\nIt seems that the culprit is that this step:\n\n    git http-fetch -a -w heads/master-new $HEAD $(git config remote.origin.url) &&\n\ncreates \".git/heads/master-new\" when what it was asked to create was\n\".git/refs/heads/master-new\".  Perhaps there is something fishy in\nthe conversion in walker.c?  We used to do lock_ref_sha1() on\n\"heads/master-new\", which prepended \"refs/\" prefix before calling\nlock_ref_sha1_basic() that expects the full path from $GIT_DIR/, but\nref_transaction_update(), which wants to see the full path, is still\nfed \"heads/master-new\" after this series.\n"},{"id":"239396","messageId":"CAL=YDWmCLvQRe6CQddKQxhHXrPnTx7iXh=t0JQ_GhE2jvmgNhQ@mail.gmail.com","threadId":"36467","inReplyTo":"xmqqzjjd40q3.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 00/13] Use ref transactions from most callers","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-22T22:43:56Z","receivedAt":"2014-04-22T22:43:56Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"I will look at this once i finish the next respin.\n\nOn Tue, Apr 22, 2014 at 12:34 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Ronnie Sahlberg <sahlberg@google.com> writes:\n>\n>> This patch series changes most of the places where the ref functions for\n>> locking and writing refs to instead use the new ref transaction API. There\n>> are still three more places where write_ref_sha1() is called from outside\n>> of refs.c but those all will require more complex work and review so those\n>> changes belong in a different patch series.\n>>\n>> Version 2:\n>>  - Add a patch to ref_transaction_commit to make it honor onerr even if the\n>>    error triggered in ref_Transaction_commit itself rather than in a call\n>>    to other functions (that already honor onerr).\n>>  - Add a patch to make the update_ref() helper function use transactions\n>>    internally.\n>>  - Change ref_transaction_update to die() instead of error() if we pass\n>>    if a NULL old_sha1 but have have_old == true.\n>>  - Change ref_transaction_create to die() instead of error() if new_sha1\n>>    is false but we pass it a null_sha1.\n>>  - Change ref_transaction_delete die() instead of error() if we pass\n>>    if a NULL old_sha1 but have have_old == true.\n>>  - Change several places to do  if(!transaction || ref_transaction_update()\n>>    || ref_Transaction_commit()) die(generic-message) instead of checking each\n>>    step separately and having a different message for each failure.\n>>    Most users are likely not interested in what step of the transaction\n>>    failed and only whether it failed or not.\n>>  - Change commit.c to only pass a pointer to ref_transaction_update\n>>    iff current_head is non-NULL.\n>>    The previous patch used to compute a garbage pointer for\n>>    current_head->object.sha1 and relied on the fact that ref_transaction_update\n>>    would not try to dereference this pointer if !!current_head was 0.\n>>  - Updated commit message for the walker_fetch change to try to justify why\n>>    the change in locking semantics should not be harmful.\n>\n> Will queue, but when applied on top of mh/ref-transaction and tested\n> standalone, it appears to fail test #14 of t5550-http-fetch-dumb.sh\n> for me.\n>\n> It seems that the culprit is that this step:\n>\n>     git http-fetch -a -w heads/master-new $HEAD $(git config remote.origin.url) &&\n>\n> creates \".git/heads/master-new\" when what it was asked to create was\n> \".git/refs/heads/master-new\".  Perhaps there is something fishy in\n> the conversion in walker.c?  We used to do lock_ref_sha1() on\n> \"heads/master-new\", which prepended \"refs/\" prefix before calling\n> lock_ref_sha1_basic() that expects the full path from $GIT_DIR/, but\n> ref_transaction_update(), which wants to see the full path, is still\n> fed \"heads/master-new\" after this series.\n>\n"},{"id":"239397","messageId":"CAL=YDW=8tHEEZ5_gMhC6nEzJui-L=SKR9b+UQmD-EUDxyTGUdA@mail.gmail.com","threadId":"36467","inReplyTo":"xmqqbnvt5ge2.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 02/13] refs.c: use a single exit path from transaction commit and handle onerr","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-22T22:46:41Z","receivedAt":"2014-04-22T22:46:41Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Tue, Apr 22, 2014 at 12:11 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Ronnie Sahlberg <sahlberg@google.com> writes:\n>\n>> diff --git a/refs.c b/refs.c\n>> index 138ab70..9daf89e 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -3414,12 +3414,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n>>                          const char *msg, enum action_on_err onerr)\n>> ...\n>> +     if (ret) {\n>> +             const char *str = \"Cannot commit transaction.\";\n>> +             switch (onerr) {\n>> +             case UPDATE_REFS_MSG_ON_ERR: error(str); break;\n>> +             case UPDATE_REFS_DIE_ON_ERR: die(str); break;\n>> +             case UPDATE_REFS_QUIET_ON_ERR: break;\n>> +             }\n>> +     }\n>>       return ret;\n>>  }\n>\n> I think this is a response to Michael's earlier review \"do different\n> callers want to give different error messages more suitable for the\n> situation?\".  I suspect that the ideal endgame may end up all\n> callers passing QUIET_ON_ERR and doing the error message themselves,\n> e.g. branch.c::craete_branch() may end something like this:\n>\n>     ...\n>     if (!dont_change_ref)\n>         if (ref_transaction_commit(..., UPDATE_REFS_QUIET_ON_ERR))\n>                 die_errno(_(\"Failed to write branch '%s'\"),\n>                           skip_prefix(ref.buf, \"refs/heads/));\n>\n> And in the meantime until the callers are converted, we may end up\n> showing the fallback \"Cannot commit transaction.\" but that would be\n> OK during the development and polishing of this series.\n>\n\nThat is a good idea.\nI will try to address that in the next respin.\n"}]}