{"thread":{"id":"51608","subject":"[PATCH 1/1] delete multiple tags in a single transaction","startedAt":"2019-08-08T04:00:08Z","lastAt":"2019-08-09T03:05:34Z","messageCount":8,"participants":["Phil Hord","Martin Ågren","Elijah Newren","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"380108","messageId":"20190808035935.30023-1-phil.hord@gmail.com","threadId":"51608","inReplyTo":null,"subject":"[PATCH 1/1] delete multiple tags in a single transaction","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2019-08-08T03:59:35Z","receivedAt":"2019-08-08T04:00:08Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"From: Phil Hord \n\n'git tag -d' accepts one or more tag refs to delete, but each deletion\nis done by calling `delete_ref` on each argv. This is painfully slow\nwhen removing from packed refs. Use delete_refs instead so all the\nremovals can be done inside a single transaction with a single write.\n\nI have a repo with 24,000 tags, most of which are not useful to any\ndevelopers. Having this many refs slows down many operations that\nwould otherwise be very fast. Removing these tags when they've been\naccidentally fetched again takes about 30 minutes using delete_ref.\n\n git tag -l feature/* | xargs git tag -d\n\nRemoving the same tags using delete_refs takes less than 5 seconds.\n\nSigned-off-by: Phil Hord \n---\n builtin/tag.c | 52 +++++++++++++++++++++++++++++++++++++++++----------\n 1 file changed, 42 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex e0a4c25382..f652af83e7 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -72,10 +72,10 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,\n }\n \n typedef int (*each_tag_name_fn)(const char *name, const char *ref,\n-\t\t\t\tconst struct object_id *oid, const void *cb_data);\n+\t\t\t\tconst struct object_id *oid, void *cb_data);\n \n static int for_each_tag_name(const char **argv, each_tag_name_fn fn,\n-\t\t\t const void *cb_data)\n+\t\t\t void *cb_data)\n {\n \tconst char **p;\n \tstruct strbuf ref = STRBUF_INIT;\n@@ -97,18 +97,50 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn,\n \treturn had_error;\n }\n \n-static int delete_tag(const char *name, const char *ref,\n-\t\t const struct object_id *oid, const void *cb_data)\n+struct tag_args {\n+\tchar *oid_abbrev;\n+\tchar *refname;\n+};\n+\n+static int make_string_list(const char *name, const char *ref,\n+\t\t\t const struct object_id *oid, void *cb_data)\n {\n-\tif (delete_ref(NULL, ref, oid, 0))\n-\t\treturn 1;\n-\tprintf(_(\"Deleted tag '%s' (was %s)\\n\"), name,\n-\t find_unique_abbrev(oid, DEFAULT_ABBREV));\n+\tstruct string_list *ref_list = cb_data;\n+\tstruct tag_args *info = xmalloc(sizeof(struct tag_args));\n+\n+\tstring_list_append(ref_list, ref);\n+\n+\tinfo->oid_abbrev = xstrdup(find_unique_abbrev(oid, DEFAULT_ABBREV));\n+\tinfo->refname = xstrdup(name);\n+\tref_list->items[ref_list->nr - 1].util = info;\n \treturn 0;\n }\n \n+static int delete_tags(const char **argv)\n+{\n+\tint result;\n+\tstruct string_list ref_list = STRING_LIST_INIT_DUP;\n+\tstruct string_list_item *ref_list_item;\n+\n+\tresult = for_each_tag_name(argv, make_string_list, (void *) &ref_list);\n+\tif (!result)\n+\t\tresult = delete_refs(NULL, &ref_list, REF_NO_DEREF);\n+\n+\tfor_each_string_list_item(ref_list_item, &ref_list) {\n+\t\tstruct tag_args * info = ref_list_item->util;\n+\t\tif (!result)\n+\t\t\tprintf(_(\"Deleted tag '%s' (was %s)\\n\"), info->refname,\n+\t\t\t\tinfo->oid_abbrev);\n+\t\tfree(info->oid_abbrev);\n+\t\tfree(info->refname);\n+\t\tfree(info);\n+\t}\n+\tstring_list_clear(&ref_list, 0);\n+\treturn result;\n+}\n+\n static int verify_tag(const char *name, const char *ref,\n-\t\t const struct object_id *oid, const void *cb_data)\n+\t\t const struct object_id *oid, void *cb_data)\n {\n \tint flags;\n \tconst struct ref_format *format = cb_data;\n@@ -511,7 +543,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tif (filter.merge_commit)\n \t\tdie(_(\"--merged and --no-merged options are only allowed in list mode\"));\n \tif (cmdmode == 'd')\n-\t\treturn for_each_tag_name(argv, delete_tag, NULL);\n+\t\treturn delete_tags(argv);\n \tif (cmdmode == 'v') {\n \t\tif (format.format && verify_ref_format(&format))\n \t\t\tusage_with_options(git_tag_usage, options);\n-- \n2.23.0.rc1.174.g4cc1b04b4c.dirty\n\n"},{"id":"380124","messageId":"CAN0heSptKHL8mrU9DTXT9T7HDN56a3+DAGczxkEtbGxp9sB8hg@mail.gmail.com","threadId":"51608","inReplyTo":"20190808035935.30023-1-phil.hord@gmail.com","subject":"Re: [PATCH 1/1] delete multiple tags in a single transaction","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-08-08T12:47:20Z","receivedAt":"2019-08-08T12:47:33Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Thu, 8 Aug 2019 at 06:09, Phil Hord wrote:\n> I have a repo with 24,000 tags, most of which are not useful to any\n> developers. Having this many refs slows down many operations that\n> would otherwise be very fast. Removing these tags when they've been\n> accidentally fetched again takes about 30 minutes using delete_ref.\n>\n> git tag -l feature/* | xargs git tag -d\n>\n> Removing the same tags using delete_refs takes less than 5 seconds.\n\nThis looks worthwhile pursuing...\n\n> -static int delete_tag(const char *name, const char *ref,\n> - const struct object_id *oid, const void *cb_data)\n> +struct tag_args {\n> + char *oid_abbrev;\n> + char *refname;\n> +};\n> +\n> +static int make_string_list(const char *name, const char *ref,\n> + const struct object_id *oid, void *cb_data)\n> {\n> - if (delete_ref(NULL, ref, oid, 0))\n> - return 1;\n\nThis provides `oid` for verifying that the tag actually points at that\nparticular oid before deleting. As far as I can tell, `oid` is no longer\nused like that in the post-image. I'm not sure it matters, since we just\nlooked it up, but that might be worth mentioning, perhaps.\n\n> - printf(_(\"Deleted tag '%s' (was %s)\\n\"), name,\n> - find_unique_abbrev(oid, DEFAULT_ABBREV));\n> + struct string_list *ref_list = cb_data;\n> + struct tag_args *info = xmalloc(sizeof(struct tag_args));\n> +\n> + string_list_append(ref_list, ref);\n> +\n> + info->oid_abbrev = xstrdup(find_unique_abbrev(oid, DEFAULT_ABBREV));\n> + info->refname = xstrdup(name);\n> + ref_list->items[ref_list->nr - 1].util = info;\n> return 0;\n> }\n>\n> +static int delete_tags(const char **argv)\n> +{\n> + int result;\n> + struct string_list ref_list = STRING_LIST_INIT_DUP;\n> + struct string_list_item *ref_list_item;\n> +\n> + result = for_each_tag_name(argv, make_string_list, (void *) &ref_list);\n\nIf any tag is non-existing (or some other error happens here), we don't\ncontinue to the actual deleting. That breaks t7004 which has a test for\nremoving an existing and a non-existing tag -- it wants the existing one\nto be removed and the non-existing one not to interfere.\n\n> + if (!result)\n> + result = delete_refs(NULL, &ref_list, REF_NO_DEREF);\n\nSo this should perhaps be something more like an unconditional\n\n result |= delete_refs(...);\n\nThat makes the test suite happy, but perhaps only short-term ... See\nbelow...\n\n> + for_each_string_list_item(ref_list_item, &ref_list) {\n> + struct tag_args * info = ref_list_item->util;\n> + if (!result)\n> + printf(_(\"Deleted tag '%s' (was %s)\\n\"), info->refname,\n> + info->oid_abbrev);\n\nChange this conditional here, too, methinks. You'd need to separate\nerrors from looking up tags from errors about deleting refs, so having a\nsingle \"result\" is probably not sufficient.\n\nProbably worth inspecting the output of that `git tag -d` a bit in\nt7004, to make sure we just claim to delete one tag, and have errors.\n\nYour patch reshuffles the error and success messages (for certain\nusages). I think that's ok, but might be worth mentioning.\n\nI'm not too familiar with the refs API, so take this with a grain of\nsalt...\n\n> + free(info->oid_abbrev);\n> + free(info->refname);\n> + free(info);\n> + }\n> + string_list_clear(&ref_list, 0);\n> + return result;\n> +}\n\nMartin\n"},{"id":"380125","messageId":"20190808125330.3104954-1-martin.agren@gmail.com","threadId":"51608","inReplyTo":"CAN0heSptKHL8mrU9DTXT9T7HDN56a3+DAGczxkEtbGxp9sB8hg@mail.gmail.com","subject":"[PATCH] t7004: check existence of correct tag","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-08-08T12:53:30Z","receivedAt":"2019-08-08T12:53:47Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"We try to delete the non-existing tag \"anothertag\", but for the\nverifications, we check that the tag \"myhead\" doesn't exist. \"myhead\"\nisn't used in this test except for this checking. Comparing to the test\ntwo tests earlier, it looks like a copy-paste mistake.\n\nPerhaps it's overkill to check that `git tag -d` didn't decide to\n*create* a tag. But since we're trying to be this careful, let's\nactually check the correct tag. While we're doing this, let's use a more\ndescriptive tag name instead -- \"nonexistingtag\" should be obvious.\n\nSigned-off-by: Martin Ågren \n---\n\n > Probably worth inspecting the output of that `git tag -d` a bit in\n > t7004, to make sure we just claim to delete one tag, and have errors.\n\n Here's something I noticed while looking into this test. If you do\n expand on this test, feel free to pick this up, either as a preparatory\n patch or squash it into your \"expand test\" patch (if you do one). If\n you don't pick this up at all, I'll pursue it separately, later.\n\n Martin\n\n t/t7004-tag.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 80eb13d94e..e4cf605907 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -227,10 +227,10 @@ test_expect_success \\\n test_expect_success \\\n \t'trying to delete two tags, existing and not, should fail in the 2nd' '\n \ttag_exists mytag &&\n-\t! tag_exists myhead &&\n-\ttest_must_fail git tag -d mytag anothertag &&\n+\t! tag_exists nonexistingtag &&\n+\ttest_must_fail git tag -d mytag nonexistingtag &&\n \t! tag_exists mytag &&\n-\t! tag_exists myhead\n+\t! tag_exists nonexistingtag\n '\n \n test_expect_success 'trying to delete an already deleted tag should fail' \\\n-- \n2.23.0.rc0.30.g51cf315870\n\n"},{"id":"380151","messageId":"CABPp-BFH++aJinkzg+qsZDRN6R5-E8LPCG_u+udZLW6o0MGBug@mail.gmail.com","threadId":"51608","inReplyTo":"20190808035935.30023-1-phil.hord@gmail.com","subject":"Re: [PATCH 1/1] delete multiple tags in a single transaction","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-08-08T18:15:24Z","receivedAt":"2019-08-08T18:15:38Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Aug 7, 2019 at 9:11 PM Phil Hord wrote:\n>\n> From: Phil Hord \n>\n> 'git tag -d' accepts one or more tag refs to delete, but each deletion\n> is done by calling `delete_ref` on each argv. This is painfully slow\n> when removing from packed refs. Use delete_refs instead so all the\n> removals can be done inside a single transaction with a single write.\n\nNice, thanks for working on this.\n\n> I have a repo with 24,000 tags, most of which are not useful to any\n> developers. Having this many refs slows down many operations that\n> would otherwise be very fast. Removing these tags when they've been\n> accidentally fetched again takes about 30 minutes using delete_ref.\n\nI also get really slow times on a repo with ~20,000 tags (though order\n~3 minutes rather than ~30, probably due to having an SSD on this\nmachine) -- but ONLY IF the refs are packed first (git pack-refs\n--all). If the refs are loose, it's relatively quick to delete a\ndozen thousand or so tags (order of a few seconds). It might be worth\nmentioning in the commit message that this only makes a significant\ndifference in the case where the refs are packed.\n\n> git tag -l feature/* | xargs git tag -d\n>\n> Removing the same tags using delete_refs takes less than 5 seconds.\n\nIt appears this same bug also affects `git branch -d` when deleting\nlots of branches (or remote tracking branches) and they are all\npacked; could you apply the same fix there?\n\nIn constrast, it appears that `git update-ref --stdin` is fast\nregardless of whether the refs are packed, e.g.\n git tag -l feature/* | sed -e 's%^%delete refs/tags/%' | git\nupdate-ref --stdin\nfinishes quickly (order of a few seconds).\n"},{"id":"380161","messageId":"xmqq4l2rfnvl.fsf@gitster-ct.c.googlers.com","threadId":"51608","inReplyTo":"20190808035935.30023-1-phil.hord@gmail.com","subject":"Re: [PATCH 1/1] delete multiple tags in a single transaction","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-08T19:39:10Z","receivedAt":"2019-08-08T19:39:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phil Hord writes:\n\n> From: Phil Hord \n>\n> 'git tag -d' accepts one or more tag refs to delete, but each deletion\n> is done by calling `delete_ref` on each argv. This is painfully slow\n> when removing from packed refs. Use delete_refs instead so all the\n> removals can be done inside a single transaction with a single write.\n>\n> I have a repo with 24,000 tags, most of which are not useful to any\n> developers. Having this many refs slows down many operations that\n> would otherwise be very fast. Removing these tags when they've been\n> accidentally fetched again takes about 30 minutes using delete_ref.\n>\n> git tag -l feature/* | xargs git tag -d\n>\n> Removing the same tags using delete_refs takes less than 5 seconds.\n\nMakes sense. As mentioned elsewhere in the thread already,\na batched update-ref would open the packed-refs ony once because\neverything is done in a single transaction, so presumably a pipeline\nlike this\n\n\tgit tag -l feature/* | \n\tsed -e 's|^|delete refs/tags/|' |\n\tgit update-ref --stdin\n\nmay work well, and \"git tag -d\" that gets these refs on the command\nline should be capable of doing the same.\n\n> -static int delete_tag(const char *name, const char *ref,\n> -\t\t const struct object_id *oid, const void *cb_data)\n> +struct tag_args {\n> +\tchar *oid_abbrev;\n> +\tchar *refname;\n> +};\n> +\n> +static int make_string_list(const char *name, const char *ref,\n> +\t\t\t const struct object_id *oid, void *cb_data)\n\nPlease think about a few more minutes before naming a function like\nthis, and make it a habit for your future patches.\n\nWe can see that the callback is used to insert more strings into a\nstring list, but the type (i.e. string_list) used to represent the\nset is not all that important. What is more important is why you\nare building that set for, and saying what is in the set (as opposed\nto saying that the container happens to be a string_list) would be a\ngood first step.\n\nI presume that you are enumerating the tags to be deleted, together\nwith the data necessary for you to report the deletion of the tags?\n\n> {\n> -\tif (delete_ref(NULL, ref, oid, 0))\n> -\t\treturn 1;\n> -\tprintf(_(\"Deleted tag '%s' (was %s)\\n\"), name,\n> -\t find_unique_abbrev(oid, DEFAULT_ABBREV));\n> +\tstruct string_list *ref_list = cb_data;\n> +\tstruct tag_args *info = xmalloc(sizeof(struct tag_args));\n> +\n> +\tstring_list_append(ref_list, ref);\n> +\n> +\tinfo->oid_abbrev = xstrdup(find_unique_abbrev(oid, DEFAULT_ABBREV));\n> +\tinfo->refname = xstrdup(name);\n> +\tref_list->items[ref_list->nr - 1].util = info;\n> \treturn 0;\n> }\n> \n> +static int delete_tags(const char **argv)\n> +{\n> +\tint result;\n> +\tstruct string_list ref_list = STRING_LIST_INIT_DUP;\n> +\tstruct string_list_item *ref_list_item;\n> +\n> +\tresult = for_each_tag_name(argv, make_string_list, (void *) &ref_list);\n> +\tif (!result)\n> +\t\tresult = delete_refs(NULL, &ref_list, REF_NO_DEREF);\n> +\n> +\tfor_each_string_list_item(ref_list_item, &ref_list) {\n> +\t\tstruct tag_args * info = ref_list_item->util;\n> +\t\tif (!result)\n> +\t\t\tprintf(_(\"Deleted tag '%s' (was %s)\\n\"), info->refname,\n> +\t\t\t\tinfo->oid_abbrev);\n> +\t\tfree(info->oid_abbrev);\n> +\t\tfree(info->refname);\n> +\t\tfree(info);\n\nIt is not performance critical, but info->refname is computable from\nref_list_item->string, isn't it? I am just wondering if we can do\nthis without having to allocate the .util field for each of 20,000\ntags. We still need to remember oid (or oid_abbrev, but if I were\nwriting this, I'd record the full oid in .util and make the code\nthat prints call find_unique_abbrev() on it), so I guess we cannot\nreally leave .util NULL.\n\n> +\t}\n> +\tstring_list_clear(&ref_list, 0);\n> +\treturn result;\n\nWe used to return the returned value from for_each_tag_name() that\nrepeatedly called delete_tag(). \n\nNow we return value from delete_refs(). Are our caller(s) OK with\nthe values that may come back from that function? Can delete_refs()\nreturn a value that is not appropriate to be returned from\ncmd_tag(), for example a negative value?\n\n> +}\n> +\n> static int verify_tag(const char *name, const char *ref,\n> -\t\t const struct object_id *oid, const void *cb_data)\n> +\t\t const struct object_id *oid, void *cb_data)\n> {\n> \tint flags;\n> \tconst struct ref_format *format = cb_data;\n> @@ -511,7 +543,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n> \tif (filter.merge_commit)\n> \t\tdie(_(\"--merged and --no-merged options are only allowed in list mode\"));\n> \tif (cmdmode == 'd')\n> -\t\treturn for_each_tag_name(argv, delete_tag, NULL);\n> +\t\treturn delete_tags(argv);\n\nThanks.\n"},{"id":"380183","messageId":"CABURp0p5xbsq+8UsFerMAY8EG-ndXgd19EUsHOgQG-dnDnTAgg@mail.gmail.com","threadId":"51608","inReplyTo":"CABPp-BFH++aJinkzg+qsZDRN6R5-E8LPCG_u+udZLW6o0MGBug@mail.gmail.com","subject":"Re: [PATCH 1/1] delete multiple tags in a single transaction","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2019-08-08T23:43:16Z","receivedAt":"2019-08-08T23:43:31Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"On Thu, Aug 8, 2019 at 11:15 AM Elijah Newren wrote:\n>\n> On Wed, Aug 7, 2019 at 9:11 PM Phil Hord wrote:\n> >\n> > From: Phil Hord \n> >\n> > 'git tag -d' accepts one or more tag refs to delete, but each deletion\n> > is done by calling `delete_ref` on each argv. This is painfully slow\n> > when removing from packed refs. Use delete_refs instead so all the\n> > removals can be done inside a single transaction with a single write.\n>\n> Nice, thanks for working on this.\n>\n> > I have a repo with 24,000 tags, most of which are not useful to any\n> > developers. Having this many refs slows down many operations that\n> > would otherwise be very fast. Removing these tags when they've been\n> > accidentally fetched again takes about 30 minutes using delete_ref.\n>\n> I also get really slow times on a repo with ~20,000 tags (though order\n> ~3 minutes rather than ~30, probably due to having an SSD on this\n> machine) -- but ONLY IF the refs are packed first (git pack-refs\n> --all). If the refs are loose, it's relatively quick to delete a\n> dozen thousand or so tags (order of a few seconds). It might be worth\n> mentioning in the commit message that this only makes a significant\n> difference in the case where the refs are packed.\n\nI'm also using an SSD but I still see about 10 tags per second being\ndeleted with the current code (and packed-refs). I see that I'm\nCPU-bound, so I guess most of the time is spent searching through\n.git/packed-refs. Probably it will run faster as it progresses. I\nguess the 18,000 branches in my repo keep me on the wrong end of O(N).\n\nMy VM is on an all-flash storage array, but I can't say much about its\nwrite throughput since it's one VM among many.\n\nPreviously I thought I saw a significant speedup between v2.7.4 (on my\ndevelopment vm) and v2.22.0 (on my laptop). But this week I saw it was\nslow again on my laptop. I looked for the regression but didn't find\nanyone touching that code. Then I wrote this patch.\n\nBut it should have occurred to me while I was in the code that there\nis a different path for unpacked refs which could explain my previous\nspeeds. I didn't think I had any unpacked refs, though, since every\ntime I look in .git/refs for what I want, I find it relatively empty.\nI see 'git pack-refs --help' says that new refs should show up loose,\nbut I can't say that has happened for me. Maybe a new clone uses\npacked-refs for *everything* and only newly fetched things are loose.\nIs that it? I guess since I seldom fetch tags after the first clone,\nit makes sense they would all be packed.\n\n> > git tag -l feature/* | xargs git tag -d\n> >\n> > Removing the same tags using delete_refs takes less than 5 seconds.\n>\n> It appears this same bug also affects `git branch -d` when deleting\n> lots of branches (or remote tracking branches) and they are all\n> packed; could you apply the same fix there?\n\nWill do.\n\n> In constrast, it appears that `git update-ref --stdin` is fast\n> regardless of whether the refs are packed, e.g.\n> git tag -l feature/* | sed -e 's%^%delete refs/tags/%' | git\n> update-ref --stdin\n> finishes quickly (order of a few seconds).\n\nNice! That trick is going in my wiki for devs to use on their VMs.\nThanks for that.\n"},{"id":"380184","messageId":"CABURp0qHmuNQD4qxL8A5fCJaRsNZfZ51d3e2N3nD-x0jCMhnBw@mail.gmail.com","threadId":"51608","inReplyTo":"xmqq4l2rfnvl.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] delete multiple tags in a single transaction","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2019-08-08T23:58:30Z","receivedAt":"2019-08-08T23:58:48Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"On Thu, Aug 8, 2019 at 12:39 PM Junio C Hamano wrote:\n>\n> Phil Hord writes:\n>\n> > From: Phil Hord \n> >\n> > 'git tag -d' accepts one or more tag refs to delete, but each deletion\n> > is done by calling `delete_ref` on each argv. This is painfully slow\n> > when removing from packed refs. Use delete_refs instead so all the\n> > removals can be done inside a single transaction with a single write.\n> >\n> > I have a repo with 24,000 tags, most of which are not useful to any\n> > developers. Having this many refs slows down many operations that\n> > would otherwise be very fast. Removing these tags when they've been\n> > accidentally fetched again takes about 30 minutes using delete_ref.\n> >\n> > git tag -l feature/* | xargs git tag -d\n> >\n> > Removing the same tags using delete_refs takes less than 5 seconds.\n>\n> Makes sense. As mentioned elsewhere in the thread already,\n> a batched update-ref would open the packed-refs ony once because\n> everything is done in a single transaction, so presumably a pipeline\n> like this\n>\n> git tag -l feature/* |\n> sed -e 's|^|delete refs/tags/|' |\n> git update-ref --stdin\n>\n> may work well, and \"git tag -d\" that gets these refs on the command\n> line should be capable of doing the same.\n>\n> > -static int delete_tag(const char *name, const char *ref,\n> > - const struct object_id *oid, const void *cb_data)\n> > +struct tag_args {\n> > + char *oid_abbrev;\n> > + char *refname;\n> > +};\n> > +\n> > +static int make_string_list(const char *name, const char *ref,\n> > + const struct object_id *oid, void *cb_data)\n>\n> Please think about a few more minutes before naming a function like\n> this, and make it a habit for your future patches.\n>\n> We can see that the callback is used to insert more strings into a\n> string list, but the type (i.e. string_list) used to represent the\n> set is not all that important. What is more important is why you\n> are building that set for, and saying what is in the set (as opposed\n> to saying that the container happens to be a string_list) would be a\n> good first step.\n>\n> I presume that you are enumerating the tags to be deleted, together\n> with the data necessary for you to report the deletion of the tags?\n\nHm. collect_tags? collect_tags_to_delete?\n\nIt's true I didn't put enought thought into that. I was experimenting\na bit here and was surprised how little code I ended up needing.\n\n> > {\n> > - if (delete_ref(NULL, ref, oid, 0))\n> > - return 1;\n> > - printf(_(\"Deleted tag '%s' (was %s)\\n\"), name,\n> > - find_unique_abbrev(oid, DEFAULT_ABBREV));\n> > + struct string_list *ref_list = cb_data;\n> > + struct tag_args *info = xmalloc(sizeof(struct tag_args));\n> > +\n> > + string_list_append(ref_list, ref);\n> > +\n> > + info->oid_abbrev = xstrdup(find_unique_abbrev(oid, DEFAULT_ABBREV));\n> > + info->refname = xstrdup(name);\n> > + ref_list->items[ref_list->nr - 1].util = info;\n> > return 0;\n> > }\n> >\n> > +static int delete_tags(const char **argv)\n> > +{\n> > + int result;\n> > + struct string_list ref_list = STRING_LIST_INIT_DUP;\n> > + struct string_list_item *ref_list_item;\n> > +\n> > + result = for_each_tag_name(argv, make_string_list, (void *) &ref_list);\n> > + if (!result)\n> > + result = delete_refs(NULL, &ref_list, REF_NO_DEREF);\n> > +\n> > + for_each_string_list_item(ref_list_item, &ref_list) {\n> > + struct tag_args * info = ref_list_item->util;\n> > + if (!result)\n> > + printf(_(\"Deleted tag '%s' (was %s)\\n\"), info->refname,\n> > + info->oid_abbrev);\n> > + free(info->oid_abbrev);\n> > + free(info->refname);\n> > + free(info);\n>\n> It is not performance critical, but info->refname is computable from\n> ref_list_item->string, isn't it?\n\nOh, I guess it is. It's a fixed offset into the string, after all.\nThanks. I did look for a way to avoid the struct noise. Just not\nwell.\n\n> I am just wondering if we can do\n> this without having to allocate the .util field for each of 20,000\n> tags. We still need to remember oid (or oid_abbrev, but if I were\n> writing this, I'd record the full oid in .util and make the code\n> that prints call find_unique_abbrev() on it), so I guess we cannot\n> really leave .util NULL.\n\nMy original patch did this (.util = oid). But then I needed a name.\nI'll go back to keeping the oid. Much cleaner.\n\n>\n> > + }\n> > + string_list_clear(&ref_list, 0);\n> > + return result;\n>\n> We used to return the returned value from for_each_tag_name() that\n> repeatedly called delete_tag().\n>\n> Now we return value from delete_refs(). Are our caller(s) OK with\n> the values that may come back from that function? Can delete_refs()\n> return a value that is not appropriate to be returned from\n> cmd_tag(), for example a negative value?\n\nYes it does. Will fix.\n\n>\n> > +}\n> > +\n> > static int verify_tag(const char *name, const char *ref,\n> > - const struct object_id *oid, const void *cb_data)\n> > + const struct object_id *oid, void *cb_data)\n> > {\n> > int flags;\n> > const struct ref_format *format = cb_data;\n> > @@ -511,7 +543,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n> > if (filter.merge_commit)\n> > die(_(\"--merged and --no-merged options are only allowed in list mode\"));\n> > if (cmdmode == 'd')\n> > - return for_each_tag_name(argv, delete_tag, NULL);\n> > + return delete_tags(argv);\n>\n> Thanks.\n"},{"id":"380194","messageId":"20190809030531.GA14576@sigill.intra.peff.net","threadId":"51608","inReplyTo":"CABURp0p5xbsq+8UsFerMAY8EG-ndXgd19EUsHOgQG-dnDnTAgg@mail.gmail.com","subject":"Re: [PATCH 1/1] delete multiple tags in a single transaction","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-09T03:05:31Z","receivedAt":"2019-08-09T03:05:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 08, 2019 at 04:43:16PM -0700, Phil Hord wrote:\n\n> > I also get really slow times on a repo with ~20,000 tags (though order\n> > ~3 minutes rather than ~30, probably due to having an SSD on this\n> > machine) -- but ONLY IF the refs are packed first (git pack-refs\n> > --all). If the refs are loose, it's relatively quick to delete a\n> > dozen thousand or so tags (order of a few seconds). It might be worth\n> > mentioning in the commit message that this only makes a significant\n> > difference in the case where the refs are packed.\n> \n> I'm also using an SSD but I still see about 10 tags per second being\n> deleted with the current code (and packed-refs). I see that I'm\n> CPU-bound, so I guess most of the time is spent searching through\n> .git/packed-refs. Probably it will run faster as it progresses. I\n> guess the 18,000 branches in my repo keep me on the wrong end of O(N).\n\nRight, deleting individually from packed-refs is inherently quadratic,\nbecause each deletion has to rewrite the entire file. So if you delete\nall (or the majority of them), that's O(n^2) individual entry writes.\n\nThe loose case is just touching the filesystem for each entry (and the\nrefs code is smart enough not to bother rewriting packed-refs if the\nentry isn't present there). That _can_ be slow if you have a lot of\nentries in the same directory (because some filesystems are particularly\nbad at this).\n\nSo the actual backing storage speed isn't really that important. All the\ntime goes to copying the same packed-refs entries over and over, whether\nthey hit the disk or not.\n\nYour solution (using a single transaction) is definitely the right one\n(and probably should apply to \"branch -d\", too). That's what we did long\nago for update-ref, and I think nobody ever really noticed for the\nporcelain commands because they don't tend to be used for such bulk\nchanges.\n\n> But it should have occurred to me while I was in the code that there\n> is a different path for unpacked refs which could explain my previous\n> speeds. I didn't think I had any unpacked refs, though, since every\n> time I look in .git/refs for what I want, I find it relatively empty.\n> I see 'git pack-refs --help' says that new refs should show up loose,\n> but I can't say that has happened for me. Maybe a new clone uses\n> packed-refs for *everything* and only newly fetched things are loose.\n> Is that it? I guess since I seldom fetch tags after the first clone,\n> it makes sense they would all be packed.\n\nRight, a fresh clone always writes all of its entries as packed refs.\nIt used to be done by hand, but it happens in a special \"initial\ntransaction\" method these days, since 58f233ce1e\n(initial_ref_transaction_commit(): function for initial ref creation,\n2015-06-22).\n\n> > In constrast, it appears that `git update-ref --stdin` is fast\n> > regardless of whether the refs are packed, e.g.\n> > git tag -l feature/* | sed -e 's%^%delete refs/tags/%' | git\n> > update-ref --stdin\n> > finishes quickly (order of a few seconds).\n> \n> Nice! That trick is going in my wiki for devs to use on their VMs.\n> Thanks for that.\n\nPlease do encourage people to use for-each-ref instead of the \"tag -l\"\nporcelain, as the latter is subject to change. My usual bulk deletion\ncommand is:\n\n git for-each-ref --format='delete %(refname)' refs/tags/feature/ |\n git update-ref --stdin\n\n-Peff\n"}]}