Re: [PATCHv3 1/5] refs: add match_pattern()
- From
Tom Grennan <tmgrennan@gmail.com>
- Date
- Feb 22, 2012, 23:47 UTC
- Message-ID
- <20120222234733.GD2410@tgrennan-laptop>
- In-Reply-To
- <7vobsrbcny.fsf@alter.siamese.dyndns.org>
On Tue, Feb 21, 2012 at 10:33:05PM -0800, Junio C Hamano wrote:
Show 16 quoted lines
>Tom Grennan <tmgrennan@gmail.com> writes:
>
>> +static int match_path(const char *name, const char *pattern, int nlen)
>> +{
>> + int plen = strlen(pattern);
>> +
>> + return ((plen <= nlen) &&
>> + !strncmp(name, pattern, plen) &&
>> + (name[plen] == '\0' ||
>> + name[plen] == '/' ||
>> + pattern[plen-1] == '/'));
>> +}
>
>This is a counterpart to the tail match found in ls-remote, so we would
>want to call it with a name that makes it clear this is a leading path
>match not just "path" match. Perhaps match_leading_path() or something.OK
Show 31 quoted lines
>> +int match_pattern(const char *name, const char **match,
>> + struct string_list *exclude, int flags)
>> +{
>> + int nlen = strlen(name);
>> +
>> + if (exclude) {
>> + struct string_list_item *x;
>> + for_each_string_list_item(x, exclude) {
>> + if (!fnmatch(x->string, name, 0))
>> + return 0;
>> + }
>> + }
>> + if (!match || !*match)
>> + return 1;
>> + for (; *match; match++) {
>> + if (flags == FNM_PATHNAME)
>> + if (match_path(name, *match, nlen))
>> + return 1;
>> + if (!fnmatch(*match, name, flags))
>> + return 1;
>> + }
>> + return 0;
>> +}
>
>As an API for a consolidated and generic function, the design needs a bit
>more improving, I would think.
>
> - The name match_pattern() was OK for a static function inside a single
> file, but it is way too vague for a global function. This is to match
> refnames, so I suspect there should at least be a string "ref_"
> somewhere in its name.OK
> - You pass "flags" argument, so that later we _could_ enhance the > implementation to cover needs for new callers, but alas, it uses its > full bits to express only one "do we do FNM_PATHNAME or not?" bit of > information, so essentially "flags" does not give us any expandability.
I agree.
> - Is it a sane assumption that a caller that asks FNM_PATHNAME will > always want match_path() semantics, too? Aren't these two logically > independent?
Yes, these should be ligically independent although the current use has combined them.
> - Is it a sane assumption that a caller that gives an exclude list will > want neither FNM_PATHNAME semantics nor match_path() semantics?
I'm not sure. I tried using FNM_PATHNAME with both exclusion and match patterns of git-for-each-ref but I couldn't get it to do something like this: git for-each-ref ... --exclude '*HEAD' refs/remotes/
I don't remember if this worked, git for-each-ref ... --exclude HEAD refs/remotes/
Now I see how an implicit TRAILING match would be useful, git for-each-ref ... --exclude /HEAD refs/remotes/
Where git-for-each-ref uses this flag: REF_MATCH_LEADING | REF_MATCH_TRAILING | REF_MATCH_FNM_PATH
I'll experiment with this more.
> - Positive patterns are passed in "const char **match", and negative ones > are in "struct string_list *". Doesn't the inconsistency strike you as > strange?
Yes, I tried to minimize change but the conversion of argv's to string_list's won't add that much.
Show 52 quoted lines
>Perhaps like...
>
>#define REF_MATCH_LEADING 01
>#define REF_MATCH_TRAILING 02
>#define REF_MATCH_FNM_PATH 04
>
>static int match_one(const char *name, size_t namelen, const char *pattern,
> unsigned flags)
>{
> if ((flags & REF_MATCH_LEADING) &&
> match_leading_path(name, pattern, namelen))
> return 1;
> if ((flags & REF_MATCH_TRAILING) &&
> match_trailing_path(name, pattern, namelen))
> return 1;
> if (!fnmatch(pattern, name,
> (flags & REF_MATCH_FNM_PATH) ? FNM_PATHNAME : 0))
> return 1;
> return 0;
>}
>
>int ref_match_pattern(const char *name,
> const char **pattern, const char **exclude, unsigned flags)
>{
> size_t namelen = strlen(name);
> if (exclude) {
> while (*exclude) {
> if (match_one(name, namelen, *exclude, flags))
> return 0;
> exclude++;
> }
> }
> if (!pattern || !*pattern)
> return 1;
> while (*pattern) {
> if (match_one(name, namelen, *pattern, flags))
> return 1;
> pattern++;
> }
> return 0;
>}
>
>and then the caller could do something like
>
> ref_match_pattern("refs/heads/master",
> ["maste?", NULL],
> ["refs/heads/", NULL],
> (REF_MATCH_FNM_PATH|REF_MATCH_LEADING));
>
>Note that the above "ref_match_pattern()" gives the same "flags" for the
>call to match_one() for elements in both positive and negative array and
>it is very deliberate. See review comment to [3/5] for the reasoning.OK, I think that I understand, but please confirm, you'd expect no output in the above example, right?
-- TomG