{"thread":{"id":"36495","subject":"[PATCH v3 02/19] refs.c: allow passing NULL to ref_transaction_free","startedAt":"2014-04-25T16:14:35Z","lastAt":"2014-04-30T14:20:12Z","messageCount":48,"participants":["Ronnie Sahlberg","Jonathan Nieder","Michael Haggerty","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":19},"messages":[{"id":"239653","messageId":"1398442494-23438-1-git-send-email-sahlberg@google.com","threadId":"36495","inReplyTo":null,"subject":"[PATCH v3 00/19] Use ref transactions from most callers","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-25T16:14:35Z","receivedAt":"2014-04-25T16:14:35Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"This patch series changes most of the places where the ref functions for\nlocking and writing refs to instead use the new ref transaction API. There\nare still three more places where write_ref_sha1() is called from outside\nof refs.c but those all will require more complex work and review so those\nchanges belong in a different patch series.\n\nI think I have covered all issues raised on the previous reviews and also\ndone a bunch of cleanups and improvements to the transaction API.\n\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 (19):\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: make ref_transaction_commit return an error string\n  refs.c: return error string from ref_update_reject_duplicates on\n    failure\n  update-ref.c: use the error string from _commit to print better\n    message\n  refs.c: make update_ref_write to return error string 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: pass **transaction to commit and have it clear the pointer\n\n branch.c              |  39 ++++++++++------\n builtin/commit.c      |  23 +++++-----\n builtin/replace.c     |  14 +++---\n builtin/tag.c         |  14 +++---\n builtin/update-ref.c  |  30 ++++++++-----\n fast-import.c         |  18 ++++----\n refs.c                | 122 +++++++++++++++++++++++++++++++++++---------------\n refs.h                |  32 +++++++------\n sequencer.c           |  21 ++++++---\n t/t1400-update-ref.sh |  16 +++----\n 10 files changed, 206 insertions(+), 123 deletions(-)\n\n-- \n1.9.1.521.g5dc89fa\n"},{"id":"239666","messageId":"1398442494-23438-2-git-send-email-sahlberg@google.com","threadId":"36495","inReplyTo":"1398442494-23438-1-git-send-email-sahlberg@google.com","subject":"[PATCH v3 01/19] refs.c: constify the sha arguments for ref_transaction_create|delete|update","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-25T16:14:36Z","receivedAt":"2014-04-25T16:14:36Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"ref_transaction_create|delete|update has no need to modify the sha1\narguments passed to it so it should use const unsigned char* instead\nof unsigned char*.\n\nSome functions, such as fast_forward_to(), already have its old/new\nsha1 arguments as consts. This function will at some point need to\nuse ref_transaction_update() in which case this change is required.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n refs.c | 7 ++++---\n refs.h | 7 ++++---\n 2 files changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 728a761..138ab70 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3329,7 +3329,8 @@ static struct ref_update *add_update(struct ref_transaction *transaction,\n \n void ref_transaction_update(struct ref_transaction *transaction,\n \t\t\t    const char *refname,\n-\t\t\t    unsigned char *new_sha1, unsigned char *old_sha1,\n+\t\t\t    const unsigned char *new_sha1,\n+\t\t\t    const unsigned char *old_sha1,\n \t\t\t    int flags, int have_old)\n {\n \tstruct ref_update *update = add_update(transaction, refname);\n@@ -3343,7 +3344,7 @@ void ref_transaction_update(struct ref_transaction *transaction,\n \n void ref_transaction_create(struct ref_transaction *transaction,\n \t\t\t    const char *refname,\n-\t\t\t    unsigned char *new_sha1,\n+\t\t\t    const unsigned char *new_sha1,\n \t\t\t    int flags)\n {\n \tstruct ref_update *update = add_update(transaction, refname);\n@@ -3357,7 +3358,7 @@ void ref_transaction_create(struct ref_transaction *transaction,\n \n void ref_transaction_delete(struct ref_transaction *transaction,\n \t\t\t    const char *refname,\n-\t\t\t    unsigned char *old_sha1,\n+\t\t\t    const unsigned char *old_sha1,\n \t\t\t    int flags, int have_old)\n {\n \tstruct ref_update *update = add_update(transaction, refname);\ndiff --git a/refs.h b/refs.h\nindex 0f08def..892c5b6 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -239,7 +239,8 @@ void ref_transaction_rollback(struct ref_transaction *transaction);\n  */\n void ref_transaction_update(struct ref_transaction *transaction,\n \t\t\t    const char *refname,\n-\t\t\t    unsigned char *new_sha1, unsigned char *old_sha1,\n+\t\t\t    const unsigned char *new_sha1,\n+\t\t\t    const unsigned char *old_sha1,\n \t\t\t    int flags, int have_old);\n \n /*\n@@ -250,7 +251,7 @@ void ref_transaction_update(struct ref_transaction *transaction,\n  */\n void ref_transaction_create(struct ref_transaction *transaction,\n \t\t\t    const char *refname,\n-\t\t\t    unsigned char *new_sha1,\n+\t\t\t    const unsigned char *new_sha1,\n \t\t\t    int flags);\n \n /*\n@@ -260,7 +261,7 @@ void ref_transaction_create(struct ref_transaction *transaction,\n  */\n void ref_transaction_delete(struct ref_transaction *transaction,\n \t\t\t    const char *refname,\n-\t\t\t    unsigned char *old_sha1,\n+\t\t\t    const unsigned char *old_sha1,\n \t\t\t    int flags, int have_old);\n \n /*\n-- \n1.9.1.521.g5dc89fa\n"},{"id":"239648","messageId":"1398442494-23438-3-git-send-email-sahlberg@google.com","threadId":"36495","inReplyTo":"1398442494-23438-1-git-send-email-sahlberg@google.com","subject":"[PATCH v3 02/19] refs.c: allow passing NULL to ref_transaction_free","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-25T16:14:37Z","receivedAt":"2014-04-25T16:14:37Z","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 <sahlberg@google.com>\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.521.g5dc89fa\n"},{"id":"239659","messageId":"1398442494-23438-4-git-send-email-sahlberg@google.com","threadId":"36495","inReplyTo":"1398442494-23438-1-git-send-email-sahlberg@google.com","subject":"[PATCH v3 03/19] refs.c: make ref_transaction_commit return an error string","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-25T16:14:38Z","receivedAt":"2014-04-25T16:14:38Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Let ref_transaction_commit return an optional error string that describes\nthe transaction failure. Start by returning the same error as update_ref_lock\nreturns, modulo the initial error:/fatal: preamble.\n\nThis will make it easier for callers to craft better error messages when\na transaction call fails.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n builtin/update-ref.c |  2 +-\n refs.c               | 12 +++++++++++-\n refs.h               |  5 ++++-\n 3 files changed, 16 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..0712912 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, char **err,\n+\t\t\t   enum action_on_err onerr)\n {\n \tint ret = 0, delnum = 0, i;\n \tconst char **delnames;\n@@ -3424,6 +3425,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \tif (!n)\n \t\treturn 0;\n \n+\tif (err)\n+\t\t*err = NULL;\n+\n \t/* Allocate work space */\n \tdelnames = xmalloc(sizeof(*delnames) * n);\n \n@@ -3443,6 +3447,12 @@ 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\tconst char *str = \"Cannot lock the ref '%s'.\";\n+\t\t\t\t*err = xmalloc(PATH_MAX + 24);\n+\t\t\t\tsnprintf(*err, PATH_MAX + 24, str,\n+\t\t\t\t\t update->refname);\n+\t\t\t}\n \t\t\tret = 1;\n \t\t\tgoto cleanup;\n \t\t}\ndiff --git a/refs.h b/refs.h\nindex 892c5b6..615c73d 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 error is non-NULL it will return an error string that describes\n+ * why a commit failed. This string must be free()ed by the caller.\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, char **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.521.g5dc89fa\n"},{"id":"239660","messageId":"1398442494-23438-5-git-send-email-sahlberg@google.com","threadId":"36495","inReplyTo":"1398442494-23438-1-git-send-email-sahlberg@google.com","subject":"[PATCH v3 04/19] refs.c: return error string from ref_update_reject_duplicates on failure","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-25T16:14:39Z","receivedAt":"2014-04-25T16:14:39Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n refs.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 0712912..9d9eab8 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\tchar **err,\n \t\t\t\t\tenum action_on_err onerr)\n {\n \tint i;\n@@ -3400,6 +3401,11 @@ 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\t*err = xmalloc(PATH_MAX + 41);\n+\t\t\t\tsnprintf(*err, PATH_MAX + 41, str,\n+\t\t\t\t\t updates[i]->refname);\n+\t\t\t}\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@@ -3433,7 +3439,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.521.g5dc89fa\n"},{"id":"239658","messageId":"1398442494-23438-6-git-send-email-sahlberg@google.com","threadId":"36495","inReplyTo":"1398442494-23438-1-git-send-email-sahlberg@google.com","subject":"[PATCH v3 05/19] update-ref.c: use the error string from _commit to print better message","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-25T16:14:40Z","receivedAt":"2014-04-25T16:14:40Z","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 error string\nthat is returned to print a better log message if/after the transaction\nfails.\n\nUpdate the tests to reflect that the log message is now slightly different\n  fatal: update_ref failed: Cannot lock the ref 'some ref'\nversus from the previous\n  fatal: Cannot lock the ref 'some ref'\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n builtin/update-ref.c  | 12 +++++++-----\n t/t1400-update-ref.sh | 16 ++++++++--------\n 2 files changed, 15 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex aaa06aa..47c9b53 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+\tchar *err;\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,18 @@ 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 (!transaction)\n+\t\t\tdie(\"failed to update refs\");\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(\"update_ref failed: %s\", err);\n+\t\treturn 0;\n \t}\n \n \tif (end_null)\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 48ccc4d..53ed0cb 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -453,7 +453,7 @@ test_expect_success 'stdin fails with duplicate refs' '\n \tcreate $a $m\n \tEOF\n \ttest_must_fail git update-ref --stdin <stdin 2>err &&\n-\tgrep \"fatal: Multiple updates for ref '\"'\"'$a'\"'\"' not allowed.\" err\n+\tgrep \"fatal: update_ref failed: Multiple updates for ref '\"'\"'$a'\"'\"' not allowed.\" err\n '\n \n test_expect_success 'stdin create ref works' '\n@@ -511,7 +511,7 @@ test_expect_success 'stdin create ref works with path with space to blob' '\n test_expect_success 'stdin update ref fails with wrong old value' '\n \techo \"update $c $m $m~1\" >stdin &&\n \ttest_must_fail git update-ref --stdin <stdin 2>err &&\n-\tgrep \"fatal: Cannot lock the ref '\"'\"'$c'\"'\"'\" err &&\n+\tgrep \"fatal: update_ref failed: Cannot lock the ref '\"'\"'$c'\"'\"'\" err &&\n \ttest_must_fail git rev-parse --verify -q $c\n '\n \n@@ -547,7 +547,7 @@ test_expect_success 'stdin update ref works with right old value' '\n test_expect_success 'stdin delete ref fails with wrong old value' '\n \techo \"delete $a $m~1\" >stdin &&\n \ttest_must_fail git update-ref --stdin <stdin 2>err &&\n-\tgrep \"fatal: Cannot lock the ref '\"'\"'$a'\"'\"'\" err &&\n+\tgrep \"fatal: update_ref failed: Cannot lock the ref '\"'\"'$a'\"'\"'\" err &&\n \tgit rev-parse $m >expect &&\n \tgit rev-parse $a >actual &&\n \ttest_cmp expect actual\n@@ -634,7 +634,7 @@ test_expect_success 'stdin update refs fails with wrong old value' '\n \tupdate $c  ''\n \tEOF\n \ttest_must_fail git update-ref --stdin <stdin 2>err &&\n-\tgrep \"fatal: Cannot lock the ref '\"'\"'$c'\"'\"'\" err &&\n+\tgrep \"fatal: update_ref failed: Cannot lock the ref '\"'\"'$c'\"'\"'\" err &&\n \tgit rev-parse $m >expect &&\n \tgit rev-parse $a >actual &&\n \ttest_cmp expect actual &&\n@@ -807,7 +807,7 @@ test_expect_success 'stdin -z fails option with unknown name' '\n test_expect_success 'stdin -z fails with duplicate refs' '\n \tprintf $F \"create $a\" \"$m\" \"create $b\" \"$m\" \"create $a\" \"$m\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin <stdin 2>err &&\n-\tgrep \"fatal: Multiple updates for ref '\"'\"'$a'\"'\"' not allowed.\" err\n+\tgrep \"fatal: update_ref failed: Multiple updates for ref '\"'\"'$a'\"'\"' not allowed.\" err\n '\n \n test_expect_success 'stdin -z create ref works' '\n@@ -847,7 +847,7 @@ test_expect_success 'stdin -z create ref works with path with space to blob' '\n test_expect_success 'stdin -z update ref fails with wrong old value' '\n \tprintf $F \"update $c\" \"$m\" \"$m~1\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin <stdin 2>err &&\n-\tgrep \"fatal: Cannot lock the ref '\"'\"'$c'\"'\"'\" err &&\n+\tgrep \"fatal: update_ref failed: Cannot lock the ref '\"'\"'$c'\"'\"'\" err &&\n \ttest_must_fail git rev-parse --verify -q $c\n '\n \n@@ -883,7 +883,7 @@ test_expect_success 'stdin -z update ref works with right old value' '\n test_expect_success 'stdin -z delete ref fails with wrong old value' '\n \tprintf $F \"delete $a\" \"$m~1\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin <stdin 2>err &&\n-\tgrep \"fatal: Cannot lock the ref '\"'\"'$a'\"'\"'\" err &&\n+\tgrep \"fatal: update_ref failed: Cannot lock the ref '\"'\"'$a'\"'\"'\" err &&\n \tgit rev-parse $m >expect &&\n \tgit rev-parse $a >actual &&\n \ttest_cmp expect actual\n@@ -952,7 +952,7 @@ test_expect_success 'stdin -z update refs fails with wrong old value' '\n \tgit update-ref $c $m &&\n \tprintf $F \"update $a\" \"$m\" \"$m\" \"update $b\" \"$m\" \"$m\" \"update $c\" \"$m\" \"$Z\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin <stdin 2>err &&\n-\tgrep \"fatal: Cannot lock the ref '\"'\"'$c'\"'\"'\" err &&\n+\tgrep \"fatal: update_ref failed: Cannot lock the ref '\"'\"'$c'\"'\"'\" err &&\n \tgit rev-parse $m >expect &&\n \tgit rev-parse $a >actual &&\n \ttest_cmp expect actual &&\n-- \n1.9.1.521.g5dc89fa\n"},{"id":"239664","messageId":"1398442494-23438-7-git-send-email-sahlberg@google.com","threadId":"36495","inReplyTo":"1398442494-23438-1-git-send-email-sahlberg@google.com","subject":"[PATCH v3 06/19] refs.c: make update_ref_write to return error string on failure","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-25T16:14:41Z","receivedAt":"2014-04-25T16:14:41Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change update_ref_write to also return an error string on failure.\nThis makes the error avaialbel to ref_transaction_commit callers if the\ntransaction failed dur to update_ref_sha1/write_ref_sha1 failures.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n refs.c | 10 +++++++---\n 1 file changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 9d9eab8..7562151 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3253,10 +3253,14 @@ 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    char **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\t*err = xmalloc(PATH_MAX + 26);\n+\t\t\tsnprintf(*err, PATH_MAX + 26, str, refname);\n+\t\t}\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 +3386,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@@ -3472,7 +3476,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.521.g5dc89fa\n"},{"id":"239650","messageId":"1398442494-23438-8-git-send-email-sahlberg@google.com","threadId":"36495","inReplyTo":"1398442494-23438-1-git-send-email-sahlberg@google.com","subject":"[PATCH v3 07/19] refs.c: remove the onerr argument to ref_transaction_commit","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-25T16:14:42Z","receivedAt":"2014-04-25T16:14:42Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Since we now pass meaningful error messages back from ref_transaction_commit\non failures, we no longer need to provide a onerr argument. The callers can now\non commit failures die() with a meaningful, and in most cases even better than\nbefore, error message.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\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 47c9b53..df3cd92 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -368,8 +368,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(\"update_ref failed: %s\", err);\n \t\treturn 0;\n \t}\ndiff --git a/refs.c b/refs.c\nindex 7562151..0e2df81 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3397,8 +3397,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\tchar **err,\n-\t\t\t\t\tenum action_on_err onerr)\n+\t\t\t\t\tchar **err)\n {\n \tint i;\n \tfor (i = 1; i < n; i++)\n@@ -3410,22 +3409,13 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n \t\t\t\tsnprintf(*err, PATH_MAX + 41, str,\n \t\t\t\t\t updates[i]->refname);\n \t\t\t}\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, char **err,\n-\t\t\t   enum action_on_err onerr)\n+\t\t\t   const char *msg, char **err)\n {\n \tint ret = 0, delnum = 0, i;\n \tconst char **delnames;\n@@ -3443,7 +3433,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@@ -3455,7 +3445,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\tconst char *str = \"Cannot lock the ref '%s'.\";\n@@ -3476,7 +3467,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 615c73d..f3db632 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -272,8 +272,7 @@ void ref_transaction_delete(struct ref_transaction *transaction,\n  * why a commit failed. This string must be free()ed by the caller.\n  */\n int ref_transaction_commit(struct ref_transaction *transaction,\n-\t\t\t   const char *msg, char **err,\n-\t\t\t   enum action_on_err onerr);\n+\t\t\t   const char *msg, char **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.521.g5dc89fa\n"},{"id":"239654","messageId":"1398442494-23438-9-git-send-email-sahlberg@google.com","threadId":"36495","inReplyTo":"1398442494-23438-1-git-send-email-sahlberg@google.com","subject":"[PATCH v3 08/19] refs.c: change ref_transaction_update() to do error checking and return status","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-25T16:14:43Z","receivedAt":"2014-04-25T16:14:43Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Update ref_transaction_update() do some basic error checking and return\ntrue on error. Update all callers to check ref_transaction_update() for error.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n builtin/update-ref.c | 10 ++++++----\n refs.c               |  9 +++++++--\n refs.h               | 10 +++++-----\n 3 files changed, 18 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex df3cd92..13d094d 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 0e2df81..65cf407 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3334,19 +3334,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 f3db632..6a361d7 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.521.g5dc89fa\n"},{"id":"239656","messageId":"1398442494-23438-10-git-send-email-sahlberg@google.com","threadId":"36495","inReplyTo":"1398442494-23438-1-git-send-email-sahlberg@google.com","subject":"[PATCH v3 09/19] refs.c: change ref_transaction_create to do error checking and return status","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-25T16:14:44Z","receivedAt":"2014-04-25T16:14:44Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Do basic error checking in ref_transaction_create() and make it return\nstatus. Update all callers to check the result of ref_transaction_create()\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n builtin/update-ref.c |  4 +++-\n refs.c               | 17 +++++++++++------\n refs.h               |  8 ++++----\n 3 files changed, 18 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 13d094d..5f7dfc1 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 65cf407..762be85 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3354,18 +3354,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 6a361d7..e984267 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.521.g5dc89fa\n"},{"id":"239661","messageId":"1398442494-23438-11-git-send-email-sahlberg@google.com","threadId":"36495","inReplyTo":"1398442494-23438-1-git-send-email-sahlberg@google.com","subject":"[PATCH v3 10/19] refs.c: ref_transaction_delete to check for error and return status","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-25T16:14:45Z","receivedAt":"2014-04-25T16:14:45Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change ref_transaction_delete() to do basic error checking and return\nstatus. Update all callers to check the return for ref_transaction_delete()\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n builtin/update-ref.c |  5 +++--\n refs.c               | 15 ++++++++++-----\n refs.h               |  8 ++++----\n 3 files changed, 17 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 5f7dfc1..a600ab3 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 762be85..1557c3c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3373,19 +3373,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 e984267..8135131 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.521.g5dc89fa\n"},{"id":"239649","messageId":"1398442494-23438-12-git-send-email-sahlberg@google.com","threadId":"36495","inReplyTo":"1398442494-23438-1-git-send-email-sahlberg@google.com","subject":"[PATCH v3 11/19] tag.c: use ref transactions when doing updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-25T16:14:46Z","receivedAt":"2014-04-25T16:14:46Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change tag.c to use ref transactions for all ref updates.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n builtin/tag.c | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 40356e3..dd53fb4 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+\tchar *err = NULL;\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  die(_(\"%s: cannot update the ref: %s\"), ref.buf, err);\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.521.g5dc89fa\n"},{"id":"239651","messageId":"1398442494-23438-13-git-send-email-sahlberg@google.com","threadId":"36495","inReplyTo":"1398442494-23438-1-git-send-email-sahlberg@google.com","subject":"[PATCH v3 12/19] replace.c: use the ref transaction functions for updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-25T16:14:47Z","receivedAt":"2014-04-25T16:14:47Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Update replace.c to use ref transactions for updates.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n builtin/replace.c | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex b62420a..cf0f56d 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+\tchar *err = NULL;\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  die(_(\"%s: failed to replace ref: %s\"), ref, err);\n \n \treturn 0;\n }\n-- \n1.9.1.521.g5dc89fa\n"},{"id":"239662","messageId":"1398442494-23438-14-git-send-email-sahlberg@google.com","threadId":"36495","inReplyTo":"1398442494-23438-1-git-send-email-sahlberg@google.com","subject":"[PATCH v3 13/19] commit.c: use ref transactions for updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-25T16:14:48Z","receivedAt":"2014-04-25T16:14:48Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change commit.c to use ref transactions for all ref updates.\nMake sure we pass a NULL pointer to ref_transaction_update if have_old\nis false.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n builtin/commit.c | 23 ++++++++++-------------\n 1 file changed, 10 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex d9550c5..7e4c306 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+\tchar *err = NULL;\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);\n \t}\n \n \tunlink(git_path(\"CHERRY_PICK_HEAD\"));\n-- \n1.9.1.521.g5dc89fa\n"},{"id":"239663","messageId":"1398442494-23438-15-git-send-email-sahlberg@google.com","threadId":"36495","inReplyTo":"1398442494-23438-1-git-send-email-sahlberg@google.com","subject":"[PATCH v3 14/19] sequencer.c: use ref transactions for all ref updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-25T16:14:49Z","receivedAt":"2014-04-25T16:14:49Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change to use ref transactions for all updates to refs.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n sequencer.c | 22 ++++++++++++++++------\n 1 file changed, 16 insertions(+), 6 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex bde5f04..7d59f58 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -272,19 +272,29 @@ 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+\tchar *err = NULL;\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\"), err);\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.521.g5dc89fa\n"},{"id":"239665","messageId":"1398442494-23438-16-git-send-email-sahlberg@google.com","threadId":"36495","inReplyTo":"1398442494-23438-1-git-send-email-sahlberg@google.com","subject":"[PATCH v3 15/19] fast-import.c: change update_branch to use ref transactions","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-25T16:14:50Z","receivedAt":"2014-04-25T16:14:50Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change update_branch() to use ref transactions for updates.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n fast-import.c | 19 +++++++++++--------\n 1 file changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex fb4738d..a2b05fa 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1678,36 +1678,39 @@ 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+\tchar *err = NULL;\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, err);\n+\t}\n \treturn 0;\n }\n \n-- \n1.9.1.521.g5dc89fa\n"},{"id":"239667","messageId":"1398442494-23438-17-git-send-email-sahlberg@google.com","threadId":"36495","inReplyTo":"1398442494-23438-1-git-send-email-sahlberg@google.com","subject":"[PATCH v3 16/19] branch.c: use ref transaction for all ref updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-25T16:14:51Z","receivedAt":"2014-04-25T16:14:51Z","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 <sahlberg@google.com>\n---\n branch.c | 39 +++++++++++++++++++++++++--------------\n 1 file changed, 25 insertions(+), 14 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 660097b..23cde1e 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,34 @@ 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\tchar *err = NULL;\n+\n+\t\ttransaction = ref_transaction_begin();\n+\t\tif (forcing) {\n+\t\t\tif (!transaction ||\n+\t\t\t    ref_transaction_update(transaction, ref.buf, sha1,\n+\t\t\t\t\t\t   NULL, 0, 0) ||\n+\t\t\t    ref_transaction_commit(transaction, msg, &err))\n+\t\t\t  die_errno(_(\"%s: failed to write ref: %s\"),\n+\t\t\t\t    ref.buf, err);\n+\t\t} else {\n+\t\t\tif (!transaction ||\n+\t\t\t    ref_transaction_create(transaction, ref.buf, sha1,\n+\t\t\t\t\t\t   0) ||\n+\t\t\t    ref_transaction_commit(transaction, msg, &err))\n+\t\t\t  die_errno(_(\"%s: failed to write ref: %s\"),\n+\t\t\t\t    ref.buf, err);\n+\t\t}\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.521.g5dc89fa\n"},{"id":"239655","messageId":"1398442494-23438-18-git-send-email-sahlberg@google.com","threadId":"36495","inReplyTo":"1398442494-23438-1-git-send-email-sahlberg@google.com","subject":"[PATCH v3 17/19] refs.c: change update_ref to use a transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-25T16:14:52Z","receivedAt":"2014-04-25T16:14:52Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change the update_ref helper function to use a ref transaction internally.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n refs.c | 23 +++++++++++++++++++----\n 1 file changed, 19 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 1557c3c..95df4a9 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3397,11 +3397,26 @@ 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+\tchar *err = NULL;\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     const 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: error(str, refname, err); break;\n+\t\tcase UPDATE_REFS_DIE_ON_ERR: die(str, refname, err); break;\n+\t\tcase UPDATE_REFS_QUIET_ON_ERR: break;\n+\t\tfree(err);\n+\t\t}\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.521.g5dc89fa\n"},{"id":"239657","messageId":"1398442494-23438-19-git-send-email-sahlberg@google.com","threadId":"36495","inReplyTo":"1398442494-23438-1-git-send-email-sahlberg@google.com","subject":"[PATCH v3 18/19] refs.c: free the transaction before returning when number of updates is 0","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-25T16:14:53Z","receivedAt":"2014-04-25T16:14:53Z","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 <sahlberg@google.com>\n---\n refs.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 95df4a9..ffa9c83 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3452,8 +3452,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 \tif (err)\n \t\t*err = NULL;\n-- \n1.9.1.521.g5dc89fa\n"},{"id":"239652","messageId":"1398442494-23438-20-git-send-email-sahlberg@google.com","threadId":"36495","inReplyTo":"1398442494-23438-1-git-send-email-sahlberg@google.com","subject":"[PATCH v3 19/19] refs.c: pass **transaction to commit and have it clear the pointer","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-25T16:14:54Z","receivedAt":"2014-04-25T16:14:54Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change ref_transaction_commit to take a pointer to a pointer for the\ntransaction. This allows us to clear the transaction pointer from within\nref_transaction_commit so that it becomes NULL in the caller.\n\nThis makes transaction handling in the callers much nicer since instead of\nhaving to write horrible code like :\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\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n branch.c             |  4 ++--\n builtin/commit.c     |  2 +-\n builtin/replace.c    |  2 +-\n builtin/tag.c        |  2 +-\n builtin/update-ref.c |  2 +-\n fast-import.c        |  7 +++----\n refs.c               | 18 ++++++++++--------\n refs.h               |  3 ++-\n sequencer.c          |  7 +++----\n 9 files changed, 24 insertions(+), 23 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 23cde1e..5d68467 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -303,14 +303,14 @@ void create_branch(const char *head,\n \t\t\tif (!transaction ||\n \t\t\t    ref_transaction_update(transaction, ref.buf, sha1,\n \t\t\t\t\t\t   NULL, 0, 0) ||\n-\t\t\t    ref_transaction_commit(transaction, msg, &err))\n+\t\t\t    ref_transaction_commit(&transaction, msg, &err))\n \t\t\t  die_errno(_(\"%s: failed to write ref: %s\"),\n \t\t\t\t    ref.buf, err);\n \t\t} else {\n \t\t\tif (!transaction ||\n \t\t\t    ref_transaction_create(transaction, ref.buf, sha1,\n \t\t\t\t\t\t   0) ||\n-\t\t\t    ref_transaction_commit(transaction, msg, &err))\n+\t\t\t    ref_transaction_commit(&transaction, msg, &err))\n \t\t\t  die_errno(_(\"%s: failed to write ref: %s\"),\n \t\t\t\t    ref.buf, err);\n \t\t}\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 7e4c306..3142827 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1682,7 +1682,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\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    ref_transaction_commit(&transaction, sb.buf, &err)) {\n \t\trollback_index_files();\n \t\tdie(_(\"HEAD: cannot update ref: %s\"), err);\n \t}\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex cf0f56d..51e9ddf 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -162,7 +162,7 @@ static int replace_object(const char *object_ref, const char *replace_ref,\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    ref_transaction_commit(&transaction, NULL, &err))\n \t  die(_(\"%s: failed to replace ref: %s\"), ref, err);\n \n \treturn 0;\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex dd53fb4..60b57a1 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -646,7 +646,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\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    ref_transaction_commit(&transaction, NULL, &err))\n \t  die(_(\"%s: cannot update the ref: %s\"), ref.buf, err);\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));\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex a600ab3..4a0901d 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -373,7 +373,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\tif (ref_transaction_commit(&transaction, msg, &err))\n \t\t\tdie(\"update_ref failed: %s\", err);\n \t\treturn 0;\n \t}\ndiff --git a/fast-import.c b/fast-import.c\nindex a2b05fa..3ce2f47 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1703,11 +1703,10 @@ 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, err);\n \t}\ndiff --git a/refs.c b/refs.c\nindex ffa9c83..0b60250 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3401,10 +3401,10 @@ int update_ref(const char *action, const char *refname,\n \tchar *err = NULL;\n \n \tt = ref_transaction_begin();\n-\tif ((!t ||\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\t\t\t   !!oldval) ||\n+\t    ref_transaction_commit(&t, action, &err)) {\n \t     const char *str = \"update_ref failed for ref '%s': %s\";\n \n \t\tref_transaction_rollback(t);\n@@ -3444,16 +3444,17 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n \treturn 0;\n }\n \n-int ref_transaction_commit(struct ref_transaction *transaction,\n+int ref_transaction_commit(struct ref_transaction **transaction,\n \t\t\t   const char *msg, char **err)\n {\n \tint ret = 0, delnum = 0, i;\n \tconst char **delnames;\n-\tint n = transaction->nr;\n-\tstruct ref_update **updates = transaction->updates;\n+\tint n = (*transaction)->nr;\n+\tstruct ref_update **updates = (*transaction)->updates;\n \n \tif (!n) {\n-\t\tref_transaction_free(transaction);\n+\t\tref_transaction_free(*transaction);\n+\t\t*transaction = NULL;\n \t\treturn 0;\n \t}\n \n@@ -3527,7 +3528,8 @@ cleanup:\n \t\tif (updates[i]->lock)\n \t\t\tunlock_ref(updates[i]->lock);\n \tfree(delnames);\n-\tref_transaction_free(transaction);\n+\tref_transaction_free(*transaction);\n+\t*transaction = NULL;\n \treturn ret;\n }\n \ndiff --git a/refs.h b/refs.h\nindex 8135131..85127f2 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -270,8 +270,9 @@ int ref_transaction_delete(struct ref_transaction *transaction,\n  * problem.  The ref_transaction is freed by this function.\n  * If error is non-NULL it will return an error string that describes\n  * why a commit failed. This string must be free()ed by the caller.\n+ * *transaction is reset to NULL in this call.\n  */\n-int ref_transaction_commit(struct ref_transaction *transaction,\n+int ref_transaction_commit(struct ref_transaction **transaction,\n \t\t\t   const char *msg, char **err);\n \n /** Lock a ref and then write its file */\ndiff --git a/sequencer.c b/sequencer.c\nindex 7d59f58..3f6eced 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -283,11 +283,10 @@ 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\"), err);\n-- \n1.9.1.521.g5dc89fa\n"},{"id":"239713","messageId":"20140425215318.GA9218@google.com","threadId":"36495","inReplyTo":"1398442494-23438-1-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v3 00/19] Use ref transactions from most callers","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-04-25T21:53:36Z","receivedAt":"2014-04-25T21:53:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ronnie Sahlberg wrote:\n\n> This patch series changes most of the places where the ref functions for\n> locking and writing refs to instead use the new ref transaction API.\n\nThanks.  Is this series based against mh/ref-transaction from \"next\"?\n\n[...]\n> I think I have covered all issues raised on the previous reviews and also\n> done a bunch of cleanups and improvements to the transaction API.\n\nWhoops, sorry I replied to an old message.\n"},{"id":"239714","messageId":"CAL=YDWkArmAdoDeNXqRn3Mxm-_nuOMyhAcooHt8c_pr98pJJbg@mail.gmail.com","threadId":"36495","inReplyTo":"20140425215318.GA9218@google.com","subject":"Re: [PATCH v3 00/19] Use ref transactions from most callers","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-25T22:04:34Z","receivedAt":"2014-04-25T22:04:34Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Fri, Apr 25, 2014 at 2:53 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Ronnie Sahlberg wrote:\n>\n>> This patch series changes most of the places where the ref functions for\n>> locking and writing refs to instead use the new ref transaction API.\n>\n> Thanks.  Is this series based against mh/ref-transaction from \"next\"?\n\nYes, against mh/ref-transaction\n\n\n>\n> [...]\n>> I think I have covered all issues raised on the previous reviews and also\n>> done a bunch of cleanups and improvements to the transaction API.\n>\n> Whoops, sorry I replied to an old message.\n"},{"id":"239715","messageId":"20140425221001.GB9218@google.com","threadId":"36495","inReplyTo":"1398442494-23438-4-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v3 03/19] refs.c: make ref_transaction_commit return an error string","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-04-25T22:10:01Z","receivedAt":"2014-04-25T22:10:01Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ronnie Sahlberg wrote:\n\n> Let ref_transaction_commit return an optional error string that describes\n> the transaction failure.  Start by returning the same error as update_ref_lock\n> returns, modulo the initial error:/fatal: preamble.\n\ns/returns/prints/?\n\n> This will make it easier for callers to craft better error messages when\n> a transaction call fails.\n\nInteresting.  Can you give an example?  What kind of behavior are we\nexpecting in callers other than die()-ing or cleaning up and then\ndie()-ing?\n\nI like this more than having the caller pass in a flag/callback/etc to\ndecide how noisy to be and whether to gracefully accept errors or exit.\nSo it seems like an improvement, but may always returning error()\nwould be better --- more context would help in clarifying this.\n\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 error is non-NULL it will return an error string that describes\n> + * why a commit failed. This string must be free()ed by the caller.\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, char **err,\n> +\t\t\t   enum action_on_err onerr);\n\nIs the idea that if I pass in a pointer &err then\nref_transaction_commit will take the action described by onerr *and*\nwrite its error message to err?\n\nProbably squashing with patch 07 would make this easier to read (and\nwouldn't require changing any messages at that point).\n\n[...]\n> --- a/refs.c\n> +++ b/refs.c\n[...]\n> @@ -3443,6 +3447,12 @@ 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\tconst char *str = \"Cannot lock the ref '%s'.\";\n> +\t\t\t\t*err = xmalloc(PATH_MAX + 24);\n> +\t\t\t\tsnprintf(*err, PATH_MAX + 24, str,\n> +\t\t\t\t\t update->refname);\n> +\t\t\t}\n\nMight be clearer to use a helper similar to path.c::mkpathdup\n\n\tchar *aprintf(const char *fmt, ...)\n\t{\n\t\tchar *result;\n\t\tstruct strbuf sb = STRBUF_INIT;\n\t\tva_list args;\n\n\t\tva_start(args, fmt);\n\t\tstrbuf_vaddf(&sb, fmt, args);\n\t\tva_end(args);\n\n\t\treturn strbuf_detach(&sb);\n\t}\n\nor to have the caller pass in a pointer to strbuf instead of char *.\n\nThe rest looks good to me.\n\nThanks,\nJonathan\n"},{"id":"239716","messageId":"20140425221248.GC9218@google.com","threadId":"36495","inReplyTo":"1398442494-23438-5-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v3 04/19] refs.c: return error string from ref_update_reject_duplicates on failure","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-04-25T22:12:48Z","receivedAt":"2014-04-25T22:12:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ronnie Sahlberg wrote:\n\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\tchar **err,\n>  \t\t\t\t\tenum action_on_err onerr)\n>  {\n>  \tint i;\n> @@ -3400,6 +3401,11 @@ 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\t*err = xmalloc(PATH_MAX + 41);\n> +\t\t\t\tsnprintf(*err, PATH_MAX + 41, str,\n> +\t\t\t\t\t updates[i]->refname);\n> +\t\t\t}\n\nSame issues as the previous patch: it's too easy to get the buffer size\nwrong when updating the message (or, worse, when making it\ntranslatable).  aprintf or a strbuf should work better.\n\nOtherwise seems sensible.\n"},{"id":"239717","messageId":"20140425223620.GD9218@google.com","threadId":"36495","inReplyTo":"1398442494-23438-6-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v3 05/19] update-ref.c: use the error string from _commit to print better message","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-04-25T22:36:20Z","receivedAt":"2014-04-25T22:36:20Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ronnie Sahlberg wrote:\n\n> Call ref_transaction_commit with QUIET_ON_ERR and use the error string\n> that is returned to print a better log message if/after the transaction\n> fails.\n\nAh, so that's how the transition to a better API happens.  Makes sense.\n\n(A mention of QUIET_ON_ERR in the patch that introduces the &err\nparameter could help, or feel free to ignore these comments, since it's\nall well by the end of the series.)\n\n> Update the tests to reflect that the log message is now slightly different\n>   fatal: update_ref failed: Cannot lock the ref 'some ref'\n> versus from the previous\n>   fatal: Cannot lock the ref 'some ref'\n\nMakes sense as a demo of what the new code allows, but is this error\nmessage better?  The use of 'git update-ref' is an implementation\ndetail that the user doesn't need to know about.\n\nI think I'd prefer the result of plain die(\"%s\", err), even though\nthat's a no-op.\n\n[...]\n> +++ b/builtin/update-ref.c\n[...]\n> @@ -359,17 +360,18 @@ 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 (!transaction)\n> +\t\t\tdie(\"failed to update refs\");\n\nThis can't happen (xcalloc is defined to die() on malloc failure).\nIf you want to protect against it anyway, die(\"BUG: ...\")?\n\nThanks,\nJonathan\n"},{"id":"239719","messageId":"20140425224507.GE9218@google.com","threadId":"36495","inReplyTo":"1398442494-23438-7-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v3 06/19] refs.c: make update_ref_write to return error string on failure","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-04-25T22:45:07Z","receivedAt":"2014-04-25T22:45:07Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ronnie Sahlberg wrote:\n\n> Change update_ref_write to also return an error string on failure.\n> This makes the error avaialbel to ref_transaction_commit callers if the\n> transaction failed dur to update_ref_sha1/write_ref_sha1 failures.\n\nNits:\n\n * available\n * during\n\nProbably should come right after or before patch 3.  Same nit as patch\n3 about using asprintf or passing in a pointer to strbuf.\n"},{"id":"239720","messageId":"20140425224714.GF9218@google.com","threadId":"36495","inReplyTo":"1398442494-23438-8-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v3 07/19] refs.c: remove the onerr argument to ref_transaction_commit","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-04-25T22:47:14Z","receivedAt":"2014-04-25T22:47:14Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ronnie Sahlberg wrote:\n\n> Since we now pass meaningful error messages back from ref_transaction_commit\n> on failures, we no longer need to provide a onerr argument.\n\nYay!  More precisely, now that all callers use\nUPDATE_REFS_QUIET_ON_ERR there's no need to support any other\nbehavior.\n\nThanks for cleaning up the error handling here.\n"},{"id":"239723","messageId":"535AE89B.2000408@alum.mit.edu","threadId":"36495","inReplyTo":"1398442494-23438-12-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v3 11/19] tag.c: use ref transactions when doing updates","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-25T22:58:35Z","receivedAt":"2014-04-25T22:58:35Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:\n> Change tag.c to use ref transactions for all ref updates.\n> \n> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n> ---\n>  builtin/tag.c | 14 ++++++++------\n>  1 file changed, 8 insertions(+), 6 deletions(-)\n> \n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index 40356e3..dd53fb4 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> +\tchar *err = NULL;\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  die(_(\"%s: cannot update the ref: %s\"), ref.buf, err);\n\nFormatting nit: die() should be indented by two TABs.\n\n>  \tif (force && !is_null_sha1(prev) && hashcmp(prev, object))\n>  \t\tprintf(_(\"Updated tag '%s' (was %s)\\n\"), tag, find_unique_abbrev(prev, DEFAULT_ABBREV));\n>  \n> \n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"239725","messageId":"535AE8FF.80509@alum.mit.edu","threadId":"36495","inReplyTo":"1398442494-23438-13-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v3 12/19] replace.c: use the ref transaction functions for updates","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-25T23:00:15Z","receivedAt":"2014-04-25T23:00:15Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:\n> Update replace.c to use ref transactions for updates.\n> \n> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n> ---\n>  builtin/replace.c | 14 ++++++++------\n>  1 file changed, 8 insertions(+), 6 deletions(-)\n> \n> diff --git a/builtin/replace.c b/builtin/replace.c\n> index b62420a..cf0f56d 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> +\tchar *err = NULL;\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  die(_(\"%s: failed to replace ref: %s\"), ref, err);\n\ndie() should be indented by two TABs.\n\n>  \n>  \treturn 0;\n>  }\n> \n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"239728","messageId":"535AECC5.3090100@alum.mit.edu","threadId":"36495","inReplyTo":"1398442494-23438-17-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v3 16/19] branch.c: use ref transaction for all ref updates","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-25T23:16:21Z","receivedAt":"2014-04-25T23:16:21Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:\n> Change create_branch to use a ref transaction when creating the new branch.\n> ref_transaction_create will check that the ref does not already exist and fail\n> otherwise meaning that we no longer need to keep a lock on the ref during the\n> setup_tracking. This simplifies the code since we can now do the transaction\n> in one single step.\n> \n> If the forcing flag is false then use ref_transaction_create since this will\n> fail if the ref already exist. Otherwise use ref_transaction_update.\n> \n> This also fixes a race condition in the old code where two concurrent\n> create_branch could race since the lock_any_ref_for_update/write_ref_sha1\n> did not protect against the ref already existsing. I.e. one thread could end up\n> overwriting a branch even if the forcing flag is false.\n> \n> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n> ---\n>  branch.c | 39 +++++++++++++++++++++++++--------------\n>  1 file changed, 25 insertions(+), 14 deletions(-)\n> \n> diff --git a/branch.c b/branch.c\n> index 660097b..23cde1e 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,34 @@ 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\tchar *err = NULL;\n> +\n> +\t\ttransaction = ref_transaction_begin();\n> +\t\tif (forcing) {\n> +\t\t\tif (!transaction ||\n> +\t\t\t    ref_transaction_update(transaction, ref.buf, sha1,\n> +\t\t\t\t\t\t   NULL, 0, 0) ||\n> +\t\t\t    ref_transaction_commit(transaction, msg, &err))\n> +\t\t\t  die_errno(_(\"%s: failed to write ref: %s\"),\n> +\t\t\t\t    ref.buf, err);\n> +\t\t} else {\n> +\t\t\tif (!transaction ||\n> +\t\t\t    ref_transaction_create(transaction, ref.buf, sha1,\n> +\t\t\t\t\t\t   0) ||\n> +\t\t\t    ref_transaction_commit(transaction, msg, &err))\n> +\t\t\t  die_errno(_(\"%s: failed to write ref: %s\"),\n> +\t\t\t\t    ref.buf, err);\n> +\t\t}\n\nYou've got some indentation problems above.\n\nBut actually, there seems like a lot of duplicated code here.  Couldn't\nyou instead do a single block with have_old set based on forcing:\n\n    ref_transaction_update(transaction, ref.buf, sha1,\n\t\t\t   null_sha1, 0, !forcing)\n\n?\n\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> \n\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"239731","messageId":"535AEFAF.9000908@alum.mit.edu","threadId":"36495","inReplyTo":"1398442494-23438-18-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v3 17/19] refs.c: change update_ref to use a transaction","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-25T23:28:47Z","receivedAt":"2014-04-25T23:28:47Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:\n> Change the update_ref helper function to use a ref transaction internally.\n> \n> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n> ---\n>  refs.c | 23 +++++++++++++++++++----\n>  1 file changed, 19 insertions(+), 4 deletions(-)\n> \n> diff --git a/refs.c b/refs.c\n> index 1557c3c..95df4a9 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -3397,11 +3397,26 @@ 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> +\tchar *err = NULL;\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\nYou seem to have extra parentheses around the first || subexpression.\n\nYou also don't need the parentheses around the && expression because &&\nbinds more tightly than ||.\n\nBut using \"!(t = NULL)\" in the middle of a conditional expression is\npretty obscure.  I think you will change this in a later patch, so I\nwill defer my comments.\n\n> +\t     const char *str = \"update_ref failed for ref '%s': %s\";\n\nIndentation error.\n\n> +\n> +\t\tref_transaction_rollback(t);\n> +\t\tswitch (onerr) {\n> +\t\tcase UPDATE_REFS_MSG_ON_ERR: error(str, refname, err); break;\n> +\t\tcase UPDATE_REFS_DIE_ON_ERR: die(str, refname, err); break;\n> +\t\tcase UPDATE_REFS_QUIET_ON_ERR: break;\n> +\t\tfree(err);\n> +\t\t}\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> \n\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"239735","messageId":"535B0C6F.509@alum.mit.edu","threadId":"36495","inReplyTo":"1398442494-23438-20-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v3 19/19] refs.c: pass **transaction to commit and have it clear the pointer","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-26T01:31:27Z","receivedAt":"2014-04-26T01:31:27Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:\n> Change ref_transaction_commit to take a pointer to a pointer for the\n> transaction. This allows us to clear the transaction pointer from within\n> ref_transaction_commit so that it becomes NULL in the caller.\n> \n> This makes transaction handling in the callers much nicer since instead of\n> having to write horrible code like :\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> \n> we 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\nI understand the motivation for this change, but passing\npointer-to-pointer is unconventional in a case like this.  Unfortunately\nI ran out of steam for the night before I could think about alternatives.\n\nRegarding patches 01-18, I agree with Jonathan's comments and made a few\nof my own.  I definitely don't think that code like \"!(t = NULL)\" should\nstay in our code base as more than a transitional state.  But I'd have\nto play with the alternatives before I can form an opinion on your\nsuggestion in this patch.\n\nThanks for working on this!\n\nMichael\n\n> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n> ---\n>  branch.c             |  4 ++--\n>  builtin/commit.c     |  2 +-\n>  builtin/replace.c    |  2 +-\n>  builtin/tag.c        |  2 +-\n>  builtin/update-ref.c |  2 +-\n>  fast-import.c        |  7 +++----\n>  refs.c               | 18 ++++++++++--------\n>  refs.h               |  3 ++-\n>  sequencer.c          |  7 +++----\n>  9 files changed, 24 insertions(+), 23 deletions(-)\n> \n> diff --git a/branch.c b/branch.c\n> index 23cde1e..5d68467 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -303,14 +303,14 @@ void create_branch(const char *head,\n>  \t\t\tif (!transaction ||\n>  \t\t\t    ref_transaction_update(transaction, ref.buf, sha1,\n>  \t\t\t\t\t\t   NULL, 0, 0) ||\n> -\t\t\t    ref_transaction_commit(transaction, msg, &err))\n> +\t\t\t    ref_transaction_commit(&transaction, msg, &err))\n>  \t\t\t  die_errno(_(\"%s: failed to write ref: %s\"),\n>  \t\t\t\t    ref.buf, err);\n>  \t\t} else {\n>  \t\t\tif (!transaction ||\n>  \t\t\t    ref_transaction_create(transaction, ref.buf, sha1,\n>  \t\t\t\t\t\t   0) ||\n> -\t\t\t    ref_transaction_commit(transaction, msg, &err))\n> +\t\t\t    ref_transaction_commit(&transaction, msg, &err))\n>  \t\t\t  die_errno(_(\"%s: failed to write ref: %s\"),\n>  \t\t\t\t    ref.buf, err);\n>  \t\t}\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 7e4c306..3142827 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1682,7 +1682,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\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    ref_transaction_commit(&transaction, sb.buf, &err)) {\n>  \t\trollback_index_files();\n>  \t\tdie(_(\"HEAD: cannot update ref: %s\"), err);\n>  \t}\n> diff --git a/builtin/replace.c b/builtin/replace.c\n> index cf0f56d..51e9ddf 100644\n> --- a/builtin/replace.c\n> +++ b/builtin/replace.c\n> @@ -162,7 +162,7 @@ static int replace_object(const char *object_ref, const char *replace_ref,\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    ref_transaction_commit(&transaction, NULL, &err))\n>  \t  die(_(\"%s: failed to replace ref: %s\"), ref, err);\n>  \n>  \treturn 0;\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index dd53fb4..60b57a1 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -646,7 +646,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\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    ref_transaction_commit(&transaction, NULL, &err))\n>  \t  die(_(\"%s: cannot update the ref: %s\"), ref.buf, err);\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> diff --git a/builtin/update-ref.c b/builtin/update-ref.c\n> index a600ab3..4a0901d 100644\n> --- a/builtin/update-ref.c\n> +++ b/builtin/update-ref.c\n> @@ -373,7 +373,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\tif (ref_transaction_commit(&transaction, msg, &err))\n>  \t\t\tdie(\"update_ref failed: %s\", err);\n>  \t\treturn 0;\n>  \t}\n> diff --git a/fast-import.c b/fast-import.c\n> index a2b05fa..3ce2f47 100644\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -1703,11 +1703,10 @@ 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, err);\n>  \t}\n> diff --git a/refs.c b/refs.c\n> index ffa9c83..0b60250 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -3401,10 +3401,10 @@ int update_ref(const char *action, const char *refname,\n>  \tchar *err = NULL;\n>  \n>  \tt = ref_transaction_begin();\n> -\tif ((!t ||\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\t\t\t   !!oldval) ||\n> +\t    ref_transaction_commit(&t, action, &err)) {\n>  \t     const char *str = \"update_ref failed for ref '%s': %s\";\n>  \n>  \t\tref_transaction_rollback(t);\n> @@ -3444,16 +3444,17 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n>  \treturn 0;\n>  }\n>  \n> -int ref_transaction_commit(struct ref_transaction *transaction,\n> +int ref_transaction_commit(struct ref_transaction **transaction,\n>  \t\t\t   const char *msg, char **err)\n>  {\n>  \tint ret = 0, delnum = 0, i;\n>  \tconst char **delnames;\n> -\tint n = transaction->nr;\n> -\tstruct ref_update **updates = transaction->updates;\n> +\tint n = (*transaction)->nr;\n> +\tstruct ref_update **updates = (*transaction)->updates;\n>  \n>  \tif (!n) {\n> -\t\tref_transaction_free(transaction);\n> +\t\tref_transaction_free(*transaction);\n> +\t\t*transaction = NULL;\n>  \t\treturn 0;\n>  \t}\n>  \n> @@ -3527,7 +3528,8 @@ cleanup:\n>  \t\tif (updates[i]->lock)\n>  \t\t\tunlock_ref(updates[i]->lock);\n>  \tfree(delnames);\n> -\tref_transaction_free(transaction);\n> +\tref_transaction_free(*transaction);\n> +\t*transaction = NULL;\n>  \treturn ret;\n>  }\n>  \n> diff --git a/refs.h b/refs.h\n> index 8135131..85127f2 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -270,8 +270,9 @@ int ref_transaction_delete(struct ref_transaction *transaction,\n>   * problem.  The ref_transaction is freed by this function.\n>   * If error is non-NULL it will return an error string that describes\n>   * why a commit failed. This string must be free()ed by the caller.\n> + * *transaction is reset to NULL in this call.\n>   */\n> -int ref_transaction_commit(struct ref_transaction *transaction,\n> +int ref_transaction_commit(struct ref_transaction **transaction,\n>  \t\t\t   const char *msg, char **err);\n>  \n>  /** Lock a ref and then write its file */\n> diff --git a/sequencer.c b/sequencer.c\n> index 7d59f58..3f6eced 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -283,11 +283,10 @@ 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\"), err);\n> \n\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"239951","messageId":"CAL=YDWnD74eZ8teXsY1RfWj9ZTvmNmhYa1+3SiDBo+gCRTqf7w@mail.gmail.com","threadId":"36495","inReplyTo":"20140425223620.GD9218@google.com","subject":"Re: [PATCH v3 05/19] update-ref.c: use the error string from _commit to print better message","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T15:11:48Z","receivedAt":"2014-04-28T15:11:48Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Good ideas.\nApplied, thanks.\n\nOn Fri, Apr 25, 2014 at 3:36 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Ronnie Sahlberg wrote:\n>\n>> Call ref_transaction_commit with QUIET_ON_ERR and use the error string\n>> that is returned to print a better log message if/after the transaction\n>> fails.\n>\n> Ah, so that's how the transition to a better API happens.  Makes sense.\n>\n> (A mention of QUIET_ON_ERR in the patch that introduces the &err\n> parameter could help, or feel free to ignore these comments, since it's\n> all well by the end of the series.)\n>\n>> Update the tests to reflect that the log message is now slightly different\n>>   fatal: update_ref failed: Cannot lock the ref 'some ref'\n>> versus from the previous\n>>   fatal: Cannot lock the ref 'some ref'\n>\n> Makes sense as a demo of what the new code allows, but is this error\n> message better?  The use of 'git update-ref' is an implementation\n> detail that the user doesn't need to know about.\n>\n> I think I'd prefer the result of plain die(\"%s\", err), even though\n> that's a no-op.\n>\n> [...]\n>> +++ b/builtin/update-ref.c\n> [...]\n>> @@ -359,17 +360,18 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n>>               die(\"Refusing to perform update with empty message.\");\n>>\n>>       if (read_stdin) {\n>> -             int ret;\n>>               transaction = ref_transaction_begin();\n>> -\n>> +             if (!transaction)\n>> +                     die(\"failed to update refs\");\n>\n> This can't happen (xcalloc is defined to die() on malloc failure).\n> If you want to protect against it anyway, die(\"BUG: ...\")?\n>\n> Thanks,\n> Jonathan\n"},{"id":"239964","messageId":"CAL=YDWk_7+GGocnvdf314xHnYEUgWGxzndbFQmOoQ927zob8Vg@mail.gmail.com","threadId":"36495","inReplyTo":"535AE89B.2000408@alum.mit.edu","subject":"Re: [PATCH v3 11/19] tag.c: use ref transactions when doing updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T15:15:23Z","receivedAt":"2014-04-28T15:15:23Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Done.\n\nThanks.\n\nOn Fri, Apr 25, 2014 at 3:58 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:\n>> Change tag.c to use ref transactions for all ref updates.\n>>\n>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n>> ---\n>>  builtin/tag.c | 14 ++++++++------\n>>  1 file changed, 8 insertions(+), 6 deletions(-)\n>>\n>> diff --git a/builtin/tag.c b/builtin/tag.c\n>> index 40356e3..dd53fb4 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>>       struct strbuf ref = STRBUF_INIT;\n>>       unsigned char object[20], prev[20];\n>>       const char *object_ref, *tag;\n>> -     struct ref_lock *lock;\n>>       struct create_tag_options opt;\n>>       char *cleanup_arg = NULL;\n>>       int annotate = 0, force = 0, lines = -1;\n>> @@ -496,6 +495,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>>       const char *msgfile = NULL, *keyid = NULL;\n>>       struct msg_arg msg = { 0, STRBUF_INIT };\n>>       struct commit_list *with_commit = NULL;\n>> +     struct ref_transaction *transaction;\n>> +     char *err = NULL;\n>>       struct option options[] = {\n>>               OPT_CMDMODE('l', \"list\", &cmdmode, N_(\"list tag names\"), 'l'),\n>>               { OPTION_INTEGER, 'n', NULL, &lines, N_(\"n\"),\n>> @@ -641,11 +642,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>>       if (annotate)\n>>               create_tag(object, tag, &buf, &opt, prev, object);\n>>\n>> -     lock = lock_any_ref_for_update(ref.buf, prev, 0, NULL);\n>> -     if (!lock)\n>> -             die(_(\"%s: cannot lock the ref\"), ref.buf);\n>> -     if (write_ref_sha1(lock, object, NULL) < 0)\n>> -             die(_(\"%s: cannot update the ref\"), ref.buf);\n>> +     transaction = ref_transaction_begin();\n>> +     if (!transaction ||\n>> +         ref_transaction_update(transaction, ref.buf, object, prev,\n>> +                                0, !is_null_sha1(prev)) ||\n>> +         ref_transaction_commit(transaction, NULL, &err))\n>> +       die(_(\"%s: cannot update the ref: %s\"), ref.buf, err);\n>\n> Formatting nit: die() should be indented by two TABs.\n>\n>>       if (force && !is_null_sha1(prev) && hashcmp(prev, object))\n>>               printf(_(\"Updated tag '%s' (was %s)\\n\"), tag, find_unique_abbrev(prev, DEFAULT_ABBREV));\n>>\n>>\n>\n> Michael\n>\n> --\n> Michael Haggerty\n> mhagger@alum.mit.edu\n> http://softwareswirl.blogspot.com/\n"},{"id":"239953","messageId":"CAL=YDW=P5ZyXTsnd3VumCrVdH2G1qhMxyJpgkgJJ0Tn50VViaA@mail.gmail.com","threadId":"36495","inReplyTo":"535AE8FF.80509@alum.mit.edu","subject":"Re: [PATCH v3 12/19] replace.c: use the ref transaction functions for updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T15:17:19Z","receivedAt":"2014-04-28T15:17:19Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Done.\n\nThanks.\n\nOn Fri, Apr 25, 2014 at 4:00 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:\n>> Update replace.c to use ref transactions for updates.\n>>\n>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n>> ---\n>>  builtin/replace.c | 14 ++++++++------\n>>  1 file changed, 8 insertions(+), 6 deletions(-)\n>>\n>> diff --git a/builtin/replace.c b/builtin/replace.c\n>> index b62420a..cf0f56d 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>> +     char *err = NULL;\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);\n>\n> die() should be indented by two TABs.\n>\n>>\n>>       return 0;\n>>  }\n>>\n>\n> Michael\n>\n> --\n> Michael Haggerty\n> mhagger@alum.mit.edu\n> http://softwareswirl.blogspot.com/\n"},{"id":"240002","messageId":"CAL=YDW=CBnWU3t05MwAYkGP+0cLPdVqypM-jjZhCzUiWEjCwsA@mail.gmail.com","threadId":"36495","inReplyTo":"20140425221001.GB9218@google.com","subject":"Re: [PATCH v3 03/19] refs.c: make ref_transaction_commit return an error string","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T17:46:30Z","receivedAt":"2014-04-28T17:46:30Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Good points.\n\n\n\nOn Fri, Apr 25, 2014 at 3:10 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Ronnie Sahlberg wrote:\n>\n>> Let ref_transaction_commit return an optional error string that describes\n>> the transaction failure.  Start by returning the same error as update_ref_lock\n>> returns, modulo the initial error:/fatal: preamble.\n>\n> s/returns/prints/?\n\nDone, and then was deleted when I reworded the message.\n\n>\n>> This will make it easier for callers to craft better error messages when\n>> a transaction call fails.\n>\n> Interesting.  Can you give an example?  What kind of behavior are we\n> expecting in callers other than die()-ing or cleaning up and then\n> die()-ing?\n\nI was thinking a bit too far ahead. You could in theory keep logging multiple\nlock failures during the _commit() and then when the transaction fails\nand returns\nit will have appended a list of all refs that failed and not just the\nfirst ref that failed.\n\n\n>\n> I like this more than having the caller pass in a flag/callback/etc to\n> decide how noisy to be and whether to gracefully accept errors or exit.\n> So it seems like an improvement, but may always returning error()\n> would be better --- more context would help in clarifying this.\n>\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 error is non-NULL it will return an error string that describes\n>> + * why a commit failed. This string must be free()ed by the caller.\n>>   */\n>>  int ref_transaction_commit(struct ref_transaction *transaction,\n>> -                        const char *msg, enum action_on_err onerr);\n>> +                        const char *msg, char **err,\n>> +                        enum action_on_err onerr);\n>\n> Is the idea that if I pass in a pointer &err then\n> ref_transaction_commit will take the action described by onerr *and*\n> write its error message to err?\n\nTemporarily, yes.\nShortly after this patch I remove the onerr argument completely.\nBut I want to keep the \"pass error back to caller\" and \"get rid of\nonerr\" as two separate patches.\nI think it is easier to follow the flow of changes if they are done in\ntwo separate steps.\n\n>\n> Probably squashing with patch 07 would make this easier to read (and\n> wouldn't require changing any messages at that point).\n\nSee above.\n\n>\n> [...]\n>> --- a/refs.c\n>> +++ b/refs.c\n> [...]\n>> @@ -3443,6 +3447,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n>>                                              update->flags,\n>>                                              &update->type, onerr);\n>>               if (!update->lock) {\n>> +                     if (err) {\n>> +                             const char *str = \"Cannot lock the ref '%s'.\";\n>> +                             *err = xmalloc(PATH_MAX + 24);\n>> +                             snprintf(*err, PATH_MAX + 24, str,\n>> +                                      update->refname);\n>> +                     }\n>\n> Might be clearer to use a helper similar to path.c::mkpathdup\n>\n>         char *aprintf(const char *fmt, ...)\n>         {\n>                 char *result;\n>                 struct strbuf sb = STRBUF_INIT;\n>                 va_list args;\n>\n>                 va_start(args, fmt);\n>                 strbuf_vaddf(&sb, fmt, args);\n>                 va_end(args);\n>\n>                 return strbuf_detach(&sb);\n>         }\n>\n> or to have the caller pass in a pointer to strbuf instead of char *.\n\nstrbuf as argument is probably the right thing to do. I am doing that change.\n\n>\n> The rest looks good to me.\n>\n> Thanks,\n> Jonathan\n"},{"id":"240021","messageId":"CAL=YDWkSdiUd-6A60ncGaDrFV2pc5WtRMv8iCSHHqFLkKH=pfw@mail.gmail.com","threadId":"36495","inReplyTo":"535B0C6F.509@alum.mit.edu","subject":"Re: [PATCH v3 19/19] refs.c: pass **transaction to commit and have it clear the pointer","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T17:59:19Z","receivedAt":"2014-04-28T17:59:19Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Fri, Apr 25, 2014 at 6:31 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:\n>> Change ref_transaction_commit to take a pointer to a pointer for the\n>> transaction. This allows us to clear the transaction pointer from within\n>> ref_transaction_commit so that it becomes NULL in the caller.\n>>\n>> This makes transaction handling in the callers much nicer since instead of\n>> having to write horrible code like :\n>>       t = ref_transaction_begin();\n>>       if ((!t ||\n>>           ref_transaction_update(t, refname, sha1, oldval, flags,\n>>                                  !!oldval)) ||\n>>           (ref_transaction_commit(t, action, &err) && !(t = NULL))) {\n>>               ref_transaction_rollback(t);\n>>\n>> we can now just do the much nicer\n>>       t = ref_transaction_begin();\n>>       if (!t ||\n>>           ref_transaction_update(t, refname, sha1, oldval, flags,\n>>                                  !!oldval) ||\n>>           ref_transaction_commit(&t, action, &err)) {\n>>               ref_transaction_rollback(t);\n>\n> I understand the motivation for this change, but passing\n> pointer-to-pointer is unconventional in a case like this.  Unfortunately\n> I ran out of steam for the night before I could think about alternatives.\n\nI see.\nYes passing a pointer to pointer is not ideal.\nBut I still want to be able to use the pattern\n       t = ref_transaction_begin();\n       if (!t ||\n           ref_transaction_update(t, ...) ||\n           ref_transaction_commit(t, ...)) {\n               ref_transaction_rollback(t);\n\nMaybe the problem is that ref_transaction_commit() implicitely also\nfrees the transaction.\n\n\nWhat about changing ref_transaction_commit() would NOT free the\ntransaction and thus a caller would\nalways have to explicitely free the transaction afterwards?\n\nSomething like this :\n       t = ref_transaction_begin();\n       if (!t ||\n           ref_transaction_update(t, ...) ||\n           ref_transaction_commit(&t, ...)) {\n               ref_transaction_rollback(t);\n       }\n       ref_transaction_free(t);\n\n\n>\n> Regarding patches 01-18, I agree with Jonathan's comments and made a few\n> of my own.  I definitely don't think that code like \"!(t = NULL)\" should\n> stay in our code base as more than a transitional state.  But I'd have\n> to play with the alternatives before I can form an opinion on your\n> suggestion in this patch.\n>\n> Thanks for working on this!\n>\n> Michael\n>\n>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n>> ---\n>>  branch.c             |  4 ++--\n>>  builtin/commit.c     |  2 +-\n>>  builtin/replace.c    |  2 +-\n>>  builtin/tag.c        |  2 +-\n>>  builtin/update-ref.c |  2 +-\n>>  fast-import.c        |  7 +++----\n>>  refs.c               | 18 ++++++++++--------\n>>  refs.h               |  3 ++-\n>>  sequencer.c          |  7 +++----\n>>  9 files changed, 24 insertions(+), 23 deletions(-)\n>>\n>> diff --git a/branch.c b/branch.c\n>> index 23cde1e..5d68467 100644\n>> --- a/branch.c\n>> +++ b/branch.c\n>> @@ -303,14 +303,14 @@ void create_branch(const char *head,\n>>                       if (!transaction ||\n>>                           ref_transaction_update(transaction, ref.buf, sha1,\n>>                                                  NULL, 0, 0) ||\n>> -                         ref_transaction_commit(transaction, msg, &err))\n>> +                         ref_transaction_commit(&transaction, msg, &err))\n>>                         die_errno(_(\"%s: failed to write ref: %s\"),\n>>                                   ref.buf, err);\n>>               } else {\n>>                       if (!transaction ||\n>>                           ref_transaction_create(transaction, ref.buf, sha1,\n>>                                                  0) ||\n>> -                         ref_transaction_commit(transaction, msg, &err))\n>> +                         ref_transaction_commit(&transaction, msg, &err))\n>>                         die_errno(_(\"%s: failed to write ref: %s\"),\n>>                                   ref.buf, err);\n>>               }\n>> diff --git a/builtin/commit.c b/builtin/commit.c\n>> index 7e4c306..3142827 100644\n>> --- a/builtin/commit.c\n>> +++ b/builtin/commit.c\n>> @@ -1682,7 +1682,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>>                                  current_head ?\n>>                                  current_head->object.sha1 : NULL,\n>>                                  0, !!current_head) ||\n>> -         ref_transaction_commit(transaction, sb.buf, &err)) {\n>> +         ref_transaction_commit(&transaction, sb.buf, &err)) {\n>>               rollback_index_files();\n>>               die(_(\"HEAD: cannot update ref: %s\"), err);\n>>       }\n>> diff --git a/builtin/replace.c b/builtin/replace.c\n>> index cf0f56d..51e9ddf 100644\n>> --- a/builtin/replace.c\n>> +++ b/builtin/replace.c\n>> @@ -162,7 +162,7 @@ static int replace_object(const char *object_ref, const char *replace_ref,\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>> +         ref_transaction_commit(&transaction, NULL, &err))\n>>         die(_(\"%s: failed to replace ref: %s\"), ref, err);\n>>\n>>       return 0;\n>> diff --git a/builtin/tag.c b/builtin/tag.c\n>> index dd53fb4..60b57a1 100644\n>> --- a/builtin/tag.c\n>> +++ b/builtin/tag.c\n>> @@ -646,7 +646,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>>       if (!transaction ||\n>>           ref_transaction_update(transaction, ref.buf, object, prev,\n>>                                  0, !is_null_sha1(prev)) ||\n>> -         ref_transaction_commit(transaction, NULL, &err))\n>> +         ref_transaction_commit(&transaction, NULL, &err))\n>>         die(_(\"%s: cannot update the ref: %s\"), ref.buf, err);\n>>       if (force && !is_null_sha1(prev) && hashcmp(prev, object))\n>>               printf(_(\"Updated tag '%s' (was %s)\\n\"), tag, find_unique_abbrev(prev, DEFAULT_ABBREV));\n>> diff --git a/builtin/update-ref.c b/builtin/update-ref.c\n>> index a600ab3..4a0901d 100644\n>> --- a/builtin/update-ref.c\n>> +++ b/builtin/update-ref.c\n>> @@ -373,7 +373,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n>>               if (end_null)\n>>                       line_termination = '\\0';\n>>               update_refs_stdin();\n>> -             if (ref_transaction_commit(transaction, msg, &err))\n>> +             if (ref_transaction_commit(&transaction, msg, &err))\n>>                       die(\"update_ref failed: %s\", err);\n>>               return 0;\n>>       }\n>> diff --git a/fast-import.c b/fast-import.c\n>> index a2b05fa..3ce2f47 100644\n>> --- a/fast-import.c\n>> +++ b/fast-import.c\n>> @@ -1703,11 +1703,10 @@ static int update_branch(struct branch *b)\n>>               }\n>>       }\n>>       transaction = ref_transaction_begin();\n>> -     if ((!transaction ||\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>> +                                0, 1) ||\n>> +         ref_transaction_commit(&transaction, msg, &err)) {\n>>               ref_transaction_rollback(transaction);\n>>               return error(\"Unable to update branch %s: %s\", b->name, err);\n>>       }\n>> diff --git a/refs.c b/refs.c\n>> index ffa9c83..0b60250 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -3401,10 +3401,10 @@ int update_ref(const char *action, const char *refname,\n>>       char *err = NULL;\n>>\n>>       t = ref_transaction_begin();\n>> -     if ((!t ||\n>> +     if (!t ||\n>>           ref_transaction_update(t, refname, sha1, oldval, flags,\n>> -                                !!oldval)) ||\n>> -         (ref_transaction_commit(t, action, &err) && !(t = NULL))) {\n>> +                                !!oldval) ||\n>> +         ref_transaction_commit(&t, action, &err)) {\n>>            const char *str = \"update_ref failed for ref '%s': %s\";\n>>\n>>               ref_transaction_rollback(t);\n>> @@ -3444,16 +3444,17 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n>>       return 0;\n>>  }\n>>\n>> -int ref_transaction_commit(struct ref_transaction *transaction,\n>> +int ref_transaction_commit(struct ref_transaction **transaction,\n>>                          const char *msg, char **err)\n>>  {\n>>       int ret = 0, delnum = 0, i;\n>>       const char **delnames;\n>> -     int n = transaction->nr;\n>> -     struct ref_update **updates = transaction->updates;\n>> +     int n = (*transaction)->nr;\n>> +     struct ref_update **updates = (*transaction)->updates;\n>>\n>>       if (!n) {\n>> -             ref_transaction_free(transaction);\n>> +             ref_transaction_free(*transaction);\n>> +             *transaction = NULL;\n>>               return 0;\n>>       }\n>>\n>> @@ -3527,7 +3528,8 @@ cleanup:\n>>               if (updates[i]->lock)\n>>                       unlock_ref(updates[i]->lock);\n>>       free(delnames);\n>> -     ref_transaction_free(transaction);\n>> +     ref_transaction_free(*transaction);\n>> +     *transaction = NULL;\n>>       return ret;\n>>  }\n>>\n>> diff --git a/refs.h b/refs.h\n>> index 8135131..85127f2 100644\n>> --- a/refs.h\n>> +++ b/refs.h\n>> @@ -270,8 +270,9 @@ int ref_transaction_delete(struct ref_transaction *transaction,\n>>   * problem.  The ref_transaction is freed by this function.\n>>   * If error is non-NULL it will return an error string that describes\n>>   * why a commit failed. This string must be free()ed by the caller.\n>> + * *transaction is reset to NULL in this call.\n>>   */\n>> -int ref_transaction_commit(struct ref_transaction *transaction,\n>> +int ref_transaction_commit(struct ref_transaction **transaction,\n>>                          const char *msg, char **err);\n>>\n>>  /** Lock a ref and then write its file */\n>> diff --git a/sequencer.c b/sequencer.c\n>> index 7d59f58..3f6eced 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -283,11 +283,10 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n>>       strbuf_addf(&sb, \"%s: fast-forward\", action_name(opts));\n>>\n>>       transaction = ref_transaction_begin();\n>> -     if ((!transaction ||\n>> +     if (!transaction ||\n>>           ref_transaction_update(transaction, \"HEAD\", to, from,\n>> -                                0, !unborn)) ||\n>> -         (ref_transaction_commit(transaction, sb.buf, &err) &&\n>> -          !(transaction = NULL))) {\n>> +                                0, !unborn) ||\n>> +         ref_transaction_commit(&transaction, sb.buf, &err)) {\n>>               ref_transaction_rollback(transaction);\n>>               strbuf_release(&sb);\n>>               return error(_(\"HEAD: Could not fast-forward: %s\\n\"), err);\n>>\n>\n>\n> --\n> Michael Haggerty\n> mhagger@alum.mit.edu\n> http://softwareswirl.blogspot.com/\n"},{"id":"239993","messageId":"CAL=YDWkTcb327q9oqC1Ypm8n6tBgFnGsn2jiAK+fRNq1q74=8g@mail.gmail.com","threadId":"36495","inReplyTo":"20140425221248.GC9218@google.com","subject":"Re: [PATCH v3 04/19] refs.c: return error string from ref_update_reject_duplicates on failure","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T18:23:01Z","receivedAt":"2014-04-28T18:23:01Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Thanks.\n\nReplaced with strbuf_addf() instead.\n\nOn Fri, Apr 25, 2014 at 3:12 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Ronnie Sahlberg wrote:\n>\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>> +                                     char **err,\n>>                                       enum action_on_err onerr)\n>>  {\n>>       int i;\n>> @@ -3400,6 +3401,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n>>               if (!strcmp(updates[i - 1]->refname, updates[i]->refname)) {\n>>                       const char *str =\n>>                               \"Multiple updates for ref '%s' not allowed.\";\n>> +                     if (err) {\n>> +                             *err = xmalloc(PATH_MAX + 41);\n>> +                             snprintf(*err, PATH_MAX + 41, str,\n>> +                                      updates[i]->refname);\n>> +                     }\n>\n> Same issues as the previous patch: it's too easy to get the buffer size\n> wrong when updating the message (or, worse, when making it\n> translatable).  aprintf or a strbuf should work better.\n>\n> Otherwise seems sensible.\n"},{"id":"239995","messageId":"CAL=YDW=uavcAD==aES8zwzTstDo17paiRBhRYmYB0HkfMo8wRg@mail.gmail.com","threadId":"36495","inReplyTo":"20140425224507.GE9218@google.com","subject":"Re: [PATCH v3 06/19] refs.c: make update_ref_write to return error string on failure","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T18:23:24Z","receivedAt":"2014-04-28T18:23:24Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Done, thanks\n\nOn Fri, Apr 25, 2014 at 3:45 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Ronnie Sahlberg wrote:\n>\n>> Change update_ref_write to also return an error string on failure.\n>> This makes the error avaialbel to ref_transaction_commit callers if the\n>> transaction failed dur to update_ref_sha1/write_ref_sha1 failures.\n>\n> Nits:\n>\n>  * available\n>  * during\n>\n> Probably should come right after or before patch 3.  Same nit as patch\n> 3 about using asprintf or passing in a pointer to strbuf.\n"},{"id":"240004","messageId":"CAL=YDWkEWjeTZi6QOzosiaJ1ZA5p6zcD2asbJ4o8NW=4EHxhVQ@mail.gmail.com","threadId":"36495","inReplyTo":"20140425224714.GF9218@google.com","subject":"Re: [PATCH v3 07/19] refs.c: remove the onerr argument to ref_transaction_commit","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T18:27:15Z","receivedAt":"2014-04-28T18:27:15Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Thanks.\n\nReworded it that we remove it since all callers now use QUIET_ON_ERR\n\nOn Fri, Apr 25, 2014 at 3:47 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Ronnie Sahlberg wrote:\n>\n>> Since we now pass meaningful error messages back from ref_transaction_commit\n>> on failures, we no longer need to provide a onerr argument.\n>\n> Yay!  More precisely, now that all callers use\n> UPDATE_REFS_QUIET_ON_ERR there's no need to support any other\n> behavior.\n>\n> Thanks for cleaning up the error handling here.\n"},{"id":"240006","messageId":"CAL=YDWkk9aqDmAA4OAYd-eFNkdKoEdp8gS=n4VyMqAFpknU3Bg@mail.gmail.com","threadId":"36495","inReplyTo":"535AEFAF.9000908@alum.mit.edu","subject":"Re: [PATCH v3 17/19] refs.c: change update_ref to use a transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T18:33:52Z","receivedAt":"2014-04-28T18:33:52Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Thanks.\n\nOn Fri, Apr 25, 2014 at 4:28 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:\n>> Change the update_ref helper function to use a ref transaction internally.\n>>\n>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n>> ---\n>>  refs.c | 23 +++++++++++++++++++----\n>>  1 file changed, 19 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/refs.c b/refs.c\n>> index 1557c3c..95df4a9 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -3397,11 +3397,26 @@ int update_ref(const char *action, const char *refname,\n>>              const unsigned char *sha1, const unsigned char *oldval,\n>>              int flags, enum action_on_err onerr)\n>>  {\n>> -     struct ref_lock *lock;\n>> -     lock = update_ref_lock(refname, oldval, flags, NULL, onerr);\n>> -     if (!lock)\n>> +     struct ref_transaction *t;\n>> +     char *err = NULL;\n>> +\n>> +     t = ref_transaction_begin();\n>> +     if ((!t ||\n>> +         ref_transaction_update(t, refname, sha1, oldval, flags,\n>> +                                !!oldval)) ||\n>> +         (ref_transaction_commit(t, action, &err) && !(t = NULL))) {\n\nThis is transient code to just show how ugly this pattern becomes when\nref_transaction_commit()\nimplicitly also frees the transaction.\n\nLater patches toward the end of the series will replace this with\nsomething nicer.\nCurrently in this patch series I pass a pointer to pointer for the\ntransaction to _commit()\nbut that is perhaps not optimal either.\nI will try and see how it would look if _commit would not free the\ntransaction at all\nand the caller would always have to call ref_transaction_free() explicitely.\n\n\nI am thinking of something like this :\n\n   t = ref_transaction_begin();\n   if ((!t ||\n       ref_transaction_update(t, ...)) ||\n       ref_transaction_commit(t, ...)) {\n          ...something...\n   }\n   ref_transaction_free(t);\n\n\n>\n> You seem to have extra parentheses around the first || subexpression.\n>\n> You also don't need the parentheses around the && expression because &&\n> binds more tightly than ||.\n>\n> But using \"!(t = NULL)\" in the middle of a conditional expression is\n> pretty obscure.  I think you will change this in a later patch, so I\n> will defer my comments.\n>\n>> +          const char *str = \"update_ref failed for ref '%s': %s\";\n>\n> Indentation error.\n\nFixed.\n\n>\n>> +\n>> +             ref_transaction_rollback(t);\n>> +             switch (onerr) {\n>> +             case UPDATE_REFS_MSG_ON_ERR: error(str, refname, err); break;\n>> +             case UPDATE_REFS_DIE_ON_ERR: die(str, refname, err); break;\n>> +             case UPDATE_REFS_QUIET_ON_ERR: break;\n>> +             free(err);\n>> +             }\n>>               return 1;\n>> -     return update_ref_write(action, refname, sha1, lock, NULL, onerr);\n>> +     }\n>> +     return 0;\n>>  }\n>>\n>>  static int ref_update_compare(const void *r1, const void *r2)\n>>\n>\n>\n> --\n> Michael Haggerty\n> mhagger@alum.mit.edu\n> http://softwareswirl.blogspot.com/\n"},{"id":"239992","messageId":"CAL=YDWkJKOM7eo7cknMH4MAAYJ=Ds9PVjUvufHzrBu=neucf4g@mail.gmail.com","threadId":"36495","inReplyTo":"535AECC5.3090100@alum.mit.edu","subject":"Re: [PATCH v3 16/19] branch.c: use ref transaction for all ref updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-28T19:16:24Z","receivedAt":"2014-04-28T19:16:24Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Fri, Apr 25, 2014 at 4:16 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:\n>> Change create_branch to use a ref transaction when creating the new branch.\n>> ref_transaction_create will check that the ref does not already exist and fail\n>> otherwise meaning that we no longer need to keep a lock on the ref during the\n>> setup_tracking. This simplifies the code since we can now do the transaction\n>> in one single step.\n>>\n>> If the forcing flag is false then use ref_transaction_create since this will\n>> fail if the ref already exist. Otherwise use ref_transaction_update.\n>>\n>> This also fixes a race condition in the old code where two concurrent\n>> create_branch could race since the lock_any_ref_for_update/write_ref_sha1\n>> did not protect against the ref already existsing. I.e. one thread could end up\n>> overwriting a branch even if the forcing flag is false.\n>>\n>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n>> ---\n>>  branch.c | 39 +++++++++++++++++++++++++--------------\n>>  1 file changed, 25 insertions(+), 14 deletions(-)\n>>\n>> diff --git a/branch.c b/branch.c\n>> index 660097b..23cde1e 100644\n>> --- a/branch.c\n>> +++ b/branch.c\n>> @@ -226,7 +226,6 @@ void create_branch(const char *head,\n>>                  int force, int reflog, int clobber_head,\n>>                  int quiet, enum branch_track track)\n>>  {\n>> -     struct ref_lock *lock = NULL;\n>>       struct commit *commit;\n>>       unsigned char sha1[20];\n>>       char *real_ref, msg[PATH_MAX + 20];\n>> @@ -285,15 +284,6 @@ void create_branch(const char *head,\n>>               die(_(\"Not a valid branch point: '%s'.\"), start_name);\n>>       hashcpy(sha1, commit->object.sha1);\n>>\n>> -     if (!dont_change_ref) {\n>> -             lock = lock_any_ref_for_update(ref.buf, NULL, 0, NULL);\n>> -             if (!lock)\n>> -                     die_errno(_(\"Failed to lock ref for update\"));\n>> -     }\n>> -\n>> -     if (reflog)\n>> -             log_all_ref_updates = 1;\n>> -\n>>       if (forcing)\n>>               snprintf(msg, sizeof msg, \"branch: Reset to %s\",\n>>                        start_name);\n>> @@ -301,13 +291,34 @@ void create_branch(const char *head,\n>>               snprintf(msg, sizeof msg, \"branch: Created from %s\",\n>>                        start_name);\n>>\n>> +     if (reflog)\n>> +             log_all_ref_updates = 1;\n>> +\n>> +     if (!dont_change_ref) {\n>> +             struct ref_transaction *transaction;\n>> +             char *err = NULL;\n>> +\n>> +             transaction = ref_transaction_begin();\n>> +             if (forcing) {\n>> +                     if (!transaction ||\n>> +                         ref_transaction_update(transaction, ref.buf, sha1,\n>> +                                                NULL, 0, 0) ||\n>> +                         ref_transaction_commit(transaction, msg, &err))\n>> +                       die_errno(_(\"%s: failed to write ref: %s\"),\n>> +                                 ref.buf, err);\n>> +             } else {\n>> +                     if (!transaction ||\n>> +                         ref_transaction_create(transaction, ref.buf, sha1,\n>> +                                                0) ||\n>> +                         ref_transaction_commit(transaction, msg, &err))\n>> +                       die_errno(_(\"%s: failed to write ref: %s\"),\n>> +                                 ref.buf, err);\n>> +             }\n>\n> You've got some indentation problems above.\n>\n> But actually, there seems like a lot of duplicated code here.  Couldn't\n> you instead do a single block with have_old set based on forcing:\n>\n>     ref_transaction_update(transaction, ref.buf, sha1,\n>                            null_sha1, 0, !forcing)\n>\n> ?\n\nDone, thanks.\n\n\nI am not sure how I feel about using _update to create new refs\nsince we already have ref_transaction_create for that purpose.\n\nref_transaction_update can either be used to update an existing ref\nor it can be used to create new refs, either by passing have_old==0\nor by passing old_sha1==null_sha1 and have_old==1\n\nMaybe the api would be cleaner if we would change it so that update\nand create does\nnot overlap and thus change _update so that it can only modify refs\nthat must already exist ?\n\n\n\n>\n>> +     }\n>> +\n>>       if (real_ref && track)\n>>               setup_tracking(ref.buf + 11, real_ref, track, quiet);\n>>\n>> -     if (!dont_change_ref)\n>> -             if (write_ref_sha1(lock, sha1, msg) < 0)\n>> -                     die_errno(_(\"Failed to write ref\"));\n>> -\n>>       strbuf_release(&ref);\n>>       free(real_ref);\n>>  }\n>>\n>\n>\n> --\n> Michael Haggerty\n> mhagger@alum.mit.edu\n> http://softwareswirl.blogspot.com/\n"},{"id":"240149","messageId":"535F6FFD.90004@alum.mit.edu","threadId":"36495","inReplyTo":"CAL=YDWkSdiUd-6A60ncGaDrFV2pc5WtRMv8iCSHHqFLkKH=pfw@mail.gmail.com","subject":"Re: [PATCH v3 19/19] refs.c: pass **transaction to commit and have it clear the pointer","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-29T09:25:17Z","receivedAt":"2014-04-29T09:25:17Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/28/2014 07:59 PM, Ronnie Sahlberg wrote:\n> On Fri, Apr 25, 2014 at 6:31 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>> On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:\n>>> Change ref_transaction_commit to take a pointer to a pointer for the\n>>> transaction. This allows us to clear the transaction pointer from within\n>>> ref_transaction_commit so that it becomes NULL in the caller.\n>>>\n>>> This makes transaction handling in the callers much nicer since instead of\n>>> having to write horrible code like :\n>>>       t = ref_transaction_begin();\n>>>       if ((!t ||\n>>>           ref_transaction_update(t, refname, sha1, oldval, flags,\n>>>                                  !!oldval)) ||\n>>>           (ref_transaction_commit(t, action, &err) && !(t = NULL))) {\n>>>               ref_transaction_rollback(t);\n>>>\n>>> we can now just do the much nicer\n>>>       t = ref_transaction_begin();\n>>>       if (!t ||\n>>>           ref_transaction_update(t, refname, sha1, oldval, flags,\n>>>                                  !!oldval) ||\n>>>           ref_transaction_commit(&t, action, &err)) {\n>>>               ref_transaction_rollback(t);\n>>\n>> I understand the motivation for this change, but passing\n>> pointer-to-pointer is unconventional in a case like this.  Unfortunately\n>> I ran out of steam for the night before I could think about alternatives.\n> \n> I see.\n> Yes passing a pointer to pointer is not ideal.\n> But I still want to be able to use the pattern\n>        t = ref_transaction_begin();\n>        if (!t ||\n>            ref_transaction_update(t, ...) ||\n>            ref_transaction_commit(t, ...)) {\n>                ref_transaction_rollback(t);\n> \n> Maybe the problem is that ref_transaction_commit() implicitely also\n> frees the transaction.\n> \n> \n> What about changing ref_transaction_commit() would NOT free the\n> transaction and thus a caller would\n> always have to explicitely free the transaction afterwards?\n> \n> Something like this :\n>        t = ref_transaction_begin();\n>        if (!t ||\n>            ref_transaction_update(t, ...) ||\n>            ref_transaction_commit(&t, ...)) {\n\nYou wouldn't need the \"&\" here then, right?\n\n>                ref_transaction_rollback(t);\n>        }\n>        ref_transaction_free(t);\n\nThat sounds like a better solution.  We would want to make sure that\nref_transaction_commit() / ref_transaction_rollback() leaves the\nref_transaction in a state that if it is accidentally passed to\nref_transaction_update() or its friends, the function calls die(\"BUG: ...\").\n\nUnless we want to make ref_transaction objects reusable.  But then we\nwould need an explicit \"allocation\" step in the boilerplate code:\n\n    t = ref_transaction_alloc();\n    while (something) {\n            if (ref_transaction_begin(t) ||\n                 ref_transaction_update(t, ...) ||\n                 ref_transaction_commit(t, ...)) {\n                    ref_transaction_rollback(t);\n            }\n    }\n    ref_transaction_free(t);\n\nNote that ref_transaction_begin() should in this case be converted to\nreturn 0-on-OK, negative-on-error for consistency.\n\nThis would bring us back to the familiar pattern alloc...use...free.\n\nI was going to say that the extra boilerplate is not worth it, and\nanyway reusing ref_transaction objects won't save any significant work.\n But then it occurred to me that ref_transaction_alloc() might be a\nplace to do more expensive work, like creating a connection to a\ndatabase, so reuse could potentially be a bigger win.\n\nAll in all, either way is OK with me.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"240150","messageId":"535F725B.5000102@alum.mit.edu","threadId":"36495","inReplyTo":"CAL=YDWkJKOM7eo7cknMH4MAAYJ=Ds9PVjUvufHzrBu=neucf4g@mail.gmail.com","subject":"Re: [PATCH v3 16/19] branch.c: use ref transaction for all ref updates","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-29T09:35:23Z","receivedAt":"2014-04-29T09:35:23Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/28/2014 09:16 PM, Ronnie Sahlberg wrote:\n> On Fri, Apr 25, 2014 at 4:16 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>> On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:\n>>> Change create_branch to use a ref transaction when creating the new branch.\n>>> ref_transaction_create will check that the ref does not already exist and fail\n>>> otherwise meaning that we no longer need to keep a lock on the ref during the\n>>> setup_tracking. This simplifies the code since we can now do the transaction\n>>> in one single step.\n>>>\n>>> If the forcing flag is false then use ref_transaction_create since this will\n>>> fail if the ref already exist. Otherwise use ref_transaction_update.\n>>>\n>>> This also fixes a race condition in the old code where two concurrent\n>>> create_branch could race since the lock_any_ref_for_update/write_ref_sha1\n>>> did not protect against the ref already existsing. I.e. one thread could end up\n>>> overwriting a branch even if the forcing flag is false.\n>>>\n>>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n>>> ---\n>>>  branch.c | 39 +++++++++++++++++++++++++--------------\n>>>  1 file changed, 25 insertions(+), 14 deletions(-)\n>>>\n>>> diff --git a/branch.c b/branch.c\n>>> index 660097b..23cde1e 100644\n>>> --- a/branch.c\n>>> +++ b/branch.c\n>>> @@ -226,7 +226,6 @@ void create_branch(const char *head,\n>>>                  int force, int reflog, int clobber_head,\n>>>                  int quiet, enum branch_track track)\n>>>  {\n>>> -     struct ref_lock *lock = NULL;\n>>>       struct commit *commit;\n>>>       unsigned char sha1[20];\n>>>       char *real_ref, msg[PATH_MAX + 20];\n>>> @@ -285,15 +284,6 @@ void create_branch(const char *head,\n>>>               die(_(\"Not a valid branch point: '%s'.\"), start_name);\n>>>       hashcpy(sha1, commit->object.sha1);\n>>>\n>>> -     if (!dont_change_ref) {\n>>> -             lock = lock_any_ref_for_update(ref.buf, NULL, 0, NULL);\n>>> -             if (!lock)\n>>> -                     die_errno(_(\"Failed to lock ref for update\"));\n>>> -     }\n>>> -\n>>> -     if (reflog)\n>>> -             log_all_ref_updates = 1;\n>>> -\n>>>       if (forcing)\n>>>               snprintf(msg, sizeof msg, \"branch: Reset to %s\",\n>>>                        start_name);\n>>> @@ -301,13 +291,34 @@ void create_branch(const char *head,\n>>>               snprintf(msg, sizeof msg, \"branch: Created from %s\",\n>>>                        start_name);\n>>>\n>>> +     if (reflog)\n>>> +             log_all_ref_updates = 1;\n>>> +\n>>> +     if (!dont_change_ref) {\n>>> +             struct ref_transaction *transaction;\n>>> +             char *err = NULL;\n>>> +\n>>> +             transaction = ref_transaction_begin();\n>>> +             if (forcing) {\n>>> +                     if (!transaction ||\n>>> +                         ref_transaction_update(transaction, ref.buf, sha1,\n>>> +                                                NULL, 0, 0) ||\n>>> +                         ref_transaction_commit(transaction, msg, &err))\n>>> +                       die_errno(_(\"%s: failed to write ref: %s\"),\n>>> +                                 ref.buf, err);\n>>> +             } else {\n>>> +                     if (!transaction ||\n>>> +                         ref_transaction_create(transaction, ref.buf, sha1,\n>>> +                                                0) ||\n>>> +                         ref_transaction_commit(transaction, msg, &err))\n>>> +                       die_errno(_(\"%s: failed to write ref: %s\"),\n>>> +                                 ref.buf, err);\n>>> +             }\n>>\n>> You've got some indentation problems above.\n>>\n>> But actually, there seems like a lot of duplicated code here.  Couldn't\n>> you instead do a single block with have_old set based on forcing:\n>>\n>>     ref_transaction_update(transaction, ref.buf, sha1,\n>>                            null_sha1, 0, !forcing)\n>>\n>> ?\n> \n> Done, thanks.\n> \n> \n> I am not sure how I feel about using _update to create new refs\n> since we already have ref_transaction_create for that purpose.\n> \n> ref_transaction_update can either be used to update an existing ref\n> or it can be used to create new refs, either by passing have_old==0\n> or by passing old_sha1==null_sha1 and have_old==1\n\nHold onto your socks then, because I think in the future update() should\nget a have_new parameter too.  That way it can also be used to verify\nthe current value of a reference by passing have_old=1, have_new=0\nwithout also re-setting the reference unnecessarily like now.  Though I\nadmit, have_old=have_new=0 might *not* be so useful :-)\n\n> Maybe the api would be cleaner if we would change it so that update\n> and create does\n> not overlap and thus change _update so that it can only modify refs\n> that must already exist ?\n\nI have no compunctions about using update() to create or delete a\nreference.  My point of view is that update() is the general case, and\ncreate() and delete() are special-cases that exist only for the\nconvenience of callers.  For example, our future pluggable backends\nmight only have to implement update(), and the other two functions could\ndelegate to it at the abstract layer.\n\nPlus, making this stricter would make it impossible to eliminate\nduplicate code like in the example above, which is itself evidence that\nupdate() is a useful abstraction.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"240179","messageId":"CAL=YDWnav-26kBmmJ+XJq-hQEkdv3UUdz6wQ5Jn-cq+H8Ozb+g@mail.gmail.com","threadId":"36495","inReplyTo":"535F725B.5000102@alum.mit.edu","subject":"Re: [PATCH v3 16/19] branch.c: use ref transaction for all ref updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-29T15:11:02Z","receivedAt":"2014-04-29T15:11:02Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Tue, Apr 29, 2014 at 2:35 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> On 04/28/2014 09:16 PM, Ronnie Sahlberg wrote:\n>> On Fri, Apr 25, 2014 at 4:16 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>>> On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:\n>>>> Change create_branch to use a ref transaction when creating the new branch.\n>>>> ref_transaction_create will check that the ref does not already exist and fail\n>>>> otherwise meaning that we no longer need to keep a lock on the ref during the\n>>>> setup_tracking. This simplifies the code since we can now do the transaction\n>>>> in one single step.\n>>>>\n>>>> If the forcing flag is false then use ref_transaction_create since this will\n>>>> fail if the ref already exist. Otherwise use ref_transaction_update.\n>>>>\n>>>> This also fixes a race condition in the old code where two concurrent\n>>>> create_branch could race since the lock_any_ref_for_update/write_ref_sha1\n>>>> did not protect against the ref already existsing. I.e. one thread could end up\n>>>> overwriting a branch even if the forcing flag is false.\n>>>>\n>>>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n>>>> ---\n>>>>  branch.c | 39 +++++++++++++++++++++++++--------------\n>>>>  1 file changed, 25 insertions(+), 14 deletions(-)\n>>>>\n>>>> diff --git a/branch.c b/branch.c\n>>>> index 660097b..23cde1e 100644\n>>>> --- a/branch.c\n>>>> +++ b/branch.c\n>>>> @@ -226,7 +226,6 @@ void create_branch(const char *head,\n>>>>                  int force, int reflog, int clobber_head,\n>>>>                  int quiet, enum branch_track track)\n>>>>  {\n>>>> -     struct ref_lock *lock = NULL;\n>>>>       struct commit *commit;\n>>>>       unsigned char sha1[20];\n>>>>       char *real_ref, msg[PATH_MAX + 20];\n>>>> @@ -285,15 +284,6 @@ void create_branch(const char *head,\n>>>>               die(_(\"Not a valid branch point: '%s'.\"), start_name);\n>>>>       hashcpy(sha1, commit->object.sha1);\n>>>>\n>>>> -     if (!dont_change_ref) {\n>>>> -             lock = lock_any_ref_for_update(ref.buf, NULL, 0, NULL);\n>>>> -             if (!lock)\n>>>> -                     die_errno(_(\"Failed to lock ref for update\"));\n>>>> -     }\n>>>> -\n>>>> -     if (reflog)\n>>>> -             log_all_ref_updates = 1;\n>>>> -\n>>>>       if (forcing)\n>>>>               snprintf(msg, sizeof msg, \"branch: Reset to %s\",\n>>>>                        start_name);\n>>>> @@ -301,13 +291,34 @@ void create_branch(const char *head,\n>>>>               snprintf(msg, sizeof msg, \"branch: Created from %s\",\n>>>>                        start_name);\n>>>>\n>>>> +     if (reflog)\n>>>> +             log_all_ref_updates = 1;\n>>>> +\n>>>> +     if (!dont_change_ref) {\n>>>> +             struct ref_transaction *transaction;\n>>>> +             char *err = NULL;\n>>>> +\n>>>> +             transaction = ref_transaction_begin();\n>>>> +             if (forcing) {\n>>>> +                     if (!transaction ||\n>>>> +                         ref_transaction_update(transaction, ref.buf, sha1,\n>>>> +                                                NULL, 0, 0) ||\n>>>> +                         ref_transaction_commit(transaction, msg, &err))\n>>>> +                       die_errno(_(\"%s: failed to write ref: %s\"),\n>>>> +                                 ref.buf, err);\n>>>> +             } else {\n>>>> +                     if (!transaction ||\n>>>> +                         ref_transaction_create(transaction, ref.buf, sha1,\n>>>> +                                                0) ||\n>>>> +                         ref_transaction_commit(transaction, msg, &err))\n>>>> +                       die_errno(_(\"%s: failed to write ref: %s\"),\n>>>> +                                 ref.buf, err);\n>>>> +             }\n>>>\n>>> You've got some indentation problems above.\n>>>\n>>> But actually, there seems like a lot of duplicated code here.  Couldn't\n>>> you instead do a single block with have_old set based on forcing:\n>>>\n>>>     ref_transaction_update(transaction, ref.buf, sha1,\n>>>                            null_sha1, 0, !forcing)\n>>>\n>>> ?\n>>\n>> Done, thanks.\n>>\n>>\n>> I am not sure how I feel about using _update to create new refs\n>> since we already have ref_transaction_create for that purpose.\n>>\n>> ref_transaction_update can either be used to update an existing ref\n>> or it can be used to create new refs, either by passing have_old==0\n>> or by passing old_sha1==null_sha1 and have_old==1\n>\n> Hold onto your socks then, because I think in the future update() should\n> get a have_new parameter too.  That way it can also be used to verify\n> the current value of a reference by passing have_old=1, have_new=0\n> without also re-setting the reference unnecessarily like now.  Though I\n> admit, have_old=have_new=0 might *not* be so useful :-)\n>\n>> Maybe the api would be cleaner if we would change it so that update\n>> and create does\n>> not overlap and thus change _update so that it can only modify refs\n>> that must already exist ?\n>\n> I have no compunctions about using update() to create or delete a\n> reference.  My point of view is that update() is the general case, and\n> create() and delete() are special-cases that exist only for the\n> convenience of callers.  For example, our future pluggable backends\n> might only have to implement update(), and the other two functions could\n> delegate to it at the abstract layer.\n>\n> Plus, making this stricter would make it impossible to eliminate\n> duplicate code like in the example above, which is itself evidence that\n> update() is a useful abstraction.\n>\n\nOk, Fair enough.\nIn that case, in the future we should change\nref_transaction_create/ref_transaction_delete to just\nbecome wrappers that invoke ref_transaction_update.\n\n\n> Michael\n>\n> --\n> Michael Haggerty\n> mhagger@alum.mit.edu\n> http://softwareswirl.blogspot.com/\n"},{"id":"240186","messageId":"xmqqr44gkqfp.fsf@gitster.dls.corp.google.com","threadId":"36495","inReplyTo":"535F725B.5000102@alum.mit.edu","subject":"Re: [PATCH v3 16/19] branch.c: use ref transaction for all ref updates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-29T17:15:22Z","receivedAt":"2014-04-29T17:15:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> I have no compunctions about using update() to create or delete a\n> reference.  My point of view is that update() is the general case, and\n> create() and delete() are special-cases that exist only for the\n> convenience of callers.  For example, our future pluggable backends\n> might only have to implement update(), and the other two functions could\n> delegate to it at the abstract layer.\n\nSounds like a sensible position.\n\nThanks for commenting, Michael, and thanks for working on this,\nRonnie.\n"},{"id":"240195","messageId":"CAL=YDWmrWT_N8k19t86tfHoTNSbpKh6qRSh-XMUvO9u4fdGk=Q@mail.gmail.com","threadId":"36495","inReplyTo":"535F6FFD.90004@alum.mit.edu","subject":"Re: [PATCH v3 19/19] refs.c: pass **transaction to commit and have it clear the pointer","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-29T18:58:41Z","receivedAt":"2014-04-29T18:58:41Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Tue, Apr 29, 2014 at 2:25 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> On 04/28/2014 07:59 PM, Ronnie Sahlberg wrote:\n>> On Fri, Apr 25, 2014 at 6:31 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>>> On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:\n>>>> Change ref_transaction_commit to take a pointer to a pointer for the\n>>>> transaction. This allows us to clear the transaction pointer from within\n>>>> ref_transaction_commit so that it becomes NULL in the caller.\n>>>>\n>>>> This makes transaction handling in the callers much nicer since instead of\n>>>> having to write horrible code like :\n>>>>       t = ref_transaction_begin();\n>>>>       if ((!t ||\n>>>>           ref_transaction_update(t, refname, sha1, oldval, flags,\n>>>>                                  !!oldval)) ||\n>>>>           (ref_transaction_commit(t, action, &err) && !(t = NULL))) {\n>>>>               ref_transaction_rollback(t);\n>>>>\n>>>> we can now just do the much nicer\n>>>>       t = ref_transaction_begin();\n>>>>       if (!t ||\n>>>>           ref_transaction_update(t, refname, sha1, oldval, flags,\n>>>>                                  !!oldval) ||\n>>>>           ref_transaction_commit(&t, action, &err)) {\n>>>>               ref_transaction_rollback(t);\n>>>\n>>> I understand the motivation for this change, but passing\n>>> pointer-to-pointer is unconventional in a case like this.  Unfortunately\n>>> I ran out of steam for the night before I could think about alternatives.\n>>\n>> I see.\n>> Yes passing a pointer to pointer is not ideal.\n>> But I still want to be able to use the pattern\n>>        t = ref_transaction_begin();\n>>        if (!t ||\n>>            ref_transaction_update(t, ...) ||\n>>            ref_transaction_commit(t, ...)) {\n>>                ref_transaction_rollback(t);\n>>\n>> Maybe the problem is that ref_transaction_commit() implicitely also\n>> frees the transaction.\n>>\n>>\n>> What about changing ref_transaction_commit() would NOT free the\n>> transaction and thus a caller would\n>> always have to explicitely free the transaction afterwards?\n>>\n>> Something like this :\n>>        t = ref_transaction_begin();\n>>        if (!t ||\n>>            ref_transaction_update(t, ...) ||\n>>            ref_transaction_commit(&t, ...)) {\n>\n> You wouldn't need the \"&\" here then, right?\n>\n>>                ref_transaction_rollback(t);\n>>        }\n>>        ref_transaction_free(t);\n>\n> That sounds like a better solution.  We would want to make sure that\n> ref_transaction_commit() / ref_transaction_rollback() leaves the\n> ref_transaction in a state that if it is accidentally passed to\n> ref_transaction_update() or its friends, the function calls die(\"BUG: ...\").\n\nThanks!\n\nGood idea.\nI will add a transaction->status field that can track OPEN/CLOSED/ERROR\nand die(BUG:...) appropriately in _commit/_create/_delete/_update if\nit has the wrong value.\n\n\n>\n> Unless we want to make ref_transaction objects reusable.  But then we\n> would need an explicit \"allocation\" step in the boilerplate code:\n>\n>     t = ref_transaction_alloc();\n>     while (something) {\n>             if (ref_transaction_begin(t) ||\n>                  ref_transaction_update(t, ...) ||\n>                  ref_transaction_commit(t, ...)) {\n>                     ref_transaction_rollback(t);\n>             }\n>     }\n>     ref_transaction_free(t);\n>\n> Note that ref_transaction_begin() should in this case be converted to\n> return 0-on-OK, negative-on-error for consistency.\n>\n> This would bring us back to the familiar pattern alloc...use...free.\n>\n> I was going to say that the extra boilerplate is not worth it, and\n> anyway reusing ref_transaction objects won't save any significant work.\n>  But then it occurred to me that ref_transaction_alloc() might be a\n> place to do more expensive work, like creating a connection to a\n> database, so reuse could potentially be a bigger win.\n\nACK, but I don't think we need reusable transaction yet. Once the API\nis cleaned up\nit should be reasonably easy to add in the future if we see a need for it.\nSounds reasonable to you ?\n\n\n>\n> All in all, either way is OK with me.\n>\n> Michael\n>\n> --\n> Michael Haggerty\n> mhagger@alum.mit.edu\n> http://softwareswirl.blogspot.com/\n"},{"id":"240306","messageId":"5361069C.1020108@alum.mit.edu","threadId":"36495","inReplyTo":"CAL=YDWmrWT_N8k19t86tfHoTNSbpKh6qRSh-XMUvO9u4fdGk=Q@mail.gmail.com","subject":"Re: [PATCH v3 19/19] refs.c: pass **transaction to commit and have it clear the pointer","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-30T14:20:12Z","receivedAt":"2014-04-30T14:20:12Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/29/2014 08:58 PM, Ronnie Sahlberg wrote:\n> On Tue, Apr 29, 2014 at 2:25 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>> [...]\n>> Unless we want to make ref_transaction objects reusable.  [...]\n> ACK, but I don't think we need reusable transaction yet. Once the API\n> is cleaned up\n> it should be reasonably easy to add in the future if we see a need for it.\n> Sounds reasonable to you ?\n\nYes, reusable transaction objects would be pretty straightforward to\nretrofit, so I agree that there is no need to implement it now.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"}]}