{"thread":{"id":"36707","subject":"[PATCH 0/2] remote: optimize rm/prune ref deletion","startedAt":"2014-05-20T10:34:12Z","lastAt":"2014-05-27T16:55:02Z","messageCount":16,"participants":["Jens Lindström","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"242239","messageId":"537B2FA4.7020001@opera.com","threadId":"36707","inReplyTo":null,"subject":"[PATCH 0/2] remote: optimize rm/prune ref deletion","fromName":"Jens Lindström","fromEmail":"jl@opera.com","sentAt":"2014-05-20T10:34:12Z","receivedAt":"2014-05-20T10:34:12Z","isPatch":true,"sender":{"key":"jl@opera.com","avatar":"https://avatars.githubusercontent.com/u/134918550?v=4"},"body":"At work, we have some shared repositories with far too many refs in\nthem, which causes various issues, performance and otherwise.  We plan\nto move most of the refs out of them, but for that to help users that\nhave already fetched all the refs into their local repositories, those\nusers should want to run 'git remote prune'.\n\nIt turns out that 'git remote prune' (and also 'git remote rm') has\nits own rather severe performance issues relating to removing the\nobsolete refs from the local repository, which I've addressed in the\nfollowing patches.  The performance improves from many CPU minutes\n(long enough that I could never be bothered to let it run to\ncompletion) to around a few seconds, when removing ~15000 refs.\n\nJens Lindström (2):\n  remote: defer repacking packed-refs when deleting refs\n  remote prune: optimize \"dangling symref\" check/warning\n\n builtin/remote.c | 25 +++++++++++++++++++++----\n refs.c           | 34 +++++++++++++++++++++++++++-------\n refs.h           |  4 ++++\n 3 files changed, 52 insertions(+), 11 deletions(-)\n\n--\n1.9.1\n"},{"id":"242240","messageId":"537B30E7.5020505@opera.com","threadId":"36707","inReplyTo":"537B2FA4.7020001@opera.com","subject":"[PATCH 1/2] remote: defer repacking packed-refs when deleting refs","fromName":"Jens Lindström","fromEmail":"jl@opera.com","sentAt":"2014-05-20T10:39:35Z","receivedAt":"2014-05-20T10:39:35Z","isPatch":true,"sender":{"key":"jl@opera.com","avatar":"https://avatars.githubusercontent.com/u/134918550?v=4"},"body":"When 'git remote rm' or 'git remote prune' were used in a repository\nwith many refs, and needed to delete many refs, a lot of time was spent\ndeleting those refs since for each deleted ref, repack_without_refs()\nwas called to rewrite packed-refs without just that deleted ref.\n\nTo avoid this, defer the repacking until after all refs have been\ndeleted (by delete_ref()), and then call repack_without_refs() once to\nrepack without all the deleted refs.\n\nSigned-off-by: Jens Lindström <jl@opera.com>\n---\nThis patch changes behavior when the operation is aborted in the\nmiddle, so that loose refs and ref logs might have been deleted, but\nnot the corresponding entries in packed-refs, since packed-refs is now\nonly updated at the end.  This is a bit unfortunate, and may\n\"resurrect\" an obsolete packed-refs entry by deleting the loose ref\nthat had overridden it.\n\nMitigating factors would be that this only affects remote tracking\nbranches that we were about to delete anyway, and that in the case of\n'git remote prune' were apparently not actually matching a ref in the\nremote.\n\nAlso, it is a lot harder to interrupt the operation in the middle when\nit takes a few seconds compared to when it takes many minutes to\ncomplete.  :-)\n\n builtin/remote.c | 19 ++++++++++++++++---\n refs.c           | 15 +++++++++------\n refs.h           |  3 +++\n 3 files changed, 28 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex b3ab4cf..ce60a30 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -749,15 +749,18 @@ static int mv(int argc, const char **argv)\n \n static int remove_branches(struct string_list *branches)\n {\n+\tconst char **branch_names = xmalloc(branches->nr * sizeof(*branch_names));\n \tint i, result = 0;\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+\t\tconst char *refname = branch_names[i] = item->string;\n \t\tunsigned char *sha1 = item->util;\n \n-\t\tif (delete_ref(refname, sha1, 0))\n+\t\tif (delete_ref(refname, sha1, REF_DEFERREPACK))\n \t\t\tresult |= error(_(\"Could not remove branch %s\"), refname);\n \t}\n+\tresult |= repack_without_refs(branch_names, branches->nr);\n+\tfree(branch_names);\n \treturn result;\n }\n \n@@ -1303,6 +1306,7 @@ static int prune_remote(const char *remote, int dry_run)\n {\n \tint result = 0, i;\n \tstruct ref_states states;\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@@ -1316,13 +1320,16 @@ 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+\t\tdelete_refs = xmalloc(states.stale.nr * sizeof(*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\tdelete_refs[i] = refname;\n+\n \t\tif (!dry_run)\n-\t\t\tresult |= delete_ref(refname, NULL, 0);\n+\t\t\tresult |= delete_ref(refname, NULL, REF_DEFERREPACK);\n \n \t\tif (dry_run)\n \t\t\tprintf_ln(_(\" * [would prune] %s\"),\n@@ -1333,6 +1340,12 @@ static int prune_remote(const char *remote, int dry_run)\n \t\twarn_dangling_symref(stdout, dangling_msg, refname);\n \t}\n \n+\tif (states.stale.nr) {\n+\t\tif (!dry_run)\n+\t\t\tresult |= repack_without_refs(delete_refs, states.stale.nr);\n+\t\tfree(delete_refs);\n+\t}\n+\n \tfree_remote_ref_states(&states);\n \treturn result;\n }\ndiff --git a/refs.c b/refs.c\nindex 28d5eca..3b62aca 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2431,7 +2431,7 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)\n \treturn 0;\n }\n \n-static int repack_without_refs(const char **refnames, int n)\n+int repack_without_refs(const char **refnames, int n)\n {\n \tstruct ref_dir *packed;\n \tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\n@@ -2507,11 +2507,14 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n \t\treturn 1;\n \tret |= delete_ref_loose(lock, flag);\n \n-\t/* removing the loose one could have resurrected an earlier\n-\t * packed one.  Also, if it was not loose we need to repack\n-\t * without it.\n-\t */\n-\tret |= repack_without_ref(lock->ref_name);\n+\tif (!(delopt & REF_DEFERREPACK))\n+\t{\n+\t\t/* removing the loose one could have resurrected an earlier\n+\t\t * packed one.  Also, if it was not loose we need to repack\n+\t\t * without it.\n+\t\t */\n+\t\tret |= repack_without_ref(lock->ref_name);\n+\t}\n \n \tunlink_or_warn(git_path(\"logs/%s\", lock->ref_name));\n \tclear_loose_ref_cache(&ref_cache);\ndiff --git a/refs.h b/refs.h\nindex 87a1a79..0db5584 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -132,6 +132,8 @@ 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+\n extern int ref_exists(const char *);\n \n /*\n@@ -149,6 +151,7 @@ extern struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char *\n \n /** Locks any ref (for 'HEAD' type refs). */\n #define REF_NODEREF\t0x01\n+#define REF_DEFERREPACK\t0x02\n extern struct ref_lock *lock_any_ref_for_update(const char *refname,\n \t\t\t\t\t\tconst unsigned char *old_sha1,\n \t\t\t\t\t\tint flags, int *type_p);\n--\n1.9.1\n"},{"id":"242241","messageId":"537B3155.1010309@opera.com","threadId":"36707","inReplyTo":"537B2FA4.7020001@opera.com","subject":"[PATCH 2/2] remote prune: optimize \"dangling symref\" check/warning","fromName":"Jens Lindström","fromEmail":"jl@opera.com","sentAt":"2014-05-20T10:41:25Z","receivedAt":"2014-05-20T10:41:25Z","isPatch":true,"sender":{"key":"jl@opera.com","avatar":"https://avatars.githubusercontent.com/u/134918550?v=4"},"body":"When 'git remote prune' was used to delete many refs in a repository\nwith many refs, a lot of time was spent checking for (now) dangling\nsymbolic refs pointing to the deleted ref, since warn_dangling_symref()\nwas once per deleted ref to check all other refs in the repository.\n\nAvoid this using the new warn_dangling_symrefs() function which\nmakes one pass over all refs and checks for all the deleted refs in\none go, after they have all been deleted.\n\nSigned-off-by: Jens Lindström <jl@opera.com>\n---\n builtin/remote.c |  6 +++++-\n refs.c           | 19 ++++++++++++++++++-\n refs.h           |  1 +\n 3 files changed, 24 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex ce60a30..5e4a8dd 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -1306,6 +1306,7 @@ static int prune_remote(const char *remote, int dry_run)\n {\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@@ -1327,6 +1328,7 @@ static int prune_remote(const char *remote, int dry_run)\n \t\tconst char *refname = states.stale.items[i].util;\n \n \t\tdelete_refs[i] = refname;\n+\t\tstring_list_insert(&delete_refs_list, refname);\n \n \t\tif (!dry_run)\n \t\t\tresult |= delete_ref(refname, NULL, REF_DEFERREPACK);\n@@ -1337,9 +1339,11 @@ static int prune_remote(const char *remote, int dry_run)\n \t\telse\n \t\t\tprintf_ln(_(\" * [pruned] %s\"),\n \t\t\t       abbrev_ref(refname, \"refs/remotes/\"));\n-\t\twarn_dangling_symref(stdout, dangling_msg, refname);\n \t}\n \n+\twarn_dangling_symrefs(stdout, dangling_msg, &delete_refs_list);\n+\tstring_list_clear(&delete_refs_list, 0);\n+\n \tif (states.stale.nr) {\n \t\tif (!dry_run)\n \t\t\tresult |= repack_without_refs(delete_refs, states.stale.nr);\ndiff --git a/refs.c b/refs.c\nindex 3b62aca..fdd8b74 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1611,6 +1611,7 @@ int peel_ref(const char *refname, unsigned char *sha1)\n struct warn_if_dangling_data {\n \tFILE *fp;\n \tconst char *refname;\n+\tconst struct string_list *refnames;\n \tconst char *msg_fmt;\n };\n \n@@ -1625,8 +1626,12 @@ static int warn_if_dangling_symref(const char *refname, const unsigned char *sha\n \t\treturn 0;\n \n \tresolves_to = resolve_ref_unsafe(refname, junk, 0, NULL);\n-\tif (!resolves_to || strcmp(resolves_to, d->refname))\n+\tif (!resolves_to\n+\t    || (d->refname\n+\t\t? strcmp(resolves_to, d->refname)\n+\t\t: !string_list_has_string(d->refnames, resolves_to))) {\n \t\treturn 0;\n+\t}\n \n \tfprintf(d->fp, d->msg_fmt, refname);\n \tfputc('\\n', d->fp);\n@@ -1639,6 +1644,18 @@ void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname)\n \n \tdata.fp = fp;\n \tdata.refname = refname;\n+\tdata.refnames = NULL;\n+\tdata.msg_fmt = msg_fmt;\n+\tfor_each_rawref(warn_if_dangling_symref, &data);\n+}\n+\n+void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, const struct string_list *refnames)\n+{\n+\tstruct warn_if_dangling_data data;\n+\n+\tdata.fp = fp;\n+\tdata.refname = NULL;\n+\tdata.refnames = refnames;\n \tdata.msg_fmt = msg_fmt;\n \tfor_each_rawref(warn_if_dangling_symref, &data);\n }\ndiff --git a/refs.h b/refs.h\nindex 0db5584..cd4710d 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -89,7 +89,7 @@ static inline const char *has_glob_specials(const char *pattern)\n extern int for_each_rawref(each_ref_fn, void *);\n \n extern void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname);\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--\n1.9.1\n"},{"id":"242316","messageId":"xmqqtx8kxn7f.fsf@gitster.dls.corp.google.com","threadId":"36707","inReplyTo":"537B30E7.5020505@opera.com","subject":"Re: [PATCH 1/2] remote: defer repacking packed-refs when deleting refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-20T19:30:12Z","receivedAt":"2014-05-20T19:30:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lindström <jl@opera.com> writes:\n\n> When 'git remote rm' or 'git remote prune' were used in a repository\n> with many refs, and needed to delete many refs, a lot of time was spent\n> deleting those refs since for each deleted ref, repack_without_refs()\n> was called to rewrite packed-refs without just that deleted ref.\n>\n> To avoid this, defer the repacking until after all refs have been\n> deleted (by delete_ref()), and then call repack_without_refs() once to\n> repack without all the deleted refs.\n>\n> Signed-off-by: Jens Lindström <jl@opera.com>\n> ---\n> This patch changes behavior when the operation is aborted in the\n> middle, so that loose refs and ref logs might have been deleted, but\n> not the corresponding entries in packed-refs, since packed-refs is now\n> only updated at the end.\n\nAlso this makes it a bit more dangerous for processes accessing the\nrepository while pruning is in progress by exposing stale refs that\nmay be pointing at objects that are no longer present in the object\nstore in the packed-refs file for a longer period (\"git fsck\" may\nfail, for example).\n\nAs long as we accept that removing a remote or pruning one are kinds\nof \"maintenance\" operations and nobody else should be peeking into\nthe repository during maintenance period, it may be alright, but\naborting the operation in the middle will essentially leave the\nrepository in a corrupted state---the \"abort corrupts\" is a downside\nwith or without this change and is not a new problem.\n\nA bit safer way to organize might be to first create a list of the\nrefs to be removed in-core, update packed-refs without these refs to\nbe removed, and then finally remove the loose ones, but I haven't\nthought things through.\n"},{"id":"242319","messageId":"xmqqlhtwxkg9.fsf@gitster.dls.corp.google.com","threadId":"36707","inReplyTo":"xmqqtx8kxn7f.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] remote: defer repacking packed-refs when deleting refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-20T20:29:42Z","receivedAt":"2014-05-20T20:29:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> A bit safer way to organize might be to first create a list of the\n> refs to be removed in-core, update packed-refs without these refs to\n> be removed, and then finally remove the loose ones, but I haven't\n> thought things through.\n\nPerhaps a removal of remote can go in this order to be resistant\nagainst an abort-in-the-middle.\n\n * update packed-refs without the refs that came from the remote.\n\n   - when interrupted before the new temporary file is renamed to\n     the final, it would be a no-op.\n\n   - when interrupted after the rename, only some refs that came\n     from the remote may disappear.\n\n * remove the loose refs that came from the remote.\n\n * finally, remove the configuration related to the remote.\n\nThis order would let you interrupt \"remote rm\" without leaving the\nrepository in a broken state.  Before the final state, it may appear\nthat you have some but not all remote-tracking refs from the remote\nin your repository, but you would not have any ref that point at an\nobsolete object.  Running \"remote rm\" again, once it finishes, will\ngive you the desired result.\n\nA longer-term goal might be to have ref-transaction infrastructure\nclever enough to coalesce the \"repack-without-these-refs\" requests\ninto one automatically without forcing the callers you are fixing\ncare about these things, though.  And such a transaction semantics\nmay have to also cover the updating of configuration files.\n"},{"id":"242558","messageId":"CAEef6WyXGXAdm_DkzNsuLgeFFpQsxvTiuJbK3ypc8-Cz2qD_KQ@mail.gmail.com","threadId":"36707","inReplyTo":"xmqqlhtwxkg9.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] remote: defer repacking packed-refs when deleting refs","fromName":"Jens Lindström","fromEmail":"jl@opera.com","sentAt":"2014-05-23T10:03:06Z","receivedAt":"2014-05-23T10:03:06Z","isPatch":true,"sender":{"key":"jl@opera.com","avatar":"https://avatars.githubusercontent.com/u/134918550?v=4"},"body":"On Tue, May 20, 2014 at 10:29 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> A bit safer way to organize might be to first create a list of the\n>> refs to be removed in-core, update packed-refs without these refs to\n>> be removed, and then finally remove the loose ones, but I haven't\n>> thought things through.\n>\n> Perhaps a removal of remote can go in this order to be resistant\n> against an abort-in-the-middle.\n>\n>  * update packed-refs without the refs that came from the remote.\n>\n>    - when interrupted before the new temporary file is renamed to\n>      the final, it would be a no-op.\n>\n>    - when interrupted after the rename, only some refs that came\n>      from the remote may disappear.\n>\n>  * remove the loose refs that came from the remote.\n>\n>  * finally, remove the configuration related to the remote.\n>\n> This order would let you interrupt \"remote rm\" without leaving the\n> repository in a broken state.  Before the final state, it may appear\n> that you have some but not all remote-tracking refs from the remote\n> in your repository, but you would not have any ref that point at an\n> obsolete object.  Running \"remote rm\" again, once it finishes, will\n> give you the desired result.\n\nRemoving the remote configuration (I assume you mean the\n\"remote.<name>\" section from .git/config) last in 'remote rm' would be\na bit better I think.  Especially with the very slow removal of\nbranches an impatient user would likely abort the command if there\nwere many remote-tracking branches, after which rerunning would fail\nsince the remote configuration was already gone, and then there would\nbe no obvious way to get rid of the remaining remote-tracking\nbranches.\n\nDoing the repacking first and then run through and delete loose refs\nand ref logs leads to a smaller and nicer patch as well; no need to\ntell delete_ref() to not repack then, since repack_without_refs() will\njust find that the ref isn't in packed-refs already and do nothing.\n\nOne additional change was required in\nbuiltin/remote.c:remove_branches().  It used to pass in the expected\nSHA-1 of the ref to delete_ref(), which only works if the ref exists.\nIf repack_without_refs() is called first, most refs won't exist, and\ndelete_ref() would fail.  Branch removal from 'remote prune' didn't\nhave this problem, since it called delete_ref() with a NULL SHA-1.\n\n\n> A longer-term goal might be to have ref-transaction infrastructure\n> clever enough to coalesce the \"repack-without-these-refs\" requests\n> into one automatically without forcing the callers you are fixing\n> care about these things, though.  And such a transaction semantics\n> may have to also cover the updating of configuration files.\n\nYes, some kind of more advanced transaction mechanism would be great,\nand would likely solve this type of performance issue by design.\n\n/ Jens\n"},{"id":"242562","messageId":"537F2252.3010101@opera.com","threadId":"36707","inReplyTo":"537B2FA4.7020001@opera.com","subject":"[PATCH v2 0/3] remote: optimize rm/prune ref deletion","fromName":"Jens Lindström","fromEmail":"jl@opera.com","sentAt":"2014-05-23T10:26:26Z","receivedAt":"2014-05-23T10:26:26Z","isPatch":true,"sender":{"key":"jl@opera.com","avatar":"https://avatars.githubusercontent.com/u/134918550?v=4"},"body":"Changes since previous version:\n\n * Additionally change the order that 'remote rm' does things so that it\n   removes the remote configuration as the last step and only if the\n   other steps succeeded.\n\n * Change the packed-refs repacking patch to repack before deleting refs\n   instead of after.  This makes the patch simpler, since delete_ref()\n   no longer needs to be told not to repack.\n\nJens Lindstrom (3):\n  remote rm: delete remote configuration as the last step\n  remote: repack packed-refs once when deleting multiple refs\n  remote prune: optimize \"dangling symref\" check/warning\n\n builtin/remote.c | 37 ++++++++++++++++++++++++++++++-------\n refs.c           | 21 +++++++++++++++++++--\n refs.h           |  3 +++\n 3 files changed, 52 insertions(+), 9 deletions(-)\n\n--\n1.9.1\n"},{"id":"242563","messageId":"537F22DB.7070102@opera.com","threadId":"36707","inReplyTo":"537F2252.3010101@opera.com","subject":"[PATCH v2 1/3] remote rm: delete remote configuration as the last","fromName":"Jens Lindström","fromEmail":"jl@opera.com","sentAt":"2014-05-23T10:28:43Z","receivedAt":"2014-05-23T10:28:43Z","isPatch":true,"sender":{"key":"jl@opera.com","avatar":"https://avatars.githubusercontent.com/u/134918550?v=4"},"body":"When removing a remote, delete the remote-tracking branches before\ndeleting the remote configuration.  This way, if the operation fails or\nis aborted while deleting the remote-tracking branches, the command can\nbe rerun to complete the operation.\n\nSigned-off-by: Jens Lindström <jl@opera.com>\n---\n builtin/remote.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex b3ab4cf..84802cd 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -789,10 +789,6 @@ static int rm(int argc, const char **argv)\n \tknown_remotes.to_delete = remote;\n \tfor_each_remote(add_known_remote, &known_remotes);\n \n-\tstrbuf_addf(&buf, \"remote.%s\", remote->name);\n-\tif (git_config_rename_section(buf.buf, NULL) < 1)\n-\t\treturn error(_(\"Could not remove config section '%s'\"), buf.buf);\n-\n \tread_branches();\n \tfor (i = 0; i < branch_list.nr; i++) {\n \t\tstruct string_list_item *item = branch_list.items + i;\n@@ -837,6 +833,12 @@ static int rm(int argc, const char **argv)\n \t}\n \tstring_list_clear(&skipped, 0);\n \n+\tif (!result) {\n+\t\tstrbuf_addf(&buf, \"remote.%s\", remote->name);\n+\t\tif (git_config_rename_section(buf.buf, NULL) < 1)\n+\t\t\treturn error(_(\"Could not remove config section '%s'\"), buf.buf);\n+\t}\n+\n \treturn result;\n }\n \n-- \n1.9.1\n"},{"id":"242564","messageId":"537F2319.7040105@opera.com","threadId":"36707","inReplyTo":"537F2252.3010101@opera.com","subject":"[PATCH v2 2/3] remote: repack packed-refs once when deleting multiple refs","fromName":"Jens Lindström","fromEmail":"jl@opera.com","sentAt":"2014-05-23T10:29:45Z","receivedAt":"2014-05-23T10:29:45Z","isPatch":true,"sender":{"key":"jl@opera.com","avatar":"https://avatars.githubusercontent.com/u/134918550?v=4"},"body":"When 'git remote rm' or 'git remote prune' were used in a repository\nwith many refs, and needed to delete many remote-tracking refs, a lot\nof time was spent deleting those refs since for each deleted ref,\nrepack_without_refs() was called to rewrite packed-refs without just\nthat deleted ref.\n\nTo avoid this, call repack_without_refs() first to repack without all\nthe refs that will be deleted, before calling delete_ref() to delete\neach one completely.  The call to repack_without_ref() in delete_ref()\nthen becomes a no-op, since packed-refs already won't contain any of\nthe deleted refs.\n\nSigned-off-by: Jens Lindström <jl@opera.com>\n---\nNote: remove_branches() no longer checks that the remote-tracking\nbranches it deletes point at the right object before deleting them\nby passing the expected SHA-1 to delete_ref().  This was a required\nchange since all packed refs have been deleted already by the time\nwe call delete_ref(), which causes delete_ref() to fail if given an\nexpected SHA-1 to check.  'remote prune' already behaved this way.\n\n builtin/remote.c | 20 ++++++++++++++++++--\n refs.c           |  2 +-\n refs.h           |  2 ++\n 3 files changed, 21 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 84802cd..d33abe6 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -749,15 +749,23 @@ 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+\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+\tresult |= repack_without_refs(branch_names, branches->nr);\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-\t\tunsigned char *sha1 = item->util;\n \n-\t\tif (delete_ref(refname, sha1, 0))\n+\t\tif (delete_ref(refname, NULL, 0))\n \t\t\tresult |= error(_(\"Could not remove branch %s\"), refname);\n \t}\n+\n \treturn result;\n }\n \n@@ -1305,6 +1313,7 @@ static int prune_remote(const char *remote, int dry_run)\n {\n \tint result = 0, i;\n \tstruct ref_states states;\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@@ -1318,6 +1327,13 @@ 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\tresult |= repack_without_refs(delete_refs, states.stale.nr);\n+\t\tfree(delete_refs);\n \t}\n \n \tfor (i = 0; i < states.stale.nr; i++) {\ndiff --git a/refs.c b/refs.c\nindex 28d5eca..262c1c2 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2431,7 +2431,7 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)\n \treturn 0;\n }\n \n-static int repack_without_refs(const char **refnames, int n)\n+int repack_without_refs(const char **refnames, int n)\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 87a1a79..f287c7a 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -132,6 +132,8 @@ 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+\n extern int ref_exists(const char *);\n \n /*\n-- \n1.9.1\n"},{"id":"242565","messageId":"537F2341.4050702@opera.com","threadId":"36707","inReplyTo":"537F2252.3010101@opera.com","subject":"[PATCH v2 3/3] remote prune: optimize \"dangling symref\" check/warning","fromName":"Jens Lindström","fromEmail":"jl@opera.com","sentAt":"2014-05-23T10:30:25Z","receivedAt":"2014-05-23T10:30:25Z","isPatch":true,"sender":{"key":"jl@opera.com","avatar":"https://avatars.githubusercontent.com/u/134918550?v=4"},"body":"When 'git remote prune' was used to delete many refs in a repository\nwith many refs, a lot of time was spent checking for (now) dangling\nsymbolic refs pointing to the deleted ref, since warn_dangling_symref()\nwas once per deleted ref to check all other refs in the repository.\n\nAvoid this using the new warn_dangling_symrefs() function which\nmakes one pass over all refs and checks for all the deleted refs in\none go, after they have all been deleted.\n\nSigned-off-by: Jens Lindström <jl@opera.com>\n---\n builtin/remote.c |  7 ++++++-\n refs.c           | 19 ++++++++++++++++++-\n refs.h           |  1 +\n 3 files changed, 25 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex d33abe6..9b3e368 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -1313,6 +1313,7 @@ static int prune_remote(const char *remote, int dry_run)\n {\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@@ -1339,6 +1340,8 @@ static int prune_remote(const char *remote, int dry_run)\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 \n@@ -1348,9 +1351,11 @@ static int prune_remote(const char *remote, int dry_run)\n \t\telse\n \t\t\tprintf_ln(_(\" * [pruned] %s\"),\n \t\t\t       abbrev_ref(refname, \"refs/remotes/\"));\n-\t\twarn_dangling_symref(stdout, dangling_msg, refname);\n \t}\n \n+\twarn_dangling_symrefs(stdout, dangling_msg, &delete_refs_list);\n+\tstring_list_clear(&delete_refs_list, 0);\n+\n \tfree_remote_ref_states(&states);\n \treturn result;\n }\ndiff --git a/refs.c b/refs.c\nindex 262c1c2..59fb700 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1611,6 +1611,7 @@ int peel_ref(const char *refname, unsigned char *sha1)\n struct warn_if_dangling_data {\n \tFILE *fp;\n \tconst char *refname;\n+\tconst struct string_list *refnames;\n \tconst char *msg_fmt;\n };\n \n@@ -1625,8 +1626,12 @@ static int warn_if_dangling_symref(const char *refname, const unsigned char *sha\n \t\treturn 0;\n \n \tresolves_to = resolve_ref_unsafe(refname, junk, 0, NULL);\n-\tif (!resolves_to || strcmp(resolves_to, d->refname))\n+\tif (!resolves_to\n+\t    || (d->refname\n+\t\t? strcmp(resolves_to, d->refname)\n+\t\t: !string_list_has_string(d->refnames, resolves_to))) {\n \t\treturn 0;\n+\t}\n \n \tfprintf(d->fp, d->msg_fmt, refname);\n \tfputc('\\n', d->fp);\n@@ -1639,6 +1644,18 @@ void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname)\n \n \tdata.fp = fp;\n \tdata.refname = refname;\n+\tdata.refnames = NULL;\n+\tdata.msg_fmt = msg_fmt;\n+\tfor_each_rawref(warn_if_dangling_symref, &data);\n+}\n+\n+void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, const struct string_list *refnames)\n+{\n+\tstruct warn_if_dangling_data data;\n+\n+\tdata.fp = fp;\n+\tdata.refname = NULL;\n+\tdata.refnames = refnames;\n \tdata.msg_fmt = msg_fmt;\n \tfor_each_rawref(warn_if_dangling_symref, &data);\n }\ndiff --git a/refs.h b/refs.h\nindex f287c7a..1440acc 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -89,6 +89,7 @@ static inline const char *has_glob_specials(const char *pattern)\n extern int for_each_rawref(each_ref_fn, void *);\n \n extern void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname);\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-- \n1.9.1\n"},{"id":"242581","messageId":"xmqq38g0l8we.fsf@gitster.dls.corp.google.com","threadId":"36707","inReplyTo":"CAEef6WyXGXAdm_DkzNsuLgeFFpQsxvTiuJbK3ypc8-Cz2qD_KQ@mail.gmail.com","subject":"Re: [PATCH 1/2] remote: defer repacking packed-refs when deleting refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-23T17:09:05Z","receivedAt":"2014-05-23T17:09:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lindström <jl@opera.com> writes:\n\n> Removing the remote configuration (I assume you mean the\n> \"remote.<name>\" section from .git/config) last in 'remote rm' would be\n> a bit better I think.  Especially ...\n\nYes, that is exactly why I suggested it ;-)\n\n> Doing the repacking first and then run through and delete loose refs\n> and ref logs leads to a smaller and nicer patch as well ...\n\nHmph, that would be a bonus, then.  I suggested it primarily as a\ncorrectness thing against an interrupted operation.\n\n> One additional change was required in\n> builtin/remote.c:remove_branches().  It used to pass in the expected\n> SHA-1 of the ref to delete_ref(), which only works if the ref exists.\n> If repack_without_refs() is called first, most refs won't exist,...\n\nWhy?  A ref, before you start pruning or removing a remote, could be\nin one of these three states.\n\n (a) only loose refs/remotes/them/frotz exists\n (b) both loose and packed refs/remotes/them/nitfol exist\n (c) only packed refs/remotes/them/xyzzy exists\n\nAnd then you repack packed-refs file without these three refs.  When\nyou do so, you know that you would need to remove frotz and nitfol,\nand also you know you do not want to call delete_ref for xyzzy, no?\n\nIn other words, the problem you are describing in remove_branches(),\nthat it wants to make sure that the ref-to-be-removed still points\nat the expected object, does not sound like it is doing anything\ninherently wrong---rather, it sounds like you didn't make necessary\nchanges to the caller to make sure that you do not call delete_ref()\non something you know you removed already.\n\nPuzzled....\n"},{"id":"242594","messageId":"xmqqa9a8jpdy.fsf@gitster.dls.corp.google.com","threadId":"36707","inReplyTo":"537F22DB.7070102@opera.com","subject":"Re: [PATCH v2 1/3] remote rm: delete remote configuration as the last","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-23T18:55:53Z","receivedAt":"2014-05-23T18:55:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lindström <jl@opera.com> writes:\n\n> When removing a remote, delete the remote-tracking branches before\n> deleting the remote configuration.  This way, if the operation fails or\n> is aborted while deleting the remote-tracking branches, the command can\n> be rerun to complete the operation.\n>\n> Signed-off-by: Jens Lindström <jl@opera.com>\n\nI think this is a good change, regardless of the \"calling delete-ref\nmillions of times, each time rewriting the whole packed-ref file, is\ninefficient\" issue.\n\nI wonder if the new \"if (!result)\" block wants to have its own else\nclause to let the users know that the definition of the remote was\nstill left intact under such an unusual condition where ref deletion\nsomehow fails, but it would be OK as the users have presumably seen\nerror messages and understand that removal did not happen.\n\nWill queue.  Thanks.\n\n> ---\n>  builtin/remote.c | 10 ++++++----\n>  1 file changed, 6 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/remote.c b/builtin/remote.c\n> index b3ab4cf..84802cd 100644\n> --- a/builtin/remote.c\n> +++ b/builtin/remote.c\n> @@ -789,10 +789,6 @@ static int rm(int argc, const char **argv)\n>  \tknown_remotes.to_delete = remote;\n>  \tfor_each_remote(add_known_remote, &known_remotes);\n>  \n> -\tstrbuf_addf(&buf, \"remote.%s\", remote->name);\n> -\tif (git_config_rename_section(buf.buf, NULL) < 1)\n> -\t\treturn error(_(\"Could not remove config section '%s'\"), buf.buf);\n> -\n>  \tread_branches();\n>  \tfor (i = 0; i < branch_list.nr; i++) {\n>  \t\tstruct string_list_item *item = branch_list.items + i;\n> @@ -837,6 +833,12 @@ static int rm(int argc, const char **argv)\n>  \t}\n>  \tstring_list_clear(&skipped, 0);\n>  \n> +\tif (!result) {\n> +\t\tstrbuf_addf(&buf, \"remote.%s\", remote->name);\n> +\t\tif (git_config_rename_section(buf.buf, NULL) < 1)\n> +\t\t\treturn error(_(\"Could not remove config section '%s'\"), buf.buf);\n> +\t}\n> +\n>  \treturn result;\n>  }\n"},{"id":"242595","messageId":"xmqq61kwjon6.fsf@gitster.dls.corp.google.com","threadId":"36707","inReplyTo":"537F2319.7040105@opera.com","subject":"Re: [PATCH v2 2/3] remote: repack packed-refs once when deleting multiple refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-23T19:11:57Z","receivedAt":"2014-05-23T19:11:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lindström <jl@opera.com> writes:\n\n> When 'git remote rm' or 'git remote prune' were used in a repository\n> with many refs, and needed to delete many remote-tracking refs, a lot\n> of time was spent deleting those refs since for each deleted ref,\n> repack_without_refs() was called to rewrite packed-refs without just\n> that deleted ref.\n>\n> To avoid this, call repack_without_refs() first to repack without all\n> the refs that will be deleted, before calling delete_ref() to delete\n> each one completely.  The call to repack_without_ref() in delete_ref()\n> then becomes a no-op, since packed-refs already won't contain any of\n> the deleted refs.\n>\n> Signed-off-by: Jens Lindström <jl@opera.com>\n> ---\n> Note: remove_branches() no longer checks that the remote-tracking\n> branches it deletes point at the right object before deleting them\n> by passing the expected SHA-1 to delete_ref().  This was a required\n> change since all packed refs have been deleted already by the time\n> we call delete_ref(), which causes delete_ref() to fail if given an\n> expected SHA-1 to check.  'remote prune' already behaved this way.\n>\n>  builtin/remote.c | 20 ++++++++++++++++++--\n>  refs.c           |  2 +-\n>  refs.h           |  2 ++\n>  3 files changed, 21 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/remote.c b/builtin/remote.c\n> index 84802cd..d33abe6 100644\n> --- a/builtin/remote.c\n> +++ b/builtin/remote.c\n> @@ -749,15 +749,23 @@ 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> +\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> +\tresult |= repack_without_refs(branch_names, branches->nr);\n> +\tfree(branch_names);\n\nHmph.  I wonder if you can refactor/enhance the interface to the\nrepack_without_refs() function before this step in the series, so\nthat the function lets the caller to know if each ref was actually\nremoved or it was not in packed-refs from the beginning, and also I\nwonder if such a refactoring helps.  My gut feeling is that it would\nnot help that much this particular series, but it would probably be\na good thing to do in the longer run. So probably it is better to do\nwithout such a change as a part of this series.\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> -\t\tunsigned char *sha1 = item->util;\n\nHere, you can check if the refname still exists as a ref; if it no\nlonger exists, it would mean that the only copy of the ref was in\nthe packed-refs file and you have already deleted it, and you can\nrefrain from calling delete_ref() altogether, e.g.\n\n\n\tif (!ref_exists(refname))\n\t\tcontinue; /* already removed the sole copy from packed-ref */\n\nand then still retain the safetly against racing somebody else who\ncreated or updated the ref you wanted to remove here by passing the\nobject name to delete_ref().\n\n> -\t\tif (delete_ref(refname, sha1, 0))\n> +\t\tif (delete_ref(refname, NULL, 0))\n>  \t\t\tresult |= error(_(\"Could not remove branch %s\"), refname);\n>  \t}\n> +\n>  \treturn result;\n>  }\n"},{"id":"242597","messageId":"xmqqwqdci9gm.fsf@gitster.dls.corp.google.com","threadId":"36707","inReplyTo":"xmqq61kwjon6.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 2/3] remote: repack packed-refs once when deleting multiple refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-23T19:25:13Z","receivedAt":"2014-05-23T19:25:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> -\t\tif (delete_ref(refname, sha1, 0))\n>> +\t\tif (delete_ref(refname, NULL, 0))\n>>  \t\t\tresult |= error(_(\"Could not remove branch %s\"), refname);\n\nBy the way, how does this series interact with what Ronnie and Michael\nare working on with their ref-transaction series?\n"},{"id":"242638","messageId":"CAEef6Wxg2QkWDzjuogh5sjjE=HZ9NRUmiY9AhExSXK7H3pAcNA@mail.gmail.com","threadId":"36707","inReplyTo":"xmqq38g0l8we.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] remote: defer repacking packed-refs when deleting refs","fromName":"Jens Lindström","fromEmail":"jl@opera.com","sentAt":"2014-05-24T07:54:17Z","receivedAt":"2014-05-24T07:54:17Z","isPatch":true,"sender":{"key":"jl@opera.com","avatar":"https://avatars.githubusercontent.com/u/134918550?v=4"},"body":"On Fri, May 23, 2014 at 7:09 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jens Lindström <jl@opera.com> writes:\n>> One additional change was required in\n>> builtin/remote.c:remove_branches().  It used to pass in the expected\n>> SHA-1 of the ref to delete_ref(), which only works if the ref exists.\n>> If repack_without_refs() is called first, most refs won't exist,...\n>\n> Why?  A ref, before you start pruning or removing a remote, could be\n> in one of these three states.\n>\n>  (a) only loose refs/remotes/them/frotz exists\n>  (b) both loose and packed refs/remotes/them/nitfol exist\n>  (c) only packed refs/remotes/them/xyzzy exists\n>\n> And then you repack packed-refs file without these three refs.  When\n> you do so, you know that you would need to remove frotz and nitfol,\n> and also you know you do not want to call delete_ref for xyzzy, no?\n>\n> In other words, the problem you are describing in remove_branches(),\n> that it wants to make sure that the ref-to-be-removed still points\n> at the expected object, does not sound like it is doing anything\n> inherently wrong---rather, it sounds like you didn't make necessary\n> changes to the caller to make sure that you do not call delete_ref()\n> on something you know you removed already.\n>\n> Puzzled....\n\nThere is one reason why one would want to call delete_ref() even if\nthe ref itself was already fully deleted by repack_without_refs()\n(because it was only packed) and that is that delete_ref() also\nremoves the ref log, if there is one.  We could refactor the deletion\nto\n\n  1) repack_without_refs() on all refs\n  2) delete_ref() on still existing (loose) refs\n  3) delete_ref_log() on all refs\n\nto let us only call delete_ref() on existing refs, and then keep the\ncurrent value check.\n\nPersonally I don't feel that we're solving an important problem here.\nMaking sure not to overwrite a ref that has been updated since we read\nits value is of course of great value for 'receive-pack' and 'commit'\nand such, but in this case we're removing a remote and all its\nremote-tracking branches and configuration.  If a remote-tracking\nbranch is updated concurrently, the current value check would fail,\nand the remote configuration and that one branch would remain.  But if\nthe update had happened just an instant earlier, just before we\nstarted removing the remote, we would have happily deleted that\nremote-tracking branch as well, throwing away whatever information the\nupdate stored in it.\n\n/ Jens\n"},{"id":"242753","messageId":"xmqqha4bgo0p.fsf@gitster.dls.corp.google.com","threadId":"36707","inReplyTo":"CAEef6Wxg2QkWDzjuogh5sjjE=HZ9NRUmiY9AhExSXK7H3pAcNA@mail.gmail.com","subject":"Re: [PATCH 1/2] remote: defer repacking packed-refs when deleting refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-27T16:55:02Z","receivedAt":"2014-05-27T16:55:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lindström <jl@opera.com> writes:\n\n>> Puzzled....\n>\n> There is one reason why one would want to call delete_ref() even if\n> the ref itself was already fully deleted by repack_without_refs()\n> (because it was only packed) and that is that delete_ref() also\n> removes the ref log, if there is one.\n\nAhh, ok, no longer puzzled---I completely forgot about that part.\n\n> We could refactor the deletion to\n>\n>   1) repack_without_refs() on all refs\n>   2) delete_ref() on still existing (loose) refs\n>   3) delete_ref_log() on all refs\n>\n> to let us only call delete_ref() on existing refs, and then keep the\n> current value check.\n\nI tend to agree that it is sufficient for the purpose of this topic\nto be loose about the check; the refactoring can come later, as part\nof the ref-transaction refactoring that is going on in a separate\nthread.\n"}]}