{"thread":{"id":"36785","subject":"[PATCH v12 16/41] refs.c: add transaction.status and track OPEN/CLOSED/ERROR","startedAt":"2014-05-29T16:07:42Z","lastAt":"2014-06-03T20:55:02Z","messageCount":18,"participants":["Ronnie Sahlberg","Jonathan Nieder"],"isPatch":true,"patchVersion":12,"patchTotal":41},"messages":[{"id":"242950","messageId":"1401379676-9307-1-git-send-email-sahlberg@google.com","threadId":"36785","inReplyTo":null,"subject":"[PATCH v12 00/44] Use ref transactions for all ref updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-29T16:07:42Z","receivedAt":"2014-05-29T16:07:42Z","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\nRonnie please review these remaining patches in this series.\n\n\n Sahlberg (44):\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: ref_transaction_delete to check for error and return status\n  tag.c: use ref transactions when doing updates\n  replace.c: use the ref transaction functions for updates\n  commit.c: use ref transactions for updates\n  sequencer.c: use ref transactions for all ref updates\n  fast-import.c: change update_branch to use ref transactions\n  branch.c: use ref transaction for all ref updates\n  refs.c: change update_ref to use a transaction\n  refs.c: free the transaction before returning when number of updates\n    is 0\n  refs.c: ref_transaction_commit should not free the transaction\n  fetch.c: clear errno before calling functions that might set it\n  fetch.c: change s_update_ref to use a ref transaction\n  fetch.c: use a single ref transaction for all ref updates\n  receive-pack.c: use a reference transaction for updating the refs\n  fast-import.c: use a ref transaction when dumping tags\n  walker.c: use ref transaction for ref updates\n  refs.c: make write_ref_sha1 static\n  refs.c: make lock_ref_sha1 static\n  refs.c: add transaction.status and track OPEN/CLOSED/ERROR\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: add a new flag for transaction delete for refs we know are\n    packed only\n  refs.c: pass a skip list to name_conflict_fn\n  refs.c: make rename_ref use a transaction\n  refs.c: remove forward declaration of write_ref_sha1\n\n branch.c               |  30 ++--\n builtin/commit.c       |  24 ++-\n builtin/fetch.c        |  29 +--\n builtin/receive-pack.c |  21 +--\n builtin/replace.c      |  15 +-\n builtin/tag.c          |  15 +-\n builtin/update-ref.c   |  32 ++--\n cache.h                |   2 +\n fast-import.c          |  42 +++--\n lockfile.c             |  21 ++-\n refs.c                 | 468 +++++++++++++++++++++++++++++--------------------\n refs.h                 |  54 +++---\n sequencer.c            |  24 ++-\n t/t3200-branch.sh      |   2 +-\n walker.c               |  51 +++---\n 15 files changed, 491 insertions(+), 339 deletions(-)\n\n-- \n2.0.0.rc3.510.g20c254b\n"},{"id":"242958","messageId":"1401379676-9307-2-git-send-email-sahlberg@google.com","threadId":"36785","inReplyTo":"1401379676-9307-1-git-send-email-sahlberg@google.com","subject":"[PATCH v12 06/41] refs.c: add an err argument to repack_without_refs","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-29T16:07:43Z","receivedAt":"2014-05-29T16:07:43Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Update repack_without_refs to take an err argument and update it if there\nis a failure. Pass the err variable from ref_transaction_commit to this\nfunction so that callers can print a meaningful error message if _commit\nfails due to a problem in repack_without_refs.\n\nAdd a new function unable_to_lock_message that takes a strbuf argument and\nfills in the reason for the failure.\n\nIn commit_packed_refs, make sure that we propagate any errno that\ncommit_lock_file might have set back to our caller.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\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..5858da8 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -559,6 +559,8 @@ struct lock_file {\n #define LOCK_DIE_ON_ERROR 1\n #define LOCK_NODEREF 2\n extern int unable_to_lock_error(const char *path, int err);\n+extern void unable_to_lock_message(const char *path, int err,\n+\t\t\t\t   struct strbuf *buf);\n extern NORETURN void unable_to_lock_index_die(const char *path, int err);\n extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);\n extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);\ndiff --git a/lockfile.c b/lockfile.c\nindex 8fbcb6a..92e0397 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_message(const char *path, int err, struct strbuf *buf)\n {\n-\tstruct strbuf buf = STRBUF_INIT;\n \n \tif (err == EEXIST) {\n-\t\tstrbuf_addf(&buf, \"Unable to create '%s.lock': %s.\\n\\n\"\n+\t\tstrbuf_addf(buf, \"Unable to create '%s.lock': %s.\\n\\n\"\n \t\t    \"If no other git process is currently running, this probably means a\\n\"\n \t\t    \"git process crashed in this repository earlier. Make sure no other git\\n\"\n \t\t    \"process is running and remove the file manually to continue.\",\n \t\t\t    absolute_path(path), strerror(err));\n \t} else\n-\t\tstrbuf_addf(&buf, \"Unable to create '%s.lock': %s\",\n+\t\tstrbuf_addf(buf, \"Unable to create '%s.lock': %s\",\n \t\t\t    absolute_path(path), strerror(err));\n-\treturn strbuf_detach(&buf, NULL);\n }\n \n int unable_to_lock_error(const char *path, int err)\n {\n-\tchar *msg = unable_to_lock_message(path, err);\n-\terror(\"%s\", msg);\n-\tfree(msg);\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tunable_to_lock_message(path, err, &buf);\n+\terror(\"%s\", buf.buf);\n+\tstrbuf_release(&buf);\n \treturn -1;\n }\n \n NORETURN void unable_to_lock_index_die(const char *path, int err)\n {\n-\tdie(\"%s\", unable_to_lock_message(path, err));\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tunable_to_lock_message(path, err, &buf);\n+\tdie(\"%s\", buf.buf);\n }\n \n int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)\ndiff --git a/refs.c b/refs.c\nindex 25c3a93..6e28841 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_message(git_path(\"packed-refs\"), errno,\n+\t\t\t\t\t       err);\n+\t\t\treturn -1;\n+\t\t}\n \t\tunable_to_lock_error(git_path(\"packed-refs\"), errno);\n \t\treturn error(\"cannot delete '%s' from packed refs\", refnames[i]);\n \t}\n@@ -2470,12 +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.g3833130\n"},{"id":"242951","messageId":"1401379676-9307-3-git-send-email-sahlberg@google.com","threadId":"36785","inReplyTo":"1401379676-9307-1-git-send-email-sahlberg@google.com","subject":"[PATCH v12 08/41] refs.c: add an err argument to delete_ref_loose","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-29T16:07:44Z","receivedAt":"2014-05-29T16:07:44Z","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 <sahlberg@google.com>\n---\n refs.c | 35 +++++++++++++++++++++++++++++------\n 1 file changed, 29 insertions(+), 6 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 9d079ec..d768a5e 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2491,16 +2491,38 @@ static int repack_without_ref(const char *refname)\n \treturn repack_without_refs(&refname, 1, NULL);\n }\n \n-static int delete_ref_loose(struct ref_lock *lock, int flag)\n+static int add_err_if_unremovable(const char *op, const char *file,\n+\t\t\t\t  struct strbuf *e, int rc)\n+{\n+\tint err = errno;\n+\tif (rc < 0  && errno != ENOENT) {\n+\t\tstrbuf_addf(e, \"unable to %s %s: %s\",\n+\t\t\t    op, file, strerror(errno));\n+\t\terrno = err;\n+\t\treturn -1;\n+\t}\n+\treturn 0;\n+}\n+\n+static int unlink_or_err(const char *file, struct strbuf *err)\n+{\n+\tif (err)\n+\t\treturn add_err_if_unremovable(\"unlink\", file, err,\n+\t\t\t\t\t      unlink(file));\n+\telse\n+\t\treturn unlink_or_warn(file);\n+}\n+\n+static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)\n {\n \tif (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {\n \t\t/* loose */\n-\t\tint err, i = strlen(lock->lk->filename) - 5; /* .lock */\n+\t\tint res, i = strlen(lock->lk->filename) - 5; /* .lock */\n \n \t\tlock->lk->filename[i] = 0;\n-\t\terr = unlink_or_warn(lock->lk->filename);\n+\t\tres = unlink_or_err(lock->lk->filename, err);\n \t\tlock->lk->filename[i] = '.';\n-\t\tif (err && errno != ENOENT)\n+\t\tif (res && errno != ENOENT)\n \t\t\treturn 1;\n \t}\n \treturn 0;\n@@ -2514,7 +2536,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 +3516,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.g3833130\n"},{"id":"242948","messageId":"1401379676-9307-4-git-send-email-sahlberg@google.com","threadId":"36785","inReplyTo":"1401379676-9307-1-git-send-email-sahlberg@google.com","subject":"[PATCH v12 16/41] refs.c: add transaction.status and track OPEN/CLOSED/ERROR","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-29T16:07:45Z","receivedAt":"2014-05-29T16:07:45Z","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 <sahlberg@google.com>\n---\n refs.c | 36 +++++++++++++++++++++++++++++++++++-\n 1 file changed, 35 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex b5186cd..00d8fc9 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3331,6 +3331,30 @@ struct ref_update {\n };\n \n /*\n+ * Transaction states.\n+ * OPEN:   The transaction is in a valid state and can accept new updates.\n+ *         An OPEN transaction can be committed.\n+ * CLOSED: If an open transaction is successfully committed the state will\n+ *         change to CLOSED. No further changes can be made to a CLOSED\n+ *         transaction.\n+ *         CLOSED means that all updates have been successfully committed and\n+ *         the only thing that remains is to free the completed transaction.\n+ * ERROR:  The transaction has failed and is no longer committable.\n+ *         Eventhough the transaction can no longer be committed it is still\n+ *         possible to add additional updates to the transaction. The reason\n+ *         for this is to allow a caller to continue trying more updates\n+ *         so that a caller can report a list of ALL updates that would fail\n+ *         instead of just the first update that fails.\n+ *         An ERRORed transaction can not be committed and must be rolled\n+ *         back using transaction_free.\n+ */\n+enum ref_transaction_state {\n+\tREF_TRANSACTION_OPEN   = 0,\n+\tREF_TRANSACTION_CLOSED = 1,\n+\tREF_TRANSACTION_ERROR  = 2,\n+};\n+\n+/*\n  * Data structure for holding a reference transaction, which can\n  * consist of checks and updates to multiple references, carried out\n  * as atomically as possible.  This structure is opaque to callers.\n@@ -3339,6 +3363,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@@ -3476,8 +3502,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@@ -3540,6 +3571,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.g3833130\n"},{"id":"242962","messageId":"1401379676-9307-5-git-send-email-sahlberg@google.com","threadId":"36785","inReplyTo":"1401379676-9307-1-git-send-email-sahlberg@google.com","subject":"[PATCH v12 24/41] receive-pack.c: use a reference transaction for updating the refs","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-29T16:07:46Z","receivedAt":"2014-05-29T16:07:46Z","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 <sahlberg@google.com>\n---\n builtin/receive-pack.c | 31 ++++++++++++++++++++++---------\n 1 file changed, 22 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex c323081..13f4a63 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -46,6 +46,7 @@ static void *head_name_to_free;\n static int sent_capabilities;\n static int shallow_update;\n static const char *alt_shallow_file;\n+static struct string_list error_strings = STRING_LIST_INIT_DUP;\n \n static enum deny_action parse_deny_action(const char *var, const char *value)\n {\n@@ -475,7 +476,6 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \tconst char *namespaced_name;\n \tunsigned char *old_sha1 = cmd->old_sha1;\n \tunsigned char *new_sha1 = cmd->new_sha1;\n-\tstruct ref_lock *lock;\n \n \t/* only refs/... are allowed */\n \tif (!starts_with(name, \"refs/\") || check_refname_format(name + 5, 0)) {\n@@ -576,19 +576,31 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\treturn NULL; /* good */\n \t}\n \telse {\n+\t\tstruct strbuf err = STRBUF_INIT;\n+\t\tstruct ref_transaction *transaction;\n+\n \t\tif (shallow_update && si->shallow_ref[cmd->index] &&\n \t\t    update_shallow_ref(cmd, si))\n \t\t\treturn \"shallow error\";\n \n-\t\tlock = lock_any_ref_for_update(namespaced_name, old_sha1,\n-\t\t\t\t\t       0, NULL);\n-\t\tif (!lock) {\n-\t\t\trp_error(\"failed to lock %s\", name);\n-\t\t\treturn \"failed to lock\";\n-\t\t}\n-\t\tif (write_ref_sha1(lock, new_sha1, \"push\")) {\n-\t\t\treturn \"failed to write\"; /* error() already called */\n+\t\ttransaction = ref_transaction_begin(&err);\n+\t\tif (!transaction ||\n+\t\t    ref_transaction_update(transaction, namespaced_name,\n+\t\t\t\t\t   new_sha1, old_sha1, 0, 1, &err) ||\n+\t\t    ref_transaction_commit(transaction, \"push\", &err)) {\n+\n+\t\t\tconst char *str;\n+\t\t\tstring_list_append(&error_strings, err.buf);\n+\t\t\tstr = error_strings.items[error_strings.nr - 1].string;\n+\t\t\tstrbuf_release(&err);\n+\n+\t\t\tref_transaction_free(transaction);\n+\t\t\trp_error(\"%s\", str);\n+\t\t\treturn str;\n \t\t}\n+\n+\t\tref_transaction_free(transaction);\n+\t\tstrbuf_release(&err);\n \t\treturn NULL; /* good */\n \t}\n }\n@@ -1215,5 +1227,6 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t\tpacket_flush(1);\n \tsha1_array_clear(&shallow);\n \tsha1_array_clear(&ref);\n+\tstring_list_clear(&error_strings, 0);\n \treturn 0;\n }\n-- \n2.0.0.rc3.474.g3833130\n"},{"id":"242952","messageId":"1401379676-9307-6-git-send-email-sahlberg@google.com","threadId":"36785","inReplyTo":"1401379676-9307-1-git-send-email-sahlberg@google.com","subject":"[PATCH v12 25/41] fast-import.c: use a ref transaction when dumping tags","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-29T16:07:47Z","receivedAt":"2014-05-29T16:07:47Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n fast-import.c | 29 +++++++++++++++++++++++------\n 1 file changed, 23 insertions(+), 6 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 4a7b196..587ef4a 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1735,15 +1735,32 @@ static void dump_tags(void)\n {\n \tstatic const char *msg = \"fast-import\";\n \tstruct tag *t;\n-\tstruct ref_lock *lock;\n-\tchar ref_name[PATH_MAX];\n+\tstruct strbuf ref_name = STRBUF_INIT;\n+\tstruct strbuf err = STRBUF_INIT;\n+\tstruct ref_transaction *transaction;\n \n+\ttransaction = ref_transaction_begin(&err);\n+\tif (!transaction) {\n+\t\tfailure |= error(\"%s\", err.buf);\n+\t\tgoto cleanup;\n+\t}\n \tfor (t = first_tag; t; t = t->next_tag) {\n-\t\tsprintf(ref_name, \"tags/%s\", t->name);\n-\t\tlock = lock_ref_sha1(ref_name, NULL);\n-\t\tif (!lock || write_ref_sha1(lock, t->sha1, msg) < 0)\n-\t\t\tfailure |= error(\"Unable to update %s\", ref_name);\n+\t\tstrbuf_reset(&ref_name);\n+\t\tstrbuf_addf(&ref_name, \"refs/tags/%s\", t->name);\n+\n+\t\tif (ref_transaction_update(transaction, ref_name.buf, t->sha1,\n+\t\t\t\t\t   NULL, 0, 0, &err)) {\n+\t\t\tfailure |= error(\"%s\", err.buf);\n+\t\t\tgoto cleanup;\n+\t\t}\n \t}\n+\tif (ref_transaction_commit(transaction, msg, &err))\n+\t\tfailure |= error(\"%s\", err.buf);\n+\n+ cleanup:\n+\tref_transaction_free(transaction);\n+\tstrbuf_release(&ref_name);\n+\tstrbuf_release(&err);\n }\n \n static void dump_marks_helper(FILE *f,\n-- \n2.0.0.rc3.474.g3833130\n"},{"id":"242961","messageId":"1401379676-9307-7-git-send-email-sahlberg@google.com","threadId":"36785","inReplyTo":"1401379676-9307-1-git-send-email-sahlberg@google.com","subject":"[PATCH v12 26/41] walker.c: use ref transaction for ref updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-29T16:07:48Z","receivedAt":"2014-05-29T16:07:48Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Switch to using ref transactions in walker_fetch(). As part of the refactoring\nto use ref transactions we also fix a potential memory leak where in the\noriginal code if write_ref_sha1() would fail we would end up returning from\nthe function without free()ing the msg string.\n\nNote that this function is only called when fetching from a remote HTTP\nrepository onto the local (most of the time single-user) repository which\nlikely means that the type of collissions that the previous locking would\nprotect against and cause the fetch to fail for to be even more rare.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n walker.c | 59 +++++++++++++++++++++++++++++++++++------------------------\n 1 file changed, 35 insertions(+), 24 deletions(-)\n\ndiff --git a/walker.c b/walker.c\nindex 1dd86b8..60d9f9e 100644\n--- a/walker.c\n+++ b/walker.c\n@@ -251,39 +251,36 @@ void walker_targets_free(int targets, char **target, const char **write_ref)\n int walker_fetch(struct walker *walker, int targets, char **target,\n \t\t const char **write_ref, const char *write_ref_log_details)\n {\n-\tstruct ref_lock **lock = xcalloc(targets, sizeof(struct ref_lock *));\n+\tstruct strbuf ref_name = STRBUF_INIT;\n+\tstruct strbuf err = STRBUF_INIT;\n+\tstruct ref_transaction *transaction = NULL;\n \tunsigned char *sha1 = xmalloc(targets * 20);\n-\tchar *msg;\n-\tint ret;\n+\tchar *msg = NULL;\n \tint i;\n \n \tsave_commit_buffer = 0;\n \n-\tfor (i = 0; i < targets; i++) {\n-\t\tif (!write_ref || !write_ref[i])\n-\t\t\tcontinue;\n-\n-\t\tlock[i] = lock_ref_sha1(write_ref[i], NULL);\n-\t\tif (!lock[i]) {\n-\t\t\terror(\"Can't lock ref %s\", write_ref[i]);\n-\t\t\tgoto unlock_and_fail;\n+\tif (write_ref) {\n+\t\ttransaction = ref_transaction_begin(&err);\n+\t\tif (!transaction) {\n+\t\t\terror(\"%s\", err.buf);\n+\t\t\tgoto rollback_and_fail;\n \t\t}\n \t}\n-\n \tif (!walker->get_recover)\n \t\tfor_each_ref(mark_complete, NULL);\n \n \tfor (i = 0; i < targets; i++) {\n \t\tif (interpret_target(walker, target[i], &sha1[20 * i])) {\n \t\t\terror(\"Could not interpret response from server '%s' as something to pull\", target[i]);\n-\t\t\tgoto unlock_and_fail;\n+\t\t\tgoto rollback_and_fail;\n \t\t}\n \t\tif (process(walker, lookup_unknown_object(&sha1[20 * i])))\n-\t\t\tgoto unlock_and_fail;\n+\t\t\tgoto rollback_and_fail;\n \t}\n \n \tif (loop(walker))\n-\t\tgoto unlock_and_fail;\n+\t\tgoto rollback_and_fail;\n \n \tif (write_ref_log_details) {\n \t\tmsg = xmalloc(strlen(write_ref_log_details) + 12);\n@@ -294,19 +291,33 @@ int walker_fetch(struct walker *walker, int targets, char **target,\n \tfor (i = 0; i < targets; i++) {\n \t\tif (!write_ref || !write_ref[i])\n \t\t\tcontinue;\n-\t\tret = write_ref_sha1(lock[i], &sha1[20 * i], msg ? msg : \"fetch (unknown)\");\n-\t\tlock[i] = NULL;\n-\t\tif (ret)\n-\t\t\tgoto unlock_and_fail;\n+\t\tstrbuf_reset(&ref_name);\n+\t\tstrbuf_addf(&ref_name, \"refs/%s\", write_ref[i]);\n+\t\tif (ref_transaction_update(transaction, ref_name.buf,\n+\t\t\t\t\t   &sha1[20 * i], NULL, 0, 0,\n+\t\t\t\t\t   &err)) {\n+\t\t\terror(\"%s\", err.buf);\n+\t\t\tgoto rollback_and_fail;\n+\t\t}\n+\t}\n+\tif (write_ref) {\n+\t\tif (ref_transaction_commit(transaction,\n+\t\t\t\t\t   msg ? msg : \"fetch (unknown)\",\n+\t\t\t\t\t   &err)) {\n+\t\t\terror(\"%s\", err.buf);\n+\t\t\tgoto rollback_and_fail;\n+\t\t}\n+\t\tref_transaction_free(transaction);\n \t}\n-\tfree(msg);\n \n+\tfree(msg);\n \treturn 0;\n \n-unlock_and_fail:\n-\tfor (i = 0; i < targets; i++)\n-\t\tif (lock[i])\n-\t\t\tunlock_ref(lock[i]);\n+rollback_and_fail:\n+\tref_transaction_free(transaction);\n+\tfree(msg);\n+\tstrbuf_release(&err);\n+\tstrbuf_release(&ref_name);\n \n \treturn -1;\n }\n-- \n2.0.0.rc3.474.g3833130\n"},{"id":"242949","messageId":"1401379676-9307-8-git-send-email-sahlberg@google.com","threadId":"36785","inReplyTo":"1401379676-9307-1-git-send-email-sahlberg@google.com","subject":"[PATCH v12 31/41] refs.c: make prune_ref use a transaction to delete the ref","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-29T16:07:49Z","receivedAt":"2014-05-29T16:07:49Z","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 <sahlberg@google.com>\n---\n refs.c | 28 +++++++++++++++++++++-------\n refs.h | 11 ++++++++++-\n 2 files changed, 31 insertions(+), 8 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 7cea694..60593d7 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@@ -3536,9 +3549,10 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\tstruct ref_update *update = updates[i];\n \n \t\tif (update->lock) {\n-\t\t\tdelnames[delnum++] = update->lock->ref_name;\n \t\t\tret |= delete_ref_loose(update->lock, update->type,\n \t\t\t\t\t\terr);\n+\t\t\tif (!(update->flags & REF_ISPRUNING))\n+\t\t\t\tdelnames[delnum++] = update->lock->ref_name;\n \t\t}\n \t}\n \ndiff --git a/refs.h b/refs.h\nindex c38ee09..dee7c8f 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -171,8 +171,17 @@ extern int ref_exists(const char *);\n  */\n extern int peel_ref(const char *refname, unsigned char *sha1);\n \n-/** Locks any ref (for 'HEAD' type refs). */\n+/*\n+ * Flags controlling lock_any_ref_for_update(), ref_transaction_update(),\n+ * ref_transaction_create(), etc.\n+ * REF_NODEREF: act on the ref directly, instead of dereferencing\n+ *              symbolic references.\n+ *\n+ * Flags >= 0x100 are reserved for internal use.\n+ */\n #define REF_NODEREF\t0x01\n+\n+/** Locks any ref (for 'HEAD' type refs). */\n extern struct ref_lock *lock_any_ref_for_update(const char *refname,\n \t\t\t\t\t\tconst unsigned char *old_sha1,\n \t\t\t\t\t\tint flags, int *type_p);\n-- \n2.0.0.rc3.474.g3833130\n"},{"id":"242960","messageId":"1401379676-9307-9-git-send-email-sahlberg@google.com","threadId":"36785","inReplyTo":"1401379676-9307-1-git-send-email-sahlberg@google.com","subject":"[PATCH v12 32/41] refs.c: make delete_ref use a transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-29T16:07:50Z","receivedAt":"2014-05-29T16:07:50Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change delete_ref to use a ref transaction for the deletion. At the same time\nsince we no longer have any callers of repack_without_ref we can now delete\nthis function.\n\nChange delete_ref to return 0 on success and -1 on failure instead of the\nprevious 0 on success either 1 or -1 on failure.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n refs.c | 36 ++++++++++++++----------------------\n 1 file changed, 14 insertions(+), 22 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 60593d7..e2c2249 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@@ -2538,24 +2533,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-\n-\tlock = lock_ref_sha1_basic(refname, sha1, delopt, &flag);\n-\tif (!lock)\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+\tstruct ref_transaction *transaction;\n+\tstruct strbuf err = STRBUF_INIT;\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+\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+\t}\n+\tref_transaction_free(transaction);\n+\treturn 0;\n }\n \n /*\n-- \n2.0.0.rc3.474.g3833130\n"},{"id":"242956","messageId":"1401379676-9307-10-git-send-email-sahlberg@google.com","threadId":"36785","inReplyTo":"1401379676-9307-1-git-send-email-sahlberg@google.com","subject":"[PATCH v12 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-29T16:07:51Z","receivedAt":"2014-05-29T16:07:51Z","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 <sahlberg@google.com>\n---\n branch.c               |  4 ++--\n builtin/commit.c       |  4 ++--\n builtin/fetch.c        |  3 +--\n builtin/receive-pack.c |  7 ++++---\n builtin/replace.c      |  4 ++--\n builtin/tag.c          |  4 ++--\n builtin/update-ref.c   | 13 +++++++------\n fast-import.c          |  8 ++++----\n refs.c                 | 34 +++++++++++++++++++++-------------\n refs.h                 |  8 ++++----\n sequencer.c            |  4 ++--\n walker.c               |  5 ++---\n 12 files changed, 53 insertions(+), 45 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex c1eae00..e0439af 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -301,8 +301,8 @@ void create_branch(const char *head,\n \t\ttransaction = ref_transaction_begin(&err);\n \t\tif (!transaction ||\n \t\t    ref_transaction_update(transaction, ref.buf, sha1,\n-\t\t\t\t\t   null_sha1, 0, !forcing, &err) ||\n-\t\t    ref_transaction_commit(transaction, msg, &err))\n+\t\t\t\t\t   null_sha1, 0, !forcing, msg, &err) ||\n+\t\t    ref_transaction_commit(transaction, &err))\n \t\t\tdie(\"%s\", err.buf);\n \t\tref_transaction_free(transaction);\n \t}\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 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 13f4a63..5653fa2 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -586,10 +586,11 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\ttransaction = ref_transaction_begin(&err);\n \t\tif (!transaction ||\n \t\t    ref_transaction_update(transaction, namespaced_name,\n-\t\t\t\t\t   new_sha1, old_sha1, 0, 1, &err) ||\n-\t\t    ref_transaction_commit(transaction, \"push\", &err)) {\n-\n+\t\t\t\t\t   new_sha1, old_sha1, 0, 1, \"push\",\n+\t\t\t\t\t   &err) ||\n+\t\t    ref_transaction_commit(transaction, &err)) {\n \t\t\tconst char *str;\n+\n \t\t\tstring_list_append(&error_strings, err.buf);\n \t\t\tstr = error_strings.items[error_strings.nr - 1].string;\n \t\t\tstrbuf_release(&err);\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex 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 587ef4a..7ca8b5a 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1709,8 +1709,8 @@ static int update_branch(struct branch *b)\n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction ||\n \t    ref_transaction_update(transaction, b->name, b->sha1, old_sha1,\n-\t\t\t\t   0, 1, &err) ||\n-\t    ref_transaction_commit(transaction, msg, &err)) {\n+\t\t\t\t   0, 1, msg, &err) ||\n+\t    ref_transaction_commit(transaction, &err)) {\n \t\tref_transaction_free(transaction);\n \t\terror(\"%s\", err.buf);\n \t\tstrbuf_release(&err);\n@@ -1749,12 +1749,12 @@ static void dump_tags(void)\n \t\tstrbuf_addf(&ref_name, \"refs/tags/%s\", t->name);\n \n \t\tif (ref_transaction_update(transaction, ref_name.buf, t->sha1,\n-\t\t\t\t\t   NULL, 0, 0, &err)) {\n+\t\t\t\t\t   NULL, 0, 0, msg, &err)) {\n \t\t\tfailure |= error(\"%s\", err.buf);\n \t\t\tgoto cleanup;\n \t\t}\n \t}\n-\tif (ref_transaction_commit(transaction, msg, &err))\n+\tif (ref_transaction_commit(transaction, &err))\n \t\tfailure |= error(\"%s\", err.buf);\n \n  cleanup:\ndiff --git a/refs.c b/refs.c\nindex 696bc4f..9c19267 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@@ -2539,8 +2539,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@@ -3290,6 +3290,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@@ -3342,9 +3343,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@@ -3365,7 +3367,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@@ -3379,13 +3381,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@@ -3399,13 +3403,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@@ -3420,6 +3426,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@@ -3433,8 +3441,8 @@ int update_ref(const char *action, const char *refname,\n \tt = ref_transaction_begin(&err);\n \tif (!t ||\n \t    ref_transaction_update(t, refname, sha1, oldval, flags,\n-\t\t\t\t   !!oldval, &err) ||\n-\t    ref_transaction_commit(t, action, &err)) {\n+\t\t\t\t   !!oldval, action, &err) ||\n+\t    ref_transaction_commit(t, &err)) {\n \t\tconst char *str = \"update_ref failed for ref '%s': %s\";\n \n \t\tref_transaction_free(t);\n@@ -3475,7 +3483,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@@ -3524,7 +3532,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \n \t\tif (!is_null_sha1(update->new_sha1)) {\n \t\t\tret = write_ref_sha1(update->lock, update->new_sha1,\n-\t\t\t\t\t     msg);\n+\t\t\t\t\t     update->msg);\n \t\t\tupdate->lock = NULL; /* freed by write_ref_sha1 */\n \t\t\tif (ret) {\n \t\t\t\tconst char *str = \"Cannot update the ref '%s'.\";\ndiff --git a/refs.h b/refs.h\nindex dee7c8f..5c6c20f 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -287,7 +287,7 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \t\t\t   const char *refname,\n \t\t\t   const unsigned char *new_sha1,\n \t\t\t   const unsigned char *old_sha1,\n-\t\t\t   int flags, int have_old,\n+\t\t\t   int flags, int have_old, const char *msg,\n \t\t\t   struct strbuf *err);\n \n /*\n@@ -302,7 +302,7 @@ int ref_transaction_update(struct ref_transaction *transaction,\n int ref_transaction_create(struct ref_transaction *transaction,\n \t\t\t   const char *refname,\n \t\t\t   const unsigned char *new_sha1,\n-\t\t\t   int flags,\n+\t\t\t   int flags, const char *msg,\n \t\t\t   struct strbuf *err);\n \n /*\n@@ -316,7 +316,7 @@ int ref_transaction_create(struct ref_transaction *transaction,\n int ref_transaction_delete(struct ref_transaction *transaction,\n \t\t\t   const char *refname,\n \t\t\t   const unsigned char *old_sha1,\n-\t\t\t   int flags, int have_old,\n+\t\t\t   int flags, int have_old, const char *msg,\n \t\t\t   struct strbuf *err);\n \n /*\n@@ -325,7 +325,7 @@ int ref_transaction_delete(struct ref_transaction *transaction,\n  * problem.\n  */\n int ref_transaction_commit(struct ref_transaction *transaction,\n-\t\t\t   const char *msg, struct strbuf *err);\n+\t\t\t   struct strbuf *err);\n \n /*\n  * Free an existing transaction and all associated data.\ndiff --git a/sequencer.c b/sequencer.c\nindex fd8acaf..f9906ef 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -285,8 +285,8 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction ||\n \t    ref_transaction_update(transaction, \"HEAD\", to, from,\n-\t\t\t\t   0, !unborn, &err) ||\n-\t    ref_transaction_commit(transaction, sb.buf, &err)) {\n+\t\t\t\t   0, !unborn, sb.buf, &err) ||\n+\t    ref_transaction_commit(transaction, &err)) {\n \t\tref_transaction_free(transaction);\n \t\terror(\"%s\", err.buf);\n \t\tstrbuf_release(&sb);\ndiff --git a/walker.c b/walker.c\nindex 60d9f9e..fd9ef87 100644\n--- a/walker.c\n+++ b/walker.c\n@@ -295,15 +295,14 @@ int walker_fetch(struct walker *walker, int targets, char **target,\n \t\tstrbuf_addf(&ref_name, \"refs/%s\", write_ref[i]);\n \t\tif (ref_transaction_update(transaction, ref_name.buf,\n \t\t\t\t\t   &sha1[20 * i], NULL, 0, 0,\n+\t\t\t\t\t   msg ? msg : \"fetch (unknown)\",\n \t\t\t\t\t   &err)) {\n \t\t\terror(\"%s\", err.buf);\n \t\t\tgoto rollback_and_fail;\n \t\t}\n \t}\n \tif (write_ref) {\n-\t\tif (ref_transaction_commit(transaction,\n-\t\t\t\t\t   msg ? msg : \"fetch (unknown)\",\n-\t\t\t\t\t   &err)) {\n+\t\tif (ref_transaction_commit(transaction, &err)) {\n \t\t\terror(\"%s\", err.buf);\n \t\t\tgoto rollback_and_fail;\n \t\t}\n-- \n2.0.0.rc3.474.g3833130\n"},{"id":"242953","messageId":"1401379676-9307-11-git-send-email-sahlberg@google.com","threadId":"36785","inReplyTo":"1401379676-9307-1-git-send-email-sahlberg@google.com","subject":"[PATCH v12 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-29T16:07:52Z","receivedAt":"2014-05-29T16:07:52Z","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 <sahlberg@google.com>\n---\n refs.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 5680028..df00993 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.g3833130\n"},{"id":"242954","messageId":"1401379676-9307-12-git-send-email-sahlberg@google.com","threadId":"36785","inReplyTo":"1401379676-9307-1-git-send-email-sahlberg@google.com","subject":"[PATCH v12 38/41] refs.c: pass a skip list to name_conflict_fn","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-29T16:07:53Z","receivedAt":"2014-05-29T16:07:53Z","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 <sahlberg@google.com>\n---\n refs.c | 36 ++++++++++++++++++++++++++----------\n 1 file changed, 26 insertions(+), 10 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex edf73b7..ec8d642 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@@ -2624,10 +2637,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@@ -2660,7 +2675,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@@ -2675,7 +2690,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@@ -3520,7 +3535,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.g3833130\n"},{"id":"242955","messageId":"1401379676-9307-13-git-send-email-sahlberg@google.com","threadId":"36785","inReplyTo":"1401379676-9307-1-git-send-email-sahlberg@google.com","subject":"[PATCH v12 39/41] refs.c: propagate any errno==ENOTDIR from _commit back to the callers","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-29T16:07:54Z","receivedAt":"2014-05-29T16:07:54Z","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 <sahlberg@google.com>\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 ec8d642..6414afc 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3504,7 +3504,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@@ -3522,9 +3522,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@@ -3538,10 +3539,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@@ -3559,6 +3562,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@@ -3569,14 +3573,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@@ -3589,6 +3595,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 5c6c20f..88732a1 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -323,6 +323,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.g3833130\n"},{"id":"242959","messageId":"1401379676-9307-14-git-send-email-sahlberg@google.com","threadId":"36785","inReplyTo":"1401379676-9307-1-git-send-email-sahlberg@google.com","subject":"[PATCH v12 40/41] fetch.c: change s_update_ref to use a ref transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-29T16:07:55Z","receivedAt":"2014-05-29T16:07:55Z","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 <sahlberg@google.com>\n---\n builtin/fetch.c | 21 +++++++++++++--------\n 1 file changed, 13 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex faa1233..b13e8f9 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -375,22 +375,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+\tstruct strbuf err = STRBUF_INIT;\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+\tref_transaction_free(transaction);\n \treturn 0;\n }\n \n-- \n2.0.0.rc3.474.g3833130\n"},{"id":"242957","messageId":"1401379676-9307-15-git-send-email-sahlberg@google.com","threadId":"36785","inReplyTo":"1401379676-9307-1-git-send-email-sahlberg@google.com","subject":"[PATCH v12 41/41] refs.c: make write_ref_sha1 static","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-29T16:07:56Z","receivedAt":"2014-05-29T16:07:56Z","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 <sahlberg@google.com>\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 6414afc..aefa5bc 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2618,6 +2618,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@@ -2848,7 +2851,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 88732a1..88020c5 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -195,9 +195,6 @@ extern int commit_ref(struct ref_lock *lock);\n /** Release any lock taken but not written. **/\n extern void unlock_ref(struct ref_lock *lock);\n \n-/** Writes sha1 into the ref specified by the lock. **/\n-extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);\n-\n /** Setup reflog before using. **/\n int log_ref_setup(const char *refname, char *logfile, int bufsize);\n \n-- \n2.0.0.rc3.474.g3833130\n"},{"id":"242964","messageId":"CAL=YDWnProozg=5ven5Xq5JJ=twWsm834pupa=CrYeLpJPzjkQ@mail.gmail.com","threadId":"36785","inReplyTo":"1401379676-9307-1-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v12 00/44] Use ref transactions for all ref updates","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-29T16:13:11Z","receivedAt":"2014-05-29T16:13:11Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"These patches are the remaining patches that need review in this series.\nPlease review them.\n\nOn Thu, May 29, 2014 at 9:07 AM, Ronnie Sahlberg <sahlberg@google.com> wrote:\n> This patch series can also be found at\n> https://github.com/rsahlberg/git/tree/ref-transactions\n>\n>\n> Ronnie please review these remaining patches in this series.\n>\n>\n>  Sahlberg (44):\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: ref_transaction_delete to check for error and return status\n>   tag.c: use ref transactions when doing updates\n>   replace.c: use the ref transaction functions for updates\n>   commit.c: use ref transactions for updates\n>   sequencer.c: use ref transactions for all ref updates\n>   fast-import.c: change update_branch to use ref transactions\n>   branch.c: use ref transaction for all ref updates\n>   refs.c: change update_ref to use a transaction\n>   refs.c: free the transaction before returning when number of updates\n>     is 0\n>   refs.c: ref_transaction_commit should not free the transaction\n>   fetch.c: clear errno before calling functions that might set it\n>   fetch.c: change s_update_ref to use a ref transaction\n>   fetch.c: use a single ref transaction for all ref updates\n>   receive-pack.c: use a reference transaction for updating the refs\n>   fast-import.c: use a ref transaction when dumping tags\n>   walker.c: use ref transaction for ref updates\n>   refs.c: make write_ref_sha1 static\n>   refs.c: make lock_ref_sha1 static\n>   refs.c: add transaction.status and track OPEN/CLOSED/ERROR\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: add a new flag for transaction delete for refs we know are\n>     packed only\n>   refs.c: pass a skip list to name_conflict_fn\n>   refs.c: make rename_ref use a transaction\n>   refs.c: remove forward declaration of write_ref_sha1\n>\n>  branch.c               |  30 ++--\n>  builtin/commit.c       |  24 ++-\n>  builtin/fetch.c        |  29 +--\n>  builtin/receive-pack.c |  21 +--\n>  builtin/replace.c      |  15 +-\n>  builtin/tag.c          |  15 +-\n>  builtin/update-ref.c   |  32 ++--\n>  cache.h                |   2 +\n>  fast-import.c          |  42 +++--\n>  lockfile.c             |  21 ++-\n>  refs.c                 | 468 +++++++++++++++++++++++++++++--------------------\n>  refs.h                 |  54 +++---\n>  sequencer.c            |  24 ++-\n>  t/t3200-branch.sh      |   2 +-\n>  walker.c               |  51 +++---\n>  15 files changed, 491 insertions(+), 339 deletions(-)\n>\n> --\n> 2.0.0.rc3.510.g20c254b\n>\n"},{"id":"242969","messageId":"20140529181732.GF12314@google.com","threadId":"36785","inReplyTo":"1401379676-9307-2-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v12 06/41] refs.c: add an err argument to repack_without_refs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-29T18:17:32Z","receivedAt":"2014-05-29T18:17:32Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nRonnie Sahlberg wrote:\n\n> Update repack_without_refs to take an err argument and update it if there\n> is a failure. Pass the err variable from ref_transaction_commit to this\n> function so that callers can print a meaningful error message if _commit\n> fails due to a problem in repack_without_refs.\n>\n> Add a new function unable_to_lock_message that takes a strbuf argument and\n> fills in the reason for the failure.\n>\n> In commit_packed_refs, make sure that we propagate any errno that\n> commit_lock_file might have set back to our caller.\n>\n> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n> ---\n>  cache.h    |  2 ++\n>  lockfile.c | 21 ++++++++++++---------\n>  refs.c     | 25 +++++++++++++++++++------\n>  3 files changed, 33 insertions(+), 15 deletions(-)\n\nI don't want to sound like a broken record, but this still has the\nsame issues I described before at\nhttp://thread.gmane.org/gmane.comp.version-control.git/250197/focus=250309.\n\nThe commit message or documentation or notes after the three dashes\nabove could explain what I missed when making my suggestions.\nOtherwise I get no reality check as a reviewer, other reviewers get\nless insight into what's happening in the patch, people in the future\nlooking into the patch don't understand its design as well, etc.\n\nAs a general rule, that is a good practice anyway --- even when a\nreviewer was confused, what they got confused about can be an\nindication of where to make the code or design documentation (commit\nmessage) more clear, and when reposting a patch it can be a good\nopportunity to explain how the patch evolved.\n\nWhat would be wrong with the line of API documentation and the TODO\ncomment for a known bug I suggested?  If they are a bad idea, can you\nexplain that so I can learn from it?  Or if they were confusing, would\nyou like a patch that explains what I mean?\n\nJonathan\n"},{"id":"243206","messageId":"CAL=YDW=WvBH3TEzvgaFd=FzbQPcQZOTxDZw9U-5wcv5e2RDFfA@mail.gmail.com","threadId":"36785","inReplyTo":"20140529181732.GF12314@google.com","subject":"Re: [PATCH v12 06/41] refs.c: add an err argument to repack_without_refs","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-03T20:55:02Z","receivedAt":"2014-06-03T20:55:02Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Thanks.\n\nI have done all the additions of save_errno you suggested.\n\n\nOn Thu, May 29, 2014 at 11:17 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Hi,\n>\n> Ronnie Sahlberg wrote:\n>\n>> Update repack_without_refs to take an err argument and update it if there\n>> is a failure. Pass the err variable from ref_transaction_commit to this\n>> function so that callers can print a meaningful error message if _commit\n>> fails due to a problem in repack_without_refs.\n>>\n>> Add a new function unable_to_lock_message that takes a strbuf argument and\n>> fills in the reason for the failure.\n>>\n>> In commit_packed_refs, make sure that we propagate any errno that\n>> commit_lock_file might have set back to our caller.\n>>\n>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n>> ---\n>>  cache.h    |  2 ++\n>>  lockfile.c | 21 ++++++++++++---------\n>>  refs.c     | 25 +++++++++++++++++++------\n>>  3 files changed, 33 insertions(+), 15 deletions(-)\n>\n> I don't want to sound like a broken record, but this still has the\n> same issues I described before at\n> http://thread.gmane.org/gmane.comp.version-control.git/250197/focus=250309.\n>\n> The commit message or documentation or notes after the three dashes\n> above could explain what I missed when making my suggestions.\n> Otherwise I get no reality check as a reviewer, other reviewers get\n> less insight into what's happening in the patch, people in the future\n> looking into the patch don't understand its design as well, etc.\n>\n> As a general rule, that is a good practice anyway --- even when a\n> reviewer was confused, what they got confused about can be an\n> indication of where to make the code or design documentation (commit\n> message) more clear, and when reposting a patch it can be a good\n> opportunity to explain how the patch evolved.\n>\n> What would be wrong with the line of API documentation and the TODO\n> comment for a known bug I suggested?  If they are a bad idea, can you\n> explain that so I can learn from it?  Or if they were confusing, would\n> you like a patch that explains what I mean?\n>\n> Jonathan\n"}]}