# [PATCH 1/2] ref-filter: split ref_kind_from_filter 7 messages from 2016-10-19 to 2016-10-20. Participants: Jeff King, Junio C Hamano, Santiago Torres. Thread: https://gitlist.dev/t/44330 ## Jeff King, 2016-10-19 20:35 Subject: [PATCH 1/2] ref-filter: split ref_kind_from_filter Message-ID: <20161019203546.dfqmi2czcxopgj6w@sigill.intra.peff.net> URL: https://gitlist.dev/e/20161019203546.dfqmi2czcxopgj6w%40sigill.intra.peff.net In-Reply-To: <20161019203520.zevkb75at2nrogdm@sigill.intra.peff.net> ``` This function does two things: if we know we are filtering only a certain kind of ref, then we can immediately know that we have that kind. If not, then we compute the kind from the fully-qualified refname. The latter half is useful for other callers; let's split it out. Signed-off-by: Jeff King --- ref-filter.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index cfbcd73..77ec9de 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1329,7 +1329,7 @@ static struct ref_array_item *new_ref_array_item(const char *refname, return ref; } -static int filter_ref_kind(struct ref_filter *filter, const char *refname) +static int ref_kind_from_refname(const char *refname) { unsigned int i; @@ -1342,11 +1342,7 @@ static int filter_ref_kind(struct ref_filter *filter, const char *refname) { "refs/tags/", FILTER_REFS_TAGS} }; - if (filter->kind == FILTER_REFS_BRANCHES || - filter->kind == FILTER_REFS_REMOTES || - filter->kind == FILTER_REFS_TAGS) - return filter->kind; - else if (!strcmp(refname, "HEAD")) + if (!strcmp(refname, "HEAD")) return FILTER_REFS_DETACHED_HEAD; for (i = 0; i < ARRAY_SIZE(ref_kind); i++) { @@ -1357,6 +1353,15 @@ static int filter_ref_kind(struct ref_filter *filter, const char *refname) return FILTER_REFS_OTHERS; } +static int filter_ref_kind(struct ref_filter *filter, const char *refname) +{ + if (filter->kind == FILTER_REFS_BRANCHES || + filter->kind == FILTER_REFS_REMOTES || + filter->kind == FILTER_REFS_TAGS) + return filter->kind; + return ref_kind_from_refname(refname); +} + /* * A call-back given to for_each_ref(). Filter refs and keep them for * later object processing. -- 2.10.1.619.g16351a7 ``` ## Jeff King, 2016-10-19 20:35 Subject: Re: [PATCH v4 2/7] ref-filter: add function to print single ref_array_item Message-ID: <20161019203520.zevkb75at2nrogdm@sigill.intra.peff.net> URL: https://gitlist.dev/e/20161019203520.zevkb75at2nrogdm%40sigill.intra.peff.net In-Reply-To: <20161019170733.ey3d53miykn5t5cq@LykOS.localdomain> ``` On Wed, Oct 19, 2016 at 01:07:34PM -0400, Santiago Torres wrote: > > I guess that may complicate things for the caller you add in this > > series, which may not have a fully-qualified refname (which is obviously > > how filter_ref_kind() figures it out). I'd argue that is a bug, though, > > as things like "%(refname)" are generally expected to print out the > > fully refname ("git tag --format=%(refname)" does so, and you can use > > "%(refname:short)" if you want the shorter part). > > Hmm, I hadn't actually noticed that. Do you have any suggestions in how to > address this? > > In general this feels like a consequence of disambiguating .git/tags/* > within builtin/tag.c rather than letting ref-filter figure it out. The partial solution would look like something below. It's not too bad because git-tag always knows that it's working a ref in the refs/tags namespace (and we don't even have to qualify it ourselves, for_each_tag_name already does it for us). But verify-tag feeds arbitrary sha1 expressions. See the notes in the second patch. [1/2]: ref-filter: split ref_kind_from_filter [2/2]: tag: send fully qualified refnames to verify_tag_and_format builtin/tag.c | 2 +- ref-filter.c | 21 +++++++++++++-------- ref-filter.h | 6 +++++- tag.c | 2 +- 4 files changed, 20 insertions(+), 11 deletions(-) -Peff ``` ## Jeff King, 2016-10-19 20:39 Subject: [PATCH 2/2] tag: send fully qualified refnames to verify_tag_and_format Message-ID: <20161019203943.epjxnfci7vcqg4xv@sigill.intra.peff.net> URL: https://gitlist.dev/e/20161019203943.epjxnfci7vcqg4xv%40sigill.intra.peff.net In-Reply-To: <20161019203520.zevkb75at2nrogdm@sigill.intra.peff.net> ``` The ref-filter code generally expects to see fully qualified refs, so that things like "%(refname)" and "%(refname:short)" work as expected. We can do so easily from git-tag, which always works with refnames in the refs/tags namespace. As a bonus, we can drop the "kind" parameter from pretty_print_ref() and just deduce it automatically. Unfortunately, things are not so simple for verify-tag, which takes an arbitrary sha1 expression. It has no clue if a refname as used or not, and whether it was in the refs/tags namespace. In an ideal world, get_sha1_with_context() would optionally tell us about any refs we resolved while it was working, and we could just feed that refname (and then in cases where we didn't use a ref at all, like a bare sha1, we could fallback to just showing the sha1 name the user gave us). Signed-off-by: Jeff King --- I think you'd really just squash the various bits of this into your series at the right spots, though I don't mind it on top, either. The big question is to what degree we should care about the verify-tag case. I don't think it's any worse off with this change than it is with your series (its "kind" becomes "OTHER", but I don't think that is actually used for display at all; the name remains the same). I'd be OK with leaving it like this, as a known bug, until get_sha1_with_context() learns to tell us about the ref. It's an unhandled corner case in a brand-new feature, not a regression in an existing one. builtin/tag.c | 2 +- ref-filter.c | 4 ++-- ref-filter.h | 6 +++++- tag.c | 2 +- 4 files changed, 9 insertions(+), 5 deletions(-) diff --git a/builtin/tag.c b/builtin/tag.c index 49aeb50..18eab7e 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -114,7 +114,7 @@ static int verify_tag(const char *name, const char *ref, if (fmt_pretty) flags = GPG_VERIFY_QUIET; - return verify_and_format_tag(sha1, name, fmt_pretty, flags); + return verify_and_format_tag(sha1, ref, fmt_pretty, flags); } static int do_sign(struct strbuf *buffer) diff --git a/ref-filter.c b/ref-filter.c index 77ec9de..74da17a 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1643,11 +1643,11 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu } void pretty_print_ref(const char *name, const unsigned char *sha1, - const char *format, unsigned kind) + const char *format) { struct ref_array_item *ref_item; ref_item = new_ref_array_item(name, sha1, 0); - ref_item->kind = kind; + ref_item->kind = ref_kind_from_refname(name); show_ref_array_item(ref_item, format, 0); free_array_item(ref_item); } diff --git a/ref-filter.h b/ref-filter.h index 3d23090..fed2f5e 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -107,7 +107,11 @@ struct ref_sorting *ref_default_sorting(void); /* Function to parse --merged and --no-merged options */ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset); +/* + * Print a single ref, outside of any ref-filter. Note that the + * name must be a fully qualified refname. + */ void pretty_print_ref(const char *name, const unsigned char *sha1, - const char *format, unsigned kind); + const char *format); #endif /* REF_FILTER_H */ diff --git a/tag.c b/tag.c index d3512c0..d5a7cfb 100644 --- a/tag.c +++ b/tag.c @@ -62,7 +62,7 @@ int verify_and_format_tag(const unsigned char *sha1, const char *name, free(buf); if (fmt_pretty) - pretty_print_ref(name, sha1, fmt_pretty, FILTER_REFS_TAGS); + pretty_print_ref(name, sha1, fmt_pretty); return ret; } -- 2.10.1.619.g16351a7 ``` ## Junio C Hamano, 2016-10-19 21:23 Subject: Re: [PATCH 1/2] ref-filter: split ref_kind_from_filter Message-ID: URL: https://gitlist.dev/e/xmqqd1iwkp7m.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20161019203546.dfqmi2czcxopgj6w@sigill.intra.peff.net> ``` Jeff King writes: > Subject: Re: [PATCH 1/2] ref-filter: split ref_kind_from_filter I think you meant ref_kind_from_refname() ;-) Looks like a good idea. > This function does two things: if we know we are filtering > only a certain kind of ref, then we can immediately know > that we have that kind. If not, then we compute the kind > from the fully-qualified refname. The latter half is useful > for other callers; let's split it out. > > Signed-off-by: Jeff King > --- > ref-filter.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/ref-filter.c b/ref-filter.c > index cfbcd73..77ec9de 100644 > --- a/ref-filter.c > +++ b/ref-filter.c > @@ -1329,7 +1329,7 @@ static struct ref_array_item *new_ref_array_item(const char *refname, > return ref; > } > > -static int filter_ref_kind(struct ref_filter *filter, const char *refname) > +static int ref_kind_from_refname(const char *refname) > { > unsigned int i; > > @@ -1342,11 +1342,7 @@ static int filter_ref_kind(struct ref_filter *filter, const char *refname) > { "refs/tags/", FILTER_REFS_TAGS} > }; > > - if (filter->kind == FILTER_REFS_BRANCHES || > - filter->kind == FILTER_REFS_REMOTES || > - filter->kind == FILTER_REFS_TAGS) > - return filter->kind; > - else if (!strcmp(refname, "HEAD")) > + if (!strcmp(refname, "HEAD")) > return FILTER_REFS_DETACHED_HEAD; > > for (i = 0; i < ARRAY_SIZE(ref_kind); i++) { > @@ -1357,6 +1353,15 @@ static int filter_ref_kind(struct ref_filter *filter, const char *refname) > return FILTER_REFS_OTHERS; > } > > +static int filter_ref_kind(struct ref_filter *filter, const char *refname) > +{ > + if (filter->kind == FILTER_REFS_BRANCHES || > + filter->kind == FILTER_REFS_REMOTES || > + filter->kind == FILTER_REFS_TAGS) > + return filter->kind; > + return ref_kind_from_refname(refname); > +} > + > /* > * A call-back given to for_each_ref(). Filter refs and keep them for > * later object processing. ``` ## Jeff King, 2016-10-19 21:33 Subject: Re: [PATCH 1/2] ref-filter: split ref_kind_from_filter Message-ID: <20161019213311.7gmlrqdnjoibq3de@sigill.intra.peff.net> URL: https://gitlist.dev/e/20161019213311.7gmlrqdnjoibq3de%40sigill.intra.peff.net In-Reply-To: ``` On Wed, Oct 19, 2016 at 02:23:41PM -0700, Junio C Hamano wrote: > Jeff King writes: > > > Subject: Re: [PATCH 1/2] ref-filter: split ref_kind_from_filter > > I think you meant ref_kind_from_refname() ;-) > > Looks like a good idea. Heh, I actually meant filter_ref_kind(), which is the original function. But any name that is actually a real function would do. :) -Peff ``` ## Santiago Torres, 2016-10-20 16:57 Subject: Re: [PATCH 2/2] tag: send fully qualified refnames to verify_tag_and_format Message-ID: <20161020165700.qwgli5mbya3d7nzz@LykOS.localdomain> URL: https://gitlist.dev/e/20161020165700.qwgli5mbya3d7nzz%40LykOS.localdomain In-Reply-To: <20161019203943.epjxnfci7vcqg4xv@sigill.intra.peff.net> ``` On Wed, Oct 19, 2016 at 04:39:44PM -0400, Jeff King wrote: > The ref-filter code generally expects to see fully qualified > refs, so that things like "%(refname)" and "%(refname:short)" > work as expected. We can do so easily from git-tag, which > always works with refnames in the refs/tags namespace. As a > bonus, we can drop the "kind" parameter from > pretty_print_ref() and just deduce it automatically. > > Unfortunately, things are not so simple for verify-tag, > which takes an arbitrary sha1 expression. It has no clue if > a refname as used or not, and whether it was in the > refs/tags namespace. > > In an ideal world, get_sha1_with_context() would optionally > tell us about any refs we resolved while it was working, and > we could just feed that refname (and then in cases where we > didn't use a ref at all, like a bare sha1, we could fallback > to just showing the sha1 name the user gave us). > > Signed-off-by: Jeff King > --- > I think you'd really just squash the various bits of this into your > series at the right spots, though I don't mind it on top, either. > > The big question is to what degree we should care about the verify-tag > case. I don't think it's any worse off with this change than it is with > your series (its "kind" becomes "OTHER", but I don't think that is > actually used for display at all; the name remains the same). I'd be OK > with leaving it like this, as a known bug, until get_sha1_with_context() > learns to tell us about the ref. It's an unhandled corner case in a > brand-new feature, not a regression in an existing one. I see now, I think I can sprinkle some of these changes on 2/7 then. The rest should be doing 4/7 and 5/7. Does this sound ok? ``` ## Jeff King, 2016-10-20 21:39 Subject: Re: [PATCH 2/2] tag: send fully qualified refnames to verify_tag_and_format Message-ID: <20161020213904.vfvqqvtow4hycdmx@sigill.intra.peff.net> URL: https://gitlist.dev/e/20161020213904.vfvqqvtow4hycdmx%40sigill.intra.peff.net In-Reply-To: <20161020165700.qwgli5mbya3d7nzz@LykOS.localdomain> ``` On Thu, Oct 20, 2016 at 12:57:01PM -0400, Santiago Torres wrote: > > I think you'd really just squash the various bits of this into your > > series at the right spots, though I don't mind it on top, either. > > > > The big question is to what degree we should care about the verify-tag > > case. I don't think it's any worse off with this change than it is with > > your series (its "kind" becomes "OTHER", but I don't think that is > > actually used for display at all; the name remains the same). I'd be OK > > with leaving it like this, as a known bug, until get_sha1_with_context() > > learns to tell us about the ref. It's an unhandled corner case in a > > brand-new feature, not a regression in an existing one. > > I see now, I think I can sprinkle some of these changes on 2/7 then. The > rest should be doing 4/7 and 5/7. Does this sound ok? Yep, that sounds about right. -Peff ```