[PATCH] gitweb: make leftmost column of blame less cluttered.

[PATCH] gitweb: make leftmost column of blame less cluttered.

[Junio C Hamano]

Instead of labelling each and every line with clickable commit
object name, this makes the blame output to show them only on
the first line of each group of lines from the same revision.

Also it makes mouse-over to show the minimum authorship and
authordate information for extra cuteness ;-).

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

 * I've been staying away from the party to paint the bikeshed,
   but I had a bit of time to kill tonight.  Let's see if people
   might like this...

 gitweb/gitweb.perl |   67 +++++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 54 insertions(+), 13 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 44991b1..7e4ec8d 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2429,6 +2429,41 @@ sub git_tag {
 	git_footer_html();
 }
 
+sub git_blame_flush_chunk {
+	my ($name, $revdata, $color, $rev, @line) = @_;
+	my $label = substr($rev, 0, 8);
+	my $line = scalar(@line);
+	my $cnt = 0;
+	my $pop = '';
+
+	if ($revdata->{$rev} ne '') {
+		$pop = ' title="' . esc_html($revdata->{$rev}) . '"';
+	}
+
+	for (@line) {
+		my ($lineno, $data) = @$_;
+		$cnt++;
+		print "<tr class=\"$color\">\n";
+		if ($cnt == 1) {
+			print "<td class=\"sha1\"$pop";
+			if ($line > 1) {
+				print " rowspan=\"$line\"";
+			}
+			print ">";
+			print $cgi->a({-href => href(action=>"commit",
+						     hash=>$rev,
+						     file_name=>$name)},
+				      $label);
+			print "</td>\n";
+		}
+		print "<td class=\"linenr\">".
+		    "<a id=\"l$lineno\" href=\"#l$lineno\" class=\"linenr\">" .
+		    esc_html($lineno) . "</a></td>\n";
+		print "<td class=\"pre\">" . esc_html($data) . "</td>\n";
+		print "</tr>\n";
+	}
+}
+
 sub git_blame2 {
 	my $fd;
 	my $ftype;
@@ -2474,27 +2509,33 @@ sub git_blame2 {
 <table class="blame">
 <tr><th>Commit</th><th>Line</th><th>Data</th></tr>
 HTML
+	my @chunk = ();
+	my %revdata = ();
 	while (<$fd>) {
 		/^([0-9a-fA-F]{40}).*?(\d+)\)\s{1}(\s*.*)/;
-		my $full_rev = $1;
-		my $rev = substr($full_rev, 0, 8);
-		my $lineno = $2;
-		my $data = $3;
-
+		my ($full_rev, $author, $date, $lineno, $data) =
+		    /^([0-9a-f]{40}).*?\s\((.*?)\s+([-\d]+ [:\d]+ [-+\d]+)\s+(\d+)\)\s(.*)/;
+		if (!exists $revdata{$full_rev}) {
+			$revdata{$full_rev} = "$author, $date";
+		}
 		if (!defined $last_rev) {
 			$last_rev = $full_rev;
 		} elsif ($last_rev ne $full_rev) {
+			git_blame_flush_chunk($file_name,
+					      \%revdata,
+					      $rev_color[$current_color],
+					      $last_rev, @chunk);
+			@chunk = ();
 			$last_rev = $full_rev;
 			$current_color = ++$current_color % $num_colors;
 		}
-		print "<tr class=\"$rev_color[$current_color]\">\n";
-		print "<td class=\"sha1\">" .
-			$cgi->a({-href => href(action=>"commit", hash=>$full_rev, file_name=>$file_name)},
-			        esc_html($rev)) . "</td>\n";
-		print "<td class=\"linenr\"><a id=\"l$lineno\" href=\"#l$lineno\" class=\"linenr\">" .
-		      esc_html($lineno) . "</a></td>\n";
-		print "<td class=\"pre\">" . esc_html($data) . "</td>\n";
-		print "</tr>\n";
+		push @chunk, [$lineno, $data];
+	}
+	if (@chunk) {
+		git_blame_flush_chunk($file_name,
+				      \%revdata,
+				      $rev_color[$current_color],
+				      $last_rev, @chunk);
 	}
 	print "</table>\n";
 	print "</div>";
-- 
1.4.2.1.gc9fffe

Re: [PATCH] gitweb: make leftmost column of blame less cluttered.

[Luben Tuikov]

--- Junio C Hamano <junkio@cox.net> wrote:

> Instead of labelling each and every line with clickable commit
> object name, this makes the blame output to show them only on
> the first line of each group of lines from the same revision.
> 
> Also it makes mouse-over to show the minimum authorship and
> authordate information for extra cuteness ;-).
> 
> Signed-off-by: Junio C Hamano <junkio@cox.net>
> ---

ACK.  Please commit.

   Luben

> 
>  * I've been staying away from the party to paint the bikeshed,
>    but I had a bit of time to kill tonight.  Let's see if people
>    might like this...
> 
>  gitweb/gitweb.perl |   67 +++++++++++++++++++++++++++++++++++++++++----------
>  1 files changed, 54 insertions(+), 13 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 44991b1..7e4ec8d 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2429,6 +2429,41 @@ sub git_tag {
>  	git_footer_html();
>  }
>  
> +sub git_blame_flush_chunk {
> +	my ($name, $revdata, $color, $rev, @line) = @_;
> +	my $label = substr($rev, 0, 8);
> +	my $line = scalar(@line);
> +	my $cnt = 0;
> +	my $pop = '';
> +
> +	if ($revdata->{$rev} ne '') {
> +		$pop = ' title="' . esc_html($revdata->{$rev}) . '"';
> +	}
> +
> +	for (@line) {
> +		my ($lineno, $data) = @$_;
> +		$cnt++;
> +		print "<tr class=\"$color\">\n";
> +		if ($cnt == 1) {
> +			print "<td class=\"sha1\"$pop";
> +			if ($line > 1) {
> +				print " rowspan=\"$line\"";
> +			}
> +			print ">";
> +			print $cgi->a({-href => href(action=>"commit",
> +						     hash=>$rev,
> +						     file_name=>$name)},
> +				      $label);
> +			print "</td>\n";
> +		}
> +		print "<td class=\"linenr\">".
> +		    "<a id=\"l$lineno\" href=\"#l$lineno\" class=\"linenr\">" .
> +		    esc_html($lineno) . "</a></td>\n";
> +		print "<td class=\"pre\">" . esc_html($data) . "</td>\n";
> +		print "</tr>\n";
> +	}
> +}
> +
>  sub git_blame2 {
>  	my $fd;
>  	my $ftype;
> @@ -2474,27 +2509,33 @@ sub git_blame2 {
>  <table class="blame">
>  <tr><th>Commit</th><th>Line</th><th>Data</th></tr>
>  HTML
> +	my @chunk = ();
> +	my %revdata = ();
>  	while (<$fd>) {
>  		/^([0-9a-fA-F]{40}).*?(\d+)\)\s{1}(\s*.*)/;
> -		my $full_rev = $1;
> -		my $rev = substr($full_rev, 0, 8);
> -		my $lineno = $2;
> -		my $data = $3;
> -
> +		my ($full_rev, $author, $date, $lineno, $data) =
> +		    /^([0-9a-f]{40}).*?\s\((.*?)\s+([-\d]+ [:\d]+ [-+\d]+)\s+(\d+)\)\s(.*)/;
> +		if (!exists $revdata{$full_rev}) {
> +			$revdata{$full_rev} = "$author, $date";
> +		}
>  		if (!defined $last_rev) {
>  			$last_rev = $full_rev;
>  		} elsif ($last_rev ne $full_rev) {
> +			git_blame_flush_chunk($file_name,
> +					      \%revdata,
> +					      $rev_color[$current_color],
> +					      $last_rev, @chunk);
> +			@chunk = ();
>  			$last_rev = $full_rev;
>  			$current_color = ++$current_color % $num_colors;
>  		}
> -		print "<tr class=\"$rev_color[$current_color]\">\n";
> -		print "<td class=\"sha1\">" .
> -			$cgi->a({-href => href(action=>"commit", hash=>$full_rev, file_name=>$file_name)},
> -			        esc_html($rev)) . "</td>\n";
> -		print "<td class=\"linenr\"><a id=\"l$lineno\" href=\"#l$lineno\" class=\"linenr\">" .
> -		      esc_html($lineno) . "</a></td>\n";
> -		print "<td class=\"pre\">" . esc_html($data) . "</td>\n";
> -		print "</tr>\n";
> +		push @chunk, [$lineno, $data];
> +	}
> +	if (@chunk) {
> +		git_blame_flush_chunk($file_name,
> +				      \%revdata,
> +				      $rev_color[$current_color],
> +				      $last_rev, @chunk);
>  	}
>  	print "</table>\n";
>  	print "</div>";
> -- 
> 1.4.2.1.gc9fffe
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] gitweb: make leftmost column of blame less cluttered.

[Junio C Hamano]

Luben Tuikov <ltuikov@yahoo.com> writes:

> --- Junio C Hamano <junkio@cox.net> wrote:
>
>> Instead of labelling each and every line with clickable commit
>> object name, this makes the blame output to show them only on
>> the first line of each group of lines from the same revision.
>> 
>> Also it makes mouse-over to show the minimum authorship and
>> authordate information for extra cuteness ;-).
>> 
>> Signed-off-by: Junio C Hamano <junkio@cox.net>
>> ---
>
> ACK.  Please commit.

Won't, at least as its current shape.  Somebody privately
mentioned that the code risks slurping the entire file in the
@chunk if it is untouched since the initial import, which is not
what we want.

The memory consumption worries aside, that would make the
clickable commit object name to appear only very at the
beginning of the page and would make it inconvenient to actually
visit the commit after scrolling down to see later lines.

It might become usable if it is given a cap to limit the number
of lines to put in a chunk.  I dunno.

Re: [PATCH] gitweb: make leftmost column of blame less cluttered.

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> Luben Tuikov <ltuikov@yahoo.com> writes:
>> --- Junio C Hamano <junkio@cox.net> wrote:
>>
>>> Instead of labelling each and every line with clickable commit
>>> object name, this makes the blame output to show them only on
>>> the first line of each group of lines from the same revision.
>>...
>> ACK.  Please commit.
>
> Won't, at least as its current shape.  Somebody privately
> mentioned that the code risks slurping the entire file in the
> @chunk if it is untouched since the initial import, which is not
> what we want.
>
> The memory consumption worries aside, that would make the
> clickable commit object name to appear only very at the
> beginning of the page and would make it inconvenient to actually
> visit the commit after scrolling down to see later lines.
>
> It might become usable if it is given a cap to limit the number
> of lines to put in a chunk.  I dunno.

Perhaps like this.  This has hardcoded chunk-cap of 20 lines,
which means we never buffer more than 40 lines and never emit
more than 20 lines at a time.  The reason we do not immediately
flush after getting 20 lines is if we have a block of 21 lines
we would end up giving 20 line chunk and 1 line chunk and then a
chunk for the different revision (orphaned line).  In such a
case we are better off giving two evenly divided chunks and that
is why we buffer up to twice the chunk-cap size.

If people are interested we probably would want to make 20
configurable.  Maybe not.  I personally suspect it does not
matter much.

-- >8 --
[PATCH] gitweb: make leftmost column of blame less cluttered.

Instead of labelling each and every line with clickable commit
object name, this makes the blame output to show them only on
the first line of each group of lines from the same revision.

Also it makes mouse-over to show the minimum authorship and
authordate information for extra cuteness ;-).

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 gitweb/gitweb.perl |   99 +++++++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 86 insertions(+), 13 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
old mode 100755
new mode 100644
index 3e9d4a0..55d1b2c
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2430,9 +2430,64 @@ sub git_tag {
 	git_footer_html();
 }
 
+sub git_blame_flush_chunk {
+	my ($name, $revdata, $color, $rev, @line) = @_;
+	my $label = substr($rev, 0, 8);
+	my $line = scalar(@line);
+	my $cnt = 0;
+	my $pop = '';
+
+	if ($revdata->{$rev} ne '') {
+		$pop = ' title="' . esc_html($revdata->{$rev}) . '"';
+	}
+
+	for (@line) {
+		my ($lineno, $data) = @$_;
+		$cnt++;
+		print "<tr class=\"$color\">\n";
+		if ($cnt == 1) {
+			print "<td class=\"sha1\"$pop";
+			if ($line > 1) {
+				print " rowspan=\"$line\"";
+			}
+			print ">";
+			print $cgi->a({-href => href(action=>"commit",
+						     hash=>$rev,
+						     file_name=>$name)},
+				      $label);
+			print "</td>\n";
+		}
+		print "<td class=\"linenr\">".
+		    "<a id=\"l$lineno\" href=\"#l$lineno\" class=\"linenr\">" .
+		    esc_html($lineno) . "</a></td>\n";
+		print "<td class=\"pre\">" . esc_html($data) . "</td>\n";
+		print "</tr>\n";
+	}
+}
+
+# We can have up to N*2 lines.  If it is more than N lines, split it
+# into two to avoid orphans.
+sub git_blame_flush_chunk_1 {
+	my ($chunk_cap, $name, $revdata, $color, $rev, @chunk) = @_;
+	if ($chunk_cap < @chunk) {
+		my @first = splice(@chunk, 0, @chunk/2);
+		git_blame_flush_chunk($name,
+				      $revdata,
+				      $color,
+				      $rev,
+				      @first);
+	}
+	git_blame_flush_chunk($name,
+			      $revdata,
+			      $color,
+			      $rev,
+			      @chunk);
+}
+
 sub git_blame2 {
 	my $fd;
 	my $ftype;
+	my $chunk_cap = 20;
 
 	my ($have_blame) = gitweb_check_feature('blame');
 	if (!$have_blame) {
@@ -2475,27 +2530,45 @@ sub git_blame2 {
 <table class="blame">
 <tr><th>Commit</th><th>Line</th><th>Data</th></tr>
 HTML
+	my @chunk = ();
+	my %revdata = ();
 	while (<$fd>) {
 		/^([0-9a-fA-F]{40}).*?(\d+)\)\s{1}(\s*.*)/;
-		my $full_rev = $1;
-		my $rev = substr($full_rev, 0, 8);
-		my $lineno = $2;
-		my $data = $3;
-
+		my ($full_rev, $author, $date, $lineno, $data) =
+		    /^([0-9a-f]{40}).*?\s\((.*?)\s+([-\d]+ [:\d]+ [-+\d]+)\s+(\d+)\)\s(.*)/;
+		if (!exists $revdata{$full_rev}) {
+			$revdata{$full_rev} = "$author, $date";
+		}
 		if (!defined $last_rev) {
 			$last_rev = $full_rev;
 		} elsif ($last_rev ne $full_rev) {
+			git_blame_flush_chunk_1($chunk_cap,
+						$file_name,
+						\%revdata,
+						$rev_color[$current_color],
+						$last_rev, @chunk);
+			@chunk = ();
 			$last_rev = $full_rev;
 			$current_color = ++$current_color % $num_colors;
 		}
-		print "<tr class=\"$rev_color[$current_color]\">\n";
-		print "<td class=\"sha1\">" .
-			$cgi->a({-href => href(action=>"commit", hash=>$full_rev, file_name=>$file_name)},
-			        esc_html($rev)) . "</td>\n";
-		print "<td class=\"linenr\"><a id=\"l$lineno\" href=\"#l$lineno\" class=\"linenr\">" .
-		      esc_html($lineno) . "</a></td>\n";
-		print "<td class=\"pre\">" . esc_html($data) . "</td>\n";
-		print "</tr>\n";
+		elsif ($chunk_cap * 2 < @chunk) {
+			# We have more than N*2 lines from the same
+			# revision.  Flush N lines and leave N lines
+			# in @chunk to avoid orphaned lines.
+			my @first = splice(@chunk, 0, $chunk_cap);
+			git_blame_flush_chunk($file_name,
+					      \%revdata,
+					      $rev_color[$current_color],
+					      $last_rev, @first);
+		}
+		push @chunk, [$lineno, $data];
+	}
+	if (@chunk) {
+		git_blame_flush_chunk_1($chunk_cap,
+					$file_name,
+					\%revdata,
+					$rev_color[$current_color],
+					$last_rev, @chunk);
 	}
 	print "</table>\n";
 	print "</div>";
-- 
1.4.2.3.gbe06

Re: [PATCH] gitweb: make leftmost column of blame less cluttered.

[Luben Tuikov]

--- Junio C Hamano <junkio@cox.net> wrote:
> Luben Tuikov <ltuikov@yahoo.com> writes:
> > ACK.  Please commit.
> 
> Won't, at least as its current shape.  Somebody privately
> mentioned that the code risks slurping the entire file in the
> @chunk if it is untouched since the initial import, which is not
> what we want.

Indeed that's true.

> The memory consumption worries aside, that would make the
> clickable commit object name to appear only very at the
> beginning of the page and would make it inconvenient to actually
> visit the commit after scrolling down to see later lines.
> 
> It might become usable if it is given a cap to limit the number
> of lines to put in a chunk.  I dunno.

I cannot imagine this to be a problem, since an initial commit
is "very well known" (hopefully that makes sense to people using
SCMs religously).

I'll give it try.
   Luben

Re: [PATCH] gitweb: make leftmost column of blame less cluttered.

[Luben Tuikov]

--- Junio C Hamano <junkio@cox.net> wrote:
> Perhaps like this.  This has hardcoded chunk-cap of 20 lines,
> which means we never buffer more than 40 lines and never emit
> more than 20 lines at a time.  The reason we do not immediately
> flush after getting 20 lines is if we have a block of 21 lines
> we would end up giving 20 line chunk and 1 line chunk and then a
> chunk for the different revision (orphaned line).  In such a
> case we are better off giving two evenly divided chunks and that
> is why we buffer up to twice the chunk-cap size.
> 
> If people are interested we probably would want to make 20
> configurable.  Maybe not.  I personally suspect it does not
> matter much.

I think this would get us into too much "special cases".  The colored
chunks of commits were added with the intention of "single commit",
so I think we should be ok, to have a single row at the top for
the initial commit when the file had not been changed since.

Initial commits are normally "well known commits", so I think it should
be ok to not have to print every 20 or so lines as this breaks the 
color-block-per-commit intention.

   Luben


> 
> -- >8 --
> [PATCH] gitweb: make leftmost column of blame less cluttered.
> 
> Instead of labelling each and every line with clickable commit
> object name, this makes the blame output to show them only on
> the first line of each group of lines from the same revision.
> 
> Also it makes mouse-over to show the minimum authorship and
> authordate information for extra cuteness ;-).
> 
> Signed-off-by: Junio C Hamano <junkio@cox.net>
> ---
>  gitweb/gitweb.perl |   99 +++++++++++++++++++++++++++++++++++++++++++++-------
>  1 files changed, 86 insertions(+), 13 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> old mode 100755
> new mode 100644
> index 3e9d4a0..55d1b2c
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2430,9 +2430,64 @@ sub git_tag {
>  	git_footer_html();
>  }
>  
> +sub git_blame_flush_chunk {
> +	my ($name, $revdata, $color, $rev, @line) = @_;
> +	my $label = substr($rev, 0, 8);
> +	my $line = scalar(@line);
> +	my $cnt = 0;
> +	my $pop = '';
> +
> +	if ($revdata->{$rev} ne '') {
> +		$pop = ' title="' . esc_html($revdata->{$rev}) . '"';
> +	}
> +
> +	for (@line) {
> +		my ($lineno, $data) = @$_;
> +		$cnt++;
> +		print "<tr class=\"$color\">\n";
> +		if ($cnt == 1) {
> +			print "<td class=\"sha1\"$pop";
> +			if ($line > 1) {
> +				print " rowspan=\"$line\"";
> +			}
> +			print ">";
> +			print $cgi->a({-href => href(action=>"commit",
> +						     hash=>$rev,
> +						     file_name=>$name)},
> +				      $label);
> +			print "</td>\n";
> +		}
> +		print "<td class=\"linenr\">".
> +		    "<a id=\"l$lineno\" href=\"#l$lineno\" class=\"linenr\">" .
> +		    esc_html($lineno) . "</a></td>\n";
> +		print "<td class=\"pre\">" . esc_html($data) . "</td>\n";
> +		print "</tr>\n";
> +	}
> +}
> +
> +# We can have up to N*2 lines.  If it is more than N lines, split it
> +# into two to avoid orphans.
> +sub git_blame_flush_chunk_1 {
> +	my ($chunk_cap, $name, $revdata, $color, $rev, @chunk) = @_;
> +	if ($chunk_cap < @chunk) {
> +		my @first = splice(@chunk, 0, @chunk/2);
> +		git_blame_flush_chunk($name,
> +				      $revdata,
> +				      $color,
> +				      $rev,
> +				      @first);
> +	}
> +	git_blame_flush_chunk($name,
> +			      $revdata,
> +			      $color,
> +			      $rev,
> +			      @chunk);
> +}
> +
>  sub git_blame2 {
>  	my $fd;
>  	my $ftype;
> +	my $chunk_cap = 20;
>  
>  	my ($have_blame) = gitweb_check_feature('blame');
>  	if (!$have_blame) {
> @@ -2475,27 +2530,45 @@ sub git_blame2 {
>  <table class="blame">
>  <tr><th>Commit</th><th>Line</th><th>Data</th></tr>
>  HTML
> +	my @chunk = ();
> +	my %revdata = ();
>  	while (<$fd>) {
>  		/^([0-9a-fA-F]{40}).*?(\d+)\)\s{1}(\s*.*)/;
> -		my $full_rev = $1;
> -		my $rev = substr($full_rev, 0, 8);
> -		my $lineno = $2;
> -		my $data = $3;
> -
> +		my ($full_rev, $author, $date, $lineno, $data) =
> +		    /^([0-9a-f]{40}).*?\s\((.*?)\s+([-\d]+ [:\d]+ [-+\d]+)\s+(\d+)\)\s(.*)/;
> +		if (!exists $revdata{$full_rev}) {
> +			$revdata{$full_rev} = "$author, $date";
> +		}
>  		if (!defined $last_rev) {
>  			$last_rev = $full_rev;
>  		} elsif ($last_rev ne $full_rev) {
> +			git_blame_flush_chunk_1($chunk_cap,
> +						$file_name,
> +						\%revdata,
> +						$rev_color[$current_color],
> +						$last_rev, @chunk);
> +			@chunk = ();
>  			$last_rev = $full_rev;
>  			$current_color = ++$current_color % $num_colors;
>  		}
> -		print "<tr class=\"$rev_color[$current_color]\">\n";
> -		print "<td class=\"sha1\">" .
> -			$cgi->a({-href => href(action=>"commit", hash=>$full_rev, file_name=>$file_name)},
> -			        esc_html($rev)) . "</td>\n";
> -		print "<td class=\"linenr\"><a id=\"l$lineno\" href=\"#l$lineno\" class=\"linenr\">" .
> -		      esc_html($lineno) . "</a></td>\n";
> -		print "<td class=\"pre\">" . esc_html($data) . "</td>\n";
> -		print "</tr>\n";
> +		elsif ($chunk_cap * 2 < @chunk) {
> +			# We have more than N*2 lines from the same
> +			# revision.  Flush N lines and leave N lines
> +			# in @chunk to avoid orphaned lines.
> +			my @first = splice(@chunk, 0, $chunk_cap);
> +			git_blame_flush_chunk($file_name,
> +					      \%revdata,
> +					      $rev_color[$current_color],
> +					      $last_rev, @first);
> +		}
> +		push @chunk, [$lineno, $data];
> +	}
> +	if (@chunk) {
> +		git_blame_flush_chunk_1($chunk_cap,
> +					$file_name,
> +					\%revdata,
> +					$rev_color[$current_color],
> +					$last_rev, @chunk);
>  	}
>  	print "</table>\n";
>  	print "</div>";
> -- 
> 1.4.2.3.gbe06
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] gitweb: make leftmost column of blame less cluttered.

[Martin Waitz]

[-- Attachment #1: Type: text/plain, Size: 670 bytes --]

hoi :)

On Tue, Oct 03, 2006 at 12:25:45AM -0700, Junio C Hamano wrote:
> [PATCH] gitweb: make leftmost column of blame less cluttered.
> 
> Instead of labelling each and every line with clickable commit
> object name, this makes the blame output to show them only on
> the first line of each group of lines from the same revision.
> 
> Also it makes mouse-over to show the minimum authorship and
> authordate information for extra cuteness ;-).

what about making the line itself clickable and adding the
mouse-over there, too?
Then you don't need to worry about repeating the commit id,
and perhaps it is not even needed any more?

-- 
Martin Waitz

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] gitweb: make leftmost column of blame less cluttered.

[Luben Tuikov]

--- Martin Waitz <tali@admingilde.org> wrote:
> hoi :)
> 
> On Tue, Oct 03, 2006 at 12:25:45AM -0700, Junio C Hamano wrote:
> > [PATCH] gitweb: make leftmost column of blame less cluttered.
> > 
> > Instead of labelling each and every line with clickable commit
> > object name, this makes the blame output to show them only on
> > the first line of each group of lines from the same revision.
> > 
> > Also it makes mouse-over to show the minimum authorship and
> > authordate information for extra cuteness ;-).
> 
> what about making the line itself clickable and adding the
> mouse-over there, too?
> Then you don't need to worry about repeating the commit id,
> and perhaps it is not even needed any more?

I like Junio's first patch on the subject with the only
objection that the "chunk" can be 100s of 1000s of lines
if the file is too big and there had never been any changes
since the initial commit.

I like the fact that the "data part" of blame is text, and
that the commit-8 is on the left, and color-chunked.  Sometimes
people simply _remember_ a number of commit-8's and thus the
layout of blame is intentional, since they can look to the left
and recognize a commit-8...

    Luben

Re: [PATCH] gitweb: make leftmost column of blame less cluttered.

[Junio C Hamano]

Luben Tuikov <ltuikov@yahoo.com> writes:

>> Then you don't need to worry about repeating the commit id,
>> and perhaps it is not even needed any more?

I am contradicting myself but I personally like what Pasky runs
at http://repo.or.cz/ too, although it goes to the other extreme
to spend more space for line-by-line annotation ;-)

> I like Junio's first patch on the subject with the only
> objection that the "chunk" can be 100s of 1000s of lines
> if the file is too big and there had never been any changes
> since the initial commit.
>
> I like the fact that the "data part" of blame is text, and
> that the commit-8 is on the left, and color-chunked.  Sometimes
> people simply _remember_ a number of commit-8's and thus the
> layout of blame is intentional, since they can look to the left
> and recognize a commit-8...

It is not only the initial commit.  A substantial rewrite and
new development also has the same issue.

I think you are also contradicting yourself by saying that
people would recognize a commit-8, and at the same time you do
not like the chunk code that makes sure you do not get too few
of them.  If people _do_ recognize commit-8 (I seriously doubt
that), then wouldn't it help to make sure you have them on every
couple-dozen lines so that the user would see the familiar one
even when scrolled?

Re: [PATCH] gitweb: make leftmost column of blame less cluttered.

[Luben Tuikov]

--- Junio C Hamano <junkio@cox.net> wrote:
> > I like the fact that the "data part" of blame is text, and
> > that the commit-8 is on the left, and color-chunked.  Sometimes
> > people simply _remember_ a number of commit-8's and thus the
> > layout of blame is intentional, since they can look to the left
> > and recognize a commit-8...
> 
> It is not only the initial commit.  A substantial rewrite and
> new development also has the same issue.
> 
> I think you are also contradicting yourself by saying that
> people would recognize a commit-8, and at the same time you do
> not like the chunk code that makes sure you do not get too few
> of them.  If people _do_ recognize commit-8 (I seriously doubt
> that), then wouldn't it help to make sure you have them on every
> couple-dozen lines so that the user would see the familiar one
> even when scrolled?

It is not that I don't like it.  For example if we didn't have
the block-per-commit-coloring, then we'd make use of this, but it
seems that the block-per-commit-coloring exists for the purpose to
show conglomerations of same-commit lines, thus obviating the need
to repeat it (commit-8) every so many lines.

The other question is how many lines should the repeat-chunk be?

In my case I'd like to set it to infinity, since the
block-per-commit-coloring gives me the same information.

The other extreme case is set it to 1, thus obviating the need to
use block-per-commit-coloring.

The middle ground as it seems to me, neither infinity nor 1, is
to just use the block-per-commit-coloring and use your idea of printing
the commit-8 only on the leading block row with mouse-over author
and date info. That's an excellent idea.*

    Luben
* I've three patches which implement your excellent idea but without
using a "stack-like" chunk, eliminating the concern of rare but present
files with 100s of 1000s of lines with only an initial commit.  Should I
post them?

Re: [PATCH] gitweb: make leftmost column of blame less cluttered.

[Jakub Narebski]

Luben Tuikov wrote:

> --- Junio C Hamano <junkio@cox.net> wrote:
>> > I like the fact that the "data part" of blame is text, and
>> > that the commit-8 is on the left, and color-chunked.  Sometimes
>> > people simply _remember_ a number of commit-8's and thus the
>> > layout of blame is intentional, since they can look to the left
>> > and recognize a commit-8...
>> 
>> It is not only the initial commit.  A substantial rewrite and
>> new development also has the same issue.
>> 
>> I think you are also contradicting yourself by saying that
>> people would recognize a commit-8, and at the same time you do
>> not like the chunk code that makes sure you do not get too few
>> of them.  If people _do_ recognize commit-8 (I seriously doubt
>> that), then wouldn't it help to make sure you have them on every
>> couple-dozen lines so that the user would see the familiar one
>> even when scrolled?
> 
> It is not that I don't like it.  For example if we didn't have
> the block-per-commit-coloring, then we'd make use of this, but it
> seems that the block-per-commit-coloring exists for the purpose to
> show conglomerations of same-commit lines, thus obviating the need
> to repeat it (commit-8) every so many lines.

Colors repeat. They are not enough.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH] gitweb: make leftmost column of blame less cluttered.

[Junio C Hamano]

Luben Tuikov <ltuikov@yahoo.com> writes:

> It is not that I don't like it.  For example if we didn't have
> the block-per-commit-coloring, then we'd make use of this, but it
> seems that the block-per-commit-coloring exists for the purpose to
> show conglomerations of same-commit lines, thus obviating the need
> to repeat it (commit-8) every so many lines.
>
> The other question is how many lines should the repeat-chunk be?
>
> In my case I'd like to set it to infinity, since the
> block-per-commit-coloring gives me the same information.

Not that I particularly care that deeply, but my intention was:

 (0) having commit-8 on every line was distracting;

 (1) grouping clue comes solely from the zebra shading;
     placement of commit-8 does not have anything to do with the
     grouping.  It was just that the old interface had N
     commit-8 links for a group that consists of N lines.  I
     made it to approximately N/20 links, and you are in favor
     of having 1 link per group.

 (2) commit-8 is the only visually obviously clickable thing to
     get to the commit.  Having them only on the first line in
     the group means for a large group it would scroll off the
     top after reading all the lines in the group through to
     decide it is worth inspecting; think of a case where one
     commit added 80 lines of code and each line is shown in
     14-dot high font on 1024x768 display which would give you
     50 lines or so at most.

View blame output of README from git.git and scoll down to
around line 400 to see what I mean, with an window that is
around 30 lines.  Jon's ASCII art extends for about 40 lines or
so.

Put it another way, it is a redundancy just like the same set of
links people place at the top and the bottom of a page.  It is
redundant in the sense that you can always scroll to either one,
but having closer to the mouse pointer helps accessing them.  It
is different from the other redundancy we love to hate ;-) that
the shortlog page had -- commit link and the clickable commit
title were immediately next to each other and there was no
argument for that redundancy from accessibility point of view
(the argument was purely "is the clickable commit title obvious
enough?").

> The middle ground as it seems to me, neither infinity nor 1, is
> to just use the block-per-commit-coloring and use your idea of printing
> the commit-8 only on the leading block row with mouse-over author
> and date info. That's an excellent idea.*

I am not quite sure what you mean by this.

Re: [PATCH] gitweb: make leftmost column of blame less cluttered.

[Luben Tuikov]

--- Junio C Hamano <junkio@cox.net> wrote:
>  (1) grouping clue comes solely from the zebra shading;
>      placement of commit-8 does not have anything to do with the
>      grouping.  It was just that the old interface had N
>      commit-8 links for a group that consists of N lines.  I
>      made it to approximately N/20 links, and you are in favor
>      of having 1 link per group.

Yes, either 1 or infinity is fine with me.

>  (2) commit-8 is the only visually obviously clickable thing to
>      get to the commit.  Having them only on the first line in
>      the group means for a large group it would scroll off the
>      top after reading all the lines in the group through to
>      decide it is worth inspecting; think of a case where one
>      commit added 80 lines of code and each line is shown in
>      14-dot high font on 1024x768 display which would give you
>      50 lines or so at most.

Yes, I completely understand the problem you're describing
here and have seen even more extreme examples of it.

Consider the general common case where files have had
commits since the initital commit and have various blocks-per-commit.
The eye gets accomodated to seeing one leading commit-8 _per commit block_.

All of a sudden now, it would also see "leading" commit-8
every arbitrary-N rows, leading the brain to believe that this
is a commit-block, while it is not.

This breaks this learned assumption, and makes it confusing
if this is a block-commit or if it is the _previous_ commit.

> Put it another way, it is a redundancy just like the same set of
> links people place at the top and the bottom of a page.  It is
> redundant in the sense that you can always scroll to either one,
> but having closer to the mouse pointer helps accessing them.  It

I use PgUp in this case.

> > The middle ground as it seems to me, neither infinity nor 1, is
> > to just use the block-per-commit-coloring and use your idea of printing
> > the commit-8 only on the leading block row with mouse-over author
> > and date info. That's an excellent idea.*
> 
> I am not quite sure what you mean by this.

Well, you introduced this paramenter N to decide how large the
limit is where one needs to print another redundant commit-8.  Yes?

Analyse N and its impact on output:
  1) N approaches infinity (extreme case)
  2) N = 1 (extreme case)
  3) N = something in between (20 is what you had selected)

1) When N approaces infinity, we get the behaviour that there is
no redunant printing of the commit-8 every N rows, since one cannot
have infinite rows. (i.e. N doesn't equal any positive integer)
The commit-8 is printed only on the first row of the commit-block.

2) When N is set to 1, we get the old behaviour, where the same
commit-8 is printed on every line of the commit-block.  The redundancy
is of order the number of rows of the commit-block.  The extremes
are:
  i) Same commit-8 on every line of the file, meaning there were no
     commits since the inital commit.
  ii) Every line was touched by a commit which didn't touch any
     other line. Every line has a different commit.
In both extremes we get a commit-8 per line.

3) Set N to some integer, 1 < N. The question is what should that
integer be?

That's a very interesting question.

Suppose you set N to some positive integer k > 1.

WLG, consider a text file of at least 4*k lines, with 3 commits,
such that commit 1 touches lines 0 to k-1, commit 2 touches
lines k to 3*k-1, and commit 3 touches lines 3k to 4*k-1.

Here is a picture, wlg:

1 : line 0
    ...
    line k-1
2 : line k
    ...
    line 2k-1
2 : line 2k
    ...
    line 3k-1
3:  line 3k
    ...
    line 4k-1

Notice that commit 2 is printed at line 2k as well as line k.
This is inconsistent with the assumption that a leading
commit-8 is different than the previous one, and introduces
a new commit-block.

That is, there is no good arbitration for N other than 1 or
infinity, such that the brain doesn't need to break the assumption
that a leading commit introduces a block of different commit than
the previous. And this is indeed what the "zebra" coloring is all
about.

The more extreme case is where you'd have a very large file with
many large commits touching an integer line of lines multiple of k,
such that the smallest commit-block is at least 2k lines.

Then you'd get this confusing patchwork of zebra coloring and in
each block you'd get at least two commit-8 which are the same,
confusing if it is different than the previous or not, since
commit-8s appear at least every k lines.

All I'm saying is that I'd rather keep the leading commit-8 consistent
with the "zebra" coloring.

    Luben

Re: [PATCH] gitweb: make leftmost column of blame less cluttered.

[Junio C Hamano]

Luben Tuikov <ltuikov@yahoo.com> writes:

> Yes, I completely understand the problem you're describing
> here and have seen even more extreme examples of it.

I agree with your conclusion.  More than one commit-8 placed in
a large group end up visually suggesting that they are
separating the group into multiple pieces that have some
meaning, which is not, because the contrast of zebra shading
between groups is very subtle and is much weaker than commit-8
"clickable handles".

I am not suggesting to make the contrast of shading stronger;
that would make things harder to read, and readability matters
more on that page.

Will revert mine from "next" (along with the mistakenly dropped
executable bit) and replace with your patch 1 and 2.

Re: [PATCH] gitweb: make leftmost column of blame less cluttered.

[Luben Tuikov]

--- Junio C Hamano <junkio@cox.net> wrote:
> Luben Tuikov <ltuikov@yahoo.com> writes:
> 
> > Yes, I completely understand the problem you're describing
> > here and have seen even more extreme examples of it.
> 
> I agree with your conclusion.  More than one commit-8 placed in
> a large group end up visually suggesting that they are
> separating the group into multiple pieces that have some
> meaning, which is not, because the contrast of zebra shading
> between groups is very subtle and is much weaker than commit-8
> "clickable handles".
> 
> I am not suggesting to make the contrast of shading stronger;
> that would make things harder to read, and readability matters
> more on that page.
> 
> Will revert mine from "next" (along with the mistakenly dropped
> executable bit) and replace with your patch 1 and 2.

Sounds great -- thanks!

   Luben