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

On Thu, Aug 27, 2015 at 8:54 PM, Michael Haggerty [off-list ref] wrote:

On 08/27/2015 02:42 PM, Karthik Nayak wrote:

quoted

On Wed, Aug 26, 2015 at 9:40 PM, Michael Haggerty [off-list ref] wrote:

quoted

On 08/22/2015 05:39 AM, Karthik Nayak wrote:

quoted

[...]
+     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.

Well then we'd need another ref_array just for that, that also could
be an option.
But apart from printing it first, everything else is the same for all the refs.

quoted

quoted

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.

No, of course, I missed that while typing here.

-- 
Regards,
Karthik Nayak