Re: [PATCH 1/5] gitweb: Option for filling only specified info in fill_project_list_info
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 9, 2012, 22:04 UTC
- Message-ID
- <7vhayzvf38.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1328359648-29511-2-git-send-email-jnareb@gmail.com>
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".
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.
Show 77 quoted lines
> 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);