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) > > > +{ > > + 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
