{"thread":{"id":"36766","subject":"[PATCH v11 00/41] Use ref transactions","startedAt":"2014-05-27T20:25:19Z","lastAt":"2014-06-03T21:32:49Z","messageCount":89,"participants":["Ronnie Sahlberg","Jonathan Nieder"],"isPatch":true,"patchVersion":11,"patchTotal":41},"messages":[{"id":"242774","messageId":"1401222360-21175-1-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":null,"subject":"[PATCH v11 00/41] Use ref transactions","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:19Z","receivedAt":"2014-05-27T20:25:19Z","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 11:\n - Updates after JNs review of the series.\n\nRonnie Sahlberg (41):\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.c: log transaction error from the update_ref\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: pack all refs before we start to rename a ref\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 | 25 +--\n builtin/receive-pack.c | 44 ++++--\n builtin/replace.c | 15 +-\n builtin/tag.c | 15 +-\n builtin/update-ref.c | 34 ++--\n cache.h | 2 +\n fast-import.c | 42 +++--\n lockfile.c | 21 +--\n refs.c | 420 ++++++++++++++++++++++++++++++++-----------------\n refs.h | 80 ++++++----\n sequencer.c | 24 ++-\n t/t3200-branch.sh | 6 +-\n walker.c | 55 ++++---\n 15 files changed, 531 insertions(+), 306 deletions(-)\n\n-- \n2.0.0.rc3.474.g0203784\n"},{"id":"242801","messageId":"1401222360-21175-2-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 01/41] refs.c: remove ref_transaction_rollback","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:20Z","receivedAt":"2014-05-27T20:25:20Z","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.rc3.474.g0203784\n"},{"id":"242778","messageId":"1401222360-21175-3-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 02/41] refs.c: ref_transaction_commit should not free the transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:21Z","receivedAt":"2014-05-27T20:25:21Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Signed-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.rc3.474.g0203784\n"},{"id":"242813","messageId":"1401222360-21175-4-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 03/41] refs.c: constify the sha arguments for ref_transaction_create|delete|update","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:22Z","receivedAt":"2014-05-27T20:25:22Z","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.rc3.474.g0203784\n"},{"id":"242775","messageId":"1401222360-21175-5-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 04/41] refs.c: allow passing NULL to ref_transaction_free","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:23Z","receivedAt":"2014-05-27T20:25:23Z","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.rc3.474.g0203784\n"},{"id":"242812","messageId":"1401222360-21175-6-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 05/41] refs.c: add a strbuf argument to ref_transaction_commit for error logging","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:24Z","receivedAt":"2014-05-27T20:25:24Z","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.rc3.474.g0203784\n"},{"id":"242815","messageId":"1401222360-21175-7-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 06/41] refs.c: add an err argument to repack_without_refs","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:25Z","receivedAt":"2014-05-27T20:25:25Z","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_strbuf 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\nSigned-off-by: Ronnie Sahlberg \n---\n cache.h | 2 ++\n lockfile.c | 21 ++++++++++++---------\n refs.c | 25 +++++++++++++++++++------\n 3 files changed, 33 insertions(+), 15 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 8c6cdc2..48548d6 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_strbuf(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..9e04b43 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -157,33 +157,36 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \treturn lk->fd;\n }\n \n-static char *unable_to_lock_message(const char *path, int err)\n+void unable_to_lock_strbuf(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_strbuf(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_strbuf(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..2da62ac 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2208,6 +2208,7 @@ int commit_packed_refs(void)\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 +2218,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 +2431,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 +2448,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_strbuf(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 +2479,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@@ -3481,7 +3494,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.rc3.474.g0203784\n"},{"id":"242814","messageId":"1401222360-21175-8-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 07/41] refs.c: make ref_update_reject_duplicates take a strbuf argument for errors","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:26Z","receivedAt":"2014-05-27T20:25:26Z","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 2da62ac..4c7f4f7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3410,6 +3410,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@@ -3417,6 +3418,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@@ -3447,7 +3451,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.rc3.474.g0203784\n"},{"id":"242802","messageId":"1401222360-21175-9-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 08/41] refs.c: add an err argument to delete_ref_loose","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:27Z","receivedAt":"2014-05-27T20:25:27Z","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 | 39 +++++++++++++++++++++++++++++++++------\n 1 file changed, 33 insertions(+), 6 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 4c7f4f7..891b80c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2491,17 +2491,43 @@ 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 && err != 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}\n+\treturn rc;\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\tif (err)\n+\t\t\t\tstrbuf_addf(err,\n+\t\t\t\t\t \"failed to delete loose ref '%s'\",\n+\t\t\t\t\t lock->lk->filename);\n \t\t\treturn 1;\n+\t\t}\n \t}\n \treturn 0;\n }\n@@ -2514,7 +2540,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@@ -3494,7 +3520,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.rc3.474.g0203784\n"},{"id":"242794","messageId":"1401222360-21175-10-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 09/41] refs.c: make update_ref_write update a strbuf on failure","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:28Z","receivedAt":"2014-05-27T20:25:28Z","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 891b80c..b28f7d4 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3301,10 +3301,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@@ -3425,7 +3428,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@@ -3507,7 +3510,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.rc3.474.g0203784\n"},{"id":"242811","messageId":"1401222360-21175-11-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 10/41] update-ref.c: log transaction error from the update_ref","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:29Z","receivedAt":"2014-05-27T20:25:29Z","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.rc3.474.g0203784\n"},{"id":"242804","messageId":"1401222360-21175-12-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 11/41] refs.c: remove the onerr argument to ref_transaction_commit","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:30Z","receivedAt":"2014-05-27T20:25:30Z","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 b28f7d4..00b6e9a 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3439,8 +3439,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@@ -3450,22 +3449,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@@ -3480,7 +3470,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@@ -3492,7 +3482,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@@ -3510,7 +3501,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.rc3.474.g0203784\n"},{"id":"242800","messageId":"1401222360-21175-13-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 12/41] refs.c: change ref_transaction_update() to do error checking and return status","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:31Z","receivedAt":"2014-05-27T20:25:31Z","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 00b6e9a..79a4477 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3376,19 +3376,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.rc3.474.g0203784\n"},{"id":"242797","messageId":"1401222360-21175-14-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 13/41] refs.c: change ref_transaction_create to do error checking and return status","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:32Z","receivedAt":"2014-05-27T20:25:32Z","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\nSigned-off-by: Ronnie Sahlberg \n---\n builtin/update-ref.c | 4 +++-\n refs.c | 18 ++++++++++++------\n refs.h | 25 ++++++++++++++++++-------\n 3 files changed, 33 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 79a4477..709758d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3397,18 +3397,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..22b8cc3 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -215,6 +215,15 @@ enum action_on_err {\n };\n \n /*\n+ * Transaction functions that take an err argument will append an error\n+ * string to this buffer if there was a failure.\n+ * This string is not cleared on each call and may contain an aggregate of\n+ * errors from several previous calls.\n+ * If the caller needs a guarantee that the buffer will only contain the\n+ * current or most recent error it must call strbuf_reset before calling\n+ * the transaction function.\n+ */\n+/*\n * Begin a reference transaction. The reference transaction must\n * be freed by calling ref_transaction_free().\n */\n@@ -236,7 +245,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 +259,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 +283,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.rc3.474.g0203784\n"},{"id":"242803","messageId":"1401222360-21175-15-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 14/41] refs.c: update ref_transaction_delete to check for error and return status","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:33Z","receivedAt":"2014-05-27T20:25:33Z","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\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 709758d..beddce0 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3417,19 +3417,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 22b8cc3..e0ab989 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -273,11 +273,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.rc3.474.g0203784\n"},{"id":"242799","messageId":"1401222360-21175-16-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 15/41] refs.c: make ref_transaction_begin take an err argument","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:34Z","receivedAt":"2014-05-27T20:25:34Z","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 beddce0..1c660e0 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3345,7 +3345,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 e0ab989..6c830f2 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -227,7 +227,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.rc3.474.g0203784\n"},{"id":"242807","messageId":"1401222360-21175-17-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 16/41] refs.c: add transaction.status and track OPEN/CLOSED/ERROR","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:35Z","receivedAt":"2014-05-27T20:25:35Z","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 | 42 +++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 41 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 1c660e0..9992da4 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3335,6 +3335,27 @@ 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. No further updates are possible\n+ * and any calls except free will return an error.\n+ * An ERRORed transaction can not be committed.\n+ * An ERRORed transaction can be rolled back and discarded by calling\n+ * by calling 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@@ -3343,6 +3364,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@@ -3385,6 +3408,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\treturn -1;\n+\n \tif (have_old && !old_sha1)\n \t\tdie(\"BUG: have_old is true but old_sha1 is NULL\");\n \n@@ -3405,6 +3431,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\treturn -1;\n+\n \tif (!new_sha1 || is_null_sha1(new_sha1))\n \t\tdie(\"BUG: create ref with null new_sha1\");\n \n@@ -3425,6 +3454,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\treturn -1;\n+\n \tif (have_old && !old_sha1)\n \t\tdie(\"BUG: have_old is true but old_sha1 is NULL\");\n \n@@ -3480,8 +3512,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\treturn transaction->status;\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@@ -3544,6 +3581,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.rc3.474.g0203784\n"},{"id":"242805","messageId":"1401222360-21175-18-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 17/41] tag.c: use ref transactions when doing updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:36Z","receivedAt":"2014-05-27T20:25:36Z","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.rc3.474.g0203784\n"},{"id":"242796","messageId":"1401222360-21175-19-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 18/41] replace.c: use the ref transaction functions for updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:37Z","receivedAt":"2014-05-27T20:25:37Z","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 3da1bae..f24d814 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -133,7 +133,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 \tif (snprintf(ref, sizeof(ref),\n \t\t \"refs/replace/%s\",\n@@ -156,12 +157,14 @@ static int replace_object_sha1(const char *object_ref,\n \telse if (!force)\n \t\tdie(\"replace ref '%s' already exists\", ref);\n \n-\tlock = lock_any_ref_for_update(ref, prev, 0, NULL);\n-\tif (!lock)\n-\t\tdie(\"%s: cannot lock the ref\", ref);\n-\tif (write_ref_sha1(lock, repl, NULL) < 0)\n-\t\tdie(\"%s: cannot update the ref\", ref);\n+\ttransaction = ref_transaction_begin(&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.rc3.474.g0203784\n"},{"id":"242808","messageId":"1401222360-21175-20-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 19/41] commit.c: use ref transactions for updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:38Z","receivedAt":"2014-05-27T20:25:38Z","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 d28505a..7db194f 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1581,11 +1581,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@@ -1707,16 +1708,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@@ -1725,10 +1716,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.rc3.474.g0203784\n"},{"id":"242798","messageId":"1401222360-21175-21-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 20/41] sequencer.c: use ref transactions for all ref updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:39Z","receivedAt":"2014-05-27T20:25:39Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change to use ref transactions for all updates to refs.\n\nSigned-off-by: Ronnie Sahlberg \n---\n sequencer.c | 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.rc3.474.g0203784\n"},{"id":"242810","messageId":"1401222360-21175-22-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 21/41] fast-import.c: change update_branch to use ref transactions","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:40Z","receivedAt":"2014-05-27T20:25:40Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change update_branch() to use ref transactions for updates.\n\nSigned-off-by: Ronnie Sahlberg \n---\n fast-import.c | 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.rc3.474.g0203784\n"},{"id":"242809","messageId":"1401222360-21175-23-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 22/41] branch.c: use ref transaction for all ref updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:41Z","receivedAt":"2014-05-27T20:25:41Z","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.rc3.474.g0203784\n"},{"id":"242792","messageId":"1401222360-21175-24-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 23/41] refs.c: change update_ref to use a transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:42Z","receivedAt":"2014-05-27T20:25:42Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change the update_ref helper function to use a ref transaction internally.\n\nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 25 +++++++++++++++++++++----\n 1 file changed, 21 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 9992da4..fd8f3cf 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3474,11 +3474,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) && !(t = NULL))) {\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.rc3.474.g0203784\n"},{"id":"242795","messageId":"1401222360-21175-25-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 24/41] receive-pack.c: use a reference transaction for updating the refs","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:43Z","receivedAt":"2014-05-27T20:25:43Z","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 | 43 ++++++++++++++++++++++++++++++++++---------\n 1 file changed, 34 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex c323081..c88dc03 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -46,6 +46,8 @@ static void *head_name_to_free;\n static int sent_capabilities;\n static int shallow_update;\n static const char *alt_shallow_file;\n+static int num_strings;\n+static const char **error_strings;\n \n static enum deny_action parse_deny_action(const char *var, const char *value)\n {\n@@ -468,6 +470,13 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n \treturn 0;\n }\n \n+static const char *add_error_string(const char *str)\n+{\n+\terror_strings = xrealloc(error_strings,\n+\t\t\t\t sizeof(*error_strings) * ++num_strings);\n+\treturn error_strings[num_strings - 1] = xstrdup(str);\n+}\n+\n static const char *update(struct command *cmd, struct shallow_info *si)\n {\n \tconst char *name = cmd->ref_name;\n@@ -475,7 +484,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 +584,27 @@ 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+\t\t\tconst char *str = add_error_string(err.buf);\n+\t\t\tref_transaction_free(transaction);\n+\t\t\tstrbuf_release(&err);\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@@ -1120,6 +1136,14 @@ static int delete_only(struct command *commands)\n \treturn 1;\n }\n \n+static void free_all_strings(void)\n+{\n+\tint i;\n+\tfor (i = 0; i < num_strings; i++)\n+\t\tfree((void *)error_strings[i]);\n+\tfree(error_strings);\n+}\n+\n int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n {\n \tint advertise_refs = 0;\n@@ -1166,6 +1190,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t\tusage(receive_pack_usage);\n \n \tsetup_path();\n+\tatexit(free_all_strings);\n \n \tif (!enter_repo(dir, 0))\n \t\tdie(\"'%s' does not appear to be a git repository\", dir);\n-- \n2.0.0.rc3.474.g0203784\n"},{"id":"242786","messageId":"1401222360-21175-26-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 25/41] fast-import.c: use a ref transaction when dumping tags","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:44Z","receivedAt":"2014-05-27T20:25:44Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Signed-off-by: Ronnie Sahlberg \n---\n fast-import.c | 17 ++++++++++++-----\n 1 file changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 4a7b196..3db5b3d 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1735,15 +1735,22 @@ static void dump_tags(void)\n {\n \tstatic const char *msg = \"fast-import\";\n \tstruct tag *t;\n-\tstruct ref_lock *lock;\n \tchar ref_name[PATH_MAX];\n+\tstruct strbuf err = STRBUF_INIT;\n+\tstruct ref_transaction *transaction;\n \n+\ttransaction = ref_transaction_begin(&err);\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\tsnprintf(ref_name, PATH_MAX, \"refs/tags/%s\", t->name);\n+\n+\t\tif (ref_transaction_update(transaction, ref_name, t->sha1,\n+\t\t\t\t\t NULL, 0, 0, &err))\n+\t\t\tbreak;\n \t}\n+\tif (ref_transaction_commit(transaction, msg, &err))\n+\t\tfailure |= error(\"%s\", err.buf);\n+\tref_transaction_free(transaction);\n+\tstrbuf_release(&err);\n }\n \n static void dump_marks_helper(FILE *f,\n-- \n2.0.0.rc3.474.g0203784\n"},{"id":"242782","messageId":"1401222360-21175-27-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 26/41] walker.c: use ref transaction for ref updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:45Z","receivedAt":"2014-05-27T20:25:45Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Switch to using ref transactions in walker_fetch(). As part of the refactoring\nto use ref transactions we also fix a potential memory leak where in the\noriginal code if write_ref_sha1() would fail we would end up returning from\nthe function without free()ing the msg string.\n\nThis changes the locking slightly for walker_fetch. Previously the code would\nlock all refs before writing them but now we do not lock the refs until the\ncommit stage. There is thus a very short window where changes could be done\nlocally during the fetch which would be overwritten when the fetch completes\nand commits its transaction. But this window should be reasonably short.\nEven if this race does trigger, since both the old code and the new code\njust overwrites the refs to the new values without checking or comparing\nthem with the previous value, this is not too dissimilar to a similar scenario\nwhere you first do a ref change locally and then later do a fetch that\noverwrites the local change. With this in mind I do not see the change in\nlocking semantics to be critical.\n\nNote that this function is only called when fetching from a remote HTTP\nrepository onto the local (most of the time single-user) repository which\nlikely means that the type of collissions that the previous locking would\nprotect against and cause the fetch to fail for to be even more rare.\n\nSigned-off-by: Ronnie Sahlberg \n---\n walker.c | 56 +++++++++++++++++++++++++++++++++-----------------------\n 1 file changed, 33 insertions(+), 23 deletions(-)\n\ndiff --git a/walker.c b/walker.c\nindex 1dd86b8..51ce1c6 100644\n--- a/walker.c\n+++ b/walker.c\n@@ -251,39 +251,37 @@ 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;\n \tunsigned char *sha1 = xmalloc(targets * 20);\n \tchar *msg;\n-\tint ret;\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\tstrbuf_release(&err);\n+\t\t\treturn -1;\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 +292,31 @@ 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\tbreak;\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\tref_transaction_free(transaction);\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+\tfree(msg);\n+\tstrbuf_release(&err);\n+\tstrbuf_release(&ref_name);\n \n \treturn -1;\n }\n-- \n2.0.0.rc3.474.g0203784\n"},{"id":"242793","messageId":"1401222360-21175-28-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 27/41] refs.c: make lock_ref_sha1 static","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:46Z","receivedAt":"2014-05-27T20:25:46Z","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 fd8f3cf..01d8ac4 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2124,7 +2124,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 6c830f2..1e25e1c 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -132,9 +132,6 @@ extern int ref_exists(const char *);\n */\n extern int peel_ref(const char *refname, unsigned char *sha1);\n \n-/** Locks a \"refs/\" ref returning the lock on success and NULL on failure. **/\n-extern struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char *old_sha1);\n-\n /** Locks any ref (for 'HEAD' type refs). */\n #define REF_NODEREF\t0x01\n extern struct ref_lock *lock_any_ref_for_update(const char *refname,\n-- \n2.0.0.rc3.474.g0203784\n"},{"id":"242788","messageId":"1401222360-21175-29-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 28/41] refs.c: remove the update_ref_lock function","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:47Z","receivedAt":"2014-05-27T20:25:47Z","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 01d8ac4..0f82cdb 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3281,24 +3281,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@@ -3550,12 +3532,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.rc3.474.g0203784\n"},{"id":"242776","messageId":"1401222360-21175-30-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 29/41] refs.c: remove the update_ref_write function","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:48Z","receivedAt":"2014-05-27T20:25:48Z","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\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 0f82cdb..979b79a 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3281,25 +3281,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@@ -3552,14 +3533,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.rc3.474.g0203784\n"},{"id":"242806","messageId":"1401222360-21175-31-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 30/41] refs.c: remove lock_ref_sha1","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:49Z","receivedAt":"2014-05-27T20:25:49Z","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 979b79a..4ca84f7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2124,15 +2124,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@@ -2337,8 +2328,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.rc3.474.g0203784\n"},{"id":"242787","messageId":"1401222360-21175-32-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 31/41] refs.c: make prune_ref use a transaction to delete the ref","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:50Z","receivedAt":"2014-05-27T20:25:50Z","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 | 5 +++++\n 2 files changed, 26 insertions(+), 7 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 4ca84f7..1819434 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@@ -2328,17 +2334,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@@ -3546,9 +3559,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 1e25e1c..39c97f8 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -235,6 +235,11 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);\n */\n \n /*\n+ * ref_transaction_update ref_transaction_create and ref_transaction_delete\n+ * all take a flag argument. Currently the only public flag is REF_NODEREF.\n+ * Flag values >= 0x100 are reserved for internal use.\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 * be deleted. If have_old is true, then old_sha1 holds the value\n-- \n2.0.0.rc3.474.g0203784\n"},{"id":"242783","messageId":"1401222360-21175-33-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 32/41] refs.c: make delete_ref use a transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:51Z","receivedAt":"2014-05-27T20:25:51Z","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\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 1819434..88e7bb6 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2494,11 +2494,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@@ -2542,24 +2537,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.rc3.474.g0203784\n"},{"id":"242789","messageId":"1401222360-21175-34-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 33/41] refs.c: pass the ref log message to _create/delete/update instead of _commit","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:52Z","receivedAt":"2014-05-27T20:25:52Z","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\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 | 5 +++--\n builtin/replace.c | 4 ++--\n builtin/tag.c | 4 ++--\n builtin/update-ref.c | 13 +++++++------\n fast-import.c | 8 ++++----\n refs.c | 36 ++++++++++++++++++++++--------------\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 7db194f..608296c 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1721,8 +1721,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 c88dc03..c91d3d6 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -594,8 +594,9 @@ 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+\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 = add_error_string(err.buf);\n \t\t\tref_transaction_free(transaction);\n \t\t\tstrbuf_release(&err);\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex f24d814..09bfd40 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -160,8 +160,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 3db5b3d..b98091d 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@@ -1744,10 +1744,10 @@ static void dump_tags(void)\n \t\tsnprintf(ref_name, PATH_MAX, \"refs/tags/%s\", t->name);\n \n \t\tif (ref_transaction_update(transaction, ref_name, t->sha1,\n-\t\t\t\t\t NULL, 0, 0, &err))\n+\t\t\t\t\t NULL, 0, 0, msg, &err))\n \t\t\tbreak;\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 \tref_transaction_free(transaction);\n \tstrbuf_release(&err);\ndiff --git a/refs.c b/refs.c\nindex 88e7bb6..e485d2f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2343,8 +2343,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@@ -2543,8 +2543,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@@ -3294,6 +3294,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@@ -3343,9 +3344,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@@ -3366,7 +3368,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@@ -3383,13 +3385,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@@ -3406,13 +3410,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@@ -3430,6 +3436,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@@ -3441,10 +3449,10 @@ int update_ref(const char *action, const char *refname,\n \tstruct strbuf err = STRBUF_INIT;\n \n \tt = ref_transaction_begin(&err);\n-\tif ((!t ||\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) && !(t = NULL))) {\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@@ -3485,7 +3493,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@@ -3534,7 +3542,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 39c97f8..84e12b5 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -253,7 +253,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@@ -268,7 +268,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@@ -282,7 +282,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@@ -291,7 +291,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 51ce1c6..584189f 100644\n--- a/walker.c\n+++ b/walker.c\n@@ -296,13 +296,12 @@ 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\tbreak;\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\tref_transaction_free(transaction);\n \t\t\tgoto rollback_and_fail;\n-- \n2.0.0.rc3.474.g0203784\n"},{"id":"242777","messageId":"1401222360-21175-35-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 34/41] refs.c: pass NULL as *flags to read_ref_full","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:53Z","receivedAt":"2014-05-27T20:25:53Z","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 e485d2f..3cdfc72 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2642,7 +2642,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.rc3.474.g0203784\n"},{"id":"242781","messageId":"1401222360-21175-36-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 35/41] refs.c: pack all refs before we start to rename a ref","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:54Z","receivedAt":"2014-05-27T20:25:54Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"This means that most loose refs will no longer be present after the rename\nwhich triggered a test failure since it assumes the file for an unrelated\nref would still be present after the rename.\n\nWe want to do this to make it easier to handle atomic renames in rename_ref for\nthe case 'git branch -m foo/bar foo'. If we can guarantee that foo/bar does not\nexist as a loose ref we can avoid locking foo/bar.lock during transaction\ncommit and thus make it possible to delete the foo directory and re-create\nit as a file(branch) in a single transaction.\n\nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 3 +++\n t/t3200-branch.sh | 6 +++---\n 2 files changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 3cdfc72..08dde5b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2637,6 +2637,9 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \t\treturn error(\"unable to move logfile logs/%s to \"TMP_RENAMED_LOG\": %s\",\n \t\t\toldrefname, strerror(errno));\n \n+\tif (pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE))\n+\t\treturn error(\"unable to pack refs\");\n+\n \tif (delete_ref(oldrefname, orig_sha1, REF_NODEREF)) {\n \t\terror(\"unable to delete old %s\", oldrefname);\n \t\tgoto rollback;\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex ac31b71..de0c2b9 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -288,9 +288,9 @@ test_expect_success 'deleting a dangling symref' '\n test_expect_success 'renaming a symref is not allowed' '\n \tgit symbolic-ref refs/heads/master2 refs/heads/master &&\n \ttest_must_fail git branch -m master2 master3 &&\n-\tgit symbolic-ref refs/heads/master2 &&\n-\ttest_path_is_file .git/refs/heads/master &&\n-\ttest_path_is_missing .git/refs/heads/master3\n+\tgit rev-parse --verify refs/heads/master &&\n+\ttest_must_fail git symbolic-ref refs/heads/master3 &&\n+\ttest_must_fail git rev-parse refs/heads/master3\n '\n \n test_expect_success SYMLINKS 'git branch -m u v should fail when the reflog for u is a symlink' '\n-- \n2.0.0.rc3.474.g0203784\n"},{"id":"242785","messageId":"1401222360-21175-37-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 36/41] refs.c: move the check for valid refname to lock_ref_sha1_basic","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:55Z","receivedAt":"2014-05-27T20:25:55Z","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\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\nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 08dde5b..2952871 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2043,6 +2043,9 @@ 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\treturn NULL;\n+\n \tlock = xcalloc(1, sizeof(struct ref_lock));\n \tlock->lock_fd = -1;\n \n@@ -2134,8 +2137,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.rc3.474.g0203784\n"},{"id":"242790","messageId":"1401222360-21175-38-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 37/41] refs.c: call lock_ref_sha1_basic directly from commit","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:56Z","receivedAt":"2014-05-27T20:25:56Z","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 2952871..28138ea 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3525,12 +3525,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.rc3.474.g0203784\n"},{"id":"242791","messageId":"1401222360-21175-39-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 38/41] refs.c: pass a skip list to name_conflict_fn","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:57Z","receivedAt":"2014-05-27T20:25:57Z","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 | 39 +++++++++++++++++++++++++++++----------\n 1 file changed, 29 insertions(+), 10 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 28138ea..d3812b7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -793,11 +793,17 @@ 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+\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 (data->oldrefname && !strcmp(data->oldrefname, entry->name))\n \t\treturn 0;\n \tif (names_conflict(data->refname, entry->name)) {\n@@ -812,15 +818,19 @@ 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+\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@@ -2032,7 +2042,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@@ -2079,7 +2090,9 @@ 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, NULL,\n+\t\t\t\t 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@@ -2137,7 +2150,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@@ -2618,6 +2631,9 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \tint log = !lstat(git_path(\"logs/%s\", oldrefname), &loginfo);\n \tconst char *symref = NULL;\n \n+\tif (!strcmp(oldrefname, newrefname))\n+\t\treturn 0;\n+\n \tif (log && S_ISLNK(loginfo.st_mode))\n \t\treturn error(\"reflog for %s is a symlink\", oldrefname);\n \n@@ -2628,10 +2644,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, oldrefname,\n+\t\t\t\t get_packed_refs(&ref_cache), NULL, 0))\n \t\treturn 1;\n \n-\tif (!is_refname_available(newrefname, oldrefname, get_loose_refs(&ref_cache)))\n+\tif (!is_refname_available(newrefname, oldrefname,\n+\t\t\t\t get_loose_refs(&ref_cache), NULL, 0))\n \t\treturn 1;\n \n \tif (log && rename(git_path(\"logs/%s\", oldrefname), git_path(TMP_RENAMED_LOG)))\n@@ -2664,7 +2682,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@@ -2679,7 +2697,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@@ -3530,7 +3548,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.rc3.474.g0203784\n"},{"id":"242780","messageId":"1401222360-21175-40-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 39/41] refs.c: propagate any errno==ENOTDIR from _commit back to the callers","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:58Z","receivedAt":"2014-05-27T20:25:58Z","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 | 23 ++++++++++++++++-------\n refs.h | 4 ++++\n 2 files changed, 20 insertions(+), 7 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex d3812b7..c108007 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3517,7 +3517,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, save_errno = 0;\n \tconst char **delnames;\n \tint n = transaction->nr;\n \tstruct ref_update **updates = transaction->updates;\n@@ -3535,9 +3535,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@@ -3551,10 +3552,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\tsave_errno = errno;\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@@ -3572,6 +3575,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@@ -3582,14 +3586,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@@ -3602,6 +3608,9 @@ cleanup:\n \t\tif (updates[i]->lock)\n \t\t\tunlock_ref(updates[i]->lock);\n \tfree(delnames);\n+\terrno = save_errno;\n+\tif (save_errno == ENOTDIR)\n+\t\tret = -2;\n \treturn ret;\n }\n \ndiff --git a/refs.h b/refs.h\nindex 84e12b5..2823b72 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -289,6 +289,10 @@ 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 -2 if the\n+ * failure was due to a name collision (ENOTDIR).\n */\n int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t struct strbuf *err);\n-- \n2.0.0.rc3.474.g0203784\n"},{"id":"242784","messageId":"1401222360-21175-41-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 40/41] fetch.c: change s_update_ref to use a ref transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:25:59Z","receivedAt":"2014-05-27T20:25:59Z","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 | 22 ++++++++++++++--------\n 1 file changed, 14 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex faa1233..c46ccd9 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -45,6 +45,7 @@ static struct transport *gsecondary;\n static const char *submodule_prefix = \"\";\n static const char *recurse_submodules_default;\n static int shown_url = 0;\n+static struct strbuf err = STRBUF_INIT;\n \n static int option_parse_recurse_submodules(const struct option *opt,\n \t\t\t\t const char *arg, int unset)\n@@ -375,22 +376,27 @@ static int s_update_ref(const char *action,\n {\n \tchar msg[1024];\n \tchar *rla = getenv(\"GIT_REFLOG_ACTION\");\n-\tstatic struct ref_lock *lock;\n+\tstruct ref_transaction *transaction;\n \n \tif (dry_run)\n \t\treturn 0;\n \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+\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 ref_transaction_commit(transaction, &err)) {\n+\t\tref_transaction_free(transaction);\n+\t\terror(\"%s\", err.buf);\n+\t\tstrbuf_release(&err);\n \t\treturn errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :\n \t\t\t\t\t STORE_REF_ERROR_OTHER;\n+\t}\n+\tstrbuf_release(&err);\n+\tref_transaction_free(transaction);\n \treturn 0;\n }\n \n-- \n2.0.0.rc3.474.g0203784\n"},{"id":"242779","messageId":"1401222360-21175-42-git-send-email-sahlberg@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"[PATCH v11 41/41] refs.c: make write_ref_sha1 static","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-27T20:26:00Z","receivedAt":"2014-05-27T20:26:00Z","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 c108007..2b51547 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2622,6 +2622,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@@ -2855,7 +2858,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 2823b72..47775e8 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -147,9 +147,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.rc3.474.g0203784\n"},{"id":"242825","messageId":"20140527232020.GG12314@google.com","threadId":"36766","inReplyTo":"1401222360-21175-3-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v11 02/41] refs.c: ref_transaction_commit should not free the transaction","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-27T23:20:20Z","receivedAt":"2014-05-27T23:20:20Z","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> builtin/update-ref.c | 1 +\n> refs.c | 1 -\n> refs.h | 5 ++---\n> 3 files changed, 3 insertions(+), 4 deletions(-)\n\nReviewed-by: Jonathan Nieder \n"},{"id":"242827","messageId":"20140528001117.GH12314@google.com","threadId":"36766","inReplyTo":"1401222360-21175-7-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v11 06/41] refs.c: add an err argument to repack_without_refs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-28T00:11:17Z","receivedAt":"2014-05-28T00:11:17Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nComments from http://marc.info/?l=git&m=140079653930751&w=2:\n\nRonnie Sahlberg wrote:\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_strbuf(const char *path, int err,\n> +\t\t\t\t struct strbuf *buf);\n\n\"unable_to_lock_strbuf\" could be called \"unable_to_lock_message\" (which\nwould make its behavior more obvious IMHO).\n\n[...]\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2208,6 +2208,7 @@ int commit_packed_refs(void)\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\nThis is about making errno meaningful when commit_packed_refs returns\nan error. Probably its API documentation should say so to make it\nobvious when people modify it in the future that they should preserve\nthat property or audit callers.\n\n[...]\n> @@ -2444,6 +2448,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_strbuf(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\nVia the new call to unable_to_lock_..., repack_without_refs cares\nabout errno after a failed call to lock_packed_refs. lock_packed_refs\ncan only fail in hold_lock_file_for_update. hold_lock_file_for_update\nis a thin wrapper around lockfile.c::lock_file. lock_file can error\nout because\n\n\tstrlen(path) >= max_path_len\n\tadjust_shared_perm failed [calls error(), clobbers errno]\n\topen failed\n\nSo lock_file needs a similar kind of fix, and it's probably worth\nupdating API documentation for these calls to make it clear that their\nerrno is used (though that's not a new problem since\nrepack_without_refs already called unable_to_lock_error). Could be a\nseparate, earlier patch (or a TODO comment in this patch to be\naddressed with a later patch) since it's fixing an existing bug.\n\nHope that helps,\nJonathan\n"},{"id":"242828","messageId":"20140528002538.GI12314@google.com","threadId":"36766","inReplyTo":"1401222360-21175-9-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v11 08/41] refs.c: add an err argument to delete_ref_loose","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-28T00:25:38Z","receivedAt":"2014-05-28T00:25:38Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nComments from http://marc.info/?l=git&m=140079653930751&w=2:\n\nRonnie Sahlberg wrote:\n\n[...]\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2491,17 +2491,43 @@ 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 && err != 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}\n> +\treturn rc;\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\nThe new unlink_or_err has an odd contract when the err argument is passed.\nOn error:\n\n * if errno != ENOENT, it will append a message to err and return -1 (good)\n * if errno == ENOENT, it will not append a message to err but will\n still return -1.\n\nThis means the caller has to check errno to figure out whether err is\nmeaningful when it returns an error.\n\nPerhaps it should return 0 when errno == ENOENT. After all, in that\ncase the file does not exist any more, which is all we wanted.\n\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\tif (err)\n> +\t\t\t\tstrbuf_addf(err,\n> +\t\t\t\t\t \"failed to delete loose ref '%s'\",\n> +\t\t\t\t\t lock->lk->filename);\n\nOn failure we seem to add our own message to err, too, so the\nresulting message would be something like\n\n\tfatal: unable to unlink .git/refs/heads/master: \\\n\tPermission deniedfailed to delete loose ref '.git/refs/heads/master.lock'\n\nIs the second strbuf_addf a typo?\n\nThanks,\nJonathan\n"},{"id":"242829","messageId":"20140528002738.GJ12314@google.com","threadId":"36766","inReplyTo":"1401222360-21175-11-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v11 10/41] update-ref.c: log transaction error from the update_ref","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-28T00:27:38Z","receivedAt":"2014-05-28T00:27:38Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nComments from http://marc.info/?l=git&m=140079653930751&w=2:\n\nRonnie Sahlberg wrote:\n\n> [Subject: update-ref.c: log transaction error from the update_ref]\n\nThe above description suggests that this is going to add new logging,\nor in other words that update_ref was being silent about transaction\nerrors before.\n\nThe actual intent is for there to be no functional change, right? I'd\nsay something like \"update-ref: use err argument to get error from\nref_transaction_commit\" or something similar to make it clearer that\nthis is just about changing APIs. Or if there's an intended\nfunctional change, then the commit message could say something about\nthat.\n\nThanks,\nJonathan\n"},{"id":"242830","messageId":"20140528004200.GK12314@google.com","threadId":"36766","inReplyTo":"1401222360-21175-14-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v11 13/41] refs.c: change ref_transaction_create to do error checking and return status","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-28T00:42:00Z","receivedAt":"2014-05-28T00:42:00Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nRonnie Sahlberg wrote:\n\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -215,6 +215,15 @@ enum action_on_err {\n> };\n> \n> /*\n> + * Transaction functions that take an err argument will append an error\n> + * string to this buffer if there was a failure.\n> + * This string is not cleared on each call and may contain an aggregate of\n> + * errors from several previous calls.\n> + * If the caller needs a guarantee that the buffer will only contain the\n> + * current or most recent error it must call strbuf_reset before calling\n> + * the transaction function.\n> + */\n> +/*\n\nIf I look at the documentation for ref_transaction_create(), it is not\nobvious I should look up here for how it handles errors. Not sure how\nto fix that --- maybe this should go in a new\nDocumentation/technical/api-ref-transactions.txt file? Or maybe the\ntop of refs.h where struct ref_transaction is declared.\n\nThe content seems odd to me. It says the string will contain an\naggregate of errors from previous calls, but what it doesn't say is\nthat that aggregate is a bunch of error messages juxtaposed without a\nnewline or space between. Is the idea that some callers will want\nthis aggregate?\n\nWouldn't it be clearer to say how the err argument is meant to be used\nfrom the caller's perspective? E.g.,\n\n\tOn error, transaction functions append a message about what\n\twent wrong to the 'err' argument. The message mentions what\n\tref was being updated (if any) when the error occurred so it\n\tcan be passed to 'die' or 'error' as-is:\n\n\t\tif (ref_transaction_update(..., &err)) {\n\t\t\tret = error(\"%s\", err.buf);\n\t\t\tgoto cleanup;\n\t\t}\n\nIf it's expected that the err argument will be used to aggregate\nmultiple messages in some callers then it would be useful to give an\nexample of that, too, but I don't think that's needed.\n\nJonathan\n"},{"id":"242860","messageId":"CAL=YDWnV+nCLdSJ6k38syV8R=zQD_ox1nCLwvbv9UGky=NLdzA@mail.gmail.com","threadId":"36766","inReplyTo":"20140528002538.GI12314@google.com","subject":"Re: [PATCH v11 08/41] refs.c: add an err argument to delete_ref_loose","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-28T14:43:03Z","receivedAt":"2014-05-28T14:43:03Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Tue, May 27, 2014 at 5:25 PM, Jonathan Nieder wrote:\n> Hi,\n>\n> Comments from http://marc.info/?l=git&m=140079653930751&w=2:\n>\n> Ronnie Sahlberg wrote:\n>\n> [...]\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -2491,17 +2491,43 @@ static int repack_without_ref(const char *refname)\n>> return 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>> + struct strbuf *e, int rc)\n>> +{\n>> + int err = errno;\n>> + if (rc < 0 && err != ENOENT) {\n>> + strbuf_addf(e, \"unable to %s %s: %s\",\n>> + op, file, strerror(errno));\n>> + errno = err;\n>> + }\n>> + return rc;\n>> +}\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> The new unlink_or_err has an odd contract when the err argument is passed.\n> On error:\n>\n> * if errno != ENOENT, it will append a message to err and return -1 (good)\n> * if errno == ENOENT, it will not append a message to err but will\n> still return -1.\n>\n> This means the caller has to check errno to figure out whether err is\n> meaningful when it returns an error.\n>\n> Perhaps it should return 0 when errno == ENOENT. After all, in that\n> case the file does not exist any more, which is all we wanted.\n\nGood idea.\nThanks. Done.\n\n>\n>\n>> +\n>> +static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)\n>> {\n>> if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {\n>> /* loose */\n>> - int err, i = strlen(lock->lk->filename) - 5; /* .lock */\n>> + int res, i = strlen(lock->lk->filename) - 5; /* .lock */\n>>\n>> lock->lk->filename[i] = 0;\n>> - err = unlink_or_warn(lock->lk->filename);\n>> + res = unlink_or_err(lock->lk->filename, err);\n>> lock->lk->filename[i] = '.';\n>> - if (err && errno != ENOENT)\n>> + if (res && errno != ENOENT) {\n>> + if (err)\n>> + strbuf_addf(err,\n>> + \"failed to delete loose ref '%s'\",\n>> + lock->lk->filename);\n>\n> On failure we seem to add our own message to err, too, so the\n> resulting message would be something like\n>\n> fatal: unable to unlink .git/refs/heads/master: \\\n> Permission deniedfailed to delete loose ref '.git/refs/heads/master.lock'\n>\n> Is the second strbuf_addf a typo?\n\nYeah, we don't need it.\nRemoved.\n\n>\n> Thanks,\n> Jonathan\n"},{"id":"242861","messageId":"CAL=YDWnvo-oLd80Kgd1qnqUSSSrwNfcq_wHEjLRCa0Jr4XCjwA@mail.gmail.com","threadId":"36766","inReplyTo":"20140528002738.GJ12314@google.com","subject":"Re: [PATCH v11 10/41] update-ref.c: log transaction error from the update_ref","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-28T14:46:34Z","receivedAt":"2014-05-28T14:46:34Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Tue, May 27, 2014 at 5:27 PM, Jonathan Nieder wrote:\n> Hi,\n>\n> Comments from http://marc.info/?l=git&m=140079653930751&w=2:\n>\n> Ronnie Sahlberg wrote:\n>\n>> [Subject: update-ref.c: log transaction error from the update_ref]\n>\n> The above description suggests that this is going to add new logging,\n> or in other words that update_ref was being silent about transaction\n> errors before.\n>\n> The actual intent is for there to be no functional change, right? I'd\n> say something like \"update-ref: use err argument to get error from\n> ref_transaction_commit\" or something similar to make it clearer that\n> this is just about changing APIs. Or if there's an intended\n> functional change, then the commit message could say something about\n> that.\n\nThanks.\nI reworded the commit message.\n\n>\n> Thanks,\n> Jonathan\n"},{"id":"242862","messageId":"CAL=YDWkQhq2oCkyBG0-ojUDwgApYj1qZt1vXa2gnYsJOEbnvxQ@mail.gmail.com","threadId":"36766","inReplyTo":"20140528004200.GK12314@google.com","subject":"Re: [PATCH v11 13/41] refs.c: change ref_transaction_create to do error checking and return status","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-28T14:55:18Z","receivedAt":"2014-05-28T14:55:18Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Tue, May 27, 2014 at 5:42 PM, Jonathan Nieder wrote:\n> Hi,\n>\n> Ronnie Sahlberg wrote:\n>\n>> --- a/refs.h\n>> +++ b/refs.h\n>> @@ -215,6 +215,15 @@ enum action_on_err {\n>> };\n>>\n>> /*\n>> + * Transaction functions that take an err argument will append an error\n>> + * string to this buffer if there was a failure.\n>> + * This string is not cleared on each call and may contain an aggregate of\n>> + * errors from several previous calls.\n>> + * If the caller needs a guarantee that the buffer will only contain the\n>> + * current or most recent error it must call strbuf_reset before calling\n>> + * the transaction function.\n>> + */\n>> +/*\n>\n> If I look at the documentation for ref_transaction_create(), it is not\n> obvious I should look up here for how it handles errors. Not sure how\n> to fix that --- maybe this should go in a new\n> Documentation/technical/api-ref-transactions.txt file? Or maybe the\n> top of refs.h where struct ref_transaction is declared.\n>\n> The content seems odd to me. It says the string will contain an\n> aggregate of errors from previous calls, but what it doesn't say is\n> that that aggregate is a bunch of error messages juxtaposed without a\n> newline or space between. Is the idea that some callers will want\n> this aggregate?\n>\n> Wouldn't it be clearer to say how the err argument is meant to be used\n> from the caller's perspective? E.g.,\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> if (ref_transaction_update(..., &err)) {\n> ret = error(\"%s\", err.buf);\n> goto cleanup;\n> }\n>\n> If it's expected that the err argument will be used to aggregate\n> multiple messages in some callers then it would be useful to give an\n> example of that, too, but I don't think that's needed.\n\nThanks. Updated the comment in refs.h\n\n>\n> Jonathan\n"},{"id":"242864","messageId":"CAL=YDWnSL2vb2NcpPJnfMaR=p4iQhihyhiowwKXuuA-OiUpmfA@mail.gmail.com","threadId":"36766","inReplyTo":"20140528001117.GH12314@google.com","subject":"Re: [PATCH v11 06/41] refs.c: add an err argument to repack_without_refs","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-28T15:31:49Z","receivedAt":"2014-05-28T15:31:49Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Tue, May 27, 2014 at 5:11 PM, Jonathan Nieder wrote:\n> Hi,\n>\n> Comments from http://marc.info/?l=git&m=140079653930751&w=2:\n>\n> Ronnie Sahlberg wrote:\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_strbuf(const char *path, int err,\n>> + struct strbuf *buf);\n>\n> \"unable_to_lock_strbuf\" could be called \"unable_to_lock_message\" (which\n> would make its behavior more obvious IMHO).\n\nRenamed. Thanks.\n\n>\n> [...]\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -2208,6 +2208,7 @@ int commit_packed_refs(void)\n>> struct packed_ref_cache *packed_ref_cache =\n>> get_packed_ref_cache(&ref_cache);\n>> int error = 0;\n>> + int save_errno = 0;\n>\n> This is about making errno meaningful when commit_packed_refs returns\n> an error. Probably its API documentation should say so to make it\n> obvious when people modify it in the future that they should preserve\n> that property or audit callers.\n>\n> [...]\n>> @@ -2444,6 +2448,11 @@ static int repack_without_refs(const char **refnames, int n)\n>> return 0; /* no refname exists in packed refs */\n>>\n>> if (lock_packed_refs(0)) {\n>> + if (err) {\n>> + unable_to_lock_strbuf(git_path(\"packed-refs\"), errno,\n>> + err);\n>> + return -1;\n>> + }\n>> unable_to_lock_error(git_path(\"packed-refs\"), errno);\n>\n> Via the new call to unable_to_lock_..., repack_without_refs cares\n> about errno after a failed call to lock_packed_refs. lock_packed_refs\n> can only fail in hold_lock_file_for_update. hold_lock_file_for_update\n> is a thin wrapper around lockfile.c::lock_file. lock_file can error\n> out because\n>\n> strlen(path) >= max_path_len\n> adjust_shared_perm failed [calls error(), clobbers errno]\n> open failed\n>\n> So lock_file needs a similar kind of fix, and it's probably worth\n> updating API documentation for these calls to make it clear that their\n> errno is used (though that's not a new problem since\n> repack_without_refs already called unable_to_lock_error). Could be a\n> separate, earlier patch (or a TODO comment in this patch to be\n> addressed with a later patch) since it's fixing an existing bug.\n\nI will add it to my todo list.\nI think passing of errno around is too fragile and that we should\navoid ad-hoc save_errno hacks and implement dedicated return codes to\nreplace errno.\nWe should only inspect errno immediately after a syscall has failed.\n\n>\n> Hope that helps,\n> Jonathan\n"},{"id":"242868","messageId":"20140528164950.GL12314@google.com","threadId":"36766","inReplyTo":"CAL=YDWnvo-oLd80Kgd1qnqUSSSrwNfcq_wHEjLRCa0Jr4XCjwA@mail.gmail.com","subject":"Re: [PATCH v11 10/41] update-ref.c: log transaction error from the update_ref","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-28T16:49:50Z","receivedAt":"2014-05-28T16:49:50Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ronnie Sahlberg wrote:\n\n> I reworded the commit message.\n\nThanks much. Looks good.\n"},{"id":"242870","messageId":"20140528170700.GM12314@google.com","threadId":"36766","inReplyTo":"CAL=YDWkQhq2oCkyBG0-ojUDwgApYj1qZt1vXa2gnYsJOEbnvxQ@mail.gmail.com","subject":"Re: [PATCH v11 13/41] refs.c: change ref_transaction_create to do error checking and return status","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-28T17:07:00Z","receivedAt":"2014-05-28T17:07:00Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ronnie Sahlberg wrote:\n\n> Updated the comment in refs.h\n\nThanks.\n\n> +++ b/refs.h\n> @@ -215,6 +215,31 @@ enum action_on_err {\n> };\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> + * if (ref_transaction_update(..., &err)) {\n> + * ret = error(\"%s\", err.buf);\n> + * goto cleanup;\n> + * }\n> + *\n> + * The message is appended to err without first clearing err.\n> + * This allow shte caller to prepare preamble text to the generated\n\ns/allow shte/allows the/\n\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\nNice.\n\n> + *\n> + * If the caller wants err to only contain the message for the current error\n> + * and nothing else caller will need to guarantee that err is empty or\n> + * call strbuf_reset before calling the transaction function.\n\nI don't think this paragraph is needed --- especially with the\nclarification about how to add a preamble, the contract is clear.\n\n> + */\n> +/*\n> * Begin a reference transaction. The reference transaction must\n> * be freed by calling ref_transaction_free().\n> */\n\nNow that the comment is longer, it's harder to miss, but it still is\nin an odd place for someone looking to understand what the 'err'\nargument to ref_transaction_update() means.\n\nHow about this patch for squashing in?\n\ndiff --git i/refs.h w/refs.h\nindex c741a6c..913ca93 100644\n--- i/refs.h\n+++ w/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@@ -215,31 +254,6 @@ enum action_on_err {\n };\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- * if (ref_transaction_update(..., &err)) {\n- * ret = error(\"%s\", err.buf);\n- * goto cleanup;\n- * }\n- *\n- * The message is appended to err without first clearing err.\n- * This allow shte 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- * If the caller wants err to only contain the message for the current error\n- * and nothing else caller will need to guarantee that err is empty or\n- * call strbuf_reset before calling the transaction function.\n- */\n-/*\n * Begin a reference transaction. The reference transaction must\n * be freed by calling ref_transaction_free().\n */\n"},{"id":"242873","messageId":"CAL=YDWnZQajozAcjLi85xKgkRazScv0Q_5XoVvV47u7AumL2gg@mail.gmail.com","threadId":"36766","inReplyTo":"20140528170700.GM12314@google.com","subject":"Re: [PATCH v11 13/41] refs.c: change ref_transaction_create to do error checking and return status","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-28T17:15:22Z","receivedAt":"2014-05-28T17:15:22Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Wed, May 28, 2014 at 10:07 AM, Jonathan Nieder wrote:\n> Ronnie Sahlberg wrote:\n>\n>> Updated the comment in refs.h\n>\n> Thanks.\n>\n>> +++ b/refs.h\n>> @@ -215,6 +215,31 @@ enum action_on_err {\n>> };\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>> + * if (ref_transaction_update(..., &err)) {\n>> + * ret = error(\"%s\", err.buf);\n>> + * goto cleanup;\n>> + * }\n>> + *\n>> + * The message is appended to err without first clearing err.\n>> + * This allow shte caller to prepare preamble text to the generated\n>\n> s/allow shte/allows the/\n>\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> Nice.\n>\n>> + *\n>> + * If the caller wants err to only contain the message for the current error\n>> + * and nothing else caller will need to guarantee that err is empty or\n>> + * call strbuf_reset before calling the transaction function.\n>\n> I don't think this paragraph is needed --- especially with the\n> clarification about how to add a preamble, the contract is clear.\n>\n>> + */\n>> +/*\n>> * Begin a reference transaction. The reference transaction must\n>> * be freed by calling ref_transaction_free().\n>> */\n>\n> Now that the comment is longer, it's harder to miss, but it still is\n> in an odd place for someone looking to understand what the 'err'\n> argument to ref_transaction_update() means.\n>\n> How about this patch for squashing in?\n>\n> diff --git i/refs.h w/refs.h\n> index c741a6c..913ca93 100644\n> --- i/refs.h\n> +++ w/refs.h\n> @@ -10,6 +10,45 @@ struct ref_lock {\n> int 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> @@ -215,31 +254,6 @@ enum action_on_err {\n> };\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> - * if (ref_transaction_update(..., &err)) {\n> - * ret = error(\"%s\", err.buf);\n> - * goto cleanup;\n> - * }\n> - *\n> - * The message is appended to err without first clearing err.\n> - * This allow shte 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> - * If the caller wants err to only contain the message for the current error\n> - * and nothing else caller will need to guarantee that err is empty or\n> - * call strbuf_reset before calling the transaction function.\n> - */\n> -/*\n> * Begin a reference transaction. The reference transaction must\n> * be freed by calling ref_transaction_free().\n> */\n\nDone.\nPlease re-review.\n"},{"id":"242874","messageId":"20140528172507.GO12314@google.com","threadId":"36766","inReplyTo":"CAL=YDWnZQajozAcjLi85xKgkRazScv0Q_5XoVvV47u7AumL2gg@mail.gmail.com","subject":"Re: [PATCH v11 13/41] refs.c: change ref_transaction_create to do error checking and return status","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-28T17:25:07Z","receivedAt":"2014-05-28T17:25:07Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ronnie Sahlberg wrote:\n\n> Please re-review.\n\n06df8942 is\nReviewed-by: Jonathan Nieder \n\nThanks.\n"},{"id":"242878","messageId":"CAL=YDW=-FFB=8u7TxMJf_bxeDF2L_AiqS6ST293ZGc6MO=Az_A@mail.gmail.com","threadId":"36766","inReplyTo":"20140528172507.GO12314@google.com","subject":"Re: [PATCH v11 13/41] refs.c: change ref_transaction_create to do error checking and return status","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-28T18:01:13Z","receivedAt":"2014-05-28T18:01:13Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Thanks\n\nCan you re-review these patches for me :\n\nplease review\n\n67b8fce refs.c: add an err argument to repack_without_refs\n738ac43 refs.c: add an err argument to delete_ref_loose\nb78b0e0 refs.c: update ref_transaction_delete to check for error and\nreturn status\ne558f96 refs.c: add transaction.status and track OPEN/CLOSED/ERROR\n5a7d9bf sequencer.c: use ref transactions for all ref updates\n0ea69df fast-import.c: change update_branch to use ref transactions\n57c92fb refs.c: change update_ref to use a transaction\n\n\n\n\n\nOn Wed, May 28, 2014 at 10:25 AM, Jonathan Nieder wrote:\n> Ronnie Sahlberg wrote:\n>\n>> Please re-review.\n>\n> 06df8942 is\n> Reviewed-by: Jonathan Nieder \n>\n> Thanks.\n"},{"id":"242879","messageId":"20140528180945.GQ12314@google.com","threadId":"36766","inReplyTo":"CAL=YDW=-FFB=8u7TxMJf_bxeDF2L_AiqS6ST293ZGc6MO=Az_A@mail.gmail.com","subject":"Re: [PATCH v11 13/41] refs.c: change ref_transaction_create to do error checking and return status","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-28T18:09:45Z","receivedAt":"2014-05-28T18:09:45Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ronnie Sahlberg wrote:\n\n> Can you re-review these patches for me :\n>\n> please review\n>\n> 67b8fce refs.c: add an err argument to repack_without_refs\n> 738ac43 refs.c: add an err argument to delete_ref_loose\n> b78b0e0 refs.c: update ref_transaction_delete to check for error and\n> return status\n> e558f96 refs.c: add transaction.status and track OPEN/CLOSED/ERROR\n> 5a7d9bf sequencer.c: use ref transactions for all ref updates\n> 0ea69df fast-import.c: change update_branch to use ref transactions\n> 57c92fb refs.c: change update_ref to use a transaction\n\nThe easiest way for me is if you send them to the list so I can\ncomment inline (and others can chime in with other comments, etc).\nThis works like a reroll of an entire series (using format-patch -v if\nyou use format-patch, etc) except that instead of sending as a new\nthread the updated patch is just sent in response to some review\ncomments or the earlier version of that patch.\n\ngit send-email has an --in-reply-to option for this. That involves\ngetting the Message-Id, which can be a little fussy.\n\nOthers who use send-email might know better tricks for this. I never\nended up learning how to use git send-email (and I'm a little scared\nof it) so I just hit \"reply\" in mutt and put the patch there.\n\nThanks and sorry for the fuss,\nJonathan\n"},{"id":"242883","messageId":"20140528183032.GR12314@google.com","threadId":"36766","inReplyTo":"CAL=YDWnSL2vb2NcpPJnfMaR=p4iQhihyhiowwKXuuA-OiUpmfA@mail.gmail.com","subject":"Re: [PATCH v11 06/41] refs.c: add an err argument to repack_without_refs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-28T18:30:32Z","receivedAt":"2014-05-28T18:30:32Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nLooking at 67b8fcee:\n\nRonnie Sahlberg wrote:\n> On Tue, May 27, 2014 at 5:11 PM, Jonathan Nieder wrote:\n>> Ronnie Sahlberg wrote:\n\n>>> --- a/refs.c\n>>> +++ b/refs.c\n>>> @@ -2208,6 +2208,7 @@ int commit_packed_refs(void)\n>>> struct packed_ref_cache *packed_ref_cache =\n>>> get_packed_ref_cache(&ref_cache);\n>>> int error = 0;\n>>> + int save_errno = 0;\n>>\n>> This is about making errno meaningful when commit_packed_refs returns\n>> an error. Probably its API documentation should say so to make it\n>> obvious when people modify it in the future that they should preserve\n>> that property or audit callers.\n\n67b8fcee doesn't address this. While it's often even better to return\nan error message or meaningful error number so the caller doesn't have\nto worry about errno at all, I think being explicit in the comment\nabove a declaration about which functions leave behind a meaningful\nerrno can make error handling saner.\n\nSo I still think that the above would be a good idea.\n\n[...]\n>> [...]\n>>> @@ -2444,6 +2448,11 @@ static int repack_without_refs(const char **refnames, int n)\n>>> return 0; /* no refname exists in packed refs */\n>>>\n>>> if (lock_packed_refs(0)) {\n>>> + if (err) {\n>>> + unable_to_lock_strbuf(git_path(\"packed-refs\"), errno,\n>>> + err);\n>>> + return -1;\n>>> + }\n>>> unable_to_lock_error(git_path(\"packed-refs\"), errno);\n>>\n>> Via the new call to unable_to_lock_..., repack_without_refs cares\n>> about errno after a failed call to lock_packed_refs. lock_packed_refs\n>> can only fail in hold_lock_file_for_update. hold_lock_file_for_update\n>> is a thin wrapper around lockfile.c::lock_file. lock_file can error\n>> out because\n>>\n>> strlen(path) >= max_path_len\n[...]\n>> Could be a\n>> separate, earlier patch (or a TODO comment in this patch to be\n>> addressed with a later patch) since it's fixing an existing bug.\n>\n> I will add it to my todo list.\n\nMy worry with that is that it is too easy to forget that there is a\nproblem at all. That's not *that* bad --- if no one remembers, how\nserious of a problem was it, really?\n\nExcept that it makes reading code difficult. It's easier to read\nsome code that prints strerror(errno) in a place that is known to\nsometimes have errno==0 if there is a comment\n\n\t/*\n\t * NEEDSWORK: Tweak lock_packed_refs to either reliably\n\t * set errno to a sane value on error or to propagate\n\t * back error information another way.\n\t */\n\tperror(...);\n\nOtherwise, the reader has to keep scratching their head, wondering\n\"how did this ever work?\".\n\nThat's why I suggested adding a TODO comment.\n\n> I think passing of errno around is too fragile and that we should\n> avoid ad-hoc save_errno hacks and implement dedicated return codes to\n> replace errno.\n> We should only inspect errno immediately after a syscall has failed.\n\nSure, agreed in principle. ;-)\n\nThanks,\nJonathan\n"},{"id":"242884","messageId":"20140528184234.GS12314@google.com","threadId":"36766","inReplyTo":"1401222360-21175-15-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v11 14/41] refs.c: update ref_transaction_delete to check for error and return status","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-28T18:42:34Z","receivedAt":"2014-05-28T18:42:34Z","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> builtin/update-ref.c | 5 +++--\n> refs.c | 16 +++++++++++-----\n> refs.h | 12 ++++++++----\n> 3 files changed, 22 insertions(+), 11 deletions(-)\n\nReviewed-by: Jonathan Nieder \n\n[...]\n> +++ b/refs.c\n> @@ -3417,19 +3417,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\nCould combine this into the 'if (have_old &&' check.\n\nThanks,\nJonathan\n"},{"id":"242885","messageId":"20140528185123.GT12314@google.com","threadId":"36766","inReplyTo":"1401222360-21175-17-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v11 16/41] refs.c: add transaction.status and track OPEN/CLOSED/ERROR","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-28T18:51:23Z","receivedAt":"2014-05-28T18:51:23Z","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[...]\n> @@ -3385,6 +3408,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\treturn -1;\n\nI still think this is a step in the wrong direction. It means that\ninstead of being able to do\n\n\tif (ref_transaction_update(..., &err))\n\t\tdie(\"%s\", err.buf);\n\nand be certain that err.buf has a meaningful message, now I have to\nworry that if the state were not REF_TRANSACTION_OPEN (e.g., due to\nsomeone else's code forgetting to handle an error) then the result\nwould be a plain\n\n\tfatal:\n\nThe behavior would be much easier to debug if the code were\n\n\tif (transaction->state != REF_TRANSACTION_OPEN)\n\t\tdie(\"BUG: update on transaction that is not open\");\n\nsince then the symptom would be\n\n\tfatal: BUG: update on transaction that is not open\n\nwhich is easier to work with.\n\nIf a non-OPEN state were a normal, recoverable error, then it could\nappend a message to the *err argument. But there's no message that\ncould be put there that would be meaningful to the end-user. So I\nsuspect it's not a normal, recoverable error.\n\nIf we want to collect errors to make _commit() later fail with a\nmessage like '38 refs failed to update' or something (or a\nstring_list, if the API is to change that way in the future), then\n_update() should not fail. It can record information about what is\nwrong with this update in the transaction and succeed so the caller\nknows to keep going and collect other updates before the _commit()\nthat will fail.\n\nOf course it's easily possible I'm missing something. That's just how\nI see it now.\n\nDoes that make sense?\n\nJonathan\n"},{"id":"242888","messageId":"20140528185342.GU12314@google.com","threadId":"36766","inReplyTo":"1401222360-21175-21-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v11 20/41] sequencer.c: use ref transactions for all ref updates","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-28T18:53:42Z","receivedAt":"2014-05-28T18:53:42Z","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> sequencer.c | 24 ++++++++++++++++--------\n> 1 file changed, 16 insertions(+), 8 deletions(-)\n\nReviewed-by: Jonathan Nieder \n"},{"id":"242889","messageId":"20140528185432.GV12314@google.com","threadId":"36766","inReplyTo":"1401222360-21175-22-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v11 21/41] fast-import.c: change update_branch to use ref transactions","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-28T18:54:32Z","receivedAt":"2014-05-28T18:54:32Z","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 | 25 +++++++++++++++----------\n> 1 file changed, 15 insertions(+), 10 deletions(-)\n\nReviewed-by: Jonathan Nieder \n"},{"id":"242894","messageId":"20140528193139.GW12314@google.com","threadId":"36766","inReplyTo":"1401222360-21175-24-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v11 23/41] refs.c: change update_ref to use a transaction","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-28T19:31:39Z","receivedAt":"2014-05-28T19:31:39Z","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> @@ -3474,11 +3474,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\n(style) Extra parens.\n\n> +\t (ref_transaction_commit(t, action, &err) && !(t = NULL))) {\n\nNo need for this assignment-in-if.\n\nWith the following squashed in,\nReviewed-by: Jonathan Nieder \n\ndiff --git i/refs.c w/refs.c\nindex 568b358..fb462a3 100644\n--- i/refs.c\n+++ w/refs.c\n@@ -3474,10 +3474,10 @@ int update_ref(const char *action, const char *refname,\n \tstruct strbuf err = STRBUF_INIT;\n \n \tt = ref_transaction_begin(&err);\n-\tif ((!t ||\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) && !(t = NULL))) {\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"},{"id":"242897","messageId":"20140528194746.GX12314@google.com","threadId":"36766","inReplyTo":"1401222360-21175-26-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v11 25/41] fast-import.c: use a ref transaction when dumping tags","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-28T19:47:46Z","receivedAt":"2014-05-28T19:47:46Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ronnie Sahlberg wrote:\n\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -1735,15 +1735,22 @@ static void dump_tags(void)\n> {\n> \tstatic const char *msg = \"fast-import\";\n> \tstruct tag *t;\n> -\tstruct ref_lock *lock;\n> \tchar ref_name[PATH_MAX];\n> +\tstruct strbuf err = STRBUF_INIT;\n> +\tstruct ref_transaction *transaction;\n> \n> +\ttransaction = ref_transaction_begin(&err);\n> \tfor (t = first_tag; t; t = t->next_tag) {\n> -\t\tsprintf(ref_name, \"tags/%s\", t->name);\n> +\t\tsnprintf(ref_name, PATH_MAX, \"refs/tags/%s\", t->name);\n\nThat ignores the error if the refname happens to go longer than\nPATH_MAX.\n\nThis is the part of fast-import that doesn't need to be super\nfast ;-). (The objects have been written to the pack, and now\nwe just need to write some refs.) Could this use a strbuf? By that,\nI mean something like\n\ndiff --git i/fast-import.c w/fast-import.c\nindex 3db5b3d..d5f6e63 100644\n--- i/fast-import.c\n+++ w/fast-import.c\n@@ -1735,21 +1735,28 @@ static void dump_tags(void)\n {\n \tstatic const char *msg = \"fast-import\";\n \tstruct tag *t;\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+\tstrbuf_addstr(&ref_name, \"refs/tags/\");\n+\n \ttransaction = ref_transaction_begin(&err);\n \tfor (t = first_tag; t; t = t->next_tag) {\n-\t\tsnprintf(ref_name, PATH_MAX, \"refs/tags/%s\", t->name);\n+\t\tstrbuf_setlen(&ref_name, strlen(\"refs/tags/\"));\n+\t\tstrbuf_addstr(&ref_name, t->name);\n \n-\t\tif (ref_transaction_update(transaction, ref_name, t->sha1,\n-\t\t\t\t\t NULL, 0, 0, &err))\n-\t\t\tbreak;\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 done;\n+\t\t}\n \t}\n \tif (ref_transaction_commit(transaction, msg, &err))\n \t\tfailure |= error(\"%s\", err.buf);\n+done:\n \tref_transaction_free(transaction);\n+\tstrbuf_release(&ref_name);\n \tstrbuf_release(&err);\n }\n \n"},{"id":"242898","messageId":"20140528195645.GY12314@google.com","threadId":"36766","inReplyTo":"1401222360-21175-27-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v11 26/41] walker.c: use ref transaction for ref updates","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-28T19:56:46Z","receivedAt":"2014-05-28T19:56:46Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ronnie Sahlberg wrote:\n\n> Switch to using ref transactions in walker_fetch(). As part of the refactoring\n> to use ref transactions we also fix a potential memory leak where in the\n> original code if write_ref_sha1() would fail we would end up returning from\n> the function without free()ing the msg string.\n\nSounds good.\n\n> This changes the locking slightly for walker_fetch. Previously the code would\n> lock all refs before writing them but now we do not lock the refs until the\n> commit stage.\n\nI don't think this actually changes locking in any significant way.\n(Timing changes, but that's to be expected whenever code changes.)\n\nThe old contract was:\n\nYou run git http-fetch with refnames passed with \"-w\". http-fetch\nwill overwrite the refs with their new values.\n\nThe new contract is:\n\nYou run git http-fetch with refnames passed with \"-w\". http-fetch\nwill overwrite the refs with their new values.\n\nWhat changed?\n\n[...]\n> Note that this function is only called when fetching from a remote HTTP\n> repository onto the local (most of the time single-user) repository which\n> likely means that the type of collissions that the previous locking would\n> protect against and cause the fetch to fail for to be even more rare.\n\nMore to the point, while this function is used by 'git remote-http' (and\nhence by \"git fetch https://foo/bar/baz\"), it is only used to fetch\nobjects by that code path. The remote helper machinery handles the ref\nupdates on its own (and passes NULL as the write_ref argument to the\ndumb walker).\n\n[...]\n> Signed-off-by: Ronnie Sahlberg \n> ---\n> walker.c | 56 +++++++++++++++++++++++++++++++++-----------------------\n> 1 file changed, 33 insertions(+), 23 deletions(-)\n\nThe code change looks good from a quick glance.\n\ngcc 4.6.3 doesn't seem to be smart enough to notice that 'transaction'\nis always initialized before it is used, so it (optionally) could be\nworth initializing 'transition = NULL' to work around that.\n\nSo I think this just needs a simpler commit message and then it would be\ngood to go.\n\nJonathan\n"},{"id":"242900","messageId":"20140528203904.GZ12314@google.com","threadId":"36766","inReplyTo":"1401222360-21175-30-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v11 29/41] refs.c: remove the update_ref_write function","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-28T20:39:04Z","receivedAt":"2014-05-28T20:39:04Z","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 | 35 +++++++++--------------------------\n> 1 file changed, 9 insertions(+), 26 deletions(-)\n\nReviewed-by: Jonathan Nieder \n"},{"id":"242901","messageId":"CAL=YDW=XS49hYg2HdQkP5-hX8v4D7dzEH1wP8jSPvMt6PxX-Uw@mail.gmail.com","threadId":"36766","inReplyTo":"20140528195645.GY12314@google.com","subject":"Re: [PATCH v11 26/41] walker.c: use ref transaction for ref updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-28T20:56:33Z","receivedAt":"2014-05-28T20:56:33Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Wed, May 28, 2014 at 12:56 PM, Jonathan Nieder wrote:\n> Ronnie Sahlberg wrote:\n>\n>> Switch to using ref transactions in walker_fetch(). As part of the refactoring\n>> to use ref transactions we also fix a potential memory leak where in the\n>> original code if write_ref_sha1() would fail we would end up returning from\n>> the function without free()ing the msg string.\n>\n> Sounds good.\n>\n>> This changes the locking slightly for walker_fetch. Previously the code would\n>> lock all refs before writing them but now we do not lock the refs until the\n>> commit stage.\n>\n> I don't think this actually changes locking in any significant way.\n> (Timing changes, but that's to be expected whenever code changes.)\n>\n> The old contract was:\n>\n> You run git http-fetch with refnames passed with \"-w\". http-fetch\n> will overwrite the refs with their new values.\n>\n> The new contract is:\n>\n> You run git http-fetch with refnames passed with \"-w\". http-fetch\n> will overwrite the refs with their new values.\n>\n> What changed?\n\nTiming for the locks basically.\nWhich means the outcome for certain races could change slightly.\nBut not important.\n\n>\n> [...]\n>> Note that this function is only called when fetching from a remote HTTP\n>> repository onto the local (most of the time single-user) repository which\n>> likely means that the type of collissions that the previous locking would\n>> protect against and cause the fetch to fail for to be even more rare.\n>\n> More to the point, while this function is used by 'git remote-http' (and\n> hence by \"git fetch https://foo/bar/baz\"), it is only used to fetch\n> objects by that code path. The remote helper machinery handles the ref\n> updates on its own (and passes NULL as the write_ref argument to the\n> dumb walker).\n>\n> [...]\n>> Signed-off-by: Ronnie Sahlberg \n>> ---\n>> walker.c | 56 +++++++++++++++++++++++++++++++++-----------------------\n>> 1 file changed, 33 insertions(+), 23 deletions(-)\n>\n> The code change looks good from a quick glance.\n>\n> gcc 4.6.3 doesn't seem to be smart enough to notice that 'transaction'\n> is always initialized before it is used, so it (optionally) could be\n> worth initializing 'transition = NULL' to work around that.\n>\n> So I think this just needs a simpler commit message and then it would be\n> good to go.\n>\n\nDone, thanks.\n"},{"id":"242904","messageId":"CAL=YDWkCN1ZW48YM3pwVoRLu4-TPwa4cr1m420H+LLx=DBvM=g@mail.gmail.com","threadId":"36766","inReplyTo":"20140528193139.GW12314@google.com","subject":"Re: [PATCH v11 23/41] refs.c: change update_ref to use a transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-28T21:14:35Z","receivedAt":"2014-05-28T21:14:35Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Done.\nThanks.\n\nOn Wed, May 28, 2014 at 12:31 PM, Jonathan Nieder wrote:\n> Ronnie Sahlberg wrote:\n>\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -3474,11 +3474,28 @@ int update_ref(const char *action, const char *refname,\n>> const unsigned char *sha1, const unsigned char *oldval,\n>> int flags, enum action_on_err onerr)\n>> {\n>> - struct ref_lock *lock;\n>> - lock = update_ref_lock(refname, oldval, flags, NULL, onerr);\n>> - if (!lock)\n>> + struct ref_transaction *t;\n>> + struct strbuf err = STRBUF_INIT;\n>> +\n>> + t = ref_transaction_begin(&err);\n>> + if ((!t ||\n>> + ref_transaction_update(t, refname, sha1, oldval, flags,\n>> + !!oldval, &err)) ||\n>\n> (style) Extra parens.\n>\n>> + (ref_transaction_commit(t, action, &err) && !(t = NULL))) {\n>\n> No need for this assignment-in-if.\n>\n> With the following squashed in,\n> Reviewed-by: Jonathan Nieder \n>\n> diff --git i/refs.c w/refs.c\n> index 568b358..fb462a3 100644\n> --- i/refs.c\n> +++ w/refs.c\n> @@ -3474,10 +3474,10 @@ int update_ref(const char *action, const char *refname,\n> struct strbuf err = STRBUF_INIT;\n>\n> t = ref_transaction_begin(&err);\n> - if ((!t ||\n> + if (!t ||\n> ref_transaction_update(t, refname, sha1, oldval, flags,\n> - !!oldval, &err)) ||\n> - (ref_transaction_commit(t, action, &err) && !(t = NULL))) {\n> + !!oldval, &err) ||\n> + ref_transaction_commit(t, action, &err)) {\n> const char *str = \"update_ref failed for ref '%s': %s\";\n>\n> ref_transaction_free(t);\n"},{"id":"242907","messageId":"CAL=YDWnq=MPF3EYZLJJxR8TJLF=QSk5v=OpZC7S97GqL3=3Zbw@mail.gmail.com","threadId":"36766","inReplyTo":"20140528185123.GT12314@google.com","subject":"Re: [PATCH v11 16/41] refs.c: add transaction.status and track OPEN/CLOSED/ERROR","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-28T21:50:34Z","receivedAt":"2014-05-28T21:50:34Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Wed, May 28, 2014 at 11:51 AM, Jonathan Nieder wrote:\n> Ronnie Sahlberg wrote:\n>\n>> --- a/refs.c\n>> +++ b/refs.c\n> [...]\n>> @@ -3385,6 +3408,9 @@ int ref_transaction_update(struct ref_transaction *transaction,\n>> {\n>> struct ref_update *update;\n>>\n>> + if (transaction->state != REF_TRANSACTION_OPEN)\n>> + return -1;\n>\n> I still think this is a step in the wrong direction. It means that\n> instead of being able to do\n>\n> if (ref_transaction_update(..., &err))\n> die(\"%s\", err.buf);\n>\n> and be certain that err.buf has a meaningful message, now I have to\n> worry that if the state were not REF_TRANSACTION_OPEN (e.g., due to\n> someone else's code forgetting to handle an error) then the result\n> would be a plain\n>\n> fatal:\n>\n> The behavior would be much easier to debug if the code were\n>\n> if (transaction->state != REF_TRANSACTION_OPEN)\n> die(\"BUG: update on transaction that is not open\");\n>\n> since then the symptom would be\n>\n> fatal: BUG: update on transaction that is not open\n>\n> which is easier to work with.\n>\n> If a non-OPEN state were a normal, recoverable error, then it could\n> append a message to the *err argument. But there's no message that\n> could be put there that would be meaningful to the end-user. So I\n> suspect it's not a normal, recoverable error.\n>\n> If we want to collect errors to make _commit() later fail with a\n> message like '38 refs failed to update' or something (or a\n> string_list, if the API is to change that way in the future), then\n> _update() should not fail.\n\nAgreed.\nI have removed the if (transaction->state != REF_TRANSACTION_OPEN)\ncheck from _update/_delete/_create and documented this usecase.\n\nThanks.!\n\n It can record information about what is\n> wrong with this update in the transaction and succeed so the caller\n> knows to keep going and collect other updates before the _commit()\n> that will fail.\n>\n> Of course it's easily possible I'm missing something. That's just how\n> I see it now.\n>\n> Does that make sense?\n\nMakes perfect sense.\n"},{"id":"242908","messageId":"20140528215118.GA12314@google.com","threadId":"36766","inReplyTo":"1401222360-21175-32-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v11 31/41] refs.c: make prune_ref use a transaction to delete the ref","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-28T21:51:18Z","receivedAt":"2014-05-28T21:51:18Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ronnie Sahlberg wrote:\n\n> Change prune_ref to delete the ref using a ref transaction. To do this we also\n> need to add a new flag REF_ISPRUNING that will tell the transaction that we\n> do not want to delete this ref from the packed refs.\n\ns/from the packed refs/from packed-refs, nor delete the ref's reflog/\n\n[...]\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -235,6 +235,11 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);\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> * refname, so the caller retains ownership of the parameter. flags\n> * can be REF_NODEREF; it is passed to update_ref_lock().\n> */\n> \n> /*\n> + * ref_transaction_update ref_transaction_create and ref_transaction_delete\n> + * all take a flag argument. Currently the only public flag is REF_NODEREF.\n> + * Flag values >= 0x100 are reserved for internal use.\n> + */\n> +/*\n> * Add a reference update to transaction. new_sha1 is the value that\n\nThe comment right before here already tries to explain the flag argument,\nthough it isn't in an obvious place so it's easy to miss. Maybe the flag\nargument should be explained in the overview documentation for the\nref_transaction API near the top of the file (but I haven't thought that\nthrough, so leaving it alone).\n\nHow about this as a way to make the reserved flag values easier to\nfind when adding new flags?\n\ndiff --git i/refs.h w/refs.h\nindex 25ac4a9..dee7c8f 100644\n--- i/refs.h\n+++ w/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@@ -265,11 +274,6 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);\n */\n \n /*\n- * ref_transaction_update ref_transaction_create and ref_transaction_delete\n- * all take a flag argument. Currently the only public flag is REF_NODEREF.\n- * Flag values >= 0x100 are reserved for internal use.\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 * be deleted. If have_old is true, then old_sha1 holds the value\n"},{"id":"242909","messageId":"CAL=YDWnBu_vWM67TcnRAPJPij9NV_473C1GNEkjBv1gWAm4RNg@mail.gmail.com","threadId":"36766","inReplyTo":"20140528215118.GA12314@google.com","subject":"Re: [PATCH v11 31/41] refs.c: make prune_ref use a transaction to delete the ref","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-28T21:56:15Z","receivedAt":"2014-05-28T21:56:15Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Wed, May 28, 2014 at 2:51 PM, Jonathan Nieder wrote:\n> Ronnie Sahlberg wrote:\n>\n>> Change prune_ref to delete the ref using a ref transaction. To do this we also\n>> need to add a new flag REF_ISPRUNING that will tell the transaction that we\n>> do not want to delete this ref from the packed refs.\n>\n> s/from the packed refs/from packed-refs, nor delete the ref's reflog/\n>\n> [...]\n>> --- a/refs.h\n>> +++ b/refs.h\n>> @@ -235,6 +235,11 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);\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>> * refname, so the caller retains ownership of the parameter. flags\n>> * can be REF_NODEREF; it is passed to update_ref_lock().\n>> */\n>>\n>> /*\n>> + * ref_transaction_update ref_transaction_create and ref_transaction_delete\n>> + * all take a flag argument. Currently the only public flag is REF_NODEREF.\n>> + * Flag values >= 0x100 are reserved for internal use.\n>> + */\n>> +/*\n>> * Add a reference update to transaction. new_sha1 is the value that\n>\n> The comment right before here already tries to explain the flag argument,\n> though it isn't in an obvious place so it's easy to miss. Maybe the flag\n> argument should be explained in the overview documentation for the\n> ref_transaction API near the top of the file (but I haven't thought that\n> through, so leaving it alone).\n>\n> How about this as a way to make the reserved flag values easier to\n> find when adding new flags?\n>\n> diff --git i/refs.h w/refs.h\n> index 25ac4a9..dee7c8f 100644\n> --- i/refs.h\n> +++ w/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 0x01\n> +\n> +/** Locks any ref (for 'HEAD' type refs). */\n> extern struct ref_lock *lock_any_ref_for_update(const char *refname,\n> const unsigned char *old_sha1,\n> int flags, int *type_p);\n> @@ -265,11 +274,6 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);\n> */\n>\n> /*\n> - * ref_transaction_update ref_transaction_create and ref_transaction_delete\n> - * all take a flag argument. Currently the only public flag is REF_NODEREF.\n> - * Flag values >= 0x100 are reserved for internal use.\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> * be deleted. If have_old is true, then old_sha1 holds the value\n\nThanks. Changed.\n"},{"id":"242911","messageId":"CAL=YDWkUhdoJkdg_zaq+p=XRu7H9fqNXDz89uPhbr4equTyVLQ@mail.gmail.com","threadId":"36766","inReplyTo":"20140528194746.GX12314@google.com","subject":"Re: [PATCH v11 25/41] fast-import.c: use a ref transaction when dumping tags","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-28T22:06:26Z","receivedAt":"2014-05-28T22:06:26Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Wed, May 28, 2014 at 12:47 PM, Jonathan Nieder wrote:\n> Ronnie Sahlberg wrote:\n>\n>> --- a/fast-import.c\n>> +++ b/fast-import.c\n>> @@ -1735,15 +1735,22 @@ static void dump_tags(void)\n>> {\n>> static const char *msg = \"fast-import\";\n>> struct tag *t;\n>> - struct ref_lock *lock;\n>> char ref_name[PATH_MAX];\n>> + struct strbuf err = STRBUF_INIT;\n>> + struct ref_transaction *transaction;\n>>\n>> + transaction = ref_transaction_begin(&err);\n>> for (t = first_tag; t; t = t->next_tag) {\n>> - sprintf(ref_name, \"tags/%s\", t->name);\n>> + snprintf(ref_name, PATH_MAX, \"refs/tags/%s\", t->name);\n>\n> That ignores the error if the refname happens to go longer than\n> PATH_MAX.\n>\n> This is the part of fast-import that doesn't need to be super\n> fast ;-). (The objects have been written to the pack, and now\n> we just need to write some refs.) Could this use a strbuf? By that,\n> I mean something like\n>\n> diff --git i/fast-import.c w/fast-import.c\n> index 3db5b3d..d5f6e63 100644\n> --- i/fast-import.c\n> +++ w/fast-import.c\n> @@ -1735,21 +1735,28 @@ static void dump_tags(void)\n> {\n> static const char *msg = \"fast-import\";\n> struct tag *t;\n> - char ref_name[PATH_MAX];\n> + struct strbuf ref_name = STRBUF_INIT;\n> struct strbuf err = STRBUF_INIT;\n> struct ref_transaction *transaction;\n>\n> + strbuf_addstr(&ref_name, \"refs/tags/\");\n> +\n> transaction = ref_transaction_begin(&err);\n> for (t = first_tag; t; t = t->next_tag) {\n> - snprintf(ref_name, PATH_MAX, \"refs/tags/%s\", t->name);\n> + strbuf_setlen(&ref_name, strlen(\"refs/tags/\"));\n> + strbuf_addstr(&ref_name, t->name);\n>\n> - if (ref_transaction_update(transaction, ref_name, t->sha1,\n> - NULL, 0, 0, &err))\n> - break;\n> + if (ref_transaction_update(transaction, ref_name.buf, t->sha1,\n> + NULL, 0, 0, &err)) {\n> + failure |= error(\"%s\", err.buf);\n> + goto done;\n> + }\n> }\n> if (ref_transaction_commit(transaction, msg, &err))\n> failure |= error(\"%s\", err.buf);\n> +done:\n> ref_transaction_free(transaction);\n> + strbuf_release(&ref_name);\n> strbuf_release(&err);\n> }\n\nChanged to strbuf. Thanks.\n\n>\n"},{"id":"242913","messageId":"20140528221720.GB12314@google.com","threadId":"36766","inReplyTo":"CAL=YDWkUhdoJkdg_zaq+p=XRu7H9fqNXDz89uPhbr4equTyVLQ@mail.gmail.com","subject":"Re: [PATCH v11 25/41] fast-import.c: use a ref transaction when dumping tags","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-28T22:17:20Z","receivedAt":"2014-05-28T22:17:20Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ronnie Sahlberg wrote:\n> On Wed, May 28, 2014 at 12:47 PM, Jonathan Nieder wrote:\n\n>> --- i/fast-import.c\n>> +++ w/fast-import.c\n>> @@ -1735,21 +1735,28 @@ static void dump_tags(void)\n>> {\n>> static const char *msg = \"fast-import\";\n>> struct tag *t;\n>> - char ref_name[PATH_MAX];\n>> + struct strbuf ref_name = STRBUF_INIT;\n>> struct strbuf err = STRBUF_INIT;\n>> struct ref_transaction *transaction;\n>>\n>> + strbuf_addstr(&ref_name, \"refs/tags/\");\n>> +\n>> transaction = ref_transaction_begin(&err);\n>> for (t = first_tag; t; t = t->next_tag) {\n>> - snprintf(ref_name, PATH_MAX, \"refs/tags/%s\", t->name);\n>> + strbuf_setlen(&ref_name, strlen(\"refs/tags/\"));\n>> + strbuf_addstr(&ref_name, t->name);\n>>\n>> - if (ref_transaction_update(transaction, ref_name, t->sha1,\n>> - NULL, 0, 0, &err))\n>> - break;\n>> + if (ref_transaction_update(transaction, ref_name.buf, t->sha1,\n>> + NULL, 0, 0, &err)) {\n>> + failure |= error(\"%s\", err.buf);\n>> + goto done;\n>> + }\n>> }\n>> if (ref_transaction_commit(transaction, msg, &err))\n>> failure |= error(\"%s\", err.buf);\n>> +done:\n>> ref_transaction_free(transaction);\n>> + strbuf_release(&ref_name);\n>> strbuf_release(&err);\n>> }\n>\n> Changed to strbuf. Thanks.\n\nThanks. The semantics when ref_transaction_update() fail seem weird ---\nsee above. (refs.h tells me \"A failure to update means that the\ntransaction as a whole has failed and will need to be rolled back\", so I\nassume that the function should be rolling back instead of calling\n_commit at that point.)\n"},{"id":"242917","messageId":"CAL=YDW=ruMzd=twadncjgFTh3yv=796cN72amJ4ep8a41tgmrA@mail.gmail.com","threadId":"36766","inReplyTo":"20140528221720.GB12314@google.com","subject":"Re: [PATCH v11 25/41] fast-import.c: use a ref transaction when dumping tags","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-28T23:20:52Z","receivedAt":"2014-05-28T23:20:52Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Wed, May 28, 2014 at 3:17 PM, Jonathan Nieder wrote:\n> Ronnie Sahlberg wrote:\n>> On Wed, May 28, 2014 at 12:47 PM, Jonathan Nieder wrote:\n>\n>>> --- i/fast-import.c\n>>> +++ w/fast-import.c\n>>> @@ -1735,21 +1735,28 @@ static void dump_tags(void)\n>>> {\n>>> static const char *msg = \"fast-import\";\n>>> struct tag *t;\n>>> - char ref_name[PATH_MAX];\n>>> + struct strbuf ref_name = STRBUF_INIT;\n>>> struct strbuf err = STRBUF_INIT;\n>>> struct ref_transaction *transaction;\n>>>\n>>> + strbuf_addstr(&ref_name, \"refs/tags/\");\n>>> +\n>>> transaction = ref_transaction_begin(&err);\n>>> for (t = first_tag; t; t = t->next_tag) {\n>>> - snprintf(ref_name, PATH_MAX, \"refs/tags/%s\", t->name);\n>>> + strbuf_setlen(&ref_name, strlen(\"refs/tags/\"));\n>>> + strbuf_addstr(&ref_name, t->name);\n>>>\n>>> - if (ref_transaction_update(transaction, ref_name, t->sha1,\n>>> - NULL, 0, 0, &err))\n>>> - break;\n>>> + if (ref_transaction_update(transaction, ref_name.buf, t->sha1,\n>>> + NULL, 0, 0, &err)) {\n>>> + failure |= error(\"%s\", err.buf);\n>>> + goto done;\n>>> + }\n>>> }\n>>> if (ref_transaction_commit(transaction, msg, &err))\n>>> failure |= error(\"%s\", err.buf);\n>>> +done:\n>>> ref_transaction_free(transaction);\n>>> + strbuf_release(&ref_name);\n>>> strbuf_release(&err);\n>>> }\n>>\n>> Changed to strbuf. Thanks.\n>\n> Thanks. The semantics when ref_transaction_update() fail seem weird ---\n> see above. (refs.h tells me \"A failure to update means that the\n> transaction as a whole has failed and will need to be rolled back\", so I\n> assume that the function should be rolling back instead of calling\n> _commit at that point.)\n\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 4a7b196..e24c7e4 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1735,15 +1735,24 @@ static void dump_tags(void)\n {\n static const char *msg = \"fast-import\";\n struct tag *t;\n- struct ref_lock *lock;\n- char ref_name[PATH_MAX];\n+ struct strbuf ref_name = STRBUF_INIT;\n+ struct strbuf err = STRBUF_INIT;\n+ struct ref_transaction *transaction;\n\n+ transaction = ref_transaction_begin(&err);\n for (t = first_tag; t; t = t->next_tag) {\n- sprintf(ref_name, \"tags/%s\", t->name);\n- lock = lock_ref_sha1(ref_name, NULL);\n- if (!lock || write_ref_sha1(lock, t->sha1, msg) < 0)\n- failure |= error(\"Unable to update %s\", ref_name);\n+ strbuf_reset(&ref_name);\n+ strbuf_addf(&ref_name, \"refs/tags/%s\", t->name);\n+\n+ if (ref_transaction_update(transaction, ref_name.buf, t->sha1,\n+ NULL, 0, 0, &err))\n+ break;\n }\n+ if (ref_transaction_commit(transaction, msg, &err))\n+ failure |= error(\"%s\", err.buf);\n+ ref_transaction_free(transaction);\n+ strbuf_release(&ref_name);\n+ strbuf_release(&err);\n }\n\n\nI rely on the fact that if the transaction has failed then it is safe\nto call ref_transaction_commit since it is guaranteed to return an\nerror too.\n"},{"id":"242919","messageId":"20140528233940.GC12314@google.com","threadId":"36766","inReplyTo":"CAL=YDW=ruMzd=twadncjgFTh3yv=796cN72amJ4ep8a41tgmrA@mail.gmail.com","subject":"Re: [PATCH v11 25/41] fast-import.c: use a ref transaction when dumping tags","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-28T23:39:40Z","receivedAt":"2014-05-28T23:39:40Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ronnie Sahlberg wrote:\n\n> I rely on the fact that if the transaction has failed then it is safe\n> to call ref_transaction_commit since it is guaranteed to return an\n> error too.\n\nYes, I am saying that behavior for ref_transaction_commit is weird.\n\nUsually when ref_transaction_commit is called I can do\n\n\tstruct strbuf err = STRBUF_INIT;\n\tif (ref_transaction_commit(..., &err))\n\t\tdie(\"%s\", err.buf);\n\nand I know that since ref_transaction_commit has returned a nonzero\nresult, err.buf is populated with a sensible message that will\ndescribe what went wrong.\n\nThat's true even if there's a bug elsewhere in code I didn't write\n(e.g., someone forgot to check the return value from\nref_transaction_update).\n\nBut the guarantee you are describing removes that property. It\ncreates a case where ref_transaction_commit can return nonzero without\nupdating err. So I get the following message:\n\n\tfatal:\n\nI don't think that's a good outcome.\n\nSure, if I am well acquainted with the API, I can make sure to use the\nsame strbuf for all transaction API calls. But that would result in\nstrange behavior, too: if multiple _update calls fail, then I get\nconcatenated messages.\n\nOkay, I can make sure to do at most one failing _update, before\ncalling _commit and printing the error. But at that point, what is\nthe advantage over normal exception handling, where the error gets\nhandled at the _update call site?\n\nJonathan\n"},{"id":"242963","messageId":"CAL=YDW=WmNObkTO_uybTToeMKGGQf5NC0oFvy_pMrsg+ehpzog@mail.gmail.com","threadId":"36766","inReplyTo":"20140528233940.GC12314@google.com","subject":"Re: [PATCH v11 25/41] fast-import.c: use a ref transaction when dumping tags","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-29T16:10:41Z","receivedAt":"2014-05-29T16:10:41Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Wed, May 28, 2014 at 4:39 PM, Jonathan Nieder wrote:\n> Ronnie Sahlberg wrote:\n>\n>> I rely on the fact that if the transaction has failed then it is safe\n>> to call ref_transaction_commit since it is guaranteed to return an\n>> error too.\n>\n> Yes, I am saying that behavior for ref_transaction_commit is weird.\n>\n> Usually when ref_transaction_commit is called I can do\n>\n> struct strbuf err = STRBUF_INIT;\n> if (ref_transaction_commit(..., &err))\n> die(\"%s\", err.buf);\n>\n> and I know that since ref_transaction_commit has returned a nonzero\n> result, err.buf is populated with a sensible message that will\n> describe what went wrong.\n>\n> That's true even if there's a bug elsewhere in code I didn't write\n> (e.g., someone forgot to check the return value from\n> ref_transaction_update).\n>\n> But the guarantee you are describing removes that property. It\n> creates a case where ref_transaction_commit can return nonzero without\n> updating err. So I get the following message:\n>\n> fatal:\n>\n> I don't think that's a good outcome.\n\nIn this case \"fatal:\" can not happen.\nThis is no more subtle than most of the git core.\n\n\nI have changed this function to explicitly abort on _update failing\nbut I think this is making the api too restrictive.\n\n\n>\n> Sure, if I am well acquainted with the API, I can make sure to use the\n> same strbuf for all transaction API calls. But that would result in\n> strange behavior, too: if multiple _update calls fail, then I get\n> concatenated messages.\n>\n> Okay, I can make sure to do at most one failing _update, before\n> calling _commit and printing the error. But at that point, what is\n> the advantage over normal exception handling, where the error gets\n> handled at the _update call site?\n>\n> Jonathan\n"},{"id":"242967","messageId":"20140529174106.GE12314@google.com","threadId":"36766","inReplyTo":"CAL=YDW=WmNObkTO_uybTToeMKGGQf5NC0oFvy_pMrsg+ehpzog@mail.gmail.com","subject":"Re: [PATCH v11 25/41] fast-import.c: use a ref transaction when dumping tags","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-29T17:41:06Z","receivedAt":"2014-05-29T17:41:06Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ronnie Sahlberg wrote:\n> On Wed, May 28, 2014 at 4:39 PM, Jonathan Nieder wrote:\n\n>> Usually when ref_transaction_commit is called I can do\n>>\n>> struct strbuf err = STRBUF_INIT;\n>> if (ref_transaction_commit(..., &err))\n>> die(\"%s\", err.buf);\n>>\n>> and I know that since ref_transaction_commit has returned a nonzero\n>> result, err.buf is populated with a sensible message that will\n>> describe what went wrong.\n[...]\n>> But the guarantee you are describing removes that property. It\n>> creates a case where ref_transaction_commit can return nonzero without\n>> updating err. So I get the following message:\n>>\n>> fatal:\n>>\n>> I don't think that's a good outcome.\n>\n> In this case \"fatal:\" can not happen.\n> This is no more subtle than most of the git core.\n>\n> I have changed this function to explicitly abort on _update failing\n> but I think this is making the api too restrictive.\n\nI don't want to push you toward making a change you think is wrong. I\ncertainly don't own the codebase, and there are lots of other people\n(e.g., Michael, Junio, Jeff) to get advice from. So I guess I should\ntry to address this.\n\nI'm not quite sure what you mean by too restrictive.\n\n a. Having API constraints that aren't enforced by the function makes\n using the API too fussy.\n\n I agree with that. That was something I liked about keeping track\n of the OPEN/CLOSED state of a transaction, which would let\n functions like _commit die() if someone is misusing the API so the\n problem gets detected early.\n\n b. Having to check the return value from _update() is too fussy.\n \n It certainly seems *possible* to have an API that doesn't require\n checking the return value, while still avoiding the usability\n problem I described in the quoted message above. For example:\n\n * _update() returns void and has no strbuf parameter\n * error handling happens by checking the error from _commit()\n\n That would score well on the scale described at\n http://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html\n\n An API where checking the return value is optional would be\n doable, too. For example:\n\n * _update() returns int and has a strbuf parameter\n * if the strbuf parameter is NULL, the caller is expected to\n wait for _commit() to check for errors, and a relevant\n message will be passed back then\n * if the strbuf parameter is non-NULL, then calling _commit()\n after an error is an API violation\n\nI don't understand the comment about no more subtle than most of git.\nAre you talking about the errno action at a distance you found in some\nfunctions? I thought we agreed that those were mistakes that accrue\nwhen people aim for a quick fix without thinking about maintainability\nand something git should have less of.\n\nJonathan\n"},{"id":"243019","messageId":"20140530172808.GG12314@google.com","threadId":"36766","inReplyTo":"1401222360-21175-33-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v11 32/41] refs.c: make delete_ref use a transaction","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-30T17:28:08Z","receivedAt":"2014-05-30T17:28: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> refs.c | 34 +++++++++++++---------------------\n> 1 file changed, 13 insertions(+), 21 deletions(-)\n\nReviewed-by: Jonathan Nieder \n\n[...]\n> +++ b/refs.c\n[...]\n> @@ -2542,24 +2537,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[...]\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> +\t}\n[...]\n> -\tret |= repack_without_ref(lock->ref_name);\n\nThe old return value could be 1 or -1 depending on how the deletion\nfailed. Now it's consistently 1.\n\nThe only callers I see that care are cmd_symbolic_ref and\ncmd_update_ref, for which 1 is better (-1 would result in an exit\nstatus of 255, which means something like \"died with signal 127\").\n\nThanks,\nJonathan\n"},{"id":"243023","messageId":"20140530173831.GH12314@google.com","threadId":"36766","inReplyTo":"1401222360-21175-34-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v11 33/41] refs.c: pass the ref log message to _create/delete/update instead of _commit","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-30T17:38:31Z","receivedAt":"2014-05-30T17:38:31Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ronnie Sahlberg wrote:\n\n> Change the reference transactions so that we pass the reflog message\n> through to the create/delete/update function instead of the commit message.\n> This allows for individual messages for each change in a multi ref\n> transaction.\n\nNice.\n\nThat reminds me: in the future, do we want to have some way to figure\nout what ref updates happened together? E.g., cvsnt introduced commit\nidentifiers to answer a similar kind of question in CVS per-file\nhistory. If some backend wants to support that, the API this patch\nintroduces would handle it fine --- good.\n\n[...]\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\nUnrelated change snuck in?\n\nThe rest of the patch is\nReviewed-by: Jonathan Nieder \n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex faa1233..55f457c 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -673,9 +673,10 @@ 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:\n"},{"id":"243028","messageId":"20140530180815.GI12314@google.com","threadId":"36766","inReplyTo":"1401222360-21175-36-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v11 35/41] refs.c: pack all refs before we start to rename a ref","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-30T18:08:15Z","receivedAt":"2014-05-30T18:08:15Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ronnie Sahlberg wrote:\n\n> We want to do this to make it easier to handle atomic renames in rename_ref for\n> the case 'git branch -m foo/bar foo'.\n\nIn an ideal world, a part of me would rather see \"git branch -m\" doing\nsomething more targeted by only packing the two refs it is working\nwith, and only when it knows it has to so the normal \"git branch -m\nfoo bar\" case doesn't have to suffer. But:\n\nThat would be more complicated.\n\nPacking refs is not very slow and has some good benefits for\nperformance of later commands. Once we've committed to writing out a\nnew pack-refs file, it's not so bad to enumerate all loose refs to get\nmore benefit from writing out the new packed-refs file.\n\nRenaming refs is something usually done by humans and not by scripts\nor remote clients. I'm not too worried about \"git branch -m\" in a\ntight loop getting slower.\n\nSo I think this is an okay thing to do.\n\n> If we can guarantee that foo/bar does not\n> exist as a loose ref we can avoid locking foo/bar.lock during transaction\n> commit\n\nThat is not quite true --- there's still value in locking foo/bar to\navoid clobbering a concurrent ref update.\n\nIf git performed the entire rename transaction in the packed-refs\nfile, we could avoid that race completely (for 'git branch -m foo/bar\nfoo': lock refs, make sure the loose refs are pruned, perform the\nupdate in packed-refs, unlock refs. for 'git branch -m foo foo/bar':\nlock foo, prune foo, lock foo/bar, prune foo/bar, perform the update\nin packed-refs, unlock refs).\n\nEven without doing that, packing refs first has a benefit in terms of\nordering: if you do (1) delete loose foo/bar, (2) write loose foo, (3)\nrewrite packed-refs without foo/bar, then because you've packed refs\nfirst, no objects become unreferenced during step (1), which means\nthat a hypothetical version of \"git gc\" that used filesystem snapshots\nwould not see any relevant objects as safe to clean up no matter when\nit runs.\n\n> Signed-off-by: Ronnie Sahlberg \n> ---\n> refs.c | 3 +++\n> t/t3200-branch.sh | 6 +++---\n> 2 files changed, 6 insertions(+), 3 deletions(-)\n\nWith a clearer commit message,\nReviewed-by: Jonathan Nieder \n"},{"id":"243029","messageId":"20140530181242.GJ12314@google.com","threadId":"36766","inReplyTo":"1401222360-21175-37-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v11 36/41] refs.c: move the check for valid refname to lock_ref_sha1_basic","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-30T18:12:42Z","receivedAt":"2014-05-30T18:12:42Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ronnie Sahlberg wrote:\n\n> Move the check for check_refname_format from lock_any_ref_for_update\n> to lock_ref_sha1_basic. At some later stage we will get rid of\n> lock_any_ref_for_update completely.\n\nDo you know if this will cause any functional change?\n\nWhat is the potential impact? Is that impact worth it? (Given how\nbroken the recovery codepaths currently are, I suspect this change is\nworth it, but it seems worth documenting in the log message.)\n\nThanks,\nJonathan\n"},{"id":"243066","messageId":"20140531002230.GL12314@google.com","threadId":"36766","inReplyTo":"1401222360-21175-40-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v11 39/41] refs.c: propagate any errno==ENOTDIR from _commit back to the callers","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-31T00:22:30Z","receivedAt":"2014-05-31T00:22:30Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nRonnie Sahlberg wrote:\n\n> For these cases, save the errno value and abort and make sure that the caller\n> can see errno==ENOTDIR.\n>\n> Also start defining specific return codes for _commit, assign -1 as a generic\n> error and -2 as the error that refers to a name conflict. Callers can (and\n> should) use that return value inspecting errno directly.\n\nHeh. Here's a patch on top to hopefully finish that part of the job.\n\nUnfortunately, the value of errno after calling lock_ref_sha1_basic is\nnot reliable.\nhttp://thread.gmane.org/gmane.comp.version-control.git/249159/focus=249407\nlists the error paths that are broken (marked with \"[!]\" in that\nmessage).\n\ndiff --git i/builtin/fetch.c w/builtin/fetch.c\nindex b13e8f9..0a01cda 100644\n--- i/builtin/fetch.c\n+++ w/builtin/fetch.c\n@@ -377,6 +377,7 @@ static int s_update_ref(const char *action,\n \tchar *rla = getenv(\"GIT_REFLOG_ACTION\");\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@@ -387,16 +388,22 @@ static int s_update_ref(const char *action,\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 ref_transaction_commit(transaction, &err)) {\n-\t\tref_transaction_free(transaction);\n-\t\terror(\"%s\", err.buf);\n-\t\tstrbuf_release(&err);\n-\t\treturn errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :\n-\t\t\t\t\t STORE_REF_ERROR_OTHER;\n-\t}\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 \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\ndiff --git i/refs.c w/refs.c\nindex dbaabba..b22b99b 100644\n--- i/refs.c\n+++ w/refs.c\n@@ -3499,7 +3499,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, save_errno = 0;\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@@ -3535,7 +3535,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t\t\t delnames, delnum);\n \t\tif (!update->lock) {\n \t\t\tif (errno == ENOTDIR)\n-\t\t\t\tsave_errno = errno;\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@@ -3590,8 +3590,7 @@ cleanup:\n \t\tif (updates[i]->lock)\n \t\t\tunlock_ref(updates[i]->lock);\n \tfree(delnames);\n-\terrno = save_errno;\n-\tif (save_errno == ENOTDIR)\n+\tif (df_conflict)\n \t\tret = -2;\n \treturn ret;\n }\ndiff --git i/refs.h w/refs.h\nindex 88732a1..1583097 100644\n--- i/refs.h\n+++ w/refs.h\n@@ -325,9 +325,11 @@ int ref_transaction_delete(struct ref_transaction *transaction,\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 -2 if the\n- * failure was due to a name collision (ENOTDIR).\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"},{"id":"243192","messageId":"CAL=YDW=j9m=_QrFLNu3w_UeH_JKUxmk4+VMh0-FFTdDq0CdOJg@mail.gmail.com","threadId":"36766","inReplyTo":"20140530180815.GI12314@google.com","subject":"Re: [PATCH v11 35/41] refs.c: pack all refs before we start to rename a ref","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-03T16:39:21Z","receivedAt":"2014-06-03T16:39:21Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Thanks!\n\nOn Fri, May 30, 2014 at 11:08 AM, Jonathan Nieder wrote:\n> Ronnie Sahlberg wrote:\n>\n>> We want to do this to make it easier to handle atomic renames in rename_ref for\n>> the case 'git branch -m foo/bar foo'.\n>\n> In an ideal world, a part of me would rather see \"git branch -m\" doing\n> something more targeted by only packing the two refs it is working\n> with, and only when it knows it has to so the normal \"git branch -m\n> foo bar\" case doesn't have to suffer. But:\n>\n> That would be more complicated.\n>\n> Packing refs is not very slow and has some good benefits for\n> performance of later commands. Once we've committed to writing out a\n> new pack-refs file, it's not so bad to enumerate all loose refs to get\n> more benefit from writing out the new packed-refs file.\n>\n> Renaming refs is something usually done by humans and not by scripts\n> or remote clients. I'm not too worried about \"git branch -m\" in a\n> tight loop getting slower.\n>\n> So I think this is an okay thing to do.\n>\n>> If we can guarantee that foo/bar does not\n>> exist as a loose ref we can avoid locking foo/bar.lock during transaction\n>> commit\n>\n> That is not quite true --- there's still value in locking foo/bar to\n> avoid clobbering a concurrent ref update.\n>\n> If git performed the entire rename transaction in the packed-refs\n> file, we could avoid that race completely (for 'git branch -m foo/bar\n> foo': lock refs, make sure the loose refs are pruned, perform the\n> update in packed-refs, unlock refs. for 'git branch -m foo foo/bar':\n> lock foo, prune foo, lock foo/bar, prune foo/bar, perform the update\n> in packed-refs, unlock refs).\n>\n> Even without doing that, packing refs first has a benefit in terms of\n> ordering: if you do (1) delete loose foo/bar, (2) write loose foo, (3)\n> rewrite packed-refs without foo/bar, then because you've packed refs\n> first, no objects become unreferenced during step (1), which means\n> that a hypothetical version of \"git gc\" that used filesystem snapshots\n> would not see any relevant objects as safe to clean up no matter when\n> it runs.\n>\n>> Signed-off-by: Ronnie Sahlberg \n>> ---\n>> refs.c | 3 +++\n>> t/t3200-branch.sh | 6 +++---\n>> 2 files changed, 6 insertions(+), 3 deletions(-)\n>\n> With a clearer commit message,\n> Reviewed-by: Jonathan Nieder \n"},{"id":"243193","messageId":"CAL=YDW=W4V-Eb8nK5dx_dKyVHuKDwvGJhLihosgmGZx8kO-O2w@mail.gmail.com","threadId":"36766","inReplyTo":"20140530172808.GG12314@google.com","subject":"Re: [PATCH v11 32/41] refs.c: make delete_ref use a transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-03T16:59:12Z","receivedAt":"2014-06-03T16:59:12Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Thanks!\n\nOn Fri, May 30, 2014 at 10:28 AM, Jonathan Nieder wrote:\n> Ronnie Sahlberg wrote:\n>\n>> Signed-off-by: Ronnie Sahlberg \n>> ---\n>> refs.c | 34 +++++++++++++---------------------\n>> 1 file changed, 13 insertions(+), 21 deletions(-)\n>\n> Reviewed-by: Jonathan Nieder \n>\n> [...]\n>> +++ b/refs.c\n> [...]\n>> @@ -2542,24 +2537,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> [...]\n>> + if (!transaction ||\n>> + ref_transaction_delete(transaction, refname, sha1, delopt,\n>> + sha1 && !is_null_sha1(sha1), &err) ||\n>> + ref_transaction_commit(transaction, NULL, &err)) {\n>> + error(\"%s\", err.buf);\n>> + ref_transaction_free(transaction);\n>> + strbuf_release(&err);\n>> return 1;\n>> + }\n> [...]\n>> - ret |= repack_without_ref(lock->ref_name);\n>\n> The old return value could be 1 or -1 depending on how the deletion\n> failed. Now it's consistently 1.\n>\n> The only callers I see that care are cmd_symbolic_ref and\n> cmd_update_ref, for which 1 is better (-1 would result in an exit\n> status of 255, which means something like \"died with signal 127\").\n>\n> Thanks,\n> Jonathan\n"},{"id":"243194","messageId":"CAL=YDWkjWJxx-ZYNO1GjzsfiecH=vmj5e0bgJeYH1ccMa4zjKQ@mail.gmail.com","threadId":"36766","inReplyTo":"20140530173831.GH12314@google.com","subject":"Re: [PATCH v11 33/41] refs.c: pass the ref log message to _create/delete/update instead of _commit","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-03T17:01:48Z","receivedAt":"2014-06-03T17:01:48Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Fri, May 30, 2014 at 10:38 AM, Jonathan Nieder wrote:\n> Ronnie Sahlberg wrote:\n>\n>> Change the reference transactions so that we pass the reflog message\n>> through to the create/delete/update function instead of the commit message.\n>> This allows for individual messages for each change in a multi ref\n>> transaction.\n>\n> Nice.\n>\n> That reminds me: in the future, do we want to have some way to figure\n> out what ref updates happened together? E.g., cvsnt introduced commit\n> identifiers to answer a similar kind of question in CVS per-file\n> history. If some backend wants to support that, the API this patch\n> introduces would handle it fine --- good.\n>\n> [...]\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>> }\n>> }\n>> }\n>> -\n>> if (rc & STORE_REF_ERROR_DF_CONFLICT)\n>> error(_(\"some local refs could not be updated; try running\\n\"\n>> - \" 'git remote prune %s' to remove any old, conflicting \"\n>> + \"'git remote prune %s' to remove any old, conflicting \"\n>> \"branches\"), remote_name);\n>\n> Unrelated change snuck in?\n\nYeah, there shouldn't be a space there.\n\n>\n> The rest of the patch is\n> Reviewed-by: Jonathan Nieder \n\nThanks!\n\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index faa1233..55f457c 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -673,9 +673,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n> }\n> }\n> }\n> +\n> if (rc & STORE_REF_ERROR_DF_CONFLICT)\n> error(_(\"some local refs could not be updated; try running\\n\"\n> - \"'git remote prune %s' to remove any old, conflicting \"\n> + \" 'git remote prune %s' to remove any old, conflicting \"\n> \"branches\"), remote_name);\n>\n> abort:\n"},{"id":"243197","messageId":"CAL=YDWm9UtD0fLS0RjaQYxAk5OMGcDmoC4QK5NrzMQjXOGuWjQ@mail.gmail.com","threadId":"36766","inReplyTo":"20140531002230.GL12314@google.com","subject":"Re: [PATCH v11 39/41] refs.c: propagate any errno==ENOTDIR from _commit back to the callers","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-03T18:21:37Z","receivedAt":"2014-06-03T18:21:37Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Fri, May 30, 2014 at 5:22 PM, Jonathan Nieder wrote:\n> Hi,\n>\n> Ronnie Sahlberg wrote:\n>\n>> For these cases, save the errno value and abort and make sure that the caller\n>> can see errno==ENOTDIR.\n>>\n>> Also start defining specific return codes for _commit, assign -1 as a generic\n>> error and -2 as the error that refers to a name conflict. Callers can (and\n>> should) use that return value inspecting errno directly.\n>\n> Heh. Here's a patch on top to hopefully finish that part of the job.\n\nThanks. Updated.\n\n>\n> Unfortunately, the value of errno after calling lock_ref_sha1_basic is\n> not reliable.\n\nI have changed the patch for lock_ref_sha1_basic so it now does :\n if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n errno = EINVAL;\n return NULL;\n }\n\nso this function should no longer fail without changing errno to a,\nhopefully, sane value.\n\n\n> http://thread.gmane.org/gmane.comp.version-control.git/249159/focus=249407\n> lists the error paths that are broken (marked with \"[!]\" in that\n> message).\n>\n> diff --git i/builtin/fetch.c w/builtin/fetch.c\n> index b13e8f9..0a01cda 100644\n> --- i/builtin/fetch.c\n> +++ w/builtin/fetch.c\n> @@ -377,6 +377,7 @@ static int s_update_ref(const char *action,\n> char *rla = getenv(\"GIT_REFLOG_ACTION\");\n> struct ref_transaction *transaction;\n> struct strbuf err = STRBUF_INIT;\n> + int ret, df_conflict = 0;\n>\n> if (dry_run)\n> return 0;\n> @@ -387,16 +388,22 @@ static int s_update_ref(const char *action,\n> transaction = ref_transaction_begin(&err);\n> if (!transaction ||\n> ref_transaction_update(transaction, ref->name, ref->new_sha1,\n> - ref->old_sha1, 0, check_old, msg, &err) ||\n> - ref_transaction_commit(transaction, &err)) {\n> - ref_transaction_free(transaction);\n> - error(\"%s\", err.buf);\n> - strbuf_release(&err);\n> - return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :\n> - STORE_REF_ERROR_OTHER;\n> - }\n> + ref->old_sha1, 0, check_old, msg, &err))\n> + goto fail;\n> +\n> + ret = ref_transaction_commit(transaction, &err);\n> + if (ret == UPDATE_REFS_NAME_CONFLICT)\n> + df_conflict = 1;\n> + if (ret)\n> + goto fail;\n> ref_transaction_free(transaction);\n> return 0;\n> +fail:\n> + ref_transaction_free(transaction);\n> + error(\"%s\", err.buf);\n> + strbuf_release(&err);\n> + return df_conflict ? STORE_REF_ERROR_DF_CONFLICT\n> + : STORE_REF_ERROR_OTHER;\n> }\n>\n> #define REFCOL_WIDTH 10\n> diff --git i/refs.c w/refs.c\n> index dbaabba..b22b99b 100644\n> --- i/refs.c\n> +++ w/refs.c\n> @@ -3499,7 +3499,7 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n> int ref_transaction_commit(struct ref_transaction *transaction,\n> struct strbuf *err)\n> {\n> - int ret = 0, delnum = 0, i, save_errno = 0;\n> + int ret = 0, delnum = 0, i, df_conflict = 0;\n> const char **delnames;\n> int n = transaction->nr;\n> struct ref_update **updates = transaction->updates;\n> @@ -3535,7 +3535,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n> delnames, delnum);\n> if (!update->lock) {\n> if (errno == ENOTDIR)\n> - save_errno = errno;\n> + df_conflict = 1;\n> if (err)\n> strbuf_addf(err, \"Cannot lock the ref '%s'.\",\n> update->refname);\n> @@ -3590,8 +3590,7 @@ cleanup:\n> if (updates[i]->lock)\n> unlock_ref(updates[i]->lock);\n> free(delnames);\n> - errno = save_errno;\n> - if (save_errno == ENOTDIR)\n> + if (df_conflict)\n> ret = -2;\n> return ret;\n> }\n> diff --git i/refs.h w/refs.h\n> index 88732a1..1583097 100644\n> --- i/refs.h\n> +++ w/refs.h\n> @@ -325,9 +325,11 @@ int ref_transaction_delete(struct ref_transaction *transaction,\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 -2 if the\n> - * failure was due to a name collision (ENOTDIR).\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> struct strbuf *err);\n>\n"},{"id":"243201","messageId":"20140603183124.GD30019@google.com","threadId":"36766","inReplyTo":"1401222360-21175-1-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v11 00/41] Use ref transactions","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-06-03T18:31:24Z","receivedAt":"2014-06-03T18:31:24Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nRonnie Sahlberg wrote:\n\n> It\n> converts all ref updates, inside refs.c as well as external, to use the\n> transaction API for updates. This makes most of the ref updates to become\n> atomic when there are failures locking or writing to a ref.\n\nI'm still excited about this series. Most of it looks ready. The\nremaining problems I see fall into a few categories:\n\n * The rename_ref codepath is strange. You've convinced me that the\n approach you're taking there is not much of a regression, but it's\n confusing enough that I'd be happier if someone else takes a closer\n look (or I can try to find time to).\n\n * I think the approach taken in the patch \"add transaction.status and\n track OPEN/CLOSED/ERROR\" is a mistake and would make callers more\n error-prone. The basic idea of checking that the caller is using\n the API right is valuable, so there is something in that patch I\n really like --- it's just the details (involving the same kind of\n easy-to-clobber error messages as errno provides with stdio) that\n seem broken.\n\n I suspect I'm just not communicating very well there. Maybe\n mhagger or someone else could give it a sanity check.\n\n * Some commit messages (e.g., the one to \"pack all refs before we\n start to rename a ref\") are confusing. That might be a sign that\n what those patches are trying to do is confusing.\n\n * The error handling in \"add an err argument to repack_without_refs\"\n is a thorny thicket. It still has bugs. I can completely\n understand not wanting to take that on but I think it is important\n to add a NEEDSWORK comment describing the known bugs to help people\n when they work with this code in the future.\n\nI realize the process of addressing review comments in such a long\nseries has been a bit of a pain, and if there's anything I can do to\nmake it easier please let me know. Hopefully adding some other\nreviewers can help.\n\nThanks,\nJonathan\n"},{"id":"243202","messageId":"CAL=YDWkFt6cfHUyTtxYAhd=PU=5FYfh0kULRKGifAH3q0+wjOA@mail.gmail.com","threadId":"36766","inReplyTo":"20140530181242.GJ12314@google.com","subject":"Re: [PATCH v11 36/41] refs.c: move the check for valid refname to lock_ref_sha1_basic","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-03T18:33:14Z","receivedAt":"2014-06-03T18:33:14Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Fri, May 30, 2014 at 11:12 AM, Jonathan Nieder wrote:\n> Ronnie Sahlberg wrote:\n>\n>> Move the check for check_refname_format from lock_any_ref_for_update\n>> to lock_ref_sha1_basic. At some later stage we will get rid of\n>> lock_any_ref_for_update completely.\n>\n> Do you know if this will cause any functional change?\n>\n> What is the potential impact? Is that impact worth it? (Given how\n> broken the recovery codepaths currently are, I suspect this change is\n> worth it, but it seems worth documenting in the log message.)\n\nThanks.\n\nUpdated the commit message to mention this.\n\n This changes semantics for lock_ref_sha1_basic slightly. With this change\n it is no longer possible to open a ref that has a badly name which breaks\n any codepaths that tries to open and repair badly named refs. The normal refs\n API should not allow neither creating nor accessing refs with invalid names.\n If we need such recovery code we could add it as an option to git\nfsck and have\n git fsck be the only sanctioned way of bypassing the normal API and checks.\n\n>\n> Thanks,\n> Jonathan\n"},{"id":"243209","messageId":"CAL=YDW=M1nioeiXu5mEHPY+hgPhLbm6MC509Je5E-13UYOUnPQ@mail.gmail.com","threadId":"36766","inReplyTo":"20140529174106.GE12314@google.com","subject":"Re: [PATCH v11 25/41] fast-import.c: use a ref transaction when dumping tags","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-03T21:32:49Z","receivedAt":"2014-06-03T21:32:49Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Thanks.\n\nI have changed the transaction functions to die(BUG:) if the user\ntries to call _update/_create/_delete on a failed transaction.\n\n\n\nOn Thu, May 29, 2014 at 10:41 AM, Jonathan Nieder wrote:\n> Ronnie Sahlberg wrote:\n>> On Wed, May 28, 2014 at 4:39 PM, Jonathan Nieder wrote:\n>\n>>> Usually when ref_transaction_commit is called I can do\n>>>\n>>> struct strbuf err = STRBUF_INIT;\n>>> if (ref_transaction_commit(..., &err))\n>>> die(\"%s\", err.buf);\n>>>\n>>> and I know that since ref_transaction_commit has returned a nonzero\n>>> result, err.buf is populated with a sensible message that will\n>>> describe what went wrong.\n> [...]\n>>> But the guarantee you are describing removes that property. It\n>>> creates a case where ref_transaction_commit can return nonzero without\n>>> updating err. So I get the following message:\n>>>\n>>> fatal:\n>>>\n>>> I don't think that's a good outcome.\n>>\n>> In this case \"fatal:\" can not happen.\n>> This is no more subtle than most of the git core.\n>>\n>> I have changed this function to explicitly abort on _update failing\n>> but I think this is making the api too restrictive.\n>\n> I don't want to push you toward making a change you think is wrong. I\n> certainly don't own the codebase, and there are lots of other people\n> (e.g., Michael, Junio, Jeff) to get advice from. So I guess I should\n> try to address this.\n>\n> I'm not quite sure what you mean by too restrictive.\n>\n> a. Having API constraints that aren't enforced by the function makes\n> using the API too fussy.\n>\n> I agree with that. That was something I liked about keeping track\n> of the OPEN/CLOSED state of a transaction, which would let\n> functions like _commit die() if someone is misusing the API so the\n> problem gets detected early.\n>\n> b. Having to check the return value from _update() is too fussy.\n>\n> It certainly seems *possible* to have an API that doesn't require\n> checking the return value, while still avoiding the usability\n> problem I described in the quoted message above. For example:\n>\n> * _update() returns void and has no strbuf parameter\n> * error handling happens by checking the error from _commit()\n>\n> That would score well on the scale described at\n> http://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html\n>\n> An API where checking the return value is optional would be\n> doable, too. For example:\n>\n> * _update() returns int and has a strbuf parameter\n> * if the strbuf parameter is NULL, the caller is expected to\n> wait for _commit() to check for errors, and a relevant\n> message will be passed back then\n> * if the strbuf parameter is non-NULL, then calling _commit()\n> after an error is an API violation\n>\n> I don't understand the comment about no more subtle than most of git.\n> Are you talking about the errno action at a distance you found in some\n> functions? I thought we agreed that those were mistakes that accrue\n> when people aim for a quick fix without thinking about maintainability\n> and something git should have less of.\n>\n> Jonathan\n"}]}