Re: [PATCHv2 2/8] gitweb: Option for filling only specified info in fill_project_list_info

[Jakub Narebski <jnareb@gmail.com>]

On Mon, 20 Feb 2012, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > Introduce project_info_needs_filling($pr, $key[, \%fill_only]), which
> 
> That's "project_info_needs_filling($pr, @key[, \%fill_only])", isn't it?

Yes it is.  I'm sorry, I have missed that this needs fixing from the
first version of patch series.  Since then I have noticed that in few
cases we fill two fields at once, so we have to ask about two fields
at once as well.

> > +# entry for given @keys needs filling if at least one of interesting keys
> > +# in list is not present in %$project_info; key is interesting if $fill_only
> > +# is not passed, or is empty (all keys are interesting in both of those cases),
> > +# or if key is in $fill_only hash
> > +#
> > +# USAGE:
> > +# * project_info_needs_filling($project_info, 'key', ...)
> > +# * project_info_needs_filling($project_info, 'key', ..., \%fill_only)
> > +#   where %fill_only = map { $_ => 1 } @fill_only;
> 
> The reason this spells bad to me is that it gives the readers (and anybody
> who may want to touch gitweb code) that the caller has _two_ ways to say
> the same thing.
> 
> If a caller is interested in finding out $key in $project_info, it can
> either (1) pass $key as one of the earlier parameters to the function and
> not pass any \%fill_only, or (2) pass $key and pass \%fill_only but after
> making sure \%fill_only also has $key defined.
> 
> And if the caller passes \%fill_only, it has to remember that it needs to
> add $key to it; otherwise the earlier request "I want $key" is ignored by
> the function.

Yes, you are right.  All of this is caused by fill_project_list_info()
trying to do *two* things, instead of doing one thing and doing it well,
as is Unix philosophy.  It tried to check both that key is needed and
that key is wanted.

> Why is it a good thing to have such a convoluted API?

Thanks for a sanity check.

> Is it that "fill_project_list_info" is called by multiple callers, and
> they do not necessarily need to see all the fields that can be filled by
> the function (namely, age, age_string, descr, descr_long, owner, ctags and
> category)?  If that is the case, I must say that the approach to put the
> set filtering logic inside project_info_needs_filling function smells like
> a bad API design.

You are right.

> In other words, wouldn't a callchain that uses a more natural set of API
> functions be like this?
> 
> 	# the top-level caller that is interested only in these two fields
> 	fill_project_list_info($projlist, 'age', 'owner');
> 
> 	# in fill_project_list_info()
> 	my $projlist = shift;
> 	my %wanted = map { $_ => 1 } @_;
> 
> 	foreach my $pr (@$projlist) {
> 		if (project_info_needs_filling($pr,
> 			filter_set(\%wanted, 'age', 'age_string'))) {

That or

 		if (project_info_needs_filling($pr,  'age', 'age_string') &&
 		    project_info_is_wanted(\%wanted, 'age', 'age_string')) {

and in your case there is no duplication of field names.

filter_set() is simply intersection of two sets, but it treats empty
%wanted as a full set.

> 			... get @activity ...
>                         ($pr->{'age'}, $pr->{'age_string'}) = @activity;
> 		}
> 		...
> 	}
> 
> That way, "needs_filling" has to do one and only one thing well: it checks
> if any of the fields it was asked is missing and answers.  And a generic
> set filtering helper that takes a filtering hashref and an array can be
> used later outside of the "needs_filling" function.

Right.

> >  sub project_info_needs_filling {
> > +	my $fill_only = ref($_[-1]) ? pop : undef;
> >  	my ($project_info, @keys) = @_;
> >  
> >  	# return List::MoreUtils::any { !exists $project_info->{$_} } @keys;
> >  	foreach my $key (@keys) {
> > -		if (!exists $project_info->{$key}) {
> > +		if ((!$fill_only || !%$fill_only || $fill_only->{$key}) &&
> > +		    !exists $project_info->{$key}) {
> >  			return 1;
> >  		}
> >  	}
> >  	return;
> >  }
> >  
> > -
> 
> Shouldn't the previous patch updated not to add this blank line in the
> first place?

O.K.

> >  # fills project list info (age, description, owner, category, forks)
> 
> Is this enumeration up-to-date?  If we cannot keep it up-to-date, it may
> make sense to show only a few as examples and add ellipses.

O.K.

> >  # 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').
> 
> It is unclear what the added clause wants to say; especially, the link
> between the mention of 'age' and 'only when' is unclear.  Is asking of
> 'age' what makes a request a notable exception to remove invalid projects?
> Or is asking to partially fill fields what triggers the removal of invalid
> projects, and you mentioned 'age' as an example?
> 
> If it is the former (I am guessing it is), it would be much clearer to
> say:
> 
>     Invalid projects are removed from the returned list if and only if you
>     ask 'age' to be filled, because of such and such reasons.
 
O.K.


Sidenote: the reason is that to check that project is valid you really
have to run git command that requires repository in it[1].  Only when
filling 'age' and 'age_string' using git_get_last_activity() we always
run git command.

[1] Gitweb actually uses --git-dir=<PATH> parameter to git wrapper.

-- 
Jakub Narebski
Poland