Re: [PATCH 2/2] wildmatch: use the new precompiling wildmatch()

[Ævar Arnfjörð Bjarmason]

On Mon, Feb 26 2018, Duy Nguyen jotted:

> On Mon, Feb 26, 2018 at 3:35 AM, Ævar Arnfjörð Bjarmason
>  wrote:
>> diff --git a/config.c b/config.c
>> index b0c20e6cb8..0f595de971 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -210,6 +210,7 @@ static int include_by_gitdir(const struct config_options 
>> *opts,
>> int ret = 0, prefix;
>> const char *git_dir;
>> int already_tried_absolute = 0;
>> +   struct wildmatch_compiled *wildmatch_compiled = NULL;
>>
>> if (opts->git_dir)
>> git_dir = opts->git_dir;
>> @@ -237,8 +238,10 @@ static int include_by_gitdir(const struct 
>> config_options *opts,
>> goto done;
>> }
>>
>> -   ret = !wildmatch(pattern.buf + prefix, text.buf + prefix,
>> -icase ? WM_CASEFOLD : 0);
>> +   if (!wildmatch_compiled)
>> +   wildmatch_compiled = wildmatch_compile(pattern.buf + prefix,
>> +  icase ? WM_CASEFOLD : 
>> 0);
>> +   ret = !wildmatch_match(wildmatch_compiled, text.buf + prefix);
>
> This is a one-shot matching.

It will match once *or* twice depending on how the goto in that codepath
does.

> Is it worth converting to the new interface?

I moved it more as a showcase, to show how the interface looks like in
common scenarios. It obviously won't be a performance bottleneck or
anything like that for this specific codepath.

>>
>> if (!ret && !already_tried_absolute) {
>> /*
>> @@ -257,6 +260,7 @@ static int include_by_gitdir(const struct config_options 
>> *opts,
>>  done:
>> strbuf_release();
>> strbuf_release();
>> +   wildmatch_free(wildmatch_compiled);
>> return ret;
>>  }
>>
>> diff --git a/refs.c b/refs.c
>> index 20ba82b434..c631793d1e 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -213,7 +213,7 @@ char *resolve_refdup(const char *refname, int 
>> resolve_flags,
>>
>>  /* The argument to filter_refs */
>>  struct ref_filter {
>> -   const char *pattern;
>> +   struct wildmatch_compiled *code;
>
> This actually makes me think if we should name this struct wildmatch_pattern.

Yeah, that's a better name.

>> each_ref_fn *fn;
>> void *cb_data;
>>  };
>> @@ -291,7 +291,7 @@ static int filter_refs(const char *refname, const struct 
>> object_id *oid,
>>  {
>> struct ref_filter *filter = (struct ref_filter *)data;
>>
>> -   if (wildmatch(filter->pattern, refname, 0))
>> +   if (wildmatch_match(filter->code, refname))
>> return 0;
>> return filter->fn(refname, oid, flags, filter->cb_data);
>>  }
>> @@ -454,12 +454,13 @@ int for_each_glob_ref_in(each_ref_fn fn, const char 
>> *pattern,
>> strbuf_addch(_pattern, '*');
>> }
>>
>> -   filter.pattern = real_pattern.buf;
>> +   filter.code = wildmatch_compile(real_pattern.buf, 0);
>> filter.fn = fn;
>> filter.cb_data = cb_data;
>> ret = for_each_ref(filter_refs, );
>>
>> strbuf_release(_pattern);
>> +   wildmatch_free(filter.code);
>> return ret;
>>  }
>>
>> --
>> 2.15.1.424.g9478a66081
>>

Re: [PATCH 2/2] wildmatch: use the new precompiling wildmatch()

[Duy Nguyen]

On Mon, Feb 26, 2018 at 3:35 AM, Ævar Arnfjörð Bjarmason
 wrote:
> diff --git a/config.c b/config.c
> index b0c20e6cb8..0f595de971 100644
> --- a/config.c
> +++ b/config.c
> @@ -210,6 +210,7 @@ static int include_by_gitdir(const struct config_options 
> *opts,
> int ret = 0, prefix;
> const char *git_dir;
> int already_tried_absolute = 0;
> +   struct wildmatch_compiled *wildmatch_compiled = NULL;
>
> if (opts->git_dir)
> git_dir = opts->git_dir;
> @@ -237,8 +238,10 @@ static int include_by_gitdir(const struct config_options 
> *opts,
> goto done;
> }
>
> -   ret = !wildmatch(pattern.buf + prefix, text.buf + prefix,
> -icase ? WM_CASEFOLD : 0);
> +   if (!wildmatch_compiled)
> +   wildmatch_compiled = wildmatch_compile(pattern.buf + prefix,
> +  icase ? WM_CASEFOLD : 
> 0);
> +   ret = !wildmatch_match(wildmatch_compiled, text.buf + prefix);

This is a one-shot matching. Is it worth converting to the new interface?

>
> if (!ret && !already_tried_absolute) {
> /*
> @@ -257,6 +260,7 @@ static int include_by_gitdir(const struct config_options 
> *opts,
>  done:
> strbuf_release();
> strbuf_release();
> +   wildmatch_free(wildmatch_compiled);
> return ret;
>  }
>
> diff --git a/refs.c b/refs.c
> index 20ba82b434..c631793d1e 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -213,7 +213,7 @@ char *resolve_refdup(const char *refname, int 
> resolve_flags,
>
>  /* The argument to filter_refs */
>  struct ref_filter {
> -   const char *pattern;
> +   struct wildmatch_compiled *code;

This actually makes me think if we should name this struct wildmatch_pattern.

> each_ref_fn *fn;
> void *cb_data;
>  };
> @@ -291,7 +291,7 @@ static int filter_refs(const char *refname, const struct 
> object_id *oid,
>  {
> struct ref_filter *filter = (struct ref_filter *)data;
>
> -   if (wildmatch(filter->pattern, refname, 0))
> +   if (wildmatch_match(filter->code, refname))
> return 0;
> return filter->fn(refname, oid, flags, filter->cb_data);
>  }
> @@ -454,12 +454,13 @@ int for_each_glob_ref_in(each_ref_fn fn, const char 
> *pattern,
> strbuf_addch(_pattern, '*');
> }
>
> -   filter.pattern = real_pattern.buf;
> +   filter.code = wildmatch_compile(real_pattern.buf, 0);
> filter.fn = fn;
> filter.cb_data = cb_data;
> ret = for_each_ref(filter_refs, );
>
> strbuf_release(_pattern);
> +   wildmatch_free(filter.code);
> return ret;
>  }
>
> --
> 2.15.1.424.g9478a66081
>
-- 
Duy

[PATCH 2/2] wildmatch: use the new precompiling wildmatch()

[Ævar Arnfjörð Bjarmason]

Make some limited use of the wildmatch() interface for "precompiling"
patterns, although the current implementation does not do this yet.

The main hot codepath in dir.c is not being touched yet, but some
other invocations where we repeatedly match the same glob against
multiple strings have been migrated.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 builtin/name-rev.c | 7 ++-
 builtin/replace.c  | 7 ---
 config.c   | 8 ++--
 refs.c | 7 ---
 4 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 9e088ebd11..c75ac8d9af 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -128,14 +128,19 @@ static void name_rev(struct commit *commit,
 static int subpath_matches(const char *path, const char *filter)
 {
const char *subpath = path;
+   struct wildmatch_compiled *wildmatch_compiled =
+   wildmatch_compile(filter, 0);
 
while (subpath) {
-   if (!wildmatch(filter, subpath, 0))
+   if (!wildmatch_match(wildmatch_compiled, subpath)) {
+   wildmatch_free(wildmatch_compiled);
return subpath - path;
+   }
subpath = strchr(subpath, '/');
if (subpath)
subpath++;
}
+   wildmatch_free(wildmatch_compiled);
return -1;
 }
 
diff --git a/builtin/replace.c b/builtin/replace.c
index 83d3235721..9be72f2b7b 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -32,7 +32,7 @@ enum replace_format {
 };
 
 struct show_data {
-   const char *pattern;
+   struct wildmatch_compiled *wildmatch_compiled;
enum replace_format format;
 };
 
@@ -41,7 +41,7 @@ static int show_reference(const char *refname, const struct 
object_id *oid,
 {
struct show_data *data = cb_data;
 
-   if (!wildmatch(data->pattern, refname, 0)) {
+   if (!wildmatch_match(data->wildmatch_compiled, refname)) {
if (data->format == REPLACE_FORMAT_SHORT)
printf("%s\n", refname);
else if (data->format == REPLACE_FORMAT_MEDIUM)
@@ -70,7 +70,7 @@ static int list_replace_refs(const char *pattern, const char 
*format)
 
if (pattern == NULL)
pattern = "*";
-   data.pattern = pattern;
+   data.wildmatch_compiled = wildmatch_compile(pattern, 0);
 
if (format == NULL || *format == '\0' || !strcmp(format, "short"))
data.format = REPLACE_FORMAT_SHORT;
@@ -84,6 +84,7 @@ static int list_replace_refs(const char *pattern, const char 
*format)
format);
 
for_each_replace_ref(show_reference, (void *));
+   wildmatch_free(data.wildmatch_compiled);
 
return 0;
 }
diff --git a/config.c b/config.c
index b0c20e6cb8..0f595de971 100644
--- a/config.c
+++ b/config.c
@@ -210,6 +210,7 @@ static int include_by_gitdir(const struct config_options 
*opts,
int ret = 0, prefix;
const char *git_dir;
int already_tried_absolute = 0;
+   struct wildmatch_compiled *wildmatch_compiled = NULL;
 
if (opts->git_dir)
git_dir = opts->git_dir;
@@ -237,8 +238,10 @@ static int include_by_gitdir(const struct config_options 
*opts,
goto done;
}
 
-   ret = !wildmatch(pattern.buf + prefix, text.buf + prefix,
-icase ? WM_CASEFOLD : 0);
+   if (!wildmatch_compiled)
+   wildmatch_compiled = wildmatch_compile(pattern.buf + prefix,
+  icase ? WM_CASEFOLD : 0);
+   ret = !wildmatch_match(wildmatch_compiled, text.buf + prefix);
 
if (!ret && !already_tried_absolute) {
/*
@@ -257,6 +260,7 @@ static int include_by_gitdir(const struct config_options 
*opts,
 done:
strbuf_release();
strbuf_release();
+   wildmatch_free(wildmatch_compiled);
return ret;
 }
 
diff --git a/refs.c b/refs.c
index 20ba82b434..c631793d1e 100644
--- a/refs.c
+++ b/refs.c
@@ -213,7 +213,7 @@ char *resolve_refdup(const char *refname, int resolve_flags,
 
 /* The argument to filter_refs */
 struct ref_filter {
-   const char *pattern;
+   struct wildmatch_compiled *code;
each_ref_fn *fn;
void *cb_data;
 };
@@ -291,7 +291,7 @@ static int filter_refs(const char *refname, const struct 
object_id *oid,
 {
struct ref_filter *filter = (struct ref_filter *)data;
 
-   if (wildmatch(filter->pattern, refname, 0))
+   if (wildmatch_match(filter->code, refname))
return 0;
return filter->fn(refname, oid, flags, filter->cb_data);
 }
@@ -454,12 +454,13 @@ int for_each_glob_ref_in(each_ref_fn fn, const char 
*pattern,
strbuf_addch(_pattern, '*');
}
 
-   filter.pattern = real_pattern.buf;
+   filter.code = wildmatch_compile(real_pattern.buf, 0);
filter.fn =