[PATCH 0/5] gitweb: Faster and imrpoved project search

[PATCH 0/5] gitweb: Faster and imrpoved project search

[Jakub Narebski]

This patch series, though independent, is best applied on top of
"[PATCH 0/2] gitweb: Project search improvements" series, or in other
words on top of 'bl/gitweb-project-filter' branch.

First two patches in this series are about speeding up project search
(and perhaps in the future also project pagination).  Those two could
be squashed together.

Next two patches are about making it more visible what are we
searching for, or rather what was matched (important especially with
regexp match).  The last patch is speculative patch about showing
match using shortened description.

Jakub Narebski (5):
  gitweb: Option for filling only specified info in
    fill_project_list_info
  gitweb: Faster project search
  gitweb: Highlight matched part of project name when searching
    projects
  gitweb: Highlight matched part of project description when searching
    projects
  gitweb: Highlight matched part of shortened project description

 gitweb/gitweb.perl |  122 ++++++++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 108 insertions(+), 14 deletions(-)

-- 
1.7.9

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

[Jakub Narebski]

Introduce project_info_needs_filling($pr, $key[, @fill_only]), which
is now used in place of simple 'defined $pr->{$key}' to check if
specific slot in project needs to be filled.

This is in preparation of future lazy filling of project info in
project search and pagination of sorted list of projects.  The only
functional change is that fill_project_list_info() now checks if 'age'
is already filled before running git_get_last_activity().

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This could have been squashed with the next commit, but this way it is
pure refactoring that shouldn't change gitweb behavior.

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);
-- 
1.7.9

[PATCH 2/5] gitweb: Faster project search

[Jakub Narebski]

Before searching by some field the information we search for must be
filled in.  For this fill_project_list_info() was enhanced in previous
commit to take additional parameters which part of projects info to
fill.  This way we can limit doing expensive calculations (like
running git-for-each-ref to get 'age' / "Last changed" info) only to
projects which we will show as search results.

With this commit the number of git commands used to generate search
results is 2*<matched projects> + 1, and depends on number of matched
projects rather than number of all projects (all repositories).

Note: this is 'git for-each-ref' to find last activity, and 'git config'
for each project, and 'git --version' once.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
search_projects_list() now pre-fills required parts of project info by
itself, so running fill_project_list_info() before calling it is no
longer necessary and actually you should not do it.

 gitweb/gitweb.perl |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index b7a3752..95ca00f 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2989,6 +2989,10 @@ sub search_projects_list {
 	return @$projlist
 		unless ($tagfilter || $searchtext);
 
+	# searching projects require filling to be run before it;
+	fill_project_list_info($projlist,
+	                       $tagfilter  ? 'ctags' : (),
+	                       $searchtext ? ('path', 'descr') : ());
 	my @projects;
  PROJECT:
 	foreach my $pr (@$projlist) {
@@ -5370,12 +5374,13 @@ sub git_project_list_body {
 	# filtering out forks before filling info allows to do less work
 	@projects = filter_forks_from_projects_list(\@projects)
 		if ($check_forks);
-	@projects = fill_project_list_info(\@projects);
-	# searching projects require filling to be run before it
+	# search_projects_list pre-fills required info
 	@projects = search_projects_list(\@projects,
 	                                 'searchtext' => $searchtext,
 	                                 'tagfilter'  => $tagfilter)
 		if ($tagfilter || $searchtext);
+	# fill the rest
+	@projects = fill_project_list_info(\@projects);
 
 	$order ||= $default_projects_order;
 	$from = 0 unless defined $from;
-- 
1.7.9

[PATCH 3/5] gitweb: Highlight matched part of project name when searching projects

[Jakub Narebski]

Use newly introduced esc_html_match_hl() to escape HTML and mark match
with span element with 'match' class.  Currently only 'path' part
(i.e. project name) is highlighted; match might be on the project
description.

The code makes use of the fact that defined $search_regexp means that
there was search going on.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Introducing esc_html_match_hl() could have been split into a separate
commit, but it would be subroutine without any use.

 gitweb/gitweb.perl |   28 +++++++++++++++++++++++++++-
 1 files changed, 27 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 95ca00f..aef15c8 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1715,6 +1715,30 @@ sub chop_and_escape_str {
 	}
 }
 
+# highlight match (if any), and escape HTML
+sub esc_html_match_hl {
+	my ($str, $regexp) = @_;
+	return esc_html($str) unless defined $regexp;
+
+	my @matches;
+	while ($str =~ /$regexp/g) {
+		push @matches, [$-[0], $+[0]];
+	}
+	return esc_html($str) unless @matches;
+
+	my $out = '';
+	my $pos = 0;
+	for my $m (@matches) {
+		$out .= esc_html(substr $str, $pos, $m->[0] - $pos);
+		$out .= $cgi->span({-class => 'match'},
+		                   esc_html(substr $str, $m->[0], $m->[1] - $m->[0]));
+		$pos = $m->[1];
+	}
+	$out .= esc_html(substr $str, $pos);
+
+	return $out;
+}
+
 ## ----------------------------------------------------------------------
 ## functions returning short strings
 
@@ -5342,7 +5366,9 @@ sub git_project_list_rows {
 			print "</td>\n";
 		}
 		print "<td>" . $cgi->a({-href => href(project=>$pr->{'path'}, action=>"summary"),
-		                        -class => "list"}, esc_html($pr->{'path'})) . "</td>\n" .
+		                        -class => "list"},
+		                       esc_html_match_hl($pr->{'path'}, $search_regexp)) .
+		      "</td>\n" .
 		      "<td>" . $cgi->a({-href => href(project=>$pr->{'path'}, action=>"summary"),
 		                        -class => "list", -title => $pr->{'descr_long'}},
 		                        esc_html($pr->{'descr'})) . "</td>\n" .
-- 
1.7.9

[PATCH 4/5] gitweb: Highlight matched part of project description when searching projects

[Jakub Narebski]

Use esc_html_match_hl() from previous commit to mark match in the
_whole_ description when searching projects.

Currently, with this commit, when searching projects there is always
shown full description of a project, and not a shortened one (like for
ordinary projects list view), even if the match is on project name and
not project description.

Showing full description when there is match on it is useful to avoid
situation where match is in shortened, invisible part... well, perhaps
that could be solved (showing shortened description), but it would
require some extra code.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
The part about showing match using shortened description no longer
applies after the following patch... though it is an RFC for now.

 gitweb/gitweb.perl |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index aef15c8..c650268 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5371,7 +5371,10 @@ sub git_project_list_rows {
 		      "</td>\n" .
 		      "<td>" . $cgi->a({-href => href(project=>$pr->{'path'}, action=>"summary"),
 		                        -class => "list", -title => $pr->{'descr_long'}},
-		                        esc_html($pr->{'descr'})) . "</td>\n" .
+		                        $search_regexp
+		                        ? esc_html_match_hl($pr->{'descr_long'}, $search_regexp)
+		                        : esc_html($pr->{'descr'})) .
+		      "</td>\n" .
 		      "<td><i>" . chop_and_escape_str($pr->{'owner'}, 15) . "</i></td>\n";
 		print "<td class=\"". age_class($pr->{'age'}) . "\">" .
 		      (defined $pr->{'age_string'} ? $pr->{'age_string'} : "No commits") . "</td>\n" .
-- 
1.7.9

[PATCH/RFC 5/5] gitweb: Highlight matched part of shortened project description

[Jakub Narebski]

Previous commit make gitweb use esc_html_match_hl() to mark match in
the _whole_ description of a project when searching projects.

This commit makes gitweb highlight match in _shortened_ description,
based on match in whole description, using esc_html_match_hl_chopped()
subroutine (with some code duplication with esc_html_match_hl()).
If match is in shortened part, then trailing "... " is highlighted.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This is an RFC because of code duplication between esc_html_match_hl()
and esc_html_match_hl_chopped().

 gitweb/gitweb.perl |   41 ++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 40 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index c650268..174b4d2 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1739,6 +1739,44 @@ sub esc_html_match_hl {
 	return $out;
 }
 
+# highlight match (if any) of shortened string, and escape HTML
+sub esc_html_match_hl_chopped {
+	my ($str, $chopped, $regexp) = @_;
+	return esc_html($chopped) unless defined $regexp;
+	return esc_html_match_hl($str, $regexp) if ($str eq $chopped);
+
+	my @matches;
+	while ($str =~ /$regexp/g) {
+		push @matches, [$-[0], $+[0]];
+	}
+	return esc_html($chopped) unless @matches;
+
+	my $tail = "... ";
+	$chopped =~ s/\Q$tail\E$//; # see chop_str
+	my $len = length($chopped);
+	my $out = '';
+	my $pos = 0;
+	for my $m (@matches) {
+		if ($m->[0] > $len) {
+			$tail = $cgi->span({-class => 'match'}, $tail);
+			last;
+		}
+		$out .= esc_html(substr $str, $pos, $m->[0] - $pos);
+		$out .= $cgi->span({-class => 'match'},
+		                   esc_html(substr $chopped, $m->[0],
+		                            ($m->[1] > $len ? $len : $m->[1]) - $m->[0]));
+		if ($m->[1] > $len) {
+			$tail = $cgi->span({-class => 'match'}, $tail);
+			$pos = $len;
+			last;
+		}
+		$pos = $m->[1];
+	}
+	$out .= esc_html(substr($chopped, $pos)).$tail;
+
+	return $out;
+}
+
 ## ----------------------------------------------------------------------
 ## functions returning short strings
 
@@ -5372,7 +5410,8 @@ sub git_project_list_rows {
 		      "<td>" . $cgi->a({-href => href(project=>$pr->{'path'}, action=>"summary"),
 		                        -class => "list", -title => $pr->{'descr_long'}},
 		                        $search_regexp
-		                        ? esc_html_match_hl($pr->{'descr_long'}, $search_regexp)
+		                        ? esc_html_match_hl_chopped($pr->{'descr_long'},
+		                                                    $pr->{'descr'}, $search_regexp)
 		                        : esc_html($pr->{'descr'})) .
 		      "</td>\n" .
 		      "<td><i>" . chop_and_escape_str($pr->{'owner'}, 15) . "</i></td>\n";
-- 
1.7.9

[PATCH/RFCv2 5/5] gitweb: Highlight matched part of shortened project description

[Jakub Narebski]

Previous commit make gitweb use esc_html_match_hl() to mark match in
the _whole_ description of a project when searching projects.

This commit makes gitweb highlight match in _shortened_ description,
based on match in whole description, using esc_html_match_hl_chopped()
subroutine.

If match is in removed (chopped) part, even partially, then trailing
"... " is highlighted.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This version removes code duplication from v1.

It is still marked as RFC, because I am not sure if it is right way
to highlight match in shortened string, or if we better use full string,
or full string if match is in chopped part.

 gitweb/gitweb.perl |   40 ++++++++++++++++++++++++++++++++++++++--
 1 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index c650268..f71afe0 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1720,11 +1720,46 @@ sub esc_html_match_hl {
 	my ($str, $regexp) = @_;
 	return esc_html($str) unless defined $regexp;
 
+	return esc_html_match_hl_chopped($str, undef, $regexp);
+}
+
+
+# highlight match (if any) of shortened string, and escape HTML
+sub esc_html_match_hl_chopped {
+	my ($str, $chopped, $regexp) = @_;
+	return esc_html($chopped) unless defined $regexp;
+
 	my @matches;
 	while ($str =~ /$regexp/g) {
 		push @matches, [$-[0], $+[0]];
 	}
-	return esc_html($str) unless @matches;
+	return esc_html(defined $chopped ? $chopped : $str) unless @matches;
+
+	# filter matches so that we mark chopped string, if it is present
+	if (defined $chopped) {
+		my $tail = "... "; # see chop_str
+		unless ($chopped =~ s/\Q$tail\E$//) {
+			$tail = '';
+		}
+		my $chop_len = length($chopped);
+		my $tail_len = length($tail);
+		my @filtered;
+
+		for my $m (@matches) {
+			if ($m->[0] > $chop_len) {
+				push @filtered, [ $chop_len, $chop_len + $tail_len ] if ($tail_len > 0);
+				last;
+			} elsif ($m->[1] > $chop_len) {
+				push @filtered, [ $m->[0], $chop_len + $tail_len ];
+				last;
+			}
+			push @filtered, $m;
+		}
+
+		# further operations are on chopped string
+		$str = $chopped . $tail;
+		@matches = @filtered;
+	}
 
 	my $out = '';
 	my $pos = 0;
@@ -5372,7 +5407,8 @@ sub git_project_list_rows {
 		      "<td>" . $cgi->a({-href => href(project=>$pr->{'path'}, action=>"summary"),
 		                        -class => "list", -title => $pr->{'descr_long'}},
 		                        $search_regexp
-		                        ? esc_html_match_hl($pr->{'descr_long'}, $search_regexp)
+		                        ? esc_html_match_hl_chopped($pr->{'descr_long'},
+		                                                    $pr->{'descr'}, $search_regexp)
 		                        : esc_html($pr->{'descr'})) .
 		      "</td>\n" .
 		      "<td><i>" . chop_and_escape_str($pr->{'owner'}, 15) . "</i></td>\n";
-- 
1.7.9

Re: [PATCH 0/5] gitweb: Faster and imrpoved project search

[Felipe Contreras]

On Sat, Feb 4, 2012 at 2:47 PM, Jakub Narebski <jnareb@gmail.com> wrote:

Typo: improved

-- 
Felipe Contreras

Re: [PATCH 0/5] gitweb: Faster and imrpoved project search

[Jakub Narebski]

On Sat, 4 Feb 2012, Felipe Contreras wrote:
> 
> Typo: improved

Yeah, I have noticed this just as I have send it, but because it is in 
_cover letter_ rather than in commit message...

-- 
Jakub Narebski
Poland

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

[Junio C Hamano]

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.

> 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);

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

[Jakub Narebski]

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

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

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

That still does not answer the fundamental issues I had with the presented
API: why does it take only a single $key (please re-read my "A, B and C"
example), and what does that single $key intersecting with @fill_only have
anything to do with "needs-filling"?

After all, that 'age' check actually wants to fill 'age' and 'age_string'
in the project. Even if some other codepath starts filling 'age' in the
project with a later change, the current callers of fill_project_list_info
expects _both_ to be filled. So "I know the current implementation fills
both at the same time, so checking 'age' alone is sufficient" is not an
answer that shows good taste in the API design.

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

[Jakub Narebski]

On Fri, 10 Feb 2012, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>>> 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.
>>
>> ...
>> 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.
> 
> That still does not answer the fundamental issues I had with the presented
> API: why does it take only a single $key (please re-read my "A, B and C"
> example), and what does that single $key intersecting with @fill_only have
> anything to do with "needs-filling"?

project_info_needs_filling() in absence of @fill_only is just a thin
wrapper around "!defined $pr->{$key}", it checks for each key if it needs
to be filled.

It is used like this

  if (project_info_needs_filled("A", "A, B, C")) {
     fill A
  }
  if (project_info_needs_filled("B", "A, B, C")) {
     fill B
  }
  ...
 
> After all, that 'age' check actually wants to fill 'age' and 'age_string'
> in the project. Even if some other codepath starts filling 'age' in the
> project with a later change, the current callers of fill_project_list_info
> expects _both_ to be filled. So "I know the current implementation fills
> both at the same time, so checking 'age' alone is sufficient" is not an
> answer that shows good taste in the API design.

It is not as much matter of API, as the use of checks in loop in 
fill_project_list_info().

What is now

  my (@activity) = git_get_last_activity($pr->{'path'});
  unless (@activity) {
  	next PROJECT;
  }
  ($pr->{'age'}, $pr->{'age_string'}) = @activity;

should be

  if (!defined $pr->{'age'} ||
      !defined $pr->{'age_string'}) {
  	my (@activity) = git_get_last_activity($pr->{'path'});
  	unless (@activity) {
  		next PROJECT;
  	}
  	($pr->{'age'}, $pr->{'age_string'}) = @activity;
  }

which would translate to

  if (project_info_needs_filled($pr, 'age') ||
      project_info_needs_filled($pr, 'age_string') {
  	my (@activity) = git_get_last_activity($pr->{'path'});
  	unless (@activity) {
  		next PROJECT;
  	}
  	($pr->{'age'}, $pr->{'age_string'}) = @activity;
  }

and then with @fill_only

  if (project_info_needs_filled($pr, 'age', @fill_only) ||
      project_info_needs_filled($pr, 'age_string', @fill_only) {
  	my (@activity) = git_get_last_activity($pr->{'path'});
  	unless (@activity) {
  		next PROJECT;
  	}
  	($pr->{'age'}, $pr->{'age_string'}) = @activity;
  }

The same should be done for 'descr_long' and 'descr' which are also
always filled together.

-- 
Jakub Narebski
Poland

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> On Fri, 10 Feb 2012, Junio C Hamano wrote:
> ...
>> That still does not answer the fundamental issues I had with the presented
>> API: why does it take only a single $key (please re-read my "A, B and C"
>> example), and what does that single $key intersecting with @fill_only have
>> anything to do with "needs-filling"?
>
> project_info_needs_filling() in absence of @fill_only is just a thin
> wrapper around "!defined $pr->{$key}", it checks for each key if it needs
> to be filled.
>
> It is used like this
>
>   if (project_info_needs_filled("A", "A, B, C")) {
>      fill A
>   }
>   if (project_info_needs_filled("B", "A, B, C")) {
>      fill B
>   }
>   ...
>  
>> After all, that 'age' check actually wants to fill 'age' and 'age_string'
>> in the project. Even if some other codepath starts filling 'age' in the
>> project with a later change, the current callers of fill_project_list_info
>> expects _both_ to be filled. So "I know the current implementation fills
>> both at the same time, so checking 'age' alone is sufficient" is not an
>> answer that shows good taste in the API design.
>
> It is not as much matter of API, as the use of checks in loop in 
> fill_project_list_info().
>
> What is now
>
>   my (@activity) = git_get_last_activity($pr->{'path'});
>   unless (@activity) {
>   	next PROJECT;
>   }
>   ($pr->{'age'}, $pr->{'age_string'}) = @activity;
>
> should be
>
>   if (!defined $pr->{'age'} ||
>       !defined $pr->{'age_string'}) {
>   	my (@activity) = git_get_last_activity($pr->{'path'});
>   	unless (@activity) {
>   		next PROJECT;
>   	}
>   	($pr->{'age'}, $pr->{'age_string'}) = @activity;
>   }

Huh?  Compare that with what you wrote above "It is used like this".  This
is *NOT* using the API like that.  The caller knows it wants both age and
age-string, and if even one of them is missing, do the work to fill both.

So why isn't the info-needs-filled API not checking _both_ with a single
call?  It is only because you designed the API to accept only a single $key
instead of list of "here are what I care about".

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

[Jakub Narebski]

On Fri, 10 Feb 2012, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>> On Fri, 10 Feb 2012, Junio C Hamano wrote:
>> ...
>>> That still does not answer the fundamental issues I had with the presented
>>> API: why does it take only a single $key (please re-read my "A, B and C"
>>> example), and what does that single $key intersecting with @fill_only have
>>> anything to do with "needs-filling"?
>>
>> project_info_needs_filling() in absence of @fill_only is just a thin
>> wrapper around "!defined $pr->{$key}", it checks for each key if it needs
>> to be filled.
>>
>> It is used like this
>>
>>   if (project_info_needs_filled("A", "A, B, C")) {
>>      fill A
>>   }
>>   if (project_info_needs_filled("B", "A, B, C")) {
>>      fill B
>>   }
>>   ...
>>  
>>> After all, that 'age' check actually wants to fill 'age' and 'age_string'
>>> in the project. [...]

Thanks for noticing that... though I think more important is that some
further command would mark only 'age_string' as needed, and 
fill_project_list_info() wouldn't notice that it needs to run
git_get_last_activity() if it checks only 'age'.

>> It is not as much matter of API, as the use of checks in loop in 
>> fill_project_list_info().
>>
>> What is now
>>
>>   my (@activity) = git_get_last_activity($pr->{'path'});
>>   unless (@activity) {
>>   	next PROJECT;
>>   }
>>   ($pr->{'age'}, $pr->{'age_string'}) = @activity;
>>
>> should be
>>
>>   if (!defined $pr->{'age'} ||
>>       !defined $pr->{'age_string'}) {
>>   	my (@activity) = git_get_last_activity($pr->{'path'});
>>   	unless (@activity) {
>>   		next PROJECT;
>>   	}
>>   	($pr->{'age'}, $pr->{'age_string'}) = @activity;
>>   }
> 
> Huh?  Compare that with what you wrote above "It is used like this".  This
> is *NOT* using the API like that.  The caller knows it wants both age and
> age-string, and if even one of them is missing, do the work to fill both.
> 
> So why isn't the info-needs-filled API not checking _both_ with a single
> call?  It is only because you designed the API to accept only a single $key
> instead of list of "here are what I care about".

Passing two lists as parameters is a bit harder and makes for more
complicated API. 

So what you mean is that instead of proposed

  if (project_info_needs_filled($pr, 'age', @fill_only) ||
      project_info_needs_filled($pr, 'age_string', @fill_only) {

it should be

  if (project_info_needs_filled($pr, 'age', 'age_string', \@fill_only) {

or

  if (project_info_needs_filled($pr, ['age', 'age_string'], @fill_only) {

(with either "..., 'owner', ..." or "..., [ 'owner' ], ..." for single-key
filling), or

  if (project_info_needs_filled($pr, ['age', 'age_string'], \@fill_only) {

Is it?

-- 
Jakub Narebski
Poland

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> (with either "..., 'owner', ..." or "..., [ 'owner' ], ..." for single-key
> filling), or
>
>   if (project_info_needs_filled($pr, ['age', 'age_string'], \@fill_only) {
>
> Is it?

Whatever. I am not sure what @fill_only is needed for, if the name stands
for "only fill these fields, if this argument is empty".  After all,
doesn't the above example callsite, without ", \@fill_only" at the end,
say "I am going to use age and age_string, so these two need to be filled"
already?

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

[Jakub Narebski]

On Fri, 10 Feb 2012, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > (with either "..., 'owner', ..." or "..., [ 'owner' ], ..." for single-key
> > filling), or
> >
> >   if (project_info_needs_filled($pr, ['age', 'age_string'], \@fill_only) {
> >
> > Is it?
> 
> Whatever. I am not sure what @fill_only is needed for, if the name stands
> for "only fill these fields, if this argument is empty".  After all,
> doesn't the above example callsite, without ", \@fill_only" at the end,
> say "I am going to use age and age_string, so these two need to be filled"
> already?

No, the above example callsite says "Do I need to fill either 'age' or
'age_string'", and is followed by actually filling this info.

Currently the code flow WRT. project information goes like this:

  git_get_projects_list()
  # this fills some of project info; what is filled depends on whether
  # $projects_list is a file with list of project or directory to scan
  filter_forks_from_projects_list()
  # this does not use project info
  fill_project_list_info()
  # this fills all the rest of project info on reduced list
  search_projects_list(<search data>)
  # this uses some of project info

After 2/5 the code goes like this:

  git_get_projects_list()
  # this fills some of project info; what is filled depends on whether
  # $projects_list is a file with list of project or directory to scan
  filter_forks_from_projects_list()
  # this does not use project info
  fill_project_list_info(<search data>)
  # this fills the rest of project info required for search on reduced list
  search_projects_list(<search data>)
  # this uses some of project info
  fill_project_list_info()
  # this fills all the rest of project info on further reduced list

If @fill_only is empty, it means for fill_project_list_info to fill
all the data, if it is not empty it means that those fields needs to
be filled.

The code of fill_project_list_info goes like this

  if (do we need to fill 'age' or 'age_string'?) {
    fill 'age' and 'age_string'
  }
  if (do we need to fill 'desc_long' or 'descr'?) {
    fill 'descr_long' and 'descr'
  }
  if (we are interested in 'ctags' &&
      do we need to fill 'ctags'?) {
    fill 'ctags'
  }
  ...


-- 
Jakub Narebski
Poland

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> If @fill_only is empty, it means for fill_project_list_info to fill
> all the data, if it is not empty it means that those fields needs to
> be filled.

That is exactly what I am questioning.  Do you need "I need all these
fields to be present" and "I will fill these other fields" (which is what
@fill_only is about, no?) that is different from the former?

> The code of fill_project_list_info goes like this
>
>   if (do we need to fill 'age' or 'age_string'?) {
>     fill 'age' and 'age_string'
>   }
>   if (do we need to fill 'desc_long' or 'descr'?) {
>     fill 'descr_long' and 'descr'
>   }
>   if (we are interested in 'ctags' &&
>       do we need to fill 'ctags'?) {
>     fill 'ctags'
>   }
>   ...

Exactly.  Why do you need @fill_only at all?  If you are interested in
ctags and you want to make sure ctags is available, the question you want
to ask the helper function is "Does the project structure already have
ctags field?".  Why does the helper function needs to know anything else?

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

[Jakub Narebski]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > If @fill_only is empty, it means for fill_project_list_info to fill
> > all the data, if it is not empty it means that those fields needs to
> > be filled.
> 
> That is exactly what I am questioning.  Do you need "I need all these
> fields to be present" and "I will fill these other fields" (which is what
> @fill_only is about, no?) that is different from the former?

"I need all these fields" is a property of callsite.  fill_project_list_info()
will be called more than one time in 2/5 to incrementally complete project
info, see below.
 
"I will fill those fields" is a property of piece of code protected by
a conditional inside fill_project_list_info().

> > The code of fill_project_list_info goes like this
> >
> >   if (do we need to fill 'age' or 'age_string'?) {
> >     fill 'age' and 'age_string'
> >   }
> >   if (do we need to fill 'desc_long' or 'descr'?) {
> >     fill 'descr_long' and 'descr'
> >   }
> >   if (we are interested in 'ctags' &&
> >       do we need to fill 'ctags'?) {
> >     fill 'ctags'
> >   }
> >   ...
> 
> Exactly.  Why do you need @fill_only at all?  If you are interested in
> ctags and you want to make sure ctags is available, the question you want
> to ask the helper function is "Does the project structure already have
> ctags field?".  Why does the helper function needs to know anything else?

It is to support incremental filling of project info.  The code is to
go like this:

  create
  filter
  fill part
  filter
  fill rest

We need @fill_only for the "fill part".  As filling project info is
potentially expensive (especially the 'age' field), doing it on narrowed
(filtered) list of project is a performance win.  That is what 2/5 is
about.

-- 
Jakub Narebski
Poland

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

>> Exactly.  Why do you need @fill_only at all?  If you are interested in
>> ctags and you want to make sure ctags is available, the question you want
>> to ask the helper function is "Does the project structure already have
>> ctags field?".  Why does the helper function needs to know anything else?
>
> It is to support incremental filling of project info.  The code is to
> go like this:
>
>   create
>   filter
>   fill part
>   filter
>   fill rest
>
> We need @fill_only for the "fill part".

Again, why?

> As filling project info is
> potentially expensive (especially the 'age' field),

So you wouldn't say "I am interested in 'age' field" but show interest in,
and fill, cheaper fields in the earlier "fill" calls, and then...

> doing it on narrowed
> (filtered) list of project is a performance win.

... you drop uninteresting projects by using the partially filled
information, and show interest in more expensive 'age' in the later round
for surviving projects.

It still does not explain why you need @fill_only.
Hrm...

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

[Jakub Narebski]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>>> Exactly.  Why do you need @fill_only at all?  If you are interested in
>>> ctags and you want to make sure ctags is available, the question you want
>>> to ask the helper function is "Does the project structure already have
>>> ctags field?".  Why does the helper function needs to know anything else?
>>
>> It is to support incremental filling of project info.  The code is to
>> go like this:
>>
>>   create
>>   filter
>>   fill part
>>   filter
>>   fill rest
>>
>> We need @fill_only for the "fill part".
> 
> Again, why?

So fill_project_list_info() knows what needs to be filled (notice: not
"what to fill"), as filter might need different fields in project info
to do its work.
 
> > As filling project info is
> > potentially expensive (especially the 'age' field),
> 
> So you wouldn't say "I am interested in 'age' field" but show interest in,
> and fill, cheaper fields in the earlier "fill" calls, and then...

It is not about cheaper, it is about required by filter.  It happens that
it is cheaper.

> > doing it on narrowed
> > (filtered) list of project is a performance win.
> 
> ... you drop uninteresting projects by using the partially filled
> information, and show interest in more expensive 'age' in the later round
> for surviving projects.
> 
> It still does not explain why you need @fill_only.

So I can use single subroutine fill_project_list_info() to fill what is
required, and to fill the rest of info.

   create
   filter
   fill part('path', 'descr')
   filter('path', 'descr')
   fill rest

or

   create
   filter
   fill part('ctags')
   filter('ctags')
   fill rest

Am I clear?
-- 
Jakub Narebski
Poland