{"thread":{"id":"37250","subject":"[PATCH v2 1/5] refs.c: allow passing raw git_committer_info as email to _update_reflog","startedAt":"2014-07-30T21:52:43Z","lastAt":"2014-07-30T21:52:48Z","messageCount":6,"participants":["Ronnie Sahlberg"],"isPatch":true,"patchVersion":2,"patchTotal":5},"messages":[{"id":"246995","messageId":"1406757168-3729-1-git-send-email-sahlberg@google.com","threadId":"37250","inReplyTo":null,"subject":"[PATCH v2 0/5] ref-transactions-rename","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-30T21:52:43Z","receivedAt":"2014-07-30T21:52:43Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"\nThis patch series adds support for using transactions and atomic renames.\nIt focuses on what needs to be done in order to support fully atomic\nand rollbackable renames that may or may not involve name conflicts.\n\nBy performing the actual delete old/create new via a single operation to\nthe packed refs file this process will also appear as an atomic change for\nany external observers.\n\n(While this series is \"short\" it contains a very important change where\nwe start performing ref changes as operations inside a locked packed refs\nfile instead of as discreete operations of loose ref files.\nThis approach will be extended in the next patch series where we will start\nusing it also to make multi-ref creations/updates become fully\natomic for external observers.)\n\nVersion 2:\n -- Updated with Jonathans suggestions from last week.\n\n\nThis series is built on iand depend on the previous reflog series:\n  * rs/ref-transaction-reflog (2014-07-23) 15 commits\n   - refs.c: allow deleting refs with a broken sha1\n   - refs.c: remove lock_any_ref_for_update\n   - refs.c: make unlock_ref/close_ref/commit_ref static\n   - refs.c: rename log_ref_setup to create_reflog\n   - reflog.c: use a reflog transaction when writing during expire\n   - refs.c: allow multiple reflog updates during a single transaction\n   - refs.c: only write reflog update if msg is non-NULL\n   - refs.c: add a flag to allow reflog updates to truncate the log\n   - refs.c: add a transaction function to append a reflog entry\n   - lockfile.c: make hold_lock_file_for_append preserve meaningful errno\n   - refs.c: add a function to append a reflog entry to a fd\n   - refs.c: add a new update_type field to ref_update\n   - refs.c: rename the transaction functions\n   - refs.c: make ref_transaction_delete a wrapper for ref_transaction_update\n   - refs.c: make ref_transaction_create a wrapper to ref_transaction_update\n   (this branch uses rs/ref-transaction and rs/ref-transaction-1.)\n\n\n\nRonnie Sahlberg (5):\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: update rename_ref to use a transaction\n  refs.c: rollback the lockfile before we die() in repack_without_refs\n\n builtin/reflog.c  |  19 ++-\n builtin/remote.c  |  14 +-\n refs.c            | 377 ++++++++++++++++++++++++++++++------------------------\n refs.h            |  25 +++-\n t/t3200-branch.sh |   7 -\n 5 files changed, 255 insertions(+), 187 deletions(-)\n\n-- \n2.0.1.523.g0041e8a\n"},{"id":"246993","messageId":"1406757168-3729-2-git-send-email-sahlberg@google.com","threadId":"37250","inReplyTo":"1406757168-3729-1-git-send-email-sahlberg@google.com","subject":"[PATCH v2 1/5] refs.c: allow passing raw git_committer_info as email to _update_reflog","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-30T21:52:44Z","receivedAt":"2014-07-30T21:52:44Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"In 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 <EMAIL>\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\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 f11fee3..c6f3a2f 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@@ -671,7 +678,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 2662ef6..41cb51d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3096,7 +3096,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@@ -3143,7 +3143,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@@ -3491,8 +3491,7 @@ int transaction_update_reflog(struct ref_transaction *transaction,\n \t\t\t      const char *refname,\n \t\t\t      const unsigned char *new_sha1,\n \t\t\t      const unsigned char *old_sha1,\n-\t\t\t      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@@ -3521,13 +3520,17 @@ int transaction_update_reflog(struct ref_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 0172f48..23067a2 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -309,6 +309,28 @@ int transaction_delete_sha1(struct ref_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\n+ * then the structure contains a prebaked committer string\n+ * just like git_committer_info() 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@@ -318,8 +340,7 @@ int transaction_update_reflog(struct ref_transaction *transaction,\n \t\t\t      const char *refname,\n \t\t\t      const unsigned char *new_sha1,\n \t\t\t      const unsigned char *old_sha1,\n-\t\t\t      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.0.1.523.g0041e8a\n"},{"id":"246998","messageId":"1406757168-3729-3-git-send-email-sahlberg@google.com","threadId":"37250","inReplyTo":"1406757168-3729-1-git-send-email-sahlberg@google.com","subject":"[PATCH v2 2/5] refs.c: return error instead of dying when locking fails during transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-30T21:52:45Z","receivedAt":"2014-07-30T21:52:45Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change 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\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n refs.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 41cb51d..35f6db1 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2206,6 +2206,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@@ -2213,8 +2214,10 @@ 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_index_die(ref_file, errno);\n+\t\telse {\n+\t\t\tunable_to_lock_error(ref_file, errno);\n+\t\t\tgoto error_return;\n+\t\t}\n \t}\n \tif (bad_ref)\n \t\treturn lock;\n-- \n2.0.1.523.g0041e8a\n"},{"id":"246997","messageId":"1406757168-3729-4-git-send-email-sahlberg@google.com","threadId":"37250","inReplyTo":"1406757168-3729-1-git-send-email-sahlberg@google.com","subject":"[PATCH v2 3/5] refs.c: use packed refs when deleting refs during a transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-30T21:52:46Z","receivedAt":"2014-07-30T21:52:46Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Make 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\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n builtin/remote.c |  14 ++++--\n refs.c           | 148 +++++++++++++++++++++++++++++++++++++++++++------------\n 2 files changed, 128 insertions(+), 34 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex be8ebac..bfcc823 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -753,6 +753,9 @@ 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\treturn unable_to_lock_error(git_path(\"packed-refs\"), errno);\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@@ -1333,9 +1336,14 @@ static int prune_remote(const char *remote, int dry_run)\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\tresult |= repack_without_refs(delete_refs,\n-\t\t\t\t\t\t      states.stale.nr, NULL);\n+\t\tif (!dry_run) {\n+\t\t\tif (lock_packed_refs(0))\n+\t\t\t\tresult |= unable_to_lock_error(\n+\t\t\t\t\tgit_path(\"packed-refs\"), errno);\n+\t\t\telse\n+\t\t\t\tresult |= repack_without_refs(delete_refs,\n+\t\t\t\t\t\t\tstates.stale.nr, NULL);\n+\t\t}\n \t\tfree(delete_refs);\n \t}\n \ndiff --git a/refs.c b/refs.c\nindex 35f6db1..32f48ea 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2535,6 +2535,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@@ -2547,19 +2550,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\tif (err) {\n-\t\t\tunable_to_lock_message(git_path(\"packed-refs\"), errno,\n-\t\t\t\t\t       err);\n-\t\t\treturn -1;\n-\t\t}\n-\t\tunable_to_lock_error(git_path(\"packed-refs\"), errno);\n-\t\treturn error(\"cannot delete '%s' from packed refs\", refnames[i]);\n \t}\n+\n \tpacked = get_packed_refs(&ref_cache);\n \n \t/* Remove refnames from the cache */\n@@ -3671,10 +3667,12 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n int transaction_commit(struct ref_transaction *transaction,\n \t\t       struct strbuf *err)\n {\n-\tint ret = 0, delnum = 0, i, df_conflict = 0;\n+\tint ret = 0, delnum = 0, i, df_conflict = 0, 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 \tif (transaction->state != REF_TRANSACTION_OPEN)\n \t\tdie(\"BUG: commit called for transaction that is not open\");\n@@ -3694,12 +3692,100 @@ int transaction_commit(struct ref_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\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, sha1, 1, NULL))\n+\t\t\tcontinue;\n+\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->lock = lock_ref_sha1_basic(update->refname,\n+\t\t\t\t\t\t   (update->have_old ?\n+\t\t\t\t\t\t    update->old_sha1 :\n+\t\t\t\t\t\t    NULL),\n+\t\t\t\t\t\t   update->flags,\n+\t\t\t\t\t\t   &update->type,\n+\t\t\t\t\t\t   delnames, delnum);\n+\t\tif (!update->lock) {\n+\t\t\tif (errno == ENOTDIR)\n+\t\t\t\tdf_conflict = 1;\n+\t\t\tif (err)\n+\t\t\t\tstrbuf_addf(err, \"Cannot lock the ref '%s'.\",\n+\t\t\t\t\t    update->refname);\n+\t\t\tret = -1;\n+\t\t\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@@ -3718,6 +3804,10 @@ int transaction_commit(struct ref_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@@ -3729,6 +3819,16 @@ int transaction_commit(struct ref_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@@ -3744,7 +3844,7 @@ int transaction_commit(struct ref_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@@ -3765,21 +3865,6 @@ int transaction_commit(struct ref_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 = -1;\n-\n-\t\t\tif (!(update->flags & REF_ISPRUNING))\n-\t\t\t\tdelnames[delnum++] = update->lock->ref_name;\n-\t\t}\n-\t}\n-\n \t/*\n \t * Update all reflog files\n \t * We have already committed all ref updates and deletes.\n@@ -3832,11 +3917,12 @@ int transaction_commit(struct ref_transaction *transaction,\n \n \tif (repack_without_refs(delnames, delnum, err))\n \t\tret = -1;\n-\tfor (i = 0; i < delnum; i++)\n-\t\tunlink_or_warn(git_path(\"logs/%s\", delnames[i]));\n \tclear_loose_ref_cache(&ref_cache);\n \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 = ret ? REF_TRANSACTION_ERROR\n \t\t: REF_TRANSACTION_CLOSED;\n \n-- \n2.0.1.523.g0041e8a\n"},{"id":"246996","messageId":"1406757168-3729-5-git-send-email-sahlberg@google.com","threadId":"37250","inReplyTo":"1406757168-3729-1-git-send-email-sahlberg@google.com","subject":"[PATCH v2 4/5] refs.c: update rename_ref to use a transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-30T21:52:47Z","receivedAt":"2014-07-30T21:52:47Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change 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\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n refs.c            | 197 ++++++++++++++++++++----------------------------------\n t/t3200-branch.sh |   7 --\n 2 files changed, 71 insertions(+), 133 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 32f48ea..9c813f9 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2620,82 +2620,52 @@ 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 ref_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 write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,\n-\t\t\t  const char *logmsg);\n-\n int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsg)\n {\n-\tunsigned char sha1[20], orig_sha1[20];\n-\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 ref_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, orig_sha1,\n+\tsymref = resolve_ref_unsafe(oldrefname, sha1,\n \t\t\t\t    RESOLVE_REF_READING, &flag);\n-\tif (flag & REF_ISSYMREF)\n-\t\treturn error(\"refname %s is a symbolic ref, renaming it is not supported\",\n+\tif (flag & REF_ISSYMREF) {\n+\t\terror(\"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+\t\treturn 1;\n+\t}\n+\tif (!symref) {\n+\t\terror(\"refname %s not found\", oldrefname);\n+\t\treturn 1;\n+\t}\n \n \tif (!is_refname_available(newrefname, get_packed_refs(&ref_cache),\n \t\t\t\t  &oldrefname, 1))\n@@ -2705,70 +2675,45 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \t\t\t\t  &oldrefname, 1))\n \t\treturn 1;\n \n-\tif (log && rename(git_path(\"logs/%s\", oldrefname), git_path(TMP_RENAMED_LOG)))\n-\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, sha1, RESOLVE_REF_READING, 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-\t}\n-\n-\tif (log && rename_tmp_log(newrefname))\n-\t\tgoto rollback;\n-\n-\tlogmoved = log;\n-\n-\tlock = lock_ref_sha1_basic(newrefname, NULL, 0, NULL, NULL, 0);\n-\tif (!lock) {\n-\t\terror(\"unable to lock %s for update\", newrefname);\n-\t\tgoto rollback;\n-\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-\t}\n-\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_sha1(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+\tif (transaction_update_sha1(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, 0, NULL, NULL, 0);\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 ac31b71..64f5bf2 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -293,13 +293,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.0.1.523.g0041e8a\n"},{"id":"246994","messageId":"1406757168-3729-6-git-send-email-sahlberg@google.com","threadId":"37250","inReplyTo":"1406757168-3729-1-git-send-email-sahlberg@google.com","subject":"[PATCH v2 5/5] refs.c: rollback the lockfile before we die() in repack_without_refs","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-30T21:52:48Z","receivedAt":"2014-07-30T21:52:48Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n refs.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 9c813f9..3e98ca1 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2574,8 +2574,10 @@ int repack_without_refs(const char **refnames, int n, 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.0.1.523.g0041e8a\n"}]}