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

[karthik nayak <karthik.188@gmail.com>]

On 05/21/2015 05:20 AM, Junio C Hamano wrote:
> 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.
>
Thanks for the suggestion's Junio, will follow with the path you've 
mentioned.
>
>> -	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.
>

Looking through the current each_ref_f() names, I thought I could use
'ref_filter_handler()' instead of 'ref_filter_add()', as per your 
suggestion. What do you think?