Re: [PATCH v15 06/13] ref-filter: add option to filter out tags, branches and remotes

[Karthik Nayak]

On Wed, Sep 2, 2015 at 9:45 AM, Junio C Hamano  wrote:
> Karthik Nayak  writes:
>
 + if (filter->kind == FILTER_REFS_BRANCHES)
 + ret = for_each_fullref_in("refs/heads/", ref_filter_handler,
 _cbdata, broken);
 + else if (filter->kind == FILTER_REFS_REMOTES)
 + ret = for_each_fullref_in("refs/remotes/", ref_filter_handler,
 _cbdata, broken);
 + else if (filter->kind == FILTER_REFS_TAGS)
 + ret = for_each_fullref_in("refs/tags/", ref_filter_handler,
 _cbdata, broken);
 + else if (filter->kind & FILTER_REFS_ALL)
 + ret = for_each_fullref_in("", ref_filter_handler, _cbdata,
 broken);
>>>
>>> This if/else if/else if/ cascade and ...
>>
>> Did you notice the "==" for others and "&" for the ALL?
>
> I didn't.  Thanks for pointing it out.
>
> So the point of the earlier part of the cascade is to optimize for
> common cases?  If that is the case, it probably deserves some
> commenting.  I also suspect that a table-based control might be
> easier to maintain, but that kind of change might fall into the
> category of premature optimization.

Yes, thats the point.
Will add a comment, thanks :)

-- 
Regards,
Karthik Nayak
--
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

[PATCH v15 06/13] ref-filter: add option to filter out tags, branches and remotes

[Karthik Nayak]

From: Karthik Nayak 

Add a function called 'for_each_fullref_in()' to refs.{c,h} which
iterates through each ref for the given path without trimming the path
and also accounting for broken refs, if mentioned.

Add 'filter_ref_kind()' in ref-filter.c to check the kind of ref being
handled and return the kind to 'ref_filter_handler()', where we
discard refs which we do not need and assign the kind to needed refs.

Mentored-by: Christian Couder 
Mentored-by: Matthieu Moy 
Signed-off-by: Karthik Nayak 
---
 ref-filter.c | 63 +++-
 ref-filter.h | 13 +++--
 refs.c   |  9 +
 refs.h   |  1 +
 4 files changed, 79 insertions(+), 7 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 3a5f0a7..430c80b 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1163,6 +1163,34 @@ static struct ref_array_item *new_ref_array_item(const 
char *refname,
return ref;
 }
 
+static int filter_ref_kind(struct ref_filter *filter, const char *refname)
+{
+   unsigned int i;
+
+   static struct {
+   const char *prefix;
+   unsigned int kind;
+   } ref_kind[] = {
+   { "refs/heads/" , FILTER_REFS_BRANCHES },
+   { "refs/remotes/" , FILTER_REFS_REMOTES },
+   { "refs/tags/", FILTER_REFS_TAGS}
+   };
+
+   if (filter->kind == FILTER_REFS_BRANCHES ||
+   filter->kind == FILTER_REFS_REMOTES ||
+   filter->kind == FILTER_REFS_TAGS)
+   return filter->kind;
+   else if (!strcmp(refname, "HEAD"))
+   return FILTER_REFS_DETACHED_HEAD;
+
+   for (i = 0; i < ARRAY_SIZE(ref_kind); i++) {
+   if (starts_with(refname, ref_kind[i].prefix))
+   return ref_kind[i].kind;
+   }
+
+   return FILTER_REFS_OTHERS;
+}
+
 /*
  * A call-back given to for_each_ref().  Filter refs and keep them for
  * later object processing.
@@ -1173,6 +1201,7 @@ static int ref_filter_handler(const char *refname, const 
struct object_id *oid,
struct ref_filter *filter = ref_cbdata->filter;
struct ref_array_item *ref;
struct commit *commit = NULL;
+   unsigned int kind;
 
if (flag & REF_BAD_NAME) {
warning("ignoring ref with broken name %s", refname);
@@ -1184,6 +1213,15 @@ static int ref_filter_handler(const char *refname, const 
struct object_id *oid,
return 0;
}
 
+   /*
+* Get the current ref kind. If we're filtering tags, remotes or local 
branches
+* only then the current ref-kind is nothing but filter->kind and 
filter_ref_kind()
+* will only return that value.
+*/
+   kind = filter_ref_kind(filter, refname);
+   if (!(kind & filter->kind))
+   return 0;
+
if (*filter->name_patterns && 
!match_name_as_path(filter->name_patterns, refname))
return 0;
 
@@ -1215,6 +1253,7 @@ static int ref_filter_handler(const char *refname, const 
struct object_id *oid,
 
REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
+   ref->kind = kind;
return 0;
 }
 
@@ -1291,17 +1330,31 @@ int filter_refs(struct ref_array *array, struct 
ref_filter *filter, unsigned int
 {
struct ref_filter_cbdata ref_cbdata;
int ret = 0;
+   unsigned int broken = 0;
 
ref_cbdata.array = array;
ref_cbdata.filter = filter;
 
+   if (type & FILTER_REFS_INCLUDE_BROKEN)
+   broken = 1;
+   filter->kind = type & FILTER_REFS_KIND_MASK;
+
/*  Simple per-ref filtering */
-   if (type & (FILTER_REFS_ALL | FILTER_REFS_INCLUDE_BROKEN))
-   ret = for_each_rawref(ref_filter_handler, _cbdata);
-   else if (type & FILTER_REFS_ALL)
-   ret = for_each_ref(ref_filter_handler, _cbdata);
-   else if (type)
+   if (!filter->kind)
die("filter_refs: invalid type");
+   else {
+   if (filter->kind == FILTER_REFS_BRANCHES)
+   ret = for_each_fullref_in("refs/heads/", 
ref_filter_handler, _cbdata, broken);
+   else if (filter->kind == FILTER_REFS_REMOTES)
+   ret = for_each_fullref_in("refs/remotes/", 
ref_filter_handler, _cbdata, broken);
+   else if (filter->kind == FILTER_REFS_TAGS)
+   ret = for_each_fullref_in("refs/tags/", 
ref_filter_handler, _cbdata, broken);
+   else if (filter->kind & FILTER_REFS_ALL)
+   ret = for_each_fullref_in("", ref_filter_handler, 
_cbdata, broken);
+   if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))
+   head_ref(ref_filter_handler, _cbdata);
+   }
+
 
/*  Filters that need revision walking */
 

Re: [PATCH v15 06/13] ref-filter: add option to filter out tags, branches and remotes

[Karthik Nayak]

On Wed, Sep 2, 2015 at 3:00 AM, Junio C Hamano  wrote:
> Karthik Nayak  writes:
>
>> + if (!filter->kind)
>>   die("filter_refs: invalid type");
>> + else {
>> + if (filter->kind == FILTER_REFS_BRANCHES)
>> + ret = for_each_fullref_in("refs/heads/", 
>> ref_filter_handler, _cbdata, broken);
>> + else if (filter->kind == FILTER_REFS_REMOTES)
>> + ret = for_each_fullref_in("refs/remotes/", 
>> ref_filter_handler, _cbdata, broken);
>> + else if (filter->kind == FILTER_REFS_TAGS)
>> + ret = for_each_fullref_in("refs/tags/", 
>> ref_filter_handler, _cbdata, broken);
>> + else if (filter->kind & FILTER_REFS_ALL)
>> + ret = for_each_fullref_in("", ref_filter_handler, 
>> _cbdata, broken);
>
> This if/else if/else if/ cascade and ...

Did you notice the "==" for others and "&" for the ALL?

>
>> + if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))
>> + head_ref(ref_filter_handler, _cbdata);
>> + }
>> +
>>
>>   /*  Filters that need revision walking */
>>   if (filter->merge_commit)
>> diff --git a/ref-filter.h b/ref-filter.h
>> index 45026d0..0913ba9 100644
>> --- a/ref-filter.h
>> +++ b/ref-filter.h
>> @@ -13,8 +13,15 @@
>>  #define QUOTE_PYTHON 4
>>  #define QUOTE_TCL 8
>>
>> -#define FILTER_REFS_INCLUDE_BROKEN 0x1
>> -#define FILTER_REFS_ALL 0x2
>> +#define FILTER_REFS_INCLUDE_BROKEN 0x0001
>> +#define FILTER_REFS_TAGS   0x0002
>> +#define FILTER_REFS_BRANCHES   0x0004
>> +#define FILTER_REFS_REMOTES0x0008
>> +#define FILTER_REFS_OTHERS 0x0010
>
> ... the bit assignment here conceptually do not mesh well with each
> other.  These bits look as if I can ask for both tags and branches
> by passing 0x0006, and if the code above were
>
> empty the result set;
> if (filter->kind & FILTER_REFS_BRANCHES)
> add in things from refs/heads/ to the result set;
> if (filter->kind & FILTER_REFS_TAGS)
> add in things from refs/tags/ to the result set;
> ...
>
> without "else if", that would conceptually make sense.
>
> Alternatively, if the code does not (and will not ever) support such
> an arbitrary mixing of bits and intends to only allow "one of
> BRANCHES, REMOTES, TAGS or ALL, and no other choice", then it is
> wrong to pretend as if they can be mixed by defining the constant
> with values with non-overlapping bit patterns.  If you defined these
> constants as

Hmm, The code does support mixing if you see, whenever we mix and match
these bits (consider 0x0006) we satisfy the ` else if (filter->kind &
FILTER_REFS_ALL)`
condition. Which would then go through the entire set of refs and pick
only refs we
need using filter_ref_kind(). This may seem a little confusing but this avoids
ref type filtering when we do not mix bits up.

>
> #define FILTER_REFS_TAGS   100
> #define FILTER_REFS_BRANCHES   101
> #define FILTER_REFS_REMOTES102
> #define FILTER_REFS_OTHERS 103
>
> then nobody would be think that the function can collect both tags
> and branches by passing FILTER_REFS_TAGS|FILTER_REFS_BRANCHES.
>
> The former, i.e. keep the bits distinct and allow them to be OR'ed
> together by updating the code to allow such callers, would be more
> preferrable, of course.
>

Just to confirm, I even changed the function call of for-each-ref to
filter_refs(, , FILTER_REFS_BRANCHES |
FILTER_REFS_TAGS | FILTER_REFS_INCLUDE_BROKEN);
and it printed only "refs/heads/" and "refs/tags/".
Thanks :)

-- 
Regards,
Karthik Nayak
--
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: [PATCH v15 06/13] ref-filter: add option to filter out tags, branches and remotes

[Junio C Hamano]

Karthik Nayak  writes:

> + if (!filter->kind)
>   die("filter_refs: invalid type");
> + else {
> + if (filter->kind == FILTER_REFS_BRANCHES)
> + ret = for_each_fullref_in("refs/heads/", 
> ref_filter_handler, _cbdata, broken);
> + else if (filter->kind == FILTER_REFS_REMOTES)
> + ret = for_each_fullref_in("refs/remotes/", 
> ref_filter_handler, _cbdata, broken);
> + else if (filter->kind == FILTER_REFS_TAGS)
> + ret = for_each_fullref_in("refs/tags/", 
> ref_filter_handler, _cbdata, broken);
> + else if (filter->kind & FILTER_REFS_ALL)
> + ret = for_each_fullref_in("", ref_filter_handler, 
> _cbdata, broken);

This if/else if/else if/ cascade and ...

> + if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))
> + head_ref(ref_filter_handler, _cbdata);
> + }
> +
>  
>   /*  Filters that need revision walking */
>   if (filter->merge_commit)
> diff --git a/ref-filter.h b/ref-filter.h
> index 45026d0..0913ba9 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -13,8 +13,15 @@
>  #define QUOTE_PYTHON 4
>  #define QUOTE_TCL 8
>  
> -#define FILTER_REFS_INCLUDE_BROKEN 0x1
> -#define FILTER_REFS_ALL 0x2
> +#define FILTER_REFS_INCLUDE_BROKEN 0x0001
> +#define FILTER_REFS_TAGS   0x0002
> +#define FILTER_REFS_BRANCHES   0x0004
> +#define FILTER_REFS_REMOTES0x0008
> +#define FILTER_REFS_OTHERS 0x0010

... the bit assignment here conceptually do not mesh well with each
other.  These bits look as if I can ask for both tags and branches
by passing 0x0006, and if the code above were

empty the result set;
if (filter->kind & FILTER_REFS_BRANCHES)
add in things from refs/heads/ to the result set;
if (filter->kind & FILTER_REFS_TAGS)
add in things from refs/tags/ to the result set;
...

without "else if", that would conceptually make sense.

Alternatively, if the code does not (and will not ever) support such
an arbitrary mixing of bits and intends to only allow "one of
BRANCHES, REMOTES, TAGS or ALL, and no other choice", then it is
wrong to pretend as if they can be mixed by defining the constant
with values with non-overlapping bit patterns.  If you defined these
constants as

#define FILTER_REFS_TAGS   100
#define FILTER_REFS_BRANCHES   101
#define FILTER_REFS_REMOTES102
#define FILTER_REFS_OTHERS 103

then nobody would be think that the function can collect both tags
and branches by passing FILTER_REFS_TAGS|FILTER_REFS_BRANCHES.

The former, i.e. keep the bits distinct and allow them to be OR'ed
together by updating the code to allow such callers, would be more
preferrable, of course.



--
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: [PATCH v15 06/13] ref-filter: add option to filter out tags, branches and remotes

[Junio C Hamano]

Karthik Nayak  writes:

>>> + if (filter->kind == FILTER_REFS_BRANCHES)
>>> + ret = for_each_fullref_in("refs/heads/", ref_filter_handler,
>>> _cbdata, broken);
>>> + else if (filter->kind == FILTER_REFS_REMOTES)
>>> + ret = for_each_fullref_in("refs/remotes/", ref_filter_handler,
>>> _cbdata, broken);
>>> + else if (filter->kind == FILTER_REFS_TAGS)
>>> + ret = for_each_fullref_in("refs/tags/", ref_filter_handler,
>>> _cbdata, broken);
>>> + else if (filter->kind & FILTER_REFS_ALL)
>>> + ret = for_each_fullref_in("", ref_filter_handler, _cbdata,
>>> broken);
>>
>> This if/else if/else if/ cascade and ...
>
> Did you notice the "==" for others and "&" for the ALL?

I didn't.  Thanks for pointing it out.

So the point of the earlier part of the cascade is to optimize for
common cases?  If that is the case, it probably deserves some
commenting.  I also suspect that a table-based control might be
easier to maintain, but that kind of change might fall into the
category of premature optimization.
--
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