{"thread":{"id":"44330","subject":"[PATCH 1/2] ref-filter: split ref_kind_from_filter","startedAt":"2016-10-19T20:36:02Z","lastAt":"2016-10-20T21:39:10Z","messageCount":7,"participants":["Jeff King","Junio C Hamano","Santiago Torres"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"304414","messageId":"20161019203546.dfqmi2czcxopgj6w@sigill.intra.peff.net","threadId":"44330","inReplyTo":"20161019203520.zevkb75at2nrogdm@sigill.intra.peff.net","subject":"[PATCH 1/2] ref-filter: split ref_kind_from_filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-19T20:35:46Z","receivedAt":"2016-10-19T20:36:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This function does two things: if we know we are filtering\nonly a certain kind of ref, then we can immediately know\nthat we have that kind. If not, then we compute the kind\nfrom the fully-qualified refname. The latter half is useful\nfor other callers; let's split it out.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n ref-filter.c | 17 +++++++++++------\n 1 file changed, 11 insertions(+), 6 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex cfbcd73..77ec9de 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1329,7 +1329,7 @@ static struct ref_array_item *new_ref_array_item(const char *refname,\n \treturn ref;\n }\n \n-static int filter_ref_kind(struct ref_filter *filter, const char *refname)\n+static int ref_kind_from_refname(const char *refname)\n {\n \tunsigned int i;\n \n@@ -1342,11 +1342,7 @@ static int filter_ref_kind(struct ref_filter *filter, const char *refname)\n \t\t{ \"refs/tags/\", FILTER_REFS_TAGS}\n \t};\n \n-\tif (filter->kind == FILTER_REFS_BRANCHES ||\n-\t    filter->kind == FILTER_REFS_REMOTES ||\n-\t    filter->kind == FILTER_REFS_TAGS)\n-\t\treturn filter->kind;\n-\telse if (!strcmp(refname, \"HEAD\"))\n+\tif (!strcmp(refname, \"HEAD\"))\n \t\treturn FILTER_REFS_DETACHED_HEAD;\n \n \tfor (i = 0; i < ARRAY_SIZE(ref_kind); i++) {\n@@ -1357,6 +1353,15 @@ static int filter_ref_kind(struct ref_filter *filter, const char *refname)\n \treturn FILTER_REFS_OTHERS;\n }\n \n+static int filter_ref_kind(struct ref_filter *filter, const char *refname)\n+{\n+\tif (filter->kind == FILTER_REFS_BRANCHES ||\n+\t    filter->kind == FILTER_REFS_REMOTES ||\n+\t    filter->kind == FILTER_REFS_TAGS)\n+\t\treturn filter->kind;\n+\treturn ref_kind_from_refname(refname);\n+}\n+\n /*\n  * A call-back given to for_each_ref().  Filter refs and keep them for\n  * later object processing.\n-- \n2.10.1.619.g16351a7\n\n"},{"id":"304415","messageId":"20161019203520.zevkb75at2nrogdm@sigill.intra.peff.net","threadId":"44330","inReplyTo":"20161019170733.ey3d53miykn5t5cq@LykOS.localdomain","subject":"Re: [PATCH v4 2/7] ref-filter: add function to print single ref_array_item","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-19T20:35:20Z","receivedAt":"2016-10-19T20:36:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 19, 2016 at 01:07:34PM -0400, Santiago Torres wrote:\n\n> > I guess that may complicate things for the caller you add in this\n> > series, which may not have a fully-qualified refname (which is obviously\n> > how filter_ref_kind() figures it out). I'd argue that is a bug, though,\n> > as things like \"%(refname)\" are generally expected to print out the\n> > fully refname (\"git tag --format=%(refname)\" does so, and you can use\n> > \"%(refname:short)\" if you want the shorter part).\n> \n> Hmm, I hadn't actually noticed that. Do you have any suggestions in how to\n> address this?\n> \n> In general this feels like a consequence of disambiguating .git/tags/*\n> within builtin/tag.c rather than letting ref-filter figure it out.\n\nThe partial solution would look like something below. It's not too bad\nbecause git-tag always knows that it's working a ref in the refs/tags\nnamespace (and we don't even have to qualify it ourselves,\nfor_each_tag_name already does it for us).\n\nBut verify-tag feeds arbitrary sha1 expressions. See the notes in the\nsecond patch.\n\n  [1/2]: ref-filter: split ref_kind_from_filter\n  [2/2]: tag: send fully qualified refnames to verify_tag_and_format\n\n builtin/tag.c |  2 +-\n ref-filter.c  | 21 +++++++++++++--------\n ref-filter.h  |  6 +++++-\n tag.c         |  2 +-\n 4 files changed, 20 insertions(+), 11 deletions(-)\n\n-Peff\n"},{"id":"304416","messageId":"20161019203943.epjxnfci7vcqg4xv@sigill.intra.peff.net","threadId":"44330","inReplyTo":"20161019203520.zevkb75at2nrogdm@sigill.intra.peff.net","subject":"[PATCH 2/2] tag: send fully qualified refnames to verify_tag_and_format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-19T20:39:44Z","receivedAt":"2016-10-19T20:39:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The ref-filter code generally expects to see fully qualified\nrefs, so that things like \"%(refname)\" and \"%(refname:short)\"\nwork as expected. We can do so easily from git-tag, which\nalways works with refnames in the refs/tags namespace. As a\nbonus, we can drop the \"kind\" parameter from\npretty_print_ref() and just deduce it automatically.\n\nUnfortunately, things are not so simple for verify-tag,\nwhich takes an arbitrary sha1 expression. It has no clue if\na refname as used or not, and whether it was in the\nrefs/tags namespace.\n\nIn an ideal world, get_sha1_with_context() would optionally\ntell us about any refs we resolved while it was working, and\nwe could just feed that refname (and then in cases where we\ndidn't use a ref at all, like a bare sha1, we could fallback\nto just showing the sha1 name the user gave us).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI think you'd really just squash the various bits of this into your\nseries at the right spots, though I don't mind it on top, either.\n\nThe big question is to what degree we should care about the verify-tag\ncase. I don't think it's any worse off with this change than it is with\nyour series (its \"kind\" becomes \"OTHER\", but I don't think that is\nactually used for display at all; the name remains the same). I'd be OK\nwith leaving it like this, as a known bug, until get_sha1_with_context()\nlearns to tell us about the ref. It's an unhandled corner case in a\nbrand-new feature, not a regression in an existing one.\n\n builtin/tag.c | 2 +-\n ref-filter.c  | 4 ++--\n ref-filter.h  | 6 +++++-\n tag.c         | 2 +-\n 4 files changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 49aeb50..18eab7e 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -114,7 +114,7 @@ static int verify_tag(const char *name, const char *ref,\n \tif (fmt_pretty)\n \t\tflags = GPG_VERIFY_QUIET;\n \n-\treturn verify_and_format_tag(sha1, name, fmt_pretty, flags);\n+\treturn verify_and_format_tag(sha1, ref, fmt_pretty, flags);\n }\n \n static int do_sign(struct strbuf *buffer)\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 77ec9de..74da17a 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1643,11 +1643,11 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n }\n \n void pretty_print_ref(const char *name, const unsigned char *sha1,\n-\t\tconst char *format, unsigned kind)\n+\t\t      const char *format)\n {\n \tstruct ref_array_item *ref_item;\n \tref_item = new_ref_array_item(name, sha1, 0);\n-\tref_item->kind = kind;\n+\tref_item->kind = ref_kind_from_refname(name);\n \tshow_ref_array_item(ref_item, format, 0);\n \tfree_array_item(ref_item);\n }\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 3d23090..fed2f5e 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -107,7 +107,11 @@ struct ref_sorting *ref_default_sorting(void);\n /*  Function to parse --merged and --no-merged options */\n int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset);\n \n+/*\n+ * Print a single ref, outside of any ref-filter. Note that the\n+ * name must be a fully qualified refname.\n+ */\n void pretty_print_ref(const char *name, const unsigned char *sha1,\n-\t\tconst char *format, unsigned kind);\n+\t\t      const char *format);\n \n #endif /*  REF_FILTER_H  */\ndiff --git a/tag.c b/tag.c\nindex d3512c0..d5a7cfb 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -62,7 +62,7 @@ int verify_and_format_tag(const unsigned char *sha1, const char *name,\n \tfree(buf);\n \n \tif (fmt_pretty)\n-\t\tpretty_print_ref(name, sha1, fmt_pretty, FILTER_REFS_TAGS);\n+\t\tpretty_print_ref(name, sha1, fmt_pretty);\n \n \treturn ret;\n }\n-- \n2.10.1.619.g16351a7\n"},{"id":"304425","messageId":"xmqqd1iwkp7m.fsf@gitster.mtv.corp.google.com","threadId":"44330","inReplyTo":"20161019203546.dfqmi2czcxopgj6w@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] ref-filter: split ref_kind_from_filter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-19T21:23:41Z","receivedAt":"2016-10-19T21:23:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Subject: Re: [PATCH 1/2] ref-filter: split ref_kind_from_filter\n\nI think you meant ref_kind_from_refname() ;-)\n\nLooks like a good idea.\n\n> This function does two things: if we know we are filtering\n> only a certain kind of ref, then we can immediately know\n> that we have that kind. If not, then we compute the kind\n> from the fully-qualified refname. The latter half is useful\n> for other callers; let's split it out.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  ref-filter.c | 17 +++++++++++------\n>  1 file changed, 11 insertions(+), 6 deletions(-)\n>\n> diff --git a/ref-filter.c b/ref-filter.c\n> index cfbcd73..77ec9de 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -1329,7 +1329,7 @@ static struct ref_array_item *new_ref_array_item(const char *refname,\n>  \treturn ref;\n>  }\n>  \n> -static int filter_ref_kind(struct ref_filter *filter, const char *refname)\n> +static int ref_kind_from_refname(const char *refname)\n>  {\n>  \tunsigned int i;\n>  \n> @@ -1342,11 +1342,7 @@ static int filter_ref_kind(struct ref_filter *filter, const char *refname)\n>  \t\t{ \"refs/tags/\", FILTER_REFS_TAGS}\n>  \t};\n>  \n> -\tif (filter->kind == FILTER_REFS_BRANCHES ||\n> -\t    filter->kind == FILTER_REFS_REMOTES ||\n> -\t    filter->kind == FILTER_REFS_TAGS)\n> -\t\treturn filter->kind;\n> -\telse if (!strcmp(refname, \"HEAD\"))\n> +\tif (!strcmp(refname, \"HEAD\"))\n>  \t\treturn FILTER_REFS_DETACHED_HEAD;\n>  \n>  \tfor (i = 0; i < ARRAY_SIZE(ref_kind); i++) {\n> @@ -1357,6 +1353,15 @@ static int filter_ref_kind(struct ref_filter *filter, const char *refname)\n>  \treturn FILTER_REFS_OTHERS;\n>  }\n>  \n> +static int filter_ref_kind(struct ref_filter *filter, const char *refname)\n> +{\n> +\tif (filter->kind == FILTER_REFS_BRANCHES ||\n> +\t    filter->kind == FILTER_REFS_REMOTES ||\n> +\t    filter->kind == FILTER_REFS_TAGS)\n> +\t\treturn filter->kind;\n> +\treturn ref_kind_from_refname(refname);\n> +}\n> +\n>  /*\n>   * A call-back given to for_each_ref().  Filter refs and keep them for\n>   * later object processing.\n"},{"id":"304426","messageId":"20161019213311.7gmlrqdnjoibq3de@sigill.intra.peff.net","threadId":"44330","inReplyTo":"xmqqd1iwkp7m.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] ref-filter: split ref_kind_from_filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-19T21:33:12Z","receivedAt":"2016-10-19T21:33:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 19, 2016 at 02:23:41PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Subject: Re: [PATCH 1/2] ref-filter: split ref_kind_from_filter\n> \n> I think you meant ref_kind_from_refname() ;-)\n> \n> Looks like a good idea.\n\nHeh, I actually meant filter_ref_kind(), which is the original function.\nBut any name that is actually a real function would do. :)\n\n-Peff\n"},{"id":"304480","messageId":"20161020165700.qwgli5mbya3d7nzz@LykOS.localdomain","threadId":"44330","inReplyTo":"20161019203943.epjxnfci7vcqg4xv@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] tag: send fully qualified refnames to verify_tag_and_format","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2016-10-20T16:57:01Z","receivedAt":"2016-10-20T16:57:07Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"On Wed, Oct 19, 2016 at 04:39:44PM -0400, Jeff King wrote:\n> The ref-filter code generally expects to see fully qualified\n> refs, so that things like \"%(refname)\" and \"%(refname:short)\"\n> work as expected. We can do so easily from git-tag, which\n> always works with refnames in the refs/tags namespace. As a\n> bonus, we can drop the \"kind\" parameter from\n> pretty_print_ref() and just deduce it automatically.\n> \n> Unfortunately, things are not so simple for verify-tag,\n> which takes an arbitrary sha1 expression. It has no clue if\n> a refname as used or not, and whether it was in the\n> refs/tags namespace.\n> \n> In an ideal world, get_sha1_with_context() would optionally\n> tell us about any refs we resolved while it was working, and\n> we could just feed that refname (and then in cases where we\n> didn't use a ref at all, like a bare sha1, we could fallback\n> to just showing the sha1 name the user gave us).\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I think you'd really just squash the various bits of this into your\n> series at the right spots, though I don't mind it on top, either.\n> \n> The big question is to what degree we should care about the verify-tag\n> case. I don't think it's any worse off with this change than it is with\n> your series (its \"kind\" becomes \"OTHER\", but I don't think that is\n> actually used for display at all; the name remains the same). I'd be OK\n> with leaving it like this, as a known bug, until get_sha1_with_context()\n> learns to tell us about the ref. It's an unhandled corner case in a\n> brand-new feature, not a regression in an existing one.\n\nI see now, I think I can sprinkle some of these changes on 2/7 then. The\nrest should be doing 4/7 and 5/7. Does this sound ok?\n"},{"id":"304521","messageId":"20161020213904.vfvqqvtow4hycdmx@sigill.intra.peff.net","threadId":"44330","inReplyTo":"20161020165700.qwgli5mbya3d7nzz@LykOS.localdomain","subject":"Re: [PATCH 2/2] tag: send fully qualified refnames to verify_tag_and_format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-20T21:39:04Z","receivedAt":"2016-10-20T21:39:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 20, 2016 at 12:57:01PM -0400, Santiago Torres wrote:\n\n> > I think you'd really just squash the various bits of this into your\n> > series at the right spots, though I don't mind it on top, either.\n> > \n> > The big question is to what degree we should care about the verify-tag\n> > case. I don't think it's any worse off with this change than it is with\n> > your series (its \"kind\" becomes \"OTHER\", but I don't think that is\n> > actually used for display at all; the name remains the same). I'd be OK\n> > with leaving it like this, as a known bug, until get_sha1_with_context()\n> > learns to tell us about the ref. It's an unhandled corner case in a\n> > brand-new feature, not a regression in an existing one.\n> \n> I see now, I think I can sprinkle some of these changes on 2/7 then. The\n> rest should be doing 4/7 and 5/7. Does this sound ok?\n\nYep, that sounds about right.\n\n-Peff\n"}]}