{"thread":{"id":"29638","subject":"[PATCHv2 0/8] gitweb: Faster and improved project search","startedAt":"2012-02-15T20:38:44Z","lastAt":"2012-02-22T22:09:58Z","messageCount":16,"participants":["Jakub Narebski","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"184783","messageId":"1329338332-30358-1-git-send-email-jnareb@gmail.com","threadId":"29638","inReplyTo":null,"subject":"[PATCHv2 0/8] gitweb: Faster and improved project search","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-15T20:38:44Z","receivedAt":"2012-02-15T20:38:44Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"[Cc-ing Junio because of his involvement in discussion about first\n patch in previous version of this series.]\n\nFirst three patches in this series are mainly about speeding up\nproject search (and perhaps in the future also project pagination).\nWell, first one is unification, refactoring and future-proofing.\nThe second and third patch could be squashed together; second adds\n@fill_only, but third actually uses it.\n\nNext set of patches is about highlighting matched part, making it\neasier to recognize why project was selected, what we were searching\nfor (though better page title would also help second issue).\n\nWell, fourth patch (first in set mentioned above) is here for the\ncommit message, otherwise it could have been squashed with next one.\n\nLast patch in this series is beginning of using esc_html_match_hl()\nfor other searches in gitweb -- the easiest part.\n\nJakub Narebski (8):\n gitweb: Refactor checking if part of project info need filling\n gitweb: Option for filling only specified info in\n fill_project_list_info\n gitweb: Faster project search\n gitweb: Introduce esc_html_hl_regions\n gitweb: Highlight matched part of project name when searching\n projects\n gitweb: Highlight matched part of project description when searching\n projects\n gitweb: Highlight matched part of shortened project description\n gitweb: Use esc_html_match_hl() in 'grep' search\n\n gitweb/gitweb.perl | 158 ++++++++++++++++++++++++++++++++++++++++++++--------\n 1 files changed, 135 insertions(+), 23 deletions(-)\n\n-- \n1.7.9\n"},{"id":"184784","messageId":"1329338332-30358-2-git-send-email-jnareb@gmail.com","threadId":"29638","inReplyTo":"1329338332-30358-1-git-send-email-jnareb@gmail.com","subject":"[PATCHv2 1/8] gitweb: Refactor checking if part of project info need filling","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-15T20:38:45Z","receivedAt":"2012-02-15T20:38:45Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Extract the check if given keys (given parts) of project info needs to\nbe filled into project_info_needs_filling() subroutine. It is for now\na thin wrapper around \"!exists $project_info->{$key}\".\n\nNote that \"!defined\" was replaced by more correct \"!exists\".\n\nWhile at it uniquify treating of all project info, adding checks for\n'age' field before running git_get_last_activity(), and also checking\nfor all keys filled in code protected by conditional, and not only\none.\n\nThe code now looks like this\n\n foreach my $project (@$project_list) {\n \tif (given keys need to be filled) {\n \t\tfill given keys\n \t}\n \t...\n }\n\nSigned-off-by: Jakub Narebski \n---\nThis patch is new in this series, and splits off introduction of\nproject_info_needs_filling() function from partial filling via\n@fill_only.\n\nHopefully this makes change more clear.\n\nNote that this change stands alone, regardless of speeding up project\nsearch.\n\n gitweb/gitweb.perl | 35 +++++++++++++++++++++++++++--------\n 1 files changed, 27 insertions(+), 8 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 057ba5b..e62c2ef 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5195,6 +5195,21 @@ sub git_project_search_form {\n \tprint \"\\n\";\n }\n \n+# entry for given @keys needs filling if at least one of keys in list\n+# is not present in %$project_info\n+sub project_info_needs_filling {\n+\tmy ($project_info, @keys) = @_;\n+\n+\t# return List::MoreUtils::any { !exists $project_info->{$_} } @keys;\n+\tforeach my $key (@keys) {\n+\t\tif (!exists $project_info->{$key}) {\n+\t\t\treturn 1;\n+\t\t}\n+\t}\n+\treturn;\n+}\n+\n+\n # fills project list info (age, description, owner, category, forks)\n # for each project in the list, removing invalid projects from\n # returned list\n@@ -5206,24 +5221,28 @@ sub fill_project_list_info {\n \tmy $show_ctags = gitweb_check_feature('ctags');\n PROJECT:\n \tforeach my $pr (@$projlist) {\n-\t\tmy (@activity) = git_get_last_activity($pr->{'path'});\n-\t\tunless (@activity) {\n-\t\t\tnext PROJECT;\n+\t\tif (project_info_needs_filling($pr, 'age', 'age_string')) {\n+\t\t\tmy (@activity) = git_get_last_activity($pr->{'path'});\n+\t\t\tunless (@activity) {\n+\t\t\t\tnext PROJECT;\n+\t\t\t}\n+\t\t\t($pr->{'age'}, $pr->{'age_string'}) = @activity;\n \t\t}\n-\t\t($pr->{'age'}, $pr->{'age_string'}) = @activity;\n-\t\tif (!defined $pr->{'descr'}) {\n+\t\tif (project_info_needs_filling($pr, 'descr', 'descr_long')) {\n \t\t\tmy $descr = git_get_project_description($pr->{'path'}) || \"\";\n \t\t\t$descr = to_utf8($descr);\n \t\t\t$pr->{'descr_long'} = $descr;\n \t\t\t$pr->{'descr'} = chop_str($descr, $projects_list_description_width, 5);\n \t\t}\n-\t\tif (!defined $pr->{'owner'}) {\n+\t\tif (project_info_needs_filling($pr, 'owner')) {\n \t\t\t$pr->{'owner'} = git_get_project_owner(\"$pr->{'path'}\") || \"\";\n \t\t}\n-\t\tif ($show_ctags) {\n+\t\tif ($show_ctags &&\n+\t\t project_info_needs_filling($pr, 'ctags')) {\n \t\t\t$pr->{'ctags'} = git_get_project_ctags($pr->{'path'});\n \t\t}\n-\t\tif ($projects_list_group_categories && !defined $pr->{'category'}) {\n+\t\tif ($projects_list_group_categories &&\n+\t\t project_info_needs_filling($pr, 'category')) {\n \t\t\tmy $cat = git_get_project_category($pr->{'path'}) ||\n \t\t\t $project_list_default_category;\n \t\t\t$pr->{'category'} = to_utf8($cat);\n-- \n1.7.9\n"},{"id":"184786","messageId":"1329338332-30358-3-git-send-email-jnareb@gmail.com","threadId":"29638","inReplyTo":"1329338332-30358-1-git-send-email-jnareb@gmail.com","subject":"[PATCHv2 2/8] gitweb: Option for filling only specified info in fill_project_list_info","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-15T20:38:46Z","receivedAt":"2012-02-15T20:38:46Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Introduce project_info_needs_filling($pr, $key[, \\%fill_only]), which\nis now used in place of simple 'defined $pr->{$key}' to check if\nspecific slot in project needs to be filled.\n\nThis is in preparation of future lazy filling of project info in\nproject search and pagination of sorted list of projects. The only\nfunctional change is that fill_project_list_info() now checks if 'age'\nis already filled before running git_get_last_activity().\n\nSigned-off-by: Jakub Narebski \n---\nThis could have been squashed with the next commit, but this way it is\npure refactoring that shouldn't change gitweb behavior.\n\nChanges from v1:\n* Introduction of project_info_needs_filling() was moved to separate\n commit; this one just adds @fill_only / %fill_only, without actually \n using it.\n\n* It uses \\%fill_only (reference to hash) rather than @fill_only,\n because project_info_needs_filling() now supports multiple keys,\n and because checking key in hash is faster O(1) than checking every\n element in array O(n).\n\n Though this shouldn't matter much, as @fill_only has at most two\n or three elements, as we would see in the next commit.\n\n gitweb/gitweb.perl | 34 +++++++++++++++++++++++-----------\n 1 files changed, 23 insertions(+), 11 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex e62c2ef..cae71f5 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5195,54 +5195,66 @@ sub git_project_search_form {\n \tprint \"\\n\";\n }\n \n-# entry for given @keys needs filling if at least one of keys in list\n-# is not present in %$project_info\n+\n+# entry for given @keys needs filling if at least one of interesting keys\n+# in list is not present in %$project_info; key is interesting if $fill_only\n+# is not passed, or is empty (all keys are interesting in both of those cases),\n+# or if key is in $fill_only hash\n+#\n+# USAGE:\n+# * project_info_needs_filling($project_info, 'key', ...)\n+# * project_info_needs_filling($project_info, 'key', ..., \\%fill_only)\n+# where %fill_only = map { $_ => 1 } @fill_only;\n sub project_info_needs_filling {\n+\tmy $fill_only = ref($_[-1]) ? pop : undef;\n \tmy ($project_info, @keys) = @_;\n \n \t# return List::MoreUtils::any { !exists $project_info->{$_} } @keys;\n \tforeach my $key (@keys) {\n-\t\tif (!exists $project_info->{$key}) {\n+\t\tif ((!$fill_only || !%$fill_only || $fill_only->{$key}) &&\n+\t\t !exists $project_info->{$key}) {\n \t\t\treturn 1;\n \t\t}\n \t}\n \treturn;\n }\n \n-\n # fills project list info (age, description, owner, category, forks)\n # for each project in the list, removing invalid projects from\n-# returned list\n+# returned list, or fill only specified info (removing invalid projects\n+# only when filling 'age').\n+#\n # NOTE: modifies $projlist, but does not remove entries from it\n sub fill_project_list_info {\n-\tmy $projlist = shift;\n+\tmy ($projlist, @fill_only) = @_;\n+\tmy %fill_only = map { $_ => 1 } @fill_only;\n \tmy @projects;\n \n \tmy $show_ctags = gitweb_check_feature('ctags');\n PROJECT:\n \tforeach my $pr (@$projlist) {\n-\t\tif (project_info_needs_filling($pr, 'age', 'age_string')) {\n+\t\tif (project_info_needs_filling($pr, 'age', 'age_string', \\%fill_only)) {\n \t\t\tmy (@activity) = git_get_last_activity($pr->{'path'});\n \t\t\tunless (@activity) {\n \t\t\t\tnext PROJECT;\n \t\t\t}\n \t\t\t($pr->{'age'}, $pr->{'age_string'}) = @activity;\n \t\t}\n-\t\tif (project_info_needs_filling($pr, 'descr', 'descr_long')) {\n+\t\tif (project_info_needs_filling($pr, 'descr', 'descr_long', \\%fill_only)) {\n \t\t\tmy $descr = git_get_project_description($pr->{'path'}) || \"\";\n \t\t\t$descr = to_utf8($descr);\n \t\t\t$pr->{'descr_long'} = $descr;\n \t\t\t$pr->{'descr'} = chop_str($descr, $projects_list_description_width, 5);\n \t\t}\n-\t\tif (project_info_needs_filling($pr, 'owner')) {\n+\t\tif (project_info_needs_filling($pr, 'owner', \\%fill_only)) {\n \t\t\t$pr->{'owner'} = git_get_project_owner(\"$pr->{'path'}\") || \"\";\n \t\t}\n \t\tif ($show_ctags &&\n-\t\t project_info_needs_filling($pr, 'ctags')) {\n+\t\t project_info_needs_filling($pr, 'ctags', \\%fill_only)) {\n \t\t\t$pr->{'ctags'} = git_get_project_ctags($pr->{'path'});\n \t\t}\n \t\tif ($projects_list_group_categories &&\n-\t\t project_info_needs_filling($pr, 'category')) {\n+\t\t project_info_needs_filling($pr, 'category', \\%fill_only)) {\n \t\t\tmy $cat = git_get_project_category($pr->{'path'}) ||\n \t\t\t $project_list_default_category;\n \t\t\t$pr->{'category'} = to_utf8($cat);\n-- \n1.7.9\n"},{"id":"184785","messageId":"1329338332-30358-4-git-send-email-jnareb@gmail.com","threadId":"29638","inReplyTo":"1329338332-30358-1-git-send-email-jnareb@gmail.com","subject":"[PATCHv2 3/8] gitweb: Faster project search","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-15T20:38:47Z","receivedAt":"2012-02-15T20:38:47Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Before searching by some field the information we search for must be\nfilled in. For this fill_project_list_info() was enhanced in previous\ncommit to take additional parameters which part of projects info to\nfill. This way we can limit doing expensive calculations (like\nrunning git-for-each-ref to get 'age' / \"Last changed\" info) only to\nprojects which we will show as search results.\n\nWith this commit the number of git commands used to generate search\nresults is 2* + 1, and depends on number of matched\nprojects rather than number of all projects (all repositories).\n\nNote: this is 'git for-each-ref' to find last activity, and 'git config'\nfor each project, and 'git --version' once.\n\nSigned-off-by: Jakub Narebski \n---\nsearch_projects_list() now pre-fills required parts of project info by\nitself, so running fill_project_list_info() before calling it is no\nlonger necessary and actually you should not do it.\n\nNo changes from v1\n\n gitweb/gitweb.perl | 9 +++++++--\n 1 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex cae71f5..cc2bd6d 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -2987,6 +2987,10 @@ sub search_projects_list {\n \treturn @$projlist\n \t\tunless ($tagfilter || $searchtext);\n \n+\t# searching projects require filling to be run before it;\n+\tfill_project_list_info($projlist,\n+\t $tagfilter ? 'ctags' : (),\n+\t $searchtext ? ('path', 'descr') : ());\n \tmy @projects;\n PROJECT:\n \tforeach my $pr (@$projlist) {\n@@ -5390,12 +5394,13 @@ sub git_project_list_body {\n \t# filtering out forks before filling info allows to do less work\n \t@projects = filter_forks_from_projects_list(\\@projects)\n \t\tif ($check_forks);\n-\t@projects = fill_project_list_info(\\@projects);\n-\t# searching projects require filling to be run before it\n+\t# search_projects_list pre-fills required info\n \t@projects = search_projects_list(\\@projects,\n \t 'searchtext' => $searchtext,\n \t 'tagfilter' => $tagfilter)\n \t\tif ($tagfilter || $searchtext);\n+\t# fill the rest\n+\t@projects = fill_project_list_info(\\@projects);\n \n \t$order ||= $default_projects_order;\n \t$from = 0 unless defined $from;\n-- \n1.7.9\n"},{"id":"184788","messageId":"1329338332-30358-5-git-send-email-jnareb@gmail.com","threadId":"29638","inReplyTo":"1329338332-30358-1-git-send-email-jnareb@gmail.com","subject":"[PATCHv2 4/8] gitweb: Introduce esc_html_hl_regions","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-15T20:38:48Z","receivedAt":"2012-02-15T20:38:48Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"The esc_html_hl_regions() subroutine added in this commit is meant to\nhighlight in a given string a list of regions (given as a list of\n[ beg, end ] pairs of positions in string), using HTML element\nwith given class.\n\nIt is to be used in next commit for highlighting matched part in\nproject search.\n\nImplementation and enhancement notes:\n~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n* Currently esc_html_hl_regions() subroutine doesn't accept any\n parameters, like esc_html() does. We might want for example to\n pass nbsp=>1 to it.\n\n It can easily be done with the following code:\n\n my %opts = grep { ref($_) ne \"ARRAY\" } @sel;\n @sel = grep { ref($_) eq \"ARRAY\" } @sel;\n\n This allow adding parameters after or before regions, e.g.:\n\n esc_html_hl_regions(\"foo bar\", \"mark\", [ 0, 3 ], -nbsp => 1);\n\n* esc_html_hl_regions() escapes like esc_html(); if we wanted to\n highlight with esc_path(), we could pass subroutine reference\n to now named esc_gen_hl_regions().\n\n esc_html_hl_regions(\"foo bar\", \"mark\", \\&esc_path, [ 0, 3 ]);\n\n Note that this way we can handle -nbsp=>1 case automatically,\n e.g.\n\n esc_html_hl_regions(\"foo bar\", \"mark\",\n sub { esc_html(@_, -nbsp=>1) },\n [ 0, 3 ]);\n\n* Alternate solution for highlighting region of a string would be to\n use the idea that strings are to be HTML-escaped, and references to\n scalars are HTML (like in the idea for generic committags).\n\n This would require modifying gitweb code or esc_html to get list of\n fragments, e.g.:\n\n esc_html(\\'', 'foo', \\'', ' bar',\n { -nbsp => 1 });\n\nSigned-off-by: Jakub Narebski \n---\nThis commit is here only for the notes in the commit message,\notherwise it could have been squashed with the next commit.\n\nNote that we actually end up using first enhancement in notes section\nof the above commit message.\n\nThis patch is new; it was not present in v1\n\n gitweb/gitweb.perl | 27 +++++++++++++++++++++++++++\n 1 files changed, 27 insertions(+), 0 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex cc2bd6d..8dcd54b 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1715,6 +1715,33 @@ sub chop_and_escape_str {\n \t}\n }\n \n+# Highlight selected fragments of string, using given CSS class,\n+# and escape HTML. It is assumed that fragments do not overlap.\n+# Regions are passed as list of pairs (array references).\n+#\n+# Example: esc_html_hl_regions(\"foobar\", \"mark\", [ 0, 3 ]) returns\n+# 'foobar'\n+sub esc_html_hl_regions {\n+\tmy ($str, $css_class, @sel) = @_;\n+\treturn esc_html($str) unless @sel;\n+\n+\tmy $out = '';\n+\tmy $pos = 0;\n+\n+\tfor my $s (@sel) {\n+\t\t$out .= esc_html(substr($str, $pos, $s->[0] - $pos))\n+\t\t\tif ($s->[0] - $pos > 0);\n+\t\t$out .= $cgi->span({-class => $css_class},\n+\t\t esc_html(substr($str, $s->[0], $s->[1] - $s->[0])));\n+\n+\t\t$pos = $s->[1];\n+\t}\n+\t$out .= esc_html(substr($str, $pos))\n+\t\tif ($pos < length($str));\n+\n+\treturn $out;\n+}\n+\n ## ----------------------------------------------------------------------\n ## functions returning short strings\n \n-- \n1.7.9\n"},{"id":"184791","messageId":"1329338332-30358-6-git-send-email-jnareb@gmail.com","threadId":"29638","inReplyTo":"1329338332-30358-1-git-send-email-jnareb@gmail.com","subject":"[PATCHv2 5/8] gitweb: Highlight matched part of project name when searching projects","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-15T20:38:49Z","receivedAt":"2012-02-15T20:38:49Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Use newly introduced esc_html_match_hl() to escape HTML and mark match\nwith span element with 'match' class. Currently only 'path' part\n(i.e. project name) is highlighted; match might be on the project\ndescription.\n\nThe code makes use of the fact that defined $search_regexp means that\nthere was search going on.\n\nSigned-off-by: Jakub Narebski \n---\nIntroducing esc_html_match_hl() could have been put together with\nintroduction of esc_html_hl_regions() in previos commit.\n\nChanges from v1:\n* Main part of esc_html_match_hl() got split into esc_html_hl_regions(),\n which was introduced in previous commit.\n\n gitweb/gitweb.perl | 18 +++++++++++++++++-\n 1 files changed, 17 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 8dcd54b..5596701 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1742,6 +1742,20 @@ sub esc_html_hl_regions {\n \treturn $out;\n }\n \n+# highlight match (if any), and escape HTML\n+sub esc_html_match_hl {\n+\tmy ($str, $regexp) = @_;\n+\treturn esc_html($str) unless defined $regexp;\n+\n+\tmy @matches;\n+\twhile ($str =~ /$regexp/g) {\n+\t\tpush @matches, [$-[0], $+[0]];\n+\t}\n+\treturn esc_html($str) unless @matches;\n+\n+\treturn esc_html_hl_regions($str, 'match', @matches);\n+}\n+\n ## ----------------------------------------------------------------------\n ## functions returning short strings\n \n@@ -5389,7 +5403,9 @@ sub git_project_list_rows {\n \t\t\tprint \"\\n\";\n \t\t}\n \t\tprint \"\" . $cgi->a({-href => href(project=>$pr->{'path'}, action=>\"summary\"),\n-\t\t -class => \"list\"}, esc_html($pr->{'path'})) . \"\\n\" .\n+\t\t -class => \"list\"},\n+\t\t esc_html_match_hl($pr->{'path'}, $search_regexp)) .\n+\t\t \"\\n\" .\n \t\t \"\" . $cgi->a({-href => href(project=>$pr->{'path'}, action=>\"summary\"),\n \t\t -class => \"list\", -title => $pr->{'descr_long'}},\n \t\t esc_html($pr->{'descr'})) . \"\\n\" .\n-- \n1.7.9\n"},{"id":"184789","messageId":"1329338332-30358-7-git-send-email-jnareb@gmail.com","threadId":"29638","inReplyTo":"1329338332-30358-1-git-send-email-jnareb@gmail.com","subject":"[PATCHv2 6/8] gitweb: Highlight matched part of project description when searching projects","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-15T20:38:50Z","receivedAt":"2012-02-15T20:38:50Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Use esc_html_match_hl() from previous commit to mark match in the\n_whole_ description when searching projects.\n\nCurrently, with this commit, when searching projects there is always\nshown full description of a project, and not a shortened one (like for\nordinary projects list view), even if the match is on project name and\nnot project description.\n\nShowing full description when there is match on it is useful to avoid\nsituation where match is in shortened, invisible part... well, perhaps\nthat could be solved (showing shortened description), but it would\nrequire much more complicated code.\n\nSigned-off-by: Jakub Narebski \n---\nThe part about showing match using shortened description no longer\napplies after the following patch... though it is an RFC for now,\nthat is why it is not mentioned in the commit message.\n\nNOTE that we are actually always showing full description, so having\nfull description in \"title\" attribute to show on mouseover over\nshortened title is no longer necessary, and should probably be\nfixed... that is unless the next patch that highlights matches in\nshortened description is accepted.\n\nNo changes from previous version (but see paragraph above about\npossible changes).\n\n gitweb/gitweb.perl | 5 ++++-\n 1 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 5596701..a109ebb 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5408,7 +5408,10 @@ sub git_project_list_rows {\n \t\t \"\\n\" .\n \t\t \"\" . $cgi->a({-href => href(project=>$pr->{'path'}, action=>\"summary\"),\n \t\t -class => \"list\", -title => $pr->{'descr_long'}},\n-\t\t esc_html($pr->{'descr'})) . \"\\n\" .\n+\t\t $search_regexp\n+\t\t ? esc_html_match_hl($pr->{'descr_long'}, $search_regexp)\n+\t\t : esc_html($pr->{'descr'})) .\n+\t\t \"\\n\" .\n \t\t \"\" . chop_and_escape_str($pr->{'owner'}, 15) . \"\\n\";\n \t\tprint \"{'age'}) . \"\\\">\" .\n \t\t (defined $pr->{'age_string'} ? $pr->{'age_string'} : \"No commits\") . \"\\n\" .\n-- \n1.7.9\n"},{"id":"184787","messageId":"1329338332-30358-8-git-send-email-jnareb@gmail.com","threadId":"29638","inReplyTo":"1329338332-30358-1-git-send-email-jnareb@gmail.com","subject":"[PATCHv3/RFC 7/8] gitweb: Highlight matched part of shortened project description","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-15T20:38:51Z","receivedAt":"2012-02-15T20:38:51Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Previous commit make gitweb use esc_html_match_hl() to mark match in\nthe _whole_ description of a project when searching projects.\n\nThis commit makes gitweb highlight match in _shortened_ description,\nbased on match in whole description, using esc_html_match_hl_chopped()\nsubroutine.\n\nIf match is in removed (chopped) part, even partially, then trailing\n\"... \" is highlighted.\n\nSigned-off-by: Jakub Narebski \n---\nIt is marked as RFC, because I am not sure if it is right way to\nhighlight match in shortened string, or if we better use full string,\nor full string if match is in chopped part.\n\nChanges from v2:\n* Harden esc_html_match_hl_chopped() against calling with both\n $chopped and $regexp undefined (even though it wouldn't happen\n with current code).\n\n* esc_html_match_hl_chopped() uses now esc_html_hl_regions(),\n like esc_html_match_hl() used to do.\n\nChanges from v1:\n* Instead of esc_html_match_hl_chopped() duplicating much od code in\n esc_html_match_hl(), make esc_html_match_hl() call the *_chopped()\n one with $chopped set to undef.\n\n Now managing highlighting of $chopped part is just a matter of\n adjusting and filtering @matches to apply to $chopped rather than\n original $str where match was performed.\n\n As a side issue when match span past chop point current code uses\n one selection foo... and not two\n foo... \n\n gitweb/gitweb.perl | 40 ++++++++++++++++++++++++++++++++++++++--\n 1 files changed, 38 insertions(+), 2 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex a109ebb..a2e2023 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1747,11 +1747,46 @@ sub esc_html_match_hl {\n \tmy ($str, $regexp) = @_;\n \treturn esc_html($str) unless defined $regexp;\n \n+\treturn esc_html_match_hl_chopped($str, undef, $regexp);\n+}\n+\n+\n+# highlight match (if any) of shortened string, and escape HTML\n+sub esc_html_match_hl_chopped {\n+\tmy ($str, $chopped, $regexp) = @_;\n+\treturn esc_html(defined $chopped ? $chopped : $str) unless defined $regexp;\n+\n \tmy @matches;\n \twhile ($str =~ /$regexp/g) {\n \t\tpush @matches, [$-[0], $+[0]];\n \t}\n-\treturn esc_html($str) unless @matches;\n+\treturn esc_html(defined $chopped ? $chopped : $str) unless @matches;\n+\n+\t# filter matches so that we mark chopped string, if it is present\n+\tif (defined $chopped) {\n+\t\tmy $tail = \"... \"; # see chop_str\n+\t\tunless ($chopped =~ s/\\Q$tail\\E$//) {\n+\t\t\t$tail = '';\n+\t\t}\n+\t\tmy $chop_len = length($chopped);\n+\t\tmy $tail_len = length($tail);\n+\t\tmy @filtered;\n+\n+\t\tfor my $m (@matches) {\n+\t\t\tif ($m->[0] > $chop_len) {\n+\t\t\t\tpush @filtered, [ $chop_len, $chop_len + $tail_len ] if ($tail_len > 0);\n+\t\t\t\tlast;\n+\t\t\t} elsif ($m->[1] > $chop_len) {\n+\t\t\t\tpush @filtered, [ $m->[0], $chop_len + $tail_len ];\n+\t\t\t\tlast;\n+\t\t\t}\n+\t\t\tpush @filtered, $m;\n+\t\t}\n+\n+\t\t# further operations are on chopped string\n+\t\t$str = $chopped . $tail;\n+\t\t@matches = @filtered;\n+\t}\n \n \treturn esc_html_hl_regions($str, 'match', @matches);\n }\n@@ -5409,7 +5444,8 @@ sub git_project_list_rows {\n \t\t \"\" . $cgi->a({-href => href(project=>$pr->{'path'}, action=>\"summary\"),\n \t\t -class => \"list\", -title => $pr->{'descr_long'}},\n \t\t $search_regexp\n-\t\t ? esc_html_match_hl($pr->{'descr_long'}, $search_regexp)\n+\t\t ? esc_html_match_hl_chopped($pr->{'descr_long'},\n+\t\t $pr->{'descr'}, $search_regexp)\n \t\t : esc_html($pr->{'descr'})) .\n \t\t \"\\n\" .\n \t\t \"\" . chop_and_escape_str($pr->{'owner'}, 15) . \"\\n\";\n-- \n1.7.9\n"},{"id":"184790","messageId":"1329338332-30358-9-git-send-email-jnareb@gmail.com","threadId":"29638","inReplyTo":"1329338332-30358-1-git-send-email-jnareb@gmail.com","subject":"[PATCHv2 8/8] gitweb: Use esc_html_match_hl() in 'grep' search","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-15T20:38:52Z","receivedAt":"2012-02-15T20:38:52Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Use esc_html_match_hl() in git_search_files() i.e. subroutine that\nimplements \"grep\" search, instead of custom code which highlighted\nonly one, last match.\n\nThis required enhancing esc_html_match_hl() to accept -nbsp=>1 and\npass it down to esc_html().\n\nNote that line is untabified (tabs turned into spaces) before\nhighlighting match, which means that highlighting won't work e.g. for\nmatching tab character \"\\t\" explicitly; but this issue was present\nbefore this commit, and is not that easy to fix.\n\nSigned-off-by: Jakub Narebski \n---\nThis patch was not present in previous (v1) version of this patch\nseries.\n\nThe 'grep' search was chosen from other searches because of the\nfollowing reasons:\n* 'pickaxe' search does not show matches in diff, only filenames.\n* 'commit' search shortens leading text at beginning, one last match\n in the middle, and trailing text at the end; anyway I think this\n search should be rewritten to show just \"log\"-like view with match\n highlighting.\n\n gitweb/gitweb.perl | 34 ++++++++++++++--------------------\n 1 files changed, 14 insertions(+), 20 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex a2e2023..36a8cca 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1723,20 +1723,22 @@ sub chop_and_escape_str {\n # 'foobar'\n sub esc_html_hl_regions {\n \tmy ($str, $css_class, @sel) = @_;\n-\treturn esc_html($str) unless @sel;\n+\tmy %opts = grep { ref($_) ne 'ARRAY' } @sel;\n+\t@sel = grep { ref($_) eq 'ARRAY' } @sel;\n+\treturn esc_html($str, %opts) unless @sel;\n \n \tmy $out = '';\n \tmy $pos = 0;\n \n \tfor my $s (@sel) {\n-\t\t$out .= esc_html(substr($str, $pos, $s->[0] - $pos))\n+\t\t$out .= esc_html(substr($str, $pos, $s->[0] - $pos), %opts)\n \t\t\tif ($s->[0] - $pos > 0);\n \t\t$out .= $cgi->span({-class => $css_class},\n-\t\t esc_html(substr($str, $s->[0], $s->[1] - $s->[0])));\n+\t\t esc_html(substr($str, $s->[0], $s->[1] - $s->[0]), %opts));\n \n \t\t$pos = $s->[1];\n \t}\n-\t$out .= esc_html(substr($str, $pos))\n+\t$out .= esc_html(substr($str, $pos), %opts)\n \t\tif ($pos < length($str));\n \n \treturn $out;\n@@ -1744,23 +1746,23 @@ sub esc_html_hl_regions {\n \n # highlight match (if any), and escape HTML\n sub esc_html_match_hl {\n-\tmy ($str, $regexp) = @_;\n-\treturn esc_html($str) unless defined $regexp;\n+\tmy ($str, $regexp, %opts) = @_;\n+\treturn esc_html($str, %opts) unless defined $regexp;\n \n-\treturn esc_html_match_hl_chopped($str, undef, $regexp);\n+\treturn esc_html_match_hl_chopped($str, undef, $regexp, %opts);\n }\n \n \n # highlight match (if any) of shortened string, and escape HTML\n sub esc_html_match_hl_chopped {\n-\tmy ($str, $chopped, $regexp) = @_;\n-\treturn esc_html(defined $chopped ? $chopped : $str) unless defined $regexp;\n+\tmy ($str, $chopped, $regexp, %opts) = @_;\n+\treturn esc_html(defined $chopped ? $chopped : $str, %opts) unless defined $regexp;\n \n \tmy @matches;\n \twhile ($str =~ /$regexp/g) {\n \t\tpush @matches, [$-[0], $+[0]];\n \t}\n-\treturn esc_html(defined $chopped ? $chopped : $str) unless @matches;\n+\treturn esc_html(defined $chopped ? $chopped : $str, %opts) unless @matches;\n \n \t# filter matches so that we mark chopped string, if it is present\n \tif (defined $chopped) {\n@@ -1788,7 +1790,7 @@ sub esc_html_match_hl_chopped {\n \t\t@matches = @filtered;\n \t}\n \n-\treturn esc_html_hl_regions($str, 'match', @matches);\n+\treturn esc_html_hl_regions($str, 'match', @matches, %opts);\n }\n \n ## ----------------------------------------------------------------------\n@@ -6070,15 +6072,7 @@ sub git_search_files {\n \t\t\tprint \"
Binary file
\\n\";\n \t\t} else {\n \t\t\t$ltext = untabify($ltext);\n-\t\t\tif ($ltext =~ m/^(.*)($search_regexp)(.*)$/i) {\n-\t\t\t\t$ltext = esc_html($1, -nbsp=>1);\n-\t\t\t\t$ltext .= '';\n-\t\t\t\t$ltext .= esc_html($2, -nbsp=>1);\n-\t\t\t\t$ltext .= '';\n-\t\t\t\t$ltext .= esc_html($3, -nbsp=>1);\n-\t\t\t} else {\n-\t\t\t\t$ltext = esc_html($ltext, -nbsp=>1);\n-\t\t\t}\n+\t\t\t$ltext = esc_html_match_hl($ltext, qr/$search_regexp/i, -nbsp=>1);\n \t\t\tprint \"
\" .\n \t\t\t\t$cgi->a({-href => $file_href.'#l'.$lno,\n \t\t\t\t -class => \"linenr\"}, sprintf('%4i', $lno)) .\n-- \n1.7.9\n"},{"id":"184837","messageId":"7vmx8iy0ix.fsf@alter.siamese.dyndns.org","threadId":"29638","inReplyTo":"1329338332-30358-1-git-send-email-jnareb@gmail.com","subject":"Re: [PATCHv2 0/8] gitweb: Faster and improved project search","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-16T20:40:54Z","receivedAt":"2012-02-16T20:40:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> [Cc-ing Junio because of his involvement in discussion about first\n> patch in previous version of this series.]\n>\n> First three patches in this series are mainly about speeding up\n> project search (and perhaps in the future also project pagination).\n> Well, first one is unification, refactoring and future-proofing.\n> The second and third patch could be squashed together; second adds\n> @fill_only, but third actually uses it.\n>\n> Next set of patches is about highlighting matched part, making it\n> easier to recognize why project was selected, what we were searching\n> for (though better page title would also help second issue).\n>\n> Well, fourth patch (first in set mentioned above) is here for the\n> commit message, otherwise it could have been squashed with next one.\n>\n> Last patch in this series is beginning of using esc_html_match_hl()\n> for other searches in gitweb -- the easiest part.\n\nNotice that you never said anything about what you wanted to achieve with\nthis entire series? \" -- the easiest part.\" does not mean anything.\nThe easiest part of what?\n\nWhere in the series do the \"faster\" and \"improved\" come from? What do you\nexactly mean by \"faster\" and \"improved\"? In which commit would we find\nthe answers to the questions like:\n\n - What operation was slow and how you tackled that slowness?\n - What are the benchmark results?\n\nIn general, \"improve\" is such a loaded word (after all, patches sent to\nthe list are almost always meant to \"improve\" things) that it almost does\nnot convey a single bit of information. Are you fixing bugs? Are you\ntidying up an unreadable piece of code? Are you fixing styles? Are you\nproducing prettier output? Are you refactoring cut-and-pasted repetition\ninto a common helper function? Are you adding a new feature?\n"},{"id":"184844","messageId":"201202162253.49000.jnareb@gmail.com","threadId":"29638","inReplyTo":"7vmx8iy0ix.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 0/8] gitweb: Faster and improved project search","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-16T21:53:47Z","receivedAt":"2012-02-16T21:53:47Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 16 Feb 2012, Junio C Hamano wrote:\n> Jakub Narebski writes:\n> \n> > [Cc-ing Junio because of his involvement in discussion about first\n> > patch in previous version of this series.]\n> >\n> > First three patches in this series are mainly about speeding up\n> > project search (and perhaps in the future also project pagination).\n> > Well, first one is unification, refactoring and future-proofing.\n> > The second and third patch could be squashed together; second adds\n> > @fill_only, but third actually uses it.\n> >\n> > Next set of patches is about highlighting matched part, making it\n> > easier to recognize why project was selected, what we were searching\n> > for (though better page title would also help second issue).\n> >\n> > Well, fourth patch (first in set mentioned above) is here for the\n> > commit message, otherwise it could have been squashed with next one.\n> >\n> > Last patch in this series is beginning of using esc_html_match_hl()\n> > for other searches in gitweb -- the easiest part.\n> \n> Notice that you never said anything about what you wanted to achieve with\n> this entire series? \" -- the easiest part.\" does not mean anything.\n> The easiest part of what?\n\nGitweb, besides project search, supports searching in a give repository\nof a commit message ('commit', 'author', 'committer'), of files ('grep')\nand of changes ('pickaxe'). From those 'grep' was easiest. This is\nmentioned in comments in 8/8.\n\nWhat I'd like to achieve is unification of match highlighting code, which\nreduces code duplication. esc_html_match_hl() is also better than current\nhand-written code: it highlights all matches, and not only last match.\n\n> Where in the series do the \"faster\" and \"improved\" come from? What do you\n> exactly mean by \"faster\" and \"improved\"? In which commit would we find\n> the answers to the questions like:\n> \n> - What operation was slow and how you tackled that slowness?\n> - What are the benchmark results?\n\nThose series make project search faster because they reduce number of\ngit commands that gitweb runs when generating project search results.\nBefore number of commands was 2* + 1, after 3/8\nit is 2* + 1.\n\n\nFor search that selects 2 repositories out of 12 total\n\n* Before (warm cache):\n\n \"This page took 0.867151 seconds and 27 git commands to generate.\"\n\n* After (warm cache):\n \n \"This page took 0.673643 seconds and 5 git commands to generate.\"\n\nThe improvement in performance is even better for larger number of\nrepositories, and for cold cache / evicted cache.\n\n\nShould I do benchmark of git_project_list_body() subroutine before\nand after?\n\n> In general, \"improve\" is such a loaded word (after all, patches sent to\n> the list are almost always meant to \"improve\" things) that it almost does\n> not convey a single bit of information. Are you fixing bugs? Are you\n> tidying up an unreadable piece of code? Are you fixing styles? Are you\n> producing prettier output? Are you refactoring cut-and-pasted repetition\n> into a common helper function? Are you adding a new feature?\n\nI'm sorry about vague \"improved\". What I meant is that I have added\nhighlighting of matched part in project search, just like e.g. Google\ndoes unobtrusive highlighting of search phrase in page snippets.\n\nThis is especially important because project search matches either\nproject name or project description, and it might be not obvious why\nthe match... especially that match on description might be in shortened\nchopped-out part.\n\n\nI'll improve commit messages and cover letter in a re-roll.\n-- \nJakub Narebski\nPoland\n"},{"id":"184967","messageId":"7vfwe6ng8u.fsf@alter.siamese.dyndns.org","threadId":"29638","inReplyTo":"1329338332-30358-3-git-send-email-jnareb@gmail.com","subject":"Re: [PATCHv2 2/8] gitweb: Option for filling only specified info in fill_project_list_info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-20T06:58:09Z","receivedAt":"2012-02-20T06:58:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> Introduce project_info_needs_filling($pr, $key[, \\%fill_only]), which\n\nThat's \"project_info_needs_filling($pr, @key[, \\%fill_only])\", isn't it?\n\n> +# entry for given @keys needs filling if at least one of interesting keys\n> +# in list is not present in %$project_info; key is interesting if $fill_only\n> +# is not passed, or is empty (all keys are interesting in both of those cases),\n> +# or if key is in $fill_only hash\n> +#\n> +# USAGE:\n> +# * project_info_needs_filling($project_info, 'key', ...)\n> +# * project_info_needs_filling($project_info, 'key', ..., \\%fill_only)\n> +# where %fill_only = map { $_ => 1 } @fill_only;\n\nThe reason this spells bad to me is that it gives the readers (and anybody\nwho may want to touch gitweb code) that the caller has _two_ ways to say\nthe same thing.\n\nIf a caller is interested in finding out $key in $project_info, it can\neither (1) pass $key as one of the earlier parameters to the function and\nnot pass any \\%fill_only, or (2) pass $key and pass \\%fill_only but after\nmaking sure \\%fill_only also has $key defined.\n\nAnd if the caller passes \\%fill_only, it has to remember that it needs to\nadd $key to it; otherwise the earlier request \"I want $key\" is ignored by\nthe function.\n\nWhy is it a good thing to have such a convoluted API?\n\nIs it that \"fill_project_list_info\" is called by multiple callers, and\nthey do not necessarily need to see all the fields that can be filled by\nthe function (namely, age, age_string, descr, descr_long, owner, ctags and\ncategory)? If that is the case, I must say that the approach to put the\nset filtering logic inside project_info_needs_filling function smells like\na bad API design.\n\nIn other words, wouldn't a callchain that uses a more natural set of API\nfunctions be like this?\n\n\t# the top-level caller that is interested only in these two fields\n fill_project_list_info($projlist, 'age', 'owner');\n\n # in fill_project_list_info()\n\tmy $projlist = shift;\n my %wanted = map { $_ => 1 } @_;\n\n\tforeach my $pr (@$projlist) {\n\t\tif (project_info_needs_filling($pr,\n\t\t\tfilter_set(\\%wanted, 'age', 'age_string'))) {\n\t\t\t... get @activity ...\n ($pr->{'age'}, $pr->{'age_string'}) = @activity;\n\t\t}\n\t\t...\n\t}\n\nThat way, \"needs_filling\" has to do one and only one thing well: it checks\nif any of the fields it was asked is missing and answers. And a generic\nset filtering helper that takes a filtering hashref and an array can be\nused later outside of the \"needs_filling\" function.\n\n> sub project_info_needs_filling {\n> +\tmy $fill_only = ref($_[-1]) ? pop : undef;\n> \tmy ($project_info, @keys) = @_;\n> \n> \t# return List::MoreUtils::any { !exists $project_info->{$_} } @keys;\n> \tforeach my $key (@keys) {\n> -\t\tif (!exists $project_info->{$key}) {\n> +\t\tif ((!$fill_only || !%$fill_only || $fill_only->{$key}) &&\n> +\t\t !exists $project_info->{$key}) {\n> \t\t\treturn 1;\n> \t\t}\n> \t}\n> \treturn;\n> }\n> \n> -\n\nShouldn't the previous patch updated not to add this blank line in the\nfirst place?\n\n> # fills project list info (age, description, owner, category, forks)\n\nIs this enumeration up-to-date? If we cannot keep it up-to-date, it may\nmake sense to show only a few as examples and add ellipses.\n\n> # for each project in the list, removing invalid projects from\n> -# returned list\n> +# returned list, or fill only specified info (removing invalid projects\n> +# only when filling 'age').\n\nIt is unclear what the added clause wants to say; especially, the link\nbetween the mention of 'age' and 'only when' is unclear. Is asking of\n'age' what makes a request a notable exception to remove invalid projects?\nOr is asking to partially fill fields what triggers the removal of invalid\nprojects, and you mentioned 'age' as an example?\n\nIf it is the former (I am guessing it is), it would be much clearer to\nsay:\n\n Invalid projects are removed from the returned list if and only if you\n ask 'age' to be filled, because of such and such reasons.\n"},{"id":"184968","messageId":"7v4numnfqq.fsf@alter.siamese.dyndns.org","threadId":"29638","inReplyTo":"1329338332-30358-1-git-send-email-jnareb@gmail.com","subject":"Re: [PATCHv2 0/8] gitweb: Faster and improved project search","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-20T07:09:01Z","receivedAt":"2012-02-20T07:09:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> [Cc-ing Junio because of his involvement in discussion about first\n> patch in previous version of this series.]\n>\n> First three patches in this series are mainly about speeding up\n> project search (and perhaps in the future also project pagination).\n> Well, first one is unification, refactoring and future-proofing.\n> The second and third patch could be squashed together; second adds\n> @fill_only, but third actually uses it.\n\nI'll queue these three separately to a topic jn/gitweb-search-optim,\nand fork another topic jn/gitweb-hilite-regions from there. I haven't\nlooked the latter closely, though.\n\nThanks.\n"},{"id":"184974","messageId":"7vk43hnbt7.fsf@alter.siamese.dyndns.org","threadId":"29638","inReplyTo":"1329338332-30358-4-git-send-email-jnareb@gmail.com","subject":"Re: [PATCHv2 3/8] gitweb: Faster project search","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-20T08:33:56Z","receivedAt":"2012-02-20T08:33:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> Before searching by some field the information we search for must be\n> filled in. For this fill_project_list_info() was enhanced in previous\n> commit to take additional parameters which part of projects info to\n\n\"must be filled in.\" is correct, but that happens without the previous\npatch. The first sentence must also say:\n\n\tIn order to search by some field, the information we look for must\n\tbe filled in, but we do not have to fill other fields that are not\n\tinvolved in the search.\n\nto justify the previous \"fill_project_list_info can be asked to return\nwithout getting unused fields\" patch. The rest of the log message then\nmakes good sense.\n\n> fill. This way we can limit doing expensive calculations (like\n> running git-for-each-ref to get 'age' / \"Last changed\" info) only to\n> projects which we will show as search results.\n>\n> With this commit the number of git commands used to generate search\n> results is 2* + 1, and depends on number of matched\n> projects rather than number of all projects (all repositories).\n>\n> Note: this is 'git for-each-ref' to find last activity, and 'git config'\n> for each project, and 'git --version' once.\n"},{"id":"185208","messageId":"201202222305.14387.jnareb@gmail.com","threadId":"29638","inReplyTo":"7vfwe6ng8u.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 2/8] gitweb: Option for filling only specified info in fill_project_list_info","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-22T22:05:12Z","receivedAt":"2012-02-22T22:05:12Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, 20 Feb 2012, Junio C Hamano wrote:\n> Jakub Narebski writes:\n> \n> > Introduce project_info_needs_filling($pr, $key[, \\%fill_only]), which\n> \n> That's \"project_info_needs_filling($pr, @key[, \\%fill_only])\", isn't it?\n\nYes it is. I'm sorry, I have missed that this needs fixing from the\nfirst version of patch series. Since then I have noticed that in few\ncases we fill two fields at once, so we have to ask about two fields\nat once as well.\n\n> > +# entry for given @keys needs filling if at least one of interesting keys\n> > +# in list is not present in %$project_info; key is interesting if $fill_only\n> > +# is not passed, or is empty (all keys are interesting in both of those cases),\n> > +# or if key is in $fill_only hash\n> > +#\n> > +# USAGE:\n> > +# * project_info_needs_filling($project_info, 'key', ...)\n> > +# * project_info_needs_filling($project_info, 'key', ..., \\%fill_only)\n> > +# where %fill_only = map { $_ => 1 } @fill_only;\n> \n> The reason this spells bad to me is that it gives the readers (and anybody\n> who may want to touch gitweb code) that the caller has _two_ ways to say\n> the same thing.\n> \n> If a caller is interested in finding out $key in $project_info, it can\n> either (1) pass $key as one of the earlier parameters to the function and\n> not pass any \\%fill_only, or (2) pass $key and pass \\%fill_only but after\n> making sure \\%fill_only also has $key defined.\n> \n> And if the caller passes \\%fill_only, it has to remember that it needs to\n> add $key to it; otherwise the earlier request \"I want $key\" is ignored by\n> the function.\n\nYes, you are right. All of this is caused by fill_project_list_info()\ntrying to do *two* things, instead of doing one thing and doing it well,\nas is Unix philosophy. It tried to check both that key is needed and\nthat key is wanted.\n\n> Why is it a good thing to have such a convoluted API?\n\nThanks for a sanity check.\n\n> Is it that \"fill_project_list_info\" is called by multiple callers, and\n> they do not necessarily need to see all the fields that can be filled by\n> the function (namely, age, age_string, descr, descr_long, owner, ctags and\n> category)? If that is the case, I must say that the approach to put the\n> set filtering logic inside project_info_needs_filling function smells like\n> a bad API design.\n\nYou are right.\n\n> In other words, wouldn't a callchain that uses a more natural set of API\n> functions be like this?\n> \n> \t# the top-level caller that is interested only in these two fields\n> \tfill_project_list_info($projlist, 'age', 'owner');\n> \n> \t# in fill_project_list_info()\n> \tmy $projlist = shift;\n> \tmy %wanted = map { $_ => 1 } @_;\n> \n> \tforeach my $pr (@$projlist) {\n> \t\tif (project_info_needs_filling($pr,\n> \t\t\tfilter_set(\\%wanted, 'age', 'age_string'))) {\n\nThat or\n\n \t\tif (project_info_needs_filling($pr, 'age', 'age_string') &&\n \t\t project_info_is_wanted(\\%wanted, 'age', 'age_string')) {\n\nand in your case there is no duplication of field names.\n\nfilter_set() is simply intersection of two sets, but it treats empty\n%wanted as a full set.\n\n> \t\t\t... get @activity ...\n> ($pr->{'age'}, $pr->{'age_string'}) = @activity;\n> \t\t}\n> \t\t...\n> \t}\n> \n> That way, \"needs_filling\" has to do one and only one thing well: it checks\n> if any of the fields it was asked is missing and answers. And a generic\n> set filtering helper that takes a filtering hashref and an array can be\n> used later outside of the \"needs_filling\" function.\n\nRight.\n\n> > sub project_info_needs_filling {\n> > +\tmy $fill_only = ref($_[-1]) ? pop : undef;\n> > \tmy ($project_info, @keys) = @_;\n> > \n> > \t# return List::MoreUtils::any { !exists $project_info->{$_} } @keys;\n> > \tforeach my $key (@keys) {\n> > -\t\tif (!exists $project_info->{$key}) {\n> > +\t\tif ((!$fill_only || !%$fill_only || $fill_only->{$key}) &&\n> > +\t\t !exists $project_info->{$key}) {\n> > \t\t\treturn 1;\n> > \t\t}\n> > \t}\n> > \treturn;\n> > }\n> > \n> > -\n> \n> Shouldn't the previous patch updated not to add this blank line in the\n> first place?\n\nO.K.\n\n> > # fills project list info (age, description, owner, category, forks)\n> \n> Is this enumeration up-to-date? If we cannot keep it up-to-date, it may\n> make sense to show only a few as examples and add ellipses.\n\nO.K.\n\n> > # for each project in the list, removing invalid projects from\n> > -# returned list\n> > +# returned list, or fill only specified info (removing invalid projects\n> > +# only when filling 'age').\n> \n> It is unclear what the added clause wants to say; especially, the link\n> between the mention of 'age' and 'only when' is unclear. Is asking of\n> 'age' what makes a request a notable exception to remove invalid projects?\n> Or is asking to partially fill fields what triggers the removal of invalid\n> projects, and you mentioned 'age' as an example?\n> \n> If it is the former (I am guessing it is), it would be much clearer to\n> say:\n> \n> Invalid projects are removed from the returned list if and only if you\n> ask 'age' to be filled, because of such and such reasons.\n \nO.K.\n\n\nSidenote: the reason is that to check that project is valid you really\nhave to run git command that requires repository in it[1]. Only when\nfilling 'age' and 'age_string' using git_get_last_activity() we always\nrun git command.\n\n[1] Gitweb actually uses --git-dir= parameter to git wrapper.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"185210","messageId":"201202222309.58678.jnareb@gmail.com","threadId":"29638","inReplyTo":"7v4numnfqq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 0/8] gitweb: Faster and improved project search","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-22T22:09:58Z","receivedAt":"2012-02-22T22:09:58Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, 20 Feb 2012, Junio C Hamano wrote:\n> Jakub Narebski writes:\n> \n> > [Cc-ing Junio because of his involvement in discussion about first\n> > patch in previous version of this series.]\n> >\n> > First three patches in this series are mainly about speeding up\n> > project search (and perhaps in the future also project pagination).\n> > Well, first one is unification, refactoring and future-proofing.\n> > The second and third patch could be squashed together; second adds\n> > @fill_only, but third actually uses it.\n> \n> I'll queue these three separately to a topic jn/gitweb-search-optim,\n> and fork another topic jn/gitweb-hilite-regions from there. I haven't\n> looked the latter closely, though.\n\nO.K., when rerolling the series I will resend those in separate patch\nseries: one for performance improvements for project search (less calls\nto git commands), one for match highlighting in project search ('grep',\n'commit' and other per-project searches already highlight their matches,\nthough in suboptimal way), and perhaps one for using esc_html_match_hl()\nthorough gitweb,... though with only one patch in this series for now\nperhaps it be better joined with match highlighting for project search.\n\n-- \nJakub Narebski\nPoland\n"}]}