Re: [PATCH v13 05/12] ref-filter: add option to filter out tags, branches and remotes

[Michael Haggerty <mhagger@alum.mit.edu>]

On 08/27/2015 02:42 PM, Karthik Nayak wrote:
> On Wed, Aug 26, 2015 at 9:40 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> On 08/22/2015 05:39 AM, Karthik Nayak wrote:
>>> [...]
>>> +     if (type == FILTER_REFS_BRANCHES)
>>> +             ret = for_each_reftype_fullpath(ref_filter_handler, "refs/heads/", broken, &ref_cbdata);
>>> +     else if (type == FILTER_REFS_REMOTES)
>>> +             ret = for_each_reftype_fullpath(ref_filter_handler, "refs/remotes/", broken, &ref_cbdata);
>>> +     else if (type == FILTER_REFS_TAGS)
>>> +             ret = for_each_reftype_fullpath(ref_filter_handler, "refs/tags/", broken, &ref_cbdata);
>>> +     else if (type & FILTER_REFS_ALL) {
>>> +             ret = for_each_reftype_fullpath(ref_filter_handler, "", broken, &ref_cbdata);
>>> +             if (type & FILTER_REFS_DETACHED_HEAD)
>>> +                     head_ref(ref_filter_handler, &ref_cbdata);
>>
>> The usual promise of the for_each_ref functions is that they stop
>> iterating if the function ever returns a nonzero value. So the above
>> should be
>>
>>                 if (! ret && (type & FILTER_REFS_DETACHED_HEAD))
>>                         ret = head_ref(ref_filter_handler, &ref_cbdata);
>>
>> Also, these functions usually iterate in lexicographic order, so I think
>> you should process HEAD before the others.
> 
> This is done on purpose, cause we need to print the HEAD ref separately
> so we print the last ref_array_item in the ref_array, free that memory and
> sort and print the rest, hence HEAD ref is attached to the last.

Without having looked at the other patches, this makes me wonder whether
it makes sense to store HEAD in the ref_array at all or whether it
should be handled separately.

>> But there's another problem here. It seems like
>> FILTER_REFS_DETACHED_HEAD is only processed if (type & FILTER_REFS_ALL)
>> is nonzero. But shouldn't it be allowed to process *only* HEAD?
>>
>> So, finally, I think this code should look like
>>
>>         else if (!filter->kind)
>>                 die("filter_refs: invalid type");
>>         else {
>>                 if (filter->kind & FILTER_REFS_DETACHED_HEAD)
>>                         ret = head_ref(ref_filter_handler, &ref_cbdata);
>>                 if (! ret && (filter->kind & FILTER_REFS_ALL))
>>                         ret =
>> for_each_reftype_fullpath(ref_filter_handler, "", broken, &ref_cbdata);
>>         }
>>
> 
> So finally something like this perhaps
> 
>     if (!filter->kind)
>         die("filter_refs: invalid type");
>     else {
>         if (filter->kind == FILTER_REFS_BRANCHES)
>             ret = for_each_reftype_fullpath(ref_filter_handler,
> "refs/heads/", broken, &ref_cbdata);
>         else if (filter->kind == FILTER_REFS_REMOTES)
>             ret = for_each_reftype_fullpath(ref_filter_handler,
> "refs/remotes/", broken, &ref_cbdata);
>         else if (filter->kind == FILTER_REFS_TAGS)
>             ret = for_each_reftype_fullpath(ref_filter_handler,
> "refs/tags/", broken, &ref_cbdata);
>         else if (filter->kind & FILTER_REFS_ALL)
>             ret = for_each_reftype_fullpath(ref_filter_handler, "",
> broken, &ref_cbdata);
>         if (filter->kind & FILTER_REFS_DETACHED_HEAD)
>             head_ref(ref_filter_handler, &ref_cbdata);
>     }

Yes, but the last test should be

        if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))

unless you have a reason not to follow the usual convention that a
nonzero return value from fn means that the iteration should be aborted.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu