[PATCH 1/2] gitweb: Allow for multivalued parameters passed to href subroutine

[PATCH 1/2] gitweb: Allow for multivalued parameters passed to href subroutine

[Jakub Narebski]

Make it possible to generate URLs with multivalued parameters in the
href() subroutine, via passing reference to array of values.

Example:
  href(action=>"log", extra_options=>["--no-merges", "--first-parent"])

This also allows for easy passing back (replaying) current value
of, possibly multivalued, extra_options ('opt') parameter, using:

  href(..., extra_options=>\@extra_options)

which would be required when we start using extra_options in gitweb,
and would want to preserve its value.  For example when creating links
to next/previous page of 'log' or 'history' view output, we would want
to preserve '--no-merges' and/or '-first-parent' extra options.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This is the same patch as sent earlier, but with more detailed commit
message.  As the previous version made it into git.git, this is just
for archives.

 gitweb/gitweb.perl |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 8a32899..498b936 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -629,7 +629,13 @@ sub href(%) {
 	for (my $i = 0; $i < @mapping; $i += 2) {
 		my ($name, $symbol) = ($mapping[$i], $mapping[$i+1]);
 		if (defined $params{$name}) {
-			push @result, $symbol . "=" . esc_param($params{$name});
+			if (ref($params{$name}) eq "ARRAY") {
+				foreach my $par (@{$params{$name}}) {
+					push @result, $symbol . "=" . esc_param($par);
+				}
+			} else {
+				push @result, $symbol . "=" . esc_param($params{$name});
+			}
 		}
 	}
 	$href .= "?" . join(';', @result) if scalar @result;
-- 
1.5.2.4

[RFC/PATCH 2/2] gitweb: Add an option to show size of blobs in the tree view

[Jakub Narebski]

Add support for extra option ('opt' parameter) '-l' for the 'tree'
view, to show (in separate column, between mode and filename) the size
of blobs (files).  This option is passed through to all the 'tree'
links, so from 'tree' view with size of blobs on go to 'tree' view
also with size of blobs.

For the 'tree' and 'commit' (submodule) entries, '-' is shown in place
of size.

Currently you have to add ";opt=-l" to URL by hand to start.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This patch is an example of using infrastructure introduced by the
earlier patch: 
  gitweb: Allow for multivalued parameters passed to href subroutine

It allows to play with 'tree' view with blob size. Currently you
have to start browsing by adding ";opt=-l" to the gitweb URL by
hand.  There is not link which will change view from ordinary 'tree'
view to 'tree' view with blob sizes.

The 'tree' with blob size view is slightly more costly than the
ordinary, old 'tree' view, but not much more (0.018s vs 0.012s
in the hot cache case), so I don't think we need to control it
as a enabled (or disabled) feature, overrideable or not.  It
probably should not be default.

What do you think about this?

 gitweb/gitweb.css  |    5 ++++
 gitweb/gitweb.perl |   53 ++++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index 096313b..dd24546 100644
--- a/gitweb/gitweb.css
+++ b/gitweb/gitweb.css
@@ -326,6 +326,11 @@ td.mode {
 	font-family: monospace;
 }
 
+td.size {
+	font-family: monospace;
+	text-align: right;
+}
+
 /* styling of diffs (patchsets): commitdiff and blobdiff views */
 div.diff.header,
 div.diff.extended_header {
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 498b936..97ad1da 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -432,6 +432,7 @@ if (defined $hash_base) {
 
 my %allowed_options = (
 	"--no-merges" => [ qw(rss atom log shortlog history) ],
+	"-l" => [ qw(tree) ],
 );
 
 our @extra_options = $cgi->param('opt');
@@ -1998,16 +1999,31 @@ sub parse_ls_tree_line ($;%) {
 	my %opts = @_;
 	my %res;
 
-	#'100644 blob 0fa3f3a66fb6a137f6ec2c19351ed4d807070ffa	panic.c'
-	$line =~ m/^([0-9]+) (.+) ([0-9a-fA-F]{40})\t(.+)$/s;
+	if ($opts{'-l'}) {
+		#'100644 blob 0fa3f3a66fb6a137f6ec2c19351ed4d807070ffa   16717	panic.c'
+		$line =~ m/^([0-9]+) (.+) ([0-9a-fA-F]{40}) +(-|[0-9]+)\t(.+)$/s;
 
-	$res{'mode'} = $1;
-	$res{'type'} = $2;
-	$res{'hash'} = $3;
-	if ($opts{'-z'}) {
-		$res{'name'} = $4;
+		$res{'mode'} = $1;
+		$res{'type'} = $2;
+		$res{'hash'} = $3;
+		$res{'size'} = $4;
+		if ($opts{'-z'}) {
+			$res{'name'} = $5;
+		} else {
+			$res{'name'} = unquote($5);
+		}
 	} else {
-		$res{'name'} = unquote($4);
+		#'100644 blob 0fa3f3a66fb6a137f6ec2c19351ed4d807070ffa	panic.c'
+		$line =~ m/^([0-9]+) (.+) ([0-9a-fA-F]{40})\t(.+)$/s;
+
+		$res{'mode'} = $1;
+		$res{'type'} = $2;
+		$res{'hash'} = $3;
+		if ($opts{'-z'}) {
+			$res{'name'} = $4;
+		} else {
+			$res{'name'} = unquote($4);
+		}
 	}
 
 	return wantarray ? %res : \%res;
@@ -2679,6 +2695,9 @@ sub git_print_tree_entry {
 	# and link is the action links of the entry.
 
 	print "<td class=\"mode\">" . mode_str($t->{'mode'}) . "</td>\n";
+	if (exists $t->{'size'}) {
+		print "<td class=\"size\">$t->{'size'}</td>\n";
+	}
 	if ($t->{'type'} eq "blob") {
 		print "<td class=\"list\">" .
 			$cgi->a({-href => href(action=>"blob", hash=>$t->{'hash'},
@@ -4268,8 +4287,9 @@ sub git_tree {
 			$hash = $hash_base;
 		}
 	}
+
 	$/ = "\0";
-	open my $fd, "-|", git_cmd(), "ls-tree", '-z', $hash
+	open my $fd, "-|", git_cmd(), "ls-tree", '-z', @extra_options, $hash
 		or die_error(undef, "Open git-ls-tree failed");
 	my @entries = map { chomp; $_ } <$fd>;
 	close $fd or die_error(undef, "Reading tree failed");
@@ -4280,6 +4300,7 @@ sub git_tree {
 	git_header_html();
 	my $basedir = '';
 	my ($have_blame) = gitweb_check_feature('blame');
+	my $show_sizes = grep(/^-l$/, @extra_options);
 	if (defined $hash_base && (my %co = parse_commit($hash_base))) {
 		my @views_nav = ();
 		if (defined $file_name) {
@@ -4288,7 +4309,8 @@ sub git_tree {
 				                       hash=>$hash, file_name=>$file_name)},
 				        "history"),
 				$cgi->a({-href => href(action=>"tree",
-				                       hash_base=>"HEAD", file_name=>$file_name)},
+				                       hash_base=>"HEAD", file_name=>$file_name,
+				                       extra_options=>\@extra_options)},
 				        "HEAD"),
 		}
 		my $snapshot_links = format_snapshot_links($hash);
@@ -4296,7 +4318,8 @@ sub git_tree {
 			# FIXME: Should be available when we have no hash base as well.
 			push @views_nav, $snapshot_links;
 		}
-		git_print_page_nav('tree','', $hash_base, undef, undef, join(' | ', @views_nav));
+		git_print_page_nav('tree','', $hash_base, undef, undef,
+		                   join(' | ', @views_nav));
 		git_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash_base);
 	} else {
 		undef $hash_base;
@@ -4330,8 +4353,10 @@ sub git_tree {
 		# based on git_print_tree_entry
 		print '<td class="mode">' . mode_str('040000') . "</td>\n";
 		print '<td class="list">';
-		print $cgi->a({-href => href(action=>"tree", hash_base=>$hash_base,
-		                             file_name=>$up)},
+		print $cgi->a({-href => href(action=>"tree",
+		                             hash_base=>$hash_base,
+		                             file_name=>$up,
+		                             extra_options=>\@extra_options)},
 		              "..");
 		print "</td>\n";
 		print "<td class=\"link\"></td>\n";
@@ -4339,7 +4364,7 @@ sub git_tree {
 		print "</tr>\n";
 	}
 	foreach my $line (@entries) {
-		my %t = parse_ls_tree_line($line, -z => 1);
+		my %t = parse_ls_tree_line($line, -z => 1, -l => $show_sizes);
 
 		if ($alternate) {
 			print "<tr class=\"dark\">\n";
-- 
1.5.2.4

Re: [RFC/PATCH 2/2] gitweb: Add an option to show size of blobs in the tree view

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> It allows to play with 'tree' view with blob size. Currently you
> have to start browsing by adding ";opt=-l" to the gitweb URL by
> hand.  There is not link which will change view from ordinary 'tree'
> view to 'tree' view with blob sizes.
>
> The 'tree' with blob size view is slightly more costly than the
> ordinary, old 'tree' view, but not much more (0.018s vs 0.012s
> in the hot cache case), so I don't think we need to control it
> as a enabled (or disabled) feature, overrideable or not.  It
> probably should not be default.

I do not think there is any reason to forbid its use, as the
"-l" to ls-tree was introduced for exactly this purpose,
However, it might make sense to make the use of -l optional via
the %feature mechanism.  50% increase even on hot cache case is
not a price people who run busy sites would want to pay.

Re: [RFC/PATCH 2/2] gitweb: Add an option to show size of blobs in the tree view

[Jakub Narebski]

Gaah, I just find that I forgot in some places to preserve "-l"...
Please wait for amended patch.

-- 
Jakub Narebski

Re: [RFC/PATCH 2/2] gitweb: Add an option to show size of blobs in the tree view

[Jakub Narebski]

On Wed, 1 Aug 2007, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> It allows to play with 'tree' view with blob size. Currently you
>> have to start browsing by adding ";opt=-l" to the gitweb URL by
>> hand.  There is not link which will change view from ordinary 'tree'
>> view to 'tree' view with blob sizes.
>>
>> The 'tree' with blob size view is slightly more costly than the
>> ordinary, old 'tree' view, but not much more (0.018s vs 0.012s
>> in the hot cache case), so I don't think we need to control it
>> as a enabled (or disabled) feature, overrideable or not.  It
>> probably should not be default.
> 
> I do not think there is any reason to forbid its use, as the
> "-l" to ls-tree was introduced for exactly this purpose,
> However, it might make sense to make the use of -l optional via
> the %feature mechanism.  50% increase even on hot cache case is
> not a price people who run busy sites would want to pay.

It's 0.014s vs 0.009s - 0.010s on root dir of repacked git.git 
repository. It is I think caused by the fact that "git ls-tree -l 
<tree-ish>" has to look up each blob in the tree, and read its
header. IIRC header is uncompressed, so it doesn't need deflating;
if it is compressed, then we have to add deflating to that. The
blob size (object size) is not present in the tree object structure.
Perhaps it is something to have in mind when designing and implementing
new packv4 with its creating tree objects on demand.

But I agree that this should be protected by the %feature mechanism.
Two questions:

 1. Should we make '-l' default when turned on? Or make 'showsizes'
    %feature tristate: off, on, by default on?

 2. If it is turned off, should we silently (or not so silently)
    ignore this option, or return "Permission denied" or perhaps
    "Invalid extra options parameter"?

And how we should name this feature (key in %feature hash)?


P.S. I have received no comments on
 [RFC/PATCH] gitweb: Enable transparent compression for HTTP output
(trade CPU load for lower bandwidth usage).

-- 
Jakub Narebski
Poland

Re: [RFC/PATCH 2/2] gitweb: Add an option to show size of blobs in the tree view

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> But I agree that this should be protected by the %feature mechanism.
> Two questions:
>
>  1. Should we make '-l' default when turned on? Or make 'showsizes'
>     %feature tristate: off, on, by default on?
>
>  2. If it is turned off, should we silently (or not so silently)
>     ignore this option, or return "Permission denied" or perhaps
>     "Invalid extra options parameter"?
>
> And how we should name this feature (key in %feature hash)?

I would say we would not do this by default, only with an
explicit override with gitweb-config.perl.  I am not sure what
the good name would be.  "expensive-ls-tree" perhaps?

> P.S. I have received no comments on
>  [RFC/PATCH] gitweb: Enable transparent compression for HTTP output
> (trade CPU load for lower bandwidth usage).

That's probably nobody was interested in it.

Re: [RFC/PATCH 2/2] gitweb: Add an option to show size of blobs in the tree view

[Jakub Narebski]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> But I agree that this should be protected by the %feature mechanism.
>> Two questions:
>>
>>  1. Should we make '-l' default when turned on? Or make 'showsizes'
>>     %feature tristate: off, on, by default on?
>>
>>  2. If it is turned off, should we silently (or not so silently)
>>     ignore this option, or return "Permission denied" or perhaps
>>     "Invalid extra options parameter"?
>>
>> And how we should name this feature (key in %feature hash)?
> 
> I would say we would not do this by default, only with an
> explicit override with gitweb-config.perl.  I am not sure what
> the good name would be.  "expensive-ls-tree" perhaps?

So you think that ";opt=-l" would be required to have 'tree' view with 
blob (file) sizes, and it would be allowed only if 
gitweb_check_feature('ls-tree-size') is true (or something like that).
Should we return "Permission denied" or simply ignore "-l" extra option 
if it is prohivited by the config?

As to good name: 'showsize'? 'ls-tree-size'? 'ls-tree--long'?

>> P.S. I have received no comments on
>>  [RFC/PATCH] gitweb: Enable transparent compression for HTTP output
>> (trade CPU load for lower bandwidth usage).
> 
> That's probably nobody was interested in it.

Well, it was also posted in the middle of old thread... 

-- 
Jakub Narebski
Poland

Re: [RFC/PATCH 2/2] gitweb: Add an option to show size of blobs in the tree view

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> So you think that ";opt=-l" would be required to have 'tree' view with 
> blob (file) sizes, and it would be allowed only if 
> gitweb_check_feature('ls-tree-size') is true (or something like that).
> Should we return "Permission denied" or simply ignore "-l" extra option 
> if it is prohivited by the config?

I would say we should make config show-tree-entry-size (or
whatever name) unconditionally give the object size of the tree
entry, and drop ";opt=-l in URL" support from the code.  In
other words, if the site/repo owner wants to show it, it is
shown to everybody, otherwise nothing is shown.

Keeping the URL clean and without needless variations would help
caching; I do not see major advantage of this feature one way or
the other, so if we can keep a tree view of the same object from
the same repository the same by removing one user preference,
that would be better.

Re: [RFC/PATCH 2/2] gitweb: Add an option to show size of blobs in the tree view

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

>>> P.S. I have received no comments on
>>>  [RFC/PATCH] gitweb: Enable transparent compression for HTTP output
>>> (trade CPU load for lower bandwidth usage).
>> 
>> That's probably nobody was interested in it.
>
> Well, it was also posted in the middle of old thread... 

There probably is some of that as well.

Personally, this late in the game, I would be more interested in
resolving the File::Find() stuff, which has been a real issue in
the field, than the compressed transfer one.

Re: [RFC/PATCH 2/2] gitweb: Add an option to show size of blobs in the tree view

[Jakub Narebski]

Junio C Hamano wrote:

> Personally, this late in the game, I would be more interested in
> resolving the File::Find() stuff, which has been a real issue in
> the field, than the compressed transfer one.

I posted my tentative Ack, haven't I?

I guess that your solution is good, and doesn't have any drawbacks,
besides perhaps a tiny little bit of lost performance. So I think
that you should just commit this...


As to the new stuff: I think I postpone large changes, like blob
size in tree view, or links to no merges and first parent 
log/shortlog/history view, or list form of two pipelines we have in 
gitweb, or HTML cleanup, etc. after the v1.5.3 release.

-- 
Jakub Narebski
Poland

Re: [RFC/PATCH 2/2] gitweb: Add an option to show size of blobs in the tree view

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Junio C Hamano wrote:
>
>> Personally, this late in the game, I would be more interested in
>> resolving the File::Find() stuff, which has been a real issue in
>> the field, than the compressed transfer one.
>
> I posted my tentative Ack, haven't I?
>
> I guess that your solution is good, and doesn't have any drawbacks,
> besides perhaps a tiny little bit of lost performance. So I think
> that you should just commit this...
>
>
> As to the new stuff: I think I postpone large changes, like blob
> size in tree view, or links to no merges and first parent 
> log/shortlog/history view, or list form of two pipelines we have in 
> gitweb, or HTML cleanup, etc. after the v1.5.3 release.

Thanks, alright, I am relieved to know that we are on the same
page.