Re: [PATCH 1/1] delete multiple tags in a single transaction

From: Martin Ågren <martin.agren@gmail.com>
Date: Aug 8, 2019, 12:47 UTC
Message-ID: <CAN0heSptKHL8mrU9DTXT9T7HDN56a3+DAGczxkEtbGxp9sB8hg@mail.gmail.com>
In-Reply-To: <20190808035935.30023-1-phil.hord@gmail.com>

On Thu, 8 Aug 2019 at 06:09, Phil Hord <phil.hord@gmail.com> wrote:

Show 8 quoted lines

> I have a repo with 24,000 tags, most of which are not useful to any
> developers. Having this many refs slows down many operations that
> would otherwise be very fast. Removing these tags when they've been
> accidentally fetched again takes about 30 minutes using delete_ref.
>
>     git tag -l feature/* | xargs git tag -d
>
> Removing the same tags using delete_refs takes less than 5 seconds.

This looks worthwhile pursuing...

Show 12 quoted lines

> -static int delete_tag(const char *name, const char *ref,
> -                     const struct object_id *oid, const void *cb_data)
> +struct tag_args {
> +       char *oid_abbrev;
> +       char *refname;
> +};
> +
> +static int make_string_list(const char *name, const char *ref,
> +                           const struct object_id *oid, void *cb_data)
>  {
> -       if (delete_ref(NULL, ref, oid, 0))
> -               return 1;

This provides `oid` for verifying that the tag actually points at that particular oid before deleting. As far as I can tell, `oid` is no longer used like that in the post-image. I'm not sure it matters, since we just looked it up, but that might be worth mentioning, perhaps.

Show 20 quoted lines

> -       printf(_("Deleted tag '%s' (was %s)\n"), name,
> -              find_unique_abbrev(oid, DEFAULT_ABBREV));
> +       struct string_list *ref_list = cb_data;
> +       struct tag_args *info = xmalloc(sizeof(struct tag_args));
> +
> +       string_list_append(ref_list, ref);
> +
> +       info->oid_abbrev = xstrdup(find_unique_abbrev(oid, DEFAULT_ABBREV));
> +       info->refname = xstrdup(name);
> +       ref_list->items[ref_list->nr - 1].util = info;
>         return 0;
>  }
>
> +static int delete_tags(const char **argv)
> +{
> +       int result;
> +       struct string_list ref_list = STRING_LIST_INIT_DUP;
> +       struct string_list_item *ref_list_item;
> +
> +       result = for_each_tag_name(argv, make_string_list, (void *) &ref_list);

If any tag is non-existing (or some other error happens here), we don't continue to the actual deleting. That breaks t7004 which has a test for removing an existing and a non-existing tag -- it wants the existing one to be removed and the non-existing one not to interfere.

> +       if (!result)
> +               result = delete_refs(NULL, &ref_list, REF_NO_DEREF);

So this should perhaps be something more like an unconditional

        result |= delete_refs(...);

That makes the test suite happy, but perhaps only short-term ... See below...

Show 5 quoted lines

> +       for_each_string_list_item(ref_list_item, &ref_list) {
> +               struct tag_args * info = ref_list_item->util;
> +               if (!result)
> +                       printf(_("Deleted tag '%s' (was %s)\n"), info->refname,
> +                               info->oid_abbrev);

Change this conditional here, too, methinks. You'd need to separate errors from looking up tags from errors about deleting refs, so having a single "result" is probably not sufficient.

Probably worth inspecting the output of that `git tag -d` a bit in t7004, to make sure we just claim to delete one tag, and have errors.

Your patch reshuffles the error and success messages (for certain usages). I think that's ok, but might be worth mentioning.

I'm not too familiar with the refs API, so take this with a grain of salt...

Show 7 quoted lines

> +               free(info->oid_abbrev);
> +               free(info->refname);
> +               free(info);
> +       }
> +       string_list_clear(&ref_list, 0);
> +       return result;
> +}

Martin