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

From: Jakub Narebski <jnareb@gmail.com>
Date: Feb 10, 2012, 13:56 UTC
Message-ID: <201202101456.45944.jnareb@gmail.com>
In-Reply-To: <7vvcnftvb5.fsf@alter.siamese.dyndns.org>

On Fri, 10 Feb 2012, Junio C Hamano wrote:

Show 24 quoted lines

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

Thanks for noticing that... though I think more important is that some further command would mark only 'age_string' as needed, and fill_project_list_info() wouldn't notice that it needs to run git_get_last_activity() if it checks only 'age'.

Show 29 quoted lines

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

Passing two lists as parameters is a bit harder and makes for more complicated API.

So what you mean is that instead of proposed

  if (project_info_needs_filled($pr, 'age', @fill_only) ||
      project_info_needs_filled($pr, 'age_string', @fill_only) {

it should be

  if (project_info_needs_filled($pr, 'age', 'age_string', \@fill_only) {

or

  if (project_info_needs_filled($pr, ['age', 'age_string'], @fill_only) {

(with either "..., 'owner', ..." or "..., [ 'owner' ], ..." for single-key filling), or

  if (project_info_needs_filled($pr, ['age', 'age_string'], \@fill_only) {

Is it?

-- 
Jakub Narebski
Poland