{"thread":{"id":"25122","subject":"[PATCH 0/7] gitweb: allheads feature","startedAt":"2010-09-16T09:30:58Z","lastAt":"2010-09-22T09:34:33Z","messageCount":42,"participants":["Giuseppe Bilotta","Ævar Arnfjörð Bjarmason","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"150797","messageId":"1284629465-14798-1-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25122","inReplyTo":null,"subject":"[PATCH 0/7] gitweb: allheads feature","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-16T09:30:58Z","receivedAt":"2010-09-16T09:30:58Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"This is a rehash of an old patchset of mine that got stalled waiting for\nother independent patches to go in first, and then for me to get the\ntime to work on it again.\n\nThe first 4 patches are IMO ready for inclusing in gitweb, and their\npurpose is to introduce a new view (and a new summary block) that\ndisplay all the remote heads (assuming the feature is enabled).\nSomebody suggested via email that this could even the basis for some\nkind of 'social graph' for gitweb repositories, in a way similar to what\nis found on sites like github or gitorious, but for me the feature in\nitself can already be useful.\n\nThe last three patches are more of the RFC side, in particular the last\none. The idea is to group remote heads 'by remote' instead of just\nlisting them serially. So I first introduce code and styling to have\n'blocks of stuff' in gitweb, and then use this concept to group together\nremote heads belonging to the same remote.\n\nThe final result is rather curious and you can see it in action at\n, although it would be nice to find\na way to layout the blocks in a smarter way. What I really don't like\n(at the moment) is the way things come out in summary view instead.\n\nThe issue there is that we only gather 16 remote heads, so some remotes\nmight have no branches displayed, but it becomes difficult to detect and\nindicate when remotes have incomplete information being displayed. A\npossible solution would be to call show-ref N times (N being the number\nof remotes) with a limit of 16/N heads, but that can be a lot of calls.\nSo I'm open to suggestions on how to improve this part (maybe just show\na flat view in the remotes section of summary view?)\n\nGiuseppe Bilotta (7):\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: link heads and remotes view\n gitweb: auxiliary functions to group data\n gitweb: group styling\n gitweb: group remote heads\n\n gitweb/gitweb.perl | 100 ++++++++++++++++++++++++++++++++++++++++++---\n gitweb/static/gitweb.css | 6 +++\n 2 files changed, 99 insertions(+), 7 deletions(-)\n\n-- \n1.7.3.rc1.230.g8b572\n"},{"id":"150799","messageId":"1284629465-14798-2-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25122","inReplyTo":"1284629465-14798-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCH 1/7] gitweb: introduce remote_heads feature","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-16T09:30:59Z","receivedAt":"2010-09-16T09:30:59Z","isPatch":true,"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 | 19 +++++++++++++++++--\n 1 files changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex a85e2f6..7116c26 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,8 +3174,9 @@ 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{'class'} = $1;\n \t\t$ref_item{'name'} = $name;\n \t\t$ref_item{'id'} = $hash;\n \t\t$ref_item{'title'} = $title || '(no commit message)';\n-- \n1.7.3.rc1.230.g8b572\n"},{"id":"150800","messageId":"1284629465-14798-3-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25122","inReplyTo":"1284629465-14798-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCH 2/7] gitweb: git_get_heads_list accepts an optional list of refs.","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-16T09:31:00Z","receivedAt":"2010-09-16T09:31:00Z","isPatch":true,"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 7116c26..21e83bb 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, @class) = @_;\n+\tunless (defined @class) {\n+\t\tmy $remote_heads = gitweb_check_feature('remote_heads');\n+\t\t@class = ('heads', $remote_heads ? 'remotes' : undef);\n+\t}\n+\tmy @refs = map { \"refs/$_\" } @class;\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@refs\n \t\tor return;\n \twhile (my $line = <$fd>) {\n \t\tmy %ref_item;\n-- \n1.7.3.rc1.230.g8b572\n"},{"id":"150798","messageId":"1284629465-14798-4-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25122","inReplyTo":"1284629465-14798-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCH 3/7] gitweb: separate heads and remotes lists","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-16T09:31:01Z","receivedAt":"2010-09-16T09:31:01Z","isPatch":true,"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 21e83bb..0118739 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@@ -5112,6 +5113,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@@ -5119,7 +5121,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@@ -5197,6 +5200,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@@ -5504,13 +5514,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.rc1.230.g8b572\n"},{"id":"150801","messageId":"1284629465-14798-5-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25122","inReplyTo":"1284629465-14798-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCH 4/7] gitweb: link heads and remotes view","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-16T09:31:02Z","receivedAt":"2010-09-16T09:31:02Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Add a link in heads view to remotes view (if the feature is\nenabled), and conversely from remotes to heads.\n\nSigned-off-by: Giuseppe Bilotta \n---\n gitweb/gitweb.perl | 10 ++++++++--\n 1 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 0118739..6138c6e 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5511,7 +5511,10 @@ 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+\tmy $heads_nav = gitweb_check_feature('remote_heads') ?\n+\t\t$cgi->a({-href => href(action=>\"remotes\", -replay=>1)},\n+\t\t \"remotes\") : undef;\n+\tgit_print_page_nav('','', $head,undef,$head, $heads_nav);\n \tgit_print_header_div('summary', $project);\n \n \tmy @headslist = git_get_heads_list(undef, 'heads');\n@@ -5527,7 +5530,10 @@ 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+\tmy $heads_nav =\n+\t\t$cgi->a({-href => href(action=>\"heads\", -replay=>1)},\n+\t\t \"heads\");\n+\tgit_print_page_nav('','', $head,undef,$head, $heads_nav);\n \tgit_print_header_div('summary', $project);\n \n \tmy @remotelist = git_get_heads_list(undef, 'remotes');\n-- \n1.7.3.rc1.230.g8b572\n"},{"id":"150804","messageId":"1284629465-14798-6-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25122","inReplyTo":"1284629465-14798-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCH 5/7] gitweb: auxiliary functions to group data","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-16T09:31:03Z","receivedAt":"2010-09-16T09:31:03Z","isPatch":true,"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 | 15 +++++++++++++++\n 1 files changed, 15 insertions(+), 0 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 6138c6e..92551e4 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3765,6 +3765,21 @@ sub git_print_header_div {\n \t \"\\n\\n\";\n }\n \n+sub git_begin_group {\n+\tmy ($class, $id, @rest) = @_;\n+\n+\t$class = ' class=\"' . join(' ', 'group', $class) . '\"';\n+\n+\t$id = ' id=\"' . $id . '\"' if $id;\n+\n+\tprint \"\\n\";\n+\tgit_print_header_div(@rest);\n+}\n+\n+sub git_end_group {\n+\tprint \"\\n\"\n+}\n+\n sub print_local_time {\n \tprint format_local_time(@_);\n }\n-- \n1.7.3.rc1.230.g8b572\n"},{"id":"150802","messageId":"1284629465-14798-7-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25122","inReplyTo":"1284629465-14798-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCH 6/7] gitweb: group styling","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-16T09:31:04Z","receivedAt":"2010-09-16T09:31:04Z","isPatch":true,"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.rc1.230.g8b572\n"},{"id":"150803","messageId":"1284629465-14798-8-git-send-email-giuseppe.bilotta@gmail.com","threadId":"25122","inReplyTo":"1284629465-14798-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"[PATCH 7/7] gitweb: group remote heads","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-16T09:31:05Z","receivedAt":"2010-09-16T09:31:05Z","isPatch":true,"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 | 35 ++++++++++++++++++++++++++++-------\n 1 files changed, 28 insertions(+), 7 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 92551e4..66b5400 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -2758,6 +2758,16 @@ sub git_get_last_activity {\n \treturn (undef, undef);\n }\n \n+sub git_get_remotes {\n+\tmy ($limit) = @_;\n+\topen my $fd, '-|' , git_cmd(), 'remote';\n+\treturn () unless $fd;\n+\tmy @remotes = map { chomp ; $_ } <$fd>;\n+\tclose $fd or return ();\n+\tmy @remoteheads = git_get_heads_list($limit, 'remotes');\n+\treturn (\\@remotes, \\@remoteheads);\n+}\n+\n sub git_get_references {\n \tmy $type = shift || \"\";\n \tmy %refs;\n@@ -4979,7 +4989,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@@ -4991,6 +5001,19 @@ sub git_heads_body {\n \tprint \"\\n\";\n }\n \n+sub git_remotes_body {\n+\tmy ($remotedata, $head) = @_;\n+\tmy @remotenames = @{$remotedata->[0]};\n+\tmy @allheads = @{$remotedata->[1]};\n+\tforeach my $remote (@remotenames) {\n+\t\tmy @remoteheads = grep { $_->{'name'} =~ s!^\\Q$remote\\E/!! } @allheads;\n+\t\tgit_begin_group(\"remotes\", $remote, \"remotes/$remote\",$remote);\n+\t\tgit_heads_body(\\@remoteheads, $head);\n+\t\tgit_end_group();\n+\t}\n+\n+}\n+\n sub git_search_grep_body {\n \tmy ($commitlist, $from, $to, $extra) = @_;\n \t$from = 0 unless defined $from;\n@@ -5137,7 +5160,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(16) : ();\n \tmy @forklist;\n \tmy $check_forks = gitweb_check_feature('forks');\n \n@@ -5217,9 +5240,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@@ -5551,9 +5572,9 @@ sub git_remotes {\n \tgit_print_page_nav('','', $head,undef,$head, $heads_nav);\n \tgit_print_header_div('summary', $project);\n \n-\tmy @remotelist = git_get_heads_list(undef, 'remotes');\n+\tmy @remotelist = git_get_remotes();\n \tif (@remotelist) {\n-\t\tgit_heads_body(\\@remotelist, $head);\n+\t\tgit_remotes_body(\\@remotelist, $head);\n \t}\n \tgit_footer_html();\n }\n-- \n1.7.3.rc1.230.g8b572\n"},{"id":"150805","messageId":"AANLkTi=N=jyj8ueVNu601N9qtBktmC-kVv9+0jSA=Sv-@mail.gmail.com","threadId":"25122","inReplyTo":"1284629465-14798-4-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCH 3/7] gitweb: separate heads and remotes lists","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-16T10:19:00Z","receivedAt":"2010-09-16T10:19:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Thu, Sep 16, 2010 at 09:31, Giuseppe Bilotta\n wrote:\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> +       }\n\nNit: The $# syntax is pseudo-deprecated, and since you use 16 as a\nconstant above this would be clearer anyway:\n\n @remotelist <= 16 ? undef : ...\n"},{"id":"150806","messageId":"AANLkTinU3dQnricwXsf_-b1skWMCaNGxSLYLpOJZ94cm@mail.gmail.com","threadId":"25122","inReplyTo":"1284629465-14798-6-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCH 5/7] gitweb: auxiliary functions to group data","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-16T10:26:46Z","receivedAt":"2010-09-16T10:26:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Thu, Sep 16, 2010 at 09:31, Giuseppe Bilotta\n wrote:\n> Signed-off-by: Giuseppe Bilotta \n> ---\n>  gitweb/gitweb.perl |   15 +++++++++++++++\n>  1 files changed, 15 insertions(+), 0 deletions(-)\n>\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 6138c6e..92551e4 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -3765,6 +3765,21 @@ sub git_print_header_div {\n>              \"\\n\\n\";\n>  }\n>\n> +sub git_begin_group {\n> +       my ($class, $id, @rest) = @_;\n> +\n> +       $class = ' class=\"' . join(' ', 'group', $class) . '\"';\n> +\n> +       $id = ' id=\"' . $id . '\"' if $id;\n> +\n> +       print \"\\n\";\n\nHrm, this would be better:\n\n $cgi->div( { class => \"group $class\", $id ? (id => $id) : () } )\n\n> +       git_print_header_div(@rest);\n> +}\n> +\n> +sub git_end_group {\n> +       print \"\\n\"\n> +}\n> +\n\nBut obviously that won't fit if the closing element is being printed\nseparately.\n"},{"id":"150807","messageId":"AANLkTimFjofJo4ByFwr2Cz6-YqkHcZsHGk6r4siEFmDa@mail.gmail.com","threadId":"25122","inReplyTo":"1284629465-14798-8-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCH 7/7] gitweb: group remote heads","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-16T10:29:00Z","receivedAt":"2010-09-16T10:29:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Thu, Sep 16, 2010 at 09:31, Giuseppe Bilotta\n wrote:\n\n> +sub git_get_remotes {\n> +       my ($limit) = @_;\n> +       open my $fd, '-|' , git_cmd(), 'remote';\n> +       return () unless $fd;\n> +       my @remotes = map { chomp ; $_ } <$fd>;\n> +       close $fd or return ();\n\n return unless $fd;\n chomp(my @remotes = <$fd>);\n close $fd or return;\n"},{"id":"150815","messageId":"AANLkTimXqpx4_qn7tPB77-mmvkypbmd9uLWpnaRwRsJQ@mail.gmail.com","threadId":"25122","inReplyTo":"AANLkTi=N=jyj8ueVNu601N9qtBktmC-kVv9+0jSA=Sv-@mail.gmail.com","subject":"Re: [PATCH 3/7] gitweb: separate heads and remotes lists","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-16T11:35:33Z","receivedAt":"2010-09-16T11:35:33Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Thu, Sep 16, 2010 at 12:19 PM, Ævar Arnfjörð Bjarmason\n wrote:\n> On Thu, Sep 16, 2010 at 09:31, Giuseppe Bilotta\n> wrote:\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>> +       }\n>\n> Nit: The $# syntax is pseudo-deprecated, and since you use 16 as a\n> constant above this would be clearer anyway:\n>\n>    @remotelist <= 16 ? undef : ...\n\nI think following the gitweb coding style is better. OTOH, I can\nprobably prepare a (separate) patch to replace $# with @ where\npossible.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"150816","messageId":"AANLkTimKV6iT-inphJbCiRa2Pb3OeP-usPpCHoaRuL8O@mail.gmail.com","threadId":"25122","inReplyTo":"AANLkTimFjofJo4ByFwr2Cz6-YqkHcZsHGk6r4siEFmDa@mail.gmail.com","subject":"Re: [PATCH 7/7] gitweb: group remote heads","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-16T11:36:14Z","receivedAt":"2010-09-16T11:36:14Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Thu, Sep 16, 2010 at 12:29 PM, Ævar Arnfjörð Bjarmason\n wrote:\n> On Thu, Sep 16, 2010 at 09:31, Giuseppe Bilotta\n> wrote:\n>\n>> +sub git_get_remotes {\n>> +       my ($limit) = @_;\n>> +       open my $fd, '-|' , git_cmd(), 'remote';\n>> +       return () unless $fd;\n>> +       my @remotes = map { chomp ; $_ } <$fd>;\n>> +       close $fd or return ();\n>\n>    return unless $fd;\n>    chomp(my @remotes = <$fd>);\n>    close $fd or return;\n\nThanks a lot. I guess this shows pretty much how Perl is not exactly\nmy primary language 8-)\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"150849","messageId":"201009162326.01656.jnareb@gmail.com","threadId":"25122","inReplyTo":"1284629465-14798-1-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCH 0/7] gitweb: allheads feature","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-16T21:26:00Z","receivedAt":"2010-09-16T21:26:00Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:\n\n> This is a rehash of an old patchset of mine that got stalled waiting for\n> other independent patches to go in first, and then for me to get the\n> time to work on it again.\n> \n> The first 4 patches are IMO ready for inclusing in gitweb, and their\n> purpose is to introduce a new view (and a new summary block) that\n> display all the remote heads (assuming the feature is enabled).\n> Somebody suggested via email that this could even the basis for some\n> kind of 'social graph' for gitweb repositories, in a way similar to what\n> is found on sites like github or gitorious, but for me the feature in\n> itself can already be useful.\n\nWe might want to make git-instaweb enable this feature (and probably\nother disabled-by-default features). Otherwise some of information\nabout git repository you examine is hidden. So I agree that this \nfeature is useful by itself.\n \n> The last three patches are more of the RFC side, in particular the last\n> one. The idea is to group remote heads 'by remote' instead of just\n> listing them serially. So I first introduce code and styling to have\n> 'blocks of stuff' in gitweb, and then use this concept to group together\n> remote heads belonging to the same remote.\n\nThis is a good idea in itself; I'd take a look at implementation and\nstyling when examining individual patches.\n\n> The final result is rather curious and you can see it in action at\n> , although it would be nice to find\n> a way to layout the blocks in a smarter way. \n\nThanks for providing demo site.\n\nNote that clicking on header for remote block, which should lead to\ndisplaying of only single remote displays all remotes, see e.g.\nhttp://git.oblomov.eu/rbot/remotes/a3li. Moreover when I tried to\nhandcraft URL i.e. http://git.oblomov.eu/rbot/remote/a3li (with\n'remote' rather than 'remotes' action) I get an empty list of heads.\n\n\nAbout layout of 'remotes' view: to have remotes information aligned\nwe would have to either put everything in one single table (with remotes\nheaders being \"colspan\"), or style them with minimum width (which could\nbe good idea anyway).\n \n> What I really don't like (at the moment) is the way things come out\n> in summary view instead. \n\nWhat you don't like about it? Should it be smarter and display only\nlist of remotes, perhaps even limited to 15 elements, if there are many\nremote-tracking branches? Or is it something else?\n\n> The issue there is that we only gather 16 remote heads, so some remotes\n> might have no branches displayed, but it becomes difficult to detect and\n> indicate when remotes have incomplete information being displayed. A\n> possible solution would be to call show-ref N times (N being the number\n> of remotes) with a limit of 16/N heads, but that can be a lot of calls.\n> So I'm open to suggestions on how to improve this part (maybe just show\n> a flat view in the remotes section of summary view?)\n\nAh, I see...\n\nAlternatively we could use smart limiting on the gitweb side.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"150850","messageId":"201009162341.20380.jnareb@gmail.com","threadId":"25122","inReplyTo":"1284629465-14798-2-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCH 1/7] gitweb: introduce remote_heads feature","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-16T21:41:19Z","receivedAt":"2010-09-16T21:41:19Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 16 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\nGood idea.\n\n> \n> Signed-off-by: Giuseppe Bilotta \n> ---\n> gitweb/gitweb.perl | 19 +++++++++++++++++--\n> 1 files changed, 17 insertions(+), 2 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index a85e2f6..7116c26 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\nI agree both with this feature being turned off by default, and with\nit being per-project overridable.\n\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\nThe usual way for optionally providing extra arguments to git commands\nin gitweb is to use empty list \"()\" and not empty argument \"''\", i.e.\nit would be:\n\n +\t\t'refs/heads', ( $remote_heads ? 'refs/remotes' : ())\n\nSee for example git_get_references, parse_commits,... and evem the line\nwith \"($limit ? ...)\" above in git_get_heads_list.\n\n> \t\tor return;\n> \twhile (my $line = <$fd>) {\n> \t\tmy %ref_item;\n> @@ -3160,8 +3174,9 @@ 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{'class'} = $1;\n\nIs it used anywhere, or is it left to be used by a further commit in\nthe series? If it is the latter, perhaps it would be worth mentioning\nin the commit message?\n\n> \t\t$ref_item{'name'} = $name;\n> \t\t$ref_item{'id'} = $hash;\n> \t\t$ref_item{'title'} = $title || '(no commit message)';\n> -- \n> 1.7.3.rc1.230.g8b572\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"150851","messageId":"201009170014.12483.jnareb@gmail.com","threadId":"25122","inReplyTo":"1284629465-14798-3-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCH 2/7] gitweb: git_get_heads_list accepts an optional list of refs.","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-16T22:14:12Z","receivedAt":"2010-09-16T22:14:12Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 16 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\nI like this API very much.\n \n> Signed-off-by: Giuseppe Bilotta \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 7116c26..21e83bb 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, @class) = @_;\n\nNitpick: @class or @classes.\n\n> +\tunless (defined @class) {\n> +\t\tmy $remote_heads = gitweb_check_feature('remote_heads');\n> +\t\t@class = ('heads', $remote_heads ? 'remotes' : undef);\n\nSame comment as for previous patch:\n\n +\t\t@class = ('heads', $remote_heads ? 'remotes' : ());\n\nOr alternative solution:\n\n +\t\t@class = 'heads';\n +\t\tpush @class, 'remotes' if gitweb_check_feature('remote_heads');\n\n\n> +\t}\n> +\tmy @refs = map { \"refs/$_\" } @class;\n\n... or you would get \n\n \"Use of uninitialized value in concatenation (.) or string [...]\"\n\nwarning when 'remote_heads\" feature is disabled, isn't it?\n\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@refs\n\nNitpick: it is called '...' in git-for-each-ref manpage...\n\n> \t\tor return;\n> \twhile (my $line = <$fd>) {\n> \t\tmy %ref_item;\n> -- \n> 1.7.3.rc1.230.g8b572\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"150852","messageId":"201009170030.05999.jnareb@gmail.com","threadId":"25122","inReplyTo":"AANLkTi=N=jyj8ueVNu601N9qtBktmC-kVv9+0jSA=Sv-@mail.gmail.com","subject":"Re: [PATCH 3/7] gitweb: separate heads and remotes lists","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-16T22:30:05Z","receivedAt":"2010-09-16T22:30:05Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 16 Sep 2010, Ævar Arnfjörð Bjarmason wrote:\n> On Thu, Sep 16, 2010 at 09:31, Giuseppe Bilotta\n> wrote:\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> > +       }\n> \n> Nit: The $# syntax is pseudo-deprecated, and since you use 16 as a\n> constant above this would be clearer anyway:\n> \n> @remotelist <= 16 ? undef : ...\n\nActually gitweb uses *15* as a constant above... \n\n-- \nJakub Narebski\nPoland\n"},{"id":"150853","messageId":"201009170046.41074.jnareb@gmail.com","threadId":"25122","inReplyTo":"1284629465-14798-4-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCH 3/7] gitweb: separate heads and remotes lists","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-16T22:46:40Z","receivedAt":"2010-09-16T22:46:40Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 16 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\nVery nice.\n\nACK.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"150854","messageId":"AANLkTimO7b8dxPKdOsEnoEpY6mAB86pKM3Cbc8GA4_CG@mail.gmail.com","threadId":"25122","inReplyTo":"201009170030.05999.jnareb@gmail.com","subject":"Re: [PATCH 3/7] gitweb: separate heads and remotes lists","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-09-16T22:54:23Z","receivedAt":"2010-09-16T22:54:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Thu, Sep 16, 2010 at 22:30, Jakub Narebski wrote:\n> On Thu, 16 Sep 2010, Ævar Arnfjörð Bjarmason wrote:\n>> On Thu, Sep 16, 2010 at 09:31, Giuseppe Bilotta\n>> wrote:\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>> > +       }\n>>\n>> Nit: The $# syntax is pseudo-deprecated, and since you use 16 as a\n>> constant above this would be clearer anyway:\n>>\n>>     @remotelist <= 16 ? undef : ...\n>\n> Actually gitweb uses *15* as a constant above...\n\nI mean above that snippet, i.e. this:\n\n + my @remotelist = $remote_heads ? git_get_heads_list(16,\n'remotes') : ()\n\nI think that's a bit clearer, you're not left wondering why it's 16\nthere and 15 here, and don't have to recall that `$#remotelist <= 15`\nequals `@remotelist <= 16` (unless $[ != 0). But like I said, it's a\nnit.\n"},{"id":"150855","messageId":"201009170102.23053.jnareb@gmail.com","threadId":"25122","inReplyTo":"1284629465-14798-5-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCH 4/7] gitweb: link heads and remotes view","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-16T23:02:22Z","receivedAt":"2010-09-16T23:02:22Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:\n\n> Add a link in heads view to remotes view (if the feature is\n> enabled), and conversely from remotes to heads.\n\nGood idea... but this commit message doesn't tell us *where* this link\ndo appear. It is in lower part (the action specific part) of page\nnavigation menu.\n\n> Signed-off-by: Giuseppe Bilotta \n> ---\n> gitweb/gitweb.perl | 10 ++++++++--\n> 1 files changed, 8 insertions(+), 2 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 0118739..6138c6e 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -5511,7 +5511,10 @@ 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> +\tmy $heads_nav = gitweb_check_feature('remote_heads') ?\n> +\t\t$cgi->a({-href => href(action=>\"remotes\", -replay=>1)},\n> +\t\t \"remotes\") : undef;\n\nI think it would be more readable here to use 'if' statement instead\nof conditional operator.\n\n> +\tgit_print_page_nav('','', $head,undef,$head, $heads_nav);\n> \tgit_print_header_div('summary', $project);\n> \n> \tmy @headslist = git_get_heads_list(undef, 'heads');\n> @@ -5527,7 +5530,10 @@ 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> +\tmy $heads_nav =\n> +\t\t$cgi->a({-href => href(action=>\"heads\", -replay=>1)},\n> +\t\t \"heads\");\n> +\tgit_print_page_nav('','', $head,undef,$head, $heads_nav);\n> \tgit_print_header_div('summary', $project);\n> \n> \tmy @remotelist = git_get_heads_list(undef, 'remotes');\n> -- \n> 1.7.3.rc1.230.g8b572\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"150861","messageId":"201009170324.25801.jnareb@gmail.com","threadId":"25122","inReplyTo":"1284629465-14798-6-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCH 5/7] gitweb: auxiliary functions to group data","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-17T01:24:24Z","receivedAt":"2010-09-17T01:24:24Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:\n\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 6138c6e..92551e4 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -3765,6 +3765,21 @@ sub git_print_header_div {\n> \t \"\\n\\n\";\n> }\n> \n> +sub git_begin_group {\n> +\tmy ($class, $id, @rest) = @_;\n> +\n> +\t$class = ' class=\"' . join(' ', 'group', $class) . '\"';\n> +\n> +\t$id = ' id=\"' . $id . '\"' if $id;\n> +\n> +\tprint \"\\n\";\n\nI agree with Ævar that it would be better to use HTML generation\nsubroutines from CGI.pm, even start_div and end_div...\n\n> +\tgit_print_header_div(@rest);\n> +}\n> +\n> +sub git_end_group {\n> +\tprint \"\\n\"\n> +}\n\n... but I think that having separate subroutines for opening and\nclosing tags is a bad design / bad API (except in some rare cases).\nIt is begging for unbalanced HTML.\n\nIt would be better if it was a single subroutine wrapping 'div' around\ncontents given either as a string, or via callback (subroutine reference),\nin my opinion.\n\n> +\n> sub print_local_time {\n> \tprint format_local_time(@_);\n> }\n> -- \n> 1.7.3.rc1.230.g8b572\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"150866","messageId":"AANLkTikk7vUFMr31Azdcpe+PK0-yA2sp2EcnHjdfw0Zn@mail.gmail.com","threadId":"25122","inReplyTo":"201009170324.25801.jnareb@gmail.com","subject":"Re: [PATCH 5/7] gitweb: auxiliary functions to group data","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-17T06:54:03Z","receivedAt":"2010-09-17T06:54:03Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Fri, Sep 17, 2010 at 3:24 AM, Jakub Narebski wrote:\n>\n> ... but I think that having separate subroutines for opening and\n> closing tags is a bad design / bad API (except in some rare cases).\n> It is begging for unbalanced HTML.\n>\n> It would be better if it was a single subroutine wrapping 'div' around\n> contents given either as a string, or via callback (subroutine reference),\n> in my opinion.\n\nI'm not sure that in this case the string or callback approach would\nbe any cleaner. I'll see if perl supports closures or something like\nthat.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"150868","messageId":"AANLkTinQaad7vDCqRKQkDcdRY=M+wLmx6eD3ftg6Nxh=@mail.gmail.com","threadId":"25122","inReplyTo":"201009162326.01656.jnareb@gmail.com","subject":"Re: [PATCH 0/7] gitweb: allheads feature","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-17T07:24:37Z","receivedAt":"2010-09-17T07:24:37Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2010/9/16 Jakub Narebski :\n> On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:\n>\n>> This is a rehash of an old patchset of mine that got stalled waiting for\n>> other independent patches to go in first, and then for me to get the\n>> time to work on it again.\n>>\n>> The first 4 patches are IMO ready for inclusing in gitweb, and their\n>> purpose is to introduce a new view (and a new summary block) that\n>> display all the remote heads (assuming the feature is enabled).\n>> Somebody suggested via email that this could even the basis for some\n>> kind of 'social graph' for gitweb repositories, in a way similar to what\n>> is found on sites like github or gitorious, but for me the feature in\n>> itself can already be useful.\n>\n> We might want to make git-instaweb enable this feature (and probably\n> other disabled-by-default features).  Otherwise some of information\n> about git repository you examine is hidden.  So I agree that this\n> feature is useful by itself.\n\nI'll add a patch to that effect in the next iteration.\n\n>> The final result is rather curious and you can see it in action at\n>> , although it would be nice to find\n>> a way to layout the blocks in a smarter way.\n>\n> Thanks for providing demo site.\n>\n> Note that clicking on header for remote block, which should lead to\n> displaying of only single remote displays all remotes, see e.g.\n> http://git.oblomov.eu/rbot/remotes/a3li.  Moreover when I tried to\n> handcraft URL i.e. http://git.oblomov.eu/rbot/remote/a3li (with\n> 'remote' rather than 'remotes' action) I get an empty list of heads.\n\nThe /remote/ is just the standard behavior of git when any\nrandom string is passed as PATH_INFO (try\nhttp://git.oblomov.eu/rbot/whatever), so I cannot fix that unless I\nmap remote to remotes. The remotes/ is a TODO\n\n(This led me to think: do we want heads/ to point to the\nshortlog of that head? It currently does nothing)\n\n> About layout of 'remotes' view: to have remotes information aligned\n> we would have to either put everything in one single table (with remotes\n> headers being \"colspan\"), or style them with minimum width (which could\n> be good idea anyway).\n\nI don't like the 'one single table' approach because, in contrast to\nthe current approach it doesn't allow for automatic block\nrearrangement based on the window width.\nThe minimum width helps align the blocks vertically, but it doesn't\nalign the contents of the tables in it, so it's not really that big an\nimprovement (actually, I find it even funnier that way because of the\nillusion of alignment provided by the blocks).\n\n>> What I really don't like (at the moment) is the way things come out\n>> in summary view instead.\n>\n> What you don't like about it?  Should it be smarter and display only\n> list of remotes, perhaps even limited to 15 elements, if there are many\n> remote-tracking branches?  Or is it something else?\n>\n>> The issue there is that we only gather 16 remote heads, so some remotes\n>> might have no branches displayed, but it becomes difficult to detect and\n>> indicate when remotes have incomplete information being displayed. A\n>> possible solution would be to call show-ref N times (N being the number\n>> of remotes) with a limit of 16/N heads, but that can be a lot of calls.\n>> So I'm open to suggestions on how to improve this part (maybe just show\n>> a flat view in the remotes section of summary view?)\n>\n> Ah, I see...\n>\n> Alternatively we could use smart limiting on the gitweb side.\n\nThat was an option I was considering. I thought the limiting to 16\nrefs and tags in summary view was for performance reasons, so I\nthought that grabbing all remote heads would kind of kill the remote\nview, but for sure it's more efficient than doing a ref retrieval for\neach remote. OTOH, once we get all the refs, why would we only display\nsome of them?\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"150883","messageId":"AANLkTin43YXnfETqK2ZJASaT5gMemrJqiX+xzfn13s7n@mail.gmail.com","threadId":"25122","inReplyTo":"201009162341.20380.jnareb@gmail.com","subject":"Re: [PATCH 1/7] gitweb: introduce remote_heads feature","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-17T15:39:58Z","receivedAt":"2010-09-17T15:39:58Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2010/9/16 Jakub Narebski :\n> On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:\n>>               or return;\n>>       while (my $line = <$fd>) {\n>>               my %ref_item;\n>> @@ -3160,8 +3174,9 @@ sub git_get_heads_list {\n>>               my ($committer, $epoch, $tz) =\n>>                       ($committerinfo =~ /^(.*) ([0-9]+) (.*)$/);\n>>               $ref_item{'fullname'}  = $name;\n>> -             $name =~ s!^refs/heads/!!;\n>> +             $name =~ s!^refs/(head|remote)s/!!;\n>>\n>> +             $ref_item{'class'} = $1;\n>\n> Is it used anywhere, or is it left to be used by a further commit in\n> the series?  If it is the latter, perhaps it would be worth mentioning\n> in the commit message?\n\nActually, I think this might be a leftover from some previous\niteration (IIRC my first version of this patchset listed all heads\nunder 'heads' grouping them by the class defined here, whereas now we\nhave the two separate sections). I should probably scratch it (even\nthough a similar mechanism is already in place in format_ref_marker\nand it might be useful to coalesce this when parsing refs)\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"150884","messageId":"AANLkTi=fF7QGNrAepy8WbzgzQYUbKESO49QBtDLCHgcV@mail.gmail.com","threadId":"25122","inReplyTo":"201009170014.12483.jnareb@gmail.com","subject":"Re: [PATCH 2/7] gitweb: git_get_heads_list accepts an optional list of refs.","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-17T15:52:23Z","receivedAt":"2010-09-17T15:52:23Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2010/9/17 Jakub Narebski :\n> On Thu, 16 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>\n> I like this API very much.\n\nThank you.\n\n>> Signed-off-by: Giuseppe Bilotta \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 7116c26..21e83bb 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>> -     my $limit = shift;\n>> +     my ($limit, @class) = @_;\n>\n> Nitpick: @class or @classes.\n\nRight, the general gitweb style is for plural for arrays.\n\n>> +     unless (defined @class) {\n>> +             my $remote_heads = gitweb_check_feature('remote_heads');\n>> +             @class = ('heads', $remote_heads ? 'remotes' : undef);\n>\n> Same comment as for previous patch:\n>\n>  +             @class = ('heads', $remote_heads ? 'remotes' : ());\n>\n> Or alternative solution:\n>\n>  +             @class = 'heads';\n>  +             push @class, 'remotes' if gitweb_check_feature('remote_heads');\n>\n\nI like the () solution better. Less verbosity\n\n>>       my @headslist;\n>>\n>> -     my $remote_heads = gitweb_check_feature('remote_heads');\n>> -\n>>       open my $fd, '-|', git_cmd(), 'for-each-ref',\n>>               ($limit ? '--count='.($limit+1) : ()), '--sort=-committerdate',\n>>               '--format=%(objectname) %(refname) %(subject)%00%(committer)',\n>> -             'refs/heads', ( $remote_heads ? 'refs/remotes' : '')\n>> +             @refs\n>\n> Nitpick: it is called '...' in git-for-each-ref manpage...\n\nI'm not particularly enamoured with @refs, so @patterns it is (if I\nget what you mean here)\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"150885","messageId":"AANLkTi=vB-aoEnajR_Lif1XO7+4zgq3HK6he6umk=svJ@mail.gmail.com","threadId":"25122","inReplyTo":"201009170102.23053.jnareb@gmail.com","subject":"Re: [PATCH 4/7] gitweb: link heads and remotes view","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-17T16:01:18Z","receivedAt":"2010-09-17T16:01:18Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2010/9/17 Jakub Narebski :\n> On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:\n>\n>> Add a link in heads view to remotes view (if the feature is\n>> enabled), and conversely from remotes to heads.\n>\n> Good idea... but this commit message doesn't tell us *where* this link\n> do appear.  It is in lower part (the action specific part) of page\n> navigation menu.\n\nI will clarify it in the next rehash of this patchset.\n\n>> Signed-off-by: Giuseppe Bilotta \n>> ---\n>>  gitweb/gitweb.perl |   10 ++++++++--\n>>  1 files changed, 8 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>> index 0118739..6138c6e 100755\n>> --- a/gitweb/gitweb.perl\n>> +++ b/gitweb/gitweb.perl\n>> @@ -5511,7 +5511,10 @@ sub git_tags {\n>>  sub git_heads {\n>>       my $head = git_get_head_hash($project);\n>>       git_header_html();\n>> -     git_print_page_nav('','', $head,undef,$head);\n>> +     my $heads_nav = gitweb_check_feature('remote_heads') ?\n>> +             $cgi->a({-href => href(action=>\"remotes\", -replay=>1)},\n>> +                     \"remotes\") : undef;\n>\n> I think it would be more readable here to use 'if' statement instead\n> of conditional operator.\n\nYou're right. I'm actually thinking about putting 'tags' in that nav\nmenu too, as it makes sense to link to all refs commands there.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"150886","messageId":"201009171806.49774.jnareb@gmail.com","threadId":"25122","inReplyTo":"AANLkTikk7vUFMr31Azdcpe+PK0-yA2sp2EcnHjdfw0Zn@mail.gmail.com","subject":"Re: [PATCH 5/7] gitweb: auxiliary functions to group data","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-17T16:06:48Z","receivedAt":"2010-09-17T16:06:48Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Giuseppe Bilotta wrote:\n> On Fri, Sep 17, 2010 at 3:24 AM, Jakub Narebski wrote:\n> >\n> > ... but I think that having separate subroutines for opening and\n> > closing tags is a bad design / bad API (except in some rare cases).\n> > It is begging for unbalanced HTML.\n> >\n> > It would be better if it was a single subroutine wrapping 'div' around\n> > contents given either as a string, or via callback (subroutine reference),\n> > in my opinion.\n> \n> I'm not sure that in this case the string or callback approach would\n> be any cleaner. I'll see if perl supports closures or something like\n> that.\n\nPerl supports closures (thanks to anonymous subroutines 'sub { ... }'\nand lexical variables 'my $var'), see perlsub and \"Function Templates\"\nin perlref.\n\nI also recommend free ebook \"Higher-Order Perl\" http://hop.perl.plover.com/\n-- \nJakub Narebski\nPoland\n"},{"id":"150889","messageId":"201009171826.15454.jnareb@gmail.com","threadId":"25122","inReplyTo":"1284629465-14798-7-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCH 6/7] gitweb: group styling","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-17T16:26:15Z","receivedAt":"2010-09-17T16:26:15Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:\n\n> +div.group {\n> +\tmargin: .5em;\n> +\tborder: 1px solid #d9d8d1;\n> +\tdisplay: inline-block;\n\nIs this 'display: inline-block;' really necessary? I think we can do\nwithout it (and I've heard that there are problems with it, but those\nmight not matter in layout used by gitweb).\n\nOtherwise nice.\n\n> +}\n\n-- \nJakub Narebski\nPoland\n"},{"id":"150891","messageId":"AANLkTikjLNva7Jgh0xYeah1maFusfOiaLwY7+eixx8so@mail.gmail.com","threadId":"25122","inReplyTo":"201009171806.49774.jnareb@gmail.com","subject":"Re: [PATCH 5/7] gitweb: auxiliary functions to group data","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-17T16:41:06Z","receivedAt":"2010-09-17T16:41:06Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Fri, Sep 17, 2010 at 6:06 PM, Jakub Narebski wrote:\n> Giuseppe Bilotta wrote:\n>> On Fri, Sep 17, 2010 at 3:24 AM, Jakub Narebski wrote:\n>> >\n>> > ... but I think that having separate subroutines for opening and\n>> > closing tags is a bad design / bad API (except in some rare cases).\n>> > It is begging for unbalanced HTML.\n>> >\n>> > It would be better if it was a single subroutine wrapping 'div' around\n>> > contents given either as a string, or via callback (subroutine reference),\n>> > in my opinion.\n>>\n>> I'm not sure that in this case the string or callback approach would\n>> be any cleaner. I'll see if perl supports closures or something like\n>> that.\n>\n> Perl supports closures (thanks to anonymous subroutines 'sub { ... }'\n> and lexical variables 'my $var'), see perlsub and \"Function Templates\"\n> in perlref.\n>\n> I also recommend free ebook \"Higher-Order Perl\" http://hop.perl.plover.com/\n\nThanks for the suggestion. I'm still not convinced that such an\nimplementation would be better though. Aside from the general\naesthetical suckiness of passing closures around (and the experience\nis not any more pleasurable in Perl), there's also the matter of the\ncalling convention to use. I can think of two options:\n\n(1) we make the function callable as git_do_group($class, $id, sub {\n }, ), which is somewhat illogical since we're\nspecifying the content before the header, or\n\n(2) we make it like git_do_group($class, $id, sub { }, sub {}), which\nis even more horrible since the header div is just a\ngit_print_header_div\n\nOverall, I still get the impression that the current API is\nconsiderably cleaner, even with the small counterweight of the risk of\nleaving groups open (which is not something so dramatic anyway, IMO).\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"150892","messageId":"AANLkTi=8x4JuK99W5BOfUBVUza1sUXE3e_mDOq9-wF_z@mail.gmail.com","threadId":"25122","inReplyTo":"201009171826.15454.jnareb@gmail.com","subject":"Re: [PATCH 6/7] gitweb: group styling","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-17T16:49:08Z","receivedAt":"2010-09-17T16:49:08Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"2010/9/17 Jakub Narebski :\n> On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:\n>\n>> +div.group {\n>> +     margin: .5em;\n>> +     border: 1px solid #d9d8d1;\n>> +     display: inline-block;\n>\n> Is this 'display: inline-block;' really necessary?  I think we can do\n> without it (and I've heard that there are problems with it, but those\n> might not matter in layout used by gitweb).\n>\n> Otherwise nice.\n>\n>> +}\n\nI'm not aware of problems with inline-block (I can check\nquirksmode.org though if necessary), but without it the divs will be\nas large as the _containing_ block, meaning that each div will be\nultimately as large as the page. By using inline-block, instead, they\nbecome as large as the _contained_ stuff, thus fitting the inner\ntable.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"150893","messageId":"201009171854.03476.jnareb@gmail.com","threadId":"25122","inReplyTo":"1284629465-14798-8-git-send-email-giuseppe.bilotta@gmail.com","subject":"Re: [PATCH 7/7] gitweb: group remote heads","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-17T16:54:03Z","receivedAt":"2010-09-17T16:54:03Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:\n\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 92551e4..66b5400 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -2758,6 +2758,16 @@ sub git_get_last_activity {\n> \treturn (undef, undef);\n> }\n> \n> +sub git_get_remotes {\n> +\tmy ($limit) = @_;\n\nProbably more Perl-ish way would be to use\n\n +\tmy $limit = shift;\n\nbut this version is also all right.\n\n> +\topen my $fd, '-|' , git_cmd(), 'remote';\n> +\treturn () unless $fd;\n\nGitweb usualy uses\n\n +\topen my $fd, '-|' , git_cmd(), 'remote';\n +\t\tor return;\n\n> +\tmy @remotes = map { chomp ; $_ } <$fd>;\n\nAbout Ævar comment: while\n\n +\tchomp(my @remotes = <$fd>);\n\nmight be more Perl-ish, the above syntax is used thorough gitweb code.\nSo I'd leave it as it is now as the matter of code consistency.\n\n> +\tclose $fd or return ();\n\n +\tclose $fd or return;\n\n> +\tmy @remoteheads = git_get_heads_list($limit, 'remotes');\n> +\treturn (\\@remotes, \\@remoteheads);\n\nWhy do you want for git_get_remotes() to also return remote-tracking\nbranches (refs/remotes/)? Those are separate issues, and I think it\nwould be better API for git_get_remotes() to provide only list of\nremotes, i.e.\n\n +\treturn @remotes;\n\nEspecially that we might want in the summary view to only list remotes,\nwithout listing remote-tracking branches.\n\nThat would require more changes to the code.\n\n> +}\n> +\n> sub git_get_references {\n> \tmy $type = shift || \"\";\n> \tmy %refs;\n> @@ -4979,7 +4989,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\nThis is independent change, and should be in a separate commit, isn't it?\n\n> \t\t \"\\n\" .\n> \t\t \"\";\n> \t}\n> @@ -4991,6 +5001,19 @@ sub git_heads_body {\n> \tprint \"\\n\";\n> }\n> \n> +sub git_remotes_body {\n> +\tmy ($remotedata, $head) = @_;\n> +\tmy @remotenames = @{$remotedata->[0]};\n> +\tmy @allheads = @{$remotedata->[1]};\n\nWhy not\n\n +\tmy ($remotenames, $allheads, $head) = @_;\n\nBeside, isn't it $remote_heads and not $allheads?\n\n> +\tforeach my $remote (@remotenames) {\n\nIt would be then\n\n +\tforeach my $remote (@$remotenames) {\n\n> +\t\tmy @remoteheads = grep { $_->{'name'} =~ s!^\\Q$remote\\E/!! } @allheads;\n\nShould we display remote even if it doesn't have any remote heads\nassociated with it?\n\n> +\t\tgit_begin_group(\"remotes\", $remote, \"remotes/$remote\",$remote);\n> +\t\tgit_heads_body(\\@remoteheads, $head);\n> +\t\tgit_end_group();\n\nThis would have to be modified with change to git_begin_group() / \n/ git_end_group().\n\nBTW isn't it premature generalization? It is only place AFAIKS that \nuses git_*_group() subroutines.\n\n> +\t}\n> +\n> +}\n> +\n> sub git_search_grep_body {\n> \tmy ($commitlist, $from, $to, $extra) = @_;\n> \t$from = 0 unless defined $from;\n> @@ -5137,7 +5160,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(16) : ();\n\nNo change of git_get_remotes() does only one thing: returning list\nof remotes.\n\n> \tmy @forklist;\n> \tmy $check_forks = gitweb_check_feature('forks');\n> \n> @@ -5217,9 +5240,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\nCalling convention would change with my proposed change.\n\nWe might want to print only list of remotes (perhaps with number of\ntracked branches) in the 'summary' view. Just an idea...\n\n> \t}\n> \n> \tif (@forklist) {\n> @@ -5551,9 +5572,9 @@ sub git_remotes {\n> \tgit_print_page_nav('','', $head,undef,$head, $heads_nav);\n> \tgit_print_header_div('summary', $project);\n> \n> -\tmy @remotelist = git_get_heads_list(undef, 'remotes');\n> +\tmy @remotelist = git_get_remotes();\n> \tif (@remotelist) {\n> -\t\tgit_heads_body(\\@remotelist, $head);\n> +\t\tgit_remotes_body(\\@remotelist, $head);\n\nSame here.\n\n> \t}\n> \tgit_footer_html();\n> }\n> -- \n> 1.7.3.rc1.230.g8b572\n> \n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"150895","messageId":"201009171917.44979.jnareb@gmail.com","threadId":"25122","inReplyTo":"AANLkTikjLNva7Jgh0xYeah1maFusfOiaLwY7+eixx8so@mail.gmail.com","subject":"Re: [PATCH 5/7] gitweb: auxiliary functions to group data","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-17T17:17:44Z","receivedAt":"2010-09-17T17:17:44Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 17 Sep 2010, Giuseppe Bilotta wrote:\n> On Fri, Sep 17, 2010 at 6:06 PM, Jakub Narebski wrote:\n>> Giuseppe Bilotta wrote:\n>>> On Fri, Sep 17, 2010 at 3:24 AM, Jakub Narebski wrote:\n>>>>\n>>>> ... but I think that having separate subroutines for opening and\n>>>> closing tags is a bad design / bad API (except in some rare cases).\n>>>> It is begging for unbalanced HTML.\n>>>>\n>>>> It would be better if it was a single subroutine wrapping 'div' around\n>>>> contents given either as a string, or via callback (subroutine reference),\n>>>> in my opinion.\n>>>\n>>> I'm not sure that in this case the string or callback approach would\n>>> be any cleaner. I'll see if perl supports closures or something like\n>>> that.\n>>\n>> Perl supports closures (thanks to anonymous subroutines 'sub { ... }'\n>> and lexical variables 'my $var'), see perlsub and \"Function Templates\"\n>> in perlref.\n>>\n>> I also recommend free ebook \"Higher-Order Perl\" http://hop.perl.plover.com/\n> \n> Thanks for the suggestion. I'm still not convinced that such an\n> implementation would be better though. Aside from the general\n> aesthetical suckiness of passing closures around (and the experience\n> is not any more pleasurable in Perl), there's also the matter of the\n> calling convention to use. I can think of two options:\n> \n> (1) we make the function callable as git_do_group($class, $id, sub {\n> }, git_print_header_div>), which is somewhat illogical since we're\n> specifying the content before the header, \n\nWhy not\n\n git_do_group($class, $id, , sub { ... })\n\nor even use subroutine prototypes? We can use 'pop @_' to get last\nargument of a subroutine.\n\n[...]\n> Overall, I still get the impression that the current API is\n> considerably cleaner, even with the small counterweight of the risk of\n> leaving groups open (which is not something so dramatic anyway, IMO).\n\nMight be.\n\nBut as currently git_*group() is used in only one place, isn't it\npremature generalization ;-) ?\n-- \nJakub Narebski\nPoland\n"},{"id":"150896","messageId":"201009171922.53396.jnareb@gmail.com","threadId":"25122","inReplyTo":"AANLkTi=8x4JuK99W5BOfUBVUza1sUXE3e_mDOq9-wF_z@mail.gmail.com","subject":"Re: [PATCH 6/7] gitweb: group styling","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-17T17:22:53Z","receivedAt":"2010-09-17T17:22:53Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Giuseppe Bilotta wrote:\n> 2010/9/17 Jakub Narebski :\n>> On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:\n>>\n>>> +div.group {\n>>> +     margin: .5em;\n>>> +     border: 1px solid #d9d8d1;\n>>> +     display: inline-block;\n>>\n>> Is this 'display: inline-block;' really necessary?  I think we can do\n>> without it (and I've heard that there are problems with it, but those\n>> might not matter in layout used by gitweb).\n>>\n>> Otherwise nice.\n> \n> I'm not aware of problems with inline-block (I can check\n> quirksmode.org though if necessary), but without it the divs will be\n> as large as the _containing_ block, meaning that each div will be\n> ultimately as large as the page. By using inline-block, instead, they\n> become as large as the _contained_ stuff, thus fitting the inner\n> table.\n\nErrr... I am using old web browser that doesn't support 'inline-block',\nso I don't know what it should look like. I'll check later.\n\nBut graceful degradation to 'block' doesn't look too bad.\n-- \nJakub Narebski\nPoland\n"},{"id":"150898","messageId":"201009171925.43590.jnareb@gmail.com","threadId":"25122","inReplyTo":"201009171854.03476.jnareb@gmail.com","subject":"Re: [PATCH 7/7] gitweb: group remote heads","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-17T17:25:43Z","receivedAt":"2010-09-17T17:25:43Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Fri, 17 Sep 2010, Jakub Narebski wrote:\n> On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:\n\n> > +\t\tmy @remoteheads = grep { $_->{'name'} =~ s!^\\Q$remote\\E/!! } @allheads;\n> \n> Should we display remote even if it doesn't have any remote heads\n> associated with it?\n\nBy the way, it would be place where we could limit number of \nremote-tracking branches displayed in each remote block.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"150929","messageId":"AANLkTimN+rGCS7pokwSDVT75_LsU4aAsaHmw7Xi_HVm+@mail.gmail.com","threadId":"25122","inReplyTo":"201009171917.44979.jnareb@gmail.com","subject":"Re: [PATCH 5/7] gitweb: auxiliary functions to group data","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-18T07:51:13Z","receivedAt":"2010-09-18T07:51:13Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Fri, Sep 17, 2010 at 7:17 PM, Jakub Narebski wrote:\n> On Fri, 17 Sep 2010, Giuseppe Bilotta wrote:\n>> On Fri, Sep 17, 2010 at 6:06 PM, Jakub Narebski wrote:\n>>> Giuseppe Bilotta wrote:\n>>>> On Fri, Sep 17, 2010 at 3:24 AM, Jakub Narebski wrote:\n>>>>>\n>>>>> ... but I think that having separate subroutines for opening and\n>>>>> closing tags is a bad design / bad API (except in some rare cases).\n>>>>> It is begging for unbalanced HTML.\n>>>>>\n>>>>> It would be better if it was a single subroutine wrapping 'div' around\n>>>>> contents given either as a string, or via callback (subroutine reference),\n>>>>> in my opinion.\n>>>>\n>>>> I'm not sure that in this case the string or callback approach would\n>>>> be any cleaner. I'll see if perl supports closures or something like\n>>>> that.\n>>>\n>>> Perl supports closures (thanks to anonymous subroutines 'sub { ... }'\n>>> and lexical variables 'my $var'), see perlsub and \"Function Templates\"\n>>> in perlref.\n>>>\n>>> I also recommend free ebook \"Higher-Order Perl\" http://hop.perl.plover.com/\n>>\n>> Thanks for the suggestion. I'm still not convinced that such an\n>> implementation would be better though. Aside from the general\n>> aesthetical suckiness of passing closures around (and the experience\n>> is not any more pleasurable in Perl), there's also the matter of the\n>> calling convention to use. I can think of two options:\n>>\n>> (1) we make the function callable as git_do_group($class, $id, sub {\n>> }, > git_print_header_div>), which is somewhat illogical since we're\n>> specifying the content before the header,\n>\n> Why not\n>\n>  git_do_group($class, $id, , sub { ... })\n>\n> or even use subroutine prototypes?  We can use 'pop @_' to get last\n> argument of a subroutine.\n\nAh, I hadn't thought about pop to get the last parameter. I think it\nmakes sense. Ok, I'll give this syntax a try,\n\n> [...]\n>> Overall, I still get the impression that the current API is\n>> considerably cleaner, even with the small counterweight of the risk of\n>> leaving groups open (which is not something so dramatic anyway, IMO).\n>\n> Might be.\n>\n> But as currently git_*group() is used in only one place, isn't it\n> premature generalization ;-) ?\n\nI noticed the same comment to patch 7/7, still writing the reply for\nthat patch. I believe the feature is useful enough to deem the\nrefactoring, and since it's written already why go back? ;-) (it also\nmakes the code cleaner IMO).\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"150979","messageId":"AANLkTinkikwt5cUxuXECfeQrKZthu271U82F3ebSrEmd@mail.gmail.com","threadId":"25122","inReplyTo":"201009171854.03476.jnareb@gmail.com","subject":"Re: [PATCH 7/7] gitweb: group remote heads","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-19T05:39:11Z","receivedAt":"2010-09-19T05:39:11Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Hello,\n\nas I mentioned, this patch was the one I had most doubts about. I will\ntherefore skip over the stylistic suggestions (which I _am_ following\nfor the next release of this patchset) and only reply to the more\ntechnical remarks.\n\nOn Fri, Sep 17, 2010 at 6:54 PM, Jakub Narebski wrote:\n> On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:\n>\n>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>> index 92551e4..66b5400 100755\n>> --- a/gitweb/gitweb.perl\n>> +++ b/gitweb/gitweb.perl\n>> @@ -2758,6 +2758,16 @@ sub git_get_last_activity {\n>>       return (undef, undef);\n>>  }\n>>\n>> +sub git_get_remotes {\n\n[snip]\n\n>> +     my @remoteheads = git_get_heads_list($limit, 'remotes');\n>> +     return (\\@remotes, \\@remoteheads);\n>\n> Why do you want for git_get_remotes() to also return remote-tracking\n> branches (refs/remotes/)?  Those are separate issues, and I think it\n> would be better API for git_get_remotes() to provide only list of\n> remotes, i.e.\n>\n>  +     return @remotes;\n>\n> Especially that we might want in the summary view to only list remotes,\n> without listing remote-tracking branches.\n>\n> That would require more changes to the code.\n\nThis is kind of the main issue with this patch. What do we want to do\nwith the remotes list in summary view and the remotes view? We\nbasically have three possibilities:\n\n(1) we can make the remotes list in summary view be a 'reduced\nremotes' view: this would make it behave the most similarly to the\nother components of summary view\n(2) we can make the remotes list be much more stripped down, by only\nlisting the remotes and possibly some summarizing property such as the\nnumber of branches in it or when it was last updated\n(3) we can make the remotes list be just a copy of the full remotes view.\n\nThe third option is surely the easiest to implement. The second option\nwith _only_ a list of remotes (no extra info) is also very easy to\nimplement _and_ fast to render. The second option with extra info, or\nthe first option, on the other hand, require the retrieval of some\nadditional data which, maybe due to my limited knowledge of git,\nessentially means retrieving _all_ the remote heads and then doing the\nfiltering in gitweb. But once we're getting all the information, why\nnot display it all? isn't it faster to just display all of it, in\nwhich case we go back to option 3?\n\nIf we go with option 3, it does make sense to get all remote names and\nall remote branches at once, and thus to make the git_get_remotes()\ncall do all of the work.\n\n>> +}\n>> +\n>>  sub git_get_references {\n>>       my $type = shift || \"\";\n>>       my %refs;\n>> @@ -4979,7 +4989,7 @@ sub git_heads_body {\n>>                     \"\" .\n>>                     $cgi->a({-href => href(action=>\"shortlog\", hash=>$ref{'fullname'})}, \"shortlog\") . \" | \" .\n>>                     $cgi->a({-href => href(action=>\"log\", hash=>$ref{'fullname'})}, \"log\") . \" | \" .\n>> -                   $cgi->a({-href => href(action=>\"tree\", hash=>$ref{'fullname'}, hash_base=>$ref{'name'})}, \"tree\") .\n>> +                   $cgi->a({-href => href(action=>\"tree\", hash=>$ref{'fullname'}, hash_base=>$ref{'fullname'})}, \"tree\") .\n>\n> This is independent change, and should be in a separate commit, isn't it?\n\nProbably yes, with an explanation of the why.\n\n>>                     \"\\n\" .\n>>                     \"\";\n>>       }\n>> @@ -4991,6 +5001,19 @@ sub git_heads_body {\n>>       print \"\\n\";\n>>  }\n>>\n>> +sub git_remotes_body {\n>> +     my ($remotedata, $head) = @_;\n>> +     my @remotenames = @{$remotedata->[0]};\n>> +     my @allheads = @{$remotedata->[1]};\n>\n> Why not\n>\n>  +     my ($remotenames, $allheads, $head) = @_;\n>\n> Beside, isn't it $remote_heads and not $allheads?\n\nI think it's a leftover name choice from the first version of the\npatch. Can change.\n\n>> +     foreach my $remote (@remotenames) {\n>\n> It would be then\n>\n>  +     foreach my $remote (@$remotenames) {\n>\n>> +             my @remoteheads = grep { $_->{'name'} =~ s!^\\Q$remote\\E/!! } @allheads;\n>\n> Should we display remote even if it doesn't have any remote heads\n> associated with it?\n>\n> By the way, it would be place where we could limit number of\n> remote-tracking branches displayed in each remote block.\n\nBut does it make sense to reduce the number of displayed branches\nafter we got the information about all of them? I think it depends on\nwhat summary view is intended to do exactly.\n\n>> +             git_begin_group(\"remotes\", $remote, \"remotes/$remote\",$remote);\n>> +             git_heads_body(\\@remoteheads, $head);\n>> +             git_end_group();\n>\n> This would have to be modified with change to git_begin_group() /\n> / git_end_group().\n\nOf course.\n\n> BTW isn't it premature generalization?  It is only place AFAIKS that\n> uses git_*_group() subroutines.\n\nIt's the only current use but I believe that, since it's factored out\nnow already and since it may be used in other views too (think:\ngrouping heads or tags by prefix) it might make sense to keep it this\nway.\n\n>> +     }\n>> +\n>> +}\n>> +\n>>  sub git_search_grep_body {\n>>       my ($commitlist, $from, $to, $extra) = @_;\n>>       $from = 0 unless defined $from;\n>> @@ -5137,7 +5160,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(16) : ();\n>\n> No change of git_get_remotes() does only one thing: returning list\n> of remotes.\n\nSee above about what should git_get_remotes() do. Even better, I was\nthinking about git_get_remotes() returning a hash (mapping remote\nnames to the heads from that remote)\n\nSo the big question (which essentially determines the functionality\nprovided by this last patch in the set) is: what do we want to do in\nsummary view?\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"151013","messageId":"201009200102.22960.jnareb@gmail.com","threadId":"25122","inReplyTo":"AANLkTinkikwt5cUxuXECfeQrKZthu271U82F3ebSrEmd@mail.gmail.com","subject":"Re: [PATCH 7/7] gitweb: group remote heads","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-19T23:02:21Z","receivedAt":"2010-09-19T23:02:21Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun, 19 Sep 2010, Giuseppe Bilotta wrote:\n> On Fri, Sep 17, 2010 at 6:54 PM, Jakub Narebski wrote:\n>> On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:\n>>\n>>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>>> index 92551e4..66b5400 100755\n>>> --- a/gitweb/gitweb.perl\n>>> +++ b/gitweb/gitweb.perl\n>>> @@ -2758,6 +2758,16 @@ sub git_get_last_activity {\n>>>       return (undef, undef);\n>>>  }\n>>>\n>>> +sub git_get_remotes {\n> \n> [snip]\n> \n>>> +     my @remoteheads = git_get_heads_list($limit, 'remotes');\n>>> +     return (\\@remotes, \\@remoteheads);\n>>\n>> Why do you want for git_get_remotes() to also return remote-tracking\n>> branches (refs/remotes/)?  Those are separate issues, and I think it\n>> would be better API for git_get_remotes() to provide only list of\n>> remotes, i.e.\n>>\n>>  +     return @remotes;\n>>\n>> Especially that we might want in the summary view to only list remotes,\n>> without listing remote-tracking branches.\n>>\n>> That would require more changes to the code.\n> \n> This is kind of the main issue with this patch. What do we want to do\n> with the remotes list in summary view and the remotes view? We\n> basically have three possibilities:\n> \n> (1) we can make the remotes list in summary view be a 'reduced\n> remotes' view: this would make it behave the most similarly to the\n> other components of summary view\n> (2) we can make the remotes list be much more stripped down, by only\n> listing the remotes and possibly some summarizing property such as the\n> number of branches in it or when it was last updated\n> (3) we can make the remotes list be just a copy of the full remotes view.\n> \n> The third option is surely the easiest to implement. The second option\n> with _only_ a list of remotes (no extra info) is also very easy to\n> implement _and_ fast to render. The second option with extra info, or\n> the first option, on the other hand, require the retrieval of some\n> additional data which, maybe due to my limited knowledge of git,\n> essentially means retrieving _all_ the remote heads and then doing the\n> filtering in gitweb. But once we're getting all the information, why\n> not display it all? isn't it faster to just display all of it, in\n> which case we go back to option 3?\n\nWhen thinking about how display information about remotes and\nremote-tracking branches in 'summary' view, we have to consider that\nthis view is meant to be what it says: a *summary*. That is why both\nthe 'heads' and the 'tags' section displays only 15 items.\n\nWithout grouping remote heads the natural solution was to simply repeat\nwhat we used for 'heads' subsection, namely limit displaying to 15\nremote-tracking branches in 'remotes' subsection of the 'summary' view.\n\nWith grouping it is more complicated. We can limit number of remote\nhead *per remote*, we can limit number of remotes... but what makes\nIMVHO least sense is limit number of remote heads *then* do grouping.\n\n\nThe solution (1) i.e. limiting number of remote heads per remote, with\nor without limiting number of remotes behaves, as you wrote, most\nsimilarly to other components of 'summary' view. On the other hand\nwith large number of remotes, and large number of remote heads in those\nremotes it might be too large for a *summary* view.\n\nThe solution (3) i.e. displaying only list of remotes (perhaps limited\nto 15 remotes) is simple and fast to render. On the other hand it offers\nleast information and might be too little in the case of single remote.\n\n> \n> If we go with option 3, it does make sense to get all remote names and\n> all remote branches at once, and thus to make the git_get_remotes()\n> call do all of the work.\n\nNot if subroutine is called git_get_remotes(), IMVHO. Perhaps\ngit_group_remotes()?\n \n>>> @@ -4991,6 +5001,19 @@ sub git_heads_body {\n>>>       print \"\\n\";\n>>>  }\n>>>\n>>> +sub git_remotes_body {\n>>> +     my ($remotedata, $head) = @_;\n>>> +     my @remotenames = @{$remotedata->[0]};\n>>> +     my @allheads = @{$remotedata->[1]};\n>>\n>> Why not\n>>\n>>  +     my ($remotenames, $allheads, $head) = @_;\n>>\n>> Beside, isn't it $remote_heads and not $allheads?\n> \n> I think it's a leftover name choice from the first version of the\n> patch. Can change.\n\nO.K.\n\n>>> +     foreach my $remote (@remotenames) {\n>>\n>> It would be then\n>>\n>>  +     foreach my $remote (@$remotenames) {\n>>\n>>> +             my @remoteheads = grep { $_->{'name'} =~ s!^\\Q$remote\\E/!! } @allheads;\n>>\n>> Should we display remote even if it doesn't have any remote heads\n>> associated with it?\n>>\n>> By the way, it would be place where we could limit number of\n>> remote-tracking branches displayed in each remote block.\n> \n> But does it make sense to reduce the number of displayed branches\n> after we got the information about all of them? I think it depends on\n> what summary view is intended to do exactly.\n\nRight. But I think limit then group is not a very good choice from\nall other available.\n \n>>> +             git_begin_group(\"remotes\", $remote, \"remotes/$remote\",$remote);\n>>> +             git_heads_body(\\@remoteheads, $head);\n>>> +             git_end_group();\n>>\n>> This would have to be modified with change to git_begin_group() /\n>> / git_end_group().\n> \n> Of course.\n> \n>> BTW isn't it premature generalization?  It is only place AFAIKS that\n>> uses git_*_group() subroutines.\n> \n> It's the only current use but I believe that, since it's factored out\n> now already and since it may be used in other views too (think:\n> grouping heads or tags by prefix) it might make sense to keep it this\n> way.\n\nIf it could be used for other blocks (like 'readme', or 'heads',\nor 'tags') itself it would be even better.\n \n>>> +     }\n>>> +\n>>> +}\n>>> +\n>>>  sub git_search_grep_body {\n>>>       my ($commitlist, $from, $to, $extra) = @_;\n>>>       $from = 0 unless defined $from;\n>>> @@ -5137,7 +5160,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(16) : ();\n>>\n>> No change of git_get_remotes() does only one thing: returning list\n>> of remotes.\n> \n> See above about what should git_get_remotes() do. Even better, I was\n> thinking about git_get_remotes() returning a hash (mapping remote\n> names to the heads from that remote)\n\nWouldn't it be then named git_group_remotes() or something like that?\n\nIf remote doesn't have remote-tracking branches associated with it\n(e.g. push-only remote, or remote predating separate remotes layout)\nthe value would be undef for given remote as key in hash.\n \n> So the big question (which essentially determines the functionality\n> provided by this last patch in the set) is: what do we want to do in\n> summary view?\n\n-- \nJakub Narebski\nPoland\n"},{"id":"151052","messageId":"AANLkTim0X-3PFccXTjH3Mo5eEHL+7zBcifqHu9hWp_RP@mail.gmail.com","threadId":"25122","inReplyTo":"201009200102.22960.jnareb@gmail.com","subject":"Re: [PATCH 7/7] gitweb: group remote heads","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-20T08:15:34Z","receivedAt":"2010-09-20T08:15:34Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Mon, Sep 20, 2010 at 1:02 AM, Jakub Narebski wrote:\n>\n> When thinking about how display information about remotes and\n> remote-tracking branches in 'summary' view, we have to consider that\n> this view is meant to be what it says: a *summary*.  That is why both\n> the 'heads' and the 'tags' section displays only 15 items.\n>\n> Without grouping remote heads the natural solution was to simply repeat\n> what we used for 'heads' subsection, namely limit displaying to 15\n> remote-tracking branches in 'remotes' subsection of the 'summary' view.\n>\n> With grouping it is more complicated.  We can limit number of remote\n> head *per remote*, we can limit number of remotes... but what makes\n> IMVHO least sense is limit number of remote heads *then* do grouping.\n\nThis is something I absolutely agree with, BTW. It is the way it's\nimplemented currently because it was the quickest way to ship a\nprototype out for discussion.\n\n> The solution (1) i.e. limiting number of remote heads per remote, with\n> or without limiting number of remotes behaves, as you wrote, most\n> similarly to other components of 'summary' view.  On the other hand\n> with large number of remotes, and large number of remote heads in those\n> remotes it might be too large for a *summary* view.\n\nSo you maintain that limiting the amount of data in summary view\nshould be primary wrt to limiting the amount of time?\n\n> The solution (3) i.e. displaying only list of remotes (perhaps limited\n> to 15 remotes) is simple and fast to render.  On the other hand it offers\n> least information and might be too little in the case of single remote.\n\nIf time spent processing is not an issue, we can retrieve the number\nof heads for each remote and display that, for example. Or even play\nwith some more dynamic stuff like making each group collapsible,\nstarting with it collapsed and then display the content when the user\nhovers it with the mouse, for example.\n\n>> If we go with option 3, it does make sense to get all remote names and\n>> all remote branches at once, and thus to make the git_get_remotes()\n>> call do all of the work.\n>\n> Not if subroutine is called git_get_remotes(), IMVHO.  Perhaps\n> git_group_remotes()?\n\nThat I can do.\n\n>> It's the only current use but I believe that, since it's factored out\n>> now already and since it may be used in other views too (think:\n>> grouping heads or tags by prefix) it might make sense to keep it this\n>> way.\n>\n> If it could be used for other blocks (like 'readme', or 'heads',\n> or 'tags') itself it would be even better.\n\nThat's a possibility, although it would change the layout some (which\nis not necessarily a bad thing)\n\n> If remote doesn't have remote-tracking branches associated with it\n> (e.g. push-only remote, or remote predating separate remotes layout)\n> the value would be undef for given remote as key in hash.\n\nYes, this is something I have to take into consideration. Skip\ndisplaying them is probably the best idea (unless we have other ways\nto gather information about them).\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"151094","messageId":"201009201059.07742.jnareb@gmail.com","threadId":"25122","inReplyTo":"AANLkTim0X-3PFccXTjH3Mo5eEHL+7zBcifqHu9hWp_RP@mail.gmail.com","subject":"Re: [PATCH 7/7] gitweb: group remote heads","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-20T08:59:05Z","receivedAt":"2010-09-20T08:59:05Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Giuseppe Bilotta wrote:\n> On Mon, Sep 20, 2010 at 1:02 AM, Jakub Narebski wrote:\n> >\n> > When thinking about how display information about remotes and\n> > remote-tracking branches in 'summary' view, we have to consider that\n> > this view is meant to be what it says: a *summary*.  That is why both\n> > the 'heads' and the 'tags' section displays only 15 items.\n> >\n> > Without grouping remote heads the natural solution was to simply repeat\n> > what we used for 'heads' subsection, namely limit displaying to 15\n> > remote-tracking branches in 'remotes' subsection of the 'summary' view.\n> >\n> > With grouping it is more complicated.  We can limit number of remote\n> > head *per remote*, we can limit number of remotes... but what makes\n> > IMVHO least sense is limit number of remote heads *then* do grouping.\n> \n> This is something I absolutely agree with, BTW. It is the way it's\n> implemented currently because it was the quickest way to ship a\n> prototype out for discussion.\n\nAll right. Prototyping is good.\n\n> > The solution (1) i.e. limiting number of remote heads per remote, with\n> > or without limiting number of remotes behaves, as you wrote, most\n> > similarly to other components of 'summary' view.  On the other hand\n> > with large number of remotes, and large number of remote heads in those\n> > remotes it might be too large for a *summary* view.\n> \n> So you maintain that limiting the amount of data in summary view\n> should be primary wrt to limiting the amount of time?\n\nWell, what really affect gitweb performance is calling git commands, both\nbecause of fork overhead, and because it means disk access (and gitweb\nperformance from what I have heard is affected mainly by IO, and not CPU).\nWith grouping (displaying remotes) the difference between displaying\nremote-tracking branches (or information from them) and not displaying\nthem is an argument to git-for-each-ref. So I don't think it would \naffect performance much.\n\n> > The solution (3) i.e. displaying only list of remotes (perhaps limited\n> > to 15 remotes) is simple and fast to render.  On the other hand it offers\n> > least information and might be too little in the case of single remote.\n> \n> If time spent processing is not an issue, we can retrieve the number\n> of heads for each remote and display that, for example. Or even play\n> with some more dynamic stuff like making each group collapsible,\n> starting with it collapsed and then display the content when the user\n> hovers it with the mouse, for example.\n\nThe dynamic stuff is IMHO a good idea... provided we can either do it\nwithout JavaScript, or we can ensure that browser supports JavaScript\n(see current hack used for turning 'blame' into 'blame_incremental'\nview in gitweb).\n\nYet another solution would be to display only abbreviated list of remotes\nif its more of them than some threshold, and list remotes with abbreviated\nlist of remote-tracking branches if there are only a few remotes.\n \n> > > If we go with option 3, it does make sense to get all remote names and\n> > > all remote branches at once, and thus to make the git_get_remotes()\n> > > call do all of the work.\n> >\n> > Not if subroutine is called git_get_remotes(), IMVHO.  Perhaps\n> > git_group_remotes()?\n> \n> That I can do.\n\n> > If remote doesn't have remote-tracking branches associated with it\n> > (e.g. push-only remote, or remote predating separate remotes layout)\n> > the value would be undef for given remote as key in hash.\n> \n> Yes, this is something I have to take into consideration. Skip\n> displaying them is probably the best idea (unless we have other ways\n> to gather information about them).\n\nRight.\n\nP.S. It is not necessary for this series, but I think we should think\nabout \"single remote\" view... also because your code currently links\nto such views, which do not exist yet (remotes/ in path_info:\nhow it would be represented in CGI query format?).\n\n-- \nJakub Narebski\nPoland\n"},{"id":"151101","messageId":"AANLkTi=20gzw8DOuT=NWRqbt86AyJDEPxoo-V9A3Rvim@mail.gmail.com","threadId":"25122","inReplyTo":"201009201059.07742.jnareb@gmail.com","subject":"Re: [PATCH 7/7] gitweb: group remote heads","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-20T09:38:03Z","receivedAt":"2010-09-20T09:38:03Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Mon, Sep 20, 2010 at 10:59 AM, Jakub Narebski wrote:\n> Giuseppe Bilotta wrote:\n>> On Mon, Sep 20, 2010 at 1:02 AM, Jakub Narebski wrote:\n>> > The solution (1) i.e. limiting number of remote heads per remote, with\n>> > or without limiting number of remotes behaves, as you wrote, most\n>> > similarly to other components of 'summary' view.  On the other hand\n>> > with large number of remotes, and large number of remote heads in those\n>> > remotes it might be too large for a *summary* view.\n>>\n>> So you maintain that limiting the amount of data in summary view\n>> should be primary wrt to limiting the amount of time?\n>\n> Well, what really affect gitweb performance is calling git commands, both\n> because of fork overhead, and because it means disk access (and gitweb\n> performance from what I have heard is affected mainly by IO, and not CPU).\n> With grouping (displaying remotes) the difference between displaying\n> remote-tracking branches (or information from them) and not displaying\n> them is an argument to git-for-each-ref.  So I don't think it would\n> affect performance much.\n\nGetting the list of remote branches is, I would say, the most\nIO-intensive operation. I'm not sure how much I/O it would do though,\neven with a large number of remotes and heads. So maybe always gather\nall the information is the way to go.\n\n>> > The solution (3) i.e. displaying only list of remotes (perhaps limited\n>> > to 15 remotes) is simple and fast to render.  On the other hand it offers\n>> > least information and might be too little in the case of single remote.\n>>\n>> If time spent processing is not an issue, we can retrieve the number\n>> of heads for each remote and display that, for example. Or even play\n>> with some more dynamic stuff like making each group collapsible,\n>> starting with it collapsed and then display the content when the user\n>> hovers it with the mouse, for example.\n>\n> The dynamic stuff is IMHO a good idea... provided we can either do it\n> without JavaScript, or we can ensure that browser supports JavaScript\n> (see current hack used for turning 'blame' into 'blame_incremental'\n> view in gitweb).\n\nWhat I had in mind was something that is very easy to implement with CSS only.\n\n> Yet another solution would be to display only abbreviated list of remotes\n> if its more of them than some threshold, and list remotes with abbreviated\n> list of remote-tracking branches if there are only a few remotes.\n\nSo something like this:\n\n(1) if there are more than N remotes, only show N remote _names_ (no heads)\n(2) if there are no more than N remotes, show all remote names, each\nwith no more than M heads\n\n(with N and M to be decided, e.g. the usual 16)\n\n>> Yes, this is something I have to take into consideration. Skip\n>> displaying them is probably the best idea (unless we have other ways\n>> to gather information about them).\n>\n> Right.\n\nFor this, it would be nice to have `git remote show`, but even if I\nsent a patch to this effect gitweb should probably be left able to\ncope with older git versions not supporting it ...\n\n> P.S. It is not necessary for this series, but I think we should think\n> about \"single remote\" view... also because your code currently links\n> to such views, which do not exist yet (remotes/ in path_info:\n> how it would be represented in CGI query format?).\n\nMaybe pass the remote name as head parameter?\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"151283","messageId":"201009221034.10958.jnareb@gmail.com","threadId":"25122","inReplyTo":"AANLkTi=20gzw8DOuT=NWRqbt86AyJDEPxoo-V9A3Rvim@mail.gmail.com","subject":"Re: [PATCH 7/7] gitweb: group remote heads","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-09-22T08:34:10Z","receivedAt":"2010-09-22T08:34:10Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, Sep 20, 2010, Giuseppe Bilotta wrote:\n> On Mon, Sep 20, 2010 at 10:59 AM, Jakub Narebski wrote:\n>> Giuseppe Bilotta wrote:\n>>> On Mon, Sep 20, 2010 at 1:02 AM, Jakub Narebski wrote:\n\n>>>> The solution (1) i.e. limiting number of remote heads per remote, with\n>>>> or without limiting number of remotes behaves, as you wrote, most\n>>>> similarly to other components of 'summary' view.  On the other hand\n>>>> with large number of remotes, and large number of remote heads in those\n>>>> remotes it might be too large for a *summary* view.\n>>>\n>>> So you maintain that limiting the amount of data in summary view\n>>> should be primary wrt to limiting the amount of time?\n>>\n>> Well, what really affect gitweb performance is calling git commands, both\n>> because of fork overhead, and because it means disk access (and gitweb\n>> performance from what I have heard is affected mainly by IO, and not CPU).\n>> With grouping (displaying remotes) the difference between displaying\n>> remote-tracking branches (or information from them) and not displaying\n>> them is an argument to git-for-each-ref.  So I don't think it would\n>> affect performance much.\n> \n> Getting the list of remote branches is, I would say, the most\n> IO-intensive operation. I'm not sure how much I/O it would do though,\n> even with a large number of remotes and heads. So maybe always gather\n> all the information is the way to go.\n\nWell, that depends on the number of remote-tracking branches, and whether\nthose refs are packed (see git-pack-refs). But I agree that getting list\nof all remote branches might be I/O hit... though I am not sure if it\nwould be large enough to visibly affect gitweb performance.\n \nBy the way, besides the solution described below (list only remotes if\nthere are many of them), we could make 'remote_heads' be not simply\nboolean, but to be multiple-choice feature, configuring how 'remotes'\nview and remotes section in 'summary' page looks like. But that might\nbe overengineering.\n\n>>> If time spent processing is not an issue, we can retrieve the number\n>>> of heads for each remote and display that, for example. Or even play\n>>> with some more dynamic stuff like making each group collapsible,\n>>> starting with it collapsed and then display the content when the user\n>>> hovers it with the mouse, for example.\n>>\n>> The dynamic stuff is IMHO a good idea... provided we can either do it\n>> without JavaScript, or we can ensure that browser supports JavaScript\n>> (see current hack used for turning 'blame' into 'blame_incremental'\n>> view in gitweb).\n> \n> What I had in mind was something that is very easy to implement with\n> CSS only. \n\nDo I understand correctly that you would utilize ':hover' pseudoclass\nand 'display: none;' style? How ergonomic would this solution be?\n \n>> Yet another solution would be to display only abbreviated list of remotes\n>> if its more of them than some threshold, and list remotes with abbreviated\n>> list of remote-tracking branches if there are only a few remotes.\n> \n> So something like this:\n> \n> (1) if there are more than N remotes, only show N remote _names_ (no heads)\n> (2) if there are no more than N remotes, show all remote names, each\n> with no more than M heads\n> \n> (with N and M to be decided, e.g. the usual 16)\n\nRight.\n\n>>> Yes, this is something I have to take into consideration. Skip\n>>> displaying them is probably the best idea (unless we have other ways\n>>> to gather information about them).\n>>\n>> Right.\n> \n> For this, it would be nice to have `git remote show`, but even if I\n> sent a patch to this effect gitweb should probably be left able to\n> cope with older git versions not supporting it ...\n> \n>> P.S. It is not necessary for this series, but I think we should think\n>> about \"single remote\" view... also because your code currently links\n>> to such views, which do not exist yet (remotes/ in path_info:\n>> how it would be represented in CGI query format?).\n> \n> Maybe pass the remote name as head parameter?\n\nIn the meantime, while we don't have 'remote' view for a single remote\n(something like \"git remote show\"), it would be good if the header for\nindividual remotes didn't lead to \"'remotes/' action\".\n\n-- \nJakub Narebski\nPoland\n"},{"id":"151284","messageId":"AANLkTimn3EfW3ZAZWeAoq7Jv5c4Hu5LM0K7UeE41Si-Z@mail.gmail.com","threadId":"25122","inReplyTo":"201009221034.10958.jnareb@gmail.com","subject":"Re: [PATCH 7/7] gitweb: group remote heads","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2010-09-22T09:34:33Z","receivedAt":"2010-09-22T09:34:33Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Wed, Sep 22, 2010 at 10:34 AM, Jakub Narebski wrote:\n> On Mon, Sep 20, 2010, Giuseppe Bilotta wrote:\n>> On Mon, Sep 20, 2010 at 10:59 AM, Jakub Narebski wrote:\n>>> Giuseppe Bilotta wrote:\n>>>> On Mon, Sep 20, 2010 at 1:02 AM, Jakub Narebski wrote:\n>\n>>>>> The solution (1) i.e. limiting number of remote heads per remote, with\n>>>>> or without limiting number of remotes behaves, as you wrote, most\n>>>>> similarly to other components of 'summary' view.  On the other hand\n>>>>> with large number of remotes, and large number of remote heads in those\n>>>>> remotes it might be too large for a *summary* view.\n>>>>\n>>>> So you maintain that limiting the amount of data in summary view\n>>>> should be primary wrt to limiting the amount of time?\n>>>\n>>> Well, what really affect gitweb performance is calling git commands, both\n>>> because of fork overhead, and because it means disk access (and gitweb\n>>> performance from what I have heard is affected mainly by IO, and not CPU).\n>>> With grouping (displaying remotes) the difference between displaying\n>>> remote-tracking branches (or information from them) and not displaying\n>>> them is an argument to git-for-each-ref.  So I don't think it would\n>>> affect performance much.\n>>\n>> Getting the list of remote branches is, I would say, the most\n>> IO-intensive operation. I'm not sure how much I/O it would do though,\n>> even with a large number of remotes and heads. So maybe always gather\n>> all the information is the way to go.\n>\n> Well, that depends on the number of remote-tracking branches, and whether\n> those refs are packed (see git-pack-refs).  But I agree that getting list\n> of all remote branches might be I/O hit... though I am not sure if it\n> would be large enough to visibly affect gitweb performance.\n>\n> By the way, besides the solution described below (list only remotes if\n> there are many of them), we could make 'remote_heads' be not simply\n> boolean, but to be multiple-choice feature, configuring how 'remotes'\n> view and remotes section in 'summary' page looks like.  But that might\n> be overengineering.\n\nA config option for maximum number of remotes and maximum number of\nheads per remotes (in summary) with sane defaults (16/16?) maybe?\n\n>> What I had in mind was something that is very easy to implement with\n>> CSS only.\n>\n> Do I understand correctly that you would utilize ':hover' pseudoclass\n> and 'display: none;' style?  How ergonomic would this solution be?\n\nThat was my idea, yes. It _can_ get a little confusing without CSS3\ntransitions, due to the 'flash roll down' of the remote heads list. I\nwill try to cook up a preview sometime this week.\n\n>>> P.S. It is not necessary for this series, but I think we should think\n>>> about \"single remote\" view... also because your code currently links\n>>> to such views, which do not exist yet (remotes/ in path_info:\n>>> how it would be represented in CGI query format?).\n>>\n>> Maybe pass the remote name as head parameter?\n>\n> In the meantime, while we don't have 'remote' view for a single remote\n> (something like \"git remote show\"), it would be good if the header for\n> individual remotes didn't lead to \"'remotes/' action\".\n\nTrue. I'll fix that for the next rehash.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"}]}