threads / patch / 55022

v3use delete_refs when deleting tags or branches

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

## tl;dr

6 messages between Jan 21, 2021 and Jan 22, 2021. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Phil Hord· Jan 21, 2021, 03:23 UTC · lore
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(-)
Show changes to diff +62 −29
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
Junio C Hamano· Jan 22, 2021, 00:04 UTC · re: Phil Hord · lore

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

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?

Phil Hord· Jan 22, 2021, 00:27 UTC · re: Junio C Hamano · lore

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

On Thu, Jan 21, 2021 at 4:05 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 12 quoted lines
>
> 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
Junio C Hamano· Jan 22, 2021, 02:17 UTC · re: Phil Hord · lore

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

Phil Hord <phil.hord@gmail.com> writes:
Show 9 quoted lines
> 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
Phil Hord· Jan 22, 2021, 20:29 UTC · re: Junio C Hamano · lore

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

On Thu, Jan 21, 2021 at 6:17 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 25 quoted lines
>
> 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.

Junio C Hamano· Jan 22, 2021, 02:42 UTC · re: Phil Hord · lore

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

Phil Hord <phil.hord@gmail.com> writes:
Show 5 quoted lines
> 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.

← back to recent threads