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

From: Jakub Narebski <jnareb@gmail.com>
Date: Feb 9, 2012, 23:52 UTC
Message-ID: <201202100052.26399.jnareb@gmail.com>
In-Reply-To: <7v4nuzvbnr.fsf@alter.siamese.dyndns.org>

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

Show 31 quoted lines

> Jakub Narebski <jnareb@gmail.com> writes:
> 
>>> 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"?

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

Show 6 quoted lines

> 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;
  }

which would translate to

  if (project_info_needs_filled($pr, 'age') ||
      project_info_needs_filled($pr, 'age_string') {
  	my (@activity) = git_get_last_activity($pr->{'path'});
  	unless (@activity) {
  		next PROJECT;
  	}
  	($pr->{'age'}, $pr->{'age_string'}) = @activity;
  }

and then with @fill_only

  if (project_info_needs_filled($pr, 'age', @fill_only) ||
      project_info_needs_filled($pr, 'age_string', @fill_only) {
  	my (@activity) = git_get_last_activity($pr->{'path'});
  	unless (@activity) {
  		next PROJECT;
  	}
  	($pr->{'age'}, $pr->{'age_string'}) = @activity;
  }

The same should be done for 'descr_long' and 'descr' which are also always filled together.

-- 
Jakub Narebski
Poland