Re: [PATCH 1/5] gitweb: Option for filling only specified info in fill_project_list_info

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 9, 2012, 23:18 UTC
Message-ID: <7v4nuzvbnr.fsf@alter.siamese.dyndns.org>
In-Reply-To: <201202092336.48772.jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> writes:

Show 24 quoted lines

>> The lack of any real use of @fill_only in this patch also makes it hard to
>> judge if the new API gives a useful semantics.  I would, without looking
>> at the real usage in 2/5 patch, naïvely expect that such a lazy filling
>> scheme would say "I am going to use A, B and C; I want to know if any of
>> them is missing, because I need values for all of them and I am going to
>> call a helper function to fill them if any of them is missing. Having A
>> and B is not enough for the purpose of this query, because I still need to
>> know C and I would call the helper function that computes all of them in
>> such a case. Even though it might be wasteful to recompute A and B,
>> computing all three at once is the only helper function available to me".
>> 
>> So for a person who does not have access to the real usage of the new API,
>> being able to give only a single $key *appears* make no sense at all, and
>> also the meaning of the @fill_only parameter is unclear, especially the
>> part that checks if that single $key appears in @fill_only.
>
> ...
> information that is not already present.  If @fill_only is nonempty, it
> fills only selected information, again only if it is not already present.
> @fill_only empty means no restrictions... which probably is not very obvious,
> but is documented.
>
> project_info_needs_filling() returns true if $key is not filled and is
> interesting.

That still does not answer the fundamental issues I had with the presented
API: why does it take only a single $key (please re-read my "A, B and C"
example), and what does that single $key intersecting with @fill_only have
anything to do with "needs-filling"?

After all, that 'age' check actually wants to fill 'age' and 'age_string' in the project. Even if some other codepath starts filling 'age' in the project with a later change, the current callers of fill_project_list_info expects _both_ to be filled. So "I know the current implementation fills both at the same time, so checking 'age' alone is sufficient" is not an answer that shows good taste in the API design.