Re: [PATCH v4 06/10] ref-filter: add option to match literal pattern

[Karthik Nayak <karthik.188@gmail.com>]

On Sun, Jul 26, 2015 at 10:45 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Fri, Jul 24, 2015 at 3:04 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Since 'ref-filter' only has an option to match path names add an
>> option for plain fnmatch pattern-matching.
>>
>> This is to support the pattern matching options which are used in `git
>> tag -l` and `git branch -l` where we can match patterns like `git tag
>> -l foo*` which would match all tags which has a "foo*" pattern.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 8f2148f..0a64b84 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -951,6 +951,31 @@ static int commit_contains(struct ref_filter *filter, struct commit *commit)
>>
>>  /*
>>   * Return 1 if the refname matches one of the patterns, otherwise 0.
>> + * A pattern can be a literal prefix (e.g. a refname "refs/heads/master"
>> + * matches a pattern "refs/heads/mas") or a wildcard (e.g. the same ref
>> + * matches "refs/heads/mas*", too).
>> + */
>> +static int match_pattern(const char **patterns, const char *refname)
>> +{
>> +       for (; *patterns; patterns++) {
>> +               /*
>> +                * When no '--format' option is given we need to skip the prefix
>> +                * for matching refs of tags and branches.
>> +                */
>> +               if (!starts_with(*patterns, "refs/tags/"))
>> +                       skip_prefix(refname, "refs/tags/", &refname);
>> +               if (!starts_with(*patterns, "refs/heads/"))
>> +                       skip_prefix(refname, "refs/heads/", &refname);
>> +               if (!starts_with(*patterns, "refs/remotes/"))
>> +                       skip_prefix(refname, "refs/remotes/", &refname);
>
> Note the inefficiency here: You're performing an "expensive"
> starts_with() on each pattern for each refname even though the
> patterns will never change. You could instead compute starts_with()
> for each pattern just once, in a preprocessing step, and remember each
> result as a boolean, and then use the computed booleans here in place
> of starts_with(). For a few refnames, this may not make a difference,
> but for a project with a huge number, it could. Whether such an
> optimization is worth the complexity (at this time or ever) is
> something that can be answered by taking measurements.
>
> Also, the repetition in the code is not all that pretty. You could
> instead place "refs/tags/", "refs/heads/", and "refs/remotes/" in a
> table and then loop over them rather than repeating the code for each
> one, though whether that would be an improvement is likely a judgment
> call (so not something I'd insist upon).
>

I just put the "starts_with()" code so as to ensure that its only used when
we don't say "refs/heads/", "refs/remotes/" or "refs/tags/" in the pattern.
But looking at the tag.c and branch.c implementations this should always be
done. Hence I think Ill move it outside the loop and make it mandatory as this
pattern matching is only used by tag and branch and as they by default
remove the
path of the ref. I think it'd make sense to remove it here also.

-- 
Regards,
Karthik Nayak