Re: [PATCH v3 13/35] ls-refs: introduce ls-refs server command
From: Jeff King <hidden>
Date: 2018-02-22 09:48:38
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. 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/
+{
+ 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/? -Peff