{"thread":{"id":"25537","subject":"[PATCHv6 01/10] gitweb: introduce remote_heads feature","startedAt":"2010-10-24T10:45:26Z","lastAt":"2010-11-08T13:41:42Z","messageCount":33,"participants":["Giuseppe Bilotta","Jakub Narebski"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"154301","messageId":"1287917136-26103-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25537","inReplyTo":null,"subject":"[PATCHv6 00/10] gitweb: remote_heads feature","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-10-24T10:45:26Z","receivedAt":"2010-10-24T10:45:26Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"In this 6th version of the remote heads feature for gitweb, much of the\ninternal API has been cleaned up thanks to the suggestion from Jakub and\nothers. There were also some other stylistic changes, and a few patches were\nreordered/merged.\n\nThe first 4 patches are rather straightforward and can probably go\nstraight in. The 5th patch is a bugfix for something that is only\ntriggered by the name manipulation done with the remote heads, but can\nprobably be useful even without the rest of the series.\n\nPatch 7 provides 'single remote view', depending on patch 6 for\nimproved visuals of the page header.\n\nPatches 8 and 9 provide some infrastructure for the grouped remote heads\ndisplay introduced in the last patch.\n\nAs usual, you can see it live @ http://git.oblomov.eu (look at the rbot\nproject in particular for the \"lots of remotes\" case), comments welcome.\n\nGiuseppe Bilotta (10):\n gitweb: introduce remote_heads feature\n gitweb: git_get_heads_list accepts an optional list of refs.\n gitweb: separate heads and remotes lists\n gitweb: nagivation menu for tags, heads and remotes\n gitweb: use fullname as hash_base in heads link\n gitweb: allow action specialization in page header\n gitweb: remotes view for a single remote\n gitweb: refactor repository URL printing\n gitweb: provide a routine to display (sub)sections\n gitweb: group remote heads by remote\n\n gitweb/gitweb.perl | 283 ++++++++++++++++++++++++++++++++++++++++++++--\n gitweb/static/gitweb.css | 6 +\n 2 files changed, 279 insertions(+), 10 deletions(-)\n\n-- \n1.7.3.68.g6ec8\n"},{"id":"154300","messageId":"1287917136-26103-2-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25537","inReplyTo":"1287917136-26103-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv6 01/10] gitweb: introduce remote_heads feature","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-10-24T10:45:27Z","receivedAt":"2010-10-24T10:45:27Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"With this feature enabled, remote heads are retrieved (and displayed)\nwhen getting (and displaying) the heads list. Typical usage would be for\nlocal repository browsing, e.g. by using git-instaweb (or even a more\npermanent gitweb setup), to check the repository status and the relation\nbetween tracking branches and the originating remotes.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 18 ++++++++++++++++--\n 1 files changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 253f41a..0e71749 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -493,6 +493,18 @@ our %feature = (\n \t\t'sub' => sub { feature_bool('highlight', @_) },\n \t\t'override' => 0,\n \t\t'default' => [0]},\n+\n+\t# Enable displaying of remote heads in the heads list\n+\n+\t# To enable system wide have in $GITWEB_CONFIG\n+\t# $feature{'remote_heads'}{'default'} = [1];\n+\t# To have project specific config enable override in $GITWEB_CONFIG\n+\t# $feature{'remote_heads'}{'override'} = 1;\n+\t# and in project config gitweb.remote_heads = 0|1;\n+\t'remote_heads' => {\n+\t\t'sub' => sub { feature_bool('remote_heads', @_) },\n+\t\t'override' => 0,\n+\t\t'default' => [0]},\n );\n \n sub gitweb_get_feature {\n@@ -3160,10 +3172,12 @@ sub git_get_heads_list {\n \tmy $limit = shift;\n \tmy @headslist;\n \n+\tmy $remote_heads = gitweb_check_feature('remote_heads');\n+\n \topen my $fd, '-|', git_cmd(), 'for-each-ref',\n \t\t($limit ? '--count='.($limit+1) : ()), '--sort=-committerdate',\n \t\t'--format=%(objectname) %(refname) %(subject)%00%(committer)',\n-\t\t'refs/heads'\n+\t\t'refs/heads', ($remote_heads ? 'refs/remotes' : ())\n \t\tor return;\n \twhile (my $line = <$fd>) {\n \t\tmy %ref_item;\n@@ -3174,7 +3188,7 @@ sub git_get_heads_list {\n \t\tmy ($committer, $epoch, $tz) =\n \t\t\t($committerinfo =~ /^(.*) ([0-9]+) (.*)$/);\n \t\t$ref_item{'fullname'} = $name;\n-\t\t$name =~ s!^refs/heads/!!;\n+\t\t$name =~ s!^refs/(?:head|remote)s/!!;\n \n \t\t$ref_item{'name'} = $name;\n \t\t$ref_item{'id'} = $hash;\n-- \n1.7.3.68.g6ec8\n"},{"id":"154302","messageId":"1287917136-26103-3-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25537","inReplyTo":"1287917136-26103-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv6 02/10] gitweb: git_get_heads_list accepts an optional list of refs.","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-10-24T10:45:28Z","receivedAt":"2010-10-24T10:45:28Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"git_get_heads_list(limit, class1, class2, ...) can now be used to retrieve\nrefs/class1, refs/class2 etc. Defaults to ('heads', 'remotes') or ('heads')\ndepending on whether the 'remote_heads' feature is enabled or not.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 11 +++++++----\n 1 files changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 0e71749..e75d3f4 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3169,15 +3169,18 @@ sub parse_from_to_diffinfo {\n ## parse to array of hashes functions\n \n sub git_get_heads_list {\n-\tmy $limit = shift;\n+\tmy ($limit, @classes) = @_;\n+\tunless (defined @classes) {\n+\t\tmy $remote_heads = gitweb_check_feature('remote_heads');\n+\t\t@classes = ('heads', $remote_heads ? 'remotes' : ());\n+\t}\n+\tmy @patterns = map { \"refs/$_\" } @classes;\n \tmy @headslist;\n \n-\tmy $remote_heads = gitweb_check_feature('remote_heads');\n-\n \topen my $fd, '-|', git_cmd(), 'for-each-ref',\n \t\t($limit ? '--count='.($limit+1) : ()), '--sort=-committerdate',\n \t\t'--format=%(objectname) %(refname) %(subject)%00%(committer)',\n-\t\t'refs/heads', ($remote_heads ? 'refs/remotes' : ())\n+\t\t@patterns\n \t\tor return;\n \twhile (my $line = <$fd>) {\n \t\tmy %ref_item;\n-- \n1.7.3.68.g6ec8\n"},{"id":"154303","messageId":"1287917136-26103-4-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25537","inReplyTo":"1287917136-26103-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv6 03/10] gitweb: separate heads and remotes lists","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-10-24T10:45:29Z","receivedAt":"2010-10-24T10:45:29Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"We specialize the 'heads' action to only display local branches, and\nintroduce a 'remotes' action to display the remote branches (only\navailable when the remotes_head feature is enabled).\n\nMirroring this, we also split the heads list in summary view into\nlocal and remote lists, each linking to the appropriate action.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 30 ++++++++++++++++++++++++++++--\n 1 files changed, 28 insertions(+), 2 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex e75d3f4..667c11b 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -719,6 +719,7 @@ our %actions = (\n \t\"log\" => \\&git_log,\n \t\"patch\" => \\&git_patch,\n \t\"patches\" => \\&git_patches,\n+\t\"remotes\" => \\&git_remotes,\n \t\"rss\" => \\&git_rss,\n \t\"atom\" => \\&git_atom,\n \t\"search\" => \\&git_search,\n@@ -5126,6 +5127,7 @@ sub git_summary {\n \tmy %co = parse_commit(\"HEAD\");\n \tmy %cd = %co ? parse_date($co{'committer_epoch'}, $co{'committer_tz'}) : ();\n \tmy $head = $co{'id'};\n+\tmy $remote_heads = gitweb_check_feature('remote_heads');\n \n \tmy $owner = git_get_project_owner($project);\n \n@@ -5133,7 +5135,8 @@ sub git_summary {\n \t# These get_*_list functions return one more to allow us to see if\n \t# there are more ...\n \tmy @taglist = git_get_tags_list(16);\n-\tmy @headlist = git_get_heads_list(16);\n+\tmy @headlist = git_get_heads_list(16, 'heads');\n+\tmy @remotelist = $remote_heads ? git_get_heads_list(16, 'remotes') : ();\n \tmy @forklist;\n \tmy $check_forks = gitweb_check_feature('forks');\n \n@@ -5211,6 +5214,13 @@ sub git_summary {\n \t\t $cgi->a({-href => href(action=>\"heads\")}, \"...\"));\n \t}\n \n+\tif (@remotelist) {\n+\t\tgit_print_header_div('remotes');\n+\t\tgit_heads_body(\\@remotelist, $head, 0, 15,\n+\t\t $#remotelist <= 15 ? undef :\n+\t\t $cgi->a({-href => href(action=>\"remotes\")}, \"...\"));\n+\t}\n+\n \tif (@forklist) {\n \t\tgit_print_header_div('forks');\n \t\tgit_project_list_body(\\@forklist, 'age', 0, 15,\n@@ -5518,13 +5528,29 @@ sub git_heads {\n \tgit_print_page_nav('','', $head,undef,$head);\n \tgit_print_header_div('summary', $project);\n \n-\tmy @headslist = git_get_heads_list();\n+\tmy @headslist = git_get_heads_list(undef, 'heads');\n \tif (@headslist) {\n \t\tgit_heads_body(\\@headslist, $head);\n \t}\n \tgit_footer_html();\n }\n \n+sub git_remotes {\n+\tgitweb_check_feature('remote_heads')\n+\t\tor die_error(403, \"Remote heads view is disabled\");\n+\n+\tmy $head = git_get_head_hash($project);\n+\tgit_header_html();\n+\tgit_print_page_nav('','', $head,undef,$head);\n+\tgit_print_header_div('summary', $project);\n+\n+\tmy @remotelist = git_get_heads_list(undef, 'remotes');\n+\tif (@remotelist) {\n+\t\tgit_heads_body(\\@remotelist, $head);\n+\t}\n+\tgit_footer_html();\n+}\n+\n sub git_blob_plain {\n \tmy $type = shift;\n \tmy $expires;\n-- \n1.7.3.68.g6ec8\n"},{"id":"154309","messageId":"1287917136-26103-5-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25537","inReplyTo":"1287917136-26103-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv6 04/10] gitweb: nagivation menu for tags, heads and remotes","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-10-24T10:45:30Z","receivedAt":"2010-10-24T10:45:30Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"tags, heads and remotes are all views that inspect a (particular class\nof) refs, so allow the user to easily switch between them by adding\nthe appropriate navigation submenu to each view.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 19 ++++++++++++++++---\n 1 files changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 667c11b..f4592ad 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3736,6 +3736,19 @@ sub git_print_page_nav {\n \t \"\\n\";\n }\n \n+# returns a submenu for the nagivation of the refs views (tags, heads,\n+# remotes) with the current view disabled and the remotes view only\n+# available if the feature is enabled\n+sub format_ref_views {\n+\tmy ($current) = @_;\n+\tmy @ref_views = qw{tags heads};\n+\tpush @ref_views, 'remotes' if gitweb_check_feature('remote_heads');\n+\treturn join \" | \", map {\n+\t\t$_ eq $current ? $_ :\n+\t\t$cgi->a({-href => href(action=>$_)}, $_)\n+\t} @ref_views\n+}\n+\n sub format_paging_nav {\n \tmy ($action, $page, $has_next_link) = @_;\n \tmy $paging_nav;\n@@ -5512,7 +5525,7 @@ sub git_blame_data {\n sub git_tags {\n \tmy $head = git_get_head_hash($project);\n \tgit_header_html();\n-\tgit_print_page_nav('','', $head,undef,$head);\n+\tgit_print_page_nav('','', $head,undef,$head,format_ref_views('tags'));\n \tgit_print_header_div('summary', $project);\n \n \tmy @tagslist = git_get_tags_list();\n@@ -5525,7 +5538,7 @@ sub git_tags {\n sub git_heads {\n \tmy $head = git_get_head_hash($project);\n \tgit_header_html();\n-\tgit_print_page_nav('','', $head,undef,$head);\n+\tgit_print_page_nav('','', $head,undef,$head,format_ref_views('heads'));\n \tgit_print_header_div('summary', $project);\n \n \tmy @headslist = git_get_heads_list(undef, 'heads');\n@@ -5541,7 +5554,7 @@ sub git_remotes {\n \n \tmy $head = git_get_head_hash($project);\n \tgit_header_html();\n-\tgit_print_page_nav('','', $head,undef,$head);\n+\tgit_print_page_nav('','', $head,undef,$head,format_ref_views('remotes'));\n \tgit_print_header_div('summary', $project);\n \n \tmy @remotelist = git_get_heads_list(undef, 'remotes');\n-- \n1.7.3.68.g6ec8\n"},{"id":"154305","messageId":"1287917136-26103-6-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25537","inReplyTo":"1287917136-26103-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv6 05/10] gitweb: use fullname as hash_base in heads link","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-10-24T10:45:31Z","receivedAt":"2010-10-24T10:45:31Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Otherwise, if names are manipulated for display, the link will point to\nthe wrong head.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex f4592ad..a381892 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -4991,7 +4991,7 @@ sub git_heads_body {\n \t\t \"\" .\n \t\t $cgi->a({-href => href(action=>\"shortlog\", hash=>$ref{'fullname'})}, \"shortlog\") . \" | \" .\n \t\t $cgi->a({-href => href(action=>\"log\", hash=>$ref{'fullname'})}, \"log\") . \" | \" .\n-\t\t $cgi->a({-href => href(action=>\"tree\", hash=>$ref{'fullname'}, hash_base=>$ref{'name'})}, \"tree\") .\n+\t\t $cgi->a({-href => href(action=>\"tree\", hash=>$ref{'fullname'}, hash_base=>$ref{'fullname'})}, \"tree\") .\n \t\t \"\\n\" .\n \t\t \"\";\n \t}\n-- \n1.7.3.68.g6ec8\n"},{"id":"154308","messageId":"1287917136-26103-7-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25537","inReplyTo":"1287917136-26103-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv6 06/10] gitweb: allow action specialization in page header","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-10-24T10:45:32Z","receivedAt":"2010-10-24T10:45:32Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"An optional -action_extra parameter is given to git_header_html() to\nidentify a variant of the action that is being displayed. For example,\nthis can be used to specify that the remotes view is being used for a\nspecific remote and not to display all remotes.\n\nWhen -action_extra is provided, the action name in the header will be\nturned into a link to the action without any arguments or parameters, to\nprovide a quick link to the non-specific variant of the action.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 10 +++++++++-\n 1 files changed, 9 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex a381892..3612e63 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3529,7 +3529,15 @@ EOF\n \tif (defined $project) {\n \t\tprint $cgi->a({-href => href(action=>\"summary\")}, esc_html($project));\n \t\tif (defined $action) {\n-\t\t\tprint \" / $action\";\n+\t\t\tmy $action_print = $action ;\n+\t\t\tif (defined $opts{-action_extra}) {\n+\t\t\t\t$action_print = $cgi->a({-href => href(action=>$action)},\n+\t\t\t\t\t$action);\n+\t\t\t}\n+\t\t\tprint \" / $action_print\";\n+\t\t}\n+\t\tif (defined $opts{-action_extra}) {\n+\t\t\tprint \" / $opts{-action_extra}\";\n \t\t}\n \t\tprint \"\\n\";\n \t}\n-- \n1.7.3.68.g6ec8\n"},{"id":"154306","messageId":"1287917136-26103-8-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25537","inReplyTo":"1287917136-26103-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv6 07/10] gitweb: remotes view for a single remote","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-10-24T10:45:33Z","receivedAt":"2010-10-24T10:45:33Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"When 'remotes' view is passed the 'hash' parameter, interpret it as the\nname of a remote and limit the view the the heads of that remote.\n\nIn single-remote view we let the user switch easily to the default\nremotes view by specifying an -action_extra for the page header and by\nenabling the 'remotes' link in the reference navigation submenu.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 29 +++++++++++++++++++++++++----\n 1 files changed, 25 insertions(+), 4 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 3612e63..aa80748 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5561,14 +5561,35 @@ sub git_remotes {\n \t\tor die_error(403, \"Remote heads view is disabled\");\n \n \tmy $head = git_get_head_hash($project);\n-\tgit_header_html();\n-\tgit_print_page_nav('','', $head,undef,$head,format_ref_views('remotes'));\n-\tgit_print_header_div('summary', $project);\n+\tmy $remote = $input_params{'hash'};\n+\n+\tmy @remotelist;\n+\n+\tif (defined $remote) {\n+\t\t# only display the heads in a given remote\n+\t\t@remotelist = map {\n+\t\t\tmy $ref = $_ ;\n+\t\t\t$ref->{'name'} =~ s!^$remote/!!;\n+\t\t\t$ref\n+\t\t} git_get_heads_list(undef, \"remotes/$remote\");\n+\t} else {\n+\t\t@remotelist = git_get_heads_list(undef, 'remotes');\n+\t}\n+\n+\tgit_header_html(undef, undef, -action_extra => $remote);\n+\tgit_print_page_nav('', '', $head, undef, $head,\n+\t\tformat_ref_views($remote ? '' : 'remotes'));\n+\n+\tif (defined $remote) {\n+\t\tgit_print_header_div('remotes', \"$remote remote for $project\");\n+\t} else {\n+\t\tgit_print_header_div('summary', \"$project remotes\");\n+\t}\n \n-\tmy @remotelist = git_get_heads_list(undef, 'remotes');\n \tif (@remotelist) {\n \t\tgit_heads_body(\\@remotelist, $head);\n \t}\n+\n \tgit_footer_html();\n }\n \n-- \n1.7.3.68.g6ec8\n"},{"id":"154310","messageId":"1287917136-26103-9-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25537","inReplyTo":"1287917136-26103-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv6 08/10] gitweb: refactor repository URL printing","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-10-24T10:45:34Z","receivedAt":"2010-10-24T10:45:34Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Factor out the code to display the repository URL(s) from summary view\ninto a format_rep_url() routine.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 7 ++++++-\n 1 files changed, 6 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex aa80748..ec2626d 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3800,6 +3800,11 @@ sub git_print_header_div {\n \t \"\\n\\n\";\n }\n \n+sub format_repo_url {\n+\tmy ($name, $url) = @_;\n+\treturn \"$name$url\\n\";\n+}\n+\n sub print_local_time {\n \tprint format_local_time(@_);\n }\n@@ -5183,7 +5188,7 @@ sub git_summary {\n \t@url_list = map { \"$_/$project\" } @git_base_url_list unless @url_list;\n \tforeach my $git_url (@url_list) {\n \t\tnext unless $git_url;\n-\t\tprint \"$url_tag$git_url\\n\";\n+\t\tprint format_repo_url($url_tag, $git_url);\n \t\t$url_tag = \"\";\n \t}\n \n-- \n1.7.3.68.g6ec8\n"},{"id":"154304","messageId":"1287917136-26103-10-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25537","inReplyTo":"1287917136-26103-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv6 09/10] gitweb: provide a routine to display (sub)sections","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-10-24T10:45:35Z","receivedAt":"2010-10-24T10:45:35Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"The routine puts the given contento into a DIV element, automatically\nadding a header div. The content can be provided as a standard scalar\nvalue (which is used as-is), as a scalar ref (which is HTML-escaped), as\na function reference to be executed, or as a file handle to be dumped.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 38 ++++++++++++++++++++++++++++++++++++++\n 1 files changed, 38 insertions(+), 0 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex ec2626d..feca8bc 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3805,6 +3805,44 @@ sub format_repo_url {\n \treturn \"$name$url\\n\";\n }\n \n+# Group output by placing it in a DIV element and adding a header.\n+# Options for start_div() can be provided by passing a hash reference as the\n+# first parameter to the function.\n+# Options to git_print_header_div() can be provided by passing an array\n+# reference. This must follow the options to start_div if they are present.\n+# The content can be a scalar, which is output as-is, a scalar reference, which\n+# is output after html escaping, an IO handle passed either as *handle or\n+# *handle{IO}, or a function reference. In the latter case all following\n+# parameters will be taken as argument to the content function call.\n+sub git_print_section {\n+\tmy ($div_args, $header_args, $content);\n+\tmy $arg = shift;\n+\tif (ref($arg) eq 'HASH') {\n+\t\t$div_args = $arg;\n+\t\t$arg = shift;\n+\t}\n+\tif (ref($arg) eq 'ARRAY') {\n+\t\t$header_args = $arg;\n+\t\t$arg = shift;\n+\t}\n+\t$content = $arg;\n+\n+\tprint $cgi->start_div($div_args);\n+\tgit_print_header_div(@$header_args);\n+\n+\tif (ref($content) eq 'CODE') {\n+\t\t$content->(@_);\n+\t} elsif (ref($content) eq 'SCALAR') {\n+\t\tprint esc_html($$content);\n+\t} elsif (ref($content) eq 'GLOB' or ref($content) eq 'IO::Handle') {\n+\t\tprint <$content>;\n+\t} elsif (!ref($content) && defined($content)) {\n+\t\tprint $content;\n+\t}\n+\n+\tprint $cgi->end_div;\n+}\n+\n sub print_local_time {\n \tprint format_local_time(@_);\n }\n-- \n1.7.3.68.g6ec8\n"},{"id":"154307","messageId":"1287917136-26103-11-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25537","inReplyTo":"1287917136-26103-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv6 10/10] gitweb: group remote heads by remote","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-10-24T10:45:36Z","receivedAt":"2010-10-24T10:45:36Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"In remote and summary view, display a block for each remote, with the\nfetch and push URL(s) as well as the list of the remote heads.\n\nIn summary view, if the number of remotes is higher than a prescribed\nlimit, only display the first remotes and their fetch and push\nurls, without any heads information and without grouping.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 185 +++++++++++++++++++++++++++++++++++++++++-----\n gitweb/static/gitweb.css | 6 ++\n 2 files changed, 172 insertions(+), 19 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex feca8bc..5f08dcc 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -2772,6 +2772,66 @@ sub git_get_last_activity {\n \treturn (undef, undef);\n }\n \n+# Returns a hash ref mapping remote names to their fetch and push URLs.\n+# We return a hash ref rather than a hash so that a simple check with defined\n+# can be used to tell apart the \"no remotes\" case from other kinds of\n+# failures.\n+#\n+# It is possible to limit the retrieved remotes either by number\n+# (specifying a -limit parameter) or by name (-wanted parameter).\n+#\n+# When a single remote is wanted, we cannot use 'git remote show -n' because\n+# that command always work (assuming it's a remote URL if it's not defined),\n+# and we cannot use 'git remote show' because that would try to make a network\n+# roundtrip. So the only way to find if that particular remote is defined is to\n+# walk the list provided by 'git remote -v' and stop if and when we find what\n+# we want.\n+sub git_get_remotes_list {\n+\tmy %params = @_;\n+\tmy $limit = $params{-limit};\n+\tmy $wanted = $params{-wanted};\n+\tmy %remotes = ();\n+\n+\topen my $fd, '-|' , git_cmd(), 'remote', '-v';\n+\treturn unless $fd;\n+\twhile (my $remote = <$fd>) {\n+\t\tchomp $remote;\n+\t\t$remote =~ s!\\t(.*?)\\s+\\((\\w+)\\)$!!;\n+\t\tnext if $wanted and not $remote eq $wanted;\n+\t\tmy ($url, $key) = ($1, $2);\n+\n+\t\t# a remote may appear more than once because of multiple URLs,\n+\t\t# so if this is a remote we know already, be sure to continue,\n+\t\t# lest we end up with a remote for which we get the fetch URL\n+\t\t# bot not the push URL, for example\n+\t\tmy $more = exists $remotes{$remote};\n+\t\t$more ||= defined $limit ? (keys(%remotes) <= $limit) : 1;\n+\t\tif ($more) {\n+\t\t\t$remotes{$remote} ||= { 'heads' => () };\n+\t\t\t$remotes{$remote}{$key} = $url;\n+\t\t} else {\n+\t\t\tlast;\n+\t\t}\n+\t}\n+\tclose $fd or return;\n+\treturn \\%remotes;\n+}\n+\n+# Takes a hash of remotes as first parameter and fills it by adding the\n+# available remote heads for each of the indicated remotes.\n+# A maximum number of heads can also be specified.\n+sub git_get_remote_heads {\n+\tmy ($remotes, $limit) = @_;\n+\tmy @heads = map { \"remotes/$_\" } keys %$remotes;\n+\tmy @remoteheads = git_get_heads_list($limit, @heads);\n+\tforeach (keys %$remotes) {\n+\t\tmy $remote = $_;\n+\t\t$remotes->{$remote}{'heads'} = [ grep {\n+\t\t\t$_->{'name'} =~ s!^$remote/!!\n+\t\t\t} @remoteheads ];\n+\t}\n+}\n+\n sub git_get_references {\n \tmy $type = shift || \"\";\n \tmy %refs;\n@@ -5054,6 +5114,100 @@ sub git_heads_body {\n \tprint \"\\n\";\n }\n \n+# Display a single remote block\n+sub git_remote_body {\n+\tmy ($remote, $rdata, $limit, $head) = @_;\n+\n+\tmy $heads = $rdata->{'heads'};\n+\tmy $fetch = $rdata->{'fetch'};\n+\tmy $push = $rdata->{'push'};\n+\n+\tmy $urls = \"\\n\" ;\n+\n+\tif (defined $fetch) {\n+\t\tif ($fetch eq $push) {\n+\t\t\t$urls .= format_repo_url(\"URL\", $fetch);\n+\t\t} else {\n+\t\t\t$urls .= format_repo_url(\"Fetch URL\", $fetch);\n+\t\t\t$urls .= format_repo_url(\"Push URL\", $push) if defined $push;\n+\t\t}\n+\t} elsif (defined $push) {\n+\t\t$urls .= format_repo_url(\"Push URL\", $push);\n+\t} else {\n+\t\t$urls .= format_repo_url(\"\", \"No remote URL\");\n+\t}\n+\n+\t$urls .= \"
\\n\";\n+\n+\tmy ($maxheads, $dots);\n+\tif (defined $limit) {\n+\t\t$maxheads = $limit - 1;\n+\t\tif ($#{$heads} > $maxheads) {\n+\t\t\t$dots = $cgi->a({-href => href(action=>\"remotes\", hash=>$remote)}, \"...\");\n+\t\t}\n+\t}\n+\n+\tprint $urls;\n+\tgit_heads_body($heads, $head, 0, $maxheads, $dots);\n+}\n+\n+# Display a list of remote names with the respective fetch and push URLs\n+# This routine only gets called when there are more remotes than the given\n+# limit, so we know that we have to append an ellipsis to the table and\n+# that we have to pop one of the keys.\n+sub git_remotes_list {\n+\tmy ($remotedata) = @_;\n+\tprint \"\\n\";\n+\tmy $alternate = 1;\n+\tmy @keys = sort keys %$remotedata;\n+\tpop @keys;\n+\n+\twhile (my $remote = shift @keys) {\n+\t\tmy $rdata = $remotedata->{$remote};\n+\t\tmy $fetch = $rdata->{'fetch'};\n+\t\tmy $push = $rdata->{'push'};\n+\t\tif ($alternate) {\n+\t\t\tprint \"\\n\";\n+\t\t} else {\n+\t\t\tprint \"\\n\";\n+\t\t}\n+\t\t$alternate ^= 1;\n+\t\tprint \"\";\n+\t\tprint \"\";\n+\n+\t\tprint \"\\n\";\n+\t}\n+\tprint \"\\n\" .\n+\t \"\\n\" . \"\\n\";\n+\tprint \"
\" .\n+\t\t $cgi->a({-href=> href(action=>'remotes', hash=>$remote),\n+\t\t\t -class=> \"list name\"},esc_html($remote)) . \"\" .\n+\t\t (defined $fetch ? $cgi->a({-href=> $fetch}, \"fetch\") : \"fetch\") .\n+\t\t \" | \" .\n+\t\t (defined $push ? $cgi->a({-href=> $push}, \"push\") : \"push\") .\n+\t\t \"
\" .\n+\t $cgi->a({-href => href(action=>\"remotes\")}, \"...\") .\n+\t \"
\";\n+}\n+\n+# Display remote heads grouped by remote, unless there are too many\n+# remotes ($have_all is false), in which case we only display the remote\n+# names\n+sub git_remotes_body {\n+\tmy ($remotedata, $limit, $head) = @_;\n+\tif (not defined $limit or scalar keys %$remotedata <= $limit) {\n+\t\tgit_get_remote_heads($remotedata, $limit);\n+\t\twhile (my ($remote, $rdata) = each %$remotedata) {\n+\t\t\tgit_print_section({-class=>\"remote\", -id=>$remote},\n+\t\t\t\t[\"remotes\", $remote, $remote], sub {\n+\t\t\t\t\tgit_remote_body($remote, $rdata, $limit, $head);\n+\t\t\t\t});\n+\t\t}\n+\t} else {\n+\t\tgit_remotes_list($remotedata, $limit);\n+\t}\n+}\n+\n sub git_search_grep_body {\n \tmy ($commitlist, $from, $to, $extra) = @_;\n \t$from = 0 unless defined $from;\n@@ -5200,7 +5354,7 @@ sub git_summary {\n \t# there are more ...\n \tmy @taglist = git_get_tags_list(16);\n \tmy @headlist = git_get_heads_list(16, 'heads');\n-\tmy @remotelist = $remote_heads ? git_get_heads_list(16, 'remotes') : ();\n+\tmy $remotedata = $remote_heads ? git_get_remotes_list(-limit => 16) : undef;\n \tmy @forklist;\n \tmy $check_forks = gitweb_check_feature('forks');\n \n@@ -5278,11 +5432,9 @@ sub git_summary {\n \t\t $cgi->a({-href => href(action=>\"heads\")}, \"...\"));\n \t}\n \n-\tif (@remotelist) {\n+\tif ($remotedata) {\n \t\tgit_print_header_div('remotes');\n-\t\tgit_heads_body(\\@remotelist, $head, 0, 15,\n-\t\t $#remotelist <= 15 ? undef :\n-\t\t $cgi->a({-href => href(action=>\"remotes\")}, \"...\"));\n+\t\tgit_remotes_body($remotedata, 16, $head);\n \t}\n \n \tif (@forklist) {\n@@ -5606,31 +5758,26 @@ sub git_remotes {\n \tmy $head = git_get_head_hash($project);\n \tmy $remote = $input_params{'hash'};\n \n-\tmy @remotelist;\n+\tmy $remotedata = git_get_remotes_list(-wanted => $remote);\n+\tdie_error(500, \"Unable to get remote information\") unless defined $remotedata;\n \n-\tif (defined $remote) {\n-\t\t# only display the heads in a given remote\n-\t\t@remotelist = map {\n-\t\t\tmy $ref = $_ ;\n-\t\t\t$ref->{'name'} =~ s!^$remote/!!;\n-\t\t\t$ref\n-\t\t} git_get_heads_list(undef, \"remotes/$remote\");\n-\t} else {\n-\t\t@remotelist = git_get_heads_list(undef, 'remotes');\n+\tif (keys(%$remotedata) == 0) {\n+\t\tdie_error(404, defined $remote ?\n+\t\t\t\"Remote $remote not found\" :\n+\t\t\t\"No remotes found\");\n \t}\n \n \tgit_header_html(undef, undef, -action_extra => $remote);\n \tgit_print_page_nav('', '', $head, undef, $head,\n \t\tformat_ref_views($remote ? '' : 'remotes'));\n \n+\tgit_get_remote_heads($remotedata, undef);\n \tif (defined $remote) {\n \t\tgit_print_header_div('remotes', \"$remote remote for $project\");\n+\t\tgit_remote_body($remote, $remotedata->{$remote}, undef, $head);\n \t} else {\n \t\tgit_print_header_div('summary', \"$project remotes\");\n-\t}\n-\n-\tif (@remotelist) {\n-\t\tgit_heads_body(\\@remotelist, $head);\n+\t\tgit_remotes_body($remotedata, undef $head);\n \t}\n \n \tgit_footer_html();\ndiff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css\nindex 4132aab..79d7eeb 100644\n--- a/gitweb/static/gitweb.css\n+++ b/gitweb/static/gitweb.css\n@@ -573,6 +573,12 @@ div.binary {\n \tfont-style: italic;\n }\n \n+div.remote {\n+\tmargin: .5em;\n+\tborder: 1px solid #d9d8d1;\n+\tdisplay: inline-block;\n+}\n+\n /* Style definition generated by highlight 2.4.5, http://www.andre-simon.de/ */\n \n /* Highlighting theme definition: */\n-- \n1.7.3.68.g6ec8\n"},{"id":"154410","messageId":"201010251657.02802.jnareb@gmail.com","threadId":"25537","inReplyTo":"1287917136-26103-6-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv6 05/10] gitweb: use fullname as hash_base in heads link","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-10-25T14:56:59Z","receivedAt":"2010-10-25T14:56:59Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun, 24 Oct 2010, Giuseppe Bilotta wrote:\n\n> Otherwise, if names are manipulated for display, the link will point to\n> the wrong head.\n> \n> Signed-off-by: Giuseppe Bilotta \n\nShouldn't this patch be at beginning, as first patch of the series?\n\n> ---\n> gitweb/gitweb.perl | 2 +-\n> 1 files changed, 1 insertions(+), 1 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index f4592ad..a381892 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -4991,7 +4991,7 @@ sub git_heads_body {\n> \t\t \"\" .\n> \t\t $cgi->a({-href => href(action=>\"shortlog\", hash=>$ref{'fullname'})}, \"shortlog\") . \" | \" .\n> \t\t $cgi->a({-href => href(action=>\"log\", hash=>$ref{'fullname'})}, \"log\") . \" | \" .\n> -\t\t $cgi->a({-href => href(action=>\"tree\", hash=>$ref{'fullname'}, hash_base=>$ref{'name'})}, \"tree\") .\n> +\t\t $cgi->a({-href => href(action=>\"tree\", hash=>$ref{'fullname'}, hash_base=>$ref{'fullname'})}, \"tree\") .\n> \t\t \"\\n\" .\n> \t\t \"\";\n> \t}\n> -- \n> 1.7.3.68.g6ec8\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"154411","messageId":"201010251701.15334.jnareb@gmail.com","threadId":"25537","inReplyTo":"1287917136-26103-4-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv6 03/10] gitweb: separate heads and remotes lists","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-10-25T15:01:14Z","receivedAt":"2010-10-25T15:01:14Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun, 24 Oct 2010, Giuseppe Bilotta wrote:\n\n> We specialize the 'heads' action to only display local branches, and\n> introduce a 'remotes' action to display the remote branches (only\n> available when the remotes_head feature is enabled).\n> \n> Mirroring this, we also split the heads list in summary view into\n> local and remote lists, each linking to the appropriate action.\n> \n> Signed-off-by: Giuseppe Bilotta \n\n> -\tmy @headlist = git_get_heads_list(16);\n> +\tmy @headlist = git_get_heads_list(16, 'heads');\n\n> -\tmy @headslist = git_get_heads_list();\n> +\tmy @headslist = git_get_heads_list(undef, 'heads');\n\nHmmm... I wonder if it wouldn't be better to simply change defaults\nin git_get_heads_list() so that second argument defaults always to\n'heads', even if 'remote_heads' feature is enabled...\n\n-- \nJakub Narebski\nPoland\n"},{"id":"154413","messageId":"AANLkTimcsjo1rH5EFZxnDOrR5pfgHamxN=z30OBSa=Xo@mail.gmail.com","threadId":"25537","inReplyTo":"201010251657.02802.jnareb@gmail.com","subject":"Re: [PATCHv6 05/10] gitweb: use fullname as hash_base in heads link","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-10-25T15:07:33Z","receivedAt":"2010-10-25T15:07:33Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2010/10/25 Jakub Narebski :\n> On Sun, 24 Oct 2010, Giuseppe Bilotta wrote:\n>\n>> Otherwise, if names are manipulated for display, the link will point to\n>> the wrong head.\n>>\n>> Signed-off-by: Giuseppe Bilotta \n>\n> Shouldn't this patch be at beginning, as first patch of the series?\n\nGiven how independent it is from the rest of the series, that's\nprobably a good idea.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"154415","messageId":"201010251712.30362.jnareb@gmail.com","threadId":"25537","inReplyTo":"1287917136-26103-8-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv6 07/10] gitweb: remotes view for a single remote","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-10-25T15:12:28Z","receivedAt":"2010-10-25T15:12:28Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun, 24 Oct 2010, Giuseppe Bilotta wrote:\n\n> When 'remotes' view is passed the 'hash' parameter, interpret it as the\n> name of a remote and limit the view the the heads of that remote.\n\nI don't quite like (ab)using 'hash' parameter like that, but because\nit allows us to use e.g. 'project.git/remotes/origin' path_info\nwithout writing special code for that, I am all right with this hack.\n\n> \n> In single-remote view we let the user switch easily to the default\n> remotes view by specifying an -action_extra for the page header and by\n> enabling the 'remotes' link in the reference navigation submenu.\n\nNice!\n\n> +\tif (defined $remote) {\n> +\t\t# only display the heads in a given remote\n\nIt also strips uninteresting '/' prefix, isn't it?\nIt would be nice, though not necessary, to have this in comment.\n\n> +\t\t@remotelist = map {\n> +\t\t\tmy $ref = $_ ;\n> +\t\t\t$ref->{'name'} =~ s!^$remote/!!;\n> +\t\t\t$ref\n> +\t\t} git_get_heads_list(undef, \"remotes/$remote\");\n> +\t} else {\n\n-- \nJakub Narebski\nPoland\n"},{"id":"154416","messageId":"201010251715.54531.jnareb@gmail.com","threadId":"25537","inReplyTo":"1287917136-26103-10-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv6 09/10] gitweb: provide a routine to display (sub)sections","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-10-25T15:15:54Z","receivedAt":"2010-10-25T15:15:54Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun, 24 Oct 2010, Giuseppe Bilotta wrote:\n\n[...]\n> +# Group output by placing it in a DIV element and adding a header.\n> +# Options for start_div() can be provided by passing a hash reference as the\n> +# first parameter to the function.\n> +# Options to git_print_header_div() can be provided by passing an array\n> +# reference. This must follow the options to start_div if they are present.\n> +# The content can be a scalar, which is output as-is, a scalar reference, which\n> +# is output after html escaping, an IO handle passed either as *handle or\n> +# *handle{IO}, or a function reference. In the latter case all following\n> +# parameters will be taken as argument to the content function call.\n> +sub git_print_section {\n\nVery nice API. Good work!\n\n-- \nJakub Narebski\nPoland\n"},{"id":"154421","messageId":"AANLkTim-KAcGE1X_bgOQE+0OCA6PJA2wKb36caPvKDUH@mail.gmail.com","threadId":"25537","inReplyTo":"201010251701.15334.jnareb@gmail.com","subject":"Re: [PATCHv6 03/10] gitweb: separate heads and remotes lists","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-10-25T18:14:13Z","receivedAt":"2010-10-25T18:14:13Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2010/10/25 Jakub Narebski :\n> On Sun, 24 Oct 2010, Giuseppe Bilotta wrote:\n>\n>> We specialize the 'heads' action to only display local branches, and\n>> introduce a 'remotes' action to display the remote branches (only\n>> available when the remotes_head feature is enabled).\n>>\n>> Mirroring this, we also split the heads list in summary view into\n>> local and remote lists, each linking to the appropriate action.\n>>\n>> Signed-off-by: Giuseppe Bilotta \n>\n>> -     my @headlist = git_get_heads_list(16);\n>> +     my @headlist = git_get_heads_list(16, 'heads');\n>\n>> -     my @headslist = git_get_heads_list();\n>> +     my @headslist = git_get_heads_list(undef, 'heads');\n>\n> Hmmm... I wonder if it wouldn't be better to simply change defaults\n> in git_get_heads_list() so that second argument defaults always to\n> 'heads', even if 'remote_heads' feature is enabled...\n\nThat's a good point. I'll look into that.\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"154422","messageId":"AANLkTi=LiE3=fKdxeQU8HsKLia1ur2k2CDtqDhALj234@mail.gmail.com","threadId":"25537","inReplyTo":"201010251712.30362.jnareb@gmail.com","subject":"Re: [PATCHv6 07/10] gitweb: remotes view for a single remote","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-10-25T18:18:18Z","receivedAt":"2010-10-25T18:18:18Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2010/10/25 Jakub Narebski :\n> On Sun, 24 Oct 2010, Giuseppe Bilotta wrote:\n>\n>> When 'remotes' view is passed the 'hash' parameter, interpret it as the\n>> name of a remote and limit the view the the heads of that remote.\n>\n> I don't quite like (ab)using 'hash' parameter like that, but because\n> it allows us to use e.g. 'project.git/remotes/origin' path_info\n> without writing special code for that, I am all right with this hack.\n\nWe could rename the 'hash' parameter to be more descriptive of the\nfact that it includes this nature, but I really think it's not worth\nit.\n\n>> In single-remote view we let the user switch easily to the default\n>> remotes view by specifying an -action_extra for the page header and by\n>> enabling the 'remotes' link in the reference navigation submenu.\n>\n> Nice!\n>\n>> +     if (defined $remote) {\n>> +             # only display the heads in a given remote\n>\n> It also strips uninteresting '/' prefix, isn't it?\n> It would be nice, though not necessary, to have this in comment.\n\nGood idea.\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"154423","messageId":"AANLkTikjGz4LpsneTnJJfyFphUxCfOMa_haG6o9tPUEP@mail.gmail.com","threadId":"25537","inReplyTo":"201010251715.54531.jnareb@gmail.com","subject":"Re: [PATCHv6 09/10] gitweb: provide a routine to display (sub)sections","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-10-25T18:21:17Z","receivedAt":"2010-10-25T18:21:17Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2010/10/25 Jakub Narebski :\n> On Sun, 24 Oct 2010, Giuseppe Bilotta wrote:\n>\n> [...]\n>> +# Group output by placing it in a DIV element and adding a header.\n>> +# Options for start_div() can be provided by passing a hash reference as the\n>> +# first parameter to the function.\n>> +# Options to git_print_header_div() can be provided by passing an array\n>> +# reference. This must follow the options to start_div if they are present.\n>> +# The content can be a scalar, which is output as-is, a scalar reference, which\n>> +# is output after html escaping, an IO handle passed either as *handle or\n>> +# *handle{IO}, or a function reference. In the latter case all following\n>> +# parameters will be taken as argument to the content function call.\n>> +sub git_print_section {\n>\n> Very nice API.  Good work!\n\nI'm glad you like it. And thank you very much for the reviews that\ntook me to design it properly. Shall I take this as an ack for the\nseries, modulo the minor suggestions for the other two patches? (Just\nto know if I can include Junio in the next posting)\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"154424","messageId":"201010252038.47687.jnareb@gmail.com","threadId":"25537","inReplyTo":"1287917136-26103-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv6 00/10] gitweb: remote_heads feature","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-10-25T18:38:46Z","receivedAt":"2010-10-25T18:38:46Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun, 24 Oct 2010, Giuseppe Bilotta wrote:\n\n> In this 6th version of the remote heads feature for gitweb, much of the\n> internal API has been cleaned up thanks to the suggestion from Jakub and\n> others. There were also some other stylistic changes, and a few patches were\n> reordered/merged.\n> \n> The first 4 patches are rather straightforward and can probably go\n> straight in. The 5th patch is a bugfix for something that is only\n> triggered by the name manipulation done with the remote heads, but can\n> probably be useful even without the rest of the series.\n> \n> Patch 7 provides 'single remote view', depending on patch 6 for\n> improved visuals of the page header.\n> \n> Patches 8 and 9 provide some infrastructure for the grouped remote heads\n> display introduced in the last patch.\n\nYou can consider all patches, except the very last one (which is\nundergoing review - I will send a comment shortly), can be considered\nAcked-by. Any comments are minor, and non-blocking.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"154429","messageId":"201010252356.37472.jnareb@gmail.com","threadId":"25537","inReplyTo":"1287917136-26103-3-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv6 02/10] gitweb: git_get_heads_list accepts an optional list of refs.","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-10-25T21:56:37Z","receivedAt":"2010-10-25T21:56:37Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun, 24 Sep 2010, Giuseppe Bilotta wrote:\n\n>  sub git_get_heads_list {\n> -       my $limit = shift;\n> +       my ($limit, @classes) = @_;\n> +       unless (defined @classes) {\n> +               my $remote_heads = gitweb_check_feature('remote_heads');\n> +               @classes = ('heads', $remote_heads ? 'remotes' : ());\n> +       }\n\ndefined(@array) is deprecated at t/../gitweb/gitweb.perl line 3221.\n\nShould be simply 'unless (@classes)', or 'unless (scalar @classes)' but\nconditionals provide boolean context, which is scalar context.\n\nI'm sorry about missing it earlier.\n-- \nJakub Narebski\nPoland\n"},{"id":"154455","messageId":"AANLkTim238K41oJAgXJiGMY0E_J_fzQ4b_ovNman2Agn@mail.gmail.com","threadId":"25537","inReplyTo":"201010252356.37472.jnareb@gmail.com","subject":"Re: [PATCHv6 02/10] gitweb: git_get_heads_list accepts an optional list of refs.","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-10-26T16:30:04Z","receivedAt":"2010-10-26T16:30:04Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2010/10/25 Jakub Narebski :\n> On Sun, 24 Sep 2010, Giuseppe Bilotta wrote:\n>\n>>  sub git_get_heads_list {\n>> -       my $limit = shift;\n>> +       my ($limit, @classes) = @_;\n>> +       unless (defined @classes) {\n>> +               my $remote_heads = gitweb_check_feature('remote_heads');\n>> +               @classes = ('heads', $remote_heads ? 'remotes' : ());\n>> +       }\n>\n> defined(@array) is deprecated at t/../gitweb/gitweb.perl line 3221.\n>\n> Should be simply 'unless (@classes)', or 'unless (scalar @classes)' but\n> conditionals provide boolean context, which is scalar context.\n>\n> I'm sorry about missing it earlier.\n\nNo problem. Fixed for the next batch.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"154469","messageId":"201010270232.37961.jnareb@gmail.com","threadId":"25537","inReplyTo":"1287917136-26103-11-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv6 10/10] gitweb: group remote heads by remote","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-10-27T00:32:34Z","receivedAt":"2010-10-27T00:32:34Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun, 24 Oct 2010, Giuseppe Bilotta wrote:\n\n> In remote and summary view, display a block for each remote, with the\n> fetch and push URL(s) as well as the list of the remote heads.\n> \n> In summary view, if the number of remotes is higher than a prescribed\n> limit, only display the first remotes and their fetch and push\n> urls, without any heads information and without grouping.\n\nI like this idea.\n\nI guess that we can always implement more fancy limiting in the future\n(e.g. taking into account total number of displayed remote heads).\n\n> \n> Signed-off-by: Giuseppe Bilotta \n> ---\n> gitweb/gitweb.perl | 185 +++++++++++++++++++++++++++++++++++++++++-----\n> gitweb/static/gitweb.css | 6 ++\n> 2 files changed, 172 insertions(+), 19 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index feca8bc..5f08dcc 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -2772,6 +2772,66 @@ sub git_get_last_activity {\n> \treturn (undef, undef);\n> }\n> \n> +# Returns a hash ref mapping remote names to their fetch and push URLs.\n> +# We return a hash ref rather than a hash so that a simple check with defined\n> +# can be used to tell apart the \"no remotes\" case from other kinds of\n> +# failures.\n\nJust an idea (it is not necessary to implement it; it has its own\ndrawbacks): we could implement here\n\n return wantsarray ? %remotes : \\%remotes;\n\nso the caller that wants hash, would get hash; and the one wanting\nreference (to distinguish error from no remotes), would get hash\nreference.\n\n> +#\n> +# It is possible to limit the retrieved remotes either by number\n> +# (specifying a -limit parameter) or by name (-wanted parameter).\n\nI don't quite like limiting when generating, and would prefer do limiting\non display, especially if not doing limiting would not affect performance\nmuch (git command invoked doesn't do limiting, like in case of \ngit_get_heads_list / git_get_tags_list or *most important* parse_commits).\n\nEspecially if it complicates code that much (see below).\n\nNot doing limiting here, in git_get_remotes_list (or just git_get_remotes)\nwould also make API simpler; the single optional argument would be name of\nremote we want to retrieve.\n\n+# It is possible to limit the retrieved remotes to only single remote\n+# by passing its name as a parameter to git_get_remotes_list.\n\n\n> +#\n> +# When a single remote is wanted, we cannot use 'git remote show -n' because\n> +# that command always work (assuming it's a remote URL if it's not defined),\n> +# and we cannot use 'git remote show' because that would try to make a network\n> +# roundtrip. So the only way to find if that particular remote is defined is to\n> +# walk the list provided by 'git remote -v' and stop if and when we find what\n> +# we want.\n\nI would add 'Implemetation note: ' here, which means start with\n'Implementation note: When ...\" -- but it is not necessary.\n\n> +sub git_get_remotes_list {\n> +\tmy %params = @_;\n> +\tmy $limit = $params{-limit};\n> +\tmy $wanted = $params{-wanted};\n> +\tmy %remotes = ();\n> +\n> +\topen my $fd, '-|' , git_cmd(), 'remote', '-v';\n> +\treturn unless $fd;\n> +\twhile (my $remote = <$fd>) {\n> +\t\tchomp $remote;\n> +\t\t$remote =~ s!\\t(.*?)\\s+\\((\\w+)\\)$!!;\n> +\t\tnext if $wanted and not $remote eq $wanted;\n> +\t\tmy ($url, $key) = ($1, $2);\n> +\n> +\t\t# a remote may appear more than once because of multiple URLs,\n> +\t\t# so if this is a remote we know already, be sure to continue,\n> +\t\t# lest we end up with a remote for which we get the fetch URL\n> +\t\t# bot not the push URL, for example\n> +\t\tmy $more = exists $remotes{$remote};\n> +\t\t$more ||= defined $limit ? (keys(%remotes) <= $limit) : 1;\n> +\t\tif ($more) {\n> +\t\t\t$remotes{$remote} ||= { 'heads' => () };\n> +\t\t\t$remotes{$remote}{$key} = $url;\n> +\t\t} else {\n> +\t\t\tlast;\n> +\t\t}\n> +\t}\n> +\tclose $fd or return;\n> +\treturn \\%remotes;\n> +}\n\nWithout implementing limiting in git_get_remotes_list(), but leaving it\nfor the display subroutine, it would reduce to:\n\n+sub git_get_remotes_list {\n+\tmy $wanted = shift;\n+\tmy %remotes = ();\n+\n+\topen my $fd, '-|' , git_cmd(), 'remote', '-v';\n+\treturn unless $fd;\n+\twhile (my $remote = <$fd>) {\n+\t\tchomp $remote;\n+\t\tnext unless ($remote =~ s!\\t(.*?)\\s+\\((\\w+)\\)$!!);\n+\t\t# $wanted remote might be present multiple times in output\n+\t\tnext if ($wanted && $remote ne $wanted);\n+\t\tmy ($url, $key) = ($1, $2);\n+\n+\t\t$remotes{$remote}{$key} = $url;\n+\t}\n+\tclose $fd or return;\n+\treturn \\%remotes;\n+}\n\n> +\n> +# Takes a hash of remotes as first parameter and fills it by adding the\n> +# available remote heads for each of the indicated remotes.\n> +# A maximum number of heads can also be specified.\n\nAll git_get_* subroutines _return_ something; this looks more like fill_*\nfunction for me.\n\n> +sub git_get_remote_heads {\n> +\tmy ($remotes, $limit) = @_;\n> +\tmy @heads = map { \"remotes/$_\" } keys %$remotes;\n> +\tmy @remoteheads = git_get_heads_list($limit, @heads);\n> +\tforeach (keys %$remotes) {\n> +\t\tmy $remote = $_;\n> +\t\t$remotes->{$remote}{'heads'} = [ grep {\n> +\t\t\t$_->{'name'} =~ s!^$remote/!!\n> +\t\t\t} @remoteheads ];\n> +\t}\n> +}\n\nWe could remove limiting number of heads shown per remote also from here,\nbut this time 1.) the $limit parameter is passed down to git_get_heads_list\nwhich in turn uses $limit as parameter to git command 2.) it doesn't\nsimplify code almost at all:\n\n+sub fill_remote_heads {\n+\tmy $remotes = shift;\n+\n+\tmy @heads = map { \"remotes/$_\" } keys %$remotes;\n+\t## A QUESTION: perhaps it would be better to pass @remoteheads as parameter?\n+\tmy @remoteheads = git_get_heads_list(undef, @heads);\n+\tforeach my $remote (keys %$remotes) {\n+\t\t$remotes->{$remote}{'heads'} =\n+\t\t\t[ grep { $_->{'name'} =~ s!^$remote/!! } @remoteheads ];\n+\t}\n+}\n\n\nA QUESTION: perhaps it would be better to pass @remoteheads as parameter?\nI wonder if won't end up with calling git_get_heads_list multiple times...\nwell, the improvement can be left for later. Answering this question\nshould not affect accepting this patch series.\n\n> +\n> sub git_get_references {\n> \tmy $type = shift || \"\";\n> \tmy %refs;\n> @@ -5054,6 +5114,100 @@ sub git_heads_body {\n> \tprint \"\\n\";\n> }\n> \n> +# Display a single remote block\n\nNote that you would end up with two very similarly named subrotines:\ngit_remote_body and git_remotes_body. Perhaps it would be better to\ncall this one git_remote_block, or git_describe_remote?\n\n> +sub git_remote_body {\n> +\tmy ($remote, $rdata, $limit, $head) = @_;\n> +\n> +\tmy $heads = $rdata->{'heads'};\n> +\tmy $fetch = $rdata->{'fetch'};\n> +\tmy $push = $rdata->{'push'};\n\nWe could have used the following Perl shortcut\n\n+\tmy ($heads, $fetch, $push) = @{$rdata}{qw(heads fetch push)};\n\nbut it isn't needed, and I guess it even could hurt readibility...\n\n> +\n> +\tmy $urls = \"\\n\" ;\n> +\n> +\tif (defined $fetch) {\n> +\t\tif ($fetch eq $push) {\n> +\t\t\t$urls .= format_repo_url(\"URL\", $fetch);\n> +\t\t} else {\n> +\t\t\t$urls .= format_repo_url(\"Fetch URL\", $fetch);\n> +\t\t\t$urls .= format_repo_url(\"Push URL\", $push) if defined $push;\n> +\t\t}\n> +\t} elsif (defined $push) {\n> +\t\t$urls .= format_repo_url(\"Push URL\", $push);\n> +\t} else {\n> +\t\t$urls .= format_repo_url(\"\", \"No remote URL\");\n> +\t}\n> +\n> +\t$urls .= \"
\\n\";\n\nI'm not sure about naming this variable $urls...\n\n> +\n> +\tmy ($maxheads, $dots);\n> +\tif (defined $limit) {\n> +\t\t$maxheads = $limit - 1;\n> +\t\tif ($#{$heads} > $maxheads) {\n> +\t\t\t$dots = $cgi->a({-href => href(action=>\"remotes\", hash=>$remote)}, \"...\");\n> +\t\t}\n> +\t}\n> +\n> +\tprint $urls;\n> +\tgit_heads_body($heads, $head, 0, $maxheads, $dots);\n\nWouldn't this be simpler:\n\n+\tmy $dots;\n+\tif (defined $limit && $limit < @$heads) {\n+\t\t$dots = $cgi->a({-href => href(action=>\"remotes\", hash=>$remote)}, \"...\");\n+\t}\n+\n+\tprint $urls;\n+\tgit_heads_body($heads, $head, 0, $limit, $dots);\n\nWe would do similar trick as in other parts: request one item more than we\ndisplay to check if there is more data.\n\n> +}\n> +\n> +# Display a list of remote names with the respective fetch and push URLs\n> +# This routine only gets called when there are more remotes than the given\n> +# limit, so we know that we have to append an ellipsis to the table and\n> +# that we have to pop one of the keys.\n\nI think it would be a better idea to make this subroutine generic: display\nonly list of remotes, limited to $limit remotes maximum.\n\n+# Display a list of remote names with the respective fetch and push URLs,\n+# without displaying remote heads (used when there are too many remotes).\n\n> +sub git_remotes_list {\n> +\tmy ($remotedata) = @_;\n> +\tprint \"\\n\";\n> +\tmy $alternate = 1;\n> +\tmy @keys = sort keys %$remotedata;\n> +\tpop @keys;\n> +\n> +\twhile (my $remote = shift @keys) {\n> +\t\tmy $rdata = $remotedata->{$remote};\n> +\t\tmy $fetch = $rdata->{'fetch'};\n> +\t\tmy $push = $rdata->{'push'};\n\nIf we do explicit limiting (not assuming that $remotedata is already\nlimited on generation), it could look like the following:\n\n+sub git_remotes_list {\n+\tmy ($remotedata, $limit) = @_;\n+\n+\tprint \"
\\n\";\n+\tmy $alternate = 1;\n+\tmy @remotes = sort keys %$remotedata;\n+\t$limit = scalar @remotes\n+\t\tunless defined $limit;\n+\n+\tfor (my $i = 0; $i < $limit; $i++) {\n+\t\tmy $remote = $remotes[$i];\n+\t\tmy $rdata = $remotedata->{$remote};\n+\t\tmy $fetch = $rdata->{'fetch'};\n+\t\tmy $push = $rdata->{'push'};\n\n> +\t\tif ($alternate) {\n> +\t\t\tprint \"\\n\";\n> +\t\t} else {\n> +\t\t\tprint \"\\n\";\n> +\t\t}\n> +\t\t$alternate ^= 1;\n> +\t\tprint \"\";\n\nVery, very minor nitpick: if \"\"\nisn't?\n\n> +\t\tprint \"\";\n> +\n> +\t\tprint \"\\n\";\n> +\t}\n> +\tprint \"\\n\" .\n> +\t \"\\n\" . \"\\n\";\n\nWith explicit limiting this would be:\n\n+\tif ($limit < @remotes) {\n+\t\tprint \"\\n\" .\n+\t\t \"\\n\" .\n+\t\t \"\\n\";\n+\t}\n\n> +\tprint \"
\" .\n> +\t\t $cgi->a({-href=> href(action=>'remotes', hash=>$remote),\n> +\t\t\t -class=> \"list name\"},esc_html($remote)) . \"\" is on separate line, why \"\" .\n> +\t\t (defined $fetch ? $cgi->a({-href=> $fetch}, \"fetch\") : \"fetch\") .\n> +\t\t \" | \" .\n> +\t\t (defined $push ? $cgi->a({-href=> $push}, \"push\") : \"push\") .\n> +\t\t \"
\" .\n> +\t $cgi->a({-href => href(action=>\"remotes\")}, \"...\") .\n> +\t \"
\" .\n+\t\t $cgi->a({-href => href(action=>\"remotes\")}, \"...\") .\n+\t\t \"
\";\n> +}\n> +\n> +# Display remote heads grouped by remote, unless there are too many\n> +# remotes ($have_all is false), in which case we only display the remote\n> +# names\n\nThis of course also would have to be changed if limiting is done at\ndisplay time, not at generation time (at least for limiting list of\nremotes, if not for generating list of refs).\n\n> +sub git_remotes_body {\n> +\tmy ($remotedata, $limit, $head) = @_;\n> +\tif (not defined $limit or scalar keys %$remotedata <= $limit) {\n> +\t\tgit_get_remote_heads($remotedata, $limit);\n> +\t\twhile (my ($remote, $rdata) = each %$remotedata) {\n> +\t\t\tgit_print_section({-class=>\"remote\", -id=>$remote},\n> +\t\t\t\t[\"remotes\", $remote, $remote], sub {\n> +\t\t\t\t\tgit_remote_body($remote, $rdata, $limit, $head);\n> +\t\t\t\t});\n> +\t\t}\n> +\t} else {\n> +\t\tgit_remotes_list($remotedata, $limit);\n> +\t}\n\nMinor issue: wouldn't it make for easier reading to have less code in\nthe 'if' part of if { ... } else { ... } clause, and more code in 'else'\npart.\n\n> +}\n\nThe rewritten function, which makes invoked display subroutines to do\nthe limiting by themselves, could look like this:\n\n+# Display remote heads grouped by remote, unless there are too many\n+# remotes, in which case we only display the remote names\n+sub git_remotes_body {\n+\tmy ($remotedata, $limit, $head) = @_;\n+\n+\tif (defined $limit && $limit < scalar keys %$remotedata) {\n+\t\t# $limit limits number of displayed remotes\n+\t\tgit_remotes_list($remotedata, $limit);\n+\n+\t} else {\n+\t\t# $limit limits number of displayed heads in each remote\n+\t\tfill_remote_heads($remotedata); ## or fill_remote_heads($remotedata, $limit+1);\n+\t\twhile (my ($remote, $rdata) = each %$remotedata) {\n+\t\t\tgit_print_section(\n+\t\t\t\t{-class=>\"remote\", -id=>$remote},\n+\t\t\t\t[\"remotes\", $remote, $remote],\n+\t\t\t\tsub { git_remote_block($remote, $rdata, $limit, $head) }\n+\t\t\t);\n+\t\t}\n+\t}\n+}\n\n\n> +\n> sub git_search_grep_body {\n> \tmy ($commitlist, $from, $to, $extra) = @_;\n> \t$from = 0 unless defined $from;\n> @@ -5200,7 +5354,7 @@ sub git_summary {\n> \t# there are more ...\n> \tmy @taglist = git_get_tags_list(16);\n> \tmy @headlist = git_get_heads_list(16, 'heads');\n> -\tmy @remotelist = $remote_heads ? git_get_heads_list(16, 'remotes') : ();\n> +\tmy $remotedata = $remote_heads ? git_get_remotes_list(-limit => 16) : undef;\n\nIf git_get_remotes_list wouldn't do limiting, it would simply be\n\n-\tmy @remotelist = $remote_heads ? git_get_heads_list(16, 'remotes') : ();\n+\tmy $remotedata = $remote_heads ? git_get_remotes_list() : undef;\n\n> \tmy @forklist;\n> \tmy $check_forks = gitweb_check_feature('forks');\n> \n> @@ -5278,11 +5432,9 @@ sub git_summary {\n> \t\t $cgi->a({-href => href(action=>\"heads\")}, \"...\"));\n> \t}\n> \n> -\tif (@remotelist) {\n> +\tif ($remotedata) {\n> \t\tgit_print_header_div('remotes');\n> -\t\tgit_heads_body(\\@remotelist, $head, 0, 15,\n> -\t\t $#remotelist <= 15 ? undef :\n> -\t\t $cgi->a({-href => href(action=>\"remotes\")}, \"...\"));\n> +\t\tgit_remotes_body($remotedata, 16, $head);\n> \t}\n> \n> \tif (@forklist) {\n\nThis wouldn't change, and would be the same even if git_get_remotes_list\nwouldn't do limiting.\n\n\nPerhaps it would be worth adding comment about git_remotes:\n\n@@ -5599,6 +5740,7 @@ sub git_heads {\n \tgit_footer_html();\n }\n \n+# dual lived: used both for single remote view, and for list of all remotes\n sub git_remotes {\n \tgitweb_check_feature('remote_heads')\n \t\tor die_error(403, \"Remote heads view is disabled\");\n\n\nThough I am not sure if it is good public API. Perhaps it is...\n\n> @@ -5606,31 +5758,26 @@ sub git_remotes {\n> \tmy $head = git_get_head_hash($project);\n> \tmy $remote = $input_params{'hash'};\n> \n> -\tmy @remotelist;\n> +\tmy $remotedata = git_get_remotes_list(-wanted => $remote);\n> +\tdie_error(500, \"Unable to get remote information\") unless defined $remotedata;\n\nIt would be little simpler with git_get_remotes_list not doing the\nlimiting, because of which we can use slightly simpler API.\n\n+\tmy $remotedata = git_get_remotes_list($remote);\n+\tdie_error(500, \"Unable to get remote information\")\n+\t\tunless defined $remotedata;\n\n> \n> -\tif (defined $remote) {\n> -\t\t# only display the heads in a given remote\n> -\t\t@remotelist = map {\n> -\t\t\tmy $ref = $_ ;\n> -\t\t\t$ref->{'name'} =~ s!^$remote/!!;\n> -\t\t\t$ref\n> -\t\t} git_get_heads_list(undef, \"remotes/$remote\");\n> -\t} else {\n> -\t\t@remotelist = git_get_heads_list(undef, 'remotes');\n> +\tif (keys(%$remotedata) == 0) {\n\nYou can write simply\n\n+\tunless (%$remotedata) {\n\n>From perldata(1): \"If you evaluate a hash in scalar context, it returns\nfalse if the hash is empty.\"\n\n> +\t\tdie_error(404, defined $remote ?\n> +\t\t\t\"Remote $remote not found\" :\n> +\t\t\t\"No remotes found\");\n> \t}\n> \n> \tgit_header_html(undef, undef, -action_extra => $remote);\n> \tgit_print_page_nav('', '', $head, undef, $head,\n> \t\tformat_ref_views($remote ? '' : 'remotes'));\n> \n> +\tgit_get_remote_heads($remotedata, undef);\n\n+\tfill_remote_heads($remotedata, undef);\n\n> \tif (defined $remote) {\n> \t\tgit_print_header_div('remotes', \"$remote remote for $project\");\n> +\t\tgit_remote_body($remote, $remotedata->{$remote}, undef, $head);\n> \t} else {\n> \t\tgit_print_header_div('summary', \"$project remotes\");\n> -\t}\n> -\n> -\tif (@remotelist) {\n> -\t\tgit_heads_body(\\@remotelist, $head);\n> +\t\tgit_remotes_body($remotedata, undef $head);\n> \t}\n\nYou are missing comma after 'undef' here\n\n-\tif (@remotelist) {\n-\t\tgit_heads_body(\\@remotelist, $head);\n+\t\tgit_remotes_body($remotedata, undef, $head);\n \t}\n\n-- \nJakub Narebski\nShadeHawk on #git\n"},{"id":"154480","messageId":"201010271007.45508.jnareb@gmail.com","threadId":"25537","inReplyTo":"201010270232.37961.jnareb@gmail.com","subject":"Re: [PATCHv6 10/10] gitweb: group remote heads by remote","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-10-27T08:07:44Z","receivedAt":"2010-10-27T08:07:44Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, 27 Oct 2010, Jakub Narebski wrote:\n> On Sun, 24 Oct 2010, Giuseppe Bilotta wrote:\n\n> > +# It is possible to limit the retrieved remotes either by number\n> > +# (specifying a -limit parameter) or by name (-wanted parameter).\n> \n> I don't quite like limiting when generating, and would prefer do limiting\n> on display, especially if not doing limiting would not affect performance\n> much (git command invoked doesn't do limiting, like in case of \n> git_get_heads_list / git_get_tags_list or *most important* parse_commits).\n> \n> Especially if it complicates code that much (see below).\n> \n> Not doing limiting here, in git_get_remotes_list (or just git_get_remotes)\n> would also make API simpler; the single optional argument would be name of\n> remote we want to retrieve.\n\nNote that you can see the changes I have mentioned here:\n\n git://repo.or.cz/git/jnareb-git.git gitweb/allheads-jn\n\n http://repo.or.cz/w/git/jnareb-git.git/shortlog/refs/heads/gitweb/allheads-jn\n http://github.com/jnareb/git/commits/gitweb%2Fallheads-jn\n \n\n-- \nJakub Narebski\nPoland\n"},{"id":"154490","messageId":"201010271438.28026.jnareb@gmail.com","threadId":"25537","inReplyTo":"1287917136-26103-11-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv6 10/10] gitweb: group remote heads by remote","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-10-27T12:38:27Z","receivedAt":"2010-10-27T12:38:27Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun, 24 Oct 2010, Giuseppe Bilotta wrote:\n\nI forgot to mention this:\n\n> +       foreach (keys %$remotes) {\n> +               my $remote = $_;\n\nWhy not simply\n\n + foreach my $remote (keys %$remotes) {\n\n-- \nJakub Narebski\nPoland\n"},{"id":"154962","messageId":"AANLkTinyi22OczYuD4urJKkfh8AzyuZoTzvzAvWa1Bo4@mail.gmail.com","threadId":"25537","inReplyTo":"201010270232.37961.jnareb@gmail.com","subject":"Re: [PATCHv6 10/10] gitweb: group remote heads by remote","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-11-02T10:49:12Z","receivedAt":"2010-11-02T10:49:12Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2010/10/27 Jakub Narebski :\n> On Sun, 24 Oct 2010, Giuseppe Bilotta wrote:\n>\n>> In remote and summary view, display a block for each remote, with the\n>> fetch and push URL(s) as well as the list of the remote heads.\n>>\n>> In summary view, if the number of remotes is higher than a prescribed\n>> limit, only display the first remotes and their fetch and push\n>> urls, without any heads information and without grouping.\n>\n> I like this idea.\n\nThanks.\n\n> I guess that we can always implement more fancy limiting in the future\n> (e.g. taking into account total number of displayed remote heads).\n\nThe limiting seems to be the most debatable part of this patch 8-)\n\n>> +# Returns a hash ref mapping remote names to their fetch and push URLs.\n>> +# We return a hash ref rather than a hash so that a simple check with defined\n>> +# can be used to tell apart the \"no remotes\" case from other kinds of\n>> +# failures.\n>\n> Just an idea (it is not necessary to implement it; it has its own\n> drawbacks): we could implement here\n>\n>  return wantsarray ? %remotes : \\%remotes;\n>\n> so the caller that wants hash, would get hash; and the one wanting\n> reference (to distinguish error from no remotes), would get hash\n> reference.\n\nIt does simplify the summary view case a little, good point.\n\n>> +#\n>> +# It is possible to limit the retrieved remotes either by number\n>> +# (specifying a -limit parameter) or by name (-wanted parameter).\n>\n> I don't quite like limiting when generating, and would prefer do limiting\n> on display, especially if not doing limiting would not affect performance\n> much (git command invoked doesn't do limiting, like in case of\n> git_get_heads_list / git_get_tags_list or *most important* parse_commits).\n>\n> Especially if it complicates code that much (see below).\n>\n> Not doing limiting here, in git_get_remotes_list (or just git_get_remotes)\n> would also make API simpler; the single optional argument would be name of\n> remote we want to retrieve.\n\nHm. By the same token, there would be no need to do the limiting even\nwhen trying to get information about a single remote, meaning we could\nmake the sub totally parameter-less. OTOH, this would make the calling\nroutine quite more complex (since it would have to check if the key is\nthere, and then select that single key etc), much more so than\nlimiting the number of displayed heads. I'll take the numerical\nlimiting off the routine.\n\n>> +#\n>> +# When a single remote is wanted, we cannot use 'git remote show -n' because\n>> +# that command always work (assuming it's a remote URL if it's not defined),\n>> +# and we cannot use 'git remote show' because that would try to make a network\n>> +# roundtrip. So the only way to find if that particular remote is defined is to\n>> +# walk the list provided by 'git remote -v' and stop if and when we find what\n>> +# we want.\n>\n> I would add 'Implemetation note: ' here, which means start with\n> 'Implementation note: When ...\" -- but it is not necessary.\n\nCan do that.\n\n>> +\n>> +# Takes a hash of remotes as first parameter and fills it by adding the\n>> +# available remote heads for each of the indicated remotes.\n>> +# A maximum number of heads can also be specified.\n>\n> All git_get_* subroutines _return_ something; this looks more like fill_*\n> function for me.\n\nI'm not particularly enamored with _get_, fill will do\n\n>\n>> +sub git_get_remote_heads {\n>> +     my ($remotes, $limit) = @_;\n>> +     my @heads = map { \"remotes/$_\" } keys %$remotes;\n>> +     my @remoteheads = git_get_heads_list($limit, @heads);\n>> +     foreach (keys %$remotes) {\n>> +             my $remote = $_;\n>> +             $remotes->{$remote}{'heads'} = [ grep {\n>> +                     $_->{'name'} =~ s!^$remote/!!\n>> +                     } @remoteheads ];\n>> +     }\n>> +}\n>\n> We could remove limiting number of heads shown per remote also from here,\n> but this time 1.) the $limit parameter is passed down to git_get_heads_list\n> which in turn uses $limit as parameter to git command  2.) it doesn't\n> simplify code almost at all:\n>\n> +sub fill_remote_heads {\n> +       my $remotes = shift;\n> +\n> +       my @heads = map { \"remotes/$_\" } keys %$remotes;\n> +       ## A QUESTION: perhaps it would be better to pass @remoteheads as parameter?\n> +       my @remoteheads = git_get_heads_list(undef, @heads);\n> +       foreach my $remote (keys %$remotes) {\n> +               $remotes->{$remote}{'heads'} =\n> +                       [ grep { $_->{'name'} =~ s!^$remote/!! } @remoteheads ];\n> +       }\n> +}\n>\n>\n> A QUESTION: perhaps it would be better to pass @remoteheads as parameter?\n> I wonder if won't end up with calling git_get_heads_list multiple times...\n> well, the improvement can be left for later.  Answering this question\n> should not affect accepting this patch series.\n\nI'm not sure I actually understand the question. Who should we pass\n@remoteheads to?\n\n>> +\n>>  sub git_get_references {\n>>       my $type = shift || \"\";\n>>       my %refs;\n>> @@ -5054,6 +5114,100 @@ sub git_heads_body {\n>>       print \"\\n\";\n>>  }\n>>\n>> +# Display a single remote block\n>\n> Note that you would end up with two very similarly named subrotines:\n> git_remote_body and git_remotes_body.  Perhaps it would be better to\n> call this one git_remote_block, or git_describe_remote?\n\nMakes sense. I'll go with git_remote_block for similarity with git_remotes_list\n\n>> +sub git_remote_body {\n>> +     my ($remote, $rdata, $limit, $head) = @_;\n>> +\n>> +     my $heads = $rdata->{'heads'};\n>> +     my $fetch = $rdata->{'fetch'};\n>> +     my $push = $rdata->{'push'};\n>\n> We could have used the following Perl shortcut\n>\n> +       my ($heads, $fetch, $push) = @{$rdata}{qw(heads fetch push)};\n>\n> but it isn't needed, and I guess it even could hurt readibility...\n\nOTOH, if readibility was a priority, we wouldn't use Perl would we ;-)\nJoking aside, I think I'll leave it as is, unless there's some\nsignificant performance improvement in using the latter syntax.\n\n>> +\n>> +     my $urls = \"\\n\" ;\n>> +\n>> +     if (defined $fetch) {\n>> +             if ($fetch eq $push) {\n>> +                     $urls .= format_repo_url(\"URL\", $fetch);\n>> +             } else {\n>> +                     $urls .= format_repo_url(\"Fetch URL\", $fetch);\n>> +                     $urls .= format_repo_url(\"Push URL\", $push) if defined $push;\n>> +             }\n>> +     } elsif (defined $push) {\n>> +             $urls .= format_repo_url(\"Push URL\", $push);\n>> +     } else {\n>> +             $urls .= format_repo_url(\"\", \"No remote URL\");\n>> +     }\n>> +\n>> +     $urls .= \"
\\n\";\n>\n> I'm not sure about naming this variable $urls...\n\nI'm open to suggestions. $urls_table ?\n\n>> +\n>> +     my ($maxheads, $dots);\n>> +     if (defined $limit) {\n>> +             $maxheads = $limit - 1;\n>> +             if ($#{$heads} > $maxheads) {\n>> +                     $dots = $cgi->a({-href => href(action=>\"remotes\", hash=>$remote)}, \"...\");\n>> +             }\n>> +     }\n>> +\n>> +     print $urls;\n>> +     git_heads_body($heads, $head, 0, $maxheads, $dots);\n>\n> Wouldn't this be simpler:\n>\n> +       my $dots;\n> +       if (defined $limit && $limit < @$heads) {\n> +               $dots = $cgi->a({-href => href(action=>\"remotes\", hash=>$remote)}, \"...\");\n> +       }\n> +\n> +       print $urls;\n> +       git_heads_body($heads, $head, 0, $limit, $dots);\n>\n> We would do similar trick as in other parts: request one item more than we\n> display to check if there is more data.\n\nRight.\n\n>> +}\n>> +\n>> +# Display a list of remote names with the respective fetch and push URLs\n>> +# This routine only gets called when there are more remotes than the given\n>> +# limit, so we know that we have to append an ellipsis to the table and\n>> +# that we have to pop one of the keys.\n>\n> I think it would be a better idea to make this subroutine generic: display\n> only list of remotes, limited to $limit remotes maximum.\n\nThat's probably a good idea. I've taken some suggestions from your\nproposed code, but kept the while loop.\n\n>> +             print \"\" .\n>> +                   $cgi->a({-href=> href(action=>'remotes', hash=>$remote),\n>> +                            -class=> \"list name\"},esc_html($remote)) . \"\";\n>\n> Very, very minor nitpick: if \"\" is on separate line, why \"\"\n> isn't?\n\nGood question.\n\n>> +     print \"\";\n>> +}\n>> +\n>> +# Display remote heads grouped by remote, unless there are too many\n>> +# remotes ($have_all is false), in which case we only display the remote\n>> +# names\n>\n> This of course also would have to be changed if limiting is done at\n> display time, not at generation time (at least for limiting list of\n> remotes, if not for generating list of refs).\n\nActually, it doesn't need to change much, since $limit was still taken\ninto consideration.\n\n>> +sub git_remotes_body {\n>> +     my ($remotedata, $limit, $head) = @_;\n>> +     if (not defined $limit or scalar keys %$remotedata <= $limit) {\n>> +             git_get_remote_heads($remotedata, $limit);\n>> +             while (my ($remote, $rdata) = each %$remotedata) {\n>> +                     git_print_section({-class=>\"remote\", -id=>$remote},\n>> +                             [\"remotes\", $remote, $remote], sub {\n>> +                                     git_remote_body($remote, $rdata, $limit, $head);\n>> +                             });\n>> +             }\n>> +     } else {\n>> +             git_remotes_list($remotedata, $limit);\n>> +     }\n>\n> Minor issue: wouldn't it make for easier reading to have less code in\n> the 'if' part of if { ... } else { ... } clause, and more code in 'else'\n> part.\n\nI was just thinking about that. It also makes the conditional more obvious.\n\n> Perhaps it would be worth adding comment about git_remotes:\n>\n> @@ -5599,6 +5740,7 @@ sub git_heads {\n>        git_footer_html();\n>  }\n>\n> +# dual lived: used both for single remote view, and for list of all remotes\n\nGood idea.\n\n> Though I am not sure if it is good public API.  Perhaps it is...\n\nThe alternative would be to have git_remote to handle the single\nremote case, and possibly even have the action name be 'remote' rather\nthan 'remotes' in that case.\n\n>> -     if (defined $remote) {\n>> -             # only display the heads in a given remote\n>> -             @remotelist = map {\n>> -                     my $ref = $_ ;\n>> -                     $ref->{'name'} =~ s!^$remote/!!;\n>> -                     $ref\n>> -             } git_get_heads_list(undef, \"remotes/$remote\");\n>> -     } else {\n>> -             @remotelist = git_get_heads_list(undef, 'remotes');\n>> +     if (keys(%$remotedata) == 0) {\n>\n> You can write simply\n>\n> +       unless (%$remotedata) {\n>\n> From perldata(1):  \"If you evaluate a hash in scalar context, it returns\n> false if the hash is empty.\"\n\nRight.\n\n>> -     if (@remotelist) {\n>> -             git_heads_body(\\@remotelist, $head);\n>> +             git_remotes_body($remotedata, undef $head);\n>>       }\n>\n> You are missing comma after 'undef' here\n\nSubtle bug. Thanks.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"155013","messageId":"201011030058.53366.jnareb@gmail.com","threadId":"25537","inReplyTo":"AANLkTinyi22OczYuD4urJKkfh8AzyuZoTzvzAvWa1Bo4@mail.gmail.com","subject":"Re: [PATCHv6 10/10] gitweb: group remote heads by remote","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-11-02T23:58:52Z","receivedAt":"2010-11-02T23:58:52Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Tue, 2 Nov 2010, Giuseppe Bilotta wrote:\n> 2010/10/27 Jakub Narebski :\n>> On Sun, 24 Oct 2010, Giuseppe Bilotta wrote:\n>>\n>>> In remote and summary view, display a block for each remote, with the\n>>> fetch and push URL(s) as well as the list of the remote heads.\n>>>\n>>> In summary view, if the number of remotes is higher than a prescribed\n>>> limit, only display the first remotes and their fetch and push\n>>> urls, without any heads information and without grouping.\n\n>> I guess that we can always implement more fancy limiting in the future\n>> (e.g. taking into account total number of displayed remote heads).\n> \n> The limiting seems to be the most debatable part of this patch 8-)\n\nI think that untill this feature gets into gitweb we can only guess\nas to what kind of limiting would look and behave best on RL cases.\n \n>>> +#\n>>> +# It is possible to limit the retrieved remotes either by number\n>>> +# (specifying a -limit parameter) or by name (-wanted parameter).\n>>\n>> I don't quite like limiting when generating, and would prefer do limiting\n>> on display, especially if not doing limiting would not affect performance\n>> much (git command invoked doesn't do limiting, like in case of\n>> git_get_heads_list / git_get_tags_list or *most important* parse_commits).\n>>\n>> Especially if it complicates code that much (see below).\n>>\n>> Not doing limiting here, in git_get_remotes_list (or just git_get_remotes)\n>> would also make API simpler; the single optional argument would be name of\n>> remote we want to retrieve.\n> \n> Hm. By the same token, there would be no need to do the limiting even\n> when trying to get information about a single remote, meaning we could\n> make the sub totally parameter-less. OTOH, this would make the calling\n> routine quite more complex (since it would have to check if the key is\n> there, and then select that single key etc), much more so than\n> limiting the number of displayed heads. \n\nTrue, we have to balance complexity in generating function vs. complexity\nin display code.\n\n> I'll take the numerical limiting off the routine.\n\nYou meant here limiting number of remotes returned (except of case of\nlimiting to single remote), isn't it?\n\n>>> +\n>>> +# Takes a hash of remotes as first parameter and fills it by adding the\n>>> +# available remote heads for each of the indicated remotes.\n>>> +# A maximum number of heads can also be specified.\n>>\n>> All git_get_* subroutines _return_ something; this looks more like fill_*\n>> function for me.\n> \n> I'm not particularly enamored with _get_, fill will do\n\nWe have fill_from_file_info, fill_project_list_info; I think \nfill_remote_heads would be a good name.\n \n>>\n>>> +sub git_get_remote_heads {\n>>> +     my ($remotes, $limit) = @_;\n>>> +     my @heads = map { \"remotes/$_\" } keys %$remotes;\n>>> +     my @remoteheads = git_get_heads_list($limit, @heads);\n>>> +     foreach (keys %$remotes) {\n>>> +             my $remote = $_;\n>>> +             $remotes->{$remote}{'heads'} = [ grep {\n>>> +                     $_->{'name'} =~ s!^$remote/!!\n>>> +                     } @remoteheads ];\n>>> +     }\n>>> +}\n>>\n>> We could remove limiting number of heads shown per remote also from here,\n>> but this time 1.) the $limit parameter is passed down to git_get_heads_list\n>> which in turn uses $limit as parameter to git command  2.) it doesn't\n>> simplify code almost at all:\n>>\n>> +sub fill_remote_heads {\n>> +       my $remotes = shift;\n>> +\n>> +       my @heads = map { \"remotes/$_\" } keys %$remotes;\n>> +       ## A QUESTION: perhaps it would be better to pass @remoteheads as parameter?\n>> +       my @remoteheads = git_get_heads_list(undef, @heads);\n>> +       foreach my $remote (keys %$remotes) {\n>> +               $remotes->{$remote}{'heads'} =\n>> +                       [ grep { $_->{'name'} =~ s!^$remote/!! } @remoteheads ];\n>> +       }\n>> +}\n\nI now think that passing limit to fill_remote_heads (like you have) might\nbe a good solution (contrary to what I wrote earlier).\n\n>> A QUESTION: perhaps it would be better to pass @remoteheads as parameter?\n>> I wonder if won't end up with calling git_get_heads_list multiple times...\n>> well, the improvement can be left for later.  Answering this question\n>> should not affect accepting this patch series.\n> \n> I'm not sure I actually understand the question. Who should we pass\n> @remoteheads to?\n\nI meant here passing @remoteheads or \\@remoteheads, i.e. result of call\nto git_get_heads_list, to fill_remote_heads (as one of parameters). \nI was worrying, perhaps unnecessarily, about calling git_get_heads_list\nmore than once... but I guess that we did it anyway.\n\n>>> +\n>>> +     my $urls = \"\\n\" ;\n>>> +\n>>> +     if (defined $fetch) {\n>>> +             if ($fetch eq $push) {\n>>> +                     $urls .= format_repo_url(\"URL\", $fetch);\n>>> +             } else {\n>>> +                     $urls .= format_repo_url(\"Fetch URL\", $fetch);\n>>> +                     $urls .= format_repo_url(\"Push URL\", $push) if defined $push;\n>>> +             }\n>>> +     } elsif (defined $push) {\n>>> +             $urls .= format_repo_url(\"Push URL\", $push);\n>>> +     } else {\n>>> +             $urls .= format_repo_url(\"\", \"No remote URL\");\n>>> +     }\n>>> +\n>>> +     $urls .= \"
\\n\";\n>>\n>> I'm not sure about naming this variable $urls...\n> \n> I'm open to suggestions. $urls_table ?\n\nIt is better.\n \n>> Though I am not sure if it is good public API.  Perhaps it is...\n> \n> The alternative would be to have git_remote to handle the single\n> remote case, and possibly even have the action name be 'remote' rather\n> than 'remotes' in that case.\n\nWe might want to have 'remote' as alias to 'remotes' anyway.\n\nThough what you propose, having separately 'remotes' for list of all\nremotes, and 'remote' + name of remote, might be a good idea.\n \n\nThank you on diligently working on this feature.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"155037","messageId":"AANLkTi=8Qz3bFCc1qocpOqsCdSWtwUHQDiwkS7H2ypad@mail.gmail.com","threadId":"25537","inReplyTo":"201011030058.53366.jnareb@gmail.com","subject":"Re: [PATCHv6 10/10] gitweb: group remote heads by remote","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-11-03T07:49:43Z","receivedAt":"2010-11-03T07:49:43Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Wed, Nov 3, 2010 at 12:58 AM, Jakub Narebski wrote:\n> On Tue, 2 Nov 2010, Giuseppe Bilotta wrote:\n>> The limiting seems to be the most debatable part of this patch 8-)\n>\n> I think that untill this feature gets into gitweb we can only guess\n> as to what kind of limiting would look and behave best on RL cases.\n\nI think that in most cases there won't be any need for limiting.\nPublic cases of lots of remotes with lots of branches are, I suspect,\nrare.\n\n>> I'll take the numerical limiting off the routine.\n>\n> You meant here limiting number of remotes returned (except of case of\n> limiting to single remote), isn't it?\n\nYes.\n\n> We have fill_from_file_info, fill_project_list_info; I think\n> fill_remote_heads would be a good name.\n\nThat's what will be in the next rehash of the patchset\n\n>>> +sub fill_remote_heads {\n>>> +       my $remotes = shift;\n>>> +\n>>> +       my @heads = map { \"remotes/$_\" } keys %$remotes;\n>>> +       ## A QUESTION: perhaps it would be better to pass @remoteheads as parameter?\n>>> +       my @remoteheads = git_get_heads_list(undef, @heads);\n>>> +       foreach my $remote (keys %$remotes) {\n>>> +               $remotes->{$remote}{'heads'} =\n>>> +                       [ grep { $_->{'name'} =~ s!^$remote/!! } @remoteheads ];\n>>> +       }\n>>> +}\n>\n> I now think that passing limit to fill_remote_heads (like you have) might\n> be a good solution (contrary to what I wrote earlier).\n\nI hadn't taken it out anyway, so that's good ;-)\n\n>>> A QUESTION: perhaps it would be better to pass @remoteheads as parameter?\n>>> I wonder if won't end up with calling git_get_heads_list multiple times...\n>>> well, the improvement can be left for later.  Answering this question\n>>> should not affect accepting this patch series.\n>>\n>> I'm not sure I actually understand the question. Who should we pass\n>> @remoteheads to?\n>\n> I meant here passing @remoteheads or \\@remoteheads, i.e. result of call\n> to git_get_heads_list, to fill_remote_heads (as one of parameters).\n> I was worrying, perhaps unnecessarily, about calling git_get_heads_list\n> more than once... but I guess that we did it anyway.\n\nI'm still not sure I follow. git_get_heads_list only gets called once,\nwith all the remotes/ pathspecs as a single array\nargument. This _does_ mean that when the total number of remote heads\nis greater than the limit some remotes will not display complete\ninformation in summary view. The real issue here is, I think, that\nthere is no trivial way to tell which remotes have incomplete\ninformation and which don't, meaning that in the subsequent\ngit_remote_block calls we'll have no way to provide visual feedback\n(the ellipsis) when some heads are missing.\n\n>>> I'm not sure about naming this variable $urls...\n>>\n>> I'm open to suggestions. $urls_table ?\n>\n> It is better.\n\nCan do.\n\n>>> Though I am not sure if it is good public API.  Perhaps it is...\n>>\n>> The alternative would be to have git_remote to handle the single\n>> remote case, and possibly even have the action name be 'remote' rather\n>> than 'remotes' in that case.\n>\n> We might want to have 'remote' as alias to 'remotes' anyway.\n>\n> Though what you propose, having separately 'remotes' for list of all\n> remotes, and 'remote' + name of remote, might be a good idea.\n\nI hope you don't mind if I opt to leave these refinements for later ;-)\n\n> Thank you on diligently working on this feature.\n\nThank you for your review.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"155160","messageId":"201011041141.58334.jnareb@gmail.com","threadId":"25537","inReplyTo":"AANLkTi=8Qz3bFCc1qocpOqsCdSWtwUHQDiwkS7H2ypad@mail.gmail.com","subject":"Re: [PATCHv6 10/10] gitweb: group remote heads by remote","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-11-04T10:41:57Z","receivedAt":"2010-11-04T10:41:57Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, 3 Nov 2010, Giuseppe Bilotta wrote:\n> On Wed, Nov 3, 2010 at 12:58 AM, Jakub Narebski wrote:\n>> On Tue, 2 Nov 2010, Giuseppe Bilotta wrote:\n>>> The limiting seems to be the most debatable part of this patch 8-)\n>>\n>> I think that untill this feature gets into gitweb we can only guess\n>> as to what kind of limiting would look and behave best on RL cases.\n> \n> I think that in most cases there won't be any need for limiting.\n> Public cases of lots of remotes with lots of branches are, I suspect,\n> rare.\n\nIt's not about _public_ cases; I think that it is in very rare cases\nthat public repository would want to display remotes and remote-tracking\nbranches.\n\nI think remote_heads feature is more important for _local_ use, for\nexample browsing one own repository using git-instaweb. In such cases\nnumber of remotes and of remote-tracking branches might be large (I have\n11 remotes, not all active, and 58 remote-tracking branches).\n\nBTW. would next version of this series include patch to git-instaweb\nenabling 'remote_heads' feature for it (gitweb_conf function)?\n\n>>>> +sub fill_remote_heads {\n>>>> +       my $remotes = shift;\n>>>> +\n>>>> +       my @heads = map { \"remotes/$_\" } keys %$remotes;\n>>>> +       ## A QUESTION: perhaps it would be better to pass @remoteheads as parameter?\n>>>> +       my @remoteheads = git_get_heads_list(undef, @heads);\n>>>> +       foreach my $remote (keys %$remotes) {\n>>>> +               $remotes->{$remote}{'heads'} =\n>>>> +                       [ grep { $_->{'name'} =~ s!^$remote/!! } @remoteheads ];\n>>>> +       }\n>>>> +}\n[...]\n>>>> A QUESTION: perhaps it would be better to pass @remoteheads as parameter?\n>>>> I wonder if won't end up with calling git_get_heads_list multiple times...\n>>>> well, the improvement can be left for later.  Answering this question\n>>>> should not affect accepting this patch series.\n>>>\n>>> I'm not sure I actually understand the question. Who should we pass\n>>> @remoteheads to?\n>>\n>> I meant here passing @remoteheads or \\@remoteheads, i.e. result of call\n>> to git_get_heads_list, to fill_remote_heads (as one of parameters).\n>> I was worrying, perhaps unnecessarily, about calling git_get_heads_list\n>> more than once... but I guess that we did it anyway.\n> \n> I'm still not sure I follow. git_get_heads_list only gets called once,\n\nAh, that's all right then. I was worring that it is called more than\nonce in one request by gitweb.\n\n> with all the remotes/ pathspecs as a single array\n> argument. This _does_ mean that when the total number of remote heads\n> is greater than the limit some remotes will not display complete\n> information in summary view. The real issue here is, I think, that\n> there is no trivial way to tell which remotes have incomplete\n> information and which don't, meaning that in the subsequent\n> git_remote_block calls we'll have no way to provide visual feedback\n> (the ellipsis) when some heads are missing.\n\nErrr... shouldn't we leave limiting number of heads to fill_remote_heads,\nwhich can do limiting per remote (with each remote having up to $limit\nremote-tracking branches / remote heads), instead of having \ngit_get_heads_list do it?\n\nSomething like this:\n\n+sub fill_remote_heads {\n+ my ($remotes, $limit) = @_;\n+\n+ my @heads = map { \"remotes/$_\" } keys %$remotes;\n+ my @remoteheads = git_get_heads_list(undef, @heads);\n+ foreach my $remote (keys %$remotes) {\n+ $remotes->{$remote}{'heads'} =\n+ [ grep { $_->{'name'} =~ s!^$remote/!! } @remoteheads ];\n+\t\t$remotes->{$remote}{'heads'} =\n+\t\t\t[ @{$remotes->{$remote}{'heads'}}[0..$limit-1] ]\n+\t\t\tif (@{$remotes->{$remote}{'heads'}} > $limit);\n+ }\n+}\n\nThough perhaps it will be more clear with if as statement, not as modifier:\n\n+sub fill_remote_heads {\n+ my ($remotes, $limit) = @_;\n+\n+ my @heads = map { \"remotes/$_\" } keys %$remotes;\n+ my @remoteheads = git_get_heads_list(undef, @heads);\n+ foreach my $remote (keys %$remotes) {\n+ $remotes->{$remote}{'heads'} =\n+ [ grep { $_->{'name'} =~ s!^$remote/!! } @remoteheads ];\n+\t\tif (@{$remotes->{$remote}{'heads'}} > $limit) {\n+\t\t\t$remotes->{$remote}{'heads'} =\n+\t\t\t\t[ @{$remotes->{$remote}{'heads'}}[0..$limit-1] ]\n+\t\t}\t\n+ }\n+}\n\n\n>>>> Though I am not sure if it is good public API.  Perhaps it is...\n>>>\n>>> The alternative would be to have git_remote to handle the single\n>>> remote case, and possibly even have the action name be 'remote' rather\n>>> than 'remotes' in that case.\n>>\n>> We might want to have 'remote' as alias to 'remotes' anyway.\n>>\n>> Though what you propose, having separately 'remotes' for list of all\n>> remotes, and 'remote' + name of remote, might be a good idea.\n> \n> I hope you don't mind if I opt to leave these refinements for later ;-)\n\nNot a problem. For backward compatibility we would have to accept\n'remotes/' in addition to more correct 'remote/', but\nthis is not a problem.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"155358","messageId":"AANLkTimxspcGLnv+F6uKAnFAhUO++iB10b5GxnLVg_v3@mail.gmail.com","threadId":"25537","inReplyTo":"201011041141.58334.jnareb@gmail.com","subject":"Re: [PATCHv6 10/10] gitweb: group remote heads by remote","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-11-08T08:28:45Z","receivedAt":"2010-11-08T08:28:45Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Thu, Nov 4, 2010 at 11:41 AM, Jakub Narebski wrote:\n> On Wed, 3 Nov 2010, Giuseppe Bilotta wrote:\n>>\n>> I think that in most cases there won't be any need for limiting.\n>> Public cases of lots of remotes with lots of branches are, I suspect,\n>> rare.\n>\n> It's not about _public_ cases; I think that it is in very rare cases\n> that public repository would want to display remotes and remote-tracking\n> branches.\n\nThat's a good point.\n\n> I think remote_heads feature is more important for _local_ use, for\n> example browsing one own repository using git-instaweb.  In such cases\n> number of remotes and of remote-tracking branches might be large (I have\n> 11 remotes, not all active, and 58 remote-tracking branches).\n>\n> BTW. would next version of this series include patch to git-instaweb\n> enabling 'remote_heads' feature for it (gitweb_conf function)?\n\nI will look into that.\n\n>> with all the remotes/ pathspecs as a single array\n>> argument. This _does_ mean that when the total number of remote heads\n>> is greater than the limit some remotes will not display complete\n>> information in summary view. The real issue here is, I think, that\n>> there is no trivial way to tell which remotes have incomplete\n>> information and which don't, meaning that in the subsequent\n>> git_remote_block calls we'll have no way to provide visual feedback\n>> (the ellipsis) when some heads are missing.\n>\n> Errr... shouldn't we leave limiting number of heads to fill_remote_heads,\n> which can do limiting per remote (with each remote having up to $limit\n> remote-tracking branches / remote heads), instead of having\n> git_get_heads_list do it?\n>\n> Something like this:\n>\n> +sub fill_remote_heads {\n> +       my ($remotes, $limit) = @_;\n> +\n> +       my @heads = map { \"remotes/$_\" } keys %$remotes;\n> +       my @remoteheads = git_get_heads_list(undef, @heads);\n> +       foreach my $remote (keys %$remotes) {\n> +               $remotes->{$remote}{'heads'} =\n> +                       [ grep { $_->{'name'} =~ s!^$remote/!! } @remoteheads ];\n> +               $remotes->{$remote}{'heads'} =\n> +                       [ @{$remotes->{$remote}{'heads'}}[0..$limit-1] ]\n> +                       if (@{$remotes->{$remote}{'heads'}} > $limit);\n> +       }\n> +}\n>\n> Though perhaps it will be more clear with if as statement, not as modifier:\n>\n> +sub fill_remote_heads {\n> +       my ($remotes, $limit) = @_;\n> +\n> +       my @heads = map { \"remotes/$_\" } keys %$remotes;\n> +       my @remoteheads = git_get_heads_list(undef, @heads);\n> +       foreach my $remote (keys %$remotes) {\n> +               $remotes->{$remote}{'heads'} =\n> +                       [ grep { $_->{'name'} =~ s!^$remote/!! } @remoteheads ];\n> +               if (@{$remotes->{$remote}{'heads'}} > $limit) {\n> +                       $remotes->{$remote}{'heads'} =\n> +                               [ @{$remotes->{$remote}{'heads'}}[0..$limit-1] ]\n> +               }\n> +       }\n> +}\n\nEither solution is fine, but it would require grabbing all the remote\nheads. The real issue here is, I think understanding what is the\npurpose of limiting in gitweb. Is it to reduce runtime? is it to\nreduce clutter on the screen? In the first case, the limiting should\nbe done as early as possible (i.e. during the git call that retrieves\nthe data); in the latter case, is it _really_ needed at all?\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"155368","messageId":"201011081205.12191.jnareb@gmail.com","threadId":"25537","inReplyTo":"AANLkTimxspcGLnv+F6uKAnFAhUO++iB10b5GxnLVg_v3@mail.gmail.com","subject":"Re: [PATCHv6 10/10] gitweb: group remote heads by remote","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-11-08T11:05:10Z","receivedAt":"2010-11-08T11:05:10Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, 8 Nov 2010, Giuseppe Bilotta wrote:\n> On Thu, Nov 4, 2010 at 11:41 AM, Jakub Narebski wrote:\n>> On Wed, 3 Nov 2010, Giuseppe Bilotta wrote:\n\n>> I think remote_heads feature is more important for _local_ use, for\n>> example browsing one own repository using git-instaweb.  In such cases\n>> number of remotes and of remote-tracking branches might be large (I have\n>> 11 remotes, not all active, and 58 remote-tracking branches).\n>>\n>> BTW. would next version of this series include patch to git-instaweb\n>> enabling 'remote_heads' feature for it (gitweb_conf function)?\n> \n> I will look into that.\n\nIt can be as simple as\n\ndiff --git i/git-instaweb.sh w/git-instaweb.sh\nindex e6f6ecd..50f65b1 100755\n--- i/git-instaweb.sh\n+++ w/git-instaweb.sh\n@@ -580,6 +580,8 @@ gitweb_conf() {\n our \\$projectroot = \"$(dirname \"$fqgitdir\")\";\n our \\$git_temp = \"$fqgitdir/gitweb/tmp\";\n our \\$projects_list = \\$projectroot;\n+\n+$feature{'remote_heads'}{'default'} = [1]\n EOF\n }\n \n\nWe might want to enable more features for git-instaweb, but I think\nit would out of scope for planned commit (for 'remote heads' series).\n\n>>> with all the remotes/ pathspecs as a single array\n>>> argument. This _does_ mean that when the total number of remote heads\n>>> is greater than the limit some remotes will not display complete\n>>> information in summary view. The real issue here is, I think, that\n>>> there is no trivial way to tell which remotes have incomplete\n>>> information and which don't, meaning that in the subsequent\n>>> git_remote_block calls we'll have no way to provide visual feedback\n>>> (the ellipsis) when some heads are missing.\n>>\n>> Errr... shouldn't we leave limiting number of heads to fill_remote_heads,\n>> which can do limiting per remote (with each remote having up to $limit\n>> remote-tracking branches / remote heads), instead of having\n>> git_get_heads_list do it?\n>>\n>> Something like this:\n>>\n>> +sub fill_remote_heads {\n>> +       my ($remotes, $limit) = @_;\n>> +\n>> +       my @heads = map { \"remotes/$_\" } keys %$remotes;\n>> +       my @remoteheads = git_get_heads_list(undef, @heads);\n>> +       foreach my $remote (keys %$remotes) {\n>> +               $remotes->{$remote}{'heads'} =\n>> +                       [ grep { $_->{'name'} =~ s!^$remote/!! } @remoteheads ];\n>> +               $remotes->{$remote}{'heads'} =\n>> +                       [ @{$remotes->{$remote}{'heads'}}[0..$limit-1] ]\n>> +                       if (@{$remotes->{$remote}{'heads'}}> $limit);\n>> +       }\n>> +}\n>>\n>> Though perhaps it will be more clear with if as statement, not as modifier:\n>>\n>> +sub fill_remote_heads {\n>> +       my ($remotes, $limit) = @_;\n>> +\n>> +       my @heads = map { \"remotes/$_\" } keys %$remotes;\n>> +       my @remoteheads = git_get_heads_list(undef, @heads);\n>> +       foreach my $remote (keys %$remotes) {\n>> +               $remotes->{$remote}{'heads'} =\n>> +                       [ grep { $_->{'name'} =~ s!^$remote/!! } @remoteheads ];\n>> +               if (@{$remotes->{$remote}{'heads'}}> $limit) {\n>> +                       $remotes->{$remote}{'heads'} =\n>> +                               [ @{$remotes->{$remote}{'heads'}}[0..$limit-1] ]\n>> +               }\n>> +       }\n>> +}\n> \n> Either solution is fine, but it would require grabbing all the remote\n> heads. The real issue here is, I think understanding what is the\n> purpose of limiting in gitweb. Is it to reduce runtime? is it to\n> reduce clutter on the screen? In the first case, the limiting should\n> be done as early as possible (i.e. during the git call that retrieves\n> the data); in the latter case, is it _really_ needed at all?\n\nIt is to reduce clutter on the screen, or rather have 'summary' view\nfor a project (for a repository) to be really a _summary_. That's why\nthere is limit of 15 commits in shortlog, of 15 branches in heads, of\n15 tags. This action is meant to present balanced overview of \nrepository.\n\n\nRegarding gitweb performance, it is quite important to pass limit to\ngit-log / git-rev-list needed also for 'summary' view; passing limit\nto git command really matters here.\n\ngit_get_heads_list passes '--count='.($limit+1) to git-for-each-ref,\nbut I don't think that it improves performance in any measurable way.\nSimilar with saving a memory: it is negligible amount. So if we can\ndo better at the cost of running git_get_heads_list without a limit,\nI say go for it.\n\nNote that the costly part of git_get_heads_list is forking git command,\nso it makes absolutely no sense to run git_get_heads_list once per\nremote instead of doing limiting per-remote in Perl. The former would\naffect performance badly, I can guess.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"155371","messageId":"AANLkTikK1C5zzfOKSucPhOAQV3E=mrOunpyv4NhN+Od6@mail.gmail.com","threadId":"25537","inReplyTo":"201011081205.12191.jnareb@gmail.com","subject":"Re: [PATCHv6 10/10] gitweb: group remote heads by remote","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-11-08T11:18:51Z","receivedAt":"2010-11-08T11:18:51Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Mon, Nov 8, 2010 at 12:05 PM, Jakub Narebski wrote:\n> On Mon, 8 Nov 2010, Giuseppe Bilotta wrote:\n>> On Thu, Nov 4, 2010 at 11:41 AM, Jakub Narebski wrote:\n>>> On Wed, 3 Nov 2010, Giuseppe Bilotta wrote:\n>\n>>> I think remote_heads feature is more important for _local_ use, for\n>>> example browsing one own repository using git-instaweb.  In such cases\n>>> number of remotes and of remote-tracking branches might be large (I have\n>>> 11 remotes, not all active, and 58 remote-tracking branches).\n>>>\n>>> BTW. would next version of this series include patch to git-instaweb\n>>> enabling 'remote_heads' feature for it (gitweb_conf function)?\n>>\n>> I will look into that.\n>\n> It can be as simple as\n>\n> diff --git i/git-instaweb.sh w/git-instaweb.sh\n> index e6f6ecd..50f65b1 100755\n> --- i/git-instaweb.sh\n> +++ w/git-instaweb.sh\n> @@ -580,6 +580,8 @@ gitweb_conf() {\n>  our \\$projectroot = \"$(dirname \"$fqgitdir\")\";\n>  our \\$git_temp = \"$fqgitdir/gitweb/tmp\";\n>  our \\$projects_list = \\$projectroot;\n> +\n> +$feature{'remote_heads'}{'default'} = [1]\n>  EOF\n>  }\n\nThanks.\n\n> We might want to enable more features for git-instaweb, but I think\n> it would out of scope for planned commit (for 'remote heads' series).\n\nDefinitely 8-)\n\n>> Either solution is fine, but it would require grabbing all the remote\n>> heads. The real issue here is, I think understanding what is the\n>> purpose of limiting in gitweb. Is it to reduce runtime? is it to\n>> reduce clutter on the screen? In the first case, the limiting should\n>> be done as early as possible (i.e. during the git call that retrieves\n>> the data); in the latter case, is it _really_ needed at all?\n>\n> It is to reduce clutter on the screen, or rather have 'summary' view\n> for a project (for a repository) to be really a _summary_.  That's why\n> there is limit of 15 commits in shortlog, of 15 branches in heads, of\n> 15 tags.  This action is meant to present balanced overview of\n> repository.\n\nOk.\n\n> Regarding gitweb performance, it is quite important to pass limit to\n> git-log / git-rev-list needed also for 'summary' view; passing limit\n> to git command really matters here.\n>\n> git_get_heads_list passes '--count='.($limit+1) to git-for-each-ref,\n> but I don't think that it improves performance in any measurable way.\n> Similar with saving a memory: it is negligible amount.  So if we can\n> do better at the cost of running git_get_heads_list without a limit,\n> I say go for it.\n\nThe gain in performance is, I believe, related to the number of heads\nand the number of remotes that are to be enumerated. 11 remotes with a\ntotal of 58 remote branches (the case you mentioned, for example)\nmight not feel much of a difference between pre- and post-filtering,\nbut something bigger might start to feel the effect.\n\nI think the strongest point in favour of post-filtering is that the\nfeature is intended for use mostly for local repositories anyway.\n\n> Note that the costly part of git_get_heads_list is forking git command,\n> so it makes absolutely no sense to run git_get_heads_list once per\n> remote instead of doing limiting per-remote in Perl.  The former would\n> affect performance badly, I can guess.\n\nThat is indeed the reason why I chose to go the single call way, even\nthough it meant having the limit end up being used somewhat\nincorrectly.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"155382","messageId":"201011081441.44572.jnareb@gmail.com","threadId":"25537","inReplyTo":"AANLkTikK1C5zzfOKSucPhOAQV3E=mrOunpyv4NhN+Od6@mail.gmail.com","subject":"Re: [PATCHv6 10/10] gitweb: group remote heads by remote","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-11-08T13:41:42Z","receivedAt":"2010-11-08T13:41:42Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, Nov 8, 2010, Giuseppe Bilotta napisał:\n> On Mon, Nov 8, 2010 at 12:05 PM, Jakub Narebski wrote:\n>> On Mon, 8 Nov 2010, Giuseppe Bilotta wrote:\n>>> On Thu, Nov 4, 2010 at 11:41 AM, Jakub Narebski wrote:\n>>>> On Wed, 3 Nov 2010, Giuseppe Bilotta wrote:\n[...]\n>>>> BTW. would next version of this series include patch to git-instaweb\n>>>> enabling 'remote_heads' feature for it (gitweb_conf function)?\n>>>\n>>> I will look into that.\n>>\n>> It can be as simple as\n>>\n>> diff --git i/git-instaweb.sh w/git-instaweb.sh\n>> index e6f6ecd..50f65b1 100755\n>> --- i/git-instaweb.sh\n>> +++ w/git-instaweb.sh\n>> @@ -580,6 +580,8 @@ gitweb_conf() {\n>>  our \\$projectroot = \"$(dirname \"$fqgitdir\")\";\n>>  our \\$git_temp = \"$fqgitdir/gitweb/tmp\";\n>>  our \\$projects_list = \\$projectroot;\n>> +\n>> +$feature{'remote_heads'}{'default'} = [1]\n>>  EOF\n>>  }\n> \n> Thanks.\n\nI forgot about trailing semicolon. It should be:\n\n +$feature{'remote_heads'}{'default'} = [1];\n\n>>> Either solution is fine, but it would require grabbing all the remote\n>>> heads. The real issue here is, I think understanding what is the\n>>> purpose of limiting in gitweb. Is it to reduce runtime? is it to\n>>> reduce clutter on the screen? In the first case, the limiting should\n>>> be done as early as possible (i.e. during the git call that retrieves\n>>> the data); in the latter case, is it _really_ needed at all?\n[...]\n\n>> Regarding gitweb performance, it is quite important to pass limit to\n>> git-log / git-rev-list needed also for 'summary' view; passing limit\n>> to git command really matters here.\n>>\n>> git_get_heads_list passes '--count='.($limit+1) to git-for-each-ref,\n>> but I don't think that it improves performance in any measurable way.\n>> Similar with saving a memory: it is negligible amount.  So if we can\n>> do better at the cost of running git_get_heads_list without a limit,\n>> I say go for it.\n> \n> The gain in performance is, I believe, related to the number of heads\n> and the number of remotes that are to be enumerated. 11 remotes with a\n> total of 58 remote branches (the case you mentioned, for example)\n> might not feel much of a difference between pre- and post-filtering,\n> but something bigger might start to feel the effect.\n\nActually I would guess it would depend on what git-for-each-ref does\nit. I would guess that git-for-each-ref reads in all refs anyway,\nthe limit only matters if format contains fieldnames that need accessing\nthe object,... like e.g. '%(subject)' which git_get_heads_list requests,\nbut git_heads_body doesn't use. Ehh...\n \n> I think the strongest point in favour of post-filtering is that the\n> feature is intended for use mostly for local repositories anyway.\n\nTrue.\n \n>> Note that the costly part of git_get_heads_list is forking git command,\n>> so it makes absolutely no sense to run git_get_heads_list once per\n>> remote instead of doing limiting per-remote in Perl.  The former would\n>> affect performance badly, I can guess.\n> \n> That is indeed the reason why I chose to go the single call way, even\n> though it meant having the limit end up being used somewhat\n> incorrectly.\n\nI think that single call and post-filtering would be reasonable \ncompromise: reasonable performance (single fork), and correct results.\n\n-- \nJakub Narebski\nPoland\n"}]}