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

[Elijah Newren <newren@gmail.com>]

Hi,

On Thu, Jan 14, 2021 at 6:00 PM Phil Hord <phil.hord@gmail.com> wrote:
>
> I noticed this is still only in my local branch.   Can I get an ACK/NAK?

Sorry for missing this when you posted in August.  Thanks for sending
in the update from v1.

For other reviewers: v1 is over here:
https://lore.kernel.org/git/20190808035935.30023-1-phil.hord@gmail.com/,
and has review comments from Martin, me, Peff, and Junio.

> On Tue, Aug 13, 2019 at 7:49 PM Phil Hord <phil.hord@gmail.com> wrote:
>>
>> From: Phil Hord <phil.hord@gmail.com>
>>
>> 'git tag -d' and 'git branch -d' both accept one or more refs to
>> delete, but each deletion is done by calling `delete_ref` on each argv.
>> This is very slow when removing from packed refs as packed-refs is
>> locked and rewritten each time. Use delete_refs instead so all the
>> removals can be done inside a single transaction with a single update.

Awesome, thanks for also fixing up git branch with v2.

>> 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 refs, report the deletion's success only if the ref was
>> actually deleted. For branch deletion, remove the branch config only
>> if the branch ref is actually removed.
>>
>> A manual test deleting 24,000 tags took about 30 minutes using
>> delete_ref.  It takes about 5 seconds using delete_refs.

As I said on v1, it's really nice to have this fixed.  Thanks for doing it.

>>
>> Signed-off-by: Phil Hord <phil.hord@gmail.com>
>> ---
>> This reroll adds the same delete_refs change to 'git branch'. It checks
>> individual refs after the operation to report correctly on each whether
>> it was successfully deleted or not. Maybe this is an unnecessary step,
>> though. This handles the weird case where some file system error
>> prevented us from deleting refs, leaving us with an error from
>> delete_refs but without any idea which refs might have been affected.
>>
>>  builtin/branch.c | 50 +++++++++++++++++++++++++++++-------------------
>>  builtin/tag.c    | 45 +++++++++++++++++++++++++++++++++----------
>>  2 files changed, 65 insertions(+), 30 deletions(-)
>>
>> diff --git a/builtin/branch.c b/builtin/branch.c
>> index 2ef214632f..2273239f41 100644
>> --- a/builtin/branch.c
>> +++ b/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 refname_pos = 0;
>>
>>         switch (kinds) {
>>         case FILTER_REFS_REMOTES:
>> @@ -209,12 +212,13 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>>                 /* For subsequent UI messages */
>>                 remote_branch = 1;
>>                 allowed_interpret = INTERPRET_BRANCH_REMOTE;
>> -
>> +               refname_pos = 13;
>>                 force = 1;
>>                 break;
>>         case FILTER_REFS_BRANCHES:
>>                 fmt = "refs/heads/%s";
>>                 allowed_interpret = INTERPRET_BRANCH_LOCAL;
>> +               refname_pos = 11;
>>                 break;
>>         default:
>>                 die(_("cannot use -a with -d"));
>> @@ -265,30 +269,36 @@ 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;

The code used to set the return code to 1 if it failed to delete a branch

>> -               }
>> -               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);
>>         }
>>
>> +       delete_refs(NULL, &refs_to_delete, REF_NO_DEREF);
>> +
>> +       for_each_string_list_item(item, &refs_to_delete) {
>> +               char * describe_ref = item->util;
>> +               char * name = item->string;
>> +               if (ref_exists(name))
>> +                       ret = 1;

Now it sets the return code if the branch still exists after trying to
delete.  I thought that was subtly different...but I tried doing a
branch deletion of a non-existent branch since I thought that would be
the only difference -- however, that errors out earlier in the
codepath before even getting to the stage of deleting refs.  So I
think these are effectively the same.

>> +               else {
>> +                       char * refname = name + refname_pos;
>> +                       if (!quiet)
>> +                               printf(remote_branch
>> +                                       ? _("Deleted remote-tracking branch %s (was %s).\n")
>> +                                       : _("Deleted branch %s (was %s).\n"),
>> +                                       name + refname_pos, describe_ref);

Neither remote_branch nor refname_pos are changing throughout this
loop, which I at first thought was in error, but it looks like git
branch only allows you to delete one type or the other -- not a
mixture.  So this is correct.

>> +
>> +                       delete_branch_config(refname);
>> +               }
>> +               free(describe_ref);
>> +       }
>> +       string_list_clear(&refs_to_delete, 0);
>> +
>>         free(name);
>>         strbuf_release(&bname);
>>
>> diff --git a/builtin/tag.c b/builtin/tag.c
>> index e0a4c25382..0d11ffcd04 100644
>> --- a/builtin/tag.c
>> +++ b/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,43 @@ 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;

This used to return 1 if it failed to delete a ref.

>> -       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;

Now it unconditionally returns 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);
>> +       delete_refs(NULL, &refs_to_delete, REF_NO_DEREF);

You now only look at the result of collecting the tags, and ignore the
result of trying to delete them...

>> +
>> +       for_each_string_list_item(item, &refs_to_delete) {
>> +               const char * name = item->string;
>> +               struct object_id * oid = item->util;
>> +               if (ref_exists(name))
>> +                       result = 1;

...except that you check if the refs still exist afterward and set the
return code based on it.  Like with the branch case, I can't come up
with a case where the difference matters.  I suspect there's a race
condition there somewhere, but once you start going down that road I
think the old code may have had a bunch of races too.  It might be
nice to document with a comment that there's a small race condition
with someone else trying to forcibly re-create the ref at the same
time you are trying to delete, but I don't think it's a big deal.

If you did use the result of delete_refs(), you might have to double
check that the callers (git.c:handle_builtin() -> git.c:run_builtin()
-> builtin/tag.c:cmd_tag() -> builtin/tag.c:delete_tags()) are all
okay with the return code; it looks like handle_builtin() would pass
the return code to exit() and the git-tag manpage doesn't document the
return status, so you've at least got some leeway in terms of what
values are acceptable.  Or you could just normalize the return value
of delete_refs() down to 0 or 1.  But you'd only need to worry about
that if the race condition is something we're worried enough to
tackle.

>> +               else
>> +                       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;
>> @@ -511,7 +536,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>>         if (filter.merge_commit)
>>                 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.23.0.rc1.174.g4cc1b04b4c

Overall, I like the patch.  Peff commented on v1 that the basic idea
(use the part of the refs API that batches operations) is the right
thing to do.  I'm not that familiar with refs-touching code, but your
patch makes sense to me.  I think I spotted a minor issue (you ignore
the return status of delete_refs(), then later check the existence of
the refs afterwards to determine success, which I believe is a minor
and unlikely race condition), but I'm not sure it's worth fixing;
perhaps just mark it with #leftoverbits and move on -- the faster
branch and tag deletion is a very nice improvement.

I notice Martin said on v1 that there was a testcase that had problems
with your patch; I tested v2 and it looks like you fixed any such
issues.  I think you also addressed the feedback from Junio, though
his comments about the return code and the minor race condition I
noticed around it might mean it'd be good to get his comments.

Anyway,
Acked-by: Elijah Newren <newren@gmail.com>

I would say Reviewed-by, but I'd like to get Junio's comments on the
return code and minor race.