Re: [PATCH v3 13/35] ls-refs: introduce ls-refs server command

> 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@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