Re: [WIP/PATCH v4 1/8] for-each-ref: extract helper functions out of grab_single_ref()
Matthieu Moy writes: > Just match_refname may not carry all the semantics of the function, > which matches a prefix up to the end of string, or up to a / (but you > can't just be a prefix stopping in the middle of a word). To me, the > "_as_path" helped understanding the overall behavior of the function. Yeah, Karthik explained that the plan is to add different kinds of matching later, and in that context _as_path is a good way to tell them apart. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [WIP/PATCH v4 1/8] for-each-ref: extract helper functions out of grab_single_ref()
Junio C Hamano writes:
> Karthik Nayak writes:
>
>> /*
>> + * Given a refname, return 1 if the refname matches with one of the patterns
>> + * while the pattern is a pathname like 'refs/tags' or 'refs/heads/master'
>> + * and so on, else return 0. Supports use of wild characters.
>> + */
>> +static int match_name_as_path(const char **pattern, const char *refname)
>> +{
>
> I wonder why this is not "match_refname", in other words, what the
> significance of "as path" part of the name?
Just match_refname may not carry all the semantics of the function,
which matches a prefix up to the end of string, or up to a / (but you
can't just be a prefix stopping in the middle of a word). To me, the
"_as_path" helped understanding the overall behavior of the function.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [WIP/PATCH v4 1/8] for-each-ref: extract helper functions out of grab_single_ref()
On 05/31/2015 11:04 PM, Junio C Hamano wrote:
Karthik Nayak writes:
/*
+ * Given a refname, return 1 if the refname matches with one of the patterns
+ * while the pattern is a pathname like 'refs/tags' or 'refs/heads/master'
+ * and so on, else return 0. Supports use of wild characters.
+ */
+static int match_name_as_path(const char **pattern, const char *refname)
+{
I wonder why this is not "match_refname", in other words, what the
significance of "as path" part of the name? If you later are going
to introuce match_name_as_something_else() and that name may not be
a refname, then this naming is perfectly sane. If such a function
you will later introduce will still deal with names that are only
refnames, then match_refname_as_path() would be sensible. Otherwise
this name seems overly long (i.e. "as_path" may not be adding value)
while not being desctiptive enough (i.e. is meant to be limited to
refnames but just says "name").
Just being curious.
Good Question, This is because in the future I'll eventually add pattern
matching for 'tag -l' and 'branch -l' which wont match as path, but
would be a regular string match (with wild character support). Hence
keeping that in mind I included 'as_path()' in the function name.
The comment still is a good reminder for those who will later touch
this grab_single_ref() function to make them think twice.
Thanks.
Yes! It was removed it by mistake.
--
Regards,
Karthik
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [WIP/PATCH v4 1/8] for-each-ref: extract helper functions out of grab_single_ref()
Karthik Nayak writes:
> /*
> + * Given a refname, return 1 if the refname matches with one of the patterns
> + * while the pattern is a pathname like 'refs/tags' or 'refs/heads/master'
> + * and so on, else return 0. Supports use of wild characters.
> + */
> +static int match_name_as_path(const char **pattern, const char *refname)
> +{
I wonder why this is not "match_refname", in other words, what the
significance of "as path" part of the name? If you later are going
to introuce match_name_as_something_else() and that name may not be
a refname, then this naming is perfectly sane. If such a function
you will later introduce will still deal with names that are only
refnames, then match_refname_as_path() would be sensible. Otherwise
this name seems overly long (i.e. "as_path" may not be adding value)
while not being desctiptive enough (i.e. is meant to be limited to
refnames but just says "name").
Just being curious.
> @@ -851,40 +888,16 @@ static int grab_single_ref(const char *refname, const
> unsigned char *sha1, int f
> return 0;
> }
>
> - if (*cb->grab_pattern) {
> -...
> - }
> + if (*cb->grab_pattern && !match_name_as_path(cb->grab_pattern, refname))
> + return 0;
>
> - /*
> - * We do not open the object yet; sort may only need refname
> - * to do its job and the resulting list may yet to be pruned
> - * by maxcount logic.
> - */
> - ref = xcalloc(1, sizeof(*ref));
The comment still is a good reminder for those who will later touch
this grab_single_ref() function to make them think twice.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [WIP/PATCH v4 1/8] for-each-ref: extract helper functions out of grab_single_ref()
Why did this comment get removed? Isn't it still meaningful to the overall logic of this function? Wasn't intentional, will put that back. Sneaking in whitespace change? Noted. thanks -- Regards, Karthik -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [WIP/PATCH v4 1/8] for-each-ref: extract helper functions out of grab_single_ref()
On Sat, May 30, 2015 at 1:53 PM, Karthik Nayak wrote:
> Extract two helper functions out of grab_single_ref(). Firstly,
> new_refinfo() which is used to allocate memory for a new refinfo
> structure and copy the objectname, refname and flag to it.
> Secondly, match_name_as_path() which when given an array of patterns
> and the refname checks if the refname matches any of the patterns
> given while the pattern is a pathname, also while supporting wild
> characters.
>
> This is a preperatory patch for restructuring 'for-each-ref' and
> eventually moving most of it to 'ref-filter' to provide the
> functionality to similar commands via public API's.
>
> Helped-by: Junio C Hamano
> Mentored-by: Christian Couder
> Mentored-by: Matthieu Moy
> Signed-off-by: Karthik Nayak
> ---
> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index 83f9cf9..b33e2de 100644
> --- a/builtin/for-each-ref.c
> +++ b/builtin/for-each-ref.c
> @@ -837,6 +837,43 @@ struct grab_ref_cbdata {
> };
>
> /*
> + * Given a refname, return 1 if the refname matches with one of the patterns
> + * while the pattern is a pathname like 'refs/tags' or 'refs/heads/master'
> + * and so on, else return 0. Supports use of wild characters.
> + */
> +static int match_name_as_path(const char **pattern, const char *refname)
> +{
> + int namelen = strlen(refname);
> + for (; *pattern; pattern++) {
> + const char *p = *pattern;
> + int plen = strlen(p);
> +
> + if ((plen <= namelen) &&
> + !strncmp(refname, p, plen) &&
> + (refname[plen] == '\0' ||
> +refname[plen] == '/' ||
> +p[plen-1] == '/'))
> + return 1;
> + if (!wildmatch(p, refname, WM_PATHNAME, NULL))
> + return 1;
> + }
> + return 0;
> +}
> +
> +/* Allocate space for a new refinfo and copy the objectname and flag to it */
> +static struct refinfo *new_refinfo(const char *refname,
> + const unsigned char *objectname,
> + int flag)
> +{
> + struct refinfo *ref = xcalloc(1, sizeof(struct refinfo));
> + ref->refname = xstrdup(refname);
> + hashcpy(ref->objectname, objectname);
> + ref->flag = flag;
> +
> + return ref;
> +}
> +
> +/*
> * A call-back given to for_each_ref(). Filter refs and keep them for
> * later object processing.
> */
> @@ -851,40 +888,16 @@ static int grab_single_ref(const char *refname, const
> unsigned char *sha1, int f
> return 0;
> }
>
> - if (*cb->grab_pattern) {
> - const char **pattern;
> - int namelen = strlen(refname);
> - for (pattern = cb->grab_pattern; *pattern; pattern++) {
> - const char *p = *pattern;
> - int plen = strlen(p);
> -
> - if ((plen <= namelen) &&
> - !strncmp(refname, p, plen) &&
> - (refname[plen] == '\0' ||
> -refname[plen] == '/' ||
> -p[plen-1] == '/'))
> - break;
> - if (!wildmatch(p, refname, WM_PATHNAME, NULL))
> - break;
> - }
> - if (!*pattern)
> - return 0;
> - }
> + if (*cb->grab_pattern && !match_name_as_path(cb->grab_pattern,
> refname))
> + return 0;
>
> - /*
> -* We do not open the object yet; sort may only need refname
> -* to do its job and the resulting list may yet to be pruned
> -* by maxcount logic.
> -*/
Why did this comment get removed? Isn't it still meaningful to the
overall logic of this function?
> - ref = xcalloc(1, sizeof(*ref));
> - ref->refname = xstrdup(refname);
> - hashcpy(ref->objectname, sha1);
> - ref->flag = flag;
> + ref = new_refinfo(refname, sha1, flag);
>
> cnt = cb->grab_cnt;
> REALLOC_ARRAY(cb->grab_array, cnt + 1);
> cb->grab_array[cnt++] = ref;
> cb->grab_cnt = cnt;
> +
Sneaking in whitespace change?
> return 0;
> }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
