{"thread":{"id":"36862","subject":"[PATCH v14 02/40] refs.c: ref_transaction_commit should not free the transaction","startedAt":"2014-06-06T22:28:38Z","lastAt":"2014-06-11T18:44:41Z","messageCount":51,"participants":["Ronnie Sahlberg","Jonathan Nieder"],"isPatch":true,"patchVersion":14,"patchTotal":40},"messages":[{"id":"243560","messageId":"1402093758-3162-1-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":null,"subject":"[PATCH v14 00/40] Use ref transactions","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:28:38Z","receivedAt":"2014-06-06T22:28:38Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"This patch series can also be found at\nhttps://github.com/rsahlberg/git/tree/ref-transactions\n\n\nThis patch series is based on next and expands on the transaction API. It\nconverts all ref updates, inside refs.c as well as external, to use the\ntransaction API for updates. This makes most of the ref updates to become\natomic when there are failures locking or writing to a ref.\n\nThis version completes the work to convert all ref updates to use transactions.\nNow that all updates are through transactions I will start working on\ncleaning up the reading of refs and to create an api for managing reflogs but\nall that will go in a different patch series.\n\nVersion 14:\n - Remove the patch to pack the refs before rename. We do not need this\n with the reworked renames that will come 2 series later.\n The rename_ref changes are thus no longer part of this series.\nVersion 13:\n - This version should cover all of JNs suggestions on the previous series.\n If I missed anything I appologize.\n...\n\nRonnie Sahlberg (40):\n refs.c: remove ref_transaction_rollback\n refs.c: ref_transaction_commit should not free the transaction\n refs.c: constify the sha arguments for\n ref_transaction_create|delete|update\n refs.c: allow passing NULL to ref_transaction_free\n refs.c: add a strbuf argument to ref_transaction_commit for error\n logging\n refs.c: add an err argument to repack_without_refs\n refs.c: make ref_update_reject_duplicates take a strbuf argument for\n errors\n refs.c: add an err argument to delete_ref_loose\n refs.c: make update_ref_write update a strbuf on failure\n update-ref: use err argument to get error from ref_transaction_commit\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: update ref_transaction_delete to check for error and return\n status\n refs.c: make ref_transaction_begin take an err argument\n refs.c: add transaction.status and track OPEN/CLOSED/ERROR\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 receive-pack.c: use a reference transaction for updating the refs\n fast-import.c: use a ref transaction when dumping tags\n walker.c: use ref transaction for ref updates\n refs.c: make lock_ref_sha1 static\n refs.c: remove the update_ref_lock function\n refs.c: remove the update_ref_write function\n refs.c: remove lock_ref_sha1\n refs.c: make prune_ref use a transaction to delete the ref\n refs.c: make delete_ref use a transaction\n refs.c: pass the ref log message to _create/delete/update instead of\n _commit\n refs.c: pass NULL as *flags to read_ref_full\n refs.c: move the check for valid refname to lock_ref_sha1_basic\n refs.c: call lock_ref_sha1_basic directly from commit\n refs.c: pass a skip list to name_conflict_fn\n refs.c: propagate any errno==ENOTDIR from _commit back to the callers\n fetch.c: change s_update_ref to use a ref transaction\n refs.c: make write_ref_sha1 static\n\n branch.c | 30 +--\n builtin/commit.c | 24 ++-\n builtin/fetch.c | 36 ++--\n builtin/receive-pack.c | 32 +++-\n builtin/replace.c | 15 +-\n builtin/tag.c | 15 +-\n builtin/update-ref.c | 34 ++--\n cache.h | 2 +\n fast-import.c | 54 ++++--\n lockfile.c | 37 ++--\n refs.c | 500 ++++++++++++++++++++++++++++++++-----------------\n refs.h | 118 +++++++++---\n sequencer.c | 24 ++-\n walker.c | 58 +++---\n 14 files changed, 641 insertions(+), 338 deletions(-)\n\n-- \n2.0.0.582.ge25c160\n"},{"id":"243565","messageId":"1402093758-3162-2-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 01/40] refs.c: remove ref_transaction_rollback","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:28:39Z","receivedAt":"2014-06-06T22:28:39Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"We do not yet need both a rollback and a free function for transactions.\nRemove ref_transaction_rollback and use ref_transaction_free instead.\n\nAt a later stage we may reintroduce a rollback function if we want to start\nadding reusable transactions and similar.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 7 +------\n refs.h | 16 +++++++---------\n 2 files changed, 8 insertions(+), 15 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 6898263..48573e3 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3308,7 +3308,7 @@ struct ref_transaction *ref_transaction_begin(void)\n \treturn xcalloc(1, sizeof(struct ref_transaction));\n }\n \n-static void ref_transaction_free(struct ref_transaction *transaction)\n+void ref_transaction_free(struct ref_transaction *transaction)\n {\n \tint i;\n \n@@ -3319,11 +3319,6 @@ static void ref_transaction_free(struct ref_transaction *transaction)\n \tfree(transaction);\n }\n \n-void ref_transaction_rollback(struct ref_transaction *transaction)\n-{\n-\tref_transaction_free(transaction);\n-}\n-\n static struct ref_update *add_update(struct ref_transaction *transaction,\n \t\t\t\t const char *refname)\n {\ndiff --git a/refs.h b/refs.h\nindex 09ff483..a07a5d0 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -216,18 +216,12 @@ enum action_on_err {\n \n /*\n * Begin a reference transaction. The reference transaction must\n- * eventually be commited using ref_transaction_commit() or rolled\n- * back using ref_transaction_rollback().\n+ * eventually be commited using ref_transaction_commit() or freed by\n+ * calling ref_transaction_free().\n */\n struct ref_transaction *ref_transaction_begin(void);\n \n /*\n- * Roll back a ref_transaction and free all associated data.\n- */\n-void ref_transaction_rollback(struct ref_transaction *transaction);\n-\n-\n-/*\n * The following functions add a reference check or update to a\n * ref_transaction. In all of them, refname is the name of the\n * reference to be affected. The functions make internal copies of\n@@ -235,7 +229,6 @@ void ref_transaction_rollback(struct ref_transaction *transaction);\n * can be REF_NODEREF; it is passed to update_ref_lock().\n */\n \n-\n /*\n * Add a reference update to transaction. new_sha1 is the value that\n * the reference should have after the update, or zeros if it should\n@@ -277,6 +270,11 @@ void ref_transaction_delete(struct ref_transaction *transaction,\n int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t const char *msg, enum action_on_err onerr);\n \n+/*\n+ * Free an existing transaction and all associated data.\n+ */\n+void ref_transaction_free(struct ref_transaction *transaction);\n+\n /** Lock a ref and then write its file */\n int update_ref(const char *action, const char *refname,\n \t\tconst unsigned char *sha1, const unsigned char *oldval,\n-- \n2.0.0.582.ge25c160\n"},{"id":"243527","messageId":"1402093758-3162-3-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 02/40] refs.c: ref_transaction_commit should not free the transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:28:40Z","receivedAt":"2014-06-06T22:28:40Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Reviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n builtin/update-ref.c | 1 +\n refs.c | 1 -\n refs.h | 5 ++---\n 3 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 405267f..1fd7a89 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -369,6 +369,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \t\tupdate_refs_stdin();\n \t\tret = ref_transaction_commit(transaction, msg,\n \t\t\t\t\t UPDATE_REFS_DIE_ON_ERR);\n+\t\tref_transaction_free(transaction);\n \t\treturn ret;\n \t}\n \ndiff --git a/refs.c b/refs.c\nindex 48573e3..33541f4 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3483,7 +3483,6 @@ cleanup:\n \t\tif (updates[i]->lock)\n \t\t\tunlock_ref(updates[i]->lock);\n \tfree(delnames);\n-\tref_transaction_free(transaction);\n \treturn ret;\n }\n \ndiff --git a/refs.h b/refs.h\nindex a07a5d0..306d833 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -216,8 +216,7 @@ enum action_on_err {\n \n /*\n * Begin a reference transaction. The reference transaction must\n- * eventually be commited using ref_transaction_commit() or freed by\n- * calling ref_transaction_free().\n+ * be freed by calling ref_transaction_free().\n */\n struct ref_transaction *ref_transaction_begin(void);\n \n@@ -265,7 +264,7 @@ void ref_transaction_delete(struct ref_transaction *transaction,\n /*\n * Commit all of the changes that have been queued in transaction, as\n * atomically as possible. Return a nonzero value if there is a\n- * problem. The ref_transaction is freed by this function.\n+ * problem.\n */\n int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t const char *msg, enum action_on_err onerr);\n-- \n2.0.0.582.ge25c160\n"},{"id":"243528","messageId":"1402093758-3162-4-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 03/40] refs.c: constify the sha arguments for ref_transaction_create|delete|update","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:28:41Z","receivedAt":"2014-06-06T22:28:41Z","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\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 7 ++++---\n refs.h | 7 ++++---\n 2 files changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 33541f4..a767ef6 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3333,7 +3333,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@@ -3347,7 +3348,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@@ -3361,7 +3362,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 306d833..b893838 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -237,7 +237,8 @@ struct ref_transaction *ref_transaction_begin(void);\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@@ -248,7 +249,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@@ -258,7 +259,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-- \n2.0.0.582.ge25c160\n"},{"id":"243568","messageId":"1402093758-3162-5-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 04/40] refs.c: allow passing NULL to ref_transaction_free","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:28:42Z","receivedAt":"2014-06-06T22:28:42Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Allow ref_transaction_free(NULL) as a no-op. This makes ref_transaction_free\neasier to use and more similar to plain 'free'.\n\nIn particular, it lets us rollback unconditionally as part of cleanup code\nafter setting 'transaction = NULL' if a transaction has been committed or\nrolled back already.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/refs.c b/refs.c\nindex a767ef6..0faed29 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3312,6 +3312,9 @@ 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-- \n2.0.0.582.ge25c160\n"},{"id":"243566","messageId":"1402093758-3162-6-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 05/40] refs.c: add a strbuf argument to ref_transaction_commit for error logging","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:28:43Z","receivedAt":"2014-06-06T22:28:43Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Add a strbuf argument to _commit so that we can pass an error string back to\nthe caller. So that we can do error logging from the caller instead of from\n_commit.\n\nLonger term plan is to first convert all callers to use onerr==QUIET_ON_ERR\nand craft any log messages from the callers themselves and finally remove the\nonerr argument completely.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n builtin/update-ref.c | 2 +-\n refs.c | 6 +++++-\n refs.h | 5 ++++-\n 3 files changed, 10 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 1fd7a89..22617af 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\tref_transaction_free(transaction);\n \t\treturn ret;\ndiff --git a/refs.c b/refs.c\nindex 0faed29..25c3a93 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3418,7 +3418,8 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n }\n \n int ref_transaction_commit(struct ref_transaction *transaction,\n-\t\t\t const char *msg, enum action_on_err onerr)\n+\t\t\t const char *msg, struct strbuf *err,\n+\t\t\t enum action_on_err onerr)\n {\n \tint ret = 0, delnum = 0, i;\n \tconst char **delnames;\n@@ -3447,6 +3448,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t\t update->flags,\n \t\t\t\t\t &update->type, onerr);\n \t\tif (!update->lock) {\n+\t\t\tif (err)\n+\t\t\t\tstrbuf_addf(err, \"Cannot lock the ref '%s'.\",\n+\t\t\t\t\t update->refname);\n \t\t\tret = 1;\n \t\t\tgoto cleanup;\n \t\t}\ndiff --git a/refs.h b/refs.h\nindex b893838..94d4cd4 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -266,9 +266,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.\n+ * If err is non-NULL we will add an error string to it to explain why\n+ * the transaction failed. The string does not end in newline.\n */\n int ref_transaction_commit(struct ref_transaction *transaction,\n-\t\t\t const char *msg, enum action_on_err onerr);\n+\t\t\t const char *msg, struct strbuf *err,\n+\t\t\t enum action_on_err onerr);\n \n /*\n * Free an existing transaction and all associated data.\n-- \n2.0.0.582.ge25c160\n"},{"id":"243558","messageId":"1402093758-3162-7-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 06/40] refs.c: add an err argument to repack_without_refs","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:28:44Z","receivedAt":"2014-06-06T22:28:44Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Update repack_without_refs to take an err argument and update it if there\nis a failure. Pass the err variable from ref_transaction_commit to this\nfunction so that callers can print a meaningful error message if _commit\nfails due to a problem in repack_without_refs.\n\nAdd a new function unable_to_lock_message that takes a strbuf argument and\nfills in the reason for the failure.\n\nIn commit_packed_refs, make sure that we propagate any errno that\ncommit_lock_file might have set back to our caller.\n\nMake sure both commit_packed_refs and lock_file has errno set to a meaningful\nvalue on error. Update a whole bunch of other places to keep errno from\nbeeing clobbered.\n\nSigned-off-by: Ronnie Sahlberg \n---\n cache.h | 2 ++\n lockfile.c | 37 +++++++++++++--------\n refs.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++---------------\n 3 files changed, 110 insertions(+), 40 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex cbe1935..8b12aa8 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -559,6 +559,8 @@ struct lock_file {\n #define LOCK_DIE_ON_ERROR 1\n #define LOCK_NODEREF 2\n extern int unable_to_lock_error(const char *path, int err);\n+extern void unable_to_lock_message(const char *path, int err,\n+\t\t\t\t struct strbuf *buf);\n extern NORETURN void unable_to_lock_index_die(const char *path, int err);\n extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);\n extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);\ndiff --git a/lockfile.c b/lockfile.c\nindex 8fbcb6a..f5da18c 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -121,7 +121,7 @@ static char *resolve_symlink(char *p, size_t s)\n \treturn p;\n }\n \n-\n+/* Make sure errno contains a meaningful value on error */\n static int lock_file(struct lock_file *lk, const char *path, int flags)\n {\n \t/*\n@@ -130,8 +130,10 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t */\n \tstatic const size_t max_path_len = sizeof(lk->filename) - 5;\n \n-\tif (strlen(path) >= max_path_len)\n+\tif (strlen(path) >= max_path_len) {\n+\t\terrno = ENAMETOOLONG;\n \t\treturn -1;\n+\t}\n \tstrcpy(lk->filename, path);\n \tif (!(flags & LOCK_NODEREF))\n \t\tresolve_symlink(lk->filename, max_path_len);\n@@ -148,42 +150,49 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t\t\tlock_file_list = lk;\n \t\t\tlk->on_list = 1;\n \t\t}\n-\t\tif (adjust_shared_perm(lk->filename))\n-\t\t\treturn error(\"cannot fix permission bits on %s\",\n-\t\t\t\t lk->filename);\n+\t\tif (adjust_shared_perm(lk->filename)) {\n+\t\t\tint save_errno = errno;\n+\t\t\terror(\"cannot fix permission bits on %s\",\n+\t\t\t lk->filename);\n+\t\t\terrno = save_errno;\n+\t\t\treturn -1;\n+\t\t}\n \t}\n \telse\n \t\tlk->filename[0] = 0;\n \treturn lk->fd;\n }\n \n-static char *unable_to_lock_message(const char *path, int err)\n+void unable_to_lock_message(const char *path, int err, struct strbuf *buf)\n {\n-\tstruct strbuf buf = STRBUF_INIT;\n \n \tif (err == EEXIST) {\n-\t\tstrbuf_addf(&buf, \"Unable to create '%s.lock': %s.\\n\\n\"\n+\t\tstrbuf_addf(buf, \"Unable to create '%s.lock': %s.\\n\\n\"\n \t\t \"If no other git process is currently running, this probably means a\\n\"\n \t\t \"git process crashed in this repository earlier. Make sure no other git\\n\"\n \t\t \"process is running and remove the file manually to continue.\",\n \t\t\t absolute_path(path), strerror(err));\n \t} else\n-\t\tstrbuf_addf(&buf, \"Unable to create '%s.lock': %s\",\n+\t\tstrbuf_addf(buf, \"Unable to create '%s.lock': %s\",\n \t\t\t absolute_path(path), strerror(err));\n-\treturn strbuf_detach(&buf, NULL);\n }\n \n int unable_to_lock_error(const char *path, int err)\n {\n-\tchar *msg = unable_to_lock_message(path, err);\n-\terror(\"%s\", msg);\n-\tfree(msg);\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tunable_to_lock_message(path, err, &buf);\n+\terror(\"%s\", buf.buf);\n+\tstrbuf_release(&buf);\n \treturn -1;\n }\n \n NORETURN void unable_to_lock_index_die(const char *path, int err)\n {\n-\tdie(\"%s\", unable_to_lock_message(path, err));\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tunable_to_lock_message(path, err, &buf);\n+\tdie(\"%s\", buf.buf);\n }\n \n int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)\ndiff --git a/refs.c b/refs.c\nindex 25c3a93..7ec1f32 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1333,8 +1333,10 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \tif (flag)\n \t\t*flag = 0;\n \n-\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))\n+\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n+\t\terrno = EINVAL;\n \t\treturn NULL;\n+\t}\n \n \tfor (;;) {\n \t\tchar path[PATH_MAX];\n@@ -1342,8 +1344,10 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\tchar *buf;\n \t\tint fd;\n \n-\t\tif (--depth < 0)\n+\t\tif (--depth < 0) {\n+\t\t\terrno = ELOOP;\n \t\t\treturn NULL;\n+\t\t}\n \n \t\tgit_snpath(path, sizeof(path), \"%s\", refname);\n \n@@ -1405,9 +1409,13 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\t\t\treturn NULL;\n \t\t}\n \t\tlen = read_in_full(fd, buffer, sizeof(buffer)-1);\n+\t\tif (len < 0) {\n+\t\t\tint save_errno = errno;\n+\t\t\tclose(fd);\n+\t\t\terrno = save_errno;\n+ \t\t\treturn NULL;\n+\t\t}\n \t\tclose(fd);\n-\t\tif (len < 0)\n-\t\t\treturn NULL;\n \t\twhile (len && isspace(buffer[len-1]))\n \t\t\tlen--;\n \t\tbuffer[len] = '\\0';\n@@ -1424,6 +1432,7 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\t\t (buffer[40] != '\\0' && !isspace(buffer[40]))) {\n \t\t\t\tif (flag)\n \t\t\t\t\t*flag |= REF_ISBROKEN;\n+\t\t\t\terrno = EINVAL;\n \t\t\t\treturn NULL;\n \t\t\t}\n \t\t\treturn refname;\n@@ -1436,6 +1445,7 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\tif (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {\n \t\t\tif (flag)\n \t\t\t\t*flag |= REF_ISBROKEN;\n+\t\t\terrno = EINVAL;\n \t\t\treturn NULL;\n \t\t}\n \t\trefname = strcpy(refname_buffer, buf);\n@@ -1908,14 +1918,17 @@ static struct ref_lock *verify_lock(struct ref_lock *lock,\n \tconst unsigned char *old_sha1, int mustexist)\n {\n \tif (read_ref_full(lock->ref_name, lock->old_sha1, mustexist, NULL)) {\n+\t\tint save_errno = errno;\n \t\terror(\"Can't verify ref %s\", lock->ref_name);\n \t\tunlock_ref(lock);\n+\t\terrno = save_errno;\n \t\treturn NULL;\n \t}\n \tif (hashcmp(lock->old_sha1, old_sha1)) {\n \t\terror(\"Ref %s is at %s but expected %s\", lock->ref_name,\n \t\t\tsha1_to_hex(lock->old_sha1), sha1_to_hex(old_sha1));\n \t\tunlock_ref(lock);\n+\t\terrno = EBUSY;\n \t\treturn NULL;\n \t}\n \treturn lock;\n@@ -1928,13 +1941,15 @@ static int remove_empty_directories(const char *file)\n \t * only empty directories), remove them.\n \t */\n \tstruct strbuf path;\n-\tint result;\n+\tint result, save_errno;\n \n \tstrbuf_init(&path, 20);\n \tstrbuf_addstr(&path, file);\n \n \tresult = remove_dir_recursively(&path, REMOVE_DIR_EMPTY_ONLY);\n+\tsave_errno = errno;\n \n+\terrno = save_errno;\n \tstrbuf_release(&path);\n \n \treturn result;\n@@ -2203,11 +2218,16 @@ int lock_packed_refs(int flags)\n \treturn 0;\n }\n \n+/*\n+ * Commit the packed refs changes.\n+ * On error we must make sure that errno contains a meaningful value.\n+ */\n int commit_packed_refs(void)\n {\n \tstruct packed_ref_cache *packed_ref_cache =\n \t\tget_packed_ref_cache(&ref_cache);\n \tint error = 0;\n+\tint save_errno = 0;\n \n \tif (!packed_ref_cache->lock)\n \t\tdie(\"internal error: packed-refs not locked\");\n@@ -2217,10 +2237,13 @@ int commit_packed_refs(void)\n \tdo_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),\n \t\t\t\t 0, write_packed_entry_fn,\n \t\t\t\t &packed_ref_cache->lock->fd);\n-\tif (commit_lock_file(packed_ref_cache->lock))\n+\tif (commit_lock_file(packed_ref_cache->lock)) {\n+\t\tsave_errno = errno;\n \t\terror = -1;\n+\t}\n \tpacked_ref_cache->lock = NULL;\n \trelease_packed_ref_cache(packed_ref_cache);\n+\terrno = save_errno;\n \treturn error;\n }\n \n@@ -2427,12 +2450,12 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)\n \treturn 0;\n }\n \n-static int repack_without_refs(const char **refnames, int n)\n+static int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n {\n \tstruct ref_dir *packed;\n \tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\n \tstruct string_list_item *ref_to_delete;\n-\tint i, removed = 0;\n+\tint i, ret, removed = 0;\n \n \t/* Look for a packed ref */\n \tfor (i = 0; i < n; i++)\n@@ -2444,6 +2467,11 @@ static int repack_without_refs(const char **refnames, int n)\n \t\treturn 0; /* no refname exists in packed refs */\n \n \tif (lock_packed_refs(0)) {\n+\t\tif (err) {\n+\t\t\tunable_to_lock_message(git_path(\"packed-refs\"), errno,\n+\t\t\t\t\t err);\n+\t\t\treturn -1;\n+\t\t}\n \t\tunable_to_lock_error(git_path(\"packed-refs\"), errno);\n \t\treturn error(\"cannot delete '%s' from packed refs\", refnames[i]);\n \t}\n@@ -2470,12 +2498,16 @@ static int repack_without_refs(const char **refnames, int n)\n \t}\n \n \t/* Write what remains */\n-\treturn commit_packed_refs();\n+\tret = commit_packed_refs();\n+\tif (ret && err)\n+\t\tstrbuf_addf(err, \"unable to overwrite old ref-pack file: %s\",\n+\t\t\t strerror(errno));\n+\treturn ret;\n }\n \n static int repack_without_ref(const char *refname)\n {\n-\treturn repack_without_refs(&refname, 1);\n+\treturn repack_without_refs(&refname, 1, NULL);\n }\n \n static int delete_ref_loose(struct ref_lock *lock, int flag)\n@@ -2723,9 +2755,12 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)\n \t starts_with(refname, \"refs/remotes/\") ||\n \t starts_with(refname, \"refs/notes/\") ||\n \t !strcmp(refname, \"HEAD\"))) {\n-\t\tif (safe_create_leading_directories(logfile) < 0)\n-\t\t\treturn error(\"unable to create directory for %s\",\n-\t\t\t\t logfile);\n+\t\tif (safe_create_leading_directories(logfile) < 0) {\n+\t\t\tint save_errno = errno;\n+\t\t\terror(\"unable to create directory for %s\", logfile);\n+\t\t\terrno = save_errno;\n+\t\t\treturn -1;\n+\t\t}\n \t\toflags |= O_CREAT;\n \t}\n \n@@ -2736,15 +2771,22 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)\n \n \t\tif ((oflags & O_CREAT) && errno == EISDIR) {\n \t\t\tif (remove_empty_directories(logfile)) {\n-\t\t\t\treturn error(\"There are still logs under '%s'\",\n-\t\t\t\t\t logfile);\n-\t\t\t}\n+\t\t\t\tint save_errno = errno;\n+\t\t\t\terror(\"There are still logs under '%s'\",\n+\t\t\t\t logfile);\n+\t\t\t\terrno = save_errno;\n+\t\t\t\treturn -1;\n+ \t\t\t}\n \t\t\tlogfd = open(logfile, oflags, 0666);\n \t\t}\n \n-\t\tif (logfd < 0)\n-\t\t\treturn error(\"Unable to append to %s: %s\",\n-\t\t\t\t logfile, strerror(errno));\n+\t\tif (logfd < 0) {\n+\t\t\tint save_errno = errno;\n+\t\t\terror(\"Unable to append to %s: %s\", logfile,\n+\t\t\t strerror(errno));\n+\t\t\terrno = save_errno;\n+\t\t\treturn -1;\n+\t\t}\n \t}\n \n \tadjust_shared_perm(logfile);\n@@ -2784,8 +2826,19 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,\n \t\tlen += copy_msg(logrec + len - 1, msg) - 1;\n \twritten = len <= maxlen ? write_in_full(logfd, logrec, len) : -1;\n \tfree(logrec);\n-\tif (close(logfd) != 0 || written != len)\n-\t\treturn error(\"Unable to append to %s\", log_file);\n+\tif (written != len) {\n+\t\tint save_errno = errno;\n+\t\tclose(logfd);\n+\t\terror(\"Unable to append to %s\", log_file);\n+\t\terrno = save_errno;\n+\t\treturn -1;\n+\t}\n+\tif (close(logfd)) {\n+\t\tint save_errno = errno;\n+\t\terror(\"Unable to append to %s\", log_file);\n+\t\terrno = save_errno;\n+\t\treturn -1;\n+\t}\n \treturn 0;\n }\n \n@@ -2800,8 +2853,10 @@ int write_ref_sha1(struct ref_lock *lock,\n \tstatic char term = '\\n';\n \tstruct object *o;\n \n-\tif (!lock)\n+\tif (!lock) {\n+\t\terrno = EINVAL;\n \t\treturn -1;\n+\t}\n \tif (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {\n \t\tunlock_ref(lock);\n \t\treturn 0;\n@@ -2811,19 +2866,23 @@ int write_ref_sha1(struct ref_lock *lock,\n \t\terror(\"Trying to write ref %s with nonexistent object %s\",\n \t\t\tlock->ref_name, sha1_to_hex(sha1));\n \t\tunlock_ref(lock);\n-\t\treturn -1;\n+\t\terrno = EINVAL;\n+ \t\treturn -1;\n \t}\n \tif (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) {\n \t\terror(\"Trying to write non-commit object %s to branch %s\",\n \t\t\tsha1_to_hex(sha1), lock->ref_name);\n \t\tunlock_ref(lock);\n+\t\terrno = EINVAL;\n \t\treturn -1;\n \t}\n \tif (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 ||\n-\t write_in_full(lock->lock_fd, &term, 1) != 1\n-\t\t|| close_ref(lock) < 0) {\n+\t write_in_full(lock->lock_fd, &term, 1) != 1 ||\n+\t close_ref(lock) < 0) {\n+\t\tint save_errno = errno;\n \t\terror(\"Couldn't write %s\", lock->lk->filename);\n \t\tunlock_ref(lock);\n+\t\terrno = save_errno;\n \t\treturn -1;\n \t}\n \tclear_loose_ref_cache(&ref_cache);\n@@ -3481,7 +3540,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t}\n \t}\n \n-\tret |= repack_without_refs(delnames, delnum);\n+\tret |= repack_without_refs(delnames, delnum, err);\n \tfor (i = 0; i < delnum; i++)\n \t\tunlink_or_warn(git_path(\"logs/%s\", delnames[i]));\n \tclear_loose_ref_cache(&ref_cache);\n-- \n2.0.0.582.ge25c160\n"},{"id":"243561","messageId":"1402093758-3162-8-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 07/40] refs.c: make ref_update_reject_duplicates take a strbuf argument for errors","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:28:45Z","receivedAt":"2014-06-06T22:28:45Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Make ref_update_reject_duplicates return any error that occurs through a\nnew strbuf argument. This means that when a transaction commit fails in\nthis function we will now be able to pass a helpful error message back to the\ncaller.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 7ec1f32..2d6941b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3456,6 +3456,7 @@ static int ref_update_compare(const void *r1, const void *r2)\n }\n \n static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n+\t\t\t\t\tstruct strbuf *err,\n \t\t\t\t\tenum action_on_err onerr)\n {\n \tint i;\n@@ -3463,6 +3464,9 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n \t\tif (!strcmp(updates[i - 1]->refname, updates[i]->refname)) {\n \t\t\tconst char *str =\n \t\t\t\t\"Multiple updates for ref '%s' not allowed.\";\n+\t\t\tif (err)\n+\t\t\t\tstrbuf_addf(err, str, updates[i]->refname);\n+\n \t\t\tswitch (onerr) {\n \t\t\tcase UPDATE_REFS_MSG_ON_ERR:\n \t\t\t\terror(str, updates[i]->refname); break;\n@@ -3493,7 +3497,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-- \n2.0.0.582.ge25c160\n"},{"id":"243563","messageId":"1402093758-3162-9-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 08/40] refs.c: add an err argument to delete_ref_loose","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:28:46Z","receivedAt":"2014-06-06T22:28:46Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Add an err argument to delete_loose_ref so that we can pass a descriptive\nerror string back to the caller. Pass the err argument from transaction\ncommit to this function so that transaction users will have a nice error\nstring if the transaction failed due to delete_loose_ref.\n\nAdd a new function unlink_or_err that we can call from delete_ref_loose. This\nfunction is similar to unlink_or_warn except that we can pass it an err\nargument. If err is non-NULL the function will populate err instead of\nprinting a warning().\n\nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 35 +++++++++++++++++++++++++++++------\n 1 file changed, 29 insertions(+), 6 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 2d6941b..dd498cf 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2510,16 +2510,38 @@ static int repack_without_ref(const char *refname)\n \treturn repack_without_refs(&refname, 1, NULL);\n }\n \n-static int delete_ref_loose(struct ref_lock *lock, int flag)\n+static int add_err_if_unremovable(const char *op, const char *file,\n+\t\t\t\t struct strbuf *e, int rc)\n+{\n+\tint err = errno;\n+\tif (rc < 0 && errno != ENOENT) {\n+\t\tstrbuf_addf(e, \"unable to %s %s: %s\",\n+\t\t\t op, file, strerror(errno));\n+\t\terrno = err;\n+\t\treturn -1;\n+\t}\n+\treturn 0;\n+}\n+\n+static int unlink_or_err(const char *file, struct strbuf *err)\n+{\n+\tif (err)\n+\t\treturn add_err_if_unremovable(\"unlink\", file, err,\n+\t\t\t\t\t unlink(file));\n+\telse\n+\t\treturn unlink_or_warn(file);\n+}\n+\n+static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)\n {\n \tif (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {\n \t\t/* loose */\n-\t\tint err, i = strlen(lock->lk->filename) - 5; /* .lock */\n+\t\tint res, i = strlen(lock->lk->filename) - 5; /* .lock */\n \n \t\tlock->lk->filename[i] = 0;\n-\t\terr = unlink_or_warn(lock->lk->filename);\n+\t\tres = unlink_or_err(lock->lk->filename, err);\n \t\tlock->lk->filename[i] = '.';\n-\t\tif (err && errno != ENOENT)\n+\t\tif (res && errno != ENOENT)\n \t\t\treturn 1;\n \t}\n \treturn 0;\n@@ -2533,7 +2555,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n \tlock = lock_ref_sha1_basic(refname, sha1, delopt, &flag);\n \tif (!lock)\n \t\treturn 1;\n-\tret |= delete_ref_loose(lock, flag);\n+\tret |= delete_ref_loose(lock, flag, NULL);\n \n \t/* removing the loose one could have resurrected an earlier\n \t * packed one. Also, if it was not loose we need to repack\n@@ -3540,7 +3562,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \n \t\tif (update->lock) {\n \t\t\tdelnames[delnum++] = update->lock->ref_name;\n-\t\t\tret |= delete_ref_loose(update->lock, update->type);\n+\t\t\tret |= delete_ref_loose(update->lock, update->type,\n+\t\t\t\t\t\terr);\n \t\t}\n \t}\n \n-- \n2.0.0.582.ge25c160\n"},{"id":"243529","messageId":"1402093758-3162-10-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 09/40] refs.c: make update_ref_write update a strbuf on failure","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:28:47Z","receivedAt":"2014-06-06T22:28:47Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change update_ref_write to also update an error strbuf on failure.\nThis makes the error available to ref_transaction_commit callers if the\ntransaction failed due to update_ref_sha1/write_ref_sha1 failures.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex dd498cf..6696082 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3343,10 +3343,13 @@ static struct ref_lock *update_ref_lock(const char *refname,\n \n static int update_ref_write(const char *action, const char *refname,\n \t\t\t const unsigned char *sha1, struct ref_lock *lock,\n-\t\t\t enum action_on_err onerr)\n+\t\t\t struct strbuf *err, enum action_on_err onerr)\n {\n \tif (write_ref_sha1(lock, sha1, action) < 0) {\n \t\tconst char *str = \"Cannot update the ref '%s'.\";\n+\t\tif (err)\n+\t\t\tstrbuf_addf(err, str, refname);\n+\n \t\tswitch (onerr) {\n \t\tcase UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;\n \t\tcase UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;\n@@ -3467,7 +3470,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@@ -3549,7 +3552,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-- \n2.0.0.582.ge25c160\n"},{"id":"243559","messageId":"1402093758-3162-11-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 10/40] update-ref: use err argument to get error from ref_transaction_commit","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:28:48Z","receivedAt":"2014-06-06T22:28:48Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Call ref_transaction_commit with QUIET_ON_ERR and use the strbuf that is\nreturned to print a log message if/after the transaction fails.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n builtin/update-ref.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 22617af..aec9004 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -342,6 +342,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \tconst char *refname, *oldval, *msg = NULL;\n \tunsigned char sha1[20], oldsha1[20];\n \tint delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0;\n+\tstruct strbuf err = STRBUF_INIT;\n \tstruct option options[] = {\n \t\tOPT_STRING( 'm', NULL, &msg, N_(\"reason\"), N_(\"reason of the update\")),\n \t\tOPT_BOOL('d', NULL, &delete, N_(\"delete the reference\")),\n@@ -359,18 +360,17 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \t\tdie(\"Refusing to perform update with empty message.\");\n \n \tif (read_stdin) {\n-\t\tint ret;\n \t\ttransaction = ref_transaction_begin();\n-\n \t\tif (delete || no_deref || argc > 0)\n \t\t\tusage_with_options(git_update_ref_usage, options);\n \t\tif (end_null)\n \t\t\tline_termination = '\\0';\n \t\tupdate_refs_stdin();\n-\t\tret = ref_transaction_commit(transaction, msg, NULL,\n-\t\t\t\t\t UPDATE_REFS_DIE_ON_ERR);\n+\t\tif (ref_transaction_commit(transaction, msg, &err,\n+\t\t\t\t\t UPDATE_REFS_QUIET_ON_ERR))\n+\t\t\tdie(\"%s\", err.buf);\n \t\tref_transaction_free(transaction);\n-\t\treturn ret;\n+\t\treturn 0;\n \t}\n \n \tif (end_null)\n-- \n2.0.0.582.ge25c160\n"},{"id":"243553","messageId":"1402093758-3162-12-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 11/40] refs.c: remove the onerr argument to ref_transaction_commit","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:28:49Z","receivedAt":"2014-06-06T22:28:49Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Since all callers now use QUIET_ON_ERR we no longer need to provide an onerr\nargument any more. Remove the onerr argument from the ref_transaction_commit\nsignature.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n builtin/update-ref.c | 3 +--\n refs.c | 22 +++++++---------------\n refs.h | 3 +--\n 3 files changed, 9 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex aec9004..88ab785 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -366,8 +366,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \t\tif (end_null)\n \t\t\tline_termination = '\\0';\n \t\tupdate_refs_stdin();\n-\t\tif (ref_transaction_commit(transaction, msg, &err,\n-\t\t\t\t\t UPDATE_REFS_QUIET_ON_ERR))\n+\t\tif (ref_transaction_commit(transaction, msg, &err))\n \t\t\tdie(\"%s\", err.buf);\n \t\tref_transaction_free(transaction);\n \t\treturn 0;\ndiff --git a/refs.c b/refs.c\nindex 6696082..fb44978 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3481,8 +3481,7 @@ static int ref_update_compare(const void *r1, const void *r2)\n }\n \n static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n-\t\t\t\t\tstruct strbuf *err,\n-\t\t\t\t\tenum action_on_err onerr)\n+\t\t\t\t\tstruct strbuf *err)\n {\n \tint i;\n \tfor (i = 1; i < n; i++)\n@@ -3492,22 +3491,13 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n \t\t\tif (err)\n \t\t\t\tstrbuf_addf(err, str, updates[i]->refname);\n \n-\t\t\tswitch (onerr) {\n-\t\t\tcase UPDATE_REFS_MSG_ON_ERR:\n-\t\t\t\terror(str, updates[i]->refname); break;\n-\t\t\tcase UPDATE_REFS_DIE_ON_ERR:\n-\t\t\t\tdie(str, updates[i]->refname); break;\n-\t\t\tcase UPDATE_REFS_QUIET_ON_ERR:\n-\t\t\t\tbreak;\n-\t\t\t}\n \t\t\treturn 1;\n \t\t}\n \treturn 0;\n }\n \n int ref_transaction_commit(struct ref_transaction *transaction,\n-\t\t\t const char *msg, struct strbuf *err,\n-\t\t\t enum action_on_err onerr)\n+\t\t\t const char *msg, struct strbuf *err)\n {\n \tint ret = 0, delnum = 0, i;\n \tconst char **delnames;\n@@ -3522,7 +3512,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@@ -3534,7 +3524,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t\t (update->have_old ?\n \t\t\t\t\t\tupdate->old_sha1 : NULL),\n \t\t\t\t\t update->flags,\n-\t\t\t\t\t &update->type, onerr);\n+\t\t\t\t\t &update->type,\n+\t\t\t\t\t UPDATE_REFS_QUIET_ON_ERR);\n \t\tif (!update->lock) {\n \t\t\tif (err)\n \t\t\t\tstrbuf_addf(err, \"Cannot lock the ref '%s'.\",\n@@ -3552,7 +3543,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 94d4cd4..6ee9c9e 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -270,8 +270,7 @@ void ref_transaction_delete(struct ref_transaction *transaction,\n * the transaction failed. The string does not end in newline.\n */\n int ref_transaction_commit(struct ref_transaction *transaction,\n-\t\t\t const char *msg, struct strbuf *err,\n-\t\t\t enum action_on_err onerr);\n+\t\t\t const char *msg, struct strbuf *err);\n \n /*\n * Free an existing transaction and all associated data.\n-- \n2.0.0.582.ge25c160\n"},{"id":"243530","messageId":"1402093758-3162-13-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 12/40] refs.c: change ref_transaction_update() to do error checking and return status","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:28:50Z","receivedAt":"2014-06-06T22:28:50Z","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\nnon-zero on error. Update all callers to check ref_transaction_update() for\nerror. There are currently no conditions in _update that will return error but\nthere will be in the future. Add an err argument that will be updated on\nfailure. In future patches we will start doing both locking and checking\nfor name conflicts in _update instead of _commit at which time this function\nwill start returning errors for these conditions.\n\nAlso check for BUGs during update and die(BUG:...) if we are calling\n_update with have_old but the old_sha1 pointer is NULL.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n builtin/update-ref.c | 12 +++++++-----\n refs.c | 18 ++++++++++++------\n refs.h | 14 +++++++++-----\n 3 files changed, 28 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 88ab785..3067b11 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -16,6 +16,7 @@ static struct ref_transaction *transaction;\n \n static char line_termination = '\\n';\n static int update_flags;\n+static struct strbuf err = STRBUF_INIT;\n \n /*\n * Parse one whitespace- or NUL-terminated, possibly C-quoted argument\n@@ -197,8 +198,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, &err))\n+\t\tdie(\"%s\", err.buf);\n \n \tupdate_flags = 0;\n \tfree(refname);\n@@ -286,8 +288,9 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)\n \tif (*next != line_termination)\n \t\tdie(\"verify %s: extra input: %s\", refname, next);\n \n-\tref_transaction_update(transaction, refname, new_sha1, old_sha1,\n-\t\t\t update_flags, have_old);\n+\tif (ref_transaction_update(transaction, refname, new_sha1, old_sha1,\n+\t\t\t\t update_flags, have_old, &err))\n+\t\tdie(\"%s\", err.buf);\n \n \tupdate_flags = 0;\n \tfree(refname);\n@@ -342,7 +345,6 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \tconst char *refname, *oldval, *msg = NULL;\n \tunsigned char sha1[20], oldsha1[20];\n \tint delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0;\n-\tstruct strbuf err = STRBUF_INIT;\n \tstruct option options[] = {\n \t\tOPT_STRING( 'm', NULL, &msg, N_(\"reason\"), N_(\"reason of the update\")),\n \t\tOPT_BOOL('d', NULL, &delete, N_(\"delete the reference\")),\ndiff --git a/refs.c b/refs.c\nindex fb44978..f5e7a12 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3418,19 +3418,25 @@ 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-\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+\t\t\t struct strbuf *err)\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(\"BUG: 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 6ee9c9e..32edf3f 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -234,12 +234,16 @@ struct ref_transaction *ref_transaction_begin(void);\n * be deleted. If have_old is true, then old_sha1 holds the value\n * that the reference should have had before the update, or zeros if\n * it must not have existed beforehand.\n+ * Function returns 0 on success and non-zero on failure. A failure to update\n+ * means that the transaction as a whole has failed and will need to be\n+ * rolled back. On failure the err buffer will be updated.\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+\t\t\t struct strbuf *err);\n \n /*\n * Add a reference creation to transaction. new_sha1 is the value\n-- \n2.0.0.582.ge25c160\n"},{"id":"243531","messageId":"1402093758-3162-14-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 13/40] refs.c: change ref_transaction_create to do error checking and return status","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:28:51Z","receivedAt":"2014-06-06T22:28:51Z","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\nnon-zero on error. Update all callers to check the result of\nref_transaction_create(). There are currently no conditions in _create that\nwill return error but there will be in the future. Add an err argument that\nwill be updated on failure.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n builtin/update-ref.c | 4 +++-\n refs.c | 18 +++++++++++------\n refs.h | 55 +++++++++++++++++++++++++++++++++++++++++++++-------\n 3 files changed, 63 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 3067b11..41121fa 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -226,7 +226,9 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next)\n \tif (*next != line_termination)\n \t\tdie(\"create %s: extra input: %s\", refname, next);\n \n-\tref_transaction_create(transaction, refname, new_sha1, update_flags);\n+\tif (ref_transaction_create(transaction, refname, new_sha1,\n+\t\t\t\t update_flags, &err))\n+\t\tdie(\"%s\", err.buf);\n \n \tupdate_flags = 0;\n \tfree(refname);\ndiff --git a/refs.c b/refs.c\nindex f5e7a12..3cb99f6 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3439,18 +3439,24 @@ 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+\t\t\t struct strbuf *err)\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(\"BUG: 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 32edf3f..bda790e6 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -10,6 +10,45 @@ struct ref_lock {\n \tint force_write;\n };\n \n+/*\n+ * A ref_transaction represents a collection of ref updates\n+ * that should succeed or fail together.\n+ *\n+ * Calling sequence\n+ * ----------------\n+ * - Allocate and initialize a `struct ref_transaction` by calling\n+ * `ref_transaction_begin()`.\n+ *\n+ * - List intended ref updates by calling functions like\n+ * `ref_transaction_update()` and `ref_transaction_create()`.\n+ *\n+ * - Call `ref_transaction_commit()` to execute the transaction.\n+ * If this succeeds, the ref updates will have taken place and\n+ * the transaction cannot be rolled back.\n+ *\n+ * - At any time call `ref_transaction_free()` to discard the\n+ * transaction and free associated resources. In particular,\n+ * this rolls back the transaction if it has not been\n+ * successfully committed.\n+ *\n+ * Error handling\n+ * --------------\n+ *\n+ * On error, transaction functions append a message about what\n+ * went wrong to the 'err' argument. The message mentions what\n+ * ref was being updated (if any) when the error occurred so it\n+ * can be passed to 'die' or 'error' as-is.\n+ *\n+ * The message is appended to err without first clearing err.\n+ * This allows the caller to prepare preamble text to the generated\n+ * error message:\n+ *\n+ * strbuf_addf(&err, \"Error while doing foo-bar: \");\n+ * if (ref_transaction_update(..., &err)) {\n+ * ret = error(\"%s\", err.buf);\n+ * goto cleanup;\n+ * }\n+ */\n struct ref_transaction;\n \n /*\n@@ -236,7 +275,7 @@ struct ref_transaction *ref_transaction_begin(void);\n * it must not have existed beforehand.\n * Function returns 0 on success and non-zero on failure. A failure to update\n * means that the transaction as a whole has failed and will need to be\n- * rolled back. On failure the err buffer will be updated.\n+ * rolled back.\n */\n int ref_transaction_update(struct ref_transaction *transaction,\n \t\t\t const char *refname,\n@@ -250,11 +289,15 @@ int ref_transaction_update(struct ref_transaction *transaction,\n * that the reference should have after the update; it must not be the\n * null SHA-1. It is verified that the reference does not exist\n * already.\n+ * Function returns 0 on success and non-zero on failure. A failure to create\n+ * means that the transaction as a whole has failed and will need to be\n+ * rolled back.\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+\t\t\t struct strbuf *err);\n \n /*\n * Add a reference deletion to transaction. If have_old is true, then\n@@ -270,8 +313,6 @@ 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.\n- * If err is non-NULL we will add an error string to it to explain why\n- * the transaction failed. The string does not end in newline.\n */\n int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t const char *msg, struct strbuf *err);\n-- \n2.0.0.582.ge25c160\n"},{"id":"243533","messageId":"1402093758-3162-15-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 14/40] refs.c: update ref_transaction_delete to check for error and return status","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:28:52Z","receivedAt":"2014-06-06T22:28:52Z","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\nnon-zero of error. Update all callers to check the return for\nref_transaction_delete(). There are currently no conditions in _delete that\nwill return error but there will be in the future. Add an err argument that\nwill be updated on failure.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n builtin/update-ref.c | 5 +++--\n refs.c | 16 +++++++++++-----\n refs.h | 12 ++++++++----\n 3 files changed, 22 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 41121fa..7c9c248 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -258,8 +258,9 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next)\n \tif (*next != line_termination)\n \t\tdie(\"delete %s: extra input: %s\", refname, next);\n \n-\tref_transaction_delete(transaction, refname, old_sha1,\n-\t\t\t update_flags, have_old);\n+\tif (ref_transaction_delete(transaction, refname, old_sha1,\n+\t\t\t\t update_flags, have_old, &err))\n+\t\tdie(\"%s\", err.buf);\n \n \tupdate_flags = 0;\n \tfree(refname);\ndiff --git a/refs.c b/refs.c\nindex 3cb99f6..d9b99c7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3459,19 +3459,25 @@ 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+\t\t\t struct strbuf *err)\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(\"BUG: 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 bda790e6..58ba365 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -303,11 +303,15 @@ int ref_transaction_create(struct ref_transaction *transaction,\n * Add a reference deletion to transaction. If have_old is true, then\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+ * Function returns 0 on success and non-zero on failure. A failure to delete\n+ * means that the transaction as a whole has failed and will need to be\n+ * rolled back.\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+\t\t\t struct strbuf *err);\n \n /*\n * Commit all of the changes that have been queued in transaction, as\n-- \n2.0.0.582.ge25c160\n"},{"id":"243532","messageId":"1402093758-3162-16-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 15/40] refs.c: make ref_transaction_begin take an err argument","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:28:53Z","receivedAt":"2014-06-06T22:28:53Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Add an err argument to _begin so that on non-fatal failures in future ref\nbackends we can report a nice error back to the caller.\nWhile _begin can currently never fail for other reasons than OOM, in which\ncase we die() anyway, we may add other types of backends in the future.\nFor example, a hypothetical MySQL backend could fail in _being with\n\"Can not connect to MySQL server. No route to host\".\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n builtin/update-ref.c | 2 +-\n refs.c | 2 +-\n refs.h | 2 +-\n 3 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 7c9c248..c6ad0be 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -365,7 +365,7 @@ 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\ttransaction = ref_transaction_begin();\n+\t\ttransaction = ref_transaction_begin(&err);\n \t\tif (delete || no_deref || argc > 0)\n \t\t\tusage_with_options(git_update_ref_usage, options);\n \t\tif (end_null)\ndiff --git a/refs.c b/refs.c\nindex d9b99c7..ee87eda 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3387,7 +3387,7 @@ struct ref_transaction {\n \tsize_t nr;\n };\n \n-struct ref_transaction *ref_transaction_begin(void)\n+struct ref_transaction *ref_transaction_begin(struct strbuf *err)\n {\n \treturn xcalloc(1, sizeof(struct ref_transaction));\n }\ndiff --git a/refs.h b/refs.h\nindex 58ba365..743358f 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -257,7 +257,7 @@ enum action_on_err {\n * Begin a reference transaction. The reference transaction must\n * be freed by calling ref_transaction_free().\n */\n-struct ref_transaction *ref_transaction_begin(void);\n+struct ref_transaction *ref_transaction_begin(struct strbuf *err);\n \n /*\n * The following functions add a reference check or update to a\n-- \n2.0.0.582.ge25c160\n"},{"id":"243554","messageId":"1402093758-3162-17-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 16/40] refs.c: add transaction.status and track OPEN/CLOSED/ERROR","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:28:54Z","receivedAt":"2014-06-06T22:28:54Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Track the status of a transaction in a new status field. Check the field for\nsanity, i.e. that status must be OPEN when _commit/_create/_delete or\n_update is called or else die(BUG:...)\n\nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 40 +++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 39 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex ee87eda..dd98202 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3377,6 +3377,25 @@ struct ref_update {\n };\n \n /*\n+ * Transaction states.\n+ * OPEN: The transaction is in a valid state and can accept new updates.\n+ * An OPEN transaction can be committed.\n+ * CLOSED: If an open transaction is successfully committed the state will\n+ * change to CLOSED. No further changes can be made to a CLOSED\n+ * transaction.\n+ * CLOSED means that all updates have been successfully committed and\n+ * the only thing that remains is to free the completed transaction.\n+ * ERROR: The transaction has failed and is no longer committable.\n+ * No further changes can be made to a CLOSED transaction and it must\n+ * be rolled back using transaction_free.\n+ */\n+enum ref_transaction_state {\n+\tREF_TRANSACTION_OPEN = 0,\n+\tREF_TRANSACTION_CLOSED = 1,\n+\tREF_TRANSACTION_ERROR = 2,\n+};\n+\n+/*\n * Data structure for holding a reference transaction, which can\n * consist of checks and updates to multiple references, carried out\n * as atomically as possible. This structure is opaque to callers.\n@@ -3385,6 +3404,8 @@ struct ref_transaction {\n \tstruct ref_update **updates;\n \tsize_t alloc;\n \tsize_t nr;\n+\tenum ref_transaction_state state;\n+\tint status;\n };\n \n struct ref_transaction *ref_transaction_begin(struct strbuf *err)\n@@ -3427,6 +3448,9 @@ int ref_transaction_update(struct ref_transaction *transaction,\n {\n \tstruct ref_update *update;\n \n+\tif (transaction->state != REF_TRANSACTION_OPEN)\n+\t\tdie(\"BUG: update called for transaction that is not open\");\n+\n \tif (have_old && !old_sha1)\n \t\tdie(\"BUG: have_old is true but old_sha1 is NULL\");\n \n@@ -3447,6 +3471,9 @@ int ref_transaction_create(struct ref_transaction *transaction,\n {\n \tstruct ref_update *update;\n \n+\tif (transaction->state != REF_TRANSACTION_OPEN)\n+\t\tdie(\"BUG: create called for transaction that is not open\");\n+\n \tif (!new_sha1 || is_null_sha1(new_sha1))\n \t\tdie(\"BUG: create ref with null new_sha1\");\n \n@@ -3467,6 +3494,9 @@ int ref_transaction_delete(struct ref_transaction *transaction,\n {\n \tstruct ref_update *update;\n \n+\tif (transaction->state != REF_TRANSACTION_OPEN)\n+\t\tdie(\"BUG: delete called for transaction that is not open\");\n+\n \tif (have_old && !old_sha1)\n \t\tdie(\"BUG: have_old is true but old_sha1 is NULL\");\n \n@@ -3522,8 +3552,13 @@ 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 (transaction->state != REF_TRANSACTION_OPEN)\n+\t\tdie(\"BUG: commit called for transaction that is not open\");\n+\n+\tif (!n) {\n+\t\ttransaction->state = REF_TRANSACTION_CLOSED;\n \t\treturn 0;\n+\t}\n \n \t/* Allocate work space */\n \tdelnames = xmalloc(sizeof(*delnames) * n);\n@@ -3586,6 +3621,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \tclear_loose_ref_cache(&ref_cache);\n \n cleanup:\n+\ttransaction->state = ret ? REF_TRANSACTION_ERROR\n+\t\t: REF_TRANSACTION_CLOSED;\n+\n \tfor (i = 0; i < n; i++)\n \t\tif (updates[i]->lock)\n \t\t\tunlock_ref(updates[i]->lock);\n-- \n2.0.0.582.ge25c160\n"},{"id":"243535","messageId":"1402093758-3162-18-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 17/40] tag.c: use ref transactions when doing updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:28:55Z","receivedAt":"2014-06-06T22:28:55Z","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\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n builtin/tag.c | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex c6e8a71..c9bfc9a 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -548,7 +548,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@@ -556,6 +555,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tconst char *msgfile = NULL, *keyid = NULL;\n \tstruct msg_arg msg = { 0, STRBUF_INIT };\n \tstruct commit_list *with_commit = NULL;\n+\tstruct ref_transaction *transaction;\n+\tstruct strbuf err = STRBUF_INIT;\n \tstruct option options[] = {\n \t\tOPT_CMDMODE('l', \"list\", &cmdmode, N_(\"list tag names\"), 'l'),\n \t\t{ OPTION_INTEGER, 'n', NULL, &lines, N_(\"n\"),\n@@ -701,11 +702,13 @@ 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(&err);\n+\tif (!transaction ||\n+\t ref_transaction_update(transaction, ref.buf, object, prev,\n+\t\t\t\t 0, !is_null_sha1(prev), &err) ||\n+\t ref_transaction_commit(transaction, NULL, &err))\n+\t\tdie(\"%s\", err.buf);\n+\tref_transaction_free(transaction);\n \tif (force && !is_null_sha1(prev) && hashcmp(prev, object))\n \t\tprintf(_(\"Updated tag '%s' (was %s)\\n\"), tag, find_unique_abbrev(prev, DEFAULT_ABBREV));\n \n-- \n2.0.0.582.ge25c160\n"},{"id":"243537","messageId":"1402093758-3162-19-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 18/40] replace.c: use the ref transaction functions for updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:28:56Z","receivedAt":"2014-06-06T22:28:56Z","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\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n builtin/replace.c | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex 4b3705d..cf92e5d 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -154,7 +154,8 @@ static int replace_object_sha1(const char *object_ref,\n \tunsigned char prev[20];\n \tenum object_type obj_type, repl_type;\n \tchar ref[PATH_MAX];\n-\tstruct ref_lock *lock;\n+\tstruct ref_transaction *transaction;\n+\tstruct strbuf err = STRBUF_INIT;\n \n \tobj_type = sha1_object_info(object, NULL);\n \trepl_type = sha1_object_info(repl, NULL);\n@@ -167,12 +168,14 @@ static int replace_object_sha1(const char *object_ref,\n \n \tcheck_ref_valid(object, prev, ref, sizeof(ref), force);\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(&err);\n+\tif (!transaction ||\n+\t ref_transaction_update(transaction, ref, repl, prev,\n+\t\t\t\t 0, !is_null_sha1(prev), &err) ||\n+\t ref_transaction_commit(transaction, NULL, &err))\n+\t\tdie(\"%s\", err.buf);\n \n+\tref_transaction_free(transaction);\n \treturn 0;\n }\n \n-- \n2.0.0.582.ge25c160\n"},{"id":"243564","messageId":"1402093758-3162-20-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 19/40] commit.c: use ref transactions for updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:28:57Z","receivedAt":"2014-06-06T22:28:57Z","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\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n builtin/commit.c | 24 +++++++++++-------------\n 1 file changed, 11 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 99c2044..14cd9f4 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1613,11 +1613,12 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tconst char *index_file, *reflog_msg;\n \tchar *nl;\n \tunsigned char sha1[20];\n-\tstruct ref_lock *ref_lock;\n \tstruct commit_list *parents = NULL, **pptr = &parents;\n \tstruct stat statbuf;\n \tstruct commit *current_head = NULL;\n \tstruct commit_extra_header *extra = NULL;\n+\tstruct ref_transaction *transaction;\n+\tstruct strbuf err = STRBUF_INIT;\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage_with_options(builtin_commit_usage, builtin_commit_options);\n@@ -1739,16 +1740,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-\tif (!ref_lock) {\n-\t\trollback_index_files();\n-\t\tdie(_(\"cannot lock HEAD ref\"));\n-\t}\n-\n \tnl = strchr(sb.buf, '\\n');\n \tif (nl)\n \t\tstrbuf_setlen(&sb, nl + 1 - sb.buf);\n@@ -1757,10 +1748,17 @@ 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 (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {\n+\ttransaction = ref_transaction_begin(&err);\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, &err) ||\n+\t ref_transaction_commit(transaction, sb.buf, &err)) {\n \t\trollback_index_files();\n-\t\tdie(_(\"cannot update HEAD ref\"));\n+\t\tdie(\"%s\", err.buf);\n \t}\n+\tref_transaction_free(transaction);\n \n \tunlink(git_path(\"CHERRY_PICK_HEAD\"));\n \tunlink(git_path(\"REVERT_HEAD\"));\n-- \n2.0.0.582.ge25c160\n"},{"id":"243534","messageId":"1402093758-3162-21-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 20/40] sequencer.c: use ref transactions for all ref updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:28:58Z","receivedAt":"2014-06-06T22:28:58Z","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\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n sequencer.c | 24 ++++++++++++++++--------\n 1 file changed, 16 insertions(+), 8 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 0a80c58..fd8acaf 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -272,23 +272,31 @@ static int error_dirty_index(struct replay_opts *opts)\n static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n \t\t\tint unborn, struct replay_opts *opts)\n {\n-\tstruct ref_lock *ref_lock;\n+\tstruct ref_transaction *transaction;\n \tstruct strbuf sb = STRBUF_INIT;\n-\tint ret;\n+\tstruct strbuf err = STRBUF_INIT;\n \n \tread_cache();\n \tif (checkout_fast_forward(from, to, 1))\n \t\texit(1); /* the callee should have complained already */\n-\tref_lock = lock_any_ref_for_update(\"HEAD\", unborn ? null_sha1 : from,\n-\t\t\t\t\t 0, NULL);\n-\tif (!ref_lock)\n-\t\treturn error(_(\"Failed to lock HEAD during fast_forward_to\"));\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(&err);\n+\tif (!transaction ||\n+\t ref_transaction_update(transaction, \"HEAD\", to, from,\n+\t\t\t\t 0, !unborn, &err) ||\n+\t ref_transaction_commit(transaction, sb.buf, &err)) {\n+\t\tref_transaction_free(transaction);\n+\t\terror(\"%s\", err.buf);\n+\t\tstrbuf_release(&sb);\n+\t\tstrbuf_release(&err);\n+\t\treturn -1;\n+\t}\n \n \tstrbuf_release(&sb);\n-\treturn ret;\n+\tref_transaction_free(transaction);\n+\treturn 0;\n }\n \n static int do_recursive_merge(struct commit *base, struct commit *next,\n-- \n2.0.0.582.ge25c160\n"},{"id":"243549","messageId":"1402093758-3162-22-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 21/40] fast-import.c: change update_branch to use ref transactions","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:28:59Z","receivedAt":"2014-06-06T22:28:59Z","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\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n fast-import.c | 25 +++++++++++++++----------\n 1 file changed, 15 insertions(+), 10 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 6707a66..4a7b196 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1679,39 +1679,44 @@ found_entry:\n static int update_branch(struct branch *b)\n {\n \tstatic const char *msg = \"fast-import\";\n-\tstruct ref_lock *lock;\n+\tstruct ref_transaction *transaction;\n \tunsigned char old_sha1[20];\n+\tstruct strbuf err = STRBUF_INIT;\n \n \tif (read_ref(b->name, old_sha1))\n \t\thashclr(old_sha1);\n+\n \tif (is_null_sha1(b->sha1)) {\n \t\tif (b->delete)\n \t\t\tdelete_ref(b->name, old_sha1, 0);\n \t\treturn 0;\n \t}\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\tif (!old_cmit || !new_cmit)\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(&err);\n+\tif (!transaction ||\n+\t ref_transaction_update(transaction, b->name, b->sha1, old_sha1,\n+\t\t\t\t 0, 1, &err) ||\n+\t ref_transaction_commit(transaction, msg, &err)) {\n+\t\tref_transaction_free(transaction);\n+\t\terror(\"%s\", err.buf);\n+\t\tstrbuf_release(&err);\n+\t\treturn -1;\n+\t}\n+\tref_transaction_free(transaction);\n \treturn 0;\n }\n \n-- \n2.0.0.582.ge25c160\n"},{"id":"243551","messageId":"1402093758-3162-23-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 22/40] branch.c: use ref transaction for all ref updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:29:00Z","receivedAt":"2014-06-06T22:29:00Z","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.\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 existing. I.e. one thread could end up\noverwriting a branch even if the forcing flag is false.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n branch.c | 30 ++++++++++++++++--------------\n 1 file changed, 16 insertions(+), 14 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 660097b..c1eae00 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -226,7 +226,6 @@ void create_branch(const char *head,\n \t\t int force, int reflog, int clobber_head,\n \t\t int quiet, enum branch_track track)\n {\n-\tstruct ref_lock *lock = NULL;\n \tstruct commit *commit;\n \tunsigned char sha1[20];\n \tchar *real_ref, msg[PATH_MAX + 20];\n@@ -285,15 +284,6 @@ void create_branch(const char *head,\n \t\tdie(_(\"Not a valid branch point: '%s'.\"), start_name);\n \thashcpy(sha1, commit->object.sha1);\n \n-\tif (!dont_change_ref) {\n-\t\tlock = lock_any_ref_for_update(ref.buf, NULL, 0, NULL);\n-\t\tif (!lock)\n-\t\t\tdie_errno(_(\"Failed to lock ref for update\"));\n-\t}\n-\n-\tif (reflog)\n-\t\tlog_all_ref_updates = 1;\n-\n \tif (forcing)\n \t\tsnprintf(msg, sizeof msg, \"branch: Reset to %s\",\n \t\t\t start_name);\n@@ -301,13 +291,25 @@ void create_branch(const char *head,\n \t\tsnprintf(msg, sizeof msg, \"branch: Created from %s\",\n \t\t\t start_name);\n \n+\tif (reflog)\n+\t\tlog_all_ref_updates = 1;\n+\n+\tif (!dont_change_ref) {\n+\t\tstruct ref_transaction *transaction;\n+\t\tstruct strbuf err = STRBUF_INIT;\n+\n+\t\ttransaction = ref_transaction_begin(&err);\n+\t\tif (!transaction ||\n+\t\t ref_transaction_update(transaction, ref.buf, sha1,\n+\t\t\t\t\t null_sha1, 0, !forcing, &err) ||\n+\t\t ref_transaction_commit(transaction, msg, &err))\n+\t\t\tdie(\"%s\", err.buf);\n+\t\tref_transaction_free(transaction);\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-- \n2.0.0.582.ge25c160\n"},{"id":"243562","messageId":"1402093758-3162-24-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 23/40] refs.c: change update_ref to use a transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:29:01Z","receivedAt":"2014-06-06T22:29:01Z","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\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 25 +++++++++++++++++++++----\n 1 file changed, 21 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex dd98202..6d2caf7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3514,11 +3514,28 @@ int update_ref(const char *action, const char *refname,\n \t const unsigned char *sha1, const unsigned char *oldval,\n \t int flags, enum action_on_err onerr)\n {\n-\tstruct ref_lock *lock;\n-\tlock = update_ref_lock(refname, oldval, flags, NULL, onerr);\n-\tif (!lock)\n+\tstruct ref_transaction *t;\n+\tstruct strbuf err = STRBUF_INIT;\n+\n+\tt = ref_transaction_begin(&err);\n+\tif (!t ||\n+\t ref_transaction_update(t, refname, sha1, oldval, flags,\n+\t\t\t\t !!oldval, &err) ||\n+\t ref_transaction_commit(t, action, &err)) {\n+\t\tconst char *str = \"update_ref failed for ref '%s': %s\";\n+\n+\t\tref_transaction_free(t);\n+\t\tswitch (onerr) {\n+\t\tcase UPDATE_REFS_MSG_ON_ERR:\n+\t\t\terror(str, refname, err.buf); break;\n+\t\tcase UPDATE_REFS_DIE_ON_ERR:\n+\t\t\tdie(str, refname, err.buf); break;\n+\t\tcase UPDATE_REFS_QUIET_ON_ERR: break;\n+\t\t}\n+\t\tstrbuf_release(&err);\n \t\treturn 1;\n-\treturn update_ref_write(action, refname, sha1, lock, NULL, onerr);\n+\t}\n+\treturn 0;\n }\n \n static int ref_update_compare(const void *r1, const void *r2)\n-- \n2.0.0.582.ge25c160\n"},{"id":"243538","messageId":"1402093758-3162-25-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 24/40] receive-pack.c: use a reference transaction for updating the refs","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:29:02Z","receivedAt":"2014-06-06T22:29:02Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Wrap all the ref updates inside a transaction.\n\nSigned-off-by: Ronnie Sahlberg \n---\n builtin/receive-pack.c | 31 ++++++++++++++++++++++---------\n 1 file changed, 22 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex c323081..13f4a63 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -46,6 +46,7 @@ static void *head_name_to_free;\n static int sent_capabilities;\n static int shallow_update;\n static const char *alt_shallow_file;\n+static struct string_list error_strings = STRING_LIST_INIT_DUP;\n \n static enum deny_action parse_deny_action(const char *var, const char *value)\n {\n@@ -475,7 +476,6 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \tconst char *namespaced_name;\n \tunsigned char *old_sha1 = cmd->old_sha1;\n \tunsigned char *new_sha1 = cmd->new_sha1;\n-\tstruct ref_lock *lock;\n \n \t/* only refs/... are allowed */\n \tif (!starts_with(name, \"refs/\") || check_refname_format(name + 5, 0)) {\n@@ -576,19 +576,31 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\treturn NULL; /* good */\n \t}\n \telse {\n+\t\tstruct strbuf err = STRBUF_INIT;\n+\t\tstruct ref_transaction *transaction;\n+\n \t\tif (shallow_update && si->shallow_ref[cmd->index] &&\n \t\t update_shallow_ref(cmd, si))\n \t\t\treturn \"shallow error\";\n \n-\t\tlock = lock_any_ref_for_update(namespaced_name, old_sha1,\n-\t\t\t\t\t 0, NULL);\n-\t\tif (!lock) {\n-\t\t\trp_error(\"failed to lock %s\", name);\n-\t\t\treturn \"failed to lock\";\n-\t\t}\n-\t\tif (write_ref_sha1(lock, new_sha1, \"push\")) {\n-\t\t\treturn \"failed to write\"; /* error() already called */\n+\t\ttransaction = ref_transaction_begin(&err);\n+\t\tif (!transaction ||\n+\t\t ref_transaction_update(transaction, namespaced_name,\n+\t\t\t\t\t new_sha1, old_sha1, 0, 1, &err) ||\n+\t\t ref_transaction_commit(transaction, \"push\", &err)) {\n+\n+\t\t\tconst char *str;\n+\t\t\tstring_list_append(&error_strings, err.buf);\n+\t\t\tstr = error_strings.items[error_strings.nr - 1].string;\n+\t\t\tstrbuf_release(&err);\n+\n+\t\t\tref_transaction_free(transaction);\n+\t\t\trp_error(\"%s\", str);\n+\t\t\treturn str;\n \t\t}\n+\n+\t\tref_transaction_free(transaction);\n+\t\tstrbuf_release(&err);\n \t\treturn NULL; /* good */\n \t}\n }\n@@ -1215,5 +1227,6 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t\tpacket_flush(1);\n \tsha1_array_clear(&shallow);\n \tsha1_array_clear(&ref);\n+\tstring_list_clear(&error_strings, 0);\n \treturn 0;\n }\n-- \n2.0.0.582.ge25c160\n"},{"id":"243542","messageId":"1402093758-3162-26-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 25/40] fast-import.c: use a ref transaction when dumping tags","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:29:03Z","receivedAt":"2014-06-06T22:29:03Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Signed-off-by: Ronnie Sahlberg \n---\n fast-import.c | 29 +++++++++++++++++++++++------\n 1 file changed, 23 insertions(+), 6 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 4a7b196..587ef4a 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1735,15 +1735,32 @@ static void dump_tags(void)\n {\n \tstatic const char *msg = \"fast-import\";\n \tstruct tag *t;\n-\tstruct ref_lock *lock;\n-\tchar ref_name[PATH_MAX];\n+\tstruct strbuf ref_name = STRBUF_INIT;\n+\tstruct strbuf err = STRBUF_INIT;\n+\tstruct ref_transaction *transaction;\n \n+\ttransaction = ref_transaction_begin(&err);\n+\tif (!transaction) {\n+\t\tfailure |= error(\"%s\", err.buf);\n+\t\tgoto cleanup;\n+\t}\n \tfor (t = first_tag; t; t = t->next_tag) {\n-\t\tsprintf(ref_name, \"tags/%s\", t->name);\n-\t\tlock = lock_ref_sha1(ref_name, NULL);\n-\t\tif (!lock || write_ref_sha1(lock, t->sha1, msg) < 0)\n-\t\t\tfailure |= error(\"Unable to update %s\", ref_name);\n+\t\tstrbuf_reset(&ref_name);\n+\t\tstrbuf_addf(&ref_name, \"refs/tags/%s\", t->name);\n+\n+\t\tif (ref_transaction_update(transaction, ref_name.buf, t->sha1,\n+\t\t\t\t\t NULL, 0, 0, &err)) {\n+\t\t\tfailure |= error(\"%s\", err.buf);\n+\t\t\tgoto cleanup;\n+\t\t}\n \t}\n+\tif (ref_transaction_commit(transaction, msg, &err))\n+\t\tfailure |= error(\"%s\", err.buf);\n+\n+ cleanup:\n+\tref_transaction_free(transaction);\n+\tstrbuf_release(&ref_name);\n+\tstrbuf_release(&err);\n }\n \n static void dump_marks_helper(FILE *f,\n-- \n2.0.0.582.ge25c160\n"},{"id":"243550","messageId":"1402093758-3162-27-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 26/40] walker.c: use ref transaction for ref updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:29:04Z","receivedAt":"2014-06-06T22:29:04Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Switch to using ref transactions in walker_fetch(). As part of the refactoring\nto use ref transactions we also fix a potential memory leak where in the\noriginal code if write_ref_sha1() would fail we would end up returning from\nthe function without free()ing the msg string.\n\nNote that this function is only called when fetching from a remote HTTP\nrepository onto the local (most of the time single-user) repository which\nlikely means that the type of collissions that the previous locking would\nprotect against and cause the fetch to fail for to be even more rare.\n\nSigned-off-by: Ronnie Sahlberg \n---\n walker.c | 59 +++++++++++++++++++++++++++++++++++------------------------\n 1 file changed, 35 insertions(+), 24 deletions(-)\n\ndiff --git a/walker.c b/walker.c\nindex 1dd86b8..60d9f9e 100644\n--- a/walker.c\n+++ b/walker.c\n@@ -251,39 +251,36 @@ void walker_targets_free(int targets, char **target, const char **write_ref)\n int walker_fetch(struct walker *walker, int targets, char **target,\n \t\t const char **write_ref, const char *write_ref_log_details)\n {\n-\tstruct ref_lock **lock = xcalloc(targets, sizeof(struct ref_lock *));\n+\tstruct strbuf ref_name = STRBUF_INIT;\n+\tstruct strbuf err = STRBUF_INIT;\n+\tstruct ref_transaction *transaction = NULL;\n \tunsigned char *sha1 = xmalloc(targets * 20);\n-\tchar *msg;\n-\tint ret;\n+\tchar *msg = NULL;\n \tint i;\n \n \tsave_commit_buffer = 0;\n \n-\tfor (i = 0; i < targets; i++) {\n-\t\tif (!write_ref || !write_ref[i])\n-\t\t\tcontinue;\n-\n-\t\tlock[i] = lock_ref_sha1(write_ref[i], NULL);\n-\t\tif (!lock[i]) {\n-\t\t\terror(\"Can't lock ref %s\", write_ref[i]);\n-\t\t\tgoto unlock_and_fail;\n+\tif (write_ref) {\n+\t\ttransaction = ref_transaction_begin(&err);\n+\t\tif (!transaction) {\n+\t\t\terror(\"%s\", err.buf);\n+\t\t\tgoto rollback_and_fail;\n \t\t}\n \t}\n-\n \tif (!walker->get_recover)\n \t\tfor_each_ref(mark_complete, NULL);\n \n \tfor (i = 0; i < targets; i++) {\n \t\tif (interpret_target(walker, target[i], &sha1[20 * i])) {\n \t\t\terror(\"Could not interpret response from server '%s' as something to pull\", target[i]);\n-\t\t\tgoto unlock_and_fail;\n+\t\t\tgoto rollback_and_fail;\n \t\t}\n \t\tif (process(walker, lookup_unknown_object(&sha1[20 * i])))\n-\t\t\tgoto unlock_and_fail;\n+\t\t\tgoto rollback_and_fail;\n \t}\n \n \tif (loop(walker))\n-\t\tgoto unlock_and_fail;\n+\t\tgoto rollback_and_fail;\n \n \tif (write_ref_log_details) {\n \t\tmsg = xmalloc(strlen(write_ref_log_details) + 12);\n@@ -294,19 +291,33 @@ int walker_fetch(struct walker *walker, int targets, char **target,\n \tfor (i = 0; i < targets; i++) {\n \t\tif (!write_ref || !write_ref[i])\n \t\t\tcontinue;\n-\t\tret = write_ref_sha1(lock[i], &sha1[20 * i], msg ? msg : \"fetch (unknown)\");\n-\t\tlock[i] = NULL;\n-\t\tif (ret)\n-\t\t\tgoto unlock_and_fail;\n+\t\tstrbuf_reset(&ref_name);\n+\t\tstrbuf_addf(&ref_name, \"refs/%s\", write_ref[i]);\n+\t\tif (ref_transaction_update(transaction, ref_name.buf,\n+\t\t\t\t\t &sha1[20 * i], NULL, 0, 0,\n+\t\t\t\t\t &err)) {\n+\t\t\terror(\"%s\", err.buf);\n+\t\t\tgoto rollback_and_fail;\n+\t\t}\n+\t}\n+\tif (write_ref) {\n+\t\tif (ref_transaction_commit(transaction,\n+\t\t\t\t\t msg ? msg : \"fetch (unknown)\",\n+\t\t\t\t\t &err)) {\n+\t\t\terror(\"%s\", err.buf);\n+\t\t\tgoto rollback_and_fail;\n+\t\t}\n+\t\tref_transaction_free(transaction);\n \t}\n-\tfree(msg);\n \n+\tfree(msg);\n \treturn 0;\n \n-unlock_and_fail:\n-\tfor (i = 0; i < targets; i++)\n-\t\tif (lock[i])\n-\t\t\tunlock_ref(lock[i]);\n+rollback_and_fail:\n+\tref_transaction_free(transaction);\n+\tfree(msg);\n+\tstrbuf_release(&err);\n+\tstrbuf_release(&ref_name);\n \n \treturn -1;\n }\n-- \n2.0.0.582.ge25c160\n"},{"id":"243548","messageId":"1402093758-3162-28-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 27/40] refs.c: make lock_ref_sha1 static","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:29:05Z","receivedAt":"2014-06-06T22:29:05Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"No external callers reference lock_ref_sha1 any more so lets declare it static.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 2 +-\n refs.h | 3 ---\n 2 files changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 6d2caf7..d2fd419 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2139,7 +2139,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \treturn NULL;\n }\n \n-struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char *old_sha1)\n+static struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char *old_sha1)\n {\n \tchar refpath[PATH_MAX];\n \tif (check_refname_format(refname, 0))\ndiff --git a/refs.h b/refs.h\nindex 743358f..c38ee09 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -171,9 +171,6 @@ extern int ref_exists(const char *);\n */\n extern int peel_ref(const char *refname, unsigned char *sha1);\n \n-/** Locks a \"refs/\" ref returning the lock on success and NULL on failure. **/\n-extern struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char *old_sha1);\n-\n /** Locks any ref (for 'HEAD' type refs). */\n #define REF_NODEREF\t0x01\n extern struct ref_lock *lock_any_ref_for_update(const char *refname,\n-- \n2.0.0.582.ge25c160\n"},{"id":"243552","messageId":"1402093758-3162-29-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 28/40] refs.c: remove the update_ref_lock function","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:29:06Z","receivedAt":"2014-06-06T22:29:06Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Since we now only call update_ref_lock with onerr==QUIET_ON_ERR we no longer\nneed this function and can replace it with just calling lock_any_ref_for_update\ndirectly.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 30 ++++++------------------------\n 1 file changed, 6 insertions(+), 24 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex d2fd419..3796957 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3323,24 +3323,6 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)\n \treturn retval;\n }\n \n-static struct ref_lock *update_ref_lock(const char *refname,\n-\t\t\t\t\tconst unsigned char *oldval,\n-\t\t\t\t\tint flags, int *type_p,\n-\t\t\t\t\tenum action_on_err onerr)\n-{\n-\tstruct ref_lock *lock;\n-\tlock = lock_any_ref_for_update(refname, oldval, flags, type_p);\n-\tif (!lock) {\n-\t\tconst char *str = \"Cannot lock the ref '%s'.\";\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-\t\tcase UPDATE_REFS_QUIET_ON_ERR: break;\n-\t\t}\n-\t}\n-\treturn lock;\n-}\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 struct strbuf *err, enum action_on_err onerr)\n@@ -3590,12 +3572,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \tfor (i = 0; i < n; i++) {\n \t\tstruct ref_update *update = updates[i];\n \n-\t\tupdate->lock = update_ref_lock(update->refname,\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,\n-\t\t\t\t\t UPDATE_REFS_QUIET_ON_ERR);\n+\t\tupdate->lock = lock_any_ref_for_update(update->refname,\n+\t\t\t\t\t\t (update->have_old ?\n+\t\t\t\t\t\t\tupdate->old_sha1 :\n+\t\t\t\t\t\t\tNULL),\n+\t\t\t\t\t\t update->flags,\n+\t\t\t\t\t\t &update->type);\n \t\tif (!update->lock) {\n \t\t\tif (err)\n \t\t\t\tstrbuf_addf(err, \"Cannot lock the ref '%s'.\",\n-- \n2.0.0.582.ge25c160\n"},{"id":"243557","messageId":"1402093758-3162-30-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 29/40] refs.c: remove the update_ref_write function","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:29:07Z","receivedAt":"2014-06-06T22:29:07Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Since we only call update_ref_write from a single place and we only call it\nwith onerr==QUIET_ON_ERR we can just as well get rid of it and just call\nwrite_ref_sha1 directly. This changes the return status for _commit from\n1 to -1 on failures when writing to the ref. Eventually we will want\n_commit to start returning more detailed error conditions than the current\nsimple success/failure. For example if the commit failed due to name\nconflicts etc.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 35 +++++++++--------------------------\n 1 file changed, 9 insertions(+), 26 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 3796957..245c540 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3323,25 +3323,6 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)\n \treturn retval;\n }\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 struct strbuf *err, enum action_on_err onerr)\n-{\n-\tif (write_ref_sha1(lock, sha1, action) < 0) {\n-\t\tconst char *str = \"Cannot update the ref '%s'.\";\n-\t\tif (err)\n-\t\t\tstrbuf_addf(err, str, refname);\n-\n-\t\tswitch (onerr) {\n-\t\tcase UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;\n-\t\tcase UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;\n-\t\tcase UPDATE_REFS_QUIET_ON_ERR: break;\n-\t\t}\n-\t\treturn 1;\n-\t}\n-\treturn 0;\n-}\n-\n /**\n * Information needed for a single ref update. Set new_sha1 to the\n * new value or to zero to delete the ref. To check the old value\n@@ -3592,14 +3573,16 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\tstruct ref_update *update = updates[i];\n \n \t\tif (!is_null_sha1(update->new_sha1)) {\n-\t\t\tret = update_ref_write(msg,\n-\t\t\t\t\t update->refname,\n-\t\t\t\t\t update->new_sha1,\n-\t\t\t\t\t update->lock, 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\tret = write_ref_sha1(update->lock, update->new_sha1,\n+\t\t\t\t\t msg);\n+\t\t\tupdate->lock = NULL; /* freed by write_ref_sha1 */\n+\t\t\tif (ret) {\n+\t\t\t\tconst char *str = \"Cannot update the ref '%s'.\";\n+\n+\t\t\t\tif (err)\n+\t\t\t\t\tstrbuf_addf(err, str, update->refname);\n \t\t\t\tgoto cleanup;\n+\t\t\t}\n \t\t}\n \t}\n \n-- \n2.0.0.582.ge25c160\n"},{"id":"243556","messageId":"1402093758-3162-31-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 30/40] refs.c: remove lock_ref_sha1","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:29:08Z","receivedAt":"2014-06-06T22:29:08Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"lock_ref_sha1 was only called from one place in refc.c and only provided\na check that the refname was sane before adding back the initial \"refs/\"\npart of the ref path name, the initial \"refs/\" that this caller had already\nstripped off before calling lock_ref_sha1.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 15 +++++----------\n 1 file changed, 5 insertions(+), 10 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 245c540..bc750f4 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2139,15 +2139,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \treturn NULL;\n }\n \n-static struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char *old_sha1)\n-{\n-\tchar refpath[PATH_MAX];\n-\tif (check_refname_format(refname, 0))\n-\t\treturn NULL;\n-\tstrcpy(refpath, mkpath(\"refs/%s\", refname));\n-\treturn lock_ref_sha1_basic(refpath, old_sha1, 0, NULL);\n-}\n-\n struct ref_lock *lock_any_ref_for_update(const char *refname,\n \t\t\t\t\t const unsigned char *old_sha1,\n \t\t\t\t\t int flags, int *type_p)\n@@ -2356,8 +2347,12 @@ static void try_remove_empty_parents(char *name)\n /* make sure nobody touched the ref, and unlink */\n static void prune_ref(struct ref_to_prune *r)\n {\n-\tstruct ref_lock *lock = lock_ref_sha1(r->name + 5, r->sha1);\n+\tstruct ref_lock *lock;\n+\n+\tif (check_refname_format(r->name + 5, 0))\n+\t\treturn;\n \n+\tlock = lock_ref_sha1_basic(r->name, r->sha1, 0, NULL);\n \tif (lock) {\n \t\tunlink_or_warn(git_path(\"%s\", r->name));\n \t\tunlock_ref(lock);\n-- \n2.0.0.582.ge25c160\n"},{"id":"243555","messageId":"1402093758-3162-32-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 31/40] refs.c: make prune_ref use a transaction to delete the ref","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:29:09Z","receivedAt":"2014-06-06T22:29:09Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change prune_ref to delete the ref using a ref transaction. To do this we also\nneed to add a new flag REF_ISPRUNING that will tell the transaction that we\ndo not want to delete this ref from the packed refs. This flag is private to\nrefs.c and not exposed to external callers.\n\nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 28 +++++++++++++++++++++-------\n refs.h | 11 ++++++++++-\n 2 files changed, 31 insertions(+), 8 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex bc750f4..e19041a 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -30,6 +30,12 @@ static inline int bad_ref_char(int ch)\n }\n \n /*\n+ * Used as a flag to ref_transaction_delete when a loose ref is being\n+ * pruned.\n+ */\n+#define REF_ISPRUNING\t0x0100\n+\n+/*\n * Try to read one refname component from the front of refname. Return\n * the length of the component found, or -1 if the component is not\n * legal.\n@@ -2347,17 +2353,24 @@ static void try_remove_empty_parents(char *name)\n /* make sure nobody touched the ref, and unlink */\n static void prune_ref(struct ref_to_prune *r)\n {\n-\tstruct ref_lock *lock;\n+\tstruct ref_transaction *transaction;\n+\tstruct strbuf err = STRBUF_INIT;\n \n \tif (check_refname_format(r->name + 5, 0))\n \t\treturn;\n \n-\tlock = lock_ref_sha1_basic(r->name, r->sha1, 0, NULL);\n-\tif (lock) {\n-\t\tunlink_or_warn(git_path(\"%s\", r->name));\n-\t\tunlock_ref(lock);\n-\t\ttry_remove_empty_parents(r->name);\n+\ttransaction = ref_transaction_begin(&err);\n+\tif (!transaction ||\n+\t ref_transaction_delete(transaction, r->name, r->sha1,\n+\t\t\t\t REF_ISPRUNING, 1, &err) ||\n+\t ref_transaction_commit(transaction, NULL, &err)) {\n+\t\tref_transaction_free(transaction);\n+\t\terror(\"%s\", err.buf);\n+\t\tstrbuf_release(&err);\n+\t\treturn;\n \t}\n+\tref_transaction_free(transaction);\n+\ttry_remove_empty_parents(r->name);\n }\n \n static void prune_refs(struct ref_to_prune *r)\n@@ -3586,9 +3599,10 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\tstruct ref_update *update = updates[i];\n \n \t\tif (update->lock) {\n-\t\t\tdelnames[delnum++] = update->lock->ref_name;\n \t\t\tret |= delete_ref_loose(update->lock, update->type,\n \t\t\t\t\t\terr);\n+\t\t\tif (!(update->flags & REF_ISPRUNING))\n+\t\t\t\tdelnames[delnum++] = update->lock->ref_name;\n \t\t}\n \t}\n \ndiff --git a/refs.h b/refs.h\nindex c38ee09..dee7c8f 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -171,8 +171,17 @@ extern int ref_exists(const char *);\n */\n extern int peel_ref(const char *refname, unsigned char *sha1);\n \n-/** Locks any ref (for 'HEAD' type refs). */\n+/*\n+ * Flags controlling lock_any_ref_for_update(), ref_transaction_update(),\n+ * ref_transaction_create(), etc.\n+ * REF_NODEREF: act on the ref directly, instead of dereferencing\n+ * symbolic references.\n+ *\n+ * Flags >= 0x100 are reserved for internal use.\n+ */\n #define REF_NODEREF\t0x01\n+\n+/** Locks any ref (for 'HEAD' type refs). */\n extern struct ref_lock *lock_any_ref_for_update(const char *refname,\n \t\t\t\t\t\tconst unsigned char *old_sha1,\n \t\t\t\t\t\tint flags, int *type_p);\n-- \n2.0.0.582.ge25c160\n"},{"id":"243540","messageId":"1402093758-3162-33-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 32/40] refs.c: make delete_ref use a transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:29:10Z","receivedAt":"2014-06-06T22:29:10Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change delete_ref to use a ref transaction for the deletion. At the same time\nsince we no longer have any callers of repack_without_ref we can now delete\nthis function.\n\nChange delete_ref to return 0 on success and 1 on failure instead of the\nprevious 0 on success either 1 or -1 on failure.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 34 +++++++++++++---------------------\n 1 file changed, 13 insertions(+), 21 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex e19041a..4cdbc26 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2513,11 +2513,6 @@ static int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n \treturn ret;\n }\n \n-static int repack_without_ref(const char *refname)\n-{\n-\treturn repack_without_refs(&refname, 1, NULL);\n-}\n-\n static int add_err_if_unremovable(const char *op, const char *file,\n \t\t\t\t struct strbuf *e, int rc)\n {\n@@ -2557,24 +2552,21 @@ static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)\n \n int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n {\n-\tstruct ref_lock *lock;\n-\tint ret = 0, flag = 0;\n+\tstruct ref_transaction *transaction;\n+\tstruct strbuf err = STRBUF_INIT;\n \n-\tlock = lock_ref_sha1_basic(refname, sha1, delopt, &flag);\n-\tif (!lock)\n+\ttransaction = ref_transaction_begin(&err);\n+\tif (!transaction ||\n+\t ref_transaction_delete(transaction, refname, sha1, delopt,\n+\t\t\t\t sha1 && !is_null_sha1(sha1), &err) ||\n+\t ref_transaction_commit(transaction, NULL, &err)) {\n+\t\terror(\"%s\", err.buf);\n+\t\tref_transaction_free(transaction);\n+\t\tstrbuf_release(&err);\n \t\treturn 1;\n-\tret |= delete_ref_loose(lock, flag, NULL);\n-\n-\t/* removing the loose one could have resurrected an earlier\n-\t * packed one. Also, if it was not loose we need to repack\n-\t * without it.\n-\t */\n-\tret |= repack_without_ref(lock->ref_name);\n-\n-\tunlink_or_warn(git_path(\"logs/%s\", lock->ref_name));\n-\tclear_loose_ref_cache(&ref_cache);\n-\tunlock_ref(lock);\n-\treturn ret;\n+\t}\n+\tref_transaction_free(transaction);\n+\treturn 0;\n }\n \n /*\n-- \n2.0.0.582.ge25c160\n"},{"id":"243544","messageId":"1402093758-3162-34-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 33/40] refs.c: pass the ref log message to _create/delete/update instead of _commit","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:29:11Z","receivedAt":"2014-06-06T22:29:11Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change the reference transactions so that we pass the reflog message\nthrough to the create/delete/update function instead of the commit message.\nThis allows for individual messages for each change in a multi ref\ntransaction.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n branch.c | 4 ++--\n builtin/commit.c | 4 ++--\n builtin/fetch.c | 3 +--\n builtin/receive-pack.c | 7 ++++---\n builtin/replace.c | 4 ++--\n builtin/tag.c | 4 ++--\n builtin/update-ref.c | 13 +++++++------\n fast-import.c | 8 ++++----\n refs.c | 34 +++++++++++++++++++++-------------\n refs.h | 8 ++++----\n sequencer.c | 4 ++--\n walker.c | 5 ++---\n 12 files changed, 53 insertions(+), 45 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex c1eae00..e0439af 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -301,8 +301,8 @@ void create_branch(const char *head,\n \t\ttransaction = ref_transaction_begin(&err);\n \t\tif (!transaction ||\n \t\t ref_transaction_update(transaction, ref.buf, sha1,\n-\t\t\t\t\t null_sha1, 0, !forcing, &err) ||\n-\t\t ref_transaction_commit(transaction, msg, &err))\n+\t\t\t\t\t null_sha1, 0, !forcing, msg, &err) ||\n+\t\t ref_transaction_commit(transaction, &err))\n \t\t\tdie(\"%s\", err.buf);\n \t\tref_transaction_free(transaction);\n \t}\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 14cd9f4..e01b333 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1753,8 +1753,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\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, &err) ||\n-\t ref_transaction_commit(transaction, sb.buf, &err)) {\n+\t\t\t\t 0, !!current_head, sb.buf, &err) ||\n+\t ref_transaction_commit(transaction, &err)) {\n \t\trollback_index_files();\n \t\tdie(\"%s\", err.buf);\n \t}\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 55f457c..faa1233 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -673,10 +673,9 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t}\n \t\t}\n \t}\n-\n \tif (rc & STORE_REF_ERROR_DF_CONFLICT)\n \t\terror(_(\"some local refs could not be updated; try running\\n\"\n-\t\t \" 'git remote prune %s' to remove any old, conflicting \"\n+\t\t \"'git remote prune %s' to remove any old, conflicting \"\n \t\t \"branches\"), remote_name);\n \n abort:\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 13f4a63..5653fa2 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -586,10 +586,11 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\ttransaction = ref_transaction_begin(&err);\n \t\tif (!transaction ||\n \t\t ref_transaction_update(transaction, namespaced_name,\n-\t\t\t\t\t new_sha1, old_sha1, 0, 1, &err) ||\n-\t\t ref_transaction_commit(transaction, \"push\", &err)) {\n-\n+\t\t\t\t\t new_sha1, old_sha1, 0, 1, \"push\",\n+\t\t\t\t\t &err) ||\n+\t\t ref_transaction_commit(transaction, &err)) {\n \t\t\tconst char *str;\n+\n \t\t\tstring_list_append(&error_strings, err.buf);\n \t\t\tstr = error_strings.items[error_strings.nr - 1].string;\n \t\t\tstrbuf_release(&err);\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex cf92e5d..c42b26e 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -171,8 +171,8 @@ static int replace_object_sha1(const char *object_ref,\n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction ||\n \t ref_transaction_update(transaction, ref, repl, prev,\n-\t\t\t\t 0, !is_null_sha1(prev), &err) ||\n-\t ref_transaction_commit(transaction, NULL, &err))\n+\t\t\t\t 0, !is_null_sha1(prev), NULL, &err) ||\n+\t ref_transaction_commit(transaction, &err))\n \t\tdie(\"%s\", err.buf);\n \n \tref_transaction_free(transaction);\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex c9bfc9a..74af63e 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -705,8 +705,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction ||\n \t ref_transaction_update(transaction, ref.buf, object, prev,\n-\t\t\t\t 0, !is_null_sha1(prev), &err) ||\n-\t ref_transaction_commit(transaction, NULL, &err))\n+\t\t\t\t 0, !is_null_sha1(prev), NULL, &err) ||\n+\t ref_transaction_commit(transaction, &err))\n \t\tdie(\"%s\", err.buf);\n \tref_transaction_free(transaction);\n \tif (force && !is_null_sha1(prev) && hashcmp(prev, object))\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex c6ad0be..28b478a 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -16,6 +16,7 @@ static struct ref_transaction *transaction;\n \n static char line_termination = '\\n';\n static int update_flags;\n+static const char *msg;\n static struct strbuf err = STRBUF_INIT;\n \n /*\n@@ -199,7 +200,7 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next)\n \t\tdie(\"update %s: extra input: %s\", refname, next);\n \n \tif (ref_transaction_update(transaction, refname, new_sha1, old_sha1,\n-\t\t\t\t update_flags, have_old, &err))\n+\t\t\t\t update_flags, have_old, msg, &err))\n \t\tdie(\"%s\", err.buf);\n \n \tupdate_flags = 0;\n@@ -227,7 +228,7 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next)\n \t\tdie(\"create %s: extra input: %s\", refname, next);\n \n \tif (ref_transaction_create(transaction, refname, new_sha1,\n-\t\t\t\t update_flags, &err))\n+\t\t\t\t update_flags, msg, &err))\n \t\tdie(\"%s\", err.buf);\n \n \tupdate_flags = 0;\n@@ -259,7 +260,7 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next)\n \t\tdie(\"delete %s: extra input: %s\", refname, next);\n \n \tif (ref_transaction_delete(transaction, refname, old_sha1,\n-\t\t\t\t update_flags, have_old, &err))\n+\t\t\t\t update_flags, have_old, msg, &err))\n \t\tdie(\"%s\", err.buf);\n \n \tupdate_flags = 0;\n@@ -292,7 +293,7 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)\n \t\tdie(\"verify %s: extra input: %s\", refname, next);\n \n \tif (ref_transaction_update(transaction, refname, new_sha1, old_sha1,\n-\t\t\t\t update_flags, have_old, &err))\n+\t\t\t\t update_flags, have_old, msg, &err))\n \t\tdie(\"%s\", err.buf);\n \n \tupdate_flags = 0;\n@@ -345,7 +346,7 @@ static void update_refs_stdin(void)\n \n int cmd_update_ref(int argc, const char **argv, const char *prefix)\n {\n-\tconst char *refname, *oldval, *msg = NULL;\n+\tconst char *refname, *oldval;\n \tunsigned char sha1[20], oldsha1[20];\n \tint delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0;\n \tstruct option options[] = {\n@@ -371,7 +372,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, &err))\n \t\t\tdie(\"%s\", err.buf);\n \t\tref_transaction_free(transaction);\n \t\treturn 0;\ndiff --git a/fast-import.c b/fast-import.c\nindex 587ef4a..7ca8b5a 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1709,8 +1709,8 @@ static int update_branch(struct branch *b)\n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction ||\n \t ref_transaction_update(transaction, b->name, b->sha1, old_sha1,\n-\t\t\t\t 0, 1, &err) ||\n-\t ref_transaction_commit(transaction, msg, &err)) {\n+\t\t\t\t 0, 1, msg, &err) ||\n+\t ref_transaction_commit(transaction, &err)) {\n \t\tref_transaction_free(transaction);\n \t\terror(\"%s\", err.buf);\n \t\tstrbuf_release(&err);\n@@ -1749,12 +1749,12 @@ static void dump_tags(void)\n \t\tstrbuf_addf(&ref_name, \"refs/tags/%s\", t->name);\n \n \t\tif (ref_transaction_update(transaction, ref_name.buf, t->sha1,\n-\t\t\t\t\t NULL, 0, 0, &err)) {\n+\t\t\t\t\t NULL, 0, 0, msg, &err)) {\n \t\t\tfailure |= error(\"%s\", err.buf);\n \t\t\tgoto cleanup;\n \t\t}\n \t}\n-\tif (ref_transaction_commit(transaction, msg, &err))\n+\tif (ref_transaction_commit(transaction, &err))\n \t\tfailure |= error(\"%s\", err.buf);\n \n cleanup:\ndiff --git a/refs.c b/refs.c\nindex 4cdbc26..88d9351 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2362,8 +2362,8 @@ static void prune_ref(struct ref_to_prune *r)\n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction ||\n \t ref_transaction_delete(transaction, r->name, r->sha1,\n-\t\t\t\t REF_ISPRUNING, 1, &err) ||\n-\t ref_transaction_commit(transaction, NULL, &err)) {\n+\t\t\t\t REF_ISPRUNING, 1, NULL, &err) ||\n+\t ref_transaction_commit(transaction, &err)) {\n \t\tref_transaction_free(transaction);\n \t\terror(\"%s\", err.buf);\n \t\tstrbuf_release(&err);\n@@ -2558,8 +2558,8 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction ||\n \t ref_transaction_delete(transaction, refname, sha1, delopt,\n-\t\t\t\t sha1 && !is_null_sha1(sha1), &err) ||\n-\t ref_transaction_commit(transaction, NULL, &err)) {\n+\t\t\t\t sha1 && !is_null_sha1(sha1), NULL, &err) ||\n+\t ref_transaction_commit(transaction, &err)) {\n \t\terror(\"%s\", err.buf);\n \t\tref_transaction_free(transaction);\n \t\tstrbuf_release(&err);\n@@ -3336,6 +3336,7 @@ struct ref_update {\n \tint have_old; /* 1 if old_sha1 is valid, 0 otherwise */\n \tstruct ref_lock *lock;\n \tint type;\n+\tchar *msg;\n \tconst char refname[FLEX_ARRAY];\n };\n \n@@ -3383,9 +3384,10 @@ void ref_transaction_free(struct ref_transaction *transaction)\n \tif (!transaction)\n \t\treturn;\n \n-\tfor (i = 0; i < transaction->nr; i++)\n+\tfor (i = 0; i < transaction->nr; i++) {\n+\t\tfree(transaction->updates[i]->msg);\n \t\tfree(transaction->updates[i]);\n-\n+\t}\n \tfree(transaction->updates);\n \tfree(transaction);\n }\n@@ -3406,7 +3408,7 @@ 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+\t\t\t int flags, int have_old, const char *msg,\n \t\t\t struct strbuf *err)\n {\n \tstruct ref_update *update;\n@@ -3423,13 +3425,15 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \tupdate->have_old = have_old;\n \tif (have_old)\n \t\thashcpy(update->old_sha1, old_sha1);\n+\tif (msg)\n+\t\tupdate->msg = xstrdup(msg);\n \treturn 0;\n }\n \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+\t\t\t int flags, const char *msg,\n \t\t\t struct strbuf *err)\n {\n \tstruct ref_update *update;\n@@ -3446,13 +3450,15 @@ int ref_transaction_create(struct ref_transaction *transaction,\n \thashclr(update->old_sha1);\n \tupdate->flags = flags;\n \tupdate->have_old = 1;\n+\tif (msg)\n+\t\tupdate->msg = xstrdup(msg);\n \treturn 0;\n }\n \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+\t\t\t int flags, int have_old, const char *msg,\n \t\t\t struct strbuf *err)\n {\n \tstruct ref_update *update;\n@@ -3470,6 +3476,8 @@ int ref_transaction_delete(struct ref_transaction *transaction,\n \t\tassert(!is_null_sha1(old_sha1));\n \t\thashcpy(update->old_sha1, old_sha1);\n \t}\n+\tif (msg)\n+\t\tupdate->msg = xstrdup(msg);\n \treturn 0;\n }\n \n@@ -3483,8 +3491,8 @@ int update_ref(const char *action, const char *refname,\n \tt = ref_transaction_begin(&err);\n \tif (!t ||\n \t ref_transaction_update(t, refname, sha1, oldval, flags,\n-\t\t\t\t !!oldval, &err) ||\n-\t ref_transaction_commit(t, action, &err)) {\n+\t\t\t\t !!oldval, action, &err) ||\n+\t ref_transaction_commit(t, &err)) {\n \t\tconst char *str = \"update_ref failed for ref '%s': %s\";\n \n \t\tref_transaction_free(t);\n@@ -3525,7 +3533,7 @@ 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, struct strbuf *err)\n+\t\t\t struct strbuf *err)\n {\n \tint ret = 0, delnum = 0, i;\n \tconst char **delnames;\n@@ -3574,7 +3582,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \n \t\tif (!is_null_sha1(update->new_sha1)) {\n \t\t\tret = write_ref_sha1(update->lock, update->new_sha1,\n-\t\t\t\t\t msg);\n+\t\t\t\t\t update->msg);\n \t\t\tupdate->lock = NULL; /* freed by write_ref_sha1 */\n \t\t\tif (ret) {\n \t\t\t\tconst char *str = \"Cannot update the ref '%s'.\";\ndiff --git a/refs.h b/refs.h\nindex dee7c8f..5c6c20f 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -287,7 +287,7 @@ 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+\t\t\t int flags, int have_old, const char *msg,\n \t\t\t struct strbuf *err);\n \n /*\n@@ -302,7 +302,7 @@ int ref_transaction_update(struct ref_transaction *transaction,\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+\t\t\t int flags, const char *msg,\n \t\t\t struct strbuf *err);\n \n /*\n@@ -316,7 +316,7 @@ int ref_transaction_create(struct ref_transaction *transaction,\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+\t\t\t int flags, int have_old, const char *msg,\n \t\t\t struct strbuf *err);\n \n /*\n@@ -325,7 +325,7 @@ int ref_transaction_delete(struct ref_transaction *transaction,\n * problem.\n */\n int ref_transaction_commit(struct ref_transaction *transaction,\n-\t\t\t const char *msg, struct strbuf *err);\n+\t\t\t struct strbuf *err);\n \n /*\n * Free an existing transaction and all associated data.\ndiff --git a/sequencer.c b/sequencer.c\nindex fd8acaf..f9906ef 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -285,8 +285,8 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction ||\n \t ref_transaction_update(transaction, \"HEAD\", to, from,\n-\t\t\t\t 0, !unborn, &err) ||\n-\t ref_transaction_commit(transaction, sb.buf, &err)) {\n+\t\t\t\t 0, !unborn, sb.buf, &err) ||\n+\t ref_transaction_commit(transaction, &err)) {\n \t\tref_transaction_free(transaction);\n \t\terror(\"%s\", err.buf);\n \t\tstrbuf_release(&sb);\ndiff --git a/walker.c b/walker.c\nindex 60d9f9e..fd9ef87 100644\n--- a/walker.c\n+++ b/walker.c\n@@ -295,15 +295,14 @@ int walker_fetch(struct walker *walker, int targets, char **target,\n \t\tstrbuf_addf(&ref_name, \"refs/%s\", write_ref[i]);\n \t\tif (ref_transaction_update(transaction, ref_name.buf,\n \t\t\t\t\t &sha1[20 * i], NULL, 0, 0,\n+\t\t\t\t\t msg ? msg : \"fetch (unknown)\",\n \t\t\t\t\t &err)) {\n \t\t\terror(\"%s\", err.buf);\n \t\t\tgoto rollback_and_fail;\n \t\t}\n \t}\n \tif (write_ref) {\n-\t\tif (ref_transaction_commit(transaction,\n-\t\t\t\t\t msg ? msg : \"fetch (unknown)\",\n-\t\t\t\t\t &err)) {\n+\t\tif (ref_transaction_commit(transaction, &err)) {\n \t\t\terror(\"%s\", err.buf);\n \t\t\tgoto rollback_and_fail;\n \t\t}\n-- \n2.0.0.582.ge25c160\n"},{"id":"243547","messageId":"1402093758-3162-35-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 34/40] refs.c: pass NULL as *flags to read_ref_full","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:29:12Z","receivedAt":"2014-06-06T22:29:12Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"We call read_ref_full with a pointer to flags from rename_ref but since\nwe never actually use the returned flags we can just pass NULL here instead.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 88d9351..8848dbf 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2657,7 +2657,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \t\tgoto rollback;\n \t}\n \n-\tif (!read_ref_full(newrefname, sha1, 1, &flag) &&\n+\tif (!read_ref_full(newrefname, sha1, 1, NULL) &&\n \t delete_ref(newrefname, sha1, REF_NODEREF)) {\n \t\tif (errno==EISDIR) {\n \t\t\tif (remove_empty_directories(git_path(\"%s\", newrefname))) {\n-- \n2.0.0.582.ge25c160\n"},{"id":"243545","messageId":"1402093758-3162-36-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 35/40] refs.c: move the check for valid refname to lock_ref_sha1_basic","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:29:13Z","receivedAt":"2014-06-06T22:29:13Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Move the check for check_refname_format from lock_any_ref_for_update\nto lock_ref_sha1_basic. At some later stage we will get rid of\nlock_any_ref_for_update completely.\n\nIf lock_ref_sha1_basic fails the check_refname_format test, set errno to\nEINVAL before returning NULL. This to guarantee that we will not return an\nerror without updating errno.\n\nThis leaves lock_any_ref_for_updates as a no-op wrapper which could be removed.\nBut this wrapper is also called from an external caller and we will soon\nmake changes to the signature to lock_ref_sha1_basic that we do not want to\nexpose to that caller.\n\nThis changes semantics for lock_ref_sha1_basic slightly. With this change\nit is no longer possible to open a ref that has a badly name which breaks\nany codepaths that tries to open and repair badly named refs. The normal refs\nAPI should not allow neither creating nor accessing refs with invalid names.\nIf we need such recovery code we could add it as an option to git fsck and have\ngit fsck be the only sanctioned way of bypassing the normal API and checks.\n\nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 8848dbf..7da5357 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2058,6 +2058,11 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \tint missing = 0;\n \tint attempts_remaining = 3;\n \n+\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n+\t\terrno = EINVAL;\n+\t\treturn NULL;\n+\t}\n+\n \tlock = xcalloc(1, sizeof(struct ref_lock));\n \tlock->lock_fd = -1;\n \n@@ -2149,8 +2154,6 @@ struct ref_lock *lock_any_ref_for_update(const char *refname,\n \t\t\t\t\t const unsigned char *old_sha1,\n \t\t\t\t\t int flags, int *type_p)\n {\n-\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))\n-\t\treturn NULL;\n \treturn lock_ref_sha1_basic(refname, old_sha1, flags, type_p);\n }\n \n-- \n2.0.0.582.ge25c160\n"},{"id":"243543","messageId":"1402093758-3162-37-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 36/40] refs.c: call lock_ref_sha1_basic directly from commit","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:29:14Z","receivedAt":"2014-06-06T22:29:14Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Skip using the lock_any_ref_for_update wrapper and call lock_ref_sha1_basic\ndirectly from the commit function.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 7da5357..66427f0 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3564,12 +3564,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \tfor (i = 0; i < n; i++) {\n \t\tstruct ref_update *update = updates[i];\n \n-\t\tupdate->lock = lock_any_ref_for_update(update->refname,\n-\t\t\t\t\t\t (update->have_old ?\n-\t\t\t\t\t\t\tupdate->old_sha1 :\n-\t\t\t\t\t\t\tNULL),\n-\t\t\t\t\t\t update->flags,\n-\t\t\t\t\t\t &update->type);\n+\t\tupdate->lock = lock_ref_sha1_basic(update->refname,\n+\t\t\t\t\t\t (update->have_old ?\n+\t\t\t\t\t\t update->old_sha1 :\n+\t\t\t\t\t\t NULL),\n+\t\t\t\t\t\t update->flags,\n+\t\t\t\t\t\t &update->type);\n \t\tif (!update->lock) {\n \t\t\tif (err)\n \t\t\t\tstrbuf_addf(err, \"Cannot lock the ref '%s'.\",\n-- \n2.0.0.582.ge25c160\n"},{"id":"243546","messageId":"1402093758-3162-38-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 37/40] refs.c: pass a skip list to name_conflict_fn","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:29:15Z","receivedAt":"2014-06-06T22:29:15Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Allow passing a list of refs to skip checking to name_conflict_fn.\nThere are some conditions where we want to allow a temporary conflict and skip\nchecking those refs. For example if we have a transaction that\n1, guarantees that m is a packed refs and there is no loose ref for m\n2, the transaction will delete m from the packed ref\n3, the transaction will create conflicting m/m\n\nFor this case we want to be able to lock and create m/m since we know that the\nconflict is only transient. I.e. the conflict will be automatically resolved\nby the transaction when it deletes m.\n\nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 41 ++++++++++++++++++++++++++---------------\n 1 file changed, 26 insertions(+), 15 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 66427f0..a5fd943 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -791,15 +791,18 @@ static int names_conflict(const char *refname1, const char *refname2)\n \n struct name_conflict_cb {\n \tconst char *refname;\n-\tconst char *oldrefname;\n \tconst char *conflicting_refname;\n+\tconst char **skip;\n+\tint skipnum;\n };\n \n static int name_conflict_fn(struct ref_entry *entry, void *cb_data)\n {\n \tstruct name_conflict_cb *data = (struct name_conflict_cb *)cb_data;\n-\tif (data->oldrefname && !strcmp(data->oldrefname, entry->name))\n-\t\treturn 0;\n+\tint i;\n+\tfor (i = 0; i < data->skipnum; i++)\n+\t\tif (!strcmp(entry->name, data->skip[i]))\n+\t\t\treturn 0;\n \tif (names_conflict(data->refname, entry->name)) {\n \t\tdata->conflicting_refname = entry->name;\n \t\treturn 1;\n@@ -812,15 +815,18 @@ static int name_conflict_fn(struct ref_entry *entry, void *cb_data)\n * conflicting with the name of an existing reference in dir. If\n * oldrefname is non-NULL, ignore potential conflicts with oldrefname\n * (e.g., because oldrefname is scheduled for deletion in the same\n- * operation).\n+ * operation). skip contains a list of refs we want to skip checking for\n+ * conflicts with.\n */\n-static int is_refname_available(const char *refname, const char *oldrefname,\n-\t\t\t\tstruct ref_dir *dir)\n+static int is_refname_available(const char *refname,\n+\t\t\t\tstruct ref_dir *dir,\n+\t\t\t\tconst char **skip, int skipnum)\n {\n \tstruct name_conflict_cb data;\n \tdata.refname = refname;\n-\tdata.oldrefname = oldrefname;\n \tdata.conflicting_refname = NULL;\n+\tdata.skip = skip;\n+\tdata.skipnum = skipnum;\n \n \tsort_ref_dir(dir);\n \tif (do_for_each_entry_in_dir(dir, 0, name_conflict_fn, &data)) {\n@@ -2047,7 +2053,8 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)\n \n static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\t\t\t const unsigned char *old_sha1,\n-\t\t\t\t\t int flags, int *type_p)\n+\t\t\t\t\t int flags, int *type_p,\n+\t\t\t\t\t const char **skip, int skipnum)\n {\n \tchar *ref_file;\n \tconst char *orig_refname = refname;\n@@ -2096,7 +2103,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t * name is a proper prefix of our refname.\n \t */\n \tif (missing &&\n-\t !is_refname_available(refname, NULL, get_packed_refs(&ref_cache))) {\n+\t !is_refname_available(refname, get_packed_refs(&ref_cache),\n+\t\t\t\t skip, skipnum)) {\n \t\tlast_errno = ENOTDIR;\n \t\tgoto error_return;\n \t}\n@@ -2154,7 +2162,7 @@ struct ref_lock *lock_any_ref_for_update(const char *refname,\n \t\t\t\t\t const unsigned char *old_sha1,\n \t\t\t\t\t int flags, int *type_p)\n {\n-\treturn lock_ref_sha1_basic(refname, old_sha1, flags, type_p);\n+\treturn lock_ref_sha1_basic(refname, old_sha1, flags, type_p, NULL, 0);\n }\n \n /*\n@@ -2645,10 +2653,12 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \tif (!symref)\n \t\treturn error(\"refname %s not found\", oldrefname);\n \n-\tif (!is_refname_available(newrefname, oldrefname, get_packed_refs(&ref_cache)))\n+\tif (!is_refname_available(newrefname, get_packed_refs(&ref_cache),\n+\t\t\t\t &oldrefname, 1))\n \t\treturn 1;\n \n-\tif (!is_refname_available(newrefname, oldrefname, get_loose_refs(&ref_cache)))\n+\tif (!is_refname_available(newrefname, get_loose_refs(&ref_cache),\n+\t\t\t\t &oldrefname, 1))\n \t\treturn 1;\n \n \tif (log && rename(git_path(\"logs/%s\", oldrefname), git_path(TMP_RENAMED_LOG)))\n@@ -2678,7 +2688,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \n \tlogmoved = log;\n \n-\tlock = lock_ref_sha1_basic(newrefname, NULL, 0, NULL);\n+\tlock = lock_ref_sha1_basic(newrefname, NULL, 0, NULL, NULL, 0);\n \tif (!lock) {\n \t\terror(\"unable to lock %s for update\", newrefname);\n \t\tgoto rollback;\n@@ -2693,7 +2703,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \treturn 0;\n \n rollback:\n-\tlock = lock_ref_sha1_basic(oldrefname, NULL, 0, NULL);\n+\tlock = lock_ref_sha1_basic(oldrefname, NULL, 0, NULL, NULL, 0);\n \tif (!lock) {\n \t\terror(\"unable to lock %s for rollback\", oldrefname);\n \t\tgoto rollbacklog;\n@@ -3569,7 +3579,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t\t\t update->old_sha1 :\n \t\t\t\t\t\t NULL),\n \t\t\t\t\t\t update->flags,\n-\t\t\t\t\t\t &update->type);\n+\t\t\t\t\t\t &update->type,\n+\t\t\t\t\t\t delnames, delnum);\n \t\tif (!update->lock) {\n \t\t\tif (err)\n \t\t\t\tstrbuf_addf(err, \"Cannot lock the ref '%s'.\",\n-- \n2.0.0.582.ge25c160\n"},{"id":"243536","messageId":"1402093758-3162-39-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 38/40] refs.c: propagate any errno==ENOTDIR from _commit back to the callers","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:29:16Z","receivedAt":"2014-06-06T22:29:16Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"In _commit, ENOTDIR can happen in the call to lock_ref_sha1_basic, either when\nwe lstat the new refname and it returns ENOTDIR or if the name checking\nfunction reports that the same type of conflict happened. In both cases it\nmeans that we can not create the new ref due to a name conflict.\n\nFor these cases, save the errno value and abort and make sure that the caller\ncan see errno==ENOTDIR.\n\nAlso start defining specific return codes for _commit, assign -1 as a generic\nerror and -2 as the error that refers to a name conflict. Callers can (and\nshould) use that return value inspecting errno directly.\n\nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 22 +++++++++++++++-------\n refs.h | 6 ++++++\n 2 files changed, 21 insertions(+), 7 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex a5fd943..9a3a501 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3548,7 +3548,7 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t struct strbuf *err)\n {\n-\tint ret = 0, delnum = 0, i;\n+\tint ret = 0, delnum = 0, i, df_conflict = 0;\n \tconst char **delnames;\n \tint n = transaction->nr;\n \tstruct ref_update **updates = transaction->updates;\n@@ -3566,9 +3566,10 @@ 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);\n-\tif (ret)\n+\tif (ref_update_reject_duplicates(updates, n, err)) {\n+\t\tret = -1;\n \t\tgoto cleanup;\n+\t}\n \n \t/* Acquire all locks while verifying old values */\n \tfor (i = 0; i < n; i++) {\n@@ -3582,10 +3583,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t\t\t &update->type,\n \t\t\t\t\t\t delnames, delnum);\n \t\tif (!update->lock) {\n+\t\t\tif (errno == ENOTDIR)\n+\t\t\t\tdf_conflict = 1;\n \t\t\tif (err)\n \t\t\t\tstrbuf_addf(err, \"Cannot lock the ref '%s'.\",\n \t\t\t\t\t update->refname);\n-\t\t\tret = 1;\n+\t\t\tret = -1;\n \t\t\tgoto cleanup;\n \t\t}\n \t}\n@@ -3603,6 +3606,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \n \t\t\t\tif (err)\n \t\t\t\t\tstrbuf_addf(err, str, update->refname);\n+\t\t\t\tret = -1;\n \t\t\t\tgoto cleanup;\n \t\t\t}\n \t\t}\n@@ -3613,14 +3617,16 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\tstruct ref_update *update = updates[i];\n \n \t\tif (update->lock) {\n-\t\t\tret |= delete_ref_loose(update->lock, update->type,\n-\t\t\t\t\t\terr);\n+\t\t\tif (delete_ref_loose(update->lock, update->type, err))\n+\t\t\t\tret = -1;\n+\n \t\t\tif (!(update->flags & REF_ISPRUNING))\n \t\t\t\tdelnames[delnum++] = update->lock->ref_name;\n \t\t}\n \t}\n \n-\tret |= repack_without_refs(delnames, delnum, err);\n+\tif (repack_without_refs(delnames, delnum, err))\n+\t\tret = -1;\n \tfor (i = 0; i < delnum; i++)\n \t\tunlink_or_warn(git_path(\"logs/%s\", delnames[i]));\n \tclear_loose_ref_cache(&ref_cache);\n@@ -3633,6 +3639,8 @@ cleanup:\n \t\tif (updates[i]->lock)\n \t\t\tunlock_ref(updates[i]->lock);\n \tfree(delnames);\n+\tif (df_conflict)\n+\t\tret = -2;\n \treturn ret;\n }\n \ndiff --git a/refs.h b/refs.h\nindex 5c6c20f..1583097 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -323,7 +323,13 @@ int 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.\n+ * If the transaction is already in failed state this function will return\n+ * an error.\n+ * Function returns 0 on success, -1 for generic failures and\n+ * UPDATE_REFS_NAME_CONFLICT (-2) if the failure was due to a name\n+ * collision (ENOTDIR).\n */\n+#define UPDATE_REFS_NAME_CONFLICT -2\n int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t struct strbuf *err);\n \n-- \n2.0.0.582.ge25c160\n"},{"id":"243539","messageId":"1402093758-3162-40-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 39/40] fetch.c: change s_update_ref to use a ref transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:29:17Z","receivedAt":"2014-06-06T22:29:17Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change s_update_ref to use a ref transaction for the ref update.\n\nSigned-off-by: Ronnie Sahlberg \n---\n builtin/fetch.c | 33 +++++++++++++++++++++++----------\n 1 file changed, 23 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex faa1233..52f1ebc 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -375,23 +375,36 @@ static int s_update_ref(const char *action,\n {\n \tchar msg[1024];\n \tchar *rla = getenv(\"GIT_REFLOG_ACTION\");\n-\tstatic struct ref_lock *lock;\n+\tstruct ref_transaction *transaction;\n+\tstruct strbuf err = STRBUF_INIT;\n+\tint ret, df_conflict = 0;\n \n \tif (dry_run)\n \t\treturn 0;\n \tif (!rla)\n \t\trla = default_rla.buf;\n \tsnprintf(msg, sizeof(msg), \"%s: %s\", rla, action);\n-\tlock = lock_any_ref_for_update(ref->name,\n-\t\t\t\t check_old ? ref->old_sha1 : NULL,\n-\t\t\t\t 0, NULL);\n-\tif (!lock)\n-\t\treturn errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :\n-\t\t\t\t\t STORE_REF_ERROR_OTHER;\n-\tif (write_ref_sha1(lock, ref->new_sha1, msg) < 0)\n-\t\treturn errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :\n-\t\t\t\t\t STORE_REF_ERROR_OTHER;\n+\n+\ttransaction = ref_transaction_begin(&err);\n+\tif (!transaction ||\n+\t ref_transaction_update(transaction, ref->name, ref->new_sha1,\n+\t\t\t\t ref->old_sha1, 0, check_old, msg, &err))\n+\t\tgoto fail;\n+\n+\tret = ref_transaction_commit(transaction, &err);\n+\tif (ret == UPDATE_REFS_NAME_CONFLICT)\n+\t\tdf_conflict = 1;\n+\tif (ret)\n+\t\tgoto fail;\n+\n+\tref_transaction_free(transaction);\n \treturn 0;\n+fail:\n+\tref_transaction_free(transaction);\n+\terror(\"%s\", err.buf);\n+\tstrbuf_release(&err);\n+\treturn df_conflict ? STORE_REF_ERROR_DF_CONFLICT\n+\t\t\t : STORE_REF_ERROR_OTHER;\n }\n \n #define REFCOL_WIDTH 10\n-- \n2.0.0.582.ge25c160\n"},{"id":"243541","messageId":"1402093758-3162-41-git-send-email-sahlberg@google.com","threadId":"36862","inReplyTo":"1402093758-3162-1-git-send-email-sahlberg@google.com","subject":"[PATCH v14 40/40] refs.c: make write_ref_sha1 static","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-06T22:29:18Z","receivedAt":"2014-06-06T22:29:18Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"No external users call write_ref_sha1 any more so lets declare it static.\n\nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 6 +++++-\n refs.h | 3 ---\n 2 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 9a3a501..c9d2a89 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2634,6 +2634,9 @@ static int rename_tmp_log(const char *newrefname)\n \treturn 0;\n }\n \n+static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,\n+\t\t\t const char *logmsg);\n+\n int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsg)\n {\n \tunsigned char sha1[20], orig_sha1[20];\n@@ -2882,7 +2885,8 @@ static int is_branch(const char *refname)\n \treturn !strcmp(refname, \"HEAD\") || starts_with(refname, \"refs/heads/\");\n }\n \n-int write_ref_sha1(struct ref_lock *lock,\n+/** Writes sha1 into the ref specified by the lock. **/\n+static int write_ref_sha1(struct ref_lock *lock,\n \tconst unsigned char *sha1, const char *logmsg)\n {\n \tstatic char term = '\\n';\ndiff --git a/refs.h b/refs.h\nindex 1583097..8177052 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -195,9 +195,6 @@ extern int commit_ref(struct ref_lock *lock);\n /** Release any lock taken but not written. **/\n extern void unlock_ref(struct ref_lock *lock);\n \n-/** Writes sha1 into the ref specified by the lock. **/\n-extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);\n-\n /** Setup reflog before using. **/\n int log_ref_setup(const char *refname, char *logfile, int bufsize);\n \n-- \n2.0.0.582.ge25c160\n"},{"id":"243768","messageId":"20140610201006.GA5915@google.com","threadId":"36862","inReplyTo":"1402093758-3162-7-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v14 06/40] refs.c: add an err argument to repack_without_refs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-06-10T20:10:06Z","receivedAt":"2014-06-10T20:10:06Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ronnie Sahlberg wrote:\n\n> Update repack_without_refs to take an err argument and update it if there\n> is a failure. Pass the err variable from ref_transaction_commit to this\n> function so that callers can print a meaningful error message if _commit\n> fails due to a problem in repack_without_refs.\n>\n> Add a new function unable_to_lock_message that takes a strbuf argument and\n> fills in the reason for the failure.\n>\n> In commit_packed_refs, make sure that we propagate any errno that\n> commit_lock_file might have set back to our caller.\n>\n> Make sure both commit_packed_refs and lock_file has errno set to a meaningful\n> value on error. Update a whole bunch of other places to keep errno from\n> beeing clobbered.\n\nThis patch is doing several (all nice) things. Are they connected?\nWould splitting some of them out into separate patches make sense or\nwould it be too much fuss to be worth the trouble?\n\n[...]\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -559,6 +559,8 @@ struct lock_file {\n> #define LOCK_DIE_ON_ERROR 1\n> #define LOCK_NODEREF 2\n> extern int unable_to_lock_error(const char *path, int err);\n> +extern void unable_to_lock_message(const char *path, int err,\n> +\t\t\t\t struct strbuf *buf);\n\nIntroducing a new unable_to_lock_message helper, which has nicer\nsemantics than unable_to_lock_error and cleans up lockfile.c a little.\n\n[...]\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -121,7 +121,7 @@ static char *resolve_symlink(char *p, size_t s)\n> \treturn p;\n> }\n> \n> -\n> +/* Make sure errno contains a meaningful value on error */\n> static int lock_file(struct lock_file *lk, const char *path, int flags)\n\nMaking errno when returning from lock_file() meaningful, which should\nfix\n\n * an existing almost-bug in lock_ref_sha1_basic where it assumes\n errno==ENOENT is meaningful and could waste some work on retries\n\n * an existing bug in repack_without_refs where it prints\n strerror(errno) and picks advice based on errno, despite errno\n potentially being zero and potentially having been clobbered by\n that point\n\nTo make sure we don't lose these fixes, it would be helpful to also\nmention that errno is set in the API documentation for the relevant\nfunctions:\n\n * hold_lock_file_for_update (declared in cache.h)\n * lock_packed_refs (declared in refs.h)\n\n[...]\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1333,8 +1333,10 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n\nMaking errno when returning from resolve_ref_unsafe() meaningful,\nwhich should fix\n\n * a bug in lock_ref_sha1_basic, where it assumes EISDIR\n means it failed due to a directory being in the way\n\nTo make sure we don't lose this fix, it would be helpful to change\nMichael's understated comment (why wasn't it marked with NEEDSWORK,\nbtw?)\n\n\t * errno is sometimes set on errors, but not always.\n\nto describe the new guarantee.\n\n[...]\n> @@ -1908,14 +1918,17 @@ static struct ref_lock *verify_lock(struct ref_lock *lock,\n\nMaking errno when returning from verify_lock() meaningful, which\nshould almost but not completely fix\n\n * a bug in \"git fetch\"'s s_update_ref, which trusts the result of an\n errno == ENOTDIR check to detect D/F conflicts\n\nTo make sure we don't lose this fix, it would be helpful to also\nmention that errno is set in the API documentation for the relevant\nfunctions:\n\n * lock_any_ref_for_update (declared in refs.h), after handling the\n check_refname_format case\n * lock_ref_sha1_basic\n\nENOTDIR makes sense as a sign that a file was in the way of a\ndirectory we wanted to create. Should \"git fetch\" also look for\nENOTEMPTY or EEXIST to catch cases where a directory was in the way\nof a file to be created?\n\n> @@ -1928,13 +1941,15 @@ static int remove_empty_directories(const char *file)\n\nMaking errno when returning from remove_empty_directories() more\nobviously meaningful, which should provide some peace of mind for\npeople auditing lock_ref_sha1_basic.\n\n[...]\n> @@ -2203,11 +2218,16 @@ int lock_packed_refs(int flags)\n[...]\n> int commit_packed_refs(void)\n\nMaking errno when returning from commit_packed_refs() meaningful,\nwhich should fix\n\n * a bug in \"git clone\" where it prints strerror(errno) based on\n errno, despite errno possibly being zero and potentially having\n been clobbered by that point\n * the same kind of bug in \"git pack-refs\"\n\nand prepares for repack_without_refs() to get a meaningful\nerror message when commit_packed_refs() fails without falling into\nthe same bug.\n\nTo make sure we don't lose this fix, it would be helpful to also\nmention that errno is set in the API documentation for\ncommit_packed_refs in refs.h.\n\n[...]\n> @@ -2427,12 +2450,12 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)\n> \treturn 0;\n> }\n> \n> -static int repack_without_refs(const char **refnames, int n)\n> +static int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n\nAdding an 'err' argument to repack_without_refs and writing to it on\nerror.\n\n[...]\n> @@ -2723,9 +2755,12 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)\n\nMaking errno when returning from log_ref_setup() meaningful,\npresumably as preparation for making errno from log_ref_write()\nand write_ref_sha1() meaningful.\n\nTo make sure we don't break it by mistake, it would be helpful to also\nmention that errno is set in the API documentation for log_ref_setup\nin refs.h (or to make that function static --- why is it public?).\n\n[...]\n> @@ -2784,8 +2826,19 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,\n\nMaking errno from log_ref_write() meaningful.\n\n[...]\n> @@ -2800,8 +2853,10 @@ int write_ref_sha1(struct ref_lock *lock,\n\nMaking errno from write_ref_sha1() meaningful, which should fix\n\n * a bug in \"git checkout -b\" where it prints strerror(errno)\n despite errno possibly being zero or clobbered\n\n * a bug in \"git fetch\"'s s_update_ref, which trusts the result of an\n errno == ENOTDIR check to detect D/F conflicts\n\nTo make sure we don't lose these fixes, it would be helpful to also\nmention that errno is set in the API documentation for write_ref_sha1\nin refs.h.\n\nSo this could potentially be multiple patches:\n\n 1. Set errno in lock_packed_refs, fixing a bug in repack_without_refs\n 2. Set errno in resolve_ref_unsafe, fixing a longstanding TODO\n 3. Set errno in lock_any_ref_for_update, tightening half of the D/F check\n 4. Set errno in commit_packed_refs, fixing a bug in clone, packed-refs\n 5. Set errno in log_ref_setup, log_ref_write, and write_ref_sha1,\n fixing a bug in 'checkout -b' and the other half of the D/F check\n 6. Introduce unable_to_lock_message helper\n 7. Add an 'err' argument to repack_without_refs and write to it.\n\ndiff --git i/cache.h w/cache.h\nindex 8b12aa8..0b844c3 100644\n--- i/cache.h\n+++ w/cache.h\n@@ -562,8 +562,10 @@ extern int unable_to_lock_error(const char *path, int err);\n extern void unable_to_lock_message(const char *path, int err,\n \t\t\t\t struct strbuf *buf);\n extern NORETURN void unable_to_lock_index_die(const char *path, int err);\n+/* Sets errno on error. */\n extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);\n extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);\n+/* Sets errno on error. */\n extern int commit_lock_file(struct lock_file *);\n extern void update_index_if_able(struct index_state *, struct lock_file *);\n \n@@ -785,6 +787,11 @@ enum sharedrepo {\n \tPERM_EVERYBODY = 0664\n };\n int git_config_perm(const char *var, const char *value);\n+\n+/*\n+ * Widens permissions on 'path' to respect core.sharedrepository.\n+ * Sets errno on error.\n+ */\n int adjust_shared_perm(const char *path);\n \n /*\n@@ -979,7 +986,7 @@ extern int read_ref(const char *refname, unsigned char *sha1);\n * NULL. If more than MAXDEPTH recursive symbolic lookups are needed,\n * give up and return NULL.\n *\n- * errno is sometimes set on errors, but not always.\n+ * On error, sets errno to indicate what went wrong.\n */\n extern const char *resolve_ref_unsafe(const char *ref, unsigned char *sha1, int reading, int *flag);\n extern char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag);\ndiff --git i/dir.c w/dir.c\nindex eb6f581..daab7b0 100644\n--- i/dir.c\n+++ w/dir.c\n@@ -1527,7 +1527,8 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n {\n \tDIR *dir;\n \tstruct dirent *e;\n-\tint ret = 0, original_len = path->len, len, kept_down = 0;\n+\tint ret = 0, save_errno = 0;\n+\tint original_len = path->len, len, kept_down = 0;\n \tint only_empty = (flag & REMOVE_DIR_EMPTY_ONLY);\n \tint keep_toplevel = (flag & REMOVE_DIR_KEEP_TOPLEVEL);\n \tunsigned char submodule_head[20];\n@@ -1582,20 +1583,23 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n \t\t}\n \n \t\t/* path too long, stat fails, or non-directory still exists */\n+\t\tsave_errno = errno;\n \t\tret = -1;\n \t\tbreak;\n \t}\n \tclosedir(dir);\n \n \tstrbuf_setlen(path, original_len);\n-\tif (!ret && !keep_toplevel && !kept_down)\n-\t\tret = (!rmdir(path->buf) || errno == ENOENT) ? 0 : -1;\n-\telse if (kept_up)\n+\tif (!ret && !keep_toplevel && !kept_down) {\n+\t\tif (rmdir(path->buf) && errno != ENOENT)\n+\t\t\treturn -1;\n+\t} else if (kept_up)\n \t\t/*\n \t\t * report the uplevel that it is not an error that we\n \t\t * did not rmdir() our directory.\n \t\t */\n \t\t*kept_up = !ret;\n+\terrno = save_errno;\n \treturn ret;\n }\n \ndiff --git i/dir.h w/dir.h\nindex 55e5345..c44afbb 100644\n--- i/dir.h\n+++ w/dir.h\n@@ -185,6 +185,7 @@ extern int is_empty_dir(const char *dir);\n \n extern void setup_standard_excludes(struct dir_struct *dir);\n \n+/* sets errno on error */\n #define REMOVE_DIR_EMPTY_ONLY 01\n #define REMOVE_DIR_KEEP_NESTED_GIT 02\n #define REMOVE_DIR_KEEP_TOPLEVEL 04\ndiff --git i/refs.c w/refs.c\nindex 7ec1f32..53d8bc7 100644\n--- i/refs.c\n+++ w/refs.c\n@@ -1295,6 +1295,8 @@ static struct ref_entry *get_packed_ref(const char *refname)\n /*\n * A loose ref file doesn't exist; check for a packed ref. The\n * options are forwarded from resolve_safe_unsafe().\n+ *\n+ * Sets errno on error.\n */\n static const char *handle_missing_loose_ref(const char *refname,\n \t\t\t\t\t unsigned char *sha1,\n@@ -1316,6 +1318,7 @@ static const char *handle_missing_loose_ref(const char *refname,\n \t}\n \t/* The reference is not a packed reference, either. */\n \tif (reading) {\n+\t\terrno = ENOENT;\n \t\treturn NULL;\n \t} else {\n \t\thashclr(sha1);\n@@ -1413,7 +1416,7 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\t\tint save_errno = errno;\n \t\t\tclose(fd);\n \t\t\terrno = save_errno;\n- \t\t\treturn NULL;\n+\t\t\treturn NULL;\n \t\t}\n \t\tclose(fd);\n \t\twhile (len && isspace(buffer[len-1]))\n@@ -1949,9 +1952,9 @@ static int remove_empty_directories(const char *file)\n \tresult = remove_dir_recursively(&path, REMOVE_DIR_EMPTY_ONLY);\n \tsave_errno = errno;\n \n-\terrno = save_errno;\n \tstrbuf_release(&path);\n \n+\terrno = save_errno;\n \treturn result;\n }\n \n@@ -2039,6 +2042,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)\n \treturn logs_found;\n }\n \n+/* Sets errno on error. */\n static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\t\t\t const unsigned char *old_sha1,\n \t\t\t\t\t int flags, int *type_p)\n@@ -2152,8 +2156,10 @@ struct ref_lock *lock_any_ref_for_update(const char *refname,\n \t\t\t\t\t const unsigned char *old_sha1,\n \t\t\t\t\t int flags, int *type_p)\n {\n-\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))\n+\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n+\t\terrno = EINVAL;\n \t\treturn NULL;\n+\t}\n \treturn lock_ref_sha1_basic(refname, old_sha1, flags, type_p);\n }\n \n@@ -2218,10 +2224,6 @@ int lock_packed_refs(int flags)\n \treturn 0;\n }\n \n-/*\n- * Commit the packed refs changes.\n- * On error we must make sure that errno contains a meaningful value.\n- */\n int commit_packed_refs(void)\n {\n \tstruct packed_ref_cache *packed_ref_cache =\n@@ -2776,7 +2778,7 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)\n \t\t\t\t logfile);\n \t\t\t\terrno = save_errno;\n \t\t\t\treturn -1;\n- \t\t\t}\n+\t\t\t}\n \t\t\tlogfd = open(logfile, oflags, 0666);\n \t\t}\n \n@@ -2794,6 +2796,7 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)\n \treturn 0;\n }\n \n+/* Sets errno on error. */\n static int log_ref_write(const char *refname, const unsigned char *old_sha1,\n \t\t\t const unsigned char *new_sha1, const char *msg)\n {\n@@ -2824,7 +2827,9 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,\n \t\t committer);\n \tif (msglen)\n \t\tlen += copy_msg(logrec + len - 1, msg) - 1;\n-\twritten = len <= maxlen ? write_in_full(logfd, logrec, len) : -1;\n+\tif (len > maxlen)\n+\t\tdie(\"BUG: log_ref_write buffer not large enough?\");\n+\twritten = write_in_full(logfd, logrec, len);\n \tfree(logrec);\n \tif (written != len) {\n \t\tint save_errno = errno;\n@@ -2867,7 +2872,7 @@ int write_ref_sha1(struct ref_lock *lock,\n \t\t\tlock->ref_name, sha1_to_hex(sha1));\n \t\tunlock_ref(lock);\n \t\terrno = EINVAL;\n- \t\treturn -1;\n+\t\treturn -1;\n \t}\n \tif (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) {\n \t\terror(\"Trying to write non-commit object %s to branch %s\",\n@@ -2889,7 +2894,9 @@ int write_ref_sha1(struct ref_lock *lock,\n \tif (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||\n \t (strcmp(lock->ref_name, lock->orig_ref_name) &&\n \t log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) < 0)) {\n+\t\tint save_errno = errno;\n \t\tunlock_ref(lock);\n+\t\terrno = save_errno;\n \t\treturn -1;\n \t}\n \tif (strcmp(lock->orig_ref_name, \"HEAD\") != 0) {\n@@ -2914,8 +2921,10 @@ int write_ref_sha1(struct ref_lock *lock,\n \t\t\tlog_ref_write(\"HEAD\", lock->old_sha1, sha1, logmsg);\n \t}\n \tif (commit_ref(lock)) {\n+\t\tint save_errno = errno;\n \t\terror(\"Couldn't set %s\", lock->ref_name);\n \t\tunlock_ref(lock);\n+\t\terrno = save_errno;\n \t\treturn -1;\n \t}\n \tunlock_ref(lock);\ndiff --git i/refs.h w/refs.h\nindex 94d4cd4..c734944 100644\n--- i/refs.h\n+++ w/refs.h\n@@ -81,6 +81,7 @@ extern void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refn\n /*\n * Lock the packed-refs file for writing. Flags is passed to\n * hold_lock_file_for_update(). Return 0 on success.\n+ * On error, sets errno to indicate what went wrong.\n */\n extern int lock_packed_refs(int flags);\n \n@@ -96,6 +97,7 @@ extern void add_packed_ref(const char *refname, const unsigned char *sha1);\n * Write the current version of the packed refs cache from memory to\n * disk. The packed-refs file must already be locked for writing (see\n * lock_packed_refs()). Return zero on success.\n+ * On error, sets errno to indicate what went wrong.\n */\n extern int commit_packed_refs(void);\n \n@@ -135,7 +137,10 @@ extern int peel_ref(const char *refname, unsigned char *sha1);\n /** Locks a \"refs/\" ref returning the lock on success and NULL on failure. **/\n extern struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char *old_sha1);\n \n-/** Locks any ref (for 'HEAD' type refs). */\n+/**\n+ * Locks any ref (for 'HEAD' type refs).\n+ * On error, returns NULL and sets errno to describe the error.\n+ */\n #define REF_NODEREF\t0x01\n extern struct ref_lock *lock_any_ref_for_update(const char *refname,\n \t\t\t\t\t\tconst unsigned char *old_sha1,\n@@ -144,16 +149,25 @@ extern struct ref_lock *lock_any_ref_for_update(const char *refname,\n /** Close the file descriptor owned by a lock and return the status */\n extern int close_ref(struct ref_lock *lock);\n \n-/** Close and commit the ref locked by the lock */\n+/**\n+ * Close and commit the ref locked by the lock.\n+ * Sets errno on error.\n+ */\n extern int commit_ref(struct ref_lock *lock);\n \n /** Release any lock taken but not written. **/\n extern void unlock_ref(struct ref_lock *lock);\n \n-/** Writes sha1 into the ref specified by the lock. **/\n+/**\n+ * Writes sha1 into the ref specified by the lock.\n+ * Sets errno on error.\n+ */\n extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);\n \n-/** Setup reflog before using. **/\n+/**\n+ * Setup reflog before using.\n+ * Sets errno on error.\n+ */\n int log_ref_setup(const char *refname, char *logfile, int bufsize);\n \n /** Reads log for the value of ref during at_time. **/\n"},{"id":"243803","messageId":"CAL=YDWm7m8kagvnePKeosAg+ABH3oXFuunw+i8-3Fa-KPsRbzQ@mail.gmail.com","threadId":"36862","inReplyTo":"20140610201006.GA5915@google.com","subject":"Re: [PATCH v14 06/40] refs.c: add an err argument to repack_without_refs","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-10T21:46:07Z","receivedAt":"2014-06-10T21:46:07Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Thanks.\n\nOn Tue, Jun 10, 2014 at 1:10 PM, Jonathan Nieder wrote:\n> Ronnie Sahlberg wrote:\n>\n>> Update repack_without_refs to take an err argument and update it if there\n>> is a failure. Pass the err variable from ref_transaction_commit to this\n>> function so that callers can print a meaningful error message if _commit\n>> fails due to a problem in repack_without_refs.\n>>\n>> Add a new function unable_to_lock_message that takes a strbuf argument and\n>> fills in the reason for the failure.\n>>\n>> In commit_packed_refs, make sure that we propagate any errno that\n>> commit_lock_file might have set back to our caller.\n>>\n>> Make sure both commit_packed_refs and lock_file has errno set to a meaningful\n>> value on error. Update a whole bunch of other places to keep errno from\n>> beeing clobbered.\n>\n> This patch is doing several (all nice) things. Are they connected?\n> Would splitting some of them out into separate patches make sense or\n> would it be too much fuss to be worth the trouble?\n>\n> [...]\n>> --- a/cache.h\n>> +++ b/cache.h\n>> @@ -559,6 +559,8 @@ struct lock_file {\n>> #define LOCK_DIE_ON_ERROR 1\n>> #define LOCK_NODEREF 2\n>> extern int unable_to_lock_error(const char *path, int err);\n>> +extern void unable_to_lock_message(const char *path, int err,\n>> + struct strbuf *buf);\n>\n> Introducing a new unable_to_lock_message helper, which has nicer\n> semantics than unable_to_lock_error and cleans up lockfile.c a little.\n>\n\nDone.\n\n> [...]\n>> --- a/lockfile.c\n>> +++ b/lockfile.c\n>> @@ -121,7 +121,7 @@ static char *resolve_symlink(char *p, size_t s)\n>> return p;\n>> }\n>>\n>> -\n>> +/* Make sure errno contains a meaningful value on error */\n>> static int lock_file(struct lock_file *lk, const char *path, int flags)\n>\n> Making errno when returning from lock_file() meaningful, which should\n> fix\n>\n> * an existing almost-bug in lock_ref_sha1_basic where it assumes\n> errno==ENOENT is meaningful and could waste some work on retries\n>\n> * an existing bug in repack_without_refs where it prints\n> strerror(errno) and picks advice based on errno, despite errno\n> potentially being zero and potentially having been clobbered by\n> that point\n>\n> To make sure we don't lose these fixes, it would be helpful to also\n> mention that errno is set in the API documentation for the relevant\n> functions:\n>\n> * hold_lock_file_for_update (declared in cache.h)\n> * lock_packed_refs (declared in refs.h)\n\nDone.\n\n>\n> [...]\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -1333,8 +1333,10 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n>\n> Making errno when returning from resolve_ref_unsafe() meaningful,\n> which should fix\n>\n> * a bug in lock_ref_sha1_basic, where it assumes EISDIR\n> means it failed due to a directory being in the way\n>\n> To make sure we don't lose this fix, it would be helpful to change\n> Michael's understated comment (why wasn't it marked with NEEDSWORK,\n> btw?)\n>\n> * errno is sometimes set on errors, but not always.\n>\n> to describe the new guarantee.\n\nDone.\n\n>\n> [...]\n>> @@ -1908,14 +1918,17 @@ static struct ref_lock *verify_lock(struct ref_lock *lock,\n>\n> Making errno when returning from verify_lock() meaningful, which\n> should almost but not completely fix\n>\n> * a bug in \"git fetch\"'s s_update_ref, which trusts the result of an\n> errno == ENOTDIR check to detect D/F conflicts\n>\n> To make sure we don't lose this fix, it would be helpful to also\n> mention that errno is set in the API documentation for the relevant\n> functions:\n>\n> * lock_any_ref_for_update (declared in refs.h), after handling the\n> check_refname_format case\n> * lock_ref_sha1_basic\n>\n> ENOTDIR makes sense as a sign that a file was in the way of a\n> directory we wanted to create. Should \"git fetch\" also look for\n> ENOTEMPTY or EEXIST to catch cases where a directory was in the way\n> of a file to be created?\n>\n\nDone.\n\n>> @@ -1928,13 +1941,15 @@ static int remove_empty_directories(const char *file)\n>\n> Making errno when returning from remove_empty_directories() more\n> obviously meaningful, which should provide some peace of mind for\n> people auditing lock_ref_sha1_basic.\n>\n\nDone.\n\n> [...]\n>> @@ -2203,11 +2218,16 @@ int lock_packed_refs(int flags)\n> [...]\n>> int commit_packed_refs(void)\n>\n> Making errno when returning from commit_packed_refs() meaningful,\n> which should fix\n>\n> * a bug in \"git clone\" where it prints strerror(errno) based on\n> errno, despite errno possibly being zero and potentially having\n> been clobbered by that point\n> * the same kind of bug in \"git pack-refs\"\n>\n> and prepares for repack_without_refs() to get a meaningful\n> error message when commit_packed_refs() fails without falling into\n> the same bug.\n>\n> To make sure we don't lose this fix, it would be helpful to also\n> mention that errno is set in the API documentation for\n> commit_packed_refs in refs.h.\n>\n\nDone.\n\n> [...]\n>> @@ -2427,12 +2450,12 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)\n>> return 0;\n>> }\n>>\n>> -static int repack_without_refs(const char **refnames, int n)\n>> +static int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n>\n> Adding an 'err' argument to repack_without_refs and writing to it on\n> error.\n>\n\nDone.\n\n> [...]\n>> @@ -2723,9 +2755,12 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)\n>\n> Making errno when returning from log_ref_setup() meaningful,\n> presumably as preparation for making errno from log_ref_write()\n> and write_ref_sha1() meaningful.\n>\n> To make sure we don't break it by mistake, it would be helpful to also\n> mention that errno is set in the API documentation for log_ref_setup\n> in refs.h (or to make that function static --- why is it public?).\n>\n\nDone.\n\n> [...]\n>> @@ -2784,8 +2826,19 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,\n>\n> Making errno from log_ref_write() meaningful.\n>\n> [...]\n>> @@ -2800,8 +2853,10 @@ int write_ref_sha1(struct ref_lock *lock,\n>\n> Making errno from write_ref_sha1() meaningful, which should fix\n>\n> * a bug in \"git checkout -b\" where it prints strerror(errno)\n> despite errno possibly being zero or clobbered\n>\n> * a bug in \"git fetch\"'s s_update_ref, which trusts the result of an\n> errno == ENOTDIR check to detect D/F conflicts\n>\n> To make sure we don't lose these fixes, it would be helpful to also\n> mention that errno is set in the API documentation for write_ref_sha1\n> in refs.h.\n\nDone.\n\n>\n> So this could potentially be multiple patches:\n>\n> 1. Set errno in lock_packed_refs, fixing a bug in repack_without_refs\n> 2. Set errno in resolve_ref_unsafe, fixing a longstanding TODO\n> 3. Set errno in lock_any_ref_for_update, tightening half of the D/F check\n> 4. Set errno in commit_packed_refs, fixing a bug in clone, packed-refs\n> 5. Set errno in log_ref_setup, log_ref_write, and write_ref_sha1,\n> fixing a bug in 'checkout -b' and the other half of the D/F check\n> 6. Introduce unable_to_lock_message helper\n> 7. Add an 'err' argument to repack_without_refs and write to it.\n\nI split it up into a bunch of patches.\n\n\n>\n> diff --git i/cache.h w/cache.h\n> index 8b12aa8..0b844c3 100644\n> --- i/cache.h\n> +++ w/cache.h\n> @@ -562,8 +562,10 @@ extern int unable_to_lock_error(const char *path, int err);\n> extern void unable_to_lock_message(const char *path, int err,\n> struct strbuf *buf);\n> extern NORETURN void unable_to_lock_index_die(const char *path, int err);\n> +/* Sets errno on error. */\n> extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);\n> extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);\n> +/* Sets errno on error. */\n> extern int commit_lock_file(struct lock_file *);\n> extern void update_index_if_able(struct index_state *, struct lock_file *);\n>\n> @@ -785,6 +787,11 @@ enum sharedrepo {\n> PERM_EVERYBODY = 0664\n> };\n> int git_config_perm(const char *var, const char *value);\n> +\n> +/*\n> + * Widens permissions on 'path' to respect core.sharedrepository.\n> + * Sets errno on error.\n> + */\n> int adjust_shared_perm(const char *path);\n>\n> /*\n> @@ -979,7 +986,7 @@ extern int read_ref(const char *refname, unsigned char *sha1);\n> * NULL. If more than MAXDEPTH recursive symbolic lookups are needed,\n> * give up and return NULL.\n> *\n> - * errno is sometimes set on errors, but not always.\n> + * On error, sets errno to indicate what went wrong.\n> */\n> extern const char *resolve_ref_unsafe(const char *ref, unsigned char *sha1, int reading, int *flag);\n> extern char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag);\n> diff --git i/dir.c w/dir.c\n> index eb6f581..daab7b0 100644\n> --- i/dir.c\n> +++ w/dir.c\n> @@ -1527,7 +1527,8 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n> {\n> DIR *dir;\n> struct dirent *e;\n> - int ret = 0, original_len = path->len, len, kept_down = 0;\n> + int ret = 0, save_errno = 0;\n> + int original_len = path->len, len, kept_down = 0;\n> int only_empty = (flag & REMOVE_DIR_EMPTY_ONLY);\n> int keep_toplevel = (flag & REMOVE_DIR_KEEP_TOPLEVEL);\n> unsigned char submodule_head[20];\n> @@ -1582,20 +1583,23 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n> }\n>\n> /* path too long, stat fails, or non-directory still exists */\n> + save_errno = errno;\n> ret = -1;\n> break;\n> }\n> closedir(dir);\n>\n> strbuf_setlen(path, original_len);\n> - if (!ret && !keep_toplevel && !kept_down)\n> - ret = (!rmdir(path->buf) || errno == ENOENT) ? 0 : -1;\n> - else if (kept_up)\n> + if (!ret && !keep_toplevel && !kept_down) {\n> + if (rmdir(path->buf) && errno != ENOENT)\n> + return -1;\n> + } else if (kept_up)\n> /*\n> * report the uplevel that it is not an error that we\n> * did not rmdir() our directory.\n> */\n> *kept_up = !ret;\n> + errno = save_errno;\n> return ret;\n> }\n>\n> diff --git i/dir.h w/dir.h\n> index 55e5345..c44afbb 100644\n> --- i/dir.h\n> +++ w/dir.h\n> @@ -185,6 +185,7 @@ extern int is_empty_dir(const char *dir);\n>\n> extern void setup_standard_excludes(struct dir_struct *dir);\n>\n> +/* sets errno on error */\n> #define REMOVE_DIR_EMPTY_ONLY 01\n> #define REMOVE_DIR_KEEP_NESTED_GIT 02\n> #define REMOVE_DIR_KEEP_TOPLEVEL 04\n> diff --git i/refs.c w/refs.c\n> index 7ec1f32..53d8bc7 100644\n> --- i/refs.c\n> +++ w/refs.c\n> @@ -1295,6 +1295,8 @@ static struct ref_entry *get_packed_ref(const char *refname)\n> /*\n> * A loose ref file doesn't exist; check for a packed ref. The\n> * options are forwarded from resolve_safe_unsafe().\n> + *\n> + * Sets errno on error.\n> */\n> static const char *handle_missing_loose_ref(const char *refname,\n> unsigned char *sha1,\n> @@ -1316,6 +1318,7 @@ static const char *handle_missing_loose_ref(const char *refname,\n> }\n> /* The reference is not a packed reference, either. */\n> if (reading) {\n> + errno = ENOENT;\n> return NULL;\n> } else {\n> hashclr(sha1);\n> @@ -1413,7 +1416,7 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n> int save_errno = errno;\n> close(fd);\n> errno = save_errno;\n> - return NULL;\n> + return NULL;\n> }\n> close(fd);\n> while (len && isspace(buffer[len-1]))\n> @@ -1949,9 +1952,9 @@ static int remove_empty_directories(const char *file)\n> result = remove_dir_recursively(&path, REMOVE_DIR_EMPTY_ONLY);\n> save_errno = errno;\n>\n> - errno = save_errno;\n> strbuf_release(&path);\n>\n> + errno = save_errno;\n> return result;\n> }\n>\n> @@ -2039,6 +2042,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)\n> return logs_found;\n> }\n>\n> +/* Sets errno on error. */\n> static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n> const unsigned char *old_sha1,\n> int flags, int *type_p)\n> @@ -2152,8 +2156,10 @@ struct ref_lock *lock_any_ref_for_update(const char *refname,\n> const unsigned char *old_sha1,\n> int flags, int *type_p)\n> {\n> - if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))\n> + if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n> + errno = EINVAL;\n> return NULL;\n> + }\n> return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);\n> }\n>\n> @@ -2218,10 +2224,6 @@ int lock_packed_refs(int flags)\n> return 0;\n> }\n>\n> -/*\n> - * Commit the packed refs changes.\n> - * On error we must make sure that errno contains a meaningful value.\n> - */\n> int commit_packed_refs(void)\n> {\n> struct packed_ref_cache *packed_ref_cache =\n> @@ -2776,7 +2778,7 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)\n> logfile);\n> errno = save_errno;\n> return -1;\n> - }\n> + }\n> logfd = open(logfile, oflags, 0666);\n> }\n>\n> @@ -2794,6 +2796,7 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)\n> return 0;\n> }\n>\n> +/* Sets errno on error. */\n> static int log_ref_write(const char *refname, const unsigned char *old_sha1,\n> const unsigned char *new_sha1, const char *msg)\n> {\n> @@ -2824,7 +2827,9 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,\n> committer);\n> if (msglen)\n> len += copy_msg(logrec + len - 1, msg) - 1;\n> - written = len <= maxlen ? write_in_full(logfd, logrec, len) : -1;\n> + if (len > maxlen)\n> + die(\"BUG: log_ref_write buffer not large enough?\");\n> + written = write_in_full(logfd, logrec, len);\n> free(logrec);\n> if (written != len) {\n> int save_errno = errno;\n> @@ -2867,7 +2872,7 @@ int write_ref_sha1(struct ref_lock *lock,\n> lock->ref_name, sha1_to_hex(sha1));\n> unlock_ref(lock);\n> errno = EINVAL;\n> - return -1;\n> + return -1;\n> }\n> if (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) {\n> error(\"Trying to write non-commit object %s to branch %s\",\n> @@ -2889,7 +2894,9 @@ int write_ref_sha1(struct ref_lock *lock,\n> if (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||\n> (strcmp(lock->ref_name, lock->orig_ref_name) &&\n> log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) < 0)) {\n> + int save_errno = errno;\n> unlock_ref(lock);\n> + errno = save_errno;\n> return -1;\n> }\n> if (strcmp(lock->orig_ref_name, \"HEAD\") != 0) {\n> @@ -2914,8 +2921,10 @@ int write_ref_sha1(struct ref_lock *lock,\n> log_ref_write(\"HEAD\", lock->old_sha1, sha1, logmsg);\n> }\n> if (commit_ref(lock)) {\n> + int save_errno = errno;\n> error(\"Couldn't set %s\", lock->ref_name);\n> unlock_ref(lock);\n> + errno = save_errno;\n> return -1;\n> }\n> unlock_ref(lock);\n> diff --git i/refs.h w/refs.h\n> index 94d4cd4..c734944 100644\n> --- i/refs.h\n> +++ w/refs.h\n> @@ -81,6 +81,7 @@ extern void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refn\n> /*\n> * Lock the packed-refs file for writing. Flags is passed to\n> * hold_lock_file_for_update(). Return 0 on success.\n> + * On error, sets errno to indicate what went wrong.\n> */\n> extern int lock_packed_refs(int flags);\n>\n> @@ -96,6 +97,7 @@ extern void add_packed_ref(const char *refname, const unsigned char *sha1);\n> * Write the current version of the packed refs cache from memory to\n> * disk. The packed-refs file must already be locked for writing (see\n> * lock_packed_refs()). Return zero on success.\n> + * On error, sets errno to indicate what went wrong.\n> */\n> extern int commit_packed_refs(void);\n>\n> @@ -135,7 +137,10 @@ extern int peel_ref(const char *refname, unsigned char *sha1);\n> /** Locks a \"refs/\" ref returning the lock on success and NULL on failure. **/\n> extern struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char *old_sha1);\n>\n> -/** Locks any ref (for 'HEAD' type refs). */\n> +/**\n> + * Locks any ref (for 'HEAD' type refs).\n> + * On error, returns NULL and sets errno to describe the error.\n> + */\n> #define REF_NODEREF 0x01\n> extern struct ref_lock *lock_any_ref_for_update(const char *refname,\n> const unsigned char *old_sha1,\n> @@ -144,16 +149,25 @@ extern struct ref_lock *lock_any_ref_for_update(const char *refname,\n> /** Close the file descriptor owned by a lock and return the status */\n> extern int close_ref(struct ref_lock *lock);\n>\n> -/** Close and commit the ref locked by the lock */\n> +/**\n> + * Close and commit the ref locked by the lock.\n> + * Sets errno on error.\n> + */\n> extern int commit_ref(struct ref_lock *lock);\n>\n> /** Release any lock taken but not written. **/\n> extern void unlock_ref(struct ref_lock *lock);\n>\n> -/** Writes sha1 into the ref specified by the lock. **/\n> +/**\n> + * Writes sha1 into the ref specified by the lock.\n> + * Sets errno on error.\n> + */\n> extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);\n>\n> -/** Setup reflog before using. **/\n> +/**\n> + * Setup reflog before using.\n> + * Sets errno on error.\n> + */\n> int log_ref_setup(const char *refname, char *logfile, int bufsize);\n>\n> /** Reads log for the value of ref during at_time. **/\n"},{"id":"243860","messageId":"20140610224937.GA8557@google.com","threadId":"36862","inReplyTo":"1402093758-3162-9-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v14 08/40] refs.c: add an err argument to delete_ref_loose","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-06-10T22:49:53Z","receivedAt":"2014-06-10T22:49:53Z","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> @@ -2510,16 +2510,38 @@ static int repack_without_ref(const char *refname)\n[...]\n> +static int unlink_or_err(const char *file, struct strbuf *err)\n> +{\n> +\tif (err)\n> +\t\treturn add_err_if_unremovable(\"unlink\", file, err,\n> +\t\t\t\t\t unlink(file));\n> +\telse\n> +\t\treturn unlink_or_warn(file);\n> +}\n\nThis has two different return value conventions (my fault, I know): it\nreturns -1 for ENOENT when err == NULL but 0 for ENOENT when not. The\nusual reason to call unlink_or_warn / remove_or_warn is to be silent\nabout ENOENT so it seems simplest to fix them at the same time, as\nbelow. The only callers that could mind:\n\n * unpack-trees.c::unlink_entry, where this would be fixing a minor bug\n * builtin/apply.c::remove_file, likewise\n * delete_ref_loose, which checks errno so this should have no effect there\n\n refs.c | 2 +-\n wrapper.c | 14 ++++++--------\n 2 files changed, 7 insertions(+), 9 deletions(-)\n\ndiff --git i/refs.c w/refs.c\nindex dd498cf..e32aa97 100644\n--- i/refs.c\n+++ w/refs.c\n@@ -2541,7 +2541,7 @@ static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)\n \t\tlock->lk->filename[i] = 0;\n \t\tres = unlink_or_err(lock->lk->filename, err);\n \t\tlock->lk->filename[i] = '.';\n-\t\tif (res && errno != ENOENT)\n+\t\tif (res)\n \t\t\treturn 1;\n \t}\n \treturn 0;\ndiff --git i/wrapper.c w/wrapper.c\nindex bc1bfb8..c9605cd 100644\n--- i/wrapper.c\n+++ w/wrapper.c\n@@ -429,14 +429,12 @@ int xmkstemp_mode(char *template, int mode)\n \n static int warn_if_unremovable(const char *op, const char *file, int rc)\n {\n-\tif (rc < 0) {\n-\t\tint err = errno;\n-\t\tif (ENOENT != err) {\n-\t\t\twarning(\"unable to %s %s: %s\",\n-\t\t\t\top, file, strerror(errno));\n-\t\t\terrno = err;\n-\t\t}\n-\t}\n+\tint err;\n+\tif (!rc || errno == ENOENT)\n+\t\treturn 0;\n+\terr = errno;\n+\twarning(\"unable to %s %s: %s\", op, file, strerror(errno));\n+\terrno = err;\n \treturn rc;\n }\n \n"},{"id":"243861","messageId":"20140610225327.GB8557@google.com","threadId":"36862","inReplyTo":"1402093758-3162-17-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v14 16/40] refs.c: add transaction.status and track OPEN/CLOSED/ERROR","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-06-10T22:53:27Z","receivedAt":"2014-06-10T22:53:27Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ronnie Sahlberg wrote:\n\n> Signed-off-by: Ronnie Sahlberg \n> ---\n> refs.c | 40 +++++++++++++++++++++++++++++++++++++++-\n> 1 file changed, 39 insertions(+), 1 deletion(-)\n\nReviewed-by: Jonathan Nieder \n\nThis makes the API pretty strict, which should make it easier to\nexperiment later if operations on closed transactions turn out to be\nuseful.\n"},{"id":"243862","messageId":"20140610231841.GC8557@google.com","threadId":"36862","inReplyTo":"1402093758-3162-25-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v14 24/40] receive-pack.c: use a reference transaction for updating the refs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-06-10T23:18:41Z","receivedAt":"2014-06-10T23:18:41Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ronnie Sahlberg wrote:\n\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -46,6 +46,7 @@ static void *head_name_to_free;\n> static int sent_capabilities;\n> static int shallow_update;\n> static const char *alt_shallow_file;\n> +static struct string_list error_strings = STRING_LIST_INIT_DUP;\n[...]\n> @@ -576,19 +576,31 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n[...]\n> +\t\ttransaction = ref_transaction_begin(&err);\n> +\t\tif (!transaction ||\n> +\t\t ref_transaction_update(transaction, namespaced_name,\n> +\t\t\t\t\t new_sha1, old_sha1, 0, 1, &err) ||\n> +\t\t ref_transaction_commit(transaction, \"push\", &err)) {\n> +\n> +\t\t\tconst char *str;\n> +\t\t\tstring_list_append(&error_strings, err.buf);\n> +\t\t\tstr = error_strings.items[error_strings.nr - 1].string;\n[...]\n> +\t\t\treturn str;\n[...]\n> @@ -1215,5 +1227,6 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n> \t\tpacket_flush(1);\n> \tsha1_array_clear(&shallow);\n> \tsha1_array_clear(&ref);\n> +\tstring_list_clear(&error_strings, 0);\n> \treturn 0;\n\nI think it's okay to let error strings accumulate like this because\nthere will not be too many of them. Still I wonder, would it work to\nchange the convention to transfer ownership of the string to the caller?\n\nUltimately 'commands' is leaked so we could even avoid the strdups but\nI'd rather leave it possible to clean up.\n\nSomething like this:\n\ndiff --git i/builtin/receive-pack.c w/builtin/receive-pack.c\nindex 13f4a63..d8ab7b2 100644\n--- i/builtin/receive-pack.c\n+++ w/builtin/receive-pack.c\n@@ -46,7 +46,6 @@ static void *head_name_to_free;\n static int sent_capabilities;\n static int shallow_update;\n static const char *alt_shallow_file;\n-static struct string_list error_strings = STRING_LIST_INIT_DUP;\n \n static enum deny_action parse_deny_action(const char *var, const char *value)\n {\n@@ -195,7 +194,7 @@ static void write_head_info(void)\n \n struct command {\n \tstruct command *next;\n-\tconst char *error_string;\n+\tchar *error_string;\n \tunsigned int skip_update:1,\n \t\t did_not_exist:1;\n \tint index;\n@@ -469,7 +468,7 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n \treturn 0;\n }\n \n-static const char *update(struct command *cmd, struct shallow_info *si)\n+static char *update(struct command *cmd, struct shallow_info *si)\n {\n \tconst char *name = cmd->ref_name;\n \tstruct strbuf namespaced_name_buf = STRBUF_INIT;\n@@ -589,12 +588,9 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\t\t\t\t new_sha1, old_sha1, 0, 1, &err) ||\n \t\t ref_transaction_commit(transaction, \"push\", &err)) {\n \n-\t\t\tconst char *str;\n-\t\t\tstring_list_append(&error_strings, err.buf);\n-\t\t\tstr = error_strings.items[error_strings.nr - 1].string;\n-\t\t\tstrbuf_release(&err);\n-\n+\t\t\tchar *str = strbuf_detach(&err, NULL);\n \t\t\tref_transaction_free(transaction);\n+\n \t\t\trp_error(\"%s\", str);\n \t\t\treturn str;\n \t\t}\n@@ -659,6 +655,9 @@ static void check_aliased_update(struct command *cmd, struct string_list *list)\n \tchar cmd_oldh[41], cmd_newh[41], dst_oldh[41], dst_newh[41];\n \tint flag;\n \n+\tif (cmd->error_string)\n+\t\tdie(\"BUG: check_alised_update called with failed cmd\");\n+\n \tstrbuf_addf(&buf, \"%s%s\", get_git_namespace(), cmd->ref_name);\n \tdst_name = resolve_ref_unsafe(buf.buf, sha1, 0, &flag);\n \tstrbuf_release(&buf);\n@@ -670,7 +669,7 @@ static void check_aliased_update(struct command *cmd, struct string_list *list)\n \tif (!dst_name) {\n \t\trp_error(\"refusing update to broken symref '%s'\", cmd->ref_name);\n \t\tcmd->skip_update = 1;\n-\t\tcmd->error_string = \"broken symref\";\n+\t\tcmd->error_string = xstrdup(\"broken symref\");\n \t\treturn;\n \t}\n \n@@ -696,8 +695,9 @@ static void check_aliased_update(struct command *cmd, struct string_list *list)\n \t\t cmd->ref_name, cmd_oldh, cmd_newh,\n \t\t dst_cmd->ref_name, dst_oldh, dst_newh);\n \n-\tcmd->error_string = dst_cmd->error_string =\n-\t\t\"inconsistent aliased update\";\n+\tcmd->error_string = xstrdup(\"inconsistent aliased update\");\n+\tfree(dst_cmd->error_string);\n+\tdst_cmd->error_string = xstrdup(\"inconsistent aliased update\");\n }\n \n static void check_aliased_updates(struct command *commands)\n@@ -745,7 +745,9 @@ static void set_connectivity_errors(struct command *commands,\n \t\tif (!check_everything_connected(command_singleton_iterator,\n \t\t\t\t\t\t0, &singleton))\n \t\t\tcontinue;\n-\t\tcmd->error_string = \"missing necessary objects\";\n+\t\tif (cmd->error_string)\t/* can't happen */\n+\t\t\tcontinue;\n+\t\tcmd->error_string = xstrdup(\"missing necessary objects\");\n \t}\n }\n \n@@ -782,9 +784,9 @@ static void reject_updates_to_hidden(struct command *commands)\n \t\tif (cmd->error_string || !ref_is_hidden(cmd->ref_name))\n \t\t\tcontinue;\n \t\tif (is_null_sha1(cmd->new_sha1))\n-\t\t\tcmd->error_string = \"deny deleting a hidden ref\";\n+\t\t\tcmd->error_string = xstrdup(\"deny deleting a hidden ref\");\n \t\telse\n-\t\t\tcmd->error_string = \"deny updating a hidden ref\";\n+\t\t\tcmd->error_string = xstrdup(\"deny updating a hidden ref\");\n \t}\n }\n \n@@ -798,8 +800,11 @@ static void execute_commands(struct command *commands,\n \tstruct iterate_data data;\n \n \tif (unpacker_error) {\n-\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n-\t\t\tcmd->error_string = \"unpacker error\";\n+\t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n+\t\t\tif (cmd->error_string)\t/* can't happen */\n+\t\t\t\tcontinue;\n+\t\t\tcmd->error_string = xstrdup(\"unpacker error\");\n+\t\t}\n \t\treturn;\n \t}\n \n@@ -812,8 +817,9 @@ static void execute_commands(struct command *commands,\n \n \tif (run_receive_hook(commands, \"pre-receive\", 0)) {\n \t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n-\t\t\tif (!cmd->error_string)\n-\t\t\t\tcmd->error_string = \"pre-receive hook declined\";\n+\t\t\tif (cmd->error_string)\n+\t\t\t\tcontinue;\n+\t\t\tcmd->error_string = xstrdup(\"pre-receive hook declined\");\n \t\t}\n \t\treturn;\n \t}\n@@ -1091,7 +1097,8 @@ static void update_shallow_info(struct command *commands,\n \t\tif (is_null_sha1(cmd->new_sha1))\n \t\t\tcontinue;\n \t\tif (ref_status[cmd->index]) {\n-\t\t\tcmd->error_string = \"shallow update not allowed\";\n+\t\t\tfree(cmd->error_string);\n+\t\t\tcmd->error_string = xstrdup(\"shallow update not allowed\");\n \t\t\tcmd->skip_update = 1;\n \t\t}\n \t}\n@@ -1227,6 +1234,5 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t\tpacket_flush(1);\n \tsha1_array_clear(&shallow);\n \tsha1_array_clear(&ref);\n-\tstring_list_clear(&error_strings, 0);\n \treturn 0;\n }\n"},{"id":"243863","messageId":"20140610232209.GD8557@google.com","threadId":"36862","inReplyTo":"1402093758-3162-26-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v14 25/40] fast-import.c: use a ref transaction when dumping tags","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-06-10T23:22:09Z","receivedAt":"2014-06-10T23:22:09Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ronnie Sahlberg wrote:\n\n> Signed-off-by: Ronnie Sahlberg \n> ---\n> fast-import.c | 29 +++++++++++++++++++++++------\n> 1 file changed, 23 insertions(+), 6 deletions(-)\n\nReviewed-by: Jonathan Nieder \n"},{"id":"243864","messageId":"20140610232308.GE8557@google.com","threadId":"36862","inReplyTo":"1402093758-3162-27-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v14 26/40] walker.c: use ref transaction for ref updates","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-06-10T23:23:08Z","receivedAt":"2014-06-10T23:23:08Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ronnie Sahlberg wrote:\n\n> Signed-off-by: Ronnie Sahlberg \n> ---\n> walker.c | 59 +++++++++++++++++++++++++++++++++++------------------------\n> 1 file changed, 35 insertions(+), 24 deletions(-)\n\nReviewed-by: Jonathan Nieder \n"},{"id":"243866","messageId":"20140610233759.GF8557@google.com","threadId":"36862","inReplyTo":"1402093758-3162-32-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v14 31/40] refs.c: make prune_ref use a transaction to delete the ref","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-06-10T23:37:59Z","receivedAt":"2014-06-10T23:37:59Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ronnie Sahlberg wrote:\n\n> Signed-off-by: Ronnie Sahlberg \n> ---\n> refs.c | 28 +++++++++++++++++++++-------\n> refs.h | 11 ++++++++++-\n> 2 files changed, 31 insertions(+), 8 deletions(-)\n\nReviewed-by: Jonathan Nieder \n"},{"id":"243915","messageId":"CAL=YDWm+L3tRq8CGr_QG_fYY1abTBfTAb4=YuaaEWzvHUeGaHA@mail.gmail.com","threadId":"36862","inReplyTo":"20140610224937.GA8557@google.com","subject":"Re: [PATCH v14 08/40] refs.c: add an err argument to delete_ref_loose","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-11T18:21:27Z","receivedAt":"2014-06-11T18:21:27Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Done.\n\nThanks.\n\nOn Tue, Jun 10, 2014 at 3:49 PM, Jonathan Nieder wrote:\n> Ronnie Sahlberg wrote:\n>\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -2510,16 +2510,38 @@ static int repack_without_ref(const char *refname)\n> [...]\n>> +static int unlink_or_err(const char *file, struct strbuf *err)\n>> +{\n>> + if (err)\n>> + return add_err_if_unremovable(\"unlink\", file, err,\n>> + unlink(file));\n>> + else\n>> + return unlink_or_warn(file);\n>> +}\n>\n> This has two different return value conventions (my fault, I know): it\n> returns -1 for ENOENT when err == NULL but 0 for ENOENT when not. The\n> usual reason to call unlink_or_warn / remove_or_warn is to be silent\n> about ENOENT so it seems simplest to fix them at the same time, as\n> below. The only callers that could mind:\n>\n> * unpack-trees.c::unlink_entry, where this would be fixing a minor bug\n> * builtin/apply.c::remove_file, likewise\n> * delete_ref_loose, which checks errno so this should have no effect there\n>\n> refs.c | 2 +-\n> wrapper.c | 14 ++++++--------\n> 2 files changed, 7 insertions(+), 9 deletions(-)\n>\n> diff --git i/refs.c w/refs.c\n> index dd498cf..e32aa97 100644\n> --- i/refs.c\n> +++ w/refs.c\n> @@ -2541,7 +2541,7 @@ static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)\n> lock->lk->filename[i] = 0;\n> res = unlink_or_err(lock->lk->filename, err);\n> lock->lk->filename[i] = '.';\n> - if (res && errno != ENOENT)\n> + if (res)\n> return 1;\n> }\n> return 0;\n> diff --git i/wrapper.c w/wrapper.c\n> index bc1bfb8..c9605cd 100644\n> --- i/wrapper.c\n> +++ w/wrapper.c\n> @@ -429,14 +429,12 @@ int xmkstemp_mode(char *template, int mode)\n>\n> static int warn_if_unremovable(const char *op, const char *file, int rc)\n> {\n> - if (rc < 0) {\n> - int err = errno;\n> - if (ENOENT != err) {\n> - warning(\"unable to %s %s: %s\",\n> - op, file, strerror(errno));\n> - errno = err;\n> - }\n> - }\n> + int err;\n> + if (!rc || errno == ENOENT)\n> + return 0;\n> + err = errno;\n> + warning(\"unable to %s %s: %s\", op, file, strerror(errno));\n> + errno = err;\n> return rc;\n> }\n>\n"},{"id":"243921","messageId":"CAL=YDWn8gFep1Hhp2gnQsd4PCuRT+vBb6wBcsbL9Sjriyf-ePw@mail.gmail.com","threadId":"36862","inReplyTo":"20140610231841.GC8557@google.com","subject":"Re: [PATCH v14 24/40] receive-pack.c: use a reference transaction for updating the refs","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-11T18:44:41Z","receivedAt":"2014-06-11T18:44:41Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Thanks.\n\nDone.\nI added a function to stop leaking commands too.\n\nOn Tue, Jun 10, 2014 at 4:18 PM, Jonathan Nieder wrote:\n> Ronnie Sahlberg wrote:\n>\n>> --- a/builtin/receive-pack.c\n>> +++ b/builtin/receive-pack.c\n>> @@ -46,6 +46,7 @@ static void *head_name_to_free;\n>> static int sent_capabilities;\n>> static int shallow_update;\n>> static const char *alt_shallow_file;\n>> +static struct string_list error_strings = STRING_LIST_INIT_DUP;\n> [...]\n>> @@ -576,19 +576,31 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n> [...]\n>> + transaction = ref_transaction_begin(&err);\n>> + if (!transaction ||\n>> + ref_transaction_update(transaction, namespaced_name,\n>> + new_sha1, old_sha1, 0, 1, &err) ||\n>> + ref_transaction_commit(transaction, \"push\", &err)) {\n>> +\n>> + const char *str;\n>> + string_list_append(&error_strings, err.buf);\n>> + str = error_strings.items[error_strings.nr - 1].string;\n> [...]\n>> + return str;\n> [...]\n>> @@ -1215,5 +1227,6 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n>> packet_flush(1);\n>> sha1_array_clear(&shallow);\n>> sha1_array_clear(&ref);\n>> + string_list_clear(&error_strings, 0);\n>> return 0;\n>\n> I think it's okay to let error strings accumulate like this because\n> there will not be too many of them. Still I wonder, would it work to\n> change the convention to transfer ownership of the string to the caller?\n>\n> Ultimately 'commands' is leaked so we could even avoid the strdups but\n> I'd rather leave it possible to clean up.\n>\n> Something like this:\n>\n> diff --git i/builtin/receive-pack.c w/builtin/receive-pack.c\n> index 13f4a63..d8ab7b2 100644\n> --- i/builtin/receive-pack.c\n> +++ w/builtin/receive-pack.c\n> @@ -46,7 +46,6 @@ static void *head_name_to_free;\n> static int sent_capabilities;\n> static int shallow_update;\n> static const char *alt_shallow_file;\n> -static struct string_list error_strings = STRING_LIST_INIT_DUP;\n>\n> static enum deny_action parse_deny_action(const char *var, const char *value)\n> {\n> @@ -195,7 +194,7 @@ static void write_head_info(void)\n>\n> struct command {\n> struct command *next;\n> - const char *error_string;\n> + char *error_string;\n> unsigned int skip_update:1,\n> did_not_exist:1;\n> int index;\n> @@ -469,7 +468,7 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n> return 0;\n> }\n>\n> -static const char *update(struct command *cmd, struct shallow_info *si)\n> +static char *update(struct command *cmd, struct shallow_info *si)\n> {\n> const char *name = cmd->ref_name;\n> struct strbuf namespaced_name_buf = STRBUF_INIT;\n> @@ -589,12 +588,9 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n> new_sha1, old_sha1, 0, 1, &err) ||\n> ref_transaction_commit(transaction, \"push\", &err)) {\n>\n> - const char *str;\n> - string_list_append(&error_strings, err.buf);\n> - str = error_strings.items[error_strings.nr - 1].string;\n> - strbuf_release(&err);\n> -\n> + char *str = strbuf_detach(&err, NULL);\n> ref_transaction_free(transaction);\n> +\n> rp_error(\"%s\", str);\n> return str;\n> }\n> @@ -659,6 +655,9 @@ static void check_aliased_update(struct command *cmd, struct string_list *list)\n> char cmd_oldh[41], cmd_newh[41], dst_oldh[41], dst_newh[41];\n> int flag;\n>\n> + if (cmd->error_string)\n> + die(\"BUG: check_alised_update called with failed cmd\");\n> +\n> strbuf_addf(&buf, \"%s%s\", get_git_namespace(), cmd->ref_name);\n> dst_name = resolve_ref_unsafe(buf.buf, sha1, 0, &flag);\n> strbuf_release(&buf);\n> @@ -670,7 +669,7 @@ static void check_aliased_update(struct command *cmd, struct string_list *list)\n> if (!dst_name) {\n> rp_error(\"refusing update to broken symref '%s'\", cmd->ref_name);\n> cmd->skip_update = 1;\n> - cmd->error_string = \"broken symref\";\n> + cmd->error_string = xstrdup(\"broken symref\");\n> return;\n> }\n>\n> @@ -696,8 +695,9 @@ static void check_aliased_update(struct command *cmd, struct string_list *list)\n> cmd->ref_name, cmd_oldh, cmd_newh,\n> dst_cmd->ref_name, dst_oldh, dst_newh);\n>\n> - cmd->error_string = dst_cmd->error_string =\n> - \"inconsistent aliased update\";\n> + cmd->error_string = xstrdup(\"inconsistent aliased update\");\n> + free(dst_cmd->error_string);\n> + dst_cmd->error_string = xstrdup(\"inconsistent aliased update\");\n> }\n>\n> static void check_aliased_updates(struct command *commands)\n> @@ -745,7 +745,9 @@ static void set_connectivity_errors(struct command *commands,\n> if (!check_everything_connected(command_singleton_iterator,\n> 0, &singleton))\n> continue;\n> - cmd->error_string = \"missing necessary objects\";\n> + if (cmd->error_string) /* can't happen */\n> + continue;\n> + cmd->error_string = xstrdup(\"missing necessary objects\");\n> }\n> }\n>\n> @@ -782,9 +784,9 @@ static void reject_updates_to_hidden(struct command *commands)\n> if (cmd->error_string || !ref_is_hidden(cmd->ref_name))\n> continue;\n> if (is_null_sha1(cmd->new_sha1))\n> - cmd->error_string = \"deny deleting a hidden ref\";\n> + cmd->error_string = xstrdup(\"deny deleting a hidden ref\");\n> else\n> - cmd->error_string = \"deny updating a hidden ref\";\n> + cmd->error_string = xstrdup(\"deny updating a hidden ref\");\n> }\n> }\n>\n> @@ -798,8 +800,11 @@ static void execute_commands(struct command *commands,\n> struct iterate_data data;\n>\n> if (unpacker_error) {\n> - for (cmd = commands; cmd; cmd = cmd->next)\n> - cmd->error_string = \"unpacker error\";\n> + for (cmd = commands; cmd; cmd = cmd->next) {\n> + if (cmd->error_string) /* can't happen */\n> + continue;\n> + cmd->error_string = xstrdup(\"unpacker error\");\n> + }\n> return;\n> }\n>\n> @@ -812,8 +817,9 @@ static void execute_commands(struct command *commands,\n>\n> if (run_receive_hook(commands, \"pre-receive\", 0)) {\n> for (cmd = commands; cmd; cmd = cmd->next) {\n> - if (!cmd->error_string)\n> - cmd->error_string = \"pre-receive hook declined\";\n> + if (cmd->error_string)\n> + continue;\n> + cmd->error_string = xstrdup(\"pre-receive hook declined\");\n> }\n> return;\n> }\n> @@ -1091,7 +1097,8 @@ static void update_shallow_info(struct command *commands,\n> if (is_null_sha1(cmd->new_sha1))\n> continue;\n> if (ref_status[cmd->index]) {\n> - cmd->error_string = \"shallow update not allowed\";\n> + free(cmd->error_string);\n> + cmd->error_string = xstrdup(\"shallow update not allowed\");\n> cmd->skip_update = 1;\n> }\n> }\n> @@ -1227,6 +1234,5 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n> packet_flush(1);\n> sha1_array_clear(&shallow);\n> sha1_array_clear(&ref);\n> - string_list_clear(&error_strings, 0);\n> return 0;\n> }\n"}]}