{"thread":{"id":"37214","subject":"[PATCH 1/5] refs.c: move reflog updates into its own function","startedAt":"2014-07-25T17:55:21Z","lastAt":"2014-07-30T19:20:02Z","messageCount":10,"participants":["Ronnie Sahlberg","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"246726","messageId":"1406310926-4080-1-git-send-email-sahlberg@google.com","threadId":"37214","inReplyTo":null,"subject":"[PATCH 0/5] use packed refs for ref-transactions","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-25T17:55:21Z","receivedAt":"2014-07-25T17:55:21Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"This is a small patch series that continues ontop of the \nref-transactions-rename series I posted earlier today.\n\nThis series expands the use of the packed refs file to make multi-ref\nupdates much more atomic. Additionally it allows us to continue pushing\nexternal caller to use transactions instead of accessing (packed) refs\ndirectly which allows us to remove a whole bunch of refs functions\nfrom the public api.\n\nAfter this series there is no real need any more to expose that packed\nrefs even exist to external callers.\n\n\nThis series is built ontop of the previous series that was posted earlier\ntoday as :\n  [PATCH 0/5] ref-transactions-rename\n\n\nRonnie Sahlberg (5):\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\n builtin/clone.c       |  16 ++++--\n builtin/remote.c      |  70 +++++++++++++----------\n refs.c                | 153 +++++++++++++++++++++++++++++++++-----------------\n refs.h                |  33 -----------\n t/t5516-fetch-push.sh |   2 +-\n 5 files changed, 153 insertions(+), 121 deletions(-)\n\n-- \n2.0.1.518.g4f5a8ad\n"},{"id":"246724","messageId":"1406310926-4080-2-git-send-email-sahlberg@google.com","threadId":"37214","inReplyTo":"1406310926-4080-1-git-send-email-sahlberg@google.com","subject":"[PATCH 1/5] refs.c: move reflog updates into its own function","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-25T17:55:22Z","receivedAt":"2014-07-25T17:55:22Z","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\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n refs.c | 62 ++++++++++++++++++++++++++++++++++++--------------------------\n 1 file changed, 36 insertions(+), 26 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 619725a..1048017 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2869,6 +2869,39 @@ static 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\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\", head_sha1,\n+\t\t\t\t\t      RESOLVE_REF_READING, &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  * Writes sha1 into the ref specified by the lock. Makes sure that errno\n  * is sane on error.\n@@ -2912,34 +2945,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\", head_sha1,\n-\t\t\t\t\t      RESOLVE_REF_READING, &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@@ -3649,7 +3658,8 @@ int transaction_commit(struct ref_transaction *transaction,\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\tif (!resolve_ref_unsafe(update->refname, sha1,\n+\t\t\t\t\tRESOLVE_REF_READING, NULL))\n \t\t\tcontinue;\n \n \t\tadd_packed_ref(update->refname, sha1);\n-- \n2.0.1.518.g4f5a8ad\n"},{"id":"246727","messageId":"1406310926-4080-3-git-send-email-sahlberg@google.com","threadId":"37214","inReplyTo":"1406310926-4080-1-git-send-email-sahlberg@google.com","subject":"[PATCH 2/5] refs.c: write updates to packed refs when a transaction has more than one ref","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-25T17:55:23Z","receivedAt":"2014-07-25T17:55:23Z","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\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n builtin/clone.c       | 16 +++++++---\n refs.c                | 83 +++++++++++++++++++++++++++++++++++++--------------\n t/t5516-fetch-push.sh |  2 +-\n 3 files changed, 74 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex f7307e6..7737e12 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -497,17 +497,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 ref_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_sha1(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 1048017..efa4f0d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2539,33 +2539,18 @@ int repack_without_refs(const char **refnames, int n, 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 i, ret, removed = 0;\n+\tint i, ret;\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 \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-\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-\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, refnames[i]);\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@@ -3614,6 +3599,7 @@ int transaction_commit(struct ref_transaction *transaction,\n \t\t       struct strbuf *err)\n {\n \tint ret = 0, delnum = 0, i, df_conflict = 0, need_repack = 0;\n+\tint num_updates = 0;\n \tconst char **delnames;\n \tint n = transaction->nr;\n \tstruct packed_ref_cache *packed_ref_cache;\n@@ -3647,19 +3633,38 @@ int transaction_commit(struct ref_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 non symref refs\n+\t * to the packed refs too.\n+\t */\n \tfor (i = 0; i < n; i++) {\n \t\tstruct ref_update *update = updates[i];\n \t\tunsigned char sha1[20];\n+\t\tint flag;\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 \t\tif (!resolve_ref_unsafe(update->refname, sha1,\n-\t\t\t\t\tRESOLVE_REF_READING, NULL))\n+\t\t\t\t\tRESOLVE_REF_READING, &flag))\n+\t\t\tcontinue;\n+\t\tif (flag & REF_ISSYMREF)\n \t\t\tcontinue;\n \n \t\tadd_packed_ref(update->refname, sha1);\n@@ -3683,11 +3688,14 @@ int transaction_commit(struct ref_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 * safely delete these loose refs.\n+\t * If we are updating multiple normal refs, then those will also be in\n+\t * the packed refs file so we can delete them too.\n \t */\n \n \t/* Unlink any loose refs scheduled for deletion */\n@@ -3725,7 +3733,10 @@ int transaction_commit(struct ref_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@@ -3749,6 +3760,30 @@ int transaction_commit(struct ref_transaction *transaction,\n \t\t\tret = -1;\n \t\t\tgoto cleanup;\n \t\t}\n+\t\tif (num_updates < 2 || update->type & REF_ISSYMREF)\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@@ -3797,7 +3832,7 @@ int transaction_commit(struct ref_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\tret = write_ref_sha1(update->lock, update->new_sha1,\n \t\t\t\t\t     update->msg);\n \t\t\tupdate->lock = NULL; /* freed by write_ref_sha1 */\n@@ -3862,6 +3897,10 @@ int transaction_commit(struct ref_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(delnames, delnum, err))\n \t\tret = -1;\n \tclear_loose_ref_cache(&ref_cache);\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 67e0ab3..23978de 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.0.1.518.g4f5a8ad\n"},{"id":"246728","messageId":"1406310926-4080-4-git-send-email-sahlberg@google.com","threadId":"37214","inReplyTo":"1406310926-4080-1-git-send-email-sahlberg@google.com","subject":"[PATCH 3/5] remote.c: use a transaction for deleting refs","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-25T17:55:24Z","receivedAt":"2014-07-25T17:55:24Z","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\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n builtin/remote.c | 70 +++++++++++++++++++++++++++++++-------------------------\n 1 file changed, 39 insertions(+), 31 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 9a9cc92..3e6ef08 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -750,25 +750,27 @@ static int mv(int argc, const char **argv)\n \n static int remove_branches(struct string_list *branches)\n {\n-\tconst char **branch_names;\n \tint i, result = 0;\n+\tstruct ref_transaction *transaction;\n+\tstruct strbuf err = STRBUF_INIT;\n+\n+\ttransaction = transaction_begin(&err);\n+\tif (!transaction)\n+\t\tdie(\"%s\", err.buf);\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 (lock_packed_refs(0))\n-\t\tresult |= unable_to_lock_error(git_path(\"packed-refs\"), errno);\n-\tresult |= repack_without_refs(branch_names, branches->nr, NULL);\n-\tfree(branch_names);\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+\t\tif (transaction_delete_sha1(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 \n+ cleanup:\n+\tstrbuf_release(&err);\n+\ttransaction_free(transaction);\n \treturn result;\n }\n \n@@ -1317,10 +1319,11 @@ 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+\tstruct ref_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@@ -1331,28 +1334,26 @@ static int prune_remote(const char *remote, int dry_run)\n \t\t       states.remote->url_nr\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\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 \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\tstring_list_insert(&delete_refs_list, refname);\n \n-\t\tif (!dry_run)\n-\t\t\tresult |= delete_ref(refname, NULL, 0);\n+\t\tif (!dry_run) {\n+\t\t\tif (transaction_delete_sha1(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@@ -1362,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.0.1.518.g4f5a8ad\n"},{"id":"246725","messageId":"1406310926-4080-5-git-send-email-sahlberg@google.com","threadId":"37214","inReplyTo":"1406310926-4080-1-git-send-email-sahlberg@google.com","subject":"[PATCH 4/5] refs.c: make repack_without_refs static","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-25T17:55:25Z","receivedAt":"2014-07-25T17:55:25Z","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 | 2 +-\n refs.h | 3 ---\n 2 files changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex efa4f0d..5696d18 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2534,7 +2534,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(const char **refnames, int n, struct strbuf *err)\n+static int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n {\n \tstruct ref_dir *packed;\n \tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\ndiff --git a/refs.h b/refs.h\nindex eb918a0..7183e8e 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -155,9 +155,6 @@ 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-\t\t\t       struct strbuf *err);\n-\n extern int ref_exists(const char *);\n \n /*\n-- \n2.0.1.518.g4f5a8ad\n"},{"id":"246729","messageId":"1406310926-4080-6-git-send-email-sahlberg@google.com","threadId":"37214","inReplyTo":"1406310926-4080-1-git-send-email-sahlberg@google.com","subject":"[PATCH 5/5] refs.c: make the *_packed_refs functions static","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-25T17:55:26Z","receivedAt":"2014-07-25T17:55:26Z","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\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\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 5696d18..d2ee8a2 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1135,7 +1135,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@@ -2268,7 +2268,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@@ -2291,7 +2291,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@@ -2316,7 +2316,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 7183e8e..ec52e13 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -112,36 +112,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.0.1.518.g4f5a8ad\n"},{"id":"246946","messageId":"xmqq4mxz3m8q.fsf@gitster.dls.corp.google.com","threadId":"37214","inReplyTo":"1406310926-4080-3-git-send-email-sahlberg@google.com","subject":"Re: [PATCH 2/5] refs.c: write updates to packed refs when a transaction has more than one ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-29T21:09:57Z","receivedAt":"2014-07-29T21:09:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ronnie Sahlberg <sahlberg@google.com> writes:\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 non symref refs\n> +\t * to the packed refs too.\n> +\t */\n>  \tfor (i = 0; i < n; i++) {\n>  \t\tstruct ref_update *update = updates[i];\n>  \t\tunsigned char sha1[20];\n> +\t\tint flag;\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>  \t\tif (!resolve_ref_unsafe(update->refname, sha1,\n> -\t\t\t\t\tRESOLVE_REF_READING, NULL))\n> +\t\t\t\t\tRESOLVE_REF_READING, &flag))\n> +\t\t\tcontinue;\n> +\t\tif (flag & REF_ISSYMREF)\n>  \t\t\tcontinue;\n\nThis skipping of symref didn't use to be here.\n\nIs this an enhancement needed to implement the new \"feature\"\n(i.e. use packed refs to represent multi-update as an atomic\noperation)?  It looks to me more like an unrelated \"oops, forgot\nthat we cannot use packed refs to store symrefs\" fix-up, unless no\ncaller passed symref in updates[] in the original code, and now we\nhave callers that do.\n\nPuzzled...\n"},{"id":"246947","messageId":"xmqqzjfr27m5.fsf@gitster.dls.corp.google.com","threadId":"37214","inReplyTo":"1406310926-4080-3-git-send-email-sahlberg@google.com","subject":"Re: [PATCH 2/5] refs.c: write updates to packed refs when a transaction has more than one ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-29T21:11:14Z","receivedAt":"2014-07-29T21:11:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ronnie Sahlberg <sahlberg@google.com> writes:\n\n> +\t\tpacked = get_packed_refs(&ref_cache);;\n\ns/;;/;/; ;-)\n\nSorry, I couldn't resist the urge to type many semicolons ;-)\n"},{"id":"246986","messageId":"CAL=YDWn60wSnY3cKBDgZYdCnNDzyF9VyJ72CbBcHSi7kM2uY2A@mail.gmail.com","threadId":"37214","inReplyTo":"xmqq4mxz3m8q.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/5] refs.c: write updates to packed refs when a transaction has more than one ref","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-30T19:19:42Z","receivedAt":"2014-07-30T19:19:42Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Tue, Jul 29, 2014 at 2:09 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Ronnie Sahlberg <sahlberg@google.com> writes:\n>\n>> +     /*\n>> +      * Always copy loose refs that are to be deleted to the packed refs.\n>> +      * If we are updating multiple refs then copy all non symref refs\n>> +      * to the packed refs too.\n>> +      */\n>>       for (i = 0; i < n; i++) {\n>>               struct ref_update *update = updates[i];\n>>               unsigned char sha1[20];\n>> +             int flag;\n>>\n>>               if (update->update_type != UPDATE_SHA1)\n>>                       continue;\n>> -             if (!is_null_sha1(update->new_sha1))\n>> +             if (num_updates < 2 && !is_null_sha1(update->new_sha1))\n>>                       continue;\n>>               if (get_packed_ref(update->refname))\n>>                       continue;\n>>               if (!resolve_ref_unsafe(update->refname, sha1,\n>> -                                     RESOLVE_REF_READING, NULL))\n>> +                                     RESOLVE_REF_READING, &flag))\n>> +                     continue;\n>> +             if (flag & REF_ISSYMREF)\n>>                       continue;\n>\n> This skipping of symref didn't use to be here.\n>\n> Is this an enhancement needed to implement the new \"feature\"\n> (i.e. use packed refs to represent multi-update as an atomic\n> operation)?  It looks to me more like an unrelated \"oops, forgot\n> that we cannot use packed refs to store symrefs\" fix-up, unless no\n> caller passed symref in updates[] in the original code, and now we\n> have callers that do.\n>\n> Puzzled...\n\nIt was mainly as an enhancement.\nPrior to this patch we were only using the packed-refs trick for the\ndelete+rename operation part of a rename_ref() call.\nAnd for that case we already explicitly test for and disallow symbolic\nrefs already in rename_ref().\n\nI added this just for extra safety for \"now that we possibly delete\nmultiple refs in one transaction, should I special case/disallow the\npacked refs trick for symrefs?\"\nLooking at it again I don't think we need this special casing and it\nonly makes the code more complex and confusing.\nI will review the use of this a little and run some tests and if all\nlooks sane I will remove this ISSYMREF special casing.\n\nThank!\nronnie sahlberg\n"},{"id":"246987","messageId":"CAL=YDW=W3m9Vg=eGffgJNMo1gPqqugLaySmGhHh=je7qyhMdNw@mail.gmail.com","threadId":"37214","inReplyTo":"xmqqzjfr27m5.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/5] refs.c: write updates to packed refs when a transaction has more than one ref","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-30T19:20:02Z","receivedAt":"2014-07-30T19:20:02Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Tue, Jul 29, 2014 at 2:11 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Ronnie Sahlberg <sahlberg@google.com> writes:\n>\n>> +             packed = get_packed_refs(&ref_cache);;\n>\n> s/;;/;/; ;-)\n>\n> Sorry, I couldn't resist the urge to type many semicolons ;-)\n\nFixed, thanks!\n"}]}