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.i42rqn2vlpocqmkq@xxxxxxxxxxxxxxxxxxxxx/ > > 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
