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

[Junio C Hamano <gitster@pobox.com>]

Jakub Narebski <jnareb@gmail.com> writes:

> On Fri, 10 Feb 2012, Junio C Hamano wrote:
> ...
>> 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"?
>
> project_info_needs_filling() in absence of @fill_only is just a thin
> wrapper around "!defined $pr->{$key}", it checks for each key if it needs
> to be filled.
>
> It is used like this
>
>   if (project_info_needs_filled("A", "A, B, C")) {
>      fill A
>   }
>   if (project_info_needs_filled("B", "A, B, C")) {
>      fill B
>   }
>   ...
>  
>> 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.
>
> It is not as much matter of API, as the use of checks in loop in 
> fill_project_list_info().
>
> What is now
>
>   my (@activity) = git_get_last_activity($pr->{'path'});
>   unless (@activity) {
>   	next PROJECT;
>   }
>   ($pr->{'age'}, $pr->{'age_string'}) = @activity;
>
> should be
>
>   if (!defined $pr->{'age'} ||
>       !defined $pr->{'age_string'}) {
>   	my (@activity) = git_get_last_activity($pr->{'path'});
>   	unless (@activity) {
>   		next PROJECT;
>   	}
>   	($pr->{'age'}, $pr->{'age_string'}) = @activity;
>   }

Huh?  Compare that with what you wrote above "It is used like this".  This
is *NOT* using the API like that.  The caller knows it wants both age and
age-string, and if even one of them is missing, do the work to fill both.

So why isn't the info-needs-filled API not checking _both_ with a single
call?  It is only because you designed the API to accept only a single $key
instead of list of "here are what I care about".