blameview and file line number

blameview and file line number

[Aneesh Kumar]

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

Is it a typo or intentional ? I found the blameview output confusing.

-aneesh

[-- Attachment #2: blameview.diff --]
[-- Type: text/x-patch, Size: 437 bytes --]

diff --git a/contrib/blameview/blameview.perl b/contrib/blameview/blameview.perl
index a55f799..e8bcb1b 100755
--- a/contrib/blameview/blameview.perl
+++ b/contrib/blameview/blameview.perl
@@ -62,7 +62,7 @@ sub flush_blame_line {
 	for(my $i = 0; $i < $cnt; $i++) {
 		@{$fileview->{data}->[$lno+$i-1]}[0,1,2] =
 		    (substr($commit, 0, 8), $info,
-		     $filename . ':' . ($s_lno+$i));
+		     $filename . ':' . ($lno+$i));
 	}
 }
 

Re: blameview and file line number

[Junio C Hamano]

"Aneesh Kumar" <aneesh.kumar@gmail.com> writes:

> Is it a typo or intentional ? I found the blameview output confusing.

Compare it with gitweb or textual output (blame -n -f).

It is supposed to show the line number in the original file.
Using lno is obviously wrong, as we are not reinventing "cat -n"
here ;-).

Re: blameview and file line number

[Aneesh Kumar]

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

On 1/30/07, Junio C Hamano <junkio@cox.net> wrote:
> "Aneesh Kumar" <aneesh.kumar@gmail.com> writes:
>
> > Is it a typo or intentional ? I found the blameview output confusing.
>
> Compare it with gitweb or textual output (blame -n -f).
>
> It is supposed to show the line number in the original file.
> Using lno is obviously wrong, as we are not reinventing "cat -n"
> here ;-).

In that case the heading is wrong. It should be something other than
Filenum. How about the patch below. On top of the series i sent you
before.

-aneesh

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: blameview.diff --]
[-- Type: text/x-patch; name="blameview.diff", Size: 984 bytes --]

diff --git a/contrib/blameview/blameview.perl b/contrib/blameview/blameview.perl
index 8ad9bcf..9149e35 100755
--- a/contrib/blameview/blameview.perl
+++ b/contrib/blameview/blameview.perl
@@ -29,6 +29,7 @@ my $scrolled_window = Gtk2::ScrolledWindow->new;
 $window->add($scrolled_window);
 my $fileview = Gtk2::SimpleList->new(
     'Commit' => 'text',
+    'OrigLine' => 'text',
     'CommitInfo' => 'text',
     'FileLine' => 'text',
     'Data' => 'text'
@@ -50,7 +51,7 @@ open($fh, '-|', "git cat-file blob $hash:$fn")
 
 while(<$fh>) {
   chomp;
-  $fileview->{data}->[$.] = ['HEAD', '?', "$fn:$.", $_];
+  $fileview->{data}->[$.] = ['HEAD','?', '?', "$.", $_];
 }
 
 my $blame;
@@ -79,8 +80,8 @@ sub flush_blame_line {
 
 	for(my $i = 0; $i < $cnt; $i++) {
 		@{$fileview->{data}->[$lno+$i-1]}[0,1,2] =
-		    (substr($commit, 0, 8), $info,
-		     $filename . ':' . ($lno+$i));
+		    (substr($commit, 0, 8), 
+		     $filename . ':' . ($s_lno+$i), $info, ($lno+$i));
 	}
 }
 

Re: blameview and file line number

[Junio C Hamano]

"Aneesh Kumar" <aneesh.kumar@gmail.com> writes:

> In that case the heading is wrong. It should be something other than
> Filenum.

You mean "FileLine"?

>  my $fileview = Gtk2::SimpleList->new(
>      'Commit' => 'text',
> +    'OrigLine' => 'text',
>      'CommitInfo' => 'text',
>      'FileLine' => 'text',
>      'Data' => 'text'

I do not have strong feeling to defend what Jeff originally did,
especially as this is only a sample program, whose primary
purpose is to demonstrate how the incremental display can be
used.

Having said that, I think the behaviour of the original makes
quite a lot of sense.  HEAD commit starts out to be tentatively
blamed for everything, and the filename and the line number in
that HEAD commit are shown for everything at the beginning, and
as the processing progresses, the labels for the ones whose
truely guilty party are known are updated to show the guilty
commit, the filename from that guilty commit and the line number
in that guilty commit.  I think the label "FileLine" reflects
what it is showing quite well.

As Linus mentioned, the screen real estate is already wasted by
too much metainfomation.  Although I do not care too much about
the UI issue in it since this is only a sample program, showing
the line number for each line in the final image ($lno) to waste
more space feels doubly wrong.

By the way, telling git-gui to annotate revision.h with the
attached patch was fun to watch.

-- >8 --
[PATCH] Louder git-blame --incremental

This patch takes the ability for --incremental to monitor the
process of "HEAD starts out to be tentatively blamed for
everything, and then blame is passed onto the parents" to the
extreme.  As blame is passed on to parents, incremental output
gives the information for tentatively blamed commits.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 builtin-blame.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/builtin-blame.c b/builtin-blame.c
index 3033e9b..107524c 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -549,6 +549,8 @@ static void free_patch(struct patch *p)
 	free(p);
 }
 
+static void found_guilty_entry(struct blame_entry *ent, int final);
+
 /*
  * Link in a new blame entry to the scorebord.  Entries that cover the
  * same line range have been removed from the scoreboard previously.
@@ -682,13 +684,16 @@ static void split_blame(struct scoreboard *sb,
 		new_entry = xmalloc(sizeof(*new_entry));
 		memcpy(new_entry, &(split[1]), sizeof(struct blame_entry));
 		add_blame_entry(sb, new_entry);
+		found_guilty_entry(new_entry, 0);
 	}
-	else if (!split[0].suspect && !split[2].suspect)
+	else if (!split[0].suspect && !split[2].suspect) {
 		/*
 		 * The parent covers the entire area; reuse storage for
 		 * e and replace it with the parent.
 		 */
 		dup_entry(e, &split[1]);
+		found_guilty_entry(e, 0);
+	}
 	else if (split[0].suspect) {
 		/* me and then parent */
 		dup_entry(e, &split[0]);
@@ -696,10 +701,12 @@ static void split_blame(struct scoreboard *sb,
 		new_entry = xmalloc(sizeof(*new_entry));
 		memcpy(new_entry, &(split[1]), sizeof(struct blame_entry));
 		add_blame_entry(sb, new_entry);
+		found_guilty_entry(new_entry, 0);
 	}
 	else {
 		/* parent and then me */
 		dup_entry(e, &split[1]);
+		found_guilty_entry(e, 0);
 
 		new_entry = xmalloc(sizeof(*new_entry));
 		memcpy(new_entry, &(split[2]), sizeof(struct blame_entry));
@@ -1359,11 +1366,13 @@ static void write_filename_info(const char *path)
  * The blame_entry is found to be guilty for the range.  Mark it
  * as such, and show it in incremental output.
  */
-static void found_guilty_entry(struct blame_entry *ent)
+static void found_guilty_entry(struct blame_entry *ent, int final)
 {
-	if (ent->guilty)
-		return;
-	ent->guilty = 1;
+	if (final) {
+		if (ent->guilty)
+			return;
+		ent->guilty = 1;
+	}
 	if (incremental) {
 		struct origin *suspect = ent->suspect;
 
@@ -1385,6 +1394,7 @@ static void found_guilty_entry(struct blame_entry *ent)
 			printf("summary %s\n", ci.summary);
 			if (suspect->commit->object.flags & UNINTERESTING)
 				printf("boundary\n");
+			printf("tentative %s\n", final ? "yes" : "no");
 		}
 		write_filename_info(suspect->path);
 	}
@@ -1432,7 +1442,7 @@ static void assign_blame(struct scoreboard *sb, struct rev_info *revs, int opt)
 		/* Take responsibility for the remaining entries */
 		for (ent = sb->ent; ent; ent = ent->next)
 			if (!cmp_suspect(ent->suspect, suspect))
-				found_guilty_entry(ent);
+				found_guilty_entry(ent, 1);
 		origin_decref(suspect);
 
 		if (DEBUG) /* sanity */

Re: blameview and file line number

[Shawn O. Pearce]

Junio C Hamano <junkio@cox.net> wrote:
> As Linus mentioned, the screen real estate is already wasted by
> too much metainfomation.  Although I do not care too much about
> the UI issue in it since this is only a sample program, showing
> the line number for each line in the final image ($lno) to waste
> more space feels doubly wrong.

Actually including the final image line number is probably something
you want to do in a blame viewer.  When I rework git-gui's blame
UI I'm going to keep the original line number column, but ditch
everything else into some sort of cursor-following-floating window
(Linus' idea).

The reason is, I'll be looking at a line of code in a 5000 line
source file in Eclipse (or vi!) and want to know how it came to be.
I'll go open a blame, but now I have 5000 lines to scan through.

If there's line numbers and a scrollbar, I can binary search to
it relatively quickly.  If there's a text search function, sure I
could try to enter part of the symbol to match, but at that point
I might as well just enter the line number to jump to, especially
if the symbol appears a few times in that file.

So yes, the -L option to git-blame is *very* handy on the command
line.  But I think you already knew that...

I realize that git-gui's blame feature won't be used very often
by the really hard-core developers on this list (you know who you
are) as the command line is simply faster, easier to use, and more
powerful.  Most of the features in git-gui are being created for
people who are a tad bit afraid of a command line and prefer to
point their way through their life with a small rodent shaped device.
Those folks need something like a -L that they can make use of.

> By the way, telling git-gui to annotate revision.h with the
> attached patch was fun to watch.

Yes, especially with its current technicolor interface.  :-)

I just had to go run this, and aside from the rather horrible blame
interface in git-gui, I'm seeing that git-blame is producing data
faster than git-gui can really process it.  This causes the UI to
flash through huge batches of updates.  Probably would be much more
interesting if I disabled the fileevent handler for a few hundred
ms to let the UI catch up.  :-)

-- 
Shawn.

Re: blameview and file line number

[Jeff King]

On Tue, Jan 30, 2007 at 12:55:44PM +0530, Aneesh Kumar wrote:

> Is it a typo or intentional ? I found the blameview output confusing.

The original output was 0-based, so I suspect that line simply didn't
get updated when Junio changed the incremental format to be 1-based.

-Peff

Re: blameview and file line number

[Jeff King]

On Tue, Jan 30, 2007 at 09:06:50AM -0500, Jeff King wrote:

> On Tue, Jan 30, 2007 at 12:55:44PM +0530, Aneesh Kumar wrote:
> 
> > Is it a typo or intentional ? I found the blameview output confusing.
> 
> The original output was 0-based, so I suspect that line simply didn't
> get updated when Junio changed the incremental format to be 1-based.

Er, disregard this. Junio explained what's happening already.

-Peff