[PATCH v2 00/11] gitweb: display remote heads

[PATCH v2 00/11] gitweb: display remote heads

[Giuseppe Bilotta]

This is a patchset I presented about a year ago or so, but after a lively
discussion it dropped into silence. I'm now presenting it again, with minor
cleanups and adjustements.

Giuseppe Bilotta (11):
  gitweb: introduce remote_heads feature
  gitweb: git_get_heads_list accepts an optional list of refs.
  gitweb: separate heads and remotes list in summary view
  gitweb: optional custom name for refs in git_heads_body
  gitweb: git_split_heads_body function.
  gitweb: use CSS to style split head lists.
  gitweb: add 'remotes' action
  gitweb: display HEAD in heads list when detached
  gitweb: git_is_head_detached() function
  gitweb: add HEAD to list of shortlog refs if detached
  gitweb: CSS style and refs mark for detached HEAD

 gitweb/gitweb.css  |   15 ++++++
 gitweb/gitweb.perl |  138 ++++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 143 insertions(+), 10 deletions(-)

[PATCH v2 01/11] gitweb: introduce remote_heads feature

[Giuseppe Bilotta]

With this feature enabled, remotes are retrieved (and displayed)
when getting (and displaying) the heads list.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |   31 +++++++++++++++++++++++++++++--
 1 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 933e137..b6c4233 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -327,6 +327,18 @@ our %feature = (
 	'ctags' => {
 		'override' => 0,
 		'default' => [0]},
+
+	# Make gitweb show remotes too in the heads list
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'remote_heads'}{'default'} = [1];
+	# To have project specific config enable override in $GITWEB_CONFIG
+	# $feature{'remote_heads'}{'override'} = 1;
+	# and in project config gitweb.remote_heads = 0|1;
+	'remote_heads' => {
+		'sub' => \&feature_remote_heads,
+		'override' => 0,
+		'default' => [0]},
 );
 
 sub gitweb_check_feature {
@@ -392,6 +404,18 @@ sub feature_pickaxe {
 	return ($_[0]);
 }
 
+sub feature_remote_heads {
+	my ($val) = git_get_project_config('remote_heads', '--bool');
+
+	if ($val eq 'true') {
+		return (1);
+	} elsif ($val eq 'false') {
+		return (0);
+	}
+
+	return ($_[0]);
+}
+
 # checking HEAD file with -e is fragile if the repository was
 # initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed
 # and then pruned.
@@ -2642,10 +2666,12 @@ sub git_get_heads_list {
 	my $limit = shift;
 	my @headslist;
 
+	my ($remote_heads) = gitweb_check_feature('remote_heads');
+
 	open my $fd, '-|', git_cmd(), 'for-each-ref',
 		($limit ? '--count='.($limit+1) : ()), '--sort=-committerdate',
 		'--format=%(objectname) %(refname) %(subject)%00%(committer)',
-		'refs/heads'
+		'refs/heads', ( $remote_heads ? 'refs/remotes' : '')
 		or return;
 	while (my $line = <$fd>) {
 		my %ref_item;
@@ -2656,8 +2682,9 @@ sub git_get_heads_list {
 		my ($committer, $epoch, $tz) =
 			($committerinfo =~ /^(.*) ([0-9]+) (.*)$/);
 		$ref_item{'fullname'}  = $name;
-		$name =~ s!^refs/heads/!!;
+		$name =~ s!^refs/(head|remote)s/!!;
 
+		$ref_item{'class'} = $1;
 		$ref_item{'name'}  = $name;
 		$ref_item{'id'}    = $hash;
 		$ref_item{'title'} = $title || '(no commit message)';
-- 
1.5.6.5

[PATCH v2 02/11] gitweb: git_get_heads_list accepts an optional list of refs.

[Giuseppe Bilotta]

git_get_heads_list(limit, dir1, dir2, ...) can now be used to retrieve
refs/dir1, refs/dir2 etc. Defaults to ('heads') or ('heads', 'remotes')
depending on the remote_heads option.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index b6c4233..d7c97a3 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2663,15 +2663,18 @@ sub parse_from_to_diffinfo {
 ## parse to array of hashes functions
 
 sub git_get_heads_list {
-	my $limit = shift;
+	my ($limit, @class) = @_;
+	unless (defined @class) {
+		my ($remote_heads) = gitweb_check_feature('remote_heads');
+		@class = ('heads', $remote_heads ? 'remotes' : undef);
+	}
+	my @refs = map { "refs/$_" } @class;
 	my @headslist;
 
-	my ($remote_heads) = gitweb_check_feature('remote_heads');
-
 	open my $fd, '-|', git_cmd(), 'for-each-ref',
 		($limit ? '--count='.($limit+1) : ()), '--sort=-committerdate',
 		'--format=%(objectname) %(refname) %(subject)%00%(committer)',
-		'refs/heads', ( $remote_heads ? 'refs/remotes' : '')
+		@refs
 		or return;
 	while (my $line = <$fd>) {
 		my %ref_item;
-- 
1.5.6.5

[PATCH v2 03/11] gitweb: separate heads and remotes list in summary view

[Giuseppe Bilotta]

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index d7c97a3..ab29aec 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -4449,6 +4449,7 @@ sub git_summary {
 	my %co = parse_commit("HEAD");
 	my %cd = %co ? parse_date($co{'committer_epoch'}, $co{'committer_tz'}) : ();
 	my $head = $co{'id'};
+	my ($remote_heads) = gitweb_check_feature('remote_heads');
 
 	my $owner = git_get_project_owner($project);
 
@@ -4456,7 +4457,8 @@ sub git_summary {
 	# These get_*_list functions return one more to allow us to see if
 	# there are more ...
 	my @taglist  = git_get_tags_list(16);
-	my @headlist = git_get_heads_list(16);
+	my @headlist = git_get_heads_list(16, 'heads');
+	my @remotelist = $remote_heads ? git_get_heads_list(16, 'remotes') : ();
 	my @forklist;
 	my ($check_forks) = gitweb_check_feature('forks');
 
@@ -4535,6 +4537,13 @@ sub git_summary {
 		               $cgi->a({-href => href(action=>"heads")}, "..."));
 	}
 
+	if (@remotelist) {
+		git_print_header_div('remotes');
+		git_heads_body(\@remotelist, $head, 0, 15,
+		               $#remotelist <= 15 ? undef :
+		               $cgi->a({-href => href(action=>"heads")}, "..."));
+	}
+
 	if (@forklist) {
 		git_print_header_div('forks');
 		git_project_list_body(\@forklist, 'age', 0, 15,
-- 
1.5.6.5

[PATCH v2 04/11] gitweb: optional custom name for refs in git_heads_body

[Giuseppe Bilotta]

We make a clear separation between the hash reference and the displayed
name for refs displayed by git_heads_body. This can be used e.g. to
group them and display only the distinct part of the name.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index ab29aec..a736f2a 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -4288,16 +4288,18 @@ sub git_heads_body {
 		} else {
 			print "<tr class=\"light\">\n";
 		}
+		my $hname = $ref{'hname'} || $ref{'fullname'} || $ref{'name'};
+		my $name = $ref{'name'};
 		$alternate ^= 1;
 		print "<td><i>$ref{'age'}</i></td>\n" .
 		      ($curr ? "<td class=\"current_head\">" : "<td>") .
-		      $cgi->a({-href => href(action=>"shortlog", hash=>$ref{'fullname'}),
-		               -class => "list name"},esc_html($ref{'name'})) .
+		      $cgi->a({-href => href(action=>"shortlog", hash=>$hname),
+		               -class => "list name"},esc_html($name)) .
 		      "</td>\n" .
 		      "<td class=\"link\">" .
-		      $cgi->a({-href => href(action=>"shortlog", hash=>$ref{'fullname'})}, "shortlog") . " | " .
-		      $cgi->a({-href => href(action=>"log", hash=>$ref{'fullname'})}, "log") . " | " .
-		      $cgi->a({-href => href(action=>"tree", hash=>$ref{'fullname'}, hash_base=>$ref{'name'})}, "tree") .
+		      $cgi->a({-href => href(action=>"shortlog", hash=>$hname)}, "shortlog") . " | " .
+		      $cgi->a({-href => href(action=>"log", hash=>$hname)}, "log") . " | " .
+		      $cgi->a({-href => href(action=>"tree", hash=>$hname, hash_base=>$hname)}, "tree") .
 		      "</td>\n" .
 		      "</tr>";
 	}
-- 
1.5.6.5

[PATCH v2 05/11] gitweb: git_split_heads_body function.

[Giuseppe Bilotta]

The purpose of this function is to split a headlist into groups
determined by the leading part of the refname, and call git_heads_body()
on each group.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |   33 ++++++++++++++++++++++++++++++++-
 1 files changed, 32 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index a736f2a..836b6ba 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -4271,6 +4271,37 @@ sub git_tags_body {
 	print "</table>\n";
 }
 
+sub git_split_heads_body {
+	my ($headlist, $head, $from, $to, $extra) = @_;
+	my %headlists;
+	my $leader; my $list; my @list;
+
+	# Split @$headlist into a hash of lists
+	map {
+		my %ref = %$_;
+		$ref{'hname'} = $ref{'name'};
+		if ($ref{'name'} =~ /\//) {
+			$ref{'name'} =~ s!^([^/]+)/!!;
+			$leader = $1;
+		} else {
+			$leader = "\000";
+		}
+		if (defined $headlists{$leader}) {
+			@list = @{$headlists{$leader}}
+		} else {
+			@list = ()
+		}
+		push @list, \%ref;
+		$headlists{$leader} = [@list];
+	} @$headlist;
+
+	foreach $leader (sort(keys %headlists)) {
+		print "<b>$leader</b><br/>\n" unless $leader eq "\000";
+		$list = $headlists{$leader};
+		git_heads_body($list, $head, $from, $to, $extra);
+	}
+}
+
 sub git_heads_body {
 	# uses global variable $project
 	my ($headlist, $head, $from, $to, $extra) = @_;
@@ -4541,7 +4572,7 @@ sub git_summary {
 
 	if (@remotelist) {
 		git_print_header_div('remotes');
-		git_heads_body(\@remotelist, $head, 0, 15,
+		git_split_heads_body(\@remotelist, $head, 0, 15,
 		               $#remotelist <= 15 ? undef :
 		               $cgi->a({-href => href(action=>"heads")}, "..."));
 	}
-- 
1.5.6.5

[PATCH v2 06/11] gitweb: use CSS to style split head lists.

[Giuseppe Bilotta]

Introduce a new div class 'subsection' in the CSS and use it to style
split head lists.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.css  |   10 ++++++++++
 gitweb/gitweb.perl |    4 +++-
 2 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index a01eac8..751749f 100644
--- a/gitweb/gitweb.css
+++ b/gitweb/gitweb.css
@@ -8,6 +8,16 @@ body {
 	color: #000000;
 }
 
+div.subsection {
+	border: solid #d9d8d1;
+	border-width: 1px;
+	margin: 10px;
+}
+
+.subsection .title {
+	font-size: smaller;
+}
+
 a {
 	color: #0000cc;
 }
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 836b6ba..03e0b21 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -4296,9 +4296,11 @@ sub git_split_heads_body {
 	} @$headlist;
 
 	foreach $leader (sort(keys %headlists)) {
-		print "<b>$leader</b><br/>\n" unless $leader eq "\000";
+		print "<div class=\"subsection\">\n";
+		git_print_header_div(undef, $leader) unless $leader eq "\000";
 		$list = $headlists{$leader};
 		git_heads_body($list, $head, $from, $to, $extra);
+		print "</div>\n";
 	}
 }
 
-- 
1.5.6.5

[PATCH v2 07/11] gitweb: add 'remotes' action

[Giuseppe Bilotta]

This action is similar to the 'heads' action, but it displays
remote heads, grouped by remote repository.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 03e0b21..09728cb 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -507,6 +507,7 @@ our %actions = (
 	"commit" => \&git_commit,
 	"forks" => \&git_forks,
 	"heads" => \&git_heads,
+	"remotes" => \&git_remotes,
 	"history" => \&git_history,
 	"log" => \&git_log,
 	"rss" => \&git_rss,
@@ -4755,13 +4756,26 @@ sub git_heads {
 	git_print_page_nav('','', $head,undef,$head);
 	git_print_header_div('summary', $project);
 
-	my @headslist = git_get_heads_list();
+	my @headslist = git_get_heads_list(undef, 'heads');
 	if (@headslist) {
 		git_heads_body(\@headslist, $head);
 	}
 	git_footer_html();
 }
 
+sub git_remotes {
+	my $head = git_get_head_hash($project);
+	git_header_html();
+	git_print_page_nav('','', $head,undef,$head);
+	git_print_header_div('summary', $project . ' remotes');
+
+	my @headslist = git_get_heads_list(undef, 'remotes');
+	if (@headslist) {
+		git_split_heads_body(\@headslist, $head);
+	}
+	git_footer_html();
+}
+
 sub git_blob_plain {
 	my $type = shift;
 	my $expires;
-- 
1.5.6.5

[PATCH v2 08/11] gitweb: display HEAD in heads list when detached

[Giuseppe Bilotta]

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 09728cb..a168f6f 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2672,6 +2672,27 @@ sub git_get_heads_list {
 	my @refs = map { "refs/$_" } @class;
 	my @headslist;
 
+	if (grep { $_ eq 'heads' } @class) {
+		my @x = (git_cmd(), 'branch');
+		my @ret = split("\n", qx(@x));
+		if (grep { /^\* \(no branch\)$/ } @ret) { ;
+			my %ref_item;
+			@x = (git_cmd(), 'log', '-1', '--pretty=format:%H%n%ct%n%s');
+			my ($hash, $epoch, $title) = split("\n", qx(@x), 3);
+
+			$ref_item{'class'} = 'head';
+			$ref_item{'name'} = 'HEAD';
+			$ref_item{'id'} = $hash;
+			$ref_item{'title'} = $title || '(no commit message)';
+			if ($ref_item{'epoch'} = $epoch) {
+				$ref_item{'age'} = age_string(time - $ref_item{'epoch'});
+			} else {
+				$ref_item{'age'} = "unknown";
+			}
+			push @headslist, \%ref_item;
+		}
+	}
+
 	open my $fd, '-|', git_cmd(), 'for-each-ref',
 		($limit ? '--count='.($limit+1) : ()), '--sort=-committerdate',
 		'--format=%(objectname) %(refname) %(subject)%00%(committer)',
-- 
1.5.6.5

[PATCH v2 09/11] gitweb: git_is_head_detached() function

[Giuseppe Bilotta]

The function checks if the HEAD for the current project is detached by
checking if 'git branch' returns "* (no branch)"

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index a168f6f..ceb0271 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1844,6 +1844,13 @@ sub git_get_head_hash {
 	return $retval;
 }
 
+# check if current HEAD is detached
+sub git_is_head_detached {
+	my @x = (git_cmd(), 'branch');
+	my @ret = split("\n", qx(@x));
+	return 0 + grep { /^\* \(no branch\)$/ } @ret;
+}
+
 # get type of given object
 sub git_get_type {
 	my $hash = shift;
@@ -2673,11 +2680,9 @@ sub git_get_heads_list {
 	my @headslist;
 
 	if (grep { $_ eq 'heads' } @class) {
-		my @x = (git_cmd(), 'branch');
-		my @ret = split("\n", qx(@x));
-		if (grep { /^\* \(no branch\)$/ } @ret) { ;
+		if (git_is_head_detached()) {
 			my %ref_item;
-			@x = (git_cmd(), 'log', '-1', '--pretty=format:%H%n%ct%n%s');
+			my @x = (git_cmd(), 'log', '-1', '--pretty=format:%H%n%ct%n%s');
 			my ($hash, $epoch, $title) = split("\n", qx(@x), 3);
 
 			$ref_item{'class'} = 'head';
-- 
1.5.6.5

[PATCH v2 10/11] gitweb: add HEAD to list of shortlog refs if detached

[Giuseppe Bilotta]

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index ceb0271..256c962 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2296,6 +2296,10 @@ sub git_get_last_activity {
 sub git_get_references {
 	my $type = shift || "";
 	my %refs;
+	if (git_is_head_detached()) {
+		my $hash = git_get_head_hash($project);
+		$refs{$hash} = [ 'HEAD' ];
+	}
 	# 5dc01c595e6c6ec9ccda4f6f69c131c0dd945f8c refs/tags/v2.6.11
 	# c39ae07f393806ccf406ef966e9a15afc43cc36a refs/tags/v2.6.11^{}
 	open my $fd, "-|", git_cmd(), "show-ref", "--dereference",
-- 
1.5.6.5

[PATCH v2 11/11] gitweb: CSS style and refs mark for detached HEAD

[Giuseppe Bilotta]

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.css  |    5 +++++
 gitweb/gitweb.perl |    2 +-
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index 751749f..c0c4540 100644
--- a/gitweb/gitweb.css
+++ b/gitweb/gitweb.css
@@ -523,6 +523,11 @@ span.refs span.head {
 	border-color: #ccffcc #00cc33 #00cc33 #ccffcc;
 }
 
+span.refs span.detached {
+	background-color: #ffaaaa;
+	border-color: #ccffcc #00cc33 #00cc33 #ccffcc;
+}
+
 span.atnight {
 	color: #cc0000;
 }
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 256c962..51e133d 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2298,7 +2298,7 @@ sub git_get_references {
 	my %refs;
 	if (git_is_head_detached()) {
 		my $hash = git_get_head_hash($project);
-		$refs{$hash} = [ 'HEAD' ];
+		$refs{$hash} = [ 'detached/HEAD' ];
 	}
 	# 5dc01c595e6c6ec9ccda4f6f69c131c0dd945f8c refs/tags/v2.6.11
 	# c39ae07f393806ccf406ef966e9a15afc43cc36a refs/tags/v2.6.11^{}
-- 
1.5.6.5

Re: [PATCH v2 11/11] gitweb: CSS style and refs mark for detached HEAD

[Jakub Narebski]

On Fri, 14 Nov 2008, Giuseppe Bilotta wrote:

> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.css  |    5 +++++
>  gitweb/gitweb.perl |    2 +-
>  2 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
> index 751749f..c0c4540 100644
> --- a/gitweb/gitweb.css
> +++ b/gitweb/gitweb.css
> @@ -523,6 +523,11 @@ span.refs span.head {
>  	border-color: #ccffcc #00cc33 #00cc33 #ccffcc;
>  }
>  
> +span.refs span.detached {
> +	background-color: #ffaaaa;
> +	border-color: #ccffcc #00cc33 #00cc33 #ccffcc;
> +}

Nice and good. I think.

> +
>  span.atnight {
>  	color: #cc0000;
>  }
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 256c962..51e133d 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2298,7 +2298,7 @@ sub git_get_references {
>  	my %refs;
>  	if (git_is_head_detached()) {
>  		my $hash = git_get_head_hash($project);
> -		$refs{$hash} = [ 'HEAD' ];
> +		$refs{$hash} = [ 'detached/HEAD' ];

This, not so much.

Please, if you use some _*hack*_, and adding 'detached/' prefix is
certainly hack (you could have modified format_ref_marker() instead,
and that is probably better solution), you really should explain it
in detail in commit message.  Otherwise some time later somebody would
examine the code, say 'WTF?!?'... and even git-blame would not help ;-/

>  	}
>  	# 5dc01c595e6c6ec9ccda4f6f69c131c0dd945f8c refs/tags/v2.6.11
>  	# c39ae07f393806ccf406ef966e9a15afc43cc36a refs/tags/v2.6.11^{}
> -- 
> 1.5.6.5
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCH v2 10/11] gitweb: add HEAD to list of shortlog refs if detached

[Jakub Narebski]

On Fri, 14 Nov 2008, Giuseppe Bilotta wrote:

> Subject: [PATCH v2 10/11] gitweb: add HEAD to list of shortlog refs if detached

Errr... what?!?

It is not "list of shortlog refs", it is list of _ref markers_.

> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index ceb0271..256c962 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2296,6 +2296,10 @@ sub git_get_last_activity {
>  sub git_get_references {
>  	my $type = shift || "";
>  	my %refs;
> +	if (git_is_head_detached()) {
> +		my $hash = git_get_head_hash($project);

+	if ($hash = git_is_head_detached()) {

if using provided (in response to previous patch) implementation, but
even that can be simplified out by using "git show-ref -h ...";
see below

> +		$refs{$hash} = [ 'HEAD' ];
> +	}

Overly complicated. The '-h'/'--head' option to git-show-ref is there
for a reason.

>  	# 5dc01c595e6c6ec9ccda4f6f69c131c0dd945f8c refs/tags/v2.6.11
>  	# c39ae07f393806ccf406ef966e9a15afc43cc36a refs/tags/v2.6.11^{}
>  	open my $fd, "-|", git_cmd(), "show-ref", "--dereference",
>		($type ? ("--", "refs/$type") : ()) # use -- <pattern> if $type

-		($type ? ("--", "refs/$type") : ()) # use -- <pattern> if $type
+		($type ? ("--", "refs/$type") : ('-h')) # use -- <pattern> if $type

>		or return;

This is I think simpler.

-- 
Jakub Narebski
Poland

Re: [PATCH v2 09/11] gitweb: git_is_head_detached() function

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> The function checks if the HEAD for the current project is detached by
> checking if 'git branch' returns "* (no branch)"

This one looks more like "oops, the way detached HEAD is detected in 08 is
sucky, let's cover it up by introducing a function as an afterthought."

Have a patch that introduces git_is_head_detached() first, and then use
that function to implement the feature.  I personally think the user (that
is, 08/11) is small and isolated enough that these two can be a single
patch.

> +# check if current HEAD is detached
> +sub git_is_head_detached {
> +	my @x = (git_cmd(), 'branch');
> +	my @ret = split("\n", qx(@x));
> +	return 0 + grep { /^\* \(no branch\)$/ } @ret;
> +}

Do not read from Porcelain in scripts.

"git symbolic-ref HEAD" should error out when your HEAD is detached, and
will return refs/heads/frotz when you are on frotz branch.

But realistically speaking, what does it mean to have a detached HEAD in a
repository published via gitweb?  First of all these things are supposed
to be bare and there would be no checkout.

Re: [PATCH v2 09/11] gitweb: git_is_head_detached() function

[Giuseppe Bilotta]

On Fri, Nov 14, 2008 at 7:40 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> The function checks if the HEAD for the current project is detached by
>> checking if 'git branch' returns "* (no branch)"
>
> This one looks more like "oops, the way detached HEAD is detected in 08 is
> sucky, let's cover it up by introducing a function as an afterthought."

Half and half: it _is_ an afterthought, and although it's sucky that's
not the reason why I decided to refactor it 8-)

> Have a patch that introduces git_is_head_detached() first, and then use
> that function to implement the feature.  I personally think the user (that
> is, 08/11) is small and isolated enough that these two can be a single
> patch.

Will do. I did some patch squashing while preparing this set, but I
forgot to do this one.

>> +# check if current HEAD is detached
>> +sub git_is_head_detached {
>> +     my @x = (git_cmd(), 'branch');
>> +     my @ret = split("\n", qx(@x));
>> +     return 0 + grep { /^\* \(no branch\)$/ } @ret;
>> +}
>
> Do not read from Porcelain in scripts.
>
> "git symbolic-ref HEAD" should error out when your HEAD is detached, and
> will return refs/heads/frotz when you are on frotz branch.

Eh, I was, like, 100% sure this was the wrong way to do it, but I
didn't have any idea on how to do it using the plumbing. Thanks, I'll
look into that.

> But realistically speaking, what does it mean to have a detached HEAD in a
> repository published via gitweb?  First of all these things are supposed
> to be bare and there would be no checkout.

You know what's funny, when I first started working on the 'show
remote branches in gitweb' patchset, you had a similar objection, but
as you yourself pointed out

> But obviously people use gitweb/instaweb as a way to view their
> own live repository, and I think it makes sense to show and
> support remotes/ in such a case.  It also would make sense to
> support detached HEAD there as well.

http://kerneltrap.org/mailarchive/git/2007/8/30/256411

and that's exactly the reason why I added the part about the detached head 8-)

I have been thinking about making this detached HEAD thing an
additional option, but it _really_ seemed like overkill.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH v2 09/11] gitweb: git_is_head_detached() function

[Junio C Hamano]

"Giuseppe Bilotta" <giuseppe.bilotta@gmail.com> writes:

> I have been thinking about making this detached HEAD thing an
> additional option, but it _really_ seemed like overkill.

I agree that it does not make much sense to make this feature an option.
Detaching the HEAD in the repository itself is an enough clue from the
user to the code that the user wants to trigger the feature.

Thanks.

Re: [PATCH v2 09/11] gitweb: git_is_head_detached() function

[Nanako Shiraishi]

Quoting Junio C Hamano <gitster@pobox.com>:

> "Giuseppe Bilotta" <giuseppe.bilotta@gmail.com> writes:
>
>> I have been thinking about making this detached HEAD thing an
>> additional option, but it _really_ seemed like overkill.
>
> I agree that it does not make much sense to make this feature an option.
> Detaching the HEAD in the repository itself is an enough clue from the
> user to the code that the user wants to trigger the feature.

Shouldn't the feature to show remote tracking branches also be unconditionally active, then?

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Re: [PATCH v2 09/11] gitweb: git_is_head_detached() function

[Jakub Narebski]

On Thu, 13 Nov 2008, Giuseppe Bilotta wrote:

> The function checks if the HEAD for the current project is detached by
> checking if 'git branch' returns "* (no branch)"
> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

In my opinion this patch should really be squashed together with
previous one. They belong together.

> ---
>  gitweb/gitweb.perl |   13 +++++++++----
>  1 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index a168f6f..ceb0271 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1844,6 +1844,13 @@ sub git_get_head_hash {
>  	return $retval;
>  }
>  
> +# check if current HEAD is detached
> +sub git_is_head_detached {
> +	my @x = (git_cmd(), 'branch');
> +	my @ret = split("\n", qx(@x));
> +	return 0 + grep { /^\* \(no branch\)$/ } @ret;
> +}

First, not git-branch. Second, you can use Perl-only solution:

+# check if current HEAD is detached
+sub git_is_head_detached {
+	my $head_file = "$project/HEAD";
+	return if -l $head_file; # symlink
+	open my $fd, '<', $head_file
+		or return;
+	my $head_hash = <$fd>;
+	close $fd;
+	return if $head_hash =~ /^ref: /;
+	return $head_hash;
+}

Alternate solution would be to create git_get_symbolic_ref, and use
"!defined $current_branch" in place of "git_is_head_detached()".

> +
>  # get type of given object
>  sub git_get_type {
>  	my $hash = shift;
> @@ -2673,11 +2680,9 @@ sub git_get_heads_list {
>  	my @headslist;
>  
>  	if (grep { $_ eq 'heads' } @class) {
> -		my @x = (git_cmd(), 'branch');
> -		my @ret = split("\n", qx(@x));
> -		if (grep { /^\* \(no branch\)$/ } @ret) { ;
> +		if (git_is_head_detached()) {
>  			my %ref_item;
> -			@x = (git_cmd(), 'log', '-1', '--pretty=format:%H%n%ct%n%s');
> +			my @x = (git_cmd(), 'log', '-1', '--pretty=format:%H%n%ct%n%s');

Hmmm... git-log, git-show, or perhaps parse_commit?

>  			my ($hash, $epoch, $title) = split("\n", qx(@x), 3);
>  
>  			$ref_item{'class'} = 'head';
> -- 
> 1.5.6.5
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCH v2 08/11] gitweb: display HEAD in heads list when detached

[Jakub Narebski]

On Thu, 13 Nov 2008, Giuseppe Bilotta wrote:

First, I think that _at least_ the first two patches dealing with
detached should be squashed.

Second, not this way!


But I think that support for detached HEAD (I am not sure if it should
have to be explicitly turned on using some %feature, or reusing some
existing feature like 'remote_heads') is a very good idea. Especially
for git-instaweb.

> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl |   21 +++++++++++++++++++++
>  1 files changed, 21 insertions(+), 0 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 09728cb..a168f6f 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2672,6 +2672,27 @@ sub git_get_heads_list {
>  	my @refs = map { "refs/$_" } @class;
>  	my @headslist;
>  
> +	if (grep { $_ eq 'heads' } @class) {

First, IMHO layering violation. Resolving detached HEAD should not be
in my opinion left to git_get_heads_list, which is wrapped around 
git-for-each-ref, which for some reason (contrary for example to
"git ls-remote ." or "git show-ref -h") doesn't show HEAD even if it
is detached. Probably misfeature / a bug in git-for-each-ref. I guess
that we should resolve detached HEAD in caller.

But I am not sure about this decision. Maybe instead of showing
detached HEAD (if it is detached) for 'heads', we should show it if
there is 'HEAD' in @class (well, @refs would have to be corrected,
too)?

> +		my @x = (git_cmd(), 'branch');
> +		my @ret = split("\n", qx(@x));

                                      ^^^^^^^- bit strange

Especially compared to almost everywhere else using open ... "-|"

> +		if (grep { /^\* \(no branch\)$/ } @ret) { ;

                                                          ^
                            WTF? -------------------------|

Second, if we go the route of manually resolving detached HEAD, 
instead of adding [-h|--head] (from git-show-ref) option equivalent
to git-for-each-ref, which would work only for detached HEAD (fixing
kind of a bug), this is *not* the way to do it.

A. It should be done using encapsulation, adding is_HEAD_detached() or
git_is_head_detached() subroutine (squashing the next patch), or
simply using !defined($current_branch), where $current_branch would
be set using git_get_symbolic_ref('HEAD') or something.

B. Using porcelain, especially end-user porcelain such as git-branch,
which can change its output format (because they are porcelain). Use
equivalent plumbing, be if git-symbolic-ref ("git symbolic-ref -q HEAD"
to be more exact) to get current branch name[1], or just simply do
that in Perl: check if it is symlink, or if it starts with "ref: "
if it is regular file (IIRC HEAD, even detached HEAD, cannot get
packed into .git/packed-refs and deleted... at least I think so).

[1] This means that we have better way of detecting (and showing)
which branch is current one than comparing sha1 with resolved HEAD.
($head_hash).

> +			my %ref_item;
> +			@x = (git_cmd(), 'log', '-1', '--pretty=format:%H%n%ct%n%s');
> +			my ($hash, $epoch, $title) = split("\n", qx(@x), 3);

Errr... if we don't fix git-for-each-ref, and go that route, why not
simply use parse_commit subroutine, and extract relevant info from
there, instead of handcrafting git-log (why not git-show?) call?

You get more info than needed, but I think the cost of getting it is
almost the same, and you can reuse existing code.

And if we go --pretty=format:<...> or --pretty=tformat:<...> route for
git-log, git-rev-list or git-show, wouldn't it be possible to generate
the same output format as git-for-each-ref below?

> +
> +			$ref_item{'class'} = 'head';
> +			$ref_item{'name'} = 'HEAD';
> +			$ref_item{'id'} = $hash;
> +			$ref_item{'title'} = $title || '(no commit message)';
> +			if ($ref_item{'epoch'} = $epoch) {
> +				$ref_item{'age'} = age_string(time - $ref_item{'epoch'});

Hmmm...

> +			} else {
> +				$ref_item{'age'} = "unknown";
> +			}
> +			push @headslist, \%ref_item;
> +		}
> +	}
> +
>  	open my $fd, '-|', git_cmd(), 'for-each-ref',
>  		($limit ? '--count='.($limit+1) : ()), '--sort=-committerdate',
>  		'--format=%(objectname) %(refname) %(subject)%00%(committer)',
> -- 
> 1.5.6.5
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCH v2 07/11] gitweb: add 'remotes' action

[Jakub Narebski]

On Thu, 13 Nov 2008, Giuseppe "Oblomov" Bilotta wrote:

> This action is similar to the 'heads' action, but it displays
> remote heads, grouped by remote repository.

I think I would prefer would go together with the change that split
the 'heads' ('branches') part of summary view into 'heads' and
'remotes', so that both section title header, and '...' continuation
if present, lead to proper view.

So either

  [heads]  # or [branches]
  master
  to-submit
  origin/master
  origin/next
  ...

where both '[heads]' and (possibly) '...' link to 'heads' view showing
_both_ local branches (refs/heads/*) and remote-tracking branches
(refs/remotes/*), like in first patch of series (perhaps with some
subdivision).

Or

  [heads]
  master
  to-submit
  ...
  [remotes]
  origin/master
  origin/next
  ...

where '[heads]' link to 'heads' view which shows only local branches
(refs/heads/*), and '[remotes]' link to 'remotes' view which shows only
remote-tracking branches.
 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl |   16 +++++++++++++++-
>  1 files changed, 15 insertions(+), 1 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 03e0b21..09728cb 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -507,6 +507,7 @@ our %actions = (
>  	"commit" => \&git_commit,
>  	"forks" => \&git_forks,
>  	"heads" => \&git_heads,
> +	"remotes" => \&git_remotes,
>  	"history" => \&git_history,
>  	"log" => \&git_log,
>  	"rss" => \&git_rss,
> @@ -4755,13 +4756,26 @@ sub git_heads {
>  	git_print_page_nav('','', $head,undef,$head);
>  	git_print_header_div('summary', $project);
>  
> -	my @headslist = git_get_heads_list();
> +	my @headslist = git_get_heads_list(undef, 'heads');

Hmmm... I wonder if it would be possible to use some DWIM-mery on
the side of git_get_heads_list (for example checking if first argument
is a number, and assuming that nobody would be insane enough to use
refs/15 for namespace), and just use git_get_heads_list('heads') here.

But I guess that this form is good enough...

>  	if (@headslist) {
>  		git_heads_body(\@headslist, $head);
>  	}
>  	git_footer_html();
>  }
>  
> +sub git_remotes {
> +	my $head = git_get_head_hash($project);
> +	git_header_html();
> +	git_print_page_nav('','', $head,undef,$head);
> +	git_print_header_div('summary', $project . ' remotes');
> +
> +	my @headslist = git_get_heads_list(undef, 'remotes');
> +	if (@headslist) {
> +		git_split_heads_body(\@headslist, $head);
> +	}
> +	git_footer_html();
> +}

Nice. I see the difference from git_heads is using $project . ' remotes'
in place of $project in git_print_header_div() (why?), and using 
'remotes' in call to git_get_heads_list().

> +
>  sub git_blob_plain {
>  	my $type = shift;
>  	my $expires;
> -- 
> 1.5.6.5
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCH v2 07/11] gitweb: add 'remotes' action

[Giuseppe Bilotta]

On Sat, Nov 15, 2008 at 1:16 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Thu, 13 Nov 2008, Giuseppe "Oblomov" Bilotta wrote:
>
>> This action is similar to the 'heads' action, but it displays
>> remote heads, grouped by remote repository.
>
> I think I would prefer would go together with the change that split
> the 'heads' ('branches') part of summary view into 'heads' and
> 'remotes', so that both section title header, and '...' continuation
> if present, lead to proper view.
>
> So either
>
>  [heads]  # or [branches]
>  master
>  to-submit
>  origin/master
>  origin/next
>  ...
>
> where both '[heads]' and (possibly) '...' link to 'heads' view showing
> _both_ local branches (refs/heads/*) and remote-tracking branches
> (refs/remotes/*), like in first patch of series (perhaps with some
> subdivision).
>
> Or
>
>  [heads]
>  master
>  to-submit
>  ...
>  [remotes]
>  origin/master
>  origin/next
>  ...
>
> where '[heads]' link to 'heads' view which shows only local branches
> (refs/heads/*), and '[remotes]' link to 'remotes' view which shows only
> remote-tracking branches.

That's funny, I just squashed this patch with the summary list split
view patch 8-) I'm going for the second option, to have [heads] link
to heads which only lists local heads, and [remotes] linking to
remotes that lists the remotes. We may or may not want to rather have
[branches] instead of [heads], and keep the heads action to mean *all*
heads, local and remote, but I'm not sure about it.

>> -     my @headslist = git_get_heads_list();
>> +     my @headslist = git_get_heads_list(undef, 'heads');
>
> Hmmm... I wonder if it would be possible to use some DWIM-mery on
> the side of git_get_heads_list (for example checking if first argument
> is a number, and assuming that nobody would be insane enough to use
> refs/15 for namespace), and just use git_get_heads_list('heads') here.
>
> But I guess that this form is good enough...

I've been wondering about this myself. Another possibility would be to
use named options instead of positional parameters, but then again it
all looks like overkill, at least for the time being.

>>       if (@headslist) {
>>               git_heads_body(\@headslist, $head);
>>       }
>>       git_footer_html();
>>  }
>>
>> +sub git_remotes {
>> +     my $head = git_get_head_hash($project);
>> +     git_header_html();
>> +     git_print_page_nav('','', $head,undef,$head);
>> +     git_print_header_div('summary', $project . ' remotes');
>> +
>> +     my @headslist = git_get_heads_list(undef, 'remotes');
>> +     if (@headslist) {
>> +             git_split_heads_body(\@headslist, $head);
>> +     }
>> +     git_footer_html();
>> +}
>
> Nice. I see the difference from git_heads is using $project . ' remotes'
> in place of $project in git_print_header_div() (why?),

FWIW, I decided to scratch that additional ' remotes' string when
squashing this patch.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH v2 07/11] gitweb: add 'remotes' action

[Jakub Narebski]

On Sat, 15 Nov 2008, Giuseppe Bilotta wrote:
> On Sat, Nov 15, 2008 at 1:16 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>> On Thu, 13 Nov 2008, Giuseppe "Oblomov" Bilotta wrote:
>>
>>> This action is similar to the 'heads' action, but it displays
>>> remote heads, grouped by remote repository.
>>
>> I think I would prefer would go together with the change that split
>> the 'heads' ('branches') part of summary view into 'heads' and
>> 'remotes', so that both section title header, and '...' continuation
>> if present, lead to proper view.
>>
>> So either
>>
>>  [heads]  # or [branches]
>>  master
>>  to-submit
>>  origin/master
>>  origin/next
>>  ...
>>
>> where both '[heads]' and (possibly) '...' link to 'heads' view showing
>> _both_ local branches (refs/heads/*) and remote-tracking branches
>> (refs/remotes/*), like in first patch of series (perhaps with some
>> subdivision).
>>
>> Or
>>
>>  [heads]
>>  master
>>  to-submit
>>  ...
>>  [remotes]
>>  origin/master
>>  origin/next
>>  ...
>>
>> where '[heads]' link to 'heads' view which shows only local branches
>> (refs/heads/*), and '[remotes]' link to 'remotes' view which shows only
>> remote-tracking branches.
> 
> That's funny, I just squashed this patch with the summary list split
> view patch 8-) I'm going for the second option, to have [heads] link
> to heads which only lists local heads, and [remotes] linking to
> remotes that lists the remotes. We may or may not want to rather have
> [branches] instead of [heads], and keep the heads action to mean *all*
> heads, local and remote, but I'm not sure about it.

Errr... that was what I meant. First patch adding feature, and adding
remotes (not separated) to 'heads' section and 'heads' view (it could
have renamed 'heads' section to 'branches' but I feel that unnecessary),
and second patch squashed which adds 'remotes' section _and_ 'remotes'
action.


>> Nice. I see the difference from git_heads is using $project . ' remotes'
>> in place of $project in git_print_header_div() (why?),
> 
> FWIW, I decided to scratch that additional ' remotes' string when
> squashing this patch.

Hmmm... I'm not sure if $project in git_print_header_div() for those
two actions is good thing to have...

-- 
Jakub Narebski
Poland

Re: [PATCH v2 07/11] gitweb: add 'remotes' action

[Giuseppe Bilotta]

On Sun, Nov 16, 2008 at 1:29 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Sat, 15 Nov 2008, Giuseppe Bilotta wrote:
>> FWIW, I decided to scratch that additional ' remotes' string when
>> squashing this patch.
>
> Hmmm... I'm not sure if $project in git_print_header_div() for those
> two actions is good thing to have...

Considering it links back to summary view, it makes sense to say
$project in there.  If we decide to make it link to something else, we
should change the text accordingly. Suggestions?

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH v2 06/11] gitweb: use CSS to style split head lists.

[Jakub Narebski]

On Thu, 13 Nov 2008, Giuseppe Bilotta wrote:

> Introduce a new div class 'subsection' in the CSS and use it to style
> split head lists.

I think this patch should be squashed with the previous one.
 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.css  |   10 ++++++++++
>  gitweb/gitweb.perl |    4 +++-
>  2 files changed, 13 insertions(+), 1 deletions(-)
> 
> diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
> index a01eac8..751749f 100644
> --- a/gitweb/gitweb.css
> +++ b/gitweb/gitweb.css
> @@ -8,6 +8,16 @@ body {
>  	color: #000000;
>  }
>  
> +div.subsection {
> +	border: solid #d9d8d1;
> +	border-width: 1px;
> +	margin: 10px;
> +}
> +
> +.subsection .title {
> +	font-size: smaller;
> +}

Hmmm... do we use "subsection title" class anywhere? If you did
introduce it in earlier patch, this is one more reason to squash
them (after significant reworking)... ahh, this is link or span
or div with class .title insider div.subsection, sorry.

> +
>  a {
>  	color: #0000cc;
>  }
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 836b6ba..03e0b21 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -4296,9 +4296,11 @@ sub git_split_heads_body {
>  	} @$headlist;
>  
>  	foreach $leader (sort(keys %headlists)) {
> -		print "<b>$leader</b><br/>\n" unless $leader eq "\000";
> +		print "<div class=\"subsection\">\n";
> +		git_print_header_div(undef, $leader) unless $leader eq "\000";

Again, cannot this ugly guardian value be replaced by for example empty
string '', or undef?

>  		$list = $headlists{$leader};
>  		git_heads_body($list, $head, $from, $to, $extra);
> +		print "</div>\n";

Just in case, for easier editing and easier browsing through *occur*
buffer, I'd use here

+		print "</div>\n"; # class="subsection"


>  	}
>  }
>  

-- 
Jakub Narebski
Poland

Re: [PATCH v2 05/11] gitweb: git_split_heads_body function.

[Jakub Narebski]

On Thu, 13 Nov 2008, Giuseppe Bilotta wrote:

> The purpose of this function is to split a headlist into groups
> determined by the leading part of the refname, and call git_heads_body()
> on each group.

What is the reason of this patch? Is it to split remote-tracking
branches ('remotes' references) into remotes, and group them by
the remote repository name?

If it is true, then first: you should have wrote the _reason_ behind
this patch and not only what it does in this commit message. And use
better summary (commit title / subject of this patch).

Second, this patch wouldn't do what you want from it if there are
remotes with '/' in name.  I for example use "gsoc2008/gitweb-caching"
for Lea Wiemann repository with her GSoC 2008 work on adding caching
to gitweb.  Because there are many ways to specify remotes due to
backwards compatibility (and simplicity, as some for example prefer
old 'branches/' way to specify remotes), namely config, files under
'.git/remotes', and (from Cogito) files in '.git/branches', you would
have to either reimplement/reuse parts of git-remote (there is old Perl
implementation in contrib/examples), or use "git remote" or 
"git remote -v" command output[1].


So from me there is slight NAK on this patch, in this form.

> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl |   33 ++++++++++++++++++++++++++++++++-
>  1 files changed, 32 insertions(+), 1 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index a736f2a..836b6ba 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -4271,6 +4271,37 @@ sub git_tags_body {
>  	print "</table>\n";
>  }
>  
> +sub git_split_heads_body {
> +	my ($headlist, $head, $from, $to, $extra) = @_;

It should probably be said somewhere that git_split_heads_body has to
have the same signature as git_heads_body.

> +	my %headlists;
> +	my $leader; my $list; my @list;

Style - I would use:

+	my ($leader, $list, @list);

although I wouldn't use $list and @list together...

> +
> +	# Split @$headlist into a hash of lists
> +	map {
> +		my %ref = %$_;
> +		$ref{'hname'} = $ref{'name'};
> +		if ($ref{'name'} =~ /\//) {
> +			$ref{'name'} =~ s!^([^/]+)/!!;

As I said, this would fail on for example "gsoc2008/gitweb-caching"
remote...

> +			$leader = $1;
> +		} else {
> +			$leader = "\000";

Can't you use undef or "" for $leader? $headlists{undef} works...

> +		}
> +		if (defined $headlists{$leader}) {
> +			@list = @{$headlists{$leader}}
> +		} else {
> +			@list = ()
> +		}
> +		push @list, \%ref;
> +		$headlists{$leader} = [@list];

We have similar code in href(), but we use there:

  if (defined $ref{key}) {
  	push @{$ref{$key}}, $elem;
  } else {
  	$ref{$key} = [ $elem ];
  }

Isn't it simpler and easier to understand?


> +	} @$headlist;

Why such ugly and ungainy 'map' invocation, instead of IMHO simpler
and better here foreach loop?

> +
> +	foreach $leader (sort(keys %headlists)) {
> +		print "<b>$leader</b><br/>\n" unless $leader eq "\000";
> +		$list = $headlists{$leader};
> +		git_heads_body($list, $head, $from, $to, $extra);
> +	}
> +}

Wouldn't be it simpler to loop over @$headlist, and if prefix (or to be
more exact repository shorthand aka 'remote') changes then run 
git_heads_body, adjusting $from / $to accordingly, based on current and
remembered index? I think we can assume that list is sorted by refname,
can't we? If not then perhaps the way by building hash is good idea
after all...

> +
>  sub git_heads_body {
>  	# uses global variable $project
>  	my ($headlist, $head, $from, $to, $extra) = @_;
> @@ -4541,7 +4572,7 @@ sub git_summary {
>  
>  	if (@remotelist) {
>  		git_print_header_div('remotes');
> -		git_heads_body(\@remotelist, $head, 0, 15,
> +		git_split_heads_body(\@remotelist, $head, 0, 15,
>  		               $#remotelist <= 15 ? undef :
>  		               $cgi->a({-href => href(action=>"heads")}, "..."));
>  	}

Nice.


Footnotes:
==========
[1] It is strange that there is no explicit "git remote list"
-- 
Jakub Narebski
Poland

Re: [PATCH v2 05/11] gitweb: git_split_heads_body function.

[Giuseppe Bilotta]

On Sat, Nov 15, 2008 at 12:59 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Thu, 13 Nov 2008, Giuseppe Bilotta wrote:
>
>> The purpose of this function is to split a headlist into groups
>> determined by the leading part of the refname, and call git_heads_body()
>> on each group.
>
> What is the reason of this patch? Is it to split remote-tracking
> branches ('remotes' references) into remotes, and group them by
> the remote repository name?
>
> If it is true, then first: you should have wrote the _reason_ behind
> this patch and not only what it does in this commit message. And use
> better summary (commit title / subject of this patch).
>
> Second, this patch wouldn't do what you want from it if there are
> remotes with '/' in name.  I for example use "gsoc2008/gitweb-caching"
> for Lea Wiemann repository with her GSoC 2008 work on adding caching
> to gitweb.  Because there are many ways to specify remotes due to
> backwards compatibility (and simplicity, as some for example prefer
> old 'branches/' way to specify remotes), namely config, files under
> '.git/remotes', and (from Cogito) files in '.git/branches', you would
> have to either reimplement/reuse parts of git-remote (there is old Perl
> implementation in contrib/examples), or use "git remote" or
> "git remote -v" command output[1].

The initially intended purpose for this patch was to group remote
heads by remotes, but an interesting side-effect of doing it this way
was that it allowed to group _local_ heads too, by using the
stuff/morestuff syntax. For example, I could group gitweb/pathinfo and
gitweb/allheads together (although I disabled this grouping for local
heads in the patchset).

However, as you remark, the current patch fails to achieve even its
intended purpose, so it looks like going the 'git remote' way would be
the right way to find at least the grouping keys: this has the benefit
of allowing us to retrieve the remote URL as well by using 'git remote
-v', although it has the underside of require one additional git call.

It would also probably be a good idea to separate the actual head
grouping from the display of the grouped head lists. I wonder if Perl
has a 'tree' data structure that could be used to store the grouped
head lists ...

Ah yes, the code in this patch I was never actually really satisfied
with, hopefully I can rewrite it more sensibly with the adittional
experience I've accumulated this year.

>> +
>> +     # Split @$headlist into a hash of lists
>> +     map {
>> +             my %ref = %$_;
>> +             $ref{'hname'} = $ref{'name'};
>> +             if ($ref{'name'} =~ /\//) {
>> +                     $ref{'name'} =~ s!^([^/]+)/!!;
>
> As I said, this would fail on for example "gsoc2008/gitweb-caching"
> remote...

Would you say that in this case we want 'gsoc2008/gitweb-caching' as
the group head, or would you rather have nested groups [gsoc2008
[gitweb-caching [branches in gsoc2008/gitweb-caching] [etc]] ? I must
say that I think the latter would be quite interesting, but I _am_ a
little afraid we could turn up with way too much nested groups ...


-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH v2 05/11] gitweb: git_split_heads_body function.

[Jakub Narebski]

On Sat, 15 Nov 2008, Giuseppe Bilotta wrote:
> On Sat, Nov 15, 2008 at 12:59 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>> On Thu, 13 Nov 2008, Giuseppe Bilotta wrote:
>>
>>> The purpose of this function is to split a headlist into groups
>>> determined by the leading part of the refname, and call git_heads_body()
>>> on each group.
>>
>> What is the reason of this patch? Is it to split remote-tracking
>> branches ('remotes' references) into remotes, and group them by
>> the remote repository name?
>>
>> If it is true, then first: you should have wrote the _reason_ behind
>> this patch and not only what it does in this commit message. And use
>> better summary (commit title / subject of this patch).
>>
>> Second, this patch wouldn't do what you want from it if there are
>> remotes with '/' in name.  I for example use "gsoc2008/gitweb-caching"
>> for Lea Wiemann repository with her GSoC 2008 work on adding caching
>> to gitweb.  Because there are many ways to specify remotes due to
>> backwards compatibility (and simplicity, as some for example prefer
>> old 'branches/' way to specify remotes), namely config, files under
>> '.git/remotes', and (from Cogito) files in '.git/branches', you would
>> have to either reimplement/reuse parts of git-remote (there is old Perl
>> implementation in contrib/examples), or use "git remote" or
>> "git remote -v" command output[1].
> 
> The initially intended purpose for this patch was to group remote
> heads by remotes, but an interesting side-effect of doing it this way
> was that it allowed to group _local_ heads too, by using the
> stuff/morestuff syntax. For example, I could group gitweb/pathinfo and
> gitweb/allheads together (although I disabled this grouping for local
> heads in the patchset).

I'm not sure if it would be that useful. How many people have _many_
stuff/morestuff branches for some values of stuff/? The convention of
<initials>/<topic> of topic branches in git.git doesn't usually lead
to many branches with the same <initials>/ prefix.

> 
> However, as you remark, the current patch fails to achieve even its
> intended purpose, so it looks like going the 'git remote' way would be
> the right way to find at least the grouping keys: this has the benefit
> of allowing us to retrieve the remote URL as well by using 'git remote
> -v', although it has the underside of require one additional git call.

Now I thought about it a bit, I think your solution has merit. 

Splitting by remotes is hard and difficult to do right, especially if
you consider than 'remote' prefix doesn't need to have anything in
common with names (common prefix) of refs/remotes/* remote-tracking
branches used. It is fairly easy to do it right in common case, but
hard in uncommon one.

So perhaps the idea of using first dirname as a kind of category for
remotes is a good idea. And usually it would be also remote name.

But it really needs explanation in commit message... and quite a bit
of commit squashing.

> 
> It would also probably be a good idea to separate the actual head
> grouping from the display of the grouped head lists. I wonder if Perl
> has a 'tree' data structure that could be used to store the grouped
> head lists ...

Hash of hashes (well, hash references), see perldsc(1)?

> 
> Ah yes, the code in this patch I was never actually really satisfied
> with, hopefully I can rewrite it more sensibly with the adittional
> experience I've accumulated this year.

Code... well, perhaps... commit messages also matter.

> 
>>> +
>>> +     # Split @$headlist into a hash of lists
>>> +     map {
>>> +             my %ref = %$_;
>>> +             $ref{'hname'} = $ref{'name'};
>>> +             if ($ref{'name'} =~ /\//) {
>>> +                     $ref{'name'} =~ s!^([^/]+)/!!;
>>
>> As I said, this would fail on for example "gsoc2008/gitweb-caching"
>> remote...
> 
> Would you say that in this case we want 'gsoc2008/gitweb-caching' as
> the group head, or would you rather have nested groups [gsoc2008
> [gitweb-caching [branches in gsoc2008/gitweb-caching] [etc]] ? I must
> say that I think the latter would be quite interesting, but I _am_ a
> little afraid we could turn up with way too much nested groups ...

Now I think that having [gsoc2008] subgroup here might be a good
thing...

-- 
Jakub Narebski
Poland

Re: [PATCH v2 05/11] gitweb: git_split_heads_body function.

[Giuseppe Bilotta]

On Sun, Nov 16, 2008 at 2:13 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Sat, 15 Nov 2008, Giuseppe Bilotta wrote:
>> The initially intended purpose for this patch was to group remote
>> heads by remotes, but an interesting side-effect of doing it this way
>> was that it allowed to group _local_ heads too, by using the
>> stuff/morestuff syntax. For example, I could group gitweb/pathinfo and
>> gitweb/allheads together (although I disabled this grouping for local
>> heads in the patchset).
>
> I'm not sure if it would be that useful. How many people have _many_
> stuff/morestuff branches for some values of stuff/? The convention of
> <initials>/<topic> of topic branches in git.git doesn't usually lead
> to many branches with the same <initials>/ prefix.

Well, even if it's just two of them, it would still be nice. Or even
better, we could make it so that the grouping is skipped unless there
are at least N (to be decided) entries. This, btw, would be true for
the remotes idea too.

> Now I thought about it a bit, I think your solution has merit.
>
> Splitting by remotes is hard and difficult to do right, especially if
> you consider than 'remote' prefix doesn't need to have anything in
> common with names (common prefix) of refs/remotes/* remote-tracking
> branches used. It is fairly easy to do it right in common case, but
> hard in uncommon one.
>
> So perhaps the idea of using first dirname as a kind of category for
> remotes is a good idea. And usually it would be also remote name.
>
> But it really needs explanation in commit message... and quite a bit
> of commit squashing.

I'll probably do a single commit with a rather different logic than
the current one, too.

>> It would also probably be a good idea to separate the actual head
>> grouping from the display of the grouped head lists. I wonder if Perl
>> has a 'tree' data structure that could be used to store the grouped
>> head lists ...
>
> Hash of hashes (well, hash references), see perldsc(1)?

Ah, good, I always get those wrong. Will be an interesting challenge 8-D

>> Would you say that in this case we want 'gsoc2008/gitweb-caching' as
>> the group head, or would you rather have nested groups [gsoc2008
>> [gitweb-caching [branches in gsoc2008/gitweb-caching] [etc]] ? I must
>> say that I think the latter would be quite interesting, but I _am_ a
>> little afraid we could turn up with way too much nested groups ...
>
> Now I think that having [gsoc2008] subgroup here might be a good
> thing...

And subgroups (one for each remote) therein?

My idea would be that, if you only have
gsoc2008/gitweb-caching/branch[1-n], then you'd have a
gsoc2008/gitweb-caching group, and branch1 ... branchn as entries. If
OTOH we have gsoc2007/{gitweb-caching,gitstats}/branch*, we'd have
gsoc2008 group with gitweb-caching and gitstats subgroups, each with
its list of branches.


-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH v2 05/11] gitweb: git_split_heads_body function.

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Second, this patch wouldn't do what you want from it if there are
> remotes with '/' in name.  I for example use "gsoc2008/gitweb-caching"
> for Lea Wiemann repository with her GSoC 2008 work on adding caching
> to gitweb.

I think your point is if you also use gsoc2008/gitstats from another
remote repository, these two sets of remote tracking branches will be
shown grouped together.  But is it a bad thing?  After all, you chose to
use hierarchical names for them, _and_ you chose to use the same toplevel
hierarchy name for them.  Doesn't that mean you _wanted_ to have them both
appear in the same GSoC 2008 group?

Re: [PATCH v2 05/11] gitweb: git_split_heads_body function.

[Giuseppe Bilotta]

On Sat, Nov 15, 2008 at 1:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>
>> Second, this patch wouldn't do what you want from it if there are
>> remotes with '/' in name.  I for example use "gsoc2008/gitweb-caching"
>> for Lea Wiemann repository with her GSoC 2008 work on adding caching
>> to gitweb.
>
> I think your point is if you also use gsoc2008/gitstats from another
> remote repository, these two sets of remote tracking branches will be
> shown grouped together.  But is it a bad thing?  After all, you chose to
> use hierarchical names for them, _and_ you chose to use the same toplevel
> hierarchy name for them.  Doesn't that mean you _wanted_ to have them both
> appear in the same GSoC 2008 group?

The problem is that we have gsoc2008/gitweb-caching/branch1
gsoc2008/gitweb-caching/branch2 gsoc2008/gitstats/branch3
gsoc2008/gitstats/branch3, and my current code would show
gitweb-caching/branch1, gitweb-caching/branch2 etc under gsoc2008.

Having branch1 and branch2 under gsoc2008/gitweb-caching, and branch3
and branch4 under gsoc2008/gitstats would be more logical,
remote-wise, but it would of course lose the coupling between all the
gsoc2008 remotes.

If deep nesting is not a problem, I can code something to have
gitweb-caching and gistats under gsoc2008, and the respective branches
within.




-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH v2 05/11] gitweb: git_split_heads_body function.

[Jakub Narebski]

Giuseppe Bilotta wrote:
> On Sat, Nov 15, 2008 at 1:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jakub Narebski <jnareb@gmail.com> writes:
>>
>>> Second, this patch wouldn't do what you want from it if there are
>>> remotes with '/' in name.  I for example use "gsoc2008/gitweb-caching"
>>> for Lea Wiemann repository with her GSoC 2008 work on adding caching
>>> to gitweb.
>>
>> I think your point is if you also use gsoc2008/gitstats from another
>> remote repository, these two sets of remote tracking branches will be
>> shown grouped together.  But is it a bad thing?  After all, you chose to
>> use hierarchical names for them, _and_ you chose to use the same toplevel
>> hierarchy name for them.  Doesn't that mean you _wanted_ to have them both
>> appear in the same GSoC 2008 group?

Actually I _don't_ have 'gsoc2008/gitstats' remote, nor gsoc2008/gitstats/*
remote-tracking branches. 'gsoc2008/gitweb-caching' is the only remote
with hierarchical name. But I digress...

> The problem is that we have gsoc2008/gitweb-caching/branch1
> gsoc2008/gitweb-caching/branch2 gsoc2008/gitstats/branch3
> gsoc2008/gitstats/branch3, and my current code would show
> gitweb-caching/branch1, gitweb-caching/branch2 etc under gsoc2008.

I'm not sure if it wouldn't be simpler solution to just code _sorting_
heads-like view ('heads', 'remotes', 'tags') by ref name, or by age.
It would be best to have both, even...

Even without dividing 'remotes' view into subcategories (and
subsubcategories) you would have natural grouping:

 gsoc2008/gitweb-caching/branch1
 gsoc2008/gitweb-caching/branch2
 gsoc2008/gitstats/branch3
 gsoc2008/gitstats/branch4

if sorted by branch (ref) name, and not (possibly)

 gsoc2008/gitweb-caching/branch1
 gsoc2008/gitstats/branch4
 origin/todo
 gsoc2008/gitweb-caching/branch2
 gsoc2008/gitstats/branch3

when sorted by age (hmmm... committerdate or authordate?)

> Having branch1 and branch2 under gsoc2008/gitweb-caching, and branch3
> and branch4 under gsoc2008/gitstats would be more logical,
> remote-wise, but it would of course lose the coupling between all the
> gsoc2008 remotes.
> 
> If deep nesting is not a problem, I can code something to have
> gitweb-caching and gistats under gsoc2008, and the respective branches
> within.

The problems with nesting is those pesky remotes with only single
tracked branch to them; they are I think quote common... well, unless
you do one-shot pull, directly into local branch.

All that said, splitting 'remotes' section is difficult; using first
dirname as section is probably easiest, and good enough in most cases.
That is why I think this part should be put into separate series, to
not hinder rest of patches.

-- 
Jakub Narebski
Poland

Re: [PATCH v2 05/11] gitweb: git_split_heads_body function.

[Giuseppe Bilotta]

On Sun, Nov 16, 2008 at 1:12 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> Giuseppe Bilotta wrote:
>> The problem is that we have gsoc2008/gitweb-caching/branch1
>> gsoc2008/gitweb-caching/branch2 gsoc2008/gitstats/branch3
>> gsoc2008/gitstats/branch3, and my current code would show
>> gitweb-caching/branch1, gitweb-caching/branch2 etc under gsoc2008.
>
> I'm not sure if it wouldn't be simpler solution to just code _sorting_
> heads-like view ('heads', 'remotes', 'tags') by ref name, or by age.
> It would be best to have both, even...
>
> Even without dividing 'remotes' view into subcategories (and
> subsubcategories) you would have natural grouping:
>
>  gsoc2008/gitweb-caching/branch1
>  gsoc2008/gitweb-caching/branch2
>  gsoc2008/gitstats/branch3
>  gsoc2008/gitstats/branch4
>
> if sorted by branch (ref) name, and not (possibly)
>
>  gsoc2008/gitweb-caching/branch1
>  gsoc2008/gitstats/branch4
>  origin/todo
>  gsoc2008/gitweb-caching/branch2
>  gsoc2008/gitstats/branch3
>
> when sorted by age (hmmm... committerdate or authordate?)

Sorting is another interesting feature to look into, yes, but as you
mention it's a separate feature that would complement grouping.

>> Having branch1 and branch2 under gsoc2008/gitweb-caching, and branch3
>> and branch4 under gsoc2008/gitstats would be more logical,
>> remote-wise, but it would of course lose the coupling between all the
>> gsoc2008 remotes.
>>
>> If deep nesting is not a problem, I can code something to have
>> gitweb-caching and gistats under gsoc2008, and the respective branches
>> within.
>
> The problems with nesting is those pesky remotes with only single
> tracked branch to them; they are I think quote common... well, unless
> you do one-shot pull, directly into local branch.

My idea with this would be to only create a group if it has at least N
> 1 (probably N=2) entries.

> All that said, splitting 'remotes' section is difficult; using first
> dirname as section is probably easiest, and good enough in most cases.
> That is why I think this part should be put into separate series, to
> not hinder rest of patches.

Yes, I will resend the 'remote_heads' feature as a new (reduced)
patchset, then add (separate patchset) grouping for ref lists, and
then add (yet another patchset) detached head.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH v2 05/11] gitweb: git_split_heads_body function.

[Jakub Narebski]

Giuseppe Bilotta wrote:
> On Sun, Nov 16, 2008 at 1:12 PM, Jakub Narebski <jnareb@gmail.com> wrote:

>> The problems with nesting is those pesky remotes with only single
>> tracked branch to them; they are I think quote common... well, unless
>> you do one-shot pull, directly into local branch.
> 
> My idea with this would be to only create a group if it has at least
> N > 1 (probably N=2) entries.

A bit of complication is that you would have then series of
'uncategorized' (not in any subsection) entries / remote-tracking
branches.

>> All that said, splitting 'remotes' section is difficult; using first
>> dirname as section is probably easiest, and good enough in most cases.
>> That is why I think this part should be put into separate series, to
>> not hinder rest of patches.
> 
> Yes, I will resend the 'remote_heads' feature as a new (reduced)
> patchset, then add (separate patchset) grouping for ref lists, and
> then add (yet another patchset) detached head.

That is I think a good idea.

P.S. I think that sending this patch series for review, even if it was
not perfect was a very good idea... well, perhaps some patches could
be marked as RFC.

It is hard work to prepare good patches, then wait for review, then
wait a bit that there is no further review, working on the patches,
resend and wait for review, or for Ack and merge-in... Keep up good
work.

-- 
Jakub Narebski
Poland

Re: [PATCH v2 05/11] gitweb: git_split_heads_body function.

[Giuseppe Bilotta]

On Sun, Nov 16, 2008 at 3:21 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> Giuseppe Bilotta wrote:
>> On Sun, Nov 16, 2008 at 1:12 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>
>>> The problems with nesting is those pesky remotes with only single
>>> tracked branch to them; they are I think quote common... well, unless
>>> you do one-shot pull, directly into local branch.
>>
>> My idea with this would be to only create a group if it has at least
>> N > 1 (probably N=2) entries.
>
> A bit of complication is that you would have then series of
> 'uncategorized' (not in any subsection) entries / remote-tracking
> branches.

We'll put them in their own group 8-)

>> Yes, I will resend the 'remote_heads' feature as a new (reduced)
>> patchset, then add (separate patchset) grouping for ref lists, and
>> then add (yet another patchset) detached head.
>
> That is I think a good idea.
>
> P.S. I think that sending this patch series for review, even if it was
> not perfect was a very good idea... well, perhaps some patches could
> be marked as RFC.

That's what they were when I first sent them last year 8-)

> It is hard work to prepare good patches, then wait for review, then
> wait a bit that there is no further review, working on the patches,
> resend and wait for review, or for Ack and merge-in... Keep up good
> work.

Thanks.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH v2 04/11] gitweb: optional custom name for refs in git_heads_body

[Jakub Narebski]

On Thu, 13 Nov 2008, Giuseppe Bilotta wrote:

> We make a clear separation between the hash reference and the displayed
> name for refs displayed by git_heads_body. This can be used e.g. to
> group them and display only the distinct part of the name.

It is not clear for me from this commit message what this patch
is meant to do. Already git_heads_body (and also git_tags_body)
uses $ref{'name'} for display, and $ref{'fullname'} for linking
(to avoid possibility of tag/branch name conflict).

> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl |   12 +++++++-----
>  1 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index ab29aec..a736f2a 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -4288,16 +4288,18 @@ sub git_heads_body {
>  		} else {
>  			print "<tr class=\"light\">\n";
>  		}
> +		my $hname = $ref{'hname'} || $ref{'fullname'} || $ref{'name'};

I don't remember setting $ref{'hname'} anywhere; if there is a patch
that sets this, it should really be squashed together with this commit.
Otherwise the commit is not standalone, as it should be.

> +		my $name = $ref{'name'};

I understand that this is simply "shortcut" name, to avoid using
$ref{'name'} everywhere else, and instead using $name?  This is a bit
independent, I mean in the sense that it makes sense to squash those
patches together in a kind of "by the way" way, i.e. simplify the code
if we are making changes in this area.

>  		$alternate ^= 1;
>  		print "<td><i>$ref{'age'}</i></td>\n" .
>  		      ($curr ? "<td class=\"current_head\">" : "<td>") .
> -		      $cgi->a({-href => href(action=>"shortlog", hash=>$ref{'fullname'}),
> -		               -class => "list name"},esc_html($ref{'name'})) .
> +		      $cgi->a({-href => href(action=>"shortlog", hash=>$hname),
> +		               -class => "list name"},esc_html($name)) .
>  		      "</td>\n" .
>  		      "<td class=\"link\">" .
> -		      $cgi->a({-href => href(action=>"shortlog", hash=>$ref{'fullname'})}, "shortlog") . " | " .
> -		      $cgi->a({-href => href(action=>"log", hash=>$ref{'fullname'})}, "log") . " | " .
> -		      $cgi->a({-href => href(action=>"tree", hash=>$ref{'fullname'}, hash_base=>$ref{'name'})}, "tree") .
> +		      $cgi->a({-href => href(action=>"shortlog", hash=>$hname)}, "shortlog") . " | " .
> +		      $cgi->a({-href => href(action=>"log", hash=>$hname)}, "log") . " | " .
> +		      $cgi->a({-href => href(action=>"tree", hash=>$hname, hash_base=>$hname)}, "tree") .
>  		      "</td>\n" .
>  		      "</tr>";
>  	}

So, in short I think this patch needs work, at least better commit
message (perhaps I don't understand something...)

-- 
Jakub Narebski
Poland

Re: [PATCH v2 04/11] gitweb: optional custom name for refs in git_heads_body

[Giuseppe Bilotta]

On Sat, Nov 15, 2008 at 12:32 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Thu, 13 Nov 2008, Giuseppe Bilotta wrote:
>
>> We make a clear separation between the hash reference and the displayed
>> name for refs displayed by git_heads_body. This can be used e.g. to
>> group them and display only the distinct part of the name.
>
> It is not clear for me from this commit message what this patch
> is meant to do. Already git_heads_body (and also git_tags_body)
> uses $ref{'name'} for display, and $ref{'fullname'} for linking
> (to avoid possibility of tag/branch name conflict).

>> +             my $hname = $ref{'hname'} || $ref{'fullname'} || $ref{'name'};
>
> I don't remember setting $ref{'hname'} anywhere; if there is a patch
> that sets this, it should really be squashed together with this commit.
> Otherwise the commit is not standalone, as it should be.

The patch that sets hname is the next patch (the one that introduces
git_split_heads_body. It's quite obvious that this whole 'split head
lists' part needs some rethinking.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH v2 03/11] gitweb: separate heads and remotes list in summary view

[Jakub Narebski]

Dnia czwartek 13. listopada 2008 23:49, Giuseppe Bilotta napisał:

Very nice patch. Now that I have read it, I don't think it should be
squashed with previous patch (well, again that is only a suggestion).
Barring one issue (see below) its conciseness shows that gitweb has
quite good internal API.

> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl |   11 ++++++++++-
>  1 files changed, 10 insertions(+), 1 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index d7c97a3..ab29aec 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -4449,6 +4449,7 @@ sub git_summary {
>  	my %co = parse_commit("HEAD");
>  	my %cd = %co ? parse_date($co{'committer_epoch'}, $co{'committer_tz'}) : ();
>  	my $head = $co{'id'};
> +	my ($remote_heads) = gitweb_check_feature('remote_heads');
>  
>  	my $owner = git_get_project_owner($project);
>  
> @@ -4456,7 +4457,8 @@ sub git_summary {
>  	# These get_*_list functions return one more to allow us to see if
>  	# there are more ...
>  	my @taglist  = git_get_tags_list(16);
> -	my @headlist = git_get_heads_list(16);
> +	my @headlist = git_get_heads_list(16, 'heads');
> +	my @remotelist = $remote_heads ? git_get_heads_list(16, 'remotes') : ();

Nice.

>  	my @forklist;
>  	my ($check_forks) = gitweb_check_feature('forks');
>  
> @@ -4535,6 +4537,13 @@ sub git_summary {
>  		               $cgi->a({-href => href(action=>"heads")}, "..."));
>  	}
>  
> +	if (@remotelist) {
> +		git_print_header_div('remotes');
> +		git_heads_body(\@remotelist, $head, 0, 15,
> +		               $#remotelist <= 15 ? undef :
> +		               $cgi->a({-href => href(action=>"heads")}, "..."));
> +	}
> +

The only problem is that link leads to list of _all_ heads (best case),
or list to local branches (worst case, but I don't think gitweb does
it), instead of only list of remotes refs (remote-tracking branches),
as one would think.  Perhaps we could use 'h' (hash), or 'opt (extra
options) parameter for this action, or just add 'remotes' action? 

>  	if (@forklist) {
>  		git_print_header_div('forks');
>  		git_project_list_body(\@forklist, 'age', 0, 15,
> -- 
> 1.5.6.5

P.S. Not uptodate (git version 1.6.0.4)? Just kidding...

-- 
Jakub Narebski
Poland

Re: [PATCH v2 03/11] gitweb: separate heads and remotes list in summary view

[Giuseppe Bilotta]

2008/11/14 Jakub Narebski <jnareb@gmail.com>:
> Dnia czwartek 13. listopada 2008 23:49, Giuseppe Bilotta napisał:
>
> Very nice patch. Now that I have read it, I don't think it should be
> squashed with previous patch (well, again that is only a suggestion).

See reply in previous patch. Sometimes it's hard to tell what's the
best patch-splitting strategy ...

> Barring one issue (see below) its conciseness shows that gitweb has
> quite good internal API.

Most definitely. I find that working on gitweb is almost pleasurable ;-)
[I mean, it's still Perl, which is not Tcl but not Ruby either 8-P]

>>       my @forklist;
>>       my ($check_forks) = gitweb_check_feature('forks');
>>
>> @@ -4535,6 +4537,13 @@ sub git_summary {
>>                              $cgi->a({-href => href(action=>"heads")}, "..."));
>>       }
>>
>> +     if (@remotelist) {
>> +             git_print_header_div('remotes');
>> +             git_heads_body(\@remotelist, $head, 0, 15,
>> +                            $#remotelist <= 15 ? undef :
>> +                            $cgi->a({-href => href(action=>"heads")}, "..."));
>> +     }
>> +
>
> The only problem is that link leads to list of _all_ heads (best case),
> or list to local branches (worst case, but I don't think gitweb does
> it), instead of only list of remotes refs (remote-tracking branches),
> as one would think.  Perhaps we could use 'h' (hash), or 'opt (extra
> options) parameter for this action, or just add 'remotes' action?

Adding a 'remotes' section (and corresponding action, too) is what is
done by subsequent patches. It's quite obvious, I think, that the
patch sequence follows _very_ closely the order in which I
implemented/tested features. It might make more sense to move some of
them earlier, but then such earlier patches would only make sense
because of the ones that follow ... this is why I decided to keep the
sequence as is.

>> 1.5.6.5
>
> P.S. Not uptodate (git version 1.6.0.4)? Just kidding...

Yeah, I know, given that I work with 'next' gitweb, I could as well
just upgrade the system git to the same version 8-P

Instead, I'm just relying on stock Debian stuff, which obviously,
being in freeze now, is not updating unstable either 8-P


-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH v2 02/11] gitweb: git_get_heads_list accepts an optional list of refs.

[Jakub Narebski]

Dnia czwartek 13. listopada 2008 23:49, Giuseppe Bilotta napisał:

> git_get_heads_list(limit, dir1, dir2, ...) can now be used to retrieve
> refs/dir1, refs/dir2 etc. Defaults to ('heads') or ('heads', 'remotes')
> depending on the remote_heads option.

Minor nit: I think it would be better to use the same terminology in
commit message as in code, i.e. 'class1' instead of 'dir1', or perhaps
'ref_class1' if it would be better.

This is only a suggestion, but perhaps this patch could be squashed
with a later one?

> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl |   11 +++++++----
>  1 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index b6c4233..d7c97a3 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2663,15 +2663,18 @@ sub parse_from_to_diffinfo {
>  ## parse to array of hashes functions
>  
>  sub git_get_heads_list {
> -	my $limit = shift;
> +	my ($limit, @class) = @_;
> +	unless (defined @class) {
> +		my ($remote_heads) = gitweb_check_feature('remote_heads');
> +		@class = ('heads', $remote_heads ? 'remotes' : undef);
> +	}
> +	my @refs = map { "refs/$_" } @class;

Nice.

>  	my @headslist;
>  
> -	my ($remote_heads) = gitweb_check_feature('remote_heads');
> -
>  	open my $fd, '-|', git_cmd(), 'for-each-ref',
>  		($limit ? '--count='.($limit+1) : ()), '--sort=-committerdate',
>  		'--format=%(objectname) %(refname) %(subject)%00%(committer)',
> -		'refs/heads', ( $remote_heads ? 'refs/remotes' : '')
> +		@refs
>  		or return;
>  	while (my $line = <$fd>) {
>  		my %ref_item;

So this is a bit of generalization of (part of) previous patch,
isn't it?

-- 
Jakub Narebski
Poland

Re: [PATCH v2 02/11] gitweb: git_get_heads_list accepts an optional list of refs.

[Giuseppe Bilotta]

On Fri, Nov 14, 2008 at 7:48 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> Dnia czwartek 13. listopada 2008 23:49, Giuseppe Bilotta napisał:
>
>> git_get_heads_list(limit, dir1, dir2, ...) can now be used to retrieve
>> refs/dir1, refs/dir2 etc. Defaults to ('heads') or ('heads', 'remotes')
>> depending on the remote_heads option.
>
> Minor nit: I think it would be better to use the same terminology in
> commit message as in code, i.e. 'class1' instead of 'dir1', or perhaps
> 'ref_class1' if it would be better.

Uhm, ref/ref_class1 reads horrible, but sticking with a uniform
terminology is a good point. I adjusted the commit message
consequently.

> This is only a suggestion, but perhaps this patch could be squashed
> with a later one?

Or with the previous one, since as you remark it's a generalization of
the previous.

>>       my @headslist;
>>
>> -     my ($remote_heads) = gitweb_check_feature('remote_heads');
>> -
>>       open my $fd, '-|', git_cmd(), 'for-each-ref',
>>               ($limit ? '--count='.($limit+1) : ()), '--sort=-committerdate',
>>               '--format=%(objectname) %(refname) %(subject)%00%(committer)',
>> -             'refs/heads', ( $remote_heads ? 'refs/remotes' : '')
>> +             @refs
>>               or return;
>>       while (my $line = <$fd>) {
>>               my %ref_item;
>
> So this is a bit of generalization of (part of) previous patch,
> isn't it?

Precisely. I must say I had problems finding the proper splitting
point for some of these patches, because they had a very organic
evolution, but at the same time sqashing them together would give too
large changesets at once. You'll find that this is not the only patch
that makes the most sense only after seeing what comes later.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH v2 01/11] gitweb: introduce remote_heads feature

[Jakub Narebski]

On Thu, 13 Nov 2008, Giuseppe Bilotta wrote:

> With this feature enabled, remotes are retrieved (and displayed)
> when getting (and displaying) the heads list.

I think it would be good idea to add in commit message idea _why_
such feature would be useful, for example

  This is useful if you want to use git-instaweb to examine the state
  of repository, influding remote-tracking branches, or a repository
  is fork of other repository, and remote-tracking branches are used
  to see what commits this fork has in addition to those from forked
  (main) repository.

Or something like that.

It would be also in my opinion a good idea to modify git-instaweb.sh
(I guess better in separate commit) to make it make use of this new
feature... unless it does it already, doesn't it?

> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl |   31 +++++++++++++++++++++++++++++--
>  1 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 933e137..b6c4233 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -327,6 +327,18 @@ our %feature = (
>  	'ctags' => {
>  		'override' => 0,
>  		'default' => [0]},
> +
> +	# Make gitweb show remotes too in the heads list

I'm not native engish speaker, but shouldn't instead of "remotes too"
be "also remotes" or "remotes also"?

> +
> +	# To enable system wide have in $GITWEB_CONFIG
> +	# $feature{'remote_heads'}{'default'} = [1];
> +	# To have project specific config enable override in $GITWEB_CONFIG
> +	# $feature{'remote_heads'}{'override'} = 1;
> +	# and in project config gitweb.remote_heads = 0|1;
> +	'remote_heads' => {
> +		'sub' => \&feature_remote_heads,
> +		'override' => 0,
> +		'default' => [0]},
>  );
>  
>  sub gitweb_check_feature {
> @@ -392,6 +404,18 @@ sub feature_pickaxe {
>  	return ($_[0]);
>  }
>  
> +sub feature_remote_heads {
> +	my ($val) = git_get_project_config('remote_heads', '--bool');
> +
> +	if ($val eq 'true') {
> +		return (1);
> +	} elsif ($val eq 'false') {
> +		return (0);
> +	}
> +
> +	return ($_[0]);
> +}

Hmmm... I think it is hight time to provide option to 
git_get_project_config to _not_ use backward compatibility with 
'git config --bool', i.e. return Perl bool, and not 'true'/'false'
string.

But this is I think outside scope of this patch...

> +
>  # checking HEAD file with -e is fragile if the repository was
>  # initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed
>  # and then pruned.
> @@ -2642,10 +2666,12 @@ sub git_get_heads_list {
>  	my $limit = shift;
>  	my @headslist;
>  
> +	my ($remote_heads) = gitweb_check_feature('remote_heads');
> +
>  	open my $fd, '-|', git_cmd(), 'for-each-ref',
>  		($limit ? '--count='.($limit+1) : ()), '--sort=-committerdate',
>  		'--format=%(objectname) %(refname) %(subject)%00%(committer)',
> -		'refs/heads'
> +		'refs/heads', ( $remote_heads ? 'refs/remotes' : '')
>  		or return;
>  	while (my $line = <$fd>) {
>  		my %ref_item;
> @@ -2656,8 +2682,9 @@ sub git_get_heads_list {
>  		my ($committer, $epoch, $tz) =
>  			($committerinfo =~ /^(.*) ([0-9]+) (.*)$/);
>  		$ref_item{'fullname'}  = $name;
> -		$name =~ s!^refs/heads/!!;
> +		$name =~ s!^refs/(head|remote)s/!!;
>  
> +		$ref_item{'class'} = $1;

Nice catch.

>  		$ref_item{'name'}  = $name;
>  		$ref_item{'id'}    = $hash;
>  		$ref_item{'title'} = $title || '(no commit message)';
> -- 
> 1.5.6.5
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCH v2 01/11] gitweb: introduce remote_heads feature

[Giuseppe Bilotta]

On Fri, Nov 14, 2008 at 7:15 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Thu, 13 Nov 2008, Giuseppe Bilotta wrote:
>
>> With this feature enabled, remotes are retrieved (and displayed)
>> when getting (and displaying) the heads list.
>
> I think it would be good idea to add in commit message idea _why_
> such feature would be useful, for example
>
>  This is useful if you want to use git-instaweb to examine the state
>  of repository, influding remote-tracking branches, or a repository
>  is fork of other repository, and remote-tracking branches are used
>  to see what commits this fork has in addition to those from forked
>  (main) repository.
>
> Or something like that.

Ah yes, many commit messages in this patchset are way too terse. A
side effect of this being something like the first patchset I ever
prepared. I'll rework them to something more sensible.

> It would be also in my opinion a good idea to modify git-instaweb.sh
> (I guess better in separate commit) to make it make use of this new
> feature... unless it does it already, doesn't it?

It doesn't, but it's something I have considered. I'll work on it (on
a separate patch)


>> +     # Make gitweb show remotes too in the heads list
>
> I'm not native engish speaker, but shouldn't instead of "remotes too"
> be "also remotes" or "remotes also"?

No idea, I guess we'll wait for a native english speaker opinion 8-D

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH v2 00/11] gitweb: display remote heads

[Jakub Narebski]

On Thu, 13 Nov 2008, Giuseppe Bilotta wrote:

> This is a patchset I presented about a year ago or so, but after a lively
> discussion it dropped into silence. I'm now presenting it again, with minor
> cleanups and adjustements.

That is very nice of you to resend this series. If you could provide
link to earlier discussion of this series...
 
> Giuseppe Bilotta (11):
>   gitweb: introduce remote_heads feature
>   gitweb: git_get_heads_list accepts an optional list of refs.
>   gitweb: separate heads and remotes list in summary view
>   gitweb: optional custom name for refs in git_heads_body
>   gitweb: git_split_heads_body function.
>   gitweb: use CSS to style split head lists.
>   gitweb: add 'remotes' action
>   gitweb: display HEAD in heads list when detached
>   gitweb: git_is_head_detached() function
>   gitweb: add HEAD to list of shortlog refs if detached
>   gitweb: CSS style and refs mark for detached HEAD

I'll try to review individual patches, but I haven't examined them
yet, so perhaps there is a reason why there are so many patches in
this series?


Note that on GMane NNTP (news) interface I can see only two last
patches. Could anyone not CC-ed confirm or deny if this is VGER
anti-SPAM filter at work, or some GMane archive hiccup?

-- 
Jakub Narebski
Poland

Re: [PATCH v2 00/11] gitweb: display remote heads

[Sverre Rabbelier]

On Fri, Nov 14, 2008 at 15:33, Jakub Narebski <jnareb@gmail.com> wrote:
> Note that on GMane NNTP (news) interface I can see only two last
> patches. Could anyone not CC-ed confirm or deny if this is VGER
> anti-SPAM filter at work, or some GMane archive hiccup?

I got all of the patches just fine, and I'm not CC-ed ;).

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH v2 00/11] gitweb: display remote heads

[Giuseppe Bilotta]

On Fri, Nov 14, 2008 at 3:33 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Thu, 13 Nov 2008, Giuseppe Bilotta wrote:
>
>> This is a patchset I presented about a year ago or so, but after a lively
>> discussion it dropped into silence. I'm now presenting it again, with minor
>> cleanups and adjustements.
>
> That is very nice of you to resend this series. If you could provide
> link to earlier discussion of this series...

There was no actual discussion after I sent the series, although there
was some while I was preparing it. Plus, gmane is quite unresponsive
for me at the moment, but the whereabous of the past discussions are
these http://kerneltrap.org/mailarchive/git/2007/8/30/256411/thread

>> Giuseppe Bilotta (11):
>>   gitweb: introduce remote_heads feature
>>   gitweb: git_get_heads_list accepts an optional list of refs.
>>   gitweb: separate heads and remotes list in summary view
>>   gitweb: optional custom name for refs in git_heads_body
>>   gitweb: git_split_heads_body function.
>>   gitweb: use CSS to style split head lists.
>>   gitweb: add 'remotes' action
>>   gitweb: display HEAD in heads list when detached
>>   gitweb: git_is_head_detached() function
>>   gitweb: add HEAD to list of shortlog refs if detached
>>   gitweb: CSS style and refs mark for detached HEAD
>
> I'll try to review individual patches, but I haven't examined them
> yet, so perhaps there is a reason why there are so many patches in
> this series?

And I actually squashed some! The original patchset was 14 patches.
Now it's about 10, of which the first 6 as the actual remote heads
stuff, and the last 4 are the detached HEAD stuff.

> Note that on GMane NNTP (news) interface I can see only two last
> patches. Could anyone not CC-ed confirm or deny if this is VGER
> anti-SPAM filter at work, or some GMane archive hiccup?

Seems to be a gmane problem, I can see it too.

-- 
Giuseppe "Oblomov" Bilotta