On 05/21/2015 05:20 AM, Junio C Hamano wrote:
Karthik Nayak <karthik.188@xxxxxxxxx> 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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html