Re: [PATCH 3/4] for-each-ref: convert to ref-filter

[Junio C Hamano <gitster@pobox.com>]

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

> convert 'for-each-ref' to use the common API provided by 'ref-filter'.

Start a sentence with capital?

More importantly, the above is misleading, as if you invented a new
ref-filter API and made for-each-ref build on that new
infrastructure.

This series is in a form that is very unfriendly to reviewers.  The
previous step did not introduce any callers to ref-filter, so for
the purpose of review, it needs to be read together with this step
anyway.

And when reading these patches that way, what this half is really
doing is to move the code from for-each-ref to ref-filter, but it
does unnecessary or unrelated renaming of a handful of symbols.  It
makes it even harder to compare and contrast the original code that
was in the original for-each-ref and moved code that ends up in the
new ref-filter.  Don't do that.

You would probably want to organize them in these two steps instead:

 * Rename symbols as necessary while all the code is still in
   for-each-ref. Do not create ref-filter in this step. Justify it
   along the lines of "some symbol names were fine while they were
   file scope static implementation detail of for-each-ref, but we
   will make the machinery available from other commands by moving
   it to a library-ish place, so rename X to foo_X to clarify that
   this is about foo (which is now necessary as it is not specific
   to for-each-ref"...

 * If you want to do other tweaks like wrapping refs & num_refs into
   a single structure, do so while the code is still in
   for-each-ref.  You can do that in the same patch as the above
   (i.e. it's just part of preparatory step for a move).

 * Create ref-filter by _moving_ code from for-each-ref. Do not
   touch these moved lines in this step. You would need to add
   include at the top of for-each-ref and ref-filter, of course.


> -	for_each_rawref(grab_single_ref, &cbdata);
> -	refs = cbdata.grab_array;
> -	num_refs = cbdata.grab_cnt;
> +	refs.name_patterns = argv;
> +	for_each_rawref(ref_filter_add, &refs);

I think ref_filter_add() may be misnamed as a public API function.
grab_single_ref() was OK only because it was an implementation
dtail, but if you are making it public, the name should make it
clear that it is meant to be used as a for_each_*ref() callback
function.  Otherwise people may be tempted to add random parameter
to it in the future, but the signature of that function is dictated
by for_each_*ref() API.