threads / patch / 4467

patchgitweb: Adding a `blame' interface.

Subject: [PATCH] gitweb: Adding a `blame' interface.

## tl;dr

19 messages between Jun 11, 2006 and Jun 15, 2006. Diffs are folded; open one to read it.

replies: 18people: 9as markdown or json

Florian Forster· Jun 11, 2006, 15:45 UTC · lore

This patch adds an interface for `git-blame' to `gitweb.cgi'. Links to it are place in `git_blob'. Internally the code uses `git-annotate' because `git-blame's output differs for files that have been renamed in the past. However, I like the term `blame' better.

Signed-off-by: Florian Forster <octo@verplant.org>
---
 gitweb/gitweb.cgi |  108 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 107 insertions(+), 1 deletions(-)
b11522d270365b293197680e43e8feb87328a352
Show changes to gitweb/gitweb.cgi +107 −1
diff --git a/gitweb/gitweb.cgi b/gitweb/gitweb.cgi
index ea21fbe..91c075d 100755
--- a/gitweb/gitweb.cgi
+++ b/gitweb/gitweb.cgi
@@ -203,6 +203,9 @@ if (!defined $action || $action eq "summ
 } elsif ($action eq "tag") {
 	git_tag();
 	exit;
+} elsif ($action eq "blame") {
+	git_blame();
+	exit;
 } else {
 	undef $action;
 	die_error(undef, "Unknown action.");
@@ -1228,6 +1231,107 @@ sub git_tag {
 	git_footer_html();
 }
 
+sub git_blame {
+	my $fd;
+	die_error('404 Not Found', "What file will it be, master?") if (!$file_name);
+	$hash_base ||= git_read_head($project);
+	die_error(undef, "Reading commit failed.") unless ($hash_base);
+	my %co = git_read_commit($hash_base)
+		or die_error(undef, "Reading commit failed.");
+	if (!defined $hash) {
+		$hash = git_get_hash_by_path($hash_base, $file_name, "blob")
+			or die_error(undef, "Error lookup file.");
+	}
+	open ($fd, "-|", "$gitbin/git-annotate", '-l', '-t', '-r', $file_name, $hash_base)
+		or die_error(undef, "Open failed.");
+	git_header_html();
+	print "<div class=\"page_nav\">\n" .
+		$cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=summary")}, "summary") .
+		" | " . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=shortlog")}, "shortlog") .
+		" | " . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=log")}, "log") .
+		" | " . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=commit;h=$hash_base")}, "commit") .
+		" | " . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=commitdiff;h=$hash_base")}, "commitdiff") .
+		" | " . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=tree;h=$co{'tree'};hb=$hash_base")}, "tree") . "<br/>\n";
+	print $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blob;h=$hash;hb=$hash_base;f=$file_name")}, "blob") .
+		" | " . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blame;f=$file_name")}, "head") . "<br/>\n";
+	print "</div>\n".
+		"<div>" .
+		$cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=commit;h=$hash_base"), -class => "title"}, esc_html($co{'title'})) .
+		"</div>\n";
+	print "<div class=\"page_path\"><b>" . esc_html($file_name) . "</b></div>\n";
+	print "<div class=\"page_body\">\n";
+	print <<HTML;
+<table style="border-collapse: collapse;">
+  <tr>
+    <th>Commit</th>
+    <th>Age</th>
+    <th>Author</th>
+    <th>Line</th>
+    <th>Data</th>
+  </tr>
+HTML
+	my @line_class = (qw(light dark));
+	my $line_class_len = scalar (@line_class);
+	my $line_class_num = $#line_class;
+	while (my $line = <$fd>) {
+		my $long_rev;
+		my $short_rev;
+		my $author;
+		my $time;
+		my $lineno;
+		my $data;
+		my $age;
+		my $age_str;
+		my $age_style;
+
+		chomp $line;
+		$line_class_num = ($line_class_num + 1) % $line_class_len;
+
+		if ($line =~ m/^([0-9a-fA-F]{40})\t\(\s*([^\t]+)\t(\d+) \+\d\d\d\d\t(\d+)\)(.*)$/) {
+			$long_rev = $1;
+			$author   = $2;
+			$time     = $3;
+			$lineno   = $4;
+			$data     = $5;
+		} else {
+			print qq(  <tr><td colspan="5" style="color: red; background-color: yellow;">Unable to parse: $line</td></tr>\n);
+			next;
+		}
+		$short_rev  = substr ($long_rev, 0, 8);
+		$age        = time () - $time;
+		$age_str    = age_string ($age);
+		$age_str    =~ s/ /&nbsp;/g;
+		$age_style  = 'font-style: italic;';
+		$age_style .= ' color: #009900; background: transparent;' if ($age < 60*60*24*2);
+		$age_style .= ' font-weight: bold;' if ($age < 60*60*2);
+		$author     = esc_html ($author);
+		$author     =~ s/ /&nbsp;/g;
+		# escape tabs
+		while ((my $pos = index($data, "\t")) != -1) {
+			if (my $count = (8 - ($pos % 8))) {
+				my $spaces = ' ' x $count;
+				$data =~ s/\t/$spaces/;
+			}
+		}
+		$data = esc_html ($data);
+		$data =~ s/ /&nbsp;/g;
+
+		print <<HTML;
+  <tr class="$line_class[$line_class_num]">
+    <td style="font-family: monospace;"><a href="$my_uri?${\esc_param ("p=$project;a=commit;h=$long_rev")}" class="text">$short_rev..</a></td>
+    <td style="$age_style">$age_str</td>
+    <td>$author</td>
+    <td style="text-align: right;"><a id="$lineno" href="#$lineno" class="linenr">$lineno</a></td>
+    <td style="font-family: monospace;">$data</td>
+  </tr>
+HTML
+	} # while (my $line = <$fd>)
+	print "</table>\n\n";
+	close $fd or print "Reading blob failed.\n";
+	print "</div>";
+	git_footer_html();
+}
+
 sub git_tags {
 	my $head = git_read_head($project);
 	git_header_html();
@@ -1375,7 +1479,8 @@ sub git_blob {
 		      " | " . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=commitdiff;h=$hash_base")}, "commitdiff") .
 		      " | " . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=tree;h=$co{'tree'};hb=$hash_base")}, "tree") . "<br/>\n";
 		if (defined $file_name) {
-			print $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blob_plain;h=$hash;f=$file_name")}, "plain") .
+			print $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blame;h=$hash;hb=$hash_base;f=$file_name")}, "blame") .
+			" | " . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blob_plain;h=$hash;f=$file_name")}, "plain") .
 			" | " . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blob;hb=HEAD;f=$file_name")}, "head") . "<br/>\n";
 		} else {
 			print $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blob_plain;h=$hash")}, "plain") . "<br/>\n";
@@ -1496,6 +1601,7 @@ sub git_tree {
 			      "</td>\n" .
 			      "<td class=\"link\">" .
 			      $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blob;h=$t_hash$base_key;f=$base$t_name")}, "blob") .
+#			      " | " . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blame;h=$t_hash$base_key;f=$base$t_name")}, "blame") .
 			      " | " . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=history;h=$hash_base;f=$base$t_name")}, "history") .
 			      "</td>\n";
 		} elsif ($t_type eq "tree") {
-- 
1.3.3
Martin Langhoff· Jun 11, 2006, 22:02 UTC · re: Florian Forster · lore

Re: [PATCH] gitweb: Adding a `blame' interface.

Florian,

Looks good! git-blame/git-annotate are quite expensive to run. Do you think it would make sense making it conditional on a git-repo-config option (gitweb.blame=1)?

kernel.org is the flagship user for gitweb, so expensive options should default to off :-/

cheers,
martin
Florian Forster· Jun 12, 2006, 08:24 UTC · re: Martin Langhoff · lore

Re: [PATCH] gitweb: Adding a `blame' interface.

Hi Martin,
On Mon, Jun 12, 2006 at 10:02:05AM +1200, Martin Langhoff wrote:
> good! git-blame/git-annotate are quite expensive to run. Do you think
> it would make sense making it conditional on a git-repo-config option
> (gitweb.blame=1)?

sure, that it's a big change and if it helps the kernel.org folks ;) I'll follow-up with a patch for this in a second..

Would it help to cache `git-annotate's output, e.g. using one of the `Cache::Cache' modules? Or is browsing of blobs too sparse for this to result in a performance gain? I'm sure the modules could be integrated as a weak precondition.

I have two more points regarding gitweb's configuration:
- IMHO it would make sense to move the general gitweb-configuration
  (where are the repositories, where are the binaries, etc) out of the
  script.  As far as I know the Debian maintainer of the `gitweb'
  package has asked for this before but was refused for some reason..
  Possibly a file `gitweb.conf' in the same directory as the script
  could be read and overwrite the builtin defaults..?
- If `GIT_DIR/description' is only used by gitweb it may be more
  consistent to use the git-repo-config option `gitweb.description' in
  the future.

Regards, -octo

-- 
Florian octo Forster
Hacker in training
GnuPG: 0x91523C3D
http://verplant.org/
Florian Forster· Jun 12, 2006, 08:31 UTC · re: Florian Forster · lore

[PATCH] gitweb: Make the availability of the `blame' interface in gitweb configurable.

Since `git-annotate' is an expensive operation to run it may be desirable to deactivate this functionality. This patch introduces the `gitweb.blame' option to git-repo-config and disables the blame support by default.

Signed-off-by: Florian Forster <octo@verplant.org>
---
 gitweb/gitweb.cgi |   27 +++++++++++++++++++++++++--
 1 files changed, 25 insertions(+), 2 deletions(-)
3eea23e8d8a13579455cdf8d5088794d33bdcba2
Show changes to gitweb/gitweb.cgi +25 −2
diff --git a/gitweb/gitweb.cgi b/gitweb/gitweb.cgi
index 91c075d..5eabe06 100755
--- a/gitweb/gitweb.cgi
+++ b/gitweb/gitweb.cgi
@@ -837,6 +837,25 @@ sub git_read_projects {
 	return @list;
 }
 
+sub git_get_project_config {
+	my $key = shift;
+
+	return unless ($key);
+	$key =~ s/^gitweb\.//;
+	return if ($key =~ m/\W/);
+
+	my $val = qx(git-repo-config --get gitweb.$key);
+	return ($val);
+}
+
+sub git_get_project_config_bool {
+	my $val = git_get_project_config (@_);
+	if ($val and $val =~ m/true|yes|on/) {
+		return (1);
+	}
+	return; # implicit false
+}
+
 sub git_project_list {
 	my @list = git_read_projects();
 	my @projects;
@@ -1233,6 +1252,7 @@ sub git_tag {
 
 sub git_blame {
 	my $fd;
+	die_error('403 Permission denied', "Permission denied.") if (!git_get_project_config_bool ('blame'));
 	die_error('404 Not Found', "What file will it be, master?") if (!$file_name);
 	$hash_base ||= git_read_head($project);
 	die_error(undef, "Reading commit failed.") unless ($hash_base);
@@ -1468,6 +1488,7 @@ sub git_blob {
 		my $base = $hash_base || git_read_head($project);
 		$hash = git_get_hash_by_path($base, $file_name, "blob") || die_error(undef, "Error lookup file.");
 	}
+	my $have_blame = git_get_project_config_bool ('blame');
 	open my $fd, "-|", "$gitbin/git-cat-file blob $hash" or die_error(undef, "Open failed.");
 	git_header_html();
 	if (defined $hash_base && (my %co = git_read_commit($hash_base))) {
@@ -1479,8 +1500,10 @@ sub git_blob {
 		      " | " . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=commitdiff;h=$hash_base")}, "commitdiff") .
 		      " | " . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=tree;h=$co{'tree'};hb=$hash_base")}, "tree") . "<br/>\n";
 		if (defined $file_name) {
-			print $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blame;h=$hash;hb=$hash_base;f=$file_name")}, "blame") .
-			" | " . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blob_plain;h=$hash;f=$file_name")}, "plain") .
+			if ($have_blame) {
+				print $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blame;h=$hash;hb=$hash_base;f=$file_name")}, "blame") .  " | ";
+			}
+			print $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blob_plain;h=$hash;f=$file_name")}, "plain") .
 			" | " . $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blob;hb=HEAD;f=$file_name")}, "head") . "<br/>\n";
 		} else {
 			print $cgi->a({-href => "$my_uri?" . esc_param("p=$project;a=blob_plain;h=$hash")}, "plain") . "<br/>\n";
-- 
1.3.3
Martin Langhoff· Jun 12, 2006, 08:34 UTC · re: Florian Forster · lore

Re: [PATCH] gitweb: Adding a `blame' interface.

On 6/12/06, Florian Forster <octo@verplant.org> wrote:
Show 7 quoted lines
> On Mon, Jun 12, 2006 at 10:02:05AM +1200, Martin Langhoff wrote:
> > good! git-blame/git-annotate are quite expensive to run. Do you think
> > it would make sense making it conditional on a git-repo-config option
> > (gitweb.blame=1)?
>
> sure, that it's a big change and if it helps the kernel.org folks ;)
> I'll follow-up with a patch for this in a second..

That'd be great. I am looking into integrating other feature patches too (like tarball downloads) that are useful but costly, making them conditional too...

> Would it help to cache `git-annotate's output, e.g. using one of the

I think we can rely on proxies doing good caching -- a busy host like kernel.org will have big reverse proxies in front. A git-blame for a given file+commitsha doesn't change, so we can give it a long cache time, like... forever ;-)

Show 5 quoted lines
> I have two more points regarding gitweb's configuration:
> - IMHO it would make sense to move the general gitweb-configuration
>   (where are the repositories, where are the binaries, etc) out of the
>   script.  As far as I know the Debian maintainer of the `gitweb'
>   package has asked for this before but was refused for some reason..

Sounds like a reasonable request. I would make it rely on env vars, $ENV{GITWEB_CONFIG} can generally point to /etc/gitweb.conf, and that would override the config values we have.

This is trivial, and it means we buy a lot of flexibility from apache's httpd.conf being able to point to different config files depending on arbitrarty conditions.

BTW, I haven't seen the debian maintainer's request, was that on the list?
> - If `GIT_DIR/description' is only used by gitweb it may be more
>   consistent to use the git-repo-config option `gitweb.description' in
>   the future.

Not sure how git-repo configurations deal with long entries. Right now the description may contain html for instance.

martin
Shawn Pearce· Jun 12, 2006, 08:40 UTC · re: Martin Langhoff · lore

Re: [PATCH] gitweb: Adding a `blame' interface.

Martin Langhoff <martin.langhoff@gmail.com> wrote:
Show 6 quoted lines
> >- If `GIT_DIR/description' is only used by gitweb it may be more
> >  consistent to use the git-repo-config option `gitweb.description' in
> >  the future.
> 
> Not sure how git-repo configurations deal with long entries. Right now
> the description may contain html for instance.
It has to be escaped, which could be ugly with HTML.  For example:
  [gitweb]
    description=<div class=\"description\">\n\
This is a chunk of text which describes this repository.  Some\n\
of this text might be rather long, and might need many lines to\n\
really be able to describe the repository in a nice editor such as\n\
vi running in an 80 character wide xterm.\n\
</div>

Forget a \ in front of a double quote (") or an LF and the entry is corrupt. So as nice as it sounds it might not be the best way to obtain a description for gitweb.

-- 
Shawn.
Johannes Schindelin· Jun 12, 2006, 09:08 UTC · re: Shawn Pearce · lore

Re: [PATCH] gitweb: Adding a `blame' interface.

Hi,
On Mon, 12 Jun 2006, Shawn Pearce wrote:
Show 7 quoted lines
>   [gitweb]
>     description=<div class=\"description\">\n\
> This is a chunk of text which describes this repository.  Some\n\
> of this text might be rather long, and might need many lines to\n\
> really be able to describe the repository in a nice editor such as\n\
> vi running in an 80 character wide xterm.\n\
> </div>
AFAIK the trailing "\" will not work.

Ciao, Dscho

Shawn Pearce· Jun 12, 2006, 09:19 UTC · re: Johannes Schindelin · lore

Re: [PATCH] gitweb: Adding a `blame' interface.

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
Show 13 quoted lines
> Hi,
> 
> On Mon, 12 Jun 2006, Shawn Pearce wrote:
> 
> >   [gitweb]
> >     description=<div class=\"description\">\n\
> > This is a chunk of text which describes this repository.  Some\n\
> > of this text might be rather long, and might need many lines to\n\
> > really be able to describe the repository in a nice editor such as\n\
> > vi running in an 80 character wide xterm.\n\
> > </div>
> 
> AFAIK the trailing "\" will not work.

Actually it does. I figured out that it works (and why it works) when I implemented the GIT repository parser in Java for my pure Java version of GIT...

For example:
  [spearce@spearce-pb15 bob]$ cat .git/config 
  [core]
          repositoryformatversion = 0
          filemode = true
  [gitweb]
          description = This is a very\nlong line to put into GIT\n\
  repo config.\n\
  I hope it works.
          on = true
  [spearce@spearce-pb15 bob]$ git repo-config gitweb.description
  This is a very
  long line to put into GIT
  repo config.
  I hope it works.
  [spearce@spearce-pb15 bob]$ git repo-config gitweb.on
  true

The use of a trailing \ makes sense; the collapsing of multiple spaces into one space unless quoted inside of "" doesn't. But whatever...

-- 
Shawn.
Florian Forster· Jun 12, 2006, 18:11 UTC · re: Martin Langhoff · lore

Re: gitweb: Config file support (was: Adding a `blame' interface.)

Hi Martin,
On Mon, Jun 12, 2006 at 08:34:43PM +1200, Martin Langhoff wrote:
> > As far as I know the Debian maintainer of the `gitweb' package has
> > asked for this before but was refused for some reason..
> BTW, I haven't seen the debian maintainer's request, was that on the list?

Yes, it was a mail by Andres Salomon on May 20th, 2005 with the subject `add conf file support to gitweb'. A friend of mine asked him if he had sent the patch upstream and he pointed to this message and explained he had gotten a private reply saying that gitweb `only covers the special needs on kernel.org'.

Regards, -octo

-- 
Florian octo Forster
Hacker in training
GnuPG: 0x91523C3D
http://verplant.org/
Linus Torvalds· Jun 12, 2006, 14:59 UTC · re: Florian Forster · lore

Re: [PATCH] gitweb: Adding a `blame' interface.

On Mon, 12 Jun 2006, Florian Forster wrote:
Show 5 quoted lines
> 
> Would it help to cache `git-annotate's output, e.g. using one of the
> `Cache::Cache' modules? Or is browsing of blobs too sparse for this to
> result in a performance gain? I'm sure the modules could be integrated
> as a weak precondition.

The apache setup at least on kernel.org is already set up to do caching, as long as the generated headers for the page allow it in the first place.

So caching inside gitweb is generally pointless, at least when it's at the level of one result page. At a higher level, if the internal caching might improve performance of _other_ pages because it caches the result of some intermediate important thing, it might be a different issue.

		Linus
Florian Forster· Jun 12, 2006, 17:57 UTC · re: Linus Torvalds · lore

Re: [PATCH] gitweb: Supporting caches (was: Adding a `blame' interface.)

On Mon, Jun 12, 2006 at 07:59:39AM -0700, Linus Torvalds wrote:
> The apache setup at least on kernel.org is already set up to do
> caching, as long as the generated headers for the page allow it in the
> first place.

I've actually looked into improving native HTTP caching (mostly for small site without revers proxying) by providing a `Last-Modified' header where possible and sending a `304 Not Modified' whenever appropriate.

While it doesn't sound hard it's next to impossible: A commit's timestamp doesn't change when head a points to it (or does not longer point to it). Also displaying the timestamps as `Modified xy {seconds,minutes, hours,...} ago' possess a big problem.

(I guess the webserver could use the `If-Modified-Since' header to check if the displayed time needs to be updated, but if you ask me it's not worth the effort.)

In short, the `blob', `blob_plain', and `blobdiff' pages could profit from that because they don't display the head(s) pointing to the current commit. On the other hand, this is a little inconsistent and could be considered a bug. So I'll give up on that unless someone has a great idea how to handle this.

Regards, -octo

-- 
Florian octo Forster
Hacker in training
GnuPG: 0x91523C3D
http://verplant.org/
Jon Loeliger· Jun 12, 2006, 21:29 UTC · re: Florian Forster · lore

Re: [PATCH] gitweb: Adding a `blame' interface.

On Mon, 2006-06-12 at 03:24, Florian Forster wrote:
Show 7 quoted lines
> I have two more points regarding gitweb's configuration:
> - IMHO it would make sense to move the general gitweb-configuration
>   (where are the repositories, where are the binaries, etc) out of the
>   script.  As far as I know the Debian maintainer of the `gitweb'
>   package has asked for this before but was refused for some reason..
>   Possibly a file `gitweb.conf' in the same directory as the script
>   could be read and overwrite the builtin defaults..?
I already submitted a patch down this line on 22-Mar-2006:
    http://marc.theaimsgroup.com/?l=git&m=114308224922372&w=2

Thanks, jdl

Junio C Hamano· Jun 14, 2006, 20:27 UTC · re: Martin Langhoff · lore

Re: [PATCH] gitweb: Adding a `blame' interface.

"Martin Langhoff" <martin.langhoff@gmail.com> writes:
Show 8 quoted lines
> Florian,
>
> Looks good! git-blame/git-annotate are quite expensive to run. Do you
> think it would make sense making it conditional on a git-repo-config
> option (gitweb.blame=1)?
>
> kernel.org is the flagship user for gitweb, so expensive options
> should default to off :-/
Seconded.  Thanks Florian and Martin.
Fredrik Kuivinen· Jun 12, 2006, 21:38 UTC · re: Florian Forster · lore

Re: [PATCH] gitweb: Adding a `blame' interface.

On Sun, Jun 11, 2006 at 05:45:19PM +0200, Florian Forster wrote:
Show 6 quoted lines
> This patch adds an interface for `git-blame' to `gitweb.cgi'. Links to it are
> place in `git_blob'.
> Internally the code uses `git-annotate' because `git-blame's output differs for
> files that have been renamed in the past. However, I like the term `blame'
> better.
> 

You can pass "--compatibility" to git-blame to get output which is identical to git-annotates output. However, "--time" is not implemented in git-blame yet. I will send a patch in a separate mail.

git-blame is a bit faster than git-annotate and, as far as I know, it produces output which is correct.

- Fredrik
Johannes Schindelin· Jun 12, 2006, 22:42 UTC · re: Fredrik Kuivinen · lore

Re: [PATCH] gitweb: Adding a `blame' interface.

Hi,
On Mon, 12 Jun 2006, Fredrik Kuivinen wrote:
> git-blame is a bit faster than git-annotate and, as far as I know, it 
> produces output which is correct.
Yeah: Bring It On(tm)! I already waited for ages for this war to begin!

Ciao, Dscho

Linus Torvalds· Jun 12, 2006, 22:49 UTC · re: Johannes Schindelin · lore

Re: [PATCH] gitweb: Adding a `blame' interface.

On Tue, 13 Jun 2006, Johannes Schindelin wrote:
Show 7 quoted lines
> 
> On Mon, 12 Jun 2006, Fredrik Kuivinen wrote:
> 
> > git-blame is a bit faster than git-annotate and, as far as I know, it 
> > produces output which is correct.
> 
> Yeah: Bring It On(tm)! I already waited for ages for this war to begin!

Sadly, I don't think either of you can really do much about the fact that annotate/blame is simply the wrong model for git.

The war _I_d like to see is the GUI thing which does the "show when this section changed last" by following the history down only so far that the selected section shows up in the diff against the most current thing.

THAT is what I want. It also fits the git model much better, since you generally don't have to go back all the way.

Somebody? "Here's a nickel saying you can't do it!" (if somebody needs the motivation ;)

		Linus
Johannes Schindelin· Jun 12, 2006, 22:55 UTC · re: Linus Torvalds · lore

Re: [PATCH] gitweb: Adding a `blame' interface.

Hi,
On Mon, 12 Jun 2006, Linus Torvalds wrote:
Show 15 quoted lines
> On Tue, 13 Jun 2006, Johannes Schindelin wrote:
> > 
> > On Mon, 12 Jun 2006, Fredrik Kuivinen wrote:
> > 
> > > git-blame is a bit faster than git-annotate and, as far as I know, it 
> > > produces output which is correct.
> > 
> > Yeah: Bring It On(tm)! I already waited for ages for this war to begin!
> 
> Sadly, I don't think either of you can really do much about the fact that 
> annotate/blame is simply the wrong model for git.
> 
> The war _I_d like to see is the GUI thing which does the "show when this 
> section changed last" by following the history down only so far that the 
> selected section shows up in the diff against the most current thing.

But this is just the next step! Nothing prevents you -- once everybody agrees that blame/annotate does the right thing -- to restrict the lines of interest. And AFAICT both blame and annotate are good at stopping when all lines are accounted for.

Ciao, Dscho

Linus Torvalds· Jun 12, 2006, 23:21 UTC · re: Johannes Schindelin · lore

Re: [PATCH] gitweb: Adding a `blame' interface.

On Tue, 13 Jun 2006, Johannes Schindelin wrote:
Show 9 quoted lines
> > 
> > The war _I_d like to see is the GUI thing which does the "show when this 
> > section changed last" by following the history down only so far that the 
> > selected section shows up in the diff against the most current thing.
> 
> But this is just the next step! Nothing prevents you -- once everybody 
> agrees that blame/annotate does the right thing -- to restrict the lines 
> of interest. And AFAICT both blame and annotate are good at stopping when 
> all lines are accounted for.

You misunderstand. It's not "all", it's "any" (in fact, it would be even better if it would be on a byte-range basis, not on a line-based diff basis).

And it needs the GUI to make it useful, because nobody in their right mind will say "git showchange fs/inode.c 77-89" (and then do it iteratively if the first one wasn't actually the interesting case).

		Linus
Marco Costalba· Jun 15, 2006, 19:46 UTC · re: Linus Torvalds · lore

Re: [PATCH] gitweb: Adding a `blame' interface.

On 6/13/06, Linus Torvalds <torvalds@osdl.org> wrote:
Show 18 quoted lines
>
>
> On Tue, 13 Jun 2006, Johannes Schindelin wrote:
> >
> > On Mon, 12 Jun 2006, Fredrik Kuivinen wrote:
> >
> > > git-blame is a bit faster than git-annotate and, as far as I know, it
> > > produces output which is correct.
> >
> > Yeah: Bring It On(tm)! I already waited for ages for this war to begin!
>
> Sadly, I don't think either of you can really do much about the fact that
> annotate/blame is simply the wrong model for git.
>
> The war _I_d like to see is the GUI thing which does the "show when this
> section changed last" by following the history down only so far that the
> selected section shows up in the diff against the most current thing.
>

Probably I have misunderstood the request, but in qgit you can (from about one month ago) mouse select some lines in file content then press the filter button and see only the revisions that modify the selected text.

Also selected text is highlighted so to better view differences among revisions.

It is not clear to me in what the requested feature differs from this implementation.

    Marco

← back to recent threads