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. > - 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. -- 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