{"thread":{"id":"55022","subject":"[PATCH v3] use delete_refs when deleting tags or branches","startedAt":"2021-01-21T05:57:44Z","lastAt":"2021-01-22T20:34:13Z","messageCount":6,"participants":["Phil Hord","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"414889","messageId":"20210121032332.658991-1-phil.hord@gmail.com","threadId":"55022","inReplyTo":null,"subject":"[PATCH v3] use delete_refs when deleting tags or branches","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2021-01-21T03:23:32Z","receivedAt":"2021-01-21T05:57:44Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"From: Phil Hord <phil.hord@gmail.com>\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 very slow when\nremoving from packed refs. Use delete_refs instead so all the removals\ncan be done inside a single transaction with a single update.\n\nDo the same for 'git branch -d'.\n\nSince delete_refs performs all the packed-refs delete operations\ninside a single transaction, if any of the deletes fail then all\nthem will be skipped. In practice, none of them should fail since\nwe verify the hash of each one before calling delete_refs, but some\nnetwork error or odd permissions problem could have different results\nafter this change.\n\nAlso, since the file-backed deletions are not performed in the same\ntransaction, those could succeed even when the packed-refs transaction\nfails.\n\nAfter deleting branches, remove the branch config only if the branch\nref was removed and was not subsequently added back in.\n\nA manual test deleting 24,000 tags took about 30 minutes using\ndelete_ref.  It takes about 5 seconds using delete_refs.\n\nAcked-by: Elijah Newren <newren@gmail.com>\nSigned-off-by: Phil Hord <phil.hord@gmail.com>\n---\n\nThis version translates a nonzero return code from delete_refs into an \nerror return value of 1. It also has style cleanups suggested from v2.\n\n builtin/branch.c | 47 ++++++++++++++++++++++++++++-------------------\n builtin/tag.c    | 44 ++++++++++++++++++++++++++++++++++----------\n 2 files changed, 62 insertions(+), 29 deletions(-)\n\ndiff --git builtin/branch.c builtin/branch.c\nindex 8c0b428104..bcc00bcf18 100644\n--- builtin/branch.c\n+++ builtin/branch.c\n@@ -202,6 +202,9 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \tint remote_branch = 0;\n \tstruct strbuf bname = STRBUF_INIT;\n \tunsigned allowed_interpret;\n+\tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\n+\tstruct string_list_item *item;\n+\tint branch_name_pos;\n \n \tswitch (kinds) {\n \tcase FILTER_REFS_REMOTES:\n@@ -219,6 +222,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \tdefault:\n \t\tdie(_(\"cannot use -a with -d\"));\n \t}\n+\tbranch_name_pos = strcspn(fmt, \"%\");\n \n \tif (!force) {\n \t\thead_rev = lookup_commit_reference(the_repository, &head_oid);\n@@ -265,30 +269,35 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\t\tgoto next;\n \t\t}\n \n-\t\tif (delete_ref(NULL, name, is_null_oid(&oid) ? NULL : &oid,\n-\t\t\t       REF_NO_DEREF)) {\n-\t\t\terror(remote_branch\n-\t\t\t      ? _(\"Error deleting remote-tracking branch '%s'\")\n-\t\t\t      : _(\"Error deleting branch '%s'\"),\n-\t\t\t      bname.buf);\n-\t\t\tret = 1;\n-\t\t\tgoto next;\n-\t\t}\n-\t\tif (!quiet) {\n-\t\t\tprintf(remote_branch\n-\t\t\t       ? _(\"Deleted remote-tracking branch %s (was %s).\\n\")\n-\t\t\t       : _(\"Deleted branch %s (was %s).\\n\"),\n-\t\t\t       bname.buf,\n-\t\t\t       (flags & REF_ISBROKEN) ? \"broken\"\n-\t\t\t       : (flags & REF_ISSYMREF) ? target\n-\t\t\t       : find_unique_abbrev(&oid, DEFAULT_ABBREV));\n-\t\t}\n-\t\tdelete_branch_config(bname.buf);\n+\t\titem = string_list_append(&refs_to_delete, name);\n+\t\titem->util = xstrdup((flags & REF_ISBROKEN) ? \"broken\"\n+\t\t\t\t    : (flags & REF_ISSYMREF) ? target\n+\t\t\t\t    : find_unique_abbrev(&oid, DEFAULT_ABBREV));\n \n \tnext:\n \t\tfree(target);\n \t}\n \n+\tif (delete_refs(NULL, &refs_to_delete, REF_NO_DEREF))\n+\t\tret = 1;\n+\n+\tfor_each_string_list_item(item, &refs_to_delete) {\n+\t\tchar *describe_ref = item->util;\n+\t\tchar *name = item->string;\n+\t\tif (!ref_exists(name)) {\n+\t\t\tchar *refname = name + branch_name_pos;\n+\t\t\tif (!quiet)\n+\t\t\t\tprintf(remote_branch\n+\t\t\t\t\t? _(\"Deleted remote-tracking branch %s (was %s).\\n\")\n+\t\t\t\t\t: _(\"Deleted branch %s (was %s).\\n\"),\n+\t\t\t\t\tname + branch_name_pos, describe_ref);\n+\n+\t\t\tdelete_branch_config(refname);\n+\t\t}\n+\t\tfree(describe_ref);\n+\t}\n+\tstring_list_clear(&refs_to_delete, 0);\n+\n \tfree(name);\n \tstrbuf_release(&bname);\n \ndiff --git builtin/tag.c builtin/tag.c\nindex 24d35b746d..e8b85eefd8 100644\n--- builtin/tag.c\n+++ 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,42 @@ 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+static int collect_tags(const char *name, const char *ref,\n+\t\t\tconst 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+\n+\tstring_list_append(ref_list, ref);\n+\tref_list->items[ref_list->nr - 1].util = oiddup(oid);\n \treturn 0;\n }\n \n+static int delete_tags(const char **argv)\n+{\n+\tint result;\n+\tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\n+\tstruct string_list_item *item;\n+\n+\tresult = for_each_tag_name(argv, collect_tags, (void *)&refs_to_delete);\n+\tif (delete_refs(NULL, &refs_to_delete, REF_NO_DEREF))\n+\t\tresult = 1;\n+\n+\tfor_each_string_list_item(item, &refs_to_delete) {\n+\t\tconst char *name = item->string;\n+\t\tstruct object_id *oid = item->util;\n+\t\tif (!ref_exists(name))\n+\t\t\tprintf(_(\"Deleted tag '%s' (was %s)\\n\"),\n+\t\t\t\titem->string + 10,\n+\t\t\t\tfind_unique_abbrev(oid, DEFAULT_ABBREV));\n+\n+\t\tfree(oid);\n+\t}\n+\tstring_list_clear(&refs_to_delete, 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@@ -512,7 +536,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tif (filter.reachable_from || filter.unreachable_from)\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.30.0.281.g914876b2ce\n\n"},{"id":"414944","messageId":"xmqqpn1xalav.fsf@gitster.c.googlers.com","threadId":"55022","inReplyTo":"20210121032332.658991-1-phil.hord@gmail.com","subject":"Re: [PATCH v3] use delete_refs when deleting tags or branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-22T00:04:56Z","receivedAt":"2021-01-22T00:06:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phil Hord <phil.hord@gmail.com> writes:\n\n> diff --git builtin/branch.c builtin/branch.c\n> index 8c0b428104..bcc00bcf18 100644\n> --- builtin/branch.c\n> +++ builtin/branch.c\n\nYou wasted 15 minutes of my life by choosing to deviate the list\nnorm of sending what \"git apply -p1\" (default) would accept.  I am\nfairly trusting type and did not suspect anybody do such an evil\nthing.  Why?\n"},{"id":"414945","messageId":"CABURp0pqdK+Mrqi=r40YeUitaB2s44iYO=2UFFSh0UC_o4Mosg@mail.gmail.com","threadId":"55022","inReplyTo":"xmqqpn1xalav.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3] use delete_refs when deleting tags or branches","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2021-01-22T00:27:22Z","receivedAt":"2021-01-22T00:28:44Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"On Thu, Jan 21, 2021 at 4:05 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Phil Hord <phil.hord@gmail.com> writes:\n>\n> > diff --git builtin/branch.c builtin/branch.c\n> > index 8c0b428104..bcc00bcf18 100644\n> > --- builtin/branch.c\n> > +++ builtin/branch.c\n>\n> You wasted 15 minutes of my life by choosing to deviate the list\n> norm of sending what \"git apply -p1\" (default) would accept.  I am\n> fairly trusting type and did not suspect anybody do such an evil\n> thing.  Why?\n\nOof.  Sorry.  I forgot I have diff.noprefix=true in my local config.\nIt is a huge timesaver for me when looking at diffs on a console since\nI can quickly highlight the filename with a mouse to paste into an\neditor.\n\nSometimes it bites me, though.  Usually I notice in the diff, but this\none I was sending with format-patch / send-email.\n\nI guess I'll turn that off in git.git so I don't misfire at you again someday.\n\nPhil\n"},{"id":"414948","messageId":"xmqqlfclaf6b.fsf@gitster.c.googlers.com","threadId":"55022","inReplyTo":"CABURp0pqdK+Mrqi=r40YeUitaB2s44iYO=2UFFSh0UC_o4Mosg@mail.gmail.com","subject":"Re: [PATCH v3] use delete_refs when deleting tags or branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-22T02:17:16Z","receivedAt":"2021-01-22T02:18:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phil Hord <phil.hord@gmail.com> writes:\n\n> Oof.  Sorry.  I forgot I have diff.noprefix=true in my local config.\n> It is a huge timesaver for me when looking at diffs on a console since\n> I can quickly highlight the filename with a mouse to paste into an\n> editor.\n>\n> Sometimes it bites me, though.  Usually I notice in the diff, but this\n> one I was sending with format-patch / send-email.\n>\n> I guess I'll turn that off in git.git so I don't misfire at you again someday.\n\nI think per-repository configuration might be sufficient for this\nparticular case (after all, it is project's preference), I wonder if\na more command-specific variant of diff.noprefix so that \"log -p\"\nand \"format-patch\" can be configured separately would make sense,\nsomething like...\n\n    [diff]\n\tnoprefix = true\n    [diff \"format-patch\"]\n\tnoprefix = false\n    [diff \"show\"]\n\tnoprefix = false\n\n"},{"id":"414950","messageId":"xmqqczxxae0c.fsf@gitster.c.googlers.com","threadId":"55022","inReplyTo":"20210121032332.658991-1-phil.hord@gmail.com","subject":"Re: [PATCH v3] use delete_refs when deleting tags or branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-22T02:42:27Z","receivedAt":"2021-01-22T02:43:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phil Hord <phil.hord@gmail.com> writes:\n\n> After deleting branches, remove the branch config only if the branch\n> ref was removed and was not subsequently added back in.\n>\n> A manual test deleting 24,000 tags took about 30 minutes using\n> delete_ref.  It takes about 5 seconds using delete_refs.\n\nNicely done.  Queued and pushed out.\n"},{"id":"414991","messageId":"CABURp0rvK=53L6UsSirZ0bs-FhK+s_3s7VNvwhR4ky-Pf7HEog@mail.gmail.com","threadId":"55022","inReplyTo":"xmqqlfclaf6b.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3] use delete_refs when deleting tags or branches","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2021-01-22T20:29:27Z","receivedAt":"2021-01-22T20:34:13Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"On Thu, Jan 21, 2021 at 6:17 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Phil Hord <phil.hord@gmail.com> writes:\n>\n> > Oof.  Sorry.  I forgot I have diff.noprefix=true in my local config.\n> > It is a huge timesaver for me when looking at diffs on a console since\n> > I can quickly highlight the filename with a mouse to paste into an\n> > editor.\n> >\n> > Sometimes it bites me, though.  Usually I notice in the diff, but this\n> > one I was sending with format-patch / send-email.\n> >\n> > I guess I'll turn that off in git.git so I don't misfire at you again someday.\n>\n> I think per-repository configuration might be sufficient for this\n> particular case (after all, it is project's preference), I wonder if\n> a more command-specific variant of diff.noprefix so that \"log -p\"\n> and \"format-patch\" can be configured separately would make sense,\n> something like...\n>\n>     [diff]\n>         noprefix = true\n>     [diff \"format-patch\"]\n>         noprefix = false\n>     [diff \"show\"]\n>         noprefix = false\n\nThat seems reasonable.  I was trying to think of something clever like\na setting like \"auto\" that means \"noprefix when output is to a tty\".\nBut I still sometimes send a patch to a coworker that I copied from my\nconsole and I then have to remember to add back in the prefixes.  So\nthere is no perfect solution from Git, I think.  The correct solution\nis to teach my console to skip the prefixes when I double-click the\nfilename; or to add symlinks at `a` and `b` in my project; or\nsomething else.  But these are all more painful than noprefix = true,\nso far.\n\nFwiw - I know this issue has been discussed on the list before; there\nare others who feel this itch.\n\nHaving config diff.<command>.noprefix seems reasonable as a fix for\nformat-patch. At first glance it seems like this could get confused\nwith diff.<driver>.*, but I suppose those settings are all specific to\na driver section, so it would be easy enough to keep them separate.\n"}]}