{"thread":{"id":"29542","subject":"[PATCH 1/5] gitweb: Option for filling only specified info in fill_project_list_info","startedAt":"2012-02-04T12:47:23Z","lastAt":"2012-02-10T22:07:01Z","messageCount":21,"participants":["Jakub Narebski","Felipe Contreras","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"183821","messageId":"1328359648-29511-1-git-send-email-jnareb@gmail.com","threadId":"29542","inReplyTo":null,"subject":"[PATCH 0/5] gitweb: Faster and imrpoved project search","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-04T12:47:23Z","receivedAt":"2012-02-04T12:47:23Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"This patch series, though independent, is best applied on top of\n\"[PATCH 0/2] gitweb: Project search improvements\" series, or in other\nwords on top of 'bl/gitweb-project-filter' branch.\n\nFirst two patches in this series are about speeding up project search\n(and perhaps in the future also project pagination). Those two could\nbe squashed together.\n\nNext two patches are about making it more visible what are we\nsearching for, or rather what was matched (important especially with\nregexp match). The last patch is speculative patch about showing\nmatch using shortened description.\n\nJakub Narebski (5):\n gitweb: Option for filling only specified info in\n fill_project_list_info\n gitweb: Faster project search\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\n gitweb/gitweb.perl | 122 ++++++++++++++++++++++++++++++++++++++++++++++------\n 1 files changed, 108 insertions(+), 14 deletions(-)\n\n-- \n1.7.9\n"},{"id":"183820","messageId":"1328359648-29511-2-git-send-email-jnareb@gmail.com","threadId":"29542","inReplyTo":"1328359648-29511-1-git-send-email-jnareb@gmail.com","subject":"[PATCH 1/5] gitweb: Option for filling only specified info in fill_project_list_info","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-04T12:47:24Z","receivedAt":"2012-02-04T12:47:24Z","isPatch":true,"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\nAdding project_info_needs_filling() subroutine could have been split\ninto separate commit, but it would be subroutine without use...\n\n gitweb/gitweb.perl | 41 +++++++++++++++++++++++++++++++----------\n 1 files changed, 31 insertions(+), 10 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 913a463..b7a3752 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5185,35 +5185,56 @@ sub git_project_search_form {\n \tprint \"\\n\";\n }\n \n+# entry for given $key doesn't need filling if either $key already exists\n+# in $project_info hash, or we are interested only in subset of keys\n+# and given key is not among @fill_only.\n+sub project_info_needs_filling {\n+\tmy ($project_info, $key, @fill_only) = @_;\n+\n+\tif (!@fill_only || # we are interested in everything\n+\t grep { $key eq $_ } @fill_only) { # or key is in @fill_only\n+\t\t# check if key is already filled\n+\t\treturn !exists $project_info->{$key};\n+\t}\n+\t# uninteresting key, outside @fill_only\n+\treturn 0;\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 @projects;\n \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', @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\t($pr->{'age'}, $pr->{'age_string'}) = @activity;\n-\t\tif (!defined $pr->{'descr'}) {\n+\t\tif (project_info_needs_filling($pr, 'descr', @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 (!defined $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\tif ($show_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 && !defined $pr->{'category'}) {\n+\t\tif ($projects_list_group_categories &&\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":"183825","messageId":"1328359648-29511-3-git-send-email-jnareb@gmail.com","threadId":"29542","inReplyTo":"1328359648-29511-1-git-send-email-jnareb@gmail.com","subject":"[PATCH 2/5] gitweb: Faster project search","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-04T12:47:25Z","receivedAt":"2012-02-04T12:47:25Z","isPatch":true,"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\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 b7a3752..95ca00f 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -2989,6 +2989,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@@ -5370,12 +5374,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":"183824","messageId":"1328359648-29511-4-git-send-email-jnareb@gmail.com","threadId":"29542","inReplyTo":"1328359648-29511-1-git-send-email-jnareb@gmail.com","subject":"[PATCH 3/5] gitweb: Highlight matched part of project name when searching projects","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-04T12:47:26Z","receivedAt":"2012-02-04T12:47:26Z","isPatch":true,"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 split into a separate\ncommit, but it would be subroutine without any use.\n\n gitweb/gitweb.perl | 28 +++++++++++++++++++++++++++-\n 1 files changed, 27 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 95ca00f..aef15c8 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1715,6 +1715,30 @@ sub chop_and_escape_str {\n \t}\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+\tmy $out = '';\n+\tmy $pos = 0;\n+\tfor my $m (@matches) {\n+\t\t$out .= esc_html(substr $str, $pos, $m->[0] - $pos);\n+\t\t$out .= $cgi->span({-class => 'match'},\n+\t\t esc_html(substr $str, $m->[0], $m->[1] - $m->[0]));\n+\t\t$pos = $m->[1];\n+\t}\n+\t$out .= esc_html(substr $str, $pos);\n+\n+\treturn $out;\n+}\n+\n ## ----------------------------------------------------------------------\n ## functions returning short strings\n \n@@ -5342,7 +5366,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":"183823","messageId":"1328359648-29511-5-git-send-email-jnareb@gmail.com","threadId":"29542","inReplyTo":"1328359648-29511-1-git-send-email-jnareb@gmail.com","subject":"[PATCH 4/5] gitweb: Highlight matched part of project description when searching projects","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-04T12:47:27Z","receivedAt":"2012-02-04T12:47:27Z","isPatch":true,"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 some extra 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.\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 aef15c8..c650268 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5371,7 +5371,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":"183822","messageId":"1328359648-29511-6-git-send-email-jnareb@gmail.com","threadId":"29542","inReplyTo":"1328359648-29511-1-git-send-email-jnareb@gmail.com","subject":"[PATCH/RFC 5/5] gitweb: Highlight matched part of shortened project description","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-04T12:47:28Z","receivedAt":"2012-02-04T12:47:28Z","isPatch":true,"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 (with some code duplication with esc_html_match_hl()).\nIf match is in shortened part, then trailing \"... \" is highlighted.\n\nSigned-off-by: Jakub Narebski \n---\nThis is an RFC because of code duplication between esc_html_match_hl()\nand esc_html_match_hl_chopped().\n\n gitweb/gitweb.perl | 41 ++++++++++++++++++++++++++++++++++++++++-\n 1 files changed, 40 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex c650268..174b4d2 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1739,6 +1739,44 @@ sub esc_html_match_hl {\n \treturn $out;\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($chopped) unless defined $regexp;\n+\treturn esc_html_match_hl($str, $regexp) if ($str eq $chopped);\n+\n+\tmy @matches;\n+\twhile ($str =~ /$regexp/g) {\n+\t\tpush @matches, [$-[0], $+[0]];\n+\t}\n+\treturn esc_html($chopped) unless @matches;\n+\n+\tmy $tail = \"... \";\n+\t$chopped =~ s/\\Q$tail\\E$//; # see chop_str\n+\tmy $len = length($chopped);\n+\tmy $out = '';\n+\tmy $pos = 0;\n+\tfor my $m (@matches) {\n+\t\tif ($m->[0] > $len) {\n+\t\t\t$tail = $cgi->span({-class => 'match'}, $tail);\n+\t\t\tlast;\n+\t\t}\n+\t\t$out .= esc_html(substr $str, $pos, $m->[0] - $pos);\n+\t\t$out .= $cgi->span({-class => 'match'},\n+\t\t esc_html(substr $chopped, $m->[0],\n+\t\t ($m->[1] > $len ? $len : $m->[1]) - $m->[0]));\n+\t\tif ($m->[1] > $len) {\n+\t\t\t$tail = $cgi->span({-class => 'match'}, $tail);\n+\t\t\t$pos = $len;\n+\t\t\tlast;\n+\t\t}\n+\t\t$pos = $m->[1];\n+\t}\n+\t$out .= esc_html(substr($chopped, $pos)).$tail;\n+\n+\treturn $out;\n+}\n+\n ## ----------------------------------------------------------------------\n ## functions returning short strings\n \n@@ -5372,7 +5410,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":"183849","messageId":"201202041956.36309.jnareb@gmail.com","threadId":"29542","inReplyTo":"1328359648-29511-6-git-send-email-jnareb@gmail.com","subject":"[PATCH/RFCv2 5/5] gitweb: Highlight matched part of shortened project description","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-04T18:56:35Z","receivedAt":"2012-02-04T18:56:35Z","isPatch":true,"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---\nThis version removes code duplication from v1.\n\nIt is still marked as RFC, because I am not sure if it is right way\nto highlight match in shortened string, or if we better use full string,\nor full string if match is in chopped part.\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 c650268..f71afe0 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1720,11 +1720,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($chopped) 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 \tmy $out = '';\n \tmy $pos = 0;\n@@ -5372,7 +5407,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":"183859","messageId":"CAMP44s2gLvXXCTTpAV78=DeJA9dSV793+bx=yJmns7vCwegagQ@mail.gmail.com","threadId":"29542","inReplyTo":"1328359648-29511-1-git-send-email-jnareb@gmail.com","subject":"Re: [PATCH 0/5] gitweb: Faster and imrpoved project search","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-04T20:07:36Z","receivedAt":"2012-02-04T20:07:36Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Feb 4, 2012 at 2:47 PM, Jakub Narebski wrote:\n\nTypo: improved\n\n-- \nFelipe Contreras\n"},{"id":"183863","messageId":"201202042159.00950.jnareb@gmail.com","threadId":"29542","inReplyTo":"CAMP44s2gLvXXCTTpAV78=DeJA9dSV793+bx=yJmns7vCwegagQ@mail.gmail.com","subject":"Re: [PATCH 0/5] gitweb: Faster and imrpoved project search","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-04T20:59:00Z","receivedAt":"2012-02-04T20:59:00Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sat, 4 Feb 2012, Felipe Contreras wrote:\n> \n> Typo: improved\n\nYeah, I have noticed this just as I have send it, but because it is in \n_cover letter_ rather than in commit message...\n\n-- \nJakub Narebski\nPoland\n"},{"id":"184277","messageId":"7vhayzvf38.fsf@alter.siamese.dyndns.org","threadId":"29542","inReplyTo":"1328359648-29511-2-git-send-email-jnareb@gmail.com","subject":"Re: [PATCH 1/5] gitweb: Option for filling only specified info in fill_project_list_info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-09T22:04:11Z","receivedAt":"2012-02-09T22:04:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> This could have been squashed with the next commit, but this way it is\n> pure refactoring that shouldn't change gitweb behavior.\n\nIt is not obvious why this shouldn't change gitweb behaviour from the\npatch text.\n\nThe old code says \"Unconditionally call git_get_last_activity(), and if we\nfound nothing, do not do anything extra for this project\". The new code\nsays \"we do that but only if the project does not know its 'age' yet\".\n\nThe lack of any real use of @fill_only in this patch also makes it hard to\njudge if the new API gives a useful semantics. I would, without looking\nat the real usage in 2/5 patch, naïvely expect that such a lazy filling\nscheme would say \"I am going to use A, B and C; I want to know if any of\nthem is missing, because I need values for all of them and I am going to\ncall a helper function to fill them if any of them is missing. Having A\nand B is not enough for the purpose of this query, because I still need to\nknow C and I would call the helper function that computes all of them in\nsuch a case. Even though it might be wasteful to recompute A and B,\ncomputing all three at once is the only helper function available to me\".\n\nSo for a person who does not have access to the real usage of the new API,\nbeing able to give only a single $key *appears* make no sense at all, and\nalso the meaning of the @fill_only parameter is unclear, especially the\npart that checks if that single $key appears in @fill_only.\n\n> Adding project_info_needs_filling() subroutine could have been split\n> into separate commit, but it would be subroutine without use...\n>\n> gitweb/gitweb.perl | 41 +++++++++++++++++++++++++++++++----------\n> 1 files changed, 31 insertions(+), 10 deletions(-)\n>\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 913a463..b7a3752 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -5185,35 +5185,56 @@ sub git_project_search_form {\n> \tprint \"\\n\";\n> }\n> \n> +# entry for given $key doesn't need filling if either $key already exists\n> +# in $project_info hash, or we are interested only in subset of keys\n> +# and given key is not among @fill_only.\n> +sub project_info_needs_filling {\n> +\tmy ($project_info, $key, @fill_only) = @_;\n> +\n> +\tif (!@fill_only || # we are interested in everything\n> +\t grep { $key eq $_ } @fill_only) { # or key is in @fill_only\n> +\t\t# check if key is already filled\n> +\t\treturn !exists $project_info->{$key};\n> +\t}\n> +\t# uninteresting key, outside @fill_only\n> +\treturn 0;\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 @projects;\n> \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', @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\t($pr->{'age'}, $pr->{'age_string'}) = @activity;\n> -\t\tif (!defined $pr->{'descr'}) {\n> +\t\tif (project_info_needs_filling($pr, 'descr', @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 (!defined $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\tif ($show_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 && !defined $pr->{'category'}) {\n> +\t\tif ($projects_list_group_categories &&\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"},{"id":"184279","messageId":"201202092336.48772.jnareb@gmail.com","threadId":"29542","inReplyTo":"7vhayzvf38.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/5] gitweb: Option for filling only specified info in fill_project_list_info","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-09T22:36:48Z","receivedAt":"2012-02-09T22:36:48Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 9 Feb 2012, Junio C Hamano wrote:\n> Jakub Narebski writes:\n> \n> > This could have been squashed with the next commit, but this way it is\n> > pure refactoring that shouldn't change gitweb behavior.\n> \n> It is not obvious why this shouldn't change gitweb behaviour from the\n> patch text.\n> \n> The old code says \"Unconditionally call git_get_last_activity(), and if we\n> found nothing, do not do anything extra for this project\". The new code\n> says \"we do that but only if the project does not know its 'age' yet\".\n\nWell, till next patch in this series gitweb always filled some parts of\nproject info by processing $projects_list i.e. reading a file or scanning\na directory. Then fill_project_list_info was called to fill the rest\nof projects data.\n\nWhich parts were filled in first step depends on whether $projects_list\nis a file with list of projects or a directory to scan, but 'age' was \n_never_ filled.\n\nBut I agree that it is not obvious.\n\n\nI think that better solution would be to either squash 1/5 and 2/5, or\nmake 1/5 only about introduction of project_info_needs_filling(), without\n@fill_only but with the 'age' check change.\n\n> The lack of any real use of @fill_only in this patch also makes it hard to\n> judge if the new API gives a useful semantics. I would, without looking\n> at the real usage in 2/5 patch, naïvely expect that such a lazy filling\n> scheme would say \"I am going to use A, B and C; I want to know if any of\n> them is missing, because I need values for all of them and I am going to\n> call a helper function to fill them if any of them is missing. Having A\n> and B is not enough for the purpose of this query, because I still need to\n> know C and I would call the helper function that computes all of them in\n> such a case. Even though it might be wasteful to recompute A and B,\n> computing all three at once is the only helper function available to me\".\n> \n> So for a person who does not have access to the real usage of the new API,\n> being able to give only a single $key *appears* make no sense at all, and\n> also the meaning of the @fill_only parameter is unclear, especially the\n> part that checks if that single $key appears in @fill_only.\n\nfill_project_list_info() is, at least after a fix with 'age', about filling\ninformation that is not already present. If @fill_only is nonempty, it\nfills only selected information, again only if it is not already present.\n@fill_only empty means no restrictions... which probably is not very obvious,\nbut is documented.\n\nproject_info_needs_filling() returns true if $key is not filled and is\ninteresting.\n \n> > Adding project_info_needs_filling() subroutine could have been split\n> > into separate commit, but it would be subroutine without use...\n> >\n> > gitweb/gitweb.perl | 41 +++++++++++++++++++++++++++++++----------\n> > 1 files changed, 31 insertions(+), 10 deletions(-)\n> >\n> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> > index 913a463..b7a3752 100755\n> > --- a/gitweb/gitweb.perl\n> > +++ b/gitweb/gitweb.perl\n> > @@ -5185,35 +5185,56 @@ sub git_project_search_form {\n> > \tprint \"\\n\";\n> > }\n> > \n> > +# entry for given $key doesn't need filling if either $key already exists\n> > +# in $project_info hash, or we are interested only in subset of keys\n> > +# and given key is not among @fill_only.\n> > +sub project_info_needs_filling {\n> > +\tmy ($project_info, $key, @fill_only) = @_;\n\nSo in new version @fill_only will be removed...\n\n> > +\n> > +\tif (!@fill_only || # we are interested in everything\n> > +\t grep { $key eq $_ } @fill_only) { # or key is in @fill_only\n> > +\t\t# check if key is already filled\n> > +\t\treturn !exists $project_info->{$key};\n\n...and this will be the only line of project_info_needs_filling() wrapper.\n\n> > +\t}\n> > +\t# uninteresting key, outside @fill_only\n> > +\treturn 0;\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\nHere also @fill_only would be not present.\n\n> > \tmy @projects;\n> > \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', @fill_only)) {\n\nThis change makes 'age' not special.\n\n[...]\n\n-- \nJakub Narebski\nPoland\n"},{"id":"184284","messageId":"7v4nuzvbnr.fsf@alter.siamese.dyndns.org","threadId":"29542","inReplyTo":"201202092336.48772.jnareb@gmail.com","subject":"Re: [PATCH 1/5] gitweb: Option for filling only specified info in fill_project_list_info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-09T23:18:16Z","receivedAt":"2012-02-09T23:18:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n>> The lack of any real use of @fill_only in this patch also makes it hard to\n>> judge if the new API gives a useful semantics. I would, without looking\n>> at the real usage in 2/5 patch, naïvely expect that such a lazy filling\n>> scheme would say \"I am going to use A, B and C; I want to know if any of\n>> them is missing, because I need values for all of them and I am going to\n>> call a helper function to fill them if any of them is missing. Having A\n>> and B is not enough for the purpose of this query, because I still need to\n>> know C and I would call the helper function that computes all of them in\n>> such a case. Even though it might be wasteful to recompute A and B,\n>> computing all three at once is the only helper function available to me\".\n>> \n>> So for a person who does not have access to the real usage of the new API,\n>> being able to give only a single $key *appears* make no sense at all, and\n>> also the meaning of the @fill_only parameter is unclear, especially the\n>> part that checks if that single $key appears in @fill_only.\n>\n> ...\n> information that is not already present. If @fill_only is nonempty, it\n> fills only selected information, again only if it is not already present.\n> @fill_only empty means no restrictions... which probably is not very obvious,\n> but is documented.\n>\n> project_info_needs_filling() returns true if $key is not filled and is\n> interesting.\n\nThat still does not answer the fundamental issues I had with the presented\nAPI: why does it take only a single $key (please re-read my \"A, B and C\"\nexample), and what does that single $key intersecting with @fill_only have\nanything to do with \"needs-filling\"?\n\nAfter all, that 'age' check actually wants to fill 'age' and 'age_string'\nin the project. Even if some other codepath starts filling 'age' in the\nproject with a later change, the current callers of fill_project_list_info\nexpects _both_ to be filled. So \"I know the current implementation fills\nboth at the same time, so checking 'age' alone is sufficient\" is not an\nanswer that shows good taste in the API design.\n"},{"id":"184288","messageId":"201202100052.26399.jnareb@gmail.com","threadId":"29542","inReplyTo":"7v4nuzvbnr.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/5] gitweb: Option for filling only specified info in fill_project_list_info","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-09T23:52:26Z","receivedAt":"2012-02-09T23:52:26Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 10 Feb 2012, Junio C Hamano wrote:\n> Jakub Narebski writes:\n> \n>>> The lack of any real use of @fill_only in this patch also makes it hard to\n>>> judge if the new API gives a useful semantics. I would, without looking\n>>> at the real usage in 2/5 patch, naïvely expect that such a lazy filling\n>>> scheme would say \"I am going to use A, B and C; I want to know if any of\n>>> them is missing, because I need values for all of them and I am going to\n>>> call a helper function to fill them if any of them is missing. Having A\n>>> and B is not enough for the purpose of this query, because I still need to\n>>> know C and I would call the helper function that computes all of them in\n>>> such a case. Even though it might be wasteful to recompute A and B,\n>>> computing all three at once is the only helper function available to me\".\n>>> \n>>> So for a person who does not have access to the real usage of the new API,\n>>> being able to give only a single $key *appears* make no sense at all, and\n>>> also the meaning of the @fill_only parameter is unclear, especially the\n>>> part that checks if that single $key appears in @fill_only.\n>>\n>> ...\n>> information that is not already present. If @fill_only is nonempty, it\n>> fills only selected information, again only if it is not already present.\n>> @fill_only empty means no restrictions... which probably is not very obvious,\n>> but is documented.\n>>\n>> project_info_needs_filling() returns true if $key is not filled and is\n>> interesting.\n> \n> That still does not answer the fundamental issues I had with the presented\n> API: why does it take only a single $key (please re-read my \"A, B and C\"\n> example), and what does that single $key intersecting with @fill_only have\n> anything to do with \"needs-filling\"?\n\nproject_info_needs_filling() in absence of @fill_only is just a thin\nwrapper around \"!defined $pr->{$key}\", it checks for each key if it needs\nto be filled.\n\nIt is used like this\n\n if (project_info_needs_filled(\"A\", \"A, B, C\")) {\n fill A\n }\n if (project_info_needs_filled(\"B\", \"A, B, C\")) {\n fill B\n }\n ...\n \n> After all, that 'age' check actually wants to fill 'age' and 'age_string'\n> in the project. Even if some other codepath starts filling 'age' in the\n> project with a later change, the current callers of fill_project_list_info\n> expects _both_ to be filled. So \"I know the current implementation fills\n> both at the same time, so checking 'age' alone is sufficient\" is not an\n> answer that shows good taste in the API design.\n\nIt is not as much matter of API, as the use of checks in loop in \nfill_project_list_info().\n\nWhat is now\n\n my (@activity) = git_get_last_activity($pr->{'path'});\n unless (@activity) {\n \tnext PROJECT;\n }\n ($pr->{'age'}, $pr->{'age_string'}) = @activity;\n\nshould be\n\n if (!defined $pr->{'age'} ||\n !defined $pr->{'age_string'}) {\n \tmy (@activity) = git_get_last_activity($pr->{'path'});\n \tunless (@activity) {\n \t\tnext PROJECT;\n \t}\n \t($pr->{'age'}, $pr->{'age_string'}) = @activity;\n }\n\nwhich would translate to\n\n if (project_info_needs_filled($pr, 'age') ||\n project_info_needs_filled($pr, 'age_string') {\n \tmy (@activity) = git_get_last_activity($pr->{'path'});\n \tunless (@activity) {\n \t\tnext PROJECT;\n \t}\n \t($pr->{'age'}, $pr->{'age_string'}) = @activity;\n }\n\nand then with @fill_only\n\n if (project_info_needs_filled($pr, 'age', @fill_only) ||\n project_info_needs_filled($pr, 'age_string', @fill_only) {\n \tmy (@activity) = git_get_last_activity($pr->{'path'});\n \tunless (@activity) {\n \t\tnext PROJECT;\n \t}\n \t($pr->{'age'}, $pr->{'age_string'}) = @activity;\n }\n\nThe same should be done for 'descr_long' and 'descr' which are also\nalways filled together.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"184289","messageId":"7vvcnftvb5.fsf@alter.siamese.dyndns.org","threadId":"29542","inReplyTo":"201202100052.26399.jnareb@gmail.com","subject":"Re: [PATCH 1/5] gitweb: Option for filling only specified info in fill_project_list_info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-09T23:56:46Z","receivedAt":"2012-02-09T23:56:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> On Fri, 10 Feb 2012, Junio C Hamano wrote:\n> ...\n>> That still does not answer the fundamental issues I had with the presented\n>> API: why does it take only a single $key (please re-read my \"A, B and C\"\n>> example), and what does that single $key intersecting with @fill_only have\n>> anything to do with \"needs-filling\"?\n>\n> project_info_needs_filling() in absence of @fill_only is just a thin\n> wrapper around \"!defined $pr->{$key}\", it checks for each key if it needs\n> to be filled.\n>\n> It is used like this\n>\n> if (project_info_needs_filled(\"A\", \"A, B, C\")) {\n> fill A\n> }\n> if (project_info_needs_filled(\"B\", \"A, B, C\")) {\n> fill B\n> }\n> ...\n> \n>> After all, that 'age' check actually wants to fill 'age' and 'age_string'\n>> in the project. Even if some other codepath starts filling 'age' in the\n>> project with a later change, the current callers of fill_project_list_info\n>> expects _both_ to be filled. So \"I know the current implementation fills\n>> both at the same time, so checking 'age' alone is sufficient\" is not an\n>> answer that shows good taste in the API design.\n>\n> It is not as much matter of API, as the use of checks in loop in \n> fill_project_list_info().\n>\n> What is now\n>\n> my (@activity) = git_get_last_activity($pr->{'path'});\n> unless (@activity) {\n> \tnext PROJECT;\n> }\n> ($pr->{'age'}, $pr->{'age_string'}) = @activity;\n>\n> should be\n>\n> if (!defined $pr->{'age'} ||\n> !defined $pr->{'age_string'}) {\n> \tmy (@activity) = git_get_last_activity($pr->{'path'});\n> \tunless (@activity) {\n> \t\tnext PROJECT;\n> \t}\n> \t($pr->{'age'}, $pr->{'age_string'}) = @activity;\n> }\n\nHuh? Compare that with what you wrote above \"It is used like this\". This\nis *NOT* using the API like that. The caller knows it wants both age and\nage-string, and if even one of them is missing, do the work to fill both.\n\nSo why isn't the info-needs-filled API not checking _both_ with a single\ncall? It is only because you designed the API to accept only a single $key\ninstead of list of \"here are what I care about\".\n"},{"id":"184353","messageId":"201202101456.45944.jnareb@gmail.com","threadId":"29542","inReplyTo":"7vvcnftvb5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/5] gitweb: Option for filling only specified info in fill_project_list_info","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-10T13:56:44Z","receivedAt":"2012-02-10T13:56:44Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 10 Feb 2012, Junio C Hamano wrote:\n> Jakub Narebski writes:\n>> On Fri, 10 Feb 2012, Junio C Hamano wrote:\n>> ...\n>>> That still does not answer the fundamental issues I had with the presented\n>>> API: why does it take only a single $key (please re-read my \"A, B and C\"\n>>> example), and what does that single $key intersecting with @fill_only have\n>>> anything to do with \"needs-filling\"?\n>>\n>> project_info_needs_filling() in absence of @fill_only is just a thin\n>> wrapper around \"!defined $pr->{$key}\", it checks for each key if it needs\n>> to be filled.\n>>\n>> It is used like this\n>>\n>> if (project_info_needs_filled(\"A\", \"A, B, C\")) {\n>> fill A\n>> }\n>> if (project_info_needs_filled(\"B\", \"A, B, C\")) {\n>> fill B\n>> }\n>> ...\n>> \n>>> After all, that 'age' check actually wants to fill 'age' and 'age_string'\n>>> in the project. [...]\n\nThanks for noticing that... though I think more important is that some\nfurther command would mark only 'age_string' as needed, and \nfill_project_list_info() wouldn't notice that it needs to run\ngit_get_last_activity() if it checks only 'age'.\n\n>> It is not as much matter of API, as the use of checks in loop in \n>> fill_project_list_info().\n>>\n>> What is now\n>>\n>> my (@activity) = git_get_last_activity($pr->{'path'});\n>> unless (@activity) {\n>> \tnext PROJECT;\n>> }\n>> ($pr->{'age'}, $pr->{'age_string'}) = @activity;\n>>\n>> should be\n>>\n>> if (!defined $pr->{'age'} ||\n>> !defined $pr->{'age_string'}) {\n>> \tmy (@activity) = git_get_last_activity($pr->{'path'});\n>> \tunless (@activity) {\n>> \t\tnext PROJECT;\n>> \t}\n>> \t($pr->{'age'}, $pr->{'age_string'}) = @activity;\n>> }\n> \n> Huh? Compare that with what you wrote above \"It is used like this\". This\n> is *NOT* using the API like that. The caller knows it wants both age and\n> age-string, and if even one of them is missing, do the work to fill both.\n> \n> So why isn't the info-needs-filled API not checking _both_ with a single\n> call? It is only because you designed the API to accept only a single $key\n> instead of list of \"here are what I care about\".\n\nPassing two lists as parameters is a bit harder and makes for more\ncomplicated API. \n\nSo what you mean is that instead of proposed\n\n if (project_info_needs_filled($pr, 'age', @fill_only) ||\n project_info_needs_filled($pr, 'age_string', @fill_only) {\n\nit should be\n\n if (project_info_needs_filled($pr, 'age', 'age_string', \\@fill_only) {\n\nor\n\n if (project_info_needs_filled($pr, ['age', 'age_string'], @fill_only) {\n\n(with either \"..., 'owner', ...\" or \"..., [ 'owner' ], ...\" for single-key\nfilling), or\n\n if (project_info_needs_filled($pr, ['age', 'age_string'], \\@fill_only) {\n\nIs it?\n\n-- \nJakub Narebski\nPoland\n"},{"id":"184368","messageId":"7v1uq2shwg.fsf@alter.siamese.dyndns.org","threadId":"29542","inReplyTo":"201202101456.45944.jnareb@gmail.com","subject":"Re: [PATCH 1/5] gitweb: Option for filling only specified info in fill_project_list_info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-10T17:43:59Z","receivedAt":"2012-02-10T17:43:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> (with either \"..., 'owner', ...\" or \"..., [ 'owner' ], ...\" for single-key\n> filling), or\n>\n> if (project_info_needs_filled($pr, ['age', 'age_string'], \\@fill_only) {\n>\n> Is it?\n\nWhatever. I am not sure what @fill_only is needed for, if the name stands\nfor \"only fill these fields, if this argument is empty\". After all,\ndoesn't the above example callsite, without \", \\@fill_only\" at the end,\nsay \"I am going to use age and age_string, so these two need to be filled\"\nalready?\n"},{"id":"184374","messageId":"201202101917.52908.jnareb@gmail.com","threadId":"29542","inReplyTo":"7v1uq2shwg.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/5] gitweb: Option for filling only specified info in fill_project_list_info","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-10T18:17:52Z","receivedAt":"2012-02-10T18:17:52Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 10 Feb 2012, Junio C Hamano wrote:\n> Jakub Narebski writes:\n> \n> > (with either \"..., 'owner', ...\" or \"..., [ 'owner' ], ...\" for single-key\n> > filling), or\n> >\n> > if (project_info_needs_filled($pr, ['age', 'age_string'], \\@fill_only) {\n> >\n> > Is it?\n> \n> Whatever. I am not sure what @fill_only is needed for, if the name stands\n> for \"only fill these fields, if this argument is empty\". After all,\n> doesn't the above example callsite, without \", \\@fill_only\" at the end,\n> say \"I am going to use age and age_string, so these two need to be filled\"\n> already?\n\nNo, the above example callsite says \"Do I need to fill either 'age' or\n'age_string'\", and is followed by actually filling this info.\n\nCurrently the code flow WRT. project information goes like this:\n\n git_get_projects_list()\n # this fills some of project info; what is filled depends on whether\n # $projects_list is a file with list of project or directory to scan\n filter_forks_from_projects_list()\n # this does not use project info\n fill_project_list_info()\n # this fills all the rest of project info on reduced list\n search_projects_list()\n # this uses some of project info\n\nAfter 2/5 the code goes like this:\n\n git_get_projects_list()\n # this fills some of project info; what is filled depends on whether\n # $projects_list is a file with list of project or directory to scan\n filter_forks_from_projects_list()\n # this does not use project info\n fill_project_list_info()\n # this fills the rest of project info required for search on reduced list\n search_projects_list()\n # this uses some of project info\n fill_project_list_info()\n # this fills all the rest of project info on further reduced list\n\nIf @fill_only is empty, it means for fill_project_list_info to fill\nall the data, if it is not empty it means that those fields needs to\nbe filled.\n\nThe code of fill_project_list_info goes like this\n\n if (do we need to fill 'age' or 'age_string'?) {\n fill 'age' and 'age_string'\n }\n if (do we need to fill 'desc_long' or 'descr'?) {\n fill 'descr_long' and 'descr'\n }\n if (we are interested in 'ctags' &&\n do we need to fill 'ctags'?) {\n fill 'ctags'\n }\n ...\n\n\n-- \nJakub Narebski\nPoland\n"},{"id":"184382","messageId":"7vsjiipiyu.fsf@alter.siamese.dyndns.org","threadId":"29542","inReplyTo":"201202101917.52908.jnareb@gmail.com","subject":"Re: [PATCH 1/5] gitweb: Option for filling only specified info in fill_project_list_info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-10T19:49:13Z","receivedAt":"2012-02-10T19:49:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> If @fill_only is empty, it means for fill_project_list_info to fill\n> all the data, if it is not empty it means that those fields needs to\n> be filled.\n\nThat is exactly what I am questioning. Do you need \"I need all these\nfields to be present\" and \"I will fill these other fields\" (which is what\n@fill_only is about, no?) that is different from the former?\n\n> The code of fill_project_list_info goes like this\n>\n> if (do we need to fill 'age' or 'age_string'?) {\n> fill 'age' and 'age_string'\n> }\n> if (do we need to fill 'desc_long' or 'descr'?) {\n> fill 'descr_long' and 'descr'\n> }\n> if (we are interested in 'ctags' &&\n> do we need to fill 'ctags'?) {\n> fill 'ctags'\n> }\n> ...\n\nExactly. Why do you need @fill_only at all? If you are interested in\nctags and you want to make sure ctags is available, the question you want\nto ask the helper function is \"Does the project structure already have\nctags field?\". Why does the helper function needs to know anything else?\n"},{"id":"184401","messageId":"201202102230.13193.jnareb@gmail.com","threadId":"29542","inReplyTo":"7vsjiipiyu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/5] gitweb: Option for filling only specified info in fill_project_list_info","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-10T21:30:12Z","receivedAt":"2012-02-10T21:30:12Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n> Jakub Narebski writes:\n> \n> > If @fill_only is empty, it means for fill_project_list_info to fill\n> > all the data, if it is not empty it means that those fields needs to\n> > be filled.\n> \n> That is exactly what I am questioning. Do you need \"I need all these\n> fields to be present\" and \"I will fill these other fields\" (which is what\n> @fill_only is about, no?) that is different from the former?\n\n\"I need all these fields\" is a property of callsite. fill_project_list_info()\nwill be called more than one time in 2/5 to incrementally complete project\ninfo, see below.\n \n\"I will fill those fields\" is a property of piece of code protected by\na conditional inside fill_project_list_info().\n\n> > The code of fill_project_list_info goes like this\n> >\n> > if (do we need to fill 'age' or 'age_string'?) {\n> > fill 'age' and 'age_string'\n> > }\n> > if (do we need to fill 'desc_long' or 'descr'?) {\n> > fill 'descr_long' and 'descr'\n> > }\n> > if (we are interested in 'ctags' &&\n> > do we need to fill 'ctags'?) {\n> > fill 'ctags'\n> > }\n> > ...\n> \n> Exactly. Why do you need @fill_only at all? If you are interested in\n> ctags and you want to make sure ctags is available, the question you want\n> to ask the helper function is \"Does the project structure already have\n> ctags field?\". Why does the helper function needs to know anything else?\n\nIt is to support incremental filling of project info. The code is to\ngo like this:\n\n create\n filter\n fill part\n filter\n fill rest\n\nWe need @fill_only for the \"fill part\". As filling project info is\npotentially expensive (especially the 'age' field), doing it on narrowed\n(filtered) list of project is a performance win. That is what 2/5 is\nabout.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"184405","messageId":"7vobt6nz31.fsf@alter.siamese.dyndns.org","threadId":"29542","inReplyTo":"201202102230.13193.jnareb@gmail.com","subject":"Re: [PATCH 1/5] gitweb: Option for filling only specified info in fill_project_list_info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-10T21:44:02Z","receivedAt":"2012-02-10T21:44:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n>> Exactly. Why do you need @fill_only at all? If you are interested in\n>> ctags and you want to make sure ctags is available, the question you want\n>> to ask the helper function is \"Does the project structure already have\n>> ctags field?\". Why does the helper function needs to know anything else?\n>\n> It is to support incremental filling of project info. The code is to\n> go like this:\n>\n> create\n> filter\n> fill part\n> filter\n> fill rest\n>\n> We need @fill_only for the \"fill part\".\n\nAgain, why?\n\n> As filling project info is\n> potentially expensive (especially the 'age' field),\n\nSo you wouldn't say \"I am interested in 'age' field\" but show interest in,\nand fill, cheaper fields in the earlier \"fill\" calls, and then...\n\n> doing it on narrowed\n> (filtered) list of project is a performance win.\n\n... you drop uninteresting projects by using the partially filled\ninformation, and show interest in more expensive 'age' in the later round\nfor surviving projects.\n\nIt still does not explain why you need @fill_only.\nHrm...\n"},{"id":"184409","messageId":"201202102307.02055.jnareb@gmail.com","threadId":"29542","inReplyTo":"7vobt6nz31.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/5] gitweb: Option for filling only specified info in fill_project_list_info","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-02-10T22:07:01Z","receivedAt":"2012-02-10T22:07:01Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n> Jakub Narebski writes:\n> \n>>> Exactly. Why do you need @fill_only at all? If you are interested in\n>>> ctags and you want to make sure ctags is available, the question you want\n>>> to ask the helper function is \"Does the project structure already have\n>>> ctags field?\". Why does the helper function needs to know anything else?\n>>\n>> It is to support incremental filling of project info. The code is to\n>> go like this:\n>>\n>> create\n>> filter\n>> fill part\n>> filter\n>> fill rest\n>>\n>> We need @fill_only for the \"fill part\".\n> \n> Again, why?\n\nSo fill_project_list_info() knows what needs to be filled (notice: not\n\"what to fill\"), as filter might need different fields in project info\nto do its work.\n \n> > As filling project info is\n> > potentially expensive (especially the 'age' field),\n> \n> So you wouldn't say \"I am interested in 'age' field\" but show interest in,\n> and fill, cheaper fields in the earlier \"fill\" calls, and then...\n\nIt is not about cheaper, it is about required by filter. It happens that\nit is cheaper.\n\n> > doing it on narrowed\n> > (filtered) list of project is a performance win.\n> \n> ... you drop uninteresting projects by using the partially filled\n> information, and show interest in more expensive 'age' in the later round\n> for surviving projects.\n> \n> It still does not explain why you need @fill_only.\n\nSo I can use single subroutine fill_project_list_info() to fill what is\nrequired, and to fill the rest of info.\n\n create\n filter\n fill part('path', 'descr')\n filter('path', 'descr')\n fill rest\n\nor\n\n create\n filter\n fill part('ctags')\n filter('ctags')\n fill rest\n\nAm I clear?\n-- \nJakub Narebski\nPoland\n"}]}