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

From: Junio C Hamano <gitster@pobox.com>
Date: May 20, 2015, 23:50 UTC
Message-ID: <xmqqvbfmlsfu.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1432127904-21070-3-git-send-email-karthik.188@gmail.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.

Show 5 quoted lines

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