| Message ID | 20210121032332.658991-1-phil.hord@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] use delete_refs when deleting tags or branches | expand |
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?
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
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
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.
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.
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);