{"thread":{"id":"36530","subject":"[PATCH v4 01/27] refs.c: constify the sha arguments for ref_transaction_create|delete|update","startedAt":"2014-04-28T22:54:15Z","lastAt":"2014-04-29T22:24:50Z","messageCount":38,"participants":["Ronnie Sahlberg","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":27},"messages":[{"id":"240080","messageId":"1398725682-30782-1-git-send-email-sahlberg@google.com","threadId":"36530","inReplyTo":null,"subject":"[PATCH v4 00/27] Use ref transactions for all ref updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T22:54:15Z","receivedAt":"2014-04-28T22:54:15Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"This patch series is based on mhagger/ref-transactions and expands on the\ntransaction API. It converts all external (outside of refs.c) callers to\nuse the transaction API for any writes.\nThis makes most of the ref updates to become atomic when there are failures\nlocking or writing to a ref.\n\n\nVersion 4:\n - Rename patch series from \"Use ref transactions from most callers\" to\n \"Use ref transactions for all ref updates\".\n - Convert all external ref writes to use transactions and make write_ref_sha1\n and lock_ref_sha1 static functions.\n - Change the ref commit and free handling so we no longer pass pointer to\n pointer to _commit. _commit no longer frees the transaction. The caller\n MUST call _free itself.\n - Change _commit to take a strbuf pointer instead of a char* for error\n reporting back to the caller.\n - Re-add the walker patch after fixing it.\n\nVersion 3:\n - Remove the walker patch for now. Walker needs more complex solution\n so defer it until the basics are done.\n - Remove the onerr argument to ref_transaction_commit(). All callers\n that need to die() on error now have to do this explicitely.\n - Pass an error string from ref_transaction_commit() back to the callers\n so that they can craft a nice error message upon failures.\n - Make ref_transaction_rollback() accept NULL as argument.\n - Change ref_transaction_commit() to take a pointer to pointer argument for\n the transaction and have it clear the callers pointer to NULL when\n invoked. This allows for much nicer handling of transaction rollback on\n failure.\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\nRonnie Sahlberg (27):\n refs.c: constify the sha arguments for\n ref_transaction_create|delete|update\n refs.c: allow passing NULL to ref_transaction_free\n refs.c: add a strbuf argument to ref_transaction_commit for error\n logging\n refs.c: make ref_update_reject_duplicates take a strbuf argument for\n errors\n update-ref.c: log transaction error from the update_ref\n refs.c: make update_ref_write update a strbuf on failure\n refs.c: remove the onerr argument to ref_transaction_commit\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 refs.c: change update_ref to use a transaction\n refs.c: free the transaction before returning when number of updates\n is 0\n refs.c: ref_transaction_commit should not free the transaction\n fetch.c: clear errno before calling functions that might set it\n fetch.c: change s_update_ref to use a ref transaction\n fetch.c: use a single ref transaction for all ref updates\n receive-pack.c: use a reference transaction for updating the refs\n fast-import.c: use a ref transaction when dumping tags\n walker.c: use ref transaction for ref updates\n refs.c: make write_ref_sha1 static\n refs.c: make lock_ref_sha1 static\n\n branch.c | 31 +++++++-------\n builtin/commit.c | 24 +++++------\n builtin/fetch.c | 36 ++++++++--------\n builtin/receive-pack.c | 20 ++++-----\n builtin/replace.c | 15 ++++---\n builtin/tag.c | 15 ++++---\n builtin/update-ref.c | 29 +++++++------\n fast-import.c | 37 ++++++++++------\n refs.c | 112 +++++++++++++++++++++++++++++++++----------------\n refs.h | 45 ++++++++++----------\n sequencer.c | 23 +++++++---\n walker.c | 51 +++++++++++-----------\n 12 files changed, 258 insertions(+), 180 deletions(-)\n\n-- \n1.9.1.528.g98b8868.dirty\n"},{"id":"240057","messageId":"1398725682-30782-2-git-send-email-sahlberg@google.com","threadId":"36530","inReplyTo":"1398725682-30782-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 01/27] refs.c: constify the sha arguments for ref_transaction_create|delete|update","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T22:54:16Z","receivedAt":"2014-04-28T22:54:16Z","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 \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.528.g98b8868.dirty\n"},{"id":"240058","messageId":"1398725682-30782-3-git-send-email-sahlberg@google.com","threadId":"36530","inReplyTo":"1398725682-30782-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 02/27] refs.c: allow passing NULL to ref_transaction_free","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T22:54:17Z","receivedAt":"2014-04-28T22:54:17Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Allow ref_transaction_free to be called with NULL and in extension allow\nref_transaction_rollback to be called for a NULL transaction.\n\nThis allows us to write code that will\n if ( (!transaction ||\n ref_transaction_update(...)) ||\n (ref_transaction_commit(...) && !(transaction = NULL)) {\n ref_transaction_rollback(transaction);\n ...\n }\n\nIn this case transaction is reset to NULL IFF ref_transaction_commit() was\ninvoked and thus the rollback becomes ref_transaction_rollback(NULL) which\nis safe. IF the conditional triggered prior to ref_transaction_commit()\nthen transaction is untouched and then ref_transaction_rollback(transaction)\nwill rollback the failed transaction.\n\nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/refs.c b/refs.c\nindex 138ab70..2d83ef6 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3303,6 +3303,9 @@ static void ref_transaction_free(struct ref_transaction *transaction)\n {\n \tint i;\n \n+\tif (!transaction)\n+\t\treturn;\n+\n \tfor (i = 0; i < transaction->nr; i++)\n \t\tfree(transaction->updates[i]);\n \n-- \n1.9.1.528.g98b8868.dirty\n"},{"id":"240082","messageId":"1398725682-30782-4-git-send-email-sahlberg@google.com","threadId":"36530","inReplyTo":"1398725682-30782-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 03/27] refs.c: add a strbuf argument to ref_transaction_commit for error logging","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T22:54:18Z","receivedAt":"2014-04-28T22:54:18Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Add a strbuf argument to _commit so that we can pass an error string back to\nthe caller. So that we can do error logging from the caller instead of from\n_commit.\n\nLonger term plan is to first convert all callers to use onerr==QUIET_ON_ERR\nand craft any log messages from the callers themselves and finally remove the\nonerr argument completely.\n\nSigned-off-by: Ronnie Sahlberg \n---\n builtin/update-ref.c | 2 +-\n refs.c | 6 +++++-\n refs.h | 5 ++++-\n 3 files changed, 10 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 405267f..aaa06aa 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -367,7 +367,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \t\tif (end_null)\n \t\t\tline_termination = '\\0';\n \t\tupdate_refs_stdin();\n-\t\tret = ref_transaction_commit(transaction, msg,\n+\t\tret = ref_transaction_commit(transaction, msg, NULL,\n \t\t\t\t\t UPDATE_REFS_DIE_ON_ERR);\n \t\treturn ret;\n \t}\ndiff --git a/refs.c b/refs.c\nindex 2d83ef6..64e8feb 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3414,7 +3414,8 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n }\n \n int ref_transaction_commit(struct ref_transaction *transaction,\n-\t\t\t const char *msg, enum action_on_err onerr)\n+\t\t\t const char *msg, struct strbuf *err,\n+\t\t\t enum action_on_err onerr)\n {\n \tint ret = 0, delnum = 0, i;\n \tconst char **delnames;\n@@ -3443,6 +3444,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t\t update->flags,\n \t\t\t\t\t &update->type, onerr);\n \t\tif (!update->lock) {\n+\t\t\tif (err)\n+\t\t\t\tstrbuf_addf(err ,\"Cannot lock the ref '%s'.\",\n+\t\t\t\t\t update->refname);\n \t\t\tret = 1;\n \t\t\tgoto cleanup;\n \t\t}\ndiff --git a/refs.h b/refs.h\nindex 892c5b6..ff87e14 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -268,9 +268,12 @@ void ref_transaction_delete(struct ref_transaction *transaction,\n * Commit all of the changes that have been queued in transaction, as\n * atomically as possible. Return a nonzero value if there is a\n * problem. The ref_transaction is freed by this function.\n+ * If err is non-NULL we will add an error string to it to explain why\n+ * the transaction failed.\n */\n int ref_transaction_commit(struct ref_transaction *transaction,\n-\t\t\t const char *msg, enum action_on_err onerr);\n+\t\t\t const char *msg, struct strbuf *err,\n+\t\t\t enum action_on_err onerr);\n \n /** Lock a ref and then write its file */\n int update_ref(const char *action, const char *refname,\n-- \n1.9.1.528.g98b8868.dirty\n"},{"id":"240079","messageId":"1398725682-30782-5-git-send-email-sahlberg@google.com","threadId":"36530","inReplyTo":"1398725682-30782-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 04/27] refs.c: make ref_update_reject_duplicates take a strbuf argument for errors","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T22:54:19Z","receivedAt":"2014-04-28T22:54:19Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Make ref_update_reject_duplicates return any error that occurs through a\nnew strbuf argument.\n\nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 64e8feb..21a147b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3393,6 +3393,7 @@ static int ref_update_compare(const void *r1, const void *r2)\n }\n \n static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n+\t\t\t\t\tstruct strbuf *err,\n \t\t\t\t\tenum action_on_err onerr)\n {\n \tint i;\n@@ -3400,6 +3401,9 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n \t\tif (!strcmp(updates[i - 1]->refname, updates[i]->refname)) {\n \t\t\tconst char *str =\n \t\t\t\t\"Multiple updates for ref '%s' not allowed.\";\n+\t\t\tif (err)\n+\t\t\t\tstrbuf_addf(err, str, updates[i]->refname);\n+\n \t\t\tswitch (onerr) {\n \t\t\tcase UPDATE_REFS_MSG_ON_ERR:\n \t\t\t\terror(str, updates[i]->refname); break;\n@@ -3430,7 +3434,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \n \t/* Copy, sort, and reject duplicate refs */\n \tqsort(updates, n, sizeof(*updates), ref_update_compare);\n-\tret = ref_update_reject_duplicates(updates, n, onerr);\n+\tret = ref_update_reject_duplicates(updates, n, err, onerr);\n \tif (ret)\n \t\tgoto cleanup;\n \n-- \n1.9.1.528.g98b8868.dirty\n"},{"id":"240081","messageId":"1398725682-30782-6-git-send-email-sahlberg@google.com","threadId":"36530","inReplyTo":"1398725682-30782-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 05/27] update-ref.c: log transaction error from the update_ref","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T22:54:20Z","receivedAt":"2014-04-28T22:54:20Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Call ref_transaction_commit with QUIET_ON_ERR and use the strbuf that is\nreturned to print a log message if/after the transaction fails.\n\nSigned-off-by: Ronnie Sahlberg \n---\n builtin/update-ref.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex aaa06aa..207e24d 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -342,6 +342,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \tconst char *refname, *oldval, *msg = NULL;\n \tunsigned char sha1[20], oldsha1[20];\n \tint delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0;\n+\tstruct strbuf err = STRBUF_INIT;\n \tstruct option options[] = {\n \t\tOPT_STRING( 'm', NULL, &msg, N_(\"reason\"), N_(\"reason of the update\")),\n \t\tOPT_BOOL('d', NULL, &delete, N_(\"delete the reference\")),\n@@ -359,17 +360,16 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \t\tdie(\"Refusing to perform update with empty message.\");\n \n \tif (read_stdin) {\n-\t\tint ret;\n \t\ttransaction = ref_transaction_begin();\n-\n \t\tif (delete || no_deref || argc > 0)\n \t\t\tusage_with_options(git_update_ref_usage, options);\n \t\tif (end_null)\n \t\t\tline_termination = '\\0';\n \t\tupdate_refs_stdin();\n-\t\tret = ref_transaction_commit(transaction, msg, NULL,\n-\t\t\t\t\t UPDATE_REFS_DIE_ON_ERR);\n-\t\treturn ret;\n+\t\tif (ref_transaction_commit(transaction, msg, &err,\n+\t\t\t\t\t UPDATE_REFS_QUIET_ON_ERR))\n+\t\t\tdie(\"%s\", err.buf);\n+\t\treturn 0;\n \t}\n \n \tif (end_null)\n-- \n1.9.1.528.g98b8868.dirty\n"},{"id":"240061","messageId":"1398725682-30782-7-git-send-email-sahlberg@google.com","threadId":"36530","inReplyTo":"1398725682-30782-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 06/27] refs.c: make update_ref_write update a strbuf on failure","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T22:54:21Z","receivedAt":"2014-04-28T22:54:21Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change update_ref_write to also update an error strbuf on failure.\nThis makes the error available to ref_transaction_commit callers if the\ntransaction failed due to update_ref_sha1/write_ref_sha1 failures.\n\nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 21a147b..5e06fd9 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3253,10 +3253,13 @@ static struct ref_lock *update_ref_lock(const char *refname,\n \n static int update_ref_write(const char *action, const char *refname,\n \t\t\t const unsigned char *sha1, struct ref_lock *lock,\n-\t\t\t enum action_on_err onerr)\n+\t\t\t struct strbuf *err, enum action_on_err onerr)\n {\n \tif (write_ref_sha1(lock, sha1, action) < 0) {\n \t\tconst char *str = \"Cannot update the ref '%s'.\";\n+\t\tif (err)\n+\t\t\tstrbuf_addf(err, str, refname);\n+\n \t\tswitch (onerr) {\n \t\tcase UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;\n \t\tcase UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;\n@@ -3382,7 +3385,7 @@ int update_ref(const char *action, const char *refname,\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+\treturn update_ref_write(action, refname, sha1, lock, NULL, onerr);\n }\n \n static int ref_update_compare(const void *r1, const void *r2)\n@@ -3464,7 +3467,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\tret = update_ref_write(msg,\n \t\t\t\t\t update->refname,\n \t\t\t\t\t update->new_sha1,\n-\t\t\t\t\t update->lock, onerr);\n+\t\t\t\t\t update->lock, err, onerr);\n \t\t\tupdate->lock = NULL; /* freed by update_ref_write */\n \t\t\tif (ret)\n \t\t\t\tgoto cleanup;\n-- \n1.9.1.528.g98b8868.dirty\n"},{"id":"240059","messageId":"1398725682-30782-8-git-send-email-sahlberg@google.com","threadId":"36530","inReplyTo":"1398725682-30782-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 07/27] refs.c: remove the onerr argument to ref_transaction_commit","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T22:54:22Z","receivedAt":"2014-04-28T22:54:22Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Since all callers now use QUIET_ON_ERR we no longer need to provide an onerr\nargument any more. Remove the onerr argument from the ref_transaction_commit\nsignature.\n\nSigned-off-by: Ronnie Sahlberg \n---\n builtin/update-ref.c | 3 +--\n refs.c | 22 +++++++---------------\n refs.h | 3 +--\n 3 files changed, 9 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 207e24d..2bef2a0 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -366,8 +366,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \t\tif (end_null)\n \t\t\tline_termination = '\\0';\n \t\tupdate_refs_stdin();\n-\t\tif (ref_transaction_commit(transaction, msg, &err,\n-\t\t\t\t\t UPDATE_REFS_QUIET_ON_ERR))\n+\t\tif (ref_transaction_commit(transaction, msg, &err))\n \t\t\tdie(\"%s\", err.buf);\n \t\treturn 0;\n \t}\ndiff --git a/refs.c b/refs.c\nindex 5e06fd9..308e13e 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3396,8 +3396,7 @@ static int ref_update_compare(const void *r1, const void *r2)\n }\n \n static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n-\t\t\t\t\tstruct strbuf *err,\n-\t\t\t\t\tenum action_on_err onerr)\n+\t\t\t\t\tstruct strbuf *err)\n {\n \tint i;\n \tfor (i = 1; i < n; i++)\n@@ -3407,22 +3406,13 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n \t\t\tif (err)\n \t\t\t\tstrbuf_addf(err, str, updates[i]->refname);\n \n-\t\t\tswitch (onerr) {\n-\t\t\tcase UPDATE_REFS_MSG_ON_ERR:\n-\t\t\t\terror(str, updates[i]->refname); break;\n-\t\t\tcase UPDATE_REFS_DIE_ON_ERR:\n-\t\t\t\tdie(str, updates[i]->refname); break;\n-\t\t\tcase UPDATE_REFS_QUIET_ON_ERR:\n-\t\t\t\tbreak;\n-\t\t\t}\n \t\t\treturn 1;\n \t\t}\n \treturn 0;\n }\n \n int ref_transaction_commit(struct ref_transaction *transaction,\n-\t\t\t const char *msg, struct strbuf *err,\n-\t\t\t enum action_on_err onerr)\n+\t\t\t const char *msg, struct strbuf *err)\n {\n \tint ret = 0, delnum = 0, i;\n \tconst char **delnames;\n@@ -3437,7 +3427,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \n \t/* Copy, sort, and reject duplicate refs */\n \tqsort(updates, n, sizeof(*updates), ref_update_compare);\n-\tret = ref_update_reject_duplicates(updates, n, err, onerr);\n+\tret = ref_update_reject_duplicates(updates, n, err);\n \tif (ret)\n \t\tgoto cleanup;\n \n@@ -3449,7 +3439,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t\t (update->have_old ?\n \t\t\t\t\t\tupdate->old_sha1 : NULL),\n \t\t\t\t\t update->flags,\n-\t\t\t\t\t &update->type, onerr);\n+\t\t\t\t\t &update->type,\n+\t\t\t\t\t UPDATE_REFS_QUIET_ON_ERR);\n \t\tif (!update->lock) {\n \t\t\tif (err)\n \t\t\t\tstrbuf_addf(err ,\"Cannot lock the ref '%s'.\",\n@@ -3467,7 +3458,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\tret = update_ref_write(msg,\n \t\t\t\t\t update->refname,\n \t\t\t\t\t update->new_sha1,\n-\t\t\t\t\t update->lock, err, onerr);\n+\t\t\t\t\t update->lock, err,\n+\t\t\t\t\t UPDATE_REFS_QUIET_ON_ERR);\n \t\t\tupdate->lock = NULL; /* freed by update_ref_write */\n \t\t\tif (ret)\n \t\t\t\tgoto cleanup;\ndiff --git a/refs.h b/refs.h\nindex ff87e14..bc7715e 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -272,8 +272,7 @@ void ref_transaction_delete(struct ref_transaction *transaction,\n * the transaction failed.\n */\n int ref_transaction_commit(struct ref_transaction *transaction,\n-\t\t\t const char *msg, struct strbuf *err,\n-\t\t\t enum action_on_err onerr);\n+\t\t\t const char *msg, struct strbuf *err);\n \n /** Lock a ref and then write its file */\n int update_ref(const char *action, const char *refname,\n-- \n1.9.1.528.g98b8868.dirty\n"},{"id":"240071","messageId":"1398725682-30782-9-git-send-email-sahlberg@google.com","threadId":"36530","inReplyTo":"1398725682-30782-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 08/27] refs.c: change ref_transaction_update() to do error checking and return status","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T22:54:23Z","receivedAt":"2014-04-28T22:54:23Z","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 \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 2bef2a0..59c4d6b 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(\"update %s: failed\", 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 308e13e..1a903fb 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3333,19 +3333,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 bc7715e..0364a3e 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.528.g98b8868.dirty\n"},{"id":"240060","messageId":"1398725682-30782-10-git-send-email-sahlberg@google.com","threadId":"36530","inReplyTo":"1398725682-30782-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 09/27] refs.c: change ref_transaction_create to do error checking and return status","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T22:54:24Z","receivedAt":"2014-04-28T22:54:24Z","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 \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 59c4d6b..3fab810 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 1a903fb..27db737 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3353,18 +3353,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 0364a3e..01d773c 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.528.g98b8868.dirty\n"},{"id":"240077","messageId":"1398725682-30782-11-git-send-email-sahlberg@google.com","threadId":"36530","inReplyTo":"1398725682-30782-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 10/27] refs.c: ref_transaction_delete to check for error and return status","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T22:54:25Z","receivedAt":"2014-04-28T22:54:25Z","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 \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 3fab810..fc3512f 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 27db737..0a4e28e 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3372,19 +3372,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 01d773c..5eb500e 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.528.g98b8868.dirty\n"},{"id":"240076","messageId":"1398725682-30782-12-git-send-email-sahlberg@google.com","threadId":"36530","inReplyTo":"1398725682-30782-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 11/27] tag.c: use ref transactions when doing updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T22:54:26Z","receivedAt":"2014-04-28T22:54:26Z","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 \n---\n builtin/tag.c | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 40356e3..31e32cb 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,8 @@ 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 strbuf err = STRBUF_INIT;\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 +642,12 @@ 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 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, &err))\n+\t\tdie(_(\"%s: cannot update the ref: %s\"), ref.buf, err.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.528.g98b8868.dirty\n"},{"id":"240078","messageId":"1398725682-30782-13-git-send-email-sahlberg@google.com","threadId":"36530","inReplyTo":"1398725682-30782-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 12/27] replace.c: use the ref transaction functions for updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T22:54:27Z","receivedAt":"2014-04-28T22:54:27Z","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 \n---\n builtin/replace.c | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex b62420a..b037b29 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -129,7 +129,8 @@ 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+\tstruct strbuf err = STRBUF_INIT;\n \n \tif (get_sha1(object_ref, object))\n \t\tdie(\"Failed to resolve '%s' as a valid ref.\", object_ref);\n@@ -157,11 +158,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, &err))\n+\t\tdie(_(\"%s: failed to replace ref: %s\"), ref, err.buf);\n \n \treturn 0;\n }\n-- \n1.9.1.528.g98b8868.dirty\n"},{"id":"240075","messageId":"1398725682-30782-14-git-send-email-sahlberg@google.com","threadId":"36530","inReplyTo":"1398725682-30782-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 13/27] commit.c: use ref transactions for updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T22:54:28Z","receivedAt":"2014-04-28T22:54:28Z","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 \n---\n builtin/commit.c | 23 ++++++++++-------------\n 1 file changed, 10 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex d9550c5..a56f47a 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1541,11 +1541,12 @@ 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+\tstruct strbuf err = STRBUF_INIT;\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage_with_options(builtin_commit_usage, builtin_commit_options);\n@@ -1667,12 +1668,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,13 +1676,15 @@ 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-\t\trollback_index_files();\n-\t\tdie(_(\"cannot lock HEAD ref\"));\n-\t}\n-\tif (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {\n+\ttransaction = ref_transaction_begin();\n+\tif (!transaction ||\n+\t 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 ref_transaction_commit(transaction, sb.buf, &err)) {\n \t\trollback_index_files();\n-\t\tdie(_(\"cannot update HEAD ref\"));\n+\t\tdie(_(\"HEAD: cannot update ref: %s\"), err.buf);\n \t}\n \n \tunlink(git_path(\"CHERRY_PICK_HEAD\"));\n-- \n1.9.1.528.g98b8868.dirty\n"},{"id":"240074","messageId":"1398725682-30782-15-git-send-email-sahlberg@google.com","threadId":"36530","inReplyTo":"1398725682-30782-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 14/27] sequencer.c: use ref transactions for all ref updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T22:54:29Z","receivedAt":"2014-04-28T22:54:29Z","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 \n---\n sequencer.c | 23 +++++++++++++++++------\n 1 file changed, 17 insertions(+), 6 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex bde5f04..449e093 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -272,19 +272,30 @@ 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+\tstruct strbuf err = STRBUF_INIT;\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 \tstrbuf_addf(&sb, \"%s: fast-forward\", action_name(opts));\n-\tret = write_ref_sha1(ref_lock, to, sb.buf);\n+\n+\ttransaction = ref_transaction_begin();\n+\tif ((!transaction ||\n+\t ref_transaction_update(transaction, \"HEAD\", to, from,\n+\t\t\t\t 0, !unborn)) ||\n+\t (ref_transaction_commit(transaction, sb.buf, &err) &&\n+\t !(transaction = NULL))) {\n+\t\tref_transaction_rollback(transaction);\n+\t\tstrbuf_release(&sb);\n+\t\treturn error(_(\"HEAD: Could not fast-forward: %s\\n\"),\n+\t\t\t strbuf_detach(&err, NULL));\n+\t}\n+\n \tstrbuf_release(&sb);\n-\treturn ret;\n+\treturn 0;\n }\n \n static int do_recursive_merge(struct commit *base, struct commit *next,\n-- \n1.9.1.528.g98b8868.dirty\n"},{"id":"240073","messageId":"1398725682-30782-16-git-send-email-sahlberg@google.com","threadId":"36530","inReplyTo":"1398725682-30782-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 15/27] fast-import.c: change update_branch to use ref transactions","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T22:54:30Z","receivedAt":"2014-04-28T22:54:30Z","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 \n---\n fast-import.c | 20 ++++++++++++--------\n 1 file changed, 12 insertions(+), 8 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex fb4738d..300c8dc 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1678,36 +1678,40 @@ 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+\tstruct strbuf err = STRBUF_INIT;\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 \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\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\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+\ttransaction = ref_transaction_begin();\n+\tif ((!transaction ||\n+\t ref_transaction_update(transaction, b->name, b->sha1, old_sha1,\n+\t\t\t\t 0, 1)) ||\n+\t (ref_transaction_commit(transaction, msg, &err) &&\n+\t !(transaction = NULL))) {\n+\t\tref_transaction_rollback(transaction);\n+\t\treturn error(\"Unable to update branch %s: %s\", b->name,\n+\t\t\t strbuf_detach(&err, NULL));\n+\t}\n \treturn 0;\n }\n \n-- \n1.9.1.528.g98b8868.dirty\n"},{"id":"240067","messageId":"1398725682-30782-17-git-send-email-sahlberg@google.com","threadId":"36530","inReplyTo":"1398725682-30782-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 16/27] branch.c: use ref transaction for all ref updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T22:54:31Z","receivedAt":"2014-04-28T22:54:31Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change create_branch to use a ref transaction when creating the new branch.\nref_transaction_create will check that the ref does not already exist and fail\notherwise meaning that we no longer need to keep a lock on the ref during the\nsetup_tracking. This simplifies the code since we can now do the transaction\nin one single step.\n\nIf the forcing flag is false then use ref_transaction_create since this will\nfail if the ref already exist. Otherwise use ref_transaction_update.\n\nThis also fixes a race condition in the old code where two concurrent\ncreate_branch could race since the lock_any_ref_for_update/write_ref_sha1\ndid not protect against the ref already existsing. I.e. one thread could end up\noverwriting a branch even if the forcing flag is false.\n\nSigned-off-by: Ronnie Sahlberg \n---\n branch.c | 30 ++++++++++++++++--------------\n 1 file changed, 16 insertions(+), 14 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 660097b..2aa5c82 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -226,7 +226,6 @@ 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 commit *commit;\n \tunsigned char sha1[20];\n \tchar *real_ref, msg[PATH_MAX + 20];\n@@ -285,15 +284,6 @@ void create_branch(const char *head,\n \t\tdie(_(\"Not a valid branch point: '%s'.\"), start_name);\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}\n-\n-\tif (reflog)\n-\t\tlog_all_ref_updates = 1;\n-\n \tif (forcing)\n \t\tsnprintf(msg, sizeof msg, \"branch: Reset to %s\",\n \t\t\t start_name);\n@@ -301,13 +291,25 @@ void create_branch(const char *head,\n \t\tsnprintf(msg, sizeof msg, \"branch: Created from %s\",\n \t\t\t start_name);\n \n+\tif (reflog)\n+\t\tlog_all_ref_updates = 1;\n+\n+\tif (!dont_change_ref) {\n+\t\tstruct ref_transaction *transaction;\n+\t\tstruct strbuf err = STRBUF_INIT;\n+\n+\t\ttransaction = ref_transaction_begin();\n+\t\tif (!transaction ||\n+\t\t\tref_transaction_update(transaction, ref.buf, sha1,\n+\t\t\t\t\t null_sha1, 0, !forcing) ||\n+\t\t\tref_transaction_commit(transaction, msg, &err))\n+\t\t\t\tdie_errno(_(\"%s: failed to write ref: %s\"),\n+\t\t\t\t\t ref.buf, err.buf);\n+\t}\n+\n \tif (real_ref && track)\n \t\tsetup_tracking(ref.buf + 11, real_ref, track, quiet);\n \n-\tif (!dont_change_ref)\n-\t\tif (write_ref_sha1(lock, sha1, msg) < 0)\n-\t\t\tdie_errno(_(\"Failed to write ref\"));\n-\n \tstrbuf_release(&ref);\n \tfree(real_ref);\n }\n-- \n1.9.1.528.g98b8868.dirty\n"},{"id":"240068","messageId":"1398725682-30782-18-git-send-email-sahlberg@google.com","threadId":"36530","inReplyTo":"1398725682-30782-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 17/27] refs.c: change update_ref to use a transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T22:54:32Z","receivedAt":"2014-04-28T22:54:32Z","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 \n---\n refs.c | 25 +++++++++++++++++++++----\n 1 file changed, 21 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 0a4e28e..563f336 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3396,11 +3396,28 @@ 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+\tstruct ref_transaction *t;\n+\tstruct strbuf err = STRBUF_INIT;\n+\n+\tt = ref_transaction_begin();\n+\tif ((!t ||\n+\t ref_transaction_update(t, refname, sha1, oldval, flags,\n+\t\t\t\t !!oldval)) ||\n+\t (ref_transaction_commit(t, action, &err) && !(t = NULL))) {\n+\t\tconst char *str = \"update_ref failed for ref '%s': %s\";\n+\n+\t\tref_transaction_rollback(t);\n+\t\tswitch (onerr) {\n+\t\tcase UPDATE_REFS_MSG_ON_ERR:\n+\t\t\terror(str, refname, err.buf); break;\n+\t\tcase UPDATE_REFS_DIE_ON_ERR:\n+\t\t\tdie(str, refname, err.buf); break;\n+\t\tcase UPDATE_REFS_QUIET_ON_ERR: break;\n+\t\t}\n+\t\tstrbuf_release(&err);\n \t\treturn 1;\n-\treturn update_ref_write(action, refname, sha1, lock, NULL, onerr);\n+\t}\n+\treturn 0;\n }\n \n static int ref_update_compare(const void *r1, const void *r2)\n-- \n1.9.1.528.g98b8868.dirty\n"},{"id":"240069","messageId":"1398725682-30782-19-git-send-email-sahlberg@google.com","threadId":"36530","inReplyTo":"1398725682-30782-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 18/27] refs.c: free the transaction before returning when number of updates is 0","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T22:54:33Z","receivedAt":"2014-04-28T22:54:33Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"We have to free the transaction before returning in the early check for\n'return early if number of updates == 0' or else the following code would\ncreate a memory leak with the transaction never being freed :\n t = ref_transaction_begin()\n ref_transaction_commit(t)\n\nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 563f336..c6ee43c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3451,8 +3451,10 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \tint n = transaction->nr;\n \tstruct ref_update **updates = transaction->updates;\n \n-\tif (!n)\n+\tif (!n) {\n+\t\tref_transaction_free(transaction);\n \t\treturn 0;\n+\t}\n \n \t/* Allocate work space */\n \tdelnames = xmalloc(sizeof(*delnames) * n);\n-- \n1.9.1.528.g98b8868.dirty\n"},{"id":"240070","messageId":"1398725682-30782-20-git-send-email-sahlberg@google.com","threadId":"36530","inReplyTo":"1398725682-30782-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 19/27] refs.c: ref_transaction_commit should not free the transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T22:54:34Z","receivedAt":"2014-04-28T22:54:34Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change ref_transaction_commit so that it does not free the transaction.\nInstead require that a caller will end a transaction by either calling\nref_transaction_rollback or ref_transaction_free.\n\nBy having the transaction object remaining valid after _commit returns allows\nus to write much nicer code and still be able to call ref_transaction_rollback\nsafely. Instead of this horribleness\n\tt = ref_transaction_begin();\n\tif ((!t ||\n\t ref_transaction_update(t, refname, sha1, oldval, flags,\n\t\t\t\t !!oldval)) ||\n\t (ref_transaction_commit(t, action, &err) && !(t = NULL))) {\n\t\tref_transaction_rollback(t);\n\nwe can now just do the much nicer\n\tt = ref_transaction_begin();\n\tif (!t ||\n\t ref_transaction_update(t, refname, sha1, oldval, flags,\n\t\t\t\t !!oldval) ||\n\t ref_transaction_commit(&t, action, &err)) {\n\t\tref_transaction_rollback(t);\n\t\t... die/return ...\n\n\tref_transaction_free(transaction);\n\nSigned-off-by: Ronnie Sahlberg \n---\n branch.c | 1 +\n builtin/commit.c | 1 +\n builtin/replace.c | 1 +\n builtin/tag.c | 1 +\n builtin/update-ref.c | 1 +\n fast-import.c | 8 ++++----\n refs.c | 7 ++-----\n refs.h | 14 +++++++++-----\n sequencer.c | 8 ++++----\n 9 files changed, 24 insertions(+), 18 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 2aa5c82..8e58908 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -305,6 +305,7 @@ void create_branch(const char *head,\n \t\t\tref_transaction_commit(transaction, msg, &err))\n \t\t\t\tdie_errno(_(\"%s: failed to write ref: %s\"),\n \t\t\t\t\t ref.buf, err.buf);\n+\t\tref_transaction_free(transaction);\n \t}\n \n \tif (real_ref && track)\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex a56f47a..b806c59 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1686,6 +1686,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\trollback_index_files();\n \t\tdie(_(\"HEAD: cannot update ref: %s\"), err.buf);\n \t}\n+\tref_transaction_free(transaction);\n \n \tunlink(git_path(\"CHERRY_PICK_HEAD\"));\n \tunlink(git_path(\"REVERT_HEAD\"));\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex b037b29..5999f7d 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -165,6 +165,7 @@ static int replace_object(const char *object_ref, const char *replace_ref,\n \t ref_transaction_commit(transaction, NULL, &err))\n \t\tdie(_(\"%s: failed to replace ref: %s\"), ref, err.buf);\n \n+\tref_transaction_free(transaction);\n \treturn 0;\n }\n \ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 31e32cb..4701bda 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -648,6 +648,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t\t\t 0, !is_null_sha1(prev)) ||\n \t ref_transaction_commit(transaction, NULL, &err))\n \t\tdie(_(\"%s: cannot update the ref: %s\"), ref.buf, err.buf);\n+\tref_transaction_free(transaction);\n \tif (force && !is_null_sha1(prev) && hashcmp(prev, object))\n \t\tprintf(_(\"Updated tag '%s' (was %s)\\n\"), tag, find_unique_abbrev(prev, DEFAULT_ABBREV));\n \ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex fc3512f..7fe9c11 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -373,6 +373,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \t\tupdate_refs_stdin();\n \t\tif (ref_transaction_commit(transaction, msg, &err))\n \t\t\tdie(\"%s\", err.buf);\n+\t\tref_transaction_free(transaction);\n \t\treturn 0;\n \t}\n \ndiff --git a/fast-import.c b/fast-import.c\nindex 300c8dc..ab36de5 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1703,15 +1703,15 @@ static int update_branch(struct branch *b)\n \t\t}\n \t}\n \ttransaction = ref_transaction_begin();\n-\tif ((!transaction ||\n+\tif (!transaction ||\n \t ref_transaction_update(transaction, b->name, b->sha1, old_sha1,\n-\t\t\t\t 0, 1)) ||\n-\t (ref_transaction_commit(transaction, msg, &err) &&\n-\t !(transaction = NULL))) {\n+\t\t\t\t 0, 1) ||\n+\t ref_transaction_commit(transaction, msg, &err)) {\n \t\tref_transaction_rollback(transaction);\n \t\treturn error(\"Unable to update branch %s: %s\", b->name,\n \t\t\t strbuf_detach(&err, NULL));\n \t}\n+\tref_transaction_free(transaction);\n \treturn 0;\n }\n \ndiff --git a/refs.c b/refs.c\nindex c6ee43c..add95ba 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3302,7 +3302,7 @@ struct ref_transaction *ref_transaction_begin(void)\n \treturn xcalloc(1, sizeof(struct ref_transaction));\n }\n \n-static void ref_transaction_free(struct ref_transaction *transaction)\n+void ref_transaction_free(struct ref_transaction *transaction)\n {\n \tint i;\n \n@@ -3451,10 +3451,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \tint n = transaction->nr;\n \tstruct ref_update **updates = transaction->updates;\n \n-\tif (!n) {\n-\t\tref_transaction_free(transaction);\n+\tif (!n)\n \t\treturn 0;\n-\t}\n \n \t/* Allocate work space */\n \tdelnames = xmalloc(sizeof(*delnames) * n);\n@@ -3520,7 +3518,6 @@ cleanup:\n \t\tif (updates[i]->lock)\n \t\t\tunlock_ref(updates[i]->lock);\n \tfree(delnames);\n-\tref_transaction_free(transaction);\n \treturn ret;\n }\n \ndiff --git a/refs.h b/refs.h\nindex 5eb500e..fcde43e 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -210,8 +210,8 @@ enum action_on_err {\n \n /*\n * Begin a reference transaction. The reference transaction must\n- * eventually be commited using ref_transaction_commit() or rolled\n- * back using ref_transaction_rollback().\n+ * eventually be freed by either calling ref_transaction_rollback()\n+ * or ref_transaction_free().\n */\n struct ref_transaction *ref_transaction_begin(void);\n \n@@ -267,13 +267,17 @@ int ref_transaction_delete(struct ref_transaction *transaction,\n /*\n * Commit all of the changes that have been queued in transaction, as\n * atomically as possible. Return a nonzero value if there is a\n- * problem. The ref_transaction is freed by this function.\n- * If err is non-NULL we will add an error string to it to explain why\n- * the transaction failed.\n+ * problem. If err is non-NULL we will add an error string to it to explain\n+ * why the transaction failed.\n */\n int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t const char *msg, struct strbuf *err);\n \n+/*\n+ * Free an existing transaction.\n+ */\n+void ref_transaction_free(struct ref_transaction *transaction);\n+\n /** Lock a ref and then write its file */\n int update_ref(const char *action, const char *refname,\n \t\tconst unsigned char *sha1, const unsigned char *oldval,\ndiff --git a/sequencer.c b/sequencer.c\nindex 449e093..5c424be 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -283,16 +283,16 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n \tstrbuf_addf(&sb, \"%s: fast-forward\", action_name(opts));\n \n \ttransaction = ref_transaction_begin();\n-\tif ((!transaction ||\n+\tif (!transaction ||\n \t ref_transaction_update(transaction, \"HEAD\", to, from,\n-\t\t\t\t 0, !unborn)) ||\n-\t (ref_transaction_commit(transaction, sb.buf, &err) &&\n-\t !(transaction = NULL))) {\n+\t\t\t\t 0, !unborn) ||\n+\t ref_transaction_commit(transaction, sb.buf, &err)) {\n \t\tref_transaction_rollback(transaction);\n \t\tstrbuf_release(&sb);\n \t\treturn error(_(\"HEAD: Could not fast-forward: %s\\n\"),\n \t\t\t strbuf_detach(&err, NULL));\n \t}\n+\tref_transaction_free(transaction);\n \n \tstrbuf_release(&sb);\n \treturn 0;\n-- \n1.9.1.528.g98b8868.dirty\n"},{"id":"240083","messageId":"1398725682-30782-21-git-send-email-sahlberg@google.com","threadId":"36530","inReplyTo":"1398725682-30782-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 20/27] fetch.c: clear errno before calling functions that might set it","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T22:54:35Z","receivedAt":"2014-04-28T22:54:35Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"In s_update_ref there are two calls that when they fail we return an error\nbased on the errno value. In particular we want to return a specific error\nif ENOTDIR happened. Both these functions do have failure modes where they\nmay return an error without updating errno, in which case a previous and\nunrelated ENOTDIT may cause us to return the wrong error. Clear errno before\ncalling any functions if we check errno afterwards.\n\nAlso skip initializing a static variable to 0. Statics live in .bss and\nare all automatically initialized to 0.\n\nSigned-off-by: Ronnie Sahlberg \n---\n builtin/fetch.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 55f457c..a93c893 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -44,7 +44,7 @@ static struct transport *gtransport;\n static struct transport *gsecondary;\n static const char *submodule_prefix = \"\";\n static const char *recurse_submodules_default;\n-static int shown_url = 0;\n+static int shown_url;\n \n static int option_parse_recurse_submodules(const struct option *opt,\n \t\t\t\t const char *arg, int unset)\n@@ -382,6 +382,8 @@ static int s_update_ref(const char *action,\n \tif (!rla)\n \t\trla = default_rla.buf;\n \tsnprintf(msg, sizeof(msg), \"%s: %s\", rla, action);\n+\n+\terrno = 0;\n \tlock = lock_any_ref_for_update(ref->name,\n \t\t\t\t check_old ? ref->old_sha1 : NULL,\n \t\t\t\t 0, NULL);\n-- \n1.9.1.528.g98b8868.dirty\n"},{"id":"240062","messageId":"1398725682-30782-22-git-send-email-sahlberg@google.com","threadId":"36530","inReplyTo":"1398725682-30782-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 21/27] fetch.c: change s_update_ref to use a ref transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T22:54:36Z","receivedAt":"2014-04-28T22:54:36Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change s_update_ref to use a ref transaction for the ref update.\n\nSigned-off-by: Ronnie Sahlberg \n---\n builtin/fetch.c | 17 +++++++++--------\n 1 file changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex a93c893..b41d7b7 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -375,7 +375,7 @@ static int s_update_ref(const char *action,\n {\n \tchar msg[1024];\n \tchar *rla = getenv(\"GIT_REFLOG_ACTION\");\n-\tstatic struct ref_lock *lock;\n+\tstruct ref_transaction *transaction;\n \n \tif (dry_run)\n \t\treturn 0;\n@@ -384,15 +384,16 @@ static int s_update_ref(const char *action,\n \tsnprintf(msg, sizeof(msg), \"%s: %s\", rla, action);\n \n \terrno = 0;\n-\tlock = lock_any_ref_for_update(ref->name,\n-\t\t\t\t check_old ? ref->old_sha1 : NULL,\n-\t\t\t\t 0, NULL);\n-\tif (!lock)\n-\t\treturn errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :\n-\t\t\t\t\t STORE_REF_ERROR_OTHER;\n-\tif (write_ref_sha1(lock, ref->new_sha1, msg) < 0)\n+\ttransaction = ref_transaction_begin();\n+\tif (!transaction ||\n+\t ref_transaction_update(transaction, ref->name, ref->new_sha1,\n+\t\t\t\t ref->old_sha1, 0, check_old) ||\n+\t ref_transaction_commit(transaction, msg, NULL)) {\n+\t\tref_transaction_rollback(transaction);\n \t\treturn errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :\n \t\t\t\t\t STORE_REF_ERROR_OTHER;\n+\t}\n+\tref_transaction_free(transaction);\n \treturn 0;\n }\n \n-- \n1.9.1.528.g98b8868.dirty\n"},{"id":"240066","messageId":"1398725682-30782-23-git-send-email-sahlberg@google.com","threadId":"36530","inReplyTo":"1398725682-30782-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 22/27] fetch.c: use a single ref transaction for all ref updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T22:54:37Z","receivedAt":"2014-04-28T22:54:37Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change store_updated_refs to use a single ref transaction for all refs that\nare updated during the fetch. This makes the fetch more atomic when update\nfailures occur.\n\nSince ref update failures will now no longer occur in the code path for\nupdating a single ref in s_update_ref, we no longer have as detailed error\nmessage logging the exact reference and the ref log action as in the old cod\nInstead since we fail the entire transaction we log a much more generic\nmessage. But since we commit the transaction using MSG_ON_ERR we will log\nan error containing the ref name if either locking of writing the ref would\nso the regression in the log message is minor.\n\nThis will also change the order in which errors are checked for and logged\nwhich may alter which error will be logged if there are multiple errors\noccuring during a fetch.\n\nFor example, assume we have a fetch for two refs that both would fail.\nWhere the first ref would fail with ENOTDIR due to a directory in the ref\npath not existing, and the second ref in the fetch would fail due to\nthe check in update_logical_ref():\n if (current_branch &&\n !strcmp(ref->name, current_branch->name) &&\n !(update_head_ok || is_bare_repository()) &&\n !is_null_sha1(ref->old_sha1)) {\n /*\n * If this is the head, and it's not okay to update\n * the head, and the old value of the head isn't empty...\n */\n\nIn the old code since we would update the refs one ref at a time we would\nfirst fail the ENOTDIR and then fail the second update of HEAD as well.\nBut since the first ref failed with ENOTDIR we would eventually fail the who\nfetch with STORE_REF_ERROR_DF_CONFLICT\n\nIn the new code, since we defer committing the transaction until all refs\nhave been processed, we would now detect that the second ref was bad and\nrollback the transaction before we would even try start writing the update t\ndisk and thus we would not return STORE_REF_ERROR_DF_CONFLICT for this case.\n\nI think this new behaviour is more correct, since if there was a problem\nwe would not even try to commit the transaction but need to highlight this\nchange in how/what errors are reported.\nThis change in what error is returned only occurs if there are multiple\nrefs that fail to update and only some, but not all, of them fail due to\nENOTDIR.\n\nSigned-off-by: Ronnie Sahlberg \n---\n builtin/fetch.c | 35 +++++++++++++++++------------------\n 1 file changed, 17 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b41d7b7..8f974a2 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -45,6 +45,7 @@ static struct transport *gsecondary;\n static const char *submodule_prefix = \"\";\n static const char *recurse_submodules_default;\n static int shown_url;\n+struct ref_transaction *transaction;\n \n static int option_parse_recurse_submodules(const struct option *opt,\n \t\t\t\t const char *arg, int unset)\n@@ -373,27 +374,13 @@ static int s_update_ref(const char *action,\n \t\t\tstruct ref *ref,\n \t\t\tint check_old)\n {\n-\tchar msg[1024];\n-\tchar *rla = getenv(\"GIT_REFLOG_ACTION\");\n-\tstruct ref_transaction *transaction;\n-\n \tif (dry_run)\n \t\treturn 0;\n-\tif (!rla)\n-\t\trla = default_rla.buf;\n-\tsnprintf(msg, sizeof(msg), \"%s: %s\", rla, action);\n \n-\terrno = 0;\n-\ttransaction = ref_transaction_begin();\n-\tif (!transaction ||\n-\t ref_transaction_update(transaction, ref->name, ref->new_sha1,\n-\t\t\t\t ref->old_sha1, 0, check_old) ||\n-\t ref_transaction_commit(transaction, msg, NULL)) {\n-\t\tref_transaction_rollback(transaction);\n-\t\treturn errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :\n-\t\t\t\t\t STORE_REF_ERROR_OTHER;\n-\t}\n-\tref_transaction_free(transaction);\n+\tif (ref_transaction_update(transaction, ref->name, ref->new_sha1,\n+\t\t\t ref->old_sha1, 0, check_old))\n+\t\treturn STORE_REF_ERROR_OTHER;\n+\n \treturn 0;\n }\n \n@@ -565,6 +552,13 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\tgoto abort;\n \t}\n \n+\terrno = 0;\n+\ttransaction = ref_transaction_begin();\n+\tif (!transaction) {\n+\t\trc = error(_(\"cannot start ref transaction\\n\"));\n+\t\tgoto abort;\n+\t}\n+\n \t/*\n \t * We do a pass for each fetch_head_status type in their enum order, so\n \t * merged entries are written before not-for-merge. That lets readers\n@@ -676,6 +670,11 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t}\n \t\t}\n \t}\n+\tif (ref_transaction_commit(transaction, \"fetch_ref transaction\",\n+\t\t\t\t UPDATE_REFS_MSG_ON_ERR))\n+\t\trc |= errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :\n+\t\t STORE_REF_ERROR_OTHER;\n+\tref_transaction_free(transaction);\n \n \tif (rc & STORE_REF_ERROR_DF_CONFLICT)\n \t\terror(_(\"some local refs could not be updated; try running\\n\"\n-- \n1.9.1.528.g98b8868.dirty\n"},{"id":"240072","messageId":"1398725682-30782-24-git-send-email-sahlberg@google.com","threadId":"36530","inReplyTo":"1398725682-30782-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 23/27] receive-pack.c: use a reference transaction for updating the refs","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T22:54:38Z","receivedAt":"2014-04-28T22:54:38Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Wrap all the ref updates inside a transaction to make the update atomic.\n\nSigned-off-by: Ronnie Sahlberg \n---\n builtin/receive-pack.c | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex c323081..885aef4 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -46,6 +46,8 @@ static void *head_name_to_free;\n static int sent_capabilities;\n static int shallow_update;\n static const char *alt_shallow_file;\n+struct strbuf err = STRBUF_INIT;\n+struct ref_transaction *transaction;\n \n static enum deny_action parse_deny_action(const char *var, const char *value)\n {\n@@ -475,7 +477,6 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \tconst char *namespaced_name;\n \tunsigned char *old_sha1 = cmd->old_sha1;\n \tunsigned char *new_sha1 = cmd->new_sha1;\n-\tstruct ref_lock *lock;\n \n \t/* only refs/... are allowed */\n \tif (!starts_with(name, \"refs/\") || check_refname_format(name + 5, 0)) {\n@@ -580,15 +581,9 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\t update_shallow_ref(cmd, si))\n \t\t\treturn \"shallow error\";\n \n-\t\tlock = lock_any_ref_for_update(namespaced_name, old_sha1,\n-\t\t\t\t\t 0, NULL);\n-\t\tif (!lock) {\n-\t\t\trp_error(\"failed to lock %s\", name);\n-\t\t\treturn \"failed to lock\";\n-\t\t}\n-\t\tif (write_ref_sha1(lock, new_sha1, \"push\")) {\n-\t\t\treturn \"failed to write\"; /* error() already called */\n-\t\t}\n+\t\tif (ref_transaction_update(transaction, namespaced_name,\n+\t\t\t\t\t new_sha1, old_sha1, 0, 1))\n+\t\t\treturn \"failed to update\";\n \t\treturn NULL; /* good */\n \t}\n }\n@@ -812,6 +807,7 @@ static void execute_commands(struct command *commands,\n \thead_name = head_name_to_free = resolve_refdup(\"HEAD\", sha1, 0, NULL);\n \n \tchecked_connectivity = 1;\n+\ttransaction = ref_transaction_begin();\n \tfor (cmd = commands; cmd; cmd = cmd->next) {\n \t\tif (cmd->error_string)\n \t\t\tcontinue;\n@@ -827,6 +823,10 @@ static void execute_commands(struct command *commands,\n \t\t\tchecked_connectivity = 0;\n \t\t}\n \t}\n+\tif (ref_transaction_commit(transaction, \"push\", &err))\n+\t\terror(\"%s\", err.buf);\n+\tref_transaction_free(transaction);\n+\tstrbuf_reset(&err);\n \n \tif (shallow_update && !checked_connectivity)\n \t\terror(\"BUG: run 'git fsck' for safety.\\n\"\n-- \n1.9.1.528.g98b8868.dirty\n"},{"id":"240064","messageId":"1398725682-30782-25-git-send-email-sahlberg@google.com","threadId":"36530","inReplyTo":"1398725682-30782-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 24/27] fast-import.c: use a ref transaction when dumping tags","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T22:54:39Z","receivedAt":"2014-04-28T22:54:39Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Signed-off-by: Ronnie Sahlberg \n---\n fast-import.c | 17 ++++++++++++-----\n 1 file changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex ab36de5..b105f17 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1730,15 +1730,22 @@ static void dump_tags(void)\n {\n \tstatic const char *msg = \"fast-import\";\n \tstruct tag *t;\n-\tstruct ref_lock *lock;\n \tchar ref_name[PATH_MAX];\n+\tstruct strbuf err = STRBUF_INIT;\n+\tstruct ref_transaction *transaction;\n \n+\ttransaction = ref_transaction_begin();\n \tfor (t = first_tag; t; t = t->next_tag) {\n-\t\tsprintf(ref_name, \"tags/%s\", t->name);\n-\t\tlock = lock_ref_sha1(ref_name, NULL);\n-\t\tif (!lock || write_ref_sha1(lock, t->sha1, msg) < 0)\n-\t\t\tfailure |= error(\"Unable to update %s\", ref_name);\n+\t\tsprintf(ref_name, \"refs/tags/%s\", t->name);\n+\n+\t\tif (ref_transaction_update(transaction, ref_name, t->sha1,\n+\t\t\t\t\t NULL, 0, 0))\n+\t\t\tfailure |= error(\"Unable to update %s\", err.buf);\n \t}\n+\tif (failure || ref_transaction_commit(transaction, msg, &err))\n+\t\tfailure |= error(\"Unable to update %s\", err.buf);\n+\tref_transaction_free(transaction);\n+\tstrbuf_reset(&err);\n }\n \n static void dump_marks_helper(FILE *f,\n-- \n1.9.1.528.g98b8868.dirty\n"},{"id":"240084","messageId":"1398725682-30782-26-git-send-email-sahlberg@google.com","threadId":"36530","inReplyTo":"1398725682-30782-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 25/27] walker.c: use ref transaction for ref updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T22:54:40Z","receivedAt":"2014-04-28T22:54:40Z","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 \n---\n walker.c | 51 ++++++++++++++++++++++++++-------------------------\n 1 file changed, 26 insertions(+), 25 deletions(-)\n\ndiff --git a/walker.c b/walker.c\nindex 1dd86b8..382a63a 100644\n--- a/walker.c\n+++ b/walker.c\n@@ -251,24 +251,18 @@ 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+\tchar ref_name[PATH_MAX];\n+\tstruct strbuf err = STRBUF_INIT;\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 +270,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 +288,26 @@ 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\tsprintf(ref_name, \"refs/%s\", write_ref[i]);\n+\t\tif (ref_transaction_update(transaction, ref_name,\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+\n+\tif (ref_transaction_commit(transaction, msg ? msg : \"fetch (unknown)\",\n+\t\t\t\t &err)) {\n+\t\terror(\"%s\", err.buf);\n+\t\tgoto rollback_and_fail;\n \t}\n-\tfree(msg);\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+\tstrbuf_reset(&err);\n+\tref_transaction_free(transaction);\n \n \treturn -1;\n }\n-- \n1.9.1.528.g98b8868.dirty\n"},{"id":"240063","messageId":"1398725682-30782-27-git-send-email-sahlberg@google.com","threadId":"36530","inReplyTo":"1398725682-30782-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 26/27] refs.c: make write_ref_sha1 static","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T22:54:41Z","receivedAt":"2014-04-28T22:54:41Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"No external users call write_ref_sha1 any more so lets declare it static.\n\nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 4 +++-\n refs.h | 3 ---\n 2 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex add95ba..cb7d69f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -251,6 +251,8 @@ struct ref_entry {\n };\n \n static void read_loose_refs(const char *dirname, struct ref_dir *dir);\n+static int write_ref_sha1(struct ref_lock *lock,\n+\t\t\t const unsigned char *sha1, const char *logmsg);\n \n static struct ref_dir *get_ref_dir(struct ref_entry *entry)\n {\n@@ -2798,7 +2800,7 @@ static int is_branch(const char *refname)\n \treturn !strcmp(refname, \"HEAD\") || starts_with(refname, \"refs/heads/\");\n }\n \n-int write_ref_sha1(struct ref_lock *lock,\n+static int write_ref_sha1(struct ref_lock *lock,\n \tconst unsigned char *sha1, const char *logmsg)\n {\n \tstatic char term = '\\n';\ndiff --git a/refs.h b/refs.h\nindex fcde43e..bc8a95b 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -150,9 +150,6 @@ extern int commit_ref(struct ref_lock *lock);\n /** Release any lock taken but not written. **/\n extern void unlock_ref(struct ref_lock *lock);\n \n-/** Writes sha1 into the ref specified by the lock. **/\n-extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);\n-\n /** Setup reflog before using. **/\n int log_ref_setup(const char *refname, char *logfile, int bufsize);\n \n-- \n1.9.1.528.g98b8868.dirty\n"},{"id":"240065","messageId":"1398725682-30782-28-git-send-email-sahlberg@google.com","threadId":"36530","inReplyTo":"1398725682-30782-1-git-send-email-sahlberg@google.com","subject":"[PATCH v4 27/27] refs.c: make lock_ref_sha1 static","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T22:54:42Z","receivedAt":"2014-04-28T22:54:42Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"No external callers reference lock_ref_sha1 any more so lets declare it static.\n\nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 2 +-\n refs.h | 3 ---\n 2 files changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex cb7d69f..eebcacb 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2130,7 +2130,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \treturn NULL;\n }\n \n-struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char *old_sha1)\n+static struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char *old_sha1)\n {\n \tchar refpath[PATH_MAX];\n \tif (check_refname_format(refname, 0))\ndiff --git a/refs.h b/refs.h\nindex bc8a95b..b14fe86 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -132,9 +132,6 @@ extern int ref_exists(const char *);\n */\n extern int peel_ref(const char *refname, unsigned char *sha1);\n \n-/** Locks a \"refs/\" ref returning the lock on success and NULL on failure. **/\n-extern struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char *old_sha1);\n-\n /** Locks any ref (for 'HEAD' type refs). */\n #define REF_NODEREF\t0x01\n extern struct ref_lock *lock_any_ref_for_update(const char *refname,\n-- \n1.9.1.528.g98b8868.dirty\n"},{"id":"240096","messageId":"CAPig+cSWMK_kyVvaD8QCfZmPu4JVT+nOJZJLteHhDao0umHLbA@mail.gmail.com","threadId":"36530","inReplyTo":"1398725682-30782-13-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v4 12/27] replace.c: use the ref transaction functions for updates","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-04-29T00:44:56Z","receivedAt":"2014-04-29T00:44:56Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 28, 2014 at 6:54 PM, Ronnie Sahlberg wrote:\n> Update replace.c to use ref transactions for updates.\n>\n> Signed-off-by: Ronnie Sahlberg \n> ---\n> builtin/replace.c | 14 ++++++++------\n> 1 file changed, 8 insertions(+), 6 deletions(-)\n>\n> diff --git a/builtin/replace.c b/builtin/replace.c\n> index b62420a..b037b29 100644\n> --- a/builtin/replace.c\n> +++ b/builtin/replace.c\n> @@ -129,7 +129,8 @@ static int replace_object(const char *object_ref, const char *replace_ref,\n> unsigned char object[20], prev[20], repl[20];\n> enum object_type obj_type, repl_type;\n> char ref[PATH_MAX];\n> - struct ref_lock *lock;\n> + struct ref_transaction *transaction;\n> + struct strbuf err = STRBUF_INIT;\n>\n> if (get_sha1(object_ref, object))\n> die(\"Failed to resolve '%s' as a valid ref.\", object_ref);\n> @@ -157,11 +158,12 @@ static int replace_object(const char *object_ref, const char *replace_ref,\n> else if (!force)\n> die(\"replace ref '%s' already exists\", ref);\n>\n> - lock = lock_any_ref_for_update(ref, prev, 0, NULL);\n> - if (!lock)\n> - die(\"%s: cannot lock the ref\", ref);\n> - if (write_ref_sha1(lock, repl, NULL) < 0)\n> - die(\"%s: cannot update the ref\", ref);\n> + transaction = ref_transaction_begin();\n> + if (!transaction ||\n> + ref_transaction_update(transaction, ref, repl, prev,\n> + 0, !is_null_sha1(prev)) ||\n> + ref_transaction_commit(transaction, NULL, &err))\n> + die(_(\"%s: failed to replace ref: %s\"), ref, err.buf);\n\nEven though 'err' will be empty after this conditional, would\nstrbuf_release(&err) here be warranted to future-proof it? Today's\nimplementation of strbuf will not have allocated any memory, but\nperhaps a future change might pre-allocate (unlikely though that is),\nwhich would leak here.\n\n> return 0;\n> }\n> --\n> 1.9.1.528.g98b8868.dirty\n"},{"id":"240098","messageId":"CAPig+cQM-5_N5caoJVRY0+VcPXO0qhnv7hetdanOoqRQJkm3jQ@mail.gmail.com","threadId":"36530","inReplyTo":"1398725682-30782-9-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v4 08/27] refs.c: change ref_transaction_update() to do error checking and return status","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-04-29T00:51:41Z","receivedAt":"2014-04-29T00:51:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 28, 2014 at 6:54 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 \n> ---\n> builtin/update-ref.c | 10 ++++++----\n> refs.c | 9 +++++++--\n> refs.h | 10 +++++-----\n> 3 files changed, 18 insertions(+), 11 deletions(-)\n>\n> diff --git a/builtin/update-ref.c b/builtin/update-ref.c\n> index 2bef2a0..59c4d6b 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> if (*next != line_termination)\n> die(\"update %s: extra input: %s\", refname, next);\n>\n> - ref_transaction_update(transaction, refname, new_sha1, old_sha1,\n> - update_flags, have_old);\n> + if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,\n> + update_flags, have_old))\n> + die(\"update %s: failed\", refname);\n>\n> update_flags = 0;\n> free(refname);\n> @@ -286,8 +287,9 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)\n> if (*next != line_termination)\n> die(\"verify %s: extra input: %s\", refname, next);\n>\n> - ref_transaction_update(transaction, refname, new_sha1, old_sha1,\n> - update_flags, have_old);\n> + if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,\n> + update_flags, have_old))\n> + die(\"failed transaction update for %s\", refname);\n>\n> update_flags = 0;\n> free(refname);\n> diff --git a/refs.c b/refs.c\n> index 308e13e..1a903fb 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -3333,19 +3333,24 @@ static struct ref_update *add_update(struct ref_transaction *transaction,\n> return update;\n> }\n>\n> -void ref_transaction_update(struct ref_transaction *transaction,\n> +int ref_transaction_update(struct ref_transaction *transaction,\n> const char *refname,\n> const unsigned char *new_sha1,\n> const unsigned char *old_sha1,\n> int flags, int have_old)\n> {\n> - struct ref_update *update = add_update(transaction, refname);\n> + struct ref_update *update;\n> +\n> + if (have_old && !old_sha1)\n> + die(\"have_old is true but old_sha1 is NULL\");\n>\n> + update = add_update(transaction, refname);\n> hashcpy(update->new_sha1, new_sha1);\n> update->flags = flags;\n> update->have_old = have_old;\n> if (have_old)\n> hashcpy(update->old_sha1, old_sha1);\n> + return 0;\n\nAm I misreading? The commit message talks about returning true on\nerror, but this code seems to only ever die() or return 0 (false).\n\n> }\n>\n> void ref_transaction_create(struct ref_transaction *transaction,\n> diff --git a/refs.h b/refs.h\n> index bc7715e..0364a3e 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> - const char *refname,\n> - const unsigned char *new_sha1,\n> - const unsigned char *old_sha1,\n> - int flags, int have_old);\n> +int ref_transaction_update(struct ref_transaction *transaction,\n> + const char *refname,\n> + const unsigned char *new_sha1,\n> + const unsigned char *old_sha1,\n> + int flags, int have_old);\n>\n> /*\n> * Add a reference creation to transaction. new_sha1 is the value\n> --\n> 1.9.1.528.g98b8868.dirty\n"},{"id":"240103","messageId":"CAPig+cTE9+XHYfnynMykbYf=0kMivj5wFu0Lovnr8XbxyEpsyA@mail.gmail.com","threadId":"36530","inReplyTo":"1398725682-30782-16-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v4 15/27] fast-import.c: change update_branch to use ref transactions","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-04-29T02:18:18Z","receivedAt":"2014-04-29T02:18:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 28, 2014 at 6:54 PM, Ronnie Sahlberg wrote:\n> Change update_branch() to use ref transactions for updates.\n>\n> Signed-off-by: Ronnie Sahlberg \n> ---\n> fast-import.c | 20 ++++++++++++--------\n> 1 file changed, 12 insertions(+), 8 deletions(-)\n>\n> diff --git a/fast-import.c b/fast-import.c\n> index fb4738d..300c8dc 100644\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -1678,36 +1678,40 @@ found_entry:\n> static int update_branch(struct branch *b)\n> {\n> static const char *msg = \"fast-import\";\n> - struct ref_lock *lock;\n> + struct ref_transaction *transaction;\n> unsigned char old_sha1[20];\n> + struct strbuf err = STRBUF_INIT;\n>\n> if (is_null_sha1(b->sha1))\n> return 0;\n> if (read_ref(b->name, old_sha1))\n> hashclr(old_sha1);\n> - lock = lock_any_ref_for_update(b->name, old_sha1, 0, NULL);\n> - if (!lock)\n> - return error(\"Unable to lock %s\", b->name);\n> if (!force_update && !is_null_sha1(old_sha1)) {\n> struct commit *old_cmit, *new_cmit;\n>\n> old_cmit = lookup_commit_reference_gently(old_sha1, 0);\n> new_cmit = lookup_commit_reference_gently(b->sha1, 0);\n> if (!old_cmit || !new_cmit) {\n> - unlock_ref(lock);\n> return error(\"Branch %s is missing commits.\", b->name);\n> }\n>\n> if (!in_merge_bases(old_cmit, new_cmit)) {\n> - unlock_ref(lock);\n> warning(\"Not updating %s\"\n> \" (new tip %s does not contain %s)\",\n> b->name, sha1_to_hex(b->sha1), sha1_to_hex(old_sha1));\n> return -1;\n> }\n> }\n> - if (write_ref_sha1(lock, b->sha1, msg) < 0)\n> - return error(\"Unable to update %s\", b->name);\n> + transaction = ref_transaction_begin();\n> + if ((!transaction ||\n> + ref_transaction_update(transaction, b->name, b->sha1, old_sha1,\n> + 0, 1)) ||\n> + (ref_transaction_commit(transaction, msg, &err) &&\n> + !(transaction = NULL))) {\n> + ref_transaction_rollback(transaction);\n> + return error(\"Unable to update branch %s: %s\", b->name,\n> + strbuf_detach(&err, NULL));\n\nIffy strbuf handling. The strbuf content is being leaked here whether\ndetached or not.\n\n> + }\n> return 0;\n> }\n>\n> --\n> 1.9.1.528.g98b8868.dirty\n"},{"id":"240180","messageId":"CAL=YDW=R5LpG-w9zAo9eeop7QqnspcVb8OQmdTOg35sJi9fMyA@mail.gmail.com","threadId":"36530","inReplyTo":"CAPig+cQM-5_N5caoJVRY0+VcPXO0qhnv7hetdanOoqRQJkm3jQ@mail.gmail.com","subject":"Re: [PATCH v4 08/27] refs.c: change ref_transaction_update() to do error checking and return status","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-29T16:05:08Z","receivedAt":"2014-04-29T16:05:08Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Mon, Apr 28, 2014 at 5:51 PM, Eric Sunshine wrote:\n> On Mon, Apr 28, 2014 at 6:54 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 \n>> ---\n>> builtin/update-ref.c | 10 ++++++----\n>> refs.c | 9 +++++++--\n>> refs.h | 10 +++++-----\n>> 3 files changed, 18 insertions(+), 11 deletions(-)\n>>\n>> diff --git a/builtin/update-ref.c b/builtin/update-ref.c\n>> index 2bef2a0..59c4d6b 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>> if (*next != line_termination)\n>> die(\"update %s: extra input: %s\", refname, next);\n>>\n>> - ref_transaction_update(transaction, refname, new_sha1, old_sha1,\n>> - update_flags, have_old);\n>> + if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,\n>> + update_flags, have_old))\n>> + die(\"update %s: failed\", refname);\n>>\n>> update_flags = 0;\n>> free(refname);\n>> @@ -286,8 +287,9 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)\n>> if (*next != line_termination)\n>> die(\"verify %s: extra input: %s\", refname, next);\n>>\n>> - ref_transaction_update(transaction, refname, new_sha1, old_sha1,\n>> - update_flags, have_old);\n>> + if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,\n>> + update_flags, have_old))\n>> + die(\"failed transaction update for %s\", refname);\n>>\n>> update_flags = 0;\n>> free(refname);\n>> diff --git a/refs.c b/refs.c\n>> index 308e13e..1a903fb 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -3333,19 +3333,24 @@ static struct ref_update *add_update(struct ref_transaction *transaction,\n>> return update;\n>> }\n>>\n>> -void ref_transaction_update(struct ref_transaction *transaction,\n>> +int ref_transaction_update(struct ref_transaction *transaction,\n>> const char *refname,\n>> const unsigned char *new_sha1,\n>> const unsigned char *old_sha1,\n>> int flags, int have_old)\n>> {\n>> - struct ref_update *update = add_update(transaction, refname);\n>> + struct ref_update *update;\n>> +\n>> + if (have_old && !old_sha1)\n>> + die(\"have_old is true but old_sha1 is NULL\");\n>>\n>> + update = add_update(transaction, refname);\n>> hashcpy(update->new_sha1, new_sha1);\n>> update->flags = flags;\n>> update->have_old = have_old;\n>> if (have_old)\n>> hashcpy(update->old_sha1, old_sha1);\n>> + return 0;\n>\n> Am I misreading? The commit message talks about returning true on\n> error, but this code seems to only ever die() or return 0 (false).\n>\n\nI have updated the commit message to be more precise.\nThe function returns true on error but there are currently no checks\nthat actually\ndo return failure in this function. Such checks will be added in the future.\nAlso having it return an int instead of void allows us to use the pattern\n\nif(!transaction ||\n ref_transaction_update(transaction, ...) |||\n ref_transaction_commit(transaction, ...)) {\n\neventhough we do not yet have any conditions where _update will fail.\n\n\n\n\n>> }\n>>\n>> void ref_transaction_create(struct ref_transaction *transaction,\n>> diff --git a/refs.h b/refs.h\n>> index bc7715e..0364a3e 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>> - const char *refname,\n>> - const unsigned char *new_sha1,\n>> - const unsigned char *old_sha1,\n>> - int flags, int have_old);\n>> +int ref_transaction_update(struct ref_transaction *transaction,\n>> + const char *refname,\n>> + const unsigned char *new_sha1,\n>> + const unsigned char *old_sha1,\n>> + int flags, int have_old);\n>>\n>> /*\n>> * Add a reference creation to transaction. new_sha1 is the value\n>> --\n>> 1.9.1.528.g98b8868.dirty\n"},{"id":"240181","messageId":"CAL=YDWn9TJsygZdgY2gXVBScFjO3zbrbJ5XORc2ZLaY-M8PmSw@mail.gmail.com","threadId":"36530","inReplyTo":"CAPig+cSWMK_kyVvaD8QCfZmPu4JVT+nOJZJLteHhDao0umHLbA@mail.gmail.com","subject":"Re: [PATCH v4 12/27] replace.c: use the ref transaction functions for updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-29T16:18:25Z","receivedAt":"2014-04-29T16:18:25Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Mon, Apr 28, 2014 at 5:44 PM, Eric Sunshine wrote:\n> On Mon, Apr 28, 2014 at 6:54 PM, Ronnie Sahlberg wrote:\n>> Update replace.c to use ref transactions for updates.\n>>\n>> Signed-off-by: Ronnie Sahlberg \n>> ---\n>> builtin/replace.c | 14 ++++++++------\n>> 1 file changed, 8 insertions(+), 6 deletions(-)\n>>\n>> diff --git a/builtin/replace.c b/builtin/replace.c\n>> index b62420a..b037b29 100644\n>> --- a/builtin/replace.c\n>> +++ b/builtin/replace.c\n>> @@ -129,7 +129,8 @@ static int replace_object(const char *object_ref, const char *replace_ref,\n>> unsigned char object[20], prev[20], repl[20];\n>> enum object_type obj_type, repl_type;\n>> char ref[PATH_MAX];\n>> - struct ref_lock *lock;\n>> + struct ref_transaction *transaction;\n>> + struct strbuf err = STRBUF_INIT;\n>>\n>> if (get_sha1(object_ref, object))\n>> die(\"Failed to resolve '%s' as a valid ref.\", object_ref);\n>> @@ -157,11 +158,12 @@ static int replace_object(const char *object_ref, const char *replace_ref,\n>> else if (!force)\n>> die(\"replace ref '%s' already exists\", ref);\n>>\n>> - lock = lock_any_ref_for_update(ref, prev, 0, NULL);\n>> - if (!lock)\n>> - die(\"%s: cannot lock the ref\", ref);\n>> - if (write_ref_sha1(lock, repl, NULL) < 0)\n>> - die(\"%s: cannot update the ref\", ref);\n>> + transaction = ref_transaction_begin();\n>> + if (!transaction ||\n>> + ref_transaction_update(transaction, ref, repl, prev,\n>> + 0, !is_null_sha1(prev)) ||\n>> + ref_transaction_commit(transaction, NULL, &err))\n>> + die(_(\"%s: failed to replace ref: %s\"), ref, err.buf);\n>\n> Even though 'err' will be empty after this conditional, would\n> strbuf_release(&err) here be warranted to future-proof it? Today's\n> implementation of strbuf will not have allocated any memory, but\n> perhaps a future change might pre-allocate (unlikely though that is),\n> which would leak here.\n\n\nThanks.\n\n\nI have no strong feelings either way.\nA previous patch got a comment to remove a strbuf_release() because we\nknew in that\ncode path that the string would be empty and thus the call was superfluous.\n\nSo one for and one against so far.\nI will leave as is until there is more consensus.\n\n\n\n>\n>> return 0;\n>> }\n>> --\n>> 1.9.1.528.g98b8868.dirty\n"},{"id":"240183","messageId":"CAL=YDWkD+0=Cy5wjAUtVn8QNtVr3NSaUkRoEnC+QcdRhYG59vA@mail.gmail.com","threadId":"36530","inReplyTo":"CAPig+cTE9+XHYfnynMykbYf=0kMivj5wFu0Lovnr8XbxyEpsyA@mail.gmail.com","subject":"Re: [PATCH v4 15/27] fast-import.c: change update_branch to use ref transactions","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-29T16:57:16Z","receivedAt":"2014-04-29T16:57:16Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Mon, Apr 28, 2014 at 7:18 PM, Eric Sunshine wrote:\n> On Mon, Apr 28, 2014 at 6:54 PM, Ronnie Sahlberg wrote:\n>> Change update_branch() to use ref transactions for updates.\n>>\n>> Signed-off-by: Ronnie Sahlberg \n>> ---\n>> fast-import.c | 20 ++++++++++++--------\n>> 1 file changed, 12 insertions(+), 8 deletions(-)\n>>\n>> diff --git a/fast-import.c b/fast-import.c\n>> index fb4738d..300c8dc 100644\n>> --- a/fast-import.c\n>> +++ b/fast-import.c\n>> @@ -1678,36 +1678,40 @@ found_entry:\n>> static int update_branch(struct branch *b)\n>> {\n>> static const char *msg = \"fast-import\";\n>> - struct ref_lock *lock;\n>> + struct ref_transaction *transaction;\n>> unsigned char old_sha1[20];\n>> + struct strbuf err = STRBUF_INIT;\n>>\n>> if (is_null_sha1(b->sha1))\n>> return 0;\n>> if (read_ref(b->name, old_sha1))\n>> hashclr(old_sha1);\n>> - lock = lock_any_ref_for_update(b->name, old_sha1, 0, NULL);\n>> - if (!lock)\n>> - return error(\"Unable to lock %s\", b->name);\n>> if (!force_update && !is_null_sha1(old_sha1)) {\n>> struct commit *old_cmit, *new_cmit;\n>>\n>> old_cmit = lookup_commit_reference_gently(old_sha1, 0);\n>> new_cmit = lookup_commit_reference_gently(b->sha1, 0);\n>> if (!old_cmit || !new_cmit) {\n>> - unlock_ref(lock);\n>> return error(\"Branch %s is missing commits.\", b->name);\n>> }\n>>\n>> if (!in_merge_bases(old_cmit, new_cmit)) {\n>> - unlock_ref(lock);\n>> warning(\"Not updating %s\"\n>> \" (new tip %s does not contain %s)\",\n>> b->name, sha1_to_hex(b->sha1), sha1_to_hex(old_sha1));\n>> return -1;\n>> }\n>> }\n>> - if (write_ref_sha1(lock, b->sha1, msg) < 0)\n>> - return error(\"Unable to update %s\", b->name);\n>> + transaction = ref_transaction_begin();\n>> + if ((!transaction ||\n>> + ref_transaction_update(transaction, b->name, b->sha1, old_sha1,\n>> + 0, 1)) ||\n>> + (ref_transaction_commit(transaction, msg, &err) &&\n>> + !(transaction = NULL))) {\n>> + ref_transaction_rollback(transaction);\n>> + return error(\"Unable to update branch %s: %s\", b->name,\n>> + strbuf_detach(&err, NULL));\n>\n> Iffy strbuf handling. The strbuf content is being leaked here whether\n> detached or not.\n>\n\nThanks!\n\nI have updated this and all other patches to make sure we do a\nstrbuf_release() any time we have\nadded to the string.\n\nI also did a quick audit of the strbuf_detach() use in builtin/*\n(which I based my use on)\nand there seems to be quite common that strbuf_detach() is used in a\nway that will leak memory.\n\n\nI will make a note and perhaps audit all the other strbuf_detach()\ncalls for a future patch series.\n\n\n\n\n>> + }\n>> return 0;\n>> }\n>>\n>> --\n>> 1.9.1.528.g98b8868.dirty\n"},{"id":"240193","messageId":"CAPig+cQ5br8icrUPwP7=zuQnXD4-tNU5rjZ8AWhCWGwT5KegfA@mail.gmail.com","threadId":"36530","inReplyTo":"CAL=YDWn9TJsygZdgY2gXVBScFjO3zbrbJ5XORc2ZLaY-M8PmSw@mail.gmail.com","subject":"Re: [PATCH v4 12/27] replace.c: use the ref transaction functions for updates","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-04-29T18:29:10Z","receivedAt":"2014-04-29T18:29:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Apr 29, 2014 at 12:18 PM, Ronnie Sahlberg wrote:\n> On Mon, Apr 28, 2014 at 5:44 PM, Eric Sunshine wrote:\n>> On Mon, Apr 28, 2014 at 6:54 PM, Ronnie Sahlberg wrote:\n>>> Update replace.c to use ref transactions for updates.\n>>>\n>>> Signed-off-by: Ronnie Sahlberg \n>>> ---\n>>> builtin/replace.c | 14 ++++++++------\n>>> 1 file changed, 8 insertions(+), 6 deletions(-)\n>>>\n>>> diff --git a/builtin/replace.c b/builtin/replace.c\n>>> index b62420a..b037b29 100644\n>>> --- a/builtin/replace.c\n>>> +++ b/builtin/replace.c\n>>> @@ -129,7 +129,8 @@ static int replace_object(const char *object_ref, const char *replace_ref,\n>>> unsigned char object[20], prev[20], repl[20];\n>>> enum object_type obj_type, repl_type;\n>>> char ref[PATH_MAX];\n>>> - struct ref_lock *lock;\n>>> + struct ref_transaction *transaction;\n>>> + struct strbuf err = STRBUF_INIT;\n>>>\n>>> if (get_sha1(object_ref, object))\n>>> die(\"Failed to resolve '%s' as a valid ref.\", object_ref);\n>>> @@ -157,11 +158,12 @@ static int replace_object(const char *object_ref, const char *replace_ref,\n>>> else if (!force)\n>>> die(\"replace ref '%s' already exists\", ref);\n>>>\n>>> - lock = lock_any_ref_for_update(ref, prev, 0, NULL);\n>>> - if (!lock)\n>>> - die(\"%s: cannot lock the ref\", ref);\n>>> - if (write_ref_sha1(lock, repl, NULL) < 0)\n>>> - die(\"%s: cannot update the ref\", ref);\n>>> + transaction = ref_transaction_begin();\n>>> + if (!transaction ||\n>>> + ref_transaction_update(transaction, ref, repl, prev,\n>>> + 0, !is_null_sha1(prev)) ||\n>>> + ref_transaction_commit(transaction, NULL, &err))\n>>> + die(_(\"%s: failed to replace ref: %s\"), ref, err.buf);\n>>\n>> Even though 'err' will be empty after this conditional, would\n>> strbuf_release(&err) here be warranted to future-proof it? Today's\n>> implementation of strbuf will not have allocated any memory, but\n>> perhaps a future change might pre-allocate (unlikely though that is),\n>> which would leak here.\n>\n> I have no strong feelings either way.\n> A previous patch got a comment to remove a strbuf_release() because we\n> knew in that\n> code path that the string would be empty and thus the call was superfluous.\n\nIndeed, I realized later that the reason I gave for possibly adding\nthe strbuf_release() was effectively bogus. Code throughout the\nproject ignores strbuff_release() when it is obvious that the strbuf\nhasn't been used. Such code looks like this:\n\nfunc() {\n struct strbuf foo = STRBUF_INIT;\n ...code not touching 'foo'...\n if (...)\n return;\n ...code touching 'foo'...\n strbuf_release(&foo);\n}\n\nAt the point of early return, it's obvious at a glance that no content\nhas been added to 'foo'.\n\nIt was only a little while ago that I came to understand why the code\nin this patch bothered me:\n\nfunc() {\n struct strbuf foo = STRBUF_INIT;\n if (bar(&foo))\n die(...);\n ...return or fall off end without releasing 'foo'...\n}\n\nThe problem is that it's not so easy to see that it's safe to \"leak\"\nthe strbuf without detouring through the documentation of bar() and\npossibly its implementation as well, before finally resuming the\nreading of func(). That extra cost of having to study bar() will\nlikely be paid by most readers of func() who are concerned about the\nmissing strbuf_release(). The cognitive burden is greater.\n\n> So one for and one against so far.\n> I will leave as is until there is more consensus.\n\nFair enough. The fact that the variable is named 'err' in this case\nmight be enough to allow one to infer, without detouring through\nbar(), that the missing strbuf_release() is intentional.\n"},{"id":"240203","messageId":"xmqq4n1bke4b.fsf@gitster.dls.corp.google.com","threadId":"36530","inReplyTo":"1398725682-30782-1-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v4 00/27] Use ref transactions for all ref updates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-29T21:41:24Z","receivedAt":"2014-04-29T21:41:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ronnie Sahlberg writes:\n\n> This patch series is based on mhagger/ref-transactions and expands on the\n> transaction API. It converts all external (outside of refs.c) callers to\n> use the transaction API for any writes.\n> This makes most of the ref updates to become atomic when there are failures\n> locking or writing to a ref.\n\nI think I saw some more comments to be addressed on this round, but\nI'll try to push it through into 'pu' by attempting to resolve\nconflicts so that I can ask you to eyeball the result. This series\nseem to conflict with your own rs/ref-update-check-errors-early\ntopic that is already in 'next'.\n\nIf I screwed up the conflict resolution (which is very possible), I\nmay have to ask you to rebase this series on top of acc62aa (which\nis the merge of rs/ref-update-check-errors-early to 'next'), but I\nam hoping that conflict resolution I'll push out today will be\ncorrect. We'll see.\n\nThanks.\n"},{"id":"240217","messageId":"xmqqmwf3iy82.fsf@gitster.dls.corp.google.com","threadId":"36530","inReplyTo":"1398725682-30782-16-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v4 15/27] fast-import.c: change update_branch to use ref transactions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-29T22:10:05Z","receivedAt":"2014-04-29T22:10:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ronnie Sahlberg writes:\n\n> +\ttransaction = ref_transaction_begin();\n> +\tif ((!transaction ||\n> +\t ref_transaction_update(transaction, b->name, b->sha1, old_sha1,\n> +\t\t\t\t 0, 1)) ||\n> +\t (ref_transaction_commit(transaction, msg, &err) &&\n> +\t !(transaction = NULL))) {\n> +\t\tref_transaction_rollback(transaction);\n> +\t\treturn error(\"Unable to update branch %s: %s\", b->name,\n> +\t\t\t strbuf_detach(&err, NULL));\n> +\t}\n\nThe assigning of NULL to transaction and always returning true looks\nvery suspicious.\n\nAlso, if we couldn't get transaction what happens? I think you\nwanted to avoid passing NULL to rollback but at the same time I\nsuspect you wanted to fall into that \"return error()\" part, but\napparently you are failing to have that cake and eat it too, no?\n"},{"id":"240248","messageId":"CAL=YDWkv_XEXCpuAE8inD6Lj5NNHOEf9womimE70AxinO1Ufng@mail.gmail.com","threadId":"36530","inReplyTo":"xmqq4n1bke4b.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 00/27] Use ref transactions for all ref updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-29T22:24:50Z","receivedAt":"2014-04-29T22:24:50Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Tue, Apr 29, 2014 at 2:41 PM, Junio C Hamano wrote:\n> Ronnie Sahlberg writes:\n>\n>> This patch series is based on mhagger/ref-transactions and expands on the\n>> transaction API. It converts all external (outside of refs.c) callers to\n>> use the transaction API for any writes.\n>> This makes most of the ref updates to become atomic when there are failures\n>> locking or writing to a ref.\n>\n> I think I saw some more comments to be addressed on this round, but\n> I'll try to push it through into 'pu' by attempting to resolve\n> conflicts so that I can ask you to eyeball the result. This series\n> seem to conflict with your own rs/ref-update-check-errors-early\n> topic that is already in 'next'.\n>\n> If I screwed up the conflict resolution (which is very possible), I\n> may have to ask you to rebase this series on top of acc62aa (which\n> is the merge of rs/ref-update-check-errors-early to 'next'), but I\n> am hoping that conflict resolution I'll push out today will be\n> correct. We'll see.\n>\n\nThanks.\n\nI just sent v5 of the patch series where I have addressed the comments from\nEric and Michael so far.\n\nOtherwise, how hard would it be to drop\nrs/ref-update-check-errors-early ?\n\nI can just redo those patches later, and that might be the easiest solution.\n\nRegards\nronnie sahlberg\n\n\n> Thanks.\n"}]}