[PATCH v3] use delete_refs when deleting tags or branches

[PATCH v3] use delete_refs when deleting tags or branches

[Phil Hord]

From: Phil Hord <phil.hord@gmail.com>

'git tag -d' accepts one or more tag refs to delete, but each deletion
is done by calling `delete_ref` on each argv. This is very slow when
removing from packed refs. Use delete_refs instead so all the removals
can be done inside a single transaction with a single update.

Do the same for 'git branch -d'.

Since delete_refs performs all the packed-refs delete operations
inside a single transaction, if any of the deletes fail then all
them will be skipped. In practice, none of them should fail since
we verify the hash of each one before calling delete_refs, but some
network error or odd permissions problem could have different results
after this change.

Also, since the file-backed deletions are not performed in the same
transaction, those could succeed even when the packed-refs transaction
fails.

After deleting branches, remove the branch config only if the branch
ref was removed and was not subsequently added back in.

A manual test deleting 24,000 tags took about 30 minutes using
delete_ref.  It takes about 5 seconds using delete_refs.

Acked-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Phil Hord <phil.hord@gmail.com>
---

This version translates a nonzero return code from delete_refs into an 
error return value of 1. It also has style cleanups suggested from v2.

 builtin/branch.c | 47 ++++++++++++++++++++++++++++-------------------
 builtin/tag.c    | 44 ++++++++++++++++++++++++++++++++++----------
 2 files changed, 62 insertions(+), 29 deletions(-)

diff --git builtin/branch.c builtin/branch.c
index 8c0b428104..bcc00bcf18 100644
--- builtin/branch.c
+++ builtin/branch.c
@@ -202,6 +202,9 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 	int remote_branch = 0;
 	struct strbuf bname = STRBUF_INIT;
 	unsigned allowed_interpret;
+	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
+	struct string_list_item *item;
+	int branch_name_pos;
 
 	switch (kinds) {
 	case FILTER_REFS_REMOTES:
@@ -219,6 +222,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 	default:
 		die(_("cannot use -a with -d"));
 	}
+	branch_name_pos = strcspn(fmt, "%");
 
 	if (!force) {
 		head_rev = lookup_commit_reference(the_repository, &head_oid);
@@ -265,30 +269,35 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 			goto next;
 		}
 
-		if (delete_ref(NULL, name, is_null_oid(&oid) ? NULL : &oid,
-			       REF_NO_DEREF)) {
-			error(remote_branch
-			      ? _("Error deleting remote-tracking branch '%s'")
-			      : _("Error deleting branch '%s'"),
-			      bname.buf);
-			ret = 1;
-			goto next;
-		}
-		if (!quiet) {
-			printf(remote_branch
-			       ? _("Deleted remote-tracking branch %s (was %s).\n")
-			       : _("Deleted branch %s (was %s).\n"),
-			       bname.buf,
-			       (flags & REF_ISBROKEN) ? "broken"
-			       : (flags & REF_ISSYMREF) ? target
-			       : find_unique_abbrev(&oid, DEFAULT_ABBREV));
-		}
-		delete_branch_config(bname.buf);
+		item = string_list_append(&refs_to_delete, name);
+		item->util = xstrdup((flags & REF_ISBROKEN) ? "broken"
+				    : (flags & REF_ISSYMREF) ? target
+				    : find_unique_abbrev(&oid, DEFAULT_ABBREV));
 
 	next:
 		free(target);
 	}
 
+	if (delete_refs(NULL, &refs_to_delete, REF_NO_DEREF))
+		ret = 1;
+
+	for_each_string_list_item(item, &refs_to_delete) {
+		char *describe_ref = item->util;
+		char *name = item->string;
+		if (!ref_exists(name)) {
+			char *refname = name + branch_name_pos;
+			if (!quiet)
+				printf(remote_branch
+					? _("Deleted remote-tracking branch %s (was %s).\n")
+					: _("Deleted branch %s (was %s).\n"),
+					name + branch_name_pos, describe_ref);
+
+			delete_branch_config(refname);
+		}
+		free(describe_ref);
+	}
+	string_list_clear(&refs_to_delete, 0);
+
 	free(name);
 	strbuf_release(&bname);
 
diff --git builtin/tag.c builtin/tag.c
index 24d35b746d..e8b85eefd8 100644
--- builtin/tag.c
+++ builtin/tag.c
@@ -72,10 +72,10 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,
 }
 
 typedef int (*each_tag_name_fn)(const char *name, const char *ref,
-				const struct object_id *oid, const void *cb_data);
+				const struct object_id *oid, void *cb_data);
 
 static int for_each_tag_name(const char **argv, each_tag_name_fn fn,
-			     const void *cb_data)
+			     void *cb_data)
 {
 	const char **p;
 	struct strbuf ref = STRBUF_INIT;
@@ -97,18 +97,42 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn,
 	return had_error;
 }
 
-static int delete_tag(const char *name, const char *ref,
-		      const struct object_id *oid, const void *cb_data)
+static int collect_tags(const char *name, const char *ref,
+			const struct object_id *oid, void *cb_data)
 {
-	if (delete_ref(NULL, ref, oid, 0))
-		return 1;
-	printf(_("Deleted tag '%s' (was %s)\n"), name,
-	       find_unique_abbrev(oid, DEFAULT_ABBREV));
+	struct string_list *ref_list = cb_data;
+
+	string_list_append(ref_list, ref);
+	ref_list->items[ref_list->nr - 1].util = oiddup(oid);
 	return 0;
 }
 
+static int delete_tags(const char **argv)
+{
+	int result;
+	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
+	struct string_list_item *item;
+
+	result = for_each_tag_name(argv, collect_tags, (void *)&refs_to_delete);
+	if (delete_refs(NULL, &refs_to_delete, REF_NO_DEREF))
+		result = 1;
+
+	for_each_string_list_item(item, &refs_to_delete) {
+		const char *name = item->string;
+		struct object_id *oid = item->util;
+		if (!ref_exists(name))
+			printf(_("Deleted tag '%s' (was %s)\n"),
+				item->string + 10,
+				find_unique_abbrev(oid, DEFAULT_ABBREV));
+
+		free(oid);
+	}
+	string_list_clear(&refs_to_delete, 0);
+	return result;
+}
+
 static int verify_tag(const char *name, const char *ref,
-		      const struct object_id *oid, const void *cb_data)
+		      const struct object_id *oid, void *cb_data)
 {
 	int flags;
 	const struct ref_format *format = cb_data;
@@ -512,7 +536,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (filter.reachable_from || filter.unreachable_from)
 		die(_("--merged and --no-merged options are only allowed in list mode"));
 	if (cmdmode == 'd')
-		return for_each_tag_name(argv, delete_tag, NULL);
+		return delete_tags(argv);
 	if (cmdmode == 'v') {
 		if (format.format && verify_ref_format(&format))
 			usage_with_options(git_tag_usage, options);
-- 
2.30.0.281.g914876b2ce

Re: [PATCH v3] use delete_refs when deleting tags or branches

[Junio C Hamano]

Phil Hord <phil.hord@gmail.com> writes:

> diff --git builtin/branch.c builtin/branch.c
> index 8c0b428104..bcc00bcf18 100644
> --- builtin/branch.c
> +++ builtin/branch.c

You wasted 15 minutes of my life by choosing to deviate the list
norm of sending what "git apply -p1" (default) would accept.  I am
fairly trusting type and did not suspect anybody do such an evil
thing.  Why?

Re: [PATCH v3] use delete_refs when deleting tags or branches

[Phil Hord]

On Thu, Jan 21, 2021 at 4:05 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Phil Hord <phil.hord@gmail.com> writes:
>
> > diff --git builtin/branch.c builtin/branch.c
> > index 8c0b428104..bcc00bcf18 100644
> > --- builtin/branch.c
> > +++ builtin/branch.c
>
> You wasted 15 minutes of my life by choosing to deviate the list
> norm of sending what "git apply -p1" (default) would accept.  I am
> fairly trusting type and did not suspect anybody do such an evil
> thing.  Why?

Oof.  Sorry.  I forgot I have diff.noprefix=true in my local config.
It is a huge timesaver for me when looking at diffs on a console since
I can quickly highlight the filename with a mouse to paste into an
editor.

Sometimes it bites me, though.  Usually I notice in the diff, but this
one I was sending with format-patch / send-email.

I guess I'll turn that off in git.git so I don't misfire at you again someday.

Phil

Re: [PATCH v3] use delete_refs when deleting tags or branches

[Junio C Hamano]

Phil Hord <phil.hord@gmail.com> writes:

> Oof.  Sorry.  I forgot I have diff.noprefix=true in my local config.
> It is a huge timesaver for me when looking at diffs on a console since
> I can quickly highlight the filename with a mouse to paste into an
> editor.
>
> Sometimes it bites me, though.  Usually I notice in the diff, but this
> one I was sending with format-patch / send-email.
>
> I guess I'll turn that off in git.git so I don't misfire at you again someday.

I think per-repository configuration might be sufficient for this
particular case (after all, it is project's preference), I wonder if
a more command-specific variant of diff.noprefix so that "log -p"
and "format-patch" can be configured separately would make sense,
something like...

    [diff]
	noprefix = true
    [diff "format-patch"]
	noprefix = false
    [diff "show"]
	noprefix = false

Re: [PATCH v3] use delete_refs when deleting tags or branches

[Junio C Hamano]

Phil Hord <phil.hord@gmail.com> writes:

> After deleting branches, remove the branch config only if the branch
> ref was removed and was not subsequently added back in.
>
> A manual test deleting 24,000 tags took about 30 minutes using
> delete_ref.  It takes about 5 seconds using delete_refs.

Nicely done.  Queued and pushed out.

Re: [PATCH v3] use delete_refs when deleting tags or branches

[Phil Hord]

On Thu, Jan 21, 2021 at 6:17 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Phil Hord <phil.hord@gmail.com> writes:
>
> > Oof.  Sorry.  I forgot I have diff.noprefix=true in my local config.
> > It is a huge timesaver for me when looking at diffs on a console since
> > I can quickly highlight the filename with a mouse to paste into an
> > editor.
> >
> > Sometimes it bites me, though.  Usually I notice in the diff, but this
> > one I was sending with format-patch / send-email.
> >
> > I guess I'll turn that off in git.git so I don't misfire at you again someday.
>
> I think per-repository configuration might be sufficient for this
> particular case (after all, it is project's preference), I wonder if
> a more command-specific variant of diff.noprefix so that "log -p"
> and "format-patch" can be configured separately would make sense,
> something like...
>
>     [diff]
>         noprefix = true
>     [diff "format-patch"]
>         noprefix = false
>     [diff "show"]
>         noprefix = false

That seems reasonable.  I was trying to think of something clever like
a setting like "auto" that means "noprefix when output is to a tty".
But I still sometimes send a patch to a coworker that I copied from my
console and I then have to remember to add back in the prefixes.  So
there is no perfect solution from Git, I think.  The correct solution
is to teach my console to skip the prefixes when I double-click the
filename; or to add symlinks at `a` and `b` in my project; or
something else.  But these are all more painful than noprefix = true,
so far.

Fwiw - I know this issue has been discussed on the list before; there
are others who feel this itch.

Having config diff.<command>.noprefix seems reasonable as a fix for
format-patch. At first glance it seems like this could get confused
with diff.<driver>.*, but I suppose those settings are all specific to
a driver section, so it would be easy enough to keep them separate.