Jakub Narebski <jnareb@xxxxxxxxx> 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".
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.
> 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) = @_;
> +
> + 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};
> + }
> + # 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) = @_;
> 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)) {
> + my (@activity) = git_get_last_activity($pr->{'path'});
> + unless (@activity) {
> + next PROJECT;
> + }
> + ($pr->{'age'}, $pr->{'age_string'}) = @activity;
> }
> - ($pr->{'age'}, $pr->{'age_string'}) = @activity;
> - if (!defined $pr->{'descr'}) {
> + if (project_info_needs_filling($pr, 'descr', @fill_only)) {
> my $descr = git_get_project_description($pr->{'path'}) || "";
> $descr = to_utf8($descr);
> $pr->{'descr_long'} = $descr;
> $pr->{'descr'} = chop_str($descr, $projects_list_description_width, 5);
> }
> - if (!defined $pr->{'owner'}) {
> + if (project_info_needs_filling($pr, 'owner', @fill_only)) {
> $pr->{'owner'} = git_get_project_owner("$pr->{'path'}") || "";
> }
> - if ($show_ctags) {
> + if ($show_ctags &&
> + project_info_needs_filling($pr, 'ctags', @fill_only)) {
> $pr->{'ctags'} = git_get_project_ctags($pr->{'path'});
> }
> - if ($projects_list_group_categories && !defined $pr->{'category'}) {
> + if ($projects_list_group_categories &&
> + project_info_needs_filling($pr, 'category', @fill_only)) {
> my $cat = git_get_project_category($pr->{'path'}) ||
> $project_list_default_category;
> $pr->{'category'} = to_utf8($cat);
--
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