On Mon, 20 Feb 2012, Junio C Hamano wrote:
> Jakub Narebski <jnareb@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html