Re: [WIP/PATCH v4 6/8] for-each-ref: rename some functions and make them public

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 1, 2015, 14:53 UTC
Message-ID: <xmqqvbf78oow.fsf@gitster.dls.corp.google.com>
In-Reply-To: <556B6234.2010809@gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

Show 10 quoted lines

> Could be achieved using a simple wrapper around 'filter_refs()'
> something like this perhaps.
>
> int filter_refs_with_pattern(struct ref_array *ref, int
> (*for_each_ref_fn)(each_ref_fn, void *), char **patterns)
> {
> 	int i;
> 	struct ref_filter_cbdata data;
> 	data.filter.name_patterns = patterns;
> 	filter_refs(for_each_ref_fn, &data);

I presume that this is

	filter_refs(&refs, for_each_ref_fn, &data);

as you would need to have some way to get the result back ;-)

Show 10 quoted lines

> 	refs->nr = data.array.nr;
> 	for(i = 0; i < refs->nr; i++) {
> 		/* copy over the refs */
> 	}
> 	return 0;
> }
>
> Is this on the lines of what you had in mind? If it is, than I could
> just create a new patch which would make ref_filter_handler() private
> and introduce filter_refs() as shown.

Yeah.  Even though I suggested

	filter_refs(&for_each_ref, ...);

I actually would think the external interface should not mention for_each_ref() like I did. The primary reason why I felt that it is bad for the API to export a generic callback function the caller can use to call for_each_ref() or for_each_rawref()" in the longer term is because it forces us to always iterate all refs; for_each_ref() does not know what the callback filter function wants to do. The most common way to filter in the context of your GSoC project is "we limit only to refs/heads/*, and then we may also filter by other criteria" (that is "git branch" "-l" or possibly with "--contains", etc.), and it is very wasteful for that codepath to allow for_each_ref() to even enumerate and feed all refs outside the refs/heads/* area to your callback, which would involve reading all entries in packed-refs (which is a fixed cost so not an overhead) and then reading everything in .git/refs/* (which is an overhead we could and should avoid when we know we are only interested in the branches that live in refs/heads*).

Your first implementation of course can just call for_each_ref() or for_each_rawref(), and at the end of GSoC, the code may still do so. But by keeping the external interface free of for_each_ref(), you could later optimize.

And your above sample only takes for_each_ref_fn without exposing the internal use of for_each_ref(), which is good.