On 02/22, Brandon Williams wrote:
> On 02/22, Jeff King wrote:
> > On Tue, Feb 06, 2018 at 05:12:50PM -0800, Brandon Williams wrote:
> >
> > > +ls-refs takes in the following parameters wrapped in packet-lines:
> > > +
> > > + symrefs
> > > + In addition to the object pointed by it, show the underlying ref
> > > + pointed by it when showing a symbolic ref.
> > > + peel
> > > + Show peeled tags.
> > > + ref-pattern <pattern>
> > > + When specified, only references matching the one of the provided
> > > + patterns are displayed.
> >
> > How do we match those patterns? That's probably an important thing to
> > include in the spec.
>
> Yeah I thought about it when I first wrote it and was hoping that
> someone who nudge me in the right direction :)
>
> >
> > Looking at the code, I see:
> >
> > > +/*
> > > + * Check if one of the patterns matches the tail part of the ref.
> > > + * If no patterns were provided, all refs match.
> > > + */
> > > +static int ref_match(const struct argv_array *patterns, const char
> > > *refname)
> >
> > This kind of tail matching can't quite implement all of the current
> > behavior. Because we actually do the normal dwim_ref() matching, which
> > includes stuff like "refs/remotes/%s/HEAD".
> >
> > The other problem with tail-matching is that it's inefficient on the
> > server. Ideally we could get a request for "master" and only look up
> > refs/heads/master, refs/tags/master, etc. And if there are 50,000 refs
> > in refs/pull, we wouldn't have to process those at all. Of course this
> > is no worse than the current code, which not only looks at each ref but
> > actually _sends_ it. But it would be nice if we could fix this.
> >
> > There's some more discussion in this old thread:
> >
> >
> > https://public-inbox.org/git/20161024132932.i42rqn2vlpocq...@sigill.intra.peff.net/
>
> Thanks for the pointer. I was told to be wary a while about about
> performance implications on the server but no discussion ensued till now
> about it :)
>
> We always have the ability to extend the patterns accepted via a feature
> (or capability) to ls-refs, so maybe the best thing to do now would only
> support a few patterns with specific semantics. Something like if you
> say "master" only match against refs/heads/ and refs/tags/ and if you
> want something else you would need to specify "refs/pull/master"?
>
> That way we could only support globs at the end "master*" where * can
> match anything (including slashes)
After some in-office discussion it seems like the best thing to do for
this (right now since if we change our mind we can just introduce a
capability which extends the patterns supported) would be to left-anchor
the ref-patterns and only allow for a single wildcard character '*'
which matches zero or more characters (and doesn't care about slashes
'/'). This wildcard character should only be supported at the end of
the ref pattern. This means that if a client wants 'master' then they
would need to specify 'refs/heads/master' (and the other
ref_rev_parse_rules expansions) as a ref pattern. But they could say
"refs/heads/*" for all refs under refs/heads.
>
> >
> > > +{
> > > + char *pathbuf;
> > > + int i;
> > > +
> > > + if (!patterns->argc)
> > > + return 1; /* no restriction */
> > > +
> > > + pathbuf = xstrfmt("/%s", refname);
> > > + for (i = 0; i < patterns->argc; i++) {
> > > + if (!wildmatch(patterns->argv[i], pathbuf, 0)) {
> > > + free(pathbuf);
> > > + return 1;
> > > + }
> > > + }
> > > + free(pathbuf);
> > > + return 0;
> > > +}
> >
> > Does the client have to be aware that we're using wildmatch? I think
> > they'd need "refs/heads/**" to actually implement what we usually
> > specify in refspecs as "refs/heads/*". Or does the lack of WM_PATHNAME
> > make this work with just "*"?
> >
> > Do we anticipate that the client would left-anchor the refspec like
> > "/refs/heads/*" so that in theory the server could avoid looking outside
> > of /refs/heads/?
>
> Yeah we may want to anchor it by providing the leading '/' instead of
> just "refs/<blah>".
>
> >
> > -Peff
>
> I need to read over the discussion you linked to more but what sort of
> ref patterns do you believe we should support as part of the initial
> release of v2? It seems like you wanted this at some point in the past
> so I assume you have an idea of what sort of filtering would be
> beneficial.
>
> --
> Brandon Williams
--
Brandon Williams
