{"thread":{"id":"25224","subject":"[PATCHv5 00/12] gitweb: remote_heads feature","startedAt":"2010-09-24T16:02:35Z","lastAt":"2010-10-23T16:17:51Z","messageCount":41,"participants":["Giuseppe Bilotta","Jakub Narebski","Ævar Arnfjörð Bjarmason","David Ripton"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"151491","messageId":"1285344167-8518-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25224","inReplyTo":null,"subject":"[PATCHv5 00/12] gitweb: remote_heads feature","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-24T16:02:35Z","receivedAt":"2010-09-24T16:02:35Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"New version of the remote heads feature for gitweb (v5 because the\nprevious rehash was actually v4, although I forgot to prefix the\npatchset accordingly).\n\nI've included all the comments received from the previous review (unless\nI forgot about something), as well as the suggestions about how to\nselect and present remotes in summary view.\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 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-9 present a new routine for grouping data (will only be used\nby the final patch in this set presently, but it has other prospective\nuses too), patch 10 introduces refactors some code that is used in\nsummary view (patch 11) and in the final version of the remotes view\n(patch 12).\n\nRemotes view now displays the remote URL(s) together with the heads (or\nonly the URL(s) in summary view if there are too many remotes), although\nI'm open to suggestions about the opportunity of displaying ssh URLs.\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 (12):\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 extra text after action in page header\n gitweb: remotes view for a single remote\n gitweb: auxiliary function to group data\n gitweb: group styling\n gitweb: git_repo_url() routine\n gitweb: use git_repo_url() in summary\n gitweb: gather more remote data\n\n gitweb/gitweb.perl | 256 ++++++++++++++++++++++++++++++++++++++++++++--\n gitweb/static/gitweb.css | 6 +\n 2 files changed, 252 insertions(+), 10 deletions(-)\n\n-- \n1.7.3.68.g6ec8\n"},{"id":"151492","messageId":"1285344167-8518-2-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25224","inReplyTo":"1285344167-8518-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv5 01/12] gitweb: introduce remote_heads feature","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-24T16:02:36Z","receivedAt":"2010-09-24T16:02:36Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"With this feature enabled, remotes 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 a85e2f6..f09fcee 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -486,6 +486,18 @@ our %feature = (\n \t\t'sub' => sub { feature_bool('highlight', @_) },\n \t\t'override' => 0,\n \t\t'default' => [0]},\n+\n+\t# Make gitweb show remotes too 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@@ -3146,10 +3158,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@@ -3160,7 +3174,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":"151494","messageId":"1285344167-8518-3-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25224","inReplyTo":"1285344167-8518-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv5 02/12] gitweb: git_get_heads_list accepts an optional list of refs.","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-24T16:02:37Z","receivedAt":"2010-09-24T16:02:37Z","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') or ('heads', 'remotes')\ndepending on the remote_heads option.\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 f09fcee..27c455e 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3155,15 +3155,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":"151505","messageId":"1285344167-8518-4-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25224","inReplyTo":"1285344167-8518-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv5 03/12] gitweb: separate heads and remotes lists","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-24T16:02:38Z","receivedAt":"2010-09-24T16:02:38Z","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 27c455e..fe9f73e 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -712,6 +712,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@@ -5111,6 +5112,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@@ -5118,7 +5120,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@@ -5196,6 +5199,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@@ -5503,13 +5513,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":"151493","messageId":"1285344167-8518-5-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25224","inReplyTo":"1285344167-8518-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv5 04/12] gitweb: nagivation menu for tags, heads and remotes","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-24T16:02:39Z","receivedAt":"2010-09-24T16:02:39Z","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 | 20 +++++++++++++++++---\n 1 files changed, 17 insertions(+), 3 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex fe9f73e..c3ce7a3 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3721,6 +3721,20 @@ 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+\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=>$_, -replay=>1)}, $_)\n+\t} @ref_views\n+}\n+\n sub format_paging_nav {\n \tmy ($action, $page, $has_next_link) = @_;\n \tmy $paging_nav;\n@@ -5497,7 +5511,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@@ -5510,7 +5524,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@@ -5526,7 +5540,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":"151495","messageId":"1285344167-8518-6-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25224","inReplyTo":"1285344167-8518-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv5 05/12] gitweb: use fullname as hash_base in heads link","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-24T16:02:40Z","receivedAt":"2010-09-24T16:02:40Z","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 c3ce7a3..e70897e 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -4977,7 +4977,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":"151496","messageId":"1285344167-8518-7-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25224","inReplyTo":"1285344167-8518-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv5 06/12] gitweb: allow extra text after action in page header","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-24T16:02:41Z","receivedAt":"2010-09-24T16:02:41Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"This extra text is intended to 'specify' the action. Therefore, if it's\npresent, the action name in the header will be turned into a link to\nthe action itself but without specifying any parameter.\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 e70897e..76cf806 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3514,7 +3514,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{'header_extra'}) {\n+\t\t\t\t$action_print = $cgi->a({-href => href(action=>$action)},\n+\t\t\t\t\tesc_html($action));\n+\t\t\t}\n+\t\t\tprint \" / $action_print\";\n+\t\t}\n+\t\tif (defined $opts{'header_extra'}) {\n+\t\t\tprint \" / $opts{'header_extra'}\";\n \t\t}\n \t\tprint \"\\n\";\n \t}\n-- \n1.7.3.68.g6ec8\n"},{"id":"151497","messageId":"1285344167-8518-8-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25224","inReplyTo":"1285344167-8518-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv5 07/12] gitweb: remotes view for a single remote","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-24T16:02:42Z","receivedAt":"2010-09-24T16:02:42Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"If the hash parameter is passed to gitweb, remotes will interpret it as\nthe name of a remote and limit the view the the heads of that remote.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 25 ++++++++++++++++++++-----\n 1 files changed, 20 insertions(+), 5 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 76cf806..7c62701 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5547,13 +5547,28 @@ 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+\tmy $remote = $input_params{'hash'};\n+\n+\tgit_header_html(undef, undef, 'header_extra' => $remote);\n+\tgit_print_page_nav('', '', $head, undef, $head,\n+\t\tformat_ref_views($remote ? '' : 'remotes'));\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+\tif (defined $remote) {\n+\t\t# only display the heads in a given remote\n+\t\tmy @headslist = 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\tif (@headslist) {\n+\t\t\tgit_heads_body(\\@headslist, $head);\n+\t\t}\n+\t} else {\n+\t\tmy @remotelist = git_get_heads_list(undef, 'remotes');\n+\t\tif (@remotelist) {\n+\t\t\tgit_heads_body(\\@remotelist, $head);\n+\t\t}\n \t}\n \tgit_footer_html();\n }\n-- \n1.7.3.68.g6ec8\n"},{"id":"151502","messageId":"1285344167-8518-9-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25224","inReplyTo":"1285344167-8518-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv5 08/12] gitweb: auxiliary function to group data","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-24T16:02:43Z","receivedAt":"2010-09-24T16:02:43Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Signed-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 16 ++++++++++++++++\n 1 files changed, 16 insertions(+), 0 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 7c62701..8f11fb5 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3786,6 +3786,22 @@ sub git_print_header_div {\n \t \"\\n\\n\";\n }\n \n+sub git_group {\n+\tmy ($class, $id, @rest) = @_;\n+\n+\tmy $content_func = pop @rest;\n+\n+\t$class = join(' ', 'group', $class);\n+\n+\tprint $cgi->start_div({\n+\t\t-class => $class,\n+\t\t-id => $id,\n+\t});\n+\tgit_print_header_div(@rest);\n+\t$content_func->() if defined $content_func;\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":"151499","messageId":"1285344167-8518-10-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25224","inReplyTo":"1285344167-8518-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv5 09/12] gitweb: group styling","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-24T16:02:44Z","receivedAt":"2010-09-24T16:02:44Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Signed-off-by: Giuseppe Bilotta \n---\n gitweb/static/gitweb.css | 6 ++++++\n 1 files changed, 6 insertions(+), 0 deletions(-)\n\ndiff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css\nindex 4132aab..4594155 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.group {\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":"151500","messageId":"1285344167-8518-11-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25224","inReplyTo":"1285344167-8518-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv5 10/12] gitweb: git_repo_url() routine","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-24T16:02:45Z","receivedAt":"2010-09-24T16:02:45Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"The routine creates a table row with a name and a repository address,\nlike the one used at the top of summary view.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 5 +++++\n 1 files changed, 5 insertions(+), 0 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 8f11fb5..2ab9327 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3802,6 +3802,11 @@ sub git_group {\n \tprint $cgi->end_div;\n }\n \n+sub git_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-- \n1.7.3.68.g6ec8\n"},{"id":"151498","messageId":"1285344167-8518-12-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25224","inReplyTo":"1285344167-8518-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv5 11/12] gitweb: use git_repo_url() in summary","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-24T16:02:46Z","receivedAt":"2010-09-24T16:02:46Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Signed-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 2ab9327..93017a4 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5190,7 +5190,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 git_repo_url($url_tag, $git_url);\n \t\t$url_tag = \"\";\n \t}\n \n-- \n1.7.3.68.g6ec8\n"},{"id":"151501","messageId":"1285344167-8518-13-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25224","inReplyTo":"1285344167-8518-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCHv5 12/12] gitweb: gather more remote data","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-24T16:02:47Z","receivedAt":"2010-09-24T16:02:47Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Collect remote information by gathering the list of remotes, and then\nthe URL(s) and heads in each remote. In summary view, limit the number\nof remotes for which we collect data, as well as the maximum number of\nheads per remote that we display.\n\nIf the number of remotes is higher than the prescribed limit, do not\ncollect any heads information and just show the remotes names and the\nlinks to the corresponding fetch and push URLs. Otherwise, create a\ngroup for each remote and display all the information there.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 171 ++++++++++++++++++++++++++++++++++++++++++++++------\n 1 files changed, 153 insertions(+), 18 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 93017a4..1e671ff 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -2758,6 +2758,57 @@ sub git_get_last_activity {\n \treturn (undef, undef);\n }\n \n+# Return an array with: a hash of remote names mapping to the corresponding\n+# remote heads, the value of the $limit parameter, and a boolean indicating\n+# whether we managed to get all the remotes or not.\n+# If $limit is specified and the number of heads found is higher, the head\n+# list is empy. Additional filtering on the number of heads can be done when\n+# displaying the remotes.\n+sub git_get_remotes_data {\n+\tmy ($limit, $wanted) = @_;\n+\tmy %remotes;\n+\topen my $fd, '-|' , git_cmd(), 'remote', '-v';\n+\treturn unless $fd;\n+\tmy $more = 1;\n+\twhile (my $remote = <$fd> and $more) {\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 = $1;\n+\t\tmy $key = $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\t$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}\n+\t}\n+\tclose $fd or return;\n+\n+\t# if the while finished with $more being true, it means we ran\n+\t# out of remotes before we hit $limit; paradoxically, it being true out\n+\t# of the loop means there are 'no more' remotes.\n+\t# Rather than waste time renaming the variable, we just read it to\n+\t# answer the question: \"did we get all remotes before we hit\n+\t# the limit?\"\n+\tif ($more) {\n+\t\tmy @heads = map { \"remotes/$_\" } keys %remotes;\n+\t\tmy @remoteheads = git_get_heads_list(undef, @heads);\n+\t\tforeach (keys %remotes) {\n+\t\t\tmy $remote = $_;\n+\t\t\t$remotes{$remote}{'heads'} = [ grep {\n+\t\t\t\t$_->{'name'} =~ s!^$remote/!!\n+\t\t\t} @remoteheads ];\n+\t\t}\n+\t}\n+\treturn (\\%remotes, $limit, $more);\n+}\n+\n sub git_get_references {\n \tmy $type = shift || \"\";\n \tmy %refs;\n@@ -5018,6 +5069,93 @@ 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, $single) = @_;\n+\tmy %rdata = %{$rdata};\n+\tmy $heads = $rdata{'heads'};\n+\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 .= git_repo_url(\"URL\", $fetch);\n+\t\t} else {\n+\t\t\t$urls .= git_repo_url(\"Fetch URL\", $fetch);\n+\t\t\t$urls .= git_repo_url(\"Push URL\", $push) if defined $push;\n+\t\t}\n+\t} elsif (defined $push) {\n+\t\t$urls .= git_repo_url(\"Push URL\", $push);\n+\t} else {\n+\t\t$urls .= git_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+\tmy $content = sub {\n+\t\tprint $urls;\n+\t\tgit_heads_body($heads, $head, 0, $maxheads, $dots);\n+\t};\n+\n+\tif (defined $single and $single) {\n+\t\t$content->();\n+\t} else {\n+\t\tgit_group(\"remotes\", $remote, \"remotes\", $remote, $remote, $content);\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 ($remotelist, $limit, $have_all, $head) = @_;\n+\tmy %remotes = %$remotelist;\n+\tif ($have_all) {\n+\t\twhile (my ($remote, $rdata) = each %remotes) {\n+\t\t\tgit_remote_body($remote, $rdata, $limit, $head);\n+\t\t}\n+\t} else {\n+\t\tprint \"\\n\";\n+\t\tmy $alternate = 1;\n+\t\twhile (my ($remote, $rdata) = each (%$remotelist)) {\n+\t\t\tmy $fetch = $rdata->{'fetch'};\n+\t\t\tmy $push = $rdata->{'push'};\n+\t\t\tif ($alternate) {\n+\t\t\t\tprint \"\\n\";\n+\t\t\t} else {\n+\t\t\t\tprint \"\\n\";\n+\t\t\t}\n+\t\t\t$alternate ^= 1;\n+\t\t\tprint \"\";\n+\t\t\tprint \"\";\n+\n+\t\t\tprint \"\\n\";\n+\t\t}\n+\t\tprint \"\\n\" .\n+\t\t \"\\n\" . \"\\n\";\n+\t\tprint \"
\" .\n+\t\t\t $cgi->a({-href=> href(action=>'remotes', hash=>$remote),\n+\t\t\t -class=> \"list name\"},esc_html($remote)) . \"\" .\n+\t\t\t (defined $fetch ? $cgi->a({-href=> $fetch}, \"fetch\") : \"fetch\") .\n+\t\t\t \" | \" .\n+\t\t\t (defined $push ? $cgi->a({-href=> $push}, \"push\") : \"push\") .\n+\t\t\t \"
\" .\n+\t\t $cgi->a({-href => href(action=>\"remotes\")}, \"...\") .\n+\t\t \"
\";\n+\t}\n+}\n+\n sub git_search_grep_body {\n \tmy ($commitlist, $from, $to, $extra) = @_;\n \t$from = 0 unless defined $from;\n@@ -5164,7 +5302,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 @remotelist = $remote_heads ? git_get_remotes_data(16) : ();\n \tmy @forklist;\n \tmy $check_forks = gitweb_check_feature('forks');\n \n@@ -5244,9 +5382,7 @@ sub git_summary {\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\tgit_remotes_body(@remotelist, $head);\n \t}\n \n \tif (@forklist) {\n@@ -5570,26 +5706,25 @@ sub git_remotes {\n \tmy $head = git_get_head_hash($project);\n \tmy $remote = $input_params{'hash'};\n \n+\tmy @remotelist = git_get_remotes_data(undef, $remote);\n+\tdie_error(500, \"Unable to get remote information\") unless @remotelist;\n+\n+\tif (keys(%{$remotelist[0]}) == 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, 'header_extra' => $remote);\n \tgit_print_page_nav('', '', $head, undef, $head,\n \t\tformat_ref_views($remote ? '' : 'remotes'));\n-\tgit_print_header_div('summary', $project);\n \n \tif (defined $remote) {\n-\t\t# only display the heads in a given remote\n-\t\tmy @headslist = 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\tif (@headslist) {\n-\t\t\tgit_heads_body(\\@headslist, $head);\n-\t\t}\n+\t\tgit_print_header_div('remotes', \"$remote remote for $project\");\n+\t\tgit_remote_body($remote, $remotelist[0]->{$remote}, undef, $head, 1);\n \t} else {\n-\t\tmy @remotelist = git_get_heads_list(undef, 'remotes');\n-\t\tif (@remotelist) {\n-\t\t\tgit_heads_body(\\@remotelist, $head);\n-\t\t}\n+\t\tgit_print_header_div('summary', \"$project remotes\");\n+\t\tgit_remotes_body(@remotelist, $head);\n \t}\n \tgit_footer_html();\n }\n-- \n1.7.3.68.g6ec8\n"},{"id":"151730","messageId":"201009261924.06237.jnareb@gmail.com","threadId":"25224","inReplyTo":"1285344167-8518-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv5 01/12] gitweb: introduce remote_heads feature","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-26T17:24:05Z","receivedAt":"2010-09-26T17:24:05Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:\n\n> With this feature enabled, remotes are retrieved (and displayed)\n> when getting (and displaying) the heads list. Typical usage would be for\n> local repository browsing, e.g. by using git-instaweb (or even a more\n> permanent gitweb setup), to check the repository status and the relation\n> between tracking branches and the originating remotes.\n> \n> Signed-off-by: Giuseppe Bilotta \n\nNice and straighforward. For what it is worth:\n\nAcked-by: Jakub Narebski \n\n> ---\n> gitweb/gitweb.perl | 18 ++++++++++++++++--\n> 1 files changed, 16 insertions(+), 2 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index a85e2f6..f09fcee 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -486,6 +486,18 @@ our %feature = (\n> \t\t'sub' => sub { feature_bool('highlight', @_) },\n> \t\t'override' => 0,\n> \t\t'default' => [0]},\n> +\n> +\t# Make gitweb show remotes too in the heads list\n\nVery minor nitpick: it should probably be (but I am not a native\nEnglish speaker, so I migh be mistaken)\n\n +\t# Make gitweb show also remotes in the heads list\n\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-- \nJakub Narebski\nPoland\n"},{"id":"151731","messageId":"201009261927.02414.jnareb@gmail.com","threadId":"25224","inReplyTo":"1285344167-8518-3-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv5 02/12] gitweb: git_get_heads_list accepts an optional list of refs.","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-26T17:27:02Z","receivedAt":"2010-09-26T17:27:02Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:\n\n> git_get_heads_list(limit, class1, class2, ...) can now be used to retrieve\n> refs/class1, refs/class2 etc. Defaults to ('heads') or ('heads', 'remotes')\n> depending on the remote_heads option.\n\nPossible (very minor) nitpick:\n\ns/on remote_heads option/on 'remote_heads' feature/\n\n> \n> Signed-off-by: Giuseppe Bilotta \n\nVery nice extension of an API. Good work. FWIW\n\nAcked-by: Jakub Narebski \n\n> ---\n> gitweb/gitweb.perl | 11 +++++++----\n> 1 files changed, 7 insertions(+), 4 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index f09fcee..27c455e 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -3155,15 +3155,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> -- \n> 1.7.3.68.g6ec8\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"151732","messageId":"201009261939.12107.jnareb@gmail.com","threadId":"25224","inReplyTo":"1285344167-8518-4-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv5 03/12] gitweb: separate heads and remotes lists","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-26T17:39:11Z","receivedAt":"2010-09-26T17:39:11Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 24 Sep 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\nNice (and still uncontroversial) solution. FWIW\n\nAcked-by: Jakub Narebski \n\n> @@ -5118,7 +5120,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> @@ -5196,6 +5199,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\nNice thing. This meanst that \"remotes\" section is displayed *only* if\n'remote_heads' feature is enabled and we actually have remote-tracking\nbranches.\n\n> @@ -5503,13 +5513,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\nIt's a pity that we can't simply write \"git_get_heads_list('heads');\",\nbut I think it would be serious overengineering for a tiny little gain.\n\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\nHmmmm... what we have there is a bit of code repetition with git_heads\nand git_remotes, but I think is unevitable... besides you would be \nextending git_remotes subroutine in subsequent patches. So it doesn't\nmatter, I think.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"151735","messageId":"201009261952.07803.jnareb@gmail.com","threadId":"25224","inReplyTo":"1285344167-8518-5-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv5 04/12] gitweb: nagivation menu for tags, heads and remotes","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-26T17:52:07Z","receivedAt":"2010-09-26T17:52:07Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:\n\n> tags, heads and remotes are all views that inspect a (particular class\n> of) refs, so allow the user to easily switch between them by adding\n> the appropriate navigation submenu to each view.\n> \n> Signed-off-by: Giuseppe Bilotta \n\nNice idea. FWIW\n\nAcked-by: Jakub Narebski \n\n> ---\n> gitweb/gitweb.perl | 20 +++++++++++++++++---\n> 1 files changed, 17 insertions(+), 3 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index fe9f73e..c3ce7a3 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -3721,6 +3721,20 @@ 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> +\n\nMinor nitpick: this empty line here is not necessary. But I think\nthat Junio can remove it when applying.\n\n> +sub format_ref_views {\n> +\tmy ($current) = @_;\n> +\tmy @ref_views = qw{tags heads};\n\nHmmm... should we pass it as argument, or use $action in place of\n$current? Each solution has its advantages and disadvantages. Current\nsolution has the advantage of avoiding using global variables, solution\nusing $action has the (supposed) advantage of automatically detecting\ncurrent action.\n\nI would probably write\n\n +\tmy $current) = shift;\n +\tmy @ref_views = qw(tags heads);\n\nbut it makes no difference, and this style is also good.\n\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=>$_, -replay=>1)}, $_)\n> +\t} @ref_views\n> +}\n[...]\n\n> -\tgit_print_page_nav('','', $head,undef,$head);\n> +\tgit_print_page_nav('','', $head,undef,$head,format_ref_views('tags'));\n\n> -\tgit_print_page_nav('','', $head,undef,$head);\n> +\tgit_print_page_nav('','', $head,undef,$head,format_ref_views('heads'));\n\n> -\tgit_print_page_nav('','', $head,undef,$head);\n> +\tgit_print_page_nav('','', $head,undef,$head,format_ref_views('remotes'));\n\nNice API. I like it.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"151736","messageId":"201009261957.43923.jnareb@gmail.com","threadId":"25224","inReplyTo":"1285344167-8518-6-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv5 05/12] gitweb: use fullname as hash_base in heads link","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-26T17:57:43Z","receivedAt":"2010-09-26T17:57:43Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 24 Sep 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\nWell, of course of 'hash' parameter uses 'fullname' then 'hash_base'\nalso should. Good futureproofing, no argument here. FWIW\n\nAcked-by: Jakub Narebski \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 c3ce7a3..e70897e 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -4977,7 +4977,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\n-- \nJakub Narebski\nPoland\n"},{"id":"151737","messageId":"201009262011.28211.jnareb@gmail.com","threadId":"25224","inReplyTo":"1285344167-8518-7-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv5 06/12] gitweb: allow extra text after action in page header","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-26T18:11:27Z","receivedAt":"2010-09-26T18:11:27Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:\n\n> This extra text is intended to 'specify' the action. Therefore, if it's\n> present, the action name in the header will be turned into a link to\n> the action itself but without specifying any parameter.\n\nWhat this feature is intended *for*? I guess it is meant to be used\nin the case where there is additional parameter that specifies action,\nlike e.g. a single remote view, where $action is 'remotes', but there\nis extra parameter ('hash_base' is (ab)used for this) that specifies\nremote. Then we want to make 'remotes' in \"breadcrumbs\" navigation at\ntop of page to link to generic 'remotes' view. Isn't it?\n\nBut the above is just my guess, covering only case where there is both\n$action defined, and 'header_extra' option set.\n\nYou need to explain this in the commit message.\n\n> \n> Signed-off-by: Giuseppe Bilotta \n> ---\n> gitweb/gitweb.perl | 10 +++++++++-\n> 1 files changed, 9 insertions(+), 1 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index e70897e..76cf806 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -3514,7 +3514,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{'header_extra'}) {\n\nWe spell optional named parameter with '-' as prefix, for example\n$opts{'-remove_signoff'} in git_print_log(), to be able to use key\nwithout quoting (bareword-like, autoquoting), like $opts{-nohtml}\nor $opts{-pad_before}, or $opts{-size}, or $opts{-tag}... though\ngitweb is not very consisnte here.\n\nSo it should probably be\n\n +\t\t\tif (defined $opts{'-header_extra'}) {\n\nor even\n\n +\t\t\tif (defined $opts{-header_extra}) {\n\n\nI also think that we can think of better name for this option than\n'header_extra', although what this name could be eludes me.\n\n> +\t\t\t\t$action_print = $cgi->a({-href => href(action=>$action)},\n> +\t\t\t\t\tesc_html($action));\n\nI don't think we need to run esc_html on action: it is checked that it\nis one of specified set of possible values, and it can be ensured that\nit neither contains anything than printable characters, not any HTML\nspecial characters.\n\n> +\t\t\t}\n> +\t\t\tprint \" / $action_print\";\n> +\t\t}\n> +\t\tif (defined $opts{'header_extra'}) {\n> +\t\t\tprint \" / $opts{'header_extra'}\";\n\nHmmm...\n\n> \t\t}\n> \t\tprint \"\\n\";\n> \t}\n> -- \n> 1.7.3.68.g6ec8\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"151739","messageId":"201009262018.50565.jnareb@gmail.com","threadId":"25224","inReplyTo":"1285344167-8518-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv5 00/12] gitweb: remote_heads feature","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-26T18:18:50Z","receivedAt":"2010-09-26T18:18:50Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:\n\n> New version of the remote heads feature for gitweb (v5 because the\n> previous rehash was actually v4, although I forgot to prefix the\n> patchset accordingly).\n> \n> I've included all the comments received from the previous review (unless\n> I forgot about something), as well as the suggestions about how to\n> select and present remotes in summary view.\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 useful even without the rest of the series.\n\nThose patches can go as-is. For what it is worth they all have ACK\nfrom me.\n \nI am working on review of other patches in this series.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"151745","messageId":"AANLkTikotGEOOeUOwz03xtL09fV+oycV3yG1O4hQhoQB@mail.gmail.com","threadId":"25224","inReplyTo":"201009261924.06237.jnareb@gmail.com","subject":"Re: [PATCHv5 01/12] gitweb: introduce remote_heads feature","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-26T19:19:10Z","receivedAt":"2010-09-26T19:19:10Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"2010/9/26 Jakub Narebski :\n> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:\n>> +     # Make gitweb show remotes too in the heads list\n>\n> Very minor nitpick: it should probably be (but I am not a native\n> English speaker, so I migh be mistaken)\n>\n>  +     # Make gitweb show also remotes in the heads list\n\nMaybe:\n\n # Configure the display of remotes in the heads list\n\nor\n\n # Toggle the display of remotes in the heads list\n\n?\n"},{"id":"151746","messageId":"4C9FA35F.4090800@ripton.net","threadId":"25224","inReplyTo":"AANLkTikotGEOOeUOwz03xtL09fV+oycV3yG1O4hQhoQB@mail.gmail.com","subject":"Re: [PATCHv5 01/12] gitweb: introduce remote_heads feature","fromName":"David Ripton","fromEmail":"dripton@ripton.net","sentAt":"2010-09-26T19:47:43Z","receivedAt":"2010-09-26T19:47:43Z","isPatch":false,"sender":{"key":"dripton@ripton.net","avatar":"https://avatars.githubusercontent.com/u/153528?v=4"},"body":"On 09/26/10 15:19, Ævar Arnfjörð Bjarmason wrote:\n> 2010/9/26 Jakub Narebski:\n>> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:\n>>> + # Make gitweb show remotes too in the heads list\n>>\n>> Very minor nitpick: it should probably be (but I am not a native\n>> English speaker, so I migh be mistaken)\n>>\n>> + # Make gitweb show also remotes in the heads list\n>\n> Maybe:\n>\n> # Configure the display of remotes in the heads list\n>\n> or\n>\n> # Toggle the display of remotes in the heads list\n\nIMO either of those are fine. Or you could just swap a couple of words \nin the original message to make it sound a bit more natural:\n\n# Make gitweb also show remotes in the heads list\n\n-- \nDavid Ripton dripton@ripton.net\n"},{"id":"151749","messageId":"201009262255.45959.jnareb@gmail.com","threadId":"25224","inReplyTo":"1285344167-8518-8-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv5 07/12] gitweb: remotes view for a single remote","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-26T20:55:44Z","receivedAt":"2010-09-26T20:55:44Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:\n\n> If the hash parameter is passed to gitweb, remotes will interpret it as\n> the name of a remote and limit the view the the heads of that remote.\n\nErrr... I think this commit message needs rewriting to be more clear.\nPerhaps:\n\n When 'remotes' view is passed 'hash' parameter, it would interprete it\n as the name of a remote ...\n\n> \n> Signed-off-by: Giuseppe Bilotta \n> ---\n> gitweb/gitweb.perl | 25 ++++++++++++++++++++-----\n> 1 files changed, 20 insertions(+), 5 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 76cf806..7c62701 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -5547,13 +5547,28 @@ 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> +\tmy $remote = $input_params{'hash'};\n\nI am not sure about using 'hash' parameter for that.\n\nOn one hand it is a hack that allow us to not worry about adding extra\ncode to evaluate_path_info() subroutine, so that natural path_info URL\nof http://git.example.com/repo.git/remotes/ would use \nas name of remote to limit to.\n\nOn the other hand it is abusing semantic of 'hash' parameter. Remote\nname is not revision name or object id. \n\nWhat makes this issue stronger is the fact that URL is part of API,\nand if we make mistake here, we would have to maintain backward \ncompatibility (at least if it appears in a released version).\n\n> +\n> +\tgit_header_html(undef, undef, 'header_extra' => $remote);\n\nI don't quite like the name of this parameter, and I am not sure\nif I like the API either.\n\n> +\tgit_print_page_nav('', '', $head, undef, $head,\n> +\t\tformat_ref_views($remote ? '' : 'remotes'));\n\nWhy this change?\n\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> +\tif (defined $remote) {\n> +\t\t# only display the heads in a given remote\n> +\t\tmy @headslist = 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\nHmmm... do we need this temporary variable? Does it make anything\nmore clear?\n\n> +\t\tif (@headslist) {\n> +\t\t\tgit_heads_body(\\@headslist, $head);\n> +\t\t}\n\nThis part is the same (modulo name of variable) in both branches of this\nconditional.\n\n> +\t} else {\n> +\t\tmy @remotelist = git_get_heads_list(undef, 'remotes');\n> +\t\tif (@remotelist) {\n> +\t\t\tgit_heads_body(\\@remotelist, $head);\n> +\t\t}\n> \t}\n> \tgit_footer_html();\n> }\n> -- \n> 1.7.3.68.g6ec8\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"151751","messageId":"201009262347.15779.jnareb@gmail.com","threadId":"25224","inReplyTo":"1285344167-8518-9-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv5 08/12] gitweb: auxiliary function to group data","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-26T21:47:15Z","receivedAt":"2010-09-26T21:47:15Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:\n\n> Subject: gitweb: auxiliary function to group data\n>\n\nErrr... what!? git_group() is not \"auxiliary function to group data\",\nbut a template for output of group of data.\n\nIt would be probably good to describe how this output looks like\n(using e.g. ASII-art mockup) in a commit message.\n\n> Signed-off-by: Giuseppe Bilotta \n> ---\n> gitweb/gitweb.perl | 16 ++++++++++++++++\n> 1 files changed, 16 insertions(+), 0 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 7c62701..8f11fb5 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -3786,6 +3786,22 @@ sub git_print_header_div {\n> \t \"\\n\\n\";\n> }\n> \n> +sub git_group {\n\nName?\n\n> +\tmy ($class, $id, @rest) = @_;\n> +\n> +\tmy $content_func = pop @rest;\n> +\n> +\t$class = join(' ', 'group', $class);\n> +\n> +\tprint $cgi->start_div({\n> +\t\t-class => $class,\n> +\t\t-id => $id,\n> +\t});\n> +\tgit_print_header_div(@rest);\n> +\t$content_func->() if defined $content_func;\n\nMore defensive programming would be to use\n\n +\t$content_func->() if ref($content_func) eq 'CODE';\n\nOr even:\n\n +\tif (ref($content) eq 'CODE') {\n +\t\t$content->();\n +\t} elsif (ref($content) eq 'ARRAY') {\n +\t\tprint @$content;\n +\t} elsif (!ref($content) && defined($content)) {\n +\t\tprint $content;\n +\t}\n\nWell, $content could be also open filehandle...\n\n> +\tprint $cgi->end_div;\n> +}\n\nNice usage of start_div and end_div.\n\n> +\n> sub print_local_time {\n> \tprint format_local_time(@_);\n> }\n> -- \n> 1.7.3.68.g6ec8\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"151752","messageId":"201009270010.45142.jnareb@gmail.com","threadId":"25224","inReplyTo":"1285344167-8518-10-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv5 09/12] gitweb: group styling","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-26T22:10:44Z","receivedAt":"2010-09-26T22:10:44Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:\n\n> +div.group {\n> +\tmargin: .5em;\n> +\tborder: 1px solid #d9d8d1;\n> +\tdisplay: inline-block;\n> +}\n\nAs I currently at this moment don't use web browser which supports \n'display: inline-block;', I can't say much about this patch. But\nit does degrade nicely.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"151754","messageId":"201009270034.29603.jnareb@gmail.com","threadId":"25224","inReplyTo":"1285344167-8518-11-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv5 10/12] gitweb: git_repo_url() routine","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-26T22:34:28Z","receivedAt":"2010-09-26T22:34:28Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:\n\n> The routine creates a table row with a name and a repository address,\n> like the one used at the top of summary view.\n[...]\n\n> +sub git_repo_url {\n> +\tmy ($name, $url) = @_;\n> +\treturn \"$name$url\\n\";\n> +}\n\nIt should be *format_repo_url*, and not git_repo_url, isn't it?\n\nBy the way, doesn't gitweb include code dealing with repository URL;\nwhy you don't _use_ this subroutine, then?\n\n-- \nJakub Narebski\nPoland\n"},{"id":"151755","messageId":"201009270036.09100.jnareb@gmail.com","threadId":"25224","inReplyTo":"1285344167-8518-12-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv5 11/12] gitweb: use git_repo_url() in summary","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-26T22:36:08Z","receivedAt":"2010-09-26T22:36:08Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:\n\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -5190,7 +5190,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 git_repo_url($url_tag, $git_url);\n> \t\t$url_tag = \"\";\n> \t}\n\nI think this patch should be squashed together with previous one.\nFWIW, ACK (after squashing of course).\n\n-- \nJakub Narebski\nPoland\n"},{"id":"151794","messageId":"AANLkTi=QOMW3CaoiWja-it7+U0H2Nz95CC8-nkJ60jm2@mail.gmail.com","threadId":"25224","inReplyTo":"4C9FA35F.4090800@ripton.net","subject":"Re: [PATCHv5 01/12] gitweb: introduce remote_heads feature","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-27T06:42:54Z","receivedAt":"2010-09-27T06:42:54Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Sun, Sep 26, 2010 at 9:47 PM, David Ripton wrote:\n> On 09/26/10 15:19, Ævar Arnfjörð Bjarmason wrote:\n>>\n>> 2010/9/26 Jakub Narebski:\n>>>\n>>> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:\n>>>>\n>>>> +     # Make gitweb show remotes too in the heads list\n>>>\n>>> Very minor nitpick: it should probably be (but I am not a native\n>>> English speaker, so I migh be mistaken)\n>>>\n>>>  +     # Make gitweb show also remotes in the heads list\n>>\n>> Maybe:\n>>\n>>     # Configure the display of remotes in the heads list\n>>\n>> or\n>>\n>>     # Toggle the display of remotes in the heads list\n>\n> IMO either of those are fine.  Or you could just swap a couple of words in\n> the original message to make it sound a bit more natural:\n>\n> # Make gitweb also show remotes in the heads list\n\nI think I like the 'Configure the display' one better. I'll rephrase\nthe comment in the next rehash of the series.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"151796","messageId":"AANLkTi=nG9Day4ZCF88JwyeNx2qEM=+hKaKoyWRSTUFW@mail.gmail.com","threadId":"25224","inReplyTo":"201009261952.07803.jnareb@gmail.com","subject":"Re: [PATCHv5 04/12] gitweb: nagivation menu for tags, heads and remotes","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-27T06:48:30Z","receivedAt":"2010-09-27T06:48:30Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2010/9/26 Jakub Narebski :\n> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:\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>> +\n>\n> Minor nitpick: this empty line here is not necessary.  But I think\n> that Junio can remove it when applying.\n\nSince there have been a couple of stylistic suggestions for the\ncomments in the first two patches too, I can probably resend the whole\nseries including these changes, unless Junio wants to do the\nhand-tuning.\n\n>> +sub format_ref_views {\n>> +     my ($current) = @_;\n>> +     my @ref_views = qw{tags heads};\n>\n> Hmmm... should we pass it as argument, or use $action in place of\n> $current?  Each solution has its advantages and disadvantages.  Current\n> solution has the advantage of avoiding using global variables, solution\n> using $action has the (supposed) advantage of automatically detecting\n> current action.\n\nNot using $action has the advantage of making it possible to enable\nthe $action command if it's wanted, which is something that I use in a\nsubsequent patch (when enabling single-remote view). But this is of\ncourse debatable.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"151797","messageId":"AANLkTinpZ5gusCG4SoKbi54URyn7T4THNp2NeVM6uFL2@mail.gmail.com","threadId":"25224","inReplyTo":"201009262011.28211.jnareb@gmail.com","subject":"Re: [PATCHv5 06/12] gitweb: allow extra text after action in page header","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-27T06:56:20Z","receivedAt":"2010-09-27T06:56:20Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2010/9/26 Jakub Narebski :\n> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:\n>\n>> This extra text is intended to 'specify' the action. Therefore, if it's\n>> present, the action name in the header will be turned into a link to\n>> the action itself but without specifying any parameter.\n>\n> What this feature is intended *for*?  I guess it is meant to be used\n> in the case where there is additional parameter that specifies action,\n> like e.g. a single remote view, where $action is 'remotes', but there\n> is extra parameter ('hash_base' is (ab)used for this) that specifies\n> remote.  Then we want to make 'remotes' in \"breadcrumbs\" navigation at\n> top of page to link to generic 'remotes' view.  Isn't it?\n\nYes, this is exactly the intended purpose of this feature. It is of\ncourse possible to extend other views to have a similar behavior (I'm\nthinking in particular about log views here).\n\n> But the above is just my guess, covering only case where there is both\n> $action defined, and 'header_extra' option set.\n>\n> You need to explain this in the commit message.\n\nI will clarify this in the next rehash of the patch.\n\n>> Signed-off-by: Giuseppe Bilotta \n>> ---\n>>  gitweb/gitweb.perl |   10 +++++++++-\n>>  1 files changed, 9 insertions(+), 1 deletions(-)\n>>\n>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>> index e70897e..76cf806 100755\n>> --- a/gitweb/gitweb.perl\n>> +++ b/gitweb/gitweb.perl\n>> @@ -3514,7 +3514,15 @@ EOF\n>>       if (defined $project) {\n>>               print $cgi->a({-href => href(action=>\"summary\")}, esc_html($project));\n>>               if (defined $action) {\n>> -                     print \" / $action\";\n>> +                     my $action_print = $action ;\n>> +                     if (defined $opts{'header_extra'}) {\n>\n> We spell optional named parameter with '-' as prefix, for example\n> $opts{'-remove_signoff'} in git_print_log(), to be able to use key\n> without quoting (bareword-like, autoquoting), like $opts{-nohtml}\n> or $opts{-pad_before}, or $opts{-size}, or $opts{-tag}... though\n> gitweb is not very consisnte here.\n>\n> So it should probably be\n>\n>  +                     if (defined $opts{'-header_extra'}) {\n>\n> or even\n>\n>  +                     if (defined $opts{-header_extra}) {\n>\n>\n> I also think that we can think of better name for this option than\n> 'header_extra', although what this name could be eludes me.\n\nI will add the dash to the option. Naming it header_extra keeps the\nmeaning of this extra text generic, but considering that the intended\nuse is mostly for the single-remote view (or similar, if/when they are\nadded) we could call it something related (I can only think of\n'main_argument' right now but I think this would suck more than\nheader_extra).\n\n>> +                             $action_print = $cgi->a({-href => href(action=>$action)},\n>> +                                     esc_html($action));\n>\n> I don't think we need to run esc_html on action: it is checked that it\n> is one of specified set of possible values, and it can be ensured that\n> it neither contains anything than printable characters, not any HTML\n> special characters.\n\nOk, I will remove it.\n\n>> +                     }\n>> +                     print \" / $action_print\";\n>> +             }\n>> +             if (defined $opts{'header_extra'}) {\n>> +                     print \" / $opts{'header_extra'}\";\n>\n> Hmmm...\n\nYou don't sound very convinced. I had some doubts myself about whether\nthe slash should be inserted autmatically or whether it should be up\nto the caller to include it in header_extra, but I'm not sure this is\nwhat you are perplexed about.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"151799","messageId":"AANLkTimemfDPdMycasFmqUvk0eF-eD9z7P1RFnitLD9G@mail.gmail.com","threadId":"25224","inReplyTo":"201009262255.45959.jnareb@gmail.com","subject":"Re: [PATCHv5 07/12] gitweb: remotes view for a single remote","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-27T07:11:57Z","receivedAt":"2010-09-27T07:11:57Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2010/9/26 Jakub Narebski :\n> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:\n>\n>> If the hash parameter is passed to gitweb, remotes will interpret it as\n>> the name of a remote and limit the view the the heads of that remote.\n>\n> Errr... I think this commit message needs rewriting to be more clear.\n> Perhaps:\n>\n>  When 'remotes' view is passed 'hash' parameter, it would interprete it\n>  as the name of a remote ...\n\nCan do.\n\n>> Signed-off-by: Giuseppe Bilotta \n>> ---\n>>  gitweb/gitweb.perl |   25 ++++++++++++++++++++-----\n>>  1 files changed, 20 insertions(+), 5 deletions(-)\n>>\n>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>> index 76cf806..7c62701 100755\n>> --- a/gitweb/gitweb.perl\n>> +++ b/gitweb/gitweb.perl\n>> @@ -5547,13 +5547,28 @@ sub git_remotes {\n>>               or die_error(403, \"Remote heads view is disabled\");\n>>\n>>       my $head = git_get_head_hash($project);\n>> -     git_header_html();\n>> -     git_print_page_nav('','', $head,undef,$head,format_ref_views('remotes'));\n>> +     my $remote = $input_params{'hash'};\n>\n> I am not sure about using 'hash' parameter for that.\n>\n> On one hand it is a hack that allow us to not worry about adding extra\n> code to evaluate_path_info() subroutine, so that natural path_info URL\n> of http://git.example.com/repo.git/remotes/ would use \n> as name of remote to limit to.\n\nIt also spares us the introduction of a new parameter, since I'm not\naware of any of the current parameters that could take this role.\n\n> On the other hand it is abusing semantic of 'hash' parameter.  Remote\n> name is not revision name or object id.\n\nTrue. But I cannot think of a use for the hash parameter in remotes\nview, and since hash is also used for _named_ refs, it \"kind of\"\nmakes sense to use for a name that is not actually a hash or ref. The\nonly other option (aside from the use of a new parameter) would be to\nuse 'hash_base', by reading it as 'base for the ref names' in the\nsense that remote refs are 'refs/remote//name' where the base is\nthe remote name. But I doubt that's actually more sensible than using\n'hash'.\n\n> What makes this issue stronger is the fact that URL is part of API,\n> and if we make mistake here, we would have to maintain backward\n> compatibility (at least if it appears in a released version).\n\nWe _could_ go on the safe side and use a new parameter for this.\n\n>> +\n>> +     git_header_html(undef, undef, 'header_extra' => $remote);\n>\n> I don't quite like the name of this parameter, and I am not sure\n> if I like the API either.\n>\n>> +     git_print_page_nav('', '',  $head, undef, $head,\n>> +             format_ref_views($remote ? '' : 'remotes'));\n>\n> Why this change?\n\nAs I mentioned in my replies to the other respective patches, I think\nit makes sense to make \"all remotes\" view easily accessible from the\n\"single remote\" view, and there are two ways I can think of: one is\nthe \"extra header text\" way, by making the action name before it point\nto \"all remotes\". The other is to enable 'remotes' in the page nav\nsubmenu when we are in single remotes view (which is why I had the\n$current in format_ref_views instead of $action, and which is what is\ndone by this change). IMO it makes sense to have both ways available,\nbut I'm open to suggestions about different approaches.\n\n>>       git_print_header_div('summary', $project);\n>>\n>> -     my @remotelist = git_get_heads_list(undef, 'remotes');\n>> -     if (@remotelist) {\n>> -             git_heads_body(\\@remotelist, $head);\n>> +     if (defined $remote) {\n>> +             # only display the heads in a given remote\n>> +             my @headslist = map {\n>> +                     my $ref = $_ ;\n>> +                     $ref->{'name'} =~ s!^$remote/!!;\n>> +                     $ref\n>> +             } git_get_heads_list(undef, \"remotes/$remote\");\n>\n> Hmmm... do we need this temporary variable?  Does it make anything\n> more clear?\n>\n>> +             if (@headslist) {\n>> +                     git_heads_body(\\@headslist, $head);\n>> +             }\n>\n> This part is the same (modulo name of variable) in both branches of this\n> conditional.\n>\n>> +     } else {\n>> +             my @remotelist = git_get_heads_list(undef, 'remotes');\n>> +             if (@remotelist) {\n>> +                     git_heads_body(\\@remotelist, $head);\n>> +             }\n>>       }\n>>       git_footer_html();\n\nYup, I can make the logic here cleaner and reuse variables (and code)\nacross the conditionals.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"151800","messageId":"AANLkTimOJ7RXDWXy=tF+rZf1gnfB7_GHCZuU5bZ5Wc91@mail.gmail.com","threadId":"25224","inReplyTo":"201009262347.15779.jnareb@gmail.com","subject":"Re: [PATCHv5 08/12] gitweb: auxiliary function to group data","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-27T07:26:25Z","receivedAt":"2010-09-27T07:26:25Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2010/9/26 Jakub Narebski :\n> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:\n>\n>> Subject: gitweb: auxiliary function to group data\n>>\n>\n> Errr... what!?  git_group() is not \"auxiliary function to group data\",\n> but a template for output of group of data.\n\nI will rephrase\n\n> It would be probably good to describe how this output looks like\n> (using e.g. ASII-art mockup) in a commit message.\n\nWell, that would depend on the CSS that is used ... should I squash\nthe styling in this patch then?\n\n>> +sub git_group {\n>\n> Name?\n\ngit_collection? git_collect_data? I'm a little short on ideas.\ngit_section? git_subsection? the function (with different styling) can\nprobably be used even for the main sections in each view (think\nsummary view in particular).\n\n>> +     my ($class, $id, @rest) = @_;\n>> +\n>> +     my $content_func = pop @rest;\n>> +\n>> +     $class = join(' ', 'group', $class);\n>> +\n>> +     print $cgi->start_div({\n>> +             -class => $class,\n>> +             -id => $id,\n>> +     });\n>> +     git_print_header_div(@rest);\n>> +     $content_func->() if defined $content_func;\n>\n> More defensive programming would be to use\n>\n>  +     $content_func->() if ref($content_func) eq 'CODE';\n>\n> Or even:\n>\n>  +     if (ref($content) eq 'CODE') {\n>  +             $content->();\n>  +     } elsif (ref($content) eq 'ARRAY') {\n>  +             print @$content;\n>  +     } elsif (!ref($content) && defined($content)) {\n>  +             print $content;\n>  +     }\n>\n> Well, $content could be also open filehandle...\n\nAh, very interesting and very flexible, I'll steal your idea.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"151801","messageId":"AANLkTikg8ya7WNS0-Tep2s+D57Qkxo-GfuiZmq7YOJgm@mail.gmail.com","threadId":"25224","inReplyTo":"201009270010.45142.jnareb@gmail.com","subject":"Re: [PATCHv5 09/12] gitweb: group styling","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-27T07:27:10Z","receivedAt":"2010-09-27T07:27:10Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2010/9/27 Jakub Narebski :\n> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:\n>\n>> +div.group {\n>> +     margin: .5em;\n>> +     border: 1px solid #d9d8d1;\n>> +     display: inline-block;\n>> +}\n>\n> As I currently at this moment don't use web browser which supports\n> 'display: inline-block;', I can't say much about this patch.  But\n> it does degrade nicely.\n\nWould you think it's sensible to squash this with the previous patch, btw?\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"151802","messageId":"AANLkTi=N-TRCHFRo5G8oXUDoe3H_Xv+BTpvc9PwjM6Pu@mail.gmail.com","threadId":"25224","inReplyTo":"201009270034.29603.jnareb@gmail.com","subject":"Re: [PATCHv5 10/12] gitweb: git_repo_url() routine","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-27T07:29:36Z","receivedAt":"2010-09-27T07:29:36Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2010/9/27 Jakub Narebski :\n> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:\n>\n>> The routine creates a table row with a name and a repository address,\n>> like the one used at the top of summary view.\n> [...]\n>\n>> +sub git_repo_url {\n>> +     my ($name, $url) = @_;\n>> +     return \"$name$url\\n\";\n>> +}\n>\n> It should be  *format_repo_url*, and not git_repo_url, isn't it?\n\nRight.\n\n> By the way, doesn't gitweb include code dealing with repository URL;\n> why you don't _use_ this subroutine, then?\n\nThe only code for the display of a repo URL is inlined into summary\nview, which in the next patch uses this function to do the display. I\ncan probably squash them together.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"151804","messageId":"201009270942.14731.jnareb@gmail.com","threadId":"25224","inReplyTo":"AANLkTinpZ5gusCG4SoKbi54URyn7T4THNp2NeVM6uFL2@mail.gmail.com","subject":"Re: [PATCHv5 06/12] gitweb: allow extra text after action in page header","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-27T07:42:13Z","receivedAt":"2010-09-27T07:42:13Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, 27 Sep 2010, Giuseppe Bilotta wrote:\n> 2010/9/26 Jakub Narebski :\n>> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:\n\n>>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>>> index e70897e..76cf806 100755\n>>> --- a/gitweb/gitweb.perl\n>>> +++ b/gitweb/gitweb.perl\n>>> @@ -3514,7 +3514,15 @@ EOF\n>>>       if (defined $project) {\n>>>               print $cgi->a({-href => href(action=>\"summary\")}, esc_html($project));\n>>>               if (defined $action) {\n>>> -                     print \" / $action\";\n>>> +                     my $action_print = $action ;\n>>> +                     if (defined $opts{'header_extra'}) {\n[...]\n\n>> I also think that we can think of better name for this option than\n>> 'header_extra', although what this name could be eludes me.\n> \n> I will add the dash to the option. Naming it header_extra keeps the\n> meaning of this extra text generic, but considering that the intended\n> use is mostly for the single-remote view (or similar, if/when they are\n> added) we could call it something related (I can only think of\n> 'main_argument' right now but I think this would suck more than\n> header_extra).\n\nPerhaps name this argument '-subaction' or '-action_argument', or\nsomething like that; the meaning of this argument (as shown by the fact\nthat action name is hyperlinked) is to specify subaction of given action,\ni.e. (possibly) more detailed view of some part of generic (argument-less)\naction output.\n \n>>> +                     }\n>>> +                     print \" / $action_print\";\n>>> +             }\n>>> +             if (defined $opts{'header_extra'}) {\n>>> +                     print \" / $opts{'header_extra'}\";\n>>\n>> Hmmm...\n> \n> You don't sound very convinced. I had some doubts myself about whether\n> the slash should be inserted autmatically or whether it should be up\n> to the caller to include it in header_extra, but I'm not sure this is\n> what you are perplexed about.\n\nAh, I'm sorry, I have misunderstood the control flow here. I see now\nthat it is about adding subaction specifier after action, so that\n'remotes' view for single remote 'origin' (/remotes/origin path_info\nURL, see comments for patch introducing single-remote view) has\n\n _projects_ / _repo.git_ / _remotes_ / origin\n\nin the \"breadcrumbs\" navigation in page header.\n\nThis should be better described in the commit message.\n-- \nJakub Narebski\nPoland\n"},{"id":"151806","messageId":"201009270953.52562.jnareb@gmail.com","threadId":"25224","inReplyTo":"AANLkTimemfDPdMycasFmqUvk0eF-eD9z7P1RFnitLD9G@mail.gmail.com","subject":"Re: [PATCHv5 07/12] gitweb: remotes view for a single remote","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-27T07:53:52Z","receivedAt":"2010-09-27T07:53:52Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, 27 Sep 2010, Giuseppe Bilotta wrote:\n> 2010/9/26 Jakub Narebski :\n>> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:\n\n>>> +\n>>> +     git_header_html(undef, undef, 'header_extra' => $remote);\n>>\n>> I don't quite like the name of this parameter, and I am not sure\n>> if I like the API either.\n\nExplanation: what I don't like (a tiny bit) about API is the need for\nthose 'undef, undef'... but this might be unavoidable, at least without\nheavy hackery for little gain, because of the way Perl passes arguments\nto subroutines. (And no, rewrite of gitweb in Perl 6 is not a viable\nsolution ;-))\n\n> As I mentioned in my replies to the other respective patches, I think\n> it makes sense to make \"all remotes\" view easily accessible from the\n> \"single remote\" view, and there are two ways I can think of: one is\n> the \"extra header text\" way, by making the action name before it point\n> to \"all remotes\". The other is to enable 'remotes' in the page nav\n> submenu when we are in single remotes view (which is why I had the\n> $current in format_ref_views instead of $action, and which is what is\n> done by this change). IMO it makes sense to have both ways available,\n> but I'm open to suggestions about different approaches.\n\nNow I understand it, and I completly agree that it is a very good\nsolution. But it needs better description in the commit message.\n\nSidenote: http://git.oblomov.eu/rbot/remotes/anonj2 looks like doesn't\nhave this patch. In the navigation bar it has\n\n _projects_ / _rbot_ / remotes\n\nand not\n\n _projects_ / _rbot_ / _remotes_ / anonj2\n\n>>> - git_print_page_nav('','', $head,undef,$head,format_ref_views('remotes'));\n[...]\n>>> + git_print_page_nav('', '', $head, undef, $head,\n\nWhy not\n\n + git_print_page_nav('','', $head,undef,$head,\n\n>>> + format_ref_views($remote ? '' : 'remotes'));\n>>\n>> Why this change?\n\nI might be not clear, but I meant here the change in formatting of call\nto git_print_page_nav... and what I have not noticed is the fact that\nformat_ref_views is passed diferent argument.\n\nAnd I see now here why passing action-like parameter ($current) to\nformat_ref_views allow for this. I withdraw then proposal for using\n$action global variable in place of $current argument; this API is\nbetter (though perhaps this could be described in commit message)..\n \n-- \nJakub Narebski\nPoland\n"},{"id":"151808","messageId":"201009271012.23175.jnareb@gmail.com","threadId":"25224","inReplyTo":"AANLkTimOJ7RXDWXy=tF+rZf1gnfB7_GHCZuU5bZ5Wc91@mail.gmail.com","subject":"Re: [PATCHv5 08/12] gitweb: auxiliary function to group data","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-27T08:12:22Z","receivedAt":"2010-09-27T08:12:22Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Dnia poniedziałek 27. września 2010 09:26, Giuseppe Bilotta napisał:\n> 2010/9/26 Jakub Narebski :\n>> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:\n>>\n>>> Subject: gitweb: auxiliary function to group data\n>>>\n>>\n>> Errr... what!?  git_group() is not \"auxiliary function to group data\",\n>> but a template for output of group of data.\n> \n> I will rephrase\n\nThough I don't know how such rephrasing should look like.\n\n>> It would be probably good to describe how this output looks like\n>> (using e.g. ASCII-art mockup) in a commit message.\n> \n> Well, that would depend on the CSS that is used ... should I squash\n> the styling in this patch then?\n\nNo, I don't think it is needed, but it can be done.\n\n>>> +sub git_group {\n>>\n>> Name?\n> \n> git_collection? git_collect_data? I'm a little short on ideas.\n> git_section? git_subsection? the function (with different styling) can\n> probably be used even for the main sections in each view (think\n> summary view in particular).\n\ngit_group_html / git_subgroup_html / git_subsection_html\nprint_group / print_subgroup / print_section / print_subsection\n\nIt needs either *_html suffix (like git_header_html), or print_* prefix\nto denote that it prints HTML fragment, I think.\n\n>>> +     $content_func->() if defined $content_func;\n>>\n>> More defensive programming would be to use\n>>\n>>  +     $content_func->() if ref($content_func) eq 'CODE';\n>>\n>> Or even:\n>>\n>>  +     if (ref($content) eq 'CODE') {\n>>  +             $content->();\n>>  +     } elsif (ref($content) eq 'ARRAY') {\n>>  +             print @$content;\n\nThe 'ARRAY' part is probably unnecessary overengineering.\n\n>>  +     } elsif (!ref($content) && defined($content)) {\n>>  +             print $content;\n>>  +     }\n\nOr even (in the vein of further overengineering)\n\n + } elsif (ref($content) eq 'SCALAR') {\n + print esc_html($$content);\n + } elsif (!ref($content) && defined($content)) {\n + print $content;\n + }\n\nor vice versa ;-)\n\n>>\n>> Well, $content could be also open filehandle...\n\nThough I don't know how to check that. ref on filehandles return\n'GLOB'... well, we can use 'openhandle' from Scalar::Util (core).\nBut that is probably unnecessary overengineering.\n \n> Ah, very interesting and very flexible, I'll steal your idea.\n> \n> -- \n> Giuseppe \"Oblomov\" Bilotta\n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"151809","messageId":"201009271030.50258.jnareb@gmail.com","threadId":"25224","inReplyTo":"AANLkTi=nG9Day4ZCF88JwyeNx2qEM=+hKaKoyWRSTUFW@mail.gmail.com","subject":"Re: [PATCHv5 04/12] gitweb: nagivation menu for tags, heads and remotes","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-27T08:30:49Z","receivedAt":"2010-09-27T08:30:49Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, 27 Sep 2010, Giuseppe Bilotta wrote:\n> 2010/9/26 Jakub Narebski :\n>> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:\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>>> +\n>>\n>> Minor nitpick: this empty line here is not necessary.  But I think\n>> that Junio can remove it when applying.\n> \n> Since there have been a couple of stylistic suggestions for the\n> comments in the first two patches too, I can probably resend the whole\n> series including these changes, unless Junio wants to do the\n> hand-tuning.\n\nWell, the first half of this series (patches 1 to 5) are good enough,\nor almost good enough, to be merged in as they are now. They might\nneed at most some minor stylistic corrections. It depends on Junio\nwhether he wants to have resend of early part of this series, or if\nhe prefers to hand-edit those minor corrections.\n\nPatches 6-12 needs, I think, further discussion.\n \n>>> +sub format_ref_views {\n>>> +     my ($current) = @_;\n>>> +     my @ref_views = qw{tags heads};\n>>\n>> Hmmm... should we pass it as argument, or use $action in place of\n>> $current?  Each solution has its advantages and disadvantages.  Current\n>> solution has the advantage of avoiding using global variables, solution\n>> using $action has the (supposed) advantage of automatically detecting\n>> current action.\n> \n> Not using $action has the advantage of making it possible to enable\n> the $action command if it's wanted, which is something that I use in a\n> subsequent patch (when enabling single-remote view). But this is of\n> course debatable.\n\nI agree that use of format_ref_views further in this series shows that\nyour version (with $current passed as argument) is better.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"151840","messageId":"201009271747.39201.jnareb@gmail.com","threadId":"25224","inReplyTo":"1285344167-8518-13-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCHv5 12/12] gitweb: gather more remote data","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-27T15:47:36Z","receivedAt":"2010-09-27T15:47:36Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:\n\nIsn't the most important and visible information the fact that both\nstandalone 'remotes' view and 'remotes' section in the 'summary' page\nis now grouped by remotes? This should be explicitely mentioned in the\ncommit message. It is not very visible IMVHO in what is written below.\n\n> Collect remote information by gathering the list of remotes, and then\n> the URL(s) and heads in each remote. In summary view, limit the number\n> of remotes for which we collect data, as well as the maximum number of\n> heads per remote that we display.\n> \n> If the number of remotes is higher than the prescribed limit, do not\n> collect any heads information and just show the remotes names and the\n> links to the corresponding fetch and push URLs. Otherwise, create a\n> group for each remote and display all the information there.\n\nYou mean here that in 'summary' view, the 'remotes' section consist \nonly of list of remote names (is it truncated to 15 remotes) with \ncorresponding fetch and push URLs if number of remotes is higher than\nthe prescribed limit. In other case 'remotes' section consist of\nseparate subsection for each remote (is number of remotes limited \nalso in this case), and in each subsection there are displayed up to\nprescribed limit remote-tracking branches associated with given remote.\n\nIsn't it?\n\n> \n> Signed-off-by: Giuseppe Bilotta \n> ---\n> gitweb/gitweb.perl | 171 ++++++++++++++++++++++++++++++++++++++++++++++------\n> 1 files changed, 153 insertions(+), 18 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 93017a4..1e671ff 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -2758,6 +2758,57 @@ sub git_get_last_activity {\n> \treturn (undef, undef);\n> }\n> \n> +# Return an array with: a hash of remote names mapping to the corresponding\n> +# remote heads, the value of the $limit parameter, and a boolean indicating\n> +# whether we managed to get all the remotes or not.\n\nPerhaps it would be better to provide an example, rather than trying to\ndescribe the output.\n\n> +# If $limit is specified and the number of heads found is higher, the head\n> +# list is empy. Additional filtering on the number of heads can be done when\n> +# displaying the remotes.\n> +sub git_get_remotes_data {\n> +\tmy ($limit, $wanted) = @_;\n\nI'm not sure if it wouldn't be better to not worry git_get_remotes_data\nabout $limit, but do limiting on output. The amount of work gitweb\nneeds to do in both situation is, I guess, nearly the same.\n\n> +\tmy %remotes;\n> +\topen my $fd, '-|' , git_cmd(), 'remote', '-v';\n> +\treturn unless $fd;\n> +\tmy $more = 1;\n> +\twhile (my $remote = <$fd> and $more) {\n\nWhy not use 'last' instead of using $more variable (though I have not\nchecked if it would really make code simpler and more readable).\n\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 = $1;\n> +\t\tmy $key = $2;\n\nMinor nitpick: why not\n\n +\t\tmy ($url, $key) = ($1, $2);\n\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\t$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}\n> +\t}\n> +\tclose $fd or return;\n> +\n> +\t# if the while finished with $more being true, it means we ran\n> +\t# out of remotes before we hit $limit; paradoxically, it being true out\n> +\t# of the loop means there are 'no more' remotes.\n> +\t# Rather than waste time renaming the variable, we just read it to\n> +\t# answer the question: \"did we get all remotes before we hit\n> +\t# the limit?\"\n\nAh, I see that the fact that we exited early is important.\n\n> +\tif ($more) {\n> +\t\tmy @heads = map { \"remotes/$_\" } keys %remotes;\n> +\t\tmy @remoteheads = git_get_heads_list(undef, @heads);\n> +\t\tforeach (keys %remotes) {\n> +\t\t\tmy $remote = $_;\n> +\t\t\t$remotes{$remote}{'heads'} = [ grep {\n> +\t\t\t\t$_->{'name'} =~ s!^$remote/!!\n> +\t\t\t} @remoteheads ];\n> +\t\t}\n> +\t}\n> +\treturn (\\%remotes, $limit, $more);\n\nHmmmm... as it can be seen making this function do more work results\nin this ugly API. If git_get_remotes_data didn't worry about limiting,\nwe could return simply %remotes hash (or \\%remotes hash reference).\n\n> +}\n> +\n> sub git_get_references {\n> \tmy $type = shift || \"\";\n> \tmy %refs;\n> @@ -5018,6 +5069,93 @@ 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, $single) = @_;\n> +\tmy %rdata = %{$rdata};\n\nWhy do you need this, instead of simply using $rdata->{'heads'} etc.?\n\n> +\tmy $heads = $rdata{'heads'};\n\nWhy not\n\n +\tmy @heads = @{$rdata{'heads'}};\n\nOr\n\n +\tmy @heads = @{$rdata->{'heads'}};\n\nwithout this strange '%rdata = %{$rdata};'\n\n> +\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 .= git_repo_url(\"URL\", $fetch);\n> +\t\t} else {\n> +\t\t\t$urls .= git_repo_url(\"Fetch URL\", $fetch);\n> +\t\t\t$urls .= git_repo_url(\"Push URL\", $push) if defined $push;\n> +\t\t}\n> +\t} elsif (defined $push) {\n> +\t\t$urls .= git_repo_url(\"Push URL\", $push);\n> +\t} else {\n> +\t\t$urls .= git_repo_url(\"\", \"No remote URL\");\n> +\t}\n\nPerhaps reverse order of conditions would result in less nested \nconditional... but perhaps not.\n\n> +\n> +\t$urls .= \"
\\n\";\n> +\n> +\tmy ($maxheads, $dots);\n> +\tif (defined $limit) {\n> +\t\t$maxheads = $limit - 1;\n\nIf git_get_remotes_data didn't do limiting, you could use\n\n +\t\t$maxheads = scalar @heads;\n\n> +\t\tif ($#{$heads} > $maxheads) {\n> +\t\t\t$dots = $cgi->a({-href => href(action=>\"remotes\", hash=>$remote)}, \"...\");\n> +\t\t}\n> +\t}\n\nWe can always check if we got more remotes than limit.\n\n> +\n> +\tmy $content = sub {\n> +\t\tprint $urls;\n> +\t\tgit_heads_body($heads, $head, 0, $maxheads, $dots);\n> +\t};\n> +\n> +\tif (defined $single and $single) {\n\n'defined $single and $single' is the same as '$single', because\nundefined value is false-ish.\n\n> +\t\t$content->();\n> +\t} else {\n> +\t\tgit_group(\"remotes\", $remote, \"remotes\", $remote, $remote, $content);\n> +\t}\n\nHmmm... should git_remote_body be responsible for wrapping in subsection?\nWouldn't it be better to have caller use\n\n git_group(\"remotes\", $remote, \"remotes\", $remote, $remote, \n sub { git_remote_body($remote, $rdata, $limit, $head) });\n\nSidenote: strange (but perhaps unavoidable) repetition we have here...\n\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\nWouldn't it be better to put displaying only remote names in a separate\nsubroutine, which would make git_remotes_body extremely simple?\n\n> +sub git_remotes_body {\n> +\tmy ($remotelist, $limit, $have_all, $head) = @_;\n> +\tmy %remotes = %$remotelist;\n> +\tif ($have_all) {\n> +\t\twhile (my ($remote, $rdata) = each %remotes) {\n> +\t\t\tgit_remote_body($remote, $rdata, $limit, $head);\n> +\t\t}\n> +\t} else {\n> +\t\tprint \"\\n\";\n> +\t\tmy $alternate = 1;\n> +\t\twhile (my ($remote, $rdata) = each (%$remotelist)) {\n> +\t\t\tmy $fetch = $rdata->{'fetch'};\n> +\t\t\tmy $push = $rdata->{'push'};\n> +\t\t\tif ($alternate) {\n> +\t\t\t\tprint \"\\n\";\n> +\t\t\t} else {\n> +\t\t\t\tprint \"\\n\";\n> +\t\t\t}\n> +\t\t\t$alternate ^= 1;\n> +\t\t\tprint \"\";\n> +\t\t\tprint \"\";\n\nI see that you don't worry here if $fetch == $push, only if they\nare defined. But I think it might be all right... though the exact\noutput might need some discussion.\n\n> +\n> +\t\t\tprint \"\\n\";\n> +\t\t}\n> +\t\tprint \"\\n\" .\n> +\t\t \"\\n\" . \"\\n\";\n> +\t\tprint \"
\" .\n> +\t\t\t $cgi->a({-href=> href(action=>'remotes', hash=>$remote),\n> +\t\t\t -class=> \"list name\"},esc_html($remote)) . \"\" .\n> +\t\t\t (defined $fetch ? $cgi->a({-href=> $fetch}, \"fetch\") : \"fetch\") .\n> +\t\t\t \" | \" .\n> +\t\t\t (defined $push ? $cgi->a({-href=> $push}, \"push\") : \"push\") .\n> +\t\t\t \"
\" .\n> +\t\t $cgi->a({-href => href(action=>\"remotes\")}, \"...\") .\n> +\t\t \"
\";\n> +\t}\n> +}\n> +\n> sub git_search_grep_body {\n> \tmy ($commitlist, $from, $to, $extra) = @_;\n> \t$from = 0 unless defined $from;\n> @@ -5164,7 +5302,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 @remotelist = $remote_heads ? git_get_remotes_data(16) : ();\n\nThat's not @remotelist any longer, isn't it?\n\n> \tmy @forklist;\n> \tmy $check_forks = gitweb_check_feature('forks');\n> \n> @@ -5244,9 +5382,7 @@ sub git_summary {\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\tgit_remotes_body(@remotelist, $head);\n\nHmmm... (current API, with @remotelist (!) containing list of arguments\nto a subroutine).\n\n> \t}\n> \n> \tif (@forklist) {\n> @@ -5570,26 +5706,25 @@ sub git_remotes {\n> \tmy $head = git_get_head_hash($project);\n> \tmy $remote = $input_params{'hash'};\n> \n> +\tmy @remotelist = git_get_remotes_data(undef, $remote);\n> +\tdie_error(500, \"Unable to get remote information\") unless @remotelist;\n\n@remotelist is not list of remotes.\n\nWhat if there are no remotes, and no remote-tracking branches?\n\n> +\n> +\tif (keys(%{$remotelist[0]}) == 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\nAh, I see that it is handled here. Sidenote: with proposed changed\nAPI we can distinguish error case from no-remotes case by returning\nundefined value versus returning empty hash / empty hash reference.\n\n> +\n> \tgit_header_html(undef, undef, 'header_extra' => $remote);\n> \tgit_print_page_nav('', '', $head, undef, $head,\n> \t\tformat_ref_views($remote ? '' : 'remotes'));\n> -\tgit_print_header_div('summary', $project);\n> \n> \tif (defined $remote) {\n> -\t\t# only display the heads in a given remote\n> -\t\tmy @headslist = 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\tif (@headslist) {\n> -\t\t\tgit_heads_body(\\@headslist, $head);\n> -\t\t}\n> +\t\tgit_print_header_div('remotes', \"$remote remote for $project\");\n> +\t\tgit_remote_body($remote, $remotelist[0]->{$remote}, undef, $head, 1);\n> \t} else {\n> -\t\tmy @remotelist = git_get_heads_list(undef, 'remotes');\n> -\t\tif (@remotelist) {\n> -\t\t\tgit_heads_body(\\@remotelist, $head);\n> -\t\t}\n> +\t\tgit_print_header_div('summary', \"$project remotes\");\n> +\t\tgit_remotes_body(@remotelist, $head);\n> \t}\n> \tgit_footer_html();\n> }\n> -- \n> 1.7.3.68.g6ec8\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"151853","messageId":"AANLkTinudc8zX33o=vxxo=nf0L9KxFiJR8UYNomMfajN@mail.gmail.com","threadId":"25224","inReplyTo":"201009271012.23175.jnareb@gmail.com","subject":"Re: [PATCHv5 08/12] gitweb: auxiliary function to group data","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-27T19:17:49Z","receivedAt":"2010-09-27T19:17:49Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2010/9/27 Jakub Narebski :\n>>>  +     if (ref($content) eq 'CODE') {\n>>>  +             $content->();\n>>>  +     } elsif (ref($content) eq 'ARRAY') {\n>>>  +             print @$content;\n>\n> The 'ARRAY' part is probably unnecessary overengineering.\n>\n>>>  +     } elsif (!ref($content) && defined($content)) {\n>>>  +             print $content;\n>>>  +     }\n>\n> Or even (in the vein of further overengineering)\n>\n>    +     } elsif (ref($content) eq 'SCALAR') {\n>    +             print esc_html($$content);\n>    +     } elsif (!ref($content) && defined($content)) {\n>    +             print $content;\n>    +     }\n>\n> or vice versa ;-)\n>\n>>>\n>>> Well, $content could be also open filehandle...\n>\n> Though I don't know how to check that.  ref on filehandles return\n> 'GLOB'... well, we can use 'openhandle' from Scalar::Util (core).\n> But that is probably unnecessary overengineering.\n\nI have made cases for closures, scalar refs and scalar values. I've\nalso added a case for GLOB or IO::Handle to allow a file handle to be\npassed as either *handle or *handle{IO}. I've also added a comment\nblock explaining the syntax better.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"154246","messageId":"AANLkTi=7gUAbdRKLCifPidZXdLa8fCENT0=iy5bKAeiA@mail.gmail.com","threadId":"25224","inReplyTo":"201009271747.39201.jnareb@gmail.com","subject":"Re: [PATCHv5 12/12] gitweb: gather more remote data","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-10-23T16:17:51Z","receivedAt":"2010-10-23T16:17:51Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"I'm awfully sorry for the delay in the reply to this email. I got\ngobbled up by some real-world stuff just while finishing the review of\nthe last patch.\n\n2010/9/27 Jakub Narebski :\n> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:\n>\n> Isn't the most important and visible information the fact that both\n> standalone 'remotes' view and 'remotes' section in the 'summary' page\n> is now grouped by remotes?  This should be explicitely mentioned in the\n> commit message.  It is not very visible IMVHO in what is written below.\n\nYou're right. I've rewritten the message by putting the grouping\nlayout in the subject and rephrasing the body of the commit to better\ndescribe what happens both in remotes view and in limited summary\nview.\n\n>>\n>> +# Return an array with: a hash of remote names mapping to the corresponding\n>> +# remote heads, the value of the $limit parameter, and a boolean indicating\n>> +# whether we managed to get all the remotes or not.\n>\n> Perhaps it would be better to provide an example, rather than trying to\n> describe the output.\n\nI'm really not sure about how it would help, but I'll try.\n\n>> +# If $limit is specified and the number of heads found is higher, the head\n>> +# list is empy. Additional filtering on the number of heads can be done when\n>> +# displaying the remotes.\n>> +sub git_get_remotes_data {\n>> +     my ($limit, $wanted) = @_;\n>\n> I'm not sure if it wouldn't be better to not worry git_get_remotes_data\n> about $limit, but do limiting on output.  The amount of work gitweb\n> needs to do in both situation is, I guess, nearly the same.\n\nI think that the real issue is that situations in which gathering all\ndata and then discarding much of it would be very rare, being that it\nwould require a very large number of remotes (not remote heads, just\nremotes). But considering that even I, a rather casual user, have at\nleast one project where the number of remotes is in the 20s, I\nwouldn't be surprised if there were cases of humongous remote lists.\n\n>> +     my %remotes;\n>> +     open my $fd, '-|' , git_cmd(), 'remote', '-v';\n>> +     return unless $fd;\n>> +     my $more = 1;\n>> +     while (my $remote = <$fd> and $more) {\n>\n> Why not use 'last' instead of using $more variable (though I have not\n> checked if it would really make code simpler and more readable).\n\nBecause we need to know whether we bail out early or not from the\nloop. It might be appropriate to document this in the code.\n\n>> +             chomp $remote;\n>> +             $remote =~ s!\\t(.*?)\\s+\\((\\w+)\\)$!!;\n>> +             next if $wanted and not $remote eq $wanted;\n>> +             my $url = $1;\n>> +             my $key = $2;\n>\n> Minor nitpick: why not\n>\n>  +             my ($url, $key) = ($1, $2);\n\nNo reason. Can do.\n\n>> +     if ($more) {\n>> +             my @heads = map { \"remotes/$_\" } keys %remotes;\n>> +             my @remoteheads = git_get_heads_list(undef, @heads);\n>> +             foreach (keys %remotes) {\n>> +                     my $remote = $_;\n>> +                     $remotes{$remote}{'heads'} = [ grep {\n>> +                             $_->{'name'} =~ s!^$remote/!!\n>> +                     } @remoteheads ];\n>> +             }\n>> +     }\n>> +     return (\\%remotes, $limit, $more);\n>\n> Hmmmm... as it can be seen making this function do more work results\n> in this ugly API.  If git_get_remotes_data didn't worry about limiting,\n> we could return simply %remotes hash (or \\%remotes hash reference).\n\nIf I read your comments correctly, your idea would be to have two\nseparate functions, one that gets the list of remotes (with their\nrespective URLs), and a separate one (called when intended) to also\ngather the list of heads. The only case in which the latter would not\nbe called would be when in summary view more than the given number of\nmaximum remotes were returned from the previous function. Correct?\n\n>> +# Display a single remote block\n>> +sub git_remote_body {\n>> +     my ($remote, $rdata, $limit, $head, $single) = @_;\n>> +     my %rdata = %{$rdata};\n>\n> Why do you need this, instead of simply using $rdata->{'heads'} etc.?\n\nBecause I keep getting messed up with the refs syntax in Perl. Thanks\nfor the hint.\n\n>> +     my $heads = $rdata{'heads'};\n>\n> Why not\n>\n>  +     my @heads = @{$rdata{'heads'}};\n>\n> Or\n>\n>  +     my @heads = @{$rdata->{'heads'}};\n>\n> without this strange '%rdata = %{$rdata};'\n\nWell, the main use of heads was to be passed to git_heads_body so I\nthought I would just leave it as a ref.\n\n>> +     if (defined $fetch) {\n>> +             if ($fetch eq $push) {\n>> +                     $urls .= git_repo_url(\"URL\", $fetch);\n>> +             } else {\n>> +                     $urls .= git_repo_url(\"Fetch URL\", $fetch);\n>> +                     $urls .= git_repo_url(\"Push URL\", $push) if defined $push;\n>> +             }\n>> +     } elsif (defined $push) {\n>> +             $urls .= git_repo_url(\"Push URL\", $push);\n>> +     } else {\n>> +             $urls .= git_repo_url(\"\", \"No remote URL\");\n>> +     }\n>\n> Perhaps reverse order of conditions would result in less nested\n> conditional... but perhaps not.\n\nI tried. The problem is that we just have that many cases (fetch and\nno push, push and no fetch, equal fetch and push, different fetch and\npush). I could not think of a way to handle them all in a more\nstreamlined way.\n\n>> +\n>> +     $urls .= \"\\n\";\n>> +\n>> +     my ($maxheads, $dots);\n>> +     if (defined $limit) {\n>> +             $maxheads = $limit - 1;\n>\n> If git_get_remotes_data didn't do limiting, you could use\n>\n>  +             $maxheads = scalar @heads;\n>\n>> +             if ($#{$heads} > $maxheads) {\n>> +                     $dots = $cgi->a({-href => href(action=>\"remotes\", hash=>$remote)}, \"...\");\n>> +             }\n>> +     }\n>\n> We can always check if we got more remotes than limit.\n>\n\nActually, this is in reverse. get_remotes_data doesn't do limiting on\nthe number of heads, which is why $maxheads is $limit - 1 and the\nnumber of heads is compared against it. git_remotes_data does limiting\non the number of _remotes_, and if the limit is hit, git_remote_body\nwould just not be called.\n\n>> +\n>> +     my $content = sub {\n>> +             print $urls;\n>> +             git_heads_body($heads, $head, 0, $maxheads, $dots);\n>> +     };\n>> +\n>> +     if (defined $single and $single) {\n>\n> 'defined $single and $single' is the same as '$single', because\n> undefined value is false-ish.\n\nI was over-protecting against warnings about the use of undefined\nsymbols. Cleaned up.\n\n>> +             $content->();\n>> +     } else {\n>> +             git_group(\"remotes\", $remote, \"remotes\", $remote, $remote, $content);\n>> +     }\n>\n> Hmmm... should git_remote_body be responsible for wrapping in subsection?\n> Wouldn't it be better to have caller use\n>\n>  git_group(\"remotes\", $remote, \"remotes\", $remote, $remote,\n>            sub { git_remote_body($remote, $rdata, $limit, $head) });\n\nRight.\n\n> Sidenote: strange (but perhaps unavoidable) repetition we have here...\n\nEven with the new syntax discussed for the previous patches this is\nunavoidable, it seems.\n\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> Wouldn't it be better to put displaying only remote names in a separate\n> subroutine, which would make git_remotes_body extremely simple?\n\nSure.\n\n>> +                     print \"\" .\n>> +                           $cgi->a({-href=> href(action=>'remotes', hash=>$remote),\n>> +                                    -class=> \"list name\"},esc_html($remote)) . \"\";\n>> +                     print \"\" .\n>> +                           (defined $fetch ? $cgi->a({-href=> $fetch}, \"fetch\") : \"fetch\") .\n>> +                           \" | \" .\n>> +                           (defined $push ? $cgi->a({-href=> $push}, \"push\") : \"push\") .\n>> +                           \"\";\n>\n> I see that you don't worry here if $fetch == $push, only if they\n> are defined.  But I think it might be all right... though the exact\n> output might need some discussion.\n\nI'm open to suggestions.\n\n>>  sub git_search_grep_body {\n>>       my ($commitlist, $from, $to, $extra) = @_;\n>>       $from = 0 unless defined $from;\n>> @@ -5164,7 +5302,7 @@ sub git_summary {\n>>       # there are more ...\n>>       my @taglist  = git_get_tags_list(16);\n>>       my @headlist = git_get_heads_list(16, 'heads');\n>> -     my @remotelist = $remote_heads ? git_get_heads_list(16, 'remotes') : ();\n>> +     my @remotelist = $remote_heads ? git_get_remotes_data(16) : ();\n>\n> That's not @remotelist any longer, isn't it?\n\nRight. I renamed it to @remotedata across the whole file.\n\n>>       my @forklist;\n>>       my $check_forks = gitweb_check_feature('forks');\n>>\n>> @@ -5244,9 +5382,7 @@ sub git_summary {\n>>\n>>       if (@remotelist) {\n>>               git_print_header_div('remotes');\n>> -             git_heads_body(\\@remotelist, $head, 0, 15,\n>> -                            $#remotelist <= 15 ? undef :\n>> -                            $cgi->a({-href => href(action=>\"remotes\")}, \"...\"));\n>> +             git_remotes_body(@remotelist, $head);\n>\n> Hmmm... (current API, with @remotelist (!) containing list of arguments\n> to a subroutine).\n\nIt does make the call much cleaner though (think remotedata: it has\nthe data, and the information on whether the data is complete or not.\ndoesn't it make sense?)\n\n>>       }\n>>\n>>       if (@forklist) {\n>> @@ -5570,26 +5706,25 @@ sub git_remotes {\n>>       my $head = git_get_head_hash($project);\n>>       my $remote = $input_params{'hash'};\n>>\n>> +     my @remotelist = git_get_remotes_data(undef, $remote);\n>> +     die_error(500, \"Unable to get remote information\") unless @remotelist;\n>\n> What if there are no remotes, and no remote-tracking branches?\n\nPresently, we get 404 - No remotes found if there are no remotes. If\nthere are remotes without heads, the section will only contain the\nURL.\n\nThe funny thing is that if there are no remotes there will be an empty\nremotes section in summary view, so we probably to check for that too.\n\n>> +\n>> +     if (keys(%{$remotelist[0]}) == 0) {\n>> +             die_error(404, defined $remote ?\n>> +                     \"Remote $remote not found\" :\n>> +                     \"No remotes found\");\n>> +     }\n>\n> Ah, I see that it is handled here.  Sidenote: with proposed changed\n> API we can distinguish error case from no-remotes case by returning\n> undefined value versus returning empty hash / empty hash reference.\n\nThe question is: do we really want the different API? It does make a\nfew things cleaner, but it makes other things messier (I'm thinking\nhere about the calls to git_*_body; compare what we have for remotes\nwith what we have for tags or heads list in summary view ...)\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"}]}