threads / rfc / 9324

RFC patch, 2 partsgitweb: Add an option to show size of blobs in the tree view

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

## tl;dr

11 messages between Jul 31, 2007 and Aug 2, 2007. Diffs are folded; open one to read it.

replies: 10people: 2as markdown or json

Jakub Narebski· Jul 31, 2007, 11:19 UTC · lore

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

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(-)
Show changes to gitweb/gitweb.perl +7 −1
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
Jakub Narebski· Jul 31, 2007, 11:19 UTC · re: Jakub Narebski · lore

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(-)
Show changes to 2 files +44 −14

gitweb/gitweb.css, gitweb/gitweb.perl

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
Junio C Hamano· Jul 31, 2007, 22:12 UTC · re: Jakub Narebski · lore

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

Jakub Narebski <jnareb@gmail.com> writes:
Show 10 quoted lines
> 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.

Jakub Narebski· Aug 1, 2007, 13:05 UTC · re: Junio C Hamano · lore

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

On Wed, 1 Aug 2007, Junio C Hamano wrote:
Show 18 quoted lines
> 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
Junio C Hamano· Aug 1, 2007, 23:12 UTC · re: Jakub Narebski · lore

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

Jakub Narebski <jnareb@gmail.com> writes:
Show 11 quoted lines
> 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.
Jakub Narebski· Aug 1, 2007, 23:58 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 17 quoted lines
> 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'?
Show 5 quoted lines
>> 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
Junio C Hamano· Aug 2, 2007, 00:47 UTC · re: Jakub Narebski · lore

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

Jakub Narebski <jnareb@gmail.com> writes:
Show 5 quoted lines
> 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.

Junio C Hamano· Aug 2, 2007, 05:22 UTC · re: Jakub Narebski · lore

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

Jakub Narebski <jnareb@gmail.com> writes:
Show 7 quoted lines
>>> 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.

Jakub Narebski· Aug 2, 2007, 10:47 UTC · re: Junio C Hamano · lore

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

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
Junio C Hamano· Aug 2, 2007, 20:47 UTC · re: Jakub Narebski · lore

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

Jakub Narebski <jnareb@gmail.com> writes:
Show 17 quoted lines
> 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.

← back to recent threads