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

[Jakub Narebski <jnareb@gmail.com>]

On Thu, 9 Feb 2012, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > This could have been squashed with the next commit, but this way it is
> > pure refactoring that shouldn't change gitweb behavior.
> 
> It is not obvious why this shouldn't change gitweb behaviour from the
> patch text.
> 
> The old code says "Unconditionally call git_get_last_activity(), and if we
> found nothing, do not do anything extra for this project". The new code
> says "we do that but only if the project does not know its 'age' yet".

Well, till next patch in this series gitweb always filled some parts of
project info by processing $projects_list i.e. reading a file or scanning
a directory.  Then fill_project_list_info was called to fill the rest
of projects data.

Which parts were filled in first step depends on whether $projects_list
is a file with list of projects or a directory to scan, but 'age' was 
_never_ filled.

But I agree that it is not obvious.


I think that better solution would be to either squash 1/5 and 2/5, or
make 1/5 only about introduction of project_info_needs_filling(), without
@fill_only but with the 'age' check change.

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

fill_project_list_info() is, at least after a fix with 'age', about filling
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.
 
> > Adding project_info_needs_filling() subroutine could have been split
> > into separate commit, but it would be subroutine without use...
> >
> >  gitweb/gitweb.perl |   41 +++++++++++++++++++++++++++++++----------
> >  1 files changed, 31 insertions(+), 10 deletions(-)
> >
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index 913a463..b7a3752 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -5185,35 +5185,56 @@ sub git_project_search_form {
> >  	print "</div>\n";
> >  }
> >  
> > +# entry for given $key doesn't need filling if either $key already exists
> > +# in $project_info hash, or we are interested only in subset of keys
> > +# and given key is not among @fill_only.
> > +sub project_info_needs_filling {
> > +	my ($project_info, $key, @fill_only) = @_;

So in new version @fill_only will be removed...

> > +
> > +	if (!@fill_only ||            # we are interested in everything
> > +	    grep { $key eq $_ } @fill_only) { # or key is in @fill_only
> > +		# check if key is already filled
> > +		return !exists $project_info->{$key};

...and this will be the only line of project_info_needs_filling() wrapper.

> > +	}
> > +	# uninteresting key, outside @fill_only
> > +	return 0;
> > +}
> > +
> >  # fills project list info (age, description, owner, category, forks)
> >  # for each project in the list, removing invalid projects from
> > -# returned list
> > +# returned list, or fill only specified info (removing invalid projects
> > +# only when filling 'age').
> > +#
> >  # NOTE: modifies $projlist, but does not remove entries from it
> >  sub fill_project_list_info {
> > -	my $projlist = shift;
> > +	my ($projlist, @fill_only) = @_;

Here also @fill_only would be not present.

> >  	my @projects;
> >  
> >  	my $show_ctags = gitweb_check_feature('ctags');
> >   PROJECT:
> >  	foreach my $pr (@$projlist) {
> > -		my (@activity) = git_get_last_activity($pr->{'path'});
> > -		unless (@activity) {
> > -			next PROJECT;
> > +		if (project_info_needs_filling($pr, 'age', @fill_only)) {

This change makes 'age' not special.

[...]

-- 
Jakub Narebski
Poland