[PATCH 0/4] gitweb: Improving tree view (plus some cleanups)

[PATCH 0/4] gitweb: Improving tree view (plus some cleanups)

[Jakub Narebski]

This series of patches is road to patch introducing '..'
(up directory) link in "tree" view in gitweb; patches 1-3
corrects errors or inconistencies noticed in code generating
"tree" view.

Shortlog:
 [PATCH 1/4] gitweb: Whitespace cleanup - tabs are for indent, spaces are for align (2)
 [PATCH 2/4] gitweb: Do not esc_html $basedir argument to git_print_tree_entry
 [PATCH 3/4] gitweb: Improve git_print_page_path
 [PATCH 4/4] gitweb: Add '..' (up directory) to tree view if applicable.

Diffstat:
---
 gitweb/gitweb.perl |  109 ++++++++++++++++++++++++++++++++--------------------
 1 files changed, 68 insertions(+), 41 deletions(-)

-- 
Jakub Narebski
ShadeHawk on #git
Poland

[PATCH 1/4] gitweb: Whitespace cleanup - tabs are for indent, spaces are for align (2)

[Jakub Narebski]

Code should be aligned the same way, regardless of tab size.
Use tabs for indent, but spaces for align.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |   60 ++++++++++++++++++++++++++--------------------------
 1 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 035e85e..c457884 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1722,13 +1722,13 @@ sub git_print_tree_entry {
 	if ($t->{'type'} eq "blob") {
 		print "<td class=\"list\">" .
 			$cgi->a({-href => href(action=>"blob", hash=>$t->{'hash'},
-					       file_name=>"$basedir$t->{'name'}", %base_key),
-				 -class => "list"}, esc_html($t->{'name'})) . "</td>\n";
+			                       file_name=>"$basedir$t->{'name'}", %base_key),
+			        -class => "list"}, esc_html($t->{'name'})) . "</td>\n";
 		print "<td class=\"link\">";
 		if ($have_blame) {
 			print $cgi->a({-href => href(action=>"blame", hash=>$t->{'hash'},
-						     file_name=>"$basedir$t->{'name'}", %base_key)},
-				      "blame");
+				                           file_name=>"$basedir$t->{'name'}", %base_key)},
+				            "blame");
 		}
 		if (defined $hash_base) {
 			if ($have_blame) {
@@ -1740,8 +1740,8 @@ sub git_print_tree_entry {
 		}
 		print " | " .
 			$cgi->a({-href => href(action=>"blob_plain", hash_base=>$hash_base,
-					       file_name=>"$basedir$t->{'name'}")},
-				"raw");
+			                       file_name=>"$basedir$t->{'name'}")},
+			        "raw");
 		print "</td>\n";
 
 	} elsif ($t->{'type'} eq "tree") {
@@ -1809,7 +1809,7 @@ sub git_difftree_body {
 			print "<td>";
 			print $cgi->a({-href => href(action=>"blob", hash=>$diff{'to_id'},
 			                             hash_base=>$hash, file_name=>$diff{'file'}),
-				       -class => "list"}, esc_html($diff{'file'}));
+			              -class => "list"}, esc_html($diff{'file'}));
 			print "</td>\n";
 			print "<td>$mode_chng</td>\n";
 			print "<td class=\"link\">";
@@ -1836,11 +1836,11 @@ sub git_difftree_body {
 				print " | ";
 			}
 			print $cgi->a({-href => href(action=>"blame", hash_base=>$parent,
-						     file_name=>$diff{'file'})},
-				      "blame") . " | ";
+			                             file_name=>$diff{'file'})},
+			              "blame") . " | ";
 			print $cgi->a({-href => href(action=>"history", hash_base=>$parent,
-						     file_name=>$diff{'file'})},
-				      "history");
+			                             file_name=>$diff{'file'})},
+			              "history");
 			print "</td>\n";
 
 		} elsif ($diff{'status'} eq "M" || $diff{'status'} eq "T") { # modified, or type changed
@@ -1861,8 +1861,8 @@ sub git_difftree_body {
 			}
 			print "<td>";
 			print $cgi->a({-href => href(action=>"blob", hash=>$diff{'to_id'},
-						     hash_base=>$hash, file_name=>$diff{'file'}),
-				       -class => "list"}, esc_html($diff{'file'}));
+			                             hash_base=>$hash, file_name=>$diff{'file'}),
+			              -class => "list"}, esc_html($diff{'file'}));
 			print "</td>\n";
 			print "<td>$mode_chnge</td>\n";
 			print "<td class=\"link\">";
@@ -1873,19 +1873,19 @@ sub git_difftree_body {
 					print $cgi->a({-href => "#patch$patchno"}, "patch");
 				} else {
 					print $cgi->a({-href => href(action=>"blobdiff",
-								     hash=>$diff{'to_id'}, hash_parent=>$diff{'from_id'},
-								     hash_base=>$hash, hash_parent_base=>$parent,
-								     file_name=>$diff{'file'})},
-						      "diff");
+					                             hash=>$diff{'to_id'}, hash_parent=>$diff{'from_id'},
+					                             hash_base=>$hash, hash_parent_base=>$parent,
+					                             file_name=>$diff{'file'})},
+					              "diff");
 				}
 				print " | ";
 			}
 			print $cgi->a({-href => href(action=>"blame", hash_base=>$hash,
-						     file_name=>$diff{'file'})},
-				      "blame") . " | ";
+			                             file_name=>$diff{'file'})},
+			              "blame") . " | ";
 			print $cgi->a({-href => href(action=>"history", hash_base=>$hash,
-						     file_name=>$diff{'file'})},
-				      "history");
+			                             file_name=>$diff{'file'})},
+			              "history");
 			print "</td>\n";
 
 		} elsif ($diff{'status'} eq "R" || $diff{'status'} eq "C") { # renamed or copied
@@ -1913,19 +1913,19 @@ sub git_difftree_body {
 					print $cgi->a({-href => "#patch$patchno"}, "patch");
 				} else {
 					print $cgi->a({-href => href(action=>"blobdiff",
-								     hash=>$diff{'to_id'}, hash_parent=>$diff{'from_id'},
-								     hash_base=>$hash, hash_parent_base=>$parent,
-								     file_name=>$diff{'to_file'}, file_parent=>$diff{'from_file'})},
-						      "diff");
+					                             hash=>$diff{'to_id'}, hash_parent=>$diff{'from_id'},
+					                             hash_base=>$hash, hash_parent_base=>$parent,
+					                             file_name=>$diff{'to_file'}, file_parent=>$diff{'from_file'})},
+					              "diff");
 				}
 				print " | ";
 			}
 			print $cgi->a({-href => href(action=>"blame", hash_base=>$parent,
-						     file_name=>$diff{'from_file'})},
-				      "blame") . " | ";
+			                             file_name=>$diff{'from_file'})},
+			              "blame") . " | ";
 			print $cgi->a({-href => href(action=>"history", hash_base=>$parent,
-						     file_name=>$diff{'from_file'})},
-				      "history");
+			                            file_name=>$diff{'from_file'})},
+			              "history");
 			print "</td>\n";
 
 		} # we should not encounter Unmerged (U) or Unknown (X) status
@@ -2851,7 +2851,7 @@ sub git_tree {
 			# FIXME: Should be available when we have no hash base as well.
 			push @views_nav,
 				$cgi->a({-href => href(action=>"snapshot", hash=>$hash)},
-					"snapshot");
+				        "snapshot");
 		}
 		git_print_page_nav('tree','', $hash_base, undef, undef, join(' | ', @views_nav));
 		git_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash_base);
-- 
1.4.2.1

[PATCH 2/4] gitweb: Do not esc_html $basedir argument to git_print_tree_entry

[Jakub Narebski]

In git_tree, rename $base variable (which is passed as $basedir
argument to git_print_tree_entry) to $basedir. Do not esc_html
$basedir, as it is part of file_name ('f') argument in link and not
printed. Add '/' at the end only if $basedir is not empty (it is empty
for top directory) and doesn't end in '/' already.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
BUGFIX patch.

 gitweb/gitweb.perl |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index c457884..209b318 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2834,7 +2834,7 @@ sub git_tree {
 	my $refs = git_get_references();
 	my $ref = format_ref_marker($refs, $hash_base);
 	git_header_html();
-	my $base = "";
+	my $basedir = '';
 	my ($have_blame) = gitweb_check_feature('blame');
 	if (defined $hash_base && (my %co = parse_commit($hash_base))) {
 		my @views_nav = ();
@@ -2862,7 +2862,10 @@ sub git_tree {
 		print "<div class=\"title\">$hash</div>\n";
 	}
 	if (defined $file_name) {
-		$base = esc_html("$file_name/");
+		$basedir = $file_name;
+		if ($basedir ne '' && substr($basedir, -1) ne '/') {
+			$basedir .= '/';
+		}
 	}
 	git_print_page_path($file_name, 'tree', $hash_base);
 	print "<div class=\"page_body\">\n";
@@ -2878,7 +2881,7 @@ sub git_tree {
 		}
 		$alternate ^= 1;
 
-		git_print_tree_entry(\%t, $base, $hash_base, $have_blame);
+		git_print_tree_entry(\%t, $basedir, $hash_base, $have_blame);
 
 		print "</tr>\n";
 	}
-- 
1.4.2.1

[PATCH 3/4] gitweb: Improve git_print_page_path

[Jakub Narebski]

Add link to "tree root" (root directory) also for not defined name,
for example for "tree" action without defined "file_name" which means
"tree root".

Add " / " at the end of path when $type eq "tree".

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Consistency (and code simplification).

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 209b318..126cf3c 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1615,17 +1615,16 @@ sub git_print_page_path {
 	my $type = shift;
 	my $hb = shift;
 
-	if (!defined $name) {
-		print "<div class=\"page_path\">/</div>\n";
-	} else {
+
+	print "<div class=\"page_path\">";
+	print $cgi->a({-href => href(action=>"tree", hash_base=>$hb),
+	              -title => 'tree root'}, "[$project]");
+	print " / ";
+	if (defined $name) {
 		my @dirname = split '/', $name;
 		my $basename = pop @dirname;
 		my $fullname = '';
 
-		print "<div class=\"page_path\">";
-		print $cgi->a({-href => href(action=>"tree", hash_base=>$hb),
-			      -title => 'tree root'}, "[$project]");
-		print " / ";
 		foreach my $dir (@dirname) {
 			$fullname .= ($fullname ? '/' : '') . $dir;
 			print $cgi->a({-href => href(action=>"tree", file_name=>$fullname,
@@ -1641,11 +1640,12 @@ sub git_print_page_path {
 			print $cgi->a({-href => href(action=>"tree", file_name=>$file_name,
 			                             hash_base=>$hb),
 			              -title => $name}, esc_html($basename));
+			print " / ";
 		} else {
 			print esc_html($basename);
 		}
-		print "<br/></div>\n";
 	}
+	print "<br/></div>\n";
 }
 
 # sub git_print_log (\@;%) {
-- 
1.4.2.1

[PATCH 4/4] gitweb: Add '..' (up directory) to tree view if applicable

[Jakub Narebski]

Adds '..' (up directory) link at the top of "tree" view listing,
if both $hash_base and $file_name are provided, and $file_name
is not empty string.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This was meant to send as single patch...

 gitweb/gitweb.perl |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 126cf3c..c9e57f0 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2871,6 +2871,30 @@ sub git_tree {
 	print "<div class=\"page_body\">\n";
 	print "<table cellspacing=\"0\">\n";
 	my $alternate = 1;
+	# '..' (top directory) link if possible
+	if (defined $hash_base &&
+	    defined $file_name && $file_name =~ m![^/]+$!) {
+		if ($alternate) {
+			print "<tr class=\"dark\">\n";
+		} else {
+			print "<tr class=\"light\">\n";
+		}
+		$alternate ^= 1;
+
+		my $up = $file_name;
+		$up =~ s!/?[^/]+$!!;
+		undef $up unless $up;
+		# 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 "</td>\n";
+		print "<td class=\"link\"></td>\n";
+
+		print "</tr>\n";
+	}
 	foreach my $line (@entries) {
 		my %t = parse_ls_tree_line($line, -z => 1);
 
-- 
1.4.2.1

Re: [PATCH 1/4] gitweb: Whitespace cleanup - tabs are for indent, spaces are for align (2)

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Code should be aligned the same way, regardless of tab size.
> Use tabs for indent, but spaces for align.

I do not necessarily agree with that policy; the result of
applying this patch is still inconsistent in some places, and I
think that is primarily because the policy itself is flawed.

For example, a part of sub format_paging_nav looks like this:

        sub format_paging_nav {
        >>>>>>>>my ($action, $hash, $head, $page, $nrevs) = @_;
        ...
        >>>>>>>>if ($page > 0) {
        >>>>>>>>>>>>>>>>$paging_nav .= " &sdot; " .
        >>>>>>>>>>>>>>>>>>>>>>>>$cgi->a({-href => href(action=>$a
        >>>>>>>>>>>>>>>>>>>>>>>>         -accesskey => "p", -titl
        >>>>>>>>} else {
        ...

If your policy is to indent continuation lines (which is why you
have a TAB before "$cgi->a"), not having a TAB before the
continued parameter list for the $cgi->a() call look inconsistent.

If on the other hand your policy is to align parameters to an
operator that are spread over multiple lines, " &sdot; " and
"$cgi-a(..." are left and right parameters to the string
concatenation operator "." in between them, so "$cgi->a" should
be pushed back with a run of SP starting at the column that
begins $paging_nav and aligned with the DQ at the beginning of
the " &sdot; " string.

By the way, is there a handy way to view something like the
above with "cat" (like "cat -e" is an easy way to find trailing
whitespace problems)?  I usually end up running this but I feel
that there ought to be a canned command.

#!/usr/bin/perl

my $monochrome = 0;
my $tab = 8;
my $tab_color = "\033[44m";
my $reset_color = "\033[0m";
my $tab_padchar = ' ';

if ($monochrome) {
	$tab_color = $reset_color = '';
	$tab_padchar = '>';
}

while (<>) {
	chomp;
	my (@frag) = split(/\t/, $_);
	my $pos = 0;
	for (my $i = 0; $i < @frag; $i++) {
		if ($i) {
			my $len = 8 - $pos % 8;
			print $tab_color;
			print $tab_padchar x $len;
			print $reset_color;
			$pos += $len;
		}
		print $frag[$i];
		$pos += length($frag[$i]);

	}
	print "\n";
}

Re: [PATCH 1/4] gitweb: Whitespace cleanup - tabs are for indent, spaces are for align (2)

[Jakub Narebski]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> Code should be aligned the same way, regardless of tab size.
>> Use tabs for indent, but spaces for align.
> 
> I do not necessarily agree with that policy; the result of
> applying this patch is still inconsistent in some places, and I
> think that is primarily because the policy itself is flawed.

To be true I do those "whitespace cleanup" patches when I notice
that something is mis-aligned for _my_ tab width (2 spaces).
I use Emacs with show-whitespace-mode to not introduce errors
which would be aligned for 2 columsn wide tabs but be misaligned
for more common 5 or 8 characters wide tabs.
 
> For example, a part of sub format_paging_nav looks like this:
> 
>         sub format_paging_nav {
>         >>>>>>>>my ($action, $hash, $head, $page, $nrevs) = @_;
>         ...
>         >>>>>>>>if ($page > 0) {
>         >>>>>>>>>>>>>>>>$paging_nav .= " &sdot; " .
>         >>>>>>>>>>>>>>>>>>>>>>>>$cgi->a({-href => href(action=>$a
>         >>>>>>>>>>>>>>>>>>>>>>>>         -accesskey => "p", -titl
>         >>>>>>>>} else {
>         ...
> 
> If your policy is to indent continuation lines (which is why you
> have a TAB before "$cgi->a"), not having a TAB before the
> continued parameter list for the $cgi->a() call look inconsistent.
> 
> If on the other hand your policy is to align parameters to an
> operator that are spread over multiple lines, " &sdot; " and
> "$cgi-a(..." are left and right parameters to the string
> concatenation operator "." in between them, so "$cgi->a" should
> be pushed back with a run of SP starting at the column that
> begins $paging_nav and aligned with the DQ at the beginning of
> the " &sdot; " string.

That is preferred, and usually used policy, although I sometimes
use former to avoid too long lines.

I never claimed to be perfect...

Nevertheless this patch for example correct situation where the
line which should be aligned is aligned using mixture of tabs and
spaces differing from line to line in the "same alignment" block.

-- 
Jakub Narebski
Poland

Re: [PATCH 1/4] gitweb: Whitespace cleanup - tabs are for indent, spaces are for align (2)

[Linus Torvalds]

On Sun, 22 Oct 2006, Jakub Narebski wrote:
> 
> To be true I do those "whitespace cleanup" patches when I notice
> that something is mis-aligned for _my_ tab width (2 spaces).

Oh, wow.

You have clearly been damaged by some nasty GNU coding style or similar.

Please immediately turn tabs into 8 character hard-tabs, and read the 
kernel Documentation/CodingStyle document.

After that, you may have to go to some detox unit to purge yourself of the 
habit of 2-character indents, and I realize it will be hard and you'll 
feel like shit for a while, but you'll be a better person for it.

It's like kicking a bad drug habit. It may be painful for a while, and you 
might wistfully think back on the "happy days" when things went by in a 
haze of unclear indentations, but really, it's worth it.

			Linus

Re: [PATCH 1/4] gitweb: Whitespace cleanup - tabs are for indent, spaces are for align (2)

[Jakub Narebski]

Linus Torvalds wrote:
> 
> On Sun, 22 Oct 2006, Jakub Narebski wrote:
> > 
> > To be true I do those "whitespace cleanup" patches when I notice
> > that something is mis-aligned for _my_ tab width (2 spaces).
> 
> Oh, wow.
> 
> You have clearly been damaged by some nasty GNU coding style or similar.
> 
> Please immediately turn tabs into 8 character hard-tabs, and read the 
> kernel Documentation/CodingStyle document.

2 character wide spaces are minimal width indent one can work with.
And to reduce line width I use the minimal indent with. Yes, I know
that I should correct coding style instead...

Using 2 characters wide tabs for gitweb stems also from the fact
that I could not configure Emacs and cperl-mode to automatically
indent with tabs (probably because many Emacsers think that
TabsAreEvil); I should probably set some indent/offset variable
to the tab-width value, or something...
-- 
Jakub Narebski
Poland

Re: [PATCH 1/4] gitweb: Whitespace cleanup - tabs are for indent, spaces are for align (2)

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> On Sun, 22 Oct 2006, Jakub Narebski wrote:
>> 
>> To be true I do those "whitespace cleanup" patches when I notice
>> that something is mis-aligned for _my_ tab width (2 spaces).
>
> Oh, wow.
>
> You have clearly been damaged by some nasty GNU coding style or similar.
>
> Please immediately turn tabs into 8 character hard-tabs, and read the 
> kernel Documentation/CodingStyle document.

Offtopic comments.

I am from old school, so a TAB to me is always equivalent to a
run of spaces that pads up to the next column that is dividible
by 8, but there is one valid reason to occasionally take a look
at your code with unusual TAB settings to catch coding style
violations, if you _were_ following an indentation policy that
is different from what the kernel and git uses.

Aside from the textual comparison order habit (aka "never use >
or >= comparison operators") that everybody hates (and I've been
trying not to inroduce new ones, albeit slowly), I have picked
up two other "unusual" coding style habits, which I deliberately
have stayed away from applying to the code I write for git.

They both relate to the way the code is indented; unlike the
textual order comparison one, they do not affect readability to
me that much and it is easier for me to shake them when I want
to be.

They are:

 - There is no "aligning" between two lines, other than the case
   in which they start with _identical_ substring.  Not just
   that a TAB does not necessarily look the same as spaces to
   the next display column that is divisible by 8 by everybody;
   think about proportinal fonts.

   When you take this position, there is only one thing you can
   do when your logical line extends over more than one physical
   lines because it is too long.  Since there is no aligning,
   the only sensible thing to do is to indent it one level
   deeper than the first physical line of the logical line.
   Your indentation becomes like this:

   <BOL><TAB>if (very long expression that extends over<EOL>
   <BOL><TAB><TAB>more than one line) {<EOL>
   <BOL><TAB><TAB>do this and do that...<EOL>
   <BOL><TAB>}<EOL>

   When you are following this principle (and git sources do not
   and should not), viewing your code with an unusual tab width
   is a good way to check the coding style violation.

 - If you have to split a long expression over multiple lines,
   split the line _before_ a binary operator, not after, so when
   you turn your head sideways 90 degrees, you would see the
   parse tree of the expression:

        foo = a-term
                * (a-very-very-long-expression-term
                        + yet-another-term);

   If there _were_ alignment, this would perhaps look like this:

        foo =     a-term
                * (       a-very-very-long-expression-term
                        + yet-another-term);

Re: [PATCH 1/4] gitweb: Whitespace cleanup - tabs are for indent, spaces are for align (2)

[Luben Tuikov]

--- Linus Torvalds <torvalds@osdl.org> wrote:
> On Sun, 22 Oct 2006, Jakub Narebski wrote:
> > 
> > To be true I do those "whitespace cleanup" patches when I notice
> > that something is mis-aligned for _my_ tab width (2 spaces).
> 
> Oh, wow.
> 
> You have clearly been damaged by some nasty GNU coding style or similar.
> 
> Please immediately turn tabs into 8 character hard-tabs, and read the 
> kernel Documentation/CodingStyle document.
> 
> After that, you may have to go to some detox unit to purge yourself of the 
> habit of 2-character indents, and I realize it will be hard and you'll 
> feel like shit for a while, but you'll be a better person for it.
> 
> It's like kicking a bad drug habit. It may be painful for a while, and you 
> might wistfully think back on the "happy days" when things went by in a 
> haze of unclear indentations, but really, it's worth it.

Funny, but true.

One way to do it is:

(defun my-perl-mode ()
  (setq perl-indent-level 8)
  (setq perl-continued-statement-offset 8)
)

(add-hook 'perl-mode-hook 'my-perl-mode)

     Luben