{"thread":{"id":"37776","subject":"[PATCH 03/15] refs.c: use packed refs when deleting refs during a transaction","startedAt":"2014-10-21T20:36:45Z","lastAt":"2014-11-11T15:42:53Z","messageCount":27,"participants":["Ronnie Sahlberg","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":15},"messages":[{"id":"250926","messageId":"1413923820-14457-1-git-send-email-sahlberg@google.com","threadId":"37776","inReplyTo":null,"subject":"[PATCH 00/15] ref-transaction-rename","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-10-21T20:36:45Z","receivedAt":"2014-10-21T20:36:45Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"List,\n\nThsi series builds on the previous series : ref-transaction-reflog\nas applied to next. This series has been sent to the list before\nbut is now rebased to current git next.\n\nThis series can also be found at :\nhttps://github.com/rsahlberg/git/tree/ref-transactions-rename\n\nThis series converts ref rename to use a transaction. This addesses several\nissues in the old implementation, such as colliding renames might overwrite\nsomeone elses reflog, and it makes the rename atomic.\n\nAs part of the series we also move changes that cover multiple refs to happen\nas an atomic transaction/rename to the pacekd refs file. This makes it possible\nto have both the rename case (one deleted ref + one created ref) as well\nas any operation that updates multiple refs to become one atomic rename()\napplied to the packed refs file. Thus all such changes are now also atomic\nto all external observers.\n\n\nRonnie Sahlberg (15):\n refs.c: allow passing raw git_committer_info as email to\n _update_reflog\n refs.c: return error instead of dying when locking fails during\n transaction\n refs.c: use packed refs when deleting refs during a transaction\n refs.c: use a stringlist for repack_without_refs\n refs.c: update rename_ref to use a transaction\n refs.c: rollback the lockfile before we die() in repack_without_refs\n refs.c: move reflog updates into its own function\n refs.c: write updates to packed refs when a transaction has more than\n one ref\n remote.c: use a transaction for deleting refs\n refs.c: make repack_without_refs static\n refs.c: make the *_packed_refs functions static\n refs.c: replace the onerr argument in update_ref with a strbuf err\n refs.c: make add_packed_ref return an error instead of calling die\n refs.c: make lock_packed_refs take an err argument\n refs.c: add an err argument to pack_refs\n\n builtin/checkout.c | 7 +-\n builtin/clone.c | 36 ++--\n builtin/merge.c | 20 +-\n builtin/notes.c | 24 ++-\n builtin/pack-refs.c | 8 +-\n builtin/reflog.c | 19 +-\n builtin/remote.c | 69 +++---\n builtin/reset.c | 12 +-\n builtin/update-ref.c | 7 +-\n notes-cache.c | 2 +-\n notes-utils.c | 5 +-\n refs.c | 568 +++++++++++++++++++++++++++++---------------------\n refs.h | 71 +++----\n t/t3200-branch.sh | 7 -\n t/t5516-fetch-push.sh | 2 +-\n transport-helper.c | 7 +-\n transport.c | 9 +-\n 17 files changed, 500 insertions(+), 373 deletions(-)\n\n-- \n2.1.0.rc2.206.gedb03e5\n"},{"id":"250928","messageId":"1413923820-14457-2-git-send-email-sahlberg@google.com","threadId":"37776","inReplyTo":"1413923820-14457-1-git-send-email-sahlberg@google.com","subject":"[PATCH 01/15] refs.c: allow passing raw git_committer_info as email to _update_reflog","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-10-21T20:36:46Z","receivedAt":"2014-10-21T20:36:46Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"commit 167e0c6d205ed785cc17b96e22d9366accfa1665 upstream.\n\nIn many places in the code we do not have access to the individual fields\nin the committer data. Instead we might only have access to prebaked data\nsuch as what is returned by git_committer_info() containing a string\nthat consists of email, timestamp, zone etc.\n\nThis makes it inconvenient to use transaction_update_reflog since it means\nyou would have to first parse git_committer_info before you can call\nupdate_reflog.\n\nAdd a new flag REFLOG_COMMITTER_INFO_IS_VALID to _update_reflog to tell it\nthat we pass in a fully prebaked committer info string that can be used as is.\n\nAt the same time, also go over and change all references from email\nto id where the code actually refers to a committer id and not just an email\naddress. I.e. where the string is : NAME \n\nChange-Id: I179cd8cc88570801ce6ef1daa8f738e7d20851fa\nSigned-off-by: Ronnie Sahlberg \nSigned-off-by: Jonathan Nieder \n---\n builtin/reflog.c | 19 +++++++++++++------\n refs.c | 21 ++++++++++++---------\n refs.h | 25 +++++++++++++++++++++++--\n 3 files changed, 48 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 6bb7454..be88a53 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -292,7 +292,7 @@ static int unreachable(struct expire_reflog_cb *cb, struct commit *commit, unsig\n }\n \n static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n-\t\tconst char *email, unsigned long timestamp, int tz,\n+\t\tconst char *id, unsigned long timestamp, int tz,\n \t\tconst char *message, void *cb_data)\n {\n \tstruct expire_reflog_cb *cb = cb_data;\n@@ -320,9 +320,14 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n \t\tgoto prune;\n \n \tif (cb->t) {\n+\t\tstruct reflog_committer_info ci;\n+\n+\t\tmemset(&ci, 0, sizeof(ci));\n+\t\tci.id = id;\n+\t\tci.timestamp = timestamp;\n+\t\tci.tz = tz;\n \t\tif (transaction_update_reflog(cb->t, cb->refname, nsha1, osha1,\n-\t\t\t\t\t email, timestamp, tz, message, 0,\n-\t\t\t\t\t &err))\n+\t\t\t\t\t &ci, message, 0, &err))\n \t\t\treturn -1;\n \t\thashcpy(cb->last_kept_sha1, nsha1);\n \t}\n@@ -356,6 +361,7 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused,\n \tstruct expire_reflog_cb cb;\n \tstruct commit *tip_commit;\n \tstruct commit_list *tips;\n+\tstruct reflog_committer_info ci;\n \tint status = 0;\n \n \tmemset(&cb, 0, sizeof(cb));\n@@ -368,9 +374,10 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused,\n \t\tstatus |= error(\"%s\", err.buf);\n \t\tgoto cleanup;\n \t}\n+\n+\tmemset(&ci, 0, sizeof(ci));\n \tif (transaction_update_reflog(cb.t, cb.refname, null_sha1, null_sha1,\n-\t\t\t\t NULL, 0, 0, NULL, REFLOG_TRUNCATE,\n-\t\t\t\t &err)) {\n+\t\t\t\t &ci, NULL, REFLOG_TRUNCATE, &err)) {\n \t\tstatus |= error(\"%s\", err.buf);\n \t\tgoto cleanup;\n \t}\n@@ -672,7 +679,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n }\n \n static int count_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n-\t\tconst char *email, unsigned long timestamp, int tz,\n+\t\tconst char *id, unsigned long timestamp, int tz,\n \t\tconst char *message, void *cb_data)\n {\n \tstruct cmd_reflog_expire_cb *cb = cb_data;\ndiff --git a/refs.c b/refs.c\nindex ae29d11..e49fbe9 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3226,7 +3226,7 @@ struct read_ref_at_cb {\n };\n \n static int read_ref_at_ent(unsigned char *osha1, unsigned char *nsha1,\n-\t\tconst char *email, unsigned long timestamp, int tz,\n+\t\tconst char *id, unsigned long timestamp, int tz,\n \t\tconst char *message, void *cb_data)\n {\n \tstruct read_ref_at_cb *cb = cb_data;\n@@ -3273,7 +3273,7 @@ static int read_ref_at_ent(unsigned char *osha1, unsigned char *nsha1,\n }\n \n static int read_ref_at_ent_oldest(unsigned char *osha1, unsigned char *nsha1,\n-\t\t\t\t const char *email, unsigned long timestamp,\n+\t\t\t\t const char *id, unsigned long timestamp,\n \t\t\t\t int tz, const char *message, void *cb_data)\n {\n \tstruct read_ref_at_cb *cb = cb_data;\n@@ -3623,8 +3623,7 @@ int transaction_update_reflog(struct 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 const char *email,\n-\t\t\t unsigned long timestamp, int tz,\n+\t\t\t struct reflog_committer_info *ci,\n \t\t\t const char *msg, int flags,\n \t\t\t struct strbuf *err)\n {\n@@ -3652,13 +3651,17 @@ int transaction_update_reflog(struct transaction *transaction,\n \thashcpy(update->new_sha1, new_sha1);\n \thashcpy(update->old_sha1, old_sha1);\n \tupdate->reflog_fd = -1;\n-\tif (email) {\n+\tif (flags & REFLOG_COMMITTER_INFO_IS_VALID) {\n+\t\tif (!ci->committer_info)\n+\t\t\tdie(\"BUG: committer_info is NULL in reflog update\");\n+\t\tupdate->committer = xstrdup(ci->committer_info);\n+\t} else if (ci->id) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n-\t\tchar sign = (tz < 0) ? '-' : '+';\n-\t\tint zone = (tz < 0) ? (-tz) : tz;\n+\t\tchar sign = (ci->tz < 0) ? '-' : '+';\n+\t\tint zone = (ci->tz < 0) ? (-ci->tz) : ci->tz;\n \n-\t\tstrbuf_addf(&buf, \"%s %lu %c%04d\", email, timestamp, sign,\n-\t\t\t zone);\n+\t\tstrbuf_addf(&buf, \"%s %lu %c%04d\", ci->id,\n+\t\t\t ci->timestamp, sign, zone);\n \t\tupdate->committer = xstrdup(buf.buf);\n \t\tstrbuf_release(&buf);\n \t}\ndiff --git a/refs.h b/refs.h\nindex 2e97f4f..9153e1d 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -318,6 +318,28 @@ int transaction_delete_ref(struct transaction *transaction,\n * Flags >= 0x100 are reserved for internal use.\n */\n #define REFLOG_TRUNCATE 0x01\n+#define REFLOG_COMMITTER_INFO_IS_VALID 0x02\n+\n+/*\n+ * Committer data provided to reflog updates.\n+ * If flags contain REFLOG_COMMITTER_DATA_IS_VALID then the structure\n+ * contains a prebaked committer string just like git_committer_info()\n+ * would return.\n+ *\n+ * If flags does not contain REFLOG_COMMITTER_DATA_IS_VALID\n+ * then the committer info string will be generated using the passed\n+ * email, timestamp and tz fields.\n+ * This is useful for example from reflog iterators where you are passed\n+ * these fields individually and not as a prebaked git_committer_info()\n+ * string.\n+ */\n+struct reflog_committer_info {\n+\tconst char *committer_info;\n+\n+\tconst char *id;\n+\tunsigned long timestamp;\n+\tint tz;\n+};\n /*\n * Append a reflog entry for refname. If the REFLOG_TRUNCATE flag is set\n * this update will first truncate the reflog before writing the entry.\n@@ -327,8 +349,7 @@ int transaction_update_reflog(struct 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 const char *email,\n-\t\t\t unsigned long timestamp, int tz,\n+\t\t\t struct reflog_committer_info *ci,\n \t\t\t const char *msg, int flags,\n \t\t\t struct strbuf *err);\n \n-- \n2.1.0.rc2.206.gedb03e5\n"},{"id":"250925","messageId":"1413923820-14457-3-git-send-email-sahlberg@google.com","threadId":"37776","inReplyTo":"1413923820-14457-1-git-send-email-sahlberg@google.com","subject":"[PATCH 02/15] refs.c: return error instead of dying when locking fails during transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-10-21T20:36:47Z","receivedAt":"2014-10-21T20:36:47Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"commit e193c10fc4f9274d1e751cfcdcc4507818e8d498 upstream.\n\nChange lock_ref_sha1_basic to return an error instead of dying when\nwe fail to lock a file during a transaction.\nThis function is only called from transaction_commit() and it knows how\nto handle these failures.\n\nChange-Id: Ie830b7970412d9299e76a86f08633ce721130aed\nSigned-off-by: Ronnie Sahlberg \nSigned-off-by: Jonathan Nieder \n---\n refs.c | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex e49fbe9..c088d36 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2340,6 +2340,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \n \tlock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);\n \tif (lock->lock_fd < 0) {\n+\t\tlast_errno = errno;\n \t\tif (errno == ENOENT && --attempts_remaining > 0)\n \t\t\t/*\n \t\t\t * Maybe somebody just deleted one of the\n@@ -2347,8 +2348,13 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\t * again:\n \t\t\t */\n \t\t\tgoto retry;\n-\t\telse\n-\t\t\tunable_to_lock_die(ref_file, errno);\n+\t\telse {\n+\t\t\tstruct strbuf err = STRBUF_INIT;\n+\t\t\tunable_to_lock_message(ref_file, errno, &err);\n+\t\t\terror(\"%s\", err.buf);\n+\t\t\tstrbuf_reset(&err);\n+\t\t\tgoto error_return;\n+\t\t}\n \t}\n \treturn old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;\n \n-- \n2.1.0.rc2.206.gedb03e5\n"},{"id":"250924","messageId":"1413923820-14457-4-git-send-email-sahlberg@google.com","threadId":"37776","inReplyTo":"1413923820-14457-1-git-send-email-sahlberg@google.com","subject":"[PATCH 03/15] refs.c: use packed refs when deleting refs during a transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-10-21T20:36:48Z","receivedAt":"2014-10-21T20:36:48Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"commit fb5fa1d338ce113b0fea3bb955b50bbb3e827805 upstream.\n\nMake the deletion of refs during a transaction more atomic.\nStart by first copying all loose refs we will be deleting to the packed\nrefs file and then commit the packed refs file. Then re-lock the packed refs\nfile to stop anyone else from modifying these refs and keep it locked until\nwe are finished.\nSince all refs we are about to delete are now safely held in the packed refs\nfile we can proceed to immediately unlink any corresponding loose refs\nand still be fully rollback-able.\n\nThe exception is for refs that can not be resolved. Those refs are never\nadded to the packed refs and will just be un-rollback-ably deleted during\ncommit.\n\nBy deleting all the loose refs at the start of the transaction we make make\nit possible to both delete one ref and then re-create a different ref in\nthe same transaction even if the two refs would normally conflict.\n\nExample: rename m->m/m\n\nIn that example we want to delete the file 'm' so that we make room so\nthat we can create a directory with the same name in order to lock and\nwrite to the ref m/m and its lock-file m/m.lock.\n\nIf there is a failure during the commit phase we can rollback without losing\nany refs since we have so far only deleted loose refs that that are\nguaranteed to also have a corresponding entry in the packed refs file.\nOnce we have finished all updates for refs and their reflogs we can repack\nthe packed refs file and remove the to-be-deleted refs from the packed refs,\nat which point all the deleted refs will disappear in one atomic rename\noperation.\n\nThis also means that for an outside observer, deletion of multiple refs\nin a single transaction will look atomic instead of one ref being deleted\nat a time.\n\nIn order to do all this we need to change the semantics for the\nrepack_without_refs function so that we can lock the packed refs file,\ndo other stuff, and later be able to call repack_without_refs with the\nlock already taken.\nThis means we need some additional changes in remote.c to reflect the\nchanges to the repack_without_refs semantics.\n\nChange-Id: I1e4f58050acaabc6bcfa3409fbc0c2b866bb59aa\nSigned-off-by: Ronnie Sahlberg \nSigned-off-by: Jonathan Nieder \n---\n builtin/remote.c | 20 ++++++-\n refs.c | 155 ++++++++++++++++++++++++++++++++++++++++++-------------\n 2 files changed, 138 insertions(+), 37 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 7f28f92..c25420f 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -1,4 +1,5 @@\n #include \"builtin.h\"\n+#include \"lockfile.h\"\n #include \"parse-options.h\"\n #include \"transport.h\"\n #include \"remote.h\"\n@@ -753,6 +754,15 @@ static int remove_branches(struct string_list *branches)\n \tconst char **branch_names;\n \tint i, result = 0;\n \n+\tif (lock_packed_refs(0)) {\n+\t\tstruct strbuf err = STRBUF_INIT;\n+\n+\t\tunable_to_lock_message(git_path(\"packed-refs\"), errno, &err);\n+\t\terror(\"%s\", err.buf);\n+\t\tstrbuf_release(&err);\n+\t\treturn -1;\n+\t}\n+\n \tbranch_names = xmalloc(branches->nr * sizeof(*branch_names));\n \tfor (i = 0; i < branches->nr; i++)\n \t\tbranch_names[i] = branches->items[i].string;\n@@ -1337,9 +1347,15 @@ static int prune_remote(const char *remote, int dry_run)\n \t\t\tdelete_refs[i] = states.stale.items[i].util;\n \t\tif (!dry_run) {\n \t\t\tstruct strbuf err = STRBUF_INIT;\n-\t\t\tif (repack_without_refs(delete_refs, states.stale.nr,\n-\t\t\t\t\t\t&err))\n+\n+\t\t\tif (lock_packed_refs(0)) {\n+\t\t\t\tunable_to_lock_message(git_path(\"packed-refs\"),\n+\t\t\t\t\t\t errno, &err);\n \t\t\t\tresult |= error(\"%s\", err.buf);\n+\t\t\t} else\n+\t\t\t\tif (repack_without_refs(delete_refs,\n+\t\t\t\t\t\t\tstates.stale.nr, &err))\n+\t\t\t\t\tresult |= error(\"%s\", err.buf);\n \t\t\tstrbuf_release(&err);\n \t\t}\n \t\tfree(delete_refs);\ndiff --git a/refs.c b/refs.c\nindex c088d36..662af9b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2660,6 +2660,9 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)\n \treturn 0;\n }\n \n+/*\n+ * Must be called with packed refs already locked (and sorted)\n+ */\n int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n {\n \tstruct ref_dir *packed;\n@@ -2674,14 +2677,12 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n \t\tif (get_packed_ref(refnames[i]))\n \t\t\tbreak;\n \n-\t/* Avoid locking if we have nothing to do */\n-\tif (i == n)\n+\t/* Avoid processing if we have nothing to do */\n+\tif (i == n) {\n+\t\trollback_packed_refs();\n \t\treturn 0; /* no refname exists in packed refs */\n-\n-\tif (lock_packed_refs(0)) {\n-\t\tunable_to_lock_message(git_path(\"packed-refs\"), errno, err);\n-\t\treturn -1;\n \t}\n+\n \tpacked = get_packed_refs(&ref_cache);\n \n \t/* Remove refnames from the cache */\n@@ -3796,10 +3797,12 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n int transaction_commit(struct transaction *transaction,\n \t\t struct strbuf *err)\n {\n-\tint ret = 0, delnum = 0, i;\n+\tint ret = 0, delnum = 0, i, need_repack = 0;\n \tconst char **delnames;\n \tint n = transaction->nr;\n+\tstruct packed_ref_cache *packed_ref_cache;\n \tstruct ref_update **updates = transaction->updates;\n+\tstruct ref_dir *packed;\n \n \tassert(err);\n \n@@ -3821,23 +3824,109 @@ int transaction_commit(struct transaction *transaction,\n \t\tgoto cleanup;\n \t}\n \n-\t/* Acquire all ref locks while verifying old values */\n+\t/* Lock packed refs during commit */\n+\tif (lock_packed_refs(0)) {\n+\t\tif (err)\n+\t\t\tunable_to_lock_message(git_path(\"packed-refs\"),\n+\t\t\t\t\t errno, err);\n+\t\tret = -1;\n+\t\tgoto cleanup;\n+\t}\n+\n+\t/* any loose refs are to be deleted are first copied to packed refs */\n \tfor (i = 0; i < n; i++) {\n \t\tstruct ref_update *update = updates[i];\n-\t\tint flags = update->flags;\n+\t\tunsigned char sha1[20];\n \n \t\tif (update->update_type != UPDATE_SHA1)\n \t\t\tcontinue;\n+\t\tif (!is_null_sha1(update->new_sha1))\n+\t\t\tcontinue;\n+\t\tif (get_packed_ref(update->refname))\n+\t\t\tcontinue;\n+\t\tif (!resolve_ref_unsafe(update->refname, RESOLVE_REF_READING,\n+\t\t\t\t\tsha1, NULL))\n+\t\t\tcontinue;\n \n-\t\tif (is_null_sha1(update->new_sha1))\n-\t\t\tflags |= REF_DELETING;\n+\t\tadd_packed_ref(update->refname, sha1);\n+\t\tneed_repack = 1;\n+\t}\n+\tif (need_repack) {\n+\t\tpacked = get_packed_refs(&ref_cache);;\n+\t\tsort_ref_dir(packed);\n+\t\tif (commit_packed_refs()){\n+\t\t\tstrbuf_addf(err, \"unable to overwrite old ref-pack \"\n+\t\t\t\t \"file\");\n+\t\t\tret = -1;\n+\t\t\tgoto cleanup;\n+\t\t}\n+\t\t/* lock the packed refs again so no one can change it */\n+\t\tif (lock_packed_refs(0)) {\n+\t\t\tif (err)\n+\t\t\t\tunable_to_lock_message(git_path(\"packed-refs\"),\n+\t\t\t\t\t\t errno, err);\n+\t\t\tret = -1;\n+\t\t\tgoto cleanup;\n+\t\t}\n+\t}\n+\n+\t/*\n+\t * At this stage any refs that are to be deleted have been moved to the\n+\t * packed refs file anf the packed refs file is deleted. We can now\n+\t * safely delete these loose refs.\n+\t */\n+\n+\t/* Unlink any loose refs scheduled for deletion */\n+\tfor (i = 0; i < n; i++) {\n+\t\tstruct ref_update *update = updates[i];\n+\n+\t\tif (update->update_type != UPDATE_SHA1)\n+\t\t\tcontinue;\n+\t\tif (!is_null_sha1(update->new_sha1))\n+\t\t\tcontinue;\n+\t\tupdate->flags |= REF_DELETING;\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 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\tint df_conflict = (errno == ENOTDIR);\n+\n+\t\t\tstrbuf_addf(err, \"Cannot lock the ref '%s'.\",\n+\t\t\t\t update->refname);\n+\t\t\tret = df_conflict ?\n+\t\t\t TRANSACTION_NAME_CONFLICT : \n+\t\t\t TRANSACTION_GENERIC_ERROR;\n+\t\t\tgoto cleanup;\n+\t\t}\n+\t\tif (delete_ref_loose(update->lock, update->type, err)) {\n+\t\t\tret = -1;\n+\t\t\tgoto cleanup;\n+\t\t}\n+\t\ttry_remove_empty_parents((char *)update->refname);\n+\t\tif (!(update->flags & REF_ISPRUNING))\n+\t\t\t delnames[delnum++] = xstrdup(update->lock->ref_name);\n+\t\tunlock_ref(update->lock);\n+\t\tupdate->lock = NULL;\n+\t}\n+\n+\t/* Acquire all ref locks for updates while verifying old values */\n+\tfor (i = 0; i < n; i++) {\n+\t\tstruct ref_update *update = updates[i];\n \n+\t\tif (update->update_type != UPDATE_SHA1)\n+\t\t\tcontinue;\n+\t\tif (is_null_sha1(update->new_sha1))\n+\t\t\tcontinue;\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 NULL,\n-\t\t\t\t\t\t flags,\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\tret = (errno == ENOTDIR)\n@@ -3849,6 +3938,10 @@ int transaction_commit(struct transaction *transaction,\n \t\t}\n \t}\n \n+\t/* delete reflog for all deleted refs */\n+\tfor (i = 0; i < delnum; i++)\n+\t\tunlink_or_warn(git_path(\"logs/%s\", delnames[i]));\n+\n \t/* Lock all reflog files */\n \tfor (i = 0; i < n; i++) {\n \t\tstruct ref_update *update = updates[i];\n@@ -3860,6 +3953,16 @@ int transaction_commit(struct transaction *transaction,\n \t\t\tupdate->reflog_lock = update->orig_update->reflog_lock;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (log_all_ref_updates && !reflog_exists(update->refname) &&\n+\t\t create_reflog(update->refname)) {\n+\t\t\tret = -1;\n+\t\t\tif (err)\n+\t\t\t\tstrbuf_addf(err, \"Failed to setup reflog for \"\n+\t\t\t\t\t \"%s\", update->refname);\n+\t\t\tgoto cleanup;\n+\t\t}\n+\t\tif (!reflog_exists(update->refname))\n+\t\t\tcontinue;\n \t\tupdate->reflog_fd = hold_lock_file_for_append(\n \t\t\t\t\tupdate->reflog_lock,\n \t\t\t\t\tgit_path(\"logs/%s\", update->refname),\n@@ -3875,7 +3978,7 @@ int transaction_commit(struct transaction *transaction,\n \t\t}\n \t}\n \n-\t/* Perform ref updates first so live commits remain referenced */\n+\t/* Perform ref updates */\n \tfor (i = 0; i < n; i++) {\n \t\tstruct ref_update *update = updates[i];\n \n@@ -3894,23 +3997,6 @@ int transaction_commit(struct transaction *transaction,\n \t\t}\n \t}\n \n-\t/* Perform deletes now that updates are safely completed */\n-\tfor (i = 0; i < n; i++) {\n-\t\tstruct ref_update *update = updates[i];\n-\n-\t\tif (update->update_type != UPDATE_SHA1)\n-\t\t\tcontinue;\n-\t\tif (update->lock) {\n-\t\t\tif (delete_ref_loose(update->lock, update->type, err)) {\n-\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n-\t\t\t\tgoto cleanup;\n-\t\t\t}\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 \t/*\n \t * Update all reflog files\n \t * We have already committed all ref updates and deletes.\n@@ -3961,15 +4047,14 @@ int transaction_commit(struct transaction *transaction,\n \t\t}\n \t}\n \n-\tif (repack_without_refs(delnames, delnum, err)) {\n+\tif (repack_without_refs(delnames, delnum, err))\n \t\tret = TRANSACTION_GENERIC_ERROR;\n-\t\tgoto cleanup;\n-\t}\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 \n cleanup:\n+\tpacked_ref_cache = get_packed_ref_cache(&ref_cache);\n+\tif (packed_ref_cache->lock)\n+\t\trollback_packed_refs();\n \ttransaction->state = TRANSACTION_CLOSED;\n \n \tfor (i = 0; i < n; i++)\n-- \n2.1.0.rc2.206.gedb03e5\n"},{"id":"250930","messageId":"1413923820-14457-5-git-send-email-sahlberg@google.com","threadId":"37776","inReplyTo":"1413923820-14457-1-git-send-email-sahlberg@google.com","subject":"[PATCH 04/15] refs.c: use a stringlist for repack_without_refs","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-10-21T20:36:49Z","receivedAt":"2014-10-21T20:36:49Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change-Id: I87a1b93c4b4f4a10ad6a9f8e2bb4d53f89b28c4a\nSigned-off-by: Ronnie Sahlberg \n---\n builtin/remote.c | 23 ++++++++---------------\n refs.c | 42 +++++++++++++++++++++---------------------\n refs.h | 2 +-\n 3 files changed, 30 insertions(+), 37 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex c25420f..6806251 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -751,7 +751,6 @@ static int mv(int argc, const char **argv)\n static int remove_branches(struct string_list *branches)\n {\n \tstruct strbuf err = STRBUF_INIT;\n-\tconst char **branch_names;\n \tint i, result = 0;\n \n \tif (lock_packed_refs(0)) {\n@@ -763,13 +762,9 @@ static int remove_branches(struct string_list *branches)\n \t\treturn -1;\n \t}\n \n-\tbranch_names = xmalloc(branches->nr * sizeof(*branch_names));\n-\tfor (i = 0; i < branches->nr; i++)\n-\t\tbranch_names[i] = branches->items[i].string;\n-\tif (repack_without_refs(branch_names, branches->nr, &err))\n+\tif (repack_without_refs(branches, &err))\n \t\tresult |= error(\"%s\", err.buf);\n \tstrbuf_release(&err);\n-\tfree(branch_names);\n \n \tfor (i = 0; i < branches->nr; i++) {\n \t\tstruct string_list_item *item = branches->items + i;\n@@ -1327,7 +1322,6 @@ static int prune_remote(const char *remote, int dry_run)\n \tint result = 0, i;\n \tstruct ref_states states;\n \tstruct string_list delete_refs_list = STRING_LIST_INIT_NODUP;\n-\tconst char **delete_refs;\n \tconst char *dangling_msg = dry_run\n \t\t? _(\" %s will become dangling!\")\n \t\t: _(\" %s has become dangling!\");\n@@ -1335,6 +1329,11 @@ static int prune_remote(const char *remote, int dry_run)\n \tmemset(&states, 0, sizeof(states));\n \tget_remote_ref_states(remote, &states, GET_REF_STATES);\n \n+\tfor (i = 0; i < states.stale.nr; i++) {\n+\t\tstring_list_insert(&delete_refs_list,\n+\t\t\t\t states.stale.items[i].util);\n+\t}\n+\n \tif (states.stale.nr) {\n \t\tprintf_ln(_(\"Pruning %s\"), remote);\n \t\tprintf_ln(_(\"URL: %s\"),\n@@ -1342,9 +1341,6 @@ static int prune_remote(const char *remote, int dry_run)\n \t\t ? states.remote->url[0]\n \t\t : _(\"(no URL)\"));\n \n-\t\tdelete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));\n-\t\tfor (i = 0; i < states.stale.nr; i++)\n-\t\t\tdelete_refs[i] = states.stale.items[i].util;\n \t\tif (!dry_run) {\n \t\t\tstruct strbuf err = STRBUF_INIT;\n \n@@ -1353,19 +1349,16 @@ static int prune_remote(const char *remote, int dry_run)\n \t\t\t\t\t\t errno, &err);\n \t\t\t\tresult |= error(\"%s\", err.buf);\n \t\t\t} else\n-\t\t\t\tif (repack_without_refs(delete_refs,\n-\t\t\t\t\t\t\tstates.stale.nr, &err))\n+\t\t\t\tif (repack_without_refs(&delete_refs_list,\n+\t\t\t\t\t\t\t&err))\n \t\t\t\t\tresult |= error(\"%s\", err.buf);\n \t\t\tstrbuf_release(&err);\n \t\t}\n-\t\tfree(delete_refs);\n \t}\n \n \tfor (i = 0; i < states.stale.nr; i++) {\n \t\tconst char *refname = states.stale.items[i].util;\n \n-\t\tstring_list_insert(&delete_refs_list, refname);\n-\n \t\tif (!dry_run)\n \t\t\tresult |= delete_ref(refname, NULL, 0);\n \ndiff --git a/refs.c b/refs.c\nindex 662af9b..b9c8f91 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2663,31 +2663,32 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)\n /*\n * Must be called with packed refs already locked (and sorted)\n */\n-int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n+int repack_without_refs(struct string_list *without, 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, ret, removed = 0;\n+\tint count, ret, removed = 0;\n \n \tassert(err);\n \n \t/* Look for a packed ref */\n-\tfor (i = 0; i < n; i++)\n-\t\tif (get_packed_ref(refnames[i]))\n-\t\t\tbreak;\n+\tcount = 0;\n+\tfor_each_string_list_item(ref_to_delete, without)\n+\t\tif (get_packed_ref(ref_to_delete->string))\n+\t\t\tcount++;\n \n-\t/* Avoid processing if we have nothing to do */\n-\tif (i == n) {\n+\t/* No refname exists in packed refs */\n+\tif (!count) {\n \t\trollback_packed_refs();\n-\t\treturn 0; /* no refname exists in packed refs */\n+\t\treturn 0;\n \t}\n \n \tpacked = get_packed_refs(&ref_cache);\n \n \t/* Remove refnames from the cache */\n-\tfor (i = 0; i < n; i++)\n-\t\tif (remove_entry(packed, refnames[i]) != -1)\n+\tfor_each_string_list_item(ref_to_delete, without)\n+\t\tif (remove_entry(packed, ref_to_delete->string) != -1)\n \t\t\tremoved = 1;\n \tif (!removed) {\n \t\t/*\n@@ -3797,12 +3798,13 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n int transaction_commit(struct transaction *transaction,\n \t\t struct strbuf *err)\n {\n-\tint ret = 0, delnum = 0, i, need_repack = 0;\n-\tconst char **delnames;\n+\tint ret = 0, i, need_repack = 0;\n \tint n = transaction->nr;\n \tstruct packed_ref_cache *packed_ref_cache;\n \tstruct ref_update **updates = transaction->updates;\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 \n \tassert(err);\n \n@@ -3814,9 +3816,6 @@ int transaction_commit(struct transaction *transaction,\n \t\treturn 0;\n \t}\n \n-\t/* Allocate work space */\n-\tdelnames = xmalloc(sizeof(*delnames) * n);\n-\n \t/* Copy, sort, and reject duplicate refs */\n \tqsort(updates, n, sizeof(*updates), ref_update_compare);\n \tif (ref_update_reject_duplicates(updates, n, err)) {\n@@ -3908,7 +3907,8 @@ int transaction_commit(struct transaction *transaction,\n \t\t}\n \t\ttry_remove_empty_parents((char *)update->refname);\n \t\tif (!(update->flags & REF_ISPRUNING))\n-\t\t\t delnames[delnum++] = xstrdup(update->lock->ref_name);\n+\t\t\tstring_list_insert(&refs_to_delete,\n+\t\t\t\t\t update->lock->ref_name);\n \t\tunlock_ref(update->lock);\n \t\tupdate->lock = NULL;\n \t}\n@@ -3925,7 +3925,7 @@ int transaction_commit(struct transaction *transaction,\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 NULL,\n+\t\t\t\t\t\t &refs_to_delete,\n \t\t\t\t\t\t update->flags,\n \t\t\t\t\t\t &update->type);\n \t\tif (!update->lock) {\n@@ -3939,8 +3939,8 @@ int transaction_commit(struct transaction *transaction,\n \t}\n \n \t/* delete reflog for all deleted refs */\n-\tfor (i = 0; i < delnum; i++)\n-\t\tunlink_or_warn(git_path(\"logs/%s\", delnames[i]));\n+\tfor_each_string_list_item(ref_to_delete, &refs_to_delete)\n+\t\tunlink_or_warn(git_path(\"logs/%s\", ref_to_delete->string));\n \n \t/* Lock all reflog files */\n \tfor (i = 0; i < n; i++) {\n@@ -4047,7 +4047,7 @@ int transaction_commit(struct transaction *transaction,\n \t\t}\n \t}\n \n-\tif (repack_without_refs(delnames, delnum, err))\n+\tif (repack_without_refs(&refs_to_delete, err))\n \t\tret = TRANSACTION_GENERIC_ERROR;\n \tclear_loose_ref_cache(&ref_cache);\n \n@@ -4060,7 +4060,7 @@ cleanup:\n \tfor (i = 0; i < n; i++)\n \t\tif (updates[i]->lock)\n \t\t\tunlock_ref(updates[i]->lock);\n-\tfree(delnames);\n+\tstring_list_clear(&refs_to_delete, 0);\n \treturn ret;\n }\n \ndiff --git a/refs.h b/refs.h\nindex 9153e1d..d174380 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -163,7 +163,7 @@ extern void rollback_packed_refs(void);\n */\n int pack_refs(unsigned int flags);\n \n-extern int repack_without_refs(const char **refnames, int n,\n+extern int repack_without_refs(struct string_list *without,\n \t\t\t struct strbuf *err);\n \n extern int ref_exists(const char *);\n-- \n2.1.0.rc2.206.gedb03e5\n"},{"id":"250932","messageId":"1413923820-14457-6-git-send-email-sahlberg@google.com","threadId":"37776","inReplyTo":"1413923820-14457-1-git-send-email-sahlberg@google.com","subject":"[PATCH 05/15] refs.c: update rename_ref to use a transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-10-21T20:36:50Z","receivedAt":"2014-10-21T20:36:50Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"commit 0295e9cebc41020ee84da275549b164a8770ffba upstream.\n\nChange refs.c to use a single transaction to copy/rename both the refs and\nits reflog. Since we are no longer using rename() to move the reflog file\nwe no longer need to disallow rename_ref for refs with a symlink for its\nreflog so we can remove that test from the testsuite.\n\nChange the function to return 1 on failure instead of either -1 or 1.\n\nThese changes make the rename_ref operation atomic. This also eliminates the\nneed to use rename() to shift the reflog around via a temporary filename.\nAs an extension to this, since we no longer use rename() on the reflog file,\nwe can now safely perform renames even if the reflog is a symbolic link\nand thus can remove the check and fail for that condition.\n\nChange-Id: I59477d410a34298a29cf0cbf32328b9053b158fe\nSigned-off-by: Ronnie Sahlberg \nSigned-off-by: Jonathan Nieder \n---\n refs.c | 192 ++++++++++++++++++++----------------------------------\n t/t3200-branch.sh | 7 --\n 2 files changed, 70 insertions(+), 129 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex b9c8f91..f43fef4 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2752,58 +2752,26 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n \treturn 0;\n }\n \n-/*\n- * People using contrib's git-new-workdir have .git/logs/refs ->\n- * /some/other/path/.git/logs/refs, and that may live on another device.\n- *\n- * IOW, to avoid cross device rename errors, the temporary renamed log must\n- * live into logs/refs.\n- */\n-#define TMP_RENAMED_LOG \"logs/refs/.tmp-renamed-log\"\n+struct rename_reflog_cb {\n+\tstruct transaction *transaction;\n+\tconst char *refname;\n+\tstruct strbuf *err;\n+};\n \n-static int rename_tmp_log(const char *newrefname)\n+static int rename_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n+\t\t\t const char *id, unsigned long timestamp, int tz,\n+\t\t\t const char *message, void *cb_data)\n {\n-\tint attempts_remaining = 4;\n+\tstruct rename_reflog_cb *cb = cb_data;\n+\tstruct reflog_committer_info ci;\n \n- retry:\n-\tswitch (safe_create_leading_directories(git_path(\"logs/%s\", newrefname))) {\n-\tcase SCLD_OK:\n-\t\tbreak; /* success */\n-\tcase SCLD_VANISHED:\n-\t\tif (--attempts_remaining > 0)\n-\t\t\tgoto retry;\n-\t\t/* fall through */\n-\tdefault:\n-\t\terror(\"unable to create directory for %s\", newrefname);\n-\t\treturn -1;\n-\t}\n-\n-\tif (rename(git_path(TMP_RENAMED_LOG), git_path(\"logs/%s\", newrefname))) {\n-\t\tif ((errno==EISDIR || errno==ENOTDIR) && --attempts_remaining > 0) {\n-\t\t\t/*\n-\t\t\t * rename(a, b) when b is an existing\n-\t\t\t * directory ought to result in ISDIR, but\n-\t\t\t * Solaris 5.8 gives ENOTDIR. Sheesh.\n-\t\t\t */\n-\t\t\tif (remove_empty_directories(git_path(\"logs/%s\", newrefname))) {\n-\t\t\t\terror(\"Directory not empty: logs/%s\", newrefname);\n-\t\t\t\treturn -1;\n-\t\t\t}\n-\t\t\tgoto retry;\n-\t\t} else if (errno == ENOENT && --attempts_remaining > 0) {\n-\t\t\t/*\n-\t\t\t * Maybe another process just deleted one of\n-\t\t\t * the directories in the path to newrefname.\n-\t\t\t * Try again from the beginning.\n-\t\t\t */\n-\t\t\tgoto retry;\n-\t\t} else {\n-\t\t\terror(\"unable to move logfile \"TMP_RENAMED_LOG\" to logs/%s: %s\",\n-\t\t\t\tnewrefname, strerror(errno));\n-\t\t\treturn -1;\n-\t\t}\n-\t}\n-\treturn 0;\n+\tmemset(&ci, 0, sizeof(ci));\n+\tci.id = id;\n+\tci.timestamp = timestamp;\n+\tci.tz = tz;\n+\treturn transaction_update_reflog(cb->transaction, cb->refname,\n+\t\t\t\t\t nsha1, osha1, &ci, message, 0,\n+\t\t\t\t\t cb->err);\n }\n \n static int rename_ref_available(const char *oldname, const char *newname)\n@@ -2823,91 +2791,71 @@ static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,\n \n int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsg)\n {\n-\tunsigned char sha1[20], orig_sha1[20];\n-\tint flag = 0, logmoved = 0;\n-\tstruct ref_lock *lock;\n-\tstruct stat loginfo;\n-\tint log = !lstat(git_path(\"logs/%s\", oldrefname), &loginfo);\n+\tunsigned char sha1[20];\n+\tint flag = 0, log;\n+\tstruct transaction *transaction = NULL;\n+\tstruct strbuf err = STRBUF_INIT;\n \tconst char *symref = NULL;\n+\tstruct rename_reflog_cb cb;\n+\tstruct reflog_committer_info ci;\n \n-\tif (log && S_ISLNK(loginfo.st_mode))\n-\t\treturn error(\"reflog for %s is a symlink\", oldrefname);\n+\tmemset(&ci, 0, sizeof(ci));\n+\tci.committer_info = git_committer_info(0);\n \n \tsymref = resolve_ref_unsafe(oldrefname, RESOLVE_REF_READING,\n-\t\t\t\t orig_sha1, &flag);\n-\tif (flag & REF_ISSYMREF)\n-\t\treturn error(\"refname %s is a symbolic ref, renaming it is not supported\",\n-\t\t\toldrefname);\n-\tif (!symref)\n-\t\treturn error(\"refname %s not found\", oldrefname);\n-\n-\tif (!rename_ref_available(oldrefname, newrefname))\n+\t\t\t\t sha1, &flag);\n+\tif (flag & REF_ISSYMREF) {\n+\t\terror(\"refname %s is a symbolic ref, renaming it is not \"\n+\t\t \"supported\", oldrefname);\n \t\treturn 1;\n-\n-\tif (log && rename(git_path(\"logs/%s\", oldrefname), git_path(TMP_RENAMED_LOG)))\n-\t\treturn error(\"unable to move logfile logs/%s to \"TMP_RENAMED_LOG\": %s\",\n-\t\t\toldrefname, strerror(errno));\n-\n-\tif (delete_ref(oldrefname, orig_sha1, REF_NODEREF)) {\n-\t\terror(\"unable to delete old %s\", oldrefname);\n-\t\tgoto rollback;\n \t}\n-\n-\tif (!read_ref_full(newrefname, RESOLVE_REF_READING, sha1, 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-\t\t\t\terror(\"Directory not empty: %s\", newrefname);\n-\t\t\t\tgoto rollback;\n-\t\t\t}\n-\t\t} else {\n-\t\t\terror(\"unable to delete existing %s\", newrefname);\n-\t\t\tgoto rollback;\n-\t\t}\n+\tif (!symref) {\n+\t\terror(\"refname %s not found\", oldrefname);\n+\t\treturn 1;\n \t}\n \n-\tif (log && rename_tmp_log(newrefname))\n-\t\tgoto rollback;\n-\n-\tlogmoved = log;\n+\tif (!rename_ref_available(oldrefname, newrefname))\n+\t\treturn 1;\n \n-\tlock = lock_ref_sha1_basic(newrefname, NULL, NULL, 0, NULL);\n-\tif (!lock) {\n-\t\terror(\"unable to lock %s for update\", newrefname);\n-\t\tgoto rollback;\n-\t}\n-\tlock->force_write = 1;\n-\thashcpy(lock->old_sha1, orig_sha1);\n-\tif (write_ref_sha1(lock, orig_sha1, logmsg)) {\n-\t\terror(\"unable to write current sha1 into %s\", newrefname);\n-\t\tgoto rollback;\n+\tlog = reflog_exists(oldrefname);\n+\ttransaction = transaction_begin(&err);\n+\tif (!transaction)\n+\t\tgoto fail;\n+\n+\tif (strcmp(oldrefname, newrefname)) {\n+\t\tif (log && transaction_update_reflog(transaction, newrefname,\n+\t\t\t\t\t\t sha1, sha1, &ci, NULL,\n+\t\t\t\t\t\t REFLOG_TRUNCATE, &err))\n+\t\t\tgoto fail;\n+\t\tcb.transaction = transaction;\n+\t\tcb.refname = newrefname;\n+\t\tcb.err = &err;\n+\t\tif (log && for_each_reflog_ent(oldrefname, rename_reflog_ent,\n+\t\t\t\t\t &cb))\n+\t\t\tgoto fail;\n+\n+\t\tif (transaction_delete_ref(transaction, oldrefname, sha1,\n+\t\t\t\t\t REF_NODEREF,\n+\t\t\t\t\t 1, NULL, &err))\n+\t\t\tgoto fail;\n \t}\n-\n+\tif (transaction_update_ref(transaction, newrefname, sha1,\n+\t\t\t\t NULL, 0, 0, NULL, &err))\n+\t\tgoto fail;\n+\tif (log && transaction_update_reflog(transaction, newrefname, sha1,\n+\t\t\t\t\t sha1, &ci, logmsg,\n+\t\t\t\t\t REFLOG_COMMITTER_INFO_IS_VALID,\n+\t\t\t\t\t &err))\n+\t\tgoto fail;\n+\tif (transaction_commit(transaction, &err))\n+\t\tgoto fail;\n+\ttransaction_free(transaction);\n \treturn 0;\n \n- rollback:\n-\tlock = lock_ref_sha1_basic(oldrefname, NULL, NULL, 0, NULL);\n-\tif (!lock) {\n-\t\terror(\"unable to lock %s for rollback\", oldrefname);\n-\t\tgoto rollbacklog;\n-\t}\n-\n-\tlock->force_write = 1;\n-\tflag = log_all_ref_updates;\n-\tlog_all_ref_updates = 0;\n-\tif (write_ref_sha1(lock, orig_sha1, NULL))\n-\t\terror(\"unable to write current sha1 into %s\", oldrefname);\n-\tlog_all_ref_updates = flag;\n-\n- rollbacklog:\n-\tif (logmoved && rename(git_path(\"logs/%s\", newrefname), git_path(\"logs/%s\", oldrefname)))\n-\t\terror(\"unable to restore logfile %s from %s: %s\",\n-\t\t\toldrefname, newrefname, strerror(errno));\n-\tif (!logmoved && log &&\n-\t rename(git_path(TMP_RENAMED_LOG), git_path(\"logs/%s\", oldrefname)))\n-\t\terror(\"unable to restore logfile %s from \"TMP_RENAMED_LOG\": %s\",\n-\t\t\toldrefname, strerror(errno));\n-\n+ fail:\n+\terror(\"rename_ref failed: %s\", err.buf);\n+\tstrbuf_release(&err);\n+\ttransaction_free(transaction);\n \treturn 1;\n }\n \ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 432921b..c6c53e4 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -302,13 +302,6 @@ test_expect_success 'renaming a symref is not allowed' '\n \ttest_path_is_missing .git/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-\tgit branch -l u &&\n-\tmv .git/logs/refs/heads/u real-u &&\n-\tln -s real-u .git/logs/refs/heads/u &&\n-\ttest_must_fail git branch -m u v\n-'\n-\n test_expect_success 'test tracking setup via --track' '\n \tgit config remote.local.url . &&\n \tgit config remote.local.fetch refs/heads/*:refs/remotes/local/* &&\n-- \n2.1.0.rc2.206.gedb03e5\n"},{"id":"250947","messageId":"1413923820-14457-7-git-send-email-sahlberg@google.com","threadId":"37776","inReplyTo":"1413923820-14457-1-git-send-email-sahlberg@google.com","subject":"[PATCH 06/15] refs.c: rollback the lockfile before we die() in repack_without_refs","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-10-21T20:36:51Z","receivedAt":"2014-10-21T20:36:51Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"commit 3989c2a763c3b355785d609b3144c7935dffb273 upstream.\n\nChange-Id: I63a22da521ecc8eb60d7a8aaa5af666d2827a599\nSigned-off-by: Ronnie Sahlberg \nSigned-off-by: Jonathan Nieder \n---\n refs.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex f43fef4..43df656 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2702,8 +2702,10 @@ int repack_without_refs(struct string_list *without, struct strbuf *err)\n \t/* Remove any other accumulated cruft */\n \tdo_for_each_entry_in_dir(packed, 0, curate_packed_ref_fn, &refs_to_delete);\n \tfor_each_string_list_item(ref_to_delete, &refs_to_delete) {\n-\t\tif (remove_entry(packed, ref_to_delete->string) == -1)\n+\t\tif (remove_entry(packed, ref_to_delete->string) == -1) {\n+\t\t\trollback_packed_refs();\n \t\t\tdie(\"internal error\");\n+\t\t}\n \t}\n \n \t/* Write what remains */\n-- \n2.1.0.rc2.206.gedb03e5\n"},{"id":"250931","messageId":"1413923820-14457-8-git-send-email-sahlberg@google.com","threadId":"37776","inReplyTo":"1413923820-14457-1-git-send-email-sahlberg@google.com","subject":"[PATCH 07/15] refs.c: move reflog updates into its own function","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-10-21T20:36:52Z","receivedAt":"2014-10-21T20:36:52Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"write_ref_sha1 tries to update the reflog while updating the ref.\nMove these reflog changes out into its own function so that we can do the\nsame thing if we write a sha1 ref differently, for example by writing a ref\nto the packed refs file instead.\n\nNo functional changes intended. We only move some code out into a separate\nfunction.\n\nChange-Id: I5ef6b32d183455685f8966379767f8c6e1ec49c9\nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 60 +++++++++++++++++++++++++++++++++++-------------------------\n 1 file changed, 35 insertions(+), 25 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 43df656..4eec7e4 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3022,6 +3022,40 @@ int is_branch(const char *refname)\n \treturn !strcmp(refname, \"HEAD\") || starts_with(refname, \"refs/heads/\");\n }\n \n+static int write_sha1_update_reflog(struct ref_lock *lock,\n+\tconst unsigned char *sha1, const char *logmsg)\n+{\n+\tif (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||\n+\t (strcmp(lock->ref_name, lock->orig_ref_name) &&\n+\t log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) < 0)) {\n+\t\tunlock_ref(lock);\n+\t\treturn -1;\n+\t}\n+\tif (strcmp(lock->orig_ref_name, \"HEAD\") != 0) {\n+\t\t/*\n+\t\t * Special hack: If a branch is updated directly and HEAD\n+\t\t * points to it (may happen on the remote side of a push\n+\t\t * for example) then logically the HEAD reflog should be\n+\t\t * updated too.\n+\t\t * A generic solution implies reverse symref information,\n+\t\t * but finding all symrefs pointing to the given branch\n+\t\t * would be rather costly for this rare event (the direct\n+\t\t * update of a branch) to be worth it. So let's cheat and\n+\t\t * check with HEAD only which should cover 99% of all usage\n+\t\t * scenarios (even 100% of the default ones).\n+\t\t */\n+\t\tunsigned char head_sha1[20];\n+\t\tint head_flag;\n+\t\tconst char *head_ref;\n+\t\thead_ref = resolve_ref_unsafe(\"HEAD\", RESOLVE_REF_READING,\n+\t\t\t\t\t head_sha1, &head_flag);\n+\t\tif (head_ref && (head_flag & REF_ISSYMREF) &&\n+\t\t !strcmp(head_ref, lock->ref_name))\n+\t\t\tlog_ref_write(\"HEAD\", lock->old_sha1, sha1, logmsg);\n+\t}\n+\treturn 0;\n+}\n+\n /*\n * Write sha1 into the ref specified by the lock. Make sure that errno\n * is sane on error.\n@@ -3065,34 +3099,10 @@ static int write_ref_sha1(struct ref_lock *lock,\n \t\treturn -1;\n \t}\n \tclear_loose_ref_cache(&ref_cache);\n-\tif (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||\n-\t (strcmp(lock->ref_name, lock->orig_ref_name) &&\n-\t log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) < 0)) {\n+\tif (write_sha1_update_reflog(lock, sha1, logmsg)) {\n \t\tunlock_ref(lock);\n \t\treturn -1;\n \t}\n-\tif (strcmp(lock->orig_ref_name, \"HEAD\") != 0) {\n-\t\t/*\n-\t\t * Special hack: If a branch is updated directly and HEAD\n-\t\t * points to it (may happen on the remote side of a push\n-\t\t * for example) then logically the HEAD reflog should be\n-\t\t * updated too.\n-\t\t * A generic solution implies reverse symref information,\n-\t\t * but finding all symrefs pointing to the given branch\n-\t\t * would be rather costly for this rare event (the direct\n-\t\t * update of a branch) to be worth it. So let's cheat and\n-\t\t * check with HEAD only which should cover 99% of all usage\n-\t\t * scenarios (even 100% of the default ones).\n-\t\t */\n-\t\tunsigned char head_sha1[20];\n-\t\tint head_flag;\n-\t\tconst char *head_ref;\n-\t\thead_ref = resolve_ref_unsafe(\"HEAD\", RESOLVE_REF_READING,\n-\t\t\t\t\t head_sha1, &head_flag);\n-\t\tif (head_ref && (head_flag & REF_ISSYMREF) &&\n-\t\t !strcmp(head_ref, lock->ref_name))\n-\t\t\tlog_ref_write(\"HEAD\", lock->old_sha1, sha1, logmsg);\n-\t}\n \tif (commit_ref(lock)) {\n \t\terror(\"Couldn't set %s\", lock->ref_name);\n \t\tunlock_ref(lock);\n-- \n2.1.0.rc2.206.gedb03e5\n"},{"id":"250934","messageId":"1413923820-14457-9-git-send-email-sahlberg@google.com","threadId":"37776","inReplyTo":"1413923820-14457-1-git-send-email-sahlberg@google.com","subject":"[PATCH 08/15] refs.c: write updates to packed refs when a transaction has more than one ref","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-10-21T20:36:53Z","receivedAt":"2014-10-21T20:36:53Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"When we are updating more than one single ref, i.e. not a commit, then\nwrite the updated refs directly to the packed refs file instead of writing\nthem as loose refs.\n\nChange clone to use a transaction instead of using the packed refs API.\nThis changes the behavior of clone slightly. Previously clone would always\nclone all refs into a packed refs file. With this change clone will only\nclone into packed refs iff there are two or more refs being cloned.\nIf the repository we are cloning from only contains exactly one single ref\nthen clone will now store this as a loose ref. The benefit here is that\nwe no longer need to export a bunch of API functions to clone to access\npacked refs directly. Clone can now just use a normal transaction and all\nthe packed refs goodness will happen automatically.\n\nUpdate the t5516 test to cope with the fact that clone now only uses\npacked refs if there are more than one ref being cloned.\n\nWe still use loose refs for single ref transactions, such as are used\nby 'git commit' and friends. The reason for this is that if you have very\nmany refs then having to re-write the whole packed refs file for every\ncommon operation like commit would have a performance impact.\nThat said, with these changes it should now be fairly straightforward to\nadd support to optionally start using packed refs for ALL updates\nwhich could solve existing issues with name clashes in case insensitive\nfilesystems.\n\nThis change also means that multi-ref updates will now appear as a single\natomic change to any external observers instead of a sequence of discreete\nchanges.\n\nChange-Id: I70cdcdb4341048386133e5b56b08c0b580b24c1b\nSigned-off-by: Ronnie Sahlberg \n---\n builtin/clone.c | 16 ++++++---\n refs.c | 89 ++++++++++++++++++++++++++++++++++-----------------\n t/t5516-fetch-push.sh | 2 +-\n 3 files changed, 72 insertions(+), 35 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 7f509d0..5052fac 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -498,17 +498,25 @@ static struct ref *wanted_peer_refs(const struct ref *refs,\n static void write_remote_refs(const struct ref *local_refs)\n {\n \tconst struct ref *r;\n+\tstruct transaction *transaction;\n+\tstruct strbuf err = STRBUF_INIT;\n \n-\tlock_packed_refs(LOCK_DIE_ON_ERROR);\n+\ttransaction = transaction_begin(&err);\n+\tif (!transaction)\n+\t\tdie(\"%s\", err.buf);\n \n \tfor (r = local_refs; r; r = r->next) {\n \t\tif (!r->peer_ref)\n \t\t\tcontinue;\n-\t\tadd_packed_ref(r->peer_ref->name, r->old_sha1);\n+\t\tif (transaction_update_ref(transaction, r->peer_ref->name,\n+\t\t\t\t\t r->old_sha1, NULL, 0, 0, NULL,\n+\t\t\t\t\t &err))\n+\t\t\tdie(\"%s\", err.buf);\n \t}\n \n-\tif (commit_packed_refs())\n-\t\tdie_errno(\"unable to overwrite old ref-pack file\");\n+\tif (transaction_commit(transaction, &err))\n+\t\tdie(\"%s\", err.buf);\n+\ttransaction_free(transaction);\n }\n \n static void write_followtags(const struct ref *refs, const char *msg)\ndiff --git a/refs.c b/refs.c\nindex 4eec7e4..b64d0c7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2668,36 +2668,15 @@ int repack_without_refs(struct string_list *without, struct strbuf *err)\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 count, ret, removed = 0;\n+\tint ret;\n \n \tassert(err);\n \n-\t/* Look for a packed ref */\n-\tcount = 0;\n-\tfor_each_string_list_item(ref_to_delete, without)\n-\t\tif (get_packed_ref(ref_to_delete->string))\n-\t\t\tcount++;\n-\n-\t/* No refname exists in packed refs */\n-\tif (!count) {\n-\t\trollback_packed_refs();\n-\t\treturn 0;\n-\t}\n-\n \tpacked = get_packed_refs(&ref_cache);\n \n \t/* Remove refnames from the cache */\n \tfor_each_string_list_item(ref_to_delete, without)\n-\t\tif (remove_entry(packed, ref_to_delete->string) != -1)\n-\t\t\tremoved = 1;\n-\tif (!removed) {\n-\t\t/*\n-\t\t * All packed entries disappeared while we were\n-\t\t * acquiring the lock.\n-\t\t */\n-\t\trollback_packed_refs();\n-\t\treturn 0;\n-\t}\n+\t\tremove_entry(packed, ref_to_delete->string);\n \n \t/* Remove any other accumulated cruft */\n \tdo_for_each_entry_in_dir(packed, 0, curate_packed_ref_fn, &refs_to_delete);\n@@ -3759,6 +3738,7 @@ int transaction_commit(struct transaction *transaction,\n \t\t struct strbuf *err)\n {\n \tint ret = 0, i, need_repack = 0;\n+\tint num_updates = 0;\n \tint n = transaction->nr;\n \tstruct packed_ref_cache *packed_ref_cache;\n \tstruct ref_update **updates = transaction->updates;\n@@ -3792,14 +3772,30 @@ int transaction_commit(struct transaction *transaction,\n \t\tgoto cleanup;\n \t}\n \n-\t/* any loose refs are to be deleted are first copied to packed refs */\n+\t/* count how many refs we are updating (not deleting) */\n+\tfor (i = 0; i < n; i++) {\n+\t\tstruct ref_update *update = updates[i];\n+\n+\t\tif (update->update_type != UPDATE_SHA1)\n+\t\t\tcontinue;\n+\t\tif (is_null_sha1(update->new_sha1))\n+\t\t\tcontinue;\n+\n+\t\tnum_updates++;\n+\t}\n+\n+\t/*\n+\t * Always copy loose refs that are to be deleted to the packed refs.\n+\t * If we are updating multiple refs then copy all refs to the packed\n+\t * refs file.\n+\t */\n \tfor (i = 0; i < n; i++) {\n \t\tstruct ref_update *update = updates[i];\n \t\tunsigned char sha1[20];\n \n \t\tif (update->update_type != UPDATE_SHA1)\n \t\t\tcontinue;\n-\t\tif (!is_null_sha1(update->new_sha1))\n+\t\tif (num_updates < 2 && !is_null_sha1(update->new_sha1))\n \t\t\tcontinue;\n \t\tif (get_packed_ref(update->refname))\n \t\t\tcontinue;\n@@ -3811,7 +3807,7 @@ int transaction_commit(struct transaction *transaction,\n \t\tneed_repack = 1;\n \t}\n \tif (need_repack) {\n-\t\tpacked = get_packed_refs(&ref_cache);;\n+\t\tpacked = get_packed_refs(&ref_cache);\n \t\tsort_ref_dir(packed);\n \t\tif (commit_packed_refs()){\n \t\t\tstrbuf_addf(err, \"unable to overwrite old ref-pack \"\n@@ -3828,13 +3824,15 @@ int transaction_commit(struct transaction *transaction,\n \t\t\tgoto cleanup;\n \t\t}\n \t}\n+\tneed_repack = 0;\n \n \t/*\n \t * At this stage any refs that are to be deleted have been moved to the\n-\t * packed refs file anf the packed refs file is deleted. We can now\n+\t * packed refs file and the packed refs file is committed. We can now\n \t * safely delete these loose refs.\n+\t * If we are updating multiple refs then those will also be in the\n+\t * packed refs file so we can delete those too.\n \t */\n-\n \t/* Unlink any loose refs scheduled for deletion */\n \tfor (i = 0; i < n; i++) {\n \t\tstruct ref_update *update = updates[i];\n@@ -3873,7 +3871,10 @@ int transaction_commit(struct transaction *transaction,\n \t\tupdate->lock = NULL;\n \t}\n \n-\t/* Acquire all ref locks for updates while verifying old values */\n+\t/*\n+\t * Acquire all ref locks for updates while verifying old values.\n+\t * If we are multi-updating then update them in packed refs.\n+\t */\n \tfor (i = 0; i < n; i++) {\n \t\tstruct ref_update *update = updates[i];\n \n@@ -3896,6 +3897,30 @@ int transaction_commit(struct transaction *transaction,\n \t\t\t\t update->refname);\n \t\t\tgoto cleanup;\n \t\t}\n+\t\tif (num_updates < 2)\n+\t\t\tcontinue;\n+\n+\t\tif (delete_ref_loose(update->lock, update->type, err)) {\n+\t\t\tret = -1;\n+\t\t\tgoto cleanup;\n+\t\t}\n+\t\tif (write_sha1_update_reflog(update->lock, update->new_sha1,\n+\t\t\t\t\t update->msg)) {\n+\t\t\tif (err)\n+\t\t\t\tstrbuf_addf(err, \"Failed to update log '%s'.\",\n+\t\t\t\t\t update->refname);\n+\t\t\tret = -1;\n+\t\t\tgoto cleanup;\n+\t\t}\n+\t\tunlock_ref(update->lock);\n+\t\tupdate->lock = NULL;\n+\n+\t\tpacked = get_packed_refs(&ref_cache);\n+\t\tremove_entry(packed, update->refname);\n+\t\tadd_packed_ref(update->refname, update->new_sha1);\n+\t\tneed_repack = 1;\n+\n+\t\ttry_remove_empty_parents((char *)update->refname);\n \t}\n \n \t/* delete reflog for all deleted refs */\n@@ -3944,7 +3969,7 @@ int transaction_commit(struct transaction *transaction,\n \n \t\tif (update->update_type != UPDATE_SHA1)\n \t\t\tcontinue;\n-\t\tif (!is_null_sha1(update->new_sha1)) {\n+\t\tif (update->lock && !is_null_sha1(update->new_sha1)) {\n \t\t\tif (write_ref_sha1(update->lock, update->new_sha1,\n \t\t\t\t\t update->msg)) {\n \t\t\t\tupdate->lock = NULL; /* freed by write_ref_sha1 */\n@@ -4007,6 +4032,10 @@ int transaction_commit(struct transaction *transaction,\n \t\t}\n \t}\n \n+\tif (need_repack) {\n+\t\tpacked = get_packed_refs(&ref_cache);\n+\t\tsort_ref_dir(packed);\n+\t}\n \tif (repack_without_refs(&refs_to_delete, err))\n \t\tret = TRANSACTION_GENERIC_ERROR;\n \tclear_loose_ref_cache(&ref_cache);\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 7c8a769..03375a9 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -592,7 +592,7 @@ test_expect_success 'push updates up-to-date local refs' '\n \n test_expect_success 'push preserves up-to-date packed refs' '\n \n-\tmk_test testrepo heads/master &&\n+\tmk_test testrepo heads/master heads/foo heads/bar &&\n \tmk_child testrepo child &&\n \t(\n \t\tcd child &&\n-- \n2.1.0.rc2.206.gedb03e5\n"},{"id":"250929","messageId":"1413923820-14457-10-git-send-email-sahlberg@google.com","threadId":"37776","inReplyTo":"1413923820-14457-1-git-send-email-sahlberg@google.com","subject":"[PATCH 09/15] remote.c: use a transaction for deleting refs","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-10-21T20:36:54Z","receivedAt":"2014-10-21T20:36:54Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Transactions now use packed refs when deleting multiple refs so there is no\nneed to do it manually from remote.c any more.\n\nChange-Id: I50cf9c4ab0c190b99e2c6c4e2f05abe260b0fe62\nSigned-off-by: Ronnie Sahlberg \n---\n builtin/remote.c | 80 ++++++++++++++++++++++++++++----------------------------\n 1 file changed, 40 insertions(+), 40 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 6806251..42702d7 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -750,30 +750,27 @@ static int mv(int argc, const char **argv)\n \n static int remove_branches(struct string_list *branches)\n {\n-\tstruct strbuf err = STRBUF_INIT;\n \tint i, result = 0;\n+\tstruct transaction *transaction;\n+\tstruct strbuf err = STRBUF_INIT;\n \n-\tif (lock_packed_refs(0)) {\n-\t\tstruct strbuf err = STRBUF_INIT;\n-\n-\t\tunable_to_lock_message(git_path(\"packed-refs\"), errno, &err);\n-\t\terror(\"%s\", err.buf);\n-\t\tstrbuf_release(&err);\n-\t\treturn -1;\n-\t}\n+\ttransaction = transaction_begin(&err);\n+\tif (!transaction)\n+\t\tdie(\"%s\", err.buf);\n \n-\tif (repack_without_refs(branches, &err))\n+\tfor (i = 0; i < branches->nr; i++)\n+\t\tif (transaction_delete_ref(transaction,\n+\t\t\t\t\t branches->items[i].string, NULL,\n+\t\t\t\t\t 0, 0, \"remote-branches\", &err)) {\n+\t\t\tresult |= error(\"%s\", err.buf);\n+\t\t\tgoto cleanup;\n+\t\t}\n+\tif (transaction_commit(transaction, &err))\n \t\tresult |= error(\"%s\", err.buf);\n-\tstrbuf_release(&err);\n-\n-\tfor (i = 0; i < branches->nr; i++) {\n-\t\tstruct string_list_item *item = branches->items + i;\n-\t\tconst char *refname = item->string;\n-\n-\t\tif (delete_ref(refname, NULL, 0))\n-\t\t\tresult |= error(_(\"Could not remove branch %s\"), refname);\n-\t}\n \n+ cleanup:\n+\tstrbuf_release(&err);\n+\ttransaction_free(transaction);\n \treturn result;\n }\n \n@@ -1325,42 +1322,38 @@ static int prune_remote(const char *remote, int dry_run)\n \tconst char *dangling_msg = dry_run\n \t\t? _(\" %s will become dangling!\")\n \t\t: _(\" %s has become dangling!\");\n+\tstruct transaction *transaction = NULL;\n+\tstruct strbuf err = STRBUF_INIT;\n \n \tmemset(&states, 0, sizeof(states));\n \tget_remote_ref_states(remote, &states, GET_REF_STATES);\n \n-\tfor (i = 0; i < states.stale.nr; i++) {\n-\t\tstring_list_insert(&delete_refs_list,\n-\t\t\t\t states.stale.items[i].util);\n-\t}\n-\n \tif (states.stale.nr) {\n \t\tprintf_ln(_(\"Pruning %s\"), remote);\n \t\tprintf_ln(_(\"URL: %s\"),\n \t\t states.remote->url_nr\n \t\t ? states.remote->url[0]\n \t\t : _(\"(no URL)\"));\n-\n-\t\tif (!dry_run) {\n-\t\t\tstruct strbuf err = STRBUF_INIT;\n-\n-\t\t\tif (lock_packed_refs(0)) {\n-\t\t\t\tunable_to_lock_message(git_path(\"packed-refs\"),\n-\t\t\t\t\t\t errno, &err);\n-\t\t\t\tresult |= error(\"%s\", err.buf);\n-\t\t\t} else\n-\t\t\t\tif (repack_without_refs(&delete_refs_list,\n-\t\t\t\t\t\t\t&err))\n-\t\t\t\t\tresult |= error(\"%s\", err.buf);\n-\t\t\tstrbuf_release(&err);\n-\t\t}\n \t}\n \n+\tif (!dry_run) {\n+\t\ttransaction = transaction_begin(&err);\n+\t\tif (!transaction)\n+\t\t\tdie(\"%s\", err.buf);\n+\t}\n \tfor (i = 0; i < states.stale.nr; i++) {\n \t\tconst char *refname = states.stale.items[i].util;\n \n-\t\tif (!dry_run)\n-\t\t\tresult |= delete_ref(refname, NULL, 0);\n+\t\tstring_list_insert(&delete_refs_list, refname);\n+\n+\t\tif (!dry_run) {\n+\t\t\tif (transaction_delete_ref(transaction,\n+\t\t\t\t\t refname, NULL,\n+\t\t\t\t\t 0, 0, \"remote-branches\", &err)) {\n+\t\t\t\tresult |= error(\"%s\", err.buf);\n+\t\t\t\tgoto cleanup;\n+\t\t\t}\n+\t\t}\n \n \t\tif (dry_run)\n \t\t\tprintf_ln(_(\" * [would prune] %s\"),\n@@ -1370,6 +1363,13 @@ static int prune_remote(const char *remote, int dry_run)\n \t\t\t abbrev_ref(refname, \"refs/remotes/\"));\n \t}\n \n+\tif (!dry_run)\n+\t\tif (transaction_commit(transaction, &err))\n+\t\t\tresult |= error(\"%s\", err.buf);\n+\n+ cleanup:\n+\tstrbuf_release(&err);\n+\ttransaction_free(transaction);\n \twarn_dangling_symrefs(stdout, dangling_msg, &delete_refs_list);\n \tstring_list_clear(&delete_refs_list, 0);\n \n-- \n2.1.0.rc2.206.gedb03e5\n"},{"id":"250935","messageId":"1413923820-14457-11-git-send-email-sahlberg@google.com","threadId":"37776","inReplyTo":"1413923820-14457-1-git-send-email-sahlberg@google.com","subject":"[PATCH 10/15] refs.c: make repack_without_refs static","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-10-21T20:36:55Z","receivedAt":"2014-10-21T20:36:55Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change-Id: Ibf02549e5485ad07da66fe4b1c84f9e2b76b2aca\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 b64d0c7..fddd59c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2663,7 +2663,7 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)\n /*\n * Must be called with packed refs already locked (and sorted)\n */\n-int repack_without_refs(struct string_list *without, struct strbuf *err)\n+static int repack_without_refs(struct string_list *without, struct strbuf *err)\n {\n \tstruct ref_dir *packed;\n \tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\ndiff --git a/refs.h b/refs.h\nindex d174380..db435bf 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -163,9 +163,6 @@ extern void rollback_packed_refs(void);\n */\n int pack_refs(unsigned int flags);\n \n-extern int repack_without_refs(struct string_list *without,\n-\t\t\t struct strbuf *err);\n-\n extern int ref_exists(const char *);\n \n extern int is_branch(const char *refname);\n-- \n2.1.0.rc2.206.gedb03e5\n"},{"id":"250939","messageId":"1413923820-14457-12-git-send-email-sahlberg@google.com","threadId":"37776","inReplyTo":"1413923820-14457-1-git-send-email-sahlberg@google.com","subject":"[PATCH 11/15] refs.c: make the *_packed_refs functions static","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-10-21T20:36:56Z","receivedAt":"2014-10-21T20:36:56Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"We no longer need to expose the lock/add/commit/rollback functions\nfor packed refs anymore so make them static and remove them from the\npublic api.\n\nChange-Id: I1059f1690129f0232cb27872ef494024ef7f299e\nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 8 ++++----\n refs.h | 30 ------------------------------\n 2 files changed, 4 insertions(+), 34 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex fddd59c..1261a78 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1224,7 +1224,7 @@ static struct ref_dir *get_packed_refs(struct ref_cache *refs)\n \treturn get_packed_ref_dir(get_packed_ref_cache(refs));\n }\n \n-void add_packed_ref(const char *refname, const unsigned char *sha1)\n+static void add_packed_ref(const char *refname, const unsigned char *sha1)\n {\n \tstruct packed_ref_cache *packed_ref_cache =\n \t\tget_packed_ref_cache(&ref_cache);\n@@ -2393,7 +2393,7 @@ static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)\n }\n \n /* This should return a meaningful errno on failure */\n-int lock_packed_refs(int flags)\n+static int lock_packed_refs(int flags)\n {\n \tstruct packed_ref_cache *packed_ref_cache;\n \n@@ -2416,7 +2416,7 @@ int lock_packed_refs(int flags)\n * Commit the packed refs changes.\n * On error we must make sure that errno contains a meaningful value.\n */\n-int commit_packed_refs(void)\n+static int commit_packed_refs(void)\n {\n \tstruct packed_ref_cache *packed_ref_cache =\n \t\tget_packed_ref_cache(&ref_cache);\n@@ -2445,7 +2445,7 @@ int commit_packed_refs(void)\n \treturn error;\n }\n \n-void rollback_packed_refs(void)\n+static void rollback_packed_refs(void)\n {\n \tstruct packed_ref_cache *packed_ref_cache =\n \t\tget_packed_ref_cache(&ref_cache);\ndiff --git a/refs.h b/refs.h\nindex db435bf..44ae7fe 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -120,36 +120,6 @@ extern void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refn\n extern void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, const struct string_list *refnames);\n \n /*\n- * Lock the packed-refs file for writing. Flags is passed to\n- * hold_lock_file_for_update(). Return 0 on success.\n- * Errno is set to something meaningful on error.\n- */\n-extern int lock_packed_refs(int flags);\n-\n-/*\n- * Add a reference to the in-memory packed reference cache. This may\n- * only be called while the packed-refs file is locked (see\n- * lock_packed_refs()). To actually write the packed-refs file, call\n- * commit_packed_refs().\n- */\n-extern void add_packed_ref(const char *refname, const unsigned char *sha1);\n-\n-/*\n- * Write the current version of the packed refs cache from memory to\n- * disk. The packed-refs file must already be locked for writing (see\n- * lock_packed_refs()). Return zero on success.\n- * Sets errno to something meaningful on error.\n- */\n-extern int commit_packed_refs(void);\n-\n-/*\n- * Rollback the lockfile for the packed-refs file, and discard the\n- * in-memory packed reference cache. (The packed-refs file will be\n- * read anew if it is needed again after this function is called.)\n- */\n-extern void rollback_packed_refs(void);\n-\n-/*\n * Flags for controlling behaviour of pack_refs()\n * PACK_REFS_PRUNE: Prune loose refs after packing\n * PACK_REFS_ALL: Pack _all_ refs, not just tags and already packed refs\n-- \n2.1.0.rc2.206.gedb03e5\n"},{"id":"250937","messageId":"1413923820-14457-13-git-send-email-sahlberg@google.com","threadId":"37776","inReplyTo":"1413923820-14457-1-git-send-email-sahlberg@google.com","subject":"[PATCH 12/15] refs.c: replace the onerr argument in update_ref with a strbuf err","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-10-21T20:36:57Z","receivedAt":"2014-10-21T20:36:57Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Get rid of the action_on_err enum and replace the action argument to\nupdate_ref with a strbuf *err for error reporting.\n\nUpdate all callers to the new api including two callers in transport*.c\nwhich used the literal 0 instead of an enum.\n\nChange-Id: I8ee1540380393380b5a06db380fb8491bfe1ecdd\nSigned-off-by: Ronnie Sahlberg \n---\n builtin/checkout.c | 7 +++++--\n builtin/clone.c | 20 ++++++++++++--------\n builtin/merge.c | 20 +++++++++++++-------\n builtin/notes.c | 24 ++++++++++++++----------\n builtin/reset.c | 12 ++++++++----\n builtin/update-ref.c | 7 +++++--\n notes-cache.c | 2 +-\n notes-utils.c | 5 +++--\n refs.c | 14 +++-----------\n refs.h | 10 ++--------\n transport-helper.c | 7 ++++++-\n transport.c | 9 ++++++---\n 12 files changed, 78 insertions(+), 59 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 8550b6d..60a68f7 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -584,6 +584,8 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n {\n \tstruct strbuf msg = STRBUF_INIT;\n \tconst char *old_desc, *reflog_msg;\n+\tstruct strbuf err = STRBUF_INIT;\n+\n \tif (opts->new_branch) {\n \t\tif (opts->new_orphan_branch) {\n \t\t\tif (opts->new_branch_log && !log_all_ref_updates) {\n@@ -621,8 +623,9 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n \tif (!strcmp(new->name, \"HEAD\") && !new->path && !opts->force_detach) {\n \t\t/* Nothing to do. */\n \t} else if (opts->force_detach || !new->path) {\t/* No longer on any branch. */\n-\t\tupdate_ref(msg.buf, \"HEAD\", new->commit->object.sha1, NULL,\n-\t\t\t REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);\n+\t\tif (update_ref(msg.buf, \"HEAD\", new->commit->object.sha1, NULL,\n+\t\t\t REF_NODEREF, &err))\n+\t\t\tdie(\"%s\", err.buf);\n \t\tif (!opts->quiet) {\n \t\t\tif (old->path && advice_detached_head)\n \t\t\t\tdetach_advice(new->name);\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 5052fac..e0a671d 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -522,6 +522,7 @@ static void write_remote_refs(const struct ref *local_refs)\n static void write_followtags(const struct ref *refs, const char *msg)\n {\n \tconst struct ref *ref;\n+\tstruct strbuf err = STRBUF_INIT;\n \tfor (ref = refs; ref; ref = ref->next) {\n \t\tif (!starts_with(ref->name, \"refs/tags/\"))\n \t\t\tcontinue;\n@@ -529,8 +530,9 @@ static void write_followtags(const struct ref *refs, const char *msg)\n \t\t\tcontinue;\n \t\tif (!has_sha1_file(ref->old_sha1))\n \t\t\tcontinue;\n-\t\tupdate_ref(msg, ref->name, ref->old_sha1,\n-\t\t\t NULL, 0, UPDATE_REFS_DIE_ON_ERR);\n+\t\tif (update_ref(msg, ref->name, ref->old_sha1,\n+\t\t\t NULL, 0, &err))\n+\t\t\tdie(\"%s\", err.buf);\n \t}\n }\n \n@@ -593,28 +595,30 @@ static void update_remote_refs(const struct ref *refs,\n static void update_head(const struct ref *our, const struct ref *remote,\n \t\t\tconst char *msg)\n {\n+\tstruct strbuf err = STRBUF_INIT;\n \tconst char *head;\n \tif (our && skip_prefix(our->name, \"refs/heads/\", &head)) {\n \t\t/* Local default branch link */\n \t\tcreate_symref(\"HEAD\", our->name, NULL);\n \t\tif (!option_bare) {\n-\t\t\tupdate_ref(msg, \"HEAD\", our->old_sha1, NULL, 0,\n-\t\t\t\t UPDATE_REFS_DIE_ON_ERR);\n+\t\t\tupdate_ref(msg, \"HEAD\", our->old_sha1, NULL, 0, &err);\n \t\t\tinstall_branch_config(0, head, option_origin, our->name);\n \t\t}\n \t} else if (our) {\n \t\tstruct commit *c = lookup_commit_reference(our->old_sha1);\n \t\t/* --branch specifies a non-branch (i.e. tags), detach HEAD */\n-\t\tupdate_ref(msg, \"HEAD\", c->object.sha1,\n-\t\t\t NULL, REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);\n+\t\tif (update_ref(msg, \"HEAD\", c->object.sha1,\n+\t\t\t NULL, REF_NODEREF, &err))\n+\t\t\tdie(\"%s\", err.buf);\n \t} else if (remote) {\n \t\t/*\n \t\t * We know remote HEAD points to a non-branch, or\n \t\t * HEAD points to a branch but we don't know which one.\n \t\t * Detach HEAD in all these cases.\n \t\t */\n-\t\tupdate_ref(msg, \"HEAD\", remote->old_sha1,\n-\t\t\t NULL, REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);\n+\t if (update_ref(msg, \"HEAD\", remote->old_sha1,\n+\t\t\t NULL, REF_NODEREF, &err))\n+\t\tdie(\"%s\", err.buf);\n \t}\n }\n \ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex bebbe5b..a787b6a 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -396,9 +396,11 @@ static void finish(struct commit *head_commit,\n \t\t\tprintf(_(\"No merge message -- not updating HEAD\\n\"));\n \t\telse {\n \t\t\tconst char *argv_gc_auto[] = { \"gc\", \"--auto\", NULL };\n-\t\t\tupdate_ref(reflog_message.buf, \"HEAD\",\n-\t\t\t\tnew_head, head, 0,\n-\t\t\t\tUPDATE_REFS_DIE_ON_ERR);\n+\t\t\tstruct strbuf err = STRBUF_INIT;\n+\t\t\tif (update_ref(reflog_message.buf, \"HEAD\",\n+\t\t\t\t new_head, head, 0,\n+\t\t\t\t &err))\n+\t\t\t\tdie(\"%s\", err.buf);\n \t\t\t/*\n \t\t\t * We ignore errors in 'gc --auto', since the\n \t\t\t * user should see them.\n@@ -1086,6 +1088,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tunsigned char head_sha1[20];\n \tstruct commit *head_commit;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tstruct strbuf err = STRBUF_INIT;\n \tconst char *head_arg;\n \tint flag, i, ret = 0, head_subsumed;\n \tint best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;\n@@ -1214,8 +1217,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\tif (!remote_head)\n \t\t\tdie(_(\"%s - not something we can merge\"), argv[0]);\n \t\tread_empty(remote_head->object.sha1, 0);\n-\t\tupdate_ref(\"initial pull\", \"HEAD\", remote_head->object.sha1,\n-\t\t\t NULL, 0, UPDATE_REFS_DIE_ON_ERR);\n+\t\tif (update_ref(\"initial pull\", \"HEAD\", remote_head->object.sha1,\n+\t\t\t NULL, 0, &err))\n+\t\t\tdie(\"%s\", err.buf);\n \t\tgoto done;\n \t} else {\n \t\tstruct strbuf merge_names = STRBUF_INIT;\n@@ -1328,8 +1332,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\tfree(list);\n \t}\n \n-\tupdate_ref(\"updating ORIG_HEAD\", \"ORIG_HEAD\", head_commit->object.sha1,\n-\t\t NULL, 0, UPDATE_REFS_DIE_ON_ERR);\n+\tif (update_ref(\"updating ORIG_HEAD\", \"ORIG_HEAD\",\n+\t\t head_commit->object.sha1,\n+\t\t NULL, 0, &err))\n+\t\tdie(\"%s\", err.buf);\n \n \tif (remoteheads && !common)\n \t\t; /* No common ancestors found. We need a real merge. */\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 68b6cd8..b9fec39 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -674,6 +674,7 @@ static int merge_abort(struct notes_merge_options *o)\n static int merge_commit(struct notes_merge_options *o)\n {\n \tstruct strbuf msg = STRBUF_INIT;\n+\tstruct strbuf err = STRBUF_INIT;\n \tunsigned char sha1[20], parent_sha1[20];\n \tstruct notes_tree *t;\n \tstruct commit *partial;\n@@ -714,10 +715,10 @@ static int merge_commit(struct notes_merge_options *o)\n \tformat_commit_message(partial, \"%s\", &msg, &pretty_ctx);\n \tstrbuf_trim(&msg);\n \tstrbuf_insert(&msg, 0, \"notes: \", 7);\n-\tupdate_ref(msg.buf, o->local_ref, sha1,\n-\t\t is_null_sha1(parent_sha1) ? NULL : parent_sha1,\n-\t\t 0, UPDATE_REFS_DIE_ON_ERR);\n-\n+\tif (update_ref(msg.buf, o->local_ref, sha1,\n+\t\t is_null_sha1(parent_sha1) ? NULL : parent_sha1,\n+\t\t 0, &err))\n+\t\tdie(\"%s\", err.buf);\n \tfree_notes(t);\n \tstrbuf_release(&msg);\n \tret = merge_abort(o);\n@@ -728,6 +729,7 @@ static int merge_commit(struct notes_merge_options *o)\n static int merge(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT;\n+\tstruct strbuf err = STRBUF_INIT;\n \tunsigned char result_sha1[20];\n \tstruct notes_tree *t;\n \tstruct notes_merge_options o;\n@@ -808,14 +810,16 @@ static int merge(int argc, const char **argv, const char *prefix)\n \n \tresult = notes_merge(&o, t, result_sha1);\n \n-\tif (result >= 0) /* Merge resulted (trivially) in result_sha1 */\n+\tif (result >= 0) {/* Merge resulted (trivially) in result_sha1 */\n \t\t/* Update default notes ref with new commit */\n-\t\tupdate_ref(msg.buf, default_notes_ref(), result_sha1, NULL,\n-\t\t\t 0, UPDATE_REFS_DIE_ON_ERR);\n-\telse { /* Merge has unresolved conflicts */\n+\t\tif (update_ref(msg.buf, default_notes_ref(), result_sha1, NULL,\n+\t\t\t 0, &err))\n+\t\t\tdie(\"%s\", err.buf);\n+\t} else { /* Merge has unresolved conflicts */\n \t\t/* Update .git/NOTES_MERGE_PARTIAL with partial merge result */\n-\t\tupdate_ref(msg.buf, \"NOTES_MERGE_PARTIAL\", result_sha1, NULL,\n-\t\t\t 0, UPDATE_REFS_DIE_ON_ERR);\n+\t\tif (update_ref(msg.buf, \"NOTES_MERGE_PARTIAL\", result_sha1, NULL,\n+\t\t\t 0, &err))\n+\t\t\tdie(\"%s\", err.buf);\n \t\t/* Store ref-to-be-updated into .git/NOTES_MERGE_REF */\n \t\tif (create_symref(\"NOTES_MERGE_REF\", default_notes_ref(), NULL))\n \t\t\tdie(\"Failed to store link to current notes ref (%s)\",\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 4c08ddc..8ebf4ca 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -245,6 +245,7 @@ static int reset_refs(const char *rev, const unsigned char *sha1)\n {\n \tint update_ref_status;\n \tstruct strbuf msg = STRBUF_INIT;\n+\tstruct strbuf err = STRBUF_INIT;\n \tunsigned char *orig = NULL, sha1_orig[20],\n \t\t*old_orig = NULL, sha1_old_orig[20];\n \n@@ -253,13 +254,16 @@ static int reset_refs(const char *rev, const unsigned char *sha1)\n \tif (!get_sha1(\"HEAD\", sha1_orig)) {\n \t\torig = sha1_orig;\n \t\tset_reflog_message(&msg, \"updating ORIG_HEAD\", NULL);\n-\t\tupdate_ref(msg.buf, \"ORIG_HEAD\", orig, old_orig, 0,\n-\t\t\t UPDATE_REFS_MSG_ON_ERR);\n+\t\tif (update_ref(msg.buf, \"ORIG_HEAD\", orig, old_orig, 0, &err))\n+\t\t\terror(\"%s\", err.buf);\n+\t\tstrbuf_release(&err);\n \t} else if (old_orig)\n \t\tdelete_ref(\"ORIG_HEAD\", old_orig, 0);\n \tset_reflog_message(&msg, \"updating HEAD\", rev);\n-\tupdate_ref_status = update_ref(msg.buf, \"HEAD\", sha1, orig, 0,\n-\t\t\t\t UPDATE_REFS_MSG_ON_ERR);\n+\tupdate_ref_status = update_ref(msg.buf, \"HEAD\", sha1, orig, 0, &err);\n+\tif (update_ref_status)\n+\t\terror(\"%s\", err.buf);\n+\tstrbuf_release(&err);\n \tstrbuf_release(&msg);\n \treturn update_ref_status;\n }\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex af08dd9..f650647 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -358,6 +358,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\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 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@@ -421,6 +422,8 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \tif (delete)\n \t\treturn delete_ref(refname, oldval ? oldsha1 : NULL, flags);\n \telse\n-\t\treturn update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,\n-\t\t\t\t flags, UPDATE_REFS_DIE_ON_ERR);\n+\t\tif (update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,\n+\t\t\t flags, &err))\n+\t\t\tdie(\"%s\", err.buf);\n+\treturn 0;\n }\ndiff --git a/notes-cache.c b/notes-cache.c\nindex c4e9bb7..386e6d6 100644\n--- a/notes-cache.c\n+++ b/notes-cache.c\n@@ -60,7 +60,7 @@ int notes_cache_write(struct notes_cache *c)\n \t\t\tcommit_sha1, NULL, NULL) < 0)\n \t\treturn -1;\n \tif (update_ref(\"update notes cache\", c->tree.ref, commit_sha1, NULL,\n-\t\t 0, UPDATE_REFS_QUIET_ON_ERR) < 0)\n+\t\t 0, NULL) < 0)\n \t\treturn -1;\n \n \treturn 0;\ndiff --git a/notes-utils.c b/notes-utils.c\nindex b64dc1b..bcfe61e 100644\n--- a/notes-utils.c\n+++ b/notes-utils.c\n@@ -34,6 +34,7 @@ void commit_notes(struct notes_tree *t, const char *msg)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tunsigned char commit_sha1[20];\n+\tstruct strbuf err = STRBUF_INIT;\n \n \tif (!t)\n \t\tt = &default_notes_tree;\n@@ -49,8 +50,8 @@ void commit_notes(struct notes_tree *t, const char *msg)\n \n \tcreate_notes_commit(t, NULL, buf.buf, buf.len, commit_sha1);\n \tstrbuf_insert(&buf, 0, \"notes: \", 7); /* commit message starts at index 7 */\n-\tupdate_ref(buf.buf, t->ref, commit_sha1, NULL, 0,\n-\t\t UPDATE_REFS_DIE_ON_ERR);\n+\tif (update_ref(buf.buf, t->ref, commit_sha1, NULL, 0, &err))\n+\t\tdie(\"%s\", err.buf);\n \n \tstrbuf_release(&buf);\n }\ndiff --git a/refs.c b/refs.c\nindex 1261a78..c7d0825 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3675,7 +3675,7 @@ int transaction_delete_ref(struct transaction *transaction,\n \n 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+\t int flags, struct strbuf *e)\n {\n \tstruct transaction *t;\n \tstruct strbuf err = STRBUF_INIT;\n@@ -3688,16 +3688,8 @@ int update_ref(const char *action, const char *refname,\n \t\tconst char *str = \"update_ref failed for ref '%s': %s\";\n \n \t\ttransaction_free(t);\n-\t\tswitch (onerr) {\n-\t\tcase UPDATE_REFS_MSG_ON_ERR:\n-\t\t\terror(str, refname, err.buf);\n-\t\t\tbreak;\n-\t\tcase UPDATE_REFS_DIE_ON_ERR:\n-\t\t\tdie(str, refname, err.buf);\n-\t\t\tbreak;\n-\t\tcase UPDATE_REFS_QUIET_ON_ERR:\n-\t\t\tbreak;\n-\t\t}\n+\t\tif (e)\n+\t\t\tstrbuf_addf(e, str, refname, err.buf);\n \t\tstrbuf_release(&err);\n \t\treturn 1;\n \t}\ndiff --git a/refs.h b/refs.h\nindex 44ae7fe..f3e08f5 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -212,12 +212,6 @@ extern int rename_ref(const char *oldref, const char *newref, const char *logmsg\n */\n extern int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *sha1);\n \n-enum action_on_err {\n-\tUPDATE_REFS_MSG_ON_ERR,\n-\tUPDATE_REFS_DIE_ON_ERR,\n-\tUPDATE_REFS_QUIET_ON_ERR\n-};\n-\n /*\n * Begin a reference transaction. The reference transaction must\n * be freed by calling transaction_free().\n@@ -340,8 +334,8 @@ void transaction_free(struct 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-\t\tint flags, enum action_on_err onerr);\n+\t const unsigned char *sha1, const unsigned char *oldval,\n+\t int flags, struct strbuf *err);\n \n extern int parse_hide_refs_config(const char *var, const char *value, const char *);\n extern int ref_is_hidden(const char *);\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 9b29620..653cf74 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -735,6 +735,7 @@ static int push_update_refs_status(struct helper_data *data,\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct ref *ref = remote_refs;\n+\tstruct strbuf err = STRBUF_INIT;\n \tint ret = 0;\n \n \tfor (;;) {\n@@ -758,7 +759,11 @@ static int push_update_refs_status(struct helper_data *data,\n \t\tprivate = apply_refspecs(data->refspecs, data->refspec_nr, ref->name);\n \t\tif (!private)\n \t\t\tcontinue;\n-\t\tupdate_ref(\"update by helper\", private, ref->new_sha1, NULL, 0, 0);\n+\t\tif (update_ref(\"update by helper\", private, ref->new_sha1,\n+\t\t\t NULL, 0, &err))\n+\t\t\terror(\"%s\", err.buf);\n+\t\tstrbuf_release(&err);\n+\n \t\tfree(private);\n \t}\n \tstrbuf_release(&buf);\ndiff --git a/transport.c b/transport.c\nindex b56620e..51570d6 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -597,6 +597,7 @@ int transport_refs_pushed(struct ref *ref)\n void transport_update_tracking_ref(struct remote *remote, struct ref *ref, int verbose)\n {\n \tstruct refspec rs;\n+\tstruct strbuf err = STRBUF_INIT;\n \n \tif (ref->status != REF_STATUS_OK && ref->status != REF_STATUS_UPTODATE)\n \t\treturn;\n@@ -609,9 +610,11 @@ void transport_update_tracking_ref(struct remote *remote, struct ref *ref, int v\n \t\t\tfprintf(stderr, \"updating local tracking ref '%s'\\n\", rs.dst);\n \t\tif (ref->deletion) {\n \t\t\tdelete_ref(rs.dst, NULL, 0);\n-\t\t} else\n-\t\t\tupdate_ref(\"update by push\", rs.dst,\n-\t\t\t\t\tref->new_sha1, NULL, 0, 0);\n+\t\t} else if (update_ref(\"update by push\", rs.dst,\n+\t\t\t\t ref->new_sha1, NULL, 0, &err))\n+\t\t\terror(\"%s\", err.buf);\n+\n+\t\tstrbuf_release(&err);\n \t\tfree(rs.dst);\n \t}\n }\n-- \n2.1.0.rc2.206.gedb03e5\n"},{"id":"250933","messageId":"1413923820-14457-14-git-send-email-sahlberg@google.com","threadId":"37776","inReplyTo":"1413923820-14457-1-git-send-email-sahlberg@google.com","subject":"[PATCH 13/15] refs.c: make add_packed_ref return an error instead of calling die","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-10-21T20:36:58Z","receivedAt":"2014-10-21T20:36:58Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change add_packed_ref to return an error instead of calling die().\nUpdate all callers to check the return value of add_packed_ref.\n\nChange-Id: I5d5e4a75f641c4bbdb59199a233b71e86361c25c\nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 21 +++++++++++++++++----\n 1 file changed, 17 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex c7d0825..e0d5a82 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1224,15 +1224,16 @@ static struct ref_dir *get_packed_refs(struct ref_cache *refs)\n \treturn get_packed_ref_dir(get_packed_ref_cache(refs));\n }\n \n-static void add_packed_ref(const char *refname, const unsigned char *sha1)\n+static int add_packed_ref(const char *refname, const unsigned char *sha1)\n {\n \tstruct packed_ref_cache *packed_ref_cache =\n \t\tget_packed_ref_cache(&ref_cache);\n \n \tif (!packed_ref_cache->lock)\n-\t\tdie(\"internal error: packed refs not locked\");\n+\t\treturn -1;\n \tadd_ref(get_packed_ref_dir(packed_ref_cache),\n \t\tcreate_ref_entry(refname, sha1, REF_ISPACKED, 1));\n+\treturn 0;\n }\n \n /*\n@@ -3795,7 +3796,13 @@ int transaction_commit(struct transaction *transaction,\n \t\t\t\t\tsha1, NULL))\n \t\t\tcontinue;\n \n-\t\tadd_packed_ref(update->refname, sha1);\n+\t\tif (add_packed_ref(update->refname, sha1)) {\n+\t\t\tif (err)\n+\t\t\t\tstrbuf_addf(err, \"Failed to add %s to packed \"\n+\t\t\t\t\t \"refs\", update->refname);\n+\t\t\tret = -1;\n+\t\t\tgoto cleanup;\n+\t\t}\n \t\tneed_repack = 1;\n \t}\n \tif (need_repack) {\n@@ -3909,7 +3916,13 @@ int transaction_commit(struct transaction *transaction,\n \n \t\tpacked = get_packed_refs(&ref_cache);\n \t\tremove_entry(packed, update->refname);\n-\t\tadd_packed_ref(update->refname, update->new_sha1);\n+\t\tif (add_packed_ref(update->refname, update->new_sha1)) {\n+\t\t\tif (err)\n+\t\t\t\tstrbuf_addf(err, \"Failed to add %s to packed \"\n+\t\t\t\t\t \"refs\", update->refname);\n+\t\t\tret = -1;\n+\t\t\tgoto cleanup;\n+\t\t}\n \t\tneed_repack = 1;\n \n \t\ttry_remove_empty_parents((char *)update->refname);\n-- \n2.1.0.rc2.206.gedb03e5\n"},{"id":"250927","messageId":"1413923820-14457-15-git-send-email-sahlberg@google.com","threadId":"37776","inReplyTo":"1413923820-14457-1-git-send-email-sahlberg@google.com","subject":"[PATCH 14/15] refs.c: make lock_packed_refs take an err argument","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-10-21T20:36:59Z","receivedAt":"2014-10-21T20:36:59Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change-Id: Ibfc64ab19257484129ab0ad861b72c02414388df\nSigned-off-by: Ronnie Sahlberg \n---\n refs.c | 25 +++++++++++++------------\n 1 file changed, 13 insertions(+), 12 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex e0d5a82..7fb0d6c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2393,13 +2393,17 @@ static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)\n \treturn 0;\n }\n \n-/* This should return a meaningful errno on failure */\n-static int lock_packed_refs(int flags)\n+static int lock_packed_refs(struct strbuf *err)\n {\n \tstruct packed_ref_cache *packed_ref_cache;\n \n-\tif (hold_lock_file_for_update(&packlock, git_path(\"packed-refs\"), flags) < 0)\n+\tif (hold_lock_file_for_update(&packlock, git_path(\"packed-refs\"),\n+\t\t\t\t 0) < 0) {\n+\t\tif (err)\n+\t\t\tunable_to_lock_message(git_path(\"packed-refs\"),\n+\t\t\t\t\t errno, err);\n \t\treturn -1;\n+\t}\n \t/*\n \t * Get the current packed-refs while holding the lock. If the\n \t * packed-refs file has been modified since we last read it,\n@@ -2587,11 +2591,14 @@ static void prune_refs(struct ref_to_prune *r)\n int pack_refs(unsigned int flags)\n {\n \tstruct pack_refs_cb_data cbdata;\n+\tstruct strbuf err = STRBUF_INIT;\n \n \tmemset(&cbdata, 0, sizeof(cbdata));\n \tcbdata.flags = flags;\n \n-\tlock_packed_refs(LOCK_DIE_ON_ERROR);\n+\tif (lock_packed_refs(&err))\n+\t\tdie(\"%s\", err.buf);\n+\n \tcbdata.packed_refs = get_packed_refs(&ref_cache);\n \n \tdo_for_each_entry_in_dir(get_loose_refs(&ref_cache), 0,\n@@ -3757,10 +3764,7 @@ int transaction_commit(struct transaction *transaction,\n \t}\n \n \t/* Lock packed refs during commit */\n-\tif (lock_packed_refs(0)) {\n-\t\tif (err)\n-\t\t\tunable_to_lock_message(git_path(\"packed-refs\"),\n-\t\t\t\t\t errno, err);\n+\tif (lock_packed_refs(err)) {\n \t\tret = -1;\n \t\tgoto cleanup;\n \t}\n@@ -3815,10 +3819,7 @@ int transaction_commit(struct transaction *transaction,\n \t\t\tgoto cleanup;\n \t\t}\n \t\t/* lock the packed refs again so no one can change it */\n-\t\tif (lock_packed_refs(0)) {\n-\t\t\tif (err)\n-\t\t\t\tunable_to_lock_message(git_path(\"packed-refs\"),\n-\t\t\t\t\t\t errno, err);\n+\t\tif (lock_packed_refs(err)) {\n \t\t\tret = -1;\n \t\t\tgoto cleanup;\n \t\t}\n-- \n2.1.0.rc2.206.gedb03e5\n"},{"id":"250936","messageId":"1413923820-14457-16-git-send-email-sahlberg@google.com","threadId":"37776","inReplyTo":"1413923820-14457-1-git-send-email-sahlberg@google.com","subject":"[PATCH 15/15] refs.c: add an err argument to pack_refs","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-10-21T20:37:00Z","receivedAt":"2014-10-21T20:37:00Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change-Id: I1e65ac429c14f01073d95c6440f820dda1c6091b\nSigned-off-by: Ronnie Sahlberg \n---\n builtin/pack-refs.c | 8 +++++++-\n refs.c | 7 +++----\n refs.h | 3 ++-\n 3 files changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/pack-refs.c b/builtin/pack-refs.c\nindex b20b1ec..299768e 100644\n--- a/builtin/pack-refs.c\n+++ b/builtin/pack-refs.c\n@@ -10,6 +10,7 @@ static char const * const pack_refs_usage[] = {\n int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n {\n \tunsigned int flags = PACK_REFS_PRUNE;\n+\tstruct strbuf err = STRBUF_INIT;\n \tstruct option opts[] = {\n \t\tOPT_BIT(0, \"all\", &flags, N_(\"pack everything\"), PACK_REFS_ALL),\n \t\tOPT_BIT(0, \"prune\", &flags, N_(\"prune loose refs (default)\"), PACK_REFS_PRUNE),\n@@ -17,5 +18,10 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n \t};\n \tif (parse_options(argc, argv, prefix, opts, pack_refs_usage, 0))\n \t\tusage_with_options(pack_refs_usage, opts);\n-\treturn pack_refs(flags);\n+\tif (pack_refs(flags, &err)) {\n+\t\terror(\"%s\", err.buf);\n+\t\tstrbuf_release(&err);\n+\t\treturn -1;\n+\t}\n+\treturn 0;\n }\ndiff --git a/refs.c b/refs.c\nindex 7fb0d6c..a5e1eff 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2588,16 +2588,15 @@ static void prune_refs(struct ref_to_prune *r)\n \t}\n }\n \n-int pack_refs(unsigned int flags)\n+int pack_refs(unsigned int flags, struct strbuf *err)\n {\n \tstruct pack_refs_cb_data cbdata;\n-\tstruct strbuf err = STRBUF_INIT;\n \n \tmemset(&cbdata, 0, sizeof(cbdata));\n \tcbdata.flags = flags;\n \n-\tif (lock_packed_refs(&err))\n-\t\tdie(\"%s\", err.buf);\n+\tif (lock_packed_refs(err))\n+\t\treturn -1;\n \n \tcbdata.packed_refs = get_packed_refs(&ref_cache);\n \ndiff --git a/refs.h b/refs.h\nindex f3e08f5..be16c08 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -130,8 +130,9 @@ extern void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, const struct st\n /*\n * Write a packed-refs file for the current repository.\n * flags: Combination of the above PACK_REFS_* flags.\n+ * Returns 0 on success and fills in err on failure.\n */\n-int pack_refs(unsigned int flags);\n+int pack_refs(unsigned int flags, struct strbuf *err);\n \n extern int ref_exists(const char *);\n \n-- \n2.1.0.rc2.206.gedb03e5\n"},{"id":"250975","messageId":"xmqqy4s7homc.fsf@gitster.dls.corp.google.com","threadId":"37776","inReplyTo":"1413923820-14457-4-git-send-email-sahlberg@google.com","subject":"Re: [PATCH 03/15] refs.c: use packed refs when deleting refs during a transaction","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-22T19:48:43Z","receivedAt":"2014-10-22T19:48:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ronnie Sahlberg writes:\n\n> commit fb5fa1d338ce113b0fea3bb955b50bbb3e827805 upstream.\n\nHuh?\n\n> Make the deletion of refs during a transaction more atomic.\n> Start by first copying all loose refs we will be deleting to the packed\n> refs file and then commit the packed refs file. Then re-lock the packed refs\n> file to stop anyone else from modifying these refs and keep it locked until\n> we are finished.\n> Since all refs we are about to delete are now safely held in the packed refs\n> file we can proceed to immediately unlink any corresponding loose refs\n> and still be fully rollback-able.\n>\n> The exception is for refs that can not be resolved. Those refs are never\n> added to the packed refs and will just be un-rollback-ably deleted during\n> commit.\n>\n> By deleting all the loose refs at the start of the transaction we make make\n> it possible to both delete one ref and then re-create a different ref in\n> the same transaction even if the two refs would normally conflict.\n>\n> Example: rename m->m/m\n>\n> In that example we want to delete the file 'm' so that we make room so\n> that we can create a directory with the same name in order to lock and\n> write to the ref m/m and its lock-file m/m.lock.\n>\n> If there is a failure during the commit phase we can rollback without losing\n> any refs since we have so far only deleted loose refs that that are\n> guaranteed to also have a corresponding entry in the packed refs file.\n> Once we have finished all updates for refs and their reflogs we can repack\n> the packed refs file and remove the to-be-deleted refs from the packed refs,\n> at which point all the deleted refs will disappear in one atomic rename\n> operation.\n>\n> This also means that for an outside observer, deletion of multiple refs\n> in a single transaction will look atomic instead of one ref being deleted\n> at a time.\n>\n> In order to do all this we need to change the semantics for the\n> repack_without_refs function so that we can lock the packed refs file,\n> do other stuff, and later be able to call repack_without_refs with the\n> lock already taken.\n> This means we need some additional changes in remote.c to reflect the\n> changes to the repack_without_refs semantics.\n>\n> Change-Id: I1e4f58050acaabc6bcfa3409fbc0c2b866bb59aa\n> Signed-off-by: Ronnie Sahlberg \n> Signed-off-by: Jonathan Nieder \n\n\n> @@ -3821,23 +3824,109 @@ int transaction_commit(struct transaction *transaction,\n> ...\n> +\tif (need_repack) {\n> +\t\tpacked = get_packed_refs(&ref_cache);;\n> +\t\tsort_ref_dir(packed);\n> +\t\tif (commit_packed_refs()){\n\nSP between \"){\".\n\n> +\t\t\tstrbuf_addf(err, \"unable to overwrite old ref-pack \"\n> +\t\t\t\t \"file\");\n\nThis is bending backwards sacrificing readability; 80-col limit is\nnot that hard a limit. Split at the comma instead, perhaps?\n\n\t\t\tstrbuf_addf(err,\n\t\t\t\t \"unable to overwrite old ref-pack file\");\n\n> ...\n> +\t\tif (!update->lock) {\n> +\t\t\tint df_conflict = (errno == ENOTDIR);\n> +\n> +\t\t\tstrbuf_addf(err, \"Cannot lock the ref '%s'.\",\n> +\t\t\t\t update->refname);\n> +\t\t\tret = df_conflict ?\n> +\t\t\t TRANSACTION_NAME_CONFLICT : \n\nA trailing SP here...\n\n> @@ -3860,6 +3953,16 @@ int transaction_commit(struct transaction *transaction,\n> \t\t\tupdate->reflog_lock = update->orig_update->reflog_lock;\n> \t\t\tcontinue;\n> \t\t}\n> +\t\tif (log_all_ref_updates && !reflog_exists(update->refname) &&\n> +\t\t create_reflog(update->refname)) {\n> +\t\t\tret = -1;\n> +\t\t\tif (err)\n> +\t\t\t\tstrbuf_addf(err, \"Failed to setup reflog for \"\n> +\t\t\t\t\t \"%s\", update->refname);\n\nSplit after comma, perhaps?\n"},{"id":"251153","messageId":"xmqqppdcj9m9.fsf@gitster.dls.corp.google.com","threadId":"37776","inReplyTo":"1413923820-14457-6-git-send-email-sahlberg@google.com","subject":"Re: [PATCH 05/15] refs.c: update rename_ref to use a transaction","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-28T19:07:58Z","receivedAt":"2014-10-28T19:07:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ronnie Sahlberg writes:\n\n> commit 0295e9cebc41020ee84da275549b164a8770ffba upstream.\n>\n> Change refs.c to use a single transaction to copy/rename both the refs and\n> its reflog. Since we are no longer using rename() to move the reflog file\n> we no longer need to disallow rename_ref for refs with a symlink for its\n> reflog so we can remove that test from the testsuite.\n\nDo you mean that we used to do a single rename(2) to move the entire\nlogfile, but now you copy potentially thousands of reflog entries\none by one?\n\nHmmmm,... is that an improvement?\n"},{"id":"251155","messageId":"xmqqlho0j7dq.fsf@gitster.dls.corp.google.com","threadId":"37776","inReplyTo":"xmqqppdcj9m9.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 05/15] refs.c: update rename_ref to use a transaction","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-28T19:56:17Z","receivedAt":"2014-10-28T19:56:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Ronnie Sahlberg writes:\n>\n>> commit 0295e9cebc41020ee84da275549b164a8770ffba upstream.\n>>\n>> Change refs.c to use a single transaction to copy/rename both the refs and\n>> its reflog. Since we are no longer using rename() to move the reflog file\n>> we no longer need to disallow rename_ref for refs with a symlink for its\n>> reflog so we can remove that test from the testsuite.\n>\n> Do you mean that we used to do a single rename(2) to move the entire\n> logfile, but now you copy potentially thousands of reflog entries\n> one by one?\n>\n> Hmmmm,... is that an improvement?\n\nI see some value in \"keep the original while creating a new one,\njust in case we fail to fully recreate the new one so that we can\nroll back with less programming effort\". But still, we should be\nable to copy the original to new without parsing and reformatting\neach and every entry, no?\n"},{"id":"251158","messageId":"CAL=YDWm05PyO07HbiOTiweh+3AEvXnbptbzoreLw-b9YUrm-Hg@mail.gmail.com","threadId":"37776","inReplyTo":"xmqqlho0j7dq.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 05/15] refs.c: update rename_ref to use a transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-10-28T20:56:16Z","receivedAt":"2014-10-28T20:56:16Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Tue, Oct 28, 2014 at 12:56 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Ronnie Sahlberg writes:\n>>\n>>> commit 0295e9cebc41020ee84da275549b164a8770ffba upstream.\n>>>\n>>> Change refs.c to use a single transaction to copy/rename both the refs and\n>>> its reflog. Since we are no longer using rename() to move the reflog file\n>>> we no longer need to disallow rename_ref for refs with a symlink for its\n>>> reflog so we can remove that test from the testsuite.\n>>\n>> Do you mean that we used to do a single rename(2) to move the entire\n>> logfile, but now you copy potentially thousands of reflog entries\n>> one by one?\n>>\n>> Hmmmm,... is that an improvement?\n\nI think so. It makes to code a lot simpler and more atomic. As a side\neffect it removes restrictions for symlink handling and eliminates the\ntwo renames colliding race.\nThough, a read and then rewrite thousands of reflog entries will be\nslower than a single rename() syscall.\n\n>\n> I see some value in \"keep the original while creating a new one,\n> just in case we fail to fully recreate the new one so that we can\n> roll back with less programming effort\". But still, we should be\n> able to copy the original to new without parsing and reformatting\n> each and every entry, no?\n\nIs renaming a branch with a long history is such a frequent or time\ncritical event?\nI timed a git branch -m for a branch with ~2400 log entries and it\ntakes neglible time :\n real 0m0.008s\n user 0m0.000s\n sys 0m0.007s\n\n\nDuring the special rename case, we are deleting one ref and creating\nanother. For cases such as m->m/m or the reverse we must delete the\nold file/directory before we can create the new one.\n\nThe old rename code did this by renaming the file out to a common\ndirectory and then back to the new location. Which is fast (but a bit\n...)\nThe alternative is to read the old file into memory, delete it and\nthen write the content back to the new location, which is kind of what\nthe new code does.\n\nIf this turns out to be a bottleneck we can change the io when writing\nthe reflog entries to use fwrite(). Lets see if there is a problem\nfirst.\n\nregards\nronnie sahlberg\n"},{"id":"251159","messageId":"xmqqh9ynkiem.fsf@gitster.dls.corp.google.com","threadId":"37776","inReplyTo":"CAL=YDWm05PyO07HbiOTiweh+3AEvXnbptbzoreLw-b9YUrm-Hg@mail.gmail.com","subject":"Re: [PATCH 05/15] refs.c: update rename_ref to use a transaction","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-28T21:12:49Z","receivedAt":"2014-10-28T21:12:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ronnie Sahlberg writes:\n\n> I timed a git branch -m for a branch with ~2400 log entries and it\n> takes neglible time :\n> real 0m0.008s\n> user 0m0.000s\n> sys 0m0.007s\n\nI really hate this line of reasoning. Small things tend to add up.\n\nMore importantly, when you know that the end result you want to see\nis that the old and new log files are bit-for-bit identical, and if\nnot there is some bug in either parsing or formatting, why parse the\nold and reformat into the new? What would happen when there were\nmalformed entries in the old that makes your parsing fail?\n"},{"id":"251179","messageId":"CAL=YDWkOZ29+ikXJUzhZqW8-Mk91Z_E1QCiXxT1HZ1oj04pk0w@mail.gmail.com","threadId":"37776","inReplyTo":"xmqqh9ynkiem.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 05/15] refs.c: update rename_ref to use a transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-10-29T17:18:37Z","receivedAt":"2014-10-29T17:18:37Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Tue, Oct 28, 2014 at 2:12 PM, Junio C Hamano wrote:\n> Ronnie Sahlberg writes:\n>\n>> I timed a git branch -m for a branch with ~2400 log entries and it\n>> takes neglible time :\n>> real 0m0.008s\n>> user 0m0.000s\n>> sys 0m0.007s\n>\n> I really hate this line of reasoning. Small things tend to add up.\n>\n> More importantly, when you know that the end result you want to see\n> is that the old and new log files are bit-for-bit identical, and if\n> not there is some bug in either parsing or formatting, why parse the\n> old and reformat into the new? What would happen when there were\n> malformed entries in the old that makes your parsing fail?\n>\n\nFair enough. I will change it to ONLY use a transaction for the actual\nref update and keep using rename() for the reflog handling.\nOnly real change I will do for the reflog handling is to change the\ntemporary file name used to be less collission prone if there are two\nrenames happening at the same time\nso that they don't destroy each others reflogs.\n"},{"id":"251182","messageId":"xmqqtx2myawy.fsf@gitster.dls.corp.google.com","threadId":"37776","inReplyTo":"CAL=YDWkOZ29+ikXJUzhZqW8-Mk91Z_E1QCiXxT1HZ1oj04pk0w@mail.gmail.com","subject":"Re: [PATCH 05/15] refs.c: update rename_ref to use a transaction","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-29T18:43:09Z","receivedAt":"2014-10-29T18:43:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ronnie Sahlberg writes:\n\n> On Tue, Oct 28, 2014 at 2:12 PM, Junio C Hamano wrote:\n>\n>> More importantly, when you know that the end result you want to see\n>> is that the old and new log files are bit-for-bit identical, and if\n>> not there is some bug in either parsing or formatting, why parse the\n>> old and reformat into the new? What would happen when there were\n>> malformed entries in the old that makes your parsing fail?\n>>\n>\n> Fair enough. I will change it to ONLY use a transaction for the actual\n> ref update and keep using rename() for the reflog handling.\n> Only real change I will do for the reflog handling is to change the\n> temporary file name used to be less collission prone if there are two\n> renames happening at the same time\n> so that they don't destroy each others reflogs.\n\nI think it is a good idea to make renaming the entire reflog a\nlogical element of transaction (as you mentioned in our private\ndiscussion) to allow different backends implement in their best\nefficient & robust way.\n\nAnd for filesystem-backed backends, I actually think \"keep the\noriginal until we know we do not have to roll back\", that follows\nthe same pattern for the other transactional updates, is a good\nimplementation of that \"best efficient & robust way\", compared to\nthe original \"just rename it\". It frees us from having to be\nworried about what happens if we cannot rename it back.\n\nThanks.\n"},{"id":"251231","messageId":"CAL=YDW=jk1wO8KB1=Wpiw-AKa9xzLJYr36zF6HxMDPxUHoP_6Q@mail.gmail.com","threadId":"37776","inReplyTo":"xmqqtx2myawy.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 05/15] refs.c: update rename_ref to use a transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-10-30T18:46:34Z","receivedAt":"2014-10-30T18:46:34Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Wed, Oct 29, 2014 at 11:43 AM, Junio C Hamano wrote:\n> Ronnie Sahlberg writes:\n>\n>> On Tue, Oct 28, 2014 at 2:12 PM, Junio C Hamano wrote:\n>>\n>>> More importantly, when you know that the end result you want to see\n>>> is that the old and new log files are bit-for-bit identical, and if\n>>> not there is some bug in either parsing or formatting, why parse the\n>>> old and reformat into the new? What would happen when there were\n>>> malformed entries in the old that makes your parsing fail?\n>>>\n>>\n>> Fair enough. I will change it to ONLY use a transaction for the actual\n>> ref update and keep using rename() for the reflog handling.\n>> Only real change I will do for the reflog handling is to change the\n>> temporary file name used to be less collission prone if there are two\n>> renames happening at the same time\n>> so that they don't destroy each others reflogs.\n>\n> I think it is a good idea to make renaming the entire reflog a\n> logical element of transaction (as you mentioned in our private\n> discussion) to allow different backends implement in their best\n> efficient & robust way.\n\nRight. I have changed it to use an optimized function to read the\nwhole existing reflog as a blob into a strbuf\nand then a new transaction function transaction_replace_reflog(...\nthe-blob ...) to write the whole blob back to the new location.\n\n\n>\n> And for filesystem-backed backends, I actually think \"keep the\n> original until we know we do not have to roll back\", that follows\n> the same pattern for the other transactional updates, is a good\n> implementation of that \"best efficient & robust way\", compared to\n> the original \"just rename it\". It frees us from having to be\n> worried about what happens if we cannot rename it back.\n>\n> Thanks.\n"},{"id":"251234","messageId":"xmqqr3xpuy8c.fsf@gitster.dls.corp.google.com","threadId":"37776","inReplyTo":"1413923820-14457-16-git-send-email-sahlberg@google.com","subject":"Re: [PATCH 15/15] refs.c: add an err argument to pack_refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-30T19:57:39Z","receivedAt":"2014-10-30T19:57:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ronnie Sahlberg writes:\n\n> diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c\n> index b20b1ec..299768e 100644\n> --- a/builtin/pack-refs.c\n> +++ b/builtin/pack-refs.c\n> @@ -10,6 +10,7 @@ static char const * const pack_refs_usage[] = {\n> int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n> {\n> \tunsigned int flags = PACK_REFS_PRUNE;\n> +\tstruct strbuf err = STRBUF_INIT;\n> \tstruct option opts[] = {\n> \t\tOPT_BIT(0, \"all\", &flags, N_(\"pack everything\"), PACK_REFS_ALL),\n> \t\tOPT_BIT(0, \"prune\", &flags, N_(\"prune loose refs (default)\"), PACK_REFS_PRUNE),\n> @@ -17,5 +18,10 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n> \t};\n> \tif (parse_options(argc, argv, prefix, opts, pack_refs_usage, 0))\n> \t\tusage_with_options(pack_refs_usage, opts);\n> -\treturn pack_refs(flags);\n> +\tif (pack_refs(flags, &err)) {\n> +\t\terror(\"%s\", err.buf);\n> +\t\tstrbuf_release(&err);\n> +\t\treturn -1;\n> +\t}\n> +\treturn 0;\n> }\n> diff --git a/refs.c b/refs.c\n> index 7fb0d6c..a5e1eff 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2588,16 +2588,15 @@ static void prune_refs(struct ref_to_prune *r)\n> \t}\n> }\n> \n> -int pack_refs(unsigned int flags)\n> +int pack_refs(unsigned int flags, struct strbuf *err)\n> {\n> \tstruct pack_refs_cb_data cbdata;\n> -\tstruct strbuf err = STRBUF_INIT;\n> \n> \tmemset(&cbdata, 0, sizeof(cbdata));\n> \tcbdata.flags = flags;\n> \n> -\tif (lock_packed_refs(&err))\n> -\t\tdie(\"%s\", err.buf);\n> +\tif (lock_packed_refs(err))\n> +\t\treturn -1;\n> \n> \tcbdata.packed_refs = get_packed_refs(&ref_cache);\n> \n> diff --git a/refs.h b/refs.h\n> index f3e08f5..be16c08 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -130,8 +130,9 @@ extern void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, const struct st\n> /*\n> * Write a packed-refs file for the current repository.\n> * flags: Combination of the above PACK_REFS_* flags.\n> + * Returns 0 on success and fills in err on failure.\n> */\n> -int pack_refs(unsigned int flags);\n> +int pack_refs(unsigned int flags, struct strbuf *err);\n> \n> extern int ref_exists(const char *);\n\nMakes sense.\nThanks.\n"},{"id":"251713","messageId":"20141111103449.GA8371@peff.net","threadId":"37776","inReplyTo":"1413923820-14457-3-git-send-email-sahlberg@google.com","subject":"Re: [PATCH 02/15] refs.c: return error instead of dying when locking fails during transaction","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-11T10:34:49Z","receivedAt":"2014-11-11T10:34:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 21, 2014 at 01:36:47PM -0700, Ronnie Sahlberg wrote:\n\n> commit e193c10fc4f9274d1e751cfcdcc4507818e8d498 upstream.\n> \n> Change lock_ref_sha1_basic to return an error instead of dying when\n> we fail to lock a file during a transaction.\n> This function is only called from transaction_commit() and it knows how\n> to handle these failures.\n> [...]\n> -\t\telse\n> -\t\t\tunable_to_lock_die(ref_file, errno);\n> +\t\telse {\n> +\t\t\tstruct strbuf err = STRBUF_INIT;\n> +\t\t\tunable_to_lock_message(ref_file, errno, &err);\n> +\t\t\terror(\"%s\", err.buf);\n> +\t\t\tstrbuf_reset(&err);\n> +\t\t\tgoto error_return;\n> +\t\t}\n\nI coincidentally just wrote almost the identical patch, because this\nisn't just a cleanup; it fixes a real bug. During pack_refs, we call\nprune_ref to lock and delete the loose ref. If the lock fails, that's\nOK; that just means somebody else is updating it at the same time, and\nwe can skip our pruning step. But due to the unable_to_lock_die call\nhere in lock_ref_sha1_basic, the pack-refs process may die prematurely.\n\nI wonder if it is worth pulling this one out from the rest of the\nseries, as it has value (and can be applied) on its own. I did some\ndigging on the history of this, too. Here's the rationale I wrote:\n\n\n lock_ref_sha1_basic: do not die on locking errors\n \n lock_ref_sha1_basic is inconsistent about when it calls\n die() and when it returns NULL to signal an error. This is\n annoying to any callers that want to recover from a locking\n error.\n \n This seems to be mostly historical accident. It was added in\n 4bd18c4 (Improve abstraction of ref lock/write.,\n 2006-05-17), which returned an error in all cases except\n calling safe_create_leading_directories, in which case it\n died. Later, 40aaae8 (Better error message when we are\n unable to lock the index file, 2006-08-12) asked\n hold_lock_file_for_update to die for us, leaving the\n resolve_ref code-path the only one which returned NULL.\n \n We tried to correct that in 5cc3cef (lock_ref_sha1(): do not\n sometimes error() and sometimes die()., 2006-09-30),\n by converting all of the die() calls into returns. But we\n missed the \"die\" flag passed to the lock code, leaving us\n inconsistent. This state persisted until e5c223e\n (lock_ref_sha1_basic(): if locking fails with ENOENT, retry,\n 2014-01-18). Because of its retry scheme, it does not ask\n the lock code to die, but instead manually dies with\n unable_to_lock_die().\n \n We can make this consistent with the other return paths by\n converting this to use unable_to_lock_message(), and\n returning NULL. This is safe to do because all callers\n already needed to check the return value of the function,\n since it could fail (and return NULL) for other reasons.\n\nI also have some other cleanups to lock_ref_sha1_basic's error handling.\nI'd be happy to take over this patch and send it along with those\ncleanups as a separate series.\n\n-Peff\n"},{"id":"251716","messageId":"CAL=YDWkTr5n=jMKzdCM2oFAyb9v5s=sDALg3Yo7bph5n98PffQ@mail.gmail.com","threadId":"37776","inReplyTo":"20141111103449.GA8371@peff.net","subject":"Re: [PATCH 02/15] refs.c: return error instead of dying when locking fails during transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-11-11T15:42:53Z","receivedAt":"2014-11-11T15:42:53Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Tue, Nov 11, 2014 at 2:34 AM, Jeff King wrote:\n> On Tue, Oct 21, 2014 at 01:36:47PM -0700, Ronnie Sahlberg wrote:\n>\n>> commit e193c10fc4f9274d1e751cfcdcc4507818e8d498 upstream.\n>>\n>> Change lock_ref_sha1_basic to return an error instead of dying when\n>> we fail to lock a file during a transaction.\n>> This function is only called from transaction_commit() and it knows how\n>> to handle these failures.\n>> [...]\n>> - else\n>> - unable_to_lock_die(ref_file, errno);\n>> + else {\n>> + struct strbuf err = STRBUF_INIT;\n>> + unable_to_lock_message(ref_file, errno, &err);\n>> + error(\"%s\", err.buf);\n>> + strbuf_reset(&err);\n>> + goto error_return;\n>> + }\n>\n> I coincidentally just wrote almost the identical patch, because this\n> isn't just a cleanup; it fixes a real bug. During pack_refs, we call\n> prune_ref to lock and delete the loose ref. If the lock fails, that's\n> OK; that just means somebody else is updating it at the same time, and\n> we can skip our pruning step. But due to the unable_to_lock_die call\n> here in lock_ref_sha1_basic, the pack-refs process may die prematurely.\n>\n> I wonder if it is worth pulling this one out from the rest of the\n> series, as it has value (and can be applied) on its own. I did some\n> digging on the history of this, too. Here's the rationale I wrote:\n>\n>\n> lock_ref_sha1_basic: do not die on locking errors\n>\n> lock_ref_sha1_basic is inconsistent about when it calls\n> die() and when it returns NULL to signal an error. This is\n> annoying to any callers that want to recover from a locking\n> error.\n>\n> This seems to be mostly historical accident. It was added in\n> 4bd18c4 (Improve abstraction of ref lock/write.,\n> 2006-05-17), which returned an error in all cases except\n> calling safe_create_leading_directories, in which case it\n> died. Later, 40aaae8 (Better error message when we are\n> unable to lock the index file, 2006-08-12) asked\n> hold_lock_file_for_update to die for us, leaving the\n> resolve_ref code-path the only one which returned NULL.\n>\n> We tried to correct that in 5cc3cef (lock_ref_sha1(): do not\n> sometimes error() and sometimes die()., 2006-09-30),\n> by converting all of the die() calls into returns. But we\n> missed the \"die\" flag passed to the lock code, leaving us\n> inconsistent. This state persisted until e5c223e\n> (lock_ref_sha1_basic(): if locking fails with ENOENT, retry,\n> 2014-01-18). Because of its retry scheme, it does not ask\n> the lock code to die, but instead manually dies with\n> unable_to_lock_die().\n>\n> We can make this consistent with the other return paths by\n> converting this to use unable_to_lock_message(), and\n> returning NULL. This is safe to do because all callers\n> already needed to check the return value of the function,\n> since it could fail (and return NULL) for other reasons.\n>\n> I also have some other cleanups to lock_ref_sha1_basic's error handling.\n> I'd be happy to take over this patch and send it along with those\n> cleanups as a separate series.\n\n\n Sounds Good To Me.\n\n\nThanks,\nRonnie Sahlberg\n"}]}